Replace Unicode glyphs with vendored Bootstrap Icons - #128
Merged
Conversation
The chrome carried two kinds of icon and neither could be reasoned about. Fold state and resource markers were **Unicode text** — `▶ ▼`, and `▣ ▶ ▤ ◇ ≡ ◆` for a resource's class — so a row's marker said "not a note" and stopped there: the shape carried nothing about *what* the file was, at whatever weight the system font felt like. Everything else was **inline SVG pasted per call site**, each with its own stroke width and its own idea of 15px vs 16px, so a second copy of a chevron could (and did) drift from the first. Both are now one thing: a name in `ui/src/icons.ts`, resolving to path data vendored from Bootstrap Icons (twbs/icons, MIT). **The split.** `icons.gen.ts` holds shapes; `icons.ts` holds meanings. Call sites ask for `resourceIcon(r.class)` or `foldChevron(open)`, never for a literal icon name — so "what does a PDF look like in B2" has one answer, and changing it is a line here rather than a grep across three panes and a graph. **Why generated rather than imported.** Three constraints, one shape that satisfies all of them: the webview CSP is `default-src 'self'` (no CDN); `npm test` runs off the source through node, where the Vite-native `?raw` import wouldn't resolve, taking every test that transitively imports render.ts with it; and 2078 icons ship where 24 are used. So the package is a devDependency — provenance and the upgrade path — and `scripts/gen-icons.ts` lifts the named subset into a checked-in `.ts` file. Its `--check` mode runs first in `npm test`, exiting non-zero on a stale file, because a check that reports a problem and exits 0 is a hole in the gate. **The tree** spends two slots per row now: the chevron says *this folds*, the icon says *what this is*. A file leaves the chevron slot empty, which is what lines its icon up under the folder's icon rather than under its chevron. Everything else follows the same registry: the frontmatter drawer and the discovery pane's section and card folds (one `foldCaretHtml`, so three pasted `▶`/`▼` pairs can't drift), the graph's resource and dangling nodes (placed by `sceneIcon`, fill left to CSS), and the nine hand-drawn chrome icons. Two accessibility notes. `icon()` is `aria-hidden` with no opt-out: an icon here is always beside an accessible name, never instead of one — so the source toggle, which used to name itself with the literal characters it printed, gains an `aria-label`, and the broken-link and unreadable-frontmatter markers gain `role="img"` over labels a span alone would have had ignored. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PVBNfpZv9FSj2YDtUFStDK
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe UI now uses a centralized, typed Bootstrap Icons registry. A generator vendors a curated icon subset, tests validate the registry, and UI rendering and CSS use shared icon helpers. ChangesCentralized Bootstrap icon system
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant BootstrapIcons
participant gen-icons.ts
participant icons.gen.ts
participant icons.ts
participant UI
BootstrapIcons->>gen-icons.ts: provide upstream SVG files
gen-icons.ts->>icons.gen.ts: write normalized icon bodies and version
icons.gen.ts->>icons.ts: expose ICON_BODIES
icons.ts->>UI: render semantic HTML or scene icons
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@ui/scripts/gen-icons.ts`:
- Around line 107-122: Update the generated header emitted by the icon generator
around ICON_BODIES to include the complete Bootstrap Icons MIT permission
notice, or reference a committed third-party notices file shipped with the
application; ensure the generated src/icons.gen.ts contains this attribution
without relying solely on ui/node_modules.
In `@ui/src/icons.ts`:
- Around line 51-54: Escape class values with escapeHtml before interpolating
them into SVG markup in ui/src/icons.ts lines 51-54 and 67-71, covering both
opts.class and cls while preserving existing class composition. Add a
quote-containing class test in ui/src/icons.test.ts lines 143-151 and verify the
quote remains escaped inside the class attribute.
In `@ui/src/main.ts`:
- Around line 392-395: Update the badge.innerHTML construction near icon() to
escape the anomaly count before interpolation, using escapeHtml(String(n)) for
the span content while preserving the existing icon markup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 95bdf150-b2a2-4e9b-9311-290de71be8d3
⛔ Files ignored due to path filters (1)
ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
CLAUDE.mdcrates/b2-desktop/CLAUDE.mdjustfileui/package.jsonui/scripts/gen-icons.tsui/src/icons.gen.tsui/src/icons.test.tsui/src/icons.tsui/src/main.tsui/src/render.tsui/src/treenav.tsui/style.css
Three findings from review on #128. Two were right; the third was right about the rule and wrong about the fix. **The license was the real defect.** `icons.gen.ts` carried the copyright notice and pointed at `ui/node_modules/bootstrap-icons/LICENSE` for the rest — a file that is neither committed nor shipped, so anyone receiving the generated data received it without the terms it is used under. MIT requires the copyright notice *and* the permission notice to travel with any copy. The notice is now emitted from one source in the generator into two forms, because source and bundle are different distributions and a comment only covers the first: a `/*!` legal comment, which is the marker esbuild keeps when it strips every other comment (verified — it reaches ui/dist), and an exported string, which is what the suite can assert against. The string is tree-shaken out of the bundle, so the notice ships once. `--check` alone could not have guarded this: it compares the file to what the generator emits, so a generator edited to drop the notice would pass. The obligation now has a check that fails. **Class values are escaped at the emitter.** Every caller passes a literal, and a CSS class is chrome vocabulary that has no business carrying vault data — but that is the argument for escaping in `icon()`/`sceneIcon()` rather than against it. This is a markup emitter, and the seam is where this codebase makes rules structural instead of remembered, the same reason `sanitize.ts` hangs off `marked`'s postprocess hook rather than each caller (E5). **The anomaly badge drops the interpolation instead of escaping it.** The review asked for `escapeHtml(String(n))`; escaping a number is a ritual that teaches the reader to stop asking which values need it. `innerHTML` now takes a hoisted constant and the count is `append`ed as a text node, which no parser ever sees — stronger than escaping, and it stays correct if `n` ever stops being a number. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PVBNfpZv9FSj2YDtUFStDK
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces hand-rolled inline SVG and Unicode text glyphs throughout the UI with a curated subset of Bootstrap Icons (v1.13.1), vendored at build time. This unifies the icon family, makes meanings explicit (e.g., "what does a PDF look like?"), and ensures consistency across the app.
Key Changes
Icon registry (
ui/src/icons.ts): New module that maps semantic names to icon meanings —resourceIcon(class),foldChevron(open),folderIcon(open),directionIcon(direction)— so the answer to "what does a PDF look like in B2" is one place, not scattered across call sites.Icon vendoring (
ui/scripts/gen-icons.ts): Generator that lifts path data from thebootstrap-iconsnpm package intoui/src/icons.gen.ts. Runs at build time (not runtime) because the webview CSP forbids a CDN, andnpm testruns off the source through node where Vite's?rawimports don't resolve. The--checkmode gates CI: a stale generated file fails the test suite.Icon test suite (
ui/src/icons.test.ts): Pure-logic tests (no DOM, no dependencies) that verify every semantic name resolves to a real icon, that markup stays the shape the panes assume, and that the vendored data carries no scripts or handlers.Render layer (
ui/src/render.ts): Replaces Unicode carets (▶/▼) and hand-rolled inline SVG with calls toicon()andsceneIcon(). Tree rows now have two explicit slots — fold chevron and thing icon — so files line up under their folder's icon rather than under the chevron.Tree navigation (
ui/src/treenav.ts):TreeFile.glyph(a string) becomesTreeFile.icon(anIconName), keeping the module DOM-free so node can run its test off the source.Styling (
ui/style.css): New.iconrule (flex, inline-block,vertical-align);.tree-caretand.tree-iconnow flex containers that center their SVG children; removed font-size and text-align rules that applied to Unicode glyphs.Shell and controls (
ui/src/main.ts): Navigation buttons, the anomaly badge, and other chrome now useicon()instead of pasted SVG.Notable Details
Semantic names, not icon names: Callers ask for
resourceIcon("pdf")orfoldChevron(true), not"file-earmark-pdf"or"chevron-down". A typo is a type error; adding an icon is a name in the generator's manifest plusjust icons.No color of its own: Every icon uses
fill="currentColor", so hover,is-active, and graph node states tint the glyph by settingcoloron the parent — no per-icon color rules.Accessibility: Every icon is
aria-hidden="true"with no opt-out. Icons are always beside their accessible name (a label or visible text), never instead of it.Scene icons:
sceneIcon()places a nested<svg>in graph coordinates, deliberately setting nofillso the stylesheet's.gglyphrule is the only thing that colors it.Generator gate:
just test-uirunsgen-icons.ts --checkfirst, so a stale generated file fails the test suite before shipping. Bumping thebootstrap-iconsdevDependency requires re-runningjust iconsand committing the result.https://claude.ai/code/session_01PVBNfpZv9FSj2YDtUFStDK
Summary by CodeRabbit
New Features
Accessibility
Documentation