-
Notifications
You must be signed in to change notification settings - Fork 18
[codex] Add export preview diagnostics #11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
b141dd2
aabdc0d
b1c5ee5
3065705
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -174,6 +174,31 @@ function buildSaveDiagnosticMessage(formatLabel: "GIF" | "Video", reason?: strin | |
| return `${formatLabel} export save failed${reason ? `\nReason: ${reason}` : ""}`; | ||
| } | ||
|
|
||
| function getPreviewVideoDiagnostics(video: HTMLVideoElement | null) { | ||
| if (!video) { | ||
| return { present: false }; | ||
| } | ||
|
|
||
| return { | ||
| present: true, | ||
| src: video.currentSrc || video.src, | ||
| readyState: video.readyState, | ||
| networkState: video.networkState, | ||
| error: video.error | ||
| ? { | ||
| code: video.error.code, | ||
| message: video.error.message, | ||
| } | ||
| : null, | ||
| currentTime: Number.isFinite(video.currentTime) ? video.currentTime : null, | ||
| duration: Number.isFinite(video.duration) ? video.duration : null, | ||
| paused: video.paused, | ||
| ended: video.ended, | ||
| videoWidth: video.videoWidth, | ||
| videoHeight: video.videoHeight, | ||
| }; | ||
| } | ||
|
|
||
| const CAPTION_WORD_CHOICES = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12] as const; | ||
|
|
||
| export default function VideoEditor() { | ||
|
|
@@ -1855,6 +1880,33 @@ export default function VideoEditor() { | |
| setExportError(null); | ||
| setExportedFilePath(null); | ||
|
|
||
| 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, | ||
| }, | ||
| })}`, | ||
| ); | ||
|
Comment on lines
+1883
to
+1908
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win Avoid logging raw source file paths in export traces. These diagnostics include raw Also applies to: 2135-2142 🤖 Prompt for AI Agents |
||
|
|
||
| try { | ||
| const wasPlaying = isPlaying; | ||
| if (wasPlaying) { | ||
|
|
@@ -2080,6 +2132,14 @@ export default function VideoEditor() { | |
| toast.error(t("errors.exportFailedWithError", { error: message })); | ||
| } | ||
| } finally { | ||
| console.info( | ||
| `[VideoEditor] export finished ${JSON.stringify({ | ||
| sourcePath: videoSourcePath ?? videoPath, | ||
| videoPath, | ||
| previewBefore: previewBeforeExport, | ||
| previewAfter: getPreviewVideoDiagnostics(videoPlaybackRef.current?.video ?? null), | ||
| })}`, | ||
| ); | ||
| setIsExporting(false); | ||
| exporterRef.current = null; | ||
| // Reset so the next export can reopen the dialog (second export | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -215,6 +215,26 @@ function enableAllPreviewAudioTracks(video: HTMLVideoElement) { | |
| } | ||
| } | ||
|
|
||
| function getVideoElementDiagnostics(video: HTMLVideoElement) { | ||
| return { | ||
| src: video.currentSrc || video.src, | ||
| readyState: video.readyState, | ||
| networkState: video.networkState, | ||
| error: video.error | ||
| ? { | ||
| code: video.error.code, | ||
| message: video.error.message, | ||
| } | ||
| : null, | ||
| currentTime: Number.isFinite(video.currentTime) ? video.currentTime : null, | ||
| duration: Number.isFinite(video.duration) ? video.duration : null, | ||
| paused: video.paused, | ||
| ended: video.ended, | ||
| videoWidth: video.videoWidth, | ||
| videoHeight: video.videoHeight, | ||
| }; | ||
| } | ||
|
|
||
| const VideoPlayback = forwardRef<VideoPlaybackRef, VideoPlaybackProps>( | ||
| ( | ||
| { | ||
|
|
@@ -345,6 +365,18 @@ const VideoPlayback = forwardRef<VideoPlaybackRef, VideoPlaybackProps>( | |
| const speedRegionsRef = useRef<SpeedRegion[]>([]); | ||
| const motionBlurAmountRef = useRef(motionBlurAmount); | ||
| const cursorOverlayRef = useRef<PixiCursorOverlay | null>(null); | ||
|
|
||
| const logPreviewVideoEvent = useCallback( | ||
| (event: React.SyntheticEvent<HTMLVideoElement>) => { | ||
| console.info( | ||
| `[VideoPlayback] preview video ${event.type} ${JSON.stringify({ | ||
| videoPath, | ||
| diagnostics: getVideoElementDiagnostics(event.currentTarget), | ||
| })}`, | ||
|
Comment on lines
+369
to
+375
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win Redact local paths from preview diagnostics. This payload currently emits both 🤖 Prompt for AI Agents |
||
| ); | ||
| }, | ||
| [videoPath], | ||
| ); | ||
| const showCursorRef = useRef(showCursor); | ||
| const cursorSizeRef = useRef(cursorSize); | ||
| const cursorSmoothingRef = useRef(cursorSmoothing); | ||
|
|
@@ -2152,7 +2184,14 @@ const VideoPlayback = forwardRef<VideoPlaybackRef, VideoPlaybackProps>( | |
| forceResolveDuration(e.currentTarget); | ||
| } | ||
| }} | ||
| onError={() => onError("Failed to load video")} | ||
| onEmptied={logPreviewVideoEvent} | ||
| onStalled={logPreviewVideoEvent} | ||
| onSuspend={logPreviewVideoEvent} | ||
| onAbort={logPreviewVideoEvent} | ||
| onError={(event) => { | ||
| logPreviewVideoEvent(event); | ||
| onError("Failed to load video"); | ||
| }} | ||
| /> | ||
| {supplementalAudioPath && ( | ||
| <audio ref={supplementalAudioRef} src={supplementalAudioPath} preload="auto" /> | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,6 +4,27 @@ import App from "./App.tsx"; | |||||||||||||||||||||||||
| import { I18nProvider } from "./contexts/I18nContext"; | ||||||||||||||||||||||||||
| import "./index.css"; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // === One-shot uncaught trap — capture stack traces for the .geometry null deref in #8. | ||||||||||||||||||||||||||
| // Delete after the diagnostic run that closes out the issue's root cause. | ||||||||||||||||||||||||||
| const __errTrap = (tag: string) => (e: ErrorEvent | PromiseRejectionEvent) => { | ||||||||||||||||||||||||||
| const isRej = "reason" in e; | ||||||||||||||||||||||||||
| const err = isRej ? (e as PromiseRejectionEvent).reason : (e as ErrorEvent).error; | ||||||||||||||||||||||||||
| const evt = e as ErrorEvent; | ||||||||||||||||||||||||||
| console.error( | ||||||||||||||||||||||||||
| `[VideoEditor] ${tag}`, | ||||||||||||||||||||||||||
| JSON.stringify({ | ||||||||||||||||||||||||||
| message: err?.message ?? String(err), | ||||||||||||||||||||||||||
| source: isRej ? null : evt.filename, | ||||||||||||||||||||||||||
| lineno: isRej ? null : evt.lineno, | ||||||||||||||||||||||||||
| colno: isRej ? null : evt.colno, | ||||||||||||||||||||||||||
| stack: err?.stack ?? null, | ||||||||||||||||||||||||||
|
Comment on lines
+16
to
+20
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Fall back to On 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
| window.addEventListener("error", __errTrap("uncaught")); | ||||||||||||||||||||||||||
| window.addEventListener("unhandledrejection", __errTrap("unhandledrejection")); | ||||||||||||||||||||||||||
| // === end trap === | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| const windowType = new URLSearchParams(window.location.search).get("windowType") || ""; | ||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||
| windowType === "hud-overlay" || | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
📝 Committable suggestion
🤖 Prompt for AI Agents