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

Refetch aggregate queries on record creation/update/deletion of record #8885

Merged
merged 7 commits into from
Dec 5, 2024

Conversation

ijreilly
Copy link
Collaborator

@ijreilly ijreilly commented Dec 5, 2024

Closes #8755.
Refetching the aggregate queries on an object following creation, update, deletion of a record.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR adds functionality to refetch aggregate queries after record operations, since aggregate values cannot be computed optimistically in the cache like regular queries.

  • Added new useRefetchAggregateQueries hook in /packages/twenty-front/src/modules/object-record/hooks/useRefetchAggregateQueries.ts to centralize aggregate query refetching logic
  • Integrated refetch calls after mutations in record operation hooks (create/update/delete) to keep aggregate data in sync
  • Potential issue: useUpdateOneRecord.ts has duplicate refetch calls in both mutation update callback and completion
  • Added feature flag IS_AGGREGATE_QUERY_ENABLED to control aggregate functionality with proper fallbacks
  • Improved error handling needed in useRefetchAggregateQueries for failed refetch operations

13 file(s) reviewed, 12 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines +149 to 150
await refetchAggregateQueries();
return createdObjects.data?.[mutationResponseField] ?? [];
Copy link
Contributor

Choose a reason for hiding this comment

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

style: consider wrapping refetch in try/catch to handle potential refetch failures gracefully

Comment on lines 134 to +135

await refetchAggregateQueries();
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: refetchAggregateQueries could fail silently here - consider handling errors or at least logging them

@@ -126,6 +131,7 @@ export const useDeleteOneRecord = ({
throw error;
});

await refetchAggregateQueries();
Copy link
Contributor

Choose a reason for hiding this comment

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

style: refetchAggregateQueries() should be wrapped in a try/catch to handle potential refetch failures gracefully

export const useRefetchAggregateQueries = ({
objectMetadataNamePlural,
}: {
objectMetadataNamePlural: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: objectMetadataNamePlural should be validated - getAggregateQueryName will throw if this is empty

Copy link
Member

@charlesBochet charlesBochet left a comment

Choose a reason for hiding this comment

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

LGTM, could you add tests on your utils and hooks?

  • generatedAggregateQuery.ts + useAggregateManyQuery.ts
  • getAggregateQueryName.ts

@@ -141,6 +146,7 @@ export const useCreateManyRecords = <
throw error;
});

await refetchAggregateQueries();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this will seek to refetch aggregate queries even if there were no AggregateQueries performed before (like on table view for instance). I think it's ok but it does trigger a warning (yellow) in the console.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

@charlesBochet charlesBochet left a comment

Choose a reason for hiding this comment

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

Also, not related to this PR but we have inconsitent naming:
useAggregateManyRecords vs generateAggregateQuery.ts

I feel the many is implicit and there is no point having it: useAggregateManyRecords => useAggregateRecords

Copy link
Member

@Weiko Weiko left a comment

Choose a reason for hiding this comment

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

LGTM

@lucasbordeau
Copy link
Contributor

Nice

@ijreilly ijreilly merged commit 26ff344 into main Dec 5, 2024
19 checks passed
@ijreilly ijreilly deleted the handle-aggregate-refetch branch December 5, 2024 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle refetch of useAggregate at each record update
4 participants