ENG-882 Let user rearrange personal section and section items in place in left sidebar #456
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
sid597
force-pushed
the
eng-882-let-user-rearrange-using-drag-and-drop-left-sidebar-sections
branch
from
629cbda to
45df4d7
Compare
sid597
changed the base branch from
eng-852-render-left-sidebar-based-on-the-fetched-sections-and
to
graphite-base/456
sid597
force-pushed
the
graphite-base/456
branch
from
b8ff5e5 to
c54a276
Compare
sid597
force-pushed
the
eng-882-let-user-rearrange-using-drag-and-drop-left-sidebar-sections
branch
from
45df4d7 to
54b875b
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughAdds drag-and-drop reordering for left-sidebar personal sections, their items, and global pages using @hello-pangea/dnd across view and settings components; updates local config state and calls roamAlphaAPI.moveBlock on drag end; removes alias handling from personal section settings/types; updates package.json to add the DnD dependency and adjust tldraw lines. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant View as LeftSidebarView
participant DnD as @hello-pangea/dnd
participant Roam as roamAlphaAPI
User->>View: start drag (section/item)
View->>DnD: Drag context active (renderClone, activeDragSourceId)
User->>DnD: reorders / drop
DnD->>View: onDragEnd(result)
alt result.type == "SECTIONS"
View->>View: reorder local sections (setConfig)
View->>Roam: moveBlock(uid, newIndex)
else result.type == "ITEMS"
View->>View: reorder items in section (setConfig)
View->>Roam: moveBlock(uid, newIndex, parentUid)
end
View->>View: update UI state
sequenceDiagram
autonumber
actor User
participant Settings as LeftSidebar*Settings
participant DnD as @hello-pangea/dnd
participant Roam as roamAlphaAPI
participant Notify as refresh/notify
User->>Settings: drag page/section/child
Settings->>DnD: renderClone / start drag
User->>DnD: drop
DnD->>Settings: onDragEnd(result)
Settings->>Settings: reorder local array(s)
opt item has backing Roam block
Settings->>Roam: moveBlock(uid, newIndex[, parentUid])
end
Settings->>Notify: refresh or re-sync config
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks✅ Passed checks (3 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/roam/src/components/LeftSidebarView.tsx (1)
402-437: Fold/expand logic is inverted (Folded means collapsed, but isOpen is driven by folded.value)isOpen is initialized from folded.value, and Collapse uses isOpen directly, so folded=true expands instead of collapses. toggleFoldedState also treats isOpen as “folded”. This reverses UI.
Make isOpen reflect expanded state and update toggleFoldedState accordingly.
- const [isOpen, setIsOpen] = useState<boolean>( - !!config.settings?.folded.value, - ); + const [isOpen, setIsOpen] = useState<boolean>( + !config.settings?.folded.value, + );And in toggleFoldedState:
- if (isOpen) { - setIsOpen(false); - if (folded.uid) { - void deleteBlock(folded.uid); - folded.uid = undefined; - folded.value = false; - } - } else { - setIsOpen(true); - const newUid = window.roamAlphaAPI.util.generateUID(); - void createBlock({ - parentUid, - node: { text: "Folded", uid: newUid }, - }); - folded.uid = newUid; - folded.value = true; - } + if (isOpen) { + // Collapse: ensure Folded exists + setIsOpen(false); + if (!folded.uid) { + const newUid = window.roamAlphaAPI.util.generateUID(); + void createBlock({ parentUid, node: { text: "Folded", uid: newUid } }); + folded.uid = newUid; + } + folded.value = true; + } else { + // Expand: remove Folded + setIsOpen(true); + if (folded.uid) void deleteBlock(folded.uid); + folded.uid = undefined; + folded.value = false; + }Mirror the same isOpen initialization for PersonalSectionItem (Line 173-175):
- const [isOpen, setIsOpen] = useState<boolean>( - !!section.settings?.folded.value || false, - ); + const [isOpen, setIsOpen] = useState<boolean>( + !section.settings?.folded.value || false, + );
🧹 Nitpick comments (4)
apps/roam/src/components/settings/LeftSidebarGlobalSettings.tsx (1)
318-357: Type hygiene for renderClone (optional)renderClone params are untyped here, while other files type them. Add minimal types for consistency and editor tooling.
- renderClone={(provided, snapshot, rubric) => { + renderClone={( + provided: DraggableProvided, + _snapshot, + rubric: import("@hello-pangea/dnd").DraggableRubric, + ) => {apps/roam/src/components/LeftSidebarView.tsx (2)
486-489: Remove stray console logLeftover debug logging can be noisy in user consoles.
- console.log("handleGlobalPointerDownCapture");
317-325: Double-check moveBlock order adjustmentUsing +1 when moving down is done here but not in GlobalSettings. Please standardize after confirming Roam’s semantics to avoid inconsistent ordering across the app.
See my comment in LeftSidebarGlobalSettings.tsx for details and suggested verification steps.
apps/roam/src/components/settings/LeftSidebarPersonalSettings.tsx (1)
227-229: Minor: equality check stylePrefer strict equality. Not a blocker given the controlled values, but TS/ESLint often prefers ===.
- (!section.settings && section.children?.length == 0) || !section.children; + (!section.settings && (section.children?.length === 0)) || !section.children;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
apps/roam/package.json(2 hunks)apps/roam/src/components/LeftSidebarView.tsx(6 hunks)apps/roam/src/components/settings/LeftSidebarGlobalSettings.tsx(3 hunks)apps/roam/src/components/settings/LeftSidebarPersonalSettings.tsx(9 hunks)apps/roam/src/utils/getLeftSidebarSettings.ts(0 hunks)
💤 Files with no reviewable changes (1)
- apps/roam/src/utils/getLeftSidebarSettings.ts
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/package.json : Prefer existing dependencies from package.json when adding or using dependencies in the Roam Research extension
Applied to files:
apps/roam/package.json
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use Roam Depot/Extension API documentation from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when working with the Roam Extension API
Applied to files:
apps/roam/package.json
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use the roamAlphaApi documentation from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when working with the Roam API
Applied to files:
apps/roam/package.json
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Applied to files:
apps/roam/package.json
📚 Learning: 2025-08-11T19:09:58.252Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#337
File: apps/roam/src/components/DiscourseFloatingMenu.tsx:43-43
Timestamp: 2025-08-11T19:09:58.252Z
Learning: The roam subdirectory (apps/roam) is constrained to React 17 and cannot use React 18+ features like createRoot API. ReactDOM.render should be used instead of createRoot in this subdirectory.
Applied to files:
apps/roam/package.json
🪛 GitHub Actions: CI
apps/roam/src/components/LeftSidebarView.tsx
[error] 19-19: TS2305: Module '"@hello-pangea/dnd"' has no exported member 'DraggingRubric'.
🔇 Additional comments (4)
apps/roam/package.json (2)
31-31: DnD dependency addition looks good@hello-pangea/dnd ^18.0.1 is compatible with React 17 (supports 16.8+ and 19). Safe for Roam’s React 17 setup.
Based on learnings
45-47: tldraw 2.3.0 is React 17-compatible – no action required
tldraw v2.3.0 declares peerDependencies of react/react-dom ^16.8 || ^17.0 || ^18.0, so it satisfies Roam’s React 17 lock.apps/roam/src/components/LeftSidebarView.tsx (1)
363-397: Good DnD composition for sectionsWiring of DragDropContext/Droppable/Draggable and controlled state updates looks solid. Disabling dragging when length <= 1 is a nice UX touch.
apps/roam/src/components/settings/LeftSidebarPersonalSettings.tsx (1)
502-571: Reorder persistence logic looks consistentThe state reorder + moveBlock (with +1 adjust when moving downward) and a refresh on completion is consistent and user-friendly.
Please ensure the same order adjustment is necessary in all intra-parent moves (match it with GlobalSettings/LeftSidebarView after verifying Roam’s semantics).
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/roam/package.json (1)
31-46: Downgrade @hello-pangea/dnd to a React 17-compatible releaseThe Roam bundle is locked to React 17 (see retrieved learnings), but 18.x of
@hello-pangea/dndships React 18/19 type definitions. CI now fails withTS2786: 'Droppable' cannot be used as a JSX componentacross the new DnD JSX (e.g.,LeftSidebarView.tsx:242), so the build is broken. Please pin the dependency to the latest 17.x (or otherwise React‑17-compatible) release so the type surface matches our runtime.- "@hello-pangea/dnd": "^18.0.1", + "@hello-pangea/dnd": "^17.0.0",Based on learnings
🧹 Nitpick comments (1)
apps/roam/src/components/LeftSidebarView.tsx (1)
313-325: Refresh config cache after persisting reorder
moveBlockupdates Roam but we never callrefreshAndNotify, sodiscourseConfigRefsubscribers (including other sidebar instances and settings dialogs) keep stale ordering until some unrelated refresh occurs. The settings components in this PR alreadythen(refreshAndNotify)after similar moves—mirror that here to keep the shared cache consistent.- void window.roamAlphaAPI.moveBlock({ - location: { "parent-uid": config.personal.uid, order: finalIndex }, - block: { uid: removed.uid }, - }); + void window.roamAlphaAPI + .moveBlock({ + location: { "parent-uid": config.personal.uid, order: finalIndex }, + block: { uid: removed.uid }, + }) + .then(() => { + refreshAndNotify(); + }); @@ - void window.roamAlphaAPI.moveBlock({ - location: { "parent-uid": parentUid || "", order: finalIndex }, - block: { uid: removedItem.uid }, - }); + void window.roamAlphaAPI + .moveBlock({ + location: { "parent-uid": parentUid || "", order: finalIndex }, + block: { uid: removedItem.uid }, + }) + .then(() => { + refreshAndNotify(); + });Also applies to: 353-359
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
apps/roam/package.json(2 hunks)apps/roam/src/components/LeftSidebarView.tsx(6 hunks)apps/roam/src/components/settings/LeftSidebarGlobalSettings.tsx(3 hunks)apps/roam/src/components/settings/LeftSidebarPersonalSettings.tsx(9 hunks)apps/roam/src/utils/getLeftSidebarSettings.ts(0 hunks)
💤 Files with no reviewable changes (1)
- apps/roam/src/utils/getLeftSidebarSettings.ts
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/package.json : Prefer existing dependencies from package.json when adding or using dependencies in the Roam Research extension
Applied to files:
apps/roam/package.json
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use Roam Depot/Extension API documentation from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when working with the Roam Extension API
Applied to files:
apps/roam/package.json
📚 Learning: 2025-08-11T19:09:58.252Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#337
File: apps/roam/src/components/DiscourseFloatingMenu.tsx:43-43
Timestamp: 2025-08-11T19:09:58.252Z
Learning: The roam subdirectory (apps/roam) is constrained to React 17 and cannot use React 18+ features like createRoot API. ReactDOM.render should be used instead of createRoot in this subdirectory.
Applied to files:
apps/roam/package.json
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use the roamAlphaApi documentation from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when working with the Roam API
Applied to files:
apps/roam/package.json
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Applied to files:
apps/roam/package.json
🪛 GitHub Actions: CI
apps/roam/src/components/LeftSidebarView.tsx
[error] 242-242: TypeScript error TS2786: 'Droppable' cannot be used as a JSX component.
|
@mdroidian I need help with the lint errors I have spend a few hours on it still no solution or figuring out why is it so, llms are of no help either, they suggest version mismatch, tried to run with that but could not find any fix. I also have tried downgrading the library version to You can checkout the branch if feasible and have a quick look |
Could you give me some more information. Which lint errors? And what have you tried so far (other than downgrading)? |
|
Oh, are you referring to |
|
@sid597 We should really try to stick with already installed npm packages unless there is a really good reason to introduce a new one or switch (not being maintained, for example, like |
|
@mdroidian I looked at the usage of all the manual calculations we need to do in this component did not give me confidence to use it, this is not the right layer of abstraction for dnd imo Drag and drop behaviour using the existing library: https://www.loom.com/share/db0771125c5c46afb7b52c1f1267c97e?sid=7ba23a86-8dab-4418-8394-16071867e1e7 Behaviour using https://www.loom.com/share/cd6220345c5b42dd8da94933065c8a80?sid=3b03e902-5867-4ab7-a0a4-1fd5eb8b99ef We get the smooth behaviour out of box but with the existing one there is the jankiness (yes I did not spend much time using the current library to make it work smoothly but we should not imo) |
|
Thank you for the detailed explanation and videos. Yeah, much smoother. |
sid597
force-pushed
the
eng-882-let-user-rearrange-using-drag-and-drop-left-sidebar-sections
branch
from
8d990b2 to
54f29b4
Compare
sid597
force-pushed
the
eng-882-let-user-rearrange-using-drag-and-drop-left-sidebar-sections
branch
from
54f29b4 to
6c5035f
Compare
|
@mdroidian pnpm fixed the bug |
sid597
changed the title
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Couple of changes requested but looks good other than that!
Here is one user interaction that might be confusing to users, but we can address it later:
https://www.loom.com/share/f9da6ad167b0450ca40fe8649886e85f
apps/roam/src/components/settings/LeftSidebarGlobalSettings.tsx
Outdated
Show resolved
Hide resolved
apps/roam/src/components/settings/LeftSidebarPersonalSettings.tsx
Outdated
Show resolved
Hide resolved
sid597
force-pushed
the
eng-882-let-user-rearrange-using-drag-and-drop-left-sidebar-sections
branch
from
7f557d0 to
768a531
Compare
Yeah I agree that we need improvements in these areas created a linear ticket for the same ENG-941: Drag and drop interactions can be improved |
Summary by CodeRabbit
New Features
Refactor
Chores