Conversation
… in in-app routes and Stripe return_url
…ing, webhook + tests)
…, and signed status token
…elper + ops docs (Stripe unchanged)
…t dedupe + claim/lease TTL, paid terminal, mismatch→needs_review)
… payment-state + fix test cleanup
revert(auth,db): rollback oauth/db env changes to restore auth flow
…gin-redirect fix(proxy): handle missing locale correctly for /dashboard redirects
…itle Update Home title and use subtitle for OG metadata
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✅ Deploy Preview for develop-devlovers ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds Monobank PSP integration (invoice lifecycle, refunds, cancels, webhooks), DB schema/migrations and snapshots, multi-provider checkout (Stripe + Monobank), status-token access, Sentry instrumentation, leaderboard avatar refactor, many tests and docs, and a Safe-chain CI security workflow. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CheckoutRoute as Checkout Route
participant OrderService as Order Service
participant MonobankSvc as Monobank PSP Adapter
participant Database
Client->>CheckoutRoute: POST /api/shop/checkout (provider: monobank)
CheckoutRoute->>OrderService: createOrderWithItems(requestedProvider='monobank')
OrderService->>Database: INSERT order (provider=monobank)
OrderService->>MonobankSvc: createMonoAttemptAndInvoice(orderId, redirectUrl, webhookUrl)
MonobankSvc->>Database: INSERT payment_attempt (status=creating)
MonobankSvc->>MonobankSvc: buildMerchantPaymInfo(validate amounts, UAH)
MonobankSvc->>MonobankSvc: requestMono.createInvoice(payload)
MonobankSvc->>Database: UPDATE payment_attempt (invoiceId, pageUrl)
MonobankSvc-->>OrderService: return invoiceId,pageUrl,attemptId
OrderService-->>Client: 201 { order, pageUrl, attemptId }
sequenceDiagram
participant Monobank as Monobank PSP
participant WebhookRoute as Webhook Route
participant Verify as VerifyPubKey
participant WebhookService as Webhook Service
participant Database
participant PaymentState as Payment State
Monobank->>WebhookRoute: POST /api/shop/webhooks/monobank (raw + signature)
WebhookRoute->>Verify: verifyWebhookSignatureWithRefresh(raw, signature)
Verify-->>WebhookRoute: valid / invalid
WebhookRoute->>Database: compute rawSha256, check monobank_events (dedupe)
alt deduped
Database-->>WebhookRoute: already processed -> 200 OK
else new event
WebhookRoute->>Database: INSERT monobank_event
WebhookRoute->>WebhookService: handleMonobankWebhook(parsed, eventKey, mode)
WebhookService->>Database: fetch order & attempt, claim event
WebhookService->>PaymentState: guardedPaymentStatusUpdate(...)
WebhookService->>Database: UPDATE orders/payment_attempts, persist event appliedResult
WebhookService-->>WebhookRoute: appliedResult
WebhookRoute-->>Monobank: 200 OK
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
frontend/components/leaderboard/types.ts (1)
1-9:⚠️ Potential issue | 🔴 Critical
idis repurposed as rank, causing a matching bug inLeaderboardTable.In
leaderboard.ts(line 36),idis set toindex + 1(i.e., the rank), whileuserIdholds the actual database ID. However, inLeaderboardTable.tsx(line 29), the current-user matching logic comparesString(u.id) === normalizedCurrentUserId— this compares the rank against the current user's actual ID, so it will almost never match correctly.Either rename
idto something likerank(sincerankis already a duplicate) or fix the matching inLeaderboardTableto useuserId:// In LeaderboardTable.tsx const matchedUser = users.find( u => - String(u.id) === normalizedCurrentUserId || + String(u.userId) === normalizedCurrentUserId || (currentUsername && u.username === currentUsername) );And likewise for the
isMecheck at line 60:const isMe = - String(user.id) === normalizedCurrentUserId || + String(user.userId) === normalizedCurrentUserId || (currentUsername && user.username === currentUsername);frontend/db/queries/leaderboard.ts (1)
26-43:⚠️ Potential issue | 🔴 Critical
idandrankare identical — confusing and causes a bug downstream.Both
id(line 36) andrank(line 38) are set toindex + 1. Theidfield on theUsertype shadows the concept of a database ID, which leads to the matching bug inLeaderboardTable(whereuser.idis compared against the actual user ID). Consider removing the syntheticidor renaming it to avoid ambiguity.frontend/components/leaderboard/LeaderboardPodium.tsx (1)
63-65:⚠️ Potential issue | 🟡 Minor
key={user.id}uses rank, not a stable user identifier.Since
user.idisindex + 1(the rank), swapping two users' positions between renders will reuse the same DOM/animation nodes rather than remounting. For correct Framer Motion animations on reorder, useuser.userIdas the key.- key={user.id} + key={user.userId}frontend/components/leaderboard/LeaderboardTable.tsx (1)
58-65:⚠️ Potential issue | 🟡 Minor
key={user.id}should use a stable identifier.Same issue as in the podium —
user.idis the rank. Useuser.userIdfor stable React keys:- key={user.id} + key={user.userId}
🤖 Fix all issues with AI agents
In `@frontend/.env.example`:
- Line 105: Add a trailing newline at the end of .env.example and append
placeholder entries for the missing environment variables so developers can
discover required configuration; specifically add lines for MONO_WEBHOOK_MODE,
MONO_REFUND_ENABLED, MONO_PUBLIC_KEY, MONO_API_BASE, MONO_MERCHANT_TOKEN,
MONO_INVOICE_TIMEOUT_MS, MONO_WEBHOOK_CLAIM_TTL_MS, SHOP_STATUS_TOKEN_SECRET,
SHOP_BASE_URL, NEXT_PUBLIC_SENTRY_DSN, LOG_LEVEL, STRIPE_PAYMENTS_ENABLED,
STRIPE_WEBHOOK_INSTANCE_ID, WEBHOOK_INSTANCE_ID, GITHUB_TOKEN,
GITHUB_SPONSORS_TOKEN, LINKEDIN_FOLLOWER_COUNT, DATABASE_URL_LOCAL, and
NEXT_PUBLIC_ENABLE_ADMIN (each as empty placeholders), and ensure the file ends
with a single final newline character.
In `@frontend/db/queries/quiz.ts`:
- Around line 5-6: The file exports getUserLastAttemptPerQuiz and
getAttemptReviewDetails but references missing symbols (types UserLastAttempt,
AttemptReview, AttemptQuestionDetail and functions getCachedAttemptReview,
cacheAttemptReview), which breaks TypeScript; fix by either
restoring/implementing those missing types and cache helpers with the expected
shapes and signatures or refactor the two exported functions to use existing
cache utilities (e.g., getOrCreateQuestionsCache) and current types
(QuizQuestionWithAnswers) instead; locate references to UserLastAttempt in
getUserLastAttemptPerQuiz and to
AttemptReview/AttemptQuestionDetail/getCachedAttemptReview/cacheAttemptReview in
getAttemptReviewDetails and update imports/implementations accordingly so all
referenced symbols are defined.
In `@frontend/docs/payments/monobank/F0-report.md`:
- Around line 71-73: Update the docs to clarify that createMonoAttemptAndInvoice
and createMonobankAttemptAndInvoice are related: explain that
createMonoAttemptAndInvoice is the lower-level function which accepts explicit
redirectUrl and webhookUrl parameters, and createMonobankAttemptAndInvoice is a
convenience wrapper that builds those URLs internally (constructs
redirectUrl/webhookUrl) and then calls/delegates to createMonoAttemptAndInvoice;
add a short example sentence describing the wrapper pattern and mention both
function names (createMonoAttemptAndInvoice, createMonobankAttemptAndInvoice) so
readers understand the delegation.
In `@frontend/lib/env/index.ts`:
- Around line 21-22: Add JSDoc above the PAYMENTS_ENABLED and
STRIPE_PAYMENTS_ENABLED schema entries documenting that STRIPE_PAYMENTS_ENABLED
is a Stripe-specific override that, when unset, falls back to the global
PAYMENTS_ENABLED value; state the accepted values ('true'|'false'), the default
for PAYMENTS_ENABLED ('false'), and the intended precedence
(STRIPE_PAYMENTS_ENABLED > PAYMENTS_ENABLED). Update the comments near the
z.enum definitions for PAYMENTS_ENABLED and STRIPE_PAYMENTS_ENABLED so future
readers of the schema understand the fallback behavior used by the checkout
logic.
In `@frontend/lib/env/monobank.ts`:
- Around line 54-64: Remove the duplicated implementation by keeping a single
parsing helper (pick parsePositiveInt) and delete parseTimeoutMs; then replace
all usages of parseTimeoutMs with parsePositiveInt (including the call currently
using parseTimeoutMs) so callers use the deduplicated function, ensuring the
function signature and exported identifiers remain consistent after removal.
In `@frontend/lib/services/orders/monobank-refund.ts`:
- Around line 375-380: The code throws PspUnavailableError when refundRowForPsp
is null, but this represents an internal invariant failure, not a PSP outage;
replace the PspUnavailableError usage with an assertion/internal invariant error
(e.g., throw new Error or a dedicated InvariantError/AssertionError) in the same
location, update the message to indicate an internal invariant violation
(mention refundRowForPsp not initialized), and keep the metadata payload
(orderId and requestId) when throwing so operators still get context; update any
tests/handlers that expect PspUnavailableError where necessary (search for
refundRowForPsp and PspUnavailableError to locate the change).
- Around line 295-373: The conflict-handling branch duplicates the same
reconcile → deduped-check → retryable-check → paid-guard → retry logic already
used for existing refunds; extract that block into a single helper (e.g.,
handleExistingRefund(reconciled, orderRow, args)) that performs
reconcileSuccessFromOrder handling, uses
isDedupedRefundStatus/isRetryableRefundStatus, throws REFUND_ORDER_NOT_PAID when
orderRow.paymentStatus !== 'paid', performs the monobankRefunds update to set
status/providerModifiedAt/updatedAt and returns the final refund row and deduped
flag, then replace the duplicated code in both the existing-refund path and the
conflict path with calls to this helper and re-use getOrderById/mapRefundRow for
the returned result.
- Around line 412-427: The cancel flow can call cancelInvoicePayment and then
fail to persist the 'processing' refund state, causing duplicate non-idempotent
cancels; make cancellation idempotent by persisting a cancel-operation record
keyed by invoiceId + extRef before calling cancelInvoicePayment (create a unique
monobank_cancel_operations row with fields invoiceId, extRef, status), check
that table at start of the flow and if an entry exists reconcile its status
(query Monobank invoice status if needed) instead of re-sending cancel; on
successful cancel update both monobank_cancel_operations.status and the refund
row (monobankRefunds) together (preferably in a transaction or by using a status
transition that’s resilient to partial failures), and use refundRowForPsp,
cancelInvoicePayment, mapRefundRow and getOrderById to locate where to add these
checks/updates.
In `@frontend/lib/services/orders/monobank-webhook.ts`:
- Around line 543-584: The out-of-order check in
applyWebhookToMatchedOrderAttemptEvent currently uses providerModifiedAt <=
attemptProviderModifiedAt which treats same-timestamp events as stale; change
the comparison to providerModifiedAt < attemptProviderModifiedAt so events with
identical providerModifiedAt are allowed to apply, keep the rest of the branch
that persists an 'applied_noop' with appliedErrorCode 'OUT_OF_ORDER' and return
buildApplyOutcome unchanged; verify references to attemptProviderModifiedAt,
providerModifiedAt, persistEventOutcome, and buildApplyOutcome when making the
change.
- Around line 741-818: The TOCTOU happens because transitionPaymentStatus(...)
is called before atomicMarkPaidOrderAndSucceedAttempt(...), leaving payment
status = 'paid' if the atomic DB update fails; fix by making the two operations
atomic or adding a compensating rollback: either (A) move both updates into a
single DB transaction (wrap the logic so transitionPaymentStatus and
atomicMarkPaidOrderAndSucceedAttempt execute inside one transaction) or (B) if a
single transaction isn't possible, on failure of
atomicMarkPaidOrderAndSucceedAttempt call a new compensating function (e.g.,
revertPaymentStatus or transitionPaymentStatus with the previous status) to undo
the 'paid' transition and include that result in persistEventOutcome; update
error handling around transitionPaymentStatus and
atomicMarkPaidOrderAndSucceedAttempt to attempt the rollback and surface its
success/failure in the applied_with_issue outcome.
In `@frontend/lib/tests/shop/checkout-monobank-idempotency-contract.test.ts`:
- Around line 508-524: The test creates an idempotent checkout (idemKey) via
postCheckout but only calls cleanupProduct(productId) in finally, so partial
orders created before the price validation fail may remain; update the finally
block to also query and delete any orders associated with the test idempotency
key (use idemKey) — e.g., call the existing order lookup/delete helpers (or add
helpers like findOrdersByIdempotencyKey and deleteOrder) after cleanupProduct to
remove any order created by postCheckout, ensuring
createIsolatedProduct/productId and idemKey are referenced so the cleanup
targets the test artifacts.
In `@frontend/lib/tests/shop/monobank-psp-unavailable.test.ts`:
- Around line 81-121: createIsolatedProduct currently queries the DB for an
existing active product and throws if none exists, causing test order
dependency; update the test to ensure a template exists by either calling the
existing seedTemplateProductIfMissing helper (or adding a similar seeding
helper) before running the select in createIsolatedProduct, or modify
createIsolatedProduct to create and insert a minimal template product when the
select returns no rows, then clone that template for the isolated product (refer
to createIsolatedProduct and seedTemplateProductIfMissing to locate the logic).
In `@frontend/lib/tests/shop/monobank-webhook-apply-outcomes.test.ts`:
- Around line 144-158: The fallback in insertAttempt currently calls
pickEnumLabelByColumn for payment_attempts.status which breaks when the column
is text with a CHECK constraint; change insertAttempt so it does not rely on
pickEnumLabelByColumn for this column—use a safe hardcoded fallback list (e.g.
['pending','created','requires_action']) or otherwise detect a non-enum UDT and
choose from that list; update the logic around insertAttempt (and the status
assignment) to pick from the explicit array when args.status is undefined
instead of calling pickEnumLabelByColumn.
- Around line 258-265: The test inserts use insertAttempt with status:
'pending', which violates the database CHECK constraint
payment_attempts_status_check; update each insertAttempt call (the ones around
the monobank webhook tests that set status currently to 'pending') to use
status: 'active' instead (active is allowed, matches the column default and
semantics for awaiting webhook notification) so the INSERTs succeed and the
webhook logic can execute.
In `@frontend/messages/pl.json`:
- Line 47: The translation keys "dashboard.quizResults" and
"dashboard.quizReview" were removed from pl.json but are still referenced by
components; either restore those keys in pl.json or update the components to
stop using them before removing; specifically, either re-add the keys to pl.json
or refactor QuizResultRow.tsx, QuizResultsSection.tsx (which use
dashboard.quizResults) and quiz-review/[attemptId]/page.tsx, QuizReviewList.tsx,
QuizReviewCard.tsx (which use dashboard.quizReview) to use new keys or safe
fallbacks, then remove the keys once all references are updated.
🧹 Nitpick comments (50)
frontend/lib/tests/shop/payment-state-legacy-writers.test.ts (1)
103-111: Template literal${…}interpolations are not tracked.
findMatchingBracetreats template literals as opaque (skip until closing backtick), which means nested template literals (`a ${`b`} c`) would incorrectly toggle theinTemplateflag. This is unlikely to cause false positives in your service files today, but it's worth a brief comment in the code noting this limitation so future maintainers aren't surprised.frontend/docs/payments/monobank/E0-gap-report.md (3)
113-113: Clarify the proposed table strategy.The proposal mentions both
monobank_events(provider-specific) andpsp_events(generic with provider field) as examples. These represent different architectural approaches:
- Provider-specific table (
monobank_events): Mirrors the existingstripe_eventspattern, simpler queries, but requires a new table per provider.- Generic table (
psp_eventswithproviderfield): Single unified table, but requires filtering by provider in all queries.Consider documenting which approach is preferred or the trade-offs between them to guide implementation decisions.
123-123: Document theMONO_REFUND_ENABLEDenvironment variable in the FACTS section.This line references
MONO_REFUND_ENABLED=falsebut this configuration isn't documented in the FACTS section. For completeness, consider adding a subsection documenting Monobank-related environment variables (similar to the reference toMONO_WEBHOOK_MODEon line 116).
121-126: Consider adding priority indicators to the gaps.The identified gaps are legitimate, but they appear to have different levels of urgency:
- Refund functionality might be critical for production readiness
- Claim/lease fields are important for multi-instance deployments to prevent race conditions
- Processing markers improve observability
Consider adding priority levels (e.g., P0/P1/P2 or Critical/High/Medium) to help stakeholders understand which gaps should be addressed first.
frontend/db/queries/leaderboard.ts (1)
28-33: Fallback avatar URL is duplicated inUserAvatar.tsx.The Dicebear fallback URL and seed logic (
${username}-${userId}) is duplicated here (server side) and inUserAvatar.tsx(client side, line 25-26). If either changes independently, users will see mismatched avatars between SSR and client error fallback. Consider extracting a shared utility, or relying solely onUserAvatar's client-side fallback and passing the raw (possibly empty) avatar from the query.frontend/components/leaderboard/UserAvatar.tsx (1)
38-38: Consider guarding against infinite fallback errors.If the Dicebear service is unreachable,
onErrorfires on the fallback too. SincehasErroris alreadytrue, state won't change and there's no infinite loop — but the image will remain broken. A small improvement would be to track a second failure and render a plain CSS/text placeholder instead..github/workflows/security.yml (1)
20-21: Pin GitHub Actions to commit SHAs for better supply-chain security.GitHub's official security-hardening guidance recommends pinning all actions to full commit SHAs rather than mutable tags like
@v4, since tags can be moved or deleted if the action repository is compromised. This is especially important for security-focused workflows.The recommended pattern is to pin to the SHA while keeping the tag in a comment for readability:
Example
- uses: actions/checkout@v4 + uses: actions/checkout@a81bbbf8298c0fa03ea29cdc473d45eba93ac6d7 # v4.1.7Apply this to both
actions/checkoutandactions/setup-node.frontend/lib/services/orders/payment-state.ts (1)
17-31: Consider renamingALLOWED_FROM_STRIPEnow that it covers Monobank too.Since
allowedFrom()on line 46 uses this table for all non-'none'providers (including'monobank'), the nameALLOWED_FROM_STRIPEis misleading. Something likeALLOWED_FROM_PSPwould better reflect its scope.Also,
'needs_review'in its own allowed-from list (line 29) is redundant —isAllowed()already short-circuits onfrom === toat line 54.frontend/lib/tests/shop/monobank-webhook-paid-reversal.test.ts (2)
29-39: Cleanup deletion order is correct for FK constraints — consider wrapping in a transaction for robustness.The delete order (event → attempt → order) respects FK dependencies. However, if any intermediate delete fails, subsequent deletes won't run, potentially leaving orphaned test data that could interfere with other sequential tests.
♻️ Optional: wrap cleanup in a transaction
async function cleanup( orderId: string, attemptId: string, eventId: string | null ) { - if (eventId) { - await db.delete(monobankEvents).where(eq(monobankEvents.id, eventId)); - } - await db.delete(paymentAttempts).where(eq(paymentAttempts.id, attemptId)); - await db.delete(orders).where(eq(orders.id, orderId)); + await db.transaction(async (tx) => { + if (eventId) { + await tx.delete(monobankEvents).where(eq(monobankEvents.id, eventId)); + } + await tx.delete(paymentAttempts).where(eq(paymentAttempts.id, attemptId)); + await tx.delete(orders).where(eq(orders.id, orderId)); + }); }
59-84: DB inserts useas anyto bypass schema type checks.This is pragmatic for test setup, but if the schema evolves (e.g., a new required column is added), these inserts will silently produce incomplete rows rather than giving a compile-time error. Just flagging for awareness — acceptable in test code.
frontend/lib/env/index.ts (1)
33-37: Numeric env vars validated as plain strings — early validation at schema boundary would be safer.
MONO_INVOICE_VALIDITY_SECONDS,MONO_TIME_SKEW_TOLERANCE_SEC, andMONO_INVOICE_TIMEOUT_MSare validated asz.string()but represent numeric values. While downstream code inmonobank.tsapplies defensive parsing viaparsePositiveInt()andparseTimeoutMs()(which safely fallback to defaults on invalid input), non-numeric values like"86400s"would pass schema validation before being caught and silently coerced to fallbacks.Consider using
z.coerce.number().int().positive()to catch invalid values at the schema boundary instead of relying on downstream fallback logic:♻️ Example using coerce
- MONO_INVOICE_VALIDITY_SECONDS: z.string().optional().default('86400'), - MONO_TIME_SKEW_TOLERANCE_SEC: z.string().optional().default('300'), + MONO_INVOICE_VALIDITY_SECONDS: z.coerce.number().int().positive().optional().default(86400), + MONO_TIME_SKEW_TOLERANCE_SEC: z.coerce.number().int().positive().optional().default(300), MONO_PUBLIC_KEY: z.string().min(1).optional(), MONO_API_BASE: z.string().url().optional(), - MONO_INVOICE_TIMEOUT_MS: z.string().optional(), + MONO_INVOICE_TIMEOUT_MS: z.coerce.number().int().positive().optional(),frontend/lib/tests/shop/stripe-webhook-mismatch.test.ts (2)
63-63: Avoidsql.rawwith interpolated values — use a parameterized cast instead.Even though
safeCurrencyLiteralconstrains the value to'USD' | 'UAH', usingsql.rawwith string interpolation establishes a risky pattern that could be copied elsewhere without the same safeguard. Drizzle supports parameterized casts:Proposed fix
- ${sql.raw(`'${currency}'`)}, + ${currency}::currency,Or use drizzle's
sqltemplate properly:- ${sql.raw(`'${currency}'`)}, + ${sql`${currency}::currency`},
26-28:safeCurrencyLiteralis a no-op for'USD'input.The function returns
'UAH'only when input is'UAH', otherwise always'USD'. Since it's only called with'USD'on Line 124, it currently does nothing. If intended as a general-purpose guard, consider validating against both known values and throwing on unexpected input rather than silently defaulting.frontend/lib/env/monobank.ts (2)
21-33:MonobankConfigtype is referenced before its declaration on Line 36.TypeScript hoists type aliases so this compiles, but for readability move the type declaration above
getMonobankConfig().
106-123: Redundant branching — both branches return the same structure.The
if (!paymentsEnabled)branch and theelsebranch construct identical objects differing only inpaymentsEnabled. Simplify:Proposed fix
- if (!paymentsEnabled) { - return { - token, - apiBaseUrl, - paymentsEnabled: false, - invoiceTimeoutMs, - publicKey, - }; - } - - return { - token, - apiBaseUrl, - paymentsEnabled: true, - invoiceTimeoutMs, - publicKey, - }; + return { + token, + apiBaseUrl, + paymentsEnabled, + invoiceTimeoutMs, + publicKey, + };frontend/lib/tests/shop/monobank-refund-route-f4.test.ts (1)
77-119: Consider typing insert helpers to avoidas anycasts.The
as anycasts on Lines 92, 118, and 247 bypass type-checking on DB inserts, which could mask schema drift. If some columns have defaults or are optional in the schema, the insert type should already accept partial values. If the issue is specific columns, a targetedOmitor an explicit type assertion on just those fields would be safer.frontend/lib/shop/status-token.ts (1)
13-20:getSecret()readsprocess.envdirectly instead of using the env module.The rest of the codebase resolves env vars through
getServerEnv()(seefrontend/lib/env/index.ts). Reading directly fromprocess.envmeans this bypasses schema validation and caching. Consider whether this should use the centralized env access pattern for consistency.frontend/app/api/shop/orders/[id]/status/route.ts (1)
69-111: Token verification reason is not logged for non-missing_secretfailures.When the token is invalid (Line 104-110), you log
STATUS_TOKEN_INVALIDbut don't includetokenResult.reason. This would help debug token issues in production (e.g., distinguishingexpiredfrominvalid_signature).Proposed fix
logWarn('order_status_token_invalid', { requestId, orderId, code: 'STATUS_TOKEN_INVALID', + reason: tokenResult.reason, durationMs: Date.now() - startedAtMs, });frontend/lib/psp/monobank/merchant-paym-info.ts (1)
40-46: Setthis.nameon the custom error class for better stack traces.When extending
Error, thenameproperty defaults to'Error'. Settingthis.name = 'MonobankMerchantPaymInfoError'(orthis.constructor.name) improves debuggability in logs and stack traces.Proposed fix
export class MonobankMerchantPaymInfoError extends Error { code: string; constructor(code: string, message: string) { super(message); + this.name = 'MonobankMerchantPaymInfoError'; this.code = code; } }frontend/lib/tests/shop/monobank-merchant-paym-info.test.ts (1)
8-17:expectCodehelper swallows unexpected error types silently.If the function throws an error that is NOT a
MonoMerchantPaymInfoError, theexpect(error).toBeInstanceOf(MonoMerchantPaymInfoError)assertion will fail with a generic message. This is acceptable since Vitest will still report the failure, but you lose visibility into what was actually thrown. Consider adding a message or re-throwing for clarity.frontend/lib/services/orders/monobank/merchant-paym-info.ts (1)
11-28:safeLineTotalMinorsilently returns 0 for invalid inputs — downstream validation will catch it, but the error message may be confusing.When
safeLineTotalMinorreturns0(e.g., for non-integer or non-finite inputs), the downstreamparseIntegerStrictinbuildMonoMerchantPaymInfoFromSnapshotwill throw"item.lineTotalMinor must be positive"— which doesn't hint that the root cause was an invalid quantity or unit price. This is acceptable since the wrapper's callers should already validate types, but worth noting for debuggability.frontend/lib/psp/monobank.ts (2)
16-32:PspError.causeshadows the built-inError.causewithout passing it tosuper().Since ES2022,
Erroraccepts{ cause }in its options bag. Declaringcauseas a separate property shadows it and may confuse error-reporting tools that inspectError.cause. Consider passingcausethrough to the parent constructor:Proposed fix
constructor( code: PspErrorCode, message: string, safeMeta?: Record<string, unknown>, cause?: unknown ) { - super(message); + super(message, { cause }); this.code = code; this.safeMeta = safeMeta; - this.cause = cause; }
233-287: Significant duplication betweenbuildMonobankInvoicePayloadandbuildMonobankInvoicePayloadFromInput.Both functions perform nearly identical validation and payload construction (amount check, merchantPaymInfo extraction, reference/destination parsing, basketOrder parsing). The main differences are the input types and the extra reference-match check in the
FromInputvariant (line 529). Consider consolidating into one internal builder with a thin adapter layer for each input shape.Also applies to: 496-558
frontend/lib/tests/shop/monobank-webhook-crypto.test.ts (1)
33-39:makeResponsemock is missingheadersproperty.The
requestMonofunction inmonobank.ts(line 447) accessesres.headerson successful responses. The mock doesn't includeheaders, so it will beundefinedin the result. This works for these tests sincefetchWebhookPubKeydoesn't use the returned headers, but it could cause issues if the tested code path changes.Minimal fix
function makeResponse(body: string) { return { ok: true, status: 200, text: async () => body, + headers: new Headers(), }; }frontend/lib/tests/shop/monobank-http-client.test.ts (1)
5-29: Environment helper code is duplicated across test files.
ENV_KEYS,rememberEnv,restoreEnv, andpreviousEnvare repeated identically inmonobank-webhook-crypto.test.tsand here. Consider extracting to a shared test utility (e.g.,frontend/lib/tests/shop/__helpers/monobank-env.ts).frontend/lib/tests/shop/monobank-adapter.test.ts (2)
3-4: Combine imports from the same module.Both
buildMonobankInvoicePayloadandMONO_CCYare imported from the same module and can be merged into a single import statement.✏️ Suggested fix
-import { buildMonobankInvoicePayload } from '@/lib/psp/monobank'; -import { MONO_CCY } from '@/lib/psp/monobank'; +import { buildMonobankInvoicePayload, MONO_CCY } from '@/lib/psp/monobank';
7-32: Consider asserting additional payload fields for stronger coverage.The happy-path test only verifies
ccyandpaymentType. Key fields likeamount,redirectUrl,webHookUrl, andmerchantPaymInfo.reference/merchantPaymInfo.destinationare not asserted, so regressions in payload construction could go undetected.frontend/lib/services/orders/payment-attempts.ts (1)
178-179: Inconsistent null safety:inserted[0]!vs explicit check on Line 104-105.Line 104-105 in
createActiveAttemptproperly checks for a missing row and throws a descriptive error, but here a bare non-null assertion is used. If the insert somehow returns an empty array, this would silently produceundefinedat runtime.✏️ Suggested fix
- return inserted[0]!; + const row = inserted[0]; + if (!row) throw new Error('Failed to insert backfill payment_attempts row'); + return row;frontend/lib/tests/shop/monobank-env.test.ts (1)
67-67: Minor indentation inconsistency.Line 67 has an extra leading space before
it(...)compared to the other test cases.✏️ Suggested fix
- it('returns token when MONO_MERCHANT_TOKEN is set', () => { + it('returns token when MONO_MERCHANT_TOKEN is set', () => {frontend/lib/services/orders/attempt-idempotency.ts (1)
1-14: Asymmetric API between Stripe and Monobank key builders.
buildStripeAttemptIdempotencyKeyrequires aproviderparameter typed as the literal'stripe', making it redundant — callers can only ever pass'stripe'. Meanwhile,buildMonobankAttemptIdempotencyKeyhardcodes'mono'and omits a provider parameter entirely.Consider unifying the approach: either remove the
providerparam from the Stripe builder (hardcode'stripe'like Monobank hardcodes'mono'), or introduce a single generic builder:Option A: Remove redundant parameter
-export function buildStripeAttemptIdempotencyKey( - provider: 'stripe', - orderId: string, - attemptNo: number -): string { - return `pi:${provider}:${orderId}:${attemptNo}`; -} +export function buildStripeAttemptIdempotencyKey( + orderId: string, + attemptNo: number +): string { + return `pi:stripe:${orderId}:${attemptNo}`; +}frontend/lib/tests/shop/payment-attempt-idempotency-key.test.ts (1)
30-34: Consider adding a cross-order collision test for Monobank keys.The cross-order collision test only covers Stripe. Adding a similar assertion for Monobank would complete the coverage matrix.
frontend/app/api/shop/admin/orders/[id]/refund/route.ts (1)
99-141: DuplicatepaymentProvider === 'monobank'check can be consolidated.Lines 99 and 116 both guard on
targetOrder?.paymentProvider === 'monobank'. These can be merged into a single block:Suggested consolidation
if (targetOrder?.paymentProvider === 'monobank') { const { refundEnabled } = getMonobankConfig(); if (!refundEnabled) { logWarn('admin_orders_refund_disabled', { ...baseMeta, code: 'REFUND_DISABLED', orderId: orderIdForLog, durationMs: Date.now() - startedAtMs, }); return noStoreJson( { code: 'REFUND_DISABLED', message: 'Refunds are disabled.' }, { status: 409 } ); } - } - if (targetOrder?.paymentProvider === 'monobank') { const { requestMonobankFullRefund } = await import( '@/lib/services/orders/monobank-refund' ); ... + }frontend/lib/tests/shop/checkout-monobank-parse-validation.test.ts (1)
94-135: Mock return value usespaymentProvider: 'none'— intentional but worth a brief comment.The mock's return value uses
paymentProvider: 'none'andpaymentStatus: 'paid'to bypass Monobank/Stripe post-create flows, keeping the test focused on payload validation. Consider adding a brief inline comment explaining this choice to help future readers understand it's intentional and not a copy-paste oversight.frontend/lib/tests/shop/monobank-payments-disabled.test.ts (1)
86-96: Spreading the template product row may carry unintended fields.
...(tpl as any)copies every column from the template product, including fields likeid,slug,skuwhich are immediately overridden — that's fine. However, if the schema later gains columns with unique constraints or computed defaults, the spread could cause silent insertion failures. This is a minor robustness concern for test code.frontend/lib/tests/shop/monobank-api-methods.test.ts (1)
31-37:makeResponsemock lacksjsonandheaders— verify the PSP client only usestext().The mock only provides
ok,status, andtext(). IfcancelInvoicePaymentor other methods ever callres.json()or readres.headers, these tests will break silently. This is fine as long as the PSP HTTP client consistently usesres.text()for all Monobank endpoints.frontend/app/api/shop/webhooks/monobank/route.ts (3)
57-70: Duplicated webhook-mode resolution logic withgetMonobankConfig.
parseWebhookModeat line 16 duplicates the parsing inlib/env/monobank.ts. The two-tier resolution (direct env read → config fallback) adds complexity. SincegetMonobankConfig().webhookModealready readsprocess.env.MONO_WEBHOOK_MODE, you could simplify to a singlegetMonobankConfig()call wrapped in a try/catch, eliminating the localparseWebhookMode.♻️ Simplified mode resolution
- let webhookMode: WebhookMode = parseWebhookMode( - process.env.MONO_WEBHOOK_MODE - ); - if (!process.env.MONO_WEBHOOK_MODE) { - try { - webhookMode = parseWebhookMode(getMonobankConfig().webhookMode); - } catch (error) { - logError('monobank_webhook_mode_invalid', error, { - ...baseMeta, - code: 'MONO_WEBHOOK_MODE_INVALID', - }); - webhookMode = 'apply'; - } + let webhookMode: WebhookMode = 'apply'; + try { + webhookMode = getMonobankConfig().webhookMode; + } catch (error) { + logError('monobank_webhook_mode_invalid', error, { + ...baseMeta, + code: 'MONO_WEBHOOK_MODE_INVALID', + }); }
104-111: Returning 200 OK on invalid signatures is correct but warrants monitoring.This is the right pattern for webhook endpoints — non-200 responses would trigger provider retries. Ensure the
MONO_SIG_INVALIDwarning log is routed to your alerting pipeline so signature verification failures are surfaced quickly (e.g., key rotation issues, forged requests).
124-150: Unhandled webhook processing errors are silently swallowed.If
handleMonobankWebhookthrows, the error is logged but the response is still 200 OK. This is correct for preventing provider retries, but it means a persistent bug inhandleMonobankWebhookcould cause events to be permanently lost (never applied). Consider trackingWEBHOOK_APPLY_FAILEDerrors with a dead-letter or retry mechanism at the application level, separate from provider retries.frontend/app/api/shop/admin/orders/[id]/cancel-payment/route.ts (2)
109-122:dedupedis duplicated in the response body.
result.cancel.dedupedis included both inside thecancelobject (line 119, via spread) and as a top-leveldedupedproperty (line 121). If this is intentional for client convenience, consider adding a brief comment. Otherwise, remove the redundant top-level field.🔧 Remove duplicate field
cancel: { ...result.cancel, }, - deduped: result.cancel.deduped, });
22-26:noStoreJsonhelper is duplicated across admin routes.This utility appears in multiple route files (cancel-payment, refund, etc.). Consider extracting it to a shared module (e.g.,
@/lib/api/response) to reduce duplication.frontend/lib/services/orders/monobank-cancel-payment.ts (1)
161-176: Polling withsleepin a server-side request path.
pollRequestedCancelStatusbusy-waits for up to ~375ms (5 × 75ms) hitting the database on each iteration. For a low-traffic admin endpoint this is acceptable, but it adds latency to follower requests and DB load under concurrent cancel attempts.Consider documenting the polling budget or making the constants configurable for tuning.
frontend/lib/tests/shop/checkout-monobank-idempotency-contract.test.ts (1)
145-288: Dynamic schema introspection for test seeding is fragile.
seedTemplateProductIfMissingqueriesinformation_schema.columnsand builds INSERT statements dynamically based on column metadata. While this adapts to schema changes automatically, it's brittle (e.g., new column types, custom domains, or composite types could break theswitchon line 216) and difficult to debug when failures occur.Consider maintaining an explicit seed fixture that's updated alongside schema changes — it's simpler to read and debug, even if it requires occasional maintenance.
frontend/drizzle/0012_atomic_mono_cancel_payment.sql (1)
14-14: Consider whether theupdated_atcolumn should have a trigger for automatic updates.The
updated_atcolumn has a default ofnow()but no trigger to auto-update it on row modifications. The application code (drizzle's$onUpdate) handles this, which means direct SQL updates (e.g., manual fixes, migrations) won't auto-update the timestamp. This is consistent with other tables in the schema, so not a blocker.frontend/lib/services/orders/monobank-webhook.ts (2)
163-173: Seconds-vs-milliseconds heuristic inparseTimestampMsis reasonable but fragile for edge-case timestamps.The boundary
1e11correctly distinguishes current-era Unix seconds (≈1.7e9) from milliseconds (≈1.7e12). However, the boundary itself (year ~5138 in seconds, year ~1973 in milliseconds) means any millisecond timestamp before March 1973 would be misinterpreted as seconds and multiplied by 1000. This is unlikely for Monobank production data but worth a brief inline comment documenting the assumption.
194-196:buildEventKeyis a pass-through — consider inlining or documenting the intent for future divergence.Currently
buildEventKey(rawSha256)returnsrawSha256unchanged. If the intent is to allow future event key formats (e.g., composite keys), a brief comment would clarify this. Otherwise, removing the indirection would reduce cognitive load.frontend/lib/services/orders/monobank.ts (3)
249-343:finalizeAttemptWithInvoiceretry loop has no backoff delay between attempts.The loop retries the read-modify-update sequence immediately upon failure. For transient DB issues (connection pool exhaustion, brief locks), a short delay (e.g., 50–200ms) between attempts would improve resilience. Also,
maxRetries = 2reads as "2 retries" but it's actually "2 total tries" — consider renaming for clarity.🔧 Add a small delay and clarify naming
- const maxRetries = 2; + const maxTries = 2; let lastError: unknown = null; let fallbackError: unknown = null; const now = new Date(); // ... helpers ... - for (let attempt = 0; attempt < maxRetries; attempt += 1) { + for (let attempt = 0; attempt < maxTries; attempt += 1) { + if (attempt > 0) { + await new Promise(resolve => setTimeout(resolve, 100)); + } try {
660-672:PspUnavailableErrorconstructor receivescancelErrordetails viaas any— consider using a typed options bag.The
as anycast on line 672 bypasses type checking for the error constructor's second argument. IfPspUnavailableErroraccepts a typed options object, consider extending its type to includecauseandattemptIdto remove the cast.
721-724:__test__export exposes internals for testing — ensure it's not consumed in production code.Exporting internal implementations via
__test__is a common pattern, but it bypasses encapsulation. Consider adding a comment or a lint rule to prevent accidental production usage.frontend/app/api/shop/checkout/route.ts (2)
331-406:runMonobankCheckoutFlow— the lazy import and amount sanity check are good, but log level for lazy import could be reduced.Line 340:
logInfo('monobank_lazy_import_invoked', ...)fires on every Monobank checkout. This is noisy for production; considerlogDebugor removing it after initial rollout.
468-476:monobankRequestHinttriggers Monobank-specific error codes even before provider validation completes.When the client sends
provider: "mono",monobankRequestHintbecomestrue(line 473-475), so missing/invalid idempotency key errors returnINVALID_REQUEST(lines 487, 508) instead of the Stripe-oriented codesMISSING_IDEMPOTENCY_KEY/INVALID_IDEMPOTENCY_KEY. The request then hits the provider validation at line 542-544 and is rejected as invalid anyway. This means clients using"mono"get Monobank-style error codes for pre-validation issues but a generic rejection for the provider itself. This is consistent with the security posture (not leaking provider support info), but documenting this intentional behavior would help future maintainers.Also applies to: 486-498, 507-521
| RATE_LIMIT_DISABLED=0 | ||
|
|
||
| GROQ_API_KEY= | ||
| GROQ_API_KEY= No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Check trailing newline in frontend/.env.example ==="
if [ -f "frontend/.env.example" ]; then
# Check if file ends with newline
if [ -n "$(tail -c 1 frontend/.env.example)" ]; then
echo "Missing trailing newline: YES"
else
echo "Missing trailing newline: NO"
fi
echo "Last 5 lines of file:"
tail -5 frontend/.env.example | cat -A
fi
echo ""
echo "=== Env vars in frontend/.env.example ==="
if [ -f "frontend/.env.example" ]; then
grep -oP '^[A-Z_]+' frontend/.env.example | sort -u
fi
echo ""
echo "=== Search for env vars in frontend code (env/ directory) ==="
find frontend -path '*/env/*' -type f | head -10
echo ""
echo "=== Env vars referenced in frontend/lib/env/ or similar ==="
find frontend -name "*.ts" -o -name "*.tsx" -o -name "*.js" -o -name "*.jsx" | xargs grep -h 'process\.env\.' 2>/dev/null | grep -oP 'process\.env\.\K[A-Z_]+' | sort -uRepository: DevLoversTeam/devlovers.net
Length of output: 3238
Add trailing newline and update .env.example with missing environment variables.
Two issues:
-
Trailing newline: The file is missing a final newline. Add one after line 105.
-
Missing environment variables: Several env vars referenced in the codebase are not documented in
.env.example, making it harder for developers to discover required configuration. Add placeholders for:- Monobank:
MONO_WEBHOOK_MODE,MONO_REFUND_ENABLED,MONO_PUBLIC_KEY,MONO_API_BASE,MONO_MERCHANT_TOKEN,MONO_INVOICE_TIMEOUT_MS,MONO_WEBHOOK_CLAIM_TTL_MS - Shop:
SHOP_STATUS_TOKEN_SECRET,SHOP_BASE_URL - Monitoring:
NEXT_PUBLIC_SENTRY_DSN,LOG_LEVEL - Payments:
STRIPE_PAYMENTS_ENABLED,STRIPE_WEBHOOK_INSTANCE_ID,WEBHOOK_INSTANCE_ID - Other:
GITHUB_TOKEN,GITHUB_SPONSORS_TOKEN,LINKEDIN_FOLLOWER_COUNT,DATABASE_URL_LOCAL,NEXT_PUBLIC_ENABLE_ADMIN
- Monobank:
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 105-105: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
🤖 Prompt for AI Agents
In `@frontend/.env.example` at line 105, Add a trailing newline at the end of
.env.example and append placeholder entries for the missing environment
variables so developers can discover required configuration; specifically add
lines for MONO_WEBHOOK_MODE, MONO_REFUND_ENABLED, MONO_PUBLIC_KEY,
MONO_API_BASE, MONO_MERCHANT_TOKEN, MONO_INVOICE_TIMEOUT_MS,
MONO_WEBHOOK_CLAIM_TTL_MS, SHOP_STATUS_TOKEN_SECRET, SHOP_BASE_URL,
NEXT_PUBLIC_SENTRY_DSN, LOG_LEVEL, STRIPE_PAYMENTS_ENABLED,
STRIPE_WEBHOOK_INSTANCE_ID, WEBHOOK_INSTANCE_ID, GITHUB_TOKEN,
GITHUB_SPONSORS_TOKEN, LINKEDIN_FOLLOWER_COUNT, DATABASE_URL_LOCAL, and
NEXT_PUBLIC_ENABLE_ADMIN (each as empty placeholders), and ensure the file ends
with a single final newline character.
| - `createMonoAttemptAndInvoice(...)` in `frontend/lib/services/orders/monobank.ts` | ||
| - `createMonobankAttemptAndInvoice(...)` in `frontend/lib/services/orders/monobank.ts` | ||
| - Webhook apply side: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n --type=ts '\b(createMonoAttemptAndInvoice|createMonobankAttemptAndInvoice)\b' -g '!*.test.*' frontend/Repository: DevLoversTeam/devlovers.net
Length of output: 582
🏁 Script executed:
sed -n '693,745p' frontend/lib/services/orders/monobank.tsRepository: DevLoversTeam/devlovers.net
Length of output: 1307
🏁 Script executed:
sed -n '726,760p' frontend/lib/services/orders/monobank.tsRepository: DevLoversTeam/devlovers.net
Length of output: 823
Clarify the relationship between createMonoAttemptAndInvoice and createMonobankAttemptAndInvoice in the documentation.
Both functions exist and are intentional: createMonoAttemptAndInvoice is a lower-level function accepting explicit redirectUrl and webhookUrl parameters, while createMonobankAttemptAndInvoice is a wrapper that constructs these URLs internally before delegating to the first function. The documentation should note this wrapper pattern to avoid reader confusion.
🤖 Prompt for AI Agents
In `@frontend/docs/payments/monobank/F0-report.md` around lines 71 - 73, Update
the docs to clarify that createMonoAttemptAndInvoice and
createMonobankAttemptAndInvoice are related: explain that
createMonoAttemptAndInvoice is the lower-level function which accepts explicit
redirectUrl and webhookUrl parameters, and createMonobankAttemptAndInvoice is a
convenience wrapper that builds those URLs internally (constructs
redirectUrl/webhookUrl) and then calls/delegates to createMonoAttemptAndInvoice;
add a short example sentence describing the wrapper pattern and mention both
function names (createMonoAttemptAndInvoice, createMonobankAttemptAndInvoice) so
readers understand the delegation.
| PAYMENTS_ENABLED: z.enum(['true', 'false']).optional().default('false'), | ||
| STRIPE_PAYMENTS_ENABLED: z.enum(['true', 'false']).optional(), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how both flags are consumed in the codebase
rg -n --type=ts 'PAYMENTS_ENABLED|STRIPE_PAYMENTS_ENABLED' -g '!*.test.*' -g '!*.md'Repository: DevLoversTeam/devlovers.net
Length of output: 644
🏁 Script executed:
#!/bin/bash
# Check context around the usage in checkout/route.ts
sed -n '560,575p' frontend/app/api/shop/checkout/route.tsRepository: DevLoversTeam/devlovers.net
Length of output: 562
🏁 Script executed:
#!/bin/bash
# Check for any comments or documentation around the env variables in index.ts
sed -n '15,30p' frontend/lib/env/index.tsRepository: DevLoversTeam/devlovers.net
Length of output: 775
Document the relationship between PAYMENTS_ENABLED and STRIPE_PAYMENTS_ENABLED in the schema.
The usage in frontend/app/api/shop/checkout/route.ts (lines 565–576) shows that STRIPE_PAYMENTS_ENABLED is a Stripe-specific override that falls back to PAYMENTS_ENABLED when not set. This relationship should be clarified at the schema definition level with JSDoc comments to prevent configuration confusion, as developers reading just lines 21–22 won't understand the precedence rules.
🤖 Prompt for AI Agents
In `@frontend/lib/env/index.ts` around lines 21 - 22, Add JSDoc above the
PAYMENTS_ENABLED and STRIPE_PAYMENTS_ENABLED schema entries documenting that
STRIPE_PAYMENTS_ENABLED is a Stripe-specific override that, when unset, falls
back to the global PAYMENTS_ENABLED value; state the accepted values
('true'|'false'), the default for PAYMENTS_ENABLED ('false'), and the intended
precedence (STRIPE_PAYMENTS_ENABLED > PAYMENTS_ENABLED). Update the comments
near the z.enum definitions for PAYMENTS_ENABLED and STRIPE_PAYMENTS_ENABLED so
future readers of the schema understand the fallback behavior used by the
checkout logic.
| function parseTimeoutMs(raw: string | undefined, fallback: number): number { | ||
| const v = raw ? Number.parseInt(raw, 10) : NaN; | ||
| if (!Number.isFinite(v) || v <= 0) return fallback; | ||
| return v; | ||
| } | ||
|
|
||
| function parsePositiveInt(raw: string | undefined, fallback: number): number { | ||
| const v = raw ? Number.parseInt(raw, 10) : NaN; | ||
| if (!Number.isFinite(v) || v <= 0) return fallback; | ||
| return v; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
parseTimeoutMs and parsePositiveInt are identical — deduplicate.
Both functions have the exact same implementation. Remove one and reuse the other.
Proposed fix
-function parseTimeoutMs(raw: string | undefined, fallback: number): number {
- const v = raw ? Number.parseInt(raw, 10) : NaN;
- if (!Number.isFinite(v) || v <= 0) return fallback;
- return v;
-}
-
-function parsePositiveInt(raw: string | undefined, fallback: number): number {
- const v = raw ? Number.parseInt(raw, 10) : NaN;
- if (!Number.isFinite(v) || v <= 0) return fallback;
- return v;
-}
+function parsePositiveInt(raw: string | undefined, fallback: number): number {
+ const v = raw ? Number.parseInt(raw, 10) : NaN;
+ if (!Number.isFinite(v) || v <= 0) return fallback;
+ return v;
+}Then replace the parseTimeoutMs call on Line 102 with parsePositiveInt.
🤖 Prompt for AI Agents
In `@frontend/lib/env/monobank.ts` around lines 54 - 64, Remove the duplicated
implementation by keeping a single parsing helper (pick parsePositiveInt) and
delete parseTimeoutMs; then replace all usages of parseTimeoutMs with
parsePositiveInt (including the call currently using parseTimeoutMs) so callers
use the deduplicated function, ensuring the function signature and exported
identifiers remain consistent after removal.
| it('missing UAH price -> 422 PRICE_CONFIG_ERROR for monobank checkout', async () => { | ||
| const { productId } = await createIsolatedProduct({ | ||
| stock: 2, | ||
| prices: [{ currency: 'USD', priceMinor: 1000 }], | ||
| }); | ||
| const idemKey = crypto.randomUUID(); | ||
|
|
||
| try { | ||
| const res = await postCheckout(idemKey, productId); | ||
| expect(res.status).toBe(422); | ||
| const json: any = await res.json(); | ||
| expect(json.code).toBe('PRICE_CONFIG_ERROR'); | ||
| expect(createMonobankInvoiceMock).not.toHaveBeenCalled(); | ||
| } finally { | ||
| await cleanupProduct(productId).catch(() => {}); | ||
| } | ||
| }, 20_000); |
There was a problem hiding this comment.
Missing price test doesn't clean up orders created before the price check fails.
If the checkout route creates an order before discovering the price configuration error (which depends on the implementation), the finally block only cleans up the product, not any partial order. Consider also checking for and cleaning up any orders with the test's idempotency key.
🧹 Proposed cleanup addition
} finally {
+ try {
+ const [row] = await db
+ .select({ id: orders.id })
+ .from(orders)
+ .where(eq(orders.idempotencyKey, idemKey))
+ .limit(1);
+ if (row) await cleanupOrder(row.id).catch(() => {});
+ } catch {}
await cleanupProduct(productId).catch(() => {});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('missing UAH price -> 422 PRICE_CONFIG_ERROR for monobank checkout', async () => { | |
| const { productId } = await createIsolatedProduct({ | |
| stock: 2, | |
| prices: [{ currency: 'USD', priceMinor: 1000 }], | |
| }); | |
| const idemKey = crypto.randomUUID(); | |
| try { | |
| const res = await postCheckout(idemKey, productId); | |
| expect(res.status).toBe(422); | |
| const json: any = await res.json(); | |
| expect(json.code).toBe('PRICE_CONFIG_ERROR'); | |
| expect(createMonobankInvoiceMock).not.toHaveBeenCalled(); | |
| } finally { | |
| await cleanupProduct(productId).catch(() => {}); | |
| } | |
| }, 20_000); | |
| it('missing UAH price -> 422 PRICE_CONFIG_ERROR for monobank checkout', async () => { | |
| const { productId } = await createIsolatedProduct({ | |
| stock: 2, | |
| prices: [{ currency: 'USD', priceMinor: 1000 }], | |
| }); | |
| const idemKey = crypto.randomUUID(); | |
| try { | |
| const res = await postCheckout(idemKey, productId); | |
| expect(res.status).toBe(422); | |
| const json: any = await res.json(); | |
| expect(json.code).toBe('PRICE_CONFIG_ERROR'); | |
| expect(createMonobankInvoiceMock).not.toHaveBeenCalled(); | |
| } finally { | |
| try { | |
| const [row] = await db | |
| .select({ id: orders.id }) | |
| .from(orders) | |
| .where(eq(orders.idempotencyKey, idemKey)) | |
| .limit(1); | |
| if (row) await cleanupOrder(row.id).catch(() => {}); | |
| } catch {} | |
| await cleanupProduct(productId).catch(() => {}); | |
| } | |
| }, 20_000); |
🤖 Prompt for AI Agents
In `@frontend/lib/tests/shop/checkout-monobank-idempotency-contract.test.ts`
around lines 508 - 524, The test creates an idempotent checkout (idemKey) via
postCheckout but only calls cleanupProduct(productId) in finally, so partial
orders created before the price validation fail may remain; update the finally
block to also query and delete any orders associated with the test idempotency
key (use idemKey) — e.g., call the existing order lookup/delete helpers (or add
helpers like findOrdersByIdempotencyKey and deleteOrder) after cleanupProduct to
remove any order created by postCheckout, ensuring
createIsolatedProduct/productId and idemKey are referenced so the cleanup
targets the test artifacts.
| async function createIsolatedProduct(stock: number) { | ||
| const [tpl] = await db | ||
| .select() | ||
| .from(products) | ||
| .where(eq(products.isActive as any, true)) | ||
| .limit(1); | ||
|
|
||
| if (!tpl) { | ||
| throw new Error('No template product found to clone.'); | ||
| } | ||
|
|
||
| const productId = crypto.randomUUID(); | ||
| const slug = `t-mono-${crypto.randomUUID()}`; | ||
| const sku = `t-mono-${crypto.randomUUID()}`; | ||
| const now = new Date(); | ||
|
|
||
| await db.insert(products).values({ | ||
| ...(tpl as any), | ||
| id: productId, | ||
| slug, | ||
| sku, | ||
| title: `Test ${slug}`, | ||
| stock, | ||
| isActive: true, | ||
| createdAt: now, | ||
| updatedAt: now, | ||
| } as any); | ||
|
|
||
| await db.insert(productPrices).values({ | ||
| productId, | ||
| currency: 'UAH', | ||
| priceMinor: 1000, | ||
| originalPriceMinor: null, | ||
| price: toDbMoney(1000), | ||
| originalPrice: null, | ||
| createdAt: now, | ||
| updatedAt: now, | ||
| } as any); | ||
|
|
||
| return { productId }; | ||
| } |
There was a problem hiding this comment.
Test depends on a pre-existing active product in the database.
createIsolatedProduct (line 82-86) queries for any active product to use as a template. If this test runs in isolation or before any product-seeding test, it fails with "No template product found to clone." This creates an implicit ordering dependency between test files.
Consider either seeding a template product within this file (like checkout-monobank-idempotency-contract.test.ts does with seedTemplateProductIfMissing) or extracting the seeding logic into a shared test helper.
🤖 Prompt for AI Agents
In `@frontend/lib/tests/shop/monobank-psp-unavailable.test.ts` around lines 81 -
121, createIsolatedProduct currently queries the DB for an existing active
product and throws if none exists, causing test order dependency; update the
test to ensure a template exists by either calling the existing
seedTemplateProductIfMissing helper (or adding a similar seeding helper) before
running the select in createIsolatedProduct, or modify createIsolatedProduct to
create and insert a minimal template product when the select returns no rows,
then clone that template for the isolated product (refer to
createIsolatedProduct and seedTemplateProductIfMissing to locate the logic).
| async function insertAttempt(args: { | ||
| attemptId: string; | ||
| orderId: string; | ||
| status?: string; | ||
| expectedAmountMinor: number; | ||
| invoiceId: string; | ||
| providerModifiedAt: Date | null; | ||
| }) { | ||
| const attemptStatus = | ||
| args.status ?? | ||
| (await pickEnumLabelByColumn('payment_attempts', 'status', [ | ||
| 'pending', | ||
| 'created', | ||
| 'requires_action', | ||
| ])); |
There was a problem hiding this comment.
pickEnumLabelByColumn fallback for payment_attempts.status will fail if status column is text type.
The status column on payment_attempts is declared as text with a CHECK constraint, not as a PostgreSQL enum type. If args.status is not provided, pickEnumLabelByColumn will query information_schema for the UDT name, get 'text', and then fail when trying to resolve enum labels for the text type (throwing "Enum text has no labels"). Currently, all callers pass an explicit status, so this path isn't hit — but the fallback is a latent failure.
🤖 Prompt for AI Agents
In `@frontend/lib/tests/shop/monobank-webhook-apply-outcomes.test.ts` around lines
144 - 158, The fallback in insertAttempt currently calls pickEnumLabelByColumn
for payment_attempts.status which breaks when the column is text with a CHECK
constraint; change insertAttempt so it does not rely on pickEnumLabelByColumn
for this column—use a safe hardcoded fallback list (e.g.
['pending','created','requires_action']) or otherwise detect a non-enum UDT and
choose from that list; update the logic around insertAttempt (and the status
assignment) to pick from the explicit array when args.status is undefined
instead of calling pickEnumLabelByColumn.
| await insertAttempt({ | ||
| attemptId, | ||
| orderId, | ||
| status: 'pending', | ||
| expectedAmountMinor: 100, | ||
| invoiceId, | ||
| providerModifiedAt: null, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Finding schema files ==="
fd -t f "schema" frontend/ | head -20
echo ""
echo "=== Searching for payment_attempts schema definition ==="
rg -n "payment_attempts" frontend/ -t ts --max-count=5
echo ""
echo "=== Looking for status constraint in schema ==="
rg -n "payment_attempts_status_check\|status.*check" frontend/ -t ts -A 2 -B 2Repository: DevLoversTeam/devlovers.net
Length of output: 2686
🏁 Script executed:
#!/bin/bash
echo "=== Checking migration files for payment_attempts constraint ==="
fd -t f "drizzle\|migration" frontend/ | head -10
echo ""
echo "=== Searching for payment_attempts in migration files ==="
find frontend -name "*.ts" -o -name "*.sql" | xargs grep -l "payment_attempts" 2>/dev/null | head -10Repository: DevLoversTeam/devlovers.net
Length of output: 629
🏁 Script executed:
#!/bin/bash
echo "=== Reading the test file around the insertAttempt calls ==="
sed -n '240,270p' frontend/lib/tests/shop/monobank-webhook-apply-outcomes.test.ts
echo ""
echo "=== Reading other insertAttempt calls ==="
sed -n '300,330p' frontend/lib/tests/shop/monobank-webhook-apply-outcomes.test.ts
echo ""
sed -n '360,390p' frontend/lib/tests/shop/monobank-webhook-apply-outcomes.test.tsRepository: DevLoversTeam/devlovers.net
Length of output: 2356
🏁 Script executed:
#!/bin/bash
echo "=== Reading payment_attempts schema definition ==="
sed -n '520,600p' frontend/db/schema/shop.ts
echo ""
echo "=== Reading migration 0001 for initial constraint ==="
cat frontend/drizzle/0001_add_payment_attempts.sqlRepository: DevLoversTeam/devlovers.net
Length of output: 4546
🏁 Script executed:
#!/bin/bash
echo "=== Checking migration 0006 for status constraint changes ==="
cat frontend/drizzle/0006_certain_shocker.sql | grep -A 10 -B 5 "payment_attempts\|status" || echo "No matches in 0006"
echo ""
echo "=== Checking other migrations ==="
grep -r "payment_attempts.*status\|ALTER.*payment_attempts" frontend/drizzle/ | head -20Repository: DevLoversTeam/devlovers.net
Length of output: 5289
🏁 Script executed:
#!/bin/bash
echo "=== Check if tests are currently running and what happens ==="
grep -n "beforeEach\|afterEach\|beforeAll\|describe\|it(" frontend/lib/tests/shop/monobank-webhook-apply-outcomes.test.ts | head -20
echo ""
echo "=== Check for any DB setup that might disable constraints ==="
rg -n "constraint\|PRAGMA\|disable\|enforce" frontend/lib/tests/shop/monobank-webhook-apply-outcomes.test.ts
echo ""
echo "=== Look at the full insertAttempt function signature ==="
sed -n '144,195p' frontend/lib/tests/shop/monobank-webhook-apply-outcomes.test.tsRepository: DevLoversTeam/devlovers.net
Length of output: 1998
status: 'pending' violates the payment_attempts_status_check constraint.
The payment_attempts_status_check constraint only allows ('creating','active','succeeded','failed','canceled'). Inserting with status: 'pending' triggers a CHECK violation at the database level, causing the INSERT to fail and the test to fail before executing any webhook logic. The same issue affects the other insertAttempt calls at lines 317-324 and 374-381.
Use 'active' instead, which is in the constraint allowlist, serves as the column default, and is semantically appropriate for an attempt awaiting webhook notification.
Proposed fix for all three insertAttempt calls
await insertAttempt({
attemptId,
orderId,
- status: 'pending',
+ status: 'active',
expectedAmountMinor: 100,
invoiceId,
providerModifiedAt: null,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await insertAttempt({ | |
| attemptId, | |
| orderId, | |
| status: 'pending', | |
| expectedAmountMinor: 100, | |
| invoiceId, | |
| providerModifiedAt: null, | |
| }); | |
| await insertAttempt({ | |
| attemptId, | |
| orderId, | |
| status: 'active', | |
| expectedAmountMinor: 100, | |
| invoiceId, | |
| providerModifiedAt: null, | |
| }); |
🤖 Prompt for AI Agents
In `@frontend/lib/tests/shop/monobank-webhook-apply-outcomes.test.ts` around lines
258 - 265, The test inserts use insertAttempt with status: 'pending', which
violates the database CHECK constraint payment_attempts_status_check; update
each insertAttempt call (the ones around the monobank webhook tests that set
status currently to 'pending') to use status: 'active' instead (active is
allowed, matches the column default and semantics for awaiting webhook
notification) so the INSERTs succeed and the webhook logic can execute.
| }, | ||
| "homepage": { | ||
| "title": "DevLovers — Platforma do przygotowania do rozmów technicznych", | ||
| "title": "DevLovers", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for references to the removed translation keys in TypeScript/TSX files
echo "=== Searching for quizResults references ==="
rg -n "quizResults" --type=ts --type=tsx -B2 -A2
echo -e "\n=== Searching for quizReview references ==="
rg -n "quizReview" --type=ts --type=tsx -B2 -A2
echo -e "\n=== Searching for dashboard.quizResults references ==="
rg -n "dashboard\.quizResults" --type=ts --type=tsx -B2 -A2
echo -e "\n=== Searching for dashboard.quizReview references ==="
rg -n "dashboard\.quizReview" --type=ts --type=tsx -B2 -A2Repository: DevLoversTeam/devlovers.net
Length of output: 400
🏁 Script executed:
#!/bin/bash
# Search for references using glob patterns instead of file types
echo "=== Searching for quizResults references in .ts/.tsx files ==="
rg -n "quizResults" --glob="*.ts" --glob="*.tsx" -B2 -A2
echo -e "\n=== Searching for quizReview references in .ts/.tsx files ==="
rg -n "quizReview" --glob="*.ts" --glob="*.tsx" -B2 -A2
echo -e "\n=== Searching dashboard.quizResults in .ts/.tsx files ==="
rg -n "dashboard\.quizResults" --glob="*.ts" --glob="*.tsx" -B2 -A2
echo -e "\n=== Searching dashboard.quizReview in .ts/.tsx files ==="
rg -n "dashboard\.quizReview" --glob="*.ts" --glob="*.tsx" -B2 -A2Repository: DevLoversTeam/devlovers.net
Length of output: 5769
Remove the translation keys from pl.json only after updating or removing the components that reference them.
The removal of dashboard.quizResults and dashboard.quizReview keys is NOT safe. Multiple components still actively reference these translation keys:
QuizResultRow.tsxandQuizResultsSection.tsxreferencedashboard.quizResultsquiz-review/[attemptId]/page.tsx,QuizReviewList.tsx, andQuizReviewCard.tsxreferencedashboard.quizReview
Removing these translation keys without updating these components will cause runtime errors when the application attempts to fetch translations for these namespaces.
🤖 Prompt for AI Agents
In `@frontend/messages/pl.json` at line 47, The translation keys
"dashboard.quizResults" and "dashboard.quizReview" were removed from pl.json but
are still referenced by components; either restore those keys in pl.json or
update the components to stop using them before removing; specifically, either
re-add the keys to pl.json or refactor QuizResultRow.tsx, QuizResultsSection.tsx
(which use dashboard.quizResults) and quiz-review/[attemptId]/page.tsx,
QuizReviewList.tsx, QuizReviewCard.tsx (which use dashboard.quizReview) to use
new keys or safe fallbacks, then remove the keys once all references are
updated.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@frontend/db/queries/quiz.ts`:
- Line 62: The module-level Map attemptReviewCache (holding AttemptReview
objects) is unbounded and can leak memory; replace it with a bounded cache or
external cache: either swap the Map for an LRU/TTL in-memory cache (e.g., use an
LRU cache library and cap size and/or entry TTL) and update any get/set/delete
usages of attemptReviewCache accordingly, or revert to the previous Redis-backed
implementation (use the existing Redis client to store AttemptReview entries
with a sensible expiry) so entries are automatically evicted; ensure all
references to attemptReviewCache in this module use the new cache API and
preserve AttemptReview serialization/deserialization if using Redis.
- Around line 62-81: The cache key currently built by getAttemptReviewCacheKey
and used by getCachedAttemptReview/cacheAttemptReview omits userId, allowing one
user's cached AttemptReview to be returned for another user; update
getAttemptReviewCacheKey to include userId (e.g.,
`${attemptId}:${userId}:${locale}`), update all call sites in
getAttemptReviewDetails to pass the current userId into getCachedAttemptReview
and cacheAttemptReview, and ensure cache lookups occur only with the
ownership-aware key (or move the cache lookup until after the DB ownership
check) so cached entries are scoped per user.
🧹 Nitpick comments (2)
frontend/db/queries/quiz.ts (2)
68-81:asyncis unnecessary for synchronousMapoperations.Both
getCachedAttemptReviewandcacheAttemptReviewonly perform synchronousMap.get/Map.setbut are declaredasync, wrapping returns in unnecessary promises. If you plan to swap back to Redis later, keeping theasyncsignature is fine as a forward-compatible choice — otherwise, droppingasyncavoids the overhead.
482-482: Fragile double type-cast on raw SQL result.
(rows as { rows: unknown[] }).rows as UserLastAttempt[]assumes Drizzle'sdb.executereturns{ rows: ... }— this shape can change across driver versions. Consider using a runtime validation step (e.g., a lightweight schema check with Zod) or at minimum add a comment documenting the expected return shape.
| answers: QuizAnswerClient[]; | ||
| } | ||
|
|
||
| const attemptReviewCache = new Map<string, AttemptReview>(); |
There was a problem hiding this comment.
Unbounded in-memory cache — potential memory leak.
attemptReviewCache is a module-level Map that only grows and never evicts entries. In a long-running Next.js server process, every unique attempt review will accumulate indefinitely.
Consider adding a TTL-based eviction strategy (e.g., an LRU cache with a size cap) or, since the previous implementation used Redis, reverting to an external cache store that handles eviction naturally.
🤖 Prompt for AI Agents
In `@frontend/db/queries/quiz.ts` at line 62, The module-level Map
attemptReviewCache (holding AttemptReview objects) is unbounded and can leak
memory; replace it with a bounded cache or external cache: either swap the Map
for an LRU/TTL in-memory cache (e.g., use an LRU cache library and cap size
and/or entry TTL) and update any get/set/delete usages of attemptReviewCache
accordingly, or revert to the previous Redis-backed implementation (use the
existing Redis client to store AttemptReview entries with a sensible expiry) so
entries are automatically evicted; ensure all references to attemptReviewCache
in this module use the new cache API and preserve AttemptReview
serialization/deserialization if using Redis.
| const attemptReviewCache = new Map<string, AttemptReview>(); | ||
|
|
||
| function getAttemptReviewCacheKey(attemptId: string, locale: string) { | ||
| return `${attemptId}:${locale}`; | ||
| } | ||
|
|
||
| async function getCachedAttemptReview( | ||
| attemptId: string, | ||
| locale: string | ||
| ): Promise<AttemptReview | null> { | ||
| return attemptReviewCache.get(getAttemptReviewCacheKey(attemptId, locale)) ?? null; | ||
| } | ||
|
|
||
| async function cacheAttemptReview( | ||
| attemptId: string, | ||
| locale: string, | ||
| review: AttemptReview | ||
| ): Promise<void> { | ||
| attemptReviewCache.set(getAttemptReviewCacheKey(attemptId, locale), review); | ||
| } |
There was a problem hiding this comment.
Authorization bypass: cache key omits userId, leaking data across users.
The cache key is attemptId:locale (line 65), but getAttemptReviewDetails filters by userId in the DB query (line 521). Flow:
- User A requests attempt X → cache miss → DB returns data (userId matches) → cached as
"X:uk". - User B requests attempt X → cache hit → returns User A's review data, bypassing the
userIdcheck.
Include userId in the cache key, or move the cache lookup to after the ownership check.
Proposed fix — include userId in cache key
-function getAttemptReviewCacheKey(attemptId: string, locale: string) {
- return `${attemptId}:${locale}`;
+function getAttemptReviewCacheKey(attemptId: string, userId: string, locale: string) {
+ return `${attemptId}:${userId}:${locale}`;
}
async function getCachedAttemptReview(
attemptId: string,
+ userId: string,
locale: string
): Promise<AttemptReview | null> {
- return attemptReviewCache.get(getAttemptReviewCacheKey(attemptId, locale)) ?? null;
+ return attemptReviewCache.get(getAttemptReviewCacheKey(attemptId, userId, locale)) ?? null;
}
async function cacheAttemptReview(
attemptId: string,
+ userId: string,
locale: string,
review: AttemptReview
): Promise<void> {
- attemptReviewCache.set(getAttemptReviewCacheKey(attemptId, locale), review);
+ attemptReviewCache.set(getAttemptReviewCacheKey(attemptId, userId, locale), review);
}Then update the call sites in getAttemptReviewDetails:
- const cached = await getCachedAttemptReview(attemptId, locale);
+ const cached = await getCachedAttemptReview(attemptId, userId, locale);- await cacheAttemptReview(attemptId, locale, review);
+ await cacheAttemptReview(attemptId, userId, locale, review);🤖 Prompt for AI Agents
In `@frontend/db/queries/quiz.ts` around lines 62 - 81, The cache key currently
built by getAttemptReviewCacheKey and used by
getCachedAttemptReview/cacheAttemptReview omits userId, allowing one user's
cached AttemptReview to be returned for another user; update
getAttemptReviewCacheKey to include userId (e.g.,
`${attemptId}:${userId}:${locale}`), update all call sites in
getAttemptReviewDetails to pass the current userId into getCachedAttemptReview
and cacheAttemptReview, and ensure cache lookups occur only with the
ownership-aware key (or move the cache lookup until after the DB ownership
check) so cached entries are scoped per user.
5 participants
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores