Conversation
📝 WalkthroughWalkthroughThis PR refactors SwiftUI view composition and state management across multiple features. Primary changes include removing AnyView wrappers in favor of ViewBuilder and explicit view returns, migrating EventMenu from SwiftUI to UIKit, generalizing EventMutingContainerView with proper typing, modularizing ProfileView with scroll tracking, and optimizing video playback state emissions and image rendering. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant SwiftUI as EventMenuContext<br/>(SwiftUI)
participant Menu as SwiftUI Menu
participant ProfileMgr as ProfileModel<br/>Manager
User->>SwiftUI: Tap event action
SwiftUI->>Menu: Show Menu overlay
Menu->>ProfileMgr: Allocate ProfileModel<br/>(on every open)
ProfileMgr-->>Menu: Return model
Menu->>User: Display menu items
sequenceDiagram
actor User
participant UIKit as EventMenuButton<br/>(UIKit)
participant UIButton as UIButton
participant MenuActions as menuActions<br/>(static)
participant ProfileMgr as ProfileModel<br/>Manager
User->>UIKit: Initialize button
UIKit->>UIButton: Configure with deferred menu
User->>UIButton: Tap ellipsis button
UIButton->>MenuActions: Build menu (lazy)
MenuActions->>ProfileMgr: Allocate ProfileModel<br/>(on interaction)
ProfileMgr-->>MenuActions: Return model
MenuActions-->>UIButton: Return UIMenu
UIButton->>User: Display menu items
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@damus/Features/Events/EventMutingContainerView.swift`:
- Around line 47-59: The new_unmutes handler's condition is inverted: change the
check inside the onReceive for handle_notify(.new_unmutes) so it tests that
damus_state.mutelist_manager.event_muted_reason(event) == nil (meaning the event
is no longer muted) instead of != nil; when it is nil, set shown = true and
muted_reason = nil (leave existing behavior for shown/muted_reason assignments
intact) so events unmuted are un-hidden and still-muted events are not
incorrectly cleared.
In `@damus/Features/Profile/Views/FriendIcon.swift`:
- Around line 10-43: Remove the deprecated explicit scale assignment on the
renderer format in the _friendIcon initializer: delete the line assigning
UIScreen.main.scale to UIGraphicsImageRendererFormat.scale and rely on the
default format behavior instead (keep the creation of
UIGraphicsImageRendererFormat() and the renderer/image closure as-is); reference
symbols: _friendIcon, UIGraphicsImageRendererFormat, UIGraphicsImageRenderer.
In `@damus/Features/Profile/Views/ProfileView.swift`:
- Line 83: The `@State` property scrollTracker in ProfileView should be made
private to prevent external mutation; change the declaration of scrollTracker
(of type ScrollOffsetTracker) from "@State var scrollTracker =
ScrollOffsetTracker()" to a private `@State` property and update any external
accesses to use internal bindings or expose controlled interfaces if needed
(search for scrollTracker usages in ProfileView to ensure visibility changes
don't break references).
In `@damus/Shared/Utilities/LocalizationUtil.swift`:
- Around line 10-24: The module-level dictionary _bundleCache is accessed
unsafely from bundleForLocale causing a potential data race; make access
thread-safe by introducing a synchronization primitive (e.g., a private
DispatchQueue or an NSLock) guarding all reads and writes to _bundleCache, wrap
the cache lookup/update in that lock/queue, and keep the critical section
minimal (check cache under lock, if not present then compute path and create
Bundle with either a second locked update or a dispatch_sync/lock around only
the write) so callers of bundleForLocale remain safe for concurrent use.
🧹 Nitpick comments (22)
damus/Shared/Utilities/TimeAgo.swift (1)
10-16: Good optimization — but note potential thread-safety concern with shared formatter.
DateComponentsFormatter(anNSFormattersubclass) is not documented as thread-safe by Apple. Sincetime_ago_sinceis a public function callable from any thread, concurrent access to_timeAgoFormattercould theoretically race.In practice this is unlikely to manifest for
string(from: DateComponents), but if you want to be safe you could use a@Sendable-friendly approach such as a per-thread formatter via a small actor orDispatchQueue-based accessor — or simply accept the (very low) risk given the measured perf win.damus/Shared/Extensions/OffsetExtension.swift (1)
33-38: Intentional use of@Statewith a reference type to suppress view invalidation — add a brief doc comment.The pattern of wrapping a
classin@Stateto avoid SwiftUI re-renders on scroll-offset mutations is a well-known performance trick and clearly intentional here. However, since@Stateis officially designed for value types, a brief doc comment onStorageexplaining why a reference type is used (to decouple offset bookkeeping from view invalidation) would help future maintainers understand the intent and avoid "fixing" it back to@State var currentOffset.Suggested doc comment
- private class Storage { - var currentOffset: CGFloat = 0 - var previousOffset: CGFloat = 0 - } + /// Reference-type wrapper so that offset mutations do not trigger + /// SwiftUI view invalidation. Wrapped in `@State` only to preserve + /// identity across view re-renders; property changes intentionally + /// bypass the SwiftUI diff. + private class Storage { + var currentOffset: CGFloat = 0 + var previousOffset: CGFloat = 0 + }As per coding guidelines: "Ensure docstring coverage for any code added or modified."
damus/Shared/Media/Video/DamusVideoPlayer.swift (1)
209-218: RedundantDispatchQueue.main.async— the observer already fires on.main.The periodic time observer is registered with
queue: .main(line 210), so the closure already executes on the main queue. The innerDispatchQueue.main.asyncon line 212 adds an unnecessary extra hop, deferring the update to the next run-loop iteration.Proposed simplification
videoCurrentTimeObserver = player.addPeriodicTimeObserver(forInterval: CMTime(seconds: 1, preferredTimescale: 600), queue: .main) { [weak self] time in guard let self else { return } - DispatchQueue.main.async { - if self.is_editing_current_time == false { - self.current_time = time.seconds - self.currentTimeSubject.send(time.seconds) - } + if self.is_editing_current_time == false { + self.current_time = time.seconds + self.currentTimeSubject.send(time.seconds) } }damus/Shared/Media/Video/DamusVideoControlsView.swift (1)
83-96:DateComponentsFormatteris allocated on every call — consider caching.
DateComponentsFormatteris expensive to create and this runs every second viaonReceive. The PR objectives mention a "singleton DateComponentsFormatter" optimization, but this site still allocates a new one per invocation. Astatic letwould eliminate the repeated allocation.Proposed fix
+ private static let shortFormatter: DateComponentsFormatter = { + let f = DateComponentsFormatter() + f.unitsStyle = .positional + f.zeroFormattingBehavior = [.pad] + return f + }() + func formatTimeInterval(_ interval: TimeInterval) -> String { if interval.isNaN { return "--:--" } - let formatter = DateComponentsFormatter() - formatter.allowedUnits = interval >= 60 * 60 ? [.hour, .minute, .second] : [.minute, .second] - formatter.unitsStyle = .positional - formatter.zeroFormattingBehavior = [.pad] - - guard let formattedString = formatter.string(from: interval) else { + Self.shortFormatter.allowedUnits = interval >= 60 * 60 ? [.hour, .minute, .second] : [.minute, .second] + guard let formattedString = Self.shortFormatter.string(from: interval) else { return "" } return formattedString }Note: Since
allowedUnitsis mutated per call, if thread safety is a concern you could use two separate static formatters (one for short, one for long durations) instead.damus/Shared/Utilities/LocalizationUtil.swift (1)
10-27: Missing docstrings on added/modified symbols.
_bundleCache,bundleForLocale, and_enUSFallbackBundlelack documentation. As per coding guidelines: "Ensure docstring coverage for any code added or modified."damus/Shared/Extensions/KFOptionSetter+.swift (2)
114-128: BOM bytes are skipped individually rather than as a 3-byte sequence.The UTF-8 BOM is the 3-byte sequence
EF BB BF, but the loop treats each byte as an independent skip character. This means a stray0xBBor0xBFappearing before<in non-BOM data would also be skipped. Since this is just a heuristic gating strictString/SVGparsing downstream, false positives are harmless — but worth noting for correctness.Also,
data[i]assumes zero-based indexing, which holds for non-sliceDatabut is technically not guaranteed by theDataAPI. Usingdata[data.startIndex + i](or iterating via the collection directly) would be more robust.Suggested more robust BOM handling
private func data_looks_like_svg(_ data: Data) -> Bool { - // Skip leading whitespace and BOM let limit = min(data.count, 256) - var i = 0 + var i = data.startIndex + let end = data.index(data.startIndex, offsetBy: limit) + // Skip UTF-8 BOM if present + if data.count >= 3 && data[data.startIndex] == 0xEF && data[data.startIndex+1] == 0xBB && data[data.startIndex+2] == 0xBF { + i = data.startIndex + 3 + } + // Skip leading whitespace - while i < limit { + while i < end { let b = data[i] - if b == 0x20 || b == 0x09 || b == 0x0A || b == 0x0D || b == 0xEF || b == 0xBB || b == 0xBF { + if b == 0x20 || b == 0x09 || b == 0x0A || b == 0x0D { i += 1 continue } - // Must start with '<' return b == 0x3C // '<' } return false }
130-166:maxSizeproperty appears unused after this change.The
maxSizestored property onCustomImageProcessor(line 132) is no longer referenced inprocess(item:options:)now that downsampling is unconditional. The same applies toCustomCacheSerializer.maxSize(line 170). Consider removing these to avoid confusion, or mark them with a comment if they're kept for future use.#!/bin/bash # Verify that maxSize is not used anywhere in these structs beyond storage rg -n 'maxSize' --type swiftdamus/Features/Profile/Models/ProfileModel.swift (1)
41-41: Missing docstrings on new/modified members.Per coding guidelines, added or modified code should have docstring coverage. The new
contactPubkeysproperty (line 41), the modifiedfollows(pubkey:)(line 51),handle_profile_contact_event(line 137), and the changedadd_eventpath (line 154) all lack documentation. Brief doc comments explaining the caching strategy and whyobjectWillChangeis intentionally suppressed would help future maintainers.damus/Shared/Utilities/Router.swift (2)
56-61: Well-documented rationale forAnyViewin navigation routing.The comment clearly explains why
AnyViewis the right trade-off here: a one-time-per-push erasure avoids the deep_ConditionalContentnesting that a 44-branch@ViewBuilderwould create. This is a sound approach for navigation destination factories.One minor gap per coding guidelines: the modified function lacks a docstring.
📝 Suggested docstring
+ /// Resolves this route to the corresponding destination view, wrapped in `AnyView` + /// to avoid deep `_ConditionalContent` nesting from the large case switch. + /// + /// Called once per navigation push — not per frame — so the `AnyView` diffing + /// cost is negligible. // AnyView is intentional here: a 44-case `@ViewBuilder` switch creates 44 // levels of nested _ConditionalContent that SwiftUI must evaluate on every // navigation push. AnyView flattens this to a single type. Navigation // destinations are created once per push (not per frame), so the usual // AnyView diffing penalty does not apply. func view(navigationCoordinator: NavigationCoordinator, damusState: DamusState) -> AnyView {As per coding guidelines: "Ensure docstring coverage for any code added or modified".
152-154: Pre-existing:==implemented viahashValuecomparison is unsound.This is not introduced by this PR, but the new
Followingcase interacts with it. ComparinghashValuefor equality means hash collisions produce false positives — two distinct routes could compare as equal, causingNavigationCoordinator.push(line 276) to silently drop a valid navigation. With 44 cases and string-prefixed hashing the practical collision risk is low, but this violatesHashable/Equatablesemantics.Not blocking, but flagging for awareness since this PR touches the enum extensively.
damus/Features/Chat/ChatroomThreadView.swift (3)
12-17: Add a docstring toUntrustedSectionMinYKey.New types should have documentation explaining their purpose. As per coding guidelines, "Ensure docstring coverage for any code added or modified."
Proposed fix
+/// Preference key that tracks the global minimum-Y position of the untrusted-network +/// section, used to trigger the sticky header when the section scrolls near the top. private struct UntrustedSectionMinYKey: PreferenceKey { static var defaultValue: CGFloat = .infinity
293-309: Scroll-driven@Stateupdates on every frame may cause unnecessary view invalidations.
contentTopYandcontentBottomYare@Stateproperties that are updated on every scroll position change (line 297-298), but they are never read directly inbody—they're only consumed insideupdateReadingProgress()andupdateChromeVisibility(), which are called right here in the closure. For non-longform events, those methods early-return without mutating any body-visible state, yet the@Statewrites tocontentTopY/contentBottomYmay still mark the view as dirty and trigger a body re-evaluation.Given this PR's focus on reducing main-thread hitches during scrolling, consider storing these intermediate tracking values in a non-published reference type (similar to the
ScrollOffsetTracker/OffsetHelperpattern mentioned in other files in this PR) so that only the final derived states (readingProgress,chromeHidden) drive view updates.#!/bin/bash # Check if ScrollOffsetTracker or OffsetHelper reference types exist in the repo # to see if there's an established pattern to reuse here rg -n "class ScrollOffsetTracker|class OffsetHelper" --type swift
327-334: Extract magic threshold100into a named constant.The value
100on line 328 controls when the sticky header appears. A named constant next toscrollThreshold(line 43) would improve readability and make it easier to tune both thresholds together.Proposed fix
/// Minimum scroll distance before triggering chrome hide/show private let scrollThreshold: CGFloat = 15 + /// Global-Y threshold at which the untrusted-network sticky header becomes visible + private let stickyHeaderTriggerY: CGFloat = 100.onPreferenceChange(UntrustedSectionMinYKey.self) { newY in - let shouldShow = newY <= 100 + let shouldShow = newY <= stickyHeaderTriggerY if shouldShow != showStickyHeader {damus/Features/Events/EventMutingContainerView.swift (1)
21-23:@Stateproperties should beprivate.SwiftLint flags
shown(line 21) andmuted_reason(line 23) as non-private@Stateproperties. Since they are only written viaState(initialValue:)in the initializers and mutated internally, marking themprivateis safe and aligns with SwiftUI best practices.Proposed fix
- `@State` var shown: Bool + `@State` private var shown: Bool - `@State` var muted_reason: MuteItem? + `@State` private var muted_reason: MuteItem?damus/Features/Timeline/Views/TimelineView.swift (1)
131-142:handle_scroll_queuemay now be dead code.The
ScrollQueueprotocol andhandle_scroll_queuefunction remain at the bottom of this file, but the only apparent caller (theGeometryReaderinMainContent) was just replaced with the inline logic on lines 103–106. If no other callers exist, consider removing this dead code.#!/bin/bash # Check if handle_scroll_queue or ScrollQueue is used elsewhere rg -n 'handle_scroll_queue\|ScrollQueue' --type swiftdamus/Features/Profile/Views/FriendIcon.swift (1)
45-56: Same deprecatedUIScreen.main.scaleusage in_fofIcon.Same fix as above — remove the redundant and deprecated
UIScreen.main.scaleassignment.Proposed fix
private let _fofIcon: UIImage = { let size = CGSize(width: 21, height: 14) let format = UIGraphicsImageRendererFormat() - format.scale = UIScreen.main.scale let renderer = UIGraphicsImageRenderer(size: size, format: format)damus/Features/Profile/Views/ProfileView.swift (6)
68-70: Missing docstring onScrollOffsetTracker.As per coding guidelines, added/modified code should have docstring coverage. A brief doc comment explaining the purpose and why it's a reference type (to isolate scroll-driven invalidations from the parent
ProfileView) would help future readers.Suggested doc
+/// Reference-type tracker for vertical scroll offset. +/// +/// Used as `@State` (not `@StateObject`) in `ProfileView` so that changes to +/// `yOffset` do **not** invalidate `ProfileView.body`. Only child subviews that +/// declare `@ObservedObject var scrollTracker` will re-evaluate on scroll. class ScrollOffsetTracker: ObservableObject { `@Published` var yOffset: CGFloat = 0 }
345-348:blurOpacity()is duplicated across three subviews.The identical calculation appears in
ProfileBannerSection,ProfileToolbarLeading, andProfileToolbarTrailing. Extract it into a shared helper onScrollOffsetTracker(or as a free function) to keep it DRY and ensure future changes stay consistent.Suggested extraction
class ScrollOffsetTracker: ObservableObject { `@Published` var yOffset: CGFloat = 0 + + /// Opacity for the blur/overlay effect based on scroll position. + func blurOpacity(navbarHeight: CGFloat) -> Double { + let progress = -(yOffset + navbarHeight) / 100 + return Double(-yOffset > navbarHeight ? progress : 0) + } }Then in each subview, replace
blurOpacity()calls withscrollTracker.blurOpacity(navbarHeight: navbarHeight).Also applies to: 618-621, 652-655
301-305: Commented-outfollowers.subscribe()is unexplained.Line 304 has
//followers.subscribe()with no explanation of why it was disabled. Either remove it or add a// TODO:/ comment explaining the reasoning, to avoid confusion for future maintainers.
260-268: Consider usingswitchinstead of three independentifblocks.Since
filter_stateis a single enum value, these conditions are mutually exclusive. Aswitch(orif/else if) would make the exclusivity explicit and avoid evaluating all three predicates on every body evaluation.Suggested refactor
- if filter_state == FilterState.posts { - InnerTimelineView(events: profile.events, damus: damus_state, filter: content_filter(FilterState.posts)) - } - if filter_state == FilterState.posts_and_replies { - InnerTimelineView(events: profile.events, damus: damus_state, filter: content_filter(FilterState.posts_and_replies)) - } - if filter_state == FilterState.conversations && !profile.conversation_events.isEmpty { - InnerTimelineView(events: profile.events, damus: damus_state, filter: content_filter(FilterState.conversations)) - } + switch filter_state { + case .posts: + InnerTimelineView(events: profile.events, damus: damus_state, filter: content_filter(.posts)) + case .posts_and_replies: + InnerTimelineView(events: profile.events, damus: damus_state, filter: content_filter(.posts_and_replies)) + case .conversations: + if !profile.conversation_events.isEmpty { + InnerTimelineView(events: profile.events, damus: damus_state, filter: content_filter(.conversations)) + } + default: + EmptyView() + }
415-608:ProfileAboutSectionis still quite large — consider documenting its responsibilities.At ~190 lines with 6 sub-methods, this struct is the largest private subview. A docstring on the struct and at least its
bodywould satisfy the coding guidelines' docstring coverage requirement and help readers understand the layout contract (padding, spacing, etc.) at a glance. The same applies to the other new private structs (ProfileBannerSection,ProfilePicScrollEffect,ProfileToolbarLeading,ProfileToolbarTrailing). As per coding guidelines: "Ensure docstring coverage for any code added or modified."
657-659:showFollowBtn()conflates scroll position with UI visibility — worth a brief comment.The condition
blurOpacity() > 1.0as a proxy for "user has scrolled past the profile header" is non-obvious. A one-line comment explaining the intent (e.g., "show follow button in navbar once the header has scrolled offscreen") would clarify the magic threshold.
| .onReceive(handle_notify(.new_mutes)) { mutes in | ||
| let new_muted_event_reason = damus_state.mutelist_manager.event_muted_reason(event) | ||
| if new_muted_event_reason != nil { | ||
| shown = false | ||
| muted_reason = new_muted_event_reason | ||
| } | ||
| else { | ||
| EventMutedBoxView(shown: $shown, reason: muted_reason) | ||
| } | ||
| .onReceive(handle_notify(.new_unmutes)) { unmutes in | ||
| if damus_state.mutelist_manager.event_muted_reason(event) != nil { | ||
| shown = true | ||
| muted_reason = nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Inverted condition in new_unmutes handler will show still-muted events.
The new_mutes handler (line 49) correctly checks event_muted_reason(event) != nil to confirm the event is now muted. But the new_unmutes handler (line 55) uses the same != nil check — meaning it acts only when the event is still muted, which is the opposite of the intent. This will fail to un-hide events that were actually unmuted, and incorrectly clear muted_reason for events that remain muted for a different reason.
🐛 Proposed fix: invert the condition
.onReceive(handle_notify(.new_unmutes)) { unmutes in
- if damus_state.mutelist_manager.event_muted_reason(event) != nil {
+ if damus_state.mutelist_manager.event_muted_reason(event) == nil {
shown = true
muted_reason = nil
}
}📝 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.
| .onReceive(handle_notify(.new_mutes)) { mutes in | |
| let new_muted_event_reason = damus_state.mutelist_manager.event_muted_reason(event) | |
| if new_muted_event_reason != nil { | |
| shown = false | |
| muted_reason = new_muted_event_reason | |
| } | |
| else { | |
| EventMutedBoxView(shown: $shown, reason: muted_reason) | |
| } | |
| .onReceive(handle_notify(.new_unmutes)) { unmutes in | |
| if damus_state.mutelist_manager.event_muted_reason(event) != nil { | |
| shown = true | |
| muted_reason = nil | |
| } | |
| } | |
| .onReceive(handle_notify(.new_mutes)) { mutes in | |
| let new_muted_event_reason = damus_state.mutelist_manager.event_muted_reason(event) | |
| if new_muted_event_reason != nil { | |
| shown = false | |
| muted_reason = new_muted_event_reason | |
| } | |
| } | |
| .onReceive(handle_notify(.new_unmutes)) { unmutes in | |
| if damus_state.mutelist_manager.event_muted_reason(event) == nil { | |
| shown = true | |
| muted_reason = nil | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@damus/Features/Events/EventMutingContainerView.swift` around lines 47 - 59,
The new_unmutes handler's condition is inverted: change the check inside the
onReceive for handle_notify(.new_unmutes) so it tests that
damus_state.mutelist_manager.event_muted_reason(event) == nil (meaning the event
is no longer muted) instead of != nil; when it is nil, set shown = true and
muted_reason = nil (leave existing behavior for shown/muted_reason assignments
intact) so events unmuted are un-hidden and still-muted events are not
incorrectly cleared.
| /// Pre-rendered friend icons to avoid per-frame gradient mask compositing. | ||
| /// The gradient mask + SF Symbol composition is expensive in Core Animation, | ||
| /// so we render once into a UIImage and blit the bitmap on each cell. | ||
| private let _friendIcon: UIImage = { | ||
| let size = CGSize(width: 20, height: 14) | ||
| let format = UIGraphicsImageRendererFormat() | ||
| format.scale = UIScreen.main.scale | ||
| let renderer = UIGraphicsImageRenderer(size: size, format: format) | ||
| return renderer.image { rendererCtx in | ||
| let rect = CGRect(origin: .zero, size: size) | ||
| let cg = rendererCtx.cgContext | ||
|
|
||
| let config = UIImage.SymbolConfiguration(pointSize: 14, weight: .regular) | ||
| guard let symbol = UIImage(systemName: "person.fill.checkmark", withConfiguration: config) else { return } | ||
|
|
||
| // Draw symbol into the context (acts as mask source) | ||
| symbol.withTintColor(.white, renderingMode: .alwaysOriginal).draw(in: rect) | ||
|
|
||
| // Composite gradient using sourceIn so it only fills where the symbol was drawn | ||
| cg.saveGState() | ||
| cg.setBlendMode(.sourceIn) | ||
| let colors: [CGColor] = [ | ||
| UIColor(red: 204/255.0, green: 67/255.0, blue: 197/255.0, alpha: 1).cgColor, // DamusPurple | ||
| UIColor(red: 75/255.0, green: 77/255.0, blue: 255/255.0, alpha: 1).cgColor // DamusBlue | ||
| ] | ||
| guard let gradient = CGGradient(colorsSpace: CGColorSpaceCreateDeviceRGB(), colors: colors as CFArray, locations: nil) else { | ||
| cg.restoreGState() | ||
| return | ||
| } | ||
| // topTrailing → bottomTrailing to match LINEAR_GRADIENT | ||
| cg.drawLinearGradient(gradient, start: CGPoint(x: rect.maxX, y: 0), end: CGPoint(x: rect.maxX, y: rect.maxY), options: []) | ||
| cg.restoreGState() | ||
| } | ||
| }() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
UIScreen.main deprecated iOS 16 Apple documentation
💡 Result:
UIScreen.main itself isn’t removed in iOS 16, but Xcode 14+ commonly emits a warning like:
“‘main’ will be deprecated in a future version of iOS: Use a UIScreen instance found through context instead: i.e.
view.window.windowScene.screen” (stackoverflow.com)
Apple’s recommended direction is to stop treating “the screen” as a global singleton (because of multiple windows/scenes and external displays) and instead fetch the UIScreen from the relevant UIWindowScene (for the window/view you’re working with). (stackoverflow.com)
Common replacements:
- From a view/controller context:
view.window?.windowScene?.screen(stackoverflow.com) - More generally (iterate scenes/windows, then use
window.screen): (stackoverflow.com)
Citations:
- 1: https://stackoverflow.com/questions/75375870/uiscreen-main-is-deprecated-what-are-other-solutions-other-than-geometryreader?utm_source=openai
- 2: https://stackoverflow.com/questions/74458971/correct-way-to-get-the-screen-size-on-ios-after-uiscreen-main-deprecation?utm_source=openai
- 3: https://stackoverflow.com/questions/75375870/uiscreen-main-is-deprecated-what-are-other-solutions-other-than-geometryreader?utm_source=openai
- 4: https://stackoverflow.com/questions/74458971/correct-way-to-get-the-screen-size-on-ios-after-uiscreen-main-deprecation?utm_source=openai
🌐 Web query:
UIGraphicsImageRendererFormat default scale behavior
💡 Result:
UIGraphicsImageRendererFormat’s scale defaults to the main screen’s display scale (i.e., UIScreen.main.scale, commonly 2.0 or 3.0). That means a renderer created with a size in points will rasterize to points × scale in pixels unless you override it. [1] [2]
To force a specific output pixel scale, set it explicitly:
let format = UIGraphicsImageRendererFormat()
format.scale = 1.0 // produce 1 px per point
let renderer = UIGraphicsImageRenderer(size: CGSize(width: 300, height: 300),
format: format)
let image = renderer.image { _ in /* draw */ }[1] [3]
Sources: [1] [2] [3]
Remove the redundant and deprecated UIScreen.main.scale assignment.
UIGraphicsImageRendererFormat() defaults to the main screen's scale, so line 16 is unnecessary. Additionally, UIScreen.main is deprecated in favor of fetching the screen from context (e.g., view.window?.windowScene?.screen), though in a global initializer this alternative isn't available. Simply removing the explicit assignment eliminates the deprecated API usage while preserving the intended behavior.
Proposed fix
private let _friendIcon: UIImage = {
let size = CGSize(width: 20, height: 14)
let format = UIGraphicsImageRendererFormat()
- format.scale = UIScreen.main.scale
let renderer = UIGraphicsImageRenderer(size: size, format: format)🤖 Prompt for AI Agents
In `@damus/Features/Profile/Views/FriendIcon.swift` around lines 10 - 43, Remove
the deprecated explicit scale assignment on the renderer format in the
_friendIcon initializer: delete the line assigning UIScreen.main.scale to
UIGraphicsImageRendererFormat.scale and rely on the default format behavior
instead (keep the creation of UIGraphicsImageRendererFormat() and the
renderer/image closure as-is); reference symbols: _friendIcon,
UIGraphicsImageRendererFormat, UIGraphicsImageRenderer.
| @State var mute_dialog_presented: Bool = false | ||
| @State var filter_state : FilterState = .posts | ||
| @State var yOffset: CGFloat = 0 | ||
| @State var scrollTracker = ScrollOffsetTracker() |
There was a problem hiding this comment.
@State property should be private.
SwiftLint flags this correctly. SwiftUI @State properties should be private to prevent external mutation that bypasses SwiftUI's state management.
- `@State` var scrollTracker = ScrollOffsetTracker()
+ `@State` private var scrollTracker = ScrollOffsetTracker()📝 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.
| @State var scrollTracker = ScrollOffsetTracker() | |
| `@State` private var scrollTracker = ScrollOffsetTracker() |
🧰 Tools
🪛 SwiftLint (0.63.2)
[Warning] 83-83: SwiftUI state properties should be private
(private_swiftui_state)
🤖 Prompt for AI Agents
In `@damus/Features/Profile/Views/ProfileView.swift` at line 83, The `@State`
property scrollTracker in ProfileView should be made private to prevent external
mutation; change the declaration of scrollTracker (of type ScrollOffsetTracker)
from "@State var scrollTracker = ScrollOffsetTracker()" to a private `@State`
property and update any external accesses to use internal bindings or expose
controlled interfaces if needed (search for scrollTracker usages in ProfileView
to ensure visibility changes don't break references).
| private var _bundleCache: [String: Bundle] = [:] | ||
|
|
||
| func bundleForLocale(locale: Locale) -> Bundle { | ||
| let path = Bundle.main.path(forResource: locale.identifier, ofType: "lproj") | ||
| return path != nil ? (Bundle(path: path!) ?? Bundle.main) : Bundle.main | ||
| let id = locale.identifier | ||
| if let cached = _bundleCache[id] { | ||
| return cached | ||
| } | ||
| let bundle: Bundle | ||
| if let path = Bundle.main.path(forResource: id, ofType: "lproj") { | ||
| bundle = Bundle(path: path) ?? Bundle.main | ||
| } else { | ||
| bundle = Bundle.main | ||
| } | ||
| _bundleCache[id] = bundle | ||
| return bundle |
There was a problem hiding this comment.
Data race on _bundleCache: unsynchronized read/write from multiple threads.
_bundleCache is a module-level var dictionary accessed without any synchronization. If bundleForLocale is called concurrently (e.g., from background localization tasks), simultaneous read/write on Dictionary is undefined behavior in Swift and can crash.
Wrap access in a lock or serial queue.
🔒 Proposed fix using a lock
-private var _bundleCache: [String: Bundle] = [:]
+private var _bundleCache: [String: Bundle] = [:]
+private let _bundleCacheLock = NSLock()
func bundleForLocale(locale: Locale) -> Bundle {
let id = locale.identifier
+ _bundleCacheLock.lock()
if let cached = _bundleCache[id] {
+ _bundleCacheLock.unlock()
return cached
}
let bundle: Bundle
if let path = Bundle.main.path(forResource: id, ofType: "lproj") {
bundle = Bundle(path: path) ?? Bundle.main
} else {
bundle = Bundle.main
}
_bundleCache[id] = bundle
+ _bundleCacheLock.unlock()
return bundle
}Alternatively, use defer for cleaner unlock semantics (at the cost of holding the lock during Bundle.main.path lookup — acceptable given it's a fast filesystem metadata check):
-private var _bundleCache: [String: Bundle] = [:]
+private var _bundleCache: [String: Bundle] = [:]
+private let _bundleCacheLock = NSLock()
func bundleForLocale(locale: Locale) -> Bundle {
let id = locale.identifier
+ _bundleCacheLock.lock()
+ defer { _bundleCacheLock.unlock() }
if let cached = _bundleCache[id] {
return cached
}As per coding guidelines: "Never block the main thread: All network requests, database queries, and expensive computations must run on background threads/queues … UI updates must dispatch back to @MainActor" — concurrent access from these background paths makes synchronization essential here.
📝 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.
| private var _bundleCache: [String: Bundle] = [:] | |
| func bundleForLocale(locale: Locale) -> Bundle { | |
| let path = Bundle.main.path(forResource: locale.identifier, ofType: "lproj") | |
| return path != nil ? (Bundle(path: path!) ?? Bundle.main) : Bundle.main | |
| let id = locale.identifier | |
| if let cached = _bundleCache[id] { | |
| return cached | |
| } | |
| let bundle: Bundle | |
| if let path = Bundle.main.path(forResource: id, ofType: "lproj") { | |
| bundle = Bundle(path: path) ?? Bundle.main | |
| } else { | |
| bundle = Bundle.main | |
| } | |
| _bundleCache[id] = bundle | |
| return bundle | |
| private var _bundleCache: [String: Bundle] = [:] | |
| private let _bundleCacheLock = NSLock() | |
| func bundleForLocale(locale: Locale) -> Bundle { | |
| let id = locale.identifier | |
| _bundleCacheLock.lock() | |
| if let cached = _bundleCache[id] { | |
| _bundleCacheLock.unlock() | |
| return cached | |
| } | |
| let bundle: Bundle | |
| if let path = Bundle.main.path(forResource: id, ofType: "lproj") { | |
| bundle = Bundle(path: path) ?? Bundle.main | |
| } else { | |
| bundle = Bundle.main | |
| } | |
| _bundleCache[id] = bundle | |
| _bundleCacheLock.unlock() | |
| return bundle | |
| } |
🤖 Prompt for AI Agents
In `@damus/Shared/Utilities/LocalizationUtil.swift` around lines 10 - 24, The
module-level dictionary _bundleCache is accessed unsafely from bundleForLocale
causing a potential data race; make access thread-safe by introducing a
synchronization primitive (e.g., a private DispatchQueue or an NSLock) guarding
all reads and writes to _bundleCache, wrap the cache lookup/update in that
lock/queue, and keep the critical section minimal (check cache under lock, if
not present then compute path and create Bundle with either a second locked
update or a dispatch_sync/lock around only the write) so callers of
bundleForLocale remain safe for concurrent use.
|
@jb55, can you please add sign-offs and changelog entries on the commits? |
danieldaquino
left a comment
danieldaquino
left a comment
There was a problem hiding this comment.
Please format PR and commits to follow guidelines at https://github.com/damus-io/damus/blob/master/docs/CONTRIBUTING.md.
https://github.com/damus-io/damus/blob/master/.github/pull_request_template.md
|
the commits are fine, we don't need to be so anal about this |
Should we make the sign-off requirement optional in the contribution guidelines? |
|
There is an issue with the profile view banner heading: Expected behaviour (Taken from 1.17 before the PR): 
Other elements seem to be working as expected. |
danieldaquino
left a comment
danieldaquino
left a comment
There was a problem hiding this comment.
Thanks @jb55, changes make sense and views seem to be working after changes. There only seems to be one new issue (as far as I have tested) with the profile banner top bar that we should address before merging (More details on the previous comment).
|
Submitted to TestFlight for testing with users. Version and build number: v1.17 (1299) |
|
One issue raised by a user: The home view does not scroll/swipe between Notes and Replies and Notes. Quote:
@jb55, is it feasible to maintain that behaviour alongside the relevant performance improvement? |
|
Do people use that? Apparently its a huge performance issue because it actually renders both views |
Replace thread line background GeometryReader with overlay, consolidate top+bottom scroll position trackers into a single overlay, and replace untrusted section tracker with PreferenceKey. Remove dead untrustedSectionOffset state variable. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Replace AnyView type erasure with direct view return. Replace DispatchQueue.main.async yOffset update with .onChange(of: minY) which fires after layout without the overhead of dispatching to the run loop. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Fold handle_scroll_queue logic into the existing .offsetY closure which already fires every scroll frame, eliminating an extra GeometryReader that was doing redundant per-frame layout work. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Replace AnyView wrappers in space_if_spread and content with @ViewBuilder, allowing SwiftUI to preserve structural identity for the action bar rendered on every note cell. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Replace O(n) linear scan of contact event tags in follows() with a Set<Pubkey> lookup. The set is built once when the contact event arrives in handle_profile_contact_event. Co-Authored-By: Claude Opus 4.6 <[email protected]>
…ocation The formatter was being allocated on every call, which runs per-note on every timeline cell render. Profiler showed 8ms in this path. Now uses a static instance initialized once. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Array(contact.referenced_pubkeys) and Array(contact.referenced_hashtags) were running on every ProfileView.body evaluation, iterating all contact tags to build arrays for a FollowingModel that may never be used. Now Route.Following stores the contacts event directly, and the model is built lazily in the router when the user actually taps "Following". Co-Authored-By: Claude Opus 4.6 <[email protected]>
bundleForLocale was calling Bundle.main.path(forResource:ofType:) and Bundle(path:) on every call. With pluralizedString called 3+ times per ProfileView body evaluation, this added up to 12+ bundle lookups per render. Now caches bundles by locale identifier and pre-resolves the en-US fallback bundle as a static. Co-Authored-By: Claude Opus 4.6 <[email protected]>
The @State value types for currentOffset and previousOffset were causing SwiftUI to re-evaluate the OffsetHelper body on every scroll frame, triggering cascading layout passes through the LazyVStack. Co-Authored-By: Claude Opus 4.6 <[email protected]>
The FriendIcon uses LINEAR_GRADIENT.mask(SF Symbol) which forces Core Animation to perform offscreen gradient mask compositing on every timeline cell, every frame. Profiling showed ~4.4ms avg per OpacityRendererEffect hit (2 per scroll frame, ~8.8ms total) deep in the cell render path at FriendIcon. Pre-render both the gradient-masked friend icon and the gray fof icon into UIImages once at first access using UIGraphicsImageRenderer. The view now blits a cached bitmap instead of recompositing the gradient mask per frame. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Every timeline cell renders through EventView, EventShell, and EventMutingContainerView. Each had unnecessary wrapper containers that add nodes to the SwiftUI view tree: - EventView wrapped its conditional (boost/zap/text/etc) in a VStack that served no layout purpose since only one branch renders. The VStack forces SwiftUI to allocate a layout container and diff it. - EventShell wrapped its Wide/Threaded conditional in a Group, which adds a node SwiftUI must track through its diffing pass. - EventMutingContainerView used an AnyView-returning closure for custom mute boxes. AnyView type-erases the view, preventing SwiftUI from doing structural identity diffing -- it must tear down and rebuild the entire subtree instead of incremental updates. Changed to a generic MuteBox type parameter so SwiftUI knows the concrete view type at compile time. All replaced with @ViewBuilder which emits _ConditionalContent directly, letting SwiftUI structurally diff without extra containers. Co-Authored-By: Claude Opus 4.6 <[email protected]>
The 44-case @ViewBuilder switch created 44 levels of nested _ConditionalContent that SwiftUI had to evaluate on every navigation push. Replace with explicit AnyView returns to flatten the type. Navigation destinations are one-shot constructions (not per-frame), so the AnyView diffing penalty does not apply. Co-Authored-By: Claude Opus 4.6 <[email protected]>
…ecode The image processor and cache serializer were only downsampling when data exceeded maxSize (5MiB for pfp, 20MiB for banner). Images under that threshold went through DefaultImageProcessor which produces a CGImage with deferred JPEG decoding — Core Animation then re-decodes the full-resolution JPEG on the main thread during CA::Layer::prepare_commit (12ms+ for progressive JPEGs). Now all images go through CGImageSourceCreateThumbnailAtIndex which decodes directly at the target resolution (200x200 pfp, 750x250 banner), producing a fully materialized bitmap that CA can commit without any additional decode. Also adds a fast SVG pre-check (data_looks_like_svg) that scans for a '<' byte in the first 256 bytes before attempting String(data:encoding:), avoiding a speculative string allocation on every binary image. Co-Authored-By: Claude Opus 4.6 <[email protected]>
…aversal SwiftUI Menu eagerly evaluates its content builder on every row body, creating deeply nested layout nodes that cost 15ms+ per row. UIButton with UIDeferredMenuElement.uncached defers menu construction to tap time and acts as an opaque leaf node with no recursive spacing traversal. Co-Authored-By: Claude Opus 4.6 <[email protected]>
AVPlayerViewController.setPlayer rebuilds internal video layers (~13ms) even when assigned the same reference. Guard with identity check to avoid unnecessary work during SwiftUI view updates. Co-Authored-By: Claude Opus 4.6 <[email protected]>
…evaluation Replace @State var yOffset with an ObservableObject (ScrollOffsetTracker) held via @State (reference-type, so mutations do not trigger parent body). Extract banner, pfp, and toolbar into subviews that @ObservedObject the tracker, so only they re-render on scroll (~112ms body saved per frame). Co-Authored-By: Claude Opus 4.6 <[email protected]>
EventHolder.FilteredHolder already independently notifies InnerTimelineView about new events, so the manual objectWillChange.send() was redundant — it caused a full ProfileView.body re-evaluation (~152ms) on every event from the network. Co-Authored-By: Claude Opus 4.6 <[email protected]>
current_time is updated every second via addPeriodicTimeObserver, which fires objectWillChange on every @ObservedObject subscriber — including DamusVideoPlayerView on every visible video in the timeline. Each tick re-evaluates the full body (VisibilityTracker GeometryReader + BaseView updateUIViewController). Replace @published with a plain var + CurrentValueSubject so only DamusVideoControlsView (which actually displays the timestamp) observes the updates, via .onReceive. Co-Authored-By: Claude Opus 4.6 <[email protected]>
aboutSection was a computed property on ProfileView, so it was rebuilt on every body evaluation — doing NDB lookups, followedByString() name resolution, and CondensedProfilePicturesView construction each time. Extract it into a standalone struct with @ObservedObject on profile and followers only. scrollTracker is passed as a plain let (not observed) since ProfilePicScrollEffect already handles scroll observation independently. This prevents aboutSection from re-evaluating on unrelated state changes (filter_state, share sheets, etc.). Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
I dropped that commit and rebased
…On March 4, 2026 10:58:23 AM PST, "Daniel D’Aquino" ***@***.***> wrote:
danieldaquino left a comment (damus-io/damus#3626)
One issue raised by a user: The home view does not scroll/swipe between Notes and Replies and Notes.
Quote:
> Oh wow, please fix this. I’m so used to sliding between notes / notes and replies.
@jb55, is it feasible to maintain that behaviour alongside the relevant performance improvement?
--
Reply to this email directly or view it on GitHub:
#3626 (comment)
You are receiving this because you were mentioned.
Message ID: ***@***.***>
|
Yeah |
|
apparently i pushed github/experimental-pr-3626 instead of jb55/opus-perf, not sure why there are two branches for this |
|
the visual glitch on profile is definitely annoying, but i want to fix the architectural perf issues first and fix those later |
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
damus/Features/Profile/Views/ProfileView.swift (1)
531-535:⚠️ Potential issue | 🟠 MajorDon’t run profile lookups directly in
body.Line 533 and Line 534 perform synchronous lookups during render. If storage-backed, this can add scroll hitches on the main thread. Load once via async/background and bind cached state.
As per coding guidelines, "Never block the main thread: All network requests, database queries, and expensive computations must run on background threads/queues using Task { }, DispatchQueue.global(), or Swift concurrency (async/await). UI updates must dispatch back to
@MainActor".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@damus/Features/Profile/Views/ProfileView.swift` around lines 531 - 535, The body currently performs synchronous lookups (try? damus_state.profiles.lookup_lnurl and try? damus_state.profiles.lookup(id:)) which can block rendering; move these lookups out of body into asynchronous cached state: add `@State/`@StateObject properties on ProfileView (e.g., lnurlState and ndbProfileState), kick off a Task or DispatchQueue.global() in onAppear or an init async helper to call damus_state.profiles.lookup_lnurl(profile.pubkey) and damus_state.profiles.lookup(id: profile.pubkey) off the main thread, then assign results back on the main actor; update the View to read from the cached lnurlState and ndbProfileState instead of calling lookup* directly in body.damus/Shared/Extensions/KFOptionSetter+.swift (1)
137-179:⚠️ Potential issue | 🟡 MinorAdd docstrings for modified protocol entry points.
CustomImageProcessor.process(item:options:)andCustomCacheSerializer.image(with:options:)were modified but still lack///docs. Please add short behavior/contract docs (especially around SVG handling and downsampling policy).As per coding guidelines, "Ensure docstring coverage for any code added or modified".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@damus/Shared/Extensions/KFOptionSetter`+.swift around lines 137 - 179, Add /// docstrings to CustomImageProcessor.process(item:options:) and CustomCacheSerializer.image(with:options:) describing their behavior and contract: state that process handles ImageProcessItem.data by first checking for SVG via magic bytes and UTF-8 conversion and rendering SVG to a raster image (then scaling by options.scaleFactor), otherwise it downsamples using KingfisherWrapper.downsampledImage to the configured downsampleSize; also note that .image case delegates to DefaultImageProcessor (and is not expected to run), that SVG detection avoids full String allocation for binary data, and document threading/return guarantees (returns a KFCrossPlatformImage? and may be nil on failure). Keep the docs concise and mention downsampling policy and any assumptions about options.scaleFactor and original data.
♻️ Duplicate comments (1)
damus/Features/Events/EventMutingContainerView.swift (1)
54-58:⚠️ Potential issue | 🔴 CriticalFix inverted unmute condition (still unresolved).
Line 55 checks
!= nilinside.new_unmutes, which means “still muted.” That prevents truly unmuted events from being restored correctly.🐛 Proposed fix
.onReceive(handle_notify(.new_unmutes)) { unmutes in - if damus_state.mutelist_manager.event_muted_reason(event) != nil { + if damus_state.mutelist_manager.event_muted_reason(event) == nil { shown = true muted_reason = nil } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@damus/Features/Events/EventMutingContainerView.swift` around lines 54 - 58, The unmute handler in EventMutingContainerView is using the wrong condition: inside the .onReceive(handle_notify(.new_unmutes)) block it checks damus_state.mutelist_manager.event_muted_reason(event) != nil (meaning still muted) before restoring state; change the check to == nil so you detect a truly unmuted event and then set shown = true and muted_reason = nil; update the condition in the onReceive closure that references event_muted_reason(event) accordingly.
🧹 Nitpick comments (8)
damus/Shared/Extensions/OffsetExtension.swift (1)
48-50: Skip no-op offset updates with an early return guard.This callback runs in a hot scrolling path; avoid firing
onChangewhenvaluehasn’t changed.Proposed refactor
.onPreferenceChange(OffsetKey.self) { value in + guard value != storage.currentOffset else { return } storage.previousOffset = storage.currentOffset storage.currentOffset = value onChange(storage.previousOffset, storage.currentOffset) }As per coding guidelines, "Ensure nevernesting: favor early returns and guard clauses over deeply nested conditionals; simplify control flow by exiting early instead of wrapping logic in multiple layers of if statements".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@damus/Shared/Extensions/OffsetExtension.swift` around lines 48 - 50, Add an early-return guard to skip no-op offset updates: check if the incoming value equals storage.currentOffset and return immediately if so, instead of proceeding to set storage.previousOffset/currentOffset and call onChange; update the logic around storage.previousOffset, storage.currentOffset and onChange to only run when the value actually differs.damus/Features/Timeline/Views/TimelineView.swift (1)
103-106: Document and extract the new scroll-queue toggle logic.The new behavior added at Line 103–106 is important but inline and undocumented. Please move it into a small helper with a docstring so the intent is explicit and easier to maintain.
Proposed refactor
- let new_should_queue = -current > 0 - if self.events.should_queue != new_should_queue { - self.events.set_should_queue(new_should_queue) - } + updateScrollQueueState(currentOffset: current)/// Toggles event queueing based on timeline content offset. /// Queueing is enabled once content has moved above the top boundary. private func updateScrollQueueState(currentOffset: CGFloat) { let shouldQueue = -currentOffset > 0 guard events.should_queue != shouldQueue else { return } events.set_should_queue(shouldQueue) }As per coding guidelines, "Ensure docstring coverage for any code added or modified".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@damus/Features/Timeline/Views/TimelineView.swift` around lines 103 - 106, Extract the inline scroll-queue toggle logic into a private helper named updateScrollQueueState(currentOffset: CGFloat) with a docstring explaining that it toggles event queueing when the timeline content offset moves above the top boundary; inside the helper compute let shouldQueue = -currentOffset > 0, early-return if events.should_queue == shouldQueue, otherwise call events.set_should_queue(shouldQueue); then replace the original inline block (the lines using current and calling events.set_should_queue) with a call to updateScrollQueueState(currentOffset: current).damus/Features/Chat/ChatroomThreadView.swift (1)
14-15: UsemininPreferenceKey.reduceto match key semantics if multiple values are emitted.♻️ Suggested adjustment
static func reduce(value: inout CGFloat, nextValue: () -> CGFloat) { - value = nextValue() + value = min(value, nextValue()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@damus/Features/Chat/ChatroomThreadView.swift` around lines 14 - 15, The PreferenceKey.reduce implementation currently overwrites value with nextValue(); change it to take the minimum of the existing value and nextValue() to match the key's semantics — update the static func reduce(value: inout CGFloat, nextValue: () -> CGFloat) in ChatroomThreadView.swift to set value = min(value, nextValue()) so multiple emitted values are correctly aggregated.damus/Features/Events/EventMenu.swift (2)
39-64: Add doc comments for the UIKit bridge lifecycle/helpers.
makeUIView,updateUIView, andbuildDeferredMenuare central to this integration and should be explicitly documented.As per coding guidelines, "Ensure docstring coverage for any code added or modified".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@damus/Features/Events/EventMenu.swift` around lines 39 - 64, Add doc comments to the UIKit bridge functions to explain their lifecycle roles and usage: document makeUIView(context:) to describe creating and configuring the UIButton (image, tint, showsMenuAsPrimaryAction, and initial menu), updateUIView(_:context:) to state it refreshes the button's menu when state changes, and buildDeferredMenu() to explain deferred menu construction and the use of UIDeferredMenuElement to call EventMenuButton.menuActions(damus_state:event:). Place concise Swift doc comments above each of these functions mentioning threading/main-authority expectations (if any) and parameter/return semantics.
63-170: Avoid maintaining two menu-action implementations.
EventMenuButton.menuActionsnow duplicates behavior that still exists inMenuItems, which raises drift risk over time. Consider extracting one shared action builder and reusing it from both paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@damus/Features/Events/EventMenu.swift` around lines 63 - 170, The menu action logic in EventMenu.menuActions is duplicated in EventMenuButton.menuActions/MenuItems; extract the common action-building code into a single shared function (e.g., EventMenu.sharedActions(damus_state:event:) or EventMenu.buildCommonMenuActions) and replace the duplicated implementations in EventMenuButton.menuActions and MenuItems to call that shared function. Move the code that constructs the actions array (all UIAction/UIMenu creation and uses of damus_state, profileModel, isBookmarked, isMutedThread, muteDurationMenu, etc.) into the new shared function, keep muteDurationMenu and helper functions private to EventMenu, and ensure callers simply return or wrap the shared result so behavior remains identical.damus/Features/Profile/Models/ProfileModel.swift (1)
41-53: Document the follow-cache lifecycle.Please add doc comments for
contactPubkeys,follows(pubkey:), andhandle_profile_contact_event(_:)to clarify when the cache is refreshed and authoritative.As per coding guidelines, "Ensure docstring coverage for any code added or modified".
Also applies to: 137-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@damus/Features/Profile/Models/ProfileModel.swift` around lines 41 - 53, Add doc comments for the contactPubkeys property, follows(pubkey:) method, and handle_profile_contact_event(_:) method that describe the follow-cache lifecycle: state that contactPubkeys is an in-memory cache of follows for this ProfileModel, initialized empty and populated/updated only via events processed by EventHolder (mention preload_events/profile contact events), that follows(pubkey:) reads this cache as the authoritative source for current follow membership until a relevant contact/profile event updates it, and that handle_profile_contact_event(_:) is responsible for applying updates (adds/removes/refreshes) and invalidating or replacing the cache as events arrive; mention any threading/queue context (EventHolder queue) so callers know when the cache is safe to read.damus/Features/Profile/Views/ProfileView.swift (1)
68-70: Add docstrings for the new scroll-tracking architecture.
ScrollOffsetTrackerplus the extracted scroll-driven subviews are substantial additions and should have doc comments for intent/contract.As per coding guidelines, "Ensure docstring coverage for any code added or modified".
Also applies to: 334-675
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@damus/Features/Profile/Views/ProfileView.swift` around lines 68 - 70, Add docstrings for the new scroll-tracking architecture: annotate the ScrollOffsetTracker class with a brief purpose description, the contract for its `@Published` yOffset (units, coordinate origin, expected update frequency), ownership/lifecycle expectations (who instantiates and observes it), and thread/update behavior; also add similar header comments to each extracted scroll-driven subview (describe what scroll input they consume, expected yOffset ranges, how they react to values, and any public API/initializers). Ensure comments are concise, follow project docstring style, and cover intent/usage for consumers and future maintainers.damus/Shared/Extensions/KFOptionSetter+.swift (1)
112-126: SVG pre-check is broader than the documented signature check.At Line 125, the helper returns
truefor any payload whose first non-whitespace byte is<. That still triggers full UTF-8 conversion for non-SVG XML/HTML and weakens the fast-reject intent. Consider checking for<svg/<?xmlbyte prefixes beforeString(data:, encoding:).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@damus/Shared/Extensions/KFOptionSetter`+.swift around lines 112 - 126, The helper data_looks_like_svg currently returns true for any first non-whitespace byte of '<', which is too broad; modify it to peek at the subsequent bytes (safely checking bounds) and only return true when the byte sequence matches the SVG signatures like "<svg" or "<?xml" (case-insensitive if desired) — e.g., after skipping whitespace/BOM in data_looks_like_svg, check for the byte patterns for "<svg" and "<?xml" (ensure you don't read past limit or data.count) and return true only on those matches, otherwise return false so non-SVG XML/HTML is rejected quickly without attempting full UTF-8 conversion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@damus/Features/Chat/ChatroomThreadView.swift`:
- Around line 12-17: Add a doc comment for the new UntrustedSectionMinYKey
explaining its purpose (tracks the minimum Y coordinate of the "untrusted"
section in the ChatroomThreadView), what the defaultValue (.infinity) means, and
how the reduce(_:, nextValue:) implementation works (it assigns the latest
reported value). Put the doc comment immediately above the
UntrustedSectionMinYKey declaration and mention the key is a PreferenceKey used
by SwiftUI layout preference propagation so future readers understand why it
exists and how it is consumed.
In `@damus/Features/Events/EventMutingContainerView.swift`:
- Around line 25-39: Add doc comments for the two initializers and the innerBody
property/func: document the purpose of each initializer (one default when
MuteBox == EmptyView and one custom with muteBox), describe parameters
(damus_state: DamusState, event: NostrEvent, content: `@ViewBuilder` content, and
muteBox: (Binding<Bool>, MuteItem?) -> MuteBox), explain the behavior of shown
state initialization (uses should_show_event(state:ev:)), and note any semantics
of customMuteBox and how innerBody composes the view and uses the binding/Muted
state; ensure comments are concise, include parameter descriptions and any
important preconditions/side effects for innerBody, init(damus_state:event:
`@ViewBuilder` content:) and init(damus_state:event: muteBox: `@ViewBuilder`
content:).
In `@damus/Features/Events/EventView.swift`:
- Around line 34-35: Add a doc comment above the `@ViewBuilder` var body: some
View property in EventView describing the render contract: what the view
renders, when it updates, and any expectations for inputs or side effects (e.g.,
which state/props drive rendering and whether it can return different subviews
based on conditions). Update the comment to reference EventView's role so
readers know what the body is responsible for and any invariants callers can
rely on.
In `@damus/Features/Profile/Views/FriendIcon.swift`:
- Around line 45-56: The new private UIImage constant `_fofIcon` lacks the
required doc comment; add a docstring above `_fofIcon` matching the style and
content level used for `_friendIcon` describing what the icon represents
(friend-of-friend indicator), where/how it is used, and any relevant rendering
or sizing notes (e.g., pointSize/weight and tint behavior), so the declaration
for `_fofIcon` has the same docstring coverage and conventions as `_friendIcon`.
In `@damus/Features/Profile/Views/ProfileView.swift`:
- Around line 657-659: showFollowBtn() currently returns true for the signed-in
user's profile because it only checks follow_state and blurOpacity; update the
predicate to also exclude the current account by confirming profile.pubkey !=
damus_state.account?.pubkey (or the equivalent stored signed-in pubkey) so the
method becomes: return false when profile.pubkey matches the signed-in pubkey,
otherwise evaluate damus_state.contacts.follow_state(profile.pubkey) ==
.unfollows && blurOpacity() > 1.0; keep the same symbols: showFollowBtn(),
damus_state.contacts.follow_state(profile.pubkey), profile.pubkey,
damus_state.account?.pubkey, and blurOpacity().
In `@damus/Shared/Extensions/KFOptionSetter`+.swift:
- Around line 177-179: The image(with:data:options:) path currently always calls
KingfisherWrapper.downsampledImage which differs from process(item:)’s SVG
rendering; update image(with:data:options:) to detect SVG data (same check used
in process(item:), e.g., looksLikeSVG or checking data header) and for SVGs
render into a raster UIImage using UIGraphicsImageRenderer at downsampleSize and
options.scaleFactor (matching process(item:)), otherwise fall back to
KingfisherWrapper.downsampledImage; ensure you reuse the same downsampleSize,
options.scaleFactor, and the same rendering logic to keep disk-cache decoding
consistent with downloads.
In `@damus/Shared/Extensions/OffsetExtension.swift`:
- Around line 33-39: Add concise docstrings explaining the purpose and fields of
the newly introduced Storage class and the `@State` storage property: document
that Storage tracks the current and previous scroll/content offset used for
offset calculations (describe currentOffset and previousOffset semantics), and
annotate the `@State` private var storage to clarify it persists offset state for
the view's lifecycle and why it's kept as a reference type rather than value
type; attach these comments directly above the Storage class and above the
storage property (referencing Storage and storage) so future maintainers
understand their hot-path role.
In `@damus/Shared/Media/Video/DamusVideoControlsView.swift`:
- Around line 15-22: The new playback-state members lack documentation: add
concise doc comments for the `@State` property current_time and the computed
Binding currentTimeBinding in DamusVideoControlsView explaining their purpose
and contract (e.g., that current_time is the local UI scrubbing state mirroring
the player's time, and currentTimeBinding syncs changes to video.current_time
and updates current_time), describe threading/ownership expectations and when
they should be updated, and note that the binding must propagate user scrubs
back to video.current_time to keep state in sync.
In `@damus/Shared/Media/Video/DamusVideoPlayer.swift`:
- Around line 60-64: Add a docstring to the didSet observer for the stored
property current_time describing that manual writes must propagate to
currentTimeSubject to keep observers in sync, then update the current_time's
didSet to call currentTimeSubject.send(current_time) so manual scrubs
immediately notify subscribers; finally remove the now-redundant explicit
currentTimeSubject.send(...) at the periodic tick location (the manual send in
DamusVideoPlayer where the comment noted redundancy). Ensure you reference the
current_time property, its didSet observer, and currentTimeSubject when making
the changes.
In `@damus/Shared/Utilities/Router.swift`:
- Line 17: Add missing Swift docstrings for the modified route API surface: add
a triple-slash (///) documentation comment above the enum case
Following(contacts: NostrEvent) describing what the case represents and the
meaning of the contacts parameter, and add a triple-slash docstring above the
view(...) method explaining its purpose, parameters, return value, and any side
effects; ensure wording follows existing project doc style and includes
parameter names exactly as in Following(contacts: NostrEvent) and the view(...)
signature to maintain doc coverage.
In `@damus/Shared/Utilities/TimeAgo.swift`:
- Around line 10-16: Add a doc comment above the new shared formatter
_timeAgoFormatter describing its purpose (a shared DateComponentsFormatter for
short “time ago” strings), intended usage, the units it supports (second,
minute, hour, day, weekOfMonth, month, year), and any thread-safety or lifecycle
notes (e.g., it is a shared immutable formatter instance used across the
module). Reference the symbol name _timeAgoFormatter and the fact it wraps
DateComponentsFormatter so reviewers can see this is the shared helper/state the
docstring covers.
- Around line 10-16: The shared DateComponentsFormatter `_timeAgoFormatter` is
not documented and is unsafe for concurrent access from `time_ago_since`; add a
docstring above `_timeAgoFormatter` describing its purpose and thread-safety
strategy, then protect all accesses to the formatter (or replace it with
thread-local storage) — e.g., create a private serial DispatchQueue or use
thread-local storage per thread and ensure `time_ago_since` uses that
synchronized queue or thread-local formatter for all formatting calls; update
the docstring to note why synchronization/thread-local is required.
---
Outside diff comments:
In `@damus/Features/Profile/Views/ProfileView.swift`:
- Around line 531-535: The body currently performs synchronous lookups (try?
damus_state.profiles.lookup_lnurl and try? damus_state.profiles.lookup(id:))
which can block rendering; move these lookups out of body into asynchronous
cached state: add `@State/`@StateObject properties on ProfileView (e.g.,
lnurlState and ndbProfileState), kick off a Task or DispatchQueue.global() in
onAppear or an init async helper to call
damus_state.profiles.lookup_lnurl(profile.pubkey) and
damus_state.profiles.lookup(id: profile.pubkey) off the main thread, then assign
results back on the main actor; update the View to read from the cached
lnurlState and ndbProfileState instead of calling lookup* directly in body.
In `@damus/Shared/Extensions/KFOptionSetter`+.swift:
- Around line 137-179: Add /// docstrings to
CustomImageProcessor.process(item:options:) and
CustomCacheSerializer.image(with:options:) describing their behavior and
contract: state that process handles ImageProcessItem.data by first checking for
SVG via magic bytes and UTF-8 conversion and rendering SVG to a raster image
(then scaling by options.scaleFactor), otherwise it downsamples using
KingfisherWrapper.downsampledImage to the configured downsampleSize; also note
that .image case delegates to DefaultImageProcessor (and is not expected to
run), that SVG detection avoids full String allocation for binary data, and
document threading/return guarantees (returns a KFCrossPlatformImage? and may be
nil on failure). Keep the docs concise and mention downsampling policy and any
assumptions about options.scaleFactor and original data.
---
Duplicate comments:
In `@damus/Features/Events/EventMutingContainerView.swift`:
- Around line 54-58: The unmute handler in EventMutingContainerView is using the
wrong condition: inside the .onReceive(handle_notify(.new_unmutes)) block it
checks damus_state.mutelist_manager.event_muted_reason(event) != nil (meaning
still muted) before restoring state; change the check to == nil so you detect a
truly unmuted event and then set shown = true and muted_reason = nil; update the
condition in the onReceive closure that references event_muted_reason(event)
accordingly.
---
Nitpick comments:
In `@damus/Features/Chat/ChatroomThreadView.swift`:
- Around line 14-15: The PreferenceKey.reduce implementation currently
overwrites value with nextValue(); change it to take the minimum of the existing
value and nextValue() to match the key's semantics — update the static func
reduce(value: inout CGFloat, nextValue: () -> CGFloat) in
ChatroomThreadView.swift to set value = min(value, nextValue()) so multiple
emitted values are correctly aggregated.
In `@damus/Features/Events/EventMenu.swift`:
- Around line 39-64: Add doc comments to the UIKit bridge functions to explain
their lifecycle roles and usage: document makeUIView(context:) to describe
creating and configuring the UIButton (image, tint, showsMenuAsPrimaryAction,
and initial menu), updateUIView(_:context:) to state it refreshes the button's
menu when state changes, and buildDeferredMenu() to explain deferred menu
construction and the use of UIDeferredMenuElement to call
EventMenuButton.menuActions(damus_state:event:). Place concise Swift doc
comments above each of these functions mentioning threading/main-authority
expectations (if any) and parameter/return semantics.
- Around line 63-170: The menu action logic in EventMenu.menuActions is
duplicated in EventMenuButton.menuActions/MenuItems; extract the common
action-building code into a single shared function (e.g.,
EventMenu.sharedActions(damus_state:event:) or EventMenu.buildCommonMenuActions)
and replace the duplicated implementations in EventMenuButton.menuActions and
MenuItems to call that shared function. Move the code that constructs the
actions array (all UIAction/UIMenu creation and uses of damus_state,
profileModel, isBookmarked, isMutedThread, muteDurationMenu, etc.) into the new
shared function, keep muteDurationMenu and helper functions private to
EventMenu, and ensure callers simply return or wrap the shared result so
behavior remains identical.
In `@damus/Features/Profile/Models/ProfileModel.swift`:
- Around line 41-53: Add doc comments for the contactPubkeys property,
follows(pubkey:) method, and handle_profile_contact_event(_:) method that
describe the follow-cache lifecycle: state that contactPubkeys is an in-memory
cache of follows for this ProfileModel, initialized empty and populated/updated
only via events processed by EventHolder (mention preload_events/profile contact
events), that follows(pubkey:) reads this cache as the authoritative source for
current follow membership until a relevant contact/profile event updates it, and
that handle_profile_contact_event(_:) is responsible for applying updates
(adds/removes/refreshes) and invalidating or replacing the cache as events
arrive; mention any threading/queue context (EventHolder queue) so callers know
when the cache is safe to read.
In `@damus/Features/Profile/Views/ProfileView.swift`:
- Around line 68-70: Add docstrings for the new scroll-tracking architecture:
annotate the ScrollOffsetTracker class with a brief purpose description, the
contract for its `@Published` yOffset (units, coordinate origin, expected update
frequency), ownership/lifecycle expectations (who instantiates and observes it),
and thread/update behavior; also add similar header comments to each extracted
scroll-driven subview (describe what scroll input they consume, expected yOffset
ranges, how they react to values, and any public API/initializers). Ensure
comments are concise, follow project docstring style, and cover intent/usage for
consumers and future maintainers.
In `@damus/Features/Timeline/Views/TimelineView.swift`:
- Around line 103-106: Extract the inline scroll-queue toggle logic into a
private helper named updateScrollQueueState(currentOffset: CGFloat) with a
docstring explaining that it toggles event queueing when the timeline content
offset moves above the top boundary; inside the helper compute let shouldQueue =
-currentOffset > 0, early-return if events.should_queue == shouldQueue,
otherwise call events.set_should_queue(shouldQueue); then replace the original
inline block (the lines using current and calling events.set_should_queue) with
a call to updateScrollQueueState(currentOffset: current).
In `@damus/Shared/Extensions/KFOptionSetter`+.swift:
- Around line 112-126: The helper data_looks_like_svg currently returns true for
any first non-whitespace byte of '<', which is too broad; modify it to peek at
the subsequent bytes (safely checking bounds) and only return true when the byte
sequence matches the SVG signatures like "<svg" or "<?xml" (case-insensitive if
desired) — e.g., after skipping whitespace/BOM in data_looks_like_svg, check for
the byte patterns for "<svg" and "<?xml" (ensure you don't read past limit or
data.count) and return true only on those matches, otherwise return false so
non-SVG XML/HTML is rejected quickly without attempting full UTF-8 conversion.
In `@damus/Shared/Extensions/OffsetExtension.swift`:
- Around line 48-50: Add an early-return guard to skip no-op offset updates:
check if the incoming value equals storage.currentOffset and return immediately
if so, instead of proceeding to set storage.previousOffset/currentOffset and
call onChange; update the logic around storage.previousOffset,
storage.currentOffset and onChange to only run when the value actually differs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68dc0ef8-6d81-4034-8b7e-189a1078861b
📒 Files selected for processing (18)
damus/Features/Actions/ActionBar/Views/EventActionBar.swiftdamus/Features/Actions/Reposts/Views/RepostedEvent.swiftdamus/Features/Chat/ChatroomThreadView.swiftdamus/Features/Events/EventMenu.swiftdamus/Features/Events/EventMutingContainerView.swiftdamus/Features/Events/EventShell.swiftdamus/Features/Events/EventView.swiftdamus/Features/Profile/Models/ProfileModel.swiftdamus/Features/Profile/Views/FriendIcon.swiftdamus/Features/Profile/Views/ProfileView.swiftdamus/Features/Timeline/Views/TimelineView.swiftdamus/Shared/Extensions/KFOptionSetter+.swiftdamus/Shared/Extensions/OffsetExtension.swiftdamus/Shared/Media/Video/DamusVideoControlsView.swiftdamus/Shared/Media/Video/DamusVideoPlayer.swiftdamus/Shared/Utilities/LocalizationUtil.swiftdamus/Shared/Utilities/Router.swiftdamus/Shared/Utilities/TimeAgo.swift
🚧 Files skipped from review as they are similar to previous changes (4)
- damus/Shared/Utilities/LocalizationUtil.swift
- damus/Features/Events/EventShell.swift
- damus/Features/Actions/ActionBar/Views/EventActionBar.swift
- damus/Features/Actions/Reposts/Views/RepostedEvent.swift
| private struct UntrustedSectionMinYKey: PreferenceKey { | ||
| static var defaultValue: CGFloat = .infinity | ||
| static func reduce(value: inout CGFloat, nextValue: () -> CGFloat) { | ||
| value = nextValue() | ||
| } | ||
| } |
There was a problem hiding this comment.
Add a doc comment for the new UntrustedSectionMinYKey.
As per coding guidelines, "Ensure docstring coverage for any code added or modified".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@damus/Features/Chat/ChatroomThreadView.swift` around lines 12 - 17, Add a doc
comment for the new UntrustedSectionMinYKey explaining its purpose (tracks the
minimum Y coordinate of the "untrusted" section in the ChatroomThreadView), what
the defaultValue (.infinity) means, and how the reduce(_:, nextValue:)
implementation works (it assigns the latest reported value). Put the doc comment
immediately above the UntrustedSectionMinYKey declaration and mention the key is
a PreferenceKey used by SwiftUI layout preference propagation so future readers
understand why it exists and how it is consumed.
| init(damus_state: DamusState, event: NostrEvent, @ViewBuilder content: () -> Content) where MuteBox == EmptyView { | ||
| self.damus_state = damus_state | ||
| self.event = event | ||
| self.content = content() | ||
| self.customMuteBox = nil | ||
| self._shown = State(initialValue: should_show_event(state: damus_state, ev: event)) | ||
| } | ||
|
|
||
| init(damus_state: DamusState, event: NostrEvent, muteBox: @escaping MuteBoxViewClosure, @ViewBuilder content: () -> Content) { | ||
| self.init(damus_state: damus_state, event: event, content: content) | ||
|
|
||
| init(damus_state: DamusState, event: NostrEvent, muteBox: @escaping (Binding<Bool>, MuteItem?) -> MuteBox, @ViewBuilder content: () -> Content) { | ||
| self.damus_state = damus_state | ||
| self.event = event | ||
| self.content = content() | ||
| self.customMuteBox = muteBox | ||
| self._shown = State(initialValue: should_show_event(state: damus_state, ev: event)) | ||
| } |
There was a problem hiding this comment.
Add doc comments for newly added initializers and innerBody.
These newly added/modified declarations are undocumented.
📝 Suggested docs
+ /// Creates a muting container that uses the default `EventMutedBoxView`.
init(damus_state: DamusState, event: NostrEvent, `@ViewBuilder` content: () -> Content) where MuteBox == EmptyView {
...
}
+ /// Creates a muting container with a caller-provided mute box view.
init(damus_state: DamusState, event: NostrEvent, muteBox: `@escaping` (Binding<Bool>, MuteItem?) -> MuteBox, `@ViewBuilder` content: () -> Content) {
...
}
+ /// Renders mute UI and content visibility based on current mute/show state.
`@ViewBuilder`
private var innerBody: some View {
...
}As per coding guidelines, "Ensure docstring coverage for any code added or modified".
Also applies to: 62-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@damus/Features/Events/EventMutingContainerView.swift` around lines 25 - 39,
Add doc comments for the two initializers and the innerBody property/func:
document the purpose of each initializer (one default when MuteBox == EmptyView
and one custom with muteBox), describe parameters (damus_state: DamusState,
event: NostrEvent, content: `@ViewBuilder` content, and muteBox: (Binding<Bool>,
MuteItem?) -> MuteBox), explain the behavior of shown state initialization (uses
should_show_event(state:ev:)), and note any semantics of customMuteBox and how
innerBody composes the view and uses the binding/Muted state; ensure comments
are concise, include parameter descriptions and any important preconditions/side
effects for innerBody, init(damus_state:event: `@ViewBuilder` content:) and
init(damus_state:event: muteBox: `@ViewBuilder` content:).
| @ViewBuilder | ||
| var body: some View { |
There was a problem hiding this comment.
Document the modified body render contract.
The modified body declaration is missing a doc comment.
As per coding guidelines, "Ensure docstring coverage for any code added or modified".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@damus/Features/Events/EventView.swift` around lines 34 - 35, Add a doc
comment above the `@ViewBuilder` var body: some View property in EventView
describing the render contract: what the view renders, when it updates, and any
expectations for inputs or side effects (e.g., which state/props drive rendering
and whether it can return different subviews based on conditions). Update the
comment to reference EventView's role so readers know what the body is
responsible for and any invariants callers can rely on.
| private let _fofIcon: UIImage = { | ||
| let size = CGSize(width: 21, height: 14) | ||
| let format = UIGraphicsImageRendererFormat() | ||
| format.scale = UIScreen.main.scale | ||
| let renderer = UIGraphicsImageRenderer(size: size, format: format) | ||
| return renderer.image { _ in | ||
| let rect = CGRect(origin: .zero, size: size) | ||
| let config = UIImage.SymbolConfiguration(pointSize: 14, weight: .regular) | ||
| guard let symbol = UIImage(systemName: "person.fill.and.arrow.left.and.arrow.right", withConfiguration: config) else { return } | ||
| symbol.withTintColor(.gray, renderingMode: .alwaysOriginal).draw(in: rect) | ||
| } | ||
| }() |
There was a problem hiding this comment.
Add doc comments for _fofIcon to match the repo docstring rule.
_fofIcon is newly added/modified code but lacks coverage equivalent to _friendIcon.
Proposed fix
+/// Pre-rendered friend-of-friend icon to avoid per-frame symbol drawing.
+/// Rendered once into a UIImage and reused during cell rendering.
private let _fofIcon: UIImage = {As per coding guidelines, "Ensure docstring coverage for any code added or modified".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@damus/Features/Profile/Views/FriendIcon.swift` around lines 45 - 56, The new
private UIImage constant `_fofIcon` lacks the required doc comment; add a
docstring above `_fofIcon` matching the style and content level used for
`_friendIcon` describing what the icon represents (friend-of-friend indicator),
where/how it is used, and any relevant rendering or sizing notes (e.g.,
pointSize/weight and tint behavior), so the declaration for `_fofIcon` has the
same docstring coverage and conventions as `_friendIcon`.
| private func showFollowBtn() -> Bool { | ||
| damus_state.contacts.follow_state(profile.pubkey) == .unfollows && blurOpacity() > 1.0 | ||
| } |
There was a problem hiding this comment.
Prevent follow CTA from appearing on your own profile.
showFollowBtn() should exclude the signed-in user; otherwise the collapsed toolbar can show a follow button for self.
💡 Suggested fix
private func showFollowBtn() -> Bool {
- damus_state.contacts.follow_state(profile.pubkey) == .unfollows && blurOpacity() > 1.0
+ profile.pubkey != damus_state.pubkey &&
+ damus_state.contacts.follow_state(profile.pubkey) == .unfollows &&
+ blurOpacity() > 1.0
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@damus/Features/Profile/Views/ProfileView.swift` around lines 657 - 659,
showFollowBtn() currently returns true for the signed-in user's profile because
it only checks follow_state and blurOpacity; update the predicate to also
exclude the current account by confirming profile.pubkey !=
damus_state.account?.pubkey (or the equivalent stored signed-in pubkey) so the
method becomes: return false when profile.pubkey matches the signed-in pubkey,
otherwise evaluate damus_state.contacts.follow_state(profile.pubkey) ==
.unfollows && blurOpacity() > 1.0; keep the same symbols: showFollowBtn(),
damus_state.contacts.follow_state(profile.pubkey), profile.pubkey,
damus_state.account?.pubkey, and blurOpacity().
| private class Storage { | ||
| var currentOffset: CGFloat = 0 | ||
| var previousOffset: CGFloat = 0 | ||
| } | ||
|
|
||
| @State private var storage = Storage() | ||
|
|
There was a problem hiding this comment.
Add docstrings for newly introduced storage state.
Storage and its role in offset tracking were added without documentation. Please add brief doc comments to keep this hot-path behavior maintainable and explicit.
As per coding guidelines, "Ensure docstring coverage for any code added or modified".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@damus/Shared/Extensions/OffsetExtension.swift` around lines 33 - 39, Add
concise docstrings explaining the purpose and fields of the newly introduced
Storage class and the `@State` storage property: document that Storage tracks the
current and previous scroll/content offset used for offset calculations
(describe currentOffset and previousOffset semantics), and annotate the `@State`
private var storage to clarify it persists offset state for the view's lifecycle
and why it's kept as a reference type rather than value type; attach these
comments directly above the Storage class and above the storage property
(referencing Storage and storage) so future maintainers understand their
hot-path role.
| @State private var current_time: TimeInterval = .zero | ||
|
|
||
| private var currentTimeBinding: Binding<TimeInterval> { | ||
| Binding( | ||
| get: { current_time }, | ||
| set: { video.current_time = $0; current_time = $0 } | ||
| ) | ||
| } |
There was a problem hiding this comment.
Add doc comments for newly introduced playback-state members.
Line 15 and Line 17 introduce new state/binding members without doc comments, which makes the scrubbing/state-sync contract less clear for future edits.
📝 Suggested patch
- `@State` private var current_time: TimeInterval = .zero
+ /// Local UI copy of playback time used to update controls without forcing full `DamusVideoPlayer` invalidation.
+ `@State` private var current_time: TimeInterval = .zero
+ /// Binding used by the scrubber to keep local state and `DamusVideoPlayer.current_time` in sync.
private var currentTimeBinding: Binding<TimeInterval> {
Binding(
get: { current_time },
set: { video.current_time = $0; current_time = $0 }
)
}As per coding guidelines, "Ensure docstring coverage for any code added or modified".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@damus/Shared/Media/Video/DamusVideoControlsView.swift` around lines 15 - 22,
The new playback-state members lack documentation: add concise doc comments for
the `@State` property current_time and the computed Binding currentTimeBinding in
DamusVideoControlsView explaining their purpose and contract (e.g., that
current_time is the local UI scrubbing state mirroring the player's time, and
currentTimeBinding syncs changes to video.current_time and updates
current_time), describe threading/ownership expectations and when they should be
updated, and note that the binding must propagate user scrubs back to
video.current_time to keep state in sync.
| var current_time: TimeInterval = .zero | ||
|
|
||
| /// Combine subject for views that need to observe current_time changes | ||
| /// without triggering objectWillChange on every tick. | ||
| let currentTimeSubject = CurrentValueSubject<TimeInterval, Never>(.zero) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Assignments to current_time:"
rg -nP --type=swift '\bcurrent_time\s*=' \
damus/Shared/Media/Video/DamusVideoPlayer.swift \
damus/Shared/Media/Video/DamusVideoControlsView.swift
echo
echo "Publish sites for currentTimeSubject:"
rg -nP --type=swift 'currentTimeSubject\.send\s*\(' \
damus/Shared/Media/Video/DamusVideoPlayer.swift \
damus/Shared/Media/Video/DamusVideoControlsView.swiftRepository: damus-io/damus
Length of output: 590
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== DamusVideoPlayer.swift around lines 60-64 (property definition) ==="
sed -n '55,70p' damus/Shared/Media/Video/DamusVideoPlayer.swift
echo
echo "=== DamusVideoPlayer.swift around lines 209-216 (periodic observer) ==="
sed -n '205,220p' damus/Shared/Media/Video/DamusVideoPlayer.swift
echo
echo "=== DamusVideoControlsView.swift around line 20 (scrubbing) ==="
sed -n '15,30p' damus/Shared/Media/Video/DamusVideoControlsView.swiftRepository: damus-io/damus
Length of output: 2230
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== All current_time assignments in entire codebase ==="
rg -n '\bcurrent_time\s*=' --type=swift damus/ | grep -v test | head -20Repository: damus-io/damus
Length of output: 463
Add docstring to didSet observer for current_time and sync currentTimeSubject on manual writes.
The synchronization issue is confirmed. Direct writes to current_time at DamusVideoControlsView.swift:20 (scrubbing) do not notify currentTimeSubject until the next periodic tick at DamusVideoPlayer.swift:215. The proposed didSet solution is functionally correct, but it must include a docstring explaining the synchronization behavior to meet coding guidelines. Update the patch:
var current_time: TimeInterval = .zero {
didSet {
+ /// Synchronize currentTimeSubject whenever current_time is manually updated
+ /// (e.g., during scrubbing). The guard prevents redundant sends on no-op assignments.
guard oldValue != current_time else { return }
currentTimeSubject.send(current_time)
}
}Also remove the now-redundant manual send at line 215.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@damus/Shared/Media/Video/DamusVideoPlayer.swift` around lines 60 - 64, Add a
docstring to the didSet observer for the stored property current_time describing
that manual writes must propagate to currentTimeSubject to keep observers in
sync, then update the current_time's didSet to call
currentTimeSubject.send(current_time) so manual scrubs immediately notify
subscribers; finally remove the now-redundant explicit
currentTimeSubject.send(...) at the periodic tick location (the manual send in
DamusVideoPlayer where the comment noted redundancy). Ensure you reference the
current_time property, its didSet observer, and currentTimeSubject when making
the changes.
| case Relay(relay: RelayURL, showActionButtons: Binding<Bool>) | ||
| case RelayDetail(relay: RelayURL, metadata: RelayMetadata?) | ||
| case Following(following: FollowingModel) | ||
| case Following(contacts: NostrEvent) |
There was a problem hiding this comment.
Add docstrings for modified route API surface.
The modified Following route case (Line 17) and view(...) method (Line 63) are missing /// docstrings.
Suggested update
enum Route: Hashable {
+ /// Destination for viewing followed contacts/hashtags extracted from a contacts event.
case Following(contacts: NostrEvent)
@@
+ /// Builds the destination view for this route.
+ /// - Parameters:
+ /// - navigationCoordinator: Coordinator used by auth/onboarding destinations.
+ /// - damusState: Shared app state for destination construction.
+ /// - Returns: A type-erased destination view for NavigationStack.
func view(navigationCoordinator: NavigationCoordinator, damusState: DamusState) -> AnyView {As per coding guidelines, **/*.swift: "Ensure docstring coverage for any code added or modified".
Also applies to: 63-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@damus/Shared/Utilities/Router.swift` at line 17, Add missing Swift docstrings
for the modified route API surface: add a triple-slash (///) documentation
comment above the enum case Following(contacts: NostrEvent) describing what the
case represents and the meaning of the contacts parameter, and add a
triple-slash docstring above the view(...) method explaining its purpose,
parameters, return value, and any side effects; ensure wording follows existing
project doc style and includes parameter names exactly as in Following(contacts:
NostrEvent) and the view(...) signature to maintain doc coverage.
| private let _timeAgoFormatter: DateComponentsFormatter = { | ||
| let formatter = DateComponentsFormatter() | ||
| formatter.unitsStyle = .abbreviated | ||
| formatter.maximumUnitCount = 1 | ||
| formatter.allowedUnits = [.second, .minute, .hour, .day, .weekOfMonth, .month, .year] | ||
| return formatter | ||
| }() |
There was a problem hiding this comment.
Add doc comments for the new shared formatter helper/state.
Newly added/modified symbols in this file should have docstring coverage per repo rules.
As per coding guidelines, "Ensure docstring coverage for any code added or modified."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@damus/Shared/Utilities/TimeAgo.swift` around lines 10 - 16, Add a doc comment
above the new shared formatter _timeAgoFormatter describing its purpose (a
shared DateComponentsFormatter for short “time ago” strings), intended usage,
the units it supports (second, minute, hour, day, weekOfMonth, month, year), and
any thread-safety or lifecycle notes (e.g., it is a shared immutable formatter
instance used across the module). Reference the symbol name _timeAgoFormatter
and the fact it wraps DateComponentsFormatter so reviewers can see this is the
shared helper/state the docstring covers.
🧩 Analysis chain
🏁 Script executed:
cat -n damus/Shared/Utilities/TimeAgo.swiftRepository: damus-io/damus
Length of output: 2595
🌐 Web query:
DateComponentsFormatter thread-safe Apple documentation Foundation
💡 Result:
Apple doesn’t appear to explicitly state “DateComponentsFormatter is thread-safe” (or not) in a dedicated spot that’s easily accessible in the documentation archive. What Apple does document is broader guidance that applies to formatter-style objects:
- In Apple’s Threading Programming Guide → Thread Safety Summary (Documentation Archive),
NSDateFormatterandNSFormatterare listed among the “generally considered to be thread-safe” Foundation classes, and Apple reiterates the general rule that mutable objects are generally not thread-safe and must be synchronized if mutated concurrently. [1] - In Apple’s WWDC 2010 performance slides, Apple explicitly warns: “Date and number formatters are not thread-safe.” [2]
Practical takeaway for DateComponentsFormatter (Foundation): don’t share and mutate a single DateComponentsFormatter instance concurrently across threads. If you need cross-thread use, either (a) create separate instances per thread/queue, or (b) serialize access (e.g., a lock or a dedicated queue), especially if you change its properties (allowedUnits, unitsStyle, etc.). [1][2]
Sources:
[1] Apple Documentation Archive — Thread Safety Summary (Threading Programming Guide)
[2] Apple WWDC 2010 Session 147 slides — Advanced Performance Optimization on iPhone OS, Part 2 (warning that date/number formatters are not thread-safe)
🏁 Script executed:
rg "time_ago_since" --type swift -B 2 -A 2 | head -100Repository: damus-io/damus
Length of output: 3303
🏁 Script executed:
# Check the actual usage context - look for dispatch, async/await patterns around calls
rg "time_ago_since" --type swift -B 5 -A 2 | head -150Repository: damus-io/damus
Length of output: 3853
Add documentation and synchronize access to the shared formatter.
DateComponentsFormatter is not thread-safe for concurrent access (confirmed by Apple WWDC 2010 guidance). The shared mutable instance _timeAgoFormatter lacks synchronization and documentation, creating a data race risk when time_ago_since is called from multiple threads (as can happen with SwiftUI views rendering from different contexts). Additionally, the variable lacks a docstring required by coding guidelines.
Recommended fix: either (1) use thread-local storage per the suggested approach, or (2) synchronize access with a lock or dedicated DispatchQueue. Also add a docstring explaining the formatter's purpose and thread-safety handling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@damus/Shared/Utilities/TimeAgo.swift` around lines 10 - 16, The shared
DateComponentsFormatter `_timeAgoFormatter` is not documented and is unsafe for
concurrent access from `time_ago_since`; add a docstring above
`_timeAgoFormatter` describing its purpose and thread-safety strategy, then
protect all accesses to the formatter (or replace it with thread-local storage)
— e.g., create a private serial DispatchQueue or use thread-local storage per
thread and ensure `time_ago_since` uses that synchronized queue or thread-local
formatter for all formatting calls; update the docstring to note why
synchronization/thread-local is required.
Have you had a chance to fix these? Please let me know how to proceed with this PR. |
2 participants
Summary
Comprehensive SwiftUI performance overhaul targeting main-thread hitches during scrolling, profile viewing, and video playback. 20 commits across 19 files, each addressing a specific bottleneck identified via Instruments profiling.
View tree & layout optimizations
@ViewBuilderor generics, restoring SwiftUI's structural identity diffing instead of full subtree teardown/rebuild@ViewBuilderswitch created 44 levels of nested_ConditionalContent; replaced with explicitAnyViewreturns (appropriate here since navigation destinations are one-shot, not per-frame)Replace paging TabView— PostingTimelineView's.pagestyle TabView was pre-rendering both tabs (~129ms for the offscreen one); replaced with a simple conditional (REMOVED)Caching & allocation reduction
bundleForLocalewas hitting disk (Bundle.main.path + Bundle(path:)) 12+ times per ProfileView render; now cached by locale identifierfollows()was O(n) linear scan of contact event tags; now O(1) Set lookup built once on contact event arrivalObservableObject / state invalidation fixes
@PublishedwithCurrentValueSubjectscoped to controls only@State var yOffsetwith a reference-typeScrollOffsetTrackerso scroll mutations don't trigger parent body re-evaluation (~112ms saved per frame)@Statevalue types were causing body re-evaluation on every scroll frameUIKit drop-ins
UIDeferredMenuElement.uncacheddefers to tap timeCA::Layer::prepare_commit; now all images decode at target resolution viaCGImageSourceCreateThumbnailAtIndexsetPlayerrebuilds internal video layers (~13ms) even when assigned the same reference; added identity guardTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Performance Improvements
Bug Fixes