refactor(web): route all icon imports through one barrel - #7963
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
github-actions
Bot
added
size:XL
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
|
UI consistency: one web file still bypasses the new
import {
BotIcon,
CircleAlertIcon,
PencilRulerIcon,
type LucideIcon,
LockIcon,
LockOpenIcon,
PenLineIcon,
SparklesIcon,
XIcon,
} from "lucide-react";Two consequences that come from lines changed in this PR:
Smallest fix: change that one specifier to } from "~/icons";(Posted as a conversation comment rather than inline because the file is not part of this diff.) Posted via Macroscope — UI Consistency |
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This is a cross-cutting web import refactor with a new custom scanner controlling the generated production icon surface. An unresolved review comment identifies a concrete JSX parsing case that can omit required icon exports, so the parser and generated barrel need human assessment. Not approved because:
Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more. |
|
Good catch — fixed in c67ca0c. The codemod's grep had skipped ChatComposer.tsx because the file contains literal NUL bytes in a template string (grep binary detection); the sweep now reads file bytes instead, and a re-scan confirms zero direct lucide-react imports remain outside the barrel. Macroscope's UI Consistency check passes on the latest commit. |
|
Factual check on the Macroscope UI-consistency comment against current head The separate unresolved generator finding is still valid: declaration-level |
4e958a7 to
7e8c6c1
Compare
c0fb01a to
d0a4c9c
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6b9f440. Configure here.
| characters[index] = " "; | ||
| } | ||
| continue; | ||
| } |
There was a problem hiding this comment.
JSX tag state ignores attribute expressions
Medium Severity
jsx-tag never enters attribute {...} expressions and treats every > as the tag closer. Common TSX such as onClick={() => ...} or count={n > 0} drops the scanner into jsx-text, which blanks later ~/icons imports so the generated barrel can omit them.
Reviewed by Cursor Bugbot for commit 6b9f440. Configure here.
collectRuntimeIconNames treated block and line comments inside import
braces as part of member names, so icons:generate and icons:check threw
on valid TypeScript like `import { /* primary action */ XIcon } from
"~/icons"`. Strip comments from the brace contents before splitting
members, with regression coverage for inline, multiline, and trailing
comment forms.
Co-Authored-By: Claude <noreply@anthropic.com>
6b9f440 to
7fae5ca
Compare
| character === "/" && | ||
| next !== "/" && | ||
| next !== "*" && | ||
| canStartRegex(characters, index) |
There was a problem hiding this comment.
🟡 Medium icons/icon-import-parser.ts:84
A valid division expression such as count++ / divisor causes maskNonCode to treat the / as a RegExp start, so it masks through the later module specifier and collectRuntimeIconNames omits the real CheckIcon import. canStartRegex treats any preceding + as a regex trigger without distinguishing postfix ++; update it to recognize postfix increments and classify this slash as division.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/icons/icon-import-parser.ts around line 84:
A valid division expression such as `count++ / divisor` causes `maskNonCode` to treat the `/` as a RegExp start, so it masks through the later module specifier and `collectRuntimeIconNames` omits the real `CheckIcon` import. `canStartRegex` treats any preceding `+` as a regex trigger without distinguishing postfix `++`; update it to recognize postfix increments and classify this slash as division.
1 participant
1. Observed problem and reproduction
Web interface icons were imported directly from
lucide-reactacross roughly 140 source and test files. Auditing or replacing the icon set required a repository-wide call-site sweep, and nothing prevented new direct imports.To reproduce, open a web component that renders an interface glyph and inspect its imports. Before this PR, the component imported the Lucide symbol directly. The expected model is one owned icon module with an enforced import boundary.
2. Cause
The web client had no icon module and no lint rule that owned Lucide imports. Without that boundary, each call site chose its dependency source independently.
3. Change and boundary
This PR adds a generated
~/iconsbarrel, changes web call sites to import from it, and adds a restricted-import rule that blocks direct root and subpathlucide-reactimports outside the icon module.apps/web/scripts/generate-icon-barrel.tsscans real source and test usage;--checkverifies that the generated barrel is current. Focused parser and identity tests cover type-only imports, comments, strings, templates, regex literals, and JSX text.The conflict pass rebased the seven owned commits from old head
6b9f44009onto exact livemainhead1f8ed54ad, then added one conflict-adaptation commit. Upstream's composer refactor deletedThreadSyncStatusPill.tsx; it remains deleted. Four import-block conflicts kept the complete upstream files and changed only their Lucide import sources. Two new upstream composer files now use the same barrel, and regeneration added the upstream-introducedFileTextIconexport.The resolution used a rebase because the PR has no human approval or submitted human review whose commit identities need preservation. The range-diff maps all seven original commits to their rebased equivalents.
4. Intentionally unchanged or deferred
The barrel is a pure re-export. Icon choices, component objects, props, layout, animation, interactions, and all upstream composer behavior are unchanged.
This PR does not change React Native mobile, SwiftUI mobile, the desktop main or preload processes, providers, contracts, server behavior, or local, remote, relay, and tunnel connection behavior.
One current Cursor finding against the custom TSX scanner remains unresolved: JSX tag attribute expressions containing
>can make the scanner enter JSX-text state too early and miss a later icon import. This conflict-only pass did not broaden into a parser repair. The PR should not be treated as review-ready until that core finding is resolved or disproved with focused coverage.5. Affected areas
Affected: the web client and the desktop renderer that wraps it, web source and test icon imports, the generated icon barrel, web lint configuration, and maintainer documentation.
Unaffected: mobile clients, desktop shell and IPC, provider adapters, wire contracts, server projections and orchestration, reverse actions, and every connection mode. The change has no provider-specific path.
6. Validation
Current head:
7fae5ca03.CI=true vp i: passed after the isolated worktree's first test attempt showed that project-local dependencies were absent.CI=true vp test run apps/web/src/icons/icon-imports.test.ts apps/web/src/icons/index.test.ts apps/web/src/components/chat/ComposerStashMenu.test.tsx apps/web/src/components/chat/ComposerPendingUserInputPanel.test.tsx: passed, 4 files and 13 tests.CI=true vp run --filter @t3tools/web icons:check: passed; verified 202 generated exports.CI=true vp lint apps/web/scripts/generate-icon-barrel.ts apps/web/src/icons/icon-import-parser.ts apps/web/src/icons/icon-imports.test.ts apps/web/src/icons/index.test.ts apps/web/src/icons/index.ts apps/web/src/components/chat/ComposerActivityStatus.tsx apps/web/src/components/chat/ComposerBanner.tsx apps/web/src/components/chat/ComposerBannerStack.tsx apps/web/src/components/chat/ComposerPendingUserInputPanel.tsx apps/web/src/components/chat/ComposerStashMenu.tsx apps/web/src/components/chat/ComposerTasksBadge.tsx vite.config.ts: passed.CI=true vp fmt --checkover the same 12 files: passed.CI=true vp run --filter @t3tools/web typecheck: passed.git diff --check origin/main..HEAD: passed.lucide-reactimports outsideapps/web/src/icons.git range-diff d22709f759ced90b0ae7b7cb73d7ad22c2ce962b..6b9f440097c0b914454b0ad74679020408d85e52 1f8ed54add4133ac39effceded8fc1fff12d8e03..28642df5190c7802a1104738d938a16643ba04e9: all seven original commits map to the rebased commits; differences in conflict files are upstream context and the intended import-source adaptation.7. Risks, limitations, and known gaps
icons:checkis not a separate CI job. Web typecheck catches missing used exports, while the explicit freshness command also catches dead exports and ordering drift.CODING_STANDARDS.mdblob144fbf46af335d8d18a95c8d4b4f2e9e0207fa2efound no actionable conflict-resolution issue inorigin/main..7fae5ca03.8. Before and after evidence
The previous proof round captured Settings at merge-base
d22709f75and PR head3b3af3cb4. Those pairs were SHA-256 identical and ImageMagick reported zero differing pixels. The rebase preserved the pure re-export model, but these images do not prove current head7fae5ca03.Before, dark mode
After, dark mode
Historical comparison
Historical light-mode evidence
Before
After
Historical comparison
No video applies because the PR changes no motion or interaction.
9. Owning issue and stack
No upstream issue or stacked PR owns this maintenance refactor. The PR targets
main, contains only the icon-boundary change and its focused follow-up fixes, and has maintainer edits enabled.Model: GPT-5.6 Sol high
Harness: Codex in T3 Code
Note
Route all
lucide-reacticon imports through~/iconsbarrel inapps/webscripts/generate-icon-barrel.tswhich scansapps/web/srcandapps/web/testfor icon usage, validates names againstlucide-reactexports, and generatessrc/icons/index.tsas a pure re-export barrel. Includes--checkmode that errors if the barrel is stale.icon-import-parser.ts(collectRuntimeIconNames,assertKnownIconNames,maskNonCode) that strip comments, strings, regex, and JSX before matching import statements.~/iconsinstead oflucide-react.icons:checkandicons:generatenpm scripts, updatesvite.config.tslint to forbid directlucide-reactimports, and documents the policy indocs/internals/icons.md.vite.config.tslint rule now blocks directlucide-reactimports; any new file importing fromlucide-reactwill fail lint until routed through~/iconsand the barrel regenerated viaicons:generate.Macroscope summarized 7fae5ca.