-
Notifications
You must be signed in to change notification settings - Fork 4
ENG-1154: Reload graph when left sidebar is enabled #605
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
base: main
Are you sure you want to change the base?
ENG-1154: Reload graph when left sidebar is enabled #605
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughA Beta feature panel is replaced with an interactive Checkbox control in the GeneralSettings component. When toggled, the Checkbox triggers block creation or deletion and displays a reload confirmation dialog, managing local state for enabled status, block UID, and alert visibility. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10–15 minutes
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: 1
🧹 Nitpick comments (1)
apps/roam/src/components/settings/GeneralSettings.tsx (1)
50-53: Consider specifying the order parameter for consistent positioning.The
createBlockcall doesn't include anorderparameter, which means the Left Sidebar setting will appear at an unpredictable position. Other settings in this file explicitly specify order (0, 1, 3).Apply this diff to add an order parameter:
createBlock({ parentUid: settings.settingsUid, - node: { text: "(BETA) Left Sidebar" }, + node: { text: "(BETA) Left Sidebar" }, + order: 2, })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/roam/src/components/settings/GeneralSettings.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/main.mdc)
**/*.{ts,tsx}: Use Tailwind CSS for styling where possible
When refactoring inline styles, use tailwind classes
Prefertypeoverinterfacein TypeScript
Use explicit return types for functions
Avoidanytypes when possible
Prefer arrow functions over regular function declarations
Use named parameters (object destructuring) when a function has more than 2 parameters
Use PascalCase for components and types
Use camelCase for variables and functions
Use UPPERCASE for constants
Function names should describe their purpose clearly
Prefer early returns over nested conditionals for better readability
Files:
apps/roam/src/components/settings/GeneralSettings.tsx
apps/roam/**/*.{js,ts,tsx,jsx,json}
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Prefer existing dependencies from package.json when working on the Roam Research extension
Files:
apps/roam/src/components/settings/GeneralSettings.tsx
apps/roam/**/*.{ts,tsx,jsx,js,css,scss}
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Files:
apps/roam/src/components/settings/GeneralSettings.tsx
apps/roam/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
apps/roam/**/*.{ts,tsx,js,jsx}: Use the roamAlphaApi docs from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when implementing Roam functionality
Use Roam Depot/Extension API docs from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when implementing extension functionality
Files:
apps/roam/src/components/settings/GeneralSettings.tsx
apps/roam/**
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Implement the Discourse Graph protocol in the Roam Research extension
Files:
apps/roam/src/components/settings/GeneralSettings.tsx
🧠 Learnings (2)
📚 Learning: 2025-11-25T00:52:41.934Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-11-25T00:52:41.934Z
Learning: Applies to apps/roam/**/*.{ts,tsx,jsx,js,css,scss} : Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Applied to files:
apps/roam/src/components/settings/GeneralSettings.tsx
📚 Learning: 2025-08-11T19:09:58.252Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 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/src/components/settings/GeneralSettings.tsx
🧬 Code graph analysis (1)
apps/roam/src/components/settings/GeneralSettings.tsx (1)
apps/roam/src/utils/discourseConfigRef.ts (1)
getFormattedConfigTree(51-86)
🔇 Additional comments (3)
apps/roam/src/components/settings/GeneralSettings.tsx (3)
1-1: LGTM! Imports follow guidelines.The new imports properly support the Left Sidebar toggle functionality using BlueprintJS 3 components and existing roamjs-components utilities.
Also applies to: 7-10
18-24: LGTM! State initialization is correct.The state variables properly initialize from settings with appropriate fallbacks.
75-85: Alert should clarify the consequence of clicking "Later".When users click "Later" after enabling the Left Sidebar, the feature config is persisted but non-functional until reload. Consider adding clarity to the alert message about the interim state, or alternatively, prevent dismissal without reload to avoid user confusion about why the feature doesn't work immediately.
The asymmetric Alert behavior (only shown on enable, not disable) is intentional—the observer dynamically checks the enabled setting on each DOM mutation, so disabling takes effect without requiring a reload.
ccb70e1 to
90b93bb
Compare
90b93bb to
bf1b4a2
Compare

Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.