Add blocks to interactive dialogs - #10024
Open
larkox wants to merge 4 commits into
Open
Conversation
Documentation Impact Analysis — updates neededDocumentation Impact AnalysisOverall Assessment: Documentation Updates Recommended Changes SummaryThis PR adds "Blocks" support to Interactive Dialogs on mobile (covering all block-based element types including file input, which was previously unsupported on mobile), adds Input Blocks to interactive messages/posts, and modernizes the look and feel of existing interactive dialog forms. A minimum server version of 11.11.0 is required for the new block action APIs ( Documentation Impact Details
Recommended Actions
ConfidenceMedium — The PR introduces clearly new integration-facing functionality (blocks in interactive dialogs, input blocks in interactive posts, file input on mobile) that is documented at a high level in the existing integration guides. The exact RST pages that need updates are identified from search results. Confidence is medium rather than high because the related server-side PR (mattermost/mattermost#37767) may own the primary developer documentation update; the mobile docs impact is focused on mobile-specific availability and the new minimum server version. |
Coverage Comparison Report |
Contributor
📝 WalkthroughWalkthroughThis change adds native ChangesMM Blocks form contracts and translation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant InteractiveMessages
participant BlockRenderer
participant BlocksDialogShell
participant IntegrationClient
participant IntegrationServer
User->>InteractiveMessages: Trigger block action
InteractiveMessages->>BlockRenderer: Render blocks and form state
BlockRenderer->>BlocksDialogShell: Submit action parameters and form values
BlocksDialogShell->>IntegrationClient: Send typed block-action request
IntegrationClient->>IntegrationServer: POST block action
IntegrationServer-->>IntegrationClient: Return errors, refresh, navigation, or dialog response
IntegrationClient-->>BlocksDialogShell: Return action response
BlocksDialogShell-->>User: Update fields, dialog, navigation, or errors
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
detox/e2e/support/mm_blocks_test_helper.ts (1)
133-171: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDeclare the new Detox testIDs in page objects. All four sites build
by.id()matchers from raw string literals instead of page-object constants.InteractiveDialogScreenalready exposes atestIDmap, and the mm_blocks field testIDs have no page-object home yet.
detox/e2e/support/mm_blocks_test_helper.ts#L133-L171: replaceby.id('interactive_dialog.scroll_view')at lines 135, 163, 166, and 167 withInteractiveDialogScreen.testID.scrollView.detox/e2e/support/mm_blocks_test_helper.ts#L318-L332: replace the same literal at lines 325 and 328 withInteractiveDialogScreen.testID.scrollView.detox/e2e/support/mm_blocks_test_helper.ts#L688-L706: addmm_blocks_text_input.screen,mm_blocks_text_input.input, andmm_blocks.text_input.save.buttonto a page object and reference those constants.detox/e2e/test/products/channels/interactive_messages/mm_blocks_dialog_stacking.e2e.ts#L29-L30: delete the localSUBMIT_BUTTONandCLOSE_BUTTONconstants and useInteractiveDialogScreen.testID; build thechild_inputmatcher from a page-object helper.As per path instructions: "Never hardcode strings in
by.id(); use page-object testID constants 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 `@detox/e2e/support/mm_blocks_test_helper.ts` around lines 133 - 171, Replace every raw interactive dialog scroll-view ID in detox/e2e/support/mm_blocks_test_helper.ts at ranges 133-171 and 318-332 with InteractiveDialogScreen.testID.scrollView; at 688-706, add page-object constants for mm_blocks_text_input.screen, mm_blocks_text_input.input, and mm_blocks.text_input.save.button and use them in by.id matchers. In detox/e2e/test/products/channels/interactive_messages/mm_blocks_dialog_stacking.e2e.ts at 29-30, remove local SUBMIT_BUTTON and CLOSE_BUTTON constants, use InteractiveDialogScreen.testID values, and build the child_input matcher through a page-object helper. Do not leave hardcoded by.id strings.Source: Path instructions
app/components/settings/label.tsx (1)
18-21: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPreserve
RadioSettinglabel alignment.
RadioSettingrendersLabelabove entries withpaddingHorizontal: 16. RemovingmarginLeft: 15shifts the label to the container edge and misaligns it. Restore the margin or add a consumer-specific style for mm_blocks labels.🤖 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/components/settings/label.tsx` around lines 18 - 21, Update the Label styling in the theme configuration to restore the 15px left margin, or apply an equivalent consumer-specific style for mm_blocks labels, so labels rendered by RadioSetting align with entries using paddingHorizontal: 16.
🟡 Minor comments (15)
app/actions/remote/integrations.ts-120-121 (1)
120-121: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLocalize or suppress the user-facing text of this error.
Consumers display
error.messageto the user.app/components/post_list/post/body/content/interactive_messages/index.tsxsetssetActionError(message ?? actionFailedMessage)withmessage = error.message, andBlocksDialogShelldoes the same. The hardcoded English string at Line 121 therefore reaches the UI untranslated. Return an error without a display message, so the consumer falls back to its localized message, or add a translated message withdefineMessages()at the consumer.As per coding guidelines: "Define new messages with
defineMessages()and runnpm run i18n-extractto updateen.json."🤖 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/actions/remote/integrations.ts` around lines 120 - 121, Update the doBlockAction-unavailable branch to avoid exposing the hardcoded English Error message through error.message; return an error without a display message so consumers such as BlocksDialogShell and the interactive message component use their localized fallback. Keep the existing debug log and availability behavior unchanged.Source: Coding guidelines
app/components/block_renderer/mm_blocks_file_upload.test.tsx-42-43 (1)
42-43: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse an ICU plural for the upload-limit warning. Update
fileMaxWarning()inapp/utils/file/index.ts,assets/base/i18n/en.json, andSINGLE_FILE_WARNINGso count1renders “Uploads limited to 1 file maximum.” Define the message withdefineMessages()and runnpm run i18n-extract.🤖 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/components/block_renderer/mm_blocks_file_upload.test.tsx` around lines 42 - 43, Update fileMaxWarning() in app/utils/file/index.ts to use a defineMessages() ICU plural message, with count 1 rendered as “Uploads limited to 1 file maximum.” Update the matching English translation in assets/base/i18n/en.json and revise SINGLE_FILE_WARNING to the singular wording, then run npm run i18n-extract.Source: Coding guidelines
app/components/settings/bool_setting.tsx-138-145 (1)
138-145: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
valuecan beundefinedin the testID and inSwitch.
valueis an optional prop. When a caller omits it, the testID becomes<testID>.toggled.undefined.button, andSwitchbecomes uncontrolled. Normalize the value once.🐛 Proposed fix
+ const checked = value === true; ... <Switch disabled={disabled} onValueChange={onChange} - value={value} + value={checked} trackColor={trackColor} thumbColor={thumbColor} - testID={`${testID}.toggled.${value}.button`} + testID={`${testID}.toggled.${checked}.button`} />Use
checkedforthumbColoras well.🤖 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/components/settings/bool_setting.tsx` around lines 138 - 145, Normalize the optional value once in the BoolSetting component, then use the normalized boolean for the Switch value, the toggled testID, and thumbColor via the checked state. Keep the existing behavior unchanged when value is provided, while ensuring omitted value never produces undefined or an uncontrolled Switch.app/components/settings/bool_setting.tsx-117-126 (1)
117-126: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPass
optionalexplicitly inuseBooleanPropThe component-library caller omits
optional, soBoolSettingdefaults tofalseand displays the red required marker. Set the intended value explicitly.🤖 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/components/settings/bool_setting.tsx` around lines 117 - 126, Update the component-library caller of BoolSetting to pass optional explicitly as true through useBooleanProp, ensuring the setting renders the optional label instead of the required marker. Keep the existing BoolSetting rendering logic unchanged.app/components/block_renderer/translation/block_kit.ts-160-162 (1)
160-162: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard invalid datetime values before formatting.
A finite
initial_date_timecan create an invalidDate, sotoISOString()throwsRangeError.translatePostPropscatches this error but replaces the full translation with error blocks. Checkdate.getTime()before formatting, and add a regression test for an oversized finite timestamp.🤖 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/components/block_renderer/translation/block_kit.ts` around lines 160 - 162, Update the initial_date_time handling in translatePostProps to create the Date, validate date.getTime() is finite before calling toISOString(), and skip initial_value assignment for invalid dates. Add a regression test covering an oversized finite timestamp and verify translation returns normally without error blocks.app/components/block_renderer/datetime_input_element/datetime_input_element.tsx-122-137 (1)
122-137: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPass
displayTimezonetoFormattedDate.
FormattedDateacceptstimezone. Without it, the date uses the device timezone whileFormattedTimeusesdisplayTimezone. Near midnight, the two values can show different dates.🤖 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/components/block_renderer/datetime_input_element/datetime_input_element.tsx` around lines 122 - 137, Update the FormattedDate component in the selectedDate display to pass the existing displayTimezone through its timezone prop, matching the timezone already supplied to FormattedTime so both values represent the same date and time zone.app/components/block_renderer/date_input_element/date_input_element.test.tsx-50-68 (1)
50-68: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winShared
onActionmock and database hooks in the new date and datetime suites. Both suites declare oneonActionmock and never reset it, and both create the server database inbeforeAllinstead ofbeforeEach. A later test in each suite assertsexpect(onAction).not.toHaveBeenCalled()after an earlier test called it, so each suite depends on global Jest mock-clearing configuration.
app/components/block_renderer/date_input_element/date_input_element.test.tsx#L50-L68: add abeforeEachthat callsjest.clearAllMocks()and restores theDateTimeSelectormock implementation, and move the database setup and teardown tobeforeEach/afterEach.app/components/block_renderer/datetime_input_element/datetime_input_element.test.tsx#L51-L69: apply the samebeforeEachreset and the same database lifecycle change.As per coding guidelines: "Use real in-memory LokiJS databases with
autosave: false, initialize them inbeforeEach, and destroy server databases inafterEach."🤖 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/components/block_renderer/date_input_element/date_input_element.test.tsx` around lines 50 - 68, In app/components/block_renderer/date_input_element/date_input_element.test.tsx lines 50-68, add a beforeEach that clears Jest mocks and restores the DateTimeSelector mock implementation, and move database setup from beforeAll to beforeEach and teardown from afterAll to afterEach. Apply the identical changes in app/components/block_renderer/datetime_input_element/datetime_input_element.test.tsx lines 51-69, preserving real in-memory LokiJS databases with autosave disabled.Source: Coding guidelines
detox/e2e/test/products/channels/interactive_messages/mm_blocks_form_inputs.e2e.ts-534-548 (1)
534-548: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winConfirm the swallowed picker error with failure artifacts.
The
catchblock hides a real Android picker failure and lets the test continue with the default date. The assertion at line 563 then passes on a date the test never set. Record the evidence that motivated this branch, or split the Android path into an explicit platform-specific flow instead of a silent catch.As per coding guidelines: "Do not add retry loops, second taps, swallowed exceptions, or increased timeouts to address a failure without completing the artifact-reading checklist and obtaining evidence."
🤖 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 `@detox/e2e/test/products/channels/interactive_messages/mm_blocks_form_inputs.e2e.ts` around lines 534 - 548, Update the date-picker setup around nativeDateTimePicker.setDatePickerDate to avoid silently swallowing Android failures: either capture and preserve failure artifacts before allowing the default-date confirmation path, or split Android into an explicit platform-specific flow that does not rely on a swallowed exception. Keep the existing iOS dismissal behavior and ensure the assertion cannot pass without verifying the intended date was set.Source: Coding guidelines
app/components/block_renderer/form_validation.ts-193-195 (1)
193-195: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign the URL subtype check with the legacy dialog check.
includes('http://')accepts a value where the scheme is not at the start, for exampleftp://host/?next=http://x. The legacy validator inapp/utils/integrations.ts(line 197) usesstartsWithfor the same subtype. Use the same rule so both dialog paths agree.🐛 Proposed fix
- if (field.subtype === 'url' && !stringValue.includes('http://') && !stringValue.includes('https://')) { + if (field.subtype === 'url' && !stringValue.startsWith('http://') && !stringValue.startsWith('https://')) { return dialogFieldErrorMessages.badUrl; }🤖 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/components/block_renderer/form_validation.ts` around lines 193 - 195, Update the URL validation condition in the field validation flow to require the value to start with either “http://” or “https://” using startsWith, matching the legacy validator in integrations.ts. Replace the current includes checks while preserving the existing badUrl error response.app/components/block_renderer/mm_blocks_file_upload.tsx-306-325 (1)
306-325: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDuplicate
clientIdvalues can collide when the same file is picked twice.
clientIdfalls back tofile.localPath ?? file.name. In multi-select mode the same file can be added twice. Two rows then share oneclientId.patchFileupdates both rows,withoutFileremoves both rows, and React renders duplicate keys at Line 408. Add a uniqueness guard or suffix.🐛 Proposed fix
const added: UploadFileState[] = []; for (const file of toUpload) { - const clientId = file.clientId ?? file.localPath ?? file.name; + const baseId = file.clientId ?? file.localPath ?? file.name; + let clientId = baseId; + let suffix = 1; + while (sourceFilesRef.current.has(clientId) || filesRef.current.some((f) => f.clientId === clientId)) { + clientId = `${baseId}-${suffix++}`; + } sourceFilesRef.current.set(clientId, file);🤖 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/components/block_renderer/mm_blocks_file_upload.tsx` around lines 306 - 325, Update the file-ID generation in the upload handling loop before `sourceFilesRef.current.set` and `added.push` so each newly added file receives a unique `clientId`, including repeated selections of the same file in multi-select mode. Preserve existing IDs when unused, but append a deterministic suffix or otherwise resolve collisions against current source files and files being added; use the resolved ID consistently for `sourceFilesRef`, the `UploadFileState`, and `startUpload`.app/components/settings/radio_setting/radio_entry.tsx-106-118 (1)
106-118: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a visual disabled state.
disabledblocks the press, but the row keeps full opacity and the normal text colour. Users cannot see that the option is not selectable. Other disabled controls in this PR are greyed out. Apply a dimmed style whendisabledis true.🎨 Proposed fix
text: { flex: 1, color: theme.centerChannelColor, ...typography('Body', 200), }, + disabled: { + opacity: 0.32, + },- <View style={style.container}> + <View style={[style.container, disabled && style.disabled]}> <Text style={style.text}>{text}</Text> {indicator} </View>🤖 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/components/settings/radio_setting/radio_entry.tsx` around lines 106 - 118, Update the row rendered by the radio entry component so its visual style changes when disabled: apply the established dimmed/greyed styling to the container or text while preserving the normal styling when enabled. Use the existing disabled prop and style symbols rather than changing the press behavior.detox/e2e/test/products/channels/interactive_messages/mm_blocks_dialog.e2e.ts-34-51 (1)
34-51: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign the per-test navigation with the standard E2E structure.
setupChannelTest()handlesSetup.apiInit(siteOneUrl)and the initial channel-list assertion. AddChannelListScreen.toBeVisible()tobeforeEach, return to the channel list inafterEach, and reopentestChannelbefore each test.🤖 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 `@detox/e2e/test/products/channels/interactive_messages/mm_blocks_dialog.e2e.ts` around lines 34 - 51, Update the suite lifecycle around beforeEach and afterEach: in beforeEach, assert ChannelListScreen.toBeVisible() and reopen testChannel after assertSuiteRunnable(); in afterEach, navigate back to the channel list instead of only ensuring the channel screen, while preserving blocks-dialog dismissal and existing setupChannelTest behavior.Source: Path instructions
app/screens/dialog_router/blocks_dialog_router.test.tsx-63-76 (1)
63-76: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove the explicit
undefinedargument from both assertions.React 19 invokes
mockBlocksDialogShellwith only the props argument. The extra argument causestoHaveBeenCalledWithto fail.🤖 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/screens/dialog_router/blocks_dialog_router.test.tsx` around lines 63 - 76, Update both mockBlocksDialogShell assertions to call toHaveBeenCalledWith with only the expected props object, removing the explicit undefined argument so they match React 19’s single-argument invocation.detox/e2e/support/ui/screen/interactive_dialog.ts-12-42 (1)
12-42: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the modal text input testID in
fillTextElement().
textInputFieldTestID()generatesmm_blocks.text_input.${elementName}.input, butMmBlocksTextInputrendersmm_blocks_text_input.input. Update the helper so text-field actions can find the modal input.🤖 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 `@detox/e2e/support/ui/screen/interactive_dialog.ts` around lines 12 - 42, Update textInputFieldTestID in the interactive dialog screen helper to return the modal input testID rendered by MmBlocksTextInput, using the mm_blocks_text_input.input identifier instead of the current textInputTestID-based value so fillTextElement() can locate the field.Source: Path instructions
detox/utils/webhook_utils.js-444-446 (1)
444-446: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLower-case
searchTextbefore the comparison.The option text and value are lower-cased, but
searchTextis not. A query such asAlphamatches nothing. The new helpergetMmBlocksLookupOptionsat Line 1715 lower-cases the search term, so the two lookup paths behave differently.🐛 Proposed fix
- const filteredOptions = searchText ?baseOptions.filter((option) => - option.text.toLowerCase().includes(searchText) || - option.value.toLowerCase().includes(searchText)) :baseOptions.slice(0, 6); // Limit to first 6 if no search + const search = String(searchText || '').toLowerCase(); + const filteredOptions = search ? baseOptions.filter((option) => + option.text.toLowerCase().includes(search) || + option.value.toLowerCase().includes(search)) : baseOptions.slice(0, 6); // Limit to first 6 if no search🤖 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 `@detox/utils/webhook_utils.js` around lines 444 - 446, Update the filteredOptions logic in getMmBlocksLookupOptions to normalize searchText to lowercase before comparing it with the lowercased option.text and option.value, preserving case-insensitive matching consistently with the other lookup path.
🤖 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/components/autocomplete_selector/index.tsx`:
- Around line 342-381: Update the chip removal handlers in the selection
rendering and the related callback near the navigation guard to block removal
when disabled is true. Ensure the callback used by SelectedUserChipById,
SelectedChannelChip, and SelectedChip includes disabled in its dependency array
and preserves removal behavior when enabled.
In `@app/components/block_renderer/bool_input_element.tsx`:
- Around line 27-29: Update the useEffect in the boolean input component to
return immediately when element.name is blank before calling setDefaultValue,
while preserving normal default registration for valid names. Add a regression
test covering an invalid-name field followed by another field action and assert
that formValues never contains an empty-string key.
In `@app/components/block_renderer/date_input_element/date_input_element.tsx`:
- Around line 43-44: Update the date input action flow around
interactionsDisabled and element.disabled so disabled elements return before
calling setValue, while preserving the existing allowed setValue behavior when
only interactionsDisabled is active. Extend DateTimeSelector to accept and
propagate disabled state to its controls, then add coverage verifying disabled
date elements do not update their value.
In `@app/components/block_renderer/file_input_element.tsx`:
- Around line 80-82: Update FileInputElement’s lifecycle handling around
handlePendingChange to clear the field’s uploading state on unmount and whenever
element.name changes: invoke setFieldUploading for the current field with false
in a cleanup effect, while preserving the existing pending-change behavior.
In `@app/components/block_renderer/form/mm_blocks_form.tsx`:
- Around line 56-77: Update setValue and setDefaultValue to compute each next
values map before calling setValues, assign it to valuesRef.current immediately,
and pass the computed map to the state update while preserving no-op checks.
Remove ref mutations from the setValues updater functions so getValues returns
the latest values synchronously and React updater purity is maintained; rely on
the existing render-time ref assignment for alignment after commits.
In `@app/components/block_renderer/select_input_element.tsx`:
- Around line 71-73: The multi-select array is recreated on every render,
causing unnecessary label lookups. In
app/components/block_renderer/select_input_element.tsx lines 71-73, memoize
multiValue so its identity changes only when the underlying raw values or
initial option change; in app/components/autocomplete_selector/index.tsx lines
315-340, key the selectedValues memo and related effect on a joined
representation of the values rather than the array reference.
In `@app/managers/integrations_manager.ts`:
- Around line 62-67: Use a unique per-navigation key when storing the dialog
config in integrations_manager.ts, and pass that key with the title to
Screens.DIALOG_ROUTER. In app/routes/(modals)/dialog_router.tsx, read the same
key and remove only that CallbackStore entry during cleanup instead of removing
the unkeyed global callback; update both affected sites accordingly.
In `@app/screens/navigation.ts`:
- Around line 101-110: Update dismissMmBlocksExpandedContentIfOpen in
app/screens/navigation.ts (lines 101-110) to stop when router.canGoBack() is
false and enforce an iteration cap before calling navigateBack. Add a test in
app/screens/navigation.test.ts (lines 308-348) where getVisibleScreen always
returns Screens.MM_BLOCKS_CONTENT and router.canGoBack returns false, asserting
the promise resolves without calling router.back.
In `@app/utils/dialog_utils.ts`:
- Around line 33-34: Update the Apps Form fallback conversion to filter out
elements whose FILE and ACTION_BUTTON types are defined in the dialog element
constants before converting unsupported elements to text fields. Preserve all
other fallback elements and their existing conversion behavior.
In
`@detox/e2e/test/products/channels/interactive_dialog/interactive_dialog_plugin.e2e.ts`:
- Around line 366-369: Replace all hardcoded interactive-dialog testID strings
used by by.id() throughout this test, including the boolean inputs and the
referenced ranges, with the corresponding InteractiveDialogScreen builders:
boolInputTestID, selectButtonTestID, radioOptionTestID, textInputFieldTestID,
dateInputTestID, and dateTimeInputTestID. Replace scroll-view IDs with
InteractiveDialogScreen.scrollView, preserving the existing assertions and
interactions.
In
`@detox/e2e/test/products/channels/interactive_messages/mm_blocks_dialog_datetime.e2e.ts`:
- Around line 71-73: Replace all hardcoded MM Blocks testID strings in by.id()
with exported constants from the interactive dialog page object. Add constants
to interactive_dialog.ts for the date/datetime, file and error, select, text,
and boolean input IDs, then update
detox/e2e/test/products/channels/interactive_messages/mm_blocks_dialog_datetime.e2e.ts
lines 71-73 and 107-108, mm_blocks_dialog_files.e2e.ts lines 71-93, and
mm_blocks_dialog_multistep.e2e.ts lines 77-88 and 103-115; no affected literal
should remain in these suites.
In
`@detox/e2e/test/products/channels/interactive_messages/mm_blocks_form_inputs.e2e.ts`:
- Around line 36-53: Move the testID definitions used by nativeDateTimePicker,
field, and the additional by.id() calls in the test into the relevant exported
page-object constants under support/ui/screen. Update this test to import and
reference those constants instead of constructing matchers from hardcoded ID
strings, preserving each existing selector and behavior.
---
Outside diff comments:
In `@app/components/settings/label.tsx`:
- Around line 18-21: Update the Label styling in the theme configuration to
restore the 15px left margin, or apply an equivalent consumer-specific style for
mm_blocks labels, so labels rendered by RadioSetting align with entries using
paddingHorizontal: 16.
In `@detox/e2e/support/mm_blocks_test_helper.ts`:
- Around line 133-171: Replace every raw interactive dialog scroll-view ID in
detox/e2e/support/mm_blocks_test_helper.ts at ranges 133-171 and 318-332 with
InteractiveDialogScreen.testID.scrollView; at 688-706, add page-object constants
for mm_blocks_text_input.screen, mm_blocks_text_input.input, and
mm_blocks.text_input.save.button and use them in by.id matchers. In
detox/e2e/test/products/channels/interactive_messages/mm_blocks_dialog_stacking.e2e.ts
at 29-30, remove local SUBMIT_BUTTON and CLOSE_BUTTON constants, use
InteractiveDialogScreen.testID values, and build the child_input matcher through
a page-object helper. Do not leave hardcoded by.id strings.
---
Minor comments:
In `@app/actions/remote/integrations.ts`:
- Around line 120-121: Update the doBlockAction-unavailable branch to avoid
exposing the hardcoded English Error message through error.message; return an
error without a display message so consumers such as BlocksDialogShell and the
interactive message component use their localized fallback. Keep the existing
debug log and availability behavior unchanged.
In
`@app/components/block_renderer/date_input_element/date_input_element.test.tsx`:
- Around line 50-68: In
app/components/block_renderer/date_input_element/date_input_element.test.tsx
lines 50-68, add a beforeEach that clears Jest mocks and restores the
DateTimeSelector mock implementation, and move database setup from beforeAll to
beforeEach and teardown from afterAll to afterEach. Apply the identical changes
in
app/components/block_renderer/datetime_input_element/datetime_input_element.test.tsx
lines 51-69, preserving real in-memory LokiJS databases with autosave disabled.
In
`@app/components/block_renderer/datetime_input_element/datetime_input_element.tsx`:
- Around line 122-137: Update the FormattedDate component in the selectedDate
display to pass the existing displayTimezone through its timezone prop, matching
the timezone already supplied to FormattedTime so both values represent the same
date and time zone.
In `@app/components/block_renderer/form_validation.ts`:
- Around line 193-195: Update the URL validation condition in the field
validation flow to require the value to start with either “http://” or
“https://” using startsWith, matching the legacy validator in integrations.ts.
Replace the current includes checks while preserving the existing badUrl error
response.
In `@app/components/block_renderer/mm_blocks_file_upload.test.tsx`:
- Around line 42-43: Update fileMaxWarning() in app/utils/file/index.ts to use a
defineMessages() ICU plural message, with count 1 rendered as “Uploads limited
to 1 file maximum.” Update the matching English translation in
assets/base/i18n/en.json and revise SINGLE_FILE_WARNING to the singular wording,
then run npm run i18n-extract.
In `@app/components/block_renderer/mm_blocks_file_upload.tsx`:
- Around line 306-325: Update the file-ID generation in the upload handling loop
before `sourceFilesRef.current.set` and `added.push` so each newly added file
receives a unique `clientId`, including repeated selections of the same file in
multi-select mode. Preserve existing IDs when unused, but append a deterministic
suffix or otherwise resolve collisions against current source files and files
being added; use the resolved ID consistently for `sourceFilesRef`, the
`UploadFileState`, and `startUpload`.
In `@app/components/block_renderer/translation/block_kit.ts`:
- Around line 160-162: Update the initial_date_time handling in
translatePostProps to create the Date, validate date.getTime() is finite before
calling toISOString(), and skip initial_value assignment for invalid dates. Add
a regression test covering an oversized finite timestamp and verify translation
returns normally without error blocks.
In `@app/components/settings/bool_setting.tsx`:
- Around line 138-145: Normalize the optional value once in the BoolSetting
component, then use the normalized boolean for the Switch value, the toggled
testID, and thumbColor via the checked state. Keep the existing behavior
unchanged when value is provided, while ensuring omitted value never produces
undefined or an uncontrolled Switch.
- Around line 117-126: Update the component-library caller of BoolSetting to
pass optional explicitly as true through useBooleanProp, ensuring the setting
renders the optional label instead of the required marker. Keep the existing
BoolSetting rendering logic unchanged.
In `@app/components/settings/radio_setting/radio_entry.tsx`:
- Around line 106-118: Update the row rendered by the radio entry component so
its visual style changes when disabled: apply the established dimmed/greyed
styling to the container or text while preserving the normal styling when
enabled. Use the existing disabled prop and style symbols rather than changing
the press behavior.
In `@app/screens/dialog_router/blocks_dialog_router.test.tsx`:
- Around line 63-76: Update both mockBlocksDialogShell assertions to call
toHaveBeenCalledWith with only the expected props object, removing the explicit
undefined argument so they match React 19’s single-argument invocation.
In `@detox/e2e/support/ui/screen/interactive_dialog.ts`:
- Around line 12-42: Update textInputFieldTestID in the interactive dialog
screen helper to return the modal input testID rendered by MmBlocksTextInput,
using the mm_blocks_text_input.input identifier instead of the current
textInputTestID-based value so fillTextElement() can locate the field.
In
`@detox/e2e/test/products/channels/interactive_messages/mm_blocks_dialog.e2e.ts`:
- Around line 34-51: Update the suite lifecycle around beforeEach and afterEach:
in beforeEach, assert ChannelListScreen.toBeVisible() and reopen testChannel
after assertSuiteRunnable(); in afterEach, navigate back to the channel list
instead of only ensuring the channel screen, while preserving blocks-dialog
dismissal and existing setupChannelTest behavior.
In
`@detox/e2e/test/products/channels/interactive_messages/mm_blocks_form_inputs.e2e.ts`:
- Around line 534-548: Update the date-picker setup around
nativeDateTimePicker.setDatePickerDate to avoid silently swallowing Android
failures: either capture and preserve failure artifacts before allowing the
default-date confirmation path, or split Android into an explicit
platform-specific flow that does not rely on a swallowed exception. Keep the
existing iOS dismissal behavior and ensure the assertion cannot pass without
verifying the intended date was set.
In `@detox/utils/webhook_utils.js`:
- Around line 444-446: Update the filteredOptions logic in
getMmBlocksLookupOptions to normalize searchText to lowercase before comparing
it with the lowercased option.text and option.value, preserving case-insensitive
matching consistently with the other lookup path.
---
Nitpick comments:
In `@app/actions/remote/file.ts`:
- Around line 43-52: Update the catch block in fetchFileInfo to call
forceLogoutIfNecessary(serverUrl, error) before logging and returning the error,
matching the error handling used by sibling actions such as fetchPublicLink.
In `@app/client/rest/integrations.test.ts`:
- Around line 98-136: Rename the three newly added tests for
lookupInteractiveDialog, executeDialogAction, and doBlockAction to the
repository’s preferred it('should...') naming style, while preserving their
existing assertions and behavior.
In `@app/components/block_renderer/block_renderer.tsx`:
- Around line 80-82: Prevent redundant notifications in the effect around
hasUploadingFields and onUploadingChange by retaining the last notified boolean
in a ref and invoking the callback only when that value changes; alternatively,
document the required stable callback identity in the onUploadingChange prop
comment. Keep the existing notification behavior for actual hasUploadingFields
transitions.
In
`@app/components/block_renderer/datetime_input_element/datetime_input_element.tsx`:
- Line 129: Move the inline date_time_selector.at descriptor into a module-level
defineMessages() declaration in datetime_input_element, then replace the inline
intl.formatMessage descriptor with intl.formatMessage(messages.at). Run npm run
i18n-extract to update the translation catalog.
In `@app/components/block_renderer/file_input_element.test.tsx`:
- Around line 232-233: Replace the positive remove-control assertion in the file
input test with getByTestId(...).toBeTruthy(), and remove queryByTestId from the
test’s destructured helpers if no other absence assertions use it.
In `@app/components/block_renderer/translation/mm_block.ts`:
- Around line 459-529: Introduce a shared parameterized helper for the
duplicated logic in translateDateInputBlock and translateDateTimeInputBlock,
accepting the appropriate required-keys constant and block type while preserving
each function’s return type and behavior. Update both translators to delegate to
this helper so validation, parsing, and output construction remain centralized.
In
`@app/components/post_list/post/body/content/interactive_messages/index.test.tsx`:
- Around line 415-416: Replace the inline require in the test with a top-level
import of dismissMmBlocksExpandedContentIfOpen from `@screens/navigation`, and use
jest.mocked() when asserting the mocked function. Keep the existing module mock
and assertion behavior unchanged.
In `@app/components/settings/radio_setting/radio_entry.tsx`:
- Around line 83-118: Add accessibilityRole and accessibilityState to the
TouchableWithFeedback option row, using the checklist variant to select the
checkbox role and radio otherwise, and exposing isSelected as checked together
with disabled state.
In `@app/queries/servers/features.test.ts`:
- Around line 127-128: Derive the below-version fixture near atBlockActions from
BLOCK_ACTIONS_VERSION instead of hardcoding 11.10.0, ensuring it always remains
below the configured boundary when the constant changes. Alternatively,
explicitly assert with isMinimumServerVersion that the literal is below
BLOCK_ACTIONS_VERSION.
- Line 110: Rename the newly added tests in the feature flag test block to use
the repository’s “should...” naming convention, including the tests currently
beginning with “honors” and “is” at the referenced locations. Preserve each
test’s behavior and assertions while changing only their descriptions to start
with “should”.
In `@app/queries/servers/features.ts`:
- Line 69: Replace the `version || ''` fallback in the feature checks using
`isMinimumServerVersion` with `version ?? ''`, including the corresponding
occurrence near the second referenced check. Preserve the existing
`mmBlocksEnabled` and version-validation logic.
In `@app/routes/`(modals)/dialog_router.tsx:
- Around line 26-30: Replace the empty-dependency useEffect around
CallbackStore.removeCallback with useDidMount, returning the cleanup function
through that hook so CallbackStore.removeCallback runs on unmount.
In `@app/routes/`(modals)/mm_blocks_text_input.tsx:
- Line 24: Update the text-input modal configuration around headerTitle to
define and reuse the existing mm_blocks.text_input.title message through
defineMessages(), instead of constructing the message inline. Preserve the
existing translated title and fallback text, and do not add a new translation
entry.
In `@app/screens/dialog_router/blocks_dialog_router.test.tsx`:
- Around line 11-16: Update the blocks_dialog_shell mock factory to return an
explicit module object with the expected default export, while preserving the
existing jest.fn implementation that renders the mode-specific test view. Keep
mockBlocksDialogShell aligned with that default-export shape when accessing the
mock.
In `@app/screens/dialog_router/blocks_dialog_shell.test.tsx`:
- Around line 599-623: The upload test around BlockRenderer’s onUploadingChange
callbacks currently repeats the same state and does not verify independent field
tracking. Replace the duplicate true calls with two distinct field names, then
clear each field separately while asserting submit remains disabled until both
uploads finish, and preserve the final successful submit assertion.
In `@app/screens/dialog_router/blocks_dialog_shell.tsx`:
- Around line 693-701: Move the handleLegacyFooterSubmit useCallback declaration
above the derived render values showLegacySubmit and legacyBlockSubmit, grouping
it with the component’s other hooks while preserving its existing callback body
and dependency array.
In `@app/screens/dialog_router/index.test.tsx`:
- Line 50: Replace the CommonJS require for observeBlockActionsEnabled with a
typed top-level import, wrap it using jest.mocked, and update the tests to call
mockedObserveBlockActionsEnabled.mockReturnValue(of$(true)) while preserving the
existing mock behavior.
In `@app/store/navigation_store.ts`:
- Around line 186-190: Document or test the duplicate-handling behavior of
getScreensInStack(): stack navigators must preserve repeated screen IDs, while
tab navigators must process only the active tab and de-duplicate IDs. Anchor the
change to getScreensInStack and cover both navigator modes without altering the
existing positional stack behavior.
In `@app/utils/dialog_conversion.test.ts`:
- Line 918: The test fixtures in app/utils/dialog_conversion.test.ts:918-918 and
app/utils/interactive_dialog_adapter.test.ts:175-175 should be declared as
InteractiveDialogConfig & {dialog: Dialog}; remove the non-null assertions at
dialog_conversion.test.ts lines 918, 932, 946, 960, 1027, 1029, 1062, and 1079,
and interactive_dialog_adapter.test.ts lines 175, 374, and 405, relying on the
strengthened fixture type instead.
In `@app/utils/dialog_conversion.ts`:
- Around line 88-91: The value conversion branch currently treats non-empty
"off" as true; update the field-value handling around submission[fieldName] so
the literal "off" follows the same falsy path as the corresponding false values,
while preserving "on" as truthy and existing Boolean coercion for other
noncanonical strings.
In `@app/utils/dialog_to_mm_blocks.test.ts`:
- Around line 198-221: Merge the two action-button-only cases around
convertDialogToMmBlocks and dialogShouldShowSubmitChrome into one test, reusing
the shared elements and blocks assertions while checking both undefined and
'Save' submit-label outcomes without calling convertDialogToMmBlocks twice.
- Line 66: Rename the affected test cases in the dialog-to-MM-blocks test suite
to begin with “should”, including the cases currently named maps, converts,
keeps, coerces, skips, and labels. Preserve each test’s existing behavior and
assertions while applying the naming convention consistently.
In `@app/utils/dialog_to_mm_blocks.ts`:
- Around line 18-33: Update boolDefault to trim the string value before
lowercasing and comparing it, so surrounding whitespace is ignored consistently
with convertAppFormValuesToDialogSubmission. Preserve the existing handling for
undefined, empty, boolean, and unrecognized values.
In `@assets/base/i18n/en.json`:
- Line 116: Rename the unused apps.error.form.required_fields_empty translation
key in en.json to the interactive_dialog.* message ID used by both interactive
form components, preserving the existing English message value.
In `@detox/e2e/support/ui/screen/interactive_dialog.ts`:
- Around line 97-110: Update InteractiveDialog.submit so the legacy fallback is
used only when the native submit element is absent: use waitFor(...).toExist()
with a short timeout for the existence check, then tap the selected button while
allowing tap or enabled-state failures to propagate. Do not catch every
native-button error, add retries, perform a second tap after a failed tap, or
increase timeouts; remove the fallback if supported builds no longer render
legacySubmitButton.
- Around line 13-14: Remove the duplicate scroll-view identifier from the
selector definitions, keeping a single canonical `scrollView` entry and updating
`interactiveDialogScreen` usages to reference it, or define
`interactiveDialogScreen` as an explicit alias with a clarifying comment. Apply
the same change to both duplicated selector pairs.
In
`@detox/e2e/test/products/channels/interactive_dialog/interactive_dialog_plugin.e2e.ts`:
- Line 590: Replace the raw 3000-millisecond timeout in the waits around
dynamicCompaniesButton, including the occurrences near the referenced lines,
with the shared timeouts.THREE_SEC constant; use timeouts.TWO_SEC or the closest
defined shared constant for any corresponding 2000-millisecond waits. Preserve
the existing wait behavior.
In
`@detox/e2e/test/products/channels/interactive_messages/mm_blocks_dialog_datetime.e2e.ts`:
- Around line 20-22: Update the comment adjacent to EVENT_DATE and MEETING_DATE
so it accurately describes the date-only string constants, and move the “Midday
UTC keeps the calendar day stable” explanation to the T12:00:00Z construction at
the later date-parsing or datetime setup lines.
In
`@detox/e2e/test/products/channels/interactive_messages/mm_blocks_dialog_selects.e2e.ts`:
- Around line 24-25: Replace the any annotations on the shared testChannel and
testUser fixtures with types derived from the relevant helper return values,
using ReturnType or the helpers’ awaited result types as appropriate. Preserve
their existing initialization and usage while enabling TypeScript to validate
properties such as display_name and username.
- Around line 27-56: Update the test lifecycle around setupChannelTest and the
beforeEach/afterEach hooks: document that setupChannelTest wraps
Setup.apiInit(siteOneUrl) and the initial channel-list visibility assertion, run
the isolated setup in beforeEach, and explicitly assert
ChannelListScreen.toBeVisible() before each test. Change afterEach cleanup to
return to ChannelListScreen rather than ChannelScreen, while preserving the
existing dialog dismissal and recovery handling.
In `@types/api/integrations.d.ts`:
- Around line 224-227: Replace the duplicate DialogSelectOption object
definition with a type alias to the existing DialogOption type, preserving the
current public name while ensuring both option types cannot diverge.
- Line 140: Review the block payload types in BlockDialog.blocks and
DoBlockActionResponse.mm_blocks, using the existing MmBlock type where consumers
treat these values as MmBlock[]. Replace the loose array declarations and
corresponding direct casts when appropriate, while retaining the wire-level
types if boundary narrowing is intentional.
In `@types/api/mm_blocks.d.ts`:
- Around line 157-158: Update the data_source type in the relevant declaration
to use the specific literals alongside an open-ended `(string & {})` arm,
preserving arbitrary custom strings while retaining editor suggestions for
users, channels, and dynamic.
🪄 Autofix
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: 71d1bc80-5996-4220-9e1b-95e9ee573c7e
📒 Files selected for processing (119)
app/actions/remote/file.test.tsapp/actions/remote/file.tsapp/actions/remote/integrations.test.tsapp/actions/remote/integrations.tsapp/client/rest/files.test.tsapp/client/rest/files.tsapp/client/rest/integrations.test.tsapp/client/rest/integrations.tsapp/components/autocomplete_selector/index.test.tsxapp/components/autocomplete_selector/index.tsxapp/components/block_renderer/block_renderer.test.tsxapp/components/block_renderer/block_renderer.tsxapp/components/block_renderer/bool_input_element.test.tsxapp/components/block_renderer/bool_input_element.tsxapp/components/block_renderer/button_element.test.tsxapp/components/block_renderer/button_element.tsxapp/components/block_renderer/context.tsxapp/components/block_renderer/date_input_element/date_input_element.test.tsxapp/components/block_renderer/date_input_element/date_input_element.tsxapp/components/block_renderer/date_input_element/index.tsapp/components/block_renderer/datetime_input_element/datetime_input_element.test.tsxapp/components/block_renderer/datetime_input_element/datetime_input_element.tsxapp/components/block_renderer/datetime_input_element/index.tsapp/components/block_renderer/file_input_element.test.tsxapp/components/block_renderer/file_input_element.tsxapp/components/block_renderer/form/context.tsapp/components/block_renderer/form/form.test.tsxapp/components/block_renderer/form/index.tsapp/components/block_renderer/form/mm_blocks_field_error.tsxapp/components/block_renderer/form/mm_blocks_form.tsxapp/components/block_renderer/form/relayed_mm_blocks_form.tsxapp/components/block_renderer/form/types.tsapp/components/block_renderer/form_validation.test.tsapp/components/block_renderer/form_validation.tsapp/components/block_renderer/index.tsapp/components/block_renderer/layout_blocks.tsxapp/components/block_renderer/mm_blocks_context_provider.test.tsxapp/components/block_renderer/mm_blocks_context_provider.tsxapp/components/block_renderer/mm_blocks_file_upload.test.tsxapp/components/block_renderer/mm_blocks_file_upload.tsxapp/components/block_renderer/select_input_element.test.tsxapp/components/block_renderer/select_input_element.tsxapp/components/block_renderer/static_select_element.test.tsxapp/components/block_renderer/static_select_element.tsxapp/components/block_renderer/text_input_element.test.tsxapp/components/block_renderer/text_input_element.tsxapp/components/block_renderer/translation/adaptive_cards.test.tsapp/components/block_renderer/translation/adaptive_cards.tsapp/components/block_renderer/translation/block_kit.test.tsapp/components/block_renderer/translation/block_kit.tsapp/components/block_renderer/translation/mm_block.test.tsapp/components/block_renderer/translation/mm_block.tsapp/components/block_renderer/types.tsapp/components/chips/selected_channel_chip.tsxapp/components/floating_input/floating_input_container.tsxapp/components/floating_input/floating_text_input_label.tsxapp/components/post_list/index.tsapp/components/post_list/post/body/content/content.test.tsxapp/components/post_list/post/body/content/interactive_messages/index.test.tsxapp/components/post_list/post/body/content/interactive_messages/index.tsxapp/components/settings/bool_setting.tsxapp/components/settings/footer.tsxapp/components/settings/label.test.tsxapp/components/settings/label.tsxapp/components/settings/radio_setting/index.tsxapp/components/settings/radio_setting/radio_entry.tsxapp/components/settings/text_setting.tsxapp/constants/integrations.tsapp/constants/screens.tsapp/constants/versions.tsapp/managers/integrations_manager.tsapp/queries/servers/features.test.tsapp/queries/servers/features.tsapp/routes/(modals)/dialog_router.tsxapp/routes/(modals)/mm_blocks_content.tsxapp/routes/(modals)/mm_blocks_text_input.tsxapp/screens/apps_form/apps_form_field/apps_form_field.tsxapp/screens/component_library/mm_blocks.cl.tsxapp/screens/component_library/mm_blocks_editor_utils.tsapp/screens/dialog_router/blocks_dialog_router.test.tsxapp/screens/dialog_router/blocks_dialog_router.tsxapp/screens/dialog_router/blocks_dialog_shell.test.tsxapp/screens/dialog_router/blocks_dialog_shell.tsxapp/screens/dialog_router/dialog_router.tsxapp/screens/dialog_router/index.test.tsxapp/screens/dialog_router/index.tsxapp/screens/integration_selector/integration_selector.tsxapp/screens/mm_blocks_content/index.test.tsxapp/screens/mm_blocks_content/index.tsxapp/screens/mm_blocks_text_input/index.test.tsxapp/screens/mm_blocks_text_input/index.tsxapp/screens/navigation.test.tsapp/screens/navigation.tsapp/store/navigation_store.tsapp/utils/dialog_conversion.test.tsapp/utils/dialog_conversion.tsapp/utils/dialog_to_mm_blocks.test.tsapp/utils/dialog_to_mm_blocks.tsapp/utils/dialog_utils.tsapp/utils/integrations.test.tsapp/utils/integrations.tsapp/utils/interactive_dialog_adapter.test.tsapp/utils/interactive_dialog_adapter.tsassets/base/i18n/en.jsondetox/e2e/support/mm_blocks_test_helper.tsdetox/e2e/support/ui/screen/interactive_dialog.tsdetox/e2e/test/products/channels/interactive_dialog/interactive_dialog_plugin.e2e.tsdetox/e2e/test/products/channels/interactive_messages/mm_blocks_dialog.e2e.tsdetox/e2e/test/products/channels/interactive_messages/mm_blocks_dialog_datetime.e2e.tsdetox/e2e/test/products/channels/interactive_messages/mm_blocks_dialog_files.e2e.tsdetox/e2e/test/products/channels/interactive_messages/mm_blocks_dialog_multistep.e2e.tsdetox/e2e/test/products/channels/interactive_messages/mm_blocks_dialog_selects.e2e.tsdetox/e2e/test/products/channels/interactive_messages/mm_blocks_dialog_stacking.e2e.tsdetox/e2e/test/products/channels/interactive_messages/mm_blocks_dialog_validation.e2e.tsdetox/e2e/test/products/channels/interactive_messages/mm_blocks_form_inputs.e2e.tsdetox/utils/webhook_utils.jsdetox/webhook_server.jstypes/api/integrations.d.tstypes/api/mm_blocks.d.ts
mattermost-build
added
the
E2E/Run
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/components/date_time_selector/date_time_selector.test.tsx (1)
206-212: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the disabled time-button path.
This test only presses
${testID}.select.button. Add an assertion for${testID}.time.buttonand verify that pressing it does not open the picker or manual time input.🤖 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/components/date_time_selector/date_time_selector.test.tsx` around lines 206 - 212, Extend the test around the disabled date selector to also query `${testID}.time.button`, assert it is disabled, press it, and verify neither the date-time picker nor manual time input opens. Keep the existing date-button and mockHandleChange assertions unchanged.
🤖 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/components/date_time_selector/date_time_selector.test.tsx`:
- Line 194: Rename the test case in the date-time selector test suite so its
it() description starts with “should”, while preserving its existing description
of disabled select buttons and picker behavior.
In `@app/components/date_time_selector/date_time_selector.tsx`:
- Line 220: Update the date-time selector component’s disabled-state handling to
reset both useManualEntry and show whenever disabled becomes true, so
re-enabling requires a new interaction; add a test covering the disable/enable
transition while the manual input or picker is open.
---
Nitpick comments:
In `@app/components/date_time_selector/date_time_selector.test.tsx`:
- Around line 206-212: Extend the test around the disabled date selector to also
query `${testID}.time.button`, assert it is disabled, press it, and verify
neither the date-time picker nor manual time input opens. Keep the existing
date-button and mockHandleChange assertions unchanged.
🪄 Autofix
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: 4edc907d-9f5c-4c02-bb55-36447a066b4a
📒 Files selected for processing (8)
app/components/block_renderer/date_input_element/date_input_element.test.tsxapp/components/block_renderer/date_input_element/date_input_element.tsxapp/components/block_renderer/datetime_input_element/datetime_input_element.test.tsxapp/components/block_renderer/datetime_input_element/datetime_input_element.tsxapp/components/date_time_selector/date_time_selector.test.tsxapp/components/date_time_selector/date_time_selector.tsxapp/screens/apps_form/apps_form_field/apps_form_field.tsxdetox/e2e/support/ui/screen/scheduled_message_screen.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- app/components/block_renderer/date_input_element/date_input_element.test.tsx
- app/components/block_renderer/date_input_element/date_input_element.tsx
- app/components/block_renderer/datetime_input_element/datetime_input_element.test.tsx
mattermost-build
removed
the
E2E/Run
mattermost-build
added
the
E2E/Run
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/date_time_selector/date_time_selector.test.tsx (1)
194-264: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTest both disabled interaction states.
Line 220 does not test manual entry.
allowManualTimeEntryis false, so the input is absent without a button press.Lines 249-263 do not open
DateTimePickerbefore disabling. Add a separate disable and re-enable transition that opens the date picker first. SetallowManualTimeEntry={true}in the disabled-button test to verify that the time button cannot reveal manual entry.As per coding guidelines, test actual implementation behaviour.
🤖 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/components/date_time_selector/date_time_selector.test.tsx` around lines 194 - 264, The DateTimeSelector tests do not cover both disabled interaction states. Update the disabled-button test to set allowManualTimeEntry={true} so pressing the disabled time button verifies manual entry remains hidden, and extend the reset test to open the date picker before disabling, then verify it closes and stays closed after re-enabling.Source: Coding guidelines
🤖 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/components/block_renderer/mm_blocks_file_upload.tsx`:
- Around line 307-319: Ensure client IDs remain unique for the entire component
lifetime, not only while entries exist in sourceFilesRef, by tracking every
issued ID in a separate persistent ref or associating callbacks with upload
generations. Update the ID allocation and delayed upload callbacks around
sourceFilesRef so callbacks from removed or replaced uploads cannot mutate a
newly selected file, and add a test covering removal, reselection, and
completion of the stale request.
---
Outside diff comments:
In `@app/components/date_time_selector/date_time_selector.test.tsx`:
- Around line 194-264: The DateTimeSelector tests do not cover both disabled
interaction states. Update the disabled-button test to set
allowManualTimeEntry={true} so pressing the disabled time button verifies manual
entry remains hidden, and extend the reset test to open the date picker before
disabling, then verify it closes and stays closed after re-enabling.
🪄 Autofix
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: be4797f1-159c-4029-ba2d-3dd5780c797c
📒 Files selected for processing (64)
app/actions/remote/file.tsapp/client/rest/integrations.test.tsapp/components/autocomplete_selector/index.tsxapp/components/block_renderer/block_renderer.test.tsxapp/components/block_renderer/block_renderer.tsxapp/components/block_renderer/bool_input_element.test.tsxapp/components/block_renderer/bool_input_element.tsxapp/components/block_renderer/date_input_element/date_input_element.test.tsxapp/components/block_renderer/datetime_input_element/datetime_input_element.test.tsxapp/components/block_renderer/datetime_input_element/datetime_input_element.tsxapp/components/block_renderer/file_input_element.test.tsxapp/components/block_renderer/file_input_element.tsxapp/components/block_renderer/form/mm_blocks_form.tsxapp/components/block_renderer/form_validation.test.tsapp/components/block_renderer/form_validation.tsapp/components/block_renderer/mm_blocks_file_upload.test.tsxapp/components/block_renderer/mm_blocks_file_upload.tsxapp/components/block_renderer/select_input_element.tsxapp/components/block_renderer/translation/block_kit.test.tsapp/components/block_renderer/translation/block_kit.tsapp/components/block_renderer/translation/mm_block.tsapp/components/date_time_selector/date_time_selector.test.tsxapp/components/date_time_selector/date_time_selector.tsxapp/components/post_list/post/body/content/interactive_messages/index.test.tsxapp/components/post_list/post/body/content/interactive_messages/index.tsxapp/components/settings/bool_setting.tsxapp/components/settings/radio_setting/radio_entry.tsxapp/queries/servers/features.test.tsapp/queries/servers/features.tsapp/routes/(modals)/mm_blocks_text_input.tsxapp/screens/component_library/hooks.tsxapp/screens/dialog_router/blocks_dialog_router.test.tsxapp/screens/dialog_router/blocks_dialog_router.tsxapp/screens/dialog_router/blocks_dialog_shell.test.tsxapp/screens/dialog_router/blocks_dialog_shell.tsxapp/screens/dialog_router/index.test.tsxapp/screens/edit_post/edit_post.test.tsxapp/screens/navigation.test.tsapp/screens/navigation.tsapp/store/navigation_store.test.tsapp/store/navigation_store.tsapp/utils/dialog_conversion.test.tsapp/utils/dialog_conversion.tsapp/utils/dialog_to_mm_blocks.test.tsapp/utils/dialog_to_mm_blocks.tsapp/utils/file/index.test.tsapp/utils/file/index.tsapp/utils/interactive_dialog_adapter.test.tsassets/base/i18n/en.jsondetox/e2e/support/mm_blocks_test_helper.tsdetox/e2e/support/ui/screen/index.tsdetox/e2e/support/ui/screen/interactive_dialog.tsdetox/e2e/support/ui/screen/mm_blocks_text_input.tsdetox/e2e/test/products/channels/interactive_dialog/interactive_dialog_plugin.e2e.tsdetox/e2e/test/products/channels/interactive_messages/mm_blocks_dialog.e2e.tsdetox/e2e/test/products/channels/interactive_messages/mm_blocks_dialog_datetime.e2e.tsdetox/e2e/test/products/channels/interactive_messages/mm_blocks_dialog_files.e2e.tsdetox/e2e/test/products/channels/interactive_messages/mm_blocks_dialog_multistep.e2e.tsdetox/e2e/test/products/channels/interactive_messages/mm_blocks_dialog_selects.e2e.tsdetox/e2e/test/products/channels/interactive_messages/mm_blocks_dialog_stacking.e2e.tsdetox/e2e/test/products/channels/interactive_messages/mm_blocks_form_inputs.e2e.tsdetox/utils/webhook_utils.jstypes/api/integrations.d.tstypes/api/mm_blocks.d.ts
🚧 Files skipped from review as they are similar to previous changes (44)
- app/screens/navigation.test.ts
- app/screens/navigation.ts
- app/screens/dialog_router/blocks_dialog_router.tsx
- app/components/block_renderer/mm_blocks_file_upload.test.tsx
- app/components/block_renderer/bool_input_element.test.tsx
- detox/e2e/test/products/channels/interactive_messages/mm_blocks_dialog_datetime.e2e.ts
- detox/e2e/test/products/channels/interactive_messages/mm_blocks_dialog_multistep.e2e.ts
- app/utils/dialog_conversion.test.ts
- app/screens/dialog_router/blocks_dialog_shell.test.tsx
- app/store/navigation_store.ts
- app/components/block_renderer/form_validation.test.ts
- detox/e2e/test/products/channels/interactive_messages/mm_blocks_dialog.e2e.ts
- app/queries/servers/features.ts
- app/client/rest/integrations.test.ts
- app/routes/(modals)/mm_blocks_text_input.tsx
- app/screens/dialog_router/index.test.tsx
- detox/e2e/test/products/channels/interactive_messages/mm_blocks_dialog_files.e2e.ts
- app/components/settings/bool_setting.tsx
- app/components/block_renderer/form/mm_blocks_form.tsx
- detox/e2e/test/products/channels/interactive_messages/mm_blocks_dialog_stacking.e2e.ts
- app/components/block_renderer/datetime_input_element/datetime_input_element.tsx
- app/components/block_renderer/file_input_element.test.tsx
- detox/e2e/test/products/channels/interactive_messages/mm_blocks_dialog_selects.e2e.ts
- detox/e2e/test/products/channels/interactive_messages/mm_blocks_form_inputs.e2e.ts
- app/components/block_renderer/bool_input_element.tsx
- app/utils/dialog_conversion.ts
- app/components/block_renderer/block_renderer.test.tsx
- app/components/block_renderer/select_input_element.tsx
- app/components/block_renderer/translation/block_kit.ts
- types/api/integrations.d.ts
- app/components/block_renderer/datetime_input_element/datetime_input_element.test.tsx
- app/components/block_renderer/date_input_element/date_input_element.test.tsx
- types/api/mm_blocks.d.ts
- app/utils/dialog_to_mm_blocks.ts
- app/components/autocomplete_selector/index.tsx
- app/actions/remote/file.ts
- app/components/block_renderer/translation/mm_block.ts
- app/components/block_renderer/block_renderer.tsx
- app/components/settings/radio_setting/radio_entry.tsx
- detox/e2e/support/ui/screen/interactive_dialog.ts
- app/components/block_renderer/file_input_element.tsx
- app/components/block_renderer/translation/block_kit.test.ts
- detox/utils/webhook_utils.js
- detox/e2e/support/mm_blocks_test_helper.ts
Comment on lines
+307
to
+319
| const usedClientIds = new Set(sourceFilesRef.current.keys()); | ||
| for (const file of toUpload) { | ||
| // Add uniqueness by using a suffix in case the same file is selected multiple times. | ||
| let clientId = file.clientId ?? file.localPath ?? file.name; | ||
| if (usedClientIds.has(clientId)) { | ||
| let suffix = 1; | ||
| while (usedClientIds.has(`${clientId}-${suffix}`)) { | ||
| suffix += 1; | ||
| } | ||
| clientId = `${clientId}-${suffix}`; | ||
| } | ||
| usedClientIds.add(clientId); | ||
| sourceFilesRef.current.set(clientId, file); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep client IDs unique for the component lifetime.
If a user removes or replaces an in-flight file and then selects the same source again, Lines 307-319 can reuse its client ID. A delayed callback from the cancelled upload can then mark the new row as failed or uploaded with the old file ID. FileInputElement publishes these settled IDs into form state.
Keep issued IDs in a separate ref, or attach an upload generation to each callback. Add a test that removes an in-flight file, reselects it, and completes the old request after the new request starts.
Proposed fix
const sourceFilesRef = useRef(new Map<string, ExtractedFileInfo>());
+const issuedClientIdsRef = useRef(new Set<string>());
-const usedClientIds = new Set(sourceFilesRef.current.keys());
+const usedClientIds = issuedClientIdsRef.current;Based on the supplied downstream contract in app/components/block_renderer/file_input_element.tsx, settled IDs become form values.
📝 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.
Suggested change
| const usedClientIds = new Set(sourceFilesRef.current.keys()); | |
| for (const file of toUpload) { | |
| // Add uniqueness by using a suffix in case the same file is selected multiple times. | |
| let clientId = file.clientId ?? file.localPath ?? file.name; | |
| if (usedClientIds.has(clientId)) { | |
| let suffix = 1; | |
| while (usedClientIds.has(`${clientId}-${suffix}`)) { | |
| suffix += 1; | |
| } | |
| clientId = `${clientId}-${suffix}`; | |
| } | |
| usedClientIds.add(clientId); | |
| sourceFilesRef.current.set(clientId, file); | |
| const issuedClientIdsRef = useRef(new Set<string>()); | |
| const usedClientIds = issuedClientIdsRef.current; | |
| for (const file of toUpload) { | |
| // Add uniqueness by using a suffix in case the same file is selected multiple times. | |
| let clientId = file.clientId ?? file.localPath ?? file.name; | |
| if (usedClientIds.has(clientId)) { | |
| let suffix = 1; | |
| while (usedClientIds.has(`${clientId}-${suffix}`)) { | |
| suffix += 1; | |
| } | |
| clientId = `${clientId}-${suffix}`; | |
| } | |
| usedClientIds.add(clientId); | |
| sourceFilesRef.current.set(clientId, file); |
🤖 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/components/block_renderer/mm_blocks_file_upload.tsx` around lines 307 -
319, Ensure client IDs remain unique for the entire component lifetime, not only
while entries exist in sourceFilesRef, by tracking every issued ID in a separate
persistent ref or associating callbacks with upload generations. Update the ID
allocation and delayed upload callbacks around sourceFilesRef so callbacks from
removed or replaced uploads cannot mutate a newly selected file, and add a test
covering removal, reselection, and completion of the stale request.
Contributor
|
Hi Daniel, |
Contributor
Author
|
After discussing with Yasser, we are going to try to move the detox tests to use the demo plugin instead of the webhook sidecar. |
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
This PR adds Blocks to render Interactive Dialogs. It covers all the types supported by dialogs as of today (including file elements, which were not supported on mobile yet) and also gives support for other blocks like containers, scrollable, collapsible, etc...
Input blocks can also be used in interactive messages.
We also have modernized the inputs to align more with the current state of the app.
Related PR
WEB/SERVER: mattermost/mattermost#37767
Ticket Link
TBD
Screenshots
Release Note