Configure CodeRabbit for this repo's conventions - #587
Merged
Conversation
Baseline taken from the signalk-server config, then adapted to Skip's CLAUDE.md and to the traps a generic reviewer gets wrong here — chiefly that specs run against the real app services on purpose, so the usual "add a stub" advice is inert, and that a per-file mock of an aliased library is the defect #544 was. profile is `chill` rather than `assertive` because main has required_conversation_resolution enabled: every comment is a merge gate until someone resolves it, so a style nit costs more than it finds.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesCodeRabbit review configuration
Widget documentation references
The PR also deletes three planning documents and the former widget schematic guide. Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to This change configures repository review conventions and updates documentation; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Three of the four files were planning records for work that shipped long ago. Git history holds them. The fourth, widget-schematic.md, read as live documentation — README pointed at it three times for the scaffolding CLI's flags — but 8 of the 14 options it documents do not exist in the schematic's schema (description, icon, category, pathKey, pathDescription, convertUnitTo, sampleTime, interactive), and 4 that do exist go unmentioned. It came from the KIP era, in #835, and drifted from the first change afterwards. A reader following it would pass flags that do nothing. README now points at the schematic's own schema.json, which cannot drift from the CLI because it is the CLI's definition.
Lowering the profile trades away real findings to avoid a merge gate that costs one reply to clear. The instructions below are the right instrument for a reviewer that raises the wrong things.
mairas
force-pushed
the
chore/coderabbit-config
branch
from
August 13, 2026 09:00
79a266b to
c65952f
Compare
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
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.
Why
The CodeRabbit app is installed but unconfigured, so it reviews with generic defaults. On #583 that already cost a merge: it asked for tighter
toBeCloseToprecision on the unit-conversion tests, which is the opposite of the policy stated two tests up in the same file, andrequired_conversation_resolutionturned that nit into a merge gate.Approach
Baseline is
signalk-server/.coderabbit.yaml; the structure and the "what NOT to flag" idea come from there. The content is Skip's.profile: assertive, matching the baseline.Traps a generic reviewer gets wrong here, encoded as
path_instructions:src/test.tsas{ provide: AppClass, ... }is DI-inert, so the standard "add a shared stub" advice is not merely unwanted, it does nothing. A local fake must still exposeserverServiceEndpoint$andserverVersion$.vi.mock()for a libraryvitest.config.tsaliases to a shim — that is exactly what #544 was.units.service.tsdiverges from the server's imperial fuel-flow formulas by 3600× on purpose (upstreamsignalk-server#2951); do not suggest reconciling.DataService.updatePathDataruns per delta on Pi-class hardware; ask forperf-harnessnumbers rather than asserting a regression.suppressBootstrapNullfilters the replayed null./@halos-org/skip/serving path is a cross-package contract with@halos-org/skip-freeboard-panel.Four things it must stop asking for: a CHANGELOG entry (the file was removed in #586), a VERSION bump (
version-bump-checkalready enforces the subtle cycle rule), syncing with mxtommy/kip (not an upstream), and a bundled server plugin (webapp-only by design).Evidence standard from the review conventions, as
tone_instructions: name the trigger, the code path and the user-visible consequence, or do not raise it.Noise removed:
eslintoff (npm run lintalready gates every PR),markdownlintandlanguagetooloff (docs here are deliberate prose),poemandin_progress_fortuneoff.github-checks.timeout_msraised to 10 minutes becausenpm run cioutlasts the 90-second default, so CodeRabbit was giving up before the result existed.path_filtersexclude build output, the lockfile, the two vendored minified libraries, the generated MCP schema artifact, and the committed perf baselines. Verified againstgit ls-files: 172 tracked files excluded, none of them source.knowledge_base.code_guidelinespoints atCLAUDE.md, so the rest is picked up rather than duplicated here.Verification
Validated against
schema.v2.json. Note that schema only setsadditionalProperties: falseat the root, so a typo underreviewswould pass validation silently — every key and enum value was additionally checked against the schema's declared properties by walking it.One real bug that caught:
mode: offparses as the booleanfalseunder YAML 1.1, where the field wants the string"off". It is quoted, with a comment saying why.Also in this PR: the
docs/tree is removedCalled out because it is a second topic. It touches this PR's own config, which is why it rides along:
.coderabbit.yamlhad apath_instructionsexception telling CodeRabbit not to flagdocs/**-plan.mdas stale, and that exception is now dead. It is replaced by an instruction not to suggest re-creating the tree.Three of the four files were planning records for work that shipped long ago — git history holds them.
The fourth needed checking, because
README.mdpointed atdocs/widget-schematic.mdthree times as live documentation for thenpm run generate:widgetCLI. It is not live. Comparing it againsttools/schematics/create-host2-widget/schema.json:description,icon,category,pathKey,pathDescription,convertUnitTo,sampleTime,interactive.pathDefault,zonesSupport,todoBlock,debugLogging.name,title,pathType,registerWidget,addSpec,readme.It arrived in the KIP era (
mxtommy/kipPR 835, Oct 2025) and drifted from the first change afterwards. Somebody following it would pass flags that do nothing.The three README references now point at
schema.jsonitself, which cannot drift from the CLI because it is the CLI's definition.