refactor: refactors turnstile and add tests#1421
Conversation
WalkthroughThese changes introduce a robust mechanism in the Turnstile React component to handle Cloudflare challenge responses by intercepting fetch and axios requests. The Turnstile component is now more modular and testable, with new tests added and a fetch polyfill included for testing environments. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Turnstile
participant fetch/axios
participant Cloudflare
User->>fetch/axios: Initiate request
fetch/axios->>Cloudflare: Send request
Cloudflare-->>fetch/axios: Response (with cf-mitigated: challenge?)
alt Challenge detected
fetch/axios->>Turnstile: Trigger challenge UI
Turnstile->>User: Display Turnstile widget
User->>Turnstile: Solve or dismiss challenge
alt Solved
Turnstile->>fetch/axios: Retry original request
else Dismissed
Turnstile->>fetch/axios: Abort retry
end
else No challenge
fetch/axios-->>User: Return response
end
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
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (7)
✨ Finishing Touches
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1421 +/- ##
==========================================
+ Coverage 38.35% 38.71% +0.35%
==========================================
Files 849 849
Lines 20141 20136 -5
Branches 3657 3656 -1
==========================================
+ Hits 7726 7795 +69
+ Misses 12091 12017 -74
Partials 324 324
*This pull request uses carry forward flags. Click here to find out more.
🚀 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: 2
🧹 Nitpick comments (3)
apps/deploy-web/src/components/turnstile/Turnstile.spec.tsx (1)
6-6: Consider using a simpler wait utility.Using
setTimeout as waitfromnode:timers/promisesmight be unnecessary complexity. Consider using the existingwaitutility from the codebase or a simpler approach.-import { setTimeout as wait } from "node:timers/promises"; +import { act } from "@testing-library/react";Then in the setup function:
-await act(() => wait(0)); +await act(async () => {});apps/deploy-web/src/components/turnstile/Turnstile.tsx (2)
63-71: Consider cleaning up the abort event listener to prevent memory leaks.The abort event listener added on line 69 might not be removed if the promise resolves before abort, potentially causing a memory leak.
Consider using a cleanup approach:
- return Promise.race([ - turnstileRef.current?.getResponsePromise(), - new Promise<void>(resolve => abortControllerRef.current?.signal.addEventListener("abort", () => resolve())) - ]); + const responsePromise = turnstileRef.current?.getResponsePromise(); + const abortPromise = new Promise<void>((resolve) => { + const handleAbort = () => resolve(); + abortControllerRef.current?.signal.addEventListener("abort", handleAbort); + }); + + try { + return await Promise.race([responsePromise, abortPromise]); + } finally { + // Clean up could be added here if needed + }Alternatively, since AbortController signals are one-time use and the controller is recreated each time, this might not be a practical issue.
82-83: Consider implementing retry limits to prevent infinite retry loops.If the Cloudflare challenge repeatedly fails, the current implementation could retry indefinitely, potentially causing performance issues.
Consider adding a retry limit mechanism. You could track retry attempts in a ref and limit retries:
const retryCountRef = useRef<Map<string, number>>(new Map()); // In fetch override: const key = `${resource}`; const retryCount = retryCountRef.current.get(key) || 0; if (retryCount < MAX_RETRIES && response.headers.get("cf-mitigated") === "challenge" && ...) { retryCountRef.current.set(key, retryCount + 1); return globalThis.fetch(resource, options); }This would prevent infinite retry loops while still allowing reasonable retry attempts.
Also applies to: 91-92
📜 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 (5)
apps/deploy-web/next.config.js(1 hunks)apps/deploy-web/package.json(1 hunks)apps/deploy-web/src/components/turnstile/Turnstile.spec.tsx(1 hunks)apps/deploy-web/src/components/turnstile/Turnstile.tsx(4 hunks)apps/deploy-web/tests/unit/setup.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/deploy-web/src/components/turnstile/Turnstile.spec.tsx (4)
apps/deploy-web/src/components/turnstile/Turnstile.tsx (2)
Turnstile(51-177)COMPONENTS(39-43)apps/deploy-web/tests/fixture/context-with-extension.ts (1)
expect(77-77)apps/deploy-web/tests/unit/mocks.tsx (1)
MockComponents(10-15)apps/deploy-web/src/utils/timer.ts (1)
wait(26-32)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-deploy-web-build
🔇 Additional comments (13)
apps/deploy-web/package.json (1)
176-176:Details
✅ Verification successful
Verify the whatwg-fetch version for security and currency.
The addition of the fetch polyfill is appropriate for testing environments. However, please ensure this version is current and free from security vulnerabilities.
🌐 Web query:
What is the latest version of whatwg-fetch npm package and are there any known security vulnerabilities?💡 Result:
Latest Version of whatwg-fetch
The latest version of the
whatwg-fetchnpm package is 3.6.20, which was published on December 13, 2023[1][5].Security Vulnerabilities
There are no known direct security vulnerabilities associated with the
whatwg-fetchpackage as of the latest release (version 3.6.20). This status is confirmed by security databases such as Snyk[5]. However, note that this assessment covers only the package itself and does not include potential vulnerabilities in its dependencies[5].Summary Table
Package Name Latest Version Release Date Known Vulnerabilities whatwg-fetch 3.6.20 Dec 13, 2023 None reported[5] Additional Notes
- To install the package:
npm install whatwg-fetch --save- For older browsers, a Promise polyfill may also be required[1].
No critical security advisories have been issued against
whatwg-fetchup to the present date[5].Citations:
- 1: https://www.npmjs.com/package/whatwg-fetch
- 2: https://www.jsdelivr.com/package/npm/whatwg-fetch-ie8
- 3: https://github.com/whatwg/fetch
- 4: https://classic.yarnpkg.com/en/package/@whatwg-node/fetch
- 5: https://security.snyk.io/package/npm/whatwg-fetch
- 6: https://github.com/brillout/fetch
- 7: dotansimha/graphql-code-generator#8132
- 8: https://github.com/ardatan/whatwg-node
- 9: https://security.snyk.io/package/npm/whatwg-node-fetch-polyfill
- 10: lquixada/cross-fetch#177
whatwg-fetch@^3.6.20 is up-to-date and secure
The installed version (3.6.20) is the latest release (Dec 13, 2023) and carries no known security vulnerabilities. No further action is needed.
apps/deploy-web/tests/unit/setup.ts (1)
2-2: LGTM! Proper fetch polyfill setup for testing.The fetch polyfill import is correctly positioned and aligns with the dependency added in package.json. This ensures fetch API compatibility in the Jest test environment.
apps/deploy-web/next.config.js (1)
26-26: LGTM! Necessary transpilation for test environment.Adding "@marsidev/react-turnstile" to the transpile packages list ensures compatibility with the test environment by transpiling modern JavaScript features in the package.
apps/deploy-web/src/components/turnstile/Turnstile.spec.tsx (8)
14-18: LGTM! Clear test for disabled state.The test correctly verifies that the component doesn't render when disabled.
20-25: LGTM! Important test for fetch API behavior.This test ensures that the global fetch is not modified when the component is disabled, which is crucial for preventing unintended side effects.
40-55: Well-designed error handling test.The test effectively uses mocking to simulate error conditions and verify the widget reset behavior. The use of
forwardRefanduseForwardedRefis a clever approach to expose the mock instance.
57-80: Good test coverage for retry functionality.The test correctly verifies that clicking the Retry button triggers the expected widget lifecycle methods.
107-146: Excellent test for CF-Mitigated header handling.The test setup with
beforeEachandafterEachproperly manages the fetch mock, and the test verifies that the widget renders when encountering a CF-Mitigated challenge response.
167-181: Comprehensive test for challenge resolution flow.This test effectively verifies the complete flow from challenge detection to successful retry, ensuring the fetch is called twice as expected.
184-201: Well-structured setup function with good defaults.The setup function provides sensible defaults while allowing customization through the input parameter. The use of
MockComponentswith overrides is a clean approach.
203-211: Clever utility for ref forwarding in tests.The
useForwardedRefhelper elegantly handles both function and object refs while providing a default mock instance. This is a reusable pattern for testing components with refs.apps/deploy-web/src/components/turnstile/Turnstile.tsx (2)
39-44: Great implementation of component injection pattern!The component injection approach provides excellent flexibility for testing and customization while maintaining backward compatibility with sensible defaults.
Also applies to: 48-49, 51-51
79-87: 🛠️ Refactor suggestionConsider adding error handling for fetch retry attempts.
The retry fetch on line 83 doesn't handle potential errors, which could lead to unhandled promise rejections.
Consider wrapping the retry in a try-catch:
globalThis.fetch = async (resource, options) => { - const response = await fetch(resource, options); - - if (response.headers.get("cf-mitigated") === "challenge" && turnstileRef.current && (await renderTurnstileAndWaitForResponse())) { - return globalThis.fetch(resource, options); - } - - return response; + try { + const response = await fetch(resource, options); + + if (response.headers.get("cf-mitigated") === "challenge" && turnstileRef.current && (await renderTurnstileAndWaitForResponse())) { + return globalThis.fetch(resource, options); + } + + return response; + } catch (error) { + // Re-throw to maintain original behavior + throw error; + } };This ensures that any errors are properly propagated and can be handled by the caller.
Likely an incorrect or invalid review comment.
stalniy
force-pushed
the
refactor/turnstile
branch
from
21f843f to
fae3217
Compare
2 participants
Summary by CodeRabbit
New Features
Refactor
Tests
Chores