refactor: make ChatMessagePart a discriminated union#23168
Conversation
0dd5723 to
3c75c37
Compare
d8bfaaf to
3f49522
Compare
| "signature": "added in #22290, never populated by any code path", | ||
| "result_delta": "added in #22290, never populated by any code path", |
There was a problem hiding this comment.
@kylecarbs should we just drop these from ChatMessagePart?
3f49522 to
9931f72
Compare
7ddf276 to
aa3a741
Compare
… types and dead fields RenderBlock's file-reference variant used camelCase field names (fileName, startLine, endLine) while the API sends snake_case. The parser destructured and renamed every field. The file variant was defined inline duplicating ChatFilePart. Replace both inline definitions with direct references to the generated ChatFilePart and ChatFileReferencePart types. The file-reference parser case becomes a pass-through. The file case now constructs a typed object instead of casting Record<string, unknown>. Remove dead fields confirmed by Kyle: - title on thinking/reasoning blocks (never populated by backend) - text on file-reference blocks (abandoned file comment feature) This simplifies ReasoningDisclosure by removing the disclosure button path (dead without title) and the title prop. The component now always renders inline text or a streaming placeholder. Refs #23168
|
I'm fine with the frontend changes but delegating to @Emyrk for final approval as CODEOWNER |
… types and dead fields RenderBlock's file-reference variant used camelCase field names (fileName, startLine, endLine) while the API sends snake_case. The parser destructured and renamed every field. The file variant was defined inline duplicating ChatFilePart. Replace both inline definitions with direct references to the generated ChatFilePart and ChatFileReferencePart types. The file-reference parser case becomes a pass-through. The file case now constructs a typed object instead of casting Record<string, unknown>. Remove dead fields confirmed by Kyle: - title on thinking/reasoning blocks (never populated by backend) - text on file-reference blocks (abandoned file comment feature) This simplifies ReasoningDisclosure by removing the disclosure button path (dead without title) and the title prop. The component now always renders inline text or a streaming placeholder. Additional cleanup: removed createBlock helper (identical branches after title removal), joinText parameter (no production caller), appendParsedTextBlock wrapper (pass-through after title removal), appendStreamTextBlock alias (no contrasting variant). Added P2 streaming tool-result shape test. Refs #23168
… types and dead fields RenderBlock's file-reference variant used camelCase field names (fileName, startLine, endLine) while the API sends snake_case. The parser destructured and renamed every field. The file variant was defined inline duplicating ChatFilePart. Replace both inline definitions with direct references to the generated ChatFilePart and ChatFileReferencePart types. The file-reference parser case becomes a pass-through. The file case now constructs a typed object instead of casting Record<string, unknown>. Remove dead fields confirmed by Kyle: - title on thinking/reasoning blocks (never populated by backend) - text on file-reference blocks (abandoned file comment feature) This simplifies ReasoningDisclosure by removing the disclosure button path (dead without title) and the title prop. The component now always renders inline text or a streaming placeholder. Additional cleanup: removed createBlock helper (identical branches after title removal), joinText parameter (no production caller), appendParsedTextBlock wrapper (pass-through after title removal), appendStreamTextBlock alias (no contrasting variant). Added P2 streaming tool-result shape test. Refs #23168
Emyrk
left a comment
There was a problem hiding this comment.
The overall design is fine, and glad to see people using the guts api in the abusive ways that it should be! 😄
I don't see the golang side of this though. Are we doing json marshaling of this type in Go?
| Required: []string{"url"}, | ||
| Optional: []string{"source_id", "title"}, | ||
| }, | ||
| } |
There was a problem hiding this comment.
How is this used in the golang side of things? Like how are we marshaling to json for the db and unmarshaling?
There was a problem hiding this comment.
@Emyrk It's a simple flat type that we write and read from the database as jsonb, you can see it here:
Lines 101 to 214 in 6fc9f19
I could see us changing this down the line to add more type/field safety, but for now it's what we have. The writes to DB are versioned, so if we want to change it or break it in the future we can do so in a back-compatible manner.
There was a problem hiding this comment.
It feels werid to write the fields like this:
var ChatMessagePartVariants = []ChatMessagePartVariant{
{
TypeLiteral: ChatMessagePartTypeText,
Required: []string{"text"},
},
{
TypeLiteral: ChatMessagePartTypeReasoning,
Required: []string{"text"},
},
{
TypeLiteral: ChatMessagePartTypeToolCall,
Required: []string{"tool_call_id", "tool_name"},
Optional: []string{"args", "args_delta", "provider_executed"},
},
{
TypeLiteral: ChatMessagePartTypeToolResult,
Required: []string{"tool_call_id", "tool_name"},
Optional: []string{"result", "is_error", "provider_executed"},
},
...
It feels like we are duplicating source of truth. Maybe a another struct tag to reuse the json tag? And use reflect?
There was a problem hiding this comment.
@Emyrk yeah I know what you mean. I initially prototyped this with using an intermediary type var foo ChatMessagePart and referencing the structure fields field(&foo.Text) where the function used reflect to look up the json tag. While it would work and give some compile-time safety, I'm not too excited about using reflect in codersdk. I'd also like these definitions to exist close to ChatMessagePart. My second solution was this with accompanying test that enforces all fields being referenced, and using valid tags.
Another option I entertained was defining this via struct tags (e.g. Text field belongs/required by types "foo" and "bar", but that quickly becomes hard to read and easy to mess up.
For now this felt like the cleanest approach, given the data type.
There was a problem hiding this comment.
Would the reflect not only live in the main.go for api typings?
Are these field defintions used in the codersdk? How I understood it is that these field definitions exist for the api typings
There was a problem hiding this comment.
Do you mean moving all the logic to apitypings? (If not, perhaps I'm not fully understanding what you're proposing and an example would be helpful.)
Are these field defintions used in the codersdk?
Not exactly, but I originally had a concern with defining these types fully in the apitypings package as that creates a disconnect with where the type are used/defined (i.e. Go).
Even though these types are weakly defined in Go, (as linked above) e.g.:
func ChatMessageText(text string) ChatMessagePart {
return ChatMessagePart{Type: ChatMessagePartTypeText, Text: text}
}Let's say we expand this to be:
func ChatMessageText(text, foo string) ChatMessagePart {
return ChatMessagePart{Type: ChatMessagePartTypeText, Text: text, Foo: foo}
}
The author making the change may notice that the TS types are not updating, but they'd have to know to dive into the apitypings package to update the definition, compared to it being in the same file.
Another option would be to actually define these Go types in codersdk, but having them there without refactoring all code to use them would also be a bit awkward, as is the data format since we have []ChatMessagePart and expressing that as []ChatMessageText | ChatMessage... doesn't map cleanly in all situations. It's something I'm actively thinking about how we should improve though.
If you think we should move this logic to apitypings, however, happy to make the change.
There was a problem hiding this comment.
Reworked. Variant membership is now declared via variants struct tags directly on ChatMessagePart fields:
type ChatMessagePart struct {
Type ChatMessagePartType `json:"type"`
Text string `json:"text,omitempty" variants:"text,reasoning"`
ToolCallID string `json:"tool_call_id,omitempty" variants:"tool-call,tool-result"`
Args json.RawMessage `json:"args,omitempty" variants:"tool-call?"`
// ...
}Bare name = required, ? suffix = optional. Fields without a variants tag are excluded from the union.
The codegen mutation in apitypings/main.go reads these tags via reflect and builds the union from them. ChatMessagePartVariant type and ChatMessagePartVariants slice are removed from codersdk. The test validates every field either has a variants tag or is in an explicit exclusion list.
78468a8 to
677c711
Compare
…ated union
Generate a discriminated union for ChatMessagePart via a guts codegen
mutation instead of the flat interface with 20+ optional fields. Each
variant (ChatTextPart, ChatReasoningPart, ChatToolCallPart,
ChatToolResultPart, ChatFilePart, ChatFileReferencePart, ChatSourcePart)
narrows the type field to a string literal and includes only relevant
fields. Dead fields (signature, result_delta) are excluded.
Remove dead code that handled situations the backend prevents:
normalizeBlockType (backend sends canonical types), alias case branches
("thinking", "toolcall", "toolresult"), dead field fallbacks
(typedBlock.name, typedBlock.id, typedBlock.input, typedBlock.output),
and result_delta handling in mergeStreamPayload.
Add test coverage for args_delta streaming (the primary tool-call
streaming mechanism, previously untested), provider_executed skip logic,
and source part parsing/streaming with grouping and deduplication.
Fix type errors surfaced by the union: add type narrowing filter in
AgentDetailContent.tsx, correct story fixture field names, cast
reasoning story data that uses frontend-only title field.
677c711 to
08421e5
Compare
Generate a discriminated union for
ChatMessagePartvia a guts codegenmutation instead of the flat interface with 20+ optional fields. Each
variant (
ChatTextPart,ChatReasoningPart,ChatToolCallPart,ChatToolResultPart,ChatFilePart,ChatFileReferencePart,ChatSourcePart) narrows thetypefield to a string literal andincludes only the fields relevant to that part type.
Approach
The variant definitions live in
codersdk.ChatMessagePartVariants,right next to the
ChatMessagePartstruct. Each entry specifies a typeliteral and lists required/optional JSON field names. The codegen
mutation in
scripts/apitypings/main.goreads this table, copies fieldtype information from the original interface, and produces per-variant
sub-interfaces plus a union alias.
A test (
TestChatMessagePartVariants) validates every field name inthe variant table against actual struct tags via reflection, catching
renames, typos, and stale entries.
Dead fields (
signature,result_delta) are excluded from thegenerated variants. They remain on the Go struct for backward-compatible
JSON deserialization of stored rows.
Dead code removal
Remove code that handled situations the backend already prevents:
normalizeBlockType(backend always sends canonical type strings)"thinking","toolcall","toolresult")typedBlock.name,typedBlock.id,typedBlock.input,typedBlock.output, etc.)result_deltahandling inmergeStreamPayload(never set by backend)Test coverage
Add 10 new tests covering previously-untested paths:
args_deltastreaming accumulation (P0, the backend's primarytool-call streaming mechanism)
provider_executedskip logic for tool-call and tool-resultsourcepart parsing and streaming with grouping and deduplicationType error fixes
The union surfaced three issues in existing code:
AgentDetailContent.tsx: accessedmedia_type/file_idwithoutnarrowing to
ChatFilePart. Added a type predicate filter.ConversationTimeline.stories.tsx: usedtextinstead ofcontenton a
file-referencefixture (wrong field name).AgentDetail.stories.tsx: reasoning story fixture uses atitlefield that only exists at runtime (extracted by the parser via
asOptionalTitle), not on the generatedChatReasoningParttype.