From e959b6e1471c6fee91b3040419aeba5a58a81a98 Mon Sep 17 00:00:00 2001 From: Eli Fayerman Date: Tue, 28 Jul 2026 10:48:50 +0900 Subject: [PATCH 1/2] Fix SonarCloud code smells: logging fallback, dup import, dedup camera formatting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses 8 of the 19 open SonarCloud issues — the ones with real value. - log.ts: the circular-ref fallback used String(extra), which renders objects as "[object Object]" — destroying diagnostic value in exactly the case the fallback exists for (the logger is called from catch blocks). Use util.inspect, which handles cycles and prints readable structure. (S6551) - config.ts: merge the duplicated 'node:path' import. (S3863 x2) - ring.ts: export a shared formatCameraList() helper, removing the `cameras.map(c => `${c.name} (#${c.id})`).join(', ')` duplication between ring.ts and cli.ts and the nested template literals in both. (S4624 x3) - ring.ts: `!raw || !raw.refreshToken` -> `!raw?.refreshToken`. (S6582) - cli.ts: extract runList()/runRecord() from main(), which was a dispatcher with both command bodies inlined. Cognitive complexity 23 -> under 15. (S3776) - test/logic.test.mjs: `resolveLast && resolveLast()` -> `resolveLast?.()`. (S6582) No behavior change other than the improved log output on circular payloads. Deliberately not fixed (should be resolved as Won't Fix in SonarCloud): - S7785 "prefer top-level await" x4 — all are the entrypoint `main().catch(...)` pattern, which is what gives these CLIs a clean error message and a controlled exit code. Top-level await changes that to an unhandled-rejection trace. - S7735 "unexpected negated condition" x4 — all guard clauses / early returns. - S7776 (test) — use a Set instead of includes() on a 3-element array. Also left alone: S3776 on verify.ts:23 (complexity 17). That main() is a linear 4-step diagnostic script where the numbered narrative is the readability; splitting it would satisfy the gate but not improve the code. --- src/cli.ts | 69 ++++++++++++++++++++++++--------------------- src/config.ts | 3 +- src/log.ts | 8 ++++-- src/ring.ts | 10 +++++-- test/logic.test.mjs | 2 +- 5 files changed, 53 insertions(+), 39 deletions(-) diff --git a/src/cli.ts b/src/cli.ts index e952c1d..5965de1 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -1,5 +1,6 @@ -import { loadConfig } from './config.js'; -import { createRingApi } from './ring.js'; +import type { RingApi } from 'ring-client-api'; +import { loadConfig, type AppConfig } from './config.js'; +import { createRingApi, formatCameraList } from './ring.js'; import { recordClip } from './recorder.js'; import { log } from './log.js'; @@ -21,36 +22,9 @@ async function main(): Promise { try { if (command === 'list') { - const cameras = await api.getCameras(); - if (cameras.length === 0) { - log.warn('No cameras found on this account.'); - return; - } - log.info(`Found ${cameras.length} camera(s):`); - for (const c of cameras) { - const kind = c.isDoorbot ? 'doorbell' : 'camera'; - const battery = c.batteryLevel != null ? `${c.batteryLevel}%` : 'wired'; - process.stdout.write(` #${c.id} ${c.name} [${kind}, ${c.model}, battery: ${battery}]\n`); - } - return; - } - - if (command === 'record') { - const args = parseArgs(rest); - const seconds = args.seconds ? Number(args.seconds) : cfg.clipLengthSeconds; - if (!Number.isFinite(seconds) || seconds <= 0) throw new Error('--seconds must be a positive number'); - - const cameras = await api.getCameras(); - const match = pickCamera(cameras, args.camera); - if (!match) { - throw new Error( - `No camera matched ${args.camera ? `"${args.camera}"` : '(none specified)'}. ` + - `Available: ${cameras.map((c) => `${c.name} (#${c.id})`).join(', ')}`, - ); - } - const result = await recordClip(match, cfg, seconds); - log.info(`Done: ${result.path}`); - return; + await runList(api); + } else { + await runRecord(api, cfg, rest); } } finally { api.disconnect(); @@ -59,6 +33,37 @@ async function main(): Promise { } } +async function runList(api: RingApi): Promise { + const cameras = await api.getCameras(); + if (cameras.length === 0) { + log.warn('No cameras found on this account.'); + return; + } + log.info(`Found ${cameras.length} camera(s):`); + for (const c of cameras) { + const kind = c.isDoorbot ? 'doorbell' : 'camera'; + const battery = c.batteryLevel != null ? `${c.batteryLevel}%` : 'wired'; + process.stdout.write(` #${c.id} ${c.name} [${kind}, ${c.model}, battery: ${battery}]\n`); + } +} + +async function runRecord(api: RingApi, cfg: AppConfig, argv: string[]): Promise { + const args = parseArgs(argv); + const seconds = args.seconds ? Number(args.seconds) : cfg.clipLengthSeconds; + if (!Number.isFinite(seconds) || seconds <= 0) throw new Error('--seconds must be a positive number'); + + const cameras = await api.getCameras(); + const match = pickCamera(cameras, args.camera); + if (!match) { + const requested = args.camera ? `"${args.camera}"` : '(none specified)'; + throw new Error( + `No camera matched ${requested}. Available: ${formatCameraList(cameras)}`, + ); + } + const result = await recordClip(match, cfg, seconds); + log.info(`Done: ${result.path}`); +} + function parseArgs(argv: string[]): Record { const out: Record = {}; for (let i = 0; i < argv.length; i++) { diff --git a/src/config.ts b/src/config.ts index eb04d18..e5af440 100644 --- a/src/config.ts +++ b/src/config.ts @@ -1,7 +1,6 @@ import { readFileSync, existsSync } from 'node:fs'; -import { resolve, isAbsolute } from 'node:path'; +import { resolve, isAbsolute, dirname } from 'node:path'; import { fileURLToPath } from 'node:url'; -import { dirname } from 'node:path'; const __dirname = dirname(fileURLToPath(import.meta.url)); const PROJECT_ROOT = resolve(__dirname, '..'); diff --git a/src/log.ts b/src/log.ts index ec8c1b4..f3d1aab 100644 --- a/src/log.ts +++ b/src/log.ts @@ -2,6 +2,8 @@ * Minimal leveled logger. Writes a single line per event with an ISO timestamp * so output is greppable and friendly to `journalctl`/pm2 log capture. */ +import { inspect } from 'node:util'; + type Level = 'info' | 'warn' | 'error' | 'debug'; const DEBUG = process.env.RING_DEBUG === '1' || process.env.RING_DEBUG === 'true'; @@ -20,8 +22,10 @@ function emit(level: Level, msg: string, extra?: unknown): void { extraStr = JSON.stringify(extra); } catch { // Circular refs (some Error objects, API responses) must not crash the - // logger — it is frequently called from catch blocks. - extraStr = String(extra); + // logger — it is frequently called from catch blocks. `inspect` handles + // cycles and, unlike String(), does not collapse objects to + // "[object Object]". + extraStr = inspect(extra, { depth: 3, breakLength: Infinity }); } } stream.write(`${line} ${extraStr}\n`); diff --git a/src/ring.ts b/src/ring.ts index 7378b6b..664c02b 100644 --- a/src/ring.ts +++ b/src/ring.ts @@ -29,7 +29,7 @@ export function readToken(cfg: AppConfig): string { `Token file ${cfg.tokenPath} is unreadable or not valid JSON (${(err as Error).message}). Re-run \`npm run auth\`.`, ); } - if (!raw || !raw.refreshToken) { + if (!raw?.refreshToken) { throw new Error(`Token file ${cfg.tokenPath} is missing "refreshToken". Re-run \`npm run auth\`.`); } return raw.refreshToken; @@ -81,6 +81,12 @@ export function createRingApi(cfg: AppConfig): RingApi { return api; } +/** Render cameras as `Name (#id), Other (#id)` for error and warning messages. */ +export function formatCameraList(cameras: readonly Pick[]): string { + if (cameras.length === 0) return '(none)'; + return cameras.map((c) => `${c.name} (#${c.id})`).join(', '); +} + /** Get cameras filtered by the config `cameras` selector. */ export async function getSelectedCameras(api: RingApi, cfg: AppConfig): Promise { const all = await api.getCameras(); @@ -88,7 +94,7 @@ export async function getSelectedCameras(api: RingApi, cfg: AppConfig): Promise< if (selected.length === 0) { log.warn( `No cameras matched the config filter ${JSON.stringify(cfg.cameras)}. ` + - `Found: ${all.map((c) => `${c.name} (#${c.id})`).join(', ') || '(none)'}`, + `Found: ${formatCameraList(all)}`, ); } return selected; diff --git a/test/logic.test.mjs b/test/logic.test.mjs index b3a98e2..d3a7f25 100644 --- a/test/logic.test.mjs +++ b/test/logic.test.mjs @@ -42,7 +42,7 @@ const cfg = (over = {}) => ({ clipLengthSeconds: 10, recordOnMotion: true, recor function spy() { const calls = []; let resolveLast; const fn = (camera, c, seconds) => { calls.push({ seconds }); return new Promise((res) => { resolveLast = () => res({ camera: camera.name, path: 'x', bytes: 1, seconds }); }); }; - return { fn, calls, finish: () => resolveLast && resolveLast() }; + return { fn, calls, finish: () => resolveLast?.() }; } { From 688f6361c7c53006eabcf422c0c110759572b084 Mon Sep 17 00:00:00 2001 From: Eli Fayerman Date: Tue, 28 Jul 2026 10:56:14 +0900 Subject: [PATCH 2/2] Extract verify.ts steps; fix negated-condition-with-else smells MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit verify.ts (S3776, complexity 17 -> under 15): main() was a linear 4-step script with every step inlined. Extracted stepAuthAndList/stepCapture/ stepProbe/stepWatchMotion. The numbered narrative is preserved — main() now reads as the four steps in order, and the shared `step()` helper is module-level so each function still logs its own number. Steps 3 and 4 return a failure count that main() tallies, replacing the shared mutable `failures` counter they used to increment. Also fixes the four S7735 issues I previously judged as won't-fix. That judgment was wrong: none of them are guard clauses, they are all negated conditions with an else branch, where inverting genuinely reads better. - log.ts:14 `if (extra !== undefined) {...} else {...}` -> test `=== undefined` first - cli.ts:32 `batteryLevel != null ? `${n}%` : 'wired'` -> `== null ? 'wired' : `${n}%`` - verify.ts same battery ternary, and `if (!hasVideo) {fail} else {ok}` -> `if (hasVideo)` Adds local Config/Clip type aliases and uses them for waitForTriggeredRecording, replacing the inline ReturnType. No behavior change. npm test: 15 passed, 0 failed. --- src/cli.ts | 2 +- src/log.ts | 6 +-- src/verify.ts | 109 ++++++++++++++++++++++++++++++-------------------- 3 files changed, 70 insertions(+), 47 deletions(-) diff --git a/src/cli.ts b/src/cli.ts index 5965de1..63a6593 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -42,7 +42,7 @@ async function runList(api: RingApi): Promise { log.info(`Found ${cameras.length} camera(s):`); for (const c of cameras) { const kind = c.isDoorbot ? 'doorbell' : 'camera'; - const battery = c.batteryLevel != null ? `${c.batteryLevel}%` : 'wired'; + const battery = c.batteryLevel == null ? 'wired' : `${c.batteryLevel}%`; process.stdout.write(` #${c.id} ${c.name} [${kind}, ${c.model}, battery: ${battery}]\n`); } } diff --git a/src/log.ts b/src/log.ts index f3d1aab..5854a8c 100644 --- a/src/log.ts +++ b/src/log.ts @@ -13,7 +13,9 @@ function emit(level: Level, msg: string, extra?: unknown): void { const stamp = new Date().toISOString(); const line = `${stamp} [${level.toUpperCase()}] ${msg}`; const stream = level === 'error' || level === 'warn' ? process.stderr : process.stdout; - if (extra !== undefined) { + if (extra === undefined) { + stream.write(`${line}\n`); + } else { let extraStr: string; if (typeof extra === 'string') { extraStr = extra; @@ -29,8 +31,6 @@ function emit(level: Level, msg: string, extra?: unknown): void { } } stream.write(`${line} ${extraStr}\n`); - } else { - stream.write(`${line}\n`); } } diff --git a/src/verify.ts b/src/verify.ts index de93e34..cd2d42a 100644 --- a/src/verify.ts +++ b/src/verify.ts @@ -20,6 +20,11 @@ const execFileP = promisify(execFile); * 3. probe the clip for playable video (and report audio) * 4. (optional) wait for a real motion/ding event and confirm auto-recording */ +type Config = ReturnType; +type Clip = Awaited>; + +const step = (n: number, msg: string) => log.info(`[${n}] ${msg}`); + async function main(): Promise { const args = parseArgs(process.argv.slice(2)); const seconds = numArg(args.seconds, 10, 1, '--seconds'); @@ -28,53 +33,15 @@ async function main(): Promise { const api = createRingApi(cfg); let failures = 0; - const step = (n: number, msg: string) => log.info(`[${n}] ${msg}`); try { - // 1. AUTH + LIST --------------------------------------------------------- - step(1, 'Authenticating and listing cameras…'); - const cameras = await api.getCameras(); - if (cameras.length === 0) throw new Error('Authenticated but found 0 cameras on the account.'); - for (const c of cameras) { - const battery = c.batteryLevel != null ? `${c.batteryLevel}%` : 'wired'; - log.info(` #${c.id} ${c.name} [${c.isDoorbot ? 'doorbell' : 'camera'}, ${c.model}, ${battery}]`); - } - log.info(` ✓ Auth OK, ${cameras.length} camera(s) listed.`); - + const cameras = await stepAuthAndList(api); const target = pickCamera(cameras, args.camera); if (!target) throw new Error(`No camera matched "${args.camera}".`); - // 2. CAPTURE ------------------------------------------------------------- - step(2, `Capturing a ${seconds}s live clip from "${target.name}"…`); - const clip = await recordClip(target, cfg, seconds); - log.info(` ✓ Wrote ${clip.path} (${(clip.bytes / 1024 / 1024).toFixed(2)} MB).`); - - // 3. PROBE --------------------------------------------------------------- - step(3, 'Probing the clip for playable streams…'); - const streams = await probeStreams(clip.path); - const hasVideo = streams.includes('video'); - const hasAudio = streams.includes('audio'); - if (!hasVideo) { - failures++; - log.error(' ✗ No video stream found — clip is not playable.'); - } else { - log.info(' ✓ Video stream present, clip is playable.'); - } - if (hasAudio) log.info(' ✓ Audio stream present.'); - else log.warn(' ! No audio stream (Ring audio can be off in the app, or omitted for this device).'); - - // 4. MOTION TRIGGER (optional) ------------------------------------------ - if (watchMotion > 0) { - step(4, `Waiting up to ${watchMotion}s for a real motion/ding on "${target.name}" — trigger it now (walk in front / press the doorbell)…`); - const fired = await waitForTriggeredRecording(target, cfg, watchMotion); - if (fired) log.info(' ✓ Motion/ding fired and an auto-recording completed end-to-end.'); - else { - failures++; - log.error(` ✗ No motion/ding event within ${watchMotion}s. Re-run with a longer --watch-motion and trigger motion.`); - } - } else { - step(4, 'Skipping motion test (pass --watch-motion and trigger motion to verify it live).'); - } + const clip = await stepCapture(target, cfg, seconds); + failures += await stepProbe(clip); + failures += await stepWatchMotion(target, cfg, watchMotion); log.info(failures === 0 ? '\nVERIFY: all checks passed.' : `\nVERIFY: ${failures} check(s) failed.`); } finally { @@ -83,6 +50,62 @@ async function main(): Promise { } } +/** 1. Authenticate and list the real cameras on the account. */ +async function stepAuthAndList(api: ReturnType): Promise { + step(1, 'Authenticating and listing cameras…'); + const cameras = await api.getCameras(); + if (cameras.length === 0) throw new Error('Authenticated but found 0 cameras on the account.'); + for (const c of cameras) { + const battery = c.batteryLevel == null ? 'wired' : `${c.batteryLevel}%`; + log.info(` #${c.id} ${c.name} [${c.isDoorbot ? 'doorbell' : 'camera'}, ${c.model}, ${battery}]`); + } + log.info(` ✓ Auth OK, ${cameras.length} camera(s) listed.`); + return cameras; +} + +/** 2. Capture a live clip to disk. */ +async function stepCapture(target: RingCamera, cfg: Config, seconds: number): Promise { + step(2, `Capturing a ${seconds}s live clip from "${target.name}"…`); + const clip = await recordClip(target, cfg, seconds); + log.info(` ✓ Wrote ${clip.path} (${(clip.bytes / 1024 / 1024).toFixed(2)} MB).`); + return clip; +} + +/** 3. Probe the clip for a playable video stream. Returns the failure count. */ +async function stepProbe(clip: Clip): Promise { + step(3, 'Probing the clip for playable streams…'); + const streams = await probeStreams(clip.path); + let failures = 0; + + if (streams.includes('video')) { + log.info(' ✓ Video stream present, clip is playable.'); + } else { + failures++; + log.error(' ✗ No video stream found — clip is not playable.'); + } + + if (streams.includes('audio')) log.info(' ✓ Audio stream present.'); + else log.warn(' ! No audio stream (Ring audio can be off in the app, or omitted for this device).'); + + return failures; +} + +/** 4. Optionally wait for a real motion/ding and confirm auto-recording. Returns the failure count. */ +async function stepWatchMotion(target: RingCamera, cfg: Config, watchMotion: number): Promise { + if (watchMotion <= 0) { + step(4, 'Skipping motion test (pass --watch-motion and trigger motion to verify it live).'); + return 0; + } + + step(4, `Waiting up to ${watchMotion}s for a real motion/ding on "${target.name}" — trigger it now (walk in front / press the doorbell)…`); + if (await waitForTriggeredRecording(target, cfg, watchMotion)) { + log.info(' ✓ Motion/ding fired and an auto-recording completed end-to-end.'); + return 0; + } + log.error(` ✗ No motion/ding event within ${watchMotion}s. Re-run with a longer --watch-motion and trigger motion.`); + return 1; +} + /** Return the list of codec_types ("video"/"audio") present in a media file. */ async function probeStreams(file: string): Promise { const { stdout } = await execFileP('ffprobe', [ @@ -99,7 +122,7 @@ async function probeStreams(file: string): Promise { * Subscribe to motion + ding for one camera; on the first event, record a short * clip and resolve true once it's on disk. Resolves false on timeout. */ -function waitForTriggeredRecording(camera: RingCamera, cfg: ReturnType, timeoutSec: number): Promise { +function waitForTriggeredRecording(camera: RingCamera, cfg: Config, timeoutSec: number): Promise { return new Promise((resolve) => { const subs: Subscription[] = []; let done = false;