Conversation
GitHub Actions
Failure
|
Warning Review limit reached
Next review available in: 28 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
WalkthroughThe Usage page now provides configurable sharing for filtered usage charts. It derives share-card data, renders square or landscape cards, previews and exports PNG images, supports clipboard copying, persists presentation settings, and validates the workflow through unit, page, and end-to-end tests. ChangesUsage chart sharing
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
khaledosman
left a comment
khaledosman
left a comment
There was a problem hiding this comment.
Verified at 95fbf95 in a worktree at PR head: npm run typecheck clean, 558 unit tests pass, and the committed bundle carries both the new chunk and the docs/dashboard.md text. Four inline findings, none blocking.
One question outside the diff lines: a root TODOS.md duplicates issue tracking and pins line numbers (usage.py:1320) that will drift.
🤖 Review generated with Claude Code
| cancelled = true; | ||
| clearTimeout(timer); | ||
| }; | ||
| }, [presentation, hero, secondary, shown, scope]); |
There was a problem hiding this comment.
The preview effect never settles: it re-rasterizes the card every 300ms for as long as the dialog is open.
secondary (L95) and shown (L96) are fresh arrays on every render, so this dep array always differs. The effect's own setPreview (a new blob: URL each time) re-renders, which re-arms the 300ms timer, which rasterizes again. Confirmed against this head with a throwaway test: 10 rasterize calls in ~4s of idle with no user input. Each cycle is a 2160x2160 canvas draw plus a PNG encode, and the <img src> swaps every time.
Fix: useMemo secondary and shown, or key the effect on presentation plus primitives derived from the data.
| <Button | ||
| variant="primary" | ||
| isDisabled={busy || blocked} | ||
| onPress={() => withBlob(async (blob) => { await copyBlobAsImage(blob); }, "Image copied")} |
There was a problem hiding this comment.
"Image copied" is shown even when the copy failed. copyBlobAsImage swallows its own error and returns false, and withBlob ignores the return value before setting the notice.
onPress={() => withBlob(async (blob) => {
if (!(await copyBlobAsImage(blob))) {
throw new Error("The image could not be copied to the clipboard.");
}
}, "Image copied")}| } | ||
| // Merged over the defaults rather than trusted: a stored shape from an older | ||
| // build (or a hand-edited one) must not be able to crash the panel. | ||
| return { ...DEFAULTS, ...(parsed as Partial<Presentation>) }; |
There was a problem hiding this comment.
This merge does not deliver what the comment above it claims: the spread copies unvalidated values straight through. A stored ratio outside CARD_SIZES (older build, hand-edited storage) crashes the panel, reproduced here with {"ratio":"portrait"} in localStorage:
TypeError: Cannot destructure property 'width' of 'CARD_SIZES[ratio]' as it is undefined.
The existing "survives a corrupt stored presentation" test only covers unparseable JSON, so this path is uncovered. Validate each field against its known set (ratio, theme, hero, rows in {1,3,5,9}) and fall back per field.
| <div style={{ fontSize: heroSize, fontWeight: 700, lineHeight: 0.82 }}>{hero.value}</div> | ||
| <div style={{ fontSize: 44, fontWeight: 500, lineHeight: 1.1, color: palette.muted, marginTop: 8 }}> | ||
| {hero.label} | ||
| {hero.caveated ? "*" : ""} |
There was a problem hiding this comment.
The caveat asterisk has no legend on the image. Spend* (and the same mark at L215) publishes a symbol a viewer of a standalone PNG cannot resolve; the explanation lives only in docs/dashboard.md, which does not travel with the file.
Pass the unpriced count in and print * N requests unpriced alongside scope (L235) whenever a shown stat is caveated.
github-actions
Bot
added
the
missing-template
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (11)
web/src/components/ShareDialog.test.tsx (2)
116-120: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a case for a stored presentation that parses but holds an invalid value.
This test covers unparseable JSON only. The crash path flagged in
ShareDialog.tsxLines 34-50 needs valid JSON with an unknownratio. Adding that case pins the fix and prevents a regression.💚 Suggested extra case
it("survives a corrupt stored presentation instead of crashing", () => { localStorage.setItem("otari.share.presentation.v1", "{ not json"); renderDialog(); expect(screen.getByRole("button", { name: "Download PNG" })).toBeInTheDocument(); }); + + it("falls back per field when a stored value is outside its known set", () => { + localStorage.setItem( + "otari.share.presentation.v1", + JSON.stringify({ ratio: "portrait", theme: "neon", rows: 42 }), + ); + renderDialog(); + expect(screen.getByRole("button", { name: "Download PNG" })).toBeInTheDocument(); + });🤖 Prompt for 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. In `@web/src/components/ShareDialog.test.tsx` around lines 116 - 120, Add a test alongside “survives a corrupt stored presentation instead of crashing” that stores valid JSON with an unknown ratio value, renders via renderDialog(), and verifies the dialog still renders the Download PNG button. This should exercise the invalid parsed presentation path in ShareDialog.
54-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMost assertions here prove absence rather than behavior.
Six of the nine tests assert that something is not present. Those guard against past drafts, which is useful, but they pass even if the dialog renders almost nothing. Two positive cases would balance the suite: one that changes a control and asserts the resulting
ShareCardcontent, and one that asserts the failure notice when the copy action rejects. The second case also covers the clipboard fix requested inShareDialog.tsxLines 310-318.🤖 Prompt for 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. In `@web/src/components/ShareDialog.test.tsx` around lines 54 - 93, Add two behavior-focused tests to the ShareDialog suite: interact with a share control and assert that the resulting ShareCard content changes, then mock or trigger a rejected copy action and assert the dialog displays its failure notice. Reuse the existing renderDialog setup and visible control/error symbols from ShareDialog.tsx, while retaining the current absence assertions.web/src/components/ShareCard.tsx (1)
122-129: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSmall note on the row budget: it can be exceeded at nine rows.
At
rowCount = 9and square shape,rowGapis 8 and the computedrowHeightclamps to the 34 floor. The rendered list then needs9 * 34 + 8 * 8 = 370px, which is 30px above the 340rowsBudget. The floor wins over the budget by design, so the surplus has to come out of the flexible middle block. The end-to-end test asserts no overflow today, so this is not currently a defect; it is worth a comment stating that the floor deliberately overrides the budget, so a future tweak toheroSizeor padding does not reintroduce the collapsed-title bug.🤖 Prompt for 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. In `@web/src/components/ShareCard.tsx` around lines 122 - 129, Add a concise comment near the rowHeight calculation explaining that at very high row counts the minimum 34px row height may exceed rowsBudget intentionally, with the flexible middle block absorbing the surplus; preserve the existing heroSize and sizing logic.web/src/components/ShareDialog.tsx (2)
72-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo small hygiene points on state typing and the notice timer.
First,
preview,error, andnoticemodel absent values asnull. The guidelines ask forundefinedin our own TypeScript types. This is cosmetic today, so treat it as cleanup rather than a blocker.Second, the
setTimeoutat Line 164 is never cleared. If the dialog closes within the 2-second window, the callback still runs against an unmounted component. Storing the handle in a ref and clearing it in the existing unmount effect at Lines 137-147 removes the loose end.As per coding guidelines: "Use
undefinedrather thannullfor absent values in own TypeScript types".Also applies to: 163-164
🤖 Prompt for 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. In `@web/src/components/ShareDialog.tsx` around lines 72 - 75, Update ShareDialog’s preview, error, and notice state to use undefined rather than null for absent values, including corresponding initializers and checks. Store the setTimeout handle used by the notice flow in a ref, and clear it in the existing unmount effect so the callback cannot run after the dialog closes.Source: Coding guidelines
185-193: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider reusing the shared status banner components.
These inline error and warning blocks repeat the styling that
ErrorBannerandInfoBanneralready own. Reusing them keeps status surfaces consistent, which is exactly the case the guidelines carve out for raw Tailwind palette classes.As per coding guidelines: "Raw Tailwind palette classes are permitted only for status surfaces such as
ErrorBannerandInfoBanner."#!/bin/bash # Locate the shared banner components and their prop surface. fd -i 'banner' web/src --extension tsx --extension ts rg -nP --type=tsx -C3 'export function (Error|Info)Banner' web/src🤖 Prompt for 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. In `@web/src/components/ShareDialog.tsx` around lines 185 - 193, Replace the inline error and stale-warning divs in ShareDialog with the shared ErrorBanner and InfoBanner components, passing the existing error text and stale-state message through their supported props. Preserve the current conditional rendering and messages while removing the duplicated Tailwind styling.Source: Coding guidelines
web/src/components/ShareCard.test.tsx (2)
107-118: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThis height test cannot catch the collapsed-band regression it describes.
Line 112 filters heights with
h > 30before the assertion. A row that collapses to 0 is removed by the filter, so the loop at Lines 114-116 passes over an empty or partial list. The comment above the test says the regression is a band collapsing, so the filter removes exactly the failure case.Assert the row count instead, then assert each measured row height.
💚 Suggested tightening
- const rows = container.querySelectorAll<HTMLElement>('[style*="height"]'); - const heights = Array.from(rows) - .map((el) => Number.parseFloat(el.style.height)) - .filter((h) => !Number.isNaN(h) && h > 30); - // Every row keeps at least the 28px name plus breathing room. - for (const h of heights) { - expect(h).toBeGreaterThanOrEqual(34); - } + const heights = Array.from(container.querySelectorAll<HTMLElement>('[style*="height"]')) + .map((el) => Number.parseFloat(el.style.height)) + .filter((h) => !Number.isNaN(h)); + // One measured height per model row, and every row keeps at least the + // 28px name plus breathing room. + expect(heights.filter((h) => h >= 34)).toHaveLength(n); + expect(heights.some((h) => h === 0)).toBe(false);🤖 Prompt for 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. In `@web/src/components/ShareCard.test.tsx` around lines 107 - 118, Update the height assertions in the “keeps rows legible and the hero present” test to first verify the expected number of measured rows, without filtering out collapsed heights, then assert every row height meets the minimum threshold. Preserve the existing hero assertion and use the rendered model count to establish the expected row count.
62-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a user-facing query over
getByTestIdhere.Lines 64 and 76 use
screen.getByTestId("share-card"). The guidelines ask forgetByRole,getByLabelText, orgetByText. The card root is a plaindiv, so one practical option is to give it an accessible role and name inShareCard.tsx, for examplerole="img"with anaria-label, then query it withgetByRole("img", { name: ... }). That also improves the preview surface for assistive technology.As per coding guidelines: "query the UI as a user would with
getByRole,getByLabelText, orgetByTextinstead ofgetByTestId".🤖 Prompt for 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. In `@web/src/components/ShareCard.test.tsx` around lines 62 - 77, Replace the getByTestId("share-card") queries in renders both ratios at their exact pixel sizes with an accessible user-facing query. Update ShareCard’s root element to expose a stable role and accessible name, then use screen.getByRole with that name in both assertions while preserving the existing size checks.Source: Coding guidelines
web/src/lib/shareImage.ts (2)
69-76: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout to the image load so the dialog cannot hang.
The promise settles only when
onloadoronerrorfires. If the browser fires neither, for example on a very large serialized document or an unusual decode failure, the promise never settles.ShareDialog.withBlobthen leavesbusyattrueand both action buttons stay disabled with no error message. A bounded wait converts that into a normal error path.⏱️ Suggested guard
const image = new Image(); image.width = width; image.height = height; await new Promise<void>((resolve, reject) => { - image.onload = () => resolve(); - image.onerror = () => reject(new Error("The share card could not be rendered to an image.")); + const timer = setTimeout(() => { + image.src = ""; + reject(new Error("The share card took too long to render to an image.")); + }, 15_000); + image.onload = () => { + clearTimeout(timer); + resolve(); + }; + image.onerror = () => { + clearTimeout(timer); + reject(new Error("The share card could not be rendered to an image.")); + }; image.src = svgUrl; });🤖 Prompt for 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. In `@web/src/lib/shareImage.ts` around lines 69 - 76, Add a bounded timeout to the image-loading Promise in shareImage so it rejects when neither onload nor onerror fires, allowing ShareDialog.withBlob to clear busy state and report the failure. Ensure the timeout is cancelled when the image settles normally and preserve the existing load/error behavior.
121-123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe comment and the delay disagree.
The comment says "Revoked on the next tick", but the timeout is 10 seconds. The 10-second delay is the right choice for the described race; only the wording needs a small correction.
📝 Suggested wording
- // Revoked on the next tick: revoking synchronously can race the download in - // some browsers, which then saves a zero-byte file. + // Revoked well after the click: revoking synchronously can race the download + // in some browsers, which then saves a zero-byte file.🤖 Prompt for 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. In `@web/src/lib/shareImage.ts` around lines 121 - 123, Update the comment above the setTimeout in the share image download flow to describe revocation after the 10-second delay rather than “on the next tick”; leave the URL.revokeObjectURL timing and behavior unchanged.web/e2e/dashboard.spec.ts (2)
253-257: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTie the expected pixel dimensions to their source values.
2160isCARD_SIZES.square.widthmultiplied by the defaultpixelRatioof 2 inrasterize. If either value changes, this test fails with a bare number mismatch and no hint about the cause. A short derivation makes the intent readable and the failure self-explaining.💚 Suggested clarification
// PNG magic number, then the IHDR width/height, which prove the card was // rasterized at its declared size rather than as an empty or clipped canvas. + // 1080 logical px (CARD_SIZES.square) at the rasterizer's default pixelRatio of 2. + const expected = 1080 * 2; expect(bytes.subarray(0, 8).toString("hex")).toBe("89504e470d0a1a0a"); - expect(bytes.readUInt32BE(16)).toBe(2160); - expect(bytes.readUInt32BE(20)).toBe(2160); + expect(bytes.readUInt32BE(16)).toBe(expected); + expect(bytes.readUInt32BE(20)).toBe(expected);🤖 Prompt for 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. In `@web/e2e/dashboard.spec.ts` around lines 253 - 257, Update the PNG dimension assertions in the rasterization test to derive the expected width and height from CARD_SIZES.square.width and the default pixelRatio used by rasterize, rather than hard-coding 2160. Keep both dimension checks tied to these source values so future changes produce clear, self-explanatory failures.
230-236: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard the
querySelectorcast insidepage.evaluate.The cast at Line 231 assumes the card node exists. If the off-screen card is ever unmounted, the callback throws a
TypeErroroncard.childrenand the report points at the evaluate call rather than at the missing node. An explicit check produces a clearer failure.🛡️ Suggested guard
const bands = await page.evaluate(() => { - const card = document.querySelector('[data-testid="share-card"]') as HTMLElement; + const card = document.querySelector<HTMLElement>('[data-testid="share-card"]'); + if (card === null) { + throw new Error("The off-screen share card node was not rendered."); + } return {🤖 Prompt for 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. In `@web/e2e/dashboard.spec.ts` around lines 230 - 236, Add an explicit null check for the querySelector result inside the page.evaluate callback before accessing card.children or its dimensions, producing a clear failure when the share card is absent while preserving the existing measurements when it exists.
🤖 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 `@web/src/components/ShareDialog.tsx`:
- Around line 238-292: In the Shape, Theme, and Model rows button groups within
ShareDialog, add aria-pressed bindings that evaluate to true when each button’s
value matches the corresponding presentation field (ratio, theme, or rows),
mirroring the existing hideDollars control. Keep the current variant styling and
selection handlers unchanged.
In `@web/src/lib/shareImage.ts`:
- Around line 105-110: Update shareFilename to validate startIso and endIso
before deriving dates, and use a clear fallback filename or date value when
either input is empty so the result never contains an empty date segment.
Preserve the existing same-day and date-range naming behavior for valid ISO
strings, and ensure the UsagePage download remains self-describing.
In `@web/src/lib/usageTotals.ts`:
- Around line 11-20: Normalize absent share-card values to undefined: update
billedTokenTotal and cacheHitRate in web/src/lib/usageTotals.ts:11-20, 38-55,
and formatLatency in web/src/lib/shareCard.ts:10-18 while keeping
formatLatency’s input as number | null; change CardModel.key and null row-key
mapping in web/src/lib/shareCard.ts:44-53 and preserve isOther; change
resolveHero and ShareCardProps.hero to CardStat | undefined in
web/src/lib/shareCard.ts:82-107, 128-130; update availableStats, UsagePage.tsx,
and affected expectations in web/src/pages/UsagePage.tsx:851-855 and
web/src/lib/shareCard.test.ts:121-133 to check undefined, while leaving nullable
API fields such as UsageTotals.avg_latency_ms and UsageGroupRow.key unchanged.
In `@web/src/pages/UsagePage.tsx`:
- Line 997: Update web/src/pages/UsagePage.tsx:997 to pass the server-provided
model breakdown ranked by token volume under the active filters instead of the
cost-ranked capped data.by_model result. In web/src/lib/shareCard.ts:44-55, keep
cardModels client sorting only for presentation and not top-model selection. In
web/src/lib/shareCard.test.ts:28-34, add coverage using a cost-ranked capped
response to verify share-card rows come from the token-ranked server result.
- Around line 269-270: Update the error-state class in the table cell rendered
by UsagePage to replace the raw text-red-700 palette class with
text-[var(--otari-danger)], while preserving the existing muted token for
non-error rows.
---
Nitpick comments:
In `@web/e2e/dashboard.spec.ts`:
- Around line 253-257: Update the PNG dimension assertions in the rasterization
test to derive the expected width and height from CARD_SIZES.square.width and
the default pixelRatio used by rasterize, rather than hard-coding 2160. Keep
both dimension checks tied to these source values so future changes produce
clear, self-explanatory failures.
- Around line 230-236: Add an explicit null check for the querySelector result
inside the page.evaluate callback before accessing card.children or its
dimensions, producing a clear failure when the share card is absent while
preserving the existing measurements when it exists.
In `@web/src/components/ShareCard.test.tsx`:
- Around line 107-118: Update the height assertions in the “keeps rows legible
and the hero present” test to first verify the expected number of measured rows,
without filtering out collapsed heights, then assert every row height meets the
minimum threshold. Preserve the existing hero assertion and use the rendered
model count to establish the expected row count.
- Around line 62-77: Replace the getByTestId("share-card") queries in renders
both ratios at their exact pixel sizes with an accessible user-facing query.
Update ShareCard’s root element to expose a stable role and accessible name,
then use screen.getByRole with that name in both assertions while preserving the
existing size checks.
In `@web/src/components/ShareCard.tsx`:
- Around line 122-129: Add a concise comment near the rowHeight calculation
explaining that at very high row counts the minimum 34px row height may exceed
rowsBudget intentionally, with the flexible middle block absorbing the surplus;
preserve the existing heroSize and sizing logic.
In `@web/src/components/ShareDialog.test.tsx`:
- Around line 116-120: Add a test alongside “survives a corrupt stored
presentation instead of crashing” that stores valid JSON with an unknown ratio
value, renders via renderDialog(), and verifies the dialog still renders the
Download PNG button. This should exercise the invalid parsed presentation path
in ShareDialog.
- Around line 54-93: Add two behavior-focused tests to the ShareDialog suite:
interact with a share control and assert that the resulting ShareCard content
changes, then mock or trigger a rejected copy action and assert the dialog
displays its failure notice. Reuse the existing renderDialog setup and visible
control/error symbols from ShareDialog.tsx, while retaining the current absence
assertions.
In `@web/src/components/ShareDialog.tsx`:
- Around line 72-75: Update ShareDialog’s preview, error, and notice state to
use undefined rather than null for absent values, including corresponding
initializers and checks. Store the setTimeout handle used by the notice flow in
a ref, and clear it in the existing unmount effect so the callback cannot run
after the dialog closes.
- Around line 185-193: Replace the inline error and stale-warning divs in
ShareDialog with the shared ErrorBanner and InfoBanner components, passing the
existing error text and stale-state message through their supported props.
Preserve the current conditional rendering and messages while removing the
duplicated Tailwind styling.
In `@web/src/lib/shareImage.ts`:
- Around line 69-76: Add a bounded timeout to the image-loading Promise in
shareImage so it rejects when neither onload nor onerror fires, allowing
ShareDialog.withBlob to clear busy state and report the failure. Ensure the
timeout is cancelled when the image settles normally and preserve the existing
load/error behavior.
- Around line 121-123: Update the comment above the setTimeout in the share
image download flow to describe revocation after the 10-second delay rather than
“on the next tick”; leave the URL.revokeObjectURL timing and behavior unchanged.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bae5976c-5527-4237-aac5-033fb17a65db
📒 Files selected for processing (14)
TODOS.mddocs/dashboard.mdweb/e2e/dashboard.spec.tsweb/src/components/ShareCard.test.tsxweb/src/components/ShareCard.tsxweb/src/components/ShareDialog.test.tsxweb/src/components/ShareDialog.tsxweb/src/lib/shareCard.test.tsweb/src/lib/shareCard.tsweb/src/lib/shareImage.tsweb/src/lib/usageTotals.tsweb/src/pages/UsagePage.test.tsxweb/src/pages/UsagePage.tsxweb/src/test/setup.ts
github-actions
Bot
removed
the
missing-template
Adds a share icon to the bottom-right of the Usage page histogram. It opens a dialog with a live preview of a fixed-size card and hands back a PNG to copy or download. The card takes its data from whatever the page is filtered to, reading the page's existing summary rather than issuing a query of its own, so the window, the drag-to-zoom range and the entity filters all carry over and the card cannot disagree with the numbers above it. The card names its own scope, so a filtered figure is not read as the whole gateway. The dialog controls presentation only (lead stat, title, shape, theme, row count, whether dollars appear); those choices persist, the data scope never does, because restoring a stale window would silently change what the card claims. Rasterization is dependency-free: serialize the card node with XMLSerializer, wrap it in an SVG foreignObject, load that from a data: URI and draw it to a canvas. Three details are load-bearing. A blob: URL taints the canvas in Chromium so toBlob() refuses outright; XMLSerializer rather than string templates keeps an "&" in a title from making the SVG unparseable; and the card uses literal colors, not var(--otari-*), because custom properties do not resolve inside an img-loaded SVG. The card frame is fixed, so the model rows divide a height budget instead of carrying fixed heights. Hard-coded heights failed both ways: three rows left a third of a square card blank, and nine overflowed so flex-shrink collapsed the title to zero height. Model names collapse to the final segment, so a routed selector reads as `deepseek-v4-flash`. Spend is asterisked when the window holds unpriced requests, and treats a gateway with no unpriced_requests field as unknown rather than zero. A stat the window has no value for is omitted rather than published as a dash. No user or key names reach the card. Covered by unit tests for the derivations and the height budget, and by an e2e test that seeds usage and asserts the downloaded file's PNG signature and IHDR dimensions. That last one is not optional: jsdom has no canvas, no toBlob and no object URLs, so the tainted-canvas and collapsed-band bugs were both invisible to a fully green unit suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four findings from @khaledosman on #555, all confirmed: The preview never settled. `secondary` and `shown` were rebuilt on every render and sat in the rasterize effect's dependency list, so the effect's own setPreview re-armed the 300ms debounce and the card re-encoded for as long as the dialog stayed open. Both are memoized now. jsdom cannot show this: rasterize throws there and React bails on an identical error string, so nothing re-renders. The regression test is in the e2e, where the preview src must be unchanged after an idle wait, and it fails against the previous bundle. "Image copied" was shown even when the copy failed, because copyBlobAsImage swallows its error and returns false while withBlob ignored the result. It now throws, so the failure surfaces as the error banner. Stored presentation was spread over the defaults without validation, so a `ratio` this build no longer has reached CARD_SIZES[ratio] and threw on the destructure. Each field is validated against its known set and falls back on its own. The prior test only covered unparseable JSON. The caveat asterisk had no legend. A PNG travels without the docs that explain it, so the card prints "N requests unpriced" beside its scope line whenever a caveated stat is actually shown. Also drops TODOS.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/components/ShareDialog.tsx (1)
159-186: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClean up preview resources and notice timers.
Store the latest preview URL and notice timeout in refs. Revoke the URL and clear the timeout during cleanup. React ignores the
setPreviewupdater during unmount, and an earlier timeout can clear a newer notice.Add tests for closing after preview creation and for two consecutive successful actions.
🤖 Prompt for 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. In `@web/src/components/ShareDialog.tsx` around lines 159 - 186, Update ShareDialog’s preview and notice lifecycle around the existing useEffect and withBlob symbols: store the latest preview object URL and notice timeout ID in refs, update those refs whenever preview or notice changes, and revoke/clear them in the unmount cleanup instead of relying on the setPreview updater. Ensure each new notice timeout replaces or safely clears the previous one so consecutive successful actions cannot have an earlier timeout clear a newer notice, and add tests covering close-after-preview and two consecutive successful actions.
🧹 Nitpick comments (1)
web/src/components/ShareDialog.tsx (1)
90-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
undefinedfor local absent values.
preview,error,notice, andurlusenullas an absence sentinel. Replace these own TypeScript types and their comparisons withundefinedconsistently.As per coding guidelines, “Use
undefinedrather thannullfor absent values in own TypeScript types.”Also applies to: 125-125
🤖 Prompt for 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. In `@web/src/components/ShareDialog.tsx` around lines 90 - 93, Update the ShareDialog state declarations for preview, error, and notice, plus the url value, to use undefined instead of null as the absence sentinel. Replace their explicit TypeScript types, initial values, and null comparisons consistently while preserving the existing present-value behavior.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@web/src/components/ShareDialog.tsx`:
- Around line 159-186: Update ShareDialog’s preview and notice lifecycle around
the existing useEffect and withBlob symbols: store the latest preview object URL
and notice timeout ID in refs, update those refs whenever preview or notice
changes, and revoke/clear them in the unmount cleanup instead of relying on the
setPreview updater. Ensure each new notice timeout replaces or safely clears the
previous one so consecutive successful actions cannot have an earlier timeout
clear a newer notice, and add tests covering close-after-preview and two
consecutive successful actions.
---
Nitpick comments:
In `@web/src/components/ShareDialog.tsx`:
- Around line 90-93: Update the ShareDialog state declarations for preview,
error, and notice, plus the url value, to use undefined instead of null as the
absence sentinel. Replace their explicit TypeScript types, initial values, and
null comparisons consistently while preserving the existing present-value
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 74f06a6a-4447-4925-bb41-807e6d193e7d
📒 Files selected for processing (5)
web/e2e/dashboard.spec.tsweb/src/components/ShareCard.test.tsxweb/src/components/ShareCard.tsxweb/src/components/ShareDialog.test.tsxweb/src/components/ShareDialog.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/components/ShareCard.test.tsx
Correctness: - The height-budget test could not catch the regression it described. It filtered row heights to `> 30` before asserting, which discarded exactly the collapsed row, and never asserted how many rows rendered, so an empty list passed. It now asserts the count first, then each height. - rasterize could hang forever. The promise settled only on load or error, so a decode firing neither left the dialog busy with both actions disabled and nothing said. Bounded at 15s, which turns it into the normal error path. - shareFilename produced "otari-usage-.png" when the window was not yet resolved. The date is dropped rather than replaced with today, since stamping the wrong date on a card is worse than not labelling it. - The success-notice timer was never cleared, so closing the dialog inside the 2s window updated an unmounted component. Found while fixing the above: swapping the inline status blocks for the shared ErrorBanner replaced every specific message with "Something went wrong.", because errorMessage() only unwraps an Error. The error state holds an Error now, so the dialog's own wording survives. The new copy-failure test is what caught it. Accessibility and house style: - aria-pressed on the lead-stat, shape, theme and row-count groups, which signalled the current choice through `variant` alone. - The card carries role="img" and an aria-label, so its own tests query it as a user would rather than by test id. Its off-screen twin stays aria-hidden, so the dialog test and the e2e locate that one by attribute. - Own types use undefined rather than null for absent values. Nullable API fields are unchanged. - The dialog reuses ErrorBanner and InfoBanner instead of restyling them. Also: a comment stating that the 34px row floor deliberately overrides rowsBudget at nine rows, a guard so an unmounted card reports itself rather than throwing inside page.evaluate, a comment corrected to match its 10s delay, and a positive test that changes the lead stat and asserts the card follows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CodeRabbit marked the first of these resolved; it was not. - The failed-tool-calls cell used `text-red-700` where the dashboard has an `--otari-danger` token. - `cardModels` re-ranks by tokens, but it can only re-rank what arrived: the server caps the breakdown at its top 100 by spend, so above 100 distinct models a cheap high-volume one never reaches the client. Stated where the ranking happens. Ranking server-side needs a token-ordered breakdown, a gateway change this dashboard-only PR does not make. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Description
Adds a share icon to the bottom-right of the Usage page histogram. It opens a dialog with a live preview of a fixed-size card and hands back a PNG to copy or download.
Data comes from the page. The card reads the page's existing summary instead of issuing its own query, so the window, the drag-to-zoom range and the entity filters all carry over, and the card cannot disagree with the numbers above it. It names its own scope, so a filtered figure is not read as the whole gateway. The dialog controls presentation only (lead stat, title, shape, theme, row count, whether dollars appear). Those choices persist; the data scope never does, since restoring a stale window would silently change what the card claims.
Rasterization adds no dependency. Serialize the card with
XMLSerializer, wrap it in an SVGforeignObject, load it from adata:URI, draw to a canvas. Ablob:URL taints the canvas in Chromium sotoBlob()refuses outright, and the card uses literal colors rather thanvar(--otari-*)because custom properties do not resolve inside animg-loaded SVG.Honesty details. Spend is asterisked when the window holds unpriced requests, with the count printed on the card itself since a PNG travels without the docs. An absent
unpriced_requestscounts as unknown rather than zero. A stat with no value is omitted, not published as a dash. No user or key names reach the card. The footer URL is hardcoded, so a self-hosted gateway never publishes its own hostname.Frame is fixed, so rows divide a height budget. Hard-coded row heights failed both ways: three rows left a third of a square card blank, and nine overflowed so flex-shrink collapsed the title to zero height.
PR Type
Relevant issues
Closes #525
Checklist
tests/unit,tests/integration).make lint,make typecheck,make test).uv run python scripts/generate_openapi.py).Notes on the boxes above, since a tick is easy and the detail is not:
web/), nottests/unitortests/integration: no Python changed in this PR. Unit tests cover the derivations and the height budget; the e2e seeds usage and asserts the downloaded file's PNG signature and IHDR dimensions.make lint,make typecheck(mypy, 324 files),uv run pytest tests/unit(1561 passed),npm --prefix web run typecheck,npm --prefix web test(563 passed), and the Playwright suite (9 passed).make testalso runstests/integration, which needs PostgreSQL that this environment has no Docker for, so that half did not run here.provider_modelsummary dimension; that was reverted when the open-weights split was cut, sosrc/gateway/is untouched.make openapi-checkandmake postman-checkboth pass.AI Usage
AI Model/Tool used:
Claude Opus 5 (1M context), via Claude Code.
Any additional AI details you'd like to share:
Design and scope decisions were @njbrake's, iterated in conversation; the implementation and this description are the model's.
Worth knowing for review: two bugs in this feature were invisible to a fully green unit suite and only surfaced when driving a real browser. Drawing an SVG from a
blob:URL taints the canvas, sotoBlob()refused and the card never rendered at all; and a long model list overflowed the fixed frame so flex-shrink collapsed the title. jsdom has no canvas, notoBloband no object URLs, which is why the e2e assertions on the actual PNG bytes are in this PR rather than left as a follow-up.