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

Validate datasource setting on components #15414

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

adrinr
Copy link
Collaborator

@adrinr adrinr commented Jan 21, 2025

Description

Validate "table" settings on components. This is used in the components form block, multistep form block and row explorer. If the set table/view does not exist anymore, we will display a friendly error instead of a confusing error.

Screenshots

Previous

image

New behaviour

image

Launchcontrol

Improve verbosity for wrong datasource settings values

Copy link

linear bot commented Jan 21, 2025

Copy link

qa-wolf bot commented Jan 21, 2025

QA Wolf here! As you write new code it's important that your test coverage is keeping up.
Click here to request test coverage for this PR!

@github-actions github-actions bot added firestorm Data/Infra/Revenue Team size/m labels Jan 21, 2025
@adrinr adrinr marked this pull request as ready for review January 21, 2025 14:40
@adrinr adrinr requested a review from a team as a code owner January 21, 2025 14:40
@adrinr adrinr requested review from mike12345567 and removed request for a team January 21, 2025 14:40
Copy link
Collaborator

@mike12345567 mike12345567 left a comment

Choose a reason for hiding this comment

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

This LGTM - but I'll leave @aptkingston to comment as well - given it crosses the builder/client boundary!

@adrinr adrinr added the do not merge PR is not ready to be merged - generally the PR description should say why label Jan 21, 2025
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.

Nice work @adrinr! Glad this approach worked out 👌 Mostly just a few opinions here - nothing major other than just being aware that this currently wouldn't support component plugins, which is probably fine.

packages/builder/src/stores/builder/screenComponent.ts Outdated Show resolved Hide resolved

recurseFieldComponentsInChildren(screen?.props, type)
return result
}
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't account for custom components, which are stored separately, but that's probably fine. Just as long as it's a known limitation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will create a ticket for a "nice to have", and to tackle this if we have some extra time

packages/builder/src/stores/builder/screenComponent.ts Outdated Show resolved Hide resolved
packages/client/src/components/Component.svelte Outdated Show resolved Hide resolved
@adrinr
Copy link
Collaborator Author

adrinr commented Jan 21, 2025

Found an issue that causes the component to render to a frame with the wrong data, calling the API with the invalid data. This displays an unexpected error notification. Investigating why

@adrinr adrinr requested a review from aptkingston January 22, 2025 15:31
@adrinr
Copy link
Collaborator Author

adrinr commented Jan 22, 2025

Found an issue that causes the component to render to a frame with the wrong data, calling the API with the invalid data. This displays an unexpected error notification. Investigating why

Solved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge PR is not ready to be merged - generally the PR description should say why firestorm Data/Infra/Revenue Team size/m
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants