Conversation
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a KYC identity verification flow: domain types/helpers, API client endpoint, React Query hooks and prefetching, UI atom/icon, modal + styles, gate and identity pages, router integration, translations, and tests. ChangesKYC Identity Verification Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
![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 (2)
packages/widget/src/providers/api/api-client.ts (1)
252-270: ⚖️ Poor tradeoffUnvalidated response cast.
data as KycStatusResulttrusts the payload shape without runtime validation, unlike the schema-backed generated operations. A malformedkycStatuswould flow straight intokycNeedsVerification, which only matches known enum values — an unexpected string would be treated as "no verification needed" downstream. The current gate happens to fail closed on missing data, so impact is limited; consider validatingkycStatusagainstKycStatusonce the contract lands in the generated client.🤖 Prompt for 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. In `@packages/widget/src/providers/api/api-client.ts` around lines 252 - 270, The getKycStatus function currently casts the HTTP response to KycStatusResult without runtime validation; change getKycStatus to validate the parsed response (specifically the kycStatus field) against the KycStatus enum/allowed values before returning—use KycStatus (or a small validator) to accept only known enum values, map valid values into the KycStatusResult, and throw or return a controlled error/fallback for unknown/malformed values so downstream logic like kycNeedsVerification cannot silently treat invalid strings as "no verification needed."packages/widget/src/components/molecules/kyc-iframe-modal/index.tsx (1)
52-58: ⚡ Quick winConsider adding error handling for iframe load failures.
If the iframe fails to load (network error, blocked content, etc.), users will see a blank modal with no feedback. Consider adding an error boundary or
onErrorhandler to provide user feedback.🛡️ Example implementation
+ const [iframeError, setIframeError] = useState(false); + <iframe data-testid="kyc-iframe-modal__iframe" title={title} src={url} allow="camera; microphone; clipboard-write" className={iframe} + onError={() => setIframeError(true)} /> + {iframeError && ( + <Box p="4"> + <Text>Failed to load verification page. Please try again.</Text> + </Box> + )}🤖 Prompt for 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. In `@packages/widget/src/components/molecules/kyc-iframe-modal/index.tsx` around lines 52 - 58, The iframe currently rendered in KycIframeModal (the <iframe data-testid="kyc-iframe-modal__iframe" title={title} src={url} className={iframe} />) has no failure handling; add an onError handler and local error state in the KycIframeModal component to detect load failures and render a fallback UI (error message + retry or close) instead of a blank modal; also consider adding an onLoad/onLoadStart and a loading state to show a spinner while the iframe loads and include the error state in any telemetry/logging.
🤖 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 `@packages/widget/src/components/molecules/kyc-iframe-modal/index.tsx`:
- Around line 47-49: The close button rendered as <Box as="button" onClick={()
=> onOpenChange(false)}> with <XIcon /> has no accessible label; add an
accessible name by adding an aria-label (e.g., aria-label="Close" or
aria-label={t('close')} if using i18n) or include visually-hidden text inside
the button so screen readers announce its purpose; ensure the attribute is
applied to the same element using Box (the button element) that calls
onOpenChange and that any existing XIcon remains decorative (aria-hidden="true")
if applicable.
---
Nitpick comments:
In `@packages/widget/src/components/molecules/kyc-iframe-modal/index.tsx`:
- Around line 52-58: The iframe currently rendered in KycIframeModal (the
<iframe data-testid="kyc-iframe-modal__iframe" title={title} src={url}
className={iframe} />) has no failure handling; add an onError handler and local
error state in the KycIframeModal component to detect load failures and render a
fallback UI (error message + retry or close) instead of a blank modal; also
consider adding an onLoad/onLoadStart and a loading state to show a spinner
while the iframe loads and include the error state in any telemetry/logging.
In `@packages/widget/src/providers/api/api-client.ts`:
- Around line 252-270: The getKycStatus function currently casts the HTTP
response to KycStatusResult without runtime validation; change getKycStatus to
validate the parsed response (specifically the kycStatus field) against the
KycStatus enum/allowed values before returning—use KycStatus (or a small
validator) to accept only known enum values, map valid values into the
KycStatusResult, and throw or return a controlled error/fallback for
unknown/malformed values so downstream logic like kycNeedsVerification cannot
silently treat invalid strings as "no verification needed."
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 754fdc11-f0ea-415b-a05b-741de7d27146
📒 Files selected for processing (15)
packages/widget/src/Widget.tsxpackages/widget/src/components/atoms/icons/shield-check.tsxpackages/widget/src/components/molecules/kyc-iframe-modal/index.tsxpackages/widget/src/components/molecules/kyc-iframe-modal/styles.css.tspackages/widget/src/domain/types/kyc.tspackages/widget/src/hooks/api/use-kyc-status.tspackages/widget/src/pages/details/earn-page/state/earn-page-context.tsxpackages/widget/src/pages/kyc/identity-verification.page.tsxpackages/widget/src/pages/kyc/kyc-gate.page.tsxpackages/widget/src/providers/api/api-client.tspackages/widget/src/translation/English/translations.jsonpackages/widget/src/translation/French/translations.jsonpackages/widget/tests/use-cases/kyc-flow/kyc-flow.test.tsxpackages/widget/tests/use-cases/kyc-flow/kyc-helpers.test.tspackages/widget/tests/use-cases/kyc-flow/setup.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/widget/src/domain/types/kyc.ts (1)
17-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore the exported KYC types.
Line 17 and Line 54 remove
exportfrom types that were previously part of this module's public surface. That is a source-compatible break for any caller importingKycAccreditation,KycSubjectType,YieldKycEligibility, orYieldKycRequirement, and it also makes the return type ofgetYieldKycRequirementharder to consume by name. Keep these exported unless this PR is intentionally shipping a breaking API change.Suggested fix
-enum KycAccreditation { +export enum KycAccreditation { Retail = "retail", QualifiedPurchaser = "qualified_purchaser", Accredited = "accredited", } -enum KycSubjectType { +export enum KycSubjectType { Kyc = "KYC", Kyb = "KYB", } -interface YieldKycEligibility { +export interface YieldKycEligibility { countries?: string[]; usPersonAllowed?: boolean; accreditation?: KycAccreditation; subjectType?: KycSubjectType; } -type YieldKycRequirement = { +export type YieldKycRequirement = { required: boolean; kyc?: YieldKycMetadata; legacyKycUrl?: string; };Also applies to: 54-58
🤖 Prompt for 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. In `@packages/widget/src/domain/types/kyc.ts` around lines 17 - 33, The PR removed exports for public KYC types causing a breaking change; restore the exports for KycAccreditation, KycSubjectType, YieldKycEligibility and YieldKycRequirement so consumers can import them by name and so getYieldKycRequirement's return type remains referencable; locate the enum/type declarations (KycAccreditation, KycSubjectType, YieldKycEligibility, YieldKycRequirement) and add the export keyword back to each declaration and ensure any corresponding exports in index barrels are kept in sync.packages/widget/src/components/molecules/kyc-iframe-modal/index.tsx (1)
10-15:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
KycIframeModalPropsexported to avoid a public TS API break
KycIframeModalPropsinpackages/widget/src/components/molecules/kyc-iframe-modal/index.tsxis now a non-exportedtype, so any consumer doingimport type { KycIframeModalProps } ...will fail to compile. It’s also not imported anywhere in this repo, so re-exporting it (or explicitly documenting this as an intentional breaking change) is the safer default.🤖 Prompt for 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. In `@packages/widget/src/components/molecules/kyc-iframe-modal/index.tsx` around lines 10 - 15, KycIframeModalProps was made non-exported and will break consumers importing the type; make it part of the public API by exporting it (e.g., change the declaration to "export type KycIframeModalProps = { ... }") in the KycIframeModal component file and ensure any package-level re-exports (barrel/index export) also include KycIframeModalProps so downstream "import type { KycIframeModalProps }" continues to work.
🤖 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.
Outside diff comments:
In `@packages/widget/src/components/molecules/kyc-iframe-modal/index.tsx`:
- Around line 10-15: KycIframeModalProps was made non-exported and will break
consumers importing the type; make it part of the public API by exporting it
(e.g., change the declaration to "export type KycIframeModalProps = { ... }") in
the KycIframeModal component file and ensure any package-level re-exports
(barrel/index export) also include KycIframeModalProps so downstream "import
type { KycIframeModalProps }" continues to work.
In `@packages/widget/src/domain/types/kyc.ts`:
- Around line 17-33: The PR removed exports for public KYC types causing a
breaking change; restore the exports for KycAccreditation, KycSubjectType,
YieldKycEligibility and YieldKycRequirement so consumers can import them by name
and so getYieldKycRequirement's return type remains referencable; locate the
enum/type declarations (KycAccreditation, KycSubjectType, YieldKycEligibility,
YieldKycRequirement) and add the export keyword back to each declaration and
ensure any corresponding exports in index barrels are kept in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a3f132fe-ee4f-430b-8f12-7e2887dfc8ed
📒 Files selected for processing (2)
packages/widget/src/components/molecules/kyc-iframe-modal/index.tsxpackages/widget/src/domain/types/kyc.ts
…frame accessibility
1 participant
Added
/kycgate route that resolves the status and either continues to/review(approved/not_required) or renders the verification screen (not_started/pending/rejected) and fails closed if the status can't be read.getKycStatus(yieldId, address)on the API client and auseKycStatushook, with a non-blocking background prefetch when a KYC-requiring yield is selected.getYieldKycRequirement/kycNeedsVerification/resolveKycUrl.Changed
/kycfor KYC-required yields instead of going straight to/review, and prefetches KYC status on yield selection so the gate decision is instant.getKycStatusmethod that runs through the configured yields client (manually added currently)./kycroute inside the connected stake flow.Summary by CodeRabbit
New Features
Localization
Tests