desktop app: reactivity of red markers when trimming + shortcuts even if elements are focused #1331
Conversation
WalkthroughReplaced Player's document.body focus guard with a null-safe check of document.activeElement that disables shortcuts when focus is an input/textarea/contenteditable; refactored ClipTrack to compute markerValue via memo and pass value() to CutOffsetButton, and switched CutOffsetButton content rendering to a Show wrapper. Changes
Sequence Diagram(s)Player keyboard shortcut guard (high-level) sequenceDiagram
participant User
participant Document
participant Player
User->>Document: key press
Document->>Player: dispatch event
Player->>Player: read document.activeElement (null-safe)
alt activeElement is input/textarea/contenteditable
Player-->>User: ignore shortcut
else not editable or null
Player->>Player: execute shortcut handler (play/crop/split)
end
ClipTrack marker -> memo -> CutOffsetButton flow sequenceDiagram
participant TimelineUI
participant MarkerSource
participant createMemo
participant CutOffsetButton
TimelineUI->>MarkerSource: get marker
MarkerSource-->>createMemo: marker data
createMemo-->>TimelineUI: markerValue()
TimelineUI->>CutOffsetButton: pass value() (markerValue)
CutOffsetButton->>CutOffsetButton: Show chooses content (formatted time vs icon)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
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: 0
🧹 Nitpick comments (1)
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx (1)
312-339: Consider extracting the repeated marker value pattern.The same pattern appears three times for computing marker values:
{(markerValue) => { const value = createMemo(() => { const m = markerValue(); return m.type === "time" ? m.time : 0; }); // ... render with value() }}While each case has different positioning and onClick handlers, extracting the value computation logic into a shared helper could reduce duplication and improve maintainability.
Example approach:
function useMarkerValue(markerValue: Accessor<MarkerType>) { return createMemo(() => { const m = markerValue(); return m.type === "time" ? m.time : 0; }); }Then use it as:
{(markerValue) => { const value = useMarkerValue(markerValue); // ... render with value() }}Also applies to: 352-375, 691-722
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/routes/editor/Player.tsx(1 hunks)apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.
Files:
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsxapps/desktop/src/routes/editor/Player.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsxapps/desktop/src/routes/editor/Player.tsx
🧬 Code graph analysis (1)
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx (1)
apps/desktop/src/routes/editor/utils.ts (1)
formatTime(1-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (3)
apps/desktop/src/routes/editor/Player.tsx (1)
150-156: LGTM! Robust keyboard shortcut guard.The enhanced guard properly prevents shortcuts when focus is on input fields, textareas, or contenteditable elements, while allowing them to work when other UI elements (like buttons or options) are focused. This aligns well with the PR objective of ensuring shortcuts work even when editing options in the editor.
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx (2)
312-339: Improved reactivity with memoized marker values.The refactoring from direct callback rendering to memoized values (
markerValue→createMemo) properly addresses the PR objective of fixing red marker reactivity when trimming. The memoization ensures the UI updates correctly without requiring reloads.
781-786: LGTM! More idiomatic SolidJS rendering.Replacing the inline ternary with the
Showcomponent is a good refactor that aligns with SolidJS best practices for conditional rendering.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/desktop/src/routes/editor/Player.tsx (2)
27-75: Consider extracting caption initialization logic.The onMount callback is 48 lines with nested conditionals. Extracting this to a separate function (e.g.,
initializeCaptions) would improve readability and testability.Example structure:
async function initializeCaptions( editorInstance, project, setProject ) { if (!editorInstance?.path) return; await captionsStore.loadCaptions(editorInstance.path); if (!project) return; // ... rest of synchronization logic } onMount(() => initializeCaptions(editorInstance, project, setProject));
95-98: Consider extracting the end-detection tolerance as a named constant.The
0.1second tolerance is a magic number that could benefit from being a named constant (e.g.,END_TOLERANCE_SECONDS) for improved clarity and maintainability.const END_TOLERANCE_SECONDS = 0.1; const isAtEnd = () => { const total = totalDuration(); return total > 0 && total - editorState.playbackTime <= END_TOLERANCE_SECONDS; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/routes/editor/Player.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.
Files:
apps/desktop/src/routes/editor/Player.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/desktop/src/routes/editor/Player.tsx
🧬 Code graph analysis (1)
apps/desktop/src/routes/editor/Player.tsx (4)
apps/desktop/src/utils/tauri.ts (1)
commands(7-287)apps/desktop/src/routes/editor/context.ts (2)
FPS(46-46)OUTPUT_SIZE(48-51)apps/desktop/src/routes/editor/useEditorShortcuts.ts (1)
useEditorShortcuts(32-53)apps/desktop/src/routes/editor/utils.ts (1)
formatTime(1-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (6)
apps/desktop/src/routes/editor/Player.tsx (6)
16-25: LGTM! Clear dependency declaration.Explicitly destructuring the editor context at the top makes the component's dependencies transparent and easier to track.
148-159: Excellent improvement to keyboard shortcut guarding!The activeElement-based guard properly prevents shortcuts from firing when users are typing in inputs, textareas, or contenteditable elements. This directly addresses the PR objective of ensuring shortcuts work correctly when editing options.
The null-check for
activeElementis a good defensive measure.
124-146: LGTM! Robust playback control logic.The play/pause handler properly manages state transitions, includes restart-from-end logic, and has appropriate error handling with state cleanup.
77-89: LGTM! Proper caption synchronization.The effect correctly updates the current caption during playback with appropriate guards for valid time and segment availability.
340-349: LGTM! Clean checkerboard pattern implementation.The gridStyle object provides a clean transparent checkerboard pattern for the canvas background.
431-437: LGTM! Clean time formatting component.Simple, focused component that properly formats time display using the shared formatTime utility.
| const size = () => { | ||
| if (frameAspect() < containerAspect()) { | ||
| const height = (containerBounds.height ?? 0) - padding * 1; | ||
|
|
||
| return { | ||
| width: height * frameAspect(), | ||
| height, | ||
| }; | ||
| } | ||
|
|
||
| const width = (containerBounds.width ?? 0) - padding * 2; | ||
|
|
||
| return { | ||
| width, | ||
| height: width / frameAspect(), | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Inconsistent padding calculation may cause display issues.
Line 393 uses padding * 1 for height calculation, while Line 401 uses padding * 2 for width calculation. This asymmetry could lead to incorrect canvas sizing or aspect ratio.
Additionally, Line 413 subtracts padding * 2 from the width again, suggesting the height calculation should also account for padding symmetrically.
Apply this diff to maintain consistent padding:
- const height = (containerBounds.height ?? 0) - padding * 1;
+ const height = (containerBounds.height ?? 0) - padding * 2;📝 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.
| const size = () => { | |
| if (frameAspect() < containerAspect()) { | |
| const height = (containerBounds.height ?? 0) - padding * 1; | |
| return { | |
| width: height * frameAspect(), | |
| height, | |
| }; | |
| } | |
| const width = (containerBounds.width ?? 0) - padding * 2; | |
| return { | |
| width, | |
| height: width / frameAspect(), | |
| }; | |
| }; | |
| const size = () => { | |
| if (frameAspect() < containerAspect()) { | |
| const height = (containerBounds.height ?? 0) - padding * 2; | |
| return { | |
| width: height * frameAspect(), | |
| height, | |
| }; | |
| } | |
| const width = (containerBounds.width ?? 0) - padding * 2; | |
| return { | |
| width, | |
| height: width / frameAspect(), | |
| }; | |
| }; |
🤖 Prompt for AI Agents
In apps/desktop/src/routes/editor/Player.tsx around lines 391 to 407, the height
branch uses padding * 1 while the width branch and later width adjustments use
padding * 2, causing asymmetric sizing; change the height calculation to
subtract padding * 2 (i.e., use the same padding deduction as the width branch)
so both width and height account for symmetric horizontal padding, and ensure
any later width/height adjustments remain consistent with this padding
convention.
This PR:
Summary by CodeRabbit
Bug Fixes
Performance
Refactor