MM-69625 MM-69633: Collapse-all and task filtering on playbook run checklists#9928
Open
asaadmahmood wants to merge 2 commits into
Open
MM-69625 MM-69633: Collapse-all and task filtering on playbook run checklists#9928asaadmahmood wants to merge 2 commits into
asaadmahmood wants to merge 2 commits into
Conversation
The android/ gradle wrapper is on Gradle 9.0.0, but @react-native/gradle-plugin (RN 0.83.9) pins foojay-resolver-convention 0.5.0. That plugin references JvmVendorSpec.IBM_SEMERU, a field removed in Gradle 9, so it throws NoSuchFieldError while its DistributionsKt class initializes. This happens during settings evaluation, which means every ./gradlew invocation on a clean checkout fails before any task runs (not even `gradlew help` works), and `npm run android` can't build. Patch bumps foojay-resolver-convention to 1.0.0, the first release compatible with Gradle 9. (0.10.0 was tried first but still references the removed field.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ecklists
Bring two playbook-run capabilities that already exist in the webapp to mobile,
via two icons in the "Tasks" header of the run screen.
Collapse/expand all checklists
A header toggle (arrow-collapse / arrow-expand) that collapses or expands
every checklist at once. Per-checklist expand state already existed as local
state inside Checklist; rather than replace it (individual toggling is still
wanted), the run-level control is lifted into PlaybookRun and pushed down as
a `collapseAll` flag plus a monotonic `collapseAllEpoch`. The epoch is what
makes it work after a user has toggled a checklist on its own: without it,
pressing "collapse all" again when `collapseAll` hadn't changed value would
be a no-op, so each checklist re-syncs to `collapseAll` whenever the epoch
advances (via useDidUpdate).
Task filter
A filter button (filter-variant, tinted when a filter is active) opens a
bottom sheet modelled on components/files/file_filter.tsx: "All tasks", then
a Task state section (show checked / show skipped) and an Assignee section
(Me / Unassigned / Others), matching the webapp. "All tasks" is a
select-all/clear-all toggle. Filter state lives in PlaybookRun and is applied
when rendering each checklist's items.
Item index-safety fix (prerequisite for filtering)
The server mutates checklist items by their position in the checklist's
canonical order (client.setChecklistItemState(runId, checklistNum, itemNum,
...), skipChecklistItem, setAssignee, ...). checklist.tsx was rendering items
with itemNumber={renderIndex}, where renderIndex was the index into an
already-filtered array (the enhancer dropped condition-hidden items before
rendering). So whenever an item was omitted, every later item's itemNumber
shifted, and an action on one task would target a different task on the
server. This was already reachable on main via condition-hidden items;
user-facing filtering would have made it trivial to hit.
Fixed by moving all filtering to render time: the enhancer (checklist/index.ts)
now emits every item in canonical order (and additionally observes
assignee_id so the assignee filter reacts live), and checklist.tsx pairs each
item with its true index before filtering, so itemNumber is always the
server-side index regardless of what is hidden. Both the visible list and the
off-screen height-calculator copy use the same filtered set so the collapse
animation height stays correct.
Filtering logic is a pure util (utils/task_filters.ts: isItemVisible,
itemMatchesFilters, areDefaultTaskFilters) with its own unit tests; component
and screen tests cover both controls and pin the index-safety guarantee
(a filtered-out item must not shift the itemNumber of the items after it).
New i18n strings were added to en.json only, via npm run i18n-extract.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@asaadmahmood: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsI understand the commands that are listed here |
Documentation Impact Analysis — updates neededDocumentation Impact AnalysisOverall Assessment: Documentation Updates Recommended Changes SummaryThis PR adds two new user-facing controls to the Playbook Run screen in the Mattermost mobile app: a Collapse/Expand all checklists toggle and a Filter tasks bottom sheet. The filter sheet lets users narrow visible checklist items by task state (checked, skipped) and assignee (Me, Unassigned, Others). As a secondary internal change, filtering is moved from the data layer to the render layer to fix a server-index shifting bug, but this is not user-visible. Documentation Impact Details
Recommended Actions
ConfidenceMedium — The "Playbook runs on mobile" section in |
Contributor
📝 WalkthroughWalkthroughChangesPlaybook task controls
Gradle toolchain patch
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant PlaybookRun
participant TaskFilter
participant ChecklistList
participant Checklist
User->>PlaybookRun: Open task filters
PlaybookRun->>TaskFilter: Open bottom sheet
User->>TaskFilter: Change filter selection
TaskFilter->>PlaybookRun: Return updated filters
PlaybookRun->>ChecklistList: Pass filters and collapse state
ChecklistList->>Checklist: Pass checklist props
Checklist->>Checklist: Render visible items with server indexes
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/products/playbooks/screens/playbook_run/task_filter/index.tsx (1)
88-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid side effects inside state updater functions.
onFiltersChangedis called inside thesetSelectedupdater. React 18+ StrictMode double-invokes state updaters to detect impurities, so this callback fires twice per toggle.toggleAll(lines 97–101) already uses the correct pattern—computingnextfromselecteddirectly and callingonFiltersChangedoutside the updater.updateshould be consistent.♻️ Proposed refactor for consistency with toggleAll
const update = useCallback((changes: Partial<TaskFilters>) => { - setSelected((prev) => { - const next = {...prev, ...changes}; - onFiltersChanged(next); - return next; - }); -}, [onFiltersChanged]); + const next = {...selected, ...changes}; + setSelected(next); + onFiltersChanged(next); +}, [onFiltersChanged, selected]);🤖 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/task_filter/index.tsx` around lines 88 - 94, Update the update callback to compute the next TaskFilters value from the current selected state, call onFiltersChanged outside the setSelected updater, and then apply the computed value with setSelected. Keep the existing partial-change merge behavior and align the implementation with toggleAll.
🤖 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/screens/playbook_run/checklist/checklist.test.tsx`:
- Around line 503-537: Strengthen the four filter tests around Checklist by
asserting the complete rendered item ID array, not only that the filtered ID is
absent. Update the showChecked, showSkipped, showAssignedToMe, and
showAssignedToOthers cases to verify the expected array contents and length,
matching the adjacent tests’ full-result assertions.
In `@app/products/playbooks/screens/playbook_run/checklist/checklist.tsx`:
- Line 145: Initialize the checklist’s expanded state from the current
collapseAll prop instead of always defaulting to true. Update the useState
initialization near the checklist component and preserve useDidUpdate’s
synchronization behavior for subsequent collapseAll changes, so newly mounted
checklists immediately reflect the current state.
---
Nitpick comments:
In `@app/products/playbooks/screens/playbook_run/task_filter/index.tsx`:
- Around line 88-94: Update the update callback to compute the next TaskFilters
value from the current selected state, call onFiltersChanged outside the
setSelected updater, and then apply the computed value with setSelected. Keep
the existing partial-change merge behavior and align the implementation with
toggleAll.
🪄 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: 9b595e28-9a00-425d-8e32-8e3265190a3c
📒 Files selected for processing (14)
app/products/playbooks/screens/playbook_run/checklist/checklist.test.tsxapp/products/playbooks/screens/playbook_run/checklist/checklist.tsxapp/products/playbooks/screens/playbook_run/checklist/index.test.tsxapp/products/playbooks/screens/playbook_run/checklist/index.tsapp/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/screens/playbook_run/task_filter/index.test.tsxapp/products/playbooks/screens/playbook_run/task_filter/index.tsxapp/products/playbooks/utils/task_filters.test.tsapp/products/playbooks/utils/task_filters.tsassets/base/i18n/en.jsonpatches/@react-native+gradle-plugin+0.83.9.patch
Comment on lines
+503
to
+537
| it('should hide checked items when showChecked is off', () => { | ||
| const props = getFilterProps(); | ||
| props.filters = {...DEFAULT_TASK_FILTERS, showChecked: false}; | ||
|
|
||
| const {getByTestId} = renderWithIntl(<Checklist {...props}/>); | ||
|
|
||
| expect(renderedItemIds(getByTestId)).not.toContain('checked'); | ||
| }); | ||
|
|
||
| it('should hide skipped items when showSkipped is off', () => { | ||
| const props = getFilterProps(); | ||
| props.filters = {...DEFAULT_TASK_FILTERS, showSkipped: false}; | ||
|
|
||
| const {getByTestId} = renderWithIntl(<Checklist {...props}/>); | ||
|
|
||
| expect(renderedItemIds(getByTestId)).not.toContain('skipped'); | ||
| }); | ||
|
|
||
| it('should hide items assigned to the current user when Me is off', () => { | ||
| const props = getFilterProps(); | ||
| props.filters = {...DEFAULT_TASK_FILTERS, showAssignedToMe: false}; | ||
|
|
||
| const {getByTestId} = renderWithIntl(<Checklist {...props}/>); | ||
|
|
||
| expect(renderedItemIds(getByTestId)).not.toContain('mine'); | ||
| }); | ||
|
|
||
| it('should hide items assigned to other users when Others is off', () => { | ||
| const props = getFilterProps(); | ||
| props.filters = {...DEFAULT_TASK_FILTERS, showAssignedToOthers: false}; | ||
|
|
||
| const {getByTestId} = renderWithIntl(<Checklist {...props}/>); | ||
|
|
||
| expect(renderedItemIds(getByTestId)).not.toContain('theirs'); | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Filter tests only check absence, not full result set.
These four tests use .not.toContain(...) without checking length or the full expected array, unlike the adjacent tests at lines 500 and 546 that assert the complete array. As per coding guidelines, "In tests, check array lengths in addition to individual items to ensure no extra elements exist."
✅ Proposed fix: assert the full expected set
- expect(renderedItemIds(getByTestId)).not.toContain('checked');
+ expect(renderedItemIds(getByTestId)).toEqual(['unchecked', 'skipped', 'mine', 'theirs']);(similarly for the showSkipped, showAssignedToMe, and showAssignedToOthers tests)
🤖 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.test.tsx`
around lines 503 - 537, Strengthen the four filter tests around Checklist by
asserting the complete rendered item ID array, not only that the filtered ID is
absent. Update the showChecked, showSkipped, showAssignedToMe, and
showAssignedToOthers cases to verify the expected array contents and length,
matching the adjacent tests’ full-result assertions.
Source: Coding guidelines
| collapseAll, | ||
| collapseAllEpoch, | ||
| }: Props) => { | ||
| const [expanded, setExpanded] = useState(true); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Newly-mounted checklists ignore the current collapseAll state.
useDidUpdate (lines 153-156) skips the initial render, but expanded still defaults to true (line 145) regardless of collapseAll. A checklist that mounts for the first time while collapseAll is already true (e.g., a new checklist created mid-session) will render expanded until the next collapse-all toggle re-syncs it.
🔧 Proposed fix: seed initial state from the current prop
- const [expanded, setExpanded] = useState(true);
+ const [expanded, setExpanded] = useState(() => !collapseAll);Self-recovers on the next collapse/expand-all press, so impact is limited to a transient visual inconsistency.
Also applies to: 153-156
🤖 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.tsx` at line
145, Initialize the checklist’s expanded state from the current collapseAll prop
instead of always defaulting to true. Update the useState initialization near
the checklist component and preserve useDidUpdate’s synchronization behavior for
subsequent collapseAll changes, so newly mounted checklists immediately reflect
the current state.
Coverage Comparison Report |
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
2 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
Brings two playbook-run capabilities from the webapp to mobile, surfaced as two icons in the Tasks header of the run screen:
arrow-collapse/arrow-expand) that collapses or expands every checklist at once. Individual per-checklist toggling still works; a monotonic epoch counter makes the run-level control re-sync each checklist even after it has been toggled on its own.filter-variant, tinted when active) opening a bottom sheet with All tasks, a Task state section (show checked / show skipped), and an Assignee section (Me / Unassigned / Others), matching the webapp. "All tasks" is a select-all/clear-all toggle.Item index-safety fix (prerequisite for filtering)
The server mutates checklist items by their position in the checklist's canonical order (
setChecklistItemState(runId, checklistNum, itemNum, ...),skipChecklistItem,setAssignee, …).checklist.tsxwas passingitemNumber={renderIndex}from an already-filtered array (the enhancer dropped condition-hidden items), so omitting any item shifted the indices of the items after it — an action on one task could mutate a different task on the server. This was already reachable onmainvia condition-hidden items; user-facing filtering would make it trivial to trigger.Fix: the enhancer now emits every item in canonical order (and observes
assignee_idso the assignee filter reacts live), andchecklist.tsxpairs each item with its true index before filtering — soitemNumberis always the server-side index regardless of what's hidden. Both the visible list and the off-screen height-calculator copy use the same filtered set so the collapse animation height stays correct.Testing
utils/task_filters.ts) with unit tests.itemNumberof items after it).i18n
New strings added to
en.jsononly, vianpm run i18n-extract.Note on the second commit
48daae1f6is an unrelated build fix:@react-native/gradle-plugin(RN 0.83.9) pinsfoojay-resolver-convention0.5.0, which referencesJvmVendorSpec.IBM_SEMERU— removed in Gradle 9 (this repo's wrapper). Without it,./gradlewfails at settings evaluation on a clean checkout and Android can't build. Patched to foojay 1.0.0. Happy to split this into its own PR if preferred.🤖 Generated with Claude Code
Images