fix: resolve all lint issues and setup husky hooks#91
Merged
Benjtalkshow merged 3 commits intoboundlessfi:mainfrom Feb 20, 2026
Merged
fix: resolve all lint issues and setup husky hooks#91Benjtalkshow merged 3 commits intoboundlessfi:mainfrom
Benjtalkshow merged 3 commits intoboundlessfi:mainfrom
Conversation
|
@Benjtalkshow is attempting to deploy a commit to the Threadflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds Husky and lint-staged tooling; tightens API route signatures and request validation; expands Compliance and GeoRestriction services with new public APIs and state mutations; refactors withdrawal UI and compliance dialogs (tier upgrade, document upload); and updates formatting/lint configs and dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as WithdrawalSection
participant API as POST /api/withdrawal/submit
participant WithdrawalService
participant Compliance as ComplianceService
User->>UI: fill amount, currency, destination → submit
UI->>API: POST { amount, currency, destinationId, ip }
API->>WithdrawalService: submit(userId, amount, currency, destinationId, ip)
WithdrawalService->>Compliance: validateWithdrawalAmount(userId, amount)
Compliance-->>WithdrawalService: validationResult
alt validation ok
WithdrawalService->>Compliance: trackWithdrawal(userId, amount)
Compliance-->>WithdrawalService: tracked
WithdrawalService-->>API: success payload
else validation failed
WithdrawalService-->>API: error (status, message)
end
API-->>UI: JSON response
UI-->>User: show success or error
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (16)
components/wallet/withdrawal-section.tsx (1)
78-92:handleWithdrawlacks the compliance guards thatcanWithdrawenforces.The button is disabled by
canWithdraw, buthandleWithdrawitself only checksisValidAmountandcomplianceDataexistence. If the button's disabled state is ever bypassed (e.g., programmatic call, future refactor), withdrawals could be submitted during a hold or with unaccepted terms. Consider mirroring thecanWithdrawcheck at the top of this function.Also,
alert()for success/failure is fine as a placeholder but should be replaced with a toast/notification system before release.Proposed fix
const handleWithdraw = async () => { - if (!isValidAmount || !complianceData) return; + if (!canWithdraw || !complianceData) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/wallet/withdrawal-section.tsx` around lines 78 - 92, handleWithdraw currently only checks isValidAmount and complianceData but not the full set of guards enforced by canWithdraw; add the same guard at the top of handleWithdraw (preferably call canWithdraw and return early if false) so programmatic calls cannot bypass hold/terms checks, then proceed with submitMutation.mutateAsync using parsedAmount, "USD", and bankAccounts[0].id; on success call setAmount("") and trigger the existing success notification (later replace alert with toast/notification system). Ensure you reference the existing symbols handleWithdraw, canWithdraw, isValidAmount, complianceData, parsedAmount, bankAccounts, submitMutation, and setAmount when making the change.components/compliance/terms-dialog.tsx (1)
51-56: Usee.currentTargetinstead ofe.target as HTMLDivElement.
e.targetis the innermost element that received the event (could be a child<p>inside the scrollable div), whilee.currentTargetis always the element the handler is attached to.React.UIEvent<HTMLDivElement>typescurrentTargetasHTMLDivElementdirectly, so no cast is needed.♻️ Proposed fix
const handleScroll = (e: React.UIEvent<HTMLDivElement>) => { - const target = e.target as HTMLDivElement; - const bottom = - target.scrollHeight - target.scrollTop <= target.clientHeight + 50; + const { scrollHeight, scrollTop, clientHeight } = e.currentTarget; + const bottom = scrollHeight - scrollTop <= clientHeight + 50; if (bottom) setScrolled(true); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/compliance/terms-dialog.tsx` around lines 51 - 56, The scroll handler handleScroll uses e.target cast to HTMLDivElement which can be a child element; replace that with e.currentTarget (typed as HTMLDivElement) to reliably refer to the scrollable div and remove the unnecessary cast; update the bottom calculation to use currentTarget.scrollHeight, currentTarget.scrollTop, and currentTarget.clientHeight and keep the setScrolled(true) behavior.app/api/compliance/status/route.ts (1)
6-7: Drop the unused_requestparameter entirely instead of suppressing the lint warning.Next.js App Router route handlers do not require declaring the
requestparameter if it is unused. Omitting it removes both the eslint-disable comment and theNextRequestimport, which is cleaner than the current double-suppression approach.♻️ Proposed fix
-import { NextRequest, NextResponse } from "next/server"; +import { NextResponse } from "next/server"; import { getCurrentUser } from "@/lib/server-auth"; import { ComplianceService } from "@/lib/services/compliance"; import { TermsService } from "@/lib/services/terms"; -// eslint-disable-next-line `@typescript-eslint/no-unused-vars` -export async function GET(_request: NextRequest) { +export async function GET() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/compliance/status/route.ts` around lines 6 - 7, The GET route handler currently declares an unused parameter and silences linting; remove the unused parameter from the function signature (change "export async function GET(_request: NextRequest)" to "export async function GET()") and delete the corresponding eslint-disable comment and the NextRequest import so the handler is clean and no longer suppresses the rule.package.json (2)
93-93: Pinprettierto an exact version to prevent formatting drift.Prettier docs recommend installing an exact version locally. Even a patch release of Prettier can result in slightly different formatting, causing team members to format each other's changes back and forth.
♻️ Proposed fix
- "prettier": "^3.8.1", + "prettier": "3.8.1",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 93, The prettier dependency in package.json is using a caret range ("prettier": "^3.8.1") which allows patch/minor upgrades and can cause formatting drift; change the dependency value to an exact version string ("prettier": "3.8.1") in package.json (look for the "prettier" entry) and then reinstall/update the lockfile (npm install / yarn install) so the lockfile reflects the pinned version.
12-13: Runningnpm run buildin the pre-commit hook is too heavy for routine commits.Per the AI summary,
.husky/pre-commitexecuteslint-staged --concurrent false && npm run build. A full Next.js build on every commit will significantly slow down developer workflow and will block commits even for trivial changes. Pre-commit hooks should only run fast checks (lint, type-check). Move the build gate to CI instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 12 - 13, Pre-commit currently runs a full Next.js build (via `npm run build`) which is too slow; edit the `.husky/pre-commit` hook to remove the `&& npm run build` step so it only runs fast checks (e.g., `lint-staged --concurrent false` and any quick type-checks), and keep the `prepare`/husky script in package.json unchanged; move the full `npm run build` gate into your CI pipeline (e.g., add a build job in your CI config) so commits remain fast while CI enforces the build.app/api/withdrawal/submit/route.ts (1)
24-29: Preferinstanceof Errorover the unsafe type assertion; consider distinguishing validation vs. server errors.
(error as Error).messageis a bare type assertion that silently producesundefinedif a non-Errorvalue is thrown. The fallback string handles it, but the pattern is fragile. More importantly, every exception—including unexpected failures fromComplianceService,TermsService, orGeoRestrictionService—returns{ status: 400 }, which misclassifies internal server errors.🛡️ Proposed fix
- } catch (error) { + } catch (error: unknown) { console.error("Error submitting withdrawal:", error); + const isClientError = + error instanceof Error && + ["Insufficient balance", "Exceeds", "Account is", "Terms must", "Withdrawals not"].some((s) => + error.message.startsWith(s) + ); return NextResponse.json( - { error: (error as Error).message || "Withdrawal failed" }, - { status: 400 }, + { error: error instanceof Error ? error.message : "Withdrawal failed" }, + { status: isClientError ? 400 : 500 }, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/withdrawal/submit/route.ts` around lines 24 - 29, Replace the unsafe assertion in the catch block with a robust check: if (error instanceof Error) extract message = error.message and log error.stack; derive the HTTP status by preferring an explicit numeric status on the thrown error (e.g., (error as any).status || (error as any).statusCode) and fall back to 400 for known validation errors (check error.name === 'ValidationError' or error.name === 'BadRequestError') or 500 for all other/internal errors; for non-Error throws convert to a string for the message and return status 500. Update the NextResponse.json call to use the computed message and status, and ensure you still call console.error with the full error (stack when available) so failures from ComplianceService, TermsService, or GeoRestrictionService are correctly classified as server errors when appropriate.app/api/compliance/upgrade/route.ts (2)
30-46: Same silent error swallowing in GET as in the terms route.The
GEThandler discards the error (_error), whileapp/api/compliance/status/route.ts(line 26) logs withconsole.error. Standardize error handling across all compliance routes — either always log or use a shared utility.As with the terms route, the
_requestparameter + eslint-disable can be replaced by simply omitting the parameter from the signature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/compliance/upgrade/route.ts` around lines 30 - 46, The GET handler currently swallows errors and keeps an unused _request param; update the GET function to omit the unused parameter, and in the catch block capture the error (e.g., error) and log it consistently (console.error or the shared logging utility used by other compliance routes) before returning the 500 NextResponse.json; locate the GET function, getCurrentUser call, and VerificationService.getVerificationStatus reference to implement the change and mirror the error handling pattern used in app/api/compliance/status/route.ts.
12-12: No validation ontargetTierbefore passing to the service layer.
targetTieris destructured directly from user-supplied JSON with no type or value check. If the body is missing it or provides an invalid tier, the error will surface as an opaque service-layer failure. Consider validating thattargetTieris one of the expectedKYCTiervalues before proceeding.Example validation
const { targetTier } = await request.json(); + + const validTiers = ['BASIC', 'VERIFIED', 'ENHANCED']; + if (!validTiers.includes(targetTier)) { + return NextResponse.json( + { error: "Invalid target tier" }, + { status: 400 }, + ); + } + const verificationRequest =🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/compliance/upgrade/route.ts` at line 12, The handler in route.ts pulls targetTier directly from request.json() without validation; update the POST handler to validate that targetTier exists, is a string/expected type, and matches one of the permitted KYCTier values before calling the service layer (use Object.values(KYCTier) or your validation utility/zod schema to check membership), return a 400/422 response for missing/invalid input, and import/reference the KYCTier enum where validation occurs so the service never receives an unvalidated targetTier.lib/services/compliance.ts (4)
329-331:getAllTierConfigsreturns config objects without any copying — even shallower thangetTierConfig.
Object.values(this.TIER_CONFIGS)returns direct references to the static config objects. Any caller mutation corrupts the shared state. Consider reusinggetTierConfigfor consistency.Proposed fix
static getAllTierConfigs(): KYCTierConfig[] { - return Object.values(this.TIER_CONFIGS); + return (Object.keys(this.TIER_CONFIGS) as KYCTier[]).map((tier) => + this.getTierConfig(tier), + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/compliance.ts` around lines 329 - 331, getAllTierConfigs currently returns direct references from TIER_CONFIGS which allows callers to mutate shared static state; change it to return deep/shallow copies by reusing the existing getTierConfig method for each tier (call getTierConfig(directKey) for each key in TIER_CONFIGS) so callers receive independent config objects and shared state is protected (refer to static members TIER_CONFIGS, getAllTierConfigs, and getTierConfig).
204-237:validateWithdrawalAmounthas a side effect: it resets expired usage windows.A validation/read method should not mutate state. Calling
resetExpiredWindowshere means that simply checking whether a withdrawal is valid can alter the user's usage counters and timestamps. If this is intentional (lazy reset), document it clearly; otherwise, separate the reset from validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/compliance.ts` around lines 204 - 237, validateWithdrawalAmount currently calls resetExpiredWindows and thus mutates state during a pure validation; remove the side-effect by deleting the await this.resetExpiredWindows(userId, compliance) call from validateWithdrawalAmount, leave getUserCompliance and the limit checks as-is, and either (a) require callers to call resetExpiredWindows(userId, compliance) before invoking validateWithdrawalAmount or (b) add a separate method (e.g., validateWithdrawalAmountWithReset) that performs resetExpiredWindows then delegates to validateWithdrawalAmount; also add a brief doc comment to validateWithdrawalAmount stating it does not mutate state and that callers must reset windows beforehand if needed.
322-324:getTierConfigreturns a shallow copy — nestedlimitsandrequirementsare shared references to the static config.Callers mutating the returned object's
limits(e.g.,config.limits.daily = 999) would silently corruptTIER_CONFIGSfor all subsequent reads. Consider a deep copy or at minimum spreading the nestedlimits.Proposed fix
static getTierConfig(tier: KYCTier): KYCTierConfig { - return { ...this.TIER_CONFIGS[tier] }; + const config = this.TIER_CONFIGS[tier]; + return { + ...config, + limits: { ...config.limits }, + requirements: [...config.requirements], + }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/compliance.ts` around lines 322 - 324, getTierConfig currently returns a shallow copy so nested objects like limits and requirements on this.TIER_CONFIGS[tier] are still shared; change getTierConfig to return a deep copy of the config (or at minimum spread/copy nested properties) so mutations to the returned KYCTierConfig do not mutate the static TIER_CONFIGS—e.g., clone the object for the given tier inside getTierConfig (use structuredClone if available or a reliable deep-copy helper) and ensure limits and requirements are copied rather than referenced.
85-94:getUserCompliancereturns a shallow copy — nested objects (limits,usage) are shared with the DB.Methods like
trackWithdrawalandresetExpiredWindowsrely on this shared-reference behavior to mutate the DB through the "copy." This is fragile and will break silently if anyone later changes to a deep copy. For a mock store this is tolerable, but worth a comment noting the intentional shallow semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/compliance.ts` around lines 85 - 94, getUserCompliance currently returns a shallow copy which intentionally preserves shared nested objects like limits and usage so methods such as trackWithdrawal and resetExpiredWindows can mutate the mock DB via that reference; add a clear comment above getUserCompliance (referencing getUserCompliance, MOCK_COMPLIANCE_DB, createDefaultCompliance, trackWithdrawal, and resetExpiredWindows) stating this intentional shallow-copy semantics and warning that converting to a deep copy will break those mutating callers, or alternatively change the implementation to return the actual DB object if you prefer explicit shared-state behavior.lib/services/geo-restriction.ts (2)
42-80:checkLocationalways returns a hardcoded US/CA mock — restriction logic within it is unreachable.The country/state restriction checks on lines 58–77 will never trigger because the mock always returns
countryCode: "US"andregionCode: "CA", neither of which are in theRESTRICTEDlist. This makes the method unable to test restriction behavior. Consider accepting an optional override or parameterizing the mock to enable testing restricted scenarios.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/geo-restriction.ts` around lines 42 - 80, checkLocation currently always returns a hardcoded US/CA mock so RESTRICTED checks never trigger; change checkLocation to accept an optional override (e.g., a Partial<UserLocation> or explicit countryCode/regionCode params) and merge the override into the generated mock before running detectVPN/detectProxy and the RESTRICTED checks, ensuring functions named detectVPN and detectProxy are still awaited and RESTRICTED lookup uses the final countryCode/regionCode; update call sites/tests to pass overrides when simulating restricted scenarios.
82-91: VPN detection uses private IP ranges, not VPN ranges.The comment says "common VPN ranges" but
10.,172.16.,192.168.are RFC 1918 private/LAN address ranges. In practice, traffic arriving at a server would rarely have these source IPs (they'd be NATed). This mock will essentially never flag a VPN from real traffic.Consider updating the comment to reflect that this is a placeholder, or use a more representative mock heuristic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/geo-restriction.ts` around lines 82 - 91, The detectVPN mock incorrectly treats RFC1918 private ranges as "VPN ranges" — update the detectVPN function and its comment to accurately reflect that it's a placeholder and not real VPN detection: change the comment to state this is a simple placeholder/mock and add a TODO suggesting integration with a real VPN detection service (e.g., IPHub, IP2Proxy, VPNApi), and either replace the vpnRanges array with a more representative mock (e.g., a small list of known public VPN provider CIDRs or IPs) or implement a simple heuristic (e.g., check against a configurable list of known VPN ASNs/public IPs); ensure the code and comment clearly reference detectVPN so future maintainers don't confuse private LAN ranges for actual VPN detection.app/api/compliance/terms/route.ts (1)
5-16: GET swallows errors silently while POST logs them — inconsistent error handling.The
GEThandler catches the error into_errorand discards it (line 11), whereas thePOSThandler logs viaconsole.error(line 37). This makes debuggingGETfailures harder. Consider logging the error inGETas well, or using a shared error-handling pattern.Also, since
_requestis unused, you could simply omit the parameter (export async function GET()) instead of adding the eslint-disable comment — Next.js App Router route handlers don't require it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/compliance/terms/route.ts` around lines 5 - 16, The GET route handler currently swallows errors and keeps an unused _request parameter; update export async function GET to remove the unused parameter, and inside the catch for TermsService.getCurrentTermsVersion log the caught error (do not discard _error) before returning NextResponse.json({ error: "Failed to fetch terms" }, { status: 500 }); — use console.error or your app logger to include the error details so failures are visible and consistent with the POST handler..husky/pre-commit (1)
1-1: Running a full build in the pre-commit hook will significantly slow down every commit.
npm run buildfor a Next.js project can take minutes. This is excessive for a pre-commit hook—developers will be tempted to bypass it with--no-verify. Consider moving the build check to CI or to apre-pushhook and keeping onlylint-stagedhere.Proposed fix
-npx lint-staged --concurrent false && npm run build +npx lint-staged --concurrent false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.husky/pre-commit at line 1, The pre-commit hook currently runs a full build ("npx lint-staged --concurrent false && npm run build"), which slows commits; update the .husky/pre-commit hook to only run lint-staged (remove the "&& npm run build" part) and instead add the build step to a pre-push hook (create/update .husky/pre-push to run "npm run build") or enforce the build in CI; locate the string "npx lint-staged --concurrent false && npm run build" in the .husky/pre-commit file and replace it with just the lint-staged invocation, and add a corresponding pre-push/CI job to run "npm run build".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.lintstagedrc:
- Line 2: Install eslint-config-prettier as a devDependency (npm install
--save-dev eslint-config-prettier) and update your flat config in
eslint.config.mjs to import the Prettier flat share (import prettier from
"eslint-config-prettier/flat") and include the imported prettier entry as the
last element in the array passed to defineConfig (after ...nextVitals and
...nextTs) so Prettier disables conflicting ESLint formatting rules; also keep
your lint-staged entry ("*.{js,jsx,ts,tsx}": ["eslint --fix", "prettier
--write"]) as-is in .lintstagedrc.
In `@components/compliance/tier-upgrade-dialog.tsx`:
- Around line 20-38: TierUpgradeDialogProps declares currentTier but
TierUpgradeDialog doesn't destructure or use it; either remove currentTier from
the interface or update the component to accept and use it. Best fix: include
currentTier in the TierUpgradeDialog parameter list (function
TierUpgradeDialog({ open, onOpenChange, currentTier, targetTier }:
TierUpgradeDialogProps)) and add a simple validation/guard using currentTier
(e.g., compare currentTier and targetTier to ensure targetTier is higher and
show an error/disable confirm if not) so callers passing currentTier are not
ignored.
In `@components/wallet/withdrawal-section.tsx`:
- Around line 172-178: The UI currently shows validationError before
syncValidationError which can surface a stale async error; update the rendering
in withdrawal-section.tsx to prefer syncValidationError when present (i.e., use
syncValidationError || validationError) so the AlertDescription displays the
synchronous validation message first; locate the JSX that references
validationError and syncValidationError (inside the Alert / AlertDescription)
and swap the order or conditional so syncValidationError takes display priority.
- Around line 101-107: canWithdraw currently allows submission before the async
validation completes; update its logic to block submission while the server
validation is pending and to ensure validation corresponds to the current
amount. Specifically, include a check for !validateMutation.isPending (or
validateMutation.isSuccess) in the canWithdraw expression and add a tie between
the last-validated amount and parsedAmount (e.g., compare parsedAmount to the
value stored with validateMutation.data or a local lastValidatedAmount) so that
validation must have completed for the current parsedAmount before canWithdraw
becomes true; keep the existing checks (isValidAmount, parsedAmount <=
walletInfo.balance, !validationError, !syncValidationError, complianceData
checks) and add the new pending/amount-match conditions referencing canWithdraw,
validateMutation, validationError, syncValidationError, parsedAmount, and
walletInfo.balance.
- Around line 47-56: parsedAmount becomes NaN when amount is empty which makes
parsedAmount unstable in the useEffect dependency array and causes
validateMutation.mutate(NaN) to be called; update the syncValidationError
calculation (used by the effect) to treat empty/NaN parsedAmount as an
early-return case (i.e., return null if amount is falsy or parsedAmount is NaN)
so the effect won't consider parsedAmount changing on every render and won't
call validateMutation.mutate with NaN; adjust the useMemo for
syncValidationError (referencing parsedAmount, isValidAmount,
walletInfo.balance) to explicitly check Number.isNaN(parsedAmount) or
!isFinite(parsedAmount) and return null before any mutate calls in the effect.
- Around line 58-76: The effect is re-running because validateMutation (the
whole mutation object) is in the dependency array; remove validateMutation and
depend on the stable mutate function instead (use validateMutation.mutate in the
deps) so the debounce only schedules when parsedAmount or syncValidationError
change, and ensure that when syncValidationError is truthy you immediately clear
any async error by calling setValidationError(null) (or set to the sync error)
before returning; keep parsedAmount and syncValidationError in the deps and
reference validateMutation.mutate inside the effect to call the API.
---
Nitpick comments:
In @.husky/pre-commit:
- Line 1: The pre-commit hook currently runs a full build ("npx lint-staged
--concurrent false && npm run build"), which slows commits; update the
.husky/pre-commit hook to only run lint-staged (remove the "&& npm run build"
part) and instead add the build step to a pre-push hook (create/update
.husky/pre-push to run "npm run build") or enforce the build in CI; locate the
string "npx lint-staged --concurrent false && npm run build" in the
.husky/pre-commit file and replace it with just the lint-staged invocation, and
add a corresponding pre-push/CI job to run "npm run build".
In `@app/api/compliance/status/route.ts`:
- Around line 6-7: The GET route handler currently declares an unused parameter
and silences linting; remove the unused parameter from the function signature
(change "export async function GET(_request: NextRequest)" to "export async
function GET()") and delete the corresponding eslint-disable comment and the
NextRequest import so the handler is clean and no longer suppresses the rule.
In `@app/api/compliance/terms/route.ts`:
- Around line 5-16: The GET route handler currently swallows errors and keeps an
unused _request parameter; update export async function GET to remove the unused
parameter, and inside the catch for TermsService.getCurrentTermsVersion log the
caught error (do not discard _error) before returning NextResponse.json({ error:
"Failed to fetch terms" }, { status: 500 }); — use console.error or your app
logger to include the error details so failures are visible and consistent with
the POST handler.
In `@app/api/compliance/upgrade/route.ts`:
- Around line 30-46: The GET handler currently swallows errors and keeps an
unused _request param; update the GET function to omit the unused parameter, and
in the catch block capture the error (e.g., error) and log it consistently
(console.error or the shared logging utility used by other compliance routes)
before returning the 500 NextResponse.json; locate the GET function,
getCurrentUser call, and VerificationService.getVerificationStatus reference to
implement the change and mirror the error handling pattern used in
app/api/compliance/status/route.ts.
- Line 12: The handler in route.ts pulls targetTier directly from request.json()
without validation; update the POST handler to validate that targetTier exists,
is a string/expected type, and matches one of the permitted KYCTier values
before calling the service layer (use Object.values(KYCTier) or your validation
utility/zod schema to check membership), return a 400/422 response for
missing/invalid input, and import/reference the KYCTier enum where validation
occurs so the service never receives an unvalidated targetTier.
In `@app/api/withdrawal/submit/route.ts`:
- Around line 24-29: Replace the unsafe assertion in the catch block with a
robust check: if (error instanceof Error) extract message = error.message and
log error.stack; derive the HTTP status by preferring an explicit numeric status
on the thrown error (e.g., (error as any).status || (error as any).statusCode)
and fall back to 400 for known validation errors (check error.name ===
'ValidationError' or error.name === 'BadRequestError') or 500 for all
other/internal errors; for non-Error throws convert to a string for the message
and return status 500. Update the NextResponse.json call to use the computed
message and status, and ensure you still call console.error with the full error
(stack when available) so failures from ComplianceService, TermsService, or
GeoRestrictionService are correctly classified as server errors when
appropriate.
In `@components/compliance/terms-dialog.tsx`:
- Around line 51-56: The scroll handler handleScroll uses e.target cast to
HTMLDivElement which can be a child element; replace that with e.currentTarget
(typed as HTMLDivElement) to reliably refer to the scrollable div and remove the
unnecessary cast; update the bottom calculation to use
currentTarget.scrollHeight, currentTarget.scrollTop, and
currentTarget.clientHeight and keep the setScrolled(true) behavior.
In `@components/wallet/withdrawal-section.tsx`:
- Around line 78-92: handleWithdraw currently only checks isValidAmount and
complianceData but not the full set of guards enforced by canWithdraw; add the
same guard at the top of handleWithdraw (preferably call canWithdraw and return
early if false) so programmatic calls cannot bypass hold/terms checks, then
proceed with submitMutation.mutateAsync using parsedAmount, "USD", and
bankAccounts[0].id; on success call setAmount("") and trigger the existing
success notification (later replace alert with toast/notification system).
Ensure you reference the existing symbols handleWithdraw, canWithdraw,
isValidAmount, complianceData, parsedAmount, bankAccounts, submitMutation, and
setAmount when making the change.
In `@lib/services/compliance.ts`:
- Around line 329-331: getAllTierConfigs currently returns direct references
from TIER_CONFIGS which allows callers to mutate shared static state; change it
to return deep/shallow copies by reusing the existing getTierConfig method for
each tier (call getTierConfig(directKey) for each key in TIER_CONFIGS) so
callers receive independent config objects and shared state is protected (refer
to static members TIER_CONFIGS, getAllTierConfigs, and getTierConfig).
- Around line 204-237: validateWithdrawalAmount currently calls
resetExpiredWindows and thus mutates state during a pure validation; remove the
side-effect by deleting the await this.resetExpiredWindows(userId, compliance)
call from validateWithdrawalAmount, leave getUserCompliance and the limit checks
as-is, and either (a) require callers to call resetExpiredWindows(userId,
compliance) before invoking validateWithdrawalAmount or (b) add a separate
method (e.g., validateWithdrawalAmountWithReset) that performs
resetExpiredWindows then delegates to validateWithdrawalAmount; also add a brief
doc comment to validateWithdrawalAmount stating it does not mutate state and
that callers must reset windows beforehand if needed.
- Around line 322-324: getTierConfig currently returns a shallow copy so nested
objects like limits and requirements on this.TIER_CONFIGS[tier] are still
shared; change getTierConfig to return a deep copy of the config (or at minimum
spread/copy nested properties) so mutations to the returned KYCTierConfig do not
mutate the static TIER_CONFIGS—e.g., clone the object for the given tier inside
getTierConfig (use structuredClone if available or a reliable deep-copy helper)
and ensure limits and requirements are copied rather than referenced.
- Around line 85-94: getUserCompliance currently returns a shallow copy which
intentionally preserves shared nested objects like limits and usage so methods
such as trackWithdrawal and resetExpiredWindows can mutate the mock DB via that
reference; add a clear comment above getUserCompliance (referencing
getUserCompliance, MOCK_COMPLIANCE_DB, createDefaultCompliance, trackWithdrawal,
and resetExpiredWindows) stating this intentional shallow-copy semantics and
warning that converting to a deep copy will break those mutating callers, or
alternatively change the implementation to return the actual DB object if you
prefer explicit shared-state behavior.
In `@lib/services/geo-restriction.ts`:
- Around line 42-80: checkLocation currently always returns a hardcoded US/CA
mock so RESTRICTED checks never trigger; change checkLocation to accept an
optional override (e.g., a Partial<UserLocation> or explicit
countryCode/regionCode params) and merge the override into the generated mock
before running detectVPN/detectProxy and the RESTRICTED checks, ensuring
functions named detectVPN and detectProxy are still awaited and RESTRICTED
lookup uses the final countryCode/regionCode; update call sites/tests to pass
overrides when simulating restricted scenarios.
- Around line 82-91: The detectVPN mock incorrectly treats RFC1918 private
ranges as "VPN ranges" — update the detectVPN function and its comment to
accurately reflect that it's a placeholder and not real VPN detection: change
the comment to state this is a simple placeholder/mock and add a TODO suggesting
integration with a real VPN detection service (e.g., IPHub, IP2Proxy, VPNApi),
and either replace the vpnRanges array with a more representative mock (e.g., a
small list of known public VPN provider CIDRs or IPs) or implement a simple
heuristic (e.g., check against a configurable list of known VPN ASNs/public
IPs); ensure the code and comment clearly reference detectVPN so future
maintainers don't confuse private LAN ranges for actual VPN detection.
In `@package.json`:
- Line 93: The prettier dependency in package.json is using a caret range
("prettier": "^3.8.1") which allows patch/minor upgrades and can cause
formatting drift; change the dependency value to an exact version string
("prettier": "3.8.1") in package.json (look for the "prettier" entry) and then
reinstall/update the lockfile (npm install / yarn install) so the lockfile
reflects the pinned version.
- Around line 12-13: Pre-commit currently runs a full Next.js build (via `npm
run build`) which is too slow; edit the `.husky/pre-commit` hook to remove the
`&& npm run build` step so it only runs fast checks (e.g., `lint-staged
--concurrent false` and any quick type-checks), and keep the `prepare`/husky
script in package.json unchanged; move the full `npm run build` gate into your
CI pipeline (e.g., add a build job in your CI config) so commits remain fast
while CI enforces the build.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (13)
components/wallet/withdrawal-section.tsx (3)
97-102:formatCurrencyis re-created on every render — extract it or memoize.This pure helper has no dependency on component state. Moving it outside the component (or wrapping in
useCallbackwith no deps) avoids unnecessary allocations on each render.Proposed fix
+const formatCurrency = (value: number) => + new Intl.NumberFormat("en-US", { + style: "currency", + currency: "USD", + }).format(value); + export function WithdrawalSection({ walletInfo }: WithdrawalSectionProps) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/wallet/withdrawal-section.tsx` around lines 97 - 102, The formatCurrency helper is being re-created on every render inside the component; move it out of the component scope (or wrap it with useCallback(() => ..., []) if you must keep it inside) so it’s defined once and not reallocated on each render; update references in the component (e.g., where formatCurrency is used in WithdrawalSection / withdrawal-section.tsx) to use the extracted/memoized function.
43-45: Hardcoded bank accounts — placeholder data leaking into the component.
bankAccountsis a hardcoded array with mock data. This is referenced both in the UI (lines 193–215) and inhandleWithdraw(line 88) where it's used as the actualdestinationIdsent to the server. If this is intentional scaffolding, consider adding a// TODOcomment and disabling the withdraw button when no real accounts are loaded; otherwise wire this to an API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/wallet/withdrawal-section.tsx` around lines 43 - 45, The component currently contains a hardcoded bankAccounts array which leaks placeholder data into the UI and is used as the destinationId in handleWithdraw; remove this mock array and either fetch real accounts (or accept them via props) and use the selected account's id as the destinationId in handleWithdraw, or if this is scaffolding add a clear TODO comment and ensure the Withdraw button (the button that triggers handleWithdraw) is disabled when no real accounts are loaded; update references to bankAccounts in the UI (where accounts are listed) to use the real data source and ensure handleWithdraw reads a single selectedAccount.id (not the entire array) for destinationId.
81-95:handleWithdrawsilently usesalert()for user feedback.Using
window.alert()blocks the UI thread and provides no styled feedback. This is likely a placeholder — consider replacing with a toast notification or in-page feedback consistent with the rest of the UI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/wallet/withdrawal-section.tsx` around lines 81 - 95, The handler handleWithdraw currently uses blocking window.alert() for success/error which is inconsistent with the app UI; replace those alerts by invoking the app's toast/in-page notification system (e.g., the useToast or Notification component used elsewhere) inside the try/catch around submitMutation.mutateAsync so success uses toast.success("Withdrawal submitted successfully!") and errors use toast.error(error.message || "Withdrawal failed"), keep the same state updates (setAmount("")) and destinationId logic (bankAccounts[0].id) and ensure the toast hook/import is added to the component and used instead of alert().app/api/withdrawal/submit/route.ts (2)
27-28: Default status of 400 is misleading for theErrorbranch.
statusis initialized to400, but this default is never actually reached forErrorinstances — the||chain on lines 33–38 always resolves. However, if someone later refactors the chain, this silent default could mask server errors as client errors. Consider initializing to500for safety and only downgrading to400when the error is explicitly a validation/bad-request error.Proposed fix
let message = "Withdrawal failed"; - let status = 400; + let status = 500;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/withdrawal/submit/route.ts` around lines 27 - 28, The default error status should be 500 instead of 400 to avoid silently classifying server errors as client errors; change the initialization of status from 400 to 500 (the variables message and status at the top of the route handler) and only overwrite status to 400 when you detect an explicit validation/bad-request error (the branch that currently builds status via the || chain from the caught Error). Ensure the catch block preserves the actual Error behavior by setting status to 400 only for known client-error types and otherwise leaving it as 500 while still populating message with the error details.
12-12: No input validation before calling the service layer.
amount,currency, anddestinationIdare used directly from the parsed body without any type or presence checks. If the body is malformed (e.g.,amountis a string,currencyis missing), the error will surface as an opaque service-layer failure rather than a clear 400. Consider adding basic guards before invokingWithdrawalService.submit.Proposed fix
const { amount, currency, destinationId } = await request.json(); + + if (typeof amount !== "number" || !isFinite(amount) || amount <= 0) { + return NextResponse.json({ error: "Invalid amount" }, { status: 400 }); + } + if (!currency || typeof currency !== "string") { + return NextResponse.json({ error: "Currency is required" }, { status: 400 }); + } + if (!destinationId) { + return NextResponse.json({ error: "Destination is required" }, { status: 400 }); + } + const ip = request.headers.get("x-forwarded-for") || "0.0.0.0";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/withdrawal/submit/route.ts` at line 12, The route currently reads const { amount, currency, destinationId } = await request.json() and calls WithdrawalService.submit without validating inputs; add guards in the route handler to ensure amount is present and a finite number (coerce/parse and reject NaN), currency is a non-empty string (optionally check against allowed currencies), and destinationId is a non-empty string (or valid id format), and return a 400 response with a clear error message when validation fails; only call WithdrawalService.submit after these checks and include the validated/parsed values (e.g., numeric amount) when invoking WithdrawalService.submit..husky/pre-commit (1)
1-1: Consider adding a#!/usr/bin/env shshebang for portability.Husky no longer automatically adds shebang lines to hook scripts; you need to ensure your scripts start with an appropriate shebang if they require a specific interpreter. While Husky v9 invokes hooks via
shby default making it optional for simple commands, a shebang makes the interpreter explicit and avoids surprising behavior on non-standard environments.🔧 Proposed addition
+#!/usr/bin/env sh npx lint-staged --concurrent false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.husky/pre-commit at line 1, Add an explicit shell shebang to the pre-commit hook so the interpreter is deterministic: update the .husky/pre-commit script that currently runs "npx lint-staged --concurrent false" to start with a shebang line like "#!/usr/bin/env sh" (or another appropriate shell) and ensure the file is executable; this makes the hook run under the intended shell even on environments where Husky's invocation differs..husky/pre-push (1)
1-1: Adding shebang lines is optional but recommended for Husky v9.The
.husky/pre-pushand.husky/pre-commitfiles lack shebang lines. With Husky v9.1.7 (installed in this repo), the shebang is optional since Husky invokes hooks withsh -einternally and does not require the.husky.shsource line. The hooks will work correctly as-is.However, adding
#!/usr/bin/env shat the top of each file is still a best practice for editor recognition and clarity:+#!/usr/bin/env sh + npm run build🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.husky/pre-push at line 1, Add a POSIX shebang to the top of the husky hook files to improve editor recognition and clarity: open the pre-push and pre-commit hook files and prepend a single line with the POSIX shebang (#!/usr/bin/env sh) as the very first line so the hooks remain compatible with Husky v9 while being explicitly recognized by editors and tools.app/api/compliance/upgrade/route.ts (1)
34-43: Catch block returns 400 for all errors, masking server-side failures.If
VerificationService.createVerificationRequestthrows due to an internal error (e.g., database failure), returning 400 misleads the client into thinking the request was malformed. Reserve 400 for known validation errors and use 500 for unexpected failures.Suggested fix
} catch (error) { console.error("Error creating verification request:", error); + const message = (error as Error).message || "Failed to create verification request"; + const isValidationError = message.includes("Invalid") || message.includes("missing"); return NextResponse.json( { - error: - (error as Error).message || "Failed to create verification request", + error: message, }, - { status: 400 }, + { status: isValidationError ? 400 : 500 }, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/compliance/upgrade/route.ts` around lines 34 - 43, The catch in route.ts around VerificationService.createVerificationRequest currently returns a 400 for all errors; change it to detect validation/expected errors (e.g., check for a ValidationError type or an error.code from VerificationService.createVerificationRequest) and return NextResponse.json(..., { status: 400 }) only for those cases, otherwise log the error and return a 500 response via NextResponse.json(..., { status: 500 }); ensure you reference the catch that wraps VerificationService.createVerificationRequest and use NextResponse.json for both branches so clients get 400 for client-side validation failures and 500 for unexpected server-side failures.components/compliance/tier-upgrade-dialog.tsx (1)
197-201: "Already at this tier" message renders afterDialogFooter, which may appear visually disconnected.Consider moving this message above the footer (inside the content area) so it's adjacent to the upgrade button it explains.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/compliance/tier-upgrade-dialog.tsx` around lines 197 - 201, Move the "already at this tier" validation message from below the DialogFooter to inside the dialog content area just above the upgrade button so it visually associates with the action; specifically, relocate the block that renders when !isUpgradeValid && step === "info" into the content container (near the upgrade button JSX and before the DialogFooter component) so the message appears adjacent to the upgrade button and still conditionally renders based on isUpgradeValid/step.lib/services/geo-restriction.ts (3)
77-86: State-level restriction check incheckLocationis US-only, butisRestrictedis generic.
checkLocationonly evaluates state restrictions whencountryCode === "US"(Line 77), whileisRestricted(Line 119) constructs${countryCode}-${regionCode}for any country. If a non-US state-level restriction is added toRESTRICTED,checkLocationwill silently miss it.Consider making
checkLocationgeneric or delegating toisRestricted/getRestrictionReasonto avoid divergence:Suggested refactor
- // Check country-level restrictions - const countryRestriction = RESTRICTED.find( - (r) => r.code === mockLocation.countryCode && r.type === "COUNTRY", - ); - if (countryRestriction) { - mockLocation.isRestricted = true; - mockLocation.restrictionReason = countryRestriction.reason; - } - - // Check state-level restrictions - if (mockLocation.countryCode === "US" && mockLocation.regionCode) { - const stateCode = `US-${mockLocation.regionCode}`; - const stateRestriction = RESTRICTED.find( - (r) => r.code === stateCode && r.type === "STATE", - ); - if (stateRestriction) { - mockLocation.isRestricted = true; - mockLocation.restrictionReason = stateRestriction.reason; - } - } + // Delegate to centralized helpers + mockLocation.isRestricted = this.isRestricted( + mockLocation.countryCode, + mockLocation.regionCode, + ); + const reason = this.getRestrictionReason( + mockLocation.countryCode, + mockLocation.regionCode, + ); + if (reason) { + mockLocation.restrictionReason = reason; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/geo-restriction.ts` around lines 77 - 86, The state-level restriction branch in checkLocation currently only runs for countryCode === "US" which misses non-US region codes; update checkLocation to either construct the stateCode generically (const stateCode = `${mockLocation.countryCode}-${mockLocation.regionCode}`) and look it up in RESTRICTED, or better yet delegate to the existing isRestricted/getRestrictionReason helpers so the same logic is reused; adjust the block that sets mockLocation.isRestricted and mockLocation.restrictionReason to use getRestrictionReason(stateCode) or isRestricted(...) and remove the hard-coded "US" check to handle state-level restrictions for any country.
49-55: Falsy-string fields fall through to defaults due to||.Using
||forcountryCode,countryName, etc. means an explicitly passed empty string""inoverrideis treated as missing and falls back to the default. If this is intentional (empty strings are never valid), this is fine; otherwise, use??(nullish coalescing).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/geo-restriction.ts` around lines 49 - 55, The mockLocation construction uses the logical OR operator which treats empty strings as falsy and will fall back to defaults; update the assignments in the mockLocation object (the properties countryCode, countryName, regionCode, regionName, city) to use the nullish coalescing operator on override (e.g., override?.countryCode ?? "US") so that explicit empty strings are preserved while still defaulting when override is null/undefined; modify the mockLocation initialization inside lib/services/geo-restriction.ts accordingly, leaving other logic untouched.
91-100: Mock VPN list includes well-known DNS resolver IPs.
8.8.8.8(Google DNS) and1.1.1.1(Cloudflare DNS) are not VPN endpoints. Using them as mock VPN IPs may cause confusing false positives during development/testing if these IPs appear in real request headers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/geo-restriction.ts` around lines 91 - 100, The mock VPN list in detectVPN currently uses real public DNS resolver IPs via the vpnIps array (e.g., "8.8.8.8", "1.1.1.1"), which can produce false positives; replace those entries in the detectVPN function with non-routable/reserved example addresses (e.g., addresses from 192.0.2.0/24, 198.51.100.0/24, or 203.0.113.0/24) or make vpnIps configurable via environment/test fixtures so development tests never mark real public resolvers as VPNs.lib/services/compliance.ts (1)
89-98: Shallow copy leaks mutable references to nested objects (usage,limits).
{ ...MOCK_COMPLIANCE_DB[userId] }only copies top-level properties. Any external caller ofgetUserCompliancegets direct references to the sameusageandlimitsobjects stored inMOCK_COMPLIANCE_DB, and can mutate the DB unintentionally. The internal mutating methods (e.g.,trackWithdrawal) rely on this shared-reference behavior, but it breaks the encapsulation claimed by the JSDoc.Consider deep-cloning for the public return path and using a separate internal accessor for methods that need to mutate:
Suggested approach
+ /** Internal: returns the live DB record (mutable). */ + private static getComplianceRecord(userId: string): UserCompliance { + if (!MOCK_COMPLIANCE_DB[userId]) { + MOCK_COMPLIANCE_DB[userId] = this.createDefaultCompliance(userId); + } + return MOCK_COMPLIANCE_DB[userId]; + } + static async getUserCompliance(userId: string): Promise<UserCompliance> { await this.simulateDelay(); - if (!MOCK_COMPLIANCE_DB[userId]) { - MOCK_COMPLIANCE_DB[userId] = this.createDefaultCompliance(userId); - } - return { ...MOCK_COMPLIANCE_DB[userId] }; + const record = this.getComplianceRecord(userId); + return JSON.parse(JSON.stringify(record)); // deep clone for external callers }Then have internal mutating methods call
getComplianceRecorddirectly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/compliance.ts` around lines 89 - 98, getUserCompliance currently returns a shallow copy ({ ...MOCK_COMPLIANCE_DB[userId] }) which leaks mutable nested objects (usage, limits); replace that public-facing shallow copy with a deep-clone of the compliance record (so callers cannot mutate nested state) and add/use an internal accessor (e.g., getComplianceRecord or similar) that returns the actual object for internal mutating methods like trackWithdrawal and any other internal updates; ensure createDefaultCompliance remains unchanged except that getUserCompliance uses deep clone when returning to callers while internal mutators call the internal accessor to mutate the DB directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/api/compliance/terms/route.ts`:
- Line 25: The route handler in app/api/compliance/terms/route.ts should
validate the incoming termsVersionId before calling TermsService.acceptTerms:
check that req body (or parsed JSON) contains a non-null, defined,
correctly-typed termsVersionId (e.g., a non-empty string or expected type) and
immediately return a 400 Bad Request with a clear error message if the value is
missing/invalid; only call TermsService.acceptTerms when validation passes to
avoid forwarding malformed data and producing a 500 error downstream.
- Line 26: The X-Forwarded-For header can contain a comma-separated list of IPs;
update the request handler (e.g., the route handler function in route.ts, such
as the exported GET/POST handler) to parse req.headers.get('x-forwarded-for') by
splitting on ',' and taking the first element .trim() as the client IP before
storing it (use or replace the current complianceIp or ip variable), and fall
back to req.ip or remoteAddress only if the header is missing; ensure the stored
value is the single parsed IP string used in downstream audit/save logic.
In `@app/api/compliance/upgrade/route.ts`:
- Around line 15-28: VALID_TIERS currently includes "UNVERIFIED" which allows
downgrades; remove "UNVERIFIED" from VALID_TIERS and instead derive the allowed
upgrade targets from ComplianceService.TIER_CONFIGS to avoid duplication. Update
the validation around targetTier (the check that uses VALID_TIERS and the
variable targetTier) to build VALID_TIERS dynamically from
Object.keys(ComplianceService.TIER_CONFIGS) or a filtered list of those keys
excluding "UNVERIFIED", then use that derived array for the includes check so
upgrades cannot target UNVERIFIED and the list stays DRY.
In `@app/api/withdrawal/submit/route.ts`:
- Line 13: The request header "x-forwarded-for" may contain a comma-separated
list of IPs; change how `ip` is derived in route.ts so you parse
request.headers.get("x-forwarded-for"), split on commas, trim entries and select
the first non-empty entry as the client IP (falling back to
request.socket.remoteAddress or "0.0.0.0" if none). Update the `ip` assignment
that currently sets const ip = request.headers.get("x-forwarded-for") ||
"0.0.0.0" to perform this split/trim/select logic so downstream checks (rate
limiting, geo) receive only the client IP.
In `@components/compliance/tier-upgrade-dialog.tsx`:
- Around line 47-51: The dialog's internal state (step, requestId, uploadedDocs)
isn't reset when reopened or when targetTier changes; add a useEffect that
watches open and targetTier and, when open is true, calls setStep("info"),
setRequestId(null), and setUploadedDocs(new Set()) to reset state; reference the
existing state variables (step, setStep, requestId, setRequestId, uploadedDocs,
setUploadedDocs) and add the effect after their declarations inside the
component.
- Around line 73-83: The handleDocumentUpload function lacks error handling;
wrap the await VerificationService.uploadDocument(...) call in a try/catch
similar to handleUpgrade, so any thrown error is caught and handled (e.g., log
or surface via the same user-facing error/toast mechanism used in handleUpgrade)
and only update setUploadedDocs(...) on success; ensure the catch captures the
error and uses the same error reporting/path as handleUpgrade for consistency.
In `@components/wallet/withdrawal-section.tsx`:
- Around line 226-242: The UI is using a hardcoded $2.50 fee in
WithdrawalSection which can drift from server-side logic; update the fee display
and the "You'll Receive" calculation to use the server-provided fee (e.g., from
the validation response object like withdrawValidation.fee or a config value
returned by fetchWithdrawValidation()) instead of the literal 2.5, ensure you
parse/normalize that fee to a number and fall back to 0 if missing, and replace
references to the hardcoded string/number in the JSX (the span showing "$2.50"
and the expression formatCurrency(Math.max(0, (parsedAmount || 0) - 2.5))) so
the UI always reflects the authoritative server fee and currency formatting.
In `@lib/services/compliance.ts`:
- Around line 209-241: The validateWithdrawalAmount method can make decisions
based on stale windows; update it to ensure windows are reset before checking
limits by invoking resetExpiredWindows for the user (same approach as
getRemainingLimits) or by calling a public wrapper that resets windows first;
specifically, inside validateWithdrawalAmount (before reading
compliance.limits/usage) call the existing resetExpiredWindows(userId) helper
(or a newly exposed public reset method) then fetch compliance via
getUserCompliance so validations use up-to-date window data.
- Around line 258-272: The upgradeTier implementation currently applies any
KYCTier unconditionally (allowing downgrades); change it to validate tier
progression by first reading the user's currentTier via
this.getUserCompliance(userId), compute the allowed progression using
this.getNextTier(currentTier) (or compare indexes from
this.TIER_CONFIGS/TIER_ORDER), and only allow setting compliance.currentTier to
the same tier or a strictly higher tier (i.e., equal to the next tier or any
tier with a higher order); if the requested newTier is a downgrade (lower order)
or otherwise invalid, do not modify the record and return false or throw an
error to reject the operation. Ensure you still update limits,
verificationStatus, and updatedAt only when the validation passes.
---
Duplicate comments:
In `@components/wallet/withdrawal-section.tsx`:
- Around line 57-79: The effect currently depends on the entire validateMutation
object which gets a new reference on mutation state changes and causes re-render
loops; extract the stable mutate function from validateMutation (e.g., const {
mutate } = validateMutation or store mutate in a ref/useCallback) and use that
in the effect dependency list instead of validateMutation, then update the
dependency array to [syncValidationError, mutate, parsedAmount] so the
debounce/mutate logic uses the stable mutate reference (ensure the extracted
mutate is stable across renders or wrap it in a ref/useCallback if necessary).
---
Nitpick comments:
In @.husky/pre-commit:
- Line 1: Add an explicit shell shebang to the pre-commit hook so the
interpreter is deterministic: update the .husky/pre-commit script that currently
runs "npx lint-staged --concurrent false" to start with a shebang line like
"#!/usr/bin/env sh" (or another appropriate shell) and ensure the file is
executable; this makes the hook run under the intended shell even on
environments where Husky's invocation differs.
In @.husky/pre-push:
- Line 1: Add a POSIX shebang to the top of the husky hook files to improve
editor recognition and clarity: open the pre-push and pre-commit hook files and
prepend a single line with the POSIX shebang (#!/usr/bin/env sh) as the very
first line so the hooks remain compatible with Husky v9 while being explicitly
recognized by editors and tools.
In `@app/api/compliance/upgrade/route.ts`:
- Around line 34-43: The catch in route.ts around
VerificationService.createVerificationRequest currently returns a 400 for all
errors; change it to detect validation/expected errors (e.g., check for a
ValidationError type or an error.code from
VerificationService.createVerificationRequest) and return NextResponse.json(...,
{ status: 400 }) only for those cases, otherwise log the error and return a 500
response via NextResponse.json(..., { status: 500 }); ensure you reference the
catch that wraps VerificationService.createVerificationRequest and use
NextResponse.json for both branches so clients get 400 for client-side
validation failures and 500 for unexpected server-side failures.
In `@app/api/withdrawal/submit/route.ts`:
- Around line 27-28: The default error status should be 500 instead of 400 to
avoid silently classifying server errors as client errors; change the
initialization of status from 400 to 500 (the variables message and status at
the top of the route handler) and only overwrite status to 400 when you detect
an explicit validation/bad-request error (the branch that currently builds
status via the || chain from the caught Error). Ensure the catch block preserves
the actual Error behavior by setting status to 400 only for known client-error
types and otherwise leaving it as 500 while still populating message with the
error details.
- Line 12: The route currently reads const { amount, currency, destinationId } =
await request.json() and calls WithdrawalService.submit without validating
inputs; add guards in the route handler to ensure amount is present and a finite
number (coerce/parse and reject NaN), currency is a non-empty string (optionally
check against allowed currencies), and destinationId is a non-empty string (or
valid id format), and return a 400 response with a clear error message when
validation fails; only call WithdrawalService.submit after these checks and
include the validated/parsed values (e.g., numeric amount) when invoking
WithdrawalService.submit.
In `@components/compliance/tier-upgrade-dialog.tsx`:
- Around line 197-201: Move the "already at this tier" validation message from
below the DialogFooter to inside the dialog content area just above the upgrade
button so it visually associates with the action; specifically, relocate the
block that renders when !isUpgradeValid && step === "info" into the content
container (near the upgrade button JSX and before the DialogFooter component) so
the message appears adjacent to the upgrade button and still conditionally
renders based on isUpgradeValid/step.
In `@components/wallet/withdrawal-section.tsx`:
- Around line 97-102: The formatCurrency helper is being re-created on every
render inside the component; move it out of the component scope (or wrap it with
useCallback(() => ..., []) if you must keep it inside) so it’s defined once and
not reallocated on each render; update references in the component (e.g., where
formatCurrency is used in WithdrawalSection / withdrawal-section.tsx) to use the
extracted/memoized function.
- Around line 43-45: The component currently contains a hardcoded bankAccounts
array which leaks placeholder data into the UI and is used as the destinationId
in handleWithdraw; remove this mock array and either fetch real accounts (or
accept them via props) and use the selected account's id as the destinationId in
handleWithdraw, or if this is scaffolding add a clear TODO comment and ensure
the Withdraw button (the button that triggers handleWithdraw) is disabled when
no real accounts are loaded; update references to bankAccounts in the UI (where
accounts are listed) to use the real data source and ensure handleWithdraw reads
a single selectedAccount.id (not the entire array) for destinationId.
- Around line 81-95: The handler handleWithdraw currently uses blocking
window.alert() for success/error which is inconsistent with the app UI; replace
those alerts by invoking the app's toast/in-page notification system (e.g., the
useToast or Notification component used elsewhere) inside the try/catch around
submitMutation.mutateAsync so success uses toast.success("Withdrawal submitted
successfully!") and errors use toast.error(error.message || "Withdrawal
failed"), keep the same state updates (setAmount("")) and destinationId logic
(bankAccounts[0].id) and ensure the toast hook/import is added to the component
and used instead of alert().
In `@lib/services/compliance.ts`:
- Around line 89-98: getUserCompliance currently returns a shallow copy ({
...MOCK_COMPLIANCE_DB[userId] }) which leaks mutable nested objects (usage,
limits); replace that public-facing shallow copy with a deep-clone of the
compliance record (so callers cannot mutate nested state) and add/use an
internal accessor (e.g., getComplianceRecord or similar) that returns the actual
object for internal mutating methods like trackWithdrawal and any other internal
updates; ensure createDefaultCompliance remains unchanged except that
getUserCompliance uses deep clone when returning to callers while internal
mutators call the internal accessor to mutate the DB directly.
In `@lib/services/geo-restriction.ts`:
- Around line 77-86: The state-level restriction branch in checkLocation
currently only runs for countryCode === "US" which misses non-US region codes;
update checkLocation to either construct the stateCode generically (const
stateCode = `${mockLocation.countryCode}-${mockLocation.regionCode}`) and look
it up in RESTRICTED, or better yet delegate to the existing
isRestricted/getRestrictionReason helpers so the same logic is reused; adjust
the block that sets mockLocation.isRestricted and mockLocation.restrictionReason
to use getRestrictionReason(stateCode) or isRestricted(...) and remove the
hard-coded "US" check to handle state-level restrictions for any country.
- Around line 49-55: The mockLocation construction uses the logical OR operator
which treats empty strings as falsy and will fall back to defaults; update the
assignments in the mockLocation object (the properties countryCode, countryName,
regionCode, regionName, city) to use the nullish coalescing operator on override
(e.g., override?.countryCode ?? "US") so that explicit empty strings are
preserved while still defaulting when override is null/undefined; modify the
mockLocation initialization inside lib/services/geo-restriction.ts accordingly,
leaving other logic untouched.
- Around line 91-100: The mock VPN list in detectVPN currently uses real public
DNS resolver IPs via the vpnIps array (e.g., "8.8.8.8", "1.1.1.1"), which can
produce false positives; replace those entries in the detectVPN function with
non-routable/reserved example addresses (e.g., addresses from 192.0.2.0/24,
198.51.100.0/24, or 203.0.113.0/24) or make vpnIps configurable via
environment/test fixtures so development tests never mark real public resolvers
as VPNs.
…e layer purity, and lint fixes
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
lib/services/geo-restriction.ts (3)
150-152:[...RESTRICTED]is a shallow array copy — returnedRestrictedJurisdictionobjects are still mutable references into the module-levelRESTRICTEDarray.A caller can do
getRestrictedJurisdictions()[0].code = 'XX'and silently corrupt all subsequentisRestricted,getRestrictionReason, andcheckLocationcalls for the lifetime of the module.const RESTRICTEDprevents binding reassignment, not element mutation.♻️ Proposed fix — shallow-copy each entry
static getRestrictedJurisdictions(): RestrictedJurisdiction[] { - return [...RESTRICTED]; + return RESTRICTED.map((r) => ({ ...r })); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/geo-restriction.ts` around lines 150 - 152, getRestrictedJurisdictions currently returns a shallow copy of the module array (RESTRICTED) but preserves object references so callers can mutate the returned RestrictedJurisdiction objects; change getRestrictedJurisdictions to return new object copies for each entry (e.g., map over RESTRICTED and clone each RestrictedJurisdiction) so callers cannot mutate module-level state—update the method that references RESTRICTED and consider using object spread or a cloning utility (or Object.freeze on each returned item) to ensure immutability of returned entries.
91-107: Placeholder VPN/proxy detection integrated into the production code path with no indication to callers that results are unreliable.
detectVPNchecks against RFC 5737 documentation IPs (192.0.2.1,198.51.100.1) — these are reserved ranges, never routed on the internet. Any real user IP always resolves tofalse.detectProxyunconditionally returnsfalse. Both results are silently propagated intoUserLocation.isVPN/isProxy, indistinguishable from a real detection result.Consider surfacing the stub status so callers can act accordingly, e.g., adding a
detectionConfidencefield or throwing in environments where real detection is expected.Do you want me to open an issue to track integration of a real VPN/proxy detection service (e.g. IPHub, IP2Proxy, VPNApi)?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/geo-restriction.ts` around lines 91 - 107, detectVPN and detectProxy are currently silent placeholders so callers (UserLocation.isVPN / isProxy) can't tell results are unreliable; change both detectVPN and detectProxy to return a structured DetectionResult (e.g., { isDetected: boolean, detectionConfidence: number, isStub: boolean }) instead of a bare boolean, set isStub = true and detectionConfidence = 0 for the current mocked logic, and update UserLocation.isVPN / isProxy to read the new fields (or fail if an environment flag like REQUIRE_REAL_DETECTION is set) so callers can distinguish stubbed results from real detections and act accordingly.
76-86: State-level check silently drops state restriction reason when country is also restricted.
getRestrictionReason(countryCode, regionCode)evaluates country first (lines 130–135), then state. Inside this "state-level check" block:
- When the country is already restricted (set by lines 67–74),
getRestrictionReasonreturns the country reason again — the block becomes a redundant no-op.- When both country and state are restricted, the state's
reasonis silently ignored becausegetRestrictionReasonshort-circuits on the country match. The comment promises "state-level restrictions" but the implementation can never surface a state reason in that case.Use a direct state-code lookup instead, consistent with
isRestricted:♻️ Proposed fix — direct state-only lookup
- // Check state-level restrictions (generic support for any country) - if (mockLocation.regionCode) { - const reason = this.getRestrictionReason( - mockLocation.countryCode, - mockLocation.regionCode, - ); - if (reason) { - mockLocation.isRestricted = true; - mockLocation.restrictionReason = reason; - } - } + // Check state-level restrictions (generic support for any country) + if (mockLocation.regionCode) { + const stateCode = `${mockLocation.countryCode}-${mockLocation.regionCode}`; + const stateRestriction = RESTRICTED.find( + (r) => r.code === stateCode && r.type === "STATE", + ); + if (stateRestriction) { + mockLocation.isRestricted = true; + mockLocation.restrictionReason = stateRestriction.reason; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/geo-restriction.ts` around lines 76 - 86, When checking state-level restrictions you must perform a state-only lookup instead of calling getRestrictionReason(countryCode, regionCode) which returns a country match first and hides state reasons; update the region block that reads mockLocation.regionCode to fetch the state restriction reason directly (use the same state-only lookup mechanism used by isRestricted or add/call a getStateRestrictionReason(countryCode, regionCode) that checks only the region map) and then set mockLocation.isRestricted and mockLocation.restrictionReason from that state-specific result so state reasons aren’t overwritten by country-level matches.components/compliance/tier-upgrade-dialog.tsx (1)
20-25:TIER_ORDERis duplicated across files.The same tier ordering map exists in
lib/services/compliance.ts(line 272, insideupgradeTier). Consider exporting a single source of truth from the compliance service or types module to avoid them drifting apart.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/compliance/tier-upgrade-dialog.tsx` around lines 20 - 25, TIER_ORDER is duplicated; instead create and export a single canonical tier ordering constant (e.g., export const TIER_ORDER or enum) from the compliance service or shared types module and import it where needed; update the local references in this file (the TIER_ORDER constant and any usage expecting KYCTier) to use the exported symbol, and remove the duplicated definition so upgradeTier and tier-upgrade-dialog both consume the single source of truth.lib/services/compliance.ts (2)
95-102:simulateDelayis only applied togetUserCompliance.Other public async methods (
getRemainingLimits,validateWithdrawalAmount,trackWithdrawal,upgradeTier, etc.) skip the simulated delay. If the delay is meant to mimic real DB latency for testing, the inconsistency could mask timing-related bugs in callers that chain multiple service calls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/compliance.ts` around lines 95 - 102, The simulated latency is only invoked in getUserCompliance via simulateDelay, causing inconsistent async behavior across the service; update the service so all public async methods (e.g., getRemainingLimits, validateWithdrawalAmount, trackWithdrawal, upgradeTier, and any other exported async functions) also await simulateDelay at their start (or refactor delay invocation into a shared wrapper/helper that each public method calls) to ensure consistent DB-latency simulation and prevent timing-related inconsistencies in callers.
85-90:getComplianceRecordreturns a mutable DB reference — mutation methods redundantly re-assign it.Since
getComplianceRecordreturns the actual object stored inMOCK_COMPLIANCE_DB, theMOCK_COMPLIANCE_DB[userId] = compliancewrites intrackWithdrawal(line 263),upgradeTier(line 290),setHoldState(line 308), etc., are no-ops. This is harmless now, but ifgetComplianceRecordever returns a clone, all those methods silently stop persisting changes.Consider either: (a) making the pattern explicit with a comment, or (b) always writing back as a defensive practice and documenting that
getComplianceRecordmust return a direct reference.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/compliance.ts` around lines 85 - 90, getComplianceRecord currently returns the live object from MOCK_COMPLIANCE_DB so mutations persist implicitly, but callers like trackWithdrawal, upgradeTier and setHoldState assume and redundantly reassign MOCK_COMPLIANCE_DB[userId]; make this explicit and defensive by adding a short comment on getComplianceRecord that it returns a direct reference (or alternatively returns a clone), and update mutation methods (trackWithdrawal, upgradeTier, setHoldState) to always write the mutated object back to MOCK_COMPLIANCE_DB[userId] after changing fields so persistence does not depend on reference semantics; reference getComplianceRecord, MOCK_COMPLIANCE_DB, trackWithdrawal, upgradeTier and setHoldState when applying the change.components/wallet/withdrawal-section.tsx (1)
111-114:serverFeeternary is a no-op — both branches return2.5.Line 114:
validateMutation.data?.valid ? 2.5 : 2.5always evaluates to2.5. The TODO acknowledges this, but the conditional is misleading — a plainconst serverFee = 2.5would be clearer until the real fee is available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/wallet/withdrawal-section.tsx` around lines 111 - 114, The serverFee ternary in withdrawal-section.tsx is a no-op (validateMutation.data?.valid ? 2.5 : 2.5); replace it with a simple constant assignment and keep the TODO: change the line that defines serverFee to a plain const serverFee = 2.5 and leave a comment/TODO indicating to pull the real fee from validateMutation.data when available (referencing serverFee, validateMutation, parsedAmount, and isAmountValidated to locate the code).app/api/withdrawal/submit/route.ts (1)
40-60: Malformed JSON body will return 500 instead of 400.If
request.json()on line 12 throws (e.g., invalid JSON), execution falls into the catch block. Since the resultingSyntaxErrordoesn't havename === "ValidationError"or a.statusproperty, the client gets a 500 — misleading for a client-side input error.Suggested fix — catch JSON parse errors explicitly
+ let body; + try { + body = await request.json(); + } catch { + return NextResponse.json({ error: "Invalid request body" }, { status: 400 }); + } + - const { amount, currency, destinationId } = await request.json(); + const { amount, currency, destinationId } = body;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/withdrawal/submit/route.ts` around lines 40 - 60, The catch block that returns NextResponse.json currently maps most errors to 500, so JSON parse failures from request.json() become 500; update the error handling in the catch to explicitly detect JSON parse errors (e.g., error instanceof SyntaxError or error.name === "SyntaxError") and set status to 400 with a clear message like "Invalid JSON body" (or use error.message), leaving other errors handled as before; modify the catch logic around request.json(), the error/type checks you already have (error instanceof Error, err.status/err.statusCode, error.name checks), and the return NextResponse.json(...) to return 400 for SyntaxError cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/api/compliance/upgrade/route.ts`:
- Around line 35-47: The response uses (error as Error).message which can be
undefined if the thrown value isn't an Error; update the handler to derive a
safe error string before calling NextResponse.json by checking error instanceof
Error and using error.message, otherwise use String(error) if present or a
static fallback like "Failed to create verification request"; keep the existing
isValidationError logic that uses instanceof Error and reuse that safe message
variable in the NextResponse.json payload and status decision so the response
never contains { error: undefined } (refer to isValidationError, the error
variable, and the NextResponse.json call).
- Around line 16-26: The route currently only checks that targetTier is a valid
non-UNVERIFIED tier but not that it is a progression from the user’s current
tier; fetch the user’s current tier (e.g., from the request user/session or the
ComplianceService method that returns the user’s current KYCTier), compare the
rank/order using ComplianceService.TIER_CONFIGS (or an explicit ranking/array of
tiers) and reject the request with a 400 if targetTier is not strictly higher
than the current tier (mirroring the client-side isUpgradeValid logic); update
the validation around VALID_TIERS/targetTier to perform this progression check
and return an error response when the upgrade is not actually an upgrade.
In `@components/compliance/tier-upgrade-dialog.tsx`:
- Around line 206-209: The "Back" button currently always sets setStep("info")
which lets the user re-submit and call handleUpgrade again after requestId was
set; change the Back click handler so that if requestId is truthy it navigates
directly to the documents step (e.g., setStep("documents")) instead of "info",
and also prevent duplicate submits by making the "Request Upgrade" action/button
check requestId (disable the button or short-circuit handleUpgrade when
requestId exists) to ensure handleUpgrade is not called again once a requestId
has been created.
In `@components/wallet/withdrawal-section.tsx`:
- Around line 71-93: In the useEffect inside the WithdrawalSection component
(the effect that depends on syncValidationError, validate, parsedAmount), ensure
any stale async validation error is cleared as soon as a syncValidationError is
present: when syncValidationError is truthy, call setValidationError(null)
before returning so the UI won’t briefly show a previous async error; keep the
existing debounce/validate logic unchanged for the non-sync-error path (use the
existing parsedAmount, validate, and debounceTimer).
In `@lib/services/geo-restriction.ts`:
- Around line 44-50: The override.ip value is ignored when building
mockLocation.ip; update the assignment so mockLocation.ip uses override?.ip if
present (e.g., set mockLocation.ip = override?.ip ?? ip) or alternatively change
the parameter type to exclude ip (do not accept ip in Partial<UserLocation>),
and ensure the function that creates mockLocation (the mockLocation object and
the override parameter) reflects this decision so callers' intent is honored.
---
Nitpick comments:
In `@app/api/withdrawal/submit/route.ts`:
- Around line 40-60: The catch block that returns NextResponse.json currently
maps most errors to 500, so JSON parse failures from request.json() become 500;
update the error handling in the catch to explicitly detect JSON parse errors
(e.g., error instanceof SyntaxError or error.name === "SyntaxError") and set
status to 400 with a clear message like "Invalid JSON body" (or use
error.message), leaving other errors handled as before; modify the catch logic
around request.json(), the error/type checks you already have (error instanceof
Error, err.status/err.statusCode, error.name checks), and the return
NextResponse.json(...) to return 400 for SyntaxError cases.
In `@components/compliance/tier-upgrade-dialog.tsx`:
- Around line 20-25: TIER_ORDER is duplicated; instead create and export a
single canonical tier ordering constant (e.g., export const TIER_ORDER or enum)
from the compliance service or shared types module and import it where needed;
update the local references in this file (the TIER_ORDER constant and any usage
expecting KYCTier) to use the exported symbol, and remove the duplicated
definition so upgradeTier and tier-upgrade-dialog both consume the single source
of truth.
In `@components/wallet/withdrawal-section.tsx`:
- Around line 111-114: The serverFee ternary in withdrawal-section.tsx is a
no-op (validateMutation.data?.valid ? 2.5 : 2.5); replace it with a simple
constant assignment and keep the TODO: change the line that defines serverFee to
a plain const serverFee = 2.5 and leave a comment/TODO indicating to pull the
real fee from validateMutation.data when available (referencing serverFee,
validateMutation, parsedAmount, and isAmountValidated to locate the code).
In `@lib/services/compliance.ts`:
- Around line 95-102: The simulated latency is only invoked in getUserCompliance
via simulateDelay, causing inconsistent async behavior across the service;
update the service so all public async methods (e.g., getRemainingLimits,
validateWithdrawalAmount, trackWithdrawal, upgradeTier, and any other exported
async functions) also await simulateDelay at their start (or refactor delay
invocation into a shared wrapper/helper that each public method calls) to ensure
consistent DB-latency simulation and prevent timing-related inconsistencies in
callers.
- Around line 85-90: getComplianceRecord currently returns the live object from
MOCK_COMPLIANCE_DB so mutations persist implicitly, but callers like
trackWithdrawal, upgradeTier and setHoldState assume and redundantly reassign
MOCK_COMPLIANCE_DB[userId]; make this explicit and defensive by adding a short
comment on getComplianceRecord that it returns a direct reference (or
alternatively returns a clone), and update mutation methods (trackWithdrawal,
upgradeTier, setHoldState) to always write the mutated object back to
MOCK_COMPLIANCE_DB[userId] after changing fields so persistence does not depend
on reference semantics; reference getComplianceRecord, MOCK_COMPLIANCE_DB,
trackWithdrawal, upgradeTier and setHoldState when applying the change.
In `@lib/services/geo-restriction.ts`:
- Around line 150-152: getRestrictedJurisdictions currently returns a shallow
copy of the module array (RESTRICTED) but preserves object references so callers
can mutate the returned RestrictedJurisdiction objects; change
getRestrictedJurisdictions to return new object copies for each entry (e.g., map
over RESTRICTED and clone each RestrictedJurisdiction) so callers cannot mutate
module-level state—update the method that references RESTRICTED and consider
using object spread or a cloning utility (or Object.freeze on each returned
item) to ensure immutability of returned entries.
- Around line 91-107: detectVPN and detectProxy are currently silent
placeholders so callers (UserLocation.isVPN / isProxy) can't tell results are
unreliable; change both detectVPN and detectProxy to return a structured
DetectionResult (e.g., { isDetected: boolean, detectionConfidence: number,
isStub: boolean }) instead of a bare boolean, set isStub = true and
detectionConfidence = 0 for the current mocked logic, and update
UserLocation.isVPN / isProxy to read the new fields (or fail if an environment
flag like REQUIRE_REAL_DETECTION is set) so callers can distinguish stubbed
results from real detections and act accordingly.
- Around line 76-86: When checking state-level restrictions you must perform a
state-only lookup instead of calling getRestrictionReason(countryCode,
regionCode) which returns a country match first and hides state reasons; update
the region block that reads mockLocation.regionCode to fetch the state
restriction reason directly (use the same state-only lookup mechanism used by
isRestricted or add/call a getStateRestrictionReason(countryCode, regionCode)
that checks only the region map) and then set mockLocation.isRestricted and
mockLocation.restrictionReason from that state-specific result so state reasons
aren’t overwritten by country-level matches.
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 by CodeRabbit
New Features
Bug Fixes
Chores