From 89756fe2b5bc62b5f69a5dbc6b19ff5a0e4c0777 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 29 Feb 2024 21:32:42 +0200 Subject: [PATCH 01/10] Use fork with updated expensify-common Showcase the functionality --- package-lock.json | 8 ++++---- package.json | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/package-lock.json b/package-lock.json index eeef54684fdf..ad326d5c61bf 100644 --- a/package-lock.json +++ b/package-lock.json @@ -52,7 +52,7 @@ "date-fns-tz": "^2.0.0", "dom-serializer": "^0.2.2", "domhandler": "^4.3.0", - "expensify-common": "git+ssh://git@github.com/Expensify/expensify-common.git#ee0f33865e3a27245e5cf01ed0c62cf88d44bb10", + "expensify-common": "git+ssh://git@github.com/kidroca/expensify-common.git#4111539e4086cc2e6d7cccaef2844603db86f399", "expo": "^50.0.3", "expo-av": "~13.10.4", "expo-image": "1.10.1", @@ -30570,8 +30570,8 @@ }, "node_modules/expensify-common": { "version": "1.0.0", - "resolved": "git+ssh://git@github.com/Expensify/expensify-common.git#ee0f33865e3a27245e5cf01ed0c62cf88d44bb10", - "integrity": "sha512-1w2PmSG12JxHxG6hYfBrGoJz3WqQGI9QgTVmRb9lDdy482ZH6ZtgOR8LURlPB0x6FjT+IKs6zbKQqE1eNXzE/g==", + "resolved": "git+ssh://git@github.com/kidroca/expensify-common.git#4111539e4086cc2e6d7cccaef2844603db86f399", + "integrity": "sha512-X8bVm+9pzr3Cu7hiLODfrZoinENQL+xJZhF/XCHGlgJ6buMKecFFGAIBQiTTqXBWP1WT67NyI4Yr++dvlXiHrA==", "license": "MIT", "dependencies": { "classnames": "2.5.0", @@ -47355,7 +47355,7 @@ "node_modules/simply-deferred": { "version": "3.0.0", "resolved": "git+ssh://git@github.com/Expensify/simply-deferred.git#77a08a95754660c7bd6e0b6979fdf84e8e831bf5", - "integrity": "sha512-DSHaVm+6KduK2+cOnaA+choq8CowotOpFhmHDCL36Cb/CM4f++TumOhcdWCknMwI3IbZbf3u2fzVPvb4UVf8UA==", + "integrity": "sha512-ozF7i2XEEBIcuRW+ThH+wmMeww//KEf1W3MhLHo4aOBmaxvZhBPpKoMCDmvNCIA5HPkkxe6x5XFGkSqHwCd6DQ==", "engines": { "node": "*" } diff --git a/package.json b/package.json index d5e733efdbd3..f008c567c261 100644 --- a/package.json +++ b/package.json @@ -103,7 +103,7 @@ "date-fns-tz": "^2.0.0", "dom-serializer": "^0.2.2", "domhandler": "^4.3.0", - "expensify-common": "git+ssh://git@github.com/Expensify/expensify-common.git#ee0f33865e3a27245e5cf01ed0c62cf88d44bb10", + "expensify-common": "git+ssh://git@github.com/kidroca/expensify-common.git#4111539e4086cc2e6d7cccaef2844603db86f399", "expo": "^50.0.3", "expo-av": "~13.10.4", "expo-image": "1.10.1", From 2192d033f4f5e2e2e96ffddfc2e2f6849bf5f80b Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Tue, 12 Mar 2024 18:53:29 +0200 Subject: [PATCH 02/10] Routes.REPORT_ATTACHMENTS: handle sources containing their own query parameters The `source` param can receive a URL containing query parameters, and they mess up the resulting source URL parameter --- src/ROUTES.ts | 2 +- src/pages/home/report/ReportAttachments.tsx | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/ROUTES.ts b/src/ROUTES.ts index 8de1757fc1b4..254688c500af 100644 --- a/src/ROUTES.ts +++ b/src/ROUTES.ts @@ -204,7 +204,7 @@ const ROUTES = { }, REPORT_ATTACHMENTS: { route: 'r/:reportID/attachment', - getRoute: (reportID: string, source: string) => `r/${reportID}/attachment?source=${encodeURI(source)}` as const, + getRoute: (reportID: string, source: string) => `r/${reportID}/attachment?source=${encodeURIComponent(source)}` as const, }, REPORT_PARTICIPANTS: { route: 'r/:reportID/participants', diff --git a/src/pages/home/report/ReportAttachments.tsx b/src/pages/home/report/ReportAttachments.tsx index 2c6ce7a2dabd..de145d5ef7e6 100644 --- a/src/pages/home/report/ReportAttachments.tsx +++ b/src/pages/home/report/ReportAttachments.tsx @@ -16,8 +16,7 @@ function ReportAttachments({route}: ReportAttachmentsProps) { const report = ReportUtils.getReport(reportID); // In native the imported images sources are of type number. Ref: https://reactnative.dev/docs/image#imagesource - const decodedSource = decodeURI(route.params.source); - const source = Number(decodedSource) || decodedSource; + const source = Number(route.params.source) || route.params.source; const onCarouselAttachmentChange = useCallback( (attachment: Attachment) => { From 9438e17d201550c2b67bb50e2018abb4c678ca15 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Tue, 12 Mar 2024 18:57:27 +0200 Subject: [PATCH 03/10] Extracting attachments: skip creating attachment items for duplicated sources Since sharing public images, users can share the same source multiple times, this results in problems like selecting the correct initial attachment if there are more than one matches based on source It makes sense for the list of attachments presented in the carousel to be unique, if the same thing is shared multiple times it's enough to present once instead of filling the carousel with duplicates --- .../extractAttachmentsFromReport.js | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/src/components/Attachments/AttachmentCarousel/extractAttachmentsFromReport.js b/src/components/Attachments/AttachmentCarousel/extractAttachmentsFromReport.js index b934bdfdd738..83b86a0a4692 100644 --- a/src/components/Attachments/AttachmentCarousel/extractAttachmentsFromReport.js +++ b/src/components/Attachments/AttachmentCarousel/extractAttachmentsFromReport.js @@ -15,10 +15,17 @@ import CONST from '@src/CONST'; function extractAttachmentsFromReport(parentReportAction, reportActions) { const actions = [parentReportAction, ...ReportActionsUtils.getSortedReportActions(_.values(reportActions))]; const attachments = []; + const uniqueSources = new Set(); const htmlParser = new HtmlParser({ onopentag: (name, attribs) => { if (name === 'video') { + const source = tryResolveUrlFromApiRoot(attribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE]); + if (uniqueSources.has(source)) { + return; + } + + uniqueSources.add(source); const splittedUrl = attribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE].split('/'); attachments.unshift({ reportActionID: null, @@ -35,7 +42,17 @@ function extractAttachmentsFromReport(parentReportAction, reportActions) { if (name === 'img' && attribs.src) { const expensifySource = attribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE]; const source = tryResolveUrlFromApiRoot(expensifySource || attribs.src); - const fileName = attribs[CONST.ATTACHMENT_ORIGINAL_FILENAME_ATTRIBUTE] || FileUtils.getFileName(`${source}`); + if (uniqueSources.has(source)) { + return; + } + + uniqueSources.add(source); + let fileName = attribs[CONST.ATTACHMENT_ORIGINAL_FILENAME_ATTRIBUTE] || FileUtils.getFileName(`${source}`); + + // image.jpg helps render the attachment as image if we didn't obtain an extension from the source + if (!/\.\w+$/.test(fileName)) { + fileName = 'image.jpg'; + } // By iterating actions in chronological order and prepending each attachment // we ensure correct order of attachments even across actions with multiple attachments. From fef1e1c71558f00c588e943b66e52a6c402a10df Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Tue, 19 Mar 2024 10:50:19 +0200 Subject: [PATCH 04/10] Fix: AttachmentView no fallback support for icons --- .../Attachments/AttachmentView/index.js | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/components/Attachments/AttachmentView/index.js b/src/components/Attachments/AttachmentView/index.js index 461548f0d2b1..e03b7c3640c1 100755 --- a/src/components/Attachments/AttachmentView/index.js +++ b/src/components/Attachments/AttachmentView/index.js @@ -201,6 +201,22 @@ function AttachmentView({ // We also check for numeric source since this is how static images (used for preview) are represented in RN. const isImage = typeof source === 'number' || Str.isImage(source); if (isImage || (file && Str.isImage(file.name))) { + + if (imageError) { + // AttachmentViewImage can't handle icon fallbacks, so we need to handle it here + if (typeof fallbackSource === 'number' || _.isFunction(fallbackSource)) { + return ( + + ); + } + } + return ( Date: Tue, 19 Mar 2024 11:05:42 +0200 Subject: [PATCH 05/10] Fix: AttachmentView add default fallback source --- src/components/Attachments/AttachmentView/index.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/components/Attachments/AttachmentView/index.js b/src/components/Attachments/AttachmentView/index.js index e03b7c3640c1..3208ac14d854 100755 --- a/src/components/Attachments/AttachmentView/index.js +++ b/src/components/Attachments/AttachmentView/index.js @@ -79,6 +79,7 @@ const defaultProps = { reportActionID: '', isHovered: false, optionalVideoDuration: 0, + fallbackSource: Expensicons.Gallery, }; function AttachmentView({ From 95473abb3d18aa3935d9b9dc2f0a1ffb9b924434 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Tue, 19 Mar 2024 15:41:12 +0200 Subject: [PATCH 06/10] Sync updates from expensify-common --- package-lock.json | 16 ++++++++-------- package.json | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/package-lock.json b/package-lock.json index f07eb02d1fcc..9696506c0d35 100644 --- a/package-lock.json +++ b/package-lock.json @@ -51,7 +51,7 @@ "date-fns-tz": "^2.0.0", "dom-serializer": "^0.2.2", "domhandler": "^4.3.0", - "expensify-common": "git+ssh://git@github.com/kidroca/expensify-common.git#4111539e4086cc2e6d7cccaef2844603db86f399", + "expensify-common": "git+ssh://git@github.com/kidroca/expensify-common.git#6a95384b6634e2b548036f94b9e18913a52e8847", "expo": "^50.0.3", "expo-av": "~13.10.4", "expo-image": "1.10.1", @@ -22986,9 +22986,9 @@ } }, "node_modules/classnames": { - "version": "2.4.0", - "resolved": "https://registry.npmjs.org/classnames/-/classnames-2.4.0.tgz", - "integrity": "sha512-lWxiIlphgAhTLN657pwU/ofFxsUTOWc2CRIFeoV5st0MGRJHStUnWIUJgDHxjUO/F0mXzGufXIM4Lfu/8h+MpA==" + "version": "2.5.0", + "resolved": "https://registry.npmjs.org/classnames/-/classnames-2.5.0.tgz", + "integrity": "sha512-FQuRlyKinxrb5gwJlfVASbSrDlikDJ07426TrfPsdGLvtochowmkbnSFdQGJ2aoXrSetq5KqGV9emvWpy+91xA==" }, "node_modules/clean-css": { "version": "5.3.2", @@ -27269,11 +27269,11 @@ }, "node_modules/expensify-common": { "version": "1.0.0", - "resolved": "git+ssh://git@github.com/kidroca/expensify-common.git#4111539e4086cc2e6d7cccaef2844603db86f399", - "integrity": "sha512-X8bVm+9pzr3Cu7hiLODfrZoinENQL+xJZhF/XCHGlgJ6buMKecFFGAIBQiTTqXBWP1WT67NyI4Yr++dvlXiHrA==", + "resolved": "git+ssh://git@github.com/kidroca/expensify-common.git#6a95384b6634e2b548036f94b9e18913a52e8847", + "integrity": "sha512-GT+AzYWkaovOwnWbg61kbt+3O36fyhHnIqN9KBcI/KUMvQNw2txY0T9kw8lu4XncN8RdSWhz/sWAne2ep6TnNw==", "license": "MIT", "dependencies": { - "classnames": "2.4.0", + "classnames": "2.5.0", "clipboard": "2.0.11", "html-entities": "^2.4.0", "jquery": "3.6.0", @@ -27282,7 +27282,7 @@ "prop-types": "15.8.1", "react": "16.12.0", "react-dom": "16.12.0", - "semver": "^7.5.2", + "semver": "^7.6.0", "simply-deferred": "git+https://github.com/Expensify/simply-deferred.git#77a08a95754660c7bd6e0b6979fdf84e8e831bf5", "ua-parser-js": "^1.0.37", "underscore": "1.13.6" diff --git a/package.json b/package.json index 4857eb0c52a3..82f37479f4cd 100644 --- a/package.json +++ b/package.json @@ -102,7 +102,7 @@ "date-fns-tz": "^2.0.0", "dom-serializer": "^0.2.2", "domhandler": "^4.3.0", - "expensify-common": "git+ssh://git@github.com/kidroca/expensify-common.git#4111539e4086cc2e6d7cccaef2844603db86f399", + "expensify-common": "git+ssh://git@github.com/kidroca/expensify-common.git#6a95384b6634e2b548036f94b9e18913a52e8847", "expo": "^50.0.3", "expo-av": "~13.10.4", "expo-image": "1.10.1", From f25773eb0643938ea2e3ec147d18a0e0bdefa239 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Tue, 19 Mar 2024 15:42:18 +0200 Subject: [PATCH 07/10] extractAttachmentsFromReport: code comments --- .../extractAttachmentsFromReport.js | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/components/Attachments/AttachmentCarousel/extractAttachmentsFromReport.js b/src/components/Attachments/AttachmentCarousel/extractAttachmentsFromReport.js index 83b86a0a4692..9524c5203110 100644 --- a/src/components/Attachments/AttachmentCarousel/extractAttachmentsFromReport.js +++ b/src/components/Attachments/AttachmentCarousel/extractAttachmentsFromReport.js @@ -15,6 +15,8 @@ import CONST from '@src/CONST'; function extractAttachmentsFromReport(parentReportAction, reportActions) { const actions = [parentReportAction, ...ReportActionsUtils.getSortedReportActions(_.values(reportActions))]; const attachments = []; + // We handle duplicate image sources by considering the first instance as original. Selecting any duplicate + // and navigating back (<) shows the image preceding the first instance, not the selected duplicate's position. const uniqueSources = new Set(); const htmlParser = new HtmlParser({ @@ -49,9 +51,12 @@ function extractAttachmentsFromReport(parentReportAction, reportActions) { uniqueSources.add(source); let fileName = attribs[CONST.ATTACHMENT_ORIGINAL_FILENAME_ATTRIBUTE] || FileUtils.getFileName(`${source}`); - // image.jpg helps render the attachment as image if we didn't obtain an extension from the source - if (!/\.\w+$/.test(fileName)) { - fileName = 'image.jpg'; + // Public image URLs might lack a file extension in the source URL, without an extension our + // AttachmentView fails to recognize them as images and renders fallback content instead. + // We apply this small hack to add an image extension and ensure AttachmentView renders the image. + const fileInfo = FileUtils.splitExtensionFromFileName(fileName); + if (!fileInfo.fileExtension) { + fileName = `${fileInfo.fileName || 'image'}.jpg`; } // By iterating actions in chronological order and prepending each attachment From bafe945c6d4e3dba55714f587fef0ef0d0add08b Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Tue, 19 Mar 2024 15:53:30 +0200 Subject: [PATCH 08/10] Apply prettier formatting --- src/components/Attachments/AttachmentView/index.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/components/Attachments/AttachmentView/index.js b/src/components/Attachments/AttachmentView/index.js index 3208ac14d854..9fe37734e8ee 100755 --- a/src/components/Attachments/AttachmentView/index.js +++ b/src/components/Attachments/AttachmentView/index.js @@ -202,7 +202,6 @@ function AttachmentView({ // We also check for numeric source since this is how static images (used for preview) are represented in RN. const isImage = typeof source === 'number' || Str.isImage(source); if (isImage || (file && Str.isImage(file.name))) { - if (imageError) { // AttachmentViewImage can't handle icon fallbacks, so we need to handle it here if (typeof fallbackSource === 'number' || _.isFunction(fallbackSource)) { From 2241c42e490a8da2c714a5efe50af1b5e4b89700 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Tue, 19 Mar 2024 23:33:24 +0200 Subject: [PATCH 09/10] Update to latest expensify-common --- package-lock.json | 21 ++++++++++++++------- package.json | 2 +- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/package-lock.json b/package-lock.json index 9696506c0d35..2e662668c880 100644 --- a/package-lock.json +++ b/package-lock.json @@ -51,7 +51,7 @@ "date-fns-tz": "^2.0.0", "dom-serializer": "^0.2.2", "domhandler": "^4.3.0", - "expensify-common": "git+ssh://git@github.com/kidroca/expensify-common.git#6a95384b6634e2b548036f94b9e18913a52e8847", + "expensify-common": "git+ssh://git@github.com/Expensify/expensify-common.git#ed80215f3e79e41c65f5d10cb22c439edbf90a57", "expo": "^50.0.3", "expo-av": "~13.10.4", "expo-image": "1.10.1", @@ -229,7 +229,6 @@ "react-test-renderer": "18.2.0", "reassure": "^0.10.1", "setimmediate": "^1.0.5", - "shellcheck": "^1.1.0", "style-loader": "^2.0.0", "time-analytics-webpack-plugin": "^0.1.17", "ts-node": "^10.9.2", @@ -21915,7 +21914,8 @@ "node_modules/boolean": { "version": "3.2.0", "dev": true, - "license": "MIT" + "license": "MIT", + "optional": true }, "node_modules/bottleneck": { "version": "2.19.5", @@ -26056,7 +26056,8 @@ "node_modules/es6-error": { "version": "4.1.1", "dev": true, - "license": "MIT" + "license": "MIT", + "optional": true }, "node_modules/es6-object-assign": { "version": "1.1.0", @@ -27269,8 +27270,8 @@ }, "node_modules/expensify-common": { "version": "1.0.0", - "resolved": "git+ssh://git@github.com/kidroca/expensify-common.git#6a95384b6634e2b548036f94b9e18913a52e8847", - "integrity": "sha512-GT+AzYWkaovOwnWbg61kbt+3O36fyhHnIqN9KBcI/KUMvQNw2txY0T9kw8lu4XncN8RdSWhz/sWAne2ep6TnNw==", + "resolved": "git+ssh://git@github.com/Expensify/expensify-common.git#ed80215f3e79e41c65f5d10cb22c439edbf90a57", + "integrity": "sha512-ZK3pvW/iNaS8nO5XNh1EeWYZ/Cx3kBHhsD0+GWELAyBiHd/q3VZhimUEFEBDjvxqg7VVVZRljkA8NZL0GLyLXQ==", "license": "MIT", "dependencies": { "classnames": "2.5.0", @@ -28848,6 +28849,7 @@ "version": "3.0.0", "dev": true, "license": "BSD-3-Clause", + "optional": true, "dependencies": { "boolean": "^3.0.1", "es6-error": "^4.1.1", @@ -34930,6 +34932,7 @@ "version": "3.0.0", "dev": true, "license": "MIT", + "optional": true, "dependencies": { "escape-string-regexp": "^4.0.0" }, @@ -41127,6 +41130,7 @@ "version": "2.15.4", "dev": true, "license": "BSD-3-Clause", + "optional": true, "dependencies": { "boolean": "^3.0.1", "detect-node": "^2.0.4", @@ -41142,7 +41146,8 @@ "node_modules/roarr/node_modules/sprintf-js": { "version": "1.1.2", "dev": true, - "license": "BSD-3-Clause" + "license": "BSD-3-Clause", + "optional": true }, "node_modules/run-node": { "version": "1.0.0", @@ -41412,6 +41417,7 @@ "version": "7.0.1", "dev": true, "license": "MIT", + "optional": true, "dependencies": { "type-fest": "^0.13.1" }, @@ -41426,6 +41432,7 @@ "version": "0.13.1", "dev": true, "license": "(MIT OR CC0-1.0)", + "optional": true, "engines": { "node": ">=10" }, diff --git a/package.json b/package.json index 82f37479f4cd..510366262da0 100644 --- a/package.json +++ b/package.json @@ -102,7 +102,7 @@ "date-fns-tz": "^2.0.0", "dom-serializer": "^0.2.2", "domhandler": "^4.3.0", - "expensify-common": "git+ssh://git@github.com/kidroca/expensify-common.git#6a95384b6634e2b548036f94b9e18913a52e8847", + "expensify-common": "git+ssh://git@github.com/Expensify/expensify-common.git#ed80215f3e79e41c65f5d10cb22c439edbf90a57", "expo": "^50.0.3", "expo-av": "~13.10.4", "expo-image": "1.10.1", From 111a6d8fc892253a992b9c59e3da0c8737e2f46f Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 21 Mar 2024 19:08:23 +0200 Subject: [PATCH 10/10] Run `npm i` and update package-lock.json --- package-lock.json | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/package-lock.json b/package-lock.json index 531142af3909..673e79bec8cf 100644 --- a/package-lock.json +++ b/package-lock.json @@ -21915,8 +21915,7 @@ "node_modules/boolean": { "version": "3.2.0", "dev": true, - "license": "MIT", - "optional": true + "license": "MIT" }, "node_modules/bottleneck": { "version": "2.19.5", @@ -26057,8 +26056,7 @@ "node_modules/es6-error": { "version": "4.1.1", "dev": true, - "license": "MIT", - "optional": true + "license": "MIT" }, "node_modules/es6-object-assign": { "version": "1.1.0", @@ -28850,7 +28848,6 @@ "version": "3.0.0", "dev": true, "license": "BSD-3-Clause", - "optional": true, "dependencies": { "boolean": "^3.0.1", "es6-error": "^4.1.1", @@ -34933,7 +34930,6 @@ "version": "3.0.0", "dev": true, "license": "MIT", - "optional": true, "dependencies": { "escape-string-regexp": "^4.0.0" }, @@ -41131,7 +41127,6 @@ "version": "2.15.4", "dev": true, "license": "BSD-3-Clause", - "optional": true, "dependencies": { "boolean": "^3.0.1", "detect-node": "^2.0.4", @@ -41147,8 +41142,7 @@ "node_modules/roarr/node_modules/sprintf-js": { "version": "1.1.2", "dev": true, - "license": "BSD-3-Clause", - "optional": true + "license": "BSD-3-Clause" }, "node_modules/run-node": { "version": "1.0.0", @@ -41418,7 +41412,6 @@ "version": "7.0.1", "dev": true, "license": "MIT", - "optional": true, "dependencies": { "type-fest": "^0.13.1" }, @@ -41433,7 +41426,6 @@ "version": "0.13.1", "dev": true, "license": "(MIT OR CC0-1.0)", - "optional": true, "engines": { "node": ">=10" },