ENG-1252 - Query builder link renderString #675
Conversation
Co-authored-by: mclicks <[email protected]>
|
Cursor Agent can help with this pull request. Just |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Co-authored-by: mclicks <[email protected]>
Co-authored-by: mclicks <[email protected]>
mdroidian
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughAdds a new CellRender component and integrates it into the results table rendering path, adds internal error handling around Roam API render calls, adjusts ResultHeader forwardRef signature and displayName, adds explicit voiding of promise-returning table mutations, and introduces a new "render" view option. Also bumps Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/roam/src/components/results-view/ResultsTable.tsx (1)
363-396: Add fallback for emptyviewValuein alias view.When a view is set to "alias" mode without a configured value,
viewValuewill be an empty string, resulting in an empty anchor link. Use a fallback to display cell content instead:Fallback for empty viewValue
) : view === "alias" ? ( <a className={"rm-page-ref"} data-link-title={getPageTitleByPageUid(uid) || ""} href={(r[`${key}-url`] as string) || getRoamUrl(uid)} onMouseDown={(e) => { if (e.shiftKey) { openBlockInSidebar(uid); e.preventDefault(); e.stopPropagation(); } else if (e.ctrlKey) { ctrlClick?.({ text: toCellValue({ value: val, uid }), uid, }); e.preventDefault(); e.stopPropagation(); } }} onClick={(e) => { if (e.shiftKey || e.ctrlKey) { e.preventDefault(); e.stopPropagation(); } }} onContextMenu={(e) => { if (e.ctrlKey) { e.preventDefault(); e.stopPropagation(); } }} > - {viewValue} + {viewValue || cell(key)} </a>
🧹 Nitpick comments (1)
apps/roam/src/components/results-view/ResultsTable.tsx (1)
197-205: Add cleanup to prevent potential memory leaks.The
useEffectdoesn't unmount or clear the rendered content when the component unmounts ordisplayStringchanges, which could lead to memory leaks or stale DOM nodes.♻️ Add cleanup function to useEffect
useEffect(() => { const el = contentRef.current; if (el && displayString) { window.roamAlphaAPI.ui.components.renderString({ el, string: displayString, }); } + return () => { + if (el) { + el.innerHTML = ''; + } + }; }, [displayString]);Note: Verify if
renderStringprovides its own cleanup mechanism. If it returns a cleanup function or ifroamAlphaAPI.ui.components.unmountNodeshould be used instead, adjust accordingly.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/roam/src/components/results-view/ResultsTable.tsx
🧰 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/results-view/ResultsTable.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/results-view/ResultsTable.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/results-view/ResultsTable.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/results-view/ResultsTable.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/results-view/ResultsTable.tsx
🧠 Learnings (1)
📚 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/results-view/ResultsTable.tsx
🪛 GitHub Actions: CI
apps/roam/src/components/results-view/ResultsTable.tsx
[error] 200-200: TS2339: Property 'renderString' does not exist on type '{ renderBlock: (args: { uid: string; el: HTMLElement; "zoom-path"?: boolean | undefined; }) => null; renderPage: (args: { uid: string; el: HTMLElement; "hide-mentions"?: boolean | undefined; }) => null; renderSearch: (args: { ...; }) => null; unmountNode: (args: { ...; }) => void; }'.
🔇 Additional comments (1)
apps/roam/src/components/results-view/ResultsTable.tsx (1)
186-208: TherenderStringAPI is available and properly typed in the current codebase. The CellLink component successfully useswindow.roamAlphaAPI.ui.components.renderString()without any type suppression comments, and the code compiles without errors. The recent commit "Fix: Render links correctly in ResultsTable" confirms this functionality is working as intended. No type errors or API availability issues are present.Likely an incorrect or invalid review comment.
… in ResultsTable component - Updated roamjs-components dependency in package.json and pnpm-lock.yaml. - Improved error handling for rendering functions in ResultsTable, including catch blocks for internal errors. - Refactored rendering logic to use promises for better error management.
…ering - Renamed CellLink to CellRender for clarity and updated error handling context. - Introduced a new CellLink component to handle link rendering with additional props for URL and control click functionality. - Updated VIEWS in ResultsView to include 'render' option for improved view management.
…k component - Removed the CellLink component and integrated its functionality directly into ResultRow for improved clarity. - Updated link rendering logic to handle both 'link' and 'alias' views more efficiently. - Simplified event handling for link interactions, enhancing overall performance.
|
@coderabbitai full review |
| }, | ||
| ref, | ||
| ) => { | ||
| ({ |
| }, | ||
| ); | ||
|
|
||
| ResultHeader.displayName = "ResultHeader"; |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/roam/src/components/results-view/ResultsTable.tsx (1)
78-172: Potential issue: forwardRef wrapper without ref usage.The
ResultHeadercomponent is wrapped inReact.forwardRefbut the inner function doesn't accept or use therefparameter. Either:
- Remove the
forwardRefwrapper if refs aren't needed, or- Add the
refparameter and forward it to the appropriate element (likely the<td>)♻️ Option 1: Remove forwardRef if not needed
-const ResultHeader = React.forwardRef< - Record<string, HTMLTableCellElement>, - { - c: Column; - allResults: Result[]; - activeSort: Sorts; - setActiveSort: (s: Sorts) => void; - filters: FilterData; - setFilters: (f: FilterData) => void; - initialFilter: Filters; - columnWidth?: string; - } ->( - ({ +const ResultHeader = ({ + c, + allResults, + activeSort, + setActiveSort, + filters, + setFilters, + initialFilter, + columnWidth, +}: { + c: Column; + allResults: Result[]; + activeSort: Sorts; + setActiveSort: (s: Sorts) => void; + filters: FilterData; + setFilters: (f: FilterData) => void; + initialFilter: Filters; + columnWidth?: string; +}) => { - c, - allResults, - activeSort, - setActiveSort, - filters, - setFilters, - initialFilter, - columnWidth, - }) => { // ... rest of component - }, -); +};Option 2: Add ref parameter and forward it
const ResultHeader = React.forwardRef< Record<string, HTMLTableCellElement>, { c: Column; allResults: Result[]; activeSort: Sorts; setActiveSort: (s: Sorts) => void; filters: FilterData; setFilters: (f: FilterData) => void; initialFilter: Filters; columnWidth?: string; } >( ({ c, allResults, activeSort, setActiveSort, filters, setFilters, initialFilter, columnWidth, - }) => { + }, ref) => { // ... filter logic ... return ( <td + ref={(el) => { + if (ref && el) { + if (typeof ref === 'function') { + ref({ ...ref as any, [c.key]: el }); + } else { + ref.current = { ...ref.current, [c.key]: el }; + } + } + }} style={{ cursor: "pointer", textTransform: "capitalize", width: columnWidth, }}
🧹 Nitpick comments (1)
apps/roam/src/components/results-view/ResultsTable.tsx (1)
704-733: Consider adding error handling for mutation operations.While adding
voidkeywords makes the intent to ignore promises explicit, these are mutation operations (deleteBlock,createBlock) where failures could lead to data inconsistencies between UI state and persisted data. Consider whether error handling should be added to notify users of failures.♻️ Proposed error handling
- filtersNode.children.forEach((c) => void deleteBlock(c.uid)); + await Promise.all( + filtersNode.children.map((c) => + deleteBlock(c.uid).catch((error) => { + internalError({ + error, + type: "Results Table: Delete Filter Block", + context: { uid: c.uid }, + }); + }) + ) + ); Object.entries(fs) .filter( ([, data]) => data.includes.values.size || data.excludes.values.size, ) .map(([column, data]) => ({ text: column, children: [ { text: "includes", children: Array.from(data.includes.values).map((text) => ({ text, })), }, { text: "excludes", children: Array.from(data.excludes.values).map((text) => ({ text, })), }, ], })) - .forEach( - (node, order) => - void createBlock({ - parentUid: filtersNode.uid, - node, - order, - }), - ); + .forEach((node, order) => { + createBlock({ + parentUid: filtersNode.uid, + node, + order, + }).catch((error) => { + internalError({ + error, + type: "Results Table: Create Filter Block", + context: { column: node.text }, + }); + }); + });
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
apps/roam/package.jsonapps/roam/src/components/results-view/ResultsTable.tsxapps/roam/src/components/results-view/ResultsView.tsx
🧰 Additional context used
📓 Path-based instructions (5)
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/package.jsonapps/roam/src/components/results-view/ResultsView.tsxapps/roam/src/components/results-view/ResultsTable.tsx
apps/roam/**
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Implement the Discourse Graph protocol in the Roam Research extension
Files:
apps/roam/package.jsonapps/roam/src/components/results-view/ResultsView.tsxapps/roam/src/components/results-view/ResultsTable.tsx
**/*.{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/results-view/ResultsView.tsxapps/roam/src/components/results-view/ResultsTable.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/results-view/ResultsView.tsxapps/roam/src/components/results-view/ResultsTable.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/results-view/ResultsView.tsxapps/roam/src/components/results-view/ResultsTable.tsx
🧠 Learnings (8)
📚 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/**/*.{js,ts,tsx,jsx,json} : Prefer existing dependencies from package.json when working on the Roam Research extension
Applied to files:
apps/roam/package.json
📚 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/package.jsonapps/roam/src/components/results-view/ResultsTable.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 Roam Depot/Extension API docs from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when implementing extension functionality
Applied to files:
apps/roam/package.json
📚 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/package.json
📚 Learning: 2025-08-11T19:09:58.252Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 337
File: apps/roam/src/components/DiscourseFloatingMenu.tsx:43-43
Timestamp: 2025-08-11T19:09:58.252Z
Learning: The roam subdirectory (apps/roam) is constrained to React 17 and cannot use React 18+ features like createRoot API. ReactDOM.render should be used instead of createRoot in this subdirectory.
Applied to files:
apps/roam/package.json
📚 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/** : Implement the Discourse Graph protocol in the Roam Research extension
Applied to files:
apps/roam/package.json
📚 Learning: 2025-12-13T00:42:41.524Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-12-13T00:42:41.524Z
Learning: The `internalError` utility in apps/roam/src/utils/internalError.ts handles string-to-error conversion internally. Callers can pass strings directly as the error parameter without needing to convert them to Error objects first.
Applied to files:
apps/roam/src/components/results-view/ResultsTable.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/results-view/ResultsTable.tsx
🔇 Additional comments (10)
apps/roam/src/components/results-view/ResultsView.tsx (1)
54-54: LGTM! New "render" view mode added.The addition of the "render" view mode follows the existing pattern and integrates cleanly with the view system. This enables the new rendering functionality introduced in ResultsTable.tsx.
apps/roam/src/components/results-view/ResultsTable.tsx (8)
28-28: LGTM! Internal error utility imported.The
internalErrorimport is correctly added to support the new error handling throughout the file.
39-67: LGTM! Robust error handling added.The error handling for Roam API
renderPageandrenderBlockcalls properly catches errors and logs them viainternalErrorwith appropriate context. This prevents unhandled promise rejections.
174-174: LGTM! DisplayName added for better debugging.Adding
displayNameto theResultHeadercomponent is good practice, especially for forwardRef components, as it improves visibility in React DevTools.
190-203: LGTM! Error handling added to CellEmbed.The error handling for the
renderBlockcall inCellEmbedfollows the same robust pattern used elsewhere, properly catching and logging errors with context.
211-241: LGTM! New CellRender component looks good.The new
CellRendercomponent correctly uses the RoamrenderStringAPI with proper error handling. The logic for detecting pages and wrapping content in[[]]is appropriate.
388-389: LGTM! More explicit type coercion.The change from template string to
String(val)is more explicit and maintains the same behavior for type coercion.
395-396: LGTM! Render mode integrated correctly.The new "render" view mode is properly integrated, using the
CellRendercomponent to render cell content via the Roam rendering API.
404-404: LGTM! Explicit void for promise result.Adding
voidbeforeopenBlockInSidebarcorrectly indicates that the promise result is intentionally ignored in this fire-and-forget operation.apps/roam/package.json (1)
71-71: Package version 0.86.3 is valid and free from known vulnerabilities.The
roamjs-componentsversion bump from 0.85.7 to 0.86.3 is a legitimate release available on the npm registry with no reported security advisories.
https://www.loom.com/share/d4f4970750e6493e9d0fc4ba8b265437
add new column view: render
uses
renderStringapiSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.