feat: Add apns-id header to client return value
#195
Conversation
|
🚀 Thanks for opening this pull request! |
📝 WalkthroughWalkthroughClient now reads Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. Comment |
✅ 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. |
![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
🤖 Fix all issues with AI agents
In @lib/client.js:
- Around line 519-521: The statement setting header['apns-id'] when
notificationId is truthy is missing a terminating semicolon; update the
conditional block (the code that checks notificationId and assigns
header['apns-id'] = notificationId) to end the assignment line with a semicolon
to fix the syntax error.
🧹 Nitpick comments (1)
lib/client.js (1)
569-569: Use consistent quote style.This line uses double quotes while the surrounding code (lines 566-568) uses single quotes for consistency.
♻️ Proposed fix
- notificationId = headers["apns-id"]; + notificationId = headers['apns-id'];
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/client.js
🧰 Additional context used
🧬 Code graph analysis (1)
lib/client.js (1)
lib/notification/index.js (1)
headers(74-74)
🔇 Additional comments (2)
lib/client.js (2)
538-538: LGTM!The variable declaration follows the same pattern as other response header variables.
586-586: LGTM!The
notificationIdis correctly passed tocreateHeaderObject, matching the updated method signature and following the same pattern as other response headers.
RedPhoenixQ
force-pushed
the
expose-apns-id
branch
from
12fc01c to
c14ef4e
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #195 +/- ##
==========================================
- Coverage 95.84% 95.74% -0.10%
==========================================
Files 23 23
Lines 842 846 +4
==========================================
+ Hits 807 810 +3
- Misses 35 36 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/client.js (1)
1616-1669: Consider addingapns-idtest coverage for ManageChannelsClient.The device and broadcasts tests now validate
apns-idpropagation, but the ManageChannelsClient tests don't include this coverage. Since the underlyingrequest()method is shared, the feature should work, but adding a test for channels (similar to the device test pattern) would provide additional confidence.This is optional since the core logic is already tested via the device tests.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/client.jstest/client.js
🧰 Additional context used
🧬 Code graph analysis (2)
lib/client.js (1)
test/client.js (1)
notificationId(251-251)
test/client.js (1)
lib/client.js (1)
notificationId(538-538)
🔇 Additional comments (7)
lib/client.js (4)
503-523: LGTM! Clean extension to exposeapns-idin return values.The function signature extension is backwards-compatible since the new
notificationIdparameter is optional and guarded by the truthiness check before adding to the header object.
538-538: LGTM!Variable properly initialized alongside other response header variables.
569-569: LGTM!Correctly extracts
apns-idfrom response headers, following the same pattern as other APNs headers (apns-unique-id,apns-request-id,apns-channel-id).
586-591: LGTM!The
notificationIdis correctly passed tocreateHeaderObject, ensuring theapns-idis included in all resolve/reject outcomes where header information is propagated.test/client.js (3)
137-138: LGTM! Test correctly validatesapns-idpropagation for device notifications.The test properly:
- Defines the expected
apns-idvalue- Configures the mock server to return it in response headers
- Asserts that the client result includes the
apns-idAlso applies to: 150-150, 168-168
194-195: LGTM! Broadcasts test mirrors the device test pattern forapns-id.Also applies to: 207-207, 225-225
251-252: LGTM! Load test verifiesapns-idworks correctly under concurrent requests.Good coverage for ensuring the header propagation works correctly when handling a batch of 2000 simultaneous notifications.
Also applies to: 264-264, 282-282
|
I added the header to existing tests, hope that is satisfactory. The other propagated headers are also tested indirectly but are more directly linked to a specific type of api call. In my understanding the If you'd want a separate test for this specific header I'll do that. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
test/client.js (4)
137-168: Test correctly verifiesapns-idpropagation.The test properly validates that the
apns-idheader from the response is included in the returned result. The use of spread syntax to mergeadditionalHeaderInfowith the result is correct.Optional suggestion: Enhance test clarity
Consider adding a comment explaining that this test verifies the
apns-idheader propagation, or update the test description to reflect this behavior explicitly. This would make the test's purpose clearer:it('Treats HTTP 200 responses as successful for device', async () => { + // This test also verifies that the apns-id header is propagated to the result let didRequest = false;
297-363: Consider clarifying whyapns-idis not included in this test.This test validates session ping behavior and correctly omits the
apns-idheader (since pings aren't notification deliveries). However, it would be helpful to explicitly document this to distinguish it from the other tests that do includeapns-id.Optional: Add clarifying comment
it('Log pings for session', async () => { + // Note: This test does not include apns-id as it's testing ping responses, not notification delivery let establishedConnections = 0;
1913-1979: Consistent with device ping test - consider adding clarification.Like the device session ping test (lines 297-363), this test correctly omits the
apns-idheader since it's testing ping functionality rather than notification delivery. Consider adding a clarifying comment similar to the suggestion for the device ping test.
137-138: Consider using varied notification IDs for clarity.All tests use the same hardcoded
notificationId:'7dc35f9f-58d4-40dd-8c08-38ab811f57df'. While this is acceptable for mock tests, using different UUIDs per test (or test category) could improve clarity and make it easier to trace specific test scenarios.Example: Use test-specific IDs
it('Treats HTTP 200 responses as successful for device', async () => { // ... - const notificationId = '7dc35f9f-58d4-40dd-8c08-38ab811f57df'; + const notificationId = 'device-200-test-notification-id';Or use a UUID generator to create unique IDs per test if preferred.
Also applies to: 194-195, 251-252, 1622-1623, 1682-1682, 1746-1746, 1807-1807, 1867-1867
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/client.js
🧰 Additional context used
🧬 Code graph analysis (1)
test/client.js (1)
lib/client.js (3)
notificationId(538-538)requestId(536-536)uniqueId(535-535)
🔇 Additional comments (4)
test/client.js (4)
1622-1653: LGTM - Consistent test pattern for channels endpoint.The test correctly validates
apns-idpropagation for the channels endpoint, following the same pattern as the device endpoint tests.
1682-1719: LGTM - Properly tests multiple response headers.The test correctly validates that
apns-idis propagated along with other response headers (apns-channel-id,apns-request-id,apns-unique-id) in HTTP 201 responses.
1746-1779: LGTM - Good coverage of DELETE operations.The test correctly validates that
apns-idis propagated in HTTP 204 responses for DELETE operations, providing comprehensive coverage across different HTTP methods.
803-887: No changes needed—the proxy test intentionally omitsapns-idto test backward compatibility.The proxy tests (both device and channels variants) intentionally do not include the
apns-idheader in the response, unlike the standard "Treats HTTP 200 responses as successful" tests. This provides important test coverage for how the client handles responses that lack theapns-idheader. The client gracefully handles this case by extracting the header only if present. If clarity is desired for future maintainers, consider adding an inline comment explaining this is intentional backward-compatibility testing.
|
@RedPhoenixQ Thanks, we generally ask for not modifying existing tests if not necessary and add a new, specific test with an identifier that describes what it tests. So could you please revert the changes (unless necessary for existing tests to pass) and add a distinct test like |
RedPhoenixQ
force-pushed
the
expose-apns-id
branch
from
c1c65c2 to
0c0257d
Compare
3 participants
This exposes the
apns-idheader that is returned to identify a notification for debugging and tracking.Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.