fix(editor): Don't render now when startedAt is null
#15283
Conversation
…them always at the top of the list
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
n8n-assistant
bot
added
core
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
mrge found 5 issues across 10 files. Review them in mrge.io
packages/frontend/editor-ui/src/components/executions/global/GlobalExecutionsListItem.vue
Show resolved
Hide resolved
packages/frontend/editor-ui/src/components/executions/global/GlobalExecutionsListItem.vue
Show resolved
Hide resolved
packages/frontend/editor-ui/src/components/executions/workflow/WorkflowExecutionsCard.vue
Show resolved
Hide resolved
| @@ -130,7 +130,7 @@ const formattedStoppedAtDate = computed(() => { | |||
| return props.execution.stoppedAt | |||
| ? locale.displayTimer( | |||
| new Date(props.execution.stoppedAt).getTime() - | |||
| new Date(props.execution.startedAt).getTime(), | |||
| new Date(props.execution.startedAt ?? props.execution.createdAt).getTime(), | |||
There was a problem hiding this comment.
It could be nice for readability and maintainability to extract that into a function, what do you think ?
There was a problem hiding this comment.
Like this:
export function startedAtOrCreatedAt(execution: ExecutionSummary) {
return execution.startedAt ?? execution.createdAt
}
I'm not sure if this increases readability. It contains just one line and the name of the function is almost as long as the implementation. But it does create an abstraction that you have to learn.
I think what could make sense is to fix the types, for example one of these:
- creating a domain specific interface and have the store map the execution summary internally, e.g. when it get's it back from the API, it could create a field that is used by the executions lists to, e.g.
lastUpdatedor something more specific - create a mapping function that maps the execution summary to distinct types by status that this component can render, then the
startedAtwould exist forsuccess, not exist forqueuedand be optional forfailure
But all of this I would leave up to those who maintain the editor code base.
There was a problem hiding this comment.
Make sense. I like the second solution, but agreed on the whole reply
|
|
|
|
|
✅ All Cypress E2E specs passed |
|
✅ All Cypress E2E specs passed |
despairblue
deleted the
pay-2777-executions-list-always-shows-executions-as-recently-finished
branch
|
Got released with |
Summary
Executions can fail before they were started in queue mode. This happens when the worker looses the connection to postgres, but not to redis.
Then fetching or updating the execution fails, the error gets propagated to the main instance which marks the execution as failed. Then the execution won't have
startedAtset.This leads to the executions appearing as always new. Additionally they block all other executions in the executions list because
startedAt: nullis considered larger than a date and thus comes first when sorted descendingly.This was dubbed "ghost executions" by some customers, because they assumed the executions would continue running.
Date before:
pay-2777-before.mp4
Date after:
pay-2777-after.mp4
Sorting before:
pay-2777-2-before.mp4
Sorting after:
pay-2777-2-after.mp4
Related Linear tickets, Github issues, and Community forum posts
https://linear.app/n8n/issue/PAY-2777/executions-list-always-shows-executions-as-recently-finished-if-they
Review / Merge checklist
Docs updated or follow-up ticket created.PR Labeled withrelease/backport(if the PR is an urgent fix that needs to be backported)