refactor(interaction): resolve the CapturedSnapshot name collision - #1371
Merged
Conversation
resolution.ts and selector-read-shared.ts (both in src/commands/interaction/runtime/) each exported an unrelated CapturedSnapshot type with a different shape. resolution.ts's export is never imported anywhere else, so rename it to InteractionSnapshot rather than force a merge with the richer, session-carrying shape that selector-read.ts/settle.ts/stable-capture.ts actually depend on.
Size Report
Startup median (7 runs, lower is better):
Top changed chunks: no changes in the largest emitted chunks. |
Member
Author
|
Review clean at |
|
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.
The collision
src/commands/interaction/runtime/resolution.tsand its siblingselector-read-shared.tseach exported an unrelated type namedCapturedSnapshot:resolution.ts:{ snapshot: SnapshotState }selector-read-shared.ts:{ sessionName: string; session?: CommandSessionRecord; snapshot: SnapshotState }Same name, different shapes, same directory.
What changed
Renamed
resolution.ts's narrow type toInteractionSnapshot, keptselector-read-shared.ts'sCapturedSnapshotas-is. Did not unify into one type.Why rename instead of unify:
resolution.ts'sCapturedSnapshot/InteractionSnapshotis purely internal — nothing outsideresolution.tsimports it (verified via grep acrosssrc/, includingsrc/index.ts,src/sdk/, andpackage.jsonexports — neither type reaches the public surface). Its three producers (captureInteractionSnapshot,resolveSnapshotForRef,reconcileFreshObservation) never populate or consumesessionName/session— they only ever read.snapshot.resolution.tsalready separately consumes the otherCapturedSnapshot(viarequireSnapshotSession, intersected with{ session: CommandSessionRecord }) exactly where session identity is actually needed for ref freshness reconciliation. Force-merging the two would mean threading unusedsessionName/sessionfields through resolution's internal plumbing purely to satisfy a shared name — manufacturing data no consumer reads, not "truthfully supplying" it in any meaningful sense. Kept them distinct instead.Optionality decision (
session?:on selector-read-shared'sCapturedSnapshot)Left as optional. Evidence:
requireSnapshotSessionguaranteessession(throwsSESSION_NOT_FOUNDfirst, already reflected via its& { session: CommandSessionRecord }return-type intersection). But the other producer,captureSelectorSnapshot, does not guarantee it —const session = await runtime.sessions.get(sessionName);has no presence check, and the function deliberately tolerates an absent session (captureOptions.updateSession && session && ...at line 84 skips the session-store write rather than failing). This is a live, intentional code path, not a theoretical gap, so the field must stay optional.Shared guard opportunity (skipped)
captureInteractionSnapshotinresolution.tsre-inlines aruntime.sessions.get+SESSION_NOT_FOUNDguard that looks likerequireSnapshotSession. Not reused:requireSnapshotSessionadditionally requires an existingrefFrameSnapshot ?? snapshotand throwsINVALID_ARGSwhen absent.captureInteractionSnapshotmust work for a session that has no snapshot yet (it captures one fresh) — swapping in the shared helper would introduce a new failure mode and change behavior. Skipped to keep this behavior-identical.Verification
pnpm typecheck— passpnpm lint— passpnpm check:fallow— pass, no issues in the 1 changed filepnpm vitest run src/commands --project unit-core— 40 files / 339 tests, all passFiles changed
src/commands/interaction/runtime/resolution.ts(rename only; 4 lines)