Conversation
|
Claude finished @yewreeka's task —— View job Code Review Complete ✅I've completed a thorough review of PR #690. Overall, this is a well-structured feature addition with clean separation of concerns. Below are my findings: Code Quality & Best Practices ✅Strengths:
Minor Issues:
Potential Bugs 🐛Medium Priority:
Low Priority:
Performance Considerations ⚡Good:
Potential Optimizations:
Security Considerations 🔒Good:
Potential Issues:
Test Coverage 📊Coverage Analysis:
Recommendations:
Test Infrastructure:
Database Migration ✅Migration Review: migrator.registerMigration("addHasHadVerifiedAssistantToConversation") { db in
try db.alter(table: "conversation") { t in
t.add(column: "hasHadVerifiedAssistant", .boolean).notNull().defaults(to: false)
}
}
Adherence to Project Conventions ✅CLAUDE.md Compliance:
File Organization:
Summary & RecommendationsOverall Assessment: This is a high-quality PR with clean separation of concerns and good error handling. The implementation follows project conventions and introduces no breaking changes. Required Changes (Security):
Recommended Improvements:
Nice-to-Have:
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review 2 blocking correctness issues found. This PR introduces a new feature with substantial scope: new UI views, a database schema migration adding hasHadVerifiedAssistant, and changes propagating through multiple data layer files. Additionally, unresolved review comments include a high-severity timing issue and a medium-severity bash compatibility problem that could block commits. You can customize Macroscope's approvability policy. Learn more. |
yewreeka
force-pushed
the
jarod/assistant-files-links
branch
from
1db6284 to
ec11803
Compare
Shows a 'Files & Links' row in Conversation Info when a verified assistant is present. Tapping navigates to a new view with: - Segmented picker to switch between Files and Links tabs - Search to filter by filename or URL - File rows with thumbnail, name, and date - Link rows with OG image, title, and domain New files: - AssistantFilesLinksRepository: queries messages by agent senders filtered to attachments and linkPreview content types - AssistantFilesLinksView: SwiftUI view with tabs, search, and lists
Attachment keys in the database can be either StoredRemoteAttachment JSON blobs (for remote uploads) or file:// URLs (for inline attachments sent via CLI). The previous code only tried JSON parsing, which failed silently for file:// URLs, causing all inline attachments to show as 'Untitled' with no type icon. Now uses the same parsing logic as MessagesRepository.hydrateAttachment: parses filename from the file:// path (splitting on underscore) and derives mimeType from the file extension via UTType.
Replace the download affordance in Files & Links with the same file preview flow used when tapping a file attachment in the message list. Rows now open QuickLook after loading inline or remote attachment data, show a chevron instead of the iCloud download icon, and display file extension + date in the subtitle.
Stop presenting file previews via a SwiftUI sheet and instead reuse a shared UIKit QuickLook presenter that presents from the top-most view controller. This matches the message-list attachment preview host more closely and removes the modal sheet wrapper from the Files & Links screen.
Wrap QLPreviewController in a shared SwiftUI representable and present it through a hosting controller from both the messages list and the assistant Files & Links view. This unifies the preview implementation while still loading inline and remote files through the same helper.
Drop the shared SwiftUI sheet host and move both the messages-list and assistant Files & Links entry points to a shared UIKit presenter that creates and presents QLPreviewController directly. This restores consistent Quick Look toolbar behavior across both flows while keeping file loading shared.
… filters Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* Add markdown attachment preview sheet with WKWebView rendering When tapping a markdown file attachment (.md, .markdown), show a native sheet with rendered markdown instead of the QuickLook file preview. Uses marked.js (bundled) to convert markdown to HTML and renders in a WKWebView with system-matching styles. Features: - Headings, lists, code blocks, blockquotes, tables - Remote images load inline - Links rendered as tappable blue text - Dark mode support via CSS color-scheme - Share button to copy file or open in Files app - Sheet with medium/large detents and grabber - Falls back to QuickLook for non-markdown files * Fix lint violations and security issues in markdown preview - Remove unused markdownPreviewController property (UIKit retains presented controllers) - Fix XSS vulnerability: use base64-encoded markdown passed to JS instead of template literal string escaping - Add Content-Security-Policy header restricting scripts and images - Handle missing marked.js bundle with user-visible error instead of silent failure - Fix SwiftLint implicit_optional_initialization violations * Fix XSS vulnerability and add unit tests for isMarkdownFile Security: - Move base64-encoded markdown to a data-markdown attribute on the body element instead of interpolating inside a script tag. This prevents script context injection (e.g. </script> payloads) by keeping user content entirely out of JavaScript source. Tests: - Add 10 unit tests for HydratedAttachment.isMarkdownFile covering .md/.markdown extensions, case insensitivity, text/markdown and text/x-markdown MIME types, non-markdown files, nil values, and extension-over-MIME priority. * Add file attachment cache for instant repeat previews Introduce FileAttachmentCache (actor, in ConvosCore) that persists downloaded file attachment data in Caches/FileAttachments/ keyed by a SHA-256 hash of the attachment key. This mirrors how ImageCache handles image attachments. Before: every tap on a file attachment re-fetched data from the network or inline recovery, wrote to a random temp path, and deleted it on sheet dismiss. After: first tap fetches and caches; subsequent taps return the cached file URL instantly. Files are stored in Caches/ so iOS can evict under storage pressure. Also adds 5 unit tests for FileAttachmentCache. * Use X close button and hide drag indicator on markdown sheet * Harden markdown preview against XSS and injection attacks - Strip raw HTML blocks from markdown via custom marked renderer that returns empty string for html tokens - Sanitize rendered DOM: remove script, iframe, object, embed, form, and input elements after parsing - Strip inline event handlers (onload, onerror, onclick, etc.) - Remove javascript: URLs from anchor hrefs - Restrict CSP img-src to data: only (block remote image loading which enables tracking pixels and information disclosure) - Add WKNavigationDelegate to intercept link taps and open them in Safari instead of navigating inside the WKWebView * Fix path traversal vulnerability in FileAttachmentCache Sanitize filenames in all public entry points by extracting lastPathComponent and replacing '..' sequences. A malicious filename like '../../../etc/passwd' is now resolved to 'passwd' within the cache subdirectory. All file operations now go through resolvedFileURL() which applies sanitization before constructing the path. Adds test covering path traversal, dot-dot prefix, and empty filename cases. * Use @Environment dismiss and standard Done button for markdown sheet * Fix review issues: wildcard sanitization, async I/O, reload guard, button extraction Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Wrap QLPreviewController in a shared SwiftUI representable and present it through a hosting controller from both the messages list and the assistant Files & Links view. This unifies the preview implementation while still loading inline and remote files through the same helper.
yewreeka
force-pushed
the
jarod/assistant-files-links
branch
from
95c70ab to
a9636f3
Compare
| } | ||
|
|
||
| if key.hasPrefix("file://") { | ||
| let url = URL(string: key) ?? URL(fileURLWithPath: String(key.dropFirst(7))) |
There was a problem hiding this comment.
🟢 Low Repositories/AssistantFilesLinksRepository.swift:150
In parseAttachmentKey, when a file:// URL has two slashes instead of three (e.g., file://path/to/file vs file:///path/to/file), URL(string: key) succeeds but parses path as the host, leaving lastPathComponent empty or /. The filename is then extracted incorrectly or becomes empty. Consider validating the URL structure or using fileURL(withPath:) as the primary parser for file:// schemes.
- if key.hasPrefix("file://") {
- let url = URL(string: key) ?? URL(fileURLWithPath: String(key.dropFirst(7)))
+ if key.hasPrefix("file://") {
+ let url = URL(fileURLWithPath: String(key.dropFirst(7)))🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file ConvosCore/Sources/ConvosCore/Storage/Repositories/AssistantFilesLinksRepository.swift around line 150:
In `parseAttachmentKey`, when a `file://` URL has two slashes instead of three (e.g., `file://path/to/file` vs `file:///path/to/file`), `URL(string: key)` succeeds but parses `path` as the host, leaving `lastPathComponent` empty or `/`. The filename is then extracted incorrectly or becomes empty. Consider validating the URL structure or using `fileURL(withPath:)` as the primary parser for `file://` schemes.
Evidence trail:
ConvosCore/Sources/ConvosCore/Storage/Repositories/AssistantFilesLinksRepository.swift lines 149-165 (parseAttachmentKey function at REVIEWED_COMMIT), ConvosCore/Tests/ConvosCoreTests/VideoMessageTests.swift line 231 (test using two-slash file URL), Swift URL documentation showing that `URL(string: "file://host/path")` parses 'host' as the host component rather than part of the path.
| } | ||
| } | ||
|
|
||
| public func fetchLinks() async throws -> [AssistantLink] { |
There was a problem hiding this comment.
🟢 Low Repositories/AssistantFilesLinksRepository.swift:96
Only the first attachment from each message is processed. The code decodes attachmentUrls as [String] but then only uses keys.first, so messages with multiple attachments silently drop all but the first. Additionally, using the message id as AssistantFile.id means multiple attachments from the same message would collide if you later fix this.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file ConvosCore/Sources/ConvosCore/Storage/Repositories/AssistantFilesLinksRepository.swift around line 96:
Only the first attachment from each message is processed. The code decodes `attachmentUrls` as `[String]` but then only uses `keys.first`, so messages with multiple attachments silently drop all but the first. Additionally, using the message `id` as `AssistantFile.id` means multiple attachments from the same message would collide if you later fix this.
Evidence trail:
ConvosCore/Sources/ConvosCore/Storage/Repositories/AssistantFilesLinksRepository.swift lines 76-84 at REVIEWED_COMMIT:
- Line 76: `let attachmentUrlsJson: String = row["attachmentUrls"]`
- Line 77: `let keys = try? JSONDecoder().decode([String].self, from: Data(attachmentUrlsJson.utf8))`
- Line 78: `let firstKey = keys.first`
- Line 84: `return AssistantFile(id: id, ...)`
Verified that `attachmentUrls` is decoded as `[String]` but only `keys.first` is used, dropping all subsequent attachments. Message `id` is used as `AssistantFile.id`.
There was a problem hiding this comment.
🟠 High
https://github.com/xmtplabs/convos-ios/blob/95c70ab6bca267d9d1716c08d7f017b14ac6098c/Convos/Conversation Detail/ConversationViewModel.swift#L625
When the conversation ID changes, sendReadReceiptIfNeeded() is called at line 660 before loadPhotoPreferences() completes at line 657. This causes sendReadReceipts to still hold the stale preference value from the previous conversation. If switching from a conversation with sendReadReceipts=false to one with sendReadReceipts=true, the read receipt is incorrectly blocked; switching from true to false incorrectly sends a read receipt. Consider moving sendReadReceiptIfNeeded() after loadPhotoPreferences() completes, or skip the immediate call and rely on the normal message flow.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file Convos/Conversation Detail/ConversationViewModel.swift around line 625:
When the conversation ID changes, `sendReadReceiptIfNeeded()` is called at line 660 before `loadPhotoPreferences()` completes at line 657. This causes `sendReadReceipts` to still hold the stale preference value from the previous conversation. If switching from a conversation with `sendReadReceipts=false` to one with `sendReadReceipts=true`, the read receipt is incorrectly blocked; switching from `true` to `false` incorrectly sends a read receipt. Consider moving `sendReadReceiptIfNeeded()` after `loadPhotoPreferences()` completes, or skip the immediate call and rely on the normal message flow.
Evidence trail:
ConversationViewModel.swift lines 609-622 (loadPhotoPreferences uses Task { @MainActor }), lines 653-661 (observe() calls loadPhotoPreferences() then sendReadReceiptIfNeeded() synchronously), line 331 (sendReadReceipts property), line 617 (sendReadReceipts updated in async task). ConversationViewModel+ReadReceipts.swift line 17 (guard sendReadReceipts else { return }).
| # Get staged Swift files (excluding deleted files and build directories) | ||
| STAGED_SWIFT_FILES=$(git diff --cached --name-only --diff-filter=d | grep '\.swift$' || true) | ||
| STAGED_SWIFT_FILES=$(echo "$STAGED_SWIFT_FILES" | grep -v -E '^\.derivedData/|^\.build/|/\.build/' || true) | ||
| mapfile -t STAGED_SWIFT_FILES < <( |
There was a problem hiding this comment.
🟡 Medium hooks/pre-commit:23
The mapfile command at line 23 requires Bash 4.0+, but macOS ships with Bash 3.2.57 by default, causing mapfile: command not found and blocking all commits for affected developers. With set -e at line 4, the hook aborts on this error. The previous while IFS= read -r loop was compatible with Bash 3.2.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file Scripts/hooks/pre-commit around line 23:
The `mapfile` command at line 23 requires Bash 4.0+, but macOS ships with Bash 3.2.57 by default, causing `mapfile: command not found` and blocking all commits for affected developers. With `set -e` at line 4, the hook aborts on this error. The previous `while IFS= read -r` loop was compatible with Bash 3.2.
There was a problem hiding this comment.
🟢 Low
https://github.com/xmtplabs/convos-ios/blob/a9636f3bdc22d6c4dad0cb08a5d94ccf69f77a9a/Convos/Conversation Detail/Messages/Messages View Controller/View Controller/MessagesViewController.swift#L1268
The JavaScript URL filter a[href^="javascript:"] on line 1084 is case-sensitive, so mixed-case schemes like JAVASCRIPT: or JaVaScRiPt: bypass the filter and execute arbitrary JavaScript in the WebView. Consider using the same URL-parsing approach as MarkdownAttachmentPreviewSheet (lines 1268-1282) to normalize and validate the scheme with case-insensitive comparison.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file Convos/Conversation Detail/Messages/Messages View Controller/View Controller/MessagesViewController.swift around line 1268:
The JavaScript URL filter `a[href^="javascript:"]` on line 1084 is case-sensitive, so mixed-case schemes like `JAVASCRIPT:` or `JaVaScRiPt:` bypass the filter and execute arbitrary JavaScript in the WebView. Consider using the same URL-parsing approach as `MarkdownAttachmentPreviewSheet` (lines 1268-1282) to normalize and validate the scheme with case-insensitive comparison.
Evidence trail:
MessagesViewController.swift:1084 - `div.querySelectorAll('a[href^="javascript:"]').forEach(function(el) { el.removeAttribute('href'); });` (case-sensitive CSS selector)
MessagesViewController.swift:1015 - CSP with `script-src 'unsafe-inline'` allows inline scripts including javascript: URLs
MessagesViewController.swift:1268-1282 - URL parsing approach with `.toLowerCase()` for case-insensitive scheme comparison
git grep result confirms exact line 1084 for the vulnerable pattern
There was a problem hiding this comment.
🟢 Low
In processProfileSnapshotInSE, profiles with existing name or avatar are skipped entirely via continue on line 771, so the agent verification logic (lines 795-798) and markConversationHasVerifiedAssistantIfNeeded call never execute for those profiles. Pre-existing agent profiles that already have a name/avatar will never be verified through snapshot processing, leaving hasHadVerifiedAssistant unset on conversations that should have it. Consider removing the continue and merging the snapshot data with the existing profile instead.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file ConvosCore/Sources/ConvosCore/Inboxes/MessagingService+PushNotifications.swift around line 771:
In `processProfileSnapshotInSE`, profiles with existing `name` or `avatar` are skipped entirely via `continue` on line 771, so the agent verification logic (lines 795-798) and `markConversationHasVerifiedAssistantIfNeeded` call never execute for those profiles. Pre-existing agent profiles that already have a name/avatar will never be verified through snapshot processing, leaving `hasHadVerifiedAssistant` unset on conversations that should have it. Consider removing the `continue` and merging the snapshot data with the existing profile instead.
Evidence trail:
ConvosCore/Sources/ConvosCore/Inboxes/MessagingService+PushNotifications.swift lines 771-773 (continue statement for existing name/avatar), lines 795-798 (agent verification logic after continue), line 800 (markConversationHasVerifiedAssistantIfNeeded call after continue) at REVIEWED_COMMIT
1 participant
Summary
Details
Testing
Note
Add assistant files and links view to conversation info screen
AssistantFilesLinksView, a new screen where users can browse, search, and open assistant-managed files (via Quick Look) and links (via system browser), accessible from the Conversation Info screen when the conversation has ever had a verified assistant.AssistantFilesLinksRepositoryto fetch assistant files and links from the database by querying stored messages for a given conversation.hasHadVerifiedAssistantboolean column on theconversationtable, set totruebyConversationWriter,StreamProcessor,InviteJoinRequestsManager, and the NSE messaging service when a verified Convos Assistant is detected.MessagesViewControllerto use a newFileAttachmentPreviewLoader+QuickLookSheetPresenter, replacing the previous in-controllerQLPreviewControllerdata source/delegate approach.addHasHadVerifiedAssistantToConversation); existing conversations default tofalseand are only upgraded when agent verification runs.Macroscope summarized a9636f3.