fix(sdk/react-native): prevent duplicate requests by flagging fetch-t… - #18
Conversation
…riggered XHR calls
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe React Native client replaces a boolean fetch-state flag with a shared counter, handles fetch success and failure cleanup, and uses the captured state with existing request-key tracking to suppress duplicate XHR reporting. ChangesReact Native network deduplication
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant FetchInterceptor
participant DevConnect
participant originalFetch
participant XHRInterceptor
FetchInterceptor->>DevConnect: increment fetchStackCount
FetchInterceptor->>originalFetch: invoke originalFetch
originalFetch-->>FetchInterceptor: resolve or reject
XHRInterceptor->>DevConnect: read fetchStackCount
DevConnect-->>XHRInterceptor: provide isFetchXhr
XHRInterceptor->>XHRInterceptor: suppress duplicate send/loadend reporting
FetchInterceptor->>DevConnect: decrement fetchStackCount in finally
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a boolean flag isInsideFetch to track when the SDK is executing an internal fetch call, allowing the PatchedXHR implementation to skip duplicate reporting of requests initiated by fetch. The reviewer identified two main issues: re-entrancy issues where nested fetch calls could prematurely reset the boolean flag (suggesting a counter fetchStackCount instead), and a synchronous error handling regression where synchronous exceptions thrown by originalFetch would bypass the main try-catch block and prevent completion events from being sent.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| * own duplicate report if the count is > 0. | ||
| */ | ||
| private fetchInFlight: Map<string, number> = new Map(); | ||
| private isInsideFetch = false; |
There was a problem hiding this comment.
Using a simple boolean flag isInsideFetch can lead to re-entrancy issues if there are nested fetch calls (e.g., inside custom fetch wrappers or interceptors). When a nested fetch completes, it will set isInsideFetch to false, which prematurely clears the flag for the outer fetch call that is still in progress.
Consider using a counter (e.g., fetchStackCount) instead of a boolean flag to safely track nested/concurrent fetch invocations.
| private isInsideFetch = false; | |
| private fetchStackCount = 0; |
| let responsePromise: Promise<Response>; | ||
| dc.isInsideFetch = true; | ||
| try { | ||
| const response = await originalFetch(input, init); | ||
| responsePromise = originalFetch(input, init); | ||
| } finally { | ||
| dc.isInsideFetch = false; | ||
| } |
There was a problem hiding this comment.
There are two issues with this implementation:
- Synchronous Error Handling Regression: If
originalFetchthrows an error synchronously (e.g., due to invalid arguments or synchronous validation in another interceptor), the error will propagate immediately and bypass the maincatchblock starting at line 953. This means theclient:network:request_completeevent will never be sent, leaving the request permanently in a pending state in the UI. - Re-entrancy / Nested Fetches: If a nested
fetchis triggered, thefinallyblock of the nested fetch will setisInsideFetchtofalseprematurely, clearing the flag for the outer fetch.
We can resolve both issues by:
- Wrapping the synchronous call to
originalFetchso that any synchronous exceptions are captured as a rejected promise. - Using a counter (
fetchStackCount) instead of a boolean flag.
let responsePromise: Promise<Response>;
dc.fetchStackCount++;
try {
responsePromise = Promise.resolve(originalFetch(input, init));
} catch (err) {
responsePromise = Promise.reject(err);
} finally {
dc.fetchStackCount--;
}|
|
||
| function PatchedXHR(this: any) { | ||
| const xhr = new OriginalXHR(); | ||
| const isFetchXhr = dc.isInsideFetch; |
There was a problem hiding this comment.
Update this to check the counter fetchStackCount > 0 instead of the boolean isInsideFetch.
| const isFetchXhr = dc.isInsideFetch; | |
| const isFetchXhr = dc.fetchStackCount > 0; |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client_sdks/devconnect-react-native/src/client.ts`:
- Around line 924-933: Update the fetch wrapper around originalFetch and
responsePromise so synchronous exceptions are handled by the existing request
error/finalization flow. Nest the isInsideFetch toggle and originalFetch call
within the try that catches request failures, while retaining the finally reset,
ensuring request_complete error telemetry is emitted and fetchInFlight is
decremented for synchronous throws.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: faf5088c-1838-4cbe-aad1-32cb7d83f84e
📒 Files selected for processing (1)
client_sdks/devconnect-react-native/src/client.ts
…nter for tracking nested requests
Publishes the fetchStackCount counter refactor from PR #18 to npm. The 1.0.7 release shipped before that refactor was merged, so the counter-based dedup was not yet on the registry. Co-Authored-By: Claude <noreply@anthropic.com>
1 participant
…riggered XHR calls
Description
Related Issue
Type of Change
Testing
Screenshots (if applicable)
Summary by CodeRabbit