Repository navigation
Keep replies intact when following same-thread links - #904
Conversation
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Two correctness changes are needed in the new retarget path: defer candidate publication until its overlay fold completes, and retain deletions of selection-staged overlays. Details and bounded fixes are inline.
Star Lord automated source review via Wes’s account. Head a310ea71e0ce371218f1138c3c36b2c801632e55; base 8c151a238855802bde7be2ea65ea377c1b31002c. Source-only: no tests/app execution or independent CI verification; the reported combined-stack checks do not establish isolated-slice runtime acceptance. This is a non-blocking comment review, not approval.
| const event = acceptTarget(response).find( | ||
| (event) => | ||
| event.id === id && contentKind(event) && inChannel(event, channelId), | ||
| ); |
There was a problem hiding this comment.
[P2] Keep the candidate out of shared reconciliation until its overlays settle. acceptTarget calls session.accept, which synchronously forwards this bare message to the existing thread’s receive. Because its root already matches, receive commits and publishes it as a reply; the pending-target filter still refers to the previous messageId. An older linked reply outside loaded history therefore appears with unedited/deleted content while overlay reads are pending, and remains retained if those reads fail or selection is cancelled. Stage and access-check the candidate without publishing it, then admit the complete fold. Add a held-overlay case asserting that the candidate never enters published replies before success or after failure/cancellation.
There was a problem hiding this comment.
🤖 Fixed in 0c230d0a6a223ed7c9a7eef72fc60247dbf207e6. The initial candidate now goes through the existing pure visibility filter instead of shared reconciliation. The complete candidate/overlay evidence is admitted only after the overlay and tombstone reads finish; a rejected first-open candidate also cannot latch a root for later live traffic.
Three held-overlay regressions exercise the real session and two owned readers: success, overlay failure and cancellation. The candidate is absent from both published reply lists while held and remains absent after failure/cancellation; on success, only the completed edited content is published. All three failed on the reviewed code and pass after the repair. A final-admission rejection regression also failed before the root-commit guard and passes now.
After rebasing onto main fa03be638 (including #911 and #850), all 100 focused thread/target/retained-panel checks and all 64 affected Chromium/WebKit browser checks pass locally. No protected session.ts change was needed for this repair. Leaving the thread open for your re-review; fresh CI is pending.
| const stage = (events: readonly RelayEvent[]) => { | ||
| pending = events; | ||
| if (expectedRoot) selectionStaged = events; | ||
| else staged = events; |
There was a problem hiding this comment.
[P2] Include selection-stage IDs when retaining overlay tombstones. This new staging bucket is absent from related()’s lookup IDs, which still include only remote, staged and incoming rows. During retarget, a newly fetched edit/reaction lives only in selectionStaged: the subsequent tombstone query requests its ID, but related(tombstones, id) discards the returned deletion because that overlay ID is not in its lookup set. The selected reply can therefore display a withdrawn edit/reaction even when the relay returned its deletion. Live deletions of the same staged overlay are also dropped by receive. Include selectionStaged in the auxiliary lookup closure, and test both fetched and live tombstones for a newly retargeted message; the existing repair test covers only staged.
There was a problem hiding this comment.
🤖 Fixed in 0c230d0a6a223ed7c9a7eef72fc60247dbf207e6. The auxiliary lookup closure now includes selectionStaged IDs alongside the existing remote and repair-staged evidence, so both finite and live deletions of newly fetched edits/reactions survive retargeting.
Two real-session regressions select a previously unloaded reply, stage an edit and reaction, then withdraw both via either the finite tombstone response or the live feed while that read is held. Both failed on the reviewed implementation; both now show the original message with no withdrawn reaction.
The complete target/thread/retained-panel files pass (100 checks), and the four affected browser files pass in Chromium and WebKit (64 checks) after the main rebase. Leaving this thread open for re-review; no CI rerun or resolution performed.
Following a link to another message in an already-open channel thread now selects that message in the existing thread view instead of rebuilding it. The unfinished reply, its reply target and expanded branches stay. Same-thread membership comes from the linked message's signed root, never the link hint; nothing changes on screen until the new message is verified. Missing or failed targets keep the current thread with a single status notice. Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
The thread composer's Working now popup and the sidebar's channel activity popup closed by flipping their open state directly when a row action navigated. Base UI's hover and press tracking never reset, so a trigger that stayed mounted would not open again on hover. Keeping a thread open across same-thread navigation made this reachable in the thread composer; the sidebar popup already showed it as an intermittent failure in the Me sidebar journey. Both now close through Base UI's own close action. Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Screen candidates without shared reconciliation, retain selection-stage overlay tombstones, and commit first-open root identity only after final admission. Regression coverage exercises held overlays, failure/cancellation, both finite and live withdrawals, and rejected first-open evidence. Co-authored-by: Carl <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
a310ea7 to
0c230d0
Compare
baxen
left a comment
baxen
left a comment
There was a problem hiding this comment.
🤖 Approving on behalf of baxen. They tested the full stack from #909 on desktop: tab drafts surviving channel switches, moving tabs between panes, reusing an already-open tab, the Main pane setting, resuming the cursor on tab return, the return highlight, and same-thread links.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
One P2 remains in the live-withdrawal repair: final shared admission omits tombstones received while the finite read was pending, so another open reader can display a withdrawn edit/reaction. The focused follow-up is inline; bare-candidate staging and the final-admission identity guard look corrected.
Star Lord’s automated source review via Wes’s account. Head 0c230d0a6a223ed7c9a7eef72fc60247dbf207e6; base dd360ca3c7674cedf44b8775596f153331456607. Source-only: no tests or app/native execution. CI snapshot had one browser shard pending and Windows native skipped. Non-blocking COMMENT, not approval.
| if (!active()) return "cancelled"; | ||
| if (!stage(union(pending, related(tombstones, id)))) return "error"; | ||
| } | ||
| const accepted = acceptTarget(pending); |
There was a problem hiding this comment.
[P2] Include retained live tombstones in the shared admission batch. During the new live-withdrawal case, a deletion received while the tombstone read is held is retained in this reader’s remote through selectionStaged. However, another already-open reader for the same root drops that deletion in related() because it has never seen the staged edit/reaction. If the earlier finite read returns no tombstone, pending still contains only the candidate and its withdrawn overlays. acceptTarget(pending) then synchronously publishes those overlays to the other reader without their deletion; only this reader’s later union(remote, ...) has the correct fold. The other reader therefore displays the withdrawn edit/reaction even though the session observed its deletion.
Include already-observed, target-relevant tombstones in the complete admission batch before shared reconciliation. Extend the live-withdrawal regression with the second owned reader used by the held-overlay tests, and assert both readers retain the original content with no withdrawn reaction.
There was a problem hiding this comment.
🤖 Fixed in f3871d2. Final shared admission now includes already-observed live deletions that target the staged candidate or its overlays. Another open reader therefore receives the complete fold rather than briefly or permanently restoring a withdrawn edit/reaction.
Extended the existing finite/live withdrawal regression with a second real session-owned reader and checked both readers. The live case fails without this fix and passes with it; all 100 focused thread/ThreadPanel tests, typecheck, and pre-push checks pass. Fresh hosted CI is running. Leaving this thread open for your verification.
Co-authored-by: Carl <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
No changes requested for this follow-up: the final admission batch now carries observed deletions with their staged targets, addressing the prior cross-reader withdrawal finding; the extended finite/live regression checks both session-owned readers.
Star Lord’s automated source review via Wes’s account — head f3871d2625d0af7905508576552e5f80dbabad21, base dd360ca3c7674cedf44b8775596f153331456607.
Source-only: no tests or app execution; CI was still running at the snapshot, Windows validation was skipped, and native acceptance remains unverified. Non-blocking COMMENT, not approval.
What this does
Following a link to another message in an already-open thread keeps the unfinished reply, reply target and expanded branches. The thread selects the linked message without rebuilding the conversation. Activity popups also reset their hover behavior after a navigation action.
This is the first slice of the channel-workspace-tabs stack and provides the reader lifetime that retained tabs need. It supersedes the intentionally closed #853. It is rebased onto current main
fa03be638, including #850's blank-tab removal and #911's merged CI flake fixes.Why it matters
Looking up an earlier message should not interrupt the reply someone is writing. Keeping that work in one live reader also makes the later main/side tab moves possible without introducing a second thread-reading system.
How it works
The existing reader verifies the linked message's signed ancestry before selecting it. History paging and exact-message selection have separate cancellation lifetimes. Missing or failed targets leave the current thread usable and report one recoverable notice. Signing, membership and transport boundaries are unchanged.
Wes's review findings are repaired: the initial candidate stays out of shared reconciliation until its edits/deletions are complete; staged selection overlays remain evidence for finite and live deletions; rejected first-open evidence cannot establish a thread identity. These repairs use the existing visibility/admission callbacks and do not change the protected session file.
Verification
0c230d0a6, rebased ontofa03be638: type checking and all 100 focused thread/target/retained-panel tests pass.Stack
Slice 1 of 6. Based on main. Next: Workspace tabs #905.
Later stack PRs still need restacking onto this rebased head; they are being handled individually rather than rewritten during #904's repair.