Conversation
Close floating promise and sender-validation holes, share must-match helpers that were drifting, localize overlooked UI strings, and correct factual docs mismatches from the obvious-wrong audit.
📝 WalkthroughWalkthroughThis PR centralizes shared settings, MQTT TLS, GPS, parsing, and deduplication helpers. It adds IPC sender validation and error sanitization, updates game draw-offer states, improves temporary-directory cleanup, localizes renderer labels, and refreshes documentation. ChangesIPC and error handling
Reticulum game draw offers
Shared transport and runtime utilities
Tooling and documentation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
rinchen
marked this pull request as draft
August 7, 2026 01:04
Gate Accept/Decline and banners on metadata.draw_offered_by so offering a draw no longer shows the opponent-offer UI.
rinchen
changed the title
Expand helper matrix, legacy/self/opponent GamesPanel regressions, board inactive/legacy banners, and games.update store preservation.
Lock in shared mqttTls/appSettings prefixes, GPS/last-connection helpers, S&F parse, updater sender checks, and runtime source contracts from the audit fixes.
rinchen
marked this pull request as ready for review
August 7, 2026 01:28
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (2)
src/renderer/components/ChatPanel.tsx-2530-2536 (1)
2530-2536: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLimit
Unknownlocalization to MeshCore.At Line [2536], the condition runs for every protocol.
nodeDisplayNameinsrc/renderer/lib/nodeLongNameOrHex.tsLines [4]-[12] can return a legitimateUnknownname for Meshtastic or Reticulum, so this code replaces that name withcommon.unknown. Scope the replacement to MeshCore. Add tests for the MeshCore sentinel and a non-MeshCore node namedUnknown.Suggested fix
- const displaySenderName = - rawSenderName === 'Unknown' ? t('common.unknown') : rawSenderName; + const displaySenderName = + protocol === 'meshcore' && rawSenderName === 'Unknown' + ? t('common.unknown') + : rawSenderName;🤖 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/ChatPanel.tsx` around lines 2530 - 2536, Update the displaySenderName condition in the sender-name construction to localize the literal “Unknown” only when protocol is MeshCore; preserve a legitimate “Unknown” name for Meshtastic and Reticulum. Add tests covering MeshCore sentinel localization and non-MeshCore nodes named “Unknown”.Sources: Coding guidelines, Path instructions
scripts/check-insecure-temp-files.test.mjs-65-75 (1)
65-75: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winCover the asynchronous
mkdirrule.The checker now registers both
mkdirSyncandmkdirinscripts/check-insecure-temp-files.mjsLines 30-32. This test covers onlymkdirSync. Add a behavioral case forfs.mkdir(...).As per coding guidelines and path instructions, behavioral changes must include a passing test before the task is considered complete.
Suggested regression test
+ it('fails on mkdir to predictable tmpdir path', () => { + const result = runCheckOnSnippet(` +import fs from 'fs'; +import os from 'os'; +import path from 'path'; +const dir = path.join(os.tmpdir(), 'mesh-client-appimage-x64-2'); +fs.mkdir(dir, { recursive: true }, () => {}); +`); + expect(result.status).toBe(1); + expect(result.stderr).toMatch(/insecure-temporary-file|predictable/); + });🤖 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 `@scripts/check-insecure-temp-files.test.mjs` around lines 65 - 75, Add a behavioral test alongside the existing “fails on mkdirSync to predictable tmpdir path” case that runs a snippet using asynchronous fs.mkdir on a predictable os.tmpdir()-derived path and asserts the checker exits with status 1 and reports the insecure-temporary-file or predictable diagnostic. Reuse the existing runCheckOnSnippet helper and test structure.Sources: Coding guidelines, Path instructions
🧹 Nitpick comments (1)
src/renderer/hooks/useSendMessage.test.ts (1)
381-383: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the repository console-warning test helper.
Replace
vi.spyOn(console, 'warn')withmockConsoleWarnorwithMockedConsoleWarn. The renderer test rules prohibit ad-hocconsole.warnspies.🤖 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/hooks/useSendMessage.test.ts` around lines 381 - 383, Update the test case “SoftAP dead-accepted: marks failed when live reopen yields no handle” to use the repository’s mockConsoleWarn or withMockedConsoleWarn helper instead of vi.spyOn(console, 'warn'), preserving the existing warning assertions and cleanup behavior.Source: Coding guidelines
🤖 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 `@scripts/test-linux-appimage-reticulum-sidecar.mjs`:
- Around line 199-210: The AppImage extraction flow must not delete and recreate
the unique directory returned by mkdtempSync. Update prepareAppImageExtractDir
and its callers around extractAppImageWithUnsquashfs or spawnSync so setup only
creates needed children under extractDir, or is performed atomically, preserving
extraction and cleanup in the original unique directory.
In `@scripts/test-win-nsis-install.mjs`:
- Around line 171-176: Update probe7zExtract to use the existing directory
created by mkdtempSync without removing or recreating it before invoking 7-Zip.
Preserve the unique temporary-directory flow in the surrounding probeDir setup
and retain cleanup in the finally block.
- Around line 184-185: Wrap the installer execution, validation, and
failure-reporting flow using workDir and logPath in a try/finally block, and
remove workDir in finally after all diagnostic logs have been read. Preserve the
existing success and error behavior while ensuring the mkdtemp-created directory
is cleaned up for every outcome.
In `@src/main/tak-server-manager.ts`:
- Around line 79-83: Add a regression test covering the error-handling path that
invokes sanitizeLogMessage, using an error message containing CR or LF
characters. Assert that the sanitized value is used consistently for console
logging, the status.error field, and the emitted error event.
In `@src/renderer/stores/nomadPageViewerStore.ts`:
- Around line 559-560: Replace String(err) with errLikeToLogString(err) in the
pageReadyToast dynamic-import rejection handler in
src/renderer/stores/nomadPageViewerStore.ts:559-560. Update
src/renderer/runtime/auditHardening.contract.test.ts:58-63 to assert that this
handler uses errLikeToLogString(err); both changes target the existing
rejection-handler contract.
---
Other comments:
In `@scripts/check-insecure-temp-files.test.mjs`:
- Around line 65-75: Add a behavioral test alongside the existing “fails on
mkdirSync to predictable tmpdir path” case that runs a snippet using
asynchronous fs.mkdir on a predictable os.tmpdir()-derived path and asserts the
checker exits with status 1 and reports the insecure-temporary-file or
predictable diagnostic. Reuse the existing runCheckOnSnippet helper and test
structure.
In `@src/renderer/components/ChatPanel.tsx`:
- Around line 2530-2536: Update the displaySenderName condition in the
sender-name construction to localize the literal “Unknown” only when protocol is
MeshCore; preserve a legitimate “Unknown” name for Meshtastic and Reticulum. Add
tests covering MeshCore sentinel localization and non-MeshCore nodes named
“Unknown”.
---
Nitpick comments:
In `@src/renderer/hooks/useSendMessage.test.ts`:
- Around line 381-383: Update the test case “SoftAP dead-accepted: marks failed
when live reopen yields no handle” to use the repository’s mockConsoleWarn or
withMockedConsoleWarn helper instead of vi.spyOn(console, 'warn'), preserving
the existing warning assertions and cleanup behavior.
🪄 Autofix
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: 72b646eb-0c21-4816-908b-83e239efaa8f
⛔ 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 (59)
AGENTS.mdCONTRIBUTING.mddocs/accessibility-checklist.mddocs/diagnostics.mddocs/reticulum-sidecar-ipc.mdscripts/check-insecure-temp-files.mjsscripts/check-insecure-temp-files.test.mjsscripts/check-log-injection.mjsscripts/test-linux-appimage-reticulum-sidecar.mjsscripts/test-win-nsis-install.mjssrc/main/database.test.tssrc/main/index.contract.test.tssrc/main/index.ipc-security.test.tssrc/main/index.tssrc/main/meshcore-mqtt-adapter.tssrc/main/mqtt-manager.tssrc/main/tak-server-manager.tssrc/main/updater.contract.test.tssrc/main/updater.tssrc/renderer/components/ChatPanel.tsxsrc/renderer/components/GamesPanel.test.tsxsrc/renderer/components/GamesPanel.tsxsrc/renderer/components/NodeListPanel.tsxsrc/renderer/components/RawPacketLogPanel.tsxsrc/renderer/components/games/ChessBoard.test.tsxsrc/renderer/components/games/ChessBoard.tsxsrc/renderer/components/games/TicTacToeBoard.test.tsxsrc/renderer/components/games/TicTacToeBoard.tsxsrc/renderer/hooks/openMeshCoreTransport.test.tssrc/renderer/hooks/useSendMessage.test.tssrc/renderer/lib/drivers/ConnectionDriver.test.tssrc/renderer/lib/gpsSource.test.tssrc/renderer/lib/gpsSource.tssrc/renderer/lib/lastConnectionStorage.test.tssrc/renderer/lib/lastConnectionStorage.tssrc/renderer/lib/meshcoreRepeaterCredentialStorage.tssrc/renderer/lib/meshcoreRoomCredentialStorage.tssrc/renderer/lib/meshcoreRoomSyncStorage.tssrc/renderer/lib/meshtasticBacklogUtils.tssrc/renderer/lib/meshtasticMessageDedup.test.tssrc/renderer/lib/meshtasticMessageDedup.tssrc/renderer/lib/meshtasticRemoteAdminKeyStorage.tssrc/renderer/lib/mqttTls.tssrc/renderer/lib/parseStoredJson.tssrc/renderer/lib/reticulum/reticulumGamesMetadata.test.tssrc/renderer/lib/reticulum/reticulumGamesMetadata.tssrc/renderer/lib/timeConstants.tssrc/renderer/runtime/auditHardening.contract.test.tssrc/renderer/runtime/useMeshcoreRuntime.tssrc/renderer/runtime/useMeshtasticRuntime.tssrc/renderer/stores/nomadPageViewerStore.tssrc/renderer/stores/reticulumGamesStore.test.tssrc/shared/appSettingsKeyPrefixes.test.tssrc/shared/appSettingsKeyPrefixes.tssrc/shared/games-types.tssrc/shared/meshtasticTextMessagePayload.test.tssrc/shared/meshtasticTextMessagePayload.tssrc/shared/mqttTls.test.tssrc/shared/mqttTls.ts
Comment on lines
+79
to
+83
| const safe = sanitizeLogMessage(msg); | ||
| console.error('[TakServer]', safe); | ||
| this._status = { running: false, port: settings.port, clientCount: 0, error: safe }; | ||
| this.emit('status', this.getStatus()); | ||
| this.emit('error', msg); | ||
| this.emit('error', safe); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Add a regression test for sanitized server errors.
Test an error message that contains CR or LF characters. Verify that the logged value, status error, and emitted error value are sanitized.
As per coding guidelines, “Behavioral changes must include a passing test before the task is considered complete.” As per path instructions, “pair every behavioral change with passing tests.”
🤖 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/tak-server-manager.ts` around lines 79 - 83, Add a regression test
covering the error-handling path that invokes sanitizeLogMessage, using an error
message containing CR or LF characters. Assert that the sanitized value is used
consistently for console logging, the status.error field, and the emitted error
event.
Sources: Coding guidelines, Path instructions
Preserve mkdtemp extract dirs in packaging smokes, localize MeshCore Unknown only, and tighten TAK/nomad error logging tests.
Mirror Room exclusion so Message, quick DM, unread, and History no longer treat repeaters as chat peers. Sensors stay DM-eligible. Fixes #809
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
1 participant
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
hydrateMeshtasticMessagesFromDb,meshcore:openJsonFile,update:*,app:getProcessUptimeSec,device-connected/device-disconnected), sanitize TAK/updater error surfaces, and usemkdtempin packaging smoke scripts (scanner also flagsmkdirSync).mqttUsesTls, Store & Forward parse, Meshtastic 10m dedup window,app_settingskey prefixes) and route LoRa runtimes throughlastConnectionStorage/gpsSource.skipDiscoverSelftests, and correct factual docs mismatches (IPC channel names, CONTRIBUTING/Vitest scope, diagnostics paths).metadata.draw_offered_byso offering a draw shows a waiting state instead of “Your opponent offered a draw.”Test plan
pnpm exec vitest runfor IPC security, SoftAP send, ConnectionDriver/skipDiscoverSelf, MQTT TLS, backlog/dedup, insecure-temp scannerpnpm exec vitest runfor GamesPanel / ChessBoard / TicTacToeBoard /reticulumGamesMetadatadraw-offer casespnpm exec vitest runformeshcoreUtils/ NodeDetailModal / NodeListPanel repeater DM exclusionpnpm run check:i18n(hard pass after fixing RU HTML entities)--changedvsorigin/main