feat(ds): text labels and a sliding underline replace the pill tab track - #3166
feat(ds): text labels and a sliding underline replace the pill tab track#3166ElMagnea wants to merge 1 commit into
Conversation
|
|
Overall Grade |
Security Reliability Complexity Hygiene Coverage |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| JavaScript | Aug 21, 2026 8:20p.m. | Review ↗ | |
| Code coverage | Aug 21, 2026 8:20p.m. | Review ↗ |
Code Coverage Summary
| Language | Line Coverage (Overall) |
|---|---|
| Aggregate | 69.3% |
| Javascript | 69.3% |
➟ Additional coverage metrics may have been reported. See full coverage report ↗
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
There was a problem hiding this comment.
LGTM — the two nit-level findings (light-mode placeholder contrast, a layout-triggering width transition) are cosmetic/minor and don't block this.
Extended reasoning...
Overview
This PR replaces the pill-style tab track (TabView.svelte) with a text-label + sliding-underline design, applied uniformly across all 7 tab consumers (season/episode drawers, filter sidebar, import results, Plex settings, VIP usage tabs). Changes are: a rewritten TabView.svelte using a ResizeObserver-driven measurement of the active trigger to position the underline (with RTL handled via --rtl-sign), removal of the icon prop in favor of a new optional count field, and a token cleanup in modes.scss (several --color-tab-*, --color-filter-tab-*, and VIP tab tokens deleted, four new semantic tokens added to both theme mixins).
Security risks
None. This is presentational UI/CSS and component-composition code with no auth, data-access, or user-input handling involved.
Level of scrutiny
Moderate. The blast radius is wide (7 surfaces move together), but I verified the two things most likely to cause a build/runtime regression from a change like this: (1) none of the deleted CSS custom properties (--color-tablist-background, --color-tab-background, --color-filter-tab-*, VIP tab tokens) are still referenced anywhere in the codebase — the cleanup was complete; (2) the two TabView callers not included in the diff (PlexSettings.svelte, RawImport.svelte) don't pass the removed icon field, so there's no dangling prop usage that would fail type-checking. The underline-measurement logic (ResizeObserver + RTL offset math) is the one genuinely non-trivial piece, and it correctly disconnects the observer on cleanup and re-measures on tab-value changes.
Other factors
The two bugs already flagged inline are both explicitly minor: a light-mode contrast regression on an already-cosmetic placeholder tile, and a width transition that triggers layout but only on discrete, user-initiated tab switches (not a hot path). Neither affects functionality or breaks existing behavior. The PR description documents visual verification across dark/light/RTL and a clean svelte-check run, and no specs touch these files (so nothing to regress there).
| aspect-ratio: 2 / 3; | ||
| overflow: hidden; | ||
|
|
||
| background: var(--color-tablist-background); | ||
| background: var(--color-card-background); | ||
| border: var(--border-thickness-m) solid transparent; | ||
| border-radius: var(--border-radius-m); | ||
|
|
There was a problem hiding this comment.
🟡 In light mode, --color-card-background resolves to the same value as --color-background (both --shade-10), whereas the old --color-tablist-background was a distinct --shade-100. Since ImportComplete renders directly on the page background with no card wrapper, the .candidate-poster placeholder box (the always-imageless 'skip' tile, and any candidate before its poster loads) becomes invisible against the page in light mode. Purely cosmetic — text/tags stay legible and the tile stays clickable — but worth swapping to a token that's actually distinct from --color-background (e.g. a muted shade like the old tablist token).
Extended reasoning...
The bug: ImportComplete.svelte line 345 changes .candidate-poster's background from --color-tablist-background to --color-card-background. In light theme (modes.scss), --color-card-background resolves through --color-floating-background to var(--shade-10), which is identical to --color-background (also var(--shade-10)) — the page background this section renders against. The previous token, --color-tablist-background, resolved to var(--shade-100), a distinct grey that gave the placeholder box a visible boundary regardless of what it sat on.
Why nothing else masks it: ImportComplete is rendered via RawImport -> SettingsSection (no background) -> Settings.svelte's content area, which is explicitly background-color: transparent. So there's no intervening card surface to separate the tile from the page — unlike a typical card, which gets contrast from a shadow even when same-colored, .candidate-poster has border: var(--border-thickness-m) solid transparent (only becoming purple when .is-selected) and no box-shadow, so it relies entirely on the background-color diff to read as a box.
Where it shows up: the always-image-less .is-skip tile (renders only a floating 'Skip' tag with no image ever), any ambiguous candidate whose poster fails to load or is still lazy-loading, and briefly for every candidate during the image load window. In dark theme this is fine, since --color-card-background (--shade-930) differs from --color-background there.
Step-by-step proof:
- Open Settings → Data → Import in light theme, run an import that yields ambiguous matches.
ImportCompleterenders each ambiguous item's candidate grid; each.candidate-posterbox getsbackground: var(--color-card-background).- Resolve
--color-card-backgroundin light theme:--color-card-background: var(--color-floating-background)→--color-floating-background: var(--shade-10)→var(--shade-10). - Resolve the page background at this point in the tree:
Settings.sveltecontent is transparent,SettingsSectionhas no background, so the effective background is--color-background: var(--shade-10)(alsoshade-10in light theme). - Both resolve to the exact same
shade-10— the placeholder box has zero contrast against the page. The 'Skip' tile (which never gets an<img>) shows only its floating text with no visible box or tap-target boundary; other candidates lose the box outline until/unless their poster image covers it viaobject-fit: cover.
Fix: revert .candidate-poster's background to a token that's guaranteed distinct from --color-background in both themes — either restore --color-tablist-background-equivalent styling, or introduce a muted surface token (e.g. --shade-100-based) rather than --color-card-background, since the latter is only guaranteed distinct from the page when there's an intervening --color-background-styled wrapper (which doesn't exist here).
Severity rationale: this is a real, PR-introduced visual regression, but strictly cosmetic — the Skip/year tag text remains legible, the selected-state purple border still shows, and buttons remain clickable; posters that load simply cover the box. It's confined to the placeholder/no-image state in light mode only.
|
|
||
| .trakt-tab-content { | ||
| padding: 0 var(--ni-8); | ||
| .tab-underline { | ||
| position: absolute; | ||
| inset-block-end: calc(-1 * var(--tab-rule-thickness)); | ||
| inset-inline-start: 0; | ||
|
|
||
| width: var(--tab-underline-width); | ||
| height: var(--border-thickness-xs); | ||
| transform: translateX( | ||
| calc(var(--rtl-sign) * var(--tab-underline-offset)) | ||
| ); | ||
|
|
||
| background: var(--color-tab-indicator); | ||
| border-radius: var(--border-radius-xs); | ||
|
|
||
| /* Hidden until the first measurement lands, so it never flashes at 0. */ | ||
| opacity: 0; | ||
| pointer-events: none; | ||
|
|
||
| transition: | ||
| transform var(--transition-increment) ease-in-out, | ||
| width var(--transition-increment) ease-in-out, |
There was a problem hiding this comment.
🟡 The .tab-underline transition (TabView.svelte:197-200) animates width alongside transform/opacity, which per performance.md's animation rule triggers layout on every tab switch. Since the underline already uses translateX for position, swapping to clip-path: inset(0 X 0 0 round Y) (the doc's own recommended alternative for rounded fill bars) would keep the corners crisp while staying fully compositor-only.
Extended reasoning...
.tab-underline's transition list in TabView.svelte:197-200 includes width alongside transform and opacity:
transition:
transform var(--transition-increment) ease-in-out,
width var(--transition-increment) ease-in-out,
opacity var(--transition-increment) ease-in-out;.agents/rules/performance.md:20-23 states the general rule plainly: "Animate only transform and opacity. Other properties (top, left, width, height, margin, backdrop-filter) trigger layout or paint every frame." Every tab switch drives underline.width from its previous value to the new active tab's width over --transition-increment, so the browser recalculates layout for that element on each animation frame of the transition, rather than staying fully on the compositor thread the way transform/opacity-only animations do.
The same doc does carve out an exception for exactly this shape at lines 42-48: "For progress / fill bars, prefer width or clip-path: inset(...) over scaleX... width keeps corners shaped; clip-path: inset(0 X 0 0 round Y) is composited and keeps corners crisp." Since .tab-underline has border-radius: var(--border-radius-xs), scaleX is correctly avoided (it would distort those corners) — but the doc names two valid options for that case, and clip-path is the one that stays off the layout path entirely. Since transform: translateX(...) already handles the underline's position, the only remaining layout-triggering property is width, and it has a documented compositor-safe substitute sitting right next to it in the same rule.
Concretely: on switching from a short tab ("Info") to a longer one ("Episodes"), the underline animates from width: 40px to width: 96px over --transition-increment. Each frame of that transition forces the browser to recompute the box for .tab-underline (layout), then paint, then composite — instead of only compositing a clip-path inset change. Rewriting the underline as a full-rail-width element clipped via clip-path: inset(0 calc(100% - offset - width) 0 offset round var(--border-radius-xs)), animated by transitioning the clip-path value, would produce the same visual (sliding, correctly-rounded bar) while keeping the whole animation on the compositor.
That said, the practical impact here is small: the transition fires only on a discrete, user-initiated tab-switch click (not per-frame/scroll/keystroke), and .tab-underline is a position: absolute, childless leaf with no in-flow siblings, so the layout it triggers is contained to that one element rather than causing a broader reflow. This matches the doc's own "Honest tradeoffs" guidance against micro-optimizing cold, infrequent paths. Worth fixing to stay consistent with the documented rule and to get a marginally smoother slide, but not something that breaks functionality or degrades perceived performance meaningfully as shipped.
Every TabView surface moves at once: the season and episode drawers, the filter simple/advanced switch, Settings > Data import sources, the import match/skip split, Plex sync/webhook and VIP usage/history. An active tab is now a bold uppercase label with a purple underline. The underline measures the active trigger's box and slides to it, so labels no longer need equal grid columns; offsets are read from the rail's inline-start edge and replayed through --rtl-sign so the slide stays correct in RTL, and a ResizeObserver re-measures on font load, container resize and label changes. Tab icons come out of the two drawers - the label and its underline carry the state. The tab model gains an optional count rendered beside the label; nothing wires one yet, since the reviews count needs the comments query lifted out of InlineComments. Pill-era tokens are deleted: --color-tablist-background, --color-tab-background, --color-tab-hover-background, the three --color-filter-tab-* and the three VIP tab overrides. Both themes gain --color-tab-text, --color-tab-count, --color-tab-indicator and --color-tab-rule; light mode uses --shade-600 for inactive labels so the active one still reads as active. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1e34375 to
0b22321
Compare
Visuals
Filters
Seasons
Settings/Data
The Setup
Every tabbed surface in the app wears the same grey pill track: a recessed
--color-tablist-backgroundbar, equal-width columns, and a filled pill slidingbehind the active label. Three problems with it.
It is a lot of chrome for a two-word choice.
Simple/Advancedgets the same40px slab as five import sources. Equal columns mean
Infois padded out to thewidth of
Episodeswhether it needs it or not.And the pill fights whatever it sits on.
UsageTabshad to override fourtokens to keep the grey from clashing with the VIP card's purple glow;
FilterTabsoverrode three more plus the list geometry. Every surface thatwanted to look right had to re-skin the component.
This swaps the whole thing for type: bold uppercase labels on a hairline rule,
with a purple underline that slides to the active one.
What Changes
One component, so all seven surfaces move together:
Icons come out of the two drawers — the label and its underline carry the state
now, and the mockup this follows is text-only.
How It Works
The underline is measured, not gridded. Labels no longer share equal
columns, so there is no column arithmetic to position it with. The component
reads the active trigger's box against the rail and drives the underline from
two custom properties. Offsets are taken from the rail's inline-start edge and
replayed through
--rtl-sign, so the slide points the right way in RTL (checkedin
fa-IRdirection, not assumed). AResizeObserveron the triggersre-measures on font load, container resize and label changes, and the underline
stays at
opacity: 0until the first measurement lands so it never flashes atposition 0.
Spacing puts the slack on the far side of the label. The trigger keeps a
40px tap target, but its padding is asymmetric: the label sits ~8px from its
underline and the remaining height goes above it.
tabPosition="bottom"(themobile filter drawer) flips both the rule and the padding, so its labels hug
their underline the same way.
Tokens, not per-surface overrides. Deleted:
--color-tablist-background,--color-tab-background,--color-tab-hover-background, the three--color-filter-tab-*and the three VIP tab tokens — all dead once the pill isgone. Added to both themes:
--color-tab-text,--color-tab-count,--color-tab-indicator,--color-tab-rule. Light mode deliberately uses--shade-600for inactive labels rather than--color-text-secondary, which is--shade-800and sits so close to the active--shade-900that the two stateswere indistinguishable without the underline.
ImportCompleteborrowed the old tablist colour for its candidate-posterplaceholder; that now points at
--color-card-background.One thing to argue about: the tab model gained an optional
count(themockup showed
EPISODES 8), rendered beside the label throughtoHumanCount.No caller wires one — we tried it on the season drawer's Episodes tab and it
read as noise next to the
10 eps.heading right below it. Reviews is where acount would actually earn its place, and that needs the comments query lifted
out of
InlineComments. Happy to strip the field until then if you'd rather notcarry it.
Testing
deno fmtandsvelte-checkclean (9551 files, 0 errors, 0 warnings). The unitsuite passes on this code, though it was run from the main worktree — a fresh
worktree resolves a different
viteand every spec dies loading vitest'sreporter. No spec touches these files.
Visually verified in dark, light and RTL, plus hover and tab-switching, on the
real season and episode drawers and on the VIP glow card — the surface that
needed the most overrides before now needs none.