eng-1144: DiscourseContext UI disappears #602
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughPropagated an updated onRefresh signature that accepts an optional boolean Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
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
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/DiscourseContext.tsx (1)
337-370: forceRefresh cannot bypass result cache because ignoreCache is hard-coded to falseIn
ContextContent, the intent appears to be:
onRefreshdoes a normal (cached) load.forceRefreshtriggers a refresh that bypasses the cache via the newignoreCacheplumbing and/orupdateCount.However, the current implementation prevents that:
const [updateCount, setUpdateCount] = useState(0); const onRefresh = useCallback( (ignoreCache?: boolean) => { setRawQueryResults({}); getDiscourseContextResults({ uid, onResult: addLabels, ignoreCache: false, // <- argument is ignored; always cached }).finally(() => setLoading(false)); }, [uid, setRawQueryResults, setLoading, addLabels, updateCount], ); const forceRefresh = () => { setUpdateCount((prevCount) => prevCount + 1); onRefresh(true); };Issues:
ignoreCacheparameter is unused;getDiscourseContextResultsalways receivesignoreCache: false, soforceRefreshnever bypassesresultCache.updateCountis only in the dependency array; it’s never read, so it doesn’t influence behavior beyond changingonRefresh’s identity.delayedRefresh(and the toolbar Refresh button viaforceRefresh) therefore do not achieve the “hard refresh” behavior described in the PR.A minimal fix is to actually propagate
ignoreCacheand drop the unusedupdateCountindirection:- const [updateCount, setUpdateCount] = useState(0); - - // never set ignoreCache to true directly, - // as it may be negated by the useCallback cache. - // go through forceRefresh when needed - const onRefresh = useCallback( - (ignoreCache?: boolean) => { - setRawQueryResults({}); - getDiscourseContextResults({ - uid, - onResult: addLabels, - ignoreCache: false, - }).finally(() => setLoading(false)); - }, - [uid, setRawQueryResults, setLoading, addLabels, updateCount], - ); - - const forceRefresh = () => { - setUpdateCount((prevCount) => prevCount + 1); - onRefresh(true); - }; + // Standard refresh uses cached results; `forceRefresh` below opts out. + const onRefresh = useCallback( + (ignoreCache?: boolean) => { + setRawQueryResults({}); + getDiscourseContextResults({ + uid, + onResult: addLabels, + ignoreCache, + }).finally(() => setLoading(false)); + }, + [uid, addLabels], + ); + + const forceRefresh = () => { + onRefresh(true); + };
delayedRefreshwill then correctly do a hard refresh:const delayedRefresh = () => { window.setTimeout(forceRefresh, 250); };If you do want to retain
updateCountas part of the cache-busting strategy (e.g., by folding it into thecacheKeyinsidegetDiscourseContextResults), it needs to be incorporated into that key or into theignoreCachevalue — right now it has no effect.As-is, the new
forceRefreshAPI doesn’t actually bypass the context results cache, which undermines the main goal of this PR.Also applies to: 372-374
🧹 Nitpick comments (3)
apps/roam/src/utils/getDiscourseContextResults.ts (1)
14-26: ignoreCache boolean broadening looks correct and preserves cache semanticsUsing
ignoreCache?: booleanin bothBuildQueryConfigandgetDiscourseContextResultsis consistent with the!ignoreCachecheck inbuildQueryConfigand cleanly allows explicit cache bypass when needed, while keeping the default (undefined/false) behavior cached.If you want to make the default explicit, you could also default the param:
const getDiscourseContextResults = async ({ uid: targetUid, relations = getDiscourseRelations(), nodes = getDiscourseNodes(relations), ignoreCache = false, onResult, }: { /* ... */ }) => { const args = { ignoreCache }; // ... };Purely optional; current implementation is functionally sound.
Also applies to: 169-183
apps/roam/src/components/results-view/ResultsView.tsx (2)
231-243: ResultsView refresh API and forceRefresh fallback look goodThe updated props (
onRefresh: () => voidplus optionalforceRefresh?: () => void) and the refresh button logic that prefersforceRefresh()and falls back toonRefresh()align well with the new separation of “normal” vs. explicit refresh.If you want to shave a little inline logic, you could simplify the button handler to:
<Button icon="refresh" minimal onClick={() => (forceRefresh ?? onRefresh)()} />Functionally equivalent; just slightly tighter.
Also applies to: 274-286, 577-585
1367-1372: Kanban onQuery now calls plain onRefresh, which matches the new signatureChanging
onQueryto() => onRefresh()is consistent with the updatedonRefresh: () => voidtype and removes the now-defunct flag argument. Behavior-wise this means Kanban’s re-query uses the standard refresh path instead of any special background/flagged mode, which fits the new API.You could also simplify to
onQuery={onRefresh}since there’s no argument, but that’s purely stylistic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/roam/src/components/DiscourseContext.tsx(5 hunks)apps/roam/src/components/results-view/ResultsView.tsx(4 hunks)apps/roam/src/utils/getDiscourseContextResults.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/components/results-view/ResultsView.tsxapps/roam/src/utils/getDiscourseContextResults.tsapps/roam/src/components/DiscourseContext.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/ResultsView.tsxapps/roam/src/utils/getDiscourseContextResults.tsapps/roam/src/components/DiscourseContext.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/utils/getDiscourseContextResults.tsapps/roam/src/components/DiscourseContext.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/utils/getDiscourseContextResults.tsapps/roam/src/components/DiscourseContext.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/ResultsView.tsxapps/roam/src/utils/getDiscourseContextResults.tsapps/roam/src/components/DiscourseContext.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/DiscourseContext.tsx
🧬 Code graph analysis (1)
apps/roam/src/components/DiscourseContext.tsx (1)
apps/roam/src/components/CreateRelationDialog.tsx (1)
CreateRelationButton(360-383)
🔇 Additional comments (1)
apps/roam/src/components/DiscourseContext.tsx (1)
220-234: forceRefresh is correctly threaded through ContextTab and into ResultsViewThe addition of
forceRefresh: () => voidonContextTaband its propagation down to<ResultsView ... forceRefresh={forceRefresh} />matches the new ResultsView API and cleanly distinguishes external “hard” refreshes from standardonRefreshcalls.The use in the bottom
Tabs(<ContextTab ... onRefresh={onRefresh} forceRefresh={forceRefresh} />) also looks consistent with howContextContentdefines those two responsibilities.No changes needed here.
Also applies to: 269-281, 408-416
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
useCallback shouldn't be the issue here.
Let's try to go back and identify what line of code introduced this regression. From there, let's identify why that line of code is causing this behavior. Then, let's find the solution.
I do not believe introducing another refresh function is the correct solution for this problem.
| }).finally(() => setLoading(false)); | ||
| }, [uid, setRawQueryResults, setLoading, addLabels]); | ||
| // never set ignoreCache to true directly, | ||
| // as it may be negated by the useCallback cache. |
There was a problem hiding this comment.
useCallback does not “cache” results or arguments. It only memoizes the function reference (identity).
| ignoreCache, | ||
| }).finally(() => setLoading(false)); | ||
| }, | ||
| [uid, setRawQueryResults, setLoading, addLabels, updateCount], |
There was a problem hiding this comment.
React Hook useCallback has an unnecessary dependency: 'updateCount'. Either exclude it or remove the dependency array.eslintreact-hooks/exhaustive-deps
maparent
force-pushed
the
eng-1144-discourse-context-ui-disappears
branch
2 times, most recently
from
c225b5c to
b9ba045
Compare
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
maparent
changed the title
|
Ok, replaced the previous implementation by a different one. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/roam/src/components/DiscourseContext.tsx (1)
364-371: Refactor useEffect dependencies to avoid potential issues.The dependency array includes
loading(unused in the effect body) andonRefresh(which itself depends onaddLabels, creating a circular dependency chain). This could cause unnecessary re-runs.Consider restructuring:
useEffect(() => { if (!results) { onRefresh(false); } else { results.forEach(addLabels); setLoading(false); } - }, [onRefresh, results, setLoading, loading, addLabels]); + }, [results, addLabels]);Then ensure
onRefreshis called with the currentaddLabelsby wrapping the call:useEffect(() => { if (!results) { - onRefresh(false); + setRawQueryResults({}); + void getDiscourseContextResults({ + uid, + onResult: addLabels, + ignoreCache: false, + }).finally(() => setLoading(false)); } else { results.forEach(addLabels); setLoading(false); } - }, [results, addLabels]); + }, [results, addLabels, uid]);This eliminates the circular dependency while maintaining the same behavior.
apps/roam/src/components/results-view/ResultsTable.tsx (1)
177-177: Minor formatting change unrelated to PR objectives.The CellEmbed JSX was reformatted to a single line. While this is fine, it appears unrelated to the core PR objective of separating cache control in the
onRefreshfunction.Consider keeping formatting changes separate from functional changes in future PRs for clearer change tracking.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/roam/src/components/DiscourseContext.tsx(2 hunks)apps/roam/src/components/results-view/ResultsTable.tsx(3 hunks)apps/roam/src/components/results-view/ResultsView.tsx(3 hunks)apps/roam/src/utils/getDiscourseContextResults.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/roam/src/utils/getDiscourseContextResults.ts
🧰 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/DiscourseContext.tsxapps/roam/src/components/results-view/ResultsTable.tsxapps/roam/src/components/results-view/ResultsView.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/DiscourseContext.tsxapps/roam/src/components/results-view/ResultsTable.tsxapps/roam/src/components/results-view/ResultsView.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/DiscourseContext.tsxapps/roam/src/components/results-view/ResultsTable.tsxapps/roam/src/components/results-view/ResultsView.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/DiscourseContext.tsxapps/roam/src/components/results-view/ResultsTable.tsxapps/roam/src/components/results-view/ResultsView.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/DiscourseContext.tsxapps/roam/src/components/results-view/ResultsTable.tsxapps/roam/src/components/results-view/ResultsView.tsx
🧠 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/components/results-view/ResultsTable.tsxapps/roam/src/components/results-view/ResultsView.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,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/src/components/results-view/ResultsTable.tsxapps/roam/src/components/results-view/ResultsView.tsx
📚 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/src/components/results-view/ResultsView.tsx
🔇 Additional comments (7)
apps/roam/src/components/DiscourseContext.tsx (3)
231-231: LGTM! Prop signature correctly updated.The onRefresh prop type now accepts an optional
ignoreCacheboolean parameter, aligning with the PR's goal to enable cache control.
348-358: Verify the defaultignoreCache = trueparameter.The default parameter causes all calls without an explicit argument to bypass the cache. While line 366 explicitly passes
falsefor the initial load, user-triggered refreshes (lines 284, 361) will use the defaulttrue.Confirm this is the intended behavior: initial loads benefit from caching, while user actions always force fresh data.
Based on the PR objectives, this appears correct, but verification ensures the caching strategy aligns with the intended fix for excessive calls.
284-284: Past setTimeout issue resolved.The previous bug where
setTimeout(() => forceRefresh, 250)didn't invoke the function has been fixed. The current implementation correctly passesonRefreshdirectly tosetTimeout, which will execute it after the delay.Calling
onRefreshwithout arguments uses the defaultignoreCache = true, appropriately forcing a refresh after creating a relation.apps/roam/src/components/results-view/ResultsTable.tsx (1)
191-191: LGTM! Type signatures correctly updated.The
onRefreshprop types in bothResultRowPropsandResultsTableare now consistent with the updated signature across the codebase, enabling cache control via the optionalignoreCacheparameter.Also applies to: 394-394
apps/roam/src/components/results-view/ResultsView.tsx (3)
241-241: LGTM! Parameter renamed for clarity.The
loadInBackgroundparameter has been renamed toignoreCache, which more clearly expresses its purpose. This directly addresses the PR objective to remove the apparently unusedloadInBackgroundargument and replace it with explicit cache control semantics.
1364-1364: LGTM! Simplified to rely on default parameter.The call changed from
onRefresh(true)toonRefresh(), relying on the default parameter value. Since the default inContextContent.onRefreshisignoreCache = true, the behavior remains the same while the code is more concise.
41-41: The import pathroamjs-components/types/nativeis valid and follows the codebase's established pattern for native Roam types.The codebase uses both
roamjs-components/typesandroamjs-components/types/nativeimports. The/nativepath is consistently used for core native Roam types likeRoamBasicNode,OnloadArgs,PullBlock,DatalogClause, andInputTextNode. This change aligns with how similar types are imported elsewhere (e.g.,isFlagEnabled.ts,parseQuery.ts,parseResultSettings.ts).However, this import standardization appears unrelated to the PR's primary cache control objectives. Consider documenting this refactoring separately or consolidating it with other import path standardization changes across the codebase.
|
@mdroidian A bit puzzled by the coderabbit suggestion here. TBH, I don't understand why static functions have to be in the arguments of |
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
re: deps
React’s exhaustive-deps rule wants every reactive value used inside an effect — including functions — in the dependency array so React can re-run the effect with up-to-date values and avoid stale closures. ref
Taking a quick look, with the addition of ignoreCache, the useEffect/onRefresh could probably use a restructure. Some thought should be put into the different scenarios and what the ramifications are.
If this PR fixes the error and multiple scenarios in the UI have been tested (a loom video would really be great here), then this can probably be deffered.
| preventSavingSettings?: boolean; | ||
| onEdit?: () => void; | ||
| onDeleteQuery?: () => void; | ||
| onRefresh: (loadInBackground?: boolean) => void; |
There was a problem hiding this comment.
loadInBackground is 'used' in QueryBuilder.tsx. If we are changing this, should probably remove the loadInBackground reference.
There was a problem hiding this comment.
Ok, I missed that one, thanks. And I agree with the quotation marks around 'used'.
Two things I could do: Remove the loadInBackground reference, since it does so little; (and probably just not call setLoading, but also a decision);
OR add the parameter back, and have a signature with two parameters on the type everywhere.
Which do you prefer?
There was a problem hiding this comment.
Remove. But keep the same behavior (which is setLoading(true) I believe)
There was a problem hiding this comment.
(the linear comment did not carry, copying) Done. onRefresh is called twice, this time with true (hence setLoading(true) as you say, which amounts to load in background being statically false, not true as you say in the other comment.) and in the DiscourseContext.tsx:366 useEffect, where it used to be false. I think this was to avoid a "loading" flicker on first display. I think this flicker is very short, accurately represents processing, and not worth another parameter to suppress.
There was a problem hiding this comment.
Done. onRefresh is called twice, this time with true (hence setLoading(true) as you say, which amounts to load in background being statically false, not true as you say in the other comment.) and in the DiscourseContext.tsx:366 useEffect, where it used to be false. I think this was to avoid a "loading" flicker on first display. I think this flicker is very short, accurately represents processing, and not worth another parameter to suppress.
| data={allProcessedResults} | ||
| layout={layout} | ||
| onQuery={() => onRefresh(true)} | ||
| onQuery={() => onRefresh()} |
There was a problem hiding this comment.
Could you add some context as to why this change was made, please? Kanban quite seems unrelated to discourse context UI.
There was a problem hiding this comment.
That is because I removed the loadInBackground flag, so I removed the one call instance which seemed to be using it. So it depends on what we decide to do with that flag, above.
There was a problem hiding this comment.
ok, well "load in background" can be statically true , so this line can stay the way it is if we change setLoading(true) where loadInBackground was being used.
There was a problem hiding this comment.
statically false, but otherwise agreed.
maparent
force-pushed
the
eng-1144-discourse-context-ui-disappears
branch
from
4018ed7 to
7a44b37
Compare
Distinguish the onRefresh and forceRefresh function.
The cache bypass was not consistent, leading to too many calls.
loadInBackgroundargument in theonRefreshsignatureSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.