feat: NomadNet browser with Micron rendering, URL bar, and URI handler#671
Merged
torlando-tech merged 39 commits intomainfrom Mar 13, 2026
Merged
feat: NomadNet browser with Micron rendering, URL bar, and URI handler#671torlando-tech merged 39 commits intomainfrom
torlando-tech merged 39 commits intomainfrom
Conversation
Implement a native NomadNet page browser allowing users to connect to nomadnetwork.node destinations and browse their Micron-formatted pages directly from Android. Phase 1 - Micron parser (:micron module): - Pure Kotlin parser ported from NomadNet's MicronParser.py - Handles headings, dividers, colors, formatting, links, form fields - 69 unit tests covering all markup features Phase 2 - Python page download: - request_nomadnet_page() in reticulum_wrapper.py - Separate link cache for nomadnetwork.node (distinct from lxmf.delivery) - Identity discovery via path request for stale announces - Cached active links reused without re-resolving identity Phase 3 - AIDL/Service plumbing: - requestNomadnetPage + cancelNomadnetPageRequest in AIDL - Binder and ServiceReticulumProtocol implementations Phase 4 - Browser UI: - NomadNetBrowserViewModel with history stack, form state, rendering modes - NomadNetBrowserScreen with loading/error/loaded states, pull-to-refresh - MicronComposables for native Compose rendering of Micron markup - Three rendering modes: monospace+scroll, monospace+zoom, proportional+wrap - "Browse Node" button on nomadnetwork.node announce detail screens - Navigation route wired in MainActivity Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix NomadNet URL parsing: handle colon-separated format (`:path` for same-node, `hash:path` for cross-node) which is the standard format used in Micron page links - Add Python-side path sanitization as defense in depth - Fix identity_hash in announce events to use actual identity hash instead of incorrectly copying destination_hash - Switch to absolute deadline-based timeouts for link establishment - Add comprehensive diagnostic logging: destination hash verification, hop count, RNS establishment timeout, teardown reason - Fix misleading "Connection rejected" error — now correctly reports timeout vs actual node closure based on RNS teardown_reason - Always request fresh path before link establishment to avoid stale routing info Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cache pages in cacheDir/nomadnet_pages/ with TTL from #!c=N directive (default 12h, 0=never cache). Enables instant back-navigation via in-memory document history and cache-first loading for revisited pages. Form submissions always bypass cache. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use a regular Column with verticalScroll + horizontalScroll for MONOSPACE_SCROLL mode instead of LazyColumn with per-line items, so swiping horizontally moves all lines as one unit. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… mode Two-finger pinch gestures scale the page content (0.5x–3x) using a layout modifier that adjusts both the visual rendering and the reported dimensions, so zooming out shows more content and zooming in expands the scrollable area. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
BoxWithConstraints captures viewport width before horizontalScroll unbounds constraints. Centered/right-aligned lines use exact viewport width (wrapping long text like NomadNet terminal), while left-aligned lines use min-width so they can extend for horizontal scrolling. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Backtick-prefixed links (`[label`dest]) and fields (`<|name`val>) had the backtick output as literal text instead of being consumed as a formatting mode entry. This caused stray backtick characters in rendered NomadNet pages. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Allow users to opt-in identify themselves to a NomadNet node by sending their public key via link.identify(). This enables nodes to serve personalized content or grant access-controlled pages. Full-stack implementation through all 7 layers: - Python: rns_api.py identify_nomadnet_link() (Strangler Fig pattern) - Kotlin: PythonWrapperManager, AIDL, Binder, ServiceReticulumProtocol - ViewModel: state tracking with auto-reset on node change - UI: fingerprint icon in top bar + overflow menu with snackbar errors Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
A bare 32-char destination hash (no colon/path) was falling through to the else branch, treating it as a path on the current node — which caused the Python layer to default back to /page/index.mu on the same node, appearing as a "reload." Now recognized as cross-node with default path. Also makes hex validation case-insensitive in both hash:path and bare hash branches. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Implement NomadNet's partial markup (`{url`refresh`fields}) for
inline async-loaded content panels. Partials are fetched from remote
nodes, parsed as nested Micron documents, and rendered inline.
- Parse block-level `{ }` syntax with URL, refresh interval, fields, pid
- PartialManager orchestrates async loading with Semaphore(2) concurrency
- Auto-refresh via configurable interval; p:<pid> links trigger reload
- Extract shared resolveNomadNetUrl() eliminating duplicated URL parsing
- Fix angle brackets without backtick being silently dropped by parser
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Show an AlertDialog before revealing identity to the node operator. After successful identification, automatically refresh the page so the node can serve identity-aware content. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…on behavior) The Kotlin parser was emitting backticks literally for unrecognized formatting commands (e.g. backtick-space), while the Python reference silently discards them. This caused stray backticks after links in styled NomadNet pages like `[Send`dest]` . Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
canGoBack was a plain Boolean getter on a MutableList, invisible to Compose recomposition. BackHandler(enabled=...) read false once and never updated, so the system back always exited the browser. Replace with a StateFlow<Boolean> updated on history push/pop and collected via collectAsState() so BackHandler re-enables reactively. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
In scroll mode, disable softWrap and use widthIn(min=) instead of width() for all alignments. The fixed width() on centered/right lines was capping max width to the viewport, silently clipping wide content even though horizontalScroll was enabled on the parent. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace Modifier.layout with TextStyle.lineHeight for half-block pixel height so SpanStyle.background fills the full line (no transparent gaps). Bundle JetBrains Mono NL (Apache 2.0) so block elements (▄ █) have the same advance width as ASCII, fixing jagged row widths in pixel art with embedded text. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…esolution 53 tests covering: - ResolveNomadNetUrlTest (15): URL parsing for all link formats - NomadNetPageCacheTest (14): file cache with TTL, expiration, content preservation - NomadNetBrowserViewModelTest (23): navigation state machine, history, forms, identify Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ection Add browser-style address bar to NomadNet browser that displays the current page URL (hash:/path) in a tappable rounded field. Tapping opens the keyboard for editing; pressing Go navigates to the new URL. Overflow menu gains Copy URL and Share items. Register nomadnetwork:// as an Android intent-filter scheme so external apps and shared links can open NomadNet pages or start LXMF conversations (nomadnetwork://lxmf@hash). The intent handler uses raw string parsing to avoid Android's URI parser misinterpreting the colon in hash:/path. Detect lxmf@ links in Micron pages and emit a NavigationEvent so the browser screen can navigate to the conversation view. Extend conversation message linkification to recognize nomadnetwork:// and lxma:// URIs alongside standard http(s) URLs, making shared NomadNet links clickable in chat. Move ktlint PostToolUse hook to git pre-commit to avoid stripping imports between multi-step edits. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
app/src/main/java/com/lxmf/messenger/ui/screens/NomadNetBrowserScreen.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/lxmf/messenger/viewmodel/NomadNetBrowserViewModel.kt
Outdated
Show resolved
Hide resolved
Contributor
Greptile SummaryThis PR adds a full NomadNet page browser to the app: a standalone Key improvements compared to the initial version (many prior thread issues addressed):
Remaining new issues found in this review pass:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as NomadNetBrowserScreen
participant VM as NomadNetBrowserViewModel
participant Cache as NomadNetPageCache
participant PM as PartialManager
participant SRP as ServiceReticulumProtocol
participant Py as rns_api.py (Python)
UI->>VM: loadPage(nodeHash, path)
VM->>Cache: get(nodeHash, path)
alt Cache hit
Cache-->>VM: markup string
VM->>VM: MicronParser.parse(markup)
VM->>PM: detectAndLoad(document)
else Cache miss
VM->>SRP: requestNomadnetPage(hash, path)
SRP->>Py: request_nomadnet_page(bytes, path)
Py-->>SRP: {success, content, path}
SRP-->>VM: Result<NomadnetPageResult>
VM->>Cache: put(nodeHash, path, content, cacheTime)
VM->>VM: MicronParser.parse(content)
VM->>PM: detectAndLoad(document)
end
VM-->>UI: BrowserState.PageLoaded
PM->>SRP: requestNomadnetPage(partialUrl) [per partial]
SRP->>Py: request_nomadnet_page(bytes, partialPath)
Py-->>SRP: {success, content}
SRP-->>PM: Result<NomadnetPageResult>
PM-->>UI: partialStates update
UI->>VM: cancelLoading()
VM->>SRP: cancelNomadnetPageRequest()
SRP->>Py: cancel_nomadnet_page_request()
UI->>VM: identifyToNode()
VM->>SRP: identifyNomadnetLink(nodeHash)
SRP->>Py: identify_nomadnet_link(bytes)
Py-->>SRP: {success, already_identified}
SRP-->>VM: Result<Boolean>
VM->>VM: refresh() [if !alreadyIdentified]
Prompt To Fix All With AIThis is a comment left during a code review.
Path: app/src/main/java/com/lxmf/messenger/nomadnet/PartialManager.kt
Line: 282-290
Comment:
**Masked form fields forwarded to partial endpoints that didn't declare them**
`buildFormDataJson()` serialises every key/value in `_formFields` and attaches it to every partial request. `MicronElement.Partial.fieldNames` records which specific form fields that partial depends on, but this list is never consulted here — it is stored in `PartialState` and otherwise ignored.
Concretely: if a page has a password field (`!|password`) alongside a partial that only declares `fieldNames = ["search"]`, the partial request will include the plaintext password value alongside the search query. The partial endpoint (which may be on a different path or even a different node) receives data it never asked for.
Per NomadNet's reference implementation (`Browser.py`), only the fields declared in the partial definition should be forwarded.
The fix is to thread `partial.fieldNames` through `loadPartial`/`fetchAndUpdate` and filter the map:
```kotlin
private fun buildFormDataJson(allowedFields: List<String>): String? {
val fields = formFields()
if (fields.isEmpty()) return null
val filtered = if (allowedFields.isEmpty()) fields
else fields.filterKeys { it in allowedFields }
if (filtered.isEmpty()) return null
val json = JSONObject()
for ((k, v) in filtered) json.put(k, v)
return json.toString()
}
```
Then update `fetchAndUpdate` to accept and pass through the `fieldNames` from the partial.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: app/src/main/java/com/lxmf/messenger/ui/components/MicronComposables.kt
Line: 85-96
Comment:
**Redundant `TextMeasurer` allocations in `LazyColumn` path**
`MicronPageContent` is called once per line item in the `LazyColumn` branch of `NomadNetBrowserScreen` (i.e. `MONOSPACE_ZOOM` and `PROPORTIONAL_WRAP` modes). Each call lands here and runs:
1. `rememberTextMeasurer()` — a composable that allocates a `TextMeasurer` slot per item
2. `remember(density) { textMeasurer.measure(...) }` — actually performs the measurement
But `squareLineHeightSp` is only used when `renderingMode == RenderingMode.MONOSPACE_SCROLL`, and that mode never uses `LazyColumn`. So for a 200-line page in `PROPORTIONAL_WRAP` mode, 200 `TextMeasurer` objects are allocated and 200 measurements are computed, all of which are discarded (the branch below assigns `TextUnit.Unspecified`).
Consider hoisting the measurement above the `LazyColumn` in `NomadNetBrowserScreen` and passing `squareLineHeightSp` directly as a parameter to `MicronPageContent` / `MicronLineComposable`, or guard the `remember` block:
```kotlin
val measuredLineHeightSp =
if (renderingMode == RenderingMode.MONOSPACE_SCROLL) {
remember(density) {
val result = textMeasurer.measure(
AnnotatedString("A"),
TextStyle(fontFamily = JetBrainsMonoFamily, fontSize = 14.sp),
)
with(density) { (result.size.width * 2).toDp().toSp() }
}
} else {
TextUnit.Unspecified
}
```
This avoids computing the measurement entirely in modes that don't use it, reducing wasted work for each recycled `LazyColumn` item.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 05111b4 |
app/src/main/java/com/lxmf/messenger/nomadnet/PartialManager.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/lxmf/messenger/service/binder/ReticulumServiceBinder.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/lxmf/messenger/service/manager/PythonWrapperManager.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/lxmf/messenger/ui/screens/NomadNetBrowserScreen.kt
Outdated
Show resolved
Hide resolved
…owser PR
- Fix all detekt violations in app and micron modules (LongMethod, ReturnCount,
CyclomaticComplexMethod, LoopWithTooManyJumpStatements, TooManyFunctions,
UseCheckOrError, UnusedParameter, NoRelaxedMocks, NoVerifyOnlyTests)
- Fix race condition in PartialManager: use atomic StateFlow.update{} instead
of non-atomic read-modify-write pattern
- Fix JSON string injection in error handlers via JSONObject.quote()
- Fix retry button targeting initial destination instead of failed URL
- Remove unused isDark and fontFamily parameters from MicronComposables
- Restructure NomadNetPageCache.get() and PartialManager URL resolution to
reduce return count
- Add proper @Suppress annotations for parser functions with inherent complexity
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
app/src/main/java/com/lxmf/messenger/viewmodel/NomadNetBrowserViewModel.kt
Show resolved
Hide resolved
Contributor
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
app/src/main/java/com/lxmf/messenger/MainActivityIntentHandler.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/lxmf/messenger/viewmodel/NomadNetBrowserViewModel.kt
Outdated
Show resolved
Hide resolved
removeLast() resolves to Java 21's SequencedCollection.removeLast() when compiled with JDK 21, causing NoSuchMethodError on CI (JDK 17). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
app/src/main/java/com/lxmf/messenger/viewmodel/NomadNetBrowserViewModel.kt
Show resolved
Hide resolved
app/src/main/java/com/lxmf/messenger/nomadnet/PartialManager.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/lxmf/messenger/viewmodel/NomadNetBrowserViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/lxmf/messenger/viewmodel/NomadNetBrowserViewModel.kt
Show resolved
Hide resolved
Comment on lines
+181
to
+183
| val document = MicronParser.parse(cached) | ||
| emitPageLoaded(document, path, destinationHash) | ||
| return |
Contributor
There was a problem hiding this comment.
MicronParser.parse called on the main thread for cache hits
loadPage is invoked from a LaunchedEffect which runs on the main dispatcher by default. When a cache hit is found, MicronParser.parse(cached) is called synchronously on the main thread (line 181), and the same pattern exists in navigateToLink's cached-page branch (line 253) which is called directly from Compose's onLinkClick.
For typical small NomadNet pages this is fine, but a large page with many lines could block the composition thread long enough to cause a visible jank frame.
Consider wrapping the parse in withContext(Dispatchers.Default):
val document = withContext(Dispatchers.Default) {
MicronParser.parse(cached)
}
emitPageLoaded(document, path, destinationHash)(This requires making loadPage a suspend function, or performing the work inside a viewModelScope.launch.)
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/main/java/com/lxmf/messenger/viewmodel/NomadNetBrowserViewModel.kt
Line: 181-183
Comment:
**`MicronParser.parse` called on the main thread for cache hits**
`loadPage` is invoked from a `LaunchedEffect` which runs on the main dispatcher by default. When a cache hit is found, `MicronParser.parse(cached)` is called synchronously on the main thread (line 181), and the same pattern exists in `navigateToLink`'s cached-page branch (line 253) which is called directly from Compose's `onLinkClick`.
For typical small NomadNet pages this is fine, but a large page with many lines could block the composition thread long enough to cause a visible jank frame.
Consider wrapping the parse in `withContext(Dispatchers.Default)`:
```kotlin
val document = withContext(Dispatchers.Default) {
MicronParser.parse(cached)
}
emitPageLoaded(document, path, destinationHash)
```
(This requires making `loadPage` a `suspend` function, or performing the work inside a `viewModelScope.launch`.)
How can I resolve this? If you propose a fix, please make it concise.The micron module had 878 lines of tests but no JaCoCo plugin, so codecov reported 0% patch coverage for all parser code. Add jacoco plugin to generate XML reports, include :micron:test in CI module tests job, and add micron/ to codecov flags. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
app/src/main/java/com/lxmf/messenger/ui/screens/NomadNetBrowserScreen.kt
Show resolved
Hide resolved
Follow the strangler fig pattern: move request_nomadnet_page and cancel_nomadnet_page_request from the 8000-line reticulum_wrapper.py monolith to rns_api.py. Split the monolithic method into three focused methods (_establish_link, _send_page_request, request_nomadnet_page). Kotlin callers now go through PythonWrapperManager.requestNomadnetPage() -> rns_api.py, matching the existing pattern for identifyNomadnetLink. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
app/src/main/java/com/lxmf/messenger/viewmodel/NomadNetBrowserViewModel.kt
Show resolved
Hide resolved
app/src/main/java/com/lxmf/messenger/viewmodel/NomadNetBrowserViewModel.kt
Show resolved
Hide resolved
app/src/main/java/com/lxmf/messenger/ui/screens/NomadNetBrowserScreen.kt
Outdated
Show resolved
Hide resolved
- Add fetchEpoch counter to prevent stale cancel coroutines from poisoning subsequent retry requests. cancelLoading() captures the epoch before dispatching to IO; if a new fetch has started by the time it runs, the cancel is skipped. - Add isPullRefreshing StateFlow so PullToRefreshBox spinner stays visible during refresh. The previous approach (checking BrowserState.Loading) couldn't work because the PullToRefreshBox is only composed in the PageLoaded branch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…lumn - Replace '|' with '\u001F' (Unit Separator) as the annotation separator between link destination and field names, so multi-partial reload links like p:pid1|pid2 are not incorrectly split - Forward pageBackground/pageForeground from the parent document when creating per-line MicronDocuments in LazyColumn rendering mode, so pages with #!bg= directives render correct backgrounds Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment on lines
+192
to
+197
| val cached = pageCache.get(destinationHash, path) | ||
| if (cached != null) { | ||
| val document = MicronParser.parse(cached) | ||
| emitPageLoaded(document, path, destinationHash) | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
Blocking file I/O in loadPage on the main thread
pageCache.get() calls cacheDir.listFiles() (a blocking filesystem scan) and on a hit, file.readText() (a blocking read). Both loadPage and navigateToLink are non-suspend functions that run on the main thread — loadPage is called from a LaunchedEffect on the main dispatcher, and navigateToLink is called directly from onLinkClick (a Compose click callback). For a fully-warm cache with many entries, listFiles() alone can take several milliseconds, causing dropped frames.
Note: the MicronParser.parse call that immediately follows the cache read adds to the main-thread work. Both should be moved off together with withContext(Dispatchers.Default/IO). For example:
viewModelScope.launch {
val cached = withContext(Dispatchers.IO) { pageCache.get(destinationHash, path) }
if (cached != null) {
val document = withContext(Dispatchers.Default) { MicronParser.parse(cached) }
emitPageLoaded(document, path, destinationHash)
return@launch
}
fetchPage(destinationHash, path, cacheResponse = true)
}The same fix applies to the cache hit path in navigateToLink (lines 252–256).
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/main/java/com/lxmf/messenger/viewmodel/NomadNetBrowserViewModel.kt
Line: 192-197
Comment:
**Blocking file I/O in `loadPage` on the main thread**
`pageCache.get()` calls `cacheDir.listFiles()` (a blocking filesystem scan) and on a hit, `file.readText()` (a blocking read). Both `loadPage` and `navigateToLink` are non-suspend functions that run on the main thread — `loadPage` is called from a `LaunchedEffect` on the main dispatcher, and `navigateToLink` is called directly from `onLinkClick` (a Compose click callback). For a fully-warm cache with many entries, `listFiles()` alone can take several milliseconds, causing dropped frames.
Note: the `MicronParser.parse` call that immediately follows the cache read adds to the main-thread work. Both should be moved off together with `withContext(Dispatchers.Default/IO)`. For example:
```kotlin
viewModelScope.launch {
val cached = withContext(Dispatchers.IO) { pageCache.get(destinationHash, path) }
if (cached != null) {
val document = withContext(Dispatchers.Default) { MicronParser.parse(cached) }
emitPageLoaded(document, path, destinationHash)
return@launch
}
fetchPage(destinationHash, path, cacheResponse = true)
}
```
The same fix applies to the cache hit path in `navigateToLink` (lines 252–256).
How can I resolve this? If you propose a fix, please make it concise.…nk cache cap
- Fix checkbox checked-state using String.contains() (substring match)
instead of split(",").contains() (list membership), which caused
false positives when checkbox values share prefixes
- Move NomadNetPageCache.cleanExpired() to a background thread in init
to avoid blocking the main thread with file I/O on startup
- Cap _nomadnet_links dict at MAX_IDENTIFIED_LINKS entries, tearing
down the oldest link when evicting to release RNS resources
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment on lines
+117
to
+126
| link = wrapper._nomadnet_links.get(dest_hash_hex) | ||
| if link is not None and link.status != RNS.Link.ACTIVE: | ||
| # Stale link, remove it | ||
| log_debug("RnsApi", "request_nomadnet_page", | ||
| f"Stale link to {dest_hash_hex[:16]}, will re-establish") | ||
| wrapper._nomadnet_links.pop(dest_hash_hex, None) | ||
| link = None | ||
|
|
||
| # Only recall identity and establish link if no cached active link | ||
| if link is not None: |
Contributor
There was a problem hiding this comment.
Bug: A race condition exists when evicting from the _nomadnet_links cache. Concurrent modifications can cause a RuntimeError because the dictionary's size is checked before it is iterated.
Severity: MEDIUM
Suggested Fix
Wrap the cache eviction logic in a threading.Lock. The lock should be acquired before checking len(wrapper._nomadnet_links) and released after the pop() operation to ensure the check-then-evict sequence is atomic and prevent race conditions with other threads.
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: python/rns_api.py#L117-L126
Potential issue: Concurrent calls to `request_nomadnet_page()` can create a race
condition when the `_nomadnet_links` cache is full. The code checks the dictionary's
size and then gets an iterator to find an item to evict. If another thread modifies the
dictionary between the size check and the iteration, the operation can fail with a
`RuntimeError: dictionary changed size during iteration`. While the
`MAX_CONCURRENT_FETCHES` semaphore limits concurrency to two, and the race window is
very small, this could still lead to an unexpected crash under specific load conditions.
app/src/main/java/com/lxmf/messenger/viewmodel/NomadNetBrowserViewModel.kt
Show resolved
Hide resolved
- Add @volatile to currentNodeHash for cross-thread visibility - Fix TOCTOU race in detectAndLoad using ConcurrentHashMap.putIfAbsent - Stop auto-refresh partials after 3 consecutive errors Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
app/src/main/java/com/lxmf/messenger/nomadnet/PartialManager.kt
Outdated
Show resolved
Hide resolved
- Reserve job slot with placeholder before launching coroutine, so clear() can never miss an in-flight job between launch and map insert - Add MAX_CACHED_LINKS = 8 for live RNS.Link objects (was reusing MAX_IDENTIFIED_LINKS = 200, far too large for heavyweight objects) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace `if (...) continue` with `if (...) { ... }` guard to keep
the loop at one jump statement (the safe-cast `?: continue`).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Some-O-ne
reviewed
Mar 13, 2026
| */ | ||
| @Suppress("ReturnCount") | ||
| fun parse(colorStr: String): MicronColor? { | ||
| if (colorStr.length < 3) return null |
Owner
Author
There was a problem hiding this comment.
Thank you for the reminder!
Comment on lines
+79
to
+101
| * - `0` → do not cache (returns immediately) | ||
| * - positive → cache for that many seconds | ||
| */ | ||
| fun put( | ||
| nodeHash: String, | ||
| path: String, | ||
| content: String, | ||
| cacheSeconds: Int?, | ||
| ) { | ||
| val ttl = cacheSeconds ?: DEFAULT_CACHE_SECONDS | ||
| if (ttl <= 0) return | ||
|
|
||
| val key = cacheKey(nodeHash, path) | ||
|
|
||
| try { | ||
| cacheDir.mkdirs() | ||
|
|
||
| // Remove any existing entries for this key | ||
| removeByKey(key) | ||
|
|
||
| val expiresMs = System.currentTimeMillis() + ttl * 1000L | ||
| val file = File(cacheDir, "${key}_$expiresMs") | ||
| file.writeText(content) |
Contributor
There was a problem hiding this comment.
Non-atomic removeByKey + writeText can leave orphaned cache files
put() is called from fetchPage on Dispatchers.IO. If two concurrent callers write the same (nodeHash, path) key — e.g. a rapid link re-tap while a fresh network fetch is landing — the removeByKey + writeText sequence races:
- Thread A:
removeByKey(key)deletes the existing file - Thread B:
removeByKey(key)finds nothing (already gone) - Thread A: writes
"${key}_timestamp1" - Thread B: writes
"${key}_timestamp2"
Both files now exist for the same key. get() picks whichever the filesystem returns first via firstOrNull, leaving the other file as an orphan until cleanExpired() runs. Over time this can accumulate duplicate files for high-traffic keys.
Consider adding a per-operation lock (a Mutex keyed by the cache key, or a simple @Synchronized on the method since disk I/O is already the bottleneck) to make the remove-then-write atomic.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/main/java/com/lxmf/messenger/nomadnet/NomadNetPageCache.kt
Line: 79-101
Comment:
**Non-atomic `removeByKey` + `writeText` can leave orphaned cache files**
`put()` is called from `fetchPage` on `Dispatchers.IO`. If two concurrent callers write the same `(nodeHash, path)` key — e.g. a rapid link re-tap while a fresh network fetch is landing — the `removeByKey` + `writeText` sequence races:
1. Thread A: `removeByKey(key)` deletes the existing file
2. Thread B: `removeByKey(key)` finds nothing (already gone)
3. Thread A: writes `"${key}_timestamp1"`
4. Thread B: writes `"${key}_timestamp2"`
Both files now exist for the same key. `get()` picks whichever the filesystem returns first via `firstOrNull`, leaving the other file as an orphan until `cleanExpired()` runs. Over time this can accumulate duplicate files for high-traffic keys.
Consider adding a per-operation lock (a `Mutex` keyed by the cache key, or a simple `@Synchronized` on the method since disk I/O is already the bottleneck) to make the remove-then-write atomic.
How can I resolve this? If you propose a fix, please make it concise.The tests verified Log.e calls from a Dispatchers.IO coroutine using advanceUntilIdle(), which only controls the test dispatcher. Use verify(timeout = 1000) to wait for the IO thread to complete. Also add missing 3-arg Log.e(String, String, Throwable) mock stub. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
heeey torlando when truecolor color tags? :> |
app/src/main/java/com/lxmf/messenger/viewmodel/NomadNetBrowserViewModel.kt
Show resolved
Hide resolved
- fetchEpoch is read from IO threads; @volatile ensures visibility - Replace blocking response_event.wait() with 0.5s polling loop that checks _cancel_flag, matching the pattern used in _establish_link Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment on lines
+175
to
+178
| jobs[key] = | ||
| scope.launch(Dispatchers.IO) { | ||
| fetchAndUpdate(key, existing.url, existing.refreshInterval) | ||
| } |
Contributor
There was a problem hiding this comment.
Bug: The reloadPartial function's non-atomic job assignment can cause a race condition with clear(), leading to leaked coroutines and stale UI state updates during navigation.
Severity: MEDIUM
Suggested Fix
Make the job creation and assignment in reloadPartial atomic. Use a placeholder pattern similar to the one in detectAndLoad, where a temporary Job is placed in the map atomically using putIfAbsent before the real coroutine is launched and replaces it. This ensures that concurrent calls to clear() will correctly cancel the job.
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/nomadnet/PartialManager.kt#L175-L178
Potential issue: A race condition exists in `PartialManager.reloadPartial`. The function
directly assigns a new job to the `jobs` map without an atomic operation. If a user
triggers a partial reload and then navigates away, the `clear()` function may be called
concurrently. If `clear()` runs after the old job is cancelled but before the new one is
assigned, the new job will be added to an already-cleared map. This "leaked" coroutine
will continue running and eventually update `_states` with stale data, potentially
causing the UI to display incorrect content from the previous page.
Replace blocking time.sleep(2s) with 0.25s polling loop so cancel requests are responsive even during the existing-path fast path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
app/src/main/java/com/lxmf/messenger/ui/components/MicronComposables.kt
Outdated
Show resolved
Hide resolved
Calling rememberTextMeasurer() inside an if-block violates Compose's rules of remember — the slot table ordering must be stable across recompositions. Hoisting the call ensures consistent ordering even when renderingMode changes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
Will have to get to it tomorrow. I'm gonna have to do another round of testing after all the automated iteration this went through anyway |
app/src/main/java/com/lxmf/messenger/ui/components/MicronComposables.kt
Outdated
Show resolved
Hide resolved
…ility Move the text measurement remember block unconditionally outside the if-block so the slot table is stable across rendering mode changes. The measurement is cheap (single character) and the result is only applied in MONOSPACE_SCROLL mode. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Rewrites the NomadNet page link establishment to match the NomadNet TUI's proven path-first sequence (Browser.py __load): ensure path → recall identity → create destination → establish link. The old code did identity-first (recall identity → construct destination → request path with recomputed hash → unconditional 2s fresh-path wait). This caused failures because the path table starts empty on every app restart for non-transport nodes. Key changes: - Path resolution uses the original destination hash throughout, never a recomputed one from Destination.hash - Removed the unconditional 2s fresh-path wait that delayed every page load even when a valid path already existed - Replaced single-shot RNS.Transport.await_path with a retry loop that re-sends request_path every 5s. RNS sends the path request packet exactly once with no retry — if the packet or response is lost, the path never arrives. The retry loop works around this. - Cancel flag is polled every 0.25s during path wait Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment on lines
+282
to
+290
| private fun buildFormDataJson(): String? { | ||
| val fields = formFields() | ||
| if (fields.isEmpty()) return null | ||
| val json = JSONObject() | ||
| for ((k, v) in fields) { | ||
| json.put(k, v) | ||
| } | ||
| return json.toString() | ||
| } |
Contributor
There was a problem hiding this comment.
Masked form fields forwarded to partial endpoints that didn't declare them
buildFormDataJson() serialises every key/value in _formFields and attaches it to every partial request. MicronElement.Partial.fieldNames records which specific form fields that partial depends on, but this list is never consulted here — it is stored in PartialState and otherwise ignored.
Concretely: if a page has a password field (!|password) alongside a partial that only declares fieldNames = ["search"], the partial request will include the plaintext password value alongside the search query. The partial endpoint (which may be on a different path or even a different node) receives data it never asked for.
Per NomadNet's reference implementation (Browser.py), only the fields declared in the partial definition should be forwarded.
The fix is to thread partial.fieldNames through loadPartial/fetchAndUpdate and filter the map:
private fun buildFormDataJson(allowedFields: List<String>): String? {
val fields = formFields()
if (fields.isEmpty()) return null
val filtered = if (allowedFields.isEmpty()) fields
else fields.filterKeys { it in allowedFields }
if (filtered.isEmpty()) return null
val json = JSONObject()
for ((k, v) in filtered) json.put(k, v)
return json.toString()
}Then update fetchAndUpdate to accept and pass through the fieldNames from the partial.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/main/java/com/lxmf/messenger/nomadnet/PartialManager.kt
Line: 282-290
Comment:
**Masked form fields forwarded to partial endpoints that didn't declare them**
`buildFormDataJson()` serialises every key/value in `_formFields` and attaches it to every partial request. `MicronElement.Partial.fieldNames` records which specific form fields that partial depends on, but this list is never consulted here — it is stored in `PartialState` and otherwise ignored.
Concretely: if a page has a password field (`!|password`) alongside a partial that only declares `fieldNames = ["search"]`, the partial request will include the plaintext password value alongside the search query. The partial endpoint (which may be on a different path or even a different node) receives data it never asked for.
Per NomadNet's reference implementation (`Browser.py`), only the fields declared in the partial definition should be forwarded.
The fix is to thread `partial.fieldNames` through `loadPartial`/`fetchAndUpdate` and filter the map:
```kotlin
private fun buildFormDataJson(allowedFields: List<String>): String? {
val fields = formFields()
if (fields.isEmpty()) return null
val filtered = if (allowedFields.isEmpty()) fields
else fields.filterKeys { it in allowedFields }
if (filtered.isEmpty()) return null
val json = JSONObject()
for ((k, v) in filtered) json.put(k, v)
return json.toString()
}
```
Then update `fetchAndUpdate` to accept and pass through the `fieldNames` from the partial.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+85
to
+96
| val textMeasurer = rememberTextMeasurer() | ||
| val measuredLineHeightSp = | ||
| remember(density) { | ||
| val result = | ||
| textMeasurer.measure( | ||
| AnnotatedString("A"), | ||
| TextStyle(fontFamily = JetBrainsMonoFamily, fontSize = 14.sp), | ||
| ) | ||
| with(density) { (result.size.width * 2).toDp().toSp() } | ||
| } | ||
| val squareLineHeightSp = | ||
| if (renderingMode == RenderingMode.MONOSPACE_SCROLL) measuredLineHeightSp else TextUnit.Unspecified |
Contributor
There was a problem hiding this comment.
Redundant TextMeasurer allocations in LazyColumn path
MicronPageContent is called once per line item in the LazyColumn branch of NomadNetBrowserScreen (i.e. MONOSPACE_ZOOM and PROPORTIONAL_WRAP modes). Each call lands here and runs:
rememberTextMeasurer()— a composable that allocates aTextMeasurerslot per itemremember(density) { textMeasurer.measure(...) }— actually performs the measurement
But squareLineHeightSp is only used when renderingMode == RenderingMode.MONOSPACE_SCROLL, and that mode never uses LazyColumn. So for a 200-line page in PROPORTIONAL_WRAP mode, 200 TextMeasurer objects are allocated and 200 measurements are computed, all of which are discarded (the branch below assigns TextUnit.Unspecified).
Consider hoisting the measurement above the LazyColumn in NomadNetBrowserScreen and passing squareLineHeightSp directly as a parameter to MicronPageContent / MicronLineComposable, or guard the remember block:
val measuredLineHeightSp =
if (renderingMode == RenderingMode.MONOSPACE_SCROLL) {
remember(density) {
val result = textMeasurer.measure(
AnnotatedString("A"),
TextStyle(fontFamily = JetBrainsMonoFamily, fontSize = 14.sp),
)
with(density) { (result.size.width * 2).toDp().toSp() }
}
} else {
TextUnit.Unspecified
}This avoids computing the measurement entirely in modes that don't use it, reducing wasted work for each recycled LazyColumn item.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/main/java/com/lxmf/messenger/ui/components/MicronComposables.kt
Line: 85-96
Comment:
**Redundant `TextMeasurer` allocations in `LazyColumn` path**
`MicronPageContent` is called once per line item in the `LazyColumn` branch of `NomadNetBrowserScreen` (i.e. `MONOSPACE_ZOOM` and `PROPORTIONAL_WRAP` modes). Each call lands here and runs:
1. `rememberTextMeasurer()` — a composable that allocates a `TextMeasurer` slot per item
2. `remember(density) { textMeasurer.measure(...) }` — actually performs the measurement
But `squareLineHeightSp` is only used when `renderingMode == RenderingMode.MONOSPACE_SCROLL`, and that mode never uses `LazyColumn`. So for a 200-line page in `PROPORTIONAL_WRAP` mode, 200 `TextMeasurer` objects are allocated and 200 measurements are computed, all of which are discarded (the branch below assigns `TextUnit.Unspecified`).
Consider hoisting the measurement above the `LazyColumn` in `NomadNetBrowserScreen` and passing `squareLineHeightSp` directly as a parameter to `MicronPageContent` / `MicronLineComposable`, or guard the `remember` block:
```kotlin
val measuredLineHeightSp =
if (renderingMode == RenderingMode.MONOSPACE_SCROLL) {
remember(density) {
val result = textMeasurer.measure(
AnnotatedString("A"),
TextStyle(fontFamily = JetBrainsMonoFamily, fontSize = 14.sp),
)
with(density) { (result.size.width * 2).toDp().toSp() }
}
} else {
TextUnit.Unspecified
}
```
This avoids computing the measurement entirely in modes that don't use it, reducing wasted work for each recycled `LazyColumn` item.
How can I resolve this? If you propose a fix, please make it concise.
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
hash:/path, with Copy URL and Share menu itemsnomadnetwork://URI handler — external apps can open NomadNet pages or LXMF conversations via deep linksnomadnetwork://andlxma://URLs are now clickable in conversation messagesTest plan
hash:/pathadb shell am start -a android.intent.action.VIEW -d "nomadnetwork://hash:/page/index.mu"opens browsernomadnetwork://links in conversations are clickable and open browserlxmf@hashlinks on Micron pages open conversation screen🤖 Generated with Claude Code