Implement two-stage protocol fallback in fetchProps for chat requests' or 'Add legacy protocol-1 fallback to fetchProps for compatibility#2478
Open
ojuolokun86 wants to merge 3 commits intoWhiskeySockets:masterfrom
Conversation
Contributor
|
Thanks for opening this pull request and contributing to the project! The next step is for the maintainers to review your changes. If everything looks good, it will be approved and merged into the main branch. In the meantime, anyone in the community is encouraged to test this pull request and provide feedback. ✅ How to confirm it worksIf you’ve tested this PR, please comment below with: This helps us speed up the review and merge process. 📦 To test this PR locally:If you encounter any issues or have feedback, feel free to comment as well. |
📝 WalkthroughWalkthroughfetchProps was rewritten to try protocol-2 ( Changes
Sequence DiagramsequenceDiagram
participant Caller as Caller
participant FP as fetchProps()
participant P2 as Protocol-2 (xmlns: 'abt')
participant P1 as Protocol-1 (xmlns: 'w')
participant Creds as Credentials Storage
participant Evt as Event Emitter
participant Logger as Logger
Caller->>FP: call fetchProps()
FP->>P2: send IQ props request (protocol: '2')
alt Protocol-2 returns valid props
P2-->>FP: props node
FP->>FP: reduce props -> dict
FP->>Creds: update lastPropHash (if present)
FP->>Evt: emit creds.update (if hash present)
FP->>Logger: log "fetched props"
FP-->>Caller: return props
else Protocol-2 fails or empty props
P2-->>FP: error or missing/empty props
FP->>Logger: warn about protocol-2 failure
FP->>P1: send IQ props request (protocol: '1', maybe lastPropHash)
alt Protocol-1 returns valid props
P1-->>FP: props node
FP->>FP: reduce props -> dict
FP->>Creds: update lastPropHash (if present)
FP->>Evt: emit creds.update (if hash present)
FP->>Logger: log "fetched props"
FP-->>Caller: return props
else Protocol-1 fails or empty props
P1-->>FP: error or missing/empty props
FP->>Logger: warn about protocol-1 failure
FP-->>Caller: return {}
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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)
Comment |
![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 (1)
src/Socket/chats.ts (1)
882-885: Emit only the changed credential fields.
creds.updateis used as a partial-update event everywhere else in this file. Sending the fullauthState.credsobject here leaks a live mutable reference across the event bus for a single-field change and makes downstream persistence do more work than necessary.Suggested fix
- authState.creds.lastPropHash = propsNode2.attrs.hash; - ev.emit('creds.update', authState.creds); + authState.creds.lastPropHash = propsNode2.attrs.hash; + ev.emit('creds.update', { lastPropHash: propsNode2.attrs.hash });- authState.creds.lastPropHash = propsNode1.attrs.hash; - ev.emit('creds.update', authState.creds); + authState.creds.lastPropHash = propsNode1.attrs.hash; + ev.emit('creds.update', { lastPropHash: propsNode1.attrs.hash });Also applies to: 918-921
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Socket/chats.ts` around lines 882 - 885, The emit currently sends the full mutable authState.creds object; change the ev.emit('creds.update', authState.creds) calls to emit only the changed fields as a partial object (e.g., after setting authState.creds.lastPropHash = propsNode2.attrs.hash, call ev.emit('creds.update', { lastPropHash: propsNode2.attrs.hash })); apply the same pattern to the other similar emit (the one that updates a single creds field around the later block) so callers receive only the changed key(s) and we avoid leaking a live creds reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Socket/chats.ts`:
- Around line 879-888: If the ABT protocol call (query()) succeeds but
getBinaryNodeChild(resultNode2, 'props') is missing or
reduceBinaryNodeToDictionary(propsNode2, 'prop') yields an empty object, fall
back to the legacy protocol:'1' path instead of returning an empty result;
update the logic around propsNode2 handling in the function that calls query()
so that after calling getBinaryNodeChild you check for both a missing propsNode2
and an empty props object, only set authState.creds.lastPropHash and emit
creds.update when propsNode2.attrs.hash exists, and invoke the existing legacy
code path (the same code used when query() throws) when props are absent/empty
to ensure the fallback path runs.
---
Nitpick comments:
In `@src/Socket/chats.ts`:
- Around line 882-885: The emit currently sends the full mutable authState.creds
object; change the ev.emit('creds.update', authState.creds) calls to emit only
the changed fields as a partial object (e.g., after setting
authState.creds.lastPropHash = propsNode2.attrs.hash, call
ev.emit('creds.update', { lastPropHash: propsNode2.attrs.hash })); apply the
same pattern to the other similar emit (the one that updates a single creds
field around the later block) so callers receive only the changed key(s) and we
avoid leaking a live creds reference.
🪄 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: 24feedf8-a3f5-41e0-bdae-c7e979ab4f9a
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
src/Socket/chats.ts
ojuolokun86
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Socket/chats.ts`:
- Around line 951-954: The current fallback hides the legacy (protocol-1) error
because logger.warn doesn't include error objects and the code rethrows
firstError instead of preserving both failures; update the block around the
protocol fallback handling (references: variables firstError and err1 and the
logger.warn call) to log both errors (e.g., logger.error or logger.warn
including err1 and firstError) and throw an aggregated error that preserves both
failures (e.g., create a new Error whose message includes both err1 and
firstError details or attach one as the cause/property) so callers can see full
failure context.
🪄 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: 882807d3-9bf1-4490-b541-9b8f3f08605b
📒 Files selected for processing (2)
.gitignoresrc/Socket/chats.ts
✅ Files skipped from review due to trivial changes (1)
- .gitignore
Comment on lines
+951
to
+954
| logger.warn('⚠️ both protocol 1 and 2 failed'); | ||
|
|
||
| // 🔥 IMPORTANT: throw real error instead of silent {} | ||
| throw firstError || err1; |
There was a problem hiding this comment.
Don't lose the protocol-1 failure details.
Line 954 will always rethrow the outer firstError, and Line 951 logs no error objects. If both requests fail, the actual legacy failure is hidden, which makes this fallback hard to debug.
Suggested change
- } catch (err1) {
- logger.warn('⚠️ both protocol 1 and 2 failed');
-
- // 🔥 IMPORTANT: throw real error instead of silent {}
- throw firstError || err1;
+ } catch (err1) {
+ logger.warn({ protocol2Error: firstError, protocol1Error: err1 }, 'both protocol 1 and 2 failed');
+ throw err1;
}📝 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.
Suggested change
| logger.warn('⚠️ both protocol 1 and 2 failed'); | |
| // 🔥 IMPORTANT: throw real error instead of silent {} | |
| throw firstError || err1; | |
| logger.warn({ protocol2Error: firstError, protocol1Error: err1 }, 'both protocol 1 and 2 failed'); | |
| throw err1; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Socket/chats.ts` around lines 951 - 954, The current fallback hides the
legacy (protocol-1) error because logger.warn doesn't include error objects and
the code rethrows firstError instead of preserving both failures; update the
block around the protocol fallback handling (references: variables firstError
and err1 and the logger.warn call) to log both errors (e.g., logger.error or
logger.warn including err1 and firstError) and throw an aggregated error that
preserves both failures (e.g., create a new Error whose message includes both
err1 and firstError details or attach one as the cause/property) so callers can
see full failure context.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Socket/chats.ts (1)
907-908:⚠️ Potential issue | 🟡 MinorInclude both caught errors in logs before returning
{}.Line 907 and Line 951 currently log generic warnings without error objects, which makes protocol fallback failures hard to diagnose.
Suggested logging fix
- logger.warn('⚠️ protocol 2 failed or invalid, trying protocol 1'); + logger.warn({ protocol2Error: err2 }, 'protocol 2 failed or invalid, trying protocol 1'); ... - logger.warn('⚠️ both protocol 1 and 2 failed'); + logger.warn({ protocol2Error: firstError, protocol1Error: err1 }, 'both protocol 1 and 2 failed'); return {};Also applies to: 951-954
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Socket/chats.ts` around lines 907 - 908, The current fallback logic logs only generic warnings (logger.warn('⚠️ protocol 2 failed or invalid, trying protocol 1') and the similar message at the second fallback) and then returns {} without logging the caught error objects; update the catch blocks around the protocol parsing code (the places that call logger.warn for "protocol 2 failed" and the later fallback) to include the caught Error objects in the log (e.g., logger.warn('⚠️ protocol 2 failed or invalid, trying protocol 1', err) and similarly for the protocol 1 fallback) so both exceptions are recorded before returning {}.
🤖 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/Socket/chats.ts`:
- Around line 907-908: The current fallback logic logs only generic warnings
(logger.warn('⚠️ protocol 2 failed or invalid, trying protocol 1') and the
similar message at the second fallback) and then returns {} without logging the
caught error objects; update the catch blocks around the protocol parsing code
(the places that call logger.warn for "protocol 2 failed" and the later
fallback) to include the caught Error objects in the log (e.g., logger.warn('⚠️
protocol 2 failed or invalid, trying protocol 1', err) and similarly for the
protocol 1 fallback) so both exceptions are recorded before returning {}.
Contributor
|
This PR is stale because it has been open for 14 days with no activity. Remove the stale label or comment or this will be closed in 14 days |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Improvements
Chores