feat(editor): on-screen keystroke overlay for recordings#733
Conversation
Adds the capture and display-logic foundation for an on-screen keystroke
overlay (typed keys / shortcut chords during playback + export — a common
tutorial/demo need; fits "additional editor tools and workflow polish").
This is part 1 of 2 and is intentionally inert: the feature flag defaults
OFF and the renderer/settings land in a follow-up, so there is no user-facing
change and no risk to existing recording/edit/export flows.
Capture (main process), privacy-gated:
- Listen for keydown on the existing uiohook interaction hook; record
{timeMs, key, modifiers} into a bounded keystroke telemetry buffer.
- Resolve keycodes to stable semantic tokens via uiohook's UiohookKey table
so the renderer never handles raw platform keycodes.
- Collapse auto-repeat; gate ALL storage on isKeystrokeCaptureActive
(default OFF — a global key hook can observe secrets typed while recording).
Display-logic core (pure, renderer), fully unit-tested:
- keyLabels: token -> glyph, platform-aware modifiers (Cmd/Opt/Shift/Ctrl vs
Ctrl/Alt/Win/Super).
- keystrokeCoalescing: chord grouping, rapid-key merging, fade/TTL, and
progressive reveal for scrub-accurate editor replay. Frame-rate independent.
Tests: 24 new unit tests (incl. a fast-check property test); existing cursor
interaction/telemetry tests still pass.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds end-to-end keystroke telemetry: Electron keydown capture, bounded persistence, IPC retrieval, editor settings, and a Pixi overlay that labels, groups, fades, and renders captured keystrokes with unit and property-based tests. ChangesKeystroke Telemetry Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Hook as uiohook
participant Main as Electron recording
participant File as Keystroke telemetry file
participant Editor as VideoEditor
participant Overlay as PixiKeystrokeOverlay
Hook->>Main: Emit keydown
Main->>File: Persist normalized samples
Editor->>Main: Request keystroke telemetry
Main->>File: Read telemetry
File-->>Editor: Return samples
Editor->>Overlay: Pass samples and display settings
Overlay->>Overlay: Label, group, fade, and render keycaps
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 review |
✅ Action performedReview finished.
|
![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 (1)
electron/ipc/cursor/interaction.ts (1)
303-339: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider updating
lastKeystrokeTimeMson collapsed events for complete auto-repeat suppression.
lastKeystrokeTimeMsis only updated when a keystroke is recorded, not when it's collapsed. This means a held key with 30ms auto-repeat intervals will record one event every ~45ms (every other repeat passes the threshold). If complete suppression is the intent, updatelastKeystrokeTimeMson every event so subsequent repeats are always within the 45ms window of the last seen event.If partial collapsing is intentional (e.g., to show held-key state in the overlay), disregard.
♻️ Optional: complete auto-repeat suppression
// Collapse auto-repeat: ignore the same physical key re-firing rapidly. if (keycode === lastKeystrokeCode && timeMs - lastKeystrokeTimeMs < 45) { + lastKeystrokeTimeMs = timeMs; return; } lastKeystrokeCode = keycode; lastKeystrokeTimeMs = timeMs;🤖 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 `@electron/ipc/cursor/interaction.ts` around lines 303 - 339, The `onKeyDown` auto-repeat collapse logic in `interaction.ts` only updates `lastKeystrokeTimeMs` when a keystroke is recorded, so repeated keydowns can still leak through every ~45ms. Update the state inside `onKeyDown` so `lastKeystrokeTimeMs` (and, if needed, `lastKeystrokeCode`) reflects every seen keydown event, including collapsed ones, while keeping the existing privacy gate and `pushKeystrokeSample` behavior intact.
🤖 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/videoPlayback/keystrokeOverlay/keystrokeCoalescing.ts`:
- Line 63: The KeycapGroup id in keystrokeCoalescing is non-deterministic
because it depends on groups.length, which changes as the lookback window shifts
and causes React key remounts. Update the id generation in the grouping logic to
be derived only from the group’s first keystroke identity in coalesceKeystrokes,
using a stable field from the first event (or a deterministic per-event counter
from relevant if needed) instead of the array position. Ensure the resulting
KeycapGroup.id contract remains stable across frames and scrubbing.
---
Nitpick comments:
In `@electron/ipc/cursor/interaction.ts`:
- Around line 303-339: The `onKeyDown` auto-repeat collapse logic in
`interaction.ts` only updates `lastKeystrokeTimeMs` when a keystroke is
recorded, so repeated keydowns can still leak through every ~45ms. Update the
state inside `onKeyDown` so `lastKeystrokeTimeMs` (and, if needed,
`lastKeystrokeCode`) reflects every seen keydown event, including collapsed
ones, while keeping the existing privacy gate and `pushKeystrokeSample` behavior
intact.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 59eaf233-cdf3-40a6-86c5-0c5458bc24a9
📒 Files selected for processing (10)
electron/ipc/constants.tselectron/ipc/cursor/interaction.tselectron/ipc/cursor/telemetry.tselectron/ipc/state.tselectron/ipc/types.tssrc/components/video-editor/videoPlayback/keystrokeOverlay/keyLabels.test.tssrc/components/video-editor/videoPlayback/keystrokeOverlay/keyLabels.tssrc/components/video-editor/videoPlayback/keystrokeOverlay/keystrokeCoalescing.test.tssrc/components/video-editor/videoPlayback/keystrokeOverlay/keystrokeCoalescing.tssrc/components/video-editor/videoPlayback/keystrokeOverlay/keystrokeTypes.ts
Address CodeRabbit review on webadderallorg#733: - keystrokeCoalescing: derive KeycapGroup.id from the first keystroke's identity (time + key + modifiers) instead of its position in the sliding lookback window, so a group keeps a stable id across frames and does not remount/flicker during scrub. Adds a regression test. - interaction: bump lastKeystrokeTimeMs on collapsed auto-repeats so a held key is fully suppressed instead of leaking one event every ~45ms. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…layer (2/3) Builds on the capture + display foundation (webadderallorg#733): - projectPersistence + editorPreferences: persist showKeystrokes / keystrokePosition / keystrokeSize (mirrors the cursor prefs), normalized and defaulted (off / bottom-center / 1x). - PixiKeystrokeOverlay: a parent-agnostic PIXI layer mirroring PixiCursorOverlay's construct/update/reset/destroy contract, driven entirely by the tested coalescing core; pooled Graphics + Text keycaps, screen-anchored (bottom/top), size-scaled, with per-group fade. - detectGlyphStyle(): platform-aware modifier glyphs (guarded for Node tests). tsc clean (electron + renderer); 25 pure-core tests pass. Remaining wiring (settings-panel controls + parent threading, VideoPlayback instantiation, keystroke sidecar persistence + IPC, export parity) needs a local dev loop to verify and is tracked as the follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add the main-process persistence layer for keystroke telemetry, mirroring the
cursor .cursor.json sidecar exactly:
- utils: getKeystrokePathForVideo -> `${videoPath}.keystrokes.json`
- telemetry: normalizeKeystrokeTelemetrySamples, writeKeystrokeTelemetry,
persistPendingKeystrokeTelemetry, snapshotKeystrokeTelemetryForPersistence
(use KEYSTROKE_TELEMETRY_VERSION / MAX_KEYSTROKE_SAMPLES; skip/remove the
sidecar when empty, matching the cursor path).
tsc (node) + biome clean. Wiring these into set-recording-state, the stop
paths, a get-keystroke-telemetry IPC channel, and the renderer load path is
the remaining follow-up (see PR description checklist).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e prefs Adding showKeystrokes/keystrokePosition/keystrokeSize to PersistedEditorControls left captureEditorPresetSnapshot's EditorPresetSnapshot literal missing them. Add the three fields (placeholder defaults until VideoEditor manages keystroke settings in part 3). Full-project tsc is now clean except one pre-existing unused import (VideoPlayback scalePreviewBorderRadius) that predates this branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- recording.ts: enable keystroke capture at record-start when showKeystrokes is set (new set-recording-state options arg); disable + snapshot on all stop paths (native-win + set-recording-state); add get-keystroke-telemetry read handler mirroring the cursor one. - preload + electron-env: getKeystrokeTelemetry bridge, setRecordingState options, KeystrokeTelemetryPoint type. tsc (node) clean. Renderer wiring (overlay mount + settings toggle) is the remaining follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/components/video-editor/videoPlayback/keystrokeOverlay/keystrokeCoalescing.ts (2)
86-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
open &&guard.
canExtendalready checksopen &&on line 88, so theopen &&on line 93 is redundant. This is harmless but slightly reduces readability — a reader might wonder if the extra check guards against a different condition.♻️ Proposed refactor
- if (open && canExtend) { + if (canExtend) {Since
canExtendisopen && ..., it is falsy whenopenis undefined, so the behavior is identical.🤖 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/videoPlayback/keystrokeOverlay/keystrokeCoalescing.ts` around lines 86 - 93, Remove the redundant open guard from the conditional in the keystroke coalescing logic: since canExtend already includes the open check, use canExtend alone in the if statement while preserving all existing extension behavior.
29-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePotential
groupIdcollision on duplicate events.If two events share the same
timeMs,key, and modifier state (e.g., a genuinely repeated keypress at the same timestamp, or a data pipeline producing duplicate samples), they will produce identical ids. Since both could start separate groups (if they fall outsidegroupWindowMsof each other but within the lookback window), the downstream renderer would encounter duplicate React keys.Consider appending a monotonic index from the
relevantarray to guarantee uniqueness while remaining position-independent relative to the sliding window:♻️ Proposed refactor
function groupId(event: KeystrokeEvent): string { const mods = `${event.ctrl ? "c" : ""}${event.alt ? "a" : ""}` + `${event.shift ? "s" : ""}${event.meta ? "m" : ""}`; - return `k${event.timeMs}:${event.key}:${mods}`; + return `k${event.timeMs}:${event.key}:${mods}`; }Alternatively, pass an index into
groupIdfrom therelevantarray iteration:- for (const event of relevant) { + for (let i = 0; i < relevant.length; i++) { + const event = relevant[i]; // ... if (isChord) { groups.push({ - id: groupId(event), + id: groupId(event, i),-function groupId(event: KeystrokeEvent): string { +function groupId(event: KeystrokeEvent, index: number): string { const mods = `${event.ctrl ? "c" : ""}${event.alt ? "a" : ""}` + `${event.shift ? "s" : ""}${event.meta ? "m" : ""}`; - return `k${event.timeMs}:${event.key}:${mods}`; + return `k${event.timeMs}:${event.key}:${mods}:${index}`; }This keeps the id stable across frames (same
relevantordering) while guaranteeing uniqueness.The index from
relevantis stable across frames for the same event becauserelevantis sorted bytimeMs— the same event always occupies the same position among events with identical timestamps. This is a low-risk edge case but worth hardening if duplicate timestamps are possible upstream.🤖 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/videoPlayback/keystrokeOverlay/keystrokeCoalescing.ts` around lines 29 - 34, Update groupId and its relevant-array call site to incorporate the event’s monotonic index from the relevant iteration, ensuring duplicate events with identical timeMs, key, and modifiers receive distinct IDs. Preserve the existing ID components and use the stable relevant ordering so IDs remain consistent across frames.
🤖 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.
Nitpick comments:
In
`@src/components/video-editor/videoPlayback/keystrokeOverlay/keystrokeCoalescing.ts`:
- Around line 86-93: Remove the redundant open guard from the conditional in the
keystroke coalescing logic: since canExtend already includes the open check, use
canExtend alone in the if statement while preserving all existing extension
behavior.
- Around line 29-34: Update groupId and its relevant-array call site to
incorporate the event’s monotonic index from the relevant iteration, ensuring
duplicate events with identical timeMs, key, and modifiers receive distinct IDs.
Preserve the existing ID components and use the stable relevant ordering so IDs
remain consistent across frames.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 092f33a3-13d8-4a14-9f60-4436ee4257d8
📒 Files selected for processing (3)
electron/ipc/cursor/interaction.tssrc/components/video-editor/videoPlayback/keystrokeOverlay/keystrokeCoalescing.test.tssrc/components/video-editor/videoPlayback/keystrokeOverlay/keystrokeCoalescing.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/video-editor/videoPlayback/keystrokeOverlay/keystrokeCoalescing.test.ts
- electron/ipc/cursor/interaction.ts
…Rabbit) canExtend already includes the `open` truthiness check, and TS 4.4+ aliased- condition narrowing keeps `open` narrowed under `if (canExtend)`, so the extra `open &&` was redundant. Behaviour unchanged; 25 tests pass. CodeRabbit also suggested deriving the keycap id from the relevant-array index; skipped intentionally — the lookback window slides, so that index is not stable across frames and would reintroduce the remount/flicker fixed in d741c80. True duplicate events (same time+key+modifiers) are already prevented upstream by the capture-layer auto-repeat collapse. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
|
1 similar comment
|
|
…derer) Completes the keystroke overlay end to end: - VideoEditor: keystrokeTelemetry state + load effect (get-keystroke-telemetry), showKeystrokes/position/size state, props to VideoPlayback + SettingsPanel. - VideoPlayback: mount PixiKeystrokeOverlay in a sibling container, per-frame update (outside the cursor-overlay branch so it renders independently), plus attach/destroy lifecycle; drop a pre-existing unused import. - SettingsPanel: 'Show Keystrokes' toggle + size slider. - useScreenRecorder: pass showKeystrokes to setRecordingState at record start. - i18n (en): effects.showKeystrokes / effects.keystrokesSize. Full-project tsc clean (both configs); 25 keystroke tests pass; Electron dev build boots and renderer renders with zero console errors. Position Select and the other 9 locale strings are trivial follow-ups. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ext recording Add showKeystrokes/keystrokePosition/keystrokeSize to the saveEditorPreferences bundle (+ effect deps). Without this, toggling 'Show Keystrokes' only affected the in-editor overlay; capture is gated at record-start on the persisted pref (loadEditorPreferences().showKeystrokes), so the toggle must persist to actually capture keystrokes on the next recording. Closes the end-to-end loop. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move 'Show Keystrokes' out of the cramped 3-toggle header row into its own full-width labeled row at the top of the cursor panel body, with a full-size switch (was scale-75) so its on/off state is obvious. Header returns to Show Cursor + Loop cursor. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
feat(editor): keystroke overlay — render layer + persistence (Part 2/3)
starkdcc
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
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/SettingsPanel.tsx (1)
1861-1883: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCursor-section "Reset" doesn't restore keystroke settings.
resetCursorSectionresetsshowCursor,loopCursor,cursorStyle, and the various spring/click-effect settings, but the newshowKeystrokes/keystrokesSizecontrols now rendered in this same section (lines 3704-3772) aren't included. Clicking "Reset" will silently leave keystroke visibility/size untouched while everything else in the tab reverts to defaults.🔧 Suggested fix
const resetCursorSection = () => { onShowCursorChange?.(initialEditorPreferences.showCursor); onLoopCursorChange?.(initialEditorPreferences.loopCursor); + onShowKeystrokesChange?.(initialEditorPreferences.showKeystrokes); + onKeystrokesSizeChange?.(initialEditorPreferences.keystrokeSize); onCursorStyleChange?.(initialEditorPreferences.cursorStyle);🤖 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/SettingsPanel.tsx` around lines 1861 - 1883, Update resetCursorSection to also invoke the optional showKeystrokes and keystrokesSize change handlers with their corresponding values from initialEditorPreferences, alongside the existing cursor settings resets.
🧹 Nitpick comments (2)
src/components/video-editor/projectPersistence.ts (1)
109-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid re-declaring the keystroke position union inline.
keystrokePositionre-declares the same literal union already exported asKeystrokeOverlayPositionfromkeystrokeOverlay/keystrokeTypes.ts. Importing and reusing that type avoids silent drift if a position value is ever added/removed.♻️ Suggested fix
+import type { KeystrokeOverlayPosition } from "./videoPlayback/keystrokeOverlay/keystrokeTypes"; ... - keystrokePosition: "bottom-center" | "bottom-left" | "bottom-right" | "top-center"; + keystrokePosition: KeystrokeOverlayPosition;🤖 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/projectPersistence.ts` around lines 109 - 111, Update the project persistence type’s keystrokePosition field to reuse the exported KeystrokeOverlayPosition type from keystrokeOverlay/keystrokeTypes.ts, adding the necessary type import and removing the inline literal union.src/components/video-editor/VideoEditor.tsx (1)
765-769: 🎯 Functional Correctness | 🔵 TrivialPlaceholder keystroke values in preset snapshot — already tracked via TODO.
showKeystrokes/keystrokePosition/keystrokeSizeare hardcoded rather than using the real state defined earlier in this same diff (lines 474-478). The TODO already documents this is deferred until presets manage these settings — just flagging for visibility once that follow-up lands, sinceapplyEditorPresetSnapshot(923-979) correspondingly never restores these fields either.🤖 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 765 - 769, Keep the current placeholder keystroke values and TODO in the preset snapshot for now; no immediate change is required. When preset support is implemented, update the snapshot construction near the keystroke state and applyEditorPresetSnapshot to save and restore showKeystrokes, keystrokePosition, and keystrokeSize using the corresponding VideoEditor state.
🤖 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/ipc/register/recording.ts`:
- Around line 1188-1194: Update finalizeStoredVideo to also persist keystroke
telemetry: after snapping it with snapshotKeystrokeTelemetryForPersistence(),
best-effort call persistPendingKeystrokeTelemetry(videoPath), mirroring the
existing cursor telemetry persistence. Ensure this shared finalization path
flushes keystrokes for all callers without disrupting video finalization.
In `@src/components/video-editor/SettingsPanel.tsx`:
- Around line 765-772: Update SettingsPanel to destructure and render
keystrokesPosition and onKeystrokesPositionChange so the position setting
controls the keystroke overlay instead of relying on its default. Replace the
duplicated position union in SettingsPanelProps with the shared
KeystrokeOverlayPosition type, preserving the existing supported positions.
In
`@src/components/video-editor/videoPlayback/keystrokeOverlay/PixiKeystrokeOverlay.ts`:
- Around line 125-181: Update render and the pooled keycap state used by
obtainKeycap so label.fontSize and label.text are only assigned when their
values differ from the current keycap values. Preserve the existing sizing and
text behavior while preventing redundant PixiJS text/style updates on unchanged
frames.
---
Outside diff comments:
In `@src/components/video-editor/SettingsPanel.tsx`:
- Around line 1861-1883: Update resetCursorSection to also invoke the optional
showKeystrokes and keystrokesSize change handlers with their corresponding
values from initialEditorPreferences, alongside the existing cursor settings
resets.
---
Nitpick comments:
In `@src/components/video-editor/projectPersistence.ts`:
- Around line 109-111: Update the project persistence type’s keystrokePosition
field to reuse the exported KeystrokeOverlayPosition type from
keystrokeOverlay/keystrokeTypes.ts, adding the necessary type import and
removing the inline literal union.
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 765-769: Keep the current placeholder keystroke values and TODO in
the preset snapshot for now; no immediate change is required. When preset
support is implemented, update the snapshot construction near the keystroke
state and applyEditorPresetSnapshot to save and restore showKeystrokes,
keystrokePosition, and keystrokeSize using the corresponding VideoEditor state.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: daa544a7-e947-4c1a-b8fa-60668b795cc4
📒 Files selected for processing (15)
electron/electron-env.d.tselectron/ipc/cursor/telemetry.tselectron/ipc/register/recording.tselectron/ipc/utils.tselectron/preload.tssrc/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/VideoPlayback.tsxsrc/components/video-editor/editorPreferences.tssrc/components/video-editor/projectPersistence.tssrc/components/video-editor/videoPlayback/keystrokeOverlay/PixiKeystrokeOverlay.tssrc/components/video-editor/videoPlayback/keystrokeOverlay/keyLabels.tssrc/components/video-editor/videoPlayback/keystrokeOverlay/keystrokeTypes.tssrc/hooks/useScreenRecorder.tssrc/i18n/locales/en/settings.json
✅ Files skipped from review due to trivial changes (1)
- src/i18n/locales/en/settings.json
| ); | ||
| } | ||
| } else { | ||
| console.log("[stop-native] No separate audio tracks to mux"); | ||
| } | ||
| } else { | ||
| console.log("[stop-native] No separate audio tracks to mux"); | ||
| } | ||
|
|
||
| return await finalizeStoredVideo(finalVideoPath); | ||
| } catch (error) { | ||
| console.error("Failed to stop native ScreenCaptureKit recording:", error); | ||
| const fallbackPath = nativeCaptureTargetPath; | ||
| const fallbackSystemAudioPath = nativeCaptureSystemAudioPath; | ||
| const fallbackMicrophonePath = nativeCaptureMicrophonePath; | ||
| const fallbackFileSizeBytes = await getFileSizeIfPresent(fallbackPath); | ||
| setNativeScreenRecordingActive(false); | ||
| setNativeCaptureProcess(null); | ||
| setNativeCaptureTargetPath(null); | ||
| setNativeCaptureSystemAudioPath(null); | ||
| setNativeCaptureMicrophonePath(null); | ||
| setNativeCaptureStopRequested(false); | ||
| setNativeCapturePaused(false); | ||
| return await finalizeStoredVideo(finalVideoPath); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm keystroke persistence is only wired into the Windows stop path.
rg -nP 'persistPendingKeystrokeTelemetry|snapshotKeystrokeTelemetryForPersistence' electron/ipc
echo '--- finalizeStoredVideo body ---'
ast-grep run --pattern 'export async function finalizeStoredVideo($_) { $$$ }' --lang typescript electron/ipc/recording/mac.tsRepository: webadderallorg/Recordly
Length of output: 6827
🏁 Script executed:
#!/bin/bash
rg -n "finalizeStoredVideo\(" electron/ipc
rg -n "persistPendingKeystrokeTelemetry|snapshotKeystrokeTelemetryForPersistence" electron/ipc/recording electron/ipc/registerRepository: webadderallorg/Recordly
Length of output: 1324
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for range in "1175,1202" "1238,1260" "1478,1525" "1594,1615" "1768,1790"; do
echo "=== electron/ipc/register/recording.ts:$range ==="
sed -n "${range}p" electron/ipc/register/recording.ts
echo
doneRepository: webadderallorg/Recordly
Length of output: 4974
Persist keystrokes in finalizeStoredVideo too
finalizeStoredVideo is the shared stop path for macOS, FFmpeg, browser/import, and fallback flows, but it only snapshots/persists cursor telemetry. Keystrokes are snapped on stop and flushed only in the Windows branch, so recordings that reach these callers can lose their keystroke telemetry. Mirror the existing cursor persistence here with a best-effort snapshotKeystrokeTelemetryForPersistence() + persistPendingKeystrokeTelemetry(videoPath).
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import type { ChildProcessWithoutNullStreams } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 `@electron/ipc/register/recording.ts` around lines 1188 - 1194, Update
finalizeStoredVideo to also persist keystroke telemetry: after snapping it with
snapshotKeystrokeTelemetryForPersistence(), best-effort call
persistPendingKeystrokeTelemetry(videoPath), mirroring the existing cursor
telemetry persistence. Ensure this shared finalization path flushes keystrokes
for all callers without disrupting video finalization.
| showKeystrokes?: boolean; | ||
| onShowKeystrokesChange?: (enabled: boolean) => void; | ||
| keystrokesPosition?: "bottom-center" | "bottom-left" | "bottom-right" | "top-center"; | ||
| onKeystrokesPositionChange?: ( | ||
| position: "bottom-center" | "bottom-left" | "bottom-right" | "top-center", | ||
| ) => void; | ||
| keystrokesSize?: number; | ||
| onKeystrokesSizeChange?: (size: number) => void; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Files\n'
git ls-files src/components/video-editor/{SettingsPanel.tsx,VideoEditor.tsx,keystrokeTypes.ts} || true
printf '\n## Outline SettingsPanel.tsx\n'
ast-grep outline src/components/video-editor/SettingsPanel.tsx --view expanded || true
printf '\n## Outline VideoEditor.tsx\n'
ast-grep outline src/components/video-editor/VideoEditor.tsx --view expanded || true
printf '\n## Outline keystrokeTypes.ts\n'
ast-grep outline src/components/video-editor/keystrokeTypes.ts --view expanded || true
printf '\n## Search keystrokesPosition refs\n'
rg -n "keystrokesPosition|onKeystrokesPositionChange|KeystrokeOverlayPosition|keystrokePosition|setKeystrokePosition" src/components/video-editorRepository: webadderallorg/Recordly
Length of output: 15325
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '740,790p' src/components/video-editor/SettingsPanel.tsx
printf '\n---\n'
sed -n '1200,1235p' src/components/video-editor/SettingsPanel.tsx
printf '\n---\n'
sed -n '3688,3785p' src/components/video-editor/SettingsPanel.tsx
printf '\n---\n'
sed -n '1,220p' src/components/video-editor/keystrokeTypes.ts
printf '\n---\n'
sed -n '1140,1345p' src/components/video-editor/VideoEditor.tsxRepository: webadderallorg/Recordly
Length of output: 7412
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('src/components/video-editor/SettingsPanel.tsx')
text = p.read_text()
for needle in ['keystrokesPosition', 'onKeystrokesPositionChange', 'KeystrokeOverlayPosition', 'showKeystrokes', 'keystrokesSize']:
print(needle, text.count(needle))
PYRepository: webadderallorg/Recordly
Length of output: 272
Expose keystrokesPosition in SettingsPanel.
keystrokesPosition / onKeystrokesPositionChange are declared on SettingsPanelProps, but the panel never destructures or renders them, so the new keystroke overlay stays on the upstream default. Use the shared KeystrokeOverlayPosition type instead of duplicating the union.
🤖 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/SettingsPanel.tsx` around lines 765 - 772, Update
SettingsPanel to destructure and render keystrokesPosition and
onKeystrokesPositionChange so the position setting controls the keystroke
overlay instead of relying on its default. Replace the duplicated position union
in SettingsPanelProps with the shared KeystrokeOverlayPosition type, preserving
the existing supported positions.
| private render(groups: KeycapGroup[], viewport: CursorViewportRect): void { | ||
| // Size everything relative to viewport height so the overlay scales with | ||
| // the rendered video, then apply the user's size multiplier. | ||
| const fontSize = Math.max(10, Math.min(64, viewport.height * 0.032 * this.config.sizeScale)); | ||
| const padX = fontSize * 0.55; | ||
| const capHeight = fontSize * 1.7; | ||
| const capGap = fontSize * 0.32; // gap between keys within a group | ||
| const rowGap = fontSize * 0.4; // gap between stacked groups | ||
| const edgeMargin = fontSize * 1.1; | ||
| const cornerRadius = fontSize * 0.32; | ||
|
|
||
| // Lay out each group as a row of pill-shaped keycaps. | ||
| let poolIndex = 0; | ||
| const rows: { width: number; caps: { index: number; width: number }[] }[] = []; | ||
|
|
||
| for (const group of groups) { | ||
| const caps: { index: number; width: number }[] = []; | ||
| let rowWidth = 0; | ||
| for (const text of group.labels) { | ||
| const index = poolIndex++; | ||
| const keycap = this.obtainKeycap(index); | ||
| keycap.label.style.fontSize = fontSize; | ||
| keycap.label.text = text; | ||
| const capWidth = Math.max(capHeight, keycap.label.width + padX * 2); | ||
| caps.push({ index, width: capWidth }); | ||
| rowWidth += capWidth + capGap; | ||
| } | ||
| rows.push({ width: Math.max(0, rowWidth - capGap), caps }); | ||
| } | ||
|
|
||
| // Hide pooled keycaps not used this frame. | ||
| for (let i = poolIndex; i < this.pool.length; i++) { | ||
| this.pool[i].background.visible = false; | ||
| this.pool[i].label.visible = false; | ||
| } | ||
|
|
||
| const totalHeight = rows.length * capHeight + Math.max(0, rows.length - 1) * rowGap; | ||
| const topY = this.resolveTopY(viewport, totalHeight, edgeMargin); | ||
|
|
||
| rows.forEach((row, rowIndex) => { | ||
| const rowY = topY + rowIndex * (capHeight + rowGap); | ||
| const opacity = groups[rowIndex].opacity; | ||
| let x = this.resolveRowStartX(viewport, row.width, edgeMargin); | ||
|
|
||
| for (const cap of row.caps) { | ||
| const keycap = this.pool[cap.index]; | ||
| keycap.background.clear(); | ||
| keycap.background | ||
| .roundRect(x, rowY, cap.width, capHeight, cornerRadius) | ||
| .fill({ color: KEYCAP_FILL, alpha: 0.82 * opacity }) | ||
| .stroke({ color: KEYCAP_STROKE, width: 1, alpha: 0.9 * opacity }); | ||
| keycap.label.position.set(x + cap.width / 2, rowY + capHeight / 2); | ||
| keycap.label.alpha = opacity; | ||
| x += cap.width + capGap; | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Avoid re-rasterizing keycap text every frame when unchanged.
keycap.label.style.fontSize = fontSize; and keycap.label.text = text; run unconditionally for every visible keycap on every update() call (i.e. every ticker frame while keystrokes are shown), even when neither value changed from the previous frame. Per PixiJS's own docs: "Changing text or style re-rasterizes the object. Avoid doing this every frame unless necessary." With several pooled keycaps potentially active simultaneously, this adds needless canvas re-rasterization + texture upload work on a 60fps hot path alongside the existing zoom/motion-blur filters.
⚡ Proposed fix: skip redundant text/style writes
interface PooledKeycap {
background: Graphics;
label: Text;
+ lastText?: string;
+ lastFontSize?: number;
} for (const text of group.labels) {
const index = poolIndex++;
const keycap = this.obtainKeycap(index);
- keycap.label.style.fontSize = fontSize;
- keycap.label.text = text;
+ if (keycap.lastFontSize !== fontSize) {
+ keycap.label.style.fontSize = fontSize;
+ keycap.lastFontSize = fontSize;
+ }
+ if (keycap.lastText !== text) {
+ keycap.label.text = text;
+ keycap.lastText = text;
+ }
const capWidth = Math.max(capHeight, keycap.label.width + padX * 2);🤖 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/videoPlayback/keystrokeOverlay/PixiKeystrokeOverlay.ts`
around lines 125 - 181, Update render and the pooled keycap state used by
obtainKeycap so label.fontSize and label.text are only assigned when their
values differ from the current keycap values. Preserve the existing sizing and
text behavior while preventing redundant PixiJS text/style updates on unchanged
frames.
1 participant
On-screen keystroke overlay
Shows the keys and shortcut chords you press on screen during a recording — the standard "keycast" feature people expect for tutorials, demos, and bug reports. Fits the "additional editor tools and workflow polish" area in
CONTRIBUTING.md.Status: complete and verified working on real hardware — recording with the toggle on writes a
.keystrokes.jsonsidecar of real captured keys, and the editor renders them as keycaps (chords group, rapid typing merges into a word, groups fade, scrubbing reveals keys at the right time).What's included
keydownlistener on the existing uiohook interaction hook; keycodes resolved to stable semantic tokens via uiohook'sUiohookKeytable; auto-repeat collapsed. Gated on a default-off flag — nothing is recorded unless "Show Keystrokes" is enabled, so secrets typed while recording aren't captured by default..keystrokes.jsonsidecar written on stop, loaded on open, cleaned up on delete — mirroring the cursor-telemetry path exactly. New IPC:get-keystroke-telemetry, plus ashowKeystrokesflag onset-recording-state.PixiKeystrokeOverlay, a self-contained PIXI layer mirroringPixiCursorOverlay's construct/update/reset/destroy contract; pooled Graphics + Text keycaps, screen-anchored, size-scaled.Verification
tscclean (renderer + node configs).fast-checkproperty test); whole suite green (866 tests).Follow-ups (intentionally out of scope here)
SelectUI + the remaining 9 locale strings.Disclosure
Developed with AI assistance (noted in commit trailers). Happy to reshape scope, split into smaller PRs, or address review feedback — the CodeRabbit nitpicks so far are already handled inline.