feat: add Notes teleprompter mode - #152
Conversation
📝 WalkthroughWalkthroughAdds teleprompter mode to the notes window with persisted speed, font size, and mirror settings; animation-driven scrolling; expanded toolbar controls; legacy note migration; presentation styling; localized labels; and unit, component, and browser accessibility tests. ChangesNotes teleprompter
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant NotesToolbar
participant NotesWindow
participant TipTapEditor
participant requestAnimationFrame
NotesToolbar->>NotesWindow: Toggle playback or change settings
NotesWindow->>requestAnimationFrame: Schedule playback tick
requestAnimationFrame->>NotesWindow: Return frame timestamp
NotesWindow->>TipTapEditor: Update scroll position
NotesWindow->>NotesToolbar: Render current playback state
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
🧹 Nitpick comments (5)
src/components/launch/NotesToolbar.browser.test.tsx (2)
56-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStub editor is recreated on every render.
createEditor()runs in the render body, so each state update passes a new object identity anduseEditorRevision's effect (deps: [editor]) re-runsoff/onon every click. Hold it stable so the harness matches real usage.♻️ Proposed fix
function ToolbarHarness() { + const [editor] = useState(createEditor); const [isPlaying, setIsPlaying] = useState(false); const [speed, setSpeed] = useState(40); const [fontSize, setFontSize] = useState(16); const [mirrored, setMirrored] = useState(false); return ( <NotesToolbar - editor={createEditor()} + editor={editor}🤖 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/launch/NotesToolbar.browser.test.tsx` around lines 56 - 77, Update ToolbarHarness to create the editor once and preserve its identity across state updates, using a stable hook or equivalent initialization outside the render-time state changes. Pass that stable editor to NotesToolbar instead of calling createEditor() directly during each render, so useEditorRevision does not repeatedly rebind listeners after clicks.
116-129: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePositional assumptions make this brittle.
The tab loop requires DOM order to equal tab order and every control to be enabled — it only holds because the harness starts at speed 40 / font 16. Likewise
controls.at(-1)assumes mirror is rendered last. Selecting byaria-labelwould survive reordering and bound changes.🤖 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/launch/NotesToolbar.browser.test.tsx` around lines 116 - 129, The NotesToolbar focus test relies on DOM positions and ordering that change with control state and layout. Update the test to identify controls by their aria-labels, and verify tab/focus behavior using the expected labeled controls rather than controls array indices; select the mirror control by its label instead of controls.at(-1), while preserving the Pause and aria-pressed assertions.src/components/launch/NotesToolbar.test.tsx (2)
54-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the active states too.
createPropsonly ever rendersisPlaying: false/mirrored: false. A case with bothtruewould pin the Pause label andaria-pressed="true"in the jsdom suite rather than relying solely on the browser test.As per coding guidelines, "Add a test for every new behavior in the same package as the code under test."
🤖 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/launch/NotesToolbar.test.tsx` around lines 54 - 69, Add a NotesToolbar test using createProps with both isPlaying and mirrored set to true, and assert the active-state behavior: the Pause label and aria-pressed="true". Keep the existing inactive-state coverage unchanged.Source: Coding guidelines
12-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winToolbar test fixtures are copy-pasted between the two suites. The
createEditorstub and the i18n label map are byte-identical in both files; when a label key or a TipTap command is added, both copies must be edited or one suite silently drifts. Extract them into a shared fixture module (e.g.notesToolbarTestFixtures.tsnext to the component).
src/components/launch/NotesToolbar.test.tsx#L12-L52: move the label map andcreateEditorinto the shared fixture and import them here.src/components/launch/NotesToolbar.browser.test.tsx#L10-L54: import the same fixture instead of redeclaring the mock and stub.🤖 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/launch/NotesToolbar.test.tsx` around lines 12 - 52, Extract the shared i18n label map and createEditor fixture from src/components/launch/NotesToolbar.test.tsx lines 12-52 into a shared notesToolbarTestFixtures module next to the component, then import and use that fixture here. Update src/components/launch/NotesToolbar.browser.test.tsx lines 10-54 to import the same shared fixture and remove its duplicate mock and editor stub declarations.src/components/launch/NotesToolbar.tsx (1)
208-219: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicate toggle state signaling.
ToolbarButtonalways emitsaria-pressed={active}, so this Play/Pause teleprompter control reads as both “Pause” and pressed. Keep a static stateless label like “Auto-scroll” witharia-pressed, or keep the label swap and droparia-pressed.🤖 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/launch/NotesToolbar.tsx` around lines 208 - 219, Update the teleprompter ToolbarButton using active={isPlaying} and onTogglePlaying so it does not simultaneously expose a changing Play/Pause label and aria-pressed state; either use a static “Auto-scroll” label while retaining active/aria-pressed, or preserve the label swap and remove the active prop.
🤖 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/launch/NotesToolbar.tsx`:
- Around line 235-241: Update both readouts in NotesToolbar: remove their
aria-label and redundant aria-live attributes so the dynamic values are
announced directly, relying on the parent group labels. Add localized unit keys
to launch.json for all 13 locales, and use the existing
translation/locale-formatting mechanism to render the speed and font-size values
with translated units instead of hardcoded “px/s” and “px”.
---
Nitpick comments:
In `@src/components/launch/NotesToolbar.browser.test.tsx`:
- Around line 56-77: Update ToolbarHarness to create the editor once and
preserve its identity across state updates, using a stable hook or equivalent
initialization outside the render-time state changes. Pass that stable editor to
NotesToolbar instead of calling createEditor() directly during each render, so
useEditorRevision does not repeatedly rebind listeners after clicks.
- Around line 116-129: The NotesToolbar focus test relies on DOM positions and
ordering that change with control state and layout. Update the test to identify
controls by their aria-labels, and verify tab/focus behavior using the expected
labeled controls rather than controls array indices; select the mirror control
by its label instead of controls.at(-1), while preserving the Pause and
aria-pressed assertions.
In `@src/components/launch/NotesToolbar.test.tsx`:
- Around line 54-69: Add a NotesToolbar test using createProps with both
isPlaying and mirrored set to true, and assert the active-state behavior: the
Pause label and aria-pressed="true". Keep the existing inactive-state coverage
unchanged.
- Around line 12-52: Extract the shared i18n label map and createEditor fixture
from src/components/launch/NotesToolbar.test.tsx lines 12-52 into a shared
notesToolbarTestFixtures module next to the component, then import and use that
fixture here. Update src/components/launch/NotesToolbar.browser.test.tsx lines
10-54 to import the same shared fixture and remove its duplicate mock and editor
stub declarations.
In `@src/components/launch/NotesToolbar.tsx`:
- Around line 208-219: Update the teleprompter ToolbarButton using
active={isPlaying} and onTogglePlaying so it does not simultaneously expose a
changing Play/Pause label and aria-pressed state; either use a static
“Auto-scroll” label while retaining active/aria-pressed, or preserve the label
swap and remove the active prop.
🪄 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: f423f790-e66c-4f39-aa80-533f0695f1a9
📒 Files selected for processing (21)
src/components/launch/NotesToolbar.browser.test.tsxsrc/components/launch/NotesToolbar.test.tsxsrc/components/launch/NotesToolbar.tsxsrc/components/launch/NotesWindow.module.csssrc/components/launch/NotesWindow.test.tsxsrc/components/launch/NotesWindow.tsxsrc/components/launch/notesTeleprompter.test.tssrc/components/launch/notesTeleprompter.tssrc/i18n/locales/ar/launch.jsonsrc/i18n/locales/en/launch.jsonsrc/i18n/locales/es/launch.jsonsrc/i18n/locales/fr/launch.jsonsrc/i18n/locales/it/launch.jsonsrc/i18n/locales/ja-JP/launch.jsonsrc/i18n/locales/ko-KR/launch.jsonsrc/i18n/locales/pt-BR/launch.jsonsrc/i18n/locales/ru/launch.jsonsrc/i18n/locales/tr/launch.jsonsrc/i18n/locales/vi/launch.jsonsrc/i18n/locales/zh-CN/launch.jsonsrc/i18n/locales/zh-TW/launch.json
|
Good news: git fetch origin && git rebase origin/mainRe-run |
vitest 4 removed the vitest/browser re-export; tsconfig.test.json typecheck (new CI gate on the 1.8.0 line) fails on the old path.
48b9ab6 to
60bd856
Compare
|
You beat me to it — I had a rebase ready to push and the safety check stopped me, which is the right outcome. Checked your merge: no leftover conflict markers, |
|
Thanks for checking. Sorry I didn’t leave a note after pushing. |
Play was a no-op whenever there was nothing left to scroll: a note that fits the window, or a run that had already reached the bottom, flipped the button to Pause and back within two frames. isAtTeleprompterEnd() now reports the end only when there is somewhere to go, and starting playback at the bottom rewinds to the top instead of leaving the control looking inert. Playback also tracked its position through the DOM, re-reading scrollTop every frame. At the slowest speed a frame advances ~0.17px, which an engine that snaps scroll offsets to whole pixels would round away, stalling the teleprompter. resolveTeleprompterPosition() keeps the fractional position internally and hands control back to the DOM once it drifts past a pixel, so scrolling by hand mid-playback still moves the teleprompter rather than fighting it. Also: - Drop NotesToolbar.browser.test.tsx and port what jsdom can assert into NotesToolbar.test.tsx. vitest.config.ts excludes src/**/*.browser.test.*, and the repo has no vitest.browser.config.ts and no test:browser script, so the file never ran anywhere — CI included. - Restore the no-scrollbar utility the toolbar lost when it was split into two rows; without it both rows paint a horizontal scrollbar once they overflow. - Convert the remaining tiptap spacing to em so it tracks the teleprompter font size. Identical at the default 16px; heading margins stay in rem because em there resolves against the heading's own enlarged size. - Stop announcing playback twice: the play button keeps its state-dependent label and drops aria-pressed, via a new highlighted prop that styles without the toggle semantics. - Make the note read-only while playing and disable formatting with it, so a keystroke can no longer yank the scroll through scroll-caret-into-view. - Move the generic units.* keys from the launch namespace to common. - Skip the mount-time settings write, load the initial note once, and drop a no-op transform-origin rule.
|
Thanks for this — the feature is well factored, and splitting the scroll maths into a pure Two real bugs
The browser test never ran
Smaller things
Verification
Each new test was mutation-checked: reverting the corresponding fix makes exactly that test fail, so none of them are vacuous. I also drove the real Notes window in a browser at One thing I did not change: mirrored mode leaves the editor editable, so click-to-place-caret is horizontally inverted there. That seemed inherent to the glass-teleprompter use case rather than a defect — happy to revisit if you'd rather it went read-only too. |
|
Thanks for landing the fixes directly and for the depth of the verification — much appreciated. I also ran a verification pass in the real Notes window (on the same Notes sources as the current head — the newer merge only touched the website), driving the animation with deterministic On mirrored editing: caret placement and text selection are visually reversed while the mirror is on. My preference would be to treat mirrored mode as presentation-only — make the note read-only while mirrored and restore editing when the mirror is turned off, keeping the other teleprompter controls live, the same way playback already does. Would you like that as a small follow-up in this PR? I'll hold off on any code changes until you weigh in. |
Summary
This PR adds a manual teleprompter mode to the existing Notes window.
mirroring the toolbar.
playback paused.
and component coverage.
Speech-adaptive scrolling, speech recognition, and speed presets are optional
enhancements and are intentionally not included in this MVP.
Related issue
Fixes #67
Type of change
Release impact
Desktop impact
Screenshots / video
Not included.
Testing
npx tsc --noEmitandnpx tsc -p tsconfig.test.json --noEmitare both clean.npm run i18n:checkpasses — all locales matchenacross every namespace.npm run build-vitegit diff --checkManual verification in the real Notes window, driving the animation with
deterministic
requestAnimationFrametimestamp stepping (not a real-timeVSync end-to-end run): playback on content that fits the window keeps playing
instead of flipping back, 10 px/s accumulates sub-pixel steps without
stalling, playback stops on reaching the end, restarting from the bottom
rewinds to the top, long frame gaps are clamped, the note is read-only while
playing, mirroring flips the content but not the toolbar, speed/font/mirror
settings persist with playback always starting paused, the speed control's
decrease button disables at the 10 px/s floor, and the play control announces
its state through its label alone.
End-to-end tests, native capture checks, and full Electron packaging were not
run because this change is limited to the renderer Notes window.
Summary by CodeRabbit
New Features
Bug Fixes
Tests