Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sebastianhuus wants to merge 1 commit into
base: master
Choose a base branch
from
+25 −13
Open

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 25 additions & 13 deletions apps/desktop/src/components/views/GlobalModalRouter.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,18 @@

const modalProps = $derived(mapModalStateToProps(uiState.global.modal.current));

// Svelte 5 can propagate prop changes into the child block before the outer
// {#if} re-evaluates and unmounts it, causing crashes like
// "undefined is not an object (evaluating 'data.projectId')". stableModalData
// latches the last non-null modalProps so the children always see valid data;
// the {#if modalProps && stableModalData} gate handles visibility.
let stableModalData = $state<ModalData | null>(null);
$effect.pre(() => {
if (modalProps !== null) {
stableModalData = modalProps;
}
Comment on lines +92 to +95
Copy link
Copy Markdown
Contributor Author

@sebastianhuus sebastianhuus May 3, 2026

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?

});

let modal = $state<Modal>();

// Show the modal whenever modalProps becomes truthy.
Expand All @@ -98,7 +110,7 @@
// If the login confirmation modal is closed without explicit user action (e.g., via ESC),
// we should reject the incoming user to maintain state consistency.
// We check if there's still an incoming user to avoid calling reject after accept/reject buttons.
if (modalProps?.state.type === "login-confirmation") {
if (stableModalData?.state.type === "login-confirmation") {
if (userService.incomingUserLogin) {
userService.rejectIncomingUser();
}
Expand All @@ -121,23 +133,23 @@
}
</script>

{#if modalProps}
{#if modalProps && stableModalData}
<Modal
bind:this={modal}
{...modalProps.props}
{...stableModalData.props}
onClose={handleModalClose}
onSubmit={(close) => close()}
>
{#if modalProps.state.type === "commit-failed"}
<CommitFailedModalContent data={modalProps.state} oncloseclick={closeModal} />
{:else if modalProps.state.type === "author-missing"}
<AuthorMissingModalContent data={modalProps.state} close={closeModal} />
{:else if modalProps.state.type === "general-settings"}
<GeneralSettingsModalContent data={modalProps.state} />
{:else if modalProps.state.type === "project-settings"}
<ProjectSettingsModalContent data={modalProps.state} />
{:else if modalProps.state.type === "login-confirmation"}
<LoginConfirmationModalContent data={modalProps.state} close={closeModal} />
{#if stableModalData.state.type === "commit-failed"}
<CommitFailedModalContent data={stableModalData.state} oncloseclick={closeModal} />
{:else if stableModalData.state.type === "author-missing"}
<AuthorMissingModalContent data={stableModalData.state} close={closeModal} />
{:else if stableModalData.state.type === "general-settings"}
<GeneralSettingsModalContent data={stableModalData.state} />
{:else if stableModalData.state.type === "project-settings"}
<ProjectSettingsModalContent data={stableModalData.state} />
{:else if stableModalData.state.type === "login-confirmation"}
<LoginConfirmationModalContent data={stableModalData.state} close={closeModal} />
{/if}
</Modal>
{/if}
Loading