From 60fc0f4f9cf2e7f96436d1133f1be066d7cc99b5 Mon Sep 17 00:00:00 2001 From: Guilhem Lemouel Date: Wed, 23 Sep 2026 14:21:04 +0200 Subject: [PATCH] fix: read the cell at the resolved path, and clean up by it after a deploy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 6, all three at the seam the reshape moved: - A draft that exists only as a local cell — a second session tab, or an editor with autosave off — was invisible by its chosen name: resolution found it, then the callers looked for a cell at the literal path and fell through to the deployed item. The write merge then rebuilt the draft from that deployed item. Both reads now look at the resolved path. - Deploying an app by its chosen name creates the app at that name, which then wins the name back, so the cleanup deleted nothing and the draft stayed listed. The cleanup uses the path resolved before the deploy. - Discarding a draft addressed by its chosen name cleared the modified-items entry under the name while writes record it under the storage path, leaving the item marked as changed. The discard resolves once and uses that. - A listing failure no longer fails a read: only a write, which is the call that would create a second draft by trusting the path it was given. Co-Authored-By: Claude Opus 5 (1M context) --- .../copilot/chat/global/core.test.ts | 52 +++++++++++++++++++ .../components/copilot/chat/global/core.ts | 16 +++--- .../copilot/chat/global/userDraftAdapter.ts | 32 ++++++++---- 3 files changed, 84 insertions(+), 16 deletions(-) diff --git a/frontend/src/lib/components/copilot/chat/global/core.test.ts b/frontend/src/lib/components/copilot/chat/global/core.test.ts index bbc0997995..aafede147f 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.test.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.test.ts @@ -2538,6 +2538,58 @@ describe('global AI tools', () => { } }) + // A draft can exist only as a local cell — a second session tab, or an editor with + // autosave off — with no backend row to fall back to. + it('reads a cell-only draft by its chosen name', async () => { + UserDraft.save( + 'raw_app', + 'u/admin/draft_cell_only', + { summary: 'Cell only', draft_path: 'f/sales/cell_app', files: {}, runnables: {} }, + { workspace: WORKSPACE } + ) + + const read = JSON.parse( + await callGlobalTool('read_workspace_item', { type: 'app', path: 'f/sales/cell_app' }) + ) + expect(read).toMatchObject({ path: 'u/admin/draft_cell_only', summary: 'Cell only' }) + }) + + // Deploying by the chosen name creates the app at that name, which would then win the + // name back and leave the draft the deploy came from listed forever. + it('removes the source draft of an app deployed by its chosen name', async () => { + const storageKey = 'u/admin/draft_deployed_by_name' + seedBackendDraft( + 'raw_app', + storageKey, + { + summary: 'By name', + draft_path: 'f/sales/by_name', + files: { '/App.tsx': 'export default () => null' }, + runnables: {}, + data: { tables: [] } + }, + { workspace: WORKSPACE } + ) + const deployed = new Set() + vi.mocked(AppService.existsApp).mockImplementation(async ({ path }) => deployed.has(path)) + vi.mocked(AppService.createAppRaw).mockImplementation(async ({ formData }: any) => { + deployed.add(formData.app.path) + return 1 as any + }) + vi.mocked(AppService.getAppByPath).mockResolvedValueOnce({ + draft: { draft_path: 'f/sales/by_name' } + } as any) + + try { + await callGlobalTool('deploy_workspace_item', { type: 'app', path: 'f/sales/by_name' }) + expect(deployed.has('f/sales/by_name')).toBe(true) + expect(getBackendDraft('raw_app', storageKey, { workspace: WORKSPACE })).toBeUndefined() + } finally { + vi.mocked(AppService.existsApp).mockImplementation(async () => false) + vi.mocked(AppService.createAppRaw).mockImplementation(async () => 1 as any) + } + }) + it('refuses a draft_path that two drafts are staged under', async () => { for (const storage of ['u/admin/draft_a', 'u/admin/draft_b']) { seedBackendDraft( diff --git a/frontend/src/lib/components/copilot/chat/global/core.ts b/frontend/src/lib/components/copilot/chat/global/core.ts index aaf907ae6f..3265351e4f 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.ts @@ -6834,16 +6834,16 @@ async function discardLocalDraft( throw new Error(`No draft found for ${type} "${path}".`) } - await deleteGlobalDraft(workspace, type, path, triggerKind) + // Resolved once: the mask records a draft under the path it is stored at, so clearing + // it by the name the call used would leave the entry behind. + const storagePath = await resolveGlobalDraftStoragePath(workspace, type, path, triggerKind) + await deleteGlobalDraft(workspace, type, storagePath, triggerKind) // The chat's touch on the item is undone — drop it from the mask so a // pre-existing deployed item doesn't keep reading as this chat's edit. const discardedKind = itemKindFor(type, triggerKind) if (discardedKind) { - toolCallbacks.onItemDiscarded?.( - discardedKind, - liveGlobalDraftStoragePath(workspace, type, path, triggerKind) - ) + toolCallbacks.onItemDiscarded?.(discardedKind, storagePath) } toolCallbacks.setToolStatus(toolId, { @@ -8274,7 +8274,11 @@ async function deployDraft( // fork comparisons before the fallible draft cleanup below. invalidateWorkspaceComparison(workspace) - await deleteGlobalDraft(workspace, type, path, triggerKind, { preserveLiveDraft: true }) + // By the path resolved before the deploy: the app now deployed at the draft's chosen + // name would otherwise win the name back, and the draft would be left behind. + await deleteGlobalDraft(workspace, type, draftStoragePath, triggerKind, { + preserveLiveDraft: true + }) // Move the chat's mask entry to the deployed path: a draft-only item's // synthetic storage key never exists deployed, so the entry would otherwise diff --git a/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts b/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts index 886242f578..992c4965a2 100644 --- a/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts +++ b/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts @@ -336,7 +336,10 @@ type ResolvedDraft = { storagePath: string; value?: unknown } async function resolveDraft( workspace: string, itemKind: UserDraftItemKind, - path: string + path: string, + /** A write must know which draft a name belongs to; a read can fall back to the path + * itself, which is what it addressed before names were resolved at all. */ + opts: { forWrite?: boolean } = {} ): Promise { const live = liveEditorStoragePath(workspace, itemKind, path) if (live !== path) return { storagePath: live } @@ -345,13 +348,14 @@ async function resolveDraft( if (UserDraft.get(itemKind, path, { workspace }) !== undefined) return { storagePath: path } const own = await fetchBackendDraftValue(workspace, itemKind, path) if (own !== undefined) return { storagePath: path, value: own } - return { storagePath: await storagePathForChosenName(workspace, itemKind, path) } + return { storagePath: await storagePathForChosenName(workspace, itemKind, path, opts) } } async function storagePathForChosenName( workspace: string, itemKind: UserDraftItemKind, - path: string + path: string, + opts: { forWrite?: boolean } ): Promise { const staged: StagedDraft[] = [] const add = (storagePath: string, summary: string | undefined) => { @@ -365,7 +369,9 @@ async function storagePathForChosenName( rows = await DraftService.listDrafts({ workspace }) } catch (e) { // Nothing is stored at this path, so it may be a name. Without the listing that - // cannot be told, and guessing the path itself would write a second draft. + // cannot be told, and a write that guessed the path itself would create a second + // draft; a read of whatever is deployed there is harmless. + if (!opts.forWrite) return path throw new Error( `Could not load this workspace's drafts, so "${path}" cannot be matched to the draft ` + `it may name: ${e instanceof Error ? e.message : String(e)}. Try again.` @@ -494,11 +500,11 @@ export async function readGlobalDraftValue( ): Promise { const itemKind = itemKindFor(type, triggerKind) if (!itemKind) return undefined - const cell = UserDraft.get(itemKind, liveEditorStoragePath(workspace, itemKind, path), { - workspace - }) - if (cell !== undefined) return cell const resolved = await resolveDraft(workspace, itemKind, path) + // The cell at the RESOLVED path: it is the freshest state when a save is parked, failed + // or conflicted, and a draft that exists only as a cell has no backend row behind it. + const cell = UserDraft.get(itemKind, resolved.storagePath, { workspace }) + if (cell !== undefined) return cell if (resolved.value !== undefined) return resolved.value as V return (await fetchBackendDraftValue(workspace, itemKind, resolved.storagePath)) as V | undefined } @@ -538,7 +544,8 @@ export async function persistGlobalDraft( if (!itemKind) throw new Error(`Unsupported draft type "${type}".`) // Resolved rather than taken as given: a write under a chosen name that did not resolve // would create a second draft beside the one it meant to edit. - const storagePath = (await resolveDraft(workspace, itemKind, path)).storagePath + const storagePath = (await resolveDraft(workspace, itemKind, path, { forWrite: true })) + .storagePath UserDraft.seed(itemKind, storagePath, value, { workspace }) await UserDraftDbSyncer.save({ workspace, @@ -596,7 +603,12 @@ export async function getGlobalDraft( if (!itemKind) return undefined const resolved = await resolveDraft(workspace, itemKind, path) const storagePath = resolved.storagePath - const value = resolved.value ?? (await fetchBackendDraftValue(workspace, itemKind, storagePath)) + // A draft can live only as a local cell — a second session tab, or an editor with + // autosave off — with no backend row behind it. + const value = + UserDraft.get(itemKind, storagePath, { workspace }) ?? + resolved.value ?? + (await fetchBackendDraftValue(workspace, itemKind, storagePath)) if (value === undefined || value === null) return undefined const { displayPath, isLiveDraft } = liveDisplayPath(workspace, itemKind, storagePath) return userDraftEntryToWorkspaceItem(