feat(platform): UI polish and workflow validation fixes#458
Conversation
Fix assistant thinking indicator to show during empty streaming responses. Extract TeamFilterMenu into its own component. Replace Form/Description wrappers with native HTML in import forms. Fix vendors table cell spacing. Fix workflow step validation to only run when both stepType and config are present, preventing infinite retry loops on partial updates. Strip misplaced nextSteps from action config with a warning instead of failing. Improve AI assistant communication rules to be more concise and actionable. Add tests for action step validation, action parameter validation, and step config validation covering common AI agent mistake patterns.
Israeltheminer
force-pushed
the
feat/custom-agents-ux-polish
branch
from
bb51563 to
aecf3ba
Compare
📝 WalkthroughWalkthroughThis PR encompasses multiple distinct changes across the platform services: refactoring form components to use plain HTML instead of semantic UI components (Form, Description), extracting a new TeamFilterMenu component for better modularity, adding thinking animation detection in the automation assistant based on response waiting state, adjusting workflow step validation logic to require both stepType and config parameters before validation, and adding comprehensive test suites for workflow validation functions with specific focus on nextSteps field handling and parameter extraction. Additionally, the AI communication instructions are expanded with more explicit guidance on avoiding restated workflow steps and providing actionable feedback. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In
`@services/platform/app/features/customers/components/customer-import-form.tsx`:
- Around line 136-139: The two unordered lists in the CustomerImportForm
component need an explicit role attribute for VoiceOver accessibility: update
both <ul> elements inside customer-import-form.tsx (the one rendering the
localeHint/churnedNote list and the other list around lines ~167-170) to include
role="list" so Safari/VoiceOver will announce list semantics when Tailwind v4
Preflight resets list-style; locate the <ul className="list-outside list-disc
..."> instances and add role="list" to each.
In `@services/platform/app/features/products/components/product-import-form.tsx`:
- Around line 77-80: The UL that renders expected columns in the
ProductImportForm component should include an explicit role for VoiceOver
accessibility; update the <ul className="list-outside list-disc space-y-2 pl-4">
element in product-import-form.tsx to add role="list" so Safari/VoiceOver will
announce the list semantics (keep the existing Tailwind classes and list items
unchanged).
In `@services/platform/app/features/vendors/components/vendor-import-form.tsx`:
- Around line 100-102: The two unordered lists in the VendorImportForm component
need explicit list semantics for VoiceOver: add role="list" to both <ul>
elements in the vendor-import-form.tsx JSX (the <ul className="list-outside
list-disc ..."> instances) so Safari/VoiceOver correctly announces them even
when Tailwind v4 Preflight removes list-style; update both occurrences (the one
showing t('importForm.localeHint') and the other list around line ~130) to
include role="list".
In
`@services/platform/convex/workflow_engine/helpers/validation/validate_action_parameters.test.ts`:
- Around line 156-170: The test suite contains a duplicate case checking
nextSteps contamination: the test "should reject nextSteps as unknown field in
raw parameters" duplicates the earlier "should fail with unknown fields
alongside variables"; remove the redundant test or alter it to cover a different
scenario. Locate the tests referencing validateActionParameters and the two
descriptions ("should reject nextSteps as unknown field in raw parameters" and
"should fail with unknown fields alongside variables") and either delete the
former duplicate or change its input/expectation to exercise a distinct
validation path (e.g., different action name, nested unknown field, or different
malformed parameter) so each test covers unique behavior.
In
`@services/platform/convex/workflow_engine/helpers/validation/validate_step_config.test.ts`:
- Around line 153-177: Update the test comment in validate_step_config.test.ts
to clearly state that this test documents the current validator behavior
(validateStepConfig) which requires stepType even on partial updates, and that
the actual fix is implemented upstream in update_workflow_step_tool.ts where
validation is gated to run only when both stepType and config are present;
explicitly note that the expectation (failure with "Step type is required") is
intentional and should not be changed until the upstream gating is applied so
future readers don't mistakenly flip the test.
| <ul className="list-outside list-disc space-y-2 pl-4"> | ||
| <li>{t('importForm.localeHint')}</li> | ||
| <li className="text-yellow-600">{t('importForm.churnedNote')}</li> | ||
| </Stack> | ||
| </Description> | ||
| </ul> |
There was a problem hiding this comment.
Add role="list" on both <ul> elements for VoiceOver accessibility.
Both list elements (Lines 136 and 167) need role="list" for Safari/VoiceOver to properly announce list semantics when Tailwind CSS v4 Preflight resets list-style.
Proposed fix
@@ -136,7 +136,7 @@
- <ul className="list-outside list-disc space-y-2 pl-4">
+ <ul role="list" className="list-outside list-disc space-y-2 pl-4">@@ -167,7 +167,7 @@
- <ul className="list-outside list-disc space-y-2 pl-4">
+ <ul role="list" className="list-outside list-disc space-y-2 pl-4">Based on learnings: "Tailwind CSS v4 with Preflight resets list-style to none, and Safari/VoiceOver requires an explicit role='list' for proper semantics."
Also applies to: 167-170
🤖 Prompt for AI Agents
In `@services/platform/app/features/customers/components/customer-import-form.tsx`
around lines 136 - 139, The two unordered lists in the CustomerImportForm
component need an explicit role attribute for VoiceOver accessibility: update
both <ul> elements inside customer-import-form.tsx (the one rendering the
localeHint/churnedNote list and the other list around lines ~167-170) to include
role="list" so Safari/VoiceOver will announce list semantics when Tailwind v4
Preflight resets list-style; locate the <ul className="list-outside list-disc
..."> instances and add role="list" to each.
| <ul className="list-outside list-disc space-y-2 pl-4"> | ||
| <li>{t('import.expectedColumns')}</li> | ||
| <li className="text-blue-600">{t('import.draftStatusNote')}</li> | ||
| </ul> |
There was a problem hiding this comment.
Add role="list" for VoiceOver accessibility.
Tailwind CSS v4 with Preflight resets list-style to none, which causes Safari/VoiceOver to not announce the list semantics. Add role="list" to maintain accessibility.
Proposed fix
- <ul className="list-outside list-disc space-y-2 pl-4">
+ <ul role="list" className="list-outside list-disc space-y-2 pl-4">Based on learnings: "In the tale-project/tale repository, when reviewing React components written in TypeScript (.tsx files), do not remove the role='list' attribute from ul elements. Tailwind CSS v4 with Preflight resets list-style to none, and Safari/VoiceOver requires an explicit role='list' for proper semantics."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <ul className="list-outside list-disc space-y-2 pl-4"> | |
| <li>{t('import.expectedColumns')}</li> | |
| <li className="text-blue-600">{t('import.draftStatusNote')}</li> | |
| </ul> | |
| <ul role="list" className="list-outside list-disc space-y-2 pl-4"> | |
| <li>{t('import.expectedColumns')}</li> | |
| <li className="text-blue-600">{t('import.draftStatusNote')}</li> | |
| </ul> |
🤖 Prompt for AI Agents
In `@services/platform/app/features/products/components/product-import-form.tsx`
around lines 77 - 80, The UL that renders expected columns in the
ProductImportForm component should include an explicit role for VoiceOver
accessibility; update the <ul className="list-outside list-disc space-y-2 pl-4">
element in product-import-form.tsx to add role="list" so Safari/VoiceOver will
announce the list semantics (keep the existing Tailwind classes and list items
unchanged).
| <ul className="list-outside list-disc space-y-2 pl-4"> | ||
| <li>{t('importForm.localeHint')}</li> | ||
| </ul> |
There was a problem hiding this comment.
Add role="list" on both <ul> elements for VoiceOver accessibility.
Both list elements (Lines 100 and 130) need role="list" for Safari/VoiceOver to properly announce list semantics when Tailwind CSS v4 Preflight resets list-style.
Proposed fix
@@ -100,7 +100,7 @@
- <ul className="list-outside list-disc space-y-2 pl-4">
+ <ul role="list" className="list-outside list-disc space-y-2 pl-4">@@ -130,7 +130,7 @@
- <ul className="list-outside list-disc space-y-2 pl-4">
+ <ul role="list" className="list-outside list-disc space-y-2 pl-4">Based on learnings: "Tailwind CSS v4 with Preflight resets list-style to none, and Safari/VoiceOver requires an explicit role='list' for proper semantics."
Also applies to: 130-132
🤖 Prompt for AI Agents
In `@services/platform/app/features/vendors/components/vendor-import-form.tsx`
around lines 100 - 102, The two unordered lists in the VendorImportForm
component need explicit list semantics for VoiceOver: add role="list" to both
<ul> elements in the vendor-import-form.tsx JSX (the <ul className="list-outside
list-disc ..."> instances) so Safari/VoiceOver correctly announces them even
when Tailwind v4 Preflight removes list-style; update both occurrences (the one
showing t('importForm.localeHint') and the other list around line ~130) to
include role="list".
| describe('nextSteps contamination at parameter level', () => { | ||
| it('should reject nextSteps as unknown field in raw parameters', () => { | ||
| // validateActionParameters correctly rejects unknown fields | ||
| const result = validateActionParameters('set_variables', { | ||
| variables: [{ name: 'counter', value: 0 }], | ||
| nextSteps: { success: 'next_step' }, | ||
| }); | ||
|
|
||
| expect(result.valid).toBe(false); | ||
| expect( | ||
| result.errors.some( | ||
| (e) => e.includes('Unknown field') && e.includes('nextSteps'), | ||
| ), | ||
| ).toBe(true); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Duplicate test case for nextSteps contamination.
This test (lines 157-170) tests the exact same scenario as the test at lines 40-53 ("should fail with unknown fields alongside variables"). Both validate that nextSteps alongside valid variables produces an "Unknown field" error.
Consider removing one to reduce redundancy, or differentiate them if there's a subtle distinction intended.
🤖 Prompt for AI Agents
In
`@services/platform/convex/workflow_engine/helpers/validation/validate_action_parameters.test.ts`
around lines 156 - 170, The test suite contains a duplicate case checking
nextSteps contamination: the test "should reject nextSteps as unknown field in
raw parameters" duplicates the earlier "should fail with unknown fields
alongside variables"; remove the redundant test or alter it to cover a different
scenario. Locate the tests referencing validateActionParameters and the two
descriptions ("should reject nextSteps as unknown field in raw parameters" and
"should fail with unknown fields alongside variables") and either delete the
former duplicate or change its input/expectation to exercise a distinct
validation path (e.g., different action name, nested unknown field, or different
malformed parameter) so each test covers unique behavior.
| describe('partial updates (update_workflow_step scenarios)', () => { | ||
| it('should fail when stepType is undefined but config is provided', () => { | ||
| // This is the critical bug: update_workflow_step passes stepType: undefined | ||
| // when only config is being updated, causing "Step type is required" error. | ||
| // The AI agent then retries 30 times with the same payload. | ||
| const result = validateStepConfig({ | ||
| stepSlug: 'update', | ||
| name: 'Step', | ||
| stepType: undefined, | ||
| config: { | ||
| type: 'set_variables', | ||
| parameters: { | ||
| variables: [{ name: 'foo', value: 'bar' }], | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| // Current behavior: fails with "Step type is required" | ||
| // This documents the bug - a partial update with valid config shouldn't fail | ||
| // just because stepType wasn't re-sent in the update payload | ||
| expect(result.valid).toBe(false); | ||
| expect( | ||
| result.errors.some((e) => e.includes('Step type is required')), | ||
| ).toBe(true); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify test intent: This documents current behavior, not desired behavior.
The comment says "This documents the bug" and expects failure. Per the PR description, the fix for this is in update_workflow_step_tool.ts which gates validation to run only when both stepType and config are present.
Consider adding a note that this test documents the validator's current behavior (requiring stepType), and the actual fix is upstream gating. This prevents future confusion about whether the test expectation should change.
📝 Suggested comment enhancement
describe('partial updates (update_workflow_step scenarios)', () => {
it('should fail when stepType is undefined but config is provided', () => {
// This is the critical bug: update_workflow_step passes stepType: undefined
// when only config is being updated, causing "Step type is required" error.
// The AI agent then retries 30 times with the same payload.
+ //
+ // NOTE: This test documents the validator's behavior. The fix is in
+ // update_workflow_step_tool.ts which gates validation to only run when
+ // both stepType AND config are provided.
const result = validateStepConfig({📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe('partial updates (update_workflow_step scenarios)', () => { | |
| it('should fail when stepType is undefined but config is provided', () => { | |
| // This is the critical bug: update_workflow_step passes stepType: undefined | |
| // when only config is being updated, causing "Step type is required" error. | |
| // The AI agent then retries 30 times with the same payload. | |
| const result = validateStepConfig({ | |
| stepSlug: 'update', | |
| name: 'Step', | |
| stepType: undefined, | |
| config: { | |
| type: 'set_variables', | |
| parameters: { | |
| variables: [{ name: 'foo', value: 'bar' }], | |
| }, | |
| }, | |
| }); | |
| // Current behavior: fails with "Step type is required" | |
| // This documents the bug - a partial update with valid config shouldn't fail | |
| // just because stepType wasn't re-sent in the update payload | |
| expect(result.valid).toBe(false); | |
| expect( | |
| result.errors.some((e) => e.includes('Step type is required')), | |
| ).toBe(true); | |
| }); | |
| describe('partial updates (update_workflow_step scenarios)', () => { | |
| it('should fail when stepType is undefined but config is provided', () => { | |
| // This is the critical bug: update_workflow_step passes stepType: undefined | |
| // when only config is being updated, causing "Step type is required" error. | |
| // The AI agent then retries 30 times with the same payload. | |
| // | |
| // NOTE: This test documents the validator's behavior. The fix is in | |
| // update_workflow_step_tool.ts which gates validation to only run when | |
| // both stepType AND config are provided. | |
| const result = validateStepConfig({ | |
| stepSlug: 'update', | |
| name: 'Step', | |
| stepType: undefined, | |
| config: { | |
| type: 'set_variables', | |
| parameters: { | |
| variables: [{ name: 'foo', value: 'bar' }], | |
| }, | |
| }, | |
| }); | |
| // Current behavior: fails with "Step type is required" | |
| // This documents the bug - a partial update with valid config shouldn't fail | |
| // just because stepType wasn't re-sent in the update payload | |
| expect(result.valid).toBe(false); | |
| expect( | |
| result.errors.some((e) => e.includes('Step type is required')), | |
| ).toBe(true); | |
| }); |
🤖 Prompt for AI Agents
In
`@services/platform/convex/workflow_engine/helpers/validation/validate_step_config.test.ts`
around lines 153 - 177, Update the test comment in validate_step_config.test.ts
to clearly state that this test documents the current validator behavior
(validateStepConfig) which requires stepType even on partial updates, and that
the actual fix is implemented upstream in update_workflow_step_tool.ts where
validation is gated to run only when both stepType and config are present;
explicitly note that the expectation (failure with "Step type is required") is
intentional and should not be changed until the upstream gating is applied so
future readers don't mistakenly flip the test.
1 participant
Summary
TeamFilterMenuinto its own componentForm/Descriptionwrappers with native HTML in import forms, fix vendors table cell spacingstepTypeandconfigare present, preventing infinite retry loops on partial updatesnextStepsfrom action config with a warning instead of hard failingTest plan
npm run test --workspace=@tale/platformSummary by CodeRabbit
Bug Fixes
Tests
Refactor
Style
Documentation