Skip to content
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.

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

Fix datasource deletion (when selected) causing builder crash #12091

Merged
merged 3 commits into from
Oct 18, 2023

Conversation

mike12345567
Copy link
Collaborator

@mike12345567 mike12345567 commented Oct 17, 2023

Description

Quick fix for datasource deletion, due to the datasource defaulting to an empty object it appeared like a datasource had been set and the UI would attempt to render with this empty state. When not selected move off of the selected datasource page instead and handle the datasource not being selected.

Addresses: https://linear.app/budibase/issue/BUDI-7630/error-in-console-when-deleting-a-datasource-navigation-then-breaks

Feature branch env

Feature Branch Link

…o an empty object it appeared like a datasource had been set and the UI would attempt to render with this empty state. When not selected move off of the selected datasource page instead and handle the datasource not being selected.
@mike12345567 mike12345567 self-assigned this Oct 17, 2023
Copy link
Collaborator

@samwho samwho left a comment

Choose a reason for hiding this comment

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

I pulled this down and gave it a try and it fixes the problem I found. The code looks reasonable to me though I will say I'm still quite fresh on the frontend code.

Comment on lines 22 to 26
$: {
if (!datasource) {
$goto("./datasource")
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ooh I didn't know you could do this! I assumed reactive statements had to be variable assignments. Cool!

Copy link
Member

@aptkingston aptkingston Oct 18, 2023

Choose a reason for hiding this comment

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

You can indeed! You can have any arbitrary code inside a reactive statement like this, and svelte will re-run it when any of the variables used inside the statement change. What svelte determines a "change" worth invalidating reactive statements is either a reassignment (if a normal JS variable) or a store value change (if using a svelte store).

Copy link
Member

@shogunpurple shogunpurple left a comment

Choose a reason for hiding this comment

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

Seems fixed on the feature branch!

Copy link
Member

@aptkingston aptkingston left a comment

Choose a reason for hiding this comment

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

The even cleaner way to fix this is simply wrap a guard around the slot inside [datasourceId]/_layout.svelte which ensures that we won't ever render a datasource details route without having one selected in state.

A quick test locally confirms that this does fix it (and is more in line with what we do elsewhere):

...
  {#if $datasources.selected}
    <slot />
  {/if}
...

We can also remove the fallback code of

  // datasources.selected can return null temporarily on datasource deletion
  $: datasource = $datasources.selected || {}

since that isn't needed either with this change.

Copy link
Member

@aptkingston aptkingston left a comment

Choose a reason for hiding this comment

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

LGTM!

@mike12345567 mike12345567 merged commit 3abb2fe into master Oct 18, 2023
10 checks passed
@mike12345567 mike12345567 deleted the fix/BUDI-7630-fix-datasource-deletion branch October 18, 2023 09:23
@github-actions github-actions bot locked and limited conversation to collaborators Oct 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants