test(reminders): cover target title + existence resolution paths - #239
Merged
Conversation
Phase 5.4 (ref .claude/plans/tech-debt-remediation.md § 5.4). The resolveReminderTarget helper landed earlier as part of the calendar sync work (e81ba54) so the refactor portion of 5.4 is already on main. This PR closes the coverage gap it left behind. Adds a "target title resolution" describe block with five cases: - note reminder resolves title from noteCache - journal reminder uses targetId as title (no index db lookup) - missing note → targetTitle null + targetExists false - highlight with note present → highlightExists true - highlight with missing note → highlightExists false
Closes the 5.4 refactor: the test-only PR assumed the calendar-sync work had landed the refactor too, but origin/main still had toReminderWithTarget stubbed with targetTitle: null and a duplicated resolveReminderTarget in the IPC handler. - Adds resolveReminderTarget(reminder, indexDb) in reminders.ts that handles note/journal/highlight uniformly and returns targetTitle + targetExists + highlightExists. - Wires toReminderWithTarget(row, indexDb) to call the helper; updates getReminder, listReminders, getDueReminders to pass the indexDb through. - getReminder now returns ReminderWithTarget (was Reminder) since the handler always wrapped it anyway. - Removes the duplicated resolveReminderTarget + getIndexDb from reminder-handlers.ts; list/get/get-upcoming/get-due handlers now just delegate to the service. - Drops the now-dead resolveTargetTitle helper + the scheduler's manual reminder.targetTitle = resolvedTitle mutation (now set upstream by toReminderWithTarget inside getDueReminders).
Follow-up to the reminders.ts refactor: the IPC handler used to wrap
each service result with its own resolveReminderTarget(); now that the
service does that itself, the two handler tests ("resolves reminder
targets on get and list" and "handles upcoming...") are rewritten as
pure pass-through assertions — they feed the mocked service a
pre-resolved ReminderWithTarget and assert the handler forwards it
verbatim.
Also drops the notesQueries mock + import since the handler no longer
reaches into the notes cache.
h4yfans
added a commit
that referenced
this pull request
May 6, 2026
* test(reminders): cover target title + existence resolution paths Phase 5.4 (ref .claude/plans/tech-debt-remediation.md § 5.4). The resolveReminderTarget helper landed earlier as part of the calendar sync work (e81ba54) so the refactor portion of 5.4 is already on main. This PR closes the coverage gap it left behind. Adds a "target title resolution" describe block with five cases: - note reminder resolves title from noteCache - journal reminder uses targetId as title (no index db lookup) - missing note → targetTitle null + targetExists false - highlight with note present → highlightExists true - highlight with missing note → highlightExists false * refactor(reminders): resolve target title in service helper Closes the 5.4 refactor: the test-only PR assumed the calendar-sync work had landed the refactor too, but origin/main still had toReminderWithTarget stubbed with targetTitle: null and a duplicated resolveReminderTarget in the IPC handler. - Adds resolveReminderTarget(reminder, indexDb) in reminders.ts that handles note/journal/highlight uniformly and returns targetTitle + targetExists + highlightExists. - Wires toReminderWithTarget(row, indexDb) to call the helper; updates getReminder, listReminders, getDueReminders to pass the indexDb through. - getReminder now returns ReminderWithTarget (was Reminder) since the handler always wrapped it anyway. - Removes the duplicated resolveReminderTarget + getIndexDb from reminder-handlers.ts; list/get/get-upcoming/get-due handlers now just delegate to the service. - Drops the now-dead resolveTargetTitle helper + the scheduler's manual reminder.targetTitle = resolvedTitle mutation (now set upstream by toReminderWithTarget inside getDueReminders). * test(reminders): align handler tests with service-owned title resolution Follow-up to the reminders.ts refactor: the IPC handler used to wrap each service result with its own resolveReminderTarget(); now that the service does that itself, the two handler tests ("resolves reminder targets on get and list" and "handles upcoming...") are rewritten as pure pass-through assertions — they feed the mocked service a pre-resolved ReminderWithTarget and assert the handler forwards it verbatim. Also drops the notesQueries mock + import since the handler no longer reaches into the notes cache.
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
Phase 5.4 (ref
.claude/plans/tech-debt-remediation.md § 5.4).The title-resolution refactor the plan called for actually landed earlier, as part of commit
e81ba54f feat(calendar): enhance synchronization and projection updates:apps/desktop/src/main/lib/reminders.tsalready has aresolveReminderTarget(reminder, indexDb)helper dispatching ontargetType.apps/desktop/src/main/ipc/reminder-handlers.tsno longer duplicates lookup logic — it delegates straight to the service.So the remaining polish here is the explicit unit-test coverage the plan asked for. This PR adds a
target title resolutiondescribe block inreminders.test.tswith five cases:targetTitlefrom cache,targetExists: truetargetTitle === targetId(date), no index-db lookuptargetTitle: null,targetExists: falsehighlightExists: truehighlightExists: falseTest plan
pnpm exec tsc -p tsconfig.node.json --noEmitclean (exit 0)pnpm test --filter @memry/desktopgreen on the new casesFollow-up
Once merged, update the § 5.4 Progress Tracker entry in the remediation plan to reflect that the refactor + coverage are both shipped.