diff --git a/apps/vscode-e2e/src/visual/__screenshots__/electron-chat-dark-sidebar.png b/apps/vscode-e2e/src/visual/__screenshots__/electron-chat-dark-sidebar.png index 3791a71175..dfc6fee680 100644 Binary files a/apps/vscode-e2e/src/visual/__screenshots__/electron-chat-dark-sidebar.png and b/apps/vscode-e2e/src/visual/__screenshots__/electron-chat-dark-sidebar.png differ diff --git a/apps/vscode-e2e/src/visual/electron.visual.ts b/apps/vscode-e2e/src/visual/electron.visual.ts index 3ea34742e7..a71fe80d2e 100644 --- a/apps/vscode-e2e/src/visual/electron.visual.ts +++ b/apps/vscode-e2e/src/visual/electron.visual.ts @@ -89,8 +89,21 @@ for (const scenario of scenarios) { const tokenCountMask = webviewFrame .frameLocator("iframe") .locator('[data-testid="context-tokens-count"],[data-testid="context-window-size"]') + // The time under each message is the real send time, so it changes on every run, and so does the length + // of its text ("Today 9:05 AM" and "Yesterday 12:36 PM" are not equally long). A mask covers the + // element's box, so that box needs a fixed size as well: without it the masked area differs between + // runs, in width and, when the text wraps onto a second line, in height. + await contentFrame.evaluate(() => { + document.querySelectorAll('[data-testid="message-timestamp"]').forEach((time) => { + time.style.setProperty("width", "160px", "important") + time.style.setProperty("white-space", "nowrap", "important") + time.style.setProperty("overflow", "hidden", "important") + }) + }) + // Mask it, as the token counters above, and keep the rest of the scene byte-stable. + const messageTimeMask = webviewFrame.frameLocator("iframe").locator('[data-testid="message-timestamp"]') await expect(sidebar).toHaveScreenshot(`electron-${scenario.name}-sidebar.png`, { - mask: [tokenCountMask], + mask: [tokenCountMask, messageTimeMask], }) if (scenario.webviewSnapshot) { diff --git a/packages/types/src/global-settings.ts b/packages/types/src/global-settings.ts index eb51c1d67d..f5757a51c7 100644 --- a/packages/types/src/global-settings.ts +++ b/packages/types/src/global-settings.ts @@ -280,6 +280,21 @@ export const globalSettingsSchema = z.object({ * When unset (or `null`), the webview inherits VS Code's `--vscode-font-size`. */ chatFontSize: z.number().int().min(8).max(32).nullish(), + /** + * Clock format used when rendering timestamps. "auto" follows the display language's locale. + * @default "auto" + */ + timeFormat: z.enum(["auto", "12h", "24h"]).optional(), + /** + * How dates are written. "auto" follows the display language's locale. + * @default "auto" + */ + dateFormat: z.enum(["auto", "mdy", "dmy", "ymd"]).optional(), + /** + * Show relative times ("5 min ago") in task history; when off, show the absolute date/time. + * @default true + */ + showRelativeTimes: z.boolean().optional(), /** * Controls the keyboard behavior for sending messages in the chat input. * - "send": Enter sends message, Shift+Enter creates newline (default) diff --git a/packages/types/src/vscode-extension-host.ts b/packages/types/src/vscode-extension-host.ts index 40f83fbaa2..8a006c0639 100644 --- a/packages/types/src/vscode-extension-host.ts +++ b/packages/types/src/vscode-extension-host.ts @@ -50,6 +50,7 @@ export interface ExtensionMessage { | typeof VsCodeLmModelsMessageType.vsCodeLmModels | typeof VsCodeLmModelsMessageType.githubCopilotModels | typeof VsCodeLmModelsMessageType.githubCopilotSignInResult + | "settingsSaveResult" | "vsCodeLmApiAvailable" | "updatePrompt" | "systemPrompt" @@ -341,6 +342,9 @@ export type ExtensionState = Pick< | "includeTaskHistoryInEnhance" | "reasoningBlockCollapsed" | "chatFontSize" + | "timeFormat" + | "dateFormat" + | "showRelativeTimes" | "enterBehavior" | "includeCurrentTime" | "includeCurrentCost" diff --git a/src/core/config/ContextProxy.ts b/src/core/config/ContextProxy.ts index 1250a7baca..e53f60fb83 100644 --- a/src/core/config/ContextProxy.ts +++ b/src/core/config/ContextProxy.ts @@ -30,6 +30,37 @@ type RooCodeSettingsKey = keyof RooCodeSettings const PASS_THROUGH_STATE_KEYS = ["taskHistory"] +/** `ContextProxy.setValues` could not store every key: which ones it stored, which it did not, and the first failure. */ +export class SettingsWriteError extends Error { + constructor( + readonly committed: string[], + readonly failed: string[], + override readonly cause: unknown, + ) { + super( + `Could not store ${failed.join(", ")}${committed.length > 0 ? ` (stored: ${committed.join(", ")})` : ""}: ${ + cause instanceof Error ? cause.message : String(cause) + }`, + ) + this.name = "SettingsWriteError" + } +} + +/** How long the read-back of a failed secret may take: the write error must reach the caller even if it never settles. */ +const SECRET_READBACK_TIMEOUT_MS = 5_000 + +const withinReadBackDeadline = (read: PromiseLike, key: string): Promise => + new Promise((resolve, reject) => { + const timer = setTimeout( + () => + reject(new Error(`Reading back ${key} did not finish in ${SECRET_READBACK_TIMEOUT_MS / 1000} seconds`)), + SECRET_READBACK_TIMEOUT_MS, + ) + Promise.resolve(read) + .then(resolve, reject) + .finally(() => clearTimeout(timer)) + }) + export const isPassThroughStateKey = (key: string) => PASS_THROUGH_STATE_KEYS.includes(key) const globalSettingsExportSchema = globalSettingsSchema.omit({ @@ -360,13 +391,42 @@ export class ContextProxy { return value !== undefined ? value : defaultValue } + /** + * The write that last set each cached key, so a failed write only corrects the cache while it is still the latest: + * a newer write of the key has its own outcome, and an older failure must not replace its value. + */ + private readonly stateWriteOwner = new Map() + private readonly secretWriteOwner = new Map() + /** How many writes of each secret were started. Unlike the owners, it is not cleared when a write is over. */ + private readonly secretWriteCount = new Map() + updateGlobalState(key: K, value: GlobalState[K]) { if (isPassThroughStateKey(key)) { return this.originalContext.globalState.update(key, value) } + // The cache is updated first, so a read right after the call sees the value. If the store rejects, the cache is + // read back from the Memento (`globalState.get`) so that it shows what the Memento holds. This is best effort: a + // Memento can keep the attempted value in memory after a rejected update, in which case the cache and the + // Memento agree and only the durable copy, which cannot be read, may differ until the key is written again. + // The entry of the key is removed when the write is over (if it is still the latest), so the map only holds the + // writes that are in flight. An update that throws at once is turned into a rejection, so it is handled the same. + const owner = {} + this.stateWriteOwner.set(key, owner) this.stateCache[key] = value - return this.originalContext.globalState.update(key, value) + const release = () => { + if (this.stateWriteOwner.get(key) === owner) this.stateWriteOwner.delete(key) + } + return (async () => this.originalContext.globalState.update(key, value))().then(release, (error: unknown) => { + try { + if (this.stateWriteOwner.get(key) === owner) { + this.stateCache[key] = this.originalContext.globalState.get(key) + } + } finally { + release() + } + throw error + }) } private getAllGlobalState(): GlobalState { @@ -384,12 +444,54 @@ export class ContextProxy { storeSecret(key: SecretStateKey, value?: string) { // Update cache. + const owner = {} + this.secretWriteOwner.set(key, owner) + this.secretWriteCount.set(key, (this.secretWriteCount.get(key) ?? 0) + 1) this.secretCache[key] = value - // Write directly to context. - return value === undefined - ? this.originalContext.secrets.delete(key) - : this.originalContext.secrets.store(key, value) + // Write directly to context. If the write rejects, the cache is read back from the secret storage while this + // write is still the latest of the key; if that fails too, the value that was never stored is dropped from the + // cache (it stays empty until the next write, `refreshSecrets` or a restart) rather than being served. + // The entry of the key is removed when the write and its read-back are over (if it is still the latest). A store + // that throws at once is turned into a rejection, so it is handled the same. + const release = () => { + if (this.secretWriteOwner.get(key) === owner) this.secretWriteOwner.delete(key) + } + const stored = (async () => + value === undefined + ? this.originalContext.secrets.delete(key) + : this.originalContext.secrets.store(key, value))() + return stored.then(release, async (error: unknown) => { + try { + if (this.secretWriteOwner.get(key) === owner) { + try { + const readBack = await withinReadBackDeadline(this.originalContext.secrets.get(key), key) + if (this.secretWriteOwner.get(key) === owner) this.secretCache[key] = readBack + } catch (readError) { + logger.error( + `Error reading back secret ${key}: ${readError instanceof Error ? readError.message : String(readError)}`, + ) + if (this.secretWriteOwner.get(key) === owner) this.secretCache[key] = undefined + } + } + } finally { + release() + } + throw error + }) + } + + /** + * Loads one secret into the cache. A write of the key that started while the secret was being read has its own + * outcome, even if it is already over, so the (older) value that was read is dropped. Otherwise the load takes over the key from a write that is + * still pending, so that a failure of that write cannot replace what was just loaded. + */ + private async refreshSecret(key: SecretStateKey) { + const writes = this.secretWriteCount.get(key) + const value = await this.originalContext.secrets.get(key) + if (this.secretWriteCount.get(key) !== writes) return + this.secretWriteOwner.delete(key) + this.secretCache[key] = value } /** @@ -400,7 +502,7 @@ export class ContextProxy { const promises = [ ...SECRET_STATE_KEYS.map(async (key) => { try { - this.secretCache[key] = await this.originalContext.secrets.get(key) + await this.refreshSecret(key) } catch (error) { logger.error( `Error refreshing secret ${key}: ${error instanceof Error ? error.message : String(error)}`, @@ -409,7 +511,7 @@ export class ContextProxy { }), ...GLOBAL_SECRET_KEYS.map(async (key) => { try { - this.secretCache[key] = await this.originalContext.secrets.get(key) + await this.refreshSecret(key) } catch (error) { logger.error( `Error refreshing global secret ${key}: ${error instanceof Error ? error.message : String(error)}`, @@ -555,9 +657,36 @@ export class ContextProxy { return { ...globalState, ...secretState } } + /** + * Writes every value at once. This is not a transaction, for any setting: each key is stored on its own, and the + * stores that completed are not undone. If any store rejects, every store is still awaited (the call stays pending + * until the last one has settled) and then rejects with a `SettingsWriteError` that says which keys were stored and + * which were not. A failed store has already read its key back into the cache (see `updateGlobalState` and + * `storeSecret`; best effort for global state, whose Memento may keep the attempted value). Callers that need more than this, such as a + * rollback, must provide it themselves. + */ public async setValues(values: RooCodeSettings) { const entries = Object.entries(values) as [RooCodeSettingsKey, unknown][] - await Promise.all(entries.map(([key, value]) => this.setValue(key, value))) + const results = await Promise.allSettled(entries.map(([key, value]) => this.setValue(key, value))) + + const committed: string[] = [] + const failed: string[] = [] + let cause: unknown + let hasCause = false + for (const [index, [key]] of entries.entries()) { + const result = results[index] + if (result.status === "fulfilled") { + committed.push(key) + continue + } + // The first failure is the cause, even when it was rejected without a reason. + if (!hasCause) { + cause = result.reason + hasCause = true + } + failed.push(key) + } + if (failed.length > 0) throw new SettingsWriteError(committed, failed, cause) } /** diff --git a/src/core/config/__tests__/ContextProxy.spec.ts b/src/core/config/__tests__/ContextProxy.spec.ts index b4d3b1b8aa..5d78e16351 100644 --- a/src/core/config/__tests__/ContextProxy.spec.ts +++ b/src/core/config/__tests__/ContextProxy.spec.ts @@ -7,7 +7,8 @@ import { GLOBAL_STATE_KEYS, SECRET_STATE_KEYS, GLOBAL_SECRET_KEYS } from "@roo-c import { clearAllMocks } from "../../../test-utils/reset" import { makeExtensionContext, makeUri } from "../../../test-utils/vscode" -import { ContextProxy } from "../ContextProxy" +import { logger } from "../../../utils/logging" +import { ContextProxy, SettingsWriteError } from "../ContextProxy" import { providerIdentifiers, retiredProviderIdentifiers } from "@roo-code/types/provider-identifiers" vi.mock("vscode", () => ({ @@ -287,6 +288,435 @@ describe("ContextProxy", () => { expect(proxy.getGlobalState("mode")).toBe("test-mode") }) + it("is not a transaction: if one store rejects, the others stay, the cache is read back, and the keys are reported", async () => { + // Storage that really holds values: what was stored before, and what a failed store leaves behind. + const storage = new Map([ + ["timeFormat", "auto"], + ["dateFormat", "auto"], + ["showRelativeTimes", true], + ]) + mockGlobalState.get.mockImplementation((key: string) => storage.get(key)) + const failure = new Error("storage unavailable") + mockGlobalState.update.mockImplementation(async (key: string, value: unknown) => { + if (key === "dateFormat") throw failure // rejects without having stored anything + storage.set(key, value) + }) + await proxy.initialize() + + const error = await proxy + .setValues({ timeFormat: "24h", dateFormat: "ymd", showRelativeTimes: false }) + .catch((e: unknown) => e) + + expect(error).toBeInstanceOf(SettingsWriteError) + expect((error as Error).name).toBe("SettingsWriteError") + expect(error).toMatchObject({ + committed: ["timeFormat", "showRelativeTimes"], + failed: ["dateFormat"], + cause: failure, + }) + expect((error as Error).message).toBe( + "Could not store dateFormat (stored: timeFormat, showRelativeTimes): storage unavailable", + ) + // The stores that worked were not undone, and the failed key's cache shows what the Memento holds. + expect(storage.get("timeFormat")).toBe("24h") + expect(proxy.getGlobalState("timeFormat")).toBe("24h") + expect(proxy.getGlobalState("showRelativeTimes")).toBe(false) + expect(proxy.getGlobalState("dateFormat")).toBe("auto") + }) + + it("reads the cache back from storage for a key whose store failed after changing it", async () => { + const storage = new Map([["dateFormat", "auto"]]) + mockGlobalState.get.mockImplementation((key: string) => storage.get(key)) + mockGlobalState.update.mockImplementation(async (key: string, value: unknown) => { + storage.set(key, value) + throw new Error("failed after storing") + }) + await proxy.initialize() + + await expect(proxy.setValues({ dateFormat: "ymd" })).rejects.toBeInstanceOf(SettingsWriteError) + + // Storage did change, so the cache shows the stored value, not a value it assumed. + expect(proxy.getGlobalState("dateFormat")).toBe("ymd") + }) + + it("reads a secret whose store failed back from the secret storage", async () => { + mockSecrets.get.mockImplementation(async (key: string) => + key === "openAiApiKey" ? "stored-key" : undefined, + ) + mockSecrets.store.mockRejectedValue(new Error("keychain locked")) + await proxy.initialize() + + const error = await proxy.setValues({ openAiApiKey: "new-key" }).catch((e: unknown) => e) + + expect(error).toMatchObject({ committed: [], failed: ["openAiApiKey"] }) + // The cache shows what the keychain holds, not the key that was never saved. + expect(proxy.getSecret("openAiApiKey")).toBe("stored-key") + }) + + it("reports a failed secret delete with the delete error, and reads the secret back into the cache", async () => { + mockSecrets.get.mockResolvedValue("stored-key") // the delete did not take effect + await proxy.initialize() + const failure = new Error("keychain locked") + mockSecrets.delete.mockRejectedValue(failure) + + await expect(proxy.storeSecret("openAiApiKey", undefined)).rejects.toBe(failure) + + expect(mockSecrets.store).not.toHaveBeenCalled() + // The cache shows what the keychain still holds, not the deletion that never happened. + expect(proxy.getSecret("openAiApiKey")).toBe("stored-key") + expect(proxy["secretWriteOwner"].size).toBe(0) + }) + + it("turns a store that throws at once into a rejection, and corrects the cache", async () => { + mockSecrets.get.mockResolvedValue("stored-key") + await proxy.initialize() + const failure = new Error("keychain unavailable") + mockSecrets.store.mockImplementation(() => { + throw failure + }) + + await expect(proxy.storeSecret("openAiApiKey", "new-key")).rejects.toBe(failure) + + expect(proxy.getSecret("openAiApiKey")).toBe("stored-key") + expect(proxy["secretWriteOwner"].size).toBe(0) + }) + + it("turns a global state update that throws at once into a rejection, and corrects the cache", async () => { + mockGlobalState.get.mockImplementation((key: string) => (key === "dateFormat" ? "auto" : undefined)) + await proxy.initialize() + const failure = new Error("storage unavailable") + mockGlobalState.update.mockImplementation(() => { + throw failure + }) + + await expect(proxy.updateGlobalState("dateFormat", "ymd")).rejects.toBe(failure) + + expect(proxy.getGlobalState("dateFormat")).toBe("auto") + expect(proxy["stateWriteOwner"].size).toBe(0) + }) + + it.each([ + ["succeeds", false], + ["fails", true], + ])("keeps no write owner once a write of a setting or a secret %s", async (_name, fails) => { + await proxy.initialize() + if (fails) { + mockGlobalState.update.mockRejectedValue(new Error("no")) + mockSecrets.store.mockRejectedValue(new Error("no")) + } + + const writes = [proxy.updateGlobalState("dateFormat", "ymd"), proxy.storeSecret("openAiApiKey", "key")] + expect(proxy["stateWriteOwner"].size).toBe(1) // while the writes are in flight + expect(proxy["secretWriteOwner"].size).toBe(1) + await Promise.allSettled(writes) + + expect(proxy["stateWriteOwner"].size).toBe(0) + expect(proxy["secretWriteOwner"].size).toBe(0) + }) + + it("keeps the owner of a newer secret write when an older write of the key finishes first", async () => { + let finishOlder: () => void = () => {} + mockSecrets.store + .mockImplementationOnce(() => new Promise((resolve) => (finishOlder = resolve))) + .mockImplementationOnce(() => new Promise(() => {})) // the newer write is still in flight + await proxy.initialize() + + const older = proxy.storeSecret("openAiApiKey", "older") + void proxy.storeSecret("openAiApiKey", "newer") + finishOlder() + await older + + expect(proxy["secretWriteOwner"].size).toBe(1) + }) + + it("keeps the owner of a newer write when an older write of the key finishes first", async () => { + let finishOlder: () => void = () => {} + mockGlobalState.update + .mockImplementationOnce(() => new Promise((resolve) => (finishOlder = resolve))) + .mockImplementationOnce(() => new Promise(() => {})) // the newer write is still in flight + await proxy.initialize() + + const older = proxy.updateGlobalState("dateFormat", "ymd") + void proxy.updateGlobalState("dateFormat", "dmy") + finishOlder() + await older + + expect(proxy["stateWriteOwner"].size).toBe(1) + }) + + it("does not let a failed write whose read-back times out hide a secret that refreshSecrets loaded", async () => { + vi.useFakeTimers() + try { + mockSecrets.store.mockRejectedValue(new Error("keychain locked")) + await proxy.initialize() + let readBackSettles: (value: string) => void = () => {} + mockSecrets.get + .mockImplementationOnce(() => new Promise((resolve) => (readBackSettles = resolve))) // the read-back, queued first + .mockResolvedValue("loaded-key") // what the refresh reads + + const failed = expect(proxy.storeSecret("openAiApiKey", "new-key")).rejects.toThrow("keychain locked") + await vi.advanceTimersByTimeAsync(0) // the store has failed and the read-back has started + await proxy.refreshSecrets() + expect(proxy.getSecret("openAiApiKey")).toBe("loaded-key") + await vi.advanceTimersByTimeAsync(5_000) // the read-back times out + await failed + + expect(proxy.getSecret("openAiApiKey")).toBe("loaded-key") + expect(proxy["secretWriteOwner"].size).toBe(0) + readBackSettles("late") + } finally { + vi.useRealTimers() + } + }) + + it("does not replace a secret written while refreshSecrets was reading it", async () => { + await proxy.initialize() + mockSecrets.get.mockClear() // initialize() read the secrets too: wait for the read of the refresh + let finishRead: (value: string) => void = () => {} + // Only the read of this key is held back; the other keys resolve as usual. + mockSecrets.get.mockImplementation((key: string) => + key === "openAiApiKey" ? new Promise((resolve) => (finishRead = resolve)) : Promise.resolve(undefined), + ) + + const refreshing = proxy.refreshSecrets() + await vi.waitFor(() => expect(mockSecrets.get).toHaveBeenCalledWith("openAiApiKey")) + await proxy.storeSecret("openAiApiKey", "written-key") // starts and succeeds during the read + finishRead("older-key") + await refreshing + + expect(proxy.getSecret("openAiApiKey")).toBe("written-key") + }) + + it("still reports the failure when the secret cannot be read back either", async () => { + mockSecrets.store.mockRejectedValue(new Error("keychain locked")) + await proxy.initialize() + mockSecrets.get.mockRejectedValue(new Error("keychain locked")) + + await expect(proxy.setValues({ openAiApiKey: "new-key" })).rejects.toBeInstanceOf(SettingsWriteError) + + // The key that was never saved is not served: the cache is empty until the next write or a refresh. + expect(proxy.getSecret("openAiApiKey")).toBeUndefined() + }) + + it("words the report for a failure that is not an Error, and when nothing was stored", async () => { + mockGlobalState.update.mockRejectedValue("disk full") + await proxy.initialize() + + const error = await proxy.setValues({ dateFormat: "ymd" }).catch((e: unknown) => e) + + expect(error).toMatchObject({ committed: [], failed: ["dateFormat"], cause: "disk full" }) + expect((error as Error).message).toBe("Could not store dateFormat: disk full") + }) + + it("reports a failed pass-through key without touching the cache, which does not hold it", async () => { + mockGlobalState.update.mockRejectedValue(new Error("storage unavailable")) + await proxy.initialize() + mockGlobalState.get.mockClear() + + const error = await proxy.setValues({ taskHistory: [] }).catch((e: unknown) => e) + + expect(error).toMatchObject({ committed: [], failed: ["taskHistory"] }) + expect(mockGlobalState.get).not.toHaveBeenCalled() // no read-back: this key is never cached + }) + + it("logs a secret that cannot be read back as text when the failure is not an Error", async () => { + mockSecrets.store.mockRejectedValue(new Error("keychain locked")) + await proxy.initialize() + mockSecrets.get.mockRejectedValue("locked") + const logged = vi.spyOn(logger, "error").mockImplementation(() => {}) + + await expect(proxy.setValues({ openAiApiKey: "new-key" })).rejects.toBeInstanceOf(SettingsWriteError) + + expect(logged).toHaveBeenCalledWith("Error reading back secret openAiApiKey: locked") + logged.mockRestore() + }) + + it("still rejects with the write error, and drops the unsaved secret, when the read-back never settles", async () => { + vi.useFakeTimers() + try { + const failure = new Error("keychain locked") + mockSecrets.store.mockRejectedValue(failure) + await proxy.initialize() + mockSecrets.get.mockImplementation(() => new Promise(() => {})) // never settles + const logged = vi.spyOn(logger, "error").mockImplementation(() => {}) + + const storing = expect(proxy.storeSecret("openAiApiKey", "new-key")).rejects.toBe(failure) + await vi.advanceTimersByTimeAsync(4_999) + expect(proxy.getSecret("openAiApiKey")).toBe("new-key") // still waiting for the read-back + await vi.advanceTimersByTimeAsync(1) + await storing + + expect(proxy.getSecret("openAiApiKey")).toBeUndefined() + expect(logged).toHaveBeenCalledWith( + "Error reading back secret openAiApiKey: Reading back openAiApiKey did not finish in 5 seconds", + ) + expect(vi.getTimerCount()).toBe(0) + logged.mockRestore() + } finally { + vi.useRealTimers() + } + }) + + it("releases the read-back deadline when the read settles in time", async () => { + vi.useFakeTimers() + try { + mockSecrets.store.mockRejectedValue(new Error("keychain locked")) + await proxy.initialize() + mockSecrets.get.mockResolvedValue("stored-key") + + await expect(proxy.storeSecret("openAiApiKey", "new-key")).rejects.toThrow("keychain locked") + + expect(proxy.getSecret("openAiApiKey")).toBe("stored-key") + expect(vi.getTimerCount()).toBe(0) + } finally { + vi.useRealTimers() + } + }) + + it("does not read a secret back for a failed store that a newer write of the key has already superseded", async () => { + let failOlder: () => void = () => {} + mockSecrets.store + .mockImplementationOnce( + () => new Promise((_resolve, reject) => (failOlder = () => reject(new Error("no")))), + ) + .mockResolvedValue(undefined) + await proxy.initialize() + mockSecrets.get.mockClear() + + const older = proxy.storeSecret("openAiApiKey", "older").catch((e: unknown) => e) + await proxy.storeSecret("openAiApiKey", "newer") // the newer write has its own outcome + failOlder() + await older + + expect(mockSecrets.get).not.toHaveBeenCalled() + expect(proxy.getSecret("openAiApiKey")).toBe("newer") + }) + + it("keeps a newer value written while a failed secret is being read back", async () => { + mockSecrets.store.mockRejectedValueOnce(new Error("keychain locked")).mockResolvedValue(undefined) + await proxy.initialize() + let readBack: (value: string | undefined) => void = () => {} + mockSecrets.get.mockImplementation(() => new Promise((resolve) => (readBack = resolve))) + mockSecrets.get.mockClear() // initialize() read the secrets too: wait for the read-back of this write + + const failing = proxy.setValues({ openAiApiKey: "old-attempt" }).catch((e: unknown) => e) + await vi.waitFor(() => expect(mockSecrets.get).toHaveBeenCalled()) + await proxy.setValues({ openAiApiKey: "newer-key" }) // starts and succeeds during the read-back + readBack("stale-from-storage") + await failing + + expect(proxy.getSecret("openAiApiKey")).toBe("newer-key") + }) + + it("does not clear a newer value of the same key when an older write's read-back fails", async () => { + mockSecrets.store.mockRejectedValueOnce(new Error("keychain locked")).mockResolvedValue(undefined) + await proxy.initialize() + let failReadBack: () => void = () => {} + mockSecrets.get.mockImplementation( + () => new Promise((_resolve, reject) => (failReadBack = () => reject(new Error("keychain locked")))), + ) + mockSecrets.get.mockClear() // initialize() read the secrets too: wait for the read-back of this write + const logged = vi.spyOn(logger, "error").mockImplementation(() => {}) + + const failing = proxy.setValues({ openAiApiKey: "same-value" }).catch((e: unknown) => e) + await vi.waitFor(() => expect(mockSecrets.get).toHaveBeenCalled()) + await proxy.setValues({ openAiApiKey: "same-value" }) // a newer write of the same value succeeds + failReadBack() + await failing + + expect(proxy.getSecret("openAiApiKey")).toBe("same-value") + logged.mockRestore() + }) + + // A Memento can keep the attempted value in memory after a rejected update, or drop it: the cache shows + // whichever it holds. + it.each([ + ["keeps the attempted value in memory", true, "ymd"], + ["does not keep the attempted value", false, "auto"], + ])( + "reads a global setting back from a Memento that %s, when a single setValue fails", + async (_name, keeps, expected) => { + const memento = new Map([["dateFormat", "auto"]]) + mockGlobalState.get.mockImplementation((key: string) => memento.get(key)) + mockGlobalState.update.mockImplementation(async (key: string, value: unknown) => { + if (keeps) memento.set(key, value) + throw new Error("storage unavailable") + }) + await proxy.initialize() + + await expect(proxy.setValue("dateFormat", "ymd")).rejects.toThrow("storage unavailable") + + expect(proxy.getGlobalState("dateFormat")).toBe(expected) + expect(proxy.getGlobalState("dateFormat")).toBe(memento.get("dateFormat")) + }, + ) + + it("does not let an older failed write replace the value of a newer write of the same key that is still being stored", async () => { + const storage = new Map([["dateFormat", "auto"]]) + mockGlobalState.get.mockImplementation((key: string) => storage.get(key)) + let failOlder: () => void = () => {} + let landNewer: () => void = () => {} + mockGlobalState.update + .mockImplementationOnce( + () => new Promise((_resolve, reject) => (failOlder = () => reject(new Error("no")))), + ) + .mockImplementationOnce( + (key: string, value: unknown) => + new Promise((resolve) => (landNewer = () => (storage.set(key, value), resolve()))), + ) + await proxy.initialize() + + const older = proxy.setValue("dateFormat", "dmy").catch((e: unknown) => e) + const newer = proxy.setValue("dateFormat", "ymd") // still being stored + failOlder() + await older + + // Storage still holds "auto", but the newer write has its own outcome: its value stays in the cache. + expect(proxy.getGlobalState("dateFormat")).toBe("ymd") + landNewer() + await newer + expect(proxy.getGlobalState("dateFormat")).toBe("ymd") + }) + + it("stays pending until every store has settled, then reports the committed and failed keys", async () => { + let settleSlow: () => void = () => {} + mockGlobalState.update.mockImplementation((key: string) => { + if (key === "dateFormat") return Promise.reject(new Error("storage unavailable")) // fails at once + return new Promise((resolve) => (settleSlow = resolve)) // the others are slow + }) + await proxy.initialize() + let outcome: unknown = "pending" + const writing = proxy + .setValues({ dateFormat: "ymd", timeFormat: "24h" }) + .catch((e: unknown) => (outcome = e)) + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(outcome).toBe("pending") // the rejection has happened, the slow store has not settled + + settleSlow() + await writing + + expect(outcome).toBeInstanceOf(SettingsWriteError) + expect(outcome).toMatchObject({ committed: ["timeFormat"], failed: ["dateFormat"] }) + }) + + it("keeps the first failure as the cause even when it was rejected without a reason", async () => { + mockGlobalState.update.mockImplementation((key: string) => + key === "dateFormat" ? Promise.reject(undefined) : Promise.reject(new Error("later failure")), + ) + await proxy.initialize() + + const error = await proxy.setValues({ dateFormat: "ymd", timeFormat: "24h" }).catch((e: unknown) => e) + + expect(error).toMatchObject({ failed: ["dateFormat", "timeFormat"], cause: undefined }) + expect((error as Error).message).toContain("undefined") + expect((error as Error).message).not.toContain("later failure") + }) + + it("does not report a failure when every store succeeds", async () => { + await expect(proxy.setValues({ timeFormat: "24h", dateFormat: "ymd" })).resolves.toBeUndefined() + }) + it("should handle both secret and global state keys", async () => { // Spy on storeSecret and updateGlobalState const storeSecretSpy = vi.spyOn(proxy, "storeSecret") diff --git a/src/core/config/__tests__/displaySettings.spec.ts b/src/core/config/__tests__/displaySettings.spec.ts new file mode 100644 index 0000000000..1acdcfc6fd --- /dev/null +++ b/src/core/config/__tests__/displaySettings.spec.ts @@ -0,0 +1,62 @@ +import type { RooCodeSettings } from "@roo-code/types" + +import { isInvalidDisplayValue, withoutInvalidDisplaySettings } from "../displaySettings" + +describe("isInvalidDisplayValue", () => { + it.each([ + ["timeFormat", "auto"], + ["timeFormat", "12h"], + ["timeFormat", "24h"], + ["timeFormat", undefined], + ["dateFormat", "mdy"], + ["dateFormat", "ymd"], + ["showRelativeTimes", false], + ["language", "de"], + ["language", undefined], + ["language", null], + ["soundEnabled", "not a boolean"], + ["constructor", "x"], + ["toString", "y"], + ])("accepts %s = %j", (key, value) => { + expect(isInvalidDisplayValue(key, value)).toBe(false) + }) + + it.each([ + ["timeFormat", "13h"], + ["timeFormat", 24], + ["dateFormat", "dd/mm"], + ["showRelativeTimes", "yes"], + ["language", "xx-not-a-language"], + ["language", 42], + ])("rejects %s = %j", (key, value) => { + expect(isInvalidDisplayValue(key, value)).toBe(true) + }) +}) + +describe("withoutInvalidDisplaySettings", () => { + it("keeps every valid value as it is, and does not call back", () => { + const onIgnored = vi.fn() + const values: RooCodeSettings = { timeFormat: "12h", dateFormat: "dmy", showRelativeTimes: true, mode: "code" } + + expect(withoutInvalidDisplaySettings(values, onIgnored)).toEqual(values) + expect(onIgnored).not.toHaveBeenCalled() + }) + + it("drops each invalid display value and says which, keeping the others", () => { + const onIgnored = vi.fn() + // Hand-written values that the typed signature does not allow. + const values = { timeFormat: "13h", dateFormat: "ymd", showRelativeTimes: "yes", mode: "code" } + + expect(withoutInvalidDisplaySettings(values as unknown as RooCodeSettings, onIgnored)).toEqual({ + dateFormat: "ymd", + mode: "code", + }) + expect(onIgnored.mock.calls).toEqual([["timeFormat"], ["showRelativeTimes"]]) + }) + + it("leaves the language to the caller: a language is not one of the three display settings", () => { + const values = { language: "xx" } as unknown as RooCodeSettings + + expect(withoutInvalidDisplaySettings(values, vi.fn())).toEqual({ language: "xx" }) + }) +}) diff --git a/src/core/config/displaySettings.ts b/src/core/config/displaySettings.ts new file mode 100644 index 0000000000..20f23c50a1 --- /dev/null +++ b/src/core/config/displaySettings.ts @@ -0,0 +1,35 @@ +import { globalSettingsSchema, type RooCodeSettings } from "@roo-code/types" + +type ValueSchema = { safeParse(value: unknown): { success: boolean } } + +const DISPLAY_SETTING_SCHEMAS = new Map([ + ["timeFormat", globalSettingsSchema.shape.timeFormat], + ["dateFormat", globalSettingsSchema.shape.dateFormat], + ["showRelativeTimes", globalSettingsSchema.shape.showRelativeTimes], +]) + +/** + * Whether a value for the display settings or the display language is one that their schemas do not allow. These + * values reach the formatters, and a message that was written by hand is not bound by the webview's own controls. + * Any other key is never invalid here. + */ +export const isInvalidDisplayValue = (key: string, value: unknown): boolean => + key === "language" + ? !globalSettingsSchema.shape.language.nullish().safeParse(value).success + : DISPLAY_SETTING_SCHEMAS.get(key)?.safeParse(value).success === false + +/** + * The values of a programmatic write (the public API), without an invalid value for one of the three display + * settings. The others are kept as they are. + */ +export const withoutInvalidDisplaySettings = ( + values: RooCodeSettings, + onIgnored: (key: string) => void, +): RooCodeSettings => + Object.fromEntries( + Object.entries(values).filter(([key, value]) => { + const invalid = DISPLAY_SETTING_SCHEMAS.get(key)?.safeParse(value).success === false + if (invalid) onIgnored(key) + return !invalid + }), + ) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index dc4ed27621..87c37cf596 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -132,6 +132,7 @@ import { buildApiHandler } from "../../api" import { forceFullModelDetailsLoad, hasLoadedFullDetails } from "../../api/providers/fetchers/lmstudio" import { ContextProxy } from "../config/ContextProxy" +import { withoutInvalidDisplaySettings } from "../config/displaySettings" import { ProviderSettingsManager } from "../config/ProviderSettingsManager" import { CustomModesManager } from "../config/CustomModesManager" import { PendingActionSettlementError, Task } from "../task/Task" @@ -3169,6 +3170,9 @@ export class ClineProvider historyPreviewCollapsed, reasoningBlockCollapsed, chatFontSize, + timeFormat, + dateFormat, + showRelativeTimes, enterBehavior, cloudUserInfo, cloudIsAuthenticated, @@ -3350,6 +3354,9 @@ export class ClineProvider historyPreviewCollapsed: historyPreviewCollapsed ?? false, reasoningBlockCollapsed: reasoningBlockCollapsed ?? true, chatFontSize, + timeFormat: timeFormat ?? "auto", + dateFormat: dateFormat ?? "auto", + showRelativeTimes: showRelativeTimes ?? true, enterBehavior: enterBehavior ?? "send", cloudUserInfo, cloudIsAuthenticated: cloudIsAuthenticated ?? false, @@ -3583,6 +3590,9 @@ export class ClineProvider historyPreviewCollapsed: stateValues.historyPreviewCollapsed ?? false, reasoningBlockCollapsed: stateValues.reasoningBlockCollapsed ?? true, chatFontSize: stateValues.chatFontSize, + timeFormat: stateValues.timeFormat ?? "auto", + dateFormat: stateValues.dateFormat ?? "auto", + showRelativeTimes: stateValues.showRelativeTimes ?? true, enterBehavior: stateValues.enterBehavior ?? "send", cloudUserInfo, cloudIsAuthenticated, @@ -3709,7 +3719,9 @@ export class ClineProvider } public async setValues(values: RooCodeSettings) { - await this.contextProxy.setValues(values) + await this.contextProxy.setValues( + withoutInvalidDisplaySettings(values, (key) => this.log(`[setValues] Ignored an invalid value for ${key}`)), + ) } // dev diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 07bacb91f6..9826fc206b 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -15,6 +15,7 @@ import { type ClineMessage, type ExtensionMessage, type ExtensionState, + type RooCodeSettings, type WebviewMessage, ORGANIZATION_ALLOW_ALL, DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, @@ -4039,6 +4040,39 @@ describe("ClineProvider", () => { }, ) + describe("display settings written through setValues and a task's configuration", () => { + const hand = (values: Record) => values as unknown as RooCodeSettings + + it("stores valid values as they are", async () => { + const setValues = vi.spyOn(provider.contextProxy, "setValues").mockResolvedValue(undefined) + + await provider.setValues({ timeFormat: "24h", dateFormat: "ymd", showRelativeTimes: false }) + + expect(setValues).toHaveBeenCalledWith({ timeFormat: "24h", dateFormat: "ymd", showRelativeTimes: false }) + }) + + it("ignores an invalid value, keeps the others, and logs which", async () => { + const setValues = vi.spyOn(provider.contextProxy, "setValues").mockResolvedValue(undefined) + const log = vi.spyOn(provider, "log").mockImplementation(() => {}) + + await provider.setValues(hand({ timeFormat: "13h", dateFormat: "dmy", mode: "code" })) + + expect(setValues).toHaveBeenCalledWith({ dateFormat: "dmy", mode: "code" }) + expect(log).toHaveBeenCalledWith("[setValues] Ignored an invalid value for timeFormat") + }) + + it("applies the same check to the configuration of a new task", async () => { + const setValues = vi.spyOn(provider.contextProxy, "setValues").mockResolvedValue(undefined) + vi.spyOn(provider, "log").mockImplementation(() => {}) + + await provider + .createTask("hello", undefined, undefined, {}, hand({ showRelativeTimes: "yes", mode: "code" })) + .catch(() => undefined) // what happens after the configuration is written is not under test + + expect(setValues).toHaveBeenCalledWith({ mode: "code" }) + }) + }) + test("getState returns the saved destructive command guard setting", async () => { await provider.contextProxy.setValue("destructiveCommandGuardEnabled", true) @@ -4151,6 +4185,81 @@ describe("ClineProvider", () => { expect(state.allowedReadFiles).toEqual([]) }) + describe("date and time display settings", () => { + const defaults = { timeFormat: "auto", dateFormat: "auto", showRelativeTimes: true } + + /** Saves through the real `updateSettings` handler, as the settings view does. */ + const save = async (updatedSettings: Record) => { + await provider.resolveWebviewView(mockWebviewView) + const messageHandler = vi.mocked(mockWebviewView.webview.onDidReceiveMessage).mock.calls[0][0] + await messageHandler({ type: "updateSettings", updatedSettings }) + } + + test.each(Object.entries(defaults))("%s is %j when it was never saved", async (key, expected) => { + await provider.resolveWebviewView(mockWebviewView) + + expect(await provider.getState()).toMatchObject({ [key]: expected }) + expect(await provider.getStateToPostToWebview()).toMatchObject({ [key]: expected }) + }) + + test.each([ + ["timeFormat", "12h"], + ["timeFormat", "24h"], + ["timeFormat", "auto"], + ["dateFormat", "mdy"], + ["dateFormat", "dmy"], + ["dateFormat", "ymd"], + ["dateFormat", "auto"], + ["showRelativeTimes", true], + ["showRelativeTimes", false], + ])("a saved %s of %j is stored and returned to the webview", async (key, value) => { + mockPostMessage.mockClear() + await save({ [key]: value }) + + expect(mockContext.globalState.update).toHaveBeenCalledWith(key, value) + expect(await provider.getState()).toMatchObject({ [key]: value }) + expect(await provider.getStateToPostToWebview()).toMatchObject({ [key]: value }) + // The open webview is told, so the saved value does not revert on screen. + expect(mockPostMessage).toHaveBeenCalledWith( + expect.objectContaining({ type: "state", state: expect.objectContaining({ [key]: value }) }), + ) + }) + + test("the state refresh after a save reaches the active webview with the saved time format", async () => { + mockPostMessage.mockClear() + + await save({ timeFormat: "24h" }) + + const posted: unknown[][] = mockPostMessage.mock.calls + const states = posted + .map(([message]) => message as { type?: string; state?: { timeFormat?: string } }) + .filter((message) => message.type === "state") + expect(states.length).toBeGreaterThan(0) + expect(states.at(-1)?.state?.timeFormat).toBe("24h") + }) + + test("an explicit false is kept, not replaced by the default of true", async () => { + await save({ showRelativeTimes: false }) + + expect((await provider.getState()).showRelativeTimes).toBe(false) + expect((await provider.getStateToPostToWebview()).showRelativeTimes).toBe(false) + }) + + test("all three are saved together and returned together", async () => { + await save({ timeFormat: "24h", dateFormat: "dmy", showRelativeTimes: false }) + + const expected = { timeFormat: "24h", dateFormat: "dmy", showRelativeTimes: false } + expect(await provider.getState()).toMatchObject(expected) + expect(await provider.getStateToPostToWebview()).toMatchObject(expected) + }) + + test("saving one setting leaves the others at their defaults", async () => { + await save({ timeFormat: "12h" }) + + expect(await provider.getStateToPostToWebview()).toMatchObject({ ...defaults, timeFormat: "12h" }) + }) + }) + test("getState returns the saved allowed write files", async () => { await provider.contextProxy.setValue("allowedWriteFiles", ["notes.md"]) diff --git a/src/core/webview/__tests__/webviewMessageHandler.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.spec.ts index 685a7b11c1..bf50f08786 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.spec.ts @@ -772,7 +772,7 @@ describe("webviewMessageHandler - theme fixture probes", () => { }) }) -import { t } from "../../../i18n" +import { changeLanguage, t } from "../../../i18n" vi.mock("vscode", () => { const showInformationMessage = vi.fn() @@ -801,6 +801,7 @@ vi.mock("vscode", () => { }) vi.mock("../../../i18n", () => ({ + changeLanguage: vi.fn(), t: vi.fn((key: string, args?: Record) => { // For the delete confirmation with rules, we need to return the interpolated string if (key === "common:confirmation.delete_custom_mode_with_rules" && args) { @@ -3053,3 +3054,190 @@ describe("webviewMessageHandler - renameTaskWithId", () => { expect(vi.mocked(mockClineProvider.renameTaskWithId).mock.calls).toEqual(expectedCalls) }) }) + +// The display settings are written like any other setting, but the webview is not trusted: a value that its own +// controls cannot produce (a hand-written message) is ignored before anything is done for it. +describe("updateSettings with the display settings", () => { + const save = (updatedSettings: Record) => + webviewMessageHandler(mockClineProvider, { type: "updateSettings", updatedSettings }) + + beforeEach(() => { + vi.clearAllMocks() + }) + + afterEach(() => { + vi.mocked(mockClineProvider.contextProxy.setValue).mockReset().mockResolvedValue(undefined) + }) + + it.each([ + ["timeFormat", "auto"], + ["timeFormat", "12h"], + ["timeFormat", "24h"], + ["dateFormat", "auto"], + ["dateFormat", "mdy"], + ["dateFormat", "dmy"], + ["dateFormat", "ymd"], + ["showRelativeTimes", true], + ["showRelativeTimes", false], + ["timeFormat", undefined], + ["showRelativeTimes", undefined], + ])("persists %s = %j as it is", async (key, value) => { + await save({ [key]: value }) + + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith(key, value) + expect(mockClineProvider.log).not.toHaveBeenCalled() + expect(mockClineProvider.postStateToWebview).toHaveBeenCalledTimes(1) + }) + + it.each([["de"], ["ja"], ["zh-TW"]])("applies and stores the display language %s", async (language) => { + await save({ language }) + + expect(changeLanguage).toHaveBeenCalledWith(language) + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("language", language) + }) + + it.each([[undefined], [null]])("uses the default language for %s", async (language) => { + await save({ language }) + + expect(changeLanguage).toHaveBeenCalledWith("en") + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("language", "en") + }) + + it.each([ + ["timeFormat", "13h"], + ["timeFormat", 24], + ["dateFormat", "dd/mm"], + ["showRelativeTimes", "yes"], + ["language", "xx-not-a-language"], + ["language", 42], + ["language", ""], + ])("ignores %s = %j, and says so in the log", async (key, value) => { + await save({ [key]: value }) + + expect(mockClineProvider.contextProxy.setValue).not.toHaveBeenCalledWith(key, expect.anything()) + expect(changeLanguage).not.toHaveBeenCalled() + expect(mockClineProvider.log).toHaveBeenCalledWith(`[updateSettings] Ignored an invalid value for ${key}`) + }) + + it("still saves the other settings sent with an invalid display value, and refreshes the webview", async () => { + await save({ timeFormat: "13h", dateFormat: "ymd", soundEnabled: true }) + + expect(mockClineProvider.contextProxy.setValue).not.toHaveBeenCalledWith("timeFormat", expect.anything()) + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("dateFormat", "ymd") + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("soundEnabled", true) + expect(mockClineProvider.postStateToWebview).toHaveBeenCalledTimes(1) + }) + + it("is not a transaction: if a write fails, the earlier ones stay, the later ones are not written, and the error is thrown", async () => { + const failure = new Error("storage unavailable") + vi.mocked(mockClineProvider.contextProxy.setValue).mockImplementation(async (key) => { + if (key === "dateFormat") throw failure + }) + + await expect(save({ timeFormat: "24h", dateFormat: "ymd", showRelativeTimes: false })).rejects.toBe(failure) + + const written = vi.mocked(mockClineProvider.contextProxy.setValue).mock.calls.map(([key]) => key) + expect(written).toEqual(["timeFormat", "dateFormat"]) + // The webview is refreshed even so, to show what is really stored. + expect(mockClineProvider.postStateToWebview).toHaveBeenCalledTimes(1) + }) + + describe("the save result", () => { + const saveWithId = (updatedSettings: Record, requestId?: string) => + webviewMessageHandler(mockClineProvider, { type: "updateSettings", requestId, updatedSettings }) + const saveAs = (updatedSettings: Record) => saveWithId(updatedSettings, "save-1") + + const results = () => + vi + .mocked(mockClineProvider.postMessageToWebview) + .mock.calls.map(([message]) => message) + .filter((message) => message.type === "settingsSaveResult") + + it("reports a stored save, after the state was refreshed", async () => { + await saveAs({ timeFormat: "24h" }) + + expect(results()).toEqual([{ type: "settingsSaveResult", requestId: "save-1", success: true }]) + const [refreshOrder] = vi.mocked(mockClineProvider.postStateToWebview).mock.invocationCallOrder + const [resultOrder] = vi.mocked(mockClineProvider.postMessageToWebview).mock.invocationCallOrder + expect(refreshOrder).toBeLessThan(resultOrder) + }) + + it.each([ + ["an Error", new Error("storage unavailable"), "storage unavailable"], + ["something that is not an Error", "disk full", "disk full"], + ])("reports a failed save with the reason, for %s, and still throws it", async (_name, failure, reason) => { + vi.mocked(mockClineProvider.contextProxy.setValue).mockRejectedValue(failure) + + await expect(saveAs({ dateFormat: "ymd" })).rejects.toBe(failure) + + expect(results()).toEqual([ + { type: "settingsSaveResult", requestId: "save-1", success: false, error: reason }, + ]) + }) + + it("does nothing, and reports nothing, for a message that carries no settings", async () => { + await webviewMessageHandler(mockClineProvider, { type: "updateSettings", requestId: "save-1" }) + + expect(mockClineProvider.contextProxy.setValue).not.toHaveBeenCalled() + expect(mockClineProvider.postStateToWebview).not.toHaveBeenCalled() + expect(results()).toEqual([]) + }) + + it("sends no result for a save that has no request id", async () => { + await saveWithId({ timeFormat: "24h" }) + + expect(results()).toEqual([]) + }) + + it("still reports a stored save when refreshing the state fails", async () => { + vi.mocked(mockClineProvider.postStateToWebview).mockRejectedValueOnce(new Error("webview gone")) + + await saveAs({ timeFormat: "24h" }) + + expect(results()).toEqual([{ type: "settingsSaveResult", requestId: "save-1", success: true }]) + expect(mockClineProvider.log).toHaveBeenCalledWith( + "[updateSettings] Could not refresh the webview state: webview gone", + ) + }) + + it("reports a failed save when refreshing the state fails too", async () => { + vi.mocked(mockClineProvider.contextProxy.setValue).mockRejectedValue(new Error("no")) + vi.mocked(mockClineProvider.postStateToWebview).mockRejectedValueOnce("webview gone") + + await expect(saveAs({ timeFormat: "24h" })).rejects.toThrow("no") + + expect(results()).toEqual([ + { type: "settingsSaveResult", requestId: "save-1", success: false, error: "no" }, + ]) + expect(mockClineProvider.log).toHaveBeenCalledWith( + "[updateSettings] Could not refresh the webview state: webview gone", + ) + }) + }) + + it("completes a partial save when the same values are saved again", async () => { + let failOnce = true + vi.mocked(mockClineProvider.contextProxy.setValue).mockImplementation(async (key) => { + if (key === "dateFormat" && failOnce) { + failOnce = false + throw new Error("storage unavailable") + } + }) + const values = { timeFormat: "24h", dateFormat: "ymd", showRelativeTimes: false } + + await expect(save(values)).rejects.toThrow("storage unavailable") + vi.mocked(mockClineProvider.contextProxy.setValue).mockClear() + await save(values) + + const written = vi.mocked(mockClineProvider.contextProxy.setValue).mock.calls.map(([key]) => key) + expect(written).toEqual(["timeFormat", "dateFormat", "showRelativeTimes"]) + expect(mockClineProvider.postStateToWebview).toHaveBeenCalledTimes(2) // once for the failed save, once for this one + }) + + it("does not take a key that only exists on every object for a display setting", async () => { + await save({ constructor: "x", toString: "y" }) + + expect(mockClineProvider.log).not.toHaveBeenCalled() + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("constructor", "x") + }) +}) diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index 2a04aa413f..4adb29264b 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -37,6 +37,7 @@ import { TelemetryService } from "@roo-code/telemetry" import { type ApiMessage } from "../task-persistence/apiMessages" import { saveTaskMessages } from "../task-persistence" +import { isInvalidDisplayValue } from "../config/displaySettings" import { importRooTaskHistory } from "../task-persistence/importRooTaskHistory" import { ClineProvider } from "./ClineProvider" @@ -742,8 +743,18 @@ export const webviewMessageHandler = async ( } break - case "updateSettings": - if (message.updatedSettings) { + case "updateSettings": { + // The settings are written one at a time, in the order sent, and not as a transaction. If a write fails, the + // ones before it stay stored and the ones after it are not written. The display settings only change how + // times are shown, so a partial save is harmless, and saving again completes it. Either way the webview is + // refreshed and, if the request has an id, told whether the save was stored (`settingsSaveResult`): the + // settings page keeps the user's edits as unsaved after a failure. The error is then thrown, as it always was. + let failed = false + let failure: unknown + + try { + if (!message.updatedSettings) break + if (message.updatedSettings.destructiveCommandGuardEnabled === true) { try { const { ensureDcgInstalled } = await import("../../services/destructive-command-guard") @@ -763,6 +774,12 @@ export const webviewMessageHandler = async ( } for (const [key, value] of Object.entries(message.updatedSettings)) { + // An invalid display value is ignored, before anything is done for it. + if (isInvalidDisplayValue(key, value)) { + provider.log(`[updateSettings] Ignored an invalid value for ${key}`) + continue + } + let newValue = value if (key === "language") { @@ -874,11 +891,33 @@ export const webviewMessageHandler = async ( await provider.contextProxy.setValue(key as keyof RooCodeSettings, newValue) } + } catch (error) { + failed = true + failure = error + } + // A failed refresh must neither undo a stored save nor keep the webview from getting the result. + try { await provider.postStateToWebview() + } catch (error) { + provider.log( + `[updateSettings] Could not refresh the webview state: ${error instanceof Error ? error.message : String(error)}`, + ) } + if (message.requestId) { + await provider.postMessageToWebview({ + type: "settingsSaveResult", + requestId: message.requestId, + success: !failed, + ...(failed ? { error: failure instanceof Error ? failure.message : String(failure) } : {}), + }) + } + + if (failed) throw failure + break + } case "terminalOperation": if (message.terminalOperation) { diff --git a/src/extension/__tests__/api-configuration.spec.ts b/src/extension/__tests__/api-configuration.spec.ts index f4ed8af469..a10bfe60ac 100644 --- a/src/extension/__tests__/api-configuration.spec.ts +++ b/src/extension/__tests__/api-configuration.spec.ts @@ -1,6 +1,8 @@ import { describe, expect, it, vi } from "vitest" import type * as vscode from "vscode" +import type { RooCodeSettings } from "@roo-code/types" + import { API } from "../api" import type { ClineProvider } from "../../core/webview/ClineProvider" import { makeClineProviderFactory } from "../../test-utils/provider" @@ -18,6 +20,7 @@ describe("API - configuration", () => { const provider = { context: {}, on: vi.fn(), + log: vi.fn(), contextProxy: { setValues }, providerSettingsManager: { saveConfig, setModeConfig }, postStateToWebview, @@ -51,6 +54,7 @@ describe("API - configuration", () => { const provider = { context: {}, on: vi.fn(), + log: vi.fn(), contextProxy: { setValues }, providerSettingsManager: { saveConfig, setModeConfig }, postStateToWebview, @@ -63,4 +67,64 @@ describe("API - configuration", () => { expect(setModeConfig).not.toHaveBeenCalled() expect(postStateToWebview).toHaveBeenCalledOnce() }) + + describe("the display settings", () => { + const setup = () => { + const setValues = vi.fn().mockResolvedValue(undefined) + const log = vi.fn() + const provider = { + context: {}, + on: vi.fn(), + log, + contextProxy: { setValues }, + providerSettingsManager: { + saveConfig: vi.fn().mockResolvedValue("default-id"), + setModeConfig: vi.fn(), + }, + postStateToWebview: vi.fn().mockResolvedValue(undefined), + } as unknown as ClineProvider + const outputChannel = { appendLine: vi.fn() } as unknown as vscode.OutputChannel + return { api: new API(outputChannel, provider, makeClineProviderFactory()), setValues, log } + } + + it.each([ + ["timeFormat", "24h"], + ["dateFormat", "ymd"], + ["showRelativeTimes", false], + ])("stores a valid %s = %j", async (key, value) => { + const { api, setValues, log } = setup() + + await api.setConfiguration({ [key]: value }) + + expect(setValues).toHaveBeenCalledWith({ [key]: value }) + expect(log).not.toHaveBeenCalled() + }) + + it("rejects when a setting cannot be stored, and writes nothing after the settings", async () => { + const { api, setValues } = setup() + setValues.mockRejectedValue(new Error("storage unavailable")) + const provider = api["sidebarProvider"] + + await expect(api.setConfiguration({ timeFormat: "24h", currentApiConfigName: "default" })).rejects.toThrow( + "storage unavailable", + ) + + expect(provider.providerSettingsManager.saveConfig).not.toHaveBeenCalled() + expect(provider.postStateToWebview).not.toHaveBeenCalled() + }) + + it.each([ + ["timeFormat", "13h"], + ["dateFormat", "dd/mm"], + ["showRelativeTimes", "yes"], + ])("ignores an invalid %s = %j, and keeps the other values", async (key, value) => { + const { api, setValues, log } = setup() + + // A hand-written value that the typed signature does not allow. + await api.setConfiguration({ [key]: value, soundEnabled: true } as unknown as RooCodeSettings) + + expect(setValues).toHaveBeenCalledWith({ soundEnabled: true }) + expect(log).toHaveBeenCalledWith(`[setConfiguration] Ignored an invalid value for ${key}`) + }) + }) }) diff --git a/src/extension/api.ts b/src/extension/api.ts index da59c48b58..623b0492d7 100644 --- a/src/extension/api.ts +++ b/src/extension/api.ts @@ -32,6 +32,7 @@ import { Package } from "../shared/package" import type { Mode } from "../shared/modes" import type { ClineProvider } from "../core/webview/ClineProvider" import type { ClineProviderFactory } from "../core/webview/ClineProviderFactory" +import { withoutInvalidDisplaySettings } from "../core/config/displaySettings" import { readTaskMessages } from "../core/task-persistence/taskMessages" import { Terminal } from "../integrations/terminal/Terminal" import { TerminalRegistry } from "../integrations/terminal/TerminalRegistry" @@ -628,7 +629,17 @@ export class API extends EventEmitter implements RooCodeAPI { ) } - public async setConfiguration(values: RooCodeSettings) { + /** + * Stores the settings, then the API profile and the mode profiles. The settings are written with + * `ContextProxy.setValues`, which is not a transaction (see there): if a setting cannot be stored, this rejects + * with a `SettingsWriteError` that lists the keys that were stored and the keys that were not (the cache of the + * failed keys has been read back from storage), the stored settings stay, and nothing after the settings is + * written. Invalid values of the display settings are dropped and logged first. + */ + public async setConfiguration(allValues: RooCodeSettings) { + const values = withoutInvalidDisplaySettings(allValues, (key) => + this.sidebarProvider.log(`[setConfiguration] Ignored an invalid value for ${key}`), + ) await this.sidebarProvider.contextProxy.setValues(values) await this.sidebarProvider.providerSettingsManager.saveConfig(values.currentApiConfigName || "default", values) if (values.modeApiConfigs) { diff --git a/webview-ui/playwright/gallery/stories.tsx b/webview-ui/playwright/gallery/stories.tsx index b87ee61669..7d169431ce 100644 --- a/webview-ui/playwright/gallery/stories.tsx +++ b/webview-ui/playwright/gallery/stories.tsx @@ -415,6 +415,25 @@ export const stories: Record = { ) }, + "chat-row-timestamps": async () => { + const { ChatRowTimestampsStory } = await import("@/components/chat/__tests__/MessageTimestamps.visual.fixture") + return + }, + "task-header-timestamp": async () => { + const { TaskHeaderTimestampStory } = + await import("@/components/chat/__tests__/MessageTimestamps.visual.fixture") + return + }, + "task-item-footer-absolute": async () => { + const { TaskItemFooterAbsoluteStory } = + await import("@/components/chat/__tests__/MessageTimestamps.visual.fixture") + return + }, + "language-settings": async () => { + const { LanguageSettingsStory } = + await import("@/components/settings/__tests__/LanguageSettings.visual.fixture") + return + }, "anthropic-settings": async () => { const [{ AppProviders }, { Anthropic }, { providerIdentifiers }] = await Promise.all([ import("../AppProviders"), diff --git a/webview-ui/src/__tests__/ContextWindowProgress.spec.tsx b/webview-ui/src/__tests__/ContextWindowProgress.spec.tsx index f5c2ba1062..052d58267e 100644 --- a/webview-ui/src/__tests__/ContextWindowProgress.spec.tsx +++ b/webview-ui/src/__tests__/ContextWindowProgress.spec.tsx @@ -5,8 +5,9 @@ import { providerIdentifiers } from "@roo-code/types" import TaskHeader from "@src/components/chat/TaskHeader" -// Mock formatLargeNumber function -vi.mock("@/utils/format", () => ({ +// Keep the real format helpers (other components in the tree use them) and override only formatLargeNumber. +vi.mock("@/utils/format", async (importOriginal) => ({ + ...(await importOriginal()), formatLargeNumber: vi.fn((num) => num.toString()), })) diff --git a/webview-ui/src/components/chat/ChatRow.tsx b/webview-ui/src/components/chat/ChatRow.tsx index 3b6a3ca66b..2040501b3f 100644 --- a/webview-ui/src/components/chat/ChatRow.tsx +++ b/webview-ui/src/components/chat/ChatRow.tsx @@ -38,6 +38,7 @@ import WarningRow from "./WarningRow" import McpResourceRow from "../mcp/McpResourceRow" import { Mention } from "./Mention" +import { isShowableTimestamp, MessageTimestamp } from "./MessageTimestamp" import { CheckpointSaved } from "./checkpoints/CheckpointSaved" import { FollowUpSuggest } from "./FollowUpSuggest" import { BatchFilePermission } from "./BatchFilePermission" @@ -231,6 +232,8 @@ export const ChatRowContent = ({ clineMessages, currentTaskItem, enableCheckpoints, + timeFormat, + dateFormat, } = useExtensionState() const { info: model } = useSelectedModel(apiConfiguration) const [isEditing, setIsEditing] = useState(false) @@ -1248,6 +1251,13 @@ export const ChatRowContent = ({ )} + {/* When the reply was sent. Held back while it streams, so the line does not appear and + shift as the text grows. */} + {!message.partial && isShowableTimestamp(message.ts) && ( +
+ +
+ )} ) case "user_feedback": @@ -1323,6 +1333,12 @@ export const ChatRowContent = ({ )} + {/* When the message was sent. Hidden while editing: the edit box replaces the bubble. */} + {!isEditing && isShowableTimestamp(message.ts) && ( +
+ +
+ )} ) case "user_feedback_diff": @@ -1384,6 +1400,12 @@ export const ChatRowContent = ({ ) : null} + {/* When the final result was sent. Held back while it streams, like other replies. */} + {!message.partial && isShowableTimestamp(message.ts) && ( +
+ +
+ )} ) case "shell_integration_warning": diff --git a/webview-ui/src/components/chat/MessageTimestamp.tsx b/webview-ui/src/components/chat/MessageTimestamp.tsx new file mode 100644 index 0000000000..c76ef2e14b --- /dev/null +++ b/webview-ui/src/components/chat/MessageTimestamp.tsx @@ -0,0 +1,51 @@ +import { Clock } from "lucide-react" + +import { cn } from "@src/lib/utils" +import { formatDateTime, formatMessageTime, type DateFormat, type TimeFormat } from "@/utils/format" + +import { StandardTooltip } from "../ui/standard-tooltip" + +interface MessageTimestampProps { + /** Epoch ms the message was sent. */ + ts: number + /** From the user's Language settings; both default to "auto". */ + timeFormat?: TimeFormat + dateFormat?: DateFormat + className?: string +} + +/** + * Whether `ts` is a time that can be shown. Number.isFinite is not enough: a finite value beyond ±8.64e15 ms is an + * Invalid Date, and toISOString() would throw during render and take the whole chat row down with it. Callers that + * wrap the timestamp in spacing use this, so an invalid time leaves no empty block behind. + */ +export const isShowableTimestamp = (ts: number) => + Number.isFinite(ts) && ts > 0 && !Number.isNaN(new Date(ts).getTime()) + +/** + * "🕒 Yesterday 11:43 AM" under a sent message. + * + * Purely presentational: the caller supplies the user's time/date formats, so + * the component has no dependency on extension state and renders anywhere. The + * tooltip always carries the full date and time. A semantic