Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughMemoization optimization applied to ChangesComponent Memoization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/roam/src/components/results-view/ResultsTable.tsx (1)
164-175: ⚡ Quick winAdd explicit return types to the new memoized component functions.
Both newly changed component callbacks are missing explicit return types. Please annotate them (e.g.,
: JSX.Element) to match repo TS rules.Suggested patch
+type CellRenderProps = { content: string; uid: string }; const CellRender = memo( - ({ content, uid }: { content: string; uid: string }) => { + ({ content, uid }: CellRenderProps): JSX.Element => { const isPage = !!getPageTitleByPageUid(uid); const displayString = isPage ? `[[${content}]]` : content; return ( <span className="roamjs-query-link-cell"> <RenderRoamBlockString string={displayString} /> </span> ); }, ); @@ const ResultRow = memo( ({ @@ - }: ResultRowProps) => { + }: ResultRowProps): JSX.Element => {As per coding guidelines, "Use explicit return types for functions in TypeScript".
Also applies to: 190-421
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/roam/src/components/results-view/ResultsTable.tsx` around lines 164 - 175, The memoized component callbacks are missing explicit return types; update the CellRender memo callback (and the other memoized callback in the same file) to annotate their function signatures with an explicit return type like ": JSX.Element" (e.g., change "({ content, uid }: { content: string; uid: string }) => { ... }" to "({ content, uid }: { content: string; uid: string }): JSX.Element => { ... }"); ensure React/JSX types are available in scope if needed and keep the memo wrapper (CellRender) unchanged otherwise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/roam/src/components/results-view/ResultsTable.tsx`:
- Around line 164-175: The memoized component callbacks are missing explicit
return types; update the CellRender memo callback (and the other memoized
callback in the same file) to annotate their function signatures with an
explicit return type like ": JSX.Element" (e.g., change "({ content, uid }: {
content: string; uid: string }) => { ... }" to "({ content, uid }: { content:
string; uid: string }): JSX.Element => { ... }"); ensure React/JSX types are
available in scope if needed and keep the memo wrapper (CellRender) unchanged
otherwise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 33b3ed15-5a98-4915-84b9-e9065866fd3a
📒 Files selected for processing (1)
apps/roam/src/components/results-view/ResultsTable.tsx
|
@mdroidian Just checking: I think you wanted me to look into why embeddings were not affected, correct? Or did you intend to do it as part of review? |
I can take a look. Memoizing |
2 participants
https://linear.app/discourse-graphs/issue/ENG-1642/discourse-context-overlay-gets-re-triggered-every-hover-in-a-query
https://www.loom.com/share/b8b7fcfe79334491946a95b2f0530103
Diagnostic: The RenderRoamBlockString inside CellRender seems to use useEffects to clear and re-render its contents. So when React asks CellRender to recreate the virtual DOM, in particular on every mouseEnter/mouseLeave to the resultsTable, the effects re-run, and the content is re-rendered from scratch. The solution provided by Claude was to apply React.memo on the CellRender,ResultRow,ResultTable stack, so these are not re-rendered when the props are the same. We had to make sure each prop was wrapped in either useMemo or useCallback.
Note: This induced a lot of whitespace changes. The only effective changes are introducing React.memo on those three components.
Summary by CodeRabbit