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 backend unarchive/restore handlers/routes and updates item mutations to set Changes
Sequence Diagram(s)sequenceDiagram
participant FE as Frontend (TaskDrawer / useItems)
participant Repo as ItemRepository (Hono / Tauri)
participant Server as Backend (Hono handlers)
participant DB as Database (Postgres)
participant Guest as Guest Store (guestLocalItems)
FE->>Repo: unarchiveItem(id)
alt Non-Guest (Hono/Tauri)
Repo->>Server: POST /api/items/:id/unarchive
Server->>DB: UPDATE items SET is_archived=false, updated_at=now(), sync_status='synced' WHERE id=... AND deleted_at IS NULL
DB-->>Server: result (row/404)
Server-->>Repo: success / 404
Repo-->>FE: resolve / throw
FE->>FE: fetchActiveItems / refresh active projection
else Guest mode
Repo->>Guest: mutate guestLocalItems (is_archived=false, sync_status='local_only', updated_at=...)
Guest-->>Repo: success
Repo-->>FE: resolve
FE->>FE: refreshGuestActiveProjection
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/backend/src/index.ts (1)
436-447:⚠️ Potential issue | 🟠 MajorRestrict
restoreto rows that are actually soft-deleted.Right now
restorewill also clearis_archivedon an archived-but-not-deleted row, which effectively makes/restoreanother unarchive endpoint. That breaks the API contract documented for soft-deleted items.Suggested fix
Apply the same guard to both restore implementations:
- const { error } = await supabase + const now = new Date().toISOString(); + const { data, error } = await supabase .from('items') .update({ deleted_at: null, is_archived: false, - updated_at: new Date().toISOString(), + updated_at: now, sync_status: 'synced', }) .eq('id', id) + .not('deleted_at', 'is', null) + .select('id') + .maybeSingle(); if (error) return c.json({ error: error.message }, 400); + if (!data) return c.json({ error: 'Item is not deleted' }, 409);Also applies to: 657-668
🤖 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 436 - 447, The restore endpoint currently updates items regardless of deletion state and inadvertently clears is_archived on non-deleted rows; modify the Supabase update queries used in the restore handlers (the .from('items')...update(...) calls that end with .eq('id', body.id)) to add a condition that only targets rows where deleted_at IS NOT NULL (i.e., restrict the update to soft-deleted rows), and apply the same change to the other restore implementation referenced in the file so both handlers only affect actually soft-deleted items.
🧹 Nitpick comments (3)
apps/frontend/src/components/ArchivedList.vue (1)
1-26: Consider extracting the shared archived/deleted list UI.
ArchivedList.vueandDeletedList.vueare almost identical apart from the emitted event and button label. Pulling the common row/layout into a small shared component will make future styling or badge-state fixes land in one place instead of two.Also applies to: 28-213
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/src/components/ArchivedList.vue` around lines 1 - 26, ArchivedList.vue and DeletedList.vue duplicate the same row/layout logic (props like items, syncMap, errorMap, isSyncing, isProcessing, use of SyncStatusBadge and displayStatus) differing only by the emitted event name ('unarchive-item' vs 'restore-item') and the action button label; extract the shared row into a new small presentational component (e.g., ItemListRow or ArchivedDeletedRow) that accepts the item, sync/error maps, status label function, an actionLabel prop, and an actionEvent or actionHandler prop; update ArchivedList.vue and DeletedList.vue to render the new component for each item, passing through props and binding the appropriate event name/handler (emit 'unarchive-item' or 'restore-item' from the parent when the child signals action) so styling/logic lives in one place and only the label/event differ.apps/frontend/ARCHITECTURE.md (1)
275-275: Document the inline status mutation path too.
apps/frontend/src/components/TaskDrawer.vuealso callsupdateItemStatus()viahandleStatusChange(), but this section only enumerates the create/edit/archive/delete/unarchive/restore flows. Adding that direct status-update path here would keep the architecture note aligned with the component's actual responsibilities.Also applies to: 295-295
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/ARCHITECTURE.md` at line 275, The architecture note omits the direct status-update flow present in components: update the paragraph describing "Self-contained CRUD" to also mention status mutations invoked inline — specifically note that TaskDrawer.vue calls updateItemStatus() via handleStatusChange() (using useItems() directly) in addition to the listed createItem/updateItem/archiveItem/unarchiveItem/softDeleteItem/restoreItem paths so the document reflects the component's actual responsibilities.apps/frontend/src/components/TaskDrawer.vue (1)
503-534: Complete the ARIA tab pattern or keep these as plain buttons.Once we opt into
role="tablist"/role="tab", assistive tech expects matchingtabpanels plusaria-controls/aria-labelledbyand focus management. The UI is usable visually, but the current semantics are incomplete and can be misannounced. Please verify with a keyboard/screen-reader pass after wiring this up.Also applies to: 631-663
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/src/components/TaskDrawer.vue` around lines 503 - 534, The tab markup uses role="tablist" and role="tab" but lacks matching tabpanel elements, aria-controls/aria-labelledby, and keyboard focus handling; update the TaskDrawer tab implementation (nav with role="tablist", buttons that call setCurrentTab and use currentTab) to implement a full ARIA tabs pattern: give each tab button a unique id and aria-controls pointing to the corresponding tabpanel id, render matching elements (or a container component) with role="tabpanel" and aria-labelledby pointing back to the tab id, and add keyboard handling (left/right arrows, Home/End, and Enter/Space activation) so focus moves between tabs while setCurrentTab still updates currentTab; ensure ids are unique and test with keyboard and a screen reader to verify expected announcements and focus behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@API_SMOKE_TEST_VERIFICATION.md`:
- Around line 385-401: The verification doc uses stale hardcoded line numbers
for the detail-view buttons; update API_SMOKE_TEST_VERIFICATION.md to remove the
line-range reference and instead point to the component and symbols
(TaskDrawer.vue and the buttons wired to handleArchive and handleDelete, or a
permalink to the file/commit), so future edits won't break the reference;
replace "TaskDrawer.vue Line 723-729" with "TaskDrawer.vue — danger-zone buttons
(handleArchive, handleDelete)" or a stable permalink to the exact file/commit.
In `@apps/frontend/src/components/TaskDrawer.vue`:
- Around line 224-240: loadTaskTabItems currently uses a single isLoadingTaskTab
flag and only logs errors, which lets stale responses clear loading state and
first-load failures fall through to drawerNoTasksInTab; fix by giving each tab
its own loading and error state (e.g., isLoadingArchived, isLoadingDeleted and
errorArchived, errorDeleted), or by using a per-request token/sequence/id inside
loadTaskTabItems to ignore out-of-order responses before updating
archivedItems/deletedItems, and ensure you only clear the matching tab's loading
flag and set its error state on catch so drawerNoTasksInTab can use those
per-tab flags to decide whether to show empty state vs loading/error; apply the
same pattern where similar single-flag logic exists (lines noted around the
other occurrences).
In `@apps/frontend/src/composables/useItems.ts`:
- Around line 522-524: The unarchive/restore branches only remove/filter the
local items array so a restored item that isn't currently present in items.value
won't appear; after calling itemRepository.unarchiveItem(id) (and the equivalent
restore call around lines 577-579) re-load the active projection into the shared
state instead of just filtering: call the repository method that returns the
active list (e.g. itemRepository.getActiveItems() or
itemRepository.fetchActiveItems()) and assign its result to items.value, or
invoke the existing shared refresh function (e.g.
loadActiveItems()/refreshItems()) so MainView.vue sees the restored item
immediately.
---
Outside diff comments:
In `@apps/backend/src/index.ts`:
- Around line 436-447: The restore endpoint currently updates items regardless
of deletion state and inadvertently clears is_archived on non-deleted rows;
modify the Supabase update queries used in the restore handlers (the
.from('items')...update(...) calls that end with .eq('id', body.id)) to add a
condition that only targets rows where deleted_at IS NOT NULL (i.e., restrict
the update to soft-deleted rows), and apply the same change to the other restore
implementation referenced in the file so both handlers only affect actually
soft-deleted items.
---
Nitpick comments:
In `@apps/frontend/ARCHITECTURE.md`:
- Line 275: The architecture note omits the direct status-update flow present in
components: update the paragraph describing "Self-contained CRUD" to also
mention status mutations invoked inline — specifically note that TaskDrawer.vue
calls updateItemStatus() via handleStatusChange() (using useItems() directly) in
addition to the listed
createItem/updateItem/archiveItem/unarchiveItem/softDeleteItem/restoreItem paths
so the document reflects the component's actual responsibilities.
In `@apps/frontend/src/components/ArchivedList.vue`:
- Around line 1-26: ArchivedList.vue and DeletedList.vue duplicate the same
row/layout logic (props like items, syncMap, errorMap, isSyncing, isProcessing,
use of SyncStatusBadge and displayStatus) differing only by the emitted event
name ('unarchive-item' vs 'restore-item') and the action button label; extract
the shared row into a new small presentational component (e.g., ItemListRow or
ArchivedDeletedRow) that accepts the item, sync/error maps, status label
function, an actionLabel prop, and an actionEvent or actionHandler prop; update
ArchivedList.vue and DeletedList.vue to render the new component for each item,
passing through props and binding the appropriate event name/handler (emit
'unarchive-item' or 'restore-item' from the parent when the child signals
action) so styling/logic lives in one place and only the label/event differ.
In `@apps/frontend/src/components/TaskDrawer.vue`:
- Around line 503-534: The tab markup uses role="tablist" and role="tab" but
lacks matching tabpanel elements, aria-controls/aria-labelledby, and keyboard
focus handling; update the TaskDrawer tab implementation (nav with
role="tablist", buttons that call setCurrentTab and use currentTab) to implement
a full ARIA tabs pattern: give each tab button a unique id and aria-controls
pointing to the corresponding tabpanel id, render matching elements (or a
container component) with role="tabpanel" and aria-labelledby pointing back to
the tab id, and add keyboard handling (left/right arrows, Home/End, and
Enter/Space activation) so focus moves between tabs while setCurrentTab still
updates currentTab; ensure ids are unique and test with keyboard and a screen
reader to verify expected announcements and focus behavior.
🪄 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: c1bf298f-180c-4fd7-9e35-23eb05326c0f
📒 Files selected for processing (12)
API_SMOKE_TEST_VERIFICATION.mdapps/backend/API_DOCUMENT.mdapps/backend/src/index.tsapps/frontend/ARCHITECTURE.mdapps/frontend/src/api/honoClient.tsapps/frontend/src/api/itemRepository.tsapps/frontend/src/components/ActiveList.vueapps/frontend/src/components/ArchivedList.vueapps/frontend/src/components/DeletedList.vueapps/frontend/src/components/TaskDrawer.vueapps/frontend/src/composables/useItems.tsapps/frontend/src/composables/useSettings.ts
💤 Files with no reviewable changes (1)
- apps/frontend/src/components/ActiveList.vue
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
apps/frontend/src/components/ActionableItemRow.vue (1)
36-38: Consider handling null motivation gracefully.
item.motivationcan benullaccording to the Item type definition. Renderingnulldirectly will display nothing, which may confuse users.💡 Suggested improvement
<span class="motivation"> - <span class="fire">🔥</span> {{ item.motivation }} + <span class="fire">🔥</span> {{ item.motivation ?? '—' }} </span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/src/components/ActionableItemRow.vue` around lines 36 - 38, The span rendering item.motivation may receive null; update the ActionableItemRow.vue rendering logic (the <span class="motivation"> block that outputs {{ item.motivation }}) to handle null/undefined by either showing a sensible fallback (e.g., "No motivation provided" or an empty placeholder) or conditionally rendering the span only when item.motivation is truthy; modify the template to use a ternary/fallback expression (e.g., {{ item.motivation || 'No motivation provided' }}) or a v-if on item.motivation to avoid showing a confusing empty UI element.API_SMOKE_TEST_VERIFICATION.md (2)
15-15: Add language identifiers to fenced code blocks.Static analysis flags multiple fenced code blocks without language specifiers. Adding language hints improves syntax highlighting and tooling support.
📝 Example fix
-``` +```text TaskDrawer.handleArchive() → useItems.archiveItem(id)For JavaScript/TypeScript blocks, use
javascriptortypescript. For flow diagrams, usetextorplaintext.Also applies to: 54-54, 65-65, 101-101, 114-114, 150-150, 198-198, 255-255, 274-274, 282-282, 413-413, 428-428, 440-440, 452-452, 464-464
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@API_SMOKE_TEST_VERIFICATION.md` at line 15, Several fenced code blocks (e.g., the snippet showing "TaskDrawer.handleArchive() → useItems.archiveItem(id)") lack language identifiers; update each fence to include an appropriate language hint (use "text" or "plaintext" for plain output/flow diagrams and "javascript" or "typescript" for JS/TS snippets) so syntax highlighters and tooling can parse them — apply this to the shown occurrences and the other flagged blocks (lines referenced in the review) by changing ``` to ```text, ```javascript, or ```typescript as appropriate.
479-482: Minor: Repeated adverb "correctly".Static analysis flagged the repeated use of "correctly" in close succession.
📝 Suggested fix
✅ **All verification checks passed**: - Archive/Unarchive operations correctly implemented end-to-end -- Delete/Restore operations correctly prevent duplication +- Delete/Restore operations prevent duplication as expected - Database state remains authoritative (`sync_status = 'synced'`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@API_SMOKE_TEST_VERIFICATION.md` around lines 479 - 482, The three bullet lines in the verification summary repeat the adverb "correctly"; update the bullet text for clarity and variety by removing or replacing one or more instances of "correctly" (e.g., change "Archive/Unarchive operations correctly implemented end-to-end" to "Archive/Unarchive operations implemented end-to-end" or "Delete/Restore operations correctly prevent duplication" to "Delete/Restore operations prevent duplication") so the lines remain equivalent in meaning but avoid consecutive repetition; edit the strings shown in the diff to use the revised phrasing.apps/backend/src/index.ts (1)
594-611: Consider extracting duplicate logic into shared handlers.The inline route handlers for
/api/items/:id/archiveand/api/items/:id/unarchiveduplicate the logic fromhandleArchiveItemandhandleUnarchiveItem, except they readidfrom the URL param instead of the request body. This could lead to maintenance drift.Consider unifying by having the named handlers accept an optional
idparameter, or extracting a shared core function:♻️ Example refactor approach
// Shared core logic async function performArchive(c: Context<AppEnv>, id: string): Promise<Response> { const { token } = c.get('auth'); const supabase = createSupabaseWithToken(token); const { error } = await supabase .from('items') .update({ is_archived: true, updated_at: new Date().toISOString(), sync_status: 'synced', }) .eq('id', id) .is('deleted_at', null); if (error) return c.json({ error: error.message }, 400); return c.body(null, 204); } // Routes can then delegate: app.post('/api/items/:id/archive', (c) => performArchive(c, c.req.param('id'))); app.post('/api/items/archive', async (c) => { const body = await parseJson(c, { id: '' }); if (!body.id) return c.json({ error: 'id is required' }, 400); return performArchive(c, body.id); });Also applies to: 613-630
🤖 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 594 - 611, The archive/unarchive route handlers duplicate logic from handleArchiveItem and handleUnarchiveItem; extract the shared DB update code into a single core function (e.g., performArchive and performUnarchive or a generic performToggleArchive) that accepts (c: Context<AppEnv>, id: string) and returns the same Response shape, then have the existing routes app.post('/api/items/:id/archive') and the body-based handlers call that core function (use c.req.param('id') for URL routes and parseJson to extract id for body routes), keep error handling and supabase creation inside the core function and remove the inline duplication in the route handlers.apps/frontend/src/components/TaskDrawer.vue (1)
767-769: Consider showing the list component alongside loading state.Currently, when
isLoadingArchivedis true, only the loading message shows. If the user had previously loaded data and switches away then back, they see the loading indicator instead of stale data. This is fine, but consider whether showing both (list + spinner overlay) would provide a better UX for subsequent loads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/src/components/TaskDrawer.vue` around lines 767 - 769, The template currently hides archivedItems when isLoadingArchived is true; change the rendering so the archived list component (or v-for using archivedItems) is rendered whenever archivedItems.length > 0 regardless of isLoadingArchived, and show the loading state as an overlay/spinner when isLoadingArchived is true; specifically update the conditional logic around isLoadingArchived, errorArchived and the archived list (references: isLoadingArchived, errorArchived, archivedItems, and the list component or v-for that displays archived items) so that the list renders for stale data and the loading indicator is shown on top (e.g., render list unconditionally when items exist and change the loading div to an overlay or use v-show instead of v-if).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@API_SMOKE_TEST_VERIFICATION.md`:
- Around line 197-203: Update the documentation to reflect the real frontend
behavior: replace the local-filter line items.value = items.value.filter((i) =>
i.id !== id) with a description that the UI calls
itemRepository.getActiveItems() and assigns its result to items.value (i.e.,
items.value is refreshed from the server via itemRepository.getActiveItems), and
also note that the deleted-tab local array is filtered to remove the restored
item so the deleted projection updates immediately while the active list is
refreshed from the server.
- Around line 100-106: Update the documentation snippet describing authenticated
unarchive behavior to match the implementation in useItems.ts: replace the
local-filter example with a note that useItems.ts calls
itemRepository.getActiveItems() and refreshes items.value from the server (this
ensures the active list includes the newly unarchived item), and explicitly
mention that the archived tab is locally filtered to remove the item; reference
useItems.ts and the itemRepository.getActiveItems call to locate the
implementation.
---
Nitpick comments:
In `@API_SMOKE_TEST_VERIFICATION.md`:
- Line 15: Several fenced code blocks (e.g., the snippet showing
"TaskDrawer.handleArchive() → useItems.archiveItem(id)") lack language
identifiers; update each fence to include an appropriate language hint (use
"text" or "plaintext" for plain output/flow diagrams and "javascript" or
"typescript" for JS/TS snippets) so syntax highlighters and tooling can parse
them — apply this to the shown occurrences and the other flagged blocks (lines
referenced in the review) by changing ``` to ```text, ```javascript, or
```typescript as appropriate.
- Around line 479-482: The three bullet lines in the verification summary repeat
the adverb "correctly"; update the bullet text for clarity and variety by
removing or replacing one or more instances of "correctly" (e.g., change
"Archive/Unarchive operations correctly implemented end-to-end" to
"Archive/Unarchive operations implemented end-to-end" or "Delete/Restore
operations correctly prevent duplication" to "Delete/Restore operations prevent
duplication") so the lines remain equivalent in meaning but avoid consecutive
repetition; edit the strings shown in the diff to use the revised phrasing.
In `@apps/backend/src/index.ts`:
- Around line 594-611: The archive/unarchive route handlers duplicate logic from
handleArchiveItem and handleUnarchiveItem; extract the shared DB update code
into a single core function (e.g., performArchive and performUnarchive or a
generic performToggleArchive) that accepts (c: Context<AppEnv>, id: string) and
returns the same Response shape, then have the existing routes
app.post('/api/items/:id/archive') and the body-based handlers call that core
function (use c.req.param('id') for URL routes and parseJson to extract id for
body routes), keep error handling and supabase creation inside the core function
and remove the inline duplication in the route handlers.
In `@apps/frontend/src/components/ActionableItemRow.vue`:
- Around line 36-38: The span rendering item.motivation may receive null; update
the ActionableItemRow.vue rendering logic (the <span class="motivation"> block
that outputs {{ item.motivation }}) to handle null/undefined by either showing a
sensible fallback (e.g., "No motivation provided" or an empty placeholder) or
conditionally rendering the span only when item.motivation is truthy; modify the
template to use a ternary/fallback expression (e.g., {{ item.motivation || 'No
motivation provided' }}) or a v-if on item.motivation to avoid showing a
confusing empty UI element.
In `@apps/frontend/src/components/TaskDrawer.vue`:
- Around line 767-769: The template currently hides archivedItems when
isLoadingArchived is true; change the rendering so the archived list component
(or v-for using archivedItems) is rendered whenever archivedItems.length > 0
regardless of isLoadingArchived, and show the loading state as an
overlay/spinner when isLoadingArchived is true; specifically update the
conditional logic around isLoadingArchived, errorArchived and the archived list
(references: isLoadingArchived, errorArchived, archivedItems, and the list
component or v-for that displays archived items) so that the list renders for
stale data and the loading indicator is shown on top (e.g., render list
unconditionally when items exist and change the loading div to an overlay or use
v-show instead of v-if).
🪄 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: 32968a85-a108-467e-9222-a3d45fe69280
📒 Files selected for processing (8)
API_SMOKE_TEST_VERIFICATION.mdapps/backend/src/index.tsapps/frontend/ARCHITECTURE.mdapps/frontend/src/components/ActionableItemRow.vueapps/frontend/src/components/ArchivedList.vueapps/frontend/src/components/DeletedList.vueapps/frontend/src/components/TaskDrawer.vueapps/frontend/src/composables/useItems.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/frontend/src/components/ArchivedList.vue
- apps/frontend/ARCHITECTURE.md
- apps/frontend/src/components/DeletedList.vue
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/stores/user.ts (1)
7-13:⚠️ Potential issue | 🟠 MajorReject
@usernames at validation time, not just during normalization.
apps/backend/src/index.tsstill accepts any non-emptyusernameon signup, but this store now normalizes that value tonull. A user can register with a value likealice@work, get a success response, and then immediately see a fallback name instead of what they entered. Please enforce the same rule in the signup form/backend so accepted usernames are never silently rewritten.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/src/stores/user.ts` around lines 7 - 13, normalizeUsername currently strips usernames containing '@' to null, causing accepted signups to be silently rewritten; update the signup validation so usernames with '@' are rejected up-front. Add the same rejection rule used by normalizeUsername (no empty/whitespace-only, not equal to 'unknown' case-insensitive, and must not include '@') to the client-side signup form validator and to the server-side signup handler (the backend signup validation function/route that currently accepts any non-empty username). Return a validation error to the user when the rule fails so inputs like "alice@work" are never accepted and later normalized away.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@API_SMOKE_TEST_VERIFICATION.md`:
- Around line 45-52: The Markdown tables (e.g., the "Database State After
Archive" table containing the `is_archived`, `deleted_at`, `sync_status`,
`updated_at` rows) need blank lines inserted before and after the table markup
to satisfy markdownlint MD058; edit the section(s) so there is an empty line
above the table header line (`| Field | Before | After |`) and an empty line
after the final table row, and apply the same change to the other tables in this
document to keep the file lint-clean.
In `@apps/backend/src/index.ts`:
- Around line 381-399: The update call in performToggleArchive currently doesn't
detect zero-row matches because supabase-js v2 returns {data:null, error:null}
for no-op updates; modify the mutation chain in performToggleArchive to append
.select('id').maybeSingle() to the update query (i.e.,
supabase.from('items').update(...).eq('id', id).is('deleted_at',
null).select('id').maybeSingle()), then inspect the returned data and if it's
null return c.json({ error: 'not found' }, 404) instead of a 204; apply the same
.select('id').maybeSingle() + null-check -> 404 pattern to the restore handlers
and all soft-delete/restore mutation paths so you reliably detect and return 404
when no row was changed.
In `@apps/frontend/src/components/ActionableItemRow.vue`:
- Around line 36-38: The motivation cell currently uses a hardcoded fallback
"N/A" (in the <span class="motivation"> template) which is not localized; change
it to use a localized fallback provided the same way as actionLabel/statusLabel
— either accept a prop like motivationLabel or motivationFallback and render {{
item.motivation ?? motivationLabel }} or resolve the fallback in the parent and
pass it in, and update the component props/computed that mirror how
actionLabel/statusLabel are handled so the fallback follows the active locale.
In `@apps/frontend/src/components/TaskDrawer.vue`:
- Around line 331-343: The catch blocks in the restore/unarchive handlers (e.g.,
handleUnarchiveFromList and the similar handler around lines 346-358) merely log
errors and swallow failures; update each catch to set the appropriate reactive
error ref (use errorArchived or errorDeleted as appropriate) and emit a failure
event so the UI shows feedback: when unarchiveItem or deleteItem rejects, assign
the caught error to errorArchived.value or errorDeleted.value, call emit('error'
or emit a specific failure toast event) and keep finally resetting the
isRestoring... flag; ensure you reference isRestoringTabItem, archivedItems,
unarchiveItem, emit and the corresponding errorArchived/errorDeleted refs so the
component surfaces the failure instead of silently logging it.
---
Outside diff comments:
In `@apps/frontend/src/stores/user.ts`:
- Around line 7-13: normalizeUsername currently strips usernames containing '@'
to null, causing accepted signups to be silently rewritten; update the signup
validation so usernames with '@' are rejected up-front. Add the same rejection
rule used by normalizeUsername (no empty/whitespace-only, not equal to 'unknown'
case-insensitive, and must not include '@') to the client-side signup form
validator and to the server-side signup handler (the backend signup validation
function/route that currently accepts any non-empty username). Return a
validation error to the user when the rule fails so inputs like "alice@work" are
never accepted and later normalized away.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: 2c47799c-a7c9-4898-81f7-55edd5216b4e
📒 Files selected for processing (5)
API_SMOKE_TEST_VERIFICATION.mdapps/backend/src/index.tsapps/frontend/src/components/ActionableItemRow.vueapps/frontend/src/components/TaskDrawer.vueapps/frontend/src/stores/user.ts
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The branch was updated while autofix was in progress. Please try again. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/backend/src/index.ts (2)
324-334:⚠️ Potential issue | 🟡 MinorStatus update handlers don't detect zero-row matches.
Unlike the archive/restore handlers which now return 404 when no row matches,
handleUpdateItemStatusandPATCH /api/items/:id/statuswill silently return 204 even if the item doesn't exist or was deleted. For consistency with other mutation endpoints, consider adding the same.select('id').maybeSingle()+ 404 pattern.🔧 Suggested fix for handleUpdateItemStatus
- const { error } = await supabase + const { data, error } = await supabase .from('items') .update({ status: validatedStatus, updated_at: new Date().toISOString(), sync_status: 'synced', }) - .eq('id', body.id); + .eq('id', body.id) + .select('id') + .maybeSingle(); if (error) return c.json({ error: error.message }, 400); + if (!data) return c.json({ error: '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 324 - 334, handleUpdateItemStatus (the PATCH /api/items/:id/status handler) currently ignores zero-row matches and always returns 204; modify the Supabase update call to include a returning select and use maybeSingle() (e.g. .update({...}).select('id').maybeSingle()) then check the returned data for null and return c.json({ error: 'Not found' }, 404) when no row was matched, otherwise proceed to return 204; ensure you still handle and surface Supabase errors from the error object.
372-386:⚠️ Potential issue | 🟡 MinorItem update handler also lacks zero-row detection.
Same as the status update handlers,
handleUpdateItemwill return 204 even if the item doesn't exist. Consider applying the same.select('id').maybeSingle()+ 404 pattern for consistency.🤖 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 372 - 386, The update handler currently only checks supabase error and returns 204 even when no row was updated; modify the Supabase call in the handleUpdateItem logic to append .select('id').maybeSingle() to the .update(...) query, then inspect the returned data (the presence of data.id) and return c.json({ error: 'Not found' }, 404) if no row was found/updated, otherwise keep returning 204; ensure you still handle and return supabase error.message as before when error is present.
🧹 Nitpick comments (3)
apps/frontend/src/components/ActionableItemRow.vue (2)
161-181: Add an explicit keyboard focus-visible style on the action button.The button has hover/disabled styles, but a dedicated
:focus-visibletreatment would improve keyboard accessibility consistency.Proposed diff
.row-action:hover { border-color: `#a855f7`; background: color-mix(in srgb, var(--bg-primary) 70%, `#ffffff`); } +.row-action:focus-visible { + outline: 2px solid `#a855f7`; + outline-offset: 2px; +} + .row-action:disabled { opacity: 0.6; cursor: not-allowed; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/src/components/ActionableItemRow.vue` around lines 161 - 181, Add a dedicated keyboard focus-visible style for the action button selector (.row-action) by implementing a .row-action:focus-visible rule that provides a clear, high-contrast visible indicator (e.g., distinct border-color and/or ring/outline/box-shadow) separate from hover and disabled states; ensure it uses :focus-visible (not :focus) for keyboard-only activation, preserves existing border-radius/padding, and does not reduce accessibility by conflicting with .row-action:hover or .row-action:disabled.
7-8: Use sparse-map typings forsyncMapanderrorMap.The maps are initialized as empty objects and populated selectively (lines 11–12, 19–30 in
useSyncStatus.ts), then accessed via optional lookups in the template (syncMap[item.id],errorMap[item.id]). Type them asPartial<Record<...>>to accurately reflect this sparse-map behavior.Proposed diff
defineProps<{ item: Item; - syncMap: Record<string, 'pending' | 'success' | 'error'>; - errorMap: Record<string, string>; + syncMap: Partial<Record<string, 'pending' | 'success' | 'error'>>; + errorMap: Partial<Record<string, string>>; isSyncing: boolean; isProcessing?: boolean; actionLabel: string; motivationFallback: string; statusLabel: (status: Item['status']) => string; }>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/src/components/ActionableItemRow.vue` around lines 7 - 8, The types for syncMap and errorMap are too strict for sparse usage; change their declarations from Record<string, 'pending' | 'success' | 'error'> and Record<string, string> to Partial<Record<string, 'pending' | 'success' | 'error'>> and Partial<Record<string, string>> respectively so they reflect selective population and optional lookups (affects syncMap and errorMap used in useSyncStatus.ts and accessed as syncMap[item.id]/errorMap[item.id] in ActionableItemRow.vue).apps/backend/src/index.ts (1)
630-672: Consider extracting shared mutation logic to reduce duplication.The soft-delete logic in
DELETE /api/items/:id(lines 635-644) andhandleSoftDeleteItem(lines 429-438) is nearly identical, differing only in how theidis obtained (URL param vs. body). The same applies to the restore handlers.You could extract the mutation logic similar to how
performToggleArchivewas done:♻️ Refactor suggestion
async function performSoftDelete(c: Context<AppEnv>, id: string): Promise<Response> { if (!id) return c.json({ error: 'id is required' }, 400); const { token } = c.get('auth'); const supabase = createSupabaseWithToken(token); const { data, error } = await supabase .from('items') .update({ deleted_at: new Date().toISOString(), updated_at: new Date().toISOString(), sync_status: 'synced', }) .eq('id', id) .select('id') .maybeSingle(); if (error) return c.json({ error: error.message }, 400); if (!data) return c.json({ error: 'not found' }, 404); return c.body(null, 204); } // Then use in both places: app.delete('/api/items/:id', async (c) => performSoftDelete(c, c.req.param('id'))); // handleSoftDeleteItem would call performSoftDelete(c, body.id)🤖 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 630 - 672, The DELETE and POST/restore handlers duplicate the same Supabase mutation logic (similar to performToggleArchive); extract the shared update logic into a helper like performSoftDelete(c, id) and performRestore(c, id) that accept Context and an id, validate the id, create the Supabase client via createSupabaseWithToken(token), run the .from('items').update(...) mutation (setting deleted_at/updated_at/sync_status or clearing deleted_at/is_archived accordingly), handle error/data responses (return c.json on error/not found, c.body null 204 on success), and then call these helpers from both app.delete('/api/items/:id') and handleSoftDeleteItem (and likewise for the restore route) to remove duplication while keeping id sourcing (c.req.param vs request body) in the callers.
🤖 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 187-200: The fallback branches that return metadataUsername (when
resolvedProfileUsername fails) currently only check metadataUsername.length > 0
and no '@' but do not reject the literal "unknown" (case-insensitive); update
both places (the if in the error branch checking resolvedProfileUsername and the
one in the catch block) to also ensure metadataUsername.toLowerCase() !==
'unknown' (or call isValidUsername(metadataUsername) if available) before
returning it, otherwise return fallbackName.
---
Outside diff comments:
In `@apps/backend/src/index.ts`:
- Around line 324-334: handleUpdateItemStatus (the PATCH /api/items/:id/status
handler) currently ignores zero-row matches and always returns 204; modify the
Supabase update call to include a returning select and use maybeSingle() (e.g.
.update({...}).select('id').maybeSingle()) then check the returned data for null
and return c.json({ error: 'Not found' }, 404) when no row was matched,
otherwise proceed to return 204; ensure you still handle and surface Supabase
errors from the error object.
- Around line 372-386: The update handler currently only checks supabase error
and returns 204 even when no row was updated; modify the Supabase call in the
handleUpdateItem logic to append .select('id').maybeSingle() to the .update(...)
query, then inspect the returned data (the presence of data.id) and return
c.json({ error: 'Not found' }, 404) if no row was found/updated, otherwise keep
returning 204; ensure you still handle and return supabase error.message as
before when error is present.
---
Nitpick comments:
In `@apps/backend/src/index.ts`:
- Around line 630-672: The DELETE and POST/restore handlers duplicate the same
Supabase mutation logic (similar to performToggleArchive); extract the shared
update logic into a helper like performSoftDelete(c, id) and performRestore(c,
id) that accept Context and an id, validate the id, create the Supabase client
via createSupabaseWithToken(token), run the .from('items').update(...) mutation
(setting deleted_at/updated_at/sync_status or clearing deleted_at/is_archived
accordingly), handle error/data responses (return c.json on error/not found,
c.body null 204 on success), and then call these helpers from both
app.delete('/api/items/:id') and handleSoftDeleteItem (and likewise for the
restore route) to remove duplication while keeping id sourcing (c.req.param vs
request body) in the callers.
In `@apps/frontend/src/components/ActionableItemRow.vue`:
- Around line 161-181: Add a dedicated keyboard focus-visible style for the
action button selector (.row-action) by implementing a .row-action:focus-visible
rule that provides a clear, high-contrast visible indicator (e.g., distinct
border-color and/or ring/outline/box-shadow) separate from hover and disabled
states; ensure it uses :focus-visible (not :focus) for keyboard-only activation,
preserves existing border-radius/padding, and does not reduce accessibility by
conflicting with .row-action:hover or .row-action:disabled.
- Around line 7-8: The types for syncMap and errorMap are too strict for sparse
usage; change their declarations from Record<string, 'pending' | 'success' |
'error'> and Record<string, string> to Partial<Record<string, 'pending' |
'success' | 'error'>> and Partial<Record<string, string>> respectively so they
reflect selective population and optional lookups (affects syncMap and errorMap
used in useSyncStatus.ts and accessed as syncMap[item.id]/errorMap[item.id] in
ActionableItemRow.vue).
🪄 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: 647032dd-cec1-457d-ae8c-d9e89c5e6e88
📒 Files selected for processing (8)
API_SMOKE_TEST_VERIFICATION.mdapps/backend/src/index.tsapps/frontend/src/components/ActionableItemRow.vueapps/frontend/src/components/ArchivedList.vueapps/frontend/src/components/DeletedList.vueapps/frontend/src/components/TaskDrawer.vueapps/frontend/src/composables/useSettings.tsapps/frontend/src/views/LoginView.vue
✅ Files skipped from review due to trivial changes (2)
- apps/frontend/src/composables/useSettings.ts
- API_SMOKE_TEST_VERIFICATION.md
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/frontend/src/components/DeletedList.vue
- apps/frontend/src/components/ArchivedList.vue
- apps/frontend/src/components/TaskDrawer.vue
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/backend/src/index.ts (1)
614-627: Consider extracting duplicated status update logic.This inline route handler duplicates the logic in
handleUpdateItemStatus(lines 309-338). The only difference is that this route getsidfrom path params whilehandleUpdateItemStatusgets it from body.♻️ Suggested refactor
Extract shared logic into a helper:
+async function performUpdateStatus(c: Context<AppEnv>, id: string, status: string): Promise<Response> { + const validatedStatus = parseStatus(status); + if (!validatedStatus) { + return c.json({ error: 'invalid status' }, 400); + } + + const { token } = c.get('auth'); + const supabase = createSupabaseWithToken(token); + + const { data, error } = await supabase + .from('items') + .update({ + status: validatedStatus, + updated_at: new Date().toISOString(), + sync_status: 'synced', + }) + .eq('id', id) + .select('id') + .maybeSingle(); + + if (error) return c.json({ error: error.message }, 400); + if (!data) return c.json({ error: 'Not found' }, 404); + return c.body(null, 204); +} async function handleUpdateItemStatus(c: Context<AppEnv>): Promise<Response> { const body = await parseJson(c, { id: '', status: '' }); if (!body.id || !body.status) { return c.json({ error: 'id and status are required' }, 400); } - // ... rest of implementation + return performUpdateStatus(c, body.id, body.status); } app.patch('/api/items/:id/status', async (c) => { const id = c.req.param('id'); const body = await parseJson(c, { status: '' }); if (!body.status) { return c.json({ error: 'status is required' }, 400); } - // ... rest of implementation + return performUpdateStatus(c, id, body.status); });🤖 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 614 - 627, Extract the duplicated status-update logic into a shared helper (e.g., updateItemStatusById) and call it from both the inline route and handleUpdateItemStatus; the helper should accept (id: string, validatedStatus: string, ctx/request-context) perform the supabase .from('items').update({... status: validatedStatus, updated_at: new Date().toISOString(), sync_status: 'synced'}) .eq('id', id) .select('id').maybeSingle() call, and return a uniform result or throw/return structured error so both callers can map it to the same responses (400 for supabase error.message, 404 when no data, 204 on success). Ensure you remove the duplicated block in the route and replace it with a call to updateItemStatusById and preserve existing response behavior.
🤖 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 428-448: performSoftDelete currently updates any item by id
regardless of its deletion state; add the same deleted filter used elsewhere by
applying .is('deleted_at', null) to the Supabase query (the chain that calls
.from('items')... before .eq('id', id)) so that already-deleted items are not
re-soft-deleted and will return 404 when no matching non-deleted row exists;
keep the existing error/data handling (error -> 400, !data -> 404) unchanged.
---
Nitpick comments:
In `@apps/backend/src/index.ts`:
- Around line 614-627: Extract the duplicated status-update logic into a shared
helper (e.g., updateItemStatusById) and call it from both the inline route and
handleUpdateItemStatus; the helper should accept (id: string, validatedStatus:
string, ctx/request-context) perform the supabase .from('items').update({...
status: validatedStatus, updated_at: new Date().toISOString(), sync_status:
'synced'}) .eq('id', id) .select('id').maybeSingle() call, and return a uniform
result or throw/return structured error so both callers can map it to the same
responses (400 for supabase error.message, 404 when no data, 204 on success).
Ensure you remove the duplicated block in the route and replace it with a call
to updateItemStatusById and preserve existing response behavior.
🪄 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: ab231206-e23a-4eaa-ae58-63d2d2dd3558
📒 Files selected for processing (2)
apps/backend/src/index.tsapps/frontend/src/components/ActionableItemRow.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/frontend/src/components/ActionableItemRow.vue
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
Bug Fixes
Documentation
UX