Skip to content

fix(desktop): persist prompt history across sessions#2068

Merged
esengine merged 3 commits into
esengine:mainfrom
ferstar:fix/desktop-persisted-prompt-history
May 29, 2026
Merged

fix(desktop): persist prompt history across sessions#2068
esengine merged 3 commits into
esengine:mainfrom
ferstar:fix/desktop-persisted-prompt-history

Conversation

@ferstar

@ferstar ferstar commented May 27, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Persist desktop prompt-history navigation through session JSONL logs instead of composer-local memory
  • Add sidecar RPC support for older/newer prompt history cursor navigation
  • Preserve duplicate prompts as separate history entries and invalidate prompt-history cache when session metadata changes
  • Backfill Japanese plan feedback labels required by latest upstream desktop build

Fixes #2051

Verification

  • npm run test -- tests/session.test.ts desktop/src/ui/composer-at-popup.test.tsx desktop/src/App.test.ts
  • npm run test -- tests/public-api.test.ts tests/web-tools.test.ts
  • npm run typecheck
  • npm --prefix desktop run build
  • git diff --check
  • npm run verify (pre-push hook)

@esengine esengine left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Substance looks well thought out: cursor-based session walk, cache with proper invalidation on every write path, nonces to dodge stale results, reset on session change. Tests cover dupes, workspace scoping, and cross-session walks. Closes #2051 properly.

Three things before merge:

  1. Scope creep — please split. Two changes here are unrelated to prompt history persistence:

    • desktop/src/i18n/ja.tssendFeedback / skipFeedback / cancelFeedbackLabel / refineFeedbackLabel backfill. This is exactly the scope of #2069, which is open in parallel; they will conflict.
    • tests/web-tools.test.ts — a new vi.mock("../src/config.js", …) block. I can't see how that's related to this PR.

    Please drop both from this PR and rebase; #2069 will land separately and the web-tools change should have its own PR with a motivating description.

  2. Public API surface — is this intentional? You're adding promptHistoryStep, PromptHistoryCursor, and PromptHistoryEntry to src/index.ts (and bumping the public-api allowlist). That's a stability commitment to downstream consumers, but the function reads like an internal desktop helper. Unless there's a planned external consumer, I'd prefer keeping it un-exported and having desktop.ts import directly from ./memory/session.js.

  3. 5s cache TTL — minor. All mutating helpers correctly call invalidatePromptHistoryCache(). Add a one-line comment near the cache constant calling out that invalidation is the primary correctness mechanism and 5s is just a backstop for external mtime changes. (Optional, but helps the next reader.)

CI is green and the core change is good. Fix 1, decide on 2, and we're ready.

@ferstar ferstar force-pushed the fix/desktop-persisted-prompt-history branch 2 times, most recently from 9246765 to 6af1f15 Compare May 28, 2026 14:32
@ferstar ferstar force-pushed the fix/desktop-persisted-prompt-history branch from 6af1f15 to d4f8ecb Compare May 28, 2026 14:34
@ferstar ferstar requested a review from esengine May 28, 2026 15:07
@ferstar

ferstar commented May 28, 2026

Copy link
Copy Markdown
Contributor Author

Thanks, addressed. I rebased onto the latest main and kept this PR scoped to persisted prompt history: dropped the ja/web-tools changes, kept the prompt history helper out of the public API, added the cache TTL note, and replaced the config-backed prompt history path with the session-log/cursor implementation. CI is green and the branch is clean for re-review.

@ferstar

ferstar commented May 28, 2026

Copy link
Copy Markdown
Contributor Author

One extra note on the follow-up commit: I removed the config-backed prompt history path so there is a single source of truth. Since prompts are already persisted in session logs, the cursor-based session walk avoids duplicating history state and prevents config/session drift.

@esengine esengine left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Re-reviewed — both blockers from my original review are resolved:

  1. Scope creep dropped: the desktop/src/i18n/ja.ts backfill (that's #2069's territory) and the unrelated tests/web-tools.test.ts vi.mock are both gone. The diff is now focused on prompt-history persistence (session.ts/config.ts) + its composer wiring.
  2. Public API kept internal: src/index.ts is untouched — promptHistoryStep/PromptHistoryCursor/PromptHistoryEntry are no longer exported, so desktop.ts consumes them directly from ./memory/session.js without committing to a downstream-stability surface, as preferred.
    The core (cursor-based session walk, cache with per-write invalidation, nonces, reset-on-session-change) was already solid. CI green. Approving — merging if it's conflict-free.

@esengine

Copy link
Copy Markdown
Owner

Approving the content (both blockers resolved). It just needs a rebase — it now conflicts with main on src/config.ts (recent merges touched it). git fetch origin main && git merge origin/main, resolve config.ts, and I'll merge. No further changes needed.

@ferstar

ferstar commented May 29, 2026

Copy link
Copy Markdown
Contributor Author

Merged latest origin/main into the PR branch and resolved the conflicts. Head is now cceed18; PR is mergeable again. Local verify only hits the existing Exa-config-sensitive web-tools test on my machine, and that test passes under a clean HOME.

@esengine esengine merged commit 4e4faf2 into esengine:main May 29, 2026
4 checks passed
@zhangyapu1 zhangyapu1 mentioned this pull request May 29, 2026
Closed
@ferstar ferstar deleted the fix/desktop-persisted-prompt-history branch May 29, 2026 08:25
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.

Desktop: prompt history recall is in-memory only; it is lost across sessions and app restarts

2 participants