Repository navigation
Show a failed app import or redeploy once, and keep a failed import beside its form - #1871
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors application deployment and import error handling by introducing a reusable DeployFailure component to display failure messages and handle SSH access failure links. It updates useImportApplication and RedeployApplicationModal to use mutateAsync from React Query, ensuring that deployment outcomes are correctly handled even if the calling component unmounts. It also suppresses global error toasts for these mutations, routing errors to inline panels or context-aware toasts instead, backed by comprehensive tests. Feedback is provided regarding a TypeScript typing issue in RedeployApplicationModal.tsx where useRef<symbol>(undefined) may cause compilation failures under strict null checks; using useRef<symbol>() is recommended instead.
| // that settles after the modal was closed or left must not touch whatever is open now, so it | ||
| // reports through toasts alone. Set beside the state it mirrors, not from an effect: a deploy can | ||
| // fail before the render that shows the panel commits. | ||
| const deployOnScreen = useRef<symbol>(undefined); |
There was a problem hiding this comment.
In TypeScript, initializing useRef<symbol>(undefined) types deployOnScreen.current strictly as symbol and will fail compilation under strict null checks because undefined is not assignable to symbol. Since undefined is later assigned to deployOnScreen.current (on lines 48 and 69), the ref should be typed to allow undefined.
Using useRef<symbol>() is the idiomatic way to define a ref that can hold a symbol or undefined.
| const deployOnScreen = useRef<symbol>(undefined); | |
| const deployOnScreen = useRef<symbol>(); |
There was a problem hiding this comment.
Declining. Studio is on @types/react 19.3.0, where every useRef overload takes a required initialValue (index.d.ts:1744, :1756, :1768); the third, useRef<T>(initialValue: T | undefined): RefObject<T | undefined>, is the one useRef<symbol>(undefined) resolves to, so the ref is already symbol | undefined and tsc -b is clean. I applied the suggested useRef<symbol>() and ran tsc -b: it fails with TS2554: Expected 1 arguments, but got 0 at line 46, then TS2322: Type 'undefined' is not assignable to type 'symbol' at lines 48 and 69.
🤖 Generated by Claude Code (Claude Opus 5.5); posted via @dawsontoth.
…t beside its form A failed deploy showed the same failure in two toasts: the deploy mutation's global one, and the caller's own. The import form's only failure display was a toast that faded, for failures the user has to act on and retry. The deploy mutation now opts out of the global toast and keeps its RUM report with a mutation-level console.error. The import form keeps the failure inline above the Import button until the next attempt, with the SSH-keys link the redeploy panel already offered. Redeploy shows a streamed failure in its progress panel, toasting only when the panel is gone, and the buffered path, which relied on the global toast, now says why it failed. Both callers await mutateAsync so a failure that settles after the form unmounts still reaches the user, and every failure display reads the server's reason through describeError rather than axios's generic message. Closes #1833 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…is open now Awaiting mutateAsync means a deploy's continuation now runs after its form or modal is gone. An import that finished after the user left the form moved the editor's selection to the new project, and a redeploy that succeeded after its modal was closed closed and reset whatever redeploy modal was open by then. The import still refreshes the tree and reports its result, but leaves the selection alone once the form has unmounted. The redeploy modal records which deploy its progress panel shows; any other deploy reports through toasts and never settles the panel or closes the modal. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… teardown Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…omments that narrated it The cross-model round on the port found the new comments narrating framework mechanics and caller behavior, and two claims in DeployFailure's that no longer held (that nothing else reports an SSH failure, and that RUM drops both halves of its regex). The rule they were circling, that every deploy caller shows its own failure and awaits mutateAsync, now lives once in applications/DESIGN.md; the code points there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…hat ran it The closing cross-model round found that switching to another instance's Applications page mid-import keeps useImportApplication mounted (same route component), so a mount check let instance A's failure land inline on instance B's form, with a record link built from B's route params. The hook now tracks which instance's form is on screen. A failure is shown inline only on the form of the instance that ran the import, and toasts with that import's own record action otherwise; the editor selection moves only for that instance too. Also drops the duplicated DESIGN.md pointers and the issue-number history from test notes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
5889adc to
ab4e737
Compare
…ailed The rebase onto stage brought in a write surface this PR's caller audit predates: activating a staged release (#1876's chain). Its mutation opts out of the global toast, like import and redeploy since #1871, so an activation that timed out or got a 504 showed "Failed to activate <app>" with axios's own text and no warning that it may have gone through. deployFailureTitle (new, beside the deploy reporters) is the one place a deploy's failure title is decided: "Couldn't confirm <app> was activated" for a write with no verdict, "Failed to activate <app>" otherwise. The import form and the redeploy modal, which the rebase had given their own copies of that rule, now use it too. reportDeployFailure shows the unknown-outcome copy, and the activation's progress panel says so as well. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
⊙ Problem
Every failed deploy showed the same failure twice.
useDeployComponentMutationdidn't opt out of the global mutation toast, somutationErrorHandlertoasted it, and then the import form ("Failed to import …") or the redeploy modal ("Failed to redeploy …") toasted it again. The streamed redeploy also showed it in its progress panel, so that path showed it three times. The import form had no persistent failure display at all, just a toast that faded, even though these are failures the user has to fix and retry: #1699 measured one session retrying the same import 9 times in 22 minutes. Closes #1833.💡 Solution
The deploy mutation now opts out of the global toast. It keeps the RUM report with a mutation-level
console.error, the pattern AGENTS.md documents in "DroppingerrorHandlerfrom anonErroralso drops the RUM report". Each caller now shows the failure once:DeployFailurecomponent, so a private-repository SSH failure also gets the "Manage SSH keys" link here. The loading toast is dismissed and no error toast is shown.Every one of these displays now gets its text from
describeError. Before, the import and redeploy toasts usederror.message, which on the buffered path is axios'sRequest failed with status code 500and not the server's reason. Only the global toast had shown the real text.Both callers now await
mutateAsyncinstead of passing callbacks tomutate. React Query drops amutatecaller's callbacks once the component unmounts. Before this PR the global toast still reached a user who had navigated away, so without this change a failure that arrived after they left would have been silent. In that case it now arrives as a toast. Because the continuation now outlives its form, the second commit keeps it out of whatever is on screen by then. An import that finishes after the form unmounted still refreshes the tree and reports its result, but it leaves the editor selection alone. The redeploy modal records which deploy its panel is showing, and any other deploy never settles that panel or closes the modal.Also fixed: an inconclusive stream (
SSEInconclusiveError) no longer shows a generic "Error" toast on top of the "verifying…" health-check result. That toast came from the global handler, which never treated the inconclusive case as anything special. RUM still receives it, as it did before.#1808 is the broader version of this: React Query's global
errorHandlerdouble-reports and double-shows failed route loads. This PR doesn't change the global handler. It only opts this one mutation out, the way the sign-in, sign-up, forgot-password and add-domain mutations already do.⚖️ Alternatives
mutateand accept a silent failure after unmount. Rejected: that would be a regression, because the global toast had covered that case.onErrorand toast only then. Rejected: the mutation-levelonErrorruns before any caller's, and theMutationdoesn't expose its observers publicly.🔧 Changes
deployComponent.ts:useDeployComponentMutationsetsmeta: { skipGlobalErrorToast: true }and a mutation-levelonError: console.error.DeployFailure.tsx(new): the failure block and its SSH-keys link, moved out ofDeployProgress. It takes an optional title and hasrole="alert".DeployProgress.tsx: rendersDeployFailurein place of its inline markup.useImportApplication.ts: holdsimportFailurein component state, clears it on the next attempt, awaitsmutateAsync, toasts only once unmounted, and leaves the editor selection alone after unmount.ImportInstructions.tsxandNewApplication/index.tsx: renderimportFailureabove the Import button.RedeployApplicationModal.tsx: both paths awaitmutateAsync. The streamed path keeps the deploy that owns the on-screen panel in a ref and toasts only for one that doesn't own it. The buffered path replaces its progress toast with the reason fromdescribeError.✅ Verification
Route: component and hook tests through the real
useDeployComponentMutationand aQueryClientbuilt on the app's ownmutationErrorHandler, so the global-toast opt-out is what gets exercised. Only the network edge (instanceClient.post,deployComponentStream) and the toasts are mocked. No browser run: the deploy forms sit behind a signed-in instance, and the failure has to come from a real Harper.deployComponent.test.ts: the parent PR's test said the global toast showed the guidance. It now checks the opposite. The hook rejects through an observer that unmounts first, and the test asserts that no toast fires and thatconsole.errorgets the explained error exactly once.useImportApplication.test.ts(new) covers five cases. A failed import keeps the server's reason (not axios's) for the form, shows no error toast, and reports to RUM once. A retry clears the failure. A failure after unmount becomes a toast. A success after unmount reports and refreshes but doesn't touch the selection. An inconclusive stream is verified and not reported as a failure.RedeployApplicationModal.test.tsx(new) covers four cases. A streamed failure shows in the open panel with no toast. A failure after the modal was closed, or after the page was left, toasts once. A deploy that succeeds after the modal was closed and reopened leaves the reopened modal open. A buffered failure replaces its progress toast with the server's reason.NewApplication.test.tsx: the inline alert renders beside the Import button, and an SSH failure links toconfig/ssh-keys.skipGlobalErrorToast(8 tests), the mutation-levelconsole.error(3), the import's mounted check (1),describeErrorin place oferror.message(2), the panel-ownership check (1 to 2 each), and the two stale-continuation guards (1 each). Fails-on-base: all 11 new or changed tests fail againstclaude/1699-missing-package-json-guidance's source.npx vitest runoversrc/features/instance/applicationsandsrc/integrations/api/instance/applicationspassed 37 files and 398 tests.npx tsc -b,npx oxlint --format stylish .andnpx dprint checkall exited 0. The pre-commit hook's full suite passed on every commit, most recently 399 files and 3,695 tests.git fetchcould not sign through the 1Password SSH agent, and the Harper domain leg failed both rounds on an expired Claude OAuth session, so I triaged the findings by hand.useMutationonErroris dropped on unmount. Rejected. Mutation-level options run inMutation.executewhether or not an observer is still attached, and the hook test above proves it: with the global handler opted out, thatonErroris the only thing that can produce theconsole.error, and deleting it turns the test red.runHealthCheckcould reject outside atry. Rejected:useComponentHealthCheckcatches every read failure inside its polling loop and always resolves.?.guard to the test teardown that the CLI's self-check flagged.useRef<symbol>(undefined)touseRef<symbol>(). Under@types/react19 everyuseRefoverload requires an argument, and applying the suggestion failstsc -bwith TS2554 and two TS2322s. Since then this branch has merged fix(applications): explain a git deploy with no root package.json #1841's fix for its own gemini comment (0efc440: the explained Axios body omits the raw text fields instead of setting them toundefined). This PR's own diff is unchanged.🤖 Generated with Claude Code (Anthropic Claude Opus); posted via @dawsontoth.
Related PRs: #1841 overlaps (stacked base; this PR replaces its global-toast test)
Complexity: medium
Review-Coverage: authored=claude; ran=codex,gemini; blocked=cursor-composer(no-receipt),domain(auth); declined=cursor-grok,cursor-kimi,cursor-muse; rounds=2; full=1 @ f09223e
Review-Attention: study ~9m (raised: degraded review, open major) @ f09223e