Tasks first paint: last-known projects, folders, and sidebar summary - #1677
Conversation
lnittman
changed the title
|
🚨 SLOP COP 🚨 · I am SlopCop. I am reviewing this PR for security, code quality, performance, architecture, duplication, and end-to-end behavior. I will post one final review when the checks finish. |
![bb-slop-cop[bot]](https://avatars.githubusercontent.com/in/4577281?s=60&v=4){kind=small}
| // does. | ||
| const routeScopeJustChanged = previousRouteScope.current !== routeScope; | ||
| previousRouteScope.current = routeScope; | ||
| if (!routeScopeJustChanged && !tasksQuery.isLoading) { |
There was a problem hiding this comment.
🚨 slopcop/review — A failed scope request settles with another scope’s data
useTasksQuery keeps the prior data when a request fails and sets isLoading to false. This effect then marks the new route as settled. Because data remains defined, the body skips its error state and renders the prior route’s tasks under the new project or Active route. A user can then edit tasks in the wrong context. Please bind each result to its query scope or clear prior-scope data after a failed scope change. Add a route-change test that rejects the new fetch and shows an error with no old rows.
| (data) => { | ||
| if (seq !== seqRef.current) return; | ||
| const snapshot = snapshotRef.current; | ||
| if (snapshot !== undefined) writeQuerySnapshot(snapshot.name, data); |
There was a problem hiding this comment.
🚨 slopcop/review — Coordinate snapshot writes across hook instances
The sequence guard belongs to one hook instance, but this storage key is shared. The shell, list, and task dialog can call useProjects at the same time. A slow old hook can resolve after a newer hook and replace the newer snapshot. Cleanup also does not invalidate the old request. Please use a shared per-key revision or a shared query owner. Add a test that resolves the newer request first and confirms that the older request cannot replace its snapshot.
| // Skeleton only while there is nothing to draw: a refetch (manual | ||
| // refresh, invalidation) keeps the last-known rows on screen, and a | ||
| // snapshot-hydrated mount never shows the placeholder at all. | ||
| isLoading={ |
There was a problem hiding this comment.
🚨 slopcop/review — Keep usable projects visible after a companion request fails
A cold folder or summary failure leaves that query’s data undefined after loading ends. This condition then remains true forever, and TasksSidebar hides the loaded project rows behind a skeleton. Please distinguish an active load from a settled failure and render available projects without unavailable folders or counts. Add a test where projects succeed, the summary or folder request fails, and project navigation remains usable.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain-English summary: This PR stores the last Tasks sidebar state in browser storage. A reopened Tasks panel can show known projects and counts immediately. It also hides stale list results while the user changes routes.
I found three material defects:
- A failed route fetch can show tasks from the prior route. A user could edit a task under the wrong project context.
- Independent query hooks can write shared snapshots out of order. An older response can replace a newer browser snapshot.
- A folder or summary failure can leave the project sidebar behind a permanent skeleton. Loaded projects then become unavailable.
Please fix these cases and add the tests described in the inline comments.
I found no confirmed security defect. Zod validates cached data, and React escapes stored names. I found no measured performance blocker.
The plugin cannot use the host query client directly. A shared Tasks query owner is the clean refactor. It can remove duplicate requests and coordinate snapshot writes.
The Tasks type check passed. All 28 focused shell tests passed. GitHub package tests also passed. The full local suite had unrelated timeouts during heavy parallel load. Isolated reruns passed.
The development launcher did not become ready during its plugin SDK build. I stopped it, so I could not complete the browser route test.
The current CI failure comes from the stale-base package-version guard. This PR also needs a rebase before merge.
…y state The shell collapsed 'projects unknown' and 'no projects' into one boolean, so every mount rendered the list chrome and skeleton, then swapped to the empty state once listProjects resolved. And useTasksQuery kept nothing between mounts, so each visit started from a blank slate. - useTasksQuery gains an opt-in persisted snapshot (localStorage, versioned key, validated against the query's own RPC output schema on read; written only after a successful fetch). folders, projects, and sidebar summary opt in. - The sidebar skeleton and the empty state now gate on data presence, not the in-flight flag: a hydrated mount paints last-known truth immediately, a refetch keeps rows on screen, and emptiness only paints once it is known. - manage.test clears localStorage between tests like shell.test already did.
…in tests The snapshot key carries a version so a shape change is never read as trusted data, but entries written under an older version would sit in the profile forever; drop them once per page load on first snapshot access. Mounting the shell now persists last-known sidebar data, so every test starts from a cold profile via the shared vitest setup instead of per-file clears.
…pe change loads Switching between All tasks, Active, and a project reuses the same ListView instance, and its query keeps the previous scope's result while the new scope fetches, so returning from an empty Active to All flashed "No tasks yet" (Active's emptiness presented as All's truth) for the length of the fetch. The existing settled-scope signal only fed scroll restoration, and it was a ref, so settling could not rerender anything anyway. The scope-settled signal is now state and gates the body: while a changed scope is in flight the list reads as loading, and only a settled result may claim the scope is empty or broken (a held error is the previous scope's too). Settling also skips the commit that changes the scope: the query's own effect flips isLoading in that same commit, but sibling effects still read the previous render's value, so that commit must never settle the new scope with the old scope's data. Covered by an All-to-Active-to-All test with the return fetch deferred, which fails against the old gate.
…dy have The scope gate landed too wide: it keyed on the scroll scope, which includes filters and sort, so toggling a filter or reordering flashed loading rows over data the view already held (sort is client-side and never even refetches). The body now gates on the route scope alone, the fetch identity across All, Active, and a project, which is the boundary where held data belongs to another view; the scroll-restoration signal returns to its original full-scope form.
daf1400 to
2a44df0
Compare
- A failed fetch for a changed query scope no longer keeps the previous scope's rows: useTasksQuery tracks which deps key its data belongs to and drops it when a different key's request fails, so ListView shows the error instead of the prior route's tasks. - Snapshot writes are monotonic across hook instances: each request claims a per-name revision at start and writeQuerySnapshot ignores responses that resolve after a later request already wrote. - The sidebar skeleton clears once every query has a first answer (data or error), so a folders/summary failure cannot hide loaded projects; projects whose folder is unknown render ungrouped instead of vanishing. Co-Authored-By: Claude <noreply@anthropic.com>
|
Pushed 85a4ffc addressing the three SlopCop findings (review was against daf1400; the sidebar gate now lives in
All four new tests fail on the previous head and pass now;
|
2 participants
Part of #1676 (item 1).
Tasks keeps an opt-in last-known snapshot for folders, projects, and the sidebar summary in
useTasksQuery(localStorage, versioned key, validated against the query's own RPC output schema on read, written only after a successful fetch) and seeds the query state from it on mount, so a repeat visit paints the last resolved state immediately; the sidebar skeleton shows only while that data is absent. The empty state still waits for a resolved empty list. Snapshots from older storage versions are pruned once per load; the shared vitest setup clears storage per test.Plugin code only (
plugins/tasks); no SDK or host changes.isLoadingsemantics are untouched (the list view's scroll restoration depends on its flip-on-refetch behavior).