[malibio]

Closes #2813

Switch visibility (component/token level)

• New --switch-track token for the unchecked track, used by switch.svelte instead of --input. Clears WCAG 1.4.11 (3:1) in both themes: light 3.57:1 on card (was 1.25:1 — deliberate improvement), dark 3.50:1 on card (was near-invisible). Thumb vs track in dark: 3.56:1.
• --input is unchanged, so input.svelte / textarea.svelte are unaffected.

Labs

• Team synchronization switch: visible, disabled (not focusable, can't be toggled by click or keyboard); AI Chat toggle unchanged.
• developer@nodespace.ai is a real mailto: anchor (no {@html}). It uses text-foreground + underline rather than text-primary: per app.css, primary is 3.67:1 in light and not meant for small body text.

Sidebar

• database-switcher.svelte deleted. databaseStore.load() it ran on mount moved to the sidebar's onMount.
• Favorites and Conflicts omitted from the nav items, with a comment following the AI Chat precedent. openConflicts() and the conflict journal are untouched; the per-node conflict badge still opens it.
• New reusable nav-item badge (navBadges map keyed by item id; nav items now render through one navItemButton snippet). Settings gets an alert badge when the active database is missing. The glyph sits on the icon, so it shows in the collapsed rail too, and its text is added to the button's accessible name ("Settings — Active database is missing"), so the alert isn't conveyed by colour alone.

Verification

• Tests: new navigation-sidebar-nav-items.test.ts (hidden items, badge present/cleared, expanded+collapsed, click still opens Settings); Labs and layout tests updated.
• Checked in the running app (isolated daemon) in both themes: computed track colours match the tokens, disabled switch at 0.5 opacity is still visible in dark, clicking and pressing Space doesn't change it, AI Chat still toggles, badge shows in expanded and collapsed sidebar after setting a missing status.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review — PR #2932

Review Type: Initial Review

Summary

A clean, well-scoped change that does what the issue asks, and in a few places does it better (token-level fix, one shared nav-item snippet, badge text in the accessible name). The PR has one commit and every change serves #2813. I checked the contrast numbers against the tokens (light hsl(240 5% 55%) ≈ 3.6:1 on white; dark hsl(240 4% 45%) ≈ 3.5:1 on --card, ≈ 3.56:1 vs the --background thumb). The three affected test files pass locally (58/58). I found no blockers. The findings below are small UX and accuracy follow-ups.

Requirements Check

1. Switch visibility

• ✅ Unchecked switch is visible in dark mode (track 3.50:1 on card, thumb 3.56:1 vs track)
• ✅ Checked (bg-primary) stays distinct from unchecked (neutral grey) in both themes
• ✅ Light mode is deliberately improved (1.25:1 to 3.57:1), not regressed
• ✅ Meets WCAG 1.4.11 (3:1) on its typical surface
• ✅ Fixed at token + component level (--switch-track, switch.svelte), so every Switch benefits
• ✅ --input is unchanged, so input.svelte / textarea.svelte are unaffected and need no re-check

2. Team sync email

• ✅ A working mailto: link
• ✅ Real markup, no {@html}
• ✅ Legible in both themes. ⚠️ It uses text-foreground + underline, not the text-primary link in account-settings.svelte. The deviation is justified (primary is 3.67:1 in light and too faint for small text) but not quite "consistent" (see 🟢 below)

3. Team sync toggle disabled

• ✅ Visible, and neither click nor keyboard toggles it (native disabled)
• ✅ Disabled state is evident in both themes (author checked dark mode at opacity-50)
• ✅ Not focus-reachable; AT reports it as disabled
• ✅ syncEnabled can't be flipped from this UI (no onCheckedChange wired)
• ✅ AI Chat toggle stays interactive (asserted in test)

4/5. Database switcher, Favorites, Conflicts

• ✅ database-switcher.svelte deleted and no references remain; its databaseStore.load() moved to the sidebar's onMount
• ✅ The missing state now shows on Settings (item 6)
• ✅ Favorites and Conflicts are omitted, with a comment explaining why and what brings them back (follows the AI Chat precedent)
• ✅ openConflicts(), open-conflicts.ts, pane-content conflicts tab and conflict-indicator-badge are intact
• ✅ Settings is still reachable
• ✅ Tests updated (layout.test.ts now asserts the exact id list, which is stronger than the old length check)

6. Missing-database badge

• ✅ Shows when status === 'missing', clears otherwise (tested)
• ✅ Visible in expanded and collapsed sidebar (the glyph sits on the icon; tested both ways)
• ✅ Legible in both themes (destructive glyph on a --sidebar-background disc)
• ✅ Conveyed accessibly: aria-label = "Settings — Active database is missing", which starts with the visible label (so WCAG 2.5.3 holds); glyph is aria-hidden
• ✅ Reusable: a navBadges map keyed by item id plus a single navItemButton snippet
• ✅ Clicking a badged Settings item still opens Settings (tested)

Verification

• ✅ Author checked the running app in both themes (computed track colours, disabled behaviour, badge expanded and collapsed)

Code Review Findings

🔴 Critical

None.

🟡 Improvements

• 🟡 The badged Settings item lands on the default section, not Settings → Database.
  📁 packages/desktop-app/src/lib/components/layout/navigation-sidebar.svelte:291
  The deleted switcher called openSettings('database'), so one click from the missing-DB alert reached the place to fix it. Now it calls openSettings(), and the user has to find the Database section after following an error signal. The AC ("opens Settings as normal") is still met, but the recovery path takes an extra step. Suggested fix: let NavBadge carry an optional target, e.g. { label, category: 'database' }, and pass it through: openSettings(navBadges.settings?.category). The badge would then own its call-to-action and stay reusable (a future Conflicts badge would need the same thing). Principle: an error signal should lead straight to its fix (affordance/UX); keep the badge concept cohesive (SRP).

• 🟡 The Labs comment says "the disabled switch keeps it off", which is not true for a persisted true.
  📁 packages/desktop-app/src/lib/components/settings/sections/labs-settings.svelte:40-41
  If syncEnabled=true is already in localStorage, the switch shows checked-but-disabled, resolveProSyncVariant() / settings-pane still gate on it, and no UI can turn it off. Under the pre-release no-backward-compat rule, not migrating that value is acceptable. It only affects the two devs' machines, where clearing localStorage fixes it. But the comment claims something the code does not do. Either reword it ("reflects the stored flag but cannot change it"), or, if "not ready" should really mean off, make gating ignore the flag while the toggle is disabled. The one-line comment fix is enough. Principle: comments must not contradict behaviour (accuracy over intent).

🟢 Nits

• 🟢 Nit: 📁 packages/desktop-app/src/lib/utils/open-settings.ts:5-6: The docstring still names "the database indicator" as a caller. That component is now deleted, so drop it from the list.
• 🟢 Nit: 📁 packages/desktop-app/src/lib/components/layout/navigation-sidebar.svelte:18: import type { NavigationItem } could join the existing $lib/stores/layout.svelte import block at lines 3-9.
• 🟢 Nit: The link style now differs between labs-settings.svelte (text-foreground + underline) and account-settings.svelte:144 (text-primary + underline). The new choice is the more defensible one by the app.css contrast notes. A small follow-up could converge settings links on one class so "consistent with other settings links" holds by construction.
• 🟢 Nit: 📁 packages/desktop-app/src/tests/components/settings/labs-settings.test.ts: fireEvent.keyDown(' ' / 'Enter') on a disabled button proves little in Happy-DOM, which does not synthesize activation from keys. The disabled attribute assertion and the manual check are what carry this criterion. That's fine, but the test is weaker than its name suggests.
• 🟢 Observation (no action): Critical boot initialization (databaseStore.load(): active-DB selection, window pin, Pro-sync activation) is still triggered from a nav component's onMount. This PR preserves that behaviour correctly, and improves it slightly, because the old switcher only mounted when the sidebar was expanded. Longer term it belongs in the app shell, not the sidebar. The onDaemonReconnect retry covers the gap today.

Standards / Security / Performance

• No {@html}, no console.*, no lint suppression, no issue numbers in code comments (ADR references only). ✅
• The mailto: href is static, so there is no injection surface. The badge label is a static string. ✅
• Performance: one extra $derived over databaseStore.activeDatabase (a linear find over a tiny list). Negligible. ✅
• No new dependencies (@lucide/svelte is already used). ✅

Recommendation: APPROVE

Every acceptance criterion is met, and the change is a clear improvement in code health: about 40 lines of duplicated nav-button markup collapse into one snippet, a dead component is deleted, and a token-level a11y fix lands. The two 🟡 items are quick follow-ups and can go in this PR or be deferred at the author's discretion.

🤖 Generated with Claude Code

[malibio]

Automated pragmatic-code-review: APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.

[malibio]

Review addressed (bf7efb7)

✅ Addressed (4)

• 🟡 Badged Settings click now opens Settings → Database — NavBadge.settingsCategory lets the badge own its call-to-action; test asserts openSettings('database') when badged and openSettings(undefined) otherwise.
• 🟡 Labs comment reworded: the disabled switch reflects the stored syncEnabled but this UI can't change it (no longer claims it "keeps it off").
• 🟢 open-settings.ts docstring no longer lists the deleted database indicator.
• 🟢 NavigationItem type import merged into the existing layout import.

⏭️ Skipped (3)

• Shared settings-link class — two call sites, and they differ deliberately (Labs uses foreground for small-text contrast); revisit if a third link appears.
• Removing the Happy-DOM Space/Enter assertions — harmless, and the disabled assertion plus the in-app check (click + Space) already cover the criterion.
• Moving app-wide startup loading to the app shell — pre-existing, separate refactor.

Re-review decision: NO RE-REVIEW NEEDED — small, low-risk changes (one optional field + one argument, comments, import), covered by tests; pre-push gate passed.

🤖 Generated with Claude Code