Repository navigation
fix(ui): show grep search parameters in pending tool state #2070
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 all commits
41ef84a
f9be07d
8a55a16
4d9b67d
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 |
|---|---|---|
|
|
@@ -92,16 +92,33 @@ func NewGrepToolMessageItem( | |
| // GrepToolRenderContext renders grep tool messages. | ||
| type GrepToolRenderContext struct{} | ||
|
|
||
| // partialGrepParams recovers the grep parameters that already finished | ||
| // streaming from a partial tool call input. | ||
| func partialGrepParams(input string) tools.GrepParams { | ||
| fields := partialJSONFields(input) | ||
| var params tools.GrepParams | ||
| params.Pattern, _ = fields["pattern"].(string) | ||
| params.Path, _ = fields["path"].(string) | ||
| params.Include, _ = fields["include"].(string) | ||
| params.LiteralText, _ = fields["literal_text"].(bool) | ||
| return params | ||
| } | ||
|
|
||
| // RenderTool implements the [ToolRenderer] interface. | ||
| func (g *GrepToolRenderContext) RenderTool(sty *styles.Styles, width int, opts *ToolRenderOpts) string { | ||
| cappedWidth := cappedMessageWidth(width) | ||
| if opts.IsPending() { | ||
| return pendingTool(sty, "Grep", opts.Anim, opts.Compact) | ||
| } | ||
|
|
||
| var params tools.GrepParams | ||
| if err := json.Unmarshal([]byte(opts.ToolCall.Input), ¶ms); err != 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. The pending check now runs after the input is parsed, but while a tool call is still streaming its input is usually incomplete. That means users will briefly see an "Invalid parameters" error instead of the loading spinner. The pending branch should fall back to the old spinner when parsing fails, rather than showing an error. Can you test this out?
Contributor
Author
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. Good catch — a partial input like Now falls back to the old
Contributor
Author
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. Went one step further here: while the input streams, a token walk (
Contributor
Author
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. I am thinking of stream parsing it instead though (WIP)
Contributor
Author
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. Stream parsing landed: after the token walk, the raw tail is matched for the value still streaming, so the pattern grows character by character (cut escapes like a trailing \ or short \u are trimmed). Covered in |
||
| return toolErrorContent(sty, &message.ToolResult{Content: "Invalid parameters"}, cappedWidth) | ||
| if !opts.IsPending() { | ||
| return toolErrorContent(sty, &message.ToolResult{Content: "Invalid parameters"}, cappedWidth) | ||
| } | ||
| // While the tool call input is still streaming it is incomplete | ||
| // and not valid JSON yet. Show whatever fields already arrived, | ||
| // or the plain spinner until the pattern is complete. | ||
| params = partialGrepParams(opts.ToolCall.Input) | ||
| if params.Pattern == "" { | ||
| return pendingTool(sty, "Grep", opts.Anim, opts.Compact) | ||
| } | ||
| } | ||
|
|
||
| toolParams := []string{params.Pattern} | ||
|
|
@@ -115,6 +132,15 @@ func (g *GrepToolRenderContext) RenderTool(sty *styles.Styles, width int, opts * | |
| toolParams = append(toolParams, "literal", "true") | ||
| } | ||
|
|
||
| if opts.IsPending() { | ||
|
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 old pending view respected
Contributor
Author
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.
I think this one is actually covered — toolHeader applies the nested style itself: nested := opts != nil && opts.Compact // tools.go, toolHeaderSame |
||
| header := toolHeader(sty, opts.Status, "Grep", cappedWidth, opts, toolParams...) | ||
| var animView string | ||
| if opts.Anim != nil { | ||
| animView = opts.Anim.Render() | ||
| } | ||
| return header + " " + animView | ||
| } | ||
|
|
||
| header := toolHeader(sty, opts.Status, "Grep", cappedWidth, opts, toolParams...) | ||
| if opts.Compact { | ||
| return header | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,213 @@ | ||
| package chat | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/charmbracelet/crush/internal/message" | ||
| "github.com/charmbracelet/crush/internal/ui/anim" | ||
| "github.com/charmbracelet/crush/internal/ui/styles" | ||
| "github.com/charmbracelet/x/ansi" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| // TestGrepToolMessageItem_PendingShowsSearchParameters guards that a running | ||
| // grep renders its search parameters next to the spinner instead of hiding | ||
| // them behind a bare "Grep" label. | ||
| func TestGrepToolMessageItem_PendingShowsSearchParameters(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| sty := styles.CharmtonePantera() | ||
| ctx := &GrepToolRenderContext{} | ||
| out := ansi.Strip(ctx.RenderTool(&sty, 120, &ToolRenderOpts{ | ||
| ToolCall: message.ToolCall{ | ||
| ID: "grep1", | ||
| Name: "grep", | ||
| Input: `{"pattern":"A9041","path":".","include":"*.go","literal_text":true}`, | ||
| Finished: false, | ||
| }, | ||
| Status: ToolStatusRunning, | ||
| })) | ||
|
|
||
| require.Contains(t, out, "Grep") | ||
| require.Contains(t, out, "A9041") | ||
| require.Contains(t, out, "path=.") | ||
| require.Contains(t, out, "include=*.go") | ||
| require.Contains(t, out, "literal=true") | ||
| require.NotContains(t, out, "Invalid parameters") | ||
| } | ||
|
|
||
| // TestGrepToolMessageItem_PendingPartialInputFallsBackToSpinner guards that | ||
| // while the tool call input is still streaming and no field usable for the | ||
| // header exists yet (nothing streamed, a key not closed, or a value not | ||
| // started), the pending state falls back to the plain spinner instead of | ||
| // flashing an "Invalid parameters" error. | ||
| func TestGrepToolMessageItem_PendingPartialInputFallsBackToSpinner(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| sty := styles.CharmtonePantera() | ||
| ctx := &GrepToolRenderContext{} | ||
| streamingPrefixes := []string{"", "{", `{"pattern"`, `{"pattern":`} | ||
|
|
||
| for _, compact := range []bool{false, true} { | ||
| for _, input := range streamingPrefixes { | ||
| a := anim.New(anim.Settings{ID: "grep-anim", Label: "Working"}) | ||
| out := ctx.RenderTool(&sty, 120, &ToolRenderOpts{ | ||
| ToolCall: message.ToolCall{ | ||
| ID: "grep2", | ||
| Name: "grep", | ||
| Input: input, | ||
| Finished: false, | ||
| }, | ||
| Status: ToolStatusRunning, | ||
| Anim: a, | ||
| Compact: compact, | ||
| }) | ||
| require.Equal(t, pendingTool(&sty, "Grep", a, compact), out, | ||
| "pending grep with partial input %q must render the plain spinner (compact=%v)", input, compact) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // TestGrepToolMessageItem_PendingPartialInputShowsCompleteFields guards that | ||
| // fields whose JSON value already closed are rendered while the rest of the | ||
| // input is still streaming, so the search context appears progressively. | ||
| func TestGrepToolMessageItem_PendingPartialInputShowsCompleteFields(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| sty := styles.CharmtonePantera() | ||
| ctx := &GrepToolRenderContext{} | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| input string | ||
| contains []string | ||
| missing []string | ||
| }{ | ||
| { | ||
| name: "pattern closed, object not", | ||
| input: `{"pattern":"fo"`, | ||
| contains: []string{"fo"}, | ||
| missing: []string{"path=", "include=", "literal="}, | ||
| }, | ||
| { | ||
| name: "path closed, object not", | ||
| input: `{"pattern":"foo","path":"."`, | ||
| contains: []string{"foo", "path=."}, | ||
| missing: []string{"include=", "literal="}, | ||
| }, | ||
| { | ||
| name: "all fields closed, closing brace missing", | ||
| input: `{"pattern":"foo","path":".","include":"*.go","literal_text":true`, | ||
| contains: []string{"foo", "path=.", "include=*.go", "literal=true"}, | ||
| missing: []string{}, | ||
| }, | ||
| { | ||
| name: "wrongly typed pattern shows no header at all", | ||
| input: `{"pattern":123,"path":"."`, | ||
| contains: []string{"Grep"}, | ||
| missing: []string{"123", "path="}, | ||
| }, | ||
| { | ||
| name: "pattern grows while its value streams", | ||
| input: `{"pattern":"fo`, | ||
| contains: []string{"fo"}, | ||
| missing: []string{"path=", "Invalid parameters"}, | ||
| }, | ||
| { | ||
| name: "cut escape at end of streaming value", | ||
| input: `{"pattern":"a\u00`, | ||
| contains: []string{"Grep a"}, | ||
| missing: []string{"u00"}, | ||
| }, | ||
| { | ||
| name: "escaped quote inside streaming value", | ||
| input: `{"pattern":"a\"b`, | ||
| contains: []string{`a"b`}, | ||
| missing: []string{}, | ||
| }, | ||
| { | ||
| name: "partial next key keeps complete pattern", | ||
| input: `{"pattern":"foo","pa`, | ||
| contains: []string{"foo"}, | ||
| missing: []string{"pa"}, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| out := ansi.Strip(ctx.RenderTool(&sty, 120, &ToolRenderOpts{ | ||
| ToolCall: message.ToolCall{ | ||
| ID: "grep5", | ||
| Name: "grep", | ||
| Input: tt.input, | ||
| Finished: false, | ||
| }, | ||
| Status: ToolStatusRunning, | ||
| })) | ||
|
|
||
| for _, want := range tt.contains { | ||
| require.Contains(t, out, want) | ||
| } | ||
| for _, notWant := range tt.missing { | ||
| require.NotContains(t, out, notWant) | ||
| } | ||
| require.NotContains(t, out, "Invalid parameters") | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // TestGrepToolMessageItem_PendingHeaderMatchesCompletedHeader guards that the | ||
| // pending state renders the very same header as the completed state, so | ||
| // nested (compact) grep calls keep their styling while loading. | ||
| func TestGrepToolMessageItem_PendingHeaderMatchesCompletedHeader(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| sty := styles.CharmtonePantera() | ||
| ctx := &GrepToolRenderContext{} | ||
| tc := message.ToolCall{ | ||
| ID: "grep3", | ||
| Name: "grep", | ||
| Input: `{"pattern":"foo","path":"."}`, | ||
| } | ||
| opts := func(finished bool) *ToolRenderOpts { | ||
| tc := tc | ||
| tc.Finished = finished | ||
| return &ToolRenderOpts{ | ||
| ToolCall: tc, | ||
| Status: ToolStatusRunning, | ||
| Compact: true, | ||
| } | ||
| } | ||
|
|
||
| a := anim.New(anim.Settings{ID: "grep-anim", Label: "Working"}) | ||
| pendingOpts := opts(false) | ||
| pendingOpts.Anim = a | ||
| pending := ctx.RenderTool(&sty, 120, pendingOpts) | ||
| completed := ctx.RenderTool(&sty, 120, opts(true)) | ||
|
|
||
| require.Equal(t, completed+" "+a.Render(), pending, | ||
| "pending compact grep must reuse the completed (nested-styled) header plus the spinner") | ||
| } | ||
|
|
||
| // TestGrepToolMessageItem_InvalidInputStillErrorsWhenFinished guards that the | ||
| // fallback only applies to pending calls: once a call is finished, invalid | ||
| // JSON still renders the "Invalid parameters" error. | ||
| func TestGrepToolMessageItem_InvalidInputStillErrorsWhenFinished(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| sty := styles.CharmtonePantera() | ||
| ctx := &GrepToolRenderContext{} | ||
| out := ansi.Strip(ctx.RenderTool(&sty, 120, &ToolRenderOpts{ | ||
| ToolCall: message.ToolCall{ | ||
| ID: "grep4", | ||
| Name: "grep", | ||
| Input: `{"pattern":"fo`, | ||
| Finished: true, | ||
| }, | ||
| Status: ToolStatusError, | ||
| })) | ||
|
|
||
| require.Contains(t, out, "Invalid parameters") | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.