fix(smart-forms): allow visibility placeholders - #95
Conversation
AI ReviewRelaxes Checklist
U5 detail: S4 detail: Ran Issues found[warning — U6] key := value[2 : len(value)-2]
return strings.TrimSpace(key) != "" && !strings.ContainsAny(key, "{}")
Suggested fix — tighten the guard to reject keys that contain any whitespace: return key == strings.TrimSpace(key) && key != "" && !strings.ContainsAny(key, "{}")None of the test cases cover this variant, so the current test suite cannot catch the regression if pong-server behaviour ever diverges. [warning — U6] PR description claims The PR body states the change covers "nested and This is low-risk for the bug being fixed, but the phrasing in the PR description overstates coverage. Consider either:
This review was generated automatically. A human maintainer should still make the merge decision. |
AI ReviewRe-review after synchronize: relaxes Checklist
U5 detail: S4 detail: Issues found[warning — U6] The PR description and both docs files state that the fix "applies to … items expanded from However, no test exercises a section that uses both Fixed since last review ✅
This review was generated automatically. A human maintainer should still make the merge decision. |
AI ReviewRe-review after synchronize: relaxes Checklist
U5 detail: S4 detail: Issues foundNo issues found. Fixed since last review ✅
This review was generated automatically. A human maintainer should still make the merge decision. |
No schema in the bundled swagger sets `additionalProperties: false`, so the save endpoint stores a typo'd `visibilty` or a `head[].id` verbatim and the defect surfaces only as a console error in the browser: the item never hides, or the column list renders empty. `cduschema` now derives from the swagger the property surface of every item `class`, plus the nested spots where the schema IS precise (`extra`, `options[]`, a table's `head[]` / `body[]`), and `ValidateFile` rejects a key no variant declares. The allowlist is the UNION over every schema variant of a class. The swagger splits one class across type variants that each redeclare only part of the surface (`value` is on `Edit-int` but not on `Edit-default`), so checking a single variant would reject valid config. `TestProbeDocumentedKeysPresentInUnion` guards that assumption — a swagger update that drops a real key fails the tests instead of blocking users' pushes — and a class with no derived rule is skipped rather than rejected. Also transcribes two renderer-only rules the swagger cannot express (it carries no `minLength` anywhere): - a `label` / `image` `value` may not be an empty string; - an `image` `value` must be a URL the *server* can fetch — the renderer proxies it through `/api/1.0/image?src=`, which rejects a `data:` URI. Documented in `cdu-page-protocol.md` §5.1 / §10.1. Deliberately out of scope: whether a `visibility` placeholder is legal. #95 argues from pong-server that a pure `{{key}}` resolves server-side, and owns that question — this commit leaves the existing enum check and its message untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(cduschema): reject item keys and nested shapes the renderer rejects No schema in the bundled swagger sets `additionalProperties: false`, so the save endpoint stores a typo'd `visibilty` or a `head[].id` verbatim and the defect surfaces only as a console error in the browser: the item never hides, or the column list renders empty. `cduschema` now derives from the swagger the property surface of every item `class`, plus the nested spots where the schema IS precise (`extra`, `options[]`, a table's `head[]` / `body[]`), and `ValidateFile` rejects a key no variant declares. The allowlist is the UNION over every schema variant of a class. The swagger splits one class across type variants that each redeclare only part of the surface (`value` is on `Edit-int` but not on `Edit-default`), so checking a single variant would reject valid config. `TestProbeDocumentedKeysPresentInUnion` guards that assumption — a swagger update that drops a real key fails the tests instead of blocking users' pushes — and a class with no derived rule is skipped rather than rejected. Also transcribes two renderer-only rules the swagger cannot express (it carries no `minLength` anywhere): - a `label` / `image` `value` may not be an empty string; - an `image` `value` must be a URL the *server* can fetch — the renderer proxies it through `/api/1.0/image?src=`, which rejects a `data:` URI. Documented in `cdu-page-protocol.md` §5.1 / §10.1. Deliberately out of scope: whether a `visibility` placeholder is legal. #95 argues from pong-server that a pure `{{key}}` resolves server-side, and owns that question — this commit leaves the existing enum check and its message untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(smartform): audit the whole env tree for cross-file token defects on push `cduschema.ValidateFile` is per-file by signature — one relPath, one source — so it can never tell whether a page's `[[key]]` resolves against the locale files, or whether a `{{key}}` has a viewModel default. Those defects survive every server-side check too: the app_content endpoint stores the source opaquely and pong-server serves whatever it cannot substitute verbatim. The user meets them as a literal `[[key]]` on the rendered page. New `cduschema.ValidateTree` audits the whole env tree at once, and `pushSmartForm` runs it alongside the per-file pass. It is handed the WHOLE tree, not just the files being written: deleting a viewModel default breaks an untouched page, which is exactly what a changed-files audit would miss. The error/warning split follows what the runtime can still rescue: - a missing LOCALE key is an ERROR. Locale resolves from the static files only, so an unresolved `[[key]]` always reaches the browser as text. - a missing viewModel default is a WARNING. The bound process returns a per-request viewModel merged over the defaults, so it may well be filled at runtime — it renders literally only when the backend call fails. Also reported as warnings: a `label`/`image` bound to a default of `""` (the renderer rejects an empty value), a default no page references, and — for a LITERAL `contentLoop` — the exact entries missing a key the template uses. Placeholders inside a TEMPLATED `contentLoop` are backend-filled and never reported, so list pages stay quiet. Warnings are surfaced on a successful push under a new `warnings` field. Drive-by: `sortedSet` here and `sortedKeys2` in validate.go were byte-identical duplicates; both are now one generic `sortedKeys`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(cduschema): stop the new push checks from rejecting valid config Review follow-up on the two validation commits. Each item below was reproduced against this branch before it was changed. The swagger-derived key union was NARROWER than the protocol this repo documents, so seven shapes straight out of `cdu-page-protocol.md` §5 aborted a push: `mainMenu.options`, `carousel.items`/`extra`, `comments.title`, `timer.extra.duration`, `file.extra.{downloadUrl,uploadUrl,auth}`, `upload.extra.compression`, `attachment.extra.downloadUrl` — plus the §4 base envelope (`value`, `required`, `error`, `errorMsg`, `submitOnChange`, `extra`), of which the swagger's File.allOf[0] carries only half. `applySupplements` now widens the union from that same table, never switching a check on where the swagger produced no rule (`row`/`draggable` stay rule-less, `carousel.extra` stays unchecked). The probe test walked ten hand-picked classes and missed every failing one; it now mirrors §5 row for row. `ValidateTree` findings were tree-wide errors, so one dangling `[[key]]` in a page nobody touched — written through the web UI, or predating these checks — blocked every future push, and `pushSmartForm` has no force flag to get an unrelated fix out. `ValidateTreeScoped` keeps reading the whole tree but blocks only when this push is what breaks it: the page config, the app `locale`, or that page's `locale` is among the files being written. Everything else is a warning tagged `pre-existing`. Three narrower defects in the token scan: - `\[\[…\]\]` was matched in every string, so `"regexp": "^[[:alpha:]]+$"` aborted the push over a locale key nobody wrote. `regexp`/`mask` are pattern fields and are skipped. - `contentLoop` scoped the whole section map, so `"title": "{{section_title}}"` next to a loop was dropped and its default then reported as dead. Only `content` is loop-scoped now. - `walk` harvested strings from map values only, so tokens in a string array element were invisible. And `image.value` as a `data:` URI is now rejected, which is what the changelog entry and §4 already claimed — the renderer proxies through `/api/1.0/image?src=` and that proxy answers `400 "URL is not allowed"`. The no-false-positives test had enshrined the opposite. make build, make vet, go test ./... green; make discovery produces no drift. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QeU5WiHehPjJReAfefezBo --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
701eb1f to
7eb3ad0
Compare
AI ReviewRe-review after synchronize: relaxes
Checklist
U1 detail: All three commits follow the U5 detail: S4 detail: Ran U6 detail: Issues foundNo issues found. This review was generated automatically. A human maintainer should still make the merge decision. |
3 participants
What & why
pushSmartFormrejected page configs that used a pure{{viewModelKey}}placeholder forvisibility, although pong-server resolves that field before the page reaches the CDU renderer.This change accepts pure visibility placeholders for forms, sections, and rendered items in
header,modalHeader, andcontent, including nested andcontentLoopitems. Malformed, embedded, and multiple placeholders remain invalid. The unsupported sectionfooterslot is deliberately not broadened.The Smart Form skill and CDU protocol reference now distinguish server-side visibility binding from client-side reactive visibility.
Type of change
Checklist
make build && make vet && make testpass locallymake discoveryproduces no diffplugins/simulator/docs/.envcommitted; TLS behaviour is unchangedCHANGELOG.md→UnreleasedNotes for reviewers
Verified against pong-server template rendering: form and section visibility are injected explicitly, while rendered item properties are injected recursively. Placeholder values must resolve to
visible,disabled, orhidden.