mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-11 13:28:57 +00:00
21dfb1152fa25d992989cf5f76bb67fefded4a11
802 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 xarmian
|
f2c14dca79 |
refactor: one write-door relation path, and neutral wording for two reasons (TASK-2878)
## The web round, which had never had one The TypeScript union and CopyItemDialog changed across three commits with only the Go-side parity gate looking at them. First review round on those files found two, both in the sentences a user actually reads: `not_found` said "the item it refers to no longer exists". That reason is ALSO what the server collapses a hidden target to — telling the two apart is the existence oracle the collapse exists to prevent — so asserting non-existence is wrong for half the cases and is a claim the response cannot support. Now "could not be found". `target_missing` said "the field declares no collection to link to". It is emitted for two causes: no target declared, AND a declared target that is not a collection in this workspace. The wording named only the first and misdiagnosed the second as a schema that says nothing when it says something broken. Now "has no valid collection to link to", and the store's own message for the same reason was over-claiming identically and is fixed with it. Neither is a code defect. Both are the write-up half of the same rule this unit keeps enforcing: say what is true, not what is convenient. ## The consolidation, per the lead's CONVE-139 ruling Create and full-`fields` update each spelled out the SAME four steps — resolve the whole map, keep only the caller's issues, resolve the defaults validation injected after the main pass, drop a default whose target the caller cannot see. Round 10 was the third time one of those steps had to be stated separately, which is what showed the shape. Extracted into `resolveRelationsForWrite`, called twice, with the rule written once above it. NOT extended to the migrate doors, and stated in the code rather than left to look like an omission: they reach the same rule through `store.MigrateRelationReferents`, which cannot call this — the visibility layer is request-scoped by construction and `store` cannot import `server`. Unifying the families needs a caller-supplied visibility predicate on the store API. That is IDEA-2886, filed, with the shape it would take. Gates: internal/server ok 359.4s · internal/store ok 365.5s · go vet clean · gofmt clean · make lint 0 issues · npm run check 0 errors. Postgres green on the parent commit: store 543.9s, server 298.4s. |
||
|
xarmian
|
c4d429d14c |
fix: three codex round-6 findings, one premise corrected (TASK-2878)
Round 6 confirmed round 5 and found three. All three are real; one arrived
with an account of its own cause that the test refuted, and the corrected
route is narrower than the report.
## A NON-STRING RELATION DEFAULT, AND WHERE IT ACTUALLY GETS IN
The finding: injected defaults are never type-checked, so `42` or `[]` can
persist in a relation field. True, but not by the route described.
`MigrateFields` injects destination defaults ITSELF, so in the ordinary case
the key is present when `ValidateFieldsDetailed` runs and its type IS
checked — a numeric default lands in needs_value with "must be a string",
which is correct behaviour. My first test asserted the wrong thing and
FAILED against the fixed build, which is how I found this out.
The unchecked route is narrower: a NULL OVERRIDE deletes the key after
MigrateFields filled it, so validation injects the default itself — and its
own injection branch `continue`s PAST the type check. That is the one way a
non-string reaches a relation field unchallenged. The late-default pass owns
values that arrive from defaults, so it reports this one:
`invalid_shape`, a new reason, because every existing reason describes a
lookup that never happened.
Retargeting the test then exposed a second defect IN MY OWN FIX: the late
pass's `if len(late) == 0 { return nil, nil }` discarded the non-string drops
it had just recorded. The value vanished from all three buckets. Green on the
first route, silent on the second.
THE PARITY GATE FROM AN EARLIER COMMIT CAUGHT THE NEW REASON: adding
`invalid_shape` failed TestCopyPreflightDropReasonsAreRenderedByTheDialog
until the TypeScript union and CopyItemDialog learned it. That gate exists
because `referent_not_portable` shipped unrendered in BUG-2674, and it just
did its job on its author.
## A WHITESPACE-ONLY VALUE IS "NO REFERENCE", NOT A BAD ONE
The store resolver trims and ignores `" "`. The server wrapper checked the
UNTRIMMED string, so the value fell through to the visibility loop — and
since round 1's vanished-target arm turns a missing lookup into a refusal,
`" "` came back as not_found instead of an empty field. A defect my own
round-1 fix introduced: before it, that path did `continue`.
## A MALFORMED CARRIED VALUE IS NOT "NOT PORTABLE"
The cross-workspace branch dropped every carried value without looking,
including non-strings, and labelled them `referent_not_portable` — a false
account of why the value is going. It is not a reference at all. Left in
place now for ValidateFields to reject on shape, which is what the
SAME-workspace branch already did with it: the two modes disagreeing about
one malformed value was the defect.
## Counterfactuals
Skip non-string defaults again -> TestCopyEndpoint_NonStringRelationDefault
DETECTED. Restore the untrimmed skip -> TestRelationDoors_WhitespaceOnly
DETECTED. Both build-checked, and the first form of the second mutant did NOT
build (unused import) — reported as such rather than scored, since a
non-compiling mutant produces no failures and reads as survived.
Gates: internal/server ok 324.8s · internal/store ok 344.5s · internal/mcp ok
16.9s · go vet clean · gofmt clean · make lint 0 issues · npm run check 0
errors. Postgres green on the parent commit (store 596.1s, server 299.4s);
re-running on this tree.
|
||
|
xarmian
|
34861f8658 |
feat(mcp): ToolSurfaceVersion 0.29, and the drop-reason renderer it exposed (TASK-2878)
PLAN-2857 U1. The bump, its documentation sweep, and the consumer this
change turned from a rare wart into a routine one.
THE BUMP, at 0.29 rather than 0.28. Rebasing onto main found
|
||
|
xarmian
|
a1716d8170 |
ci(web): decide the npm audit gate from the report, not the exit code, and run it last (BUG-2881) (#1247)
* ci(web): decide the npm audit gate from the report, not the exit code, and run it last (BUG-2881) `npm audit` exits non-zero identically for "a HIGH/CRITICAL advisory exists" and "the advisory service was unreachable". The Web job ran it before Build / Type check / vitest under `bash -e`, so a registry timeout (main, 03:50Z) and a 503 (#1246, 04:33Z) on 2026-09-04 each produced a red row with every frontend verification step SKIPPED — a lane that read like a failure and had asked nothing. scripts/ci-audit.mjs runs the audit in --json mode and decides from the report: metadata.vulnerabilities present → fail iff high+critical > 0, naming the advisories; an error envelope or unparseable output → a GitHub warning annotation saying the gate did not run, exit 0. The step moves to the end of the job so the frontend's own verdict always exists whatever the audit does. Verified locally against five report shapes (transport timeout envelope, E503 envelope, one high advisory, clean, garbage) and two live runs (the real registry: clean; a dead registry: warning, exit 0). `--input <file>` is the seam those checks use. Fixes BUG-2881 * ci(web): the audit gate fails closed — retry an unreachable advisory service, then fail under its own title Codex round 1 on #1247: the first draft warned and exited 0 when the advisory service could not be asked, which made the only supply-chain gate pass exactly when it had not run. A gate that passes when it cannot run is not a gate. Now: up to three attempts with backoff (registry blips are usually seconds long), then `::error title=npm audit did not run` and exit 1. The title is distinct from `::error title=npm audit` (a real advisory) so the checks tab tells the two apart without opening the log; re-running is the remedy for the first and never for the second. Because the step runs last, Build / Type check / vitest have already produced their result either way — the original blindness is gone regardless of which way this step fails. Verified against the same five saved shapes (transport and E503 envelopes and garbage now exit 1 under the did-not-run title; a high advisory exits 1 under the advisory title; clean exits 0) and two live runs (real registry: clean; dead registry: three attempts logged, exit 1). Refs BUG-2881 * ci(web): the audit gate refuses counts it cannot read, and refuses bad tuning without crashing Codex round 2 on #1247. (1) metadata.vulnerabilities was checked for presence, not for shape: Number("x") + Number(null) > 0 is false, so a malformed count read as a clean audit — a second fail-open, one layer deeper than round 1's. high/critical must now be non-negative integers or the report is unreadable, which is the fail-closed path. (2) The two env knobs are operator-set, but CI_AUDIT_ATTEMPTS=NaN left the retry loop unexecuted and threw a TypeError, and CI_AUDIT_BACKOFF_MS=Infinity parked Atomics.wait forever; both now fall back to the default with a line saying so. Refs BUG-2881 * build: the local preflight runs the same audit gate CI does, and runs it last Codex round 3 on #1247 (blast radius): `make web-check` still chained bare `npm audit && npm run check`, so a registry blip stopped svelte-check locally exactly as it had in CI, and CONTRIBUTING documented the bare command as the way to reproduce the gate. New `web-audit` target runs `npm run audit:ci`; `check` runs it after web-check and web-test, mirroring the Web job's order. CONTRIBUTING and docs/architecture.md say so. Refs BUG-2881 * build: web-audit stands alone — no `web` prerequisite, so `check` runs npm ci once and no new target reaches it Codex round 4 on #1247: `web-audit: web` made `check` run `npm ci` twice (`web` is .PHONY) and added a target CLAUDE.md's worktree rule did not list as reaching `npm ci`. `npm audit` reads the lockfile and needs neither node_modules nor a build — verified by running it with node_modules removed — so the prerequisite goes; CLAUDE.md's safe list gains `web-audit`. Refs BUG-2881 |
||
|
xarmian
|
552230bbea |
fix(web): a cleared picker owes a refresh even on an unchanged scope (TASK-2877)
Codex review round 13, and it collapses round 12's fix into a simpler one. `lastScope` means "the scope the rows on screen answer for". The not-ready branch REMOVES those rows, so afterwards they answer for nothing — which is what null says, and the next run therefore owes a refresh whether or not the scope itself moved. Leaving the old value there meant rehydrating on the SAME workspace and collection compared equal, so a server-sourced picker took the early return and sat empty permanently: its rows were cleared and nothing was left to re-query it. Round 12 deferred the COMMIT past the early return to keep a cold-window scope change from being forgotten. With this invalidation in place that deferral changed no outcome — its mutant could not be killed — so it went and the commit moved back to where the value is computed. One rule stated once, rather than two mechanisms aimed at two halves of it. Also hardened the mutation harness, after it bit: a harness timeout kills the runner with SIGTERM, which does not run `finally`, so an earlier killed run left the working tree MUTATED. I then read a pre-existing test "failing" in that tree and had a plausible defect and a fix half-written before checking the file — the failure was M31's mutant, not my change. The runner now restores from its backups on SIGTERM/SIGINT/SIGHUP. Cheap, and the alternative is reasoning about code nobody wrote. Matrix: 35 mutants, all killed; baseline and restore both 97/97. |
||
|
xarmian
|
60fe815300 |
fix(web): a scope refresh stays owed until a run serves it (TASK-2877)
Codex review round 12, and the tail of round 11's fix. `lastScope` was committed as soon as the effect computed it, before the not-ready branch — which clears the picker and gives up WITHOUT serving the scope. So a scope change arriving while the workspace state is dropped was recorded as handled by the run that handled nothing: at hydration `scopeChanged` read false, a server-sourced picker took the early return, and it sat empty until the user retyped or it remounted. Committed now only by a run that is actually going to serve the scope. Leaving it stale is what keeps the refresh owed. Matrix: 35 mutants, all killed; baseline and restore both 96/96. The new one — committing `lastScope` early again — dies on the added leg. |
||
|
xarmian
|
268594e57e |
fix(web): a scope change re-queries a server-sourced picker too (TASK-2877)
Codex review round 11 — the tail of round 10's fix, and mine.
The refresh effect now tracks the scope, but it returns early for
server-sourced non-empty queries. That early return is right for an index
DELTA — the index is not that caller's source of truth, and a request per
delta is the rate-limiter pressure the debounce exists to avoid — and
wrong for a scope CHANGE, where the rows on screen are answers to a
different question and stay selectable under the new scope. A scope change
happens when a schema is edited or a pane is retargeted, not per delta, so
the rate-limiter argument does not reach it.
Two lines that looked like guards went, both measured rather than argued:
* `void collection` — the scope pair reads `collection` to build itself,
which IS the subscription, so the separate read added nothing and its
mutant could not be killed.
* the `lastScope !== null` first-run guard — at mount the query box is
empty, and the only reader of `scopeChanged` needs a non-empty query,
so the first run cannot change an outcome either way.
`lastScope` starts null rather than seeded from the props: seeding
captured their mount-time values outside a reactive scope, which
svelte-check flagged (`state_referenced_locally`) — two warnings this
branch introduced and has now removed. svelte-check is back to the six
pre-existing warnings in files this branch does not touch.
Matrix: 34 mutants, all killed; baseline and restore both 95/95.
|
||
|
xarmian
|
74d6564c4e |
fix(web): the picker's collection scope is a tracked input (TASK-2877)
Codex review round 10. The refresh effect read `collection` inside `untrack`, so a relation field whose declared target CHANGES under an open picker — a schema edit, or an SSE-driven collection refresh; `ItemDetail` does not remount the picker for either — kept listing rows from the collection it used to point at, still selectable under the new scope. Everything else in that effect is untracked to keep it off the keystroke path, and the scope was swept up in that. But `collection` is not a per-keystroke value: it is the question the results answer. Predates this unit — it arrived with the U3 extraction (TASK-2862) — and is fixed here rather than filed because U8 makes `collection` load-bearing in a new way: it is now the destination an inline create writes to, so a stale scope means rows from one collection listed beside a create row aimed at another. The test drives the change through a NEW single-prop setter on `ItemPickerProbe`, not through `rerender`. That distinction is the whole reason the probe exists, and its own header says so: `rerender` replaces the entire props object and re-runs the effect whether or not it tracks the prop under test, so a rerender-driven version of this test passes against the untracked build. Verified rather than assumed — the mutant that restores `untrack` dies against the setter version. Matrix: 32 mutants, all killed; baseline and restore both 94/94. |
||
|
xarmian
|
ded64ce232 |
docs(web): record why three races are deliberately not fenced (TASK-2877)
Codex review round 9, two P1s, both declined — and the reasoning goes
beside the fences rather than into a commit message, which is the lesson
round 7 taught when a round-3 decline was re-raised because a reviewer
reading the diff had no way to see it.
A CONCURRENT FIELD CHANGE (SSE, another tab) landing mid-POST is ordinary
last-write-wins on a field the user is actively editing, and it is what
every other type in this component already does — a text field blurred
after a remote change overwrites it too. The race is adjudicated at the
server: `ItemDetail.updateField` sends `expected_updated_at` and
refetch-retries a 409 (BUG-2273 / IDEA-1480). Fencing it here would make
relation fields alone behave differently from every other field, on a rule
the item's own optimistic-concurrency check already enforces.
A LOST RESPONSE on a create that committed is real and is not fixable
here. `item create` has no idempotency key and titles are not unique
(colliding slugs get `-2` suffixes, `store.uniqueSlug`). Nothing
auto-retries — a retry is a person clicking Create again with the picker's
state in front of them — and the repo's standing rule for the identical
shape is exactly that ("Never retry it automatically" for `item copy`).
Filed as IDEA-2880. Deliberately NOT patched client-side: checking for a
same-title item before retrying would rest on the same ranked, paged,
possibly-stale evidence the create row itself rests on, and would look
like a guarantee the client cannot make.
No behaviour change; gates re-run rather than assumed — 2104 web tests,
svelte-check 0 errors.
|
||
|
xarmian
|
c516871328 |
fix(web): read page completeness from the page, not from total (TASK-2877)
Codex review round 8, one P1, and the mechanism checks out in the server
source rather than only in the abstract.
Round 7 gated the cold answer on `(res.total ?? rows.length) <= rows.length`.
`store.search` makes that unreliable in exactly the case it was guarding:
when the count query errors it sets `total = -1`, floors it to 0, and then
floors it again to `len(results)` — "Ensure total is never less than actual
results", `internal/store/search.go:604-608`. So a broken count is
indistinguishable on the wire from an exact-fit page, and the check calls
it complete. The `?? rows.length` fallback was the same mistake a second
time: unknown read as fine, which is the polarity error rounds 3 and 5
already went around on `coldFailed`.
Completeness now comes from the PAGE: a page SHORTER than the limit the
server echoes back is proof there is no next page, and that holds whatever
the count did. A full page is not proof either way, so it does not count
as an answer. No `total` in the decision at all.
The U8 fixtures now carry the real response shape. `total`, `limit` and
`offset` are non-optional on `SearchResponse` and the Go handler always
sends them, so `{ results: [] }` was not a smaller version of a real
response — it was one that cannot occur, and it was quietly deciding the
very question these tests are about.
Matrix: 31 mutants, all killed; baseline and restore both 93/93. M28b —
the previous `total`-based implementation — SURVIVED at first, and the
fixture was why: it asserted against `total: 84, limit: 2`, which both
implementations reject. The leg that discriminates is the floored one
(`total: 2` on a full page of 2 with 84 really matching), i.e. the shape
the server actually emits when the count fails. A mutant that survives
because the fixture never reproduces the real failure is a fixture
finding, not a code finding.
|
||
|
xarmian
|
ff44e917ae |
fix(web): count the 401 drop; a truncated page is not an answer (TASK-2877)
Codex review round 7. Two taken, one answered in the code. P1 — `resetGenerationFor` counted `reset()` and missed the OTHER drop. `bootstrap()`'s unauthorized/forbidden branch clears `state.items`, resets the MiniSearch index and wipes the persisted cache without going through `reset()`, so the fence added in round 6 did not see the revocation case it exists for. Both droppers now call one `markWorkspaceDropped(ws)` helper. Two call sites, because deleting the state entry and clearing rows in place are genuinely different operations; the helper is what makes the pairing greppable, and a test fails if a third site starts clearing rows without it. That test is STRUCTURAL, and deliberately so. Reaching the 401 branch through the front door needs a warm cache plus a pending resync plus a 401 from /items-changes — a fixture larger than the invariant it would check, and I tried it first. The invariant that actually has to hold is "clearing rows and counting the drop travel together". The site-count assertion is what keeps it honest: a NEW clear site fails loudly rather than going silently unexamined, which is how this kind of instrument usually rots. Its own mutant (the 401 branch stops counting) dies. P2 — a TRUNCATED cold page is not an answer to "does this exact title exist"; the row may be on a page nobody fetched. `SearchResponse` carries `total`, so `coldAnswered` now requires a complete page. Same defect as trusting the local ranker's window, arriving from the server side — the third variant of one mistake, which is why the rule is now stated once and asked everywhere: offer only where something authoritative has answered. P2 (query change mid-create) was raised for the second time, having been declined in round 3 with reasons that lived only in a commit message — which a reviewer reading the diff never sees. The reasoning is now a comment beside the fences: the three that exist each stand for an act meaning "not this one" (escaping out, choosing another row, landing on a different item or workspace); typing is mid-thought, the user did ask for the item being created, and cancelling would orphan that row with the field still empty. A decision worth keeping is worth putting where the next reader is looking. Matrix: 29 mutants, all killed; baseline and restore both 93/93. |
||
|
xarmian
|
7b32e57cc9 |
fix(web): a dropped workspace needs an identity signal, not an epoch (TASK-2877)
Codex review round 6, and it corrects the reasoning round 5 shipped.
Round 5 fenced the create on `scopeEpochFor(ws) === epoch` and recorded
the residual as needing a coincidence — a purge plus resyncs landing back
on the captured number. That was wrong, and wrong in the direction that
matters: `reset()` deletes the state and the replacement starts at
`scopeEpoch` 0, which is ALSO the value whenever no projection resync has
ever run. That is the ordinary case, so the equality check passed
trivially across exactly the event it was added to catch. A residual I
called exotic was the default path.
The fix is the signal the store did not expose: `resetGenerationFor(ws)`,
a monotonic per-workspace count of drops, deliberately kept OUTSIDE the
`workspaces` map because `reset()` deletes that entry. Both existing
counters — `scopeEpoch` and the internal `generation` — live on the state
object and restart with its replacement; they are safe only because their
readers hold a REFERENCE to the object, which a caller outside the module
cannot. It is bumped even when the reset found no state to drop, so a
purge racing a first bootstrap does not read as no purge.
`createRelationTarget` now asks two questions rather than one:
* `indexStillOurs()` — is this the index the request was authorized
against? It gates the UPSERT, which was previously unconditional on
the argument that a real row belongs in the index. That argument does
not survive a purge: a brand-new id was never in `upsert`'s fenced
set (nothing to fence — the row did not exist when the purge ran), so
the write lands and is persisted to IDB, resurrecting a row into a
workspace the user may have just lost access to. This is the gap
BUG-2098's own comment describes.
* `stillWaiting()` — is the user still waiting on THIS create? It gates
the link and the toast, and it is now ONE predicate rather than two
hand-copied condition lists. The failure path had drifted from the
success path by exactly the reset half (round 6 P2); sharing the
predicate is what stops that recurring.
Matrix: 27 mutants, all killed; baseline and restore both 91/91. New
store surface carries its own suite, including a CONTROL asserting that
`scopeEpochFor` genuinely cannot answer this question — if that ever stops
holding, the cheaper round-5 fence was sufficient after all and this
accessor should go.
|
||
|
xarmian
|
f7bb735771 |
fix(web): state the cold rule positively; catch the epoch reset (TASK-2877)
Codex review round 5, two P1s, both about `localIndex.reset()` — the sign-out / 403-purge / deleted-workspace path. THE FLAG WAS THE WRONG WAY ROUND. `coldFailed` asked "did the last search fail", and that was false in three states that are not answers at all: before the first request, after a failure, and after a reset drops every row while the query sits in the box. Each one read as "fine" and put a create row on screen backed by nothing. Inverted to `coldAnswered` — set in exactly one place, by the event that earns it, and cleared wherever the answer stops describing what is in the box. A flag that must be cleared everywhere is one that will be missed somewhere; this is the same defect arriving twice (round 3 caught the failure case, round 5 the reset case) because the polarity made silence indistinguishable from success. THE EPOCH FENCE HAD TO BE TWO-SIDED. `upsert`'s own guard refuses a captured epoch BELOW the current one, which catches a resync. But `reset()` DELETES the workspace state and the next bootstrap starts a fresh one at `scopeEpoch` 0 — so a captured 7 is not below 0, sails through, and links a row minted under an identity that no longer holds. `createRelationTarget` now requires equality. The residual is in the code comment rather than papered over: a reset plus resyncs landing back on exactly the captured number would compare equal, which an exposed reset generation would catch and this does not. Also dropped the `loading` term from `showCreate`. It and the per-query `coldAnswered` reset were a redundant PAIR — each survived removal while the other stood, which is one guard and one line that looks like a guard, not defence in depth (this repo has a note about exactly that shape). `coldAnswered` is the one kept: it states the rule (something authoritative has answered FOR THIS QUERY) where `loading` is a UI state that correlates with it. Matrix: 24 mutants, all killed; baseline and restore both 85/85. Killing the per-query reset needed `aria-expanded`, not the row's absence — with `loading` still gating the MARKUP, `.picker-create` is missing either way and asserting on it measures the branch instead of the rule. Third time this suite has been fooled by that same separation. Re-verified end to end in a real browser on this exact build: create row offered for a non-matching query and keyboard-reachable; Enter created COLO-6 "Chartreuse" in COLORS (colors 2 -> 3, cars unchanged) with `status: approved` — the schema's declared default, which the "+ New" `options[0]` heuristic would have gotten wrong; the car's field holds that id; a second pass at the same text offers the existing row and no create; Escape leaves the value untouched; no bare UUID anywhere on the page. |
||
|
xarmian
|
6a335dd120 |
fix(web): absence is only evidence from a settled index; fence the error toast (TASK-2877)
Codex review round 4. Two taken, one declined. P1 — `bootstrapState === 'ready'` was the wrong authority for the create row. It coexists with `pendingResync`: `localIndex` hydrates from the IDB cache and serves those rows while delta-sync catches up, so during that window an item that EXISTS can be missing from the snapshot. The create row is derived from ABSENCE, and a cache snapshot cannot support that inference — presence still can, since the row was real when it was cached. `indexCanProveAbsence()` is asked ONLY by `showCreate`; search and listing keep using `isWarm`, because showing cached rows during a resync is right and it is only the "therefore no such item exists" step the cache cannot bear. The window is seconds and a duplicate outlives it. That leaves one rule across the whole unit, applied in four places now: offer only where something authoritative has answered. Cold is authorized by `/search` (the server answered); a settled index is authorized by the in-RAM collection; a resyncing index and a failed search authorize nothing. P2 — the failure path was unfenced while the success path was not, so a create the user escaped out of, or one belonging to a workspace they have since left, still threw its error over whatever they were looking at. Same three conditions, same reasoning: the difference between reporting and not is whether they are still waiting on it. DECLINED, with reasons, so it is not re-flagged: the "A->B->A gap" in the workspace fence. The classic gap bites when an identifier can be REBOUND to a different object between capture and compare. Here the pair (workspace slug, item slug) is what the fence compares, and the parent subtree is keyed on the item slug, so returning to the same pair returns to the SAME item — applying the create there is correct, not stale. Item refs are sequential and never reused, so the identifier cannot be rebound within a workspace. Matrix: 22 mutants, all killed; baseline and restore both 82/82. Four anchors went stale this round because the fence now appears on two paths and matched twice — the harness refused to score them rather than silently mutating the wrong copy, which is the reason it checks. |
||
|
xarmian
|
761e6e2453 |
fix(web): a failed cold search is not evidence that nothing matched (TASK-2877)
Codex review round 3 P2. `coldSearch`'s catch leaves exactly the state a
successful empty answer leaves — no rows, not loading — and the result
list is right to render both as "No results". The create row is not: an
empty answer is evidence that no such item exists; a failed one is no
evidence at all, and offering to create on no evidence is how a duplicate
gets minted while the index is cold and the network is unhappy. Same rule
the permission gate already follows — no answer must not read as
permission.
A `coldFailed` flag now separates the two, and where it is CLEARED was
settled by the matrix rather than by symmetry. Three reset sites looked
obviously needed and three mutants removing them survived:
* the cold branch of `runQuery` — `loading` is true for that entire
window and already suppresses the row, and both `coldSearch` branches
assign the flag outright when the request settles;
* the empty-query branch — covered twice over, since an empty query
offers no create row at all;
* the workspace-reset effect — same as the first.
All three are gone rather than carrying a comment claiming a protection
they do not provide, which is the disposition this plan's own U3 note
records for an unkillable guard. The ONE reachable reset is the warm
branch: it is the only path that produces a fresh verdict without going
through `coldSearch`, so without it a single network blip suppresses the
affordance for the rest of the session even once the authoritative in-RAM
answer is available. That one has a test, and its mutant dies.
Round 3 also raised a P1 I am NOT taking: typing a new query while a
create is in flight does not cancel it. The three fences that exist —
escape, picking another row, retargeting — each stand for an act that
means "not this one". Typing is not such an act; it is mid-thought, and
the user did explicitly ask for the item that is being created. Treating
it as a cancel would leave the created row orphaned and the field unset,
which is a worse outcome than a field that ends up holding exactly what
was asked for. Told to Codex in the next round rather than left to be
re-flagged.
Matrix: 20 mutants, all killed; baseline and restore both 80/80.
|
||
|
xarmian
|
5dffd734c1 |
fix(web): fence the create against cancel and against a workspace switch (TASK-2877)
Codex review round 2, two P1s, both confirmed at the lines they name. CANCEL. `oncancel` only closed the picker, so backing out did not supersede an in-flight create — the pending promise then resolved and selected an item the user had just declined. Backing out is as explicit a choice as picking a different row, and now bumps the same counter. WORKSPACE SWITCH. `ItemDetail` keys its fields subtree on `itemSlug` ALONE, so switching workspaces to an item carrying the SAME ref — and every workspace has a TASK-5 — reuses this component rather than remounting it, and `destroyed` never fires. The completion then wrote an item ID from the previous workspace into the new workspace's item. `createRelationTarget` already captured `ws` and `collSlug` before the request; it now compares them to the live props before applying, which is the DR-6b shape `ChildItems.submitCreate` uses for the same reason. The `localIndex.upsert` still runs ahead of all three fences and still uses the CAPTURED workspace: the item genuinely exists in the workspace it was created in, and the fences are about where the VALUE is written, not about hiding a real row. Matrix now 17 mutants, all killed; baseline and restore both 78/78. The two added here — cancel not bumping the counter, and the ws/collection comparison removed — are what stand in for having seen these two tests red before the fix, since pin and fix landed in one edit. |
||
|
xarmian
|
83e4abc966 |
fix(web): fence the in-flight create; ask the index, not the ranking (TASK-2877)
Codex review round 1, three findings, all confirmed by reading the code
they name rather than taken on the report.
P1 — the create completion had no fence, and there are two ways past it.
`ItemDetail` wraps its fields section in `{#key itemSlug}`, so an item
switch DESTROYS this component; the promise survives, and `onchange` calls
into the persistent parent, whose `updateField` builds its PATCH against
whatever item is current at CALL time. A create started on car A therefore
wrote its colour onto car B. Separately the picker stays open across the
round trip, so the user can settle on another row (or clear the field)
before it lands — and last-write-wins is the wrong rule there, because the
later write is an explicit choice and the earlier one is a promise they
have moved past. A `destroyed` flag and a supersede counter, checked
together, close both. The `localIndex.upsert` deliberately runs BEFORE the
fences: the row exists on the server whatever happened locally, and
withholding it would leave a picker offering to create it a second time.
P2 — `targetCollection` read the global collection list with no freshness
gate, so during a workspace switch a slug match against the PREVIOUS
workspace's rows yielded a foreign collection ID, and `canEditCollection`
answered about that. Same gate `knownCollectionSlugs` already had, which
this derivation was missing.
P2 — the exact-title suppression was asking the RANKING. `warmSearch`
requests `limit + excluded.size` hits, so an exact row the ranker placed
outside that window is simply absent from `rawResults` and the picker
offers a duplicate. The question has an authoritative answer in
`localIndex`, already in RAM, so the warm path now scans the collection
directly. The `rawResults` check stays and is NOT redundant: while the
index is cold there is nothing to scan, and the server's rows are the only
evidence the row exists — pinned by its own leg, which is what killed the
mutant that removed it.
Mutation matrix now 15 mutants, all killed; baseline and restore both
76/76. Two rounds of it earned their keep beyond the fixes: M3 SURVIVED
once the index scan landed, and the mutant was faithful — the suite had no
cold-path exact-match leg, so the surviving mutant found a real hole in my
tests rather than a redundant line in the code.
|
||
|
xarmian
|
e331342450 |
feat(web): relation fields create their target inline, permission-gated (TASK-2877)
PLAN-2857 U8, caller half. `FieldEditor` hands the picker an `oncreate`
only when the viewer may create in the field's DECLARED TARGET, so it
decides both of the unit's gates by deciding whether to pass one.
The gate is `canEditCollection` on the target collection — the same
predicate behind the collection page's "+ New" — asked about where the
item would LAND, not about where the user is standing. It needs the
collection's ID, which only the loaded collection list carries; a target
the list does not know yields no create row, because "no answer" must not
read as "allowed".
NO FIELD VALUES ARE SENT, and that is a decision with a receipt. The
server fills every missing key that declares a `Default` and stores the
defaulted map (`items.ValidateFields`, then "Marshal validated/defaulted
fields back" in `createItemChecked`), so the schema's own answer is
already the right one. The collection page's "+ New" guesses
`status.options[0]` instead; driven live against a Colors collection whose
status options are [draft, approved] with `default: approved`, the created
row came back `{"status":"approved"}` — the declared default, which that
heuristic would have gotten wrong. The cost is that a target carrying a
REQUIRED field with no default refuses the create; that surfaces as a
toast naming the field, which is the honest outcome for a row this picker
cannot fill in.
The new item is upserted into `localIndex` under the epoch captured BEFORE
the request (BUG-2098 — a projection resync landing mid-flight means the
response was authorized under a scope that no longer applies). That upsert
is what makes the picker's exact-title suppression true on the very next
keystroke; without it the same text offers to create a second item.
Mutation matrix, all killed: creating in a collection other than the
declared target, the permission gate removed, the upsert removed, and the
epoch read after the request rather than before.
|
||
|
xarmian
|
322b461606 |
feat(web): the scoped picker offers an inline create row (TASK-2877)
PLAN-2857 U8, picker half. When a scoped picker's query matches nothing — or nothing EXACTLY — it offers a trailing "Create "<query>" in <collection>" row, keyboard-reachable like any other row. The affordance is opt-in at the call site: it appears only when the host passes `oncreate`, which is how both of U8's scope rules are expressed without this component knowing either. "Relation fields only" is the Relationships tab passing nothing; the permission gate is the caller's, because "may this user create in the target collection" is the collection-level `canEditCollection` cascade that lives in the workspace store. Result rows and the create row become ONE `options` list, in render and keyboard order, so arrowing onto the create row needs no special case and cannot fall out of step with what is on screen. `activeId` already addressed rows by identity; the create row takes a NUL-prefixed sentinel id in the same namespace, which no UUID can collide with. Two suppressions carry weight and both are pinned: * EXACT-TITLE. Tested against `rawResults` — the source's answer before exclusion and the row bound — because an exact match pushed past `limit` or excluded by the caller would otherwise read as "no such item" and offer to mint a duplicate of a row that exists. This IS the no-duplicate half of the unit's proving test: there is no create-time uniqueness check anywhere, because the second pass at the same text never reaches a create. * LOADING. Mid-flight, "nothing matched" is not yet known. The one assertion that can fail here is `aria-expanded`, not the row's absence: the markup renders the loading branch INSTEAD of the listbox, so a build that offered the row mid-flight would still show no `.picker-create` and merely leak a combobox announcing itself expanded over no listbox. That is trap #1 from this plan's false-green note, met in my own diff. Re-entrant creates are dropped while one is in flight, so two Enters inside a single round trip cannot mint two items — a duplicate the exact-title check cannot catch, since no row exists yet to match. Mutation matrix, all killed: exact-title suppression removed (3 tests), re-entrancy guard removed, `loading` term removed, `collection` term removed, Enter dispatching over `results` (the pre-U8 line), create row prepended rather than trailing. |
||
|
xarmian
|
6f7c09a44a |
fix(web): judge a relation's collection only when the list and index agree (TASK-2868)
Codex round 2, P1, real — and it is the retag window my round-1 fix left open. `retagCollection` moves the indexed ROWS onto the new slug immediately; `collectionStore.loadCollections(ws)` is fired next to it with `void` — not awaited, and its rejection swallowed. So between those two there is a state where the collection list still holds the OLD slug (making the declared target read as 'live') while the row already carries the NEW one. Judging the mismatch there reported the value as "Unresolved reference" — and because that refetch is unawaited and its failure unobserved, the state is PERMANENT when it fails, not a paint-frame flicker. The fix reframes what the mismatch is evidence OF. A collection mismatch means the value is wrong only when the collection list and the item index agree about the world — that is, when the current list knows BOTH the declared target and the row's own collection. Two ways they disagree, and neither is the value's fault: the target was renamed away (round 1), or the rename reached the index before the list (this round). Requiring both slugs to be known collapses both to "don't judge", while a genuine cross-collection value — target `colors`, row `tasks`, both live — still resolves to null. That is also why this is not fixed by invalidating freshness: `collectionsAreFreshFor` answers "loaded for this workspace", not "current", and teaching it about pending/failed refreshes is a store-wide change to serve one consumer. The agreement test needs nothing new. New control leg alongside it, because two "don't judge" guards in a row are one edit away from never judging: both slugs live, mismatch, still rejected. Mutation matrix 17 of 17 killed (N17 new — judge as soon as the target reads live, i.e. round 1's shape; N11/N14/N15 anchors refreshed). Gates: `npm run check` 1093 files 0 errors, 6 pre-existing warnings; full web suite 123 files / 2063 tests green. Context 55.8% at this boundary (`session-shape`, which lives at /home/dave/claude/bin and is not on PATH — my earlier "not measured" reports read `command -v` failing as the tool not existing). |
||
|
xarmian
|
09f80b3d4f |
fix(web): a renamed target collection must not read as lost data (TASK-2868)
Codex round 1 on this unit, P1, correct — and it is a defect the PREVIOUS
commit introduced.
`models.FieldDef.Collection` holds the target's SLUG (the schema editor binds
`<option value={c.slug}>`), and `store.UpdateCollection` re-slugifies on rename
without migrating the relation definitions that point at it — the string
"relation" does not appear in that file at all. Meanwhile `localIndex.applyRetag`
correctly moves the indexed ROWS onto the new slug. So after a rename the field
and the rows disagree, and the collection check added last commit reported every
stored value as "Unresolved reference": a schema problem presenting to the user
as lost data, on data that is completely fine.
Now three-valued. The collection check applies only while the declared target
still names a LIVE collection; a stale target falls back to id-only resolution
so the chip keeps rendering, and the field goes read-only because a picker aimed
at a renamed collection would list nothing (`getByCollection` and
`localSearch` both filter on that slug). `'unknown'` — the collection list not
yet loaded — is deliberately NOT read as stale: that is absence of evidence, and
treating it as stale would flash every relation field into read-only on first
paint.
Filed **BUG-2873** for the root cause, with both candidate fixes (migrate
dependent schemas on rename, or store the collection ID) and the argument that
the second is what PLAN-2857's own "store the ID, titles change" reasoning
implies for the collection pointer too.
**Two of the three new tests were wrong first, in ways that let a mutant live.**
- The stale-target test set up the STORE's collection list but left the row's
`collection_slug` matching `field.collection` — so field and row agreed, and
the mutant making the check unconditional passed. A rename retags the rows;
modelling only half of it reconstructs a scenario that cannot fail.
- The unknown-vs-stale test asserted the chip renders. A stale target also
renders the chip, so it could not tell the two apart. It asserts EDITABILITY
now, which is the only thing that actually differs.
Mutation matrix 16 of 16 killed, including the four new ones (N14 stale target
invalidates values, N15 unknown reads as stale, N16 stale target stays editable,
plus N11 refreshed for the new shape).
Gates: `npm run check` 1093 files 0 errors, 6 pre-existing warnings; full web
suite 123 files / 2061 tests green; `go build ./...` ok, gofmt clean.
|
||
|
xarmian
|
5079a532c9 |
fix(web): resolve a relation by id in its own collection; collapse the picker (TASK-2868)
Three defects, all found by driving the Cars/Colors example from IDEA-2856 in a real browser against a locally built binary. None of them showed up in the twelve component tests, and two are mine. **1. A legacy free-text value rendered as a working reference.** `localIndex.findByIdOrSlug` resolves by id OR SLUG, so the string `"red"` — exactly what the old text fallback has been writing into these fields — resolved to the item slugged `red` and rendered as a live chip. The field's contract is that it stores an item ID; a slug match makes the chip lie about what is stored, and slugs are mutable, so the same value could point elsewhere tomorrow. Now resolves by id only, and the browser leg that was meant to prove "a legacy value reads as unresolved" is the one that caught it. **2. It could resolve into the WRONG COLLECTION.** That helper is workspace-wide, so a relation declared against `colors` would render an item from `tasks` sharing the identifier. This is the same defect PLAN-2857's recon recorded against the server's `ResolveItem` — I wrote that finding down in the design doc and then reproduced it in my own client code a few hours later. **3. The field showed a permanently-open search box.** The first browser pass rendered the chip, the picker input still holding the query, and the result list still listing the row just chosen — the same item three times, under every relation field on the page. A field shows its VALUE; the picker is for changing it. Now: chip + Change / Clear, picker on demand, closing when a choice is made. Also **filed BUG-2872** rather than absorbing it: the activity timeline on the same page renders a relation change as `color: → <uuid>`. The panel now honours IDEA-2856's "never a bare UUID"; that surface does not. The e2e invariant is scoped to the field row on purpose, so the gap is recorded rather than hidden by loosening the assertion to the page body. Mutation matrix 13 of 13 killed. N13 (emit the value but leave the picker open) survived the first pass — the test asserted the emit and not the CLOSE, which is the half the browser pass had rejected. Two mutants that had to die separately do: N1 (drop the deleted branch) kills leg (b), N2 (unresolved reads as deleted) kills leg (c), so the two states are genuinely distinguished rather than sharing a branch. Gates: `npm run check` 1093 files 0 errors, 6 pre-existing warnings; full web suite 123 files / 2058 tests green. |
||
|
xarmian
|
a04233aaa9 |
feat(web): relation fields render a linked chip and edit through the picker (TASK-2868)
PLAN-2857 U2. Absorbs the relation half of TASK-2216.
Before this, `relation` fell through `fields/FieldEditor.svelte`'s `{:else}`
text fallback: an editable free-text input in edit mode, and `{value ?? '—'}`
in display mode. Since `internal/items/validate.go:275` accepts ANY string for
a relation, that combination did not merely fail to edit — it SAVED. Typing
"red" into a relation field stored the literal string and showed no error, and
the display arm rendered a raw UUID when the value happened to be one. So U2 is
closing a silent corruption hole, not adding an editor to a read-only field.
**Three render states, not two.** A value that resolves to nothing and a value
whose target was deleted are different facts about the item, and the third is
the COMMON case on existing data — arbitrary strings are what the old fallback
has been writing. All three resolve locally: `localIndex` holds soft-deleted
rows alongside live ones (`getByCollection` filters them out rather than
dropping them), so a dangling target is a row carrying `deleted_at`. No fetch,
no loading state. The invariant across every state, asserted in every leg: a
raw item ID never reaches the user.
**The branch is gated on `wsSlug` AND `field.collection`, and the second call
site sits on the far side of that gate deliberately.** `CopyItemDialog` builds
its `FieldDef` from a preflight row whose shape carries no `collection`
(`ItemCopyPreflightNeedsValue`), and it copies ACROSS workspaces — so an
unscoped picker there would offer SOURCE-workspace items as the value for a
DESTINATION-workspace field, and look authoritative doing it. A free-text box
at least looks like something the user owns. Read-only is the honest state
until TASK-2869 (U2b) extends the preflight contract; U1 makes the garbage
write a 400 in the meantime.
That gate is a fact about the CALLERS, invisible from the component's own
render tests, so both sides are asserted at the call sites
(`fieldEditorRelationCallers.test.ts`) — including that `toFieldDef` still
builds from a shape with no `collection`, which fails loudly when U2b lands
rather than letting the gate drift.
Pin first, per team CONVE-29: the test file was written and run BEFORE the
branch existed — 4 failed / 2 passed, the two passers being the gate legs,
which pass vacuously while no picker exists anywhere. That is why they ship
with a control leg that mounts one.
One pin leg was STRENGTHENED rather than relaxed when it failed against the new
code: leg (a) asserted "some anchor exists" and failed because the test passed
no `username`, which is what builds the href. The fix was to give it one and
assert the exact href, plus a new leg (a2) for the resolved-but-no-route case —
where the chip degrades to a non-link and must still name the item rather than
degrade to the raw value, which is precisely what the old arm did.
Gates: `npm run check` 1092 files 0 errors, 6 pre-existing warnings; full web
suite 122 files / 2048 tests green.
|
||
|
xarmian
|
cd5c5702d4 |
feat(web): give ItemPicker a source model; keep the Relationships tab on server FTS (TASK-2862)
Lead ruling on PR #1241, and the right call. The extraction had silently moved the add-relationship search onto the warm local path, and `localIndex` strips `content` by design — so a user who links an item by a phrase they remember from its BODY lost that, with no signal anything had changed. Consistency with the other pickers does not buy back a capability under CONVE-139. `source` is now an explicit MODEL choice, not a performance one: 'index' (default) — `localSearch` over title / ref / tags / parent / field values, no network call, server only as a cold fallback. Right for a RELATION field, where you are choosing a row from a known collection and know what it is called. U2 onward take this. 'server' — always `/search`, whose FTS also indexes body content. Right for the Relationships tab, where you are finding an item you remember rather than one you can name, and what it did before this component existed. ItemDetail passes it. An empty-query LISTING stays on the index for both: it is not a search, `/search` cannot answer one (it requires a `q`), and the rows are local either way. Only QUERIES follow `source`. Two supporting changes fall out of it rather than being bolted on: **`rawResults` + a derived `results`.** The exclusion filter is now part of the derivation, so a late `excludeIds` — `ItemDetail` loads `itemLinks` asynchronously — re-filters on its own. Without that, honouring a late exclusion on the server-backed caller would have meant re-issuing the request, which is the rate-limiter pressure the debounce exists to avoid. The refresh effect no longer needs `excludeIds` as a dependency at all, and server-sourced QUERIES are explicitly not re-run on an index delta. **The highlight is an ID, not an index.** `activeId` is state; `activeIndex` derives from it. Identity survives the list changing underneath — a delta, a late exclusion — where an index silently moves the highlight onto whatever slid into that position. This deletes the hand-rolled preserve/restore that lived in the effect, so no future site that changes the list has to remember to do it. Pins, per the ruling: the server caller queries `/search` with a hydrated index and never touches `localSearch`; the control leg asserts the default source on the same warm index never reaches the network; and a source-level test asserts ItemDetail's call site still carries `source="server"` — a regression invisible from the component's own tests, which is why it is asserted at the call site. Verified in a real browser against a locally built binary with a marker string present ONLY in an item's body and never in its title, so the local index cannot answer it: the Relationships picker finds it, arrows to it, and creates the link. Mutation matrix 20 of 20 killed, including the three new ones — ignore `source` (3 failed), re-query on a delta (1 failed), drop `source="server"` at the call site (1 failed). Gates: `npm run check` 1092 files 0 errors, 6 pre-existing warnings; full web suite 121 files / 2041 tests green. |
||
|
xarmian
|
4b0a818178 |
fix(web): drop stale rows on an index reset; re-filter on a late exclusion set (TASK-2862)
Codex round 4, both findings real. **P1 — an index reset left rows on screen.** `localIndex.reset()` (sign-out, a 403 membership purge, a deleted workspace) drops the workspace state and resets the search index, which bumps the epoch — so the refresh effect DID run, saw a non-`ready` state, and returned. Rows the viewer may no longer be allowed to see stayed listed and selectable, and a cold response already in flight could still add more. The effect now tears down on that path: invalidate `seq`, cancel the debounce, clear the results and the highlight. I wrote that teardown behind a "only if we are showing or awaiting something" guard first. The mutant removing the guard could not be killed, and working out why showed the guard is dead — the one non-ready run that reaches it with nothing to clear is the cold mount, where it changes nothing either way. So the guard went, rather than acquiring a comment claiming it protects something. The control leg stays: an ordinary cold mount must still complete its own search. **P2 — `excludeIds` arriving late did not re-filter.** `ItemDetail` loads `itemLinks` asynchronously, so a picker opened before that resolves was offering items already linked to the source; clicking one is a duplicate-link write the user did not know they were making. The effect now tracks the exclusion set too. **The first test for it was a false green, and that is the more useful half.** Driving the prop change through testing-library's `rerender` REPLACES the whole props object, which re-runs the refresh effect whether or not it tracks `excludeIds` — so M20 (delete that dependency) survived while the production path, where a real parent changes one prop, was broken. Adding "the typed query survived, so this was not a remount" did not help: the instrument was wrong in a different way than suspected. It now runs through `ItemPickerProbe.svelte`, a test-only host in the shape of `FreezeProbe` / `GuardProbe`, which changes exactly one prop. M20 dies against it. Mutation matrix rebuilt and re-run, 18 of 18 killed. The harness itself was rewritten this round after a regex edit corrupted its escapes and reported seventeen false ANCHOR MISSes — anchors are now generated with explicit tabs into a JSON file rather than hand-escaped in source. Verified again in a real browser against a locally built binary, including the round-4 P2 path end to end: link a target, re-open the picker, and confirm the now-linked item is no longer offered. Gates: `npm run check` 1092 files 0 errors, 6 pre-existing warnings; full web suite 121 files / 2035 tests green. |
||
|
xarmian
|
03ec276cac |
fix(web): refresh on the search epoch, not the workspace cursor (TASK-2862)
Codex round 3, one P2, correct — and it catches that round 2's fix tracked the wrong signal. `localIndex.upsert()` and `remove()` — optimistic creates and edits, the 403 purge — mutate `state.items` and mirror to `localSearch` WITHOUT advancing `state.cursor`. So a picker wired to the cursor stayed stale after exactly the mutations a user is most likely to cause while it is open. `localSearch.epoch(ws)` is the right dependency and strictly dominates the cursor: it is bumped by every write to the search index, and every `localIndex` path that touches `state.items` mirrors there — verified one by one this round: `applyDelta` (three sites), `upsert`, `remove`, `removeByCollection`, `applyRetag` (behind `retagCollection`), `reset`. It also exists for precisely this consumer shape; its own doc comment describes an `$effect` re-deriving search results, added for the identical staleness bug in TASK-1364 round 3. I should have found that before reaching for the cursor. The distinction is pinned by a test that bumps the epoch and asserts the cursor never moved, and by mutant M16 (track the cursor instead — 3 failed). The double deliberately KEEPS `cursorFor` rather than dropping it, so a build wired to the cursor still runs and simply fails to refresh: a mutant that dies of a TypeError would prove nothing about which signal is correct. Mutation matrix, 15 of 15 killed (M13 rewritten, M16 new). Gates: `npm run check` 1091 files 0 errors, 6 pre-existing warnings; full web suite 121 files / 2031 tests green. |
||
|
xarmian
|
3de173a2d4 |
fix(web): keep an open picker current with the local index (TASK-2862)
Codex round 2. One finding accepted and fixed, one declined with the
measurement — see below.
**Accepted: an open picker held a stale COPY of the rows.** `results` is a
snapshot, so an SSE delta landing while the picker was on screen left it
listing what the workspace used to contain until the query changed or it
remounted. This is the same missing dependency as round 1's cold-mount finding,
so the two collapse into one effect: it now tracks the bootstrap state AND the
workspace cursor (`$state` on `WorkspaceState`, bumped by every applied delta
batch), and re-lists on either.
Re-listing on every delta made the round-1 empty-query guard the wrong shape —
it would have frozen a typed query's results for as long as the user kept
typing nothing. Replaced with something better: the re-list now PRESERVES the
highlighted row **by id**, recomputing its index against the new results. A
blind re-list takes back a row the user arrowed down to; keeping the old INDEX
is worse still, silently moving the highlight onto whatever slid into that
position. If the row is gone, the highlight clears. Both wrong variants are
pinned as mutants (M14, M15).
**Declined: "empty-picker Escape does not reach pane handling."** Re-raised
from round 1 as a behaviour rather than a prose issue. Measured before
declining:
- Pre-change, the add-relationship box was a bare `<input type="text">` with
no keydown handler at all, so Escape there did nothing. Same today when a
caller omits `oncancel`. Not a regression.
- Both pane hosts bail on text-entry targets BEFORE running the escape stack
(`[collection]/+page.svelte:2321`, `[collection]/[slug]/+page.svelte:465`),
with the rule stated in their own comments: "Text-editing targets own ESC
locally". Making the picker hand Escape upward would contradict a
route-level policy, which is not this unit's to change.
- The one caller today, ItemDetail, DOES pass `oncancel`, so in the shipped
configuration Escape clears the query and then closes the form — strictly
more than it did before.
The prop doc says plainly what omitting `oncancel` costs, so U2 cannot acquire
the gap by accident.
Mutation matrix re-run on the changed tree, 13 of 13 killed (M11 rewritten for
the new effect; M13/M14/M15 new — drop the cursor dependency, re-list blind,
preserve the index instead of the identity).
Gates: `npm run check` 1091 files 0 errors, 6 pre-existing warnings; full web
suite 121 files / 2030 tests green.
|
||
|
xarmian
|
3c63bfbd9d |
fix(web): re-list a scoped picker after hydration; correct the Escape prose (TASK-2862)
Codex round 1, both findings P2, both real. The second one is the more useful of the two because what it caught was a claim I made without reading the code it was about. **A scoped picker mounted before the local index hydrated stayed empty.** `recent()` ran on mount and on input only, and returns [] while the index is cold — so a relation field opened during hydration (U2's case) showed nothing until the user typed, then silently started working. A `$effect` now re-lists when the bootstrap state reaches `ready`. It tracks that read ONLY; `query` is read inside `untrack`, because reading it reactively would re-run the effect on every keystroke and race `oninput` — the same reason `onMount` owns the first run. The refresh is guarded on an empty query, and that guard is now pinned by its own test: without it, hydration landing mid-session resets `activeIndex` and takes back a row the user had already arrowed down to — a keystroke they never made, at a moment they cannot predict. **The Escape comment was wrong.** It said that declining to stopPropagation "correctly lets Escape reach the pane". It does not: both pane hosts' keydown handlers call `isTextEntryTarget(target)` and RETURN before they run the escape stack (`[collection]/+page.svelte:2321`, `[collection]/[slug]/+page.svelte:465`), on the deliberate rule that a text field owns Escape locally. So a picker with no `oncancel` leaves Escape with no owner at all — which is also what the inline search this replaces did, so it is not a regression, but it is not what I wrote either. I had the escapeStack file open and read the STACK rather than its DRIVER. Fixed in three places: the `oncancel` prop doc now says plainly what omitting it does, the handler comment states the real mechanism and marks the stopPropagation as belt-and-braces rather than load-bearing, and the control test is renamed off the false claim — it asserts what it actually establishes (the picker does not consume a key it has nothing to do with, including `defaultPrevented === false`). Test-harness note: `bootstrapStateFor` is now backed by a `SvelteMap` in the suite instead of a `vi.fn` return value. A bare mock return is not reactive, so a test driving it that way cannot tell a working refresh from a missing one — the first version of the hydration test passed against both. `SvelteMap` is a runtime class, so a plain `.ts` file gets the tracked get / triggering set that the real store gets from `$state`. Mutation matrix re-run on the changed tree, 12 of 12 killed — the two new ones being M11 (drop the hydration re-list, 1 failed) and M12 (drop its empty-query guard, 1 failed; survived until the selection test above was added). Gates: `npm run check` 1091 files 0 errors, 6 pre-existing warnings; full web suite 121 files / 2028 tests green. |
||
|
xarmian
|
17ec291908 |
fix(web): align the type-select to the picker input, not to the whole picker (TASK-2862)
Caught in a real browser, not by a test. `.add-link-controls` is a flex row and the picker is a COLUMN (input, then its result list), so the row's default `align-items: stretch` made the "Related / Blocks / …" select as tall as the input PLUS the open result list. Before the extraction the results were a sibling of this row, so the question never came up. `align-items: flex-start` restores the pre-extraction look. Verified against a locally built binary on both Playwright projects — desktop-chromium and mobile-chromium (Pixel 7) — driving the real flow: the form opens focused, the input carries `role=combobox`, typing filters, ArrowDown sets `aria-activedescendant` to the first option's id, Enter creates the link (read back from `/links`) and closes the form. At the mobile width the row keeps its shape and long titles ellipsis rather than overflow. |
||
|
xarmian
|
43c33d1aeb |
test(web): make the picker's fence and consumption guards actually discriminate (TASK-2862)
Two tests from the previous commit passed against builds with the thing they
name deleted. Both are fixed here, and the mutants that exposed them now die.
**The staleness fence.** The first version released a superseded response while
the picker was still `loading`, and asserted the stale row was not in the DOM.
It passed with `if (mySeq !== seq) return;` removed — the loading branch renders
instead of the result list, so the row was in `results` and merely off screen.
The scenario now lets the SECOND request land first, so the picker is settled
and the fence is the only thing between the stale row and the DOM.
**The unmount leg.** "Unmount, resolve, assert the row is absent" cannot fail:
an unmounted component renders nothing whatever the teardown does. Replaced
with the half that IS observable — closing the picker inside the debounce
window means the request is never sent. `onDestroy`'s `seq` bump is kept and
now says in a comment that no test can kill its removal, rather than being
defended by one that would pass either way.
**The consumption guard.** `/<ItemPicker\b/` matches inside an HTML comment, so
commenting the mount out left all five source assertions green — the guard
passed against the exact regression it exists to catch. It now runs against a
comment-stripped copy, which also removes a false `{#key itemSlug}` match from
the explanatory comment above the real directive.
Mutation matrix, all ten killed (each applied to the shipped tree, suite run,
tree restored from a COPY — never `git checkout`):
M1 drop the row cap ...................... 2 failed
M2 always take the server path ........... 1 failed
M3 never take the server path ............ 3 failed
M4 drop the staleness check .............. 1 failed (survived before this commit)
M5 stop consuming Escape ................. 1 failed
M6 drop aria-activedescendant ............ 1 failed
M7 ignore the exclusion set .............. 1 failed
M8 drop the collection scope on /search .. 1 failed
M10 drop onDestroy's clearTimeout ......... 1 failed
M9 comment out ItemDetail's mount ........ 3 failed (survived before this commit)
|
||
|
xarmian
|
a85016972a |
feat(web): extract the add-relationship search into a shared ItemPicker (TASK-2862)
PLAN-2857 U3. The relation-field editor (U2) and the Relationships tab need
the same "find an item and choose it" control; today it exists once, inline in
ItemDetail, closing over that component's `item` and `itemLinks`. This lifts it
out so U2 mounts it rather than copying it.
WHERE THE CANDIDATES COME FROM. The design pass ruled OUT the dropdown-vs-
search threshold PLAN-2857 asked for. `localIndex` is a workspace-wide in-RAM
read model of every item — `/workspaces/{ws}/items-index` takes no limit
parameter — and `localSearch` is a MiniSearch index built over it, so a target
collection's rows are already in memory, already ranked, and cost no network
call. The threshold was pricing a round-trip that does not happen. One control,
always filter-shaped, correct at three items and at three thousand; the tests
assert the rendered row count is a function of `limit`, not of collection size.
The server `/search` endpoint stays as the COLD path — used while the local
index has not hydrated, which is also the behaviour ItemDetail had before this
change. It is not a mode the user can select. Only that path is debounced: the
debounce exists to keep per-keystroke requests off the rate limiter, and the
warm path issues no requests to limit.
FENCES. The per-query `seq` moves into the picker and additionally fires on
unmount. The item-switch fence is NOT duplicated: ItemDetail already mounts
this region inside `{#key itemSlug}` (PLAN-2105 / TASK-2112), so a switch
destroys the picker and its continuation with it — a source-level test pins
that the picker stays inside an OPEN key block, since "a key exists somewhere
above" would pass with the picker outside all of them.
New behaviour beyond the extraction, both required by the unit:
- keyboard navigation (arrows / Enter / Escape) with aria-activedescendant,
which the inline search never had;
- a scoped picker opens with the target collection listed most-recently-
updated first, so it is useful before the user types.
Escape consumes only what it actually closes and stops propagation only then —
the page's Escape driver is a bubble-phase window listener feeding
`runTopEscape`, so an empty picker with no `oncancel` correctly lets the key
reach the pane. There is a control leg for exactly that.
`formatItemRef` is widened from `Item` to `Pick<Item, 'item_number' |
'collection_prefix'>`: the picker deals in `ItemIndexRow` (`Omit<Item,
'content' | 'moved_to'>`), which carries both fields but is not an `Item`.
Strictly wider, so every existing caller still type-checks.
The result-list CSS moves into the picker rather than being shared by class
name — Svelte scopes styles per component, and the note already in ItemDetail
records what reusing a class name across that boundary costs. The picker sizes
everything in `em` so a host sets the scale once on its own wrapper.
Verification: `npm run check` 0 errors (the 6 warnings are pre-existing and in
other files); 15 new component tests + 5 source-level consumption tests green.
|
||
|
xarmian
|
af24997c72 |
fix(web): re-resolve the follow target by id at fire time (BUG-2848)
Codex round 1, P2, and a real latent bug in the previous commit. Capturing the item OBJECT and reusing it 140ms later keeps a stale snapshot: a rename during the debounce changes the slug, `openItemPane` builds the URL from that slug, and the id-only existence check passes happily on the way to a dead URL. What is captured is now the IDENTITY — `targetId` — and the callback re-resolves the current row from `filteredItems` before opening it. That still follows a row that MOVED, which is the whole point of the fix, and still skips one that was DELETED, while picking up any change to the row itself. Also answering the round's second P2 in the spec rather than in code: the race is probabilistic and cannot be made deterministic without a seam in the page. The asymmetry is what makes that acceptable, and it is now written down — a round that misses the 140ms window still PASSES on a correct build, because the cursor moves, the pane follows and the intended row is where it should be. So missing costs power, not correctness; the failure mode is a false green, never a false red. Three rounds put a false green around 1 in 1700 against a build that loses the keypress 11 times in 12. pane-follow-live-list + pane-controller: 44/44 across both projects. |
||
|
xarmian
|
1a76531eb3 |
fix(web): capture the pane-follow target at keypress, not when the timer fires (BUG-2848)
The list is SSE-live and the pane-follow is debounced 140ms. The callback
re-read `filteredItems[focusedIndex]` when the timer fired, which made a
keystroke depend on the list holding still for those 140ms. It does not.
The failure was silent, and that is what made it hard to see. `j` advanced
`focusedIndex`; an item arriving during the debounce shifted every index below
it, sliding the PANED item down onto that very index; the callback read it
back, found "the focused row is already the paned item", and returned through
its own guard. No cursor move, no re-target, no error — a discarded keystroke.
The target is now captured BY IDENTITY at keypress time. The callback still
re-checks pane state and that the row still exists — identity, not position, so
a row that MOVED is followed correctly and only a row that was DELETED is
skipped.
MEASURED, because the first two explanations were both wrong.
The trail's diagnosis was that an insert leaves `focusedIndex` behind so `j`
lands on the already-open row. A snap-back $effect re-syncs the cursor to the
open item on every `filteredItems` change and prevents exactly that; a pin that
waited for the row to settle passed every candidate assertion.
So the second hypothesis was that the snap-back undoes the cursor move during
the debounce, and the fix was to suppress it while a follow is in flight.
Measured: 12 of 12 failures, WORSE than the 11 of 12 baseline. The stale index
lands on the paned item by itself; the snap-back was never the culprit.
Capture-at-keypress, same harness, same sweep size:
baseline (unfixed) 11/12 lost the keypress
suppress snap-back 12/12 lost the keypress
capture target at keypress 0/12
across all three measured properties — the pane re-targeted, the cursor moved,
and the pane landed on the row that was actually below the cursor.
The new spec CAUSES the race rather than waiting for it: it seeds a row above
the cursor, then lands a second insert across the keypress inside the debounce.
Three rounds per run, because one round caught the unfixed build 11 times in 12
and three make a false green not worth reasoning about. Counterfactual against
the unfixed controller: 3 of 4 desktop runs fail with the bug's signature —
`Expected: "DOC-16"` (the intended row) versus `Received: "DOC-15"` (the row
that was already open).
It asserts three things and none is redundant: `retargeted` alone passes if the
pane wanders anywhere; `cursorMoved` alone passes if the cursor moves and the
pane ignores it; `intended` is what pins the actual contract. A fourth that
suggests itself — "the pane agrees with the focused row" — passes VACUOUSLY on
the bug, since cursor and pane are then both stuck on the opened row. It was
measured doing that and is deliberately absent.
pane-controller.spec.ts is unchanged and still green (21/21). Its intermittent
failure was this defect, not the shared-workspace pollution it was filed as —
it just could not cause the race, so it only caught it when a sibling test's
seed happened to land in the window.
|
||
|
xarmian
|
58f50909af |
Merge pull request #1234 from PerpetualSoftware/feat/idea-2843-composer-quote-handle
feat(web): comment on a selection, with comments back under the item content (IDEA-2843) |
||
|
xarmian
|
e92f6f235d |
fix(web): the sidebar footer's Settings label wrapped once the GitHub link joined the row (BUG-2844)
MEASURED, not eyeballed. The desktop sidebar is 260px wide, less 24px of .sidebar-inner padding, so the footer row has 235px. Four 32px controls and four 8px gaps are fixed cost; .settings-btn is the only flex:1 item and got what was left — a 75px box, 51px of content after its 12px padding. The label "⚙ Settings" needs 56.3px. Five pixels short, so the gear and the word landed on separate lines and the row grew from 33.9px to 51.7px. The five pixels arrived with the GitHub link (IDEA-2711, PR #1229): a 32px control plus a fifth gap took 40px out of a box that had about 22px of slack. space-2 -> space-1 on the row returns 16px and the Settings padding another 8px, all of which lands in the one shrinkable item: a 75px content box against 56.3px of text, a 33% margin rather than the 5% either change alone would have left. Measured after: one line box, row height back to 33.86px. ONLY THE DESKTOP ARM WAS BROKEN, which the dispatch's "every width the layout supports" is what surfaced. Below 768px the sidebar is 280px AND the collapse button is gone, so the row carries four controls in 255px and the label had 135px to itself. It measured one line box before this change and still does — and both mobile legs of the new spec PASS against the unfixed CSS, which is what makes the desktop failures mean something. .github-btn also gains `flex-shrink: 0`, which every other control in the row already had. Harmless today — its automatic minimum size equals its 32px content box — but it made the row's one shrinkable item ambiguous, and .settings-btn is meant to be that item. THE TEST IS AN E2E SPEC BECAUSE NOTHING ELSE CAN HOLD IT. jsdom performs no layout, so a vitest render of Sidebar.svelte reports identical geometry with and without the bug. It asserts LINE BOXES rather than row height: height grows for other reasons and could stay put through a wrap, while Range.getClientRects() returns one rect per line, so the count is the question itself. Counterfactual run against the reverted declarations: desktop fails "Expected: 1, Received: 2", the shrinkable-control leg fails on the extra item, mobile stays green. Gates: web unit tests 115 files / 1978 tests green; svelte-check 0 errors (the 6 warnings are pre-existing, in files this does not touch); go build and go vet clean. The full Go suite was NOT re-run locally — no Go file changed — and CI covers it; naming the narrowing rather than reporting a leg I did not run. |
||
|
xarmian
|
7a7e9d669b |
fix(web): the selection toolbar is a row again (IDEA-2843)
Codex round 7. `.bubble-menu` had no layout of its own. Its buttons are themselves `display: flex`, so they are block-level and STACK — invisible while the menu held one action, wrong the moment Comment joined Extract. It also falsified the dimensions `positionMenu` clamps against, so the menu drifted over the text it points at. A row layout on the container; the expanded state opts out, since the extract form lays itself out. Layout is not observable in jsdom, so the assertion lives in the e2e: the two buttons share a row (y within 4px) and Extract sits to the right of Comment. Removing the row layout fails it in a real browser — verified. Declined, with the reason recorded in the code: "Comments 1+" can appear when the only unfetched entries are activity or versions. `+` reads as a LOWER BOUND, and a lower bound of 1 over exactly one comment is true. Knowing whether more comments exist means fetching the rest of the feed, so the alternative trades a true imprecise count for a confident wrong one. Gates: 119 files / 2005 unit tests, svelte-check 0 errors, e2e 2/2 on the selection spec against a rebuilt binary. Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo |
||
|
xarmian
|
4ebad409a2 |
fix(web): gate the composer's item identity during A→B navigation (IDEA-2843)
Codex round 6. 1. [P1] During an A→B navigation `item` still holds A while loadData fetches B, so ItemTimeline received A's itemId/collectionId beside B's itemSlug — and an attachment dropped in the composer inside that window is associated with the WRONG item. The wiring is PRE-EXISTING and identical on main. What changed is the exposure: the composer used to sit behind the Activity tab, and tabs reset to Details on an item switch, so reaching it inside the load window took a deliberate tab click. It is now on the tab you land on. Widening a latent hole is the same as opening one, so it is fixed here. Fixed by feeding honest inputs rather than adding a gate: the host passes itemId/collectionId only while `itemMatchesRef`, and ItemTimeline's canEdit already derives false without them, so the composer hides until the identities agree. 2. [P2] A load failure's banner outlived it — `loadMore` set `error` and never cleared it, so a successful retry left the failure sitting beside the entries it claimed had not loaded. Newly visible because the error is mirrored to the tabs now. Test boundary, stated: the new test covers ItemTimeline's half of the gate (no identity ⇒ no composer), verified by a control that flips its default to permissive. The host's half — the `itemMatchesRef ? … : undefined` — is not unit-testable here, since ItemDetail cannot be mounted in jsdom. Gates: 119 files / 2005 unit tests, svelte-check 0 errors. E2E: 37 passed across the five affected specs. An earlier run of that same set had one failure — capstone's "stale back-settle" nav test — which did not reproduce alone or in an identical re-run, and sits outside this diff's surface (history back-settle and drill targeting; nothing here touches either). Recorded rather than dropped. Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo |
||
|
xarmian
|
09a183844d |
fix(web): a submit no longer erases what arrived mid-flight; empty states wait for the last page (IDEA-2843)
Codex round 5. 1. Data loss, in the handle I added. `doSubmit` clears the composer on success, and a quote pushed in through `appendMarkdown` during the round trip was cleared with it — the quote simply vanished. The clear now requires the composer to still hold what was SENT. This also fixes a PRE-EXISTING loss by the same mechanism: text the user typed while a submit was in flight was erased too. It is the same class as the item-identity capture already guarding this path (PLAN-2105 / TASK-2112) — that one asks "is this still the same item", this one asks "is this still the same content". 2. Empty states appeared while more pages remained. A first page carrying only other kinds made a filtered view say "No versions yet." before the pages that would have contradicted it were fetched — a claim about the item made from one page of a feed. Both views now wait for the last page; until then the "Load more" button is what the reader sees. Gates: 119 files / 2004 unit tests, svelte-check 0 errors. The mid-flight fix has a negative control — restoring the unconditional clear fails the new test. Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo |
||
|
xarmian
|
b82c689eda |
fix(web): pagination honours the caller's filter; the host's Load more is styled (IDEA-2843)
Codex round 4.
1. Filtered pagination, fixed properly this time. Round 1 gave the TABS a
host-side retry wrapper and left the same defect in the owner's own
button: the Comments view could also page without showing a new comment.
Fixing the instance and not the class, twice on the same defect.
`loadMore(forKinds?)` now takes the caller's view filter and the hop loop
counts only entries that filter admits, defaulting to the component's own
`visibleKinds`. Whoever pressed the button says what progress means. The
host wrapper is deleted — MAX_EMPTY_HOPS already bounds the walk, so the
six-round loop on top of it was compensation for the missing filter.
Caught while wiring it: the owner's own button was `onclick={loadMore}`,
which passed the MouseEvent as `forKinds`. svelte-check found that; no
test would have.
2. The host's "Load more" was unstyled. Its class name matches
ItemTimeline's, but Svelte scopes styles per component, so the copied
NAME got browser defaults and nothing warned — CSS is the part no test
here asserts. Styles copied over with the reason recorded. Swept the
other class names the split moved across that boundary
(entry-list, compose, timeline-header, entry-count, empty): none is used
in the host, so the population is this one.
Gates: 119 files / 2003 unit tests, svelte-check 0 errors. The pagination
fix has a negative control — restoring the unfiltered break fails the new
test, which needed descending fixture timestamps to avoid passing for the
unrelated "cursor did not move" reason.
Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo
|
||
|
xarmian
|
abe8d93c10 |
fix(web): per-view titles, counts and empty states; a failed quote is not silent (IDEA-2843)
Codex round 3. 1. The split left every view describing the WHOLE feed. The comments section was headed "Timeline" with a count of every entry, so an item with three activity entries and no comments read "Timeline 3" over an empty list — a count of things the reader cannot see. And `showEmpty` was computed over the whole feed, so a view whose own slice was empty rendered nothing at all: no entries, no explanation. Title, count and empty state now describe what the view RENDERS. `title` and `emptyLabel` are props: "Comments" / "No comments yet." on Details, "No changes yet." / "No versions yet." on the tabs. The deliberate choice this reverses is mine — I passed showEmpty over the whole feed on the grounds that a filtered-out tab must not claim the item has no history. Right premise, wrong fix: the answer is to say something true about the slice, not to say nothing. 2. A failed quote was silently discarded. `appendMarkdown` returns false precisely so "did nothing" is distinguishable from "inserted" — and the only caller threw the boolean away and hid the menu, putting the silent no-op back exactly where the handle was built to remove it. A false now keeps the menu and the selection and reports it. Gates: 119 files / 2002 unit tests, svelte-check 0 errors, 14 e2e in desktop-chromium against a binary rebuilt from this tree. The discarded-return fix has a negative control — restoring the bare `onComment(...)` call fails the new test. (A first attempt at that control failed on shell quoting and silently ran against UNMUTATED code; it was re-run properly before this claim.) Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo |
||
|
xarmian
|
8d7fdc22f3 |
fix(web): consult the error state everywhere success was assumed (IDEA-2843)
Codex round 2, and both findings are follow-through misses on my own round-1 fix: I added an `error` to the mirrored feed and then left the code that assumes a load succeeded reading only `entries` and `hasMore`. - A failed load rendered the error AND "No timeline entries yet." One is a statement about the ITEM; a failed load knows nothing about the item, so the pair says something false beside something true. `showEmpty` now consults `error`. - The filtered "load more" wrapper retried a dead server up to six times per click. `loadMore()` catches and resolves, leaving `hasMore` true, so the loop had no reason to stop. It bails on `error` now. CONVE-18 sweep rather than the two named instances. The population is every consumer of the mirrored feed — six read sites in the markup, four in the wrapper. Two were defects (both above). One is a deliberate non-change: the "Load more" button still renders while an error is showing, because that is the retry affordance. The class also reaches the OWNER, where the same empty-beside-error contradiction is PRE-EXISTING on main and is fixed here too, since leaving it would mean the comments view kept the bug the tabs just lost. The regression test asserts the mounted owner's DOM. A first version recomputed `entries.length === 0 && !loading && !error` and asserted that — which passes whatever the component actually renders — so it was replaced. Gates: 119 files / 2001 unit tests, svelte-check 0 errors. Reverting the owner's guard fails the new test. Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo |
||
|
xarmian
|
5c0d8e9d34 |
fix(web): three codex round-1 findings on the timeline split (IDEA-2843)
All three are consequences of the two-view split that the split's own tests did not reach. 1. Multi-paragraph selections lost their paragraph breaks. The bubble menu builds `selectedText` with `textBetween(..., ' ')` because Extract uses it as an item TITLE, where newlines would be wrong — so quoting through it flattened two paragraphs into one run-on line. Worse, it made `toBlockquote`'s blank-line handling unreachable from production: that behaviour had a passing test and no call site that could produce it. The quote now re-extracts with a paragraph separator, and a test asserts the blockquote's blank line end to end. 2. Activity and Versions rendered a FAILED load as "No timeline entries yet." Loading and error were the owner's states and did not cross the mirror, so an unreachable server and an empty timeline looked identical on the tabs that only render entries. `error` joins the mirror; both states render. 3. "Load more" could visibly do nothing on a filtered view. The owner's hop loop stops as soon as a page adds an entry of ANY kind, so a page of pure comments ends it having added nothing to the changes view. Pre-existing on Versions; widened to Activity when comments moved off it. The host now pages until THIS view's list grows, bounded at six rounds — not fixed in the owner, which would have to know what the other view is rendering. Gates: 119 files / 2000 unit tests, svelte-check 0 errors. Fixes 1 and 2 have negative controls: reverting to the space-joined text fails 1, dropping `error` from the mirror fails 1. Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo |
||
|
xarmian
|
346a5c92a9 |
feat(web): Comment action on the selection toolbar, quoting into the composer (IDEA-2843)
GitHub #1228. Selecting a passage in an item's content now offers Comment beside Extract; it quotes the selection as a markdown blockquote into the comment composer under the content, appending after a blank line so an in-progress draft survives. The selection is NOT consumed — unlike Extract, which replaces it with a wiki-link — so a reader can quote the same passage twice or keep reading. - toBlockquote() prefixes EVERY line including blank ones. An unprefixed blank line ends a blockquote in markdown, so quoting two paragraphs without it silently drops the second out of the quote and leaves it looking like the commenter's own words. - The action renders when the host supplies `onComment`. A composer to quote into IS the capability; a flag that is always true beside a callback that is always supplied would be two ways to say one thing. - The button's accessible name is "Comment on selection". The composer's submit button is also named "Comment", and two identically-named buttons with different effects is a real ambiguity for name-based navigation — found by the first end-to-end run failing on a locator, not on behaviour. A NEGATIVE result, measured and kept. The action was briefly gated peek-independently, reasoning that a peeking master keeps a live composer (BUG-2263) but could not act on a selection. That state does not exist: a drag-selection in a peeking master RE-ACTIVATES it (focus-follows-editing, PLAN-2179 DR-2), so a selection and a frozen master never coexist. The gate is back on `mutationsEnabled`, and e2e/selection-comment-peek.spec.ts asserts the re-activation so a future change that makes selections survive the freeze turns red there instead of quietly reopening the question. Gates: 119 files / 1998 unit tests, svelte-check 0 errors, and 37 e2e in desktop-chromium — the 2 new ones plus the 35 in the four specs the comment relocation touched, run against a binary built from this tree. Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo |
||
|
xarmian
|
7aef246dcd |
feat(web): comments move under the item content; Activity keeps changes (IDEA-2843)
GitHub #1228. Reviewing an agent-written doc meant many small comments, and every one cost a trip to the Activity tab and back. TASK-2294's own spec put an activity preview on the Details panel; it never shipped, and the comments being tab-only is the half that was left. Dave ruled the full move. One component cannot render in two DOM locations, so ItemTimeline stays the SINGLE owner — one fetch, one SSE subscription, one composer — mounted under the content on Details rendering comments, and mirrors its feed out through a new bindable `feed` prop. The Activity and Versions panels render that same feed through a second TimelineEntryList. - The mirror publishes the WHOLE feed, not the owner's rendered slice. The owner renders comments only, so publishing `visibleEntries` would leave both tabs permanently empty with nothing to report. Tested, and the one-word mutation fails it. - `loadMore` rides in the mirror: pagination is a property of the ONE feed, and a tab that can show older entries but not ask for them is a dead end. - The kind partition is three shared constants with an exhaustiveness check, not literals at the mount sites. A kind in none of them renders NOWHERE — which is how note/decision shipped invisible the first time (BUG-2301). Adding a kind to TimelineEntry without routing it is now a build error or a failing test rather than a silent hole. - The comments section carries its own {#key itemSlug}: it left the block that used to provide that remount, and dropping the guard would have been invisible. It wraps only the timeline — the collab editor must never be keyed. Five e2e specs asserted comments behind the Activity tab and are updated. attachment-lifecycle's tab round-trip is preserved deliberately: its claim is that the panel is CSS-hidden rather than unmounted, so it now goes out to Activity and BACK rather than asserting against a hidden panel. Gates: 117 files / 1987 tests pass, svelte-check 0 errors. Both new properties have negative controls — publishing the rendered slice fails 1, unrouting a kind fails 2. Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo |
||
|
xarmian
|
f932469380 |
refactor(web): extract TimelineEntryList from ItemTimeline, no behaviour change (IDEA-2843)
Comments move under the item content on Details while Activity keeps changes and Versions keeps versions, so the one feed has to render in two DOM locations. One component instance cannot be in two places, and the constraint on this work is ONE subscription and ONE composer — so the rendered list becomes its own presentational component and ItemTimeline stays the single owner of fetching, SSE, pagination, the attachment probe, the paint fence and every mutation. This commit is the extraction only. Nothing moves location and no behaviour changes; the second mount site is the next commit. - TimelineEntryList.svelte: the entry loop, rail chrome, the five card branches and their CSS, lifted verbatim. `listEl` is bindable because the owner's delegated lightbox listeners and imperative image-a11y pass attach to the container and stay with the owner. `showEmpty` is passed rather than derived from the rendered entries, preserving the owner's condition over the WHOLE feed — a tab that filters everything out must render an empty list, not claim the item has no history. - The comment-card callbacks are optional here with no-op defaults: a list rendering no comments has nothing to hand them, and a card that could call one only renders when the owner supplied the real handler. Evidence, and the reason it counts: the existing suite passes UNCHANGED — 116 files, 1983 tests — and svelte-check reports 0 errors. A refactor whose only claim is "nothing changed" is exactly where an untouched suite is the right instrument, but only if it actually exercises the moved markup. It does: rendering the list over an empty array instead of `entries` fails 64 tests across 7 files. Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo |
||
|
xarmian
|
a2bdd904a5 |
docs,test(web): correct two claims the mutation matrix refuted (IDEA-2843)
Both corrections are to MY OWN rationale, not to behaviour. - The setContent-over-insertContentAt comment read as a defect avoided. Measured: swapping to insertContentAt leaves all five tests green, so both routes preserve the blockquote today. Restated as what it is — a preference for not depending on normalizeInline's leading-<p> rule — and marked explicitly unenforced. - The explicit `empty = editor.isEmpty` was inert: dropping it leaves the suite green, because setContent emits an update by default and onUpdate maintains the flag. Removed. The test asserting submit becomes enabled is the real guard, and it goes red if a tiptap bump flips that default. The test file's claim that its blockquote assertion catches an insertContentAt implementation was false for the same reason; it now states what the assertion does catch (a genuine flatten, verified) and what it does not. |
||
|
xarmian
|
9363dbb749 |
feat(web): imperative appendMarkdown handle on CommentEditor (IDEA-2843)
The selection toolbar's forthcoming Comment action needs to drop a
blockquote of the reader's selection into the ALREADY-MOUNTED composer.
The obvious route is a silent no-op: CommentEditor reads `content` once,
inside `new Editor({...})` in onMount, and has no $effect syncing it, so
writing the prop on a live composer drops the text with no error.
- appendMarkdown(markdown): appends after a blank line when a draft
exists, never replaces; returns false when there was nothing to insert
or no live editor, so a caller can tell 'inserted' from 'did nothing'.
- setContent (block parse) rather than insertContentAt: tiptap-markdown
overrides insertContentAt with { inline: true }, where a blockquote
survives only incidentally.
- A {#key} remount was the ruled-out alternative: it would destroy an
in-progress draft, which is what doSubmit's identity capture
(PLAN-2105 / TASK-2112) exists to protect.
Tests assert the quote TEXT and its blockquote tag, not the composer's
visibility — the broken version opens the composer too.
|
||
|
xarmian
|
100da86188 |
feat(nav): surface the repo link in the sidebar footer
A running instance had no visible connection to the project it is, so people went to a search engine. The reporter called it "purely a QOL addition"; what makes it worth a commit is that the link already EXISTED and was simply unreachable — buried in the user-menu dropdown under Resources. This is a discoverability fix, not a new capability. Icon-only, in `.footer-row` beside the collapse / theme / bell controls, because that row is already where this instance's chrome lives. The mark carries no accessible name of its own, so the anchor has an `aria-label` naming both the destination and the new tab, and the SVG is `aria-hidden` so it is not announced twice. `$lib/brand/links.ts` exists because the URL was already written twice in `UserMenuResources` and this would have been the third copy. A URL duplicated across every surface that shows it is a rename that goes half-applied. The module holds addresses only — list ORDER stays with the surface that renders a list, since the two lists are deliberately different lengths and the ordering contract lives in docs/brand.md. The tests assert the binding, and the last one asserts the property the others cannot: comparing a rendered href to the imported constant passes identically for a component that hardcoded the same string, because both sides end up as the same characters. That is a question about SOURCE, so a narrow guard reads the two consumers and asserts the literal is absent while `GITHUB_REPO_URL` is present — so the absence means "imported" rather than "the link was deleted". Named files rather than a glob, so it cannot fail for an unrelated file nor quietly stop covering these two. Closes #1168 |
||
|
xarmian
|
7f640a9c40 |
feat(attachments): render markdown and plain-text attachments in the viewer
The arm itself, plus the render seam and the browser proof. `renderMarkdownDocument` is a third thin wrapper in FRONT of the shared `marked` pipeline, following `renderMarkedWithAttachments`'s precedent rather than standing up a second renderer. It omits two things deliberately. No wiki-link resolution: an attached `.md` was authored elsewhere, so resolving its `[[brackets]]` against whatever workspace is showing it would silently retarget a foreign document's links at local items — BUG-2830's hazard entered through the front door. No attachment context, and it CLEARS the module-level context for the duration rather than merely leaving it alone: a nested call from a resolver or a `missing` hook that itself renders markdown would otherwise inherit the outer document's workspace and resolver. Save, clear, restore, mirroring the sibling wrapper. Sanitization is inherited, not re-derived, so an attached document is governed by the same allowlist as item content. Plain text does NOT go through the pipeline. A `.txt` renders in a `<pre>` as text, because interpreting a plain-text file's asterisks as formatting would misrepresent its content; Svelte escapes it, so that path emits no HTML at all. THE FALLBACK ARM'S CONDITION CHANGED, and this is the part to check hardest. It read `shownRenderer !== 'raster-image'`, which was correct while the union had one member and would have drawn "No preview available" over every text document the moment it had two. It is now `=== null` — the registry's actual "no renderer claims this" answer — which says what it means and stays correct when 'pdf' lands. INTERACTION, all of it found by review or re-reading rather than by the unit suite: - The wheel handler consumed every wheel before its exclusions, so the card could never scroll. The text exclusion returns BEFORE `preventDefault`, the opposite of every other exclusion there: the others want the wheel swallowed, this one wants it delivered. Scroll chaining to the inert page is stopped by `overscroll-behavior: contain`, so the guarantee the `preventDefault` provided is kept. - The full-bleed layer took `pointer-events: auto`, so a click on the empty area targeted it and backdrop-close was broken for this arm alone. The layer is inert; the CARD is the interactive surface. - `touch-action` INTERSECTS down the ancestor chain, so the card's `pan-y` could never override the stage's `none` and a phone could not scroll at all. The stage gives up its pan claim on this arm only. - The card had no tab stop. The viewer's arrow keys are its own next/previous navigation, so a keyboard-only user could open a document and never reach past its first screen. `tabindex="0"` plus `role="document"` and the filename label; the linter warning is suppressed narrowly with its reason at the site. - The focus-handoff effect tracked `loader.phase` — the IMAGE loader, which this arm disposes, so it never changes there. This is the only arm whose CONTENT is focusable, so a reload unmounted a focused link and stranded focus outside the modal. - `resolvedSize` and `revalidateToken` ride the load key SCOPED to the text arm. The key is shared, so an unconditional append re-ran the effect for the raster arm too and restarted image loads. The token is there because a parent RESTORE drives the image loader through the metadata probe's answer but cannot see a failed text GET's `error` phase — without it, a preview that 404'd while archived stayed permanently errored after the restore that fixed it. E2E, because three of these guarantees are CSS mechanisms and the jsdom suite injects no component styles — `getComputedStyle` there returns the engine default for every element, so two assertions written for them could not fail and were deleted rather than banked as coverage. `web/e2e/attachment-text-preview.spec.ts` covers the render, backdrop close, scrolling with no leak to the surface behind, and selection. Its FIRST RUN is what caught the feature rendering raw source, which the whole green unit suite could not see. CONVE-23 sweep on the prose this falsified: the ADMISSION vs THE ARM block enumerated 'raster-image loads bytes / null is no-bytes' as a two-way split, and the fallback arm described itself as "an entry the viewer cannot draw as an image". Both rewritten, plus a paragraph on why two byte-loading arms leave the no-bytes invariant unchanged in kind. `.pad-e2e-*/` is gitignored: a shared checkout runs concurrent suites, so each seat points `PAD_E2E_DATA_DIR` at its own, and those hold a generated encryption key and a multi-MB WAL. `item-attachment-strip.spec.ts` changes here because it is a CONSEQUENCE of this arm, not a separate concern. Two of its tests uploaded a `text/plain` file and asserted the viewer showed "No preview available" over it — true when written, false by design once text previews. CI caught them; my sweep had not, because I swept the unit tests and stated that boundary nowhere, which reads identically to a complete sweep (CONVE-18's amended half). The fix keeps each test's SUBJECT — both are producer→host wiring tests whose named subject is the fallback arm — and moves the vehicle to PDF, which keeps every property the fixture was chosen for while remaining unclaimed by any renderer. It carries a note saying it will go red again when PLAN-2393 builds the `'pdf'` slot, and that the red is the design: pick the next unclaimed type, never weaken the assertion. Closes #1169 |
||
|
xarmian
|
eca31efa9e |
feat(attachments): add the viewer text loader with two independent size gates
The markdown/plain-text counterpart to `viewerImageLoader`. The image
loader hands a URL to an `<img>` and lets the browser fetch, decode,
cache and cancel; text has no such element, so this module owns all
three of the things that come for free there:
- cancellation: no `src` reassignment drops the previous request, so
every load carries an `AbortController` and repointing aborts it
- staleness: a late `await` can resolve after the user has navigated, so
every completion is checked against the issuing token AND the active
id (the no-`{#key}` switch-safety class)
- the size bound: the browser will stream a 20 MB log renamed `.md` into
memory quite happily
TWO SIZE GATES, neither subsuming the other. The metadata gate refuses
before any request and saves the transfer. The response gate bounds the
bytes actually read, and exists because `LightboxImage.size_bytes` is
`number | null` BY DECLARATION — an emitter knows only what its own
surface gave it — so the metadata gate passes vacuously for any entry
that arrived without a size. Deleting either leaves a real hole, and the
tests name which hole each one covers.
The gates are restated at the request CHOKEPOINT rather than only at the
renderer, following the image loader's stated reason: the renderer
showing nothing is not the same as the loader asking for nothing. Tests
assert 'fetch was never called', not 'nothing was displayed'.
Fetching a type the server will not inline is not the risk; markdown is
served `Content-Disposition: attachment` and `fetch()` is unimpeded by
that. What keeps it safe is that the bytes never become active
same-origin content — PLAN-2393 DR-6 is honoured by the allowlist, which
admits neither HTML nor JavaScript, not by the disposition header.
Response-derived overflow reports NO figure. In that branch the declared
size is absent or demonstrably wrong, and echoing it produces "This file
is 10 B — too large to preview" — a sentence that reads as a viewer bug
rather than a file problem. Only the metadata branch, which has a
trustworthy number, reports one.
The bound is measured in BYTES on both the streaming and the
`response.text()` leg, and the streaming leg decodes with
`{ stream: true }` so a chunk boundary splitting a multi-byte character
cannot corrupt the text. The module is honest about where the fallback
is weaker: it bounds what is RENDERED everywhere, but what is HELD only
on the streaming path, whose fallback audience is jsdom.
20 tests. The stream fixture's `text()` THROWS, so an implementation
that ignored the stream and buffered everything cannot pass the tests
written to forbid that; the fixture reports read count and cancellation
so a test can assert the read STOPPED, since draining-then-measuring
reaches the same `too-large` and only the read count tells them apart.
`retry` is inert outside `error` — for `too-large` the guard is dead
code, but while LOADING it is the only thing preventing two concurrent
requests for one entry, a case a surviving mutant exposed.
Refs IDEA-2712
|