Initial app setup with databases, auth and testing#2
Open
nealhorner wants to merge 28 commits into
Open
Conversation
…to package.json and package-lock.json
…ng DATABASE_URL for Postgres connection
…onment variable management
…or Postgres and SQLite
…d SQLITE_DATABASE_URL for improved clarity and flexibility
📝 WalkthroughWalkthroughThis PR adds Better Auth integration, migrates application auth from a custom session model to Better Auth (schema, adapters, runtime), separates Postgres/SQLite env vars, implements desktop local/remote mode with Tauri-sidecar management, and introduces comprehensive Playwright E2E suites plus supporting dev scripts and docs. ChangesBetter Auth, Desktop Modes, DB split, and E2E
Sequence DiagramssequenceDiagram
participant Desktop UI
participant Tauri Runtime
participant Node Sidecar
participant Server API
participant Database
Desktop UI->>Tauri Runtime: start_local_api()
Tauri Runtime->>Tauri Runtime: Read DesktopConfig
alt mode != "local"
Tauri Runtime->>Desktop UI: error/deny
else mode == "local"
Tauri Runtime->>Node Sidecar: spawn process with PORT + SQLITE_DATABASE_URL
Node Sidecar->>Node Sidecar: wait for TCP ready
Tauri Runtime->>Tauri Runtime: cache base_url
Desktop UI->>Server API: GET /api/v1/me (same-origin)
Server API->>Database: resolve session/appUser via Better Auth
Server API->>Desktop UI: user + isTenantAdmin
end
sequenceDiagram
participant Web UI
participant Server API
participant Better Auth
participant Database
Web UI->>Server API: GET /api/v1/setup/status
Server API->>Database: check tenant.setupCompleteAt
Server API->>Web UI: { needsSetup, prefill, setupTokenConfigured }
Web UI->>Server API: POST /api/v1/setup/complete (authSettings, admin creds)
Server API->>Better Auth: auth.api.signUpEmail(admin)
Better Auth->>Database: create auth user
Server API->>Database: create tenant, AppUser, org/project/board/ticket
Server API->>Web UI: 201 { ok: true }
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (16)
apps/server/src/lib/setup-config.ts (1)
27-36: ⚡ Quick winConsider wrapping file parsing in try-catch for clearer error messages.
Both
JSON.parse()andinstanceConfigSchema.parse()can throw. If the config file is malformed or fails validation, the error won't indicate which file caused the issue. Adding a try-catch with a descriptive error message would help operators debug configuration problems.Suggested improvement
export function loadInstanceConfigFile(): InstanceConfigFile | null { const p = process.env.TRACKR_CONFIG_PATH; if (!p) return null; if (!existsSync(p)) { return null; } - const raw = readFileSync(p, "utf8"); - const j = JSON.parse(raw) as unknown; - return instanceConfigSchema.parse(j); + try { + const raw = readFileSync(p, "utf8"); + const j = JSON.parse(raw) as unknown; + return instanceConfigSchema.parse(j); + } catch (err) { + throw new Error(`Failed to load config from ${p}: ${err instanceof Error ? err.message : err}`); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/lib/setup-config.ts` around lines 27 - 36, Wrap the JSON parsing and schema validation inside loadInstanceConfigFile in a try-catch to surface a clear, contextual error that includes the TRACKR_CONFIG_PATH; specifically catch errors from JSON.parse(raw) and instanceConfigSchema.parse(j) and rethrow (or throw a new Error) with a message like "Failed to load/parse/validate config at <path>: <original error message>" so operators know which file failed (refer to process.env.TRACKR_CONFIG_PATH, JSON.parse, and instanceConfigSchema.parse to locate the code).apps/desktop/e2e/global-setup.ts (1)
19-20: ⚡ Quick winFragile CWD-based repo-root resolution
path.resolve(process.cwd(), "..", "..")relies onprocess.cwd()beingapps/desktopat the time Playwright's global setup runs. Turbo sets this correctly when invoking the workspace script, but runningnpx playwright testdirectly from the repo root (or any other directory) resolves two levels above the repo root, causingexecSyncto fail with a non-existent path.Use the setup file's own location instead:
♻️ Proposed fix
import { execSync } from "node:child_process"; import path from "node:path"; +import { fileURLToPath } from "node:url"; +const __filename = fileURLToPath(import.meta.url); +const __dirname = path.dirname(__filename); export default async function globalSetup(): Promise<void> { ... - const repoRoot = path.resolve(process.cwd(), "..", ".."); + // __dirname = apps/desktop/e2e → ../../.. = repo root + const repoRoot = path.resolve(__dirname, "../../.."); const prismaDir = path.join(repoRoot, "packages", "prisma");If the file is compiled to CJS (verify with the
db.tscheck above),__dirnameis already available and no shim is needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/e2e/global-setup.ts` around lines 19 - 20, The repoRoot calculation is fragile because it uses path.resolve(process.cwd(), "..", "..") which breaks when tests are run from other CWDs; update repoRoot to derive from the setup file location instead (use __dirname if the compiled bundle is CJS, otherwise compute from import.meta.url via fileURLToPath and path.dirname), and then set prismaDir = path.join(repoRoot, "packages", "prisma") as before; change the code around the repoRoot and prismaDir definitions (replace usage of process.cwd() and reference path.resolve(process.cwd(), "..", "..")) so the path is computed from the file's directory rather than the current working directory.apps/server/src/__tests__/auth-routes.test.ts (1)
59-87: 💤 Low valueConsider adding a test case for when the app user is not found.
The test covers the success path where
findFirstAppUserreturns a user withauthUserId. Consider adding coverage for the scenario where no matching app user exists (returnsnull), to verify the expected error response.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/__tests__/auth-routes.test.ts` around lines 59 - 87, Add a new unit test in apps/server/src/__tests__/auth-routes.test.ts that mocks findFirstAppUser to return null and issues a POST to the /api/v1/auth/dev-login route (same request shape as the existing test); assert the response status and body represent the expected error (e.g., 404 or 400 and an error message), and verify that signInEmail is not called. Use the same test harness/makeApp() setup, and reference findFirstAppUser and signInEmail in the assertions to ensure the negative path is covered.apps/server/e2e/global-setup.ts (1)
18-32: 💤 Low valueConsider adding error handling for the
execSynccall.If
prisma migrate resetfails (e.g., connection timeout, invalid credentials), the error propagates and halts the test run, which is acceptable. However, a clearer error message could help debugging CI failures.♻️ Optional: wrap with try-catch for clearer error context
+ try { execSync( "npx prisma migrate reset --force --skip-seed --config prisma.config.ts", { cwd: prismaDir, stdio: "inherit", env: { ...process.env, POSTGRES_DATABASE_URL: dbUrl, }, }, ); + } catch (err) { + console.error("[playwright global-setup] prisma migrate reset failed"); + throw err; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/e2e/global-setup.ts` around lines 18 - 32, Wrap the execSync call that runs "npx prisma migrate reset ..." in a try-catch inside apps/server/e2e/global-setup.ts so failures provide clearer context; catch the error around the execSync invocation (the block that uses prismaDir and env with POSTGRES_DATABASE_URL/dbUrl), log a descriptive message including prismaDir and the caught error (e.g., via console.error or existing logger), and then rethrow or call process.exit(1) to fail fast with the improved message.apps/desktop/src/routes/+layout.svelte (1)
45-48: 💤 Low valueConsider caching config to avoid redundant reads on every navigation.
afterNavigatecallsgetDesktopConfig()on every navigation, which could be inefficient if it reads from disk/storage each time. Consider only reloading when necessary (e.g., after settings changes) or caching the config.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/routes/`+layout.svelte around lines 45 - 48, afterNavigate currently calls getDesktopConfig() on every navigation causing redundant reads; change this by caching the result (e.g., store the config in the existing cfg variable or a Svelte writable store) and only call getDesktopConfig() if cfg is unset or explicitly invalidated. Update the afterNavigate handler to use the cached cfg, add a clear/invalidateConfig function or listen for settings-change events to force a reload, and keep guardRemoteSession() as-is but ensure it uses the cached cfg when needed so navigation no longer triggers disk/storage reads each time.apps/server/src/lib/initial-setup.ts (2)
81-85: 💤 Low valueMinor: batch column creation with
createMany.The sequential
prisma.boardColumn.createcalls in a loop could be replaced with a singleprisma.boardColumn.createManyfor slightly better performance during setup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/lib/initial-setup.ts` around lines 81 - 85, Replace the sequential prisma.boardColumn.create loop in initial-setup.ts with a single prisma.boardColumn.createMany call: map the existing columns array to include the boardId for each item (e.g., columns.map(c => ({ boardId: board.id, ...c }))), call await prisma.boardColumn.createMany({ data: mappedColumns }) and adjust any code that relied on created records (createMany returns a count, not created rows); keep using the same symbols (columns, board.id, prisma.boardColumn.create -> createMany) and await the promise for proper setup sequencing.
22-106: ⚖️ Poor tradeoffConsider wrapping seed operations in a transaction.
After the
appUseris created (line 59), failures in subsequent operations (org, project, board, columns, member, ticket) will leave the database in an inconsistent state. The tenant won't be rolled back, but the seeded defaults may be partially created.For first-time setup this is unlikely to cause user-facing issues since a retry would fail on duplicate email. However, wrapping lines 69-105 in a
prisma.$transaction()would ensure atomicity of the default data seeding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/lib/initial-setup.ts` around lines 22 - 106, The seed steps after creating appUser (admin) in runFirstTimeSetup are not atomic; wrap the subsequent org/project/board/columns/projectMember/ticket creation and the final tenant.update in a single Prisma transaction so partial seeds can't be left behind. Replace the multiple prisma.* calls (organization.create, project.create, board.create, boardColumn.create loop, projectMember.create, boardColumn.findFirstOrThrow, ticket.create, tenant.update) with a prisma.$transaction callback and use the transaction client (e.g., tx.organization.create, tx.project.create, etc.) inside it so all creations and the tenant update succeed or roll back together.apps/web/src/routes/setup/+page.svelte (1)
36-41: 💤 Low valueSimplify the auth settings prefill loop.
The current implementation uses type casting and non-null assertions. A cleaner approach would use
Object.assignwith validated values.💡 Simplified prefill
if (p?.authSettings) { - for (const k of Object.keys(auth) as (keyof AuthFlags)[]) { - if (typeof p.authSettings![k] === "boolean") - (auth as Record<string, boolean>)[k] = p.authSettings![k] as boolean; - } + Object.assign(auth, { + password: p.authSettings.password ?? auth.password, + google: p.authSettings.google ?? auth.google, + apple: p.authSettings.apple ?? auth.apple, + github: p.authSettings.github ?? auth.github, + okta: p.authSettings.okta ?? auth.okta, + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/setup/`+page.svelte around lines 36 - 41, The loop that copies p.authSettings into auth (checking p?.authSettings and iterating keys) can be simplified: validate p.authSettings exists, then build an object containing only keys whose values are boolean and merge it into auth via Object.assign. Update the code that references p.authSettings, auth, and AuthFlags to avoid non-null assertions and casts by filtering Object.entries(p.authSettings) for typeof value === "boolean" and then Object.assign(auth, filteredObject).apps/desktop/e2e/onboarding.spec.ts (1)
29-41: 💤 Low valueConsider extracting duplicate credentials to a constant.
The admin credentials are duplicated between
completeSetupIfNeeded(lines 34-35) and the test (lines 55-56). While acceptable for test readability, extracting to a constant would reduce drift risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/e2e/onboarding.spec.ts` around lines 29 - 41, Duplicate admin credentials are hard-coded in the test fetch body and in completeSetupIfNeeded; extract them into shared constants (e.g., ADMIN_EMAIL, ADMIN_PASSWORD, TENANT_NAME or a single SETUP_CREDENTIALS object) and reference those constants from both the POST payload in the fetch to /api/v1/setup/complete and from the completeSetupIfNeeded helper so both places use the same source of truth and avoid drift.apps/desktop/src/routes/+page.svelte (1)
35-52: 💤 Low valueConsider validating the remote URL before saving.
The
chooseRemotefunction trims the URL but doesn't validate its format before persisting. While<input type="url">provides browser-level validation, it's bypassed if called programmatically. Consider adding a basic URL validation check.💡 Optional validation
async function chooseRemote() { err = ""; busy = true; try { + const trimmed = remoteUrl.trim(); + try { + new URL(trimmed); + } catch { + err = "Invalid URL format"; + return; + } await setDesktopConfig({ version: 1, mode: "remote", - remoteBaseUrl: remoteUrl.trim(), + remoteBaseUrl: trimmed, localDisplayName: null, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/routes/`+page.svelte around lines 35 - 52, The chooseRemote function currently trims remoteUrl but does not validate it before calling setDesktopConfig; add a validation step using a robust check (e.g., attempt new URL(trimmed) or a strict regex) against the trimmed remoteUrl and if it fails set err to a helpful message and return early (do not call setDesktopConfig or goto), otherwise proceed to call setDesktopConfig with remoteBaseUrl set to the trimmed value; update references in this flow (chooseRemote, remoteUrl, setDesktopConfig, getDesktopConfig) accordingly so invalid URLs are rejected before persisting.apps/server/src/middleware/session.ts (1)
10-24: 💤 Low valueConsider adding error handling around the Better Auth API call.
If
auth.api.getSessionthrows an unexpected error (e.g., network issue to auth service, malformed headers), this middleware will propagate the exception and fail the request. For resilience, consider wrapping the call in a try-catch so that transient auth failures gracefully fall back to an unauthenticated state rather than causing 500 errors.♻️ Proposed defensive handling
export const loadSession = createMiddleware(async (c, next) => { - const s = await auth.api.getSession({ headers: c.req.raw.headers }); - if (!s?.user?.id) { + let s: Awaited<ReturnType<typeof auth.api.getSession>> | null = null; + try { + s = await auth.api.getSession({ headers: c.req.raw.headers }); + } catch { + // Treat auth failures as unauthenticated + } + if (!s?.user?.id) { c.set("authUser", null); return next(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/middleware/session.ts` around lines 10 - 24, Wrap the call to auth.api.getSession inside a try-catch in the loadSession middleware so transient or unexpected errors from auth.api.getSession do not bubble up; on catch, log the error (or use c.log/processLogger) and call c.set("authUser", null) then return next() to treat the request as unauthenticated; keep the existing logic for prisma.appUser.findFirst unchanged so only failures from auth.api.getSession are handled defensively.apps/desktop/src/lib/api.ts (1)
70-81: 💤 Low valueRedundant config fetch in
apiFetch.
apiFetchcallsgetDesktopConfig()on line 74, thenapiBaseUrl()on line 75, butapiBaseUrl()also callsgetDesktopConfig()internally. While caching prevents duplicate Tauri invocations after the first call, this adds unnecessary await overhead and is misleading.Consider refactoring to fetch config once and pass it to a variant of
apiBaseUrl.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/api.ts` around lines 70 - 81, apiFetch currently calls getDesktopConfig() and then apiBaseUrl(), but apiBaseUrl() calls getDesktopConfig() again causing redundant awaits; refactor by fetching the config once in apiFetch and pass it into a new or overloaded helper (e.g., add apiBaseUrlFromConfig(config: DesktopConfig) or change apiBaseUrl to accept an optional config parameter) so apiFetch uses the single config instance to compute base URL and set credentials; update apiFetch to call apiBaseUrlFromConfig(config) (or apiBaseUrl(config)) and remove the second getDesktopConfig() call, and adapt any callers of the modified apiBaseUrl signature accordingly.apps/web/src/routes/login/+page.svelte (2)
62-66: ⚡ Quick winError response parsing may lose structured error information.
The server may return a JSON error body (e.g.,
{ "error": "...", "message": "..." }), but the code reads the raw text. This could display raw JSON to users or miss the actual error message.♻️ Proposed improvement for error parsing
if (!res.ok) { - const t = (await res.text().catch(() => "")) || "Sign in failed"; - err = t; + try { + const body = await res.json(); + err = body.message ?? body.error ?? "Sign in failed"; + } catch { + err = "Sign in failed"; + } return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/login/`+page.svelte around lines 62 - 66, Replace the raw text-only error handling for the fetch response (the res / t / err block) with logic that first attempts to parse res.json() and extract a user-friendly message from common fields like message or error, falling back to res.text() and then a default "Sign in failed"; set err to that extracted string so structured JSON errors are shown instead of raw JSON. Ensure any JSON parse errors are caught and ignored so the fallback to text/default still occurs.
30-45: 💤 Low valueNo loading indicator during initial config fetch.
The
onMountfetch happens asynchronously while the form is already rendered with defaultauthvalues. This could cause a brief flash of the password form before OAuth buttons appear (or vice versa), and the redirect to/setuphappens after the page is visible.Consider adding a loading state to avoid layout shifts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/login/`+page.svelte around lines 30 - 45, The page renders with default auth values while onMount fetches /api/v1/public-config, causing a flash and delayed redirect; introduce a loading boolean (e.g., loading) initialized true, set it before/after the fetch inside onMount around the fetch("/api/v1/public-config") call, and only render the form/UI when loading is false (or render a spinner when true); if j.data.setupComplete is false call goto("/setup") before clearing loading (so redirect occurs immediately) and still update setupComplete, auth, and authAvailable after the fetch. Ensure you update references to setupComplete/auth/authAvailable rendering logic to wait for loading to be false.apps/desktop/src-tauri/src/lib.rs (2)
125-135: ⚡ Quick winSilent failure when stopping sidecar if lock is poisoned.
The
if let Ok(...)pattern silently ignores lock poisoning. If a previous thread panicked while holding the lock,stop_sidecarwon't kill the child process, potentially leaving orphaned processes.Consider using
.lock().unwrap_or_else(|e| e.into_inner())to recover from poisoned locks, or at least log a warning.♻️ Proposed fix to handle poisoned locks
fn stop_sidecar(state: &State<SidecarState>) { - if let Ok(mut c) = state.child.lock() { + let mut c = state.child.lock().unwrap_or_else(|e| e.into_inner()); - if let Some(ref mut ch) = *c { - let _ = ch.kill(); - } - *c = None; + if let Some(ref mut ch) = *c { + let _ = ch.kill(); } - if let Ok(mut u) = state.base_url.lock() { - *u = None; - } + *c = None; + let mut u = state.base_url.lock().unwrap_or_else(|e| e.into_inner()); + *u = None; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src-tauri/src/lib.rs` around lines 125 - 135, The stop_sidecar function currently ignores poisoned mutexes by using if let Ok(...) which can skip killing the child process; change the two lock acquisitions on state.child and state.base_url to recover from poisoned locks (e.g. use .lock().unwrap_or_else(|e| e.into_inner()) or equivalent) so you still obtain the mutex guard, kill the child (call ch.kill()) and clear the Option, and add a warning log when a PoisonError is encountered to aid debugging; reference the stop_sidecar function and the state.child and state.base_url mutexes when making the change.
233-249: 💤 Low valueClosing existing window may race with building the new one.
Line 237-238 closes an existing
remote_authwindow, then immediately builds a new one with the same label. If the close is asynchronous, there could be a brief conflict. Consider usingset_urlon an existing window instead of close + recreate, or await the close completion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src-tauri/src/lib.rs` around lines 233 - 249, The current open_remote_login_window closes an existing window returned by app.get_webview_window("remote_auth") then immediately recreates it, which can race; instead, detect the existing window (app.get_webview_window("remote_auth")) and if present call its set_url or navigate method to point to the new login URL (reusing the window) or, if you must close and recreate, await/confirm the completion of w.close() before calling WebviewWindowBuilder::new; update open_remote_login_window to use w.set_url(WebviewUrl::External(login)) when w exists or await the close future prior to building a new WebviewWindowBuilder to avoid label conflict.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src-tauri/src/lib.rs`:
- Around line 212-231: start_sidecar currently spawns the Node sidecar and
returns base_url immediately, which can cause “connection refused” errors; after
spawning the child in start_sidecar (and after setting *state.child and before
setting/returning *state.base_url), poll the sidecar until it accepts
connections by repeatedly attempting a simple TCP connect to 127.0.0.1:port or a
minimal HTTP GET to base_url with a short timeout/backoff (e.g., try ~50 times
with sleep 100ms), and only set *state.base_url and return Ok(base_url) once a
probe succeeds; on timeout/failed readiness, kill the spawned child process,
clear the child state, and return an Err describing the readiness failure.
Ensure you reference start_sidecar, SidecarState (state.child/state.base_url),
the local port variable, and add the needed blocking probe utility (or enable
reqwest blocking) if you choose HTTP probing.
- Around line 99-110: The current generate_local_display_name function
deterministically derives an index from SystemTime, causing collisions and
limited uniqueness; replace the time-based selection with a proper random source
(e.g., use the rand crate or UUIDs) to pick adjective and noun or append a short
random suffix. Update generate_local_display_name to use rand::thread_rng() (or
uuid::Uuid::new_v4()) to select indices into ADJ and NOUN (refer to ADJ and NOUN
arrays) or to generate a unique hex/short-id, and add the chosen dependency to
Cargo.toml; remove the duration_since fallback logic since it will no longer be
needed. Ensure the function remains annotated with #[tauri::command] and still
returns a String like "local-<adj>-<noun>-<rand>" or
"local-<adj>-<noun>-<shortid>".
In `@apps/desktop/src/routes/`+layout.svelte:
- Around line 29-38: The guardRemoteSession function should handle network/fetch
errors when calling fetch(`${base}/api/v1/me`): wrap the await fetch call in a
try/catch (or handle rejection) so network failures don't cause unhandled
promise rejections; on error, log or silently handle the failure and navigate to
the connection flow (use goto("/connect")) or otherwise return early;
specifically update guardRemoteSession to catch errors from fetch for
cfg.remoteBaseUrl and use $page.url.pathname and goto to keep the same control
flow.
In `@apps/desktop/src/routes/connect/`+page.svelte:
- Around line 22-50: The submit function currently uses try/finally without a
catch so thrown network/CORS errors from the first fetch or the me fetch get
swallowed and err is never set; add a catch block around the existing
try/finally in submit to catch any thrown Error (or non-Response exceptions) and
set err to a helpful message (e.g., error.message or a fallback like "Network
error during sign in"), then return (or otherwise stop further processing)
before the finally that resets busy; update references to the two fetch calls
(the first fetch to `${base}/api/auth/sign-in/email` and the second fetch to
`${base}/api/v1/me`) to rely on this catch for unexpected exceptions so the UI
shows feedback instead of silently re-enabling the form.
In `@apps/server/playwright.config.mjs`:
- Around line 17-42: Playwright may start the "server-shell" project before the
API on port 3000 is ready because webServer.url only checks 5173; update the
readiness so both services are confirmed before tests run: either change
webServer to use Playwright's multi-server support (provide an array of
webServer entries/URLs checking both http://127.0.0.1:5173 and
http://127.0.0.1:3000), or modify the dev:e2e command (the script invoked by
webServer.command) to block until both ports are listening or expose a combined
readiness endpoint and point webServer.url to that; ensure reuseExistingServer
logic (reuseExistingServer) still applies when adding the second readiness
check.
In `@apps/server/src/env.ts`:
- Around line 44-52: The isLocalOnlyMode logic includes a leading null from
toOrigin(process.env.BETTER_AUTH_URL) but filters nulls from
BETTER_AUTH_TRUSTED_ORIGINS, causing wrong false results; fix by building
configuredOrigins with both sources mapped through toOrigin and then filtering
out nulls consistently (e.g., .filter((o): o is string => Boolean(o))), and
change the return to explicitly guard empty lists: return
configuredOrigins.length > 0 && configuredOrigins.every((origin) =>
isLocalOrigin(origin)); update references in isLocalOnlyMode, toOrigin usage,
and ensure this preserves the betterAuth.ts fallback-secret path behavior when
only BETTER_AUTH_TRUSTED_ORIGINS is set.
In `@apps/server/src/lib/betterAuth.ts`:
- Around line 19-23: The code currently leaves secret undefined when
BETTER_AUTH_SECRET is missing or too short; update the validation around the
constant secret (and the existing localOnlyMode/isLocalOnlyMode check) to
enforce a fail-fast behavior: if !secret || secret.length < 32 and localOnlyMode
is false, throw an Error (or call process.exit with a clear processLogger.error)
describing the invalid BETTER_AUTH_SECRET so the app won't start insecurely;
retain the current relaxed behavior only when localOnlyMode is true and log a
warning in that case.
In `@apps/server/src/routes/v1/setup.ts`:
- Around line 111-118: The catch block for runFirstTimeSetup currently returns a
400 client error; change it to return a 500 server error for unexpected
failures: capture the thrown error in the catch (e) and return jsonError(c,
"server_error", "Could not complete setup. Email may already be in use.", 500,
getRequestId(c)); optionally log the caught error via your logger before
returning. Ensure you update the catch that wraps runFirstTimeSetup/first-time
setup logic (the catch using jsonError and getRequestId) so genuine server
issues return HTTP 500 instead of 400.
In `@apps/web/src/routes/`+layout.svelte:
- Around line 16-17: The reactive statement uses get(page) so isBare doesn't
re-evaluate on navigation; replace the get(page) usage with the auto-subscribed
$page to ensure reactivity (update the line that computes isBare to reference
$page.url.pathname) and keep the bare array as-is; change any other references
that rely on get(page) in this component to use $page so route changes update
isBare and dependent UI correctly.
- Around line 39-42: The fetch for public config in +layout.svelte (the const pc
/ const d block) lacks error handling and can throw on network failures; wrap
the await fetch(...).json() in a try/catch, check response.ok before parsing,
and fall back to a safe default (e.g., { data: { setupComplete: false } }) on
error; mirror the pattern used in loadTenantName(), log the error (or use
processLogger) and ensure downstream code reads from the safe default so
navigation won't break.
- Around line 47-51: The early return "if (path === \"/login\" || path ===
\"/setup\") return;" makes the later block that checks "if (path === \"/setup\"
&& d.setupComplete) { await goto(\"/\"); return; }" unreachable; fix by either
removing the redundant early-return or moving the setup-completion redirect (the
check using path, d.setupComplete and goto) above the early-return so the
redirect can run when setupComplete is true, ensuring only one of those branches
remains and the unreachable code is eliminated.
In `@apps/web/src/routes/login/`+page.svelte:
- Around line 16-17: The file exposes hardcoded dev credentials in the variables
email and password; update the initialization of email and password (the let
email and let password bindings) so they only default to development values when
running in dev mode (e.g. check import.meta.env.DEV or NODE_ENV ===
"development"), otherwise initialize them to empty strings (or remove defaults
entirely) to ensure production bundles do not contain the credentials; change
the logic where email/password are assigned to conditionally set the dev values
only when the dev flag is true.
In `@apps/web/src/routes/welcome/`+page.svelte:
- Around line 8-18: The onMount block containing the fetch("/api/v1/me") call
can throw network exceptions that prevent redirect; wrap the async logic in a
try/catch around the fetch/json parsing and set isAdmin only on success, and in
the catch call await goto("/login") (and return) so any thrown error
(network/CORS/etc.) triggers the same redirect path as a non-ok response;
reference the onMount anonymous async function, the fetch("/api/v1/me") call,
the json parsing into j, the isAdmin assignment, and the goto("/login") call
when making this change.
---
Nitpick comments:
In `@apps/desktop/e2e/global-setup.ts`:
- Around line 19-20: The repoRoot calculation is fragile because it uses
path.resolve(process.cwd(), "..", "..") which breaks when tests are run from
other CWDs; update repoRoot to derive from the setup file location instead (use
__dirname if the compiled bundle is CJS, otherwise compute from import.meta.url
via fileURLToPath and path.dirname), and then set prismaDir =
path.join(repoRoot, "packages", "prisma") as before; change the code around the
repoRoot and prismaDir definitions (replace usage of process.cwd() and reference
path.resolve(process.cwd(), "..", "..")) so the path is computed from the file's
directory rather than the current working directory.
In `@apps/desktop/e2e/onboarding.spec.ts`:
- Around line 29-41: Duplicate admin credentials are hard-coded in the test
fetch body and in completeSetupIfNeeded; extract them into shared constants
(e.g., ADMIN_EMAIL, ADMIN_PASSWORD, TENANT_NAME or a single SETUP_CREDENTIALS
object) and reference those constants from both the POST payload in the fetch to
/api/v1/setup/complete and from the completeSetupIfNeeded helper so both places
use the same source of truth and avoid drift.
In `@apps/desktop/src-tauri/src/lib.rs`:
- Around line 125-135: The stop_sidecar function currently ignores poisoned
mutexes by using if let Ok(...) which can skip killing the child process; change
the two lock acquisitions on state.child and state.base_url to recover from
poisoned locks (e.g. use .lock().unwrap_or_else(|e| e.into_inner()) or
equivalent) so you still obtain the mutex guard, kill the child (call ch.kill())
and clear the Option, and add a warning log when a PoisonError is encountered to
aid debugging; reference the stop_sidecar function and the state.child and
state.base_url mutexes when making the change.
- Around line 233-249: The current open_remote_login_window closes an existing
window returned by app.get_webview_window("remote_auth") then immediately
recreates it, which can race; instead, detect the existing window
(app.get_webview_window("remote_auth")) and if present call its set_url or
navigate method to point to the new login URL (reusing the window) or, if you
must close and recreate, await/confirm the completion of w.close() before
calling WebviewWindowBuilder::new; update open_remote_login_window to use
w.set_url(WebviewUrl::External(login)) when w exists or await the close future
prior to building a new WebviewWindowBuilder to avoid label conflict.
In `@apps/desktop/src/lib/api.ts`:
- Around line 70-81: apiFetch currently calls getDesktopConfig() and then
apiBaseUrl(), but apiBaseUrl() calls getDesktopConfig() again causing redundant
awaits; refactor by fetching the config once in apiFetch and pass it into a new
or overloaded helper (e.g., add apiBaseUrlFromConfig(config: DesktopConfig) or
change apiBaseUrl to accept an optional config parameter) so apiFetch uses the
single config instance to compute base URL and set credentials; update apiFetch
to call apiBaseUrlFromConfig(config) (or apiBaseUrl(config)) and remove the
second getDesktopConfig() call, and adapt any callers of the modified apiBaseUrl
signature accordingly.
In `@apps/desktop/src/routes/`+layout.svelte:
- Around line 45-48: afterNavigate currently calls getDesktopConfig() on every
navigation causing redundant reads; change this by caching the result (e.g.,
store the config in the existing cfg variable or a Svelte writable store) and
only call getDesktopConfig() if cfg is unset or explicitly invalidated. Update
the afterNavigate handler to use the cached cfg, add a clear/invalidateConfig
function or listen for settings-change events to force a reload, and keep
guardRemoteSession() as-is but ensure it uses the cached cfg when needed so
navigation no longer triggers disk/storage reads each time.
In `@apps/desktop/src/routes/`+page.svelte:
- Around line 35-52: The chooseRemote function currently trims remoteUrl but
does not validate it before calling setDesktopConfig; add a validation step
using a robust check (e.g., attempt new URL(trimmed) or a strict regex) against
the trimmed remoteUrl and if it fails set err to a helpful message and return
early (do not call setDesktopConfig or goto), otherwise proceed to call
setDesktopConfig with remoteBaseUrl set to the trimmed value; update references
in this flow (chooseRemote, remoteUrl, setDesktopConfig, getDesktopConfig)
accordingly so invalid URLs are rejected before persisting.
In `@apps/server/e2e/global-setup.ts`:
- Around line 18-32: Wrap the execSync call that runs "npx prisma migrate reset
..." in a try-catch inside apps/server/e2e/global-setup.ts so failures provide
clearer context; catch the error around the execSync invocation (the block that
uses prismaDir and env with POSTGRES_DATABASE_URL/dbUrl), log a descriptive
message including prismaDir and the caught error (e.g., via console.error or
existing logger), and then rethrow or call process.exit(1) to fail fast with the
improved message.
In `@apps/server/src/__tests__/auth-routes.test.ts`:
- Around line 59-87: Add a new unit test in
apps/server/src/__tests__/auth-routes.test.ts that mocks findFirstAppUser to
return null and issues a POST to the /api/v1/auth/dev-login route (same request
shape as the existing test); assert the response status and body represent the
expected error (e.g., 404 or 400 and an error message), and verify that
signInEmail is not called. Use the same test harness/makeApp() setup, and
reference findFirstAppUser and signInEmail in the assertions to ensure the
negative path is covered.
In `@apps/server/src/lib/initial-setup.ts`:
- Around line 81-85: Replace the sequential prisma.boardColumn.create loop in
initial-setup.ts with a single prisma.boardColumn.createMany call: map the
existing columns array to include the boardId for each item (e.g., columns.map(c
=> ({ boardId: board.id, ...c }))), call await prisma.boardColumn.createMany({
data: mappedColumns }) and adjust any code that relied on created records
(createMany returns a count, not created rows); keep using the same symbols
(columns, board.id, prisma.boardColumn.create -> createMany) and await the
promise for proper setup sequencing.
- Around line 22-106: The seed steps after creating appUser (admin) in
runFirstTimeSetup are not atomic; wrap the subsequent
org/project/board/columns/projectMember/ticket creation and the final
tenant.update in a single Prisma transaction so partial seeds can't be left
behind. Replace the multiple prisma.* calls (organization.create,
project.create, board.create, boardColumn.create loop, projectMember.create,
boardColumn.findFirstOrThrow, ticket.create, tenant.update) with a
prisma.$transaction callback and use the transaction client (e.g.,
tx.organization.create, tx.project.create, etc.) inside it so all creations and
the tenant update succeed or roll back together.
In `@apps/server/src/lib/setup-config.ts`:
- Around line 27-36: Wrap the JSON parsing and schema validation inside
loadInstanceConfigFile in a try-catch to surface a clear, contextual error that
includes the TRACKR_CONFIG_PATH; specifically catch errors from JSON.parse(raw)
and instanceConfigSchema.parse(j) and rethrow (or throw a new Error) with a
message like "Failed to load/parse/validate config at <path>: <original error
message>" so operators know which file failed (refer to
process.env.TRACKR_CONFIG_PATH, JSON.parse, and instanceConfigSchema.parse to
locate the code).
In `@apps/server/src/middleware/session.ts`:
- Around line 10-24: Wrap the call to auth.api.getSession inside a try-catch in
the loadSession middleware so transient or unexpected errors from
auth.api.getSession do not bubble up; on catch, log the error (or use
c.log/processLogger) and call c.set("authUser", null) then return next() to
treat the request as unauthenticated; keep the existing logic for
prisma.appUser.findFirst unchanged so only failures from auth.api.getSession are
handled defensively.
In `@apps/web/src/routes/login/`+page.svelte:
- Around line 62-66: Replace the raw text-only error handling for the fetch
response (the res / t / err block) with logic that first attempts to parse
res.json() and extract a user-friendly message from common fields like message
or error, falling back to res.text() and then a default "Sign in failed"; set
err to that extracted string so structured JSON errors are shown instead of raw
JSON. Ensure any JSON parse errors are caught and ignored so the fallback to
text/default still occurs.
- Around line 30-45: The page renders with default auth values while onMount
fetches /api/v1/public-config, causing a flash and delayed redirect; introduce a
loading boolean (e.g., loading) initialized true, set it before/after the fetch
inside onMount around the fetch("/api/v1/public-config") call, and only render
the form/UI when loading is false (or render a spinner when true); if
j.data.setupComplete is false call goto("/setup") before clearing loading (so
redirect occurs immediately) and still update setupComplete, auth, and
authAvailable after the fetch. Ensure you update references to
setupComplete/auth/authAvailable rendering logic to wait for loading to be
false.
In `@apps/web/src/routes/setup/`+page.svelte:
- Around line 36-41: The loop that copies p.authSettings into auth (checking
p?.authSettings and iterating keys) can be simplified: validate p.authSettings
exists, then build an object containing only keys whose values are boolean and
merge it into auth via Object.assign. Update the code that references
p.authSettings, auth, and AuthFlags to avoid non-null assertions and casts by
filtering Object.entries(p.authSettings) for typeof value === "boolean" and then
Object.assign(auth, filteredObject).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fcdc6d13-7663-4e14-9e23-232c295fb1fa
⛔ Files ignored due to path filters (2)
apps/desktop/src-tauri/icons/icon.pngis excluded by!**/*.pngpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (62)
.env.schema.github/workflows/ci.yml.gitignore.vscode/settings.jsonCONTRIBUTING.mdREADME.mdapps/desktop/e2e/global-setup.tsapps/desktop/e2e/onboarding.spec.tsapps/desktop/e2e/tauri-e2e-bridge.tsapps/desktop/package.jsonapps/desktop/playwright.config.mjsapps/desktop/src-tauri/capabilities/default.jsonapps/desktop/src-tauri/src/lib.rsapps/desktop/src/lib/api.tsapps/desktop/src/lib/desktopConfig.tsapps/desktop/src/routes/+layout.svelteapps/desktop/src/routes/+page.svelteapps/desktop/src/routes/connect/+page.svelteapps/desktop/turbo.jsonapps/desktop/vite.config.jsapps/server/e2e/global-setup.tsapps/server/e2e/onboarding.spec.tsapps/server/package.jsonapps/server/playwright.config.mjsapps/server/src/__tests__/auth-routes.test.tsapps/server/src/__tests__/integration/dev-flow.integration.test.tsapps/server/src/env.tsapps/server/src/hono-env.d.tsapps/server/src/index.tsapps/server/src/lib/betterAuth.tsapps/server/src/lib/db.tsapps/server/src/lib/initial-setup.tsapps/server/src/lib/platform.tsapps/server/src/lib/setup-config.tsapps/server/src/middleware/session.tsapps/server/src/routes/v1/auth.tsapps/server/src/routes/v1/index.tsapps/server/src/routes/v1/setup.tsapps/server/src/scripts/seed-postgres.tsapps/server/vitest.config.tsapps/web/package.jsonapps/web/src/lib/auth-client.tsapps/web/src/lib/shell.tsapps/web/src/routes/+layout.svelteapps/web/src/routes/login/+page.svelteapps/web/src/routes/projects/+page.svelteapps/web/src/routes/setup/+page.svelteapps/web/src/routes/welcome/+page.sveltepackage.jsonpackages/desktop-local-api/src/__tests__/local-api.integration.test.tspackages/desktop-local-api/src/db.tspackages/desktop-local-api/src/index.tspackages/prisma/package.jsonpackages/prisma/prisma.config.sqlite.tspackages/prisma/prisma.config.tspackages/prisma/prisma/postgres/migrations/20250426120000_better_auth_and_app_user/migration.sqlpackages/prisma/prisma/postgres/migrations/20260502160558/migration.sqlpackages/prisma/prisma/postgres/migrations/migration_lock.tomlpackages/prisma/prisma/postgres/schema.prismapackages/prisma/seed.tsscripts/dev-e2e-desktop.mjsscripts/dev-e2e.mjs
…de tenant admin option
… improve environment variable handling for E2E tests
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/prisma/seed.ts (1)
4-6: Silent fallback to localhost in seed script — consider adding a guardThe code silently falls back to
localhost:5432/trackrwhenPOSTGRES_DATABASE_URLis unset. There is no intermediate fallback (the AI summary incorrectly mentionsDATABASE_URL), and the file contains nodotenvimport.This is compounded by Prisma v7's removal of automatic environment variable loading. If the seed is run without
POSTGRES_DATABASE_URLexplicitly set in the shell environment, it will silently target the hardcoded localhost address. This works fine for local development but creates a risk of accidental database misconfiguration in CI or staging if env vars are not properly supplied.Consider adding an explicit guard to fail fast or warn when the variable is absent, especially in production contexts:
Optional robustness improvements
Fail fast in production:
const connectionString = process.env.POSTGRES_DATABASE_URL ?? (process.env.NODE_ENV === "production" ? (() => { throw new Error("POSTGRES_DATABASE_URL must be set in production"); })() : "postgresql://postgres:postgres@localhost:5432/trackr");Or log a warning:
const connectionString = process.env.POSTGRES_DATABASE_URL ?? // eslint-disable-next-line no-console (console.warn("⚠️ POSTGRES_DATABASE_URL not set, using local default"), "postgresql://postgres:postgres@localhost:5432/trackr");Note: The same pattern exists in
prisma.config.ts(lines 10–12) and affects Prisma CLI commands as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/prisma/seed.ts` around lines 4 - 6, The seed script currently assigns connectionString to a hardcoded localhost URL when POSTGRES_DATABASE_URL is missing; change this to fail-fast or warn: at the top of packages/prisma/seed.ts (and mirror in prisma.config.ts) explicitly load environment variables if desired (e.g., require('dotenv').config()) and then check process.env.POSTGRES_DATABASE_URL — if it's undefined and NODE_ENV !== 'development' throw an error and exit (or at minimum log a clear warning and refuse to run), otherwise allow a local-only fallback only when NODE_ENV === 'development'; update the connectionString assignment to use the validated POSTGRES_DATABASE_URL and remove the silent hardcoded default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/env.ts`:
- Around line 9-18: toOrigin currently returns the ASCII string "null" for
opaque origins (e.g. new URL("tauri://localhost").origin === "null"), which then
survives .filter(Boolean) and breaks isLocalOnlyMode; update the toOrigin
function to detect opaque origins by checking the parsed URL's origin and
returning actual null when origin === "null" (i.e., after constructing new
URL(value) if origin === "null" return null, otherwise return origin), so
downstream checks in isLocalOrigin and isLocalOnlyMode behave correctly.
In `@apps/server/src/lib/betterAuth.ts`:
- Around line 6-17: Update the top comment around the createRequire/nodeRequire
usage to specify the correct minimal Node versions for requiring ESM: "Node ≥
22.12.0 or ≥ 20.19.0" instead of "Node 22+", and keep the note about better-auth
not using top-level await; then add an engines entry to apps/server/package.json
to enforce the minimum Node engine (e.g., "node": ">=20.19.0 || >=22.12.0") so
runtime installs fail fast; touch the lines referencing createRequire,
nodeRequire and the betterAuth/prisma/social-providers/plugins imports to locate
the change.
---
Nitpick comments:
In `@packages/prisma/seed.ts`:
- Around line 4-6: The seed script currently assigns connectionString to a
hardcoded localhost URL when POSTGRES_DATABASE_URL is missing; change this to
fail-fast or warn: at the top of packages/prisma/seed.ts (and mirror in
prisma.config.ts) explicitly load environment variables if desired (e.g.,
require('dotenv').config()) and then check process.env.POSTGRES_DATABASE_URL —
if it's undefined and NODE_ENV !== 'development' throw an error and exit (or at
minimum log a clear warning and refuse to run), otherwise allow a local-only
fallback only when NODE_ENV === 'development'; update the connectionString
assignment to use the validated POSTGRES_DATABASE_URL and remove the silent
hardcoded default.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4112ea35-e5fc-47e3-bd36-0f6ba9882582
⛔ Files ignored due to path filters (1)
apps/desktop/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.env.schemaapps/desktop/src-tauri/Cargo.tomlapps/desktop/src-tauri/src/lib.rsapps/desktop/src/routes/+layout.svelteapps/desktop/src/routes/connect/+page.svelteapps/server/playwright.config.mjsapps/server/src/env.tsapps/server/src/lib/betterAuth.tsapps/server/vitest.config.tspackages/prisma/seed.ts
✅ Files skipped from review due to trivial changes (3)
- apps/desktop/src-tauri/Cargo.toml
- apps/server/playwright.config.mjs
- apps/server/vitest.config.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/routes/+layout.svelte
Comment on lines
+9
to
+18
| function toOrigin(value?: string): string | null { | ||
| if (!value) { | ||
| return null; | ||
| } | ||
| try { | ||
| return new URL(value).origin; | ||
| } catch { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm that Node.js URL parser returns the string "null" for tauri:// origins.
node -e "
const u = new URL('tauri://localhost');
console.log('origin:', JSON.stringify(u.origin));
console.log('is truthy:', Boolean(u.origin));
"Repository: nealhorner/trackr
Length of output: 92
🏁 Script executed:
cat -n apps/server/src/env.tsRepository: nealhorner/trackr
Length of output: 1903
toOrigin("tauri://localhost") returns the truthy string "null", breaking isLocalOnlyMode detection for tauri-only setups.
Per the WHATWG URL Standard, non-standard-scheme URLs produce an opaque origin serialized as the ASCII string "null". The Node.js URL parser confirms: new URL("tauri://localhost").origin === "null" (a truthy string, not JavaScript null). This string passes the .filter(Boolean) at line 48, but then isLocalOrigin("null") returns false because:
- Line 21's hardcoded checks for
"tauri://localhost"don't match the string"null" - Parsing
new URL("null")throws (no valid scheme), returningfalsefrom the catch block
Concrete impact: if BETTER_AUTH_TRUSTED_ORIGINS=tauri://localhost, configuredOrigins becomes ["null"], every(isLocalOrigin) returns false, and isLocalOnlyMode() returns false despite being a local-only setup. This causes betterAuth.ts to fail at startup due to missing BETTER_AUTH_SECRET.
https://tauri.localhost is unaffected because https: is a special scheme with a valid origin.
🛠️ Proposed fix
function toOrigin(value?: string): string | null {
- if (!value) {
- return null;
- }
+ if (!value) return null;
+ const trimmed = value.trim();
+ // tauri:// is a non-standard scheme; the WHATWG URL parser produces the
+ // opaque-origin string "null" for it. Preserve the raw value so that
+ // isLocalOrigin() can match it with its hardcoded check.
+ if (trimmed === "tauri://localhost") return "tauri://localhost";
try {
- return new URL(value).origin;
+ const origin = new URL(trimmed).origin;
+ // Guard against other non-standard schemes that also produce the opaque
+ // origin string "null" — treat them as unparseable and drop them.
+ return origin !== "null" ? origin : null;
} catch {
return null;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function toOrigin(value?: string): string | null { | |
| if (!value) { | |
| return null; | |
| } | |
| try { | |
| return new URL(value).origin; | |
| } catch { | |
| return null; | |
| } | |
| } | |
| function toOrigin(value?: string): string | null { | |
| if (!value) return null; | |
| const trimmed = value.trim(); | |
| // tauri:// is a non-standard scheme; the WHATWG URL parser produces the | |
| // opaque-origin string "null" for it. Preserve the raw value so that | |
| // isLocalOrigin() can match it with its hardcoded check. | |
| if (trimmed === "tauri://localhost") return "tauri://localhost"; | |
| try { | |
| const origin = new URL(trimmed).origin; | |
| // Guard against other non-standard schemes that also produce the opaque | |
| // origin string "null" — treat them as unparseable and drop them. | |
| return origin !== "null" ? origin : null; | |
| } catch { | |
| return null; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/env.ts` around lines 9 - 18, toOrigin currently returns the
ASCII string "null" for opaque origins (e.g. new URL("tauri://localhost").origin
=== "null"), which then survives .filter(Boolean) and breaks isLocalOnlyMode;
update the toOrigin function to detect opaque origins by checking the parsed
URL's origin and returning actual null when origin === "null" (i.e., after
constructing new URL(value) if origin === "null" return null, otherwise return
origin), so downstream checks in isLocalOrigin and isLocalOnlyMode behave
correctly.
Comment on lines
+6
to
+17
| const nodeRequire = createRequire(__filename); | ||
| // better-auth is published as ESM; Node 22+ can still load it from CJS via `createRequire` | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports, @typescript-eslint/no-explicit-any | ||
| const { betterAuth } = nodeRequire("better-auth") as any; | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports, @typescript-eslint/no-explicit-any | ||
| const { prismaAdapter } = nodeRequire("better-auth/adapters/prisma") as any; | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports, @typescript-eslint/no-explicit-any | ||
| const { google, github, apple } = nodeRequire( | ||
| "better-auth/social-providers", | ||
| ) as any; | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports, @typescript-eslint/no-explicit-any | ||
| const { genericOAuth, okta } = nodeRequire("better-auth/plugins") as any; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1. Check declared engines constraint (if any).
fd -i 'package.json' apps/server --exec grep -H '"engines"' {}
# 2. Check the Node.js version in use (.nvmrc / .node-version / Volta).
fd -g '.nvmrc' -g '.node-version' --exec cat {}
cat package.json 2>/dev/null | grep -A5 '"volta"' || true
# 3. Check if better-auth or its sub-paths use top-level await.
fd -e mjs -e js . -p 'node_modules/better-auth' --exec grep -l 'await ' {} | head -20Repository: nealhorner/trackr
Length of output: 274
🏁 Script executed:
# Find and check package.json files
find . -maxdepth 3 -name package.json -type f | head -20Repository: nealhorner/trackr
Length of output: 293
🏁 Script executed:
# Check for .nvmrc and .node-version files
find . -maxdepth 2 \( -name '.nvmrc' -o -name '.node-version' \) -type fRepository: nealhorner/trackr
Length of output: 43
🏁 Script executed:
# Check apps/server/package.json for engines constraint if it exists
cat apps/server/package.json 2>/dev/null | head -50Repository: nealhorner/trackr
Length of output: 801
🌐 Web query:
Node.js require ESM unflagged version 22.12.0 20.19.0 experimental flag
💡 Result:
In Node.js, the ability to use require to load native ES modules (ESM) without throwing ERR_REQUIRE_ESM was previously behind the --experimental-require-module flag. - Node.js 22.12.0 (released 2024-12-03): Unflagged, now enabled by default (flag defaults to true). Still experimental; emits a warning on first use unless from node_modules. Disable with --no-experimental-require-module. Supports synchronous ESM only (no top-level await, throws ERR_REQUIRE_ASYNC_MODULE if present). Check with process.features.require_module. - Node.js 20.19.0 (released 2025-03-13): Backported and unflagged, enabled by default. No warning unless --trace-require-module used. Same limitations and disable option. Prior versions require the explicit --experimental-require-module flag. By 2026, marked stable across LTS lines including these versions.
Citations:
- 1: https://github.com/nodejs/node/releases/tag/v22.12.0
- 2: https://nodejs.org/en/blog/release/v22.12.0
- 3: https://joyeecheung.github.io/blog/2025/12/30/require-esm-in-node-js-from-experiment-to-stability/
- 4: https://github.com/nodejs/node/releases/tag/v20.19.0
- 5: https://nodejs.org/en/blog/release/v20.19.0
- 6: module: unflag --experimental-require-module nodejs/node#55085
- 7: Tracking issue: require(esm) nodejs/node#52697
🌐 Web query:
better-auth 1.6.9 package ESM top-level await
💡 Result:
better-auth 1.6.9 does not indicate usage of top-level await; instead it is published as an ES module package ("type": "module") and its async behavior is implemented via regular async functions/Promises (e.g., awaitable context and provider config). If you are encountering a top-level-await-related error, it’s likely coming from your build/tooling (CJS output or transforming ESM/TLA) rather than from better-auth 1.6.9 itself. What the evidence shows: 1) better-auth 1.6.9 packages are ESM (“type”: “module”). The v1.6.9 release comparison shows both the better-auth and @better-auth/core package.json files with "type": "module". 2) The project uses async resolution patterns instead of requiring top-level await. For example, Better Auth has logic that awaits a promise-based context in middleware (const authContext = await ctx) rather than using module-level top-level await. 3) The Better Auth changelog for recent versions includes ESM/CJS compatibility fixes for a dependency (samlify), but it does not mention top-level await as the underlying issue. Practical guidance: - Ensure your consumer build outputs ESM (not CJS) if your code or dependencies use top-level await. - If you run a bundler/transpiler that converts ESM to CJS (or otherwise rewrites module execution), top-level await may fail—this is a common root cause seen in ESM/CJS + tooling scenarios. If you paste the exact error message and your runtime/build setup (Node version, framework, bundler, whether you transpile node_modules), I can pinpoint whether it’s your tooling rather than better-auth itself.
Citations:
- 1: https://registry.npmjs.org/better-auth
- 2: better-auth/better-auth@v1.6.8...v1.6.9
- 3: better-auth/better-auth@1bbdcfbd
- 4: https://better-auth.com/changelog
- 5: https://www.npmjs.com/package/better-auth
- 6: Unexpected
top-level await errorfortype: module+ top level await innode_modulesprivatenumber/tsx#552
require(ESM) requires Node.js ≥ 22.12.0 (or ≥ 20.19.0) — the comment "Node 22+" understates this.
require(ESM) is unflagged from Node.js v20.19.0+ and v22.12.0+. On v22.0.0–v22.11.x the --experimental-require-module flag is still required, and on any v20 prior to v20.19.0 it is unavailable entirely — without it, require() of an ES module throws ERR_REQUIRE_ESM.
Consider:
- Tightening the comment to
"Node ≥ 22.12.0 or ≥ 20.19.0". - Adding an
enginesconstraint inapps/server/package.jsonto enforce the minimum version (currently missing).
Note: better-auth@1.6.9 does not use top-level await, so that concern does not apply here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/lib/betterAuth.ts` around lines 6 - 17, Update the top
comment around the createRequire/nodeRequire usage to specify the correct
minimal Node versions for requiring ESM: "Node ≥ 22.12.0 or ≥ 20.19.0" instead
of "Node 22+", and keep the note about better-auth not using top-level await;
then add an engines entry to apps/server/package.json to enforce the minimum
Node engine (e.g., "node": ">=20.19.0 || >=22.12.0") so runtime installs fail
fast; touch the lines referencing createRequire, nodeRequire and the
betterAuth/prisma/social-providers/plugins imports to locate the change.
…ecks and improve guard function logic
… error handling and ensure valid data structure
…logic for setup and login paths
…te configuration; adjust login page to conditionally set credentials based on environment
…etch; redirect to login on failure
…ettierignore and .vitest-cache to .gitignore, and enhance build/test scripts for better modularity and clarity
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 111: The CI job integration_and_e2e is currently blocked by build_desktop
via the needs: [quality, build_web_stack, build_desktop] entry; remove
build_desktop from the needs array (so it becomes needs: [quality,
build_web_stack]) in the integration_and_e2e job definition to avoid
desktop-only failures gating web/server integration and Playwright tests. Locate
the integration_and_e2e job in the workflow and update its needs list
accordingly.
In @.husky/pre-commit:
- Line 1: The pre-commit hook runs "npm run format:fix" but doesn't restage
files, so commits may contain unformatted versions; update the .husky/pre-commit
hook to restage changes after formatting (e.g., run the formatter then call git
add to stage modified files) or switch to using lint-staged so only staged files
are formatted and automatically re-staged; ensure the hook either stages
formatted changes (git add) or uses lint-staged invocation instead of only
running "npm run format:fix".
- Line 3: The pre-commit hook currently runs the full monorepo build via the
command "npm run build:all", which forces local Rust/Tauri toolchain installs;
update the hook to skip the desktop build by replacing "npm run build:all" with
a fast build script that excludes desktop artifacts (for example "npm run
build:web" or a project-specific "npm run build:fast" that omits Tauri/Rust), or
wrap the call to only run targeted packages; ensure the updated line in
.husky/pre-commit no longer invokes "npm run build:all" so web/server-only
contributors can commit without the desktop toolchain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3ab0b162-b45f-4c02-acd6-b54f38c30a6e
📒 Files selected for processing (6)
.github/workflows/ci.yml.gitignore.husky/pre-commit.prettierignorepackage.jsonturbo.json
✅ Files skipped from review due to trivial changes (2)
- .prettierignore
- .gitignore
| run: npm run build | ||
|
|
||
| integration_and_e2e: | ||
| needs: [quality, build_web_stack, build_desktop] |
There was a problem hiding this comment.
Don't gate web/server tests on the desktop build.
Line 111 makes integration_and_e2e wait for build_desktop, but this job only runs server integration tests and Playwright with --filter=!@trackr/desktop. A desktop-only failure now prevents unrelated web/server validation from running.
Suggested fix
- needs: [quality, build_web_stack, build_desktop]
+ needs: [quality, build_web_stack]📝 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
| needs: [quality, build_web_stack, build_desktop] | |
| needs: [quality, build_web_stack] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml at line 111, The CI job integration_and_e2e is
currently blocked by build_desktop via the needs: [quality, build_web_stack,
build_desktop] entry; remove build_desktop from the needs array (so it becomes
needs: [quality, build_web_stack]) in the integration_and_e2e job definition to
avoid desktop-only failures gating web/server integration and Playwright tests.
Locate the integration_and_e2e job in the workflow and update its needs list
accordingly.
| @@ -1,4 +1,4 @@ | |||
| npm run format:fix | |||
There was a problem hiding this comment.
Don't auto-fix in pre-commit unless you restage.
Line 1 writes formatting changes to the working tree, but this hook never adds them back to the index. The hook can pass on formatted files while the commit still contains the pre-format version.
Suggested fix
-npm run format:fix
+npm run format:check📝 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
| npm run format:fix | |
| npm run format:check |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.husky/pre-commit at line 1, The pre-commit hook runs "npm run format:fix"
but doesn't restage files, so commits may contain unformatted versions; update
the .husky/pre-commit hook to restage changes after formatting (e.g., run the
formatter then call git add to stage modified files) or switch to using
lint-staged so only staged files are formatted and automatically re-staged;
ensure the hook either stages formatted changes (git add) or uses lint-staged
invocation instead of only running "npm run format:fix".
| npm run test | ||
| npm run build -- --filter=!@trackr/desktop | ||
| npm run format No newline at end of file | ||
| npm run build:all |
There was a problem hiding this comment.
Keep the desktop build out of the pre-commit fast path.
Line 3 runs the full monorepo build, so every commit now depends on the Rust/Tauri desktop toolchain being installed locally. That will block web/server-only contributors from committing at all.
Suggested fix
-npm run build:all
+npx turbo build --filter=@trackr/web --filter=@trackr/server📝 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
| npm run build:all | |
| npx turbo build --filter=@trackr/web --filter=@trackr/server |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.husky/pre-commit at line 3, The pre-commit hook currently runs the full
monorepo build via the command "npm run build:all", which forces local
Rust/Tauri toolchain installs; update the hook to skip the desktop build by
replacing "npm run build:all" with a fast build script that excludes desktop
artifacts (for example "npm run build:web" or a project-specific "npm run
build:fast" that omits Tauri/Rust), or wrap the call to only run targeted
packages; ensure the updated line in .husky/pre-commit no longer invokes "npm
run build:all" so web/server-only contributors can commit without the desktop
toolchain.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Initial app setup with databases, auth, and testing.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores