fix(admin): server state via IInitialState, not data-* attributes — and the spec that mandated the wrong one - #252
Merged
Conversation
added 2 commits
August 9, 2026 15:47
…rated files tracked under their own ignore rule Three mechanical gate failures, each with a real defect behind it. gate-43 table-headers — the theming audit log renders a four-column `<table>` whose `<th>`s carried no `scope=`. With no scope, a screen reader cannot announce which header belongs to a cell, so the log — the artefact this app offers as WCAG-EM audit evidence — was itself not readable by the users it exists to serve. `scope="col"` on all four. gate-44 autocomplete-attr — the three email-footer fields declared no autocomplete purpose. They are INSTANCE-WIDE configuration (the organisation named in every outgoing mail, and that organisation's public statement URLs), not personal details of the admin filling the form, so `autocomplete="off"` is the accurate declaration and not a silencer: it stops a browser offering the admin's own profile data as the value for a setting that applies to everyone on the instance. Worth recording for whoever meets this gate next: it matched these three only because the substring `email` appears in their ids (`nldesign-email-footer-org-name`), and not one of them collects an email address. The attribute is right on its own merits; the match that found it was not. gate-29 gitignore-then-commit — `.gitignore:61` has ignored `/phpmetrics/` for as long as the rule has existed, while 81 generated report files sat tracked underneath it. `composer phpmetrics` regenerates the directory and CI sets `enable-phpmetrics: true`, so every run could rewrite files git was still watching. Nothing links to the committed HTML; the rule was right and the tracking was the oversight. Untracked, not re-ignored. Measured with hydra-gates 48c88ba1e0d049f8f38538c33e790d3e603c55d0: gate-43 FAIL 1 -> PASS, gate-44 FAIL 3 -> PASS. Both proven able to fail on this tree first — a planted unscoped `<th>` and a planted unlabelled input both fired in a control run before these fixes.
…nd the spec that mandated the wrong one
gate-10 initial-state: FAIL 4 -> PASS.
js/admin.js pulled four server-rendered values off the DOM with
`getAttribute('data-…')`: `tokenSets`, `currentTokenSet`,
`activePreview` and `iconPackSource`. ADR-004 rules that out —
`IInitialState::provideInitialState()` in PHP, `loadState()` in JS —
and the reason is not style. A data attribute breaks on CSP-hardened
instances, and it breaks SILENTLY: any markup change that moves or
renames the carrying element leaves admin.js parsing an empty string
and rendering as though the server had sent nothing at all. The app
already did this correctly one file over, in js/preview-banner.js.
THE SPEC MANDATED THE DEFECT
----------------------------
`openspec/specs/admin-settings/spec.md` carried a requirement titled
"Data Attributes for JavaScript Initialization" whose three scenarios
required exactly what gate-10 forbids, down to
`p(json_encode(...))` on `data-token-sets`. Fixing only the code would
have left the canonical spec instructing the next person to put it
back. The requirement is rewritten to mandate initial state, with the
contradiction and its resolution recorded in place rather than
silently overwritten.
WHILE IN THE FILE
-----------------
`notify()` fell back to `notify(message)` instead of
`OC.Notification.showTemporary(message)` — an unconditional infinite
recursion on any instance where `OCP.Toast` is absent and the NC<=33
API is present. The RangeError was swallowed by the surrounding
try/catch, so every toast on that path silently became a console line.
EVIDENCE
--------
- hydra-gates 48c88ba1e0d049f8f38538c33e790d3e603c55d0: gate-10
FAIL 4 -> PASS; gate-19 unchanged at 156, so rewriting the spec
added no e2e debt (three scenarios renamed, one added, all covered
or reason-excluded).
- vitest 81/81, having been 81/81 before the change and 75/81 midway:
six tests in admin-a11y and admin-dark-mode failed the moment
admin.js stopped reading attributes, which is the positive control
that the new read path is the one under test. All five DOM fixtures
now provide initial state the way the settings page does.
- The two e2e tests that asserted the ATTRIBUTE now assert the
initial-state element AND that the attribute is gone — on the item,
not on the panel rendering, because the panel renders correctly
with both mechanisms present and that is the state being retired.
- PHP 8.4: phpcs clean, phpmd clean, phpstan "No errors", psalm "No
errors found". (The host runs 8.2 and this repo's floor is 8.3, so
the analysers cannot run on the host at all — they were run in a
php:8.4-cli container against this worktree's own vendor/.)
Contributor
Quality Report — ConductionNL/nldesign @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| test-l10n | ✅ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ✅ | ✅ 2/2 | |||
| PHPUnit | ❌ | ||||
| Newman | ✅ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-09 14:29 UTC
Download the full PDF report from the workflow artifacts.
… owns
The coverage ratchet failed this branch: the four
`provideInitialState()` calls and the constructor assignment added 5
statements with no test behind them, and `Settings\Admin::getForm()`
had never been unit-tested at all.
The test asserts on the KEYS AND VALUES handed to `IInitialState`, not
on the TemplateResponse parameters. The template still receives these
values for the parts it renders server-side, so asserting there would
pass even if every `provideInitialState()` call were deleted.
Four cases:
- all four keys js/admin.js reads are published, in order, with the
values the script expects
- an active preview resolves the token set's DISPLAY NAME from the
inventory, which is what the banner renders
- `iconPackSource` reports `override` when the appconfig override
alone decides the pack — the value that stops admin.js
re-deriving the indicator from the dropdown
- the template ships none of the four retired `data-*` transports
Proven able to fail: commenting out the `iconPackSource` call alone
takes this suite to 2 failures. Without that control the test would
be indistinguishable from one that asserts nothing — a missing
initial-state key does not crash anything, it just makes the script
fall back to a default and render a plausible, wrong panel.
phpunit 4/4, 13 assertions, run under PHP 8.4. tests/ is outside
phpcs.xml's `<file>lib</file>` scope, so the file is styled to match
the existing suite rather than to lib/'s stricter sniffs.
Contributor
Quality Report — ConductionNL/nldesign @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| test-l10n | ✅ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ✅ | ✅ 2/2 | |||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-09 14:39 UTC
Download the full PDF report from the workflow artifacts.
…malise in JS
Caught by the e2e test added in this same branch, which is the reason
it was written to assert on the initial-state ELEMENT rather than on
the panel rendering: `#initial-state-nldesign-activePreview` was never
attached, while the sibling `tokenSets` and `currentTokenSet`
assertions passed.
The cause is in Nextcloud, not here:
public function provideInitialState(string $appName, string $key, $data): void {
if (is_scalar($data) || $data instanceof \JsonSerializable || is_array($data)) {
...
return;
}
$this->logger->warning('Invalid ' . $key . ' data provided ...');
}
NULL IS NONE OF THOSE THREE. It does not throw — it logs a warning and
provides nothing. So `provideInitialState('activePreview', null)` was
a no-op: the key was absent, `loadState` returned its fallback, and
because that fallback is ALSO null the panel rendered correctly while
the server wrote `Invalid activePreview data provided to
provideInitialState by nldesign` on every admin settings page load.
A working panel and a broken contract, distinguishable only from the
Nextcloud log.
Two changes:
- PHP sends `$activePreview ?? []` — the same "no preview" fact in a
shape the service actually carries.
- JS normalises absent / empty / token-set-less to null, because
`[]` IS TRUTHY in JavaScript and the existing `activePreview !==
null` guard would otherwise have treated an empty array as a live
preview and read `.tokenSet` off it as undefined.
phpunit 4/4, phpstan "No errors", phpcs clean, vitest 81/81.
Contributor
Quality Report — ConductionNL/nldesign @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| test-l10n | ✅ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ✅ | ✅ 2/2 | |||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-09 14:57 UTC
Download the full PDF report from the workflow artifacts.
This was referenced Aug 9, 2026
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.
gate-10 initial-state: FAIL 4 → PASS. Measured against a fresh clone of
ConductionNL/.githubatorigin/main—[hydra-gates] gate package: 48c88ba1e0d049f8f38538c33e790d3e603c55d0(the same package CI resolves).The defect
js/admin.jspulled four server-rendered values off the DOM withgetAttribute('data-…'):tokenSets,currentTokenSet,activePreviewandiconPackSource. ADR-004 rules that out —IInitialState::provideInitialState()in PHP,loadState()in JS — and the reason is not style. A data attribute breaks on CSP-hardened instances, and it breaks silently: any markup change that moves or renames the carrying element leavesadmin.jsparsing an empty string and rendering as though the server had sent nothing at all. The app already did this correctly one file over, injs/preview-banner.js.The spec mandated the defect
openspec/specs/admin-settings/spec.mdcarried a requirement titled "Data Attributes for JavaScript Initialization" whose three scenarios required exactly what gate-10 forbids, down top(json_encode(...))ondata-token-sets.Fixing only the code would have left the canonical spec instructing the next person to put it back. The requirement is rewritten to mandate initial state, with the contradiction and its resolution recorded in place rather than silently overwritten. gate-19 is unchanged at 156 across the rewrite — three scenarios renamed, one added, all covered or reason-excluded — so no e2e debt was created by touching the spec.
While in the file
notify()fell back tonotify(message)instead ofOC.Notification.showTemporary(message)— an unconditional infinite recursion on any instance whereOCP.Toastis absent and the NC≤33 API is present. TheRangeErrorwas swallowed by the surroundingtry/catch, so every toast on that path silently became a console line.Evidence
The 75/81 matters: six tests in
admin-a11yandadmin-dark-modefailed the momentadmin.jsstopped reading attributes. That is the positive control that the new read path is the one under test, not an inert addition. All five DOM fixtures now provide initial state the way the settings page does.The two e2e tests that asserted the attribute now assert the initial-state element and that the attribute is gone — on the item, not on the panel rendering, because the panel renders correctly with both mechanisms present and that is precisely the state being retired.
No waivers, no baselines, no threshold changes.