fix(shell): compose the runtime manifest instead of replacing the app's own - #559
Merged
rubenvdlinde merged 3 commits intoSep 5, 2026
Conversation
…'s own Reproduced on a clean instance rather than reasoned about, and the finding is worse than the 13 failing tests suggested: with tier 3, launchpad rendered NOTHING on a fresh install. `#workspace-vue` held 7 bytes, an empty comment, against 43,564 for the same instance on the commit before tier 3. `GET /api/manifest` is not this app's manifest. ManifestController says so in its own docblock: it assembles a document from the user's OpenRegister dashboard objects, one page and one menu entry per dashboard, and "when the user has no dashboards the manifest returns empty pages/menu". App.vue read `runtimeManifest?.value ?? this.manifest`. An empty object is truthy, so a user with no dashboards got a manifest with no pages and no menu, and tier 3 builds both the router and the nav from the manifest. A user WITH dashboards was no better off, only less obviously: the runtime document still replaced the nine declared pages, the ADR-114 footer and the walkthrough, which is what the chrome and routing specs have been failing on. So the two are composed, which is what each is for. The bundled manifest owns the declared surfaces; the runtime one contributes the dashboards only the server knows about, and the live `runtime` block. THE SETUP WIZARD CAME WITH CnAppRoot, and nothing accounted for it. CnAppRoot opens CnSetupWizard while ANY step is outstanding, optional or not (nextcloud-vue#806) — and `completed` is not `settled`: the status endpoint answered `"completed": true` while still reporting `demo-data` and `load-demo-data` as `done: false`. `cn-wizard-dialog` is an aria-modal that intercepts pointer events, so every click landed on it. Measured locally: with the wizard up, dashboard-sharing failed 4 of 4; with the choice recorded, runtime-shell-canEdit and add-widget-modal ran 10 passed, 0 failed. globalSetup now records the choice once, because the marker is per user and settling it per spec makes the suite order-dependent. Three manifest references could never resolve. The `dashboards` menu entry and two walkthrough steps pointed at `dashboard-detail`, whose path is `/dashboards/:id`, so vue-router threw `Missing required param "id"` on every render. They were inert until tier 3 gave the manifest a real router. A third step named the `Flows` PAGE where a menu-entry id belongs. Six labels were raw i18n keys. `launchpad.menu.dashboards` and five others render as the key itself in all 36 locales but Dutch, which is the only one carrying them. They are plain English now, like every other entry in this menu already was. Dutch loses a translation it only ever had for these six; that is the same position `Flows`, `Store` and `Reports` are already in, and worth a follow-up for all of them together rather than a special case for these. 682 unit tests pass. The development push run is the E2E proof, since that leg does not run on a pull request. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 20:09 UTC
Download the full PDF report from the workflow artifacts.
… already is gate-63 caught the label I chose. ADR-079 reserves `settings`, `appsettings` and `app settings` for a settings-foldout entry, because NcAppNavigation's foldout is ITSELF labelled "Settings" — an entry by that name renders as Settings > Settings, which is the duplication ADR-079 D1 removed a whole menu to stop. Replacing the raw `launchpad.menu.adminSettings` key with "Settings" walked straight into it. "Admin settings" says where the entry goes — it redirects to /settings/admin/launchpad — and is not reserved.⚠️ FOLLOW-UP, NOT SILENTLY FIXED HERE: this entry now carries the same label as the one nc-vue's shared chrome already renders, and both lead to the same admin page. ADR-079 D1's precedent says an in-app duplicate of the Nextcloud admin settings page should go, not be renamed. Removing a menu surface is a product decision rather than a gate fix, so it is named here instead of taken. Verified locally: gate-63 reports `checked 2 manifest(s): 0 failure(s)`, 682 unit tests pass, webpack build clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 20:18 UTC
Download the full PDF report from the workflow artifacts.
… English gate-102 caught what my last commit deferred, and it was right to: a manifest string with no `l10n/nl.json` key renders its English source to a Dutch user, and nothing else reports it. `check:l10n-js` compares nl.json to nl.js, and a string absent from BOTH is in sync.⚠️ CORRECTION TO THAT COMMIT MESSAGE. It said Dutch loses a translation it only ever had for these six labels. That was wrong, and the gate is what measured it: five of the six English strings I introduced — Dashboards, Templates, Settings, Dashboard — were ALREADY keys in nl.json, so replacing the dotted i18n keys cost Dutch nothing there. Only "Admin settings" was new, because only that label had no prior English form to have been catalogued under. So there is no follow-up owed for the other five, and the one that was genuinely missing is added rather than noted: "Beheerinstellingen", matching the file's own "Instellingen > Beheer" and "beheerder". Verified locally: gate-102 `checked 51 manifest string(s)` with no findings, gate-63 `0 failure(s)`, prettier clean, 682 unit tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rubenvdlinde
requested review from
Rem-Dam,
WilcoLouwerse,
bbrands02 and
rjzondervan
as code owners
September 5, 2026 20:25
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 20:28 UTC
Download the full PDF report from the workflow artifacts.
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.
Reproduced, not reasoned about
I stood up a clean Nextcloud with launchpad and openregister and compared the commit before tier 3 (#551's parent) against
development, on the same instance. The finding is worse than the 13 failing tests suggested:#workspace-vuecontents5aacaf1)developmentwith tier 3With tier 3, launchpad rendered nothing at all on a fresh install.
The cause
GET /api/manifestis not this app's manifest.ManifestControllersays so in its own docblock: it assembles a document from the user's OpenRegister dashboard objects, one page and one menu entry per dashboard, and "when the user has no dashboards the manifest returns empty pages/menu".App.vuereadruntimeManifest?.value ?? this.manifest. An empty object is truthy, so a user with no dashboards got a manifest with no pages and no menu — and tier 3 builds both the router and the nav from the manifest.A user with dashboards was no better off, only less obviously: the runtime document still replaced the nine declared pages, the ADR-114 footer and the walkthrough. That is what the chrome and routing specs have been failing on.
The two are now composed, which is what each is for. The bundled manifest owns the declared surfaces; the runtime one contributes the dashboards only the server knows about.
The setup wizard came with CnAppRoot
CnAppRootopensCnSetupWizardwhile any step is outstanding, optional or not (nextcloud-vue#806). This app had no wizard at all until tier 3 rooted it onCnAppRoot, so nothing in the e2e accounted for one.completedis notsettled.GET /api/setup/statusanswered"completed": truewhile still reportingdemo-dataandload-demo-dataasdone: false.cn-wizard-dialogis anaria-modalthat intercepts pointer events. Measured locally:dashboard-sharing4 of 4 failed,cn-wizard-dialognamed as the interceptorruntime-shell-canEdit+add-widget-modal10 passed, 0 failed — both of which fail on CIglobalSetuprecords the choice once. Per-spec dismissal would make the suite order-dependent, because the marker is per user.Three manifest references could never resolve
The
dashboardsmenu entry and two walkthrough steps pointed atdashboard-detail, whose path is/dashboards/:id— so vue-router threwMissing required param "id"on every render. They were inert until tier 3 gave the manifest a real router. A third step named theFlowspage where a menu-entry id belongs.Six labels were raw i18n keys
launchpad.menu.dashboardsand five others render as the key itself in all 36 locales but Dutch, the only one carrying them. They are plain English now, like every other entry in this menu already was (Flows,Store,Reports).Dutch loses a translation it only ever had for these six. That is the same position its siblings are already in, and worth one follow-up for all of them rather than a special case here.
Verification
682 unit tests pass, eslint and prettier clean, webpack build clean, and the render is measured above. The
developmentpush run is the E2E proof, since that leg does not run on a pull request.🤖 Generated with Claude Code