feat: nested lorebook folders - #2353
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Ship sub-folders for lorebooks (the nesting half of Pasta-Devs#2127; the entry-duplication half landed in Pasta-Devs#2157). Folders can now nest into a tree. - scanner: activation gating resolves "effective disabled" by walking the parentFolderId chain (cycle-guarded), so a disabled folder OR any disabled ancestor gates its entries -- nesting can't silently leak gated content. - lib/lorebook-folder-tree: canReparentFolder (same-lorebook, no self-parent, no cycles) is the write-time guard; buildFolderForest groups folders into a render-ready tree (a dangling/missing parent falls back to root, so deleting a parent promotes its children). - storage: reorderLorebookFolders optionally sets parentFolderId, so nest / un-nest + per-parent order happen in one pass; reorder/reparent refreshes activation. - editor UI: recursive folder-tree render (depth indent, collapse hides the subtree); a per-folder parent dropdown filtered by canReparentFolder; folder drag-to-nest via the header (top = reorder before, middle = nest, bottom = reorder after) and via a folder's body (incl. the empty placeholder, the left-indent margin targeting shallower ancestors); drag into the root area to un-nest. Validation is client-side because the lorebook-folder collection is stored generically with no server-side field validation (matching every other lorebook edit); the scanner independently guards cycles + disabled ancestors at read time, so malformed import/direct-write data can't hang generation. Closes Pasta-Devs#2127.
980f904 to
15687d7
Compare
|
@cha1latte ready for review btw |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/features/catalog/lorebooks/components/editor/use-lorebook-editor-drag-drop.ts`:
- Around line 243-285: The handlers handleFolderDragOverRow,
handleFolderBodyNestDragOver, and handleRootFolderDragOver currently set
folderRootDropActive and folderDropTarget but never clear stale state when the
cursor moves to a different target or an invalid/self target; update each
handler to proactively clear the other state when the current hover becomes
invalid or is a different target (e.g., call setFolderRootDropActive(false)
and/or setFolderDropTarget(null) as appropriate before returning or when
switching zones), and ensure when an invalid/self target is detected you clear
both folderRootDropActive and folderDropTarget so no previous highlight remains.
In `@src/features/catalog/lorebooks/components/entries/LorebookFolderRow.tsx`:
- Around line 270-280: The parent dropdown is hidden when folders.length > 1 is
false, preventing clearing a stale parentFolderId; update the render guard
around CompactSelect so it also shows when a dangling parent exists (e.g.,
change the condition from folders.length > 1 to (folders.length > 1 ||
localParentId != null && localParentId !== "")), keeping the existing props
(value={localParentId ?? ""}, onChange={handleParentChange},
options={parentOptions}) and the surrounding span/onClick to preserve behavior
so users can select “(no parent)” to clear the stale parent.
In `@src/features/catalog/lorebooks/lib/lorebook-folder-tree.ts`:
- Around line 27-37: Ensure the source folder exists before allowing a
move-to-root: instead of returning { ok: true } immediately when newParentId ===
null, first build byId and lookup folder using folderId (the folder variable),
and return { ok: false, reason: "Folder not found." } if it's missing; only
after confirming folder exists should you allow newParentId === null to return
success. Also preserve the existing checks that create byId from folders and
validate newParent (newParentId, newParent.lorebookId) when applicable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c8e36630-a0c8-423e-94ac-4d803e64efe1
📒 Files selected for processing (9)
src/engine/contracts/schemas/lorebook.schema.tssrc/engine/contracts/types/lorebook.tssrc/engine/generation/active-lorebook-scanner.tssrc/features/catalog/lorebooks/components/editor/LorebookEditor.tsxsrc/features/catalog/lorebooks/components/editor/LorebookEntriesTab.tsxsrc/features/catalog/lorebooks/components/editor/use-lorebook-editor-drag-drop.tssrc/features/catalog/lorebooks/components/entries/LorebookFolderRow.tsxsrc/features/catalog/lorebooks/hooks/use-lorebooks.tssrc/features/catalog/lorebooks/lib/lorebook-folder-tree.ts
|
Additional review notes from the nested-folder pass:
|
- Select-all-visible now excludes entries under a collapsed ancestor, not just directly-collapsed folders (collectHiddenFolderIds walks the subtree), so it can't select entries the user can't see. - Deleting a folder reparents its direct child folders to root (clears the stale parentFolderId) alongside the existing entry reparenting, so later reorder/reparent logic never reads a dangling parent. Adds a regression test. - Folder rows under a disabled ancestor now show muted icon/toggle, a "parent off" cue, and an accurate tooltip, matching the scanner's activation gating.
✅ Bunny Review CompletedTip Review posted. The specimen has left the observation table.
|
🐰 Bunny ReviewBunny Merge Signal: Ready With NotesWarning READY WITH NOTES
Note Mode: 🧭 Specimen Summary
🔎 Isolated DefectsTip No actionable defects isolated. ✅ Resolved Since Last Review
🧹 Nitpicks
✅ Control Checks
🧪 Observations
🧰 CI Status
|
- Add validate_lorebook_folder_for_create/_patch: reject a parentFolderId in another lorebook, a self-parent, a missing parent, or a cycle, at the generic create/update path — not just the editor's canReparentFolder. Imports, remote callers, and command writes can no longer persist a malformed tree. Adds a regression test (cycle + cross-lorebook rejected, root move allowed). - buildFolderForest now promotes folders unreachable from any root (trapped in a malformed parent cycle) up to the roots, so they stay visible and repairable instead of vanishing from the editor.
- validate_lorebook_folder_for_patch now checks the effective post-patch (lorebookId, parentFolderId) pair, so a lorebookId-only patch can't leave a parent that has become cross-lorebook. Test extended to cover the evasion. - buildFolderForest severs the cyclic child edge when promoting an unreachable node to the roots, so a rescued cycle member appears once (as a root) instead of also under its parent — a clean forest, not a hall of mirrors.
A folder's lorebook is an ownership key the app never changes (entry transfers move entries, never folders). Allowing a lorebookId patch stranded a root folder's children — or a child's parent — across books. Reject any change to a different lorebook outright instead of validating ever-more-elaborate cross-book move shapes. Test covers both the child and root cases.
Linked issue
Closes #2127 — this is the nested-folders half. (The intra-lorebook entry-duplication half shipped earlier in #2157.)
Why this change
parentFolderIdexisted but was reserved/always-null. This ships nesting end-to-end: organize a lorebook's folders into a tree, set parentage by dropdown or drag, and (the load-bearing part) extend the folder-disabled activation gate to disabled ancestors, so nesting can never silently leak gated content into the prompt.What changed
active-lorebook-scanner.ts) — activation now resolves "effective disabled" by walking each entry's folderparentFolderIdchain (with a cycle guard): a folder OR any of its ancestors being disabled gates the entry, regardless of the entry's own flag.lib/lorebook-folder-tree.ts) —canReparentFolderis the single write-time guard (same lorebook, no self-parent, no cycles, with reasons);buildFolderForestgroups folders into{ roots, childrenByParent }for rendering. A dangling/missing parent falls back to root, which auto-promotes a deleted folder's children to the top level.hooks/use-lorebooks.ts) —reorderLorebookFolderscan now also setparentFolderId, so nest/un-nest + per-parent ordering happen in one pass; reorder/reparent invalidates the activation cache.LorebookEntriesTab.tsx); a per-folder parent dropdown filtered to valid parents (LorebookFolderRow.tsx); drag-to-nest via the header (top = reorder before, middle = nest, bottom = reorder after) and via a folder's body including the empty placeholder, with the left-indent margin targeting shallower ancestors; drag to the root area to un-nest (use-lorebook-editor-drag-drop.ts,LorebookEditor.tsx).Refactor impact
Primary owner: Catalog / Lorebooks editor (frontend), plus the activation scanner (engine generation)
Impact areas reviewed:
src-tauri/src/lib.rschange.Boundary notes:
canReparentFolderas the one write-time guard and the existing generic storage gateway; no new storage collection, command, or cross-boundary path.Pressure points touched:
ModeSurface/GameSurface/shared mode UI, nosrc-tauri/src/lib.rs, no imports, no version-bearing files.Validation
pnpm typecheck,pnpm build,pnpm check:architecture,pnpm check:docs, or fullpnpm checkwhen warranted)pnpm checkpasses before PR push/handoffManual verification notes
pnpm typecheckpluscheck:architecture/check:frontend/check:rust/check:discovery/check:docs/check:launcher-safety/check:unusedall pass locally.check:line-endingsfails only on pre-existing CRLF in files this PR doesn't touch (DESIGN.md,custom_components/*.py); their committed versions are LF, so CI is unaffected.Feature Discoverability
src/features/shell/discovery/because this PR adds or materially changes a user-discoverable feature, workflow, setting, mode, panel, import path, agent, media capability, or advanced tool.Reason:
lorebooksdiscovery entry opens the panel). The registry is surface-level, not per-action, so no new entry is required andcheck:discoverypasses unchanged (same call as the entry-duplication PR feat: duplicate lorebook entries #2157).Docs and release impact
README.mdCONTRIBUTING.mddocs/developer/AGENTS.mdProof
Core claim:
Verification run:
Evidence:
*.test.ts, and the template asks not to submit them as proof) cover the scanner's ancestor chain-walk and thecanReparentFolder/buildFolderForestedge cases (self, cross-lorebook, direct + deep cycles, dangling-parent→root). Happy to fold them in if the team wants them tracked.Design notes (recorded for review):
canReparentFolderruns client-side because the lorebook-folder collection is stored generically with no server-side field validation — matching how every other lorebook edit is validated. The activation scanner independently resolves disabled ancestors and guards against cycles at read time, so a malformed parent introduced by an import or a direct write can never hang generation or leak gated content. If the lorebook layer later moves into Rust, the same three checks (same lorebook, no self-parent, no cycle) port directly.buildFolderForest) rather than cascade-deleting — consistent with how an entry whose folder is gone falls back to root.Manual verification requested:
UI evidence
Summary by CodeRabbit
New Features
Bug Fixes
Documentation