fix(wgc): capture per-step stop timing, bump timeout, add diagnostic tool#36
Merged
Conversation
First of three PRs for the docs site effort tracked in ROADMAP.md (tier 'Site & documentation', see PR #32). - website/: minimal Docusaurus 3 site (TS config, Infima-tweaked theme, hero landing with WIP badge, intro doc only). - .github/workflows/docs.yml: build on PR, deploy to GitHub Pages on push to main. Concurrency group, minimal permissions. - website/postcss.config.cjs: overrides the monorepo Tailwind config so Docusaurus CSS pipeline doesn't try to load Tailwind. PR #2 will polish the landing (full bento, demo, footer polish). PR #3 will migrate docs/ -> website/docs/.
…tool Issue #34: Windows 10 recording stop times out at 15s with no useful data. Three changes: 1. The stop timeout (electron/ipc/handlers.ts:415) was 15s, shorter than the macOS equivalent at 30s. Bumped to 60s so the Media Foundation SinkWriter::Finalize has room to drain on slow encoders. 2. Added per-step elapsed-time logs ([stop-timing] step=... elapsed_ms=...) around the cleanup chain in main.cpp so future stop hangs pinpoint which step is slow. These go to stderr, which Electron already captures into nativeWindowsCaptureOutput. 3. Save Diagnostics (electron/ipc/handlers.ts) previously hardcoded logs: [] in the renderer; now includes helperOutput (helper stdout + stderr, capped at 64KB) and mainProcessLogs (a console ring buffer populated when OPENSCREEN_DIAGNOSTIC=1). 4. New scripts/diagnostic-tool/ runs the helper outside the Electron app, captures [stop-timing] lines, writes a structured JSON report. Used as a standalone diagnostic bundle attached to bug reports. 5. .github/workflows/diagnostic-artifact.yml builds per-platform diagnostic zips on every push to main, retained 14 days. Windows job smoke-tests the bundle before upload. Replaces scripts/repro-stop-hang.mjs and scripts/wgc-diagnostic-bundle/ which were scratch tools; the new diagnostic.mjs subsumes both.
📝 WalkthroughWalkthroughAdds diagnostic capture for Electron and native helpers, a standalone diagnostic CLI with packaging workflows, and a new Docusaurus website with Pages deployment. ChangesDiagnostic capture and artifact flow
Website setup and deployment
Sequence Diagram(s)sequenceDiagram
participant diagnostic.mjs
participant NativeHelper
participant JSONReport
diagnostic.mjs->>NativeHelper: spawn with config JSON
diagnostic.mjs->>NativeHelper: send stop after --duration
NativeHelper-->>diagnostic.mjs: stdout, stderr, [stop-timing] lines
diagnostic.mjs->>JSONReport: write parsed timings and helper output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
1 task
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (5)
electron/ipc/handlers.ts (1)
2895-2899: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueDiagnostic payload may embed user-identifying strings.
helperOutputandmainProcessLogscan contain absolute paths (which include the OS username), selected window/display names, and microphone/webcam device names. The save is user-initiated and the buffer is gated, so this is acceptable, but since these files are intended to be attached to upstream bug reports, consider a brief redaction pass (e.g., scrub the home-directory prefix from paths) or a note in the save dialog so reporters know what is being shared.🤖 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/handlers.ts` around lines 2895 - 2899, The diagnostic payload assembled in the IPC save flow can include user-identifying strings from helperOutput and mainProcessLogs. Update the save/report path in the IPC handler to either add a lightweight redaction step (for example, scrub home-directory prefixes and other obvious local identifiers before writing) or clearly warn in the save dialog that the attached logs may contain such details. Keep the change localized around the diagnostic payload construction and save dialog code so the behavior is easy to locate later.website/README.md (1)
7-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
npm ciin the setup instructions.The
.github/workflows/docs.ymlbuild job already installswebsite/withnpm ci, sonpm installcan drift from the dependency graph the site is built and tested against.Proposed fix
- npm install + npm ci🤖 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 `@website/README.md` around lines 7 - 11, Update the setup steps in the README to use npm ci instead of npm install so the website instructions match the locked dependency graph used by the docs build. Keep the existing flow around cd website and npm run dev, and adjust the install step in the setup snippet only.website/src/pages/index.tsx (1)
27-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a Docusaurus internal link for the docs CTA.
href="/openscreen/docs/intro"hardcodes the currentbaseUrlfromwebsite/docusaurus.config.ts, so this page becomes the first place that drifts if the site path changes. Route this through Docusaurus instead.Suggested change
+import Link from "`@docusaurus/Link`"; import useDocusaurusContext from "`@docusaurus/useDocusaurusContext`"; import Layout from "`@theme/Layout`"; import Heading from "`@theme/Heading`"; @@ - <a className={styles.secondaryCta} href="/openscreen/docs/intro"> - Read the docs - </a> + <Link className={styles.secondaryCta} to="/docs/intro"> + Read the docs + </Link>🤖 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 `@website/src/pages/index.tsx` around lines 27 - 35, The docs CTA in the homepage JSX uses a hardcoded site path, which can drift from the configured base URL. Update the secondary CTA in the index page to use Docusaurus routing instead of a raw href, using the existing link handling in the homepage component so it follows the configured baseUrl automatically.scripts/diagnostic-tool/README.md (1)
23-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: add a language to fenced code blocks.
markdownlint flags MD040 on these fences. Use
text(orconsole) for the usage/layout blocks to satisfy the linter.Also applies to: 52-52
🤖 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 `@scripts/diagnostic-tool/README.md` at line 23, The README’s fenced code blocks in the diagnostic tool usage/layout sections are missing a language tag, which triggers markdownlint MD040. Update the affected fences in the README content to specify an appropriate language such as text or console for the blocks around the documented examples, including the other referenced fence occurrence, so the linter accepts them.Source: Linters/SAST tools
.github/workflows/diagnostic-artifact.yml (1)
18-20: 🔒 Security & Privacy | 🔵 TrivialPin the workflow actions and disable checkout credential persistence
actions/checkout@v4andactions/setup-node@v4are still floating tags here. Addpersist-credentials: falseto both checkout steps and pin these actions to full commit SHAs; the rest of the workflows in this repo use the same unpinned pattern, so this is a repo-wide hardening change.🤖 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 @.github/workflows/diagnostic-artifact.yml around lines 18 - 20, The workflow still uses floating action tags and keeps checkout credentials persisted, so harden the diagnostic artifact job by updating the two action references in the workflow to full commit SHAs and adding persist-credentials: false to each actions/checkout step. Apply the same pattern wherever this workflow uses actions/checkout or actions/setup-node so the repo is consistent with the intended pinned-action hardening.Source: Linters/SAST tools
🤖 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 @.github/workflows/diagnostic-artifact.yml:
- Around line 44-53: The Windows smoke test in the Smoke-test the bundle step is
running on a non-interactive runner, which can prevent WGC capture from
acquiring a display and cause the job to fail before stopTiming is written.
Update this step in diagnostic-artifact.yml to run on an interactive self-hosted
Windows runner, or relax/remove the stopTiming.Count gate in the smoke-test
logic so the workflow does not depend on interactive display capture. Use the
existing Smoke-test the bundle step and the diagnostic.bat invocation as the
place to make the change.
In @.github/workflows/docs.yml:
- Around line 30-31: The workflow currently uses floating action refs in the
docs pipeline, so update every `uses:` entry in the workflow to an immutable
commit SHA instead of tags like `actions/checkout@v4` and
`actions/setup-node@v4`. Make the same pinning change for the other affected
`uses:` steps in this workflow as well, keeping the existing action names but
replacing each version tag with the corresponding commit SHA.
In `@scripts/diagnostic-tool/diagnostic.bat`:
- Around line 1-3: The batch launcher in diagnostic.bat is using LF-only
endings; convert the file to CRLF so Windows cmd parsing stays reliable, and
ensure it remains pinned to CRLF by updating the repository’s line-ending rules
for diagnostic.bat (for example via the file’s Git attributes). Focus the fix on
the diagnostic.bat launcher so its ending is preserved across checkouts and
commits.
In `@scripts/diagnostic-tool/diagnostic.mjs`:
- Around line 50-51: The `--duration` handling in `diagnostic.mjs` can turn
missing or invalid input into `NaN`, which then causes the timeout to fire
immediately; update the argument parsing around the `opts.duration` assignment
to validate the value before storing it. In the CLI parsing branch for
`--duration`/`-d`, check that the next argv entry exists and parses to a finite
numeric value, and only then convert it to milliseconds; otherwise report a
clear usage/error message and avoid scheduling the stop timer with an invalid
delay.
In `@website/docusaurus.config.ts`:
- Around line 1-3: Normalize the Docusaurus config to satisfy Biome by fixing
the import ordering at the top of the file and reformatting the wrapped editUrl
configuration block. Update the Config/Preset imports in the same sorted style
Biome expects, and then run the auto-fix on the config object so the editUrl
assignment is kept on a single normalized line or wrapped consistently. Use the
docusaurus config export as the target for the formatting changes.
In `@website/package.json`:
- Around line 28-39: Reformat the package manifest so it matches Biome’s
expected formatting: adjust the browserslist configuration in the package.json
manifest so the production array is laid out consistently with the rest of the
file, and ensure the file ends with a trailing newline. Use the package.json
manifest and its browserslist/engines sections as the reference points.
In `@website/postcss.config.cjs`:
- Around line 3-5: The PostCSS config needs formatting cleanup so Biome stops
failing on the closing brace/trailing newline. Update the module.exports object
in the PostCSS config to match the repo’s formatting conventions and ensure the
file ends with the correct trailing newline, using the existing module.exports
symbol as the reference point.
In `@website/sidebars.ts`:
- Line 9: Run the formatter on the sidebars module so Biome inserts the missing
trailing newline after the exported `sidebars` default export. Update the
`export default sidebars;` statement formatting to match the rest of the file
and ensure the file ends with the expected newline.
In `@website/src/pages/index.tsx`:
- Around line 1-5: The imports and JSX in the index page need to be reformatted
to satisfy Biome. Run organize-imports and formatting on the component in
index.tsx, and make sure the Layout/Heading usage and related JSX in the page
component are rewritten into the formatter’s preferred style without changing
behavior.
In `@website/tsconfig.json`:
- Around line 1-6: The tsconfig JSON is missing the required trailing newline at
EOF, which Biome flags during CI. Update the website tsconfig file so it ends
with a single newline after the closing brace; this is a formatting-only fix and
can be verified against the JSON document itself.
---
Nitpick comments:
In @.github/workflows/diagnostic-artifact.yml:
- Around line 18-20: The workflow still uses floating action tags and keeps
checkout credentials persisted, so harden the diagnostic artifact job by
updating the two action references in the workflow to full commit SHAs and
adding persist-credentials: false to each actions/checkout step. Apply the same
pattern wherever this workflow uses actions/checkout or actions/setup-node so
the repo is consistent with the intended pinned-action hardening.
In `@electron/ipc/handlers.ts`:
- Around line 2895-2899: The diagnostic payload assembled in the IPC save flow
can include user-identifying strings from helperOutput and mainProcessLogs.
Update the save/report path in the IPC handler to either add a lightweight
redaction step (for example, scrub home-directory prefixes and other obvious
local identifiers before writing) or clearly warn in the save dialog that the
attached logs may contain such details. Keep the change localized around the
diagnostic payload construction and save dialog code so the behavior is easy to
locate later.
In `@scripts/diagnostic-tool/README.md`:
- Line 23: The README’s fenced code blocks in the diagnostic tool usage/layout
sections are missing a language tag, which triggers markdownlint MD040. Update
the affected fences in the README content to specify an appropriate language
such as text or console for the blocks around the documented examples, including
the other referenced fence occurrence, so the linter accepts them.
In `@website/README.md`:
- Around line 7-11: Update the setup steps in the README to use npm ci instead
of npm install so the website instructions match the locked dependency graph
used by the docs build. Keep the existing flow around cd website and npm run
dev, and adjust the install step in the setup snippet only.
In `@website/src/pages/index.tsx`:
- Around line 27-35: The docs CTA in the homepage JSX uses a hardcoded site
path, which can drift from the configured base URL. Update the secondary CTA in
the index page to use Docusaurus routing instead of a raw href, using the
existing link handling in the homepage component so it follows the configured
baseUrl automatically.
🪄 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: 0082474f-65b7-4edb-8b68-d29fd082ca86
⛔ Files ignored due to path filters (2)
website/package-lock.jsonis excluded by!**/package-lock.jsonwebsite/static/img/logo.svgis excluded by!**/*.svg
📒 Files selected for processing (23)
.github/workflows/diagnostic-artifact.yml.github/workflows/docs.ymlelectron/diagnostics/main-log-buffer.test.tselectron/diagnostics/main-log-buffer.tselectron/ipc/handlers.tselectron/main.tselectron/native/wgc-capture/src/main.cpppackage.jsonscripts/diagnostic-tool/README.mdscripts/diagnostic-tool/diagnostic.batscripts/diagnostic-tool/diagnostic.mjsscripts/diagnostic-tool/diagnostic.shwebsite/.gitignorewebsite/README.mdwebsite/docs/intro.mdwebsite/docusaurus.config.tswebsite/package.jsonwebsite/postcss.config.cjswebsite/sidebars.tswebsite/src/css/custom.csswebsite/src/pages/index.module.csswebsite/src/pages/index.tsxwebsite/tsconfig.json
- ci: Windows smoke test now validates bundle structure and the diagnostic.bat --help exit code instead of running capture. The non-interactive runner has no display, so WGC cannot acquire frames and the helper exits before [stop-timing] lines appear (verified locally). Real users exercise the capture path on their own boxes. - tool: --duration / --output / --source now validate their values and exit with a clear error on missing or non-numeric input, rather than silently turning into NaN and firing the stop timer immediately. - tool: diagnostic.bat is now CRLF-terminated so Windows cmd parses it reliably. - docs: tag fenced code blocks in the diagnostic-tool README with 'text' to satisfy markdownlint MD040.
Collaborator
Author
|
CodeRabbit review triage — addressed in 2780203: Real, fixed in my code:
Pre-merge checks not addressed here:
|
Mechanical biome --write pass on the Docusaurus scaffolding files introduced in cce4524. No behavior change; resolves CodeRabbit formatting comments and unblocks the lint job.
Resolves the last remaining CodeRabbit review comment on PR #36. Pins actions/checkout, actions/setup-node, actions/upload-pages-artifact, and actions/deploy-pages to their current v4/v3 commit SHAs with inline version comments for grep-ability.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/docs.yml (1)
30-30: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDisable credential persistence for
actions/checkout.This job executes repo-controlled scripts on
pull_requestafter checkout, so keeping the default token in git config makes the read-scopedGITHUB_TOKENunnecessarily exposable. Addpersist-credentials: falseon Line 30 and only opt back in where a later step actually needs authenticated git.Suggested change
- - uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 + - uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 + with: + persist-credentials: false🤖 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 @.github/workflows/docs.yml at line 30, The docs workflow checkout step is leaving the default Git credentials persisted, which is unnecessary for this job. Update the `actions/checkout` step in the docs workflow to set `persist-credentials: false`, and only re-enable credential persistence in any later step that truly needs authenticated git access. Use the existing `actions/checkout` job step as the anchor for the change.Source: Linters/SAST tools
🤖 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.
Duplicate comments:
In @.github/workflows/docs.yml:
- Line 30: The docs workflow checkout step is leaving the default Git
credentials persisted, which is unnecessary for this job. Update the
`actions/checkout` step in the docs workflow to set `persist-credentials:
false`, and only re-enable credential persistence in any later step that truly
needs authenticated git access. Use the existing `actions/checkout` job step as
the anchor for the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 318b3c75-579a-485e-a8b4-6506abf4d17b
📒 Files selected for processing (1)
.github/workflows/docs.yml
This was referenced Jun 26, 2026
7 tasks
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
Closes #34 — Windows 10 recording stop times out at 15s with no useful diagnostics. Three orthogonal changes that together turn 'Stop hangs after 15s, file lost' into 'Stop hangs after 15s, here is exactly which cleanup step is slow on this hardware':
ativeWindowsCaptureOutput\ buffer.
Related issue
Refs #34 (not closes — we still need a data point from the reporter to pin down which step is slow on their hardware).
Type of change
Release impact
Desktop impact
Testing
ode scripts/diagnostic-tool/diagnostic.mjs\ against the instrumented helper; stop completes in ~100 ms with full per-step breakdown on Win11 + Parsec virtual display. Cannot reproduce the user's >15 s hang locally.
Next step for the bug reporter
After this lands, attach a Windows diagnostic bundle from the next CI run (artifact: \openscreen-diagnostic-windows-x64). The user runs \diagnostic.bat --duration 30, attaches the resulting JSON, and the maintainer reads \stopTiming\ to pinpoint the slow step on their hardware.
Summary by CodeRabbit