feat: reCAPTCHA 렌더 안정화 및 결과 페이지 404 예외 상태 처리#34
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 35 minutes and 53 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughreCAPTCHA Enterprise 통합을 위해 새로운 로딩 및 렌더링 모듈을 추가하고, 캡차 위젯을 리팩토링했습니다. 결과 페이지에서 상세 정보를 사용할 수 없는 경우를 처리하는 기능을 추가하고, 관련 UI와 타입을 업데이트했습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant CaptchaWidget
participant recaptchaEnterprise
participant window as window.grecaptcha<br/>.enterprise
participant Container as DOM Container
CaptchaWidget->>recaptchaEnterprise: renderRecaptchaEnterprise(container, options)
activate recaptchaEnterprise
recaptchaEnterprise->>recaptchaEnterprise: loadRecaptchaEnterprise()
activate recaptchaEnterprise
recaptchaEnterprise->>recaptchaEnterprise: Create script element<br/>google.com/recaptcha/enterprise.js
recaptchaEnterprise->>recaptchaEnterprise: Add to document head
recaptchaEnterprise->>window: Wait for load event<br/>+ grecaptcha.enterprise.render
window-->>recaptchaEnterprise: API ready
deactivate recaptchaEnterprise
recaptchaEnterprise->>recaptchaEnterprise: Check AbortSignal state
alt Signal aborted
recaptchaEnterprise-->>CaptchaWidget: Reject with RECAPTCHA_RENDER_ABORTED
else Signal not aborted
recaptchaEnterprise->>window: render(container, {siteKey, theme, callbacks})
window->>Container: Render widget UI
window-->>recaptchaEnterprise: Widget ID
recaptchaEnterprise-->>CaptchaWidget: Resolve with widget ID
end
deactivate recaptchaEnterprise
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![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
🧹 Nitpick comments (4)
src/pages/Captcha/lib/recaptchaEnterprise.ts (1)
93-108: 기존 스크립트 경로의 드문 레이스에 대한 방어 제안
dataset.loadStatus가'loaded'/'error'둘 다 아닌 상태(= 아직loading)에서waitForScriptReady(existingScript)로 진입하는데, 그 사이 원본 스크립트의load/error가 이미 발생·처리되어dataset.loadStatus가 설정됐을 가능성이 존재합니다. 이 경우 새로 붙인addEventListener는 영영 발화되지 않아waitForEnterpriseApiReady의 5s 타임아웃까지 대기하게 됩니다. 리스너 부착 직후 dataset을 한 번 더 확인해 짧은 경로로 빠지도록 가드를 추가하면 안전합니다.♻️ 제안 수정 diff
function waitForScriptReady(script: HTMLScriptElement): Promise<void> { return new Promise((resolve, reject) => { const onLoad = () => { void waitForEnterpriseApiReady().then(resolve).catch(reject); }; const onError = () => { reject(new Error('Failed to load reCAPTCHA Enterprise script.')); }; script.addEventListener('load', onLoad, { once: true }); script.addEventListener('error', onError, { once: true }); + + // 리스너 부착 사이에 load/error가 이미 발생·반영된 경우 복구 + if (script.dataset.loadStatus === 'loaded') { + onLoad(); + } else if (script.dataset.loadStatus === 'error') { + onError(); + } }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Captcha/lib/recaptchaEnterprise.ts` around lines 93 - 108, In loadEnterpriseScriptBySource, guard against the rare race where existingScript.dataset.loadStatus is still neither 'loaded' nor 'error' when you attach listeners: after you attach the load/error listeners to existingScript (the same element used with waitForScriptReady), immediately re-check existingScript.dataset.loadStatus and short-circuit — if it's 'loaded' or isEnterpriseApiReady() then await waitForEnterpriseApiReady() and return; if it's 'error' throw the same Error('Failed to load reCAPTCHA Enterprise script.'); otherwise proceed to await waitForScriptReady(existingScript).src/pages/Result/api/createResultPageFetcher.ts (2)
32-42: 안내 문구 문자열 중복 — 공용 상수로 추출 권장.동일한 한국어 문구가 이 파일(L36),
ResultPage.tsx(L80 hero description), 그리고 테스트 기대값에 세 번 중복되어 있습니다. 문구가 변경될 때 한 곳이라도 누락되면 UI와 API/테스트 간 불일치가 발생합니다. 공용 상수로 추출해 단일 출처로 유지하세요.♻️ 제안 리팩터 예시
+// src/pages/Result/constants/detailUnavailable.ts +export const DETAIL_UNAVAILABLE_MESSAGE = + '상세 분석 데이터를 찾지 못했습니다. 잠시 후 다시 시도하거나 다시 검사해 주세요.';-function createDetailUnavailableResultPageData(url: string): ResultPageData { - return { - detailUnavailable: true, - previewUrl: url, - siteMeta: '상세 분석 데이터를 찾지 못했습니다. 잠시 후 다시 시도하거나 다시 검사해 주세요.', +import { DETAIL_UNAVAILABLE_MESSAGE } from '../constants/detailUnavailable'; + +function createDetailUnavailableResultPageData(url: string): ResultPageData { + return { + detailUnavailable: true, + previewUrl: url, + siteMeta: DETAIL_UNAVAILABLE_MESSAGE,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Result/api/createResultPageFetcher.ts` around lines 32 - 42, 현재 createDetailUnavailableResultPageData 함수의 siteMeta 문자열이 ResultPage.tsx의 hero description 및 테스트 기대값과 중복되어 있으므로, 이 문구를 공용 상수로 추출하여 단일 소스로 유지하도록 리팩터하세요: 새로운 상수(EXPORTed const, 예: DETAIL_UNAVAILABLE_MESSAGE)를 적절한 모듈에 생성한 뒤 createDetailUnavailableResultPageData 내 siteMeta에 상수를 사용하고 ResultPage.tsx의 hero description과 테스트들에서 해당 상수를 import하여 사용하도록 변경하며, 테스트 기대값도 상수를 참조하도록 업데이트하여 중복된 리터럴을 제거하세요.
50-64: 타입 좁히기로?? ''폴백 제거 권장.해당
try/catch는hasRecoverableUrl이 참일 때만 진입하므로recoverableUrl은 이 시점에 항상string입니다. 그럼에도 L60의recoverableUrl ?? ''는 TS 타입 좁힘이 안 되는 것을 방어하기 위한 폴백인데, 빈 문자열이detailUnavailable데이터의siteName/siteUrl/visitUrl등으로 흘러 들어가면 "다시 검사하기" 동작이 빈 URL 기반으로 수행되어 혼선을 일으킬 수 있습니다. 조건문에서recoverableUrl자체로 판정해 좁히거나, 진입 가드에서 바로 변수로 보존하세요.♻️ 제안 리팩터
- const recoverableUrl = resolveRecoverableUrl(session); - const hasRecoverableUrl = Boolean(recoverableUrl); - - if (!session.analysisDetail && hasRecoverableUrl) { + const recoverableUrl = resolveRecoverableUrl(session); + + if (!session.analysisDetail && recoverableUrl) { try { const detailSession = await ensureScanDetail(); return toResultPageData(detailSession, tone); } catch (error) { if (isApiError(error) && error.statusCode === 404) { if (hasSessionResult(session)) { return toResultPageData(session, tone); } - return createDetailUnavailableResultPageData(recoverableUrl ?? ''); + return createDetailUnavailableResultPageData(recoverableUrl); } throw error; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Result/api/createResultPageFetcher.ts` around lines 50 - 64, The catch block uses recoverableUrl with a defensive `?? ''` fallback which masks that recoverableUrl is actually guaranteed when hasRecoverableUrl is true; remove the `?? ''` fallback and narrow/capture recoverableUrl before the try so TypeScript knows it's a string — e.g., check/assign recoverableUrl when computing hasRecoverableUrl (or add an explicit guard `if (!recoverableUrl) throw`) so inside the catch you can call createDetailUnavailableResultPageData(recoverableUrl) directly; update references in this async flow (ensureScanDetail, toResultPageData, createDetailUnavailableResultPageData, hasSessionResult) accordingly.src/pages/Result/api/createResultPageFetcher.test.ts (1)
21-66: 테스트 시나리오 커버리지 양호 — 추가 보완 제안.404 → 세션 폴백, 404 →
detailUnavailable두 핵심 분기를 모두 검증하여 PR 목적을 잘 반영합니다. 필요 시 다음 분기도 추가 고려해볼 만합니다:
ensureScanDetail이 비‑404ApiError(예: 500)를 던지는 경우 → 에러가 그대로 전파되는지.!session.analysisDetail && !hasRecoverableUrl상태 + 세션 결과 없음 →SCAN_SESSION_REQUIRED에러 확인.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Result/api/createResultPageFetcher.test.ts` around lines 21 - 66, Add two tests for createResultPageFetcher: (1) mock ensureScanDetail to reject with a non-404 ApiError (e.g. statusCode 500) and assert fetchResultPageData propagates/throws that error (use vi.mocked(ensureScanDetail).mockRejectedValue(...) and expect(fetchResultPageData()).rejects.toThrow(...)); (2) simulate the case where there is no session.analysisDetail, no recoverable URL (mock resolveRequestedUrlFromSearch to return null/undefined), and no session final result, then call fetchResultPageData and assert it throws the SCAN_SESSION_REQUIRED error (or the specific error type/message your implementation uses). Reference createResultPageFetcher, ensureScanDetail, resolveRequestedUrlFromSearch, and the SCAN_SESSION_REQUIRED error in the new tests.
🤖 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/pages/Captcha/ui/CaptchaWidget.tsx`:
- Around line 47-49: The promise resolution in
renderRecaptchaEnterprise(...).then(...) only assigns widgetId, which can leak a
widget if the effect's cleanup already ran; update the .then handler to check
the disposed flag (or other cleanup indicator used in this component) when the
promise resolves and, if disposed is true, immediately call
resetRecaptchaEnterprise with the returned widgetId instead of just assigning
it, otherwise store widgetId as before; ensure the handler references the same
widgetId/ disposed symbols used in the surrounding effect so
resetRecaptchaEnterprise(widgetId) runs when necessary.
In `@src/pages/Result/api/createResultPageFetcher.test.ts`:
- Around line 16-19: The test teardown uses vi.clearAllMocks() which only clears
mock call history but leaves mockReturnValue/mockImplementation in place,
causing leaks (e.g., resolveRequestedUrlFromSearch.mockReturnValue in later
tests); update the beforeEach to call vi.resetAllMocks() instead so all mocks
(return values and implementations) are restored to their factory defaults,
keeping tests isolated alongside the existing
useScanSessionStore.getState().clearSession() call.
In `@src/pages/Result/ResultPage.tsx`:
- Around line 96-102: Update the statusLabel string on the siteCard constant in
ResultPage.tsx from the English "ANALYSIS NEEDED" to a Korean label to match the
page's UX tone (e.g., "분석 대기" or "정보 부족"); edit the siteCard object (referencing
siteCard, statusLabel, tone, visitLabel) so the value is the chosen Korean text
and keep the object typed as const.
---
Nitpick comments:
In `@src/pages/Captcha/lib/recaptchaEnterprise.ts`:
- Around line 93-108: In loadEnterpriseScriptBySource, guard against the rare
race where existingScript.dataset.loadStatus is still neither 'loaded' nor
'error' when you attach listeners: after you attach the load/error listeners to
existingScript (the same element used with waitForScriptReady), immediately
re-check existingScript.dataset.loadStatus and short-circuit — if it's 'loaded'
or isEnterpriseApiReady() then await waitForEnterpriseApiReady() and return; if
it's 'error' throw the same Error('Failed to load reCAPTCHA Enterprise
script.'); otherwise proceed to await waitForScriptReady(existingScript).
In `@src/pages/Result/api/createResultPageFetcher.test.ts`:
- Around line 21-66: Add two tests for createResultPageFetcher: (1) mock
ensureScanDetail to reject with a non-404 ApiError (e.g. statusCode 500) and
assert fetchResultPageData propagates/throws that error (use
vi.mocked(ensureScanDetail).mockRejectedValue(...) and
expect(fetchResultPageData()).rejects.toThrow(...)); (2) simulate the case where
there is no session.analysisDetail, no recoverable URL (mock
resolveRequestedUrlFromSearch to return null/undefined), and no session final
result, then call fetchResultPageData and assert it throws the
SCAN_SESSION_REQUIRED error (or the specific error type/message your
implementation uses). Reference createResultPageFetcher, ensureScanDetail,
resolveRequestedUrlFromSearch, and the SCAN_SESSION_REQUIRED error in the new
tests.
In `@src/pages/Result/api/createResultPageFetcher.ts`:
- Around line 32-42: 현재 createDetailUnavailableResultPageData 함수의 siteMeta 문자열이
ResultPage.tsx의 hero description 및 테스트 기대값과 중복되어 있으므로, 이 문구를 공용 상수로 추출하여 단일 소스로
유지하도록 리팩터하세요: 새로운 상수(EXPORTed const, 예: DETAIL_UNAVAILABLE_MESSAGE)를 적절한 모듈에 생성한
뒤 createDetailUnavailableResultPageData 내 siteMeta에 상수를 사용하고 ResultPage.tsx의
hero description과 테스트들에서 해당 상수를 import하여 사용하도록 변경하며, 테스트 기대값도 상수를 참조하도록 업데이트하여
중복된 리터럴을 제거하세요.
- Around line 50-64: The catch block uses recoverableUrl with a defensive `??
''` fallback which masks that recoverableUrl is actually guaranteed when
hasRecoverableUrl is true; remove the `?? ''` fallback and narrow/capture
recoverableUrl before the try so TypeScript knows it's a string — e.g.,
check/assign recoverableUrl when computing hasRecoverableUrl (or add an explicit
guard `if (!recoverableUrl) throw`) so inside the catch you can call
createDetailUnavailableResultPageData(recoverableUrl) directly; update
references in this async flow (ensureScanDetail, toResultPageData,
createDetailUnavailableResultPageData, hasSessionResult) accordingly.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 49050c24-233a-45e9-b1e2-4299f760a7dd
📒 Files selected for processing (12)
.env.examplescripts/setupEnv.mjssrc/pages/Captcha/hooks/useCaptchaPage.tssrc/pages/Captcha/lib/recaptchaEnterprise.tssrc/pages/Captcha/styles/captchaPage.css.tssrc/pages/Captcha/ui/CaptchaWidget.tsxsrc/pages/Result/ResultPage.tsxsrc/pages/Result/api/createResultPageFetcher.test.tssrc/pages/Result/api/createResultPageFetcher.tssrc/pages/Result/lib/toResultPageData.tssrc/pages/Result/types/resultPage.types.tssrc/pages/Result/ui/ResultStatusPage.tsx
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
1 participant
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
#33
google.com실패 시recaptcha.net폴백 추가/api/v1/scan/detail404를 일반 에러가 아닌 예외 상태로 처리했습니다.detailUnavailable상태 데이터 반환Tasks
CaptchaWidget,useCaptchaPage,recaptchaEnterprise렌더 안정화createResultPageFetcher에 404 예외 상태 처리 로직 추가ResultPage,ResultStatusPage에detailUnavailableUI 분기 추가ResultPageData타입 확장 (detailUnavailable)createResultPageFetcher.test.ts)Summary by CodeRabbit
릴리스 노트
새로운 기능
버그 수정
스타일