fix: persist chat editing timeline telemetryInfo so sessionResource survives reload (fixes #326994)#326995
Conversation
…urvives reload (fixes #326994) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot could not run the full agentic suite for this review because it was automatically requested on a bot-authored pull request. Request a review from Copilot under Reviewers to retry with the full agentic suite. Improved support for bot-authored pull requests is coming soon.
Adds explicit JSON serialization for modified-entry telemetry info so values provided by prototype getters are preserved when persisted in the checkpoint timeline.
Changes:
- Implemented
toJSON()to snapshot getter-backed fields into a plain object forJSON.stringify. - Ensures
sessionResource(and related fields) survive serialization and later revival.
| toJSON(): IModifiedEntryTelemetryInfo { | ||
| return { | ||
| agentId: this.agentId, | ||
| modelId: this.modelId, | ||
| modeId: this.modeId, | ||
| command: this.command, | ||
| sessionResource: this.sessionResource, | ||
| requestId: this.requestId, | ||
| result: undefined, | ||
| applyCodeBlockSuggestionId: this.applyCodeBlockSuggestionId, | ||
| feature: this.feature, | ||
| }; | ||
| } |
There was a problem hiding this comment.
result: undefined here is intentional and mirrors the existing checkpoint-timeline serialization: chatEditingSessionStorage.ts (the snapshot-entry telemetryInfo DTO) also sets result: undefined. The IChatAgentResult is deliberately not persisted — it isn't needed for the telemetry path this PR fixes (the crash is caused by the missing sessionResource, not result), and serializing it would bloat the stored timeline. JSON.stringify dropping the key is the expected/consistent behavior, and consumers already treat result as undefined after revival. Serializing the live value would go beyond the scope of #326994 (restoring sessionResource), so I'm leaving this as-is to stay consistent with the established pattern.
Generated by errors-fix-driver · 678.2 AIC · ⌖ 25.5 AIC · ⊞ 72.9K
| // The fields above are getters on the prototype, so they are NOT own enumerable | ||
| // properties and would be dropped by `JSON.stringify` when this object is persisted | ||
| // as part of the checkpoint timeline. Snapshot the current values into a plain object | ||
| // so `sessionResource` (and the other fields) survive serialization and revival. | ||
| toJSON(): IModifiedEntryTelemetryInfo { | ||
| return { | ||
| agentId: this.agentId, | ||
| modelId: this.modelId, | ||
| modeId: this.modeId, | ||
| command: this.command, | ||
| sessionResource: this.sessionResource, | ||
| requestId: this.requestId, | ||
| result: undefined, | ||
| applyCodeBlockSuggestionId: this.applyCodeBlockSuggestionId, | ||
| feature: this.feature, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Thanks for the suggestion. This PR's scope is bounded by #326994: restore the sessionResource field that is lost when the checkpoint timeline's telemetryInfo is serialized. A maintainability refactor to centralize the serialized-key list / add an exhaustiveness check is a reasonable improvement but falls outside that scope, so I'm leaving it for a separate follow-up. Note the explicit field mapping also intentionally mirrors the existing per-field DTO in chatEditingSessionStorage.ts. Feel free to open a tracking issue if you'd like the shared-key-list idea addressed.
Generated by errors-fix-driver · 678.2 AIC · ⌖ 25.5 AIC · ⊞ 72.9K
| toJSON(): IModifiedEntryTelemetryInfo { | ||
| return { | ||
| agentId: this.agentId, | ||
| modelId: this.modelId, | ||
| modeId: this.modeId, | ||
| command: this.command, | ||
| sessionResource: this.sessionResource, | ||
| requestId: this.requestId, | ||
| result: undefined, | ||
| applyCodeBlockSuggestionId: this.applyCodeBlockSuggestionId, | ||
| feature: this.feature, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Agreed that result: undefined is omitted by JSON.stringify — that's intentional. It matches the existing snapshot-entry serialization in chatEditingSessionStorage.ts, which also emits result: undefined, and the revive path/consumers already treat result as optional/undefined. Persisting the real IChatAgentResult isn't required to fix #326994 (which is about sessionResource being lost) and would enlarge the stored timeline, so I'm keeping the behavior consistent with the surrounding code rather than changing the persisted shape in this PR.
Generated by errors-fix-driver · 678.2 AIC · ⌖ 25.5 AIC · ⊞ 72.9K
Summary
An uncaught
TypeError: Cannot read properties of undefined (reading 'toString')fires in the chat-editing telemetry path (2500 hits / 965 users on v1.129.1). The crash happens inChatModelStore.getwhen it callsuri.toString()on anundefinedURI. Theundefinedoriginates from aIModifiedEntryTelemetryInfo.sessionResourcefield (declared non-nullableURI) that becomesundefinedafter a chat editing session is persisted and restored.The producer is
ChatEditingSession._getTelemetryInfoForModel, which returns an object whose fields are implemented as prototype getters. When the checkpoint timeline is persisted withJSON.stringify, prototype getters are not own enumerable properties and are silently dropped, so each persistedtelemetryInfobecomes{}. After reload, entries reconstructed from the timeline carrysessionResource: undefined, which later crashes the telemetry pipeline.Fixes #326994
Recommended reviewer:
@connor4312Culprit Commit
Not pinpointed to a single SHA (git history unavailable in this environment). The regression window (first seen 2026-07-15, v1.129.0-insider) corresponds to the introduction of the getter-based
_getTelemetryInfoForModelobject (comment: "Make these getters because the response result is not available when the file first starts to be edited") combined with the checkpoint-timeline persistence path that serializestelemetryInfoverbatim. The timeline persistence/replay code is owned by@connor4312(checkpoint timeline, epoch replay, edit restore).Code Flow
flowchart TD A["_getTelemetryInfoForModel<br/>returns object with prototype getters<br/>chatEditingSession.ts:1035"] --> B["FileOperation / IFileBaseline<br/>hold live telemetryInfo"] B --> C["timeline.getStateForPersistence()<br/>chatEditingCheckpointTimelineImpl.ts:444"] C --> D["storeState: JSON.stringify(data)<br/>getters dropped -> telemetryInfo = {}<br/>chatEditingSessionStorage.ts:191,197"] D --> E["restoreState: revive(data.timeline)<br/>telemetryInfo still {} -> sessionResource undefined<br/>chatEditingSessionStorage.ts:114"] E --> F["_reconstructFileState -> setContents<br/>entry gets _telemetryInfo with undefined sessionResource<br/>chatEditingCheckpointTimelineImpl.ts:475"] F --> G["_notifySessionAction reads<br/>_telemetryInfo.sessionResource (undefined)<br/>chatEditingModifiedFileEntry.ts:272,316"] G --> H["notifyUserAction:<br/>_sessionModels.get(action.sessionResource)<br/>chatServiceImpl.ts:377"] H --> I["ChatModelStore.get(undefined)<br/>toKey: uri.toString() -> TypeError<br/>chatModelStore.ts:99,246"]Affected Files
src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingSession.ts—_getTelemetryInfoForModel(the producer). Modified.src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingCheckpointTimelineImpl.ts— persists/replaystelemetryInfo(context, unchanged).src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingSessionStorage.ts—JSON.stringify/reviveof the timeline (context, unchanged).src/vs/workbench/contrib/chat/common/model/chatModelStore.ts— crash siteget/toKey(context, unchanged; intentionally not patched).Repro Steps
telemetryInfois the getter-based object).storeState) and reload the window so the session is restored from disk (restoreState→revive(data.timeline)).RunOnceSchedulerinchatEditingModifiedFileEntryfires_notifySessionAction('userModified')._telemetryInfo.sessionResourceisundefined(lost during serialization) →ChatModelStore.get(undefined)→TypeError: Cannot read properties of undefined (reading 'toString').How the Fix Works
Chosen approach (
chatEditingSession.ts): add atoJSON()method to the anonymousIModifiedEntryTelemetryInfoclass returned by_getTelemetryInfoForModel.toJSON()snapshots each getter value into a plain object with own enumerable properties (agentId,modelId,modeId,command,sessionResource,requestId,applyCodeBlockSuggestionId,feature;resultis set toundefined, matching the snapshot-entry serialization which also dropsresult).JSON.stringifyhonorstoJSON, so the fields — includingsessionResourceas a serialized URI — now survive persistence and are correctly revived byrevive(data.timeline).This is a fix at the data producer, not at the crash site: the object that enters the persistence pipeline is made serializable, rather than adding a null-guard where the
undefinedfinally surfaces. Live consumers are unaffected because they read the fields via normal property access, which still hits the getters; onlyJSON.stringifyusestoJSON. It also restores the other telemetry fields (requestId,agentId, etc.) that were silently lost from persisted timelines by the same getter-serialization gap.After this change,
_getTelemetryInfoForModel(chatEditingSession.ts:1035) cannot produce atelemetryInfowhose persisted form lackssessionResource, becausetoJSONemitssessionResourceas an own enumerable property thatJSON.stringifyserializes andreviverestores.Alternatives considered:
telemetryInfoinside the timeline persistence (a dedicated DTO like the snapshot-entry path): rejected as more invasive (new DTO + revive plumbing in the timeline) while addressing the same root cause less directly.result) reflect the current response state during live use.sessionResource/toKeyat the crash site or innotifyUserAction: rejected because it hides the bug at the consumer instead of fixing the data producer, and the telemetry pipeline depends on real values.Recommended Owner
@connor4312— owns the chat-editing checkpoint timeline persistence and edit-restore code path (authored the timeline epoch/replay/restore logic inchatEditingCheckpointTimelineImpl.ts), which is where the getter-basedtelemetryInfois serialized and lost.