refactor(ui): keep history reviews in one renderer - #992
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Greptile SummaryThis PR replaces child-process terminal handoff with a single retained OpenTUI renderer and React root for history and commit reviews. It also introduces cancellable asynchronous bundled-VCS loading and workspace-owned extension reuse.
Confidence Score: 4/5The PR is not ready to merge because trusting repository extensions from an embedded history review does not actually load them, and the explicit filename requirement must also be satisfied. The borrowed-extension ownership guard suppresses the registry replacement requested by the trust workflow, leaving newly trusted extensions unavailable in the current workspace; the remaining findings concern an explicit naming rule and contradictory lifecycle documentation. Files Needing Attention: src/ui/AppHost.tsx, src/extensions/default/vcs/asyncProcess.ts, src/ui/log/LogSessionHost.tsx, src/ui/hooks/useIntermediateRenderAfterMount.ts, website/src/content/docs/docs/workflows/git-history.md Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Log as LogApp
participant Host as LogSessionHost
participant Startup as Startup/VCS pipeline
participant Review as AppHost
participant Broker as Session broker
User->>Log: Open selected commit
Log->>Host: Provider-planned review action
Host->>Startup: Prepare with cancellation signal
Startup-->>Host: Review bootstrap
Host->>Broker: Start fresh review session
Host->>Review: Mount into retained renderer/root
Review-->>User: Commit review frame
User->>Review: Quit / return
Review->>Host: Retire review
Host->>Broker: Stop session
Host-->>User: Restore retained history
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if ( | ||
| extensionOwnership === "owned" && | ||
| (options?.reloadExtensions || cwd !== extensionsCwdRef.current) |
There was a problem hiding this comment.
When a user trusts repository extensions from an embedded history review, the trust flow requests an extension reload. This ownership check skips that reload for borrowed reviews, so the review reloads with the same registry that omitted those extensions. The newly trusted extensions therefore remain unavailable even though the normal trust flow promises to enable them immediately. The owning history session should perform the registry replacement, or the UI should clearly require a workspace restart.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/AppHost.tsx
Line: 293-295
Comment:
**Trust Reload Is Skipped**
When a user trusts repository extensions from an embedded history review, the trust flow requests an extension reload. This ownership check skips that reload for borrowed reviews, so the review reloads with the same registry that omitted those extensions. The newly trusted extensions therefore remain unavailable even though the normal trust flow promises to enable them immediately. The owning history session should perform the registry replacement, or the UI should clearly require a workspace restart.
---
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.
Fixed in c66b0ab. Borrowed embedded reviews now tell the trust controller that they cannot replace the session-owned extension registry, so granting trust records the decision and clearly reports that Hunk must be restarted to load those extensions instead of pretending the current review reloaded them.
Responded by Pi using gpt-5.6-sol.
This comment was generated by Pi using gpt-5.6-sol
| @@ -0,0 +1,98 @@ | |||
| const DEFAULT_TERMINATION_GRACE_MS = 250; | |||
There was a problem hiding this comment.
The repository requires .ts and .tsx filenames to use dash-case, but this PR adds asyncProcess.ts, LogSessionHost.tsx, and useIntermediateRenderAfterMount.ts. Rename these files, their colocated tests, and their imports to dash-case 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/extensions/default/vcs/asyncProcess.ts
Line: 1
Comment:
**New Filenames Violate Dash-Case**
The repository requires `.ts` and `.tsx` filenames to use dash-case, but this PR adds `asyncProcess.ts`, `LogSessionHost.tsx`, and `useIntermediateRenderAfterMount.ts`. Rename these files, their colocated tests, and their imports to dash-case 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. Hunk does not require dash-case TypeScript filenames; the repository consistently uses camelCase for helpers/hooks and PascalCase for React components. The proposed renames would conflict with established local conventions.
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 dash-case finding was incorrect: this repository's local convention uses camelCase for helpers/hooks and PascalCase for React components (for example, currentReviewRefresh.ts and AppHost.tsx). No rename is needed; I'll withdraw this comment.
a9f42e8 to
c66b0ab
Compare
c66b0ab to
06cc4a1
Compare
06cc4a1 to
8620900
Compare
1 participant
Stack
Problem
The process-level readiness handoff in #989 shortened review startup but still crossed two alternate-screen renderers. Some terminals briefly displayed unreadable stale content between history and review. Renderer/root transitions also exposed unsafe initial redraw behavior when mounting the review dynamically.
Approach
CliRendererand one React/OpenTUI root alive for the completehunk logsessionintermediateRender()calls while preserving redraws for later geometry changesAnimation is intentionally excluded. This PR stabilizes the single-renderer snap transition first.
Validation
git diff --checkpassedThe PTY scenario asserts that opening, returning, and repeated review cycles do not emit an alternate-screen exit between surfaces.
Residual risk
Bundled provider implementations still contain some synchronous repository probes inherited from ordinary startup. The loading frame is committed before those probes begin, and VCS loading receives cancellation where the adapter supports it. Fully preempting a non-cooperative synchronous third-party extension requires broader asynchronous provider-runtime work outside this transition refactor.
This PR description was generated by Pi using GPT-5.6-sol