fix(preview, UI): resolve WebGL context loss and implement premium custom header menus#87
Conversation
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds localized File, Edit, and View menus to the video editor titlebar, connects quit actions through Electron IPC, updates titlebar controls, and enables non-macOS menu-bar autohide for editor and notes windows. ChangesEditor desktop updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant EditorUser
participant EditorMenuBar
participant VideoEditor
participant electronAPI
participant ElectronMainProcess
EditorUser->>EditorMenuBar: Select File > Quit
EditorMenuBar->>VideoEditor: Invoke onQuit
VideoEditor->>electronAPI: Call quitApp()
electronAPI->>ElectronMainProcess: Send app-quit
ElectronMainProcess->>ElectronMainProcess: Call app.quit()
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/video-editor/VideoEditor.tsx (1)
2864-2878: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd an accessible label to the language
<select>.The
<select>at line 2866 has noaria-labelor associated<label>. TheLanguagesicon is a sibling element, not a label, so screen readers will announce an unlabeled combobox. The PR objectives mention focus accessibility.🛡️ Proposed fix
<select value={locale} onChange={(e) => setLocale(e.target.value as Locale)} + aria-label={rawT("common.language") || "Language"} className="bg-transparent text-[13px] font-semibold outline-none cursor-pointer appearance-none pr-1"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/video-editor/VideoEditor.tsx` around lines 2864 - 2878, Add an accessible name to the language select in the locale picker by associating it with a visible label or adding an appropriate aria-label. Keep the existing locale value, onChange behavior, and styling unchanged.
🧹 Nitpick comments (1)
src/components/video-editor/VideoEditor.tsx (1)
2748-2862: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
as nevertype assertions with proper i18n key typing.Every menu label uses
rawT("common.actions.file" as never) || "File", bypassing TypeScript's type checking on i18n keys. The|| "Fallback"pattern suggests these keys may not exist in locale files yet, meaning non-English users will see English menu labels. As per coding guidelines, TypeScript code must stay in strict mode —as neverdefeats the type safety that strict mode provides.♻️ Proposed refactor
Add the new keys to the i18n type definitions and locale files, then call
rawTwithout the assertion:- {rawT("common.actions.file" as never) || "File"} + {rawT("common.actions.file")}If the keys are genuinely dynamic, consider a type-safe wrapper that accepts a known key union and returns a non-nullable string, so the
|| "Fallback"pattern becomes unnecessary.Run the following script to check if the keys exist in locale files:
#!/bin/bash # Description: Check if the new i18n keys exist in locale files. # Search for the key paths in locale files for key in "common.actions.file" "common.actions.edit" "common.actions.view" \ "common.actions.undo" "common.actions.redo" "common.actions.reload" \ "common.actions.quit" "dialogs.unsavedChanges.newProject" \ "dialogs.unsavedChanges.loadProject" "dialogs.unsavedChanges.saveProject" \ "dialogs.unsavedChanges.saveProjectAs"; do echo "=== $key ===" rg -n "$key" src/i18n/ --type=json -g '!*.test.*' || echo "NOT FOUND" done🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/video-editor/VideoEditor.tsx` around lines 2748 - 2862, Replace every as never assertion in the menu labels within the File, Edit, and View DropdownMenu blocks with properly typed i18n keys. Add the missing keys to the i18n type definitions and all locale files, then call rawT directly and remove the English fallback expressions once the translations are guaranteed to exist.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 2748-2862: Update the shortcut labels in the File and Edit menus
to use the existing isMac value: display ⌘ for macOS and Ctrl otherwise. Apply
this to the New, Load, Save, Save As, Quit, Undo, and Redo DropdownMenuShortcut
elements while leaving the shortcut behavior unchanged.
---
Outside diff comments:
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 2864-2878: Add an accessible name to the language select in the
locale picker by associating it with a visible label or adding an appropriate
aria-label. Keep the existing locale value, onChange behavior, and styling
unchanged.
---
Nitpick comments:
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 2748-2862: Replace every as never assertion in the menu labels
within the File, Edit, and View DropdownMenu blocks with properly typed i18n
keys. Add the missing keys to the i18n type definitions and all locale files,
then call rawT directly and remove the English fallback expressions once the
translations are guaranteed to exist.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e689baa4-090d-4a76-9490-919d9d1187ad
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
electron/windows.tspackage.jsonsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/VideoPlayback.test.tssrc/components/video-editor/VideoPlayback.tsx
Adds an in-app File/Edit/View menu bar to the editor titlebar and auto-hides the native OS menu bar on Windows/Linux (electron/windows.ts), for a consistent custom-styled menu across platforms. Maintainer cleanup on top of the original PR getopenscreen#87: - Dropped the WebGL context-loss fix that was duplicated from getopenscreen#19 (this branch had merged getopenscreen#19's branch in); that fix now lands solely via getopenscreen#19. - Dropped the unrelated package.json `allowScripts` block and the @electron/windows-sign / postject / cross-dirname lockfile additions. - Extracted the menu into a testable EditorMenuBar component built from a pure model, with unit + interaction tests (EditorMenuBar.test.tsx). - Removed the unnecessary `as never` i18n casts and dead `|| "…"` fallbacks; the keys already exist in common/dialogs, so rawT() is called directly (matches the existing showInFolder usage). - Made shortcut hints platform-aware (⌘ on macOS, Ctrl elsewhere), matching the accelerators wired in the native menu (electron/main.ts) and the editor keydown handler. - Routed Quit through a new `app-quit` IPC (app.quit()) instead of window.close(), matching the native menu's role:"quit". Co-authored-by: Gede Cahya <49280905+gede-cahya@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
EtienneLescot
force-pushed
the
fix-export-preview-and-menu-ui
branch
from
e9c907b to
adf4ddc
Compare
|
Maintainer update — force-pushed a trimmed, menu-only branch. Heads up @gede-cahya: I rebased this PR to resolve the overlap with #19 and applied the review fixes directly on the branch. Trimmed the duplication
Fixes applied
Verification: Thanks for the feature — you're kept as co-author on the commit. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/video-editor/EditorMenuBar.tsx (1)
177-185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
onSelectoveronClickfor Radix primitives, and remove dead CSS modifiers.For Radix UI's
DropdownMenuItem, it is safer and more idiomatic to use theonSelectprop instead ofonClick. This ensures the dropdown's internal close and focus management behaves correctly.Additionally, the Tailwind
disabled:modifiers target the CSS:disabledpseudo-class. Since Radix renders menu items as<div>elements, these modifiers are functionally dead code. The Radix primitive already handles disabled styling internally by applyingdata-[disabled]:opacity-50 data-[disabled]:pointer-events-none.♻️ Proposed fix
<DropdownMenuItem - onClick={item.onSelect} + onSelect={() => item.onSelect()} disabled={item.disabled} className={ item.danger ? "hover:bg-red-500/20 focus:bg-red-500/20 focus:text-red-400 text-red-400 cursor-pointer justify-between" - : "hover:bg-white/[0.08] focus:bg-white/[0.08] focus:text-white cursor-pointer justify-between disabled:opacity-40 disabled:pointer-events-none" + : "hover:bg-white/[0.08] focus:bg-white/[0.08] focus:text-white cursor-pointer justify-between" } >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/video-editor/EditorMenuBar.tsx` around lines 177 - 185, Update the DropdownMenuItem in the menu item rendering to pass item.onSelect through onSelect instead of onClick, and remove the ineffective disabled:opacity-40 and disabled:pointer-events-none Tailwind modifiers from the non-danger className while preserving the existing styling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 2759-2764: Add an aria-label describing the selected language to
the select element in the locale selector, keeping its existing value, onChange
handler, styling, and behavior unchanged.
---
Nitpick comments:
In `@src/components/video-editor/EditorMenuBar.tsx`:
- Around line 177-185: Update the DropdownMenuItem in the menu item rendering to
pass item.onSelect through onSelect instead of onClick, and remove the
ineffective disabled:opacity-40 and disabled:pointer-events-none Tailwind
modifiers from the non-danger className while preserving the existing styling.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d080bf8-bb00-48aa-af45-750ee80b981a
📒 Files selected for processing (7)
electron/electron-env.d.tselectron/main.tselectron/preload.tselectron/windows.tssrc/components/video-editor/EditorMenuBar.test.tsxsrc/components/video-editor/EditorMenuBar.tsxsrc/components/video-editor/VideoEditor.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- electron/windows.ts
Addresses CodeRabbit review on EditorMenuBar: - DropdownMenuItem now uses onSelect (idiomatic for Radix — fires for pointer and keyboard selection with correct close/focus handling) instead of onClick. - Removed disabled:opacity-40 / disabled:pointer-events-none: Radix renders items as <div>, so the CSS :disabled modifiers never match; the base DropdownMenuItem already dims disabled items via data-[disabled]:opacity-50 / data-[disabled]:pointer-events-none. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds aria-label={ts("language.title")} to the titlebar language <select>
so screen readers announce it (the Languages icon is decorative).
Addresses CodeRabbit review.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds an in-app File/Edit/View menu bar to the editor titlebar and auto-hides the native OS menu bar on Windows/Linux (electron/windows.ts), for a consistent custom-styled menu across platforms. Maintainer cleanup on top of the original PR #87: - Dropped the WebGL context-loss fix that was duplicated from #19 (this branch had merged #19's branch in); that fix now lands solely via #19. - Dropped the unrelated package.json `allowScripts` block and the @electron/windows-sign / postject / cross-dirname lockfile additions. - Extracted the menu into a testable EditorMenuBar component built from a pure model, with unit + interaction tests (EditorMenuBar.test.tsx). - Removed the unnecessary `as never` i18n casts and dead `|| "…"` fallbacks; the keys already exist in common/dialogs, so rawT() is called directly (matches the existing showInFolder usage). - Made shortcut hints platform-aware (⌘ on macOS, Ctrl elsewhere), matching the accelerators wired in the native menu (electron/main.ts) and the editor keydown handler. - Routed Quit through a new `app-quit` IPC (app.quit()) instead of window.close(), matching the native menu's role:"quit". Co-authored-by: Gede Cahya <49280905+gede-cahya@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
This PR fixes the disappearing video preview on export (WebGL context loss) and replaces the native window menu bar with a premium custom inline dropdown menu bar (File, Edit, View) styled with dark glassmorphism and focus accessibility.
Summary by CodeRabbit