Skip to content

ui(sync): remediation pass on Sync tab redesign stack#968

Merged
kunickiaj merged 5 commits into
mainfrom
04-23-ui_sync_remediation_pass_on_sync_tab_redesign_stack
Apr 24, 2026
Merged

ui(sync): remediation pass on Sync tab redesign stack#968
kunickiaj merged 5 commits into
mainfrom
04-23-ui_sync_remediation_pass_on_sync_tab_redesign_stack

Conversation

@kunickiaj
Copy link
Copy Markdown
Owner

Description

Dogfood of the merged Sync tab redesign (PRs #963-#967) surfaced five real
issues. Fixing them together since they're all the same "Sync tab polish"
story.

  • Device list was a 2-column grid above 900px, so expansion rendered
    inside a narrow sidebar column while the other row sat empty. Switched
    to single-column full-width rows; outer border/radius/background now
    live on .peer-card, the row head is transparent content, and the
    drawer has a border-top separator inside the same card.
  • Row head was a grid that broke when the direction glyph was absent,
    causing the chevron to wrap to a second line. Switched to flex;
    .device-row-toggle { margin-left: auto } keeps the toggle right-edge
    regardless of which middle children render.
  • Row expand toggle was a hand-rolled <button> with a unicode ▸/▾
    that was nearly invisible against --surface-1. Replaced with Radix
    Collapsible (@radix-ui/react-collapsible, already installed) + a
    properly sized inline Lucide chevron SVG that rotates on data-state="open".
    Radix gives us correct aria-expanded/aria-controls wiring and
    keyboard handling for free. Module-level expandedPeerId still
    enforces single-open-at-a-time; handleOpenChange reads the live
    module state (not a stale closure) so one row taking over doesn't
    clobber the new pointer.
  • View diagnostics → and ← Back to sync had different styles.
    Collapsed into one .sync-subview-link class used for both
    directions — utility-nav treatment, quieter than any nearby button.
  • work is offline surfaced as a Needs Attention item. Computers
    power-cycle; the row already carries the Offline badge + pip. Dropped
    the peerStatus === "offline" branch from sync-view-model.ts's
    attention-item derivation. Auth/trust/repair failures still fire.
  • Pairing payload was double-hidden ("Show pairing command" →
    revealed a card that just said "Pairing payload hidden" unless
    diagnostics were on). The payload is a local command shared with your
    own other device, not a secret. The outer disclosure is enough
    gating; removed the redaction branch in diagnostics/helpers.ts.
  • Sync tab cards had no gap between them while Feed did. The
    redesign's #syncMainView / #syncDiagnosticsView wrappers broke
    the .tab-panel flex-gap inheritance; restored the same gap inside
    each sub-view wrapper.

Type of Change

  • Bug fix
  • UX polish

Testing

  • `pnpm run tsc && pnpm run lint && pnpm run test` (1475 passing)
  • Manual: Sync tab — device row toggle is visible mint-on-surface
    chevron that rotates 90° on open; expand/collapse one row cleanly
    stacks in a single column; View diagnostics / Back to sync read as
    the same utility link; offline peer no longer shows up in Needs
    attention; "Show pairing command" reveals the command directly.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3939b10a8b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/ui/src/tabs/sync/diagnostics/helpers.ts
The sync diagnostics sub-view sent includeDiagnostics=1, which flipped
getSemanticIndexDiagnostics into a per-memory chunk-coverage check that
runs a vec0 probe for every active memory on the Node main thread. On
a dogfood DB this stalled the event loop for seconds and caused every
concurrent HTTP request to queue behind it — the viewer answered once
then went silent.

Force the sync status endpoint onto the existing fast COUNT(DISTINCT)
join, regardless of diagnostics mode. The deep per-chunk verifier is
still reachable via an explicit { fastCounts: false } opt-in for
non-request-path tooling; nothing in prod sets it today.
… groups panel

Before: group cards rendered inside an unstyled .coordinator-admin-request-list
with no gap, using the paddingless .peer-card (which Sync device rows use) so
all card contents bled to the card edge. The scope-defaults drawer was a raw
conditional sibling render with a button-label swap, no aria-expanded, and
the drawer's top border bled to the card edge.

- Wrap the visible group cards in .peer-list (already a 1fr grid with gap).
- Add .peer-card--padded variant so generic content cards get interior
  padding without regressing the Sync tab's device-row (paddingless by
  design — row-head owns its own chrome).
- Replace the Scope defaults label swap with a Radix Collapsible: the
  action button is the Trigger (with data-state / aria-expanded /
  aria-controls + a chevron that rotates on open), and Collapsible.Content
  hosts the drawer using the existing .coordinator-admin-group-preferences
  chrome. Inline SVG chevron mirrors the one in sync-peers.tsx.
- Drop the .coordinator-admin-group-preferences wrapper div from the
  editor body so the class lives on exactly one node (the Collapsible
  host), and remove its now-redundant margin-top.
kunickiaj added a commit that referenced this pull request Apr 24, 2026
With sync redaction enabled (the default), /api/sync/pairing returns
{ redacted: true, pairing_filter_hint: ... } with no device_id,
fingerprint, public_key, or addresses. Previously the UI happily turned
that into a base64-encoded 'codemem sync pair --accept-file -' command
that deterministically fails in the CLI accept path.

Detect redacted: true explicitly and show the state to the user instead
of a broken copyable command. The outer 'Show pairing command'
disclosure still gates user exposure; this only kicks in when the
SERVER redacted the response, which can't be avoided from the UI
without flipping includeDiagnostics=1.

Addresses Codex review feedback on #968.
Copy link
Copy Markdown
Owner Author

kunickiaj commented Apr 24, 2026

Merge activity

  • Apr 24, 4:05 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Apr 24, 4:05 PM UTC: @kunickiaj merged this pull request with Graphite.

@kunickiaj kunickiaj merged commit 62bec3c into main Apr 24, 2026
8 checks passed
@kunickiaj kunickiaj deleted the 04-23-ui_sync_remediation_pass_on_sync_tab_redesign_stack branch April 24, 2026 16:05
kunickiaj added a commit that referenced this pull request Apr 24, 2026
With sync redaction enabled (the default), /api/sync/pairing returns
{ redacted: true, pairing_filter_hint: ... } with no device_id,
fingerprint, public_key, or addresses. Previously the UI happily turned
that into a base64-encoded 'codemem sync pair --accept-file -' command
that deterministically fails in the CLI accept path.

Detect redacted: true explicitly and show the state to the user instead
of a broken copyable command. The outer 'Show pairing command'
disclosure still gates user exposure; this only kicks in when the
SERVER redacted the response, which can't be avoided from the UI
without flipping includeDiagnostics=1.

Addresses Codex review feedback on #968.
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