ENG-1188: add pull watch for top level blocks #632
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. |
sid597
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughA new utility function is added that registers a pull watcher on the Roam Alpha API for block properties. The function extracts a featureFlags block UID from a provided map and conditionally registers a data.pullWatch listener that logs changes to the console. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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: 2
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/roam/src/utils/pullWatchBlockPropsBasedSettings.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/utils/pullWatchBlockPropsBasedSettings.ts
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/utils/pullWatchBlockPropsBasedSettings.ts
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/utils/pullWatchBlockPropsBasedSettings.ts
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/utils/pullWatchBlockPropsBasedSettings.ts
apps/roam/**
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Implement the Discourse Graph protocol in the Roam Research extension
Files:
apps/roam/src/utils/pullWatchBlockPropsBasedSettings.ts
🧠 Learnings (2)
📚 Learning: 2025-12-22T05:43:01.282Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 630
File: apps/roam/src/utils/settingsUsingBlockProps.ts:64-66
Timestamp: 2025-12-22T05:43:01.282Z
Learning: In `apps/roam/src/utils/settingsUsingBlockProps.ts`, the `setBlockPropBasedSettings` function expects callers to ensure that the `value` parameter matches the expected type for the given `keys` path. Type validation is handled at the caller side rather than within the utility function.
Applied to files:
apps/roam/src/utils/pullWatchBlockPropsBasedSettings.ts
📚 Learning: 2025-11-06T13:48:35.007Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-11-06T13:48:35.007Z
Learning: In apps/roam/src/utils/createReifiedBlock.ts, the `setBlockProps` function does not need await as it is synchronous or fire-and-forget.
Applied to files:
apps/roam/src/utils/pullWatchBlockPropsBasedSettings.ts
🧬 Code graph analysis (1)
apps/roam/src/utils/pullWatchBlockPropsBasedSettings.ts (1)
apps/roam/src/data/blockPropsSettingsConfig.ts (1)
TOP_LEVEL_BLOCK_PROP_KEYS(2-2)
⏰ 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). (7)
- 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
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
| export const setupPullWatchBlockPropsBasedSettings = ( | ||
| blockUids: Record<string, string>, | ||
| ) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explicit return type per coding guidelines.
The function is missing an explicit return type. Per the coding guidelines, all functions should have explicit return types.
🔎 Proposed fix
-export const setupPullWatchBlockPropsBasedSettings = (
- blockUids: Record<string, string>,
-) => {
+export const setupPullWatchBlockPropsBasedSettings = (
+ blockUids: Record<string, string>,
+): void => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const setupPullWatchBlockPropsBasedSettings = ( | |
| blockUids: Record<string, string>, | |
| ) => { | |
| export const setupPullWatchBlockPropsBasedSettings = ( | |
| blockUids: Record<string, string>, | |
| ): void => { |
🤖 Prompt for AI Agents
In apps/roam/src/utils/pullWatchBlockPropsBasedSettings.ts around lines 3 to 5,
the exported function lacks an explicit return type; add a return type
annotation after the parameter list (e.g. export const
setupPullWatchBlockPropsBasedSettings = (blockUids: Record<string, string>):
void => { ... }) — if the function actually returns a value or an unsubscribe
function, replace void with the correct type (e.g. Promise<void> or () => void)
to satisfy the coding guidelines.
| if (featureFlagsBlockUid) { | ||
| window.roamAlphaAPI.data.addPullWatch( | ||
| "[:block/props]", | ||
| `[:block/uid "${featureFlagsBlockUid}"]`, | ||
| (before, after) => { | ||
| console.log("feature flags changed", before, after); | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and read the pullWatchBlockPropsBasedSettings file
find . -name "*pullWatchBlockPropsBasedSettings*" -type fRepository: DiscourseGraphs/discourse-graph
Length of output: 132
🏁 Script executed:
#!/bin/bash
# Read the file in question and context
fd "pullWatchBlockPropsBasedSettings" --exec cat -n {} \;Repository: DiscourseGraphs/discourse-graph
Length of output: 720
🏁 Script executed:
#!/bin/bash
# Search for setupPullWatchBlockPropsBasedSettings usage in the codebase
rg -n "setupPullWatchBlockPropsBasedSettings" -A 3 -B 3 --type ts --type tsxRepository: DiscourseGraphs/discourse-graph
Length of output: 102
🏁 Script executed:
#!/bin/bash
# Search for how setupPullWatchBlockPropsBasedSettings is called
rg -n "setupPullWatchBlockPropsBasedSettings" -A 5 -B 2Repository: DiscourseGraphs/discourse-graph
Length of output: 802
🏁 Script executed:
#!/bin/bash
# Search for other addPullWatch usage in codebase to see cleanup patterns
rg -n "addPullWatch" -A 5 -B 2Repository: DiscourseGraphs/discourse-graph
Length of output: 1402
🏁 Script executed:
#!/bin/bash
# Search for callers of setupPullWatchBlockPropsBasedSettings - where is it invoked?
rg -n "setupPullWatchBlockPropsBasedSettings\(" -B 5 -A 5Repository: DiscourseGraphs/discourse-graph
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Get more context around the useRoamStore usage to understand the cleanup pattern
cat -n apps/roam/src/components/canvas/useRoamStore.ts | sed -n '410,430p'Repository: DiscourseGraphs/discourse-graph
Length of output: 880
🏁 Script executed:
#!/bin/bash
# Search for imports and actual usage of setupPullWatchBlockPropsBasedSettings
rg -n "setupPullWatchBlockPropsBasedSettings" --type tsRepository: DiscourseGraphs/discourse-graph
Length of output: 187
🏁 Script executed:
#!/bin/bash
# Look for where this function might be called - check if it's exported and used
git grep -n "setupPullWatchBlockPropsBasedSettings"Repository: DiscourseGraphs/discourse-graph
Length of output: 187
🏁 Script executed:
#!/bin/bash
# Check the full context of useRoamStore to see the pullWatchProps definition
cat -n apps/roam/src/components/canvas/useRoamStore.ts | sed -n '400,430p'Repository: DiscourseGraphs/discourse-graph
Length of output: 1479
🏁 Script executed:
#!/bin/bash
# Search more broadly - check all files including .tsx for any reference
rg "setupPullWatchBlockPropsBasedSettings" --type-list
rg "setupPullWatchBlockPropsBasedSettings" .Repository: DiscourseGraphs/discourse-graph
Length of output: 594
🏁 Script executed:
#!/bin/bash
# Check the context around line 400-430 to understand the full picture
cat -n apps/roam/src/components/canvas/useRoamStore.ts | sed -n '395,435p'Repository: DiscourseGraphs/discourse-graph
Length of output: 1615
🏁 Script executed:
#!/bin/bash
# Check if setupPullWatchBlockPropsBasedSettings is actually called anywhere
rg "setupPullWatchBlockPropsBasedSettings\(" --type ts -B 3 -A 3Repository: DiscourseGraphs/discourse-graph
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Look for imports of setupPullWatchBlockPropsBasedSettings
rg "import.*setupPullWatchBlockPropsBasedSettings"Repository: DiscourseGraphs/discourse-graph
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Check the type definition for AddPullWatch to understand return value
rg -n "type AddPullWatch|interface AddPullWatch" -A 5Repository: DiscourseGraphs/discourse-graph
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Check if setupPullWatchBlockPropsBasedSettings might be called dynamically or exported in a barrel export
cat apps/roam/src/utils/pullWatchBlockPropsBasedSettings.tsRepository: DiscourseGraphs/discourse-graph
Length of output: 594
🏁 Script executed:
#!/bin/bash
# Check index files that might re-export this
find apps/roam/src -name "index.ts" -o -name "index.tsx" | xargs grep -l "setupPullWatchBlockPropsBasedSettings" 2>/dev/nullRepository: DiscourseGraphs/discourse-graph
Length of output: 59
🏁 Script executed:
#!/bin/bash
# Final check - search entire repo for any reference including comments
rg "setupPullWatchBlockPropsBasedSettings" --type ts -C 10Repository: DiscourseGraphs/discourse-graph
Length of output: 1238
🏁 Script executed:
#!/bin/bash
# Check if there are any newer commits or branches that might use this function
git log --oneline --all -S "setupPullWatchBlockPropsBasedSettings" 2>/dev/null | head -20Repository: DiscourseGraphs/discourse-graph
Length of output: 118
🏁 Script executed:
#!/bin/bash
# Check the git diff or status to see if this is a new file in this PR
git log --oneline --follow -- apps/roam/src/utils/pullWatchBlockPropsBasedSettings.ts 2>/dev/null | head -10Repository: DiscourseGraphs/discourse-graph
Length of output: 118
🏁 Script executed:
#!/bin/bash
# Check the commit details to understand this PR better
git show 8f17d89 --statRepository: DiscourseGraphs/discourse-graph
Length of output: 40103
🏁 Script executed:
#!/an/bin/bash
# Check if the function is used in that commit or any subsequent commits
git show 8f17d89 --name-onlyRepository: DiscourseGraphs/discourse-graph
Length of output: 29331
🏁 Script executed:
#!/bin/bash
# Check the full diff of that commit
git show 8f17d89Repository: DiscourseGraphs/discourse-graph
Length of output: 50391
Critical: Add cleanup mechanism to prevent memory leaks when pull watch is registered.
The addPullWatch listener persists indefinitely without cleanup. Although this function is currently unused, it must provide a cleanup mechanism when called. Pattern in the codebase (e.g., useRoamStore.ts) shows the correct approach:
return () => {
window.roamAlphaAPI.data.removePullWatch(
"[:block/props]",
`[:block/uid "${featureFlagsBlockUid}"]`,
callback
);
};Additionally, the callback only logs to console—implement the actual settings update logic to complete this feature.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.