fix: preserve streaming bubble when tool calls display is hidden (4.14 regression)#67271
Closed
EronFan wants to merge 3 commits intoopenclaw:mainfrom
Closed
fix: preserve streaming bubble when tool calls display is hidden (4.14 regression)#67271EronFan wants to merge 3 commits intoopenclaw:mainfrom
EronFan wants to merge 3 commits intoopenclaw:mainfrom
Conversation
Active Memory ON + OpenAI Codex (OAuth) causes session timeout spam because the access token and expires fields were included in the session auth epoch hash. When Active Memory triggers token refresh (every ~1hr), the access token rotates, causing the epoch hash to change and invalidating the session. Only hash the refresh token (stable identity) for OAuth credentials. Access token and expires are transient and must not affect session. Fixes openclaw#67084
clearDocumentContent() fetched only the first page of blocks from the Feishu API, leaving stale content in documents with more than ~50 top-level blocks when write action was called. Add a do-while pagination loop so all blocks are collected before batchDelete is called. Fixes openclaw#67252
…4 regression) Fixes openclaw#67250 - Control UI streaming text disappears when tool calls display is hidden. When showToolCalls was false and no text content had arrived yet (isStreaming=true, markdown empty), the render returned nothing and cleared the streaming bubble. Add !opts.isStreaming guard to the empty-bubble suppression condition, so streaming bubbles are never suppressed regardless of tool calls visibility. This mirrors the fix pattern for openclaw#67028 and openclaw#66316.
Contributor
Greptile SummaryThis PR bundles two independent fixes: (1) a do-while pagination loop in Confidence Score: 5/5Safe to merge; both fixes are correct and well-motivated with no P0/P1 issues. All remaining findings are P2: one redundant null-coalescing operator and one missing positive test case for the codex-cli OAuth refresh rotation path. Neither affects runtime correctness. No files require special attention. Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/feishu/src/docx.ts
Line: 493-496
Comment:
**Redundant `?? []` after array method chain**
`items.filter(...).map(...)` always returns an array, so the `?? []` fallback can never fire. Since `items` is initialised as `[]` and only ever populated via `push`, the result is always a plain array. The trailing operator is dead code.
```suggestion
const childIds = items
.filter((b) => b.parent_id === docToken && b.block_type !== 1)
.map((b) => b.block_id);
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/cli-auth-epoch.test.ts
Line: 163-215
Comment:
**Missing positive test for codex-cli local OAuth refresh rotation**
The updated "mixes local codex and auth-profile state" test verifies that rotating `localAccess` does *not* change the epoch — which is the new stable behaviour — and that rotating `profileRefresh` *does* change it. However, `localRefresh` is set but never mutated, so the test never asserts that a local codex-cli OAuth refresh-token change actually invalidates the epoch. The new `encodeCodexCredential` OAuth branch is the only path where this matters, and it goes untested. A parallel test analogous to "changes when OAuth refresh token rotates" would close this gap for codex-cli.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: preserve streaming bubble when tool..." | Re-trigger Greptile |
Comment on lines
493
to
496
| const childIds = | ||
| existing.data?.items | ||
| ?.filter((b) => b.parent_id === docToken && b.block_type !== 1) | ||
| items | ||
| .filter((b) => b.parent_id === docToken && b.block_type !== 1) | ||
| .map((b) => b.block_id) ?? []; |
Contributor
There was a problem hiding this comment.
?? [] after array method chain
items.filter(...).map(...) always returns an array, so the ?? [] fallback can never fire. Since items is initialised as [] and only ever populated via push, the result is always a plain array. The trailing operator is dead code.
Suggested change
| const childIds = | |
| existing.data?.items | |
| ?.filter((b) => b.parent_id === docToken && b.block_type !== 1) | |
| items | |
| .filter((b) => b.parent_id === docToken && b.block_type !== 1) | |
| .map((b) => b.block_id) ?? []; | |
| const childIds = items | |
| .filter((b) => b.parent_id === docToken && b.block_type !== 1) | |
| .map((b) => b.block_id); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/feishu/src/docx.ts
Line: 493-496
Comment:
**Redundant `?? []` after array method chain**
`items.filter(...).map(...)` always returns an array, so the `?? []` fallback can never fire. Since `items` is initialised as `[]` and only ever populated via `push`, the result is always a plain array. The trailing operator is dead code.
```suggestion
const childIds = items
.filter((b) => b.parent_id === docToken && b.block_type !== 1)
.map((b) => b.block_id);
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
163
to
215
| @@ -113,7 +180,7 @@ describe("resolveCliAuthEpoch", () => { | |||
| type: "oauth", | |||
| provider: "openai", | |||
| access: "profile-access", | |||
| refresh, | |||
| refresh: profileRefresh, | |||
| expires: 1, | |||
| }, | |||
| }, | |||
| @@ -124,12 +191,14 @@ describe("resolveCliAuthEpoch", () => { | |||
| provider: "codex-cli", | |||
| authProfileId: "openai:work", | |||
| }); | |||
| access = "local-access-b"; | |||
| // Local OAuth access token rotation must NOT invalidate session | |||
| localAccess = "local-access-b"; | |||
| const second = await resolveCliAuthEpoch({ | |||
| provider: "codex-cli", | |||
| authProfileId: "openai:work", | |||
| }); | |||
| refresh = "profile-refresh-b"; | |||
| // Profile refresh token rotation MUST invalidate session | |||
| profileRefresh = "profile-refresh-b"; | |||
| const third = await resolveCliAuthEpoch({ | |||
| provider: "codex-cli", | |||
| authProfileId: "openai:work", | |||
| @@ -138,7 +207,9 @@ describe("resolveCliAuthEpoch", () => { | |||
| expect(first).toBeDefined(); | |||
| expect(second).toBeDefined(); | |||
| expect(third).toBeDefined(); | |||
| expect(second).not.toBe(first); | |||
| // Local access token rotation must NOT change epoch | |||
| expect(second).toBe(first); | |||
| // Profile refresh token rotation MUST change epoch | |||
| expect(third).not.toBe(second); | |||
| }); | |||
| }); | |||
Contributor
There was a problem hiding this comment.
The updated "mixes local codex and auth-profile state" test verifies that rotating localAccess does not change the epoch — which is the new stable behaviour — and that rotating profileRefresh does change it. However, localRefresh is set but never mutated, so the test never asserts that a local codex-cli OAuth refresh-token change actually invalidates the epoch. The new encodeCodexCredential OAuth branch is the only path where this matters, and it goes untested. A parallel test analogous to "changes when OAuth refresh token rotates" would close this gap for codex-cli.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/cli-auth-epoch.test.ts
Line: 163-215
Comment:
**Missing positive test for codex-cli local OAuth refresh rotation**
The updated "mixes local codex and auth-profile state" test verifies that rotating `localAccess` does *not* change the epoch — which is the new stable behaviour — and that rotating `profileRefresh` *does* change it. However, `localRefresh` is set but never mutated, so the test never asserts that a local codex-cli OAuth refresh-token change actually invalidates the epoch. The new `encodeCodexCredential` OAuth branch is the only path where this matters, and it goes untested. A parallel test analogous to "changes when OAuth refresh token rotates" would close this gap for codex-cli.
How can I resolve this? If you propose a fix, please make it concise.|
Closing this PR because the author has more than 10 active PRs in this repo. Please reduce the active PR queue and reopen or resubmit once it is back under the limit. You can close your own PRs to get back under the limit. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #67250
Problem
Control UI streaming text disappears when tool calls display is hidden (4.14 regression).
When is and no text content has arrived yet during streaming ( empty), the render returns and clears the streaming bubble.
Solution
Add guard to the empty-bubble suppression condition in , so streaming bubbles are never suppressed regardless of tool calls visibility.
Diff
// Never suppress streaming bubbles — streaming text may arrive after tool-only content if ( + !opts.isStreaming && !markdown && !visibleToolCards &&Testing
pnpm test -- --run ui/src/ui/chat/grouped-render.ts— 132 tests passpnpm test -- --run ui/src/ui/controllers/chat.ts— 177 tests passRelated
Same fix pattern as #67028 (WebChat message disappear) and #66316 (history reload race). All three issues share the root cause: streaming state being prematurely cleared.