mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-21 18:13:26 +00:00
xarmian
38aa872864
* fix(web): debounce typed-input field saves to stop per-keystroke activity rows (BUG-1466)
Text / number / URL fields in FieldEditor wired oninput directly to
onchange, so every keystroke became an item PATCH and an activity row.
Typing `ui/editor/tiptap` into a `component` field produced a 30-step
keystroke chain in the audit metadata (visible on BUG-1419's timeline).
Wrap the typed-input branches in a 500ms idle debounce, flush on blur
so tabbing away commits immediately, and flush on unmount so navigation
never drops a pending value. Discrete inputs (select / date / checkbox,
number ±1 buttons) keep firing on the user action — they aren't typing.
Mirrors the markdown content debounce pattern in the detail page.
* fix(activity): collapse same-field runs in merged changes metadata + unify diff separator (BUG-1466)
Follow-up to the web-side typing debounce. Two related changes:
1) collapseChanges() walks the merged "; "-delimited changes string and
collapses runs of consecutive same-field entries into a single
"field: first-old → last-new". Drops net no-ops (typed then backspaced).
When the web-side debounce in FieldEditor still produces multiple
PATCHes within the 5-minute coalesce window — or for older rows that
pre-date the debounce — the timeline now reads as one transition
instead of a chain. Run-based (not global) collapse so interleaved
edits on different fields keep their chronology.
2) diffFields now joins entries with "; " instead of ", " so the joiner
is consistent with mergeActivityMeta and TimelineActivityCard.svelte's
split delimiter. Multi-field PATCHes previously rendered as a single
unparseable blob in the web timeline because the parser only split on
";" — fixed as a side-effect.
Adds TestCollapseChanges (10 cases including the BUG-1419 repro) and
TestMergeActivityMeta_CollapsesSameFieldRun. Updates the existing
TestDiffFieldsPrimitives expectation to match the new joiner.
* fix(fields,activity): two follow-ups per Codex review (round 1)
[P1] FieldEditor.svelte::handleNumberStep
The ±1 buttons computed `(Number(value) || 0) + delta` AFTER calling
flushPendingSave(). But `value` is the parent prop — flushing fires
onchange asynchronously, so at the moment of the step computation
the prop still holds the pre-typed value. Typing 10 over 5 and
clicking + would flush 10 then send 6, overwriting the typed value.
Compute `base` from `pendingValue` (if hasPending) BEFORE clearing
the timer state, then send `base + delta` in one onchange call.
[P2] activities.go::collapseChanges
The drop-net-no-op step removed entries where `from == to`. But
diffFields intentionally emits same-display entries for
same-cardinality structured-field replacements like
`implementation_notes: (1 note) → (1 note)` (see
TestDiffFieldsSameCardinalityArrayChangeStillReported) — the labels
match because formatChangeValue summarizes by count, not content,
but the underlying data did change. Dropping them silently hides
real updates from the activity feed.
Track `mergedCount` per entry: increment when collapsing a run,
initialize to 1 on parse. Only drop when `mergedCount > 1 && from == to`
— i.e. only when the no-op resulted from collapsing multiple input
segments (the typed-then-backspaced case).
Tests: 2 new TestCollapseChanges cases (single structured-field
preserved, interleaved structured-field around a typed run).
* fix(fields,activity): two follow-ups per Codex review (round 2)
[P1] FieldEditor.svelte number stepper focus race
The number ±1 buttons race with the input's onblur handler: blur
fires before click in the natural focus-transfer flow, so
flushPendingSave clears hasPending → handleNumberStep reads stale
`value` from the parent prop → typing 25 over 10 and clicking +
sends 25 then 11, losing the typed value.
Add onmousedown={preventDefault} on both ±1 buttons. Mousedown
precedes blur, and preventDefault on mousedown suppresses the
natural focus transfer — the input keeps focus through the click,
so hasPending survives until handleNumberStep reads it.
[P2] collapseChanges still drops repeated same-display structured runs
Round 1's mergedCount>1 rule still dropped runs like
`implementation_notes: (1 note) → (1 note); implementation_notes: (1 note) → (1 note)`
— two real updates whose display strings happen to match because
formatChangeValue summarises array-valued fields by count. Each
PATCH represented a different underlying note (diffFields uses
reflect.DeepEqual to detect that), but the merged display showed
no transition.
Track `hadTransition` per entry: true iff the run had a display-
level transition (initial from != to, or a subsequent entry's `to`
differed from the anchored from). Drop only when
mergedCount > 1 && from == to && hadTransition — i.e. only true
net-cancellations (typed-then-backspaced). Same-display structured
repeats stay; real `foo → bar → foo` swings still drop.
Tests: 2 new TestCollapseChanges cases (repeated same-display preserved,
real foo→bar→foo swing still dropped).
* fix(fields,activity): two follow-ups per Codex review (round 3)
[P1] FieldEditor cross-item leak via debounce timer
When the parent reuses a FieldEditor instance across an item swap
(same schema, same field.key, different item — common when
navigating between items in the same collection), the parent's
`updateField` closure reads `item.id` at CALL time. A pending
timer set while item A was active would fire after item B mounted,
patching B with A's typed value.
Two-pronged fix in FieldEditor:
- Add a $effect that tracks the `value` prop and drops any pending
save the moment the parent re-props us. The user actively typed
for a now-stale context; aborting is safer than silently writing
to the new context. Also covers external collab/SSE rebases of
the same field on the same item.
- Switch the unmount cleanup from flush → drop. When the parent
navigates to an item whose schema lacks this field, the
FieldEditor unmounts AFTER the parent's `item` has already been
replaced, so a final onchange call would route through
updateField → wrong item. Blur is the supported commit gesture
(clicking elsewhere within the page, ±1 buttons, tab-out — all
flush eagerly); unmount-without-blur is treated as "user
abandoned the edit."
[P2] collapseChanges still dropped structured count-return swings
Round 2's hadTransition rule still dropped a run like
`implementation_notes: (1 note) → (2 notes); implementation_notes: (2 notes) → (1 note)`
— the user added a note then removed the original, ending with a
different single note. The merged display reads `(1 note) → (1 note)`
with hadTransition=true, indistinguishable from a typed-then-
backspaced cancellation. But formatChangeValue summaries are
LOSSY: same display label can wrap entirely different raw values.
Add a `hasLossySummary` flag per entry — true iff either `from`
or `to` matches the `(text)` format formatChangeValue emits for
structured fields. Propagate stickily across the run (any lossy
entry pins the whole run). Skip the drop step when the run
carries lossy summaries — we can't recover the raw delta from the
merged string, so we preserve the entry rather than risk hiding
a real change.
Tests: 2 new TestCollapseChanges cases (count-return swing
preserved, lossy-on-one-side still pins the run).
* fix(fields): demote hasPending from \$state to plain let — \$effect was cancelling every keystroke (round 4 [P1])
Critical regression in round 3: the value-track \$effect read
\`hasPending\` inside its body, which Svelte 5 promotes to a reactive
dependency. scheduleSave() setting \`hasPending = true\` retriggered
the same \$effect, whose body cleared the typing timer + pendingValue
+ hasPending before the debounce could fire. Net effect: typing into
any text / number / URL field was silently dropped — onchange never
ran, the field never saved.
hasPending is only read from imperative handlers (scheduleSave,
flushPendingSave, handleNumberStep, the value-track \$effect, the
unmount cleanup) — never from a template or other reactive context.
Demoting it to a plain \`let\` removes the unwanted subscription while
preserving the round-3 behaviour: external value-prop changes still
trigger the \$effect (it tracks \`value\`), the body reads hasPending
imperatively to decide whether to clear pending state.
No tests added — this is a Svelte reactivity edge case that can't
be unit-tested without a DOM. svelte-autofixer's pre-existing
"variable assigned inside \$effect" suggestion previously flagged the
hasPending mutation; that signal is gone now.
Per Codex review round 4.