Skip to content

fix(web): ask before Close with comment closes a pull request - #18175

Merged
juliusmarminge merged 4 commits into
pingdotgg:mainfrom
ilyaliao:fix/pr-dashboard-dialog-buttons
Oct 20, 2026
Merged

juliusmarminge merged 4 commits into
pingdotgg:mainfrom
ilyaliao:fix/pr-dashboard-dialog-buttons

Conversation

@ilyaliao

@ilyaliao ilyaliao commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Problem

“Close with comment” closes a pull request on its first click, while the other close controls ask for confirmation.

Change

The comment form now uses the same inline confirmation behavior: the first click arms the action, and a second click confirms it.

Clicking away, pressing Escape, or letting the confirmation expire cancels it without clearing the draft. “Reopen with comment” remains a single-click action.

Scope and approval

This is a maintainer-requested takeover of the existing focused bug fix.

Based on the original fix by @ilyaliao.

Verification

Scoped web typecheck, lint, and format checks pass. No new unit tests were added for reuse of the existing confirmation hook. Independent Opus and Sol full-diff reviews found no defects.

Browser verification used the unchanged production comment form, hook, and styles in an inert form-level fixture at 1280×800. It did not use the full PR dashboard or make GitHub requests. Before, one click submitted immediately. After, the first click made no callback and the second made exactly one trimmed close/comment callback. Outside/textarea clicks, Escape, Tab-away, and the three-second timeout disarmed while preserving the draft. Reopen remained one click; empty/pending states prevented duplicate submission. The close button stayed 152.79 px wide while idle, armed, and pending.

Before: first click submits After: first click asks for confirmation
Before: production form submits close immediately in inert fixture After: production form asks Confirm close with no callback

Before recording · After recording

No backend, authentication, real PR close, or Electron shell integration was exercised.

Taken over with Claude Opus 5.5 (tests and review), Claude Fable 5.1 (implementation), and GPT-6.1 Sol (review) through T3 Code; coordinated with Codex.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Oct 11, 2026
macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Oct 11, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 5ebb133

Macroscope's review found this PR approvable — This is a small, self-contained UI bug fix that adds confirmation before the existing close-with-comment action while preserving other comment and reopen flows. It introduces no schema, infrastructure, sensitive-area, product-default, or static-analysis changes.

You can add or adjust custom eligibility rules. Learn more.

@juliusmarminge juliusmarminge added the macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews label Oct 11, 2026 — with ChatGPT Codex Connector
@macroscopeapp
macroscopeapp Bot dismissed their stale review October 11, 2026 09:29

Dismissing prior approval to re-evaluate 564d009

@coderabbitai

coderabbitai Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: a5f1edf6-d80a-44d2-88e3-9730063b1b48
📥 Commits

Reviewing files that changed from the base of the PR and between 564d009 and b3aa48d.

📒 Files selected for processing: 1
  • apps/web/src/components/pullRequest/PullRequestCommentForm.tsx

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.

📝 Walkthrough

The pull request comment form now requests confirmation before submitting a close action. Reopen actions still submit directly. The close button displays confirmation and submitting labels, with stable width across labels.

Changes: Pull request actions

Layer / File(s) Summary
Close and reopen action handling
apps/web/src/components/pullRequest/PullRequestCommentForm.tsx
The form uses pullRequestReviewKey and useInlineActionConfirm to confirm close actions before submission. Reopen actions submit directly. Button labels distinguish pending confirmation and submission states, while an invisible label keeps the close button width stable.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to b3aa4

Close with comment now asks for confirmation before closing a pull request. Reopen is unchanged. No merge-blocking risk was found.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to 564d0

The change adds confirmation before an existing close action without expanding access or changing permissions. The confirmation is tied to the selected pull request and button, while existing submission and failure handling remain intact.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The introduced behavior affects intentional submission for the selected pull request. It does not add a new network entrypoint, target-selection mechanism, credential capability or broader action scope: the confirming click invokes the same existing close submission path.

Trust Boundaries and Controls

  • observed — Close and reopen remain conditional on pull-request state, provider capabilities and viewer permissions. Confirmation is an additional client-side intent control, not a replacement for authorization. The textarea shortcut still submits only a comment, not a close action.
Pre-merge checks: Passed 4
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Title check Passed The title clearly and concisely describes the main change: requiring confirmation before “Close with comment” closes a pull request. It uses a conventional commit format.
Description check Passed The description includes all required sections: Problem, Change, Scope and approval, and Verification. It explains the behavior, approval context, focused verification, UI evidence, limitations, and a…
Full details: Description check

Explanation

The description includes all required sections: Problem, Change, Scope and approval, and Verification. It explains the behavior, approval context, focused verification, UI evidence, limitations, and agent usage.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Oct 11, 2026
Close with comment in the pull request composer posted the comment and
closed the pull request on one click. It now confirms in place with
useInlineActionConfirm, like Close pull request in the header menu.
@ilyaliao
ilyaliao force-pushed the fix/pr-dashboard-dialog-buttons branch from 564d009 to b3aa48d Compare October 18, 2026 04:15
@juliusmarminge
juliusmarminge enabled auto-merge (squash) October 18, 2026 19:08
@juliusmarminge juliusmarminge self-assigned this Oct 18, 2026
@juliusmarminge

Copy link
Copy Markdown
Member

Taking over this one. I’ll reduce the tests to the useful coverage, update the implementation against current main, and verify it before publishing the changes.

juliusmarminge and others added 3 commits October 18, 2026 15:14
Drop the comment that restated the code and put the close branch first so
the reopen labels do not nest a second ternary.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@macroscopeapp
macroscopeapp Bot dismissed their stale review October 19, 2026 01:36

Dismissing prior approval to re-evaluate 5ebb133

@juliusmarminge
juliusmarminge merged commit 098eb55 into pingdotgg:main Oct 20, 2026
29 of 30 checks passed
aorwall added a commit to aorwall/t3code that referenced this pull request Oct 20, 2026
Merges upstream `pingdotgg/t3code` at `143ab1606d` (base `ab6be13d43`,
376 commits) into the fork. No merge PR was open, so this branches from
`main`.

## Merge

- **Landed:** 1177 files out of 1177 in the upstream range. The only
missing files are `apps/server/src/cli/pair.ts` and its test, which the
fork deliberately keeps deleted. The fork delta is now 817 files.
- **Conflicts:** 14, each resolved by its `preflight.mjs` verdict. The
tracker entry in `docs/fork/upstream-merge-log.md` covers each one. The
ones a reviewer should read:
- **`ProjectDefaultsSettings.tsx`:** upstream moved the
agent-browser-access row to `IntegrationsSettings.tsx`
(`AgentBrowserAccessSetting`). The fork's `workspaceOwnsProjectDefaults`
gate moved with it and returns null at a Workspace project scope.
- **`terminal.ts`:** keeps the fork's `TerminalMetadataState` seed and
takes upstream's `Stream.drop(1)`. Upstream's new `runtime.test.ts`
metadata test now reads `.terminals`.
- **`Sidebar.tsx` / `Sidebar.logic.ts`:** upstream's new
`resolveSidebarRowThreadId` resolves against the unfiltered shells. It
now takes `source`, because the fork's `isSidebarSubagentThread` hides
only provider-native subagents.
- **`ChatView.tsx` (auto-merged):** the fork's hosted script path calls
`storeSetActiveTerminal` again. The local path drops its early
`setTerminalOpen(true)`, following upstream pingdotgg#18340, so the drawer no
longer claims the default terminal id.
- **`AGENTS.md`:** keeps the fork's prose and adds upstream's new
`Effect.fn` rule.
- **`apps/mobile/package.json`:** takes upstream's test script and keeps
`tsc` rather than `tsc-rs`.
- **`pnpm-lock.yaml`:** `tsc-rs` entries stripped, then re-derived with
`install.mjs`.
- **Other fixes:**
- `fork/goToDefinition.test.ts` moved to browser mode for upstream's new
`no-dom-test-environment` lint rule.
- `ThreadDetailsPrRows.browser.test.tsx` now passes
`activeProjectScriptsEditable`.
- One new `duplicateAddExceptions` entry was added and one stale one
retired.
- **Unsupported methods:** ADD 0, DROP 0, KEEP 2.
`packages/contracts/src/rpc.ts` is unchanged.

## Verification

`verify.mjs --sequential`:
- duplicate-adds, tripwires, resolution-check, unsupported-methods,
lockfile, fmt, lint, typecheck and build all pass.
- `test` fails. The files that fail when run alone are the same five as
on 2026-10-19 (`AgentScope`, `OpenCodeServerLedger`, ssh `tunnel`,
`AcpAdapterV2`, `OrchestratorReplayFixtures`), all sandbox
process-reaping or auth issues.
- Two new failures are also environmental: the partial-clone tests in
`GitVcsDriver.test.ts` and `RepositoryIdentityResolver.test.ts`. The
sandbox's `~/.gitconfig` rewrites `git@github.com:` to https with
`insteadOf`.
- `@t3tools/web` had one real failure, upstream's new Sidebar subagent
test. It is fixed and passes on its own.

**Caveat:** the last amend changed only that test fixture and the docs.
The sandbox restarted three times during this run, and the typecheck and
fmt re-run after that amend did not finish. A follow-up comment will
report it.

## Usable as-is
- LaTeX/KaTeX math in chat markdown, behind the client setting
`mathRenderingEnabled` in Appearance, off by default (pingdotgg#18735).
- Mermaid renders while a message streams (pingdotgg#18308). Large changed-file
trees are virtualized (pingdotgg#21482, pingdotgg#21831). HTML previews skip the loading
skeleton (pingdotgg#21480).
- Pasted folders become workspace references (pingdotgg#22072). PR check rows
aligned, and skipped checks get a slash icon (pingdotgg#22090, pingdotgg#21498). A
confirmation before "Close with comment" (pingdotgg#18175).
- Subagent overflow badge (pingdotgg#19860). Context usage survives a fork
(pingdotgg#22075). Copy paths relative to the thread workspace (pingdotgg#21497). PDF
preview (pingdotgg#21095).
- Contract decoding tolerates newer PR shapes (`ForwardCompatibleArray`
/ `ForwardCompatibleOptional`; pingdotgg#19899, pingdotgg#21575, pingdotgg#21665).
- In-app browser search engine settings (pingdotgg#17688). These are client-only,
but they only matter where the browser panel is exposed.

## Unsupported in Moatless / needs implementation
- **`thread.stop` and `capabilities.threadStop` (pingdotgg#22052):** stops a
thread without resolving its run. The client falls back to the existing
stop path until the backend advertises `threadStop`. Added to the
capabilities gap.
- **`blocking: false` on async questions (pingdotgg#15430):** keeps a thread
Working while a question is open. The backend has to emit the field.
- **Provider `optionDescriptors` / `installedVersion` (pingdotgg#19519,
pingdotgg#19567):** drive dynamic model options and update checks. Both are
optional, so the client degrades.
- **Terminal `resume: true` on resubscribe (pingdotgg#18444):** the backend
should not reopen a closed shell.
- **`rootPath` on git diff/status results:** used for workspace-relative
paths.
- **`repositoryWebUrl` on PR links (pingdotgg#21575).**
- **Relay, T3 Connect, hosted sign-in and `t3 update` work:** not
applicable to the single-origin Moatless deployment.

## Backend behavior to consider reproducing in Moatless
- **Settlement:** an empty inactive thread auto-settles (pingdotgg#15783).
Settling clears snooze (pingdotgg#17547). An interrupted run does not start the
settle timer on a snoozed thread (pingdotgg#17789).
- **Runs:** auto-delivered messages queue between provider turns
(pingdotgg#15691). A message promoted into a stopping run stays queued (pingdotgg#16926).
Compaction cadence survives restarts (pingdotgg#21880).
- **ACP:** native foreground work settles independently, and child
routing moves between runs (pingdotgg#21614–pingdotgg#21616). Delegated children stop once
(pingdotgg#18826).
- **Terminals:** slow clients no longer queue unbounded output (pingdotgg#18429).
Shells do not inherit tmux (pingdotgg#13359). Script commands stay out of shell
history (pingdotgg#13903).
- **Files:** saves no longer follow a symlink out of the workspace
(pingdotgg#18819). This one is security-relevant.
- **Git:** new worktrees fetch the base branch's own tracked remote
(pingdotgg#21032). Rename and non-ASCII paths are kept in changed-file lists
(pingdotgg#20175). Rollback blocked by an index lock now says so (pingdotgg#11340).

The `docs/fork/gaps.md` entries "Capabilities are reported…" and
"Runtime fixes upstream made to its own server" are extended with these.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---
Moatless task:
https://moatless.soaplabstest.com/tasks/d311aafc-7a38-48b7-9c14-30c4cac930a7
Pipyakas pushed a commit to Pipyakas/t3code that referenced this pull request Oct 21, 2026
…tgg#18175)

Co-authored-by: Julius Marminge <51714798+juliusmarminge@users.noreply.github.com>
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
(cherry picked from commit 098eb55)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants