Fix Svelte 5 reactive race when closing project settings modal#13613
Fix Svelte 5 reactive race when closing project settings modal#13613sebastianhuus wants to merge 1 commit intogitbutlerapp:masterfrom
Conversation
|
Thanks a lot for taking a stab at this! I can reproduce the issue, and it's a more recent one as it's first time I see this. Screen.Recording.2026-05-03.at.16.36.08.movPreviously it would fail after removing the project due to some other condition, but maybe with this PR all of these are fixed? |
|
Yes hopefully so! I don't have anything else to add here so if there are no suggestions I'll send this to Copilot and we can hopefully get this merged?🤞 |
There was a problem hiding this comment.
Pull request overview
This PR hardens the desktop app's global modal router against a Svelte 5 teardown race when closing settings-related modals, so modal children do not briefly receive null/undefined modal data during unmount.
Changes:
- Add
stableModalData, a last-known-good snapshot of the current global modal payload. - Pass modal props/content and login-confirmation close handling through the stable snapshot instead of the live modal state.
- Tighten the modal render guard so the router only renders when both the current modal state and the stable snapshot are available.
| $effect.pre(() => { | ||
| if (modalProps !== null) { | ||
| stableModalData = modalProps; | ||
| } |
There was a problem hiding this comment.
Good spot but not really something I've introduced imo. Maybe plan this for a new PR since this is a narrow bug fix?
5da3a49 to
70ae4c7
Compare
|
updated branch after new changes to main |
70ae4c7 to
4e86532
Compare
When closeSettings() sets modalProps to null, Svelte 5 can propagate
updated props to child components before the outer {#if} guard destroys
the block, causing TypeErrors like "null is not an object (evaluating
'$.get(modalProps).state')" and "undefined is not an object (evaluating
'data.projectId')".
Introduce stableModalData — a $state updated only in $effect.pre
when modalProps is non-null. Child components receive stableModalData
(frozen at the last valid value during teardown) rather than reading
modalProps directly, breaking the reactive propagation chain.
4e86532 to
cfe98d1
Compare
|
I updated one comment which was inaccurate based on Copilot's feedback. I think rest is good. There was one suggestion for adding a regression test, but I'm not sure how to set up a test to check for the target UI flow. If anyone wants to chime in and add that to the PR, feel free. |
|
Thanks a lot! I could reproduce that the original issue is fixed, and the project gets removed successfully. For testing, one could probably add a test to playwright if it doesn't exist there yet, but it's nothing I personally would need to merge this. Let's see if @estib-vega or @mtsgrd can merge the PR - I only looked at the outcome while thinking that the change is probably rather minimal. More NotesIn my specific case, however, I ran into a couple of other, probably unrelated issues with recently removed projects still showing up in the project list after removal. But that state it was only in because it couldn't open a repo that was deleted on disk. Then the removal of that project wasn't offered anymore, also another issue.
Below there was a time when non-existing repos could be removed. Maybe previously there was a backend error code that the frontend would respond to that is gone now? Might be.
|


🧢 Changes
When closeSettings() sets modalProps to null, Svelte 5 can propagate updated props to child components before the outer {#if} guard destroys the block, causing TypeErrors like "null is not an object (evaluating '$.get(modalProps).state')" and "undefined is not an object (evaluating 'data.projectId')".
Introduce stableModalData — a $state updated only in $effect.pre when modalProps is non-null. Child components receive stableModalData (frozen at the last valid value during teardown) rather than reading modalProps directly, breaking the reactive propagation chain.
Related issues:
☕️ Reasoning
Sent a little bug report in the Discord when I found I could no longer remove projects on v0.19.9. The previous approach was very minimal — each commit patched exactly the line that crashed in production telemetry:
5c4f013: telemetry showed null.close() → swap to handleModalClose
20e5a84: CI showed teardown race → add closeModal() with null guard
ac13c2e: error page showed "project not found" → reorder navigation/delete
If someone later adds a new modal type that closes itself via state mutation, it won't reintroduce this class of bug.
Media
Before fix, GitButler Client running 0.19.9
export-1777758137455.mp4
After fix:
export-1777758306650.mp4