fix: use state.values.recentMessages for cloud compatibility #12
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,111 +1,59 @@ | ||
| import { describe, test, expect } from 'bun:test'; | ||
| import { buildConversationContext } from '../../src/utils/context'; | ||
| import { createMockMessage, createMockState } from '../helpers/mockRuntime'; | ||
|
There was a problem hiding this comment. Unused Import: |
||
|
|
||
| describe('buildConversationContext', () => { | ||
| test('returns message text when no recent messages in values', () => { | ||
|
There was a problem hiding this comment. Test Coverage Gap: The old tests validated important behaviors that are now gone:
Consider adding a test that verifies behavior when |
||
| const message = createMockMessage({ | ||
| content: { text: 'Activate my workflow' }, | ||
| }); | ||
| const state = createMockState(); | ||
|
|
||
| const result = buildConversationContext(message, state); | ||
| expect(result).toBe('Activate my workflow'); | ||
| }); | ||
|
|
||
| test('returns empty string when no text and no recent messages', () => { | ||
| const message = createMockMessage({ content: { text: '' } }); | ||
| const state = createMockState(); | ||
|
|
||
| const result = buildConversationContext(message, state); | ||
|
There was a problem hiding this comment. Test Clarity: This test expects an empty string when both text and recentMessages are empty. This is correct, but consider adding a test case for when |
||
| expect(result).toBe(''); | ||
| }); | ||
|
|
||
| test('handles undefined state', () => { | ||
|
There was a problem hiding this comment. Missing Edge Case Test: Consider adding a test for when test('handles undefined message content', () => {
const message = createMockMessage({ content: undefined });
const state = createMockState();
const result = buildConversationContext(message, undefined);
expect(result).toBe('');
});This would ensure the type safety issue mentioned in |
||
| const message = createMockMessage({ content: { text: 'Hello' } }); | ||
|
|
||
| const result = buildConversationContext(message, undefined); | ||
| expect(result).toBe('Hello'); | ||
| }); | ||
|
|
||
| test('appends current request to recentMessages', () => { | ||
|
There was a problem hiding this comment. Test Coverage: The tests now validate the new string-based format, which is good. However, consider adding tests for:
These edge cases are important for robustness in production. There was a problem hiding this comment. Test Coverage Gap: Consider adding a test for when test('handles empty string recentMessages', () => {
const message = createMockMessage({ content: { text: 'Test' } });
const state = createMockState({
values: { recentMessages: '' },
});
const result = buildConversationContext(message, state);
expect(result).toBe('Test');
});This would clarify the expected behavior for the falsy check on line 8 of There was a problem hiding this comment. Test Coverage - Missing Case: Consider adding a test for when Example: test('handles non-string recentMessages gracefully', () => {
const message = createMockMessage({ content: { text: 'Hello' } });
const state = createMockState({
values: { recentMessages: { invalid: 'object' } },
});
const result = buildConversationContext(message, state);
expect(result).toBe('Hello'); // Should fall back to just the message
}); |
||
| const message = createMockMessage({ content: { text: 'Activate it' } }); | ||
| const state = createMockState({ | ||
| values: { | ||
| recentMessages: | ||
| 'User: Show me my workflows\nAssistant: Here are your workflows: Stripe, Gmail', | ||
| }, | ||
| }); | ||
|
|
||
| const result = buildConversationContext(message, state); | ||
| expect(result).toContain('User: Show me my workflows'); | ||
| expect(result).toContain('Assistant: Here are your workflows'); | ||
| expect(result).toContain('Current request: Activate it'); | ||
| }); | ||
|
|
||
| test('preserves recentMessages formatting from provider', () => { | ||
|
There was a problem hiding this comment. Missing Test Coverage: While this test verifies that preformatted messages are preserved, there's no test coverage for:
Consider adding tests like: test('handles non-string recentMessages gracefully', () => {
const message = createMockMessage({ content: { text: 'Test' } });
const state = createMockState({
values: { recentMessages: 123 }, // number instead of string
});
const result = buildConversationContext(message, state);
expect(result).toBe('Test'); // Should fall back to message text
});There was a problem hiding this comment. Good test coverage: This test verifies that the function preserves whatever formatting the provider supplies, which is exactly the right behavior. This demonstrates the simplified approach works correctly with different provider implementations. There was a problem hiding this comment. Good Test: This test validates that pre-formatted messages from providers are preserved as-is, which is important for the cloud compatibility fix. Nice addition! 👍 However, the test name "preserves recentMessages formatting from provider" could be more specific about what formatting is being preserved (timestamps, custom prefixes, etc.). There was a problem hiding this comment. ✅ Excellent Test: This test is critical for ensuring the fix works correctly. It verifies that:
This is a key requirement for cross-provider compatibility. Great addition! |
||
| const message = createMockMessage({ content: { text: 'Do something' } }); | ||
| const preformattedMessages = `[2024-01-01 10:00] Alice: Hello | ||
| [2024-01-01 10:01] Bot: Hi there! | ||
| [2024-01-01 10:02] Alice: Help me`; | ||
|
|
||
| const state = createMockState({ | ||
| values: { recentMessages: preformattedMessages }, | ||
| }); | ||
|
|
||
| const result = buildConversationContext(message, state); | ||
| expect(result).toBe(`${preformattedMessages}\n\nCurrent request: Do something`); | ||
| }); | ||
| }); | ||
|
There was a problem hiding this comment. Missing test coverage: Consider adding tests for edge cases:
test('handles non-string recentMessages gracefully', () => {
const message = createMockMessage({ content: { text: 'Test' } });
const state = createMockState({
values: { recentMessages: ['msg1', 'msg2'] }, // Wrong type
});
const result = buildConversationContext(message, state);
expect(result).toBe('Test'); // Should fallback safely
});
test('handles undefined message content', () => {
const message = createMockMessage({ content: undefined });
const state = createMockState();
expect(() => buildConversationContext(message, state)).not.toThrow();
});There was a problem hiding this comment. Missing Test Coverage: The tests don't cover important edge cases:
Consider adding tests like: test('handles non-string recentMessages gracefully', () => {
const message = createMockMessage({ content: { text: 'Request' } });
const state = createMockState({
values: { recentMessages: ['msg1', 'msg2'] }, // Array instead of string
});
const result = buildConversationContext(message, state);
expect(result).toBe('Request'); // Should fallback safely
});
test('handles empty current message with recentMessages', () => {
const message = createMockMessage({ content: { text: '' } });
const state = createMockState({
values: { recentMessages: 'User: Previous message' },
});
const result = buildConversationContext(message, state);
// Verify expected behavior for this edge case
}); |
||
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.
Minor - Code Style: Good job removing the unused
createMockRuntimeimport. The test cleanup is thorough and maintains good coverage.