Conversation
WalkthroughAdds the Temporal polyfill dependency; extends graph type system and parsing to support DATETIME/DATE/TIME/DURATION; refactors Graph internals to centralize metadata resolution and parsing; updates public typings to include Temporal types; and expands tests to validate temporal round-trips and related graph behaviors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor TestSuite as Test Suite
participant Graph as Graph
participant Client as Redis Client
participant MetaCache as Metadata Cache
participant Temporal as "@js-temporal/polyfill"
TestSuite->>Graph: query(queryText, params)
Graph->>Client: GRAPH.QUERY <graph>, queryText, params
Client-->>Graph: Reply (headers, data, metaIds)
rect rgb(245,250,255)
note right of Graph: Metadata resolution (centralized)
Graph->>MetaCache: lookup(metaIds)
alt cache miss
Graph->>Client: GRAPH.RO_QUERY ... (metadata request)
Client-->>Graph: metadata names
Graph->>MetaCache: update(cache)
end
end
rect rgb(240,255,240)
note right of Graph: Value parsing & conversion (per-cell)
loop for each cell
alt temporal type detected
Graph->>Temporal: parse(encodedTemporal)
Temporal-->>Graph: PlainDate / PlainTime / PlainDateTime / Duration
else non-temporal
Graph-->>Graph: parse per type (int/string/array/point/vector)
end
end
end
Graph-->>TestSuite: GraphReply (data?, stats, cached?)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #409 +/- ##
==========================================
+ Coverage 92.61% 92.67% +0.05%
==========================================
Files 23 23
Lines 555 573 +18
Branches 87 91 +4
==========================================
+ Hits 514 531 +17
Misses 41 41
- Partials 0 1 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
src/graph.ts (3)
301-303: Parse BOOLEAN case-insensitively.Server casing shouldn’t matter; keep it robust.
Apply this diff:
- return value === "true"; + return value.toLowerCase() === "true";
322-333: Simplify MAP decoding loop for clarity.Avoid index mutation inside the subscript.
Apply this diff:
- for (let i = 0; i < value.length; i++) { - map[value[i++] as string] = this.#parseValue( - value[i] as GraphRawValue, - promises - ); - } + for (let i = 0; i < value.length; i += 2) { + const key = value[i] as string; + map[key] = this.#parseValue(value[i + 1] as GraphRawValue, promises); + }
6-6: Optional: prefer top-level type import to avoid deep path coupling.If @redis/client exports RedisCommandArgument, use:
-import { RedisCommandArgument } from "@redis/client/dist/lib/commands"; +import type { RedisCommandArgument } from "@redis/client";Would you like me to verify current @redis/client typings across supported versions?
tests/graphAndQuery.spec.ts (3)
289-299: Assert directly instead of expecting an assertion to throw.Cleaner and less confusing.
Apply this diff:
- expect(() => { - expect(executionTime).toBeLessThan(1); - }).toThrow(); + expect(executionTime).toBeGreaterThanOrEqual(1);
55-56: Drop unnecessary await.selectGraph is synchronous.
Apply this diff:
- const copyGraph = await clientInstance.selectGraph("graphcopy"); + const copyGraph = clientInstance.selectGraph("graphcopy");
566-570: Remove debug logs from tests.They add noise to CI output.
Apply this diff:
- console.log( - (result.data as { time: Temporal.PlainTime }[])[0].time.toString() - ); - console.log(timeObj.toString());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.json(1 hunks)src/graph.ts(1 hunks)tests/graphAndQuery.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/graphAndQuery.spec.ts (3)
src/falkordb.ts (1)
FalkorDB(122-193)tests/dbConnection.ts (1)
client(3-15)src/graph.ts (1)
query(143-146)
src/graph.ts (4)
src/commands/QUERY.ts (1)
QueryReply(35-43)src/clients/client.ts (1)
Client(8-80)src/commands/index.ts (1)
QueryOptions(58-61)index.ts (2)
ConstraintType(2-2)EntityType(2-2)
🪛 Biome (2.1.2)
src/graph.ts
[error] 356-356: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 357-357: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (3)
package.json (1)
50-51: Temporal polyfill dependency looks good.Adding "@js-temporal/polyfill" as a runtime dependency is appropriate given its use in src and tests.
tests/graphAndQuery.spec.ts (2)
275-287: Slow log assertions may be brittle; guard for empty logs.Depending on server config, slow log might be empty. Add a length check to avoid index errors.
Apply this diff:
- expect(slowLogResults).toBeDefined(); - expect(slowLogResults[0].command).toBe("GRAPH.QUERY"); + expect(slowLogResults).toBeDefined(); + expect(slowLogResults.length).toBeGreaterThan(0); + expect(slowLogResults[0].command).toBe("GRAPH.QUERY");
1-6: Test imports LGTM.Temporal polyfill is correctly imported; suite-scoped DB lifecycle is sensible.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/graph.ts (4)
118-122: Fix POINT type: values are numbers, not strings.This issue was flagged in a previous review and remains unresolved. The GraphValue type declares latitude and longitude as strings, but they are parsed as numbers (lines 343-344).
Apply this diff:
| { - latitude: string; - longitude: string; + latitude: number; + longitude: number; }
272-275: Normalize empty-data replies to match the declared GraphReply shape.This issue was flagged in a previous review and remains unresolved. Returning the raw reply when
reply.datais falsy leaks the internal "headers" field, violating the GraphReply type contract.Apply this diff:
- if (!reply.data) return reply; + if (!reply.data) return { metadata: reply.metadata };
362-370: Wrap DURATION case declarations in a block to satisfy noSwitchDeclarations.This issue was flagged in a previous review and by Biome static analysis. The
constdeclarations on lines 363-364 can bleed through to other cases without block scoping.Apply this diff:
- case GraphValueTypes.DURATION: - const time = Temporal.Instant.fromEpochMilliseconds(value * 1000); - const epoch = Temporal.Instant.fromEpochMilliseconds(0); - - return epoch - .toZonedDateTimeISO("UTC") - .until(time.toZonedDateTimeISO("UTC"), { - largestUnit: "years", - }); + case GraphValueTypes.DURATION: { + const time = Temporal.Instant.fromEpochMilliseconds(value * 1000); + const epoch = Temporal.Instant.fromEpochMilliseconds(0); + return epoch + .toZonedDateTimeISO("UTC") + .until(time.toZonedDateTimeISO("UTC"), { largestUnit: "years" }); + }
154-156: Invalidate metadata cache on delete operation.This issue was flagged in a previous review and remains unresolved. When the graph is deleted, the metadata cache should be cleared to prevent stale data in subsequent operations.
Apply this diff:
async delete() { - return this.#client.delete(this.#name); + const res = await this.#client.delete(this.#name); + this.#metadata = undefined; + return res; }Also applies to: constraintCreate (176-189), constraintDrop (191-204), and query/index operations (479, 567)
🧹 Nitpick comments (1)
src/graph.ts (1)
450-480: Consider validating label and property names to prevent injection.The
createTypedIndexmethod builds Cypher queries using string interpolation withlabelandpropertiesparameters. While typical usage involves literal strings, consider validating these inputs to prevent potential Cypher injection if user-controlled values are passed.Example validation:
const VALID_IDENTIFIER = /^[a-zA-Z_][a-zA-Z0-9_]*$/; async createTypedIndex( idxType: string, entityType: "NODE" | "EDGE", label: string, properties: string[], options?: Record<string, string | number | boolean> ): Promise<QueryReply> { if (!VALID_IDENTIFIER.test(label)) { throw new Error(`Invalid label: ${label}`); } for (const prop of properties) { if (!VALID_IDENTIFIER.test(prop)) { throw new Error(`Invalid property: ${prop}`); } } // ... rest of implementation }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.json(1 hunks)src/graph.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code graph analysis (1)
src/graph.ts (4)
src/commands/QUERY.ts (1)
QueryReply(35-43)src/clients/client.ts (1)
Client(9-86)src/commands/index.ts (1)
QueryOptions(61-64)index.ts (2)
ConstraintType(2-2)EntityType(2-2)
🪛 Biome (2.1.2)
src/graph.ts
[error] 363-363: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 364-364: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (6)
src/graph.ts (6)
128-130: LGTM! Public API correctly hides internal headers.The GraphReply type properly omits the internal "headers" field from QueryReply and replaces "data" with a typed version, providing a clean public API.
32-35: LGTM! Temporal types correctly added to enum.The new DATETIME, DATE, TIME, and DURATION enum values follow the sequential numbering from FalkorDB's resultset_formatter.h and are properly integrated.
79-82: LGTM! Temporal raw value types correctly defined.The temporal raw value types properly specify
numberas the underlying value type, consistent with the parsing logic that expects Unix timestamp values.
123-126: LGTM! GraphValue union properly extended with Temporal types.The GraphValue union correctly includes the Temporal types that correspond to the parsing output, providing a type-safe public API for temporal data.
6-6: No issues found with the Temporal polyfill import.Version 0.5.1 is the latest published version (March 31, 2025) and contains only bug fixes with no known vulnerabilities. The import is appropriate as-is.
350-361: Timezone semantics for DATETIME/DATE/TIME parsing are correct.FalkorDB's temporal types (DATE, TIME, DATETIME) are "local" types—timezone-naive, not timezone-aware. The wire protocol sends these as epoch seconds. The parsing code correctly converts them: multiply by 1000 (seconds → milliseconds), create an Instant, normalize to UTC, then extract the Plain* type (which removes timezone information as intended, since these types are timezone-naive by design).
Tests confirm the behavior is correct and round-trip values match expectations. No changes needed.
barakb
left a comment
barakb
left a comment
There was a problem hiding this comment.
there is ai comment that seems important
2 participants
Summary by CodeRabbit
New Features
Chores
Tests
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.