feat(routing): serve the pages the manifest declares - #551
Merged
Conversation
LaunchPad declared nine pages and served ONE. /store, /reports and /flows each redirected to /dashboard, and /reports/dashboards rendered the workspace grid — because there was no vue-router at all. `createRouter` appeared nowhere in src/: navigation was Pinia state that never touched the URL, and this was the only app in the fleet that did not root on CnAppRoot. main.js has named this change since Tier 1 adoption landed. Its own file header already described the intent — "App.vue watches it and passes the live value down to CnAppRoot" — and the implementation stopped short. WHAT CHANGED App.vue roots on CnAppRoot. The route table is derived from manifest.pages, so a declared page is a routed page by construction. `/` and `/dashboards/:id` render WorkspaceApp; the rest render their declared page types. The org navigation rail stays exactly where it is. It is NOT an app menu — it renders an org-wide tree from GET /api/admin/org-navigation, group-filtered, with its own position setting — so it coexists with CnAppNav rather than folding into it, and it renders only when an admin has configured a tree. The bespoke skip link is gone. It existed BECAUSE this app did not root on NcContent; CnAppRoot renders it, and two bypass links are worse than one. #launchpad-main-content keeps tabindex="-1" for the quick-search Esc contract, which needs it independently. A dashboard has an address now. /dashboards/:id was declared all along and routed nowhere; WorkspaceApp watches the param and switches. An unknown id is left alone deliberately — the store already resolves a sensible active dashboard, and overriding would make a bad link empty the page instead of falling back. TWO PAGES WERE NOT PAGES admin-settings named AdminSettingsPage, which has never existed, and admin-templates-index named TemplatesPage, which is a TAB inside the Nextcloud admin section. Routing them would have rendered half an admin surface beside the real one. Both resolve to AdminSettingsRedirect, which sends the operator to /settings/admin/launchpad and renders a real anchor as well as navigating. TESTS The four test.fail() markers come off — this is the change that earns them. The tripwire is INVERTED rather than deleted: it asserted the absence of CnAppNav, and now asserts the shared chrome renders with the workspace still inside it. A new test asserts a dashboard has an address, asking the app which dashboards exist rather than seeding an id. Verified: check:manifest, format, check:schema-l10n and lint pass; webpack compiles (exit 0, the 3 warnings are the pre-existing bundle-size ones); vitest WorkspaceApp 14 passed; playwright --list collects 9. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rubenvdlinde
requested review from
Rem-Dam,
SudoThijn and
remko48
as code owners
September 5, 2026 16:08
Contributor
Quality Report — ConductionNL/launchpad @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| format | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| composer | ✅ | ✅ 104/104 | |||
| npm | ✅ | ✅ 527/527 | |||
| 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-05 16:11 UTC
Download the full PDF report from the workflow artifacts.
Three findings, all real. 🔴 REQ-SHELL-001 SUPPRESSED THE NAVIGATION SLOT. PageController passed `id-app-navigation => null` so Nextcloud allocated no left panel — the app rendered its own slide-in sidebar and nothing else. CnAppRoot renders NcContent, and NcContent allocates that panel for CnAppNav, so leaving the suppression in place would have left the shared chrome with nowhere to render: an empty rail, not an error. Every other app in the fleet passes no slot ids at all. Found by reading the spec this change contradicts, not by CI. gate-16 wanted @SPEC on three changed methods. The change had no spec to point at, so one is written — `specs/manifest-routing/spec.md`, six requirements including REQ-ROUTE-004, which records that it supersedes REQ-SHELL-001's chrome-slot clause. Every anchor was checked to resolve; a tag that merely exists is not evidence. gate-26 wanted a visual baseline for two new page components. AdminSettingsRedirect gets a real test: both admin routes are opened and must either reach /settings/admin/launchpad or render the notice, since the redirect deliberately does both. WorkspaceApp gets `@visual exclude` with the reason — it is not a new screen, it is this app's oldest and only one, and a baseline captured now would be a baseline of the screen that already shipped. Verified: all 78 applicable hydra gates pass locally against origin/development; check:manifest, format and lint green; phpcs clean on PageController; playwright --list collects 10. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rubenvdlinde
requested review from
WilcoLouwerse,
bbrands02 and
rjzondervan
as code owners
September 5, 2026 16:24
Contributor
Quality Report — ConductionNL/launchpad @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| format | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| composer | ✅ | ✅ 104/104 | |||
| npm | ✅ | ✅ 527/527 | |||
| 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-05 16:27 UTC
Download the full PDF report from the workflow artifacts.
This was referenced Sep 5, 2026
rubenvdlinde
added a commit
that referenced
this pull request
Sep 5, 2026
…t behind it (#555) #551 adopted the shared shell, and the dashboard switcher has been unreachable ever since. 58 e2e tests across eleven specs fail as `locator.click` timing out, and all 240 interception lines in that run name the same element: `[data-testid="cn-nav"] .app-navigation`. The panel slides out, it is visible, and every click on it lands on the navigation instead. This is a stacking context, not a z-index. The sidebar is `position: fixed`, which escapes the scroll flow but not the stacking context. Until #551 this view WAS the page, so there was no context to escape and nothing else claimed the left 280px. Now it renders inside `#app-content`, a sibling of `.app-navigation`, and NcAppNavigation carries `z-index: 1400` on itself. A sibling with a z-index paints above the whole of a sibling subtree whose own z-index is auto, so the sidebar's 1500 could never win: it was competing inside a box that had already lost. Raising the number again would have changed nothing, which is worth saying because that is the obvious first move. The sidebar and its backdrop are therefore teleported to `body`, making them siblings of the navigation rather than descendants of the content, which is the only place their z-index means what it says. The backdrop goes from 999 to 1450 while it is there. At 999 the navigation painted over it even once teleported, so a click meant to dismiss the sidebar navigated instead. 1450 puts it above the navigation and below the sidebar it sits behind. Verified locally: 682 unit tests pass, webpack build clean, eslint and prettier clean. Co-authored-by: Conduction Release Bot <release-bot@conduction.nl> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
rubenvdlinde
added a commit
that referenced
this pull request
Sep 5, 2026
…ve (#557) * fix(shell): the dashboard switcher opens above the app navigation, not behind it #551 adopted the shared shell, and the dashboard switcher has been unreachable ever since. 58 e2e tests across eleven specs fail as `locator.click` timing out, and all 240 interception lines in that run name the same element: `[data-testid="cn-nav"] .app-navigation`. The panel slides out, it is visible, and every click on it lands on the navigation instead. This is a stacking context, not a z-index. The sidebar is `position: fixed`, which escapes the scroll flow but not the stacking context. Until #551 this view WAS the page, so there was no context to escape and nothing else claimed the left 280px. Now it renders inside `#app-content`, a sibling of `.app-navigation`, and NcAppNavigation carries `z-index: 1400` on itself. A sibling with a z-index paints above the whole of a sibling subtree whose own z-index is auto, so the sidebar's 1500 could never win: it was competing inside a box that had already lost. Raising the number again would have changed nothing, which is worth saying because that is the obvious first move. The sidebar and its backdrop are therefore teleported to `body`, making them siblings of the navigation rather than descendants of the content, which is the only place their z-index means what it says. The backdrop goes from 999 to 1450 while it is there. At 999 the navigation painted over it even once teleported, so a click meant to dismiss the sidebar navigated instead. 1450 puts it above the navigation and below the sidebar it sits behind. Verified locally: 682 unit tests pass, webpack build clean, eslint and prettier clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(shell): the floating controls move with the backdrop they sit above #555 teleported the sidebar and its backdrop to `body` and raised the backdrop to 1450. It should have moved the floating controls at the same time, and the e2e said so within one run. Those controls are `position: fixed` with `z-index: 1000`, and that number was not arbitrary: it sat just above the backdrop's original 999 so that "click the hamburger again to close the sidebar" works — the sidebar is open, the backdrop is up, and the toggle has to stay reachable through it. Teleporting one half of that pairing broke it. The backdrop became a body-level sibling while the controls stayed inside `#app-content`, whose own z-index is auto, so the backdrop painted over them whatever number they carried. `runtime-shell-canEdit` names it exactly: the backdrop intercepting pointer events over a button the same log calls visible, enabled and stable. So both halves move, and the controls go to 1460 — above the backdrop, below the sidebar they never overlap. The comment says which number it is above and why the Teleport is what makes it mean anything, because the next person to see 1460 will otherwise try raising it. Verified locally: 682 unit tests pass, webpack build clean, eslint and prettier clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Conduction Release Bot <release-bot@conduction.nl> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
LaunchPad declared nine pages and served one.
/store,/reportsand/flowseach redirected to/dashboard, and/reports/dashboardsrendered the workspace grid — because there was no vue-router at all.createRouterappeared nowhere insrc/: navigation was Pinia state that never touched the URL, and this was the only app in the fleet that did not root onCnAppRoot.main.jshas named this change since Tier 1 adoption landed, and its own file header already described the intent — "App.vue watches it and passes the live value down to CnAppRoot" — while the implementation stopped short. #549 filed the gap; this closes it.The shell
App.vueroots onCnAppRoot. The route table is derived frommanifest.pages, so a declared page is a routed page by construction./and/dashboards/:idrenderWorkspaceApp; the rest render their declared page types.The org navigation rail stays exactly where it is. It is not an app menu — it renders an org-wide tree from
GET /api/admin/org-navigation, group-filtered, with its own position setting (REQ-ONAV-002/004/005). It coexists withCnAppNavrather than folding into it, andshouldRenderis false on an empty tree, so a default instance has one navigation and not two.The bespoke skip link is gone. It existed because this app did not root on
NcContent;CnAppRootrenders it, and two bypass links are worse than one.#launchpad-main-contentkeepstabindex="-1", which the quick-search Esc contract needs independently of any link.A dashboard has an address
/dashboards/:idwas declared in the manifest all along and routed nowhere.WorkspaceAppwatches the route param —immediate, so a cold deep link selects on arrival — and callsswitchDashboard. An absent or unknown id is left alone deliberately: the store's resolver already picks a sensible active dashboard, and overriding it would make a bad link empty the page instead of falling back.Two of the nine were not pages at all
admin-settingsnamed a component,AdminSettingsPage, that has never existed.admin-templates-indexnamedTemplatesPage, which is a tab inside the Nextcloud admin section (lib/Settings/LaunchPadAdmin.php, mounted bysrc/admin.js). Routing them would have rendered half an admin surface beside the real one.Both resolve to
AdminSettingsRedirect, which sends the operator to/settings/admin/launchpad. It renders a real anchor as well as navigating, because a redirect that only runs inmountedleaves a blank page for anyone whose navigation is slow or blocked.Tests
The four
test.fail()markers come off — this is the change that earns them. Playwright fails a run when atest.fail()test passes, which is exactly the notification that was wanted.The tripwire is inverted, not deleted. It asserted the absence of
CnAppNavand existed for this moment; it now asserts the shared chrome renders with the workspace still inside it, because a shell that rendered without the grid would be a regression the nav assertion alone would not catch.A new test asserts a dashboard has an address. It asks the app which dashboards the user has rather than seeding an id, since a hardcoded id passes on the instance it was written against and nowhere else.
Verified
check:manifest,format,check:schema-l10n,lintnpm run buildvitestWorkspaceAppplaywright test --listThe E2E leg runs only on the
developmentpush, so that run is where routing is actually proven. Two things are deliberately left for later and written intotasks.md: the org rail renders only insideWorkspaceApp, so it appears on the dashboard routes and not the others (which matches today's behaviour exactly, since there were no other routes, but is now a visible inconsistency); andDashboardsReporthas never rendered, so whether its widgets resolve is unproven until that run.🤖 Generated with Claude Code