Studio: open a project source in a preview and quick-edit modal - #8870
Open
Rafael-Silva-Oliveira wants to merge 26 commits into
Open
Studio: open a project source in a preview and quick-edit modal#8870Rafael-Silva-Oliveira wants to merge 26 commits into
Rafael-Silva-Oliveira wants to merge 26 commits into
Conversation
The project Sources panel rendered every document as a flat wrap of chips with no way to find one by name and no cap, so a project with dozens of sources pushed the rest of the page off-screen and removal was one chip at a time. Adds a permanent search field, sort by uploaded/name/size, multi-select with a confirmed bulk remove, and auto-collapse past 25 sources behind a "Show all" toggle. Filter/sort/collapse logic lives in a plain .ts module so the node:test suite can cover it; the suite cannot load .tsx. The formatting helpers the settings uploaded-files list already had (formatSize, formatUploadedAt, toSortTime, fileTypeLabel) move alongside it so both call sites share one copy rather than drifting. Selection is opt-in on DocumentStatusChip, so the thread bar and knowledge base dialog keep the plain chip. Linked-folder documents and not-yet- uploaded chips stay out of selection: the DELETE route answers 409 for linked-folder rows, so offering a bulk remove for them would promise an action the backend refuses. sizeBytes was only computed by the settings /documents route, so sorting a project's sources by size had nothing to sort on. It moves into _doc_view behind a shared _stored_size helper, which also de-duplicates the inline copy in list_all_uploaded_documents. list_documents did not select stored_path at all, so every scoped list reported a null size regardless. No scope, chunk, ingestion or retrieval code is touched, so what the model can retrieve is unchanged. Removal stays non-destructive to the user's filesystem: _remove_stored_upload resolves with realpath and deletes only inside Unsloth's uploads root, which the new backend tests pin down along with the symlink-redirect and outside-the-root cases. Refs unslothai#8840 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two issues raised in review on unslothai#8850. remove() captured `documents` from the render that created it and, on failure, restored that whole snapshot. Mid-batch that snapshot still holds the rows earlier iterations deleted, so one failure resurrected every already-removed source, and each returned 404 on its next action. It also swallowed the error, so the batch could not tell a failure from a success and cleared the selection regardless. remove() now restores only the single row it removed, and returns whether the delete actually landed. Dropping `documents` from its dependencies also stops the callback changing identity on every list update. Existing callers use `void remove(...)` and ignore the result, so the new return is additive. The bulk loop collects failures, keeps exactly those selected so a retry is one click, tells the user, and refreshes against the server so the list cannot keep showing rows the backend no longer has. isBulkRemovable also let through rows that are pending or running once they had a server id, so "Select all" could select an indexing document that DocumentStatusChip deliberately refuses to select individually, and bulk remove would then race the live ingestion worker. It now excludes them, and moves into source-list.ts so the predicate the chips and Select all share is covered by tests. Refs unslothai#8840 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The per-file checkbox claimed the left edge of every chip and the chip body claimed the click, which is the affordance the upcoming preview panel needs: clicking a source should open it on the right, not toggle a checkbox. Selection becomes all-or-nothing. Select all highlights every eligible chip, Deselect all clears it, and there is no per-file toggle in between, so the chip keeps only its file icon and its individual "x". The highlight is a border, tint and ring rather than a control. Selection is now derived from the visible list rather than stored as a set of ids, which removes a class of staleness: an id set had to be pruned whenever a document was deleted elsewhere, finished indexing, or dropped out of the search. A boolean cannot go stale, and it resets when the selection would cover nothing. Bulk remove drops "Clear" and no longer keeps failed rows selected. With no per-file toggle the user cannot curate a retry set, so a partial selection would misreport what the next removal is about to delete; it clears and, on failure, still reconciles against the server. isBulkRemovable keeps gating which chips highlight, so pending, running, optimistic and linked-folder rows are excluded from Select all exactly as before. Refs unslothai#8840 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three issues from the second review pass. Selection was derived live from the visible list, so it silently rewrote what Bulk remove would delete: selecting all matches for a narrow query and then clearing the query extended the set to up to 25 unrelated sources, and expanding a collapsed list added every newly revealed row. "Select all" now captures the ids it covered at the moment it was pressed, so the action can only ever delete what was highlighted when the user chose it. The stored set is pruned as documents leave the list, which is what the derived version was buying; narrowing the search no longer deselects, since the highlight simply hides with the chip. An optimistic upload has neither createdAt nor sizeBytes, so every sort mode ranked it last and, on a project past the collapse limit, the new chip was sliced straight out of the visible slice: the upload looked like it never started, and completion only patches status so it could stay hidden. Rows still in flight now pin to the top in every mode. Bulk remove reconciled only after a failure. A poll or an external refresh overlapping the sequential loop can read a document before its DELETE lands and re-insert the optimistically removed row afterwards, so an entirely successful batch could still leave a deleted source on screen. It now refreshes after every batch. Per-chip remove is disabled while a batch runs; a concurrent single delete would race the loop and make it report a false failure on the 404. Refs unslothai#8840 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
"Select all" built its set from the rendered slice, so on a project of 27 sources with 25 shown it selected 25 and silently left two behind. Collapsing is a display cap; it must not narrow what an action applies to. The set now comes from the matched list, so it spans every source the current search matches whether or not the list is expanded. Search scoping is unchanged: Select all still covers only what the query matches, and still excludes linked-folder, indexing and optimistic rows. Adds tests pinning collapse as display-only: Select all covers all 27 of 27, expanding does not change the covered set, a 30-match query past the render cap selects all 30, and the rendered slice is always a prefix of the matched list rather than a different membership. Retrieval was never affected. visibleSources and SOURCES_COLLAPSED_LIMIT are referenced only by this panel, and the search_knowledge_base tool resolves documents by project scope straight from the database, so what the model can retrieve has never depended on what the panel renders. Refs unslothai#8840 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two issues from the third review pass, both confirmed in the code. Pinning in-flight rows to the top of the sort was not enough on its own. Job completion patches only status, error, progress and numChunks, so an upload that finishes before the four-second indexing poll keeps the optimistic row's missing createdAt and sizeBytes. The pin releases the moment status flips to "completed", the row sorts to time 0, and past the collapse limit it drops straight out of the visible slice. Sorting cannot repair that -- there is no timestamp to rank by -- so handleFiles now refreshes after every upload and the row picks up its real server metadata. Uploads during a bulk remove could also destroy data. Ingestion dedups by content hash and returns the existing document id, so re-adding a file that matches a document still queued in the sequential delete loop resolves to that id and the loop then deletes it: the upload reports success and no source survives. The picker and the drop handler are both refused while a batch runs, the drop guard reading a ref so it cannot act on a stale capture, and Add sources is disabled to match. The empty-state button is untouched, since a project with no sources has no batch to collide with. Refs unslothai#8840 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
refresh() had no request-generation guard, so response ordering decided the final state. The indexing poll and the external-update subscription both fire on their own schedules: one that read the list before a DELETE could resolve after the reconciliation that followed it and write its pre-delete rows last, putting a removed source back on screen where it would 404 on its next action. Awaiting the reconciliation did not help, because the losing request was already in flight. Refreshes now take a ticket and write only while it is the newest. Deletes invalidate in flight requests as well, so a list read taken before a removal is discarded rather than allowed to reinstate the row. The counter lives in a small RefreshGeneration class rather than inline in the hook, so the ordering rules are unit tested: newest wins regardless of arrival order, a delete staleness-marks earlier reads, the reconciliation after a delete still writes, and a full bulk batch keeps every earlier request stale. Refs unslothai#8840 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Clicking a source chip opened nothing. This adds a modal that displays it -- PDFs rendered, docx as the text Unsloth indexed, markdown rendered with a View/Edit toggle -- and lets plain-text sources be edited in place. Escape, a backdrop click and the corner X all close it, confirming first when there are unsaved edits. A modal rather than a side panel because the project page already docks Run settings on the right. Saving writes the new text to a fresh file in the managed uploads root and re-ingests it as a replacement, carrying scope, project_id, kb_id and thread_id over unchanged so retrieval keeps finding the source where it was. The document it replaces is retired only once the re-index completes, so a failed parse or embed leaves the original searchable instead of destroying it. That reuses the existing `replaces` mechanism, exposed as one keyword argument on start_ingestion and guarded against being combined with dedupe. Nothing outside the uploads root is ever opened for writing. Linked-folder sources are therefore display-only: their stored file is a snapshot that folder sync recreates from the user's real file, so an edit here would be silently wiped by the next sync -- and the original in the user's folder is never touched either way. PDFs and Word documents are display-only too, having no faithful plain-text round trip. Which of those applies is decided by the backend and sent as `editable` plus a reason, so the client holds no list of file extensions and the two cannot disagree about what may be saved. HTML is shown as source text, never rendered, matching the text/plain the file route already serves it as. Refs unslothai#8840
The reason appeared twice on screen -- under the title and again in the footer -- and a read-only source carried both a Close button and the corner X. Keep the footer copy and the X. Also drop "Unsloth" from the Word-document wording, which read as branding inside the product. Refs unslothai#8840
Markdown had a View/Edit pair; HTML fell through to raw source with no way to see the page. Replace the boolean markdown flag with a `preview` mode the backend picks per extension, so the modal switches on one field and a newly supported upload type gets its viewer by naming it there rather than by touching the client. HTML now renders through the chat artifact canvas (ArtifactHtmlFrame): a sandboxed, opaque-origin iframe with network access denied by default. That sandbox is what makes rendering an uploaded page safe, and is why this must never become an innerHTML. Edit still holds the raw markup, and saving re-indexes the visible text the HTML parser strips out -- script contents included in neither. "source" types (.txt) have no richer view, so they open straight in the editor with no toggle. Also drop the Cancel button: the corner X closes the modal and runs the same unsaved-changes guard. Refs unslothai#8840
A canvas page can be laid out far wider than the modal -- a 1600px figure, say -- and it renders in a separate origin, so nothing inside it can be zoomed from here. Scale the frame from its top-left and widen it by the inverse, so zooming out reveals more of the page instead of shrinking a column of it. Same step and ceiling as the PDF viewer; half its floor, because 0.5 does not bring a page that wide fully into view. Read-only HTML gets the control too, which is why the toolbar no longer hangs off the editable flag. Refs unslothai#8840
At 50% the wrapper was sized 200% and scaled by half, landing back at exactly the container's size -- but the container kept overflow-auto, so its scrollbar sat alongside the iframe's own for the same surface. Only zooming in can overflow the box, so scroll it only above 1. Refs unslothai#8840
for more information, see https://pre-commit.ci
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2da1877fd1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Five findings, all confirmed before fixing. P1 -- a save reported success as soon as the PUT returned, but that only starts the re-index. A failed parse or embed then dropped the replacement, left the original in place, and the edit vanished with no error. Wait for the job to reach a terminal status and surface a failure. The waiting loop lives in lib/poll-job.ts with the fetch injected, because rag-api pulls in the auth barrel and an image asset the node:test runner cannot load; 6 tests cover it. P1 -- two concurrent saves of one source could both pass the status check, since it read a row from an already-closed connection. Each replacement then retired the same old row, leaving two versions indexed. Claim the document with a conditional UPDATE so testing and claiming are one atomic statement; the second request 409s. A start that never happens releases the claim, so a source is never stuck reading as indexing. P2 -- a sub-cap file holding a byte that is not valid UTF-8 decoded to U+FFFD and was still offered as editable, so saving rewrote that byte and corrupted content outside the edit. _document_text now reports whether the text is the file verbatim, and only a faithful read is editable. P2 -- .docx extraction skipped the size cap, so a small compressed file could return many megabytes of JSON. Capped in characters, which cannot exceed the byte cap. P2 -- max_length counts characters, so 400k CJK characters passed validation and wrote 1.2 MB, over the cap the next GET enforces; the source went read-only right after being saved. Check the encoded length and 413. Refs unslothai#8840
for more information, see https://pre-commit.ci
Author
Codex review addressed in
|
| Finding | Fix | |
|---|---|---|
| P1 | Save reported success before the re-index finished, so a failed embed silently lost the edit | Wait for the job to reach a terminal status; surface failures and keep the modal open |
| P1 | Two concurrent saves each retired the same old row, leaving two versions indexed | Atomic claim via a conditional UPDATE; the second request 409s, and a failed start releases the claim |
| P2 | A byte that is not valid UTF-8 decoded to U+FFFD, and saving rewrote it |
_document_text reports whether the text is the file verbatim; only a faithful read is editable |
| P2 | .docx extraction skipped the size cap |
Capped in characters, which cannot exceed the byte cap |
| P2 | max_length counts characters, so 400k CJK wrote 1.2 MB over the cap |
Reject on encoded length with 413, before writing or claiming |
Verification
- Backend 63 pass across the affected suites (24 in
test_rag_document_content.py, up from 19) - Frontend 2740 pass (up 6 — the new
poll-jobtests) with the same 21 pre-existing local failures, which are Windows-only path bugs in the test harness and do not occur on CI typecheck0 errors ·i18n:check:strictparity clean ·buildgreen at 58 MB of the 75 MB budget
New tests worth pointing at: a second concurrent save is refused and its text is never indexed; a replacement that never starts leaves the source editable rather than stuck reading as indexing; a latin-1 byte makes a source read-only while leaving the file untouched; and ordinary multibyte UTF-8 (café, naïve, 日本語) stays editable, so the guard does not overreach.
Still stacked on #8850 — please take that one first.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 827e6a474b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Collaborator
|
@Rafael-Silva-Oliveira Please resolve fixed Codex reviews to stop Codex context filling up. |
Author
Will do, still need to check the latest codex reviews, will check tomorrow |
Second Codex round on the source preview modal, all three from the concurrency machinery the first round added. Only the replacement retiring the old row released the edit claim, so a failed parse or embed, a lost lease or a cancellation left the source reading as indexing forever: polled endlessly, and refusing both a retry and a removal. The claim is now handed back on every non-successful exit, from one call in the worker's finally. A delete landing while a replacement was in flight removed the old row and reported success, then the worker published the replacement and its retire of the absent old id was a no-op -- so a source the user had just deleted came back under a new id. The worker now checks both rows in the one transaction that retires the old one and withdraws the replacement when the original is gone, and the DELETE route reads and deletes inside the same lock so the two cannot interleave. A textarea reports every line as LF whatever the file held, so one edited character in a CRLF file rewrote every line ending in it. The backend now names the file's convention and the editor restores it on save; a file that mixes conventions cannot round-trip through a textarea at all, so it previews read-only, as an invalid-UTF-8 file already does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Author
Second Codex review addressed in
|
| Finding | Fix | |
|---|---|---|
| P1 | Only a successful replacement released the edit claim, so a failed re-index left the source stuck indexing forever | Hand the claim back from the worker's finally — one call site covering failure, cancellation and lease loss |
| P1 | A delete during a save was a no-op against the old row, so the deleted source came back as its replacement | The worker checks both rows in the transaction that retires the old one and withdraws the replacement when the original is gone; DELETE reads and deletes under the same lock |
| P2 | A textarea reports LF for every line, so one edited character rewrote every line ending in a CRLF file | The backend names the file's convention and the editor restores it; a file mixing conventions previews read-only |
Verification
- Backend 429 pass across the 21
test_rag_*suites, up 4 on the same baseline (19 local failures unchanged before and after — they aresqlite3/ GGUF-cache gaps in my local env, not on CI) test_rag_document_content.py28 pass, run 11× consecutively for the two new concurrency tests- Frontend 2740 pass with the same 21 pre-existing Windows-only harness path failures, none in RAG files ·
typecheck0 errors ·ruff checkclean
New tests worth pointing at: a failed re-index leaves the source editable and accepts a second save; a delete issued mid-embed through the real route leaves both rows, both uploads and both search terms gone; and CRLF / LF / mixed each land where they should.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2cadae6db5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Third Codex round on the source preview modal. The edit claim is released by the worker on every non-successful exit, but a crash never reaches that finally and the relationship lived only in the dead process's arguments, so startup reconciliation had nothing to work with and left the original 'running' forever. The replacement row now records what it replaces, and reconciliation hands the claim back. Settling the retirement could raise with the old row still present -- a BEGIN IMMEDIATE timing out behind a long writer -- and that path returned "published", so the job completed, the claim release restored the old row, and both the old and the edited copy stayed searchable. Every outcome now leaves exactly one document: a failed settlement withdraws the replacement, chunks and upload included, and reports why. A replacement cannot go through the dedupe branch (it owns `replaces`), so editing one source into another's exact bytes indexed the same content twice and retrieval, which dedupes only by chunk id, returned both. The route refuses that edit rather than silently merging two named sources. On the client, a single delete only invalidated before its DELETE, so a poll starting in the gap could restore the row; it now invalidates after as well, as the bulk path already did. And a reconcile awaiting its DELETEs could land after a project switch with the newest ticket and write the old project's rows into the new one -- shown with the new project's controls, since the panel is reused without a key. A refresh now discards its rows when the scope moved out from under it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Author
Third Codex review addressed in
|
| Finding | Fix | |
|---|---|---|
| P1 | A crash mid-edit lost the replacement relationship with the process, so startup could never release the original's claim | Persist replaces_document_id on the replacement row; reconciliation hands the claim back |
| P1 | A bulk removal's trailing reconcile could land after a project switch and write project A's rows into project B | refresh discards its rows when the scope moved out from under it — one fix covering every awaiting caller |
| P2 | A failed retirement reported success, leaving the old and edited copies both searchable | Withdraw the replacement instead; exactly one document now survives every outcome, and each withdrawal reports its own cause |
| P2 | Editing one source into another's exact bytes indexed the same content twice | The route refuses with 409 rather than silently merging two named sources |
| P2 | A single delete invalidated only before its DELETE, so a poll in the gap could restore the row | Invalidate after it commits too, as the bulk path already did |
Two are answered differently from the letter of the suggestion, with the reasoning on each thread: propagating the settlement exception would have swapped two searchable copies for two searchable copies (chunks are already committed, and retrieval filters by scope not status), and re-entering the dedupe branch would reopen the replaces/dedupe constraint the first round settled.
Verification
- Backend 432 pass across the 21
test_rag_*suites, up 7. Failure set diffed against a clean worktree at2cadae6d: identical, zero new (the 20 local failures aresqlite3/GGUF-cache gaps in my env, not on CI) - Frontend 2740 pass, unchanged, with the same 10 pre-existing Windows-only harness path failures ·
typecheck0 errors ·eslintclean on the changed file ·ruff checkclean
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0bec42547
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Fourth Codex round, all follow-ons from the third. Startup recovery released the original's claim before deciding what to do with the replacement, so a crash landing between "replacement completed" and the retirement published both versions. Recovery now finishes what the worker had all but done: a completed replacement retires the original, and anything else fails and hands the original back. Exactly one survives. The duplicate-content check sat outside the scope lock, so two clients editing two different sources to the same new bytes both saw no twin and then admitted one after the other. It now runs under the same lock start_ingestion admits through, so the loser sees the winner's row. Capping .docx extraction bounded the reply but not the work: the parser had already built the whole document. A .docx declares how much it unpacks to, which costs no decompression to read, so an archive that cannot fit the cap is never handed to the parser, and the rest accumulates to the cap instead of joining a second full copy. On the client, a failed DELETE restored its row without checking the scope, so a delete failing after a project switch put the old project's document into the new one's list -- the same hole the refresh guard closed, by the other door. And the save poll turned five minutes of elapsed time into failure even while the job was visibly advancing, reporting a save as lost whose edit then landed anyway, behind a retry that could only 409. The timeout now bounds silence: any change in stage or progress renews it, so only a worker that stops saying anything trips it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Author
Fourth Codex review addressed in
|
| Finding | Fix | |
|---|---|---|
| P1 | A crash between "replacement completed" and the retirement published both versions: my claim release ran before that branch | Recovery finishes what the worker had all but done — a completed replacement retires the original, anything else hands the original back. Exactly one survives |
| P2 | The duplicate-content check sat outside scope_lock, so two clients editing two different sources to the same bytes both passed it |
Moved under the same lock start_ingestion admits through, so the loser sees the winner's row |
| P2 | Capping .docx extraction bounded the reply but not the work — the parser had already built the whole document |
The archive declares its unpacked size; one that cannot fit the cap never reaches the parser |
| P2 | A failed DELETE restored its row without a scope check — the same hole as the stale refresh, by the other door | remove captures its scope and restores only into it |
| P2 | The save poll turned 5 minutes of elapsed time into failure while the job was visibly advancing | The timeout now bounds silence: any change in stage or progress renews it |
The first is the one I'd flag as genuinely load-bearing — thank you for catching that the release ordering undid the branch below it.
Verification
- Backend 436 pass across the 21
test_rag_*suites, up 4. Failure set diffed against the clean baseline: identical, zero new - Frontend 2742 pass, up 2, with the same 10 pre-existing Windows-only harness path failures ·
typecheck0 errors ·eslintandruff checkclean - The recovery test was checked against a reverted fix to confirm it actually catches the bug, and the concurrent-save test runs both PUTs from a thread pool
One thing noted but not changed, since it is outside this PR: ingestion parses the same .docx at upload without the new unpacked-size guard. Happy to open a follow-up if you want that bound there too.
All 18 threads on this PR are resolved. @mahiatlinux
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 591165df87
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Fifth Codex round, all at the seams of the fourth. The claim was committed in its own transaction before start_ingestion hashed the file and ran embedder discovery, so a crash in that window left the original 'running' with no row recording what it waited for -- exactly what recovery needs to release it. The claim now happens inside the admission transaction that writes that row, so the two are never apart. Recovery published a completed replacement without checking the original still existed, so a source deleted while the crashed job sat unreconciled came back under the replacement's id. The delete wins here now, as it does in the worker. Recovery also deleted the original's row without its file, and nothing sweeps the uploads root, so those bytes were leaked for good. add_chunks committed the replacement's rows before the retirement started its own transaction, and retrieval filters by scope rather than status, so a query landing between the two answered from both versions. The chunks, the completed status and the retirement now commit together. Embedding reported itself once and then went quiet for its whole duration, which is indistinguishable from a dead worker -- so the save dialog's stall timeout could still call a live job failed, and the lease went unrenewed across it too. It now reports once per batch across the span it owns. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
for more information, see https://pre-commit.ci
Author
Fifth Codex review addressed in
|
| Finding | Fix | |
|---|---|---|
| P1 | The claim committed before start_ingestion hashed the file and ran embedder discovery, so a crash there stranded the original with nothing recording what it waited for |
The claim now happens inside the admission transaction that writes replaces_document_id — never observable apart |
| P1 | Recovery published a completed replacement without checking the original still existed, so a deleted source came back under a new id | The delete wins in recovery too, exactly as it does in the worker |
| P2 | add_chunks committed before the retirement's own transaction, and retrieval filters by scope not status, so a query between them answered from both versions |
Chunks, completed status and retirement now commit as one |
| P2 | Embedding reported itself once and went quiet for its whole duration — indistinguishable from a dead worker, and the lease went unrenewed too | One progress update per embedding batch, across the span the stage owns |
| P2 | Recovery deleted the original's row but not its file, and nothing sweeps the uploads root | Path read before the delete, removed after the commit, uploads-root confined |
Verification
- Backend 440 pass across the 21
test_rag_*suites, up 4. Failure set diffed against the clean baseline: identical, zero new - Frontend 2742 pass, unchanged, with the same 10 pre-existing Windows-only harness path failures ·
typecheck,eslint,ruff checkclean - Both new behaviours checked against a reverted fix to confirm the tests actually catch them
- The
_embed_pass/_embed_allsignature change was threaded through the three test stubs that patch them
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6380b717bc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Sixth Codex round, all three from the atomic publish added in the last one. Committing the job separately from the replacement left a crash window in which a completed replacement had no original left -- which is exactly what recovery reads as "the user deleted it", so it would withdraw a replacement that had in fact succeeded and take the source and both files with it. The job's completion now rides in the publish transaction, and the settled replacement drops its replaces pointer there too, so an edit that finished leaves nothing for recovery to find and nothing it could misread. The retired original's file was deleted while the deletion of its row was still uncommitted, so a rollback restored a document whose bytes were already gone -- a source that previews and downloads as nothing. The path comes back to the caller now and is removed only after the commit, as the recovery path already did. On the client, a delete losing to another remover answers 404 and was treated as a failure, restoring a row for a document that no longer exists. On a completed project, with no indexing poll to correct it, that row sits there and 404s on every later action. 404 is the state the delete asked for, so it now reports success. Failures carry their status for that, via a RagRequestError in types/rag so the node runner can reach it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Author
Sixth Codex review addressed in
|
| Finding | Fix | |
|---|---|---|
| P1 | Committing the job separately left a window where a completed replacement had no original — which recovery reads as "the user deleted it", so it would withdraw an edit that had succeeded, losing the source and both files | The job's completion rides in the publish transaction, and the settled replacement clears its replaces pointer there too |
| P2 | The retired original's file was deleted while its row deletion was uncommitted, so a rollback restored a document whose bytes were gone | The path returns to the caller and is removed only after the commit lands |
| P2 | A delete losing to another remover answers 404, which was treated as failure and restored a row for a document that no longer exists | 404 is the state the delete asked for, so it reports success |
Verification
- Backend 442 pass across the 21
test_rag_*suites, up 2. Failure set diffed against the clean baseline: identical, zero new - Frontend 2746 pass, up 4 (
tests/rag-already-gone.test.ts), same 10 pre-existing Windows-only harness failures ·typecheck,eslinton changed files,ruff checkall clean - Rebased onto
6380b717b
One note on test hygiene rather than product code: running test_rag_document_content.py alongside two other suites intermittently fails with sqlite3.OperationalError: no such table: ingestion_jobs in fixture setup (~1 in 3 locally). I checked this against a clean worktree at 6380b717b and it flakes there identically without any of my changes, so it is a pre-existing test-isolation race in the shared rag_home fixture, not something this PR introduced. Happy to open a separate issue for it.
for more information, see https://pre-commit.ci
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Quality-of-life work on the Unsloth Studio project Sources panel. Second of a stacked pair.
Refs #8840
Important
Stacked on #8850 — review only the last 5 commits.
This branch is cut from
feat/project-sources-search-sort(PR #8850). GitHub cannot target a base branch that exists only in my fork, so this PR has to point atmainand therefore replays #8850's commits in its diff.The work to review here is the 5 commits from
955e2econward:Everything before those belongs to #8850. Please merge that one first; this becomes a clean 8-file diff once it lands, and I'll rebase if it needs changes.
The problem
A source chip is inert. You can remove it, but you cannot see what is inside it. The only document viewer in Studio is
document-preview-sheet.tsx, a right-sideSheetwired exclusively to chat citations — and the right side of the project page is already docked by Run settings, so a second right-side panel would fight it.What this does
Clicking a source chip opens a centered modal that displays the file and lets plain-text sources be edited in place. Escape, a backdrop click and the corner X all close it, confirming first when there are unsaved edits.
.md.markdown.html.htm.txt.pdf.docxRetrieval safety
The constraint carried over from #8850: these are QoL changes, and no document may become invisible to the model. May also be worth mentioning #8854, as the tool seems to throw an error when asking a model to list the thread source files it has access to.
Saving writes the edit to a new file in the managed uploads root and re-ingests it as a replacement, carrying
scope,project_id,kb_idandthread_idover unchanged — so the replacement lands in the same scope across all four denormalized copies (documents.scope,chunks.scope,chunks_fts.scope, thechunks_vecpartition key).The document it replaces is retired only once the re-index completes, via the existing
replacesmechanism in_replace_old_document. A failed parse or embed therefore leaves the original searchable instead of destroying it. That is exposed as one new keyword argument onstart_ingestion, guarded with aValueErroragainst being combined withdedupe— withdedupe=True(every existing caller) behaviour is byte-identical.Nothing outside the uploads root is ever written
Linked-folder sources are display-only. Their
stored_pathis a snapshot that folder sync recreates from the user's real file, so an edit here would be silently wiped by the next sync — and the original in the user's own folder is never opened for writing either way.PUTanswers 409 for them, matching whatDELETEalready does..pdfand.docxare display-only for a different reason: neither survives being retyped as plain text.Which of these applies is decided by the backend and sent as
editableplus a reason, so the client holds no list of file extensions and the two cannot disagree about what may be saved.New endpoints
GET /api/rag/documents/{id}/content— text, how to preview it, and whether it may be editedPUT /api/rag/documents/{id}/content— save and re-indexBoth confine to
_is_managed_preview_pathand go through_require_document_owner, the same guards as the existing signed-file route. Text over 1 MB previews truncated and refuses to save, so a save can never silently delete the tail of a file.Reuse rather than new code
ArtifactHtmlFrame— the chat artifact canvas renders HTML in a sandboxed, opaque-origin iframe with network access denied by default. That sandbox is what makes rendering an uploaded page safe; it is never dropped into the app's own origin.MarkdownPreview— already used by the release-notes panelPdfPreview— exported from the citation sheet rather than duplicatedDocumentStatusChip.onOpen— the prop was already there, unwiredDialog— Radix routes Escape, backdrop and X through oneonOpenChange, so the unsaved-changes guard is one branch covering all threeNo new dependencies.
Tests
19 new backend tests in
test_rag_document_content.py. The ones that matter:test_saving_an_edit_reindexes_into_the_same_scope— searches for the new text, asserts the old is gone, checks the replacement'sscopeandproject_idtest_a_failed_reindex_leaves_the_original_searchable— makes the embedder raise, then asserts the original survives and still retrievestest_a_linked_folder_source_cannot_be_edited— asserts the linked file's bytes and mtime are untouched after a rejected savetest_saving_writes_only_inside_the_uploads_roottest_editing_html_saves_the_markup_and_indexes_its_visible_text— markup round-trips; script contents are not indexedtest_emptying_a_source_keeps_it_as_an_empty_documentNo new frontend unit tests: the suite runs
node --experimental-strip-types --test, which cannot load.tsx, and every decidable rule here deliberately lives on the server where pytest can reach it. Adding a frontend module purely to have something to assert would duplicate the extension list and create a second source of truth.Verification
typecheck0 errors ·i18n:check:strictparity clean ·buildgreen at 58 MB of the 75 MB budgetKnown limitations
Screenshots: