Skip to content

fix(router): classify /_browser fixture routes as Internal in Routes UI - #2588

Merged
bpamiri merged 2 commits into
developfrom
fix/bot-2577-browser-routes-appear-in-application-routes-ui-eve
May 12, 2026
Merged

bpamiri merged 2 commits into
developfrom
fix/bot-2577-browser-routes-appear-in-application-routes-ui-eve

Conversation

@wheels-bot

@wheels-bot wheels-bot Bot commented May 11, 2026

Copy link
Copy Markdown
Contributor

Summary

Classifies framework-internal /_browser/* browser-test fixture routes as
Internal in the dev Routes UI instead of leaking them into the Application
tab. The bucket predicate in vendor/wheels/public/views/routes.cfm
previously matched only controller == "wheels.public" or
pattern == "/wheels/app/tests", so the fixture routes (which use
controllers like BrowserTestHome / BrowserTestSessions and are
auto-mounted when loadBrowserTestFixtures is enabled in dev/test) fell
through to the App tab even though they ship with the framework and have
no corresponding files under /app. One-line extension to the predicate
adds a Left(r.pattern, 9) EQ "/_browser" check.

Related Issue

Closes #2577

Type of Change

  • Bug fix
  • New feature
  • Enhancement to existing feature
  • Documentation update
  • Refactoring

Feature Completeness Checklist

  • Tests -- vendor/wheels/tests/specs/view/routesViewBrowserFixturesSpec.cfc pins the predicate so the classification can't drift back. Failing → passing.
  • Framework Docs -- Not required for this fix (bot-update-docs.yml will add MDX updates as a follow-up commit if needed).
  • AI Reference Docs -- Not required for this fix (handled by bot-update-docs.yml).
  • CLAUDE.md -- Not required for this fix (handled by bot-update-docs.yml).
  • CHANGELOG.md -- Entry added under [Unreleased] → Fixed.
  • Test runner passes -- bash tools/test-local.sh view → pass:506 fail:0 err:0 after the fix. Before the fix the new spec produced pass:505 fail:1.

Test Plan

  1. With the framework running in dev mode and loadBrowserTestFixtures=true, open /wheels/routes.
  2. Confirm the /_browser/* routes (home, login, dashboard, login-as, logout, authenticate) now appear under the "Internal" tab, not "App".
  3. Confirm the existing wheels.public and /wheels/app/tests internal classifications are unchanged.

The Routes UI predicate previously matched only `controller == "wheels.public"`
or `pattern == "/wheels/app/tests"`, so the `/_browser/*` browser-test fixture
routes (registered via `loadBrowserTestFixtures` in dev/test environments) fell
through to the Application tab even though they ship with the framework and
have no corresponding files under /app. Extend the predicate to also classify
routes whose pattern begins with `/_browser` as internal.
… tab in Routes UI

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@wheels-bot

wheels-bot Bot commented May 11, 2026

Copy link
Copy Markdown
Contributor Author

Wheels Bot — Docs updated

Added a doc commit to this PR:

  • web/sites/guides/src/content/docs/v4-0-0-snapshot/testing/browser-tests.mdx — noted that all /_browser/* fixture routes appear under the Internal tab in the Routes UI, not Application
  • CLAUDE.md — extended the "Fixture routes" gotcha bullet with the same note

@wheels-bot wheels-bot Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wheels Bot — Reviewer A

TL;DR: This is a minimal, correct one-line extension to the route-classification predicate in routes.cfm that moves /_browser/* fixture routes from the App tab to the Internal tab in the dev Routes UI. The fix is well-scoped, cross-engine safe, and the regression spec follows the established structural-assertion pattern for view files. No blocking findings — two minor nits only.

Correctness

The fix is sound:

// vendor/wheels/public/views/routes.cfm, line 15
|| (StructKeyExists(r, "pattern") && Left(r.pattern, 9) EQ "/_browser")
  • "/_browser" is exactly 9 characters, so the Left() call is correct.
  • The StructKeyExists(r, "pattern") guard (already used for the /wheels/app/tests arm) is correctly applied here too, preventing null-key access on routes that only carry a controller key.
  • Short-circuit && evaluation means Left() is never called when pattern is absent.

Cross-engine

No issues. The one cross-engine concern in the area (Left(str, 0) crashing Lucee 7) does not apply here — the second argument is the fixed literal 9, never zero.

Tests

vendor/wheels/tests/specs/view/routesViewBrowserFixturesSpec.cfc

Extends wheels.WheelsTest, uses BDD describe/it closures, and ##2577 is correctly double-hash-escaped in the string literals. Three cases cover the new arm plus the two pre-existing arms as non-regression guards.

Minor nit — comment/code mismatch (line 28–30):

// Look in the categorization region (the first ~600 chars
// after the bucket declaration).
var windowLen = Min(800, Len(src) - anchor);

The comment says "~600 chars" but the code uses 800. Harmless in practice (the /_browser predicate is at line 15, only ~200 chars past the first "internalRoutes" on line 7), but the comment should match the code. Suggest updating to "~800 chars" or noting that the window must extend past the entire if block.

Minor nit — anchor resolves to struct initializer, not predicate (line 25):

var anchor = findNoCase("internalRoutes", src);

This resolves to the struct-literal initializer at line 7 ("internalRoutes" = []), not to the classification predicate itself. The 800-char window from there does include the predicate, and "/_browser" does not appear anywhere in the HTML output section of the file, so the assertion is valid today. A more targeted anchor such as findNoCase("ArrayAppend(routes.internalRoutes", src) would be more precise and survive future growth of the cfscript preamble, but given that this mirrors the established structural-assertion pattern used by packageListViewTestsLinkSpec.cfc and MigratorViewIconsSpec.cfc, the current form is acceptable.

Commits

fix(router): classify /_browser fixture routes as Internal in Routes UI — valid type (fix), optional scope (router), subject under 100 chars, not ALL-CAPS.

Docs

CHANGELOG [Unreleased] → Fixed entry is present and accurate.

@wheels-bot wheels-bot Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wheels Bot — Reviewer A

TL;DR: The new docs(web/guides) commit closes the documentation gap flagged in the prior round — CLAUDE.md and browser-tests.mdx both now note that /_browser/* routes appear under the Internal tab in the Routes UI. No new correctness, cross-engine, or security issues are introduced. One pre-existing nit from the previous review remains unaddressed (minor). Overall verdict: comment (no blocking findings).

Correctness

The docs-only commit is accurate:

  • CLAUDE.md line updated: They must come before .wildcard() in routes.cfm. In the Routes UI (/wheels/routes) all /_browser/* routes appear under the Internal tab, not Application. — matches the predicate change in routes.cfm.
  • browser-tests.mdx append: same fact, correctly placed immediately after the paragraph describing loginAs and the /_browser/login-as fixture route.

Both additions are factually correct. The first commit's core fix (Left(r.pattern, 9) EQ "/_browser" with StructKeyExists guard, "/_browser" = 9 chars) was validated in the previous review and is unchanged.

Tests

The routesViewBrowserFixturesSpec.cfc from the first commit is unchanged and remains correct. One pre-existing nit from the prior review is still present:

File: vendor/wheels/tests/specs/view/routesViewBrowserFixturesSpec.cfc, lines 28-29

// Look in the categorization region (the first ~600 chars
// after the bucket declaration).
var windowLen = Min(800, Len(src) - anchor);

The comment says "~600 chars" but the actual window is 800. The assertion is correct in practice (the /_browser literal is ~200 chars from the anchor), but the comment is misleading. Suggest updating to "~800 chars" or removing the size claim entirely. This does not block merge.

Commits

Both commits conform to commitlint.config.js:

Commit Type Scope Subject Valid?
0ee2d25 fix router classify /_browser fixture routes as Internal in Routes UI Yes
cafcd82 docs web/guides note /_browser fixture routes appear under Internal tab in Routes UI Yes

Docs

  • CHANGELOG.md — entry present under [Unreleased] > Fixed.
  • CLAUDE.md — updated.
  • web/sites/guides/src/content/docs/v4-0-0-snapshot/testing/browser-tests.mdx — updated.
  • All three changes are accurate and consistent with each other.

Cross-engine / Security

No changes to these dimensions. The Left(str, 9) call with a fixed literal 9 does not trigger the Lucee 7 Left(str, 0) crash. No security surface affected.

@wheels-bot

wheels-bot Bot commented May 11, 2026

Copy link
Copy Markdown
Contributor Author

Wheels Bot — Reviewer B (round 1)

A's review is clean — accurate claims, no false praise, two real (minor) nits. One process observation about SHA coverage, but nothing that changes the recommendation. Converging on approve.

Sycophancy

None detected. A used COMMENTED (not APPROVED) despite calling out only minor nits, and the summary is honest: "No blocking findings — two minor nits only."

False positives

None detected. Each of A's claims checked out against the diff:

  • Left(r.pattern, 9) — "/_browser" is exactly 9 characters. Correct.
  • StructKeyExists(r, "pattern") guard — present in the added line. Correct.
  • Comment/code mismatch ("~600 chars" vs windowLen = Min(800, ...)) — verified in the spec diff at the window-calculation block. Real nit.
  • Anchor resolves to the struct-initializer occurrence of internalRoutes, not the predicate — findNoCase("internalRoutes", src) matches the earliest occurrence (the initializer routes.internalRoutes = []). Real nit; A's judgment that the 800-char window still covers the predicate is correct for the current file layout.
  • Cross-engine Left(str, 0) concern — A correctly ruled it out; the literal is 9, never zero.

Missed issues

One minor process observation: A's review marker carries SHA 0ee2d25878742209d1176abd954d89da4df67404 (the fix commit), while the current head is cafcd82be5e8f75f413c565eb5cd455fa36adbcb (a subsequent docs commit by the bot). A's review therefore did not formally cover the two docs-layer changes in that follow-up commit (CLAUDE.md and browser-tests.mdx). Both are one-sentence additions that correctly state "all /_browser/* routes appear under the Internal tab" — trivially correct and low-risk — so the gap does not change the recommendation, but a human reviewer should be aware A's coverage predates those lines.

No other missed issues. The ## over-escaping in //-style comments inside the spec is unnecessary (comments are not evaluated) but harmless and not worth flagging.

Verdict alignment

A's COMMENTED state with "no blocking findings" is consistent with what the diff shows: a minimal, correct one-line predicate extension, a structural regression spec that mirrors the established pattern, and two genuine but non-blocking nits. Verdict is appropriate.

Convergence

Aligned. The docs-commit gap is a process observation, not a correctness gap. A's implicit approve recommendation is correct for this change. The two nits A raised (comment/code mismatch on the window size description, imprecise anchor for the structural assertion) are real but do not require changes before merge. A human can decide whether to ask the author to fix the comment or let it ride.

@wheels-bot

wheels-bot Bot commented May 11, 2026

Copy link
Copy Markdown
Contributor Author

Wheels Bot — Reviewer B (round 1)

A's round-2 review is accurate and appropriately scoped. No sycophancy, no false positives. One minor editorial observation: A silently dropped the anchor-precision nit it raised in round 1 without noting the omission, but A did label that nit "acceptable" in round 1, so dropping it in round 2 is defensible. The one carried-forward nit (comment says "~600 chars", code uses 800) is genuine and verified. A's "comment" verdict with explicit "does not block merge" is effectively an approval with a minor note — converging approve.

Sycophancy

None detected. A chose COMMENTED rather than APPROVE, which is the correct posture when an open (minor) nit remains. No evidence-free "LGTM" language. The security, migration, deploy, and DI subsystems are not touched by this PR.

False positives

None detected. I verified each of A's claims against the diff:

  • "/_browser" is 9 characters: correct — /, _, b, r, o, w, s, e, r.
  • StructKeyExists guard correctly applied: correct — the new arm mirrors the existing /wheels/app/tests arm exactly.
  • Comment says "~600 chars", code uses 800: verified in routesViewBrowserFixturesSpec.cfc lines 28–30. Accurate.
  • CLAUDE.md and browser-tests.mdx additions are factually correct: verified against the predicate change and the routes.cfm diff. Both say the same thing; both are accurate.
  • Both commits conform to commitlint.config.js: verified. Types fix and docs, scopes router and web/guides, subjects under 100 chars, not ALL-CAPS.
  • Left(str, 9) does not trigger the Lucee 7 Left(str, 0) crash: correct per CLAUDE.md.

Missed issues

One editorial gap: A's round-1 review raised two nits but round 2 carries only one. The dropped nit (anchor resolves to struct initializer at line 7 rather than the classification predicate) was already labeled "acceptable" in round 1, so omitting it is reasonable — but a note like "second nit from round 1 accepted as-is" would have been clearer. Not a substantive miss.

No other missed issues. The /_browser string does not appear in the HTML output section of routes.cfm, so the structural assertion is valid. The ##2577 double-hash escaping in string literals is correct. No .ai/wheels/ update is required for a Routes UI classification fix — the meaningful developer-facing note is now in both CLAUDE.md and browser-tests.mdx.

Verdict alignment

A's COMMENTED (no blocking findings, explicit "does not block merge") is consistent with the single outstanding minor nit. Appropriate.

Convergence

Aligned. The PR is a minimal, correct fix with accurate documentation and a regression spec that follows the established structural-assertion pattern. The one remaining nit (misleading comment in test file) is non-blocking by A's own assessment and confirmed by B. Joint recommendation: approve and merge.

@bpamiri
bpamiri marked this pull request as ready for review May 12, 2026 01:02
@bpamiri
bpamiri merged commit bc80573 into develop May 12, 2026
16 checks passed
@bpamiri
bpamiri deleted the fix/bot-2577-browser-routes-appear-in-application-routes-ui-eve branch May 12, 2026 01:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

/_browser routes appear in application Routes UI even though corresponding controllers are not in /app

1 participant