chore: add frontend architecture governance baseline - #667
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR establishes a frontend architecture governance system by introducing a canonical manifest that specifies inventory rules, schema, metrics, and owner lanes, paired with a CLI tool that scans TypeScript/TSX files, classifies them by production vs. visibility-only status, assigns owner lanes, and generates JSON or markdown inventory reports. ChangesFrontend Architecture Governance Inventory
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
github-actions
Bot
added
the
documentation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Astro-Han
added
task
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request establishes a frontend architecture governance framework by introducing a manifest document and an inventory script to track file sizes and ownership across the project. The feedback provided focuses on improving the accuracy of the inventory script's file classification logic. Specifically, suggestions were made to refine the comment-stripping regex to handle trailing comments, broaden the detection of import statements in facade files, and strengthen the logic-detection heuristics to ensure that files containing functional code are not misclassified as pure configurations.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
script/frontend-inventory.mjs (2)
335-335: ⚡ Quick winConsider adding error handling for git and file operations.
The script uses
execFileSyncandreadFileSyncwithout try/catch blocks. If git is unavailable, the repo isn't checked out, or a file is deleted between listing and reading, the script will crash with an uncaught exception.For a warn-only governance tool this is acceptable, but adding graceful error handling would improve the developer experience.
♻️ Proposed error handling
Wrap the git command:
function listFrontendFiles() { + try { const stdout = execFileSync("git", ["ls-files", ...FRONTEND_PATHS], { encoding: "utf8" }).trim() return stdout ? stdout.split("\n").filter(Boolean).sort() : [] + } catch (err) { + console.error("Error: git ls-files failed. Ensure you are in a git repository.") + console.error(err.message) + process.exit(1) + } }And wrap file reads:
const records = files.map((path) => { + try { const content = readFileSync(path, "utf8") + } catch (err) { + console.error(`Error reading ${path}: ${err.message}`) + return null + } // ... rest of mapping -}) +}).filter(Boolean)Also applies to: 342-342
🤖 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 `@script/frontend-inventory.mjs` at line 335, Wrap the execFileSync call that runs git ls-files (the line calling execFileSync with FRONTEND_PATHS) in a try/catch so failure to run git returns an empty string or list and logs a console.warn (or processLogger.warn) instead of throwing; likewise wrap subsequent readFileSync calls that read each listed file in try/catch so a missing/deleted file logs a warning and is skipped. Ensure you only call .trim() if stdout is a string, and propagate an empty array/result when git or file reads fail so the script continues gracefully.
157-160: ⚡ Quick winConsider validating numeric arguments.
If a non-numeric value is passed to
--max-rows,Number()will returnNaN, which could cause unexpected behavior inslice(0, maxRows)on line 431.🛡️ Proposed validation
if (arg === "--max-rows") { - out.maxRows = Number(argv[index + 1] ?? out.maxRows) + const val = Number(argv[index + 1] ?? out.maxRows) + out.maxRows = Number.isNaN(val) ? out.maxRows : val index += 1 }🤖 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 `@script/frontend-inventory.mjs` around lines 157 - 160, The code sets out.maxRows = Number(argv[index + 1] ?? out.maxRows) without validating, which can produce NaN and break later slice(0, maxRows); change the parsing to explicitly parse and validate the next argv token (e.g., use parseInt/Number) and check Number.isFinite or !Number.isNaN and positive integer before assigning to out.maxRows (otherwise keep the previous value or throw a clear error); update the argument-handling block that references arg, argv, index and out.maxRows so invalid inputs are rejected or defaulted safely prior to the later slice usage.
🤖 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.
Nitpick comments:
In `@script/frontend-inventory.mjs`:
- Line 335: Wrap the execFileSync call that runs git ls-files (the line calling
execFileSync with FRONTEND_PATHS) in a try/catch so failure to run git returns
an empty string or list and logs a console.warn (or processLogger.warn) instead
of throwing; likewise wrap subsequent readFileSync calls that read each listed
file in try/catch so a missing/deleted file logs a warning and is skipped.
Ensure you only call .trim() if stdout is a string, and propagate an empty
array/result when git or file reads fail so the script continues gracefully.
- Around line 157-160: The code sets out.maxRows = Number(argv[index + 1] ??
out.maxRows) without validating, which can produce NaN and break later slice(0,
maxRows); change the parsing to explicitly parse and validate the next argv
token (e.g., use parseInt/Number) and check Number.isFinite or !Number.isNaN and
positive integer before assigning to out.maxRows (otherwise keep the previous
value or throw a clear error); update the argument-handling block that
references arg, argv, index and out.maxRows so invalid inputs are rejected or
defaulted safely prior to the later slice usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 98d1e51f-5f68-4554-be57-aae374b8a902
📒 Files selected for processing (3)
.github/frontend-architecture-manifest.mdpackage.jsonscript/frontend-inventory.mjs
d45ab7b to
e3be0a7
Compare
github-actions
Bot
removed
app
|
Handled the 2 CodeRabbit nitpicks in 5da4d3d: |
|
Fixed the baseline mismatch in 57032bf. The manifest now marks |
Goal:
Add a shared UI tool-name contract so app and ui code stop duplicating high-risk tool id literals while preserving current rendering behavior.
Scope:
- Add @opencode-ai/ui/tool-contract exports for current shared tool names and the legacy agent name.
- Replace duplicated tool-name literals in UI message/tool rendering and app status extraction paths.
- Add app/ui contract tests for public import compatibility and opencode tool id drift.
- Keep behavior unchanged for legacy "task" rendering and current agent/web/todo/question tool names.
- Update the CI workflow structure test to account for the existing unit-ui-focused blocking aggregate job introduced on dev.
Verification:
- bun test src/components/tool-contract.test.ts
- bun test src/ui-tool-contract.test.ts
- bun test test/github/ci-workflow.test.ts
- bun run typecheck
- git diff --check
- Temporary Tool.define drift check: changing Tool.define("todowrite") while keeping permission "todowrite" makes the contract test fail, then passes after revert.
- GitHub checks green, including ci, unit-opencode, unit-ui-focused, desktop smoke, e2e artifacts, perf-probe-baseline, CodeQL, and CodeRabbit.
- reviewThreads unresolved = 0
Review follow-ups:
- Addressed CodeRabbit nitpick by making tool id matching quote-agnostic.
- Tightened the same test beyond raw string matching so permission or copy strings cannot mask Tool.define/id drift.
- Fixed the CI workflow test drift exposed after rebasing on #667.
Residual risk:
- The contract test is still source-based because opencode does not expose stable tool ids as an importable test surface yet.
Goal: Extract the timeline staging owner out of MessageTimeline as the first #601 message-flow stack root. Scope: - Move createTimelineStaging into packages/app/src/pages/session/session-timeline-staging.ts. - Keep MessageTimeline wired with the same sessionKey, turnStart, renderedUserMessages, and { init: 10, batch: 3 } config. - Add browser-condition staging tests for non-windowed render, staged batches, active-session message growth, completed-session backfill, and session switch rAF cancellation. - Preserve active staging when the same session receives more messages mid-stage so the historical window does not pop to full render. - Add the concrete #670 boundary to the frontend architecture manifest. Verification: - bun --cwd packages/app test --preload ./happydom.ts src/pages/session/session-timeline-staging.test.ts src/pages/session/use-session-history-window.test.ts src/pages/session/session-timeline-scroll-controller.test.ts src/pages/session/session-timeline-scroll-anchors.test.ts -> 1110 pass / 2674 expects - bun run typecheck -> 8 successful tasks - git diff --check - GitHub checks green, including ci, unit-app, unit-opencode, unit-desktop, unit-ui-focused, desktop smoke, e2e artifacts, perf-probe-baseline, CodeQL, and CodeRabbit. - reviewThreads unresolved = 0 Review follow-ups: - Fixed Gemini staging-pop thread and resolved it after replying in-thread. - Added the missing manifest entry that the PR body claimed. - Refreshed PR body verification after #667 and #669 landed on dev. Residual risk: - Electron manual verification was not run because this is scoped to behavior-preserving extraction plus tests, with no visible UI or copy change.
1 participant
Topology Update (2026-05-16)
Final base:
dev.Dependency status: foundation governance PR; no code PR needs to be stacked on this branch after the topology rewrite.
Review order: review first as the governance/report baseline, then review flat PRs and the single message-timeline stack.
Verification update: after review fixes, the reproducible inventory baseline is generated at
5da4d3d61(fix: harden frontend inventory classification). Counts changed from the earlier draft because the script now includes root-levelsrc/*.ts(x)files and keeps logic-bearingindex.ts/ pure-config candidates in the production ratchet set.Summary
Add the frontend architecture governance baseline for
packages/app/srcandpackages/ui/src:.github/frontend-architecture-manifest.mdscript/frontend-inventory.mjsfrontend:inventoryandfrontend:inventory:jsonThis PR does not refactor UI code, move imports, or change public runtime behavior.
Why
The UI rewrite work needs a shared, reviewable source of truth before more architecture slices start. The manifest captures the file-size baseline, owner lanes, exception schema, ratchet stages, and first PR stack boundaries so the next agent does not need chat context to continue.
Related Issue
Related to #599. Also references #601, #604, #605, #606, #595, #615, and #638 as owner lanes.
Architecture Boundary
Owner lane: #599 mainline / governance.
Base:
dev.Depends on: none.
Touched files:
.github/frontend-architecture-manifest.mdscript/frontend-inventory.mjspackage.jsonArchitecture effect:
Behavior unchanged: yes. This is governance/reporting only.
Public write status: PR body only. No issue body/comment was created or updated.
Human Review Status
Pending. A human should make the final merge decision after reviewing the final diff and verification evidence.
Review Focus
.github/frontend-architecture-manifest.mdacceptable as the canonical tracked home, givendocs/is local-only in this checkout?Risk Notes
Low runtime risk. The script is warn-only and is not wired into CI as a hard gate. The main risk is classification drift; the manifest records schema version
1and says future metric changes must use a new schema version instead of mixing baselines.How To Verify
Screenshots or Recordings
Not applicable. No visible UI changes.
Checklist
dev, and my PR title and commit messages use Conventional Commits in EnglishSummary by CodeRabbit
Release Notes
New Features
frontend:inventoryandfrontend:inventory:jsonnpm scripts to generate reports on frontend file metrics, classifications, and ownership assignments.Documentation