feat: open the issue-report modal from the error fallback - #10875
Merged
Merged
Conversation
FeedbackModal now reads a nullable request client so a crash-time report still produces partial environment details. Source collection and environment retry stay disabled until a client exists.
The shared error screen now owns a local dialog and mounts FeedbackModal with the application store, because root crashes remove ancestor modal and tooltip providers.
Boundary tests exercise provider loss, a missing client, closed-dialog request skipping, focus restore, retry, and source opt-in.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
All contributors have signed the CLA ✍️ ✅ |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Add a local fallback Toaster and correct the missing-client explanation before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds issue reporting to React error fallbacks, including partial diagnostics when the request client is unavailable.
Changes:
- Adds a local
FeedbackModaldialog to error fallbacks. - Supports nullable request clients and disabled notebook source.
- Adds coverage for fallback, diagnostics, focus, and recovery behavior.
File summaries
| File | Summary | Review notes |
|---|---|---|
frontend/src/components/editor/chrome/components/feedback-button.tsx |
Handles nullable clients and partial diagnostics. | Nit (1 vote): explain missing-client state before notebook/connection checks. |
frontend/src/components/editor/chrome/components/__tests__/feedback-button.test.tsx |
Tests missing-client and retry scenarios. | — |
frontend/src/components/editor/boundary/ErrorBoundary.tsx |
Adds the report action and standalone dialog. | Moderate (2 votes): mount a local Toaster in the fallback. |
frontend/src/components/editor/boundary/__tests__/ErrorBoundary.test.tsx |
Tests dialog integration and recovery behavior. | — |
Review details
Suppressed comments (1)
frontend/src/components/editor/chrome/components/feedback-button.tsx:121
requestClient != nullis now part ofnotebookSourceAvailable, but it is not represented in this reason chain. In the startup/root fallback the connection defaults toNOT_STARTED(and the filename may still be unset), so a missing client is reported as “Connect the notebook…” or “Save the notebook first” even though neither action can restore source access. Check the missing-client case before the notebook/connection checks so the explanation matches the disabled state.
: connection.state !== WebSocketState.OPEN
? "Connect the notebook to include its source."
: "Notebook source is unavailable.";
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Coverage Report for ./frontend
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Mount a local Toaster after a root crash, lazy-load FeedbackModal so table tests do not hit a static-state mock TDZ, and explain a missing request client before connection state.
Light2Dark
previously approved these changes
Sep 17, 2026
Comment on lines
+113
to
+121
| const notebookSourceReason = notebookSourceAvailable | ||
| ? undefined | ||
| : filename === null | ||
| ? "Save the notebook first." | ||
| : !codeAvailable | ||
| ? "Notebook source is hidden in this view." | ||
| : "Connect the notebook to include its source."; | ||
| : requestClient == null | ||
| ? "Notebook source is unavailable." | ||
| : filename === null | ||
| ? "Save the notebook first." | ||
| : !codeAvailable | ||
| ? "Notebook source is hidden in this view." | ||
| : "Connect the notebook to include its source."; |
Member
There was a problem hiding this comment.
nit, but can we avoid this nested ternary for readability
Replace the nested ternary with an early-return helper so the missing-client explanation stays readable.
Light2Dark
approved these changes
Sep 17, 2026
This branch was successfully deployed
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
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
Adds Report an issue to the shared React error fallback so a crashed cell (or a startup/root/island failure) can open the existing issue-report modal instead of a plain GitHub link.
The fallback owns a local dialog because a root crash removes
ModalProvider.FeedbackModalreads the nullablerequestClientAtominstead ofuseRequestClient(), because that hook throws when the client is still unset — which is exactly when this screen can appear. Other callers keep the strict hook. A missing client yields partial environment details, no Retry, and disabled notebook source.Related: MO-7659