fix(reticulum): block RNCP sends to unreachable receive dests - #747
Conversation
Probe the rncp receive destination before send/fetch/retry so stuck 0% transfers are not queued when the peer is offline or inbound listening is off; prompt an enable request when chat still looks reachable, and surface pending ASK offers on Chat and the Remote sidebar badge.
Ask the peer to enable receive and reply with their rncp receive hash so senders can autofill a live destination.
📝 WalkthroughWalkthroughRNCP transfers now perform destination reachability checks before transfer actions. Reticulum DM views display matching pending offers with accept or reject actions. The Remote sidebar tab displays pending-offer counts. ChangesRNCP transfer flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ChatDmRncpControl
participant ensureRncpDestinationReachable
participant probeReticulumPeer
participant ConfirmModal
participant RNCPTransfer
ChatDmRncpControl->>ensureRncpDestinationReachable: check destination
ensureRncpDestinationReachable->>probeReticulumPeer: probe RNCP hash
probeReticulumPeer-->>ensureRncpDestinationReachable: reachability result
ensureRncpDestinationReachable-->>ChatDmRncpControl: return status
ChatDmRncpControl->>ConfirmModal: confirm listener enablement when needed
ConfirmModal-->>ChatDmRncpControl: confirm request
ChatDmRncpControl->>RNCPTransfer: send file after successful check
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/renderer/components/remote/ChatDmRncpControl.tsx (1)
192-259: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftConsider extracting the reachability-gate logic shared with
RemoteTransferSection.tsx.This block reimplements the same three-way branch (
reachable/listenerLikelyOff/peerUnreachable) and confirm-modal wiring thatRemoteTransferSection.tsx'sassertReachableForTransfer(Lines 97-113) implements independently. Extracting a shared hook, for exampleuseRncpReachabilityGate(lxmfPeerHash)returning{ checkReachable, enableRequestConfirmOpen, closeEnableRequestConfirm }, would remove the duplication and reduce the risk of the two call sites diverging in behavior (see the related comment onRemoteTransferSection.tsx'shandleRetry, where a related bug slipped into one implementation but not the other).🤖 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 `@src/renderer/components/remote/ChatDmRncpControl.tsx` around lines 192 - 259, Extract the shared RNCP reachability and listener-confirmation flow from handleSend and RemoteTransferSection’s assertReachableForTransfer into a reusable useRncpReachabilityGate hook. Preserve the reachable, listenerLikelyOff, and peerUnreachable outcomes, including enableRequestConfirmOpen state and its close handler, then update both call sites to use the hook and remove their duplicated branching.src/renderer/components/remote/RemoteTransferSection.tsx (1)
233-246: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMove
incrementRetry(transferId)after the reachability check succeeds.In
handleRetry(lines 233–246),incrementRetry(transferId)at line 239 increments the stored transfer'sretryCountbefore the reachability gate at lines 241–246. TheincrementRetryfunction uses Zustand'sset()to mutate the transfer record in the store immediately. IfassertReachableForTransferreturnsfalse, the function returns at line 245 without attempting a retry, butretryCountis already incremented. When the destination remains transiently unreachable across multiple manual or automatic retry attempts, the retry count reachesmaxRetry(line 675 hides the Retry button whentransfer.retryCount >= maxRetry) without any real retry attempt occurring. The user then loses the ability to retry even after the destination becomes reachable.Move
incrementRetryto execute only afterassertReachableForTransfersucceeds, so the count reflects actual retry attempts.Do you want me to generate a regression test for this path once the fix is in place?
🤖 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 `@src/renderer/components/remote/RemoteTransferSection.tsx` around lines 233 - 246, Move the incrementRetry(transferId) call in handleRetry to after assertReachableForTransfer succeeds, while preserving the existing early return when reachability fails. This ensures retryCount changes only when an actual retry attempt proceeds.
🟡 Other comments (4)
src/renderer/components/Sidebar.test.tsx-241-256 (1)
241-256: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd axe coverage for the new RNCP badge and banner.
Both new UI elements use saturated colors and interactive controls. Add
hydrateAxeThemeColors()andaxe()assertions for their visible states.
src/renderer/components/Sidebar.test.tsx#L241-L256: Assert no axe violations when the Remote pending-offer badge is visible.src/renderer/components/remote/ChatDmRncpOfferBanner.test.tsx#L35-L64: Assert no axe violations when a matching offer renders Accept and Reject controls.As per coding guidelines, “assert accessibility with
vitest-axe; callhydrateAxeThemeColors()instead of mockingthemeColors.” As per path instructions, “For new badges/banners and saturated color combinations, add axe accessibility coverage and ensure contrast.”🤖 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 `@src/renderer/components/Sidebar.test.tsx` around lines 241 - 256, Add vitest-axe coverage for both new UI states: in src/renderer/components/Sidebar.test.tsx lines 241-256, call hydrateAxeThemeColors() and assert axe() reports no violations after rendering the visible Remote pending-offer badge; in src/renderer/components/remote/ChatDmRncpOfferBanner.test.tsx lines 35-64, do the same for a matching RNCP offer rendering Accept and Reject controls. Do not mock themeColors.Sources: Coding guidelines, Path instructions
src/renderer/components/Sidebar.tsx-81-89 (1)
81-89: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMatch the capped accessible count to the visible badge.
When
remotePendingOffersexceeds 99, the badge displays99+but the accessible label receives99. Pass'99+'to the translation in this case, asaria.tabWithUnreadalready does.Proposed fix
- count: badgeCount > 99 ? 99 : badgeCount, + count: badgeCount > 99 ? '99+' : badgeCount,🤖 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 `@src/renderer/components/Sidebar.tsx` around lines 81 - 89, Update the showRemoteBadge branch of the tabAriaLabel calculation to pass the capped string '99+' when badgeCount exceeds 99, matching the visible badge; retain the numeric badgeCount for values up to 99 and leave the aria.tabWithUnread branch unchanged.src/renderer/components/Sidebar.tsx-121-124 (1)
121-124: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a higher-contrast Remote badge color.
bg-amber-600withtext-whitefails contrast requirements for the 10px badge text. Use a darker amber utility, such asbg-amber-800, and cover this state withvitest-axe.Proposed fix
- showRemoteBadge ? 'bg-amber-600' : 'bg-red-600' + showRemoteBadge ? 'bg-amber-800' : 'bg-red-600'As per path instructions, “For new badges/banners and saturated color combinations, add axe accessibility coverage and ensure contrast.”
🤖 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 `@src/renderer/components/Sidebar.tsx` around lines 121 - 124, Update the remote state styling in the Sidebar badge className to use a darker amber utility such as bg-amber-800 with white text, preserving the existing red styling for non-remote badges. Add vitest-axe coverage that exercises the remote badge state and verifies it has no accessibility violations.Source: Path instructions
src/renderer/components/remote/RemoteTransferSection.tsx-97-113 (1)
97-113: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGeneric error message during retry misrepresents
listenerLikelyOff.When
opts.promptEnableisfalse(thehandleRetrypath) andreach.status === 'listenerLikelyOff', the code falls through to the same toast used forpeerUnreachable: "the peer may be offline." In this state the LXMF peer is actually reachable; only the receive listener appears disabled. Showing the generic offline message during automatic or manual retries can mislead the user into believing the destination is completely gone, when sending an enable request would resolve it.Use a distinct message for this branch, for example a toast that mentions the listener is likely off, so retry failures are diagnosable.
🤖 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 `@src/renderer/components/remote/RemoteTransferSection.tsx` around lines 97 - 113, Update assertReachableForTransfer so listenerLikelyOff remains a distinct branch when opts.promptEnable is false, displaying a listener-specific error toast instead of the generic peerUnreachable message; preserve the existing confirmation flow when prompting is enabled and the generic toast for other unreachable statuses.
🤖 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 `@src/renderer/components/remote/ChatDmRncpOfferBanner.tsx`:
- Around line 71-85: Update the pending-offer action flow around handleAccept
and handleReject to track in-flight state per transfer_id, synchronously
blocking repeated or opposite actions before React renders disabled controls.
Disable both Accept and Reject buttons for that offer while either helper is
pending, and clear the per-transfer state in finally so failed accepts remain
retryable. Add a behavioral test that keeps Accept pending, clicks Reject, and
verifies only one IPC request is issued.
In `@src/renderer/lib/ensureRncpDestinationReachable.ts`:
- Around line 27-30: Extract the repeated 32-character hexadecimal validation
into a shared isRncpHexHash(value: string) helper, then replace the inline
checks in ensureRncpDestinationReachable and RemoteTransferSection’s
resolveLxmfPeerHash with that helper. Preserve the existing trimming and
lowercasing behavior before validation.
---
Outside diff comments:
In `@src/renderer/components/remote/ChatDmRncpControl.tsx`:
- Around line 192-259: Extract the shared RNCP reachability and
listener-confirmation flow from handleSend and RemoteTransferSection’s
assertReachableForTransfer into a reusable useRncpReachabilityGate hook.
Preserve the reachable, listenerLikelyOff, and peerUnreachable outcomes,
including enableRequestConfirmOpen state and its close handler, then update both
call sites to use the hook and remove their duplicated branching.
In `@src/renderer/components/remote/RemoteTransferSection.tsx`:
- Around line 233-246: Move the incrementRetry(transferId) call in handleRetry
to after assertReachableForTransfer succeeds, while preserving the existing
early return when reachability fails. This ensures retryCount changes only when
an actual retry attempt proceeds.
---
Other comments:
In `@src/renderer/components/remote/RemoteTransferSection.tsx`:
- Around line 97-113: Update assertReachableForTransfer so listenerLikelyOff
remains a distinct branch when opts.promptEnable is false, displaying a
listener-specific error toast instead of the generic peerUnreachable message;
preserve the existing confirmation flow when prompting is enabled and the
generic toast for other unreachable statuses.
In `@src/renderer/components/Sidebar.test.tsx`:
- Around line 241-256: Add vitest-axe coverage for both new UI states: in
src/renderer/components/Sidebar.test.tsx lines 241-256, call
hydrateAxeThemeColors() and assert axe() reports no violations after rendering
the visible Remote pending-offer badge; in
src/renderer/components/remote/ChatDmRncpOfferBanner.test.tsx lines 35-64, do
the same for a matching RNCP offer rendering Accept and Reject controls. Do not
mock themeColors.
In `@src/renderer/components/Sidebar.tsx`:
- Around line 81-89: Update the showRemoteBadge branch of the tabAriaLabel
calculation to pass the capped string '99+' when badgeCount exceeds 99, matching
the visible badge; retain the numeric badgeCount for values up to 99 and leave
the aria.tabWithUnread branch unchanged.
- Around line 121-124: Update the remote state styling in the Sidebar badge
className to use a darker amber utility such as bg-amber-800 with white text,
preserving the existing red styling for non-remote badges. Add vitest-axe
coverage that exercises the remote badge state and verifies it has no
accessibility violations.
🪄 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: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 2797378e-4c8f-4731-a3a0-304a0d3e7faf
⛔ Files ignored due to path filters (16)
src/renderer/locales/cs/translation.jsonis excluded by!src/renderer/locales/**src/renderer/locales/de/translation.jsonis excluded by!src/renderer/locales/**src/renderer/locales/en/translation.jsonis excluded by!src/renderer/locales/**src/renderer/locales/es/translation.jsonis excluded by!src/renderer/locales/**src/renderer/locales/fr/translation.jsonis excluded by!src/renderer/locales/**src/renderer/locales/id/translation.jsonis excluded by!src/renderer/locales/**src/renderer/locales/it/translation.jsonis excluded by!src/renderer/locales/**src/renderer/locales/ja/translation.jsonis excluded by!src/renderer/locales/**src/renderer/locales/ko/translation.jsonis excluded by!src/renderer/locales/**src/renderer/locales/nl/translation.jsonis excluded by!src/renderer/locales/**src/renderer/locales/pl/translation.jsonis excluded by!src/renderer/locales/**src/renderer/locales/pt-BR/translation.jsonis excluded by!src/renderer/locales/**src/renderer/locales/ru/translation.jsonis excluded by!src/renderer/locales/**src/renderer/locales/tr/translation.jsonis excluded by!src/renderer/locales/**src/renderer/locales/uk/translation.jsonis excluded by!src/renderer/locales/**src/renderer/locales/zh/translation.jsonis excluded by!src/renderer/locales/**
📒 Files selected for processing (12)
src/renderer/App.tsxsrc/renderer/components/ChatPanel.tsxsrc/renderer/components/Sidebar.test.tsxsrc/renderer/components/Sidebar.tsxsrc/renderer/components/remote/ChatDmRncpControl.test.tsxsrc/renderer/components/remote/ChatDmRncpControl.tsxsrc/renderer/components/remote/ChatDmRncpOfferBanner.test.tsxsrc/renderer/components/remote/ChatDmRncpOfferBanner.tsxsrc/renderer/components/remote/RemoteTransferSection.test.tsxsrc/renderer/components/remote/RemoteTransferSection.tsxsrc/renderer/lib/ensureRncpDestinationReachable.test.tssrc/renderer/lib/ensureRncpDestinationReachable.ts
| const destinationHash = args.destinationHash.trim().toLowerCase(); | ||
| if (!/^[0-9a-f]{32}$/.test(destinationHash)) { | ||
| return { status: 'peerUnreachable' }; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract the hex-hash validator to reduce duplication.
The /^[0-9a-f]{32}$/ check appears twice in this file (Line 28 and Line 44), and an equivalent check is duplicated in RemoteTransferSection.tsx's resolveLxmfPeerHash (Line 94). Extract a small shared helper, for example isRncpHexHash(value: string): boolean, and reuse it in both files. This keeps the destination-hash and LXMF-hash validation rules consistent if the format ever changes.
♻️ Proposed helper extraction
+export function isRncpHexHash(value: string): boolean {
+ return /^[0-9a-f]{32}$/.test(value);
+}
+
export async function ensureRncpDestinationReachable(
args: EnsureRncpDestinationReachableArgs,
): Promise<RncpDestinationReachability> {
const destinationHash = args.destinationHash.trim().toLowerCase();
- if (!/^[0-9a-f]{32}$/.test(destinationHash)) {
+ if (!isRncpHexHash(destinationHash)) {
return { status: 'peerUnreachable' };
}
...
const lxmf = args.lxmfPeerHash?.trim().toLowerCase() ?? '';
- if (!/^[0-9a-f]{32}$/.test(lxmf) || lxmf === destinationHash) {
+ if (!isRncpHexHash(lxmf) || lxmf === destinationHash) {
return { status: 'peerUnreachable' };
}Also applies to: 43-46
🤖 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 `@src/renderer/lib/ensureRncpDestinationReachable.ts` around lines 27 - 30,
Extract the repeated 32-character hexadecimal validation into a shared
isRncpHexHash(value: string) helper, then replace the inline checks in
ensureRncpDestinationReachable and RemoteTransferSection’s resolveLxmfPeerHash
with that helper. Preserve the existing trimming and lowercasing behavior before
validation.
Block double Accept/Reject on the Chat DM offer banner with per-transfer in-flight state, and reuse isRncpHexHash for preflight/LXMF peer validation.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/renderer/components/remote/RemoteTransferSection.tsx (2)
379-379: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRequire a validated, non-duplicate LXMF target.
resolveLxmfPeerHashreturnsnullwhen the saved LXMF value is missing or invalid, but?? parsedHashthen passes the generic destination field tosendRncpRequestEnable. This field can contain an RNCP receive hash, not an LXMF delivery hash. The direct request path also does not reject a saved LXMF hash equal toparsedHash, although the preflight helper rejects that duplicate.Require a validated, non-duplicate LXMF target. If direct LXMF input is supported, preserve the input kind and validate it separately before sending.
As per path instructions, AGENTS.md requires “validate destination and LXMF hashes at boundaries” and reuse existing RNCP validation helpers.
🤖 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 `@src/renderer/components/remote/RemoteTransferSection.tsx` at line 379, Update the direct transfer path around peerLxmfHash so sendRncpRequestEnable receives only a validated LXMF delivery hash; remove the fallback to parsedHash, reject missing or invalid resolved values, and reject a saved LXMF hash equal to parsedHash using the existing RNCP validation helpers. If direct LXMF input is supported, preserve its input type and validate it separately at this boundary.Source: Path instructions
244-249: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve retry state and prevent duplicate retry submissions.
incrementRetry(transferId)runs beforeassertReachableForTransferat Line 242. If reachability fails, no RNCP request starts, but the transfer still consumes a retry. The automatic retry marker is also set beforehandleRetryruns at Lines 296-299, so a blocked probe can prevent recovery after the peer becomes reachable.Add a per-transfer in-flight guard before the probe. Run the probe before changing retry state. Increment the retry count only when the sidecar accepts the resubmission.
As per path instructions, AGENTS.md requires “preserve state on probe/send failures” and “guard duplicate actions while async transfers or offer actions are in flight.”
🤖 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 `@src/renderer/components/remote/RemoteTransferSection.tsx` around lines 244 - 249, Update the retry flow around handleRetry and the automatic retry marker to guard each transfer against concurrent retry submissions before assertReachableForTransfer. Run the reachability probe first, leaving retry state and automatic retry markers unchanged when it fails; incrementRetry and mark the retry as in flight only after the sidecar accepts the resubmission, and clear the guard when the submission completes or fails.Source: Path instructions
🧹 Nitpick comments (1)
src/renderer/components/remote/RemoteTransferSection.test.tsx (1)
161-206: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the new fetch, retry, and enable-request boundary branches.
The changed tests cover send success, unreachable peers, and listener-likely-off confirmation. The component changes also gate fetch and retry operations. Add or verify tests for blocked fetches, blocked retries, and missing, invalid, or duplicate LXMF targets. Assert that blocked operations do not invoke RNCP IPC or dispatch an enable request.
As per path instructions, AGENTS.md requires Vitest coverage for “every behavioral path,” including unreachable peers, listener-disabled fallback, and invalid/duplicate hashes.
#!/bin/bash set -euo pipefail rg -n -C 4 'fetch|retry|sendRncpRequestEnable|lxmf_peer_hash|invalid|duplicate' \ src/renderer/components/remote/RemoteTransferSection.test.tsx🤖 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 `@src/renderer/components/remote/RemoteTransferSection.test.tsx` around lines 161 - 206, Expand the RemoteTransferSection tests to cover fetch and retry operations when the destination is unreachable, plus missing, invalid, or duplicate LXMF target hashes. Verify listener-disabled fallback behavior and assert each blocked operation neither invokes RNCP IPC nor dispatches sendRncpRequestEnable; retain the existing send success and confirmation coverage.
🤖 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.
Outside diff comments:
In `@src/renderer/components/remote/RemoteTransferSection.tsx`:
- Line 379: Update the direct transfer path around peerLxmfHash so
sendRncpRequestEnable receives only a validated LXMF delivery hash; remove the
fallback to parsedHash, reject missing or invalid resolved values, and reject a
saved LXMF hash equal to parsedHash using the existing RNCP validation helpers.
If direct LXMF input is supported, preserve its input type and validate it
separately at this boundary.
- Around line 244-249: Update the retry flow around handleRetry and the
automatic retry marker to guard each transfer against concurrent retry
submissions before assertReachableForTransfer. Run the reachability probe first,
leaving retry state and automatic retry markers unchanged when it fails;
incrementRetry and mark the retry as in flight only after the sidecar accepts
the resubmission, and clear the guard when the submission completes or fails.
---
Nitpick comments:
In `@src/renderer/components/remote/RemoteTransferSection.test.tsx`:
- Around line 161-206: Expand the RemoteTransferSection tests to cover fetch and
retry operations when the destination is unreachable, plus missing, invalid, or
duplicate LXMF target hashes. Verify listener-disabled fallback behavior and
assert each blocked operation neither invokes RNCP IPC nor dispatches
sendRncpRequestEnable; retain the existing send success and confirmation
coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 23b1fe08-5b2b-4c60-8e0e-62e85fac28ad
📒 Files selected for processing (5)
src/renderer/components/remote/ChatDmRncpOfferBanner.test.tsxsrc/renderer/components/remote/ChatDmRncpOfferBanner.tsxsrc/renderer/components/remote/RemoteTransferSection.test.tsxsrc/renderer/components/remote/RemoteTransferSection.tsxsrc/renderer/lib/ensureRncpDestinationReachable.ts
Require a validated LXMF delivery hash for enable requests, and only bump retry state after the sidecar accepts a resubmit that passed the reachability probe.
|
@coderabbitai resume @coderabbitai review Please re-review including commits after the pause (offer-action guard, hash helper, enable-request/retry gates). |
|
✅ Action performedReviews resumed. Review finished.
|
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/components/remote/RemoteTransferSection.tsx (1)
101-117: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix the toast message and silence it during unattended auto-retry.
assertReachableForTransfershows the samepeerUnreachabletoast for bothpeerUnreachableandlistenerLikelyOffwheneveropts.promptEnableisfalse. During retry, this tells the user "the peer may be offline" even when the LXMF probe succeeded and only the RNCP listener is off. Use a status-specific message so retry failures accurately reflect the cause.Separately,
handleRetryis invoked automatically by the auto-retry effect (Lines 301-311) on mount whenever a failed transfer withretryArgsexists andsettings.autoRetryTransferistrue(the default, perDEFAULT_REMOTE_SETTINGS). BecauseassertReachableForTransferalways callsaddToaston non-reachable results, simply rendering this component with a pre-existing failed transfer triggers a background network probe and an error toast without any user action. Add asilentoption so automated retries can suppress the toast, and let only user-initiated retries and sends surface it.🐛 Proposed fix
const assertReachableForTransfer = useCallback( - async (destinationHash: string, opts: { promptEnable: boolean }): Promise<boolean> => { + async ( + destinationHash: string, + opts: { promptEnable: boolean; silent?: boolean }, + ): Promise<boolean> => { const reach = await ensureRncpDestinationReachable({ destinationHash, lxmfPeerHash: resolveLxmfPeerHash(destinationHash), }); if (reach.status === 'reachable') return true; if (reach.status === 'listenerLikelyOff' && opts.promptEnable) { setEnableRequestConfirmOpen(true); return false; } - addToast(t('reticulumRemote.transfer.peerUnreachable'), 'error'); + if (!opts.silent) { + addToast( + reach.status === 'listenerLikelyOff' + ? t('reticulumRemote.transfer.listenerLikelyOffBody') + : t('reticulumRemote.transfer.peerUnreachable'), + 'error', + ); + } return false; }, [addToast, resolveLxmfPeerHash, t], );Then pass
silent: truefrom the automatic-retry call path (e.g., a parameter onhandleRetrydistinguishing manual clicks from the auto-retry effect) while keeping manual retry and send/fetch calls non-silent.🤖 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 `@src/renderer/components/remote/RemoteTransferSection.tsx` around lines 101 - 117, Update assertReachableForTransfer to choose a status-specific unreachable message, distinguishing listenerLikelyOff from peerUnreachable, and add a silent option that suppresses toast emission. Thread this option through handleRetry and pass silent: true from the automatic-retry effect, while keeping manual retry and send/fetch paths non-silent.
🟡 Other comments (1)
src/renderer/components/remote/RemoteTransferSection.tsx-383-407 (1)
383-407: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a distinct error message for saved LXMF peer hash validation failures.
When
savedForDestexists but itslxmf_peer_hashis missing, malformed, or equal toparsedHash, the code showst('reticulumRemote.errors.invalidAddress'). Per the test assertions, this renders as "Enter a valid 32-character hex destination hash." That message tells the user to fix the destination-hash input field, but the actual problem is the saved LXMF peer hash in the address book, which this field does not edit. A user who re-types a correctly formatted destination hash still cannot resolve this error. Show a message that identifies the saved-peer-hash problem instead.♻️ Proposed fix
if (savedForDest) { const peer = savedForDest.lxmf_peer_hash?.trim().toLowerCase() ?? ''; if (!isRncpHexHash(peer) || peer === parsedHash) { - addToast(t('reticulumRemote.errors.invalidAddress'), 'error'); + addToast(t('reticulumRemote.errors.invalidSavedPeerHash'), 'error'); return; } peerLxmfHash = peer; }🤖 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 `@src/renderer/components/remote/RemoteTransferSection.tsx` around lines 383 - 407, Update the validation branch inside handleRequestEnable for an existing savedForDest so missing, malformed, or destination-duplicate lxmf_peer_hash values use a distinct saved-peer-hash error translation instead of invalidAddress. Leave the destination-field validation and direct-hash fallback behavior unchanged.
🤖 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.
Outside diff comments:
In `@src/renderer/components/remote/RemoteTransferSection.tsx`:
- Around line 101-117: Update assertReachableForTransfer to choose a
status-specific unreachable message, distinguishing listenerLikelyOff from
peerUnreachable, and add a silent option that suppresses toast emission. Thread
this option through handleRetry and pass silent: true from the automatic-retry
effect, while keeping manual retry and send/fetch paths non-silent.
---
Other comments:
In `@src/renderer/components/remote/RemoteTransferSection.tsx`:
- Around line 383-407: Update the validation branch inside handleRequestEnable
for an existing savedForDest so missing, malformed, or destination-duplicate
lxmf_peer_hash values use a distinct saved-peer-hash error translation instead
of invalidAddress. Leave the destination-field validation and direct-hash
fallback behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 9bfe9c14-b131-428a-8f1c-574572d1c73d
📒 Files selected for processing (5)
src/renderer/components/remote/ChatDmRncpOfferBanner.test.tsxsrc/renderer/components/remote/ChatDmRncpOfferBanner.tsxsrc/renderer/components/remote/RemoteTransferSection.test.tsxsrc/renderer/components/remote/RemoteTransferSection.tsxsrc/renderer/lib/ensureRncpDestinationReachable.ts
1 participant
Summary
pendingOffers.Test plan
rncp.sendnot invoked.pendingOffers > 0.Summary by CodeRabbit