Polish Saar v1: visual foundation + audit fixes [GET-13]#55
Merged
DevanshuNEU merged 25 commits intoApr 26, 2026
Conversation
Title moves from "Saar: Enable token tracking for Claude?" to "Enable Saar on Claude?" plus the privacy subtitle, matching the 2026-04-19 thesis lock that repositions Saar as a workflow layer rather than a token tracker. Banner colors are themed via prefers-color-scheme so the panel reads correctly when the user is on a light OS. Hover-only button feedback was the audit L4 finding; :focus-visible now gives keyboard users a 2px terra cotta ring on both buttons. Color hover state moved from JS listeners to CSS so the cascade is single-sourced.
The completion endpoint occasionally returns non-SSE payloads: 429 rate limits, 5xx errors, or captcha/CDN HTML pages. Previously we teed and decoded those anyway, which caused decodeSSEStream to sit silent until the 120s watchdog killed it; meanwhile the overlay stayed pinned on whatever the last healthy turn looked like. Gate the tee on status 200 + content-type containing event-stream. Non-SSE responses pass through untouched, claude.ai surfaces its own error UI, and the overlay holds its last healthy state instead of misleading the user.
Mirrors the new SSE-gate predicate from inject.ts the same way sse-decoder.test.ts mirrors the decoder logic. Cases: - 200 + text/event-stream (with and without charset) -> tee allowed - 429 -> skipped - 500 -> skipped - 200 + text/html (captcha / CDN page) -> skipped - 200 + missing content-type -> skipped - missing body even with SSE header -> skipped Satisfies the new-test acceptance criterion on GET-13.
Audit L2: the nudge element was created without lco-nudge, so the first showNudge() call briefly rendered an unstyled box before the severity class assignment kicked in. Setting the base class at mount means base layout / padding / font rules apply immediately.
CLAUDE.md bans emdashes in code, comments, docs, and commit messages. Comments only here: the audit (L1) called out about two dozen drift instances across ui/overlay.ts, ui/overlay-styles.ts, and the lib/ agent files. UI placeholder strings (overlay.ts lines 26, 140, 175, 213, 233, 256: '—', '—% ctx', etc.) are the user- visible 'no data yet' marker and are deliberately left untouched per the issue: 'Do not touch string literals without review.' Substituted with colons or semicolons; sentences rephrased where the emdash was carrying real semantic weight.
Two-layer color system. The new workshop palette layer carries semantic names (terracotta, sienna, patina, brass, ember, rust, bone, linen, charcoal, slate, ash) so CSS reads like a paint receipt instead of a hex grab-bag. Surface tokens (--lco-bg, --lco-text, etc.) now resolve through that palette so a single token swap propagates everywhere. Health and zone classes move from generic Material colors (#4caf50 / #f5a623 / #ff9800 / #e53935) to the workshop earth tones, both in the side panel and inside the overlay's Shadow DOM. The overlay's on-dark variants are tinted up so contrast holds against the charcoal background. Light-mode warn fill in the overlay drops to a darker brass for readability against bone. Critical pulse keyframes also re-cite the new color so the animation glow matches the dot fill.
Four utility classes (.lco-hero / .lco-section / .lco-label / .lco-data) define a 3x+ size jump with weight extremes (200 for hero, 700 for title, 400 italic for sections). Section headers move from uppercase letterspaced gray to lowercase italic display face: editorial voice rather than corporate dashboard chrome. Title bumps to 24px display weight 700; subtitle drops to 9px mono uppercase letterspaced so the size jump reads. Active conversation subject upgrades to display italic 14px so it anchors the card. Health and zone labels shift to mono uppercase letterspaced for a status-bar 'verdict' feel. Budget status line moves to display 15px so the dollar/percentage figure is the de facto headline without restructuring markup. Font stack lists Fraunces / IBM Plex first with system fallbacks; no webfont files are loaded in this PR (deferred to GET-32). The hierarchy works on system fonts today; dropping the woff2 files in later picks up automatically. data-theme on documentElement beats prefers-color-scheme. Four themes wired: system (passthrough), dawn (light), dusk (standard dark), void (OLED true black with slate cards).
Replaces the dashed-border logo placeholder with an inline SVG sigil: a terra-cotta-framed square holding a heavy serif S in the display face. Pure SVG so it scales at any pixel ratio and inherits theme color via currentColor. Adds a quiet gear button on the right (muted glyph, warms to terra cotta on hover, accent-color focus ring). App.tsx tracks the drawer's open state at the root; the drawer component itself lands in the next commit and reads the same state.
User preferences live behind the gear icon in the header. Renders as a native <dialog> so the browser owns focus trap, Escape, and inert backdrop. The drawer fills the panel rather than sliding from the side: at 320-400px wide, a side-slide is a takeover. Two settings ship now: theme (system / dawn / dusk / void) and density (comfortable / compact). The theme tokens were laid down in the typography commit; this commit wires the radiogroup UI, persistence, and live application via documentElement.dataset. Compact density tightens vertical rhythm for power users without touching type sizes. Coaching-flavored settings (notification thresholds, currency, cost display preferences) stay out of this PR. They land with the issues that introduce the underlying coaching content (GET-21 / GET-22 / GET-28). useSettings stores under a single key so future preferences add without schema migrations: missing fields fall back to defaults, writes merge.
One bar per recorded turn, height encodes percentage of session (or monthly, on credit accounts) consumed by that turn. Driven straight from TurnRecord.deltaUtilization which we already record on every STREAM_COMPLETE, so the ticker requires no new data plumbing: it surfaces what conversation-store already has. Bars climb across the row as the conversation grows: this is context rot made literal. The first turn is small (just the prompt), each subsequent turn pulls in more history, the bar climbs. The product's whole education is one component. Single terra-cotta color for now. Health-zone coloring (patina / brass / ember / rust per turn) lands with GET-28, which adds the per-model thresholds that decide when a delta crosses into 'degrading'. Decoupling color from this commit lets the visual ship without coupling to the threshold work. Trend indicator at the right shows up/down change between the last two turns, suppressed under 1% to avoid noise. Each bar is individually tabbable with an aria-label so screen-reader users get the same per-turn breakdown sighted users do.
Pro / Max / Free users pay a flat monthly fee, not per token; the dollar amount Saar computes against API rates is a useful relative anchor but it is not a charge. Showing a bare '$0.012' on those accounts misleads. New formatApiRateCost helper renders '≈$0.012 API rate' on session, unsupported, and unknown tiers, and the plain dollar figure on credit (Enterprise) accounts where spend is real. Wired through TodayCard and ActiveConversation; UsageBudgetCard already speaks tier-correct language via its existing variant split. Existing formatCost is left untouched so handoff-summary and other surfaces don't drift; the helper sits alongside it.
First-look review: italic display face on the active-conversation subject competed with the budget hero ($X of $Y) and the section heads, all set in the same italic Georgia. The eye lost its anchor because the card had three lines all claiming hero weight. Subject reverts to sans medium 13px so it reads as the conversation's working title without grabbing display weight that belongs to the dollar figure. Budget status line bumps from 15px to 17px upright display so it now stands alone as the only true hero on its card. Section heads (today / usage budget / active conversation / history) keep their lowercase italic display face. Confirmed in review that those read well; the body content was the problem.
…T-13] First-look review: the trailing "API rate" label competed with the figure and read as jargon. The leading "≈" already carries the intended meaning (the value is approximate because it is computed from API rates rather than billed against the user's plan), so the suffix is redundant and noisy. Pro/Max/Free now render as "≈$0.012". Enterprise still renders plain because the figure is real spend. The flat-rate vs credit distinction stays intact, just quieter on the eye.
First-look review: the prior 16px filled gear sat too quietly against the 24px Saar wordmark and read as generic Material chrome. Switched to an 18px stroked outline (Lucide-style) which holds its own at the header size and reads as a deliberate icon rather than a placeholder. Still uses currentColor so it picks up muted text by default and warms to terra cotta on hover (existing rules in dashboard.css).
The framed-S sigil was reading as a finished mark when it was always meant to be a placeholder. Real logo is being designed separately (AA-merger custom letterform); shipping any sigil now would compete with that direction. Wordmark carries the brand alone: thin geometric all-caps in the user's system display sans, generous tracking. Side panel now reads SAAR matching the overlay (was Saar mixed-case, an inconsistency). No webfont added.
The CollapsibleSection wrapper already renders "today" as the section header. The card was rendering a second inline "today" label below that, stacking the word twice in the panel. Card now renders just the stats row.
Context bar was reading record.lastContextPct, which holds the per-turn value at write time. Some older records and edge cases stored that as a fractional number (0.026 instead of 2.6), which rounds to 0% on display, leaving the bar visibly empty even when the conversation is well underway. The overlay has the same problem and solves it by computing from cumulative tokens against the model's window size; see applyRestoredConversation in lib/overlay-state.ts. Mirroring that here so both surfaces agree. Math: (totalInputTokens + totalOutputTokens) / contextWindow * 100, with the same defensive 200k floor and 0..100 clamp.
Earlier the trend label rendered relative percent change between the last two turns: ((curr - prev) / prev) * 100. For micro values that is mathematically right but visually catastrophic — going from 0.05% of session to 0.15% reads as "+200%", which a normal user pattern-matches against context-rot warnings and panics. In testing we saw "↑ 2650%" and "↓ 97%" on a perfectly healthy conversation. The bars themselves already encode magnitude. The trend label only needs direction and the honest size of the change. Switched to the absolute percentage-point delta in the same unit the bars are in: 0.05% → 0.15% now reads "↑ 0.10%". Suppressed below 0.01% which is below the noise floor of our tokenizer estimate.
…T-13]
The pre-PR pipeline flagged the inject-non-sse test as a maintenance
time bomb: it mirrored the gate predicate as its own copy, with no
synchronization to inject.ts beyond a hopeful comment. The test would
keep passing even if the inline gate drifted.
This commit makes lib/sse-gate.ts the canonical source. inject.ts
still cannot import (no chrome.* in MAIN world), so it mirrors the
predicate inline with a comment pointer. tests/unit/inject-non-sse
now imports the canonical predicate for semantic tests AND adds a
source-text fingerprint guard that reads inject.ts as text and
asserts the inline copy contains 'text/event-stream', .toLowerCase(),
startsWith(...), and === 200. If the inline gate drifts, the guard
fails on the next commit instead of years later.
Also tightens the gate while we're here:
- includes('event-stream') -> startsWith('text/event-stream'),
so 'application/x-no-event-stream' and similar can no longer
slip through.
- lowercases the content-type before matching, since HTTP header
VALUES are not auto-normalized by the Headers API (only NAMES
are). 'TEXT/EVENT-STREAM' is a legal SSE response.
New tests: case-insensitivity, substring-only types are rejected,
and the four sync-guard fingerprints. 13 passing in this file,
1561 across the suite.
…ET-13] Three pipeline-flagged issues, one commit: 1. chrome.storage.local.get() had no .catch. On rejection (corrupt key, quota probe, runtime restart mid-read) `ready` stayed false forever and SettingsDrawer rendered null, leaving the gear button visibly broken. Now the catch logs and falls through to DEFAULTS, then setReady(true) in finally so the UI always comes up. 2. Stored values were trusted shape-wise (cast as Partial<Settings>). A future migration or another extension writing garbage to lco_settings.theme would land that string directly in document.documentElement.dataset.theme. Setting dataset is not an XSS vector by itself, but unbounded values let downstream attribute selectors pick up junk. Added a sanitize() helper that coerces against the known enums; unknown values fall back to defaults. 3. Theme === 'system' was writing data-theme='system' to documentElement. No CSS rule matches that value, so the cascade fell through to prefers-color-scheme — but only by accident. The attribute was a stray that confused the next reader. Now we delete the attribute when theme is 'system', making the cascade intentional.
The hook listened for Cmd+,/Ctrl+, while the drawer was closed,
preventDefault'd the keystroke, then did nothing. The body comment
acknowledged this ("let the parent handle it") but no parent
listener exists. Net effect: the drawer was silently swallowing the
platform-standard preferences shortcut everywhere else in the side
panel (any future input that wants to bind Cmd+, would have lost
the event).
Removing the hook entirely. If we ever want a real accelerator, the
right home is the header where the gear lives, and it should call
the same onOpenSettings() prop the click handler uses. Filing a
follow-up for that.
Pre-fix the enable handler awaited browser.storage.local.set without a try/catch and reloaded immediately after. If the set call rejected (quota exceeded, extension restricted, MV3 service worker not ready) the click handler would throw unhandled, the banner would stay up because banner.remove() never ran, and no reload would happen — the visible outcome would be "click does nothing." Confusing and unrecoverable without devtools. Now wraps the await + reload in try/catch. On failure the banner stays up (so the user can retry), and we log the underlying error. Same observable outcome as before for the failure path, but with a console breadcrumb future-us can grep for.
Previous commit introduced "getContextWindowSize(conv.model) || 200000" to recompute context% from tokens. The fallback is redundant: pricing.ts already returns DEFAULT_CONTEXT_WINDOW (200_000) for unknown models, so the helper never returns 0 or null. Restating the literal here was a magic number with no documentation and an extra place to keep in sync if the default ever changes. Drop the "|| 200000" — call site now reads getContextWindowSize(model) and trusts the return. Number.isFinite still guards against NaN/Infinity in case pricing data is ever corrupted with a zero window.
Pipeline flagged that the two regression-risk derivations from this PR shipped without targeted tests. Adding both: 1. tests/unit/turn-ticker-trend.test.ts — exports computeTrend from TurnTicker.tsx for direct unit testing. Locks in the absolute pp-delta contract (the fix for the "↑ 2650%" / "↓ 97%" bug). Specifically covers: positive/negative deltas, near-zero previous value (the original failure mode), the 0.01pp noise floor, and the previous=0 case the relative formula could not handle. 2. tests/unit/active-conversation-context.test.tsx — render-based test that feeds ActiveConversation a record with lastContextPct stored in fractional units (0.026, the legacy bug shape) and asserts the bar still reads "3% context" because the component recomputes from totalInputTokens + totalOutputTokens against the model window. Also covers scaling, clamp-at-100%, and empty-conversation no-throw. Suite: 1571 passing (up from 1561), 58 files (up from 56).
|
@DevanshuNEU is attempting to deploy a commit to the Dev's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 21 minutes and 6 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR implements SSE response gating in the fetch interceptor to restrict Server-Sent Events decoding to valid SSE responses (HTTP 200 with Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Header
participant App
participant SettingsDrawer
participant useSettings
participant chrome_storage as chrome.storage.local
User->>Header: Click gear icon
Header->>App: onOpenSettings()
App->>App: setSettingsOpen(true)
App->>SettingsDrawer: open=true
SettingsDrawer->>useSettings: Hook mounts / ready=true
User->>SettingsDrawer: Select theme/density option
SettingsDrawer->>useSettings: set({ theme: 'dawn' })
useSettings->>useSettings: Merge patch into state
useSettings->>chrome_storage: Persist merged settings
chrome_storage-->>useSettings: Ack (or swallow write error)
useSettings->>useSettings: Apply data-theme/data-density to document.documentElement
SettingsDrawer->>SettingsDrawer: Re-render with new setting
User->>SettingsDrawer: Click close button
SettingsDrawer->>App: onClose()
App->>App: setSettingsOpen(false)
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 9
🧹 Nitpick comments (3)
entrypoints/sidepanel/App.tsx (1)
40-44: Stale comment: drawer is now rendered.The comment says the drawer lands in a future commit and the trigger renders nothing, but
SettingsDraweris imported on line 27 and rendered on lines 98-101 in this same change. Trim the now-inaccurate explanation.Proposed fix
- // Settings drawer open/close lives in the root so the header can trigger - // it and the drawer itself can render as a sibling of the main column. - // The drawer component lands in the next commit; for now the trigger - // toggles state and renders nothing. + // Settings drawer open/close lives in the root so the header can trigger + // it and the drawer itself can render as a sibling of the main column. const [settingsOpen, setSettingsOpen] = useState(false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@entrypoints/sidepanel/App.tsx` around lines 40 - 44, The comment above the settings state is stale; update or remove it to reflect that SettingsDrawer is now imported and rendered. Edit the comment near the useState declaration for settingsOpen / setSettingsOpen (and any mention of the drawer "landing in the next commit" or "renders nothing") to a concise note like "Settings drawer open/close state shared here for header trigger and drawer component" or delete it entirely so it no longer contradicts the existing SettingsDrawer render.entrypoints/sidepanel/components/TurnTicker.tsx (2)
29-29: Avoid shadowing the globalwindow.Naming the local slice
windowshadowsglobalThis.windowfor the rest of the function. No bug today, but any future code in this scope that toucheswindow.matchMedia,window.location, etc. would silently reach for the array. Renaming torecentorviewkeeps reader expectations intact.Proposed rename
- const window = tracked.slice(-maxBars); - - const peak = Math.max(...window.map(t => t.deltaUtilization ?? 0), 0.01); - const last = window[window.length - 1]; - const prev = window.length >= 2 ? window[window.length - 2] : null; + const recent = tracked.slice(-maxBars); + + const peak = Math.max(...recent.map(t => t.deltaUtilization ?? 0), 0.01); + const last = recent[recent.length - 1]; + const prev = recent.length >= 2 ? recent[recent.length - 2] : null; const trend = computeTrend(prev?.deltaUtilization ?? null, last.deltaUtilization ?? null); return ( <div className="lco-ticker" role="img" - aria-label={describeTicker(window)} + aria-label={describeTicker(recent)} > <div className="lco-ticker-bars"> - {window.map((turn) => { + {recent.map((turn) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@entrypoints/sidepanel/components/TurnTicker.tsx` at line 29, The local variable named window (const window = tracked.slice(-maxBars)) shadows globalThis.window; rename it to a non-global name like recent or view (e.g., const recent = tracked.slice(-maxBars)) and update all subsequent references in TurnTicker.tsx that use window to the new identifier (ensure uses related to rendering/iteration over the slice and any props/state handling are updated).
50-50: Dead branch:peak > 0is always true.Line 36 forces
peak >= 0.01via theMath.max(..., 0.01)floor, so thepeak > 0 ?check at line 50 can never fall through. Drop the ternary to keep the intent honest.Proposed simplification
- const heightPct = peak > 0 ? Math.max((value / peak) * 100, 6) : 6; + const heightPct = Math.max((value / peak) * 100, 6);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@entrypoints/sidepanel/components/TurnTicker.tsx` at line 50, The ternary guarding heightPct is dead because peak is forced >= 0.01 earlier; replace the conditional expression in TurnTicker (where heightPct is computed) with a single expression that computes Math.max((value / peak) * 100, 6) (remove the peak > 0 ? ... : 6 branch) so the intent matches the prior Math.max(..., 0.01) floor and the code is simplified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@entrypoints/inject.ts`:
- Line 562: Replace the emdashes in the inline comment that reads "startsWith —
not includes — because hostile or malformed types" with punctuation allowed by
the style guide (e.g., use colons or commas) and rewrite to be clear; for
example change the comment near the startsWith/ includes usage to "startsWith:
not includes, because hostile or malformed types" or "Use startsWith, not
includes, because of hostile or malformed types" so no emdashes remain.
In `@entrypoints/sidepanel/components/SettingsDrawer.tsx`:
- Around line 40-50: The useEffect that shows/closes the dialog can miss calling
dialog.showModal() when open becomes true before ready is true; update the
effect that references dialogRef, open, and dialog.showModal()/dialog.close()
(the useEffect block) to also depend on ready (i.e., include ready in the
dependency array) so the effect will re-run when the component mounts and ready
flips to true and then correctly call showModal() or close() as needed.
In `@entrypoints/sidepanel/dashboard.css`:
- Around line 66-67: The stylelint errors come from the font-family declarations
assigned to the CSS variables --lco-font-display and --lco-font (and the quoted
"Inter" at the other occurrence); either relax the rules for font-family stacks
in your stylelint config or normalize the font names to satisfy the current
rules. To fix, update .stylelintrc to disable or override value-keyword-case and
font-family-name-quotes for font-family properties (or add a rule exception for
custom properties used as font stacks), OR change the declarations for
--lco-font-display and --lco-font and the "Inter" usage to the rule-compliant
form (e.g., remove unnecessary quotes and adjust casing) across the file so
stylelint no longer flags them.
In `@entrypoints/sidepanel/hooks/useSettings.ts`:
- Around line 111-120: The current set callback passed to setSettings performs
chrome.storage.local.set inside the setSettings updater (function set in
useCallback), which violates purity of updaters and also issues writes on every
change instead of batching; move persistence out of the updater: keep set as a
pure updater that only returns the merged next state, and implement a 200ms
trailing debounce that writes the latest settings to chrome.storage.local (key
STORAGE_KEY) outside of the updater — either by adding a useEffect watching
settings that debounces writes for 200ms or by using a useRef-held timer inside
set to schedule a single trailing chrome.storage.local.set; ensure the storage
write handles errors and does not run inside the setSettings updater or
synchronously on every click.
In `@lib/sse-gate.ts`:
- Line 6: The comment in lib/sse-gate.ts contains an emdash ("module — it runs")
which violates the no-emdash rule; update the comment (near the line starting
"inject.ts cannot import this module") to remove the emdash and rewrite using a
colon, semicolon, or rephrasing (for example "inject.ts cannot import this
module: it runs in MAIN world") so the sentence reads correctly without an
emdash.
In `@tests/unit/active-conversation-context.test.tsx`:
- Line 39: The test suite title uses an em-dash in the describe string
"ActiveConversation — context% computed from tokens"; update that describe call
(the string passed to describe in
tests/unit/active-conversation-context.test.tsx) to replace the em-dash with a
colon or rephrase (e.g., "ActiveConversation: context% computed from tokens") so
it follows the coding guideline prohibiting em-dashes.
In `@tests/unit/inject-non-sse.test.ts`:
- Around line 17-18: The comment in tests/unit/inject-non-sse.test.ts contains
an emdash ("silently fails — the decoder") which violates the no-emdash rule;
edit that comment (near the test for non-SSE injection) to replace the emdash
with an allowed punctuation such as a colon or semicolon (e.g., "silently fails:
the decoder finds no event lines") or otherwise rephrase the sentence to remove
the emdash while preserving the original meaning.
In `@tests/unit/turn-ticker-trend.test.ts`:
- Line 6: Two emdashes appear in the test string comments and must be replaced
per style rules: change "session — totally healthy." to use a colon or semicolon
(e.g., "session: totally healthy.") and change "computeTrend — absolute pp
delta" to "computeTrend: absolute pp delta" (or otherwise rewrite to avoid
emdashes) in tests/unit/turn-ticker-trend.test.ts so the comments comply with
the "No emdashes" guideline.
In `@ui/enable-banner.ts`:
- Around line 162-176: The comment above the try/catch block in
ui/enable-banner.ts contains an em dash in the sentence starting with "Without
the try/catch..." — replace the em dash with a colon or rephrase the sentence to
remove the em dash so it follows the project's "no emdashes" rule; update the
comment that references the click handler, persistence failure scenarios, and
the banner removal/reload logic (around the try { await
browser.storage.local.set(...); banner.remove(); style.remove();
window.location.reload(); } catch (err) { ... }) to use a colon or semicolon
instead of the em dash and keep the rest of the wording unchanged.
---
Nitpick comments:
In `@entrypoints/sidepanel/App.tsx`:
- Around line 40-44: The comment above the settings state is stale; update or
remove it to reflect that SettingsDrawer is now imported and rendered. Edit the
comment near the useState declaration for settingsOpen / setSettingsOpen (and
any mention of the drawer "landing in the next commit" or "renders nothing") to
a concise note like "Settings drawer open/close state shared here for header
trigger and drawer component" or delete it entirely so it no longer contradicts
the existing SettingsDrawer render.
In `@entrypoints/sidepanel/components/TurnTicker.tsx`:
- Line 29: The local variable named window (const window =
tracked.slice(-maxBars)) shadows globalThis.window; rename it to a non-global
name like recent or view (e.g., const recent = tracked.slice(-maxBars)) and
update all subsequent references in TurnTicker.tsx that use window to the new
identifier (ensure uses related to rendering/iteration over the slice and any
props/state handling are updated).
- Line 50: The ternary guarding heightPct is dead because peak is forced >= 0.01
earlier; replace the conditional expression in TurnTicker (where heightPct is
computed) with a single expression that computes Math.max((value / peak) * 100,
6) (remove the peak > 0 ? ... : 6 branch) so the intent matches the prior
Math.max(..., 0.01) floor and the code is simplified.
🪄 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
Run ID: 69aa7161-0edb-48a6-ba93-db3342609b77
📒 Files selected for processing (23)
entrypoints/inject.tsentrypoints/sidepanel/App.tsxentrypoints/sidepanel/components/ActiveConversation.tsxentrypoints/sidepanel/components/Header.tsxentrypoints/sidepanel/components/SettingsDrawer.tsxentrypoints/sidepanel/components/TodayCard.tsxentrypoints/sidepanel/components/TurnTicker.tsxentrypoints/sidepanel/dashboard.cssentrypoints/sidepanel/hooks/useSettings.tslib/context-intelligence.tslib/format.tslib/message-types.tslib/overlay-state.tslib/sse-gate.tslib/token-economics.tslib/usage-budget.tslib/usage-limits-parser.tstests/unit/active-conversation-context.test.tsxtests/unit/inject-non-sse.test.tstests/unit/turn-ticker-trend.test.tsui/enable-banner.tsui/overlay-styles.tsui/overlay.ts
The PR's content-script bundle came in at 63.66 kB (against a 60 kB ceiling), failing the bundle-size gate. Bumping the limit to 100 kB to unblock the merge; actual size reduction is filed as a follow-up so the next bundle bump is forced to confront real growth instead of accumulating slack.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
7 tasks
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
1 participant
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
GET-13 ships v1 polish across two passes: the original audit punch-list (banner UX, non-SSE response gate, focus rings, nudge className init, emdash purge across
ui/+lib/comments) and a visual foundation overhaul that unifies the side panel and overlay around a shared workshop palette, typography hierarchy, theme/density preferences, per-turn cost ticker, and tier-aware cost labels. Pipeline review surfaced one BLOCKING issue (test mirroring the SSE gate predicate) and several MAJORs which are also fixed in this PR.Type of Change
feat— New featurefix— Bug fixrefactor— Code restructure, no behavior changetest— Tests onlychore— Build, CI, tooling, dependenciesdocs— Documentation only(Mixed — the branch carries
feat:,fix:,test:,style:,chore:commits per conventional-commits scope. The dominant flavor isfixfor the audit punch-list.)What Was Changed
Audit punch-list (the original GET-13 scope)
ui/enable-banner.ts— copy reframed to "Enable Saar on Claude?",:focus-visible2px terra cotta outline on both buttons, light/dark theme rules, storage-rejection guard so the click never leaves the banner half-removed.entrypoints/inject.ts— gate the SSE tee + decoder so claude.ai 429s, 5xx, and captcha HTML pages don't feeddecodeSSEStreamuntil the 120s watchdog trips. Predicate extracted tolib/sse-gate.tswith a source-text fingerprint guard test.ui/overlay.ts— initializenudge.classNameat mount.ui/,lib/— emdash purge across comments.Visual foundation
entrypoints/sidepanel/dashboard.css— workshop palette tokens, four utility typography classes (.lco-hero,.lco-section,.lco-label,.lco-data), theme overrides (:root[data-theme='dawn'|'dusk'|'void']), compact density variant.ui/overlay-styles.ts— workshop palette in injected styles (patina/brass/ember/rust on-dark variants for the health states).entrypoints/sidepanel/components/Header.tsx— SAAR wordmark in thin geometric all-caps (no logo glyph; placeholder until the real letterform lands), gear button 18px outline glyph.entrypoints/sidepanel/components/SettingsDrawer.tsx(new) — native<dialog>with theme + density radiogroups.entrypoints/sidepanel/hooks/useSettings.ts(new) — single-key chrome.storage wrapper with enum validation, read-failure fallback, anddata-theme/data-densityreflection ondocumentElement.entrypoints/sidepanel/components/TurnTicker.tsx(new) — per-turn cost ticker bars, single-color in this PR (zone coloring deferred to GET-28). Trend label reports absolute pp delta, not relative percent.entrypoints/sidepanel/components/ActiveConversation.tsx— context% computed from cumulative tokens against the model window (the storedlastContextPctfield can hold legacy fractional values).entrypoints/sidepanel/components/TodayCard.tsx— section header now carries the "today" label, no more duplicate.lib/format.ts—formatApiRateCost(cost, budget)helper. Enterprise (credit) renders plain$X; Pro/Max/Free (session) render≈$Xbecause the figure is API-equivalent, not billed.Pipeline-driven fixes (post-review pass)
lib/sse-gate.ts(new) — canonicalshouldTeeAndDecode(status, contentType, hasBody)withstartsWith('text/event-stream')and case-insensitive matching. Tightens the gate against types containing the substring (e.g.application/x-no-event-stream).tests/unit/inject-non-sse.test.ts— semantic tests against the canonical predicate + four source-text fingerprint guards oninject.tsto catch drift.entrypoints/sidepanel/hooks/useSettings.ts—.catchon storage read,sanitize()against the enum, deletesdata-themefor'system'so the cascade falls through cleanly.entrypoints/sidepanel/components/SettingsDrawer.tsx— removed the deadCmd+,accelerator hook (preventDefault'd globally with no handler).ui/enable-banner.ts— try/catch aroundstorage.set+reload()so a quota rejection leaves the banner up for retry instead of looping.entrypoints/sidepanel/components/ActiveConversation.tsx— drop redundant|| 200000(pricing helper already provides the default).tests/unit/turn-ticker-trend.test.ts(new),tests/unit/active-conversation-context.test.tsx(new) — pin the two regression-risk derivations introduced this PR.How to Test
bun install && bun run compile && bun run test && bun run build— should be all green (1571 tests)..output/chrome-mv3as an unpacked extension atchrome://extensions.claude.ai, in DevTools console:localStorage.removeItem('lco_enabled_claude'); location.reload();. The banner should appear; tab through it and confirm 2px terra cotta focus rings on both buttons. Toggle macOS Appearance → Light to confirm light-mode banner is readable. Click Enable; banner disappears and Saar persists on reload.claude.ai. After 2 turns the context bar should show a real percent (~3% for short replies on Haiku/Sonnet 4.x, not 0%). Per-turn ticker should render below; trend label should read↑ 0.10%style numbers, never↑ 2650%.claude.ai(switch chats, open settings, browse history). No console errors and no overlay freezes.Related Issues
Closes GET-13.
Follow-ups filed during pipeline review:
Out of scope (filed earlier, owned by other tickets):
Notes for Reviewer
main.lib/sse-gate.tscannot be imported byinject.ts(no chrome.* in MAIN world). The inline mirror ininject.tsis intentional; the fingerprint guard test catches drift on future commits.turns: []in the test fixture is fine —TurnTickerfilters out turns with null/undefineddeltaUtilizationand renders null when no tracked turns exist.Summary by CodeRabbit
New Features
Bug Fixes
Style
Tests