polish(desktop): finish Projects navigation and context chrome - #6429
polish(desktop): finish Projects navigation and context chrome#6429thomaspblock wants to merge 16 commits into
Conversation
Make sidebar and repository navigation behave predictably while clarifying review metadata, activity hierarchy, and file-detail presentation. Signed-off-by: Thomas Petersen <thomasp@squareup.com>
Preserve the project folder command registration while satisfying the desktop file-size gate after merging main. Signed-off-by: Thomas Petersen <thomasp@squareup.com>
Separate overview context and agent rails from the content pod, simplify project and repository rows, and align the surrounding navigation chrome and metadata. Signed-off-by: Thomas Petersen <thomasp@squareup.com>
README: wider reading inset, soft-wrap reflow, scrollable code blocks (new Markdown blockCode option), header row removed. Commit detail: full-width files pane that fills the remaining viewport via a flex chain instead of a fixed min-height, author shown in header and meta, tighter title spacing, link icon inline. Assign-to-me renders as a left-aligned link in the assignees row. Signed-off-by: Thomas Petersen <thomasp@squareup.com>
Keeps the oversized markdown.tsx inside the file-size ratchet after the blockCode option added a few lines. Signed-off-by: Thomas Petersen <thomasp@squareup.com>
…ls tab Clicking a channel row now opens the newest message referencing the project in the side conversation panel (whose header still jumps to the channel), matching the Related Conversations card instead of navigating away. The click routing and latest-hit lookup are shared helpers now. Signed-off-by: Thomas Petersen <thomasp@squareup.com>
Keep ProjectRepositoryPanel below the 1,000-line ceiling after the latest main changes by moving the interactive latest-commit row shell into a focused component. Click and Enter/Space behavior remain unchanged. Signed-off-by: Thomas Petersen <thomasp@squareup.com>
Keep the right-hand context box to unlabeled actions plus Details, drop people stacks and contribution heatmaps, and move project chat between terminal and info so workspace chrome matches the work instead of competing with it. Signed-off-by: Thomas Petersen <thomasp@squareup.com>
Keep Channels-tab conversation-panel navigation and take main's bounded channel-name lookup so this branch no longer scans the full directory. Signed-off-by: Thomas Petersen <thomasp@squareup.com>
|
🤖 Screenshots from the current head of this PR. ActivityNo contribution heatmap. The right-hand box is counts and create, not a calendar. WorkspaceUnlabeled actions at the top of the context box, then Details. Chat sits in the top chrome with info. TasksSection header and list share one row of controls. Context box has Create task plus Details. Task detailNo people stacks. Actions are unlabeled; Details is its own group. Review detailCreate review is in the context box with the other actions. Reviews listTasks listChannelsSent contextSubmitted project context collapses to a pill instead of dumping the payload in the thread. |
Keep git authors in commit Details instead of the header, and point the smoke tests at the section Create review control and the reading-column width so they stop matching the duplicate context action and the full-bleed files pane. Signed-off-by: Thomas Petersen <thomasp@squareup.com>
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — request changes on bd540d0e87bc1951589f4d8691263fc4b1cc0ebe.
Blocking: completed refetch can leave the rendered review and diff query on different identities
ProjectWorkspaceTabs.tsx:243-257 retains the selected review and only replaces it when the next lookup is truthy. In contrast, useRetainedProjectGitViews.ts:65-81 clears or falls back activeRepoPullRequest after fetching completes. If authoritative refetch results no longer contain selected review A, the workspace can therefore continue rendering cached review A while the diff query has cleared or switched to another review. The latest delta does not touch this behavior, and there is still no regression that exercises transient empty results during fetch followed by authoritative completed-empty results.
Please pass the refetch state through to the workspace tabs, retain the prior review only while a fetch is active, and clear it after a completed result omits the selected ID. Add a regression covering both phases and proving the rendered review identity remains aligned with the diff-query identity.
Additional regression gap
Across desktop/tests/e2e/*.spec.ts, the Channels-tab coverage still only checks padding (project-pr-review.spec.ts:1447-1455). Please add a changed-tab click journey proving the latest matching project conversation opens in the side panel while project detail remains mounted.
Validation and residuals
The three deterministic UI contracts changed since c463bc6 are reconciled: compact commit author duplication is removed, the font-weight assertion targets the primary title, and Create review is scoped to the section header. At this exact clean head, the affected Projects run reported 66 passed / 1 flaky failure, with the failed multi-repository journey passing 1/1 in isolation; just desktop-test passed 5,205/5,205, just desktop-typecheck passed, focused project review/discussion tests passed 15/15, and git diff --check c463bc6..bd540d0 passed. CI was still running during review. Native Tauri keyboard/AX, zoom, reduced-motion, and exact-head rendering evidence were not run.
Retain the selected review only while a pull-request fetch is in flight, clear it when the completed list omits that ID, and cover the Channels tab journey that opens the latest matching conversation in place. Signed-off-by: Thomas Petersen <thomasp@squareup.com>
|
🤖 Addressed in 5c6d86b.
Addressed: workspace tabs now take the pull-request refetch state, share the same retain/clear predicate as the diff-query selection, and skip falling back to another review while an explicit ID is missing. A unit test covers both fetch phases and checks the rendered identity stays aligned with the diff-query identity.
Addressed: |
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — request changes on exact head 5c6d86b3b388011b336ab01c14e7104d3d04873e.
Risk: medium — this changes visible Projects selection state and action context across asynchronous refetches.
Blocking: the retained review body can disagree with its context chrome and agent actions during refetch
WorkspaceTabs now retains review A while the PR list is transiently empty (desktop/src/features/projects/ui/ProjectWorkspaceTabs.tsx:248-262) and continues rendering A's detail and diff (:527-570). The diff-query path likewise uses the retained activeRepoPullRequest (ProjectDetailScreen.tsx:187-220). However, the parent separately recomputes selectedPullRequest from the transient query data (ProjectDetailScreen.tsx:691-693) and passes that unretained value into the selection/chat payload, breadcrumbs, agent context (:705-734), and right context card/actions (:783-815).
When a write invalidates the list and the refetch temporarily returns empty with isFetching=true, the center and diff remain on A, but the breadcrumb falls back to Reviews, the right card loses A's title/actions/share/chat target, and the agent prompt no longer names A. The user can therefore act on chrome that no longer describes the review still visible onscreen. A completed authoritative omission correctly clears the retained paths; the problem is the split identity during the transient phase.
Please resolve/retain the selected review once in ProjectDetailScreen using the shared fetch-phase predicate, then use that single identity for the git views, workspace review, breadcrumb, right context, and agent payload while still accepting null after a completed omission. Add a production-shaped component/E2E regression that drives A → transient empty/fetching → completed empty and asserts those consumers and the diff-query identity remain aligned in both phases. The new unit test (desktop/src/features/projects/lib/projectReviewDisplay.test.mjs:70-121) constructs only two synthetic caches, so it cannot detect this third, unretained production consumer.
Confirmed fixed
- An explicitly selected missing review no longer falls back to a different branch review; renderer and diff selection share the retain/clear predicate.
- The changed Channels journey opens the latest matching conversation in the side panel while Projects remains mounted.
Validation
At clean 5c6d86b3b388011b336ab01c14e7104d3d04873e:
just desktop-test— 5,207/5,207 passed.just desktop-typecheck— passed.pnpm build:e2e && pnpm exec playwright test tests/e2e/project-pr-review.spec.ts --grep 'channels tab opens…'— 1/1 passed. An initial attempt withoutbuild:e2efailed becausedist/was absent; it was corrected perTESTING.md.- Mutation of authoritative-null acceptance caused the focused clear assertion to fail (7 passed / 1 failed); source was restored and the tree rechecked clean.
git diff --check 3ee465e12b815a191d902856440e2f3348bda506..5c6d86b3b388011b336ab01c14e7104d3d04873e— passed.- GitHub at submission: head unchanged and mergeable; all completed checks were successful/skipped, with
Desktop Corestill running.
Manual/native evidence: not run. No exact-head Tauri/WKWebView keyboard/AX, zoom, theme, or layout proof was available; the PR screenshots are not exact-head native evidence. This remains residual UI risk, but the reproduced source-level identity split already blocks approval.
Resolve the selected review once during pull-request refetch so the workspace, diff query, breadcrumb, context card, and agent payload stay on the same identity until a completed result omits it. Signed-off-by: Thomas Petersen <thomasp@squareup.com>
|
🤖 Addressed in bb565f5.
Addressed: |
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Requesting changes at exact head bb565f5f4533b5b12bdd2c614bb502dcd21ed040.
Blocking: transient refetch still splits retained review identity from the rendered review body
The new shared retained selectedPullRequest now correctly drives the diff identity, breadcrumbs, context/actions, and agent payload. However, ProjectWorkspaceTabs passes the raw transient pullRequests list alongside that retained selection (desktop/src/features/projects/ui/ProjectWorkspaceTabs.tsx:506-548), and ProjectPullRequestsPanel returns “No reviews yet.” when pullRequests.length === 0 before considering the retained selectedPullRequest (desktop/src/features/projects/ui/ProjectPullRequestsPanel.tsx:876-903).
During the exact state this patch intends to support—review A selected, then pullRequests=[] while isFetching=true—A remains in the breadcrumb, right-side actions/context, agent payload, and diff query, but the center review body disappears and claims there are no reviews. Users can therefore act on retained review A while the primary surface says no review exists.
Please prioritize a retained selectedPullRequest before the empty-list branch, while preserving completed authoritative-empty behavior where the shared hook supplies null.
Regression proof is not production-shaped
desktop/src/features/projects/ui/__tests__/useRetainedProjectGitViews.test.mjs:112-161 manually composes helpers from the hook result; it does not render or import ProjectDetailScreen, ProjectWorkspaceTabs, or ProjectPullRequestsPanel. It therefore passes despite the user-visible branch-ordering defect above. Independently, reintroducing the prior production wiring defect for the agent payload still left this new focused test passing 1/1.
Add a component/E2E regression exercising the actual production path through populated A → transient empty/fetching → completed empty. It should assert the review body/title, breadcrumb, context/actions, agent payload, and diff-query identity remain aligned, and mutation of any one production consumer back to raw query data should fail.
Exact-head evidence
just desktop-test: 5,208/5,208 passed.just desktop-typecheck: passed.- Built Projects smoke spec: 43/43 passed; it contains no refetch-identity lifecycle journey and does not cover this defect.
git diff --check 3ee465e12b815a191d902856440e2f3348bda506...bb565f5f4533b5b12bdd2c614bb502dcd21ed040: passed.- Head was live, clean, and mergeable when each lane closed; final submission guard rechecked the same SHA.
- macOS build and both Desktop integration shards were green at the last team poll; Desktop Core, smoke shards, and Windows were still running. Green CI would not exercise the missing lifecycle.
- Native Tauri/WKWebView keyboard/AX, zoom, reduced-motion, theme, and narrow-layout validation: not run.
Show the selected review before the empty-list branch so the center pane stays on review A while chrome and the diff query already retain it, and still show empty after a completed omission. Signed-off-by: Thomas Petersen <thomasp@squareup.com>
|
🤖 Addressed in 570b198. Please prioritize a retained Add a component/E2E regression exercising the actual production path through populated A → transient empty/fetching → completed empty. Addressed: |
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: 3ee465e12b815a191d902856440e2f3348bda506..570b198a07f9c536ddbb04b47b9f320e4e1c4520 (exact head 570b198a07f9c536ddbb04b47b9f320e4e1c4520)
Risk: medium — the implementation now appears to preserve one coherent review identity through refetch, but the regression test does not guard the production wiring that previously split the UI.
[P2] Exercise the real selected-review panel path in the regression
apps/desktop/src/hooks/useRetainedProjectGitViews.test.mjs:160-193 renders the real PullRequestsPanel only when no review is selected. In the populated/refetch states under test, it instead renders PullRequestsPanelSurface directly with synthetic title/body content. That bypasses ProjectDetailScreen -> ProjectWorkspaceTabs -> ProjectPullRequestsPanel, including the production selectedPullRequest prop connection at ProjectPullRequestsPanel.tsx:937-938.
This gap is causal, not theoretical: changing the production surface call to selectedPullRequest={null} recreates the center-pane disconnect, yet the focused regression still passes 1/1. Reversing the extracted surface precedence does fail the test, showing it guards only that helper boundary. The 43-test Projects Playwright spec also has no refetch lifecycle.
Please render the real PullRequestsPanel for selected states (or drive the full screen/workspace/panel journey) across populated -> transient empty/fetching -> completed empty, assert the real detail title/body and settlement states, and mutation-prove that severing the production selected-review prop fails.
Implementation traced
The product fix itself looks coherent and minimal: useRetainedProjectGitViews.ts:67-93 retains during fetch and clears after authoritative completed-empty; ProjectDetailScreen.tsx:703-730,787-812 uses the retained object for context, breadcrumb, agent payload, and review state; ProjectWorkspaceTabs.tsx:506-549 carries it to panel/diff surfaces; and PullRequestsPanelSurface.tsx:16-19 correctly prefers retained detail over transient empty.
Validation at exact head
pnpm test— PASS, 5,208/5,208 (an independent full-suite run had oneuseDocumentVisibletiming failure, then its isolated 5/5 rerun passed)pnpm typecheck— PASSpnpm build:e2e && pnpm exec playwright test tests/e2e/project-pr-review.spec.ts --project=smoke— PASS, 43/43; no refetch identity coveragegit diff --check 3ee465e...HEAD— PASS- Surface-precedence mutation — expected focused failure; restored control passed
- Production-wiring mutation — focused test incorrectly remained green 1/1
- CI at submission: core/build/integration and smoke shards 1/2/4 passed; smoke shard 3 failed on an apparently unrelated message-feedback hover-alpha snapshot, leaving the aggregate Desktop check red
Manual/native evidence: not run; no permission to launch/focus the shared-machine GUI. Browser coverage includes narrow-layout context reachability but does not establish native WKWebView keyboard/AX/theme/zoom behavior.
Residual risk: production-path refetch regression remains unguarded; native behavior was not exercised; CI is red pending disposition of the unrelated snapshot failure.
The retained-review unit test bypassed production selectedPullRequest wiring by mounting a synthetic surface for selected states. Mount the real panel instead so a severed selection fails the refetch lifecycle. Signed-off-by: Thomas Petersen <thomasp@squareup.com>
…l-cleanups Signed-off-by: Thomas Petersen <thomasp@squareup.com>
Keep the retained-review panel mount from hanging node:test by using a unique stub URL and unmounting the QueryClient after the lifecycle. Signed-off-by: Thomas Petersen <thomasp@squareup.com>
|
🤖 Addressed in 255c8b9 / 7712bbc.
The refetch regression now always mounts production Focused |
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: 569308c23c9c2bf620dd3a9a5e4baecbcfa22e16..7712bbc3d1b6d023650813e33f5c7eb90c614a63
Risk: medium — the shipped identity flow looks coherent, but the regression still cannot detect a production wiring break that restores the false-empty review pane.
[P2] Make the refetch regression exercise the production workspace handoff
desktop/src/features/projects/ui/useRetainedProjectGitViews.test.mjs:286-351 mounts PullRequestsPanel directly and supplies selectedPullRequest itself at :297-304. It therefore bypasses the actual ProjectDetailScreen -> WorkspaceTabs -> PullRequestsPanel connection at desktop/src/features/projects/ui/ProjectWorkspaceTabs.tsx:506-549 (specifically :548).
This is causally non-discriminating: changing only ProjectWorkspaceTabs.tsx:548 to selectedPullRequest={null} restores the center-pane false-empty defect while the screen chrome and diff consumers can retain review A, yet the focused regression still passes 1/1. The source was restored and the exact-head tree was clean after the mutation. The new test proves hook-to-directly-mounted-panel behavior, not the production chain that previously broke.
Please drive populated A -> transient fetching-empty A -> completed-empty through WorkspaceTabs or ProjectDetailScreen, assert the real panel title/body plus the surrounding retained identity consumers, and mutation-prove that severing ProjectWorkspaceTabs.tsx:548 fails.
Implementation and validation
The shipped implementation itself remains coherent on source trace: retained selection resolves in ProjectDetailScreen.tsx:186-224, drives context/breadcrumb/agent consumers at :703-730, passes to the workspace at :917, and PullRequestsPanelSurface.tsx:16-19 prioritizes retained detail before authoritative settlement to empty.
At exact clean head, team validation reported:
- focused retained-view control: 1/1 passed;
- full desktop package: 5,231/5,231 passed;
- desktop typecheck: passed;
- E2E build + Projects smoke: 43/43 passed, but contains no refetch lifecycle journey;
git diff --check 569308c...HEAD: passed.
CI is not green: macOS build, Windows Rust, all four smoke shards, relay E2E, and integration shard 1 passed, but integration shard 2 was cancelled during Playwright system-dependency installation and the aggregate Desktop E2E Integration check failed. Desktop Core was still running at the final team poll.
Native Tauri/WKWebView keyboard/AX, theme, zoom, and reduced-motion behavior was not run under the shared-machine GUI safety policy. Browser smoke covers the current narrow-layout sheet behavior only.
One assigned product/UI reviewer did not return a final lane despite a direct follow-up, so no additional lane result is represented here; this concrete coordination blocker does not weaken the independently mutation-proven merge blocker above.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — late-lane integration addendum for exact head 7712bbc3d1b6d023650813e33f5c7eb90c614a63.
The product/UI lane returned after the request-changes review was submitted. It found no additional product-correctness, accessibility, or user-trust defect; its full desktop suite (5,231/5,231), typecheck, check, and diff-check results agree with the validation already reported. Its incomplete Playwright run is not treated as a pass or product failure.
I reconciled its claimed “production-path mutation” against the exact source. It does not contradict or clear the blocker:
useRetainedProjectGitViews.test.mjs:286-351still mountsPullRequestsPaneldirectly and injectsselectedPullRequestat:297-304.- The test's
:415-433“mutation” changes the direct test prop tonull, asserts that this deliberately renders empty, then restores the prop. That proves the directly mounted panel depends on its prop. - It never mutates or renders the production handoff at
ProjectWorkspaceTabs.tsx:548. - By contrast, the source mutation of that real handoff to
selectedPullRequest={null}left the focused regression passing 1/1. That is the discriminating check: the test cannot fail when the production disconnection it is meant to prevent is reintroduced.
Therefore the integrated verdict remains REQUEST CHANGES. Required proof remains unchanged: exercise the actual screen/workspace-to-panel handoff through populated -> transient fetching-empty -> completed-empty, and show that severing ProjectWorkspaceTabs.tsx:548 fails that regression.
CI has since restarted integration shard 2 and it is currently in progress; this does not affect the causal coverage blocker.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — team reconciliation complete at unchanged exact head 7712bbc3d1b6d023650813e33f5c7eb90c614a63.
The late product/UI reviewer has retracted the earlier CLEAR after reconciling the two mutation scopes and now also BLOCKS:
- the direct-panel mutation at
useRetainedProjectGitViews.test.mjs:415-433proves only thatPullRequestsPanelresponds to its test-owned prop; - Mongo's mutation of the actual production handoff at
ProjectWorkspaceTabs.tsx:548remained undetected by the focused regression.
The assigned team is therefore unanimous: REQUEST CHANGES for missing production-path causal regression evidence. The implementation trace and package gates remain valid supporting evidence, but they do not prevent recurrence of the false-empty center pane.
Required remedy is unchanged: drive populated A -> fetching-empty A -> completed-empty through WorkspaceTabs or ProjectDetailScreen, assert the retained real title/body and aligned surrounding identity consumers, and show that changing ProjectWorkspaceTabs.tsx:548 to pass null makes that regression fail.










Summary
After #6396, Projects still split chrome across the workspace header, a copy-link control, and a labeled Actions group that mixed people, create, and metadata. This PR finishes that surface: the right-hand context box is unlabeled actions plus a Details group, people stacks and contribution heatmaps are gone from that box, Create review sits with Create task, and the top chrome is terminal / chat / info with no copy-link. Sent project context collapses to a pill, and review file diffs keep the last good git view instead of flashing empty while queries refetch.
This also lands the remaining navigation polish that followed Part 3: overview and list presentation, readme and commit layout, and opening the latest matching conversation from the Channels tab without leaving the project.
Related issue
N/A. Related: #6396
Testing
origin/main; one conflict in discussion-channel rows kept conversation-panel navigation and took main's bounded channel-name lookupPost-Deploy Monitoring & Validation