-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat(customers): Filter insight by person in PersonFeedCanvas #42247
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
base: master
Are you sure you want to change the base?
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Size Change: 0 B Total Size: 3.41 MB ℹ️ View Unchanged
|
42868dc to
512e8c9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4 files reviewed, 3 comments
| if ( | ||
| isInsightVizNode(modifiedQuery) && | ||
| !isHogQLQuery(modifiedQuery.source) && | ||
| !isActorsQuery(modifiedQuery.source) && | ||
| !isDefaultFilterApplied | ||
| ) { | ||
| modifiedQuery.source.properties = canvasFiltersOverride | ||
| updateAttributes({ ...attributes, isDefaultFilterApplied: true }) | ||
| } | ||
|
|
||
| if (isDataTableNode(modifiedQuery) && isEventsQuery(modifiedQuery.source) && !isDefaultFilterApplied) { | ||
| modifiedQuery.source.fixedProperties = canvasFiltersOverride | ||
| updateAttributes({ ...attributes, isDefaultFilterApplied: true }) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: updateAttributes is called inside useMemo, which violates React rules - useMemo should be pure and not have side effects. This can cause unexpected behavior and infinite render loops.
Move the filter application logic to a useEffect hook instead:
useEffect(() => {
if (
isInsightVizNode(modifiedQuery) &&
!isHogQLQuery(modifiedQuery.source) &&
!isActorsQuery(modifiedQuery.source) &&
!isDefaultFilterApplied &&
canvasFiltersOverride
) {
updateAttributes({ ...attributes, isDefaultFilterApplied: true })
}
if (isDataTableNode(modifiedQuery) && isEventsQuery(modifiedQuery.source) && !isDefaultFilterApplied && canvasFiltersOverride) {
updateAttributes({ ...attributes, isDefaultFilterApplied: true })
}
}, [isDefaultFilterApplied, canvasFiltersOverride, query.kind, query.source?.kind])
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/scenes/notebooks/Nodes/NotebookNodeQuery.tsx
Line: 184:197
Comment:
**logic:** `updateAttributes` is called inside `useMemo`, which violates React rules - `useMemo` should be pure and not have side effects. This can cause unexpected behavior and infinite render loops.
Move the filter application logic to a `useEffect` hook instead:
```
useEffect(() => {
if (
isInsightVizNode(modifiedQuery) &&
!isHogQLQuery(modifiedQuery.source) &&
!isActorsQuery(modifiedQuery.source) &&
!isDefaultFilterApplied &&
canvasFiltersOverride
) {
updateAttributes({ ...attributes, isDefaultFilterApplied: true })
}
if (isDataTableNode(modifiedQuery) && isEventsQuery(modifiedQuery.source) && !isDefaultFilterApplied && canvasFiltersOverride) {
updateAttributes({ ...attributes, isDefaultFilterApplied: true })
}
}, [isDefaultFilterApplied, canvasFiltersOverride, query.kind, query.source?.kind])
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this specific case is safe, as we're updating isDefaultFilterApplied, which is not in the dependency array.
daibhin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a comment on the prop in Notebooks. I'm going to tag the PA team because they own the query node (I'm trying to keep my involvement with notebooks to the framework level)
1c8b4e8 to
f4403f6
Compare

Problem
When viewing a person's feed in a notebook, we need to automatically apply filters to show only events related to that person. Currently, this filtering is not applied automatically, requiring users to manually set up filters.
Changes
canvasFiltersOverrideto the Notebook component and notebookLogic to allow passing filters from parent componentsisDataTableNode,isEventsQuery, etc.How did you test this code?
Changelog: Yes