feat(ui): the components an external module turned out to need #31

Merged
julian merged 14 commits from feat/ui-access-denied-and-rate into main 2026-08-15 22:15:49 +02:00
Owner

The component kit only — the builder fix that used to ride along is now #32,
which should land first.

Everything here came out of building the filebrowser module against this
library and finding it short. The rule was: if the module wants a control the
kit does not have, the control belongs in the kit — a second listbox, a second
breadcrumb, a second progress bar is how two parts of one product start
disagreeing about what a dropdown looks like.

What is new

Component Why it is here and not in a module
Select One listbox implementation. A native <select> popup is drawn by the browser, takes color-scheme and nothing else, and beside a designed control reads as a hole cut in the page. Drawing it means owing the platform's behaviour, so it is all there: arrows, Home/End, Enter/Space, Escape, Tab-dismiss, outside-pointerdown, scroll and resize dismissal, focus restored to the trigger.
SplitButton An action joined to the variant it will use, so "Download" and ".tar.gz" read as one control instead of a button beside a mystery dropdown. Composes Select in its attached form.
LinkButton A download has to be an anchor — that is what makes middle-click, copy-link and open-in-new-tab work. Button's shape on one.
Breadcrumbs Path plus drop targets. The separator belongs between segments, which only sounds obvious until a root that is itself a separator renders as / / srv.
DropZone The whole wrapped area is the target, and the overlay names where a drop lands. Resets in the capture phase, because a child that stops propagation leaves the overlay painted over the page forever.
CompositionBar "What a total is made of", as distinct from ProgressBar's "how far along".
ActionProgress + progress.ts One progress surface in two densities over the same numbers, so the compact line and the verbose panel cannot disagree. RateEstimator is EWMA over cumulative counters with a debounce; etaSeconds/formatRate/formatEta refuse to answer rather than invent a figure.
AccessDenied The nav is not filtered by role, so a viewer can reach a page whose every route answers 403. This says so in the sign-in screen's language and deliberately describes nothing about what is behind it.
Six IconNames folder, image, video, audio, archive, link — enough to label a file listing.
ConfirmDialog.confirmPhrase For the actions where "are you sure" is not a real question. A recursive delete gets answered yes by reflex; typing the name is the pause.

Since the review

  • Split. packages/module-builder/build-module.ts is gone from this branch
    and is #32 on its own, with the explicit compiler options, tests and docs
    that review asked for. Nothing here touches build behaviour for any module.
  • Rebased onto current main, so the conflict is resolved. It was in
    ConfirmDialog's focus effect: main gave it a latch so a failed action that
    leaves the dialog up no longer yanks focus back to the default button, and
    this branch made it phrase-aware. Both survive — the latch still fires once
    per open cycle, and what it focuses is the phrase field when one is required.
  • Commit identity fixed. The commits were authored as
    oehring@ngenn.net, which is not an identity on the signing key, so Forgejo
    showed them unverified. Same content, re-signed under tobias@thisilike.xyz.

Review before opening this

A read of the whole diff turned up four defects, fixed in the last commit:

  • RateEstimator used lastMs === 0 as "nothing seen yet", so a caller timing
    from zero — the natural clock for an upload — lost its first sample. Its test
    passed for the wrong reason and now feeds an elapsed clock explicitly.
  • Two keyed {#each} blocks would throw on a duplicate key that only the
    caller's good manners prevented. Neither list reorders; the key bought
    nothing and could take the page down.
  • A breadcrumb inside a <form> was a submit button.
  • The dialog's tab trap typed an element as both an input and a button.

Verification

deno task check clean, deno fmt --check clean, 40 UI tests pass. Behaviour
checked in a browser against a running server with the filebrowser module
loaded as a real external clone: the root Select opens, ticks the current
root, and switching loads that root's own data; the split Download control
shows its selected format; breadcrumbs, drop targets, the composition bar and
the compact progress line all render. After the rebase, both dialog paths were
re-checked — a plain delete focuses Cancel and Tab cycles the two buttons; a
recursive one focuses the phrase field, Confirm stays disabled, and the trap
cycles only the stops that can actually take focus.

The component kit only — the builder fix that used to ride along is now #32, which should land first. Everything here came out of building the filebrowser module against this library and finding it short. The rule was: if the module wants a control the kit does not have, the control belongs in the kit — a second listbox, a second breadcrumb, a second progress bar is how two parts of one product start disagreeing about what a dropdown looks like. ## What is new | Component | Why it is here and not in a module | |---|---| | `Select` | One listbox implementation. A native `<select>` popup is drawn by the browser, takes `color-scheme` and nothing else, and beside a designed control reads as a hole cut in the page. Drawing it means owing the platform's behaviour, so it is all there: arrows, Home/End, Enter/Space, Escape, Tab-dismiss, outside-pointerdown, scroll and resize dismissal, focus restored to the trigger. | | `SplitButton` | An action joined to the variant it will use, so "Download" and ".tar.gz" read as one control instead of a button beside a mystery dropdown. Composes `Select` in its attached form. | | `LinkButton` | A download has to be an anchor — that is what makes middle-click, copy-link and open-in-new-tab work. Button's shape on one. | | `Breadcrumbs` | Path plus drop targets. The separator belongs *between* segments, which only sounds obvious until a root that is itself a separator renders as `/ / srv`. | | `DropZone` | The whole wrapped area is the target, and the overlay names where a drop lands. Resets in the capture phase, because a child that stops propagation leaves the overlay painted over the page forever. | | `CompositionBar` | "What a total is made of", as distinct from ProgressBar's "how far along". | | `ActionProgress` + `progress.ts` | One progress surface in two densities over the same numbers, so the compact line and the verbose panel cannot disagree. `RateEstimator` is EWMA over cumulative counters with a debounce; `etaSeconds`/`formatRate`/`formatEta` refuse to answer rather than invent a figure. | | `AccessDenied` | The nav is not filtered by role, so a viewer can reach a page whose every route answers 403. This says so in the sign-in screen's language and deliberately describes nothing about what is behind it. | | Six `IconName`s | folder, image, video, audio, archive, link — enough to label a file listing. | | `ConfirmDialog.confirmPhrase` | For the actions where "are you sure" is not a real question. A recursive delete gets answered yes by reflex; typing the name is the pause. | ## Since the review - **Split.** `packages/module-builder/build-module.ts` is gone from this branch and is #32 on its own, with the explicit compiler options, tests and docs that review asked for. Nothing here touches build behaviour for any module. - **Rebased onto current main**, so the conflict is resolved. It was in `ConfirmDialog`'s focus effect: main gave it a latch so a failed action that leaves the dialog up no longer yanks focus back to the default button, and this branch made it phrase-aware. Both survive — the latch still fires once per open cycle, and what it focuses is the phrase field when one is required. - **Commit identity fixed.** The commits were authored as `oehring@ngenn.net`, which is not an identity on the signing key, so Forgejo showed them unverified. Same content, re-signed under `tobias@thisilike.xyz`. ## Review before opening this A read of the whole diff turned up four defects, fixed in the last commit: - `RateEstimator` used `lastMs === 0` as "nothing seen yet", so a caller timing from zero — the natural clock for an upload — lost its first sample. Its test passed for the wrong reason and now feeds an elapsed clock explicitly. - Two keyed `{#each}` blocks would throw on a duplicate key that only the caller's good manners prevented. Neither list reorders; the key bought nothing and could take the page down. - A breadcrumb inside a `<form>` was a submit button. - The dialog's tab trap typed an element as both an input and a button. ## Verification `deno task check` clean, `deno fmt --check` clean, 40 UI tests pass. Behaviour checked in a browser against a running server with the filebrowser module loaded as a real external clone: the root `Select` opens, ticks the current root, and switching loads that root's own data; the split Download control shows its selected format; breadcrumbs, drop targets, the composition bar and the compact progress line all render. After the rebase, both dialog paths were re-checked — a plain delete focuses Cancel and Tab cycles the two buttons; a recursive one focuses the phrase field, Confirm stays disabled, and the trap cycles only the stops that can actually take focus.
Both came out of building the filebrowser module, and neither is specific to
it — which is the test for whether something belongs in the kit rather than in
the module that needed it first.

**AccessDenied.** The module list is not filtered by role, so a viewer reaches
a page whose every route answers 403 and reads "HTTP 403: forbidden" over a
spinner. The person is not doing anything wrong; they simply do not have the
role, and the page should say so. It borrows the sign-in screen's language —
the radial wash, the rising card, the gradient mark — in the colour of a
refusal, and deliberately says nothing about what is behind it: a refusal that
describes the thing it refuses is a disclosure with a nicer font.

**ActionProgress.** Two presentations of one set of numbers. Compact is a
single line — title, bar, percentage, ETA — for ordinary work, because a page
should not become a progress bar with an application attached. Verbose is the
panel with per-entity rows, byte counts and a rate, for transfers, where a
stall is the thing people watch. One control moves between them; both read the
same props, so they cannot disagree. Typed structurally like StepList and
ProgressList: the caller's data does not have to BE a task run.

**RateEstimator, etaSeconds, formatRate, formatEta, transferred.** The
arithmetic under that, in progress.ts beside itemPercent. The rate is smoothed
over a short window because the instantaneous figure — bytes since the last
event over the time since the last event — swings between zero and absurd on
every chunk boundary, and an ETA computed from it is unreadable; two samples
are required before it reports anything, and a counter that restarts (the next
file) is not read as negative throughput. An ETA with no total or no rate is
omitted rather than invented, and `transferred` reports a total only when every
entity knows its own size, because a partial total makes the remaining bytes
look smaller than they are and the ETA with them.

formatRate delegates to formatBytes(n, true) rather than spelling a byte count
a second way; two spellings of one number is how two parts of a screen end up
disagreeing.
folder, image, video, audio, archive and link. One path each, the same 24-unit
grid and stroke as the rest, so they inherit weight and colour without any
special handling — adding an icon here is a name in the union and a path in the
record, which is the point of keeping them in one file.

No aliases: what maps a file's category onto one of these is the module that
classifies files, not this library. A kit that knows what "backups" means is a
kit that has opinions about somebody else's data model.
Four more, plus one existing dialog taught a trick. Each was written inside a
module first, which is the honest way to find out whether something is
reusable, and each is here because the answer was yes.

**Breadcrumbs** — path navigation, with optional drop handlers so an ancestor
can accept a drag ("move this up a level") without this component knowing what
is being dragged. The separator sits BETWEEN segments, which sounds obvious
until a root that is itself a separator renders as "/ / srv".

**DropZone** — files dropped onto a surface. The zone is the whole wrapped
area, because aiming at a strip in a small window is the part people miss, and
the overlay names the destination: a drop that guesses where it lands is a drop
people undo. Counts dragenter/dragleave depth, which is what distinguishes
leaving the zone from crossing one of its children.

**CompositionBar** — what a total is made of, as a bar and a legend. Distinct
from ProgressBar, which answers "how far along"; this answers "of what". Takes
its colours from the categorical series tokens so two charts on one dashboard
do not each invent a palette.

**LinkButton** — a download has to be an anchor for the browser to treat it as
one, and a real href is what makes middle-click and open-in-new-tab work.
Button's shape on an `<a>`.

**ConfirmDialog** gains `confirmPhrase` and a detail snippet, rather than a
second dialog growing beside it: a recursive delete needs the operator to type
the name, and needs room for the counts and paths that make the question
answerable. The phrase field takes focus, because where one is required that
field IS the task, and Enter stays inert until it matches.
Crumb and Segment were exported from their components, and `deno check` cannot
read a type out of a .svelte module — the same rule the index has stated all
along, which I walked straight past.
**Breadcrumbs drew no separator between the first two crumbs.** The rule was
`i > 1`, right only when the first crumb IS a separator, as "/" is for a path —
"Home / Settings" ran together. That assumption is now a prop the caller
states rather than something the component believes about everybody.

**ConfirmDialog lost its focus trap whenever a phrase was required.** Tab was
returned unhandled, so it walked out of a modal dialog into the page behind it.
The trap covers the dialog's own stops now, and skips disabled ones: `.focus()`
on a disabled button does nothing, so including the still-locked Confirm
stalled the cycle at Cancel instead of wrapping.

**DropZone could leave its overlay painted over the page.** A child that
handles its own drop and stops propagation — a directory row accepting an
upload — meant the zone's handler never ran, and a drop fires no dragleave to
balance the counter. It resets in the capture phase now, before any child sees
the event, and on `dragend` for a drag abandoned with Escape.

**SplitButton** joins an action to the variant it will use. It began as a
native `<select>`, on the reasoning that the platform already has keyboard
navigation, type-ahead and screen-reader semantics right — but a native popup
is drawn by the browser, takes `color-scheme` and nothing else, and cannot have
this application's surface, radius or accent. Beside a designed control it
reads as a hole in the page. So the menu is ours, and it owes the behaviour the
platform would have given: roving focus with the arrows, Home/End, Enter or
Space to choose, Escape to dismiss, Tab to leave, pointerdown elsewhere to
close, and the trigger takes focus back every time it closes.

RateEstimator gains `ageMs`, for a caller deciding whether its number is still
worth showing: a stalled transfer stops producing events, and an estimator
never fed again keeps reporting the rate from before the stall.
The styled menu was written inside SplitButton, which was fine until the next
plain dropdown needed it — a second copy of roving focus, Escape handling and
outside-click dismissal is exactly what this library exists to prevent.

`Select` now owns that behaviour and SplitButton composes it in an "attached"
appearance, where the border and radius belong to the parent. Standalone it is
an ordinary picker; attached it is the right-hand half of a split control.

Dismissal also covers scrolling now: an absolutely positioned menu does not
follow the page under it, so a scroll leaves it hanging beside nothing.
Vite loads the nearest tsconfig.json when it transforms a module's .ts files,
and an external module repository carrying one is not unusual — an editor
wants it, svelte-check needs it. If its `extends` resolves in the author's
checkout and not in /data/modules/src/<name>/, where a clone is actually
built, the build throws, the host records a frontend-build failure and the
module is disabled. A file that exists for tooling took down the module.

Passing tsconfigRaw as a STRING is what makes vite skip the lookup entirely;
an object still reads the file first and merges over it. The builder already
decides how a module is compiled — target, format, which specifiers are
external — so reading the module's opinion of that was never right either.

Found by deploying the filebrowser module the way modules are actually
deployed: cloned, rather than copied into a core checkout.
fix(ui): four things a read of the whole diff found
All checks were successful
Build and Deploy / verify (pull_request) Successful in 1m13s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m11s
22d8ead011
**A first sample lost to a sentinel.** RateEstimator treated `lastMs === 0` as
"nothing seen yet", so a caller timing from zero — which is the natural clock
for an upload — had its first sample read as the first one twice, and the
whole estimate arrived a sample late. A flag says the thing the sentinel was
standing in for. The test that covered this passed before the fix for the
wrong reason, so it now feeds an elapsed clock explicitly.

**Two keyed `{#each}` blocks that could throw.** Svelte raises on a duplicate
key, and neither the crumb values nor the segment labels are guaranteed unique
by anything but the caller's good manners — "Home / Home" in a breadcrumb, two
segments both called "other". Neither list reorders, so the key bought nothing
and could take the page down.

**A breadcrumb inside a form was a submit button.** The default type is
`submit`; every other button in this library says `type="button"`.

**A type that claimed an element was both an input and a button.** The tab
trap holds either, so the union is what it is, and findIndex removes the last
cast with it.
julian requested changes 2026-08-11 17:40:21 +02:00
Dismissed
julian left a comment

Scope of this review: packages/module-builder/build-module.ts only. The UI components are not reviewed here — see the last section for why I think they should not travel together.

The build fix is right

Verified against the pinned vite 7.3.6 in the deno cache (dist/node/chunks/config.js:6070):

let tsconfigRaw = options?.tsconfigRaw;
if (typeof tsconfigRaw !== "string") {
  ... loadTsconfigJsonForFile(filename, config) ... throw e;
}

A string short-circuits the entire block, including the TSConfckParseError rethrow — and that rethrow is the crash. An object would still read the file and merge over it, exactly as the description says.

The blast radius is structural rather than one module's bad luck. Every module tsconfig in this repo does "extends": "../../../tsconfig.svelte.json" and carries the comment "An external module repository would carry a file exactly like this one." That target does not exist in a clone at /data/modules/src/<name>/, so any module authored to our own documented convention took itself down at startup. One line, correct layer.

1. "{}" discards more than the file read

Vite forwards eleven meaningful compiler options to esbuild. Through extends, module builds have been getting target: "ESNext" and verbatimModuleSyntax: true. After this change they get neither, and that lands on the built-in modules too — this is not only an external-module change.

  • Dropping target: ESNext flips esbuild's useDefineForClassFields from true to false. Class fields lower to constructor assignment instead of Object.defineProperty. docker/frontend/job_follow.svelte.ts has classes with fields; they are private, so the emitted semantics are very likely equivalent — but nobody decided that, it fell out of an empty object.
  • Dropping verbatimModuleSyntax means the build now elides type-only imports that check:svelte still requires import type for. The drift is in the safe direction (the checker is stricter than the build), but the build and the type check no longer assert the same language, which is the sort of gap this repo otherwise closes deliberately.

Keeps the property that matters — still a string, still skips the lookup — while making the choice explicit:

esbuild: {
  tsconfigRaw:
    '{"compilerOptions":{"target":"ESNext","verbatimModuleSyntax":true,"useDefineForClassFields":true}}',
},

"How a module is compiled is the builder's decision" is the right principle, and this is what stating the decision looks like. "{}" states esbuild's defaults instead.

2. No test, and the regression is silent

packages/module-builder/ has no tests, and the failure mode is the module is disabled at startup — a notification, not a build failure anyone is watching. The next person who wants one compiler option will write tsconfigRaw: { compilerOptions: { ... } }, which reads identically at the call site and re-breaks every external module.

A temp directory with a tsconfig.json whose extends cannot resolve, a trivial entry file, and an assertion that buildModuleFrontend resolves would pin the exact property being bought here.

3. docs/modules.md says nothing about tsconfig

Module authors now have a file that governs svelte-check and not the build. That is a reasonable split, but it has to be written down, or the first author whose paths or target fails to affect the output will file it as a bug. A paragraph in the authoring guide.

Checked, not problems

  • .svelte components are unaffected: vite's esbuild plugin only matches /\.(m?ts|[jt]sx)$/, and the Svelte 5 compiler strips template TypeScript itself. So the breakage was confined to a module's .ts files, and this covers all of them.
  • There is no matching svelte.config.js hazard. The builder subprocess sets no cwd, so vite's root is the server's, never the clone, and a module's svelte config is never loaded. The tsconfig leaked in anyway because esbuild resolves it per-file, nearest-to-source — which is precisely why one of these was a bug and the other never was.
  • The shell and shared-singleton builds are correctly left alone; their tsconfig is in-repo and resolves.

Please rebase

The PR is currently not mergeable — main has moved since #27. Needs conflicts resolved before this can go in.

One more thing

These eight lines are the only part of ~1650 changed lines that alters production build behaviour for every module, built-in and external. It wants a different reviewer's attention than a component kit does, and it fixes a bug that is disabling a module in the field right now. I would land it on its own first and let the components follow.

**Scope of this review:** `packages/module-builder/build-module.ts` only. The UI components are not reviewed here — see the last section for why I think they should not travel together. ## The build fix is right Verified against the pinned vite 7.3.6 in the deno cache (`dist/node/chunks/config.js:6070`): ```js let tsconfigRaw = options?.tsconfigRaw; if (typeof tsconfigRaw !== "string") { ... loadTsconfigJsonForFile(filename, config) ... throw e; } ``` A string short-circuits the entire block, including the `TSConfckParseError` rethrow — and that rethrow is the crash. An object would still read the file and merge over it, exactly as the description says. The blast radius is structural rather than one module's bad luck. Every module tsconfig in this repo does `"extends": "../../../tsconfig.svelte.json"` and carries the comment *"An external module repository would carry a file exactly like this one."* That target does not exist in a clone at `/data/modules/src/<name>/`, so any module authored to our own documented convention took itself down at startup. One line, correct layer. ## 1. `"{}"` discards more than the file read Vite forwards eleven meaningful compiler options to esbuild. Through `extends`, module builds have been getting `target: "ESNext"` and `verbatimModuleSyntax: true`. After this change they get neither, and that lands on the built-in modules too — this is not only an external-module change. - Dropping `target: ESNext` flips esbuild's `useDefineForClassFields` from **true to false**. Class fields lower to constructor assignment instead of `Object.defineProperty`. `docker/frontend/job_follow.svelte.ts` has classes with fields; they are private, so the emitted semantics are very likely equivalent — but nobody decided that, it fell out of an empty object. - Dropping `verbatimModuleSyntax` means the build now elides type-only imports that `check:svelte` still requires `import type` for. The drift is in the safe direction (the checker is stricter than the build), but the build and the type check no longer assert the same language, which is the sort of gap this repo otherwise closes deliberately. Keeps the property that matters — still a string, still skips the lookup — while making the choice explicit: ```ts esbuild: { tsconfigRaw: '{"compilerOptions":{"target":"ESNext","verbatimModuleSyntax":true,"useDefineForClassFields":true}}', }, ``` "How a module is compiled is the builder's decision" is the right principle, and this is what stating the decision looks like. `"{}"` states esbuild's defaults instead. ## 2. No test, and the regression is silent `packages/module-builder/` has no tests, and the failure mode is *the module is disabled at startup* — a notification, not a build failure anyone is watching. The next person who wants one compiler option will write `tsconfigRaw: { compilerOptions: { ... } }`, which reads identically at the call site and re-breaks every external module. A temp directory with a `tsconfig.json` whose `extends` cannot resolve, a trivial entry file, and an assertion that `buildModuleFrontend` resolves would pin the exact property being bought here. ## 3. `docs/modules.md` says nothing about tsconfig Module authors now have a file that governs svelte-check and not the build. That is a reasonable split, but it has to be written down, or the first author whose `paths` or `target` fails to affect the output will file it as a bug. A paragraph in the authoring guide. ## Checked, not problems - `.svelte` components are unaffected: vite's esbuild plugin only matches `/\.(m?ts|[jt]sx)$/`, and the Svelte 5 compiler strips template TypeScript itself. So the breakage was confined to a module's `.ts` files, and this covers all of them. - There is no matching `svelte.config.js` hazard. The builder subprocess sets no `cwd`, so vite's root is the server's, never the clone, and a module's svelte config is never loaded. The tsconfig leaked in anyway because esbuild resolves it per-file, nearest-to-source — which is precisely why one of these was a bug and the other never was. - The shell and shared-singleton builds are correctly left alone; their tsconfig is in-repo and resolves. ## Please rebase The PR is currently not mergeable — main has moved since #27. Needs conflicts resolved before this can go in. ## One more thing These eight lines are the only part of ~1650 changed lines that alters production build behaviour for every module, built-in and external. It wants a different reviewer's attention than a component kit does, and it fixes a bug that is disabling a module in the field right now. I would land it on its own first and let the components follow.
@ -48,0 +52,4 @@
// in the author's checkout but not in /data/modules/src fails the build
// and disables the module. How a module is compiled is the builder's
// decision anyway, not the module's.
esbuild: { tsconfigRaw: "{}" },
Owner

"{}" also drops target: ESNext and verbatimModuleSyntax: true, which module builds have been inheriting through extends. That flips useDefineForClassFields to false and lets the build elide type-only imports the checker still rejects — for built-in modules too, not just external ones.

Still a string, still skips the lookup, but says what it wants:

esbuild: {
  tsconfigRaw:
    '{"compilerOptions":{"target":"ESNext","verbatimModuleSyntax":true,"useDefineForClassFields":true}}',
},
`"{}"` also drops `target: ESNext` and `verbatimModuleSyntax: true`, which module builds have been inheriting through `extends`. That flips `useDefineForClassFields` to false and lets the build elide type-only imports the checker still rejects — for built-in modules too, not just external ones. Still a string, still skips the lookup, but says what it wants: ```ts esbuild: { tsconfigRaw: '{"compilerOptions":{"target":"ESNext","verbatimModuleSyntax":true,"useDefineForClassFields":true}}', }, ```
thisilike force-pushed feat/ui-access-denied-and-rate from 22d8ead011
All checks were successful
Build and Deploy / verify (pull_request) Successful in 1m13s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m11s
to 05d68a2ce9
All checks were successful
Build and Deploy / verify (pull_request) Successful in 1m17s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m10s
2026-08-11 19:23:45 +02:00
Compare
Author
Owner

All four addressed. The builder change left this branch entirely — it is #32,
based on current main and mergeable — so this PR is the component kit and
nothing else. Force-pushed, hence your review shows against gone commits.

Compiler options, in #32. Taken as written:

esbuild: {
  tsconfigRaw:
    '{"compilerOptions":{"target":"ESNext","verbatimModuleSyntax":true,"useDefineForClassFields":true}}',
},

You were right that "{}" was esbuild's defaults wearing the sentence "the
builder decides".

Tests, in #32. The config is now separable from running it
(moduleBuildConfig), so one test reads the decisions without a build. Three
in total: the options are what we say and are a string; a module whose
tsconfig cannot resolve its extends still builds; the import contract still
rejects a bare specifier.

Checked they fail for the right reason rather than merely passing — rewriting
the value as tsconfigRaw: { compilerOptions: … }, exactly the spelling you
predicted, fails both new tests with the original
TSConfckParseError: failed to resolve "extends".

They run as deno task test:builder, added to ci. Separate task because
importing vite reads os.release() and esbuild spawns its service binary, so
it is the one place needing unrestricted --allow-run; test stays narrow and
now ignores the package.

Docs, in #32. "Your tsconfig.json governs your editor, not the build"
that nothing in it reaches the output, why that is what keeps a module
loadable, and the two safe ways to keep one.

Rebase, here. Onto current main; the conflict was ConfirmDialog's focus
effect, where your latch from #27 met this branch's phrase-aware target. Both
survive: the latch still fires once per open cycle, and what it focuses is the
phrase field when a phrase is required. Re-checked both paths in a browser
after — a plain delete focuses Cancel and Tab cycles two buttons; a recursive
one focuses the phrase field, Confirm stays disabled, and the trap cycles only
the stops that can take focus.

One more thing that changed while rebasing: these commits were authored as
oehring@ngenn.net, which is not an identity on the signing key, so they read
as unverified. Same content, re-signed under tobias@thisilike.xyz.

All four addressed. The builder change left this branch entirely — it is #32, based on current main and mergeable — so this PR is the component kit and nothing else. Force-pushed, hence your review shows against gone commits. **Compiler options, in #32.** Taken as written: ```ts esbuild: { tsconfigRaw: '{"compilerOptions":{"target":"ESNext","verbatimModuleSyntax":true,"useDefineForClassFields":true}}', }, ``` You were right that `"{}"` was esbuild's defaults wearing the sentence "the builder decides". **Tests, in #32.** The config is now separable from running it (`moduleBuildConfig`), so one test reads the decisions without a build. Three in total: the options are what we say and are a string; a module whose tsconfig cannot resolve its `extends` still builds; the import contract still rejects a bare specifier. Checked they fail for the right reason rather than merely passing — rewriting the value as `tsconfigRaw: { compilerOptions: … }`, exactly the spelling you predicted, fails both new tests with the original `TSConfckParseError: failed to resolve "extends"`. They run as `deno task test:builder`, added to `ci`. Separate task because importing vite reads `os.release()` and esbuild spawns its service binary, so it is the one place needing unrestricted `--allow-run`; `test` stays narrow and now ignores the package. **Docs, in #32.** *"Your tsconfig.json governs your editor, not the build"* — that nothing in it reaches the output, why that is what keeps a module loadable, and the two safe ways to keep one. **Rebase, here.** Onto current main; the conflict was `ConfirmDialog`'s focus effect, where your latch from #27 met this branch's phrase-aware target. Both survive: the latch still fires once per open cycle, and what it focuses is the phrase field when a phrase is required. Re-checked both paths in a browser after — a plain delete focuses Cancel and Tab cycles two buttons; a recursive one focuses the phrase field, Confirm stays disabled, and the trap cycles only the stops that can take focus. One more thing that changed while rebasing: these commits were authored as `oehring@ngenn.net`, which is not an identity on the signing key, so they read as unverified. Same content, re-signed under `tobias@thisilike.xyz`.
julian requested changes 2026-08-11 22:56:00 +02:00
Dismissed
julian left a comment

Scope: the whole kit this time, against 05d68a2. The split is what I asked for and the four fixes from your own read all check out — I verified each rather than taking the list's word: the #started flag replaces the zero sentinel, both duplicate-key {#each} blocks are gone, the breadcrumb button has its type, the trap's union type is right. The rebase also holds: the latch is non-reactive, burns once per open cycle and only once a stop has actually taken focus, and the phrase field wins as the target. The busy-open path does what the comment promises.

Five things remain. Three are in ConfirmDialog, all small; two are nits I'd take in the same pass since you're in the file.

1. Enter in a matched phrase field cancels

ConfirmDialog.svelte:123. Danger variant with confirmPhrase: the operator types the full phrase — the deliberate act this feature exists to require — and presses Enter in the input. Focus is neither button, so the handler falls through to variant === "danger" → cancel(): dialog closes, typed phrase discarded.

That contradicts the file's own rule, stated four lines up: "Enter acts on what has FOCUS." Focus was in the phrase field; cancelling acts on nothing that had focus. It also inverts the established type-to-confirm convention (GitHub's repo delete, for one): once the phrase matches, Enter in the field is the confirmation. Unmatched, !locked already guards the branch and Enter correctly does nothing.

Fix: when focus is in phraseEl and locked is false, confirm; never cancel from inside the field.

2. A busy dialog leaks Tab into the page

ConfirmDialog.svelte:140. No phrase, busy set: both buttons disabled, stops filters to empty, and the early return skips preventDefault. Tab walks out of a modal into the page behind it — the exact hole the comment above the trap says was closed for the phrase case. Move e.preventDefault() above the empty check: a modal eats Tab even when it has nowhere to send it.

3. The dialog's own buttons lack type="button"

ConfirmDialog.svelte:183 and :191. The same class of bug this PR fixes for Breadcrumbs in 05d68a2. The dialog renders inline where it is used; inside a form, Cancel and Confirm are both submits. Same one-word fix as the breadcrumb got.

4. "1h 60m left"

progress.ts:134. Math.round((seconds % 3600) / 60) reaches 60 when the remainder is within 30 seconds of a full hour — formatEta(7170) is "1h 60m left". Math.floor, or carry into the hour.

5. Duplicate option values throw

Select.svelte:156. The keyed {#each} on option.value is the one keyed block left, and it throws on a duplicate value. Unique values are a fair contract for a select — but the other two blocks lost their keys for exactly this failure, so this one keeping its key is now a decision worth one line of documentation on the options prop.

Checked, not problems

  • Select's empty-options modulo (NaN) is unreachable: onMenuKey hangs off the ul, an empty menu has no focusable item, so focus never enters it — and even reached, items[NaN]?.focus() is a no-op. No change asked.
  • Select's listener add/remove is symmetric, focus restore on every close path is right.
  • DropZone: depth counter clamps at zero, capture-phase reset does what its comment claims. dragend never firing for an Explorer-originated drag is the platform's limitation, not this code's.
  • etaSeconds/transferred refuse-to-invent contracts hold; the EWMA and its debounce are correct.

All five are one sitting in two files. The shape of this is right.

**Scope:** the whole kit this time, against `05d68a2`. The split is what I asked for and the four fixes from your own read all check out — I verified each rather than taking the list's word: the `#started` flag replaces the zero sentinel, both duplicate-key `{#each}` blocks are gone, the breadcrumb button has its `type`, the trap's union type is right. The rebase also holds: the latch is non-reactive, burns once per open cycle and only once a stop has actually taken focus, and the phrase field wins as the target. The busy-open path does what the comment promises. Five things remain. Three are in `ConfirmDialog`, all small; two are nits I'd take in the same pass since you're in the file. ## 1. Enter in a matched phrase field cancels `ConfirmDialog.svelte:123`. Danger variant with `confirmPhrase`: the operator types the full phrase — the deliberate act this feature exists to require — and presses Enter in the input. Focus is neither button, so the handler falls through to `variant === "danger" → cancel()`: dialog closes, typed phrase discarded. That contradicts the file's own rule, stated four lines up: *"Enter acts on what has FOCUS."* Focus was in the phrase field; cancelling acts on nothing that had focus. It also inverts the established type-to-confirm convention (GitHub's repo delete, for one): once the phrase matches, Enter in the field is the confirmation. Unmatched, `!locked` already guards the branch and Enter correctly does nothing. Fix: when focus is in `phraseEl` and `locked` is false, confirm; never cancel from inside the field. ## 2. A busy dialog leaks Tab into the page `ConfirmDialog.svelte:140`. No phrase, `busy` set: both buttons disabled, `stops` filters to empty, and the early `return` skips `preventDefault`. Tab walks out of a modal into the page behind it — the exact hole the comment above the trap says was closed for the phrase case. Move `e.preventDefault()` above the empty check: a modal eats Tab even when it has nowhere to send it. ## 3. The dialog's own buttons lack `type="button"` `ConfirmDialog.svelte:183` and `:191`. The same class of bug this PR fixes for Breadcrumbs in `05d68a2`. The dialog renders inline where it is used; inside a form, Cancel and Confirm are both submits. Same one-word fix as the breadcrumb got. ## 4. "1h 60m left" `progress.ts:134`. `Math.round((seconds % 3600) / 60)` reaches 60 when the remainder is within 30 seconds of a full hour — `formatEta(7170)` is "1h 60m left". `Math.floor`, or carry into the hour. ## 5. Duplicate option values throw `Select.svelte:156`. The keyed `{#each}` on `option.value` is the one keyed block left, and it throws on a duplicate value. Unique values are a fair contract for a select — but the other two blocks lost their keys for exactly this failure, so this one keeping its key is now a decision worth one line of documentation on the `options` prop. ## Checked, not problems - `Select`'s empty-options modulo (`NaN`) is unreachable: `onMenuKey` hangs off the `ul`, an empty menu has no focusable item, so focus never enters it — and even reached, `items[NaN]?.focus()` is a no-op. No change asked. - `Select`'s listener add/remove is symmetric, focus restore on every close path is right. - `DropZone`: depth counter clamps at zero, capture-phase reset does what its comment claims. `dragend` never firing for an Explorer-originated drag is the platform's limitation, not this code's. - `etaSeconds`/`transferred` refuse-to-invent contracts hold; the EWMA and its debounce are correct. All five are one sitting in two files. The shape of this is right.
Owner

Danger + matched phrase + Enter in the input lands here and cancels: focus is neither button, so the fall-through runs cancel(), closing the dialog and discarding the phrase the operator just typed. Contradicts "Enter acts on what has FOCUS" — focus was in the field. When focus is in phraseEl and locked is false, this should confirm; unmatched is already guarded by !locked.

Danger + matched phrase + Enter in the input lands here and cancels: focus is neither button, so the fall-through runs `cancel()`, closing the dialog and discarding the phrase the operator just typed. Contradicts "Enter acts on what has FOCUS" — focus was in the field. When focus is in `phraseEl` and `locked` is false, this should confirm; unmatched is already guarded by `!locked`.
@ -128,3 +180,4 @@
{/if}
<div class="buttons">
<button
bind:this={cancelEl}
Owner

Missing type="button", here and on Confirm below — the same implicit-submit bug 05d68a2 fixes for Breadcrumbs. Rendered inside a form, both buttons submit it.

Missing `type="button"`, here and on Confirm below — the same implicit-submit bug `05d68a2` fixes for Breadcrumbs. Rendered inside a form, both buttons submit it.
@ -103,0 +137,4 @@
(el): el is HTMLInputElement | HTMLButtonElement =>
el !== null && !el.disabled,
);
if (stops.length === 0) return;
Owner

Busy dialog, no phrase: both buttons disabled, stops is empty, and this return skips preventDefault — Tab walks out of the modal into the page behind it, the hole the comment above says was closed. e.preventDefault() belongs before this check: a modal eats Tab even with nowhere to send it.

Busy dialog, no phrase: both buttons disabled, `stops` is empty, and this `return` skips `preventDefault` — Tab walks out of the modal into the page behind it, the hole the comment above says was closed. `e.preventDefault()` belongs before this check: a modal eats Tab even with nowhere to send it.
@ -0,0 +153,4 @@
aria-label={label}
onkeydown={onMenuKey}
>
{#each options as option, i (option.value)}
Owner

The one keyed {#each} left, and it throws on duplicate option.value. Fair contract for a select, but the other two blocks lost their keys for exactly this failure — worth one line on the options prop saying values must be unique.

The one keyed `{#each}` left, and it throws on duplicate `option.value`. Fair contract for a select, but the other two blocks lost their keys for exactly this failure — worth one line on the `options` prop saying values must be unique.
@ -33,0 +131,4 @@
return s ? `${m}m ${s}s left` : `${m}m left`;
}
const h = Math.floor(seconds / 3600);
return `${h}h ${Math.round((seconds % 3600) / 60)}m left`;
Owner

Math.round reaches 60 when the remainder is within 30s of a full hour: formatEta(7170) → "1h 60m left". Math.floor, or carry into the hour.

`Math.round` reaches 60 when the remainder is within 30s of a full hour: `formatEta(7170)` → "1h 60m left". `Math.floor`, or carry into the hour.
fix(ui): five from review, three of them in the same dialog
All checks were successful
Build and Deploy / verify (pull_request) Successful in 1m16s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m10s
803e0afb43
**Enter in a matched phrase field cancelled.** The operator does the one
deliberate thing the feature asks for — types the phrase — presses Enter, and
a danger dialog threw the typing away and closed. The rule four lines above
says Enter acts on what has FOCUS, and focus was in the field, so cancelling
there acted on nothing. It confirms now; unmatched, the existing `!locked`
guard still means Enter does nothing.

**A busy dialog leaked Tab into the page.** No phrase and `busy` set leaves
both buttons disabled, so the stops list is empty and the early return skipped
`preventDefault` — Tab walked out of a modal into the page behind it, the same
hole the trap exists to close. A modal eats Tab even when it has nowhere to
send it.

**The dialog's own buttons had no `type`.** Rendered inline inside a form,
Cancel and Confirm were both submits — the bug this branch already fixed for
Breadcrumbs, one file over.

**"1h 60m left".** Rounding the minute remainder on its own reaches 60 within
half a minute of the hour. Rounds to whole minutes first and carries; the test
pins 7170s, which was the sentence that could not exist.

**Select's key.** Its `{#each}` is the last keyed block here, and a duplicate
value throws. That is the right contract for a picker — two options answering
to one value cannot be told apart — so it is written on the prop rather than
left as the odd one out.

Verified in a browser against a running server: with the phrase matched, Enter
in the field starts the delete run (two runs in Activity where the old code
produced none); unmatched, the dialog stays open; Escape still cancels.
Author
Owner

All five in 803e0af.

1. Enter in a matched phrase field cancelled. Fixed, and you were right
about which rule it broke — focus was in the field, so cancelling acted on
nothing that had focus. Enter there confirms now; unmatched, the existing
!locked guard still means it does nothing.

Verified in a browser rather than by reading: with the phrase matched, Enter in
the field starts the delete run — two filebrowser/delete runs in Activity
where the old code produced none. Unmatched, the dialog stays open. Escape
still cancels. (Both runs then failed delete-failed, which is my dev server's
--allow-write=data,/host/root refusing /tmp/srv — the run existing at all
is the evidence.)

2. Busy dialog leaked Tab. e.preventDefault() moved above the empty
check. A modal eats Tab even when it has nowhere to send it.

3. type="button" on Cancel and Confirm. Same one-word fix the breadcrumb
got, one file over.

4. "1h 60m left". Rounds to whole minutes first and carries. formatEta
now has assertions for 7170s (the sentence that could not exist), 3629s and
5400s.

5. Select's key. Kept, documented on the options prop: values must be
unique because two options answering to one value cannot be told apart, which
is why this block keeps a key where the lists dropped theirs.

deno task check clean, deno fmt --check clean, 40 UI tests pass.

Also: thanks for #32 — merging that first, as you suggested.

All five in `803e0af`. **1. Enter in a matched phrase field cancelled.** Fixed, and you were right about which rule it broke — focus was in the field, so cancelling acted on nothing that had focus. Enter there confirms now; unmatched, the existing `!locked` guard still means it does nothing. Verified in a browser rather than by reading: with the phrase matched, Enter in the field starts the delete run — two `filebrowser/delete` runs in Activity where the old code produced none. Unmatched, the dialog stays open. Escape still cancels. (Both runs then failed `delete-failed`, which is my dev server's `--allow-write=data,/host/root` refusing `/tmp/srv` — the run existing at all is the evidence.) **2. Busy dialog leaked Tab.** `e.preventDefault()` moved above the empty check. A modal eats Tab even when it has nowhere to send it. **3. `type="button"` on Cancel and Confirm.** Same one-word fix the breadcrumb got, one file over. **4. "1h 60m left".** Rounds to whole minutes first and carries. `formatEta` now has assertions for 7170s (the sentence that could not exist), 3629s and 5400s. **5. Select's key.** Kept, documented on the `options` prop: values must be unique because two options answering to one value cannot be told apart, which is why this block keeps a key where the lists dropped theirs. `deno task check` clean, `deno fmt --check` clean, 40 UI tests pass. Also: thanks for #32 — merging that first, as you suggested.
Owner

A review of this PR against the filebrowser module and the mobile app turned up gaps on the portable-UI side, now tracked as three issues:

  • #34 — portable UI schema additions (confirmPhrase, open-URL action, file input kind, breadcrumbs): types in packages/sdk/ui.ts, validation in ui-schema.ts, and the web schema renderer.
  • OpsDeck/mobile#1 — the Android renderer half of the same four additions.
  • OpsDeck/module_filebrowser#1 — module-side fixes: the roots page declares free-space fields its route never fills, and the scan/delete actions don't use progressTopic for live progress on mobile.

The kit in this PR is web-only by design, but it widens the gap between what the module can do on web and what it can do everywhere else — most concretely: this PR gives the web a typed-phrase confirmation for recursive delete while the same delete on Android stays a one-tap confirm.

I'm not merging this until all three are addressed, so the two surfaces move together rather than web running ahead again.

A review of this PR against the filebrowser module and the mobile app turned up gaps on the portable-UI side, now tracked as three issues: - **#34** — portable UI schema additions (`confirmPhrase`, open-URL action, `file` input kind, breadcrumbs): types in `packages/sdk/ui.ts`, validation in `ui-schema.ts`, and the web schema renderer. - **OpsDeck/mobile#1** — the Android renderer half of the same four additions. - **OpsDeck/module_filebrowser#1** — module-side fixes: the roots page declares free-space fields its route never fills, and the scan/delete actions don't use `progressTopic` for live progress on mobile. The kit in this PR is web-only by design, but it widens the gap between what the module can do on web and what it can do everywhere else — most concretely: this PR gives the web a typed-phrase confirmation for recursive delete while the same delete on Android stays a one-tap confirm. I'm not merging this until all three are addressed, so the two surfaces move together rather than web running ahead again.
feat(schema): the four additions the portable UI was missing
All checks were successful
Build and Deploy / verify (pull_request) Successful in 1m15s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m11s
555246d2be
Closes #34. The core half of OpsDeck/mobile#1: types, validation and the web
renderer for all four, because the portable schema is the whole module on a
phone and what it cannot say, a phone cannot do.

**`confirmPhrase` on an action.** A field inside the confirmation that the
operator must type before the button works. Validation refuses a phrase with
no `confirm` to live in — a client that ignores the field falls back to the
plain dialog, and without a `confirm` there would be no dialog at all.

**`open` on an action.** Module-relative, resolved against the module's own
authenticated routes, so serving bytes needs no public URL. An action now does
exactly one of three things — post, navigate, open — and validation enforces
the count.

**`parent` on a page.** A field rather than a breadcrumbs block: validation
strips an unknown field but REJECTS an unknown block, 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.

**`file` input + multipart.** A form holding one submits as
`multipart/form-data`, sent over XMLHttpRequest because fetch reports no upload
progress at all. Parts carry no type, so numbers and booleans arrive as text
and a list contributes one part per element rather than a joined string.

The web renderer's own gaps, found while wiring it: a block it did not
recognise crashed the page rather than being skipped, and the source pattern
rejected percent escapes, so a path carried in a query string could not be
written at all.

## What review changed

- **The branch carried the whole filebrowser module** — 34 files swept in by a
  `git add packages` over a directory staged for manual testing. It was
  unwired, so `check` and `check:svelte` could not see it while `test`, `fmt`
  and `lint` all failed on it. Gone; the tree is the schema change alone.
- **`open` without `download` navigated to a blank page.** The shell
  intercepts same-origin clicks for its router and `/api/mod/…` matches no
  route. `LinkButton` gains a `target`, and a non-download open uses it — the
  interceptor stands down for a target.
- **A confirmed `open` behaved differently from an unconfirmed one**, because
  `location.href` cannot carry `download`. Both paths now click the same
  detached anchor.
- **`parent.label` documented a default the renderer cannot produce** — a
  renderer holds one page, not the page list, so it cannot read a sibling's
  title. The contract says what actually happens.
- **A submitted file control kept its selection**, so the next submit sent the
  other fields with no bytes behind them. The controls are keyed on a counter
  bumped after a successful send.
- Tests for the rules the description claimed and did not cover: options on a
  file, `multiple` on a non-file, and the widened source pattern from both
  sides — `%2F` accepted, a bare, truncated or non-hex `%` rejected.

`data/` is excluded from the toolchain now. It is the runtime directory:
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
fmt rewrites them, lint reports them and their tests run against a CWD they
were never written for. CI never sees it, which is why it costs a local
afternoon instead.

Gate: `deno fmt --check`, `deno lint`, `deno task check`, `deno task
check:svelte` all clean; `deno task test` 450 passed.
test(schema): pin the three rules nothing was asserting
All checks were successful
Build and Deploy / verify (pull_request) Successful in 1m14s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m12s
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.
Merge pull request 'feat(schema): the four additions the portable UI was missing' (#35) from feat/schema-parity into feat/ui-access-denied-and-rate
All checks were successful
Build and Deploy / verify (pull_request) Successful in 1m16s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m9s
c62d5b6567
Reviewed-on: #35
Reviewed-by: Julian Imhof <julianimhof00@gmail.com>
julian approved these changes 2026-08-12 13:19:33 +02:00
Dismissed
julian left a comment

Re-reviewed c62d5b6. Two things are new since my last pass here: 803e0af, which is the five fixes and has never been reviewed, and the #35 merge. Both check out. Approving.

The merge added nothing

git diff 8308df1 c62d5b6 is empty — #35 was stacked on 803e0af, so merging it is a fast-forward in content and the tree is exactly the one I approved on #35. Nothing there needs a second reading, and the composite I reviewed on that PR (ActionsBlock's confirmPhrase against this branch's dialog) is the composite that is here.

The five fixes, verified in the code

  • Enter in a matched phrase field. ConfirmDialog.svelte:127focused === phraseEl → onconfirm(), ahead of the danger branch, and the !locked guard four lines up still means an unmatched phrase makes Enter do nothing. The rule the file states is the rule it now follows.
  • Tab out of a busy dialog. :148preventDefault() above the empty check. With no phrase and busy set, stops is empty and Tab is now eaten rather than handed to the page behind.
  • type="button" on Cancel (:192) and Confirm (:201).
  • "1h 60m left". progress.ts:136 rounds to whole minutes first and carries; formatEta(7170) is "2h 0m left" and the three new assertions in rate_test.ts pin it.
  • Select's key. Documented on the options prop, with the reason the other two blocks dropped theirs and this one keeps it.

Gates

Clean worktree at c62d5b6 merged with current main (c816874, which now carries #32 — the merge is clean, and docs/modules.md, which both branches edited, keeps #32's "Your tsconfig.json governs your editor, not the build" section and #35's portable-UI section intact):

deno fmt --check 313 files clean · deno lint 195 files clean · deno task check clean · deno task check:svelte 0 errors · deno task test 451 passed / 0 failed · deno task test:builder 3 passed.

Two nits, neither blocking

1. A confirmPhrase that interpolates to empty silently drops the gate. ActionsBlock.svelte:216 passes interpolate(pending.confirmPhrase, params), and resolve.ts:16 substitutes "" for a param the page does not have. ConfirmDialog.svelte:51 reads "" as "no phrase required", so a phrase naming a mistyped or undeclared param renders as an ordinary one-tap confirm — the brake is gone and the action still runs. Every other interpolation fails loudly (a bad param in path produces a 404 someone sees); this one fails into the weaker behaviour. Falling back to the literal template when the interpolation comes out empty keeps the gate on for the price of an odd-looking phrase. Nothing validates {param} names against the page's route either, which is the general version of the same gap and much larger than this PR.

2. formatEta's minute branch has the shape you just fixed one branch up. formatEta(119.7) is "1m 60s left" and formatEta(59.7) is "60s left". Not reachable through etaSeconds, which rounds — but formatEta is exported from @opsdeck/ui, and ActionProgress's etaSec is a prop a module fills in, so a module dividing bytes by rate itself lands there. Same round-first-and-carry as line 136.

While you are in the docs: the "Components for work that takes a while" section lists RunFeed / ProgressBar / GateList and itemPercent / overallPercent, and is the natural home for ActionProgress and the RateEstimator / etaSeconds / formatRate / formatEta set this PR adds — it is the one section about exactly this concern and it does not know they exist. Also, "a phone hands it to its download manager" is no longer what happens: OpsDeck/mobile#2 tried DownloadManager, found it does not inherit the app's cleartext allowance, and fetches through the app's own client instead.

On the condition I set for merging

My earlier comment said this would not land until the three cross-surface issues were addressed, so where they stand: #34 is in this branch as #35; OpsDeck/mobile#1 is closed by mobile#2, merged; OpsDeck/module_filebrowser#1 has PR #2 open, which is waiting on me rather than on you. Web and Android now carry the same four additions, which was the point — I will review the filebrowser PR next, and this can go in.

Re-reviewed `c62d5b6`. Two things are new since my last pass here: `803e0af`, which is the five fixes and has never been reviewed, and the #35 merge. Both check out. Approving. ## The merge added nothing `git diff 8308df1 c62d5b6` is empty — #35 was stacked on `803e0af`, so merging it is a fast-forward in content and the tree is exactly the one I approved on #35. Nothing there needs a second reading, and the composite I reviewed on that PR (ActionsBlock's `confirmPhrase` against this branch's dialog) is the composite that is here. ## The five fixes, verified in the code - **Enter in a matched phrase field.** `ConfirmDialog.svelte:127` — `focused === phraseEl → onconfirm()`, ahead of the danger branch, and the `!locked` guard four lines up still means an unmatched phrase makes Enter do nothing. The rule the file states is the rule it now follows. - **Tab out of a busy dialog.** `:148` — `preventDefault()` above the empty check. With no phrase and `busy` set, `stops` is empty and Tab is now eaten rather than handed to the page behind. - **`type="button"`** on Cancel (`:192`) and Confirm (`:201`). - **"1h 60m left".** `progress.ts:136` rounds to whole minutes first and carries; `formatEta(7170)` is "2h 0m left" and the three new assertions in `rate_test.ts` pin it. - **`Select`'s key.** Documented on the `options` prop, with the reason the other two blocks dropped theirs and this one keeps it. ## Gates Clean worktree at `c62d5b6` **merged with current main** (`c816874`, which now carries #32 — the merge is clean, and `docs/modules.md`, which both branches edited, keeps #32's "Your tsconfig.json governs your editor, not the build" section and #35's portable-UI section intact): `deno fmt --check` 313 files clean · `deno lint` 195 files clean · `deno task check` clean · `deno task check:svelte` 0 errors · `deno task test` **451 passed / 0 failed** · `deno task test:builder` 3 passed. ## Two nits, neither blocking **1. A `confirmPhrase` that interpolates to empty silently drops the gate.** `ActionsBlock.svelte:216` passes `interpolate(pending.confirmPhrase, params)`, and `resolve.ts:16` substitutes `""` for a param the page does not have. `ConfirmDialog.svelte:51` reads `""` as "no phrase required", so a phrase naming a mistyped or undeclared param renders as an ordinary one-tap confirm — the brake is gone and the action still runs. Every other interpolation fails loudly (a bad param in `path` produces a 404 someone sees); this one fails into the weaker behaviour. Falling back to the literal template when the interpolation comes out empty keeps the gate on for the price of an odd-looking phrase. Nothing validates `{param}` names against the page's route either, which is the general version of the same gap and much larger than this PR. **2. `formatEta`'s minute branch has the shape you just fixed one branch up.** `formatEta(119.7)` is "1m 60s left" and `formatEta(59.7)` is "60s left". Not reachable through `etaSeconds`, which rounds — but `formatEta` is exported from `@opsdeck/ui`, and `ActionProgress`'s `etaSec` is a prop a module fills in, so a module dividing bytes by rate itself lands there. Same round-first-and-carry as line 136. While you are in the docs: the "Components for work that takes a while" section lists `RunFeed` / `ProgressBar` / `GateList` and `itemPercent` / `overallPercent`, and is the natural home for `ActionProgress` and the `RateEstimator` / `etaSeconds` / `formatRate` / `formatEta` set this PR adds — it is the one section about exactly this concern and it does not know they exist. Also, "a phone hands it to its download manager" is no longer what happens: OpsDeck/mobile#2 tried `DownloadManager`, found it does not inherit the app's cleartext allowance, and fetches through the app's own client instead. ## On the condition I set for merging My earlier comment said this would not land until the three cross-surface issues were addressed, so where they stand: #34 is in this branch as #35; OpsDeck/mobile#1 is closed by mobile#2, merged; OpsDeck/module_filebrowser#1 has PR #2 open, which is waiting on me rather than on you. Web and Android now carry the same four additions, which was the point — I will review the filebrowser PR next, and this can go in.
`interpolate` substitutes "" for a param the page does not have, and an
empty phrase is how ConfirmDialog spells "no phrase required". A phrase
naming a mistyped or undeclared param therefore rendered as an ordinary
one-tap confirm — the brake gone, the action still running, on exactly
the operations chosen as too dangerous to answer by reflex. Every other
interpolation in the renderer fails loudly; this one failed into the
weaker behaviour.

An unresolved placeholder now keeps the literal template. Odd to look
at, still a phrase somebody has to type. A partial fill falls back too:
"delete " with a trailing space is a phrase nobody can type.

The rule lives in resolve.ts rather than the component so it can be
asserted; the shell's svelte tsconfig excludes its own tests for the
same reason it already excluded the ui package's — `Deno` and
`@std/assert` are not in that config's world.
The hour branch was fixed last round and the minute branch kept the
shape that was wrong: `formatEta(119.7)` was "1m 60s left" and
`formatEta(59.7)` was "60s left". Rounding a remainder on its own
reaches the next unit within half of it, at every scale.

Round to whole seconds first, then split. `etaSeconds` already hands
over whole seconds, but `formatEta` is exported from `@opsdeck/ui` and
`ActionProgress.etaSec` is a prop a module fills in, so a module
dividing bytes by a rate itself arrives here with the fraction on it.
docs(modules): the progress surface, and what Android actually does
All checks were successful
Build and Deploy / verify (pull_request) Successful in 1m34s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m3s
851cca5bec
"Components for work that takes a while" is the one section about
exactly this concern and it did not know `ActionProgress` or the
`RateEstimator` / `transferred` / `etaSeconds` / `formatRate` /
`formatEta` set exist. Added, with the refusal-to-invent contract
stated: a made-up ETA is worse than none, because nobody can tell it
from a real one.

Also: an `open` action is no longer handed to the system download
manager on Android — mobile#2 found it inherits neither the app's
session nor its cleartext allowance and fetches with the app's own
client instead. And a confirmPhrase may carry {param}, so say what
happens when the page cannot fill one.
Author
Owner

Both nits and the docs, in 992f5b9, 203f62b, 851cca5 on top of the approved c62d5b6 — nothing else touched, so the tree you approved is still the tree, plus three commits.

1. A confirmPhrase that interpolates to empty. Fixed, and taken slightly wider than the empty case: any unresolved {param} keeps the literal template, not just a phrase that comes out entirely empty. delete {name} with no name was "delete " — a phrase with a trailing space that nobody can type, so the gate was broken in the other direction. Both now render the template.

The rule moved to resolve.ts as resolveConfirmPhrase rather than staying a helper inside ActionsBlock, because a rule about when a safety gate disappears should be assertable. schema/resolve_test.ts pins it: the ordinary fill, the empty interpolation with the interpolate("{name}", {}) === "" step spelled out beside it so the test says why, the partial fill, a param present but empty, no phrase at all, and a phrase with no placeholders.

That test needed one line elsewhere: svelte-check sweeps the shell package by directory, so a Deno test file inside it fails on Deno and @std/assert. packages/shell/tsconfig.json already excluded the ui package's tests for exactly that reason; the exclusion now covers its own.

Verified in a browser, not only in the test — a temporary demo action with confirmPhrase: "delete {missing}" on a page that declares no such param, with demo's frontend dropped from its manifest so the page renders through the schema renderer:

  • Before the fix that action was a one-tap confirm.
  • Now: Type delete {missing} to confirm, the phrase field takes focus, Confirm stays disabled until the template is typed literally, and Enter in the matched field runs the action (the notification count goes to 1). Escape still cancels.
  • A phrase with no placeholder (delete demo) is unchanged.

Both temporary edits are reverted; the diff is the three commits and nothing from demo.

2. formatEta's minute branch. Rounds once, before splitting, at every scale — Math.round(seconds) first, then hours/minutes/seconds off the whole number. formatEta(119.7) is "2m left", formatEta(59.7) is "1m left", formatEta(3599.7) is "1h 0m left"; all three are asserted, alongside the hour cases from last round. The comment now says the general rule rather than describing the one branch it was written under, and names why an exported formatter has to hold it: ActionProgress.etaSec is a prop a module fills in.

Docs. All three:

  • "Components for work that takes a while" now covers ActionProgress — the two densities, when each is right, onDensity to render the switch — and the numbers behind it: RateEstimator, transferred, etaSeconds / formatRate / formatEta, with the refuse-to-invent contract stated as the contract rather than an implementation detail.
  • The open paragraph no longer says a phone hands it to its download manager: Android fetches with the app's own HTTP client, because the download manager runs outside the app and inherits neither its session nor its cleartext allowance (mobile#2).
  • One sentence on {param} in a confirmPhrase, since the fallback above is behaviour a module author can see.

Gates, clean worktree at 851cca5: deno fmt --check 313 files · deno lint 195 files · deno task check clean · deno task check:svelte 0 errors · deno task test 452 passed / 0 failed (451 + the new resolve test).

On your larger point — nothing validates {param} names against the page's route — agreed that it is the general version and bigger than this PR. Worth an issue; the fallback here means a bad name is now visible rather than silently weakening a confirmation, which is the part that could not wait.

Both nits and the docs, in `992f5b9`, `203f62b`, `851cca5` on top of the approved `c62d5b6` — nothing else touched, so the tree you approved is still the tree, plus three commits. **1. A `confirmPhrase` that interpolates to empty.** Fixed, and taken slightly wider than the empty case: any unresolved `{param}` keeps the literal template, not just a phrase that comes out entirely empty. `delete {name}` with no `name` was "delete " — a phrase with a trailing space that nobody can type, so the gate was broken in the other direction. Both now render the template. The rule moved to `resolve.ts` as `resolveConfirmPhrase` rather than staying a helper inside `ActionsBlock`, because a rule about when a safety gate disappears should be assertable. `schema/resolve_test.ts` pins it: the ordinary fill, the empty interpolation with the `interpolate("{name}", {}) === ""` step spelled out beside it so the test says *why*, the partial fill, a param present but empty, no phrase at all, and a phrase with no placeholders. That test needed one line elsewhere: svelte-check sweeps the shell package by directory, so a Deno test file inside it fails on `Deno` and `@std/assert`. `packages/shell/tsconfig.json` already excluded the ui package's tests for exactly that reason; the exclusion now covers its own. Verified in a browser, not only in the test — a temporary demo action with `confirmPhrase: "delete {missing}"` on a page that declares no such param, with demo's frontend dropped from its manifest so the page renders through the schema renderer: - **Before the fix** that action was a one-tap confirm. - **Now**: *Type `delete {missing}` to confirm*, the phrase field takes focus, Confirm stays disabled until the template is typed literally, and Enter in the matched field runs the action (the notification count goes to 1). Escape still cancels. - A phrase with no placeholder (`delete demo`) is unchanged. Both temporary edits are reverted; the diff is the three commits and nothing from demo. **2. `formatEta`'s minute branch.** Rounds once, before splitting, at every scale — `Math.round(seconds)` first, then hours/minutes/seconds off the whole number. `formatEta(119.7)` is "2m left", `formatEta(59.7)` is "1m left", `formatEta(3599.7)` is "1h 0m left"; all three are asserted, alongside the hour cases from last round. The comment now says the general rule rather than describing the one branch it was written under, and names why an exported formatter has to hold it: `ActionProgress.etaSec` is a prop a module fills in. **Docs.** All three: - "Components for work that takes a while" now covers `ActionProgress` — the two densities, when each is right, `onDensity` to render the switch — and the numbers behind it: `RateEstimator`, `transferred`, `etaSeconds` / `formatRate` / `formatEta`, with the refuse-to-invent contract stated as the contract rather than an implementation detail. - The `open` paragraph no longer says a phone hands it to its download manager: Android fetches with the app's own HTTP client, because the download manager runs outside the app and inherits neither its session nor its cleartext allowance (mobile#2). - One sentence on `{param}` in a `confirmPhrase`, since the fallback above is behaviour a module author can see. **Gates**, clean worktree at `851cca5`: `deno fmt --check` 313 files · `deno lint` 195 files · `deno task check` clean · `deno task check:svelte` 0 errors · `deno task test` **452 passed / 0 failed** (451 + the new resolve test). On your larger point — nothing validates `{param}` names against the page's route — agreed that it is the general version and bigger than this PR. Worth an issue; the fallback here means a bad name is now visible rather than silently weakening a confirmation, which is the part that could not wait.
Author
Owner

Filed the general version as #36 — the {param} names in a schema are never checked against the page that has to fill them, with the per-site failure modes (the live topic one is fully silent) and the refuse-or-warn call left open for you. Re-requested your review here for the three commits.

Filed the general version as #36 — the `{param}` names in a schema are never checked against the page that has to fill them, with the per-site failure modes (the live `topic` one is fully silent) and the refuse-or-warn call left open for you. Re-requested your review here for the three commits.
julian approved these changes 2026-08-15 22:09:54 +02:00
julian left a comment

Re-reviewed the delta: 992f5b9, 203f62b, 851cca5 on top of the approved c62d5b6. The diff is exactly the two nit fixes plus docs — 120 lines over 7 files, nothing else touched. Approving; two non-blocking nits below, both one-line and neither worth another round.

Nit 1, verified fixed — and wider than asked

resolveConfirmPhrase (resolve.ts:31) does what the comment promises: any template with an unresolved {param} keeps the literal template, so the gate can no longer vanish into a one-tap confirm. Taking it wider than the empty case was right — "delete {name}" with no name interpolating to "delete " (trailing space, untypeable) was the same defect pointed the other way, and the test pins that case explicitly. The params[k] === "" case counting as unresolved is also correct: a param present but empty is no more fillable than an absent one.

The test file is the right kind: interpolate("{name}", {}) === "" asserted beside the fallback so the test records why the rule exists. And moving the rule out of ActionsBlock into resolve.ts is what made it assertable at all — the tsconfig exclusion it needed is honest (src/**/*_test.ts runs under Deno, not svelte-check; I checked the tree — no pre-existing shell test files get silently unshadowed by it, this is the first).

Nit 2, verified fixed

formatEta now rounds to whole seconds once, up front, then splits. I checked every boundary rather than the three new assertions alone: 59.5 → "1m", 119.7 → "2m", 3599.7 → "1h 0m", 3629 → "1h 0m", 7170 → "2h 0m", and no branch can emit "60s" or "60m" — the minute branch's total % 60 is structurally under 60, and the hour branch carries. The comment now states the general rule and names why an exported formatter must hold it. Done.

Docs, verified against the code

Every name the new ActionProgress section drops is real: the props (title/percent/doneBytes/totalBytes/rate/etaSec/items/density/onDensity), overallPercent, transferred's only-when-every-entity-knows contract, RateEstimator's two-sample refusal and restart behaviour. The Android correction matches what mobile#2 actually built. The {param} fallback sentence matches the code it describes.

One naming slip: the docs say push(bytes, elapsedMs) but the signature is push(bytes, nowMs). Semantically the docs are right — the estimator only ever uses deltas, and the #started flag exists precisely so an elapsed-from-zero clock works — but a reader who hovers the method sees nowMs and will wonder which document is lying. Either name the parameter as the signature spells it, or say "any monotonic millisecond clock", which is the actual contract.

One edge I checked that the tests do not cover

resolveConfirmPhrase's !params[m[1]] reads through the prototype: params is a plain object literal (router.svelte.ts:24), so a phrase naming {toString} or {constructor} finds an inherited function, counts as "resolved", and interpolates to function toString() { [native code] }. The gate survives — the phrase is non-empty, merely absurd — so this is robustness, not a hole, and the same read-through has always been in interpolate itself. Object.hasOwn(params, m[1]) && params[m[1]] closes it for one line if you touch the file again; otherwise it is #36's problem, where it belongs.

Gates

Clean worktree at 851cca5 on my machine: deno task check clean · deno task check:svelte 0 errors · deno fmt --check clean on the touched files · the two changed test files pass 8/8. The full deno task test shows 12 failures here, all environmental — DuckDB's native binding fails LoadLibraryExW under Windows and the docker path tests are the known #20 territory — none touch this PR's files, and your 452/0 on a platform where the binding loads is the number that counts.

Where the condition stands

Of the three cross-surface issues I held this on: #34 landed here as #35, mobile#1 is closed by mobile#2 (merged), and filebrowser#1 is addressed by filebrowser#2 — open, "Closes #1", and waiting on my review, not on you. That was already the state when I approved c62d5b6 and said this can go in; still true. #36 is filed for the general param-validation gap and correctly left out of this PR. Merging.

Re-reviewed the delta: `992f5b9`, `203f62b`, `851cca5` on top of the approved `c62d5b6`. The diff is exactly the two nit fixes plus docs — 120 lines over 7 files, nothing else touched. Approving; two non-blocking nits below, both one-line and neither worth another round. ## Nit 1, verified fixed — and wider than asked `resolveConfirmPhrase` (`resolve.ts:31`) does what the comment promises: any template with an unresolved `{param}` keeps the literal template, so the gate can no longer vanish into a one-tap confirm. Taking it wider than the empty case was right — `"delete {name}"` with no `name` interpolating to `"delete "` (trailing space, untypeable) was the same defect pointed the other way, and the test pins that case explicitly. The `params[k] === ""` case counting as unresolved is also correct: a param present but empty is no more fillable than an absent one. The test file is the right kind: `interpolate("{name}", {}) === ""` asserted *beside* the fallback so the test records why the rule exists. And moving the rule out of `ActionsBlock` into `resolve.ts` is what made it assertable at all — the tsconfig exclusion it needed is honest (`src/**/*_test.ts` runs under Deno, not svelte-check; I checked the tree — no pre-existing shell test files get silently unshadowed by it, this is the first). ## Nit 2, verified fixed `formatEta` now rounds to whole seconds once, up front, then splits. I checked every boundary rather than the three new assertions alone: 59.5 → "1m", 119.7 → "2m", 3599.7 → "1h 0m", 3629 → "1h 0m", 7170 → "2h 0m", and no branch can emit "60s" or "60m" — the minute branch's `total % 60` is structurally under 60, and the hour branch carries. The comment now states the general rule and names why an exported formatter must hold it. Done. ## Docs, verified against the code Every name the new `ActionProgress` section drops is real: the props (`title`/`percent`/`doneBytes`/`totalBytes`/`rate`/`etaSec`/`items`/`density`/`onDensity`), `overallPercent`, `transferred`'s only-when-every-entity-knows contract, `RateEstimator`'s two-sample refusal and restart behaviour. The Android correction matches what mobile#2 actually built. The `{param}` fallback sentence matches the code it describes. One naming slip: the docs say `push(bytes, elapsedMs)` but the signature is `push(bytes, nowMs)`. Semantically the docs are *right* — the estimator only ever uses deltas, and the `#started` flag exists precisely so an elapsed-from-zero clock works — but a reader who hovers the method sees `nowMs` and will wonder which document is lying. Either name the parameter as the signature spells it, or say "any monotonic millisecond clock", which is the actual contract. ## One edge I checked that the tests do not cover `resolveConfirmPhrase`'s `!params[m[1]]` reads through the prototype: `params` is a plain object literal (`router.svelte.ts:24`), so a phrase naming `{toString}` or `{constructor}` finds an inherited function, counts as "resolved", and interpolates to `function toString() { [native code] }`. The gate survives — the phrase is non-empty, merely absurd — so this is robustness, not a hole, and the same read-through has always been in `interpolate` itself. `Object.hasOwn(params, m[1]) && params[m[1]]` closes it for one line if you touch the file again; otherwise it is #36's problem, where it belongs. ## Gates Clean worktree at `851cca5` on my machine: `deno task check` clean · `deno task check:svelte` 0 errors · `deno fmt --check` clean on the touched files · the two changed test files pass 8/8. The full `deno task test` shows 12 failures here, all environmental — DuckDB's native binding fails `LoadLibraryExW` under Windows and the docker path tests are the known #20 territory — none touch this PR's files, and your 452/0 on a platform where the binding loads is the number that counts. ## Where the condition stands Of the three cross-surface issues I held this on: #34 landed here as #35, mobile#1 is closed by mobile#2 (merged), and filebrowser#1 is addressed by filebrowser#2 — open, "Closes #1", and waiting on my review, not on you. That was already the state when I approved `c62d5b6` and said this can go in; still true. #36 is filed for the general param-validation gap and correctly left out of this PR. Merging.
julian merged commit fb29e2c206 into main 2026-08-15 22:15:49 +02:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No assignees
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!31
No description provided.