User Data Management: Frontend Pages#1476
Open
ThePitter wants to merge 26 commits intosillsdev:developfrom
Open
User Data Management: Frontend Pages#1476ThePitter wants to merge 26 commits intosillsdev:developfrom
ThePitter wants to merge 26 commits intosillsdev:developfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a User Data Management (UDM) deletion-request flow: unauthenticated pages and API for email+Turnstile verification, manifest/icon/theme utilities, localization entries, DB-backed confirmation codes, a scheduled cleanup job, and related types/utilities. Changes
Sequence DiagramsequenceDiagram
participant User as User (Browser)
participant UI as Deletion Request Page
participant Turnstile as Cloudflare Turnstile
participant API as /api/delete-request
participant DB as Database
participant Email as Email Service
participant Confirm as Confirmation Page
User->>UI: Enter email & select deletion scope
UI->>Turnstile: Render widget / request token
User->>Turnstile: Complete captcha
Turnstile-->>UI: Return token
UI->>API: POST { email, token, productId, deletionType }
API->>Turnstile: Verify token with secret
Turnstile-->>API: Verification result
alt verification failed
API-->>UI: 400 error (verification failed)
UI->>User: Show error
else verification succeeded
API->>API: Generate 6-digit code + expiry
API->>DB: Insert productUserChanges record
DB-->>API: Acknowledge
API->>Email: Send verification email (code)
Email-->>API: Sent
API-->>UI: { success: true }
UI->>User: Redirect to Confirm page
User->>Confirm: Submit code
Confirm->>API: verifyCode action
API->>DB: Validate code & mark confirmed
API-->>Confirm: success / failure
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly Related PRs
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (6)
src/routes/(unauthenticated)/products/[id]/user-data/+layout.server.ts (2)
240-246: Consider parallelizing text file fetches for better performance.The title, short description, and long description files are fetched sequentially. Since they're independent, fetching them in parallel would reduce overall latency.
⚡ Proposed optimization
- const title = await fetchTextFile(baseUrl, files, language, 'title.txt'); - const shortDesc = await fetchTextFile(baseUrl, files, language, 'short_description.txt'); - const longDesc = - (await fetchTextFile(baseUrl, files, language, 'full_description.txt')) || - (await fetchTextFile(baseUrl, files, language, 'description.txt')); + const [title, shortDesc, fullDesc, fallbackDesc] = await Promise.all([ + fetchTextFile(baseUrl, files, language, 'title.txt'), + fetchTextFile(baseUrl, files, language, 'short_description.txt'), + fetchTextFile(baseUrl, files, language, 'full_description.txt'), + fetchTextFile(baseUrl, files, language, 'description.txt') + ]); + const longDesc = fullDesc || fallbackDesc;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(unauthenticated)/products/[id]/user-data/+layout.server.ts around lines 240 - 246, The three independent text fetches (title, shortDesc, longDesc) are done sequentially; change to run them in parallel by using Promise.all to await multiple fetchTextFile calls concurrently (reference fetchTextFile, title, shortDesc variables). For longDesc, initiate both fetches for 'full_description.txt' and 'description.txt' in parallel and then pick the first non-empty result (or fallback logic) instead of awaiting them serially; keep baseUrl, files, and language as arguments to each call and preserve existing error/empty-result handling.
204-210: Add timeout to external manifest fetch to prevent hanging requests.The
fetchcall to retrieve the manifest JSON has no timeout. If the storage service is slow or unresponsive, this could cause the request to hang indefinitely.♻️ Proposed fix using AbortController
let manifest: PlayListingManifest | null = null; try { - manifest = (await fetch(manifestArtifact.Url).then((r) => r.json())) as PlayListingManifest; + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 5000); + const res = await fetch(manifestArtifact.Url, { signal: controller.signal }); + clearTimeout(timeoutId); + manifest = (await res.json()) as PlayListingManifest; } catch { manifest = null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(unauthenticated)/products/[id]/user-data/+layout.server.ts around lines 204 - 210, The fetch of manifestArtifact.Url to populate manifest (PlayListingManifest) has no timeout and can hang; wrap the fetch in an AbortController with a short timeout (e.g., 2–5s) via setTimeout to call controller.abort(), pass controller.signal to fetch(manifestArtifact.Url, { signal }), clear the timer on success, and handle the abort by catching the thrown error and treating it the same as other failures (set manifest = null) so page rendering still continues; update the try/catch around the fetch in +layout.server.ts where manifest is assigned.src/routes/(unauthenticated)/api/delete-request/+server.ts (1)
45-56: Consider handling duplicate pending verification requests.The schema has no unique constraint on
(ProductId, Email), allowing multiple pending verification records for the same user. This could lead to:
- Database bloat from repeated requests
- Confusion about which code is valid
Consider either updating existing pending records or deleting old unconfirmed requests before creating a new one.
♻️ Example: Delete old unconfirmed requests first
+ // Remove any existing unconfirmed requests for this email/product + await DatabaseWrites.productUserChanges.deleteMany({ + where: { + ProductId: productId, + Email: normalizedEmail, + DateConfirmed: null + } + }); await DatabaseWrites.productUserChanges.create({ data: { ProductId: productId, Email: normalizedEmail,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(unauthenticated)/api/delete-request/+server.ts around lines 45 - 56, Before inserting a new verification row via DatabaseWrites.productUserChanges.create, first locate and remove or update any existing unconfirmed records for the same ProductId and Email (where DateConfirmed is null and DateExpires is in the future/expired as desired) to prevent multiple pending requests; modify the flow in the delete-request handler to either delete old records (using ProductId and normalizedEmail with DateConfirmed === null) or update them to a single canonical pending row, then create the new record with ConfirmationCode, DateExpires, DateCreated/DateUpdated and DateConfirmed set to null so there is always at most one pending verification per (ProductId, Email).src/routes/(unauthenticated)/products/[id]/user-data/+page.svelte (3)
190-192: Email in URL query parameter may be logged in browser history and server access logs.Passing the email via query string exposes it in browser history, bookmarks, and potentially server access logs. Consider using a session-based approach or POST to the confirmation page.
One alternative is to store a short-lived reference ID in the database and redirect using that ID instead of the email address.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(unauthenticated)/products/[id]/user-data/+page.svelte around lines 190 - 192, The code builds confirmUrl and calls window.location.assign(confirmUrl.toString()) while putting the user's email in the query string (confirmUrl.searchParams.set('email', email)), which leaks PII to browser history and logs; instead generate a short-lived reference token (e.g. store a mapping token -> email server-side) and redirect using that token or submit the email via a POST or session storage; update the flow so the client calls your backend to create the reference ID, then use confirmUrl with the token (not the raw email) when calling window.location.assign, and ensure server-side handlers (for the token) resolve the email and expire the token.
180-188: Add resilience for non-JSON error responses.If the server returns a non-JSON response (e.g., HTML error page),
res.json()will throw. Consider adding a try-catch or checking the content-type first.♻️ Proposed fix
- const data = await res.json(); + let data: { success?: boolean }; + try { + data = await res.json(); + } catch { + alert('Verification failed.'); + window.turnstile?.reset?.(); + turnstileToken = null; + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(unauthenticated)/products/[id]/user-data/+page.svelte around lines 180 - 188, Wrap the res.json() call in a safe parse path: check res.headers.get('content-type') for "application/json" or try/catch the await res.json() so non-JSON responses don't throw; if parsing fails, handle it the same as an error (alert 'Verification failed.', call window.turnstile?.reset?.(), set turnstileToken = null and return). Update the block that uses res, data, turnstileToken and window.turnstile?.reset?.() to use this defensive parsing approach.
388-392: Hardcoded Turnstile sitekey should come from environment configuration.The sitekey
0x4AAAAAACW5LHZjYD-EBWMsis hardcoded. This makes it difficult to use different sitekeys for development/staging/production environments and exposes the key in source control.Consider passing the sitekey from the server via page data or using a public environment variable (
PUBLIC_TURNSTILE_SITEKEY).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(unauthenticated)/products/[id]/user-data/+page.svelte around lines 388 - 392, The Turnstile sitekey is hardcoded in the cf-turnstile div; replace it with a configurable value by sourcing the key from environment/public config and passing it into the page component (for example expose PUBLIC_TURNSTILE_SITEKEY from the runtime env and use the page's load data or import from $env/* to populate the data-sitekey attribute). Update the component where the div and handleTurnstileSuccess are used to read the sitekey from page data (or the public env import) instead of the literal string, and add a server-side or top-level public env reference (e.g., in the page load or +page.server/+layout load) to surface the PUBLIC_TURNSTILE_SITEKEY for dev/staging/production differences.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/routes/`(unauthenticated)/api/delete-request/+server.ts:
- Around line 16-20: The code currently uses a non-null assertion on
process.env.TURNSTILE_SECRET_KEY (secret!) which will crash at runtime if the
env var is missing; update the handler in +server.ts to first read const secret
= process.env.TURNSTILE_SECRET_KEY and validate it (if (!secret) return a
Response with an appropriate error message and status like 500 or 503 indicating
missing server configuration), then use formData.append('secret', secret)
without the non-null assertion; ensure the response body clearly indicates the
missing TURNSTILE_SECRET_KEY so callers and logs can surface the configuration
error.
- Around line 41-43: The console.log exposing the raw verification code (using
variables normalizedEmail, productId, code) must be removed or redacted; update
the logging call so it never prints the full OTP — either remove it entirely in
production, or replace it with a non-sensitive message (e.g., "verification code
sent for {normalizedEmail} product {productId}") or a masked value (show only
last 1–2 characters) and gate any debugging logs behind an explicit
non-production check; modify the statement that currently constructs the string
with code to use the redacted/masked message or conditional debug logging
instead.
- Line 38: The verification code generation uses Math.random() (variable code)
which is not cryptographically secure; update the implementation in the handler
that defines const code = ... to use Node's crypto module (import crypto) and
generate a secure 6-digit integer (e.g., crypto.randomInt(100000, 1000000)) or
derive digits from crypto.randomBytes, then convert to string; ensure the crypto
import is added and replace the Math.random-based expression with the
crypto-based call to produce a zero-padded 6-digit string if needed.
In `@src/routes/`(unauthenticated)/products/[id]/confirm/+page.server.ts:
- Around line 170-172: The console.log in +page.server.ts is emitting a live
one-time code and PII (normalizedEmail, params.id, code); remove this statement
entirely or replace it with a non-sensitive audit log that does not include the
verification code or full email (e.g., log an event like
"sent_verification_code" with a masked email or a hashed identifier and the
product id if needed). Locate the console.log(...) call that references
normalizedEmail, params.id, and code and either delete it or change it to log
only non-sensitive metadata (maskedEmail or hashedEmail and no code).
- Around line 161-168: The code is pre-rendering EmailLayoutTemplate before
calling sendEmail, which causes the layout to be nested because sendEmail
already injects body into EmailLayoutTemplate; update the call in the handler
that calls sendEmail so the third argument is the raw body data (e.g., an object
or string containing the subject/content like { INSERT_SUBJECT: `Your code is:
${code}`, INSERT_CONTENT: `Your code is: ${code}` } or just the content string)
instead of addProperties(EmailLayoutTemplate, ...); change the call that
currently uses addProperties(EmailLayoutTemplate, {...}) to pass only the inner
content payload so sendEmail can apply EmailLayoutTemplate itself.
- Around line 147-158: Before inserting a new confirmation row, expire any prior
pending codes for the same product/email so old codes cannot be used later: run
an update (e.g., DatabaseWrites.productUserChanges.updateMany or equivalent)
filtering on ProductId === params.id, Email === normalizedEmail, DateConfirmed
=== null and DateExpires > now and set DateExpires = now (or set a sentinel like
DateConfirmed = now) to invalidate them, then proceed with
DatabaseWrites.productUserChanges.create to insert the new code; if your data
layer supports transactions, perform the update + create inside a transaction
(see DatabaseWrites.productUserChanges.updateMany and
DatabaseWrites.productUserChanges.create) to avoid race conditions.
- Line 236: The handler in
src/routes/(unauthenticated)/products/[id]/confirm/+page.server.ts currently
throws redirect(303, '/dashboard') on successful verification which sends
unauthenticated clients into the app; instead, remove that redirect and return
page data so the public confirmation page is rendered (e.g., replace the throw
redirect(303, '/dashboard') line with a return of a success payload like {
verified: true } or a redirect to the same confirmation route), ensuring the
confirmation view is shown for non-enhanced clients.
- Around line 141-142: Replace the insecure Math.random() used to generate the
6-digit OTP (the const code assignment) with Node's cryptographic RNG: call
crypto.randomInt(100000, 1000000) to produce a uniformly distributed 6-digit
integer and convert it to a string; also add the required import for the crypto
module (e.g., import { randomInt } from 'crypto' or import * as crypto from
'crypto') and use that symbol (randomInt or crypto.randomInt) in the code
generation spot so the OTP is generated securely without modulo bias.
In `@src/routes/`(unauthenticated)/products/[id]/confirm/+page.svelte:
- Around line 196-208: handleVerifyCode is discarding the server's specific
error string because it only reads result.data.message; update the result
handling in handleVerifyCode (the returned async ({ result }) => { ... } block)
to read result.data.error first (falling back to result.data.message and then
the generic string) and assign that to the error variable so server messages
like "Code expired" or "No code sent to this email" are preserved.
- Around line 245-343: The page uses hardcoded English strings for the headings,
instructions, buttons and success text (when rendering by step, and in the forms
handled by handleSendCode and handleVerifyCode), so the udm_* message catalog
entries added in the PR never get used; replace all hardcoded text inside the h1
(step branches), the descriptive paragraphs, the button labels ("Send Code",
"Verify Code", "Change Email"), the "Didn't receive the code?" line and the
verification-complete messages with calls to the app's message/translation
helper using the corresponding udm_* keys (and interpolate {email} where
needed). Locate the UI by the unique symbols step, email, code, handleSendCode
and handleVerifyCode and import/use the existing translation function (e.g. the
project's translate/t/$i18n helper) so the udm_* keys are rendered instead of
the hardcoded English.
- Around line 175-210: The step state is only updated client-side inside
handleSendCode and handleVerifyCode, making progression JS-only; to persist it,
initialize step from page data (e.g. data.step ?? (initialEmail ? 'code' :
'email')) and change your form actions to return a step value in their action
response; then in handleSendCode and handleVerifyCode, after checking
result.type, read result.data.step (if present) and assign step =
result.data.step (falling back to the existing local assignments), so the
server-side POST response can drive the UI transition and the state survives
non-JS navigation.
In `@src/routes/`(unauthenticated)/products/[id]/user-data/+page.svelte:
- Around line 333-362: Radio selection named deletionType isn't captured because
the radio inputs lack value attributes and the submit handler doesn't read the
selected value; add value="data" to the radio next to m.udm_scope_data_title()
and value="account" to the radio next to m.udm_scope_account_title(), then
either bind the group to a component variable (e.g., bind:group={deletionType})
or ensure the form submit handler (the on:submit handler that sends the API
request) reads the value via FormData.get('deletionType') and includes that
value in the payload sent to the backend (update the handler that constructs the
API request to include deletionType).
---
Nitpick comments:
In `@src/routes/`(unauthenticated)/api/delete-request/+server.ts:
- Around line 45-56: Before inserting a new verification row via
DatabaseWrites.productUserChanges.create, first locate and remove or update any
existing unconfirmed records for the same ProductId and Email (where
DateConfirmed is null and DateExpires is in the future/expired as desired) to
prevent multiple pending requests; modify the flow in the delete-request handler
to either delete old records (using ProductId and normalizedEmail with
DateConfirmed === null) or update them to a single canonical pending row, then
create the new record with ConfirmationCode, DateExpires,
DateCreated/DateUpdated and DateConfirmed set to null so there is always at most
one pending verification per (ProductId, Email).
In `@src/routes/`(unauthenticated)/products/[id]/user-data/+layout.server.ts:
- Around line 240-246: The three independent text fetches (title, shortDesc,
longDesc) are done sequentially; change to run them in parallel by using
Promise.all to await multiple fetchTextFile calls concurrently (reference
fetchTextFile, title, shortDesc variables). For longDesc, initiate both fetches
for 'full_description.txt' and 'description.txt' in parallel and then pick the
first non-empty result (or fallback logic) instead of awaiting them serially;
keep baseUrl, files, and language as arguments to each call and preserve
existing error/empty-result handling.
- Around line 204-210: The fetch of manifestArtifact.Url to populate manifest
(PlayListingManifest) has no timeout and can hang; wrap the fetch in an
AbortController with a short timeout (e.g., 2–5s) via setTimeout to call
controller.abort(), pass controller.signal to fetch(manifestArtifact.Url, {
signal }), clear the timer on success, and handle the abort by catching the
thrown error and treating it the same as other failures (set manifest = null) so
page rendering still continues; update the try/catch around the fetch in
+layout.server.ts where manifest is assigned.
In `@src/routes/`(unauthenticated)/products/[id]/user-data/+page.svelte:
- Around line 190-192: The code builds confirmUrl and calls
window.location.assign(confirmUrl.toString()) while putting the user's email in
the query string (confirmUrl.searchParams.set('email', email)), which leaks PII
to browser history and logs; instead generate a short-lived reference token
(e.g. store a mapping token -> email server-side) and redirect using that token
or submit the email via a POST or session storage; update the flow so the client
calls your backend to create the reference ID, then use confirmUrl with the
token (not the raw email) when calling window.location.assign, and ensure
server-side handlers (for the token) resolve the email and expire the token.
- Around line 180-188: Wrap the res.json() call in a safe parse path: check
res.headers.get('content-type') for "application/json" or try/catch the await
res.json() so non-JSON responses don't throw; if parsing fails, handle it the
same as an error (alert 'Verification failed.', call
window.turnstile?.reset?.(), set turnstileToken = null and return). Update the
block that uses res, data, turnstileToken and window.turnstile?.reset?.() to use
this defensive parsing approach.
- Around line 388-392: The Turnstile sitekey is hardcoded in the cf-turnstile
div; replace it with a configurable value by sourcing the key from
environment/public config and passing it into the page component (for example
expose PUBLIC_TURNSTILE_SITEKEY from the runtime env and use the page's load
data or import from $env/* to populate the data-sitekey attribute). Update the
component where the div and handleTurnstileSuccess are used to read the sitekey
from page data (or the public env import) instead of the literal string, and add
a server-side or top-level public env reference (e.g., in the page load or
+page.server/+layout load) to surface the PUBLIC_TURNSTILE_SITEKEY for
dev/staging/production differences.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3f82bfaf-2930-41f4-9ab1-cfc45bf3f24d
📒 Files selected for processing (22)
.codeclimate.yml.vscode/settings.jsonREADME.mddeployment/ci/docker-compose.ymldeployment/development/docker-compose.ymleslint-rules/tsconfig.jsonrenovate.jsonsrc/app.csssrc/app.d.tssrc/lib/locales/en-US.jsonsrc/lib/locales/es-419.jsonsrc/lib/locales/fr-FR.jsonsrc/lib/otel/development_config.ymlsrc/lib/otel/production_config.ymlsrc/routes/(unauthenticated)/+layout.server.tssrc/routes/(unauthenticated)/+layout.sveltesrc/routes/(unauthenticated)/api/delete-request/+server.tssrc/routes/(unauthenticated)/products/[id]/confirm/+page.server.tssrc/routes/(unauthenticated)/products/[id]/confirm/+page.sveltesrc/routes/(unauthenticated)/products/[id]/user-data/+layout.server.tssrc/routes/(unauthenticated)/products/[id]/user-data/+page.sveltesrc/routes/(unauthenticated)/products/[id]/user-data/about/+page.svelte
src/routes/(unauthenticated)/products/[id]/confirm/+page.server.ts
Outdated
Show resolved
Hide resolved
src/routes/(unauthenticated)/products/[id]/confirm/+page.server.ts
Outdated
Show resolved
Hide resolved
src/routes/(unauthenticated)/products/[id]/confirm/+page.server.ts
Outdated
Show resolved
Hide resolved
src/routes/(unauthenticated)/products/[id]/confirm/+page.svelte
Outdated
Show resolved
Hide resolved
Comment on lines
+196
to
+208
| const handleVerifyCode: SubmitFunction = () => { | ||
| loading = true; | ||
| error = ''; | ||
| return async ({ result }) => { | ||
| loading = false; | ||
| if (result.type === 'success') { | ||
| step = 'verified'; | ||
| } else if (result.type === 'redirect') { | ||
| step = 'verified'; | ||
| } else if (result.type === 'failure') { | ||
| const message = (result.data as { message?: string } | undefined)?.message; | ||
| error = message || 'Invalid code. Please check your email and try again.'; | ||
| } |
Contributor
There was a problem hiding this comment.
Read the server's error field in the verify handler.
verifyCode failures return { error: ... }, so the specific server messages like “Code expired” and “No code sent to this email” are currently discarded in favor of the generic fallback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/`(unauthenticated)/products/[id]/confirm/+page.svelte around lines
196 - 208, handleVerifyCode is discarding the server's specific error string
because it only reads result.data.message; update the result handling in
handleVerifyCode (the returned async ({ result }) => { ... } block) to read
result.data.error first (falling back to result.data.message and then the
generic string) and assign that to the error variable so server messages like
"Code expired" or "No code sent to this email" are preserved.
…code entering from this page to the one build in UDM
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/routes/(unauthenticated)/products/[id]/user-data/+layout.server.ts (1)
204-211: Consider adding a timeout to the manifest fetch.If the artifact storage is slow or unresponsive, the
fetchcall could hang indefinitely, blocking page load. A timeout (e.g., viaAbortSignal.timeout()) would improve resilience.♻️ Optional timeout example
let manifest: PlayListingManifest | null = null; try { - manifest = (await fetch(manifestArtifact.Url).then((r) => r.json())) as PlayListingManifest; + manifest = (await fetch(manifestArtifact.Url, { signal: AbortSignal.timeout(5000) }).then((r) => r.json())) as PlayListingManifest; } catch { manifest = null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(unauthenticated)/products/[id]/user-data/+layout.server.ts around lines 204 - 211, The manifest fetch in the block that sets `let manifest: PlayListingManifest | null = null` should use a request timeout to avoid hanging; update the fetch against `manifestArtifact.Url` to pass an abort signal (e.g., via AbortSignal.timeout(ms)) and handle the abort/error in the existing try/catch so `manifest` stays null on timeout; ensure you reference the same `manifest` variable and `PlayListingManifest` typing and keep the early return `if (!manifest) return { app };`.src/routes/(unauthenticated)/+layout.server.ts (1)
18-23: Consider eliminating the TOCTOU race by catching readFile errors directly.The
existsSynccheck followed byreadFilehas a minor time-of-check to time-of-use gap. For static locale files this is very low risk, but you could simplify by attempting to read and catchingENOENT.♻️ Alternative approach
- let ret = null; - if (existsSync(filePath)) { - const file = (await readFile(filePath)).toString(); - ret = JSON.parse(file) as Entries<L10NKeys, Entries<string, string>>; - } + let ret = null; + try { + const file = (await readFile(filePath)).toString(); + ret = JSON.parse(file) as Entries<L10NKeys, Entries<string, string>>; + } catch { + ret = null; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(unauthenticated)/+layout.server.ts around lines 18 - 23, Remove the time-of-check/time-of-use pattern around existsSync/filePath and instead try reading the file directly: wrap the readFile(filePath) call in a try/catch, JSON.parse the file on success and assign to ret (keeping the same type annotation Entries<L10NKeys, Entries<string, string>>), set ret = null if the caught error is an ENOENT (file not found), and rethrow or propagate any other errors; update the return to still be [locale, ret] as [Locale, typeof ret].src/routes/(unauthenticated)/products/[id]/confirm/+page.server.ts (1)
29-64: Extract shared helpers to reduce duplication.
resolveIconUrlandgetLatestBuiltFileare nearly identical to the implementations insrc/routes/(unauthenticated)/products/[id]/user-data/+layout.server.ts. Consider extracting these to a shared module (e.g.,$lib/products/manifest-utils.ts).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(unauthenticated)/products/[id]/confirm/+page.server.ts around lines 29 - 64, Two helper functions, resolveIconUrl and getLatestBuiltFile, are duplicated across modules; extract them into a single shared utility module and import them where needed: create a new module that exports resolveIconUrl and getLatestBuiltFile (and any dependent types such as ArtifactRef or DatabaseReads wrappers), move the implementations there, update the current file to import and use those exported functions instead of keeping local copies, and ensure any relative dependencies (DatabaseReads, URL usage) are provided or re-exported so both original locations compile and pass existing tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/routes/`(unauthenticated)/+layout.server.ts:
- Around line 18-22: The JSON.parse call when reading the locale file (the block
that assigns ret after readFile(filePath)) should be wrapped in a try-catch so a
malformed ldml.json doesn't throw and abort the Promise.all; catch parsing
errors around JSON.parse(file) in +layout.server.ts (the code that reads
filePath into file and sets ret = JSON.parse(file) as Entries<...>), log or
handle the error (e.g., processLogger.warn or console.warn) and leave ret as
null (or skip that locale) to allow other locales to continue loading.
---
Nitpick comments:
In `@src/routes/`(unauthenticated)/+layout.server.ts:
- Around line 18-23: Remove the time-of-check/time-of-use pattern around
existsSync/filePath and instead try reading the file directly: wrap the
readFile(filePath) call in a try/catch, JSON.parse the file on success and
assign to ret (keeping the same type annotation Entries<L10NKeys,
Entries<string, string>>), set ret = null if the caught error is an ENOENT (file
not found), and rethrow or propagate any other errors; update the return to
still be [locale, ret] as [Locale, typeof ret].
In `@src/routes/`(unauthenticated)/products/[id]/confirm/+page.server.ts:
- Around line 29-64: Two helper functions, resolveIconUrl and
getLatestBuiltFile, are duplicated across modules; extract them into a single
shared utility module and import them where needed: create a new module that
exports resolveIconUrl and getLatestBuiltFile (and any dependent types such as
ArtifactRef or DatabaseReads wrappers), move the implementations there, update
the current file to import and use those exported functions instead of keeping
local copies, and ensure any relative dependencies (DatabaseReads, URL usage)
are provided or re-exported so both original locations compile and pass existing
tests.
In `@src/routes/`(unauthenticated)/products/[id]/user-data/+layout.server.ts:
- Around line 204-211: The manifest fetch in the block that sets `let manifest:
PlayListingManifest | null = null` should use a request timeout to avoid
hanging; update the fetch against `manifestArtifact.Url` to pass an abort signal
(e.g., via AbortSignal.timeout(ms)) and handle the abort/error in the existing
try/catch so `manifest` stays null on timeout; ensure you reference the same
`manifest` variable and `PlayListingManifest` typing and keep the early return
`if (!manifest) return { app };`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a9fea19a-8620-4cf1-95ee-19c0450fd038
📒 Files selected for processing (12)
src/app.d.tssrc/lib/locales/en-US.jsonsrc/lib/locales/es-419.jsonsrc/lib/locales/fr-FR.jsonsrc/routes/(unauthenticated)/+layout.server.tssrc/routes/(unauthenticated)/+layout.sveltesrc/routes/(unauthenticated)/api/delete-request/+server.tssrc/routes/(unauthenticated)/products/[id]/confirm/+page.server.tssrc/routes/(unauthenticated)/products/[id]/confirm/+page.sveltesrc/routes/(unauthenticated)/products/[id]/user-data/+layout.server.tssrc/routes/(unauthenticated)/products/[id]/user-data/+page.sveltesrc/routes/(unauthenticated)/products/[id]/user-data/about/+page.svelte
🚧 Files skipped from review as they are similar to previous changes (6)
- src/app.d.ts
- src/routes/(unauthenticated)/api/delete-request/+server.ts
- src/lib/locales/es-419.json
- src/lib/locales/en-US.json
- src/routes/(unauthenticated)/products/[id]/user-data/+page.svelte
- src/routes/(unauthenticated)/products/[id]/user-data/about/+page.svelte
FyreByrd
requested changes
Mar 16, 2026
Contributor
FyreByrd
left a comment
FyreByrd
left a comment
There was a problem hiding this comment.
All of the coderabbit feedback seems relevant to me and should also be addressed.
There is a lot of duplicated code that could be moved to shared files.
If you're pulling significant amounts of code from outside (i.e. the color conversion stuff) please leave a comment crediting where you got it from.
We use a forms library for pretty much every page action handler. Please use it for consistency.
| "udm_scope_account_desc": "This will permanently remove your login and saved content.", | ||
| "udm_scope_warning": "Warning: Deletions are permanent and cannot be undone. Some data may be retained for legal or compliance purposes.", | ||
| "udm_items_removed_title": "Items to be removed", | ||
| "udm_item_bookmarks": "Bookmarks", |
Contributor
There was a problem hiding this comment.
Bookmarks, Notes, Highlights, and Reading Plan progress could probably be combined into one i18n key. See directory_searchHelp for how this would work.
Author
There was a problem hiding this comment.
Ok I did some research, it's typically frowned upon in practice to include HTML tags in translations and standard workflows now, plus it would likely require using Svelte's @html tag which gets flagged for potential XSS scripting vulnerabilities. I can change it if you want, but for now I am going to leave it as is.
Contributor
There was a problem hiding this comment.
This file should not have been edited/reformatted. Please make sure your formatting configuration is correct.
Author
There was a problem hiding this comment.
I'm not seeing changes on my end, but I'll double check.
Contributor
There was a problem hiding this comment.
This file should not have been edited/reformatted. Please make sure your formatting configuration is correct.
Author
There was a problem hiding this comment.
I'm not seeing changes on my end, but I'll double check.
| Email: normalizedEmail, | ||
| Change: 'User data deletion request verification', | ||
| DateCreated: new Date(), | ||
| DateUpdated: new Date(), |
Contributor
There was a problem hiding this comment.
DateCreated and DateUpdated are automatically handled by Prisma
| } | ||
|
|
||
| const code = Math.floor(100000 + Math.random() * 900000).toString(); | ||
| const expiresAt = new Date(Date.now() + 10 * 60 * 1000); |
Contributor
There was a problem hiding this comment.
A comment here explaining the expiry date would be helpful.
| const iconSrc = app.icon ?? DEFAULT_ICON; | ||
| let themeColor = $state(app.themeColor ?? '#0e795b'); | ||
|
|
||
| function hexToRgb(hex: string) { |
Contributor
There was a problem hiding this comment.
So........ Why are all of these color conversion functions duplicated between here and the confirmation page rather than extracted to a shared file?
Contributor
There was a problem hiding this comment.
The tailwind classes are a bit of a mess. Please use DaisyUI classes where possible.
Contributor
There was a problem hiding this comment.
This does not need to be reformatted
Author
There was a problem hiding this comment.
Undone in a commit I am making, added newline back.
|
|
||
| <div | ||
| data-theme="light" | ||
| class="grid w-full h-full place-items-center place-content-center [background-color:#0068a6]" |
Contributor
There was a problem hiding this comment.
I don't think you needed to change the styling in this layout. If you wanted some specific changes that are not compatible with this layout, there are probably better ways to do it. Try looking at the @ notation for Svelte file names.
Author
There was a problem hiding this comment.
I couldn't find anything that discusses the @ in file names. However, I did undo the formatting and will put it in a separate file for these pages.
Contributor
There was a problem hiding this comment.
I don't want this to be in the root unauthenticated layout. It may be better to move it further down into the UDM part of the routes, since that is where you are using it.
…on requests. It is set to run at midnight daily.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/routes/(unauthenticated)/products/[id]/confirm/+page.svelte (2)
206-209:⚠️ Potential issue | 🟡 MinorServer error messages are discarded due to field name mismatch.
The server's
verifyCodeaction returnsfail(400, { error: '...' })with messages like "Code expired", "No code sent to this email", and "Invalid code". However, the client readsresult.data.messageinstead ofresult.data.error, causing all server errors to be replaced with the generic fallback.🐛 Proposed fix
} else if (result.type === 'failure') { - const message = (result.data as { message?: string } | undefined)?.message; + const data = result.data as { error?: string; message?: string } | undefined; + const message = data?.error ?? data?.message; error = message || 'Invalid code. Please check your email and try again.'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(unauthenticated)/products/[id]/confirm/+page.svelte around lines 206 - 209, The client is reading result.data.message but the server returns fail(400, { error: '...' }), so update the failure branch (the block checking result.type === 'failure' in the confirm handler) to extract the error from result.data.error (and fall back to result.data.message or a generic string) and assign that to the local error variable; adjust the type assertion used on result.data accordingly so both { error?: string } and { message?: string } are supported.
246-254: 🛠️ Refactor suggestion | 🟠 MajorHardcoded English strings should use translation keys.
As noted in a previous review, all UI text (headings, instructions, buttons, success messages) should use the
udm_*translation keys added in this PR. This applies to the step headings here and throughout the template.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(unauthenticated)/products/[id]/confirm/+page.svelte around lines 246 - 254, The h1 currently renders hardcoded English strings based on the step variable; replace those literals inside the conditional (the block that checks step === 'verified' / 'code' / default) with the udm_* translation keys (e.g., udm_verified, udm_check_your_email, udm_enter_your_email) using the project's translation helper (the same pattern used elsewhere in this template, e.g., the $t or t function/ store). Update the conditional branches in the h1 to call the translation function with the appropriate udm_* keys so the headings are localized.
🧹 Nitpick comments (3)
src/routes/(unauthenticated)/products/[id]/confirm/+page.svelte (3)
299-307: Consider addinginputmode="numeric"for the verification code input.Since the code is 6 digits, adding
inputmode="numeric"will show a numeric keyboard on mobile devices, improving the user experience:<input type="text" name="code" bind:value={code} placeholder="000000" maxlength="6" + inputmode="numeric" + pattern="[0-9]*" autocomplete="one-time-code"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(unauthenticated)/products/[id]/confirm/+page.svelte around lines 299 - 307, The verification code input (the <input> bound to the variable `code` in the confirm page) is missing an inputmode, so mobile devices may not surface a numeric keyboard; add inputmode="numeric" to the <input name="code" bind:value={code} ...> element to ensure a numeric keyboard appears for the 6-digit one-time code and improve UX.
190-193: Apply the same fix tohandleSendCodefor consistency.The
sendCodeaction may also return error fields. For consistency and future-proofing, read botherrorandmessagefields:♻️ Proposed fix
} else if (result.type === 'failure') { - const message = (result.data as { message?: string } | undefined)?.message; + const data = result.data as { error?: string; message?: string } | undefined; + const message = data?.error ?? data?.message; error = message || 'Failed to send code. Please try again.'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(unauthenticated)/products/[id]/confirm/+page.svelte around lines 190 - 193, The handleSendCode branch that handles result.type === 'failure' should mirror the other handler by reading both possible error fields; update the failure case in handleSendCode to extract (result.data as { error?: string; message?: string } | undefined) and set the component error variable to error || message || a default like 'Failed to send code. Please try again.' so both 'error' and 'message' responses from sendCode are respected.
158-174: Remove or address the uncertain comment; consolidaterootreference.The comment "This is Svelte 5, do we support this yet?" suggests uncertainty. Since Svelte 5 is being used throughout this file with
$state,$derived, and$effect, this comment is outdated and should be removed.Additionally, the code inconsistently references both
rootanddocument.documentElement:$effect(() => { if (typeof document !== 'undefined') { const root = document.documentElement; root.style.setProperty('--color-primary', primaryHex); - document.documentElement.style.setProperty('--color-primary-content', primaryContentHex); - document.documentElement.style.setProperty('--p', primaryHSL); - document.documentElement.style.setProperty('--pf', primaryHSL); - document.documentElement.style.setProperty('--pc', primaryContentHSL); - document.documentElement.style.setProperty('--color-base-100', '#ffffff'); - document.documentElement.style.setProperty('--color-base-200', '#f5f7fa'); - document.documentElement.style.setProperty('--color-base-300', '#e5e7eb'); - document.documentElement.style.setProperty('--color-base-content', '#1f2937'); - document.documentElement.style.setProperty('--outer-bg', lightBgHex); - document.documentElement.style.setProperty('--root-bg', lightBgHex); + root.style.setProperty('--color-primary-content', primaryContentHex); + root.style.setProperty('--p', primaryHSL); + root.style.setProperty('--pf', primaryHSL); + root.style.setProperty('--pc', primaryContentHSL); + root.style.setProperty('--color-base-100', '#ffffff'); + root.style.setProperty('--color-base-200', '#f5f7fa'); + root.style.setProperty('--color-base-300', '#e5e7eb'); + root.style.setProperty('--color-base-content', '#1f2937'); + root.style.setProperty('--outer-bg', lightBgHex); + root.style.setProperty('--root-bg', lightBgHex); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(unauthenticated)/products/[id]/confirm/+page.svelte around lines 158 - 174, Remove the outdated "This is Svelte 5..." comment and consolidate use of the root variable inside the $effect callback: keep the existing typeof document !== 'undefined' guard, assign const root = document.documentElement, then replace all occurrences of document.documentElement.style.setProperty(...) with root.style.setProperty(...); ensure all style.setProperty calls that currently use document.documentElement are changed (references: $effect, const root, primaryHex, primaryContentHex, primaryHSL, primaryContentHSL, lightBgHex).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/server/job-executors/databaseCleanup.ts`:
- Around line 11-17: The DatabaseWrites.productUserChanges.deleteMany call is
not awaited, which lets the job finish early and swallows DB errors; update the
cleanup task to await DatabaseWrites.productUserChanges.deleteMany(...) (same
pattern as await DatabaseWrites.productBuilds.deleteMany used elsewhere) so the
promise is resolved before the job completes and any errors are
propagated/handled by the surrounding try/catch or job runner.
In `@src/routes/`(unauthenticated)/products/[id]/confirm/+page.svelte:
- Around line 265-272: The email and code inputs lack accessible labels—add
explicit <label> elements tied to each input via id/for (e.g., give the email
input id="email" and add a visible or screen-reader-only label text like "Email
address"; similarly give the code input id="code" and add a label "Verification
code" or an sr-only label) so assistive tech can announce them; keep existing
placeholder and required attributes, and use a utility class like "sr-only" for
visually hidden labels if you don't want them visible.
- Around line 339-346: The enhance callback handleVerifyCode currently detects
result.type === 'redirect' but doesn't apply it, leaving the user on the
'verified' step; update handleVerifyCode to call applyAction(result) when
result.type === 'redirect' (so the browser follows the server redirect to
/dashboard), ensuring you import or access applyAction from SvelteKit's enhance
context; alternatively remove the explicit redirect branch to let the default
enhance behavior handle redirects.
---
Duplicate comments:
In `@src/routes/`(unauthenticated)/products/[id]/confirm/+page.svelte:
- Around line 206-209: The client is reading result.data.message but the server
returns fail(400, { error: '...' }), so update the failure branch (the block
checking result.type === 'failure' in the confirm handler) to extract the error
from result.data.error (and fall back to result.data.message or a generic
string) and assign that to the local error variable; adjust the type assertion
used on result.data accordingly so both { error?: string } and { message?:
string } are supported.
- Around line 246-254: The h1 currently renders hardcoded English strings based
on the step variable; replace those literals inside the conditional (the block
that checks step === 'verified' / 'code' / default) with the udm_* translation
keys (e.g., udm_verified, udm_check_your_email, udm_enter_your_email) using the
project's translation helper (the same pattern used elsewhere in this template,
e.g., the $t or t function/ store). Update the conditional branches in the h1 to
call the translation function with the appropriate udm_* keys so the headings
are localized.
---
Nitpick comments:
In `@src/routes/`(unauthenticated)/products/[id]/confirm/+page.svelte:
- Around line 299-307: The verification code input (the <input> bound to the
variable `code` in the confirm page) is missing an inputmode, so mobile devices
may not surface a numeric keyboard; add inputmode="numeric" to the <input
name="code" bind:value={code} ...> element to ensure a numeric keyboard appears
for the 6-digit one-time code and improve UX.
- Around line 190-193: The handleSendCode branch that handles result.type ===
'failure' should mirror the other handler by reading both possible error fields;
update the failure case in handleSendCode to extract (result.data as { error?:
string; message?: string } | undefined) and set the component error variable to
error || message || a default like 'Failed to send code. Please try again.' so
both 'error' and 'message' responses from sendCode are respected.
- Around line 158-174: Remove the outdated "This is Svelte 5..." comment and
consolidate use of the root variable inside the $effect callback: keep the
existing typeof document !== 'undefined' guard, assign const root =
document.documentElement, then replace all occurrences of
document.documentElement.style.setProperty(...) with
root.style.setProperty(...); ensure all style.setProperty calls that currently
use document.documentElement are changed (references: $effect, const root,
primaryHex, primaryContentHex, primaryHSL, primaryContentHSL, lightBgHex).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ac9900f-285c-4eab-9175-00d2db857935
📒 Files selected for processing (6)
src/lib/bullmq.tssrc/lib/server/bullmq/BullWorker.tssrc/lib/server/bullmq/types.tssrc/lib/server/job-executors/databaseCleanup.tssrc/lib/server/job-executors/index.tssrc/routes/(unauthenticated)/products/[id]/confirm/+page.svelte
…se of @html Svelte tag for better security.
…new more localized scope in routes
Author
|
@James-Voight When you get the chance please review the changes and if you could take a look at the stuff I tagged you on |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
src/routes/(unauthenticated)/products/[id]/user-data/+page.svelte (1)
50-50:⚠️ Potential issue | 🟠 MajorDeletion scope is not captured in the request payload.
Line 50 posts only
token, andproductId, while Lines 193–212 render scope radios without bound values. The API cannot tell whether the user selected data-only deletion or full account deletion.🐛 Proposed fix
<script lang="ts"> @@ let { data }: Props = $props(); let turnstileToken: string | null = null; + let deletionType = $state<'data' | 'account'>('data'); @@ - body: JSON.stringify({ email, token: turnstileToken, productId: app.id }) + body: JSON.stringify({ email, token: turnstileToken, productId: app.id, deletionType }) @@ <input type="radio" name="deletionType" + value="data" + bind:group={deletionType} class="radio radio-primary radio-sm mt-1" - checked /> @@ - <input type="radio" name="deletionType" class="radio radio-primary radio-sm mt-1" /> + <input + type="radio" + name="deletionType" + value="account" + bind:group={deletionType} + class="radio radio-primary radio-sm mt-1" + />Also applies to: 193-212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(unauthenticated)/products/[id]/user-data/+page.svelte at line 50, The POST payload currently only includes email, token, and productId but omits the deletion scope selected by the user; bind the radio group rendered in the scope inputs (the Svelte radios in the component) to a component variable (e.g., scope or deletionScope) and include that variable in the request body (e.g., JSON.stringify({ email, token: turnstileToken, productId: app.id, scope: deletionScope })). Also ensure the radios have a default value and the bound variable is validated before sending.src/routes/(unauthenticated)/products/[id]/confirm/+page.server.ts (2)
175-178:⚠️ Potential issue | 🟠 MajorDo not log live OTP + full email, even in development logs.
This still emits active credentials and PII for an unauthenticated security flow.
🧹 Safer logging
- if (process.env.NODE_ENV === 'development') { - console.log( - `[UDM TEST] Verification code for ${normalizedEmail} (product ${params.id}): ${code}` - ); - } + if (process.env.NODE_ENV === 'development') { + console.log(`[UDM TEST] sent_verification_code product=${params.id}`); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(unauthenticated)/products/[id]/confirm/+page.server.ts around lines 175 - 178, The current development-only console.log prints full OTP and full email (normalizedEmail and code) which exposes PII/credentials; replace this with a safe/de‑identified log inside the same NODE_ENV check by removing any plaintext OTP/email and instead log non-sensitive info such as params.id, a redactedEmail (e.g., show only first char and domain or a hash of normalizedEmail) and a masked code indicator or its length (do not include the actual code); update the console.log in the block that references normalizedEmail, params.id and code accordingly so development logs never contain the raw OTP or full email.
87-94:⚠️ Potential issue | 🟠 MajorAvoid exposing internal org/project names on a public UDM route.
Lines 87–94 fall back to internal
Organization.Name/Project.Namewhen manifest data is unavailable. This leaks internal naming to unauthenticated users.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(unauthenticated)/products/[id]/confirm/+page.server.ts around lines 87 - 94, The code is exposing internal org/project names via product.Project.Organization?.Name and product.Project.Name in the developer and app name fallbacks; change these fallbacks to use public manifest fields (e.g., product.Manifest?.developerDisplayName and product.Manifest?.appDisplayName or a dedicated public name field such as product.PublicName) and only use non-sensitive defaults like 'Unknown developer' and 'App' when those public fields are missing; update the developer variable and the AppInfo.name assignment (the developer variable and the app constant) to stop referencing product.Project or Organization names directly.
🧹 Nitpick comments (2)
src/routes/(unauthenticated)/products/[id]/user-data/+page.svelte (1)
82-86: Clean up the global Turnstile callback on unmount.
window.handleTurnstileSuccessis assigned on mount but never cleared. Add teardown to avoid stale global handlers after route transitions.♻️ Proposed cleanup
- import { onMount } from 'svelte'; + import { onDestroy, onMount } from 'svelte'; @@ onMount(() => { @@ window.handleTurnstileSuccess = (token: string) => { turnstileToken = token; }; }); + + onDestroy(() => { + if (window.handleTurnstileSuccess) { + window.handleTurnstileSuccess = undefined; + } + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(unauthenticated)/products/[id]/user-data/+page.svelte around lines 82 - 86, The global Turnstile callback window.handleTurnstileSuccess is set during component mount but not cleaned up; update the Svelte component that sets window.handleTurnstileSuccess (the onMount block around the turnstile setup) to also remove it on unmount by adding an onDestroy teardown that deletes or sets window.handleTurnstileSuccess = undefined (or delete window.handleTurnstileSuccess) so the global handler is not left stale after route transitions.src/routes/(unauthenticated)/products/[id]/user-data/+layout.server.ts (1)
87-126: Extract shared manifest helpers to one module.
resolveIconUrlandgetLatestBuiltFileare duplicated across this file andsrc/routes/(unauthenticated)/products/[id]/confirm/+page.server.ts. Centralizing will prevent behavior drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(unauthenticated)/products/[id]/user-data/+layout.server.ts around lines 87 - 126, Extract resolveIconUrl and getLatestBuiltFile into a shared helper module and import them where needed; move the implementations as-is into a new exported module (e.g., manifestHelpers) and replace the local functions in this layout server file and the other server file with imports of resolveIconUrl and getLatestBuiltFile, ensuring the exported getLatestBuiltFile keeps the same signature (productId: string, artifactType: string) and that resolveIconUrl accepts (icon: string | null | undefined, baseUrl: URL, artifactUrl: URL) so existing callers need no changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/locales/fr-FR.json`:
- Line 632: The FR locale key "downloads_title" in src/lib/locales/fr-FR.json is
still English ("Downloads"); update its value to the appropriate French
translation (e.g., "Téléchargements") by replacing the string for the
"downloads_title" key so the FR locale is fully localized.
In `@src/routes/`(unauthenticated)/products/[id]/confirm/+page.server.ts:
- Around line 140-166: The updateMany and create calls on
DatabaseWrites.productUserChanges must be made atomic to prevent race
conditions; wrap the update and subsequent create inside a single transaction
(e.g., use DatabaseWrites.$transaction or your ORM's transaction API) so that
the existing codes are expired and the new ConfirmationCode is inserted as one
atomic operation; ensure you use params.id, normalizedEmail, code, and expiresAt
inside that transaction and propagate/throw any transaction errors so callers
know the operation failed.
- Line 135: The OTP generation uses crypto.randomInt with an exclusive upper
bound so randomInt(100000, 999999) never yields 999999; update the call that
assigns code (the variable named code) to use an exclusive max one higher (e.g.
randomInt(100000, 1000000)) so the full 6-digit range 100000–999999 can be
produced.
In `@src/routes/`(unauthenticated)/products/[id]/user-data/+layout.server.ts:
- Around line 67-71: The code constructs fileUrl via new URL(path, baseUrl) and
then fetches it, which allows an absolute path in path to point to an external
origin; update the logic in the block around findFilePath, fileUrl and fetch so
that you reject or sanitize absolute URLs: either ensure path is a relative
pathname (e.g., reject inputs starting with "http://" or "//" or starting with
'/') before calling new URL, or after constructing fileUrl verify fileUrl.origin
=== new URL(baseUrl).origin and only call fetch when the origins match;
reference findFilePath, fileUrl, baseUrl and the fetch call when applying the
origin check/sanitization.
---
Duplicate comments:
In `@src/routes/`(unauthenticated)/products/[id]/confirm/+page.server.ts:
- Around line 175-178: The current development-only console.log prints full OTP
and full email (normalizedEmail and code) which exposes PII/credentials; replace
this with a safe/de‑identified log inside the same NODE_ENV check by removing
any plaintext OTP/email and instead log non-sensitive info such as params.id, a
redactedEmail (e.g., show only first char and domain or a hash of
normalizedEmail) and a masked code indicator or its length (do not include the
actual code); update the console.log in the block that references
normalizedEmail, params.id and code accordingly so development logs never
contain the raw OTP or full email.
- Around line 87-94: The code is exposing internal org/project names via
product.Project.Organization?.Name and product.Project.Name in the developer and
app name fallbacks; change these fallbacks to use public manifest fields (e.g.,
product.Manifest?.developerDisplayName and product.Manifest?.appDisplayName or a
dedicated public name field such as product.PublicName) and only use
non-sensitive defaults like 'Unknown developer' and 'App' when those public
fields are missing; update the developer variable and the AppInfo.name
assignment (the developer variable and the app constant) to stop referencing
product.Project or Organization names directly.
In `@src/routes/`(unauthenticated)/products/[id]/user-data/+page.svelte:
- Line 50: The POST payload currently only includes email, token, and productId
but omits the deletion scope selected by the user; bind the radio group rendered
in the scope inputs (the Svelte radios in the component) to a component variable
(e.g., scope or deletionScope) and include that variable in the request body
(e.g., JSON.stringify({ email, token: turnstileToken, productId: app.id, scope:
deletionScope })). Also ensure the radios have a default value and the bound
variable is validated before sending.
---
Nitpick comments:
In `@src/routes/`(unauthenticated)/products/[id]/user-data/+layout.server.ts:
- Around line 87-126: Extract resolveIconUrl and getLatestBuiltFile into a
shared helper module and import them where needed; move the implementations
as-is into a new exported module (e.g., manifestHelpers) and replace the local
functions in this layout server file and the other server file with imports of
resolveIconUrl and getLatestBuiltFile, ensuring the exported getLatestBuiltFile
keeps the same signature (productId: string, artifactType: string) and that
resolveIconUrl accepts (icon: string | null | undefined, baseUrl: URL,
artifactUrl: URL) so existing callers need no changes.
In `@src/routes/`(unauthenticated)/products/[id]/user-data/+page.svelte:
- Around line 82-86: The global Turnstile callback window.handleTurnstileSuccess
is set during component mount but not cleaned up; update the Svelte component
that sets window.handleTurnstileSuccess (the onMount block around the turnstile
setup) to also remove it on unmount by adding an onDestroy teardown that deletes
or sets window.handleTurnstileSuccess = undefined (or delete
window.handleTurnstileSuccess) so the global handler is not left stale after
route transitions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 447812ba-8f26-463c-9925-e96d5777b1e4
⛔ Files ignored due to path filters (1)
static/placeholder-icon.svgis excluded by!**/*.svg
📒 Files selected for processing (15)
src/app.csssrc/lib/locales/en-US.jsonsrc/lib/locales/es-419.jsonsrc/lib/locales/fr-FR.jsonsrc/lib/products/UDMtypes.tssrc/lib/server/job-executors/databaseCleanup.tssrc/lib/utils/theme.tssrc/routes/(unauthenticated)/+layout.sveltesrc/routes/(unauthenticated)/api/delete-request/+server.tssrc/routes/(unauthenticated)/products/[id]/+layout.server.tssrc/routes/(unauthenticated)/products/[id]/+layout.sveltesrc/routes/(unauthenticated)/products/[id]/confirm/+page.server.tssrc/routes/(unauthenticated)/products/[id]/confirm/+page.sveltesrc/routes/(unauthenticated)/products/[id]/user-data/+layout.server.tssrc/routes/(unauthenticated)/products/[id]/user-data/+page.svelte
✅ Files skipped from review due to trivial changes (1)
- src/app.css
🚧 Files skipped from review as they are similar to previous changes (4)
- src/lib/server/job-executors/databaseCleanup.ts
- src/routes/(unauthenticated)/api/delete-request/+server.ts
- src/routes/(unauthenticated)/+layout.svelte
- src/lib/locales/es-419.json
| "udm_about_title": "À propos de cette application", | ||
| "udm_show_more": "Afficher plus", | ||
| "udm_back_manage": "Retour à Gérer mes données", | ||
| "downloads_title": "Downloads", |
Contributor
There was a problem hiding this comment.
Localize downloads_title for French users.
Line 632 is still English ("Downloads"), which causes mixed-language UI in the FR locale.
🌐 Proposed fix
- "downloads_title": "Downloads",
+ "downloads_title": "Téléchargements",📝 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.
Suggested change
| "downloads_title": "Downloads", | |
| "downloads_title": "Téléchargements", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/locales/fr-FR.json` at line 632, The FR locale key "downloads_title"
in src/lib/locales/fr-FR.json is still English ("Downloads"); update its value
to the appropriate French translation (e.g., "Téléchargements") by replacing the
string for the "downloads_title" key so the FR locale is fully localized.
| const normalizedEmail = form.data.email.trim().toLowerCase(); | ||
|
|
||
| // 1. Generate a random 6-digit code | ||
| const code = randomInt(100000, 999999).toString(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js crypto.randomInt(min, max), is maxexclusive, and should a full 6-digit range includerandomInt(100000, 1000000)?
💡 Result:
Yes, in Node.js crypto.randomInt(min, max), the max parameter is exclusive (upper bound not included), and the function returns a random integer n such that min <= n < max. For a full 6-digit range (100000 to 999999 inclusive), use randomInt(100000, 1000000). This generates numbers from 100000 inclusive to 999999 inclusive (900000 possible values), as 1000000 is excluded. This is confirmed by the official Node.js source code and documentation translations, which explicitly state the range as [min, max) with min inclusive and max exclusive. Multiple secondary sources (Deno docs, Bun docs, GeeksforGeeks, Stack Overflow) consistently describe it this way.
Citations:
- 1: https://docs.deno.com/api/node/crypto/~/randomInt
- 2: https://www.geeksforgeeks.org/node-js/node-js-crypto-randomint-method/
- 3: https://bun.com/reference/node/crypto/randomInt
- 4: https://dev.to/kais_blog/how-to-generate-a-secure-random-number-in-node-js-16io
- 5: https://nodejs.cn/api/crypto/crypto_randomint_min_max_callback.html
- 6: https://github.com/nodejs/node/blob/main/lib/internal/crypto/random.js
Fix randomInt upper bound: max parameter is exclusive, not inclusive.
Line 135 uses randomInt(100000, 999999), but Node's crypto.randomInt(min, max) treats max as exclusive. This means 999999 is never generated, producing only values up to 999998.
✅ Fix
- const code = randomInt(100000, 999999).toString();
+ const code = randomInt(100000, 1_000_000).toString();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/`(unauthenticated)/products/[id]/confirm/+page.server.ts at line
135, The OTP generation uses crypto.randomInt with an exclusive upper bound so
randomInt(100000, 999999) never yields 999999; update the call that assigns code
(the variable named code) to use an exclusive max one higher (e.g.
randomInt(100000, 1000000)) so the full 6-digit range 100000–999999 can be
produced.
Comment on lines
+140
to
+166
| await DatabaseWrites.productUserChanges.updateMany({ | ||
| where: { | ||
| Email: normalizedEmail, | ||
| ProductId: params.id, | ||
| DateConfirmed: null, | ||
| DateExpires: { | ||
| gt: new Date() | ||
| } | ||
| }, | ||
| data: { | ||
| DateUpdated: new Date(), | ||
| DateExpires: new Date() // expire existing codes immediately | ||
| } | ||
| }); | ||
|
|
||
| await DatabaseWrites.productUserChanges.create({ | ||
| data: { | ||
| ProductId: params.id, | ||
| Email: normalizedEmail, | ||
| Change: 'User data deletion request verification', | ||
| DateCreated: new Date(), | ||
| DateUpdated: new Date(), | ||
| ConfirmationCode: code, | ||
| DateExpires: expiresAt, | ||
| DateConfirmed: null | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
Make code invalidation and code creation atomic.
Lines 140–166 perform updateMany and create separately. Concurrent requests can interleave and leave more than one valid unconfirmed code for the same email/product.
🔒 Transactional fix
- await DatabaseWrites.productUserChanges.updateMany({
- where: {
- Email: normalizedEmail,
- ProductId: params.id,
- DateConfirmed: null,
- DateExpires: {
- gt: new Date()
- }
- },
- data: {
- DateUpdated: new Date(),
- DateExpires: new Date() // expire existing codes immediately
- }
- });
-
- await DatabaseWrites.productUserChanges.create({
- data: {
- ProductId: params.id,
- Email: normalizedEmail,
- Change: 'User data deletion request verification',
- DateCreated: new Date(),
- DateUpdated: new Date(),
- ConfirmationCode: code,
- DateExpires: expiresAt,
- DateConfirmed: null
- }
- });
+ await DatabaseWrites.$transaction(async (tx) => {
+ const now = new Date();
+ await tx.productUserChanges.updateMany({
+ where: {
+ Email: normalizedEmail,
+ ProductId: params.id,
+ DateConfirmed: null,
+ DateExpires: { gt: now }
+ },
+ data: {
+ DateUpdated: now,
+ DateExpires: now
+ }
+ });
+
+ await tx.productUserChanges.create({
+ data: {
+ ProductId: params.id,
+ Email: normalizedEmail,
+ Change: 'User data deletion request verification',
+ DateCreated: now,
+ DateUpdated: now,
+ ConfirmationCode: code,
+ DateExpires: expiresAt,
+ DateConfirmed: null
+ }
+ });
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/`(unauthenticated)/products/[id]/confirm/+page.server.ts around
lines 140 - 166, The updateMany and create calls on
DatabaseWrites.productUserChanges must be made atomic to prevent race
conditions; wrap the update and subsequent create inside a single transaction
(e.g., use DatabaseWrites.$transaction or your ORM's transaction API) so that
the existing codes are expired and the new ConfirmationCode is inserted as one
atomic operation; ensure you use params.id, normalizedEmail, code, and expiresAt
inside that transaction and propagate/throw any transaction errors so callers
know the operation failed.
Comment on lines
+67
to
+71
| const path = findFilePath(files, language, filename) ?? `${language}/${filename}`; | ||
| const fileUrl = new URL(path, baseUrl); | ||
| try { | ||
| const res = await fetch(fileUrl); | ||
| if (!res.ok) return ''; |
Contributor
There was a problem hiding this comment.
Pin listing file fetches to the artifact origin.
new URL(path, baseUrl) accepts absolute URLs. A malicious/tainted manifest file entry could force server-side fetches to external hosts.
🛡️ Origin guard
async function fetchTextFile(baseUrl: URL, files: string[], language: string, filename: string) {
const path = findFilePath(files, language, filename) ?? `${language}/${filename}`;
const fileUrl = new URL(path, baseUrl);
+ if (fileUrl.host !== baseUrl.host || fileUrl.protocol !== baseUrl.protocol) return '';
try {
const res = await fetch(fileUrl);
if (!res.ok) return '';
return (await res.text()).trim();
} catch {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/`(unauthenticated)/products/[id]/user-data/+layout.server.ts
around lines 67 - 71, The code constructs fileUrl via new URL(path, baseUrl) and
then fetches it, which allows an absolute path in path to point to an external
origin; update the logic in the block around findFilePath, fileUrl and fetch so
that you reject or sanitize absolute URLs: either ensure path is a relative
pathname (e.g., reject inputs starting with "http://" or "//" or starting with
'/') before calling new URL, or after constructing fileUrl verify fileUrl.origin
=== new URL(baseUrl).origin and only call fetch when the origins match;
reference findFilePath, fileUrl, baseUrl and the fetch call when applying the
origin check/sanitization.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/routes/`(unauthenticated)/api/delete-request/+server.ts:
- Around line 32-37: Wrap the Turnstile fetch/parse in a try/catch and add a
timeout via AbortController to prevent hanging; specifically, when calling
fetch('https://challenges.cloudflare.com/turnstile/v0/siteverify', { method:
'POST', body: formData }) create an AbortController, use setTimeout to abort
after a short configurable period (e.g. a few seconds), await the fetch into the
verification variable inside the try block, check verification.ok before calling
await verification.json() into result, and on any error (fetch aborted, non-OK
response, or JSON parse failure) catch it and return a controlled 502/503
response instead of letting the error bubble.
- Around line 58-76: productUserChanges.create() always inserts a new
unconfirmed row which allows multiple pending codes and can leave a code
persisted if sendEmail() fails; change the flow in the handler that calls
productUserChanges.create and sendEmail so you first try to find an existing
pending record for the same Email and ProductId where DateConfirmed is null (or
enforce a unique pending constraint in schema.prisma for (Email, ProductId,
DateConfirmed=null)), then either update that row with a new
ConfirmationCode/DateExpires or perform an upsert instead of blind create, and
ensure sendEmail is executed after a successful DB upsert and if sendEmail fails
you delete or revert the pending row (or mark it invalid) so orphaned codes are
not left behind; update related logic in
src/routes/(unauthenticated)/products/[id]/confirm/+page.server.ts which expects
the newest unconfirmed record if you change uniqueness semantics.
- Around line 12-18: Wrap the call to request.json() in a try/catch and avoid
destructuring immediately; first parse into a rawBody, verify it's an object,
then validate and normalize fields (ensure email, token, productId are strings,
trim and lowercase email into normalizedEmail, reject whitespace-only values),
validate email format (simple regex) and token/productId format (UUID or
expected pattern) and only then destructure/use deletionType and other values;
on any validation failure return json({ success: false }, { status: 400 }) to
avoid passing malformed values to Prisma/mailer.
- Line 49: The one-line generation of the 6-digit code using randomInt is using
an exclusive upper bound too low; update the call that produces the code (the
const code assignment where randomInt is used) to use randomInt(100000, 1000000)
so the full range 100000–999999 is possible and then continue converting to
string as before.
In `@src/routes/`(unauthenticated)/products/[id]/user-data/+page.svelte:
- Around line 67-69: The code currently appends raw PII (email) to the
confirmation URL using confirmUrl.searchParams and then navigates with
window.location.assign, which exposes the email in history/logs; instead
generate or request a short-lived opaque request id (e.g., ask the backend for a
confirmationToken) and set that token as the only query param on confirmUrl, or
store the email in client-side sessionStorage and navigate without any email
param; update the usage of confirmUrl.searchParams.set('email', ...) to use the
token key (e.g., 'token') or remove setting and save the email under
sessionStorage before calling window.location.assign(confirmUrl.toString()) so
no raw email appears in the URL.
- Around line 58-64: The code parses JSON before checking res.ok which throws on
non-JSON responses; change the logic in the response handling around the fetch
so you first check res.ok (or wrap res.json() in try/catch), and if !res.ok
handle it by resetting the Turnstile token and UI (call
window.turnstile?.reset?.() and set turnstileToken = null) and calling
m.udm_alert_verification_failed(); when parsing the body fail, fall back to
res.text() (or treat as failure) to avoid unhandled exceptions and ensure the
token is always reset in the error path (update the block that currently calls
res.json() and then checks res.ok).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d480109-f7af-463a-a38f-1eef8c750847
📒 Files selected for processing (2)
src/routes/(unauthenticated)/api/delete-request/+server.tssrc/routes/(unauthenticated)/products/[id]/user-data/+page.svelte
Comment on lines
+12
to
+18
| const { email, token, productId, deletionType } = await request.json(); | ||
|
|
||
| if (!email || !token || !productId) return json({ success: false }, { status: 400 }); | ||
| if (deletionType !== 'data' && deletionType !== 'account') { | ||
| return json({ success: false }, { status: 400 }); | ||
| } | ||
| const normalizedEmail = String(email).trim().toLowerCase(); |
Contributor
There was a problem hiding this comment.
Validate the parsed body before destructuring it.
request.json() can throw on malformed input, and the current truthiness checks happen before trimming or type checks. Whitespace-only strings and non-string JSON can still fall through here and surface later as 500s in Prisma or the mailer. Parse defensively and validate normalized email, token, and productId values server-side, including email/UUID format.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/`(unauthenticated)/api/delete-request/+server.ts around lines 12 -
18, Wrap the call to request.json() in a try/catch and avoid destructuring
immediately; first parse into a rawBody, verify it's an object, then validate
and normalize fields (ensure email, token, productId are strings, trim and
lowercase email into normalizedEmail, reject whitespace-only values), validate
email format (simple regex) and token/productId format (UUID or expected
pattern) and only then destructure/use deletionType and other values; on any
validation failure return json({ success: false }, { status: 400 }) to avoid
passing malformed values to Prisma/mailer.
Comment on lines
+32
to
+37
| const verification = await fetch('https://challenges.cloudflare.com/turnstile/v0/siteverify', { | ||
| method: 'POST', | ||
| body: formData | ||
| }); | ||
|
|
||
| const result = await verification.json(); |
Contributor
There was a problem hiding this comment.
Time-box and guard the Turnstile verification request.
This public handler makes an external network call with no timeout or error handling. A slow/failed Cloudflare response or a non-JSON upstream error currently bubbles out as an uncaught 500 on the request path. Please wrap it in a timeout + try/catch and return a controlled 502/503.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/`(unauthenticated)/api/delete-request/+server.ts around lines 32 -
37, Wrap the Turnstile fetch/parse in a try/catch and add a timeout via
AbortController to prevent hanging; specifically, when calling
fetch('https://challenges.cloudflare.com/turnstile/v0/siteverify', { method:
'POST', body: formData }) create an AbortController, use setTimeout to abort
after a short configurable period (e.g. a few seconds), await the fetch into the
verification variable inside the try block, check verification.ok before calling
await verification.json() into result, and on any error (fetch aborted, non-OK
response, or JSON parse failure) catch it and return a controlled 502/503
response instead of letting the error bubble.
| } | ||
|
|
||
| // Generate a verification code and store it with an expiration time. This expiration time is 10 minutes, and is automatically reduced by a minute if the user types the wrong code to prevent brute-force attempts. | ||
| const code = randomInt(100000, 999999).toString(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js, does crypto.randomInt(min, max)includemax, or is the upper bound exclusive?
💡 Result:
In Node.js, crypto.randomInt(min, max) generates a random integer n such that min <= n < max. The upper bound max is exclusive; it does not include max.
Citations:
- 1: https://github.com/nodejs/node/blob/main/lib/internal/crypto/random.js
- 2: nodejs/node@44d89a9faa
- 3: https://stackoverflow.com/questions/33609404/node-js-how-to-generate-random-numbers-in-specific-range-using-crypto-randomby
- 4: https://docs.deno.com/api/node/crypto/~/randomInt
- 5: https://www.geeksforgeeks.org/node-js/node-js-crypto-randomint-method/
- 6: https://bun.com/reference/node/crypto/randomInt
- 7: https://nodejs.cn/api/crypto/crypto_randomint_min_max_callback.html
🏁 Script executed:
cat -n src/routes/\(unauthenticated\)/api/delete-request/+server.ts | sed -n '45,55p'Repository: sillsdev/appbuilder-portal
Length of output: 697
Use 1000000 as the exclusive upper bound.
crypto.randomInt(100000, 999999) has an exclusive upper bound, so the highest value it can return is 999998, not 999999. To include 999999 in the range, use randomInt(100000, 1000000).
Suggested fix
- const code = randomInt(100000, 999999).toString();
+ const code = randomInt(100000, 1000000).toString();📝 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.
Suggested change
| const code = randomInt(100000, 999999).toString(); | |
| const code = randomInt(100000, 1000000).toString(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/`(unauthenticated)/api/delete-request/+server.ts at line 49, The
one-line generation of the 6-digit code using randomInt is using an exclusive
upper bound too low; update the call that produces the code (the const code
assignment where randomInt is used) to use randomInt(100000, 1000000) so the
full range 100000–999999 is possible and then continue converting to string as
before.
Comment on lines
+58
to
+76
| await DatabaseWrites.productUserChanges.create({ | ||
| data: { | ||
| ProductId: productId, | ||
| Email: normalizedEmail, | ||
| Change: change, | ||
| ConfirmationCode: code, | ||
| DateExpires: expiresAt, | ||
| DateConfirmed: null | ||
| } | ||
| }); | ||
|
|
||
| await sendEmail( | ||
| [{ email: normalizedEmail, name: normalizedEmail }], | ||
| 'Your verification code', | ||
| addProperties(EmailLayoutTemplate, { | ||
| INSERT_SUBJECT: `Your code is: ${code}`, | ||
| INSERT_CONTENT: `Your code is: ${code}` | ||
| }) | ||
| ); |
Contributor
There was a problem hiding this comment.
Avoid creating a second live code on every retry.
productUserChanges.create() always inserts a fresh unconfirmed row. src/lib/prisma/schema.prisma does not make (Email, ProductId, DateConfirmed) unique, and src/routes/(unauthenticated)/products/[id]/confirm/+page.server.ts only reads the newest unconfirmed record, so double-clicks/retries silently invalidate older emailed codes while stale pending rows pile up. If sendEmail() fails after the insert, you also persist a code the user never received. Reuse/update the existing pending request (or enforce a single pending row) and invalidate it on delivery failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/`(unauthenticated)/api/delete-request/+server.ts around lines 58 -
76, productUserChanges.create() always inserts a new unconfirmed row which
allows multiple pending codes and can leave a code persisted if sendEmail()
fails; change the flow in the handler that calls productUserChanges.create and
sendEmail so you first try to find an existing pending record for the same Email
and ProductId where DateConfirmed is null (or enforce a unique pending
constraint in schema.prisma for (Email, ProductId, DateConfirmed=null)), then
either update that row with a new ConfirmationCode/DateExpires or perform an
upsert instead of blind create, and ensure sendEmail is executed after a
successful DB upsert and if sendEmail fails you delete or revert the pending row
(or mark it invalid) so orphaned codes are not left behind; update related logic
in src/routes/(unauthenticated)/products/[id]/confirm/+page.server.ts which
expects the newest unconfirmed record if you change uniqueness semantics.
Comment on lines
+58
to
+64
| const data = await res.json(); | ||
|
|
||
| if (!res.ok || !data.success) { | ||
| alert(m.udm_alert_verification_failed()); | ||
| window.turnstile?.reset?.(); | ||
| turnstileToken = null; | ||
| return; |
Contributor
There was a problem hiding this comment.
Handle non-JSON API failures.
Line 58 parses JSON before checking res.ok. If the endpoint throws and SvelteKit returns HTML/text, this rejects, the user gets no alert, and the token is never reset after a server-side failure.
Suggested fix
- const data = await res.json();
-
- if (!res.ok || !data.success) {
+ let payload: { success?: boolean } | null = null;
+ try {
+ payload = await res.json();
+ } catch {
+ payload = null;
+ }
+
+ if (!res.ok || !payload?.success) {
alert(m.udm_alert_verification_failed());
window.turnstile?.reset?.();
turnstileToken = null;
return;
}📝 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.
Suggested change
| const data = await res.json(); | |
| if (!res.ok || !data.success) { | |
| alert(m.udm_alert_verification_failed()); | |
| window.turnstile?.reset?.(); | |
| turnstileToken = null; | |
| return; | |
| let payload: { success?: boolean } | null = null; | |
| try { | |
| payload = await res.json(); | |
| } catch { | |
| payload = null; | |
| } | |
| if (!res.ok || !payload?.success) { | |
| alert(m.udm_alert_verification_failed()); | |
| window.turnstile?.reset?.(); | |
| turnstileToken = null; | |
| return; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/`(unauthenticated)/products/[id]/user-data/+page.svelte around
lines 58 - 64, The code parses JSON before checking res.ok which throws on
non-JSON responses; change the logic in the response handling around the fetch
so you first check res.ok (or wrap res.json() in try/catch), and if !res.ok
handle it by resetting the Turnstile token and UI (call
window.turnstile?.reset?.() and set turnstileToken = null) and calling
m.udm_alert_verification_failed(); when parsing the body fail, fall back to
res.text() (or treat as failure) to avoid unhandled exceptions and ensure the
token is always reset in the error path (update the block that currently calls
res.json() and then checks res.ok).
Comment on lines
+67
to
+69
| const confirmUrl = new URL(`/products/${app.id}/confirm`, window.location.origin); | ||
| confirmUrl.searchParams.set('email', email); | ||
| window.location.assign(confirmUrl.toString()); |
Contributor
There was a problem hiding this comment.
Don’t put the email address in the confirmation URL.
Line 68 writes raw PII into the query string, which will end up in browser history, access logs, and potentially Referer headers. Prefer a short-lived opaque request id from the API or client-side state like sessionStorage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/`(unauthenticated)/products/[id]/user-data/+page.svelte around
lines 67 - 69, The code currently appends raw PII (email) to the confirmation
URL using confirmUrl.searchParams and then navigates with
window.location.assign, which exposes the email in history/logs; instead
generate or request a short-lived opaque request id (e.g., ask the backend for a
confirmationToken) and set that token as the only query param on confirmUrl, or
store the email in client-side sessionStorage and navigate without any email
param; update the usage of confirmUrl.searchParams.set('email', ...) to use the
token key (e.g., 'token') or remove setting and save the email under
sessionStorage before calling window.location.assign(confirmUrl.toString()) so
no raw email appears in the URL.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add pages for user data deletion requests, including sending an email to the user and verifying that it is a real email account. This is separate from the Cloudfare Turnstile integration that will complete this project.
#1251
@James-Voight
Summary by CodeRabbit
New Features
Infrastructure