test/browser/connection: fix hmac extraction #1939
Conversation
WalkthroughThe pull request modifies the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
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 (
|
github-actions
bot
temporarily deployed
to
staging/pull/1939/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1939/features
Zariel
force-pushed
the
rar-655
branch
2 times, most recently
from
1c3d568 to
079a7dc
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1939/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1939/features
Relax the connectionKey hmac extraction and account for the possibilities of multiple - characters. RAR-655
|
supersedes #1903 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/browser/connection.test.js (1)
218-218: Document the connection key format.The comment
/* connectionKey has the form <instanceID>!<hmac>-<handleID> */could be more detailed to explain that the HMAC portion may contain hyphens.-/* strip instanceID and handleID from connectionKey */ +/** + * Extracts the HMAC portion from a connection key. + * @param {string} key - Connection key in the format "<instanceID>!<hmac>-<handleID>" + * where the HMAC portion may contain hyphens. + * @returns {string} The extracted HMAC portion. + */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/browser/connection.test.js(1 hunks)
🔇 Additional comments (1)
test/browser/connection.test.js (1)
219-224: LGTM! The HMAC extraction logic handles multiple hyphens correctly.The implementation correctly uses
lastIndexOf('-')to handle cases where the HMAC portion contains hyphens, addressing the PR objective.Let's verify the format assumption with a quick search:
✅ Verification successful
Implementation correctly follows the documented connection key format
The code aligns with the documented format
<instanceID>!<hmac>-<handleID>and correctly handles cases where the HMAC portion contains hyphens. The format is consistently used across test files.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for connection key format documentation or usage # Expected: Find references confirming the "<instanceID>!<hmac>-<handleID>" format # Search for connection key format documentation rg -l "connection.*key.*format|connectionKey.*format" # Search for connection key usage patterns rg "!.*-.*" --type jsLength of output: 618
Script:
#!/bin/bash # Check ably.d.ts for connection key format documentation rg -A 3 "connectionKey" ably.d.ts # Search for any additional connection key usages in test files ast-grep --pattern 'connectionKey: $_'Length of output: 74
There was a problem hiding this comment.
why go to a lot of effort to parse out the connectionId from the connectionKey rather than just comparing connectionIds? trying to do the former is inherently flaky: the connectionkey format is a secret of the server and the sdk should not depend on it, either in code or tests. the library exposes the connectionId in connection.id, just use that
There was a problem hiding this comment.
Agree with Simon on this, ideally we should not depend on some internal realtime knowledge that can change any time when there is an alternative.
It looks like @lawrence-forooghian just opened a PR #1957 that fixes this issue the way Simon suggested - removes parsing of the connection key and compare connection id. I suggest we go with that PR and close this one.
|
D'oh, completely missed the existence of this PR, sorry! Will merge #1957. |
Relax the connectionKey hmac extraction and account for the possibilities of multiple - characters.
RAR-655
Summary by CodeRabbit