Merged
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 Supabase-backed auth and token handling to the Hono backend, implements protected item CRUD, sync and debug endpoints, updates frontend to produce/use access tokens (client headers, types, store), adjusts TypeScript/Vite config, and tweaks desktop window coordinates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Frontend as Frontend (HonoClient + Store)
participant Backend as Hono Backend
participant Supabase as Supabase
Client->>Frontend: Submit credentials / request actions
Frontend->>Frontend: tokenGetter reads store.accessToken at request time
Frontend->>Backend: HTTP request (Authorization: Bearer <token>)
Backend->>Backend: requireAuth parses token, validates user (via Supabase)
Backend->>Supabase: Query/Insert/Update items using token or anon client
Supabase-->>Backend: Returns user/session/items
Backend->>Backend: normalizeItem / parseStatus
Backend-->>Frontend: JSON response (items, ids, or auth tokens)
Frontend-->>Client: Render updated UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (8)
apps/backend/src/index.ts (5)
9-10: Clarify dotenv loading behavior.The second
dotenv.config()call won't override variables already set by the first call (dotenv's default behavior). If this is intentional (local.envtakes precedence over root), this is correct. Otherwise, consider using theoverrideoption for the second call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/index.ts` around lines 9 - 10, The two dotenv.config calls (dotenv.config({ path: resolve(process.cwd(), '.env') }) and dotenv.config({ path: resolve(process.cwd(), '../../.env') })) currently leave values from the first call intact because dotenv does not overwrite existing env vars; if you want root .env to override local .env, change the second call to pass override: true (dotenv.config({ path: resolve(process.cwd(), '../../.env'), override: true })), or if local should always win keep as-is — update the second dotenv.config invocation accordingly to explicitly set override or remove the redundant load to match the intended precedence.
152-177: Consider adding rate limiting to the login endpoint.The login endpoint lacks rate limiting, which could expose it to brute-force attacks. Consider adding rate limiting middleware or leveraging Supabase's built-in rate limiting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/index.ts` around lines 152 - 177, The login handler at app.post('/api/auth/login') currently accepts credentials via parseJson and calls createAnonSupabase.auth.signInWithPassword without any throttling; add rate limiting to mitigate brute-force attempts by wrapping this route or adding middleware that tracks attempts per IP or per account (e.g., using an in-memory store, Redis, or a library) and rejects requests after a threshold with a 429 status before calling createAnonSupabase and fetchProfileUsername; ensure the limiter checks occur before invoking signInWithPassword and that successful logins reset the counter for that key.
401-479: Debug seed endpoint should also be guarded in production.Similar to the reset endpoint, seed operations should be restricted to non-production environments to prevent accidental test data creation.
🛡️ Proposed fix
app.post('/api/debug/seed', async (c) => { + if (process.env.NODE_ENV === 'production') { + return c.json({ error: 'This endpoint is disabled in production' }, 403); + } + const { userId, token } = c.get('auth');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/index.ts` around lines 401 - 479, The /api/debug/seed route handler should be protected from running in production: add an environment check at the start of the app.post('/api/debug/seed', ...) handler (mirroring the reset endpoint) that detects production (e.g. process.env.NODE_ENV === 'production' or your existing isProduction/isProd helper) and immediately returns a non-descriptive 404/403 or no-op response instead of performing the seed; keep the check before createSupabaseWithToken and the upsert so no test data is created in production.
297-305: Consider refactoring internal request forwarding to use shared handlers.The pattern of forwarding requests via
app.request()works but has drawbacks:
- Auth middleware runs twice for each forwarded request
- Additional overhead from creating new Request objects
- Harder to trace and debug
A cleaner approach would be to extract the core logic into shared handler functions.
♻️ Example refactor using shared handler
+async function handleCreateItem(c: Context<AppEnv>): Promise<Response> { + const { userId, token } = c.get('auth'); + const supabase = createSupabaseWithToken(token); + // ... existing create logic from POST /api/items +} app.post('/api/items', async (c) => { - const { userId, token } = c.get('auth'); - // ... existing logic + return handleCreateItem(c); }); app.post('/api/items/create', async (c) => { - return app.request( - new Request(new URL('/api/items', c.req.url).toString(), { - method: 'POST', - headers: c.req.raw.headers, - body: await c.req.raw.text(), - }), - ); + return handleCreateItem(c); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/index.ts` around lines 297 - 305, The current POST forwarding in app.post('/api/items/create', ...) recreates a Request and re-invokes middleware (causing double auth and extra overhead); refactor by extracting the core logic of the '/api/items' endpoint into a shared handler function (e.g., export async function handleCreateItem(ctx) or createItemHandler) that contains the actual create logic, then replace the existing '/api/items' route to call that handler and change the '/api/items/create' route to call the same handler directly (passing the original context/body) instead of using app.request; ensure the shared handler accepts the same context or explicit params (req body, auth user) so middleware runs only once and request construction is removed.
47-47: Consider restricting CORS origins for production.The default
cors()configuration allows all origins, which is fine for development but should be restricted in production to prevent unauthorized cross-origin requests.♻️ Example production-ready CORS config
-app.use('/api/*', cors()); +app.use('/api/*', cors({ + origin: process.env.NODE_ENV === 'production' + ? (process.env.ALLOWED_ORIGINS?.split(',') ?? []) + : '*', +}));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/index.ts` at line 47, The current use of app.use('/api/*', cors()) allows all origins; update the CORS setup to restrict origins in production by replacing that call with a CORS middleware configured with an origin option (e.g., an allowlist array) and conditional logic based on NODE_ENV (or a config flag); locate the app.use('/api/*', cors()) line and implement a check that uses a scoped allowlist (or a function to validate origin) and only permits those origins in production while keeping the permissive cors() for development/testing.apps/frontend/src/api/honoClient.ts (1)
64-75: Method naming is misleading.The
post()method accepts amethodparameter allowing PATCH/DELETE, but the name suggests it only handles POST requests. Consider renaming torequest()or creating separate methods for clarity.♻️ Proposed rename for clarity
- async post(path: string, body?: unknown, method: string = 'POST'): Promise<Response> { + async request(path: string, body?: unknown, method: string = 'POST'): Promise<Response> {Or expose proper RESTful methods:
async patch(path: string, body?: unknown): Promise<Response> { return this.request(path, body, 'PATCH'); } async delete(path: string): Promise<Response> { return this.request(path, undefined, 'DELETE'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/src/api/honoClient.ts` around lines 64 - 75, The post() method name is misleading because it accepts a method parameter; rename post() to request() (or add a new request(path, body?, method='POST') and keep post() as a thin wrapper) and add explicit helpers like patch(path, body?) and delete(path) that call request() with 'PATCH' and 'DELETE' respectively; update all callers of post() to use the new request()/post()/patch()/delete() helpers and ensure the signature and behavior (JSON body encoding, headers via getHeaders(), and error handling) remain identical in the function formerly named post().apps/frontend/src/types/item.ts (1)
4-5: Consider updating stale comments.The comments reference "Rust backend" (lines 5, 11-12), but this PR introduces a Hono/Node.js backend. Consider updating these comments to reflect the current architecture.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/src/types/item.ts` around lines 4 - 5, Update the stale header comments that mention the "Rust backend" to reference the current Hono/Node.js backend and the corresponding server-side types; specifically edit the comments around the Item and UUID type declarations (symbols: Item, UUID) so they no longer state they "Match the Rust backend" but instead reflect that they mirror the Hono/Node.js backend types/structures.apps/backend/package.json (1)
11-12: No security vulnerabilities found, though versions are outdated.Both
@supabase/supabase-js@2.57.4anddotenv@16.6.1have no known CVEs. However, they lag behind current releases: supabase-js has version 2.98.0 available (41 minor versions ahead), and dotenv has version 17.3.1 available (including a major version bump to 17.x). Consider updating to receive latest bug fixes, features, and improvements.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/package.json` around lines 11 - 12, Update the package versions for the two dependencies in package.json: bump "@supabase/supabase-js" from "2.57.4" to the latest 2.x release (e.g., 2.98.0) and bump "dotenv" from "16.6.1" to the current 17.x release (e.g., 17.3.1); after updating the entries for "@supabase/supabase-js" and "dotenv", run your package manager install and test the app to ensure no breaking changes (adjust imports or usage if dotenv v17 or newer changed APIs).
🤖 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/backend/src/index.ts`:
- Around line 481-492: The reset endpoint app.post('/api/debug/reset') currently
deletes all items and must be guarded by environment checks; before calling
createSupabaseWithToken() and performing the deletion, add an environment guard
that only allows this route in safe environments (e.g., process.env.NODE_ENV ===
'development' || process.env.NODE_ENV === 'test' or a dedicated config flag),
and return a 403/405 JSON response when invoked in production. Ensure the check
is done at the start of the route handler (before any DB operations) and keep
the existing behavior unchanged for allowed environments so
createSupabaseWithToken and the deletion logic only run when the guard permits
it.
In `@apps/frontend/src/api/honoClient.ts`:
- Around line 32-44: The getHeaders() method currently calls useUserStore()
which can run outside Vue's setup and throw "no active Pinia"; modify the Hono
client so auth is injected instead: add a token getter or accept a token
parameter on requests and remove useUserStore() from getHeaders(); update the
class to expose a setter like setTokenGetter(fn: () => string | undefined) or
change getHeaders(token?: string) to use the passed token, and ensure the
module-level honoClient singleton is initialized without calling Pinia so
callers set the token getter (or pass token) after Pinia is ready; search for
getHeaders(), useUserStore(), and the honoClient singleton to apply the change.
---
Nitpick comments:
In `@apps/backend/package.json`:
- Around line 11-12: Update the package versions for the two dependencies in
package.json: bump "@supabase/supabase-js" from "2.57.4" to the latest 2.x
release (e.g., 2.98.0) and bump "dotenv" from "16.6.1" to the current 17.x
release (e.g., 17.3.1); after updating the entries for "@supabase/supabase-js"
and "dotenv", run your package manager install and test the app to ensure no
breaking changes (adjust imports or usage if dotenv v17 or newer changed APIs).
In `@apps/backend/src/index.ts`:
- Around line 9-10: The two dotenv.config calls (dotenv.config({ path:
resolve(process.cwd(), '.env') }) and dotenv.config({ path:
resolve(process.cwd(), '../../.env') })) currently leave values from the first
call intact because dotenv does not overwrite existing env vars; if you want
root .env to override local .env, change the second call to pass override: true
(dotenv.config({ path: resolve(process.cwd(), '../../.env'), override: true })),
or if local should always win keep as-is — update the second dotenv.config
invocation accordingly to explicitly set override or remove the redundant load
to match the intended precedence.
- Around line 152-177: The login handler at app.post('/api/auth/login')
currently accepts credentials via parseJson and calls
createAnonSupabase.auth.signInWithPassword without any throttling; add rate
limiting to mitigate brute-force attempts by wrapping this route or adding
middleware that tracks attempts per IP or per account (e.g., using an in-memory
store, Redis, or a library) and rejects requests after a threshold with a 429
status before calling createAnonSupabase and fetchProfileUsername; ensure the
limiter checks occur before invoking signInWithPassword and that successful
logins reset the counter for that key.
- Around line 401-479: The /api/debug/seed route handler should be protected
from running in production: add an environment check at the start of the
app.post('/api/debug/seed', ...) handler (mirroring the reset endpoint) that
detects production (e.g. process.env.NODE_ENV === 'production' or your existing
isProduction/isProd helper) and immediately returns a non-descriptive 404/403 or
no-op response instead of performing the seed; keep the check before
createSupabaseWithToken and the upsert so no test data is created in production.
- Around line 297-305: The current POST forwarding in
app.post('/api/items/create', ...) recreates a Request and re-invokes middleware
(causing double auth and extra overhead); refactor by extracting the core logic
of the '/api/items' endpoint into a shared handler function (e.g., export async
function handleCreateItem(ctx) or createItemHandler) that contains the actual
create logic, then replace the existing '/api/items' route to call that handler
and change the '/api/items/create' route to call the same handler directly
(passing the original context/body) instead of using app.request; ensure the
shared handler accepts the same context or explicit params (req body, auth user)
so middleware runs only once and request construction is removed.
- Line 47: The current use of app.use('/api/*', cors()) allows all origins;
update the CORS setup to restrict origins in production by replacing that call
with a CORS middleware configured with an origin option (e.g., an allowlist
array) and conditional logic based on NODE_ENV (or a config flag); locate the
app.use('/api/*', cors()) line and implement a check that uses a scoped
allowlist (or a function to validate origin) and only permits those origins in
production while keeping the permissive cors() for development/testing.
In `@apps/frontend/src/api/honoClient.ts`:
- Around line 64-75: The post() method name is misleading because it accepts a
method parameter; rename post() to request() (or add a new request(path, body?,
method='POST') and keep post() as a thin wrapper) and add explicit helpers like
patch(path, body?) and delete(path) that call request() with 'PATCH' and
'DELETE' respectively; update all callers of post() to use the new
request()/post()/patch()/delete() helpers and ensure the signature and behavior
(JSON body encoding, headers via getHeaders(), and error handling) remain
identical in the function formerly named post().
In `@apps/frontend/src/types/item.ts`:
- Around line 4-5: Update the stale header comments that mention the "Rust
backend" to reference the current Hono/Node.js backend and the corresponding
server-side types; specifically edit the comments around the Item and UUID type
declarations (symbols: Item, UUID) so they no longer state they "Match the Rust
backend" but instead reflect that they mirror the Hono/Node.js backend
types/structures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d4c74e9f-e33d-48cb-a807-5dfa7f00acf9
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
apps/backend/package.jsonapps/backend/src/index.tsapps/frontend/src/api/authRepository.tsapps/frontend/src/api/honoClient.tsapps/frontend/src/stores/user.tsapps/frontend/src/types/item.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/backend/src/index.ts (2)
48-59: Development CORS is very permissive — verify this is intentional.In non-production mode, any origin is allowed (line 56 falls through to returning
originfor non-whitelisted values). While convenient for development, ensure this doesn't accidentally apply in staging environments whereNODE_ENVmight not be'production'.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/index.ts` around lines 48 - 59, The CORS setup in app.use('/api/*', cors(...)) uses allowedOrigins and currently returns the incoming origin for non-production, effectively allowing any origin in development which may be dangerous for staging; change the logic so non-production environments only allow origins from an explicit whitelist (e.g., extend allowedOrigins from an env var like CORS_ALLOWED_ORIGINS or add a new CORS_ALLOW_ALL flag) and modify the origin callback to reject non-whitelisted origins instead of falling back to returning origin; update the origin function and allowedOrigins handling to reference allowedOrigins and process.env.CORS_ALLOW_ALL (or similar) so staging can enforce a whitelist while still allowing an explicit dev override.
548-569: Command aliases work correctly but trigger auth middleware twice.The internal
app.request()approach for Tauri command compatibility functions correctly. Note that auth validation runs twice: once for/api/commands/*and again for/api/items/*. This is acceptable overhead for the abstraction benefit but could be optimized by calling handlers directly if performance becomes a concern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/index.ts` around lines 548 - 569, Command alias routes (app.get('/api/commands/...') and app.post('/api/commands/create_item')) use app.request which causes auth middleware to run twice; replace app.request calls by invoking the existing item route handlers directly (the handlers for '/api/items/active', '/api/items/archived', '/api/items/deleted', and '/api/items/create') or by extracting the core handler logic into callable functions and calling those from both the '/api/items/*' routes and the '/api/commands/*' aliases, making sure to pass the original context or propagate authenticated user/state (c or c.state) and the same request body (use await c.req.raw.text() for POST) so you don't re-run authentication middleware.apps/frontend/src/api/config.ts (1)
41-49: Consider memoizinggetApiMode()if called frequently.The helper functions
isTauriMode()andisHonoMode()callgetApiMode()each time, which re-runs the detection logic. If these are called in hot paths (e.g., per-request), consider caching the result:♻️ Optional: Memoization pattern
+let cachedMode: ApiMode | null = null; + export function getApiMode(): ApiMode { + if (cachedMode !== null) return cachedMode; + const hasWindow = typeof window !== 'undefined'; const runtime = hasWindow ? (window as any) : undefined; const isTauriRuntime = !!runtime?.__TAURI__ || !!runtime?.__TAURI_INTERNALS__; // Never fall back to Hono when running inside Tauri. if (isTauriRuntime) { - return 'tauri'; + cachedMode = 'tauri'; + return cachedMode; } // Environment override applies outside Tauri runtime. if (import.meta.env.VITE_API_MODE === 'tauri') { - return 'tauri'; + cachedMode = 'tauri'; + return cachedMode; } if (import.meta.env.VITE_API_MODE === 'hono') { - return 'hono'; + cachedMode = 'hono'; + return cachedMode; } // Default to Hono for browser environments - return 'hono'; + cachedMode = 'hono'; + return cachedMode; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/src/api/config.ts` around lines 41 - 49, The helpers isTauriMode() and isHonoMode() call getApiMode() on every invocation causing repeated detection work; cache the detected mode in a module-level variable (e.g., cachedApiMode) and have getApiMode() return the cached value after the first computation (or expose a getApiModeCached() used by isTauriMode/isHonoMode) so subsequent calls to isTauriMode()/isHonoMode() are cheap; update references to use the cached getter and ensure initialization runs lazily on first call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/backend/src/index.ts`:
- Around line 48-59: The CORS setup in app.use('/api/*', cors(...)) uses
allowedOrigins and currently returns the incoming origin for non-production,
effectively allowing any origin in development which may be dangerous for
staging; change the logic so non-production environments only allow origins from
an explicit whitelist (e.g., extend allowedOrigins from an env var like
CORS_ALLOWED_ORIGINS or add a new CORS_ALLOW_ALL flag) and modify the origin
callback to reject non-whitelisted origins instead of falling back to returning
origin; update the origin function and allowedOrigins handling to reference
allowedOrigins and process.env.CORS_ALLOW_ALL (or similar) so staging can
enforce a whitelist while still allowing an explicit dev override.
- Around line 548-569: Command alias routes (app.get('/api/commands/...') and
app.post('/api/commands/create_item')) use app.request which causes auth
middleware to run twice; replace app.request calls by invoking the existing item
route handlers directly (the handlers for '/api/items/active',
'/api/items/archived', '/api/items/deleted', and '/api/items/create') or by
extracting the core handler logic into callable functions and calling those from
both the '/api/items/*' routes and the '/api/commands/*' aliases, making sure to
pass the original context or propagate authenticated user/state (c or c.state)
and the same request body (use await c.req.raw.text() for POST) so you don't
re-run authentication middleware.
In `@apps/frontend/src/api/config.ts`:
- Around line 41-49: The helpers isTauriMode() and isHonoMode() call
getApiMode() on every invocation causing repeated detection work; cache the
detected mode in a module-level variable (e.g., cachedApiMode) and have
getApiMode() return the cached value after the first computation (or expose a
getApiModeCached() used by isTauriMode/isHonoMode) so subsequent calls to
isTauriMode()/isHonoMode() are cheap; update references to use the cached getter
and ensure initialization runs lazily on first call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb3940aa-b3e9-48bf-8a71-34c20c336a29
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
apps/backend/package.jsonapps/backend/src/index.tsapps/desktop/src-tauri/tauri.conf.jsonapps/frontend/src/api/config.tsapps/frontend/src/api/honoClient.tsapps/frontend/src/main.tsapps/frontend/src/types/item.tsapps/frontend/src/vite-env.d.tsapps/frontend/tsconfig.app.json
✅ Files skipped from review due to trivial changes (1)
- apps/frontend/src/vite-env.d.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/backend/package.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
apps/backend/src/index.ts (1)
132-138: Consider merging parsed JSON with fallback defaults.The current
parseJsonimplementation only uses the fallback when JSON parsing fails completely. This forces callers to defensively check every property. A merge pattern would be more robust.♻️ Proposed refactor to merge with defaults
-async function parseJson<T>(c: Context, fallback: T): Promise<T> { +async function parseJson<T extends Record<string, unknown>>(c: Context, fallback: T): Promise<T> { try { - return await c.req.json<T>(); + const parsed = await c.req.json<Partial<T>>(); + return { ...fallback, ...parsed }; } catch { return fallback; } }This ensures all expected properties have default values, preventing runtime errors from missing fields.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/index.ts` around lines 132 - 138, The parseJson function returns only the parsed body or the fallback on parse error, which forces callers to validate each property; change parseJson<T>(c: Context, fallback: T) to parse the request body and merge it with the fallback defaults (for example shallow-merge when both fallback and parsed value are objects: return { ...fallback, ...(parsed as any) }), still returning fallback if parsing fails or parsed is not an object, and preserve the generic return type; update the implementation in the parseJson function so consumers receive a result with default fields filled in.apps/frontend/src/api/config.ts (1)
34-36: Clarify API semantics by renaming backend mode helpers.The function names
isTauriMode()andisHonoMode()are misleading. Lines 34–36 allow the environment variable to force'tauri'mode even in a plain browser, so these helpers don't reliably indicate whether the Tauri runtime exists—they only indicate backend selection. Consider renaming tousesTauriBackend()andusesHonoBackend()to clarify that they reflect backend configuration, not runtime detection. (Note: These helpers are currently unused in the codebase; all callers directly usegetApiMode().)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/src/api/config.ts` around lines 34 - 36, Rename the misleading runtime-detection helpers isTauriMode() and isHonoMode() to descriptive backend-selection names (e.g., usesTauriBackend() and usesHonoBackend()) because getApiMode()/cachedApiMode and import.meta.env.VITE_API_MODE can force the 'tauri' mode even in a plain browser; update all references to these functions accordingly and keep their logic using getApiMode() so they reflect backend configuration rather than runtime presence (also update any JSDoc/comments referencing runtime detection).
🤖 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/backend/src/index.ts`:
- Line 444: The two sync endpoints return different shapes (/api/items/sync
returns { count: 0 } while /api/commands/sync_items returns a bare number),
causing inconsistent client parsing; update the /api/commands/sync_items handler
to return a JSON object with the same shape as /api/items/sync (e.g., c.json({
count: <number> })) or vice‑versa if you prefer the raw number—make the change
in the route handler for "/api/commands/sync_items" (and any helper function it
calls) and update any related tests or callers to expect the unified { count: n
} response.
- Around line 217-236: In handleCreateItem, calling body.title.trim() can throw
when body.title is undefined because parseJson doesn't merge defaults; change
the logic to defensively obtain a string title before trimming (e.g., treat
non-strings/undefined as empty string), e.g. derive title from body.title only
after verifying it's a string or coalescing to '' and then call .trim(); keep
the subsequent validation that returns c.json({ error: 'title is required' },
400) when the trimmed title is empty. Also update the related
durationMinutes/motivation parsing to similarly guard types if needed.
- Around line 320-327: In the POST /api/auth/login handler, avoid calling
body.email.trim() directly because body.email can be undefined; instead coalesce
or validate body.email before trimming (e.g., treat missing/non-string email as
empty string or explicitly check typeof body.email === 'string') so trim() is
only called on a string; update the handler in apps/backend/src/index.ts (the
app.post('/api/auth/login') block) similarly to how handleCreateItem was made
defensive and ensure parseJson usage remains consistent.
In `@apps/frontend/src/api/config.ts`:
- Around line 33-45: The code silently coerces any unknown
import.meta.env.VITE_API_MODE into 'hono' by falling through to the default;
instead validate VITE_API_MODE and fail loudly on invalid values: read
import.meta.env.VITE_API_MODE, check allowed values ('tauri'|'hono'), set
cachedApiMode accordingly, and if the value is present but invalid throw or log
a clear error before returning to avoid accidental silent switches; update the
logic around cachedApiMode and the conditional branches to perform explicit
validation of VITE_API_MODE.
---
Nitpick comments:
In `@apps/backend/src/index.ts`:
- Around line 132-138: The parseJson function returns only the parsed body or
the fallback on parse error, which forces callers to validate each property;
change parseJson<T>(c: Context, fallback: T) to parse the request body and merge
it with the fallback defaults (for example shallow-merge when both fallback and
parsed value are objects: return { ...fallback, ...(parsed as any) }), still
returning fallback if parsing fails or parsed is not an object, and preserve the
generic return type; update the implementation in the parseJson function so
consumers receive a result with default fields filled in.
In `@apps/frontend/src/api/config.ts`:
- Around line 34-36: Rename the misleading runtime-detection helpers
isTauriMode() and isHonoMode() to descriptive backend-selection names (e.g.,
usesTauriBackend() and usesHonoBackend()) because getApiMode()/cachedApiMode and
import.meta.env.VITE_API_MODE can force the 'tauri' mode even in a plain
browser; update all references to these functions accordingly and keep their
logic using getApiMode() so they reflect backend configuration rather than
runtime presence (also update any JSDoc/comments referencing runtime detection).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 14075f35-d15d-4308-8d39-c0063adb05c3
📒 Files selected for processing (2)
apps/backend/src/index.tsapps/frontend/src/api/config.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/backend/src/index.ts (2)
233-236:⚠️ Potential issue | 🔴 CriticalGuard
title.trim()against non-string payloads.The nullish coalescing only fixed the
undefinedcase. A valid JSON body like{ "title": 123 }still turns this into a 500 becausetrim()is called on a number. Narrowtitleto a string first so bad payloads hit the existing400path instead.🐛 Minimal fix
- const title = (body.title ?? '').trim(); + const title = typeof body.title === 'string' ? body.title.trim() : '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/index.ts` around lines 233 - 236, The current line computing const title = (body.title ?? '').trim() calls trim() on non-strings (e.g. numbers) and can throw; change it to first narrow or check body.title is a string before trimming so invalid types fall through to the existing 400 validation. Locate the title assignment in index.ts (the const title line that uses body.title) and replace it with a safe check like: if typeof body.title === 'string' then trim, otherwise treat as empty/invalid so the request hits the 400 error path.
325-327:⚠️ Potential issue | 🔴 CriticalNarrow auth fields to strings before trimming/passing them on.
Same issue here:
{ "email": 123 }still throws ontrim(), and non-stringpasswordvalues bypass the intended validation. Coerce both fields to strings first so invalid payloads return400instead of failing at runtime.🐛 Minimal fix
- const email = (body.email ?? '').trim(); - const password = body.password ?? ''; + const email = typeof body.email === 'string' ? body.email.trim() : ''; + const password = typeof body.password === 'string' ? body.password : '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/index.ts` around lines 325 - 327, The email/password extraction currently assumes strings and calls trim() directly; coerce both fields to strings first to avoid runtime errors and ensure invalid types are handled as bad requests: change how you derive email and password from parseJson's result (the variables email and password) by converting body.email and body.password to String() (or using '' + value) before trimming/using them so non-string inputs like numbers produce a 400 via your existing validation rather than throwing.
🤖 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/backend/src/index.ts`:
- Around line 124-129: The parseStatus helper currently calls toLowerCase on
non-strings and silently maps typos to 'backlog'; change parseStatus to validate
type and only accept the explicit values ('todo', 'inprogress', 'done') and
return null (or undefined) for any invalid input instead of defaulting to
'backlog', and ensure it does not call toLowerCase unless raw is a string; then
update the status-update route callers to call parseStatus(body.status), check
if the result is falsy (e.g. if (!status)) and return a 400 JSON error like {
error: 'invalid status' } instead of mutating the item.
---
Duplicate comments:
In `@apps/backend/src/index.ts`:
- Around line 233-236: The current line computing const title = (body.title ??
'').trim() calls trim() on non-strings (e.g. numbers) and can throw; change it
to first narrow or check body.title is a string before trimming so invalid types
fall through to the existing 400 validation. Locate the title assignment in
index.ts (the const title line that uses body.title) and replace it with a safe
check like: if typeof body.title === 'string' then trim, otherwise treat as
empty/invalid so the request hits the 400 error path.
- Around line 325-327: The email/password extraction currently assumes strings
and calls trim() directly; coerce both fields to strings first to avoid runtime
errors and ensure invalid types are handled as bad requests: change how you
derive email and password from parseJson's result (the variables email and
password) by converting body.email and body.password to String() (or using '' +
value) before trimming/using them so non-string inputs like numbers produce a
400 via your existing validation rather than throwing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91d39275-551a-4177-a9fe-f57c2dbfa28f
📒 Files selected for processing (2)
apps/backend/src/index.tsapps/frontend/src/api/config.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/backend/src/index.ts`:
- Around line 235-238: Validate input types on the parsed request body before
calling .trim() or coercing numbers: ensure body.title is a string before
trimming (if not, return a 400), ensure body.motivation is a finite number or a
numeric string that parses to a finite number before assigning to motivation
(reject otherwise), and ensure body.duration_minutes / body.durationMinutes are
numbers or numeric strings that parse to a finite integer for durationMinutes
(reject otherwise); also validate body.due is either null/undefined or a valid
date/string you can parse (reject malformed dates). Locate the code that
computes title, motivation, due, and durationMinutes in the request handler (the
variables title, motivation, due, durationMinutes) and add typeof checks and
Number.isFinite (or equivalent parsing+validation) with early 400 responses on
invalid types instead of blindly trimming/coercing.
- Around line 337-338: The code currently does const email = (body.email ??
'').trim(); and const password = body.password ?? ''; which still allows
non-string inputs (e.g. body.email = {}) to throw and lets non-string password
reach Supabase; update the request handling to first assert types: check typeof
body.email === 'string' and typeof body.password === 'string' before calling
trim or passing them on, and if either is not a string return the existing 400
error path; specifically modify the logic around the email and password
variables (the email/password extraction block) to reject non-strings early
rather than coercing or using them directly.
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.
close #39
Summary by CodeRabbit
New Features
Improvements
Desktop
Type/Build