Fix SonarCloud code smells (8 of 19) - #8
Merged
Conversation
…a formatting
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.
|
🤖 Review skipped: Repository Owner rate limit exceeded. Free accounts are limited to 3 reviews per 4 hours across all repositories. Upgrade to a paid plan for unlimited reviews. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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<typeof loadConfig>.
No behavior change. npm test: 15 passed, 0 failed.
|
🤖 Review skipped: Repository Owner rate limit exceeded. Free accounts are limited to 3 reviews per 4 hours across all repositories. Upgrade to a paid plan for unlimited reviews. |
|
Owner
Author
Update:
|
fayerman-source
added a commit
that referenced
this pull request
Jul 28, 2026
Clears the two SonarCloud issues that surfaced on main after #8. S2187 (BLOCKER, test/logic.test.mjs): the file contained 15 real assertions, but as a hand-rolled harness (bare blocks, a local ok() helper, a manual pass/fail tally), so Sonar's JavaScript analyzer saw no recognizable tests. Migrated to the built-in node:test runner — no new dependencies, and a real upgrade: per-test isolation, proper reporting, and a nonzero exit driven by the runner instead of a hand-rolled process.exit. 11 tests, all 15 original assertions preserved. Retention cases now build a fresh temp dir each via a setup() helper instead of sharing one directory, so they no longer depend on execution order. Multi-step state-machine sequences stay grouped one-test-each, since their assertions are checkpoints in a single watchCamera run and splitting them would mean replaying setup. Test script is `node --test test/*.test.mjs`, not `node --test test/`: since Node 22 positional args are glob patterns and a bare directory matches nothing. Verified failing on Node 24, passing on both with the glob. S6505 (.github/workflows/ci.yml): `npm ci` ran dependency lifecycle scripts, a supply-chain foothold where any transitive dep can execute arbitrary code at install time. Now `npm ci --ignore-scripts`, with no npm rebuild afterwards — the three script-shipping deps (esbuild, ffmpeg-for-homebridge, protobufjs) only fetch or build binaries this job never uses, since it type-checks and runs hermetic tests that touch neither ffmpeg nor tsx. Skipping the rebuild is both the stronger hardening and a faster, less network-dependent job. Verified: clean `npm ci --ignore-scripts` into a scratch copy builds and passes 11/11, and a deliberately broken assertion exits 1, so this cannot silently green the build. CI: build-test and SonarCloud Code Analysis both passed; SonarCloud reported 0 open issues in the PR context.
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.
Resolves the 8 SonarCloud issues worth code changes. The remaining 11 are rule mismatches that should be dismissed as Won't Fix in the SonarCloud UI (rationale below).
Fixed
S6551src/log.tsString(extra)→[object Object], destroying diagnostic value in exactly the case the fallback exists for (the logger is called fromcatchblocks). Nowutil.inspect(extra, {depth: 3}), which handles cycles and prints readable structure. The only fix here with practical impact.S3863×2src/config.tsnode:pathimport.S4624×3src/ring.ts,src/cli.tsformatCameraList()helper. Removes the nested template literals and the duplicatedcameras.map(c => `${c.name} (#${c.id})`).join(', ')across both files.S6582src/ring.ts!raw || !raw.refreshToken→!raw?.refreshToken.S3776src/cli.tsmain()was a dispatcher with both command bodies inlined; extractedrunList()/runRecord(). Cognitive complexity 23 → under 15.S6582test/logic.test.mjsresolveLast && resolveLast()→resolveLast?.().No behavior change other than improved log output on circular payloads.
Not fixed — dismiss as Won't Fix
S7785"prefer top-level await" ×4 (auth.ts:82,cli.ts:90,index.ts:45,verify.ts:165) — every one is the entrypointmain().catch(err => { log.error(...); process.exit(1) }). That pattern is what gives these CLIs a clean error message and a controlled exit code; top-levelawaitturns failures into unhandled-rejection traces with a different exit path. The rule is wrong for a CLI entrypoint.S7735"unexpected negated condition" ×4 (cli.ts:32,log.ts:14,verify.ts:39,57) — all guard clauses / early returns (if (!DEBUG) return;). Inverting them makes the code worse.S7776(test/logic.test.mjs:28) — use aSetinstead ofArray.includes()on a 3-element array in a test assertion. Micro-optimization with negative readability value.Left alone (judgment call, not dismissed)
S3776onverify.ts:23(complexity 17/15). Thatmain()is a linear 4-step diagnostic script where the numbered narrative is the readability. Splitting intostepAuth/stepCapture/stepProbe/stepWatchwould satisfy the gate without improving the code. Happy to do it if you want the quality gate fully green.Verification
npm test(builds viatscthen runs the suite): 15 passed, 0 failed, no type errors.🤖 Generated with Claude Code