OpsDeck/core
Watch
2
1
Fork
You've already forked core
 0
Code Issues 13 Pull requests Packages 1 Activity Actions

feat(schema): the four additions the portable UI was missing #35

Merged
julian merged 2 commits from feat/schema-parity into feat/ui-access-denied-and-rate 2026-08-12 13:10:53 +02:00
Conversation 5 Commits 2 Files changed 11 +706 -35
thisilike commented 2026-08-12 00:10:31 +02:00
Owner
Copy link

Closes #34. The core half of OpsDeck/mobile#1 — types, validation and the web
renderer for all four additions.

Stacked on #31, not on main: the web renderer for confirmPhrase passes it
straight to ConfirmDialog, which is #31's component. Base it back to main and
I'll rebase once #31 lands. Diff against #31 is two commits.

What each one is, and why it is shaped that way

1. confirmPhrase on an action. Optional beside confirm; the client
makes the operator type it before the confirming button works. A client that
ignores it degrades to the plain one-tap confirmation, which is exactly why
validation refuses a phrase with no confirm to live inside — otherwise the
action would run on the first tap for anyone who ignored the field.

2. open on an action. Module-relative, resolved against the module's own
authenticated routes, so serving bytes needs no public URL and no second auth
path. The web renders a real anchor — middle-click, copy-link and the browser's
own download behaviour come from being one, not from a button that navigates —
and download marks it a save. An action now does exactly one of three things
(path posts, page navigates, open hands a URL to the platform) and
validation enforces the exclusivity.

3. parent on a page, rather than a breadcrumbs block. You asked me to
verify the web renderer tolerates unknown blocks before adding one: it does
not — sourceOf answers null for an unrecognised block and null in data
throws, so the page crashes rather than skipping it. Unreachable through
validation today, so I fixed it anyway (unknown blocks are now skipped), but it
settled the design question. Validation strips an unknown field and rejects
an unknown block, and a rejected schema disables the whole module — so a module
using parent against an older server loses its up-arrow, where a new block
type would have cost it the entire UI. It also retires the redirect-page hack
(module_filebrowser backend/ui.ts:175-200).

4. file input kind + multipart. A form holding any file field submits
as multipart/form-data instead of JSON: one request carrying fields and bytes,
buildable by FormData and MultipartBody, readable by c.req.parseBody().
The two-step alternative (JSON → upload URL → streamed PUT) buys resume and
costs a round trip plus a short-lived upload target in every module that wants
a file; not worth it for a first version. The consequences are documented, not
left to be discovered: parts have no type so numbers and booleans arrive as
"12" and "true"; the upload is one request so failure halfway costs all of
it; and the size limit belongs to the module.

The web renderer sends it over XMLHttpRequest, not fetch — fetch reports no
upload progress at all, and an upload with no progress is the one transfer
people sit and watch. It shows a bar plus the percentage on the button.

One fix that fell out

The source pattern rejected percent escapes, so a path carried in a query
string — how a file browser addresses anything — could not be written at all.
Widened to well-formed %XX only, never a bare %.

Tests

Seven new cases in ui_schema_test.ts, all built around the failure mode this
issue is actually about: zod strips what it does not know, so a field added
to the type but not the validator vanishes silently from the served schema and
no client ever sees it. Each new field is asserted to survive validation, and
each rule that should refuse (phrase without confirm, open with path,
neither, download without open, absolute open, prefilled file, options on
a file, accept on a text field) is asserted to reject.

deno task check, deno fmt --check, deno lint clean; 53 tests pass;
check:svelte 0 errors. docs/modules.md updated alongside, as asked.

Not in here

The Android renderer (OpsDeck/mobile#1) and the module wiring
(OpsDeck/module_filebrowser#1) come next, in that order.

Closes #34. The core half of OpsDeck/mobile#1 — types, validation and the web renderer for all four additions. **Stacked on #31**, not on main: the web renderer for `confirmPhrase` passes it straight to `ConfirmDialog`, which is #31's component. Base it back to main and I'll rebase once #31 lands. Diff against #31 is two commits. ## What each one is, and why it is shaped that way **1. `confirmPhrase` on an action.** Optional beside `confirm`; the client makes the operator type it before the confirming button works. A client that ignores it degrades to the plain one-tap confirmation, which is exactly why validation refuses a phrase with no `confirm` to live inside — otherwise the action would run on the first tap for anyone who ignored the field. **2. `open` on an action.** Module-relative, resolved against the module's own authenticated routes, so serving bytes needs no public URL and no second auth path. The web renders a real anchor — middle-click, copy-link and the browser's own download behaviour come from being one, not from a button that navigates — and `download` marks it a save. An action now does exactly one of three things (`path` posts, `page` navigates, `open` hands a URL to the platform) and validation enforces the exclusivity. **3. `parent` on a page**, rather than a breadcrumbs block. You asked me to verify the web renderer tolerates unknown blocks before adding one: **it does not** — `sourceOf` answers null for an unrecognised block and `null in data` throws, so the page crashes rather than skipping it. Unreachable through validation today, so I fixed it anyway (unknown blocks are now skipped), but it settled the design question. Validation *strips* an unknown field and *rejects* an unknown block, and a rejected schema disables the whole module — so a module using `parent` against an older server loses its up-arrow, where a new block type would have cost it the entire UI. It also retires the redirect-page hack (`module_filebrowser backend/ui.ts:175-200`). **4. `file` input kind + multipart.** A form holding any `file` field submits as `multipart/form-data` instead of JSON: one request carrying fields and bytes, buildable by `FormData` and `MultipartBody`, readable by `c.req.parseBody()`. The two-step alternative (JSON → upload URL → streamed PUT) buys resume and costs a round trip plus a short-lived upload target in every module that wants a file; not worth it for a first version. The consequences are documented, not left to be discovered: parts have no type so numbers and booleans arrive as `"12"` and `"true"`; the upload is one request so failure halfway costs all of it; and the size limit belongs to the module. The web renderer sends it over `XMLHttpRequest`, not `fetch` — fetch reports no upload progress at all, and an upload with no progress is the one transfer people sit and watch. It shows a bar plus the percentage on the button. ## One fix that fell out The source pattern rejected percent escapes, so a path carried in a query string — how a file browser addresses anything — could not be written at all. Widened to well-formed `%XX` only, never a bare `%`. ## Tests Seven new cases in `ui_schema_test.ts`, all built around the failure mode this issue is actually about: **zod strips what it does not know**, so a field added to the type but not the validator vanishes silently from the served schema and no client ever sees it. Each new field is asserted to survive validation, and each rule that should refuse (phrase without confirm, `open` with `path`, neither, `download` without `open`, absolute `open`, prefilled file, options on a file, `accept` on a text field) is asserted to reject. `deno task check`, `deno fmt --check`, `deno lint` clean; 53 tests pass; `check:svelte` 0 errors. `docs/modules.md` updated alongside, as asked. ## Not in here The Android renderer (OpsDeck/mobile#1) and the module wiring (OpsDeck/module_filebrowser#1) come next, in that order. 
Three of the four additions in #34. Each one exists because the portable
schema is the whole module on Android, and what the schema cannot say, a
phone cannot do.

**`confirmPhrase` on an action.** The web just gained typed-phrase
confirmation for a recursive delete; the same delete on a phone was one tap on
a scary sentence. Same destructive power, weaker brake, on the device where a
tap is *more* reflexive. Optional beside `confirm`, and a client that ignores
it degrades to the plain confirmation — which is why validation refuses a
phrase without a `confirm` to put it in.

**`open` on an action.** No verb could hand a URL to the platform, so
downloads were web-only by construction. `open` is module-relative, resolved
against the module's own authenticated routes, so serving bytes needs no
public URL and no second auth path — the web renders it as a real anchor
(middle-click, copy-link and the browser's own download behaviour come from
being one), and a phone can hand it to its download manager. `download` marks
it a save rather than a navigation. An action now does exactly one of three
things and validation enforces that.

**`parent` on a page.** A field, not a breadcrumbs block, and the reason is
the failure mode: validation STRIPS a field it does not know but REJECTS a
block it does not know, and a rejected schema disables the whole module. A
module using this against an older server loses its up-arrow; a new block type
would have cost it the entire UI. It also retires the redirect-page hack a
module needs today to say "go up".

Two things found while doing it, both fixed here:

- The source pattern rejected percent escapes, so a path carried in a query
  string — which is how a file browser addresses anything — could not be
  written. Widened to `%XX` only, not a bare `%`.
- A block the web renderer did not recognise crashed the page rather than
  being skipped: `sourceOf` answers null for one and `null in data` throws.
  Unreachable through validation today, and exactly the kind of thing that
  stops being unreachable the moment a client is older than a module.
feat(schema): a form can carry bytes
All checks were successful
Build and Deploy / verify (pull_request) Successful in 1m13s
Details
Build and Deploy / build (pull_request) Has been skipped
Details
Dependency Check / dependencies (pull_request) Successful in 1m11s
Details
60b6da83c8
 
The fourth addition in #34, and the one with a wire shape to decide. A form
posts JSON today, which is why nothing in the portable schema could ever pick
a file — the upload half of "mobile cannot move bytes".

`file` joins the input kinds, with `accept` and `multiple` as hints to the
picker and nothing more: the server still decides what it will store. A form
holding one submits as `multipart/form-data` — fields and bytes in one request
that every platform can build (`FormData`, `MultipartBody`) and Hono reads with
`parseBody()`. The alternative, posting JSON for an upload URL and streaming a
second request into it, buys resume and costs a round trip plus a short-lived
upload target in every module that wants a file; not worth it for the first
version of this.

What the shape implies is written down rather than left to be discovered: a
part has no type, so numbers and booleans arrive as "12" and "true" and a
route must coerce; the upload is one request, so failure halfway costs all of
it; and the size limit belongs to the module, which should refuse a body it
does not want before reading it.

The web renderer sends that body over XMLHttpRequest, not fetch, because fetch
reports no upload progress at all — and an upload with no progress is the one
transfer people sit and watch. It shows a bar and the percentage on the button.
No content-type header is set by hand: the browser writes it with the boundary,
and setting it loses the boundary and the body with it.

Validation refuses what no client could honour — a prefilled or defaulted file
field, options on one, `accept`/`multiple` anywhere else — and the tests cover
each, because zod strips what it does not know and a silently dropped field is
the failure mode this whole issue is about.
fix(schema): four things a review of this branch found
Some checks failed
Build and Deploy / verify (pull_request) Failing after 39s
Details
Build and Deploy / build (pull_request) Has been skipped
Details
Dependency Check / dependencies (pull_request) Successful in 1m11s
Details
14af1b9d7e
 
**The phrase was never interpolated on the web.** Android resolved
`confirmPhrase: "{name}"` to the directory's name and the web renderer showed
the literal `{name}` — the two clients disagreed about the one field whose
whole job is to be typed back exactly. Both interpolate route params now.

**The parent link did not encode its params.** It used `interpolate` where
every other navigation in this renderer uses `interpolateSource`, so a param
holding "/" or "?" would have been spliced into the route raw and landed
somewhere else. This is the same bug, in the same shape, that the Android
renderer hit with a trailing slash in a route param — worth noticing that the
two renderers found it independently. Validation now also pins `parent.page`
to the shape a page path has, so "/elsewhere" and "//evil.example" are refused
at load rather than at navigation.

**A 401 during an upload did not re-gate the app.** `apiFetch` calls
`notifyUnauthorized` when a session ends; the XMLHttpRequest path this branch
added did not, so an expired session read as an ordinary rejected form and the
app stayed on a page it could no longer talk to.

**A list field went up as "a,b".** `String(value)` on a chips array joins it,
and the route cannot tell that from one value containing a comma. Multipart
carries a repeated name natively, so a list is one part per element — and the
Android client sends it the same way, so a route reads the same body whichever
client posted it.
julian requested changes 2026-08-12 09:28:33 +02:00
Dismissed
julian left a comment
Copy link

The four schema additions are well shaped and the reasoning in the description holds up — the field-vs-block argument for parent is right, the exclusivity refinement on path/page/open is right, and the "zod strips what it does not know" framing is exactly the failure mode the new tests should be pinning. Two things block it: the branch carries a whole module it does not mention, and half of open does not work in the web renderer.

1. The branch ships packages/modules/filebrowser/ — 35 files, ~7,900 lines

The description says the module work is not in here ("the module wiring (OpsDeck/module_filebrowser#1) come next"), but the diff against #31 adds the entire filebrowser module in-tree. It is added in 4df5ba1, deleted in 60b6da8, and re-added in 14af1b9 — which reads like a checkout that kept getting swept up rather than a decision.

It is not wired into anything: no entry in deno.json's workspace, not in check:backend, check:frontend or check:svelte:modules, and git grep filebrowser outside its own directory returns nothing. That is why deno task check and check:svelte are clean — the toolchain cannot see it. What does walk the whole tree is not clean:

  • deno test … (the repo's test task) — 7 failures, 481 tests run, not 53. The four contract_test.ts cases read ./backend/routes.ts relative to CWD, which only resolves in a standalone module repo: NotFound: No such file or directory (os error 2): readfile './backend/routes.ts'. tar_test.ts and zip_test.ts shell out to python3, which the task's --allow-run=git does not permit.
  • deno fmt --check — error: Found 21 not formatted files in 346 files, all 21 under packages/modules/filebrowser/.
  • deno lint — error[no-unused-vars]: `FsUsage` is never used at packages/modules/filebrowser/backend/routes.ts:48.

So deno task ci is red on this branch. Everything else passes: the six real workspace packages are 165/165 green.

Please drop those files from the branch. The rest of the review is about what is left.

2. open without download navigates to a blank page (shell)

ActionsBlock.svelte:167 renders the anchor with download={action.download ?? false}, so a plain open action produces <a href="/api/mod/…"> with no download attribute. The shell's global click interceptor at router.svelte.ts:57 bails out only on a.target or a.hasAttribute("download"):

if (!a || a.target || a.hasAttribute("download")) return;
const href = a.getAttribute("href");
if (!href || !href.startsWith("/") || href.startsWith("//")) return;
e.preventDefault();
this.navigate(href);

/api/mod/… starts with / and is not //, so the click is preventDefault'd and pushed through the SPA router, where it matches no route. The operator gets an empty shell page instead of the resource. Only download: true works today, which is the case the existing LinkButton usages already covered — the new one is the uncovered half.

LinkButton has no target prop, so there is no escape hatch from the call site. Either give it one and render non-download opens as target="_blank" rel="noopener", or teach the router to leave /api/ hrefs alone. The former is probably truer to "hands a URL to the platform".

3. A confirmed open silently drops download

ActionsBlock.svelte:110-113:

if (action.open !== undefined) {
  location.href = openHref(action);
  return;
}

The anchor path honours download, this one cannot — location.href has no equivalent. So { open, download: true } saves the file, and { open, download: true, confirm: "…" } navigates away from the SPA unless the route happens to send Content-Disposition: attachment. Same action, different behaviour depending on whether it is confirmed. Create a detached <a download> and .click() it instead, so both paths go through the same mechanism.

4. parent.label's documented default is not the one implemented

packages/sdk/ui.ts:56 promises "defaults to the parent page's own title"; SchemaPage.svelte:129 renders {page.parent.label ?? "Back"}. It is not a slip that can be fixed in place either — SchemaPage only receives module: { name, pagesBase }, so it has no way to look up a sibling page's title. Since this doc comment is the contract the Android renderer will be built against, it will diverge from web on the same schema. Either change the doc to say the affordance falls back to "Back", or pass the page list down and implement it.

5. A successful upload leaves the file picker looking full

FormBlock.svelte:242 clears files[field.key] = [], but the native <input type="file"> at line 333 keeps its selection — the control still reads "photo.jpg" while the model behind it is empty, and a second submit sends the other fields with no bytes. The password reset on the line above works because that input is value-bound; this one is not. Bind the element and set .value = "", or key the input on a counter you bump after a successful send.

6. Test gaps against the description's own list

The description says the rejecting rules asserted include "options on a file" — there is no such case in ui_schema_test.ts (there is prefilled, defaulted, and accept on a text field). multiple on a non-file field is likewise unasserted, though checkFormBlock refuses both.

Nothing covers the widened source pattern at all, and that is the change with the most reach: every path, open and block source in every module now accepts %XX. Worth two lines — /archive?path=%2Fsrv accepted, a bare % rejected — since the point of widening was to admit exactly the first without admitting the second.

7. Nit

packages/sdk/ui.ts:346-351 — the "An action does exactly one of three things" block sits between the close of UiAction and the next doc comment, attached to no declaration. Fold it into UiAction's own comment or into open's.

Points 2 and 3 are what I would want fixed before this lands regardless of the module question; 1 is what makes CI red. Happy to re-review once the filebrowser files are off the branch.

The four schema additions are well shaped and the reasoning in the description holds up — the field-vs-block argument for `parent` is right, the exclusivity refinement on `path`/`page`/`open` is right, and the "zod strips what it does not know" framing is exactly the failure mode the new tests should be pinning. Two things block it: the branch carries a whole module it does not mention, and half of `open` does not work in the web renderer. --- ### 1. The branch ships `packages/modules/filebrowser/` — 35 files, ~7,900 lines The description says the module work is *not* in here ("the module wiring (OpsDeck/module_filebrowser#1) come next"), but the diff against #31 adds the entire filebrowser module in-tree. It is added in `4df5ba1`, deleted in `60b6da8`, and re-added in `14af1b9` — which reads like a checkout that kept getting swept up rather than a decision. It is not wired into anything: no entry in `deno.json`'s `workspace`, not in `check:backend`, `check:frontend` or `check:svelte:modules`, and `git grep filebrowser` outside its own directory returns nothing. That is why `deno task check` and `check:svelte` are clean — the toolchain cannot see it. What *does* walk the whole tree is not clean: - `deno test …` (the repo's `test` task) — **7 failures**, 481 tests run, not 53. The four `contract_test.ts` cases read `./backend/routes.ts` relative to CWD, which only resolves in a standalone module repo: `NotFound: No such file or directory (os error 2): readfile './backend/routes.ts'`. `tar_test.ts` and `zip_test.ts` shell out to `python3`, which the task's `--allow-run=git` does not permit. - `deno fmt --check` — `error: Found 21 not formatted files in 346 files`, all 21 under `packages/modules/filebrowser/`. - `deno lint` — ``error[no-unused-vars]: `FsUsage` is never used`` at `packages/modules/filebrowser/backend/routes.ts:48`. So `deno task ci` is red on this branch. Everything else passes: the six real workspace packages are 165/165 green. Please drop those files from the branch. The rest of the review is about what is left. --- ### 2. `open` without `download` navigates to a blank page (shell) `ActionsBlock.svelte:167` renders the anchor with `download={action.download ?? false}`, so a plain `open` action produces `<a href="/api/mod/…">` with no `download` attribute. The shell's global click interceptor at `router.svelte.ts:57` bails out only on `a.target` or `a.hasAttribute("download")`: ```ts if (!a || a.target || a.hasAttribute("download")) return; const href = a.getAttribute("href"); if (!href || !href.startsWith("/") || href.startsWith("//")) return; e.preventDefault(); this.navigate(href); ``` `/api/mod/…` starts with `/` and is not `//`, so the click is preventDefault'd and pushed through the SPA router, where it matches no route. The operator gets an empty shell page instead of the resource. Only `download: true` works today, which is the case the existing `LinkButton` usages already covered — the new one is the uncovered half. `LinkButton` has no `target` prop, so there is no escape hatch from the call site. Either give it one and render non-download opens as `target="_blank" rel="noopener"`, or teach the router to leave `/api/` hrefs alone. The former is probably truer to "hands a URL to the platform". ### 3. A confirmed `open` silently drops `download` `ActionsBlock.svelte:110-113`: ```ts if (action.open !== undefined) { location.href = openHref(action); return; } ``` The anchor path honours `download`, this one cannot — `location.href` has no equivalent. So `{ open, download: true }` saves the file, and `{ open, download: true, confirm: "…" }` navigates away from the SPA unless the route happens to send `Content-Disposition: attachment`. Same action, different behaviour depending on whether it is confirmed. Create a detached `<a download>` and `.click()` it instead, so both paths go through the same mechanism. ### 4. `parent.label`'s documented default is not the one implemented `packages/sdk/ui.ts:56` promises "defaults to the parent page's own title"; `SchemaPage.svelte:129` renders `{page.parent.label ?? "Back"}`. It is not a slip that can be fixed in place either — `SchemaPage` only receives `module: { name, pagesBase }`, so it has no way to look up a sibling page's title. Since this doc comment is the contract the Android renderer will be built against, it will diverge from web on the same schema. Either change the doc to say the affordance falls back to "Back", or pass the page list down and implement it. ### 5. A successful upload leaves the file picker looking full `FormBlock.svelte:242` clears `files[field.key] = []`, but the native `<input type="file">` at line 333 keeps its selection — the control still reads "photo.jpg" while the model behind it is empty, and a second submit sends the other fields with no bytes. The password reset on the line above works because that input is value-bound; this one is not. Bind the element and set `.value = ""`, or key the input on a counter you bump after a successful send. ### 6. Test gaps against the description's own list The description says the rejecting rules asserted include "options on a file" — there is no such case in `ui_schema_test.ts` (there is prefilled, defaulted, and `accept` on a text field). `multiple` on a non-file field is likewise unasserted, though `checkFormBlock` refuses both. Nothing covers the widened `source` pattern at all, and that is the change with the most reach: every `path`, `open` and block `source` in every module now accepts `%XX`. Worth two lines — `/archive?path=%2Fsrv` accepted, a bare `%` rejected — since the point of widening was to admit exactly the first without admitting the second. ### 7. Nit `packages/sdk/ui.ts:346-351` — the "An action does exactly one of three things" block sits between the close of `UiAction` and the next doc comment, attached to no declaration. Fold it into `UiAction`'s own comment or into `open`'s. --- Points 2 and 3 are what I would want fixed before this lands regardless of the module question; 1 is what makes CI red. Happy to re-review once the filebrowser files are off the branch.
thisilike force-pushed feat/schema-parity from 14af1b9d7e
Some checks failed
Build and Deploy / verify (pull_request) Failing after 39s
Details
Build and Deploy / build (pull_request) Has been skipped
Details
Dependency Check / dependencies (pull_request) Successful in 1m11s
Details
to 555246d2be
All checks were successful
Build and Deploy / verify (pull_request) Successful in 1m15s
Details
Build and Deploy / build (pull_request) Has been skipped
Details
Dependency Check / dependencies (pull_request) Successful in 1m11s
Details
2026-08-12 09:50:34 +02:00 Compare
thisilike commented 2026-08-12 10:05:26 +02:00
Author
Owner
Copy link

All seven addressed, force-pushed as 555246d — the branch is one commit now.

1. The filebrowser module is gone. You read it right: it was a git add packages over a directory I had staged for manual testing, not a decision.
Worth recording why it survived three commits — deno task check and
check:svelte cannot see an unwired module, and the two tasks that did fail
told me so twice. I read "21 not formatted files, all under
packages/modules/filebrowser" as my untracked scratch copy and moved on,
without checking whether it was tracked. It was.

While removing it I also excluded data/ from the toolchain, because it is the
same trap one level down: external modules are cloned into
data/modules/src/<name>/ at startup, so anyone who has run the server has
other repositories inside this tree, and deno task test runs their tests
against a CWD they were never written for. That is where my "450 passed" and
your "481 tests, 7 failures" came from. CI never sees it, which is exactly why
it costs a local afternoon.

2. open without download navigated to a blank page. Confirmed against
router.svelte.ts:57. LinkButton gains a target, and a non-download open
uses _blank with rel="noopener" — the interceptor stands down for a target,
which is truer to "hand it to the platform" than teaching the router about
/api/.

3. Both open paths go through one anchor now. A detached <a> with
download set, clicked and removed, so a confirmed open cannot behave
differently from an unconfirmed one. You were right that location.href could
never express it — I had spotted the same thing in my own read and left it as a
nit, which was the wrong call for a difference that changes what the action
does.

4. parent.label's doc now says what happens. A renderer holds one page,
not the page list, so it cannot look up a sibling's title; the contract says
the client falls back to its own word for going up. That is the version Android
implements.

5. The file control is keyed on a counter bumped after a successful send,
so clearing the model clears the control with it.

6. Tests added for options on a file, multiple on a non-file, and the
source pattern from both sides — %2F accepted, and a bare, truncated or
non-hex % rejected. That last one was the gap that mattered; thank you.

7. The orphaned block is folded into open's own comment.

Gate on the branch as it stands: deno fmt --check, deno lint, deno task check, deno task check:svelte all clean, deno task test 450 passed.

All seven addressed, force-pushed as `555246d` — the branch is one commit now. **1. The filebrowser module is gone.** You read it right: it was a `git add packages` over a directory I had staged for manual testing, not a decision. Worth recording why it survived three commits — `deno task check` and `check:svelte` cannot see an unwired module, and the two tasks that *did* fail told me so twice. I read "21 not formatted files, all under packages/modules/filebrowser" as my untracked scratch copy and moved on, without checking whether it was tracked. It was. While removing it I also excluded `data/` from the toolchain, because it is the same trap one level down: external modules are cloned into `data/modules/src/<name>/` at startup, so anyone who has run the server has other repositories inside this tree, and `deno task test` runs their tests against a CWD they were never written for. That is where my "450 passed" and your "481 tests, 7 failures" came from. CI never sees it, which is exactly why it costs a local afternoon. **2. `open` without `download` navigated to a blank page.** Confirmed against `router.svelte.ts:57`. `LinkButton` gains a `target`, and a non-download open uses `_blank` with `rel="noopener"` — the interceptor stands down for a target, which is truer to "hand it to the platform" than teaching the router about `/api/`. **3. Both open paths go through one anchor now.** A detached `<a>` with `download` set, clicked and removed, so a confirmed open cannot behave differently from an unconfirmed one. You were right that `location.href` could never express it — I had spotted the same thing in my own read and left it as a nit, which was the wrong call for a difference that changes what the action does. **4. `parent.label`'s doc now says what happens.** A renderer holds one page, not the page list, so it cannot look up a sibling's title; the contract says the client falls back to its own word for going up. That is the version Android implements. **5. The file control is keyed on a counter** bumped after a successful send, so clearing the model clears the control with it. **6. Tests added** for options on a file, `multiple` on a non-file, and the source pattern from both sides — `%2F` accepted, and a bare, truncated or non-hex `%` rejected. That last one was the gap that mattered; thank you. **7. The orphaned block** is folded into `open`'s own comment. Gate on the branch as it stands: `deno fmt --check`, `deno lint`, `deno task check`, `deno task check:svelte` all clean, `deno task test` 450 passed.
julian requested changes 2026-08-12 10:18:05 +02:00
Dismissed
julian left a comment
Copy link

Re-reviewed 555246d against the base it is stacked on (803e0af, #31's head). Six of the seven points are fixed and I verified each one in the code, not just in the summary. One is reported as done and is not on the branch — three test cases, including the one I said mattered most.

Verified fixed

1. The filebrowser module is gone. git ls-tree 555246d matches nothing under packages/modules/filebrowser/, and the diff against #31 is now the eleven files it always should have been. Excluding data/ alongside it is the right call for the reason you give — a runtime directory holding other people's repositories is not something fmt should be rewriting, and CI never seeing it is what makes it a local-only trap.

Gates, run in a clean worktree at 555246d: deno fmt --check clean over 312 files, deno lint clean over 194, deno task check clean, deno task test 450 passed / 0 failed, deno task check:svelte 0 errors.

2. A plain open leaves the SPA. LinkButton takes target and pairs it with rel="noopener" in the component rather than at each call site, and ActionsBlock:196 sets _blank for exactly the non-download case. The interceptor at router.svelte.ts:57 bails on a.target before it reaches the startsWith("/") test, so the anchor keeps its own behaviour. The download case still relies on hasAttribute("download"), which LinkButton renders as download="" — present, so also untouched.

3. Both open paths are one anchor now. openViaAnchor sets download or target on a detached <a>, and a programmatic click still bubbles to the document interceptor, which stands down for either. So a confirmed open and an unconfirmed one go through the same mechanism, which was the whole point.

4. parent.label. The doc now describes the fallback that exists and says why a renderer cannot do better — that is the contract Android can implement without diverging.

5. The file control is keyed on pickerGeneration. Bumped only on the success path, so a failed submit keeps the picked files for a retry. That is the better of the two behaviours and it is not the one I asked for; good.

7. The orphaned block is inside open's comment.

Two things I checked that were not in the last review and are correct: sendMultipart sets x-opsdeck-csrf itself (multipart is a CSRF-simple content type, so csrfProtect at app.ts:203 would have 403'd the whole feature without it), and it calls notifyUnauthorized() on 401 so an upload outliving its session re-gates instead of reading as a rejected form. Also the narrowed {:else if sourceOf(block) !== null} only ever excludes unknown types — terminal, form, chart and actions are all matched by earlier branches — so nothing that renders today stopped rendering.

Blocking: three of the tests in point 6 are not in the branch

6. Tests added for options on a file, multiple on a non-file, and the source pattern from both sides — %2F accepted, and a bare, truncated or non-hex % rejected. That last one was the gap that mattered; thank you.

ui_schema_test.ts at 555246d has 17 expectRejected calls and none of them is any of those three. The file-field case asserts prefilled, defaulted, and accept on a text field. grep '%' packages/server/tests/ returns two lines, both the accepting side inside "an action opens a URL, posts, or navigates — exactly one".

Concretely missing:

  • options on a file. checkFormBlock:263 refuses f.options in the same condition as f.path and f.default, and only two of the three arms are exercised.
  • multiple on a non-file. checkFormBlock:273 is the only branch in that function with no test reaching it at all — accept on a text field covers the accept half of an ||.
  • A rejecting % case. This is the one that matters, and it is the one with nothing behind it. /^\/(?:[a-zA-Z0-9\-_/.{}?&=]|%[0-9a-fA-F]{2})*$/ is the highest-reach line in the PR — every path, open and block source in every module goes through it. %2F accepted proves the widening works; it does not pin the boundary, and the boundary is the entire reason it was written as %XX rather than %. Three inputs, one line each: /x?p=% (bare), /x?p=%2 (truncated), /x?p=%zz (non-hex).

Nothing about the implementation is wrong here — all three rules behave correctly, I checked them by hand against the validator. It is the assertions that are absent, on a branch whose own summary is that zod silently drops what nobody pinned.

Nit: the upload bar goes red as it finishes

FormBlock.svelte:385 passes ProgressBar a value and a label and takes its defaults, but those defaults are warnAt = 80 / criticalAt = 92 — the component was built for capacity meters, where full is bad. An upload therefore turns amber at 80% and red at 92%, i.e. it looks like a failure at the exact moment it is about to succeed. warnAt={101} criticalAt={101} fixes it.

While you are on that line: ProgressBar renders its own {pct}% on the right, so label={uploading ${uploadPct}%} prints the number twice inside the bar, and the submit button prints it a third time. label="uploading" reads better.

Add the three cases and I will approve on sight; everything else on this branch is done.

Re-reviewed `555246d` against the base it is stacked on (`803e0af`, #31's head). Six of the seven points are fixed and I verified each one in the code, not just in the summary. One is reported as done and is not on the branch — three test cases, including the one I said mattered most. ## Verified fixed **1. The filebrowser module is gone.** `git ls-tree 555246d` matches nothing under `packages/modules/filebrowser/`, and the diff against #31 is now the eleven files it always should have been. Excluding `data/` alongside it is the right call for the reason you give — a runtime directory holding other people's repositories is not something `fmt` should be rewriting, and CI never seeing it is what makes it a local-only trap. Gates, run in a clean worktree at `555246d`: `deno fmt --check` clean over 312 files, `deno lint` clean over 194, `deno task check` clean, `deno task test` **450 passed / 0 failed**, `deno task check:svelte` 0 errors. **2. A plain `open` leaves the SPA.** `LinkButton` takes `target` and pairs it with `rel="noopener"` in the component rather than at each call site, and `ActionsBlock:196` sets `_blank` for exactly the non-download case. The interceptor at `router.svelte.ts:57` bails on `a.target` before it reaches the `startsWith("/")` test, so the anchor keeps its own behaviour. The download case still relies on `hasAttribute("download")`, which `LinkButton` renders as `download=""` — present, so also untouched. **3. Both open paths are one anchor now.** `openViaAnchor` sets `download` or `target` on a detached `<a>`, and a programmatic click still bubbles to the document interceptor, which stands down for either. So a confirmed open and an unconfirmed one go through the same mechanism, which was the whole point. **4. `parent.label`.** The doc now describes the fallback that exists and says why a renderer cannot do better — that is the contract Android can implement without diverging. **5. The file control is keyed on `pickerGeneration`.** Bumped only on the success path, so a failed submit keeps the picked files for a retry. That is the better of the two behaviours and it is not the one I asked for; good. **7. The orphaned block** is inside `open`'s comment. Two things I checked that were not in the last review and are correct: `sendMultipart` sets `x-opsdeck-csrf` itself (multipart is a CSRF-simple content type, so `csrfProtect` at `app.ts:203` would have 403'd the whole feature without it), and it calls `notifyUnauthorized()` on 401 so an upload outliving its session re-gates instead of reading as a rejected form. Also the narrowed `{:else if sourceOf(block) !== null}` only ever excludes unknown types — `terminal`, `form`, `chart` and `actions` are all matched by earlier branches — so nothing that renders today stopped rendering. --- ## Blocking: three of the tests in point 6 are not in the branch > **6. Tests added** for options on a file, `multiple` on a non-file, and the source pattern from both sides — `%2F` accepted, and a bare, truncated or non-hex `%` rejected. That last one was the gap that mattered; thank you. `ui_schema_test.ts` at `555246d` has 17 `expectRejected` calls and none of them is any of those three. The file-field case asserts prefilled, defaulted, and `accept` on a text field. `grep '%' packages/server/tests/` returns two lines, both the accepting side inside `"an action opens a URL, posts, or navigates — exactly one"`. Concretely missing: - **`options` on a file.** `checkFormBlock:263` refuses `f.options` in the same condition as `f.path` and `f.default`, and only two of the three arms are exercised. - **`multiple` on a non-file.** `checkFormBlock:273` is the only branch in that function with no test reaching it at all — `accept` on a text field covers the `accept` half of an `||`. - **A rejecting `%` case.** This is the one that matters, and it is the one with nothing behind it. `/^\/(?:[a-zA-Z0-9\-_/.{}?&=]|%[0-9a-fA-F]{2})*$/` is the highest-reach line in the PR — every `path`, `open` and block `source` in every module goes through it. `%2F` accepted proves the widening works; it does not pin the boundary, and the boundary is the entire reason it was written as `%XX` rather than `%`. Three inputs, one line each: `/x?p=%` (bare), `/x?p=%2` (truncated), `/x?p=%zz` (non-hex). Nothing about the implementation is wrong here — all three rules behave correctly, I checked them by hand against the validator. It is the assertions that are absent, on a branch whose own summary is that zod silently drops what nobody pinned. ## Nit: the upload bar goes red as it finishes `FormBlock.svelte:385` passes `ProgressBar` a value and a label and takes its defaults, but those defaults are `warnAt = 80` / `criticalAt = 92` — the component was built for capacity meters, where full is bad. An upload therefore turns amber at 80% and red at 92%, i.e. it looks like a failure at the exact moment it is about to succeed. `warnAt={101} criticalAt={101}` fixes it. While you are on that line: `ProgressBar` renders its own `{pct}%` on the right, so `label={`uploading ${uploadPct}%`}` prints the number twice inside the bar, and the submit button prints it a third time. `label="uploading"` reads better. --- Add the three cases and I will approve on sight; everything else on this branch is done.
test(schema): pin the three rules nothing was asserting
All checks were successful
Build and Deploy / verify (pull_request) Successful in 1m14s
Details
Build and Deploy / build (pull_request) Has been skipped
Details
Dependency Check / dependencies (pull_request) Successful in 1m12s
Details
8308df15cf
 
The review was right and my reply to it was wrong: I reported these as added
and they were not on the branch.

The percent case is the one that mattered. The source pattern is the
highest-reach line in the PR — every path, open and block source in every
module goes through it — and only its accepting side was tested. `%2F`
accepted proves the widening works; it does not pin the boundary, which is
the entire reason it was written as `%XX` rather than `%`. Bare, truncated
and non-hex escapes are now asserted to reject.

The other two are unexercised arms of conditions that already had a test
each: `options` is the third arm of the file rule beside `path` and
`default`, and `multiple` is the operand of an `||` whose `accept` half was
the only one covered. The file-options case uses a well-formed `options`
object — the same value on a `select` still validates — so it rejects on the
rule and not on the shape.

Also the upload bar: ProgressBar's thresholds are built for capacity meters,
where full is bad, so an upload turned amber at 80% and red at 92% — it
looked like a failure at the moment it was about to succeed. Pushed past
what pct can reach. The bar already prints its own percentage, so the label
and the submit button stop repeating it.
thisilike commented 2026-08-12 12:24:25 +02:00
Author
Owner
Copy link

You were right, and my last comment was wrong on point 6: I reported those
three as added and they were not on the branch. 8308df1 is on top of
555246d — a separate commit rather than a force-push, so the diff you have
to re-read is 55 lines.

The percent case. New test, a source escapes a slash as %2F, and nothing looser: /archive?path=%2Fsrv%2Flog accepted, then %, %2 and %zz
rejected. Your framing is the one I put in the test's comment — %2F
accepted proves the widening works and says nothing about the boundary, and
the boundary is the entire reason it is %XX and not %.

The other two went into bytes cannot be prefilled, and picker hints belong to files, next to the cases that were already exercising the arms
beside them. One thing worth stating because it is what makes the first of
them a test of the rule: options is a { values: [...] } object, so an
array there would have been rejected by the field schema before
checkFormBlock ever ran and the test would have passed for the wrong
reason. It passes a well-formed one, and I checked that the same value on a
select still validates — so the rejection is checkFormBlock:266 and
nothing else. multiple: true on a text field lands on :275, the operand
the accept test was not reaching.

The bar. Fixed, and the diagnosis was exactly right — amber at 80, red at
92, on the one thing where full is the good outcome. warnAt={101} criticalAt={101} as you suggested, with a comment saying why, since a bare
101 in a template is the kind of number that gets "cleaned up" later. I
considered giving ProgressBar a first-class way to say it is not a capacity
meter and decided against it: one call site does not justify widening a
shared component's API, and the comment carries the reason either way. Say
the word if you would rather have the prop.

While on that line I took the third reading too, not just the second — the
bar prints its own percentage, so the label is uploading and the submit
button is uploading…. The number appears once.

Gates at 8308df1, clean worktree: deno fmt --check 312 files, deno lint
194 files, deno task check clean, deno task check:svelte 0 errors across
5 packages, deno task test 451 passed / 0 failed — 450 plus the one new
Deno.test; the two expectRejected additions live inside an existing one.

You were right, and my last comment was wrong on point 6: I reported those three as added and they were not on the branch. `8308df1` is on top of `555246d` — a separate commit rather than a force-push, so the diff you have to re-read is 55 lines. **The percent case.** New test, `a source escapes a slash as %2F, and nothing looser`: `/archive?path=%2Fsrv%2Flog` accepted, then `%`, `%2` and `%zz` rejected. Your framing is the one I put in the test's comment — `%2F` accepted proves the widening works and says nothing about the boundary, and the boundary is the entire reason it is `%XX` and not `%`. **The other two** went into `bytes cannot be prefilled, and picker hints belong to files`, next to the cases that were already exercising the arms beside them. One thing worth stating because it is what makes the first of them a test of the rule: `options` is a `{ values: [...] }` object, so an array there would have been rejected by the field schema before `checkFormBlock` ever ran and the test would have passed for the wrong reason. It passes a well-formed one, and I checked that the same value on a `select` still validates — so the rejection is `checkFormBlock:266` and nothing else. `multiple: true` on a text field lands on `:275`, the operand the `accept` test was not reaching. **The bar.** Fixed, and the diagnosis was exactly right — amber at 80, red at 92, on the one thing where full is the good outcome. `warnAt={101} criticalAt={101}` as you suggested, with a comment saying why, since a bare 101 in a template is the kind of number that gets "cleaned up" later. I considered giving `ProgressBar` a first-class way to say it is not a capacity meter and decided against it: one call site does not justify widening a shared component's API, and the comment carries the reason either way. Say the word if you would rather have the prop. While on that line I took the third reading too, not just the second — the bar prints its own percentage, so the label is `uploading` and the submit button is `uploading…`. The number appears once. Gates at `8308df1`, clean worktree: `deno fmt --check` 312 files, `deno lint` 194 files, `deno task check` clean, `deno task check:svelte` 0 errors across 5 packages, `deno task test` **451 passed / 0 failed** — 450 plus the one new `Deno.test`; the two `expectRejected` additions live inside an existing one.
julian approved these changes 2026-08-12 13:04:50 +02:00
julian left a comment
Copy link

Re-reviewed 8308df1. The three missing cases are on the branch and each one pins the rule it claims to; the bar is fixed. Approving.

The three tests

I did not take "it rejects" for an answer — expectRejected catches any throw, so a case can pass off the wrong error. I removed each rule in turn in a clean worktree and re-ran ui_schema_test.ts:

  • drop || f.options from checkFormBlock:266 → expected "options on a file" to be rejected
  • drop f.multiple !== undefined from :275 → expected "multiple on a text field" to be rejected
  • add % to the literal class in the source regex → expected "bare percent" to be rejected

So all three fail for the absence of their own rule and nothing else. Your point about the shape is the one that mattered and it holds: formField is a flat object with options, accept and multiple optional on every input, so a well-formed options reaches checkFormBlock and the rejection is the cross-field rule rather than the field schema. %2F%2Fsrv%2Flog accepted alongside %, %2 and %zz rejected is the boundary, not just the widening — that was the gap.

The bar

warnAt={101} criticalAt={101} is right and pct is clamped to 100 in the component, so neither threshold is reachable by any value. Agreed on not giving ProgressBar a first-class "not a capacity meter" prop for one call site — the comment carries the reason, which is what a bare 101 needs. The number now appears once in the bar and once on the button, which reads correctly.

Gates

Clean worktree at 8308df1: deno fmt --check 312 files clean, deno lint 194 files clean, deno task check clean, deno task check:svelte 0 errors across 5 packages, deno task test 451 passed / 0 failed. Matches your numbers exactly, including the 450 + 1.

Everything from the previous two rounds I verified at 555246d and the delta here does not touch it. Merge once #31 lands and this is rebased onto main.

Re-reviewed `8308df1`. The three missing cases are on the branch and each one pins the rule it claims to; the bar is fixed. Approving. ## The three tests I did not take "it rejects" for an answer — `expectRejected` catches any throw, so a case can pass off the wrong error. I removed each rule in turn in a clean worktree and re-ran `ui_schema_test.ts`: - drop `|| f.options` from `checkFormBlock:266` → `expected "options on a file" to be rejected` - drop `f.multiple !== undefined` from `:275` → `expected "multiple on a text field" to be rejected` - add `%` to the literal class in the `source` regex → `expected "bare percent" to be rejected` So all three fail for the absence of their own rule and nothing else. Your point about the shape is the one that mattered and it holds: `formField` is a flat object with `options`, `accept` and `multiple` optional on every input, so a well-formed `options` reaches `checkFormBlock` and the rejection is the cross-field rule rather than the field schema. `%2F%2Fsrv%2Flog` accepted alongside `%`, `%2` and `%zz` rejected is the boundary, not just the widening — that was the gap. ## The bar `warnAt={101} criticalAt={101}` is right and `pct` is clamped to 100 in the component, so neither threshold is reachable by any value. Agreed on not giving `ProgressBar` a first-class "not a capacity meter" prop for one call site — the comment carries the reason, which is what a bare 101 needs. The number now appears once in the bar and once on the button, which reads correctly. ## Gates Clean worktree at `8308df1`: `deno fmt --check` 312 files clean, `deno lint` 194 files clean, `deno task check` clean, `deno task check:svelte` 0 errors across 5 packages, `deno task test` **451 passed / 0 failed**. Matches your numbers exactly, including the 450 + 1. Everything from the previous two rounds I verified at `555246d` and the delta here does not touch it. Merge once #31 lands and this is rebased onto main.
julian merged commit c62d5b6567 into feat/ui-access-denied-and-rate 2026-08-12 13:10:53 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
julian
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
julian
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
OpsDeck/core!35
No description provided.