[MM-69381] Add check/uncheck chip to playbook checklist items#9959
Open
vish9812 wants to merge 5 commits into
Open
[MM-69381] Add check/uncheck chip to playbook checklist items#9959vish9812 wants to merge 5 commits into
vish9812 wants to merge 5 commits into
Conversation
Documentation Impact Analysis — updates neededDocumentation Impact AnalysisOverall Assessment: Documentation Updates Recommended Changes SummaryThis PR adds a task activity indicator to Playbooks checklist items on mobile. When a task is checked or unchecked, a compact chip now appears on the item showing who performed the action and when; tapping it opens a detail view with the actor's avatar, name, relative time, and absolute time. The change also bumps the server database schema to v21 (adding a Documentation Impact Details
Recommended Actions
ConfidenceMedium — The "Playbook runs on mobile" section in |
Coverage Comparison Report |
Contributor
📝 WalkthroughWalkthroughThe change persists playbook-run timeline events, derives checklist task activity and actors, and displays activity chips and detail rows with localized relative and absolute times. Checklist components now receive timeline events and time-format preferences, while friendly-date formatting supports narrow relative-time output. ChangesPlaybook run activity
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PlaybookRun
participant ChecklistList
participant Checklist
participant ChecklistItem
participant TaskActivityIndicator
PlaybookRun->>ChecklistList: pass timelineEvents
ChecklistList->>Checklist: forward timelineEvents
Checklist->>ChecklistItem: forward timelineEvents
ChecklistItem->>ChecklistItem: derive activity and activityActor
ChecklistItem->>TaskActivityIndicator: render activity chip
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/database/migration/server/index.test.ts (1)
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winApply the required test naming convention consistently.
app/database/migration/server/index.test.ts#L9-L9: rename the case to start withshould.app/products/playbooks/database/models/playbook_run.test.ts#L28-L45: rename both cases to start withshould; use object cases and$labelto avoid the unused callback parameter.As per coding guidelines, tests should use
it('should...')naming and unused parameters must be removed completely.🤖 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 `@app/database/migration/server/index.test.ts` at line 9, Rename the test in app/database/migration/server/index.test.ts:9 to start with “should”. In app/products/playbooks/database/models/playbook_run.test.ts:28-45, rename both cases to start with “should”, convert their cases to objects using $label for descriptions, and remove the unused callback parameter.Source: Coding guidelines
app/products/playbooks/screens/playbook_run/checklist/checklist_item/task_activity.test.ts (1)
37-102: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest titles don't follow the
it('should...')convention.None of the titles in this file (e.g., Line 37, Line 55, Line 86) start with "should". As per path instructions,
app/**/*.test.tsguideline states: "Tests should use it('should...') naming, not test('happy path')."🤖 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 `@app/products/playbooks/screens/playbook_run/checklist/checklist_item/task_activity.test.ts` around lines 37 - 102, Rename every test title in the checklist activity suite to follow the it('should...') convention, including the parameterized tests and collision cases. Preserve each test’s existing meaning and assertions while prefixing titles with clear “should” wording.Source: Path instructions
🤖 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 `@app/products/playbooks/database/models/playbook_run.ts`:
- Around line 39-43: Update the TimelineEvent type guard around the event
validation to verify every required TimelineEvent property, including id,
creator_user_id, and playbook_run_id, alongside the existing checks before
narrowing the value. Ensure only objects satisfying the complete TimelineEvent
contract are returned as valid, or narrow the guard’s declared type if those
fields are not required.
In
`@app/products/playbooks/screens/playbook_run/checklist/checklist_item/task_activity_indicator.tsx`:
- Around line 119-176: The outer accessible View in the chip and detail-row
branches currently wraps tappable BaseChip or PressableOpacity content. Remove
that accessibility container behavior, place the accessibility label on the
non-interactive content, and assign the tappable chip/avatar its own appropriate
accessibility role and label while preserving the existing press handlers.
---
Nitpick comments:
In `@app/database/migration/server/index.test.ts`:
- Line 9: Rename the test in app/database/migration/server/index.test.ts:9 to
start with “should”. In
app/products/playbooks/database/models/playbook_run.test.ts:28-45, rename both
cases to start with “should”, convert their cases to objects using $label for
descriptions, and remove the unused callback parameter.
In
`@app/products/playbooks/screens/playbook_run/checklist/checklist_item/task_activity.test.ts`:
- Around line 37-102: Rename every test title in the checklist activity suite to
follow the it('should...') convention, including the parameterized tests and
collision cases. Preserve each test’s existing meaning and assertions while
prefixing titles with clear “should” wording.
🪄 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
Run ID: 7e6b35a8-eaa9-47d0-b736-4e46a3ab092f
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (33)
app/components/friendly_date/friendly_date.test.tsxapp/components/friendly_date/index.tsxapp/database/migration/server/index.test.tsapp/database/migration/server/index.tsapp/database/schema/server/index.tsapp/database/schema/server/test.tsapp/products/playbooks/database/models/playbook_run.test.tsapp/products/playbooks/database/models/playbook_run.tsapp/products/playbooks/database/operators/transformers/index.test.tsapp/products/playbooks/database/operators/transformers/index.tsapp/products/playbooks/database/schema/playbook_run.tsapp/products/playbooks/screens/playbook_run/checklist/checklist.test.tsxapp/products/playbooks/screens/playbook_run/checklist/checklist.tsxapp/products/playbooks/screens/playbook_run/checklist/checklist_item/checklist_item.test.tsxapp/products/playbooks/screens/playbook_run/checklist/checklist_item/checklist_item.tsxapp/products/playbooks/screens/playbook_run/checklist/checklist_item/checklist_item_bottom_sheet/checklist_item_bottom_sheet.test.tsxapp/products/playbooks/screens/playbook_run/checklist/checklist_item/checklist_item_bottom_sheet/checklist_item_bottom_sheet.tsxapp/products/playbooks/screens/playbook_run/checklist/checklist_item/checklist_item_bottom_sheet/index.test.tsxapp/products/playbooks/screens/playbook_run/checklist/checklist_item/checklist_item_bottom_sheet/index.tsapp/products/playbooks/screens/playbook_run/checklist/checklist_item/index.test.tsxapp/products/playbooks/screens/playbook_run/checklist/checklist_item/index.tsapp/products/playbooks/screens/playbook_run/checklist/checklist_item/task_activity.test.tsapp/products/playbooks/screens/playbook_run/checklist/checklist_item/task_activity.tsapp/products/playbooks/screens/playbook_run/checklist/checklist_item/task_activity_indicator.test.tsxapp/products/playbooks/screens/playbook_run/checklist/checklist_item/task_activity_indicator.tsxapp/products/playbooks/screens/playbook_run/checklist/index.test.tsxapp/products/playbooks/screens/playbook_run/checklist_list.test.tsxapp/products/playbooks/screens/playbook_run/checklist_list.tsxapp/products/playbooks/screens/playbook_run/playbook_run.test.tsxapp/products/playbooks/screens/playbook_run/playbook_run.tsxapp/products/playbooks/types/api.d.tsapp/products/playbooks/types/database/models/playbook_run.tstest/test_helper.ts
…ents Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mattermost-build
added
the
E2E/Run
vish9812
changed the title
mattermost-build
removed
the
E2E/Run
vish9812
added
2: Dev Review
mattermost-build
added
the
E2E/Run
mattermost-build
removed
the
E2E/Run
Contributor
|
Does clicking on that chip open the bottom sheet @vish9812 ? |
Author
|
@asaadmahmood clicking on both chips open the regular profile bottom sheet. Clicking on the task opens the "task" bottom sheet you see. |
mattermost-build
added
the
E2E/Run
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.ruby-version:
- Line 1: Update the Ruby version declared in .ruby-version from the EOL 3.2
series to a supported release, using Ruby 3.4 or newer. If retaining Ruby 3.2 is
required, document the exception instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
vish9812
force-pushed
the
mm-69381-playbook-task-activity
branch
from
July 24, 2026 08:12
62d1ad7 to
b451c98
Compare
mattermost-build
added
E2E/Run
Contributor
|
@asaadmahmood isn't that double chip confusing? Especially when there is an assignee and a different person checks on it. Given the space in the mobile app, shouldn't that info be accessed through the bottom sheet? i like how it looks in it and I don't see a lot of value in it being in the checklist view |
mattermost-build
removed
the
E2E/Run
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
5 participants
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.
Summary
Adds task activity support to playbook run checklist items:
task_activitymodel/schema field onPlaybookRun).Ticket Link
JIRA - https://mattermost.atlassian.net/browse/MM-69381
Related Web PR - mattermost/mattermost-plugin-playbooks#2307
Checklist
Device Information
This PR was tested on: ios and android.
Screenshots