fix(ui): prepare history reviews before terminal handoff - #989
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR introduces a versioned parent/child IPC handshake so the history renderer remains visible while a selected review performs startup work, then transfers terminal ownership only after the child reports readiness.
Confidence Score: 4/5The PR should not merge until the bootstrapping child is prevented from reading or writing the parent-owned terminal and the explicit repository requirements are satisfied. The readiness protocol orders renderer destruction and terminal release correctly, but inherited stdin and stdout allow third-party startup code to corrupt the retained history surface or consume its input; the new files also contain confirmed repository-rule violations. Files Needing Attention: src/ui/log/reviewLaunch.ts, src/core/process/terminalHandoff.ts Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User
participant H as History parent
participant R as History renderer
participant C as Review child
U->>H: Open selected commit
H->>R: Show "Preparing review…"
H->>C: Spawn with IPC handoff marker
C->>C: Resolve config, extensions, changeset, theme
C-->>H: ready
H->>R: Unmount and destroy
H-->>C: release
C->>C: Mount review renderer
C-->>H: Exit status
H->>R: Restore retained history
Prompt To Fix All With AI### Issue 1
src/ui/log/reviewLaunch.ts:80
**Bootstrap inherits terminal streams**
When a third-party extension factory runs during pre-readiness startup, the child already inherits the terminal's stdin and stdout even though the history renderer still owns them. An extension that directly reads or writes these streams can consume the parent's keystrokes or overwrite the loading surface before terminal ownership is released. Isolate both streams during bootstrap or otherwise prevent child access until the release message.
### Issue 2
src/core/process/terminalHandoff.ts:1
**Filename violates dash-case rule**
The new `terminalHandoff.ts` file uses camelCase, but the repository requires dash-case names for `.ts` and `.tsx` files. Rename it and its imports to `terminal-handoff.ts`; this repository requirement must be satisfied before merging.
### Issue 3
src/core/process/terminalHandoff.ts:13-20
**Environment access bypasses Varlock**
The new handoff helpers default directly to `process.env`, and the same direct access appears in the helpers at lines 70 and 100 and in `reviewLaunch.ts` at line 89. This violates the repository directive requiring type-safe, validated environment access through Varlock. Route these values through the prescribed environment boundary 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: "fix(ui): prepare history reviews before ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| env: { ...process.env, HUNK_RETURN_TO_HISTORY: "1" }, | ||
| stdio: "inherit", | ||
| env: terminalHandoffEnv({ ...env, HUNK_RETURN_TO_HISTORY: "1" }, themeMode), | ||
| stdio: ["inherit", "inherit", "pipe", "ipc"], |
There was a problem hiding this comment.
When a third-party extension factory runs during pre-readiness startup, the child already inherits the terminal's stdin and stdout even though the history renderer still owns them. An extension that directly reads or writes these streams can consume the parent's keystrokes or overwrite the loading surface before terminal ownership is released. Isolate both streams during bootstrap or otherwise prevent child access until the release message.
Knowledge Base Used:
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/log/reviewLaunch.ts
Line: 80
Comment:
**Bootstrap inherits terminal streams**
When a third-party extension factory runs during pre-readiness startup, the child already inherits the terminal's stdin and stdout even though the history renderer still owns them. An extension that directly reads or writes these streams can consume the parent's keystrokes or overwrite the loading surface before terminal ownership is released. Isolate both streams during bootstrap or otherwise prevent child access until the release message.
**Knowledge Base Used:**
- [Application runtime and startup](https://app.greptile.com/modem/-/custom-context/knowledge-base/modem-dev/hunk/-/docs/application-runtime.md)
- [CLI startup and bootstrap flow](https://app.greptile.com/modem/-/custom-context/knowledge-base/modem-dev/hunk/-/docs/cli-startup-and-bootstrap.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Resolved by the top stack layer in #992: history and review now run in one process, one renderer, and one React root, so there is no child bootstrap inheriting terminal streams while another renderer owns them. The cross-process handoff introduced by this PR is removed before the stack reaches its final state.
Responded by Pi using gpt-5.6-sol.
This comment was generated by Pi using gpt-5.6-sol
| @@ -0,0 +1,116 @@ | |||
| const HANDOFF_ENV = "HUNK_TERMINAL_HANDOFF"; | |||
There was a problem hiding this comment.
The new terminalHandoff.ts file uses camelCase, but the repository requires dash-case names for .ts and .tsx files. Rename it and its imports to terminal-handoff.ts; this repository requirement must be satisfied 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/core/process/terminalHandoff.ts
Line: 1
Comment:
**Filename violates dash-case rule**
The new `terminalHandoff.ts` file uses camelCase, but the repository requires dash-case names for `.ts` and `.tsx` files. Rename it and its imports to `terminal-handoff.ts`; this repository requirement must be satisfied 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.
No change. This repository does not have a dash-case filename rule; the active project guidance and surrounding source use camelCase/PascalCase filenames extensively. In addition, terminalHandoff.ts is deleted by #992 when the cross-process handoff is replaced with the persistent-renderer architecture.
Responded by Pi using gpt-5.6-sol.
This comment was generated by Pi using gpt-5.6-sol
There was a problem hiding this comment.
You're right — I incorrectly inferred a dash-case filename rule. The repository's active guidance and surrounding files support terminalHandoff.ts, and since this file is removed by #992 as part of the persistent-renderer follow-up, there is no change needed for this PR. I'm retracting this comment.
| } | ||
|
|
||
| /** Read the parent's already-detected terminal mode without querying the owned terminal again. */ | ||
| export function terminalHandoffThemeMode( | ||
| env: NodeJS.ProcessEnv = process.env, | ||
| ): "dark" | "light" | undefined { | ||
| if (!hasTerminalHandoff(env)) return undefined; | ||
| const value = env[HANDOFF_THEME_MODE_ENV]; |
There was a problem hiding this comment.
The new handoff helpers default directly to process.env, and the same direct access appears in the helpers at lines 70 and 100 and in reviewLaunch.ts at line 89. This violates the repository directive requiring type-safe, validated environment access through Varlock. Route these values through the prescribed environment boundary 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/core/process/terminalHandoff.ts
Line: 13-20
Comment:
**Environment access bypasses Varlock**
The new handoff helpers default directly to `process.env`, and the same direct access appears in the helpers at lines 70 and 100 and in `reviewLaunch.ts` at line 89. This violates the repository directive requiring type-safe, validated environment access through Varlock. Route these values through the prescribed environment boundary 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.There was a problem hiding this comment.
No change. The cited Varlock directive belongs to a different repository and is not part of Hunk’s project guidance or environment architecture. The entire terminal-handoff module and its private environment protocol are also removed by #992.
Responded by Pi using gpt-5.6-sol.
This comment was generated by Pi using gpt-5.6-sol
There was a problem hiding this comment.
You're right. The Varlock directive isn't applicable to Hunk, and #992 removes terminalHandoff.ts and the private handoff environment protocol entirely. This comment is no longer actionable; no change is needed here.
0221414 to
bba0637
Compare
bba0637 to
1b521c7
Compare
1 participant
Stack
Problem
Opening a commit from
hunk logdestroyed the history renderer before starting and bootstrapping the review child. That restored the shell's previous terminal screen for the entire startup interval. Returning used the same sequential renderer boundary.Approach
Opening commit/Preparing review…transition stateNormal
hunk diff,hunk show, pager, and static startup remain unchanged.Validation
git diff --checkpassedAnimation scope
This PR removes the user-visible bootstrap-length flash and shows a real loading surface. It does not fake a cross-process slide: history and review still use separate OpenTUI renderers, so they cannot safely composite live frames. A genuine slide-in review panel requires a single long-lived renderer and should reuse the animation primitive from #941 after that work lands, alongside an embeddable fresh review-session lifecycle.
This PR description was generated by Pi using GPT-5.6-sol