fix(view): paginationNav anchor modes default to window-aware auto-suppress - #2733
Conversation
…ppress Widens `showFirst` / `showLast` / `showPrevious` / `showNext` on `paginationNav()` from `boolean` to `any`, accepting the tri-state strings `"auto"` / `"always"` / `"never"` with backwards-compatible boolean coercion (`true` -> `"always"`, `false` -> `"never"`). Defaults flip from `true` to `"auto"`. Under `"auto"` the first/last anchors only render when the visible page-number window does not already reach the boundary, restoring the legacy 3.x `paginationLinks(alwaysShowAnchors=false)` semantics that a like-for-like swap to `paginationNav()` previously lost. Adds a `windowSize` arg on `paginationNav()` so the auto-mode predicates stay coherent with `pageNumberLinks()`'s window. Fixes #2716 Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
…aysShowAnchors parity Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Wheels Bot — Docs updatedAdded a doc commit to this PR:
|
![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 correctly restores the 3.x window-aware suppress behaviour for showFirst/showLast in paginationNav(), introduces a clean tri-state API with backwards-compatible boolean coercion, and ships a solid test suite for those two anchors. However, the defaults for showPrevious/showNext also flip from true to "auto", and their auto-mode logic is not window-based — it instead silently drops the disabled spans that previousPageLink()/nextPageLink() previously rendered at page boundaries. That change is undocumented in the PR body, untested, and is a user-visible regression from the old default. This review would be request-changes but GitHub prevents self-review; a human maintainer should act on findings 1 and 3 before merging.
Correctness
1. showPrevious/showNext auto mode silently removes disabled spans at page boundaries (medium)
vendor/wheels/view/pagination.cfc, lines 447–455:
case "previous":
return arguments.pg.currentPage > 1;
case "next":
return arguments.pg.currentPage < arguments.pg.totalPages;When showPrevious="auto" (the new default) and currentPage == 1, $paginationShouldShowAnchor returns false, so previousPageLink() is never called. Under the old default of showPrevious=true, paginationNav() always called previousPageLink(), which renders a disabled <span>Previous</span> on page 1 (because showDisabled=true is previousPageLink's own default — see functions.cfm lines 281–299). The disabled span disappears entirely under "auto".
The PR body says: "Under 'auto' the first/last anchors only render when the visible page-number window does not already reach the boundary." That sentence names first/last only. Any app that relied on the disabled spans to indicate "you are at the first/last page" will silently lose that UI after upgrading.
Suggested fix — pick one:
(a) Explicitly document the prev/next boundary-hide behaviour in the @showPrevious/@showNext param javadoc comments and in the PR body, and add tests (see finding 3 below).
(b) Make "auto" for prev/next behave identically to the old true — always call the sub-helper and let it decide whether to render a link or disabled span — and reserve boundary-hide for "never". Under this interpretation, "auto" would mean "smart window-based suppression" for first/last and "delegate to sub-helper" for prev/next, which is semantically cleaner.
2. windowSize flows through local.subArgs into sub-helpers that do not accept it (low)
vendor/wheels/view/pagination.cfc, line 359:
local.skipArgs = "handle,navClass,showFirst,showLast,showPrevious,showNext,showInfo,showSinglePage,encode";windowSize is absent from skipArgs, so it enters local.subArgs and is forwarded via argumentCollection to firstPageLink(), lastPageLink(), previousPageLink(), and nextPageLink(). Those helpers do not declare windowSize. CFML engines silently ignore extra named arguments, and these helpers already receive unknown route params through the same mechanism, so this is not a crash risk today. It is a latent fragility if $args() ever tightens validation or if a caller has a route segment named windowSize. The clean fix is to add "windowSize" to local.skipArgs and pass it explicitly only to pageNumberLinks().
Tests
3. No spec for showPrevious/showNext auto mode at page boundaries
The new describe("paginationNav anchor display modes", ...) block (spec file line 287) exercises showFirst/showLast across all modes and boundary conditions. There is no it() for:
- previous on page 1 in auto mode (does the disabled span appear or not?)
- next on last page in auto mode (same question)
These are precisely the cases where finding 1 lives. Without explicit specs the contract is invisible to future maintainers and regressions will go undetected.
Conventions
No issues. public access on the two new $-prefixed helpers is correct — CLAUDE.md documents that $integrateComponents() only copies public methods, so private here would silently break the view mixin on Lucee/Adobe. Named-argument style is consistent throughout.
Cross-engine
No concerns. IsBoolean(), ListFindNoCase(), LCase(), Throw(), and switch/case are standard CFML that behave identically on Lucee 6/7, Adobe CF 2023/2025, and BoxLang. The ternary arguments.value ? "always" : "never" is standard CFScript. The inline closure in the spec (expect(() => { _controller.paginationNav(showFirst = "bogus") }).toThrow()) sits inside expect(), not in a new Component(...) constructor arg list, so it avoids the Adobe CF ASTcffunction bug documented in .ai/wheels/cross-engine-compatibility.md.
Commits
Single commit fix(view): paginationNav anchor modes default to window-aware auto-suppress (8f54c063). Type fix, scope view, subject 69 chars, not ALL-CAPS — commitlint-clean. DCO Signed-off-by: claude[bot] trailer is present and correct. The commit body accurately describes the first/last behaviour but likewise omits the prev/next disabled-span change.
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: The new docs commit (df9a3d622) correctly updates CLAUDE.md and the 3x-to-4x upgrade guide: the boolean-to-string migration example and the prose summary are accurate. However, two findings from the previous review remain unaddressed, and the new CLAUDE.md prose introduces a documentation gap for showPrevious/showNext auto-mode behavior. The core correctness issue — showPrevious="auto" silently drops the disabled-span output that the old showPrevious=true default produced at page boundaries — is still undocumented and untested. This review would be request-changes; a human maintainer should act on findings 1 and 3 before merging.
Correctness
1. showPrevious/showNext auto mode silently removes disabled spans at page boundaries (medium — unaddressed from previous review)
vendor/wheels/view/pagination.cfc, lines 450–453:
case "previous":
return arguments.pg.currentPage > 1;
case "next":
return arguments.pg.currentPage < arguments.pg.totalPages;Under the old default of showPrevious=true, paginationNav() always called previousPageLink(). Because previousPageLink() defaults to showDisabled=true, it renders <span class="disabled">Previous</span> on page 1. Under the new default of showPrevious="auto", $paginationShouldShowAnchor returns false on page 1 and previousPageLink() is never called — the disabled span disappears entirely.
The function docblock at lines 427–433 does acknowledge this ("Previous/next render whenever the current page is not at that boundary"), but that sentence lives only in an internal CFC comment. The PR body, CHANGELOG, CLAUDE.md, and upgrade guide all describe only the first/last window-based suppression behavior. Apps that relied on the disabled Previous/Next spans to signal "you are at the boundary" will silently lose that UI element on upgrade.
Suggested fixes (pick one):
(a) Document the prev/next boundary-hide in CHANGELOG, CLAUDE.md, and the upgrade guide. Add a sentence such as: "Under "auto" the previous/next anchors are hidden entirely when the current page is already at the first/last page." This makes the behavior opt-in-knowingly rather than a surprise.
(b) Make "auto" for prev/next always call the sub-helper and let showDisabled decide — the same delegation that the old showPrevious=true path used. Under this reading, "auto" means "window-based suppression for first/last; delegate to sub-helper for prev/next", and "never" is the explicit hide-everything choice. This eliminates the regression entirely and matches 3.x semantics most closely.
2. windowSize leaks into sub-helpers that do not accept it (low — unaddressed from previous review)
vendor/wheels/view/pagination.cfc, line 359:
local.skipArgs = "handle,navClass,showFirst,showLast,showPrevious,showNext,showInfo,showSinglePage,encode";windowSize is absent from skipArgs, so it ends up in local.subArgs and is forwarded via argumentCollection to firstPageLink(), lastPageLink(), previousPageLink(), and nextPageLink(), none of which declare that arg. CFML engines silently ignore extra named args today, but this is fragile: if a route segment is named windowSize, or if $args() validation ever tightens, behavior will silently change. The clean fix is to add "windowSize" to skipArgs and pass it explicitly only to the pageNumberLinks() call.
Tests
3. No spec for showPrevious/showNext auto mode at page boundaries (unaddressed from previous review)
vendor/wheels/tests/specs/view/paginationHelpersSpec.cfc: the new describe("paginationNav anchor display modes", ...) block has thorough coverage of showFirst/showLast in all three modes, boolean coercion, and windowSize propagation. There is no it() for:
- previous on page 1 in auto mode (does
<span class="disabled">appear or not?) - next on the last page in auto mode (same question)
These are exactly the cases where finding 1 lives. Without explicit specs the contract is invisible to future maintainers and regressions will go undetected.
Docs
4. CLAUDE.md auto-mode prose describes first/last only, silent on prev/next (new)
CLAUDE.md, updated in df9a3d622:
`showFirst` / `showLast` / `showPrevious` / `showNext` accept `"auto"` (default),
`"always"`, or `"never"`. Under `"auto"` the first/last anchors are hidden when the
visible page-number window already reaches the boundary.
The sentence lists all four args but describes only first/last auto behavior. A developer reading this will reasonably infer that showPrevious="auto" behaves like showPrevious="always" (always calls the sub-helper), which is incorrect — the disabled span disappears at page boundaries. If finding 1 is resolved by documenting rather than reverting the behavior, this sentence is the right place to add the clarification.
The upgrade-guide addition in 3x-to-4x.mdx is accurate and appropriately scoped; no issues there.
Commits
Both commits are commitlint-clean:
fix(view): paginationNav anchor modes default to window-aware auto-suppress(8f54c063) — typefix, scopeview, 69-char subject, not ALL-CAPS, DCO present.docs(web/guides): document paginationNav anchor display modes and alwaysShowAnchors parity(df9a3d622) — typedocs, scopeweb/guides, 68-char subject, not ALL-CAPS, DCO present.
Wheels Bot — Reviewer B (round 1)A's review is substantive and accurate. All three findings are real. I verified each against the actual diff and the merged file. Verdict (request-changes) is consistent with the findings. Recommending converged-changes. SycophancyNone detected. A did not approve despite outstanding correctness and test coverage concerns. False positivesNone detected.
Missed issuesNone of significance. The only wrinkle worth flagging beyond A's review: if address-review applies Finding 2's fix, it must pair the Verdict alignmentA's request-changes verdict is correct: the PR ships an undocumented and untested behavioral change (prev/next disabled spans disappearing at page boundaries under the new ConvergenceAligned. The consensus is request-changes with two concrete actions:
Finding 2 (low) can be addressed in the same pass: add |
Wheels Bot — Reviewer B (round 1)A's review is well-grounded with no sycophancy and no false positives. All four findings are confirmed by the diff and live code. A's SycophancyNone detected. A's review is critical throughout; the TL;DR explicitly calls for human action before merge. False positivesNone detected. I verified each claim:
Missed issuesNone significant. Commits are commitlint-clean (A verified both SHA hashes; I corroborated). DCO trailers are present. No cross-engine concerns in the new logic — the switch/case and Verdict alignmentA's ConvergenceAligned with A. All four findings are substantiated. The consensus required changes are: (a) document the |
Addresses the converged-changes consensus from Reviewer A and Reviewer B on PR #2733: - `vendor/wheels/view/pagination.cfc`: under `"auto"`, `showPrevious` and `showNext` now always delegate to `previousPageLink()` / `nextPageLink()`, preserving the legacy disabled-span boundary indicator that the original PR silently dropped at page 1 / last page. `"never"` remains the explicit hide-everything choice. Option (b) from the reviewers' converged guidance. - `vendor/wheels/view/pagination.cfc`: `windowSize` added to `local.skipArgs` so it no longer leaks into `firstPageLink()`, `lastPageLink()`, `previousPageLink()`, `nextPageLink()` (none of which declare it). It is passed explicitly to `pageNumberLinks()` to keep the windowed page-number coverage coherent with the auto-mode predicates. - `vendor/wheels/tests/specs/view/paginationHelpersSpec.cfc`: six new `it()` blocks under `paginationNav anchor display modes` covering Previous on page 1 and Next on last page in auto / never modes, plus the corresponding non-boundary paths. - `CHANGELOG.md`, `CLAUDE.md`, `web/sites/guides/src/content/docs/v4-0-1-snapshot/upgrading/3x-to-4x.mdx`: prose updated to describe the prev/next auto delegation and the `"never"` opt-out, alongside the existing first/last auto-suppress documentation. Local test run: `directory=wheels.tests.specs.view` against the Lucee 7 + SQLite server returns 529 pass, 0 fail, 0 error (up from 523 — six new tests added). 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):
Verification: The new commit will trigger a fresh Reviewer A run on the updated SHA. Convergence loop continues until reviewers align on |
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: Commit `55d46b84b` ("fix(view): address Reviewer A/B consensus findings (round 1)") resolves all four findings carried from the previous two review rounds. The disabled-span regression for `showPrevious`/`showNext` in auto mode is corrected; `windowSize` no longer leaks into anchor sub-helpers; six new `it()` blocks cover the Previous/Next boundary contract; documentation across CHANGELOG.md, CLAUDE.md, and the v4-0-1-snapshot upgrade guide now explicitly describes the prev/next auto-delegation behavior and the `"never"` opt-out. The PR is clean. Verdict: approve (self-review blocked by GitHub; a human maintainer may merge).
Correctness
Finding 1 (rounds 1 & 2): `showPrevious`/`showNext` auto mode silently dropping disabled spans — RESOLVED.
`vendor/wheels/view/pagination.cfc`, lines 455–461 (HEAD):
```cfm
switch (arguments.side) {
case "first":
return (arguments.pg.currentPage - arguments.windowSize) > 1;
case "last":
return arguments.pg.totalPages > (arguments.pg.currentPage + arguments.windowSize);
}
return true;
```
The `case "previous"` and `case "next"` branches that previously returned `false` at page boundaries have been removed. Both sides now fall through to `return true` in auto mode, so `previousPageLink()` / `nextPageLink()` are always called and render the disabled `` at the boundary by default — matching the legacy `showPrevious=true` / `showNext=true` behavior. Option (b) from the converged reviewer guidance.
Finding 2 (rounds 1 & 2): `windowSize` leaking into anchor sub-helpers — RESOLVED.
`vendor/wheels/view/pagination.cfc`, line 364:
```cfm
local.skipArgs = "handle,navClass,showFirst,showLast,showPrevious,showNext,showInfo,showSinglePage,windowSize,encode";
```
`windowSize` is excluded from the passthrough struct and delivered explicitly to `pageNumberLinks()` at line 393:
```cfm
local.content &= pageNumberLinks(argumentCollection = local.subArgs, windowSize = arguments.windowSize);
```
No duplicate risk — since `windowSize` is in `skipArgs`, it is absent from `local.subArgs` before the named arg is appended.
Tests
Finding 3 (rounds 1 & 2): No specs for Previous/Next auto-mode boundary behavior — RESOLVED.
`vendor/wheels/tests/specs/view/paginationHelpersSpec.cfc`, lines 345–383. Six new `it()` blocks:
- "renders disabled Previous span in auto mode on first page" — page 1/10, asserts `toInclude("Previous")` and `toInclude("disabled")`
- "renders Previous link in auto mode when not on first page" — page 3/10, asserts `toInclude("Previous")` and `toInclude("<a")`
- "renders disabled Next span in auto mode on last page" — page 10/10, asserts `toInclude("Next")` and `toInclude("disabled")`
- "renders Next link in auto mode when not on last page" — page 3/10, asserts `toInclude("Next")` and `toInclude("<a")`
- "hides Previous entirely with never mode on first page"
- "hides Next entirely with never mode on last page"
Minor nit (no action required): the `toInclude("<a")` assertions in the "renders … link" specs are not specific to the Previous/Next link — page number links also emit `<a` elements. The adjacent `toInclude("Previous")` / `toInclude("Next")` assertions carry the weight. Adequate for the test's intent; a combined selector would be more precise but this is not blocking.
Docs
Finding 4 (round 2): CLAUDE.md and upgrade guide silent on prev/next auto-delegation — RESOLVED.
`CLAUDE.md` (HEAD):
Under `"auto"` the previous/next anchors always delegate to `previousPageLink()` / `nextPageLink()` — those render a disabled `` at the boundary so the position indicator is preserved; use `"never"` to hide the boundary indicator entirely.
CHANGELOG.md and `web/sites/guides/src/content/docs/v4-0-1-snapshot/upgrading/3x-to-4x.mdx` carry matching prose. The `v4-0-0` stable guide predates this feature and correctly remains unchanged.
Commits
Three commits:
- `fix(view): paginationNav anchor modes default to window-aware auto-suppress` (`8f54c063`) — type `fix`, scope `view`, 69 chars, DCO present. ✓
- `docs(web/guides): document paginationNav anchor display modes and alwaysShowAnchors parity` (`df9a3d622`) — type `docs`, scope `web/guides`, 68 chars, DCO present. ✓
- `fix(view): address Reviewer A/B consensus findings (round 1)` (`55d46b84b`) — type `fix`, scope `view`, 58 chars, DCO present. Commit body cites each changed file with specific rationale. ✓
Cross-engine
No new concerns. `$paginationAnchorMode` and `$paginationShouldShowAnchor` use only `IsBoolean()`, `ListFindNoCase()`, `LCase()`, `Throw()`, and `switch/case` — standard CFScript that behaves identically on Lucee 6/7, Adobe CF 2023/2025, and BoxLang. Public access on both `$`-prefixed helpers is correct per the `$integrateComponents()` requirement (CLAUDE.md § "Known cross-engine gotchas": `$integrateComponents()` only copies public methods). No `client` scope, no bracket-notation function calls, no struct member function collisions introduced.
Wheels Bot — Reviewer B (round 1)A's review of commit SycophancyNone detected. A did not issue a blanket LGTM — every resolved finding is verified against specific line citations in the diff. False positivesNone detected. I verified each claim:
Missed issuesNone detected. A's minor nit on Verdict alignmentA's approve verdict is consistent with the findings. All four prior-round findings are resolved; no new issues are introduced; commits are commitlint-clean with DCO trailers; no security, migration, deploy, or DI surfaces are touched. ConvergenceAligned with A. The PR is review-clean on SHA |
Manually triggered baseline refresh via .github/workflows/refresh-visual-baselines.yml on branch fix/bot-2716-paginationnav-default-anchor-display-ux-differs-fr. Run when an intentional content/layout change makes the visual-regression check fail. The new PNG(s) under web/tests/visual-baselines/ are now the expected rendering; re-run the failing visual-regression job to flip the check green.
3a1651b to
f289924
Compare
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: The new commit on this PR (f2899246c) is a CI-automated visual baseline refresh — only web/tests/visual-baselines/blog.png changed (binary, 116 714 → 121 482 bytes). All code, test, and documentation changes from the prior three commits were reviewed and approved in round 3. The visual snapshot matches the expected blog-page rendering (navigation bar, blog post cards, RSS link) with no anomalies. No new correctness, convention, cross-engine, security, or commit-hygiene concerns. Verdict: approve.
Correctness
No CFML code changed in this commit. All four findings from previous review rounds were resolved in commit 55d46b84b (round 3 approve). The pagination logic, $paginationAnchorMode, $paginationShouldShowAnchor, windowSize passthrough, and all six new boundary specs remain unchanged at HEAD.
Commits
chore(web): refresh visual baseline(s) (blog) — type chore, scope web, subject 42 chars, not ALL-CAPS, authored by github-actions[bot] via .github/workflows/refresh-visual-baselines.yml. Commitlint-clean. The commit body explains the trigger (manual dispatch on the PR branch) and the update convention (new PNG is now the expected rendering; re-run visual-regression to flip the check green). No DCO trailer required for bot-authored baseline refreshes per the workflow convention.
Wheels Bot — Reviewer B (round 1)A's review of the visual-baseline-refresh commit is well-reasoned and the approve verdict is correct. One unverifiable positive claim is worth naming; no false positives; no missed blocking issues. Aligning and emitting SycophancyNone detected. A does not issue a blanket LGTM — the rationale is explicit: the only change in commit False positivesNone detected, with one caveat worth noting:
Missed issues
No other missed issues. Commitlint is satisfied ( Verdict alignmentA's approve verdict is consistent with the findings. The prior three commits' changes (core pagination logic, tests, CLAUDE.md, CHANGELOG, upgrade guide) were reviewed and approved in round 3. The sole new change is a CI-generated visual baseline PNG. Approve is appropriate. ConvergenceAligned with A. The unverifiable "no anomalies" claim and the uncited DCO exemption are stylistic concerns that do not change the outcome. The PR is review-clean on SHA |
…-display-ux-differs-fr
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: The new commit (e18b6c9e3) is a human-maintainer merge of develop into the PR branch, bringing it up to date before merge. All four findings resolved in rounds 1–3 remain intact; the paginationNav code, test spec, defaults registration, CHANGELOG, CLAUDE.md, and upgrade guide are unchanged from the round-4 approved state. The two CHANGELOG/upgrade-guide conflicts were resolved cleanly — both the PR's paginationNav entry and the develop-side entries appear without duplication or loss. The ~22-file batch from develop (deploy CLI improvements, FakeSshPool, PackagesCommandHelpSpec, web docs) is unrelated to this PR's scope and was already reviewed on develop. Verdict: approve (self-review blocked by GitHub; a human maintainer may merge).
Correctness
All PR code is intact at HEAD:
vendor/wheels/view/pagination.cfc — $paginationAnchorMode and $paginationShouldShowAnchor are present with correct logic; windowSize is in local.skipArgs and delivered explicitly to pageNumberLinks(); case "previous": / case "next": branches are absent from $paginationShouldShowAnchor so both fall through to return true in auto mode (preserving the disabled-span boundary indicator). No regression from the merge.
vendor/wheels/events/init/functions.cfm — defaults remain "auto" for all four anchors and windowSize = 2 is registered.
CHANGELOG conflict resolved correctly: the PR's paginationNav entry and develop's packages-help entry are both present under ### Fixed, in the expected order.
web/sites/guides/src/content/docs/v4-0-1-snapshot/upgrading/3x-to-4x.mdx conflict resolved correctly: the PR's paginationLinks section describing the auto/always/never tri-state API is intact; the develop side only appended new sections (CORS, dedicated changelog page, index reorder).
Tests
vendor/wheels/tests/specs/view/paginationHelpersSpec.cfc — 45 it() blocks at HEAD, unchanged from the round-3 state that introduced the 6 new Previous/Next boundary specs and the updated defaults spec. No test regressions from the merge.
Commits
Five commits on the branch:
fix(view): paginationNav anchor modes default to window-aware auto-suppress(8f54c063) — commitlint-clean, DCO present. ✓docs(web/guides): document paginationNav anchor display modes and alwaysShowAnchors parity(df9a3d622) — commitlint-clean, DCO present. ✓fix(view): address Reviewer A/B consensus findings (round 1)(55d46b84b) — commitlint-clean, DCO present. ✓chore(web): refresh visual baseline(s) (blog)(f2899246c) — commitlint-clean, bot-authored. ✓Merge branch 'develop' into fix/bot-2716-…(e18b6c9e3) — human merge commit by Peter Amiri. DCO not required for merge commits. ✓
Wheels Bot — Reviewer B (round 1)A's review of the merge commit ( SycophancyNone detected. A did not issue a blanket LGTM — each claim cites a specific line or construct in the diff. A correctly noted that GitHub blocks self-review (the review state is COMMENTED, not APPROVED), which is the right way to handle that constraint. False positivesNone detected. I verified each of A's assertions against the actual diff:
Missed issuesNone detected. A's minor nit on the Cross-engine portability: Commits: all five are commitlint-clean with DCO trailers present on bot-authored commits. The merge commit ( Verdict alignmentA's effective approve verdict is consistent with the findings. All four prior-round issues are confirmed resolved; the merge is clean; no new defects introduced. ConvergenceAligned with A. The PR is review-clean on SHA |
…-display-ux-differs-fr
…-display-ux-differs-fr
…-display-ux-differs-fr
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: The two new commits since round 5 (7cd7211f7, 713b9b607) are both human-maintainer merges of develop into the PR branch, pulling in code from #2726 (paginationNav unknown-arg check) and #2727 (paginationLinks deprecation notice). Those changes were already reviewed and approved on develop independently. The integration with this PR's anchor-mode code is clean: validation order is correct, windowSize is in skipArgs and never lands in unknownArgs, and the early-return reorganisation is sound. One minor doc nit: CLAUDE.md's "Accepted pass-through keys" list includes windowSize, but windowSize is now a direct paginationNav() parameter (in skipArgs), not a sub-helper passthrough. Functionally harmless — passing windowSize to paginationNav() still works correctly and will not trigger InvalidArgument. All findings from rounds 1–5 remain resolved. Verdict: approve (self-review blocked by GitHub; a human maintainer may merge).
Correctness
All four findings resolved in round 3 remain intact at HEAD. Specific verification against the new merges:
Validation order (vendor/wheels/view/pagination.cfc, lines 348–435): The develop-side unknown-arg check from #2726 runs before local.pg = pagination(arguments.handle), which is intentional (comment: "Validate before the totalPages early-return so the check fires on single-page or empty result sets too"). The PR's $paginationAnchorMode and $paginationShouldShowAnchor calls remain after the early-return for single-page results — correct, since if the function returns "" before rendering, no anchor mode resolution is needed.
windowSize double-pass risk eliminated: windowSize is in local.skipArgs (line 363, from round-1 fix) and absent from local.allowedSubArgs (line 367, from #2726). The for-loop skips it, so it never enters local.unknownArgs. windowSize is delivered explicitly to pageNumberLinks() only (line 439). No regression.
showFirst="bogus" interaction with unknown-arg check: showFirst is in skipArgs and therefore excluded from the unknown-arg loop entirely. A bad anchor mode string will not trip Wheels.PaginationNav.InvalidArgument; it will trip Wheels.InvalidArgument from $paginationAnchorMode — but only after the early-return guard, so single-page result sets silently return "" without throwing. Acceptable semantics.
Docs
Minor nit (no action required): CLAUDE.md at HEAD:
Accepted pass-through keys:
format,text,name,class,disabledClass,showDisabled,pageNumberAsParam,windowSize,classForCurrent, ...
windowSize is listed as a pass-through key, but it is a direct paginationNav() parameter (declared in the function signature; present in local.skipArgs). It is not forwarded to sub-helpers via argumentCollection — it is delivered explicitly to pageNumberLinks(). A developer reading "pass-through key" might infer that windowSize would reach all sub-helpers, which is not the case. That said, the functional contract is correct: passing windowSize to paginationNav() is accepted and works. This line was introduced by the develop-side #2726 commit before this PR's windowSize parameter existed on develop; now that the merge has landed, the label is slightly imprecise. Worth a fixup ("direct parameter" or "own arg") in a follow-up but does not block this PR.
Tests
vendor/wheels/tests/specs/view/paginationHelpersSpec.cfc at HEAD has 45 it() blocks across the paginationNav describe tree. $clearRoutes() is defined at the bottom of the CFC (line 463), satisfying the CLAUDE.md requirement. The route-segment regression test (paginationNav(route="userTimeline", userId=42)) correctly saves and restores application.wheels.routes, staticRoutes, namedRoutePositions, and URLRewriting in the finally block. No test-isolation concerns.
vendor/wheels/tests/specs/view/paginationLinksDeprecationSpec.cfc (from #2727 develop merge): uses beforeEach/afterEach to clean the request-scoped deprecation guard flag. The ##2714 in the describe label correctly renders as #2714 at runtime — valid CFML escaping.
Cross-engine
links.cfc deprecation guard (StructKeyExists(request.wheels, "$paginationLinksDeprecationLogged")) is safe across all engines: request.wheels is always present by the time view helpers execute (initialized in onRequestStart.cfm). No client scope, no bracket-notation function calls, no struct member function collisions introduced by either develop merge.
Commits
Two new merge commits since round 5:
Merge branch 'develop' into fix/bot-2716(7cd7211f7) — human merge by Peter Amiri, DCO not required for merge commits. ✓Merge branch 'develop' into fix/bot-2716(713b9b607) — same. ✓
All five bot-authored commits on the branch remain commitlint-clean with DCO trailers present (verified in rounds 3–5).
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: The single new commit since round 6 (61ef732a7) is another human-maintainer merge of develop into the PR branch, pulling in PR #2732 (feat(config): auto-register per-package CFML mapping in PackageLoader). That change is entirely orthogonal to this PR's pagination scope — vendor/wheels/view/pagination.cfc, the test spec, the defaults in functions.cfm, and the upgrade guide are untouched. CHANGELOG and CLAUDE.md conflicts were resolved cleanly: the PR's pagination entries remain intact alongside the new PackageLoader mapping entry. All four findings resolved in rounds 1–3 remain resolved at HEAD. Verdict: approve (self-review blocked by GitHub; a human maintainer may merge).
Correctness
All PR code is intact at HEAD:
vendor/wheels/view/pagination.cfc, lines 495–501:
switch (arguments.side) {
case "first":
return (arguments.pg.currentPage - arguments.windowSize) > 1;
case "last":
return arguments.pg.totalPages > (arguments.pg.currentPage + arguments.windowSize);
}
return true;No case "previous": / case "next": branches — both sides fall through to return true in auto mode, preserving the disabled-span boundary indicator via previousPageLink() / nextPageLink(). Correct.
local.skipArgs at line 358 still contains "windowSize", and windowSize is delivered explicitly to pageNumberLinks() only. No regression from the merge.
Commits
Six bot-authored commits on the branch, all commitlint-clean with DCO trailers (verified in rounds 3–6):
fix(view): paginationNav anchor modes default to window-aware auto-suppress(8f54c063) ✓docs(web/guides): document paginationNav anchor display modes and alwaysShowAnchors parity(df9a3d622) ✓fix(view): address Reviewer A/B consensus findings (round 1)(55d46b84b) ✓chore(web): refresh visual baseline(s) (blog)(f2899246c) ✓- Merge commits (
e18b6c9e3,7cd7211f7,713b9b607,61ef732a7) — human-authored, DCO not required. ✓
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This PR correctly widens paginationNav()'s four show* args to a tri-state "auto" / "always" / "never" API with backwards-compatible boolean coercion, defaults all four to "auto", and promotes windowSize to a first-class named argument so the auto-mode predicates stay coherent with pageNumberLinks()'s window. The window boundary math is correct, the cross-engine safety rules are followed (public access + $ prefix for both new internal functions), and the test suite is comprehensive. There are two documentation nits worth fixing before merge but no correctness or cross-engine issues.
Correctness
The $paginationShouldShowAnchor window predicates are correct:
- First:
(currentPage - windowSize) > 1— hides First whencurrentPage <= windowSize + 1, i.e. page 1 is inside the window. ✓ - Last:
totalPages > (currentPage + windowSize)— hides Last whencurrentPage >= totalPages - windowSize, i.e. the last page is inside the window. ✓ - Previous/Next under
"auto": always returnstrue, delegating to the sub-helpers which render a disabled<span class="disabled">at boundaries. This correctly preserves the legacyshowPrevious=trueboundary indicator. ✓
One minor consistency gap: $paginationAnchorMode validation is silently skipped when totalPages <= 1 and showSinglePage=false because paginationNav() returns "" before reaching the validation calls (lines 456–463 of pagination.cfc). Calling paginationNav(showFirst="bogus") on an empty result set returns "" instead of throwing Wheels.InvalidArgument. The unknown-argument check at lines 416–450 fires early (before the single-page guard), but mode-string validation does not.
// pagination.cfc ~line 456 — early return precedes $paginationAnchorMode calls
if (local.pg.totalPages <= 1 && !arguments.showSinglePage) {
return "";
}
// $paginationAnchorMode called here — never reached for empty/single-page when showSinglePage=false
local.firstMode = $paginationAnchorMode(value = arguments.showFirst, argName = "showFirst");The test at line 534 of the spec only exercises the multi-page path (totalRecords=100). Adding a counterpart test for totalPages=1 + invalid mode would close the gap, but the practical impact is zero — if there are no pages, invalid mode values are never acted upon anyway.
Docs
1. CLAUDE.md "Accepted pass-through keys" still lists windowSize (stale)
vendor/wheels/view/pagination.cfc, local.skipArgs (line 405):
local.skipArgs = "handle,navClass,showFirst,showLast,showPrevious,showNext,showInfo,showSinglePage,windowSize,...";windowSize is no longer a pass-through — it is a declared numeric windowSize first-class argument of paginationNav(), consumed internally and forwarded explicitly to pageNumberLinks(). The PR removes it from local.allowedSubArgs (correct), but the CLAUDE.md "Accepted pass-through keys" paragraph is unchanged and still includes windowSize in the list:
Accepted pass-through keys:
format,text,name,class,disabledClass,showDisabled,pageNumberAsParam,windowSize, ...
windowSize should be removed from that list and described as a first-class paginationNav() argument (like navClass or showInfo). Functionally harmless — windowSize still works — but a future contributor reading CLAUDE.md will form the wrong mental model about what "pass-through" means here.
2. Bootstrap 5 CLAUDE.md example does not reflect the new default behavior
The Bootstrap 5 swap example in CLAUDE.md is unchanged by this PR:
// Bootstrap 5 — like-for-like swap for legacy paginationLinks() Bootstrap markup
#paginationNav(
navClass="",
prepend='<ul class="pagination">',
append="</ul>",
prependToPage='<li class="page-item">',
appendToPage="</li>",
class="page-link",
classForCurrent="active",
addActiveClassToPrependedParent=true
)#With the new "auto" default, First/Last are now hidden when the window reaches the boundary — which is actually more faithful to 3.x alwaysShowAnchors=false semantics. But a developer migrating from 3.x with alwaysShowAnchors=true (which was the old paginationLinks() default) would need showFirst="always" / showLast="always" to restore the always-visible behavior. The upgrade guide (3x-to-4x.mdx) covers this correctly; the CLAUDE.md snippet does not. Adding a comment line to the example would complete the picture.
Tests
Test coverage is thorough: auto mode at boundaries and away from boundaries, "always", "never", boolean coercion, windowSize propagation, disabled Previous/Next spans on boundary pages, and the Wheels.InvalidArgument throw. The existing test for "includes all sections by default" was correctly updated to use g.setPagination() with an explicit mid-range page so the auto-mode predicates are well-defined regardless of the seeded author count.
Commits
All substantive commits conform to commitlint: fix(view):, docs(web/guides):, chore(web):. Merge-sync commits are auto-generated and exempt.
- Remove `windowSize` from the "Accepted pass-through keys" list — it is a first-class `paginationNav()` argument (in `skipArgs`), not a sub-helper passthrough. Document its first-class status explicitly so future contributors form the right mental model about what "pass-through" means in the validation context. - Annotate the Bootstrap 5 like-for-like swap example with a one-line note that callers wanting 3.x `alwaysShowAnchors=true` behaviour should pass `showFirst=\"always\"` / `showLast=\"always\"`. The upgrade guide already covers this; the CLAUDE.md quick-reference example did not. Signed-off-by: Peter Amiri <petera@pai.com>
Wheels Bot — Reviewer B (round 1)A’s review is substantively accurate. Both documentation findings are real, the correctness analysis is sound, and no cross-engine issues were missed. Aligned with A’s direction; the two doc fixes are concrete enough for SycophancyNone detected. A did not approve without evidence; the False positivesNone detected.
Missed issuesOne minor procedural note: A explicitly called these findings “worth fixing before merge” but submitted a No cross-engine issues were missed. The two new internal functions ( Verdict alignmentA’s ConvergenceAligned. Joint recommendation: apply two
Triggering address-review to apply both changes. |
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This iteration addresses all three findings from the previous review (round 1). The showPrevious/showNext auto-mode now correctly delegates to the sub-helpers that render disabled spans at boundaries; windowSize is correctly wired as a first-class paginationNav() arg rather than a passthrough leak; and the test suite is thorough. Two minor observations remain — one inconsistency in validation placement and one DCO note on the head commit — neither blocks merge.
Correctness
1. Anchor-mode validation fires after the single-page early-return (minor)
vendor/wheels/view/pagination.cfc, lines 456–463:
// Return empty if only one page and showSinglePage is false
if (local.pg.totalPages <= 1 && !arguments.showSinglePage) {
return "";
}
local.firstMode = $paginationAnchorMode(value = arguments.showFirst, argName = "showFirst");$paginationAnchorMode — which unconditionally throws Wheels.InvalidArgument for bad strings — is only reached when there is more than one page. On a single-page result set with showSinglePage = false, a typo like showFirst="alwys" silently returns "" rather than throwing.
Compare with the unknown-arg validation at lines 428–451, which carries an explicit comment: "Validate before the totalPages early-return so the check fires on single-page (or empty) result sets too." The anchor-mode check should follow the same rationale. The fix is simply to call $paginationAnchorMode for all four args before the early-return, then use the normalised mode values in the rest of the function.
Note: unlike the unknown-arg check, $paginationAnchorMode has no showErrorInformation gate — it throws in production too. Moving it before the early-return would mean a production app with a one-page result and a bad mode string throws instead of returning "". Whether that is intentional (fail loud) or should be gated is a design call, but the current behaviour is inconsistent with the comment on the unknown-arg check and will confuse future contributors.
Tests
2. windowSize propagation to pageNumberLinks is untested
vendor/wheels/tests/specs/view/paginationHelpersSpec.cfc, lines around the windowSize test:
it("respects windowSize when computing auto mode predicates", () => {
g.setPagination(totalRecords = 100, currentPage = 5, perPage = 10)
result = _controller.paginationNav(windowSize = 4)
expect(result).notToInclude("First")
})This test verifies only the auto-mode side effect (First is hidden because the wider window reaches page 1). It does not verify that windowSize = 4 is passed through to pageNumberLinks() — i.e., that the rendered output contains 9 numbered page links (pages 1–9) rather than the default 5 (pages 3–7).
If someone removed the explicit pageNumberLinks(argumentCollection = local.subArgs, windowSize = arguments.windowSize) line at line 493 and the window fell back to its own $args() default, this test would still pass. A companion assertion like expect(result).toInclude(">8<") (checking that page 8 is rendered as a link) would guard against that regression.
Commits
3. Head-commit DCO email mismatch (observation)
The head commit 6b379e7fe has git author email peter@alurium.com but the Signed-off-by trailer uses petera@pai.com:
Signed-off-by: Peter Amiri <petera@pai.com>
The DCO GitHub App compares the trailer against the contributor's registered GitHub account emails. If both addresses are registered the check passes; if not, the required DCO status check will block the merge. Worth confirming before this lands.
What was fixed since the last review
showPrevious/showNextauto mode now returnstruefrom$paginationShouldShowAnchor(via the switch fall-through), always delegating topreviousPageLink()/nextPageLink(). Those helpers render the disabled<span class="disabled">at page boundaries by default, preserving the legacy indicator. Previously they returnedfalseat boundaries, silently dropping the spans. ✓windowSizemoved fromallowedSubArgstoskipArgsand passed explicitly topageNumberLinks(). The auto-mode predicates in$paginationShouldShowAnchoruse the same value, keeping the boundary detection coherent with the rendered window. ✓- Tests for
showPrevious/showNextauto-mode boundary behaviour (disabled spans + active links) added. ✓
Wheels Bot — Address Review (round 2)Reviewer A and Reviewer B converged on
Both findings are already addressed at the current head (
No additional commit pushed by the bot this round — the diff would be empty. Posting this comment with the address-review marker so the convergence loop closes cleanly on the current SHA rather than re-firing. |
Wheels Bot — Reviewer B (round 1)A's review is technically accurate across all three findings. I verified both code claims against the diff and the live file. Two findings are concrete and bot-addressable; the DCO observation requires human follow-through. Converging on changes for the two code findings. SycophancyNone detected. A issued a COMMENTED state and explicitly called out two code-level issues before noting they don't block merge. False positivesNone detected.
Missed issuesNone of consequence. A few minor observations that do not require changes:
Verdict alignmentA's COMMENTED state is consistent with the findings — neither is a functional regression, so not requesting changes is defensible. Both are concrete and mechanical, however, so converging on changes is the cleaner outcome that keeps the codebase consistent. ConvergenceA and B are aligned. Two changes should be applied:
Finding 3 (DCO email mismatch) is for the committer: confirm |
- Move $paginationAnchorMode validation calls for showFirst/showLast/
showPrevious/showNext above the totalPages early-return in
paginationNav() so invalid mode strings throw on single-page or
empty result sets too. Matches the unknown-arg validation's stated
rationale a few lines above; eliminates the silent-success on
totalPages <= 1 that B flagged.
- Strengthen the windowSize propagation spec in paginationHelpersSpec
with expect(result).toInclude(">8<") so the test fails if windowSize
is silently dropped from the pageNumberLinks() call. The existing
notToInclude("First") alone could not catch a regression there
because the First-hide predicate reads arguments.windowSize directly.
Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Wheels Bot — Address Review (round 3)Applied the two new consensus findings from Reviewer A and Reviewer B's convergence on SHA
Skipped (out of bot scope)
Verification Local Lucee 7 + SQLite test endpoint ( The new commit will trigger a fresh Reviewer A run on the updated SHA. Convergence loop continues until reviewers align on |
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: The round-3 commit (e7d0fcf09) resolves both findings carried from the previous round. $paginationAnchorMode validation is now placed before the totalPages early-return, so a bad mode string throws on single-page or empty result sets as well as multi-page ones. The windowSize propagation spec is strengthened with expect(result).toInclude(">8<"), making the regression guard tight. The develop merge (6c6dd4a54) brings in the XSS security suite and CLI/package improvements — orthogonal to this PR's scope and already reviewed on develop. The PR is clean. Verdict: approve (self-review blocked by GitHub; a human maintainer may merge).
Correctness
Finding 1 (round 9): Anchor-mode validation fires after single-page early-return — RESOLVED.
vendor/wheels/view/pagination.cfc, lines 458-461 at HEAD:
local.firstMode = $paginationAnchorMode(value = arguments.showFirst, argName = "showFirst");
local.lastMode = $paginationAnchorMode(value = arguments.showLast, argName = "showLast");
local.previousMode = $paginationAnchorMode(value = arguments.showPrevious, argName = "showPrevious");
local.nextMode = $paginationAnchorMode(value = arguments.showNext, argName = "showNext");
local.pg = pagination(arguments.handle);
// Return empty if only one page and showSinglePage is false
if (local.pg.totalPages <= 1 && !arguments.showSinglePage) {
return "";
}Validation now runs before pagination(arguments.handle) and the single-page guard. A typo like showFirst="bogus" will throw Wheels.InvalidArgument regardless of totalPages. The code comment at lines 453-457 explains the rationale and the absence of a showErrorInformation gate. Correct.
Tests
Finding 2 (round 9): windowSize propagation to pageNumberLinks untested — RESOLVED.
vendor/wheels/tests/specs/view/paginationHelpersSpec.cfc, lines 474-484:
it("respects windowSize when computing auto mode predicates", () => {
g.setPagination(totalRecords = 100, currentPage = 5, perPage = 10)
result = _controller.paginationNav(windowSize = 4)
expect(result).notToInclude("First")
// windowSize=4 expands the rendered page-number window to pages 1-9 ...
expect(result).toInclude(">8<")
})The added toInclude(">8<") assertion verifies that windowSize reaches pageNumberLinks(). If the explicit windowSize = arguments.windowSize line at pagination.cfc line 495 were removed and the arg fell back to the sub-helper's default (2), the window would collapse to pages 3-7 and page 8 would disappear from output — this assertion catches that regression. The notToInclude("First") alone could not.
Commits
The commit history since the last approved SHA (6b379e7fe) is:
6c6dd4a54—Merge branch 'develop' into fix/bot-2716...(human merge by Peter Amiri). DCO not required for merge commits. Conflict resolutions are clean:pagination.cfcmerge correctly places the anchor-mode changes alongside the develop-side unknown-arg validation;paginationHelpersSpec.cfcmerge retains all 45+it()blocks; CHANGELOG and upgrade-guide conflict resolutions are without duplication or loss. Passes.e7d0fcf09—fix(view): address Reviewer A/B consensus findings (round 3). Authorclaude[bot] <41898282+claude[bot]@users.noreply.github.com>, DCO trailer matches. Typefix, scopeview, 60-char subject. Passes.
The DCO concern from round 9 (Peter Amiri signing with petera@pai.com while committing as peter@alurium.com) applied to commit 6b379e7fe. That commit remains in history. If the DCO App reports a failure on it, the simplest remedy is for Peter to add the alternate email to his GitHub account — no code change needed.
Cross-engine
No new concerns from either commit. $paginationAnchorMode and $paginationShouldShowAnchor continue to use only IsBoolean(), ListFindNoCase(), LCase(), Throw(), and switch/case — standard CFScript that behaves identically on Lucee 6/7, Adobe CF 2023/2025, and BoxLang. Both helpers retain public access with $ prefix, satisfying the $integrateComponents() requirement (CLAUDE.md: $integrateComponents() only copies public methods).
Wheels Bot — Reviewer B (round 1)A's review is sound. All cited code matches the live file, both prior findings are genuinely resolved, and the verdict is consistent with the evidence. Converging on approve. SycophancyNone detected. The approve verdict is backed by specific line citations and mechanistic reasoning for each finding — not a blanket LGTM. False positivesNone detected.
Missed issuesNone detected. Fresh scan of the diff for areas A did not explicitly call out:
Verdict alignmentA's ConvergenceAligned. No remaining disputes. Joint recommendation: approve and merge when the DCO check on the Peter Amiri commit ( |
…-display-ux-differs-fr
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: The single new commit since round 10 (8f0a8cc05) is a human-maintainer merge of develop into the PR branch, pulling in PR #2735 (feat(web/blog): wheels + claude stdio MCP post + OpenCode template fix). That change is entirely orthogonal to this PR's pagination scope — vendor/wheels/view/pagination.cfc, the test spec, functions.cfm defaults, and all docs from previous rounds are untouched. The CHANGELOG conflict was resolved cleanly. All findings resolved in rounds 1–10 remain intact at HEAD. Verdict: approve (self-review blocked by GitHub; a human maintainer may merge).
Correctness
All PR code is intact at HEAD.
vendor/wheels/view/pagination.cfc, lines 492–501:
switch (arguments.side) {
case "first":
return (arguments.pg.currentPage - arguments.windowSize) > 1;
case "last":
return arguments.pg.totalPages > (arguments.pg.currentPage + arguments.windowSize);
}
return true;No case "previous": / case "next": branches — both fall through to return true in auto mode, preserving the disabled-span boundary indicator. local.skipArgs at line 358 still includes "windowSize"; it is delivered explicitly to pageNumberLinks() only. Anchor-mode validation ($paginationAnchorMode) still fires before pagination(arguments.handle) and the single-page early-return. No regression from the merge.
vendor/wheels/events/init/functions.cfm — defaults remain "auto" for all four anchors and windowSize = 2. ✓
vendor/wheels/tests/specs/view/paginationHelpersSpec.cfc — unchanged from round 3; all 45+ it() blocks, including the six Previous/Next boundary specs and the strengthened windowSize propagation assertion (toInclude(">8<")). ✓
CHANGELOG conflict resolved cleanly: the PR's paginationNav tri-state entry appears first under ### Fixed, the new #2735 wheels mcp setup / OpenCode template fix entry follows immediately. No duplication or loss.
Commits
New commits since round 10:
30fb11edd—feat(web/blog): wheels + claude stdio MCP post + OpenCode template fix— human-authored by Peter Amiri, merged todevelopas PR #2735. Commitlint-clean: typefeat, scopeweb/blog, subject 68 chars, not ALL-CAPS. ✓8f0a8cc05—Merge branch 'develop' into fix/bot-2716-…— human merge commit by Peter Amiri. DCO not required for merge commits. ✓
All bot-authored commits on the branch (8f54c063, df9a3d622, 55d46b84b, e7d0fcf09) remain commitlint-clean with DCO trailers present (verified in rounds 3–10).
Wheels Bot — Reviewer B (round 1)A's review is mostly accurate. The orthogonality claim checks out, the CHANGELOG conflict assessment is correct, and the approve verdict is appropriate for the change reviewed. One line-number citation is off, and the outstanding DCO concern from prior rounds is carried implicitly rather than explicitly. SycophancyNone detected. The approve verdict is backed by specific code-level verification of unchanged pagination files and the CHANGELOG conflict resolution — not a blanket LGTM. False positivesOne minor citation error:
Missed issuesOne procedural gap:
Verdict alignmentA's effective approve verdict is consistent with the reviewed change: the two new commits are orthogonal to the pagination scope, the CHANGELOG conflict was resolved cleanly, and no regressions were introduced. ConvergenceAligned with A. The DCO gap is human-only (maintainer adds |
Manually triggered baseline refresh via .github/workflows/refresh-visual-baselines.yml on branch fix/bot-2716-paginationnav-default-anchor-display-ux-differs-fr. Run when an intentional content/layout change makes the visual-regression check fail. The new PNG(s) under web/tests/visual-baselines/ are now the expected rendering; re-run the failing visual-regression job to flip the check green.
d9c2345 to
dfd2725
Compare
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: The single new commit since round 11 (dfd27257c) is a CI-automated visual baseline refresh — only web/tests/visual-baselines/blog.png changed (binary, 121171 → 115992 bytes). This is the same mechanical pattern as the round-4 baseline refresh (f2899246c), triggered by the develop-merge of PR #2735 (the new blog post) which altered the page layout enough to fail the visual regression check. No CFML code, test logic, or documentation was touched. All findings resolved in rounds 1–10 remain intact at HEAD. Verdict: approve (self-review blocked by GitHub; a human maintainer may merge).
Correctness
No CFML code changed in this commit. All four core findings resolved across rounds 1–10 remain intact:
$paginationShouldShowAnchor— nocase "previous":/case "next":branches; both fall through toreturn truein auto mode, preserving disabled-span boundary indicators. ✓local.skipArgsstill contains"windowSize";windowSizeis delivered explicitly topageNumberLinks()only. ✓$paginationAnchorModevalidation fires beforepagination(arguments.handle)and the single-page early-return. ✓windowSizepropagation spec carries thetoInclude(">8<")assertion that guards against regression. ✓
vendor/wheels/events/init/functions.cfm — defaults remain "auto" for all four anchors and windowSize = 2. ✓
vendor/wheels/tests/specs/view/paginationHelpersSpec.cfc — unchanged from round 3; all 45+ it() blocks present. ✓
Commits
chore(web): refresh visual baseline(s) (blog) (dfd27257c) — authored by github-actions[bot] via .github/workflows/refresh-visual-baselines.yml. Type chore, scope web, subject 42 chars, not ALL-CAPS — commitlint-clean. DCO trailer not required for bot-authored baseline refreshes per workflow convention.
All four prior bot-authored commits (8f54c063, df9a3d622, 55d46b84b, e7d0fcf09) remain commitlint-clean with DCO trailers present (verified in rounds 3–10). ✓
Wheels Bot — Reviewer B (round 1)A's review is accurate and appropriately scoped. The single change is a CI-generated visual baseline PNG, identical in pattern to the round-4 refresh. Every code-level and commit-level claim I can verify matches the actual diff. Converging on approve. SycophancyNone detected. A verified each of the four prior-round findings at their current line locations rather than issuing a blanket carry-forward. False positivesNone detected.
Missed issuesNone detected. The outstanding DCO concern for commit Verdict alignmentApprove is consistent with the findings. The prior three commits' substantive changes (pagination logic, tests, CLAUDE.md, CHANGELOG, upgrade guide) were reviewed and approved in rounds 3–10. The sole new change is a routine visual baseline PNG refresh. ConvergenceAligned with A. No changes required before merge. |
…-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>
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: The new merge commit (0c79f8652) integrates three develop PRs — #2730 (Bootstrap-style wrappers), #2731 (viewStyle presets), and #2736 (QueryBuilder empty-IN fix) — into this branch. The conflict resolution correctly wires the tri-state anchor modes through $paginationShouldShowAnchor() on the viewStyle path. However, a gap in the integration means windowSize is never forwarded to pageNumberLinks() when a non-plain viewStyle is used. All findings from rounds 1–12 remain resolved; this is a new correctness issue introduced by the merge. This review would be request-changes; a human maintainer should act on finding 1 before merging.
Correctness
windowSize not forwarded to pageNumberLinks() inside $renderPaginationNav() (medium)
vendor/wheels/view/pagination.cfc, lines 506–515 (the viewStyle early-return path):
return $renderPaginationNav(
viewStyle = arguments.viewStyle,
pg = local.pg,
showInfo = arguments.showInfo,
showFirst = $paginationShouldShowAnchor(mode = local.firstMode, side = "first", pg = local.pg, windowSize = arguments.windowSize),
showPrevious = $paginationShouldShowAnchor(mode = local.previousMode, side = "previous", pg = local.pg, windowSize = arguments.windowSize),
showNext = $paginationShouldShowAnchor(mode = local.nextMode, side = "next", pg = local.pg, windowSize = arguments.windowSize),
showLast = $paginationShouldShowAnchor(mode = local.lastMode, side = "last", pg = local.pg, windowSize = arguments.windowSize),
subArgs = local.subArgs
);windowSize is passed to all four $paginationShouldShowAnchor() calls — correctly governing the First/Last auto-mode predicates. But windowSize is not passed to $renderPaginationNav() itself. Inside that function (lines 814–886):
// Reuse pageNumberLinks() so the window logic stays in one place.
local.pageArgs = StructCopy(arguments.subArgs);
local.pageArgs.viewStyle = arguments.viewStyle;
local.items &= pageNumberLinks(argumentCollection = local.pageArgs); // line 850windowSize is in local.skipArgs (line 435), so it is absent from local.subArgs and therefore absent from local.pageArgs. pageNumberLinks() falls back to its own $args() default of windowSize = 2 (confirmed in functions.cfm line 302).
Effect: paginationNav(viewStyle="bootstrap5", windowSize=4) will:
- Correctly hide First when
(currentPage - 4) <= 1(auto-mode predicate useswindowSize=4) - Incorrectly render page number links with a window of 2 on each side (the
pageNumberLinksdefault), not 4
The auto-mode predicate and the rendered window are now evaluated against different windowSize values, making the boundary-suppression logic incoherent for non-plain viewStyles with a non-default windowSize.
This is a gap in the merge resolution. $renderPaginationNav was introduced by PR #2731 without knowledge of the windowSize parameter (which was added by this PR, #2733). The conflict resolution correctly updated the $paginationShouldShowAnchor() calls to use arguments.windowSize, but missed adding windowSize to the $renderPaginationNav() call and signature.
Suggested fix:
- Add
numeric windowSizeto$renderPaginationNav()'s signature (line 814). - Add
windowSize = arguments.windowSizeto the$renderPaginationNav()call at line 506. - Set
local.pageArgs.windowSize = arguments.windowSizebefore thepageNumberLinks()call at line 850.
Tests
No test for windowSize + non-plain viewStyle combination
The existing windowSize propagation test (spec around line 474):
it("respects windowSize when computing auto mode predicates", () => {
g.setPagination(totalRecords = 100, currentPage = 5, perPage = 10)
result = _controller.paginationNav(windowSize = 4)
expect(result).notToInclude("First")
expect(result).toInclude(">8<")
})This test uses the default viewStyle="plain" path. The viewStyle path goes through $renderPaginationNav() and is untested for windowSize propagation. A companion spec like:
it("forwards windowSize to pageNumberLinks in Bootstrap 5 viewStyle", () => {
g.setPagination(totalRecords = 100, currentPage = 5, perPage = 10)
result = _controller.paginationNav(viewStyle = "bootstrap5", windowSize = 4)
expect(result).toInclude(">8<")
})would have caught finding 1 and should be added alongside the fix.
Commits
The new merge commit (0c79f8652) is a human-maintainer merge by Peter Amiri. DCO not required for merge commits. The conflict resolutions for CHANGELOG.md, CLAUDE.md, functions.cfm, pagination.cfc, and the test spec are all sound — the only gap is the windowSize forward described above.
All four bot-authored commits on the branch (8f54c063, df9a3d622, 55d46b84b, e7d0fcf09) remain commitlint-clean with DCO trailers present (verified in rounds 3–12).
Cross-engine
No new concerns from the merged code. StructCopy(), NumberFormat(), EncodeForHTML(), Max() / Min(), and switch/case in the new $renderPaginationPage / $renderPaginationNav / $renderPaginationNavLink helpers are standard CFML that behaves identically on Lucee 6/7, Adobe CF 2023/2025, and BoxLang. All three new internal functions carry public access with $ prefix, satisfying the $integrateComponents() requirement (CLAUDE.md: $integrateComponents() only copies public methods). No client scope, no bracket-notation function calls introduced.
Wheels Bot — Reviewer B (round 1)A's review is accurate and well-grounded. The single correctness finding is real, the suggested fix is mechanically correct, and the test-gap observation is valid. Aligning with A's request-changes direction and emitting SycophancyNone detected. A's TL;DR explicitly calls for human action before merge and the review is critical throughout — no blanket LGTM. False positivesNone detected. I verified every substantive claim against the live file:
Missed issuesNone of significance. The plain-style path is clean; the issue is isolated to the Verdict alignmentA's effective request-changes verdict is consistent with the finding. ConvergenceAligned with A. Two concrete changes are needed:
|
Forward `windowSize` to `pageNumberLinks()` on the `paginationNav()` viewStyle path so the auto-mode predicate and the rendered window are evaluated against the same value. - `vendor/wheels/view/pagination.cfc`: add `numeric windowSize` to `$renderPaginationNav()` signature, pass `windowSize` at the call site, and copy it into `local.pageArgs` before invoking `pageNumberLinks()`. - `vendor/wheels/tests/specs/view/paginationHelpersSpec.cfc`: add a regression-guard spec under "paginationNav with viewStyle presets" asserting that `paginationNav(viewStyle="bootstrap5", windowSize=4)` with `currentPage=5, totalRecords=100, perPage=10` includes page 8 in the output. Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Wheels Bot — Address Review (round 4)Applied consensus findings from Reviewer A and Reviewer B's convergence (round 13 of the review loop):
Tests: The new commit will trigger a fresh Reviewer A run on the updated SHA. Convergence loop continues until reviewers align on |
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: Commit 21c32a8b3 ("fix(view): address Reviewer A/B consensus findings (round 4)") resolves the single outstanding finding from round 13. windowSize is now correctly forwarded to pageNumberLinks() on the paginationNav() viewStyle path, closing the incoherence between the auto-mode predicate and the rendered window for non-plain viewStyles with a non-default windowSize. The fix is minimal, correctly targeted, and accompanied by the regression-guard spec requested in round 13. All findings from rounds 1–13 remain resolved. Verdict: approve (blocked by GitHub self-review rule — a human maintainer should merge).
Correctness
Finding 13.1 — windowSize not forwarded on the viewStyle path (resolved)
The three-step fix in vendor/wheels/view/pagination.cfc:
required numeric windowSizeadded to$renderPaginationNav()signature (line 823).windowSize = arguments.windowSizepassed at the$renderPaginationNav()call site (line 514).local.pageArgs.windowSize = arguments.windowSizeset before thepageNumberLinks()call (line 854).
This is exactly what was prescribed. Before this commit, windowSize was excluded from local.subArgs via skipArgs (line 435), so local.pageArgs inside $renderPaginationNav() never held it and pageNumberLinks() fell back to its default of 2. The auto-mode predicate and the rendered window were evaluated against different windowSize values. Both now use the same caller-supplied arguments.windowSize end-to-end.
Verified against the current live file: the plain path at line 547 (pageNumberLinks(argumentCollection = local.subArgs, windowSize = arguments.windowSize)) was already correct from earlier commits; the viewStyle path is now in parity.
Tests
Finding 13.2 — no test for windowSize + non-plain viewStyle (resolved)
The new spec under describe("paginationNav with viewStyle presets") (lines 372–382):
it("forwards windowSize to pageNumberLinks() on the viewStyle path", () => {
g.setPagination(totalRecords = 100, currentPage = 5, perPage = 10)
result = _controller.paginationNav(viewStyle = "bootstrap5", windowSize = 4)
expect(result).toInclude(">8<")
})The assertion is sound: with windowSize=4, endPage = Min(10, 5+4) = 9, so page 8 is in the rendered window; with the former windowSize=2 default, endPage = Min(10, 5+2) = 7, so page 8 would be absent. This spec would have caught finding 13.1 before merge and correctly guards against regression.
Cross-engine
No new concerns. The three-line change is standard CFML — a named function parameter declaration, a named-argument call, and a struct key assignment. All three behave identically on Lucee 6/7, Adobe CF 2023/2025, and BoxLang. $renderPaginationNav() carries public access with $ prefix, satisfying the $integrateComponents() requirement (CLAUDE.md: $integrateComponents() only copies public methods). No client scope, no bracket-notation calls, no struct.map() collisions.
Commits
fix(view): address Reviewer A/B consensus findings (round 4) — type fix, scope view, subject under 100 chars, not ALL-CAPS. DCO trailer present: Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>.
All four earlier bot-authored commits (8f54c063, df9a3d622, 55d46b84b, e7d0fcf09) remain commitlint-clean with DCO trailers confirmed in prior rounds.
Manually triggered baseline refresh via .github/workflows/refresh-visual-baselines.yml on branch fix/bot-2716-paginationnav-default-anchor-display-ux-differs-fr. Run when an intentional content/layout change makes the visual-regression check fail. The new PNG(s) under web/tests/visual-baselines/ are now the expected rendering; re-run the failing visual-regression job to flip the check green.
Wheels Bot — Reviewer B (round 1)A's review is accurate and tightly verified. All three cited code locations match the actual live file at the lines A states. The test assertion math (windowSize=4 renders pages 1-9, page 8 present; windowSize=2 renders pages 3-7, page 8 absent) is correct. The approve verdict is consistent with the evidence - round 4 of this PR closed the last remaining finding without introducing new concerns, and A gave no LGTM without evidence. SycophancyNone detected. A audited all three changed lines by number, verified the plain-path parity claim, and confirmed the spec logic before recommending approval. No rubber-stamp language. False positivesNone detected.
Missed issuesNone found.
Verdict alignmentAligned. The fix is correct, the regression spec is sound, and approve is the appropriate verdict. ConvergenceAll findings from prior rounds remain resolved, the round 4 commit is a minimal three-line addition that closes the sole outstanding issue (windowSize not forwarded on the viewStyle path), and this review cycle has reached genuine agreement rather than sycophantic collapse. Recommending converged-approve. |
1 participant
Summary
Widens
paginationNav()'sshowFirst/showLast/showPrevious/showNextargs frombooleantoany, accepting the tri-state strings"auto"/"always"/"never". Backwards compatible — booleans coerce (true→"always",false→"never"). Defaults flip fromtrueto"auto". Under"auto", the first/last anchors only render when the visible page-number window does not already reach the boundary, restoring the legacy 3.xpaginationLinks(alwaysShowAnchors=false)semantics that a like-for-like swap topaginationNav()previously lost. A newwindowSizearg onpaginationNav()keeps the auto-mode predicates coherent withpageNumberLinks()'s window. Invalid mode strings throwWheels.InvalidArgument.Related Issue
Fixes #2716
Recommended path from research: #2716 (comment)
Type of Change
Feature Completeness Checklist
Signed-off-by:trailervendor/wheels/tests/specs/view/paginationHelpersSpec.cfc(newpaginationNav anchor display modesdescribe block plus two updated paginationNav specs for the new default)bot-update-docs.ymlbot-update-docs.ymlbot-update-docs.yml[Unreleased]→ Fixedcurl ".../wheels/core/tests?db=sqlite&format=json&directory=wheels.tests.specs.view"returns523 pass, 0 fail, 0 error; security suite (directory=wheels.tests.specs.security) returns174 pass, 0 fail, 0 errorTest Plan
describe("paginationNav anchor display modes", ...)block exercises:"auto"mode hides First on the page where the window already includes page 1"auto"mode hides Last on the page where the window already includes the last page"auto"mode renders First / Last when the window does NOT reach the boundary"always"and"never"overridestrue→"always",false→"never") for backwards compatibilitywindowSizearg propagates through the auto-mode predicatesWheels.InvalidArgumentpaginationNavspecs updated to reflect the new"auto"default