chore(coverage): extend the D38 gate to co-located StyleSheet modules - #607
chore(coverage): extend the D38 gate to co-located StyleSheet modules#607thomasluizon wants to merge 3 commits into
Conversation
tools/redesign-coverage.mjs walked only .tsx, so the 234 local/spacing-scale suppressions living in .ts StyleSheet modules (*-styles.ts, *.styles.ts, styles.ts) were invisible to the D38 denominator, letting a .ts file no #539 ticket owns silently defeat a future drive-suppressions-to-zero backstop. Extend the walker (renamed walkTsx -> walkSurfaceFiles) to also collect co-located StyleSheet modules, and add a /chat.styles matcher to R11 so app/chat.styles.ts lands on the same ticket as its sibling chat.tsx. All 24 style modules now map to exactly one ticket; tool exits 0 with zero orphans. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: PR #607 — chore(coverage): extend the D38 gate to co-located StyleSheet modules
Scope: PR #607 in orbit-ui-mobile
Recommendation: APPROVE
Summary
Single-file diff (tools/redesign-coverage.mjs, +13/-8) widens the D38 orphan-file gate from .tsx-only to also walk co-located StyleSheet modules (*.styles.ts, *-styles.ts, styles.ts), and widens the R11-screen-astra FILE_RULES entry to match /chat.styles so apps/mobile/app/chat.styles.ts doesn't fall through as an orphan. No prior review threads exist on this PR (checked via gh pr view --json reviews,comments and a GraphQL review-threads query — only an automated Vercel comment). All 24 style modules currently in apps/mobile/apps/web were manually traced against the ordered FILE_RULES table; every file resolves to exactly one ticket, including chat.styles.ts, confirming the PR's stated verification.
Findings
Critical: None. High: None.
Medium
[MEDIUM] Gate's own doc describes it as .tsx-only after the scope changed
· dimension: 2 (Dead/stale code) — doc-behavior drift
· location: orbit-ui-mobile/CLAUDE.md:85, orbit-ui-mobile/tools/README.md:23
· issue: Both the root CLAUDE.md docs registry entry for `.claude/manifests/surfaces.json`
and the tools/README.md catalog row for redesign-coverage.mjs still say the D38 walk
covers "every `.tsx` under apps/*/app + apps/*/components". This diff extends the walk
to also include co-located StyleSheet modules, but neither description was updated.
· risk: A future agent reading either doc (both are exactly what this repo points agents
to instead of exploring the codebase) will believe style modules sit outside the D38
denominator and skip claiming them, reintroducing the orphan-file failure this gate
exists to prevent.
· fix: CLAUDE.md:85 and tools/README.md:23 — change "every .tsx ... mapped to exactly one
ticket" to "every .tsx and co-located StyleSheet module ... mapped to exactly one ticket".
· reference: root CLAUDE.md "When you change a doc, update its At a glance header and this
registry in the same edit"; tools/CONVENTIONS.md "Adding one" (README.md catalog row is
the tool's contract-facing description).
Low/Info: The widened rule's correctness was confirmed by manual trace against FILE_RULES (direct script execution was unavailable in this environment).
Subagents
All five gated N/A: the diff touches only tools/redesign-coverage.mjs — no apps/web/apps/mobile file, no i18n string, no shared-types/DTO, no orbit-api code, no UI file.
Validation
Skipped per CI-adapted instructions (Build/Unit Tests/SonarCloud run as separate required checks on this PR).
Not verifiable in CI
- Dimensions requiring
orbit-api(contract-aligner, orbit-api side of the backward-compat guard): N/A regardless — diff has no cross-repo surface. - Dimensions 8–14 (DESIGN.md/AI-slop, Parity, i18n, Contract drift, Security, Backend hard rules, FEATURES.md parity): all N/A — the diff's only file is outside every workspace's UI/i18n/shared-types/
orbit-apisurface.
What's good
- The
/chat.stylesaddition is minimally targeted at exactly the one file it needs to catch, not a generic loosening. - The header comment states the WHY (style modules carry
local/spacing-scale/z-index suppressions the redesign must own). isStyleModulematches exactly the three naming conventions actually in use (verified no*.style.tssingular or other variant exists that would evade it).walkTsx→walkSurfaceFilesrename has no dangling references anywhere in the repo.
Recommendation
Approve. The one Medium finding (stale .tsx-only wording in CLAUDE.md and tools/README.md) is worth a quick follow-up but doesn't block merge — Medium never forces a request-changes verdict per the rubric's signal gate.
| Severity | Count |
|---|---|
| Critical | 0 |
| High | 0 |
| Medium | 1 |
| Low/Info | 1 |
|
There was a problem hiding this comment.
Code Review: PR #607
Recommendation: REQUEST CHANGES
Summary
PR #607 (chore/coverage-gate-style-modules → main) set out to extend tools/redesign-coverage.mjs's D38 walker to co-located StyleSheet modules. That work is real, but it already shipped independently as PR #587 (chore(coverage): extend the D38 gate to co-located StyleSheet modules, squash-merged as 6546a627 on 2026-07-24). This branch subsequently merged main a second time (commit d721dce8), which absorbed that already-merged change back into itself. The net effect: PR #607's current head is now byte-identical to main across the entire repository.
Findings
High
PR #607 is now a no-op duplicate of already-merged PR #587
- Evidence:
git diff ea0e179ab0b263d8d690e97b2efa85d29cc3a77d HEAD --stat(main vs. PR head) returns completely empty — zero files, zero lines changed anywhere in the repo. Independently verified in this review session.git log -- tools/redesign-coverage.mjsshows the only commit touching that file since #587 landed isea0e179a(#606, unrelated harness-execution work), which is already present on both branches. - Risk: The PR body's own description and verification steps assert this PR ships a code change to
tools/redesign-coverage.mjs. It does not — merging it would create a squash-merge commit inmain's history that claims a change while shipping a literal empty diff, muddying the D38 audit trail and changelog provenance. A prior review of this PR (before its second merge ofmain) approved a real+13/-8diff that no longer exists on this head; that approval is stale and should not be used to fast-track merging. - Fix: Close PR #607 as a duplicate of already-merged #587 rather than merging it. There is nothing to recover — the underlying engineering is sound and already shipped.
Medium / Low / Info
None attributable to this PR's diff (which is empty).
Subagents
N/A — diff touches no files, so parity-checker, i18n-syncer, contract-aligner, security-reviewer, and design-reviewer all have nothing to verdict.
Validation
Skipped per CI instructions (Build/Unit Tests/SonarCloud run as separate required checks on this PR).
What's good
The underlying engineering (widening the D38 walker to co-located StyleSheet modules) is sound and already correctly reviewed and shipped via #587. Merging #607 would not break main (diff is empty, so it's harmless at runtime) — the objection is git-history hygiene and the stale PR description, not correctness risk.
Recommendation
Close PR #607 without merging — it duplicates already-merged PR #587 and its diff against main is empty.
| Severity | Count |
|---|---|
| Critical | 0 |
| High | 1 |
| Medium | 0 |
| Low / Info | 0 |
|
Closing as a duplicate, confirmed empty. The reviewer is right and I opened this PR in error. The D38 style-module widening already shipped as #587 (squash-merged My mistake, worth naming so it does not repeat. I found this branch with Nothing is lost. The engineering is on |
1 participant
What
tools/redesign-coverage.mjsnow walks co-located style modules (*.styles.ts,*-styles.ts,styles.ts) alongside.tsx, so each must map to exactly one #539 ticket by directory rule.Why
D38 asserts that every file under
apps/*/appandapps/*/componentsis claimed by exactly one redesign ticket, because the surface manifest only sees files a surface closure reaches. The gate implemented that for.tsxonly.Style modules are the wrong thing to exclude: they carry the
local/spacing-scaleand z-index suppressions the redesign has to own. A ticket could be signed off as visually complete while the file holding its actual spacing violations was never claimed by anything.One rule needed widening as a result:
R11-screen-astranow also matches/chat.styles, since the chat screen's style module sits besidechat.tsxrather than inside a/chat/directory.Verification
node tools/redesign-coverage.mjsexits 0 on this branch, so the widened denominator is fully claimed with no orphans. Exit 1 remains the behaviour for any unclaimed surface or orphaned file.Provenance
Written earlier on 2026-07-24 and pushed, but no PR was ever opened, so it sat on the remote unreviewed. Found while sweeping for unmerged work; opening it rather than losing it.
🤖 Generated with Claude Code