fix(desktop): persist prompt history across sessions#2068
Conversation
esengine
left a comment
There was a problem hiding this comment.
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:
-
Scope creep — please split. Two changes here are unrelated to prompt history persistence:
desktop/src/i18n/ja.ts—sendFeedback / skipFeedback / cancelFeedbackLabel / refineFeedbackLabelbackfill. This is exactly the scope of #2069, which is open in parallel; they will conflict.tests/web-tools.test.ts— a newvi.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.
-
Public API surface — is this intentional? You're adding
promptHistoryStep,PromptHistoryCursor, andPromptHistoryEntrytosrc/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 havingdesktop.tsimport directly from./memory/session.js. -
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.
9246765 to
6af1f15
Compare
6af1f15 to
d4f8ecb
Compare
|
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. |
|
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
left a comment
There was a problem hiding this comment.
Re-reviewed — both blockers from my original review are resolved:
- Scope creep dropped: the
desktop/src/i18n/ja.tsbackfill (that's #2069's territory) and the unrelatedtests/web-tools.test.tsvi.mockare both gone. The diff is now focused on prompt-history persistence (session.ts/config.ts) + its composer wiring. - Public API kept internal:
src/index.tsis untouched —promptHistoryStep/PromptHistoryCursor/PromptHistoryEntryare no longer exported, sodesktop.tsconsumes them directly from./memory/session.jswithout 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.
|
Approving the content (both blockers resolved). It just needs a rebase — it now conflicts with main on |
|
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. |
Summary
Fixes #2051
Verification