feat: Improved initial prompt handling #142
Conversation
|
✅ Preview binaries are ready! To test with modules: |
hugodutka
left a comment
hugodutka
left a comment
There was a problem hiding this comment.
Please add a test in conversation_test.go to check if the new logic behaves correctly.
|
resolved all the comments. |
| func TestInitialPromptReadiness(t *testing.T) { | ||
| now := time.Now() | ||
| changing := st.ConversationStatusChanging | ||
| stable := st.ConversationStatusStable |
There was a problem hiding this comment.
IMO these add indirection and make it harder to read the tests. I'd suggest inlining the original values instead.
| // Agent becomes ready | ||
| c.AddSnapshot("ready") | ||
| c.AddSnapshot("ready") | ||
| c.AddSnapshot("ready") |
There was a problem hiding this comment.
Do we really need to test snapshot interval/stability length in all of these tests? I imagine we could simplify by changing the config and only add a single snapshot.
Note: Since we're never testing the opposite (assert.False(..., c.Status())), testing snapshot/stability this way doesn't actually add any value.
| // Screen changes should cause changing status | ||
| c.AddSnapshot("changing") | ||
| assert.Equal(t, changing, c.Status()) | ||
| assert.True(t, c.AgentReadyForInitialPrompt) |
There was a problem hiding this comment.
If screen is changing, can we really assume ready for initial prompt is true?
lib/screentracker/conversation.go
Outdated
|
|
||
| if !c.InitialPromptSent && !c.AgentReadyForInitialPrompt { | ||
| if len(snapshots) > 0 && c.cfg.IsAgentReadyForInitialPrompt(snapshots[len(snapshots)-1].screen) { | ||
| c.AgentReadyForInitialPrompt = true |
There was a problem hiding this comment.
Suggestion: Consider resetting this after initial prompt is sent.
3 participants
Example - Opencode agent:
The PR tries to fix this by taking into account whether the input box is visible or not.