feat: multi-upload drag-and-drop, delete cascade, dark mode fix, chat readability

[Kaiohz]

Changes

Multi-file upload + drag-and-drop

• useUploadFiles hook with concurrency limit (3), partial failure handling, folder structure preservation via webkitRelativePath
• UploadButton supports multiple file selection
• RagPage browse section is a dropzone with visual feedback (overlay + hint bar)
• Folder drag-and-drop preserves structure in MinIO via FileSystem API (webkitGetAsEntry)

Delete cascade

• useDeleteFile now passes workingDir to backend for pgvector cascade
• ragApi.deleteFile sends working_dir query param (omitted when empty)

Dark mode fix

• useSettingsStore.applyThemeClass now also writes to composable-ui-theme localStorage key
• Fixes anti-FOUC inline script reading stale value on page reload

Chat readability

• leading-relaxed → leading-loose (1.625 → 2.0)
• Bubble padding p-3 → p-4 (12px → 16px)
• Added paragraph margins ([&_p]:my-1.5), list margins ([&_li]:my-1, [&_ul]:my-1), code block margins

Extensions

• Aligned frontend accept list with backend: added .svg, .bmp, .rtf, .odt, .ods, .doc, .xls, .ppt

Tests

• 548 frontend tests pass
• New: useUploadFiles.test.tsx (6 tests)
• Updated: UploadButton.test.tsx, useDeleteFile.test.tsx, ragApi.test.ts

[Kaiohz]

Code review — feat: multi-upload drag-and-drop, delete cascade, dark mode fix, chat readability

Reviewed commit df0c2ffee097f0c784d8e04cb5ee7414de53c829 (PR #21, 175 files, +7290/-3726).

Score: 7/10 — code soigné, tests solides, mais scope creep significatif et une dead dependency à corriger.

---

✅ Points forts

• useUploadFiles hook : pool de workers manuel propre (mapWithConcurrency), gestion d'erreur partielle explicite ({succeeded, failed, errors}), retourne un Error seulement si 100% d'échec. Limite de concurrence de 3 = bon défaut pour ne pas étrangler le backend.
• extractFilesFromDrop exporté et testable : Object.defineProperty(file, "webkitRelativePath", …) est le seul moyen propre de contourner le getter readonly de la spec — bien isolé dans un helper pur.
• Dark mode fix bien packagé : sync composable-ui-theme localStorage + anti-FOUC inline script dans index.html. Le commentaire // NOTE: theme is also managed by useThemeStore (legacy, still used by main.tsx…) assume la dette ouvertement, c'est bien.
• MARKDOWN_PROSE_CLASSES extrait : avant c'était de la duplication inline entre ChatMessage et d'autres composants de chat — maintenant c'est un module dédié, seul owner du styling prose.
• 6 tests pour useUploadFiles : happy path, structure webkitRelativePath, partial failure, readEntries — bonne couverture.
• useDeleteFile test : 2 tests ciblés (succès + erreur), propre, pas de sur-mocking.
• Fallthrough default-composable : la fallback est partagée entre useIndexFile/useIndexFolder au lieu d'être dupliquée (cf. le commit mcp-raganything #57 leçon similaire).
• Mergeable state : clean ✅, CI verte, 548 frontend tests pass.

---

⚠️ Points à traiter

1. 🔴 react-dropzone ajouté à package.json mais JAMAIS utilisé. Le dropzone est codé à la main dans RagPage.tsx (handleDrop/handleDragOver/handleDragEnter/handleDragLeave). Ça ajoute 41 lignes au package-lock.json pour du dead code. Action : npm uninstall react-dropzone ou l'utiliser réellement (il couvrirait une bonne partie du code manuel de RagPage).

2. 🟠 Scope creep majeur — le titre mentionne 4 features, mais la PR contient aussi :

   • Suppression complète de la pipeline LightRAG : useIndexFile.ts, useIndexFolder.ts, useRagQuery.ts, PipelineToggle.tsx, leurs tests, et streamEvent.ts/chunkResponse.ts domain entities (15 fichiers supprimés). C'est une décision architecturale qui mériterait sa propre PR (ou au minimum un flag explicite dans le titre : "feat: …, BREAKING: remove LightRAG pipeline").
   • Subagent timeline / ToolCallBadge / ToolResultBlock dans ChatMessage.tsx (+145 lignes) : gros changement UI/feature mélangé avec le commit "chat readability".
   • useChatStore.test.ts réécrit (+400 lignes, fichier added alors qu'il existait avant).

   Recommandation : split en 3 PRs au squash :

   • feat(multi-upload, delete-cascade) (le cœur)
   • refactor(rag): remove LightRAG pipeline, keep classical only
   • feat(chat): subagent timeline + readability

3. 🟠 Squash obligatoire avant merge : 2 commits, le premier f5ceaf4821 wip: agent config editors, chat, rag UI ne devrait pas rester en historique. HEAD actuel df0c2ffee0 est bon, juste à squash.

4. 🟡 extractFilesFromDrop : Object.defineProperty mérite un commentaire explicite. Le pattern est correct (c'est le seul moyen de fixer webkitRelativePath qui est readonly sur File) mais c'est non-évident. Sans commentaire, le prochain dev va l'enlever en se disant "c'est hacky".

5. 🟡 useDeleteFile typing : workingDir?: string puis workingDir ?? "" côté hook pour masquer l'undefined. Le port ragApi.deleteFile(objectName, workingDir: string) attend un string required. Préférer propager string | undefined jusqu'au port et laisser l'omission se faire là-bas (déjà fait dans ragApi avec if (workingDir) params.set(...)). Le ?? "" actuel n'est pas faux mais brouille le contrat.

6. 🟡 handleDragLeave e.currentTarget === e.target : pattern classique mais fragile sur les enfants. OK ici car l'overlay est absolute inset-0 (donc enfants du dropzone, pas trigger). Mais un commentaire serait utile vu que c'est exactement le piège classique des dropzones custom.

7. 🟡 useSettingsStore dual-store : le commentaire reconnaît lui-même que c'est du tech debt. La sync composable-ui-theme ajoute une 2e source de vérité. Acceptable pour ce ticket mais ouvrir un ticket de refacto (useThemeStore → absorbé par useSettingsStore).

8. 🟢 Nit — package.json ne mentionne pas @types/dom-webgl2 dans le body de la PR alors qu'il est listé dans les deps (vérifié : absent en fait, c'est react-dropzone qui a été ajouté). Le diff body est cohérent.

9. 🟢 Nit — README + QA_REPORT : +315+232 lignes de doc/screenshot. Très bien pour la traçabilité QA, mais ça grossit artificiellement la PR. Pull à part possible.

---

📋 Résumé

Catégorie	Verdict
Code quality	✅ Bon, propre, TDD respecté
Tests	✅ 6 tests useUploadFiles + 2 useDeleteFile, bien ciblés
Architecture	⚠️ LightRAG removal en douce dans une PR de feature
Scope	⚠️ 4+ features mélangées — split recommandé
Dependencies	🔴 react-dropzone inutilisé à retirer
Documentation	✅ QA_REPORT exhaustif avec screenshots
CI / mergeable	✅ clean, 548 tests pass

Verdict : request changes principalement sur la react-dropzone dead dep + le scope creep. Le code en lui-même est mergable, c'est la forme de la PR qui bloque.

---

Review postée via SoluBot.

[Kaiohz]

Code Review — PR #21

Branch: main ← feat: multi-upload drag-and-drop, delete cascade, dark mode fix, chat readability
HEAD SHA: 58b9382
Scope: 173 files, +7244 / −3726, 1 commit
Score: 7/10 — solid delivery on all the stated goals, but the commit is too large and ships repo-noise that should not be in git.

---

TL;DR

The four advertised features all work and are well-tested:

• ✅ useUploadFiles (concurrency-3, partial-failure handling, webkitRelativePath preservation) — clean, well-tested, the extractFilesFromDrop helper is properly split out for unit testing.
• ✅ useDeleteFile → ragApi.deleteFile now sends working_dir for pgvector cascade — minimal, correct, exactly the right shape.
• ✅ Dark mode fix: applyThemeClass now syncs to composable-ui-theme, so the anti-FOUC inline script in index.html and the Settings page toggle share the same key. Bug squashed, root cause explained in code comment.
• ✅ Chat readability: MARKDOWN_PROSE_CLASSES extracted to a constant in markdownStyles.ts (good — no duplication), leading-relaxed → leading-loose, padding p-3 → p-4, paragraph/list/code margins added. Visible and non-invasive.

Tests look thoughtful — 6/6 happy/sad paths on useUploadFiles (concurrency, partial failure, all-fail, webkitRelativePath, success shape), thorough useSettingsStore coverage including localStorage side-effects and DOM class toggles. Architecture (hexagonal ports/adapters) is preserved.

---

🔴 Blockers

1. 15 PNG screenshots + 2 QA reports committed to the repo (qa-screenshots/01-… through 09-…, QA_REPORT.md, QA_REPORT_SETTINGS.md).

This is 17 binary/text files in the working tree that have nothing to do with the feature. They should live in the PR description, an internal doc, or a CI artifact — not in main. Every clone now carries ~1-2 MB of PNGs that will never be referenced by the app.

👉 Action: remove qa-screenshots/ and the two QA_REPORT*.md files, re-commit (or amend). The visual evidence is already in the PR description.

2. The PR is a single commit, 173 files, +7244 / −3726.

Looking at the diff, this clearly bundles several independent concerns:

• A complete Tetris design system migration (Sidebar, AppShell, MainHeader, SettingsPage, layout refactor, Tailwind tokens, dual theme stores, +450 lines in index.css)
• Multi-upload + dropzone feature
• Delete-cascade wiring
• Dark mode fix
• Chat readability tweaks
• Accept-list extension
• RAG pipeline refactor (removal of useIndexFile, useIndexFolder, useRagQuery, IndexActionMenu, PipelineToggle, ragIndexApi, ragQueryApi, etc.)

Splitting this would have made review tractable and reduced blast radius. As-is, reverting "the dark mode fix" requires reverting a Tetris migration.

👉 Action (next time): one concern = one PR, or at minimum one commit per concern.

3. Dual theme stores — useThemeStore and useSettingsStore both manage theme.

useSettingsStore.ts even contains the comment:

theme is also managed by useThemeStore (legacy, still used by main.tsx for anti-FOUC). This store owns the Settings page theme toggle; the two stores are intentionally independent in this ticket. A future refactor should consolidate them into a single source of truth.

This is a known landmine shipped to main. The fix is small (one store subscribes to the other, or just one store), and the code comment itself acknowledges it. Two sources of truth for theme means:

• Toggling via Settings page → useSettingsStore updates, but useThemeStore.theme stays stale.
• The AppToaster in main.tsx reads useSettingsStore for the toaster theme — fine today, but any future component that reads useThemeStore will silently desync.
• Reading from localStorage directly in useSettingsStore.applyThemeClass and useThemeStore.applyThemeClass duplicates the same string "composable-ui-theme" in both files.

👉 Action: consolidate before merge, or open a follow-up issue with an owner and a deadline in the PR description (not just a code comment).

---

🟡 Suggestions

4. useUploadFiles.ts — MAX_CONCURRENCY = 3 is a magic constant.
Hard-coded at module scope. Should probably be a useUploadFiles({ concurrency?: number }) option, or at least lifted to a named export so tests and callers can override.

5. extractFilesFromDrop uses Object.defineProperty(file, "webkitRelativePath", …) to spoof a non-standard readonly getter.

This works (the comment in the code explains why), but it's the kind of thing that will break the day browsers or jsdom tighten the File API. Worth a JSDoc on the function explaining the contract: "Returns File objects with synthesized webkitRelativePath so the upload hook can preserve folder structure on drop. Files from <input webkitdirectory> already have the property set natively."

6. useUploadFiles.mutationFn returns two different shapes:

• All success → results.map(...) (array of API responses)
• Partial failure → { succeeded, failed, errors }

The UploadButton and RagPage both handle this with Array.isArray(data) ? success : warning. Discriminated union would be cleaner:

type UploadOutcome = { kind: "all-ok"; items: unknown[] }
                    | { kind: "partial"; succeeded: number; failed: number; errors: string[] }
                    | { kind: "all-failed" }; // throws, not returned

Today it's "guess the shape from the runtime type." This will bite the next caller.

7. useSettingsStore writes the theme to localStorage on every setter call, but setAccent early-returns on invalid hex without persisting or notifying.

Minor, but the asymmetry between setAccent (silent reject) and the other setters (no validation) is a small smell. Either validate everywhere or nowhere, and surface rejection to the caller.

8. RagPage.tsx is now 326 lines with 4 useCallbacks, two queries, a workspace query, and a dropzone overlay.

It's still readable, but it's the kind of file that wants to be split into useRagPageDropzone, useRagPageNavigation, etc. The dropzone logic specifically (handlers + extractFilesFromDrop call) is a great candidate for its own hook.

9. tailwind.config.ts shows +34 / −54. Worth eyeballing the diff to confirm the token system is intentional and that dark: variants are coherent with the new applyThemeClass flow.

10. index.css +450 / −76. Big change. Spot-check that:

• The new CSS custom properties (--app-font-scale, --app-font-family, --chat-font-size) compose cleanly with the theme tokens.
• text-fg / text-fg-2 / bg-surface-warm are defined for both html.dark and :root (light) — light mode is a new thing in this PR and token coverage bugs are easy to ship.

11. ConfirmDeleteDialog.tsx and CreateFolderDialog.tsx are new. I didn't deep-dive but both have dedicated tests, so likely fine. Worth a glance for accessibility (focus trap, ESC handling) given they're user-blocking dialogs.

12. useChatStore is a complete rewrite (+80 / −41, +400 lines of new tests). Big surface area. I'd want to see the diff in isolation, not buried in a 173-file PR.

13. TraceEvent and the events?: TraceEvent[] plumbing on ChatMessage — the new buildSubagentTimelines / parentThinking / parentToolEvents helpers are good (pure, testable), but TraceEvent.source === null for parent vs subagent is a sentinel-by-convention. A discriminated union ({ kind: "parent" } | { kind: "subagent"; name: string }) would be safer.

---

✅ Things done well

• Anti-FOUC fix is correct and root-caused — reading the same key in index.html and writing it from the store closes the loop. The fact that this was a real bug (the inline script read a key nothing was writing to) makes the fix satisfying.
• useSettingsStore test coverage is exemplary — DOM class toggles, CSS variables, localStorage round-trip, defaults, reset. Whoever wrote this cared.
• Hexagonal architecture preserved — ragFilePort.ts got the workingDir parameter, ragApi.ts got the query param, no shortcut through the API layer.
• Dropzone UX detail — e.currentTarget === e.target for handleDragLeave is the right call (prevents flicker when crossing child elements). Nicely commented.
• MARKDOWN_PROSE_CLASSES extracted to a constant — avoids future Tailwind class duplication across message components.
• Accept list aligned with backend — .svg, .bmp, .rtf, .odt, .ods, .doc, .xls, .ppt added; small but it would have been a silent UX bug otherwise.

---

Recommendation

Request changes for:

1. Remove qa-screenshots/ and QA_REPORT*.md from the tree.
2. Either consolidate useThemeStore / useSettingsStore now, or open a tracked follow-up issue before merge.

Once those are addressed, this is mergeable. The actual feature code is solid — the friction is around packaging (one big commit, repo noise, dual-store debt) more than the code itself.

Happy to re-review after the cleanup.

[Kaiohz]

Code Review — PR #21

Branch: main ← feat/delete-cascade-multi-upload-darkmode-chat
HEAD SHA: 7530faaa6b7ac71dd2bb909a4d6219abec93eb4e
Scope: 152 files, +6804 / −3725, 1 commit (squashed)
CI: ✅ test — success, mergeable_state: clean

Thanks for the force-push — the diff is much cleaner now (1 squashed commit, dead code removed, no leftover TopNav/MainLayout/PipelineToggle traces). The scope is ambitious (RAG multi-upload, delete cascade, dark mode fix, chat readability, several new chat surfaces) and the new tests (useUploadFiles.test.tsx, useDeleteFile.test.tsx, UploadButton.test.tsx, ragApi.test.ts, useChatStore.test.ts, traceEvent.test.ts) are a clear net positive.

Overall 7.5/10 — solid functional delivery with a few concerns around a couple of edge cases and one potential bug I'd like addressed before merge. Details below.

---

✅ Highlights

• useUploadFiles concurrency map (mapWithConcurrency in useUploadFiles.ts:8) — clean implementation, single-pass with a shared index counter, no race in the results[currentIndex] assignment.
• partial / all-ok / throw outcome shape (useUploadFiles.ts:80) — explicit, type-safe, easy to consume in the UI. Toast in UploadButton and RagPage correctly maps each branch.
• Drag-and-drop folder traversal (extractFilesFromDrop / traverseEntry in useUploadFiles.ts:32) — the Object.defineProperty workaround to set webkitRelativePath on a File is the only way; the comment justifying it is a nice touch.
• Delete cascade — ragApi.deleteFile correctly sends working_dir only when non-empty (ragApi.ts:39), matching the documented backend contract.
• Dark mode fix — useSettingsStore.applyThemeClass now also writes composable-ui-theme (useSettingsStore.ts:60), which the anti-FOUC inline script in index.html:8 reads. Closes the reload race.
• appendTraceEvent typed accumulator (useChatStore.ts:42) — subagent keying, parent vs subagent branching, structured_response parsing, and isStreaming flip on AI_MESSAGE are all handled in one pure transition.
• isValidTraceEvent guard in chatApi.streamMessage (chatApi.ts:24) — runtime validation of the SSE event shape is a real defensive win, and the test coverage in traceEvent.test.ts confirms the contract.
• Drag-leave fix in RagPage (RagPage.tsx:128) — the currentTarget === target check correctly handles the "leaving a child, not the dropzone" case. Comment is well-placed.
• Test coverage on the new bits — useUploadFiles.test.tsx covers all 4 interesting paths (concurrency, webkitRelativePath, no-relativePath, partial failure); ragApi.test.ts covers the new working_dir query param; useChatStore.test.ts covers parent vs subagent routing including structured_response valid / missing / malformed.
• Chat readability — the readability pass is well-scoped and the shared MARKDOWN_PROSE_CLASSES constant (markdownStyles.ts) is reused in 3 places.

---

🟡 Should fix before merge

1. ⚠️ Race: appendTraceEvent flips isStreaming=false on AI_MESSAGE, but the SSE stream may still be flushing events after that (useChatStore.ts:96)

In useStreamChat.ts:42, onComplete is what the SSE wrapper calls when the connection closes — it invalidates the history query and clears isStreaming / pendingUserMessage. But the store now also sets isStreaming=false on the first AI_MESSAGE event (useChatStore.ts:96). If the backend sends additional events (subagent CONTENT/TOOL_CALL/TOOL_RESULT) after the AI_MESSAGE (e.g. a final late TOOL_RESULT), appendTraceEvent will still be called — fine for the data — but the isStreaming=false flip happens earlier than the connection actually closing, so the "Processing…" / "Thinking…" spinner disappears while events are still arriving. Visually this shows up as: AI bubble appears, then 200ms later tool badges pop in, in a second non-streaming render. Inconsistent UX.

Suggestion: keep isStreaming=true until either (a) the SSE onclose callback fires, or (b) only flip it in onComplete. The AI_MESSAGE event should just trigger the structured_response parse and turn off the "Processing…" indicator inside the streaming bubble, not the global flag. Consider adding a streamingTurn.complete: boolean derived in clearStream instead.

2. ⚠️ getDirFromFile returns "" for top-level files in a folder (useUploadFiles.ts:21)

function getDirFromFile(file: File): string {
  const relativePath = (file as File & { webkitRelativePath?: string }).webkitRelativePath;
  if (!relativePath?.includes("/")) return "";
  ...
}

If a user drops a folder "Reports" containing "Q4.pdf" directly at the root of the browse view, the file's webkitRelativePath is "Reports/Q4.pdf" and the prefix becomes currentPrefix + "Reports/" — correct. ✅

But if they drop a folder "Reports" containing a top-level file like "Q4.pdf" alongside a sub-file "sub/Q5.pdf", both work because they share the same prefix. ✅

The edge case is a folder dropped at the root of an open subfolder: if currentPrefix="docs/" and the user drops folder "Reports" containing only "Q4.pdf", the file lands in docs/Reports/ (correct). But if the user drops a plain file into a subfolder (no webkitRelativePath set), it goes to docs/ (correct). The code is right, but the missing webkitRelativePath case means a user dropping a folder via <input webkitdirectory> works, while a user dropping the same folder via drag-and-drop only works if traverseEntry is invoked (i.e. only from extractFilesFromDrop). The UploadButton path doesn't go through extractFilesFromDrop, so dropping a folder onto the file input does nothing — the browser will filter out the folder entries for a plain <input type="file" multiple>. That's a UX gap, not a code bug, but worth documenting in the prop docstring.

3. ⚠️ useStreamChat swallows stream errors when AbortController is in flight (useStreamChat.ts:35)

abortRef.current = chatApi.streamMessage(...) stores the controller. If the user navigates away mid-stream, the useEffect cleanup on line 60 aborts — but setError(null) / setStreaming(false) cleanup is only done in onComplete, not in the abort path. The store ends up in streamingTurn != null, isStreaming=true, pendingUserMessage="..." until the user navigates back. The clearStream is called only in onComplete and in the explicit cancel() path. If the React component unmounts, the abort fires but the store state is dirty.

Suggestion: in the useEffect cleanup, call useChatStore.getState().clearStream() in addition to abortRef.current.abort(). Same for the catch on onError — setError is called but streamingTurn is not cleared, so a future initStreamingTurn will overwrite it correctly, but the previous turn's events stay attached until then.

4. ⚠️ chatApi.streamMessage returns a controller, but the caller never sees the URL Promise rejection (chatApi.ts:104)

streamUrl().then((url) => { fetchEventSource(...) }) — if configRepository.getConfig() rejects (e.g. config not loaded yet), the onError callback is never invoked, the controller is never aborted, and onComplete is never called. The store will sit in isStreaming=true forever.

Suggestion: chain a .catch on the streamUrl().then to call onError(err) and resolve the leak.

5. ⚠️ default-composable workingDir fallback repeated 3x in RagPage (RagPage.tsx:99, 106, 158)

const dir = workingDir || currentPrefix || "default-composable";

This is a quiet business rule (which workspace wins when the user hasn't picked one) embedded inline three times. If we change the fallback, we have to remember all three sites.

Suggestion: extract a resolveWorkingDir(currentPrefix, workingDir) helper either in RagPage or, better, in a rag/workingDir.ts module so the unit tests can cover the rule explicitly.

---

🟠 Worth addressing soon (not blocking)

6. Inline anti-FOUC script in index.html:8 reads composable-ui-theme as the SOLE source of truth — this works for first paint, but the useSettingsStore initial state ALSO reads composable-ui-settings (useSettingsStore.ts:69). If both keys are present and disagree (e.g. partial migration), the inline script wins for theme (correct) but the store hydration takes the stale theme value. After applyThemeClass(initialState.theme) runs on store init (useSettingsStore.ts:88), if the stored theme disagrees with the inline-script value, the page will flicker from light → dark on hydration. Worth checking with the actual flow: the inline script runs first, then the store hydrates and re-applies the same theme. If they match, no flicker. If they differ (stale storage), brief flicker.

Suggestion: in useSettingsStore.ts:69, when hydrating, prefer localStorage.getItem("composable-ui-theme") over the nested parsed.state.theme for the initial theme. Or: write to BOTH keys in applyThemeClass (already does the theme-only key) and remove the redundancy. The current code does write to both — good — but the read path still goes through composable-ui-settings only.

7. ConfirmDeleteDialog name interpolation is misleading (ConfirmDeleteDialog.tsx:33)

<DialogTitle className="font-display text-lg">Delete {name}</DialogTitle>

For a file, name = objectName (e.g. "docs/reports/q4.pdf"), so the title becomes "Delete docs/reports/q4.pdf" — the full path leaks into the title. The "name" prop is being conflated with display name. Same for folders, where the full prefix leaks.

Suggestion: pass a displayName prop derived in the caller (basename of the object name, folder name extracted from the prefix). Or derive it inside the dialog with a small helper. Visual polish, not a bug.

8. RagPage.handleCreateFolder builds the prefix by string concat (RagPage.tsx:135)

const prefix = currentPrefix + name;

If currentPrefix is "docs/" and name is "reports", the result is "docs/reports" — correct, but missing the trailing /. The CreateFolderDialog then calls ragApi.createFolder("docs/reports") (no trailing slash). The listFolders filter then does params.set("prefix", "docs/") and the comparison is "docs/reports".startsWith("docs/") — works for listing but downstream listFiles("docs/") will return files inside docs/reports because the prefix docs/reports is a sub-path. This is a latent inconsistency. Suggest normalizing: always end folder prefixes with / at the boundary, or document and test the contract explicitly.

9. RagPage.handleFileIndexClassical uses default-composable as the workspace fallback, but ragApi.uploadFile doesn't pass working_dir at all (RagPage.tsx:99)

Upload goes through useUploadFiles → ragApi.uploadFile(prefix, file) and ignores workingDir. Indexing goes through useClassicalIndexFile and reads workingDir. So a file uploaded to docs/reports/q4.pdf indexed with workingDir="docs/" will be stored in pgvector under the wrong workspace. This is the same latent mismatch I called out in #5 — fixing the helper would surface it.

10. SubagentPanel uses useId to generate aria controls but doesn't expose it as an expandable section — fine for now, but if you add a collapse/expand later you'll need to refactor. Not blocking.

11. useUploadFiles exposes DEFAULT_UPLOAD_CONCURRENCY but no override path — if a user drops 100 files, concurrency=3 means 34 sequential rounds. Consider letting the caller pass { concurrency: 10 } for bulk operations, or computing concurrency from the batch size (e.g. min(8, ceil(n/4))).

12. useSettingsStore.toggleTheme and setTheme both call applyThemeClass + persist(get()) — duplicated. Consider one private _applyTheme(theme) helper that does both.

13. applyThemeClass doesn't re-write --meta / --accent to keep the theme tokens consistent — if the user changes accent while toggling theme, the order matters: setTheme → setAccent will rewrite the CSS vars twice. Minor perf, not a correctness issue.

---

🟢 Nits (optional)

• useStreamChat.ts:13 — generateTurnId uses crypto.randomUUID if available, but the fallback uses Date.now() which is collision-prone on rapid double-clicks. Consider Math.random().toString(36).slice(2, 10) + Date.now().toString(36).
• useUploadFiles.ts:34 — webkitGetAsEntry?.() returns null (not undefined) when the item is not a file. The if (entry) guard handles it, but the getAsFile() fallback after is dead code if the item is a file (entry is always non-null in that case). Could simplify.
• RagPage.tsx:240 — the "Glissez-déposez des fichiers" hint is in French while the rest of the UI is in English. Inconsistent.
• index.html:18 — lang="fr" is set on the <html> but the UI is English. Either flip the lang attribute or translate the strings.
• useSettingsStore.ts:75 — comment says "Only --accent, --meta and --accent-on are driven by the store" but applyChatTypography also writes to --app-font-scale, --app-font-family, --chat-font-size. Comment is stale.
• SubagentPanel.tsx:46 — the memo equality is by reference for toolCalls / toolResults, which is fine because useChatStore always returns new arrays. Good.
• MessageList.tsx:73 — useDeferredValue on streamingTurn?.parentContent is a good choice; the streaming render stays responsive.
• useUploadFiles.test.tsx:65 — vi.mocked(ragApi.uploadFile).mockImplementation(async () => ({ message: "uploaded" })) returns { message: "uploaded" } but the test then expects data.kind === "all-ok". That works because the hook only checks r.ok, not the shape. Worth a comment in the test explaining the contract.
• RagPage.tsx:177 — the dropzone overlay uses z-50 which can clash with the mobile sidebar overlay (Sidebar.tsx:127, also z-50). On a mobile view, drag-and-drop on the rag view while the sidebar is open will z-fight. Edge case.

---

Summary

Score: 7.5 / 10

Strong PR — feature-complete, well-tested on the new hooks, the architecture decisions (concurrency map, partial-success discriminated union, typed SSE event guards, structured response parsing in the store) are all defensible. The two issues I'd want fixed before merge are the isStreaming flip on AI_MESSAGE (#1) — it has visible UX impact and is reachable in production — and the streamUrl Promise rejection in chatApi.streamMessage (#4) — silent failure mode that leaves the store in a broken state. The default-composable fallback duplication (#5) is a small refactor that will save you a debugging session later.

Happy to re-review once the first two are addressed. The rest can ship as follow-ups.

---

Reviewed by SoluBot — Composable UI PR #21 — HEAD 7530faaa

[Kaiohz]

Code Review — PR #21

Verdict : 🟡 REQUEST CHANGES — Score 6.5/10

La livraison fonctionne (CI verte, tests OK, refactor de design system cohérent) mais plusieurs problèmes de scope, cohérence de description et au moins un bug fonctionnel latent doivent être traités avant merge.

---

🔴 Critiques (à fixer avant merge)

1. PR title / description ne couvrent pas ~75% du scope réel

• Title : multi-upload drag-and-drop, delete cascade, dark mode fix, chat readability
• Title omet : refonte complète du design system (tokens Tetris, font pixelisée Press Start 2P, keyframes), nouveau useSettingsStore + useSettingsPage (Theme/Accent/Chat/LLM/Reset), Sidebar + MainHeader + AppShell refactor, TraceEvent (6 types) en remplacement de StreamEvent, SubagentPanel, ToolCallBadge, ToolResultBlock, StructuredResponseLive, useSidebarStore, breaking changes (useMessages → useThreadHistory).
• 153 fichiers, +6810/-3725 dans un seul commit : impossible à reviewer sérieusement. Squash en sous-PRs thématiques ou au minimum mettre à jour la description pour qu'elle reflète la réalité. Le reviewer doit savoir qu'il review un redesign global, pas une feature incrémentale.

2. Bug fonctionnel — Object.defineProperty(file, "webkitRelativePath", ...) ne marche pas

Dans useUploadFiles.ts:traverseEntry (lignes ~6570-6585 du diff) :

Object.defineProperty(file, "webkitRelativePath", {
  value: path + file.name,
  writable: false,
});

Sur Chromium/Edge/Firefox, webkitRelativePath est un getter natif défini sur le prototype File.prototype (non-configurable en pratique). defineProperty sur l'instance sans configurable: true et sans que la propriété soit "own" de l'instance lève TypeError: Cannot redefine property: webkitRelativePath en mode strict (donc en TS). Résultat : le drag-and-drop de dossier ne fonctionne pas en prod, seulement dans des cas où le browser implémente webkitRelativePath via une property d'instance (rare).

Le commentaire dans le code le reconnaît implicitement ("is a readonly getter") mais la solution proposée ne fonctionne pas. Alternatives :

• Conserver le path en map externe (Map<File, string>) et passer le dir calculé en paramètre à ragApi.uploadFile, sans dépendre de webkitRelativePath.
• Renommer le paramètre en dir côté API et ne plus lire webkitRelativePath côté hook.

Bloqueur.

3. Incohérence README ↔ code

• README ligne 127 : useUploadFile, useCreateFolder, useDeleteFile, useDeleteFolder, useClassicalIndexFile, useClassicalIndexFolder, useClassicalQuery
• Mais le hook s'appelle useUploadFiles (pluriel). Le README n'est pas à jour et va induire les contributeurs en erreur.
• index.html passe lang="en" → lang="fr" sans qu'aucun texte UI ne soit localisé (toujours en anglais). Incohérent.

---

🟠 Importants (à fixer rapidement)

4. Tests — couverture insuffisante vu l'ampleur du refactor

• 548 tests frontend pass, mais seulement 6 nouveaux tests sur le hook le plus risqué (useUploadFiles : concurrency, partial failure, drop de dossier avec webkitGetAsEntry).
• Aucun test sur :
  • useSettingsStore (persistence, anti-FOUC sync avec composable-ui-theme, partial state restore)
  • extractFilesFromDrop (la logique la plus complexe)
  • resolveWorkingDir (3 branches, importance sécurité)
  • TraceEvent reducer de useChatStore (logique de streaming la plus subtile)
  • useDeleteFile / useDeleteFolder (silently fallback to DEFAULT_WORKING_DIR = default-composable → risque de supprimer le workspace d'un autre projet)
• Le fix "Indexing without a working_dir" est documenté comme Bug 1 dans le README, mais aucun test ne couvre ce court-circuit.

5. resolveWorkingDir — fallback silencieux dangereux

src/application/lib/workingDir.ts:

export function resolveWorkingDir(currentPrefix: string, workingDir: string): string {
  return workingDir || currentPrefix || DEFAULT_WORKING_DIR;
}

Utilisé dans 3 endroits (handleFileDelete, handleFolderDelete, l'upload avec prefix vide). Si un user supprime un fichier sans avoir sélectionné de working_dir, on tombe sur default-composable — il peut supprimer les vectors d'un autre projet par accident. La description de la PR parle d'un fix de cascade delete, mais ce fallback n'a aucune confirmation utilisateur. Suggestion : logger un console.warn et/ou ajouter le ConfirmDeleteDialog (qui existe déjà) à ce flow de confirmation.

6. useUploadFiles — getDirFromFile retourne "" pour les fichiers simples

function getDirFromFile(file: File): string {
  const relativePath = (file as File & { webkitRelativePath?: string }).webkitRelativePath;
  if (!relativePath?.includes("/")) return "";
  ...
}

Pour un fichier uploadé via le picker (input multiple, pas de drop), webkitRelativePath === "" → on appelle ragApi.uploadFile(prefix + "", file). Le file est uploadé à la racine même si prefix est some/folder/, pas dans le sous-dossier. Vérifier que ragApi.uploadFile n'a pas un guard contre prefix === "" ou un effet de bord sur le path.

7. useUploadFiles — race condition + ordering des erreurs

• results[currentIndex] = { ok: false, error } : OK car l'index est dans une variable locale capturée par le worker.
• Mais : si tous les uploads échouent, on throw new Error(errors[0]) — on perd les N-1 autres erreurs. La toast affichera juste la première. Idem pour le data.errors retourné dans le cas partial qui n'est jamais consommé par l'UI (seul succeeded et failed sont affichés).
• Le test couvre 6 cas mais ne teste jamais l'UI rendering de data.kind === "partial".

8. useSettingsStore — double source de vérité thème

• Le store écrit dans composable-ui-settings (state complet JSON) et dans composable-ui-theme (string "dark"|"light") via applyThemeClass.
• index.html lit composable-ui-theme au boot (anti-FOUC) mais le toggle UI est sur composable-ui-settings.
• Si les deux divergent (race condition pendant que main.tsx lit le store), on a un flash. Le commentaire README ligne 83 le reconnaît ("slated for removal in a future refactor") mais ne le fixe pas maintenant.
• Au minimum : un test unitaire qui vérifie que setTheme("light") met bien à jour les deux clés.

9. Changement lang="en" → lang="fr" non justifié

Aucune UI string n'est localisée. Soit on revert, soit on commit aussi la locale FR. Sinon on a un attribut HTML menteur qui affecte l'a11y (screen readers, hyphenation).

---

🟡 Mineurs (nice-to-have)

10. useUploadFiles — magic number DEFAULT_UPLOAD_CONCURRENCY = 3

Pas de commentaire justifiant le 3 (capacité backend ? rate-limit ? test empirique ?). Suggestion : extraire dans un fichier de config ou commenter le rationnel.

11. extractFilesFromDrop — pas de gestion de quota

Si un user drop 5000 fichiers, on lance 5000 reads asynchrones via traverseEntry × Promise.all. Pas de backpressure ni de limite de taille. mapWithConcurrency (utilisé après) protège l'upload, mais pas la phase d'extraction.

12. mapWithConcurrency — results[i] = ... dans la boucle

results[i] n'est jamais réassigné à un index déjà écrit, OK. Mais le typage UploadResult permet undefined (les trous avant l'arrivée d'un worker). Considérer results.fill({ ok: false, error: "cancelled" } as UploadResult) au début pour garantir un tableau dense.

13. README ligne 175 — "concurrency 3 simultaneous requests" mais pas de bouton UI pour configurer. Cohérent avec "magic number" ci-dessus.

14. useDeleteFile / useDeleteFolder — pas de onMutate optimistic update

On attend l'invalidation du query → re-fetch → re-render. Sur un gros workspace, l'UI peut paraître "gelée" 1-2s. Suggestion : optimistic remove + rollback on error (côté onMutate).

15. Tailwind v4 @theme — 7 nouveaux tokens d'espacement (--spacing-1 à --spacing-12) mais l'ancien --space-X reste utilisé dans le CSS. Mort ou transition ?

---

✅ Points positifs

• Fix anti-FOUC dark mode propre : useSettingsStore.applyThemeClass synchronise les deux clés, et le inline script dans index.html lit la bonne clé au boot. Fix légitime et bien commenté.
• Multi-upload : mapWithConcurrency est une impl simple et correcte (sauf le bug defineProperty).
• Partial failure : le discriminated union UploadOutcome (all-ok vs partial) est propre et TypeScript-friendly.
• Delete dialog : ConfirmDeleteDialog (warning récurseur pour les folders) — bonne UX.
• README : la nouvelle section "RAG File Browser" est claire, mais doit être alignée avec le code (cf. point 3).
• Tests count stable : 548 → 548 + 6 (mais 6 c'est faible vu l'ampleur).
• CI verte : test (22.x) success en un seul run. Pas de flaky.
• Indexing guard "Set a working_dir first" — bonne amélioration UX, court-circuit avant l'appel API.

---

🛠️ Recommandations actionnables

1. Squasher en 4-5 PRs thématiques (ou au moins updater la description) :
   • RAG: multi-upload + delete + cascade
   • Theme tokens + Settings page
   • Layout (AppShell, Sidebar, MainHeader)
   • Chat refactor (TraceEvent, useThreadHistory, subagent panel)
2. Fixer le bug Object.defineProperty (point 2) — bloquant.
3. Ajouter 15-20 tests sur les zones à risque (points 4, 7, 8).
4. Aligner README sur le code (point 3) ou revert lang="fr".
5. Logger un warning quand resolveWorkingDir tombe sur DEFAULT_WORKING_DIR (point 5).
6. Re-run les tests manuels sur le drag-and-drop de dossier (point 2) avant merge.

---

Score : 6.5/10

• +2 pour la feature multi-upload bien pensée (sauf bug defineProperty)
• +1.5 pour le design system refactor (cohérent, tokenisé, dark/light)
• +1 pour les fixes UX (FOUC, working_dir guard, ConfirmDeleteDialog)
• -2 pour le bug fonctionnel latent (drag-and-drop dossier)
• -1 pour la couverture de tests sur le refactor
• -1 pour le scope non documenté
• -0.5 pour les incohérences README/lang

---

Review basée sur le diff public de la PR + API metadata (CI status, fichiers, commits). Token GitHub local expiré — review non postée en commentaire GitHub, transmise ici en intégralité.