-
Notifications
You must be signed in to change notification settings - Fork 410
refactor(tui): extract attachments from document parts in session #3484
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
85616ba
5b3f376
1e98435
0193f93
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -488,8 +488,35 @@ func (a *App) Run(ctx context.Context, cancel context.CancelFunc, message string | |
| case att.Content != "": | ||
| // Inline content attachment (e.g. pasted text). | ||
| a.processInlineAttachment(att, &textBuilder) | ||
| case len(att.Data) > 0: | ||
| // Binary inline content. | ||
| doc, resizeMeta, procErr := chat.ProcessAttachmentWithMetadata(chat.MessagePart{ | ||
| Type: chat.MessagePartTypeDocument, | ||
| Document: &chat.Document{ | ||
| Name: att.Name, | ||
| MimeType: att.MimeType, | ||
| Source: chat.DocumentSource{ | ||
| InlineData: att.Data, | ||
| }, | ||
| }, | ||
| }) | ||
| if procErr != nil { | ||
| slog.WarnContext(ctx, "skipping inline attachment: processing failed", "name", att.Name, "error", procErr) | ||
| a.sendEvent(ctx, runtime.Warning(fmt.Sprintf("Skipped attachment %s: %s", att.Name, procErr), "")) | ||
| continue | ||
| } | ||
| if resizeMeta != nil { | ||
| if note := chat.FormatDimensionNote(resizeMeta); note != "" { | ||
| textBuilder.WriteString("\n") | ||
| textBuilder.WriteString(note) | ||
| } | ||
| } | ||
| binaryParts = append(binaryParts, chat.MessagePart{ | ||
| Type: chat.MessagePartTypeDocument, | ||
| Document: &doc, | ||
| }) | ||
| default: | ||
| slog.DebugContext(ctx, "skipping attachment with no file path or content", "name", att.Name) | ||
| slog.DebugContext(ctx, "skipping attachment with no file path, content, or data", "name", att.Name) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -599,7 +626,8 @@ func (a *App) processFileAttachment(ctx context.Context, att messages.Attachment | |
| // For images, emit a dimension note so the model can map coordinates back to the original. | ||
| if resizeMeta != nil { | ||
| if note := chat.FormatDimensionNote(resizeMeta); note != "" { | ||
| textBuilder.WriteString("\n" + note) | ||
| textBuilder.WriteString("\n") | ||
| textBuilder.WriteString(note) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Splitting |
||
| } | ||
| } | ||
| *binaryParts = append(*binaryParts, chat.MessagePart{ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,10 @@ type Attachment struct { | |
| // backing temp file is cleaned up before the message reaches the app layer. | ||
| // Empty for file-reference attachments that are read from disk. | ||
| Content string | ||
| // MimeType is the MIME type of the binary data, if Data is present. | ||
| MimeType string | ||
| // Data holds raw binary content for non-text inline attachments. | ||
| Data []byte | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The struct's doc comment at the top of this file is now stale. It still states that the value is either |
||
| } | ||
|
|
||
| // Session lifecycle messages control session state and persistence. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -785,8 +785,8 @@ func (p *chatPage) handleInlineEditCancelled(msg messages.InlineEditCancelledMsg | |
| } | ||
|
|
||
| // extractAttachmentsFromSession extracts attachments from a session message at the given position. | ||
| // Attachments are stored as text parts in MultiContent with format "Contents of <filename>: <dataURL>". | ||
| // TODO(krisetto): meh we can store and retrieve attachments better in the session itself | ||
| // Legacy attachments are stored as text parts in MultiContent with format "Contents of <filename>: <dataURL>". | ||
| // New attachments are stored as Document parts. | ||
| func (p *chatPage) extractAttachmentsFromSession(position int) []msgtypes.Attachment { | ||
| sess := p.app.Session() | ||
| if sess == nil || position < 0 || position >= len(sess.Messages) { | ||
|
|
@@ -805,20 +805,37 @@ func (p *chatPage) extractAttachmentsFromSession(position int) []msgtypes.Attach | |
| } | ||
|
|
||
| var attachments []msgtypes.Attachment | ||
| const prefix = "Contents of " | ||
| const legacyPrefix = "Contents of " | ||
|
|
||
| // Skip the first part (main text content), look for attachment parts | ||
| for i := 1; i < len(msg.MultiContent); i++ { | ||
| part := msg.MultiContent[i] | ||
|
|
||
| if part.Type == chat.MessagePartTypeDocument && part.Document != nil { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No regression test accompanies this change, and this is the exact path that previously shipped the context-overflow bug. A unit test on |
||
| content := part.Document.Source.InlineText | ||
| data := part.Document.Source.InlineData | ||
| mimeType := part.Document.MimeType | ||
|
|
||
| if content != "" || len(data) > 0 { | ||
| attachments = append(attachments, msgtypes.Attachment{ | ||
| Name: part.Document.Name, | ||
| Content: content, | ||
| MimeType: mimeType, | ||
| Data: data, | ||
| }) | ||
| } | ||
| continue | ||
| } | ||
|
|
||
| if part.Type != chat.MessagePartTypeText { | ||
| continue | ||
| } | ||
| text := part.Text | ||
| if !strings.HasPrefix(text, prefix) { | ||
| if !strings.HasPrefix(text, legacyPrefix) { | ||
| continue | ||
| } | ||
| // Parse "Contents of <filename>: <dataURL>" | ||
| rest := text[len(prefix):] | ||
| rest := text[len(legacyPrefix):] | ||
| before, after, ok := strings.Cut(rest, ": ") | ||
| if !ok { | ||
| continue | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Routing binary payloads back through
ProcessAttachmentWithMetadatais the right call and keeps the size and decompression-bomb guards. Worth a test for the resend path: an already-processed image re-entersResizeImage, which returnsResized:falsewhen it already fits within limits, soFormatDimensionNoteyields no duplicate dimension note. A test would document that idempotence.