chore: JSDoc total coverage + strict checkJs#14
Merged
flyingrobots merged 4 commits intomainfrom Feb 28, 2026
Merged
Conversation
- Fix all JSDoc @param names to match underscore-prefixed abstract params - Add EncryptionMeta, KdfParamSet, DeriveKeyParams typedefs to CryptoPort - Widen port return types to allow sync|async (sha256, encryptBuffer, etc.) - Add full @param/@returns annotations to all adapter overrides - Add VaultMetadata, VaultState, VaultEncryptionMeta typedefs to VaultService - Remove @Private on #private members (TS18010) - Type Semaphore#queue as Array<() => void> - Type StatsCollector#startTime as number|null - Use (...args: unknown[]) => void for EventEmitter listeners - Create ambient.d.ts for @git-stunts/plumbing and bun modules - Add tsconfig.checkjs.json with strict checkJs enabled - Install @types/node for typecheck (dev only) Result: tsc --checkJs passes with 0 errors (was 342), eslint 0 errors, all 785 tests pass.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR adds comprehensive JSDoc type annotations and ambient type declarations across the codebase. Key changes include: widening CryptoPort return types (string|Promise, Buffer|Uint8Array), renaming port parameters to underscore-prefixed variants (TS8024 fix), introducing new type definitions (EncryptionMeta, KdfParamSet, VaultMetadata), annotating CLI and observer handlers, and minor guards for encryption streams. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/ui/manifest-view.js (1)
89-94:⚠️ Potential issue | 🟡 MinorGuard
blobaccess before slicing to prevent render crashes.If a chunk lacks
blob,c.blob.slice(...)will throw and break the view.🛡️ Proposed defensive rendering fix
- const chunkRows = displayChunks.map((/** `@type` {{ index: number, size: number, digest: string, blob: string }} */ c) => [ + const chunkRows = displayChunks.map((/** `@type` {{ index: number, size: number, digest: string, blob?: string }} */ c) => [ String(c.index), formatBytes(c.size), `${c.digest.slice(0, 12)}...`, - `${c.blob.slice(0, 12)}...`, + typeof c.blob === 'string' ? `${c.blob.slice(0, 12)}...` : '-', ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/ui/manifest-view.js` around lines 89 - 94, Guard access to c.blob when building chunkRows inside the displayChunks.map to avoid render crashes; update the mapping in the chunkRows creation so it checks c.blob (and optionally c.digest) before calling .slice, e.g. use a conditional/fallback like (c.blob ? c.blob.slice(0,12) + '...' : '<no-blob>') for that array element (and similarly for c.digest if needed) so undefined values won't cause exceptions during rendering.
🧹 Nitpick comments (5)
bin/git-cas.js (1)
189-189: Prefer typed option shapes overanycasts on CAS calls.These
anycasts weaken the strictcheckJsguarantees this PR is aiming for. Consider replacingRecord<string, any>+anycasts with local JSDoc typedefs (or exported option types) for store/restore/rotate payloads.Also applies to: 283-286, 485-485, 538-538
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/git-cas.js` at line 189, Replace the ad-hoc /** `@type` {any} */ casts with explicit JSDoc typedefs for the CAS payload shapes (e.g., define a StoreOptions typedef and similar RestoreOptions/RotateOptions) and use those types where variables like storeOpts are declared/used and when calling cas methods (cas.storeFile, cas.restoreFile, cas.rotate*). Locate the variables named storeOpts (and other payload variables mentioned in this PR) and add local or exported JSDoc `@typedefs` that describe the expected properties (use Record<string, unknown> or specific keys instead of any), then annotate the variable declarations or the cas.* call sites with the new typedef names so checkJs can validate types without any casts.tsconfig.checkjs.json (1)
21-27: Minor: Redundant include pattern.Line 26 (
src/types/ambient.d.ts) is already covered by line 24 (src/**/*.d.ts). This redundancy is harmless but can be removed for cleaner configuration.♻️ Proposed simplification
"include": [ "index.js", "src/**/*.js", "src/**/*.d.ts", "index.d.ts", - "src/types/ambient.d.ts", "bin/**/*.js" ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tsconfig.checkjs.json` around lines 21 - 27, Remove the redundant include entry "src/types/ambient.d.ts" from the "include" array in tsconfig.checkjs.json because it's already matched by the glob "src/**/*.d.ts"; update the "include" array to keep only the more general patterns (e.g., "src/**/*.d.ts") to simplify the config.bin/ui/heatmap.js (1)
94-100: Avoidanyforoptions.manifestto preserve strict typing value.
anydrops type safety exactly where normalization happens. Prefer a narrow union-like shape.♻️ Proposed JSDoc tightening
- * `@param` {any} options.manifest - The manifest (Manifest instance or plain ManifestData). + * `@param` {ManifestData|{ toJSON: () => ManifestData }} options.manifest - The manifest (Manifest instance or plain ManifestData).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/ui/heatmap.js` around lines 94 - 100, The JSDoc for the renderHeatmap parameter is using `any` for `options.manifest`, which loses type-safety; update the JSDoc to a narrower union (e.g., `Manifest | ManifestData | { toJSON(): ManifestData }`) so callers and tooling know the expected shape, and then keep the normalization in renderHeatmap (the `manifest.toJSON ? manifest.toJSON() : manifest` logic) but annotate the local `m` as `ManifestData` in the same JSDoc block so `chunks` and other properties are typed; reference the `renderHeatmap` function and the `manifest.toJSON`/`ManifestData` symbols when making this change.bin/ui/manifest-view.js (1)
142-148: Replaceanyonoptions.manifestwith a concrete shape.Using
anyhere bypasses strictness and can hide invalid manifest inputs.♻️ Proposed JSDoc tightening
- * `@param` {any} options.manifest - The manifest (Manifest instance or plain ManifestData). + * `@param` {ManifestData|{ toJSON: () => ManifestData }} options.manifest - The manifest (Manifest instance or plain ManifestData).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/ui/manifest-view.js` around lines 142 - 148, The JSDoc for renderManifestView currently types options.manifest as any; replace it with a concrete type (e.g., Manifest | ManifestData or an explicit object shape) so callers and IDEs get proper typing and invalid manifests are caught. Update the JSDoc parameter to reference the existing ManifestData typedef (or declare a short typedef for the union Manifest|ManifestData or an object with an optional toJSON(): ManifestData method) and keep the implementation using manifest.toJSON ? manifest.toJSON() : manifest; ensure the JSDoc name matches the function signature renderManifestView and the ManifestData symbol used elsewhere.src/ports/CryptoPort.js (1)
97-97: Widenoptions.saltJSDoc for consistency with cross-runtime byte inputs.The port now broadly accepts
Buffer|Uint8Arrayin related paths;saltshould match that shape.♻️ Proposed type consistency fix
- * `@param` {Buffer} [options.salt] - Salt for the KDF (random if omitted). + * `@param` {Buffer|Uint8Array} [options.salt] - Salt for the KDF (random if omitted).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ports/CryptoPort.js` at line 97, Update the JSDoc for the salt parameter to allow cross-runtime byte inputs: change the `@param` from {Buffer} [options.salt] to {Buffer|Uint8Array} [options.salt] in src/ports/CryptoPort.js (the JSDoc that documents options.salt used by the CryptoPort methods) so the documented type matches the actual accepted types (Buffer and Uint8Array).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/actions.js`:
- Around line 38-39: Guard the HINTS lookup to avoid inherited properties by
checking ownership before indexing: in the block that computes hint (referencing
the variables hint, code and the HINTS object), replace the direct index access
with a conditional that first verifies
Object.prototype.hasOwnProperty.call(HINTS, code) (or an equivalent own-key
check) and only then uses HINTS[code] to assign hint; this prevents resolving
inherited keys like __proto__ while preserving the existing behavior when code
is a known HINTS key.
In `@bin/git-cas.js`:
- Around line 57-62: Before casting metadata.encryption, add an explicit runtime
guard that checks metadata and metadata.encryption exist and have the expected
fields (e.g., kdf, kdf.salt, kdf.algorithm, kdf.iterations); if the check fails,
throw a clear Error like "Missing or malformed encryption metadata" (include
enough context such as metadata.id or similar) so the subsequent use of kdf and
the call to cas.deriveKey are safe; then continue to read const { kdf } =
metadata.encryption and call cas.deriveKey as before.
- Around line 31-33: The readKeyFile function currently returns whatever
readFileSync(keyFilePath) yields without validating size; update readKeyFile to
read the file into a Buffer, verify buffer.length === 32 and throw a clear error
(e.g., "invalid key length") if not, otherwise return the Buffer; reference the
readKeyFile function and the keyFilePath parameter when adding the length check
and error handling.
In `@bin/ui/dashboard-view.js`:
- Around line 94-95: The clamp logic treats size.width === 0 as falsy and skips
slicing; update the check used by clamp to explicitly test for a positive width
(e.g., use size.width > 0) so zero-width panes are handled correctly, and ensure
filterLine (which uses model.filtering and model.filterText) still calls clamp
with the same string when width is positive; modify the clamp definition and its
usage so the condition is an explicit numeric greater-than check rather than a
truthiness check.
In `@bin/ui/progress.js`:
- Around line 111-114: In the interactive branch (where deps.ctx.mode ===
'interactive') replace the direct process.stderr.write call with the
context-aware writer: call deps.ctx.io.write(`\r\x1b[K${status}`) so output
honors options.ctx overrides; optionally guard with a fallback to
process.stderr.write if deps.ctx.io is undefined. Keep the same status string
and still call deps.bar.update(pct).
In `@src/infrastructure/adapters/EventEmitterObserver.js`:
- Around line 19-21: The JSDoc/type mismatch: `data.action` is used without
guard but the type marks it optional; update the type for the metric parameter
in EventEmitterObserver so `action` is required (change Record<string, unknown>
& { action?: string } to Record<string, unknown> & { action: string }) and
update the JSDoc to say `action` is required; additionally add a short runtime
assertion in the method that accesses data.action (check data.action and throw
or log + return) to avoid forming event names like "chunk:undefined".
---
Outside diff comments:
In `@bin/ui/manifest-view.js`:
- Around line 89-94: Guard access to c.blob when building chunkRows inside the
displayChunks.map to avoid render crashes; update the mapping in the chunkRows
creation so it checks c.blob (and optionally c.digest) before calling .slice,
e.g. use a conditional/fallback like (c.blob ? c.blob.slice(0,12) + '...' :
'<no-blob>') for that array element (and similarly for c.digest if needed) so
undefined values won't cause exceptions during rendering.
---
Nitpick comments:
In `@bin/git-cas.js`:
- Line 189: Replace the ad-hoc /** `@type` {any} */ casts with explicit JSDoc
typedefs for the CAS payload shapes (e.g., define a StoreOptions typedef and
similar RestoreOptions/RotateOptions) and use those types where variables like
storeOpts are declared/used and when calling cas methods (cas.storeFile,
cas.restoreFile, cas.rotate*). Locate the variables named storeOpts (and other
payload variables mentioned in this PR) and add local or exported JSDoc
`@typedefs` that describe the expected properties (use Record<string, unknown> or
specific keys instead of any), then annotate the variable declarations or the
cas.* call sites with the new typedef names so checkJs can validate types
without any casts.
In `@bin/ui/heatmap.js`:
- Around line 94-100: The JSDoc for the renderHeatmap parameter is using `any`
for `options.manifest`, which loses type-safety; update the JSDoc to a narrower
union (e.g., `Manifest | ManifestData | { toJSON(): ManifestData }`) so callers
and tooling know the expected shape, and then keep the normalization in
renderHeatmap (the `manifest.toJSON ? manifest.toJSON() : manifest` logic) but
annotate the local `m` as `ManifestData` in the same JSDoc block so `chunks` and
other properties are typed; reference the `renderHeatmap` function and the
`manifest.toJSON`/`ManifestData` symbols when making this change.
In `@bin/ui/manifest-view.js`:
- Around line 142-148: The JSDoc for renderManifestView currently types
options.manifest as any; replace it with a concrete type (e.g., Manifest |
ManifestData or an explicit object shape) so callers and IDEs get proper typing
and invalid manifests are caught. Update the JSDoc parameter to reference the
existing ManifestData typedef (or declare a short typedef for the union
Manifest|ManifestData or an object with an optional toJSON(): ManifestData
method) and keep the implementation using manifest.toJSON ? manifest.toJSON() :
manifest; ensure the JSDoc name matches the function signature
renderManifestView and the ManifestData symbol used elsewhere.
In `@src/ports/CryptoPort.js`:
- Line 97: Update the JSDoc for the salt parameter to allow cross-runtime byte
inputs: change the `@param` from {Buffer} [options.salt] to {Buffer|Uint8Array}
[options.salt] in src/ports/CryptoPort.js (the JSDoc that documents options.salt
used by the CryptoPort methods) so the documented type matches the actual
accepted types (Buffer and Uint8Array).
In `@tsconfig.checkjs.json`:
- Around line 21-27: Remove the redundant include entry "src/types/ambient.d.ts"
from the "include" array in tsconfig.checkjs.json because it's already matched
by the glob "src/**/*.d.ts"; update the "include" array to keep only the more
general patterns (e.g., "src/**/*.d.ts") to simplify the config.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (33)
CHANGELOG.mdbin/actions.jsbin/git-cas.jsbin/ui/context.jsbin/ui/dashboard-cmds.jsbin/ui/dashboard-view.jsbin/ui/dashboard.jsbin/ui/encryption-card.jsbin/ui/heatmap.jsbin/ui/history-timeline.jsbin/ui/manifest-view.jsbin/ui/progress.jsjsr.jsonpackage.jsonsrc/domain/services/Semaphore.jssrc/domain/services/VaultService.jssrc/infrastructure/adapters/BunCryptoAdapter.jssrc/infrastructure/adapters/EventEmitterObserver.jssrc/infrastructure/adapters/GitPersistenceAdapter.jssrc/infrastructure/adapters/GitRefAdapter.jssrc/infrastructure/adapters/NodeCryptoAdapter.jssrc/infrastructure/adapters/SilentObserver.jssrc/infrastructure/adapters/StatsCollector.jssrc/infrastructure/adapters/WebCryptoAdapter.jssrc/infrastructure/chunkers/FixedChunker.jssrc/infrastructure/codecs/CborCodec.jssrc/infrastructure/codecs/JsonCodec.jssrc/ports/CodecPort.jssrc/ports/CryptoPort.jssrc/ports/GitPersistencePort.jssrc/ports/GitRefPort.jssrc/types/ambient.d.tstsconfig.checkjs.json
- Remove redundant tsconfig include entry
- Widen CryptoPort salt type to Buffer|Uint8Array
- Guard blob/digest access in manifest chunk rendering
- Replace any casts with typed option shapes (StoreFileOpts, inline types)
- Replace any manifest params with ManifestData | { toJSON(): ManifestData }
- Guard HINTS lookup against prototype keys via getHint()
- Validate 32-byte key file length in readKeyFile()
- Add runtime guard for metadata.encryption in deriveVaultKey()
- Fix clamp width check to use typeof + explicit > 0
- Use ctx.io.write in interactive progress output
- Make EventEmitterObserver action required with runtime guard
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
@param/@returnsannotations to every function, class method, and callback across all 32 source files (ports, adapters, services, CLI)tsconfig.checkjs.jsonwith strictcheckJs—tsc --noEmitpasses with zero errors (was 342)src/types/ambient.d.tsfor@git-stunts/plumbingandbunmodule declarationsTest plan
npx tsc -p tsconfig.checkjs.json --noEmit— 0 errorsnpx eslint .— 0 errorsnpm test— 785 tests passnpm pack --dry-run— cleannpx jsr publish --dry-run --allow-dirty— cleanSummary by CodeRabbit
Release Notes: Version 5.2.2
New Features
Bug Fixes
Chores