From b4ad874600c64fca2903ff7860c6548f7a071ade Mon Sep 17 00:00:00 2001 From: Pier-Jean Malandrino Date: Thu, 7 May 2026 11:09:13 +0200 Subject: [PATCH] fix(#256): drop pushDocumentToStore duplicate, align rechunk return shape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The frontend had two endpoints pointing at the same operation: - features/document/api.ts:pushDocumentToStore → POST /push - features/chunks/api.ts:pushChunksToStore → POST /chunks/push Backend now exposes a single /chunks/push route (semantically more accurate — it's chunks that get pushed, not the doc itself), so the document-side variant is removed. document/store.ts:pushToStore now delegates to chunks/api. rechunkDocument returned {jobId} based on the old async-job model. The new backend rechunk runs synchronously and returns the chunk list directly. The DocsLibraryPage caller now reports a count (via new docs.rechunkDone i18n key) instead of a fake job id. Adds data-e2e=tab-{mode} on the doc workspace tab strip so the new Karate scenario can target the chunks tab without text matching. Extends chunks/api.test.ts with HTTP error-propagation guards on every call — the original 404 bug would have surfaced in CI. --- frontend/src/features/chunks/api.test.ts | 46 ++++++++++++++++++++ frontend/src/features/document/api.test.ts | 21 +++------ frontend/src/features/document/api.ts | 14 +++--- frontend/src/features/document/store.test.ts | 23 ++++++---- frontend/src/features/document/store.ts | 9 ++-- frontend/src/pages/DocWorkspacePage.vue | 1 + frontend/src/pages/DocsLibraryPage.vue | 10 ++--- frontend/src/shared/i18n.ts | 2 + 8 files changed, 85 insertions(+), 41 deletions(-) diff --git a/frontend/src/features/chunks/api.test.ts b/frontend/src/features/chunks/api.test.ts index 3b0c2ab..f5c1d34 100644 --- a/frontend/src/features/chunks/api.test.ts +++ b/frontend/src/features/chunks/api.test.ts @@ -128,4 +128,50 @@ describe('chunks API', () => { }) expect(result).toEqual(response) }) + + // ---- Error propagation: each call lets the apiFetch error bubble up so + // the store can map it to a user-visible message. The original 404 bug + // (#256) silently swallowed the error — these guard tests would have + // caught it. + describe('error propagation', () => { + it('fetchChunks rejects with apiFetch error (404)', async () => { + apiFetch.mockRejectedValue(new Error('404: Not Found')) + await expect(fetchChunks('d1')).rejects.toThrow('404') + }) + + it('updateChunk rejects with apiFetch error (404)', async () => { + apiFetch.mockRejectedValue(new Error('404: Not Found')) + await expect(updateChunk('d1', 'c1', { text: 'x' })).rejects.toThrow('404') + }) + + it('mergeChunks rejects with apiFetch error (409)', async () => { + apiFetch.mockRejectedValue(new Error('409: Conflict')) + await expect(mergeChunks('d1', ['a', 'b'])).rejects.toThrow('409') + }) + + it('splitChunk rejects with apiFetch error (400)', async () => { + apiFetch.mockRejectedValue(new Error('400: Bad Request')) + await expect(splitChunk('d1', 'c1', 0)).rejects.toThrow('400') + }) + + it('dropChunk rejects with apiFetch error (404)', async () => { + apiFetch.mockRejectedValue(new Error('404: Not Found')) + await expect(dropChunk('d1', 'c1')).rejects.toThrow('404') + }) + + it('addChunk rejects with apiFetch error (500)', async () => { + apiFetch.mockRejectedValue(new Error('500: Internal Server Error')) + await expect(addChunk('d1', 'x')).rejects.toThrow('500') + }) + + it('fetchChunkDiff rejects with apiFetch error (404)', async () => { + apiFetch.mockRejectedValue(new Error('404: Not Found')) + await expect(fetchChunkDiff('d1', 'store')).rejects.toThrow('404') + }) + + it('pushChunksToStore rejects with apiFetch error (503)', async () => { + apiFetch.mockRejectedValue(new Error('503: Service Unavailable')) + await expect(pushChunksToStore('d1', 'store')).rejects.toThrow('503') + }) + }) }) diff --git a/frontend/src/features/document/api.test.ts b/frontend/src/features/document/api.test.ts index 0836592..ebdd057 100644 --- a/frontend/src/features/document/api.test.ts +++ b/frontend/src/features/document/api.test.ts @@ -6,7 +6,6 @@ import { deleteDocument, getPreviewUrl, rechunkDocument, - pushDocumentToStore, fetchDocumentTree, } from './api' @@ -72,25 +71,17 @@ describe('document API', () => { expect(getPreviewUrl('abc', 3, 300)).toBe('/api/documents/abc/preview?page=3&dpi=300') }) - it('rechunkDocument calls POST /api/documents/:id/rechunk', async () => { - apiFetch.mockResolvedValue({ jobId: 'job-1' }) + it('rechunkDocument calls POST /api/documents/:id/rechunk and returns chunks', async () => { + const chunks = [{ id: 'c1' }, { id: 'c2' }] + apiFetch.mockResolvedValue(chunks) const result = await rechunkDocument('42') - expect(apiFetch).toHaveBeenCalledWith('/api/documents/42/rechunk', { method: 'POST' }) - expect(result).toEqual({ jobId: 'job-1' }) - }) - - it('pushDocumentToStore calls POST /api/documents/:id/push with store', async () => { - apiFetch.mockResolvedValue({ jobId: 'job-2' }) - - const result = await pushDocumentToStore('42', 'my-store') - - expect(apiFetch).toHaveBeenCalledWith('/api/documents/42/push', { + expect(apiFetch).toHaveBeenCalledWith('/api/documents/42/rechunk', { method: 'POST', - body: JSON.stringify({ store: 'my-store' }), + body: JSON.stringify({}), }) - expect(result).toEqual({ jobId: 'job-2' }) + expect(result).toEqual(chunks) }) it('fetchDocumentTree calls GET /api/documents/:id/tree', async () => { diff --git a/frontend/src/features/document/api.ts b/frontend/src/features/document/api.ts index 2736a9a..7c3b451 100644 --- a/frontend/src/features/document/api.ts +++ b/frontend/src/features/document/api.ts @@ -1,4 +1,4 @@ -import type { Document, DocTreeNode } from '../../shared/types' +import type { DocChunk, Document, DocTreeNode } from '../../shared/types' import { apiFetch } from '../../shared/api/http' export function fetchDocuments(): Promise { @@ -27,14 +27,12 @@ export function getPreviewUrl(id: string, page = 1, dpi = 150): string { return `/api/documents/${id}/preview?page=${page}&dpi=${dpi}` } -export function rechunkDocument(id: string): Promise<{ jobId: string }> { - return apiFetch<{ jobId: string }>(`/api/documents/${id}/rechunk`, { method: 'POST' }) -} - -export function pushDocumentToStore(id: string, store: string): Promise<{ jobId: string }> { - return apiFetch<{ jobId: string }>(`/api/documents/${id}/push`, { +/** Rechunk the canonical chunkset. Backend runs synchronously and returns + * the new chunks — there is no async job to poll. */ +export function rechunkDocument(id: string): Promise { + return apiFetch(`/api/documents/${id}/rechunk`, { method: 'POST', - body: JSON.stringify({ store }), + body: JSON.stringify({}), }) } diff --git a/frontend/src/features/document/store.test.ts b/frontend/src/features/document/store.test.ts index efdf13f..296b35f 100644 --- a/frontend/src/features/document/store.test.ts +++ b/frontend/src/features/document/store.test.ts @@ -7,10 +7,14 @@ vi.mock('./api', () => ({ uploadDocument: vi.fn(), deleteDocument: vi.fn(), rechunkDocument: vi.fn(), - pushDocumentToStore: vi.fn(), +})) + +vi.mock('../chunks/api', () => ({ + pushChunksToStore: vi.fn(), })) import * as api from './api' +import * as chunksApi from '../chunks/api' describe('useDocumentStore', () => { beforeEach(() => { @@ -138,12 +142,12 @@ describe('useDocumentStore', () => { expect(store.loading).toBe(false) }) - it('rechunk() returns jobId on success', async () => { - api.rechunkDocument.mockResolvedValue({ jobId: 'j1' }) + it('rechunk() returns chunk count on success', async () => { + api.rechunkDocument.mockResolvedValue([{ id: 'c1' }, { id: 'c2' }, { id: 'c3' }]) const store = useDocumentStore() const result = await store.rechunk('42') expect(api.rechunkDocument).toHaveBeenCalledWith('42') - expect(result).toBe('j1') + expect(result).toBe(3) }) it('rechunk() returns null on error', async () => { @@ -154,16 +158,19 @@ describe('useDocumentStore', () => { expect(result).toBeNull() }) - it('pushToStore() returns jobId on success', async () => { - api.pushDocumentToStore.mockResolvedValue({ jobId: 'j2' }) + it('pushToStore() delegates to chunks/api.pushChunksToStore and returns jobId', async () => { + chunksApi.pushChunksToStore.mockResolvedValue({ + jobId: 'j2', + summary: { embeds: 5, tokens: 50 }, + }) const store = useDocumentStore() const result = await store.pushToStore('42', 'my-store') - expect(api.pushDocumentToStore).toHaveBeenCalledWith('42', 'my-store') + expect(chunksApi.pushChunksToStore).toHaveBeenCalledWith('42', 'my-store') expect(result).toBe('j2') }) it('pushToStore() returns null on error', async () => { - api.pushDocumentToStore.mockRejectedValue(new Error('fail')) + chunksApi.pushChunksToStore.mockRejectedValue(new Error('fail')) vi.spyOn(console, 'error').mockImplementation(() => {}) const store = useDocumentStore() const result = await store.pushToStore('42', 'my-store') diff --git a/frontend/src/features/document/store.ts b/frontend/src/features/document/store.ts index 3dfc68d..9e72420 100644 --- a/frontend/src/features/document/store.ts +++ b/frontend/src/features/document/store.ts @@ -2,6 +2,7 @@ import { defineStore } from 'pinia' import { ref } from 'vue' import type { Document } from '../../shared/types' import { appMaxFileSizeMb } from '../../shared/appConfig' +import { pushChunksToStore } from '../chunks/api' import * as api from './api' export const useDocumentStore = defineStore('document', () => { @@ -65,10 +66,10 @@ export const useDocumentStore = defineStore('document', () => { selectedId.value = id } - async function rechunk(id: string): Promise { + async function rechunk(id: string): Promise { try { - const res = await api.rechunkDocument(id) - return res.jobId + const chunks = await api.rechunkDocument(id) + return chunks.length } catch (e) { error.value = (e as Error).message || 'Failed to rechunk' console.error('Rechunk failed', e) @@ -78,7 +79,7 @@ export const useDocumentStore = defineStore('document', () => { async function pushToStore(id: string, store: string): Promise { try { - const res = await api.pushDocumentToStore(id, store) + const res = await pushChunksToStore(id, store) return res.jobId } catch (e) { error.value = (e as Error).message || 'Failed to push to store' diff --git a/frontend/src/pages/DocWorkspacePage.vue b/frontend/src/pages/DocWorkspacePage.vue index 194c425..e6feb07 100644 --- a/frontend/src/pages/DocWorkspacePage.vue +++ b/frontend/src/pages/DocWorkspacePage.vue @@ -28,6 +28,7 @@ :aria-selected="activeMode === m" :disabled="!modeEnabled(m)" :title="!modeEnabled(m) ? t('workspace.modeDisabled') : undefined" + :data-e2e="`tab-${m}`" @click="switchMode(m)" > {{ t(`workspace.tabs.${m}`) }} diff --git a/frontend/src/pages/DocsLibraryPage.vue b/frontend/src/pages/DocsLibraryPage.vue index 48c593c..d06bb79 100644 --- a/frontend/src/pages/DocsLibraryPage.vue +++ b/frontend/src/pages/DocsLibraryPage.vue @@ -369,12 +369,10 @@ function clearSelection(): void { async function bulkRechunk(): Promise { const ids = [...selectedIds.value] clearSelection() - const jobs = await Promise.all(ids.map((id) => docStore.rechunk(id))) - const dispatched = jobs.filter(Boolean) - if (dispatched.length) { - // Surface job ids as a brief toast via window.alert for now - // E9 (RunsPage) will provide proper job tracking - window.alert(t('docs.jobDispatched', { jobId: dispatched.join(', ') })) + const counts = await Promise.all(ids.map((id) => docStore.rechunk(id))) + const succeeded = counts.filter((n): n is number => n !== null).length + if (succeeded) { + window.alert(t('docs.rechunkDone', { n: succeeded })) } } diff --git a/frontend/src/shared/i18n.ts b/frontend/src/shared/i18n.ts index 166ae96..2654e78 100644 --- a/frontend/src/shared/i18n.ts +++ b/frontend/src/shared/i18n.ts @@ -76,6 +76,7 @@ const messages: Messages = { 'docs.pushSubmit': 'Ingérer', 'docs.pushCancel': 'Annuler', 'docs.jobDispatched': 'Job lancé ({jobId})', + 'docs.rechunkDone': 'Re-chunkage terminé ({n} document(s))', // Doc import (#214) 'docsNew.title': 'Importer des documents', @@ -587,6 +588,7 @@ const messages: Messages = { 'docs.pushSubmit': 'Ingest', 'docs.pushCancel': 'Cancel', 'docs.jobDispatched': 'Job dispatched ({jobId})', + 'docs.rechunkDone': 'Rechunk done ({n} document(s))', // Doc import (#214) 'docsNew.title': 'Import documents',