feat(router): move pipelinq off hash routing to clean path URLs - #1684
Merged
Conversation
Sixth app off `#` routing, after stackiq (softwarecatalog#899), zaakafhandelapp (#609), opencatalogi (#1341), keepiq (#582) and larpinq (#651). Two source parts: 1. createWebHashHistory -> createWebHistory. 2. routerBase(), derived from the URL actually being served. Nextcloud serves the app under BOTH /apps/pipelinq/... and /index.php/apps/pipelinq/..., but generateUrl() returns only the form the instance is configured for. Arriving on the other leaves the path outside the router base, vue-router cannot resolve it, and the catch-all redirects to '/' -- no error, deep link silently swallowed. This suite pins the /index.php form in several specs, so without it every deep link the suite makes would break. The test surface was the bulk of the work, and it was not only URLs: 30 spec files `${APP}/#/x`, `/apps/pipelinq/#/x` -> real paths navigation.spec.ts REQUIRED_ENTRIES mapped page ids to HASH hrefs and asserted equality; now route paths compared as a SUFFIX, because the base legitimately differs between the two URL forms and pinning either one would fail on the other for a non-defect dashboard.spec.ts default-route URL regex, and the KPI tile selector `a[href^="#/"]` + its `/^#\\//` assertion xwiki + declarative-view-system 9 toHaveURL(/#\\/…/) assertions helpers/pipelinq.ts revealNavEntry() selected leaves by `href*="#/"`, which matches NOTHING once the shell leaves hash routing. Leaves are now selected by the app segment; group captions keep their bare `href="#"`, so that remains the discriminator between the two. Stale prose corrected rather than deleted in helpers/pipelinq.ts, navigation.spec.ts and the event-wiring spec: all three told the reader a path deep-link resets the SPA to the Dashboard, which was true and is now the opposite. Verified against the published @conduction/nextcloud-vue (USE_LOCAL_LIB=false): /apps/pipelinq/ -> Sales overview /apps/pipelinq/leads -> Leads /index.php/apps/pipelinq/leads -> Leads /index.php/apps/pipelinq/clients -> Clients No hash anywhere, and nav hrefs are real paths carrying whichever base the page was loaded under -- which is exactly why the suffix assertion replaced the equality one. e2e: navigation.spec.ts + dashboard.spec.ts 9/9, event-wiring.spec.ts 3/3. eslint exits 0 and prettier --check on the FULL glob is clean.
Contributor
Quality Report — ConductionNL/pipelinq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| check-vue-demi | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| check-l10n-js | ✅ | ||||
| composer | ✅ | ✅ 106/106 | |||
| npm | ✅ | ✅ 637/637 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-09-01 08:55 UTC
Download the full PDF report from the workflow artifacts.
rubenvdlinde
added a commit
that referenced
this pull request
Sep 1, 2026
…lf-done (#1694) development went from 2 failing specs to 54 after #1684 moved pipelinq off hash routing. The migration updated 34 files but left two things behind. ## visual-coverage-spa-pages.spec.ts: 40+ of the 54 openSpaRoute changed its goto and not its assertion: - page.goto(`/apps/pipelinq/#${hash}`) + page.goto(`/apps/pipelinq/${hash}`) with `hash` values like "/my-work". That is TWO bugs in one line change: - the URL became /apps/pipelinq//my-work, a double slash, because the base already ends in "/" and the argument already starts with one; - the assertion three lines below still built `new RegExp("#" + hash + "$")`, so it waited 10s for a "#" that path routing never produces. The observed failure was "Received: http://localhost:8080/apps/pipelinq/", the catch-all redirect to the Dashboard, which is exactly what the comment above it predicts an unmatched route does. Renamed the parameter `hash` -> `route` so the next reader is not told it is a hash, and rewrote that comment: the surviving PATH is now the proof the route matched. ## Eight assertions in four other specs pipeline, pipelinq-pos-grouping, pos-transaction-core and request-management still carried /#\\/…/ patterns for navClick targets and one href check.⚠️ Third time today this exact trap has cost a red development: a pull_request into development SKIPS the E2E job, so a routing or naming change merges green and only breaks afterwards, in specs the PR never touched. #1672 did it with a nav rename, #1684 with the router, and both were green when merged. Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
rubenvdlinde
added a commit
that referenced
this pull request
Sep 1, 2026
… URLs (#1703) The last hash-routed surface in the fleet. The seven app shells went first (softwarecatalog#899, zaakafhandelapp#609, opencatalogi#1341, keepiq#582, larpinq#651, pipelinq#1684, openregister#3270); pipelinq's customer portal is a SECOND entry point (src/portal.js) and was left behind. The server already serves it: `portalPage#subpath` answers /portal/{path} with requirement `^(?!api/).*`, so every portal deep link reaches the SPA shell while /portal/api/* stays with the real controllers. That api exclusion is precisely what makes history mode safe here — and it is not hypothetical, because the booking client WAS calling /portal/services and getting the shell back at HTTP 200 until #1697. Two source parts: 1. createWebHashHistory -> createWebHistory. 2. routerBase(), derived from the URL actually being served. Nextcloud serves the portal under BOTH /apps/pipelinq/portal/... and /index.php/apps/pipelinq/portal/..., but generateUrl() returns only the form the instance is configured for. Arriving on the other leaves the path outside the router base, vue-router resolves nothing, and the portal lands on its default route — which for a customer-facing booking link means the booking is silently lost with no error shown. Test changes, and one of them is not a URL: appointment-booking.spec.ts BOOKING_ROUTE dropped its `#` visual-coverage-booking-portal openPortalRoute() built `${BASE}#${hash}`.⚠️ Its access-proof assertion ALSO read the hash — `toHaveURL(new RegExp('#' + hash))` — and renaming the parameter alone left that behind as `ReferenceError: hash is not defined`. The assertion now matches the PATH suffix and keeps its original job: installPortalGuard() rewrites to /login for anything without meta.public, so a URL that still ends in the requested route proves both that the route matched and that it was classified public (ADR-005). Also corrected three stale comments that told the reader pipelinq is hash-routed — ci-seed.sh, visual-coverage-export-pages and visual-coverage-spa-pages. All three were left over from #1684 and now describe the opposite of what the code does. ci-seed.sh's mattered most: it justified NOT worrying about overwrite.cli.url by saying hash history makes the path prefix irrelevant. That immunity is real but it now comes from routerBase(), not from the hash, and the comment said otherwise. Verified against the dev instance, with a build of this branch served to the browser: /apps/pipelinq/portal/book/haircut-simple resolves /index.php/apps/pipelinq/portal/book/haircut-simple resolves RELOAD on both resolves No hash in either URL. The reload is the case hash mode existed to avoid. Both were run twice to separate flaky from broken — the shared dev container drops into maintenance mode intermittently, which fails global-setup at `input[name="user"]` and reads exactly like a routing break. e2e: visual-coverage-booking-portal + portal-accessibility, 8/8 pass. eslint 0 errors, prettier --check on the FULL glob clean, webpack compiles, and `createWebHashHistory` appears zero times in the portal bundle. Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
rubenvdlinde
added a commit
that referenced
this pull request
Sep 1, 2026
* fix(e2e): follow the shell off hash routing onto path deep links #1684 moved pipelinq's shell to createWebHistory and #1703 did the same for the customer portal. The e2e suite kept navigating by hash, so every '#/<route>' goto mounted the app at its default route: the shell was present and the assertions read a Dashboard that was never the page under test. That is the whole of the 48-failure run — pages.spec alone lost all eleven index routes to it. Two shapes were wrong: - The route constants and the portal gotos still carried a '#'. - Six shared helpers joined a leading-slash route onto a trailing-slash base ('/apps/pipelinq/' + '/bookings'), which had been harmless while the '#' separated them and became '/apps/pipelinq//bookings' without it. A doubled slash falls outside the router base, so it resolved nowhere. The portal specs passed on the previous run only because #1703 had not landed yet; they carry the same defect and are fixed here too. * style(e2e): wrap the operational nav locator to prettier's width --------- Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
rubenvdlinde
added a commit
that referenced
this pull request
Sep 2, 2026
…a load (#1724) Both failed with a bare 'Test timeout of 60000ms exceeded' and no failed assertion, which reads as a hang rather than as work that no longer fits. Same shape as billing-categories in #1713 and pipelinq-pos-grouping in #1716. commercial-dashboard ran openApp() then goto('/operational') then reload(). Under hash routing only the first was a document load. Since the shell moved to createWebHistory all three boot the app, and openApp() lands on the Commercial dashboard, which is the one page this test does not want. Its comment claimed the deep link had to be a hash because a path-form goto boots the shell at the default dashboard — true before #1684, and now the opposite. appointment-booking called openApp() and then gotoHash() twice. gotoHash() already waits for #content-vue and clears both overlays, so openApp() was purely a third load onto a Dashboard the test never looks at. Neither raises a timeout: both stop paying for loads they do not need. Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
rubenvdlinde
pushed a commit
that referenced
this pull request
Sep 2, 2026
…eeds
Follow-up in the same PR. The nc-vue 2.30.0 bump fixed the defect these four
tests were reporting, and it changed their failure MODE rather than clearing
them:
before the bump Error: toHaveCount failed — cn-report-card, expected 4,
received 0
after the bump Test timeout of 60000ms exceeded, with NO assertion error
at all
That second shape is the tell. No assertion failed, so the cards render now;
the tests simply ran out of budget. Each one calls openApp() — which boots
the shell and dismisses the walkthrough and support dialogs — then does a
full navigation, and `type:"reports"` is mapped through
defineAsyncComponent, so the first of them also pays for fetching that chunk.
A bare timeout reads as "the page is broken" when it means "this test is too
slow", which is exactly how the openregister route sweep misreported itself
earlier today.
180s, with the reasoning written where the next reader will hit it.
Also corrected two comments that survived #1684 and now say the opposite of
what the code does: both told the reader that a path-form goto boots the
shell at the Dashboard and the route must travel in the hash. The shell has
been on createWebHistory(routerBase()) since #1684, and the code beneath
those comments already uses paths.
⚠️ Verified in CI rather than locally, deliberately. Every `page.goto` in
this spec exceeds even the old 30s budget on this machine — a page load
against the shared dev container takes ~13s — and a full local run of the
file was OOM-killed. The targeted check that mattered did pass here:
/apps/pipelinq/reports renders 4 cn-report-card elements with 2.30.0 built
from this branch.
rubenvdlinde
added a commit
that referenced
this pull request
Sep 2, 2026
… page (#1731) * chore(deps): bump @conduction/nextcloud-vue to 2.30.0 for the Reports page The four `tests/e2e/rapportage.spec.ts` failures on development are the last of that suite's red, and they are not a test problem: the Reports page has rendered EMPTY since #1691 shipped it. Root cause, in the library rather than here. `CnReportsPage` declared two props — `page` and `translate` — and read its cards from `this.page.config.cards`. `CnPageRenderer.resolvedProps()` returns `{ ...topLevel, ...normalizedConfig, ...params }`: it spreads the page's CONFIG KEYS as top-level props and never passes a `page` object. So `page` was always `{}`, `cards` was always `[]`, and the component rendered its empty state for every consumer. The contrast makes the contract obvious — CnIndexPage declares 96 props precisely because it reads the spread. Confirmed at runtime rather than inferred: the page MOUNTS (its `cn-reports-empty` marker is present) with zero `cn-report-card` elements, which is exactly the 0 CI reported against an expected 4. nextcloud-vue#920 fixed it by declaring `cards`, `categories`, `description`, `title` and the label props, with 82 lines of new unit tests, and 2.30.0 published it. This bump is all that is needed here. Verified against a build of this branch: /apps/pipelinq/reports renders 4 `cn-report-card` elements and the "Channel analytics" card is visible.⚠️ The rest of rapportage.spec.ts cannot be judged on this machine — every `page.goto` in it exceeds the spec's 30s budget because a page load takes ~13s against the shared dev container. In CI those same navigations complete and the failure was the card assertion, which is what this fixes. CI is the instrument for the remainder. eslint 0 errors, prettier --check on the FULL glob clean, 59 vitest tests pass.⚠️ Two corrections to earlier reports of mine, both from measuring the wrong world. The export-pages family was already fixed by #1706 and #1711 while I was working, so development's E2E is 4 failed / 328 passed rather than the 44 I quoted from a stale branch. And my first reading said the `reports` page type was unimplemented — that was a stale local node_modules (2.27.2 against a lockfile pinning 2.29.0) plus a bundle-override that did not match `?v=` query strings. Both instruments were wrong; the CI log settled it. * fix(e2e): give the rapportage specs the budget the Reports page now needs Follow-up in the same PR. The nc-vue 2.30.0 bump fixed the defect these four tests were reporting, and it changed their failure MODE rather than clearing them: before the bump Error: toHaveCount failed — cn-report-card, expected 4, received 0 after the bump Test timeout of 60000ms exceeded, with NO assertion error at all That second shape is the tell. No assertion failed, so the cards render now; the tests simply ran out of budget. Each one calls openApp() — which boots the shell and dismisses the walkthrough and support dialogs — then does a full navigation, and `type:"reports"` is mapped through defineAsyncComponent, so the first of them also pays for fetching that chunk. A bare timeout reads as "the page is broken" when it means "this test is too slow", which is exactly how the openregister route sweep misreported itself earlier today. 180s, with the reasoning written where the next reader will hit it. Also corrected two comments that survived #1684 and now say the opposite of what the code does: both told the reader that a path-form goto boots the shell at the Dashboard and the route must travel in the hash. The shell has been on createWebHistory(routerBase()) since #1684, and the code beneath those comments already uses paths.⚠️ Verified in CI rather than locally, deliberately. Every `page.goto` in this spec exceeds even the old 30s budget on this machine — a page load against the shared dev container takes ~13s — and a full local run of the file was OOM-killed. The targeted check that mattered did pass here: /apps/pipelinq/reports renders 4 cn-report-card elements with 2.30.0 built from this branch. --------- Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Sixth app off
#routing, after stackiq (#899), zaakafhandelapp (#609), opencatalogi (#1341), keepiq (#582) and larpinq (#651).Source: two lines and a helper
createWebHashHistory→createWebHistory.routerBase(), derived from the URL actually being served. Nextcloud serves the app under both/apps/pipelinq/...and/index.php/apps/pipelinq/...;generateUrl()returns only one. Arriving on the other leaves the path outside the router base and the catch-all redirects to/— no error, deep link silently swallowed. This suite pins the/index.phpform in several specs, so without it every deep link the suite makes would break.The test surface was the bulk, and not only URLs
${APP}/#/xand/apps/pipelinq/#/x→ real pathsnavigation.spec.tsREQUIRED_ENTRIESmapped page ids to hash hrefs and asserted equality → route paths compared as a suffixdashboard.spec.tsa[href^="#/"]and its/^#\//assertiontoHaveURL(/#\/…/)assertionshelpers/pipelinq.tsrevealNavEntry()selected leaves byhref*="#/"— which matches nothing once the shell leaves hash routingThe suffix comparison is deliberate: the base legitimately differs between the two URL forms, so pinning either one would fail on the other for a reason that is not a routing defect. Group captions keep their bare
href="#", so the app segment remains a clean discriminator between leaves and captions.Stale prose was corrected rather than deleted in three files — all told the reader that a path deep-link resets the SPA to the Dashboard, which was true and is now the opposite.
Verification
/apps/pipelinq//apps/pipelinq/leads/index.php/apps/pipelinq/leads/index.php/apps/pipelinq/clientsNo hash anywhere, and nav hrefs are real paths carrying whichever base the page was loaded under — exactly why the suffix assertion replaced the equality one.
e2e:
navigation.spec.ts+dashboard.spec.ts9/9,event-wiring.spec.ts3/3.eslint0,prettier --checkon the full glob clean, webpack compiles.