Skip to content

Re-home project avatar customization onto the workspace context menu - #213

Open
Zeus-Deus wants to merge 2 commits into
mainfrom
restore-project-context-menu
Open

Re-home project avatar customization onto the workspace context menu#213
Zeus-Deus wants to merge 2 commits into
mainfrom
restore-project-context-menu

Conversation

@Zeus-Deus

Copy link
Copy Markdown
Owner

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.tsx has 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.md said "Customization is currently unreachable from the UI", and docs/features/sidebar.md listed 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 offers Set 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 useProjectAppearance hook 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

  • WorkspaceContextMenuItems takes the project section as a projectMenu slot, so that module stays workspace-scoped and free of project-appearance concerns.
  • ProjectImageDialog is owned by WorkspaceInboxMenu, outside the ContextMenu subtree — selecting the menu item unmounts the menu, which would otherwise tear the dialog down with it.
  • PROJECT_COLORS now has a single home; the unmounted project group imports it so the two palettes can't drift.

Verification

  • npm run check clean.
  • Full frontend suite: 3063 tests across 210 files pass.
  • 12 new tests in project-appearance-menu.test.tsx cover 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.
  • Visually confirmed end-to-end against npm run dev: right-click → submenu → dialog, and a color pick repainting all four codemux avatars at once, including the three never clicked.

cargo check fails in this worktree on a missing agent-browser sidecar 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, and docs/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.

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.
@Zeus-Deus

Copy link
Copy Markdown
Owner Author

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, EMPTY_APPEARANCE as a frozen module singleton correctly avoids the fresh-object-per-render snapshot trap, and owning ProjectImageDialog outside the ContextMenu subtree is the correct fix for the unmount-on-select problem.

The load race guard throws away more than it needs to

set((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 setColor lands before load resolves, the project's persisted imageUrl and imageVersion are dropped from the store for the rest of the session — the avatar loses its custom image and falls back to a colour swatch, until the app restarts and reads them again.

ProjectAppearanceMenu calls useProjectAppearance(projectPath) on mount, so the load starts when the submenu opens and will normally have resolved by the time the user reaches a colour item — this needs you to beat one dbGetUiState round trip. Narrow, but the failure is confusing (image vanishes) and the fix is small: merge per key, keeping whatever the write already set.

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 null is a legitimate written value, so this wants an explicit "which fields were written" marker rather than ?? if you want it airtight.)

loadStarted is never invalidated, which drops refresh-on-mount

The old hook re-read on every mount; the store reads once per process and caches forever (loadStarted only clears in the test helper). For a single-window app with one writer that's fine and it's the point of the dedupe.

Worth flagging because #212 leans on the old behaviour in a code comment — "Bumped on each open so appearance edits made elsewhere in the session are picked up, matching useProjectAppearance's refresh-on-mount contract" — and that contract no longer exists after this PR. Whichever lands second needs its comment corrected.

One dialog per row

ProjectImageDialog is rendered inside every WorkspaceInboxMenu, i.e. once per workspace card and settled row. A closed Radix dialog renders nothing so the cost is negligible, but it makes per-workspace state (showImageDialog) own a per-project setting, and N instances all read the same useProjectAppearance(repo.path). Not worth changing on its own; worth knowing if the menu grows more project-level dialogs (Archive Project, per the follow-up note).

Overlap

sidebar-inbox.tsx / sidebar-inbox-card.tsx are also edited by #206 and #208, and workspace-inbox-menu.tsx by #206. #206's version of WorkspaceInboxMenu adds snooze entries to the same menu this PR adds a projectMenu slot to.

@Zeus-Deus

Copy link
Copy Markdown
Owner Author

Review by GPT 5.6 Sol xhigh found an in-flight load data-loss race.

If setColor runs while the initial load(projectPath) is awaiting its three reads, the setter creates a byPath entry from EMPTY_APPEARANCE. When the reads resolve, load sees that any entry exists and returns the whole state unchanged (project-appearance-store.ts:68-81). The newly selected color is protected, but an untouched persisted image and image version are discarded for the rest of the session. The inverse happens when setImage races the load: the untouched persisted color is lost.

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).
@Zeus-Deus

Copy link
Copy Markdown
Owner Author

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 null clears), with regression tests covering both race directions. CI was green on the head.

Blocked on a rebase now that #206 merged — the sidebar files this PR touches (sidebar-inbox.tsx, sidebar-inbox-card.tsx, and friends) changed on main. Expected to be a mostly textual rebase; the appearance-store and context-menu work itself is untouched by the conflict.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant