Roam: Left-Sidebar [Eng-678] create mvp0 #334
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Caution Review failedAn error occurred during the review process. Please try again later. 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: 7
🧹 Nitpick comments (11)
apps/roam/src/components/settings/Settings.tsx (1)
169-181: Make Left Sidebar header styling consistent with other sectionsOther section headers use “text-lg font-semibold text-neutral-dark”. Mirror that here for visual consistency.
- <SectionHeader>Left Sidebar</SectionHeader> + <SectionHeader className="text-lg font-semibold text-neutral-dark"> + Left Sidebar + </SectionHeader>apps/roam/src/components/LeftSidebarView.tsx (2)
146-174: Simplify single/double-click detectionManual timers work but are brittle. Consider using onDoubleClick for the “open” action and onClick for “toggle collapse.” Keep shift-click behavior for opening in sidebar.
Example:
<div className="sidebar-title-button" onClick={() => collapsable && setIsOpen((p) => !p)} onDoubleClick={(e) => { e.preventDefault(); if (titleRef.type === "page") void window.roamAlphaAPI.ui.mainWindow.openPage({ page: { uid: getPageUidByPageTitle(titleRef.title) }, }); else if (titleRef.type === "block") void window.roamAlphaAPI.ui.mainWindow.openBlock({ block: { uid: titleRef.uid }, }); }} onMouseDown={(e) => { if (e.shiftKey) { // preserve shift-click open behavior e.preventDefault(); if (titleRef.type === "page") void openTarget(e as any, { kind: "page", title: titleRef.title }); else if (titleRef.type === "block") void openTarget(e as any, { kind: "block", uid: titleRef.uid }); } }} />
59-95: Optional: improve accessibility of clickable rowsClickable divs should expose role and keyboard handlers.
- <div key={child.uid} style={{ padding: "4px 0 4px 4px" }}> - <div + <div key={child.uid} style={{ padding: "4px 0 4px 4px" }}> + <div className={`section-child-item ${isTodo ? "todo-item" : "page"}`} style={{ color: "#495057", lineHeight: 1.5, borderRadius: 3, cursor: "pointer", }} - onClick={onClick} + role="button" + tabIndex={0} + onClick={onClick} + onKeyDown={(e) => (e.key === "Enter" || e.key === " ") && onClick(e as any)} > {label} </div> </div>Apply similarly to the simple personal section item.
apps/roam/src/components/settings/LeftSidebar.tsx (8)
3-3: Avoid recomputing all page names on every render; memoizegetAllPageNames().
getAllPageNames()can be expensive in large graphs. Memoize once per mount and reuse.-import React, { useCallback, useEffect, useState } from "react"; +import React, { useCallback, useEffect, useMemo, useState } from "react";return ( <div className="flex flex-col gap-4 p-1"> + { /* Compute once */ } + { /* eslint-disable react-hooks/rules-of-hooks */ } + const allPageNames = useMemo(() => getAllPageNames(), []);- <AutocompleteInput + <AutocompleteInput key={getAutocompleteKey()} value={getPageInput()} setValue={setPageInput} placeholder="Add page…" - options={getAllPageNames()} + options={allPageNames} maxItemsDisplayed={50} />If lint complains about hooks in JSX, move the
useMemoto the top of the component body beforereturn.Also applies to: 127-129, 149-156
114-125: RemovesetTimeout(…, 0)in controlled setters.This anti-pattern risks state updates after unmount and complicates reasoning. Directly set state.
-const setPageInput = useCallback( - (value: string) => { - setTimeout(() => { - setNewPageInputs((prev) => ({ - ...prev, - [globalSection.childrenUid]: value, - })); - }, 0); - }, +const setPageInput = useCallback( + (value: string) => { + setNewPageInputs((prev) => ({ + ...prev, + [globalSection.childrenUid]: value, + })); + }, [globalSection.childrenUid], );If this was a workaround for AutocompleteInput focus/blur behavior, call it out in a comment or keep a minimal debounce with
useRefinstead ofsetTimeout.
175-179: Add accessible titles to icon-only buttons.Improves a11y and UX.
- <Button + <Button icon="trash" minimal small - onClick={() => void removePage(p)} + title="Remove page" + onClick={() => void removePage(p)} />
424-431: Memoize page list in Personal component too.Avoid recomputing
getAllPageNames()on every render for both the “Add child” and “Add section or page” inputs.- const [settingsDialogSectionUid, setSettingsDialogSectionUid] = useState< + const [settingsDialogSectionUid, setSettingsDialogSectionUid] = useState< string | null >(null); const [expandedChildLists, setExpandedChildLists] = useState<Set<string>>( new Set(), ); + const allPageNames = useMemo(() => getAllPageNames(), []);- <AutocompleteInput + <AutocompleteInput key={childAutocompleteKeys[inputKey] || 0} value={childInput} setValue={setChildInput} placeholder="Add child page…" - options={getAllPageNames()} + options={allPageNames} maxItemsDisplayed={50} />- <AutocompleteInput + <AutocompleteInput key={autocompleteKey} value={newSectionInput} setValue={setNewSectionInput} placeholder="Add section or page…" - options={getAllPageNames()} + options={allPageNames} maxItemsDisplayed={50} />Also applies to: 482-488, 196-209
497-498: Avoid returning Promises from event handlers; prefix withvoid.Prevents unhandled Promise warnings and clarifies intent.
- onClick={() => addSection(newSectionInput)} + onClick={() => void addSection(newSectionInput)}- onClick={() => - addChildToSection(section.childrenUid!, childInput) - } + onClick={() => void addChildToSection(section.childrenUid!, childInput)}- onClick={() => removeChild(child)} + onClick={() => void removeChild(child)}Also applies to: 433-440, 453-454
210-250: Centralize skeleton creation (single source-of-truth).UI components shouldn’t mutate config shape. Prefer moving “ensure Left Sidebar skeleton” into a shared utility (e.g., in
~/utils/getLeftSidebarSettingsor~/utils/discourseConfigRef) so:
- It runs once, deterministically.
- All consumers see a consistent, validated tree.
- UI code focuses on edits, not initialization.
I can draft
ensureLeftSidebarSkeleton()and wire both components to call it. Say the word.
129-185: UX polish: surface errors to users, not just console.Where create/delete operations fail, notify users (toaster/snackbar) so they understand that a change didn’t persist.
Examples:
- add/remove page
- add/remove section
- add/remove child
- convert to complex section
I can wire Blueprint Toaster or an existing notification helper here.
Also applies to: 466-597
131-138: Labels consistency (minor).“Open” vs “Open?” and “Truncate-result?” style differs between Global and Personal. If these strings are only labels (not used for parsing in
getLeftSidebarSettings), consider making them consistent for a cleaner UX.Please confirm whether
getLeftSidebarSettingsparses by label text or by structure/UID only. If by label text, keep as-is to avoid breaking parsing.Also applies to: 370-394
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/roam/src/components/LeftSidebarView.tsx(1 hunks)apps/roam/src/components/settings/LeftSidebar.tsx(1 hunks)apps/roam/src/components/settings/Settings.tsx(3 hunks)apps/roam/src/utils/discourseConfigRef.ts(3 hunks)apps/roam/src/utils/getLeftSidebarSettings.ts(1 hunks)apps/roam/src/utils/initializeObserversAndListeners.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#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/Settings.tsx
🧬 Code Graph Analysis (5)
apps/roam/src/components/settings/Settings.tsx (1)
apps/roam/src/components/settings/LeftSidebar.tsx (2)
LeftSidebarGlobalSections(17-185)LeftSidebarPersonalSections(187-599)
apps/roam/src/utils/discourseConfigRef.ts (1)
apps/roam/src/utils/getLeftSidebarSettings.ts (2)
LeftSidebarConfig(31-37)getLeftSidebarSettings(39-52)
apps/roam/src/utils/initializeObserversAndListeners.ts (1)
apps/roam/src/components/LeftSidebarView.tsx (1)
mountLeftSidebarInto(353-395)
apps/roam/src/components/settings/LeftSidebar.tsx (3)
apps/roam/src/utils/discourseConfigRef.ts (1)
getFormattedConfigTree(31-57)apps/roam/src/utils/renderNodeConfigPage.ts (1)
DISCOURSE_CONFIG_PAGE_TITLE(28-28)apps/roam/src/utils/getLeftSidebarSettings.ts (1)
LeftSidebarPersonalSectionConfig(15-22)
apps/roam/src/utils/getLeftSidebarSettings.ts (1)
apps/roam/src/utils/getExportSettings.ts (4)
IntSetting(13-13)BooleanSetting(12-12)getUidAndBooleanSetting(56-62)getUidAndIntSetting(49-55)
🔇 Additional comments (9)
apps/roam/src/utils/getLeftSidebarSettings.ts (3)
39-52: Solid top-level extraction flowLocating "Left Sidebar" and delegating to global/personal parsers is clean and resilient to missing nodes via safe fallbacks.
54-73: Global section parsing looks correct
- Presence-based boolean for "Open" aligns with existing getUidAndBooleanSetting contract.
- Children and UIDs default safely when missing.
No changes needed.
75-116: Personal sections: good simple/complex split and safe fallbacks
- isSimple derivation is clear.
- Complex branch returns settings and children safely.
No changes needed here.
apps/roam/src/utils/discourseConfigRef.ts (1)
10-13: Left Sidebar config integrated correctly
- Type FormattedConfigTree extended with leftSidebar
- getFormattedConfigTree returns leftSidebar via getLeftSidebarSettings
- Import path and types align with the new utility
Looks good.
Also applies to: 28-29, 55-56
apps/roam/src/components/settings/Settings.tsx (1)
29-33: Imports for Left Sidebar settings are correctComponents are co-located and namespacing matches.
apps/roam/src/utils/initializeObserversAndListeners.ts (3)
44-45: Correct dependency addedImporting mountLeftSidebarInto here is the right place for DOM integration.
106-116: Immediate mount is useful; ensure it’s coordinated with readinessThis pre-empts waiting for the observer. With the proposed onReady flow in LeftSidebarView, this is fine and avoids blank “starred” content removal until the sidebar has content.
Please validate end-to-end after applying the LeftSidebarView onReady change to ensure the default starred pages aren’t removed until the custom sidebar is ready.
118-131: Observer-based mount is appropriate and idempotentUsing className filter and wrapper scoping is sufficient. mountLeftSidebarInto guards via a fixed root id.
apps/roam/src/components/settings/LeftSidebar.tsx (1)
18-25: Default fallbacks cover missing config—no null-guards neededThe
getFormattedConfigTree→getLeftSidebarSettingspath always supplies defaults when nodes are absent:
- leftSidebarNode?.children and childrenNode?.children both fall back to
[]- UIDs default to
""and BooleanSettings default tofalse- Personal sections likewise return empty arrays and default values
As a result, on initial render
settings.leftSidebar.global.childrenis always an array (neverundefined), anduid/valuefields are non-null. No additional guards or early returns are required here.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
apps/roam/src/components/settings/LeftSidebar.tsx (2)
91-113: Harden addPage: trim, case-insensitive dedupe, and guard missing childrenUidPrevents whitespace-only entries, case-variant duplicates, and failures if Children is missing.
- const addPage = async (page: string) => { - if (!page || pages.some((p) => p.text === page)) { - return; - } + const addPage = async (page: string) => { + const name = page.trim(); + if (!name) return; + if (pages.some((p) => p.text.toLowerCase() === name.toLowerCase())) return; + if (!globalSection.childrenUid) { + console.error("Global Section 'Children' UID missing."); + return; + } try { await createBlock({ parentUid: globalSection.childrenUid, order: "last", - node: { text: page }, + node: { text: name }, });
386-446: Avoid non-null assertions on childrenUid; guard and handle missingPrevents runtime crashes if parsing missed the Children node.
- const inputKey = section.childrenUid!; + const inputKey = section.childrenUid; + if (!inputKey) return null; const childInput = sectionChildInputs[inputKey] || ""; @@ - addChildToSection(section.childrenUid!, childInput); + addChildToSection(inputKey, childInput); @@ - <Button + <Button icon="plus" small minimal disabled={!childInput} - onClick={() => addChildToSection(section.childrenUid!, childInput)} + onClick={() => void addChildToSection(inputKey, childInput)} />apps/roam/src/components/LeftSidebarView.tsx (2)
336-355: Avoid rendering with empty configThe component renders immediately even if the config is empty, which could cause the blank sidebar issue mentioned in past reviews.
360-372: Defer removal of default starred list until readyThe function removes the default starred-pages immediately, which can cause a blank sidebar if the LeftSidebarView isn't ready with content.
apps/roam/src/utils/getLeftSidebarSettings.ts (1)
138-151: Robustify defaults for invalid setting valuesThe default values only apply when the setting node or UID is missing, but not when the value is invalid (e.g., non-numeric for truncateResult).
🧹 Nitpick comments (7)
apps/roam/src/components/settings/Settings.tsx (1)
168-180: Align SectionHeader styling with other headersFor visual consistency, use the same typography as “Personal Settings” and “Global Settings”.
- <SectionHeader>Left Sidebar</SectionHeader> + <SectionHeader className="text-lg font-semibold text-neutral-dark"> + Left Sidebar + </SectionHeader>apps/roam/src/utils/discourseConfigRef.ts (2)
10-13: Optional: import defaults-aware helper to harden parsingTo avoid undefined UIDs or missing children when the skeleton hasn’t been ensured, prefer the defaults-aware accessor.
-import { - LeftSidebarConfig, - getLeftSidebarSettings, -} from "./getLeftSidebarSettings"; +import { LeftSidebarConfig, getLeftSidebarSettings } from "./getLeftSidebarSettings"; +import { getLeftSidebarSettingsWithDefaults } from "./ensureLeftSidebarStructure";
55-56: Optional: return leftSidebar with defaults to improve resilienceIf the config tree is partially populated, using defaults avoids null-ish values.
- leftSidebar: getLeftSidebarSettings(configTreeRef.tree), + leftSidebar: getLeftSidebarSettingsWithDefaults(configTreeRef.tree),apps/roam/src/utils/initializeObserversAndListeners.ts (1)
118-131: Skip redundant mounts if root already existsMinor optimization to avoid unnecessary ReactDOM.render calls when the observer fires multiple times for the same container.
const leftSidebarObserver = createHTMLObserver({ tag: "DIV", useBody: true, className: "starred-pages-wrapper", callback: (el) => { try { const container = el as HTMLDivElement; - mountLeftSidebarInto(container); + if (container.querySelector("#dg-left-sidebar-root")) return; + mountLeftSidebarInto(container); } catch (e) { console.error("[DG][LeftSidebar] leftSidebarObserver error", e); } }, });apps/roam/src/components/settings/LeftSidebar.tsx (1)
38-54: Remove debug log from initializationAvoid noisy logs in production.
try { await ensureLeftSidebarReady(); const config = getFormattedConfigTree(); - console.log("config", config); setSettings(config); setIsInitialized(true);apps/roam/src/components/LeftSidebarView.tsx (1)
285-285: Consider persisting Global Section open stateThe Global Section uses local React state for open/closed, unlike Personal Sections which persist to Roam blocks. This means the Global Section state won't survive page reloads.
Consider using the same persistence mechanism as Personal Sections for consistency, or document why this difference is intentional.
apps/roam/src/utils/getLeftSidebarSettings.ts (1)
144-151: Reconsider default values for collapsable and open settingsThe defaults set both
collapsableandopento false. This means sections are non-collapsible and closed by default, which seems inconsistent. Consider:
- If not collapsible, the open state is irrelevant
- If collapsible, defaulting to closed might hurt discoverability
Consider more intuitive defaults:
const collapsableSetting = getBoolean("Collapsable?"); if (!settingsNode?.uid || !collapsableSetting.uid) { - collapsableSetting.value = false; + collapsableSetting.value = true; } const openSetting = getBoolean("Open?"); if (!settingsNode?.uid || !openSetting.uid) { - openSetting.value = false; + openSetting.value = true; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/roam/src/components/LeftSidebarView.tsx(1 hunks)apps/roam/src/components/settings/LeftSidebar.tsx(1 hunks)apps/roam/src/components/settings/Settings.tsx(2 hunks)apps/roam/src/utils/discourseConfigRef.ts(2 hunks)apps/roam/src/utils/ensureLeftSidebarStructure.ts(1 hunks)apps/roam/src/utils/getLeftSidebarSettings.ts(1 hunks)apps/roam/src/utils/initializeObserversAndListeners.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
apps/roam/src/utils/ensureLeftSidebarStructure.ts (2)
apps/roam/src/utils/renderNodeConfigPage.ts (1)
DISCOURSE_CONFIG_PAGE_TITLE(28-28)apps/roam/src/utils/getLeftSidebarSettings.ts (3)
LeftSidebarConfig(34-40)getLeftSidebarGlobalSectionConfig(65-84)getLeftSidebarPersonalSectionConfig(86-123)
apps/roam/src/utils/discourseConfigRef.ts (1)
apps/roam/src/utils/getLeftSidebarSettings.ts (2)
LeftSidebarConfig(34-40)getLeftSidebarSettings(42-63)
apps/roam/src/components/settings/Settings.tsx (1)
apps/roam/src/components/settings/LeftSidebar.tsx (2)
LeftSidebarGlobalSections(200-218)LeftSidebarPersonalSections(587-605)
apps/roam/src/components/settings/LeftSidebar.tsx (3)
apps/roam/src/utils/getLeftSidebarSettings.ts (2)
LeftSidebarPersonalSectionConfig(18-25)LeftSidebarGlobalSectionConfig(27-32)apps/roam/src/utils/discourseConfigRef.ts (2)
FormattedConfigTree(20-29)getFormattedConfigTree(31-57)apps/roam/src/utils/ensureLeftSidebarStructure.ts (1)
ensureLeftSidebarReady(72-83)
apps/roam/src/utils/getLeftSidebarSettings.ts (1)
apps/roam/src/utils/getExportSettings.ts (4)
IntSetting(13-13)BooleanSetting(12-12)getUidAndBooleanSetting(56-62)getUidAndIntSetting(49-55)
apps/roam/src/components/LeftSidebarView.tsx (2)
apps/roam/src/utils/getLeftSidebarSettings.ts (2)
LeftSidebarPersonalSectionConfig(18-25)LeftSidebarConfig(34-40)apps/roam/src/utils/discourseConfigRef.ts (1)
getFormattedConfigTree(31-57)
apps/roam/src/utils/initializeObserversAndListeners.ts (1)
apps/roam/src/components/LeftSidebarView.tsx (1)
mountLeftSidebarInto(357-397)
🔇 Additional comments (7)
apps/roam/src/components/settings/Settings.tsx (1)
28-31: Imports look correct and scopedThe Left Sidebar settings components are imported from the right location and named exports match.
apps/roam/src/utils/discourseConfigRef.ts (1)
20-29: Good extension of the formatted config treeAdding leftSidebar to FormattedConfigTree is a clean extension and keeps existing fields intact.
apps/roam/src/utils/initializeObserversAndListeners.ts (3)
44-45: Import is correct and localizedThe new mount function is imported from the right module.
106-117: Immediate mount is a good UX improvementProactively mounting if the wrapper already exists avoids waiting for the observer tick.
340-341: Observer correctly added to the listEnsures teardown via the returned observers array.
apps/roam/src/components/LeftSidebarView.tsx (2)
50-58: Add null check before using page UIDThe function gets a page UID but only checks for null on line 51, yet still attempts to use it on line 57 even when it's undefined.
Apply this diff to handle the null case properly:
const uid = getPageUidByPageTitle(target.title); if (!uid) return; if (e.shiftKey) { await window.roamAlphaAPI.ui.rightSidebar.addWindow({ window: { type: "outline", "block-uid": uid }, }); } else { - await window.roamAlphaAPI.ui.mainWindow.openPage({ page: { uid } }); + if (uid) { + await window.roamAlphaAPI.ui.mainWindow.openPage({ page: { uid } }); + } }Likely an incorrect or invalid review comment.
176-176: Client-side generateUID is supported in Roam
The call towindow.roamAlphaAPI.util.generateUID()is part of Roam’s in-browser API and is widely used by community extensions and roam/js scripts. If this component only runs inside the Roam app (e.g. in a roam/js block or Depot extension), no fallback is necessary.
Optionally, if you ever expect this code to execute outside of the Roam context, you can guard the call with a check forwindow.roamAlphaAPI?.util?.generateUIDand throw or log an error—but there’s no need to supply a temporary UID in normal in-app use.
| setTimeout(() => { | ||
| try { | ||
| ReactDOM.render(<LeftSidebarView />, root); | ||
| } catch (e) { | ||
| console.error("[DG][LeftSidebar] render error", e); | ||
| } | ||
| }, 500); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace fixed timeout with proper initialization check
Using a fixed 500ms timeout is unreliable - it might be too short in some cases or unnecessarily long in others.
Consider using a more robust initialization check:
- setTimeout(() => {
- try {
- ReactDOM.render(<LeftSidebarView />, root);
- } catch (e) {
- console.error("[DG][LeftSidebar] render error", e);
- }
- }, 500);
+ const waitForConfig = (retries = 10) => {
+ const config = getFormattedConfigTree().leftSidebar;
+ const hasContent = (config.global.children?.length || 0) > 0 ||
+ (config.personal.sections?.length || 0) > 0;
+
+ if (hasContent || retries <= 0) {
+ try {
+ ReactDOM.render(<LeftSidebarView />, root);
+ } catch (e) {
+ console.error("[DG][LeftSidebar] render error", e);
+ }
+ } else {
+ setTimeout(() => waitForConfig(retries - 1), 100);
+ }
+ };
+
+ waitForConfig();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| setTimeout(() => { | |
| try { | |
| ReactDOM.render(<LeftSidebarView />, root); | |
| } catch (e) { | |
| console.error("[DG][LeftSidebar] render error", e); | |
| } | |
| }, 500); | |
| const waitForConfig = (retries = 10) => { | |
| const config = getFormattedConfigTree().leftSidebar; | |
| const hasContent = | |
| (config.global.children?.length || 0) > 0 || | |
| (config.personal.sections?.length || 0) > 0; | |
| if (hasContent || retries <= 0) { | |
| try { | |
| ReactDOM.render(<LeftSidebarView />, root); | |
| } catch (e) { | |
| console.error("[DG][LeftSidebar] render error", e); | |
| } | |
| } else { | |
| setTimeout(() => waitForConfig(retries - 1), 100); | |
| } | |
| }; | |
| waitForConfig(); |
🤖 Prompt for AI Agents
In apps/roam/src/components/LeftSidebarView.tsx around lines 390 to 396, replace
the fixed setTimeout render delay with a robust initialization check that
ensures the target root element exists and the app is ready before rendering;
implement either waiting for document.readyState/DOMContentLoaded or use a
short, cancellable poll (or a MutationObserver) to detect the root element, then
call ReactDOM.render (or createRoot) once and only once, and handle errors
within that single render attempt; ensure you clear any observers/timeouts after
success and retain the existing console.error handling for render exceptions.
| !getPageInput() || pages.some((p) => p.text === getPageInput()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Match disabled condition to normalization logic
Use trimmed input and case-insensitive duplicate check to mirror addPage.
- disabled={
- !getPageInput() || pages.some((p) => p.text === getPageInput())
- }
+ disabled={
+ !getPageInput().trim() ||
+ pages.some(
+ (p) => p.text.toLowerCase() === getPageInput().trim().toLowerCase(),
+ )
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| !getPageInput() || pages.some((p) => p.text === getPageInput()) | |
| } | |
| disabled={ | |
| - !getPageInput() || pages.some((p) => p.text === getPageInput()) | |
| + !getPageInput().trim() || | |
| + pages.some( | |
| + (p) => | |
| + p.text.toLowerCase() === getPageInput().trim().toLowerCase(), | |
| + ) | |
| } |
🤖 Prompt for AI Agents
In apps/roam/src/components/settings/LeftSidebar.tsx around lines 175 to 176,
the disabled condition uses the raw input and a case-sensitive check but addPage
normalizes (trims and lowercases) before comparing; change the condition to use
a normalized value (const normalized = getPageInput()?.trim().toLowerCase()) and
disable when normalized is empty or pages.some(p => p.text.trim().toLowerCase()
=== normalized) so the button matches the same trimmed, case-insensitive
duplicate logic as addPage.
| const addSection = async (sectionName: string) => { | ||
| if (!sectionName || sections.some((s) => s.text === sectionName)) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| await createBlock({ | ||
| parentUid: personalSection.uid, | ||
| order: "last", | ||
| node: { text: sectionName }, | ||
| }); | ||
| refreshSections(); | ||
| setNewSectionInput(""); | ||
| setAutocompleteKey((prev) => prev + 1); | ||
| } catch (error) { | ||
| console.error("Failed to add section:", error); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden addSection: trim and avoid case-variant duplicates
Avoids accidental duplicates and whitespace-only sections.
- const addSection = async (sectionName: string) => {
- if (!sectionName || sections.some((s) => s.text === sectionName)) {
- return;
- }
+ const addSection = async (sectionName: string) => {
+ const name = sectionName.trim();
+ if (!name) return;
+ if (sections.some((s) => s.text.toLowerCase() === name.toLowerCase())) {
+ return;
+ }
try {
await createBlock({
parentUid: personalSection.uid,
order: "last",
- node: { text: sectionName },
+ node: { text: name },
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const addSection = async (sectionName: string) => { | |
| if (!sectionName || sections.some((s) => s.text === sectionName)) { | |
| return; | |
| } | |
| try { | |
| await createBlock({ | |
| parentUid: personalSection.uid, | |
| order: "last", | |
| node: { text: sectionName }, | |
| }); | |
| refreshSections(); | |
| setNewSectionInput(""); | |
| setAutocompleteKey((prev) => prev + 1); | |
| } catch (error) { | |
| console.error("Failed to add section:", error); | |
| } | |
| }; | |
| const addSection = async (sectionName: string) => { | |
| const name = sectionName.trim(); | |
| if (!name) return; | |
| if (sections.some((s) => s.text.toLowerCase() === name.toLowerCase())) { | |
| return; | |
| } | |
| try { | |
| await createBlock({ | |
| parentUid: personalSection.uid, | |
| order: "last", | |
| node: { text: name }, | |
| }); | |
| refreshSections(); | |
| setNewSectionInput(""); | |
| setAutocompleteKey((prev) => prev + 1); | |
| } catch (error) { | |
| console.error("Failed to add section:", error); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In apps/roam/src/components/settings/LeftSidebar.tsx around lines 254 to 271,
the addSection handler should reject whitespace-only input and avoid duplicates
that differ only by leading/trailing spaces or case; fix by trimming the
incoming sectionName into a const (e.g., const name = sectionName.trim()),
return early if name === "" or if sections.some(s => s.text.trim().toLowerCase()
=== name.toLowerCase()), use the trimmed name when creating the block node ({
text: name }), and keep the existing refreshSections(), setNewSectionInput(""),
and autocomplete key update inside the try/catch.
| } else { | ||
| const hasGlobalSection = sidebarNode.children?.some( | ||
| (n) => n.text === "Global Section", | ||
| ); | ||
| const hasPersonalSection = sidebarNode.children?.some( | ||
| (n) => n.text === userName + "/Personal Section", | ||
| ); | ||
|
|
||
| if (!hasGlobalSection) { | ||
| await createBlock({ | ||
| parentUid: sidebarNode.uid, | ||
| order: 0, | ||
| node: { | ||
| text: "Global Section", | ||
| children: [ | ||
| { text: "Open", children: [{ text: "false" }] }, | ||
| { text: "Children" }, | ||
| ], | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| if (!hasPersonalSection) { | ||
| await createBlock({ | ||
| parentUid: sidebarNode.uid, | ||
| node: { text: userName + "/Personal Section" }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure Global Section’s Open/Children even when Global exists
Currently, if “Global Section” exists but is missing “Open” and/or “Children,” they remain absent. Make the ensure step idempotent and complete.
} else {
- const hasGlobalSection = sidebarNode.children?.some(
- (n) => n.text === "Global Section",
- );
+ const hasGlobalSection = sidebarNode.children?.some(
+ (n) => n.text === "Global Section",
+ );
const hasPersonalSection = sidebarNode.children?.some(
(n) => n.text === userName + "/Personal Section",
);
if (!hasGlobalSection) {
await createBlock({
parentUid: sidebarNode.uid,
order: 0,
node: {
text: "Global Section",
children: [
{ text: "Open", children: [{ text: "false" }] },
{ text: "Children" },
],
},
});
}
+ // Re-read to get the latest children and ensure nested nodes exist
+ const refreshed = getBasicTreeByParentUid(configPageUid).find(
+ (n) => n.text === "Left Sidebar",
+ );
+ const globalNode = refreshed?.children?.find((n) => n.text === "Global Section");
+ if (globalNode) {
+ const hasOpen = globalNode.children?.some((c) => c.text === "Open");
+ const hasChildren = globalNode.children?.some((c) => c.text === "Children");
+ if (!hasOpen) {
+ await createBlock({
+ parentUid: globalNode.uid,
+ order: 0,
+ node: { text: "Open", children: [{ text: "false" }] },
+ });
+ }
+ if (!hasChildren) {
+ await createBlock({
+ parentUid: globalNode.uid,
+ order: 1,
+ node: { text: "Children" },
+ });
+ }
+ }
+
if (!hasPersonalSection) {
await createBlock({
parentUid: sidebarNode.uid,
node: { text: userName + "/Personal Section" },
});
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else { | |
| const hasGlobalSection = sidebarNode.children?.some( | |
| (n) => n.text === "Global Section", | |
| ); | |
| const hasPersonalSection = sidebarNode.children?.some( | |
| (n) => n.text === userName + "/Personal Section", | |
| ); | |
| if (!hasGlobalSection) { | |
| await createBlock({ | |
| parentUid: sidebarNode.uid, | |
| order: 0, | |
| node: { | |
| text: "Global Section", | |
| children: [ | |
| { text: "Open", children: [{ text: "false" }] }, | |
| { text: "Children" }, | |
| ], | |
| }, | |
| }); | |
| } | |
| if (!hasPersonalSection) { | |
| await createBlock({ | |
| parentUid: sidebarNode.uid, | |
| node: { text: userName + "/Personal Section" }, | |
| }); | |
| } | |
| } else { | |
| const hasGlobalSection = sidebarNode.children?.some( | |
| (n) => n.text === "Global Section", | |
| ); | |
| const hasPersonalSection = sidebarNode.children?.some( | |
| (n) => n.text === userName + "/Personal Section", | |
| ); | |
| if (!hasGlobalSection) { | |
| await createBlock({ | |
| parentUid: sidebarNode.uid, | |
| order: 0, | |
| node: { | |
| text: "Global Section", | |
| children: [ | |
| { text: "Open", children: [{ text: "false" }] }, | |
| { text: "Children" }, | |
| ], | |
| }, | |
| }); | |
| } | |
| // Re-read to get the latest children and ensure nested nodes exist | |
| const refreshed = getBasicTreeByParentUid(configPageUid).find( | |
| (n) => n.text === "Left Sidebar", | |
| ); | |
| const globalNode = refreshed?.children?.find( | |
| (n) => n.text === "Global Section" | |
| ); | |
| if (globalNode) { | |
| const hasOpen = globalNode.children?.some((c) => c.text === "Open"); | |
| const hasChildren = globalNode.children?.some((c) => c.text === "Children"); | |
| if (!hasOpen) { | |
| await createBlock({ | |
| parentUid: globalNode.uid, | |
| order: 0, | |
| node: { text: "Open", children: [{ text: "false" }] }, | |
| }); | |
| } | |
| if (!hasChildren) { | |
| await createBlock({ | |
| parentUid: globalNode.uid, | |
| order: 1, | |
| node: { text: "Children" }, | |
| }); | |
| } | |
| } | |
| if (!hasPersonalSection) { | |
| await createBlock({ | |
| parentUid: sidebarNode.uid, | |
| node: { text: userName + "/Personal Section" }, | |
| }); | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/roam/src/utils/ensureLeftSidebarStructure.ts around lines 39 to 66, the
code only creates a new "Global Section" when missing but does not ensure that
an existing "Global Section" has the required "Open" (with a child "false") and
"Children" nodes; make the operation idempotent by locating the existing "Global
Section" node when present, examine its children for an "Open" child (and that
Open child has a child "false") and a "Children" child, and call createBlock to
add any missing child blocks with the correct parentUid set to the Global
Section node's uid and appropriate order/structure so that running the function
multiple times leaves no gaps.
| const personal = getLeftSidebarPersonalSectionConfig( | ||
| personalLeftSidebarNode, | ||
| ) || { | ||
| uid: "", | ||
| text: "", | ||
| isSimple: true, | ||
| sections: [], | ||
| }; |
There was a problem hiding this comment.
Fix type of personal default: remove extraneous fields
The fallback object includes properties not in the expected type ({ uid; sections }). This will fail excess property checks.
- const personal = getLeftSidebarPersonalSectionConfig(
- personalLeftSidebarNode,
- ) || {
- uid: "",
- text: "",
- isSimple: true,
- sections: [],
- };
+ const personal =
+ getLeftSidebarPersonalSectionConfig(personalLeftSidebarNode) || {
+ uid: "",
+ sections: [],
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const personal = getLeftSidebarPersonalSectionConfig( | |
| personalLeftSidebarNode, | |
| ) || { | |
| uid: "", | |
| text: "", | |
| isSimple: true, | |
| sections: [], | |
| }; | |
| const personal = | |
| getLeftSidebarPersonalSectionConfig(personalLeftSidebarNode) || { | |
| uid: "", | |
| sections: [], | |
| }; |
🤖 Prompt for AI Agents
In apps/roam/src/utils/ensureLeftSidebarStructure.ts around lines 106 to 113,
the fallback object for personal includes extraneous fields (text, isSimple) not
present on the expected type ({ uid; sections }), causing excess property check
errors; replace the fallback with an object matching the type (e.g., { uid: "",
sections: [] }) or adjust the function call to allow a broader type (or use a
safe type assertion) so the default value exactly matches the expected shape.
|
ngmi |
Summary by CodeRabbit
New Features
Improvements
For Reviewing:
The settings are going to be saved in
roam/js/discourse-graphpage, the structure is going to be as in following example:We extract the rendering data using the logic:
Left SidebarblockGlobal Sectionopencondition{username}/Personal SectionSettingsandChildrentab use the subgroup rendering otherwise normalOn a section header user can
shift+clickto open in sidebar, double click to open page directly,