Skip to content

feat: Preview files in new transfert#628

Open
aymericmariaux wants to merge 12 commits intomainfrom
preview-files-in-new-transfert
Open

feat: Preview files in new transfert#628
aymericmariaux wants to merge 12 commits intomainfrom
preview-files-in-new-transfert

Conversation

@aymericmariaux
Copy link
Copy Markdown

No description provided.

Copilot AI review requested due to automatic review settings April 21, 2026 13:39
@aymericmariaux aymericmariaux force-pushed the preview-files-in-new-transfert branch from f628c89 to 7dbc019 Compare April 21, 2026 13:43
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the file-details UI to support previewing (opening) locally picked files while creating a new transfer, while keeping download behavior for existing transfers.

Changes:

  • Replace isDownloadButtonVisible with an isNewTransfer flag across file list components/screens.
  • In “new transfer” mode, tapping a file now opens the local URI instead of triggering download UI.
  • Minor call-site refactors (parameter reordering, trailing lambda usage) in transfer screens.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
app/src/main/java/com/infomaniak/swisstransfer/ui/screen/newtransfer/filesdetails/NewTransferFilesDetailsScreen.kt Passes isNewTransfer = true into the shared files details UI for new transfers.
app/src/main/java/com/infomaniak/swisstransfer/ui/screen/main/transfers/TransfersScreenWrapper.kt Adjusts argument passing and refactors ExistingTransferFilesDetailsScreen invocation.
app/src/main/java/com/infomaniak/swisstransfer/ui/screen/main/transferdetails/components/ExistingTransferFilesDetailsScreen.kt Updates to isNewTransfer = false for existing transfers; adjusts parameter order.
app/src/main/java/com/infomaniak/swisstransfer/ui/screen/main/transferdetails/TransferDetailsScreen.kt Updates file list to isNewTransfer = false in transfer details.
app/src/main/java/com/infomaniak/swisstransfer/ui/components/transfer/FilesDetailsScreen.kt Updates API to require withFileSize/withSpaceLeft/isNewTransfer; forwards to FileItemList.
app/src/main/java/com/infomaniak/swisstransfer/ui/components/FileItemList.kt Implements “open local file” click behavior for new transfers; adjusts list API.
app/src/main/java/com/infomaniak/swisstransfer/ui/components/FileItem.kt Minor parameter reordering in composables.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread app/src/main/java/com/infomaniak/swisstransfer/ui/components/FileItemList.kt Outdated
@aymericmariaux aymericmariaux changed the title Preview files in new transfert feat: Preview files in new transfert Apr 21, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 21, 2026

PR Reviewer Guide 🔍

(Review updated until commit b547669)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 Security concerns

File URI Permission Issue:
The openLocalFile function grants FLAG_GRANT_READ_URI_PERMISSION to external apps when opening files. This flag is only effective for content:// URIs. If file.uid is a raw file path that gets converted to a file:// URI, the permission flag is ignored on Android N+ (API 24+), potentially causing FileUriExposedException crashes, or on older versions, exposing the file without proper permission management. Ensure the URI passed to this function is a content URI (e.g., from FileProvider or Storage Access Framework). The use of safeStartActivity correctly mitigates ActivityNotFoundException risks when no app can handle the file type.

⚡ Recommended focus areas for review

Incorrect URI Source

The code uses file.uid.toUri() to open local files for preview when isNewTransfer is true. If file.uid is a unique identifier (e.g., UUID) rather than a file path or content URI string, this creates an invalid URI and the file will fail to open. The field file.localPath (referenced in the produceState initial value) is likely the correct source for the file location. Verify that uid contains the actual file URI or use the appropriate path field with FileProvider if exposing to external apps.

isNewTransfer -> fun() { context.openLocalFile(file.uid.toUri()) }

@github-actions
Copy link
Copy Markdown

Failed to generate code suggestions for PR

@github-actions
Copy link
Copy Markdown

Persistent review updated to latest commit 69f7085

@github-actions
Copy link
Copy Markdown

Persistent review updated to latest commit b547669

Comment thread app/src/main/java/com/infomaniak/swisstransfer/ui/components/FileItemList.kt Outdated
Comment thread app/src/main/java/com/infomaniak/swisstransfer/ui/components/FileItemList.kt Outdated
@github-actions
Copy link
Copy Markdown

Failed to generate code suggestions for PR

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread app/src/main/java/com/infomaniak/swisstransfer/ui/components/FileItemList.kt Outdated
Comment thread app/src/main/java/com/infomaniak/swisstransfer/ui/components/FileItemList.kt Outdated
@github-actions
Copy link
Copy Markdown

Failed to generate code suggestions for PR

1 similar comment
@github-actions
Copy link
Copy Markdown

Failed to generate code suggestions for PR

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (1)

app/src/main/java/com/infomaniak/swisstransfer/ui/components/FileItemList.kt:140

  • Inside produceState, value = uri.toString() will turn a null Uri? into the literal string "null". That makes the UI treat it as a real thumbnail URL/URI and will trigger a failed image load before falling back. Prefer keeping the state null when the flow emits null so the preview logic can short-circuit cleanly.
                previewUriForFile = produceState(file.thumbnailPath ?: file.localPath) {
                    transferFlow.collectLatest { transfer ->
                        previewUriForFile(transfer, file).collect { uri: Uri? ->
                            value = uri.toString()
                        }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread app/src/main/java/com/infomaniak/swisstransfer/ui/components/FileItemList.kt Outdated
Comment thread app/src/main/java/com/infomaniak/swisstransfer/ui/components/FileItemList.kt Outdated
Comment thread app/src/main/java/com/infomaniak/swisstransfer/ui/components/FileItemList.kt Outdated
@github-actions
Copy link
Copy Markdown

Failed to generate code suggestions for PR

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

app/src/main/java/com/infomaniak/swisstransfer/ui/components/FileItemList.kt:148

  • uri is nullable here, but uri.toString() will produce the literal string "null" when uri == null, which then gets treated as a real thumbnail URI (and will trigger a failed image load). Set the produced state to null when the flow emits null (e.g., use uri?.toString()).
                previewUriForFile = produceState(file.thumbnailPath ?: file.localPath) {
                    transferFlow.collectLatest { transfer ->
                        previewUriForFile(transfer, file).collect { uri: Uri? ->
                            value = uri.toString()
                        }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@aymericmariaux aymericmariaux force-pushed the preview-files-in-new-transfert branch from 206c8dd to 6e3e58f Compare April 24, 2026 09:03
@sonarqubecloud
Copy link
Copy Markdown

@aymericmariaux aymericmariaux requested a review from LouisCAD April 24, 2026 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants