Skip to content

Simplify the desktop pull request reader #1158

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alexeyzimarev merged 12 commits into from
Sep 25, 2026
Merged

Simplify the desktop pull request reader #1158

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
b8bd021
Simplify the desktop pull request reader (#1157)
nortonandreev Sep 25, 2026
0e578bf
Pulse only the pending marker, on one shared clock (#1157)
nortonandreev Sep 25, 2026
4f0a7a4
Show the resolved-threads filter as a switch on a fixed-height row (#…
nortonandreev Sep 25, 2026
705aafb
Keep a session's pull requests and checks current while it runs (#1157)
nortonandreev Sep 25, 2026
4421ac0
Move the pull request reader onto the app's button and tooltip styles…
nortonandreev Sep 25, 2026
3d876f1
Give the pull request reader one loading, empty and updated state (#1…
nortonandreev Sep 25, 2026
3d8a73b
Reload the open pull request tab on the poll (#1157)
nortonandreev Sep 25, 2026
482260d
Reload a stale pull request tab when it is opened again (#1157)
nortonandreev Sep 25, 2026
3f9f2f6
Steady the pull request reader's state glyphs and refresh control (#1…
nortonandreev Sep 25, 2026
fee2033
Reload checks on their own when the PR head changes (#1157)
nortonandreev Sep 25, 2026
74cdef7
Reset head-change recovery when the pull request changes (#1157)
nortonandreev Sep 25, 2026
bbd3af6
Show the selected pull request while it is still loading (#1157)
nortonandreev Sep 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions src/Capacitor.App/App.axaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,31 @@
<Setter Property="Foreground" Value="{StaticResource KcapFaintBrush}" />
<Setter Property="Opacity" Value="0.55" />
</Style>
<!-- Ghost sizes. Declared after the ghost rules on purpose: a later style wins, so a
size that also recolours has to come after the colour it replaces. -->
<Style Selector="Button.kcapGhost.kcapIcon">
<Setter Property="Padding" Value="7" />
<Setter Property="MinWidth" Value="30" />
<Setter Property="MinHeight" Value="30" />
<Setter Property="CornerRadius" Value="7" />
<Setter Property="HorizontalContentAlignment" Value="Center" />
<Setter Property="VerticalContentAlignment" Value="Center" />
</Style>
<Style Selector="Button.kcapGhost.kcapQuiet">
<Setter Property="Padding" Value="8,5" />
<Setter Property="FontSize" Value="12.5" />
<Setter Property="CornerRadius" Value="6" />
</Style>
<Style Selector="Button.kcapGhost.kcapQuiet /template/ ContentPresenter#PART_ContentPresenter">
<Setter Property="Foreground" Value="{StaticResource KcapMutedBrush}" />
</Style>
<Style Selector="Button.kcapGhost.kcapQuiet:pointerover /template/ ContentPresenter#PART_ContentPresenter,
Button.kcapGhost.kcapQuiet:pressed /template/ ContentPresenter#PART_ContentPresenter">
<Setter Property="Foreground" Value="{StaticResource KcapTextBrush}" />
</Style>
<Style Selector="Button.kcapGhost.kcapQuiet:disabled /template/ ContentPresenter#PART_ContentPresenter">
<Setter Property="Foreground" Value="{StaticResource KcapFaintBrush}" />
</Style>
<!-- Destructive ghost (Stop): Fluent hover paints the presenter near-white, which hides
the danger colour. Rest and hover both pin KcapDangerBrush. -->
<Style Selector="Button.kcapDanger">
Expand Down Expand Up @@ -522,6 +547,17 @@
<Setter Property="BorderBrush" Value="Transparent" />
<Setter Property="Foreground" Value="{StaticResource KcapTextBrush}" />
</Style>
<!-- A refresh glyph turning while its read is in flight: the click's only feedback when
the result is unchanged. -->
<Style Selector="Path.kcapSpinning">
<Setter Property="RenderTransform" Value="rotate(0deg)" />
<Style.Animations>
<Animation Duration="0:0:0.9" IterationCount="Infinite">
<KeyFrame Cue="0%"><Setter Property="RotateTransform.Angle" Value="0" /></KeyFrame>
<KeyFrame Cue="100%"><Setter Property="RotateTransform.Angle" Value="360" /></KeyFrame>
</Animation>
</Style.Animations>
</Style>
<!-- In-flight state markers (a starting session's dot, a running check, a live tool-call
row) share one pulse. -->
<Style Selector=":is(Control).pulsing, Border.toolRunning">
Expand Down
2 changes: 1 addition & 1 deletion src/Capacitor.App/ViewModels/PullRequestContextViewModel.Presentation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public PullRequestStatus ChecksStatus {
if (!CanDisplay) return new("");
var checks = _sections.GetValueOrDefault("checks");
if (checks is { Coverage: "complete", Stopped: false, Total.Kind: "exact", Completed: { } completed }
&& checks.Head == _overview?.HeadSha && _time.GetUtcNow().UtcDateTime - completed < TimeSpan.FromSeconds(30)
&& checks.Head == _overview?.HeadSha && _time.GetUtcNow().UtcDateTime - completed < RowsFreshFor
&& completed >= _overview?.Checks?.Availability.FetchedAt && checks.Pages.Sum(page => page.Rows.Length) == checks.Total.Value) {
var rows = checks.Pages.SelectMany(page => page.Rows).ToArray();
if (rows.Length == 0) return new("No checks reported", Detail: "No checks were reported for this commit.");
Expand Down
86 changes: 58 additions & 28 deletions src/Capacitor.App/ViewModels/PullRequestContextViewModel.Projections.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,18 @@ namespace Capacitor.App.ViewModels;
public sealed partial class PullRequestContextViewModel {
public string Title => CanDisplay && _overview?.Title is { } title ? title
: _selected is { IsAvailable: true } choice ? choice.Link.Title ?? choice.Label : _selected?.Label ?? "";
public string Branches => CanDisplay ? (_overview?.HeadRef ?? "?") + " → " + (_overview?.BaseRef ?? "?") : "";
public string FetchedLabel => CanDisplay && _overviewRead?.FetchedAt is { } at ? "Fetched " + at.ToLocalTime().ToString("HH:mm:ss", CultureInfo.CurrentCulture) : "";
public string Branches => CanDisplay && (_overview?.HeadRef is not null || _overview?.BaseRef is not null)
? (_overview.HeadRef ?? "?") + " → " + (_overview.BaseRef ?? "?") : "";
public bool HasBranches => Branches.Length > 0;
public string FetchedLabel => CanDisplay && _overviewRead?.FetchedAt is { } at ? "Updated " + at.ToLocalTime().ToString("HH:mm:ss", CultureInfo.CurrentCulture) : "";
public string AccessLabel => CanDisplay ? (_grace ? "Access refresh paused" : "Access checked for " + (_overview?.AccessCheckedFor ?? "linked GitHub account")) : "";
public string? Description => CanDisplayReader && _section == "overview" ? _overview?.Description : null;
public bool DescriptionTruncated => CanDisplayReader && _overview?.DescriptionTruncated == true;
public string DescriptionNote => !CanDisplayReader ? "Refresh access to open PR content." : _overview?.Description is null ? "Description unavailable." : _overview.Description.Length == 0 ? "No description." : "";
/// The selected pull request has not arrived yet. Distinct from a section's own first load,
/// which runs only once that overview can be shown.
public bool IsSelectionLoading => IsReading && !CanDisplay && _selected is { IsAvailable: true };
public bool ShowsLoading => IsSelectionLoading || IsSectionLoading;
public string DescriptionNote => IsSelectionLoading ? "" : !CanDisplayReader ? "Refresh access to open PR content." : _overview?.Description is null ? "Description unavailable." : _overview.Description.Length == 0 ? "No description." : "";
public bool IsOverview => _section == "overview";
public bool IsThreads => _section == "threads";
public bool IsThreadComments => _section == "thread_comments";
Expand All @@ -28,33 +34,47 @@ public sealed partial class PullRequestContextViewModel {
public string PageNote {
get {
if (_section == "overview") return "";
if (!CanDisplayReader) return "Refresh access to open PR content.";
if (CurrentSection is not { } state) return _pageRequests.Contains(SectionKey) ? "Loading…" : "Choose Refresh to load this section.";
if (!CanDisplayReader) return IsSelectionLoading ? "" : "Refresh access to open PR content.";
if (CurrentSection is not { } state) return _pageRequests.Contains(SectionKey) ? "" : "This section has not loaded yet.";
if (state.Error is { } error) return error;
if (state.Coverage != "complete") return "Limited snapshot: an ordered subset. More may be available on GitHub.";
if (state.Pages.Count > 0 && state.Pages.Sum(page => page.Rows.Length) == 0) return _section switch {
return "";
}
}
public string EmptyNote {
get {
if (_section == "overview" || !CanDisplayReader || CurrentSection is not { Error: null } state) return "";
if (state.Pages.Count == 0 || state.Pages.Sum(page => page.Rows.Length) > 0) return "";
return _section switch {
"threads" when state.Excluded is { Kind: "exact", Value: > 0 } => $"No unresolved threads ({state.Excluded.Value} resolved).",
"threads" => IncludeResolved ? "No threads." : "No unresolved threads.", "checks" => "No checks reported.", "reviewers" => "No reviewers reported.",
"reviews" => "No published reviews.", _ => "No comments."
"threads" => IncludeResolved ? "No threads." : "No unresolved threads.", "checks" => "No checks reported.", "reviewers" => "No reviewers yet.",
"reviews" => "No submitted reviews.", _ => "No comments."
};
return "";
}
}
public bool HasEmptyNote => EmptyNote.Length > 0;
/// A first load only: a reload keeps the rows it is replacing on screen.
public bool IsSectionLoading => _section != "overview" && CanDisplayReader && CurrentSection is null && _pageRequests.Contains(SectionKey);
public string LoadingNote => "Loading " + (IsSelectionLoading ? "pull request" : _section switch {
"checks" => "checks", "reviewers" => "reviewers", "reviews" => "reviews", "threads" => "threads", "thread_comments" => "replies", _ => "comments"
}) + "…";
public string SnapshotLabel => CanDisplayReader && CurrentSection?.Completed is { } at
? "Snapshot " + at.ToLocalTime().ToString("HH:mm:ss", CultureInfo.CurrentCulture) + (CurrentSection.Head is { Length: >= 7 } head ? " · " + head[..7] : "") : "";
public string SectionTitle => _section switch { "checks" => "Checks", "reviewers" => "Reviewers", "reviews" => "Published reviews",
"threads" => "Inline threads", "thread_comments" => "Thread replies", "conversation" => "Conversation", _ => "Description" };
? "Updated " + at.ToLocalTime().ToString("HH:mm:ss", CultureInfo.CurrentCulture) : "";
/// The footer names one thing, the time; which commit the checks ran on rides its hover.
public string FreshnessDetail => !IsOverview && CanDisplayReader && CurrentSection?.Head is { Length: >= 7 } head
? "Checks ran on commit " + head[..7] : "When GitHub last sent what this tab shows";

static readonly string[] NotifiedProperties = [
nameof(Notice), nameof(IsReading), nameof(HasChoice), nameof(HasPullRequest), nameof(HasListed), nameof(IsLegacy), nameof(CanOpenReader), nameof(Section), nameof(CanReveal), nameof(CanDisplay),
nameof(Title), nameof(Branches), nameof(FetchedLabel), nameof(AccessLabel),
nameof(Description), nameof(DescriptionTruncated), nameof(DescriptionNote), nameof(IsOverview), nameof(IsThreads), nameof(IsThreadComments), nameof(IncludeResolved),
nameof(Title), nameof(Branches), nameof(HasBranches), nameof(EmptyNote), nameof(HasEmptyNote), nameof(IsSectionLoading), nameof(LoadingNote), nameof(ShowsRefreshing), nameof(FetchedLabel), nameof(AccessLabel),
nameof(Description), nameof(DescriptionTruncated), nameof(DescriptionNote), nameof(IsOverview), nameof(IsThreads), nameof(IsThreadComments), nameof(IncludeResolved), nameof(IsSelectionLoading), nameof(ShowsLoading),
nameof(HasNotice), nameof(ShowsSignIn), nameof(ShowsLinkGitHub), nameof(ShowReaderContent), nameof(Rows), nameof(HasMore),
nameof(CanReloadEarlier), nameof(PageNote), nameof(SnapshotLabel), nameof(SectionTitle),
nameof(CanReloadEarlier), nameof(PageNote), nameof(SnapshotLabel), nameof(FreshnessDetail),
nameof(ReaderNote), nameof(HasReaderNote), nameof(ShowsInstallTool), nameof(InstallToolLabel),
];

void Notify() {
if (!IsReading) _userRefresh = false;
_readerNote = _readers is null ? null
: _selected?.Subject is { } subject ? _readers.NoteFor(subject.Provider, subject.Host)
: _primaryRepo?.Invoke() is { } repository ? _readers.NoteFor(repository.Provider, repository.Host) : null;
Expand Down Expand Up @@ -96,6 +116,11 @@ void ReconcileRows(PullRequestRow[] rows) {
};
static string Author(PullRequestActorDto? actor) => actor is null ? "Unknown author" : actor.Kind == "team"
? actor.Name ?? actor.Login ?? "Team" : actor.Login ?? actor.Name ?? "Unknown author";
static string Joined(params string?[] parts) => string.Join(" · ", parts.Where(part => !string.IsNullOrEmpty(part)));
/// GitHub serves a user's avatar at /{login}.png on github.com and Enterprise hosts alike; teams and bots keep their glyph.
static string? AvatarUrl(PullRequestActorDto? actor, PullRequestSubjectDto subject) =>
subject.Provider == "github" && actor is { Kind: "user", Login: { Length: > 0 } login } && Uri.CheckHostName(subject.Host) == UriHostNameType.Dns
? $"https://{subject.Host}/{Uri.EscapeDataString(login)}.png?size=64" : null;
static string Dated(DateTime? at) => at?.ToLocalTime().ToString("g", CultureInfo.CurrentCulture) ?? "";
static string Outcome(string? value) => value switch {
"success" => "Passed", "failure" => "Failed", "pending" => "Pending", "neutral" => "Neutral", "skipped" => "Skipped",
Expand All @@ -109,28 +134,33 @@ PullRequestRow ToRow(PullRequestCommentDto item, PullRequestSubjectDto subject)
? new(item.Id, Author(item.Author), Dated(item.CreatedAt), item.Body, null, PrLink(item.Url, subject), "available", item.BodyTruncated, ActorKind: item.Author?.Kind)
: Unavailable(item.Id, "Comment", PrLink(item.Url, subject), item.Availability);
PullRequestRow ToRow(PullRequestReviewDto item, PullRequestSubjectDto subject) => item.Availability == "available" && item.State is not ("pending" or "PENDING")
? new(item.Id, Author(item.Author), ReviewState(item.State) + " · " + Dated(item.SubmittedAt), item.Body, null, PrLink(item.Url, subject), "available", item.BodyTruncated)
? new(item.Id, Author(item.Author), Joined(ReviewState(item.State), Dated(item.SubmittedAt)), item.Body, null, PrLink(item.Url, subject), "available", item.BodyTruncated)
: Unavailable(item.Id, "Review", PrLink(item.Url, subject), item.Availability);
PullRequestRow ToRow(PullRequestReviewerDto item, PullRequestSubjectDto subject) => item.Availability == "available"
? new(item.Id, Author(item.Actor), (item.Requested == true ? "Review requested · " : "") + ReviewState(item.ReviewState),
null, null, PrLink(item.Url, subject), "available", IsReviewer: true, ActorKind: item.Actor?.Kind,
Status: ReviewRowStatus(item.ReviewState), ReviewRequested: item.Requested == true)
: Unavailable(item.Id, "Reviewer", null, item.Availability) with { IsReviewer = true };
PullRequestRow ToRow(PullRequestReviewerDto item, PullRequestSubjectDto subject) {
if (item.Availability != "available") return Unavailable(item.Id, "Reviewer", null, item.Availability) with { IsReviewer = true };
var status = ReviewRowStatus(item.ReviewState, item.Requested == true);
return new(item.Id, Author(item.Actor), (item.Requested == true ? "Review requested · " : "") + status.Text,
null, null, PrLink(item.Url, subject), "available", IsReviewer: true, ActorKind: item.Actor?.Kind, AvatarUrl: AvatarUrl(item.Actor, subject),
Status: status, ReRequested: item.Requested == true && item.ReviewState is not null);
}
PullRequestRow ToRow(PullRequestCheckDto item, PullRequestSubjectDto subject) => item.Availability == "available"
? new(item.Id, item.Name ?? "Unnamed check", Outcome(item.Outcome) + " · " + (item.AppName ?? item.Source ?? "Unknown source")
+ (PullRequestWire.CheckLink(item.Url) is { } url ? " · " + new Uri(url).Host : ""),
? new(item.Id, item.Name ?? "Unnamed check",
Joined(item.AppName ?? item.Source, PullRequestWire.CheckLink(item.Url) is { } url ? new Uri(url).Host : null),
null, null, PullRequestWire.CheckLink(item.Url), "available", IsCheck: true, Outcome: item.Outcome ?? "unknown",
Status: new(Outcome(item.Outcome), item.Outcome switch {
"success" => "success", "failure" or "timed_out" or "action_required" => "failure", "pending" => "pending", _ => "neutral"
}))
: Unavailable(item.Id, "Check", PullRequestWire.CheckLink(item.Url), item.Availability) with { IsCheck = true, Outcome = "unknown" };
static PullRequestStatus ReviewRowStatus(string? state) => new(ReviewState(state), state switch {
"approved" => "success", "changes_requested" => "failure", "commented" => "commented", _ => "neutral"
});
static PullRequestStatus ReviewRowStatus(string? state, bool requested) => state switch {
"approved" => new("Approved", "success"), "changes_requested" => new("Changes requested", "failure"),
"commented" => new("Commented", "commented"), "dismissed" => new("Dismissed"),
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."),
Comment on lines +157 to +158

@qodo-code-review qodo-code-review Bot Sep 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

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

@nortonandreev nortonandreev Sep 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

};
PullRequestRow ToRow(PullRequestThreadDto item, PullRequestSubjectDto subject) => item.Availability == "available"
? new(item.Id, (item.Path ?? "Unknown file") + (item.Line is { } line ? ":" + line.ToString(CultureInfo.InvariantCulture) : ""),
(item.IsResolved == true ? "Resolved" : item.IsResolved == false ? "Unresolved" : "Resolution unknown")
+ (item.IsOutdated == true ? " · Outdated" : "") + " · " + Author(item.RootComment?.Availability == "available" ? item.RootComment.Author : null),
Joined(item.IsResolved switch { true => "Resolved", false => "Unresolved", _ => null }, item.IsOutdated == true ? "Outdated" : null,
item.RootComment is { Availability: "available", Author: { } author } ? Author(author) : null),
item.RootComment?.Availability == "available" ? item.RootComment.Body : null, item.DiffHunk, PrLink(item.Url, subject),
"available", item.HunkTruncated || item.RootComment?.BodyTruncated == true, IsThread: true)
: Unavailable(item.Id, "Thread", PrLink(item.Url, subject), item.Availability);
Expand Down
Loading
Loading