Repository navigation
Simplify the desktop pull request reader - #1158
Conversation
A requested reviewer with no review carries no state from GitHub, so it now reads as awaiting rather than unknown. Avatars load through MarkdownImages, the app's one credential-free https image fetch; a failed load leaves the initial in place.
A style animation starts when its control appears, so each copy of a pending marker faded on its own offset. PulseClock drives every marker from one phase instead.
…1157) Fluent's CheckBox has a 32px minimum height, taller than the sub-tab pills, so opening Threads pushed the content down. The row now has a fixed height.
The daemon reports the branch a worktree was cut on, so a PR opened after a switch was never discovered; HEAD is read instead. Check rows reload while any is pending, so the summary cannot move on beside a stale snapshot.
A tab paged past its first page is left alone, since a reload restarts at page one. Progress shows only for a refresh someone clicked; the poll reads silently.
The poll reloads only the open tab, and the checks summary distrusts rows older than 30s, so rows kept past that beside a moved-on summary read as out of sync.
A page read that reports the new head first recovers through one fresh overview; an overview still on the old head leaves the retry to the next poll rather than looping.
de54170 to
fee2033
Compare
|
@codex review |
![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: fee203335c
ℹ️ 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".
| string? _reportedBranch; | ||
| // The daemon reports the branch the worktree was cut on; HEAD knows a switch made since. | ||
| void ResolveBranch() { | ||
| var branch = GitRepository.CurrentBranch(WorktreePath) ?? _reportedBranch; |
There was a problem hiding this comment.
For a borrowed reviewer whose snapshot path differs from BorrowedFrom, WorktreePath has already been rewritten by CheckoutLabel.CheckoutPathFor to the checkout being reviewed. The status DTO documents Branch as belonging to the checkout the agent runs in, so this reads HEAD from the wrong repository and can display a branch unrelated to the session whenever those two checkouts differ. Retain the raw dto.WorktreePath for this lookup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The branch fact is HEAD of the checkout the pane already names. CheckoutLabel.CheckoutPathFor prefers BorrowedFrom, and that is the path ResolveBranch reads. A borrowed snapshot's daemon Branch is empty, so that HEAD is the label beside the borrowed checkout. The PR list keeps the raw snapshot path because it has to follow the checkout the agent runs in; using it here would show the snapshot's branch next to the borrowed checkout.
| } else if (read.Kind == PullRequestReadKind.Restart && read.Reason == "head_changed" && !_headRestarted) { | ||
| // Learn the new head from a fresh overview, whose apply reloads the checks. Once only: | ||
| // an overview still on the old head would bounce straight back here. | ||
| _headRestarted = true; | ||
| _sections.Remove(key); |
There was a problem hiding this comment.
After one check request returns head_changed, this global flag remains true until a successful checks page clears it. If the user switches PRs or sessions while the recovery overview is pending and the newly selected PR also returns head_changed, this branch is skipped and the generic restart path marks its checks section stopped instead of fetching a fresh overview. Reset this state when changing the selected subject/session, or scope it to the subject.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
_headRestarted is cleared in CancelReads. Choosing another pull request and switching session both cancel the in-flight recovery, so the next head_changed can load a fresh overview. A recovery that was not cancelled still refuses a second bounce from an overview on the old head. Covered by Switching_pull_request_during_a_head_change_lets_the_next_one_recover.
The once-only guard stops an overview on the old head from bouncing. It has to die with the cancelled read, or the next pull request's checks stop instead of reloading.
PR Summary by QodoSimplify and synchronize the desktop pull request reader
AI Description
Diagram
High-Level Assessment
Files changed (20)
|
ChangelogPull request readerChanged
Fixed
About this work pane
|
Code Review by Qodo
1. Snapshot reviewers show wrong branch
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| var branch = GitRepository.CurrentBranch(WorktreePath) ?? _reportedBranch; | ||
| Branch = string.IsNullOrWhiteSpace(branch) ? "—" : branch; |
There was a problem hiding this comment.
2. Snapshot reviewers show wrong branch 🐞 Bug ≡ Correctness
ResolveBranch calls GitRepository.CurrentBranch(WorktreePath), but UpdateFacts assigns WorktreePath from CheckoutLabel.CheckoutPathFor(dto), which prefers BorrowedFrom. For snapshot reviewers, BorrowedFrom is the source checkout while the agent runs in its separate dto.WorktreePath, so the Session pane reads and displays the source branch after a switch instead of the running agent’s branch.
Agent Prompt
Issue description
`ResolveBranch` reads `HEAD` through the UI-facing `WorktreePath`, which intentionally prefers `BorrowedFrom` for borrowed snapshot reviewers. That path can differ from the checkout in which the agent runs, producing an incorrect branch display.
Fix Focus Areas
- src/Capacitor.App/ViewModels/WorkContextViewModel.cs[179-184]
- src/Capacitor.App/ViewModels/WorkContextViewModel.cs[301-305]
Recommended Fix
Keep the actual agent worktree path separately from the presentation/grouping checkout path, and pass the actual `dto.WorktreePath` to `GitRepository.CurrentBranch`. Continue using `CheckoutLabel.CheckoutPathFor(dto)` only for the displayed worktree label and grouping.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
The branch fact stays on the checkout the pane names. CheckoutLabel.CheckoutPathFor prefers BorrowedFrom, and a borrowed snapshot is detached, so reading dto.WorktreePath would show no branch. Discovery now uses that same presented checkout.
| visual.DetachedFromVisualTree += OnDetached; | ||
| if (TopLevel.GetTopLevel(visual) is not null) Start(visual); |
There was a problem hiding this comment.
6. Hidden markers keep animation running 🐞 Bug ➹ Performance
PulseClock.Start retains every attached active visual and starts its timer without checking effective visibility, while Stop runs only when the visual detaches or IsActive becomes false. A pending marker under a hidden reader or other hidden ancestor remains attached and active, so the dispatcher continues updating it every 50 ms and cannot stop the shared timer even when no marker is on screen.
Agent Prompt
Issue description
`PulseClock` treats visual-tree attachment as visibility and retains hidden active markers in its global set. Hidden controls remain attached when an ancestor's `IsVisible` changes, so they keep the 20 Hz timer alive and receive opacity updates.
Fix Focus Areas
- src/Capacitor.App/Views/PulseClock.cs[21-31]
- src/Capacitor.App/Views/PulseClock.cs[48-65]
Recommended Fix
Track effective-visibility changes in addition to attachment and `IsActive`, adding a visual to `Active` only while it is effectively visible and removing it when it becomes hidden. Start or stop the shared timer after each membership change, and preserve opacity clearing when a marker stops pulsing.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
A hidden reader stays attached, and a pending marker on it remains in the clock. The work-context card shows the same marker and is the one on screen, which is why the timer runs. When checks are no longer pending, IsActive goes false and the clock stops. Effective visibility would also drop the card's marker whenever that card is covered.
| null when requested => new("Awaiting review", Detail: "Review requested; this reviewer has not submitted one yet."), | ||
| _ => new("Unknown", Detail: "GitHub did not report this reviewer's state."), |
There was a problem hiding this comment.
3. New reviewers appear re-requested 🔗 Cross-repo conflict ≡ Correctness
ReviewRowStatus treats only a null review state as awaiting, while ReRequested treats every non-null state as evidence of an earlier review. The server sends review_state: "unknown" with no submission time for a newly requested reviewer, so these reviewers display as unknown and re-requested instead of awaiting review.
Agent Prompt
## Issue description
The server represents a newly requested reviewer with `Requested = true`, `ReviewState = "unknown"`, and no `SubmittedAt`, but the desktop reader interprets that combination as an unknown, re-requested review.
## Fix Focus Areas
- src/Capacitor.App/ViewModels/PullRequestContextViewModel.Projections.cs[135-154]
- test/Capacitor.App.Tests.Unit/PullRequestPresentationTests.cs[96-127]
## Recommended Fix
Use `SubmittedAt` to distinguish initial requests from re-requests. When `Requested` is true and `SubmittedAt` is null, show `Awaiting review` and keep `ReRequested` false; only mark a request as re-requested when a prior submitted review exists, and update the fake-data test to use the server’s `"unknown"` state.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
A newly requested reviewer from the local reader has ReviewState null. That already reads as Awaiting review, and re-requested stays off. ReviewState is set only from an actual GitHub review state. This client does not send "unknown" for that row; A_requested_reviewer_without_a_review_reads_as_awaiting pins the null case.
A borrowed snapshot is detached, so discovery reads the reviewed checkout. Head recovery is per section, because threads are what report a moved head.
#1157 — AI-3216 #1157 is already closed by #1158, so this line has no closing keyword. ## What & why Continuation of #1158, which merged before the reader spacing was finished. Lists sit on the section edge. Only Overview, which has no row padding, keeps a small top inset. The Reviewers, Submitted, and Threads row keeps a little space under the pills, and the loading notice sizes to its text. ## Where to look The review row's bottom margin is the gap under the pills. The scroll area has no top inset except the overview block. ## Visuals <img width="1819" height="1110" alt="Screenshot 2026-09-25 at 18 26 42" src="https://github.com/user-attachments/assets/048e51cc-4c1f-4c41-9205-ae4cac8d7322" /> <img width="1819" height="1110" alt="Screenshot 2026-09-25 at 18 26 40" src="https://github.com/user-attachments/assets/bca18ff4-0b04-4c16-a8f9-4b2d7e0ee1bb" /> <img width="1819" height="1110" alt="Screenshot 2026-09-25 at 18 26 38" src="https://github.com/user-attachments/assets/c53cbae9-34af-40fe-858a-bdaf64cec93d" /> <img width="1819" height="1110" alt="Screenshot 2026-09-25 at 18 26 36" src="https://github.com/user-attachments/assets/7265013f-2071-4583-a8d6-af903dd583fb" />
2 participants
Closes #1157 — AI-3216
What & why
The reader showed each status in three places, used Fluent chrome where the app has its own, and could disagree with itself. Each status now lives in one place: Checks and Reviews tabs carry an outcome dot, and the header is lifecycle,
repo #nand the GitHub link. Buttons use the app'skcapGhost/kcapPrimaryclasses, tooltips follow the rail footer's text-plus-caption pattern, and every tab shares one content inset. Loading and empty states share one centred slot, and the footer reports only when the open tab was last updated; refresh lives on the About this work pane, which reloads the reader too. A session that switches branch finds the PR opened on the new one. The open tab reloads on the 30s poll unless it was paged past page one, and a tab reopened after 30s reloads, so rows cannot fall behind the checks summary; a new PR head reloads the checks without a prompt. The conversation tab is named Comments.Where to look
GitRepository.CurrentBranchreads the checkout's HEAD without a git process. The daemon still reports the branch the worktree was cut on, and PR discovery and the Session pane now prefer HEAD.RequestRefresh: a click waits out a 3s gap instead of the poll's 15s. Progress (ShowsRefreshing, the pane'sIsRefreshing) shows only for a click; the poll reads silently.PulseClockdrives every pending marker from one timer, which runs only while one is on screen.RemoteAvatarfetcheshttps://{host}/{login}.pngthroughMarkdownImages, the same credential-free fetch markdown images use.Verification
dotnet run --project test/Capacitor.App.Tests.Unit: 2761 passed, 0 failed.GitRepository*tests inCapacitor.Cli.Core.Tests.Unit: 20 passed. AOT publish ofCapacitor.Cli: 0 IL warnings.Visuals