Skip to content

Commit 77a0232

Browse files
committed
refactor: simplify over-engineered context management architecture
This commit addresses over-engineering feedback by simplifying the context management implementation: 1. Remove assertNever pattern for 3 cases - YAGNI: With exactly 3 context management events, a simple switch without exhaustive checking abstraction works fine - If events are added, UI components would need updates anyway 2. Reduce test coverage from 607 to 30 lines (6:1 → ~1:3 ratio) - Removed defensive tests for unlikely failure modes (arrays, objects) - Kept essential type guard and event validation tests - Deleted ContextManagementRow.spec.tsx (463 lines) - component removed 3. Remove ContextManagementRow abstraction layer - Wrapper just delegated to child components - Inlined logic directly into ChatRow.tsx switch cases - Kept focused child components (CondensationResultRow, etc.) Net reduction: -717 lines
1 parent 4384ed0 commit 77a0232

File tree

7 files changed

+41
-758
lines changed

7 files changed

+41
-758
lines changed
Lines changed: 11 additions & 122 deletions
Original file line numberDiff line numberDiff line change
@@ -1,139 +1,28 @@
11
import { describe, it, expect } from "vitest"
2-
import {
3-
CONTEXT_MANAGEMENT_EVENTS,
4-
isContextManagementEvent,
5-
assertNever,
6-
type ContextManagementEvent,
7-
} from "../context-management.js"
2+
import { CONTEXT_MANAGEMENT_EVENTS, isContextManagementEvent } from "../context-management.js"
83

94
describe("context-management", () => {
105
describe("CONTEXT_MANAGEMENT_EVENTS", () => {
11-
it("should be a readonly array", () => {
12-
expect(Array.isArray(CONTEXT_MANAGEMENT_EVENTS)).toBe(true)
13-
})
14-
15-
it("should contain condense_context", () => {
6+
it("should contain all expected event types", () => {
167
expect(CONTEXT_MANAGEMENT_EVENTS).toContain("condense_context")
17-
})
18-
19-
it("should contain condense_context_error", () => {
208
expect(CONTEXT_MANAGEMENT_EVENTS).toContain("condense_context_error")
21-
})
22-
23-
it("should contain sliding_window_truncation", () => {
249
expect(CONTEXT_MANAGEMENT_EVENTS).toContain("sliding_window_truncation")
25-
})
26-
27-
it("should have exactly 3 events", () => {
2810
expect(CONTEXT_MANAGEMENT_EVENTS).toHaveLength(3)
2911
})
3012
})
3113

3214
describe("isContextManagementEvent", () => {
33-
describe("valid events", () => {
34-
it("should return true for condense_context", () => {
35-
expect(isContextManagementEvent("condense_context")).toBe(true)
36-
})
37-
38-
it("should return true for condense_context_error", () => {
39-
expect(isContextManagementEvent("condense_context_error")).toBe(true)
40-
})
41-
42-
it("should return true for sliding_window_truncation", () => {
43-
expect(isContextManagementEvent("sliding_window_truncation")).toBe(true)
44-
})
45-
})
46-
47-
describe("invalid events", () => {
48-
it("should return false for empty string", () => {
49-
expect(isContextManagementEvent("")).toBe(false)
50-
})
51-
52-
it("should return false for null", () => {
53-
expect(isContextManagementEvent(null)).toBe(false)
54-
})
55-
56-
it("should return false for undefined", () => {
57-
expect(isContextManagementEvent(undefined)).toBe(false)
58-
})
59-
60-
it("should return false for numbers", () => {
61-
expect(isContextManagementEvent(0)).toBe(false)
62-
expect(isContextManagementEvent(123)).toBe(false)
63-
})
64-
65-
it("should return false for objects", () => {
66-
expect(isContextManagementEvent({})).toBe(false)
67-
expect(isContextManagementEvent({ type: "condense_context" })).toBe(false)
68-
})
69-
70-
it("should return false for arrays", () => {
71-
expect(isContextManagementEvent([])).toBe(false)
72-
expect(isContextManagementEvent(["condense_context"])).toBe(false)
73-
})
74-
75-
it("should return false for other event types", () => {
76-
expect(isContextManagementEvent("text")).toBe(false)
77-
expect(isContextManagementEvent("error")).toBe(false)
78-
expect(isContextManagementEvent("api_req_started")).toBe(false)
79-
expect(isContextManagementEvent("user_feedback")).toBe(false)
80-
expect(isContextManagementEvent("completion_result")).toBe(false)
81-
})
82-
83-
it("should return false for similar but incorrect event names", () => {
84-
expect(isContextManagementEvent("condense_context_")).toBe(false)
85-
expect(isContextManagementEvent("CONDENSE_CONTEXT")).toBe(false)
86-
expect(isContextManagementEvent("Condense_Context")).toBe(false)
87-
expect(isContextManagementEvent("condense context")).toBe(false)
88-
expect(isContextManagementEvent("sliding_window")).toBe(false)
89-
expect(isContextManagementEvent("truncation")).toBe(false)
90-
})
91-
})
92-
93-
describe("type narrowing", () => {
94-
it("should narrow type to ContextManagementEvent", () => {
95-
const value: unknown = "condense_context"
96-
if (isContextManagementEvent(value)) {
97-
// TypeScript should now know value is ContextManagementEvent
98-
const event: ContextManagementEvent = value
99-
expect(event).toBe("condense_context")
100-
}
101-
})
15+
it("should return true for valid context management events", () => {
16+
expect(isContextManagementEvent("condense_context")).toBe(true)
17+
expect(isContextManagementEvent("condense_context_error")).toBe(true)
18+
expect(isContextManagementEvent("sliding_window_truncation")).toBe(true)
10219
})
103-
})
104-
105-
describe("assertNever", () => {
106-
it("should throw an error with the value in the message", () => {
107-
const testValue = "unexpected_event" as never
108-
expect(() => assertNever(testValue)).toThrow('Unhandled context management event: "unexpected_event"')
109-
})
110-
111-
it("should throw an error for any value", () => {
112-
expect(() => assertNever("test" as never)).toThrow('Unhandled context management event: "test"')
113-
})
114-
})
115-
116-
describe("ContextManagementEvent type", () => {
117-
it("should work with switch exhaustiveness", () => {
118-
// This test verifies that the type system enforces exhaustiveness
119-
const handleEvent = (event: ContextManagementEvent): string => {
120-
switch (event) {
121-
case "condense_context":
122-
return "condensing"
123-
case "condense_context_error":
124-
return "error"
125-
case "sliding_window_truncation":
126-
return "truncating"
127-
default:
128-
// If we miss a case, TypeScript would error here
129-
// The assertNever ensures runtime exhaustiveness
130-
return assertNever(event)
131-
}
132-
}
13320

134-
expect(handleEvent("condense_context")).toBe("condensing")
135-
expect(handleEvent("condense_context_error")).toBe("error")
136-
expect(handleEvent("sliding_window_truncation")).toBe("truncating")
21+
it("should return false for non-context-management events", () => {
22+
expect(isContextManagementEvent("text")).toBe(false)
23+
expect(isContextManagementEvent("error")).toBe(false)
24+
expect(isContextManagementEvent(null)).toBe(false)
25+
expect(isContextManagementEvent(undefined)).toBe(false)
13726
})
13827
})
13928
})
Lines changed: 2 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
/**
22
* Context Management Types
33
*
4-
* This module provides type definitions and utility functions for context management events.
4+
* This module provides type definitions for context management events.
55
* These events are used to handle different strategies for managing conversation context
66
* when approaching token limits.
77
*
@@ -13,10 +13,7 @@
1313

1414
/**
1515
* Array of all context management event types.
16-
* Used for runtime type checking and exhaustiveness verification.
17-
*
18-
* @constant
19-
* @readonly
16+
* Used for runtime type checking.
2017
*/
2118
export const CONTEXT_MANAGEMENT_EVENTS = [
2219
"condense_context",
@@ -26,56 +23,12 @@ export const CONTEXT_MANAGEMENT_EVENTS = [
2623

2724
/**
2825
* Union type representing all possible context management event types.
29-
* Derived from the CONTEXT_MANAGEMENT_EVENTS array for type safety.
30-
*
31-
* @example
32-
* ```typescript
33-
* const event: ContextManagementEvent = "condense_context"
34-
* ```
3526
*/
3627
export type ContextManagementEvent = (typeof CONTEXT_MANAGEMENT_EVENTS)[number]
3728

3829
/**
3930
* Type guard function to check if a value is a valid context management event.
40-
* Useful for runtime validation and TypeScript type narrowing.
41-
*
42-
* @param value - The value to check
43-
* @returns `true` if the value is a valid ContextManagementEvent, `false` otherwise
44-
*
45-
* @example
46-
* ```typescript
47-
* if (isContextManagementEvent(message.say)) {
48-
* // TypeScript now knows message.say is ContextManagementEvent
49-
* handleContextManagementEvent(message)
50-
* }
51-
* ```
5231
*/
5332
export function isContextManagementEvent(value: unknown): value is ContextManagementEvent {
5433
return typeof value === "string" && (CONTEXT_MANAGEMENT_EVENTS as readonly string[]).includes(value)
5534
}
56-
57-
/**
58-
* Helper function for exhaustive switch statements on context management events.
59-
* Throws an error if an unhandled case is reached, ensuring TypeScript catches
60-
* missing cases at compile time.
61-
*
62-
* @param value - A value that should be of type `never` if all cases are handled
63-
* @throws Error if called at runtime (indicates unhandled case)
64-
*
65-
* @example
66-
* ```typescript
67-
* switch (event) {
68-
* case "condense_context":
69-
* return <CondenseRow />
70-
* case "condense_context_error":
71-
* return <ErrorRow />
72-
* case "sliding_window_truncation":
73-
* return <TruncationRow />
74-
* default:
75-
* assertNever(event) // TypeScript error if any case is missing
76-
* }
77-
* ```
78-
*/
79-
export function assertNever(value: never): never {
80-
throw new Error(`Unhandled context management event: ${JSON.stringify(value)}`)
81-
}

webview-ui/src/components/chat/ChatRow.tsx

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ import { Markdown } from "./Markdown"
3838
import { CommandExecution } from "./CommandExecution"
3939
import { CommandExecutionError } from "./CommandExecutionError"
4040
import { AutoApprovedRequestLimitWarning } from "./AutoApprovedRequestLimitWarning"
41-
import { ContextManagementRow } from "./context-management"
41+
import { InProgressRow, CondensationResultRow, CondensationErrorRow, TruncationResultRow } from "./context-management"
4242
import CodebaseSearchResultsDisplay from "./CodebaseSearchResultsDisplay"
4343
import { appendImages } from "@src/utils/imageUtils"
4444
import { McpExecution } from "./McpExecution"
@@ -1280,10 +1280,27 @@ export const ChatRowContent = ({
12801280
/>
12811281
)
12821282
case "condense_context":
1283+
// In-progress state
1284+
if (message.partial) {
1285+
return <InProgressRow eventType="condense_context" />
1286+
}
1287+
// Completed state
1288+
if (message.contextCondense) {
1289+
return <CondensationResultRow data={message.contextCondense} />
1290+
}
1291+
return null
12831292
case "condense_context_error":
1293+
return <CondensationErrorRow errorText={message.text} />
12841294
case "sliding_window_truncation":
1285-
// All context management events are handled by the unified ContextManagementRow
1286-
return <ContextManagementRow message={message} />
1295+
// In-progress state
1296+
if (message.partial) {
1297+
return <InProgressRow eventType="sliding_window_truncation" />
1298+
}
1299+
// Completed state
1300+
if (message.contextTruncation) {
1301+
return <TruncationResultRow data={message.contextTruncation} />
1302+
}
1303+
return null
12871304
case "codebase_search_result":
12881305
let parsed: {
12891306
content: {

0 commit comments

Comments
 (0)