Conversation
📝 WalkthroughWalkthroughReplaced hardcoded localhost endpoints with environment-driven base URLs ( Changes
Sequence Diagram(s)(none — changes are environment URL substitutions, an enum addition, and dependency bumps; no new multi-component control flow requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom Pre-merge Checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
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 |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/invoices/invoices.ts (1)
14-14: Fix typo in property name.The property is named
invocesbut should likely beinvoices. This will returnundefinedif the API returns a field namedinvoices.🔎 Proposed fix
- return respJson.invoces + return respJson.invoicesVerify the correct API response field name:
#!/bin/bash # Search for API response type definitions or other invoice references rg -n -C3 "invoices?:" --type ts ast-grep --pattern 'interface $_ { $$$ invoice$_: $_ $$$ }'
🤖 Fix all issues with AI agents
In @src/routes/invoices/invoices.ts:
- Line 8: listInvoice reads process.env.BILLING_URL directly, which is undefined
on the client; replace use of process.env.BILLING_URL with the SvelteKit public
env import (e.g., import { PUBLIC_BILLING_URL } from '$env/static/public' or
'$env/dynamic/public') and update listInvoice to build the URL from
PUBLIC_BILLING_URL; add a runtime validation that PUBLIC_BILLING_URL is defined
and throw or return a clear error when it's missing before calling fetch. Do the
same rename/replace and validation for AUTH_URL usages (rename to
PUBLIC_AUTH_URL and import/validate) in src/stores/auth.ts and the list
functions in src/routes/users/users.ts and src/routes/mfa/mfa.ts so all
client-side code uses PUBLIC_ prefixed envs and checks for presence.
In @src/routes/mfa/mfa.ts:
- Around line 17-18: The error messages are wrong copy-paste entries saying
"Refresh didn't work" in the MFA handlers; update the thrown Error in the
listMfa function (when resp.status !== 200) to a message like "listMfa failed"
(include status and/or response text for debugging), and similarly update the
thrown Error in createMfa to "createMfa failed" (also include status/response
details). Adjust both occurrences mentioned (the one around resp.status !== 200
for listMfa and the one around resp.status !== 200 for createMfa) so they
accurately describe the failing operation and include useful error context.
- Line 15: The code uses process.env.AUTH_URL to build the url constant in
src/routes/mfa/mfa.ts (const url = `${process.env.AUTH_URL}/mfa/${userId}`),
which is undefined on the client; rename the env variable to PUBLIC_AUTH_URL and
replace usage with an imported PUBLIC_AUTH_URL from SvelteKit’s $env (either
$env/static/public for build-time or $env/dynamic/public for runtime), update
the import and the url construction in both functions in mfa.ts and mirror the
same change in stores/auth.ts so all client-side references use PUBLIC_AUTH_URL
from the $env module.
In @src/routes/users/users.ts:
- Line 11: Replace direct use of process.env.AUTH_URL in listUser and
createNewUser with a validated getter: add a getAuthUrl() helper that reads
process.env.PUBLIC_AUTH_URL, throws if missing, and returns the value, then
change the url construction from `${process.env.AUTH_URL}/user` to
`${getAuthUrl()}/user`; ensure the env var uses SvelteKit's PUBLIC_ prefix
(PUBLIC_AUTH_URL) so client code can access it and apply the same pattern to
other modules mentioned (mfa.ts, auth.ts, invoices.ts).
In @src/stores/auth.ts:
- Around line 97-98: The error thrown in the logout flow is mislabeled: inside
the logout function where you check resp.status !== 200 (refer to the logout
function and the resp.status !== 200 check), replace the incorrect message
"Login didn't work" with a clear logout-specific message (e.g., "Logout failed"
or include response status/details) so the thrown Error accurately reflects the
failing operation.
- Line 38: The authentication functions in createCredentialStore (login,
chooseStrategy, loginCode, logout, refresh) and the handlers in users.ts and
mfa.ts use process.env.AUTH_URL directly and will build invalid URLs if AUTH_URL
is undefined; add a getAuthUrl() helper (e.g., in src/utils/env.ts) that throws
a clear Error if process.env.AUTH_URL is missing, then replace all direct
process.env.AUTH_URL references with getAuthUrl() calls (or import the helper)
so every URL is validated before use.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/routes/invoices/invoices.tssrc/routes/mfa/mfa.tssrc/routes/users/users.tssrc/stores/auth.ts
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
package.json (2)
20-49: Consider separating dependency updates from feature changes.The devDependency version bumps are unrelated to the PR's stated objective of replacing hardcoded URLs with environment variables. Mixing dependency maintenance with feature work can complicate rollbacks and make it harder to trace issues.
37-37: Consider addingglobals.audioWorkletto the ESLint config.The upgrade from
globalsv16.5.0 to v17.0.0 includes a breaking change: theaudioWorkletenvironment was split out of the browser environment. While your codebase currently does not use Web Audio API features, updating the ESLint config on line 22 to include...globals.audioWorkletaligns with the recommended migration path:globals: { ...globals.browser, ...globals.node, ...globals.audioWorklet }This is optional if you do not plan to use Web Audio API, but including it ensures the config remains consistent with the package's design and prevents potential linting issues if audioWorklet globals are used in the future.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
package.json
🔇 Additional comments (1)
package.json (1)
28-28: All updated package versions are valid and exist on npm.The specified versions for @sveltejs/vite-plugin-svelte, @testing-library/svelte, globals, jsdom, sass-embedded, svelte, svelte-check, and typescript-eslint have been confirmed to exist and are available on the npm registry. No security vulnerabilities have been introduced by these specific package updates.
Note: npm audit identified a pre-existing low-severity vulnerability in the
cookiedependency (via @sveltejs/kit), which is unrelated to the packages being updated in this PR.
remove hard coded url and insert env
Summary by CodeRabbit
Chores
Bug Fixes / Notes
✏️ Tip: You can customize this high-level summary in your review settings.