-
Notifications
You must be signed in to change notification settings - Fork 53.3k
refactor(editor): Improve published state clarity #23940
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
refactor(editor): Improve published state clarity #23940
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
E2E Tests: n8n tests passed after 5m 42.2s Run Details
Groups
This message was posted automatically by
currents.dev | Integration Settings
|
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.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.vue">
<violation number="1" location="packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.vue:237">
P2: The indicator will be rendered but invisible. With `showIndicator: true` and `indicatorClass: ''`, the dot element is created but has no background color since neither `indicatorPublished` nor `indicatorChanges` CSS classes are applied. Consider setting `indicatorClass: 'published'` to match the similar `'published-node-issues'` state, or keep `showIndicator: false` if no visual indicator is intended.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.vue
Show resolved
Hide resolved
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.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.vue">
<violation number="1" location="packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.vue:292">
P2: Removing `isWorkflowSaving` check allows clicking publish during ongoing manual saves. The `saveBeforePublish` function only handles autosave states but doesn't wait for manual saves in progress, which could lead to concurrent save operations. Consider either keeping this check or updating `saveBeforePublish` to await ongoing manual saves.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| <N8nButton | ||
| :loading="autoSaveForPublish" | ||
| :disabled="!publishButtonEnabled || isWorkflowSaving" | ||
| :disabled="!publishButtonConfig.enabled || readOnlyForPublish" |
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.
P2: Removing isWorkflowSaving check allows clicking publish during ongoing manual saves. The saveBeforePublish function only handles autosave states but doesn't wait for manual saves in progress, which could lead to concurrent save operations. Consider either keeping this check or updating saveBeforePublish to await ongoing manual saves.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.vue, line 292:
<comment>Removing `isWorkflowSaving` check allows clicking publish during ongoing manual saves. The `saveBeforePublish` function only handles autosave states but doesn't wait for manual saves in progress, which could lead to concurrent save operations. Consider either keeping this check or updating `saveBeforePublish` to await ongoing manual saves.</comment>
<file context>
@@ -289,7 +289,7 @@ defineExpose({
<N8nButton
:loading="autoSaveForPublish"
- :disabled="!publishButtonConfig.enabled || isWorkflowSaving || readOnlyForPublish"
+ :disabled="!publishButtonConfig.enabled || readOnlyForPublish"
type="secondary"
data-test-id="workflow-open-publish-modal-button"
</file context>
| :disabled="!publishButtonConfig.enabled || readOnlyForPublish" | |
| :disabled="!publishButtonConfig.enabled || isWorkflowSaving || readOnlyForPublish" |
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.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/frontend/editor-ui/src/features/workflows/workflowHistory/components/WorkflowHistoryListItem.vue">
<violation number="1" location="packages/frontend/editor-ui/src/features/workflows/workflowHistory/components/WorkflowHistoryListItem.vue:121">
P2: Missing condition check in `mainTooltipDate` for the "Current Changes" case (`index === 0 && !isVersionActive`). If `versionPublishInfo.value` exists, the tooltip will incorrectly show a TimeAgo date, which is a regression from the old behavior where no date was shown for "Current Changes".</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const publishedByUserName = computed(() => { | ||
| const userId = lastPublishInfo.value?.userId; | ||
| if (!userId) { | ||
| const mainTooltipDate = 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.
P2: Missing condition check in mainTooltipDate for the "Current Changes" case (index === 0 && !isVersionActive). If versionPublishInfo.value exists, the tooltip will incorrectly show a TimeAgo date, which is a regression from the old behavior where no date was shown for "Current Changes".
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/frontend/editor-ui/src/features/workflows/workflowHistory/components/WorkflowHistoryListItem.vue, line 121:
<comment>Missing condition check in `mainTooltipDate` for the "Current Changes" case (`index === 0 && !isVersionActive`). If `versionPublishInfo.value` exists, the tooltip will incorrectly show a TimeAgo date, which is a regression from the old behavior where no date was shown for "Current Changes".</comment>
<file context>
@@ -98,21 +98,40 @@ const versionPublishInfo = computed(() => {
-const publishedByUserName = computed(() => {
- const userId = lastPublishInfo.value?.userId;
- if (!userId) {
+const mainTooltipDate = computed(() => {
+ if (props.isGrouped) {
return null;
</file context>
|
@claude review |
|
Claude finished @RobTF9's task —— View job PR Review: Improve published state clarityReview Tasks
OverviewThis PR refactors the published state logic across workflow components to provide clearer visual indicators. The changes are well-structured and improve the user experience around workflow publishing states. Detailed Review✅ WorkflowHeaderDraftPublishActions.vue - Excellent refactoringThe complete rewrite with a proper Strengths:
Minor concern from cubic-dev-ai review:
|
Summary
Related Linear tickets, Github issues, and Community forum posts
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)