Skip to content

feat(phrocs): make r restart crashed procs too#59209

Merged
gantoine merged 3 commits into
masterfrom
posthog-code/phrocs-r-restarts-crashed
May 20, 2026
Merged

feat(phrocs): make r restart crashed procs too#59209
gantoine merged 3 commits into
masterfrom
posthog-code/phrocs-r-restarts-crashed

Conversation

@oliverb123
Copy link
Copy Markdown
Contributor

Problem

In phrocs the r hotkey only worked on running procs — if a proc had crashed, r did nothing and you had to reach for s (start) instead. Two different keys for the same user intent ("rerun this thing") is friction, especially because the proc-detail view doesn't make it obvious which key applies to which state.

Changes

  • r now also starts a proc in StatusCrashed, alongside its existing behavior of restarting a running proc. Clean exits (StatusDone), manual stops (StatusStopped), and never-started procs are still left alone — those are the user's chosen end-states and r shouldn't silently undo them.
  • Updated the Restart binding gating so the hotkey is only enabled when there's actually something to do (running || crashed), keeping help-text discoverability honest.

How did you test this code?

I'm an agent. I haven't manually exercised the TUI; Go isn't available in this sandbox. The change is covered by automated tests in tools/phrocs/internal/tui/restart_failed_test.go:

  • TestUpdateProcKeys_Restart{Disabled,Enabled}* — binding-gating across fresh, running, crashed, clean-exit, and manually-stopped states.
  • TestHandleNormalKey_RestartKeyRevivesCrashedProc — end-to-end check that pressing r on a crashed proc actually kicks off a new Start, not just that the binding is enabled.

Reviewer should run go test ./tools/phrocs/internal/tui/... to confirm.

Publish to changelog?

no

Docs update

No docs changes — the in-app help still reads r: restart, which fits both cases.

🤖 Agent context

Authored by PostHog Code (Claude Opus 4.7). Approach:

  • Identified the two touch points: the binding-gating in updateProcKeys and the keypress dispatch in handleNormalKey.
  • Considered routing crashed procs through Restart (which calls Stop then Start) for code symmetry — rejected in favor of a direct Start call since there's no live process to stop and the dbg log is clearer about what's happening.
  • Mirrored the existing RestartAllFailed test structure (runUntilStatus, modelWithProcs) for the new tests rather than introducing a new harness.

Created with PostHog Code

Pressing `r` on a crashed proc now starts it back up, in addition to
restarting a running one — same user intent of "rerun this thing", just
with no live process to stop first. The binding gates on
running || StatusCrashed so clean exits and manual stops are left alone.

Generated-By: PostHog Code
Task-Id: cb29ca97-345f-4b3d-b485-2ca9ef89b576
@oliverb123 oliverb123 marked this pull request as ready for review May 20, 2026 14:05
Copilot AI review requested due to automatic review settings May 20, 2026 14:05
@oliverb123 oliverb123 requested a review from a team May 20, 2026 14:05
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 20, 2026

Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
tools/phrocs/internal/tui/restart_failed_test.go:313-338
**Missing `t.Cleanup` leaves subprocess leaked on timeout**

`p.Start` succeeds and a `sleep 30` process is forked, but there's no `t.Cleanup(func() { p.Stop() })` registered before the spin-loop. If `t.Fatal("sleeper never started")` fires, the subprocess is never reaped. Compare with `TestUpdateProcKeys_RestartEnabledWhileRunning` which correctly adds `t.Cleanup` immediately after the successful `Start` call.

### Issue 2 of 2
tools/phrocs/internal/tui/restart_failed_test.go:224-338
**Binding-gating tests should be a single table-driven test**

The five `TestUpdateProcKeys_Restart*` functions all follow the same pattern — set up proc state, call `m.updateProcKeys()`, assert `Restart.Enabled()`. Per the team's simplicity rules ("OnceAndOnlyOnce") and the explicit preference for parameterised tests, these should be collapsed into one table-driven test with a per-case setup func. At minimum `TestUpdateProcKeys_RestartEnabledAfterCrash` and `TestUpdateProcKeys_RestartDisabledAfterCleanExit` are identical modulo the shell command and expected boolean, and can be merged immediately.

Reviews (1): Last reviewed commit: "feat(phrocs): make `r` restart crashed p..." | Re-trigger Greptile

Comment thread tools/phrocs/internal/tui/restart_failed_test.go Outdated
Comment thread tools/phrocs/internal/tui/restart_failed_test.go Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates phrocs’ TUI so the r hotkey (“restart”) also revives crashed processes (in addition to restarting currently-running ones), aligning the keybinding with the user intent of “rerun this proc”.

Changes:

  • Enable the Restart keybinding when the active proc is running || crashed.
  • Update the r key handler to call Start when the active proc is crashed (instead of doing nothing).
  • Add tests covering Restart binding gating across proc states and an end-to-end “press r to revive crashed proc” scenario.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
tools/phrocs/internal/tui/keys.go Enables Restart for crashed procs and starts a crashed proc on r keypress.
tools/phrocs/internal/tui/restart_failed_test.go Adds tests for restart key gating and verifies r restarts a crashed proc.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tools/phrocs/internal/tui/restart_failed_test.go Outdated
Comment thread tools/phrocs/internal/tui/keys.go Outdated
- Snapshot p.Status() once in updateProcKeys so running/crashed are
  derived from the same observation, avoiding a redundant trip through
  the proc mutex (Copilot).
- Collapse the five TestUpdateProcKeys_Restart* gating tests into one
  table-driven test (Greptile).
- Extract a waitRunning helper that registers t.Cleanup before the
  spin loop, so a t.Fatal inside the loop can no longer leak a
  sleep-30 subprocess (Greptile / Copilot).

Generated-By: PostHog Code
Task-Id: cb29ca97-345f-4b3d-b485-2ca9ef89b576
@gantoine gantoine requested review from gantoine and removed request for a team May 20, 2026 14:27
@gantoine gantoine merged commit 0ea5d4c into master May 20, 2026
147 checks passed
@gantoine gantoine deleted the posthog-code/phrocs-r-restarts-crashed branch May 20, 2026 15:14
@deployment-status-posthog
Copy link
Copy Markdown

deployment-status-posthog Bot commented May 20, 2026

Deploy status

Environment Status Deployed At Workflow
dev ✅ Deployed 2026-05-20 15:46 UTC Run
prod-us ✅ Deployed 2026-05-20 16:06 UTC Run
prod-eu ✅ Deployed 2026-05-20 16:12 UTC Run

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.

3 participants