GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — feat(site): per-ref views in the git panel by DanielleMaywood · Pull Request #29544 · coder/coder Conversation
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this 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: 896d6f495c
ℹ️ 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".
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| ], | ||
| }); | ||
|
|
||
| // The default view targets the primary ref. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove comments that merely narrate assertions
This comment, along with the similar comments before the assertions around lines 102, 132, and 168, only paraphrases the immediately following expectation without explaining a non-obvious constraint. Remove these narration comments and retain only rationale such as the legacy-keyless-row explanation later in the test.
AGENTS.md reference: site/AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 7e27d9d. The narration comments cited at lines 102, 132, and 168 were removed in an earlier pass; the one remaining narration comment (before the branch-only fetch assertion) is now gone too. The fixture-rationale comments for the keyless legacy row stay, as you suggested.
Generated by Coder Agents
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| triggerIdentifier: branchName || "Branch", | ||
| itemPrimary: "Branch", | ||
| itemSecondary: branchName || undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Include the remote in branch-only ref labels
When multiple tracked repositories use the same branch name, such as feature/foo, these fields render identical dropdown entries and an identical active trigger even though the ref identity and fetched diff also depend on remote_origin. Users, including screen-reader users, cannot determine which repository they are selecting, so include a repository or origin identifier in the branch-only label.
AGENTS.md reference: site/AGENTS.md:L12-L12
Useful? React with 👍 / 👎.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 7e27d9d, for both remote row kinds. When a chat tracks more than one origin, every remote entry names its repository (parsed deterministically from remote_origin): branch-only rows read "coder/coder · feature/shared" and PR rows read "coder/other-project · PR #5", in both the dropdown and the active trigger. PR rows get the same treatment because PR numbers are only unique per repository, so they had the identical cross-repo ambiguity. Single-origin chats keep the old labels unchanged. A test asserts the menu items' accessible names are all distinct, and a new MultipleOrigins story covers the visual state.
Generated by Coder Agents
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
896d6f4 to
86c5cf6
Compare
86c5cf6 to
e87f9d8
Compare
9adea34 to
bcdc567
Compare
bcdc567 to
975c2cc
Compare
b791a68 to
9c6655d
Compare
|
@codex review |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this 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: 7e27d9d0d4
ℹ️ 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".
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| const effectiveView = fallbackView(view, { | ||
| localRepos, | ||
| remoteDiffStats, | ||
| primaryRefId, | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Persist the reconciled fallback selection
When the selected ref or repository disappears during a status update, fallbackView changes only effectiveView; the stored view remains pointed at the missing item. If that item later reappears, such as after a ref is re-reported or a repository watcher reconnects, the panel silently switches back to it instead of retaining the fallback that the user has been shown. Persist the fallback when the current selection becomes invalid, as the removed reconciliation effect previously did.
AGENTS.md reference: site/AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping the derived reconciliation without persistence.
A ref row cannot disappear and reappear: the server has no delete path for chat_diff_statuses, so rows only leave when the chat is deleted, which remounts the panel. The only reachable case where the selected ref stops matching is the keyless primary being demoted by a newer ref, and falling back to the new primary there is intended and covered by tests.
For a local view, the reachable disappearance is a transient agent disconnect. Returning to the repo the user was viewing once the watcher reconnects is the intended everDirty behavior. Persisting the fallback would discard that selection permanently because of a temporary disconnect.
This reply was generated by Coder Agents.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
7e27d9d to
9b020bf
Compare
a9210b7 to
48935e7
Compare
48935e7 to
045fbb8
Compare
045fbb8 to
2ade5e8
Compare
The git panel now shows one view per tracked ref. Selecting a ref loads that ref's diff. A chat tracks one status row per ref, and the first row is the primary. The view switcher lists one entry per ref from diff_statuses. Each entry shows the PR state and number, or the branch name when no PR exists. Selecting an entry fetches that ref's diff with its origin and branch selector. The diff body, PR title, and stats follow the selection. A selection that resolves to nothing falls back to the newest ref. The inline comment box resets when the ref switches, so a comment draft cannot leak from one PR into another.
Every mock data fixture now starts with mock: mockDiffStatus, mockChatWithDiffStatus, and mockDiffContents. Factory helpers keep their make prefix: makePrStatus and makeRepo.
Every call site passed the same chat id. A const removes the argument that no caller varied.
The effective view was a four-level ternary chain. It is now a reconcile function whose branches read in order: hidden remote tab, untracked ref, gone repo root, then the fallbacks. The selector object for the selected ref is a named variable instead of an inline JSX object, and the closure predicates use named variables instead of re-narrowing the union.
The query enable conditions are named variables: hasPrUrl and hasRefSelector state what the enable rule checks.
The component mixed its derivation logic inline: diff line counting, repo set unions, switcher item building, view reconciliation, and the selected-ref lookup each lived as nested code inside the component body. Each is now a module-level helper with a name, and the component reads as wiring between them. The diff fetch enable check states its contract in one line.
The view selection, the selected row, and its PR metadata were one unbroken run of declarations. Each group now sits under its own comment with a blank line between groups.
The first row of diff_statuses is no longer the primary. The git panel now gets the primary from diff_status. It uses the primary to pick the default view and to decide if a keyless row can show.
jakehwll
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little stressed about injection, pre approving for that fix 🙂
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| {/* A remount per ref keeps any open inline comment box | ||
| from leaking into a different PR's diff. */} | ||
| <CommentableDiffViewer | ||
| key={`${remoteRef?.remote_origin ?? ""}/${remoteRef?.git_branch ?? ""}`} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love this
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
2ade5e8 to
cb0dbd0
Compare
The remount key joined the origin and the branch with "/", and both can contain "/". Two refs could then share a key and skip the remount. The view switcher already has a unique id for each ref, so RemoteContent now uses it as its key.
Uh oh!
There was an error while loading. Please reload this page.
Part 5 of 6. Depends on #29544. Supersedes #29545, which merged with a flattened head by mistake. The top bar PR chip now reads `diff_statuses` instead of the single primary. With one PR it stays a direct link. With several it shows "N PRs" and opens a menu with one link per PR. When the PRs span multiple origins, each entry is prefixed with `owner/repo` so equal PR numbers stay distinct. Refs without a PR number (branch-only refs) are left out. Each menu entry shows the PR state icon, the PR number, the title on one line, and an external-link icon. Screen readers hear the PR state. The menu is 288px wide and shows five entries before it scrolls. `PRMenuLinks` renders the entries. `originRepoLabel` moves from the git panel to `utils/originRepoLabel.ts` so both places share it. Generated by Coder Agents.
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
Labels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
3 participants
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.