fix(desktop): preserve new task across renderer reloads - #2174
Conversation
|
Thanks for keeping the reload marker small and fail-soft. Checking it before bootstrap reconciliation is the right direction, and ordinary cold-start behavior remains unchanged when no marker is present. I found one broader ownership gap around the marker lifecycle:
Could we make successful Session creation clear the intent at an authority that survives renderer replacement, preserve the scoped draft required by the issue, and extend the E2E to cover the real label, draft restoration, and an in-flight first send? These cases appear to share the same lifecycle boundary rather than needing separate patches. |
|
Addressed in
Validation: desktop typecheck passed; desktop build passed; all 1,662 desktop unit tests passed; Biome and diff checks passed. Local Playwright is currently blocked before test discovery by the existing local Slack dependency mismatch ( |
|
Follow-up |
|
Thanks for tightening the lifecycle handling and adding draft coverage. After rechecking the updated flow, I think there is one remaining ownership issue, although I would treat it as a single non-P1 concern.
Could we avoid inferring ownership from the catalog set difference? The smallest safe option for this PR would be to keep the marker focused on preserving the new-task surface and draft, and remove cross-reload auto-adoption. Exact in-flight recovery could then follow separately using a stable pending Session or operation identity. If recovery remains in this PR, it should reconcile that exact causal identity on every authoritative refresh and cover an unrelated concurrent creation with a controlled test barrier. The rest of the reload and draft-preservation path looks good. |
46476c3 to
bc2fb71
Compare
|
Addressed in The reload marker is now limited to preserving the explicit new-task surface and its scoped draft. On renderer bootstrap, the selection lease is released immediately when that marker is present, so neither existing history nor an unrelated Session that appears concurrently can be adopted or clear the draft. The unit coverage now includes that unrelated-new-Session case, and the E2E that claimed in-flight first-send adoption has been removed; the empty-surface and draft-restoration E2E remains. Exact in-flight recovery is intentionally deferred until the create path exposes a stable pending Session or operation identity that can be reconciled causally on every authoritative refresh. Validation: desktop typecheck passed, Biome passed, and the directly affected test file passed 7/7. The full local Windows desktop suite did not complete within its 5-minute timeout and was terminated with the resulting |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for addressing the ownership concern. The revised implementation is substantially simpler: the reload marker now owns only the explicit new-task surface and its scoped draft, unrelated Sessions are no longer inferred or adopted from catalog differences, and ordinary cold-start restoration remains unchanged. The focused tests and full CI are green.
Two low-priority follow-ups remain, neither of which should block this PR:
- In-flight first-send recovery still lacks a causal pending Session or operation identity. One observable variant is navigating to another module while the first send is pending: the send may succeed without
setActiveIdrunning, leaving the reload marker until the user explicitly opens a Session. The Session remains safely persisted and accessible from history, so this is a narrow, recoverable edge rather than a merge blocker. - The host draft-persistence seam currently writes the raw draft to
sessionStorage, while the renderer-local draft store bounds entries to 120,000 characters. Sharing the same bound would avoid stale persisted drafts if a very large write exceeds the storage quota.
Exact in-flight recovery can follow separately once the create path exposes a stable pending Session or operation identity. Approving the scoped, safer solution here.
Summary
Closes #2170
Verification
npm run build:testnpm run format:checknpm test: product tests passed except one unrelated CLI timing test (marks an inherited running Bash card detached after rewind) that passed immediately when rerun aloneLocal E2E note
The new Electron E2E is included, but local Playwright collection on this checkout is blocked before discovery by the current Slack dependency pair:
@slack/socket-modecalls a missing@slack/web-api.addAppMetadata. CI remains the authoritative E2E run.