Merged
Conversation
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
editor | 108e96f | Commit Preview URL Branch Preview URL |
Mar 23 2026, 07:33 PM |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors page management from a flat, index-based list to an ID-addressable hierarchical tree. Adds Page.id/children, a page-tree library, tree-aware usePages hook, new TreeList/TreeItem UI with drag-and-drop, updates PagesMenu/Toolbar/App to ID-based APIs, tweaks Button/ContextMenu types, and adds tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant TreeItem as TreeItem
participant PagesMenu as PagesMenu
participant usePages as usePages Hook
participant PageTree as PageTree Lib
User->>TreeItem: drag item over target
TreeItem->>TreeItem: compute midpoint, show indicator
User->>TreeItem: drop
TreeItem->>PagesMenu: onDrop(itemId, position)
PagesMenu->>usePages: moveItem(itemId, "up"/"down"/"in"/"out")
usePages->>PageTree: call moveItemBefore/After/Into/Out(...)
PageTree->>PageTree: structuredClone, remove source, insert destination
PageTree-->>usePages: return new tree
usePages->>usePages: setPages(newTree), persist currentPageId
usePages-->>PagesMenu: updated pages
PagesMenu->>TreeItem: re-render with updated positions
sequenceDiagram
participant User as User
participant PagesMenu as PagesMenu
participant usePages as usePages Hook
participant App as App
participant Toolbar as Toolbar
User->>PagesMenu: click page item
PagesMenu->>usePages: setCurrentPageId(pageId)
usePages->>usePages: update currentPageId, persist
usePages-->>App: render with currentPageId
App->>Toolbar: pass currentPageId and renameItem
User->>Toolbar: rename page
Toolbar->>usePages: renameItem(currentPageId, newName)
usePages->>PageTree: update node name
usePages-->>App: render updated page
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai hello review |
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (2)
tests/usePageTree.lib.test.ts (1)
141-171: Add a self/descendant drop regression test.A bad
moveItemIntoimplementation can create a cycle by dropping a node into itself or one of its descendants, andsrc/components/TreeList.tsxwould then recurse forever. This is the highest-risk folder DnD case, but it is not covered here yet.🧪 Suggested test
describe("moveItemInto", () => { it("should move item into another item's children", () => { const tree = [ { id: "a", name: "A", content: "" }, { id: "b", name: "B", content: "", children: [{ id: "c", name: "C", content: "" }], }, { id: "d", name: "D", content: "" }, ]; const result = moveItemInto(structuredClone(tree), "d", "b"); expect(result.map((i) => i.id)).toEqual(["a", "b"]); const b = result.find((i) => i.id === "b") as Page; expect(b.children?.map((i) => i.id)).toEqual(["c", "d"]); }); it("should move into empty children", () => { const tree: Page[] = [ { id: "a", name: "A", content: "" }, { id: "b", name: "B", content: "", children: [] }, ]; const result = moveItemInto(structuredClone(tree), "a", "b"); expect(result.map((i) => i.id)).toEqual(["b"]); const b = result.find((i) => i.id === "b") as Page; expect(b.children?.map((i) => i.id)).toEqual(["a"]); }); + + it("should not move an item into its own descendant", () => { + const tree: Page[] = [ + { + id: "b", + name: "B", + content: "", + children: [{ id: "c", name: "C", content: "" }], + }, + ]; + + const result = moveItemInto(structuredClone(tree), "b", "c"); + expect(result).toEqual(tree); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/usePageTree.lib.test.ts` around lines 141 - 171, The test suite is missing a regression test ensuring moveItemInto prevents dropping a node into itself or one of its descendants (which causes TreeList.tsx to recurse forever); add a new test in tests/usePageTree.lib.test.ts that builds a tree with nested children, attempts moveItemInto(structuredClone(tree), "x", "x") and also moveItemInto(..., "ancestor", "descendant") and asserts the tree is unchanged (or that moveItemInto returns an error/false if that's the intended behavior). Locate the moveItemInto usage in the file and ensure the test references the same function names and Page shape so we catch regressions that would allow self/descendant moves.src/components/TreeList.tsx (1)
31-33: Only make the root tree scrollable.
TreeListis recursive, so Line 33 gives every nested subtree the same cappedoverflow-y-autocontainer. Deep folders will end up with stacked scrollbars and clipped drag targets. This is much easier to use if the scroll wrapper only exists at the top level.♻️ Suggested direction
type TreeListProps = { pages: Page[]; currentPageId: string; openFolders: Set<string>; dragItemId: string | null; dragOverItemId: string | null; dragOverPosition: "before" | "after" | null; + nested?: boolean; onSelectPage: (pageId: string) => void; onToggleFolder: (folderId: string) => void; onContextMenu: (e: MouseEvent, item: Page) => void; onDragStart: (e: DragEvent, item: Page) => void; @@ const TreeList = (props: TreeListProps): JSX.Element => { return ( - <div class="max-h-[min(calc(100vh-150px),calc(38px*8))] overflow-y-auto"> + <div + class={ + props.nested + ? "" + : "max-h-[min(calc(100vh-150px),calc(38px*8))] overflow-y-auto" + } + > @@ <TreeList + nested pages={item.children ?? []} currentPageId={props.currentPageId} openFolders={props.openFolders} dragItemId={props.dragItemId}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/TreeList.tsx` around lines 31 - 33, The root cause is that TreeList renders the same scroll wrapper for every recursive call; modify TreeList (component TreeList, type TreeListProps) to accept an indicator (e.g., add an optional prop isRoot?: boolean or depth: number default 0) and only render the capped scroll container when isRoot === true or depth === 0; in recursive renders pass isRoot={false} or depth={depth+1} so nested subtrees render without the "max-h[...] overflow-y-auto" wrapper. Ensure you update the JSX wrapper conditionally (use className only on the root case) and propagate the new prop in the places where TreeList calls itself.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/PagesMenu.tsx`:
- Around line 231-235: The canMoveIn predicate in PagesMenu.tsx is gating "Move
Into Page" only when the previous sibling already has children; update the
canMoveIn function (and its use of contextMenu() / getPrevSibling) to allow the
action whenever a previous sibling exists by returning prev !== null (i.e.,
remove the (prev.children?.length ?? 0) > 0 check) so targetFolder.children can
be created on demand as done in usePages.ts and the UI can convert a plain page
into its first folder.
In `@src/components/TreeItem.tsx`:
- Around line 99-117: The Button currently invokes both onMouseDown and onClick
(Button's onClick set via hasChildren() ? props.onToggle : props.onSelect),
causing double toggles and preventing selection for items with children; remove
the onClick prop and consolidate the single-click behavior inside onMouseDown:
in the Button component remove the onClick attribute and update the onMouseDown
handler to call e.preventDefault() and then call exactly one callback (if
hasChildren() call props.onToggle(), else call props.onSelect()), ensuring pages
with children can still call props.onSelect when appropriate by adding any
keyboard/modifier handling there.
In `@src/components/ui/Button.tsx`:
- Around line 21-24: The ButtonProps<TVariant> currently allows both label and
children optional while the component renders label ?? children, causing
ambiguous/empty renders; restore the XOR contract by changing ButtonProps to a
union that requires exactly one of label or children (e.g., { label: string;
children?: never } | { children: JSX.Element; label?: never } combined with
ButtonAttributes & ButtonVariants<TVariant>), or alternatively update the Button
component render path (in the Button function that uses label ?? children) to
explicitly render both (label followed by children) or throw/console.error when
neither is provided; reference ButtonProps and the Button render using label ??
children when making the change so TypeScript enforces the intended contract.
In `@src/hooks/usePages.ts`:
- Around line 251-271: The current "out" branch in usePages.ts is reinserting
using parentItems[0] (a former sibling) which mis-anchors the moved node;
replace the manual remove+findAndInsert logic with the existing moveItemOut
helper from src/lib/page-tree.ts (use moveItemOut to compute the new pages tree
from the current pages() and itemId), then setPages(...) with that result,
ensuring you still preserve any necessary cloning semantics (e.g.,
structuredClone of pages() if moveItemOut mutates) and remove the
removeFromTree/findAndInsert block; reference symbols: moveItemOut,
removeFromTree, pages(), setPages, itemId, parentItems.
- Around line 175-195: The deleteItem logic in deleteItem currently only checks
equality of currentPageId() === itemId and can leave currentPageId pointing at a
deleted descendant; update deleteItem to detect if the current selection is
inside the subtree being removed by using the existing collectIds traversal (or
a helper like collectIds) to build a list of surviving pages (pages() excluding
the entire subtree rooted at itemId) and then: if currentPageId() is not present
in that surviving list, call setCurrentPageId to a valid fallback (e.g., the
first surviving page id or null/undefined when no pages remain); keep the
existing call to removeFromTree(itemId) but ensure the selection update runs
before or based on the computed surviving list so selection never references a
deleted id (reference functions/vars: deleteItem, currentPageId, collectIds,
pages, setCurrentPageId, removeFromTree).
- Around line 201-205: findPageParent(pages(), itemId) returns null for
root-level nodes, but the current early return makes move actions no-op for
top-level pages; change the logic in usePages.ts so that when parentItems is
null you use pages() as the sibling container for "up", "down", and "in" moves
while still treating "out" as requiring a real parent. Locate the code paths
that call findPageParent and findPageIndex (referencing parentItems, pages(),
itemId, currentIndex, and findPageIndex) and: if parentItems === null and the
requested action is not "out", set parentItems = pages(); otherwise keep the
early return for "out" so behavior is unchanged.
- Around line 117-146: When rehydrating pages in onMount, ensure currentPageId
is set and persisted reactively: after parsing storedPages (and calling
setPages), if storedCurrentPage is missing or doesn't exist in the parsedPages,
setCurrentPageId to the first valid page id (parsedPages[0].id or
DEFAULT_PAGE.id fallback); add a createEffect that writes currentPageId() to
localStorage whenever currentPageId() changes (similar to the existing pages
effect) so selection changes and delete fallbacks are persisted and cross-tab
"storage" events stay in sync; keep the existing storage listener but use the
validated id when applying setCurrentPageId.
- Around line 275-301: updateParentSiblings currently finds oldSiblings[0] and
mistakenly assigns its children (it.children = newSiblings) which overwrites a
folder's children instead of replacing the parent container's sibling entries;
change the logic in updateParentSiblings/update so when you match oldFirst (the
node at items[i]) you replace entries in the current items array at that index
with newSiblings (e.g., write newSiblings into items starting at i, and
splice/remove any leftover items if lengths differ) rather than setting
it.children; update the loop that handles the non-folder case (the inner
for-loop that assigns items[j] = sibling) to write into items[i + j] (and adjust
length with splice) so the parent container is updated correctly—refer to
updateParentSiblings, update, pages(), oldSiblings, oldFirst, and it in the
diff.
In `@src/lib/page-tree.ts`:
- Around line 68-98: Both moveItemBefore and moveItemAfter remove the node (via
removeItemFromTree) before verifying the insert target, which can drop the
subtree when the target is invalid or inside the moved subtree; change the flow
to first locate and validate the insertion site (use findParentOf and
findIndexInParent) and ensure the target is neither the same node nor a
descendant of itemId, and only then call removeItemFromTree and splice the
removed item into targetParent; use the existing symbols moveItemBefore,
moveItemAfter, removeItemFromTree, findParentOf, and findIndexInParent to
implement these checks so removal happens only after confirming a valid
insertion point.
- Around line 109-116: The bug is that findItemInTree(newItems, intoItemId) can
return a target that is inside the branch being moved, then
removeItemFromTree(newItems, itemId) detaches that branch and the stale target
reference corrupts the tree; fix by removing the item before locating the target
or by guarding against moving into its own descendant: call
removeItemFromTree(newItems, itemId) first to get the detached item and the
updated tree, then call findItemInTree(updatedTree, intoItemId) (or explicitly
check that intoItemId is not equal to itemId or a descendant of the removed
item) and only then push the removed item into target.children.
---
Nitpick comments:
In `@src/components/TreeList.tsx`:
- Around line 31-33: The root cause is that TreeList renders the same scroll
wrapper for every recursive call; modify TreeList (component TreeList, type
TreeListProps) to accept an indicator (e.g., add an optional prop isRoot?:
boolean or depth: number default 0) and only render the capped scroll container
when isRoot === true or depth === 0; in recursive renders pass isRoot={false} or
depth={depth+1} so nested subtrees render without the "max-h[...]
overflow-y-auto" wrapper. Ensure you update the JSX wrapper conditionally (use
className only on the root case) and propagate the new prop in the places where
TreeList calls itself.
In `@tests/usePageTree.lib.test.ts`:
- Around line 141-171: The test suite is missing a regression test ensuring
moveItemInto prevents dropping a node into itself or one of its descendants
(which causes TreeList.tsx to recurse forever); add a new test in
tests/usePageTree.lib.test.ts that builds a tree with nested children, attempts
moveItemInto(structuredClone(tree), "x", "x") and also moveItemInto(...,
"ancestor", "descendant") and asserts the tree is unchanged (or that
moveItemInto returns an error/false if that's the intended behavior). Locate the
moveItemInto usage in the file and ensure the test references the same function
names and Page shape so we catch regressions that would allow self/descendant
moves.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed2abd2c-2cbb-4301-a55c-06820d2be4c4
📒 Files selected for processing (12)
biome.jsoncsrc/components/App.tsxsrc/components/PagesMenu.tsxsrc/components/Toolbar.tsxsrc/components/TreeItem.tsxsrc/components/TreeList.tsxsrc/components/ui/Button.tsxsrc/hooks/usePages.tssrc/lib/get-matches.tssrc/lib/page-tree.tssrc/types/index.tstests/usePageTree.lib.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/page-tree.lib.test.ts (2)
118-139: Missing cross-parent test case.
moveItemBeforeincludes a test for moving items between different parent levels (lines 99-115), butmoveItemAfterlacks an equivalent. Consider adding a similar test to ensure cross-parent moves work correctly when inserting after a target in a different branch.♻️ Suggested test addition
it("should not move if item doesn't exist", () => { const tree = [ { id: "a", name: "A", content: "" }, { id: "b", name: "B", content: "" }, ]; const result = moveItemAfter(structuredClone(tree), "xyz", "a"); expect(result.map((i) => i.id)).toEqual(["a", "b"]); }); + + it("should move between different parents", () => { + const tree = [ + { id: "a", name: "A", content: "" }, + { + id: "b", + name: "B", + content: "", + children: [{ id: "c", name: "C", content: "" }], + }, + ]; + + const result = moveItemAfter(structuredClone(tree), "c", "a"); + expect(result[0]?.id).toBe("a"); + expect(result[1]?.id).toBe("c"); + const b = result.find((i) => i.id === "b") as Page; + expect(b.children?.length).toBe(0); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/page-tree.lib.test.ts` around lines 118 - 139, The test suite for moveItemAfter is missing a cross-parent move case; add a test analogous to the existing moveItemBefore cross-parent test that constructs a nested tree (using the same node shapes used elsewhere), calls moveItemAfter(structuredClone(tree), "<movedId>", "<targetId>") where the moved item and target are in different parent branches, and asserts the resulting tree structure and id ordering (and parent/children membership) match the expected post-move state; reference moveItemAfter and the existing moveItemBefore test as the template for setup, invocation, and expectations.
65-69: Consider removing@ts-expect-errorwith proper typing.The suppression exists because
tree[1].childrencould beundefinedby inference whileparentisPage[] | null. Consider adding explicit type annotation to the fixture or usingtoBewith the non-null-asserted reference.♻️ Suggested fix
it("should return children array for nested item", () => { const parent = findParentOf(tree, "c"); - // `@ts-expect-error` - expect(parent).toEqual(tree[1].children); + expect(parent).toBe(tree[1].children!); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/page-tree.lib.test.ts` around lines 65 - 69, The test currently silences a TypeScript error with `@ts-expect-error`; remove that and fix typing by ensuring the fixture and assertion types align: either annotate the test fixture variable tree with the concrete type that guarantees children exist (e.g., give tree the Page[] type where children is always Page[]), or keep tree's current type and change the assertion to explicitly handle non-null (e.g., type-assert or use the non-null assertion on tree[1].children when comparing) so findParentOf, parent, and tree[1].children share a compatible Page[] | null/undefined contract; update the symbols findParentOf, parent, tree, and tree[1].children accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/page-tree.lib.test.ts`:
- Around line 118-139: The test suite for moveItemAfter is missing a
cross-parent move case; add a test analogous to the existing moveItemBefore
cross-parent test that constructs a nested tree (using the same node shapes used
elsewhere), calls moveItemAfter(structuredClone(tree), "<movedId>",
"<targetId>") where the moved item and target are in different parent branches,
and asserts the resulting tree structure and id ordering (and parent/children
membership) match the expected post-move state; reference moveItemAfter and the
existing moveItemBefore test as the template for setup, invocation, and
expectations.
- Around line 65-69: The test currently silences a TypeScript error with
`@ts-expect-error`; remove that and fix typing by ensuring the fixture and
assertion types align: either annotate the test fixture variable tree with the
concrete type that guarantees children exist (e.g., give tree the Page[] type
where children is always Page[]), or keep tree's current type and change the
assertion to explicitly handle non-null (e.g., type-assert or use the non-null
assertion on tree[1].children when comparing) so findParentOf, parent, and
tree[1].children share a compatible Page[] | null/undefined contract; update the
symbols findParentOf, parent, tree, and tree[1].children accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dd5c6bc7-2a9d-4e76-9aad-008de4ab073f
📒 Files selected for processing (1)
tests/page-tree.lib.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/hooks/usePages.ts (1)
206-237:⚠️ Potential issue | 🔴 CriticalSubtree deletion still miscalculates surviving pages.
Two issues remain in the
deleteItemlogic:
Line 218: The condition
isDescendantOrSelf(pages())checks whetheritemIdexists anywhere in the tree—not whethercurrentIdis a descendant ofitemId. This makes the guard condition effectively always true for validitemIdvalues.Lines 220-230:
collectSurvivingrecurses into the children of the deleted item and incorrectly counts them as survivors. When deleting a folder, its descendants are still added tosurvivingItems.🐛 Proposed fix
const deleteItem = (itemId: string) => { const currentId = currentPageId(); - const isDescendantOrSelf = (items: Page[]): boolean => { - for (const item of items) { - if (item.id === itemId) return true; - if (item.children && isDescendantOrSelf(item.children)) { - return true; - } - } - return false; - }; - - if (currentId === itemId || isDescendantOrSelf(pages())) { - const survivingItems: Page[] = []; - const collectSurviving = (items: Page[]) => { - for (const item of items) { - if (item.id !== itemId) { - survivingItems.push(item); - } - if (item.children) { - collectSurviving(item.children); - } + + // Collect all IDs in the subtree being deleted + const deletedIds = new Set<string>(); + const collectDeletedIds = (items: Page[], targetId: string): boolean => { + for (const item of items) { + if (item.id === targetId) { + const addAll = (nodes: Page[]) => { + for (const n of nodes) { + deletedIds.add(n.id); + if (n.children) addAll(n.children); + } + }; + addAll([item]); + return true; } - }; - collectSurviving(pages()); - if (survivingItems.length > 0 && survivingItems[0]) { - setCurrentPageId(survivingItems[0].id); + if (item.children && collectDeletedIds(item.children, targetId)) { + return true; + } + } + return false; + }; + collectDeletedIds(pages(), itemId); + + // If current selection will be deleted, pick a new one + if (deletedIds.has(currentId)) { + const findFirstSurvivor = (items: Page[]): string | null => { + for (const item of items) { + if (!deletedIds.has(item.id)) return item.id; + if (item.children) { + const found = findFirstSurvivor(item.children); + if (found) return found; + } + } + return null; + }; + const survivor = findFirstSurvivor(pages()); + if (survivor) { + setCurrentPageId(survivor); } } removeFromTree(itemId); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/usePages.ts` around lines 206 - 237, The current deleteItem logic mis-evaluates the guard and collects deleted descendants as survivors; change isDescendantOrSelf to check whether currentPageId() is a descendant of the node-to-delete (i.e., implement isDescendant(currentId, itemId, pages()) that walks the tree and returns true only if currentId is found within the subtree rooted at itemId), and update collectSurviving so it does not recurse into the subtree being deleted (only recurse into item.children when item.id !== itemId) so that removed descendants are not added to survivingItems; keep references to currentPageId, deleteItem, isDescendantOrSelf (replace/rename to isDescendant), collectSurviving, pages(), setCurrentPageId, and removeFromTree when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/hooks/usePages.ts`:
- Around line 206-237: The current deleteItem logic mis-evaluates the guard and
collects deleted descendants as survivors; change isDescendantOrSelf to check
whether currentPageId() is a descendant of the node-to-delete (i.e., implement
isDescendant(currentId, itemId, pages()) that walks the tree and returns true
only if currentId is found within the subtree rooted at itemId), and update
collectSurviving so it does not recurse into the subtree being deleted (only
recurse into item.children when item.id !== itemId) so that removed descendants
are not added to survivingItems; keep references to currentPageId, deleteItem,
isDescendantOrSelf (replace/rename to isDescendant), collectSurviving, pages(),
setCurrentPageId, and removeFromTree when making these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25218049-dcb7-43e9-b40e-9cf304f4ae1d
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
biome.jsoncpackage.jsonsrc/components/PagesMenu.tsxsrc/components/TreeItem.tsxsrc/components/TreeList.tsxsrc/components/ui/Button.tsxsrc/hooks/usePages.tssrc/lib/page-tree.tstests/page-tree.lib.test.ts
💤 Files with no reviewable changes (1)
- biome.jsonc
✅ Files skipped from review due to trivial changes (3)
- package.json
- tests/page-tree.lib.test.ts
- src/components/TreeItem.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- src/components/ui/Button.tsx
- src/lib/page-tree.ts
- src/components/TreeList.tsx
- src/components/PagesMenu.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/PagesMenu.tsx`:
- Around line 41-42: The dragleave handlers are not keyed to the specific zone
so clearing highlights can remove the wrong active folder; update the callbacks
onDragLeave and onDragLeaveNestable (and the corresponding calls around
handleDragLeave/handleDragLeaveNestable and the similar logic at the other
block) to accept and propagate a zone key (use itemId+position for item zones
and `nest-${folderId}` for nest zones), store that as the currently active zone
(instead of relying solely on dragItemId()), and only clear the active highlight
when the received zone key exactly matches the stored active zone; ensure
callers pass the new zone key and the handlers compare against the stored zone
before clearing.
- Around line 139-145: When the PagesMenu unmounts, transient signals
(contextMenu, openFolders, dragItemId, dragOverItemId, dragOverPosition) keep
stale values; add a createEffect that watches props.isOpen and when it becomes
false call setContextMenu(null), setOpenFolders(new Set()), setDragItemId(null),
setDragOverItemId(null) and setDragOverPosition(null) to reset all transient UI
state; place the effect in the PagesMenu component scope so it runs on
open/close toggles.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ab998a75-9a85-410b-9f72-a7565c043314
📒 Files selected for processing (7)
src/components/PagesMenu.tsxsrc/components/TreeItem.tsxsrc/components/ui/Button.tsxsrc/components/ui/ContextMenu.tsxsrc/hooks/usePages.tssrc/lib/page-tree.tstests/page-tree.lib.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/page-tree.lib.test.ts
- src/lib/page-tree.ts
- src/components/TreeItem.tsx
- src/hooks/usePages.ts
- src/components/ui/Button.tsx
Comment on lines
+41
to
+42
| onDragLeave: (e: DragEvent) => void; | ||
| onDragLeaveNestable: () => void; |
There was a problem hiding this comment.
dragleave is not keyed to the zone that fired it.
Regular drop highlights never clear here because handleDragLeave() compares against dragItemId(), while handleDragLeaveNestable() can clear a different active folder because it only checks for the nest- prefix. Pass a zone key through these callbacks (itemId + position / nest-${folderId}) and clear only when that same zone is still active.
Also applies to: 269-283
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/PagesMenu.tsx` around lines 41 - 42, The dragleave handlers
are not keyed to the specific zone so clearing highlights can remove the wrong
active folder; update the callbacks onDragLeave and onDragLeaveNestable (and the
corresponding calls around handleDragLeave/handleDragLeaveNestable and the
similar logic at the other block) to accept and propagate a zone key (use
itemId+position for item zones and `nest-${folderId}` for nest zones), store
that as the currently active zone (instead of relying solely on dragItemId()),
and only clear the active highlight when the received zone key exactly matches
the stored active zone; ensure callers pass the new zone key and the handlers
compare against the stored zone before clearing.
Comment on lines
+139
to
+145
| const [contextMenu, setContextMenu] = createSignal<ContextMenuState>(null); | ||
| const [openFolders, setOpenFolders] = createSignal<Set<string>>(new Set()); | ||
| const [dragItemId, setDragItemId] = createSignal<string | null>(null); | ||
| const [dragOverItemId, setDragOverItemId] = createSignal<string | null>(null); | ||
| const [dragOverPosition, setDragOverPosition] = createSignal< | ||
| "before" | "after" | null | ||
| >(null); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd src/components && wc -l PagesMenu.tsxRepository: rmandotdev/editor
Length of output: 78
🏁 Script executed:
cd src/components && head -n 150 PagesMenu.tsx | tail -n 20Repository: rmandotdev/editor
Length of output: 961
🏁 Script executed:
cd src/components && sed -n '139,150p' PagesMenu.tsxRepository: rmandotdev/editor
Length of output: 629
🏁 Script executed:
cd src/components && sed -n '340,400p' PagesMenu.tsxRepository: rmandotdev/editor
Length of output: 1560
🏁 Script executed:
cd src/components && grep -n "createEffect\|createEffectCleanup" PagesMenu.tsxRepository: rmandotdev/editor
Length of output: 43
🏁 Script executed:
cd src/components && head -n 20 PagesMenu.tsxRepository: rmandotdev/editor
Length of output: 556
🏁 Script executed:
cd src/components && grep -n "props.isOpen" PagesMenu.tsxRepository: rmandotdev/editor
Length of output: 95
Reset transient menu and drag state when the panel closes.
The signals are declared at component scope, outside the <Show> component. When props.isOpen toggles to false, the DOM unmounts but these signals retain their values. Reopening the panel will display stale context menu positions, open folder states, or drag indicators from the previous session.
Add a createEffect that monitors props.isOpen and clears the state when it becomes false:
Suggested fix
-import { createSignal, Show } from "solid-js";
+import { createEffect, createSignal, Show } from "solid-js"; const handleDragEnd = () => {
setDragItemId(null);
setDragOverItemId(null);
setDragOverPosition(null);
};
+
+ createEffect(() => {
+ if (!props.isOpen) {
+ setContextMenu(null);
+ setOpenFolders(new Set());
+ handleDragEnd();
+ }
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const [contextMenu, setContextMenu] = createSignal<ContextMenuState>(null); | |
| const [openFolders, setOpenFolders] = createSignal<Set<string>>(new Set()); | |
| const [dragItemId, setDragItemId] = createSignal<string | null>(null); | |
| const [dragOverItemId, setDragOverItemId] = createSignal<string | null>(null); | |
| const [dragOverPosition, setDragOverPosition] = createSignal< | |
| "before" | "after" | null | |
| >(null); | |
| import { createEffect, createSignal, Show } from "solid-js"; | |
| // ... other code ... | |
| const [contextMenu, setContextMenu] = createSignal<ContextMenuState>(null); | |
| const [openFolders, setOpenFolders] = createSignal<Set<string>>(new Set()); | |
| const [dragItemId, setDragItemId] = createSignal<string | null>(null); | |
| const [dragOverItemId, setDragOverItemId] = createSignal<string | null>(null); | |
| const [dragOverPosition, setDragOverPosition] = createSignal< | |
| "before" | "after" | null | |
| >(null); | |
| const handleDragEnd = () => { | |
| setDragItemId(null); | |
| setDragOverItemId(null); | |
| setDragOverPosition(null); | |
| }; | |
| createEffect(() => { | |
| if (!props.isOpen) { | |
| setContextMenu(null); | |
| setOpenFolders(new Set()); | |
| handleDragEnd(); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/PagesMenu.tsx` around lines 139 - 145, When the PagesMenu
unmounts, transient signals (contextMenu, openFolders, dragItemId,
dragOverItemId, dragOverPosition) keep stale values; add a createEffect that
watches props.isOpen and when it becomes false call setContextMenu(null),
setOpenFolders(new Set()), setDragItemId(null), setDragOverItemId(null) and
setDragOverPosition(null) to reset all transient UI state; place the effect in
the PagesMenu component scope so it runs on open/close toggles.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/components/App.tsx (2)
39-39: Redundant function wrapper.
() => getCurrentPage()is equivalent to just usinggetCurrentPagedirectly. IfgetCurrentPageis already a reactive getter from the hook, the extra wrapper adds no value.- const currentPage = () => getCurrentPage(); + const currentPage = getCurrentPage;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/App.tsx` at line 39, Replace the redundant wrapper function currentPage that returns getCurrentPage with a direct reference to the getter: remove the arrow wrapper "const currentPage = () => getCurrentPage();" and use the existing getCurrentPage function/value directly wherever currentPage is used (or reassign: const currentPage = getCurrentPage) to avoid the unnecessary indirection; locate the symbol currentPage in src/components/App.tsx and update its usage to reference getCurrentPage.
110-112: Consider removing trivial wrapper.
selectPageByTreeItemdirectly delegates tosetCurrentPageIdwithout additional logic. Unless you plan to add logic here (e.g., closing menus, validation), you could passsetCurrentPageIddirectly toPagesMenuasselectPageByTreeItem.- const selectPageByTreeItem = (itemId: string) => { - setCurrentPageId(itemId); - };Then at Line 154:
- selectPageByTreeItem={selectPageByTreeItem} + selectPageByTreeItem={setCurrentPageId}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/App.tsx` around lines 110 - 112, The function selectPageByTreeItem is a trivial wrapper around setCurrentPageId; remove the wrapper and pass setCurrentPageId directly to PagesMenu instead. Find the selectPageByTreeItem declaration and its usages (prop passed into the <PagesMenu> component) and replace the prop value with setCurrentPageId, then delete the selectPageByTreeItem function; if you intend to keep the indirection for future logic, instead add a TODO comment above selectPageByTreeItem indicating future enhancements.src/components/PagesMenu.tsx (2)
326-338: Remove redundant conditional with identical branches.Both branches of this conditional execute exactly the same code, making the
draggedParent === targetParentcheck unnecessary.♻️ Suggested simplification
- if (draggedParent === targetParent) { - const newPages = - position === "before" - ? moveItemBefore(props.pages, draggedId, targetItem.id) - : moveItemAfter(props.pages, draggedId, targetItem.id); - props.setPages(newPages); - } else { - const newPages = - position === "before" - ? moveItemBefore(props.pages, draggedId, targetItem.id) - : moveItemAfter(props.pages, draggedId, targetItem.id); - props.setPages(newPages); - } + const newPages = + position === "before" + ? moveItemBefore(props.pages, draggedId, targetItem.id) + : moveItemAfter(props.pages, draggedId, targetItem.id); + props.setPages(newPages);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/PagesMenu.tsx` around lines 326 - 338, The conditional checking draggedParent === targetParent is redundant because both branches run identical code; remove the if/else and replace it with a single block that computes newPages using position ? moveItemBefore(props.pages, draggedId, targetItem.id) : moveItemAfter(props.pages, draggedId, targetItem.id) (or equivalent ternary/if) and then calls props.setPages(newPages); keep references to draggedId, targetItem.id, position, moveItemBefore, moveItemAfter, and props.setPages.
163-173: Consider validating against whitespace-only names.Line 169 checks
if (newName)which would reject empty strings but would accept whitespace-only names like" ". If whitespace names are invalid, consider trimming:- if (newName) { - props.renameItem(c.itemId, newName); + const trimmed = newName?.trim(); + if (trimmed) { + props.renameItem(c.itemId, trimmed);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/PagesMenu.tsx` around lines 163 - 173, The onRename handler currently accepts whitespace-only names because it checks if (newName) only; update onRename to trim the input (e.g., const trimmed = newName.trim()) and validate that trimmed is non-empty before calling props.renameItem(c.itemId, trimmed); use the trimmed value for renaming and clear the context menu as before; locate this logic in the onRename function and use findItemInTree/props.renameItem identifiers when applying the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/App.tsx`:
- Line 39: Replace the redundant wrapper function currentPage that returns
getCurrentPage with a direct reference to the getter: remove the arrow wrapper
"const currentPage = () => getCurrentPage();" and use the existing
getCurrentPage function/value directly wherever currentPage is used (or
reassign: const currentPage = getCurrentPage) to avoid the unnecessary
indirection; locate the symbol currentPage in src/components/App.tsx and update
its usage to reference getCurrentPage.
- Around line 110-112: The function selectPageByTreeItem is a trivial wrapper
around setCurrentPageId; remove the wrapper and pass setCurrentPageId directly
to PagesMenu instead. Find the selectPageByTreeItem declaration and its usages
(prop passed into the <PagesMenu> component) and replace the prop value with
setCurrentPageId, then delete the selectPageByTreeItem function; if you intend
to keep the indirection for future logic, instead add a TODO comment above
selectPageByTreeItem indicating future enhancements.
In `@src/components/PagesMenu.tsx`:
- Around line 326-338: The conditional checking draggedParent === targetParent
is redundant because both branches run identical code; remove the if/else and
replace it with a single block that computes newPages using position ?
moveItemBefore(props.pages, draggedId, targetItem.id) :
moveItemAfter(props.pages, draggedId, targetItem.id) (or equivalent ternary/if)
and then calls props.setPages(newPages); keep references to draggedId,
targetItem.id, position, moveItemBefore, moveItemAfter, and props.setPages.
- Around line 163-173: The onRename handler currently accepts whitespace-only
names because it checks if (newName) only; update onRename to trim the input
(e.g., const trimmed = newName.trim()) and validate that trimmed is non-empty
before calling props.renameItem(c.itemId, trimmed); use the trimmed value for
renaming and clear the context menu as before; locate this logic in the onRename
function and use findItemInTree/props.renameItem identifiers when applying the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b9dac52f-959c-4ff3-9757-4da0daf83240
📒 Files selected for processing (2)
src/components/App.tsxsrc/components/PagesMenu.tsx
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
1 participant
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.
Closes #6
Summary by CodeRabbit
New Features
Bug Fixes / UX
Tests
Chores