fix(view): surface paginationLinks() deprecation at runtime and in upgrade check#2727
Conversation
…grade check paginationLinks() has been documented as deprecated in favor of paginationNav() since #1930 but the deprecation was a roadmap statement only — no runtime signal, no upgrade-check coverage. 3.x → 4.0 upgraders could finish the migration without ever learning the helper is on its way out. Two surfaces: 1. vendor/wheels/view/links.cfc — paginationLinks() now writes a one-time per-request WriteLog(type="warning", ...) pointing at paginationNav() and the individual composables. Guarded by request.wheels.$paginationLinksDeprecationLogged so a paginated view rendering the helper many times still only logs once. Pattern mirrors $checkPluginsDeprecation() in Plugins.cfc. 2. cli/lucli/Module.cfc — adds a 3.x → 4.x grep check for paginationLinks( in app/views, with a remediation pointer to paginationNav() and the composable helpers. Now visible via `wheels upgrade check --to=4.0.0`. Spec at vendor/wheels/tests/specs/view/paginationLinksDeprecationSpec.cfc asserts both surfaces — the runtime guard flag is set on first call and preserved on subsequent calls, and the upgrade-scanner block in Module.cfc contains the paginationLinks grep target. Fixes #2714 Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
…check coverage Signed-off-by: wheels-bot[bot] <wheels-bot[bot]@users.noreply.github.com> Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Wheels Bot — Docs updatedAdded a doc commit to this PR:
No changes were needed in |
![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
This PR closes the two-surface deprecation gap for paginationLinks() (#2714): a runtime one-time WriteLog warning in vendor/wheels/view/links.cfc and a grep check added to the wheels upgrade check 3.x to 4.x block in cli/lucli/Module.cfc. The implementation is correct and cross-engine safe. One user-facing correctness issue should be addressed before merge, and the test architecture has a fixable duplication problem.
Verdict: changes requested (cannot submit as --request-changes on a self-authored PR; treating this as equivalent.)
Correctness
cli/lucli/Module.cfc, line 3749 — fix string references CLAUDE.md
fix: "Replace paginationLinks() with paginationNav() ... See CLAUDE.md ## Pagination View Helpers and issue ##1930."
CLAUDE.md is the wheels framework repository's internal AI instruction file. End users running wheels upgrade check --to=4.0.0 against their own application directory will not have this file, and seeing See CLAUDE.md # Pagination View Helpers in their terminal is a dead reference. Every other fix string in this block is self-contained (e.g. 'Change to extends="wheels.WheelsTest"', "Use service() or inject() from the DI container instead"). Replace the CLAUDE.md pointer with a self-contained description:
fix: "Replace paginationLinks() with paginationNav() (the all-in-one nav helper) or compose firstPageLink/previousPageLink/pageNumberLinks/nextPageLink/lastPageLink directly. See the 3.x -> 4.x upgrade guide or https://github.com/wheels-dev/wheels/issues/1930."Tests
vendor/wheels/tests/specs/view/paginationLinksDeprecationSpec.cfc, lines 61-79 — duplicates UpgradeCheckCoverageSpec block-extraction boilerplate
The upgrade-check static-inspection test reproduces the exact same module-path resolution (expandPath("/wheels/../..")), find("currentMajor <= 3 && targetMajor >= 4", ...) slice, and find("// Run checks", ...) sentinel that already lives in vendor/wheels/tests/specs/cli/UpgradeCheckCoverageSpec.cfc (lines 25-43). The right move is to add one more it() to UpgradeCheckCoverageSpec.cfc:
it("scans for deprecated paginationLinks() helper", () => {
expect(findNoCase("paginationLinks", block) > 0).toBeTrue(
"3.x -> 4.x checks should grep views for paginationLinks( (##2714)."
);
});The runtime-warning half (flag guard tests) belongs where it is in specs/view/. But the CLI coverage assertion should live alongside the other 7 upgrade-check it() blocks in specs/cli/UpgradeCheckCoverageSpec.cfc so there is a single authoritative location. If the // Run checks sentinel ever changes, duplicated extraction logic in two specs will silently drift.
vendor/wheels/tests/specs/view/linksSpec.cfc not updated
linksSpec.cfc's beforeEach/afterEach (lines 9-31) do not clear request.wheels.$paginationLinksDeprecationLogged. After this PR, the first paginationLinks() call in the test suite run fires WriteLog and the flag stays set for remaining linksSpec tests (no cascading calls, no failures). Acceptable. The deprecation spec's own beforeEach resets the flag before each it, so that spec re-fires WriteLog per test — log noise only, not a failure.
Conventions
vendor/wheels/tests/specs/view/paginationLinksDeprecationSpec.cfc, lines 1-18 — multi-line block comment
CLAUDE.md: "Never write multi-paragraph docstrings or multi-line comment blocks — one short line max." The 18-line block comment at the top of the new spec violates this. The context is fully captured by the test names and inline issue references.
Cross-engine
No issues. WriteLog(type="warning", ...), StructKeyExists(request.wheels, ...), and reFindNoCase with paginationLinks\s*\( are all cross-engine compatible. The backslash in the CFML string literal passes through as-is to the Java regex engine where \s* correctly matches optional whitespace, consistent with the adjacent Cors and RateLimiter patterns (Module.cfc lines 3654, 3668).
Commits
Single commit fix(view): surface paginationLinks() deprecation at runtime and in upgrade check — valid type, valid scope, subject under 100 chars, not ALL-CAPS. Clean.
Docs
CHANGELOG entry under [Unreleased] -> ### Fixed is present. Framework docs and AI reference docs deferred to bot-update-docs.yml per the checklist — acceptable.
Required before merge:
- Fix the
fixstring incli/lucli/Module.cfcto remove theCLAUDE.mdreference. - Move the upgrade-check
it()intoUpgradeCheckCoverageSpec.cfcand remove the duplicate block-extraction code frompaginationLinksDeprecationSpec.cfc.
Wheels Bot — Reviewer B (round 1)A's review is substantively correct on all three findings and the cross-engine analysis holds up. One significant omission: A caught the CLAUDE.md multi-line-comment violation in the spec file header but missed identical violations in the two production files touched by the same PR. Those need to be in the required list before we can converge. SycophancyNone detected. A requested changes and backed every finding with a file reference and a concrete fix. False positivesNone detected.
Missed issuesMulti-line comment blocks in production files — same violation A flagged in the spec, but missed in CLAUDE.md states: "Never write multi-paragraph docstrings or multi-line comment blocks — one short line max."
A should add these three removals to the required-before-merge list. Verdict alignmentA's verdict (request-changes) is consistent with the findings. The dead CLAUDE.md reference is a user-facing correctness bug; requesting changes is appropriate. ConvergenceNot aligned yet. A's two required items are correct, but the missed multi-line comment violations in |
There was a problem hiding this comment.
Wheels Bot — Reviewer A
This follow-up covers head SHA 32dcca470 (commit docs(web/guides): note paginationLinks() runtime warning and upgrade-check coverage). The new commit adds accurate, well-scoped MDX updates to both v4-0-0 and v4-0-1-snapshot upgrade guides, closing the unchecked Framework Docs line in the PR checklist. The two required changes from the previous Reviewer A pass are still unaddressed.
Verdict: changes requested (cannot submit as --request-changes on a self-authored PR — treating this as equivalent.)
Correctness
cli/lucli/Module.cfc, line 3749 — fix string references CLAUDE.md (unresolved from previous review).
The line currently reads:
fix: "Replace paginationLinks() with paginationNav() ... See CLAUDE.md Pagination View Helpers and issue ##1930."
CLAUDE.md is the framework repo's internal AI instruction file. It does not ship with the framework and is not present in a user's application directory. An upgrader running wheels upgrade check --to=4.0.0 against their own app will see See CLAUDE.md Pagination View Helpers in their terminal — a dead reference. Every other fix string in this block is self-contained. Replace with a self-contained description and a URL pointing to the upgrade guide or #1930.
Tests
vendor/wheels/tests/specs/view/paginationLinksDeprecationSpec.cfc, lines 61-74 — block-extraction boilerplate duplicated from UpgradeCheckCoverageSpec.cfc (unresolved from previous review).
The upgrade-check coverage it() block re-implements the same expandPath("/wheels/../.."), find("currentMajor <= 3 && targetMajor >= 4", ...) slice, and find("// Run checks", ...) sentinel that already exists in vendor/wheels/tests/specs/cli/UpgradeCheckCoverageSpec.cfc lines 26-44. Two independent specs sharing the same extraction logic will silently drift if the // Run checks sentinel ever changes. The fix is to add one it() to UpgradeCheckCoverageSpec.cfc (asserting findNoCase("paginationLinks", block) > 0) and remove the duplicate extraction code from paginationLinksDeprecationSpec.cfc. The runtime-warning flag tests (the two it() blocks under describe("runtime warning")) belong where they are — only the static-inspection test should move.
Conventions
CLAUDE.md: "Never write multi-paragraph docstrings or multi-line comment blocks — one short line max." Three new comment blocks exceed this limit: the 18-line header in paginationLinksDeprecationSpec.cfc (lines 1-18), the 7-line block in Module.cfc (lines 3736-3742), and the 6-line block in links.cfc. Nits — not blocking.
Docs
The new docs(web/guides) commit is correct and complete. Both v4-0-0 and v4-0-1-snapshot upgrade guides now accurately describe the runtime warning, the per-request guard flag, and the wheels upgrade check coverage.
Commits
Both commits conform to commitlint.config.js. Types valid (fix, docs), scopes reasonable (view, web/guides), subjects under 100 chars and not ALL-CAPS. DCO trailers present on both.
Required before merge:
- Remove the
CLAUDE.mdreference from thefixstring incli/lucli/Module.cfcline 3749 — replace with the upgrade guide URL or the GitHub issue URL. - Move the static upgrade-check
it()intoUpgradeCheckCoverageSpec.cfcand remove the duplicated block-extraction code frompaginationLinksDeprecationSpec.cfc.
There was a problem hiding this comment.
Wheels Bot — Reviewer A response (round 1)
My second review on SHA `32dcca470` (posted 34 seconds after B's comment) already flags the multi-line comment blocks in all three files. B's "missed issues" finding was accurate against my first review but is resolved in the current one. The remaining disagreement is about severity: B argues the production-file comment blocks belong in the required list; I maintain they are nits. I will concede one sub-point — the stale NOTE — and defend the rest.
Conceded points
- Stale NOTE in `Module.cfc`: B is right that the 3-line NOTE explaining `##` doubling becomes stale when required fix #1 is applied. If the `fix` string is rewritten to a URL (no more `##1930`), the NOTE has no referent and is dead documentation. Removing it is not an independent required item — it is a consequence of required fix #1. I have updated required fix #1's description to include: "remove the adjacent 3-line NOTE about `##` doubling (it will be stale once the URL replaces the `##1930` reference)."
Defended points
-
B's characterization of what I missed: B's critique states "A caught the CLAUDE.md multi-line-comment violation in the spec file header but missed identical violations in the two production files." This was accurate about my first review (on SHA `87a440d3d`). My second review on the current SHA explicitly names all three: the 18-line spec header, the 7-line block in `Module.cfc` (lines 3736–3742), and the 6-line block in `links.cfc`. The violations were not overlooked in the review that governs the current head.
-
Severity classification — nit vs. required: CLAUDE.md's "one short line max" rule is unconditional, and B is consistent to apply it equally across spec and production files. However, my "required before merge" designation tracks impact, not just rule membership. Required fix #1 is a user-facing correctness bug: an upgrader running `wheels upgrade check` sees `See CLAUDE.md ## Pagination View Helpers` in their terminal — a dead reference that no user can resolve. Required fix #2 is a test-architecture correctness issue: duplicated extraction logic across two specs will silently drift if the `// Run checks` sentinel ever changes. The 5-line block in `links.cfc` and the 4-line preamble in `Module.cfc` are style violations. They do not affect user-visible output, cross-engine behavior, or test correctness. Calling them nits is a principled distinction, not an inconsistency — the same rule at different severity levels produces different priority labels depending on whether violation has observable consequences. Both the spec's 18-line header and the production-file blocks are in the nits bucket; none of them belong in required.
Updated verdict
Still request-changes: required fix #1 (remove the dead `CLAUDE.md` reference from the `fix` string and remove the stale NOTE alongside it) and required fix #2 (move the static upgrade-check `it()` into `UpgradeCheckCoverageSpec.cfc`) remain unaddressed. The multi-line comment nits in `links.cfc` and `Module.cfc` stay in the nits bucket.
Manually triggered baseline refresh via the refresh-visual-baselines.yml workflow on the PR's branch. 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.
…test Reviewer A's required-before-merge items on #2727: 1. `cli/lucli/Module.cfc` — the `fix` string in the new `paginationLinks` upgrade check referenced `CLAUDE.md ## Pagination View Helpers`. End users running `wheels upgrade check --to=4.0.0` against their own app have no `CLAUDE.md` from this repo. Replace with the public GitHub issue URL. Remove the adjacent NOTE about doubling `##` — it was only relevant while the `fix` string contained `##1930`, and the URL form no longer needs the doubling. 2. `vendor/wheels/tests/specs/view/paginationLinksDeprecationSpec.cfc` — the upgrade-check coverage `it()` duplicated the block-extraction boilerplate (`expandPath`, `find("currentMajor <= 3...")`, etc.) that already lives in `UpgradeCheckCoverageSpec.cfc`. Move the assertion into that spec as one more `it()` alongside the other 3.x → 4.x coverage checks, and trim the deprecation spec down to the runtime warning tests only. Reviewer B's adjacent point (multi-line comment blocks per CLAUDE.md's "one short line max" rule): collapse the 5-line block in `vendor/wheels/view/links.cfc`, the 7-line preamble in `Module.cfc`, and the 18-line header in `paginationLinksDeprecationSpec.cfc`. Refs #2714, #2727.
bpamiri
force-pushed
the
fix/bot-2714-paginationlinks-is-deprecated-but-emits-no-runtime
branch
from
7804548 to
701eafc
Compare
…test Reviewer A's required-before-merge items on #2727: 1. `cli/lucli/Module.cfc` — the `fix` string in the new `paginationLinks` upgrade check referenced `CLAUDE.md ## Pagination View Helpers`. End users running `wheels upgrade check --to=4.0.0` against their own app have no `CLAUDE.md` from this repo. Replace with the public GitHub issue URL. Remove the adjacent NOTE about doubling `##` — it was only relevant while the `fix` string contained `##1930`, and the URL form no longer needs the doubling. 2. `vendor/wheels/tests/specs/view/paginationLinksDeprecationSpec.cfc` — the upgrade-check coverage `it()` duplicated the block-extraction boilerplate (`expandPath`, `find("currentMajor <= 3...")`, etc.) that already lives in `UpgradeCheckCoverageSpec.cfc`. Move the assertion into that spec as one more `it()` alongside the other 3.x → 4.x coverage checks, and trim the deprecation spec down to the runtime warning tests only. Reviewer B's adjacent point (multi-line comment blocks per CLAUDE.md's "one short line max" rule): collapse the 5-line block in `vendor/wheels/view/links.cfc`, the 7-line preamble in `Module.cfc`, and the 18-line header in `paginationLinksDeprecationSpec.cfc`. Refs #2714, #2727.
bpamiri
force-pushed
the
fix/bot-2714-paginationlinks-is-deprecated-but-emits-no-runtime
branch
from
701eafc to
51c8085
Compare
There was a problem hiding this comment.
Wheels Bot — Reviewer A
Both required-before-merge items from the two previous Reviewer A passes are now addressed in commit 701eafc98. One new issue introduced by that commit will block the DCO required status check and must be fixed before merge.
Verdict: changes requested (cannot submit as --request-changes on a self-authored PR — treating this as equivalent).
Correctness
All prior required fixes resolved:
-
cli/lucli/Module.cfc— the deadCLAUDE.md ## Pagination View Helpersreference has been replaced withhttps://github.com/wheels-dev/wheels/issues/1930. The adjacent 3-line NOTE about##doubling was correctly removed alongside it. End users runningwheels upgrade check --to=4.0.0now see a self-contained, followable URL. -
vendor/wheels/tests/specs/view/paginationLinksDeprecationSpec.cfc— the duplicate block-extraction boilerplate (expandPath,find("currentMajor <= 3..."),find("// Run checks", ...)) has been removed. The static upgrade-check assertion is now a singleit()inUpgradeCheckCoverageSpec.cfcalongside the other seven 3.x → 4.x coverage checks, which is the right home for it. -
Reviewer B multi-line comment nits resolved: the 5-line block in
links.cfc, the 7-line preamble inModule.cfc, and the 18-line header inpaginationLinksDeprecationSpec.cfchave all been collapsed to single lines.
Commits
Commit 701eafc98 — DCO sign-off missing.
The human-authored commit 701eafc98 (author bpamiri) ends with:
Refs #2714, #2727.
There is no Signed-off-by: trailer. CONTRIBUTING.md § DCO states:
The DCO GitHub App runs as a required status check on every PR. It verifies that every commit in the PR carries a valid Signed-off-by: trailer and blocks merge if any are missing.
Fix: amend the commit to add the trailer:
git commit --amend -s
git push --force-with-lease
The name and email in the trailer must match the contributor's GitHub identity per CONTRIBUTING.md. The three bot-authored commits (87a440d3d, 32dcca470, 6b375abd9) carry sign-offs or are from github-actions[bot], which the DCO app exempts. Only the human commit is affected.
Cross-engine / Tests / Security
No new issues. The regex pattern paginationLinks\s*\( at Module.cfc:3739 is correct: CFML passes the literal characters \s*\( through to reFindNoCase, and Java regex correctly interprets \s* as optional whitespace and \( as a literal parenthesis — the same convention used by the existing grep checks at lines 3617, 3654, and 3668. WriteLog, StructKeyExists, and the BDD spec matchers (toBeFalse, toBeTrue, toBe) are cross-engine compatible.
One action required: add the DCO sign-off to commit 701eafc98 and force-push. The feature implementation is otherwise clean and ready to merge.
There was a problem hiding this comment.
Wheels Bot — Reviewer A
This pass covers head SHA 51c8085b5 (commit fix(cli): address PR #2727 review — remove dead CLAUDE.md ref, dedup test by Peter Amiri). All required-before-merge items from prior Reviewer A passes are resolved. One issue — the same DCO blocker from the previous pass — is still present on the replacement commit and must be fixed before merge.
Verdict: changes requested (cannot submit as --request-changes on a self-authored PR — treating this as equivalent).
Commits
Commit 51c8085b5 — DCO sign-off missing (same issue as 701eafc98).
The previous Reviewer A pass (on SHA 701eafc98) flagged a missing Signed-off-by: trailer on the human-authored commit. That commit was force-pushed and replaced by 51c8085b5, but the replacement has the same omission. The commit body ends with:
Refs #2714, #2727.
No Signed-off-by: trailer follows. The DCO GitHub App is a required status check on every PR and will block merge until every commit in the branch carries a valid trailer. The other three commits are fine: 87a440d3d (bot, has trailer), 32dcca470 (bot, has two trailers), 6b375abd9 (github-actions[bot], DCO-exempt).
Fix:
git commit --amend -s
git push --force-with-leaseThe trailer must match the contributor's GitHub identity: Signed-off-by: Peter Amiri <peter@alurium.com>.
Prior required items — all resolved
-
Dead
CLAUDE.mdreference incli/lucli/Module.cfc(fixstring) — resolved. Thefixstring now readsSee https://github.com/wheels-dev/wheels/issues/1930. The adjacent NOTE about##doubling was removed alongside it. End users runningwheels upgrade check --to=4.0.0see a self-contained, followable URL. -
Duplicate block-extraction boilerplate in
paginationLinksDeprecationSpec.cfc— resolved. TheexpandPath/find("currentMajor <= 3...")/find("// Run checks", ...)extraction code is gone from the deprecation spec. A singleit("scans for deprecated paginationLinks() helper")was added toUpgradeCheckCoverageSpec.cfc(line 91) alongside the other seven 3.x → 4.x coverage checks. -
Multi-line comment blocks per CLAUDE.md
one short line max— resolved. The 18-line header inpaginationLinksDeprecationSpec.cfc, the 7-line preamble inModule.cfc, and the 5-line block inlinks.cfchave all been collapsed to single-line comments.
Correctness (no new issues)
The runtime guard in vendor/wheels/view/links.cfc (line ~236):
if (!StructKeyExists(request.wheels, "$paginationLinksDeprecationLogged")) {
request.wheels.$paginationLinksDeprecationLogged = true;
WriteLog(type = "warning", text = "[Wheels] paginationLinks() is deprecated...");
}is correct and mirrors the $checkPluginsDeprecation() pattern. The upgrade check entry in Module.cfc (lines 3733–3742) uses the same struct shape as every adjacent check; the pattern: "paginationLinks\s*\(" literal passes \s*\( through to the Java regex engine where it correctly matches optional whitespace followed by a literal parenthesis. The UpgradeCheckCoverageSpec.cfc addition at line 91 accesses block from the enclosing describe closure scope, consistent with all seven preceding it blocks in that spec.
One action required: add the Signed-off-by: trailer to commit 51c8085b5 and force-push. The implementation is otherwise complete and ready to merge.
Wheels Bot — Reviewer B (round 1)A correctly identified the only remaining blocker (missing DCO sign-off on the human commit), but cited a SHA that does not exist in this PR. The verdict (request-changes) is right; the factual reference is wrong and will confuse the contributor trying to find and amend the commit. Not converged pending SHA correction. SycophancyNone detected. A requested changes and substantiated the finding. False positivesA cited SHA The PR contains exactly four commits:
There is no As a secondary consequence: A's idempotency marker uses the nonexistent SHA rather than the current head ( A's practical fix instruction ( All three previously flagged issues verified as actually resolved in the diff:
Missed issuesNone detected. The diff is clean on cross-engine compatibility ( Verdict alignmentA's request-changes verdict is consistent with the finding. The PR cannot merge with a missing DCO on the human commit; that will block the DCO required status check regardless of which SHA A named. ConvergenceNot aligned. A needs to correct the SHA in the review narrative — the commit requiring DCO remediation is |
Wheels Bot — Reviewer B (round 1)A's review of the updated SHA is accurate. The single required item — missing DCO trailer on commit SycophancyNone detected. A requested changes and backed the finding with a concrete commit hash, the verbatim commit body, and a specific remediation command. False positivesNone detected.
Missed issuesNone detected.
Verdict alignmentA's request-changes verdict is consistent with the finding: a missing DCO trailer is a required status check that blocks merge regardless of implementation correctness. ConvergenceAligned on request-changes. The sole remaining action is for Peter Amiri to amend |
…test Reviewer A's required-before-merge items on #2727: 1. `cli/lucli/Module.cfc` — the `fix` string in the new `paginationLinks` upgrade check referenced `CLAUDE.md ## Pagination View Helpers`. End users running `wheels upgrade check --to=4.0.0` against their own app have no `CLAUDE.md` from this repo. Replace with the public GitHub issue URL. Remove the adjacent NOTE about doubling `##` — it was only relevant while the `fix` string contained `##1930`, and the URL form no longer needs the doubling. 2. `vendor/wheels/tests/specs/view/paginationLinksDeprecationSpec.cfc` — the upgrade-check coverage `it()` duplicated the block-extraction boilerplate (`expandPath`, `find("currentMajor <= 3...")`, etc.) that already lives in `UpgradeCheckCoverageSpec.cfc`. Move the assertion into that spec as one more `it()` alongside the other 3.x → 4.x coverage checks, and trim the deprecation spec down to the runtime warning tests only. Reviewer B's adjacent point (multi-line comment blocks per CLAUDE.md's "one short line max" rule): collapse the 5-line block in `vendor/wheels/view/links.cfc`, the 7-line preamble in `Module.cfc`, and the 18-line header in `paginationLinksDeprecationSpec.cfc`. Refs #2714, #2727. Signed-off-by: Peter Amiri <peter@alurium.com>
bpamiri
force-pushed
the
fix/bot-2714-paginationlinks-is-deprecated-but-emits-no-runtime
branch
from
51c8085 to
c96878c
Compare
There was a problem hiding this comment.
Wheels Bot — Reviewer A response (round 1)
B's latest critique (on my review of SHA `51c8085b5427eabf563e9cd2f30a8bd381b149a7`) confirms that all findings are accurate and independently verified. We are aligned on `request-changes`. B's earlier comment (23:29:19 UTC) also correctly flagged a factual error in my prior pass — I concede that finding below.
Conceded points
- Wrong SHA in my prior review (`PRR_kwDOABMUH88AAAABAGqZhw`). My review submitted at 23:20 UTC referenced commit `701eafc98` — a SHA that does not appear in this PR's commit history. B correctly identified this as a factual error and noted the idempotency marker was broken as a consequence (a future A pass on the current head would find no matching marker and re-run). I corrected the SHA in the follow-up review submitted at 23:26 UTC, which uses the correct head SHA `51c8085b5427eabf563e9cd2f30a8bd381b149a7`. B's critique was accurate; the self-correction happened to precede B's comment by three minutes, but the error was real.
Defended points
- None required. B's latest round on the current SHA raises no false positives, no missed issues, and independently verifies the DCO finding and all three prior required-before-merge items. B's 23:29:22 comment confirms convergence without dispute.
Updated verdict
Still `request-changes`: the sole remaining blocker is the missing `Signed-off-by:` trailer on commit `51c8085b5` (Peter Amiri, "fix(cli): address PR #2727 review"). This is a human-only action — `git commit --amend -s; git push --force-with-lease` from the contributor's local branch. The implementation is otherwise complete and ready to merge once DCO is cleared.
Wheels Bot — Address Review (round 1)No bot commit required. The single consensus finding from Reviewer A and B (convergence at SHA The contributor has already resolved it: the PR head has moved to The two SHAs have an empty tree diff ( A fresh Reviewer A pass will run automatically on |
Wheels Bot — Reviewer B (round 1)A's response correctly concedes the wrong-SHA error from the prior pass and defends no remaining findings. However, A's updated verdict is stale: the PR was updated before A's response was submitted, and A reviewed the wrong state. After independently reviewing the current HEAD SycophancyNone detected. A substantiated every concession and stated its verdict with evidence. False positivesA's "sole remaining blocker" claim is factually wrong for the current PR state. A states: "the sole remaining blocker is the missing But A's response was submitted at 23:31:26 UTC; Peter's commit was committed at 23:30:29 UTC. A had one minute to notice the new push and did not. Missed issuesA did not review the new commit After independently reviewing the full PR diff for the current HEAD:
No new issues found. Verdict alignmentA's stated verdict ( ConvergenceAligned on approve. Every required-before-merge item from A's reviews and B's round-1 findings has been addressed in |
…d-but-emits-no-runtime
bpamiri
deleted the
fix/bot-2714-paginationlinks-is-deprecated-but-emits-no-runtime
branch
There was a problem hiding this comment.
Wheels Bot — Reviewer A
This pass covers head SHA 4fcbfe376636f457c3594c8fbc3f2decbfe58fa4 (a merge-from-develop synchronise commit on top of Peter Amiri's fix(cli): address PR #2727 review at c96878cf7). The PR is already merged. All three required-before-merge items from prior Reviewer A passes are now fully resolved. No new issues are introduced by the current head. Overall verdict: approve.
Correctness — all clear
All prior blocking findings are resolved in c96878cf7:
Fix 1 (dead CLAUDE.md reference in cli/lucli/Module.cfc) — confirmed resolved.
The fix string at line ~3746 now reads:
fix: "Replace paginationLinks() with paginationNav() ... See https://github.com/wheels-dev/wheels/issues/1930."
The adjacent 7-line comment block and the NOTE about ## doubling were correctly removed alongside it. End users running wheels upgrade check --to=4.0.0 now see a self-contained, followable URL.
Fix 2 (duplicate block-extraction boilerplate in paginationLinksDeprecationSpec.cfc) — confirmed resolved.
The expandPath / find("currentMajor <= 3...", ...) / find("// Run checks", ...) extraction is gone from the deprecation spec. UpgradeCheckCoverageSpec.cfc now contains one new it() alongside the other seven 3.x → 4.x coverage checks — single authoritative location for the static upgrade-check assertion. ✓
Fix 3 (multi-line comment blocks per CLAUDE.md) — confirmed resolved.
vendor/wheels/view/links.cfc: 5-line block collapsed to single line ✓cli/lucli/Module.cfc: 7-line preamble collapsed to single line ✓vendor/wheels/tests/specs/view/paginationLinksDeprecationSpec.cfc: 18-line header removed entirely ✓
DCO — confirmed resolved.
87a440d3d(bot):Signed-off-by: claude[bot]✓32dcca470(bot):Signed-off-by: wheels-bot[bot]+Signed-off-by: claude[bot]✓c96878cf7(Peter Amiri):Signed-off-by: Peter Amiri <peter@alurium.com>✓6b375abd9(github-actions[bot]): DCO-exempt ✓
Cross-engine
No issues. WriteLog(type="warning", text="..."), StructKeyExists(request.wheels, ...), and the BDD matchers (toBeFalse, toBeTrue, toBe) are cross-engine compatible across Lucee, Adobe CF, and BoxLang.
The regex pattern paginationLinks\s*\( in Module.cfc is correct: CFML string literals pass the backslash through literally to the Java regex engine, where \s* matches optional whitespace and \( matches a literal parenthesis. Consistent with adjacent grep patterns at Module.cfc lines 3617, 3654, and 3668. ✓
Tests
paginationLinksDeprecationSpec.cfc covers exactly the two runtime-guard cases:
- Guard flag unset before first call, set after —
structKeyExistschecktoBeFalse()thentoBeTrue(). ✓ - Guard flag value preserved on subsequent calls — sets flag to sentinel
"first", calls again, assertstoBe("first"). ✓
beforeEach/afterEach correctly clean up both the flag and the encode setting. ✓
Docs
Both v4-0-0 and v4-0-1-snapshot upgrade guides accurately describe the runtime warning, the per-request guard flag, and the wheels upgrade check coverage. CHANGELOG [Unreleased] entry is present. ✓
Commits
All commits conform to commitlint.config.js. Types and scopes valid (fix(view), docs(web/guides), fix(cli)). Subjects under 100 chars, not ALL-CAPS. ✓
Clean implementation, all review rounds resolved, DCO cleared. Ready to ship.
|
Wheels Bot - Reviewer B (round 1) A's review on the post-merge sync SHA is accurate. The PR merged at 01:47:11Z; A's review came in at 01:52:20Z. A correctly noted the merged state and verified all three prior required-before-merge items. No sycophancy, no false positives, no missed issues. Converging on approve. Sycophancy: None detected. A backed every finding with a diff line reference and confirmed all prior required items with evidence. False positives: None detected. Fix 1 (dead CLAUDE.md ref), Fix 2 (duplicate block-extraction boilerplate), Fix 3 (multi-line comment blocks), regex pattern correctness, and DCO - all verified against the actual diff. A's claims are accurate. Missed issues: None detected. I re-scanned the full diff. Test structure in paginationLinksDeprecationSpec.cfc is correct (beforeEach/afterEach manage state correctly, two it() blocks cover guard-flag-on-first-call and idempotency). The sync commit brings no new code beyond what prior rounds already reviewed. Cross-engine compatibility is clean (StructKeyExists, WriteLog, structDelete are standard builtins across all CI engines). Verdict alignment: A's approve verdict is consistent with the findings: no remaining blockers, DCO cleared, implementation complete. Convergence: Aligned on approve. All required-before-merge items from every prior review round are resolved. The PR is merged. |
1 participant
Summary
paginationLinks()has been documented as deprecated in favor ofpaginationNav()since #1930, but the deprecation was a roadmap statement only — there was no runtime signal, nowheels upgrade checkcoverage, and no shim in the legacy-adapter. 3.x → 4.0 upgraders could finish the migration without ever learning the helper is on its way out. Surfaced by titan Phase 2.4 (Wheels 4.0 upgrade), 2026-05-15.Two surfaces in this PR (the framework-side half of the issue — the wheels-legacy-adapter shim is out of scope and lives in
wheels-dev/wheels-legacy-adapter):Runtime warning.
vendor/wheels/view/links.cfc::paginationLinks()now writes a one-time per-requestWriteLog(type="warning", ...)pointing atpaginationNav()and the individual composables (firstPageLink/previousPageLink/pageNumberLinks/nextPageLink/lastPageLink). Guarded byrequest.wheels.$paginationLinksDeprecationLoggedso a paginated view rendering the helper many times still only logs once per request. Pattern mirrors$checkPluginsDeprecation()invendor/wheels/Plugins.cfc(the established one-time-warning convention in the repo — no newDeprecationLoggerintroduced, as the triage comment recommended).Upgrade-check coverage.
cli/lucli/Module.cfc(the file referenced byUpgradeCheckCoverageSpec—MigrationScanner.cfcfrom the issue body does not exist by that name) now grep-scansapp/views/**/*.{cfm,cfc}forpaginationLinks(in the3.x → 4.xblock, with a remediation pointer that namespaginationNav()and the individual helpers.Fixes #2714
Related Issue
Closes #2714
Type of Change
Feature Completeness Checklist
Signed-off-by:vendor/wheels/tests/specs/view/paginationLinksDeprecationSpec.cfc(new). One spec for the runtime guard flag (asserts the flag is set on first call and preserved on subsequent calls), one spec for the upgrade-scanner coverage (static inspection ofcli/lucli/Module.cfc, mirrors the existingUpgradeCheckCoverageSpecpattern).bot-update-docs.ymlfollows up if the MDX guide needs the deprecation called out.bot-update-docs.ymlfollows up.bot-update-docs.ymlfollows up.[Unreleased]→### Fixed.wheelsCLI installed and Docker access was not granted fortools/test-matrix.sh, so the failing → passing transition is verified against the existing patterns it mirrors (UpgradeCheckCoverageSpec.cfcfor the static check,paginationHelpersSpec.cfcfor the controller-driven helper call) rather than executed in-process. Reviewer A / Reviewer B + thecompat-matrix.ymlCI run on this PR are the authoritative pass-gate.Test Plan
compat-matrix.ymlis green for Lucee 6 / Lucee 7 / Adobe 2023 / Adobe 2025 / BoxLang across all hard-gated databases.paginationLinks()call in a 3.x-style view and not on subsequent calls within the same request.wheels upgrade check --to=4.0.0against a sample app that still callspaginationLinks()lists the deprecated-helper finding with theapp/views/...:Nline reference.Screenshots / Output
n/a