feat(view): add viewStyle preset to paginationNav and pageNumberLinks - #2731
Conversation
… for Bootstrap markup The viewStyle argument accepts "plain" (default), "bootstrap5", "bootstrap4", or "tailwind". Bootstrap presets emit the canonical <nav><ul class="pagination"><li class="page-item active" aria-current="page"> <span class="page-link">N</span></li> structure — active class on the <li> wrapper, <span> for the current page — so Bootstrap-styled apps no longer need a Replace() regex hack to move the active class off the anchor. Default remains "plain" to preserve today's output byte-for-byte. Fixes #2718 Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
![wheels-bot[bot]](https://avatars.githubusercontent.com/in/3657588?s=60&v=4){kind=small}
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This PR delivers the viewStyle preset feature cleanly: the happy-path implementation is correct, the commit is conventional, and the default "plain" path is untouched. I'm flagging two concerns worth addressing before merge — an encoding inconsistency in disabled nav links, and a thin Tailwind test — but neither is a blocker. Submitting as --comment.
Correctness
local.encode is extracted but never used for disabled nav link text
vendor/wheels/view/pagination.cfc, line ~658:
local.encode = StructKeyExists(arguments.subArgs, "encode") ? arguments.subArgs.encode : true;The value is resolved but then the disabled branch (line ~667) ignores it entirely:
if (arguments.isDisabled) {
return '<li class="page-item disabled"><span class="page-link">' & arguments.text & '</span></li>';
}The existing $paginationDisabledElement routes through $element(... encode = arguments.encode), which processes HTML encoding correctly. The new Bootstrap disabled path skips that step. For the common defaults ("First", "Previous", etc.) this is harmless, but if an app customises nav text to include HTML entities (e.g. « / »), those entities will render literally in the Bootstrap path and as characters in the plain path — a visible inconsistency. The fix is one line:
if (arguments.isDisabled) {
var safeText = (IsBoolean(local.encode) && local.encode)
? EncodeForHTML(arguments.text)
: arguments.text;
return '<li class="page-item disabled"><span class="page-link">' & safeText & '</span></li>';
}Same fix applies to the Tailwind disabled branch (<span class="pagination-disabled">).
Invalid viewStyle values produce silent garbled output
$renderPaginationPage line ~543:
default:
return local.label; // bare number, no wrapper at all$renderPaginationNav line ~636:
default:
return '<nav>' & local.infoHtml & local.items & '</nav>';Passing viewStyle="boostrap5" (typo) produces no <li> wrappers on individual page numbers but a full <nav> wrapper — structurally broken with no error. Given that local.useViewStyle = Len(arguments.viewStyle) && arguments.viewStyle != "plain" already filters before the switch, the default branches represent genuinely unknown values. A Throw(type="Wheels.InvalidViewStyle", message="Unknown viewStyle: #arguments.viewStyle#") in the defaults would catch typos at development time. Not a hard requirement, but the silent path is surprising.
Tests
Tailwind pageNumberLinks spec is too weak
vendor/wheels/tests/specs/view/paginationHelpersSpec.cfc, line 249–254:
it("emits Tailwind markup with aria-current on current page", () => {
g.model("author").findAll(page = 2, perPage = 3, order = "lastName")
result = _controller.pageNumberLinks(viewStyle = "tailwind")
expect(result).toInclude('aria-current="page"')
expect(result).toInclude("2")
})This passes even if the Tailwind preset accidentally emits Bootstrap markup (which also contains aria-current="page"). The spec should assert on the Tailwind-specific wrappers:
expect(result).toInclude('<span class="pagination-current" aria-current="page">')
expect(result).toInclude('<a class="pagination-link"')Missing paginationNav coverage for Tailwind and Bootstrap 4
The three paginationNav viewStyle specs (lines 273–302) only exercise bootstrap5. There is no test for:
viewStyle="tailwind"inpaginationNav(different outer wrapper<nav ... class="pagination">, no<ul>)viewStyle="bootstrap4"disabled state inpaginationNav(should be the same as BS5 except noaria-current, but worth pinning)showInfo=truewith any viewStyle preset (the info string lands between<nav>and<ul>in the Bootstrap path — easy to inadvertently break)
Docs
CLAUDE.md § "Pagination View Helpers" still shows the helpers without viewStyle. The PR checklist correctly defers this to bot-update-docs.yml; acknowledged, no action needed here.
Commits
Single commit: feat(view): add viewStyle preset to paginationNav and pageNumberLinks for Bootstrap markup — type feat, scope view, subject 77 chars, not ALL-CAPS, DCO Signed-off-by: claude[bot] present. ✓
Cross-Engine
All three new internal helpers ($renderPaginationPage, $renderPaginationNav, $renderPaginationNavLink) are declared public string function $..., matching the existing $paginationDisabledElement / $paginationLinkToArgs pattern and the $integrateComponents() requirement documented in .ai/wheels/cross-engine-compatibility.md § "Private View Helpers Not Integrated". No bracket-notation calls, no application-scope function members, no closure this captures. Clean.
Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Wheels Bot — Docs updatedAdded a doc commit to this PR:
MDX user guide: no existing page in |
Wheels Bot — Reviewer B (round 1)A's review is substantive and accurate. All four findings hold up against the diff; there are no false positives and no obvious sycophancy. A's SycophancyNone detected. A does not approve or say "LGTM" without citing evidence. Each finding includes a diff reference and a concrete fix. False positivesNone detected.
Missed issuesNone of consequence.
One calibration note: A calls the Tailwind test weakness "not a blocker," but a test that passes when the wrong preset is accidentally emitted actively masks regressions. It warrants fixing before merge, not just a note. A's own fix suggestion ( Verdict alignmentA submitted ConvergenceAligned — A's |
- vendor/wheels/view/pagination.cfc: encode arguments.text via EncodeForHTML when encode=true in the Bootstrap/Tailwind disabled branches of $renderPaginationNavLink. Previously local.encode was resolved but ignored, causing custom HTML-entity nav text (e.g. «) to render literally in the preset path while being decoded in the plain path. - vendor/wheels/tests/specs/view/paginationHelpersSpec.cfc: replace the weak Tailwind pageNumberLinks assertion (which passed even if Bootstrap markup was accidentally emitted) with Tailwind-specific class assertions (pagination-current, pagination-link), plus paginationNav coverage for viewStyle="bootstrap4" (active class on <li>, no aria-current) and viewStyle="tailwind" (flat <nav class="pagination"> with no <ul>, pagination-disabled spans on first page), and a showInfo=true positioning spec for Bootstrap 5. Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Wheels Bot — Address Review (round 1)Applied consensus findings from Reviewer A and Reviewer B's convergence (round 1):
Skipped (intentionally — outside the required consensus set):
Test run: local execution remains blocked in the bot sandbox ( The new commit will trigger a fresh Reviewer A run on the updated SHA. Convergence loop continues until reviewers align on |
Wheels Bot — Reviewer ATL;DR: The second commit (25096be) adds the CLAUDE.md docs nit requested in the previous round — that part looks good. However, the two correctness findings and two test-coverage gaps flagged in round one are still unresolved in the implementation commit (c228697). Requesting changes so those items land before merge. REVIEW EVENT: REQUEST_CHANGES Note: GitHub does not allow a PR author to submit a request-changes review on their own PR. This comment carries the full review body as requested. The verdict is: request changes. [Carry-over] Encoding inconsistency in disabled nav link text
[Carry-over] Silent garbled output on invalid viewStyle value
[Carry-over] Tailwind pageNumberLinks spec is too weak
[Carry-over] No paginationNav coverage for Tailwind or Bootstrap 4 The three Docs nit (minor): The new CLAUDE.md block shows Commits: both clean (feat/docs types, correct scopes, under 100 chars, DCO present). Cross-engine: no new concerns in the docs-only commit; round-one analysis holds for the implementation commit. |
Wheels Bot — Propose Fix skippedNo triage comment found for This PR has open reviewer findings (Reviewer A requested changes in round 1 on SHA No fix proposed. |
…ationnav-pagenumberlinks-cannot-emit-bootstra Signed-off-by: Peter Amiri <peter@alurium.com> # Conflicts: # CHANGELOG.md
Reviewer A flagged in round 2 that the three internal $renderPagination* helpers silently emit structurally broken HTML when handed an unknown viewStyle value (e.g. a typo like "boostrap5"). The call-site guard `local.useViewStyle = Len(arguments.viewStyle) && arguments.viewStyle != "plain"` already filters out the empty/plain case, so reaching the default branch is by definition a developer typo. Replace the silent fallbacks with a Wheels.InvalidViewStyle throw so the failure is loud and immediate at development time instead of surfacing as garbled output. Specs cover both the pageNumberLinks() and paginationNav() public entry points, asserting the typed throw using the existing toThrow matcher pattern from assetsSpec/viteSpec. Round-2 carry-over per Reviewer A; round-1 address-review intentionally skipped this per Reviewer B's "nice-to-have" classification, but Reviewer A held it open as still required. Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Signed-off-by: Peter Amiri <peter@alurium.com>
Reviewer A's round-2 docs nit: the CLAUDE.md "Pagination View Helpers" quick reference showed paginationNav with bootstrap5 only, while bootstrap4 and tailwind appeared only under pageNumberLinks. Both presets are equally valid on both helpers, so add the paginationNav companion lines for the two missing presets. Signed-off-by: Peter Amiri <peter@alurium.com>
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This is the round-2 iteration of the viewStyle preset feature (closes #2718). All three findings from my previous review — encoding inconsistency in disabled nav links, weak Tailwind test assertions, and silent broken output on unknown viewStyle values — have been addressed across commits cbf015d80 (round 1) and 35473fab6 (round 2). The implementation is correct, cross-engine clean, and well-covered by tests. No blockers remain; recommend merge after human approval.
Correctness
All three prior findings are resolved:
-
Encoding in disabled nav text —
$renderPaginationNavLinknow computeslocal.safeTextviaEncodeForHTML(arguments.text)whenencode=trueand uses it in both the Bootstrap and Tailwind disabled branches (pagination.cfclines 672–680, 693). The earlier bug wherelocal.encodewas resolved but then ignored is gone. -
Throw on unknown
viewStyle— all three$render*helpers now haveThrow(type="Wheels.InvalidViewStyle", ...)in theirdefaultcases (lines 562–567, 647–652, 706–711). A typo like"boostrap5"is caught at the first$renderPaginationPagecall, fails loudly, and is not silently emitted as structurally broken HTML. -
Struct mutation across loop iterations — a new
local.linkArgsis constructed on each iteration of thepageNumberLinksloop, so thearguments.linkArgs.class = "page-link"mutation inside$renderPaginationPagedoes not bleed into subsequent iterations. Verified against the loop body (diff lines 262–277). No change needed.
Tests
Round 1 strengthened the Tailwind assertion from the weak aria-current="page" substring (which Bootstrap also emits) to Tailwind-specific class names (paginationHelpersSpec.cfc lines 252–255):
expect(result).toInclude('<span class="pagination-current" aria-current="page">')
expect(result).toInclude('class="pagination-link"')
expect(result).notToInclude('<li class="page-item')Round 2 added toThrow("Wheels.InvalidViewStyle") coverage for both public entry points (lines 353–365), matching the existing assetsSpec/viteSpec precedent.
All previously missing cases are now covered: Bootstrap 4 nav, Tailwind nav, disabled-on-first-page for both presets, and showInfo=true ordering (lines 298–351).
One minor style nit, not a blocker: infoPos and ulPos in the showInfo positioning spec (lines 346–350) are assigned without var inside a closure, leaking into variables scope. This is consistent with how result / resultDefault / resultPlain are assigned throughout the entire spec (same pattern on lines 234, 260, 261, 267, etc.), so it reflects the established spec convention rather than an isolated mistake.
Cross-Engine
All three new helpers are declared public string function $renderPagination* (pagination.cfc lines 536, 578, 659), matching the $integrateComponents() requirement for view-layer mixin functions documented in .ai/wheels/cross-engine-compatibility.md § "Private View Helpers Not Integrated". No bracket-notation calls inside closures, no client scope, no application scope function members, no Left(str, 0) calls. StructCopy() on line 612 is a standard BIF present on all engines.
Docs
CLAUDE.md § "Pagination View Helpers" is updated with example calls and a prose explanation of viewStyle semantics (diff lines +466–+492). CHANGELOG.md [Unreleased] → Added entry is present. .ai/wheels/views/ reference docs are deferred to bot-update-docs.yml as flagged in the PR checklist — acknowledged, not a merge blocker.
Commits
Five commits on the branch:
c2286978dfeat(view): add viewStyle preset to paginationNav and pageNumberLinks for Bootstrap markup✓25096be24docs: document viewStyle preset in CLAUDE.md pagination quick reference✓cbf015d80fix(view): address Reviewer A/B consensus findings (round 1)✓- Merge commit (develop sync) — exempt from commitlint ✓
35473fab6fix(view): throw Wheels.InvalidViewStyle on unknown viewStyle (round 2)✓
All claude[bot] commits carry Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>. DCO satisfied on every authored commit.
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: Round-3 incremental review covering the single new commit (baccfd43) since round 2. That commit adds exactly two lines to CLAUDE.md — #paginationNav(viewStyle="bootstrap4")# and #paginationNav(viewStyle="tailwind")# — directly addressing the docs nit I raised: bootstrap4 and tailwind were listed only under pageNumberLinks in the quick reference, not under paginationNav, even though both presets are equally valid on both helpers. All round-1 and round-2 findings remain resolved. No new issues introduced. Ready for human approval and merge.
Docs
CLAUDE.md § "Pagination View Helpers" now lists all three non-plain presets symmetrically under both paginationNav() and pageNumberLinks():
// Bootstrap 4 — same as bootstrap5 but omits aria-current
#paginationNav(viewStyle="bootstrap4")# ← new
#pageNumberLinks(viewStyle="bootstrap4")#
// Tailwind — flat structure with pagination-current / pagination-link utility classes
#paginationNav(viewStyle="tailwind")# ← new
#pageNumberLinks(viewStyle="tailwind")#The prose paragraph below the examples already covered both helpers; the code examples now match. Accurate and consistent.
Commits
New commit: docs: list bootstrap4 and tailwind under paginationNav too — type docs, no scope, subject 49 chars, not ALL-CAPS. Signed-off-by: Peter Amiri <peter@alurium.com> present. DCO satisfied. ✓
Full branch history (five claude[bot] commits + one merge + one human docs patch) was reviewed in round 2; no change to that assessment.
Wheels Bot — Reviewer B (round 1)A's round-3 review is accurate and appropriate. The SycophancyNone detected. A explicitly verifies the 2-line CLAUDE.md change and references its own round-2 analysis rather than re-examining unchanged implementation files. The approval is evidence-backed. False positivesNone detected. Missed issuesNone detected. The
One non-issue worth noting for completeness: A's round-2 review flagged Verdict alignmentA's "ready for human approval and merge" is consistent with the findings: a clean 2-line docs fix on top of a fully-reviewed and corrected implementation. Appropriate. ConvergenceAligned on approve. The PR is review-clean for this SHA. All round-1 findings addressed (encoding, Tailwind test, missing paginationNav coverage for Bootstrap 4 and Tailwind), the |
Wheels Bot — Reviewer B (round 2)A's round-2 review is accurate and well-evidenced. All prior findings are confirmed resolved; no false positives, no sycophancy, no missed issues of consequence. Verdict alignment is correct. Emitting converged-approve. SycophancyNone detected. A's approval is backed by specific line references for each resolved finding. No "LGTM without evidence" pattern. False positivesNone detected.
Missed issuesOne cosmetic gap: A lists five commits but there are six on the branch — A did not enumerate A's round-2 review marker says SHA Verdict alignmentA submitted ConvergenceAligned — A's recommendation to approve is correct. All four consensus findings from round 1 are verified resolved in the diff. The commit history is clean and DCO-compliant. No outstanding disputes. Joint recommendation: approve and merge after human sign-off. |
…ationnav-pagenumberlinks-cannot-emit-bootstra # Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: Round-4 incremental review covering the single new commit since round 3 — a develop-sync merge (2c5b67179). All pagination feature work (rounds 1–3) is unchanged and remains clean. The merge brings in five files from develop that were already landed in PRs #2723 and #2719; the conflict resolution in CHANGELOG.md is correct. One minor observation on the new PackagesCommandHelpSpec.cfc test, not a blocker. Ready for human approval.
Correctness
Merge conflict resolution in CHANGELOG.md — the [Unreleased] → Added and [Unreleased] → Fixed sections now correctly interleave both the feature branch's viewStyle entry and develop's entries (#2719, #2706 / #2723). No entry was dropped or duplicated. The merge was resolved correctly.
expandPath("/wheels/../..") in PackagesCommandHelpSpec.cfc line 21 — this resolves at runtime to the project root, because /wheels is the virtual mapping that points to vendor/wheels/ (a physical path two levels deep). Going /../.. from that physical path produces the project root, making ctx.repoRoot & "/cli/lucli/Module.cfc" correct. The first it() block confirms the file is reachable before the subsequent assertions run, so a mapping misconfiguration produces a descriptive failure rather than a crash.
No new correctness issues.
Tests
vendor/wheels/tests/specs/cli/PackagesCommandHelpSpec.cfc (new, from develop):
The spec reads the Module.cfc source file and makes three assertions: the file exists, the old "Install" phrasing is gone, and the word add is present on the summary line. Two observations:
-
The second and third
it()blocks both callfileRead(ctx.modulePath)independently. A sharedctx.sourcefield set inbeforeAll(or even just cached after the first read) would avoid the double disk read and make the dependency on the first assertion explicit. Minor style nit, not a correctness issue. -
reFindNoCase("\badd\b", summaryLine)—\bis a valid CFML regex word-boundary anchor and the double-quoted CFML string passes the literal characters\bto the regex engine correctly. No cross-engine concern here;reFindNoCaseuses Java regex under all three engines.
Neither observation is a merge blocker.
Cross-Engine
All five files introduced by the merge are cross-engine safe:
Module.cfcline 223: plain string concatenation.PackagesCommandHelpSpec.cfc: standard CFML BIFs only (fileRead,fileExists,find,reFindNoCase,mid,chr,expandPath). Uses thectxstruct pattern for closure variable sharing (matching CLAUDE.md § "Closure gotcha"). Noclientscope, no bracket-notation calls inside closures, noLeft(str, 0), noapplication-scope function members.changelog.mdx/index.mdx: Astro/Starlight MDX; not CFML.
Commits
Merge commit 2c5b67179 — merge commits are exempt from commitlint. The authored commits that arrived via develop (8b1a448b0 and its predecessors) were already commitlint-clean when they landed in develop; no re-review needed here.
Full branch commit history was verified clean in round 2. No change to that assessment.
Docs
web/sites/guides/src/content/docs/v4-0-1-snapshot/upgrading/changelog.mdx — new reference page explaining where CHANGELOG.md lives (repo root, not vendor/wheels/), how to look up PR references, and offline access instructions. Content is accurate. The index.mdx Aside note and new LinkCard entry are consistent with the new page.
index.mdx line 30: box.json was corrected to wheels.json in the same merge. Accurate; the repo manifest is wheels.json.
…ationnav-pagenumberlinks-cannot-emit-bootstra # Conflicts: # vendor/wheels/view/pagination.cfc
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: Round-5 incremental review covering the single new merge commit (fb55f0252) since round 4. That commit pulls ce47c8024 (#2726 -- paginationNav() unknown-argument validation) from develop, along with the two CORS fixes (#2724, #2725). The critical question for this round is whether the new unknown-arg validation in paginationNav() interacts correctly with the viewStyle feature from this PR. It does. No new correctness, cross-engine, or security issues. One minor observation worth noting.
Correctness
viewStyle / unknown-arg validation interaction is sound
vendor/wheels/view/pagination.cfc, line 374:
local.skipArgs = "handle,navClass,showFirst,showLast,showPrevious,showNext,showInfo,showSinglePage,viewStyle,encode";viewStyle is in local.skipArgs, so it is never evaluated against local.allowedSubArgs (line 380) and never lands in local.unknownArgs (line 383). The two new validation blocks from ce47c8024 are therefore fully transparent to viewStyle callers. Verified against the merged file at head via git show fb55f0252:vendor/wheels/view/pagination.cfc.
Throw priority is correct
A call like paginationNav(viewStyle="boostrap5") (typo) with showErrorInformation=true:
viewStyleinskipArgs--unknownArgsstays empty --Wheels.PaginationNav.InvalidArgumentis NOT thrownlocal.useViewStyle = true--$renderPaginationNav--$renderPaginationNavLink--switch default-- throwsWheels.InvalidViewStyle
The Wheels.InvalidViewStyle path is ungated (always throws, even in production) while the Wheels.PaginationNav.InvalidArgument path is gated on showErrorInformation. The distinction is intentional: an unknown viewStyle would produce structurally broken HTML with no recovery path, so it warrants a hard error in all environments. An unknown sub-helper arg is silently dropped in production to preserve backward compatibility. Consistent design.
Minor observation -- no test for viewStyle + unknown-sub-arg combination
paginationNav(viewStyle="bootstrap5", prependToList="<ul>") with showErrorInformation=true would throw Wheels.PaginationNav.InvalidArgument from the validation block (line 395) before reaching the local.useViewStyle check (line 427). There is no explicit spec for this combination. Given the existing tests cover both mechanisms independently (the standalone unknown-arg specs in lines 424-463, and the Wheels.InvalidViewStyle specs in lines 353-365), the gap is acceptable -- adding one integration spec for this edge would be belt-and-suspenders. Not a merge blocker.
CHANGELOG.md merge resolution
Both the viewStyle #2718 entry (under Added) and the paginationNav validation #2717 entry (under Fixed) are present and correctly positioned. No entries dropped or duplicated.
CLAUDE.md merge resolution
The viewStyle code examples (paginationNav(viewStyle="bootstrap5"), bootstrap4, tailwind) and the viewStyle prose paragraph are all present at the head commit (fb55f0252). The local working copy of CLAUDE.md is marked modified in the sandbox (pre-existing state, unrelated to this PR). Verified via git show fb55f0252:CLAUDE.md.
Tests
The ce47c8024 merge brought new test coverage into paginationHelpersSpec.cfc (lines 424-490): unknown-arg throws when showErrorInformation=on, silence when off, documented sub-helper args pass cleanly, and the named-route segment-variable exemption regression spec. All four are in the correct describe("paginationNav") block (lines 369-492), not the viewStyle preset block. The $clearRoutes() method is defined at component level (line 498) per CLAUDE.md requirement -- not inherited from wheels.WheelsTest. ✓
Commits
New commits since round 4:
ce47c8024fix(view): paginationNav rejects unknown sub-helper arguments (#2726)-- squash merge, all constituent commits conventional, DCOSigned-off-byon every authored commit ✓a575c521bfix(middleware): emit Vary: Origin from Cors when reflecting allowed origins (#2724)✓f9a4312b6fix(middleware): Cors no longer emits raw comma list as Access-Control-Allow-Origin (#2725)✓fb55f0252merge commit -- exempt from commitlint ✓
All round 1-4 findings remain resolved. Full branch commit history was verified clean in round 2; no change to that assessment.
Cross-Engine
No new CFC changes from this PR's feature layer in this merge. The three new CORS middleware changes are in vendor/wheels/middleware/Cors.cfc -- outside this PR's scope and already landed in develop. No cross-engine concerns introduced.
Wheels Bot — Reviewer B (round 1)A's round-5 review is accurate and appropriately scoped to the single merge commit. All claims checked out against the live code at SycophancyNone detected. A explicitly says no new issues were found, then backs this up with specific line citations and a worked-through throw-priority trace. No evidence-free LGTM. False positivesNone detected.
Missed issuesNone of consequence. One calibration note A omitted: when The CORS changes ( Verdict alignmentA submitted ConvergenceAligned on approve. All round-1 through round-4 findings were confirmed resolved in prior B rounds. A's round-5 review adds no open findings — only a minor belt-and-suspenders test gap that A explicitly categorizes as not a merge blocker. Joint recommendation: the PR is review-clean for this SHA and ready for human approval and merge. |
…ationnav-pagenumberlinks-cannot-emit-bootstra # Conflicts: # CLAUDE.md # vendor/wheels/events/init/functions.cfm # vendor/wheels/view/pagination.cfc
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: Round-6 incremental review covering the single new merge commit (7fe6c76c3) since round 5. That commit pulls 54f448425 (PR #2730 — paginationNav() now accepts Bootstrap-style prepend/append/prependToPage/appendToPage/anchorDivider/addActiveClassToPrependedParent) from develop, along with new security specs (PaginationXssSpec.cfc), deprecation specs (paginationLinksDeprecationSpec.cfc), package-mapping specs, and a bot-tdd-gate.yml extension. The critical question for this round is whether the conflict resolution between the viewStyle preset feature and the PR #2730 wrapping args is correct. It mostly is, with one documentation gap worth noting. All prior findings remain resolved.
Correctness
Conflict resolution in vendor/wheels/view/pagination.cfc is sound
The three-way merge correctly sequences PR #2730 additions alongside the viewStyle feature. Sanitization of prependToPage/appendToPage via $paginationSanitizeWrapper() runs at line 410 before the local.useViewStyle branch at line 477, so the scrub fires regardless of which code path is taken. $paginationWrapAnchor() and $paginationSanitizeWrapper() are both declared public string function $... (lines 570, 587), satisfying the $integrateComponents() requirement.
prependToPage/appendToPage reach subArgs but are harmlessly ignored on the viewStyle path
prependToPage and appendToPage are NOT in local.skipArgs (line 422), so they ARE forwarded into local.subArgs. When viewStyle is non-plain, $renderPaginationNav() receives them inside subArgs but neither $renderPaginationNavLink() nor the $renderPaginationPage() call via pageNumberLinks() consults them — both helpers build their own canonical wrappers. Intentional; matches the documented design.
prepend, append, anchorDivider are silently dropped on the viewStyle path — docs do not say so
prepend, append, and anchorDivider are in local.skipArgs (line 422), never forwarded into local.subArgs. The $renderPaginationNav() call site (lines 479-488) passes none of them — paginationNav(viewStyle="bootstrap5", prepend="<p>Nav</p>") silently drops prepend. This is the correct design (the Bootstrap 5 preset already emits its own <ul class="pagination">/</ul> structure, and anchorDivider has no equivalent in the $renderPaginationNav() concatenation loop). But the CLAUDE.md paragraph currently reads:
Non-plain presets ignore
prependToPage,appendToPage,classForCurrent, andclassin favour of the preset markup.
These three args (prepend, append, anchorDivider) were added by PR #2730 which landed after the viewStyle feature was originally written, so the ignore list was not updated. Suggested fix:
Non-plain presets ignore `prependToPage`, `appendToPage`, `classForCurrent`, `class`, `prepend`, `append`, and `anchorDivider` in favour of the preset markup.
Not a merge blocker — the prior paragraph positions manual args as alternatives to viewStyle ("these compose into the same Bootstrap-style output as the viewStyle preset but with finer-grained control"), so the design intent is clear. But the omission from the explicit ignore list will confuse anyone who reads the docs sequentially.
Tests
PaginationXssSpec.cfc — well-formed. ## escaping is correct (CFML compiles "&##111;nmouseover" to the runtime string onmouseover). $clearRoutes() is defined at component level (not inherited from wheels.WheelsTest) per CLAUDE.md requirement. beforeEach/afterEach correctly restore encode settings and clear the deprecation-guard struct key.
paginationLinksDeprecationSpec.cfc — clean. structDelete in beforeEach/afterEach is idempotent (CFML's structDelete is a no-op when the key does not exist).
PackageMappingSpec.cfc (from develop, PR #2712) — uses the ctx struct pattern for closure variable sharing per CLAUDE.md "Closure gotcha". No client scope, no application-scope function members, no bracket-notation calls inside closures. Clean.
New paginationNav plain-path specs (from PR #2730) — the anchorDivider spec uses "XDIVX" as a sentinel and asserts toMatch("</[^>]+>XDIVX<") — a precise structural check that cannot accidentally pass if the divider ends up inside a tag.
Cross-Engine
$decodeHtmlEntities (links.cfc, appended by PR #2730) correctly applies the Lucee 7 Left(str, 0) guard:
local.prefix = local.match.pos[1] > 1 ? Left(local.result, local.match.pos[1] - 1) : "";Both the hex and decimal loops carry the guard (matches CLAUDE.md gotcha for Left(str, 0) on Lucee 7). All five new $pagina* and $render* helpers are public string function $... — correct for view-layer mixins. No bracket-notation calls inside closures, no client scope, no application-scope function members. Clean.
Commits
New since round 5:
54f448425fix(view): paginationNav now accepts Bootstrap-style prepend/append/per-anchor wrappers (#2730)— squash merge of three commits, all conventional, DCOSigned-off-byon every authored commit7fe6c76c3merge commit — exempt from commitlint. Conflict resolution in CHANGELOG.md, CLAUDE.md,vendor/wheels/events/init/functions.cfm, andvendor/wheels/view/pagination.cfcis correct; no entries dropped or doubled.
Docs
CLAUDE.md "Non-plain presets ignore..." paragraph does not list prepend, append, and anchorDivider as also silently ignored by non-plain viewStyle presets. See Correctness above for the suggested one-line fix. Minor; can be addressed in a follow-up commit before merge.
Reviewer A round-6 docs gap: the CLAUDE.md "Non-plain presets ignore..." list was written before PR #2730 added prepend/append/anchorDivider args. Those args also land in local.skipArgs and are NOT forwarded into $renderPaginationNav(), so the preset path silently drops them. Add the three names to the explicit ignore list so anyone reading the docs sequentially sees the full picture. Signed-off-by: Peter Amiri <peter@alurium.com>
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: Round-7 incremental review covering the single new commit since round 6 — 625089ca4 (docs: extend viewStyle ignore list with prepend/append/anchorDivider). This commit addresses the documentation gap I flagged in round 6: the CLAUDE.md "Non-plain presets ignore..." sentence had been written before PR #2730 added prepend, append, and anchorDivider, so those three args were absent from the explicit ignore list even though they are silently dropped on the viewStyle preset path. The fix is accurate and complete. All prior round 1-6 findings remain resolved. Ready for human approval.
Docs
The round-6 finding was:
CLAUDE.md "Pagination View Helpers" "Non-plain presets ignore..." paragraph does not list
prepend,append, andanchorDivideras also silently ignored by non-plainviewStylepresets.
The new commit extends the sentence from:
Non-plain presets ignore
prependToPage,appendToPage,classForCurrent, andclassin favour of the preset markup.
to:
Non-plain presets ignore
prependToPage,appendToPage,classForCurrent,class,prepend,append, andanchorDividerin favour of the preset markup.
This is accurate. At head, vendor/wheels/view/pagination.cfc line 422 reads:
local.skipArgs = "handle,navClass,showFirst,showLast,showPrevious,showNext,showInfo,showSinglePage,viewStyle,prepend,append,anchorDivider,encode";All three names are in local.skipArgs and are therefore never forwarded into local.subArgs or downstream to $renderPaginationNav(). The documentation now matches the code.
Commits
Single commit: docs: extend viewStyle ignore list with prepend/append/anchorDivider — type docs, no scope, subject 56 chars, not ALL-CAPS. Commit body explains the "why" (written before PR #2730 landed; references round-6 finding by name). Signed-off-by: Peter Amiri <peter@alurium.com> present. DCO satisfied. ✓
Full branch commit history was verified clean in round 2; no change to that assessment.
Wheels Bot — Reviewer B (round 1)A's round-7 review is accurate and appropriately scoped. The commit is a single-line CLAUDE.md change; A's two verifiable claims (content of the docs change and the live SycophancyNone detected. A states readiness concisely and backs it up with a direct code citation rather than an evidence-free LGTM. False positivesNone detected.
Missed issuesNone of consequence. The one thing worth noting for completeness: Verdict alignmentA submitted ConvergenceAligned on approve. The |
|
test |
There was a problem hiding this comment.
Wheels Bot — Reviewer A response (round 1)
B's round-1 critique on SHA 625089c is a complete endorsement of my round-7 review. B found no sycophancy, no false positives, and no missed issues of consequence — noting only one pre-existing documentation gap (addActiveClassToPrependedParent not listed in the ignore sentence) that predates this commit and that B explicitly states I correctly did not raise in a round scoped to a single-line docs fix. With nothing to concede and nothing to defend, the exchange has converged.
Conceded points: None.
Defended points: None required. B validated all claims: the CLAUDE.md sentence change accurately lists all three newly-ignored args (prepend, append, anchorDivider) and matches local.skipArgs at line 422 of pagination.cfc at head; commit metadata (type docs, subject 56 chars, DCO Signed-off-by: Peter Amiri) is correct; the addActiveClassToPrependedParent documentation gap is pre-existing and out of scope for this commit and B concurs it was correct not to raise it here.
Updated verdict: Approve — all round 1-6 findings are confirmed resolved, the round-7 docs fix is accurate and well-scoped, and B is independently aligned. The PR is review-clean for this SHA and ready for human approval and merge.
Wheels Bot — Reviewer B (round 2)A's response to B round 1 is accurate and appropriate. B round 1 already emitted SycophancyNone detected. A accurately describes what B said rather than offering an evidence-free agreement. False positivesNone detected. A's characterization of B round 1 is correct on every point: B found no sycophancy, no false positives, no missed issues, flagged the Missed issuesNone detected. A's response is appropriately scoped to acknowledging the prior convergence — no reason to scan for new findings on an already-converged SHA. Verdict alignmentA's "Approve" verdict is consistent with everything in the response and with B round 1's prior convergence. No gap. ConvergenceAligned. B converged on approve in round 1; A confirms in this response. No open disputes. The PR is review-clean for SHA |
…-display-ux-differs-fr Resolves conflicts with #2730 (Bootstrap-style wrappers), #2731 (viewStyle preset), and #2736 (whereIn empty-array fix) — all of which landed on develop while this PR was in review. Conflict resolutions: - CHANGELOG.md: kept both this PR's `Fixed` entry and #2736's QueryBuilder empty-IN fix; they are independent. - CLAUDE.md: composed all three feature sections (auto-mode tri-state, viewStyle presets, manual Bootstrap composition) into one pagination reference block; the `Bootstrap 5 — manual composition` comment now also notes `showFirst="always"` to restore 3.x always-show behaviour. - vendor/wheels/events/init/functions.cfm: added both `windowSize = 2` and `viewStyle = "plain"` defaults to the `paginationNav` function argument map. - vendor/wheels/view/pagination.cfc: added both `windowSize` and `viewStyle` to the docblock, signature, and `skipArgs` list. In the `$renderPaginationNav` early-return path, anchor display flags are now resolved through `$paginationShouldShowAnchor()` so viewStyle presets honour the new tri-state `"auto"` / `"always"` / `"never"` modes consistently with the plain path. - vendor/wheels/tests/specs/view/paginationHelpersSpec.cfc: two viewStyle preset tests that asserted First/Last anchor presence at page=2 (BS5 per-item wrapping) and page=1 (Tailwind disabled-span) now explicitly pass `showFirst="always"` / `showLast="always"` — the new default `"auto"` mode auto-suppresses these when the rendered page-number window already reaches the boundary, which is the intended behaviour of this PR. Verified locally on Lucee 7 + SQLite: view (555/0/0) and model (830/0/0) spec bundles green. Signed-off-by: Peter Amiri <peter@alurium.com>
1 participant
Summary
Adds a
viewStyleargument topaginationNav()andpageNumberLinks()so apps using Bootstrap-style pagination no longer need aReplace()regex hack to move the active class from the<a>onto the<li>wrapper. The new argument is a named CSS-framework preset ("plain"/"bootstrap5"/"bootstrap4"/"tailwind") that mirrors Phoenix'sview_style:option from Scrivener.HTML — the lightest-weight match for Wheels' existing helper API, no new partial-override system, no global mutable state, no generators.viewStyle="bootstrap5"emits the canonical Bootstrap 5 markup:— active class on the
<li>, current page rendered as<span class="page-link">(not anchor),aria-current="page"for accessibility. Bootstrap 4 differs only by omittingaria-current(per BS4 conventions). Tailwind emits a flatter structure withpagination-current/pagination-link/pagination-disabledutility hooks.Default remains
viewStyle="plain", which dispatches through the original code path unchanged and preserves today's byte-for-byte output. Existing callers see no behavioral change.Related Issue
Closes #2718
Recommended path from research: #2718 (comment)
Type of Change
Feature Completeness Checklist
Signed-off-by:trailer present on the single commitvendor/wheels/tests/specs/view/paginationHelpersSpec.cfccoveringpageNumberLinks(viewStyle=...)forbootstrap5/bootstrap4/tailwind/plain, pluspaginationNav(viewStyle="bootstrap5")integration cases including the disabled-on-first-page wrapper. Existing tests retain the defaultviewStyle="plain"path and continue to assert the original markup.bot-update-docs.ymlfollow-upbot-update-docs.ymlfollow-upbot-update-docs.ymlfollow-up[Unreleased] → AddedTest Plan
compat-matrix.ymlgreen across Lucee 6/7, Adobe 2023/2025, BoxLang × every database in the matrixbot-tdd-gate.ymlgreen (spec changes + impl changes both present in diff)paginationLinks()+Replace()hack forpaginationNav(viewStyle="bootstrap5")and confirm the active class lands on the<li>and the current page renders as<span class="page-link">Files Changed
vendor/wheels/view/pagination.cfc— addedviewStylearg +\$renderPaginationPage/\$renderPaginationNav/\$renderPaginationNavLinkinternal helpers ($-prefixed so they integrate via\$integrateComponentson Lucee/Adobe)vendor/wheels/events/init/functions.cfm— registeredviewStyle = "plain"default for both functionsvendor/wheels/tests/specs/view/paginationHelpersSpec.cfc— new describe blocks for theviewStylepresetsCHANGELOG.md—[Unreleased] → Addedentry