Skip to content

fix(reticulum): align RMAP toggles on stack restart and persist prefs #740

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rinchen merged 2 commits into from
Jul 31, 2026
Merged

fix(reticulum): align RMAP toggles on stack restart and persist prefs #740

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b9918e2
fix(reticulum): align RMAP toggles on stack restart and persist prefs
rinchen Jul 31, 2026
ff3009d
fix(reticulum): harden RMAP restart refresh and add coverage
rinchen Jul 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions src/main/database.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,9 +413,33 @@ describe('app_settings table + message retention defaults (schema sync)', () =>
expect(INDEX_SOURCE).toContain('meshcoreRoomSync:');
expect(INDEX_SOURCE).toContain('meshcoreRoomLastPost:');
expect(INDEX_SOURCE).toContain('meshcoreRoomCredential:');
expect(INDEX_SOURCE).toContain('reticulumRmapAnnounceIntervalMin');
expect(INDEX_SOURCE).toContain('reticulumRmapReachableOn');
expect(INDEX_SOURCE).toContain('reticulumRmapHeightMeters');
expect(INDEX_SOURCE).not.toContain('reticulumRmapNotAllowed');
expect(INDEX_SOURCE).toMatch(/key not allowed/);
expect(INDEX_SOURCE).toMatch(/INSERT OR REPLACE INTO app_settings\(key, value\) VALUES/);
});

it('appSettings:set allowlists RMAP prefs for SQLite persistence', () => {
// Source-level: Electron-bound IPC cannot be exercised here; assert write path + allowlist.
const allowListBlock = INDEX_SOURCE.slice(
INDEX_SOURCE.indexOf('APP_SETTINGS_ALLOWED_KEYS'),
INDEX_SOURCE.indexOf('APP_SETTINGS_MAX_VALUE_LENGTH'),
);
expect(allowListBlock).toContain("'reticulumRmapAnnounceIntervalMin'");
expect(allowListBlock).toContain("'reticulumRmapReachableOn'");
expect(allowListBlock).toContain("'reticulumRmapHeightMeters'");
expect(allowListBlock).not.toContain("'reticulumRmapNotAllowed'");
expect(INDEX_SOURCE).toMatch(
/isAppSettingsKeyAllowed\(key\)[\s\S]*?throw new Error\('appSettings:set: key not allowed'\)/,
);
expect(INDEX_SOURCE).toMatch(
/\.prepareOnce\('INSERT OR REPLACE INTO app_settings\(key, value\) VALUES \(\?, \?\)'\)/,
);
expect(INDEX_SOURCE).toMatch(/SELECT key, value FROM app_settings/);
});
Comment on lines +416 to +441

@coderabbitai coderabbitai Bot Jul 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Test the IPC persistence behavior.

These assertions only inspect src/main/index.ts text. They do not invoke appSettings:set or verify a SQLite write and read. A broken handler can keep these source strings and still fail to persist RMAP settings.

Capture the registered IPC handler in the Electron mock. Invoke it for each allowed RMAP key. Assert the persisted values through the existing SQLite test fixture. Keep the rejected-key assertion.

As per coding guidelines: “Ship a passing test for behavioral changes.” As per path instructions: “Prefer behavioral assertions; skip style-only test nits.”

🤖 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/main/database.test.ts` around lines 416 - 441, Replace the
source-text-only assertions in the appSettings:set test with behavioral
coverage: capture the registered IPC handler from the Electron mock, invoke it
for each allowed RMAP key, and verify the corresponding values are written and
readable through the existing SQLite fixture. Retain the rejected-key assertion
while ensuring the test exercises the actual persistence path rather than only
checking INDEX_SOURCE strings.

Sources: Coding guidelines, Path instructions


it('appSettings:set rejects oversized values to bound DB writes', () => {
expect(INDEX_SOURCE).toContain('APP_SETTINGS_MAX_VALUE_LENGTH');
expect(INDEX_SOURCE).toContain('appSettingsMaxValueLengthForKey');
Expand Down
3 changes: 3 additions & 0 deletions src/main/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3455,6 +3455,9 @@ const APP_SETTINGS_ALLOWED_KEYS: ReadonlySet<string> = new Set([
'reduceMotion',
'alwaysShowMessageActions',
'reticulumAutostart',
'reticulumRmapAnnounceIntervalMin',
'reticulumRmapReachableOn',
'reticulumRmapHeightMeters',
Comment on lines +3458 to +3460

@coderabbitai coderabbitai Bot Jul 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Add persistence coverage for the three allowlisted keys.

Add a passing test that writes and reads reticulumRmapAnnounceIntervalMin, reticulumRmapReachableOn, and reticulumRmapHeightMeters through the existing SQLite-backed app-settings path. Also verify that an unlisted RMAP key remains rejected.

As per coding guidelines: “Ship a passing test for behavioral changes.”

As per path instructions: “Add or update behavioral tests, particularly for confirm/cancel paths and settings persistence.”

🧰 Tools
🪛 ast-grep (0.45.0)

[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from 'child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').

(detect-child-process-typescript)

🤖 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/main/index.ts` around lines 3458 - 3460, Add behavioral coverage for the
allowlisted RMAP keys in the existing SQLite-backed app-settings persistence
tests: write and read reticulumRmapAnnounceIntervalMin,
reticulumRmapReachableOn, and reticulumRmapHeightMeters, and assert that an
unlisted RMAP key is rejected. Reuse the existing settings persistence test path
and helpers.

Sources: Coding guidelines, Path instructions

/** Legacy blob; prefer meshtasticRemoteAdminKey:<nodeNum> per-node keys. */
'meshtasticRemoteAdminKeyByNode',
]);
Expand Down
148 changes: 145 additions & 3 deletions src/renderer/components/reticulum/ReticulumInterfacesPanel.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import { render, screen, waitFor } from '@testing-library/react';
import { render, screen, waitFor, within } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import { beforeEach, describe, expect, it, vi } from 'vitest';
import { axe } from 'vitest-axe';

import { hydrateAxeThemeColors } from '@/renderer/lib/a11yTestHelpers';
import { GPS_SETTINGS_STORAGE_KEY } from '@/renderer/lib/gpsSource';
import {
buildDefaultHubAddRequest,
RETICULUM_DEFAULT_HUB_PRESETS,
Expand All @@ -20,13 +21,18 @@ vi.mock('react-i18next', () => ({
useTranslation: () => ({ t: (key: string) => key }),
}));

const { addToastMock, restartStackMock } = vi.hoisted(() => ({
addToastMock: vi.fn(),
restartStackMock: vi.fn().mockResolvedValue(undefined),
}));

vi.mock('@/renderer/components/Toast', () => ({
useToast: () => ({ addToast: vi.fn() }),
useToast: () => ({ addToast: addToastMock }),
}));

vi.mock('@/renderer/lib/sessions/reticulumSession', () => ({
tryGetReticulumSession: () => ({
restartStack: vi.fn().mockResolvedValue(undefined),
restartStack: restartStackMock,
}),
}));

Expand All @@ -43,8 +49,31 @@ const defaultProps = {
onBeginBleConnectGrace: vi.fn(),
};

const rmapCapableRnode: ReticulumInterfaceRow = {
id: 'rnode-41f4',
name: 'RNode 41F4',
type: 'rnode',
enabled: true,
status: 'up',
serial_port: 'ble://eccf2847-e1fd-3f5f-0811-064db1639a3d',
discoverable: false,
};

const rmapWorldHub: ReticulumInterfaceRow = {
id: 'rmap-world',
name: 'RMAP World',
type: 'tcp',
enabled: true,
status: 'up',
host: 'rmap.world',
port: 4242,
};

describe('ReticulumInterfacesPanel', () => {
beforeEach(() => {
addToastMock.mockClear();
restartStackMock.mockClear();
localStorage.removeItem(GPS_SETTINGS_STORAGE_KEY);
useConnectionStore.setState({ connections: {} });
useIdentityStore.setState({ identities: {}, activeIdentityId: null });
window.electronAPI.reticulum.proxyPost = vi.fn().mockResolvedValue({ ok: true });
Expand All @@ -64,6 +93,16 @@ describe('ReticulumInterfacesPanel', () => {
if (path === '/api/v1/config/audit') {
return Promise.resolve({ issues: [] });
}
if (path === '/api/v1/stack/settings') {
return Promise.resolve({
enable_transport: true,
share_instance: false,
loglevel: 4,
});
}
if (path === '/api/v1/interfaces') {
return Promise.resolve({ interfaces: [rmapCapableRnode, rmapWorldHub] });
}
return Promise.resolve({});
});
});
Expand Down Expand Up @@ -1110,4 +1149,107 @@ describe('ReticulumInterfacesPanel', () => {
).toBeInTheDocument();
expect(screen.queryByText('identity not configured')).not.toBeInTheDocument();
});

describe('RMAP discoverable toggle restart confirm', () => {
beforeEach(() => {
localStorage.setItem(
GPS_SETTINGS_STORAGE_KEY,
JSON.stringify({ staticLat: 40.19444, staticLon: -105.06722 }),
);
});

it('refreshes then shows restart confirm; confirm restarts the stack', async () => {
const user = userEvent.setup();
const onRefresh = vi.fn().mockResolvedValue(undefined);

render(
<ReticulumInterfacesPanel
{...defaultProps}
onRefresh={onRefresh}
interfaces={[rmapCapableRnode, rmapWorldHub]}
/>,
);

await user.click(
screen.getByRole('checkbox', {
name: 'connectionPanel.reticulumInterfaces.rmapDiscoverableAria',
}),
);

await waitFor(() => {
expect(onRefresh).toHaveBeenCalled();
});
expect(await screen.findByText('reticulumRmapDiscovery.restartTitle')).toBeInTheDocument();
expect(restartStackMock).not.toHaveBeenCalled();

const dialog = screen.getByRole('alertdialog');
await user.click(
within(dialog).getByRole('button', { name: 'reticulumRmapDiscovery.restartConfirm' }),
);

await waitFor(() => {
expect(restartStackMock).toHaveBeenCalled();
});
expect(screen.queryByText('reticulumRmapDiscovery.restartTitle')).not.toBeInTheDocument();
});

it('cancel closes restart confirm and shows the restart hint', async () => {
const user = userEvent.setup();
const onRefresh = vi.fn().mockResolvedValue(undefined);

render(
<ReticulumInterfacesPanel
{...defaultProps}
onRefresh={onRefresh}
interfaces={[rmapCapableRnode, rmapWorldHub]}
/>,
);

await user.click(
screen.getByRole('checkbox', {
name: 'connectionPanel.reticulumInterfaces.rmapDiscoverableAria',
}),
);

expect(await screen.findByText('reticulumRmapDiscovery.restartTitle')).toBeInTheDocument();

const dialog = screen.getByRole('alertdialog');
await user.click(within(dialog).getByRole('button', { name: 'common.cancel' }));

expect(screen.queryByText('reticulumRmapDiscovery.restartTitle')).not.toBeInTheDocument();
expect(
screen.getByText('connectionPanel.reticulumInterfaces.restartStackHint'),
).toBeInTheDocument();
expect(restartStackMock).not.toHaveBeenCalled();
});

it('still shows restart confirm when onRefresh rejects after a successful toggle', async () => {
const user = userEvent.setup();
const onRefresh = vi.fn().mockRejectedValue(new Error('refresh failed'));

render(
<ReticulumInterfacesPanel
{...defaultProps}
onRefresh={onRefresh}
interfaces={[rmapCapableRnode, rmapWorldHub]}
/>,
);

await user.click(
screen.getByRole('checkbox', {
name: 'connectionPanel.reticulumInterfaces.rmapDiscoverableAria',
}),
);

expect(await screen.findByText('reticulumRmapDiscovery.restartTitle')).toBeInTheDocument();
expect(addToastMock).toHaveBeenCalledWith(
'connectionPanel.reticulumInterfaces.rmapEnableSuccess',
'success',
);
expect(addToastMock).not.toHaveBeenCalledWith(
expect.stringContaining('rmapToggleFailed'),
'error',
);
});
});
});
34 changes: 30 additions & 4 deletions src/renderer/components/reticulum/ReticulumInterfacesPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,7 @@ export function ReticulumInterfacesPanel({
} | null>(null);
const [editingInterface, setEditingInterface] = useState<ReticulumInterfaceRow | null>(null);
const [restartStackHint, setRestartStackHint] = useState(false);
const [showRmapRestartConfirm, setShowRmapRestartConfirm] = useState(false);
const [addingDefaultHubs, setAddingDefaultHubs] = useState(false);
const [rmapToggleBusyId, setRmapToggleBusyId] = useState<string | null>(null);

Expand Down Expand Up @@ -701,8 +702,13 @@ export function ReticulumInterfacesPanel({
});
if (synced) {
addToast(t('connectionPanel.reticulumRmap.syncSuccess'), 'success');
setRestartStackHint(true);
await onRefresh();
try {
await onRefresh();
} catch (e) {
// catch-no-log-ok refresh failure must not mask successful RMAP sync
console.debug('[ReticulumInterfacesPanel] rmap sync refresh ' + errLikeToLogString(e));
}
setShowRmapRestartConfirm(true);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
} catch (e) {
addToast(t('connectionPanel.reticulumRmap.syncFailed'), 'error');
Expand Down Expand Up @@ -736,8 +742,13 @@ export function ReticulumInterfacesPanel({
: t('connectionPanel.reticulumInterfaces.rmapDisableSuccess', { name: iface.name }),
'success',
);
setRestartStackHint(true);
await onRefresh();
try {
await onRefresh();
} catch (e) {
// catch-no-log-ok refresh failure must not mask successful RMAP toggle
console.debug('[ReticulumInterfacesPanel] rmap toggle refresh ' + errLikeToLogString(e));
}
setShowRmapRestartConfirm(true);
} catch (e) {
if (e instanceof ReticulumRmapGpsRequiredError) {
addToast(t('reticulumRmapDiscovery.gpsMissingWarning'), 'error');
Expand Down Expand Up @@ -885,6 +896,21 @@ export function ReticulumInterfacesPanel({
}}
/>
) : null}
{showRmapRestartConfirm ? (
<ConfirmModal
title={t('reticulumRmapDiscovery.restartTitle')}
message={t('reticulumRmapDiscovery.restartBody')}
confirmLabel={t('reticulumRmapDiscovery.restartConfirm')}
onConfirm={() => {
setShowRmapRestartConfirm(false);
void restartStackForInterfaceChange();
}}
onCancel={() => {
setShowRmapRestartConfirm(false);
setRestartStackHint(true);
}}
/>
) : null}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
<ReticulumInterfaceDevicePickerModal
open={devicePicker.open}
mode={devicePicker.mode}
Expand Down