fix: too fast polling #4007
fix: too fast polling #4007
Conversation
WalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
warning [email protected]: This version is no longer supported. Please see https://eslint.org/version-support for other options. (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
maxinteger
force-pushed
the
fix/SPARK-586223-too-fast-auth-polling
branch
from
0dd0d89
to
46623f8
Compare
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
![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
🧹 Outside diff range and nitpick comments (4)
packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js (4)
464-469: Include 'expires_in' in 'sampleData' for consistencyThe
sampleDataobject lacks theexpires_infield, which is used in other tests for timing purposes. Including it will make the test data consistent and ensure accurate timing in the test.Apply this diff to include
expires_in:const sampleData = { device_code: 'test123', + expires_in: 300, user_code: '421175', verification_uri: 'http://example.com', verification_uri_complete: 'http://example.com', interval: 2, };
501-506: Include 'expires_in' in 'sampleData' for consistencyThe
sampleDataobject lacks theexpires_infield, which is used in other tests for timing purposes. Including it will make the test data consistent and ensure accurate timing in the test.Apply this diff to include
expires_in:const sampleData = { device_code: 'test123', + expires_in: 300, user_code: '421175', verification_uri: 'http://example.com', verification_uri_complete: 'http://example.com', interval: 2, };
611-611: Remove commented-out codeThere's a commented-out call to
await clock.runAllAsync(). Please remove it if it's no longer needed to keep the code clean.Apply this diff to remove the commented code:
- //await clock.runAllAsync()
715-715: Inconsistent error message—use 'authorization_pending'The error message in the rejected promises is
'authorizationPending', but according to the OAuth 2.0 Device Flow specification, it should be'authorization_pending'. Ensure consistency in error messages.Apply this diff to correct the error messages:
// At line 715 - .rejects({statusCode: 428, body: {message: 'authorizationPending'}}); + .rejects({statusCode: 428, body: {message: 'authorization_pending'}}); // At line 751 - webex.request.rejects({statusCode: 428, body: {message: 'authorizationPending'}}); + webex.request.rejects({statusCode: 428, body: {message: 'authorization_pending'}});Also applies to: 751-751
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js(8 hunks)packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js(13 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js
[error] 361-361: Illegal return statement outside of a function
(parse)
[error] 437-437: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 448-461: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
🔇 Additional comments (2)
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js (2)
448-461: 🛠️ Refactor suggestion
Remove redundant block statement in cancelQRCodePolling method
There's an unnecessary block statement in the cancelQRCodePolling method that doesn't serve any purpose and can be safely removed to simplify the code.
Apply this diff to remove the redundant block:
cancelQRCodePolling(withCancelEvent = true) {
- {
if (this.pollingTimer && withCancelEvent) {
this.eventEmitter.emit('qRCodeLogin', {
eventType: 'pollingCanceled',
});
}
this.currentPollingId = null;
clearTimeout(this.pollingExpirationTimer);
this.pollingExpirationTimer = null;
clearTimeout(this.pollingTimer);
this.pollingTimer = null;
- }
}Ensure that all braces are correctly matched and that the method does not contain unnecessary blocks that could confuse readers or cause unintended behaviors.
Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 448-461: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
356-361:
Fix 'return' statement outside of function scope
The return statement at line 361 is outside the function scope, which causes a syntax error: "Illegal return statement outside of a function." This prevents the code from executing correctly.
Apply this diff to ensure the return statement is within the function scope:
if (this.pollingTimer) {
this.eventEmitter.emit('qRCodeLogin', {
eventType: 'authorizationFailure',
data: {message: 'There is already a polling request'},
});
- return;
+ return;
}Make sure the indentation reflects that the return statement is inside the if block and the function _startQRCodePolling.
Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 361-361: Illegal return statement outside of a function
(parse)
packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js
Show resolved
Hide resolved
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js
Show resolved
Hide resolved
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js (2)
363-364: Consider using a named constant for the default expiration.The default value of 300 seconds should be defined as a named constant at the class level for better maintainability.
+ static DEFAULT_EXPIRATION_SECONDS = 300; + - const {device_code: deviceCode, expires_in: expiresIn = 300} = options; + const {device_code: deviceCode, expires_in: expiresIn = this.DEFAULT_EXPIRATION_SECONDS} = options;
447-459: Consider extracting timer cleanup logic.While the implementation is correct, the timer cleanup logic could be more maintainable if extracted into a separate method.
+ _cleanupTimers() { + clearTimeout(this.pollingExpirationTimer); + this.pollingExpirationTimer = null; + clearTimeout(this.pollingTimer); + this.pollingTimer = null; + } + cancelQRCodePolling(withCancelEvent = true) { if (this.pollingTimer && withCancelEvent) { this.eventEmitter.emit('qRCodeLogin', { eventType: 'pollingCanceled', }); } this.currentPollingId = null; - clearTimeout(this.pollingExpirationTimer); - this.pollingExpirationTimer = null; - clearTimeout(this.pollingTimer); - this.pollingTimer = null; + this._cleanupTimers(); }🧰 Tools
🪛 Biome (1.9.4)
[error] 447-460: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js(7 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js
[error] 360-360: Illegal return statement outside of a function
(parse)
[error] 436-436: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 447-460: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
🔇 Additional comments (5)
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js (5)
70-112: LGTM! Well-structured property declarations.
The new properties are well-documented with JSDoc comments and follow a consistent naming convention. The eventEmitter and polling-related properties provide a clear separation of concerns for managing the polling state.
Line range hint 295-326: LGTM! Robust initialization with proper error handling.
The changes properly prevent concurrent polling requests and maintain consistent event emission patterns. The success path appropriately transitions to polling state.
408-414: LGTM! Proper handling of slow_down error.
The implementation correctly handles the slow_down error by doubling the interval and continuing polling, which aligns with common rate-limiting best practices.
435-437: 🛠️ Refactor suggestion
Refactor assignment in arrow function.
The assignment within the arrow function expression can be confusing and is flagged by static analysis. Let's make it more explicit.
- const schedulePolling = (interval) =>
- (this.pollingTimer = setTimeout(polling, interval * 1000));
+ const schedulePolling = (interval) => {
+ this.pollingTimer = setTimeout(polling, interval * 1000);
+ };Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 436-436: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
Line range hint 295-459: Verify the polling interval behavior in production.
The changes look good and address the PR objectives. However, since this is a critical authentication component, it would be prudent to verify the polling behavior in production, especially:
- The handling of slow_down errors
- The prevention of stale events from previous polling sessions
- The cleanup of timers on cancellation
✅ Verification successful
Based on the search results, I can see that the polling interval behavior and slow_down handling are properly implemented in the codebase. Let me verify the specific aspects mentioned in the review comment with additional context.
Based on the search results, I can see that the code has proper implementation and test coverage for all the key polling behaviors:
- Timer cleanup and stale event prevention:
- Uses
currentPollingIdto prevent stale events - Properly cleans up timers in
cancelQRCodePolling - Checks for existing polling before starting new one
- Slow down error handling:
- Correctly handles 400 status with 'slow_down' message
- Doubles the interval when slow_down is received
- Has test coverage: "should skip a interval when server ask for slow_down"
- Polling interval behavior:
- Properly respects polling intervals
- Has test coverage for interval behavior
- Handles expiration timeouts correctly
The implementation is robust and well-tested. No production verification needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any existing issues or discussions about polling intervals
# and rate limiting in the codebase
# Search for related comments about polling intervals
rg -i "polling.*interval|slow.*down" --type js
# Search for rate limiting related code
ast-grep --pattern 'slow_down'
Length of output: 132670
Script:
#!/bin/bash
# Search for polling timer cleanup and stale event prevention
rg -A 5 "pollingTimer|currentPollingId" --type js
# Search for slow_down error handling implementation
rg -A 5 "slow_down.*statusCode.*400" --type js
# Search for test cases related to polling behavior
rg -A 10 "should.*polling" --type js
Length of output: 143498
🧰 Tools
🪛 Biome (1.9.4)
[error] 436-436: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
| @@ -446,46 +446,46 @@ describe('plugin-authorization-browser-first-party', () => { | |||
| describe('#initQRCodeLogin()', () => { | |||
| it('should prevent concurrent request if there is already a polling request', async () => { | |||
There was a problem hiding this comment.
question. Shouldn't the initQRCodeLogin return immediately if there is already a polling request? How does the code reach the catch block and emit the getUserCodeFailure event?
There was a problem hiding this comment.
Previously we hade 2 ways to report errors, 1) reject the promise from initQRCodeLogin and 2) emit error form _startQRCodePolling.
Then we unified the two and now we handle all errors with events and initQRCodeLogin does not return with promise anymore
- skip 1 interval in polling after the server respond with 'slow_down' - prevent emitting events from previous polling sessions - refactored timeout logic issue: https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-586223
- Removed unused import
maxinteger
force-pushed
the
fix/SPARK-586223-too-fast-auth-polling
branch
from
92f1630
to
53045fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js (2)
611-611: Remove commented codeThe commented line
//await clock.runAllAsync()is unnecessary and should be removed.
781-785: Maintain consistent whitespaceThere are inconsistent blank lines around assertions. Consider maintaining a single blank line between logical groups of assertions for better readability.
Apply this diff:
webex.authorization._startQRCodePolling(options); assert.isDefined(webex.authorization.pollingTimer); - webex.authorization.cancelQRCodePolling(); assert.isNull(webex.authorization.pollingTimer); -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js(7 hunks)packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js(13 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js
[error] 360-360: Illegal return statement outside of a function
(parse)
[error] 436-436: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 447-460: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
🔇 Additional comments (8)
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js (5)
70-112: LGTM! Well-structured property declarations
The new properties are well-documented and form a coherent set for managing polling state and events.
Line range hint 295-326: LGTM! Good concurrent polling prevention
The check for existing polling requests prevents resource waste and provides clear feedback through events.
Line range hint 374-433: LGTM! Robust polling implementation with proper error handling
The implementation effectively handles various scenarios:
- Prevents stale events using pollingId tracking
- Properly handles 'slow_down' responses by doubling the interval
- Maintains polling for pending authorizations
🧰 Tools
🪛 Biome (1.9.4)
[error] 436-436: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
447-459: LGTM! Complete cleanup of polling resources
The cancellation implementation:
- Properly cleans up all timers
- Prevents stale events by resetting currentPollingId
- Provides flexibility with the withCancelEvent parameter
🧰 Tools
🪛 Biome (1.9.4)
[error] 447-460: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
435-437: 🛠️ Refactor suggestion
Refactor the arrow function to avoid assignment in expression
The current implementation assigns within an expression, which can be confusing and is flagged by static analysis.
Apply this diff to improve clarity:
-const schedulePolling = (interval) =>
- (this.pollingTimer = setTimeout(polling, interval * 1000));
+const schedulePolling = (interval) => {
+ this.pollingTimer = setTimeout(polling, interval * 1000);
+};Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 436-436: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js (3)
664-693: Well-implemented test for slow_down handling
This test case effectively verifies that the polling mechanism respects the server's slow_down message by skipping one interval. This directly addresses the PR objective of fixing too fast polling.
695-736: Good test coverage for polling state management
This test case thoroughly verifies that responses from previous polling sessions are properly ignored, preventing race conditions and ensuring clean state management.
273-279: 🛠️ Refactor suggestion
Use consistent assertion style
The test uses expect while the rest of the test file uses assert. For consistency, use assert from chai.
Apply this diff to maintain consistency:
- expect(err?.message).toBe('Cannot convert object to primitive value');
+ assert.equal(err?.message, 'Cannot convert object to primitive value');Likely invalid or redundant comment.
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js
Show resolved
Hide resolved
| this.pollingExpirationTimer = setTimeout(() => { | ||
| this.cancelQRCodePolling(false); | ||
| this.eventEmitter.emit('qRCodeLogin', { | ||
| eventType: 'authorizationFailure', |
There was a problem hiding this comment.
the emitted event types should probably be in some enum or some other type so that client using this API can easily know what are all possible events they can get without reading the implementation.
- added exported Events object for better consumer side API
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js (1)
28-33: Consider defining event types as an enumerationFollowing up on the previous review comment, consider defining all possible event types (getUserCodeSuccess, getUserCodeFailure, etc.) in the Events constant to make the API more discoverable.
export const Events = { /** * QR code login events */ - qRCodeLogin: 'qRCodeLogin', + qRCodeLogin: { + name: 'qRCodeLogin', + types: { + GET_USER_CODE_SUCCESS: 'getUserCodeSuccess', + GET_USER_CODE_FAILURE: 'getUserCodeFailure', + AUTHORIZATION_SUCCESS: 'authorizationSuccess', + AUTHORIZATION_FAILURE: 'authorizationFailure', + AUTHORIZATION_PENDING: 'authorizationPending', + POLLING_CANCELED: 'pollingCanceled' + } + } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js(7 hunks)packages/@webex/plugin-authorization-browser-first-party/src/index.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js
[error] 362-362: Illegal return statement outside of a function
(parse)
[error] 370-370: Illegal return statement outside of a function
(parse)
[error] 446-446: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 457-470: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
🔇 Additional comments (7)
packages/@webex/plugin-authorization-browser-first-party/src/index.js (2)
17-17: LGTM! Clean addition of Events export.
The addition of the Events export aligns with the PR's objective to improve polling session management and event handling.
17-17: Verify Events constant usage.
Let's verify how the Events constant is being used in relation to the polling mechanism.
✅ Verification successful
Events constant is correctly used with polling mechanism
The Events constant is properly defined and used in the codebase. It defines a single event qRCodeLogin which is emitted during various polling states including:
- When handling slow_down responses from the server
- During polling initialization
- For authorization success/failure states
- When polling is cancelled
- When authorization is pending
The polling mechanism correctly uses the Events constant to communicate its state changes, and the slow_down error handling is properly implemented with the event system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the Events constant definition and its usage in polling-related code
# Check Events definition and usage in authorization.js
echo "Checking Events definition and usage in authorization.js:"
rg -A 5 "Events.*=.*{" packages/@webex/plugin-authorization-browser-first-party/src/authorization.js
# Check for polling-related usage of Events
echo "Checking polling-related usage of Events:"
rg -A 10 "slow_down.*Events|Events.*slow_down" packages/@webex/plugin-authorization-browser-first-party/
Length of output: 515
Script:
#!/bin/bash
# Let's try a different approach to check Events definition and usage
# First, let's check the Events constant definition
echo "Checking Events constant definition:"
rg "export const Events" packages/@webex/plugin-authorization-browser-first-party/src/
# Then, check any Events object definition
echo -e "\nChecking Events object definition:"
rg "const Events = {" packages/@webex/plugin-authorization-browser-first-party/src/
# Check all usages of Events in the codebase
echo -e "\nChecking all Events usages:"
rg "Events\." packages/@webex/plugin-authorization-browser-first-party/src/
# Check files importing Events
echo -e "\nChecking Events imports:"
rg "import.*Events" packages/@webex/plugin-authorization-browser-first-party/src/
Length of output: 2146
Script:
#!/bin/bash
# Let's check the Events definition and see if there are any polling-related events
echo "Checking Events definition content:"
rg -A 10 "export const Events = {" packages/@webex/plugin-authorization-browser-first-party/src/authorization.js
# Also check for any polling-related code
echo -e "\nChecking for polling-related code:"
rg -B 2 -A 5 "poll" packages/@webex/plugin-authorization-browser-first-party/src/authorization.js
Length of output: 4672
Script:
#!/bin/bash
# Let's check the actual Events constant definition with a simpler pattern
echo "Checking Events constant definition:"
rg -B 1 -A 5 "^export const Events" packages/@webex/plugin-authorization-browser-first-party/src/authorization.js
# Also check if there are any other files using the slow_down error handling
echo -e "\nChecking for slow_down handling in other files:"
rg "slow_down" packages/@webex/plugin-authorization-browser-first-party/src/
Length of output: 836
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js (5)
80-122: LGTM! Well-documented properties
The new properties are well-documented with JSDoc comments and have clear purposes.
Line range hint 305-346: LGTM! Robust initialization with proper guards
The implementation properly prevents concurrent polling requests and consistently handles success/failure cases.
418-423: LGTM! Good implementation of backoff strategy
The implementation properly handles the slow_down message by doubling the polling interval.
457-469: LGTM! Thorough cleanup of polling resources
The implementation properly cleans up all timers and prevents stale requests through pollingId reset.
🧰 Tools
🪛 Biome (1.9.4)
[error] 457-470: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
445-447: 🛠️ Refactor suggestion
Avoid assignment within expression
Following up on the previous review comment, the assignment within the arrow function expression should be refactored for better readability.
-const schedulePolling = (interval) =>
- (this.pollingTimer = setTimeout(polling, interval * 1000));
+const schedulePolling = (interval) => {
+ this.pollingTimer = setTimeout(polling, interval * 1000);
+};🧰 Tools
🪛 Biome (1.9.4)
[error] 446-446: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
COMPLETES [SPARK-586223](issue: https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-586223)
This pull request addresses
Handle
slow_downerror message from serverby making the following changes
Change Type
The following scenarios were tested
I certified that
I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly
Make sure to have followed the contributing guidelines before submitting.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Eventsconstant.Bug Fixes
Tests