-
Notifications
You must be signed in to change notification settings - Fork 52.7k
feat(editor): Filter Executions by executed node #16036
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
Conversation
Introduced the ability to filter executions by specific nodes in both backend and frontend components.
|
Alex O seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
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.
cubic found 10 issues across 12 files. Review them in cubic.dev
React with 👍 or 👎 to teach cubic. Tag @cubic-dev-ai to give specific feedback.
| "contextMenu.addSticky": "Add sticky note", | ||
| "contextMenu.editSticky": "Edit sticky note", | ||
| "contextMenu.changeColor": "Change color", | ||
| "contextMenu.FilterExecutionsBy": "Filter Executions by", |
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.
Translation key naming inconsistency. The key uses camelCase 'FilterExecutionsBy' but is referenced in code as 'filter_executions_by'.
| "contextMenu.FilterExecutionsBy": "Filter Executions by", | |
| "contextMenu.filter_executions_by": "Filter Executions by", |
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.
Will change this and some other variables to be consistent with the rest of the naming
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.
changed
| : [], | ||
| ); | ||
| const FilterExecutionsBy = computed(() => { |
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.
Rule violated: Ensure every PR has unit tests for Vue files
Missing unit tests for WorkflowExecutionsView.vue component
| const idConditions: string[] = []; | ||
| query.nodesExecuted.forEach((node, idx) => { | ||
| idConditions.push( | ||
| `json_type(json_extract(execution_data.data, '$[4].' || :nodeName${idx})) IS NOT NULL`, |
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.
SQL function json_extract may not be compatible with all database types
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.
Is this query executed in the sqlite and the postgres setup aswell?
Or only the sqlite?
Cause:
"json_extract is natively supported in modern SQLite versions (3.9.0 and above, released since 2015) if it is compiled with the JSON1 extension—most pre-built binaries include this by default."
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.
The query is executed in postgres set up as well. Can you change the way the execution data is extracted?
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.
updated the query to also support postgresdb
| const idConditions: string[] = []; | ||
| query.nodesExecuted.forEach((node, idx) => { | ||
| idConditions.push( | ||
| `json_type(json_extract(execution_data.data, '$[4].' || :nodeName${idx})) IS NOT NULL`, |
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.
Hardcoded array index '$[4]' in JSON path is brittle and lacks documentation
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.
I analyzed the json in the database and it seems the 4th element is a list where all executed nodes of an execution are listed. so we should always find the node we are searching for in the 4th element.
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.
The hardcoded index is brittle, indeed. @Alexandero89 have you found any other way to do this?
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.
actually it seems this is some kind of standard. The executed nodes are always saved at position 4.
I think someone of your team at n8n should know if this is a standard and if its always saved at this position or if we can do this in some better way.
| startedBefore?: string; | ||
| annotationTags?: string[]; | ||
| vote?: ExecutionFilterVote; | ||
| nodesExecuted: string[]; |
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.
Type inconsistency between ExecutionFilterType and ExecutionsQueryFilter interfaces. In ExecutionFilterType, nodesExecuted is optional, but in ExecutionsQueryFilter, it's required. This could cause runtime errors when transforming between these types in executionFilterToQueryFilter function.
| nodesExecuted: string[]; | |
| + nodesExecuted?: string[]; |
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.
done
| import Edge from './elements/edges/CanvasEdge.vue'; | ||
| import Node from './elements/nodes/CanvasNode.vue'; | ||
| import { useViewportAutoAdjust } from '@/components/canvas/composables/useViewportAutoAdjust'; | ||
| import { useRouter, useRoute } from 'vue-router'; |
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.
Rule violated: Ensure every PR has unit tests for Vue files
Missing unit tests for the new filter executions functionality
| async function onFilterExecutionBy(input: { ids: string[] }) { | ||
| const workflowId = route.params.name as string; | ||
| if (!workflowId || !input || !input.ids || input.ids.length === 0) return; | ||
| const customFilter = nodeDataById.value[input.ids[0]]?.name; |
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.
Missing validation for nodeDataById.value[input.ids[0]] existence before accessing name property
| const customFilter = nodeDataById.value[input.ids[0]]?.name; | |
| + const customFilter = nodeDataById.value[input.ids[0]]?.name || undefined; |
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.
done
| const qb = this.createQueryBuilder('execution') | ||
| .select(fields) | ||
| .innerJoin('execution.workflow', 'workflow') | ||
| // Add join to execution_data in the query builder |
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.
Comment indicates this join might not be needed for all queries, but it's added unconditionally
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.
I will have to think about how to solve this better. not every execution needs this join.
feedback is very welcomed here
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.
implemented a better solution in the new commit. have a look
| payload?: CanvasNodeEventBusEvents[keyof CanvasNodeEventBusEvents]; | ||
| }; | ||
| tidyUp: { source: CanvasLayoutSource }; | ||
| filter_executions_by: { ids: string[] }; |
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.
The event only uses the first node ID from the array in the implementation, but the type suggests multiple IDs are supported.
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.
For this proof of concept i implemented the filtering for only one node.
The idea is later to be able to filter by multiple nodes that are marked.
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.
Let's start with one node 👍
| case 'extract_sub_workflow': | ||
| return emit('extract-workflow', nodeIds); | ||
| case 'filter_executions_by': | ||
| onFilterExecutionBy({ ids: nodeIds }); |
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.
Redundant function call before emitting the same event
|
Hey @Alexandero89, Thanks for the PR, We have created "GHC-2353" as the internal reference to get this reviewed. One of us will be in touch if there are any changes needed, in most cases this is normally within a couple of weeks but it depends on the current workload of the team. |
BundleMonUnchanged files (2)
No change in files bundle size Groups updated (2)
Final result: ✅ View report in BundleMon website ➡️ |
| .innerJoin('execution.workflow', 'workflow'); | ||
|
|
||
| // Add join to execution_data only if nodesExecuted is not null or not an empty array | ||
| if (nodesExecuted && Array.isArray(nodesExecuted) ? nodesExecuted.length > 0 : 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.
Won't this always be true?
If nodesExecuted is undefined?, then true is returned,
If it's an array, nodesExecuted.length > 0 is returned which is also 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.
You are right. Changed the check to be more precise
nikhilkuria
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.
Really nice idea @Alexandero89 👏
Let's polish it and make it better
|
Also, can you fix the broken tests? |
changing label
better check for nodesExecuted not to be empty
better db query to also support postgresdb
void the promise instead of ignoring it
| if (query.nodesExecuted && query.nodesExecuted.length > 0) { | ||
| const idConditions: string[] = []; | ||
| query.nodesExecuted.forEach((node, idx) => { | ||
| const fragment = isPostgres |
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.
Did you get the changes to work in a postgresdb?
I had to change it as follows for the filtering to work
const fragment = isPostgres
? `jsonb_exists(execution_data.data::jsonb->4, :nodeName${idx})`
: `json_type(json_extract(execution_data.data, '$[4].' || :nodeName${idx})) IS NOT NULL`;
Is this related to different postgres versions?
|
I think it's shaping real nice. The check for postgres is the way to go. Nice work! |
|
Can you also take a look here, please? |
|
Hi @Alexandero89 did you manage to take a look at my last comments |
|
@Alexandero89 we are closing this PR. When you find time to work on, please reopen and we can continue our conversation |
Introduced the ability to filter executions by specific nodes in both backend and frontend components.
Summary
This PR is a first draft and also serves as a Request for Comments (RFC).
Feedback and suggestions are highly appreciated!
Showcasevideo
bildschirmaufzeichnung-vom-2025-06-05-02-47-12_wKow5MCV.mp4
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)