-
Notifications
You must be signed in to change notification settings - Fork 2
ENG-917 Suggestions body and actions #473
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: eng-914-panel-manager
Are you sure you want to change the base?
Conversation
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughImplements controlled open/close behavior for Discourse suggestions panels, introduces a new SuggestionsBody component, updates PanelManager to manage per-panel isOpen state and updates, revises hyde.ts APIs and parameters, adjusts suggestive mode settings initialization/creation logic, removes two JSX comments, and reorders a setup script in database/package.json. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant PanelManager
participant DiscourseSuggestionsPanel as SuggestionsPanel
participant SuggestionsBody
Note over PanelManager,SuggestionsPanel: Controlled panel open/close
User->>PanelManager: Open panel for tag
PanelManager->>PanelManager: addPanel(tag, { isOpen: true })
PanelManager->>SuggestionsPanel: Render with isOpen=true, onToggle(...)
SuggestionsPanel->>SuggestionsPanel: handleToggle(next)
SuggestionsPanel-->>PanelManager: onToggle(next)
PanelManager->>PanelManager: updatePanelState(tag, { isOpen: next })
PanelManager->>SuggestionsPanel: Re-render with updated isOpen
SuggestionsPanel->>SuggestionsBody: Render body when isOpen
sequenceDiagram
autonumber
participant UI as SuggestionsBody
participant Hyde as performHydeSearch
participant Roam as Roam/Backend APIs
Note over UI,Hyde: Hyde params now use pageTitle, blockUid, discourseNode
UI->>Hyde: performHydeSearch({ pageTitle, blockUid, discourseNode })
Hyde->>Roam: Fetch context (parent/child, refs) as needed
Roam-->>Hyde: Context data
Hyde->>Hyde: Filter/score candidates (uid optional)
Hyde-->>UI: Results (groups/nodes)
UI->>UI: Update cache, filters, counters
UI-->>User: Display suggestions and actions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 (2)
apps/roam/src/components/settings/SuggestiveModeSettings.tsx (1)
14-102
: Statefulsettings
now goes staleStoring
settings
in component state means the values backingFlagPanel
/PageGroupsPanel
freeze after the first render. Subsequent edits (toggling a flag, editing page groups) happen in Roam, but this component never re‑fetches the config, so it keeps handing the oldvalue
/initialGroups
back to the children. The UI drifts from the real config and reverts user changes on re-render. Please switch back to derivingsettings
fromgetFormattedConfigTree()
each render or add a watcher that refreshes state whenever the underlying config changes.apps/roam/src/utils/hyde.ts (1)
434-488
: Boolean settings no longer honoured
getExtensionAPI().settings.get(...)
gives strings like"false"
for unchecked toggles. Casting withas boolean
doesn’t coerce at runtime, so both settings read as truthy and the “grab parent/child” and “grab referenced pages” filters always run. This regresses users who turned them off. Please restoregetBooleanSetting
(or explicitly coerce with something like=== true || value === "true"
) before branching on these flags.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/roam/src/components/DiscourseSuggestionsPanel.tsx
(4 hunks)apps/roam/src/components/PanelManager.tsx
(6 hunks)apps/roam/src/components/SuggestionsBody.tsx
(1 hunks)apps/roam/src/components/settings/PageGroupPanel.tsx
(0 hunks)apps/roam/src/components/settings/SuggestiveModeSettings.tsx
(3 hunks)apps/roam/src/utils/hyde.ts
(7 hunks)packages/database/package.json
(1 hunks)
💤 Files with no reviewable changes (1)
- apps/roam/src/components/settings/PageGroupPanel.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 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/hyde.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
43ea70b
to
8e1c9fc
Compare
436430a
to
ac171db
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I couldn't fully test

Until then, here are some suggestions:

Remove the bottom border from this Navbar.
If you need the border, put it as a top border of the collapsed component, so it only shows up when unfolded.

Remove the card border of <SuggestionsBody>
... too many lines
I only see that the comment has been moved to this PR, but not addressed: #473 (comment)
Do you plan on addressing it still in this PR or somewhere else?
… 3. fix panel manager scroll issue, add new panel to top 4. If no result from search show message 5. fix pading and view for filter button row 6. use formatted tree instead of extensionapi, fix when suggestions don't exist
ac171db
to
e6b1f75
Compare
121a02e
to
1ae2a38
Compare
https://www.loom.com/share/1ddd5b55abbe4313a0f5da18c86e06fd?sid=26ac4d77-0c72-4ec2-987b-b38d731e09b9
Pending todos for next pr (all relate to db function so want to do them together):
New issues that got created
selection
insteadAll issues from #340 are addressed
Summary by CodeRabbit
New Features
Improvements
Chores