This is related to an issue with how Bull handles stalled jobs, see
https://github.com/OptimalBits/bull/issues/1415 for reference.
CPU intensive workflows can in certain cases take a long while to finish
up, thereby blocking the thread and causing Bull queue to think the job
has stalled, even though it finished successfully. In these cases the
error handling could then overwrite the successful execution data with
the error message.
## Issue
In community edition, clicking on "View plans" button on "Settings" ->
"Usage and plan" page (e.g. http://127.0.0.1:5678/settings/usage) opens
two new tabs with n8n pricing (one of them with UTM tracking, another
without).
This was introduced in #6317 , when click handler of "View plans" link
container [started
calling](https://github.com/n8n-io/n8n/pull/6317/files#diff-0bf26afac8a06e03b3d39d0668f22408859355b585a9ab420800c125e33f0691R109)
`uiStore.goToUpgrade(...)` which opens n8n pricing in a new tab, while
browser opens another tab for the link URL.
The simplest fix, implemented in this PR, is to prevent default event
handling (so that, after `onViewPlans` is called, browser will not
attempt to process the click additionally as clicking on the link),
similarly to how it is prevented on some other pages. It only solves the
immediate problem of browser opening two new tabs on clicking "View
plans".
Note that **I didn't implement any tests for the changed behavior**,
because it was not covered by tests before, and I couldn't quite figure
out how to cover it now within the existing test approach (considering
that testing the fact that only one new tab is open will likely require
to write entirely new tests relying on puppeteer; as far as I can see,
no existing `editor-ui` tests are doing anything like that). I'll gladly
implement tests for the new behavior if you tell me how you would like
them to look.
The existing tests for `editor-ui` still pass; I didn't run tests for
other subpackages (see "additional contribution notes" below).
## Additional notes on the issue.
I'm not sure that the change in this PR is the correct long-term
solution for the issue, because the URLs for these two methods (custom
click handler for link container and default link handling) are slightly
different:
* Custom click handler calls `useTelemetryStore().track('User clicked
upgrade CTA', ...)`; then calls `sendUsageTelemetry('view_plans')` (it
feels weird that two calls to telemetry are made); then opens new tab
for `https://n8n.io/pricing?utm_campaign=open&source=usage_page` (note
that prior to #7316 the second call to telemetry was done after the new
tab is opened, not before);
* Link itself refers to another page, with slightly different tracking
parameters:
`https://subscription.n8n.io/?instanceid=[REDACTED]&version=1.10.0&callback=http%3A%2F%2F127.0.0.1%3A5678%2Fsettings%2Fusage&source=usage_page`;
but this page redirects to `https://n8n.io/pricing/`.
It is not clear which one of the two is the right way of doing things.
Although `goToUpgrade` is called in 20 places throughout `editor-ui`,
while `viewPlansUrl`, as far as I can see, is used for this button only.
Additionally, since Settings pages don't work without JS anyway, I can
only think of two separate scenarios where any tab would be opened:
* Left-clicking the link (or Ctrl-clicking, or pressing Space or Enter
when the link is focused, or tapping): previously, both custom click
handler was executed and link's `href` was opened; in this PR, only
custom click handler is executed (similarly to how it is done in the
other places where `goToUpgrade` is called);
* Right-clicking (or long tapping, or opening context menu in any other
way) and selecting "open link in new tab" (or similar): opens a new tab
for URL from the `href` attribute (and does not send any telemetry at
all).
I'd say that the better permanent solution would probably be to get rid
of one of these methods entirely, and only rely on another in all cases
(for me, as an outside contributor, the preferred way would be for
custom click handler to only send telemetry, while letting my browser
handle the actual navigation). However, that would be a large change,
much more than one line in this PR.
Additionally, other similar places where `goToUpgrade` is currently
called (directly or indirectly) would also need to be adapted for this
change.
## Additional contribution notes
As a first-time contributor, I've encountered several things I didn't
expect; I'm not sure if they should be expected or are issues:
1. Tests for the entire monorepo consume a lot of RAM; 20GB free RAM was
not enough, so I couldn't run tests for the entire monorepo and had to
only run them for `packages/editor-ui`;
2. Linting is very slow; `pnpm lint` in `packages/editor-ui` takes ten
minutes to complete;
3. It seems that types are not checked. Code OSS highlights numerous
errors in code files: for example, `'debug'` is incompatible with
`CloudUpdateLinkSourceType` expected by `goToUpgrade` here:
3e7a4d3b2c/packages/editor-ui/src/composables/useExecutionDebugging.ts (L128)
However, I'm not getting any errors during build. There is a `typecheck`
script defined in `package.json`, but `pnpm typecheck` fails with:
```
n8n-toy-demo:~/projects/n8n/packages/editor-ui$ pnpm typecheck
> n8n-editor-ui@1.10.0 typecheck
/home/inga/projects/n8n/packages/editor-ui
> vue-tsc --emitDeclarationOnly
error TS5069: Option 'emitDeclarationOnly' cannot be specified without
specifying
option 'declaration' or option 'composite'.
Found 1 error.
ELIFECYCLE Command failed with exit code 1.
n8n-toy-demo:~/projects/n8n/packages/editor-ui$
```
Replacing `--emitDeclarationsOnly` with `--noEmit` in `package.json`
unblocks typechecking and results in seemingly, at first glance, correct
"Found 1924 errors in 306 files" (at least several of the reported
errors that I've checked seem to be correct).
But maybe I'm missing something and there are not in fact two thousands
type errors in `editor-ui`?
In a rare edge case an undefined queue could be returned - this should
not happen and now an error is thrown.
Also using the opportunity to remove a cyclic dependency from the Queue.
Github issue / Community forum post (link here to close automatically):
#7423
This PR updates reference passed to the `checkout` action by the
`cy-pull-request.ym`. This should fix three existing issues:
- Failing unit tests for external pull requests
- Failing e2e tests for external PRs
- Passing empty `ref` to `lint` job which makes linter run on a wrong
branch
- Fix pinch-to-zoom
- Support command + scroll to zoom
- Improve accuracy of zooming (scroll more = zoom more)
- Zoom limits
- Zoom relative to mouse position
