feat(library): item card component with thumbnail, status, artifacts, collection link#2097
Conversation
… collection link Add LibraryItemCard component per docs/design/library-app.md sections 2-4. Card shows thumbnail (or placeholder), title, kind badge, media duration, pipeline status per stage (jobs shape), artifact list (text, transcript, description, ocr) with preview, link-to-collection action, and a disabled Download stub until P3. Failure states are always visible -- no silent empties for missing thumbnails, pipeline stages, artifacts, or errors. Includes lib/library.ts with types and API client for the library store (items, artifacts, jobs) and 25 component tests covering pending, processing, ready, and error states.
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? |
📝 WalkthroughWalkthroughAdds typed library API operations, a ChangesLibrary feature
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
desktop/src/components/LibraryItemCard.tsx (1)
100-100: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueItem-level
previewis repeated for every text artifact.
previewis derived once fromitem.meta_json(Line 100) but rendered inside the per-artifact loop (Lines 223-231), so multiple text artifacts (e.g.transcript+ocr) all show the same item-level preview. If per-artifact previews are intended, read the preview fromart.meta_jsoninstead.Also applies to: 223-231
🤖 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 `@desktop/src/components/LibraryItemCard.tsx` at line 100, The artifact rendering logic in LibraryItemCard is using the item-level preview for every artifact. Update the per-artifact rendering path to derive preview from each artifact’s art.meta_json, while preserving the existing fallback behavior when no artifact preview is available; remove or stop using the item-level preview for those rendered artifacts.desktop/src/lib/library.ts (1)
86-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
fetchJsonfor JSON-returning helpers. Both helpers re-implement the exactres.ok+content-type+res.json()+ try/catch logic already centralized infetchJson, causing drift risk in validation behavior. (deleteLibraryItem/reprocessLibraryItemcorrectly stay as-is since they only needres.ok.)
desktop/src/lib/library.ts#L86-L98: replace the manual fetch/parse withfetchJson<LibraryItemDetail | null>(url, null).desktop/src/lib/library.ts#L137-L146: route the POST throughfetchJson(passinginitwith method/body andnullfallback) instead of duplicating the parse/validation block.🤖 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 `@desktop/src/lib/library.ts` around lines 86 - 98, Reuse the centralized fetchJson validation instead of duplicating fetch, response checks, JSON parsing, and error handling: in desktop/src/lib/library.ts lines 86-98, update getLibraryItem to call fetchJson<LibraryItemDetail | null>(url, null); in desktop/src/lib/library.ts lines 137-146, update the POST helper to call fetchJson with its method/body init and null fallback. Leave deleteLibraryItem and reprocessLibraryItem unchanged.
🤖 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 `@desktop/src/components/LibraryItemCard.tsx`:
- Around line 150-165: Update the thumbnail rendering in LibraryItemCard so an
image load failure replaces the failed <img> with the existing visible “No
thumbnail” placeholder instead of merely hiding it. Preserve the current
placeholder for absent thumbnailArtifact values and ensure the onError handling
leaves no blank thumbnail area.
In `@desktop/src/lib/library.ts`:
- Around line 128-150: Update ingestLibraryUrl and IngestLibraryOptions so the
declared source option is not silently ignored: include opts?.source in the JSON
request payload if the backend accepts it, otherwise remove source from the
interface. Keep the existing url and title behavior unchanged.
---
Nitpick comments:
In `@desktop/src/components/LibraryItemCard.tsx`:
- Line 100: The artifact rendering logic in LibraryItemCard is using the
item-level preview for every artifact. Update the per-artifact rendering path to
derive preview from each artifact’s art.meta_json, while preserving the existing
fallback behavior when no artifact preview is available; remove or stop using
the item-level preview for those rendered artifacts.
In `@desktop/src/lib/library.ts`:
- Around line 86-98: Reuse the centralized fetchJson validation instead of
duplicating fetch, response checks, JSON parsing, and error handling: in
desktop/src/lib/library.ts lines 86-98, update getLibraryItem to call
fetchJson<LibraryItemDetail | null>(url, null); in desktop/src/lib/library.ts
lines 137-146, update the POST helper to call fetchJson with its method/body
init and null fallback. Leave deleteLibraryItem and reprocessLibraryItem
unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 31d60514-6f3e-4942-9e19-4d18ddfb6bbf
📒 Files selected for processing (3)
desktop/src/components/LibraryItemCard.test.tsxdesktop/src/components/LibraryItemCard.tsxdesktop/src/lib/library.ts
| {thumbnailArtifact ? ( | ||
| <img | ||
| src={thumbnailArtifact.path} | ||
| alt={`Thumbnail for ${item.title || "item"}`} | ||
| className="w-full aspect-video object-cover rounded-lg bg-shell-surface" | ||
| onError={(e) => { | ||
| const target = e.target as HTMLImageElement; | ||
| target.style.display = "none"; | ||
| }} | ||
| /> | ||
| ) : ( | ||
| <div className="flex flex-col items-center justify-center w-full aspect-video rounded-lg bg-shell-surface text-shell-text-tertiary"> | ||
| <ImageIcon size={20} /> | ||
| <span className="text-[10px] mt-1">No thumbnail</span> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Broken thumbnail fails silently.
When a thumbnail artifact exists but the image fails to load, onError just hides the <img> (Line 157), leaving blank space with no fallback. This contradicts the PR's "visible failure states for missing thumbnails" goal (and the "failure must be visible, never silent" note at Line 167). Consider swapping in the placeholder on error rather than hiding.
🤖 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 `@desktop/src/components/LibraryItemCard.tsx` around lines 150 - 165, Update
the thumbnail rendering in LibraryItemCard so an image load failure replaces the
failed <img> with the existing visible “No thumbnail” placeholder instead of
merely hiding it. Preserve the current placeholder for absent thumbnailArtifact
values and ensure the onError handling leaves no blank thumbnail area.
| export interface IngestLibraryOptions { | ||
| title?: string; | ||
| source?: string; | ||
| } | ||
|
|
||
| export async function ingestLibraryUrl( | ||
| url: string, | ||
| opts?: IngestLibraryOptions, | ||
| ): Promise<{ item_id: string; status: string } | null> { | ||
| try { | ||
| const res = await fetch("/api/library/ingest", { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json", Accept: "application/json" }, | ||
| body: JSON.stringify({ url, title: opts?.title ?? "" }), | ||
| }); | ||
| if (!res.ok) return null; | ||
| const ct = res.headers.get("content-type") ?? ""; | ||
| if (!ct.includes("application/json")) return null; | ||
| return await res.json(); | ||
| } catch { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
source option is declared but never sent.
IngestLibraryOptions.source (Line 130) is never included in the request body — Line 141 only serializes url and title. Either wire it into the payload or drop it from the interface to avoid a silently ignored option.
🔧 Proposed fix (if backend accepts a source field)
- body: JSON.stringify({ url, title: opts?.title ?? "" }),
+ body: JSON.stringify({ url, title: opts?.title ?? "", source: opts?.source ?? "" }),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export interface IngestLibraryOptions { | |
| title?: string; | |
| source?: string; | |
| } | |
| export async function ingestLibraryUrl( | |
| url: string, | |
| opts?: IngestLibraryOptions, | |
| ): Promise<{ item_id: string; status: string } | null> { | |
| try { | |
| const res = await fetch("/api/library/ingest", { | |
| method: "POST", | |
| headers: { "Content-Type": "application/json", Accept: "application/json" }, | |
| body: JSON.stringify({ url, title: opts?.title ?? "" }), | |
| }); | |
| if (!res.ok) return null; | |
| const ct = res.headers.get("content-type") ?? ""; | |
| if (!ct.includes("application/json")) return null; | |
| return await res.json(); | |
| } catch { | |
| return null; | |
| } | |
| } | |
| export interface IngestLibraryOptions { | |
| title?: string; | |
| source?: string; | |
| } | |
| export async function ingestLibraryUrl( | |
| url: string, | |
| opts?: IngestLibraryOptions, | |
| ): Promise<{ item_id: string; status: string } | null> { | |
| try { | |
| const res = await fetch("/api/library/ingest", { | |
| method: "POST", | |
| headers: { "Content-Type": "application/json", Accept: "application/json" }, | |
| body: JSON.stringify({ url, title: opts?.title ?? "", source: opts?.source ?? "" }), | |
| }); | |
| if (!res.ok) return null; | |
| const ct = res.headers.get("content-type") ?? ""; | |
| if (!ct.includes("application/json")) return null; | |
| return await res.json(); | |
| } catch { | |
| return null; | |
| } | |
| } |
🤖 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 `@desktop/src/lib/library.ts` around lines 128 - 150, Update ingestLibraryUrl
and IngestLibraryOptions so the declared source option is not silently ignored:
include opts?.source in the JSON request payload if the backend accepts it,
otherwise remove source from the interface. Keep the existing url and title
behavior unchanged.
|
nemotron-super review VERDICT: Blocking issues found
Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
Code Review Roast 🔥Verdict: 18 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)
🏆 Best part: The test coverage is genuinely impressive — 25 tests covering pending, processing, ready, and error states with proper fixtures. The component structure is clean and the "failure states always visible" philosophy is baked into the design. I'm almost impressed. 💀 Worst part: The thumbnail 📊 Overall: Like a beautifully wrapped gift box with a brick inside — the component looks great and tests are thorough, but the thumbnail path bug and silent error swallowing will cause real production pain. Fix the critical issues and this merges clean. Files Reviewed (3 files)
|
|
nemotron-ultra-kilo review I'll analyze the diff directly from your message. The diff shows two new files being added: Let me analyze the diff for correctness, security, testing, and style issues: Let me analyze the diff for correctness, security, testing, and style issues: Analysis of LibraryItemCard.tsxCorrectness Bugs
Security Issues
Missing/Weak Test Cases (LibraryItemCard.test.tsx)
Style Issues
Analysis of LibraryItemCard.tsxCorrectness Bugs
Security Issues
Missing/Weak Test Cases (LibraryItemCard.test.tsx)
Style Issues
Automated first-pass review by the nemotron-ultra-kilo lane. The lead still reviews before merge. |
|
nemotron-ultra-orB review Based on the diff provided in your message, here's my code review: VERDICT: Several correctness bugs, missing error handling, and test gaps
Style/Consistency:
Automated first-pass review by the nemotron-ultra-orB lane. The lead still reviews before merge. |
1 participant
Autonomous build of board card tsk-cpgdhh.
Add LibraryItemCard component per docs/design/library-app.md sections 2-4.
Card shows thumbnail (or placeholder), title, kind badge, media duration,
pipeline status per stage (jobs shape), artifact list (text, transcript,
description, ocr) with preview, link-to-collection action, and a disabled
Download stub until P3. Failure states are always visible -- no silent
empties for missing thumbnails, pipeline stages, artifacts, or errors.
Includes lib/library.ts with types and API client for the library store
(items, artifacts, jobs) and 25 component tests covering pending,
processing, ready, and error states.
Files:
desktop/src/components/LibraryItemCard.test.tsx | 397 ++++++++++++++++++++++++
desktop/src/components/LibraryItemCard.tsx | 267 ++++++++++++++++
desktop/src/lib/library.ts | 150 +++++++++
3 files changed, 814 insertions(+)
Summary by CodeRabbit
New Features
Tests