Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a new Vue TaskDrawer component (create/view/edit) with full CRUD wiring to useItems, enhances TaskList with selection/edit emits and keyboard accessibility, updates frontend item API/repository/composable types and methods, and adds backend consolidated item update/archive/delete handlers and CORS adjustments. Changes
Sequence DiagramsequenceDiagram
participant Parent as Parent Component
participant TaskDrawer as TaskDrawer (UI)
participant TaskList as TaskList (UI)
participant useItems as useItems (composable)
participant Backend as Backend/API
Parent->>TaskDrawer: set `open`, `mode`, `selectedItem`, `items`
TaskDrawer->>TaskList: render list (syncMap, errorMap)
TaskList->>TaskDrawer: emit `select-item` / `edit-item`
alt Create flow
TaskDrawer->>TaskDrawer: initialize create form
TaskDrawer->>useItems: createItem(newPayload)
useItems->>Backend: POST /api/items
Backend-->>useItems: created item
useItems-->>TaskDrawer: created item
TaskDrawer->>Parent: emit selected item, set mode=view
end
alt Edit flow
TaskDrawer->>TaskDrawer: hydrate edit form from selectedItem
TaskDrawer->>useItems: updateItem(updatePayload)
useItems->>Backend: PATCH /api/items/{id}
Backend-->>useItems: success
useItems-->>TaskDrawer: refreshed items
TaskDrawer->>Parent: emit updated item, set mode=view
end
alt Delete / Archive
TaskDrawer->>useItems: deleteItem(id) / archiveItem(id)
useItems->>Backend: DELETE or archive command
Backend-->>useItems: success
useItems-->>TaskDrawer: items refreshed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/frontend/src/components/TaskDrawer.vue`:
- Line 35: editMotivation is initialized to 5 which forces null motivations to
become 5 during edit; change editMotivation to allow null (e.g.,
ref<number|null>(null)), set it from the item’s existing motivation when
entering edit mode (assign editMotivation.value = currentItem.motivation which
may be null in your open/edit handler), and in the save handler only update the
item’s motivation when the user actually changed it (compare originalMotivation
!== editMotivation.value or update only when editMotivation.value !== null
depending on intended semantics) so opening the editor won’t mutate null
motivations; update usages in the declaration (editMotivation), the edit/open
function that initializes it, and the save/submit function that writes it back.
- Line 28: The numeric inputs bound with v-model.number (createDuration and
editDuration) can become "" when cleared and must be normalized to null before
sending as durationMinutes to the API; update the submit handlers (where
durationMinutes is assembled for the create and edit API calls) to coerce "" to
null (e.g., if createDuration === "" or editDuration === "" then set
durationMinutes = null, else keep the numeric value), or replace the v-model
bindings with Vue computed adapters that map "" to null so all payloads always
send number | null.
In `@apps/frontend/src/components/TaskList.vue`:
- Line 30: The task rows rendered by the v-for (items) use a clickable div with
class "task-row" and onclick handler handleSelectItem(item) but are not
keyboard-accessible; make each row focusable and operable by keyboard by giving
the element a tabindex (e.g., tabindex="0" or making it a semantic
button/anchor), add an appropriate ARIA role if you keep the div
(role="button"), and attach a keydown handler that invokes
handleSelectItem(item) when Enter or Space is pressed so keyboard users can
select rows the same way as mouse users.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 275e941c-fe09-4350-8ccb-b0a5ea680646
📒 Files selected for processing (2)
apps/frontend/src/components/TaskDrawer.vueapps/frontend/src/components/TaskList.vue
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/backend/src/index.ts (2)
374-412: Consider returning 404 when item not found.Both handlers return 204 regardless of whether a row was actually modified. If the item doesn't exist or is already deleted, the client receives a success response.
This is acceptable for idempotent operations but may mask issues during debugging.
♻️ Optional: Check affected rows
- const { error } = await supabase + const { error, count } = await supabase .from('items') .update({ is_archived: true, updated_at: new Date().toISOString() }) .eq('id', id) - .is('deleted_at', null); + .is('deleted_at', null) + .select('id', { count: 'exact', head: true }); if (error) return c.json({ error: error.message }, 400); + if (count === 0) return c.json({ error: 'Item not found' }, 404); return c.body(null, 204);🤖 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 374 - 412, Both handlers always return 204 even when no row was modified; update the logic in handleArchiveItem and handleSoftDeleteItem to check the Supabase response data (the returned rows) in addition to error: after the await supabase...update(...) call inspect the returned data (e.g., data or row array) and if it's empty or no rows were affected return c.json({ error: 'not found' }, 404) instead of 204; otherwise proceed to return 204. Ensure you check the same query that updates is_archived or deleted_at/updated_at so you only return 204 when a row was actually changed.
138-140: Consider supporting UUID v7 for future-proofing.The regex validates UUID versions 1-5, which works with
crypto.randomUUID()(v4). However, UUID v7 (time-ordered UUIDs) is gaining adoption. If the system ever migrates to v7, this validation would reject valid UUIDs.♻️ Optional: Support all RFC 9562 versions
function isUuid(value: string): boolean { - return /^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i.test(value); + return /^[0-9a-f]{8}-[0-9a-f]{4}-[1-7][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i.test(value); }🤖 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 138 - 140, The isUuid function currently only accepts UUID versions 1–5; update its validation to also accept UUID v7 by changing the version character class in the regex (e.g., replace [1-5] with [1-7] or otherwise allow '7') so valid v7 strings from crypto.randomUUID() successors are accepted; ensure you update the regex used in isUuid and keep the rest of the pattern and the /i flag intact.
🤖 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 291-293: Add the same UUID validation used elsewhere: call
isUuid(id) before accepting the request (like in
handleUpdateItem/handleArchiveItem/handleSoftDeleteItem) and return a 400 JSON
error if it fails; specifically, in the handler that currently checks if (!id ||
!body.status) add a check if (!isUuid(id)) return c.json({ error: 'invalid id
format' }, 400) so ID format is consistently validated across endpoints.
In `@apps/frontend/src/api/honoClient.ts`:
- Around line 3-23: The current resolveHonoBaseUrl function falls back to a
hardcoded DEFAULT_RENDER_BASE_URL which can leak Authorization headers; remove
that silent remote fallback and require an explicit base URL or env variable
instead—modify resolveHonoBaseUrl (and remove or stop using
DEFAULT_RENDER_BASE_URL) so that if neither explicitBaseUrl nor
VITE_HONO_BASE_URL is set and the host is not localhost/127.0.0.1 it returns an
empty string or throws an explicit error, so callers (e.g., where
getHeaders/requests are assembled) must handle the missing base URL rather than
sending tokens to the hardcoded Render host.
---
Nitpick comments:
In `@apps/backend/src/index.ts`:
- Around line 374-412: Both handlers always return 204 even when no row was
modified; update the logic in handleArchiveItem and handleSoftDeleteItem to
check the Supabase response data (the returned rows) in addition to error: after
the await supabase...update(...) call inspect the returned data (e.g., data or
row array) and if it's empty or no rows were affected return c.json({ error:
'not found' }, 404) instead of 204; otherwise proceed to return 204. Ensure you
check the same query that updates is_archived or deleted_at/updated_at so you
only return 204 when a row was actually changed.
- Around line 138-140: The isUuid function currently only accepts UUID versions
1–5; update its validation to also accept UUID v7 by changing the version
character class in the regex (e.g., replace [1-5] with [1-7] or otherwise allow
'7') so valid v7 strings from crypto.randomUUID() successors are accepted;
ensure you update the regex used in isUuid and keep the rest of the pattern and
the /i flag intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 869f9dfc-52b4-498e-b578-0b25ed4f6412
📒 Files selected for processing (9)
.env.exampleapps/backend/src/index.tsapps/frontend/ARCHITECTURE.mdapps/frontend/README.mdapps/frontend/src/api/honoClient.tsapps/frontend/src/api/itemRepository.tsapps/frontend/src/components/TaskDrawer.vueapps/frontend/src/components/TaskList.vueapps/frontend/src/composables/useItems.ts
✅ Files skipped from review due to trivial changes (1)
- apps/frontend/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/frontend/src/components/TaskList.vue
Comment on lines
+291
to
293
| if (!id || !body.status) { | ||
| return c.json({ error: 'id and status are required' }, 400); | ||
| } |
Contributor
There was a problem hiding this comment.
Missing UUID format validation.
Other handlers (handleUpdateItem, handleArchiveItem, handleSoftDeleteItem) validate the ID with isUuid(id), but this handler does not. This creates an inconsistent validation approach across endpoints.
🔧 Proposed fix to add UUID validation
const id = typeof idFromPath === 'string' && idFromPath.trim().length > 0 ? idFromPath : body.id;
- if (!id || !body.status) {
- return c.json({ error: 'id and status are required' }, 400);
+ if (!id || !isUuid(id)) {
+ return c.json({ error: 'valid item id is required' }, 400);
+ }
+ if (!body.status) {
+ return c.json({ error: 'status is required' }, 400);
}🤖 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 291 - 293, Add the same UUID
validation used elsewhere: call isUuid(id) before accepting the request (like in
handleUpdateItem/handleArchiveItem/handleSoftDeleteItem) and return a 400 JSON
error if it fails; specifically, in the handler that currently checks if (!id ||
!body.status) add a check if (!isUuid(id)) return c.json({ error: 'invalid id
format' }, 400) so ID format is consistently validated across endpoints.
Comment on lines
+3
to
+23
| const DEFAULT_RENDER_BASE_URL = 'https://taskgraph-100program9-server.onrender.com'; | ||
|
|
||
| function resolveHonoBaseUrl(explicitBaseUrl?: string): string { | ||
| if (explicitBaseUrl && explicitBaseUrl.trim().length > 0) { | ||
| return explicitBaseUrl.trim().replace(/\/+$/, ''); | ||
| } | ||
|
|
||
| const envBaseUrl = (import.meta.env.VITE_HONO_BASE_URL as string | undefined)?.trim(); | ||
| if (envBaseUrl && envBaseUrl.length > 0) { | ||
| return envBaseUrl.replace(/\/+$/, ''); | ||
| } | ||
|
|
||
| if (typeof window !== 'undefined') { | ||
| const host = window.location.hostname.toLowerCase(); | ||
| if (host === 'localhost' || host === '127.0.0.1') { | ||
| return 'http://localhost:10000'; | ||
| } | ||
| } | ||
|
|
||
| return DEFAULT_RENDER_BASE_URL; | ||
| } |
Contributor
There was a problem hiding this comment.
Remove hardcoded remote fallback to prevent accidental data/token leakage.
If VITE_HONO_BASE_URL is unset outside localhost, requests (with Authorization from getHeaders) are silently sent to the hardcoded Render host. This can leak user data/tokens to the wrong backend in misconfigured deployments.
Safer resolver approach
-const DEFAULT_RENDER_BASE_URL = 'https://taskgraph-100program9-server.onrender.com';
-
function resolveHonoBaseUrl(explicitBaseUrl?: string): string {
if (explicitBaseUrl && explicitBaseUrl.trim().length > 0) {
return explicitBaseUrl.trim().replace(/\/+$/, '');
}
const envBaseUrl = (import.meta.env.VITE_HONO_BASE_URL as string | undefined)?.trim();
if (envBaseUrl && envBaseUrl.length > 0) {
return envBaseUrl.replace(/\/+$/, '');
}
if (typeof window !== 'undefined') {
const host = window.location.hostname.toLowerCase();
if (host === 'localhost' || host === '127.0.0.1') {
return 'http://localhost:10000';
}
}
- return DEFAULT_RENDER_BASE_URL;
+ throw new Error('Missing VITE_HONO_BASE_URL for non-local environment.');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const DEFAULT_RENDER_BASE_URL = 'https://taskgraph-100program9-server.onrender.com'; | |
| function resolveHonoBaseUrl(explicitBaseUrl?: string): string { | |
| if (explicitBaseUrl && explicitBaseUrl.trim().length > 0) { | |
| return explicitBaseUrl.trim().replace(/\/+$/, ''); | |
| } | |
| const envBaseUrl = (import.meta.env.VITE_HONO_BASE_URL as string | undefined)?.trim(); | |
| if (envBaseUrl && envBaseUrl.length > 0) { | |
| return envBaseUrl.replace(/\/+$/, ''); | |
| } | |
| if (typeof window !== 'undefined') { | |
| const host = window.location.hostname.toLowerCase(); | |
| if (host === 'localhost' || host === '127.0.0.1') { | |
| return 'http://localhost:10000'; | |
| } | |
| } | |
| return DEFAULT_RENDER_BASE_URL; | |
| } | |
| function resolveHonoBaseUrl(explicitBaseUrl?: string): string { | |
| if (explicitBaseUrl && explicitBaseUrl.trim().length > 0) { | |
| return explicitBaseUrl.trim().replace(/\/+$/, ''); | |
| } | |
| const envBaseUrl = (import.meta.env.VITE_HONO_BASE_URL as string | undefined)?.trim(); | |
| if (envBaseUrl && envBaseUrl.length > 0) { | |
| return envBaseUrl.replace(/\/+$/, ''); | |
| } | |
| if (typeof window !== 'undefined') { | |
| const host = window.location.hostname.toLowerCase(); | |
| if (host === 'localhost' || host === '127.0.0.1') { | |
| return 'http://localhost:10000'; | |
| } | |
| } | |
| throw new Error('Missing VITE_HONO_BASE_URL for non-local environment.'); | |
| } |
🤖 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 3 - 23, The current
resolveHonoBaseUrl function falls back to a hardcoded DEFAULT_RENDER_BASE_URL
which can leak Authorization headers; remove that silent remote fallback and
require an explicit base URL or env variable instead—modify resolveHonoBaseUrl
(and remove or stop using DEFAULT_RENDER_BASE_URL) so that if neither
explicitBaseUrl nor VITE_HONO_BASE_URL is set and the host is not
localhost/127.0.0.1 it returns an empty string or throws an explicit error, so
callers (e.g., where getHeaders/requests are assembled) must handle the missing
base URL rather than sending tokens to the hardcoded Render host.
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.
Summary by CodeRabbit
New Features
Improvements
Documentation