ENG-1069: Move existing shortcuts to new global shortcuts #586
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 a favorites migration that converts existing starred-pages into the Left Sidebar block structure, tracks migration via a new BooleanSetting, and makes Changes
Sequence Diagram(s)sequenceDiagram
participant LeftSidebar as LeftSidebarView
participant Starred as StarredPagesContainer
participant Config as DiscourseConfigPage
participant RoamAPI as Roam API
Note over LeftSidebar,Starred: Mount flow on extension load
LeftSidebar->>Starred: Detect existing starred-pages container
LeftSidebar->>LeftSidebar: call migrateFavorites()
LeftSidebar->>RoamAPI: get pages from starred container
RoamAPI-->>LeftSidebar: list of starred page titles/uids
LeftSidebar->>Config: get Left Sidebar config & favoritesMigrated flag
Config-->>LeftSidebar: config uid and favoritesMigrated setting
alt not migrated
LeftSidebar->>RoamAPI: get/create Left Sidebar, Global-Section, Children blocks
RoamAPI-->>LeftSidebar: created/located block uids
LeftSidebar->>RoamAPI: create/update child blocks for titles (deduplicate)
RoamAPI-->>LeftSidebar: created child block uids
LeftSidebar->>Config: set favoritesMigrated = true
Config-->>LeftSidebar: acknowledge
else already migrated
LeftSidebar-->>LeftSidebar: skip migration
end
LeftSidebar->>Starred: clear wrappers / cleanup DOM
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
🧹 Nitpick comments (1)
apps/roam/src/components/LeftSidebarView.tsx (1)
412-499: migrateFavorites logic matches the one-time migration designThe helper cleanly implements the intended flow:
- Respects the graph-level
favoritesMigratedflag and exits early when set.- Uses the
*/Personal-Sectionheuristic against the live Left Sidebar tree to avoid touching graphs where the plugin has already been configured by any user.- Lazily creates
Left Sidebar,Global-Section, andChildrenblocks when missing, usinggetBasicTreeByParentUidto reuse existing ones when present.- Reads starred page titles from the legacy
.starred-pagesDOM, dedupes against existingChildrenentries, and only writes missing ones.- Marks migration complete by creating a
Favorites Migratedchild underLeft Sidebarand refreshing the config tree, so subsequent sessions are fast.Two small, optional refinements you might consider:
Add an explicit return type for clarity and to align with your TS guidelines:
-const migrateFavorites = async (starredPagesContainer: Element) => {
+const migrateFavorites = async (
- starredPagesContainer: Element,
+): Promise => {
// ...
}- If you ever expect duplicate DOM entries in `.starred-pages`, you could dedupe `titles` before filtering against `existingTitles` (right now dedupe only happens vs. existing children, not within the new list itself). Neither is blocking; the current implementation is sound and matches the PR objectives. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 79af2b74aff7c88464e49059b726914c2a4694fe and f4c95d50fa8e0248f2542bfc2e1480ea7358717d. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `apps/roam/src/components/LeftSidebarView.tsx` (2 hunks) * `apps/roam/src/utils/getLeftSidebarSettings.ts` (2 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (5)</summary> <details> <summary>**/*.{ts,tsx}</summary> **📄 CodeRabbit inference engine (.cursor/rules/main.mdc)** > `**/*.{ts,tsx}`: Use Tailwind CSS for styling where possible > When refactoring inline styles, use tailwind classes > Prefer `type` over `interface` in TypeScript > Use explicit return types for functions > Avoid `any` types 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/LeftSidebarView.tsx` - `apps/roam/src/utils/getLeftSidebarSettings.ts` </details> <details> <summary>apps/roam/**/*.{js,ts,tsx,jsx,json}</summary> **📄 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/LeftSidebarView.tsx` - `apps/roam/src/utils/getLeftSidebarSettings.ts` </details> <details> <summary>apps/roam/**/*.{ts,tsx,jsx,js,css,scss}</summary> **📄 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/LeftSidebarView.tsx` - `apps/roam/src/utils/getLeftSidebarSettings.ts` </details> <details> <summary>apps/roam/**/*.{ts,tsx,js,jsx}</summary> **📄 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/LeftSidebarView.tsx` - `apps/roam/src/utils/getLeftSidebarSettings.ts` </details> <details> <summary>apps/roam/**</summary> **📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)** > Implement the Discourse Graph protocol in the Roam Research extension Files: - `apps/roam/src/components/LeftSidebarView.tsx` - `apps/roam/src/utils/getLeftSidebarSettings.ts` </details> </details><details> <summary>🧬 Code graph analysis (2)</summary> <details> <summary>apps/roam/src/components/LeftSidebarView.tsx (2)</summary><blockquote> <details> <summary>apps/roam/src/utils/renderNodeConfigPage.ts (1)</summary> * `DISCOURSE_CONFIG_PAGE_TITLE` (29-29) </details> <details> <summary>apps/roam/src/utils/discourseConfigRef.ts (1)</summary> * `getFormattedConfigTree` (51-86) </details> </blockquote></details> <details> <summary>apps/roam/src/utils/getLeftSidebarSettings.ts (1)</summary><blockquote> <details> <summary>apps/roam/src/utils/getExportSettings.ts (2)</summary> * `BooleanSetting` (12-12) * `getUidAndBooleanSetting` (56-62) </details> </blockquote></details> </details> </details> <details> <summary>🔇 Additional comments (3)</summary><blockquote> <details> <summary>apps/roam/src/utils/getLeftSidebarSettings.ts (2)</summary><blockquote> `37-45`: **favoritesMigrated added to LeftSidebarConfig is well-scoped** Adding `favoritesMigrated: BooleanSetting` to `LeftSidebarConfig` cleanly exposes the graph-level migration flag and matches how it’s consumed in `LeftSidebarView` (`config.favoritesMigrated.value`). Type choice and placement look correct. --- `174-181`: **favoritesMigrated lookup correctly uses Left Sidebar children** Using `getUidAndBooleanSetting` against `leftSidebarChildren` with text `"Favorites Migrated"` aligns with how the flag is written in `migrateFavorites` (as a direct child of `Left Sidebar`). Missing-flag behavior (`value === false`) is safe and keeps the migration idempotent. </blockquote></details> <details> <summary>apps/roam/src/components/LeftSidebarView.tsx (1)</summary><blockquote> `42-43`: **New imports line up with migration responsibilities** `getBasicTreeByParentUid` and `DISCOURSE_CONFIG_PAGE_TITLE` are the right primitives for reading/patching the config tree during migration; both are used only in `migrateFavorites`, keeping concerns localized. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
sid597
changed the title
sid597
force-pushed
the
eng-1069-check-if-the-existing-shortcuts-move-to-the-global-shortcuts
branch
from
9df36dd to
35e8404
Compare
sid597
force-pushed
the
eng-1069-check-if-the-existing-shortcuts-move-to-the-global-shortcuts
branch
from
35e8404 to
459bd2c
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/roam/src/components/LeftSidebarView.tsx (2)
419-419: Remove redundant type assertion.The
favoritesMigratedfield is already typed asBooleanSettingin theLeftSidebarConfiginterface (Line 39 in getLeftSidebarSettings.ts), so the type assertion is unnecessary.- if ((config.favoritesMigrated as BooleanSetting).value) return; + if (config.favoritesMigrated.value) return;
489-502: Consider consolidating config refresh calls.If
newTitles.length > 0, the code callsrefreshAndNotify()(Line 495) and thenrefreshConfigTree()(Line 502), resulting in two refresh operations. SincerefreshAndNotify()already callsrefreshConfigTree(), the second call is redundant when titles are added.Refactor to refresh once:
const newTitles = titles.filter((t) => !existingTitles.has(t)); if (newTitles.length > 0) { await Promise.all( newTitles.map((text) => createBlock({ parentUid: childrenUid, node: { text } }), ), ); - refreshAndNotify(); } await createBlock({ parentUid: leftSidebarUid, node: { text: "Favorites Migrated" }, }); - refreshConfigTree(); + refreshAndNotify();This calls
refreshAndNotify()once at the end, covering both the title additions and the flag creation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/roam/src/components/LeftSidebarView.tsx(3 hunks)apps/roam/src/utils/getLeftSidebarSettings.ts(2 hunks)apps/roam/src/utils/initializeObserversAndListeners.ts(1 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/utils/initializeObserversAndListeners.tsapps/roam/src/utils/getLeftSidebarSettings.tsapps/roam/src/components/LeftSidebarView.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/utils/initializeObserversAndListeners.tsapps/roam/src/utils/getLeftSidebarSettings.tsapps/roam/src/components/LeftSidebarView.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/utils/initializeObserversAndListeners.tsapps/roam/src/utils/getLeftSidebarSettings.tsapps/roam/src/components/LeftSidebarView.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/utils/initializeObserversAndListeners.tsapps/roam/src/utils/getLeftSidebarSettings.tsapps/roam/src/components/LeftSidebarView.tsx
apps/roam/**
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Implement the Discourse Graph protocol in the Roam Research extension
Files:
apps/roam/src/utils/initializeObserversAndListeners.tsapps/roam/src/utils/getLeftSidebarSettings.tsapps/roam/src/components/LeftSidebarView.tsx
🧠 Learnings (2)
📚 Learning: 2025-11-06T13:48:35.007Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-11-06T13:48:35.007Z
Learning: In apps/roam/src/utils/createReifiedBlock.ts, the `setBlockProps` function does not need await as it is synchronous or fire-and-forget.
Applied to files:
apps/roam/src/components/LeftSidebarView.tsx
📚 Learning: 2025-11-05T21:57:14.909Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 534
File: apps/roam/src/utils/createReifiedBlock.ts:40-48
Timestamp: 2025-11-05T21:57:14.909Z
Learning: In the discourse-graph repository, the function `getPageUidByPageTitle` from `roamjs-components/queries/getPageUidByPageTitle` is a synchronous function that returns a string directly (the page UID or an empty string if not found), not a Promise. It should be called without `await`.
Applied to files:
apps/roam/src/components/LeftSidebarView.tsx
🧬 Code graph analysis (3)
apps/roam/src/utils/initializeObserversAndListeners.ts (1)
apps/roam/src/components/LeftSidebarView.tsx (1)
mountLeftSidebar(505-525)
apps/roam/src/utils/getLeftSidebarSettings.ts (1)
apps/roam/src/utils/getExportSettings.ts (2)
BooleanSetting(12-12)getUidAndBooleanSetting(56-62)
apps/roam/src/components/LeftSidebarView.tsx (3)
apps/roam/src/utils/renderNodeConfigPage.ts (1)
DISCOURSE_CONFIG_PAGE_TITLE(29-29)apps/roam/src/utils/discourseConfigRef.ts (1)
getFormattedConfigTree(51-86)apps/roam/src/utils/getExportSettings.ts (1)
BooleanSetting(12-12)
🔇 Additional comments (2)
apps/roam/src/utils/getLeftSidebarSettings.ts (1)
39-39: LGTM! Clean integration of migration flag.The
favoritesMigratedfield follows the existing pattern for boolean settings (e.g.,collapsable,folded), usesgetUidAndBooleanSettingcorrectly, and aligns with the migration logic in LeftSidebarView.tsx.Also applies to: 174-180
apps/roam/src/components/LeftSidebarView.tsx (1)
505-524: LGTM! Async mounting with proper migration sequencing.The signature change to
asyncwith explicitPromise<void>return type is correct. Migration runs before DOM manipulation (Line 514 before Line 515), and the guard at Line 513 ensures migration only runs when the root element doesn't exist, preventing redundant execution within the same mount cycle.
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Nice. Couple suggestions, but generally looks good 👍
| if (existingStarred) { | ||
| existingStarred.remove(); | ||
| } | ||
| await migrateFavorites(); |
There was a problem hiding this comment.
Ideally we only run this when the left sidebar flag is turned on. It is probably unncessary to run every load.
There was a problem hiding this comment.
yeah and this will only run when the flag is turned on
There was a problem hiding this comment.
But this runs on every load, correct? That is not ideal.
sid597
force-pushed
the
eng-1069-check-if-the-existing-shortcuts-move-to-the-global-shortcuts
branch
from
459bd2c to
4027227
Compare
sid597
deleted the
eng-1069-check-if-the-existing-shortcuts-move-to-the-global-shortcuts
branch
Solution
Implemented one-time migration that extracts starred pages from Roam's native$\to$ Global-Section $\to$ Children in the graph configuration.
.starred-pagesDOM and adds them to Left SidebarCases Handled
Fresh Install (New Graph)
Existing Install (Collaborative Graph)
{userId}/Personal-Sectionunder Left SidebarFresh Install, Multiple Sessions
Multi-User Graph (Late Joiner)
Key Design Decisions
*/Personal-Sectionpattern to detect if plugin already in use by any userrefreshConfigTree()after creating flag to prevent duplicate blocks in same sessionTechnical Notes
mountLeftSidebarbefore DOM is clearedgetUidAndBooleanSettingpattern consistent with existing config flagsawaitchains to ensure flag is created before subsequent callshttps://www.loom.com/share/fc639adb1f4341d68403a19094b6dd8c
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
✏️ Tip: You can customize this high-level summary in your review settings.