ENG-852 Render left sidebar based on fetched sections and settings #440
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
changed the title
sid597
force-pushed
the
eng-852-render-left-sidebar-based-on-the-fetched-sections-and
branch
from
885d8da to
f7a65f0
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughAdds a new React LeftSidebarView component driven by formatted Roam config and a mountLeftSidebar function. Integrates a DOM mutation observer to detect the starred-pages wrapper and mount the custom left sidebar. Implements item opening (blocks/pages), folding/unfolding, collapsible sections, and alias/truncation handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant DOM as DOM
participant Observer as leftSidebarObserver
participant Mount as mountLeftSidebar()
participant React as LeftSidebarView
participant Roam as roamAlphaAPI
DOM->>Observer: Mutation: DIV.starred-pages-wrapper detected
Observer->>Mount: mountLeftSidebar(container)
Mount->>React: ReactDOM.render(LeftSidebarView)
Note over React: Renders Global & Personal sections from formatted config
rect rgba(230,245,255,0.6)
note right of React: User interactions
participant User as User
User->>React: Click item
alt Block reference
React->>Roam: openBlockInSidebar / open block (shift behavior)
else Page title
React->>Roam: open page / open outline window (shift behavior)
end
User->>React: Toggle chevron
React->>React: toggleFoldedState (create/delete "Folded" child)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 Pre-merge checks✅ Passed checks (3 passed)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
apps/roam/src/utils/initializeObserversAndListeners.ts (1)
103-112: Left sidebar observer: make idempotent and drop noisy logging.Guard against repeated mounts and avoid console spam.
- const leftSidebarObserver = createHTMLObserver({ + const leftSidebarObserver = createHTMLObserver({ tag: "DIV", useBody: true, className: "starred-pages-wrapper", - callback: (el) => { - console.log("[DG][LeftSidebar] leftSidebarObserver callback", el); - const container = el as HTMLDivElement; - mountLeftSidebar(container); - }, + callback: (el) => { + const container = el as HTMLDivElement; + if (container.querySelector("#dg-left-sidebar-root")) return; // already mounted + mountLeftSidebar(container); + }, });apps/roam/src/components/LeftSidebarView.tsx (4)
30-57: Right sidebar open: verify window payload; avoid ts-expect-error.Roam’s API typically uses
page-uidfor page outlines. Replaceblock-uidand remove the suppression.- if (e.shiftKey) { - await window.roamAlphaAPI.ui.rightSidebar.addWindow({ - // @ts-expect-error - todo test - // eslint-disable-next-line @typescript-eslint/naming-convention - window: { type: "outline", "block-uid": uid }, - }); - } else { + if (e.shiftKey) { + await window.roamAlphaAPI.ui.rightSidebar.addWindow({ + // eslint-disable-next-line @typescript-eslint/naming-convention + window: { type: "outline", "page-uid": uid }, + }); + } else { await window.roamAlphaAPI.ui.mainWindow.openPage({ page: { uid } }); }
89-120: Section item accessibility.Provide button semantics for keyboard users and screen readers.
- return ( - <div key={child.uid} className="py-1 pl-1"> - <div - className={ - "section-child-item page cursor-pointer rounded-sm leading-normal text-gray-600" - } - onClick={onClick} - > - {label} - </div> - </div> - ); + return ( + <div key={child.uid} className="py-1 pl-1"> + <div + className="section-child-item page cursor-pointer rounded-sm leading-normal text-gray-600" + role="button" + tabIndex={0} + aria-label={`Open ${label}`} + onClick={onClick} + onKeyDown={(e) => (e.key === "Enter" || e.key === " ") && onClick(e as any)} + > + {label} + </div> + </div> + );
252-261: Config memoization won’t update on config changes.
useMemo(() => getFormattedConfigTree()..., [])locks the view to initial config. If the config tree refreshes (e.g., viarefreshConfigTree()), the sidebar won’t update until remount.Consider wiring a lightweight pub/sub or an
extensionAPI.settings.onChangetrigger to force a re-render of this component.
279-289: Minor: preserve existing root classes instead of overwrite.Switching to
classList.add(as above) avoids clobbering future styling. No separate change needed if you adopt the previous diff.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/roam/src/components/LeftSidebarView.tsx(1 hunks)apps/roam/src/utils/initializeObserversAndListeners.ts(3 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 (5)
apps/roam/src/utils/initializeObserversAndListeners.ts (2)
48-48: Import looks good.
343-344: Observer registration LGTM.apps/roam/src/components/LeftSidebarView.tsx (3)
16-23: parseReference: clarify non-ref inputs.If
textis plain (no [[ ]] or (( ))),extractRef(text)may return the original string; treating it as a page is fine, but verify this matches expected UX (no-op if page doesn’t exist vs. creating/opening).Would you prefer creating the page when missing?
153-166: Click behavior: shift-click vs. toggle.Current logic toggles on click unless Shift; good. After fixing fold semantics, ensure the chevron reflects state. No change needed here.
221-229: Global toggle semantics depend on the fixed toggleFoldedState.After inverting
toggleFoldedState, this handler will behave correctly. Nothing else to change.Please re-test the “Global-Section → Folded” case from the PR instructions to confirm it renders collapsed on load.
sid597
changed the title
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Found some bugs
https://www.loom.com/share/9a552a7c3b83483ab75acdbed38b65a6
I did change 📝 Daily Notes {{Create Today's Entry:SmartBlock:UserDNPToday:RemoveButton=false}} {{Pick A Day:SmartBlock:UserDNPDateSelect:RemoveButton=false}} to a block ref but it would only navigate there ~10% of the time randomly clicking on that header
sid597
force-pushed
the
eng-851-get-left-sidebar-sections-and-settings-from-config-page
branch
from
57e4be1 to
32f9c22
Compare
sid597
force-pushed
the
eng-852-render-left-sidebar-based-on-the-fetched-sections-and
branch
2 times, most recently
from
9c1db6a to
6b9694d
Compare
| if (!uid) return; | ||
| if (e.shiftKey) { | ||
| await window.roamAlphaAPI.ui.rightSidebar.addWindow({ | ||
| // @ts-expect-error - todo test |
There was a problem hiding this comment.
😅 I copied it from other parts of codebase, I was not sure what to do about it so I searched and copied it to atleast keep it consistent if someone searched for it in future
sid597
force-pushed
the
eng-852-render-left-sidebar-based-on-the-fetched-sections-and
branch
from
d72bcb0 to
6517f19
Compare
sid597
changed the base branch from
eng-851-get-left-sidebar-sections-and-settings-from-config-page
to
graphite-base/440
sid597
force-pushed
the
graphite-base/440
branch
from
98d057d to
bc3a60e
Compare
sid597
force-pushed
the
eng-852-render-left-sidebar-based-on-the-fetched-sections-and
branch
from
6517f19 to
5efb611
Compare
|
@mdroidian https://www.loom.com/share/cee226c07fd94db78c612eb10f874723?sid=b10ebc99-b31f-4376-b422-23b01956772d added (BETA) setting and we only render or mount if its checked |
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
A few things
-
I think the "Feature Flag" is going to have to be global for this one. We can make it overridable when we do the settings overhaul, but for the most part it will be turned on for the whole graph. Let's change it to global.
-
Add the same left padding to headers as children have. Right now headers have no left padding.
- Don't show hover/pointer cursor on GLOBAL if it isn't collapsible (because it isn't clickable).
| import { render as renderToast } from "roamjs-components/components/Toast"; | ||
| import refreshConfigTree from "~/utils/refreshConfigTree"; | ||
| import { refreshAndNotify } from "~/components/LeftSidebarView"; | ||
| import { memo } from "react"; |
There was a problem hiding this comment.
add this to the other react import at the top
* 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
sid597
force-pushed
the
eng-852-render-left-sidebar-based-on-the-fetched-sections-and
branch
from
b64e6e1 to
d6e1a10
Compare
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
🚀
After you nix the useEffect, feel free to merge
|
@mdroidian you have to merge its blocked for me |
) * 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
To test this pr paste the following in your
roam/js/discourse-graphpage (change you username)Summary by CodeRabbit