feat: add offline mode banner (#623)#626
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR adds a persistent offline-mode banner at the top of all screens that appears when Reticulum is in Key changes:
Issues found:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User
participant VM as SettingsViewModel
participant SRP as ServiceReticulumProtocol
participant SC as ServiceConnection
participant UI as OfflineModeBanner
U->>VM: restartService()
VM->>VM: isRestarting = true
VM-->>UI: banner shows "Reconnecting…" + spinner
VM->>SRP: applyInterfaceChanges()
SRP->>SC: unbindService()
SC->>SRP: _networkStatus = SHUTDOWN
SRP-->>VM: networkStatus flow → SHUTDOWN
VM-->>UI: networkStatus = SHUTDOWN (banner still visible via isRestarting)
SRP->>SC: bindService()
SC->>SRP: onServiceConnected → CONNECTING → INITIALIZING → READY
SRP-->>VM: networkStatus flow → READY
VM-->>UI: networkStatus = READY
VM->>VM: isRestarting = false
VM-->>UI: shouldShowOfflineBanner(READY)=false, isRestarting=false → banner hidden
Note over U,UI: Settings → Shutdown path (bug fix)
U->>VM: shutdownService()
VM->>SRP: unbindService()
SRP->>SRP: _networkStatus = SHUTDOWN (NEW — was missing before)
SRP-->>VM: networkStatus flow → SHUTDOWN
VM-->>UI: banner appears with "Reconnect" button
|
| @Composable | ||
| fun OfflineModeBanner( | ||
| networkStatus: NetworkStatus, |
There was a problem hiding this comment.
Bug: A race condition causes the "Reconnecting..." banner to disappear prematurely because the isRestarting flag is cleared before the service's networkStatus is updated to READY.
Severity: MEDIUM
Suggested Fix
Decouple the UI state from the completion of the applyInterfaceChanges() call. The isRestarting flag should only be set to false after the service has confirmed it has reached the READY state, not just when the restart process has been initiated. This ensures the banner remains visible for the entire duration of the reconnection process.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
app/src/main/java/com/lxmf/messenger/ui/components/OfflineModeBanner.kt#L40-L42
Potential issue: A race condition exists when restarting the service. The `isRestarting`
state is set to `false` as soon as the `applyInterfaceChanges()` function returns, which
occurs before the service has fully re-initialized and the `networkStatus` becomes
`READY`. The offline banner's visibility is determined by
`shouldShowOfflineBanner(networkStatus) || isRestarting`. Because `isRestarting` becomes
false prematurely, there is a window of time where both conditions are false, causing
the "Reconnecting..." banner to disappear before the connection is actually
re-established, potentially confusing the user.
Did we get this right? 👍 / 👎 to inform future reviews.
…623) Show a thin banner above the TopAppBar across all screens when the service is in SHUTDOWN or ERROR state. The banner displays "Columba is offline" with a Reconnect button, or "Reconnecting..." with a spinner while the service restarts. Status bar insets are consumed at the Column level so inner TopAppBars don't double-pad when the banner is visible. Also fix ServiceReticulumProtocol.unbindService() to emit SHUTDOWN status — previously only onServiceDisconnected (process crash) set it, so explicit shutdown via Settings never updated the networkStatus flow. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move hardcoded UI strings to res/values/strings.xml for localization readiness. Add semantics contentDescription to the reconnecting spinner so TalkBack users know it is a loading indicator. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
26a1599 to
6512789
Compare
…tion Moves statusBarsPadding() from the parent Column (toggled instantly via boolean) into OfflineModeBanner's Row so it animates together with AnimatedVisibility, preventing a ~24dp layout jump during banner exit. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| Modifier | ||
| .fillMaxWidth() | ||
| .statusBarsPadding() |
There was a problem hiding this comment.
Status bar insets not consumed for sibling NavHost
Modifier.statusBarsPadding() is applied to the Row inside the banner. In Compose, inset consumption propagates down to children of the modifier's owner — not laterally to sibling composables. The NavHost is a sibling in the outer Column, so it never sees those consumed insets.
As a result, every inner screen's TopAppBar (which uses TopAppBarDefaults.windowInsets = WindowInsets.statusBars by default) still applies the full status-bar height as its own top padding. When the banner is visible and the NavHost starts below the banner, the TopAppBar is rendered already below the status bar but still adds status-bar-height padding, making it appear taller than intended.
The PR description says "Banner consumes status bar insets so inner TopAppBars don't double-pad", but the current implementation doesn't achieve this because statusBarsPadding() on a sibling's child does not propagate consumption outward.
A targeted fix is to consume the status-bar insets on the NavHost itself when the banner is visible, so that inner Scaffolds receive zero status-bar insets:
val bannerVisible = shouldShowOfflineBanner(networkStatus) || isRestarting
NavHost(
modifier = Modifier
.weight(1f)
.then(
if (bannerVisible)
Modifier.consumeWindowInsets(WindowInsets.statusBars)
else
Modifier
),
...
)This requires importing androidx.compose.foundation.layout.consumeWindowInsets (already used in several screen files, e.g. SettingsScreen.kt).
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/main/java/com/lxmf/messenger/ui/components/OfflineModeBanner.kt
Line: 58-60
Comment:
**Status bar insets not consumed for sibling NavHost**
`Modifier.statusBarsPadding()` is applied to the `Row` inside the banner. In Compose, inset consumption propagates **down** to children of the modifier's owner — not laterally to sibling composables. The `NavHost` is a sibling in the outer `Column`, so it never sees those consumed insets.
As a result, every inner screen's `TopAppBar` (which uses `TopAppBarDefaults.windowInsets` = `WindowInsets.statusBars` by default) still applies the full status-bar height as its own top padding. When the banner is visible and the `NavHost` starts below the banner, the `TopAppBar` is rendered already below the status bar but still adds status-bar-height padding, making it appear taller than intended.
The PR description says "Banner consumes status bar insets so inner TopAppBars don't double-pad", but the current implementation doesn't achieve this because `statusBarsPadding()` on a sibling's child does not propagate consumption outward.
A targeted fix is to consume the status-bar insets on the `NavHost` itself when the banner is visible, so that inner Scaffolds receive zero status-bar insets:
```kotlin
val bannerVisible = shouldShowOfflineBanner(networkStatus) || isRestarting
NavHost(
modifier = Modifier
.weight(1f)
.then(
if (bannerVisible)
Modifier.consumeWindowInsets(WindowInsets.statusBars)
else
Modifier
),
...
)
```
This requires importing `androidx.compose.foundation.layout.consumeWindowInsets` (already used in several screen files, e.g. `SettingsScreen.kt`).
How can I resolve this? If you propose a fix, please make it concise.
Summary
ServiceReticulumProtocol.unbindService()to emitNetworkStatus.SHUTDOWN— previously only process crashes triggered the status update, so explicit shutdown from Settings never surfaced to the UITest plan
shouldShowOfflineBanner()covering allNetworkStatusvariantscompileNoSentryDebugKotlin)Closes #623
🤖 Generated with Claude Code