Add page/button wiring guardrails and fix audited defects#1030
Merged
Conversation
A repo-wide wiring audit found the app well-wired, with a few real gaps. This fixes the clear-cut defects and installs a prevention layer so the class of bug (dead buttons, orphan routes) is caught by a gate instead of by a user noticing a control that does nothing. Fixes: - Wire the dead "Language and region" globe button in the workflow header to the codebase's disabled "coming soon" placeholder convention (aria-disabled + sr-only note); it was styled and labelled but had no handler and was not disabled. - Correct three stale doc claims: the /documents/source redirect target in the site-map generator (canonical /documents/[id], not the invalid-id /documents/search fallback the first-redirect regex captured), and, in codebase-index, /applications (a redirect, not a composer mode home) plus the missing dsm/specifiers mode-param redirects. Prevention: - New AGENTS.md `page-and-button-wiring` durable rule block. - New local ESLint rule `require-button-wiring` flagging a `<button type="button">` with no handler / formAction / disabled state (the globe-button class); scoped to src/**, mockups exempt. - New offline tests/route-reachability.test.ts asserting every static production page route has an inbound nav link (catches orphan routes such as /tools), with a documented allowlist. - New docs/wiring-conventions.md reference, registered in docs/README and cross-linked from codebase-index. - Record the judgment-call findings (#6-#10) in the /issues ledger. Verified offline: lint, typecheck, sitemap:check, docs:check-index, knip, and the full unit suite (both new gates included) pass. The unrelated, pre-existing pdf-extraction-budget process-timing test fails on origin/main too (tracked as ledger #2). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BFM6m7WAnLuo9kqh2ExPNh
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds documented button and navigation conventions, production ESLint enforcement for unwired buttons, route-reachability tests, a canonical sitemap redirect, and a disabled “coming soon” language-region control with updated UI coverage. ChangesWiring and reachability
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
BigSimmo
marked this pull request as ready for review
July 21, 2026 01:26
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
BigSimmo
enabled auto-merge (squash)
July 21, 2026 01:27
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
tests/route-reachability.test.ts (1)
29-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRequire a non-empty reason for every allowlisted route.
The stale-entry check validates only the key; an entry such as
["/route", ""]would still silence an orphan. Assertreason.trim().length > 0while iterating the allowlist so exemptions cannot bypass the documented rationale requirement.As per coding guidelines, route-reachability tests must maintain coverage for production page routes unless exemptions are consciously documented.
Also applies to: 131-135
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/route-reachability.test.ts` around lines 29 - 43, Update the REACHABILITY_ALLOWLIST validation in the route-reachability tests to assert that each exemption reason has non-whitespace content by checking reason.trim().length > 0 during iteration, while preserving the existing route-key validation and production-route coverage behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/wiring-conventions.md`:
- Around line 47-49: Update the globe defect fix date in the “Language and
region” guidance to 2026-07-21, matching the maintained date in AGENTS.md while
leaving the surrounding wiring convention unchanged.
In `@eslint-rules/require-button-wiring.mjs`:
- Around line 26-48: Update isWiringAttr to recognize only the onClick handler
and the existing EXPLICIT_WIRING_ATTRS entries, removing the broad on* event
check; also remove formAction from EXPLICIT_WIRING_ATTRS because it does not
provide behavior for type="button". Preserve isTypeButton and the
disabled/aria-disabled placeholder handling.
In `@eslint.config.mjs`:
- Around line 18-21: Update MOCKUP_IGNORES to remove the broad "**/*mockup*"
pattern and retain only the documented /mockups routes, *-mockups/ directories,
and *-mockups.tsx singleton patterns. In the associated button-rule file scope,
include all declared JavaScript and TypeScript source extensions, including .js,
while preserving the existing mockup exclusions.
In `@tests/route-reachability.test.ts`:
- Around line 82-87: Update the Therapy Compass route discovery logic around
tcBindingsSrc to import the exported BASE and RESERVED_SEGMENTS constants when
possible; otherwise require both source regex matches to succeed and fail the
test explicitly if parsing fails. Preserve adding every reserved route target
from the parsed values, without silently falling back to an empty set.
- Around line 89-106: Replace the raw full-file regex check in isReachable and
referenceRegExp with AST/token-aware extraction of actual Link hrefs and
router/redirect navigation calls, or reuse the existing shared route manifest if
available. Ensure comments, strings, ownership checks, and unrelated object
properties do not count as references, while object-form and computed navigation
expressions are recognized; preserve builderTargets handling and selfFile
exclusion.
- Around line 53-64: Update collectSourceFiles to scan JavaScript and JSX files
in addition to TypeScript and TSX files, while preserving the existing directory
recursion, mockup exclusion, relative-path normalization, and file-content
collection behavior.
- Around line 69-75: Update the builderTargets setup in the route reachability
test to invoke appModeHomeHref with representative query inputs, including
document and namespaced search cases, and add each resulting path to the set.
Preserve the existing default-target collection and ensure all valid
query-dependent static routes are covered.
---
Nitpick comments:
In `@tests/route-reachability.test.ts`:
- Around line 29-43: Update the REACHABILITY_ALLOWLIST validation in the
route-reachability tests to assert that each exemption reason has non-whitespace
content by checking reason.trim().length > 0 during iteration, while preserving
the existing route-key validation and production-route coverage behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9c7f63b5-7a76-4c22-8868-189d396bfe99
📒 Files selected for processing (12)
AGENTS.mddocs/README.mddocs/codebase-index.mddocs/outstanding-issues.mddocs/site-map.mddocs/wiring-conventions.mdeslint-rules/require-button-wiring.mjseslint.config.mjsscripts/generate-site-map.tssrc/components/clinical-dashboard/master-search-header.tsxtests/route-reachability.test.tstests/ui-tools.spec.ts
… review
Addresses the CodeRabbit review threads on the new wiring tooling:
- require-button-wiring: only onClick (not any on*) and disabled/aria-disabled
count as wired; drop formAction (no submit effect on type=button). onFocus/
onMouseEnter no longer mask a click that does nothing.
- eslint.config MOCKUP_IGNORES: replace the broad **/*mockup* substring (which
would exempt arbitrary production files) with the precise documented surface
(src/app/mockups/**, *-mockups/, *-mockup(s).tsx); widen the rule's file scope
to src/**/*.{js,jsx,ts,tsx}.
- route-reachability test: scan .js/.jsx source too; also seed builderTargets
with the query-bearing appModeHomeHref variant (document/namespaced-search
routes); fail loudly if therapy-compass BASE/RESERVED_SEGMENTS can't be parsed
instead of silently checking nothing.
- Align the globe-defect fix date to 2026-07-21 across the doc + rule + config.
Verified: npm run lint (0 problems — no button newly flagged), typecheck,
route-reachability (2/2), format:check.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013f523gDfoBXPYT4H9vXRkE
…m' into claude/wiring-audit-issues-emke3m
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@eslint-rules/require-button-wiring.mjs`:
- Around line 26-27: Update the button-wiring checks using EXPLICIT_WIRING_ATTRS
and the onClick handling to validate statically known literal values rather than
attribute presence alone: treat disabled={false}, aria-disabled="false", and
onClick={null} as unwired, while preserving dynamic expressions and
aria-disabled="true" as valid wiring. Apply the same value-aware logic in the
related validation path around the button analysis.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3fa4a994-406b-4935-a7e7-17d1897c2b39
📒 Files selected for processing (4)
docs/wiring-conventions.mdeslint-rules/require-button-wiring.mjseslint.config.mjstests/route-reachability.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/route-reachability.test.ts
- docs/wiring-conventions.md
- eslint.config.mjs
CodeRabbit re-review: disabled={false}, aria-disabled="false", and onClick={null}
satisfied the rule despite doing nothing. Add isStaticallyOff() so a statically-
known off value (literal false/null, string "false", identifier undefined — bare
or in an expression container) no longer counts as wiring/disabled, while dynamic
expressions and aria-disabled="true" stay valid. lint stays green (0 problems; no
existing button relies on those patterns).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013f523gDfoBXPYT4H9vXRkE
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A repo-wide page/button wiring audit (85 pages, 40 API routes, 146 components). The app is actually well-wired — the audit surfaced only a few real gaps. This PR fixes the clear-cut defects and installs a prevention layer so the class of bug (dead buttons, orphan routes) is caught by a gate instead of by a user noticing a control that does nothing.
Fixes
master-search-header.tsx) was styled +aria-labelled but had noonClickand was not disabled. Converted to the codebase's disabled "coming soon" placeholder convention (aria-disabled+sr-onlynote), matchingfavourites-hub.tsx./documents/sourceredirect target in the site-map generator (the canonical/documents/[id], not the invalid-id/documents/searchfallback the first-redirect()regex captured); and incodebase-index.md,/applications(a redirect, not a composer mode home) plus the missingdsm/specifiersmode-param redirects.Prevention (documentation + memory + rules)
AGENTS.mdpage-and-button-wiringdurable block.require-button-wiringflags a<button type="button">with no handler /formAction/ disabled state (the globe-button class). Scoped tosrc/**; mockups exempt.tests/route-reachability.test.tsasserts every static production page route has an inbound nav link (catches orphans like/tools), with a documentedREACHABILITY_ALLOWLIST. Closes the gapsite-map.test.tsleft (it checks routes are documented, not reachable).docs/wiring-conventions.md, registered indocs/README.mdand cross-linked fromcodebase-index.md.#006–#010in the/issuesledger (docs/outstanding-issues.md): the/toolsvs/?mode=toolsduplicate entry point, the unuseddocument-flow-routes.tshref builders, the server-only/api/jobs, and the un-built "coming soon" placeholders.Both new gates are offline and join
verify:cheap+ CI automatically (lint and the unit suite).Verification
npm run verify:pr-localDuring development, use
npm run verify:cheapas the faster iteration gate before the final PR-local preflight.npm run verify:uiwhen UI, routing, styling, browser behavior, reduced-motion, or forced-colors behavior changednpm run verify:releasebefore release or handoff confidence claimsRan (all offline, all green):
npm run verify:cheap— everycheck:*,sitemap:check,docs:check-index,check:knip,check:gate-manifest,lint,typecheck, and the full unit suite (3030 passed, incl. the two new gates). Confirmed the newrequire-button-wiringrule has zerosrc/**violations after the globe fix, and the reachability test flags exactly/toolswhen its allowlist entry is removed (guard has teeth, zero false positives).Verification not run: verify:pr-local/verify:release— not needed for this docs + lint/test-tooling + disabled-placeholder change; the offlineverify:cheapsuperset was run instead.UI verification not run: browser e2e (verify:ui) was not run in this remote environment.The only UI change is the globe button → disabled placeholder; its Playwright assertion intests/ui-tools.spec.tswas updated to the new accessible name andaria-disabledstate, and lint/typecheck pass.One unrelated, pre-existing test fails:
tests/pdf-extraction-budget.test.ts(a Python child-process SIGKILL-timing test) — verified to fail identically on cleanorigin/main; it is environment/timing-related and tracked as ledger#002. Not touched by this PR.Risk and rollout
Clinical Governance Preflight
N/A — this change is UI wiring conventions, documentation, lint/test tooling, and a disabled-placeholder button. It does not touch ingestion, answer generation, search/ranking/retrieval, source rendering, document access, privacy, production env, or clinical output.
Notes
src/lib/rag/**or other RAG-ranking surface was modified, so noRAG impact:line is required.src/lib/rag/clinical-search.tsin ledger row#005(advisorydocs:check-linksonly; present onorigin/main) was intentionally left untouched — it is another row on a protected RAG surface and out of scope here.Generated by Claude Code
Summary by CodeRabbit
/documents/sourcesitemap redirect to point to the canonical document viewer.