fix: skip session config call when local session is idle-killed#1765
Merged
jonathanlab merged 1 commit intomainfrom Apr 21, 2026
Merged
Conversation
Generated-By: PostHog Code Task-Id: 3a57f0a0-4443-471a-8891-1dd7bbf70c95
Prompt To Fix All With AIThis is a comment left during a code review.
Path: apps/code/src/renderer/features/sessions/service/service.test.ts
Line: 1500-1521
Comment:
**Incomplete assertions and missing `connecting` status coverage**
The `disconnected` test only checks that the backend call is skipped, but unlike the idle-killed test above it doesn't verify that the local session state and persisted config are still updated — which is the other half of the intended behaviour. Additionally, the `connecting` status is handled in the same conditional in `service.ts` but has no test at all.
Consider also parameterising all three cases (`idleKilled`, `disconnected`, `connecting`) into a single test, per the team's preference for parameterised tests. Example:
```ts
it.each([
{ desc: "idle-killed", overrides: { status: "error" as const, idleKilled: true } },
{ desc: "disconnected", overrides: { status: "disconnected" as const } },
{ desc: "connecting", overrides: { status: "connecting" as const } },
])(
"skips backend call when local session is $desc so reconnect restore handles it",
async ({ overrides }) => {
const service = getSessionService();
mockSessionStoreSetters.getSessionByTaskId.mockReturnValue(
createMockSession({
...overrides,
configOptions: [
{ id: "mode", name: "Mode", type: "select", category: "mode", currentValue: "default", options: [] },
],
}),
);
await service.setSessionConfigOption("task-123", "mode", "acceptEdits");
expect(mockTrpcAgent.setConfigOption.mutate).not.toHaveBeenCalled();
expect(mockSessionStoreSetters.updateSession).toHaveBeenCalledTimes(1);
expect(mockSessionStoreSetters.updateSession).toHaveBeenCalledWith(
"run-123",
{ configOptions: [expect.objectContaining({ id: "mode", currentValue: "acceptEdits" })] },
);
expect(mockSessionConfigStore.updatePersistedConfigOptionValue)
.toHaveBeenCalledWith("run-123", "mode", "acceptEdits");
},
);
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: skip session config call when local..." | Re-trigger Greptile |
Comment on lines
+1500
to
+1521
| it("skips backend call when local session is reconnecting (disconnected status)", async () => { | ||
| const service = getSessionService(); | ||
| mockSessionStoreSetters.getSessionByTaskId.mockReturnValue( | ||
| createMockSession({ | ||
| status: "disconnected", | ||
| configOptions: [ | ||
| { | ||
| id: "mode", | ||
| name: "Mode", | ||
| type: "select", | ||
| category: "mode", | ||
| currentValue: "default", | ||
| options: [], | ||
| }, | ||
| ], | ||
| }), | ||
| ); | ||
|
|
||
| await service.setSessionConfigOption("task-123", "mode", "acceptEdits"); | ||
|
|
||
| expect(mockTrpcAgent.setConfigOption.mutate).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
Incomplete assertions and missing
connecting status coverage
The disconnected test only checks that the backend call is skipped, but unlike the idle-killed test above it doesn't verify that the local session state and persisted config are still updated — which is the other half of the intended behaviour. Additionally, the connecting status is handled in the same conditional in service.ts but has no test at all.
Consider also parameterising all three cases (idleKilled, disconnected, connecting) into a single test, per the team's preference for parameterised tests. Example:
it.each([
{ desc: "idle-killed", overrides: { status: "error" as const, idleKilled: true } },
{ desc: "disconnected", overrides: { status: "disconnected" as const } },
{ desc: "connecting", overrides: { status: "connecting" as const } },
])(
"skips backend call when local session is $desc so reconnect restore handles it",
async ({ overrides }) => {
const service = getSessionService();
mockSessionStoreSetters.getSessionByTaskId.mockReturnValue(
createMockSession({
...overrides,
configOptions: [
{ id: "mode", name: "Mode", type: "select", category: "mode", currentValue: "default", options: [] },
],
}),
);
await service.setSessionConfigOption("task-123", "mode", "acceptEdits");
expect(mockTrpcAgent.setConfigOption.mutate).not.toHaveBeenCalled();
expect(mockSessionStoreSetters.updateSession).toHaveBeenCalledTimes(1);
expect(mockSessionStoreSetters.updateSession).toHaveBeenCalledWith(
"run-123",
{ configOptions: [expect.objectContaining({ id: "mode", currentValue: "acceptEdits" })] },
);
expect(mockSessionConfigStore.updatePersistedConfigOptionValue)
.toHaveBeenCalledWith("run-123", "mode", "acceptEdits");
},
);Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/features/sessions/service/service.test.ts
Line: 1500-1521
Comment:
**Incomplete assertions and missing `connecting` status coverage**
The `disconnected` test only checks that the backend call is skipped, but unlike the idle-killed test above it doesn't verify that the local session state and persisted config are still updated — which is the other half of the intended behaviour. Additionally, the `connecting` status is handled in the same conditional in `service.ts` but has no test at all.
Consider also parameterising all three cases (`idleKilled`, `disconnected`, `connecting`) into a single test, per the team's preference for parameterised tests. Example:
```ts
it.each([
{ desc: "idle-killed", overrides: { status: "error" as const, idleKilled: true } },
{ desc: "disconnected", overrides: { status: "disconnected" as const } },
{ desc: "connecting", overrides: { status: "connecting" as const } },
])(
"skips backend call when local session is $desc so reconnect restore handles it",
async ({ overrides }) => {
const service = getSessionService();
mockSessionStoreSetters.getSessionByTaskId.mockReturnValue(
createMockSession({
...overrides,
configOptions: [
{ id: "mode", name: "Mode", type: "select", category: "mode", currentValue: "default", options: [] },
],
}),
);
await service.setSessionConfigOption("task-123", "mode", "acceptEdits");
expect(mockTrpcAgent.setConfigOption.mutate).not.toHaveBeenCalled();
expect(mockSessionStoreSetters.updateSession).toHaveBeenCalledTimes(1);
expect(mockSessionStoreSetters.updateSession).toHaveBeenCalledWith(
"run-123",
{ configOptions: [expect.objectContaining({ id: "mode", currentValue: "acceptEdits" })] },
);
expect(mockSessionConfigStore.updatePersistedConfigOptionValue)
.toHaveBeenCalledWith("run-123", "mode", "acceptEdits");
},
);
```
How can I resolve this? If you propose a fix, please make it concise.
charlesvien
approved these changes
Apr 21, 2026
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
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.
No description provided.