Conversation
Owner
Author
kunickiaj
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36591cca7f
ℹ️ 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".
This was referenced Apr 23, 2026
Owner
Author
|
Thanks — this is the same P0 deep-link regression Ramsay flagged. Already addressed in the stacked remediation PR #967: |
Owner
Author
Merge activity
|
…verview card, add header link (PR 1/4)
kunickiaj
force-pushed
the
04-23-ui_sync_carve_diagnostics_into_sync_diagnostics_sub-view_remove_overview_card_add_header_link_pr_1_4_
branch
from
36591cc to
e484150
Compare
kunickiaj
deleted the
04-23-ui_sync_carve_diagnostics_into_sync_diagnostics_sub-view_remove_overview_card_add_header_link_pr_1_4_
branch
kunickiaj
added a commit
that referenced
this pull request
Apr 23, 2026
…imitives (#964) ## Description PR 2 of the Sync tab redesign (plan: docs/plans/2026-04-23-sync-tab-redesign.md). Changes how each device is presented in the People & devices list: - **Compact row header.** Every device now renders as a 56px clickable row: PresencePip · name · direction arrow · trust badge · "Sync: {timestamp}" meta · chevron. Hover elevates, focus-visible draws a 2px accent outline. - **Click-to-expand drawer.** Clicking a row expands an in-place drawer that carries the existing device-management surface (Sync now / rename / remove / ownership assignment / sharing scope). Only one row's drawer is open at a time; clicking another row collapses the previous one. - **aria-expanded / aria-controls** on each row button, a `section` landmark for the drawer with a descriptive \`aria-label\`. - **Shared primitives** added: - \`PresencePip\` (atom) with 6-state matrix (online / offline / degraded / attention / syncing / unknown), pulse animations gated by \`prefers-reduced-motion\`. - \`TrustChip\` (molecule wrapping Chip) for trust-state tone mapping. Not wired into the row yet — deferred to a follow-up so the existing trust badge render path stays intact. - \`ActionShelf\` (molecule) for toolbar-style primary + secondary buttons. - **Primary-filled button variant** (\`settings-button.sync-btn-primary\`) for the "at most one filled-accent button per line of sight" rule. ## Type of Change - [x] UX redesign (plan-backed) - [x] New shared primitives (additive) ## Testing - \`pnpm run tsc && pnpm run lint && pnpm run test\` (1475 passing) - Manual (next step): Sync tab — click a device row; drawer expands inline. Click another row; previous collapses. All existing device actions (rename, sync, remove, scope, ownership) work unchanged. Stacked on #963. PR 3 adds the loading-states vocabulary (skeletons, empty state, pulse + pip wiring on Sync now). PR 4 is the handoff anatomy pages.
kunickiaj
added a commit
that referenced
this pull request
Apr 23, 2026
## Description PR 4 (final) of the Sync tab redesign (plan: docs/plans/2026-04-23-sync-tab-redesign.md). Ships four pattern anatomy pages + a README index at docs/design/: - \`device-row-anatomy.md\` — list-item density pattern (compact row + click-to-expand drawer). DOM structure, states, touch-target rules, accessibility wiring. - \`action-shelf-anatomy.md\` — toolbar layout and button-prominence rules. "At most one filled-primary per line of sight" convention for the devices toolbar and any future sibling toolbars. - \`attention-vocabulary-anatomy.md\` — the unified offline / degraded / attention / syncing visual language. Pip sizes, pulse timing, Gestalt pairing between attention rows and device rows. - \`disclosure-region-anatomy.md\` — inline-expand vs. popover vs. modal decision table, focus-management guidance, anti-patterns. These document the patterns introduced by PRs 1–3 so other tabs can adopt the same vocabulary without re-deriving decisions. ## Type of Change - [x] Documentation ## Testing Docs-only change; no \`tsc\`/\`lint\`/\`test\` impact. Verified that the four pages render correctly as markdown. Stacked on #965. Completes the 4-PR Sync tab redesign series: - #963 — diagnostics sub-view + Overview card removal - #964 — device-row + drawer + primitives - #965 — syncing-pip override + skeleton + empty state - #966 — handoff anatomy pages (this PR)
kunickiaj
added a commit
that referenced
this pull request
Apr 23, 2026
## Description Remediation PR on top of #963–#966. Addresses the findings from four brutal reviews across the stack. Batched here so reviewers see one coherent "Ramsay fix" diff instead of amending four in-flight PRs. ### Bugs fixed - **Deep-link regression (#963, P0).** Bookmarking \`#sync/diagnostics\` no longer lands on the main view. \`setActiveTab\` now only rewrites the hash when the top-level tab actually changed; a new \`parseTabFromHash\` helper centralizes the sub-view-aware split. - **Hashchange listener mismatch (#963, P0).** \`app.ts\` now uses \`parseTabFromHash\` too, so sub-view hash transitions (e.g. back button from \`#sync\` → \`#sync/diagnostics\`) fire \`switchTab\` correctly. - **Dead \`listMount\`/\`listHeading\` plumbing (#963, P1).** Removed from \`TeamSyncPanelProps\` and \`render-team-sync.ts\`. The old \`listHeading = list?.previousElementSibling\` was toggling the wrong sibling's \`hidden\` after the \`#syncTeamStatus\` element was removed — silent visibility corruption. - **Row \`<button>\` a11y grenade (#964, P0).** The row head is no longer a \`<button>\`. The chevron is now the only interactive element (\`.device-row-toggle\`), with a clean accessible name ("Expand device {name}"). Badges, direction glyphs, and the name with its \`title\` attribute no longer pollute the button's accessible-name computation. - **\`ActionShelf\` and \`TrustChip\` shipped unused (#964, P2).** Deleted both; along with their dead CSS (\`.action-shelf\`, \`.settings-button.sync-btn-primary\`, \`.peer-scope-chip\` tone variants). Reintroduce when a consumer actually needs them. - **\`syncing\` pip override is vaporware (#965, P1).** Reverted. The local \`syncBusy\` state only fires on the per-row Sync-now button; global Sync now never flipped any row, and the backend has no per-peer in-flight signal. The pulse CSS stays so the state is ready when a real signal lands. Filed as follow-up. ### Doc drift fixed (#966) - Removed phantom \`--pip-size-sm\` / \`--pip-size-md\` tokens (PresencePip sizes are just \`6 | 8\` literals). - Removed \`--accent-warm-strong\` implication that attention pips use a distinct hue — they don't, the CSS falls back to \`--accent-warm\`. Pulse alone distinguishes attention from offline. - \`DeviceRow\` props block relabeled as a proposed interface; the real component is still \`SyncPeerCard\`. - Deleted \`action-shelf-anatomy.md\` (no consumer primitive). - Updated "don't nest drawers" anti-pattern to acknowledge that inline sub-disclosures inside a drawer are fine (\`PeerScopeCollapsible\` inside the device drawer is the example). - Focus-management claim clarified as emergent, not designed. ### Code smell sweep (#964) - Deleted underscore-prefixed dead vars (\`_effectiveInclude\`, \`_effectiveExclude\`, \`_inheritsGlobal\`, \`_summaryText\`). - Moved the mid-file \`renderIntoSyncMount\` import to the top. ## Type of Change - [x] Bug fix - [x] Code cleanup - [x] Documentation correction ## Testing - \`pnpm run tsc && pnpm run lint && pnpm run test\` (1475 passing, 386 files linted — 2 fewer than before thanks to the primitive deletions). - Manual next step: bookmark \`#sync/diagnostics\`, reload, verify the sub-view loads; tab through the device row, verify only the chevron receives focus. Stacks on #966. Closes out the Ramsay review feedback across the redesign stack.
2 tasks
kunickiaj
added a commit
that referenced
this pull request
Apr 24, 2026
## 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 - [x] Bug fix - [x] 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
PR 1 of the Sync tab redesign (plan:
docs/plans/2026-04-23-sync-tab-redesign.md). Two cuts that together
deliver the biggest single density win the plan targets:
Sync tab now has two sibling regions inside `#tab-sync`:
`#syncMainView` and `#syncDiagnosticsView`, toggled by a new
`sync-view-controller.ts` that reads the URL hash. A "View
diagnostics →" link in the Sync tab header navigates there; a
"← Back to sync" link above the diagnostics card returns to the main
view. The tab router in `state.ts` was taught to strip the
`/diagnostics` segment before matching so `#sync/diagnostics` still
activates the sync tab.
statusSummary derivation in render-team-sync.ts are gone; the presence
signal lives in the header chip and the Needs attention section above.
PRs 2-4 continue the redesign: DeviceRow + DeviceDetailDrawer (2),
PresencePip state matrix + skeletons + empty states (3), handoff
anatomy pages (4).
Type of Change
Testing
files linted)
it swaps in the diagnostics sub-view; back link returns to main. Team
status Overview card no longer renders.