ENG-949: Add query sections to left sidebar#1056
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b645caa0f3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
📝 WalkthroughWalkthroughThis PR extends the left sidebar to support query-block sections as a new section type. Query sections run Roam queries and display results, with customizable alias names and result limits that persist to Roam blocks. The implementation adds schema fields, parsing utilities, a settings UI for configuration, and a new QuerySectionItem component for rendering. ChangesQuery-block sidebar sections
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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. 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/roam/src/components/settings/LeftSidebarPersonalSettings.tsx`:
- Around line 348-394: The debounce timeout set in handleAliasChange via
aliasUpdateTimeoutRef can fire after the component unmounts; add a cleanup
effect that clears aliasUpdateTimeoutRef.current on unmount to prevent stale
writes. Implement a useEffect with no dependencies (or appropriate lifecycle)
that returns a cleanup function calling
clearTimeout(aliasUpdateTimeoutRef.current) and nulling
aliasUpdateTimeoutRef.current, ensuring handleAliasChange, aliasUpdateTimeoutRef
and any created timeouts are cancelled when the component unmounts.
- Around line 742-753: The default for the new query section "Result-limit" is
inconsistent: the node initialization sets the child text to "10" while the
newSection.settings.resultLimit.value is 10, but the schema/default snapshots
expect 0 (unlimited). Update both the block node initialization (the child text
under node: { text: "Result-limit", children: [...] }) and the
newSection.settings.resultLimit.value (where resultLimit is created with uid:
resultLimitUid) to use 0 so the created section matches the schema/default
snapshots.
In `@apps/roam/src/components/settings/utils/zodSchema.ts`:
- Line 225: The "Result-limit" schema currently uses z.number().default(0) which
permits negative and fractional values; update the Zod schema for the
"Result-limit" field to require a non-negative integer (use Zod integer +
minimum zero validation) while preserving the default of 0 so runtime slicing
cannot receive negatives or decimals.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7a813ba3-f8cb-4ddd-b71b-630631050145
📒 Files selected for processing (7)
apps/roam/src/components/LeftSidebarView.tsxapps/roam/src/components/settings/LeftSidebarPersonalSettings.tsxapps/roam/src/components/settings/utils/accessors.tsapps/roam/src/components/settings/utils/zodSchema.example.tsapps/roam/src/components/settings/utils/zodSchema.tsapps/roam/src/utils/getExportSettings.tsapps/roam/src/utils/getLeftSidebarSettings.ts
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c5f06fd90
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
A couple possible blockers:
- Could you point to me where the alias is being store in block props? I see
createBlocka few places, but no check/handle ofuse new store settings. - we should reuse the query builder observer regex/observer checking logic if possible for
{{query block}}
| }; | ||
|
|
||
| const BLOCK_REF_FULL_MATCH = new RegExp(`^${BLOCK_REF_REGEX.source}$`); | ||
| const QUERY_BLOCK_MARKER = /\{\{query block(?::[^}]*)?\}\}/; |
There was a problem hiding this comment.
This should exist in a few places already. Try to see if we can DRY this.
There was a problem hiding this comment.
the existing spots:
parseResultSettings.ts:41has/{{query block:(.*?)}}/, but that only handles{{query block:alias}}.listActiveQueries.ts:8checks{{query block}}, but not{{query block:alias}}.resolveQueryBuilderRef.ts:11checks one known alias:{{query block:${queryRef}}}.initializeObserversAndListeners.ts:146-147usescreateButtonObserver({ attribute: "query-block" }), but that is for rendered DOM buttons, not raw block text.
So there is no existing exported helper that covers both raw block forms: {{query block}} and {{query block:alias}}.
There was a problem hiding this comment.
listActiveQueries should be checking for both. We could reuse it there.
There was a problem hiding this comment.
you mean to fix listActiveQueries and then use that here?
If we do that then there is circular dependency and need more refactoring to circumvent that do we want to do that here? I am fine either way.
There was a problem hiding this comment.
What is the circular dependency?
There was a problem hiding this comment.
-> Modify listActiveQueries.ts
-> Import it into getLeftsidebarSettings.ts
Now the import path is:
getLeftSidebarSettings.ts
imports listActiveQueries.ts
listActiveQueries.ts
imports getQueryPages from QueryPagesPanel.tsx
QueryPagesPanel.tsx
imports getPersonalSetting from accessors.ts
accessors.ts
imports getLeftSidebarSettings.ts
There was a problem hiding this comment.
Ah, no, just export and reuse the QUERY_BLOCK_MARKER
There was a problem hiding this comment.
Should listActiveQueries.ts find both the normal query blocks and query blocks with aliases? currently it only searches for the {{query block}} ?
| const BLOCK_REF_FULL_MATCH = new RegExp(`^${BLOCK_REF_REGEX.source}$`); | ||
| const QUERY_BLOCK_MARKER = /\{\{query block(?::[^}]*)?\}\}/; | ||
|
|
||
| export const isQueryBlockRef = (text: string): boolean => { |
There was a problem hiding this comment.
How does the current query block observer handle this? Can we reuse that logic instead of writing it again?
There was a problem hiding this comment.
The current observer watches the button Roam displays for {{query block}} blocks (initializeObserversAndListeners.ts:146-149). In this sidebar path we only have the saved section text, usually a block ref like ((uid)), and need to decide whether to render QuerySectionItem before rendering the sidebar row.
So we dereference the block ref with getTextByBlockUid and check the block text.
| }; | ||
|
|
||
| /* eslint-disable @typescript-eslint/naming-convention */ | ||
| export const sectionsToBlockProps = ( |
There was a problem hiding this comment.
@mdroidian For block props we don't do per-setting writes. We map the whole personal left-sidebar settings structure here because we have to update the data locally for the component, but also persist that same structure in block props.
Below, around line 81, we have the syncAllSectionsToBlockProps function which writes this mapped structure to block props via setPersonalSetting. For this query alias path, we build nextSections, update local state with setSections(nextSections), and then call syncAllSectionsToBlockProps(nextSections) around line 426.
2 participants
https://www.loom.com/share/bc4e7221aa9240ec9f58a9ba3f5657ab
Summary by CodeRabbit