feat(launch): add capture-excluded notes window for recordings#43
Open
Itzadetunji wants to merge 10 commits into
Open
feat(launch): add capture-excluded notes window for recordings#43Itzadetunji wants to merge 10 commits into
Itzadetunji wants to merge 10 commits into
Conversation
Changed the query parameter from "windowType=notes" to "showNotes=true" in both the Electron window creation and the React app. This enhances clarity and consistency in how the notes window is displayed based on URL parameters.
Modified the Electron notes window to enhance its appearance and functionality by adjusting dimensions, background color, and removing unnecessary properties. Updated the React NotesWindow component to ensure it occupies the full screen height and width, improving user experience for note-taking.
Updated the dimensions of the Electron notes window for better usability and added max width and height constraints. Enhanced the React NotesWindow component to store and retrieve notes from local storage, improving the user experience by preserving notes across sessions.
Rearranged the order of window creation to ensure the countdown overlay is displayed correctly. Updated the title of the NotesWindow to "OpenScreen - Notes" for better branding and clarity.
Updated the NotesWindow component to utilize useLayoutEffect instead of useEffect for setting initial notes from local storage, improving performance and ensuring the notes are rendered correctly before the browser paints the UI.
Eliminated the allowScripts configuration for specific dependencies in package.json, streamlining the file and removing unnecessary entries.
📝 WalkthroughWalkthroughAdds a notes window flow: Electron creates and manages a notes BrowserWindow, the renderer opens it through ChangesNotes Window Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Author
|
@EtienneLescot, I would love to get feedback on this PR so I know what to improve upon |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@electron/windows.ts`:
- Around line 324-346: The Notes window is still using the default OS window
chrome because the BrowserWindow created in createNotesWindow() does not disable
the frame. Update the BrowserWindow options in createNotesWindow() to make it
frameless by setting the appropriate frame/title bar option so the window
renders without OS controls.
In `@src/App.tsx`:
- Around line 23-24: App currently mounts NotesWindow through two separate
conditions, so when windowType=notes and showNotes=true are both set it renders
twice and competes for the same notes localStorage state. Update App’s render
logic so NotesWindow is created through a single mutually exclusive path, using
the existing windowType/showNotes checks to choose only one branch and prevent
duplicate mounts; adjust the related NotesWindow render sites in App
accordingly.
In `@src/components/launch/LaunchWindow.tsx`:
- Around line 1048-1055: The new Notes icon-only button in LaunchWindow needs an
accessible name because the Tooltip content does not label the trigger. Update
the button that calls window.electronAPI.openNotes() to expose a clear label for
assistive tech, using the existing t("tooltips.openNotes") text as the button’s
accessible name while keeping the Tooltip for hover behavior.
In `@src/components/launch/NotesWindow.tsx`:
- Around line 17-20: The NotesWindow textarea is using hardcoded user-facing
placeholder text, so it bypasses the app’s localization flow. Update the
placeholder in NotesWindow to use the existing i18n/translation mechanism used
elsewhere in the launch UI, and ensure the new key is added to the locale
resources instead of leaving "Take notes here..." inline.
🪄 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: 5a24189e-3595-4780-8f6b-fa7b7d89cd0e
📒 Files selected for processing (10)
electron/electron-env.d.tselectron/ipc/handlers.tselectron/main.tselectron/preload.tselectron/windows.tssrc/App.tsxsrc/components/launch/LaunchWindow.tsxsrc/components/launch/NotesWindow.tsxsrc/i18n/locales/en/launch.jsonsrc/main.tsx
Removed the conditional rendering of NotesWindow from the main App component and integrated it directly within the return statement when showNotes is true. Added aria-label for the open notes button in LaunchWindow for better accessibility. Updated the placeholder text in NotesWindow to utilize localized strings for improved user experience.
Removed the conditional block for rendering NotesWindow and integrated it directly within the return statement, improving code clarity. This change ensures that NotesWindow is displayed when showNotes is true, streamlining the component structure.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/App.tsx`:
- Around line 117-120: The notes render path in App.tsx is not exclusive, so
when showNotes is true the NotesWindow renders but content still falls through
to the default Openscreen panel. Update the render logic around TooltipProvider,
NotesWindow, and the content branch so the notes route short-circuits the
default panel and only one view is shown. Use the existing showNotes and
windowType checks to make the NotesWindow path mutually exclusive from the
fallback content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
117
to
120
| return ( | ||
| <TooltipProvider> | ||
| {showNotes && <NotesWindow />} | ||
| {content} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the notes render path exclusive.
Per the new route contract, the notes window is opened with ?showNotes=true. In that case windowType stays empty, so content still falls through to the default "Openscreen" panel and renders alongside NotesWindow at Line 120. That breaks the notes-only window.
Suggested fix
return (
<TooltipProvider>
- {showNotes && <NotesWindow />}
- {content}
+ {showNotes ? <NotesWindow /> : content}
<Toaster theme="dark" />
</TooltipProvider>
);📝 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.
Suggested change
| return ( | |
| <TooltipProvider> | |
| {showNotes && <NotesWindow />} | |
| {content} | |
| return ( | |
| <TooltipProvider> | |
| {showNotes ? <NotesWindow /> : content} | |
| <Toaster theme="dark" /> | |
| </TooltipProvider> | |
| ); |
🤖 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/App.tsx` around lines 117 - 120, The notes render path in App.tsx is not
exclusive, so when showNotes is true the NotesWindow renders but content still
falls through to the default Openscreen panel. Update the render logic around
TooltipProvider, NotesWindow, and the content branch so the notes route
short-circuits the default panel and only one view is shown. Use the existing
showNotes and windowType checks to make the NotesWindow path mutually exclusive
from the fallback content.
Collaborator
|
@Itzadetunji thank you for this PR, could you provide us with screenshots of the feature ? |
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.
Notes window
Recording-time plain-text notes in a separate Electron window, opened from the HUD. The window uses content protection so it is excluded from most screen capture pipelines.
Pull request summary
Use the section below as the PR description (
gh pr create --body-file docs/features/notes-window.md— remove everything above this heading first, or copy from Summary onward).Summary
Adds a dedicated Notes window for taking plain-text notes while recording. Users open it from a new notepad button on the recording HUD. Notes live in a separate
BrowserWindow(not the HUD overlay) withsetContentProtection(true)so the window is excluded from most screen capture pipelines.Implementation follows the existing multi-window pattern: main-process factory in
electron/windows.ts, lifecycle wrapper inelectron/main.ts, IPC viaopen-notes/electronAPI.openNotes(), and renderer routing with the?showNotes=truequery param to renderNotesWindow. Re-opening notes focuses the existing window instead of creating duplicates. Note content is persisted locally vialocalStorage.Related issue
Fixes #
Type of change
Release impact
Desktop impact
Screenshots / video
Testing
Manual
npm run devlocalStorage).Commands
npm run lintnpx tsc --noEmitnpm run testnpm run i18n:checkPlatforms tested
Architecture
OpenScreen uses one Vite/React entry (
index.html) for every window. The notes window is a secondBrowserWindowthat loads the same app with?showNotes=true.App.tsxrenders<NotesWindow />when that param is present.electron/windows.tscreateNotesWindow()— size, title,setContentProtection, load URLelectron/main.tsnotesWindowref,createNotesWindowWrapper(),closedhandlerelectron/ipc/handlers.tsopen-notes— focus existing or createelectron/preload.tsopenNotes()electron/electron-env.d.tsopenNotesreturn typesrc/App.tsx,src/main.tsxshowNotesquery paramsrc/components/launch/NotesWindow.tsxlocalStoragesrc/components/launch/LaunchWindow.tsxsrc/i18n/locales/en/launch.jsontooltips.openNotesWindow options
The notes window is a standard framed, resizable window:
skipTaskbar: false)Persistence
Notes are stored in renderer
localStorageunder the key"notes". This is global to the app install, not tied to a specific recording or project.Known limitations (v1)
tooltips.openNotesadded to English locale only; other locales need updating beforenpm run i18n:checkpassesRelease note (changelog)
Summary by CodeRabbit
New Features
showNotes=trueURL parameter, plus new notes tooltips.Bug Fixes