feat(activity-feed-v2): wire TaskFormV2 into TaskModalV2 with edit prefill and error handling#4667
Merged
Merged
Conversation
…efill and error handling
Contributor
WalkthroughTaskModalV2 now supports create and edit task flows with submission callbacks, error notices, and task-form wiring. TaskFormV2 adds task tracking attributes. Messages, styling, and tests were updated to match the new modal behavior. ChangesTask modal create/edit feature
Sequence Diagram(s)sequenceDiagram
participant User
participant TaskFormV2
participant TaskModalV2
participant TaskAPI
User->>TaskFormV2: submit values
TaskFormV2->>TaskModalV2: handle submit
TaskModalV2->>TaskModalV2: diff assignees and parse due date
alt create mode
TaskModalV2->>TaskAPI: createTask(payload)
else edit mode
TaskModalV2->>TaskAPI: editTask(payload)
end
TaskAPI-->>TaskModalV2: success or error
TaskModalV2-->>User: onSubmitSuccess/onSubmitError and notice rendering
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/elements/content-sidebar/activity-feed-v2/task-modal-v2/TaskFormV2.tsx (1)
204-216: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPass the target id through
inputProps, notdataTargetId.DatePickerdoesn’t define or forward adataTargetIdprop; onlyinputPropsreaches the underlying<input>, so this selector will be dropped here. UseinputProps={{ 'data-target-id': 'TaskFormV2-dueDateInput' }}instead.🤖 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 `@src/elements/content-sidebar/activity-feed-v2/task-modal-v2/TaskFormV2.tsx` around lines 204 - 216, The TaskFormV2 DatePicker is using a non-forwarded dataTargetId prop, so the selector is being dropped. Update the DatePicker usage in TaskFormV2 to pass the target marker via inputProps instead, and keep the identifier on the underlying input using the same TaskFormV2-dueDateInput value. This change should be made where the DatePicker is rendered so the selector reaches the actual input element.
🧹 Nitpick comments (5)
src/elements/content-sidebar/activity-feed-v2/task-modal-v2/TaskFormV2.tsx (2)
113-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate assignee-diff logic with inconsistent group counting.
This independently re-implements the added/removed assignee diff that
TaskModalV2'sdiffAssigneesutil already computes for the actual submit payload. Maintaining two separate implementations risks drift, and they already differ:numassigneesadded/numgroupsaddedsplit by type, butdata-resin-numassigneesremovedisremoved.length(untyped), with nonumgroupsremovedcounterpart — removed groups get folded into the "assignees" removed count, which is inconsistent with how additions are tracked.Consider extracting a shared diff helper (or exporting
diffAssigneesfrom wherever it's defined) and reusing it here, then deriving both add/remove counts split by type for symmetry.♻️ Example fix for removed-count symmetry
- const userCount = added.filter(u => u.type === 'user').length; - const groupCount = added.filter(u => u.type === 'group').length; + const userCount = added.filter(u => u.type === 'user').length; + const groupCount = added.filter(u => u.type === 'group').length; + const removedUserCount = removed.filter(a => a.target.type === 'user').length; + const removedGroupCount = removed.filter(a => a.target.type === 'group').length; const submitDate = dueDate ? toSubmitDate(dueDate, originalDueDateTime).getTime() : undefined; return { 'data-resin-assigneesadded': added.map(u => u.value).join(','), 'data-resin-assigneesremoved': removed.map(a => a.target.id).join(','), 'data-resin-duedate': submitDate, 'data-resin-numassigneesadded': userCount, - 'data-resin-numassigneesremoved': removed.length, + 'data-resin-numassigneesremoved': removedUserCount, 'data-resin-numgroupsadded': groupCount, + 'data-resin-numgroupsremoved': removedGroupCount, 'data-resin-taskid': taskId, };🤖 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 `@src/elements/content-sidebar/activity-feed-v2/task-modal-v2/TaskFormV2.tsx` around lines 113 - 131, The assignee diff logic in TaskFormV2 is duplicated and inconsistent with TaskModalV2’s diffAssignees behavior. Reuse the shared diff helper used for submit payloads instead of recalculating added/removed users here, and make the Resin metrics derived from that same result. Ensure the counts in resinTags are symmetric for both additions and removals by type, rather than using removed.length for all removals while splitting only added counts.
78-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
taskIddefaults to empty string rather than being omitted.In create mode
TaskModalV2passestaskId={editingTask?.id}which isundefined; the default''then yieldsdata-resin-taskid=""rather than omitting the attribute. Minor analytics noise, low impact.Also applies to: 128-128
🤖 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 `@src/elements/content-sidebar/activity-feed-v2/task-modal-v2/TaskFormV2.tsx` at line 78, The TaskFormV2 default for taskId is forcing an empty string, which causes TaskModalV2 to emit a blank data-resin-taskid attribute in create mode instead of omitting it. Update TaskFormV2 (and the related TaskModalV2 usage) so taskId stays undefined when not provided, and only include the resin/task ID attribute when a real id exists.src/elements/content-sidebar/activity-feed-v2/task-modal-v2/messages.ts (1)
114-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAwkward phrasing in
groupExceedsLimitWarningMessage.The default message text mixes "can not" / "cannot" and reads as a run-on sentence: "One or more groups can not receive this task as a group size cannot exceed the limit of {max} assignees per group." Consider tightening the copy for clarity and consistency.
✏️ Proposed copy fix
groupExceedsLimitWarningMessage: { id: 'be.taskModalV2.groupExceedsLimitWarningMessage', defaultMessage: - 'One or more groups can not receive this task as a group size cannot exceed the limit of {max} assignees per group.', + 'One or more groups were not assigned this task because group size cannot exceed the limit of {max} assignees per group.', description: 'Body of the warning notice when a group assignee exceeds the per-group maximum', },🤖 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 `@src/elements/content-sidebar/activity-feed-v2/task-modal-v2/messages.ts` around lines 114 - 119, The copy in groupExceedsLimitWarningMessage is awkward and inconsistent (“can not” vs “cannot”) and should be tightened for clarity; update the defaultMessage in the messages definition to a shorter, grammatically clean warning that says the group cannot receive the task because the group size exceeds the {max} assignees limit, while keeping the same intent and placeholder.src/elements/content-sidebar/activity-feed-v2/task-modal-v2/__tests__/TaskModalV2.test.tsx (2)
16-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated test mocks/helpers across two test files.
The
UserSelectorMockPropstype, the@box/user-selectorjest.mockfactory, andbuildUserAssigneeare copied verbatim fromTaskFormV2.test.tsx. Consider extracting these into a shared test-utils module imported by both suites to avoid drift if one copy is updated and the other isn't.Also applies to: 62-69
🤖 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 `@src/elements/content-sidebar/activity-feed-v2/task-modal-v2/__tests__/TaskModalV2.test.tsx` around lines 16 - 47, The test suite has duplicated user selector mock setup, including the UserSelectorMockProps type and the `@box/user-selector` jest.mock factory, which is copied from TaskFormV2.test.tsx and can drift over time. Extract the shared mock/helpers into a common test-utils module and import them into TaskModalV2.test.tsx and TaskFormV2.test.tsx, keeping buildUserAssignee and the lastUserSelectorProps wiring centralized so both suites use the same implementation.
308-312: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMagic number "250" hardcoded in assertion.
The expected text hardcodes
250instead of referencingTASK_MAX_GROUP_ASSIGNEES, which is the value actually interpolated into the message. If the constant changes, this assertion silently goes stale rather than failing meaningfully against the same source of truth.🤖 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 `@src/elements/content-sidebar/activity-feed-v2/task-modal-v2/__tests__/TaskModalV2.test.tsx` around lines 308 - 312, The warning notice test in TaskModalV2 should not hardcode the assignee limit in the expected string. Update the assertion in the group-exceeds-limit case to use TASK_MAX_GROUP_ASSIGNEES, matching the value interpolated by TaskModalV2 and keeping the test aligned with the same source of truth. Locate the failing expectation in the TaskModalV2.test.tsx suite and replace the literal limit with the shared constant.
🤖 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
`@src/elements/content-sidebar/activity-feed-v2/task-modal-v2/__tests__/TaskModalV2.test.tsx`:
- Around line 218-220: The test is invoking the state-updating callback from
UserSelectorProps directly, which should be wrapped in act() to keep React
updates deterministic and avoid warnings. Update the TaskModalV2 test to import
act from react, then wrap each
lastUserSelectorProps.onSelectedUsersChange?.(...) call in act() at every listed
call site, following the same pattern used in TaskFormV2.test.tsx; use the
onSelectedUsersChange callback name to locate all occurrences.
In `@src/elements/content-sidebar/activity-feed-v2/task-modal-v2/TaskModalV2.tsx`:
- Around line 258-264: The InlineNotice in TaskModalV2 currently always uses
messages.createTaskErrorTitle, so edit-mode failures still show create-task
copy. Update the title selection in the TaskModalV2 error rendering path to
mirror the existing isEditMode body logic, using an edit-specific title when
isEditMode is true and the create title otherwise. Keep the change localized to
the inline notice that renders the task error message.
- Around line 282-294: TaskModalV2 is passing new initial* props into
TaskFormV2, but TaskFormV2 only uses them for its initial useState values, so
changing editingTask can leave stale form state attached to the new task. Update
the TaskModalV2 render of TaskFormV2 so the form resets whenever the edited task
changes, either by forcing a remount with a stable key tied to
editingTask/taskId or by ensuring TaskFormV2 syncs its internal state when those
props change. Keep the fix centered around TaskModalV2, TaskFormV2, and
editingTask so the form state always matches the currently edited task.
---
Outside diff comments:
In `@src/elements/content-sidebar/activity-feed-v2/task-modal-v2/TaskFormV2.tsx`:
- Around line 204-216: The TaskFormV2 DatePicker is using a non-forwarded
dataTargetId prop, so the selector is being dropped. Update the DatePicker usage
in TaskFormV2 to pass the target marker via inputProps instead, and keep the
identifier on the underlying input using the same TaskFormV2-dueDateInput value.
This change should be made where the DatePicker is rendered so the selector
reaches the actual input element.
---
Nitpick comments:
In
`@src/elements/content-sidebar/activity-feed-v2/task-modal-v2/__tests__/TaskModalV2.test.tsx`:
- Around line 16-47: The test suite has duplicated user selector mock setup,
including the UserSelectorMockProps type and the `@box/user-selector` jest.mock
factory, which is copied from TaskFormV2.test.tsx and can drift over time.
Extract the shared mock/helpers into a common test-utils module and import them
into TaskModalV2.test.tsx and TaskFormV2.test.tsx, keeping buildUserAssignee and
the lastUserSelectorProps wiring centralized so both suites use the same
implementation.
- Around line 308-312: The warning notice test in TaskModalV2 should not
hardcode the assignee limit in the expected string. Update the assertion in the
group-exceeds-limit case to use TASK_MAX_GROUP_ASSIGNEES, matching the value
interpolated by TaskModalV2 and keeping the test aligned with the same source of
truth. Locate the failing expectation in the TaskModalV2.test.tsx suite and
replace the literal limit with the shared constant.
In `@src/elements/content-sidebar/activity-feed-v2/task-modal-v2/messages.ts`:
- Around line 114-119: The copy in groupExceedsLimitWarningMessage is awkward
and inconsistent (“can not” vs “cannot”) and should be tightened for clarity;
update the defaultMessage in the messages definition to a shorter, grammatically
clean warning that says the group cannot receive the task because the group size
exceeds the {max} assignees limit, while keeping the same intent and
placeholder.
In `@src/elements/content-sidebar/activity-feed-v2/task-modal-v2/TaskFormV2.tsx`:
- Around line 113-131: The assignee diff logic in TaskFormV2 is duplicated and
inconsistent with TaskModalV2’s diffAssignees behavior. Reuse the shared diff
helper used for submit payloads instead of recalculating added/removed users
here, and make the Resin metrics derived from that same result. Ensure the
counts in resinTags are symmetric for both additions and removals by type,
rather than using removed.length for all removals while splitting only added
counts.
- Line 78: The TaskFormV2 default for taskId is forcing an empty string, which
causes TaskModalV2 to emit a blank data-resin-taskid attribute in create mode
instead of omitting it. Update TaskFormV2 (and the related TaskModalV2 usage) so
taskId stays undefined when not provided, and only include the resin/task ID
attribute when a real id exists.
🪄 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: 9881b5ff-a45a-468c-8f1d-7c432ddea16d
⛔ Files ignored due to path filters (1)
i18n/en-US.propertiesis excluded by!i18n/**
📒 Files selected for processing (6)
src/elements/content-sidebar/activity-feed-v2/task-modal-v2/TaskFormV2.tsxsrc/elements/content-sidebar/activity-feed-v2/task-modal-v2/TaskModalV2.scsssrc/elements/content-sidebar/activity-feed-v2/task-modal-v2/TaskModalV2.tsxsrc/elements/content-sidebar/activity-feed-v2/task-modal-v2/__tests__/TaskFormV2.test.tsxsrc/elements/content-sidebar/activity-feed-v2/task-modal-v2/__tests__/TaskModalV2.test.tsxsrc/elements/content-sidebar/activity-feed-v2/task-modal-v2/messages.ts
- Move error notice into its own component plus tests - Add types.ts; rename RuntimeAssignee to TaskAssignee - Key TaskFormV2 on editing task id to reset state on switch - Wrap onSelectedUsersChange in act() in TaskModalV2 tests - Rename createTaskErrorTitle to taskErrorTitle for clarity
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/elements/content-sidebar/activity-feed-v2/task-modal-v2/__tests__/TaskModalV2.test.tsx (1)
57-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
buildUserAssigneeduplicated across test files.This helper is identical to the one in
TaskFormV2.test.tsx(lines 57-64 there). Consider extracting a shared test-fixtures util for thetask-modal-v2test suite to avoid maintaining two copies in lockstep.🤖 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 `@src/elements/content-sidebar/activity-feed-v2/task-modal-v2/__tests__/TaskModalV2.test.tsx` around lines 57 - 64, The helper buildUserAssignee is duplicated in this test file and TaskFormV2.test.tsx, so extract the shared TaskAssignee fixture into a common task-modal-v2 test utility and update both suites to import it. Keep the existing shape and defaults used by buildUserAssignee, and replace the local definition with the shared helper so the two test files stay in sync.src/elements/content-sidebar/activity-feed-v2/task-modal-v2/TaskErrorNotice.tsx (1)
19-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication between
getErrorStatusandgetErrorCode.Both helpers follow the identical "check direct field, else fall back to
response.<field>" shape. Could be collapsed into a single generic helper, but the duplication is small and localized.🤖 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 `@src/elements/content-sidebar/activity-feed-v2/task-modal-v2/TaskErrorNotice.tsx` around lines 19 - 34, The `getErrorStatus` and `getErrorCode` helpers in `TaskErrorNotice` duplicate the same direct-field-then-`response` fallback logic; consider extracting that pattern into one small generic helper and reuse it for both status and code lookups. Keep the existing behavior intact while reducing the repeated shape in these two functions.
🤖 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
`@src/elements/content-sidebar/activity-feed-v2/task-modal-v2/__tests__/TaskModalV2.test.tsx`:
- Around line 57-64: The helper buildUserAssignee is duplicated in this test
file and TaskFormV2.test.tsx, so extract the shared TaskAssignee fixture into a
common task-modal-v2 test utility and update both suites to import it. Keep the
existing shape and defaults used by buildUserAssignee, and replace the local
definition with the shared helper so the two test files stay in sync.
In
`@src/elements/content-sidebar/activity-feed-v2/task-modal-v2/TaskErrorNotice.tsx`:
- Around line 19-34: The `getErrorStatus` and `getErrorCode` helpers in
`TaskErrorNotice` duplicate the same direct-field-then-`response` fallback
logic; consider extracting that pattern into one small generic helper and reuse
it for both status and code lookups. Keep the existing behavior intact while
reducing the repeated shape in these two functions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 695984a1-10fd-40ef-9f34-e2fc671eef26
⛔ Files ignored due to path filters (1)
i18n/en-US.propertiesis excluded by!i18n/**
📒 Files selected for processing (11)
src/elements/content-sidebar/activity-feed-v2/task-modal-v2/TaskErrorNotice.tsxsrc/elements/content-sidebar/activity-feed-v2/task-modal-v2/TaskFormV2.tsxsrc/elements/content-sidebar/activity-feed-v2/task-modal-v2/TaskModalV2.scsssrc/elements/content-sidebar/activity-feed-v2/task-modal-v2/TaskModalV2.tsxsrc/elements/content-sidebar/activity-feed-v2/task-modal-v2/__tests__/TaskErrorNotice.test.tsxsrc/elements/content-sidebar/activity-feed-v2/task-modal-v2/__tests__/TaskFormV2.test.tsxsrc/elements/content-sidebar/activity-feed-v2/task-modal-v2/__tests__/TaskModalV2.test.tsxsrc/elements/content-sidebar/activity-feed-v2/task-modal-v2/messages.tssrc/elements/content-sidebar/activity-feed-v2/task-modal-v2/types.tssrc/elements/content-sidebar/activity-feed-v2/task-modal-v2/utils/__tests__/contactMapping.test.tssrc/elements/content-sidebar/activity-feed-v2/task-modal-v2/utils/contactMapping.ts
✅ Files skipped from review due to trivial changes (1)
- src/elements/content-sidebar/activity-feed-v2/task-modal-v2/utils/tests/contactMapping.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/elements/content-sidebar/activity-feed-v2/task-modal-v2/TaskModalV2.scss
- src/elements/content-sidebar/activity-feed-v2/task-modal-v2/tests/TaskFormV2.test.tsx
- src/elements/content-sidebar/activity-feed-v2/task-modal-v2/messages.ts
- src/elements/content-sidebar/activity-feed-v2/task-modal-v2/TaskFormV2.tsx
- src/elements/content-sidebar/activity-feed-v2/task-modal-v2/TaskModalV2.tsx
zhirongwang
approved these changes
Jun 30, 2026
jmcbgaston
approved these changes
Jun 30, 2026
jpan-box
approved these changes
Jul 1, 2026
Contributor
Merge Queue Status
This pull request spent 11 seconds in the queue, including 1 second running CI. Required conditions to merge
|
This was referenced Jul 2, 2026
This was referenced Jul 10, 2026
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
4 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
Wires
TaskFormV2(from PR #4662) intoTaskModalV2(S-1 shell) with edit-mode prefill, footer buttons, error handling, and analytics. This is the last piece before V2 can be wired intoActivityFeedV2in a follow-up PR.What lands
<TaskFormV2>inside<Modal.Body>, footer buttons inside<Modal.Footer>, error notice above the formmode: 'edit'requireseditingTask,editingAssignees, andeditTasktogether. Eliminates the "consumer forgot editTask" silent fallthrough.editingTask: description, completion rule, due date, assigneesaddedAssignees/removedAssigneesfor the v1editTaskpayload shape; create mode calls the existing positionalcreateTaskcallback{max}placeholder usingTASK_MAX_GROUP_ASSIGNEESerror.status/error.codeANDerror.response?.status/error.response?.data?.codeisSubmittingsets the form'sisDisabled, the submit button'sloading, and disables CancelcreateTask/editTask: synchronous throws route tohandleErrorso the modal recoversdue_atguard: unparseable strings fall back tonullrather than crashingfromDatedata-resin-component="taskmodalv2"on Modal.Content; Cancel/Submit buttons carrydata-resin-target(reusing v1'sAPPROVAL_FORM_CANCEL/APPROVAL_FORM_POSTconstants) anddata-target-id.TaskFormV2form root carries v1'saddResinInfopayload plus per-fielddata-target-id.Test plan
62/62 tests pass across the
task-modal-v2module (S-1 shell + S-2 mapper + S-3 form + S-4 wiring).Behavioral coverage on the modal:
isOpen, close button, Escape, CancelcreateTaskcalled with the expected positional payloadeditTaskcalled with the diffedaddedAssignees/removedAssigneesonSubmitSuccess/onSubmitErrorinvoked appropriately{max}interpolation)error.response.status) correctly trigger the same notices as direct-shape errorscreateTaskroutes throughonSubmitErrorand re-enables Canceldue_atfalls back to empty datecreateTaskwithtype: 'group'isDisabledcascades to cancel button, form fields, user-selector mockOut of scope
TaskModalV2intoActivityFeedV2.tsx(S-5)Follow-ups
ActivityFeedV2.tsxto renderTaskModalV2(next PR)Summary by CodeRabbit
New Features
Bug Fixes
Style