Skip to content

Fix Svelte 5 reactive race when closing project settings modal#13613

Open
sebastianhuus wants to merge 1 commit intogitbutlerapp:masterfrom
sebastianhuus:fix/guard-modal-close-on-delete
Open

Fix Svelte 5 reactive race when closing project settings modal#13613
sebastianhuus wants to merge 1 commit intogitbutlerapp:masterfrom
sebastianhuus:fix/guard-modal-close-on-delete

Conversation

@sebastianhuus
Copy link
Copy Markdown
Contributor

🧢 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:

  • Failed to remove a project #13252: A bit different but points to @mtsgrd who pushed related fixes a few days after the issue was raised. I think this compliments these and will help avoid similar problems.

☕️ 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.

image

Media

Before fix, GitButler Client running 0.19.9

  1. Remove project
  2. Error toast shows
export-1777758137455.mp4

After fix:

  1. Remove project
  2. Project view is updated to a different project, project removed from project list and no error toast
    • Console complains about IPC cache issue, but as you can see the UI bug has been resolved.
export-1777758306650.mp4

@Byron Byron added the 🎉reproduced🎉 The issue could be reproduced by following the instructions label May 3, 2026
@Byron
Copy link
Copy Markdown
Collaborator

Byron commented May 3, 2026

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.mov

Previously it would fail after removing the project due to some other condition, but maybe with this PR all of these are fixed?

@sebastianhuus
Copy link
Copy Markdown
Contributor Author

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?🤞

@sebastianhuus sebastianhuus marked this pull request as ready for review May 3, 2026 19:26
Copilot AI review requested due to automatic review settings May 3, 2026 19:26
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +106 to +109
$effect.pre(() => {
if (modalProps !== null) {
stableModalData = modalProps;
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good spot but not really something I've introduced imo. Maybe plan this for a new PR since this is a narrow bug fix?

@sebastianhuus sebastianhuus force-pushed the fix/guard-modal-close-on-delete branch from 5da3a49 to 70ae4c7 Compare May 4, 2026 18:04
@sebastianhuus
Copy link
Copy Markdown
Contributor Author

updated branch after new changes to main

Copilot AI review requested due to automatic review settings May 4, 2026 19:01
@sebastianhuus sebastianhuus force-pushed the fix/guard-modal-close-on-delete branch from 70ae4c7 to 4e86532 Compare May 4, 2026 19:01
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

Comment thread apps/desktop/src/components/views/GlobalModalRouter.svelte
Comment thread apps/desktop/src/components/views/GlobalModalRouter.svelte Outdated
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.
@sebastianhuus sebastianhuus force-pushed the fix/guard-modal-close-on-delete branch from 4e86532 to cfe98d1 Compare May 4, 2026 20:10
@sebastianhuus
Copy link
Copy Markdown
Contributor Author

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.

@Byron Byron requested review from estib-vega and mtsgrd May 5, 2026 02:37
@Byron
Copy link
Copy Markdown
Collaborator

Byron commented May 5, 2026

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 Notes

In 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.

Screenshot 2026-05-05 at 10 42 49

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.

Screenshot 2026-05-05 at 10 44 49

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🎉reproduced🎉 The issue could be reproduced by following the instructions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants