Re-home project avatar customization onto the workspace context menu - #213
Re-home project avatar customization onto the workspace context menu#213Zeus-Deus wants to merge 2 commits into
Conversation
PR #198 replaced the nested sidebar project tree with the flat workspace inbox. The project image/color menu lived only on the project-group header, so unmounting the tree took it with it — `sidebar-project-group.tsx` has had zero non-test importers since, and nothing in the shipping UI could set or clear a project avatar. Saved avatars still rendered; they just couldn't be changed. Nothing had been deleted, so this re-homes the entry point rather than rebuilding the feature. Right-clicking any workspace in the inbox (active card or settled row) now shows a `Project "<name>"` submenu between the workspace actions and the device actions, offering the same `ProjectImageDialog` and the same 12-color palette. Each inbox row already carried a `repo: {name, path}` for its avatar, so resolving the owning project needed no new plumbing; the submenu is labeled with the project name because the setting applies to the whole project, not the clicked row. Appearance state moves into a shared `project-appearance-store.ts`. The old `useProjectAppearance` hook read the DB per component with no invalidation, so re-homing it as-is would have left a project's other cards stale until remount — visible immediately, since the inbox shows several workspaces per project. One writer, many readers means every card, settled row, rail button, and filter-dropdown entry of a project repaints together. The store also dedupes the read (one round-trip per project, not per mounted avatar) and keeps an in-flight read from clobbering a value the user just picked. `WorkspaceContextMenuItems` takes the project section as a `projectMenu` slot, so that module stays workspace-scoped. `PROJECT_COLORS` now has a single home; the unmounted project group imports it so the palettes can't drift. Archive Project was the other casualty of the same unmounting and is still unreachable; it is left for a follow-up.
Review — Opus 5 (high effort)Right diagnosis and the right response — re-homing an orphaned entry point rather than rebuilding the feature. The shared store is a genuine improvement over per-component reads, The load race guard throws away more than it needs toset((s) => {
// A write that landed while this read was in flight wins
if (s.byPath[projectPath]) return s;
...
});The intent is right, but the granularity is wrong: it discards the entire read, not just the field the write touched. So if
const existing = s.byPath[projectPath];
return { byPath: { ...s.byPath, [projectPath]: {
customColor: existing?.customColor ?? (color || null),
imageUrl: existing?.imageUrl ?? (image || null),
imageVersion: existing?.imageVersion ?? (version || null),
}}};(With the caveat that
|
|
Review by GPT 5.6 Sol xhigh found an in-flight load data-loss race. If The existing slow-load test only asserts that the changed color is not clobbered, so it misses the loss of the other fields. Track dirty fields/revisions and merge untouched persisted fields, or await/dedupe the initial load before applying a field-level write. Add both race directions with the other persisted field populated. The focused appearance suite passed 12/12 tests, confirming this is an uncovered interleaving. Coordination note: this head has pairwise textual conflicts with #206, #207, and #208. |
… load The load guard bailed out entirely if any entry existed for the project, so a setColor/setImage that landed while the initial read was in flight dropped the other persisted fields for the session (e.g. a color pick during load lost the saved image until restart). Track explicitly written fields per project and have load merge: written fields keep their in-store values, untouched fields take the persisted ones. A dirty-field set rather than a null-coalescing merge, since null is a legitimate written value (clearing a color must survive the load). Load dedupe and the frozen EMPTY_APPEARANCE snapshot are unchanged. Adds both race-direction tests and corrects the docs note about when persisted values are read (once per session, not per mount).
|
Verified: the load-race issue from review is genuinely fixed — the appearance store now keeps a per-project dirty-field set and merges the persisted read field-by-field (written fields win, including explicit Blocked on a rebase now that #206 merged — the sidebar files this PR touches ( |
The regression
PR #198 replaced the nested sidebar project tree with the flat workspace inbox. The project image/color menu lived only on the project-group header, so unmounting the tree took it with it —
sidebar-project-group.tsxhas had zero non-test importers since, and nothing in the shipping UI could set or clear a project avatar.The repo's own docs already recorded this.
docs/features/project-avatars.mdsaid "Customization is currently unreachable from the UI", anddocs/features/sidebar.mdlisted it under Current Constraints.Nothing had been deleted —
ProjectImageDialog, the palette,resolveImageUrl, the favicon cache-busting, and the persistence keys were all intact and tested, just orphaned. So this re-homes the entry point rather than rebuilding the feature.What changed
Right-clicking any workspace in the inbox (active card or settled row) now shows a
Project "<name>"submenu, between the workspace actions and the device actions. It offersSet image…— the same dialog as before — and the same 12-color accent palette.Each inbox row already carried a
repo: {name, path}for its avatar, so resolving "which project is this workspace from" needed no new plumbing. The submenu is labeled with the project name because the setting applies to the whole project, not the one row that was clicked.Beyond a straight port: live propagation
The old
useProjectAppearancehook read the DB per component with no invalidation. Re-homed as-is, setting an image on one card would have left the project's other cards stale until remount — visible immediately, since the inbox shows several workspaces per project.Appearance state now lives in a shared
src/stores/project-appearance-store.ts: one writer, many readers. Every card, settled row, rail button, and filter-dropdown entry of a project repaints together. The store also dedupes the read (one round-trip per project, not per mounted avatar) and keeps an in-flight read from clobbering a value the user just picked.Structure
WorkspaceContextMenuItemstakes the project section as aprojectMenuslot, so that module stays workspace-scoped and free of project-appearance concerns.ProjectImageDialogis owned byWorkspaceInboxMenu, outside theContextMenusubtree — selecting the menu item unmounts the menu, which would otherwise tear the dialog down with it.PROJECT_COLORSnow has a single home; the unmounted project group imports it so the two palettes can't drift.Verification
npm run checkclean.project-appearance-menu.test.tsxcover menu rendering, the image-vs-color entry points, persistence key/value shape (including clear-writes-empty), cross-surface propagation, read dedup, and the slow-load-vs-user-write race.npm run dev: right-click → submenu → dialog, and a color pick repainting all fourcodemuxavatars at once, including the three never clicked.cargo checkfails in this worktree on a missingagent-browsersidecar binary — a fresh-checkout setup gap (src-tauri/binaries/is gitignored and its postinstall was blocked), unrelated to this change, which touches no Rust.Docs
Updated
docs/features/project-avatars.md,docs/features/sidebar.md,docs/core/STATUS.md, anddocs/INDEX.md— all four carried the now-stale "unreachable" claim.Follow-up
Archive Project was the other casualty of the same unmounting and is still unreachable. Left for a separate PR.