Skip to content

chat: fix last response state stored as active in the session index#288161

Merged
connor4312 merged 1 commit intomainfrom
connor4312/index-repsonse-state
Jan 15, 2026
Merged

chat: fix last response state stored as active in the session index#288161
connor4312 merged 1 commit intomainfrom
connor4312/index-repsonse-state

Conversation

@connor4312
Copy link
Copy Markdown
Member

No description provided.

Copilot AI review requested due to automatic review settings January 15, 2026 19:41
@connor4312 connor4312 enabled auto-merge (squash) January 15, 2026 19:41
@connor4312 connor4312 self-assigned this Jan 15, 2026
@vs-code-engineering vs-code-engineering bot added this to the January 2026 milestone Jan 15, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes an issue where chat sessions with active (Pending or NeedsInput) response states were being stored incorrectly in the session index. The fix ensures that when session metadata is persisted, any transient active states (Pending/NeedsInput) are converted to Cancelled state, preventing inconsistent state in persisted data.

Changes:

  • Modified getSessionMetadata function to normalize transient response states before storing session metadata

Comment on lines +677 to +679
let lastResponseState = session instanceof ChatModel ?
(session.lastRequest?.response?.state ?? ResponseModelState.Complete) :
ResponseModelState.Complete;
Copy link

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The conditional expression spans multiple lines making it harder to read. Consider extracting this into a helper function or restructuring for clarity. The same pattern also appears in chatServiceImpl.ts line 361, suggesting an opportunity for shared logic.

Copilot uses AI. Check for mistakes.
Comment on lines +681 to +683
if (lastResponseState === ResponseModelState.Pending || lastResponseState === ResponseModelState.NeedsInput) {
lastResponseState = ResponseModelState.Cancelled;
}
Copy link

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The state normalization logic (converting Pending/NeedsInput to Cancelled) lacks test coverage. Consider adding tests in chatSessionStore.test.ts that verify sessions with Pending or NeedsInput states are stored with Cancelled state in the session index.

Copilot uses AI. Check for mistakes.
@connor4312 connor4312 merged commit 8e70e1e into main Jan 15, 2026
27 of 28 checks passed
@connor4312 connor4312 deleted the connor4312/index-repsonse-state branch January 15, 2026 20:09
connor4312 added a commit that referenced this pull request Jan 28, 2026
Guard old sessions from Insiders that may have Pending or NeedsInput states,
converting them to Complete state for consistency. This ensures proper handling
of sessions migrated from pre-PR #288161 versions.

- Added check to convert Pending/NeedsInput states to Complete state
- Preserves backward compatibility with old session data
- Prevents state display issues in migrated sessions

(Commit message generated by Copilot)
connor4312 added a commit that referenced this pull request Jan 28, 2026
Guard old sessions from Insiders that may have Pending or NeedsInput states,
converting them to Complete state for consistency. This ensures proper handling
of sessions migrated from pre-PR #288161 versions.

- Added check to convert Pending/NeedsInput states to Complete state
- Preserves backward compatibility with old session data
- Prevents state display issues in migrated sessions

(Commit message generated by Copilot)
@vs-code-engineering vs-code-engineering bot locked and limited conversation to collaborators Mar 1, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants