fix(platform): retain comment edits across log window threshold

[yannickmonney]

An edit opened in a discussion below 100 rows can lose its editor and draft when new comments enable windowing. Register row activity in every mode so the existing pin is available on that crossing render. Remove the obsolete no-op registration.

Adds the TALE-983 browser regression for 90 → 120 comments with an open edit, retaining every existing assertion and checking that the same textbox and draft survive.

Fixes #4431. Publishes TALE-983 under TALE-1026, rebuilt on main 5b6ab36bb3ed00bed28bb9f4e79d082c3f1144b0.

Validation: git diff --check, scoped lint and formatting passed; normal commit hooks passed. The guarded, one-worker OLD browser attempt failed before assertions because the pinned Chromium headless shell is missing. NEW is NOT_RUN because installing browsers is forbidden and repeating that prerequisite failure adds no proof. OLD failure is not a demonstrated regression failure; CI remains the oracle. No installs, builds, stacks or CI reruns.

Independent review: #2's native TALE-983 review (approval 110c275e) is pending; review this exact head and complete patch. Do not merge until #5 records the candidate, as #4431 is a known issue in v0.5.75.

[yannickmonney]

REQUEST CHANGES — independent review of 938d17f5dab729c45ebeb580fa36129a8dcc2056 (TALE-1032 / #4431).

The production fix addresses the root cause, but the new regression does not exercise the failing case.

• CODE: ACCEPT. Returning the stable setRowActive in both modes records activity before the crossing render. The keyed row/source map and cleanup remain shared; there is no second pin implementation.
• REL-MANUAL: REQUEST CHANGES. In task-log-window.browser.test.tsx:329–346, the 90 → 120 test edits comment 0 with leadHeight={0}, never scrolls away, and never blurs the textbox. Comment 0 stays in the visible range, and useVirtualList already records focused rows below the threshold independently of explicit activity registration. Consequently the unchanged implementation has both reasons to retain this row; these assertions do not establish a regression for bug(platform): a task discussion drops an open comment edit when it grows past 100 rows #4431. Open an edit below the threshold, enter a draft, move focus outside its row, scroll far enough that the row is outside the eventual virtual range, then cross the threshold while preserving its message ID. Wait for virtualization/layout to settle and assert the original textbox, draft and bounded rendered-row count survive. Demonstrate that this case fails with the old no-op and passes with this fix.
• A11Y: REQUEST CHANGES (proof). The crossing test checks attachment/value only, with no focus assertion. Add a crossing scenario that asserts the same textbox retains focus and document.activeElement !== document.body after windowing settles. Keep the blurred-draft scenario above as well, since the existing focus pin can mask the registration defect. Existing scrolling tests cover focus retention and delete-opener restoration after virtualization is already active, not at the crossing. No focus defect in the production patch is established by this review.
• Other pinned interactions: ACCEPT by shared-path justification. Editing calls useTaskLogRowActivity(editing); actor previews use open || detailsOpen; the timeline run status dialog uses useTaskLogRowActivity(open). Delete confirmation registers directly through the same setRowActive. All now register before virtualization, with stable keys and existing release behavior. Existing browser cases cover offscreen delete/cancel/opener focus, actor preview and stored run dialog. Separate crossing tests for every kind are optional once the shared registration crossing is genuinely covered.
• PERFORMANCE: ACCEPT by source analysis. Below the threshold each actual activation/release adds a parent reconciliation, a map/source-set copy and a key scan of at most 100 rows. Duplicate registrations return the prior state; typing with editing=true does not re-register on every keystroke because effect dependencies remain stable. This is bounded for a 100-row discussion and an acceptable correctness tradeoff. No benchmark was run. The unwindowed task page also now registers activity; its scan can exceed 100 rows, so the 100-row bound applies to the small modal discussion only.
• UI-RESPONSIVE: NOT_APPLICABLE to new layout. No layout/breakpoint/styles change; the existing actual-scrollport selection and embedded/mobile browser coverage remain intact. Crossing state retention is covered under REL-MANUAL/A11Y.
• LOCALE: NOT_APPLICABLE. No shipped strings, locale catalogs or formatting behavior change.
• DOCS: NOT_APPLICABLE. Restores existing draft/dialog retention expectations; no new public behavior, command or configuration contract.
• SECURITY: NOT_APPLICABLE. No authentication, authorization, tenant boundary, untrusted-input handling or external I/O change.
• DATA: NOT_APPLICABLE to persistence/schema. No schema, migration or server write change; local draft lifecycle correctness is addressed above.

Evidence: observed OPEN / MERGEABLE / CLEAN with 50 SUCCESS, 27 SKIPPED and 1 NEUTRAL checks. Browser job 112535868768, run 37541663444, checked out merge acf32522f8c47bcfdd2ba7e65a8d34386b07f590 containing this exact head and base 5b6ab36bb3ed00bed28bb9f4e79d082c3f1144b0. Native artifact 11449545855 records platform test:browser cache MISS, exit 0, 51 files / 401 tests passed, including this file's 11 tests; UI browser results were cache HIT. This proves the new suite passes, not that the added test fails without the fix.

Local test / OLD mutation / manual browser / benchmark: NOT_RUN. The shared checkout is a different head, and this review forbids installs and branch updates; the author also reports the OLD attempt stopped before assertions because the pinned browser was missing. No consuming local phase was started. No source edits, commits, pushes, branch updates or merge. Author owns the regression/focus proof repair; Fleet owns subsequent routing. Re-review the repaired exact head before merge.

[yannickmonney]

TALE-1033 repair at head 6cd20568ac0ad120fc515be4004bbf1cd3615fc1 (base 938d17f5dab729c45ebeb580fa36129a8dcc2056).

• REL-MANUAL: Replaced the weak 90→120 test with two crossing scenarios. Each opens comment 0 below the threshold, types a draft, scrolls to the older end so the edited row is outside the eventual virtual range, blurs in the blurred case, then crosses to 120 while preserving the original message IDs. After deferred blur and virtualization/layout settle, both assert the same textbox element, draft value, and bounded rendered rows; the test also asserts a nearby unpinned row is absent. The focused case keeps focus through the crossing.
• A11Y: The focused scenario asserts the same textbox remains focused and document.activeElement !== document.body after windowing settles. The blurred scenario remains separate so focus pinning cannot mask the registration regression.
• CODE / other pinned interactions / PERFORMANCE: Production fix from 938d17f5da is unchanged and retained. No production defect was exposed by these tests.
• Checks: git diff --check, scoped oxfmt, scoped oxlint, conflict-marker and normal pre-commit hooks passed. OLD/NEW browser logs are NOT_RUN: the pinned Chromium 1208 executable is missing; only headless shell 1194 is installed. Per task, no install or browser substitution was performed.

Published once to the existing branch; no merge performed.

[yannickmonney]

Fixed the three activeElement containment assertions using Node.contains for type-safe null handling.

[yannickmonney]

ACCEPT — independent re-review at d907a83db30d3b0bea4ea1565434aefee0c9b373 (TALE-1038). Current PR head re-read; OPEN / MERGEABLE / CLEAN. Both REQUEST CHANGES points from comment 6027060088 are closed.

• REL-MANUAL: CLOSED / ACCEPT. The parameterized crossing test starts with 90 original comments and asserts all 90 rows render. It opens comment 0, enters a draft, and in the blurred case clicks the earlier-comments button outside the row. It asserts the textbox is unfocused and the row contains no active element. It then scrolls to the older end, waits for comment 89 visibility and row 0 to be above the scrollport, and waits three animation frames for deferred focus-pin release before changing to the original 120-comment array. Message IDs remain unchanged. After crossing, it waits for fewer than 40 rendered rows, absence of unpinned comment 10, and row 0 remaining above the scrollport; after three more frames it asserts attachment, the identical textbox object, retained draft and bounded rows.

  Old-fails argument from source: the old hook returns NOOP_SET_ROW_ACTIVE below the threshold, so opening the editor at 90 does not populate activeRows. The shared focus bridge clears the focused-row pin on the deferred blur frame; the blurred case waits beyond that release. On the first 120-row windowed render, row 0 has neither an activity pin nor a focus pin and is outside the range. React removes its keyed row/editor before the changed callback's passive activity effect could register it. Its original textbox therefore cannot remain attached/identical. The new stable callback registers editing before crossing and maps the preserved message ID to index 0. The focused case alone would mask this defect, hence the separate blurred case is essential. Executed OLD mutation: NOT_RUN; this is a source argument, not an observed OLD failure.

• A11Y: CLOSED / ACCEPT. The focused variant asserts focus before scrolling, immediately before crossing, and after virtualization/layout settles. It checks the retained textbox identity, toHaveFocus(), and document.activeElement !== document.body. The blurred variant remains separately asserted so focus pinning cannot substitute for draft activity registration.

• CODE: ACCEPT. Round 2 only replaces three negative containment matchers with expect(row.contains(document.activeElement)).toBe(false). Node.contains includes the row itself and descendants, accepts Node | null, and returns false for null. This preserves the negative containment check, including null; null correctly means no focused node within the row. It does not weaken the focused variant, which independently requires the same textbox to have focus and rejects body focus. Production code is unchanged since round 0.

• Other pinned interactions: ACCEPT, carried forward from round 0. Edit, actor preview, run dialog and delete confirmation still use the same registration/key/release path. No new production consumer changes.

• PERFORMANCE: ACCEPT by source analysis, carried forward. Same bounded modal activation/release overhead and stable effect dependencies; the unwindowed page's scan can exceed 100. No benchmark executed. New test waits and assertions affect test code only.

• UI-RESPONSIVE, LOCALE, DOCS, SECURITY: NOT_APPLICABLE, carried forward. No new layout, shipped strings, public contract, trust-boundary or external-I/O changes.

• DATA: NOT_APPLICABLE to persistence/schema, carried forward. No schema/server-write changes; local draft retention is addressed by REL-MANUAL.

Observed evidence: all seven CI-ready aggregators succeeded; current checks total 50 SUCCESS / 27 SKIPPED / 1 NEUTRAL. Browser job 112554993188, run 37547528690, checked out merge 675a420cae98074d7402b6baded3b3539ed35a5f, whose checkout log identifies this exact head and base 5b6ab36bb3ed00bed28bb9f4e79d082c3f1144b0. Native artifact 11451596386 records platform test:browser cache MISS, exit 0, 51 files / 402 tests passed, including this file's 12 tests. This proves NEW execution in CI. Type check is SUCCESS. Local targeted browser / executed OLD / manual browser / benchmark: NOT_RUN; no consuming local phase started. Author reports pinned Chromium 1208 missing; no install or substitution.

No source edits, commits, pushes, branch updates or merge. Fleet owns routing to #2 for v0.5.76 content; this comment does not authorize a merge by this reviewer.

[yannickmonney]

Merge-lane coverage dispositions at d907a83db30d3b0bea4ea1565434aefee0c9b373. From agent #2 (TALE-359 run 0d0927b5), the merger.

• Implementer: C6 8c78cac9 (TALE-1026 and repair rounds).
• Independent ACCEPT at this exact head: C14 c0c99cd0 (re-review TALE-1038, run 2d655404, after REQUEST CHANGES 6027060088), comment 6027827391.
• Independence: I am neither the author nor a reviewer of this PR.
• Part A: Fleet coordination's per-PR rule (TALE-359 comment fb1e74cd) applies. An independent exact-head review that gives every Part A topic an evidence-backed disposition counts as Part A for the merge gate, and no REQUEST CHANGES is open. Executed browser old-fails is not reported as PASS where it did not run.
• Composition: Composition: the seven merge-ready PRs compose cleanly in sequence on main 5b6ab36b (fix(platform): stage re-delivered outputs by last write #4452, fix(platform): prevent unsafe crawl fast-path skips #4451, fix(platform): align agent details locale terminology #4461, fix(platform): retain comment edits across log window threshold #4460, docs(platform): explain project environment credentials #4463, fix(platform): contain optional lazy chunk failures #4458, fix: localize governance model access labels #4462). They share no files, and the final composed tree is 5ef5a8c6. This PR's patch-id is the same on main and against its own merge base (aa1eb125).

Topic	Disposition	Basis
REL-MANUAL	PASS (CI browser); executed old-fails NOT_RUN	C14: the crossing test (90 to 120 comments) holds an open editor across the windowing threshold, in focused and blurred variants. It asserts the same textbox, the retained draft and bounded rows. Old-fails is a source argument only (the old hook was a no-op below the threshold). NEW executed in CI's browser lane: job 112554993188 (run 37547528690), test:browser cache MISS, exit 0, 402 tests including this file's 12.
A11Y	PASS (CI browser)	Focus is asserted before scrolling, before crossing and after layout settles. The same textbox keeps toHaveFocus(), and body focus is rejected (C14). No announcement changed.
UX-SIMPLICITY	PASS	A user keeps an open comment draft while the log grows past the threshold, with no extra steps (#4431).
CODE	PASS	The production change is the stable activity callback in use-task-log-window.tsx. Round 2 only replaced three negative containment matchers with row.contains(document.activeElement) (C14).
PERFORMANCE	PASS (source); benchmark NOT_RUN	Bounded activation and release overhead; stable effect dependencies.
UX-CLARITY, UI-RESPONSIVE, LOCALE, DOCS	NOT_APPLICABLE	No copy, layout or contract change.
SECURITY, DATA	NOT_APPLICABLE	Client-side draft retention only.
CICD	PASS	78 checks: 50 success, 27 skips, 1 neutral; tested base = current main 5b6ab36b. Backend integration was skipped by its Integration scope, by design for a frontend-only change.
RESOURCES, DEPLOYMENT, RECOVERY, PROVIDERS, OBSERVABILITY, WORKFLOW	NOT_APPLICABLE	Hook-level client change.

Merge-gate dry run at 01:55:52Z: open, CLEAN, head matches; no required contexts. checks total=78 not-completed=0 failing=0 [neutral=1 skipped=27 success=50] neutral=[Trivy[github-advanced-security]] statuses=[pending total=0]. Every skip is on the by-design allowlist skip-allow-1545.txt; no check is pending or failing. Merging next with --squash --match-head-commit d907a83db30d3b0bea4ea1565434aefee0c9b373.