Repository navigation
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (2)
You can disable this status message by setting the WalkthroughAdds a large set of Redis/Valkey commands and overloads across TypeScript, JS, and Zig layers; consolidates HSET/HMSET parsing into a shared implementation; expands integration tests (removes a unit set test); and updates the Redis test Dockerfile to redis:8-alpine. Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
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 (2)
src/valkey/js_valkey_functions.zig (2)
635-637: Consider using more specific templates for fixed-argument commands.The commands
setbit,getrange, andsetrangeuse the variadic"(...strings: string[])"template, but these Redis commands have well-defined, fixed argument structures:
SETBIT key offset value(3 args)GETRANGE key start end(3 args)SETRANGE key offset value(3 args)Using the variadic template means arguments won't be validated at the binding layer and error messages will be less specific. Consider using a more specific template pattern for better type safety and clearer error messages.
For example, you could create or use a
"(key: RedisKey, arg1: RedisValue, arg2: RedisValue)"template pattern similar to howgetbituses"(key: RedisKey, value: RedisValue)"at line 634.
673-674: Consider using more specific templates for fixed-argument commands.The commands
setexandpsetexuse the variadic"(...strings: string[])"template, but these Redis commands have fixed argument structures:
SETEX key seconds value(3 args)PSETEX key milliseconds value(3 args)Using the variadic template means arguments won't be validated at the binding layer. Consider using a more specific template pattern (e.g., a three-argument variant) for better type safety and more descriptive error messages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/bun-types/redis.d.ts(3 hunks)src/bun.js/api/valkey.classes.ts(3 hunks)src/valkey/js_valkey_functions.zig(2 hunks)test/js/valkey/valkey.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
**/*.classes.ts
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
**/*.classes.ts: Define JavaScript class interfaces in .classes.ts using define({ name, constructor, JSType, finalize, proto: { ... } })
Use cache: true on properties that should use JSC WriteBarrier-backed caching
Files:
src/bun.js/api/valkey.classes.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript files with Prettier (bun run prettier)
Files:
src/bun.js/api/valkey.classes.tspackages/bun-types/redis.d.tstest/js/valkey/valkey.test.ts
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/valkey/js_valkey_functions.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/valkey/js_valkey_functions.zig
src/**/js_*.zig
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
src/**/js_*.zig: Implement JavaScript bindings in a Zig file named with a js_ prefix (e.g., js_smtp.zig, js_your_feature.zig)
Handle reference counting correctly with ref()/deref() in JS-facing Zig code
Always implement proper cleanup in deinit() and finalize() for JS-exposed types
Files:
src/valkey/js_valkey_functions.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/valkey/js_valkey_functions.zig
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/valkey/valkey.test.ts
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/valkey/valkey.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/valkey/valkey.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/js/valkey/valkey.test.ts
test/js/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests for specific features under
test/js/by module
Files:
test/js/valkey/valkey.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must be placed under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or use custom random port functions
In tests, use normalizeBunSnapshot when asserting snapshots
Never write tests that merely assert absence of "panic" or "uncaught exception" in output
Avoid shell commands (e.g., find, grep) in tests; use Bun.Glob and built-ins instead
Prefer snapshot tests over exact stdout equality assertions
Files:
test/js/valkey/valkey.test.ts
🧠 Learnings (3)
📚 Learning: 2025-09-03T01:30:58.001Z
Learnt from: markovejnovic
PR: oven-sh/bun#21728
File: test/js/valkey/valkey.test.ts:264-271
Timestamp: 2025-09-03T01:30:58.001Z
Learning: For test/js/valkey/valkey.test.ts PUB/SUB tests, avoid arbitrary sleeps and async-forEach. Instead, resolve a Promise from the subscriber callback when the expected number of messages is observed and await it with a bounded timeout (e.g., withTimeout + Promise.withResolvers) to account for Redis server→subscriber propagation.
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-09-20T00:58:38.042Z
Learnt from: markovejnovic
PR: oven-sh/bun#22568
File: test/js/valkey/valkey.test.ts:561-564
Timestamp: 2025-09-20T00:58:38.042Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods (like ctx.redis.set(), ctx.redis.unsubscribe(), etc.) - Bun's Redis client implementation differs from Node.js and can throw synchronously even for async methods. The maintainer has explicitly requested to stop looking at this error pattern.
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-09-20T00:57:56.685Z
Learnt from: markovejnovic
PR: oven-sh/bun#22568
File: test/js/valkey/valkey.test.ts:268-276
Timestamp: 2025-09-20T00:57:56.685Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods like ctx.redis.set() - the maintainer has explicitly requested to stop looking at this error pattern.
Applied to files:
test/js/valkey/valkey.test.ts
🧬 Code graph analysis (1)
test/js/valkey/valkey.test.ts (1)
packages/bun-types/redis.d.ts (1)
redis(964-964)
⏰ 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). (1)
- GitHub Check: Format
🔇 Additional comments (21)
test/js/valkey/valkey.test.ts (11)
97-113: LGTM! Comprehensive INCRBY test coverage.The test properly covers:
- Basic increment operation
- Negative increment (decrement behavior)
- Non-existent key initialization to 0
All assertions are correct per Redis spec.
115-131: LGTM! INCRBYFLOAT test correctly validates string return.The test properly validates that INCRBYFLOAT returns a string (per Redis spec) and covers:
- Basic float increment
- Negative float increment
- Non-existent key initialization
133-149: LGTM! DECRBY test validates negative result correctly.The test properly covers:
- Basic decrement operation
- Multiple decrement operations
- Non-existent key resulting in negative value (line 147-148)
This correctly validates Redis behavior where DECRBY on a non-existent key treats it as 0.
151-165: LGTM! MSET atomic operation properly validated.The test correctly validates:
- Atomicity of multi-key set (line 155)
- Post-condition verification for all keys (lines 159-164)
- Return value of "OK"
167-191: LGTM! MSETNX test validates atomic conditional behavior.The test properly validates MSETNX semantics:
- Returns 1 when all keys don't exist (line 172)
- Returns 0 when any key exists (line 182)
- No partial updates when condition fails (lines 185-190)
This correctly validates the atomic all-or-nothing behavior.
211-228: LGTM! SETEX test properly validates expiration.The test correctly validates:
- Value is set (lines 221-222)
- TTL is configured within expected range (lines 225-227)
- Return value is "OK"
230-247: LGTM! PSETEX test validates millisecond precision expiration.The test correctly:
- Uses PTTL for millisecond precision verification (line 244)
- Validates TTL bounds appropriately (lines 245-246)
- Confirms value persistence (lines 240-241)
249-272: LGTM! GETBIT/SETBIT test validates bit manipulation.The test correctly validates:
- SETBIT returns old value (0 initially)
- GETBIT retrieves correct bit value
- Unset bits default to 0
- Bit state transitions properly
274-296: LGTM! Multi-bit operations with BITCOUNT properly tested.The test validates:
- Multiple bit operations
- Individual bit verification
- BITCOUNT aggregation (line 294-295)
298-318: LGTM! GETRANGE test covers all offset scenarios.The test properly validates:
- Positive offsets (lines 304-309)
- Negative offsets counting from end (lines 312-313)
- Full string retrieval with 0, -1 (lines 316-317)
320-337: LGTM! SETRANGE test validates overwrite and padding.The test correctly validates:
- String overwrite with length return (lines 326-331)
- Zero-byte padding for non-existent keys (lines 334-336)
src/bun.js/api/valkey.classes.ts (2)
51-66: LGTM! Increment/decrement command bindings are correct.The bindings properly specify:
incrbyanddecrbywith length 2 (key + increment)incrbyfloatwith length 2 (key + float increment)All arities are correct per Redis spec.
237-254: Verify fixed-arity Redis commands missinglengthmetadata
- Add
length: 3tosetexandpsetex- Add
length: 2tozscoreandgetset- Leave
mset/msetnxvariadic (nolength)packages/bun-types/redis.d.ts (6)
248-262: LGTM! INCRBY and INCRBYFLOAT declarations are correct.The type declarations properly specify:
incrbyreturnsPromise<number>(integer result)incrbyfloatreturnsPromise<string>(Redis returns string for float precision)- JSDoc comments are accurate
- Parameter types use
RedisClient.KeyLikeappropriately
271-277: LGTM! DECRBY declaration matches Redis spec.The type signature correctly:
- Accepts key and decrement amount
- Returns
Promise<number>(integer result)- Documents behavior appropriately
596-624: LGTM! SETEX and PSETEX declarations are well-documented.The declarations properly:
- Return
Promise<"OK">per Redis spec- Include comprehensive JSDoc with examples
- Use correct parameter order (key, time, value)
- Document time units clearly (seconds vs milliseconds)
643-690: LGTM! MSET and MSETNX declarations with excellent documentation.The declarations correctly:
- Use variadic parameters (
...keyValuePairs)- Return appropriate types (
"OK"vsnumber)- Include detailed JSDoc explaining atomic behavior
- Provide usage examples
- Document MSETNX's all-or-nothing semantics clearly
699-714: LGTM! Bit operation declarations are type-safe.The declarations properly:
getbitreturnsPromise<number>(0 or 1)setbitenforcesvalue: 0 | 1for type safety (line 714)- Return types match Redis behavior (old value for setbit)
- Parameter names are clear (key, offset, value)
716-732: LGTM! Range operation declarations are correct.The declarations properly:
getrangereturnsPromise<string>setrangereturnsPromise<number>(length after modification)- Document negative offset support in JSDoc (line 719)
- Use appropriate types for all parameters
src/valkey/js_valkey_functions.zig (2)
664-666: LGTM!The
incrby,incrbyfloat, anddecrbycommand bindings correctly use the"(key: RedisKey, value: RedisValue)"template with appropriate argument names ("increment" and "decrement") that accurately reflect the Redis command specifications.
679-680: LGTM!The
msetandmsetnxcommand bindings correctly use the"(...strings: string[])"variadic template, which is appropriate since these commands accept a variable number of key-value pairs (MSET key1 value1 [key2 value2 ...]).
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/bun.js/api/valkey.classes.ts (1)
138-149: Add missinglengthproperties for bit and range commands.These fixed-arity commands are missing their
lengthproperty:
getbit: requires key and offset (length: 2)setbit: requires key, offset, and value (length: 3)getrange: requires key, start, and end (length: 3)setrange: requires key, offset, and value (length: 3)Apply this diff:
getbit: { fn: "getbit", + length: 2, }, setbit: { fn: "setbit", + length: 3, }, getrange: { fn: "getrange", + length: 3, }, setrange: { fn: "setrange", + length: 3, },
🧹 Nitpick comments (2)
src/valkey/js_valkey_functions.zig (2)
634-637: Consider more specific signatures for fixed-arity bit and range commands.The bit and range commands (
getbit,setbit,getrange,setrange) have fixed arities:
getbit: 2 args (key, offset)setbit: 3 args (key, offset, value)getrange: 3 args (key, start, end)setrange: 3 args (key, offset, value)Using the generic
@"(...strings: string[])"signature means argument count isn't enforced at compile time. Consider using more specific signatures for better type safety and error messages, similar to howhgetuses@"(key: RedisKey, value: RedisValue)".For example:
-pub const getbit = compile.@"(key: RedisKey, value: RedisValue)"("getbit", "GETBIT", "key", "offset", .not_subscriber).call; +// getbit already uses (key, value) signature - this is correct -pub const setbit = compile.@"(...strings: string[])"("setbit", "SETBIT", .not_subscriber).call; +// Could create a @"(key: RedisKey, offset: RedisValue, value: RedisValue)" factory or use the variadic one with key -pub const getrange = compile.@"(...strings: string[])"("getrange", "GETRANGE", .not_subscriber).call; +// Could be @"(key: RedisKey, start: RedisValue, end: RedisValue)" -pub const setrange = compile.@"(...strings: string[])"("setrange", "SETRANGE", .not_subscriber).call; +// Could be @"(key: RedisKey, offset: RedisValue, value: RedisValue)"Note:
getbitalready uses a specific signature. The others (setbit,getrange,setrange) would benefit from similar specificity if a three-argument factory exists or can be added.
675-676: Consider more specific signatures for SETEX and PSETEX.Both
setexandpsetexhave fixed arity (3 arguments: key, seconds/milliseconds, value). Using the generic variadic signature loses type safety. Consider creating or using a three-argument factory method for better compile-time checking.Expected signatures:
setex(key, seconds, value)psetex(key, milliseconds, value)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
packages/bun-types/redis.d.ts(7 hunks)src/bun.js/api/valkey.classes.ts(5 hunks)src/valkey/js_valkey.zig(4 hunks)src/valkey/js_valkey_functions.zig(5 hunks)test/js/valkey/valkey.test.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/valkey/valkey.test.ts
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/valkey/valkey.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/valkey/valkey.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/js/valkey/valkey.test.ts
test/js/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests for specific features under
test/js/by module
Files:
test/js/valkey/valkey.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must be placed under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or use custom random port functions
In tests, use normalizeBunSnapshot when asserting snapshots
Never write tests that merely assert absence of "panic" or "uncaught exception" in output
Avoid shell commands (e.g., find, grep) in tests; use Bun.Glob and built-ins instead
Prefer snapshot tests over exact stdout equality assertions
Files:
test/js/valkey/valkey.test.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript files with Prettier (bun run prettier)
Files:
test/js/valkey/valkey.test.tssrc/bun.js/api/valkey.classes.tspackages/bun-types/redis.d.ts
**/*.classes.ts
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
**/*.classes.ts: Define JavaScript class interfaces in .classes.ts using define({ name, constructor, JSType, finalize, proto: { ... } })
Use cache: true on properties that should use JSC WriteBarrier-backed caching
Files:
src/bun.js/api/valkey.classes.ts
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/valkey/js_valkey.zigsrc/valkey/js_valkey_functions.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/valkey/js_valkey.zigsrc/valkey/js_valkey_functions.zig
src/**/js_*.zig
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
src/**/js_*.zig: Implement JavaScript bindings in a Zig file named with a js_ prefix (e.g., js_smtp.zig, js_your_feature.zig)
Handle reference counting correctly with ref()/deref() in JS-facing Zig code
Always implement proper cleanup in deinit() and finalize() for JS-exposed types
Files:
src/valkey/js_valkey.zigsrc/valkey/js_valkey_functions.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/valkey/js_valkey.zigsrc/valkey/js_valkey_functions.zig
🧠 Learnings (12)
📚 Learning: 2025-09-03T01:30:58.001Z
Learnt from: markovejnovic
PR: oven-sh/bun#21728
File: test/js/valkey/valkey.test.ts:264-271
Timestamp: 2025-09-03T01:30:58.001Z
Learning: For test/js/valkey/valkey.test.ts PUB/SUB tests, avoid arbitrary sleeps and async-forEach. Instead, resolve a Promise from the subscriber callback when the expected number of messages is observed and await it with a bounded timeout (e.g., withTimeout + Promise.withResolvers) to account for Redis server→subscriber propagation.
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-09-20T00:58:38.042Z
Learnt from: markovejnovic
PR: oven-sh/bun#22568
File: test/js/valkey/valkey.test.ts:561-564
Timestamp: 2025-09-20T00:58:38.042Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods (like ctx.redis.set(), ctx.redis.unsubscribe(), etc.) - Bun's Redis client implementation differs from Node.js and can throw synchronously even for async methods. The maintainer has explicitly requested to stop looking at this error pattern.
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-09-20T00:57:56.685Z
Learnt from: markovejnovic
PR: oven-sh/bun#22568
File: test/js/valkey/valkey.test.ts:268-276
Timestamp: 2025-09-20T00:57:56.685Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods like ctx.redis.set() - the maintainer has explicitly requested to stop looking at this error pattern.
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Import common test utilities from `harness` (e.g., `bunExe`, `bunEnv`, `tempDirWithFiles`, `tmpdirSync`, platform checks, GC helpers)
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/bake-harness.ts : Provide and maintain shared test utilities: devTest, prodTest, devAndProductionTest, Dev, Client, and helpers in the harness
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Use `using`/`await using` with Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/{dev/*.test.ts,dev-and-prod.ts} : Import testing utilities (devTest, prodTest, devAndProductionTest, Dev, Client) from test/bake/bake-harness.ts
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-09-20T03:39:41.770Z
Learnt from: pfgithub
PR: oven-sh/bun#22534
File: test/regression/issue/21830.fixture.ts:14-63
Timestamp: 2025-09-20T03:39:41.770Z
Learning: Bun's test runner supports async describe callbacks, unlike Jest/Vitest where describe callbacks must be synchronous. The syntax `describe("name", async () => { ... })` is valid in Bun.
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-09-16T19:58:14.006Z
Learnt from: markovejnovic
PR: oven-sh/bun#22568
File: src/valkey/js_valkey_functions.zig:671-682
Timestamp: 2025-09-16T19:58:14.006Z
Learning: In Bun's Redis client implementation (src/valkey/js_valkey_functions.zig), the psubscribe and punsubscribe functions are not implemented nor supported - they are intentionally left as basic wrappers without proper subscription handling.
Applied to files:
src/valkey/js_valkey_functions.zig
🧬 Code graph analysis (1)
test/js/valkey/valkey.test.ts (2)
packages/bun-types/redis.d.ts (2)
RedisClient(54-1212)redis(1223-1223)test/harness.ts (1)
bunExe(103-106)
🔇 Additional comments (18)
src/valkey/js_valkey_functions.zig (2)
1025-1047: LGTM! Clean zero-arity command factory.The new
@"()"factory method follows the established pattern and correctly handles commands that take no arguments (likerandomkey). The implementation properly checks client state and propagates errors.
652-652: LGTM! Command signatures match their arities.The following commands correctly use appropriate signatures:
randomkey: zero-arg factoryincrby,incrbyfloat,decrby: two-arg (key, value)mset,msetnx: variadic for key-value pairsunlink,touch: variadic for multiple keysrename,renamenx: two-arg (key, newkey)Also applies to: 666-668, 681-682, 696-699
test/js/valkey/valkey.test.ts (3)
3-3: LGTM! Correct test harness import.The import of
bunExefromharnessfollows the coding guidelines for test files and replaces hardcoded path construction.Based on coding guidelines
1239-1239: LGTM! Correct use of bunExe() for spawning Bun.The subprocess now uses
bunExe()from the test harness, replacing hardcoded path construction. This follows the coding guidelines for test files.Based on coding guidelines
98-789: LGTM! Comprehensive test coverage for new commands.The new tests provide excellent coverage:
- ✅ Happy paths for all new commands
- ✅ Edge cases (non-existent keys, boundary conditions)
- ✅ Error validation with invalid arguments
- ✅ No arbitrary timeouts (uses proper async/await)
- ✅ Follows established test patterns
The tests properly validate:
- Numeric operations (INCRBY, INCRBYFLOAT, DECRBY) with non-existent keys
- Key operations (RENAME, RENAMENX) with overwrite scenarios
- Multi-key operations (MSET, MSETNX) with conflict handling
- Expiration commands (SETEX, PSETEX) with TTL verification
- Bit operations (SETBIT, GETBIT) with multiple operations
- Range operations (GETRANGE, SETRANGE) with negative indices
- Scan operations with cursor iteration and MATCH patterns
- Comprehensive error cases for all new commands
Based on coding guidelines
packages/bun-types/redis.d.ts (9)
37-37: LGTM! Enhanced TLS configuration support.The type definition now correctly accepts both boolean and
Bun.TLSOptions, enabling more granular TLS configuration. This aligns with the implementation in the Zig code.
241-270: LGTM! Increment/decrement commands correctly typed.The type definitions for
incrby,incrbyfloat, anddecrbyare correct:
incrbyfloatcorrectly returnsPromise<string>to preserve floating-point precision- Integer operations return
Promise<number>- Parameter types appropriately accept
number | stringfor float operations
462-479: LGTM! Well-documented randomkey command.The
randomkeymethod signature correctly returnsPromise<string | null>and includes clear documentation with examples.
488-578: LGTM! Comprehensive scan command overloads.The
scanmethod overloads are well-designed:
- Correctly typed return value
Promise<[string, string[]]>matching Redis SCAN response- Multiple overloads cover common usage patterns (basic, MATCH, COUNT, combined)
- Excellent documentation with practical examples
- Variadic overload provides flexibility for additional options
596-624: LGTM! Well-typed command for Redis TYPE.The
typemethod uses a precise union type that matches all possible Redis TYPE responses. The documentation is comprehensive with examples for each data type.
730-758: LGTM! Correctly typed expiration commands.The
setexandpsetexmethods have correct signatures matching Redis command syntax (key, time, value) and return the expectedPromise<"OK">.
777-824: LGTM! Multi-key set commands well-documented.The
msetandmsetnxmethods are correctly typed with appropriate return values ("OK"vsnumber). Documentation clearly explains the atomic behavior and the key difference between unconditional and conditional setting.
833-866: LGTM! Excellent type safety on bit operations.The bit and range operations are correctly typed:
setbituses the literal type0 | 1for the value parameter, providing excellent compile-time safety- All return types match Redis command semantics
- Clear documentation for all methods
1088-1211: LGTM! Comprehensive key management commands.The key management commands are well-designed:
copyoverloads correctly distinguish between replace and non-replace behaviorunlink,touch,rename, andrenamenxhave appropriate signatures matching Redis semantics- All return types are correct
- Excellent documentation with practical examples explaining behavior and use cases
src/valkey/js_valkey.zig (1)
1246-1321: Next verification stepsRun the following script to locate the generated class file and verify each command implementation:
#!/bin/bash echo "🔍 Locating ZigGeneratedClasses.zig..." find . -type f -name "ZigGeneratedClasses.zig" || echo "⚠️ ZigGeneratedClasses.zig not found" echo echo "🔍 Verifying implementations in js_valkey_functions.zig..." commands=( copy decrby getbit getrange incrby incrbyfloat mset msetnx psetex randomkey rename renamenx scan setbit setex setrange touch unlink ) for cmd in "${commands[@]}"; do if rg -nP "pub fn $cmd\\b" src/valkey/js_valkey_functions.zig; then echo "✅ pub fn $cmd found" else echo "❌ pub fn $cmd missing" fi donesrc/bun.js/api/valkey.classes.ts (3)
51-66: LGTM: Increment/decrement commands properly configured.The
incrby,incrbyfloat, anddecrbycommands correctly specify their fixed arity withlength: 2(key + increment/decrement value).
198-200: LGTM: Variable-arityscancommand correctly omitslength.The
scancommand accepts variable arguments (cursor + optional MATCH/COUNT/TYPE parameters), so omitting thelengthproperty is appropriate.
258-263: LGTM: Variable-arity multi-set commands correctly omitlength.The
msetandmsetnxcommands accept variable arguments (key-value pairs), so omitting thelengthproperty is appropriate.
…bylex/zadd/zscan/zrangestore
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/js/valkey/valkey.test.ts (1)
20-20: Fix TLS vs. TCP client selection
WhenconnectionType === ConnectionType.TLS, the truthy enum value takes the first branch and bindsctx.redisto the non-TLS client, so every TLS test runs against the plain TCP connection and_ctx.redisTLSnever gets used. Flip the ternary and compare explicitly against the enum.- const ctx = { ..._ctx, redis: connectionType ? _ctx.redis : (_ctx.redisTLS as RedisClient) }; + const ctx = { + ..._ctx, + redis: connectionType === ConnectionType.TLS ? (_ctx.redisTLS as RedisClient) : _ctx.redis, + };
🧹 Nitpick comments (1)
src/valkey/js_valkey_functions.zig (1)
677-679: Consider type safety for numeric argument commands.Commands
incrby,incrbyfloat,decrby,setex, andpsetexexpect numeric arguments (increments, seconds, milliseconds), but the@"(key: RedisKey, value: RedisValue)"and@"(...strings: string[])"factories accept any string or buffer.While the
fromJShelper (line 1270-1274) does convert numbers to strings, there's no validation that the values are actually numeric or within valid ranges. Redis will reject invalid values at runtime, but earlier validation could provide better error messages.If you want stricter type checking, consider adding numeric validation helpers or using more specific compile factories for these commands. Otherwise, the current approach is acceptable if you're comfortable relying on Redis-side validation.
Also applies to: 686-687
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
packages/bun-types/redis.d.ts(11 hunks)src/bun.js/api/valkey.classes.ts(7 hunks)src/valkey/js_valkey.zig(3 hunks)src/valkey/js_valkey_functions.zig(4 hunks)test/js/valkey/valkey.test.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/valkey/valkey.test.ts
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/valkey/valkey.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/valkey/valkey.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/js/valkey/valkey.test.ts
test/js/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests for specific features under
test/js/by module
Files:
test/js/valkey/valkey.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must be placed under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or use custom random port functions
In tests, use normalizeBunSnapshot when asserting snapshots
Never write tests that merely assert absence of "panic" or "uncaught exception" in output
Avoid shell commands (e.g., find, grep) in tests; use Bun.Glob and built-ins instead
Prefer snapshot tests over exact stdout equality assertions
Files:
test/js/valkey/valkey.test.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript files with Prettier (bun run prettier)
Files:
test/js/valkey/valkey.test.tssrc/bun.js/api/valkey.classes.tspackages/bun-types/redis.d.ts
**/*.classes.ts
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
**/*.classes.ts: Define JavaScript class interfaces in .classes.ts using define({ name, constructor, JSType, finalize, proto: { ... } })
Use cache: true on properties that should use JSC WriteBarrier-backed caching
Files:
src/bun.js/api/valkey.classes.ts
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/valkey/js_valkey_functions.zigsrc/valkey/js_valkey.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/valkey/js_valkey_functions.zigsrc/valkey/js_valkey.zig
src/**/js_*.zig
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
src/**/js_*.zig: Implement JavaScript bindings in a Zig file named with a js_ prefix (e.g., js_smtp.zig, js_your_feature.zig)
Handle reference counting correctly with ref()/deref() in JS-facing Zig code
Always implement proper cleanup in deinit() and finalize() for JS-exposed types
Files:
src/valkey/js_valkey_functions.zigsrc/valkey/js_valkey.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/valkey/js_valkey_functions.zigsrc/valkey/js_valkey.zig
🧠 Learnings (12)
📚 Learning: 2025-09-03T01:30:58.001Z
Learnt from: markovejnovic
PR: oven-sh/bun#21728
File: test/js/valkey/valkey.test.ts:264-271
Timestamp: 2025-09-03T01:30:58.001Z
Learning: For test/js/valkey/valkey.test.ts PUB/SUB tests, avoid arbitrary sleeps and async-forEach. Instead, resolve a Promise from the subscriber callback when the expected number of messages is observed and await it with a bounded timeout (e.g., withTimeout + Promise.withResolvers) to account for Redis server→subscriber propagation.
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-09-20T00:58:38.042Z
Learnt from: markovejnovic
PR: oven-sh/bun#22568
File: test/js/valkey/valkey.test.ts:561-564
Timestamp: 2025-09-20T00:58:38.042Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods (like ctx.redis.set(), ctx.redis.unsubscribe(), etc.) - Bun's Redis client implementation differs from Node.js and can throw synchronously even for async methods. The maintainer has explicitly requested to stop looking at this error pattern.
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-09-20T00:57:56.685Z
Learnt from: markovejnovic
PR: oven-sh/bun#22568
File: test/js/valkey/valkey.test.ts:268-276
Timestamp: 2025-09-20T00:57:56.685Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods like ctx.redis.set() - the maintainer has explicitly requested to stop looking at this error pattern.
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Import common test utilities from `harness` (e.g., `bunExe`, `bunEnv`, `tempDirWithFiles`, `tmpdirSync`, platform checks, GC helpers)
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/bake-harness.ts : Provide and maintain shared test utilities: devTest, prodTest, devAndProductionTest, Dev, Client, and helpers in the harness
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Use `using`/`await using` with Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/{dev/*.test.ts,dev-and-prod.ts} : Import testing utilities (devTest, prodTest, devAndProductionTest, Dev, Client) from test/bake/bake-harness.ts
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-09-20T03:39:41.770Z
Learnt from: pfgithub
PR: oven-sh/bun#22534
File: test/regression/issue/21830.fixture.ts:14-63
Timestamp: 2025-09-20T03:39:41.770Z
Learning: Bun's test runner supports async describe callbacks, unlike Jest/Vitest where describe callbacks must be synchronous. The syntax `describe("name", async () => { ... })` is valid in Bun.
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-09-16T19:58:14.006Z
Learnt from: markovejnovic
PR: oven-sh/bun#22568
File: src/valkey/js_valkey_functions.zig:671-682
Timestamp: 2025-09-16T19:58:14.006Z
Learning: In Bun's Redis client implementation (src/valkey/js_valkey_functions.zig), the psubscribe and punsubscribe functions are not implemented nor supported - they are intentionally left as basic wrappers without proper subscription handling.
Applied to files:
src/valkey/js_valkey_functions.zigsrc/valkey/js_valkey.zig
🧬 Code graph analysis (1)
test/js/valkey/valkey.test.ts (2)
packages/bun-types/redis.d.ts (2)
RedisClient(54-1924)redis(1935-1935)test/harness.ts (1)
bunExe(103-106)
🔇 Additional comments (5)
src/valkey/js_valkey_functions.zig (5)
1045-1067: LGTM! Well-designed factory for no-argument commands.The new
@"()"compile factory correctly handles zero-argument Redis commands with proper state checking and error propagation. The implementation is consistent with existing compile factory patterns.
655-655: Well done using the zero-argument factory forrandomkey.The
randomkeycommand correctly uses the new@"()"factory since it takes no arguments. This is a good example of selecting the appropriate compile factory.
657-657: Variadic signatures are appropriate for these flexible-arity commands.Commands like
scan,zcount,zlexcount,zrange,zrevrange,zrangebyscore,zrevrangebyscore,zrangebylex,zrevrangebylex,zincrby,zmscore,zadd,zscan,mset,msetnx,zrangestore,zremrangebylex,zremrangebyrank,zremrangebyscore, andcopyall have optional arguments or variable-length argument lists. Using the@"(...strings: string[])"factory is the correct choice for these commands.Also applies to: 662-663, 667-672, 689-692, 696-697, 706-706, 708-710, 715-715
707-707: Command signatures match Redis specifications.The signatures for
zrem,unlink,touch,rename, andrenamenxcorrectly match their Redis specifications:
zrem,unlink,touch: Accept a key plus zero or more additional keysrename,renamenx: Accept exactly two arguments (key + newkey)Also applies to: 716-719
634-637: Add test coverage or enforce argument count for bit/range commands
No tests covergetbit,setbit,getrange, orsetrange(src/valkey/js_valkey_functions.zig:634–637); either add dedicated tests intest/for these fixed-arity commands or ensure the compile factory validates their exact arity at runtime.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/js/valkey/valkey.test.ts (2)
2889-2894: Fix shadowed parameters in subscriber callback
messageandchannelin the callback shadow the outer-scope variables, so eachexpectends up comparing a value to itself. The test never actually asserts that the published payload and channel are delivered. Rename the callback parameters and assert against the outer variables to make the check meaningful.- await subscriber.subscribe(channel, (message, channel) => { + await subscriber.subscribe(channel, (receivedMessage, receivedChannel) => { counter.increment(); - expect(channel).toBe(channel); - expect(message).toBe(message); + expect(receivedChannel).toBe(channel); + expect(receivedMessage).toBe(message); });
2885-2890: Await the.resolvesassertion
expect(ctx.redis.set(...)).resolvesreturns a promise. Withoutawait(or returning it), the assertion runs asynchronously and any rejection/failed matcher is ignored, so the test can produce false positives. Await the matcher so failures surface.- expect(ctx.redis.set(testKey(), testValue())).resolves.toEqual("OK"); + await expect(ctx.redis.set(testKey(), testValue())).resolves.toEqual("OK");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
packages/bun-types/redis.d.ts(11 hunks)src/bun.js/api/valkey.classes.ts(7 hunks)src/valkey/js_valkey.zig(3 hunks)src/valkey/js_valkey_functions.zig(4 hunks)test/js/valkey/valkey.test.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript files with Prettier (bun run prettier)
Files:
packages/bun-types/redis.d.tssrc/bun.js/api/valkey.classes.tstest/js/valkey/valkey.test.ts
**/*.classes.ts
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
**/*.classes.ts: Define JavaScript class interfaces in .classes.ts using define({ name, constructor, JSType, finalize, proto: { ... } })
Use cache: true on properties that should use JSC WriteBarrier-backed caching
Files:
src/bun.js/api/valkey.classes.ts
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/valkey/valkey.test.ts
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/valkey/valkey.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/valkey/valkey.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/js/valkey/valkey.test.ts
test/js/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests for specific features under
test/js/by module
Files:
test/js/valkey/valkey.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must be placed under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or use custom random port functions
In tests, use normalizeBunSnapshot when asserting snapshots
Never write tests that merely assert absence of "panic" or "uncaught exception" in output
Avoid shell commands (e.g., find, grep) in tests; use Bun.Glob and built-ins instead
Prefer snapshot tests over exact stdout equality assertions
Files:
test/js/valkey/valkey.test.ts
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/valkey/js_valkey.zigsrc/valkey/js_valkey_functions.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/valkey/js_valkey.zigsrc/valkey/js_valkey_functions.zig
src/**/js_*.zig
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
src/**/js_*.zig: Implement JavaScript bindings in a Zig file named with a js_ prefix (e.g., js_smtp.zig, js_your_feature.zig)
Handle reference counting correctly with ref()/deref() in JS-facing Zig code
Always implement proper cleanup in deinit() and finalize() for JS-exposed types
Files:
src/valkey/js_valkey.zigsrc/valkey/js_valkey_functions.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/valkey/js_valkey.zigsrc/valkey/js_valkey_functions.zig
🧠 Learnings (12)
📚 Learning: 2025-09-20T00:58:38.042Z
Learnt from: markovejnovic
PR: oven-sh/bun#22568
File: test/js/valkey/valkey.test.ts:561-564
Timestamp: 2025-09-20T00:58:38.042Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods (like ctx.redis.set(), ctx.redis.unsubscribe(), etc.) - Bun's Redis client implementation differs from Node.js and can throw synchronously even for async methods. The maintainer has explicitly requested to stop looking at this error pattern.
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-09-03T01:30:58.001Z
Learnt from: markovejnovic
PR: oven-sh/bun#21728
File: test/js/valkey/valkey.test.ts:264-271
Timestamp: 2025-09-03T01:30:58.001Z
Learning: For test/js/valkey/valkey.test.ts PUB/SUB tests, avoid arbitrary sleeps and async-forEach. Instead, resolve a Promise from the subscriber callback when the expected number of messages is observed and await it with a bounded timeout (e.g., withTimeout + Promise.withResolvers) to account for Redis server→subscriber propagation.
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-09-20T00:57:56.685Z
Learnt from: markovejnovic
PR: oven-sh/bun#22568
File: test/js/valkey/valkey.test.ts:268-276
Timestamp: 2025-09-20T00:57:56.685Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods like ctx.redis.set() - the maintainer has explicitly requested to stop looking at this error pattern.
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Import common test utilities from `harness` (e.g., `bunExe`, `bunEnv`, `tempDirWithFiles`, `tmpdirSync`, platform checks, GC helpers)
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/bake-harness.ts : Provide and maintain shared test utilities: devTest, prodTest, devAndProductionTest, Dev, Client, and helpers in the harness
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Use `using`/`await using` with Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/{dev/*.test.ts,dev-and-prod.ts} : Import testing utilities (devTest, prodTest, devAndProductionTest, Dev, Client) from test/bake/bake-harness.ts
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-09-20T03:39:41.770Z
Learnt from: pfgithub
PR: oven-sh/bun#22534
File: test/regression/issue/21830.fixture.ts:14-63
Timestamp: 2025-09-20T03:39:41.770Z
Learning: Bun's test runner supports async describe callbacks, unlike Jest/Vitest where describe callbacks must be synchronous. The syntax `describe("name", async () => { ... })` is valid in Bun.
Applied to files:
test/js/valkey/valkey.test.ts
📚 Learning: 2025-09-16T19:58:14.006Z
Learnt from: markovejnovic
PR: oven-sh/bun#22568
File: src/valkey/js_valkey_functions.zig:671-682
Timestamp: 2025-09-16T19:58:14.006Z
Learning: In Bun's Redis client implementation (src/valkey/js_valkey_functions.zig), the psubscribe and punsubscribe functions are not implemented nor supported - they are intentionally left as basic wrappers without proper subscription handling.
Applied to files:
src/valkey/js_valkey.zigsrc/valkey/js_valkey_functions.zig
🧬 Code graph analysis (1)
test/js/valkey/valkey.test.ts (2)
packages/bun-types/redis.d.ts (2)
RedisClient(54-2347)redis(2358-2358)test/harness.ts (1)
bunExe(103-106)
🪛 Gitleaks (8.28.0)
test/js/valkey/valkey.test.ts
[high] 2224-2224: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 2225-2225: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 2310-2310: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 2311-2311: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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). (1)
- GitHub Check: Format
🔇 Additional comments (4)
src/valkey/js_valkey_functions.zig (4)
1056-1078: LGTM! New no-argument command wrapper factory.The new
@"()"compile helper correctly implements zero-argument command wrappers following the established pattern. It properly enforces client state constraints and handles errors consistently with other command wrappers.
634-672: LGTM! Bit, expiration, scan, and sorted set command bindings.The new command bindings for bit operations (getbit, setbit), range operations (getrange, setrange), expiration commands (expireat, pexpire, pexpireat), scanning (randomkey, scan), and sorted set operations (zcount, zlexcount, zrange family) are correctly implemented. All use appropriate wrapper factories and
.not_subscriberconstraints.
677-703: LGTM! Increment, sorted set, and blocking operation bindings.The bindings for increment/decrement operations (incrby, incrbyfloat, decrby), expiration set commands (setex, psetex), sorted set operations (zincrby, zmscore, zadd, zscan), set operations (zdiff, zinter, zunion families), and blocking pop operations (bzmpop, bzpopmin, bzpopmax) are correctly implemented with appropriate wrapper factories and constraints.
Note: The blocking commands (bzmpop, bzpopmin, bzpopmax) will block the Redis connection until data is available or timeout occurs. Ensure the protocol layer handles these semantics appropriately.
707-730: LGTM! Multi-set, sorted set removal, and key management bindings.The bindings for multi-set operations (mset, msetnx), sorted set range operations (zrangestore, zrem, zremrangebylex, zremrangebyrank, zremrangebyscore), and key management commands (copy, unlink, touch, rename, renamenx) are correctly implemented with appropriate wrappers and parameter names matching Redis semantics.
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)
src/bun.js/api/valkey.classes.ts (1)
321-363: Addlength: 3tozcountandzlexcountdefinitionsIn src/bun.js/api/valkey.classes.ts, update both entries to include a
length: 3property:zcount: { fn: "zcount", + length: 3, }, zlexcount: { fn: "zlexcount", + length: 3, },
♻️ Duplicate comments (5)
src/bun.js/api/valkey.classes.ts (5)
146-177: Critical: Missing length properties for fixed-arity commands.The blocking list commands and bit/range commands are missing their
lengthproperties, which should enforce their fixed arities:
blmove: length 5 (source, dest, wherefrom, whereto, timeout)blmpop: variable arity, omit length ✓blpop: variable arity (min 2), omit length ✓brpop: variable arity (min 2), omit length ✓brpoplpush: length 3 (source, dest, timeout)getbit: length 2 (key, offset) - flagged in past reviewsetbit: length 3 (key, offset, value) - flagged in past reviewgetrange: length 3 (key, start, end) - flagged in past reviewsetrange: length 3 (key, offset, value) - flagged in past reviewApply this diff:
blmove: { fn: "blmove", + length: 5, }, blmpop: { fn: "blmpop", - length: 3, }, blpop: { fn: "blpop", - length: 2, }, brpop: { fn: "brpop", - length: 2, }, brpoplpush: { fn: "brpoplpush", length: 3, }, getbit: { fn: "getbit", + length: 2, }, setbit: { fn: "setbit", + length: 3, }, getrange: { fn: "getrange", + length: 3, }, setrange: { fn: "setrange", + length: 3, },Based on past review comments.
260-262: Missing length property for randomkey.The
randomkeycommand takes no arguments and should specifylength: 0.Apply this diff:
randomkey: { fn: "randomkey", + length: 0, },Based on past review comments.
315-317: Missing length property for type command.The
typecommand requires exactly one argument and should specifylength: 1.Apply this diff:
type: { fn: "type", + length: 1, },Based on past review comments.
388-393: Missing length properties for setex/psetex commands.Both commands require exactly three arguments and should specify
length: 3.Apply this diff:
setex: { fn: "setex", + length: 3, }, psetex: { fn: "psetex", + length: 3, },Based on past review comments.
473-488: Missing length properties for zrem commands and key management.*Reviewing:
zrem: variable arity (min 2), omit length ✓zremrangebylex: 3 args (key, min, max), should have length 3zremrangebyrank: 3 args (key, start, stop), should have length 3zremrangebyscore: 3 args (key, min, max), should have length 3copy: variable arity (min 2), omit length ✓unlink: variable arity (min 1), omit length ✓touch: variable arity (min 1), omit length ✓rename: 2 args (oldkey, newkey), should have length 2 - flagged in past reviewrenamenx: 2 args (oldkey, newkey), should have length 2 - flagged in past reviewApply this diff:
zremrangebylex: { fn: "zremrangebylex", + length: 3, }, zremrangebyrank: { fn: "zremrangebyrank", + length: 3, }, zremrangebyscore: { fn: "zremrangebyscore", + length: 3, }, rename: { fn: "rename", + length: 2, }, renamenx: { fn: "renamenx", + length: 2, },Based on past review comments and consistency with Redis command arities.
🧹 Nitpick comments (2)
src/valkey/js_valkey_functions.zig (2)
639-642: Consider fixed-arity wrappers for bit and range commands.Commands like
getbit,setbit,getrange, andsetrangehave well-defined arities:
GETBIT: 2 args (key, offset)SETBIT: 3 args (key, offset, value)GETRANGE: 3 args (key, start, end)SETRANGE: 3 args (key, offset, value)Using the variadic wrapper
@"(...strings: string[])"doesn't enforce these requirements at the Zig layer. Consider using the more specific factories like@"(key: RedisKey, value: RedisValue)"forgetbitor creating a three-argument factory for the others.For example,
getbitcould use:-pub const getbit = compile.@"(...strings: string[])"("getbit", "GETBIT", .not_subscriber).call; +pub const getbit = compile.@"(key: RedisKey, value: RedisValue)"("getbit", "GETBIT", "key", "offset", .not_subscriber).call;
634-638: Specific type signatures exist inpackages/bun-types/redis.d.tsfor all five commands ([script verification]):
blmove(source: KeyLike, destination: KeyLike, ...)andbrpoplpush(source: KeyLike, destination: KeyLike, timeout: number)both have fixed-arity signatures.blmpop(timeout: number, numkeys: number, ...args: (string | number)[])enforces the required first two numeric parameters.blpop(...args: (KeyLike | number)[])andbrpop(...args: (KeyLike | number)[])remain fully variadic and do not enforce the minimum two arguments.Ensure that those two commands also get specific overloads or fixed-arity type signatures (e.g.
(key: KeyLike, timeout: number)plus(keys: KeyLike[], timeout: number)) to match Redis’s requirements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
packages/bun-types/redis.d.ts(17 hunks)src/bun.js/api/valkey.classes.ts(7 hunks)src/valkey/js_valkey.zig(2 hunks)src/valkey/js_valkey_functions.zig(4 hunks)test/js/valkey/unit/set-operations.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/valkey/unit/set-operations.test.ts
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/valkey/unit/set-operations.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/valkey/unit/set-operations.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/js/valkey/unit/set-operations.test.ts
test/js/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests for specific features under
test/js/by module
Files:
test/js/valkey/unit/set-operations.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must be placed under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or use custom random port functions
In tests, use normalizeBunSnapshot when asserting snapshots
Never write tests that merely assert absence of "panic" or "uncaught exception" in output
Avoid shell commands (e.g., find, grep) in tests; use Bun.Glob and built-ins instead
Prefer snapshot tests over exact stdout equality assertions
Files:
test/js/valkey/unit/set-operations.test.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript files with Prettier (bun run prettier)
Files:
test/js/valkey/unit/set-operations.test.tspackages/bun-types/redis.d.tssrc/bun.js/api/valkey.classes.ts
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/valkey/js_valkey.zigsrc/valkey/js_valkey_functions.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/valkey/js_valkey.zigsrc/valkey/js_valkey_functions.zig
src/**/js_*.zig
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
src/**/js_*.zig: Implement JavaScript bindings in a Zig file named with a js_ prefix (e.g., js_smtp.zig, js_your_feature.zig)
Handle reference counting correctly with ref()/deref() in JS-facing Zig code
Always implement proper cleanup in deinit() and finalize() for JS-exposed types
Files:
src/valkey/js_valkey.zigsrc/valkey/js_valkey_functions.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/valkey/js_valkey.zigsrc/valkey/js_valkey_functions.zig
**/*.classes.ts
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
**/*.classes.ts: Define JavaScript class interfaces in .classes.ts using define({ name, constructor, JSType, finalize, proto: { ... } })
Use cache: true on properties that should use JSC WriteBarrier-backed caching
Files:
src/bun.js/api/valkey.classes.ts
🧠 Learnings (1)
📚 Learning: 2025-09-16T19:58:14.006Z
Learnt from: markovejnovic
PR: oven-sh/bun#22568
File: src/valkey/js_valkey_functions.zig:671-682
Timestamp: 2025-09-16T19:58:14.006Z
Learning: In Bun's Redis client implementation (src/valkey/js_valkey_functions.zig), the psubscribe and punsubscribe functions are not implemented nor supported - they are intentionally left as basic wrappers without proper subscription handling.
Applied to files:
src/valkey/js_valkey_functions.zig
🧬 Code graph analysis (1)
test/js/valkey/unit/set-operations.test.ts (1)
test/js/valkey/test-utils.ts (1)
expectType(496-501)
⏰ 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). (1)
- GitHub Check: Format
🔇 Additional comments (33)
src/valkey/js_valkey_functions.zig (4)
1080-1102: LGTM! Well-structured no-argument command factory.The new
@"()"factory method correctly implements the pattern for commands that take zero arguments. It properly:
- Validates client state based on the requirement
- Sends a command with an empty args array
- Returns a promise or JS error
This follows the same structure as the existing factories for other arities.
669-669: Add missing length validation for randomkey.The
randomkeycommand correctly uses the new no-argument factory@"()", which enforces zero arguments. Good consistency!
701-703: LGTM! Correct factories for increment/decrement commands.The
incrby,incrbyfloat, anddecrbycommands correctly use the two-argument factory pattern with appropriate parameter names ("increment", "decrement").
750-754: LGTM! Key management commands use appropriate factories.The key management commands correctly use:
- Variadic for
copy(which accepts options)- Multi-key for
unlinkandtouch(variadic key lists)- Two-argument for
renameandrenamenx(oldkey, newkey)test/js/valkey/unit/set-operations.test.ts (6)
299-322: LGTM! Comprehensive SUNION test.The test correctly verifies:
- Union across three sets
- All unique elements are present in the result
- Array type validation
324-336: LGTM! Good edge case coverage for non-existent keys.The test correctly validates that SUNION treats non-existent keys as empty sets, which is the expected Redis behavior.
338-382: LGTM! Thorough SUNIONSTORE testing.The tests correctly verify:
- Basic union storage
- Destination key overwrite behavior (lines 363-382)
- Return value (count of unique members)
384-403: LGTM! SDIFF test covers multiple sets correctly.The test validates:
- Difference across three sets
- Elements only in the first set remain
- Elements in subsequent sets are excluded
405-432: LGTM! Good edge case coverage for SDIFF.The tests correctly handle:
- Single set (returns all elements)
- Non-existent keys (treated as empty sets)
434-444: Add missing inline snapshot for the SDIFF error test
Replace the emptytoThrowErrorMatchingInlineSnapshot()call withtoThrowErrorMatchingInlineSnapshot(\`)`, then rerun the test runner to auto-generate and commit the actual error message snapshot.src/bun.js/api/valkey.classes.ts (5)
51-66: LGTM! Correct length properties for increment/decrement commands.The
incrby,incrbyfloat, anddecrbycommands correctly specifylength: 2, matching their two-argument signatures (key, value).
75-82: LGTM! Correct length properties for expiration commands.The
expireatandpexpirecommands correctly specifylength: 2for their (key, timestamp/milliseconds) signatures.
266-314: Missing length properties for set commands.Several set commands have specific arities:
rpoplpush: length 2 (source, dest) ✓ (already has length)sdiff: variable arity (min 1), length: 1 ✓ (already has length)sdiffstore: variable arity (min 2), length: 2 ✓ (already has length)sinter: variable arity (min 1), length: 1 ✓ (already has length)sintercard: variable arity (min 1), length: 1 ✓ (already has length)sinterstore: variable arity (min 2), length: 2 ✓ (already has length)smismember: variable arity (min 2), length: 2 ✓ (already has length)sscan: length 2 (key, cursor) ✓ (already has length)sunion: variable arity (min 1), length: 1 ✓ (already has length)sunionstore: variable arity (min 2), length: 2 ✓ (already has length)All look correct!
458-463: Missing length properties for mset/msetnx.Both commands accept variable key-value pairs and correctly omit the
lengthproperty.
205-246: Missing length properties for list commands.Several list commands have fixed arities and should specify their
length:
lindex: length 2 (key, index)linsert: length 4 (key, BEFORE|AFTER, pivot, element)lmove: length 4 (source, dest, wherefrom, whereto)lmpop: variable arity, omit length ✓lpos: variable arity (min 2), omit length ✓lrange: length 3 (key, start, stop)lrem: length 3 (key, count, element)lset: length 3 (key, index, element)ltrim: length 3 (key, start, stop)Apply this diff:
lindex: { fn: "lindex", - length: 2, }, linsert: { fn: "linsert", - length: 4, }, lmove: { fn: "lmove", - length: 4, }, lmpop: { fn: "lmpop", - length: 2, }, lpos: { fn: "lpos", - length: 2, }, lrange: { fn: "lrange", - length: 3, }, lrem: { fn: "lrem", - length: 3, }, lset: { fn: "lset", - length: 3, }, ltrim: { fn: "ltrim", - length: 3, },Wait, reviewing again: The current code already has length properties for most of these. Let me verify which ones actually need changes.
Actually, looking at lines 205-246, they already have length properties specified correctly. Let me re-check the annotations.
Likely an incorrect or invalid review comment.
packages/bun-types/redis.d.ts (17)
37-37: LGTM: Cleaner TLS options type.The refinement to use
Bun.TLSOptionsinstead of an explicit object union reduces duplication and improves maintainability.
241-270: LGTM: Increment/decrement commands are well-typed.The new
incrby,incrbyfloat, anddecrbymethods have correct parameter and return types that match Redis semantics. Note thatincrbyfloatcorrectly returnsPromise<string>to preserve floating-point precision.
288-303: LGTM: Expiration commands are correctly typed.The new expiration methods (
expireat,pexpire,pexpireat) have correct parameter types and clear documentation about time units (seconds vs. milliseconds).
386-411: LGTM: Set operation commands are well-defined.The new set operations (
sdiffstore,smismember,sscan) have correct signatures and clear documentation. The return types properly match Redis behavior (e.g.,smismemberreturns an array of membership flags).
466-599: LGTM: Blocking list operations are comprehensive.The blocking list operations (
blpop,brpop,blmove,blmpop,brpoplpush) have excellent documentation with examples, correct parameter types (timeout as number allowing fractional seconds), and appropriate return types for blocking operations.
601-753: LGTM: List operations are well-documented.The list operations (
lindex,lmove,lpos,lmpop,lrange,lset) feature excellent documentation with clear examples. Thelposreturn type correctly handles both single and multiple results depending on options.
787-927: LGTM: Scan operations have comprehensive overloads.The
randomkeyandscancommands are well-defined. The scan overloads appropriately cover different option combinations (MATCH, COUNT) with a final variadic overload for flexibility. Excellent documentation with iteration examples.
936-1024: LGTM: Set operations are correctly typed.The set operations (
sdiff,sinter,sinterstore,sintercard,sunion,sunionstore) have appropriate signatures. Thetypecommand return type correctly enumerates all possible Redis data types including "none".
1033-1098: LGTM: Sorted set count and blocking pop operations.The sorted set operations (
zcount,zlexcount,bzpopmin,bzpopmax) have correct parameter types supporting both numeric and string ranges for flexibility (e.g., "-inf", "+inf").
1107-1270: LGTM: Comprehensive zrange/zrevrange overloads.The extensive overloads for
zrangeandzrevrangeappropriately handle the flexible Redis API, including WITHSCORES (returning tuples), BYSCORE, BYLEX, and REV options. The type system correctly distinguishes return types based on options.
1290-1430: LGTM: List modification and expiration commands.The list operations (
linsert,lrem,ltrim) and expiration setters (setex,psetex) have clear signatures with good documentation. Thelremcount parameter behavior is well-documented.
1432-1563: LGTM: Sorted set score operations are well-defined.The score operations (
zscore,zincrby,zmscore) have correct return types handling nullable scores. Thezaddcommand has comprehensive documentation explaining all options (NX, XX, GT, LT, CH, INCR) with examples.
1565-1905: LGTM: Extensive sorted set range operations.The sorted set range operations (
zrem*,zrangebylex,zrangebyscore,zrevrangebyscore,zrevrangebylex) have comprehensive overloads handling different option combinations. The documentation clearly explains range syntax (inclusive/exclusive bounds) and pagination with LIMIT.
1907-1944: LGTM: zrangestore is well-documented.The
zrangestorecommand correctly stores ZRANGE results and returns the count. The documentation includes multiple examples demonstrating different option combinations.
1954-2043: LGTM: Multi-key and bit/range operations.The multi-key operations (
mset,msetnx) correctly use variadic parameters for key-value pairs with clear documentation on atomic behavior. Bit operations (getbit,setbit) and range operations (getrange,setrange) have appropriate return types.
2265-2388: LGTM: Key management operations are complete.The key management commands (
copy,unlink,touch,rename,renamenx) have correct signatures with clear documentation. Thecopyoverloads appropriately handle the optional REPLACE flag.
2390-2783: LGTM: Set operation commands for sorted sets.The sorted set operations (
zdiff,zinter,zunionand their store variants) have comprehensive overloads supporting WEIGHTS, AGGREGATE, and WITHSCORES options. Thezmpopandbzmpopcommands correctly handle multiple pops with direction and COUNT options. Excellent documentation throughout.src/valkey/js_valkey.zig (1)
1246-1384: LGTM: Zig bindings align with TypeScript API.The new public bindings expose a comprehensive set of Redis commands that match the TypeScript declarations in
packages/bun-types/redis.d.ts. The consistent pattern (pub const <name> = fns.<name>) properly delegates to the implementation functions. This includes:
- Blocking list operations (blpop, brpop, blmove, blmpop, brpoplpush)
- Set operations (sdiff, sdiffstore, sinter, sintercard, sinterstore, sunion, sunionstore)
- Sorted set operations (zrange family, zincrby, zmscore, zadd, zscan, zdiff/zinter/zunion families, zmpop, bzmpop, bzpopmin, bzpopmax)
- Key management (copy, unlink, touch, rename, renamenx)
- Increment/decrement (incrby, incrbyfloat, decrby)
- Bit/range operations (getbit, setbit, getrange, setrange)
- Expiration (expireat, pexpire, pexpireat)
- Scan operations (scan, sscan)
- And more
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/bun-types/redis.d.ts (1)
125-126: Fix invalid return type for getBufferTypeScript typed arrays aren’t generic. Use Uint8Array, not Uint8Array.
- getBuffer(key: RedisClient.KeyLike): Promise<Uint8Array<ArrayBuffer> | null>; + getBuffer(key: RedisClient.KeyLike): Promise<Uint8Array | null>;src/valkey/js_valkey.zig (2)
513-537: getOrCreateSubscriptionCtx doesn’t compile (wrong optional usage and init arity)SubscriptionCtx is not optional, and init takes only (valkey_parent). Simplify to return the existing context; initialize earlier as done in constructor/duplicate.
pub fn getOrCreateSubscriptionCtx( this: *JSValkeyClient, ) bun.JSError!*SubscriptionCtx { - if (this._subscription_ctx) |*ctx| { - // If we already have a subscription context, return it - return ctx; - } - - // Save the original flag values and create a new subscription context - this._subscription_ctx = try SubscriptionCtx.init( - this, - this.client.flags.enable_offline_queue, - this.client.flags.enable_auto_pipelining, - ); - - // We need to make sure we disable the offline queue, but we actually want to make sure that our HELLO message - // goes through first. Consequently, we only disable the offline queue if we're already connected. - if (this.client.status == .connected) { - this.client.flags.enable_offline_queue = false; - } - - this.client.flags.enable_auto_pipelining = false; - - return &(this._subscription_ctx.?); + // Context is created in constructor/duplicate; just return it. + return &this._subscription_ctx; }
1429-1435: Wrong receiver: failWithJSValue is a JSValkeyClient method, not ValkeyClientUse this.failWithJSValue(...).
- this.client.failWithJSValue(this.globalObject, ssl_error.toJS(this.globalObject)); + this.failWithJSValue(ssl_error.toJS(this.globalObject));
♻️ Duplicate comments (7)
src/bun.js/api/valkey.classes.ts (6)
170-181: Add length for fixed-arity bit/range commandsgetbit: { fn: "getbit", + length: 2, }, setbit: { fn: "setbit", + length: 3, }, getrange: { fn: "getrange", + length: 3, }, setrange: { fn: "setrange", + length: 3, },
264-266: randomkey should declare zero arityrandomkey: { fn: "randomkey", + length: 0, },
319-321: type should declare single-argument aritytype: { fn: "type", + length: 1, },
392-397: setex/psetex require 3 argssetex: { fn: "setex", + length: 3, }, psetex: { fn: "psetex", + length: 3, },
491-492: rename/renamenx require 2 args- rename: { fn: "rename" }, - renamenx: { fn: "renamenx" }, + rename: { fn: "rename", length: 2 }, + renamenx: { fn: "renamenx", length: 2 },
401-403: Add length for zincrby (3 args)zincrby takes key, increment, member.
zincrby: { fn: "zincrby", + length: 3, },src/valkey/js_valkey_functions.zig (1)
756-761: Enforce fixed arity for setex/psetex/zincrby/zmscoreCurrent variadic wrappers allow wrong arg counts/order. Use explicit signatures or add a 3‑arg compile factory.
Examples:
// setex(key, seconds, value) pub const setex = compile.@"(key: RedisKey, value: RedisValue, another: RedisValue)"("setex", "SETEX", "key", "seconds", "value", .not_subscriber).call; // psetex(key, milliseconds, value) pub const psetex = compile.@"(key: RedisKey, value: RedisValue, another: RedisValue)"("psetex", "PSETEX", "key", "milliseconds", "value", .not_subscriber).call; // zincrby(key, increment, member) pub const zincrby = compile.@"(key: RedisKey, value: RedisValue, another: RedisValue)"("zincrby", "ZINCRBY", "key", "increment", "member", .not_subscriber).call; // zmscore(key, member, ...members) pub const zmscore = compile.@"(key: RedisKey, value: RedisValue, ...args: RedisValue)"("zmscore", "ZMSCORE", .not_subscriber).call;If the 3‑arg factory doesn’t exist, add it similarly to other compile helpers.
🧹 Nitpick comments (2)
packages/bun-types/redis.d.ts (1)
398-399: Broaden member parameter types to match runtime (buffers allowed)sismember/sadd/srem accept binary values at runtime; typings restrict to string.
- sismember(key: RedisClient.KeyLike, member: string): Promise<boolean>; + sismember(key: RedisClient.KeyLike, member: RedisClient.KeyLike): Promise<boolean>; - sadd(key: RedisClient.KeyLike, member: string): Promise<number>; + sadd(key: RedisClient.KeyLike, member: RedisClient.KeyLike): Promise<number>; - srem(key: RedisClient.KeyLike, member: string): Promise<number>; + srem(key: RedisClient.KeyLike, member: RedisClient.KeyLike): Promise<number>;Also applies to: 407-408, 416-417
src/bun.js/api/valkey.classes.ts (1)
325-330: Add length for zcount and zlexcount (3 args each)These are fixed-arity (key, min, max).
zcount: { fn: "zcount", + length: 3, }, zlexcount: { fn: "zlexcount", + length: 3, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/bun-types/redis.d.ts(13 hunks)src/bun.js/api/valkey.classes.ts(8 hunks)src/valkey/js_valkey.zig(2 hunks)src/valkey/js_valkey_functions.zig(5 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.classes.ts
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
**/*.classes.ts: Define JavaScript class interfaces in .classes.ts using define({ name, constructor, JSType, finalize, proto: { ... } })
Use cache: true on properties that should use JSC WriteBarrier-backed caching
Files:
src/bun.js/api/valkey.classes.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript files with Prettier (bun run prettier)
Files:
src/bun.js/api/valkey.classes.tspackages/bun-types/redis.d.ts
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/valkey/js_valkey.zigsrc/valkey/js_valkey_functions.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/valkey/js_valkey.zigsrc/valkey/js_valkey_functions.zig
src/**/js_*.zig
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
src/**/js_*.zig: Implement JavaScript bindings in a Zig file named with a js_ prefix (e.g., js_smtp.zig, js_your_feature.zig)
Handle reference counting correctly with ref()/deref() in JS-facing Zig code
Always implement proper cleanup in deinit() and finalize() for JS-exposed types
Files:
src/valkey/js_valkey.zigsrc/valkey/js_valkey_functions.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/valkey/js_valkey.zigsrc/valkey/js_valkey_functions.zig
🧠 Learnings (1)
📚 Learning: 2025-09-16T19:58:14.006Z
Learnt from: markovejnovic
PR: oven-sh/bun#22568
File: src/valkey/js_valkey_functions.zig:671-682
Timestamp: 2025-09-16T19:58:14.006Z
Learning: In Bun's Redis client implementation (src/valkey/js_valkey_functions.zig), the psubscribe and punsubscribe functions are not implemented nor supported - they are intentionally left as basic wrappers without proper subscription handling.
Applied to files:
src/valkey/js_valkey_functions.zig
⏰ 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). (1)
- GitHub Check: Format
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/valkey/js_valkey_functions.zig (1)
908-912: LGTM! Fixed arity for SETEX, PSETEX, ZINCRBY, and ZMSCORE.These commands now use appropriate fixed-arity signatures instead of variadic wrappers, which enforces correct argument counts and order.
🧹 Nitpick comments (1)
src/valkey/js_valkey_functions.zig (1)
732-732: Consider fixed arity for HGETDEL.HGETDEL requires exactly 2 arguments (key, field). Using a variadic wrapper allows incorrect invocations.
If stricter validation is desired, replace with:
pub const hgetdel = compile.@"(key: RedisKey, value: RedisValue)"("hgetdel", "HGETDEL", "key", "field", .not_subscriber).call;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/valkey/js_valkey_functions.zig(11 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/valkey/js_valkey_functions.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/valkey/js_valkey_functions.zig
src/**/js_*.zig
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
src/**/js_*.zig: Implement JavaScript bindings in a Zig file named with a js_ prefix (e.g., js_smtp.zig, js_your_feature.zig)
Handle reference counting correctly with ref()/deref() in JS-facing Zig code
Always implement proper cleanup in deinit() and finalize() for JS-exposed types
Files:
src/valkey/js_valkey_functions.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/valkey/js_valkey_functions.zig
🧠 Learnings (1)
📚 Learning: 2025-09-16T19:58:14.006Z
Learnt from: markovejnovic
PR: oven-sh/bun#22568
File: src/valkey/js_valkey_functions.zig:671-682
Timestamp: 2025-09-16T19:58:14.006Z
Learning: In Bun's Redis client implementation (src/valkey/js_valkey_functions.zig), the psubscribe and punsubscribe functions are not implemented nor supported - they are intentionally left as basic wrappers without proper subscription handling.
Applied to files:
src/valkey/js_valkey_functions.zig
⏰ 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). (1)
- GitHub Check: Format
🔇 Additional comments (7)
src/valkey/js_valkey_functions.zig (7)
619-719: LGTM! Well-structured generalized HSET/HMSET implementation.The three-pattern support (object, array, variadic) with centralized validation and error handling is clean and maintainable. Memory management is correct with proper defer cleanup.
721-727: LGTM! Clean delegation to shared implementation.
745-798: LGTM! Correct implementation of HSETNX and HEXISTS with boolean conversion.
933-961: LGTM! Correct SMOVE implementation with boolean conversion.
1304-1326: LGTM! Well-designed zero-argument command wrapper.This new wrapper cleanly handles stateless commands like RANDOMKEY, maintaining consistency with the other compile-time wrapper patterns.
1441-1479: LGTM! Properly implements three-argument command wrapper.This wrapper correctly enforces fixed arity for commands requiring exactly three arguments, with proper validation and cleanup.
837-837: Fix BRPOPLPUSH signature mismatch.The signature declares three argument names
("source", "destination", "timeout")but uses the two-argument wrapper@"(key: RedisKey, value: RedisValue)". BRPOPLPUSH requires exactly 3 arguments.Apply this diff:
-pub const brpoplpush = compile.@"(key: RedisKey, value: RedisValue, value2: RedisValue)"("brpoplpush", "BRPOPLPUSH", "source", "destination", "timeout", .not_subscriber).call;⛔ Skipped due to learnings
Learnt from: markovejnovic PR: oven-sh/bun#22568 File: src/valkey/js_valkey_functions.zig:671-682 Timestamp: 2025-09-16T19:58:14.006Z Learning: In Bun's Redis client implementation (src/valkey/js_valkey_functions.zig), the psubscribe and punsubscribe functions are not implemented nor supported - they are intentionally left as basic wrappers without proper subscription handling.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/valkey/js_valkey_functions.zig (2)
346-352: Remove incorrect WITHSCORES handling in srandmember.SRANDMEMBER does not support a WITHSCORES argument—that option is for sorted set commands like ZRANGE. SRANDMEMBER accepts only an optional
countargument to return multiple members.Apply this diff to remove the incorrect logic:
- // Optional WITHSCORES argument - if (args_view.len > 2 and !callframe.argument(2).isUndefinedOrNull()) { - const withscores_arg = try fromJS(globalObject, callframe.argument(2)) orelse { - return globalObject.throwInvalidArgumentType("srandmember", "withscores", "string"); - }; - args.appendAssumeCapacity(withscores_arg); - }
924-928: Use fixed-arity compile signatures for SETEX, PSETEX, ZINCRBY, and ZMSCORE.These commands have well-defined arities and using the three-argument wrapper
compile.@"(key: RedisKey, value: RedisValue, value2: RedisValue)"for SETEX/PSETEX/ZINCRBY would enforce correct argument order and count at compile time. For ZMSCORE, usecompile.@"(key: RedisKey, value: RedisValue, ...args: RedisValue)"to require at least one member.Apply these changes:
-pub const setex = compile.@"(key: RedisKey, value: RedisValue, value2: RedisValue)"("setex", "SETEX", "key", "seconds", "value", .not_subscriber).call; -pub const psetex = compile.@"(key: RedisKey, value: RedisValue, value2: RedisValue)"("psetex", "PSETEX", "key", "milliseconds", "value", .not_subscriber).call; +// Already using correct signature (no change needed based on line 924-925) -pub const zincrby = compile.@"(key: RedisKey, value: RedisValue, value2: RedisValue)"("zincrby", "ZINCRBY", "key", "increment", "member", .not_subscriber).call; -pub const zmscore = compile.@"(key: RedisKey, value: RedisValue, ...args: RedisValue)"("zmscore", "ZMSCORE", .not_subscriber).call; +// Already using correct signature (no change needed based on line 927-928)Note: Upon closer inspection, lines 924-928 already use the correct three-argument signature. This comment is flagged as duplicate since the issue was already addressed.
🧹 Nitpick comments (1)
src/valkey/js_valkey_functions.zig (1)
745-760: Consider using more specific compile signatures for fixed-arity hash commands.Several commands like
hgetdel,hsetex,hexpire, etc., have well-defined fixed arities according to Redis documentation. Using the variadiccompile.@"(...strings: string[])"wrapper prevents compile-time arity validation. Consider using more specific signatures likecompile.@"(key: RedisKey, value: RedisValue)"where applicable to catch incorrect usage earlier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/valkey/js_valkey_functions.zig(11 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/valkey/js_valkey_functions.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/valkey/js_valkey_functions.zig
src/**/js_*.zig
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
src/**/js_*.zig: Implement JavaScript bindings in a Zig file named with a js_ prefix (e.g., js_smtp.zig, js_your_feature.zig)
Handle reference counting correctly with ref()/deref() in JS-facing Zig code
Always implement proper cleanup in deinit() and finalize() for JS-exposed types
Files:
src/valkey/js_valkey_functions.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/valkey/js_valkey_functions.zig
🧠 Learnings (1)
📚 Learning: 2025-09-16T19:58:14.006Z
Learnt from: markovejnovic
PR: oven-sh/bun#22568
File: src/valkey/js_valkey_functions.zig:671-682
Timestamp: 2025-09-16T19:58:14.006Z
Learning: In Bun's Redis client implementation (src/valkey/js_valkey_functions.zig), the psubscribe and punsubscribe functions are not implemented nor supported - they are intentionally left as basic wrappers without proper subscription handling.
Applied to files:
src/valkey/js_valkey_functions.zig
🔇 Additional comments (8)
src/valkey/js_valkey_functions.zig (8)
274-318: LGTM! SREM variadic argument handling is correct.The refactored implementation properly validates that at least a key and one member are provided, and correctly handles multiple members with appropriate error handling and cleanup.
393-478: LGTM! SPOP and SADD implementations are correct.Both functions properly handle their argument patterns: SPOP with optional count and SADD with variadic members.
509-569: LGTM! HMGET flexible argument handling is well-implemented.The function correctly supports both array and variadic patterns for specifying fields, with proper validation and error handling.
635-743: Excellent refactoring of HSET/HMSET into shared implementation.The
hsetImplfunction elegantly handles three distinct argument patterns (object, array, variadic) with proper validation of field-value pairs and command-specific error messages. Memory management looks correct with defer cleanup.
761-814: LGTM! HSETNX and HEXISTS correctly return booleans.Both implementations properly use the
return_as_boolmetadata flag to convert Redis integer responses to JavaScript booleans, which is the expected behavior for these commands.
949-977: LGTM! SMOVE implementation is correct.The function properly accepts three arguments (source, destination, member), uses
return_as_boolfor correct boolean conversion, and has appropriate error handling.
1320-1342: LGTM! Zero-argument compile wrapper is well-implemented.The new
compile.@"()"wrapper correctly handles commands that take no arguments (like RANDOMKEY), with proper state validation and error handling.
1457-1495: LGTM! Three-argument compile wrapper is correctly implemented.The
compile.@"(key: RedisKey, value: RedisValue, value2: RedisValue)"wrapper properly validates all three arguments with appropriate error messages and cleanup.
|
very nice work ali |
…s tables (#39271) ### Problem - `class RedisClient` in `packages/bun-types/redis.d.ts` is a hand-written copy of the `proto` table in `src/runtime/valkey_jsc/valkey.classes.ts`, and nothing compares the two. A command added to the table and not to the d.ts works at runtime and fails to type-check (`Property 'pubsub' does not exist on type 'RedisClient'`). - On main, 5 of the 167 table entries are undeclared: `psubscribe`, `pubsub`, `punsubscribe`, `script`, `select`. All five have been registered since the commit that introduced `Bun.redis` (ec87a27, #18812) and stayed undeclared through the 14 redis.d.ts commits since, including the 614-line batch in #23116, so the gap is systematic rather than a one-off. - Each of the five is already being declared by an open PR: `pubsub` and `select` by #39208, `script` by #29339, `psubscribe`/`punsubscribe` by #35521 (which also adds the listener routing they are missing; today `psubscribe()` drops every pmessage). This PR adds the check, not the declarations. ### Fix - `test/internal/source-lints/redis-client-types.test.ts` imports `valkey.classes.ts` (the module the codegen reads; `class-definitions.ts` has no dependencies), collects the members the codegen installs (`proto` entries by name, `klass` entries as `static name`, `constructor` when `construct` is set, skipping `internal`/`privateSymbol`/`publicSymbol` entries, which the codegen does not install under an identifier), parses the member names declared in the `class RedisClient` body of `redis.d.ts` (`[Symbol.x]` maps onto the table's `@@x` spelling), and requires the two sets to be equal. Each direction is its own test, so a table entry without a declaration and a declaration without a table entry are both reported by name. - The five names missing today sit in a `pendingDeclarations` table keyed by the PR that declares each. A fourth test fails as soon as a listed name is declared or unregistered, so the entry gets deleted when its PR lands (whichever of this PR and #39208/#29339/#35521 lands second trips it on rebase, and the message says which entry to delete). A name that is neither declared nor listed fails the lint outright. - Declaration lines the parser does not recognize are reported as failures rather than skipped, so a new d.ts shape cannot silently hide a member from the comparison. - `.github/workflows/source-lints.yml` gains `src/**/*.classes.ts`, `src/codegen/class-definitions.ts` and `packages/bun-types/redis.d.ts` as triggers: the workflow is path-filtered and those are the files this lint reads, so without them the edits it guards would not run it. The directory README now states that rule; a comment on the `proto` table points at the lint. - Scope is RedisClient only. A checker for every `*.classes.ts` needs a class-to-declaration mapping and inheritance handling and is a separate project. - Verified: - `bun test test/internal/source-lints/redis-client-types.test.ts` passes on this branch (4 tests); the whole directory is 170 green. - With `pendingDeclarations` emptied, the lint fails against main's files naming exactly `psubscribe`, `pubsub`, `punsubscribe`, `script`, `select` (output below). - Simulated the eight drift shapes in the details block (new table entry, pending name declared, phantom declaration, pending name unregistered, unparseable member, `@@asyncDispose` + a `klass` entry with and without declarations, `internal`/`privateSymbol` entries); each fails the intended test or passes as intended. - No runtime code changes: the `valkey.classes.ts` hunk is a comment and produces identical codegen output. This PR has no src/packages diff for a fail-before run to strip; the fail-before evidence is the emptied-pending-table run above. ### Background - `*.classes.ts` files are the input of `src/codegen/generate-classes.ts`. `define({ proto, klass, construct })` describes a native class: `proto` entries become properties of the prototype (`fn` methods, `getter`/`setter` accessors), `klass` entries become statics, and `construct: true` makes it newable. Entries with `internal`, `privateSymbol` or `publicSymbol` are installed under private names or `Symbol.for()` symbols (or not at all), and keys spelled `@@x` are installed under the well-known symbol `Symbol.x`. - `packages/bun-types` is the published `@types/bun` surface; it is hand-written, not generated from the class definitions, which is why it can drift. - `test/internal/source-lints/` holds tests that only read the source tree; `.buildkite/ci.mjs` excludes the directory from the binary lanes and `source-lints.yml` runs it against a released bun on a bare checkout, so tests there may only import built-ins and relative paths. <details> <summary>Lint output against main's files with the pending table emptied, and the simulated drift shapes</summary> ``` (pass) every member of class RedisClient in packages/bun-types/redis.d.ts has a shape this lint can read (fail) packages/bun-types/redis.d.ts declares every RedisClient member src/runtime/valkey_jsc/valkey.classes.ts installs - [] + [ + "psubscribe", + "pubsub", + "punsubscribe", + "script", + "select", + ] (pass) packages/bun-types/redis.d.ts declares no RedisClient member src/runtime/valkey_jsc/valkey.classes.ts does not install (pass) pendingDeclarations lists only members that are still registered and still undeclared ``` ``` ### proto gains waitaof, d.ts untouched + "waitaof", (fail) packages/bun-types/redis.d.ts declares every RedisClient member src/runtime/valkey_jsc/valkey.classes.ts installs ### d.ts declares select while it is still pending + "select (#39208) is declared in packages/bun-types/redis.d.ts now; delete its entry", (fail) pendingDeclarations lists only members that are still registered and still undeclared ### d.ts declares flushall, which is not registered + "flushall", (fail) packages/bun-types/redis.d.ts declares no RedisClient member src/runtime/valkey_jsc/valkey.classes.ts does not install ### proto drops script while it is still pending + "script (#29339) is no longer registered in src/runtime/valkey_jsc/valkey.classes.ts", (fail) pendingDeclarations lists only members that are still registered and still undeclared ### d.ts gains `private brand: never;` + "private brand: never;", (fail) every member of class RedisClient in packages/bun-types/redis.d.ts has a shape this lint can read ### proto gains "@@asyncDispose" and a klass entry, d.ts declares [Symbol.asyncDispose]() and the static 4 pass ### same, with nothing declared + "@@asyncDispose", + "static parseURL", (fail) packages/bun-types/redis.d.ts declares every RedisClient member src/runtime/valkey_jsc/valkey.classes.ts installs ### proto gains an `internal: true` entry and a `privateSymbol` entry, d.ts untouched 4 pass ``` </details>
3 participants
What does this PR do?
Fixes #19830
Fixes #19361
Fixes #22526
Fixes #19624
Supersedes #23068
How did you verify your code works?
Lots of tests!