MM-69627: Add beta task requirements for checklist items - #2381
MM-69627: Add beta task requirements for checklist items#2381asaadmahmood wants to merge 1 commit into
Conversation
Allow playbook authors to define required text fields on tasks, collect values at checkoff during a run, and gate the feature behind System Console beta settings. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Test check 2e67666 — action needed PR adds good tests for 5 of 7 new components/behaviours, but EditRequirementsModal (a non-trivial form component) has no tests, and the critical server-side validation in ModifyCheckedState (enforcing all requirement values before close) is entirely untested. More details (truncated)Test Files Detected
AnalysisWell-covered areas:
Notable gaps:
...truncated. View full analysis details Suggestions
To override, comment |
📝 WalkthroughWalkthroughAdds task requirements to playbook checklist data, GraphQL contracts, beta-feature settings, run-state APIs, server validation, and the web checklist interface for defining, viewing, editing, and completing requirements. ChangesTask requirements
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant BetaFeaturesSetting
participant GlobalSettings
participant ChecklistItem
participant FillRequirementsModal
participant PlaybookRunAPI
Admin->>BetaFeaturesSetting: enable task_requirements
BetaFeaturesSetting->>GlobalSettings: save BetaFeatures
GlobalSettings->>ChecklistItem: expose enabled setting
ChecklistItem->>FillRequirementsModal: request requirement values
FillRequirementsModal->>PlaybookRunAPI: submit state and requirement_values
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🧹 Nitpick comments (1)
webapp/src/components/checklist_item/checklist_item.test.tsx (1)
35-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the beta-enabled checklist flow.
This fixed
falsemock prevents this suite from testing requirement modal interception and value submission. Add atrueselector case covering checkbox completion andonChange(Closed, values).🤖 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 `@webapp/src/components/checklist_item/checklist_item.test.tsx` around lines 35 - 37, Update the useAppSelector mock in the checklist item test suite to support a beta-enabled true case, then add coverage for checkbox completion that verifies requirement modal interception and invokes onChange with Closed and the submitted values. Preserve the existing false-selector coverage.
🤖 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 `@webapp/src/components/checklist_item/fill_requirements_modal.tsx`:
- Around line 19-20: Update fill_requirements_modal.tsx at lines 19-20 and 62-72
so the save callbacks return and await the asynchronous update result, retaining
the modal when the save fails. In checklist_item.tsx lines 733-742, return the
onChange promise and only close/reset the modal after the response succeeds.
---
Nitpick comments:
In `@webapp/src/components/checklist_item/checklist_item.test.tsx`:
- Around line 35-37: Update the useAppSelector mock in the checklist item test
suite to support a beta-enabled true case, then add coverage for checkbox
completion that verifies requirement modal interception and invokes onChange
with Closed and the submitted values. Preserve the existing false-selector
coverage.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1142e82e-1196-4f72-83b8-204a033ec04f
⛔ Files ignored due to path filters (2)
webapp/src/graphql/generated/gql.tsis excluded by!**/generated/**webapp/src/graphql/generated/graphql.tsis excluded by!**/generated/**
📒 Files selected for processing (35)
client/playbook.goclient/settings.goplugin.jsonserver/api/graphql_playbook.goserver/api/playbook_runs.goserver/api/schema.graphqlsserver/api/settings.goserver/app/permissions_service_test.goserver/app/playbook.goserver/app/playbook_run.goserver/app/playbook_run_service.goserver/app/task_requirements_test.goserver/config/beta_features_test.goserver/config/config.goserver/config/configuration.goserver/config/service.gowebapp/src/client.tswebapp/src/components/backstage/beta_features_setting.test.tsxwebapp/src/components/backstage/beta_features_setting.tsxwebapp/src/components/checklist/checklist_list.tsxwebapp/src/components/checklist_item/checklist_item.test.tsxwebapp/src/components/checklist_item/checklist_item.tsxwebapp/src/components/checklist_item/checklist_item_draggable.tsxwebapp/src/components/checklist_item/edit_requirements_modal.tsxwebapp/src/components/checklist_item/fill_requirements_modal.test.tsxwebapp/src/components/checklist_item/fill_requirements_modal.tsxwebapp/src/components/checklist_item/hover_menu.tsxwebapp/src/components/checklist_item/inputs.tsxwebapp/src/components/checklist_item/requirements_accordion.test.tsxwebapp/src/components/checklist_item/requirements_accordion.tsxwebapp/src/graphql/playbook.graphqlwebapp/src/index.tsxwebapp/src/selectors.tswebapp/src/types/playbook.tswebapp/src/types/settings.ts
| onSave: (values: Record<string, string>) => void; | ||
| onSaveAndComplete: (values: Record<string, string>) => void; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Propagate requirement-save errors before dismissing the modal. The callback contract discards the result of setChecklistItemState, while the parent closes immediately; failed saves silently lose the user’s work.
webapp/src/components/checklist_item/fill_requirements_modal.tsx#L19-L20: type save callbacks to return the asynchronous update result.webapp/src/components/checklist_item/fill_requirements_modal.tsx#L62-L72: await that result and keep the modal available on failure.webapp/src/components/checklist_item/checklist_item.tsx#L733-L742: return theonChangepromise and close/reset only after a successful response.
📍 Affects 2 files
webapp/src/components/checklist_item/fill_requirements_modal.tsx#L19-L20(this comment)webapp/src/components/checklist_item/fill_requirements_modal.tsx#L62-L72webapp/src/components/checklist_item/checklist_item.tsx#L733-L742
🤖 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 `@webapp/src/components/checklist_item/fill_requirements_modal.tsx` around
lines 19 - 20, Update fill_requirements_modal.tsx at lines 19-20 and 62-72 so
the save callbacks return and await the asynchronous update result, retaining
the modal when the save fails. In checklist_item.tsx lines 733-742, return the
onChange promise and only close/reset the modal after the response succeeds.
Willyfrog
left a comment
Willyfrog
left a comment
There was a problem hiding this comment.
not doing a full review, I was just curious on the accordion element. I left a comment on it
| ); | ||
| }; | ||
|
|
||
| const Accordion = styled.div` |
There was a problem hiding this comment.
I really like this, but I wonder if this shouldn't be done by Mattermost's main repo and make it available for plugins
There was a problem hiding this comment.
Ideally in the future we can just have it as a common component and export it. But I don't want this to pause this update.
3 participants
Summary
Task requirements).Jira: https://mattermost.atlassian.net/browse/MM-69627
Test plan
make check-styleand relevant unit tests (go test+ webapp jest for requirements/beta)Made with Cursor