fix(ui): govern leftover icon sizes with ICON_SIZE - #3127
Conversation
|
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)
Included review availability: Your plan includes up to 3 reviews per rolling hour; 0 remain after this review. 📝 WalkthroughSummaryThis PR completes the governed icon-size migration for the remaining The Babel-based scanner resolves supported imports and bindings. It excludes comments, strings, unrelated The check runs through Source of truthThis PR extends the existing Scope and complexityThe call-site changes are small and coherent. The parser-based scanner adds necessary complexity because text matching cannot resolve imports or distinguish icon props from unrelated syntax. No safe deletion or simplification is evident. Removing scanner cases or tests would weaken enforcement or regression coverage. Risks and validation
Review-relevant risksThe scanner changes repository governance by enforcing the icon-size scale in CI. Any material change to this governance behavior requires independent human review under repository policy. No user-visible behavior, public contract, security, licensing, or release effect was identified in the current diff. The person performing the merge must review the final diff, and a maintainer makes the final determination. WalkthroughThe pull request adds Babel-based validation for numeric sizes on imported Lucide icons. It replaces three hardcoded icon sizes with shared constants, adds scanner tests, and runs the new check in CI. ChangesIcon size governance
Estimated code review effort: 4 (Complex) | ~30 minutes Merge Risk: ⚪ Minimal · up to This change centralizes the remaining icon sizes and adds CI enforcement; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant CI
participant checkIconSize
participant SourceFiles
participant BabelParser
CI->>checkIconSize: run check:icon-size
checkIconSize->>SourceFiles: scan configured source globs
SourceFiles-->>checkIconSize: return source text
checkIconSize->>BabelParser: parse TSX
BabelParser-->>checkIconSize: return AST
checkIconSize-->>CI: report violations or success
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: afd08f49-0d1a-411c-9cee-1adc4872f934
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
.github/workflows/ci.ymlapps/desktop/src/renderer/settings/about-settings-page.tsxapps/desktop/src/renderer/settings/request-customization-editor.tsxpackage.jsonpackages/ui/stories/icons.stories.tsxscripts/check-icon-size.mjsscripts/check-icon-size.test.mjs
Included review availability: Your plan includes up to 3 reviews per rolling hour; 0 remain after this review.
Shadowed parameters and local tags are no longer treated as imported icons. Numeric string sizes such as size="16" are rejected. The Comp story is governed only when Comp is actually bound to the icon seam.
Biome wants the long boolean and defineBinding calls wrapped.
|
Thanks for routing the leftover sizes through ICON_SIZE — the two product mappings are correct and the check does run on real TSX. Two P2s surfaced in review; both need a decision before merge. Conclusion: PASS with two mandatory-handling P2s. P2-1 — the About hero change is a visual change, not a routing: P2-2 — the 282-line AST checker is oversized for the threat and has blind spots. Optional nits (P3): the story "governance" is nominal — the check can't see AI-assisted review disclosure: this review was produced with AI assistance (two pi review subagents on 中文摘要（AI 辅助审查）结论：PASS（两个 P2 需处理）。① P2-1：About hero 的 size={30}→ICON_SIZE.plate(28) 是未披露的视觉变更——Sparkles 缩小 2px（占板比例 62.5%→58.3%），且删除了"64% of 48px plate, matching providerLogo"的注释依据，而 providerLogo 仍用 64% 填充惯例；PR 描述暗示纯路由但实际改了像素。需明确确认收敛到 28 是有意的（并在描述中说明），或按 issue #2359 规则保留 30 作为记录在案的例外。② P2-2：282 行手写 AST lint 超配——被治理语料全仓仅 3 处裸像素（都是本 PR 修的），命名空间/元数据机制是可证明的死逻辑（约 100 行），且 8 种平凡间接形式（变量、spread、模板串、表达式、字符串容器、参数透传、动态 import、本地 re-export）实测均可绕过，治理声明被高估。建议：给全 AST 方案理由、删死逻辑、或明确声明覆盖边界并写进测试。P3 可选：story 治理是表面治理（解构标签不被查）、GLOBS 不含 apps/desktop/stories、模板字面量未测、描述档位名与代码不符。 |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Routing the remaining product call sites through the existing ICON_SIZE seam is the right direction, and this head fixes the earlier shadowing and direct numeric-string false negatives. The focused tests and live checks are green on its old base.
The new required check does not pass the current-main merge result: current main added five governed 18px icons after this branch's base. Please rebase and migrate those sites before merging. More broadly, the first-principles contract should match what the checker can actually prove. The smallest coherent choice is either (a) explicitly enforce direct JSX literals and delete the unused/generalized scope machinery, or (b) expand static-value and controlled-derived-icon coverage. A 319-line authority that claims all raw pixels while accepting common indirections is the least desirable middle ground.
Reviewed with Codex using two independent reviewer agents; I reproduced the current-main gate failure and verified the latest head, changed checker, public discussion, and live CI.
中文
把剩余产品调用点接入现有 ICON_SIZE seam 的方向正确，这个 head 也修复了之前的遮蔽误报和直接数字字符串漏报；聚焦测试与旧 base 上的实时检查均通过。
但新增 required check 无法通过与当前 main 的合并结果：当前 main 在该分支 base 之后新增了 5 个受治理的 18px 图标。合并前应 rebase 并迁移这些调用点。更根本地说，checker 能证明的范围应与契约一致。最小的一致方案要么只明确治理直接 JSX literal，并删掉未产生价值的泛化 scope 逻辑；要么补齐静态值和受控派生图标的覆盖。319 行的 authority 一边声称禁止所有 raw pixel，一边允许常见间接写法，是最不理想的中间状态。
本次由 Codex 配合两个独立 reviewer agent 审查；我复现了 current-main gate 失败，并核验了最新 head、checker 变更、公开讨论和实时 CI。
| - run: npm ci | ||
| - run: npm run lint | ||
| - run: npm run format:check | ||
| - run: npm run check:icon-size |
There was a problem hiding this comment.
P1 — This required gate fails after merging current main. Current main added remote-project-directory-dialog.tsx after this branch's base, with five governed Lucide icons still using size={18} (lines 207, 209, 256, 277, 278). Applying this PR and running the scanner reports all five, so the merged result cannot pass this new CI step despite the old-base checks being green. Rebase, map those sites to the appropriate ICON_SIZE rung, and rerun this job.
| !attribute.value | ||
| ) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
P2 — Make the stated governance boundary honest. The scanner still accepts common static raw-pixel forms such as size={'16'}, size={`16`}, size={8 + 8}, spreads, and prop passthrough; controlled <Comp> icons in the changed story are also outside the authority. Either narrow the contract to direct literals and simplify this 319-line implementation, or add explicit static-value/derived-icon coverage and fixtures.
|
/agentic_review |
Code Review by Qodo
1. Dynamic icons escape governance
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| function bindParams(scope, node) { | ||
| const params = node.params ?? []; | ||
| for (const param of params) { | ||
| for (const name of patternNames(param)) defineBinding(scope, name, 'not-icon'); | ||
| } |
There was a problem hiding this comment.
1. Dynamic icons escape governance 🐞 Bug ⚙ Maintainability
Fix-now: callback parameters are always bound as non-icons, so the Comp produced from
Object.entries(Icons) is not recognized and changing the story back to <Comp size={20}> produces
no violation. The CI check therefore does not enforce the exact story call site this PR claims to
govern.
Agent Prompt
## Issue description
The icon-size checker treats every function or callback parameter as `not-icon`. Consequently, the `Comp` parameter in the icon story's `LUCIDE_ICONS.map(...)` callback is disconnected from the icon namespace that populated the collection, allowing raw numeric sizes to pass CI.
## Issue Context
The story intentionally derives its icon list from `Object.entries(Icons)` so it remains synchronized with the centralized icon export seam. Deleting that dynamic path or replacing it with a manually maintained list would duplicate authority; reuse the existing scope/binding analyzer by propagating known icon provenance through the collection and destructured callback binding. Add regression coverage for the actual story pattern; this adds limited data-flow state and corresponding test maintenance because the existing direct-binding model cannot represent this provenance.
## Fix Focus Areas
- scripts/check-icon-size.mjs[151-155]
- scripts/check-icon-size.mjs[251-262]
- scripts/check-icon-size.test.mjs[11-26]
- packages/ui/stories/icons.stories.tsx[32-33]
- packages/ui/stories/icons.stories.tsx[57-70]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
This PR changes icon sizing in visible Settings surfaces. Could you please add screenshots confirming the resulting About hero icon and request-header delete icon? A compact before/after composite is fine. Thanks! Posted by Codex on behalf of Astro-Han. |
2 participants
Summary
ICON_SIZEand--icon-*already define the chrome / dense / hero rungs. Two product call sites and the icon story still passed raw pixels.This change:
size={16},size={30}, and storysize={20}throughICON_SIZEnpm run check:icon-size, which parses TSX so comments, strings, and unrelatedsizeprops are not false positivesFixes #2359
Test plan
npm run check:icon-sizescripts/check-icon-size.test.mjs