ENG-1044 Experiments with different datalog query modalities #539
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds an optional Changes
Sequence DiagramsequenceDiagram
autonumber
participant Caller
participant fireQuery
participant PropScanner as "Prop Scanner\n(PROP_NAME_RE / PULL_RE)"
participant ConflictCheck
participant FastAPI as "window.roamAlphaAPI.data.fast.q"
participant AsyncAPI as "window.roamAlphaAPI.data.async.q"
participant Remapper as "renamePropsInResult"
Caller->>fireQuery: fireQuery(query, { local: true })
fireQuery->>PropScanner: scan query for namespaced props
PropScanner-->>fireQuery: mapping info (namespaced → original)
fireQuery->>ConflictCheck: check for name collisions in pull blocks
alt no collisions (safe for slow path)
ConflictCheck->>AsyncAPI: call async.q with namespaced query
AsyncAPI-->>Remapper: returns namespaced results
Remapper-->>fireQuery: remapped results (original prop names)
else collisions (must avoid remap ambiguity)
ConflictCheck->>FastAPI: call fast.q with original query
FastAPI-->>fireQuery: returns results (no remapping)
end
fireQuery-->>Caller: return final results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![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
🧹 Nitpick comments (4)
apps/roam/src/utils/fireQuery.ts (4)
34-34: Add documentation for the newlocalflag.Consider adding a JSDoc comment to explain what the
localflag does and how it changes query execution behavior (e.g., uses Roam's local async/fast APIs instead of backend API, with automatic property name remapping).
308-308: Simplify the RegExp declaration.The
RegExpconstructor is redundant when using a regex literal.Apply this diff:
-const PROP_NAME_RE = new RegExp(/\:\w+\/\w+\b/, "g"); +const PROP_NAME_RE = /\:\w+\/\w+\b/g;
353-353: Verify the naming conflict detection logic.The conflict detection checks if the number of unique short names equals the number of namespaced properties:
Object.keys(propNamesSub).length === propNames.size. This assumes that if all short names are unique (no hash collisions), it's safe to useasync.qwith property renaming.However, consider these edge cases:
- Multiple properties with the same namespace could have the same short name:
:ns1/propand:ns2/propboth map to"prop"- The current logic would correctly detect this as a conflict, but ensure this is the intended behavior
The logic appears correct, but it would benefit from a comment explaining the conflict detection strategy.
357-370: Consider extractingrenamePropsas a module-level helper function.The
renamePropsfunction is defined inline within the query execution logic. Extracting it would improve:
- Testability: The recursive property renaming logic could be unit tested independently
- Maintainability: The function could be reused if needed elsewhere
- Readability: Reduces nesting in the already complex query execution flow
Example extraction:
const renamePropsInResult = ( result: json | null, mapping: Record<string, string> ): json | null => { const rename = (x: json | null): json | null => { if (Array.isArray(x)) return x.map(rename); if (x === null || x === undefined) return x; if (typeof x === "object") { return Object.fromEntries( Object.entries(x as object).map(([k, v]) => [ mapping[k] || k, rename(v), ]), ); } return x; }; return rename(result); };Then use it as:
queryResults = renamePropsInResult(queryResults as json, propNamesSub) as unknown[][];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/roam/src/utils/fireQuery.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/main.mdc)
**/*.{ts,tsx}: Prefertypeoverinterface
Use explicit return types for functions
Avoidanytypes when possible
Files:
apps/roam/src/utils/fireQuery.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/main.mdc)
**/*.{ts,tsx,js,jsx}: Prefer arrow functions over regular function declarations
Use named parameters (object destructuring) when a function has more than 2 parameters
Use Prettier with the project's configuration
Maintain consistent naming conventions: PascalCase for components and types
Maintain consistent naming conventions: camelCase for variables and functions
Maintain consistent naming conventions: UPPERCASE for constants
Files:
apps/roam/src/utils/fireQuery.ts
apps/roam/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
apps/roam/**/*.{js,jsx,ts,tsx}: Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Use the roamAlphaApi documentation from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when working with the Roam API
Use Roam Depot/Extension API documentation from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when working with the Roam Extension API
Files:
apps/roam/src/utils/fireQuery.ts
🧠 Learnings (1)
📚 Learning: 2025-06-22T10:40:52.752Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 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/fireQuery.ts
🔇 Additional comments (3)
apps/roam/src/utils/fireQuery.ts (3)
17-17: LGTM!The type import is appropriate for typing the property renaming logic introduced in the local query path.
357-369: Add explicit return type torenamePropsfunction.The inline
renamePropsfunction lacks an explicit return type, which violates the coding guidelines for TypeScript functions.Apply this diff:
- const renameProps = (x: json | null): json | null => { + const renameProps = (x: json | null): json | null => {Wait, looking at the code again, it already seems to have the return type. Let me re-check... Actually, line 357 shows:
const renameProps = (x: json | null): json | null => {. This already has an explicit return type, so this is fine.
345-374: Behavioral differences between async.q and fast.q cannot be verified—recommend manual testing.The web search found no official Roam documentation explicitly describing these APIs. The differences are inferred from community code patterns only.
Beyond the lack of official documentation, the code itself reveals a potential inconsistency: results from
async.qare passed throughrenameProps()to restore namespaced property names, butfast.qresults skip this transformation entirely. This means the two code paths return objects with different property naming conventions, which could break downstream code expecting namespaced names.Before merging, verify:
- Whether both APIs return compatible data structures
- Whether the
fast.qbranch intentionally skips property renaming or if this is a bug- Whether the caller expects consistent property naming regardless of which path executes
maparent
force-pushed
the
eng-1044-experiments-with-datalog-query-modalities
branch
from
0c8157c to
11e54c0
Compare
maparent
force-pushed
the
eng-1044-experiments-with-datalog-query-modalities
branch
from
11e54c0 to
d0580a0
Compare
maparent
force-pushed
the
eng-1044-experiments-with-datalog-query-modalities
branch
from
204b73b to
4f30769
Compare
|
Copying linear comment: I think this could go as-is, but I can think of two things you may want: Copy in the migration panel to tell people to wait, and expect an unresponsive UI We could even have a progression bar, as in the export? Right now I'm using the local async function, and as I said did not run into issues. You seemed to expect issues in that case, may be worth digging into where you saw them, since right now cannot reproduce. Not fully related: when the backend query fails and it retries locally… it is internal to Roam, but it would be nice to capture this and send it to posthog. We could substitute for the error handler, but that's invasive. Can you see a way I cannot? |
|
Loom video showing that local queries work fine (on test-plugin-akamatsulab2): |
Sorry, I don't follow. We could substitute what for the error handler? |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Yes. But this can be a separate task/PR
Probably not worth the effort |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/roam/src/utils/fireQuery.ts (1)
375-398: Local query implementation with property remapping strategy.The implementation introduces two query paths when
localis enabled:
- async.q with remapping (lines 383-390): When property names don't conflict, uses async query and remaps non-namespaced results back
- fast.q fallback (lines 391-394): When conflicts exist, uses synchronous query
Observations:
Type assertions (lines 387-390): Casting to
jsonand back tounknown[][]relies on runtime structure matching the type, which is reasonable given the controlled query context.Conflict detection clarity: The logic
Object.keys(propNamesSub).length === propNames.sizechecks for duplicate non-namespaced property names (e.g., both:block/uidand:node/uidwould conflict on"uid"). A comment explaining this would improve maintainability.Comment on line 376 acknowledges a potential optimization to scan only pull patterns rather than the entire query string.
Given the experimental nature of this feature (per PR objectives), this implementation provides a solid foundation for exploring local query modalities.
Optional: Add clarifying comment for the conflict detection logic
+ // Check if all non-namespaced names are unique (e.g., no conflict between :block/uid and :node/uid) if (Object.keys(propNamesSub).length === propNames.size) {
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/roam/src/utils/fireQuery.tsapps/roam/src/utils/getExportTypes.tsapps/roam/src/utils/getRelationData.tsapps/roam/src/utils/migrateRelations.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/fireQuery.tsapps/roam/src/utils/getRelationData.tsapps/roam/src/utils/getExportTypes.tsapps/roam/src/utils/migrateRelations.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/fireQuery.tsapps/roam/src/utils/getRelationData.tsapps/roam/src/utils/getExportTypes.tsapps/roam/src/utils/migrateRelations.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/fireQuery.tsapps/roam/src/utils/getRelationData.tsapps/roam/src/utils/getExportTypes.tsapps/roam/src/utils/migrateRelations.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/fireQuery.tsapps/roam/src/utils/getRelationData.tsapps/roam/src/utils/getExportTypes.tsapps/roam/src/utils/migrateRelations.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/fireQuery.tsapps/roam/src/utils/getRelationData.tsapps/roam/src/utils/getExportTypes.tsapps/roam/src/utils/migrateRelations.ts
🧠 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/utils/fireQuery.ts
📚 Learning: 2025-06-22T10:40:52.752Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 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/fireQuery.ts
📚 Learning: 2025-12-19T16:59:40.640Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-12-19T16:59:40.640Z
Learning: In this codebase, type assertions at system boundaries (e.g., Roam datalog queries) are acceptable when the output shape is known from the query structure, such as when using `as Array<[string, string]>` after datalog queries with `:find ?uid ?title` clauses.
Applied to files:
apps/roam/src/utils/fireQuery.ts
🧬 Code graph analysis (2)
apps/roam/src/utils/fireQuery.ts (1)
apps/roam/src/utils/getBlockProps.ts (1)
json(1-7)
apps/roam/src/utils/getExportTypes.ts (1)
apps/roam/src/utils/getRelationData.ts (1)
getRelationDataUtil(9-67)
🔇 Additional comments (6)
apps/roam/src/utils/migrateRelations.ts (1)
23-23: LGTM - Local mode enabled for migration.The change enables local query mode for relation data retrieval during migration. This aligns with the PR's objective to experiment with different datalog query modalities.
apps/roam/src/utils/getExportTypes.ts (1)
377-378: LGTM - Consistent local mode integration for exports.The addition of
local: trueproperly propagates the local query flag through the export data flow, enabling local query execution for relation data during exports.apps/roam/src/utils/getRelationData.ts (2)
9-17: LGTM - Well-structured options object refactoring.The refactoring from positional parameters to an options object with the optional
localflag improves maintainability and follows TypeScript best practices.
32-32: LGTM - Proper flag propagation through call chain.The
localflag is correctly threaded throughgetRelationData→getRelationDataUtil→fireQuery, maintaining type safety and API consistency.Also applies to: 69-76
apps/roam/src/utils/fireQuery.ts (2)
17-17: LGTM - Appropriate type import.The type-only import of
jsonfromgetBlockPropsproperly supports the typing of the property renaming logic.
31-40: LGTM - Type extension for local flag.The addition of
local?: booleantoFireQueryArgsis properly typed and maintains backward compatibility.
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
A few clarifying questions. Also, I ran coderabbit to check the new changes, could you please address it's comment?
maparent
force-pushed
the
eng-1044-experiments-with-datalog-query-modalities
branch
from
e20bcb9 to
0ae0f52
Compare
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/roam/src/utils/fireQuery.ts (1)
378-408: Consider simplifying or documenting the disabled slow path.The
preferSlow = falseflag on line 378 makes lines 382-398 and 400-408 effectively unreachable. While the past review comments indicate this is intentionally preserved for future comprehensive testing, the code could be clearer for maintainability:
- Option 1: Add a more prominent comment explaining why this code is preserved but disabled
- Option 2: Remove the dead code and restore it from git history when global query testing is ready
- Option 3: Wrap it in a feature flag or environment variable if you expect to enable it soon
The
propNamesSub !== undefinedcheck on line 404 is also misleading sincepropNamesSubcan only be set inside theif (preferSlow)block.Example: More explicit preservation comment
- // for now assume fast results are equivalent to slow. - // We have seen some errors in (fast) backend queries, but not so far in local fast ones. - const preferSlow = false; + // NOTE: The slow path (async.q with property remapping) is intentionally disabled + // but preserved for potential future use pending comprehensive query testing. + // See discussion: https://github.com/DiscourseGraphs/discourse-graph/pull/539 + // When enabled, it handles name conflicts in pull clauses by remapping properties. + const preferSlow = false;
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/roam/src/utils/fireQuery.tsapps/roam/src/utils/getExportTypes.tsapps/roam/src/utils/getRelationData.tsapps/roam/src/utils/migrateRelations.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/roam/src/utils/getExportTypes.ts
- apps/roam/src/utils/getRelationData.ts
- apps/roam/src/utils/migrateRelations.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/fireQuery.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/fireQuery.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/fireQuery.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/fireQuery.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/fireQuery.ts
🧠 Learnings (9)
📚 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/fireQuery.ts
📚 Learning: 2025-08-25T15:53:21.799Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 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/fireQuery.ts
📚 Learning: 2025-11-23T23:53:43.094Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-11-23T23:53:43.094Z
Learning: In `apps/roam/src/utils/supabaseContext.ts`, the Roam URL normalization uses a targeted string replacement `url.replace("/?server-port=3333#/", "/#/")` rather than URL parsing to avoid impacting unforeseen URL patterns. This conservative approach is intentional to handle only the specific known case.
Applied to files:
apps/roam/src/utils/fireQuery.ts
📚 Learning: 2025-06-22T10:40:52.752Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 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/fireQuery.ts
📚 Learning: 2025-11-25T00:52:27.779Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/main.mdc:0-0
Timestamp: 2025-11-25T00:52:27.779Z
Learning: Applies to **/*.{ts,tsx} : Avoid `any` types when possible
Applied to files:
apps/roam/src/utils/fireQuery.ts
📚 Learning: 2025-09-01T19:09:07.052Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-09-01T19:09:07.052Z
Learning: User prefers to avoid making types explicit in each require statement due to anti-DRY concerns when working with module imports in TypeScript.
Applied to files:
apps/roam/src/utils/fireQuery.ts
📚 Learning: 2025-11-25T00:52:27.779Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/main.mdc:0-0
Timestamp: 2025-11-25T00:52:27.779Z
Learning: Applies to **/*.{ts,tsx} : Use explicit return types for functions
Applied to files:
apps/roam/src/utils/fireQuery.ts
📚 Learning: 2025-11-25T00:52:27.779Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/main.mdc:0-0
Timestamp: 2025-11-25T00:52:27.779Z
Learning: Applies to **/*.{ts,tsx} : Prefer `type` over `interface` in TypeScript
Applied to files:
apps/roam/src/utils/fireQuery.ts
📚 Learning: 2025-12-19T16:59:40.640Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-12-19T16:59:40.640Z
Learning: In this codebase, type assertions at system boundaries (e.g., Roam datalog queries) are acceptable when the output shape is known from the query structure, such as when using `as Array<[string, string]>` after datalog queries with `:find ?uid ?title` clauses.
Applied to files:
apps/roam/src/utils/fireQuery.ts
🧬 Code graph analysis (1)
apps/roam/src/utils/fireQuery.ts (1)
apps/roam/src/utils/getBlockProps.ts (1)
json(1-7)
🔇 Additional comments (4)
apps/roam/src/utils/fireQuery.ts (4)
17-17: LGTM: Clean type additions for local query support.The import of the
jsontype and the addition of thelocal?: booleanflag toFireQueryArgsare well-integrated and support the new local query functionality without breaking existing code.Also applies to: 34-34
319-320: LGTM: Regex patterns correctly handle hyphenated properties.The
PROP_NAME_REpattern now correctly matches hyphenated property names (e.g.,:block/text-align,:user/display-name) as recommended in the previous review. The patterns are appropriate for extracting namespace/property patterns and pull expressions from datalog queries.
322-340: LGTM: Well-structured recursive property renaming.The
renamePropsInResulthelper correctly handles all JSON value types with proper recursion. The explicit return type and clean implementation follow the coding guidelines.
379-417: Local query implementation is functional and correct.The local query path correctly uses
async.fast.qfor local execution and preserves the existingbackend.qpath for non-local queries. The branching logic works as intended.
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
I want to clarify intent here, since I think there was some understandable misalignment.
This PR originally proposed broader query changes, which were out of scope at the time. In the recent product meeting, when the reified relations migration came up and we discussed a local flag, what I meant specifically was:
we can add a
localflag so the reified-relations migration can run local queries instead of backend queries
I did not mean to reopen the broader query refactor or introduce additional execution paths.
What needs to change in this PR
For this PR to land, it needs to be reduced to a very narrow scope:
-
Add
local?: boolean -
When
local === true, use:roamAlphaAPI.data.async.fast.q
-
Otherwise, preserve existing backend behavior
Everything else (slow paths, property renaming, regex inspection, future fallbacks, commented logic) should be removed.
Why this matters
The additional logic introduces:
- dead or disabled paths
- speculative future handling
- unnecessary cognitive overhead for reviewers and maintainers
If we later find real issues with fast.q, we can address them in a separate, focused PR with concrete evidence.
Summary
I’m aligned on the use case, and I acknowledge my wording in the meeting may have been ambiguous. That said, this PR still needs to be significantly narrowed before merge.
Please cut this down to the minimal local flag + async.fast.q change (≈10–20 LOC), then re-request review.
Alternatively, if you’d like to keep this as an experimental or exploratory effort, please leave this PR as draft and open a separate ticket specifically for introducing a local flag for the reified relations migration use case.
|
I knew that's what you want. I'm pushing back in this case. We have bugs we don't understand yet, we can't track them that well, and until we do, we have actually reasons to believe that this currently dead code might turn out to be useful. You've said a few times it's just something that can live in git, but with our policy of squashing and killing branches, if I don't leave it in the code it's gone from git. (I could keep the code privately, but that means nobody else can discover that code.) I'm aware it's making your life a bit more difficult, but we shared the decision to delay the testing infrastructure, and until it exists there are decisions we do not know enough to take, and I think they include killing that code. |
|
That said… I agree that I can leave this as a draft in a separate PR. |
maparent
force-pushed
the
eng-1044-experiments-with-datalog-query-modalities
branch
from
e70c60e to
f3637d0
Compare
maparent
changed the base branch from
main
to
eng-1208-add-local-flag-to-firequery
|
Made it into a separate PR as requested; I agree it's a cleaner separation of concerns. I will keep the branch around. |
maparent
force-pushed
the
eng-1044-experiments-with-datalog-query-modalities
branch
from
f3637d0 to
4db72af
Compare
https://linear.app/discourse-graphs/issue/ENG-1044/experiments-with-different-datalog-query-modalities
This contains code that would allow to use the slow vs fast async catalog queries, if it turns out to have an impact in accuracy after we have a proper testing harness.