tsk-ihf2vw [OPEN] Library UI: storage accounting view#2099
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
Next review available in: 21 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
✨ 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 |
|
nemotron-super review VERDICT: Test flaw in storage view test may cause false test failure.
Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| expect(screen.queryByText(/Paused at cap/)).toBeNull(); | ||
| }); | ||
|
|
||
| it("shows paused-at-cap warning when total exceeds cap", async () => { |
There was a problem hiding this comment.
🔥 The Roast: This test creates 60 mock items and assumes their total mock bytes will exceed the 50GB cap. But mockItemBytes generates 100MB–4.1GB per item deterministically via a hash of the ID. With 60 items at minimum 100MB each, that's only 6GB — nowhere near 50GB. At maximum it's 246GB. Whether this test passes depends entirely on the hash values of overflow-0 through overflow-59. This is a flaky test waiting to happen.
🩹 The Fix:
| it("shows paused-at-cap warning when total exceeds cap", async () => { | |
| const manyItems = Array.from({ length: 60 }, (_, i) => ({ | |
| ...MOCK_ITEMS[0], | |
| id: `overflow-${i}`, | |
| title: `Overflow Item ${i}`, | |
| source_type: "youtube", | |
| created_at: 1700000000 + i, | |
| updated_at: 1700000000 + i, | |
| // Force large mock bytes by using IDs that hash to max values | |
| // Or better: mock deriveMockStorageData directly in the test | |
| })); |
Better approach: Mock the fetch to return items, then spy on deriveMockStorageData or just override the storage calculation in the test. But since it's not exported, the simplest fix is to use IDs that you've verified produce large hash values, or increase the item count significantly (e.g., 600 items guarantees >50GB even at minimum).
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| const sources: SourceStorage[] = Array.from(sourceMap.entries()) | ||
| .map(([source_type, data]) => ({ source_type, bytes: data.bytes, item_count: data.count })) | ||
| .sort((a, b) => b.bytes - a.bytes); | ||
| return { |
There was a problem hiding this comment.
🔥 The Roast: deriveMockStorageData(items) is called directly inside the render body (inside storageViewUI IIFE). This means it recalculates mock bytes for every item on every single render — including when unrelated state changes like search text, filter toggles, or detail view updates. The hash computation loops over every character of every item ID. With hundreds of items, this is wasted CPU cycles on every keystroke.
🩹 The Fix: Wrap it in useMemo:
| return { | |
| const storageViewData = useMemo(() => deriveMockStorageData(items), [items]); |
Then use storageViewData in the render instead of calling deriveMockStorageData(items) inline.
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| /* Helpers */ | ||
| /* ------------------------------------------------------------------ */ | ||
|
|
||
| const fmtBytes = (b: number): string => { |
There was a problem hiding this comment.
🔥 The Roast: fmtBytes divides by 1024 (binary) but labels the units KB, MB, GB (decimal). This is the classic "my hard drive is smaller than advertised" lie. 1024 bytes = 1 KiB, not 1 KB. Either use 1000-based units for KB/MB/GB, or label them KiB/MiB/GiB.
🩹 The Fix:
| const fmtBytes = (b: number): string => { | |
| const fmtBytes = (b: number): string => { | |
| if (b < 1024) return `${b} B`; | |
| if (b < 1024 * 1024) return `${(b / 1024).toFixed(1)} KiB`; | |
| if (b < 1024 * 1024 * 1024) return `${(b / (1024 * 1024)).toFixed(1)} MiB`; | |
| return `${(b / (1024 * 1024 * 1024)).toFixed(2)} GiB`; | |
| }; |
📏 Severity: nitpick
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| <p className="text-[10px] uppercase tracking-wider text-shell-text-tertiary mb-2">By source</p> | ||
| <table className="w-full text-xs"> | ||
| <thead> | ||
| <tr className="text-shell-text-tertiary border-b border-white/5"> |
There was a problem hiding this comment.
🔥 The Roast: Table headers (<th>) are missing scope="col". Screen readers won't know these headers apply to the columns below. This is Accessibility 101.
🩹 The Fix:
| <tr className="text-shell-text-tertiary border-b border-white/5"> | |
| <th className="text-left pb-1.5 font-normal" scope="col">Source</th> |
(Add scope="col" to all four <th> elements in this table and the next one.)
📏 Severity: suggestion
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| <td className="py-1.5 pr-3 text-shell-text-tertiary tabular-nums">{src.item_count}</td> | ||
| <td className="py-1.5 pr-3 text-shell-text-secondary text-right tabular-nums">{fmtBytes(src.bytes)}</td> | ||
| <td className="py-1.5"> | ||
| <div className="h-1.5 w-full rounded-full bg-white/5" role="progressbar" aria-valuenow={pct} aria-valuemin={0} aria-valuemax={100}> |
There was a problem hiding this comment.
🔥 The Roast: The outer <div> has role="progressbar" with aria attributes, and the inner <div> also has role="progressbar" with the same aria attributes. That's double the announcements for screen reader users — they'll hear the progress value twice. Pick one element to be the progressbar.
🩹 The Fix: Remove role="progressbar" aria-valuenow={pct} aria-valuemin={0} aria-valuemax={100} from the inner <div> (the colored bar). Keep it only on the outer track element.
📏 Severity: suggestion
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| /> | ||
| </div> | ||
| {data.paused_at_cap && ( | ||
| <div className="flex items-center gap-1.5 text-xs text-red-400 bg-red-500/10 border border-red-500/20 rounded-md px-2 py-1"> |
There was a problem hiding this comment.
🔥 The Roast: The AlertCircle icon is decorative (the text "Paused at cap..." conveys the meaning), but it's missing aria-hidden="true". Screen readers will announce "alert circle" which is noise.
🩹 The Fix:
| <div className="flex items-center gap-1.5 text-xs text-red-400 bg-red-500/10 border border-red-500/20 rounded-md px-2 py-1"> | |
| <AlertCircle size={12} aria-hidden="true" /> |
📏 Severity: nitpick
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| <tr key={item.id} className="border-t border-white/5"> | ||
| <td className="py-1.5 pr-3 text-shell-text-secondary truncate max-w-[200px]">{item.title}</td> | ||
| <td className="py-1.5 pr-3 text-shell-text-tertiary">{SOURCE_LABELS[item.source_type] ?? item.source_type}</td> | ||
| <td className="py-1.5 pr-3 text-shell-text-secondary text-right tabular-nums">{fmtBytes(item.bytes)}</td> |
There was a problem hiding this comment.
🔥 The Roast: Same duplicate role="progressbar" issue on the "By item" table progress bars. Outer div has it, inner div has it too.
🩹 The Fix: Remove the duplicate role and aria attributes from the inner colored bar <div>.
📏 Severity: suggestion
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| vi.restoreAllMocks(); | ||
| }); | ||
|
|
||
| it("renders the view mode toggle and switches to storage view", async () => { |
There was a problem hiding this comment.
🔥 The Roast: The storage view has a branch for items.length === 0 showing "No items to account for", but there's no test for it. Zero-coverage edge case.
🩹 The Fix: Add a test:
| it("renders the view mode toggle and switches to storage view", async () => { | |
| it("shows empty state when no items exist", async () => { | |
| const emptyOverrides: Record<string, Promise<Response>> = { | |
| "/api/knowledge/items": Promise.resolve({ | |
| ok: true, | |
| status: 200, | |
| headers: new Map([["content-type", "application/json"]]), | |
| json: () => Promise.resolve({ items: [], count: 0 }), | |
| } as Response), | |
| }; | |
| vi.stubGlobal("fetch", createFetchMock(emptyOverrides) as unknown as typeof fetch); | |
| render(<LibraryApp windowId="test-win" />); | |
| await waitFor(() => screen.getByRole("radio", { name: "storage" })); | |
| fireEvent.click(screen.getByRole("radio", { name: "storage" })); | |
| await waitFor(() => screen.getByText("No items to account for")); | |
| }); |
📏 Severity: suggestion
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| items: ItemStorageRow[]; | ||
| } | ||
|
|
||
| const STORAGE_CAP_BYTES = 50 * 1024 * 1024 * 1024; |
There was a problem hiding this comment.
🔥 The Roast: STORAGE_CAP_BYTES = 50 * 1024 * 1024 * 1024 — the classic "count the asterisks" game. Use the exponentiation operator for readability: 50 * 1024 ** 3.
🩹 The Fix:
| const STORAGE_CAP_BYTES = 50 * 1024 * 1024 * 1024; | |
| const STORAGE_CAP_BYTES = 50 * 1024 ** 3; |
📏 Severity: nitpick
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| return `${(b / (1024 * 1024 * 1024)).toFixed(2)} GB`; | ||
| }; | ||
|
|
||
| const mockItemBytes = (id: string): number => { |
There was a problem hiding this comment.
🔥 The Roast: The hash function uses hash |= 0 to force 32-bit signed integer behavior, but then Math.abs(hash) can return 2147483648 for INT32_MIN (-2147483648), which Math.abs doesn't negate in JS (it stays negative). The % 4000 on a negative number yields a negative remainder in JS. So Math.abs(hash) % 4000 could be negative, making the final bytes calculation wrong. In practice the hash distribution makes this astronomically unlikely, but it's a latent bug.
🩹 The Fix:
| const mockItemBytes = (id: string): number => { | |
| const mockItemBytes = (id: string): number => { | |
| let hash = 0; | |
| for (let i = 0; i < id.length; i++) { | |
| hash = ((hash << 5) - hash) + id.charCodeAt(i); | |
| hash |= 0; | |
| } | |
| return ((hash >>> 0) % 4000 + 100) * 1024 * 1024; | |
| }; |
Using >>> (unsigned right shift) converts to unsigned 32-bit, avoiding the INT32_MIN edge case and negative modulo.
📏 Severity: nitpick
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review Roast 🔥Verdict: 9 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)
🏆 Best part: The storage accounting UI is genuinely well-designed — progress bars with color thresholds (sky→amber→red), per-source and per-item breakdowns, and the "paused at cap" warning. The mock data derivation is clever for a demo. Oh wait, this part is actually clean. I need to sit down. 💀 Worst part: The flaky test at line 162. It looks like it tests the cap-exceeded case, but with 60 items at a minimum of 100MB each, you only get 6GB — an order of magnitude short of the 50GB cap. Whether it passes depends on hash lottery. This is a time bomb. 📊 Overall: Like a beautifully plated dish with a hair in it — the UI work is solid, but the test reliability and render-performance issues need fixing before merge. Files Reviewed (2 files)
|
|
nemotron-ultra-kilo review VERDICT: Overall solid feature implementation with good test coverage, but has a few correctness concerns and style issues.
Automated first-pass review by the nemotron-ultra-kilo lane. The lead still reviews before merge. |
|
nemotron-ultra-orB review VERDICT: Acceptable with concerns — mock storage accounting needs real implementation; tests have gaps and flaky assumptions
Automated first-pass review by the nemotron-ultra-orB lane. The lead still reviews before merge. |
| expect(screen.queryByText(/Paused at cap/)).toBeNull(); | ||
| }); | ||
|
|
||
| it("shows paused-at-cap warning when total exceeds cap", async () => { |
There was a problem hiding this comment.
🔥 The Roast: This test creates 60 items to exceed the 50GB cap, but mockItemBytes returns a minimum of 100MB per item (100 * 1024 * 1024). 60 × 100MB = 6GB — an order of magnitude short of 50GB. Whether this passes depends entirely on hash lottery of the overflow-{i} IDs. This is a flaky time bomb.
🩹 The Fix: Either mock deriveMockStorageData directly to return a controlled total_bytes > cap_bytes, or increase the item count to ~500+, or override mockItemBytes in the test.
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| const sources: SourceStorage[] = Array.from(sourceMap.entries()) | ||
| .map(([source_type, data]) => ({ source_type, bytes: data.bytes, item_count: data.count })) | ||
| .sort((a, b) => b.bytes - a.bytes); | ||
| return { |
There was a problem hiding this comment.
🔥 The Roast: deriveMockStorageData(items) is called inline during render (line 1111). Every keystroke, filter change, or parent re-render recalculates mock bytes for all items. With hundreds of items, this is wasted CPU. It's a pure function of items — memoize it.
🩹 The Fix: Wrap with useMemo: const data = useMemo(() => deriveMockStorageData(items), [items]);
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| /* Helpers */ | ||
| /* ------------------------------------------------------------------ */ | ||
|
|
||
| const fmtBytes = (b: number): string => { |
There was a problem hiding this comment.
🔥 The Roast: fmtBytes divides by 1024 (binary) but labels the units KB, MB, GB (decimal). Technically these are KiB, MiB, GiB. The industry ignores this distinction, but pedants will pedant.
🩹 The Fix: Either change labels to KiB/MiB/GiB, or divide by 1000 for true KB/MB/GB. Pick a lane.
📏 Severity: suggestion
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| <p className="text-[10px] uppercase tracking-wider text-shell-text-tertiary mb-2">By source</p> | ||
| <table className="w-full text-xs"> | ||
| <thead> | ||
| <tr className="text-shell-text-tertiary border-b border-white/5"> |
There was a problem hiding this comment.
🔥 The Roast: Table headers (<th>) in the "By source" section lack scope="col". Screen readers won't associate headers with their columns properly.
🩹 The Fix: Add scope="col" to each <th> in both tables.
| <tr className="text-shell-text-tertiary border-b border-white/5"> | |
| <th className="text-left pb-1.5 font-normal" scope="col">Source</th> |
📏 Severity: suggestion
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| <td className="py-1.5 pr-3 text-shell-text-tertiary tabular-nums">{src.item_count}</td> | ||
| <td className="py-1.5 pr-3 text-shell-text-secondary text-right tabular-nums">{fmtBytes(src.bytes)}</td> | ||
| <td className="py-1.5"> | ||
| <div className="h-1.5 w-full rounded-full bg-white/5" role="progressbar" aria-valuenow={pct} aria-valuemin={0} aria-valuemax={100}> |
There was a problem hiding this comment.
🔥 The Roast: The progress bar has role="progressbar" on both the outer <div> (line 1061) and inner <div> (line 1062). Screen readers will announce the progress twice — once for the track, once for the fill. Pick one.
🩹 The Fix: Remove role="progressbar" and aria attributes from the outer wrapper; keep them on the inner fill element.
📏 Severity: suggestion
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| /> | ||
| </div> | ||
| {data.paused_at_cap && ( | ||
| <div className="flex items-center gap-1.5 text-xs text-red-400 bg-red-500/10 border border-red-500/20 rounded-md px-2 py-1"> |
There was a problem hiding this comment.
🔥 The Roast: The AlertCircle icon in the "Paused at cap" warning is decorative but lacks aria-hidden="true". Screen readers may announce "Alert circle" which adds noise.
🩹 The Fix: Add aria-hidden="true" to the <AlertCircle /> component.
📏 Severity: nitpick
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| <tr key={item.id} className="border-t border-white/5"> | ||
| <td className="py-1.5 pr-3 text-shell-text-secondary truncate max-w-[200px]">{item.title}</td> | ||
| <td className="py-1.5 pr-3 text-shell-text-tertiary">{SOURCE_LABELS[item.source_type] ?? item.source_type}</td> | ||
| <td className="py-1.5 pr-3 text-shell-text-secondary text-right tabular-nums">{fmtBytes(item.bytes)}</td> |
There was a problem hiding this comment.
🔥 The Roast: Same duplicate role="progressbar" issue on the "By item" table progress bars — outer and inner divs both have it.
🩹 The Fix: Remove from outer wrapper, keep on inner fill element.
📏 Severity: suggestion
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| vi.restoreAllMocks(); | ||
| }); | ||
|
|
||
| it("renders the view mode toggle and switches to storage view", async () => { |
There was a problem hiding this comment.
🔥 The Roast: The storageViewUI has a branch for items.length === 0 (line 1000-1009) showing "No items to account for", but no test covers it. Untested code is broken code waiting to happen.
🩹 The Fix: Add a test that renders LibraryApp with empty items and asserts the empty state message appears in storage view.
📏 Severity: suggestion
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| items: ItemStorageRow[]; | ||
| } | ||
|
|
||
| const STORAGE_CAP_BYTES = 50 * 1024 * 1024 * 1024; |
There was a problem hiding this comment.
🔥 The Roast: 50 * 1024 * 1024 * 1024 is readable to people who think in powers of two, but 50 * 1024 ** 3 or 50 << 30 is clearer about intent (50 GiB).
🩹 The Fix: Use 50 * 1024 ** 3 for clarity.
📏 Severity: nitpick
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review Roast 🔥Verdict: 9 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)
🏆 Best part: The storage accounting UI is genuinely well-designed — clean tables, progress bars with color thresholds, and the "Paused at cap" warning is a nice UX touch. The mock data approach is pragmatic for a demo view. 💀 Worst part: That flaky test. It looks like it tests the cap warning, but 60 items × 100MB minimum = 6GB. You'd need ~500 items at minimum size to hit 50GB. The test passes only by hash lottery. 📊 Overall: Like a beautifully plated dish with a hair in it — the presentation (UI) is great, but the test reliability needs fixing before this leaves the kitchen. Files Reviewed (2 files)
Fix these issues in Kilo Cloud Reviewed by nemotron-3-ultra-550b-a55b:free · Input: 90.4K · Output: 9.8K · Cached: 483.1K |
1 participant
Autonomous build of board card tsk-ihf2vw.
Files:
desktop/src/apps/LibraryApp.storage.test.tsx | 190 ++++++++++++++++++++++
desktop/src/apps/LibraryApp.tsx | 230 +++++++++++++++++++++++++--
2 files changed, 406 insertions(+), 14 deletions(-)
Summary by Gitar
storageview toLibraryAppallowing users to toggle between items and storage accounting.deriveMockStorageDatato calculate and sort storage usage by source and individual items.LibraryApp.storage.test.tsxto verify view toggling, data rendering, and storage limit overflow logic.This will update automatically on new commits.