feat(tasks): link canvases from the Related picker - #1879
Conversation
github-actions
Bot
added
documentation
|
React Doctor found 1 new issue in 1 file · 1 warning · score 93 / 100 (Great) · 1 fixed · vs 1 warning
Reviewed by React Doctor for commit |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Follow-ups after the first CI runA regression CI caught that my targeted test run had missed. Test coverage for the new hook ( Worth flagging how that test was validated, because the first two versions of it passed against deliberately broken code. React Doctor's CI status. Everything this PR owns is green. The one red check, |
a33fc2f to
96d9191
Compare
Storage and contract groundwork for listing canvases alongside notes in the task drawer's Related picker (#1849). `task_canvases` mirrors `task_notes` column for column, including the absence of a foreign key to `canvases` so sync apply order stays irrelevant. The new `linkedCanvasIds` field is optional at every type site, which keeps existing task fixtures compiling and lets an older client omit the key entirely.
Asserts an existing task and its `task_notes` rows survive the migration untouched, that the statements are re-runnable, and that the table cascades from `tasks` while holding no foreign key to `canvases`.
Adds `linkedCanvasIds` to the task handler's three receive branches, to `buildPushPayload` and to `seedUnclocked`, and to the sync client's shared junction enrichment. Every receive-side write is guarded, so a payload that omits the key preserves local links while an explicit empty array still clears them. That guard is what keeps an older client, whose non-strict payload schema drops the key on parse, from erasing canvas links when it pushes a task back. Merge unions both sides the way tags and note links already do; `linkedCanvasIds` is deliberately not a field-clocked column.
The picker now offers canvases alongside notes, each with its own icon, and a
linked canvas opens as a canvas tab. Related references are modelled as a
discriminated `{ kind, id }` union at the UI layer because a note and a canvas
can share an id, which a flat list of ids could not tell apart.
Note results now come from full-text search instead of a single fetch of the 50
most recent notes filtered in memory, so an older note is findable by title. An
empty query still falls back to recent notes. Canvases are matched client-side
because search does not index them.
…ounce The empty query has no keystrokes to coalesce, so debouncing it only delayed the first paint behind the results from the previous time the picker opened.
enrichTask now reads the task_canvases junction, so the partial mock of @main/database/queries/tasks has to expose getTaskCanvasIds/setTaskCanvases or every handler test fails on the missing export. GET asserts a real canvas id so the enrichment is covered rather than defaulted.
The hook's two sources settle independently so one failing cannot blank the other, which was asserted in a comment and nothing else. The failure cases reject on a timer rather than with mockRejectedValue: an immediate rejection settles before the sibling's .then chain, which hid a handler that cleared its sibling's results.
96d9191 to
d53f190
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| <span className="flex-1 min-w-0 text-[12px] text-text-secondary leading-4 truncate"> | ||
| {info?.title ?? t('drawer.loading')} | ||
| </span> | ||
| <button |
There was a problem hiding this comment.
React Doctor · react-doctor/no-transition-all (warning)
Your users see janky animation because transition-all animates every property that changes, including expensive layout ones and instant ones like focus rings. Name the properties: transition-colors, transition-opacity, or transition-transform.
Fix → List the specific properties: transition: "opacity 200ms, transform 200ms". In Tailwind, use transition-colors, transition-opacity, or transition-transform
1 participant
Closes #1849
Behavior change
The task drawer's Related picker listed only notes, so a canvas could not be linked to a task at all. It now lists canvases alongside notes, each with its own icon, and clicking a linked canvas opens it in a canvas tab.
The same pass fixes the second defect the issue flags: the picker fetched the 50 most recently modified notes once and filtered that array in memory, so a note outside that window was unfindable no matter what you typed. Note results now come from the existing FTS index (
search:query), so the whole vault is reachable. The empty-query case still lists recent notes, so opening the picker looks the same as before.Storage design
linkedNoteIdsis not a column — it is thetask_notesjunction table. The canvas half mirrors it as a newtask_canvasesjunction, withlinkedCanvasIdsadded to the sync payload besidelinkedNoteIds.linkedNoteIdsis untouched and no canvas id ever enters it.Two alternatives were rejected on backward-compatibility grounds, not taste:
linkedCanvasIdsas a JSON column ontasksdescription: data.description ?? null), so an absent key clears the columntask_links(task_id, item_type, item_id)linkedNoteIdswould union-merge against a table it can only see half of. Also forces migrating existingtask_notesrowstask_canvasesjunction mirroringtask_notesAt the UI layer the two disjoint fields are unioned into a discriminated reference (
{ kind: 'note' | 'canvas'; id }) withkind:idReact keys. A note and a canvas can share an id, so a barestring[]would have been ambiguous.linkedCanvasIdsis deliberately not added toTASK_SYNCABLE_FIELDS. Junction relations carry no field clocks; they are union-merged, exactly liketagsandlinkedNoteIds.Migration
apps/desktop/src/main/database/drizzle-data/0055_task_canvases.sql, hand-written (the Drizzle generator has been off since 0020;db:generatewas not run). Journal entry 55 hand-appended, no meta snapshot.Additive only: one
CREATE TABLE IF NOT EXISTS, noALTER, no backfill, noDELETE. Existing installs upgrade by gaining an empty table; everytasksrow and everytask_notesrow is left exactly as it is.It mirrors
task_notescolumn for column including the absence of a foreign key tocanvases. A FK there would make sync apply order load-bearing: a task arriving before the canvas it links to would fail the whole upsert. TheON DELETE CASCADEFK totasksstays so deleting a task reclaims its links.Older-client analysis
The issue asked whether a round-trip through an old client drops the new field. Traced end to end:
linkedCanvasIds.TaskSyncPayloadSchemais a plain non-strictz.object, so Zod strips the unknown key. The task applies normally — it does not reject. This matters: a schema failure returns'skipped', which advances the cursor permanently. The schema is deliberately left non-strict.if (data.linkedCanvasIds), so an absent key preserves the local rows. Canvas links survive the round-trip.An explicit
linkedCanvasIds: []still clears, which is how a real unlink travels. Absent means "no information"; empty means "none". That distinction is what makes the round-trip safe, so it is covered by tests in both the merge and plain-apply branches.Rollback is inert: an older build never reads
task_canvases, and its migrator's max journalwhenis lower, so it applies nothing and never drops the table.One inherited property worth naming: junction merges are unions, so a concurrent unlink loses to a link. Canvas links behave exactly like note links here. This is pre-existing for notes and was matched rather than diverged from; changing it needs per-link tombstones and belongs in its own issue.
Verification
preserves local canvas links when an older client omits linkedCanvasIds on APPLYand nothing else. Removing the merge-branch guard and union fails exactly the two MERGE tests. The guards are load-bearing and the tests prove it.leaves an existing task and its note links untouched when upgrading, idempotent re-execution,cascades on task delete and has no foreign key to canvases), the three compat tests above, union-merge, and both push paths.lists notes and canvases together in the related pickerandlinks a canvas from the picker and opens the canvas when its row is clicked.pnpm lint0 · desktoptypecheck:web/typecheck:node/typecheck:test0 ·pnpm ipc:generate && pnpm ipc:check0 with no regenerated diff ·git diff --checkclean ·docs:impact --strictanddocs:build0.tsconfig.test.*exclude backlog.Notes for the reviewer
pnpm ipc:generateneeds Node 24 (--experimental-transform-typeswas removed in Node 26); the native modules are built for the Node running the tests.pnpm typecheckcurrently fails onapps/mobile/vitest.config.ts -> node:url, an architecture-boundary violation present onorigin/mainand untouched by this branch.mainis independently red on Desktop CI and an E2E shard as of the feat(mobile): UI foundation and the four search screens #1863 merge.task-linked-note-indicatorstill count only note links, so a task linked solely to a canvas shows no badge. Cosmetic, and widening those is a separate change.