feat(assistant-api): add greetingInterruptible to deployments#149
Merged
Conversation
📝 WalkthroughWalkthroughThis PR adds a new ChangesgreetingInterruptible field propagation across deployment stack
Watchdog-based idle timeout and TTS completion management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@ui/src/app/pages/assistant/actions/create-deployment/api/__tests__/voice-input-intent.test.tsx`:
- Line 68: The test file lacks real assertions to verify that
greetingInterruptible is properly propagated in the deployment request, despite
having mock surface expansions like setGreetinginterruptible at line 68 and
similar mocks at lines 266-267. Add test cases with actual assertions that
verify the outgoing deployment request carries the greetingInterruptible value,
including both a happy-path case where it is true and an edge-case regression
test where it is false. This ensures the contract is properly tested and cannot
regress unnoticed.
In
`@ui/src/app/pages/assistant/actions/create-deployment/debugger/__tests__/voice-input-intent.test.tsx`:
- Line 64: The `greetingInterruptible` field is added to the mock implementation
in the test file but lacks corresponding test assertions to verify its behavior.
Add test cases in the voice-input-intent.test.tsx file that verify the
`greetingInterruptible` field is correctly preserved in the built deployment
request, including at least a happy-path assertion (testing true and false
values are properly handled) and a regression/edge-case assertion (such as
verifying overrides or boundary conditions). This ensures the new field behavior
is properly validated according to the UI testing guidelines requiring both
happy-path and edge-case coverage for changes to ui/src files.
In
`@ui/src/app/pages/assistant/actions/create-deployment/phone/__tests__/deployment-create-edit.test.tsx`:
- Line 69: The test file lacks assertions for the newly introduced
greetingInterruptible field. Add test assertions to verify that the create and
edit deployment flows properly send the expected greetingInterruptible value.
Specifically, add a happy-path test assertion that validates
greetingInterruptible is correctly transmitted with the expected value, and add
an edge or regression test assertion that covers an alternative scenario (such
as toggling the value or testing boundary conditions). Reference the
setGreetinginterruptible mock method to verify the field is being passed through
the deployment creation and editing flows as intended.
In
`@ui/src/app/pages/assistant/actions/create-deployment/web-plugin/__tests__/voice-input-intent.test.tsx`:
- Line 62: The test file contains mock methods like setGreetinginterruptible for
API compatibility but lacks explicit assertions verifying that the
greetingInterruptible parameter is actually preserved in the deployment creation
request. Add test assertions to verify this behavior at multiple sites where the
mock is used. Include both a happy-path assertion that confirms
greetingInterruptible is correctly passed through in the outgoing request, and a
regression or edge-case assertion (such as testing with an edge value like false
or true) to ensure the parameter is handled correctly across different
scenarios.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b5a5d8bf-1dbf-49a0-8cef-ad942f2e2988
⛔ Files ignored due to path filters (1)
ui/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (42)
api/assistant-api/api/assistant-deployment/create_assistant_api_deployment.goapi/assistant-api/api/assistant-deployment/create_assistant_api_deployment_rest.goapi/assistant-api/api/assistant-deployment/create_assistant_debugger_deployment.goapi/assistant-api/api/assistant-deployment/create_assistant_debugger_deployment_rest.goapi/assistant-api/api/assistant-deployment/create_assistant_debugger_deployment_rest_test.goapi/assistant-api/api/assistant-deployment/create_assistant_phone_deployment.goapi/assistant-api/api/assistant-deployment/create_assistant_phone_deployment_rest.goapi/assistant-api/api/assistant-deployment/create_assistant_phone_deployment_rest_test.goapi/assistant-api/api/assistant-deployment/create_assistant_webplugin_deployment.goapi/assistant-api/api/assistant-deployment/create_assistant_webplugin_deployment_rest.goapi/assistant-api/api/assistant-deployment/create_assistant_whatsapp_deployment.goapi/assistant-api/api/assistant-deployment/create_assistant_whatsapp_deployment_rest.goapi/assistant-api/api/assistant-deployment/create_other_deployments_rest_test.goapi/assistant-api/api/assistant-deployment/get_all_assistant_api_deployment_rest.goapi/assistant-api/api/assistant-deployment/get_all_assistant_debugger_deployment_rest.goapi/assistant-api/api/assistant-deployment/get_all_assistant_phone_deployment_rest.goapi/assistant-api/api/assistant-deployment/get_all_assistant_webplugin_deployment_rest.goapi/assistant-api/api/assistant-deployment/get_all_assistant_whatsapp_deployment_rest.goapi/assistant-api/api/assistant-deployment/get_assistant_api_deployment_rest.goapi/assistant-api/api/assistant-deployment/get_assistant_debugger_deployment_rest.goapi/assistant-api/api/assistant-deployment/get_assistant_phone_deployment_rest.goapi/assistant-api/api/assistant-deployment/get_assistant_webplugin_deployment_rest.goapi/assistant-api/api/assistant-deployment/get_assistant_whatsapp_deployment_rest.goapi/assistant-api/internal/services/assistant/assistant.deployment.impl.service.goapi/assistant-api/internal/services/deployment.assistant.service.goapi/document-api/app/bridges/artifacts/protos/assistant_deployment_pb2.pyapi/document-api/app/bridges/artifacts/protos/assistant_deployment_pb2.pyiui/package.jsonui/src/app/pages/assistant/actions/create-deployment/api/__tests__/voice-input-intent.test.tsxui/src/app/pages/assistant/actions/create-deployment/api/edit.tsxui/src/app/pages/assistant/actions/create-deployment/api/index.tsxui/src/app/pages/assistant/actions/create-deployment/commons/configure-experience.tsxui/src/app/pages/assistant/actions/create-deployment/debugger/__tests__/voice-input-intent.test.tsxui/src/app/pages/assistant/actions/create-deployment/debugger/edit.tsxui/src/app/pages/assistant/actions/create-deployment/debugger/index.tsxui/src/app/pages/assistant/actions/create-deployment/hooks/use-deployment-section-edit.tsui/src/app/pages/assistant/actions/create-deployment/phone/__tests__/deployment-create-edit.test.tsxui/src/app/pages/assistant/actions/create-deployment/phone/edit.tsxui/src/app/pages/assistant/actions/create-deployment/phone/index.tsxui/src/app/pages/assistant/actions/create-deployment/web-plugin/__tests__/voice-input-intent.test.tsxui/src/app/pages/assistant/actions/create-deployment/web-plugin/edit.tsxui/src/app/pages/assistant/actions/create-deployment/web-plugin/index.tsx
| private outputAudio?: DeploymentAudioProvider; | ||
| setAssistantid(_: string) {} | ||
| setGreeting(_: string) {} | ||
| setGreetinginterruptible(_: boolean) {} |
There was a problem hiding this comment.
Add a real assertion for greetingInterruptible propagation.
Line 68 and Lines 266-267 only expand the mock surface; the suite still never verifies the outgoing deployment request carries greetingInterruptible (including an edge case where it is false). This allows this contract to regress unnoticed.
As per coding guidelines, ui/src/**/*.{ts,tsx} changes must include UI unit tests, and ui/src/**/@(__tests__|*.test.tsx|*.spec.tsx) must include at least a happy-path and a regression/edge assertion tied to the change.
Also applies to: 266-267
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@ui/src/app/pages/assistant/actions/create-deployment/api/__tests__/voice-input-intent.test.tsx`
at line 68, The test file lacks real assertions to verify that
greetingInterruptible is properly propagated in the deployment request, despite
having mock surface expansions like setGreetinginterruptible at line 68 and
similar mocks at lines 266-267. Add test cases with actual assertions that
verify the outgoing deployment request carries the greetingInterruptible value,
including both a happy-path case where it is true and an edge-case regression
test where it is false. This ensures the contract is properly tested and cannot
regress unnoticed.
Source: Coding guidelines
| private outputAudio?: DeploymentAudioProvider; | ||
| setAssistantid(_: string) {} | ||
| setGreeting(_: string) {} | ||
| setGreetinginterruptible(_: boolean) {} |
There was a problem hiding this comment.
greetingInterruptible is mocked but not tested in request assertions.
Line 64 and Lines 243-244 add the field to mocks/fixtures, but no test verifies the built deployment request preserves that value (or a false-path override). Please add assertions tied to this new field behavior.
As per coding guidelines, ui/src/**/*.{ts,tsx} changes must include UI unit tests, and ui/src/**/@(__tests__|*.test.tsx|*.spec.tsx) must include at least a happy-path and a regression/edge assertion tied to the change.
Also applies to: 243-244
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@ui/src/app/pages/assistant/actions/create-deployment/debugger/__tests__/voice-input-intent.test.tsx`
at line 64, The `greetingInterruptible` field is added to the mock
implementation in the test file but lacks corresponding test assertions to
verify its behavior. Add test cases in the voice-input-intent.test.tsx file that
verify the `greetingInterruptible` field is correctly preserved in the built
deployment request, including at least a happy-path assertion (testing true and
false values are properly handled) and a regression/edge-case assertion (such as
verifying overrides or boundary conditions). This ensures the new field behavior
is properly validated according to the UI testing guidelines requiring both
happy-path and edge-case coverage for changes to ui/src files.
Source: Coding guidelines
| private phoneOptions: Metadata[] = []; | ||
| setAssistantid(_: string) {} | ||
| setGreeting(_: string) {} | ||
| setGreetinginterruptible(_: boolean) {} |
There was a problem hiding this comment.
Phone deployment tests need a greetingInterruptible assertion.
Line 69 and Lines 261-262 introduce the new field in mocks, but the tests don’t assert that create/edit flows actually send the expected greetingInterruptible value. Add a happy-path assertion and one edge/regression assertion for this field.
As per coding guidelines, ui/src/**/*.{ts,tsx} changes must include UI unit tests, and ui/src/**/@(__tests__|*.test.tsx|*.spec.tsx) must include at least a happy-path and a regression/edge assertion tied to the change.
Also applies to: 261-262
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@ui/src/app/pages/assistant/actions/create-deployment/phone/__tests__/deployment-create-edit.test.tsx`
at line 69, The test file lacks assertions for the newly introduced
greetingInterruptible field. Add test assertions to verify that the create and
edit deployment flows properly send the expected greetingInterruptible value.
Specifically, add a happy-path test assertion that validates
greetingInterruptible is correctly transmitted with the expected value, and add
an edge or regression test assertion that covers an alternative scenario (such
as toggling the value or testing boundary conditions). Reference the
setGreetinginterruptible mock method to verify the field is being passed through
the deployment creation and editing flows as intended.
Source: Coding guidelines
| private outputAudio?: DeploymentAudioProvider; | ||
| setAssistantid(_: string) {} | ||
| setGreeting(_: string) {} | ||
| setGreetinginterruptible(_: boolean) {} |
There was a problem hiding this comment.
Web-plugin suite should assert greetingInterruptible behavior, not only mock it.
Line 62 and Lines 257-258 add API-surface compatibility, but no test checks that deployment creation preserves greetingInterruptible in the outgoing request (including an edge value). Please add explicit assertions.
As per coding guidelines, ui/src/**/*.{ts,tsx} changes must include UI unit tests, and ui/src/**/@(__tests__|*.test.tsx|*.spec.tsx) must include at least a happy-path and a regression/edge assertion tied to the change.
Also applies to: 257-258
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@ui/src/app/pages/assistant/actions/create-deployment/web-plugin/__tests__/voice-input-intent.test.tsx`
at line 62, The test file contains mock methods like setGreetinginterruptible
for API compatibility but lacks explicit assertions verifying that the
greetingInterruptible parameter is actually preserved in the deployment creation
request. Add test assertions to verify this behavior at multiple sites where the
mock is used. Include both a happy-path assertion that confirms
greetingInterruptible is correctly passed through in the outgoing request, and a
regression or edge-case assertion (such as testing with an edge value like false
or true) to ensure the parameter is handled correctly across different
scenarios.
Source: Coding guidelines
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openapi/artifacts/assistant-api.yaml (1)
814-815: 💤 Low valueConsider adding a description for the
greetingInterruptiblefield.Adding a brief description would improve API documentation and help consumers understand the field's purpose. This could be applied consistently across all deployment schemas.
Example:
greetingInterruptible: type: boolean description: Whether the greeting message can be interrupted by user input🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openapi/artifacts/assistant-api.yaml` around lines 814 - 815, The greetingInterruptible field in the deployment schema is missing a description property, which reduces API documentation clarity. Add a description field to the greetingInterruptible boolean property to explain its purpose, such as "Whether the greeting message can be interrupted by user input" or similar descriptive text that clarifies the field's behavior for API consumers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@openapi/artifacts/assistant-api.yaml`:
- Around line 814-815: The greetingInterruptible field in the deployment schema
is missing a description property, which reduces API documentation clarity. Add
a description field to the greetingInterruptible boolean property to explain its
purpose, such as "Whether the greeting message can be interrupted by user input"
or similar descriptive text that clarifies the field's behavior for API
consumers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a942ed77-bdca-4276-b3dc-f16c0cb53ad1
⛔ Files ignored due to path filters (1)
protos/assistant-deployment.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (3)
openapi/artifacts/assistant-api.yamlopenapi/assistant.gen.goprotos/artifacts
✅ Files skipped from review due to trivial changes (2)
- protos/artifacts
- openapi/assistant.gen.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
api/assistant-api/internal/watchdog/idle_timeout_test.go (1)
113-120: ⚡ Quick winHarden timing assertions to reduce flaky test behavior.
Lines 113-120 and 156-160 use narrow sleep/timeout windows. Under CI jitter these can intermittently fail. Prefer polling-based assertions (
require.Eventually/assert.Never) with bounded intervals instead of fixedtime.Sleep.Also applies to: 156-160
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/assistant-api/internal/watchdog/idle_timeout_test.go` around lines 113 - 120, Replace the fixed timing assertions in the idle timeout watchdog test with polling-based assertions to reduce flakiness. Instead of using fixed time.Sleep and time.After with narrow windows, use require.Eventually or assert.Never with reasonable polling intervals and timeouts. Specifically, in the select statement where you check whether a packet was pushed (around lines 117-120), replace the time.After approach with a polling assertion that repeatedly checks the condition rather than relying on a single timeout window, and similarly apply this pattern to any other locations mentioned (lines 156-160) that use fixed sleep windows.api/assistant-api/internal/watchdog/tts_completion_test.go (1)
156-164: ⚡ Quick winUse polling-based assertions for deadline-sensitive flows.
These ranges rely on strict wall-clock timing and can be flaky in slower CI workers. Replacing fixed sleeps/timeouts with
require.Eventually/assert.Neverwill make expiration checks more stable.Also applies to: 191-205, 239-243
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/assistant-api/internal/watchdog/tts_completion_test.go` around lines 156 - 164, The test uses fixed duration sleeps and timeouts (time.Sleep and time.After with hardcoded millisecond values) which makes it flaky on slower CI workers. Replace the timing-based assertions in the ttsCompletionWatchdog test with polling-based assertions using require.Eventually or assert.Never. Specifically, convert the select block that checks if a packet was pushed before the extended deadline and the fixed sleep patterns into require.Eventually or assert.Never calls that poll for the condition rather than relying on exact wall-clock timing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@api/assistant-api/internal/watchdog/idle_timeout_test.go`:
- Around line 113-120: Replace the fixed timing assertions in the idle timeout
watchdog test with polling-based assertions to reduce flakiness. Instead of
using fixed time.Sleep and time.After with narrow windows, use
require.Eventually or assert.Never with reasonable polling intervals and
timeouts. Specifically, in the select statement where you check whether a packet
was pushed (around lines 117-120), replace the time.After approach with a
polling assertion that repeatedly checks the condition rather than relying on a
single timeout window, and similarly apply this pattern to any other locations
mentioned (lines 156-160) that use fixed sleep windows.
In `@api/assistant-api/internal/watchdog/tts_completion_test.go`:
- Around line 156-164: The test uses fixed duration sleeps and timeouts
(time.Sleep and time.After with hardcoded millisecond values) which makes it
flaky on slower CI workers. Replace the timing-based assertions in the
ttsCompletionWatchdog test with polling-based assertions using
require.Eventually or assert.Never. Specifically, convert the select block that
checks if a packet was pushed before the extended deadline and the fixed sleep
patterns into require.Eventually or assert.Never calls that poll for the
condition rather than relying on exact wall-clock timing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b60de06d-77d3-44fd-ba57-f8da4b20b0b7
📒 Files selected for processing (20)
api/assistant-api/internal/adapters/internal/behavior.goapi/assistant-api/internal/adapters/internal/behavior_test.goapi/assistant-api/internal/adapters/internal/dispatch_handler.goapi/assistant-api/internal/adapters/internal/dispatch_init_flow_test.goapi/assistant-api/internal/adapters/internal/dispatch_init_packets_test.goapi/assistant-api/internal/adapters/internal/requestor.goapi/assistant-api/internal/adapters/internal/stream_test.goapi/assistant-api/internal/adapters/router/dispatch.goapi/assistant-api/internal/adapters/router/dispatch_test.goapi/assistant-api/internal/adapters/router/router.goapi/assistant-api/internal/adapters/router/router_test.goapi/assistant-api/internal/transformer/custom/tts_websocket_v1/tts.goapi/assistant-api/internal/type/packet.goapi/assistant-api/internal/watchdog/idle_timeout.goapi/assistant-api/internal/watchdog/idle_timeout_test.goapi/assistant-api/internal/watchdog/options.goapi/assistant-api/internal/watchdog/tts_completion.goapi/assistant-api/internal/watchdog/tts_completion_test.goui/src/app/components/carbon/button/index.tsxui/src/app/pages/preview-agent/voice-agent/voice-agent.tsx
💤 Files with no reviewable changes (1)
- api/assistant-api/internal/transformer/custom/tts_websocket_v1/tts.go
✅ Files skipped from review due to trivial changes (3)
- ui/src/app/pages/preview-agent/voice-agent/voice-agent.tsx
- api/assistant-api/internal/adapters/router/dispatch_test.go
- api/assistant-api/internal/watchdog/options.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds
greetingInterruptiblesupport across backend deployment creation flows.Changes
greetingInterruptibleto OpenAPI deployment create/request response models.greetingInterruptibleto deployment proto definitions and regenerated artifacts.greetingInterruptiblein deployment REST responses.Type of Change
Related Issues
Fixes #
Checklist
General
Testing
Documentation
Security
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
Release Notes
New Features
greetingInterruptiblefor the affected deployment types.UI Updates
Tests / API Schema