fix(preview, UI): resolve WebGL context loss and implement premium custom header menus#87
Open
gede-cahya wants to merge 5 commits into
Open
Conversation
On Manjaro / CachyOS (Linux/Wayland with Mesa/EGL) the preview Pixi app can lose its WebGL context during heavy GPU usage from the exporter — most likely the `VideoEncoder`'s `prefer-hardware` path or the Linux-only CPU readback in `frameRenderer.ts.readbackVideoCanvas`. The wallpaper is rendered to a 2D canvas (`frameRenderer.ts:212`, "Background renders separately, not in PixiJS") and survives the context loss, but the video sprite lives inside the lost Pixi GL context and never reappears. That is the "only background remains" symptom in issue getopenscreen#8. Fix: attach a `webglcontextlost` listener to the preview Pixi canvas. `preventDefault()` opts in to Chromium's restoration attempt, and bumping a `pixiGeneration` state tears the broken app down via the existing `useEffect` cleanup so the next mount rebuilds it from scratch with a fresh WebGL context. The recovery is logged for diagnostics ("[VideoPlayback] WebGL context lost, recreating Pixi app"), which will now reach the terminal thanks to the console-message forwarding added to `electron/main.ts` in PR getopenscreen#11. Extracted the handler into `createWebGLContextLostHandler` for unit testability and added a small focused test that covers the two invariants the bug hinges on: the event's `preventDefault` is called and the regenerate callback fires. Tested: - npx tsc --noEmit - npm run lint - npm test (244 passed, +3 new) - npm run build-vite
…indows/Linux, and implement custom header menus
📝 WalkthroughWalkthroughThe PR adds custom video editor titlebar menus and controls, enables non-macOS menu-bar autohide, permits scripts for selected packages, and adds Pixi WebGL context-loss recovery with initialization handling, listener cleanup, and tests. ChangesEditor desktop updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PixiApplication
participant VideoPlayback
participant Canvas
PixiApplication->>Canvas: initialize WebGL canvas
Canvas->>VideoPlayback: emit webglcontextlost
VideoPlayback->>Canvas: preventDefault()
VideoPlayback->>VideoPlayback: increment pixiGeneration
VideoPlayback->>PixiApplication: tear down and rebuild
Possibly related PRs
Suggested reviewers: 🚥 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
Comment on lines
+2748
to
+2862
| <div className={`flex items-center gap-0.5 ${isMac ? "ml-14" : "ml-2"}`}> | ||
| <DropdownMenu> | ||
| <DropdownMenuTrigger asChild> | ||
| <button | ||
| type="button" | ||
| className="px-2.5 py-1.5 rounded-lg text-[13px] font-semibold text-slate-300 hover:text-white hover:bg-white/[0.08] transition-all duration-150 outline-none focus-visible:ring-1 focus-visible:ring-white/20 focus-visible:bg-white/[0.08]" | ||
| > | ||
| {rawT("common.actions.file" as never) || "File"} | ||
| </button> | ||
| </DropdownMenuTrigger> | ||
| <DropdownMenuContent | ||
| align="start" | ||
| className="bg-[#09090b]/95 backdrop-blur-md border border-white/[0.08] text-slate-200 min-w-[170px]" | ||
| > | ||
| <DropdownMenuItem | ||
| onClick={handleNewProject} | ||
| className="hover:bg-white/[0.08] focus:bg-white/[0.08] focus:text-white cursor-pointer justify-between" | ||
| > | ||
| <span>{rawT("dialogs.unsavedChanges.newProject" as never) || "New Project"}</span> | ||
| <DropdownMenuShortcut className="ml-2">Ctrl+N</DropdownMenuShortcut> | ||
| </DropdownMenuItem> | ||
| <DropdownMenuSeparator className="bg-white/[0.08]" /> | ||
| <DropdownMenuItem | ||
| onClick={handleLoadProject} | ||
| className="hover:bg-white/[0.08] focus:bg-white/[0.08] focus:text-white cursor-pointer justify-between" | ||
| > | ||
| <span> | ||
| {rawT("dialogs.unsavedChanges.loadProject" as never) || "Load Project…"} | ||
| </span> | ||
| <DropdownMenuShortcut className="ml-2">Ctrl+O</DropdownMenuShortcut> | ||
| </DropdownMenuItem> | ||
| <DropdownMenuItem | ||
| onClick={handleSaveProject} | ||
| className="hover:bg-white/[0.08] focus:bg-white/[0.08] focus:text-white cursor-pointer justify-between" | ||
| > | ||
| <span> | ||
| {rawT("dialogs.unsavedChanges.saveProject" as never) || "Save Project…"} | ||
| </span> | ||
| <DropdownMenuShortcut className="ml-2">Ctrl+S</DropdownMenuShortcut> | ||
| </DropdownMenuItem> | ||
| <DropdownMenuItem | ||
| onClick={handleSaveProjectAs} | ||
| className="hover:bg-white/[0.08] focus:bg-white/[0.08] focus:text-white cursor-pointer justify-between" | ||
| > | ||
| <span> | ||
| {rawT("dialogs.unsavedChanges.saveProjectAs" as never) || "Save Project As…"} | ||
| </span> | ||
| <DropdownMenuShortcut className="ml-2">Ctrl+Shift+S</DropdownMenuShortcut> | ||
| </DropdownMenuItem> | ||
| <DropdownMenuSeparator className="bg-white/[0.08]" /> | ||
| <DropdownMenuItem | ||
| onClick={() => window.close()} | ||
| className="hover:bg-red-500/20 focus:bg-red-500/20 focus:text-red-400 text-red-400 cursor-pointer justify-between" | ||
| > | ||
| <span>{rawT("common.actions.quit" as never) || "Quit"}</span> | ||
| <DropdownMenuShortcut className="ml-2">Ctrl+Q</DropdownMenuShortcut> | ||
| </DropdownMenuItem> | ||
| </DropdownMenuContent> | ||
| </DropdownMenu> | ||
|
|
||
| <DropdownMenu> | ||
| <DropdownMenuTrigger asChild> | ||
| <button | ||
| type="button" | ||
| className="px-2.5 py-1.5 rounded-lg text-[13px] font-semibold text-slate-300 hover:text-white hover:bg-white/[0.08] transition-all duration-150 outline-none focus-visible:ring-1 focus-visible:ring-white/20 focus-visible:bg-white/[0.08]" | ||
| > | ||
| {rawT("common.actions.edit" as never) || "Edit"} | ||
| </button> | ||
| </DropdownMenuTrigger> | ||
| <DropdownMenuContent | ||
| align="start" | ||
| className="bg-[#09090b]/95 backdrop-blur-md border border-white/[0.08] text-slate-200 min-w-[130px]" | ||
| > | ||
| <DropdownMenuItem | ||
| onClick={undo} | ||
| disabled={!canUndo} | ||
| className="hover:bg-white/[0.08] focus:bg-white/[0.08] focus:text-white cursor-pointer justify-between disabled:opacity-40 disabled:pointer-events-none" | ||
| > | ||
| <span>{rawT("common.actions.undo" as never) || "Undo"}</span> | ||
| <DropdownMenuShortcut className="ml-2">Ctrl+Z</DropdownMenuShortcut> | ||
| </DropdownMenuItem> | ||
| <DropdownMenuItem | ||
| onClick={redo} | ||
| disabled={!canRedo} | ||
| className="hover:bg-white/[0.08] focus:bg-white/[0.08] focus:text-white cursor-pointer justify-between disabled:opacity-40 disabled:pointer-events-none" | ||
| > | ||
| <span>{rawT("common.actions.redo" as never) || "Redo"}</span> | ||
| <DropdownMenuShortcut className="ml-2">Ctrl+Y</DropdownMenuShortcut> | ||
| </DropdownMenuItem> | ||
| </DropdownMenuContent> | ||
| </DropdownMenu> | ||
|
|
||
| <DropdownMenu> | ||
| <DropdownMenuTrigger asChild> | ||
| <button | ||
| type="button" | ||
| className="px-2.5 py-1.5 rounded-lg text-[13px] font-semibold text-slate-300 hover:text-white hover:bg-white/[0.08] transition-all duration-150 outline-none focus-visible:ring-1 focus-visible:ring-white/20 focus-visible:bg-white/[0.08]" | ||
| > | ||
| {rawT("common.actions.view" as never) || "View"} | ||
| </button> | ||
| </DropdownMenuTrigger> | ||
| <DropdownMenuContent | ||
| align="start" | ||
| className="bg-[#09090b]/95 backdrop-blur-md border border-white/[0.08] text-slate-200 min-w-[130px]" | ||
| > | ||
| <DropdownMenuItem | ||
| onClick={() => window.location.reload()} | ||
| className="hover:bg-white/[0.08] focus:bg-white/[0.08] focus:text-white cursor-pointer justify-between" | ||
| > | ||
| <span>{rawT("common.actions.reload" as never) || "Reload"}</span> | ||
| <DropdownMenuShortcut className="ml-2">Ctrl+R</DropdownMenuShortcut> | ||
| </DropdownMenuItem> | ||
| </DropdownMenuContent> | ||
| </DropdownMenu> | ||
| </div> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keyboard shortcut labels should be platform-aware.
All shortcut hints are hardcoded as Ctrl+… (e.g., Ctrl+N, Ctrl+Z). On macOS, users expect ⌘ (Cmd) as the modifier. The isMac variable is already available at line 326 from useShortcuts().
✨ Proposed fix
- // Near line 326, derive a display modifier:
- // const { shortcuts, isMac } = useShortcuts();
+ const { shortcuts, isMac } = useShortcuts();
+ const modKey = isMac ? "⌘" : "Ctrl";
// Then in each DropdownMenuShortcut:
- <DropdownMenuShortcut className="ml-2">Ctrl+N</DropdownMenuShortcut>
+ <DropdownMenuShortcut className="ml-2">{modKey}+N</DropdownMenuShortcut>
- <DropdownMenuShortcut className="ml-2">Ctrl+O</DropdownMenuShortcut>
+ <DropdownMenuShortcut className="ml-2">{modKey}+O</DropdownMenuShortcut>
- <DropdownMenuShortcut className="ml-2">Ctrl+S</DropdownMenuShortcut>
+ <DropdownMenuShortcut className="ml-2">{modKey}+S</DropdownMenuShortcut>
- <DropdownMenuShortcut className="ml-2">Ctrl+Shift+S</DropdownMenuShortcut>
+ <DropdownMenuShortcut className="ml-2">{isMac ? "⌘⇧" : "Ctrl+Shift"}+S</DropdownMenuShortcut>
- <DropdownMenuShortcut className="ml-2">Ctrl+Q</DropdownMenuShortcut>
+ <DropdownMenuShortcut className="ml-2">{modKey}+Q</DropdownMenuShortcut>
- <DropdownMenuShortcut className="ml-2">Ctrl+Z</DropdownMenuShortcut>
+ <DropdownMenuShortcut className="ml-2">{modKey}+Z</DropdownMenuShortcut>
- <DropdownMenuShortcut className="ml-2">Ctrl+Y</DropdownMenuShortcut>
+ <DropdownMenuShortcut className="ml-2">{modKey}+Y</DropdownMenuShortcut>
- <DropdownMenuShortcut className="ml-2">Ctrl+R</DropdownMenuShortcut>
+ <DropdownMenuShortcut className="ml-2">{modKey}+R</DropdownMenuShortcut>🤖 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, 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.
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
2 participants
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 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