Fix batch sync protobuf request body#363
Conversation
Reviewer's GuideAdds an explicit Axios transformRequest for collab batch sync protobuf calls to ensure the original Uint8Array view is sent instead of its backing ArrayBuffer, and introduces a regression test that verifies the request body and config preserve the encoded protobuf view semantics. Sequence diagram for batch sync protobuf request with custom Axios transformRequestsequenceDiagram
participant Client
participant ProtobufJS
participant Axios
participant Server
Client->>ProtobufJS: encode(collabFullSyncBatchRequest)
ProtobufJS-->>Client: Uint8Array(encodedView)
Client->>Axios: collabFullSyncBatch(Uint8Array)
Axios->>Axios: transformRequest(Uint8Array data) => data
Axios-->>Server: HTTP POST body(Uint8Array view)
Server-->>Axios: 200/429/503 (arraybuffer)
Axios-->>Client: ArrayBuffer response
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The test assumption
expect(requestBody.byteLength).toBeLessThan(requestBody.buffer.byteLength);is a bit brittle and ties correctness to the current protobufjs pooling behavior; consider instead asserting that Axios receives a view (ArrayBuffer.isView) and that the sentbyteLengthmatches the encoded message length you expect. - The
transformRequest: [(data: Uint8Array) => data]signature is very narrow compared to Axios’ actualtransformRequesttype; widening the parameter type (e.g., toanyor a union) will make this more robust against future Axios type changes while still returning theUint8Arrayunmodified.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The test assumption `expect(requestBody.byteLength).toBeLessThan(requestBody.buffer.byteLength);` is a bit brittle and ties correctness to the current protobufjs pooling behavior; consider instead asserting that Axios receives a view (`ArrayBuffer.isView`) and that the sent `byteLength` matches the encoded message length you expect.
- The `transformRequest: [(data: Uint8Array) => data]` signature is very narrow compared to Axios’ actual `transformRequest` type; widening the parameter type (e.g., to `any` or a union) will make this more robust against future Axios type changes while still returning the `Uint8Array` unmodified.
## Individual Comments
### Comment 1
<location path="src/application/services/js-services/http/__tests__/collab-api.test.ts" line_range="51" />
<code_context>
+ const [, requestBody, config] = post.mock.calls[0];
+
+ expect(ArrayBuffer.isView(requestBody)).toBe(true);
+ expect(requestBody.byteLength).toBeLessThan(requestBody.buffer.byteLength);
+ expect(config.transformRequest[0](requestBody)).toBe(requestBody);
+ });
</code_context>
<issue_to_address>
**issue (testing):** This assertion makes the test brittle because it relies on the encoder always returning a view smaller than its backing buffer.
Depending on `requestBody.byteLength < requestBody.buffer.byteLength` makes this test fragile: a future protobuf encoder change could return a tightly sized `Uint8Array`, breaking the test while the behavior is still correct. The regression is specifically about Axios using `.buffer` (including zero padding), not about the current size relationship. To make this robust, either:
- Create a larger `ArrayBuffer` in the test and a `Uint8Array` view over it, and use that as the request body so the condition is under your control, or
- Remove this assertion and instead assert behavior that directly captures the regression (e.g., comparing what Axios’ default transform would send vs. what your override sends).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const [, requestBody, config] = post.mock.calls[0]; | ||
|
|
||
| expect(ArrayBuffer.isView(requestBody)).toBe(true); | ||
| expect(requestBody.byteLength).toBeLessThan(requestBody.buffer.byteLength); |
There was a problem hiding this comment.
issue (testing): This assertion makes the test brittle because it relies on the encoder always returning a view smaller than its backing buffer.
Depending on requestBody.byteLength < requestBody.buffer.byteLength makes this test fragile: a future protobuf encoder change could return a tightly sized Uint8Array, breaking the test while the behavior is still correct. The regression is specifically about Axios using .buffer (including zero padding), not about the current size relationship. To make this robust, either:
- Create a larger
ArrayBufferin the test and aUint8Arrayview over it, and use that as the request body so the condition is under your control, or - Remove this assertion and instead assert behavior that directly captures the regression (e.g., comparing what Axios’ default transform would send vs. what your override sends).
Summary
Why
The web batch-sync request was encoded by protobufjs as a small Uint8Array view backed by an 8192-byte buffer. Axios default transform converted the view to .buffer, causing the server to receive an 8192-byte body with zero padding and fail protobuf decode with invalid tag value: 0.
Tests
Summary by Sourcery
Ensure collab full-sync batch requests send the correct protobuf-encoded payload without including pooled buffer padding.
Bug Fixes:
Tests: