feat: Add push provider throttle and push priority processing #420
Conversation
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. |
📝 WalkthroughWalkthroughA throttling and prioritization mechanism for push notifications was introduced using a new Changes
Sequence Diagram(s)Throttled and Prioritized Push SendingsequenceDiagram
participant Client
participant ParsePushAdapter
participant ThrottleQueue
participant PushSender
Client->>ParsePushAdapter: send(data, installations)
alt Queue configured for push type
ParsePushAdapter->>ThrottleQueue: enqueue({task, ttl, priority})
ThrottleQueue-->>ParsePushAdapter: (waits for slot, respects TTL/priority)
ThrottleQueue->>PushSender: send(data, installations)
PushSender-->>ThrottleQueue: result
ThrottleQueue-->>ParsePushAdapter: result
else No queue configured
ParsePushAdapter->>PushSender: send(data, installations)
PushSender-->>ParsePushAdapter: result
end
ParsePushAdapter-->>Client: result
Task Dropped Due to TTL ExpirysequenceDiagram
participant ParsePushAdapter
participant ThrottleQueue
ParsePushAdapter->>ThrottleQueue: enqueue({task, ttl=short})
ThrottleQueue-->>ParsePushAdapter: (waits in queue)
alt TTL expired before execution
ThrottleQueue-->>ParsePushAdapter: null (task skipped)
else TTL valid
ThrottleQueue->>PushSender: send()
PushSender-->>ThrottleQueue: result
ThrottleQueue-->>ParsePushAdapter: result
end
Priority-based Task ExecutionsequenceDiagram
participant ParsePushAdapter
participant ThrottleQueue
ParsePushAdapter->>ThrottleQueue: enqueue({task1, priority=1})
ParsePushAdapter->>ThrottleQueue: enqueue({task2, priority=10})
ParsePushAdapter->>ThrottleQueue: enqueue({task3, priority=5})
Note right of ThrottleQueue: Tasks are sorted by priority
ThrottleQueue->>PushSender: send(task2)
ThrottleQueue->>PushSender: send(task3)
ThrottleQueue->>PushSender: send(task1)
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
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 (
|
mtrezza
changed the title
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #420 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 9 10 +1
Lines 1351 1404 +53
=========================================
+ Hits 1351 1404 +53 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (3)
README.md (2)
239-239: Fix grammatical error in documentationThe sentence has a subject-verb agreement issue.
-In the example above, pushes will be sent in bursts of 5 at once, with max. 10 pushes within 1s. On a timeline that means at `t=0ms`, 5 pushes will be sent in parallel. If sending the pushes take less than 500ms, then `intervalCapacity` will still limit to 5 pushes within the first 500ms. +In the example above, pushes will be sent in bursts of 5 at once, with max. 10 pushes within 1s. On a timeline that means at `t=0ms`, 5 pushes will be sent in parallel. If sending the pushes takes less than 500ms, then `intervalCapacity` will still limit to 5 pushes within the first 500ms.
247-247: Add missing article in documentationMissing determiner before "Default".
-| `queue.ttl` | `Infinity` | Yes | The time-to-live of the push in the queue in seconds. If a queued push expires before it is sent to the push provider, it is discarded. Default is `Infinity`, meaning pushes never expire. | +| `queue.ttl` | `Infinity` | Yes | The time-to-live of the push in the queue in seconds. If a queued push expires before it is sent to the push provider, it is discarded. The default is `Infinity`, meaning pushes never expire. |src/ParsePushAdapter.js (1)
94-98: Consider alternatives to the delete operator for better performance.The static analysis tool flags the
deleteoperation as potentially performance-impacting. Consider using object destructuring to avoid mutation:- } else if (this.queues[pushType]) { - const { ttl, priority } = data?.queue || {}; - delete data?.queue; - sendPromises.push(this.queues[pushType].enqueue({ task: () => sender.send(data, devices), ttl, priority })); + } else if (this.queues[pushType]) { + const { queue, ...dataWithoutQueue } = data || {}; + const { ttl, priority } = queue || {}; + sendPromises.push(this.queues[pushType].enqueue({ task: () => sender.send(dataWithoutQueue, devices), ttl, priority }));This approach creates a new object without the
queueproperty instead of mutating the original object.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
README.md(2 hunks)package.json(1 hunks)spec/.eslintrc.json(1 hunks)spec/ParsePushAdapter.spec.js(2 hunks)spec/ThrottleQueue.spec.js(1 hunks)spec/helper.js(1 hunks)src/ParsePushAdapter.js(4 hunks)src/ThrottleQueue.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
spec/ThrottleQueue.spec.js (2)
src/ThrottleQueue.js (1)
ThrottleQueue(3-42)spec/helper.js (1)
wait(14-14)
spec/ParsePushAdapter.spec.js (2)
src/ParsePushAdapter.js (1)
ParsePushAdapter(14-108)spec/helper.js (1)
wait(14-14)
🪛 GitHub Check: Lint
spec/helper.js
[warning] 14-14:
'setTimeout' is not defined
🪛 LanguageTool
README.md
[uncategorized] ~239-~239: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...sent in parallel. If sending the pushes take less than 500ms, then `intervalCapacity...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~247-~247: A determiner appears to be missing. Consider inserting it.
Context: ... to the push provider, it is discarded. Default is Infinity, meaning pushes never exp...
(AI_EN_LECTOR_MISSING_DETERMINER)
🪛 Biome (1.9.4)
src/ParsePushAdapter.js
[error] 96-96: Avoid the delete operator which can impact performance.
(lint/performance/noDelete)
🔇 Additional comments (10)
spec/helper.js (2)
9-9: LGTM: Increased timeout for throttling testsThe timeout increase to 50 seconds is appropriate for tests that verify throttling behavior and timing constraints.
14-18: LGTM: Clean utility function implementationThe
waitfunction is a simple and effective utility for introducing delays in asynchronous tests. The static analysis warning aboutsetTimeoutbeing undefined is a false positive -setTimeoutis a global function in Node.js environments.package.json (1)
32-32: LGTM: Adding p-queue dependencyThe
p-queue@8.1.0dependency is appropriately added to support the new ThrottleQueue functionality. This is a well-maintained library that provides the concurrency and priority queue capabilities needed for the throttling mechanism.spec/.eslintrc.json (1)
3-3: LGTM: Consistent indentationMinor formatting improvement that maintains consistent indentation throughout the ESLint configuration.
README.md (1)
187-262: Excellent documentation for the new push queue featureThe documentation comprehensively covers the new throttling and priority functionality with clear examples and parameter descriptions. The practical examples showing serial vs parallel configurations are particularly helpful.
spec/ThrottleQueue.spec.js (1)
1-67: Excellent comprehensive test suite for ThrottleQueueThe test suite thoroughly validates the core functionality of the ThrottleQueue class:
Rate limiting test: Properly verifies that tasks respect the configured interval capacity by measuring execution timing with appropriate tolerance.
TTL expiration test: Cleverly tests that expired tasks are dropped by using a blocking task and verifying the expired task doesn't execute.
Priority ordering test: Well-designed test that blocks the queue to build up tasks, then verifies they execute in correct priority order.
The tests use appropriate async patterns, timing controls, and assertions. The use of the
waithelper function fromspec/helper.jsis clean and effective.src/ThrottleQueue.js (1)
1-42: LGTM! Clean implementation of throttling queue wrapper.The
ThrottleQueueclass provides a well-designed wrapper aroundp-queuewith additional TTL functionality. The implementation correctly:
- Uses sensible defaults for throttling parameters
- Converts TTL from seconds to milliseconds
- Handles task expiration by returning null for expired tasks
- Passes priority through to the underlying queue
The code is readable and follows good practices.
spec/ParsePushAdapter.spec.js (3)
647-673: Excellent test coverage for throttling functionality.This test properly validates the throttling behavior by:
- Configuring a queue with 1 task per second limit
- Measuring timestamps of actual send calls
- Asserting that the time difference is approximately 1 second
The timing assertions with tolerance (900-1100ms) account for potential execution delays, making the test robust.
675-698: Well-designed TTL expiration test.This test effectively validates TTL functionality by:
- Using a send method that takes longer than the TTL
- Sending two pushes concurrently, one with a very short TTL
- Asserting that only one send call actually occurs
The test correctly demonstrates that expired tasks are skipped.
700-736: Comprehensive priority ordering test.This test excellently validates priority handling by:
- Using a blocking task to control queue execution timing
- Sending multiple pushes with different priorities
- Asserting that execution order matches priority order (highest first)
The test design with the blocking task ensures that all priority tasks are enqueued before any are processed, making the priority comparison valid.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
spec/helper.js (1)
14-18: Address the setTimeout linting warning.The
waitfunction implementation is correct and serves a legitimate testing purpose for timing-dependent behaviors. However, the static analysis warning about 'setTimeout' not being defined should be addressed for better compatibility.Consider importing
setTimeoutfrom the 'timers' module to resolve the linting warning:+import { setTimeout } from 'timers';Or if you prefer to keep it as a global, you can add a comment to disable the linting warning:
-const wait = (ms) => new Promise(resolve => setTimeout(resolve, ms)); +const wait = (ms) => new Promise(resolve => setTimeout(resolve, ms)); // eslint-disable-line no-undef
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
spec/helper.js(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: mtrezza
PR: parse-community/parse-server-push-adapter#420
File: src/ParsePushAdapter.js:60-65
Timestamp: 2025-07-02T21:55:05.380Z
Learning: In ParsePushAdapter.js, the queue configuration intentionally uses `intervalCapacity` as the config parameter name, which is then mapped to `intervalCap` when passed to the ThrottleQueue constructor. This mapping is by design and should not be "fixed" to match parameter names.
🪛 GitHub Check: Lint
spec/helper.js
[warning] 14-14:
'setTimeout' is not defined
🔇 Additional comments (1)
spec/helper.js (1)
3-3: LGTM: Import reordering.The import reordering is a minor code organization improvement with no functional impact.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/ParsePushAdapter.js (2)
61-61: Simplify the optional chain expression.The current expression can be simplified using optional chaining for better readability.
- const queue = Array.isArray(config) ? config.find(c => c && c.queue)?.queue : config.queue; + const queue = Array.isArray(config) ? config.find(c => c?.queue)?.queue : config?.queue;
97-97: Consider avoiding the delete operator for better performance.The delete operator can impact performance. Consider using object destructuring or creating a new object without the queue property.
- const { ttl, priority } = data?.queue || {}; - delete data?.queue; - sendPromises.push(this.queues[pushType].enqueue({ task: () => sender.send(data, devices), ttl, priority })); + const { ttl, priority } = data?.queue || {}; + const { queue, ...dataWithoutQueue } = data; + sendPromises.push(this.queues[pushType].enqueue({ task: () => sender.send(dataWithoutQueue, devices), ttl, priority }));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
spec/helper.js(1 hunks)src/ParsePushAdapter.js(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/helper.js
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mtrezza
PR: parse-community/parse-server-push-adapter#420
File: src/ParsePushAdapter.js:60-65
Timestamp: 2025-07-02T21:55:05.380Z
Learning: In ParsePushAdapter.js, the queue configuration intentionally uses `intervalCapacity` as the config parameter name, which is then mapped to `intervalCap` when passed to the ThrottleQueue constructor. This mapping is by design and should not be "fixed" to match parameter names.
src/ParsePushAdapter.js (1)
Learnt from: mtrezza
PR: parse-community/parse-server-push-adapter#420
File: src/ParsePushAdapter.js:60-65
Timestamp: 2025-07-02T21:55:05.380Z
Learning: In ParsePushAdapter.js, the queue configuration intentionally uses `intervalCapacity` as the config parameter name, which is then mapped to `intervalCap` when passed to the ThrottleQueue constructor. This mapping is by design and should not be "fixed" to match parameter names.
🪛 Biome (1.9.4)
src/ParsePushAdapter.js
[error] 61-61: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 97-97: Avoid the delete operator which can impact performance.
(lint/performance/noDelete)
🔇 Additional comments (4)
src/ParsePushAdapter.js (4)
10-10: LGTM! Clean import addition.The import statement is correctly added to support the new throttling functionality.
21-21: LGTM! Proper property initialization.The queues property is correctly initialized to store ThrottleQueue instances per push type.
60-65: LGTM! Queue configuration parsing is well-implemented.The queue configuration parsing correctly handles both array and object configurations, and the parameter name mapping from
intervalCapacitytointervalCapis intentional as per the design.
95-98: LGTM! Queue integration is well-implemented.The conditional logic correctly integrates the throttling queue when available, properly extracts TTL and priority parameters, and maintains backward compatibility when no queue is configured.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/ParsePushAdapter.js (2)
60-66: Simplify the optional chaining and consider error handling.The queue configuration logic is correct, but there are two potential improvements:
- The optional chaining can be simplified as suggested by static analysis
- Consider adding error handling for ThrottleQueue construction
Apply this diff to simplify the optional chaining:
- const queue = Array.isArray(config) ? config.find(c => c && c.queue)?.queue : config.queue; + const queue = Array.isArray(config) ? config.find(c => c?.queue)?.queue : config.queue;Consider adding error handling for ThrottleQueue construction:
if (queue) { const { concurrency, intervalCapacity, interval } = queue || {}; - this.queues[pushType] = new ThrottleQueue({ concurrency, intervalCap: intervalCapacity, interval }); + try { + this.queues[pushType] = new ThrottleQueue({ concurrency, intervalCap: intervalCapacity, interval }); + } catch (error) { + log.error(LOG_PREFIX, `Failed to create throttle queue for ${pushType}:`, error); + } }
95-98: Consider cleaning up queue metadata from data object.The queue logic correctly handles conditional enqueueing, but the
data.queuemetadata is passed through to the sender. Consider whether this metadata should be removed before sending.If the queue metadata should not be passed to the sender, apply this diff:
} else if (this.queues[pushType]) { const { ttl, priority } = data?.queue || {}; + const { queue, ...cleanData } = data || {}; - sendPromises.push(this.queues[pushType].enqueue({ task: () => sender.send(data, devices), ttl, priority })); + sendPromises.push(this.queues[pushType].enqueue({ task: () => sender.send(cleanData, devices), ttl, priority }));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/ParsePushAdapter.js(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mtrezza
PR: parse-community/parse-server-push-adapter#420
File: src/ParsePushAdapter.js:60-65
Timestamp: 2025-07-02T21:55:05.380Z
Learning: In ParsePushAdapter.js, the queue configuration intentionally uses `intervalCapacity` as the config parameter name, which is then mapped to `intervalCap` when passed to the ThrottleQueue constructor. This mapping is by design and should not be "fixed" to match parameter names.
src/ParsePushAdapter.js (1)
Learnt from: mtrezza
PR: parse-community/parse-server-push-adapter#420
File: src/ParsePushAdapter.js:60-65
Timestamp: 2025-07-02T21:55:05.380Z
Learning: In ParsePushAdapter.js, the queue configuration intentionally uses `intervalCapacity` as the config parameter name, which is then mapped to `intervalCap` when passed to the ThrottleQueue constructor. This mapping is by design and should not be "fixed" to match parameter names.
🪛 Biome (1.9.4)
src/ParsePushAdapter.js
[error] 61-61: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
src/ParsePushAdapter.js (2)
10-10: LGTM!The ThrottleQueue import follows the existing import pattern and is correctly placed.
21-21: LGTM!The queues property initialization is appropriate for storing ThrottleQueue instances per push type.
New Pull Request Checklist
Issue Description
Pushes are sent out as fast as possible when calling
Parse.Push.send()in Parse Server Cloud Code. A push provider may have API request limits that when exceeded cause requests being rejected.Approach
Pushes can now be throttled per provider using a queue configuration to prevent exceeding rate limits imposed by services like APNS and FCM.
Example for parallel sending 5 pushes every 500ms, effectively allowing up to 10 pushes per second in 2 controlled bursts:
Note
This feature has been implemented specifically in the push adapter, not in Parse Server where there is also a
PushQueue. The push queue in Parse Server is responsible for gathering the push recipients' device tokens from the database in controlled batches, and then sending them off to the push adapter and tracking the push status. Since throttling is a push provider API topic, the feature of throttling is implemented as closely as possible to the push provider API, which is the push adapter.TODOs before merging
Summary by CodeRabbit
p-queuepackage.