refactor(ui): route surfaces through one session runtime - #998
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Greptile SummaryThis PR consolidates standalone review and interactive-history surfaces under one OpenTUI renderer/root lifetime.
Confidence Score: 4/5The PR is not yet safe to merge because shutdown can hang indefinitely during provider review planning, and the new filenames must satisfy the repository's explicit dash-case requirement. The shared runtime generally preserves ordered cleanup, but it makes session completion depend on a provider promise that receives no cancellation signal; one explicit repository naming requirement is also violated. Files Needing Attention: src/ui/session/HunkSessionHost.tsx, src/app/historyReview.ts, src/app/session/reviewRuntime.ts, src/ui/session/runHunkSession.tsx Important Files Changed
Sequence DiagramsequenceDiagram
participant Entry as CLI entry
participant Runner as runHunkSession
participant Host as HunkSessionHost
participant History as History provider
participant Review as AppHost
Entry->>Runner: Start interactive session
Runner->>Host: Render initial history/review route
Host->>History: planReview(selected commit)
History-->>Host: Review action
Host->>Review: Mount fresh review runtime
Review-->>Host: Quit or return to history
Host-->>Runner: finish(exitCode)
Runner->>Runner: Cleanup broker, worker, root, renderer
Prompt To Fix All With AI### Issue 1
src/ui/session/HunkSessionHost.tsx:167-172
**Shutdown Waits Indefinitely**
If a history provider's `planReview` promise stalls, every shutdown path waits for this non-abortable call. An OS signal, Ctrl-C, terminal disconnect, or menu quit only aborts the separate preparation signal, while session completion remains deferred until `planReview` returns. As a result, `runHunkSession` can wait indefinitely and never restore the terminal. Shutdown needs to complete independently of provider planning.
### Issue 2
src/app/historyReview.ts:1
**New Filenames Violate Convention**
This new file uses a camel-cased filename, but the repository requires TypeScript filenames to use dash-case. The same violation appears in `src/app/session/reviewRuntime.ts` and `src/ui/session/HunkSessionHost.tsx`, with another occurrence elsewhere in the new session module. These files must be renamed and their imports updated before merging.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "refactor(ui): route surfaces through one..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const action = await historyRoute.runtime.planReview( | ||
| outcome.commit, | ||
| outcome.parentRevisionId === undefined | ||
| ? undefined | ||
| : { parentRevisionId: outcome.parentRevisionId }, | ||
| ); |
There was a problem hiding this comment.
If a history provider's planReview promise stalls, every shutdown path waits for this non-abortable call. An OS signal, Ctrl-C, terminal disconnect, or menu quit only aborts the separate preparation signal, while session completion remains deferred until planReview returns. As a result, runHunkSession can wait indefinitely and never restore the terminal. Shutdown needs to complete independently of provider planning.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/session/HunkSessionHost.tsx
Line: 167-172
Comment:
**Shutdown Waits Indefinitely**
If a history provider's `planReview` promise stalls, every shutdown path waits for this non-abortable call. An OS signal, Ctrl-C, terminal disconnect, or menu quit only aborts the separate preparation signal, while session completion remains deferred until `planReview` returns. As a result, `runHunkSession` can wait indefinitely and never restore the terminal. Shutdown needs to complete independently of provider planning.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
This is intentional for extension-authority safety. planReview() is third-party provider code and cancellation is cooperative; tearing down the renderer, extension registry, or terminal while that code is still executing would allow it to resume against disposed session authority. The host aborts cooperative work immediately, invalidates the preparation generation, and waits for non-cooperative work before completing teardown. This behavior is explicitly covered by the shutdown lifecycle tests. A future bounded termination policy would require an isolation boundary for provider code rather than racing teardown against an in-process promise.
This comment was generated by Pi using gpt-5.6-sol
There was a problem hiding this comment.
That context changes the assessment. Given that planReview() may resume against session authority, waiting for cooperative provider cancellation before renderer/extension teardown is the correct safety invariant; racing shutdown would be unsafe. The generation invalidation and lifecycle-test coverage address the stale-result path, so I’m withdrawing this finding.
| @@ -0,0 +1,81 @@ | |||
| import { resolve } from "node:path"; | |||
There was a problem hiding this comment.
This new file uses a camel-cased filename, but the repository requires TypeScript filenames to use dash-case. The same violation appears in src/app/session/reviewRuntime.ts and src/ui/session/HunkSessionHost.tsx, with another occurrence elsewhere in the new session module. These files must be renamed and their imports updated before merging.
Context Used: guidelines.mdc Cursor rule (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/historyReview.ts
Line: 1
Comment:
**New Filenames Violate Convention**
This new file uses a camel-cased filename, but the repository requires TypeScript filenames to use dash-case. The same violation appears in `src/app/session/reviewRuntime.ts` and `src/ui/session/HunkSessionHost.tsx`, with another occurrence elsewhere in the new session module. These files must be renamed and their imports updated before merging.
**Context Used:** guidelines.mdc Cursor rule ([source](https://github.com/modem-dev/modem/blob/main/.cursor/rules/guidelines.mdc))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
This repository does not require dash-case TypeScript filenames. Its established naming convention includes AppHost.tsx, LogApp.tsx, useAppKeyboardShortcuts.ts, and many other camel-cased component/hook/module names. The cited rule belongs to a different repository (modem-dev/modem), while Hunk’s own AGENTS.md contains no dash-case requirement. These new filenames follow the local convention, so no rename is needed.
This comment was generated by Pi using gpt-5.6-sol
There was a problem hiding this comment.
You're right. I incorrectly applied a dash-case convention from another repository; Hunk's AGENTS.md does not require it, and the existing AppHost.tsx, LogApp.tsx, and useAppKeyboardShortcuts.ts names support the local convention. No rename is needed, and I withdraw this comment.
fd0bc7f to
e8fe8d8
Compare
e8fe8d8 to
4f0492a
Compare
1 participant
Summary
runHunkSessionas the single owner of the OpenTUI renderer, React root, process signals, job control, disconnect handling, and terminal teardownHunkSessionHostthat routes retained history and fresh review surfacesValidation
bun run testbun run test:integrationbun run test:tty-smokebun run typecheckbun run lintbun run deps:checkbun test scripts/source-boundaries.test.tsbun run check:docsgit diff --checkStack
This PR description was generated by Pi using gpt-5.6-sol