-
Notifications
You must be signed in to change notification settings - Fork 3
ENG-853 left sidebar global and personal settings #441
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ENG-853 left sidebar global and personal settings #441
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. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughAdds new left-sidebar configuration UIs: a global sections panel and a per-user personal sections panel. Integrates both as tabs in Settings. Extends left sidebar settings utilities by exporting additional types/functions and returning the Left Sidebar uid. Implements block creation, retrieval, and updates for sections, settings, and children. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant SettingsDialog
participant GlobalPanel as LeftSidebarGlobalSections
participant Utils as getLeftSidebarSettings/*Config
participant Roam as Roam Blocks API
User->>SettingsDialog: Open Settings
SettingsDialog->>GlobalPanel: Mount Global Tab
GlobalPanel->>Utils: getLeftSidebarSettings (includes uid)
Utils-->>GlobalPanel: { uid, tree }
alt Global-Section missing
GlobalPanel->>Roam: createBlock(Global-Section)
GlobalPanel->>Roam: createBlock(Folded, Children)
else Global-Section exists
GlobalPanel->>Utils: getLeftSidebarGlobalSectionConfig
Utils-->>GlobalPanel: { folded, collapsable, childrenUid, pages }
end
User->>GlobalPanel: Add/Remove Page
GlobalPanel->>Roam: createBlock/deleteBlock
Roam-->>GlobalPanel: Ack
GlobalPanel-->>User: Updated list
sequenceDiagram
autonumber
actor User
participant SettingsDialog
participant PersonalPanel as LeftSidebarPersonalSections
participant Utils as getLeftSidebarSettings/*Personal
participant Roam as Roam Blocks API
User->>SettingsDialog: Open Settings
SettingsDialog->>PersonalPanel: Mount Personal Tab
PersonalPanel->>Utils: getLeftSidebarSettings (Left Sidebar subtree)
alt Personal-Section missing
PersonalPanel->>Roam: createBlock(Personal-Section for user)
end
PersonalPanel->>Utils: getLeftSidebarPersonalSectionConfig
Utils-->>PersonalPanel: Sections + refs
User->>PersonalPanel: Add Section / Convert to Complex / Edit Settings / Manage Children
alt Convert to Complex
PersonalPanel->>Roam: createBlock(Settings, Children)
end
PersonalPanel->>Roam: createBlock/updateBlock/deleteBlock
Roam-->>PersonalPanel: Ack
PersonalPanel-->>User: Updated sections and children
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
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/utils/getLeftSidebarSettings.ts (1)
108-161: Fix potential crash when Personal-Section subtree is missing
personalLeftSidebarNodecan beundefined; accessing.uidwill throw.Apply this diff:
-export const getLeftSidebarPersonalSectionConfig = ( +export const getLeftSidebarPersonalSectionConfig = ( leftSidebarChildren: RoamBasicNode[], ): { uid: string; sections: LeftSidebarPersonalSectionConfig[] } => { const userName = getCurrentUserDisplayName(); const personalLeftSidebarNode = getSubTree({ tree: leftSidebarChildren, key: userName + "/Personal-Section", }); - if (personalLeftSidebarNode.uid === "") { + if (!personalLeftSidebarNode?.uid) { return { uid: "", sections: [], }; } - const sections = (personalLeftSidebarNode?.children || []).map( + const sections = (personalLeftSidebarNode.children || []).map( (sectionNode): LeftSidebarPersonalSectionConfig => { const settingsNode = sectionNode.children?.find( (child) => child.text === "Settings", ); const childrenNode = sectionNode.children?.find( (child) => child.text === "Children", ); @@ ); return { - uid: personalLeftSidebarNode.uid, + uid: personalLeftSidebarNode.uid, sections, }; }
🧹 Nitpick comments (3)
apps/roam/src/components/settings/LeftSidebarPersonalSettings.tsx (2)
51-55: GuardextractRefusage to avoid empty UID lookupsCalling
getTextByBlockUidwith an empty/invalid UID can error; add a guard.Apply this diff:
- const ref = extractRef(section.text); - const blockText = getTextByBlockUid(ref); + const ref = extractRef(section.text); + const blockText = ref ? getTextByBlockUid(ref) : "";
20-88: Minor rendering perf: avoid recomputing originalName on every renderWrap
originalNameinuseMemokeyed bysection.textto reduce DOM queries on re-renders.- const ref = extractRef(section.text); - const blockText = ref ? getTextByBlockUid(ref) : ""; - const originalName = blockText || section.text; + const originalName = useMemo(() => { + const ref = extractRef(section.text); + const blockText = ref ? getTextByBlockUid(ref) : ""; + return blockText || section.text; + }, [section.text]);apps/roam/src/components/settings/LeftSidebarGlobalSettings.tsx (1)
110-140: Duplicate check OK; consider normalizing caseOptional: normalize to avoid “Page” vs “page” duplicates if desired UX.
- if (pages.some((p) => p.text === pageName)) { + if (pages.some((p) => p.text.toLowerCase() === pageName.toLowerCase())) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/roam/src/components/settings/LeftSidebarGlobalSettings.tsx(1 hunks)apps/roam/src/components/settings/LeftSidebarPersonalSettings.tsx(1 hunks)apps/roam/src/components/settings/Settings.tsx(3 hunks)apps/roam/src/utils/getLeftSidebarSettings.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (10)
apps/roam/src/utils/getLeftSidebarSettings.ts (4)
29-35: Exporting Global config type looks goodPublic surface area addition is fine and consistent with usage in settings panels.
37-44: Including Left Sidebar uid in the config is a good additionThis enables downstream writes without re-deriving the container uid.
46-70: Global-section extraction logic LGTMDefaults on missing nodes are handled safely.
169-175: ReturningleftSidebarUidis correct and backward compatibleSafe optional chaining and defaults.
apps/roam/src/components/settings/Settings.tsx (3)
30-31: New imports are correct and scopedNo side effects; components render inside Tabs only.
152-157: Personal Left Sidebar tab wiring looks goodDistinct TabId, lazy render via panel. No state leaks.
173-178: Global Left Sidebar tab wiring looks goodMatches personal tab pattern; no prop mismatches.
apps/roam/src/components/settings/LeftSidebarGlobalSettings.tsx (3)
185-205: Panels parent/uid usage is correct given the fix aboveOnce real UIDs are captured/derived, these controls will persist reliably.
170-173: Disable add button logic is soundPrevents duplicates without extra queries.
1-15: Imports organizationNew import for
getBasicTreeByParentUidis required by the creation fix.
apps/roam/src/components/settings/LeftSidebarGlobalSettings.tsx
Outdated
Show resolved
Hide resolved
apps/roam/src/components/settings/LeftSidebarPersonalSettings.tsx
Outdated
Show resolved
Hide resolved
mdroidian
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some requested changes.
The bugs from up the stack hinder doing a full review, so I'll stop here.
The "complexSection" in personal settings is a little confusing from a UX perspective.
EG
- Can a user set an
Aliasif the section isn't complex? - Is the only way to add children to click the settings icon to change to "complexSection?
- Maybe we can add a "Add Children" button if the section isn't "complex" yet.
Additional questions
- can you set an alias to a child of a complex section? I image that will be a request quite quickly :)
We can talk through this at our next sync
apps/roam/src/components/settings/LeftSidebarGlobalSettings.tsx
Outdated
Show resolved
Hide resolved
apps/roam/src/components/settings/LeftSidebarGlobalSettings.tsx
Outdated
Show resolved
Hide resolved
apps/roam/src/components/settings/LeftSidebarPersonalSettings.tsx
Outdated
Show resolved
Hide resolved
apps/roam/src/components/settings/LeftSidebarPersonalSettings.tsx
Outdated
Show resolved
Hide resolved
apps/roam/src/components/settings/LeftSidebarPersonalSettings.tsx
Outdated
Show resolved
Hide resolved
apps/roam/src/components/settings/LeftSidebarPersonalSettings.tsx
Outdated
Show resolved
Hide resolved
apps/roam/src/components/settings/LeftSidebarPersonalSettings.tsx
Outdated
Show resolved
Hide resolved
4c9444d to
9c1db6a
Compare
4a1dffd to
51d439f
Compare
9c1db6a to
6b9694d
Compare
1046e6f to
7e53ccc
Compare
|
what if there is no button to add children and it always present, if its enpty don't show them if not then show? @mdroidian |
mdroidian
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apps/roam/src/components/settings/LeftSidebarPersonalSettings.tsx
Outdated
Show resolved
Hide resolved
d72bcb0 to
6517f19
Compare
69d9f56 to
1919b48
Compare
6517f19 to
5efb611
Compare
1919b48 to
67b6cd5
Compare
67b6cd5 to
c58f6b1
Compare
5ba858f
into
eng-852-render-left-sidebar-based-on-the-fetched-sections-and
) * fix style * fix styling * address coderabbit * fix double click to navigate, shift click to open in sidebar * address Michael and Johnny review * 1. Use (BETA) setting, on hover change UI to reflect its clickable state * ENG-853 left sidebar global and personal settings (#441) * left sidebar global and personal settings * address coderabbit * address ui feedback * add toast, fix void lint * text in center * sync settings and left sidebar rendering * simplify props * fix lint * 1. Address review 2. Use uid instead of username 3. Use settings block for global * add button for quick settings, full width sections, on click toggles * use global settings for BETA left sidebar feature * nix useeffect
) * fix style * fix styling * address coderabbit * fix double click to navigate, shift click to open in sidebar * address Michael and Johnny review * 1. Use (BETA) setting, on hover change UI to reflect its clickable state * ENG-853 left sidebar global and personal settings (#441) * left sidebar global and personal settings * address coderabbit * address ui feedback * add toast, fix void lint * text in center * sync settings and left sidebar rendering * simplify props * fix lint * 1. Address review 2. Use uid instead of username 3. Use settings block for global * add button for quick settings, full width sections, on click toggles * use global settings for BETA left sidebar feature * nix useeffect


https://www.loom.com/share/6f453441c5eb418cb728a6e165387340?sid=3a9e642d-413e-4717-bc01-1cc1882486a6
Summary by CodeRabbit
New Features
UX Improvements