mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-11 13:28:57 +00:00
xarmian
07b2e439f2
* feat: the preflight's needs-value row names its relation target, and the copy dialog scopes its picker to the destination (TASK-2869)
U2b, per the day-55 ruling. Both blockers (U1 referent validation, U2 the
FieldEditor branch + picker) are in.
THE DEFECT. `ItemCopyPreflightNeedsValue` carried `type: "relation"` and no
target. FieldEditor gates its relation branch on `wsSlug` AND
`field.collection`, so a required relation in the destination reached the copy
dialog as a field it knew was a relation with no idea what to point at, and
rendered as FREE TEXT. Before U1 the copy stored whatever was typed.
SERVER. `collection` is added to the needs-value row, populated from the
DESTINATION schema's `def.Collection` — the only place it is known, since the
row is built from that schema. Additive and `omitempty`: a client that does not
read it is unaffected, and a row for a non-relation field is byte-identical to
before. Not a wire-version question, for the same reason
`models.ItemWriteWarnings` was not.
CLIENT. `toFieldDef` carries the collection through, and the FieldEditor call
passes `wsSlug={destWs}` — the DESTINATION, never the source. A relation
resolves at the destination, so the picker must list items the copy can
actually point at; that is same-workspace resolution AT the destination, not
the cross-workspace case PLAN-2857 rules out.
`relation` becomes collectable ONLY IF THE ROW NAMES ITS TARGET. Without a
collection, FieldEditor's gate renders the non-editable state, so offering the
row would produce a control that cannot be filled and a Confirm that cannot be
satisfied. Such a row now lands in the blocked list and the user is told which
field and why — the same disposition `multi_select` gets, for the same reason:
a control that silently cannot do its job is worse than an honest refusal.
TWO THINGS THIS UNIT TAUGHT ME THAT ARE NOT IN THE RULING.
1. THE WEB MUTANT SURVIVED, AND THAT IS WHY `isCollectable` MOVED. My first
version left the predicate inline in `CopyItemDialog.svelte`. Making
`relation` unconditionally collectable — the exact defect the negative leg
of the proving test is about — passed EVERY suite in the repo. That is
IDEA-2894's lesson arriving one unit later in the same file, so the
predicate now lives in `$lib/items/copyNeedsValue` with tests. Two mutants
die there: relation-always-collectable, and `multi_select` slipped into the
collectable set.
The Go half was pinned from the start (drop `Collection: def.Collection` ->
FAIL naming the empty value and the expected slug). Only the client half was
unpinned, and only because of where the code lived.
2. A U2-ERA TEST ASSERTED THE ABSENCE THIS UNIT CLOSES, and asserted it
CORRECTLY. `fieldEditorRelationCallers.test.ts` required that the dialog
pass no `wsSlug` and build its FieldDef from a shape with no `collection` —
which was the behaviour, and withholding `wsSlug` was what kept an unscoped
picker out. It also named this task by ref and told its successor to revisit
the gate WITH the change rather than let it drift. Inverted here: the block
now asserts the destination slug is passed, that the collection reaches the
FieldDef, and — the half that is easy to lose — that the dialog still
DELEGATES the collectability decision, so a future inlined predicate would
pass the unit tests and fail this.
Worth keeping: a test that pins a temporary absence should name what would
make it wrong. This one did, and that is the only reason its inversion was a
five-minute job instead of an argument about whether it was load-bearing.
Gates: `internal/server` ok 158.335s, `go vet` and `gofmt` clean,
`npm run check` 0 errors (6 pre-existing warnings), `make web-test` 127 files /
2123 tests. Postgres and CI are owed on this tip.
Claude-Session: https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8
* fix(cli): mirror the needs-value collection, and name the relation target in the CLI (TASK-2869)
TWO CONSUMERS I DID NOT SWEEP. The previous commit added `collection` to the
preflight's needs-value row and updated the server struct and the TypeScript
type — "both sides", as its own message put it. There are THREE sides.
`internal/cli` keeps a mirror of the preflight shape and
`TestItemCopyMirrorsMatchServerShapes` requires it to match the server field
for field. It failed in the Postgres gate, in a package the change did not
touch.
That is the third time this session a producer change broke a consumer I had
not enumerated, and the shape is always the same: I name the surfaces I edited
and call that the population. The instrument that would have caught it is not
"run more tests" but "grep for the type's name before claiming the sweep is
done" — `ItemCopyPreflightNeedsValue` appears in exactly three files and I
looked at two.
Mirrored, with a comment saying why it exists: a mirror that silently lags is a
mirror that lies, and the CLI renders these rows.
AND THE CLI NOW NAMES THE TARGET COLLECTION, which is the point of the unit on
the surface that has no picker at all. A row reading
owner_ref (Owner, relation) required — required, with no value…
tells a user a value is needed and nothing about what kind of value exists.
The dialog answers that with a scoped picker; the CLI had no answer. It now
prints the relation analogue of the `options:` line a select already gets:
owner_ref (Owner, relation) required — …
target collection: people
Test asserts the line appears for the relation row, appears EXACTLY ONCE with a
select row rendered alongside — so it cannot pass by printing unconditionally —
and that the select's own `options:` line still renders, so this did not
displace it.
Gates: `internal/cli` and `cmd/pad` green, build and gofmt clean. The full
Postgres run and CI are owed on this tip; the earlier PG run is the one that
caught the mirror and is superseded.
Claude-Session: https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8
* fix(web): the relation picker searches the preflight's canonical destination slug (TASK-2869)
Codex review, finding 3 of 3, and the only one of the three that belongs in
this unit.
`wsSlug={destWs}` handed `FieldEditor` a value that is NOT always a workspace
slug. An item can be opened through a workspace-UUID URL, the route parameter
is passed straight through as `sourceWsSlug`, and a same-workspace copy then
puts that UUID in `destWs`. `/search` resolves a workspace by SLUG only, so a
picker handed a UUID searches nothing and returns no results — a control that
looks usable, is not, and says nothing about why.
Now `pickerWsSlug`, which is the preflight response's own
`destination.workspace_slug`. The preflight IS the canonicalising round-trip:
the server resolved whatever it was given and answered with the real slug.
Falls back to `destWs` only before the first preflight returns, at which point
no needs-value row is rendered anyway.
The caller test asserts the prop AND the derivation, because asserting only the
prop would pass against a `pickerWsSlug` that was just `destWs` renamed.
THE OTHER TWO FINDINGS ARE REAL AND ARE FILED, NOT FIXED HERE.
IDEA-2898 — `ItemPicker` serves warm local-index results without re-authorising
them, so a collection whose access was revoked can still be listed. The cold
`/search` path is visibility-filtered and correct; the warm path is not. This
is PRE-EXISTING and applies to every caller of the picker, `ItemDetail`
included — last touched by TASK-2877, not by this unit. U2b widened the
exposure by adding a caller; it did not create the defect, and rewriting the
picker's cache-authorisation model inside a feature branch would be an
unrelated change riding along. The fix needs a decision about where the client
learns its access set from, which no current signal provides.
IDEA-2899 — a relation row can name a target collection that is DELETED or
UNREADABLE, so `isCollectable` says yes on a non-empty string and the user
meets a picker with nothing in it. I could not fix this correctly here, and the
reason is measured rather than assumed: the obvious test is to check the target
against `destCollections`, and that list is filtered by `canEditCollection` —
it is what the user may copy INTO. A relation TARGET needs only READ access, so
a perfectly usable target routinely is not in it. Using it would OVER-BLOCK,
refusing rows the user could have filled, which is a worse failure than the one
being fixed and invisible to whoever hits it. The right shape is probably the
server reporting the target's availability on the row it already builds — an
additive field on the same row this unit just changed, worth doing deliberately
rather than bolted on at the end of a branch.
Gates: `npm run check` 0 errors (6 pre-existing warnings), `make web-test` 127
files / 2123 tests. Postgres is running on this tip; CI is owed.
Claude-Session: https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8