-
Notifications
You must be signed in to change notification settings - Fork 3
ENG-1014 Add Admin Feature Flag for “Reified Relation Triples” Flow #532
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-1014 Add Admin Feature Flag for “Reified Relation Triples” Flow #532
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Did get a local coderabbit review. |
cc0c94b to
1cffd23
Compare
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.
Let's do the following before a full review:
- CodeRabbit web is more comprehensive than the local CLI, so let's make sure to run it on this PR as well.
- Every PR that contains UI is required to have a Loom overview video (see the Project Handbook)
- While doing
2, please walk through each bullet in theDone Whenof the Linear task to show that they are complete.
sid597
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.
From linear: Done When
-
Admin Panel now uses tabs
-
Default tab is "Home" or "Admin"
-
Default panel shows the “Use Reified Relation Triples” flag
The last 2 are not done, default tab is Node list
| } | ||
| onChange={(e) => { | ||
| const target = e.target as HTMLInputElement; | ||
| extensionAPI.settings.set( |
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.
use import { getSetting, setSetting } from "~/utils/extensionSettings"; instead of doing it this way
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.
CodeRabbit web is more comprehensive than the local CLI, so let's make sure to run it on this PR as well.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdminPanel component was refactored to implement a tabbed interface with an Admin settings tab and a Node list tab. The component now accepts an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: 0
🧹 Nitpick comments (1)
apps/roam/src/components/settings/AdminPanel.tsx (1)
106-106: Add explicit return type per coding guidelines.The component should have an explicit return type as per the project's TypeScript guidelines.
As per coding guidelines
Apply this diff:
-const AdminPanel = ({ onloadArgs }: { onloadArgs: OnloadArgs }) => { +const AdminPanel = ({ onloadArgs }: { onloadArgs: OnloadArgs }): React.ReactElement => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/roam/src/components/settings/AdminPanel.tsx(4 hunks)apps/roam/src/components/settings/Settings.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/main.mdc)
**/*.{ts,tsx}: Prefertypeoverinterface
Use explicit return types for functions
Avoidanytypes when possible
Files:
apps/roam/src/components/settings/AdminPanel.tsxapps/roam/src/components/settings/Settings.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/main.mdc)
**/*.{ts,tsx,js,jsx}: Prefer arrow functions over regular function declarations
Use named parameters (object destructuring) when a function has more than 2 parameters
Use Prettier with the project's configuration
Maintain consistent naming conventions: PascalCase for components and types
Maintain consistent naming conventions: camelCase for variables and functions
Maintain consistent naming conventions: UPPERCASE for constants
Files:
apps/roam/src/components/settings/AdminPanel.tsxapps/roam/src/components/settings/Settings.tsx
apps/roam/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
apps/roam/**/*.{js,jsx,ts,tsx}: Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Use the roamAlphaApi documentation from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when working with the Roam API
Use Roam Depot/Extension API documentation from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when working with the Roam Extension API
Files:
apps/roam/src/components/settings/AdminPanel.tsxapps/roam/src/components/settings/Settings.tsx
🧠 Learnings (4)
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 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/src/components/settings/AdminPanel.tsx
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 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/src/components/settings/AdminPanel.tsxapps/roam/src/components/settings/Settings.tsx
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 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/src/components/settings/AdminPanel.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/AdminPanel.tsx
🔇 Additional comments (4)
apps/roam/src/components/settings/Settings.tsx (1)
232-232: LGTM! Props correctly passed to AdminPanel.The onloadArgs prop is now passed to AdminPanel, correctly matching the updated component signature and enabling access to extensionAPI within the admin panel.
apps/roam/src/components/settings/AdminPanel.tsx (3)
2-14: LGTM! Imports properly added for tabbed UI.The new imports (Checkbox, Tab, TabId, Tabs, OnloadArgs, Description) are correctly added to support the tabbed interface and settings management functionality.
224-232: Consider using utility functions for settings access.A previous review comment suggested using
getSettingandsetSettingfrom~/utils/extensionSettingsinstead of accessingextensionAPI.settingsdirectly. This approach may provide better consistency across the codebase.Additionally, consider whether
defaultChecked(uncontrolled component) is appropriate here. If the setting can change externally while the panel is open, the checkbox won't reflect those changes. A controlled component with state may be more robust:const [useReifiedRelations, setUseReifiedRelations] = useState( extensionAPI.settings.get("use-reified-relations") as boolean ); // In Checkbox: checked={useReifiedRelations} onChange={(e) => { const target = e.target as HTMLInputElement; setUseReifiedRelations(target.checked); void extensionAPI.settings.set("use-reified-relations", target.checked); }}Based on learnings
213-293: LGTM! Tabbed interface well-structured.The tabbed UI is properly implemented with:
- Clean separation of Admin settings and Node list functionality
- Appropriate use of BlueprintJS 3 Tabs component
renderActiveTabPanelOnlyfor performance optimization- Preserved existing node list functionality with schema selector and loading states
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.
Remove the unused onloadArgs from the AdminPanel params (and import, etc).
Let's also separate node-list into it's own component, so we don't prematurely call supabase and load the nodes each time the Admin panel is opened.
|
That separation got done in ENG-1018. |
b702dff to
21d5b80
Compare
| Reified Relation Triples | ||
| <Description | ||
| description={ | ||
| "When ON, relations are read/written as sourceUid:relationBlockUid:destinationUid in [[roam/js/discourse-graph/relations]]." |
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.
I think instead of sourceUid:relationBlockUid:destinationUid maybe simpler wording would be better maybe the suggested text in linear?
“When ON, relations are read/written as reifiedRelationUid in [[roam/js/discourse-graph/relations]].”
This is what it looks like in ui and requires effort to read it
https://linear.app/discourse-graphs/issue/ENG-1014/add-admin-feature-flag-for-reified-relation-triples-flow
Summary by CodeRabbit