Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
mohitpubnub
added
status: in progress
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Configuration .coderabbit.yaml |
New repository configuration enabling automated code review with language, draft PR handling, GitHub checks integration, and path-specific review guidelines for core SDK, REST endpoints, types, event-engine, entity listeners, transport, and platform implementations. |
Transport Method Support src/core/types/transport-request.ts, src/core/components/request.ts |
Added PUT enum member to TransportMethod and extended AbstractRequest.request() to set request body/formData for PUT in addition to existing POST and PATCH support. |
Error Handling src/errors/pubnub-api-error.ts |
Enhanced createFromServiceResponse() to parse DataSync-style structured error responses with errors array, storing full response in errorData and concatenating error codes and messages. Added new public method toFormattedMessage(operation: RequestOperation) to format error messages with operation context. |
Estimated code review effort
🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Title check | ❓ Inconclusive | The title 'feat/dataSync' is partially related to the changeset but is overly vague and uses a branch-naming convention rather than conveying meaningful details about the main changes. | Revise the title to clearly describe the primary changes (e.g., 'Add DataSync API support with PUT request handling and error parsing' or similar) to help reviewers understand the scope at a glance. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
CLEN-3287
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/core/components/request.ts (1)
173-180: Consider updating the JSDoc to include PUT.The JSDoc comment mentions
POSTorPATCHbut the body is now also sent withPUTrequests.Proposed fix
/** * Target REST API Request body payload getter. * - * `@returns` Buffer of stringified data which should be sent with `POST` or `PATCH` request. + * `@returns` Buffer of stringified data which should be sent with `POST`, `PATCH`, or `PUT` request. */ protected get body(): ArrayBuffer | PubNubFileInterface | string | undefined {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/components/request.ts` around lines 173 - 180, Update the JSDoc for the protected getter body in the Request class (the get body(): ArrayBuffer | PubNubFileInterface | string | undefined) to state that the returned Buffer/string is used for POST, PATCH, and PUT requests; modify the comment text to explicitly include PUT alongside POST and PATCH so the doc accurately reflects where the body is sent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.coderabbit.yaml:
- Around line 5-7: The reviews block contains a misplaced property: remove the
top-level drafts entry under reviews (the stray "drafts: true" currently
directly beneath the reviews key) and rely on the existing drafts setting under
reviews.auto_review (auto_review.drafts) instead; specifically delete the
reviews-level "drafts" property so only reviews.auto_review.drafts remains.
In `@src/core/types/transport-request.ts`:
- Around line 22-25: The signature middleware currently only includes request
bodies for POST and PATCH when computing the signature, causing PUT bodies to be
omitted; update the method-check in the signature calculation (the conditional
that compares method to TransportMethod.POST / TransportMethod.PATCH) to also
include TransportMethod.PUT so that requests where method ===
TransportMethod.PUT include the body in the signature input.
In `@src/errors/pubnub-api-error.ts`:
- Around line 146-169: When building the message from the DataSync-style errors
(the block that reads errorResponse.errors and sets message), skip error items
that have neither errorCode nor message so you don't produce empty tokens;
specifically, update the logic around errors (the const errors = ... and the
subsequent message = errors.map(...).join(...)) to either filter out mapped
parts that are empty or filter out error objects where both e.errorCode and
e.message are falsy before mapping, and then join the remaining non-empty
strings into message while leaving errorData assignment unchanged.
---
Nitpick comments:
In `@src/core/components/request.ts`:
- Around line 173-180: Update the JSDoc for the protected getter body in the
Request class (the get body(): ArrayBuffer | PubNubFileInterface | string |
undefined) to state that the returned Buffer/string is used for POST, PATCH, and
PUT requests; modify the comment text to explicitly include PUT alongside POST
and PATCH so the doc accurately reflects where the body is sent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3f679b7e-746a-4687-9eb1-d5419055588d
📒 Files selected for processing (4)
.coderabbit.yamlsrc/core/components/request.tssrc/core/types/transport-request.tssrc/errors/pubnub-api-error.ts
| reviews: | ||
| # Review draft PRs so we get early feedback | ||
| drafts: true |
There was a problem hiding this comment.
Misplaced drafts property at the reviews level.
According to the CodeRabbit configuration schema, the drafts property belongs under reviews.auto_review.drafts, not directly under reviews. The schema specifies "additionalProperties": false for the reviews object, so this property will be ignored.
Since you already have drafts: true correctly placed under auto_review (line 21), you can safely remove line 7.
Proposed fix
reviews:
- # Review draft PRs so we get early feedback
- drafts: true
-
# Enable high-level summary of changes
high_level_summary: true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| reviews: | |
| # Review draft PRs so we get early feedback | |
| drafts: true | |
| reviews: | |
| # Enable high-level summary of changes | |
| high_level_summary: true |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.coderabbit.yaml around lines 5 - 7, The reviews block contains a misplaced
property: remove the top-level drafts entry under reviews (the stray "drafts:
true" currently directly beneath the reviews key) and rely on the existing
drafts setting under reviews.auto_review (auto_review.drafts) instead;
specifically delete the reviews-level "drafts" property so only
reviews.auto_review.drafts remains.
| /** | ||
| * Request will be sent using `PUT` method. | ||
| */ | ||
| PUT = 'PUT', |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check signature method in middleware.ts for PUT handling
rg -n -A 15 'signature\(req: TransportRequest\)' src/transport/Repository: pubnub/javascript
Length of output: 1212
Signature calculation must include PUT request bodies to prevent authentication failures.
The PUT enum addition is correct, but the signature middleware at src/transport/middleware.ts:69-80 only includes request body in signature calculation for POST and PATCH methods. PUT requests with bodies will have their body excluded from the signature input, causing authentication validation failures.
Update the condition to include TransportMethod.PUT:
if (method === TransportMethod.POST || method === TransportMethod.PATCH || method === TransportMethod.PUT) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/types/transport-request.ts` around lines 22 - 25, The signature
middleware currently only includes request bodies for POST and PATCH when
computing the signature, causing PUT bodies to be omitted; update the
method-check in the signature calculation (the conditional that compares method
to TransportMethod.POST / TransportMethod.PATCH) to also include
TransportMethod.PUT so that requests where method === TransportMethod.PUT
include the body in the signature input.
| } else if ( | ||
| 'errors' in errorResponse && | ||
| Array.isArray(errorResponse.errors) && | ||
| errorResponse.errors.length > 0 | ||
| ) { | ||
| // Handle DataSync-style structured error responses: | ||
| // { errors: [{ errorCode: "SYN-0008", message: "...", path: "/id" }] } | ||
| errorData = errorResponse; | ||
|
|
||
| const errors = errorResponse.errors as Array<{ | ||
| errorCode?: string; | ||
| message?: string; | ||
| path?: string; | ||
| }>; | ||
|
|
||
| message = errors | ||
| .map((e) => { | ||
| const parts: string[] = []; | ||
| if (e.errorCode) parts.push(e.errorCode); | ||
| if (e.message) parts.push(e.message); | ||
| return parts.join(': '); | ||
| }) | ||
| .join('; '); | ||
| } else errorData = errorResponse; |
There was a problem hiding this comment.
Handle edge case where error items lack both errorCode and message.
If all items in the errors array have only path (no errorCode or message), the resulting message would be empty strings joined by '; ', producing unhelpful output like "; " or an empty string.
Consider filtering out empty results:
Proposed fix
message = errors
.map((e) => {
const parts: string[] = [];
if (e.errorCode) parts.push(e.errorCode);
if (e.message) parts.push(e.message);
return parts.join(': ');
})
+ .filter((s) => s.length > 0)
.join('; ');
+
+ if (!message) message = 'Unknown error';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/errors/pubnub-api-error.ts` around lines 146 - 169, When building the
message from the DataSync-style errors (the block that reads
errorResponse.errors and sets message), skip error items that have neither
errorCode nor message so you don't produce empty tokens; specifically, update
the logic around errors (the const errors = ... and the subsequent message =
errors.map(...).join(...)) to either filter out mapped parts that are empty or
filter out error objects where both e.errorCode and e.message are falsy before
mapping, and then join the remaining non-empty strings into message while
leaving errorData assignment unchanged.
2 participants
initial code readiness for dataSync apis
Summary by CodeRabbit
New Features
Chores