fix(plan): keep plan tool switching from corrupting the view - #567
Merged
Merged
Conversation
- PlanToolFallback renders a fixed root div. Its root used to switch from a v-if comment to the spinner div after 200 ms, but Suspense keeps the fallback's first root node, so switching tools while a tool still loaded inserted into null and left the vnode tree broken until reload. - Add a PlanView test that switches tools while the first tool chunk is still loading; it fails with the production error without the fix. Closes #561 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
✅ Deploy Preview for prunplanner-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
🟢 Coverage ∅ diff coverage · +0.00% coverage variation
Metric Results Coverage variation ✅ +0.00% coverage variation (-1.00%) Diff coverage ✅ ∅ diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (b9e4f79) 3715 3628 97.66% Head commit (2b67f4a) 3715 (+0) 3628 (+0) 97.66% (+0.00%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#567) 0 0 ∅ (not applicable) Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
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.
What and why
Found by
/error-review: on/plan/:planet/:uuid, switching plan tools while a tool chunk is still loading throwsCannot read properties of null (reading 'insertBefore')(21 events, 2 users in 24h), and the view stays broken until a reload (_vtc,emitsOptions,bum/parentNodeafterwards). The real cause isPlanToolFallback, not the unkeyed Suspense: its root switched from av-ifcomment to the spinner<div>after 200 ms, but Suspense keeps the fallback's first root node as its ownel(Vue only updates it when the Suspense is the parent's root). On the next tool switch, PlanView unmounts the Suspense and inserts its replacement next to that detached comment, so the parent isnull.This also explains the timing: the spinner component arrived in 0.31.0 (
9183d006), when the errors started, and it only breaks when the first tool is still loading more than 200 ms after it was opened.Changes
PlanToolFallback.vue: a fixed root<div>around the delayed spinner.src/tests/views/PlanView.test.ts(new, local component suite): mounts the realPlanView, opens the construction cart while its chunk is held pending, waits past the 200 ms spinner delay, switches to the supply cart, then loads both chunks. It asserts that no Vue error was raised and that the supply cart renders.Not changed: the
:keyon the tool Suspense and hoisting thedefineAsyncComponentdefinitions, both suggested in the issue.toggleToolalways goes throughnull, so the Suspense is remounted on every switch anyway. With the old fallback, the test still fails with:keyalone. A timing-matrix harness with key and hoisting combined still failed 24 of 32 scenarios. Neither change is needed.How it was verified
insertBeforeon null) and passes with the fix. This happens in dev-mode Vue too, because the guard skipped in production builds does not hide this path.pnpm test,pnpm tsc,pnpm lintandpnpm knipall pass.pnpm vitest run --config vitest.components.config.ts src/tests/views/PlanView.test.ts src/tests/features/planning/componentspasses.Closes #561
🤖 Generated with Claude Code