feat/dataSync#494
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
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Warning
|
| Cohort / File(s) | Summary |
|---|---|
DataSync API surface & types src/core/types/api/data-sync.ts, lib/types/index.d.ts, src/core/endpoints/data-sync/... |
Introduces full DataSync request/response types, JSON Patch helpers, many DataSync endpoint implementations (entity classes, entities, relationships), and public typings/SDK methods for the new API. |
Core client integration src/core/pubnub-common.ts, lib/core/pubnub-common.js, src/core/constants/operations.ts, lib/core/constants/operations.js |
Instantiates and exposes dataSync on PubNubCore; adds DataSync RequestOperation constants. Adjusts sendRequest error handling to use formatted messages from API errors. |
Transport method changes src/core/types/transport-request.ts, lib/core/types/transport-request.js, src/core/components/request.ts, lib/core/components/request.js |
Adds TransportMethod.PUT and updates request construction so PUT includes body/formData population analogous to POST/PATCH. |
API error parsing & formatting src/errors/pubnub-api-error.ts, lib/errors/pubnub-api-error.js |
Parses DataSync-style errors arrays into errorData, builds composite message, and adds toFormattedMessage(operation) for operation-aware user-facing messages. |
New endpoint implementation examples src/core/endpoints/data-sync/entity/create.ts |
Adds Create Entity request class with validation, headers (Idempotency-Key support), path and JSON body shaping. |
Build/test/config updates .mocharc.json, .coderabbit.yaml |
Mocha config narrowed to a single integration test and timeout increased; .coderabbit.yaml contains unresolved merge conflict markers in reviews/path_filters. |
Lib artifacts lib/... (various generated JS files mirrored from src) |
Compiled/packaged equivalents of the TypeScript changes: DataSync constants, transport PUT, error formatting, client integration. |
Sequence Diagram(s)
sequenceDiagram
participant Client
participant PubNubCore as PubNubCore
participant DataSync as PubNubDataSync
participant RequestBuilder as AbstractRequest
participant Transport
participant Service as DataSync Service
Client->>PubNubCore: dataSync.createEntity(params)
PubNubCore->>DataSync: forward call
DataSync->>RequestBuilder: build request (operation, method, headers, body)
RequestBuilder->>Transport: send (method PUT/POST/PATCH with body/formData)
Transport->>Service: HTTP request
Service-->>Transport: HTTP response (200 or error with errors[])
Transport-->>RequestBuilder: response
RequestBuilder->>PubNubCore: parse response / create PubNubAPIError
PubNubCore->>Client: return result or throw formatted error (apiError.toFormattedMessage)
Estimated code review effort
🎯 5 (Critical) | ⏱️ ~120 minutes
"A rabbit hopped in with a patch and a PUT,
nibbling errors and stitching types in a rut.
I tunneled new endpoints and typed every field,
wrapped messages nicely so users won't yield.
🐇📦✨"
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Title check | ❓ Inconclusive | The title 'feat/dataSync' is vague and generic, using a non-descriptive format (feature branch prefix) without conveying meaningful details about the changeset's primary purpose or scope. | Revise the title to be more descriptive and specific, such as 'Add DataSync API support with entity class and relationship management' or similar, clearly summarizing the main feature being introduced. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 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
| /** | ||
| * 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.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/errors/pubnub-api-error.ts (1)
146-169:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStructured error message builder still allows empty output.
If
errors[]items lack botherrorCodeandmessage, the current join can yield empty/low-value text. Filter empty fragments and keep a fallback message.🔧 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((entry) => entry.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, The structured-error block that builds message from errorResponse.errors may produce empty/meaningless output when items lack both errorCode and message; update the logic in the else-if that handles errorResponse.errors (the errors variable and message assignment) to filter out entries that produce no parts, prefer using path if present when errorCode/message are missing, and after mapping/joining ensure you set a sensible fallback (e.g., "Unknown error" or include JSON.stringify of the error item) so message is never empty while preserving existing formatting of "errorCode: message" and joining entries with "; ".src/core/types/transport-request.ts (1)
22-25:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
PUTis added to transport methods, but request signing still excludesPUTbodies.With this enum expansion,
PUTrequests can now carry bodies, but signature generation (insrc/transport/middleware.ts, Line 69) still only appends payload forPOST/PATCH. SignedPUTrequests with bodies can fail auth validation.🔧 Proposed fix
--- a/src/transport/middleware.ts +++ b/src/transport/middleware.ts @@ - if (method === TransportMethod.POST || method === TransportMethod.PATCH) { + if ( + method === TransportMethod.POST || + method === TransportMethod.PATCH || + method === TransportMethod.PUT + ) {As per coding guidelines,
src/core/**: "Backward compatibility of public API changes".🤖 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 TransportMethod enum now includes PUT but the request-signing logic in the middleware (signature generation in the middleware function in src/transport/middleware.ts) only appends the request payload for POST and PATCH; update that logic to also treat PUT as a body-bearing method so the payload is included in the signature when method === 'PUT' (i.e., extend the conditional that currently checks for POST/PATCH to include PUT), ensure any related helper that canonicalizes the body handles PUT the same way as POST/PATCH, and add/update tests to cover signing a PUT with a body.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/core/types/transport-request.ts`:
- Around line 22-25: The TransportMethod enum now includes PUT but the
request-signing logic in the middleware (signature generation in the middleware
function in src/transport/middleware.ts) only appends the request payload for
POST and PATCH; update that logic to also treat PUT as a body-bearing method so
the payload is included in the signature when method === 'PUT' (i.e., extend the
conditional that currently checks for POST/PATCH to include PUT), ensure any
related helper that canonicalizes the body handles PUT the same way as
POST/PATCH, and add/update tests to cover signing a PUT with a body.
In `@src/errors/pubnub-api-error.ts`:
- Around line 146-169: The structured-error block that builds message from
errorResponse.errors may produce empty/meaningless output when items lack both
errorCode and message; update the logic in the else-if that handles
errorResponse.errors (the errors variable and message assignment) to filter out
entries that produce no parts, prefer using path if present when
errorCode/message are missing, and after mapping/joining ensure you set a
sensible fallback (e.g., "Unknown error" or include JSON.stringify of the error
item) so message is never empty while preserving existing formatting of
"errorCode: message" and joining entries with "; ".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 31b914fd-4c84-4161-bdfa-b7dbf18d1fd6
📒 Files selected for processing (3)
src/core/components/request.tssrc/core/types/transport-request.tssrc/errors/pubnub-api-error.ts
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
src/errors/pubnub-api-error.ts (1)
266-282:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve specific error text for non-DataSync failures.
On Line 281, this returns the generic fallback for every non-
errorspayload. SincesendRequestnow always usestoFormattedMessage(operation), timeout/network-specific messages get overwritten across the SDK.Proposed fix
public toFormattedMessage(operation: RequestOperation): string { const fallback = 'REST API request processing error, check status for details'; @@ if ( this.errorData && typeof this.errorData === 'object' && !('name' in this.errorData && 'message' in this.errorData && 'stack' in this.errorData) && 'errors' in this.errorData && Array.isArray((this.errorData as Record<string, unknown>).errors) ) { return `${operation}: ${this.message}`; } - return fallback; + return this.message || fallback; }🤖 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 266 - 282, The toFormattedMessage method currently returns a generic fallback for any errorData that doesn't contain an "errors" array, which overwrites useful timeout/network messages; update toFormattedMessage to prefer returning `${operation}: ${this.message}` whenever this.message is present/non-empty (keep the existing special-case that returns this.message when errorData.errors is an array), and only return the generic fallback when this.message is missing/empty—make this change in the toFormattedMessage method in the PubNubApiError class so non-DataSync failures preserve their specific text.
🤖 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 6-12: The .coderabbit.yaml contains unresolved Git conflict
markers (<<<<<<<, =======, >>>>>>>) around the profile/drafts blocks; remove the
conflict markers and merge the intended keys (e.g., keep profile: "assertive"
and drafts: true or whichever variant is correct), ensure there are no
duplicate/conflicting entries in both locations (the other conflict at lines
104-109), and validate the YAML (ensure proper colon/value syntax and
indentation) so the file parses cleanly.
In @.mocharc.json:
- Line 4: The .mocharc.json "spec" value was narrowed to a single file which
skips the rest of CI tests; update the "spec" key in .mocharc.json to the
repository's test suite glob (restore the original pattern used for running all
tests) instead of the single-file path so the full integration/unit test suite
runs in CI.
- Line 10: The mocha config currently sets the "timeout" value to 5000000 (≈83
minutes); change the "timeout" property in .mocharc.json to a much smaller, sane
value (for example 20000 for 20s or 5000 for 5s) to avoid hiding hangs and speed
up feedback loops, then run the test suite (or CI) to ensure the new timeout is
sufficient and adjust as needed.
In `@lib/types/index.d.ts`:
- Around line 9573-9577: The JSDoc for the "set" field incorrectly states the
SDK converts values to JSON Patch "replace" operations while the conversion
utility actually emits "add"; update the documentation to match the
implementation (or alternatively change the converter to emit "replace" if
intended). Specifically, edit the JSDoc on the "set" parameter (the comment
block that currently says "The SDK converts these to JSON Patch 'replace'
operations") to say it converts to JSON Patch "add" operations, and make the
same change for the duplicated doc blocks at the other occurrences (the comment
blocks around the same "set" description at the other noted locations). Ensure
the wording is consistent across all three spots so API users are not misled.
- Around line 772-775: The project is missing the runtime implementation for the
PubNub DataSync API referenced by the dataSync getter; add a new implementation
file (src/core/pubnub-data-sync.ts) that exports the PubNubDataSync class (or
default) matching the PubNub.PubNubDataSync type used by the getter in
src/core/pubnub-common.ts, implement the required methods used by callers,
update the import in src/core/pubnub-common.ts to import from
'./pubnub-data-sync', and rebuild so lib/core/pubnub-data-sync.js and types are
generated.
In `@src/core/endpoints/data-sync/entity/create.ts`:
- Around line 7-11: The create endpoint currently only forwards an incoming
Idempotency-Key when present; update the POST request building logic in this
file so the 'Idempotency-Key' header is always set for create operations: if an
incoming key (from the request KeySet) exists use it, otherwise generate a new
unique id (UUID/v4) and add it to the outgoing headers before calling
AbstractRequest/TransportMethod for the RequestOperation create; ensure the
header key is exactly 'Idempotency-Key' and include the generated key so retries
are idempotent.
In `@src/core/pubnub-common.ts`:
- Line 119: The import of PubNubDataSync from './pubnub-data-sync' in
pubnub-common.ts fails TS2307; fix by either adding the missing module/file or
correcting the import path/casing to the actual module name, ensuring the
exported symbol PubNubDataSync is present and its types are exported (or add a
declaration file) so the TypeScript compiler can resolve the module.
In `@src/core/types/api/data-sync.ts`:
- Around line 157-159: The toJsonPointer function currently joins dotPath tokens
without escaping, which breaks RFC 6901 for field names containing '~' or '/';
update toJsonPointer to split dotPath on '.' into tokens, for each token replace
'~' with '~0' and '/' with '~1' (in that order) and then join with '/' prefixing
the result with '/' (handle the empty dotPath case per RFC by returning an empty
string if needed). Ensure you perform the escapes on each token before joining
so names like "user/id" and "tilde~field" produce correct JSON Pointers.
- Around line 169-188: The toJsonPatchOperations function currently emits op:
'add' for set entries and silently returns an empty array when both set/remove
are absent; update toJsonPatchOperations so that for each entry in set it emits
op: 'replace' (not 'add') and validate inputs at the start: if both set is falsy
or an empty object and remove is falsy or an empty array, throw a descriptive
error (e.g., "at least one of set or remove must be provided"); keep using
toJsonPointer for path conversion and preserve existing behavior for remove
entries.
---
Duplicate comments:
In `@src/errors/pubnub-api-error.ts`:
- Around line 266-282: The toFormattedMessage method currently returns a generic
fallback for any errorData that doesn't contain an "errors" array, which
overwrites useful timeout/network messages; update toFormattedMessage to prefer
returning `${operation}: ${this.message}` whenever this.message is
present/non-empty (keep the existing special-case that returns this.message when
errorData.errors is an array), and only return the generic fallback when
this.message is missing/empty—make this change in the toFormattedMessage method
in the PubNubApiError class so non-DataSync failures preserve their specific
text.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5957f30-9e3e-413a-8cfa-6c69cd91dbc6
⛔ Files ignored due to path filters (4)
dist/web/pubnub.jsis excluded by!**/dist/**dist/web/pubnub.min.jsis excluded by!**/dist/**,!**/*.min.jsdist/web/pubnub.worker.jsis excluded by!**/dist/**dist/web/pubnub.worker.min.jsis excluded by!**/dist/**,!**/*.min.js
📒 Files selected for processing (15)
.coderabbit.yaml.mocharc.jsonlib/core/components/request.jslib/core/constants/operations.jslib/core/pubnub-common.jslib/core/types/transport-request.jslib/errors/pubnub-api-error.jslib/types/index.d.tssrc/core/components/request.tssrc/core/constants/operations.tssrc/core/endpoints/data-sync/entity/create.tssrc/core/pubnub-common.tssrc/core/types/api/data-sync.tssrc/core/types/transport-request.tssrc/errors/pubnub-api-error.ts
| <<<<<<< Updated upstream | ||
| ======= | ||
| profile: "assertive" #more feedback | ||
| # Review draft PRs so we get early feedback | ||
| drafts: true | ||
|
|
||
| >>>>>>> Stashed changes |
There was a problem hiding this comment.
Resolve merge-conflict markers before merge.
Lines 6-12 and Line 104-Line 109 contain unresolved Git conflict markers, and the YAML cannot be parsed (could not find expected ':').
Proposed fix (resolve conflicts and remove markers)
reviews:
-<<<<<<< Updated upstream
-=======
profile: "assertive" `#more` feedback
- # Review draft PRs so we get early feedback
- drafts: true
-
->>>>>>> Stashed changes
# Enable high-level summary of changes
high_level_summary: false
@@
-<<<<<<< Updated upstream
-=======
# Unrelated working directories (not part of the SDK source)
- "!docs/**"
-
->>>>>>> Stashed changesAlso applies to: 104-109
🧰 Tools
🪛 YAMLlint (1.38.0)
[error] 7-7: syntax error: could not find expected ':'
(syntax)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.coderabbit.yaml around lines 6 - 12, The .coderabbit.yaml contains
unresolved Git conflict markers (<<<<<<<, =======, >>>>>>>) around the
profile/drafts blocks; remove the conflict markers and merge the intended keys
(e.g., keep profile: "assertive" and drafts: true or whichever variant is
correct), ensure there are no duplicate/conflicting entries in both locations
(the other conflict at lines 104-109), and validate the YAML (ensure proper
colon/value syntax and indentation) so the file parses cleanly.
| "require": "tsx", | ||
| "file": ["test/setup-why.ts"], | ||
| "spec": "test/**/*.test.ts", | ||
| "spec": "test/integration/endpoints/dataSync.test.ts", |
There was a problem hiding this comment.
Do not narrow global spec to a single test file.
Line 4 now limits CI to one integration test and effectively skips the rest of the suite.
Proposed fix
- "spec": "test/integration/endpoints/dataSync.test.ts",
+ "spec": "test/**/*.test.ts",📝 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.
| "spec": "test/integration/endpoints/dataSync.test.ts", | |
| "spec": "test/**/*.test.ts", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.mocharc.json at line 4, The .mocharc.json "spec" value was narrowed to a
single file which skips the rest of CI tests; update the "spec" key in
.mocharc.json to the repository's test suite glob (restore the original pattern
used for running all tests) instead of the single-file path so the full
integration/unit test suite runs in CI.
| "test/integration/shared-worker/*.{js,ts}" | ||
| ], | ||
| "timeout": 5000, | ||
| "timeout": 5000000, |
There was a problem hiding this comment.
Reduce the test timeout to a sane bound.
Line 10 sets ~83 minutes per test, which can hide hangs and slow feedback loops significantly.
Proposed fix
- "timeout": 5000000,
+ "timeout": 30000,📝 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.
| "timeout": 5000000, | |
| "timeout": 30000, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.mocharc.json at line 10, The mocha config currently sets the "timeout"
value to 5000000 (≈83 minutes); change the "timeout" property in .mocharc.json
to a much smaller, sane value (for example 20000 for 20s or 5000 for 5s) to
avoid hiding hangs and speed up feedback loops, then run the test suite (or CI)
to ensure the new timeout is sufficient and adjust as needed.
| /** | ||
| * PubNub DataSync API group. | ||
| */ | ||
| get dataSync(): PubNub.PubNubDataSync; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check for DataSync implementation files =="
fd -i 'pubnub-data-sync\.(ts|js)$' src lib
echo
echo "== Check DataSync import/wiring sites =="
rg -n -C3 'pubnub-data-sync|dataSync' src/core/pubnub-common.ts lib/core/pubnub-common.js
echo
echo "== Check for PubNubDataSync class/export definitions =="
rg -n -C3 'class PubNubDataSync|export default .*PubNubDataSync|module.exports.*PubNubDataSync' src libRepository: pubnub/javascript
Length of output: 3989
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== List all files in src/core/ directory =="
ls -la src/core/ | grep -i pubnub-data-sync || echo "No matches in ls"
echo
echo "== Direct check for the specific file =="
test -f src/core/pubnub-data-sync.ts && echo "✓ src/core/pubnub-data-sync.ts EXISTS" || echo "✗ src/core/pubnub-data-sync.ts MISSING"
test -f lib/core/pubnub-data-sync.js && echo "✓ lib/core/pubnub-data-sync.js EXISTS" || echo "✗ lib/core/pubnub-data-sync.js MISSING"
echo
echo "== List all .ts files in src/core matching 'data' pattern =="
find src/core -name '*data*.ts' 2>/dev/null | head -20
echo
echo "== List all .js files in lib/core matching 'data' pattern =="
find lib/core -name '*data*.js' 2>/dev/null | head -20
echo
echo "== Git ls-files to check tracked files =="
git ls-files src/core | grep -i pubnub-data-sync || echo "No matches in git ls-files"Repository: pubnub/javascript
Length of output: 468
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check lib/core/ directory contents =="
ls -la lib/core/ 2>&1 | head -30
echo
echo "== Count files in lib/core/ =="
find lib/core -type f 2>/dev/null | wc -l
find lib/core -type f 2>/dev/null | head -20
echo
echo "== Check if lib directory structure exists =="
ls -la lib/ 2>&1 | head -20
echo
echo "== Final verification: Check exact import statement syntax in source =="
rg -A2 -B2 "import PubNubDataSync" src/core/pubnub-common.tsRepository: pubnub/javascript
Length of output: 2743
Missing critical implementation: src/core/pubnub-data-sync.ts does not exist
The dataSync getter is imported and wired in src/core/pubnub-common.ts (line 119) and compiled into lib/core/pubnub-common.js (line 129), but the implementation file itself is missing:
src/core/pubnub-data-sync.ts— does not existlib/core/pubnub-data-sync.js— does not exist
This will cause runtime failure when code attempts to instantiate PubNubDataSync. The type declarations in lib/types/index.d.ts define the public API contract, but there is no backing implementation. Create the implementation file and rebuild.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/types/index.d.ts` around lines 772 - 775, The project is missing the
runtime implementation for the PubNub DataSync API referenced by the dataSync
getter; add a new implementation file (src/core/pubnub-data-sync.ts) that
exports the PubNubDataSync class (or default) matching the PubNub.PubNubDataSync
type used by the getter in src/core/pubnub-common.ts, implement the required
methods used by callers, update the import in src/core/pubnub-common.ts to
import from './pubnub-data-sync', and rebuild so lib/core/pubnub-data-sync.js
and types are generated.
| * Fields to add or replace, using dot-notation keys. | ||
| * | ||
| * Each key is a dot-delimited path to the target field. | ||
| * The SDK converts these to JSON Patch "replace" operations. | ||
| * |
There was a problem hiding this comment.
Patch JSDoc says “replace”, but implementation uses JSON Patch add
The docs currently state set maps to "replace" operations, while the conversion utility emits "add" for set values. This should be aligned to avoid misleading API users.
Suggested doc fix
- * The SDK converts these to JSON Patch "replace" operations.
+ * The SDK converts these to JSON Patch "add" operations.Also applies to: 9785-9789, 9992-9994
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/types/index.d.ts` around lines 9573 - 9577, The JSDoc for the "set" field
incorrectly states the SDK converts values to JSON Patch "replace" operations
while the conversion utility actually emits "add"; update the documentation to
match the implementation (or alternatively change the converter to emit
"replace" if intended). Specifically, edit the JSDoc on the "set" parameter (the
comment block that currently says "The SDK converts these to JSON Patch
'replace' operations") to say it converts to JSON Patch "add" operations, and
make the same change for the duplicated doc blocks at the other occurrences (the
comment blocks around the same "set" description at the other noted locations).
Ensure the wording is consistent across all three spots so API users are not
misled.
| import { TransportMethod } from '../../../types/transport-request'; | ||
| import { AbstractRequest } from '../../../components/request'; | ||
| import RequestOperation from '../../../constants/operations'; | ||
| import * as DataSync from '../../../types/api/data-sync'; | ||
| import { KeySet } from '../../../types/api'; |
There was a problem hiding this comment.
Always send an Idempotency-Key for create requests.
On Line 55, the header is only forwarded if provided. This endpoint is a non-idempotent POST; retries without a key can duplicate entities.
Proposed fix
import { TransportMethod } from '../../../types/transport-request';
import { AbstractRequest } from '../../../components/request';
+import uuidGenerator from '../../../components/uuid';
import RequestOperation from '../../../constants/operations';
import * as DataSync from '../../../types/api/data-sync';
import { KeySet } from '../../../types/api';
@@
protected get headers(): Record<string, string> | undefined {
let headers = super.headers ?? {};
+ const idempotencyKey = this.parameters.idempotencyKey ?? uuidGenerator.createUUID();
- if (this.parameters.idempotencyKey)
- headers = { ...headers, 'Idempotency-Key': this.parameters.idempotencyKey };
+ headers = { ...headers, 'Idempotency-Key': idempotencyKey };
return {
...headers,
'Content-Type': 'application/vnd.pubnub.objects.entity+json;version=1',
};
}Also applies to: 52-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/endpoints/data-sync/entity/create.ts` around lines 7 - 11, The
create endpoint currently only forwards an incoming Idempotency-Key when
present; update the POST request building logic in this file so the
'Idempotency-Key' header is always set for create operations: if an incoming key
(from the request KeySet) exists use it, otherwise generate a new unique id
(UUID/v4) and add it to the outgoing headers before calling
AbstractRequest/TransportMethod for the RequestOperation create; ensure the
header key is exactly 'Idempotency-Key' and include the generated key so retries
are idempotent.
| import PubNubObjects from './pubnub-objects'; | ||
| // endregion | ||
| // region DataSync | ||
| import PubNubDataSync from './pubnub-data-sync'; |
There was a problem hiding this comment.
Missing ./pubnub-data-sync module blocks the build.
Line 119 fails with TS2307 in CI (Cannot find module './pubnub-data-sync' or its corresponding type declarations). This must be fixed before merge by adding the module or correcting the import path/casing to the actual file.
🧰 Tools
🪛 GitHub Actions: Tests
[error] 119-119: Rollup (plugin typescript) failed with TS2307: Cannot find module './pubnub-data-sync' or its corresponding type declarations.
🪛 GitHub Check: Integration and Unit tests (18.18.0, ci:node)
[failure] 119-119:
Cannot find module './pubnub-data-sync' or its corresponding type declarations.
🪛 GitHub Check: Integration and Unit tests (20, ci:node)
[failure] 119-119:
Cannot find module './pubnub-data-sync' or its corresponding type declarations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/pubnub-common.ts` at line 119, The import of PubNubDataSync from
'./pubnub-data-sync' in pubnub-common.ts fails TS2307; fix by either adding the
missing module/file or correcting the import path/casing to the actual module
name, ensuring the exported symbol PubNubDataSync is present and its types are
exported (or add a declaration file) so the TypeScript compiler can resolve the
module.
| export function toJsonPointer(dotPath: string): string { | ||
| return '/' + dotPath.split('.').join('/'); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In RFC 6901 JSON Pointer, how must '~' and '/' be escaped inside reference tokens?
💡 Result:
In RFC 6901 JSON Pointer, the characters '' (%x7E) and '/' (%x2F) must be escaped within reference tokens because they have special meanings. The tilde '' is escaped as '~0' and the slash '/' is escaped as '~1'.
Citations:
- 1: https://rfcinfo.com/rfc-6901/3-syntax/
- 2: https://rfc-editor.org/rfc/rfc6901.txt
- 3: https://datatracker.ietf.org/doc/html/rfc6901
Escape JSON Pointer tokens before building the pointer
Line 158 converts dot paths by separator replacement only. Per RFC 6901, it must escape reserved token characters (~ as ~0, / as ~1). Without escaping, valid field names like "user/id" or "tilde~field" will map to incorrect JSON Pointers.
Proposed fix
export function toJsonPointer(dotPath: string): string {
- return '/' + dotPath.split('.').join('/');
+ return (
+ '/' +
+ dotPath
+ .split('.')
+ .map((segment) => segment.replace(/~/g, '~0').replace(/\//g, '~1'))
+ .join('/')
+ );
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/types/api/data-sync.ts` around lines 157 - 159, The toJsonPointer
function currently joins dotPath tokens without escaping, which breaks RFC 6901
for field names containing '~' or '/'; update toJsonPointer to split dotPath on
'.' into tokens, for each token replace '~' with '~0' and '/' with '~1' (in that
order) and then join with '/' prefixing the result with '/' (handle the empty
dotPath case per RFC by returning an empty string if needed). Ensure you perform
the escapes on each token before joining so names like "user/id" and
"tilde~field" produce correct JSON Pointers.
| export function toJsonPatchOperations( | ||
| set?: Record<string, unknown>, | ||
| remove?: string[], | ||
| ): JsonPatchOperation[] { | ||
| const ops: JsonPatchOperation[] = []; | ||
|
|
||
| if (set) { | ||
| for (const [dotPath, value] of Object.entries(set)) { | ||
| ops.push({ op: 'add', path: toJsonPointer(dotPath), value }); | ||
| } | ||
| } | ||
|
|
||
| if (remove) { | ||
| for (const dotPath of remove) { | ||
| ops.push({ op: 'remove', path: toJsonPointer(dotPath) }); | ||
| } | ||
| } | ||
|
|
||
| return ops; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify internal contract mismatch and requirement statements in this file.
rg -n "set becomes a \"replace\" operation|converts these to JSON Patch \"replace\" operations|op: 'add'|At least one of `set` or `remove` must be provided" src/core/types/api/data-sync.tsRepository: pubnub/javascript
Length of output: 2585
🏁 Script executed:
cat -n src/core/types/api/data-sync.ts | head -200Repository: pubnub/javascript
Length of output: 6047
🏁 Script executed:
rg -n "@param|at least one|must be provided" src/core/types/api/data-sync.ts -A 2 -B 2Repository: pubnub/javascript
Length of output: 630
🏁 Script executed:
rg -l "toJsonPatchOperations" --type ts --type jsRepository: pubnub/javascript
Length of output: 112
🏁 Script executed:
rg -n "toJsonPatchOperations|op: 'add'|op: 'replace'" --type ts -B 2 -A 2 | head -100Repository: pubnub/javascript
Length of output: 976
Align set operation mapping with documented contract and reject empty patches
Line 177 emits op: 'add', but the contract at line 164 states each key in set becomes a "replace" operation. Additionally, the function returns [] when both inputs are missing, which silently violates the documented requirement (lines 307, 554, 791) that "at least one of set or remove must be provided," deferring the error to the server.
Proposed fix
export function toJsonPatchOperations(
set?: Record<string, unknown>,
remove?: string[],
): JsonPatchOperation[] {
const ops: JsonPatchOperation[] = [];
if (set) {
for (const [dotPath, value] of Object.entries(set)) {
- ops.push({ op: 'add', path: toJsonPointer(dotPath), value });
+ ops.push({ op: 'replace', path: toJsonPointer(dotPath), value });
}
}
if (remove) {
for (const dotPath of remove) {
ops.push({ op: 'remove', path: toJsonPointer(dotPath) });
}
}
+ if (ops.length === 0) {
+ throw new Error('At least one of "set" or "remove" must be provided.');
+ }
+
return ops;
}🧰 Tools
🪛 ESLint
[error] 169-172: Replace ⏎··set?:·Record<string,·unknown>,⏎··remove?:·string[],⏎ with set?:·Record<string,·unknown>,·remove?:·string[]
(prettier/prettier)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/types/api/data-sync.ts` around lines 169 - 188, The
toJsonPatchOperations function currently emits op: 'add' for set entries and
silently returns an empty array when both set/remove are absent; update
toJsonPatchOperations so that for each entry in set it emits op: 'replace' (not
'add') and validate inputs at the start: if both set is falsy or an empty object
and remove is falsy or an empty array, throw a descriptive error (e.g., "at
least one of set or remove must be provided"); keep using toJsonPointer for path
conversion and preserve existing behavior for remove entries.
2 participants
initial code readiness for dataSync apis
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes