Finish integrating Discourse Graph mode #38
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
WalkthroughThis pull request introduces updates to the Roam application's discourse graph functionality. Key changes include the removal of the Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Observers as Observers
participant Nodes as Discourse Nodes
participant Config as Configuration
App->>Observers: Initialize Observers
Observers-->>App: Return Observers and Listeners
App->>Nodes: Initialize Discourse Nodes
Nodes-->>App: Create/Update Nodes
App->>Config: Refresh Configuration Tree
Config-->>App: Update Settings
Possibly related PRs
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
apps/roam/src/utils/registerSmartBlock.ts (1)
Line range hint
10-91: Typo check: Ensure consistent naming of the process function.
Observe that you're usingproccessBlockTextwith a double “c”. Confirm if the declared function name matches throughout to avoid undefined references or confusion.- handler: ({ proccessBlockText, variables, processBlock }) => + handler: ({ processBlockText, variables, processBlock }) =>
🧹 Nitpick comments (10)
apps/roam/src/index.ts (2)
50-56: Leverage consistent ordering of initialization calls.
You’re initializing discourse nodes, refreshing configuration, adding styling, and then calling utility setup functions. This ordering is critical to avoid referencing uninitialized data. It might be helpful to document whyinitializeDiscourseNodes()is invoked beforerefreshConfigTree().
69-69: Document extension property usage.
AssigningrunQuery,listActiveQueries, andisDiscourseNodeontowindow.roamjs.extension.queryBuilderis convenient. For large teams, consider adding brief docstrings or comments for new team members.apps/roam/src/utils/initializeObserversAndListeners.ts (2)
56-66: Asynchronous callback inlinkedReferencesObserver.
Usingawaitin the observer callback is good for sequencing. However, ensure no user-facing delays or UI blocking occurs if rendering references is slow.
109-116: Config observer constructor is robust.
Creating a separate observer for the config page is helpful. Consider logging observer creation for easier debugging.apps/roam/src/utils/setQueryPages.ts (1)
3-18: Robust defaults for query-pages setting.
This function ensures "discourse-graph/queries/*" is included. Consider logging or alerting the user if a wide wildcard might cause clutter or confusion in some workflows.apps/roam/src/utils/graphViewNodeStyling.ts (1)
1-61: Functional approach to dynamic node styling looks clean.
- If the
prefixColorsarray grows large, you might want to consider more efficient string matching (e.g., a dictionary lookup) to prevent repeated startsWith checks.- Ensure that your fallback behavior (when no prefix matches) remains correct from a user experience perspective.
apps/roam/src/data/defaultDiscourseRelations.ts (2)
38-47: Ensure consistency and maintainability of coordinate definitions.
The newly added "Node Positions" entries for "Informs" look functionally correct. However, storing these positional values inline as text could make downstream usage brittle and more difficult to maintain. Consider externalizing them into a more structured configuration (e.g., a dedicated JSON or configuration object) to improve readability and maintainability.
138-151: Consider unifying or parameterizing repeated coordinates.
These positions for "Supports" partially mirror the approach in "Informs." If these coordinate lists are intended to stay in sync across multiple relation types, centralizing them or establishing a shared reference might reduce duplication and the risk of inconsistencies.apps/roam/src/settings/configPages.ts (1)
28-29: Review complexity of new config panel imports.
DiscourseNodeConfigPanelandDiscourseRelationConfigPanelcan introduce a deeper layer of customization. Ensure these panel components are thoroughly tested, since additional config panels often multiply dynamic states.apps/roam/src/utils/predefinedSelections.ts (1)
528-543: Use a consistent naming convention and logic for node types.
Handling^\s*type\s*$/iallows dynamic identification of a block’s discourse node type. If more node types are introduced in the future, consider adopting an expandable or data-driven mechanism for these checks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
apps/roam/package.json(1 hunks)apps/roam/src/components/DiscourseContext.tsx(1 hunks)apps/roam/src/components/LivePreview.tsx(0 hunks)apps/roam/src/components/ReferenceContext.tsx(0 hunks)apps/roam/src/components/canvas/LabelDialog.tsx(1 hunks)apps/roam/src/components/index.ts(0 hunks)apps/roam/src/data/defaultDiscourseRelations.ts(3 hunks)apps/roam/src/discourseGraphsMode.ts(0 hunks)apps/roam/src/index.ts(3 hunks)apps/roam/src/settings/configPages.ts(3 hunks)apps/roam/src/utils/getPlainTitleFromSpecification.ts(1 hunks)apps/roam/src/utils/graphViewNodeStyling.ts(1 hunks)apps/roam/src/utils/initializeDiscourseNodes.ts(1 hunks)apps/roam/src/utils/initializeObserversAndListeners.ts(1 hunks)apps/roam/src/utils/predefinedSelections.ts(2 hunks)apps/roam/src/utils/refreshConfigTree.ts(1 hunks)apps/roam/src/utils/registerSmartBlock.ts(2 hunks)apps/roam/src/utils/renderLinkedReferenceAdditions.ts(1 hunks)apps/roam/src/utils/setQueryPages.ts(1 hunks)
💤 Files with no reviewable changes (4)
- apps/roam/src/components/index.ts
- apps/roam/src/components/LivePreview.tsx
- apps/roam/src/discourseGraphsMode.ts
- apps/roam/src/components/ReferenceContext.tsx
✅ Files skipped from review due to trivial changes (2)
- apps/roam/src/components/canvas/LabelDialog.tsx
- apps/roam/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
apps/roam/src/utils/initializeObserversAndListeners.ts
[error] 107-107: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
🔇 Additional comments (24)
apps/roam/src/index.ts (6)
23-26: Modular approach looks good.
Extracting observer logic into initObservers and styling into addGraphViewNodeStyling fosters clarity and maintainability.
58-60: Local variable naming is clear.
Using style, dgraphStyles, and settingsStyle is easy to follow and consistent, reflecting the differing style sheets being applied.
62-67: Observer and listener management is well-encapsulated.
Storing the arrays of observers/listeners and returning them for cleanup is a robust approach. Great job centralizing the teardown logic.
84-84: Returning style elements with the extension object is a neat pattern.
This effectively references them for teardown. Keep this approach consistent across other extensions for uniformity.
94-98: Cleanup logic is thorough.
Removing the event listeners and graph styling callback prevents memory leaks. This is an excellent practice.
1-16: Consider verifying imports for unused references.
Most of these imports (e.g., parseQuery, fireQuerySync, etc.) appear to be actively used, but the large number of separate utility imports warrants an occasional verification that each import is still required. This helps maintain a tidy codebase.
✅ Verification successful
Let me verify the usage of all imports in the index.ts file to ensure they are actively used.
All imports in index.ts are actively used
Based on the search results, all imports in the index.ts file are actively used within the codebase:
addStyleis used to add the three style imports (styles, discourseGraphStyles, settingsStyles)renderToastis used for displaying various toast notificationsrunExtensionis used as the main wrapper for the extensionQueryBuilderLoadedToastis used in the toast notification for when query builder is loadedrunQueryis used in the exported API and by multiple componentsisDiscourseNodeis used in DiscourseContext and ExportDiscourseContext componentsfireQuerySyncandparseQueryare used together in the runQuerySync implementationrefreshConfigTreeis called during initialization and used across multiple settings components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find references to imported utilities in src/
rg -A 2 "from .*/utils/(runQuery|parseQuery|fireQuerySync|isDiscourseNode)"
Length of output: 3038
Script:
#!/bin/bash
# Search for usage of remaining imports from index.ts
rg "addStyle|renderToast|runExtension|QueryBuilderLoadedToast|runQuery|refreshConfigTree" -A 2
Length of output: 16352
apps/roam/src/utils/initializeObserversAndListeners.ts (8)
34-41: Function signature is clear; returning observers/listeners fosters easy teardown.
This pattern helps reduce side effects or the need for extra global variables.
42-54: Observer callback boundary checks.
Ensure that each callback (e.g., renderNodeConfigPage) gracefully handles any unexpected states if the title fails to meet the page criteria.
68-71: Refined approach to button-based rendering.
Using query-block attributes centralizes queries. Keep an eye on collisions with other attribute-based features from external libraries.
73-92: Event-driven block creation.
The pageActionListener approach is clean and well-encapsulated. If blocks can be added in quick succession, consider concurrency or rate-limiting.
94-101: Graph overview export observer.
The callback pattern looks consistent, but be mindful of performance if rendering large data sets.
117-127: hashChangeListener approach is well thought out.
Refreshing config upon leaving the page is a good pattern. If partial config updates are desired, you could consider finer-grained checks.
129-148: nodeMenuTriggerListener is intuitive.
Capturing a specific key for menu triggers is a neat pattern. Ensure the key is user-customizable or documented.
150-164: Final return structure.
Returning arrays of observers/listeners keeps them well-scoped. This improves modularity and maintainability.
apps/roam/src/utils/getPlainTitleFromSpecification.ts (1)
3-24: String transformations for node titles.
Your chained regex replacements are concise and effective. Just be sure that these transformations can handle unusual or invalid user inputs (e.g., what if target is unexpectedly missing?).
apps/roam/src/utils/initializeDiscourseNodes.ts (1)
6-8: Conditional node creation logic is succinct.
Filtering out non-default nodes prior to creation prevents duplication. Consider logging or returning the newly created nodes for debugging or further usage.
apps/roam/src/utils/refreshConfigTree.ts (1)
39-39: Consider verifying if any callers relied on a return value.
Dropping the return statement (previously returning a value) may affect consumers that expect a returned object or promise. If none exist, this change is safe. Otherwise, ensure dependent code segments have been updated accordingly.
apps/roam/src/utils/renderLinkedReferenceAdditions.ts (1)
1-51: Great addition for conditionally rendering discourse-related components!
This new utility function cleanly checks for the discourse context, ensuring idempotent rendering. No major logical or structural issues are apparent. As a minor suggestion, consider guarding against multiple calls in quick succession if performance becomes a concern.
apps/roam/src/data/defaultDiscourseRelations.ts (1)
242-255: Preserve clarity when adding coordinates for 'Opposes.'
Similarly, these newly added node positions for "Opposes" adhere to the same pattern. Ensure consistent usage across your codebase, and consider standardizing these values for easier future adjustments.
apps/roam/src/settings/configPages.ts (2)
7-8: Validate imports for new ConfigPanels.
The additions of MultiTextPanel and NumberPanel look good. Confirm that these components are available in the targeted runtime environment and verify that they align with the design expectations for multi-text and numeric inputs.
30-35: Check the integration of DEFAULT_RELATION_VALUES and observer handlers.
The direct import and usage of DEFAULT_RELATION_VALUES and pageRefObserverHandlers are appropriate if the config page truly needs them. However, double-check that these references reflect the consolidation of discourse graph logic from removed or refactored files.
apps/roam/src/components/DiscourseContext.tsx (1)
24-29: Imports enhance functionality without disrupting existing flow.
The new imports (getPageTitleValueByHtmlElement, getPageUidByPageTitle, renderWithUnmount, isDiscourseNode, CanvasReferences, and OnloadArgs) appear consistent with the added or extended discourse context usage. They don’t introduce immediate red flags, and the integration seems straightforward.
apps/roam/src/utils/predefinedSelections.ts (2)
17-19: New imports align with discourse node handling.
deriveNodeAttribute, matchDiscourseNode, and getDiscourseNodes provide specialized functionality for discourse node processing. Ensure that each of these utilities is appropriately tested and validated for edge cases (e.g., partial matches, invalid node structures).
519-527: Leverage ^discourse:(.*)$ test to handle node attributes.
This addition neatly extends the query capability for discourse attributes. Since these attributes can vary widely, make sure you have robust fallback behavior for unexpected or missing attributes in deriveNodeAttribute.
| if (isFlagEnabled("preview")) addPageRefObserver(previewPageRefHandler); | ||
| // if (isFlagEnabled("grammar.overlay")) { | ||
| // addPageRefObserver((s) => overlayPageRefHandler(s, onloadArgs)); | ||
| // } | ||
| if (!!getPageRefObserversSize()) enablePageRefObserver(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant double-negation.
Double-negation is unnecessary here.
- if (!!getPageRefObserversSize()) enablePageRefObserver();
+ if (getPageRefObserversSize()) enablePageRefObserver();📝 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.
| if (isFlagEnabled("preview")) addPageRefObserver(previewPageRefHandler); | |
| // if (isFlagEnabled("grammar.overlay")) { | |
| // addPageRefObserver((s) => overlayPageRefHandler(s, onloadArgs)); | |
| // } | |
| if (!!getPageRefObserversSize()) enablePageRefObserver(); | |
| if (isFlagEnabled("preview")) addPageRefObserver(previewPageRefHandler); | |
| // if (isFlagEnabled("grammar.overlay")) { | |
| // addPageRefObserver((s) => overlayPageRefHandler(s, onloadArgs)); | |
| // } | |
| if (getPageRefObserversSize()) enablePageRefObserver(); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 107-107: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
There was a problem hiding this comment.
Wouldn't the double negative function more like a comment of intent making it quick to groq? As the function name isn't explicit (ie: hasPageRefObservers)?
There was a problem hiding this comment.
@mdroidian, you raise a valid point that using !! can highlight the fact that the result should be treated as a boolean, especially if the function name doesn’t make it immediately obvious. However, many style guides recommend removing double negation to keep expressions simpler and more readable, instead favoring clear naming or explicit checks. For instance, renaming the function to something like hasPageRefObservers() can make the intent more explicit without relying on !!.
(=^‥^=)ノ・・・(Curious rabbit pondering the code style)
| await Promise.all( | ||
| INITIAL_NODE_VALUES.map( | ||
| (n) => | ||
| getPageUidByPageTitle(`discourse-graph/nodes/${n.text}`) || | ||
| createPage({ | ||
| title: `discourse-graph/nodes/${n.text}`, | ||
| uid: n.type, | ||
| tree: [ | ||
| { text: "Format", children: [{ text: n.format || "" }] }, | ||
| { text: "Shortcut", children: [{ text: n.shortcut || "" }] }, | ||
| { text: "Graph Overview" }, | ||
| { | ||
| text: "Canvas", | ||
| children: [ | ||
| { | ||
| text: "color", | ||
| children: [{ text: n.canvasSettings?.color || "" }], | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }), | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Batch-created pages with minimal error handling.
If createPage fails (e.g., network or API failure), it might partially create some nodes. Consider wrapping in try/catch to handle partial successes or reattempt.
try {
await Promise.all(
INITIAL_NODE_VALUES.map(/* ... */)
);
} catch (e) {
console.error("Error creating default discourse nodes", e);
}Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| export const configPageTabs = (args: OnloadArgs): ConfigTab[] => [ | ||
| { | ||
| id: "home", | ||
| fields: [ | ||
| { | ||
| title: "trigger", | ||
| description: | ||
| "The trigger to create the node menu. Must refresh after editing", | ||
| defaultValue: "\\", | ||
| // @ts-ignore | ||
| Panel: TextPanel, | ||
| }, | ||
| // @ts-ignore | ||
| { | ||
| title: "disable sidebar open", | ||
| description: "Disable opening new nodes in the sidebar when created", | ||
| Panel: FlagPanel, | ||
| } as Field<FlagField>, | ||
| // @ts-ignore | ||
| { | ||
| title: "preview", | ||
| description: | ||
| "Whether or not to display page previews when hovering over page refs", | ||
| Panel: FlagPanel, | ||
| options: { | ||
| onChange: onPageRefObserverChange(previewPageRefHandler), | ||
| }, | ||
| } as Field<FlagField>, | ||
| ], | ||
| }, | ||
| { | ||
| id: "grammar", | ||
| fields: [ | ||
| // @ts-ignore | ||
| { | ||
| title: "nodes", | ||
| Panel: CustomPanel, | ||
| description: "The types of nodes in your discourse graph", | ||
| options: { | ||
| component: DiscourseNodeConfigPanel, | ||
| }, | ||
| } as Field<CustomField>, | ||
| // @ts-ignore | ||
| { | ||
| title: "relations", | ||
| Panel: CustomPanel, | ||
| description: "The types of relations in your discourse graph", | ||
| defaultValue: DEFAULT_RELATION_VALUES, | ||
| options: { | ||
| component: DiscourseRelationConfigPanel, | ||
| }, | ||
| } as Field<CustomField>, | ||
| // @ts-ignore | ||
| { | ||
| title: "overlay", | ||
| Panel: FlagPanel, | ||
| // description: | ||
| // "Whether to overlay discourse context information over node references", | ||
| description: "Currently disabled. Being reworked.", | ||
| disabled: true, | ||
| options: { | ||
| onChange: (val) => { | ||
| onPageRefObserverChange((s) => overlayPageRefHandler(s, args))(val); | ||
| }, | ||
| }, | ||
| } as Field<FlagField>, | ||
| ], | ||
| }, | ||
| { | ||
| id: "export", | ||
| fields: [ | ||
| { | ||
| title: "max filename length", | ||
| // @ts-ignore | ||
| Panel: NumberPanel, | ||
| description: "Set the maximum name length for markdown file exports", | ||
| defaultValue: 64, | ||
| }, | ||
| { | ||
| title: "remove special characters", | ||
| // @ts-ignore | ||
| Panel: FlagPanel, | ||
| description: | ||
| "Whether or not to remove the special characters in a file name", | ||
| }, | ||
| { | ||
| title: "simplified filename", | ||
| // @ts-ignore | ||
| Panel: FlagPanel, | ||
| description: | ||
| "For discourse nodes, extract out the {content} from the page name to become the file name", | ||
| }, | ||
| { | ||
| title: "frontmatter", | ||
| // @ts-ignore | ||
| Panel: MultiTextPanel, | ||
| description: | ||
| "Specify all the lines that should go to the Frontmatter of the markdown file", | ||
| }, | ||
| { | ||
| title: "resolve block references", | ||
| // @ts-ignore | ||
| Panel: FlagPanel, | ||
| description: | ||
| "Replaces block references in the markdown content with the block's content", | ||
| }, | ||
| { | ||
| title: "resolve block embeds", | ||
| // @ts-ignore | ||
| Panel: FlagPanel, | ||
| description: | ||
| "Replaces block embeds in the markdown content with the block's content tree", | ||
| }, | ||
| // @ts-ignore | ||
| { | ||
| title: "link type", | ||
| Panel: SelectPanel, | ||
| description: "How to format links that appear in your export.", | ||
| options: { | ||
| items: ["alias", "wikilinks", "roam url"], | ||
| }, | ||
| } as Field<SelectField>, | ||
| { | ||
| title: "append referenced node", | ||
| // @ts-ignore | ||
| Panel: FlagPanel, | ||
| description: | ||
| "If a referenced node is defined in a node's format, it will be appended to the discourse context", | ||
| }, | ||
| ], | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting large config tab definitions into separate modules.
This sizeable block of tab definitions for the "home," "grammar," and "export" tabs demonstrates a comprehensive set of configuration fields. However, the approach leads to a long function body that can hamper readability and future maintenance. Extracting each tab or group of fields into its own module or function may be beneficial, avoiding an overly large monolith.
There was a problem hiding this comment.
I like your style
There was a problem hiding this comment.
@mdroidian Thanks for the kind words! Let me know if there’s anything else you’d like me to address or clarify.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/roam/src/utils/initializeObserversAndListeners.ts (1)
103-105: Remove commented code.If the grammar overlay feature is no longer needed, this commented code block should be removed. If it's still needed, consider tracking it with a TODO comment and an issue.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/roam/src/utils/initializeObserversAndListeners.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/roam/src/utils/initializeObserversAndListeners.ts
[error] 106-106: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
🔇 Additional comments (5)
apps/roam/src/utils/initializeObserversAndListeners.ts (5)
34-41: LGTM! Well-structured function signature.
The function signature is well-defined with proper TypeScript types and clear return type definition.
42-70: LGTM! Well-implemented observer pattern.
The observers are well-structured and handle different UI elements appropriately:
- Page title observer handles different page types (node config, query, canvas)
- Linked references observer handles reference additions asynchronously
- Query block observer handles rendering of query blocks
106-106: Remove redundant double-negation.
Double-negation is unnecessary here.
- if (!!getPageRefObserversSize()) enablePageRefObserver();
+ if (getPageRefObserversSize()) enablePageRefObserver();🧰 Tools
🪛 Biome (1.9.4)
[error] 106-106: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
149-162: LGTM! Clean return statement.
The return statement properly filters out null observers and includes all defined listeners.
56-56: Track TODO comment with an issue.
The TODO comment references issue #39 for implementing roam query functionality.
Let's verify if the issue is still open:
✅ Verification successful
TODO comment correctly references an open issue #39
The TODO comment in the code references issue #39 which is confirmed to be open with the title "Optimize page load observers". This indicates proper tracking of the implementation task.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if issue #39 is still open
gh issue view 39 --json state,title
Length of output: 93
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/roam/src/utils/initializeObserversAndListeners.ts (5)
42-54: Consider refactoring the page title observer using a strategy pattern.The current if-else chain could become harder to maintain as more page types are added. Consider refactoring to use a strategy pattern or lookup table for better maintainability.
+ const PAGE_RENDERERS = { + isNodeConfigPage: renderNodeConfigPage, + isQueryPage: renderQueryPage, + isCanvasPage: renderTldrawCanvas, + } as const; + const pageTitleObserver = createHTMLObserver({ tag: "H1", className: "rm-title-display", callback: (e) => { const h1 = e as HTMLHeadingElement; const title = getPageTitleValueByHtmlElement(h1); const props = { title, h1, onloadArgs }; - - if (isNodeConfigPage(title)) renderNodeConfigPage(props); - else if (isQueryPage(props)) renderQueryPage(props); - else if (isCanvasPage(props)) renderTldrawCanvas(props); + + for (const [check, renderer] of Object.entries(PAGE_RENDERERS)) { + if (globals[check](check === 'isNodeConfigPage' ? title : props)) { + renderer(props); + break; + } + } }, });
56-56: Track the TODO comment for roam query implementation.The TODO comment references GitHub issue #39 for roam query implementation.
Would you like me to help implement the roam query functionality or create a new GitHub issue to track this task?
103-106: Remove or document the commented code.The commented code for grammar overlay should either be removed if it's no longer needed or documented with a clear explanation of when it will be uncommented.
129-148: Document and extract the default trigger key.Consider extracting the default trigger key "\" as a named constant and documenting its purpose and configuration options.
+ const DEFAULT_NODE_MENU_TRIGGER = "\\"; + const configTree = getBasicTreeByParentUid(configPageUid); const trigger = getSettingValueFromTree({ tree: configTree, key: "trigger", - defaultValue: "\\", + defaultValue: DEFAULT_NODE_MENU_TRIGGER, }).trim();
150-163: Improve type safety with a type guard function.Consider extracting the observer filter into a type guard function for better type safety and reusability.
+ function isMutationObserver(observer: unknown): observer is MutationObserver { + return observer instanceof MutationObserver; + } + return { observers: [ pageTitleObserver, queryBlockObserver, configPageObserver, linkedReferencesObserver, graphOverviewExportObserver, - ].filter((o): o is MutationObserver => !!o), + ].filter(isMutationObserver), listeners: [ pageActionListener, hashChangeListener, nodeMenuTriggerListener, ], };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/roam/src/utils/initializeObserversAndListeners.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/roam/src/utils/initializeObserversAndListeners.ts
[error] 107-107: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
🔇 Additional comments (2)
apps/roam/src/utils/initializeObserversAndListeners.ts (2)
107-107: Double negation in condition.
Based on previous discussions, the double negation is intentionally used for clarity. However, consider renaming the function to make the intent more explicit (e.g., hasPageRefObservers).
🧰 Tools
🪛 Biome (1.9.4)
[error] 107-107: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
117-127: Verify sidebar navigation handling.
The comment indicates that config updates via sidebar navigation are not handled. This could lead to stale configurations.
Run the following script to find potential sidebar-related code that might need to be updated:
✅ Verification successful
Let me gather more information about the config refresh mechanism to better understand the issue.
Let me search for any alternative navigation or URL change handlers that might be relevant.
Based on the code analysis, I can now provide a final response:
Sidebar navigation issue is a known limitation by design
The current implementation only refreshes the config tree when navigating away from the config page via URL changes (using hashChangeListener). This is intentional as the sidebar navigation operates differently and doesn't trigger URL changes in the same way. The comment in the code serves as documentation of this known behavior rather than indicating a bug that needs fixing.
Key observations:
- The refresh mechanism is specifically tied to URL-based navigation through the
hashChangeListener - The comment about sidebar updates is a documentation of the current design, not a bug
- The config refresh is primarily meant for URL-based navigation away from config pages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for sidebar-related code and config handling
rg -A 5 'sidebar|side-bar|side bar' --type ts
Length of output: 3538
Script:
#!/bin/bash
# Search for config refresh related code
rg -A 5 'refreshConfigTree|configPageUid' --type ts
# Search for URL or navigation related handlers
ast-grep --pattern 'function $FUNC($ARGS) {
$$$
$URL.endsWith($$$)
$$$
}'
Length of output: 12598
Script:
#!/bin/bash
# Search for URL change or navigation handlers
rg -A 10 'navigation|URL|route' --type ts
# Look for sidebar-related event listeners
ast-grep --pattern 'addEventListener($EVENT, $HANDLER)'
Length of output: 5856
The goal of this query is delete
discourseGraphsMode.ts, which previously contained the Discourse Mode feature flag, and integrate it directly intoindex.tsor elsewhere.Summary by CodeRabbit
New Features
Bug Fixes
registerSmartBlockfunction.Documentation
Chores