(SP: 3) [Shop][Monobank] Janitor map + internal janitor endpoint stub + status UX + security/obs + J test gate#328
Conversation
…ebhook + janitor (ORIGIN_BLOCKED)
…wnership test and pass all pre-prod invariants
liudmylasovetovs
requested review from
AM1007 and
ViktorSvertoka
as code owners
|
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. |
📝 WalkthroughWalkthroughAdds dual-provider checkout (Stripe + Monobank) with feature-flag wiring, client provider selection and validation, monobank redirect + status-polling flow, monobank-specific logging and sanitization, origin/rate-limit guards, clock-based SQL fixes, currency-formatting API, and many new/updated tests across checkout, webhook, janitor, and refund flows. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant CartPage as Cart/Checkout Page
participant CheckoutAPI as Checkout API
participant PSP as Monobank PSP
participant SuccessPage as Success Page
participant Poller as MonobankRedirectStatus
Client->>CartPage: select provider, place order (provider=monobank)
CartPage->>CheckoutAPI: POST /api/shop/checkout { paymentProvider: "monobank", ... }
CheckoutAPI->>PSP: create invoice
PSP-->>CheckoutAPI: { invoiceId, pageUrl, statusToken? }
CheckoutAPI-->>CartPage: 201 { pageUrl, orderId, statusToken }
CartPage->>Client: redirect to PSP pageUrl
PSP->>Client: redirect back to site (statusToken)
Client->>SuccessPage: GET /checkout/success?orderId=...&statusToken=...
SuccessPage->>Poller: init(orderId, statusToken)
loop Polling with exponential backoff
Poller->>CheckoutAPI: GET /api/shop/orders/{id}/status?statusToken=...
CheckoutAPI-->>Poller: { paymentStatus }
alt terminal (paid/canceled/needsReview)
Poller-->>Client: render final status
else non-terminal (pending)
Poller->>Poller: wait + retry
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a89fc6a813
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| `/shop/checkout/success?flow=monobank&orderId=${encodeURIComponent( | ||
| args.orderId | ||
| )}&statusToken=${encodeURIComponent(args.statusToken)}` |
There was a problem hiding this comment.
Now that checkout redirects customers directly to Monobank (CartPageClient uses pageUrl), this redirect URL is exercised on every Monobank payment, but it is built as /shop/checkout/success?... without a locale segment. In this codebase routes are locale-prefixed (localePrefix: 'always') and the success page exists only under app/[locale]/shop/checkout/success, so Monobank will send users back to a non-matching path (404 / wrong route) after payment. The redirect URL needs to include a concrete locale.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@frontend/lib/tests/shop/monobank-janitor-job1.test.ts`:
- Around line 233-234: The test currently stubs MONO_JANITOR_JOB1_GRACE_SECONDS
with a huge value that parseEnvInt clamps to GRACE_SECONDS_MAX (86,400), making
the intent misleading; update the stub to a value within the valid range (e.g.,
vi.stubEnv('MONO_JANITOR_JOB1_GRACE_SECONDS', '1') or '86400') and adjust the
graceSeconds variable/comment accordingly so the test explicitly reflects the
effective grace window; ensure you reference the parseEnvInt/GRACE_SECONDS_MAX
behavior and the MONO_JANITOR_JOB1_GRACE_SECONDS env stub in the change.
In `@frontend/lib/tests/shop/monobank-webhook-logging-safety.test.ts`:
- Around line 103-122: The test hits the real enforceRateLimit implementation
(route.ts -> enforceRateLimit) which performs a DB query, so add a Jest mock for
the '@/lib/security/rate-limit' module in this test file similar to the other
webhook tests; mock enforceRateLimit to a resolved no-op (e.g., a
mockResolvedValue or jest.fn that returns Promise.resolve()) before calling
postWebhookRaw so the rate-limit check won't touch the DB and the
MONO_SIG_INVALID log path is reached deterministically.
In `@frontend/lib/tests/shop/orders-status-ownership.test.ts`:
- Around line 306-312: The test's type cast for the imported module's GET
handler is wrong: change the ctx params type from { params: { id: string } } to
use a Promise wrapper so it matches the actual route handler signature; update
the cast around the imported module (the mod variable / GET signature in the
test) to declare ctx: { params: Promise<{ id: string }> } so the test types
align with the route handler that awaits params.
In `@frontend/messages/uk.json`:
- Around line 389-392: Replace the untranslated English term "checkout" in the
Ukrainian messages by updating the values for the keys "unexpectedResponse" and
"startFailed" in frontend/messages/uk.json to use the Ukrainian equivalent
(e.g., "оформлення замовлення") so they read consistently with existing keys
like "placeOrder" and "unableToCheckout"; ensure the new strings maintain
original punctuation and capitalization.
🧹 Nitpick comments (23)
frontend/lib/tests/shop/monobank-webhook-multi-instance-apply.test.ts (2)
99-100:assertNotProductionDb()runs at describe-collection time, not inside a test orbeforeAll.This works because Vitest executes the describe callback synchronously during collection, so the guard fires before any test runs. However, if it throws, the error surfaces as a suite-level collection failure rather than a named test failure, which can be harder to diagnose in CI logs. Consider wrapping it in a
beforeAllfor clearer reporting. This is a minor style point — the current placement is functionally safe.
30-61: Consider adding a brief comment on whyas anyis necessary for the insert calls.The
as anycasts on lines 45 and 58 bypass the Drizzle schema's required fields. This is pragmatic for tests that only need a minimal fixture, but a one-liner comment (e.g.,// partial fixture — only fields relevant to this test) would help future readers understand this is intentional, not accidental.frontend/lib/security/origin.ts (1)
113-135: Inconsistent error response shape across guards.
guardNonBrowserFailClosedreturns{ success, code, surface }whilebuildErrorResponse(used byguardBrowserSameOriginandguardNonBrowserOnly) wraps errors in{ error: { code, message } }. Consumers parsing these responses will need to handle two different shapes. If this divergence is intentional (different contract for different surfaces), consider documenting it; otherwise, unify.frontend/lib/tests/shop/origin-posture.test.ts (1)
107-138: Consider adding a test for origin-only blocking and default surface.The new tests cover referer and sec-fetch scenarios, but there's no test verifying that:
- A request with only an
originheader (no referer, no sec-fetch) is blocked.- The default surface
'non_browser'is returned whenmetais omitted.Both are branches in the implementation that would benefit from coverage.
frontend/lib/tests/shop/monobank-webhook-signature-verify.test.ts (1)
33-39: Minimal fetch mock may break if implementation changes.
makeOkResponseonly providesok,status, andtext(). IffetchWebhookPubKeyever calls.json()or reads.headers, this mock will throw. Consider returning a more completeResponse-like object or usingnew Response(body).♻️ Slightly more robust mock
function makeOkResponse(body: string) { - return { - ok: true, - status: 200, - text: async () => body, - }; + return new Response(body, { status: 200 }); }frontend/lib/tests/shop/checkout-monobank-happy-path.test.ts (1)
99-150: Well-structured test helper with a subtle assumption about currency.The helper accepts
currency: 'USD'for the product but hardcodes'UAH'for theproductPricesentry (Line 136). This works for the current Monobank-specific test (UAH checkout), but the mismatch between the type parameter and the actual price currency could be confusing. Consider either:
- Adding a
priceCurrencyparameter, or- Documenting that this helper creates a UAH price regardless of the product's base currency.
Not blocking — it's clear in context.
frontend/app/[locale]/shop/cart/CartPageClient.tsx (1)
72-91: InitialselectedProvidermay not match available providers on first render.When
stripeEnabled=falseandmonobankEnabled=true, the initial state is'stripe', causing a brief render where no radio button appears checked. TheuseEffecton Lines 83-91 corrects this on the next tick, but users may see a momentary flicker.Consider initializing based on availability:
♻️ Suggested initialization
- const [selectedProvider, setSelectedProvider] = - useState<CheckoutProvider>('stripe'); + const [selectedProvider, setSelectedProvider] = + useState<CheckoutProvider>(() => + stripeEnabled ? 'stripe' : monobankEnabled ? 'monobank' : 'stripe' + );This would eliminate the
useEffectcorrection on initial mount (though keeping the effect is still useful for runtime changes).frontend/lib/services/orders/monobank.ts (1)
624-631:NODE_ENV !== 'test'guard is a testing smell.Silencing logs based on
NODE_ENVcreates a blind spot — if the logging code path breaks, tests won't catch it. Since the test files already mock the logging module, this guard is redundant. Consider removing it so the monoLogWarn path is exercised in tests like the regularlogWarnon line 633.Suggested fix
- if (process.env.NODE_ENV !== 'test') { - monoLogWarn(MONO_CREATE_INVOICE_FAILED, { - orderId: args.orderId, - attemptId: attempt.id, - requestId: args.requestId, - errorCode, - }); - } + monoLogWarn(MONO_CREATE_INVOICE_FAILED, { + orderId: args.orderId, + attemptId: attempt.id, + requestId: args.requestId, + errorCode, + });frontend/lib/tests/shop/monobank-logging-safety.test.ts (2)
181-205: Non-greedy[\s\S]*?in regex patterns may struggle with deeply nested braces.The patterns like
/(logInfo|logWarn)\(\s*['"\][^'"\`]+['"`]\s*,\s*{[\s\S]?}\s)/gattempt to match the closing}\s*)but[\s\S]*?doesn't understand brace nesting. On files with many logging calls, the regex might match too little (stopping at the first})`) or exhibit slow backtracking.This is acceptable for a safety-gate test but may produce false negatives on multi-line log calls where the meta object contains nested objects. Worth being aware of if the test starts missing violations.
116-122:loadFrontendEntries()is called independently in each test — consider sharing via abeforeAllhook.Each of the four tests re-walks the filesystem and re-reads every file. A single
beforeAllthat populates a sharedentriesvariable would cut I/O by ~75%.frontend/app/api/shop/admin/orders/[id]/refund/route.ts (1)
38-44:readPositiveIntEnvis a good pattern but note it rejects zero.Since
parsed > 0is the check, setting an env var to0would silently fall back to the default. This is likely intentional for rate-limit config (zero makes no sense), but worth documenting if the helper is reused elsewhere.frontend/lib/services/orders/monobank-webhook.ts (1)
578-585: Minor inconsistency: error-level logs still use unsanitizedlogError.
monoLogInfoandmonoLogWarnare used throughout, but error paths (Lines 803, 918, 962, 1056) still calllogErrordirectly, bypassingsanitizeMonobankMeta. The metadata at those call sites currently only contains IDs and status strings (no PII), so this is safe today — but for consistency and future-proofing, consider importingmonoLogErrorfor those paths as well.Also applies to: 615-622, 837-844
frontend/lib/tests/shop/monobank-webhook-paid-sticky.test.ts (1)
37-47: Consider narrowing theas anycasts with a partial type.The
as anybypasses all type checking on the insert values. A narrower approach (e.g., a test-only type orPartial<...> as typeof orders.$inferInsert) would still catch genuine column-name typos at compile time while remaining flexible.frontend/app/api/shop/webhooks/monobank/route.ts (2)
35-38:readPositiveIntEnvis duplicated incheckout/route.ts.This identical helper also exists at
frontend/app/api/shop/checkout/route.ts:52-55. Consider extracting it into a shared utility (e.g.,@/lib/env/helpers) to keep a single definition.Example shared utility
+// frontend/lib/env/helpers.ts +export function readPositiveIntEnv(name: string, fallback: number): number { + const parsed = Number.parseInt(process.env[name] ?? '', 10); + return Number.isFinite(parsed) && parsed > 0 ? parsed : fallback; +}
112-116: DualMONO_STORE_MODElog entries may confuse log consumers.
MONO_STORE_MODEis emitted once at Line 112 (reporting the configured mode) and again at Line 241 (reporting the applied result). The same log code with different semantics can complicate alerting/filtering. Consider using a distinct code (e.g.,MONO_STORE_MODE_RESULT) for the result log, or dropping the early-mode log since the result log already carries themodefield.Also applies to: 240-249
frontend/app/api/shop/checkout/route.ts (1)
369-375:NODE_ENV !== 'test'guard around logging is a code smell.This suppresses the
monoLogWarncall in tests, which means logging-safety tests (like the ones in this PR for the webhook route) can't verify this particular log path. Prefer mocking the logging module in the relevant tests instead. If the concern is log noise, the test setup already mocks@/lib/logging.Proposed: remove the env guard
if (args.totalCents !== monobankAttempt.totalAmountMinor) { - if (process.env.NODE_ENV !== 'test') { - monoLogWarn(MONO_MISMATCH, { - requestId: args.requestId, - orderId: args.order.id, - reason: 'checkout_total_amount_mismatch', - }); - } + monoLogWarn(MONO_MISMATCH, { + requestId: args.requestId, + orderId: args.order.id, + reason: 'checkout_total_amount_mismatch', + });frontend/app/api/shop/internal/monobank/janitor/route.ts (1)
306-409: Job dispatch blocks are highly repetitive.All four job branches follow the same pattern: call
runMonobankJanitorJobN, build a response with the same fields (only job4 addsreport). A dispatch map or small helper would collapse this into ~10 lines. Not urgent since the pattern is stable across 4 jobs, but worth considering if more jobs are added.Example dispatch-map approach
+ const jobRunners: Record<JobName, (args: any) => Promise<any>> = { + job1: runMonobankJanitorJob1, + job2: runMonobankJanitorJob2, + job3: runMonobankJanitorJob3, + job4: runMonobankJanitorJob4, + }; + + const runner = jobRunners[job]; + if (runner) { + const result = await runner({ dryRun, limit, requestId, runId, baseMeta }); + return noStoreJson( + { + success: true, + job, + dryRun, + limit, + processed: result.processed, + applied: result.applied, + noop: result.noop, + failed: result.failed, + ...(result.report ? { report: result.report } : {}), + requestId, + }, + requestId, + { status: 200 } + ); + }frontend/app/[locale]/shop/checkout/success/page.tsx (1)
32-33: Triple caching opt-out is redundant.
dynamic = 'force-dynamic'(Line 32) already guarantees the page is never statically rendered. Addingrevalidate = 0(Line 33) and callingnoStore()(Line 155) are both redundant with that directive. Consider keeping justdynamic = 'force-dynamic'and removing the other two to reduce noise.Also applies to: 155-155
frontend/lib/tests/shop/monobank-janitor-origin-posture.test.ts (1)
114-121: Minor: assert status before body for clearer diagnostics.If
res.statusisn't 200, thebody.successassertion at line 116 will produce a less helpful error message than checking status first. Consider swapping lines 116 and 118.Swap assertion order
const res = await POST(req); const body = await res.json(); - expect(body.success).toBe(true); - expect(res.status).toBe(200); + expect(body.success).toBe(true); expect(res.headers.get('X-Request-Id')).toBe('req-origin-allow');frontend/lib/tests/shop/orders-status-ownership.test.ts (2)
98-101: Misleading function name and rigid type literal.
insertTestProductWithUAHPriceinserts a product withcurrency: args.currency(which is always'USD'per the type), then separately adds a UAH row inproductPrices. The name suggests a UAH product, but the product itself is USD.Additionally,
currency: 'USD'as a literal type means the parameter is redundant — consider hardcoding it or widening the type if other currencies are intended.
197-299:extractStatusTokenis excessively defensive for test code.This ~100-line function tries 5 direct fields, 8 URL fields, 4 query params per URL, and then a depth-limited graph traversal — all to locate a status token. While resilient to API response changes, it risks silently masking regressions if the API moves the token to an unexpected location.
Consider narrowing this to the known response shape (e.g., just
json.statusTokenorjson.statusUrl) and letting the test break loudly if the contract changes. That way, the test serves as a contract-enforcement gate rather than an adapter.frontend/app/[locale]/shop/checkout/success/MonobankRedirectStatus.tsx (2)
71-109:SHOP_HERO_CTA_SM,SHOP_OUTLINE_BTN, andHeroCtaInnerare duplicated frompage.tsx.These exact definitions (class compositions and the
HeroCtaInnercomponent) appear in both this file and the parentpage.tsx. Extract them to a shared module (e.g.,./checkout-ui.tsxor into@/lib/shop/ui-classes) to keep a single source of truth.
178-183:Cache-Controlrequest header is non-standard for browserfetch.The
cache: 'no-store'option (line 180) is the correct fetch API mechanism to bypass the cache. TheCache-Control: no-storeheader on the request (line 181) is redundant for browser-initiatedfetch— it may be useful if an intermediate proxy/CDN inspects request headers, but otherwise it's dead weight.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/lib/services/orders/monobank-webhook.ts (1)
803-808: 🛠️ Refactor suggestion | 🟠 MajorAdd missing log codes to
MonobankLogCodeinstead of usingas anycasts.Lines 803, 918, 962, and 1056 use
as anycasts with ad-hoc string literals ('monobank_webhook_atomic_update_failed','MONO_WEBHOOK_UNKNOWN_STATUS','monobank_webhook_restock_failed') that are not defined in theMonobankLogCodetype. SinceMonobankLogCodeis strictly typed as the union of values inMONO_LOG_CODES, these strings bypass type safety entirely.Define these as exported constants in
frontend/lib/logging/monobank.tsfollowing the existing pattern (e.g.,export const MONO_WEBHOOK_ATOMIC_UPDATE_FAILED = 'MONO_WEBHOOK_ATOMIC_UPDATE_FAILED' as const;and add them toMONO_LOG_CODES), or use the rawlogErrorif these codes are intentionally one-off.
🧹 Nitpick comments (14)
frontend/lib/tests/shop/monobank-attempt-invoice.test.ts (1)
3-17:monoLogWarnMockis wired but never asserted in any test case.The mock is declared and injected into the module mock, but no
expect(monoLogWarnMock)call exists in this file. This is dead test instrumentation — either add assertions that verify expected warning emissions, or remove the mock to keep the test surface clean.frontend/lib/tests/shop/orders-status-ownership.test.ts (4)
391-395: Weak body assertion on the 200 (correct-token) path.The conditional on line 392 silently skips the
orderIdcheck if neitherjson.orderIdnorjson.idis present. For an ownership test, it's worth asserting the response body shape explicitly so regressions (e.g., the endpoint stops returningorderId) don't go unnoticed.💡 Suggested tightening
expect(res.status).toBe(200); - if (json && (json.orderId || json.id)) { - expect(json.orderId ?? json.id).toBe(orderA); - } + expect(json).toBeTruthy(); + const returnedId = json.orderId ?? json.id; + expect(returnedId).toBeDefined(); + expect(returnedId).toBe(orderA);
197-299:extractStatusTokenis resilient but heavily over-engineered for a test helper.The three-phase lookup (direct fields → URL parsing → depth-limited DFS) is ~100 lines of non-trivial logic that itself could contain bugs and is hard to reason about during test failures. If the checkout response shape changes, a simpler approach — directly asserting the expected field name — would surface the change faster and make debugging easier.
That said, this is a test file and the approach does work, so treat this as a low-priority simplification.
51-92: Consider a small env-restore helper to reduce the repetitive save/restore boilerplate.Six env vars × identical if/else/delete pattern is verbose. A utility like the one below would cut ~40 lines to ~5:
💡 Example helper
function withEnv(overrides: Record<string, string>) { const saved = new Map<string, string | undefined>(); for (const key of Object.keys(overrides)) { saved.set(key, process.env[key]); } beforeAll(() => { for (const [k, v] of Object.entries(overrides)) process.env[k] = v; resetEnvCache(); }); afterAll(() => { for (const [k, prev] of saved) { if (prev === undefined) delete process.env[k]; else process.env[k] = prev; } resetEnvCache(); }); }
340-342:assertNotProductionDb()called at describe-body scope works but is unconventional.Placing it inside
beforeAllis more idiomatic and ensures it runs in the test lifecycle rather than at module evaluation time. Current placement is functional since it throws synchronously and prevents suite setup, but it may confuse readers.frontend/lib/services/orders/monobank.ts (1)
624-637: Duplicate warning logs for the same invoice-creation failure.
monoLogWarn(MONO_CREATE_INVOICE_FAILED, …)(Line 624) internally callslogWarn(viasanitizeMonobankMeta), and then Line 631 emits a secondlogWarn('monobank_invoice_create_failed', …)for the same error path with overlapping metadata. This produces two log entries per failure, which can confuse alerting/dashboards and inflate log volume.If the intent is to migrate to the structured
monoLogWarnpattern, consider removing the legacylogWarncall. If both are needed during a transition period, a brief comment would clarify intent.♻️ Suggested fix (remove legacy call)
monoLogWarn(MONO_CREATE_INVOICE_FAILED, { orderId: args.orderId, attemptId: attempt.id, requestId: args.requestId, errorCode, + message: errorMessage, }); - logWarn('monobank_invoice_create_failed', { - orderId: args.orderId, - attemptId: attempt.id, - code: errorCode, - requestId: args.requestId, - message: errorMessage, - });frontend/lib/security/origin.ts (1)
113-138: Consider reusingbuildErrorResponseto reduce structural drift.The inline response construction works correctly, but it diverges from the shared
buildErrorResponsehelper at Lines 5-19 in both structure (surfaceis a top-level sibling oferror) and construction pattern. If a future change updates the error envelope (e.g., addingtraceId), this path would be missed.A lightweight option: extend
buildErrorResponseto accept optional extra fields, or extract a shared builder.frontend/app/api/shop/admin/orders/[id]/refund/route.ts (2)
85-88: Simplify the admin subject extraction.The double type assertion is hard to read. Consider a small helper or a simpler pattern:
♻️ Suggested simplification
- const adminSubject = - typeof (adminUser as { id?: unknown })?.id === 'string' - ? `admin_${normalizeRateLimitSubject((adminUser as { id: string }).id)}` - : getRateLimitSubject(request); + const adminId = (adminUser as Record<string, unknown>)?.id; + const adminSubject = + typeof adminId === 'string' + ? `admin_${normalizeRateLimitSubject(adminId)}` + : getRateLimitSubject(request);
144-186: Duplicatemonobankprovider check — consider consolidating.
targetOrder?.paymentProvider === 'monobank'is evaluated at Line 144 and again at Line 161. The second block could be anelse-continuation or the two could be merged into a singleifblock, since if refund is disabled the function returns early anyway.frontend/lib/tests/shop/monobank-webhook-signature-verify.test.ts (1)
40-60: Env save/restore pattern is solid but considervi.stubEnvfor consistency.Other test files in this PR (e.g.,
monobank-janitor-origin-posture.test.ts) usevi.stubEnv/vi.unstubAllEnvs. The manualrememberEnv/restoreEnvapproach here works correctly but introduces a different pattern. This may be justified since you need to deleteMONO_PUBLIC_KEY(line 48) and conditionally setDATABASE_URL(line 42-43), whichvi.stubEnvhandles differently.frontend/app/[locale]/shop/checkout/success/MonobankRedirectStatus.tsx (2)
287-312:failed,refunded, andcanceledall share the same headline/message keys, but have distinctuiStatevalues.This is intentional per the translation keys, but could confuse future maintainers:
uiState: 'failed'renders the headline"Payment not completed"and the payment status label"Payment failed". Consider adding a brief inline comment explaining the shared headline/message design decision.
470-514: Stale closure risk in polling cycle due touseCallback+setTimeoutinteraction.
runPollingCycleis in the useEffect's dependency array, but theinitializedOrderIdRefguard (Line 471) prevents re-initialization when only callback identity changes (e.g., whenpaymentsDisabledchanges). This means an already-scheduledsetTimeoutcaptures the oldrunPollingCycleclosure, and changes topaymentsDisabledmid-poll won't take effect untilrestartPollingis explicitly called.In practice,
paymentsDisabledis unlikely to change during a single session, so this is low-severity. If you want to address it later, consider using auseReffor the latestrunPollingCycleso the timer always calls the current version.frontend/app/[locale]/shop/checkout/success/page.tsx (1)
177-193: Monobank redirect branch integrates cleanly.Correct early return pattern: cart is cleared, status polling is delegated to
MonobankRedirectStatus, and standard order-loading logic is bypassed entirely.One thing to note:
SHOP_HERO_CTA_SM,SHOP_OUTLINE_BTN, andHeroCtaInnerare duplicated identically in both this file andMonobankRedirectStatus.tsx. Consider extracting them into a shared module to reduce drift over time.frontend/app/api/shop/webhooks/monobank/route.ts (1)
236-245: Minor:modeappears twice in the spread.
diagMetaalready containsmode: webhookMode(Line 135), and Line 239 addsmode: webhookModeagain. The second one wins (same value), so this is functionally harmless but adds noise.Proposed fix
monoLogInfo(MONO_STORE_MODE, { ...diagMeta, - mode: webhookMode, storeDecision: result.appliedResult, eventKey, invoiceId: result.invoiceId, reason: 'STORE_MODE_RESULT', });
…ests; minor security/log/UI cleanups
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/app/api/shop/webhooks/monobank/route.ts`:
- Around line 141-170: The rate-limit response for the missing-signature branch
uses the wrong scope string; update the details.scope passed to
rateLimitResponse in the missing-signature branch (inside the if (!hasXSign)
block) from 'monobank_webhook_invalid_signature' to
'monobank_webhook_missing_signature' so it is distinguishable from the
invalid-signature case (symbols to look for: hasXSign, enforceRateLimit,
rateLimitResponse, monoLogWarn, MONO_SIG_INVALID).
🧹 Nitpick comments (6)
frontend/lib/security/origin.ts (1)
39-47:normalizeOriginfallback returns raw trimmed input on invalid URLs.When
new URL(trimmed)throws, the catch block returnstrimmedas-is (line 45). This means a malformed origin string that isn't a valid URL passes through without normalization, which could cause origin-comparison mismatches (e.g., case sensitivity, trailing characters). Consider whether returning an empty string or throwing would be safer for your allow-list logic.frontend/lib/logging/monobank.ts (1)
160-163:monoSha256Raw— minor:cryptoimport is unused elsewhere in this file.The function is correct, but note that
node:cryptois imported solely for this single function. If this utility is also computed inmonobank-webhook.ts(which does its owncrypto.createHash('sha256')on line 1252-1255), you might want to consolidate to avoid parallel implementations.frontend/app/api/shop/admin/orders/[id]/refund/route.ts (1)
84-92: Fragile admin identity extraction — prefer using the known return type.
requireAdminApireturns a typed user object with a knownidproperty (perfrontend/lib/auth/admin.tsline 39-49 showing it returns auserwithrole). The runtime duck-typing withtypeof adminUser === 'object'and cast toRecord<string, unknown>is unnecessarily fragile and obscures intent.♻️ Suggested simplification
- const adminId = - adminUser && typeof adminUser === 'object' - ? (adminUser as Record<string, unknown>).id - : undefined; - - const adminSubject = - typeof adminId === 'string' - ? `admin_${normalizeRateLimitSubject(adminId)}` - : getRateLimitSubject(request); + const adminSubject = adminUser?.id + ? `admin_${normalizeRateLimitSubject(String(adminUser.id))}` + : getRateLimitSubject(request);frontend/lib/services/orders/monobank-webhook.ts (1)
806-811:monoLogErrorcalled withundefinedas the error argument.In three places (lines 806, 921, 965),
monoLogErroris called withundefinedas theerrorparameter. This flows through tologError(code, undefined, meta)which callsemit('error', context, meta, error)witherror = undefined. While not a bug, it means these error-level log entries carry no error object/stack trace — they're more like error-level warnings. Consider whethermonoLogWarnwould be more semantically accurate for "unexpected but handled" conditions, or if you intentionally want these at error level for alerting.Also applies to: 921-926, 965-971
frontend/app/api/shop/webhooks/monobank/route.ts (2)
108-112:MONO_STORE_MODElogged on every request regardless of mode.This logs the store-mode decision on every single webhook invocation, even when the mode is
'apply'(the default). On a high-traffic webhook endpoint, this produces a log line per request that adds no diagnostic value for the common case. Consider gating this log to only emit when mode is'store'or'drop', or downgrading to a debug/trace level for'apply'.♻️ Log only when mode differs from the default
- monoLogInfo(MONO_STORE_MODE, { - ...baseMeta, - mode: webhookMode, - storeDecision: webhookMode, - }); + if (webhookMode !== 'apply') { + monoLogInfo(MONO_STORE_MODE, { + ...baseMeta, + mode: webhookMode, + storeDecision: webhookMode, + }); + }
129-139: SHA-256 of the webhook body is computed twice — once here, once insidehandleMonobankWebhook.Line 129 computes
rawSha256 = monoSha256Raw(rawBodyBytes), and the same value is recomputed insidehandleMonobankWebhookatmonobank-webhook.tslines 1252-1255 usingcrypto.createHash('sha256').update(rawBodyBuffer).digest('hex'). TherawSha256is used here foreventKeyanddiagMeta, buthandleMonobankWebhookdoes not accept it as an argument — it recomputes it internally.Consider passing
rawSha256through tohandleMonobankWebhookto avoid the redundant hash computation on every webhook call.Also applies to: 227-234
…s) and remove duplicate sha256 hashing
* feat(mobile): improve dashboard, leaderboard & AI helper UX for touch devices - Add touch drag support for AI helper modal and explained terms reorder - Position explain button below selected word on mobile - Show delete/restore buttons always visible on mobile (no hover) - Add user avatar to dashboard profile card (same as leaderboard) - Fix leaderboard page layout - Fix Tailwind v4 canonical class warnings * Added touchcancel listener * (SP: 2) [Frontend] Quiz results dashboard, review cache fix, UX improvements (#317) * (SP: 3) [Backend] add internal janitor (jobs 1-4), claim/lease + runbook (G0-G6) (#318) * (SP: 2) [Frontend] Redesign Home Hero & Add Features Section (#319) * refactor(home): rename hero sections and add complete i18n support - Rename LegacyHeroSection → WelcomeHeroSection - Rename HeroSection → FeaturesHeroSection - Add welcomeDescription translation key to eliminate duplication - Translate all hardcoded text (headings, badges, CTAs) - Improve Ukrainian/Polish translations for better readability - Remove unused legacy components and images * feat(about): update LinkedIn follower count to reflect current stat (1.5k+) * refactor(home): implement i18n for FlipCardQA & fix memory leaks * fix(home): resolve rotateY conflict & scope keyboard events in FlipCardQA * fix(home): resolve all issues * chore(home): cleanup comments, remove dead code & fix trailing spaces * (SP: 2) [Frontend] Quiz UX improvements: violations counter, breadcrumbs, status badges (#320) * feat(quiz): add guest warning before start and bot protection Guest warning: show login/signup/continue buttons for unauthenticated users on quiz rules screen before starting. Bot protection: multi-attempt verification via Redis - each question can only be verified once per user per attempt. Keys use dynamic TTL matching quiz time limit and are cleared on retake. Additional fixes: - Footer flash on quiz navigation (added loading.tsx, eliminated redirect) - Renamed QaLoader to Loader for reuse across pages - React compiler purity errors (crypto.getRandomValues in handlers) - Start button disabled after retake (isStarting not reset) * refactor(quiz): PR review feedback - Extract shared resolveRequestIdentifier() helper to eliminate duplicated auth/IP resolution logic in route.ts and actions/quiz.ts - Return null instead of 'unknown' when identifier unresolvable, skip verification tracking for unidentifiable users - Cap Redis TTL with MAX_TTL (3600s) to prevent client-supplied timeLimitSeconds from persisting keys indefinitely - Add locale prefix to returnTo paths in guest warning links - Replace nested Button inside Link with styled Link to fix invalid HTML (interactive element nesting) * fix(quiz): fall through to IP when auth cookie is expired/invalid * feat(quiz): add quiz results dashboard and review page - Add quiz history section to dashboard with last attempt per quiz - Add review page showing incorrect questions with explanations - Add collapsible cards with expand/collapse all toggle - Add "Review Mistakes" button on quiz result screen - Add category icons to quiz page and review page headers - Add BookOpen icon to explanation block in QuizQuestion - Update guest message to mention error review benefit - Add i18n translations (en/uk/pl) for all new features * fix(quiz): scroll to next button on answer reveal, scope review cache by userId * fix(quiz): restore type imports and userId cache key after merge conflict * fix: restore type imports, sync @swc/helpers, fix indentation after merge * feat(quiz): add violations counter UI, fix disqualification threshold - Add ViolationsCounter component with color escalation (green/yellow/red) - Sticky top bar keeps counter visible on scroll (mobile/tablet) - Add i18n counter keys for en/uk/pl with ICU plural forms - Fix threshold bug: violations warning now triggers at 4+ (was 3+) to match actual integrity score calculation (100 - violations * 10 < 70) * fix(quiz): fix points mismatch between leaderboard and dashboard Dashboard showed raw pointsEarned from last quiz_attempt, while leaderboard summed improvement deltas from point_transactions. Additionally, orphaned transactions from re-seeded quizzes inflated leaderboard totals (12 rows, 83 ghost points cleaned up in DB). - Dashboard query now joins point_transactions to show actual awarded points per quiz instead of raw attempt score - Leaderboard query filters out orphaned transactions where the source attempt no longer exists in quiz_attempts * OBfix(quiz): fix points mismatch, consistent status badges, mobile UX Dashboard showed raw pointsEarned from last attempt while leaderboard summed improvement deltas from point_transactions. Orphaned transactions from re-seeded quizzes inflated leaderboard totals (cleaned up in DB). - Dashboard query joins point_transactions for actual awarded points - Leaderboard query filters orphaned transactions (source_id not in quiz_attempts) - Quiz cards use 3-level badges (Mastered/Review/Study) matching dashboard - Mobile quiz results show dash for zero points, added chevron indicator * fix(quiz): add breadcrumbs to review page, fix recommendation tautology * Header UX polish, quiz highlight fix, Blog button styling, shop i18n product descriptions (#322) * Header UX: reorder languages, swap controls, fix quiz highlight, style Blog button * shop i18n product descriptions * (SP: 1) [Frontend] Q&A: Next.js tab states + faster loader start (#324) * fix(qa): align Next.js tab states and speed up loader startup * feat(home,qa): improve home snap flow and add configurable Q&A page size * fix(i18n,qa,seed): address review issues for locale handling and pagination state * (SP: 1) [Frontend] Align quiz result messages with status badges, fix locale switch on result page (#325) * feat(quiz): add guest warning before start and bot protection Guest warning: show login/signup/continue buttons for unauthenticated users on quiz rules screen before starting. Bot protection: multi-attempt verification via Redis - each question can only be verified once per user per attempt. Keys use dynamic TTL matching quiz time limit and are cleared on retake. Additional fixes: - Footer flash on quiz navigation (added loading.tsx, eliminated redirect) - Renamed QaLoader to Loader for reuse across pages - React compiler purity errors (crypto.getRandomValues in handlers) - Start button disabled after retake (isStarting not reset) * refactor(quiz): PR review feedback - Extract shared resolveRequestIdentifier() helper to eliminate duplicated auth/IP resolution logic in route.ts and actions/quiz.ts - Return null instead of 'unknown' when identifier unresolvable, skip verification tracking for unidentifiable users - Cap Redis TTL with MAX_TTL (3600s) to prevent client-supplied timeLimitSeconds from persisting keys indefinitely - Add locale prefix to returnTo paths in guest warning links - Replace nested Button inside Link with styled Link to fix invalid HTML (interactive element nesting) * fix(quiz): fall through to IP when auth cookie is expired/invalid * feat(quiz): add quiz results dashboard and review page - Add quiz history section to dashboard with last attempt per quiz - Add review page showing incorrect questions with explanations - Add collapsible cards with expand/collapse all toggle - Add "Review Mistakes" button on quiz result screen - Add category icons to quiz page and review page headers - Add BookOpen icon to explanation block in QuizQuestion - Update guest message to mention error review benefit - Add i18n translations (en/uk/pl) for all new features * fix(quiz): scroll to next button on answer reveal, scope review cache by userId * fix(quiz): restore type imports and userId cache key after merge conflict * fix: restore type imports, sync @swc/helpers, fix indentation after merge * feat(quiz): add violations counter UI, fix disqualification threshold - Add ViolationsCounter component with color escalation (green/yellow/red) - Sticky top bar keeps counter visible on scroll (mobile/tablet) - Add i18n counter keys for en/uk/pl with ICU plural forms - Fix threshold bug: violations warning now triggers at 4+ (was 3+) to match actual integrity score calculation (100 - violations * 10 < 70) * fix(quiz): fix points mismatch between leaderboard and dashboard Dashboard showed raw pointsEarned from last quiz_attempt, while leaderboard summed improvement deltas from point_transactions. Additionally, orphaned transactions from re-seeded quizzes inflated leaderboard totals (12 rows, 83 ghost points cleaned up in DB). - Dashboard query now joins point_transactions to show actual awarded points per quiz instead of raw attempt score - Leaderboard query filters out orphaned transactions where the source attempt no longer exists in quiz_attempts * OBfix(quiz): fix points mismatch, consistent status badges, mobile UX Dashboard showed raw pointsEarned from last attempt while leaderboard summed improvement deltas from point_transactions. Orphaned transactions from re-seeded quizzes inflated leaderboard totals (cleaned up in DB). - Dashboard query joins point_transactions for actual awarded points - Leaderboard query filters orphaned transactions (source_id not in quiz_attempts) - Quiz cards use 3-level badges (Mastered/Review/Study) matching dashboard - Mobile quiz results show dash for zero points, added chevron indicator * fix(quiz): add breadcrumbs to review page, fix recommendation tautology * fix(quiz): align result messages with status badges, persist result on locale switch * chore(release): v1.0.0 * feat(jpg): add images for shop * (SP: 3) [Shop][Monobank] Janitor map + internal janitor endpoint stub + status UX + security/obs + J test gate (#328) * (SP: 3) [Backend] add internal janitor (jobs 1-4), claim/lease + runbook (G0-G6) * (SP: 3) [Backend] add provider selector, fix payments gating, i18n checkout errors * Add shop category images to public * (SP: 3) [Shop][Monobank] I1 structured logging: codes + logging safety checks * (SP: 3) [Shop][Monobank] Fail-closed non-browser origin posture for webhook + janitor (ORIGIN_BLOCKED) * (SP: 3) [Shop][Monobank] [Shop][Monobank] J gate: add orders status ownership test and pass all pre-prod invariants * (SP: 3) [Shop][Monobank] review fixes (tests, logging, success UI) * (SP: 1) [Shop][Monobank] Tighten webhook log-code typing; harden DB tests; minor security/log/UI cleanups * (SP: 1) [Shop][Monobank] harden Monobank webhook (origin/PII-safe logs) and remove duplicate sha256 hashing * (SP:2) [Frontend] Fix duplicated Q&A items after content updates (#330) * fix(qa): prevent duplicate questions and improve cache invalidation * fix(qa): keep pagination totals consistent after deduplication * (SP: 1) [Frontend] Integrate online users counter popup and fix header (#331) * feat(home): add online users counter + fix header breakpoint * deleted scrollY in OnlineCounterPopup * fixed fetch in OnlineCounterPopup * Bug/fix qa (#332) * fix(qa): prevent duplicate questions and improve cache invalidation * fix(qa): keep pagination totals consistent after deduplication * fix(qa): paginate by unique questions and bump cache namespace * chore(release): v1.0.1 --------- Co-authored-by: tetiana zorii <tanyusha.zoriy@gmail.com> Co-authored-by: Lesia Soloviova <106915140+LesiaUKR@users.noreply.github.com> Co-authored-by: liudmylasovetovs <127711697+liudmylasovetovs@users.noreply.github.com> Co-authored-by: Yevhenii Datsenko <134847096+yevheniidatsenko@users.noreply.github.com> Co-authored-by: Tetiana Zorii <131365289+TiZorii@users.noreply.github.com> Co-authored-by: Yuliia Nazymko <122815071+YNazymko12@users.noreply.github.com>
2 participants
Description
This PR hardens the Monobank payment lifecycle around reliability, safety, and operator visibility:
/status(no PSP calls from UI),orders/[id]/status(no IDOR).Related Issue
Issue: #<issue_number>
Changes
docs/monobank-janitor-map.mddocumenting exact file paths, enums/statuses, timestamps, tables, and existing job/claim mechanics to prevent janitor logic from inventing integrations.POST /api/shop/internal/monobank/janitor(stub) with non-browser guard, internal secret auth, strict payload validation, DB-based spam/rate gate (pattern aligned withrestock-stale), and consistent structured logs + error contracts.payment_attempts(and groundwork for future jobs), without changing existing business state machine behavior./statusonly;Cache-Control: no-store; reduced token leakage risk./orders/[id]/statuscannot read foreign orders).Database Changes (if applicable)
How Has This Been Tested?
Checklist
Before submitting
Reviewers
Summary by CodeRabbit
New Features
Localization
Stability & Security
Tests