perf: optimize node DB queries with batching, indexes, and SQLite PRAGMAs#59
perf: optimize node DB queries with batching, indexes, and SQLite PRAGMAs#59
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughEnable WAL and performance PRAGMAs on the master DB, add two indices; introduce server-side supertag ID caching; implement batched, parallel node assembly and refactor query evaluators and node operations to operate on bulk candidate sets with fewer queries. Changes
Sequence DiagramsequenceDiagram
participant QE as QueryEvaluator
participant NS as NodeService
participant SB as SurrealBackend
participant DB as Database
QE->>NS: assembleNodes(db, candidateNodeIds)
NS->>DB: Batch fetch nodes (inArray nodeIds)
DB-->>NS: nodes[]
NS->>DB: Batch fetch node_properties (inArray nodeIds)
DB-->>NS: properties[]
NS->>DB: Batch fetch fields/meta (inArray fieldIds)
DB-->>NS: fields[]
NS->>SB: Request supertag resolutions (system IDs)
SB->>SB: consult supertagIdCache
alt cache miss
SB->>DB: query supertag records
DB-->>SB: supertag records
SB->>SB: store in cache
end
SB-->>NS: supertag ID mappings
NS->>NS: In-memory join & assemble nodes
NS-->>QE: AssembledNode[]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
libs/nxus-db/src/services/backends/surreal-backend.ts (1)
89-91: Unbounded cache may retain stale null entries.The
supertagIdCachehas no size limit or TTL. For long-running processes, this could grow indefinitely. More importantly, cachingnull(line 160) means that if a supertag is created after an initial lookup failure, the cache will continue returningnulluntil the process restarts.If supertags are created dynamically at runtime, consider either:
- Adding a cache invalidation hook when supertags are created.
- Not caching
nullvalues (only cache positive lookups).For static/bootstrap-time supertags, this is fine as-is.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/nxus-db/src/services/backends/surreal-backend.ts` around lines 89 - 91, The supertagIdCache Map in surreal-backend.ts currently stores nulls and is unbounded; stop caching negative lookups and/or add invalidation/TTL: update logic around where supertagIdCache is written (references to supertagIdCache and the lookup path that sets null) to only set entries when a non-null string RecordId is obtained, and either implement a simple eviction/size limit or expose an invalidation hook that is called when a supertag is created/updated so callers can delete the corresponding key from supertagIdCache; ensure no code path relies on cached nulls after this change.libs/nxus-db/src/services/node.service.ts (1)
933-951: Consider selective fetch for large datasets.This pattern fetches ALL supertag property assignments (line 933-937), then filters in memory. For large databases with many nodes, this could load significant data into memory.
The tradeoff is acceptable for avoiding N+1 queries, but if performance issues arise with large datasets, consider:
- Using
inArraywith the target supertag values directly in the WHERE clause.- Adding pagination or limits.
For now, this is fine — the optimization aligns with the PR's goals.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/nxus-db/src/services/node.service.ts` around lines 933 - 951, The current code loads all supertag property rows into memory (allSupertagProps from nodeProperties where fieldNodeId = supertagField.id) and filters them in JS against allSupertagIds, which can OOM for large datasets; change the query to push filtering into the DB by using an IN-style condition (e.g., inArray or equivalent) on the parsed property value or the raw stored value to match the values in allSupertagIds, and/or add pagination/limits to the fetch, updating the logic that builds nodeIdSet accordingly so only matching rows are returned by the DB instead of post-filtered in memory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@libs/nxus-db/src/services/backends/surreal-backend.ts`:
- Around line 89-91: The supertagIdCache Map in surreal-backend.ts currently
stores nulls and is unbounded; stop caching negative lookups and/or add
invalidation/TTL: update logic around where supertagIdCache is written
(references to supertagIdCache and the lookup path that sets null) to only set
entries when a non-null string RecordId is obtained, and either implement a
simple eviction/size limit or expose an invalidation hook that is called when a
supertag is created/updated so callers can delete the corresponding key from
supertagIdCache; ensure no code path relies on cached nulls after this change.
In `@libs/nxus-db/src/services/node.service.ts`:
- Around line 933-951: The current code loads all supertag property rows into
memory (allSupertagProps from nodeProperties where fieldNodeId =
supertagField.id) and filters them in JS against allSupertagIds, which can OOM
for large datasets; change the query to push filtering into the DB by using an
IN-style condition (e.g., inArray or equivalent) on the parsed property value or
the raw stored value to match the values in allSupertagIds, and/or add
pagination/limits to the fetch, updating the logic that builds nodeIdSet
accordingly so only matching rows are returned by the DB instead of
post-filtered in memory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a4e1851d-6ca8-4469-82be-c3db0aedf879
📒 Files selected for processing (6)
libs/nxus-db/src/client/master-client.tslibs/nxus-db/src/schemas/node-schema.tslibs/nxus-db/src/services/backends/surreal-backend.tslibs/nxus-db/src/services/bootstrap.tslibs/nxus-db/src/services/node.service.tslibs/nxus-db/src/services/query-evaluator.service.ts
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libs/nxus-db/src/services/backends/surreal-backend.ts (1)
89-90: Cache lacks invalidation; may serve stale results after supertag schema changes.The
supertagIdCachecaches resolved IDs (includingnullfor not-found) but has no invalidation mechanism. If a supertag is created or deleted at runtime, the cache will return stale data.For schema-like entities that change infrequently, this is likely acceptable. If runtime supertag mutations become common, consider:
- Cache invalidation on supertag create/delete events
- TTL-based expiration
- Exposing a
clearCaches()method for testsAlso applies to: 149-161
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d9e2202b-7a51-4c0c-b569-09a454e3438f
📒 Files selected for processing (1)
libs/nxus-db/src/services/backends/surreal-backend.ts
- Don't cache null results in supertagIdCache to avoid permanently missing supertags created after a failed lookup - Fix temporal filter 'after' operator to use strict '>' instead of '>=' to match SQLite evaluator behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/nxus-db/src/services/backends/surreal-backend.ts`:
- Around line 955-959: The code currently calls assembleNode for every id in
candidateIds concurrently before applying the limit, which can spawn thousands
of parallel Surreal reads; change the implementation so you enforce the
concurrency cap before launching work (e.g., process candidateIds in chunks of
size limit or use a concurrency-controlled mapper like p-map/promise-pool) and
for each chunk call this.assembleNode(nid) and await the batch before continuing
to the next chunk; keep the same post-processing (filtering nulls into
assembledNodes) and reference the assembleNode method, the candidateIds
collection, the assembleResults variable, and the limit parameter when making
the change.
- Around line 745-750: The current code filters out nulls from resolveResults
which breaks matchAll semantics; update the logic around
resolveResults/resolveSupertagId so that if matchAll is true and any entry in
resolveResults is null (i.e., a requested supertag is missing) you immediately
return [] instead of filtering them out, otherwise (when matchAll is false)
continue filtering nulls into supertagRecordIds as before; ensure you reference
resolveResults, supertagRecordIds, resolveSupertagId and the matchAll flag when
making the change.
- Around line 1018-1048: getNodeIdsBySupertagWithInheritance currently only
fetches direct children of targetRecordId; change the collection logic
(allSupertagIds / children query) to walk the extends relationship transitively
(BFS/DFS) until no new supertag ids are discovered. Use resolveSupertagId to
seed the queue with targetRecordId, repeatedly query 'SELECT in AS child_ref
FROM extends WHERE out = $stId' for each popped id, add rid(child.child_ref) to
allSupertagIds and enqueue newly found ids, then continue to the existing
parallel has_supertag queries using the fully populated allSupertagIds set so
grandchildren/great-grandchildren are included. Ensure you still handle empty
results and preserve use of StringRecordId and rid(...) conversions as in the
original code.
- Around line 1228-1233: The temporal query currently passes
targetDate.toISOString() as a string to db.query (see surrealField, surrealOp
and matchingNodes/db.query) which fails on SurrealDB v2 and will throw if
targetDate is invalid; validate targetDate is a valid Date (e.g., check
!isNaN(targetDate.getTime())) before serializing, and pass the parameter with an
explicit <datetime> cast in the SQL (e.g., use $targetDate::<datetime>) so the
server compares datetime types correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24822884-7132-472e-b10f-60702f6cd966
📒 Files selected for processing (1)
libs/nxus-db/src/services/backends/surreal-backend.ts
| // Build SurQL temporal filter to push filtering to the server | ||
| const surrealField = field === 'createdAt' ? 'created_at' : 'updated_at' | ||
| const surrealOp = op === 'within' ? '>=' : op === 'after' ? '>' : '<' | ||
| const [matchingNodes] = await db.query<[Array<{ id: RecordId }>]>( | ||
| `SELECT id FROM node WHERE deleted_at IS NONE AND ${surrealField} ${surrealOp} $targetDate`, | ||
| { targetDate: targetDate.toISOString() }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
site:surrealdb.com SurrealDB v2 datetime strings no longer eagerly convert <datetime> cast parameters
💡 Result:
In SurrealDB / SurrealQL v2.0.0 and later, plain quoted strings are always string and are not automatically (“eagerly”) converted into datetime (even if they look like RFC 3339 timestamps). You must explicitly mark/convert them, e.g.:
- Use a datetime literal prefix:
d"2025-07-03T07:18:52Z"(parser-level) (surrealdb.com) - Or cast from a string:
<datetime>"2025-07-03T07:18:52.841147Z"(database-level conversion; invalid input errors) (surrealdb.com)
This “no eager conversion” rule is documented explicitly for datetimes in the Datetimes page and more generally in the Strings page (v2.0 behavior change). (surrealdb.com)
Sources
- [1] Datetimes | SurrealQL (surrealdb.com)
- [2] Strings | SurrealQL (surrealdb.com)
- [3] Casting | SurrealQL (surrealdb.com)
Citations:
- 1: https://surrealdb.com/docs/surrealql/datamodel/datetimes
- 2: https://surrealdb.com/docs/surrealql/datamodel/datetimes
- 3: https://surrealdb.com/docs/surrealql/datamodel/datetimes?utm_source=openai
- 4: https://surrealdb.com/docs/surrealql/datamodel/datetimes
- 5: https://surrealdb.com/docs/surrealql/datamodel/strings
- 6: https://surrealdb.com/docs/surrealql/datamodel/casting
🏁 Script executed:
# First, let's check the file exists and get its size
wc -l libs/nxus-db/src/services/backends/surreal-backend.tsRepository: popemkt/nxus
Length of output: 114
🏁 Script executed:
# Read the specific section around lines 1228-1233
sed -n '1210,1250p' libs/nxus-db/src/services/backends/surreal-backend.tsRepository: popemkt/nxus
Length of output: 1392
🏁 Script executed:
# Let's look for the function containing this code and understand the targetDate parameter
sed -n '1180,1240p' libs/nxus-db/src/services/backends/surreal-backend.tsRepository: popemkt/nxus
Length of output: 2008
Add <datetime> cast to $targetDate parameter and validate date before serializing.
SurrealDB v2 does not eagerly convert strings to datetime types. The current code passes targetDate.toISOString() as a string parameter without explicit casting, which will cause type mismatch errors when comparing against datetime fields.
Additionally, calling .toISOString() on an invalid Date object (which can occur when new Date(date) receives an invalid date string) will throw a runtime error. Add validation to ensure targetDate is a valid date before calling .toISOString().
💡 Suggested fix
const [matchingNodes] = await db.query<[Array<{ id: RecordId }>]>(
- `SELECT id FROM node WHERE deleted_at IS NONE AND ${surrealField} ${surrealOp} $targetDate`,
+ `SELECT id FROM node WHERE deleted_at IS NONE AND ${surrealField} ${surrealOp} <datetime>$targetDate`,
{ targetDate: targetDate.toISOString() },
)Also add validation before the toISOString() call to handle invalid dates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/nxus-db/src/services/backends/surreal-backend.ts` around lines 1228 -
1233, The temporal query currently passes targetDate.toISOString() as a string
to db.query (see surrealField, surrealOp and matchingNodes/db.query) which fails
on SurrealDB v2 and will throw if targetDate is invalid; validate targetDate is
a valid Date (e.g., check !isNaN(targetDate.getTime())) before serializing, and
pass the parameter with an explicit <datetime> cast in the SQL (e.g., use
$targetDate::<datetime>) so the server compares datetime types correctly.
- Change PropertyValue.value from `any` to `unknown` - Use canonical filter types (SupertagFilter, PropertyFilter, etc.) in surreal-backend instead of loose inline structural types - Widen resolveFieldId to accept `string`, removing unsafe `as FieldSystemId` casts - Use FilterOp literal union in compareValues instead of `string` - Remove unnecessary `as string` casts on FieldSystemId (branded type already extends string) - Replace non-null assertions with safe Map access patterns - Add type predicate to getNodeSupertagSystemIds filter Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| if (!results || results.length === 0) { | ||
| return null |
There was a problem hiding this comment.
🟡 supertagIdCache never caches null (negative) lookups, defeating cache purpose
The newly introduced supertagIdCache (typed Map<string, string | null> at surreal-backend.ts:100) is designed to cache both positive and negative lookup results—the has()-based lookup pattern at line 159-160 correctly handles cached null values. However, when resolveSupertagId finds no matching supertag (lines 169-170), it returns null without storing it in the cache. This means every call for a non-existent supertag system ID will always hit the database, which undermines the caching optimization this PR introduces. This is especially impactful since resolveSupertagId is called in hot paths like evaluateSupertagFilter, getNodesBySupertags, and getNodeIdsBySupertagWithInheritance—all of which may repeatedly query the same non-existent supertag IDs.
| if (!results || results.length === 0) { | |
| return null | |
| if (!results || results.length === 0) { | |
| this.supertagIdCache.set(supertagSystemId, null) | |
| return null | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libs/nxus-db/src/services/bootstrap.ts (1)
25-36: Consider exporting a cache-clearing function for test isolation.The
systemNodeIdscache persists across calls but has no clear function. UnlikeclearSystemNodeCache()innode.service.ts, tests that reset the database cannot clear this cache, potentially causing stale UUID references.♻️ Suggested addition
// UUID cache for system nodes (systemId -> UUID) const systemNodeIds = new Map<string, string>(); +/** + * Clear the system node ID cache (call between test runs) + */ +export function clearBootstrapCache(): void { + systemNodeIds.clear(); +} + /** * Get or create UUID for a system node */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/nxus-db/src/services/bootstrap.ts` around lines 25 - 36, Add a cache-clear helper to allow tests to reset the in-memory map used by getSystemNodeId: export a function (e.g., clearSystemNodeIdsCache or resetSystemNodeIds) that calls systemNodeIds.clear(); update exports to expose it so test suites can call it for isolation; keep the existing getSystemNodeId and systemNodeIds names unchanged so callers and references remain valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@libs/nxus-db/src/services/bootstrap.ts`:
- Around line 25-36: Add a cache-clear helper to allow tests to reset the
in-memory map used by getSystemNodeId: export a function (e.g.,
clearSystemNodeIdsCache or resetSystemNodeIds) that calls systemNodeIds.clear();
update exports to expose it so test suites can call it for isolation; keep the
existing getSystemNodeId and systemNodeIds names unchanged so callers and
references remain valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b0b649a-4109-43f4-b189-3af44c5bbd38
📒 Files selected for processing (4)
libs/nxus-db/src/services/backends/surreal-backend.tslibs/nxus-db/src/services/bootstrap.tslibs/nxus-db/src/services/node.service.tslibs/nxus-db/src/types/node.ts
- Return [] early when matchAll=true and any supertag is missing - Walk extends relationship transitively (BFS) instead of single level for correct deep inheritance queries - Apply limit before assembly to avoid assembling thousands of nodes - Add <datetime> cast to temporal filter for SurrealDB v2 compatibility Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| // Apply limit before assembly to avoid assembling thousands of nodes | ||
| const limit = definition.limit ?? 500 | ||
| const idsToAssemble = [...candidateIds].slice(0, limit) | ||
|
|
||
| // Assemble nodes in parallel (capped by limit) | ||
| const assembleResults = await Promise.all( | ||
| idsToAssemble.map((nid) => this.assembleNode(nid)), | ||
| ) | ||
| let assembledNodes = assembleResults.filter((n): n is AssembledNode => n !== null) | ||
|
|
||
| // Apply sorting | ||
| if (definition.sort) { | ||
| assembledNodes = this.sortNodes(assembledNodes, definition.sort) | ||
| } |
There was a problem hiding this comment.
🔴 SurrealBackend.evaluateQuery applies limit before sort, returning incorrectly ordered results
In evaluateQuery, the limit is applied at line 971 (idsToAssemble = [...candidateIds].slice(0, limit)) before sorting happens at line 980-982. This means when a query has both sort and limit, an arbitrary subset of limit nodes is assembled and then sorted — instead of sorting all candidates first and returning the top N. This produces incorrect results: e.g., a query for "latest 10 items by createdAt desc" would sort a random 500 items rather than returning the actual top 10.
The SQLite counterpart in query-evaluator.service.ts:96-107 correctly assembles all candidates, sorts them, and only then applies the limit.
| // Apply limit before assembly to avoid assembling thousands of nodes | |
| const limit = definition.limit ?? 500 | |
| const idsToAssemble = [...candidateIds].slice(0, limit) | |
| // Assemble nodes in parallel (capped by limit) | |
| const assembleResults = await Promise.all( | |
| idsToAssemble.map((nid) => this.assembleNode(nid)), | |
| ) | |
| let assembledNodes = assembleResults.filter((n): n is AssembledNode => n !== null) | |
| // Apply sorting | |
| if (definition.sort) { | |
| assembledNodes = this.sortNodes(assembledNodes, definition.sort) | |
| } | |
| // Assemble nodes in parallel (filter out deleted) | |
| const assembleResults = await Promise.all( | |
| [...candidateIds].map((nid) => this.assembleNode(nid)), | |
| ) | |
| let assembledNodes = assembleResults.filter((n): n is AssembledNode => n !== null) | |
| // Apply sorting before limit so we get the correct top-N results | |
| if (definition.sort) { | |
| assembledNodes = this.sortNodes(assembledNodes, definition.sort) | |
| } | |
| // Apply limit after sorting | |
| const limit = definition.limit ?? 500 | |
| if (assembledNodes.length > limit) { | |
| assembledNodes = assembledNodes.slice(0, limit) | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/nxus-db/src/services/backends/surreal-backend.ts (1)
1237-1256:⚠️ Potential issue | 🟠 MajorValidate temporal filter dates before serializing them.
new Date(date)can produceInvalid Date, and the nexttoISOString()will throw. This filter comes from external query input, so it needs an explicit validity check before the Surreal query.💡 Minimal guard
case 'before': case 'after': if (!date) return candidateIds targetDate = new Date(date) + if (Number.isNaN(targetDate.getTime())) { + throw new Error(`Invalid temporal filter date: ${String(date)}`) + } breakAs per coding guidelines, "Validate external inputs using Zod schemas before processing" and "Prioritize handling edge cases including errors, empty states, and loading states".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/nxus-db/src/services/backends/surreal-backend.ts` around lines 1237 - 1256, The temporal filter may create an invalid Date (targetDate) from external input; before calling targetDate.toISOString() or running db.query in the function handling op/days/date, validate targetDate (e.g., after assigning targetDate, check Number.isFinite(targetDate.getTime()) or !isNaN(targetDate.getTime())) and if invalid return candidateIds (or bail out consistently) instead of proceeding; update the block that sets targetDate (cases 'within'/'before'/'after') to perform this validity check right before building surrealField/surrealOp and invoking db.query (symbols: op, days, date, targetDate, surrealField, surrealOp, db.query, matchingNodes).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/nxus-db/src/services/backends/surreal-backend.ts`:
- Around line 801-805: The current code calls assembleNode for every id in
matchingNodeIds via Promise.all, which can spawn thousands of concurrent Surreal
reads; change both occurrences (the assembleResults = await Promise.all(...)
blocks around matchingNodeIds) to use a concurrency-limited approach: either
process matchingNodeIds in fixed-size chunks and run Promise.all per chunk, or
use a small concurrency queue/limiter (e.g., p-limit or an in-repo helper) to
call this. Ensure you still await all tasks, collect results, and then filter
out nulls as before (i.e., keep the final filter for AssembledNode). Use the
existing assembleNode function as the task and replace direct
Promise.all(matchingNodeIds.map(...)) with the limiter/chunked execution in both
places mentioned.
- Around line 969-977: The current code slices candidateIds to limit before
assembling, which breaks correct ordering when definition.sort is present;
change the logic in the method that builds assembleResults/assembledNodes so
that when definition.sort exists you assemble (or at least fetch sort keys for)
all candidateIds, then apply the sort comparator based on definition.sort to
assembledNodes and only after sorting apply the limit (definition.limit ?? 500);
for the no-sort case you may keep the existing early slice optimization, and
ensure you still filter nulls from assembleNode results into assembledNodes
before sorting/trimming.
---
Outside diff comments:
In `@libs/nxus-db/src/services/backends/surreal-backend.ts`:
- Around line 1237-1256: The temporal filter may create an invalid Date
(targetDate) from external input; before calling targetDate.toISOString() or
running db.query in the function handling op/days/date, validate targetDate
(e.g., after assigning targetDate, check Number.isFinite(targetDate.getTime())
or !isNaN(targetDate.getTime())) and if invalid return candidateIds (or bail out
consistently) instead of proceeding; update the block that sets targetDate
(cases 'within'/'before'/'after') to perform this validity check right before
building surrealField/surrealOp and invoking db.query (symbols: op, days, date,
targetDate, surrealField, surrealOp, db.query, matchingNodes).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bf227c9d-cb41-49e7-b09c-5f30f76b8d1e
📒 Files selected for processing (1)
libs/nxus-db/src/services/backends/surreal-backend.ts
| // Assemble nodes in parallel (filter out deleted) | ||
| const assembleResults = await Promise.all( | ||
| matchingNodeIds.map((nid) => this.assembleNode(nid)), | ||
| ) | ||
| return assembleResults.filter((n): n is AssembledNode => n !== null) |
There was a problem hiding this comment.
Cap parallel assembly in the supertag lookup paths.
Both methods now fire one assembleNode() per match via Promise.all(). On broad supertags, that can become thousands of concurrent Surreal reads and undo the perf gains from the rest of this PR. Please chunk these IDs or funnel them through a concurrency-limited helper before assembling.
Also applies to: 819-822
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/nxus-db/src/services/backends/surreal-backend.ts` around lines 801 -
805, The current code calls assembleNode for every id in matchingNodeIds via
Promise.all, which can spawn thousands of concurrent Surreal reads; change both
occurrences (the assembleResults = await Promise.all(...) blocks around
matchingNodeIds) to use a concurrency-limited approach: either process
matchingNodeIds in fixed-size chunks and run Promise.all per chunk, or use a
small concurrency queue/limiter (e.g., p-limit or an in-repo helper) to call
this. Ensure you still await all tasks, collect results, and then filter out
nulls as before (i.e., keep the final filter for AssembledNode). Use the
existing assembleNode function as the task and replace direct
Promise.all(matchingNodeIds.map(...)) with the limiter/chunked execution in both
places mentioned.
| // Apply limit before assembly to avoid assembling thousands of nodes | ||
| const limit = definition.limit ?? 500 | ||
| const idsToAssemble = [...candidateIds].slice(0, limit) | ||
|
|
||
| // Assemble nodes in parallel (capped by limit) | ||
| const assembleResults = await Promise.all( | ||
| idsToAssemble.map((nid) => this.assembleNode(nid)), | ||
| ) | ||
| let assembledNodes = assembleResults.filter((n): n is AssembledNode => n !== null) |
There was a problem hiding this comment.
Preserve sort semantics before trimming results.
Applying limit before assembly is only safe when no sort is requested. With definition.sort, this slices an arbitrary subset of candidateIds first and then sorts only that subset, so limit + sort can miss the true top N rows.
💡 Safe fallback
const totalCount = candidateIds.size
// Apply limit before assembly to avoid assembling thousands of nodes
const limit = definition.limit ?? 500
-const idsToAssemble = [...candidateIds].slice(0, limit)
+const idsToAssemble = definition.sort
+ ? [...candidateIds]
+ : [...candidateIds].slice(0, limit)
// Assemble nodes in parallel (capped by limit)
const assembleResults = await Promise.all(
idsToAssemble.map((nid) => this.assembleNode(nid)),
)
let assembledNodes = assembleResults.filter((n): n is AssembledNode => n !== null)
// Apply sorting
if (definition.sort) {
assembledNodes = this.sortNodes(assembledNodes, definition.sort)
}
+if (definition.limit !== undefined) {
+ assembledNodes = assembledNodes.slice(0, limit)
+}📝 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.
| // Apply limit before assembly to avoid assembling thousands of nodes | |
| const limit = definition.limit ?? 500 | |
| const idsToAssemble = [...candidateIds].slice(0, limit) | |
| // Assemble nodes in parallel (capped by limit) | |
| const assembleResults = await Promise.all( | |
| idsToAssemble.map((nid) => this.assembleNode(nid)), | |
| ) | |
| let assembledNodes = assembleResults.filter((n): n is AssembledNode => n !== null) | |
| const totalCount = candidateIds.size | |
| // Apply limit before assembly to avoid assembling thousands of nodes | |
| const limit = definition.limit ?? 500 | |
| const idsToAssemble = definition.sort | |
| ? [...candidateIds] | |
| : [...candidateIds].slice(0, limit) | |
| // Assemble nodes in parallel (capped by limit) | |
| const assembleResults = await Promise.all( | |
| idsToAssemble.map((nid) => this.assembleNode(nid)), | |
| ) | |
| let assembledNodes = assembleResults.filter((n): n is AssembledNode => n !== null) | |
| // Apply sorting | |
| if (definition.sort) { | |
| assembledNodes = this.sortNodes(assembledNodes, definition.sort) | |
| } | |
| if (definition.limit !== undefined) { | |
| assembledNodes = assembledNodes.slice(0, limit) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/nxus-db/src/services/backends/surreal-backend.ts` around lines 969 -
977, The current code slices candidateIds to limit before assembling, which
breaks correct ordering when definition.sort is present; change the logic in the
method that builds assembleResults/assembledNodes so that when definition.sort
exists you assemble (or at least fetch sort keys for) all candidateIds, then
apply the sort comparator based on definition.sort to assembledNodes and only
after sorting apply the limit (definition.limit ?? 500); for the no-sort case
you may keep the existing early slice optimization, and ensure you still filter
nulls from assembleNode results into assembledNodes before sorting/trimming.
Theme flash fixes: - Add useThemeHydrated() hook to wait for Zustand persist hydration before applying theme classes (prevents stripping inline script's correct classes with store defaults) - ThemeChooser returns null until store has hydrated, preventing wrong selection flash - Gateway visual: defer localStorage read to useEffect to avoid SSR hydration mismatch Dev port fixes: - Add server.strictPort: true to all 5 vite.config.ts files so Vite errors instead of silently picking random ports - Add server.port to each config (3000-3004) as source of truth - Add predev script to kill stale processes on ports 3000-3004 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move theme application from useEffect to module scope across all 5 apps. This runs synchronously before React renders, preventing the flash caused by useEffect stripping classes set by the inline script. Three-layer approach (same as Automaker): 1. Inline <script> in <head> — applies classes during HTML parsing 2. Module-scope applyStoredTheme() — catches SPA navigations 3. useEffect — only handles cross-tab sync via storage events For nxus-core, the Zustand-based useEffect is kept (gated on hydration) for reactive theme changes from the UI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| 'brutalism', | ||
| ] |
There was a problem hiding this comment.
🟡 Non-core apps missing 3 theme palettes causes CSS class leaking
The ALL_PALETTES arrays and ThemePalette types in nxus-calendar, nxus-gateway, nxus-recall, and nxus-workbench are missing the gothic, cyberpunk, and bauhaus themes that exist in the core app's ThemePalette type (apps/nxus-core/src/stores/theme.store.ts:29-31) and themeOptions (apps/nxus-core/src/config/theme-options.ts:153-170). When a user selects one of these themes in nxus-core, the applyStoredTheme() function in the other apps will correctly add the class (since palette !== 'default'), but will never remove it because ALL_PALETTES.forEach((p) => root.classList.remove(p)) doesn't include those values. Switching away from gothic/cyberpunk/bauhaus in the core app will leave the stale CSS class on <html> in all other mini-apps.
(Refers to lines 29-49)
Prompt for agents
Add the missing theme palettes 'gothic', 'cyberpunk', and 'bauhaus' to the ThemePalette type union and ALL_PALETTES array in all four non-core apps:
1. apps/nxus-calendar/src/routes/__root.tsx - Add to both ThemePalette type (after 'brutalism') and ALL_PALETTES array (after 'brutalism')
2. apps/nxus-gateway/src/routes/__root.tsx - Same changes
3. apps/nxus-recall/src/routes/__root.tsx - Same changes
4. apps/nxus-workbench/src/routes/__root.tsx - Same changes
For each file, add these three entries to the ThemePalette type:
| 'gothic'
| 'cyberpunk'
| 'bauhaus'
And add these three entries to the ALL_PALETTES array:
'gothic',
'cyberpunk',
'bauhaus',
This ensures the applyStoredTheme() function properly cleans up CSS classes for all themes the core app can set.
Was this helpful? React with 👍 or 👎 to provide feedback.
- Make useThemeHydrated() SSR-safe with optional chaining (fixes 500 error) - Use module-scope initialVisual to avoid gateway visual flash on load Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
synchronous=NORMAL, 64MB page cache, 256MB mmap, in-memory temp store)(node_id, field_node_id)onnode_propertiesand partial index on non-deleted nodesassembleNodes()batch assembly function — 4 queries total instead of N×(2+M+K) per nodeinArray()batch fetches.all().filter()/.all().find()patterns to compound SQLWHEREclauses acrossnode.service.tsandbootstrap.tsCOUNT(*)aggregation in bootstrap summary instead of fetching all rowsclearPropertyinstead of per-row deletionFiles changed
master-client.ts— PRAGMAs + new indexes in DDLnode-schema.ts— compound index in Drizzle schemanode.service.ts—assembleNodes(), compound WHERE clauses, batch operationsquery-evaluator.service.ts— batch candidate fetching, direct supertag matchingbootstrap.ts— compound WHERE insetProperty,COUNT(*)summarysurreal-backend.ts— supertag resolution cacheSummary by CodeRabbit
New Features
Performance Improvements
Bug Fixes