fix(apps): render array settings so Menu Board installs with its menu - #3276
Conversation
The Add -> Apps config form skipped any setting whose JSON Schema type
was `array`: widgetFor() mapped it to 'unsupported' and renderField()
dropped the field. Menu Board keeps its entire content in one array
(`item`, exploded by `{?...,item*,...}`), so the operator got Board
title / Subtitle / Currency / Footer note and no way to enter a single
item. The launch URL carried the scalars and not one `item=` param, and
because the app only falls back to its worked example when the query
string is empty, filling in any other field produced a board with a
title and nothing on it. World Clock's `tz` was affected the same way.
Port the app store's repeated-group widget: rows of the item schema's
sub-fields, each composed into one token via its `x-format`. The
composer is a faithful port of the store's lib/item-format.js so the
same rows build the same URL in both places.
Edit mode has no counterpart in the store, which only ever composes a
fresh link. Since `x-format` drops a blank field together with its
separator, a short token is genuinely ambiguous, so parseItemToken
keeps the leftmost fields and never drops one the schema marks
`required`. Parts land on a contiguous run of fields, so re-composing
an untouched row reproduces the token exactly and reopening an
installed app never rewrites its launch URL.
The remove button states its own colours: .app-btn-icon reads
--surface-text-muted, which only exists inside a .surface context, and
the modal body is not one.
Tests cover the codec, the round trip over every shape Menu Board
documents, and the widget itself. The DOM suite needs a document, so
happy-dom is added as a dev dependency and preloaded via bunfig.toml.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3276 +/- ##
=========================================
Coverage ? 90.65%
=========================================
Files ? 76
Lines ? 8467
Branches ? 898
=========================================
Hits ? 7676
Misses ? 570
Partials ? 221 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes Add → Apps manifest-driven configuration so apps with array-type settings (notably Menu Board’s item[] and World Clock’s tz[]) render an editable repeated-row control instead of silently skipping the setting, ensuring generated launch URLs include the repeated query params those apps require.
Changes:
- Add an
arraywidget type and render it as a repeated group of sub-field inputs composed into tokens via the item schema’sx-format. - Introduce a shared token codec (
applyItemFormat/parseItemToken) and add comprehensive unit + DOM tests to pin URL round-tripping and UI behavior. - Add
happy-domtest preloading for Bun so DOM-dependent modules can be tested in CI.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/anthias_server/app/static/src/test-setup.ts | Preloads a DOM environment for Bun tests via happy-dom. |
| src/anthias_server/app/static/src/apps/widget-for.ts | Maps JSON Schema type: 'array' to a supported widget ('array'). |
| src/anthias_server/app/static/src/apps/widget-for.test.ts | Updates/extends tests to assert arrays are rendered (not skipped). |
| src/anthias_server/app/static/src/apps/types.ts | Adds maxItems to the typed subset of the manifest schema. |
| src/anthias_server/app/static/src/apps/manifest-form.ts | Implements the repeated-group array widget and hooks it into renderField(). |
| src/anthias_server/app/static/src/apps/manifest-form.test.ts | DOM-level regression tests for array widget rendering/editing/reopen behavior. |
| src/anthias_server/app/static/src/apps/item-format.ts | Adds token compose/parse utilities for x-format-based array items. |
| src/anthias_server/app/static/src/apps/item-format.test.ts | Unit tests for token codec and launch URL expansion behavior. |
| src/anthias_server/app/static/sass/_styles.scss | Styling for repeated rows, add/remove controls, and empty-state hint. |
| package.json | Adds happy-dom and global registrator as dev dependencies. |
| bunfig.toml | Preloads the DOM setup module for Bun tests. |
| bun.lock | Locks new dev dependency graph (happy-dom and transitive updates). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Glassto
left a comment
Glassto
left a comment
There was a problem hiding this comment.
Review: fix(apps)
Solid fix for a real bug, and a good root-cause writeup. The one-line widget-for.ts change reverting to break 17/18 new DOM tests is a good sign that the test suite actually exercises the fix, not just its presence.
A few things I'd like resolved or clarified before approving:
-
parseItemTokenambiguity handling. The description acknowledges thatx-formatdrops a blank field and its separator, which makes some tokens genuinely ambiguous (Coffee|Espressovs.Cortado|3.10) . The "keep leftmost fields, never drop a required one" heuristic sounds reasonable, but I'd like the round-trip test to explicitly cover: a blank field in the middle of a row, multiple consecutive blanks, and a single|with everything else empty , not just "every shape Menu Board documents". Can you confirm these are covered, or add them? -
Separator escaping. Agreed that not emitting
\|here (to stay compatible with the store) is the right call for this PR. Can we open a tracking issue so it doesn't get lost? -
x-formatdropping blank-field separators. Same ask, a linked issue so this stays visible as tech debt rather than becoming permanent by omission. -
SonarQube flagged 3 new issues. Could you share what they are or link the SonarCloud report? Want to confirm none touch the parsing logic from point 1.
-
Nit: worth double-checking the
'array'mapping inwidget-for.tsdoesn't accidentally catch other array-typed schema shapes (e.g.oneOf/anyOf) elsewhere in the manifest set, beyond Menu Board/World Clock.
no notes at the_styles.scss, the happy-dom dev dependency, and the bunfig.toml preload: all look clean and low-risk.
Nothing here blocks merge on its own; mainly want (1) and (4) addressed before approving, with (2)/(3) tracked as follow-ups.
Copilot: sync() emitted `[]` when no rows were filled in. pruneEmpty()
in apps.ts clears none of its guards on an empty array, so an untouched
array setting was persisted into metadata.app.values as `{ item: [] }`
while the launch URL carried no `item=` at all. Emit undefined instead,
keeping the saved values 1:1 with the URL as that module documents.
SonarCloud flagged three issues, all in item-format.ts:
- S8786: /\{([^}]+)\}/g backtracks quadratically on an unterminated
brace. Not reachable from an allowlisted manifest, but a two-indexOf
scan is linear and no less readable, so fix rather than suppress.
- S7780: String.raw for the escape replacement.
- S7755: .at(-1) over [length - 1].
Review asked for explicit round-trip coverage of blank fields: a blank
in the middle of a row, consecutive blanks, a blank required field, and
an all-blank row - all driven from rows so the dropped separators are
the composer's own. Separator-only and dangling-separator tokens are
covered too: they normalise rather than round-trip, and a combinatorial
test pins the guarantee they rest on, that the composer can never emit
one.
Also pins an array of plain scalars (no x-format), the one array shape
no store app ships today.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
|
Thanks — all five addressed. Code changes are in ea73dd2. 1.
|
| case | row | token | round-trips |
|---|---|---|---|
| blank in the middle | section+name+description, no price |
Lunch|Soup|Ask inside |
yes |
| two consecutive blanks | name + description only | Soup|Ask inside |
yes |
| every optional blank | name only | Espresso |
yes |
| blank required field | section + price, no name | Coffee|3.10 |
yes |
| every field blank | — | `` (empty) | yes |
On your third shape, a single | with everything else empty: it does not round-trip — '\|' parses to an all-blank row and re-composes to ''. That is correct rather than a gap, and worth being explicit about: the composer can never emit such a token. It drops a blank field together with its separator, so no row produces a leading, trailing, or doubled separator. A bare | can only come from a hand-edited metadata.app.values, and normalising it away (the row contributes no item) is the right outcome.
I did not want that resting on my say-so, so there is now a combinatorial test over all 16 present/absent field combinations asserting no emitted token ever starts with, ends with, or contains a doubled separator — plus tests that '|', '||', '|||', ' | ' collapse to nothing and 'a|' / '|a' normalise to 'a'.
2 + 3. Tracking issues — opened
Both in Screenly-Labs/app-store, since that is where the x-format contract and the reference composer live:
- Screenly-Labs/app-store issue 56 — separators inside field values are not escaped (Menu Board's parser already handles
\|; nothing emits it) - Screenly-Labs/app-store issue 57 — dropping a blank field's separator is what makes short tokens ambiguous, with a suggested encoding that preserves position
Each explains why it is not fixed downstream: Anthias mirrors the store byte-for-byte so identical rows build identical URLs in both UIs, and a one-sided change would diverge them.
4. SonarCloud — 3 issues, all mine, all fixed
The check reported green, but you were right that there were 3 new issues. All three were in item-format.ts, and one did touch the parsing logic from point 1:
| rule | line | issue |
|---|---|---|
typescript:S8786 (major) |
parseFormat |
/\{([^}]+)\}/g backtracks quadratically on an unterminated brace |
typescript:S7780 (minor) |
escapeRe |
use String.raw to avoid escaping \ |
typescript:S7755 (minor) |
parseItemToken |
prefer .at(-1) over [length - 1] |
S8786 is a real finding, not a false positive: [^}]+ followed by \} retries at every length for each start position, so '{' + 'a'.repeat(n) is O(n²). Not reachable in practice — fmt comes from a manifest fetched from an allowlisted host and is ~40 chars — but the remedy is a two-indexOf scan that is linear and no less readable, so I fixed it rather than suppressing. Behaviour is identical, including the {}-is-not-a-field edge case, and the existing parseFormat tests still pass unchanged.
5. Other array-typed shapes — checked across all 16 manifests
Walked every property in every manifest in the store index, recursively, looking for oneOf / anyOf / allOf / not / $ref / prefixItems / patternProperties / union type arrays:
array properties: menu-board.item, world-clock.tz (both items.type=object, x-format, required)
combinators/unions: NONE
type values used: array, boolean, number, object, string
x-widget values: datetime, location-map, select, text, timezone
So nothing else is caught today. On the mechanism: widgetFor keys strictly on schema.type === 'array', so a combinator-only schema with no type falls through to 'text' exactly as before — the new branch cannot claim it. No nested arrays and no maxItems in the wild either.
The one plausible shape nobody ships yet is an array of plain scalars (no x-format, no sub-properties). That already degrades sensibly — one input per row, raw value as the token — and is now pinned by a test so it does not regress into something worse than the old skip.
|
One correction to my previous comment, for the record: I said the "every shape Menu Board documents" framing had missed the blank-in-the-middle case. Checking the original commit, it had not — What the new block genuinely adds is (a) driving those cases from rows rather than tokens, so the dropped separators are the composer's own rather than ones I wrote by hand, and (b) the combinatorial invariant over all 16 present/absent combinations. Both are worth having, but the coverage gap I implied was not there. The rest of that comment stands as written. SonarCloud is now reporting 0 open issues on this PR, down from the 3 noted above. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
src/anthias_server/app/static/src/apps/manifest-form.ts:257
- Saved tokens are rehydrated via
addRow(...), but becauseaddRow()currently applies themaxItemsguard, any saved entries beyond the cap will be dropped andsync()will emit a shortened value set. If you makeaddRow()cap only non-seeded rows, make sure this seed path opts into that behavior.
// Edit mode: repopulate from the tokens saved in metadata.app.values.
const saved = Array.isArray(seedValue) ? seedValue : []
for (const token of saved) {
if (typeof token !== 'string' || !token.trim()) continue
addRow(
fmt
? parseItemToken(fmt, token, required)
: { [keys[0] as string]: token },
)
}
src/anthias_server/app/static/src/apps/manifest-form.test.ts:112
- The DOM tests append a new
hosttodocument.bodyon everybeforeEach()but never remove prior hosts. This can leak DOM state between tests and makes the suite order-dependent as it grows.
Clearing the body (or removing the previous host) in beforeEach keeps the tests isolated.
beforeEach(() => {
host = document.createElement('div')
document.body.appendChild(host)
latest = {}
})
addRow() enforced maxItems for every row, including those seeded from metadata.app.values. If a manifest introduced or lowered maxItems after an install, reopening the edit modal rendered only the first maxItems rows and the sync() that follows seeding immediately propagated the truncated token list — so the operator lost items by opening the modal, before touching anything. Move the cap to the Add button's click handler: it gates the affordance, not the data. A saved config over the cap renders in full, Add stays disabled until removals bring it back under, and removal still works. Latent today (no app in the store index declares maxItems) but it would have bitten the first manifest that added one. Reported by Copilot. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
|
Issues Fixed
The Menu Board app installs from Add → Apps, but the board shows no menu items.
The config form skipped any setting whose JSON Schema type is
array:widgetFor()mapped it to'unsupported'andrenderField()dropped the field. Menu Board keeps its entire content in one array (item, exploded by{?…,item*,…}), so the operator saw Board title / Subtitle / Currency / Footer note and had no way to enter a single item. The launch URL carried the scalars and not oneitem=param.The app only falls back to its worked example when the query string is empty, so filling in any other field produced a board with a title and nothing under it — which is what made it look like the app itself was broken. World Clock's
tzwas affected identically. Those are the only two apps in the store index that use array settings.Description
Ports the app store's repeated-group widget: rows of the item schema's sub-fields, each composed into one token via its
x-format.apps/item-format.ts(new) —applyItemFormatis a faithful port of the store'slib/item-format.js, so the same rows build the same URL whether the operator configures the app in the store or here.parseItemTokenis the inverse.apps/widget-for.ts—type: 'array'→'array'.apps/manifest-form.ts—renderArrayField(): add/remove rows, compose on change, seed from saved values on edit. Timezone items get the shared IANA datalist._styles.scss— row / Add / remove / empty-state styles. The remove button states its own colours because.app-btn-iconreads--surface-text-muted, which only exists inside a.surfacecontext, and the modal body is not one.On reopening a saved config. The store never does this, but our edit modal does, and
x-formatdrops a blank field together with its separator — so a short token is genuinely ambiguous (Coffee|Espressois a section and an item;Cortado|3.10is an item and its price).parseItemTokenkeeps the leftmost fields and never drops one the schema marksrequired. Parts land on a contiguous run of fields, so re-composing an untouched row reproduces the token exactly: reopening and saving an installed app never rewrites its launch URL. That is pinned by a test over every shape Menu Board documents.Verified end-to-end by driving the real renderer in headless Chromium against the live manifests, inside a replica of the
.modal-cardDOM, then loading the generated URLs in the live apps:Corner Coffee+ footer note, zero items£3.40, description lines, footer note?tz=Europe/Oslo|Home&tz=America/New_Yorkrenders both city cardsReverting the one-line
widget-for.tschange fails 17 of the 18 new DOM tests.Dev dependency. The DOM suite needs a
document, sohappy-domis added as a dev dependency and preloaded viabunfig.toml. CI'sbun install --frozen-lockfile+bun run testpicks it up unchanged.Two upstream notes, not changed here because doing so unilaterally would diverge from the store:
|yields an ambiguous token. Menu Board's parser handles\|, but nothing emits it.x-formatdrops a blank field and its separator. Keeping the separator would make tokens unambiguous in both directions.Checklist
Both device boxes are unticked deliberately: this is operator-browser code in the Add → Apps modal, with no viewer, player or device-side component. It was verified in a real browser as described above rather than on hardware.
🤖 Generated with Claude Code