-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix: persist session renames through state.db sync #865
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import { useEffect, useState, useRef, useCallback, useMemo, memo } from "react"; | ||
| import { Plus, Search, X, ChatBubble, Trash, Pencil } from "../../assets/icons"; | ||
| import { useI18n } from "../../components/useI18n"; | ||
| import { confirmSessionRename } from "./confirmSessionRename"; | ||
|
|
||
| interface CachedSession { | ||
| id: string; | ||
|
|
@@ -393,54 +394,44 @@ function Sessions({ | |
|
|
||
| const confirmRename = useCallback( | ||
| async (sessionId: string, newTitle: string): Promise<void> => { | ||
| const trimmed = newTitle.trim(); | ||
| if (!trimmed) { | ||
| cancelRename(); | ||
| return; | ||
| } | ||
| // Capture old titles so we can roll back on failure. | ||
| let oldSessionTitle = ""; | ||
| let oldSearchResultTitle = ""; | ||
| // Optimistic update | ||
| setSessions((prev) => { | ||
| oldSessionTitle = prev.find((s) => s.id === sessionId)?.title ?? ""; | ||
| return prev.map((s) => | ||
| s.id === sessionId ? { ...s, title: trimmed } : s, | ||
| ); | ||
| const oldSessionTitle = | ||
| sessions.find((s) => s.id === sessionId)?.title ?? ""; | ||
| const oldSearchResultTitle = | ||
| searchResults.find((r) => r.sessionId === sessionId)?.title ?? ""; | ||
|
Comment on lines
+397
to
+400
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Two submissions for the same session can overlap while persistence is pending. If rename B succeeds before rename A fails, A's snapshot rollback runs last and restores the original title over B's optimistic title, leaving the modal inconsistent with the title persisted by B until another refresh. |
||
| await confirmSessionRename({ | ||
| sessionId, | ||
| value: newTitle, | ||
| currentTitle: oldSessionTitle, | ||
| isStillEditing: () => editingSessionIdRef.current === sessionId, | ||
| applyOptimistic: (title) => { | ||
| setSessions((prev) => | ||
| prev.map((s) => (s.id === sessionId ? { ...s, title } : s)), | ||
| ); | ||
| setSearchResults((prev) => | ||
| prev.map((r) => (r.sessionId === sessionId ? { ...r, title } : r)), | ||
| ); | ||
| }, | ||
| rollback: () => { | ||
| setSessions((prev) => | ||
| prev.map((s) => | ||
| s.id === sessionId ? { ...s, title: oldSessionTitle } : s, | ||
| ), | ||
| ); | ||
| setSearchResults((prev) => | ||
| prev.map((r) => | ||
| r.sessionId === sessionId | ||
| ? { ...r, title: oldSearchResultTitle } | ||
| : r, | ||
| ), | ||
| ); | ||
| }, | ||
| clearEditing: cancelRename, | ||
| inputRef: renameInputRef, | ||
| fallbackErrorMessage: t("sessions.renameFailed"), | ||
| persist: (id, title) => window.hermesAPI.updateSessionTitle(id, title), | ||
| }); | ||
| setSearchResults((prev) => { | ||
| oldSearchResultTitle = | ||
| prev.find((r) => r.sessionId === sessionId)?.title ?? ""; | ||
| return prev.map((r) => | ||
| r.sessionId === sessionId ? { ...r, title: trimmed } : r, | ||
| ); | ||
| }); | ||
| try { | ||
| await window.hermesAPI.updateSessionTitle(sessionId, trimmed); | ||
| } catch (err) { | ||
| console.error("Failed to rename session", sessionId, err); | ||
| // Rollback optimistic update | ||
| setSessions((prev) => | ||
| prev.map((s) => | ||
| s.id === sessionId ? { ...s, title: oldSessionTitle } : s, | ||
| ), | ||
| ); | ||
| setSearchResults((prev) => | ||
| prev.map((r) => | ||
| r.sessionId === sessionId | ||
| ? { ...r, title: oldSearchResultTitle } | ||
| : r, | ||
| ), | ||
| ); | ||
| } | ||
| // Guard: only clear editing state if the user hasn't started editing | ||
| // a different session while this request was in flight. | ||
| if (editingSessionIdRef.current === sessionId) { | ||
| setEditingSessionId(null); | ||
| setEditingTitle(""); | ||
| } | ||
| }, | ||
| [cancelRename], | ||
| [cancelRename, searchResults, sessions, t], | ||
| ); | ||
|
|
||
| const cancelDelete = useCallback((): void => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| import type { RefObject } from "react"; | ||
| import toast from "react-hot-toast"; | ||
| import { normalizeSessionTitle } from "../../../../shared/session-title"; | ||
|
|
||
| export interface ConfirmSessionRenameOptions { | ||
| sessionId: string; | ||
| value: string; | ||
| currentTitle: string; | ||
| /** True when this session is still the one being edited. */ | ||
| isStillEditing: () => boolean; | ||
| applyOptimistic: (title: string) => void; | ||
| rollback: () => void; | ||
| clearEditing: () => void; | ||
| inputRef: RefObject<HTMLInputElement | null>; | ||
| fallbackErrorMessage: string; | ||
| persist: (sessionId: string, title: string) => Promise<void>; | ||
| } | ||
|
|
||
| export type ConfirmSessionRenameResult = "cancelled" | "saved" | "failed"; | ||
|
|
||
| /** | ||
| * Shared inline-rename flow for the sidebar and Sessions modal: normalize, | ||
| * no-op cancel, optimistic update, durable persist, then clear editing on | ||
| * success or toast + rollback + refocus on failure. | ||
| */ | ||
| export async function confirmSessionRename( | ||
| opts: ConfirmSessionRenameOptions, | ||
| ): Promise<ConfirmSessionRenameResult> { | ||
| const normalized = normalizeSessionTitle(opts.value); | ||
| if (!normalized || normalized === opts.currentTitle) { | ||
| opts.clearEditing(); | ||
| return "cancelled"; | ||
| } | ||
|
|
||
| opts.applyOptimistic(normalized); | ||
| try { | ||
| await opts.persist(opts.sessionId, normalized); | ||
| if (opts.isStillEditing()) opts.clearEditing(); | ||
| return "saved"; | ||
| } catch (err) { | ||
| console.error("Failed to rename session", opts.sessionId, err); | ||
| opts.rollback(); | ||
| const message = | ||
| err instanceof Error && err.message | ||
| ? err.message | ||
| : opts.fallbackErrorMessage; | ||
| toast.error(message); | ||
| if (opts.isStillEditing()) { | ||
| setTimeout(() => { | ||
| opts.inputRef.current?.focus(); | ||
| opts.inputRef.current?.select(); | ||
| }, 0); | ||
| } | ||
| return "failed"; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The database has already committed when
writeCacheruns, but that function suppresses every write error. If disk access fails, this call still reports success whilesessions.jsonkeeps the old title; later incremental syncs do not repair established sessions because they select by unchangedstarted_atand refresh only message counts in the stale-entry pass.