fix(ui): avoid redundant reload after final chat event#70391
fix(ui): avoid redundant reload after final chat event#70391cholaolu-boop wants to merge 2 commits into
Conversation
Greptile SummaryThis PR avoids a redundant Confidence Score: 5/5Safe to merge; the one noted edge case (absent role implicitly treated as assistant) is a pre-existing contract assumption and is acknowledged as a follow-up. Logic changes are correct for all tested paths; tests cover the key scenarios; the only finding is a P2 edge case around role-absent messages that doesn't affect well-formed payloads from the current server. ui/src/ui/chat-event-reload.ts — the hasDisplayableAssistantFinalMessage role guard Prompt To Fix All With AIThis is a comment left during a code review.
Path: ui/src/ui/chat-event-reload.ts
Line: 9-11
Comment:
**Absent `role` is silently treated as "assistant"**
When `message` has no `role` field (or `role` is not a string), the guard on line 9 is skipped and the function falls through to the content check, returning `true` — meaning the reload is suppressed as if the message were a confirmed assistant message. If the server ever emits a final event with a message that has content but no `role`, the deferred session reload will be incorrectly dropped.
Consider returning `false` explicitly when `role` is absent:
```typescript
const role = entry.role;
if (typeof role !== "string" || role.toLowerCase() !== "assistant") {
return false;
}
```
```suggestion
if (typeof role !== "string" || role.toLowerCase() !== "assistant") {
return false;
}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(ui): avoid redundant reload after fi..." | Re-trigger Greptile |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a840eb7a48
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
a840eb7 to
4bd0589
Compare
|
ProjectClownfish pushed a narrow repair to this branch so the original contributor path can stay canonical. Source PR: #70391 |
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Keep open: current main still has the deferred reload gap this PR targets, and the branch contains a focused repair. It is not merge-ready because the PR is dirty against current main, has only test/CI proof instead of real Control UI behavior proof, and still edits release-owned changelog content. Canonical path: Close this stale PR. The latest review rated it F, the branch still lacks merge-ready proof, and there has been no human follow-up after the durable review. So I’m closing this here because the remaining work is already tracked in the canonical issue. Review detailsBest possible solution: Close this stale PR. The latest review rated it F, the branch still lacks merge-ready proof, and there has been no human follow-up after the durable review. Do we have a high-confidence way to reproduce the issue? No live reproduction was run in this read-only review. Source inspection and the PR's targeted tests give a clear unit-level path: a deferred same-session Is this the best way to solve the issue? Yes, the runtime change is the narrowest maintainable fix because it preserves direct final-event reload suppression while replaying only an already-deferred same-session reload. The PR still needs branch refresh, real behavior proof, and removal of the release-owned changelog edit. Security review: Security review cleared: The diff touches Control UI reload logic, focused tests, and changelog text; I found no concrete security or supply-chain regression. AGENTS.md: found and applied where relevant. What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against fa614d0907e8. |
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
|
This pull request has been automatically marked as stale due to inactivity. |
Summary
Testing