Restore signal indicator#3773
Merged
danieldaquino merged 3 commits intoMay 18, 2026
Merged
Conversation
The SignalView was never appearing because update_signal_from_pool was defined but never called, leaving signal data at 0/0. Fix by having RelayPool maintain its own SignalModel, updating it on connection events, relay add/remove. Wire views directly to the pool signal via NostrNetworkManager instead of the disconnected HomeModel.signal. Replace the text-based indicator with signal strength bars matching the Android app: 4 progressive-height bars colored red->yellow->green based on the connected/total relay ratio. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR relocates signal tracking into RelayPool (MainActor), exposes it via NostrNetworkManager.signal, rewires UI to use nostrNetwork.signal, replaces the text indicator with a 4-bar SignalView, removes HomeModel signal code, and disables two test debug prints. ChangesSignal Indicator Restoration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
damus/Features/Relays/Views/SignalView.swift (1)
51-68: 💤 Low valueConsider adding a safeguard for the
bar_heightsarray access.Line 59 accesses
Self.bar_heights[i]whereiranges from0..<Self.num_bars. Currently this is safe because both are 4, but if someone changesnum_barswithout updating thebar_heightsarray length, a crash will occur.🛡️ Proposed safeguard
Add a static assertion or precondition:
struct SignalView: View { let state: DamusState `@ObservedObject` var signal: SignalModel static let num_bars = 4 static let bar_heights: [CGFloat] = [4, 7, 10, 13] static let bar_width: CGFloat = 3 static let bar_spacing: CGFloat = 2 + + // Ensure bar_heights array matches num_bars + private static let _ = { + precondition(bar_heights.count == num_bars, "bar_heights.count must equal num_bars") + }()Alternatively, add a comment documenting the dependency:
static let num_bars = 4 + // IMPORTANT: bar_heights must have exactly num_bars elements static let bar_heights: [CGFloat] = [4, 7, 10, 13]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@damus/Features/Relays/Views/SignalView.swift` around lines 51 - 68, The ForEach in SignalView's body indexes Self.bar_heights[i] using 0..<Self.num_bars which will crash if num_bars and bar_heights length diverge; fix by either adding a runtime safeguard (precondition(Self.bar_heights.count >= Self.num_bars) or assertion in the type) or by changing the iteration to use the actual bar_heights indices (e.g., iterate over 0..<min(Self.num_bars, Self.bar_heights.count) or bar_heights.indices.prefix(Self.num_bars)) so Self.bar_heights[i] is always valid; update the ForEach and/or add a static comment/assert near the definitions of num_bars and bar_heights to document the invariant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@damus/Core/Networking/NostrNetworkManager/NostrNetworkManager.swift`:
- Around line 250-253: Add a concise docstring above the `@MainActor` computed
property `signal` describing its purpose (exposing the network pool's signaling
model), its thread-affinity (main actor), and what it returns (the `SignalModel`
from `pool.signal`), e.g. one or two sentences that mention it forwards to
`pool.signal` for consumers to observe network signals; place the docstring
immediately above `var signal` in `NostrNetworkManager`.
In `@damus/Core/Nostr/RelayPool.swift`:
- Around line 129-139: The method update_signal lacks a docstring; add a concise
Swift doc comment above the `@MainActor` func update_signal() that explains its
purpose (synchronizes the signal fields with current connection state),
describes behavior (reads num_connected and relays.count, updates signal.signal
and signal.max_signal only when values change), mentions thread context
(`@MainActor`) and any side effects on signal, and notes expected invariants
(e.g., non-negative counts); reference the symbols update_signal, signal,
num_connected, and relays in the comment for clarity.
---
Nitpick comments:
In `@damus/Features/Relays/Views/SignalView.swift`:
- Around line 51-68: The ForEach in SignalView's body indexes
Self.bar_heights[i] using 0..<Self.num_bars which will crash if num_bars and
bar_heights length diverge; fix by either adding a runtime safeguard
(precondition(Self.bar_heights.count >= Self.num_bars) or assertion in the type)
or by changing the iteration to use the actual bar_heights indices (e.g.,
iterate over 0..<min(Self.num_bars, Self.bar_heights.count) or
bar_heights.indices.prefix(Self.num_bars)) so Self.bar_heights[i] is always
valid; update the ForEach and/or add a static comment/assert near the
definitions of num_bars and bar_heights to document the invariant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee4c8b4f-fe20-48ea-9c48-b8b1d40c96ec
📒 Files selected for processing (6)
damus/ContentView.swiftdamus/Core/Networking/NostrNetworkManager/NostrNetworkManager.swiftdamus/Core/Nostr/RelayPool.swiftdamus/Features/Relays/Views/SignalView.swiftdamus/Features/Timeline/Models/HomeModel.swiftdamus/Features/Timeline/Views/PostingTimelineView.swift
💤 Files with no reviewable changes (1)
- damus/Features/Timeline/Models/HomeModel.swift
Comment on lines
+250
to
+253
| @MainActor | ||
| var signal: SignalModel { | ||
| self.pool.signal | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add docstring to document the property's purpose.
This new computed property lacks documentation. According to coding guidelines, all added or modified code must have docstring coverage.
📝 Proposed docstring
+ /// The current network signal model reflecting relay connectivity status.
+ ///
+ /// This property forwards the relay pool's signal state, providing access
+ /// to the number of connected relays (`signal`) and total relays (`max_signal`)
+ /// for UI components and observers.
`@MainActor`
var signal: SignalModel {As per coding guidelines: "Ensure docstring coverage for any code added or modified"
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @MainActor | |
| var signal: SignalModel { | |
| self.pool.signal | |
| } | |
| /// The current network signal model reflecting relay connectivity status. | |
| /// | |
| /// This property forwards the relay pool's signal state, providing access | |
| /// to the number of connected relays (`signal`) and total relays (`max_signal`) | |
| /// for UI components and observers. | |
| `@MainActor` | |
| var signal: SignalModel { | |
| self.pool.signal | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@damus/Core/Networking/NostrNetworkManager/NostrNetworkManager.swift` around
lines 250 - 253, Add a concise docstring above the `@MainActor` computed property
`signal` describing its purpose (exposing the network pool's signaling model),
its thread-affinity (main actor), and what it returns (the `SignalModel` from
`pool.signal`), e.g. one or two sentences that mention it forwards to
`pool.signal` for consumers to observe network signals; place the docstring
immediately above `var signal` in `NostrNetworkManager`.
Comment on lines
+129
to
+139
| @MainActor | ||
| func update_signal() { | ||
| let connected = num_connected | ||
| let total = relays.count | ||
| if signal.signal != connected { | ||
| signal.signal = connected | ||
| } | ||
| if signal.max_signal != total { | ||
| signal.max_signal = total | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add docstring to document the method's purpose and behavior.
This new method lacks documentation. According to coding guidelines, all added or modified code must have docstring coverage.
📝 Proposed docstring
+ /// Updates the signal model with current relay connection statistics.
+ ///
+ /// Computes the number of connected relays and total relay count, then updates
+ /// `signal.signal` and `signal.max_signal` if the values have changed.
+ /// This reduces unnecessary publishes to observers when values are unchanged.
`@MainActor`
func update_signal() {As per coding guidelines: "Ensure docstring coverage for any code added or modified"
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@damus/Core/Nostr/RelayPool.swift` around lines 129 - 139, The method
update_signal lacks a docstring; add a concise Swift doc comment above the
`@MainActor` func update_signal() that explains its purpose (synchronizes the
signal fields with current connection state), describes behavior (reads
num_connected and relays.count, updates signal.signal and signal.max_signal only
when values change), mentions thread context (`@MainActor`) and any side effects
on signal, and notes expected invariants (e.g., non-negative counts); reference
the symbols update_signal, signal, num_connected, and relays in the comment for
clarity.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Collaborator
Author
|
I made one more change: hide the signal indicator if we're fully connected. this was the original behavior |
Collaborator
|
Added changelog items on the merge commit. |
Collaborator
Author
|
I added #3776 to enforce this as a check like we do on the notedeck side |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix connectivity indicator that was never appearing because
update_signal_from_poolwas defined but never called, leaving signal data at 0/0Have
RelayPoolmaintain its ownSignalModel, updating it on connection events, relay add/removeWire views directly to pool signal via
NostrNetworkManagerinstead of the disconnectedHomeModel.signalReplace text-based indicator with signal strength bars matching the Android app: 4 progressive-height bars colored red→yellow→green based on connected/total relay ratio
Closes Restore signal indicator #3772
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests