Conversation
Contributor
📝 WalkthroughWalkthroughAdds frontend graph visualization (canvas-backed ScatterPlot) and a TaskDrawer for create/edit/view flows; expands item schema (description, motivation, duration), exposes updateItem across client/repository, and implements backend PATCH handlers plus safer profile username resolution and fallback logic. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TaskDrawer as Drawer (UI)
participant ItemsRepo as ItemRepository
participant Hono as HonoClient
participant Backend
participant DB as Database
User->>Drawer: edit item & submit update
Drawer->>ItemsRepo: updateItem(payload)
ItemsRepo->>Hono: PATCH /api/items/{id} with payload
Hono->>Backend: HTTP PATCH /api/items/:id
Backend->>DB: update item fields, set updated_at & sync_status
DB-->>Backend: success
Backend-->>Hono: 204 No Content
Hono-->>ItemsRepo: resolve
ItemsRepo-->>Drawer: refresh/fetchActiveItems()
Drawer-->>User: UI updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/ARCHITECTURE.md (1)
44-45:⚠️ Potential issue | 🟡 MinorUnify in-flight guard terminology to avoid implementation confusion.
Line 44 documents guard checks via
isSyncing.value, while Line 550 says guard isautoSyncInFlight. Keep one canonical term (or explicitly document both and their roles) to prevent drift.Proposed doc fix (if `isSyncing` is the canonical guard)
-- **Guards**: In-flight protection (`autoSyncInFlight` flag) prevents concurrent sync operations +- **Guards**: In-flight protection (`isSyncing.value`) prevents concurrent sync operationsAlso applies to: 550-551
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/ARCHITECTURE.md` around lines 44 - 45, The doc uses two different names for the same in-flight guard causing confusion—unify to a single canonical term (preferably isSyncing) or explicitly document both with roles; update the ARCHITECTURE.md occurrences that reference isSyncing.value and autoSyncInFlight so they consistently use the chosen name (e.g., replace autoSyncInFlight with isSyncing.value everywhere or add a short note that autoSyncInFlight is an alias for isSyncing and explain which module exposes each symbol), and ensure the guard description and examples reference the exact symbol name used in code.
🧹 Nitpick comments (3)
apps/frontend/ARCHITECTURE.md (1)
507-507: Make build verification output reproducible.“build count increases” is not a stable verification result. Prefer timestamp + concrete success criteria (e.g., exit code or artifact path) so future readers can validate consistently.
Proposed doc fix
-Result: build count increases with added graph/visualization modules (run `pnpm run build` to get latest). +Result (verified on 2026-03-14): `pnpm run build` completed successfully with exit code 0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/ARCHITECTURE.md` at line 507, Replace the unstable verification line "Result: build count increases with added graph/visualization modules (run `pnpm run build` to get latest)." with a reproducible check that records a timestamp and concrete success criteria: run `pnpm run build`, capture and record the ISO timestamp, assert the build exit code is 0 and/or verify the expected artifact path(s) exist (e.g., dist/ or build/ artifacts). Update the ARCHITECTURE.md section to show the exact command, the timestamp placeholder, and the deterministic validation steps (exit code and artifact path assertions) so readers can reproduce and validate consistently.apps/frontend/src/style.css (1)
21-24: Prefer dynamic viewport sizing over fixedheight: 100vhfor#app.Remove the fixed
height: 100vhdeclaration at line 24. The100vhunit locks the element to the browser's large viewport (toolbar retracted), causing overflow on mobile when browser UI is visible. SinceMainDashboard.vueusesh-dvhandScatterPlot.vuemeasures runtime container dimensions withgetBoundingClientRect(), ensure the root container adapts dynamically.Suggested change
`#app` { - min-height: 100vh; - height: 100vh; + min-height: 100vh; + min-height: 100dvh; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/src/style.css` around lines 21 - 24, Remove the fixed height declaration from the `#app` selector (delete "height: 100vh") so the root container uses only min-height: 100vh and can adapt to dynamic viewport sizing; leave min-height intact and rely on MainDashboard.vue's h-dvh and ScatterPlot.vue's getBoundingClientRect() measurements to provide correct runtime dimensions.apps/frontend/src/views/MainDashboard.vue (1)
124-145: Avoid the second reload aftercreateItem().
useItems.createItem()already awaitsfetchActiveItems()before returning (seeapps/frontend/src/composables/useItems.ts:173-190), sohandleRefreshItems()adds another full fetch on every create.♻️ Suggested simplification
async function handleCreateItem(payload: { title: string; description: string | null; motivation: number | null; due: string; durationMinutes?: number | null; }): Promise<void> { isCreating.value = true; try { const id = await createItem(payload); - await handleRefreshItems(); const created = items.value.find((item) => item.id === id) ?? null; if (created) { selectedItem.value = created; drawerMode.value = 'view';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/src/views/MainDashboard.vue` around lines 124 - 145, The code calls handleRefreshItems() after await createItem(payload), causing a redundant fetch because createItem (in useItems.createItem) already updates items by awaiting fetchActiveItems(); remove the extra await handleRefreshItems() call in handleCreateItem, so after const id = await createItem(payload) directly locate the created item from items.value (as the composable has already refreshed) and then set selectedItem.value and drawerMode.value as needed.
🤖 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 157-172: In fetchProfileUsername, trim the fetched username
immediately and treat the trimmed string as authoritative by checking if
trimmedUsername is non-empty; only when it's empty or on error return the
existing fallbackName, but log a generic message (no userId or email-derived
fallback) like "Profile fetch failed, using fallback username" instead of
including user-derived data; update the code paths that read data.username, the
error check, the console.log, and the return to use the trimmedUsername check
and generic message.
In `@apps/frontend/ARCHITECTURE.md`:
- Around line 473-480: The example calls startNewSession() but the destructuring
only pulls startAutoSync and stopAutoSync from useItems; update the example to
also obtain startNewSession (e.g., const { startAutoSync, stopAutoSync,
startNewSession } = useItems()) or import/startNewSession from the module where
it’s declared so that the symbol startNewSession is defined before it’s invoked
in the watch block; ensure you reference the same source as
startAutoSync/stopAutoSync to keep consistency.
- Around line 80-83: Add TaskDrawer.vue to the component tree and MainDashboard
documentation in ARCHITECTURE.md: update the component list under src/components
to include TaskDrawer.vue alongside ScatterPlot.vue, SyncStatusBadge.vue and
TaskList.vue, and in the MainDashboard section describe that MainDashboard.vue
imports and uses TaskDrawer.vue to display/edit task details (it’s imported in
MainDashboard.vue and invoked where the task details drawer is rendered),
explaining its role and interactions with the dashboard task list and selection
state.
In `@apps/frontend/src/components/ScatterPlot.vue`:
- Around line 411-418: The watcher on [graphGroups, layout, warnings] needs to
also close the floating cluster menu so it doesn't remain tied to stale nodes;
inside the async callback (after await nextTick()) call the routine that
hides/closes the menu (e.g., invoke clusterMenu.close() or set clusterMenu.open
= false / clusterMenu.visible = false depending on how clusterMenu is
implemented) before calling renderSoon(), using the existing clusterMenu
identifier so the menu is definitely dismissed when graphGroups or layout
change.
In `@apps/frontend/src/components/TaskDrawer.vue`:
- Around line 150-167: TaskDrawer.vue currently renders the modal without
accessibility hooks; update the <aside> (TaskDrawer component) to be a real
dialog by adding role="dialog" and aria-modal="true", setting aria-labelledby to
the header title element, and ensure keyboard focus management: when open
changes to true move focus to the first focusable control inside the drawer
(e.g., the Close button or first form field) and when closing restore focus to
the element that opened the drawer; add a keydown handler on the drawer to close
on Escape by calling closeDrawer and implement simple focus-trapping logic on
Tab/Shift+Tab to keep focus inside the aside while open (use the component
lifecycle/watchers around the open prop and the existing closeDrawer method to
wire these behaviors).
- Around line 316-377: The edit form only updates local refs (editTitle,
editDescription, editDue, editDuration, editMotivation) and never persists
changes; add a submit flow that collects those refs into an updated item and
persists it (either by emitting an event like "update-item" with the updated
object and selectedItem.id, or calling the repository method from useItems e.g.
updateItem(selectedItem.id, payload)), wire a submit button (or form `@submit`) to
a handler (e.g. handleEditSubmit) that disables inputs while saving, handles
success/failure, updates local state/selectedItem on success, and emits or calls
the backend so edits are actually persisted.
In `@apps/frontend/src/stores/user.ts`:
- Around line 52-64: The storedUsername value is being used as the primary
fallback before the active account is confirmed, which can reattach a stale
cached name to a different session; change the flow in the auto-login block
(authRepository.autoLogin, userId, username) so you first set userId from
localUser, derive username from normalizeUsername(localUser.username) and only
call readStoredUsername or persistUsername with a key scoped to that userId (or
consult readStoredUsername after userId is known) — if
normalizeUsername(localUser.username) is empty, call resolveSessionUsername() to
obtain the username, then persist the username using a userId-scoped cache key
instead of a single global cache.
In `@apps/frontend/tsconfig.app.json`:
- Line 9: Remove the "node" entry from the compilerOptions.types array in
tsconfig.app.json so the browser app only includes browser types (leave
"vite/client" in place); update the "compilerOptions.types" array to exclude
"node" (and ensure the JSON remains valid), and rely on the existing
tsconfig.node.json for Node/tooling type scoping.
---
Outside diff comments:
In `@apps/frontend/ARCHITECTURE.md`:
- Around line 44-45: The doc uses two different names for the same in-flight
guard causing confusion—unify to a single canonical term (preferably isSyncing)
or explicitly document both with roles; update the ARCHITECTURE.md occurrences
that reference isSyncing.value and autoSyncInFlight so they consistently use the
chosen name (e.g., replace autoSyncInFlight with isSyncing.value everywhere or
add a short note that autoSyncInFlight is an alias for isSyncing and explain
which module exposes each symbol), and ensure the guard description and examples
reference the exact symbol name used in code.
---
Nitpick comments:
In `@apps/frontend/ARCHITECTURE.md`:
- Line 507: Replace the unstable verification line "Result: build count
increases with added graph/visualization modules (run `pnpm run build` to get
latest)." with a reproducible check that records a timestamp and concrete
success criteria: run `pnpm run build`, capture and record the ISO timestamp,
assert the build exit code is 0 and/or verify the expected artifact path(s)
exist (e.g., dist/ or build/ artifacts). Update the ARCHITECTURE.md section to
show the exact command, the timestamp placeholder, and the deterministic
validation steps (exit code and artifact path assertions) so readers can
reproduce and validate consistently.
In `@apps/frontend/src/style.css`:
- Around line 21-24: Remove the fixed height declaration from the `#app` selector
(delete "height: 100vh") so the root container uses only min-height: 100vh and
can adapt to dynamic viewport sizing; leave min-height intact and rely on
MainDashboard.vue's h-dvh and ScatterPlot.vue's getBoundingClientRect()
measurements to provide correct runtime dimensions.
In `@apps/frontend/src/views/MainDashboard.vue`:
- Around line 124-145: The code calls handleRefreshItems() after await
createItem(payload), causing a redundant fetch because createItem (in
useItems.createItem) already updates items by awaiting fetchActiveItems();
remove the extra await handleRefreshItems() call in handleCreateItem, so after
const id = await createItem(payload) directly locate the created item from
items.value (as the composable has already refreshed) and then set
selectedItem.value and drawerMode.value as needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 85820f4c-8357-483d-b524-6ced743437c7
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
apps/backend/src/index.tsapps/frontend/ARCHITECTURE.mdapps/frontend/package.jsonapps/frontend/src/App.vueapps/frontend/src/components/ScatterPlot.vueapps/frontend/src/components/TaskDrawer.vueapps/frontend/src/composables/useAuth.tsapps/frontend/src/composables/useGraph.tsapps/frontend/src/stores/user.tsapps/frontend/src/style.cssapps/frontend/src/views/MainDashboard.vueapps/frontend/tsconfig.app.jsonapps/frontend/vite.config.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/frontend/src/components/TaskDrawer.vue (2)
71-76:toDatetimeLocalmay produce incorrect local time.Subtracting the timezone offset and then slicing the ISO string works for most cases, but can produce incorrect results near DST boundaries. Consider using
Intl.DateTimeFormator a library for reliable local datetime formatting.Alternative approach
function toDatetimeLocal(isoValue: string): string { const date = new Date(isoValue); if (Number.isNaN(date.getTime())) return ''; const year = date.getFullYear(); const month = String(date.getMonth() + 1).padStart(2, '0'); const day = String(date.getDate()).padStart(2, '0'); const hours = String(date.getHours()).padStart(2, '0'); const minutes = String(date.getMinutes()).padStart(2, '0'); return `${year}-${month}-${day}T${hours}:${minutes}`; }🤖 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 71 - 76, The toDatetimeLocal function currently adjusts by timezoneOffset and slices an ISO string which can be wrong around DST; replace that logic in toDatetimeLocal: parse isoValue into a Date, validate it, then build the local "YYYY-MM-DDTHH:MM" string using the Date's local getters (getFullYear, getMonth+1, getDate, getHours, getMinutes) with zero-padding, or alternatively format via Intl.DateTimeFormat with options for 24-hour year-month-day and hour:minute; ensure you remove the date.setMinutes(...) and toISOString().slice(...) usage and return the constructed local datetime string.
7-15: Consider importingCreateItemPayloadfrom shared types.The
CreateItemPayloadtype is duplicated here and inhonoClient.ts. Consider importing from a shared location to maintain consistency and reduce duplication.🤖 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 7 - 15, The CreateItemPayload type is duplicated in TaskDrawer.vue and honoClient.ts; remove the local type declaration and import CreateItemPayload from the shared types module instead (replace the local CreateItemPayload definition in TaskDrawer.vue with an import), and ensure any usages (e.g., props, method signatures) still reference CreateItemPayload; also keep DrawerMode local if it’s only used here or move it to the shared types if needed for reuse.apps/frontend/src/components/ScatterPlot.vue (1)
75-76: Consider makinggroupingEnabledconfigurable via prop.Hardcoding
groupingEnabled.value = truelimits flexibility. If grouping should always be enabled for this component, this is fine, but exposing it as a prop would allow parent components to control this behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/src/components/ScatterPlot.vue` around lines 75 - 76, The component currently hardcodes groupingEnabled.value = true; make grouping togglable by adding a prop (e.g., prop name groupingEnabled with type Boolean and default true) to the ScatterPlot component and use that prop instead of overwriting the internal ref; replace the direct assignment in the setup with initializing the reactive state from the prop (or make groupingEnabled a computed that returns props.groupingEnabled) and, if the component needs to mutate it internally, add a separate internal ref and sync it from the prop via a watch to keep parent-controlled behavior consistent; update the props definition and any places that reference groupingEnabled to use the prop/internal synced value instead of the hardcoded true.
🤖 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 309-353: In handleUpdateItem, validate and normalize
body.description before passing it to Supabase: create a validatedDescription
variable that is either a trimmed string when typeof body.description ===
'string' and not empty, or null otherwise, and replace body.description in the
supabase.from('items').update({...}) call with validatedDescription; this
ensures non-string values (objects, numbers, etc.) are not persisted into the
description column.
---
Nitpick comments:
In `@apps/frontend/src/components/ScatterPlot.vue`:
- Around line 75-76: The component currently hardcodes groupingEnabled.value =
true; make grouping togglable by adding a prop (e.g., prop name groupingEnabled
with type Boolean and default true) to the ScatterPlot component and use that
prop instead of overwriting the internal ref; replace the direct assignment in
the setup with initializing the reactive state from the prop (or make
groupingEnabled a computed that returns props.groupingEnabled) and, if the
component needs to mutate it internally, add a separate internal ref and sync it
from the prop via a watch to keep parent-controlled behavior consistent; update
the props definition and any places that reference groupingEnabled to use the
prop/internal synced value instead of the hardcoded true.
In `@apps/frontend/src/components/TaskDrawer.vue`:
- Around line 71-76: The toDatetimeLocal function currently adjusts by
timezoneOffset and slices an ISO string which can be wrong around DST; replace
that logic in toDatetimeLocal: parse isoValue into a Date, validate it, then
build the local "YYYY-MM-DDTHH:MM" string using the Date's local getters
(getFullYear, getMonth+1, getDate, getHours, getMinutes) with zero-padding, or
alternatively format via Intl.DateTimeFormat with options for 24-hour
year-month-day and hour:minute; ensure you remove the date.setMinutes(...) and
toISOString().slice(...) usage and return the constructed local datetime string.
- Around line 7-15: The CreateItemPayload type is duplicated in TaskDrawer.vue
and honoClient.ts; remove the local type declaration and import
CreateItemPayload from the shared types module instead (replace the local
CreateItemPayload definition in TaskDrawer.vue with an import), and ensure any
usages (e.g., props, method signatures) still reference CreateItemPayload; also
keep DrawerMode local if it’s only used here or move it to the shared types if
needed for reuse.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8c6b0be7-1e5d-4f86-899e-57c2f2b8d4d2
📒 Files selected for processing (10)
apps/backend/src/index.tsapps/frontend/src/api/honoClient.tsapps/frontend/src/api/itemRepository.tsapps/frontend/src/components/ScatterPlot.vueapps/frontend/src/components/TaskDrawer.vueapps/frontend/src/composables/useItems.tsapps/frontend/src/stores/user.tsapps/frontend/src/style.cssapps/frontend/src/views/LoginView.vueapps/frontend/src/views/MainDashboard.vue
✅ Files skipped from review due to trivial changes (1)
- apps/frontend/src/views/LoginView.vue
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/backend/src/index.ts (2)
318-321: Avoidanyin request fallback typing fordescription.Using
null as anyweakens type safety here;string | nullis enough and keeps the handler stricter.Proposed diff
- description: null as any, + description: null as string | null,🤖 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 318 - 321, The request body fallback uses `null as any` for the description which weakens typing; change the fallback to use a stricter union type (e.g., `null as string | null`) so `description` is typed as `string | null`. Locate the `parseJson` call that assigns `const body = await parseJson(c, { title: '', description: null as any, due: '' })` and replace the `description` fallback with the `string | null` union, ensuring any downstream usage of `body.description` treats it as possibly null.
247-250: Normalizedescriptionin create path the same way as update path.
handleCreateItemcurrently preserves whitespace-only descriptions, whilehandleUpdateItemtrims and converts empty tonull. Aligning both paths avoids inconsistent stored values.Proposed diff
- const description = - body.description === null || typeof body.description === 'string' - ? body.description - : null; + const description = + typeof body.description === 'string' && body.description.trim().length > 0 + ? body.description.trim() + : null; ... - description: description, + description,Also applies to: 269-269
🤖 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 247 - 250, In handleCreateItem make description normalization consistent with handleUpdateItem: when computing the description (current code using body.description === null || typeof body.description === 'string' ? body.description : null) trim whitespace and convert an empty string to null (i.e., if typeof body.description === 'string' then const d = body.description.trim(); use d === '' ? null : d; else preserve null or set null); update the same logic at the other occurrence noted (around the second instance referenced) so both create and update paths store trimmed-or-null descriptions consistently.
🤖 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 318-321: The request body fallback uses `null as any` for the
description which weakens typing; change the fallback to use a stricter union
type (e.g., `null as string | null`) so `description` is typed as `string |
null`. Locate the `parseJson` call that assigns `const body = await parseJson(c,
{ title: '', description: null as any, due: '' })` and replace the `description`
fallback with the `string | null` union, ensuring any downstream usage of
`body.description` treats it as possibly null.
- Around line 247-250: In handleCreateItem make description normalization
consistent with handleUpdateItem: when computing the description (current code
using body.description === null || typeof body.description === 'string' ?
body.description : null) trim whitespace and convert an empty string to null
(i.e., if typeof body.description === 'string' then const d =
body.description.trim(); use d === '' ? null : d; else preserve null or set
null); update the same logic at the other occurrence noted (around the second
instance referenced) so both create and update paths store trimmed-or-null
descriptions consistently.
This was referenced Mar 23, 2026
Merged
Merged
Merged
Merged
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 #54
Summary by CodeRabbit
New Features
UX Improvements