Add reminders to notes#38
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe pull request introduces a complete reminder management system spanning backend and frontend. Backend changes add reminder CRUD APIs, Zod schemas for validation, utility functions for file persistence and periodic checking, and a new backend event. Frontend changes include a TipTap editor extension for rendering reminders, UI components for picking reminder times and displaying notifications, state management via Pinia, and event-driven dismissal workflows. A shared Changes
Sequence DiagramssequenceDiagram
actor User
participant Editor as NoteEditor
participant Picker as ReminderPicker
participant Store as RemindersStore
participant Backend as Backend API
participant Storage as Persistence
User->>Editor: Right-click with selection
Editor->>Picker: Open picker at cursor position
User->>Picker: Select/input reminder time
Picker->>Store: confirmReminder(notePath, context, reminderAt)
Store->>Backend: createReminder(notePath, context, reminderAt)
Backend->>Storage: readRemindersFile(projectID)
Storage-->>Backend: [existing reminders]
Backend->>Backend: Generate UUID, create Reminder object
Backend->>Storage: writeRemindersFile(projectID, [..., newReminder])
Storage-->>Backend: Success
Backend-->>Store: ok(reminder)
Store->>Editor: Insert ReminderNode into document
Editor-->>User: Reminder chip displayed in text
sequenceDiagram
participant Timer as ReminderTimer
participant Storage as Persistence
participant Backend as Backend
participant Frontend as Frontend Store
participant UI as ReminderToast
Timer->>Storage: readRemindersFile(projectID)
Storage-->>Timer: [reminders]
Timer->>Timer: Check reminderAt vs current time
rect rgba(255, 0, 0, 0.5)
alt Reminder is due
Timer->>Timer: Set triggered=true
Timer->>Storage: writeRemindersFile(projectID, updated)
Timer->>Backend: emit notes++:reminderDue(reminder)
end
end
Backend-->>Frontend: notes++:reminderDue event
Frontend->>Frontend: Add to activeToasts
Frontend->>UI: Render toast with auto-dismiss timer
User->>UI: Dismiss or auto-expire
UI->>Frontend: dismissReminder or markMissed
Frontend->>Backend: dismissReminder(reminderId)
Backend->>Storage: Update reminder.dismissed flag
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
packages/backend/src/index.ts (1)
70-70: Store and call the previous timer disposer to guard against duplicate intervals.
startReminderTimer(sdk)returns a disposer function but its return value is dropped at line 70. Ifinitwere called multiple times (e.g., during plugin reload or re-initialization), interval loops would stack and duplicate reminder checks.🔧 Proposed fix
+let stopReminderTimer: (() => void) | undefined; + export function init(sdk: SDK<API, BackendEvents>) { @@ - startReminderTimer(sdk); + stopReminderTimer?.(); + stopReminderTimer = startReminderTimer(sdk); sdk.console.log("Notes++ backend initialized successfully"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/src/index.ts` at line 70, The call to startReminderTimer(sdk) returns a disposer that is currently ignored; modify the init flow so you store that returned disposer (e.g., in a module-level variable like reminderTimerDisposer) and before assigning a new disposer call the existing one if present to clear the previous interval; update the place that invokes startReminderTimer (function name startReminderTimer and the init routine that calls it) to return and keep the disposer and invoke it on subsequent init/reload to prevent stacked intervals.packages/frontend/src/components/content/editor/reminders/ReminderPicker.vue (1)
61-65: Consider validating that the selected date is in the future.While the
datetime-localinput has a:minattribute for client-side enforcement, programmatic manipulation or edge cases could allow submission of past dates. Adding a validation check would provide defense in depth.🛡️ Proposed fix
function handleConfirm() { const date = new Date(dateValue.value); if (isNaN(date.getTime())) return; + if (date <= new Date()) return; emit("confirm", date); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/components/content/editor/reminders/ReminderPicker.vue` around lines 61 - 65, In handleConfirm(), add a defensive check that the parsed dateValue (new Date(dateValue.value)) is strictly in the future before emitting: compute now = Date.now() and if date.getTime() <= now then abort (return) and optionally emit an error/event or set a validation state; otherwise emit("confirm", date). This ensures programmatic or edge-case submissions of past dates are rejected while keeping dateValue and emit as the points of change.packages/backend/src/utils/reminderFile.ts (1)
27-33: Consider validating parsed JSON against the Reminder schema.The parsed data is cast to
Reminder[]without validation. Ifreminders.jsonbecomes corrupted or malformed, invalid objects could propagate through the system. Since Zod schemas exist for reminders, consider using them here for runtime safety.🛡️ Proposed fix
+import { z } from "zod"; + +const ReminderSchema = z.object({ + id: z.string(), + notePath: z.string(), + context: z.string(), + reminderAt: z.string(), + createdAt: z.string(), + triggered: z.boolean(), + dismissed: z.boolean(), +}); + +const RemindersArraySchema = z.array(ReminderSchema); + export function readRemindersFile(projectID: string): Reminder[] { // ... try { const raw = fs.readFileSync(toSystemPath(filePath), "utf8"); const parsed = JSON.parse(raw); - return Array.isArray(parsed) ? (parsed as Reminder[]) : []; + const result = RemindersArraySchema.safeParse(parsed); + return result.success ? result.data : []; } catch { return []; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/src/utils/reminderFile.ts` around lines 27 - 33, The parsed JSON is being cast to Reminder[] without runtime validation; update the logic in reminderFile.ts to validate `parsed` using the existing Zod reminder schema (e.g., ReminderSchema or reminderSchema) instead of a blind cast: run the Zod parse/safeParse on `parsed`, return the validated array when valid, and fall back to [] (and optionally log the validation error) when validation fails so malformed reminders cannot propagate.packages/frontend/src/repositories/reminders.ts (1)
6-48: Consider using the existinghandleBackendCallutility to reduce repetition.The error handling pattern is duplicated across all four methods. The codebase has a
handleBackendCallutility inutils/backend.tsthat encapsulates this pattern and also displays toast notifications on errors.♻️ Example refactor for one method
+import { handleBackendCall } from "@/utils/backend"; + export const useRemindersRepository = () => { const sdk = useSDK(); async function getReminders() { - const result = await sdk.backend.getReminders(); - if (result.kind === "Error") { - throw new Error(`Error loading reminders: ${result.error}`); - } - return result.value; + return handleBackendCall(sdk.backend.getReminders(), sdk); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/repositories/reminders.ts` around lines 6 - 48, Replace the repeated try/error-check patterns in getReminders, createReminder, deleteReminder, and dismissReminder by delegating to the shared handleBackendCall utility from utils/backend.ts; for each function call handleBackendCall with the corresponding sdk.backend method (e.g., handleBackendCall(() => sdk.backend.getReminders()) for getReminders, handleBackendCall(() => sdk.backend.createReminder(notePath, context, reminderAt)) for createReminder, and similarly for deleteReminder(reminderId) and dismissReminder(reminderId)) and return the resolved value so toast notifications and unified error handling are used instead of manually checking result.kind in each function.packages/backend/src/utils/reminderTimer.ts (1)
45-45: Cleanup function is returned but not captured.The function returns a cleanup callback that stops the interval timer, but the return value is discarded at line 70 in
packages/backend/src/index.ts. While this is acceptable for a long-running plugin, consider capturing and exposing this cleanup function if graceful shutdown is required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/src/utils/reminderTimer.ts` at line 45, The reminderTimer function currently returns a cleanup callback (() => clearInterval(intervalId)) but that return value is discarded where it’s invoked; capture and expose that cleanup function so the app can perform graceful shutdown. Modify the caller in packages/backend/src/index.ts to store the returned cleanup (e.g., const stopReminder = startReminderTimer(...)) and register it with your shutdown handlers (process.on('SIGINT'/'SIGTERM') or the app/plugin shutdown flow) to call stopReminder() during termination; if the function is not exported in reminderTimer.ts, export the timer starter (the function that returns the cleanup) so it can be imported and its cleanup invoked from index.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/backend/src/api/reminder.ts`:
- Around line 43-56: The code accepts reminderAt as any non-empty string and
persists it, which can produce invalid dates; instead, after
createReminderSchema.parse(...) and before constructing the Reminder object in
this handler, parse reminderAt into a Date, verify !isNaN(date.getTime()),
normalize it to an ISO timestamp (e.g. date.toISOString()) and use that
normalized value in the Reminder.reminderAt field; if parsing fails return
error(...) (similar to the existing project error path) so invalid timestamps
are rejected before calling ensureProjectDirectory / writing the file.
In `@packages/backend/src/schemas/reminder.ts`:
- Line 6: Replace the loose validation on reminderAt (currently
z.string().min(1) in the reminder schema) with an ISO 8601 datetime validation
so invalid strings don't produce Invalid Date in reminderTimer.ts; specifically,
update the reminderAt schema entry to use a refinement (e.g.,
z.string().refine(val => !Number.isNaN(Date.parse(val)), { message: 'Invalid ISO
8601 datetime' })) or a strict ISO regex check to ensure Date.parse/new
Date(reminderAt) yields a valid date before saving.
In `@packages/frontend/src/components/content/editor/extensions/reminder-node.ts`:
- Around line 38-57: The close button (.reminder-chip__close) is hidden with
display:none until hover which prevents keyboard users from tabbing to it;
update the CSS selector that reveals the button
(.reminder-chip--cancellable:hover .reminder-chip__close) to also reveal it on
keyboard focus (e.g., include :focus and/or :focus-visible like
.reminder-chip--cancellable:hover .reminder-chip__close,
.reminder-chip--cancellable:focus-within .reminder-chip__close) so the button
becomes tabbable, and update the markup for the icon-only button (the element
using class reminder-chip__close) to include an explicit aria-label attribute
instead of relying on title so assistive tech can announce the action.
In
`@packages/frontend/src/components/content/editor/reminders/showReminderPicker.ts`:
- Around line 22-25: The onConfirm passed into showReminderPicker is async (from
NoteEditor.vue -> createReminder) but is invoked without awaiting before calling
cleanup; update showReminderPicker's signature to accept an async handler
(onConfirm: (date: Date) => Promise<void>) and await the call inside the picker:
await onConfirm(date); then call cleanup(), and consider wrapping the await in a
try/catch to ensure cleanup always runs (cleanup in finally) and to surface/log
errors from onConfirm.
In `@packages/frontend/src/components/shared/reminders/ReminderToast.vue`:
- Around line 92-101: The toast currently only pauses auto-dismiss on mouse
hover (isHovered) so keyboard users tabbing to interactive elements (e.g.,
"Dismiss" or "Go to Note") can still trigger handleExpire(); add focus handling
to pause the countdown by listening for focusin/focusout (or focus/blur) and
setting the same pause state used for hover. Update the root toast element in
ReminderToast.vue (where `@mouseenter`="@isHovered = true" and
`@mouseleave`="@isHovered = false" are set) to also handle `@focusin`="isHovered =
true" and `@focusout`="isHovered = false" (or introduce an explicit isPaused
computed/boolean that ORs isHovered || isFocused and use that for the timer),
and ensure interactive buttons (Dismiss, Go to Note) remain focusable so focus
events fire and prevent handleExpire() while focused.
In `@packages/frontend/src/stores/reminders.ts`:
- Around line 51-66: The UI marks reminders dismissed before persistence: in
dismissReminder(reminderId) either await repository.dismissReminder(reminderId)
before mutating activeToasts, calling setReminderState and emitter.emit, or keep
the optimistic update but add a rollback in the catch that restores
activeToasts, calls setReminderState(reminderId, previousState) and re-emits
reminderStateChanged; in both cases use sdk.window.showToast for errors and
don't leave the UI in the dismissed state when repository.dismissReminder fails.
- Around line 85-88: After hydrating the in-memory registry in
repository.getReminders().then(loadReminderStates).catch(() => {}), broadcast
reminder state-change events so mounted chips update immediately: update the
promise chain in the reminders store to, after calling
loadReminderStates(registryData), iterate the registry entries and emit the
existing reminderStateChanged event (the same event used by reminder-node.ts and
the 15s poll) for each reminder whose state is now set (or simply emit for every
hydrated id) so node views restyle themselves right after hydration; reference
repository.getReminders, loadReminderStates, the registry, and the
reminderStateChanged event when implementing this.
In `@packages/frontend/src/utils/notificationSound.ts`:
- Around line 6-32: The code creates a new AudioContext each time (const ctx in
notificationSound.ts) which can exhaust browser limits and also calls
ctx.resume() without handling rejected promises; fix by caching a singleton
AudioContext (e.g., module-level let cachedCtx and reuse/create only if
!cachedCtx or cachedCtx.state === "closed"), move the play() function to use
that cached ctx (referencing play and frequencies in the diff), and replace the
direct ctx.resume().then(play) with explicit promise handling (await or
.then(...).catch(err => process/log or silently ignore) so resume rejections are
handled); ensure osc/gain nodes are created from the cached ctx and stop/cleanup
per playback to avoid leaks.
---
Nitpick comments:
In `@packages/backend/src/index.ts`:
- Line 70: The call to startReminderTimer(sdk) returns a disposer that is
currently ignored; modify the init flow so you store that returned disposer
(e.g., in a module-level variable like reminderTimerDisposer) and before
assigning a new disposer call the existing one if present to clear the previous
interval; update the place that invokes startReminderTimer (function name
startReminderTimer and the init routine that calls it) to return and keep the
disposer and invoke it on subsequent init/reload to prevent stacked intervals.
In `@packages/backend/src/utils/reminderFile.ts`:
- Around line 27-33: The parsed JSON is being cast to Reminder[] without runtime
validation; update the logic in reminderFile.ts to validate `parsed` using the
existing Zod reminder schema (e.g., ReminderSchema or reminderSchema) instead of
a blind cast: run the Zod parse/safeParse on `parsed`, return the validated
array when valid, and fall back to [] (and optionally log the validation error)
when validation fails so malformed reminders cannot propagate.
In `@packages/backend/src/utils/reminderTimer.ts`:
- Line 45: The reminderTimer function currently returns a cleanup callback (()
=> clearInterval(intervalId)) but that return value is discarded where it’s
invoked; capture and expose that cleanup function so the app can perform
graceful shutdown. Modify the caller in packages/backend/src/index.ts to store
the returned cleanup (e.g., const stopReminder = startReminderTimer(...)) and
register it with your shutdown handlers (process.on('SIGINT'/'SIGTERM') or the
app/plugin shutdown flow) to call stopReminder() during termination; if the
function is not exported in reminderTimer.ts, export the timer starter (the
function that returns the cleanup) so it can be imported and its cleanup invoked
from index.ts.
In
`@packages/frontend/src/components/content/editor/reminders/ReminderPicker.vue`:
- Around line 61-65: In handleConfirm(), add a defensive check that the parsed
dateValue (new Date(dateValue.value)) is strictly in the future before emitting:
compute now = Date.now() and if date.getTime() <= now then abort (return) and
optionally emit an error/event or set a validation state; otherwise
emit("confirm", date). This ensures programmatic or edge-case submissions of
past dates are rejected while keeping dateValue and emit as the points of
change.
In `@packages/frontend/src/repositories/reminders.ts`:
- Around line 6-48: Replace the repeated try/error-check patterns in
getReminders, createReminder, deleteReminder, and dismissReminder by delegating
to the shared handleBackendCall utility from utils/backend.ts; for each function
call handleBackendCall with the corresponding sdk.backend method (e.g.,
handleBackendCall(() => sdk.backend.getReminders()) for getReminders,
handleBackendCall(() => sdk.backend.createReminder(notePath, context,
reminderAt)) for createReminder, and similarly for deleteReminder(reminderId)
and dismissReminder(reminderId)) and return the resolved value so toast
notifications and unified error handling are used instead of manually checking
result.kind in each function.
🪄 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: 6c746c0d-90a8-46c6-97e0-255f13a6cdf8
📒 Files selected for processing (21)
packages/backend/src/api/index.tspackages/backend/src/api/reminder.tspackages/backend/src/index.tspackages/backend/src/schemas/reminder.tspackages/backend/src/types/events.tspackages/backend/src/utils/reminderFile.tspackages/backend/src/utils/reminderTimer.tspackages/frontend/src/components/content/editor/NoteEditor.vuepackages/frontend/src/components/content/editor/extensions/reminder-node.tspackages/frontend/src/components/content/editor/reminders/ReminderPicker.vuepackages/frontend/src/components/content/editor/reminders/showReminderPicker.tspackages/frontend/src/components/shared/reminders/ReminderNotificationManager.vuepackages/frontend/src/components/shared/reminders/ReminderToast.vuepackages/frontend/src/components/shared/reminders/mountReminderNotifications.tspackages/frontend/src/index.tspackages/frontend/src/repositories/reminders.tspackages/frontend/src/stores/reminders.tspackages/frontend/src/utils/eventBus.tspackages/frontend/src/utils/notificationSound.tspackages/frontend/src/utils/reminderStates.tspackages/shared/src/index.ts
| createReminderSchema.parse({ notePath, context, reminderAt }); | ||
|
|
||
| const projectIDResult = await ensureProjectDirectory(sdk); | ||
| if (projectIDResult.kind === "Error") { | ||
| return error(projectIDResult.error); | ||
| } | ||
|
|
||
| const projectID = projectIDResult.value; | ||
| const reminder: Reminder = { | ||
| id: randomUUID(), | ||
| notePath, | ||
| context, | ||
| reminderAt, | ||
| createdAt: new Date().toISOString(), |
There was a problem hiding this comment.
Validate reminderAt as a real timestamp before persisting it.
This currently accepts any non-empty string, so bad values can be written and then parsed with new Date(reminderAt) in the frontend. The result is Invalid Date output or reminders that never transition to due/missed. Parse and normalize the timestamp here before writing the file.
💡 Suggested change
try {
createReminderSchema.parse({ notePath, context, reminderAt });
+ const parsedReminderAt = new Date(reminderAt);
+ if (Number.isNaN(parsedReminderAt.getTime())) {
+ return error("Invalid reminderAt");
+ }
@@
const reminder: Reminder = {
id: randomUUID(),
notePath,
context,
- reminderAt,
+ reminderAt: parsedReminderAt.toISOString(),
createdAt: new Date().toISOString(),
triggered: false,
dismissed: false,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/backend/src/api/reminder.ts` around lines 43 - 56, The code accepts
reminderAt as any non-empty string and persists it, which can produce invalid
dates; instead, after createReminderSchema.parse(...) and before constructing
the Reminder object in this handler, parse reminderAt into a Date, verify
!isNaN(date.getTime()), normalize it to an ISO timestamp (e.g.
date.toISOString()) and use that normalized value in the Reminder.reminderAt
field; if parsing fails return error(...) (similar to the existing project error
path) so invalid timestamps are rejected before calling ensureProjectDirectory /
writing the file.
this PR solves #20
Summary by CodeRabbit