feat/add image viewer#162
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces media sharing and viewing capabilities across desktop, iOS, and Android platforms. It adds a new Tauri iOS share-sheet plugin, implements platform-specific file downloading functionality, introduces media viewer and commenting components, and provides supporting utilities and tests. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Frontend as Frontend (React)
participant TauriBackend as Tauri Backend
participant Platform as Platform APIs
participant FileSystem as File System
User->>Frontend: Trigger download_image(url)
Frontend->>TauriBackend: Invoke download_image command
alt Desktop Path
TauriBackend->>Platform: Fetch URL via reqwest
Platform-->>TauriBackend: Image bytes
TauriBackend->>TauriBackend: Open file save dialog (rfd)
User->>TauriBackend: Select save location
TauriBackend->>FileSystem: Write bytes to selected path
FileSystem-->>TauriBackend: Success
else iOS Path
TauriBackend->>Platform: Fetch URL via reqwest
Platform-->>TauriBackend: Image bytes
TauriBackend->>FileSystem: Write to temp file
FileSystem-->>TauriBackend: Temp file path
TauriBackend->>Platform: Present UIActivityViewController (share sheet)
Platform->>User: Share options
else Android Path
TauriBackend->>Platform: Fetch URL via reqwest
Platform-->>TauriBackend: Image bytes
TauriBackend->>TauriBackend: Open save-file picker dialog
User->>TauriBackend: Select save location
alt Content URI
TauriBackend->>TauriBackend: Write via ContentResolver (JNI)
TauriBackend->>Platform: JNI call to write bytes
else File Path
TauriBackend->>FileSystem: Write bytes to path
end
FileSystem-->>TauriBackend: Success
end
TauriBackend-->>Frontend: Download complete / path
Frontend->>User: Show success toast
sequenceDiagram
actor User
participant MessageItem as Message Component
participant MediaViewer as MediaViewerModal
participant Comments as MediaCommentsSidebar
participant Store as Message Store
User->>MessageItem: Click image
MessageItem->>MediaViewer: Open modal with url/channelId
MediaViewer->>Store: Fetch server/channel messages
Store-->>MediaViewer: Image entries + comments
MediaViewer->>MediaViewer: Build filmstrip gallery
MediaViewer->>User: Display viewer with controls
User->>MediaViewer: Click comments button
MediaViewer->>Comments: Show sidebar
Comments->>Store: Query messages with reply tags
Store-->>Comments: Filtered reply messages
Comments->>User: Display comments list
User->>Comments: Type comment + press Enter
Comments->>Comments: Build reply with sourceMessage tag
Comments->>Store: Send message
Store-->>Comments: Message added
Comments->>MediaViewer: Update comment count
MediaViewer->>User: Refresh filmstrip indicator
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes This review spans multiple languages (Swift, Rust, TypeScript), introduces platform-specific code paths with varying complexity, includes substantial new React components with intricate state management, and requires verification of cross-platform integration points and file handling logic. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can customize the tone of the review comments and chat replies.Configure the |
|
Automated deployment preview for the PR in the Cloudflare Pages. |
![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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/message/MessageItem.tsx (1)
192-201:⚠️ Potential issue | 🟠 MajorDon't mount the image on an unresolved Tenor share URL.
For
tenor.com/view/..., the first render still uses the share page becausedisplayUrlfalls back tourl. If that request errors beforeresolveTenorUrl()finishes,imageErroris set and the resolved GIF never gets a chance to load.Also applies to: 289-333
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/message/MessageItem.tsx` around lines 192 - 201, processUrl currently mounts the image immediately for tenor.com/view/... by leaving displayUrl === url which can trigger imageError before resolveTenorUrl finishes; change logic in processUrl (and the analogous block around displayUrl handling at lines 289-333) to not set displayUrl or start loading the image for Tenor share URLs until resolveTenorUrl resolves: for URLs matching /tenor\.com\/view\//, await resolveTenorUrl(url) and only then call setResolvedUrl and update displayUrl (and clear/reset imageError via setImageError(false) after resolving) so the initial render does not attempt to load the share page image or set imageError prematurely.
🧹 Nitpick comments (2)
src-tauri/src/lib.rs (1)
33-33: Synchronous file I/O in async context.
std::fs::writeis blocking I/O that could stall the async runtime's executor thread on large files. Consider wrapping intokio::task::spawn_blockingfor better throughput.Also applies to: 46-46, 75-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/lib.rs` at line 33, The code uses blocking std::fs::write calls (e.g., the call shown and the other occurrences at the comment locations) inside async context; replace each synchronous write with a non-blocking pattern by moving the write into tokio::task::spawn_blocking (or switch to tokio::fs::write) and await the result, preserving the existing error mapping (map_err(|e| e.to_string())?) and using the same inputs (file.path() and bytes) inside the closure; update the surrounding function (where std::fs::write is called) to be async if needed so you can .await the spawn_blocking result and handle any join or IO errors consistently for all three occurrences.src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Logger.swift (1)
9-25: MakeStdoutRedirector.start()idempotent.Repeated calls can re-dup descriptors and rewire pipes multiple times. Add a one-time guard to prevent accidental reinitialization.
♻️ Proposed hardening
class StdoutRedirector { + private var isStarted: Bool = false private var originalStdout: Int32 = -1 private var originalStderr: Int32 = -1 @@ func start() { + guard !isStarted else { return } + isStarted = true originalStdout = dup(STDOUT_FILENO) originalStderr = dup(STDERR_FILENO) @@ guard Darwin.pipe(&stdoutPipe) == 0, Darwin.pipe(&stderrPipe) == 0 else { + isStarted = false Logger.error("Failed to create stdout/stderr pipes") return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Logger.swift` around lines 9 - 25, Make StdoutRedirector.start() idempotent by returning early if redirection is already active (e.g., check originalStdout/originalStderr != -1 or stdoutReadSource/stderrReadSource != nil) so you don't dup fds or create pipes multiple times; only perform dup(STDOUT_FILENO)/dup(STDERR_FILENO), pipe(&stdoutPipe)/pipe(&stderrPipe) and set up DispatchSourceRead when the redirector is not yet started, and leave existing descriptors/pipes unchanged on subsequent calls (or log/ignore repeated starts).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Invoke.swift`:
- Around line 33-38: The function getArgs() currently force-unwraps
self.data.data(using: .utf8), force-casts the JSON result to NSDictionary, and
force-unwraps the JSTypes.coerceDictionaryToJSObject result which can crash on
malformed input; change it to safe guards that throw descriptive errors instead:
check that self.data.data(using: .utf8) returns non-nil (throw a decoding
error), use JSONSerialization to get an Any and safely cast it to NSDictionary
(throw a parse/type error if the cast fails), call
JSTypes.coerceDictionaryToJSObject without force-unwrapping and throw if it
returns nil, and propagate/annotate these thrown errors from getArgs() so
callers can handle malformed payloads instead of the app crashing.
- Around line 87-100: The payload is storing a raw Error object
(payload["error"] = error) which is not JSON-serializable; update the reject
payload construction in Invoke.swift to replace the raw Error with a JSON-safe
representation (for example error.localizedDescription or a dictionary with
"message" and optional "code"/"domain"), ensure payload is built as a JsonObject
(avoid force-casting with as! by constructing a proper [String: JsonValue] or
using existing JsonObject helpers), and then call sendResponse(self.error,
serialize(.dictionary(payload))) using that JSON-safe payload so the reject
remains a structured JSON object.
In `@src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Tauri.swift`:
- Around line 41-45: The onWebviewCreated method currently calls
handle.instance.load(webview:) for deferred plugins but never updates the
plugin's loaded flag; after calling handle.instance.load(webview:) set
handle.loaded = true so the plugin is not loaded again on subsequent
on_webview_created events—update the loop in onWebviewCreated to mark each
handle as loaded immediately after invoking handle.instance.load(webview:).
- Around line 78-90: The cast for methodIMP currently uses OpaquePointer causing
NSError instances to be retained; change the unsafeBitCast signature to use an
NSError out-parameter: use (`@convention`(c) (Any?, Selector, Invoke,
AutoreleasingUnsafeMutablePointer<NSError?>) -> Void) and pass
AutoreleasingUnsafeMutablePointer($0) when invoking the function (keeping var
error: NSError? and the withUnsafeMutablePointer(to: &error) block), then remove
the Unmanaged.passRetained(error) line so the autoreleased NSError is not leaked
and simply call invoke.reject when error != nil; this touches plugin.instance,
selectorWithThrows, methodIMP and Invoke.
- Around line 61-99: The plugin invocation runs on ipcDispatchQueue (background)
so any handler that touches UIKit/WebKit must re-dispatch to the main actor;
update the plugin handler implementations (the methods matched by
selectorWithCompletionHandler, selectorWithThrows, and the simple selector for
invoke.command on plugin.instance) to wrap UI work with await MainActor.run {
... } or DispatchQueue.main.async { ... } before accessing UIViewController,
WKWebView, or other main-thread-only APIs, and add a short comment next to those
selector implementations reminding future authors to perform UI interactions on
the main thread.
In `@src-tauri/src/lib.rs`:
- Around line 114-129: The code calls resolver.openOutputStream and then
write/close on the returned stream without checking for a null Java object or
JNI exceptions; update the block that creates and uses `stream` (the result of
`env.call_method(... "openOutputStream" ...)` and `.l()`) to: 1) check the
returned `JObject` for null (e.g., test `stream.is_null()` or equivalent) and
return a descriptive Err if null, 2) after each JNI call (openOutputStream,
byte_array_from_slice, stream.write, stream.close) check `env.exception_check()`
and convert any pending Java exception into an Err (optionally clear the
exception with `env.exception_describe()`/`env.exception_clear()` before
mapping), and 3) return early on errors so you never call methods on a null
`stream`; apply these changes around the `openOutputStream`/`stream` variable
and the `write`/`close` calls to ensure safe null and exception handling.
In `@src/components/message/SwipeableMessage.tsx`:
- Around line 121-126: When skipping long-press detection in the
onTouchStartCapture handler of SwipeableMessage because a lightbox overlay is
present, clear the touch state so later onTouchEndCapture doesn't reuse stale
values: set touchStartTargetRef.current = null and set hasMovedRef.current =
true (so it won’t be considered a tap), then return; update the
onTouchStartCapture block that currently checks (e.target as
Element).closest?.("[data-lightbox-overlay]") to perform these resets before
returning.
In `@src/components/ui/ImageLightboxModal.tsx`:
- Around line 328-334: The blurred backdrop is using the stale `url` variable so
it doesn't update when navigating images; update the inline style to derive its
backgroundImage from the current active image (the same source used to render
the main image) — e.g., reference the component's active image variable (like
images[activeIndex].url or currentImage.url) inside ImageLightboxModal so the
backdrop uses the same dynamic value and changes when prev/next handlers update
the active index; ensure you don't capture an old `url` in a closure (use
render-time value or a derived state like `backdropUrl` updated alongside the
active image).
- Around line 87-94: The image list and currentIndex should be built from the
normalized/display URLs that the UI actually renders (same logic used by
ImageWithFallback and MessageItem rendering of GIF/share links) rather than the
raw results of extractImageUrlsFromMessage; update the useEffect block that
calls extractImageUrlsFromMessage to map each raw URL through the same
normalization/resolution routine used when images are rendered (the
share-link-to-direct-image conversion used by ImageWithFallback/MessageItem,
including special handling for tenor.googleapis.com and /tinygif variants) and
then call setImageList(normalizedUrls) and
setCurrentIndex(normalizedUrls.indexOf(url)); this ensures currentIndex is not
-1 for resolved share links and navigation uses direct image URLs.
- Around line 103-120: The key handler in handleKey should early-return when the
key event comes from interactive controls or the nested ExternalLinkWarningModal
to avoid hijacking the slider or closing the nested modal: update handleKey to
ignore events whose target or document.activeElement is an interactive element
(INPUT, TEXTAREA, SELECT, BUTTON or contentEditable=true — this covers the range
input/slider) and also ignore when the event originates from inside the
ExternalLinkWarningModal DOM (use element.closest on a class/selector used by
that modal), then continue to handle Escape, ArrowLeft/ArrowRight and call
onClose or goTo only when those guards do not apply.
---
Outside diff comments:
In `@src/components/message/MessageItem.tsx`:
- Around line 192-201: processUrl currently mounts the image immediately for
tenor.com/view/... by leaving displayUrl === url which can trigger imageError
before resolveTenorUrl finishes; change logic in processUrl (and the analogous
block around displayUrl handling at lines 289-333) to not set displayUrl or
start loading the image for Tenor share URLs until resolveTenorUrl resolves: for
URLs matching /tenor\.com\/view\//, await resolveTenorUrl(url) and only then
call setResolvedUrl and update displayUrl (and clear/reset imageError via
setImageError(false) after resolving) so the initial render does not attempt to
load the share page image or set imageError prematurely.
---
Nitpick comments:
In `@src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Logger.swift`:
- Around line 9-25: Make StdoutRedirector.start() idempotent by returning early
if redirection is already active (e.g., check originalStdout/originalStderr !=
-1 or stdoutReadSource/stderrReadSource != nil) so you don't dup fds or create
pipes multiple times; only perform dup(STDOUT_FILENO)/dup(STDERR_FILENO),
pipe(&stdoutPipe)/pipe(&stderrPipe) and set up DispatchSourceRead when the
redirector is not yet started, and leave existing descriptors/pipes unchanged on
subsequent calls (or log/ignore repeated starts).
In `@src-tauri/src/lib.rs`:
- Line 33: The code uses blocking std::fs::write calls (e.g., the call shown and
the other occurrences at the comment locations) inside async context; replace
each synchronous write with a non-blocking pattern by moving the write into
tokio::task::spawn_blocking (or switch to tokio::fs::write) and await the
result, preserving the existing error mapping (map_err(|e| e.to_string())?) and
using the same inputs (file.path() and bytes) inside the closure; update the
surrounding function (where std::fs::write is called) to be async if needed so
you can .await the spawn_blocking result and handle any join or IO errors
consistently for all three occurrences.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f6b3ccab-74b0-462a-9bd4-1938ec9a74cf
⛔ Files ignored due to path filters (2)
src-tauri/Cargo.lockis excluded by!**/*.locksrc-tauri/gen/apple/ObsidianIRC_iOS/Info.plistis excluded by!**/gen/**
📒 Files selected for processing (28)
src-tauri/Cargo.tomlsrc-tauri/plugins/share-sheet/.tauri/tauri-api/.gitignoresrc-tauri/plugins/share-sheet/.tauri/tauri-api/Package.swiftsrc-tauri/plugins/share-sheet/.tauri/tauri-api/README.mdsrc-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Channel.swiftsrc-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Invoke.swiftsrc-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/JSTypes.swiftsrc-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/JsonValue.swiftsrc-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Logger.swiftsrc-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Plugin/Plugin.swiftsrc-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Tauri.swiftsrc-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/UiUtils.swiftsrc-tauri/plugins/share-sheet/Cargo.tomlsrc-tauri/plugins/share-sheet/build.rssrc-tauri/plugins/share-sheet/ios/Package.resolvedsrc-tauri/plugins/share-sheet/ios/Package.swiftsrc-tauri/plugins/share-sheet/ios/Sources/ShareSheet/ShareSheet.swiftsrc-tauri/plugins/share-sheet/src/lib.rssrc-tauri/src/lib.rssrc/components/message/MessageItem.tsxsrc/components/message/SwipeableMessage.tsxsrc/components/ui/ImageLightboxModal.tsxsrc/lib/imageUtils.tssrc/lib/modal/components/Button.tsxsrc/lib/settings/definitions/allSettings.tssrc/store/localStorage.tstests/components/ImageLightboxModal.test.tsxvite.config.ts
💤 Files with no reviewable changes (1)
- vite.config.ts
Comment on lines
+61
to
+99
| ipcDispatchQueue.async { | ||
| let selectorWithCompletionHandler = Selector(("\(invoke.command):completionHandler:")) | ||
| let selectorWithThrows = Selector(("\(invoke.command):error:")) | ||
|
|
||
| if plugin.instance.responds(to: selectorWithCompletionHandler) { | ||
| let completion: @convention(block) (NSError?) -> Void = { error in | ||
| if let error = error { | ||
| invoke.reject("\(error)") | ||
| } | ||
| } | ||
|
|
||
| let blockObj: AnyObject = unsafeBitCast(completion, to: AnyObject.self) | ||
| let imp = plugin.instance.method(for: selectorWithCompletionHandler) | ||
|
|
||
| typealias Fn = @convention(c) (AnyObject, Selector, Invoke, AnyObject) -> Void | ||
| let fn = unsafeBitCast(imp, to: Fn.self) | ||
| fn(plugin.instance, selectorWithCompletionHandler, invoke, blockObj) | ||
| } else if plugin.instance.responds(to: selectorWithThrows) { | ||
| var error: NSError? = nil | ||
| withUnsafeMutablePointer(to: &error) { | ||
| let methodIMP: IMP! = plugin.instance.method(for: selectorWithThrows) | ||
| unsafeBitCast( | ||
| methodIMP, to: (@convention(c) (Any?, Selector, Invoke, OpaquePointer) -> Void).self)( | ||
| plugin.instance, selectorWithThrows, invoke, OpaquePointer($0)) | ||
| } | ||
| if let error = error { | ||
| invoke.reject("\(error)") | ||
| // TODO: app crashes without this leak | ||
| let _ = Unmanaged.passRetained(error) | ||
| } | ||
| } else { | ||
| let selector = Selector(("\(invoke.command):")) | ||
| if plugin.instance.responds(to: selector) { | ||
| plugin.instance.perform(selector, with: invoke) | ||
| } else { | ||
| invoke.reject("No command \(invoke.command) found for plugin \(name)") | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Do UIKit APIs such as UIViewController.present(_:animated:completion:)andWKWebView interactions need to run on the main thread / main actor on iOS?
💡 Result:
Yes.
-
UIKit (including
UIViewController.present(...)): UIKit APIs are generally not thread-safe and are intended to be used from the main thread / main dispatch queue (“All UIKit objects should be used on the main thread only.”) [1]. In modern Swift concurrency terms, many UIKit types/APIs are annotated@MainActor(for exampleUIPageViewControllerandUIViewanimation/transition APIs), which means calls must be made while isolated to the main actor [2], [3]. Practically, treatpresent(_:animated:completion:)as main-thread/main-actor only. -
WebKit /
WKWebViewinteractions:WKWebView’s public UI-facing methods are also expected to be called on the main thread (for example, developers commonly hit runtime diagnostics like “WKWebView.load(_:)must be used from main thread only”) [4]. WebKit also exposes related entry points annotated@MainActor(for exampleWKURLSchemeHandler) [5], aligning with the general rule that UI/web view interactions belong on the main actor.
If you’re in background work, hop back with await MainActor.run { ... } (or DispatchQueue.main.async { ... }) before calling these APIs. [1] [2] [3] [4] [5]
Sources: [1] Apple “Adding Behavior to a Cocoa Program” (Threading) https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/CocoaFundamentals/AddingBehaviortoaCocoaProgram/AddingBehaviorCocoa.html • [2] Apple UIPageViewController docs (annotated @MainActor) https://developer.apple.com/documentation/uikit/uipageviewcontroller • [3] Apple UIView.transition(...) docs (annotated @MainActor) https://developer.apple.com/documentation/uikit/uiview/transition%28with%3Aduration%3Aoptions%3Aanimations%3Acompletion%3A%29 • [4] Apple Developer Forums thread showing main-thread-only diagnostics for WKWebView.load https://developer.apple.com/forums/thread/713290 • [5] Apple WKURLSchemeHandler docs (annotated @MainActor) https://developer.apple.com/documentation/webkit/wkurlschemehandler
Ensure plugin handlers re-dispatch UI operations to the main actor.
Plugin commands dispatched to ipcDispatchQueue run on a background thread. If any handler presents a UI view controller, touches UIViewController, or interacts with WKWebView, it must re-dispatch to the main actor using await MainActor.run { ... } or DispatchQueue.main.async { ... } before calling those APIs—UIKit and WebKit require main-thread execution and are not thread-safe.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Tauri.swift`
around lines 61 - 99, The plugin invocation runs on ipcDispatchQueue
(background) so any handler that touches UIKit/WebKit must re-dispatch to the
main actor; update the plugin handler implementations (the methods matched by
selectorWithCompletionHandler, selectorWithThrows, and the simple selector for
invoke.command on plugin.instance) to wrap UI work with await MainActor.run {
... } or DispatchQueue.main.async { ... } before accessing UIViewController,
WKWebView, or other main-thread-only APIs, and add a short comment next to those
selector implementations reminding future authors to perform UI interactions on
the main thread.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/ui/ImageLightboxModal.tsx`:
- Around line 276-290: The catch in handleDownload currently only logs errors to
console and leaves the user without feedback; update the catch to surface the
failure to the UI by setting a user-visible message (reuse setSavedMessage or
add a new state like setErrorMessage) with the caught error details (include
error.message) and clear it after the same timeout used for success, so users
see a persistent error notice when download_image invoke fails; keep the finally
block that calls setIsDownloading(false) intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cf8724d0-03da-4ded-9256-07a2761f25d6
📒 Files selected for processing (2)
src/components/ui/ImageLightboxModal.tsxtests/components/ImageLightboxModal.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/components/ImageLightboxModal.test.tsx
Comment on lines
+276
to
+290
| const handleDownload = async () => { | ||
| setIsDownloading(true); | ||
| try { | ||
| const { invoke } = await import("@tauri-apps/api/core"); | ||
| const msg = await invoke<string>("download_image", { url: currentUrl }); | ||
| if (msg) { | ||
| setSavedMessage(msg); | ||
| setTimeout(() => setSavedMessage(""), 4000); | ||
| } | ||
| } catch { | ||
| console.error("Download failed"); | ||
| } finally { | ||
| setIsDownloading(false); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
Provide user feedback when download fails.
The catch block logs to console but doesn't inform the user. When the download fails, the spinner stops and nothing else happens, leaving the user unsure if anything went wrong.
💡 Suggested improvement
} catch {
- console.error("Download failed");
+ console.error("Download failed");
+ setSavedMessage("Download failed");
+ setTimeout(() => setSavedMessage(""), 4000);
} finally {📝 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 handleDownload = async () => { | |
| setIsDownloading(true); | |
| try { | |
| const { invoke } = await import("@tauri-apps/api/core"); | |
| const msg = await invoke<string>("download_image", { url: currentUrl }); | |
| if (msg) { | |
| setSavedMessage(msg); | |
| setTimeout(() => setSavedMessage(""), 4000); | |
| } | |
| } catch { | |
| console.error("Download failed"); | |
| } finally { | |
| setIsDownloading(false); | |
| } | |
| }; | |
| const handleDownload = async () => { | |
| setIsDownloading(true); | |
| try { | |
| const { invoke } = await import("@tauri-apps/api/core"); | |
| const msg = await invoke<string>("download_image", { url: currentUrl }); | |
| if (msg) { | |
| setSavedMessage(msg); | |
| setTimeout(() => setSavedMessage(""), 4000); | |
| } | |
| } catch { | |
| console.error("Download failed"); | |
| setSavedMessage("Download failed"); | |
| setTimeout(() => setSavedMessage(""), 4000); | |
| } finally { | |
| setIsDownloading(false); | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/ImageLightboxModal.tsx` around lines 276 - 290, The catch
in handleDownload currently only logs errors to console and leaves the user
without feedback; update the catch to surface the failure to the UI by setting a
user-visible message (reuse setSavedMessage or add a new state like
setErrorMessage) with the caught error details (include error.message) and clear
it after the same timeout used for success, so users see a persistent error
notice when download_image invoke fails; keep the finally block that calls
setIsDownloading(false) intact.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/ui/ImageLightboxModal.tsx`:
- Around line 532-534: The map over imageList in ImageLightboxModal uses
thumbUrl as the React key which can collide when URLs repeat; change the key on
the <button> rendered inside imageList.map (refer to the map callback using
thumbUrl and thumbIndex) to a composite unique value (for example combine
thumbUrl and thumbIndex or use `\`${thumbUrl}-${thumbIndex}\``) so duplicates
won't produce React key warnings or unstable element identity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 319c73ed-3214-48b7-8e85-848524d54891
📒 Files selected for processing (1)
src/components/ui/ImageLightboxModal.tsx
Comment on lines
+532
to
+534
| {imageList.map((thumbUrl, thumbIndex) => ( | ||
| <button | ||
| key={thumbUrl} |
Contributor
There was a problem hiding this comment.
Potential duplicate keys if the same image URL appears multiple times.
Using thumbUrl as the key could cause React warnings and unexpected behavior if imageList contains duplicate URLs (e.g., same image posted multiple times in chat).
Suggested fix: use composite key
{imageList.map((thumbUrl, thumbIndex) => (
<button
- key={thumbUrl}
+ key={`${thumbIndex}-${thumbUrl}`}
ref={(el) => {📝 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
| {imageList.map((thumbUrl, thumbIndex) => ( | |
| <button | |
| key={thumbUrl} | |
| {imageList.map((thumbUrl, thumbIndex) => ( | |
| <button | |
| key={`${thumbIndex}-${thumbUrl}`} | |
| ref={(el) => { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/ImageLightboxModal.tsx` around lines 532 - 534, The map
over imageList in ImageLightboxModal uses thumbUrl as the React key which can
collide when URLs repeat; change the key on the <button> rendered inside
imageList.map (refer to the map callback using thumbUrl and thumbIndex) to a
composite unique value (for example combine thumbUrl and thumbIndex or use
`\`${thumbUrl}-${thumbIndex}\``) so duplicates won't produce React key warnings
or unstable element identity.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
src/components/ui/ImageLightboxModal.tsx (3)
355-366:⚠️ Potential issue | 🟡 MinorShow users when the download fails.
The catch path only logs to the console. If
download_imagethrows, the spinner stops and the modal looks idle, so users get no indication that the save failed.💡 Suggested fix
- } catch { - console.error("Download failed"); + } catch (error) { + const message = + error instanceof Error ? error.message : "Download failed"; + console.error("Download failed", error); + setSavedMessage(`Download failed: ${message}`); + setTimeout(() => setSavedMessage(""), 4000); } finally {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/ImageLightboxModal.tsx` around lines 355 - 366, The catch block in handleDownload only logs to console so users get no feedback on failure; update handleDownload's catch to accept the error (catch (err)), console.error(err), set a user-facing saved/failure message via setSavedMessage (e.g. "Download failed" plus brief err.message when available) and start the same setTimeout to clear it so the UI shows the failure; ensure setIsDownloading is still cleared (the existing finally should remain to call setIsDownloading(false)).
605-610:⚠️ Potential issue | 🟡 MinorUse a unique key for repeated image URLs.
The same image URL can appear multiple times in chat. Using
thumbUrlalone gives React duplicate keys, which can reuse the wrong thumbnail node/ref when the strip updates.💡 Suggested fix
<button - key={thumbUrl} + key={`${thumbIndex}-${thumbUrl}`} ref={(el) => { thumbRefs.current[thumbIndex] = el; }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/ImageLightboxModal.tsx` around lines 605 - 610, The map over imageList uses thumbUrl as the React key which can duplicate when the same URL appears multiple times (causing wrong DOM reuse); change the element key to a unique composite (e.g., combine thumbUrl with thumbIndex or other stable identifier) and update any related ref logic that assumes uniqueness (the button render in the imageList.map, the key prop and the ref callback) to use the new composite key so each thumbnail node/ref is distinct.
117-121:⚠️ Potential issue | 🟠 MajorBuild the lightbox index from the same URLs the preview renders.
extractImageUrlsFromMessagegives you the raw message URLs, butcurrentIndexis matched against the already-renderedurlprop. When those differ,idxstays-1, so the active thumbnail and prev/next navigation start from the wrong place.🔎 Verification script
Expect the
urlprop and the array used for lightbox navigation to go through the same normalization/resolution path.#!/bin/bash rg -n -C4 'ImageLightboxModal|extractImageUrlsFromMessage|ImageWithFallback|tenor|tinygif' src🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/ImageLightboxModal.tsx` around lines 117 - 121, The lightbox builds its index from raw URLs returned by extractImageUrlsFromMessage but compares against the already-rendered url prop (produced by ImageWithFallback), causing a mismatch and idx === -1; fix by normalizing/resolving each extracted URL through the same routine ImageWithFallback uses before setting imageList and computing idx (i.e., replace urls = messages.flatMap(extractImageUrlsFromMessage) with a mapped list that passes each extracted URL through the same resolve/normalize function used by ImageWithFallback or a shared helper, then setImageList(normalizedUrls) and compute idx = normalizedUrls.indexOf(url); also add a safe fallback (e.g., setCurrentIndex(0) if idx === -1) to avoid broken navigation.src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Tauri.swift (1)
60-101:⚠️ Potential issue | 🟠 MajorPlugin commands still execute off the main actor.
Everything after Line 62 runs on
ipcDispatchQueue, including handlers for the new share-sheet flow. Any command that presents a controller or touchesWKWebViewfrom here violates UIKit/WebKit's main-thread requirement. Either hop the selector invocation back toDispatchQueue.mainhere or make that contract explicit and enforce it in each plugin.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Tauri.swift` around lines 60 - 101, The invoke(name:invoke:) implementation dispatches all plugin selector invocations on ipcDispatchQueue (see ipcDispatchQueue and the selectorWithCompletionHandler/selectorWithThrows/selectors), which can call into UI/WebKit and must run on the main thread; change the flow so that after locating the correct selector to call (selectorWithCompletionHandler, selectorWithThrows, or selector), you switch to DispatchQueue.main.async before calling the method/IMP/perform, or alternatively enforce/validate a main-thread contract per plugin and dispatch to main only for plugins that require it; update the branches that call fn(...), the unsafeBitCast IMP invocation, and plugin.instance.perform(...) to run on the main queue to ensure UI/ WKWebView work happens on the main thread.src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Invoke.swift (1)
93-115:⚠️ Potential issue | 🟠 MajorBuild
reject()payload as a nativeJsonObject.Line 114 still force-casts an
NSMutableDictionaryback toJsonObject. That keeps the error path on a runtime bridge instead of compile-time typing and can still trap if any inserted value is not bridgeable. ConstructpayloadasJsonObjectfrom the start and mergedictinto that native value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Invoke.swift` around lines 93 - 115, The code currently creates payload as an NSMutableDictionary and then force-casts it to JsonObject when calling sendResponse, which can trap at runtime; change payload to be a native JsonObject from the start (replace the NSMutableDictionary with var payload: JsonObject / [String: JsonValue]), set "message", optional "code" and "error" entries as JsonValues, and when handling data .dictionary(let dict) merge dict into that JsonObject (ensuring values are converted to JsonValue), then call sendResponse(self.error, serialize(.dictionary(payload))) without any force-cast.
🧹 Nitpick comments (3)
src-tauri/src/lib.rs (3)
74-76: Blocking I/O for Path variant.Same concern: use
tokio::fs::writefor thePathcase to maintain async consistency.♻️ Proposed fix
tauri_plugin_dialog::FilePath::Path(path) => { - std::fs::write(&path, &bytes).map_err(|e| e.to_string())?; + tokio::fs::write(&path, &bytes).await.map_err(|e| e.to_string())?; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/lib.rs` around lines 74 - 76, The Path arm in the match currently uses blocking std::fs::write; replace it with async tokio::fs::write and await it so the handler remains non-blocking: update the tauri_plugin_dialog::FilePath::Path(path) branch to call tokio::fs::write(&path, &bytes).await and map the error to a string as done for other branches, and ensure the enclosing function is async (or already awaits this call) so awaiting is valid.
46-46: Same blocking I/O concern as desktop.Consider using
tokio::fs::writehere as well for consistency and to avoid blocking.♻️ Proposed fix
- std::fs::write(&tmp_path, &bytes).map_err(|e| e.to_string())?; + tokio::fs::write(&tmp_path, &bytes).await.map_err(|e| e.to_string())?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/lib.rs` at line 46, Replace the blocking std::fs::write call with the async tokio::fs::write and await it: change std::fs::write(&tmp_path, &bytes).map_err(|e| e.to_string())? to tokio::fs::write(&tmp_path, &bytes).await.map_err(|e| e.to_string())? (and add use of tokio::fs if needed), ensuring the enclosing function is async or you propagate/convert to async so the await is valid.
33-33: Use async file I/O to avoid blocking the runtime.
std::fs::writeis synchronous and blocks the async runtime thread. Consider usingtokio::fs::writefor non-blocking file operations.♻️ Proposed fix
- std::fs::write(file.path(), &bytes).map_err(|e| e.to_string())?; + tokio::fs::write(file.path(), &bytes).await.map_err(|e| e.to_string())?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/lib.rs` at line 33, Replace the blocking std::fs::write call with Tokio's async writer: use tokio::fs::write instead of std::fs::write and await it (e.g., tokio::fs::write(file.path(), &bytes).await.map_err(|e| e.to_string()) ), and make the enclosing function async (or spawn a blocking task if making it async is not possible) so the await can be used; keep the existing error mapping to to_string and the same file.path() and bytes variables.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Invoke.swift`:
- Around line 61-66: The fallback paths in serialize(_ data: JsonValue) and the
similar handler around lines 81–87 are returning hand-built JSON strings like
"\"\(error)\"" which can produce invalid JSON when the error message contains
quotes or backslashes; fix by constructing a proper JSON object (e.g. { "error":
"...message..." }) and serializing it with a JSON-safe API instead of string
interpolation—use JSONEncoder/JSONSerialization or the existing
JsonValue/jsonRepresentation mechanism to produce the final String so the error
text is correctly escaped and always yields valid JSON.
In `@src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Tauri.swift`:
- Around line 20-23: Change the PluginManager.viewController stored property to
a weak reference by replacing the current declaration in class PluginManager
(the shared singleton) with a weak optional: update "public var viewController:
UIViewController?" to "public weak var viewController: UIViewController?";
ensure callers handle the optional as before (no additional ownership changes
required).
In `@src-tauri/src/lib.rs`:
- Around line 8-11: Validate and reject unsafe URLs in download_image before
delegating to download_image_impl: parse the supplied url String with url::Url
and ensure the scheme is exactly "http" or "https"; resolve the host to IP
addresses (e.g., via ToSocketAddrs or tokio lookup) and reject any addresses
that are loopback, link-local, private RFC1918 ranges, or cloud metadata IPs
like 169.254.0.0/16; if validation fails return Err with a clear message,
otherwise call download_image_impl(app, url).await. Include references to
download_image and download_image_impl so reviewers can find the check.
In `@src/components/ui/ImageLightboxModal.tsx`:
- Around line 152-164: The wheel listener effect in ImageLightboxModal runs too
early because filmstripPillRef.current is null on first open; update the
useEffect that sets up onWheel to depend on the value that indicates the
filmstrip was rendered (e.g., imageList or filmstripPillRef.current) so it
re-runs when the element appears, attach the listener to
filmstripPillRef.current inside that effect, and keep the same onWheel behavior
(stopPropagation, preventDefault, scrollLeft adjustment using
filmstripScrollRef). Ensure you add the same cleanup (removeEventListener) and
preserve { passive: false } when adding the listener so the handler is active.
---
Duplicate comments:
In `@src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Invoke.swift`:
- Around line 93-115: The code currently creates payload as an
NSMutableDictionary and then force-casts it to JsonObject when calling
sendResponse, which can trap at runtime; change payload to be a native
JsonObject from the start (replace the NSMutableDictionary with var payload:
JsonObject / [String: JsonValue]), set "message", optional "code" and "error"
entries as JsonValues, and when handling data .dictionary(let dict) merge dict
into that JsonObject (ensuring values are converted to JsonValue), then call
sendResponse(self.error, serialize(.dictionary(payload))) without any
force-cast.
In `@src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Tauri.swift`:
- Around line 60-101: The invoke(name:invoke:) implementation dispatches all
plugin selector invocations on ipcDispatchQueue (see ipcDispatchQueue and the
selectorWithCompletionHandler/selectorWithThrows/selectors), which can call into
UI/WebKit and must run on the main thread; change the flow so that after
locating the correct selector to call (selectorWithCompletionHandler,
selectorWithThrows, or selector), you switch to DispatchQueue.main.async before
calling the method/IMP/perform, or alternatively enforce/validate a main-thread
contract per plugin and dispatch to main only for plugins that require it;
update the branches that call fn(...), the unsafeBitCast IMP invocation, and
plugin.instance.perform(...) to run on the main queue to ensure UI/ WKWebView
work happens on the main thread.
In `@src/components/ui/ImageLightboxModal.tsx`:
- Around line 355-366: The catch block in handleDownload only logs to console so
users get no feedback on failure; update handleDownload's catch to accept the
error (catch (err)), console.error(err), set a user-facing saved/failure message
via setSavedMessage (e.g. "Download failed" plus brief err.message when
available) and start the same setTimeout to clear it so the UI shows the
failure; ensure setIsDownloading is still cleared (the existing finally should
remain to call setIsDownloading(false)).
- Around line 605-610: The map over imageList uses thumbUrl as the React key
which can duplicate when the same URL appears multiple times (causing wrong DOM
reuse); change the element key to a unique composite (e.g., combine thumbUrl
with thumbIndex or other stable identifier) and update any related ref logic
that assumes uniqueness (the button render in the imageList.map, the key prop
and the ref callback) to use the new composite key so each thumbnail node/ref is
distinct.
- Around line 117-121: The lightbox builds its index from raw URLs returned by
extractImageUrlsFromMessage but compares against the already-rendered url prop
(produced by ImageWithFallback), causing a mismatch and idx === -1; fix by
normalizing/resolving each extracted URL through the same routine
ImageWithFallback uses before setting imageList and computing idx (i.e., replace
urls = messages.flatMap(extractImageUrlsFromMessage) with a mapped list that
passes each extracted URL through the same resolve/normalize function used by
ImageWithFallback or a shared helper, then setImageList(normalizedUrls) and
compute idx = normalizedUrls.indexOf(url); also add a safe fallback (e.g.,
setCurrentIndex(0) if idx === -1) to avoid broken navigation.
---
Nitpick comments:
In `@src-tauri/src/lib.rs`:
- Around line 74-76: The Path arm in the match currently uses blocking
std::fs::write; replace it with async tokio::fs::write and await it so the
handler remains non-blocking: update the
tauri_plugin_dialog::FilePath::Path(path) branch to call tokio::fs::write(&path,
&bytes).await and map the error to a string as done for other branches, and
ensure the enclosing function is async (or already awaits this call) so awaiting
is valid.
- Line 46: Replace the blocking std::fs::write call with the async
tokio::fs::write and await it: change std::fs::write(&tmp_path,
&bytes).map_err(|e| e.to_string())? to tokio::fs::write(&tmp_path,
&bytes).await.map_err(|e| e.to_string())? (and add use of tokio::fs if needed),
ensuring the enclosing function is async or you propagate/convert to async so
the await is valid.
- Line 33: Replace the blocking std::fs::write call with Tokio's async writer:
use tokio::fs::write instead of std::fs::write and await it (e.g.,
tokio::fs::write(file.path(), &bytes).await.map_err(|e| e.to_string()) ), and
make the enclosing function async (or spawn a blocking task if making it async
is not possible) so the await can be used; keep the existing error mapping to
to_string and the same file.path() and bytes variables.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 949c689f-e755-41bf-83a4-fa72e8080792
📒 Files selected for processing (5)
src-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Invoke.swiftsrc-tauri/plugins/share-sheet/.tauri/tauri-api/Sources/Tauri/Tauri.swiftsrc-tauri/src/lib.rssrc/components/message/SwipeableMessage.tsxsrc/components/ui/ImageLightboxModal.tsx
Comment on lines
+8
to
+11
| #[tauri::command] | ||
| async fn download_image(app: tauri::AppHandle, url: String) -> Result<String, String> { | ||
| download_image_impl(app, url).await | ||
| } |
Contributor
There was a problem hiding this comment.
Server-Side Request Forgery (SSRF) risk: URL is not validated server-side.
The command accepts any URL and fetches it with reqwest. While the frontend filters URLs via isImageLikeUrl, Tauri commands can be invoked directly (e.g., via dev tools or IPC), bypassing frontend validation. This could allow fetching internal resources like http://169.254.169.254/ (cloud metadata) or http://localhost:8080/admin.
Consider validating the URL scheme and host on the Rust side before fetching.
🛡️ Proposed validation helper
+fn validate_image_url(url: &str) -> Result<(), String> {
+ let parsed = url::Url::parse(url).map_err(|_| "Invalid URL")?;
+
+ // Only allow http/https
+ if !matches!(parsed.scheme(), "http" | "https") {
+ return Err("Only HTTP(S) URLs are allowed".to_string());
+ }
+
+ // Block private/internal addresses
+ if let Some(host) = parsed.host_str() {
+ if host == "localhost"
+ || host.starts_with("127.")
+ || host.starts_with("10.")
+ || host.starts_with("192.168.")
+ || host.starts_with("169.254.")
+ || host.contains("metadata")
+ {
+ return Err("Internal addresses are not allowed".to_string());
+ }
+ }
+ Ok(())
+}
+
#[tauri::command]
async fn download_image(app: tauri::AppHandle, url: String) -> Result<String, String> {
+ validate_image_url(&url)?;
download_image_impl(app, url).await
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/src/lib.rs` around lines 8 - 11, Validate and reject unsafe URLs in
download_image before delegating to download_image_impl: parse the supplied url
String with url::Url and ensure the scheme is exactly "http" or "https"; resolve
the host to IP addresses (e.g., via ToSocketAddrs or tokio lookup) and reject
any addresses that are loopback, link-local, private RFC1918 ranges, or cloud
metadata IPs like 169.254.0.0/16; if validation fails return Err with a clear
message, otherwise call download_image_impl(app, url).await. Include references
to download_image and download_image_impl so reviewers can find the check.
Comment on lines
+152
to
+164
| useEffect(() => { | ||
| const el = filmstripPillRef.current; | ||
| if (!el) return; | ||
| const onWheel = (e: WheelEvent) => { | ||
| e.stopPropagation(); | ||
| e.preventDefault(); | ||
| if (filmstripScrollRef.current) { | ||
| filmstripScrollRef.current.scrollLeft += e.deltaX + e.deltaY; | ||
| } | ||
| }; | ||
| el.addEventListener("wheel", onWheel, { passive: false }); | ||
| return () => el.removeEventListener("wheel", onWheel); | ||
| }, []); |
Contributor
There was a problem hiding this comment.
This wheel listener never attaches on a normal first open.
imageList is filled by a later effect, so filmstripPillRef.current is still null the only time this runs. That means the new horizontal-scroll behavior does not initialize until a remount, and wheel input over the strip falls through to the overlay zoom handler instead.
💡 Suggested fix
useEffect(() => {
+ if (!isOpen || imageList.length <= 1) return;
const el = filmstripPillRef.current;
if (!el) return;
const onWheel = (e: WheelEvent) => {
e.stopPropagation();
e.preventDefault();
@@
};
el.addEventListener("wheel", onWheel, { passive: false });
return () => el.removeEventListener("wheel", onWheel);
- }, []);
+ }, [isOpen, imageList.length]);📝 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
| useEffect(() => { | |
| const el = filmstripPillRef.current; | |
| if (!el) return; | |
| const onWheel = (e: WheelEvent) => { | |
| e.stopPropagation(); | |
| e.preventDefault(); | |
| if (filmstripScrollRef.current) { | |
| filmstripScrollRef.current.scrollLeft += e.deltaX + e.deltaY; | |
| } | |
| }; | |
| el.addEventListener("wheel", onWheel, { passive: false }); | |
| return () => el.removeEventListener("wheel", onWheel); | |
| }, []); | |
| useEffect(() => { | |
| if (!isOpen || imageList.length <= 1) return; | |
| const el = filmstripPillRef.current; | |
| if (!el) return; | |
| const onWheel = (e: WheelEvent) => { | |
| e.stopPropagation(); | |
| e.preventDefault(); | |
| if (filmstripScrollRef.current) { | |
| filmstripScrollRef.current.scrollLeft += e.deltaX + e.deltaY; | |
| } | |
| }; | |
| el.addEventListener("wheel", onWheel, { passive: false }); | |
| return () => el.removeEventListener("wheel", onWheel); | |
| }, [isOpen, imageList.length]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/ImageLightboxModal.tsx` around lines 152 - 164, The wheel
listener effect in ImageLightboxModal runs too early because
filmstripPillRef.current is null on first open; update the useEffect that sets
up onWheel to depend on the value that indicates the filmstrip was rendered
(e.g., imageList or filmstripPillRef.current) so it re-runs when the element
appears, attach the listener to filmstripPillRef.current inside that effect, and
keep the same onWheel behavior (stopPropagation, preventDefault, scrollLeft
adjustment using filmstripScrollRef). Ensure you add the same cleanup
(removeEventListener) and preserve { passive: false } when adding the listener
so the handler is active.
matheusfillipe
force-pushed
the
feat/add-image-preview
branch
from
c90319c to
cd9eeef
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
src/components/ui/MediaViewerModal.tsx (2)
720-731: Optional: Extract inline IIFE to a small component or variable.The IIFE pattern works but is slightly harder to read than extracting this to a named variable or component.
♻️ Alternative approach
// Before the return, or as a small component: const thumbCommentCount = imageEntries[thumbIndex]?.msg?.msgid ? (commentCountByMsgid[imageEntries[thumbIndex].msg.msgid] ?? 0) : 0; // Then in JSX: {thumbCommentCount > 0 && ( <span className="absolute top-0.5 right-0.5 ..."> {thumbCommentCount > 99 ? "99+" : thumbCommentCount} </span> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/MediaViewerModal.tsx` around lines 720 - 731, Extract the inline IIFE that computes and renders the thumbnail comment badge into a small variable or component to improve readability: compute the count using imageEntries and thumbIndex with commentCountByMsgid (e.g., thumbCommentCount = imageEntries[thumbIndex]?.msg?.msgid ? (commentCountByMsgid[imageEntries[thumbIndex].msg.msgid] ?? 0) : 0) and then conditionally render the <span> only when thumbCommentCount > 0, still formatting the display as count > 99 ? "99+" : count; update the JSX where the IIFE was to use the new variable or a tiny named component instead.
396-410: Consider showing user feedback on download failure.Currently, if the download fails, the user only sees the spinner disappear with no indication of what happened. While the success case shows a toast message, the error case is silent.
💡 Suggested improvement
} catch { - console.error("Download failed"); + console.error("Download failed"); + setSavedMessage("Download failed"); + setTimeout(() => setSavedMessage(""), 4000); } finally {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/MediaViewerModal.tsx` around lines 396 - 410, The download failure path in handleDownload is silent; catch the thrown error as e (catch (e)), log it and show user-facing feedback by calling setSavedMessage with a clear error string (e.g. include e.message or a generic "Download failed") and clear it after the same timeout used for success, so users see the failure toast; keep setIsDownloading(false) in finally. Reference: handleDownload, setSavedMessage, setIsDownloading, invoke("download_image", { url: currentUrl }).tests/components/MediaViewerModal.test.tsx (2)
194-199: Consider adding test for comments button visibility with serverId/channelId.The test verifies the button is hidden without serverId/channelId, but there's no corresponding test for when it should be visible.
💡 Additional test case
test("comments toggle button shown when serverId/channelId and msgid present", () => { vi.mocked(store.getChannelMessages).mockReturnValue([ { id: "1", content: "https://example.com/image.jpg", serverId: "s1", channelId: "c1", type: "message", timestamp: new Date(), userId: "user1", reactions: [], msgid: "msg-123", }, ] as unknown as ReturnType<typeof store.getChannelMessages>); render( <MediaViewerModal {...defaultProps} url="https://example.com/image.jpg" serverId="s1" channelId="c1" />, ); expect( screen.getByRole("button", { name: /comments/i }), ).toBeInTheDocument(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/components/MediaViewerModal.test.tsx` around lines 194 - 199, Add a positive test that verifies the comments toggle button is shown when serverId, channelId and a matching message with msgid exist: mock store.getChannelMessages to return a message object (include id, content matching the modal url, serverId "s1", channelId "c1", msgid, etc.), render MediaViewerModal with {...defaultProps} plus url, serverId="s1", channelId="c1", then assert screen.getByRole("button", { name: /comments/i }) is in the document; this complements the existing test that checks the button is hidden when serverId/channelId are absent.
20-28: Store mock may not fully replicate selector behavior.The mock returns
selector({ messages: {} })for all calls, but the actual component usesuseShallowwith a selector that expectsstate.messages[key]to be an array. This works for the "no comments" case but may cause issues if tests need to verify comment count behavior.💡 Suggested improvement for future tests
If you add tests for comment count display, you'll need to enhance the mock:
vi.mock("../../src/store", () => ({ getChannelMessages: vi.fn(() => []), default: Object.assign( vi.fn((selector: (state: unknown) => unknown) => typeof selector === "function" ? selector({ messages: { "s1-c1": [] } }) : null, ), { getState: vi.fn(() => ({ messages: {} })) }, ), }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/components/MediaViewerModal.test.tsx` around lines 20 - 28, The current store mock returns selector({ messages: {} }) which breaks selectors that expect message arrays; update the mock used by getChannelMessages/default (the vi.fn((selector) => ...)) to provide a messages map with the test keys (e.g. include an entry like "s1-c1": [] or a more general empty-array fallback) so selectors like those used by useShallow that access state.messages[key] receive an array instead of undefined; keep getState consistent (returning { messages: { "s1-c1": [] } } or similar) so future tests that assert comment counts behave correctly.src/components/message/MessageItem.tsx (1)
305-312: Consider lifting modal state to avoid multiple modal instances.Each
ImageWithFallbackinstance renders its ownMediaViewerModal. While only one is open at a time, this creates multiple modal instances in the DOM. For a single message with many images, this could be inefficient.An alternative pattern would be to lift the modal to a parent component (e.g., at the channel level) and pass down a callback to open it with a specific URL. However, the current approach is simpler and works correctly, so this is a low-priority optimization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/message/MessageItem.tsx` around lines 305 - 312, Multiple ImageWithFallback instances each render their own MediaViewerModal (using lightboxOpen, displayUrl, setLightboxOpen, serverId, channelId), which creates many modal instances; lift the modal state up to a parent (e.g., channel or message list) and render a single MediaViewerModal there, replace each ImageWithFallback's local modal props with a callback prop (e.g., onOpenMedia(url, serverId, channelId)) that sets the parent's modal state (open flag + url + ids), and pass the parent's state and onClose handler to the single MediaViewerModal so only one modal component is instantiated and controlled centrally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/ui/MediaCommentsSidebar.tsx`:
- Around line 208-216: The current logic treats formatting-only changes as
"active" but still passes color: "inherit" into getPreviewStyles, which forces
an inline color and causes the textarea to flip to black; update the
previewStyle logic so getPreviewStyles is only given a color when selectedColor
is a real override (selectedColor !== null && selectedColor !== "inherit"), and
when only formatting is active call getPreviewStyles without a color (or with
color undefined) so it doesn't inject an inline color override; adjust the
hasActiveFormatting/previewStyle computation around the selectedColor,
selectedFormatting, getPreviewStyles, and previewStyle symbols accordingly.
- Around line 121-127: The reply filter currently compares
m.tags?.["+draft/reply"]?.trim() === sourceMessage.msgid without guarding when
sourceMessage.msgid is undefined; update the predicate inside the
useStore/useShallow selector to first check that sourceMessage?.msgid is a
non-empty string (e.g., truthy) before comparing, so only messages with a
draft/reply tag are considered when a sourceMessage.msgid exists; modify the
selector that builds comments (the anonymous function passed to useShallow used
by useStore) to short-circuit and return an empty array or skip the compare when
sourceMessage?.msgid is missing.
- Around line 144-149: The Enter key handler handleKeyDown should ignore Enter
presses while an IME composition is active; update handleKeyDown to check
e.nativeEvent.isComposing (or a composition tracking flag updated by
onCompositionStart/onCompositionEnd) and return early if composing, so pressing
Enter to confirm IME composition doesn't call handleSend; keep the existing
behavior for Enter without Shift when not composing.
In `@src/components/ui/MediaViewerModal.tsx`:
- Around line 97-99: The debounce timer stored in sliderDebounceRef inside the
MediaViewerModal component isn't cleared on unmount; add a useEffect in
MediaViewerModal that returns a cleanup function which checks
sliderDebounceRef.current and calls clearTimeout(sliderDebounceRef.current) (and
resets it) to prevent state updates after unmount; ensure this effect runs once
(empty dependency array) so the timer is always cleared when the component
unmounts.
In `@tests/components/MediaCommentsSidebar.test.tsx`:
- Around line 132-138: The "has emoji button" test in
MediaCommentsSidebar.test.tsx currently asserts the same "attachment options"
button as another test, so update the test (the test named "has emoji button"
that renders <MediaCommentsSidebar {...defaultProps} />) to actually target the
emoji control: either change the assertion to use screen.getByRole("button", {
name: /emoji/i }) or getByLabelText/getByTestId for the emoji button exposed by
InputToolbar; if InputToolbar lacks an accessible label, add one in the
InputToolbar component (e.g., aria-label or accessible name for the emoji
button) so the test can reliably query it by name.
---
Nitpick comments:
In `@src/components/message/MessageItem.tsx`:
- Around line 305-312: Multiple ImageWithFallback instances each render their
own MediaViewerModal (using lightboxOpen, displayUrl, setLightboxOpen, serverId,
channelId), which creates many modal instances; lift the modal state up to a
parent (e.g., channel or message list) and render a single MediaViewerModal
there, replace each ImageWithFallback's local modal props with a callback prop
(e.g., onOpenMedia(url, serverId, channelId)) that sets the parent's modal state
(open flag + url + ids), and pass the parent's state and onClose handler to the
single MediaViewerModal so only one modal component is instantiated and
controlled centrally.
In `@src/components/ui/MediaViewerModal.tsx`:
- Around line 720-731: Extract the inline IIFE that computes and renders the
thumbnail comment badge into a small variable or component to improve
readability: compute the count using imageEntries and thumbIndex with
commentCountByMsgid (e.g., thumbCommentCount =
imageEntries[thumbIndex]?.msg?.msgid ?
(commentCountByMsgid[imageEntries[thumbIndex].msg.msgid] ?? 0) : 0) and then
conditionally render the <span> only when thumbCommentCount > 0, still
formatting the display as count > 99 ? "99+" : count; update the JSX where the
IIFE was to use the new variable or a tiny named component instead.
- Around line 396-410: The download failure path in handleDownload is silent;
catch the thrown error as e (catch (e)), log it and show user-facing feedback by
calling setSavedMessage with a clear error string (e.g. include e.message or a
generic "Download failed") and clear it after the same timeout used for success,
so users see the failure toast; keep setIsDownloading(false) in finally.
Reference: handleDownload, setSavedMessage, setIsDownloading,
invoke("download_image", { url: currentUrl }).
In `@tests/components/MediaViewerModal.test.tsx`:
- Around line 194-199: Add a positive test that verifies the comments toggle
button is shown when serverId, channelId and a matching message with msgid
exist: mock store.getChannelMessages to return a message object (include id,
content matching the modal url, serverId "s1", channelId "c1", msgid, etc.),
render MediaViewerModal with {...defaultProps} plus url, serverId="s1",
channelId="c1", then assert screen.getByRole("button", { name: /comments/i }) is
in the document; this complements the existing test that checks the button is
hidden when serverId/channelId are absent.
- Around line 20-28: The current store mock returns selector({ messages: {} })
which breaks selectors that expect message arrays; update the mock used by
getChannelMessages/default (the vi.fn((selector) => ...)) to provide a messages
map with the test keys (e.g. include an entry like "s1-c1": [] or a more general
empty-array fallback) so selectors like those used by useShallow that access
state.messages[key] receive an array instead of undefined; keep getState
consistent (returning { messages: { "s1-c1": [] } } or similar) so future tests
that assert comment counts behave correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d9f2e1b7-ec4f-43c2-94b9-610c1994a0b1
📒 Files selected for processing (6)
src/components/message/MessageItem.tsxsrc/components/ui/EmojiPickerModal.tsxsrc/components/ui/MediaCommentsSidebar.tsxsrc/components/ui/MediaViewerModal.tsxtests/components/MediaCommentsSidebar.test.tsxtests/components/MediaViewerModal.test.tsx
ValwareIRC
approved these changes
Mar 20, 2026
* Support more media formats and extend media viewer * Many fixes * fix android download * Auto focus input on type * small fix
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
2 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.
TLDR: Click on image preview, get image viewer.
Summary by CodeRabbit
New Features
Tests