ENG-1115 - add command to toggle discourse context overlay from command #579
Conversation
…dler with getOverlayHandler and onPageRefObserverChange
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThe changes refactor the discourse context overlay initialization from a static observer-based approach to a dynamic handler registration system. The overlay component receives explicit icon sizing and padding adjustments. New utility functions enable applying and removing overlay handlers to existing page refs. A toggle command is added to the command palette for runtime control of the overlay feature. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 (3)
apps/roam/src/utils/registerCommandPaletteCommands.ts (1)
162-172: Consider handling potential command registration failures.All
addCommandcalls are wrapped withvoid, which discards any return values or errors. While this pattern is consistent, it means registration failures would be silent.If command registration can fail, consider wrapping in a try-catch or checking return values:
try { void addCommand("DG: Export - Current Page", exportCurrentPage); void addCommand("DG: Export - Discourse Graph", exportDiscourseGraph); void addCommand("DG: Open - Discourse Settings", renderSettingsPopup); void addCommand("DG: Open - Query Drawer", openQueryDrawerWithArgs); void addCommand( "DG: Toggle - Discourse Context Overlay", toggleDiscourseContextOverlay, ); void addCommand("DG: Query Block - Create", createQueryBlock); void addCommand("DG: Query Block - Refresh", refreshCurrentQueryBuilder); } catch (error) { console.error("Failed to register command palette commands:", error); }apps/roam/src/utils/pageRefObserverHandlers.ts (1)
134-142: Consider adding error handling when applying handlers to existing page refs.The function applies handlers to all existing page refs without error handling. If a handler throws, subsequent refs won't be processed.
Add try-catch to continue processing even if individual handlers fail:
const applyHandlersToExistingPageRefs = ( handler: (s: HTMLSpanElement) => void, ) => { const existingPageRefs = document.querySelectorAll<HTMLSpanElement>("span.rm-page-ref"); existingPageRefs.forEach((pageRef) => { - handler(pageRef); + try { + handler(pageRef); + } catch (error) { + console.error("Error applying handler to page ref:", error, pageRef); + } }); };apps/roam/src/components/DiscourseContextOverlay.tsx (1)
162-164: Prefer Tailwind classes for button styling.The inline styles for
minHeight,paddingTop, andpaddingBottomcould be replaced with Tailwind utility classes for consistency with the codebase guidelines.Apply this diff to use Tailwind classes:
<Button small id={id} className={`roamjs-discourse-context-overlay ${ loading ? "animate-pulse" : "" }`} - style={{ - minHeight: "initial", - paddingTop: ".25rem", - paddingBottom: ".25rem", - }} + className="min-h-0 py-1" minimal disabled={loading} >Note: Apply the same change to the wrapper Button at lines 213-224.
Also applies to: 217-220
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/roam/src/components/DiscourseContextOverlay.tsx(4 hunks)apps/roam/src/utils/initializeObserversAndListeners.ts(2 hunks)apps/roam/src/utils/pageRefObserverHandlers.ts(2 hunks)apps/roam/src/utils/registerCommandPaletteCommands.ts(2 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/pageRefObserverHandlers.tsapps/roam/src/components/DiscourseContextOverlay.tsxapps/roam/src/utils/registerCommandPaletteCommands.tsapps/roam/src/utils/initializeObserversAndListeners.ts
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/pageRefObserverHandlers.tsapps/roam/src/components/DiscourseContextOverlay.tsxapps/roam/src/utils/registerCommandPaletteCommands.tsapps/roam/src/utils/initializeObserversAndListeners.ts
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/pageRefObserverHandlers.tsapps/roam/src/components/DiscourseContextOverlay.tsxapps/roam/src/utils/registerCommandPaletteCommands.tsapps/roam/src/utils/initializeObserversAndListeners.ts
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/pageRefObserverHandlers.tsapps/roam/src/components/DiscourseContextOverlay.tsxapps/roam/src/utils/registerCommandPaletteCommands.tsapps/roam/src/utils/initializeObserversAndListeners.ts
apps/roam/**
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Implement the Discourse Graph protocol in the Roam Research extension
Files:
apps/roam/src/utils/pageRefObserverHandlers.tsapps/roam/src/components/DiscourseContextOverlay.tsxapps/roam/src/utils/registerCommandPaletteCommands.tsapps/roam/src/utils/initializeObserversAndListeners.ts
🧠 Learnings (3)
📚 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/utils/pageRefObserverHandlers.tsapps/roam/src/utils/initializeObserversAndListeners.ts
📚 Learning: 2025-08-25T15:53:21.799Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 372
File: apps/roam/src/components/DiscourseNodeMenu.tsx:116-116
Timestamp: 2025-08-25T15:53:21.799Z
Learning: In apps/roam/src/components/DiscourseNodeMenu.tsx, when handling tag insertion, multiple leading hashtags (like ##foo) should be preserved as they represent user intent, not normalized to a single hashtag. The current regex /^#/ is correct as it only removes one leading # before adding one back, maintaining any additional hashtags the user intended.
Applied to files:
apps/roam/src/utils/pageRefObserverHandlers.ts
📚 Learning: 2025-11-25T00:52:36.174Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/obsidian.mdc:0-0
Timestamp: 2025-11-25T00:52:36.174Z
Learning: Applies to apps/obsidian/**/*.md : Surround icons with parenthesis in documentation and display contexts, formatted as ( ![[icon-name.svg#icon]] )
Applied to files:
apps/roam/src/components/DiscourseContextOverlay.tsx
🧬 Code graph analysis (2)
apps/roam/src/utils/registerCommandPaletteCommands.ts (1)
apps/roam/src/utils/pageRefObserverHandlers.ts (2)
getOverlayHandler(16-19)onPageRefObserverChange(179-194)
apps/roam/src/utils/initializeObserversAndListeners.ts (1)
apps/roam/src/utils/pageRefObserverHandlers.ts (2)
getOverlayHandler(16-19)onPageRefObserverChange(179-194)
🔇 Additional comments (2)
apps/roam/src/utils/initializeObserversAndListeners.ts (1)
189-191: LGTM! Clean refactor to dynamic handler approach.The change from
addPageRefObserver(overlayPageRefHandler)to the newgetOverlayHandlerandonPageRefObserverChangepattern is well-structured. This enables centralized handler management and automatically applies the overlay to existing page refs when enabled.apps/roam/src/components/DiscourseContextOverlay.tsx (1)
96-96: LGTM! ICON_SIZE constant centralizes icon sizing.The new
ICON_SIZEconstant ensures consistent icon sizing across the overlay button and wrapper states, improving maintainability.Verify that the 16px size adequately addresses the layout shift issue mentioned in FEE-624. You may need to test with different line heights and font sizes to confirm no vertical shift occurs when the overlay is added/removed.
…r setting changes and improve toast message formatting.
mdroidian
deleted the
eng-1115-add-command-to-toggle-discourse-context-overlay-from-command
branch
https://www.loom.com/share/4b185d42af5147a785e9139b44139df9
Suggestion to reduce the icon/font size to remove line layout shift: FEE-624: Discourse context overlay / suggestive mode icon causes layout shift
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style
✏️ Tip: You can customize this high-level summary in your review settings.