feat(desktop): user-reviewed diagnostics package export#1472
Merged
Conversation
Astro-Han
added
enhancement
Contributor
|
Warning Review limit reached
More reviews will be available in 41 minutes and 3 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces the monolithic ChangesDiagnostics review three-step flow
Sequence Diagram(s)sequenceDiagram
actor User
participant ErrorPage as Error Page / Menu
participant openDiagnosticsReview
participant Platform as Platform (renderer)
participant IPC
participant FeedbackHandler as FeedbackHandler (main)
rect rgba(99, 132, 199, 0.5)
note over User, FeedbackHandler: Step 1 – Prepare
User->>ErrorPage: clicks "Prepare diagnostics package"
ErrorPage->>openDiagnosticsReview: openDiagnosticsReview({platform, dialog, language})
openDiagnosticsReview->>Platform: prepareReport(input)
Platform->>IPC: prepare-report(input, windowID)
IPC->>FeedbackHandler: prepareReport(input, {windowID})
FeedbackHandler-->>IPC: PrepareReportResult {ready | failed}
IPC-->>Platform: PrepareReportResult
Platform-->>openDiagnosticsReview: PrepareReportResult
alt status = "ready"
openDiagnosticsReview->>ErrorPage: dialog.show(DiagnosticsReviewBody)
else status = "failed"
openDiagnosticsReview->>User: showToast(error)
end
end
rect rgba(180, 99, 132, 0.5)
note over User, FeedbackHandler: Step 2 – Reveal (optional)
User->>ErrorPage: clicks "Reveal in Finder"
ErrorPage->>Platform: revealReport(reportId)
Platform->>IPC: reveal-report(reportId)
IPC->>FeedbackHandler: revealReport(reportId)
FeedbackHandler-->>User: opens folder / fallback directory
end
rect rgba(99, 180, 132, 0.5)
note over User, FeedbackHandler: Step 3 – Submit
User->>ErrorPage: clicks "Submit"
ErrorPage->>Platform: submitReport(reportId)
Platform->>IPC: submit-report(reportId)
IPC->>FeedbackHandler: submitReport(reportId)
FeedbackHandler-->>User: opens feedback URL
FeedbackHandler-->>ErrorPage: SubmitReportResult {opened | form-fallback | no-form | stale}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
github-actions
Bot
added
app
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Suggested priority: P2 (includes user-path files (packages/app/src/components/diagnostics-review.test.tsx, packages/app/src/components/diagnostics-review.tsx, packages/app/src/context/platform.tsx, packages/app/src/desktop-api-contract.ts, packages/app/src/desktop-api.ts, packages/app/src/i18n/en.ts, packages/app/src/i18n/zh.ts, packages/app/src/pages/error-page-source.test.ts, packages/app/src/pages/error-report.test.ts, packages/app/src/pages/error-report.ts, packages/app/src/pages/error.tsx, packages/app/src/pages/layout.tsx, packages/app/src/pages/layout/layout-commands.test.ts, packages/app/src/pages/layout/layout-commands.ts, packages/desktop-electron/src/main/feedback.test.ts, packages/desktop-electron/src/main/feedback.ts, packages/desktop-electron/src/main/index.ts, packages/desktop-electron/src/main/ipc-window-config.test.ts, packages/desktop-electron/src/main/ipc.ts, packages/desktop-electron/src/preload/index.ts, packages/desktop-electron/src/preload/types.ts, packages/desktop-electron/src/renderer/index.tsx)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
This was referenced Jun 23, 2026
Replace the auto copy/reveal/open-form problem report flow with an explicit in-app review surface. prepareReport now only generates, redacts, and saves the package and returns review metadata; revealReport and submitReport become the user's explicit follow-up actions. - contract: prepare/reveal/submit replace the single reportProblem call - renderer: a shared DiagnosticsReviewBody backs both a Dialog (menu entry) and an inline panel on the error page, which has no DialogProvider in its ErrorBoundary fallback scope - menu entry routes through a registered diagnostics.prepare command instead of a bespoke IPC channel - i18n: add diagnostics.review.* keys (en/zh); drop the dead native confirm/status copy and feedbackDialogLabels Claude-Session: https://claude.ai/code/session_011KyY9wTxQu9oZLy4yPEi3W
Astro-Han
force-pushed
the
claude/diag-review-panel
branch
from
6a87b78 to
1788832
Compare
The diagnostics-package flow was inverted: window.api.reportProblem was
replaced by prepareReport / revealReport / submitReport, and preparation
no longer copies to the clipboard, reveals the file, or opens the form.
The smoke still called the removed reportProblem and asserted clipboard
side effects, so smoke-macos-arm64 failed with
'window.api.reportProblem is not available'.
Drive prepareReport({ rendererError }) instead and assert the new
contract: status 'ready' with reportId / fileName / contents, plus the
saved markdown report still carries the renderer error, JSON payload,
and main/backend log tails. Drop the clipboard assertions (that side
effect was intentionally removed); the smoke stays prepare-only and
side-effect-free, as the old confirm:false path was.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/desktop-electron/src/main/feedback.ts (1)
180-197: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve
nullcounts when diagnostics collection fails.The contract says counts are
nullwhen a component is absent or failed, but Line 260 reports0renderer events after the failure/timeout path creates an empty"write_failed"slice. Line 258 also treats a successfully collected empty log the same as a failed/missing log.Proposed fix
let diagnostics: ProblemReportDiagnostics let logTail = "" + let logTailCollected = false @@ try { logTail = deps.logTail() + logTailCollected = true } catch (error) { fullReportFailure ??= safeFailureReason(error) } @@ const contents: DiagnosticsReviewContents = { - logLines: logTail ? logTail.split(/\r?\n/).filter((line) => line.length > 0).length : null, + logLines: logTailCollected ? logTail.split(/\r?\n/).filter((line) => line.length > 0).length : null, sessionMessages: sessionExport.status === "ok" ? sessionExport.messages.length : null, - rendererEvents: rendererDiagnostics.events.length, + rendererEvents: rendererDiagnostics.status === "ok" ? rendererDiagnostics.events.length : null, rendererError: Boolean(input.rendererError), }Also applies to: 205-210, 257-260
🤖 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 `@packages/desktop-electron/src/main/feedback.ts` around lines 180 - 197, The issue is that when diagnostics collection fails (in the try-catch blocks for deps.diagnostics(context) and deps.logTail()), the fallback approach using fallbackDiagnostics() and empty slices returns zero counts instead of preserving null values as specified in the contract. To fix this, ensure that when a diagnostics collection operation fails or times out, the resulting data structure maintains null counts to indicate the component is absent or failed, rather than using zero or empty values. This distinction is important at lines around 260 where renderer events are being reported, so the error handling for rendererDiagnostics initialization and the fallbackDiagnostics() function need to properly preserve null values for failed/missing components while only using zero counts for successfully collected but genuinely empty data.
🤖 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 `@packages/app/src/components/diagnostics-review.tsx`:
- Around line 101-123: The reveal function and submit function both make
optional platform method calls that can be rejected without proper error
handling. In the reveal function, add a catch handler to the revealReport call
to handle any promise rejections and provide feedback to the user. In the submit
function, add a catch block after the try block to handle any rejections from
submitReport (in addition to the existing finally block), ensuring that
submitting is set to false and appropriate error feedback is provided to the
user instead of leaving unhandled rejections.
In `@packages/app/src/pages/error.tsx`:
- Around line 107-113: In the catch block of the prepareDiagnostics call (around
the setStore invocation), add clearing of the review property to prevent stale
review content from remaining visible when the prepare operation fails. Include
review: undefined in the setStore object alongside the existing actionError and
actionMessage properties to ensure the review is cleared whenever an error
occurs in this error handling path.
In `@packages/app/src/pages/layout/layout-commands.ts`:
- Around line 50-53: The diagnosticsActions object in layout-commands.ts
currently exposes the prepare command unconditionally, which results in a no-op
command when diagnostics is not supported. Instead of always exposing the
command, add a gating condition by including a canPrepare function alongside the
prepare handler in the diagnosticsActions object. The canPrepare function should
check if platform.prepareReport exists to determine if the command should be
available. Apply this same gating pattern to both the diagnosticsActions object
and any other similar diagnostic-related command structures referenced in lines
187-193 to ensure consistency across all diagnostic commands.
In `@packages/desktop-electron/src/main/feedback.ts`:
- Around line 274-279: The inFlight guard in the prepareReport function is too
broad and returns the same cached promise for all concurrent callers regardless
of their input parameters and context. Scope the in-flight tracking to be
context-aware by associating the inFlight promise with the specific input and
contextOverride parameters, so that only requests with matching input parameters
return the existing promise. Requests with different input.rendererError or
contextOverride values should proceed independently with their own context,
ensuring each window's crash-page prepare gets its own package and report ID
rather than reusing the first window's IDs.
---
Outside diff comments:
In `@packages/desktop-electron/src/main/feedback.ts`:
- Around line 180-197: The issue is that when diagnostics collection fails (in
the try-catch blocks for deps.diagnostics(context) and deps.logTail()), the
fallback approach using fallbackDiagnostics() and empty slices returns zero
counts instead of preserving null values as specified in the contract. To fix
this, ensure that when a diagnostics collection operation fails or times out,
the resulting data structure maintains null counts to indicate the component is
absent or failed, rather than using zero or empty values. This distinction is
important at lines around 260 where renderer events are being reported, so the
error handling for rendererDiagnostics initialization and the
fallbackDiagnostics() function need to properly preserve null values for
failed/missing components while only using zero counts for successfully
collected but genuinely empty data.
🪄 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: 714d6b9d-8ed1-4d04-8aae-9b3f1552dfe3
📒 Files selected for processing (22)
packages/app/src/components/diagnostics-review.test.tsxpackages/app/src/components/diagnostics-review.tsxpackages/app/src/context/platform.tsxpackages/app/src/desktop-api-contract.tspackages/app/src/desktop-api.tspackages/app/src/i18n/en.tspackages/app/src/i18n/zh.tspackages/app/src/pages/error-page-source.test.tspackages/app/src/pages/error-report.test.tspackages/app/src/pages/error-report.tspackages/app/src/pages/error.tsxpackages/app/src/pages/layout.tsxpackages/app/src/pages/layout/layout-commands.test.tspackages/app/src/pages/layout/layout-commands.tspackages/desktop-electron/src/main/feedback.test.tspackages/desktop-electron/src/main/feedback.tspackages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/ipc-window-config.test.tspackages/desktop-electron/src/main/ipc.tspackages/desktop-electron/src/preload/index.tspackages/desktop-electron/src/preload/types.tspackages/desktop-electron/src/renderer/index.tsx
💤 Files with no reviewable changes (1)
- packages/app/src/pages/error-page-source.test.ts
- gate the diagnostics.prepare command when the prepareReport bridge is absent (non-desktop hosts) instead of firing a no-op - clear a prior successful review when a re-prepare fails (error page) - dedupe an in-flight prepare by window + renderer-error context, not globally, so a different context never gets the wrong package's reportId - surface a recoverable notice when reveal/submit IPC rejects rather than failing silently - tests: review-body click contract, command gate, cross-context dedupe
Replace the two hand-rolled stroked SVGs with @opencode-ai/ui Icon — folder on Reveal, lock on the privacy caveat (the shared set has no shield glyph). Aligns the panel with the app-wide Icon convention.
Drop the decorative lock/folder icons, and replace the contents list's per-row divider hairlines with a soft grouped panel separated by spacing. Label reads as primary (fg-base), counts as secondary (fg-weak).
Second review round on the review panel: - revealReport now returns RevealReportResult (revealed | opened-directory | stale | failed) end-to-end (contract -> main -> ipc/preload -> platform). A stale id or an OS-open failure the main process used to swallow now surfaces in the review notice instead of a silent no-op. - resolve prepare/reveal/submit once at the openDiagnosticsReview and error-page boundaries into a non-optional ReviewActions; the body no longer optional-chains, closing the `await undefined -> onDone` trap where a missing submit silently closed the review as if it had been sent. - tests: real reveal-contract cases in feedback.test.ts, reveal-result branches in the body check, and a side-effect-free stale reveal/submit through the real bridge in the desktop smoke.
…e, real menu-path smoke - error page now copies the redacted summary when prepare returns failed (it was dropped for a bare error), matching the menu's copy-fallback toast; extract a pure diagnosticsFailureState helper with tests - collapse the review body's stale/failed booleans into one notice signal: every reveal/submit clears it first, so a stale reveal then a failed submit shows only the latest reason instead of a mixed pair - report-problem smoke walks the real menu-command -> command registry -> dialog -> review-panel path and tears it down via Cancel, covering wiring the prepare-only calls bypassed (reveal/submit OS side effects stay on the stale calls)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/app/src/components/diagnostics-review.tsx (1)
233-235: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle clipboard write rejection in the toast action.
Line 234 can produce an unhandled rejected promise if clipboard write fails; swallow the rejection in the click handler.
Suggested patch
- onClick: () => void navigator.clipboard.writeText(summary), + onClick: () => { + void navigator.clipboard.writeText(summary).catch(() => undefined) + },🤖 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 `@packages/app/src/components/diagnostics-review.tsx` around lines 233 - 235, The onClick handler that calls navigator.clipboard.writeText(summary) does not handle promise rejections, which can result in unhandled rejected promises if the clipboard write fails. Modify the onClick handler to add rejection handling using either .catch() method chaining or by converting the handler to an async function with try-catch block to gracefully handle any errors from the clipboard operation and prevent unhandled promise rejections.
🤖 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 `@packages/desktop-electron/scripts/report-problem-smoke.mjs`:
- Around line 168-172: The window selection in the app.evaluate callback uses
the first window from BrowserWindow.getAllWindows(), which is nondeterministic
when multiple windows exist. Replace this approach with
BrowserWindow.getFocusedWindow() to get the currently focused window, and add a
fallback to the first window from getAllWindows() if no window is focused. This
ensures the menu-command is sent to the correct target window.
---
Outside diff comments:
In `@packages/app/src/components/diagnostics-review.tsx`:
- Around line 233-235: The onClick handler that calls
navigator.clipboard.writeText(summary) does not handle promise rejections, which
can result in unhandled rejected promises if the clipboard write fails. Modify
the onClick handler to add rejection handling using either .catch() method
chaining or by converting the handler to an async function with try-catch block
to gracefully handle any errors from the clipboard operation and prevent
unhandled promise rejections.
🪄 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: a0d7ba01-2730-497c-b5b7-ecb4635c174f
📒 Files selected for processing (15)
packages/app/src/components/diagnostics-review.test.tsxpackages/app/src/components/diagnostics-review.tsxpackages/app/src/context/platform.tsxpackages/app/src/desktop-api-contract.tspackages/app/src/desktop-api.tspackages/app/src/i18n/en.tspackages/app/src/i18n/zh.tspackages/app/src/pages/error-report.test.tspackages/app/src/pages/error-report.tspackages/app/src/pages/error.tsxpackages/desktop-electron/scripts/report-problem-smoke.mjspackages/desktop-electron/src/main/feedback.test.tspackages/desktop-electron/src/main/feedback.tspackages/desktop-electron/src/main/ipc.tspackages/desktop-electron/src/preload/types.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/app/src/desktop-api.ts
- packages/app/src/i18n/en.ts
- packages/app/src/components/diagnostics-review.test.tsx
- packages/app/src/pages/error-report.test.ts
- packages/app/src/desktop-api-contract.ts
- packages/app/src/pages/error.tsx
- packages/app/src/i18n/zh.ts
- packages/desktop-electron/src/main/feedback.ts
- packages/desktop-electron/src/main/feedback.test.ts
…gnostics The menu walk timed out waiting for the review dialog: BrowserWindow.getAllWindows()[0] is not guaranteed to be the window Playwright observes (a loading splash can coexist with the main window), so the menu-command fired in a renderer the smoke never sees. Target app.browserWindow(window) instead, and dump on-screen state on timeout so a future failure is self-explaining.
…ble in CI smoke) The menu-command -> command-registry -> dialog walk never rendered the review dialog in the macOS smoke. The CI app sits on the home screen and firing diagnostics.prepare there produced neither the dialog nor an error toast (on-screen dump: dialogs=0, toasts=[], shellMain=true), so the renderer-internal menu->command->DialogProvider chain doesn't activate without first opening a real project/session -- not the "thin" test the review asked for, and brittle to drive over a 3-4min CI loop. Keep the real IPC-contract coverage already in the smoke (prepareReport + stale reveal/submit through the live bridge); the menu id + disabled gating stays covered by layout-commands.test.ts and the dialog/button handlers by diagnostics-review.test.tsx.
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
Rework the Prepare diagnostics package flow so the user reviews and explicitly shares the package — PR3 of the diagnostics-package rebuild (#1465). Control is inverted away from a single main-process call that silently did everything:
prepareReport(main) only generates, redacts, saves the package, prunes older ones, and returns review metadata (status: "ready" | "failed"). No clipboard, reveal, or form side effects.revealReport(reportId)/submitReport(reportId)are the user's explicit follow-up actions, each validated against a singlependingslot so a stale report id can't act (submitReportreturns{ status: "stale" }, which the review surface shows as a clear notice rather than silently closing).DiagnosticsReviewBodylists what the package contains, states the privacy caveat, and offers reveal / submit. Menu / command entry renders it in aDialog; the crash error page renders it inline (theErrorBoundaryfallback has noDialogProviderin scope).diagnostics.preparecommand (reusing the existingmenu-commandchannel) instead of a bespoke IPC channel. If the feedback form fails to open, the surface shows a visible, selectable URL plus a copy-summary action.diagnostics.review.*keys (en + zh); removed the now-dead native-dialogconfirm/status copy,feedbackDialogLabels, anderrorReportStatusMessage.Why
Before, one
reportProblemcall did everything — generate → copy summary → reveal file → open the feedback form — with no chance for the user to see what was in the package before it left the machine. Inverting control puts an explicit human review step between generation and sharing, which is the backstop the redaction layers (PR1/PR2) are documented to rely on.Related Issue
Refs #1465 (umbrella). This is PR3; PR1 #1470 is merged, PR2 #1471 is open. Flat PR on
dev— the only overlap with PR1/PR2 is a small region offeedback.tsthat resolves trivially on rebase.Human Review Status
PendingReview Focus
pending-slot validation:revealReport/submitReportonly act on the current report id, and a stale id surfaces a notice instead of silently closing.DiagnosticsReviewBody— Dialog vs inline, the latter because theErrorBoundaryfallback has noDialogProvider.menu-commandchannel via a registereddiagnostics.preparecommand (no new IPC surface).Risk Notes
dev; thefeedback.tsoverlap with PR1/PR2 resolves trivially on rebase. Intended to land after PR1 and PR2.How To Verify
Screenshots or Recordings
Not captured — the review Dialog / inline panel are visible UI, but this environment is headless. Visual verification is left to the maintainer (see Risk Notes).
Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation