Uh oh!
There was an error while loading. Please reload this page.
refactor(frontend): group components/ by responsibility (#293) - #494
Conversation
The top level of components/ held 58 files with no organising principle. Groups them per the issue's own proposal, as pure moves + import rewrites — zero behaviour change. marketing/ HowItWorks, SignInModal, HeroCard (+test) graph/ KnowledgeGraph, 2D, 3D (+3 tests) chat/ ChatPanel, MarkdownChat, MermaidBlock, FunctionPlot, AIDisclaimerChip, ModelToggle, SharedContextToggle, SessionSummary Top level: 58 -> 38 files. `ui/`, `flashcards/` and `screens/` are unchanged. The proposal also listed study/ and social/ groups; neither had any top-level members (those screens already live in screens/), so creating them would have made empty directories. Deliberately NOT done: renaming Gradebook/ to gradebook/. The proposal wanted "the two Gradebook levels merged", but they are already split cleanly by responsibility — components/Gradebook/ holds the parts, components/screens/ Gradebook/ holds the routes. The only remaining delta is letter case, and a case-only directory rename is a genuine cross-platform hazard for the macOS contributors on this repo. Not worth it for a cosmetic gain. Moves were done with `git mv`, so history follows the files. Four things had to move WITH the files, each of which would have failed silently or confusingly otherwise: - frontend/eslint.config.mjs — the testid-enforcement `files` array is keyed by path; stale entries would have silently stopped enforcing the rule on the exact files it was written to cover. - frontend/eslint-suppressions.json — also path-keyed. Stale keys surfaced as 22 lint errors, since the suppressions no longer matched any file. - docs/frontend-testids.md — the surface table names owning files. This is the other half of the testid convention. - HeroCard.test.tsx resolved src/ as `__dirname/..`, which silently became components/ once the file moved a level deeper. Now `../..`. Also refreshed the live path references in SECURITY.md and the graph journey's header comment. Historical records (docs/frontend-audit/, docs/decisions/, docs/superpowers/plans/) are deliberately left pointing at the old paths — they are dated snapshots of what was true when written, not live indexes. part of #293 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | b72640f | Commit Preview URL Branch Preview URL | Jul 31 2026, 07:30 AM |
…293) Both passed green while silently mocking nothing — the exact failure mode a move-only refactor hides, and the reason tsc/lint/vitest all being green did not prove the move was complete. Dashboard.test.tsx vi.mock("../KnowledgeGraph") -> "../graph/KnowledgeGraph" chat/ChatPanel.test.tsx vi.mock("./Icon") -> "../Icon" vitest does not error on a mock specifier that resolves to nothing; it just declines to intercept, so the real component rendered instead of the stub and the tests stayed green either way. tsc cannot see these because a vi.mock path is a string literal, not an import. Swept every relative vi.mock specifier in src/ against the filesystem: these two were the only unresolvable ones, and there are now zero. part of #293 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
AndresL230
commented
Jul 31, 2026
Review found two silent bugs the green gates could notMulti-agent review over the move. Two vitest doesn't error on a mock specifier that resolves to nothing — it just declines to intercept. So the real components rendered instead of the stubs and the tests stayed green either way. Fixed in Review also verified, all clean: every entry in E2EWorth recording: the first run of this cycle failed with 34/35 — Gates: |
Uh oh!
There was an error while loading. Please reload this page.
Warning Review limit reached
Next review available in:7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (34)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The top level of
components/held 58 files with no organising principle. Grouped per the issue's own proposal — pure moves + import rewrites, zero behaviour change.marketing/graph/chat/Top level: 58 → 38.
ui/,flashcards/andscreens/unchanged. Moves usedgit mv, so history follows the files.Timing
The proposal flagged that the real risk here is timing, not correctness: "this touches ~50 files and will conflict with every open frontend branch. It should land when the frontend queue is empty." It is — 0 open PRs when this branched. That's why this went first.
Two deviations from the proposal, both deliberate
study/andsocial/were not created. Neither had any top-level members — those screens already live inscreens/— so the groups would have been empty directories.Gradebook/was not renamed togradebook/. The proposal wanted "the two Gradebook levels merged", but they're already split cleanly by responsibility:components/Gradebook/holds the parts,components/screens/Gradebook/holds the routes. The only remaining delta is letter case, and a case-only directory rename is a real cross-platform hazard for the macOS contributors on this repo. Not worth it for a cosmetic gain — happy to do it if you disagree.Four things that had to move with the files
Each of these would have failed silently or confusingly:
frontend/eslint.config.mjs— the testid-enforcementfilesarray is path-keyed. Stale entries would have silently stopped enforcing the rule on the exact files it exists to cover. That's a quiet failure, which is the dangerous kind.frontend/eslint-suppressions.json— also path-keyed. This one failed loudly: 22 lint errors, because the suppressions no longer matched any file.docs/frontend-testids.md— the surface table names owning files; this is the other half of the testid convention.HeroCard.test.tsxresolvedsrc/as__dirname/'..', which silently becamecomponents/once the file moved a level deeper. Now'../..'.Also refreshed live path references in
SECURITY.mdand the graph journey's header comment.Historical records (
docs/frontend-audit/,docs/decisions/,docs/superpowers/plans/) are deliberately left pointing at the old paths — they're dated snapshots of what was true when written, not live indexes. Rewriting them would falsify the record.Gates
tsc --noEmitclean ·npm run lint0 errors ·npx vitest run58 files / 415 testspart of #293