fix(store): drop the hand-rolled object store; route settings through cnFetchJson - #179
Merged
rubenvdlinde merged 3 commits intoAug 26, 2026
Merged
Conversation
… cnFetchJson
The template shipped TWO object stores. `src/store/store.js` already created
the canonical one with `createObjectStore` from @conduction/nextcloud-vue,
while `src/store/modules/object.js` defined a second `useObjectStore` as a
hand-rolled `defineStore('object')` doing raw `fetch()` with a hand-set
`requesttoken`.
Nothing imported the second one — but every app scaffolded from this template
inherited it, which is why ADR-071 finds app-local `store/modules/object.js`
variants in six apps and parallel generic stores in two more. The template was
the source that kept re-seeding the pattern ADR-071 Decisions 1 and 2 delete.
Changes:
- Delete src/store/modules/object.js (dead, and the anti-pattern's seed).
- settings.js now uses cnFetchJson instead of raw fetch + getRequestToken
(ADR-071 Decision 1). URLs still go through generateUrl, because cnFetch's
own prefixUrl only adds /index.php and does not know the instance webroot —
handing it an already-generated URL is a no-op there, so this keeps the
correct URL AND the library's blessed headers.
- settings.js gains an `error` state. Both actions previously returned null on
failure, and null was also the legitimate "nothing yet" value, so a settings
endpoint returning 500 was indistinguishable from a fresh install. Failures
are now logged AND observable; the actions still resolve rather than throw,
so a broken endpoint cannot stop the SPA mounting.
- Bump @conduction/nextcloud-vue ^2.3.0 -> ^2.16.0. REQUIRED, not cosmetic:
cnFetch/cnFetchJson first shipped in v2.11.0, and the installed tree was on
2.2.0, so the import would not have resolved. ^2.16.0 matches openregister
and decidesk; the lock resolves 2.17.0.
The spec is corrected in the same commit, because it was wrong in two ways:
- It REQUIRED the deleted module by path, and pinned `configure()` /
`registerObjectType()` / `fetchObjects()` — an API the dead module had and
the library store does not. The spec described the module nothing imported
while the code used the other one.
- Its "degrade gracefully" clause REQUIRED failures to resolve to a safe
empty/null value and said nothing about surfacing them, writing the
unobservable-failure pattern into a normative requirement. It now requires
the safe value AND a recorded error.
Verified: npm run build compiles (webpack 5.109.2, 2 pre-existing warnings).
Refs ADR-071 Decisions 1-2, ADR-026, ADR-100 Decision 5.
rubenvdlinde
requested review from
Rem-Dam,
SudoThijn and
remko48
as code owners
August 25, 2026 21:38
Contributor
Quality Report — ConductionNL/nextcloud-app-template @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ❌ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| check-manifest | ✅ | ||||
| format | ✅ | ||||
| composer | ✅ | ✅ 103/103 | |||
| npm | ✅ | ✅ 737/737 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-25 22:29 UTC
Download the full PDF report from the workflow artifacts.
…c warnings gate-47 (security-change-has-tests) flagged the previous commit: it changed CSRF/transport handling with no test co-change. It was right to. tests/settings-store.spec.js covers both halves of the fix, because either one alone is the bug: * resolving to null WITHOUT recording -> the original silent failure * recording but THROWING out of the action -> a broken endpoint blocks boot Arm 4 is the one that would have caught the original defect. It asserts that 'the backend had nothing' and 'the backend failed' are DISTINGUISHABLE, rather than merely that failure returns null — a test asserting only '=== null' passes against the broken version unchanged. VERIFIED AGAINST THE OLD BEHAVIOUR, not just the new one: re-running this exact spec against a reconstructed silent-failure store fails 4 of 6 arms, with arm 4 reporting 'indistinguishable'. A test that only passes on the fixed code cannot be shown to detect the defect it was written for. Wired into check:specs alongside the sibling node specs, so it runs in CI rather than only by hand. Also fixes the two eslint warnings the previous commit introduced (the repo runs --max-warnings 0): a jsdoc block opening with text on the /** line, and a getter missing its @PARAM. The spec harness reimplements the action bodies rather than importing pinia, matching registry.spec.js / manifest-v2.spec.js which run under bare node. That limit is stated in the file: it pins the CONTRACT, not the module wiring, and the wiring is covered by the build plus the e2e app-shell spec.
The repo runs `prettier --check` over **/*.js and eslint with --max-warnings 0, and the new spec satisfied neither. Formatting applied with prettier --write; the two top-level arrow consts (`bad`, `rejecting`) become function declarations, which is what antfu/top-level-function requires. No behaviour change — the spec still passes all six arms, and still fails four of them against a reconstructed silent-failure store, which is the property that makes it worth having.
Contributor
Quality Report — ConductionNL/nextcloud-app-template @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ❌ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| check-manifest | ✅ | ||||
| format | ❌ | ||||
| composer | ✅ | ✅ 103/103 | |||
| npm | ✅ | ✅ 737/737 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ❌ | ||||
| Newman | ❌ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-26 09:26 UTC
Download the full PDF report from the workflow artifacts.
Contributor
Quality Report — ConductionNL/nextcloud-app-template @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| check-manifest | ✅ | ||||
| format | ✅ | ||||
| composer | ✅ | ✅ 103/103 | |||
| npm | ✅ | ✅ 737/737 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-26 10:50 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
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.
The template shipped two object stores.
src/store/store.jsalready created the canonical one withcreateObjectStorefrom@conduction/nextcloud-vue. Alongside it,src/store/modules/object.jsdefined a seconduseObjectStoreas a hand-rolleddefineStore('object')doing rawfetch()with a hand-setrequesttoken.Nothing imported the second one — but every app scaffolded from this template inherited it. That is why ADR-071 finds app-local
store/modules/object.jsvariants in six apps and parallel generic stores in two more: the template is the source that kept re-seeding the exact pattern ADR-071 Decisions 1 and 2 delete. ADR-100 Decision 5 makes fixing it the precondition for any leaf migration.Changes
src/store/modules/object.js— dead, and the anti-pattern's seed.settings.js→cnFetchJson(ADR-071 Decision 1). URLs still go throughgenerateUrl:cnFetch's ownprefixUrlonly adds/index.phpand does not know the instance webroot, so an install underhttps://host/nextcloudwould break. HandingprefixUrlan already-generated URL is a no-op, so this keeps the correct URL and the library's blessed headers.settings.jsgains anerrorstate. Both actions previously returnednullon failure — andnullwas also the legitimate "nothing yet" value, so a settings endpoint returning 500 was indistinguishable from a fresh install. Failures are now logged and observable, while the actions still resolve rather than throw, so a broken endpoint cannot stop the SPA mounting.@conduction/nextcloud-vue^2.3.0→^2.16.0. Required, not cosmetic:cnFetch/cnFetchJsonfirst shipped in v2.11.0 and the installed tree was on 2.2.0, so the import would not have resolved.^2.16.0matches openregister and decidesk; the lock resolves 2.17.0.The spec was wrong in two ways, and is corrected here
configure()/registerObjectType()/fetchObjects()— an API the dead module had and the library store does not. The spec described the module nothing imported while the code used the other one.nullvalue and said nothing about surfacing them — writing the unobservable-failure pattern into a normative requirement. It now requires the safe value and a recorded error.Verification
npm run buildcompiles — webpack 5.109.2, 2 pre-existing warnings. Local eslint could not run (the repo's flat config fails against the installed@nextcloud/eslint-config, pre-existing and unrelated); CI will lint.Refs ADR-071 Decisions 1–2, ADR-026, ADR-100 Decision 5.