ENG-1642 Discourse context overlay gets re-triggered every hover in a query block when view is render#989
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. |
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 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. 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 |
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
I think the memoization fix is treating the symptom rather than the root cause.
The overlay is being retriggered because hovering the query results currently updates React state in ResultsView (revealMenuIcons), which rerenders the results table. In render view, that means Roam’s BlockString output can be recreated, which causes the page-ref observer / discourse overlay path to run again.
Memoizing ResultsTable/ResultRow can suppress that rerender in this case, but it is a broad workaround around a hover-only UI concern. It also risks hiding future legitimate updates because the whole table/row rendering path becomes dependent on memo boundaries.
A narrower fix is to remove the hover state entirely and let CSS reveal the query controls on hover. That keeps the same UI behavior without causing React to rerender the query results table every time the mouse enters or leaves the query block.
I have shown that here:
#1006
https://www.loom.com/share/764c12c0caa948d2816c5f9c1e5ea74d
|
That makes sense. Since you did write the fix, do you want to take this over? |
- Removed state management for revealMenuIcons and associated mouse event handlers. - Introduced a new className for menu icons to control visibility based on interface settings and hover state. - Updated CSS to manage menu icon display logic, enhancing user experience by streamlining the interface. This change improves the clarity and maintainability of the ResultsView component.
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
mdroidian
deleted the
eng-1642-discourse-context-overlay-gets-re-triggered-every-hover-in-a
branch
… query block when view is render (#989) * ENG-1642 Memoize results table to avoid redraws * spurious includes * Refactor ResultsView component to simplify menu icon visibility (#1006) - Removed state management for revealMenuIcons and associated mouse event handlers. - Introduced a new className for menu icons to control visibility based on interface settings and hover state. - Updated CSS to manage menu icon display logic, enhancing user experience by streamlining the interface. This change improves the clarity and maintainability of the ResultsView component. * remove memo --------- Co-authored-by: Michael Gartner <mclicks@gmail.com>
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