Mattermost mobile file upload#9898
Open
sbishel wants to merge 6 commits into
Open
Conversation
mattermost-build
added
the
E2E/Run
Coverage Comparison Report |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds FILE support across dialog conversion, Apps Form, dialog routing, REST file-info fetching, and E2E coverage, including file upload, hydration, validation, and submission payload handling. ChangesFile Upload Field in Interactive Dialogs
Sequence Diagram(s)sequenceDiagram
participant AppsFormFileField
participant AppsFormComponent
participant DialogRouter
participant Server
AppsFormFileField->>AppsFormComponent: onPendingChange(true/false)
AppsFormComponent->>AppsFormComponent: block submit while uploads active
AppsFormComponent->>DialogRouter: submit values
DialogRouter->>DialogRouter: mergeFileFieldValues + flattenFileFieldValues
DialogRouter->>Server: submitInteractiveDialog({ submission, file_ids })
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (8)
app/screens/apps_form/apps_form_file_field/apps_form_file_field.test.tsx (2)
252-257: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid TypeScript-satisfaction conditionals in these mocks.
The
if (onComplete)/if (onError)guards make the tests less strict and can hide a callback contract break. Capture those callbacks with a typed mock signature or direct assertion instead. As per coding guidelines, don't create conditionals in tests for TypeScript satisfaction.Also applies to: 289-293, 333-337, 366-370, 397-401, 467-471, 515-519
🤖 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/apps_form/apps_form_file_field/apps_form_file_field.test.tsx` around lines 252 - 257, The test mocks in apps_form_file_field.test.tsx use TypeScript-satisfaction guards around callback capture, which weakens the contract being tested. Update the mock implementations for upload and related handlers to capture callbacks directly with a typed mock signature or an explicit assertion instead of using conditional checks like if (onComplete) or if (onError). Apply the same cleanup in all affected mock blocks so the tests rely on the real callback contract rather than optional-guarded behavior.Source: Coding guidelines
328-359: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the full joined ID list.
toMatch(/id-a|id-b/)passes even if only one uploaded ID makes it intoonChange, so this won't catch the regression the spec is meant to cover. Please assert the exact joined value, or split it and verify both IDs plus length. As per coding guidelines, check array lengths in addition to individual items, and test expectations must match 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/screens/apps_form/apps_form_file_field/apps_form_file_field.test.tsx` around lines 328 - 359, The multiple-upload test in AppsFormFileField is too loose because it only matches either uploaded ID instead of verifying the full joined result. Update the assertion in the joined-IDs case to check the exact value emitted by onChange, or split the returned string and verify both IDs and the array length; use the AppsFormFileField upload callback flow and mockOnChange call capture to ensure both uploaded IDs are preserved in order.Source: Coding guidelines
app/actions/remote/file.test.ts (1)
256-278: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the forwarded file IDs here.
These specs only verify the returned
files, so they would still pass iffetchFilesInfocalledgetFileInfowith the wrong IDs or duplicated one of them. AddtoHaveBeenNthCalledWith(or inspectmock.calls) for the requested IDs as well. As per coding guidelines, test expectations must match 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/actions/remote/file.test.ts` around lines 256 - 278, The fetchFilesInfo tests only check the returned files, so they do not verify that the requested file IDs are actually forwarded to mockClient.getFileInfo. Update the specs in file.test.ts around fetchFilesInfo to assert the exact call arguments using toHaveBeenNthCalledWith or mock.calls, alongside the existing order/skip assertions. Make sure the checks reference the fetchFilesInfo and getFileInfo flow so the test fails if IDs are duplicated, reordered, or replaced.Source: Coding guidelines
app/screens/apps_form/apps_form_component.tsx (1)
279-281: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the blocked submit early return.
This new no-op path is silent today, which makes stuck-submit reports harder to trace. As per coding guidelines, "Log on early returns in handlers with a logDebug call so the no-op is traceable, and add function/class prefix to logs..."
🤖 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/apps_form/apps_form_component.tsx` around lines 279 - 281, The early return in handleSubmit is currently silent when submitting is true or uploadingFields.size > 0, so add a logDebug call before returning to trace the blocked submit path. Use the AppsFormComponent/handleSubmit context in the log message so it clearly identifies the handler and includes the reason for the no-op. Keep the behavior unchanged otherwise and ensure the log happens only on this blocked-submit branch.Source: Coding guidelines
app/screens/apps_form/apps_form_file_field/apps_form_file_field.tsx (1)
82-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRead
serverUrlfrom context inside this component.Please avoid threading
serverUrlthroughAppsFormField/AppsFormComponentjust to reach this leaf. As per coding guidelines, "Use useServerUrl() hook instead of passing serverUrl as a prop - it's available via context."Also applies to: 161-162
🤖 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/apps_form/apps_form_file_field/apps_form_file_field.tsx` around lines 82 - 95, The Props for AppsFormFileField should not require serverUrl to be threaded down from AppsFormField/AppsFormComponent; read it directly from context instead. Update AppsFormFileField to use the useServerUrl() hook where serverUrl is currently consumed, and remove the serverUrl prop from the component’s public API and all call sites that pass it through.Source: Coding guidelines
app/utils/dialog_conversion.test.ts (1)
1005-1041: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the new test cases to the repo’s
should...format.These new
it(...)names skip the required prefix, so the suite is inconsistent with the repo’s test naming convention. As per coding guidelines,**/*.test.{ts,tsx}: Useit('should...')format for test names.Proposed rename pattern
- it('collects and trims comma-joined IDs from FILE elements', () => { + it('should collect and trim comma-joined IDs from FILE elements', () => { - it('collects across multiple FILE fields', () => { + it('should collect across multiple FILE fields', () => { - it('deduplicates repeated IDs', () => { + it('should deduplicate repeated IDs', () => { - it('ignores non-FILE elements and empty values', () => { + it('should ignore non-FILE elements and empty values', () => { - it('returns an empty array when elements is omitted', () => { + it('should return an empty array when elements is omitted', () => {🤖 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/utils/dialog_conversion.test.ts` around lines 1005 - 1041, The new collectFileIds test cases in dialog_conversion.test.ts should use the repo’s required should... naming convention. Update each existing it(...) description under the collectFileIds describe block to start with should while preserving the same behavior coverage, so the test names match the established pattern used across the suite.Source: Coding guidelines
app/utils/integrations.test.ts (1)
227-240: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename these cases to the repo’s
it('should...')form.These new FILE tests are the only added cases in this block that drift from the enforced Jest naming pattern. As per coding guidelines,
Use it('should...') format for test names.🤖 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/utils/integrations.test.ts` around lines 227 - 240, The new FILE-related Jest cases in integrations.test.ts use a different naming style than the repo standard. Rename the affected `it(...)` descriptions to the enforced `it('should...')` format for these FILE test cases, keeping the test logic and assertions unchanged in the `checkDialogElementForError` block.Source: Coding guidelines
app/screens/dialog_router/dialog_router.test.tsx (1)
121-343: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a multistep FILE regression case here.
These updates cover the new
channelIdprop, but they still do not exercise the new accumulated FILE-ID path. A case where step 2 clears or replaces a step 1 upload would catch mismatches betweensubmission[field]andfile_idsbefore this ships.🤖 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/dialog_router.test.tsx` around lines 121 - 343, Add a multistep FILE regression test in dialog_router.test.tsx that exercises the accumulated FILE-ID flow. Use DialogRouter with a mock config containing a FILE field, then simulate step 1 selecting/uploading a file and step 2 clearing or replacing it so the submitted values differ from the accumulated file_ids. Assert the submit path still sends the correct FILE-ID mapping through the DialogRouter submit handler and AppsFormComponent props, catching any mismatch between submission[field] and file_ids.
🤖 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/screens/apps_form/apps_form_file_field/apps_form_file_field.test.tsx`:
- Around line 462-508: The single-file replace spec is exercising a flow that
the UI currently blocks, so it can pass without proving the real behavior. In
AppsFormFileField, either update the choose-button gating in the file field
component so a second selection is actually allowed when allowMultiple is false,
or change this test to assert the second press really opens the picker before
reusing captureOnUploadFiles. Use the AppsFormFileField choose-button and
openAttachmentOptions path to locate the relevant behavior.
In `@app/screens/apps_form/apps_form_file_field/apps_form_file_field.tsx`:
- Around line 278-281: The upload flow in apps_form_file_field.tsx does not
handle the synchronous {error} return shape from uploadFile, so a failed upload
can leave the entry stuck in uploading. Update the logic around uploadFile in
the file upload handler to detect and process a returned error immediately,
using the existing onError/onComplete flow or equivalent state cleanup, and only
store result.cancel in cancelMapRef.current when a valid cancel handle is
present.
- Around line 176-185: The cleanup in AppsFormFileField’s useEffect only cancels
in-flight transports, so the field can remain stuck in AppsFormComponent’s
uploadingFields after unmount. Update the unmount path in AppsFormFileField to
also clear this field’s pending/uploading state before or after canceling
outstanding requests, using the existing field identity used by the upload
tracking logic so submit is re-enabled when the FILE field is removed.
- Around line 190-205: The hydration path in apps_form_file_field.tsx updates
only the local entries state, so missing/deleted file IDs can remain in the
submitted comma-separated value. After fetchFilesInfo resolves in the
useDidMount logic, derive the kept IDs from the returned files and sync the form
value through the existing onChange path when hydrating, while still respecting
isMountedRef and hasInteractedRef. Use uploadedEntryFromFileInfo and the current
value parsing/splitting logic to ensure any IDs filtered out by fetchFilesInfo
are removed from the submitted value.
In `@app/screens/dialog_router/dialog_router.tsx`:
- Around line 71-75: The current accumulatedFileIds state in dialog_router.tsx
is append-only, so it can keep stale FILE attachments when a later step clears
or replaces an earlier field. Update the DialogRouter flow to track retained
file IDs per field name (or derive them from the merged submission state)
instead of a single flat array, and make sure the final submit path uses that
field-aware source when building file_ids. Review the logic around
accumulatedFileIds, the per-step merge/update handlers, and the final submit
assembly so removed uploads are dropped correctly.
In `@detox/e2e/support/ui/screen/interactive_dialog.ts`:
- Around line 182-187: The check in expectFileFieldChooseButtonDisabled
currently uses an immediate not.toExist assertion, so it can miss a picker that
appears slightly later during the animation. Update the logic around
getFileFieldChooseButton and the file_attachment.photo_library element to use a
bounded waitFor(...).not.toExist() with an appropriate timeout after the tap, so
the test verifies the sheet stays hidden instead of only being absent at one
instant.
In
`@detox/e2e/test/products/channels/interactive_dialog/interactive_dialog_plugin.e2e.ts`:
- Around line 789-810: The test in InteractiveDialogScreen mutates shared server
config by toggling FileSettings.EnableMobileUpload, so if it fails the cleanup
never runs and later specs inherit bad state. Wrap the config changes in a
try/finally around the body of MM-T6072_1, and in the finally restore
EnableMobileUpload to true after any dialog cleanup so the state is always reset
even on failure.
- Around line 749-842: Remove the `it.only` guards from the three Detox cases in
`interactive_dialog_plugin.e2e.ts` so the full spec suite runs instead of
isolating only `MM-T6070_1`, `MM-T6072_1`, and `MM-T6073_1`; keep the test
bodies and assertions unchanged, just convert each `it.only(...)` back to a
normal `it(...)` so other tests in `interactive_dialog_plugin.e2e.ts` are no
longer skipped.
In `@detox/e2e/test/setup.ts`:
- Line 10: This setup file is still importing serverOneUrl and siteOneUrl from
the old test_config module, so it can diverge from the URLs resolved during
global setup. Update the import in setup.ts (and the related admin-login/debug
usage around the referenced lines) to use the new shared server_urls module so
the same resolved URLs are used everywhere, especially when SITE_1_API_URL or
LAN-based simulator URLs are in play.
---
Nitpick comments:
In `@app/actions/remote/file.test.ts`:
- Around line 256-278: The fetchFilesInfo tests only check the returned files,
so they do not verify that the requested file IDs are actually forwarded to
mockClient.getFileInfo. Update the specs in file.test.ts around fetchFilesInfo
to assert the exact call arguments using toHaveBeenNthCalledWith or mock.calls,
alongside the existing order/skip assertions. Make sure the checks reference the
fetchFilesInfo and getFileInfo flow so the test fails if IDs are duplicated,
reordered, or replaced.
In `@app/screens/apps_form/apps_form_component.tsx`:
- Around line 279-281: The early return in handleSubmit is currently silent when
submitting is true or uploadingFields.size > 0, so add a logDebug call before
returning to trace the blocked submit path. Use the
AppsFormComponent/handleSubmit context in the log message so it clearly
identifies the handler and includes the reason for the no-op. Keep the behavior
unchanged otherwise and ensure the log happens only on this blocked-submit
branch.
In `@app/screens/apps_form/apps_form_file_field/apps_form_file_field.test.tsx`:
- Around line 252-257: The test mocks in apps_form_file_field.test.tsx use
TypeScript-satisfaction guards around callback capture, which weakens the
contract being tested. Update the mock implementations for upload and related
handlers to capture callbacks directly with a typed mock signature or an
explicit assertion instead of using conditional checks like if (onComplete) or
if (onError). Apply the same cleanup in all affected mock blocks so the tests
rely on the real callback contract rather than optional-guarded behavior.
- Around line 328-359: The multiple-upload test in AppsFormFileField is too
loose because it only matches either uploaded ID instead of verifying the full
joined result. Update the assertion in the joined-IDs case to check the exact
value emitted by onChange, or split the returned string and verify both IDs and
the array length; use the AppsFormFileField upload callback flow and
mockOnChange call capture to ensure both uploaded IDs are preserved in order.
In `@app/screens/apps_form/apps_form_file_field/apps_form_file_field.tsx`:
- Around line 82-95: The Props for AppsFormFileField should not require
serverUrl to be threaded down from AppsFormField/AppsFormComponent; read it
directly from context instead. Update AppsFormFileField to use the
useServerUrl() hook where serverUrl is currently consumed, and remove the
serverUrl prop from the component’s public API and all call sites that pass it
through.
In `@app/screens/dialog_router/dialog_router.test.tsx`:
- Around line 121-343: Add a multistep FILE regression test in
dialog_router.test.tsx that exercises the accumulated FILE-ID flow. Use
DialogRouter with a mock config containing a FILE field, then simulate step 1
selecting/uploading a file and step 2 clearing or replacing it so the submitted
values differ from the accumulated file_ids. Assert the submit path still sends
the correct FILE-ID mapping through the DialogRouter submit handler and
AppsFormComponent props, catching any mismatch between submission[field] and
file_ids.
In `@app/utils/dialog_conversion.test.ts`:
- Around line 1005-1041: The new collectFileIds test cases in
dialog_conversion.test.ts should use the repo’s required should... naming
convention. Update each existing it(...) description under the collectFileIds
describe block to start with should while preserving the same behavior coverage,
so the test names match the established pattern used across the suite.
In `@app/utils/integrations.test.ts`:
- Around line 227-240: The new FILE-related Jest cases in integrations.test.ts
use a different naming style than the repo standard. Rename the affected
`it(...)` descriptions to the enforced `it('should...')` format for these FILE
test cases, keeping the test logic and assertions unchanged in the
`checkDialogElementForError` block.
🪄 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: 9546da8f-465e-4168-b8e5-b222fff12064
📒 Files selected for processing (32)
app/actions/remote/file.test.tsapp/actions/remote/file.tsapp/client/rest/files.test.tsapp/client/rest/files.tsapp/constants/apps.tsapp/screens/apps_form/apps_form_component.tsxapp/screens/apps_form/apps_form_field.tsxapp/screens/apps_form/apps_form_file_field/apps_form_file_field.test.tsxapp/screens/apps_form/apps_form_file_field/apps_form_file_field.tsxapp/screens/apps_form/apps_form_file_field/index.tsapp/screens/apps_form/index.tsxapp/screens/dialog_router/dialog_router.test.tsxapp/screens/dialog_router/dialog_router.tsxapp/screens/dialog_router/index.tsxapp/utils/dialog_conversion.test.tsapp/utils/dialog_conversion.tsapp/utils/dialog_utils.tsapp/utils/integrations.test.tsapp/utils/integrations.tsapp/utils/interactive_dialog_adapter.test.tsapp/utils/interactive_dialog_adapter.tsapp/utils/navigation/index.tsxassets/base/i18n/en.jsondetox/e2e/global_setup.jsdetox/e2e/support/server_api/plugin.tsdetox/e2e/support/server_urls.d.tsdetox/e2e/support/server_urls.jsdetox/e2e/support/ui/screen/interactive_dialog.tsdetox/e2e/test/products/channels/interactive_dialog/interactive_dialog_plugin.e2e.tsdetox/e2e/test/setup.tstypes/api/apps.d.tstypes/api/integrations.d.ts
mattermost-build
added
E2E/Run
Documentation Impact Analysis — updates neededDocumentation Impact AnalysisOverall Assessment: Documentation Updates Recommended Changes SummaryPR 9898 adds support for a new Documentation Impact Details
Recommended Actions
ConfidenceMedium — The mobile-app-changelog entry is high-confidence (new user-visible feature with new i18n strings). The integrations-guide recommendation is medium-confidence: primary Interactive Dialog developer documentation lives at
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/utils/dialog_conversion.ts`:
- Around line 40-42: The file merge logic in dialog conversion is using the
generic `in` operator, so inherited prototype keys can be treated as real
submitted fields. Update the `elements.forEach` block in
`app/utils/dialog_conversion.ts` to use an own-property check against
`submission` before assigning into `merged`, so only actual submitted file
fields are copied and reserved names like `toString`, `constructor`, or
`__proto__` are ignored.
🪄 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: 6650ec08-ee49-419e-b8b7-c3e08743ffce
📒 Files selected for processing (9)
app/screens/apps_form/apps_form_file_field/apps_form_file_field.test.tsxapp/screens/apps_form/apps_form_file_field/apps_form_file_field.tsxapp/screens/dialog_router/dialog_router.tsxapp/utils/dialog_conversion.test.tsapp/utils/dialog_conversion.tsassets/base/i18n/en.jsondetox/e2e/support/ui/screen/interactive_dialog.tsdetox/e2e/test/products/channels/interactive_dialog/interactive_dialog_plugin.e2e.tstypes/api/integrations.d.ts
💤 Files with no reviewable changes (1)
- types/api/integrations.d.ts
✅ Files skipped from review due to trivial changes (1)
- assets/base/i18n/en.json
🚧 Files skipped from review as they are similar to previous changes (6)
- app/utils/dialog_conversion.test.ts
- detox/e2e/support/ui/screen/interactive_dialog.ts
- app/screens/apps_form/apps_form_file_field/apps_form_file_field.test.tsx
- app/screens/dialog_router/dialog_router.tsx
- detox/e2e/test/products/channels/interactive_dialog/interactive_dialog_plugin.e2e.ts
- app/screens/apps_form/apps_form_file_field/apps_form_file_field.tsx
mattermost-build
added
E2E/Run
mattermost-build
removed
the
E2E/Run
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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
Ticket Link
Checklist
E2E/Run(orE2E/Run-iOS/E2E/Run-Androidfor platform-specific runs).Device Information
This PR was tested on:
Screenshots
Release Note