new: editor shortcuts #936
Conversation
|
Warning Rate limit exceeded@ameer2468 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 35 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughAdds OS-aware keyboard shortcut hints to Tooltip and surfaces Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Tooltip as Tooltip
participant OS as ostype()
participant UI as Renderer
User->>Tooltip: Hover/Focus
Tooltip->>OS: ostype()
OS-->>Tooltip: os string
Tooltip->>Tooltip: map kbd[] → symbols (⌘/ctrl, ⌥/⎇, ⇧)
Tooltip->>UI: render content + <kbd> hints
UI-->>User: Tooltip shown
sequenceDiagram
autonumber
actor User
participant Document as document
participant Aspect as AspectRatioSelect
participant Player as Player
participant Transform as TransformController
User->>Document: press "A"
Document->>Aspect: toggle open
User->>Document: press "S"
Document->>Player: toggle split/seek
User->>Document: press "Ctrl/Cmd" "+" or "-"
Document->>Player: zoom shortcut
Player->>Transform: updateZoom(...)
User->>Document: press "C"
Document->>Player: invoke cropDialogHandler
Player->>Transform: derive crop rect & open dialog
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (9)
apps/desktop/src/routes/editor/Timeline/LayoutTrack.tsx (1)
472-476: Purple hover gradient looks good; consider aligning ring color and tokensThe new hover-time SegmentContent gradient reads well. Minor consistency nit:
- This overlay uses a purple gradient while SegmentRoot’s inner ring still uses blue (“ring-blue-300” above, and “ring-blue-5” in the main segment at Line 293). Consider switching the ring to a neutral token (e.g., ring-gray-5) or a purple token to avoid mixed accent cues. Also consider replacing raw hexes with design tokens for easier theming.
Example tweak:
- innerClass="ring-blue-300" + innerClass="ring-gray-5" # or a purple token if availablepackages/ui-solid/src/Button.tsx (1)
17-17: Secondary hover state is visually identical to base; adjust hover colorSecondary variant sets base bg to gray-5 and hover to gray-5 as well. On light mode this yields no visual feedback. Recommend a slight delta for hover, and aligning text token with the rest of the app (many places use text-gray-12/11 over numeric -500).
- "dark:bg-gray-4 bg-gray-5 enabled:hover:bg-gray-5 text-gray-500 disabled:bg-gray-6 disabled:text-gray-9 outline-blue-300 disabled:outline-blue-10", + "dark:bg-gray-4 bg-gray-5 enabled:hover:bg-gray-6 dark:enabled:hover:bg-gray-5 text-gray-12 disabled:bg-gray-6 disabled:text-gray-9 outline-blue-300 disabled:outline-blue-10",apps/desktop/src/routes/editor/ExportDialog.tsx (2)
477-499: Deduplicate repeated panel container classesMultiple panels repeat “p-4 rounded-xl dark:bg-gray-2 bg-gray-3”. Consider extracting a reusable panel class or a small component to ensure consistency and simplify future theme adjustments.
Example:
+ const panelClass = "p-4 rounded-xl dark:bg-gray-2 bg-gray-3"; ... - <div class="flex-1 p-4 rounded-xl dark:bg-gray-2 bg-gray-3"> + <div class={cx("flex-1", panelClass)}>Also applies to: 500-553, 556-616, 618-644, 646-678
588-602: KSelect trigger theming: check foreground tokensTrigger bg uses dark:bg-gray-3 bg-gray-4, but Value/Icon still use text-[--gray-500]. If the rest of the dialog standardizes on numeric tokens (text-gray-12/11), consider switching for consistency or ensure --gray-500 maps correctly in both themes.
- class="flex-1 text-sm text-left truncate tabular-nums text-[--gray-500]" + class="flex-1 text-sm text-left truncate tabular-nums text-gray-12"apps/desktop/src/routes/editor/AspectRatioSelect.tsx (3)
20-25: Global “A” shortcut: solid guard; consider meta key to avoid accidental togglesListening for KeyA on document and gating on e.target === document.body avoids conflicts with inputs. Consider also requiring a modifier (e.g., Alt/Shift) or providing a user setting to reduce accidental opens while navigating.
- if (e.code === "KeyA" && e.target === document.body) { + if (e.code === "KeyA" && e.target === document.body && (e.altKey || e.shiftKey)) {
19-19: Unused ref variable: remove or usetriggerSelect is defined and passed to ref, but never read. If you don’t programmatically focus/position the trigger, drop it to avoid linter noise. If you plan to focus on open, wire it up.
- let triggerSelect: HTMLDivElement | undefined; ... - ref={triggerSelect} + // remove both lines if not usedAlso applies to: 32-32
29-37: Controlled KSelect: defaultValue is redundantYou’re driving the component with open/value/onOpenChange/onChange. defaultValue is unnecessary (and in some libraries can cause controlled/uncontrolled warnings). Remove it.
- defaultValue="auto"Also applies to: 38-38
apps/desktop/src/routes/editor/Player.tsx (2)
148-159: Consolidate global keydown listeners and relax the target checkYou’re attaching four document-level listeners with a strict
e.target === document.bodycheck. This breaks shortcuts when focus is on non-body focusables (canvas, buttons) and scatters logic. Prefer a single handler, ignore inputs/contentEditable, and switch oncode/modifiers.-//split shortcut -createEventListener(document, "keydown", (e: KeyboardEvent) => { - if (e.code === "KeyS" && e.target === document.body) { - e.preventDefault(); - setEditorState( - "timeline", - "interactMode", - editorState.timeline.interactMode === "split" ? "seek" : "split", - ); - } -}); - -//zoom-in shortct -createEventListener(document, "keydown", (e: KeyboardEvent) => { - if ( - (e.metaKey || e.ctrlKey) && - e.code === "Equal" && - e.target === document.body - ) { - e.preventDefault(); - editorState.timeline.transform.updateZoom( - editorState.timeline.transform.zoom / 1.1, - editorState.playbackTime, - ); - } -}); - -//zoom-out shortcut -createEventListener(document, "keydown", (e: KeyboardEvent) => { - if ( - (e.metaKey || e.ctrlKey) && - e.code === "Minus" && - e.target === document.body - ) { - e.preventDefault(); - editorState.timeline.transform.updateZoom( - editorState.timeline.transform.zoom * 1.1, - editorState.playbackTime, - ); - } -}); - -//crop shortcut -createEventListener(document, "keydown", async (e: KeyboardEvent) => { - if (e.code === "KeyC" && e.target === document.body) { - e.preventDefault(); - cropDialogHandler(); - } -}); +// Global editor shortcuts +createEventListener(document, "keydown", (e: KeyboardEvent) => { + const target = e.target as HTMLElement | null; + const tag = target?.tagName?.toLowerCase(); + const editable = + target?.isContentEditable || + tag === "input" || + tag === "textarea" || + tag === "select"; + if (editable) return; + + switch (true) { + // Toggle split mode (S) + case e.code === "KeyS": + e.preventDefault(); + setEditorState( + "timeline", + "interactMode", + editorState.timeline.interactMode === "split" ? "seek" : "split", + ); + return; + // Open crop dialog (C) + case e.code === "KeyC": + e.preventDefault(); + cropDialogHandler(); + return; + // Zoom in/out (Ctrl/Cmd + +/-) + case (e.metaKey || e.ctrlKey) && (e.code === "Equal" || e.code === "Minus"): + e.preventDefault(); + const factor = e.code === "Equal" ? 1 / 1.1 : 1.1; + editorState.timeline.transform.updateZoom( + editorState.timeline.transform.zoom * factor, + editorState.playbackTime, + ); + return; + } +});Note: if
updateZoomdoesn’t internally clamp, consider clamping to yourzoomOutLimit()and a min zoom.Please verify that Ctrl/Cmd + +/- doesn’t trigger WebView page zoom on Windows/macOS, especially on non-body focus states. If it does, we may need additional prevention at the app level.
Also applies to: 160-174, 175-189, 190-197
160-161: Nit: typo in comment“shortct” → “shortcut”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
apps/desktop/src/components/Tooltip.tsx(1 hunks)apps/desktop/src/routes/editor/AspectRatioSelect.tsx(2 hunks)apps/desktop/src/routes/editor/ExportDialog.tsx(9 hunks)apps/desktop/src/routes/editor/Player.tsx(7 hunks)apps/desktop/src/routes/editor/Timeline/LayoutTrack.tsx(1 hunks)apps/desktop/src/routes/editor/ui.tsx(3 hunks)packages/ui-solid/src/Button.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
apps/desktop/src/routes/editor/Timeline/LayoutTrack.tsx (1)
apps/desktop/src/routes/editor/Timeline/Track.tsx (1)
SegmentContent(89-101)
apps/desktop/src/routes/editor/ui.tsx (1)
apps/desktop/src/components/Tooltip.tsx (1)
Tooltip(23-47)
apps/desktop/src/routes/editor/Player.tsx (2)
apps/desktop/src/routes/editor/ui.tsx (1)
EditorButton(353-418)apps/desktop/src/components/Tooltip.tsx (1)
Tooltip(23-47)
apps/desktop/src/routes/editor/AspectRatioSelect.tsx (4)
apps/desktop/src/components/Tooltip.tsx (1)
Tooltip(23-47)apps/desktop/src/utils/tauri.ts (1)
AspectRatio(358-358)apps/desktop/src/routes/editor/projectConfig.ts (1)
ASPECT_RATIOS(8-14)apps/desktop/src/routes/editor/ui.tsx (5)
MenuItem(264-279)EditorButton(353-418)PopperContent(287-293)topLeftAnimateClasses(423-424)MenuItemList(295-309)
⏰ 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 (10)
apps/desktop/src/routes/editor/ExportDialog.tsx (2)
412-446: Icon/text color swap to text-gray-12 improves contrast — looks goodThe switch to text-gray-12 and explicit icon classes for the stats row (camera duration, resolution, size, render ETA) increases readability across themes. No functional changes. LGTM.
609-611: Custom-scroll utility is defined for desktop and web—no fallback requiredI’ve confirmed that the
custom-scrollclass is explicitly defined in your desktop styles and in your web globals, so it will be bundled correctly across all targets:
- apps/desktop/src/styles/theme.css lines 151–157: defines
.custom-scrollwithoverflow-y: scrolland custom scrollbar styling.- apps/web/globals.css lines 563–566: defines
.custom-scrollwithoverflow-y: autoand matching scrollbar rules.Since the utility exists in both codebases, there’s no need to fall back to
overflow-y-auto. Feel free to merge as is.apps/desktop/src/routes/editor/ui.tsx (3)
342-351: Public API: EditorButton now accepts kbd hints — good extensionAdding kbd?: string[] to EditorButtonProps is a clean way to propagate OS-aware shortcut hints. Typing looks correct and non-breaking for existing call sites.
388-392: Tooltip now shows keyboard hints on hover — UX improvementForwarding kbd to Tooltip works as intended and keeps the implementation localized to EditorButton. Nice.
356-369: splitProps extraction validated; no ref/kbd shadowing detectedI’ve audited all EditorButton usages (in AspectRatioSelect.tsx, Player.tsx, and PresetsDropdown.tsx) and confirmed:
- None pass an explicit
refprop to<EditorButton>, so there’s no downstream collision or ambiguity with the internalrefextraction.- The
kbdprop is only used vialocal.kbdand never forwarded inothers, so it correctly drives the Tooltip UI without duplication.No changes are needed for the current code paths.
Optional future improvement:
• If you ever intend to let consumers attach a ref to the underlying DOM node, forward it explicitly—e.g.:- <Polymorphic - as="button" - {...others} + <Polymorphic + as="button" + ref={local.ref} + {...others} class={…} >apps/desktop/src/routes/editor/AspectRatioSelect.tsx (1)
72-82: Composition with EditorButton/Tooltip is clean; API usage looks correctWrapping Trigger in EditorButton and moving listbox into PopperContent/MenuItemList matches existing UI patterns. Value renderer handles “auto” vs named ratios correctly. Looks good.
Also applies to: 83-92, 94-103
apps/desktop/src/components/Tooltip.tsx (1)
12-22: Ignore async OS detection and macOS mapping concerns in Tooltip.tsxThe
type()export from@tauri-apps/plugin-osin Tauri v2 is synchronous and returns one of theOsTypestrings—including"macos"—so:
const os = ostype();yields a plainOsType("linux" | "macos" | "windows" | "ios" | "android"), not aPromise(v2.tauri.app)- A macOS host correctly returns
"macos", making the existingos === "macos"checks valid (v2.tauri.app)No changes are required to handle async behavior or normalize
"Darwin"–style values. You may optionally refactor to a booleanisMacfor readability, but this is purely stylistic.Likely an incorrect or invalid review comment.
apps/desktop/src/routes/editor/Player.tsx (3)
218-224: LGTM: Crop button uses centralized handler and advertises shortcutThe tooltip + kbd hint is consistent with the new global shortcut and improves discoverability.
277-294: LGTM: Split toggle with keyboard hint and pressed stateGood use of
KToggleButtonviaasand clear pressed styling.
296-307: LGTM: OS-aware tooltips for zoom in/out match the implemented shortcutsThe hints align with the Equal/Minus handlers and the Tooltip’s OS-aware symbol mapping.
Also applies to: 308-317
This PR:
Preview:
Summary by CodeRabbit
New Features
Improvements
Style