Eng 732 left side panel for suggestive mode #340
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughAdds a Suggestive Mode feature: UI overlays on page refs, a panel manager for collapsible Discourse Suggestions panels, Hyde-based async suggestion/search plumbing, config/settings UI, observer wiring, and highlight styling to coordinate overlays and suggestion panels. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant PageRefObs as PageRef Observer
participant Overlay as SuggestiveModeOverlay
participant PanelMgr as panelManager
participant Panel as DiscourseSuggestionsPanel
participant Body as SuggestionsBody
participant Hyde as performHydeSearch
User->>PageRefObs: page load / ref mutation
PageRefObs->>Overlay: renderSuggestive(tag,parent,onloadArgs)
Overlay->>PanelMgr: toggle({tag, blockUid, onloadArgs})
alt panel closed
PanelMgr->>Panel: create container element & mount
Panel->>Body: render(props)
else panel open
PanelMgr->>Panel: unmount & cleanup
end
User->>Body: Click "Find"
Body->>Hyde: performHydeSearch({discourseNode, blockUid, pageTitle, ...})
Hyde-->>Body: results
Body-->>User: render results & filters
sequenceDiagram
autonumber
participant Settings as HomePersonalSettings
participant Init as initializeObserversAndListeners
participant ObsHandlers as pageRefObserverHandlers
participant DOM as Page DOM
Settings->>Init: toggle "suggestive-mode-overlay" setting
Init->>ObsHandlers: addPageRefObserver(getSuggestiveOverlayHandler(onloadArgs))
ObsHandlers->>DOM: mark refs, mount Suggestive overlay
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Pre-merge checks✅ Passed checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 11
🧹 Nitpick comments (12)
apps/roam/src/styles/discourseGraphStyles.css (1)
181-184: Make highlight color themable and avoid!importantif possibleHard-coding the color and using
!importantmakes theming harder and can cause specificity battles. Prefer a CSS variable fallback and drop!importantunless there’s a known conflict.Apply this diff:
-.dg-highlight { - box-shadow: 0 0 0 2px #137cbd !important; - border-radius: 4px; -} +:root { + --dg-highlight-color: #137cbd; +} +.dg-highlight { + box-shadow: 0 0 0 2px var(--dg-highlight-color, #137cbd); + border-radius: 4px; +}If you find the outline gets overridden, consider increasing selector specificity for the target containers before reintroducing
!important. Also, verify contrast on dark themes.apps/roam/src/components/DiscourseSuggestionsPanel.tsx (1)
57-70: Minor: deduplicate toggle handlers to a single callbackBoth the heading and chevron button toggle the same state. Keeping a single
onTogglecallback reduces duplication and keeps behavior in sync.Apply this diff:
+ const onToggle = useCallback(() => setIsOpen((prev) => !prev), []); ... - <Navbar.Heading + <Navbar.Heading className="truncate" style={{ fontSize: "13px", margin: 0, fontWeight: 600, cursor: "pointer", }} - onClick={() => setIsOpen((prev) => !prev)} + onClick={onToggle} > {tag} </Navbar.Heading> ... <Button icon={isOpen ? "chevron-up" : "chevron-down"} minimal small - onClick={() => setIsOpen((prev) => !prev)} + onClick={onToggle} title={isOpen ? "Collapse" : "Expand"} />Also applies to: 80-94
apps/roam/src/components/SuggestionsBody.tsx (4)
20-26: LoosenSuggestedNodeto matchResultusage
SuggestionsListonly usesuid/text. Makingtypeoptional avoids forcing upstream mappers to fabricate it.Apply this diff:
-type SuggestedNode = Result & { type: string }; +type SuggestedNode = Result & { type?: string };
61-66: Accessibility: associate label with the inputThe label uses
htmlFor="suggest-page-input", but theAutocompleteInputinstance has noid, so screen readers won’t associate the two.Apply this diff if
AutocompleteInputsupportsidpassthrough:- <label - htmlFor="suggest-page-input" + <label + htmlFor="suggest-page-input" className="mb-1 block text-sm font-medium text-gray-700" > Suggest relationships from pages </label> @@ - <AutocompleteInput + <AutocompleteInput key={autocompleteKey} value={currentPageInput} placeholder={"Add page…"} setValue={setCurrentPageInput} options={allPages} maxItemsDisplayed={50} + id="suggest-page-input" />If it doesn’t, wrap the input in a
<label>instead.Also applies to: 78-86
191-238: Dead code path (filters/list) — keep or remove?
TypeFilterPopoverandSuggestionsListare defined but (prior to the diff above) unused. Either wire them in now (preferred) or remove to reduce bundle size until the feature lands.Also applies to: 240-302
67-76: Subtle UX: clarify “Find” vs “All Pages” actions“Find” just flips a boolean; “All Pages” resets state. Consider disabling “Find” until search is implemented or dispatching a no-op toast to set user expectations.
Also applies to: 134-160
apps/roam/src/utils/useDiscourseData.ts (2)
71-86: Guard whendiscourseNodeis missingIf
discourseNodeis falsy,getInforeturns early, but side effects likesetResults/setRefsfrom a previous tag might remain. Consider resetting state to empty when no discourse node is found.Apply this diff:
getOverlayInfo(tag, relations) .then(({ refs, results }) => { - if (!discourseNode) return; + if (!discourseNode) { + setResults([]); + setRefs(0); + setScore(0); + return; + }
17-20: Cache policy: consider size/TTL to avoid unbounded growthThe per-tag cache is unbounded and global. If tags vary widely, memory grows over time. A small TTL (e.g., 5–15 minutes) or LRU strategy can cap growth and keep data fresh.
I can provide a lightweight LRU implementation if helpful.
Also applies to: 163-177
apps/roam/src/components/PanelManager.tsx (2)
225-233: Unload listeners are fine; consider defensive cleanupGiven this code may be re-initialized in hot-reload/dev flows, optionally remove the listeners inside
handleUnloadto avoid duplicate registrations.const handleUnload = () => { cleanupObservers(); openPanels.clear(); + window.removeEventListener("beforeunload", handleUnload); + window.removeEventListener("pagehide", handleUnload); };
308-349: Remove or export the unusedrestorePanelInfrastructurefunctionThe
restorePanelInfrastructurehelper inPanelManager.tsx(lines 308–349) is defined but never called or exported. Keeping it in place increases maintenance surface without providing functionality.• Location: apps/roam/src/components/PanelManager.tsx (lines 308–349)
• Issue: no invocations or exports ofrestorePanelInfrastructurefound in the repoSuggestions:
- If you need to rehydrate panels on Roam DOM remounts, export this function and invoke it at the appropriate initialization or route-change hook.
- Otherwise, remove the entire
restorePanelInfrastructuredefinition to clean up dead code.apps/roam/src/components/DiscourseContextOverlay.tsx (2)
104-114: Add aria-label to the icon-only toggle for accessibilityThe inner toggle button is icon-only; add an aria-label to aid screen readers.
<Button data-dg-role="panel-toggle" data-dg-tag={tag} data-dg-panel-open={isPanelOpen ? "true" : "false"} icon={isPanelOpen ? "panel-table" : "panel-stats"} minimal small intent={isPanelOpen ? "primary" : "none"} + aria-label={isPanelOpen ? "Close suggestions panel" : "Open suggestions panel"} onClick={handleTogglePanel} />
48-57: Minor: avoid state read after toggle for consistencyImmediately after
panelManager.toggle,panelManager.isOpen(tag)should be accurate, but to avoid any race in future refactors, you can flip the local state optimistically or rely on thedg:panel-stateevent suggested earlier to set the authoritative state.- panelManager.toggle({ tag, blockUid, onloadArgs }); - setIsPanelOpen(panelManager.isOpen(tag)); + panelManager.toggle({ tag, blockUid, onloadArgs }); + // rely on dg:panel-state event to update `isPanelOpen`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
apps/roam/src/components/DiscourseContextOverlay.tsx(5 hunks)apps/roam/src/components/DiscourseSuggestionsPanel.tsx(1 hunks)apps/roam/src/components/PanelManager.tsx(1 hunks)apps/roam/src/components/SuggestionsBody.tsx(1 hunks)apps/roam/src/index.ts(1 hunks)apps/roam/src/styles/discourseGraphStyles.css(1 hunks)apps/roam/src/utils/useDiscourseData.ts(1 hunks)
🔇 Additional comments (5)
apps/roam/src/index.ts (1)
97-99: No changes needed:refreshConfigTreeis fully synchronousI’ve confirmed that
refreshConfigTreeis declared withoutasync, doesn’t return a promise, and performs only synchronous operations. Awaiting it would have no effect, so the existing call torefreshConfigTree()withoutawaitis correct.apps/roam/src/components/DiscourseSuggestionsPanel.tsx (1)
58-60: No Tailwind integration issues detected
Theapps/roampackage includes its owntailwind.config.ts, andtailwindcssis installed in its workspace, so utility classes liketruncatewill be applied as expected.apps/roam/src/utils/useDiscourseData.ts (1)
61-65: No action needed:discourseNode.typeis already the correct UIDWe’ve verified that the
typefield onDiscourseNodecomes directly from the configuration node’s UID (for user-defined nodes), the relation ID (for relation-backed nodes), or a fixed default string, and is never the human-readable title. BecausegetBasicTreeByParentUidexpects a page UID—andtypealready provides that—the original code is correct. There are no places inuseDiscourseData.tswheretypeis compared againstrelation.sourceorrelation.destination, so no conversion viagetPageUidByPageTitleis needed here.apps/roam/src/components/PanelManager.tsx (1)
244-253: Ignore React 18 migration in apps/roam
Theapps/roampackage is locked to React 17 and must continue usingReactDOM.render(...). The suggestion to switch tocreateRoot(...).render(...)only applies once the sub-project is upgraded to React 18 or newer, which it is not. No changes needed here.Likely an incorrect or invalid review comment.
apps/roam/src/components/DiscourseContextOverlay.tsx (1)
5-5: Consider switching to the package’s public API foruseInViewportYou’re currently on
react-in-viewport@^1.0.0-alpha.20and importing directly from the library’s internal path:import useInViewport from "react-in-viewport/dist/es/lib/useInViewport";Deep imports can break when the library’s folder/layout changes. If the package publicly exports the hook, prefer the top-level API:
import { useInViewport } from "react-in-viewport";Please verify whether your installed version exposes
useInViewportat its top level (e.g. by inspectingnode_modules/react-in-viewport/package.jsonfor anexportsor by grepping the top-level dist folder). If it’s not available, it’s safe to keep the existing deep import.
…-settings-panel-for-suggestive-mode
|
https://www.loom.com/share/fb19b2910b2344729da94b674afa9c8d?sid=e3727226-764f-4b2a-9ae2-ab773f1b62c5 @mdroidian I discovered 3 bugs, the last one related to hyde search (but it got introduced after some latest merges I will fix them but I think this video covers the what UI actions relate to what part of the pr. |
|
Cannot leave a line note outside of review scope it seems? Anyway hyde.ts:63, please use |
|
Ok... I know I need to dive into the code, but I have tons of ux questions which I realize are partly outside of the scope of this PR... and yet not. I will try to spend more time reviewing the code itself, but I went on a rant re my thoughts on how to do the suggestion panel. Roam web and app. |
There was a problem hiding this comment.
Ok. First, there is a lot that I propose that is about how to use React (more state!) and I'm not a React expert, feel very free to push back and correct me if I'm missing some advanced React patterns.
Second, I have more UX comments on the panel that are much more pointed, not reinvent-everything, but that may still be out-of-scope for this PR. Let me know. https://roamresearch.com/#/app/discourse-graphs/page/Npc55KjGO
The only really blocking comments are on the body of the useDiscourseData function.
| } | ||
| onChange={(e) => { | ||
| const target = e.target as HTMLInputElement; | ||
| extensionAPI.settings.set("suggestive-mode-overlay", target.checked); |
There was a problem hiding this comment.
@sid597 do you see these eslints in your IDE?
If so, let's try our best to solve them before submitting the PR. If you have issues, let me know and we can try to work them out together.
If you don't see these lints, definitely let me know so we can fix that.
|
@mdroidian The bugs were:
|
|
It overrides it for the time left side panel is open once its closed we revert back to what user had previously set, I think its an improvement in ux i.e the width setting is not usable in left sidebar mode, we can use better heuristics to set the main width but I do think we need to do it automatically without user intervention |
|
Ok, I'll trust your judgement here. |
sid597
changed the base branch from
main
to
eng-733-glue-ui-components-to-data-extraction-and-database
sid597
changed the base branch from
eng-733-glue-ui-components-to-data-extraction-and-database
to
main
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 7
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/settings/Settings.tsx (1)
102-127: Fix invalid nested CSS (&) in inline <style>Raw CSS doesn’t support & nesting; the WebKit scrollbar rules won’t apply as written.
Apply this diff:
<style>{` .dg-settings-tabs .bp3-tab-list { overflow-y: auto; overflow-x: hidden; max-height: 100%; /* Firefox */ scrollbar-width: thin; scrollbar-color: rgba(0, 0, 0, 0.2) transparent; - /* Webkit browsers */ - &::-webkit-scrollbar { - width: 6px; - } - &::-webkit-scrollbar-track { - background: transparent; - } - &::-webkit-scrollbar-thumb { - background-color: rgba(0, 0, 0, 0.2); - border-radius: 3px; - } } + /* WebKit browsers */ + .dg-settings-tabs .bp3-tab-list::-webkit-scrollbar { + width: 6px; + } + .dg-settings-tabs .bp3-tab-list::-webkit-scrollbar-track { + background: transparent; + } + .dg-settings-tabs .bp3-tab-list::-webkit-scrollbar-thumb { + background-color: rgba(0, 0, 0, 0.2); + border-radius: 3px; + } /* Override bp3-tab-copy font-size when text-lg is applied */ .bp3-tab-copy.text-lg { font-size: 1.125rem; } `}</style>
🧹 Nitpick comments (8)
apps/roam/src/utils/getAllDiscourseNodesSince.ts (1)
31-65: Guard against missing embedding refs.If
node.embeddingRefever comes through empty, we’ll end up issuing a needless query with the literal"undefined"UID. A quick early return keeps the API chatter down and avoids surprising results.const firstChildUid = extractRef(node.embeddingRef); + if (!firstChildUid) { + return []; + }apps/roam/src/utils/discourseConfigRef.ts (1)
28-29: Guard against missing Suggestive Mode subtree at runtimeIf the settings page doesn’t yet contain “Suggestive Mode,” getSuggestiveModeConfigAndUids must tolerate a missing subtree; otherwise consumers of getFormattedConfigTree risk undefined access. Ensure the helper returns safe defaults when the subtree is absent.
Also applies to: 55-56
apps/roam/src/components/settings/Settings.tsx (1)
254-256: Minor: avoid duplicate getVersionWithDate() callsCache the result once to avoid double compute/log noise.
- <span className="text-xs text-gray-500"> - v{getVersionWithDate().version}-{getVersionWithDate().buildDate} - </span> + {(() => { + const v = getVersionWithDate(); + return ( + <span className="text-xs text-gray-500"> + v{v.version}-{v.buildDate} + </span> + ); + })()}apps/roam/src/components/settings/NodeConfig.tsx (1)
1-1: Scope the ESLint disable to only where needed.Avoid a file-wide disable unless necessary; prefer local disables near offending code.
apps/roam/src/utils/configPageTabs.ts (1)
120-147: Normalize tab id casing to match existing tabs.Others use lowercase/slug ids (“home”, “grammar”, “export”). Consider “suggestive-mode” for consistency and to avoid potential case/whitespace issues.
Apply this diff:
- { - id: "Suggestive Mode", + { + id: "suggestive-mode",apps/roam/src/utils/getDiscourseNodes.ts (1)
29-35: Clean integration of Suggestive Rules into DiscourseNode; consider light documentation.
- New optional fields (embeddingRef, embeddingRefUid, isFirstChild) are reasonable and computed safely from config subtrees.
- Helper getUidAndBooleanSetting returns sane defaults on missing nodes.
- Suggestive Rules/Embedding Block Ref lookups are guarded and won’t throw.
Add brief JSDoc on the new fields to reduce ambiguity and highlight their config-driven nature.
Also applies to: 90-104, 108-116, 137-143
apps/roam/src/utils/pageRefObserverHandlers.ts (1)
56-84: DRY up the overlay handlers; align attribute naming.Overlay and suggestive overlay share nearly identical logic. Factor a small helper to reduce duplication and align the attribute naming convention (e.g., prefix with data-roamjs- for both).
Apply this diff (handler calls), and add the helper as shown:
-export const suggestiveOverlayPageRefHandler = ( - s: HTMLSpanElement, - onloadArgs: OnloadArgs, -) => { - if (s.parentElement && !s.parentElement.closest(".rm-page-ref")) { - const tag = - s.getAttribute("data-tag") || - s.parentElement.getAttribute("data-link-title"); - if ( - tag && - !s.getAttribute("data-discourse-suggestive-overlay") && - isDiscourseNode(getPageUidByPageTitle(tag)) - ) { - s.setAttribute("data-discourse-suggestive-overlay", "true"); - const parent = document.createElement("span"); - renderSuggestiveOverlay({ - parent, - tag: tag.replace(/\\"/g, '"'), - onloadArgs, - }); - if (s.hasAttribute("data-tag")) { - s.appendChild(parent); - } else { - s.parentElement.appendChild(parent); - } - } - } -}; +export const suggestiveOverlayPageRefHandler = ( + s: HTMLSpanElement, + onloadArgs: OnloadArgs, +) => + installOverlay({ + s, + onloadArgs, + attribute: "data-roamjs-discourse-suggestive-overlay", + render: renderSuggestiveOverlay, + });Add this helper near the top-level (outside the selected range):
type OverlayInstallArgs = { s: HTMLSpanElement; onloadArgs: OnloadArgs; attribute: string; render: (opts: { parent: HTMLElement; tag: string; onloadArgs: OnloadArgs }) => void; }; const installOverlay = ({ s, onloadArgs, attribute, render }: OverlayInstallArgs) => { if (!s.parentElement || s.parentElement.closest(".rm-page-ref")) return; const rawTag = s.getAttribute("data-tag") || s.parentElement.getAttribute("data-link-title"); if (!rawTag) return; if (s.getAttribute(attribute)) return; if (!isDiscourseNode(getPageUidByPageTitle(rawTag))) return; s.setAttribute(attribute, "true"); const parent = document.createElement("span"); render({ parent, tag: rawTag.replace(/\\"/g, '"'), onloadArgs }); (s.hasAttribute("data-tag") ? s : s.parentElement).appendChild(parent); };Do we intend both overlays to render simultaneously when both settings are enabled? If not, add a guard to prefer one.
apps/roam/src/components/settings/HomePersonalSettings.tsx (1)
62-66: Consistent use ofvoidfor ignored promises; consider global error logging.Pattern is fine. Optionally attach a
.catch(console.error)in dev to surface setting write failures during debugging.Also applies to: 111-115, 133-137, 153-153, 173-176
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
apps/roam/src/components/DiscourseSuggestionsPanel.tsx(1 hunks)apps/roam/src/components/PanelManager.tsx(1 hunks)apps/roam/src/components/SuggestionsBody.tsx(1 hunks)apps/roam/src/components/SuggestiveModeOverlay.tsx(1 hunks)apps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsx(1 hunks)apps/roam/src/components/settings/HomePersonalSettings.tsx(6 hunks)apps/roam/src/components/settings/NodeConfig.tsx(4 hunks)apps/roam/src/components/settings/PageGroupPanel.tsx(1 hunks)apps/roam/src/components/settings/Settings.tsx(2 hunks)apps/roam/src/components/settings/SuggestiveModeSettings.tsx(1 hunks)apps/roam/src/index.ts(2 hunks)apps/roam/src/styles/discourseGraphStyles.css(1 hunks)apps/roam/src/utils/configPageTabs.ts(2 hunks)apps/roam/src/utils/discourseConfigRef.ts(3 hunks)apps/roam/src/utils/getAllDiscourseNodesSince.ts(4 hunks)apps/roam/src/utils/getDiscourseNodes.ts(3 hunks)apps/roam/src/utils/getSuggestiveModeConfigSettings.ts(1 hunks)apps/roam/src/utils/hyde.ts(7 hunks)apps/roam/src/utils/initializeObserversAndListeners.ts(2 hunks)apps/roam/src/utils/pageRefObserverHandlers.ts(3 hunks)apps/roam/src/utils/renderNodeConfigPage.ts(2 hunks)apps/roam/src/utils/syncDgNodesToSupabase.ts(5 hunks)apps/roam/src/utils/useDiscourseData.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `node.type` serves as the UID field rather than having a conventional `node.uid` field. This is an unusual naming convention where the type field actually contains the unique identifier.
Applied to files:
apps/roam/src/utils/getDiscourseNodes.ts
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `type` serves as the unique identifier field, not a type classification field. The interface has no `uid` or `id` field, making `node.type` the correct field to use for UID-related operations.
Applied to files:
apps/roam/src/utils/getDiscourseNodes.tsapps/roam/src/utils/useDiscourseData.tsapps/roam/src/utils/getAllDiscourseNodesSince.ts
📚 Learning: 2025-06-22T10:40:52.752Z
Learnt from: sid597
PR: DiscourseGraphs/discourse-graph#232
File: apps/roam/src/utils/getAllDiscourseNodesSince.ts:18-31
Timestamp: 2025-06-22T10:40:52.752Z
Learning: In apps/roam/src/utils/getAllDiscourseNodesSince.ts, the user confirmed that querying for `?title` with `:node/title` and mapping it to the `text` field in the DiscourseGraphContent type is the correct implementation for retrieving discourse node content from Roam Research, despite it appearing to query page titles rather than block text content.
Applied to files:
apps/roam/src/utils/getDiscourseNodes.tsapps/roam/src/utils/useDiscourseData.tsapps/roam/src/utils/getAllDiscourseNodesSince.tsapps/roam/src/utils/hyde.ts
📚 Learning: 2025-05-30T14:49:24.016Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#182
File: apps/website/app/utils/supabase/dbUtils.ts:22-28
Timestamp: 2025-05-30T14:49:24.016Z
Learning: In apps/website/app/utils/supabase/dbUtils.ts, expanding the KNOWN_EMBEDDINGS and DEFAULT_DIMENSIONS mappings to support additional embedding models requires corresponding database model changes (creating new embedding tables), which should be scoped as separate work from API route implementations.
Applied to files:
apps/roam/src/index.tsapps/roam/src/utils/syncDgNodesToSupabase.ts
📚 Learning: 2025-08-25T15:53:21.799Z
Learnt from: sid597
PR: DiscourseGraphs/discourse-graph#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/useDiscourseData.ts
📚 Learning: 2025-06-22T10:40:21.679Z
Learnt from: sid597
PR: DiscourseGraphs/discourse-graph#232
File: apps/roam/src/utils/getAllDiscourseNodesSince.ts:15-16
Timestamp: 2025-06-22T10:40:21.679Z
Learning: In the getAllDiscourseNodesSince function in apps/roam/src/utils/getAllDiscourseNodesSince.ts, date validation is performed before the function is called, so additional date validation within the function is not needed.
Applied to files:
apps/roam/src/utils/getAllDiscourseNodesSince.ts
🔇 Additional comments (21)
apps/roam/src/utils/getAllDiscourseNodesSince.ts (3)
5-5: Reusing existing util is a solid call.Pulling in
extractRefkeeps this logic consistent with the rest of our Roam helpers.
101-111: Nice catch filtering whitespace-only titles.Skipping nodes whose resolved title trims to empty prevents us from emitting junk records.
123-133: Happy with the stitched settings block fetch.Looping through
nodeTypesand appending their settings-backed block nodes keeps downstream consumers whole once page nodes are processed.apps/roam/src/styles/discourseGraphStyles.css (1)
181-184: LGTM: simple, clear hover highlightThe class is minimal and appropriately scoped. No concerns.
apps/roam/src/utils/discourseConfigRef.ts (1)
10-13: Wiring Suggestive Mode config into the formatted tree looks correctImport location and types align with downstream usage.
apps/roam/src/utils/hyde.ts (3)
355-401: Extract Roam Datalog helpers out of hyde.tsextractPagesFromChildBlock/ParentBlock and getAllReferencesOnPage are generic Roam query helpers. Move them to a dedicated utils module (e.g., utils/roamQueries.ts) and import here. Keeps hyde.ts focused and easier to test.
Based on prior feedback from maparent.
Also applies to: 471-479
432-434: Early exit when discourseNode is absent is correctPrevents wasted queries and keeps UX predictable when current page isn’t a discourse node.
446-465: Filtering logic and current-block exclusion look good
- Skips current block via blockUid
- Skips defaults and invalid types
- Dedup via existingUids
Also applies to: 485-503, 505-512
apps/roam/src/components/settings/Settings.tsx (1)
165-170: Suggestive Mode tab integration looks correctTab wiring, title, and panel render are consistent with the rest of the dialog.
apps/roam/src/utils/initializeObserversAndListeners.ts (1)
145-147: LGTM: conditional registration for Suggestive Mode overlay is correct.Uses a stable cached handler; Set-based registry prevents duplicates and supports runtime enable/disable.
apps/roam/src/utils/renderNodeConfigPage.ts (1)
151-162: LGTM: Suggestive Rules panel wiring is consistent with existing CustomPanel usage.Label matches the subtree key used elsewhere (“Suggestive Rules”), ensuring config reads resolve correctly.
Please confirm DiscourseNodeSuggestiveRules gracefully handles a missing/empty parentUid (i.e., when the subtree hasn't been created yet).
apps/roam/src/components/settings/NodeConfig.tsx (2)
166-167: LGTM: derives Suggestive Rules UID like other sections.Consistent with getUid pattern; downstream component receives a stable parentUid or empty string.
389-400: LGTM: Adds a clear Suggestive Mode tab with rules editor.Matches patterns of other tabs; props passed align with component’s expected contract.
If suggestiveRulesUid is empty, confirm the component creates the subtree instead of failing silently.
apps/roam/src/utils/pageRefObserverHandlers.ts (1)
21-26: LGTM: cached suggestive overlay handler mirrors existing overlay handler.Stable reference avoids churn and plays well with Set-based observer management.
apps/roam/src/components/settings/HomePersonalSettings.tsx (1)
80-104: LGTM: Suggestive Mode overlay toggle is wired correctly.Persists setting and updates observer live using the cached handler.
If both overlays are enabled, confirm the UI/positioning doesn’t conflict; otherwise consider mutual exclusivity in the UI.
apps/roam/src/components/DiscourseSuggestionsPanel.tsx (1)
27-34: EscapeblockUidbefore building selectors.Line 30 interpolates
blockUidstraight into a CSS selector. Any UID containing"], quotes, or other special characters will break the selector, and the lack of escaping is also a mild injection vector. Escape the UID (useCSS.escapewhen available and fall back to a conservative replacer) once before the query and reuse it. Applies the same resolution requested in the earlier review.const toggleHighlight = useCallback( (on: boolean) => { - document - .querySelectorAll(`[data-dg-block-uid="${blockUid}"]`) + const safeUid = + typeof CSS !== "undefined" && typeof CSS.escape === "function" + ? CSS.escape(blockUid) + : blockUid.replace(/["\\]/g, "\\$&"); + document + .querySelectorAll(`[data-dg-block-uid="${safeUid}"]`) .forEach((el) => el.classList.toggle("dg-highlight", on)); }, [blockUid], );apps/roam/src/components/SuggestiveModeOverlay.tsx (1)
29-75: EscapeblockUidbefore querying for highlight targets.Line 32 has the same raw interpolation of
blockUidinto an attribute selector as previously flagged. UIDs can contain characters (",], etc.) that break the selector, and you risk selector injection. Escape the UID once (preferCSS.escape) before performing the query. This mirrors the unresolved feedback noted in the earlier review.const toggleHighlight = useCallback( (on: boolean) => { - const nodes = document.querySelectorAll( - `[data-dg-block-uid="${blockUid}"]`, - ); + const safeUid = + typeof CSS !== "undefined" && typeof CSS.escape === "function" + ? CSS.escape(blockUid) + : blockUid.replace(/["\\]/g, "\\$&"); + const nodes = document.querySelectorAll( + `[data-dg-block-uid="${safeUid}"]`, + );apps/roam/src/components/SuggestionsBody.tsx (1)
114-118: Escape UIDs before using them in selectorsRaw UIDs in attribute selectors blow up when special characters slip in. Please escape before interpolating.
- const toggleOverlayHighlight = (nodeUid: string, on: boolean) => { - document - .querySelectorAll(`[data-dg-block-uid="${nodeUid}"]`) + const toggleOverlayHighlight = (nodeUid: string, on: boolean) => { + const safeUid = + typeof CSS !== "undefined" && typeof CSS.escape === "function" + ? CSS.escape(nodeUid) + : nodeUid.replace(/"/g, '\\"'); + document + .querySelectorAll(`[data-dg-block-uid="${safeUid}"]`) .forEach((el) => el.classList.toggle("dg-highlight", on)); };apps/roam/src/components/PanelManager.tsx (1)
145-151: Escape blockUid before querying the DOMSame selector concern here—escape the uid before dropping it into the attribute selector.
- const nodes = document.querySelectorAll( - `[data-dg-block-uid="${blockUid}"]`, - ); + const safeUid = + typeof CSS !== "undefined" && typeof CSS.escape === "function" + ? CSS.escape(blockUid) + : blockUid.replace(/"/g, '\\"'); + const nodes = document.querySelectorAll( + `[data-dg-block-uid="${safeUid}"]`, + );apps/roam/src/utils/useDiscourseData.ts (2)
35-38: Escape normalized page title before embedding in EDNInterpolating
normalizePageTitle(tag)raw will break the query (or worse) if the title contains quotes/backslashes. Please escape it before building the string.- window.roamAlphaAPI.data.backend.q( - `[:find ?a :where [?b :node/title "${normalizePageTitle(tag)}"] [?a :block/refs ?b]]`, - ), + window.roamAlphaAPI.data.backend.q( + `[:find ?a :where [?b :node/title "${normalizePageTitle(tag) + .replace(/\\/g, "\\\\") + .replace(/"/g, '\\"')}"] [?a :block/refs ?b]]`, + ),
15-44: Cache never expires → stale overlay dataBecause
cache[tag]is returned unconditionally, panels never see graph updates after the first fetch (even after closing/reopening). We need a TTL or explicit invalidation so refreshed data can flow through.-const cache: { - [tag: string]: DiscourseData; -} = {}; +const CACHE_TTL_MS = 30_000; +const cache: Record<string, { data: DiscourseData; cachedAt: number }> = {}; @@ - if (cache[tag]) return cache[tag]; + const cached = cache[tag]; + if (cached && Date.now() - cached.cachedAt < CACHE_TTL_MS) { + return cached.data; + } @@ - return (cache[tag] = { - results, - refs: refs.length, - }); + const data: DiscourseData = { + results, + refs: refs.length, + }; + cache[tag] = { data, cachedAt: Date.now() }; + return data;
| export const initializeSupabaseSync = async () => { | ||
| const supabase = createClient(); | ||
| const result = await supabase | ||
| .from("Space") | ||
| .select() | ||
| .eq("url", getRoamUrl()) | ||
| .maybeSingle(); | ||
| if (!result.data) { | ||
| return; | ||
| } else { | ||
| createOrUpdateDiscourseEmbedding(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Await embedding sync to avoid unhandled rejection
initializeSupabaseSync kicks off createOrUpdateDiscourseEmbedding() but never awaits it. Callers return early, and any rejection becomes unhandled. Please await the promise (and surface the Supabase error if present).
- if (!result.data) {
- return;
- } else {
- createOrUpdateDiscourseEmbedding();
- }
+ if (!result.data) {
+ return;
+ }
+ await createOrUpdateDiscourseEmbedding();🤖 Prompt for AI Agents
In apps/roam/src/utils/syncDgNodesToSupabase.ts around lines 413 to 425, the
function calls createOrUpdateDiscourseEmbedding() without awaiting it which can
produce unhandled promise rejections; modify the code to await
createOrUpdateDiscourseEmbedding() and propagate any errors to the caller by
returning or throwing them, and when the Supabase query returns an error
(result.error) surface that error instead of silently returning—i.e., check
result.error and handle/throw it, then await the embedding call and return its
result or rethrow on failure so callers receive the failure.
There was a problem hiding this comment.
It started out as nits but turned into more. Plus the CodeRabbit suggestions.
At this point I think we might want to break this down into smaller PR's. Or merge it and and create a list of linear tickets to do immediately after merge to address the fixes. Because this is quite unwieldly. I'll let you decide how to move forward.
Also, there are some merge conflicts that need to be addressed,
Finally, I tried to get it to work locally or via https://discoursegraphs.com/releases/... but neither worked for me. Here are the errors from .../releases/...
|
New prs created out of these, review points resolved, some that did not get resolved are carried over to new prs |
3 participants
Summary by CodeRabbit