Skip to content

Commit

Permalink
fix(editor): Items count display in running workflow (#8148)
Browse files Browse the repository at this point in the history
## Summary
> Describe what the PR does and how to test. Photos and videos are
recommended.

Every time the `MainHeader` component is created, we register a new
handler for the "push" messages. Unfortunately this becomes an issue if
you go multiple times to a page that renders the `MainHeader`, e.g
`/workflow/:id`, without refreshing the page; because all handlers will
be called, causing behavior duplication.

I added the possibility of passing an ID, and made impossible to have
multiple handlers with the same ID However, it does not seems to be
needed to support an array of handlers in the pushConnection store. If
that is the case:

1. We might want to have only one handler for the push connections at
all times, which would be a much simpler approach.
2. Register the handler on app.mount instead.

The issue seems to have been introduced
[here](https://github.com/n8n-io/n8n/pull/7763/files#diff-f5dae80a64b9951bb6691f1b9d439423cf84fa0cc9601b3f2c00904f3135e8deR48)

Before the change:

https://www.loom.com/share/85cf8ef896254d848a13a6c6438daa47

With the change:

https://www.loom.com/share/f5c4ffac421d46cc8e389364e1c357d3

## Related tickets and issues

https://linear.app/n8n/issue/ADO-1596/bug-items-count-display-in-running-workflow


## Review / Merge checklist
- [x] PR title and summary are descriptive. **Remember, the title
automatically goes into the changelog. Use `(no-changelog)` otherwise.**
([conventions](https://github.com/n8n-io/n8n/blob/master/.github/pull_request_title_conventions.md))
- [ ] [Docs updated](https://github.com/n8n-io/n8n-docs) or follow-up
ticket created.
- [ ] Tests included.
> A bug is not considered fixed, unless a test is added to prevent it
from happening again.
   > A feature is not complete without tests.
  • Loading branch information
RicardoE105 committed Jan 3, 2024
1 parent 22a5f52 commit 8a3c87f
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 2 deletions.
8 changes: 7 additions & 1 deletion packages/editor-ui/src/mixins/pushConnection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,19 @@ export const pushConnection = defineComponent({
return {
retryTimeout: null as NodeJS.Timeout | null,
pushMessageQueue: [] as Array<{ message: IPushData; retriesLeft: number }>,
removeEventListener: null as (() => void) | null,
};
},
created() {
this.pushStore.addEventListener((message) => {
this.removeEventListener = this.pushStore.addEventListener((message) => {
void this.pushMessageReceived(message);
});
},
unmounted() {
if (typeof this.removeEventListener === 'function') {
this.removeEventListener();
}
},
computed: {
...mapStores(
useCredentialsStore,
Expand Down
9 changes: 8 additions & 1 deletion packages/editor-ui/src/stores/pushConnection.store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,17 @@ export const usePushConnectionStore = defineStore(STORES.PUSH, () => {
const lostConnection = ref(false);
const outgoingQueue = ref<unknown[]>([]);
const isConnectionOpen = ref(false);

const onMessageReceivedHandlers = ref<OnPushMessageHandler[]>([]);

const addEventListener = (handler: OnPushMessageHandler) => {
onMessageReceivedHandlers.value.push(handler);
return () => {
const index = onMessageReceivedHandlers.value.indexOf(handler);
if (index !== -1) {
onMessageReceivedHandlers.value.splice(index, 1);
}
};
};

function onConnectionError() {
Expand Down Expand Up @@ -139,7 +146,7 @@ export const usePushConnectionStore = defineStore(STORES.PUSH, () => {
} catch (error) {
return;
}
// TODO: Why is this received multiple times?

onMessageReceivedHandlers.value.forEach((handler) => handler(receivedData));
}

Expand Down

0 comments on commit 8a3c87f

Please sign in to comment.