ENG-1127 fix: update left sidebar when page titles change #607
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
sid597
changed the title
sid597
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThese changes refactor the Left Sidebar components to use UID-based resolution for pages and blocks instead of title-based lookup, introducing getPageTitleByPageUid and getPageUidByPageTitle functions to ensure consistent title resolution and reference handling across target opening, label display, and favorites migration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/roam/src/components/LeftSidebarView.tsx (1)
489-500: Use sequential insertion to preserve favorites order during migration.Promise.allexecutes block insertions in parallel, which can cause favorites to appear out of order in Roam due to varying network latency. Since this is a one-time migration, sequential execution is preferable:- await Promise.all( - newFavorites.map(({ uid }) => - createBlock({ parentUid: childrenUid, node: { text: uid } }), - ), - ); + for (const { uid } of newFavorites) { + // eslint-disable-next-line no-await-in-loop + await createBlock({ parentUid: childrenUid, node: { text: uid } }); + }apps/roam/src/components/settings/LeftSidebarPersonalSettings.tsx (1)
146-175: Reject storing titles as child references; require valid page UIDs.
Line 154 currently falls back tochildName.trim(), which stores a title string instead of a UID. This breaks the downstreamSectionChildrencomponent, which callsparseReference(child.text)and expects a valid reference syntax. When a page doesn't exist, reject the operation instead of storing invalid data.- const targetUid = getPageUidByPageTitle(childName) || childName.trim(); + const normalized = childName.trim(); + const targetUid = getPageUidByPageTitle(normalized); + if (!targetUid) { + renderToast({ + content: `Page "${normalized}" not found`, + intent: "danger", + id: "add-child-page-not-found", + }); + return; + }apps/roam/src/components/settings/LeftSidebarGlobalSettings.tsx (1)
176-210: GuardtargetUidbefore writing; Enter key bypasses button validation and can create empty blocks.Line 182's
!pageNamecheck only catches falsy values—whitespace-only input (e.g.," ") passes through. ThengetPageUidByPageTitlereturns""at line 180, but theonConfirmcallback on line 317 invokesaddPageregardless of the button's disabled state, allowing line 190 to write an empty block text to the database. Trim input and guard the emptytargetUidto prevent this and provide clear user feedback.- const targetUid = getPageUidByPageTitle(pageName); + const normalized = pageName.trim(); + const targetUid = getPageUidByPageTitle(normalized); + if (!targetUid) { + renderToast({ + content: `Page "${normalized}" not found`, + intent: "warning", + id: "add-global-page-not-found", + }); + return; + } if (pages.some((p) => p.text === targetUid)) { console.warn(`Page "${pageName}" already exists in global section`); return; }
🧹 Nitpick comments (2)
apps/roam/src/components/settings/LeftSidebarGlobalSettings.tsx (1)
236-240: Disable logic is good; consider trimming to matchaddPage.apps/roam/src/components/LeftSidebarView.tsx (1)
59-85: UID-based open flow looks correct; consider adding an explicit return type.-const openTarget = async (e: React.MouseEvent, targetUid: string) => { +const openTarget = async ( + e: React.MouseEvent, + targetUid: string, +): Promise<void> => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/roam/src/components/LeftSidebarView.tsx(6 hunks)apps/roam/src/components/settings/LeftSidebarGlobalSettings.tsx(5 hunks)apps/roam/src/components/settings/LeftSidebarPersonalSettings.tsx(4 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/LeftSidebarGlobalSettings.tsxapps/roam/src/components/LeftSidebarView.tsxapps/roam/src/components/settings/LeftSidebarPersonalSettings.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/LeftSidebarGlobalSettings.tsxapps/roam/src/components/LeftSidebarView.tsxapps/roam/src/components/settings/LeftSidebarPersonalSettings.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/LeftSidebarGlobalSettings.tsxapps/roam/src/components/LeftSidebarView.tsxapps/roam/src/components/settings/LeftSidebarPersonalSettings.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/LeftSidebarGlobalSettings.tsxapps/roam/src/components/LeftSidebarView.tsxapps/roam/src/components/settings/LeftSidebarPersonalSettings.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/LeftSidebarGlobalSettings.tsxapps/roam/src/components/LeftSidebarView.tsxapps/roam/src/components/settings/LeftSidebarPersonalSettings.tsx
🧠 Learnings (7)
📓 Common learnings
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`.
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 559
File: apps/roam/src/utils/findDiscourseNode.ts:37-39
Timestamp: 2025-12-07T20:54:19.988Z
Learning: In apps/roam/src/utils/findDiscourseNode.ts, the function findDiscourseNodeByTitleAndUid accepts both uid and title parameters where uid is primarily used for cache access (as the cache key) while title is used for the actual matching via matchDiscourseNode. This design reflects the pattern where downstream, the uid is mostly used to fetch the title, so the function caches by uid but matches by title.
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-06-25T22:56:17.522Z
Learning: In the Roam discourse-graph system, the existence of the configuration page (identified by DISCOURSE_CONFIG_PAGE_TITLE) and its corresponding UID is a system invariant. The code can safely assume this page will always exist, so defensive null checks are not needed when using `getPageUidByPageTitle(DISCOURSE_CONFIG_PAGE_TITLE)`.
📚 Learning: 2025-12-07T20:54:19.988Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 559
File: apps/roam/src/utils/findDiscourseNode.ts:37-39
Timestamp: 2025-12-07T20:54:19.988Z
Learning: In apps/roam/src/utils/findDiscourseNode.ts, the function findDiscourseNodeByTitleAndUid accepts both uid and title parameters where uid is primarily used for cache access (as the cache key) while title is used for the actual matching via matchDiscourseNode. This design reflects the pattern where downstream, the uid is mostly used to fetch the title, so the function caches by uid but matches by title.
Applied to files:
apps/roam/src/components/settings/LeftSidebarGlobalSettings.tsxapps/roam/src/components/LeftSidebarView.tsxapps/roam/src/components/settings/LeftSidebarPersonalSettings.tsx
📚 Learning: 2025-06-25T22:56:17.522Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-06-25T22:56:17.522Z
Learning: In the Roam discourse-graph system, the existence of the configuration page (identified by DISCOURSE_CONFIG_PAGE_TITLE) and its corresponding UID is a system invariant. The code can safely assume this page will always exist, so defensive null checks are not needed when using `getPageUidByPageTitle(DISCOURSE_CONFIG_PAGE_TITLE)`.
Applied to files:
apps/roam/src/components/settings/LeftSidebarGlobalSettings.tsxapps/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/settings/LeftSidebarGlobalSettings.tsxapps/roam/src/components/LeftSidebarView.tsxapps/roam/src/components/settings/LeftSidebarPersonalSettings.tsx
📚 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,js,jsx} : Use the roamAlphaApi docs from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when implementing Roam functionality
Applied to files:
apps/roam/src/components/settings/LeftSidebarGlobalSettings.tsxapps/roam/src/components/LeftSidebarView.tsx
📚 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/LeftSidebarView.tsx
📚 Learning: 2025-06-22T10:40:52.752Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 232
File: apps/roam/src/utils/getAllDiscourseNodesSince.ts:18-31
Timestamp: 2025-06-22T10:40:52.752Z
Learning: In apps/roam/src/utils/getAllDiscourseNodesSince.ts, the user confirmed that querying for `?title` with `:node/title` and mapping it to the `text` field in the DiscourseGraphContent type is the correct implementation for retrieving discourse node content from Roam Research, despite it appearing to query page titles rather than block text content.
Applied to files:
apps/roam/src/components/LeftSidebarView.tsx
🔇 Additional comments (5)
apps/roam/src/components/settings/LeftSidebarPersonalSettings.tsx (2)
28-28: Import addition looks consistent with the UID-based display refactor.
373-384: Label resolution is a good compatibility bridge for legacy stored values.apps/roam/src/components/settings/LeftSidebarGlobalSettings.tsx (1)
18-19: Import addition looks fine.apps/roam/src/components/LeftSidebarView.tsx (2)
43-43: Import addition looks consistent with the new label-resolution path.
439-447: Migration switching to storing UIDs is aligned with the rename-safe approach.
apps/roam/src/components/settings/LeftSidebarGlobalSettings.tsx
Outdated
Show resolved
Hide resolved
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Generally looks good, but unfortunately merging this will break sidebar for anyone already using it.
Unfortunately, I think we need to include the migration/backwards compatibility in this PR. Unless no pilots are currently using the left sidebar (but I don't think that is the case, although I could be wrong).
If you'd like it to be a separate task that gets merged into this PR, that works for me.
sid597
force-pushed
the
eng-1127-left-sidebar-doesnt-update-when-pages-are-renamed
branch
from
3cb07ce to
8686da6
Compare
|
https://www.loom.com/share/9b28a4b59ab2400c944398dc55393655 based on the commit e3c5013 |
https://www.loom.com/share/e0af00458a4346798f59d9ca5dc437ab
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.