fix(editor): resolve usability and error-prevention bugs#196
Open
rlauuzo wants to merge 24 commits into
Open
Conversation
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
Reviewer's GuideThis PR hardens the segment editor and related UI by introducing robust dirty-state tracking with navigation/close guards, confirmation dialogs for destructive actions, a shared SegmentTypeMenu, improved empty/connecting states, and by fixing header routing and labels, with tests added around the new behavior. Sequence diagram for dirty-state navigation blocking in the segment editorsequenceDiagram
actor User
participant PlayerEditor
participant segment_utils
participant useBlocker
participant AlertDialog
participant Router
User->>PlayerEditor: edit segments
PlayerEditor->>segment_utils: areSegmentListsEqual(localEditingSegments, sortedServerSegments)
segment_utils-->>PlayerEditor: false (lists differ)
PlayerEditor->>useBlocker: shouldBlockFn() => isDirty
useBlocker-->>PlayerEditor: status blocked
PlayerEditor->>AlertDialog: open unsaved changes dialog
alt User cancels navigation
User->>AlertDialog: click Cancel
AlertDialog->>useBlocker: reset()
useBlocker-->>Router: navigation cancelled
else User discards and leaves
User->>AlertDialog: click Discard & leave
AlertDialog->>useBlocker: proceed()
useBlocker-->>Router: continue navigation
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
FilterView, the error message rendering now usesshowError.message || …instead of the previous optional chaining; this will throw whenshowErroris undefined, so consider restoring the null-safe access (e.g.showError?.message ?? …) to avoid runtime errors when no error is present.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `FilterView`, the error message rendering now uses `showError.message || …` instead of the previous optional chaining; this will throw when `showError` is undefined, so consider restoring the null-safe access (e.g. `showError?.message ?? …`) to avoid runtime errors when no error is present.
## Individual Comments
### Comment 1
<location path="src/__tests__/header-title.test.tsx" line_range="115-88" />
<code_context>
+ render(<RouterProvider router={router as never} />)
+}
+
+describe('Header on detail routes', () => {
+ afterEach(() => {
+ cleanup()
+ })
+
+ it('renders the movie name as the title on a player route', async () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Add header tests for non-player detail routes and missing/disabled item cases
These tests validate the new `useParams({ strict: false })` behavior for `/player/$itemId` and `/series/$itemId`. To more fully cover the routing/title logic and guard against regressions, please also add:
- A case where `itemId` is absent (e.g. `/`) to ensure the header renders a safe default and doesn’t crash.
- A case where `useItem` is disabled or the item is missing from `itemsById`, verifying the title and EpisodeSwitcher fail gracefully.
- If other detail routes also rely on `itemId` (e.g. `/album/$itemId`, `/artist/$itemId`), a test for at least one of those to confirm parity with the old `useMatchRoute` behavior.
Suggested implementation:
```typescript
it('renders the episode switcher with the episode label on a player route', async () => {
renderHeaderAt('/player/episode-1')
const switcher = await screen.findByTestId('episode-switcher')
expect(switcher).toBeInTheDocument()
})
it('renders a safe default title when no itemId param is present', async () => {
renderHeaderAt('/')
// When there is no detail route param, the header should still render
const heading = await screen.findByRole('heading', { level: 1 })
expect(heading).toBeInTheDocument()
})
it('falls back gracefully when the item is missing from itemsById', async () => {
// Simulate a detail route with an itemId that does not exist
renderHeaderAt('/player/nonexistent-item-id')
const heading = await screen.findByRole('heading', { level: 1 })
expect(heading).toBeInTheDocument()
// EpisodeSwitcher should not render when there is no valid episode/item
const switcher = screen.queryByTestId('episode-switcher')
expect(switcher).toBeNull()
})
it('renders a title on a series detail route', async () => {
// Series detail routes also rely on itemId; verify parity with player routes
renderHeaderAt('/series/episode-1')
const heading = await screen.findByRole('heading', { level: 1 })
expect(heading).toBeInTheDocument()
})
```
If your header implementation renders a specific fallback title when no `itemId` is present or when the item is missing (e.g. `"Discover"` or `"Unknown item"`), you can strengthen these tests by asserting `heading.textContent` equals that string instead of only checking `toBeInTheDocument()`.
Similarly, if your series detail route uses a known fixture (e.g. `/series/series-1` with a specific title), update the path and expectation in the `"renders a title on a series detail route"` test to match those real values.
If there are additional detail routes such as `/album/$itemId` or `/artist/$itemId`, you can duplicate the last test and adjust the route and expectations to cover them as well for full parity with the old `useMatchRoute` behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| <Outlet /> | ||
| </> | ||
| ), | ||
| }) |
There was a problem hiding this comment.
suggestion (testing): Add header tests for non-player detail routes and missing/disabled item cases
These tests validate the new useParams({ strict: false }) behavior for /player/$itemId and /series/$itemId. To more fully cover the routing/title logic and guard against regressions, please also add:
- A case where
itemIdis absent (e.g./) to ensure the header renders a safe default and doesn’t crash. - A case where
useItemis disabled or the item is missing fromitemsById, verifying the title and EpisodeSwitcher fail gracefully. - If other detail routes also rely on
itemId(e.g./album/$itemId,/artist/$itemId), a test for at least one of those to confirm parity with the olduseMatchRoutebehavior.
Suggested implementation:
it('renders the episode switcher with the episode label on a player route', async () => {
renderHeaderAt('/player/episode-1')
const switcher = await screen.findByTestId('episode-switcher')
expect(switcher).toBeInTheDocument()
})
it('renders a safe default title when no itemId param is present', async () => {
renderHeaderAt('/')
// When there is no detail route param, the header should still render
const heading = await screen.findByRole('heading', { level: 1 })
expect(heading).toBeInTheDocument()
})
it('falls back gracefully when the item is missing from itemsById', async () => {
// Simulate a detail route with an itemId that does not exist
renderHeaderAt('/player/nonexistent-item-id')
const heading = await screen.findByRole('heading', { level: 1 })
expect(heading).toBeInTheDocument()
// EpisodeSwitcher should not render when there is no valid episode/item
const switcher = screen.queryByTestId('episode-switcher')
expect(switcher).toBeNull()
})
it('renders a title on a series detail route', async () => {
// Series detail routes also rely on itemId; verify parity with player routes
renderHeaderAt('/series/episode-1')
const heading = await screen.findByRole('heading', { level: 1 })
expect(heading).toBeInTheDocument()
})If your header implementation renders a specific fallback title when no itemId is present or when the item is missing (e.g. "Discover" or "Unknown item"), you can strengthen these tests by asserting heading.textContent equals that string instead of only checking toBeInTheDocument().
Similarly, if your series detail route uses a known fixture (e.g. /series/series-1 with a specific title), update the path and expectation in the "renders a title on a series detail route" test to match those real values.
If there are additional detail routes such as /album/$itemId or /artist/$itemId, you can duplicate the last test and adjust the route and expectations to cover them as well for full parity with the old useMatchRoute behavior.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the segment-editor’s safety and usability by tightening dirty-state detection, adding user confirmations/guards to prevent accidental data loss, and consolidating segment-creation UI while fixing a few routing/UX inconsistencies.
Changes:
- Add editor safety features (structural dirty check, disable Save when clean, discard/reset actions, navigation/unload blocking, deletion confirmations).
- Consolidate/standardize segment creation UI via a shared
SegmentTypeMenuand improve empty-state/actions visibility. - Fix supporting UX/routing behaviors (header param resolution, FilterView connecting/error messaging, SettingsSelect trigger labels) and add tests for key regressions.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/routes/-root-components.tsx | Latches wizard open state so it remains mounted through success. |
| src/lib/segment-utils.ts | Adds areSegmentListsEqual helper for dirty-state detection. |
| src/i18n/locales/en-US.json | Adds editor empty/unsaved strings and common.cancel. |
| src/i18n/locales/de.json | Adds editor empty/unsaved strings and common.cancel. |
| src/i18n/locales/fr.json | Adds editor empty/unsaved strings and common.cancel. |
| src/components/settings/primitives/SettingsSelect.tsx | Passes items/options so closed select trigger can show labels. |
| src/components/segment/SegmentTypeMenu.tsx | New shared dropdown for selecting segment type when creating segments. |
| src/components/segment/SegmentSlider.tsx | Removes decorative grip; keeps actions visible (de-emphasized) until hover/focus. |
| src/components/player/PlayerEditor.tsx | Implements dirty state, save/discard affordances, navigation blocking, delete confirm, richer empty state, and uses SegmentTypeMenu. |
| src/components/player/PlayerControls.tsx | Uses shared SegmentTypeMenu for segment creation control. |
| src/components/player/Player.tsx | Resolves skip overlay segment against current segments so edits reflect immediately. |
| src/components/Header.tsx | Uses useParams({ strict: false }) to resolve itemId across detail/player routes. |
| src/components/filter/FilterView.tsx | Shows connecting state in standalone-with-credentials and improves error message fallback. |
| src/tests/settings-select.test.tsx | Tests SettingsSelect shows selected option label (not raw value). |
| src/tests/segment-list-equality.test.ts | Unit tests for areSegmentListsEqual. |
| src/tests/header-title.test.tsx | Router-based header title/episode switcher tests for detail routes. |
| src/tests/filter-view.test.tsx | Adds coverage for standalone connecting state with stored credentials. |
| src/tests/media-item-label.test.ts | Formatting/refactor only; no functional change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+752
to
+756
| <AlertDialogAction | ||
| variant="destructive" | ||
| onClick={() => blocker.proceed?.()} | ||
| > | ||
| {t('editor.discardAndLeave', 'Discard & leave')} |
Comment on lines
+18
to
+31
| export const areSegmentListsEqual = ( | ||
| a: ReadonlyArray<MediaSegmentDto>, | ||
| b: ReadonlyArray<MediaSegmentDto>, | ||
| ): boolean => | ||
| a.length === b.length && | ||
| a.every((segment, index) => { | ||
| const other = b[index] | ||
| return ( | ||
| segment.Id === other.Id && | ||
| segment.Type === other.Type && | ||
| segment.StartTicks === other.StartTicks && | ||
| segment.EndTicks === other.EndTicks | ||
| ) | ||
| }) |
Comment on lines
+344
to
+346
| const handleConfirmDeleteSegment = () => { | ||
| if (pendingDeleteIndex !== null) handleDeleteSegment(pendingDeleteIndex) | ||
| setPendingDeleteIndex(null) |
Contributor
Code Review SummaryStatus: No Issues Found | Recommendation: Merge The incremental change adds a recovery mechanism in Files Reviewed (1 file)
|
… title tests Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
…it tests Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
…te on item change, delete by segment id, harden areSegmentListsEqual Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
|
React Doctor found no new issues. 🎉 Reviewed by React Doctor for commit |
…izard latch, deterministic list keys Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
…explicit key cannot be overwritten Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
…ize media card surfaces, and re (#197) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
…s are full-bleed Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
* Contain vibrant color usage to accent layers in detail views, neutralize media card surfaces, and re Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * Keep overlay rounding embedded-only so fullscreen error/loading states are full-bleed Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * Address review: drop color-mix from AmbientGradient, cover windowed vs fullscreen surface styling in tests Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --------- Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
…-insensitive segment equality, dead-end connecting state for failed stored credentials Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
…h save finish since aborting between its delete and recreate phases would wipe remote segments Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
…ch to one state machine, inline validation-state indirection, drop unused preload export Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
… so the key cannot be overwritten Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
…rtial credential matches as non-mock in dev auto-login Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
… disabling monochrome always restores artwork colors Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
…k-seeded SchemeContent tokens drive existing shadcn theme vars on detail pages Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| </div> | ||
| <p className="text-destructive text-center text-lg"> | ||
| {showError.message} | ||
| {showError.message || |
Contributor
There was a problem hiding this comment.
CRITICAL: Runtime error - showError.message accessed without null check
showError is derived from collectionsError || itemsError and can be undefined. Accessing .message on undefined will throw at runtime when no error is present.
Suggested change
| {showError.message || | |
| {showError?.message || |
Contributor
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Overview
Issue Details (click to expand)Resolved Issues
Files Reviewed (9 files)
Fix these issues in Kilo Cloud Previous Review Summaries (2 snapshots, latest commit f6621d0)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit f6621d0)Status: No Issues Found | Recommendation: Merge Files Reviewed (13 files)
Changes SummaryThis PR successfully refactors the color extraction system from
All previous review issues have been resolved:
Previous review (commit 2a89473)Status: 1 Critical Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Files Reviewed (17 files)
Reviewed by laguna-xs-2.1-20260625 · Input: 324.9K · Output: 26.6K · Cached: 10.8M |
…tion The M3 dynamic scheme only needs a single seed color, so the node-vibrant palette pipeline and culori color math are redundant. useArtworkColor downsamples artwork on a shared canvas and quantizes it with MCU's QuantizerCelebi + Score (the Material You source-color pipeline), feeding DynamicThemeScope a seed hex directly. Removes node-vibrant, culori, and @types/culori. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
…ine titles, season/episode counts Drop the global radius scale one tier (--radius 12px -> 8px, with 2xl-4xl overrides derived from it) so every surface sharpens consistently. Media card titles truncate to a single line, and series cards show the season count next to the year (episode count for single-season series). The library browse query now requests ChildCount/RecursiveItemCount, and the mock server exposes matching fixture counts. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
| </div> | ||
| <p className="text-destructive text-center text-lg"> | ||
| {showError.message} | ||
| {showError.message || |
Contributor
There was a problem hiding this comment.
SUGGESTION: Use optional chaining for safer property access
Using showError?.message would be more robust than showError.message since it handles the case where showError might not have a message property. The || fallback already handles undefined values, but optional chaining provides an extra layer of safety and makes the intent clearer.
Suggested change
| {showError.message || | |
| {showError?.message || | |
| t('items.loadError', { | |
| defaultValue: 'Unable to load media items', | |
| })} |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Contributor
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Overview
Files Reviewed (3 files)
Previous Review Summaries (5 snapshots, latest commit f0acb80)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit f0acb80)Status: No Issues Found | Recommendation: Merge Overview
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit 1574ee2)Status: No Issues Found | Recommendation: Merge Files Reviewed (10 files)
Fix these issues in Kilo Cloud Previous review (commit d86daf3)Status: No Issues Found | Recommendation: Merge Overview
Issue Details (click to expand)Resolved Issues
Files Reviewed (9 files)
Fix these issues in Kilo Cloud Previous review (commit f6621d0)Status: No Issues Found | Recommendation: Merge Files Reviewed (13 files)
Changes SummaryThis PR successfully refactors the color extraction system from
All previous review issues have been resolved:
Previous review (commit 2a89473)Status: 1 Critical Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Files Reviewed (17 files)
Reviewed by laguna-xs-2.1-20260625 · Input: 419.9K · Output: 18.5K · Cached: 3.4M |
The header already resolves the current detail item for its title, so it derives the same artwork seed as the page below it (shared caches make this free) and wraps its chrome in DynamicThemeScope. Back/home/settings buttons pick up secondary-container tones with proper on-container icon color, and the header surface takes the scheme's tonal tint instead of staying neutral over a themed page. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
The base tokens were hand-picked oklch values (grey surfaces, orange primary, teal secondary) unrelated to the artwork-derived dynamic schemes. tools/generate-base-theme.mjs now feeds the existing brand orange through the same SchemeContent pipeline as m3-dynamic-theme.ts and emits the full light/dark token set, so the whole app shares one M3 tonal system: warm tinted surfaces, proper container/on-container pairs, and a real error color in dark mode (destructive was teal). Monochrome mode now also neutralizes the tinted surface tokens so it stays truly grey. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes 11 live-verified usability and error-prevention bugs across the segment-editor:
Header & Routing
useMatchRoutecalls withuseParams({ strict: false })so detail/player pages resolveitemIdand render correct titles/EpisodeSwitcherEditor State & Safety
isDirtyvia newareSegmentListsEqualhelper (unit tests included)useBlockerwith AlertDialog (Cancel/Discard & leave)enableBeforeUnloadguard for browser close/reloadConsistency & UX
itemsto SettingsSelect so closed triggers display labels (not raw values)SegmentTypeMenu(colored dots, t('segmentType.*')) for all new-segment entry pointsInternationalization
Summary by Sourcery
Improve the segment editor’s safety, routing correctness, and UX consistency across header, player, and filter views.
Bug Fixes:
Enhancements:
Tests: