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.
|
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。
|
/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}
|
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. |
d4259f4 to
d99b72c
Compare
|
Hi — this PR conflicts with current I tested a rebase onto current
These are real source conflicts, so they need your judgement rather than a mechanical rebase — please rebase onto current Thanks for the contribution — happy to help if any conflict is unclear. AI-assisted maintenance note, not a review. It does not count as the required human review under |
|
@Astro-Han Rebased onto current The five governed 18px-era additions from current main are now migrated: three queue-row/retract icons in Local validation passed: icon checker 7/7 plus full repository scan, UI 213/213, workspace dependency build, Desktop typecheck, lint, format, ASF headers, and diff checks. I also rendered the real AppShell Storybook surface with both queued-message rows and the Queue/Steer control; all five target SVGs measured 14×14 with no clipping or layout shift. Fresh CI is pending and may need workflow approval. |
d99b72c to
e75619a
Compare
|
@Astro-Han Rebased onto current The conflict resolution preserves the current Exact-head local validation:
Ready for fresh exact-head CI. The fresh workflows are waiting for maintainer approval: CI 32638150941, Dependency audit 32638150976, and Release Windows check 32638151196. Please approve all three exact-head runs. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the rework — the earlier findings all look closed to me at e75619a0c35d5ad1d93bcc0e6f90830f2982172a. The five 18px call sites in remote-project-directory-dialog.tsx are migrated to ICON_SIZE, and @babel/parser is correctly a devDependency used only by the checker, so it stays out of shipped artifacts. One thing left below.
[P2] The required icon-size gate misses computed lookups into a statically closed icon record
packages/ui/src/task-ledger-panel.tsx:37-44 declares
const STATUS_ICONS = {
pending: Clock,
in_progress: CircleGauge,
...
} satisfies Record<TaskStatus, typeof Clock>;Every value is a canonical Lucide import and the type is closed, so this is exactly the "derived icon collection" the checker's own docstring (scripts/check-icon-size.mjs:21-25) promises to govern. But :175 reaches it with a computed lookup, const StatusIcon = STATUS_ICONS[task.status], and inferExpressionValue at scripts/check-icon-size.mjs:136-148 only infers member expressions guarded by !unwrapped.computed. The computed form falls through to unknown, so isGovernedIconTag (:414-417) does not treat <StatusIcon> at :191 as a governed icon.
Reproduced on an exact-head worktree:
| probe | result |
|---|---|
| unmodified head | icon size scale: ok, exit 0 |
<StatusIcon size={16}> |
icon size scale: ok, exit 0 |
<StatusIcon size={13}> |
icon size scale: ok, exit 0 |
positive control — <AlertTriangle size={13}> in chat-model-switcher.tsx:240 |
exit 1, correctly flagged |
The positive control confirms the checker itself works; the computed lookup is what escapes it.
To be explicit about severity: the UI is correct today, so there is no runtime defect. The problem is contract scope — a maintainer can put an arbitrary off-scale raw pixel on this existing status icon and the required gate stays green, while the script advertises coverage of derived collections.
Suggested direction: when a static record's values are all icons, preserve that provenance through a computed lookup so the element kind stays icon, and add a STATUS_ICONS[status] regression case. If you would rather not generalize, the alternative is to narrow the docstring and drop the claim of covering derived collections — the risk is leaving the middle state, where the promise is broader than the analysis.
ran: exact-head worktree checker baseline, the three mutation probes and the positive control above, plus a read of the record/lookup/JSX-tag branches in the checker.
did-not-run: the repository test suites. Hosted checks at this exact head are not fully terminal yet — audit is success; the CI and Windows runs were still in progress at the time of writing, so please treat their outcome as unverified here.
|
Follow-up on the CI note in my previous review: hosted checks at |
e75619a to
9500d9b
Compare
|
@Astro-Han Addressed the remaining P2 at Computed record lookup now preserves icon provenance only when the object literal is statically closed, non-empty, and every value resolves to a governed icon. Mixed records and records containing computed properties or spreads remain unknown, so the checker does not broaden into false positives. Regression coverage includes the concrete Exact-head local validation passed:
Fresh hosted CI is pending on the new head. |
| variant="ghost" | ||
| size="sm" | ||
| icon={<Copy size={14} aria-hidden="true" />} | ||
| icon={<Copy size={ICON_SIZE.chrome} aria-hidden="true" />} |
There was a problem hiding this comment.
[P2] Same leftover pixel, different rung.
This Copy sits on Button size="sm", same as the copy-path button above (:201), which already uses ICON_SIZE.control (14). The raw size={14} here was that same 14.
ICON_SIZE.chrome is 16 — the Astryx sm slot fill. Mapping only this leftover to chrome changes the glyph and splits two identical copy-on-sm-button call sites.
If the intent is “sm button → chrome”, both should move. If the intent is “preserve the leftover 14 / match the sibling”, this should be ICON_SIZE.control. The checker only bans raw pixels, so it cannot catch a wrong rung.
| key={candidate.id} | ||
| label={candidate.label} | ||
| icon={<FolderOpen size={18} aria-hidden="true" />} | ||
| icon={<FolderOpen size={ICON_SIZE.control} aria-hidden="true" />} |
There was a problem hiding this comment.
[P3] Same leftover 18px, two rungs.
FolderOpen / Check size={18} → ICON_SIZE.control (14). A few lines down, Eye / EyeOff size={18} → ICON_SIZE.chrome (16). Role-based snapping is what the scale is for (list startContent vs footer affordance), but 18 was not a rung, so both glyphs change size and they no longer match each other. Calling it out so the visual delta is deliberate, not an accident of nearest-rung.
| className="settingsAboutHero" | ||
| as="h2" | ||
| icon={<Sparkles size={30} /> /* 64% of the 48px plate, matching .providerLogo's fill */} | ||
| icon={<Sparkles size={ICON_SIZE.plate} /> /* plate glyph inside the About hero mark */} |
There was a problem hiding this comment.
[P3] size={30} was “64% of the 48px plate” (≈30.7). ICON_SIZE.plate is 28, so this shrinks 2px. Fine if plate is the law; the old 64% comment should not be read as still true.
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Independent review (kabi-grok)
Exact head 9500d9bc610471190e1775e2b4d74181c9c4ebe7. COMMENT only; not merging.
ICON_SIZE rungs on this head: meta 13 / control 14 / chrome 16 / empty 20 / plate 28.
What looks right
Trash2 size={16}→ICON_SIZE.chromepreserves 16.- Story
size={20}→ICON_SIZE.emptypreserves 20. check:icon-sizeis wired on thecodepath inci.yml.teston this SHA iscompleted/success.- I did not find remaining raw lucide
size={N}in the checker globs on this head (the leftover product pixels this PR names are gone as raw numbers).
Findings
Inline comments have the file:line detail.
- [P2]
session-inspector-panel.tsx:733— leftoverCopy size={14}mapped tochrome(16). The other sm-button Copy in the same file (:201) already usescontrol(14). Same pixel, same button size, now two rungs. Checker cannot catch a wrong rung. - [P3]
remote-project-directory-dialog.tsx— leftover18splits tocontrol(14) vschrome(16). Role-based snapping is the scale’s point; the glyphs no longer match and both change size. - [P3]
about-settings-page.tsx:170—30→plate(28), 2px under the old “64% of 48px” comment. - [P3]
scripts/check-icon-size.mjs(~553 lines +@babel/parser) is a lot of machinery to keep five rungs. It is a real guard, not dead code; just heavier than the five call sites. No change requested.
Checks
test: completed / success on this head.auditand Windows package: action_required (fork workflow approval), not success. I am not treating them as green. I am also not waiting on checks this path filter would not schedule.
Verdict
NO-GO for APPROVE until the P2 Copy rung is resolved (either both sm Copies use control, or both use chrome). No REQUEST_CHANGES.
3 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