[codex] Add export preview diagnostics#11
Open
EtienneLescot wants to merge 4 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds structured diagnostics around renderer logs, runtime errors, preview video events, and export start/finish state snapshots. ChangesDiagnostic instrumentation for video export debugging
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
1 task
EtienneLescot
pushed a commit
that referenced
this pull request
Jun 22, 2026
Adds `webContents.on('console-message', …)` forwarding on every
browser window created in `electron/main.ts`. Renderer
`console.info` / `console.warn` / `console.error` calls now show
up in the terminal where `npm run dev` is running, prefixed with
`[renderer INFO|WARNING|ERROR]` so they are easy to grep out from
the surrounding OpenH264 / Wayland noise.
This makes the export-preview diagnostics from PR #11 actually
visible to users following terminal instructions — without it the
`[VideoEditor]` / `[VideoPlayback]` lines only land in DevTools,
which is exactly the gap that caused the first Manjaro run to
produce no usable data (cfinnberg, issue #8 comment 4757223195).
Uses the non-deprecated event-based signature
(`details: { level, message, … }`) with `level` already typed as
"info" | "warning" | "error" | "debug".
Smoke-tested locally with a minimal Electron script: `console.info`,
`console.warn`, and `console.error` from a `data:` URL all
appear on stdout.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
EtienneLescot
pushed a commit
that referenced
this pull request
Jun 22, 2026
Adds `webContents.on('console-message', …)` forwarding on every
browser window created in `electron/main.ts`. Renderer
`console.info` / `console.warn` / `console.error` calls now show
up in the terminal where `npm run dev` is running, prefixed with
`[renderer INFO|WARNING|ERROR]` so they are easy to grep out from
the surrounding OpenH264 / Wayland noise.
This makes the export-preview diagnostics from PR #11 actually
visible to users following terminal instructions — without it the
`[VideoEditor]` / `[VideoPlayback]` lines only land in DevTools,
which is exactly the gap that caused the first Manjaro run to
produce no usable data (cfinnberg, issue #8 comment 4757223195).
Uses the non-deprecated event-based signature
(`details: { level, message, … }`) with `level` already typed as
"info" | "warning" | "error" | "debug".
Smoke-tested locally with a minimal Electron script: `console.info`,
`console.warn`, and `console.error` from a `data:` URL all
appear on stdout.
EtienneLescot
force-pushed
the
codex/export-preview-diagnostics
branch
from
13878d6 to
1b8e210
Compare
![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
🤖 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/main.ts`:
- Around line 134-150: The renderer console message listener in the
"browser-window-created" event handler is enabled unconditionally for all
windows, which can leak sensitive user data (paths, state, diagnostics) into
terminal and system logs during normal operation. Wrap the
window.webContents.on("console-message") listener registration inside a
conditional check that only enables it when an explicit diagnostic flag is set,
and ensure this flag is disabled by default in packaged builds. This makes
console forwarding opt-in rather than always-on.
🪄 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: 844f6f36-b459-435a-9dda-48d59a79c5a9
📒 Files selected for processing (3)
electron/main.tssrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/VideoPlayback.tsx
Comment on lines
+134
to
+150
| app.on("browser-window-created", (_event, window) => { | ||
| window.webContents.on("console-message", (details) => { | ||
| // New API: details.level is "info" | "warning" | "error" | "debug". | ||
| // Skip debug to keep the terminal readable; the user can still inspect | ||
| // DevTools for that. | ||
| const { level, message } = details; | ||
| if (level === "debug") return; | ||
| const tag = level.toUpperCase(); | ||
| const line = `[renderer ${tag}] ${message}`; | ||
| if (level === "error") { | ||
| console.error(line); | ||
| } else if (level === "warning") { | ||
| console.warn(line); | ||
| } else { | ||
| console.log(line); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Gate renderer-console forwarding behind an explicit diagnostic flag.
On Line 134, this is enabled unconditionally for every window. Since renderer logs include user paths/state (e.g., export diagnostics), this can leak sensitive data into terminal/system logs in normal runs. Make forwarding opt-in (or dev-only) and keep it disabled by default in packaged builds.
Suggested patch
- app.on("browser-window-created", (_event, window) => {
- window.webContents.on("console-message", (details) => {
+const shouldForwardRendererConsole =
+ !app.isPackaged || process.env.OPENSCREEN_ENABLE_RENDERER_CONSOLE_FORWARDING === "1";
+
+if (shouldForwardRendererConsole) {
+ app.on("browser-window-created", (_event, window) => {
+ window.webContents.on("console-message", (details) => {
// New API: details.level is "info" | "warning" | "error" | "debug".
// Skip debug to keep the terminal readable; the user can still inspect
// DevTools for that.
const { level, message } = details;
if (level === "debug") return;
const tag = level.toUpperCase();
const line = `[renderer ${tag}] ${message}`;
if (level === "error") {
console.error(line);
} else if (level === "warning") {
console.warn(line);
} else {
console.log(line);
}
- });
-});
+ });
+ });
+}📝 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
| app.on("browser-window-created", (_event, window) => { | |
| window.webContents.on("console-message", (details) => { | |
| // New API: details.level is "info" | "warning" | "error" | "debug". | |
| // Skip debug to keep the terminal readable; the user can still inspect | |
| // DevTools for that. | |
| const { level, message } = details; | |
| if (level === "debug") return; | |
| const tag = level.toUpperCase(); | |
| const line = `[renderer ${tag}] ${message}`; | |
| if (level === "error") { | |
| console.error(line); | |
| } else if (level === "warning") { | |
| console.warn(line); | |
| } else { | |
| console.log(line); | |
| } | |
| }); | |
| const shouldForwardRendererConsole = | |
| !app.isPackaged || process.env.OPENSCREEN_ENABLE_RENDERER_CONSOLE_FORWARDING === "1"; | |
| if (shouldForwardRendererConsole) { | |
| app.on("browser-window-created", (_event, window) => { | |
| window.webContents.on("console-message", (details) => { | |
| // New API: details.level is "info" | "warning" | "error" | "debug". | |
| // Skip debug to keep the terminal readable; the user can still inspect | |
| // DevTools for that. | |
| const { level, message } = details; | |
| if (level === "debug") return; | |
| const tag = level.toUpperCase(); | |
| const line = `[renderer ${tag}] ${message}`; | |
| if (level === "error") { | |
| console.error(line); | |
| } else if (level === "warning") { | |
| console.warn(line); | |
| } else { | |
| console.log(line); | |
| } | |
| }); | |
| }); | |
| } |
🤖 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/main.ts` around lines 134 - 150, The renderer console message
listener in the "browser-window-created" event handler is enabled
unconditionally for all windows, which can leak sensitive user data (paths,
state, diagnostics) into terminal and system logs during normal operation. Wrap
the window.webContents.on("console-message") listener registration inside a
conditional check that only enables it when an explicit diagnostic flag is set,
and ensure this flag is disabled by default in packaged builds. This makes
console forwarding opt-in rather than always-on.
Adds `webContents.on('console-message', …)` forwarding on every
browser window created in `electron/main.ts`. Renderer
`console.info` / `console.warn` / `console.error` calls now show
up in the terminal where `npm run dev` is running, prefixed with
`[renderer INFO|WARNING|ERROR]` so they are easy to grep out from
the surrounding OpenH264 / Wayland noise.
This makes the export-preview diagnostics from PR #11 actually
visible to users following terminal instructions — without it the
`[VideoEditor]` / `[VideoPlayback]` lines only land in DevTools,
which is exactly the gap that caused the first Manjaro run to
produce no usable data (cfinnberg, issue #8 comment 4757223195).
Uses the non-deprecated event-based signature
(`details: { level, message, … }`) with `level` already typed as
"info" | "warning" | "error" | "debug".
Smoke-tested locally with a minimal Electron script: `console.info`,
`console.warn`, and `console.error` from a `data:` URL all
appear on stdout.
…] logs The console-message event in Electron's webContents only delivers the first argument as a string in details.message. The original PR #11 passed the diagnostic object as the second argument to console.info, which got collapsed to [object Object] in the forwarded log line - defeating the whole point of capturing structured diagnostic data. This folds the payload into the first argument via template literal + JSON.stringify, so the structured src/readyState/networkState/error/ dimensions/duration/currentTime reaches stdout through the existing console-message forwarding. Also rebase onto current main so the branch keeps moving forward.
EtienneLescot
force-pushed
the
codex/export-preview-diagnostics
branch
from
1b8e210 to
b1c5ee5
Compare
…eref Mirrors the cfinnberg-reported failure log by capturing filename, lineno, colno, and stack from window 'error' and 'unhandledrejection' events so the next repro run produces a stack trace, not just the bare TypeError message. One-shot diagnostic — to be removed once the .geometry null deref is pinned and fixed.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 1883-1908: The export trace in VideoEditor’s export logging is
serializing raw filesystem paths and preview metadata with src values, which can
leak local file details. Update the console.info diagnostics in the export flow
(including the previewBeforeExport/previewAfter data and the export payload) to
redact path-like fields using getFileNameForDiagnostics() or equivalent before
JSON.stringify. Apply the same sanitization in the other export trace block
referenced by the comment so both logs consistently omit raw sourcePath,
videoPath, and webcamVideoPath values.
In `@src/components/video-editor/VideoPlayback.tsx`:
- Around line 369-375: The preview logging in logPreviewVideoEvent is leaking
local file paths by emitting videoPath and diagnostics.src verbatim. Update the
VideoPlayback preview event logging to redact local file:// sources before
calling console.info, replacing the raw path with a basename or a stable token
while keeping the rest of getVideoElementDiagnostics output intact. Ensure any
reference to videoPath or diagnostics.src in this callback is sanitized so
terminal logs do not expose user home-directory details.
In `@src/main.tsx`:
- Around line 16-20: The window error handler in main.tsx is using err?.message
as the only message source, which logs undefined/null when event.error is
missing on non-rejection ErrorEvent cases. Update the error-trap logic around
the event handling to fall back to evt.message for non-rejection errors before
String(err), while keeping the existing source, lineno, colno, and stack
behavior in the same handler.
🪄 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: 2a0e345d-7e65-445a-9b9e-adc1da3fd770
📒 Files selected for processing (4)
electron/main.tssrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/VideoPlayback.tsxsrc/main.tsx
Comment on lines
+1883
to
+1908
| const previewBeforeExport = getPreviewVideoDiagnostics(video); | ||
| console.info( | ||
| `[VideoEditor] export started ${JSON.stringify({ | ||
| format: settings.format, | ||
| sourcePath: videoSourcePath ?? videoPath, | ||
| videoPath, | ||
| webcamVideoPath, | ||
| preview: previewBeforeExport, | ||
| settings: { | ||
| exportQuality: settings.quality || exportQuality, | ||
| aspectRatio, | ||
| padding, | ||
| borderRadius, | ||
| shadowIntensity, | ||
| showBlur, | ||
| motionBlurAmount, | ||
| cropRegion, | ||
| zoomRegions: zoomRegions.length, | ||
| trimRegions: trimRegions.length, | ||
| speedRegions: speedRegions.length, | ||
| annotations: annotationRegions.length, | ||
| effectiveShowCursor, | ||
| cursorSize, | ||
| }, | ||
| })}`, | ||
| ); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Avoid logging raw source file paths in export traces.
These diagnostics include raw sourcePath / videoPath / webcamVideoPath, and previewBefore / previewAfter also carry src. Because these traces are meant to be shared, they can leak local filesystem details. Please reuse getFileNameForDiagnostics() (or similar redaction) before serializing.
Also applies to: 2135-2142
🤖 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 1883 - 1908, The
export trace in VideoEditor’s export logging is serializing raw filesystem paths
and preview metadata with src values, which can leak local file details. Update
the console.info diagnostics in the export flow (including the
previewBeforeExport/previewAfter data and the export payload) to redact
path-like fields using getFileNameForDiagnostics() or equivalent before
JSON.stringify. Apply the same sanitization in the other export trace block
referenced by the comment so both logs consistently omit raw sourcePath,
videoPath, and webcamVideoPath values.
Comment on lines
+369
to
+375
| const logPreviewVideoEvent = useCallback( | ||
| (event: React.SyntheticEvent<HTMLVideoElement>) => { | ||
| console.info( | ||
| `[VideoPlayback] preview video ${event.type} ${JSON.stringify({ | ||
| videoPath, | ||
| diagnostics: getVideoElementDiagnostics(event.currentTarget), | ||
| })}`, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Redact local paths from preview diagnostics.
This payload currently emits both videoPath and diagnostics.src verbatim. For local file:// sources that exposes usernames/home-directory paths in terminal logs that users may paste into bug reports. Log a basename or stable redacted token instead.
🤖 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.tsx` around lines 369 - 375, The
preview logging in logPreviewVideoEvent is leaking local file paths by emitting
videoPath and diagnostics.src verbatim. Update the VideoPlayback preview event
logging to redact local file:// sources before calling console.info, replacing
the raw path with a basename or a stable token while keeping the rest of
getVideoElementDiagnostics output intact. Ensure any reference to videoPath or
diagnostics.src in this callback is sanitized so terminal logs do not expose
user home-directory details.
Comment on lines
+16
to
+20
| message: err?.message ?? String(err), | ||
| source: isRej ? null : evt.filename, | ||
| lineno: isRej ? null : evt.lineno, | ||
| colno: isRej ? null : evt.colno, | ||
| stack: err?.stack ?? null, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fall back to ErrorEvent.message for non-rejection errors.
On window "error" events, event.error is often missing even when event.message, filename, and line info are populated. The current fallback logs "undefined"/"null" in that case, which defeats the diagnostic value of this trap for a common browser error shape.
Suggested fix
JSON.stringify({
- message: err?.message ?? String(err),
+ message: isRej
+ ? err?.message ?? String(err)
+ : err?.message ?? evt.message ?? String(err),
source: isRej ? null : evt.filename,
lineno: isRej ? null : evt.lineno,
colno: isRej ? null : evt.colno,
stack: err?.stack ?? null,📝 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
| message: err?.message ?? String(err), | |
| source: isRej ? null : evt.filename, | |
| lineno: isRej ? null : evt.lineno, | |
| colno: isRej ? null : evt.colno, | |
| stack: err?.stack ?? null, | |
| message: isRej | |
| ? err?.message ?? String(err) | |
| : err?.message ?? evt.message ?? String(err), | |
| source: isRej ? null : evt.filename, | |
| lineno: isRej ? null : evt.lineno, | |
| colno: isRej ? null : evt.colno, | |
| stack: err?.stack ?? null, |
🤖 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/main.tsx` around lines 16 - 20, The window error handler in main.tsx is
using err?.message as the only message source, which logs undefined/null when
event.error is missing on non-rejection ErrorEvent cases. Update the error-trap
logic around the event handling to fall back to evt.message for non-rejection
errors before String(err), while keeping the existing source, lineno, colno, and
stack behavior in the same handler.
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
1 participant
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.
Summary
src,readyState,networkState, media error, time, duration, and decoded dimensions.emptied,stalled,suspend,abort,error) so affected users can send actionable traces even when maintainers cannot reproduce locally.console.info/console.warn/console.erroroutput to the main process so the diagnostics reach the terminal wherenpm run devis running. Without this, renderer logs only show up in DevTools, which is invisible to users following the steps below.Fixes #8
Why
I cannot reproduce the Manjaro-specific preview loss locally. The fast path is not expected to cover the affected Linux recording flow because Linux/browser recordings are saved as WebM, while the current source-copy fast path only accepts readable MP4 sources. This instrumentation targets the normal export path instead.
The renderer console-message forwarding was added after the first run on Manjaro produced no
[VideoEditor]lines in the terminal — Electron renderer console output goes to DevTools, not stdout, by default. The new forwarding useswebContents.on("console-message", …)with the non-deprecated event-based API and prefixes every message with[renderer INFO|WARNING|ERROR]so it is easy to grep out from the surrounding OpenH264 / Wayland noise.How to generate diagnostics
git clone https://github.com/EtienneLescot/openscreen.git cd openscreenIf Electron fails with a
chrome-sandboxpermission error, run this once and start again:They are prefixed with
[renderer INFO](orWARNING/ERROR) because the main process forwards every renderer console call — ignore any other renderer lines that are not part of the export flow..webmor.mp4It is safe to ignore noisy terminal messages about DBus, dconf, or OpenH264 unless they appear right next to the
[VideoEditor]or[VideoPlayback]diagnostic lines.Validation
npx tsc --noEmitnpx biome check electron/main.tsdata:URL callingconsole.info/console.warn/console.errorand forwarding via the same handler shows all three lines on stdout.npm run build-viteSummary by CodeRabbit