feat(ui): the components an external module turned out to need #31
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/ui-access-denied-and-rate"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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
Select<select>popup is drawn by the browser, takescolor-schemeand 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.SplitButtonSelectin its attached form.LinkButtonBreadcrumbs/ / srv.DropZoneCompositionBarActionProgress+progress.tsRateEstimatoris EWMA over cumulative counters with a debounce;etaSeconds/formatRate/formatEtarefuse to answer rather than invent a figure.AccessDeniedIconNamesConfirmDialog.confirmPhraseSince the review
packages/module-builder/build-module.tsis gone from this branchand 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.
ConfirmDialog's focus effect: main gave it a latch so a failed action thatleaves 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.
oehring@ngenn.net, which is not an identity on the signing key, so Forgejoshowed 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:
RateEstimatorusedlastMs === 0as "nothing seen yet", so a caller timingfrom 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.
{#each}blocks would throw on a duplicate key that only thecaller's good manners prevented. Neither list reorders; the key bought
nothing and could take the page down.
<form>was a submit button.Verification
deno task checkclean,deno fmt --checkclean, 40 UI tests pass. Behaviourchecked in a browser against a running server with the filebrowser module
loaded as a real external clone: the root
Selectopens, ticks the currentroot, 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.
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.**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.Scope of this review:
packages/module-builder/build-module.tsonly. 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):A string short-circuits the entire block, including the
TSConfckParseErrorrethrow — 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 readVite forwards eleven meaningful compiler options to esbuild. Through
extends, module builds have been gettingtarget: "ESNext"andverbatimModuleSyntax: true. After this change they get neither, and that lands on the built-in modules too — this is not only an external-module change.target: ESNextflips esbuild'suseDefineForClassFieldsfrom true to false. Class fields lower to constructor assignment instead ofObject.defineProperty.docker/frontend/job_follow.svelte.tshas 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.verbatimModuleSyntaxmeans the build now elides type-only imports thatcheck:sveltestill requiresimport typefor. 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:
"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 writetsconfigRaw: { compilerOptions: { ... } }, which reads identically at the call site and re-breaks every external module.A temp directory with a
tsconfig.jsonwhoseextendscannot resolve, a trivial entry file, and an assertion thatbuildModuleFrontendresolves would pin the exact property being bought here.3.
docs/modules.mdsays nothing about tsconfigModule 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
pathsortargetfails to affect the output will file it as a bug. A paragraph in the authoring guide.Checked, not problems
.sveltecomponents 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.tsfiles, and this covers all of them.svelte.config.jshazard. The builder subprocess sets nocwd, 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.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: "{}" },"{}"also dropstarget: ESNextandverbatimModuleSyntax: true, which module builds have been inheriting throughextends. That flipsuseDefineForClassFieldsto 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:
22d8ead01105d68a2ce9All 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:
You were right that
"{}"was esbuild's defaults wearing the sentence "thebuilder decides".
Tests, in #32. The config is now separable from running it
(
moduleBuildConfig), so one test reads the decisions without a build. Threein total: the options are what we say and are a string; a module whose
tsconfig cannot resolve its
extendsstill builds; the import contract stillrejects a bare specifier.
Checked they fail for the right reason rather than merely passing — rewriting
the value as
tsconfigRaw: { compilerOptions: … }, exactly the spelling youpredicted, fails both new tests with the original
TSConfckParseError: failed to resolve "extends".They run as
deno task test:builder, added toci. Separate task becauseimporting vite reads
os.release()and esbuild spawns its service binary, soit is the one place needing unrestricted
--allow-run;teststays narrow andnow 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 focuseffect, 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 readas unverified. Same content, re-signed under
tobias@thisilike.xyz.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#startedflag replaces the zero sentinel, both duplicate-key{#each}blocks are gone, the breadcrumb button has itstype, 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 withconfirmPhrase: 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 tovariant === "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,
!lockedalready guards the branch and Enter correctly does nothing.Fix: when focus is in
phraseElandlockedis false, confirm; never cancel from inside the field.2. A busy dialog leaks Tab into the page
ConfirmDialog.svelte:140. No phrase,busyset: both buttons disabled,stopsfilters to empty, and the earlyreturnskipspreventDefault. 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. Movee.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:183and:191. The same class of bug this PR fixes for Breadcrumbs in05d68a2. 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}onoption.valueis 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 theoptionsprop.Checked, not problems
Select's empty-options modulo (NaN) is unreachable:onMenuKeyhangs off theul, 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.dragendnever firing for an Explorer-originated drag is the platform's limitation, not this code's.etaSeconds/transferredrefuse-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.
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 inphraseElandlockedis false, this should confirm; unmatched is already guarded by!locked.@ -128,3 +180,4 @@{/if}<div class="buttons"><buttonbind:this={cancelEl}Missing
type="button", here and on Confirm below — the same implicit-submit bug05d68a2fixes 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;Busy dialog, no phrase: both buttons disabled,
stopsis empty, and thisreturnskipspreventDefault— 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)}The one keyed
{#each}left, and it throws on duplicateoption.value. Fair contract for a select, but the other two blocks lost their keys for exactly this failure — worth one line on theoptionsprop 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`;Math.roundreaches 60 when the remainder is within 30s of a full hour:formatEta(7170)→ "1h 60m left".Math.floor, or carry into the hour.**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.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
!lockedguard 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/deleteruns in Activitywhere 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/rootrefusing/tmp/srv— the run existing at allis the evidence.)
2. Busy dialog leaked Tab.
e.preventDefault()moved above the emptycheck. A modal eats Tab even when it has nowhere to send it.
3.
type="button"on Cancel and Confirm. Same one-word fix the breadcrumbgot, one file over.
4. "1h 60m left". Rounds to whole minutes first and carries.
formatEtanow has assertions for 7170s (the sentence that could not exist), 3629s and
5400s.
5. Select's key. Kept, documented on the
optionsprop: values must beunique 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 checkclean,deno fmt --checkclean, 40 UI tests pass.Also: thanks for #32 — merging that first, as you suggested.
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:
confirmPhrase, open-URL action,fileinput kind, breadcrumbs): types inpackages/sdk/ui.ts, validation inui-schema.ts, and the web schema renderer.progressTopicfor 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.
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 c62d5b6is empty — #35 was stacked on803e0af, 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'sconfirmPhraseagainst this branch's dialog) is the composite that is here.The five fixes, verified in the code
ConfirmDialog.svelte:127—focused === phraseEl → onconfirm(), ahead of the danger branch, and the!lockedguard four lines up still means an unmatched phrase makes Enter do nothing. The rule the file states is the rule it now follows.:148—preventDefault()above the empty check. With no phrase andbusyset,stopsis empty and Tab is now eaten rather than handed to the page behind.type="button"on Cancel (:192) and Confirm (:201).progress.ts:136rounds to whole minutes first and carries;formatEta(7170)is "2h 0m left" and the three new assertions inrate_test.tspin it.Select's key. Documented on theoptionsprop, with the reason the other two blocks dropped theirs and this one keeps it.Gates
Clean worktree at
c62d5b6merged with current main (c816874, which now carries #32 — the merge is clean, anddocs/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 --check313 files clean ·deno lint195 files clean ·deno task checkclean ·deno task check:svelte0 errors ·deno task test451 passed / 0 failed ·deno task test:builder3 passed.Two nits, neither blocking
1. A
confirmPhrasethat interpolates to empty silently drops the gate.ActionsBlock.svelte:216passesinterpolate(pending.confirmPhrase, params), andresolve.ts:16substitutes""for a param the page does not have.ConfirmDialog.svelte:51reads""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 inpathproduces 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" andformatEta(59.7)is "60s left". Not reachable throughetaSeconds, which rounds — butformatEtais exported from@opsdeck/ui, andActionProgress'setaSecis 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/GateListanditemPercent/overallPercent, and is the natural home forActionProgressand theRateEstimator/etaSeconds/formatRate/formatEtaset 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 triedDownloadManager, 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.
"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.Both nits and the docs, in
992f5b9,203f62b,851cca5on top of the approvedc62d5b6— nothing else touched, so the tree you approved is still the tree, plus three commits.1. A
confirmPhrasethat 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 nonamewas "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.tsasresolveConfirmPhraserather than staying a helper insideActionsBlock, because a rule about when a safety gate disappears should be assertable.schema/resolve_test.tspins it: the ordinary fill, the empty interpolation with theinterpolate("{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
Denoand@std/assert.packages/shell/tsconfig.jsonalready 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: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.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.etaSecis a prop a module fills in.Docs. All three:
ActionProgress— the two densities, when each is right,onDensityto 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.openparagraph 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).{param}in aconfirmPhrase, since the fallback above is behaviour a module author can see.Gates, clean worktree at
851cca5:deno fmt --check313 files ·deno lint195 files ·deno task checkclean ·deno task check:svelte0 errors ·deno task test452 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.{param}names are never checked against the page that has to fill them #36Filed 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 livetopicone is fully silent) and the refuse-or-warn call left open for you. Re-requested your review here for the three commits.Re-reviewed the delta:
992f5b9,203f62b,851cca5on top of the approvedc62d5b6. 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 nonameinterpolating to"delete "(trailing space, untypeable) was the same defect pointed the other way, and the test pins that case explicitly. Theparams[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 ofActionsBlockintoresolve.tsis what made it assertable at all — the tsconfig exclusion it needed is honest (src/**/*_test.tsruns 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
formatEtanow 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'stotal % 60is 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
ActionProgresssection 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 ispush(bytes, nowMs). Semantically the docs are right — the estimator only ever uses deltas, and the#startedflag exists precisely so an elapsed-from-zero clock works — but a reader who hovers the method seesnowMsand 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:paramsis a plain object literal (router.svelte.ts:24), so a phrase naming{toString}or{constructor}finds an inherited function, counts as "resolved", and interpolates tofunction 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 ininterpolateitself.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
851cca5on my machine:deno task checkclean ·deno task check:svelte0 errors ·deno fmt --checkclean on the touched files · the two changed test files pass 8/8. The fulldeno task testshows 12 failures here, all environmental — DuckDB's native binding failsLoadLibraryExWunder 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
c62d5b6and said this can go in; still true. #36 is filed for the general param-validation gap and correctly left out of this PR. Merging.