Skip to content

fix(desktop): preserve new task across renderer reloads - #2174

Merged
Astro-Han merged 4 commits into
apache:mainfrom
liugddx:fix/preserve-new-task-reload
Aug 5, 2026
Merged

fix(desktop): preserve new task across renderer reloads#2174
Astro-Han merged 4 commits into
apache:mainfrom
liugddx:fix/preserve-new-task-reload

Conversation

@liugddx

@liugddx liugddx commented Aug 4, 2026

Copy link
Copy Markdown
Member

Summary

  • keep an explicit empty new-task surface across renderer/HMR reloads with a session-scoped reload lease
  • release bootstrap history selection while that lease is active
  • clear the lease when a Session is explicitly selected or first-send creation succeeds
  • keep ordinary application cold-start history restoration unchanged
  • cover reload-vs-cold-start semantics and add a real Electron E2E journey

Closes #2170

Verification

  • npm run build:test
  • focused bootstrap selection tests: 6 passed
  • npm run format:check
  • full npm test: product tests passed except one unrelated CLI timing test (marks an inherited running Bash card detached after rewind) that passed immediately when rerun alone

Local 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-mode calls a missing @slack/web-api.addAppMetadata. CI remains the authoritative E2E run.

@Astro-Han

Copy link
Copy Markdown
Contributor

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:

  • the new E2E currently looks for 新建任务, while the accessible label is 新任务, so CI times out before reaching the reload assertions;
  • first-send success clears the marker only through the originating renderer’s setActiveId. If that renderer reloads or the user navigates away before the send resolves, the Session can be created successfully while the marker remains, hiding the new Session on the next reload and allowing a duplicate retry;
  • fix(desktop): preserve the new-task surface across renderer reloads #2170 also expects the draft to remain attached to the new task, but the composer draft is still stored in a renderer-local Map and is lost on a full reload.

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.

@liugddx

liugddx commented Aug 5, 2026

Copy link
Copy Markdown
Member Author

Addressed in 74e07f4f — thanks for catching all three lifecycle gaps.

  • The E2E now uses the real accessible name, 新任务.
  • The reload intent records the authoritative Session baseline. On renderer replacement, bootstrap reconciles it against the persisted Session catalog; if first-send creation completed while the old renderer disappeared, the new renderer adopts that Session and retires the marker instead of reopening history or permitting a duplicate retry. The reload-aware lease remains alive across the initial/mounted snapshot handoff so an in-flight create can appear in the later snapshot.
  • The intent now carries only the scoped new-session draft in sessionStorage. Composer accepts an optional host persistence seam, while existing-session drafts retain their current renderer-local behavior.
  • E2E coverage now verifies draft restoration and reload during an in-flight first send, in addition to the empty new-task reload case. Unit coverage verifies baseline reconciliation and scoped draft storage.

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 (@slack/socket-mode calling missing @slack/web-api.addAppMetadata), so I am relying on the clean CI environment for the Electron assertions.

@liugddx

liugddx commented Aug 5, 2026

Copy link
Copy Markdown
Member Author

Follow-up 46476c3f fixes the CI-only test failure. The failing assertion required .maka-composer-workspace-dock to exist even though the fixture has no selected project, so the dock is legitimately absent. That assertion was unrelated to #2170 and failed before reload. I removed only those two dock assertions; the spec still verifies the empty new-task surface, scoped draft restoration after reload, and the separate in-flight first-send adoption path.

@Astro-Han

Copy link
Copy Markdown
Contributor

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.

sessionCreatedSinceNewTaskIntent() treats the first Session absent from the baseline as belonging to this new-task intent. The catalog proves that a Session is new, but not who created it, so a concurrent Bot or another creation path could be adopted and clear the user’s draft. Conversely, reconciliation currently runs only for the bootstrap snapshots; if the intended creation appears later through sessions:changed, the list refresh does not run the lease again. The current E2E reloads immediately after Enter without controlling the creation boundary, so it does not distinguish these timings.

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.

@liugddx
liugddx force-pushed the fix/preserve-new-task-reload branch from 46476c3 to bc2fb71 Compare August 5, 2026 01:10
@liugddx

liugddx commented Aug 5, 2026

Copy link
Copy Markdown
Member Author

Addressed in bc2fb712. I removed the catalog set-difference ownership inference and the cross-reload auto-adoption path.

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 EPIPE, so I am relying on CI for the full-suite result.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 setActiveId running, 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.

@Astro-Han
Astro-Han merged commit 5e03a7b into apache:main Aug 5, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(desktop): preserve the new-task surface across renderer reloads

2 participants