Skip to content

feat(ui): add Cmd+P pull request dropdown - #309

Merged
forketyfork merged 8 commits into
mainfrom
claude/cmd-p-pr-dropdown-ydwn5
Aug 22, 2026
Merged

feat(ui): add Cmd+P pull request dropdown#309
forketyfork merged 8 commits into
mainfrom
claude/cmd-p-pr-dropdown-ydwn5

Conversation

@forketyfork

@forketyfork forketyfork commented May 14, 2026

Copy link
Copy Markdown
Owner

Issue

PR #309 was created against an old main, conflicted with the current branch, and its original CI run failed. The PR dropdown also had correctness issues around stale repository results and branch badges, and fetch failures were reduced to a generic UI message with no diagnostic output. Its implementation had grown into a large file combining unrelated responsibilities.

Solution

Rebased the feature onto current main and adapted it to the current UI text-input and test-registration infrastructure. Pull-request fetches now use repository-keyed worker jobs; results are applied only for the focused repository, while stale jobs are discarded and a new repository fetch starts immediately. The component rereads .git/HEAD during updates so branch badges follow checkouts without cwd changes.

gh launch, output collection, exit, JSON-shape, and parsing failures now emit scoped ERROR logs with the repository path and escaped stdout/stderr previews capped at 2 KiB after escaping per stream. The preview is formatted into a fixed buffer so it stays below the logger's 4 KiB record limit. The logging behavior is documented in docs/configuration.md.

The PR dropdown is now decomposed into model, repository, fetch, and view modules. The component retains UI lifecycle, input handling, and fetch-job ownership, while the new modules isolate domain helpers, Git inspection, gh parsing, and rendering/cache code. Every test-bearing module is registered explicitly, and the boundaries are documented in docs/ARCHITECTURE.md.

Context

Original implementation context: https://claude.ai/code/session_01BUkFdLJ8vdB36bfZ13MksA

Both original review threads were addressed, replied to, and resolved. The branch now contains the refactor in commit 060aa0b and was force-pushed after verification.

Test plan

  • Open the PR dropdown in a GitHub repository, then inspect the configured Architect log after a fetch failure and confirm the pr_dropdown entry includes the repository path and bounded escaped output preview without a message exceeded 4096-byte logging buffer entry.
  • Enter a repository with a checked-out PR branch and confirm the collapsed pill resolves to the PR number without opening the dropdown first.
  • Expand the PR pill beside other pills and confirm the expanded panel remains below sibling overlays without overlap.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

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: cb76d7cfb4

ℹ️ 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".

Comment thread src/ui/components/pr_dropdown.zig Outdated
Comment thread src/ui/components/pr_dropdown.zig
When the focused terminal sits inside a GitHub-cloned repo, ⌘P now expands a
pill that lists open pull requests fetched via `gh pr list` on a worker
thread. Selecting an entry sends `gh pr checkout <N>` to the focused shell.
The collapsed pill shows the current branch's PR number (e.g. #123) when the
HEAD ref matches a fetched PR's head branch.

Detection of the GitHub origin and current branch is done synchronously on
the main thread from `.git/config` and `.git/HEAD` (including worktree
indirection), so the pill is hidden cheaply when the cwd is not a GitHub
repo. Listing PRs hits the network through gh, so that work runs on a
per-open std.Thread with results delivered via a mutex-guarded slot and an
atomic completion flag, matching the spirit of ADR-009.

https://claude.ai/code/session_01BUkFdLJ8vdB36bfZ13MksA
forketyfork added a commit that referenced this pull request Aug 21, 2026
Issue: PR #309 was based on stale main, failed against the current UI APIs, and could apply pull request data from a previous repository or leave its branch badge stale.\nSolution: Rebase the feature onto current main, adapt its search field and test registration to the current UI foundations, and keep repository-keyed fetch jobs so stale results are discarded while the newly focused repository fetches immediately. Refresh the branch from HEAD on each update and cover repository/result and branch/PR matching with tests.
@forketyfork
forketyfork force-pushed the claude/cmd-p-pr-dropdown-ydwn5 branch from cb76d7c to 3183b79 Compare August 21, 2026 14:43
Issue: PR #309 was based on stale main, failed against the current UI APIs, and could apply pull request data from a previous repository or leave its branch badge stale.\nSolution: Rebase the feature onto current main, adapt its search field and test registration to the current UI foundations, and keep repository-keyed fetch jobs so stale results are discarded while the newly focused repository fetches immediately. Refresh the branch from HEAD on each update and cover repository/result and branch/PR matching with tests.
@forketyfork
forketyfork force-pushed the claude/cmd-p-pr-dropdown-ydwn5 branch from 3183b79 to 8b0040d Compare August 21, 2026 14:49
Issue: PR dropdown fetch failures only surfaced as a generic UI message, leaving no diagnostic record for malformed gh responses or CLI failures.\nSolution: Add scoped error logs for gh lifecycle failures and JSON processing, including the repository path and escaped, bounded output previews. Document the new diagnostics so future dropdown failures can be investigated from the configured log.
Issue: Reproduced PR fetch failures caused the diagnostic output preview to exceed Architect's 4 KiB logging buffer, so the malformed gh response was still omitted from the log.\nSolution: Format escaped output into a fixed 2 KiB buffer, escaping structured-log delimiters and stopping at an encoded boundary. Add a regression test for the bound and document the effective preview limit.
Issue: gh can emit ANSI color sequences around --json output, causing the pull request dropdown to report a JSON parse failure.\n\nSolution: remove ANSI CSI terminal formatting before parsing gh output and cover the captured colorized response with a regression test. Document the normalization in the logging configuration reference.
Issue: four-digit pull request numbers can exceed the compact PR pill and render outside its bounds.\n\nSolution: constrain oversized PR number textures to the pill's available width while preserving their aspect ratio, leaving the default Cmd+P glyph unchanged. Add a regression test for proportional fitting and document the behavior.
Issue: the PR overlay rendered above sibling pills during their expansion, and the collapsed badge stayed at Cmd+P until the overlay was opened after entering a repository.\n\nSolution: render the PR component below the other pill overlays and start the initial repository-keyed PR fetch as soon as a GitHub repository becomes focused. The resulting branch match updates the collapsed badge without requiring an overlay interaction.
Issue: pr_dropdown.zig combined UI orchestration, repository inspection, gh process handling, JSON parsing, and SDL rendering in one large component.

Solution: split those responsibilities into model, repository, fetch, and view modules while keeping background-job ownership and UI lifecycle in the component. Register every new test-bearing module and document the boundaries.
@forketyfork
forketyfork merged commit e5597e0 into main Aug 22, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants