Adds table missing alert to destination cards on Job page #3184
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
alishakawaguchi
commented
Jan 23, 2025
alishakawaguchi
commented
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
nickzelei
reviewed
Jan 23, 2025
| @@ -159,10 +221,43 @@ export default function DestinationConnectionCard({ | |||
| <FormDescription> | |||
| The location of the destination data set. | |||
| </FormDescription> | |||
| <FormMessage /> | |||
| {/* <FormMessage /> */} | |||
Comment on lines
96
to
125
| async function validateMappings(connectionId: string) { | ||
| if (!jobmappings || jobmappings.length == 0) { | ||
| return; | ||
| } | ||
| try { | ||
| const body = create(ValidateJobMappingsRequestSchema, { | ||
| accountId: account?.id, | ||
| mappings: jobmappings, | ||
| virtualForeignKeys: [], | ||
| connectionId: connectionId, | ||
| }); | ||
| const res = await validateJobMappingsAsync(body); | ||
| setValidateMappingsResponse(res); | ||
| } catch (error) { | ||
| console.error('Failed to validate job mappings:', error); | ||
| toast.error('Unable to validate job mappings', { | ||
| description: getErrorMessage(error), | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| useEffect(() => { | ||
| if (!account?.id || !destination.connectionId || jobmappingsLength === 0) { | ||
| return; | ||
| } | ||
| const validateJobMappings = async () => { | ||
| await validateMappings(destination.connectionId); | ||
| }; | ||
| validateJobMappings(); | ||
| }, [account?.id, destination.connectionId, jobmappingsLength]); |
There was a problem hiding this comment.
Hm, why isn't this just a useQuery? All queries can be conditionally enabled anyways, which is basically your if condition.
There was a problem hiding this comment.
Another thought is that we could also conditionally invoke only if the init table schema is not enabled? maybe that is unnecessary though.
There was a problem hiding this comment.
copy paste 😭
nickzelei
reviewed
Jan 23, 2025
Comment on lines
231
to
232
| tableErrors && | ||
| tableErrors.length > 0 && ( |
There was a problem hiding this comment.
probably worth adding a loading state/skeleton here from the query for larger databases where the validation may take a while.
nickzelei
approved these changes
Jan 23, 2025
| @@ -195,7 +172,7 @@ export default function DestinationConnectionCard({ | |||
| shouldValidate: true, | |||
| } | |||
| ); | |||
| validateMappings(value); | |||
| // validateMappings(value); | |||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.