feat: redesign movies view [P19.06]#173
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis pull request refactors the movies page component and its supporting test suite. The 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: 1
🧹 Nitpick comments (5)
web/src/routes/movies/movies.test.ts (1)
136-139: Consider a more robust assertion for sorted headings.The test slices the first 2 headings to check sort order, but this implicitly depends on the "Add New" card being rendered last. If the page structure changes (e.g., "Add New" moves or additional headings are added), this test could become brittle.
Consider using a more explicit query or filtering by specific class/data attributes to isolate movie card headings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/routes/movies/movies.test.ts` around lines 136 - 139, The assertion is brittle because it slices the first two headings from screen.getAllByRole and assumes the "Add New" card's position; update the test to target only movie card headings by querying the movies container and then collecting headings within it (e.g., use the same test's container/query that renders the grid or a data-testid/class for movie cards), then assert their sorted order (reference: the headings variable and the screen.getAllByRole call and the "Add New" card text) so the test no longer depends on global heading order.web/src/routes/movies/+page.svelte (4)
342-358: Consider adding ARIA attributes for progress accessibility.The progress bar visually shows download progress but lacks ARIA attributes for screen reader users. Consider adding
role="progressbar"witharia-valuenow,aria-valuemin, andaria-valuemax.♿ Suggested improvement
<div class="h-2 overflow-hidden rounded-full bg-white/8"> <div class="bg-primary h-full rounded-full transition-[width]" style={`width: ${pct}%`} + role="progressbar" + aria-valuenow={pct} + aria-valuemin={0} + aria-valuemax={100} + aria-label="Download progress" ></div> </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/routes/movies/`+page.svelte around lines 342 - 358, The progress bar for the downloading state (inside the {`#if` status === 'downloading'} block using the pct variable and the inner div with class "bg-primary") needs ARIA attributes for accessibility: add role="progressbar" on the inner progress element (or the wrapper) and include aria-valuenow={+pct} (numeric), aria-valuemin="0", aria-valuemax="100" and an appropriate aria-label or aria-valuetext (e.g., `${pct}% acquired`) so screen readers can announce current progress; ensure the values reference the existing pct variable and do not pass a percentage string to aria-valuenow.
236-241: Consider adding explicit dimensions to prevent layout shift.The images use
loading="lazy"which is good for performance. However, without explicitwidthandheightattributes (or CSS aspect-ratio), the browser cannot reserve space before the image loads, potentially causing layout shift (CLS issues).The current CSS (
h-full w-full object-cover) relies on parent container dimensions, which may be sufficient if the container has fixed dimensions. Verify that the card layout doesn't shift when images load.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/routes/movies/`+page.svelte around lines 236 - 241, The image tag using src={backdropUrl} (the <img> with class="h-full w-full object-cover...") can cause layout shift because it relies on parent sizing and uses loading="lazy"; add explicit intrinsic dimensions or enforce an aspect ratio so the browser can reserve space before load. Fix by adding width and height attributes matching the expected image dimensions (or apply a CSS aspect-ratio on the img or its card container, e.g. aspect-ratio: 16/9) and ensure the parent card/container has a stable size; update the <img> element and its container styles (the same element with class "h-full w-full object-cover...") accordingly.
54-65: Review thewantedcategorization logic.The
commandStatusfunction categorizes movies as:
downloading: when actively downloadingmissing: whenlifecycleStatus === 'missing_from_transmission'orstatusisfailed/rejected/duplicatewanted: everything else (default fallback)Consider whether movies with
status: 'queued'orstatus: 'completed'should be categorized differently. Currently, completed movies would fall intowanted, which may not reflect user intent.If this is intentional (e.g., all non-downloading/non-missing movies are considered "wanted" in the command deck paradigm), the logic is fine. Otherwise, consider adding explicit handling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/routes/movies/`+page.svelte around lines 54 - 65, The current commandStatus(movie, live) defaults everything not downloading or flagged as missing to 'wanted', which inadvertently classifies movie.status === 'completed' (and maybe 'queued') as 'wanted'; update the commandStatus function to explicitly handle status values you care about (e.g., add explicit checks for movie.status === 'queued' and movie.status === 'completed') and return the correct CommandStatus for each (or add/choose a new CommandStatus if needed) instead of relying on the broad default; modify the conditional branch in commandStatus to check those statuses before returning 'wanted' so completed/queued movies are categorized as intended.
192-209: Tabs are missingaria-controlsand correspondingtabpanelrole.The tabs have
role="tab"andaria-selected, but for full accessibility compliance, each tab should havearia-controlspointing to the controlled panel, and the panel should haverole="tabpanel"witharia-labelledby.Since the filtered content is inline (not separate panels), this pattern may not apply directly. Consider either:
- Adding
idandaria-controls/aria-labelledbyattributes to connect tabs with the movie grid- Documenting that this is a filtering UI rather than a true tabbed interface
For filtering UIs, some teams use toggle buttons instead of tabs, but the current approach is functional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/routes/movies/`+page.svelte around lines 192 - 209, The tab buttons (role="tab") lack aria-controls and the movie grid lacks role="tabpanel"/aria-labelledby; give each button a stable id (e.g. "tab-{tab.key}") and add aria-controls pointing to a single panel id (e.g. "movies-panel") or per-tab panel id if you choose multiple panels, then on the movie grid container add role="tabpanel", id="movies-panel" and set aria-labelledby to the id of the currently selected tab (use activeTab to derive the labelledby id), or alternatively change role="tab" to a toggle button role and document that this is a filtering UI; update the button markup that references activeTab/tab.key and the movie grid container that renders the filtered movies (the element rendering the grid) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/routes/movies/`+page.svelte:
- Line 206: The span rendering the tab count (the element containing
tabCount(tab.key)) uses the unsupported Tailwind class text-inherit/80; update
that span to either replace text-inherit/80 with text-current/80 if inheriting
currentColor with opacity is acceptable, or keep text-inherit and add a separate
opacity-80 utility (e.g., use classes ml-1 text-[10px] text-inherit opacity-80)
so the intended 80% opacity is applied correctly.
---
Nitpick comments:
In `@web/src/routes/movies/`+page.svelte:
- Around line 342-358: The progress bar for the downloading state (inside the
{`#if` status === 'downloading'} block using the pct variable and the inner div
with class "bg-primary") needs ARIA attributes for accessibility: add
role="progressbar" on the inner progress element (or the wrapper) and include
aria-valuenow={+pct} (numeric), aria-valuemin="0", aria-valuemax="100" and an
appropriate aria-label or aria-valuetext (e.g., `${pct}% acquired`) so screen
readers can announce current progress; ensure the values reference the existing
pct variable and do not pass a percentage string to aria-valuenow.
- Around line 236-241: The image tag using src={backdropUrl} (the <img> with
class="h-full w-full object-cover...") can cause layout shift because it relies
on parent sizing and uses loading="lazy"; add explicit intrinsic dimensions or
enforce an aspect ratio so the browser can reserve space before load. Fix by
adding width and height attributes matching the expected image dimensions (or
apply a CSS aspect-ratio on the img or its card container, e.g. aspect-ratio:
16/9) and ensure the parent card/container has a stable size; update the <img>
element and its container styles (the same element with class "h-full w-full
object-cover...") accordingly.
- Around line 54-65: The current commandStatus(movie, live) defaults everything
not downloading or flagged as missing to 'wanted', which inadvertently
classifies movie.status === 'completed' (and maybe 'queued') as 'wanted'; update
the commandStatus function to explicitly handle status values you care about
(e.g., add explicit checks for movie.status === 'queued' and movie.status ===
'completed') and return the correct CommandStatus for each (or add/choose a new
CommandStatus if needed) instead of relying on the broad default; modify the
conditional branch in commandStatus to check those statuses before returning
'wanted' so completed/queued movies are categorized as intended.
- Around line 192-209: The tab buttons (role="tab") lack aria-controls and the
movie grid lacks role="tabpanel"/aria-labelledby; give each button a stable id
(e.g. "tab-{tab.key}") and add aria-controls pointing to a single panel id (e.g.
"movies-panel") or per-tab panel id if you choose multiple panels, then on the
movie grid container add role="tabpanel", id="movies-panel" and set
aria-labelledby to the id of the currently selected tab (use activeTab to derive
the labelledby id), or alternatively change role="tab" to a toggle button role
and document that this is a filtering UI; update the button markup that
references activeTab/tab.key and the movie grid container that renders the
filtered movies (the element rendering the grid) accordingly.
In `@web/src/routes/movies/movies.test.ts`:
- Around line 136-139: The assertion is brittle because it slices the first two
headings from screen.getAllByRole and assumes the "Add New" card's position;
update the test to target only movie card headings by querying the movies
container and then collecting headings within it (e.g., use the same test's
container/query that renders the grid or a data-testid/class for movie cards),
then assert their sorted order (reference: the headings variable and the
screen.getAllByRole call and the "Add New" card text) so the test no longer
depends on global heading order.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 58f846cc-1c8c-4ca2-bc42-028a8eb0b0a6
📒 Files selected for processing (4)
web/src/lib/components/StatusChip.svelteweb/src/lib/components/status-chip.test.tsweb/src/routes/movies/+page.svelteweb/src/routes/movies/movies.test.ts
|
1 participant
Summary
P19.06 Movies redesignagents/p19-05-tv-show-detail-redesigncleancompleted at 2026-04-17 01:28 UTCExternal AI Review
patchedb6d9cb9eaddcf85357756228b6d9cb9eaddcaddress all findings from that review.coderabbit,sonarqubeResolved Review Findings
text-inherit/80— not supported in Tailwind v4. (native GitHub thread resolved)web/src/routes/movies/+page.svelte:206thread