feat(standard-server-peer)!: use string instead of number for unique ID - #682
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis change refactors the identifier types used throughout the codebase from Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Codec
participant Queue
participant Server
Client->>Codec: encodeRequestMessage(id: string, ...)
Codec-->>Client: EncodedMessage
Client->>Server: send EncodedMessage
Server->>Codec: decodeRequestMessage(EncodedMessage)
Codec-->>Server: [id: string, type, payload]
Server->>Queue: open(id: string)
Server->>Queue: push(id: string, payload)
Server->>Codec: encodeResponseMessage(id: string, ...)
Codec-->>Server: EncodedResponse
Server->>Client: send EncodedResponse
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code Graph Analysis (1)packages/shared/src/id.test.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
dosubot
Bot
added
the
size:XL
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @unnoq, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a significant improvement to the system's unique ID generation by transitioning from numeric IDs to string-based IDs. This change enhances the robustness and scalability of ID management, eliminating concerns about number range limits and ensuring long-term uniqueness. The update impacts various components, from core ID generation logic to client-server communication protocols and internal data handling, all of which have been meticulously adjusted and tested to support the new string ID format.
Highlights
- ID Type Change: The core change involves migrating the unique ID type from
numbertostringacross the codebase. This addresses the inherent limitations of number ranges, ensuring IDs remain unique without needing resets. - Sequential ID Generation: The
SequentialIdGeneratorhas been refactored to produce unique string-based IDs using a base-32 incrementing logic, replacing the previous numeric increment andNumber.MAX_SAFE_INTEGERreset mechanism. - API and Internal Type Updates: All relevant API functions, internal data structures (like
AsyncIdQueueand controller maps), and message encoding/decoding logic have been updated to consistently use string IDs, ensuring type safety and compatibility with the new ID format. - Test Suite Alignment: Corresponding test files across client, server, and shared packages have been updated to reflect the new string ID type, ensuring that the ID generation and usage behave as expected with the new format.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Code Review
The pull request modifies the unique ID used in the standard server peer to be a string instead of a number. This change addresses the limitation of number ranges and ensures unique IDs. The code changes include updates to tests and the implementation of the SequentialIdGenerator to generate string-based IDs.
More templates
@orpc/arktype
@orpc/client
@orpc/contract
@orpc/experimental-durable-event-iterator
@orpc/hey-api
@orpc/nest
@orpc/openapi
@orpc/openapi-client
@orpc/react
@orpc/react-query
@orpc/server
@orpc/shared
@orpc/solid-query
@orpc/standard-server
@orpc/standard-server-aws-lambda
@orpc/standard-server-fetch
@orpc/standard-server-node
@orpc/standard-server-peer
@orpc/svelte-query
@orpc/tanstack-query
@orpc/valibot
@orpc/vue-colada
@orpc/vue-query
@orpc/zod
commit: |
There was a problem hiding this comment.
Pull Request Overview
This PR implements a breaking change to use string-based unique IDs instead of numbers across various modules in the standard-server-peer and related packages to guarantee uniqueness and avoid numeric overflow issues.
- Updated API signatures, internal maps, and helper functions to use string IDs.
- Adjusted tests and client/server adapter implementations accordingly.
- Migrated the sequential ID generator and queue operations to operate over strings.
Reviewed Changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/standard-server/src/hibernation.ts | Changed hibernation callback parameter from number to string. |
| packages/standard-server/src/hibernation.test.ts | Updated tests to reflect string ID usage in callbacks. |
| packages/standard-server-peer/src/server.ts | Modified Map key types and method parameters to use string IDs. |
| packages/standard-server-peer/src/server.test.ts | Adjusted test expectations to use string IDs across request/response flows. |
| packages/standard-server-peer/src/event-iterator.ts & event-iterator.test.ts | Converted function parameters and test cases to support string IDs. |
| packages/standard-server-peer/src/codec.ts & codec.test.ts | Updated the encoding/decoding functions and tests for string-based IDs. |
| packages/standard-server-peer/src/client.ts & client.test.ts | Revised client peer logic and tests to work with string IDs. |
| packages/shared/src/queue.ts & queue.test.ts | Migrated AsyncIdQueue operations from number to string IDs. |
| packages/shared/src/iterator.ts | Adapted iterator logic to open, pull, and close queues using string IDs. |
| packages/shared/src/id.ts & id.test.ts | Updated sequential ID generation to produce and manage string IDs. |
| Other adapter and durable-event-iterator related test files | Ensured all adapters and event iterators use and expect string IDs consistently. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/shared/src/id.ts (1)
10-20: Clarify the non-standard base-32 implementationThe increment logic uses a non-standard base-32 approach where reaching the maximum value (31/'v') appends '0' instead of carrying over to the next position. This generates sequences like:
0,1,2,...,v,v0,v1,...,vv,vv0rather than standard base-32:0,1,2,...,v,10,11,...,1v,20.While this approach guarantees uniqueness and maintains lexicographic ordering, consider documenting this behavior since it differs from typical base-32 arithmetic.
+ /** + * Increments the ID using a custom base-32-like approach. + * Instead of carrying over (v -> 10), appends '0' (v -> v0) for simplicity. + */ private increment(): void {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
packages/client/src/adapters/message-port/rpc-link.test.ts(2 hunks)packages/client/src/adapters/websocket/rpc-link.test.ts(2 hunks)packages/durable-event-iterator/src/durable-object/handler.test.ts(2 hunks)packages/durable-event-iterator/src/durable-object/object.test.ts(1 hunks)packages/durable-event-iterator/src/durable-object/websocket-manager.test.ts(6 hunks)packages/durable-event-iterator/src/durable-object/websocket-manager.ts(2 hunks)packages/server/src/adapters/bun-ws/rpc-handler.test.ts(4 hunks)packages/server/src/adapters/crossws/rpc-handler.test.ts(7 hunks)packages/server/src/adapters/message-port/rpc-handler.test.ts(4 hunks)packages/server/src/adapters/websocket/rpc-handler.test.ts(7 hunks)packages/server/src/adapters/ws/rpc-handler.test.ts(6 hunks)packages/server/src/hibernation/event-iterator.test.ts(4 hunks)packages/server/src/hibernation/event-iterator.ts(1 hunks)packages/shared/src/id.test.ts(1 hunks)packages/shared/src/id.ts(1 hunks)packages/shared/src/iterator.ts(3 hunks)packages/shared/src/queue.test.ts(2 hunks)packages/shared/src/queue.ts(3 hunks)packages/standard-server-peer/src/client.test.ts(17 hunks)packages/standard-server-peer/src/client.ts(2 hunks)packages/standard-server-peer/src/codec.test.ts(56 hunks)packages/standard-server-peer/src/codec.ts(5 hunks)packages/standard-server-peer/src/event-iterator.test.ts(2 hunks)packages/standard-server-peer/src/event-iterator.ts(1 hunks)packages/standard-server-peer/src/server.test.ts(1 hunks)packages/standard-server-peer/src/server.ts(3 hunks)packages/standard-server/src/hibernation.test.ts(1 hunks)packages/standard-server/src/hibernation.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (8)
packages/server/src/adapters/bun-ws/rpc-handler.test.ts (1)
packages/standard-server-peer/src/codec.ts (1)
encodeRequestMessage(95-139)
packages/server/src/adapters/websocket/rpc-handler.test.ts (1)
packages/standard-server-peer/src/codec.ts (1)
encodeRequestMessage(95-139)
packages/server/src/adapters/ws/rpc-handler.test.ts (1)
packages/standard-server-peer/src/codec.ts (1)
encodeRequestMessage(95-139)
packages/durable-event-iterator/src/durable-object/handler.test.ts (1)
packages/durable-event-iterator/src/durable-object/consts.ts (1)
DURABLE_EVENT_ITERATOR_HIBERNATION_ID_KEY(1-1)
packages/durable-event-iterator/src/durable-object/websocket-manager.ts (1)
packages/durable-event-iterator/src/durable-object/consts.ts (1)
DURABLE_EVENT_ITERATOR_HIBERNATION_ID_KEY(1-1)
packages/durable-event-iterator/src/durable-object/websocket-manager.test.ts (1)
packages/durable-event-iterator/src/durable-object/consts.ts (1)
DURABLE_EVENT_ITERATOR_HIBERNATION_ID_KEY(1-1)
packages/standard-server-peer/src/codec.ts (2)
packages/standard-server-peer/src/client.ts (1)
message(102-141)packages/standard-server-peer/src/server.ts (1)
message(50-80)
packages/standard-server-peer/src/server.ts (2)
packages/standard-server-peer/src/types.ts (1)
EncodedMessage(3-3)packages/standard-server/src/types.ts (2)
StandardRequest(13-24)StandardResponse(34-41)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: publish-commit
- GitHub Check: lint
🔇 Additional comments (46)
packages/durable-event-iterator/src/durable-object/object.test.ts (1)
87-87: LGTM! Test data correctly updated for string ID.The change from numeric ID
123to string ID'123'properly aligns with the PR objective to use strings for unique identifiers.packages/standard-server/src/hibernation.test.ts (1)
11-13: LGTM! Test properly updated for string hibernation ID.Both the hibernation callback invocation and expectation are correctly updated to use string ID
'12344'instead of numeric ID, maintaining test consistency.packages/client/src/adapters/message-port/rpc-link.test.ts (1)
37-37: LGTM! Type assertions correctly updated for string IDs.The test expectations are properly updated to verify that decoded request message IDs are now strings instead of numbers, consistent with the PR objectives.
Also applies to: 57-57
packages/standard-server-peer/src/server.test.ts (1)
7-7: LGTM! Test constant correctly updated to string ID.The
REQUEST_IDconstant is properly changed from numeric1953to string'1953', ensuring all tests in this suite use the new string ID format consistently.packages/server/src/hibernation/event-iterator.test.ts (1)
25-25: LGTM! All test cases consistently updated for string hibernation IDs.The
idvariable is properly changed from numeric39483to string'39483'across all test cases in theencodeHibernationRPCEventsuite, ensuring consistent testing of the new string ID format.Also applies to: 37-37, 49-49, 62-62
packages/client/src/adapters/websocket/rpc-link.test.ts (1)
37-37: LGTM! Test assertions correctly updated for string IDs.The test assertions have been properly updated to expect string type IDs, which aligns with the broader change from numeric to string-based identifiers.
Also applies to: 57-57
packages/shared/src/queue.test.ts (2)
5-6: LGTM! Queue ID constants properly defined as strings.The queue ID constants are correctly defined as strings, ensuring consistency with the string-based ID system.
106-134: LGTM! Queue operations consistently use string IDs.All queue operations (open, close, isOpen) have been updated to use string IDs, which maintains consistency with the AsyncIdQueue implementation that now expects string identifiers.
packages/standard-server-peer/src/event-iterator.test.ts (2)
10-11: LGTM! Queue operations updated to use string IDs.All queue operations have been consistently updated to use string IDs, aligning with the AsyncIdQueue implementation changes.
Also applies to: 36-36, 70-71, 96-96
13-34: LGTM! Event iterator payloads correctly use string queue IDs.All queue.push() operations and toEventIterator calls have been updated to use string IDs, ensuring consistency with the string-based ID system.
Also applies to: 73-93
packages/server/src/adapters/bun-ws/rpc-handler.test.ts (2)
31-31: LGTM! Request message encoding updated to use string IDs.All
encodeRequestMessagecalls have been correctly updated to use string IDs ('19' instead of 19), which aligns with the function signature that now expects a string ID parameter as shown in the codec implementation.Also applies to: 39-39, 47-47, 54-54
63-63: LGTM! Test assertions correctly expect string type IDs.The test assertions have been properly updated to expect string type IDs in the decoded response messages, ensuring consistency with the string-based ID system.
Also applies to: 77-77, 123-123
packages/server/src/adapters/message-port/rpc-handler.test.ts (2)
43-43: LGTM! Request message encoding updated to use string IDs.All
encodeRequestMessagecalls have been correctly updated to use string IDs ('19' instead of 19), consistent with the function signature that now expects a string ID parameter.Also applies to: 50-50, 57-57, 64-64
73-73: LGTM! Test assertions correctly expect string type IDs.The test assertions have been properly updated to expect string type IDs in the decoded response messages, maintaining consistency with the string-based identifier system.
Also applies to: 88-88, 134-134
packages/standard-server-peer/src/client.ts (1)
23-23: LGTM! Type changes are consistent and well-implemented.The changes correctly update both the
serverControllersmap key type and theopenmethod parameter type fromnumbertostring, maintaining type consistency throughout the class.Also applies to: 46-46
packages/server/src/adapters/websocket/rpc-handler.test.ts (2)
37-37: Test updates correctly reflect the API change.The
encodeRequestMessagecalls have been properly updated to use string IDs ('19') instead of numeric IDs (19), aligning with the function's updated signature that now expects a string as the first parameter.Also applies to: 50-50, 59-59, 94-94
69-69: Assertions correctly updated for string ID type.The test assertions have been properly updated to expect
stringtype instead ofnumbertype for the decoded message ID, which is consistent with the new string-based ID system.Also applies to: 84-84, 132-132
packages/standard-server/src/hibernation.ts (1)
4-4: Interface correctly updated for string ID consistency.The callback parameter type has been properly changed from
numbertostring, maintaining consistency with the system-wide migration to string-based identifiers.packages/server/src/adapters/crossws/rpc-handler.test.ts (2)
32-32: Test message encoding correctly updated for string IDs.The
encodeRequestMessagecalls have been properly updated to use string IDs ('19') instead of numeric IDs, which aligns with the updated function signature.Also applies to: 41-41, 53-53, 62-62
71-71: Type assertions correctly updated for string ID validation.The test assertions have been appropriately updated to expect
stringtype for the decoded message ID, which is consistent with the new string-based identifier system.Also applies to: 85-85, 131-131
packages/durable-event-iterator/src/durable-object/handler.test.ts (2)
43-43: Hibernation callback calls correctly updated for string IDs.The hibernation callback invocations have been properly updated to pass string IDs (
'123') instead of numeric IDs, which aligns with the updatedexperimental_HibernationEventIteratorCallbackinterface that now expects a string parameter.Also applies to: 80-80
46-46: Test expectations correctly updated for string hibernation IDs.The test assertions have been appropriately updated to expect string values (
'123') instead of numeric values in theserializeInternalAttachmentandsendEventsAftermethod calls, maintaining consistency with the string-based ID system.Also applies to: 51-51, 83-83, 88-88
packages/standard-server-peer/src/event-iterator.ts (1)
9-9: LGTM! Type change aligns with the string ID migration.The parameter type change from
numbertostringis consistent with the systematic refactor to use string-based IDs throughout the codebase.packages/standard-server-peer/src/codec.test.ts (1)
12-823: LGTM! Comprehensive test updates for string ID migration.All test cases have been consistently updated to:
- Use string IDs (
'198') instead of numeric IDs (198) in encode function calls- Expect string IDs in decode assertions
The changes properly validate the new string-based ID implementation and align with the updated function signatures.
packages/server/src/hibernation/event-iterator.ts (1)
27-27: LGTM! Hibernation event ID type updated consistently.The parameter type change from
numbertostringaligns with the systematic migration to string-based IDs throughout the codebase.packages/server/src/adapters/ws/rpc-handler.test.ts (1)
37-129: LGTM! RPC handler tests updated for string ID implementation.The test updates are consistent and comprehensive:
- All
encodeRequestMessagecalls now use string IDs ('19') instead of numeric IDs (19)- Type assertions properly expect string IDs in decoded messages
- Changes align with the updated function signatures requiring string IDs
packages/standard-server-peer/src/client.test.ts (1)
41-520: LGTM! Comprehensive client peer test updates for string ID migration.All test cases have been systematically updated to use string-based IDs:
- Message encoding/decoding calls now use string IDs (
'0','1') instead of numeric IDs (0,1)- Test assertions properly validate string ID types
- Coverage includes all scenarios: simple requests, abort signals, iterators, file uploads, and form data
- Changes maintain existing test logic while adapting to the new ID type
The updates are consistent and comprehensive across all test cases.
packages/durable-event-iterator/src/durable-object/websocket-manager.test.ts (2)
28-28: LGTM: Consistent hibernation ID type updatesThe hibernation ID values are correctly updated from numbers to strings, maintaining consistency with the broader type refactor.
Also applies to: 44-44, 61-61, 92-92
110-111: LGTM: Function call assertions updated correctlyThe test assertions for
encodeHibernationRPCEventSpycalls are properly updated to expect string hibernation IDs instead of numbers.Also applies to: 141-141, 144-145
packages/shared/src/iterator.ts (1)
114-116: LGTM: Correct ID type conversion for queue operationsThe numeric loop indices are properly converted to strings using
toString()before queue operations, maintaining compatibility with the updatedAsyncIdQueuethat now expects string IDs.Also applies to: 130-130, 136-136, 148-148, 151-151
packages/shared/src/id.test.ts (2)
3-17: LGTM: Test structure and assertions updated correctlyThe test properly verifies the new string-based ID generation, including the base-32 progression and the transition to 'v0' after reaching 'v'.
19-31: LGTM: Large range uniqueness testThe test effectively validates uniqueness over 100,000 generated IDs, ensuring the new string-based approach maintains uniqueness at scale.
packages/durable-event-iterator/src/durable-object/websocket-manager.ts (2)
17-17: LGTM: Hibernation ID type updated in interfaceThe hibernation ID type is correctly changed from
numbertostringin the internal attachment interface.
70-70: LGTM: Method parameter type updated consistentlyThe
hibernationIdparameter type insendEventsAftermethod is properly updated tostring, maintaining consistency with the interface changes.packages/shared/src/id.ts (1)
2-8: LGTM: String-based ID generation implementedThe refactor from numeric to string-based ID generation is correctly implemented, addressing the PR objective of ensuring unique IDs without numeric overflow concerns.
packages/shared/src/queue.ts (1)
1-104: LGTM! Well-executed type refactor from number to string IDs.The changes consistently update all ID-related types and method signatures throughout the
AsyncIdQueueclass. The core queue functionality is preserved since IDs are only used as Map/Set keys and for error messages, where strings work identically to numbers.packages/standard-server-peer/src/server.ts (4)
21-21: LGTM! Client controller storage updated to use string keys.The change from
Map<number, AbortController>toMap<string, AbortController>is consistent with the ID type refactor and maintains identical functionality.
43-48: LGTM! Method signature correctly updated to string ID.The
openmethod parameter change fromnumbertostringis consistent with the refactor and properly handles the new ID type.
50-80: LGTM! Message processing updated to handle string IDs.The
messagemethod correctly handles the string ID returned bydecodeRequestMessageand passes it consistently throughout the request processing pipeline.
82-115: LGTM! Response method updated to use string ID parameter.The
responsemethod parameter change and hibernation callback usage (line 94) are consistent with the string ID refactor.packages/standard-server-peer/src/codec.ts (6)
34-45: LGTM! Base message format updated to use string IDs.The change from
i: numbertoi: stringin theBaseMessageFormatinterface is the foundation for the ID type refactor. The comment clarifying "Id that unique in client-side" remains accurate.
88-93: LGTM! Decoded message types updated for string IDs.The
DecodedMessageUniontype correctly updates theidfield fromnumbertostring, maintaining type safety for the decoded message tuples.
95-139: LGTM! Request encoding function updated to string ID parameter.The
encodeRequestMessagefunction parameter change fromid: numbertoid: stringis consistent with the refactor. The encoding logic remains unchanged since JSON serialization handles strings natively.
141-182: LGTM! Request decoding function updated to return string ID.The
decodeRequestMessagefunction correctly declaresid: stringand maintains the same decoding logic. The returned tuple now properly uses the string ID type.
184-225: LGTM! Response encoding function updated to string ID parameter.The
encodeResponseMessagefunction parameter change is consistent with the refactor and preserves all encoding functionality.
227-269: LGTM! Response decoding function updated to return string ID.The
decodeResponseMessagefunction correctly handles the string ID type while maintaining identical decoding logic.
b5316a1 to
844081f
Compare
dosubot
Bot
added
size:L
Number range has a limit. We reset when it’s reached, but it’s still more ideal if we can guarantee the ID is always unique by use string.
Summary by CodeRabbit
New Features
Refactor
Tests
Documentation
These changes improve consistency and compatibility across the system when handling identifiers. No user-facing functionality or workflows are altered.