Document complex API boundaries - #6
Conversation
📝 WalkthroughWalkthroughThe pull request adds adjacent JSDoc contracts to exported APIs across profile, HID, lighting, session, and input-profile modules. It also adds contribution guidance and automated tests that verify the documented API surface. ChangesAPI documentation coverage
Estimated code review effort: 3 (Moderate) | ~20 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 |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e199f0b37e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function exportedFunctionNames(source) { | ||
| return [...source.matchAll(/export\s+(?:async\s+)?function\s+([A-Za-z_$][\w$]*)/g)] | ||
| .map((match) => match[1]) | ||
| .sort(); |
There was a problem hiding this comment.
Extend this surface check beyond direct export function declarations. If the workflow adds a callable as export const helper = () => ..., a class, or a separately declared symbol exposed with export { helper }, exportedFunctionNames() ignores it, so the equality assertion still passes and no JSDoc contract is checked for the new public API.
Useful? React with 👍 / 👎.
| for (const member of ["constructor", "open", "onNotification", "call", "close"]) { | ||
| assertDocumentedMember(source, file, "DeviceSession", member); |
There was a problem hiding this comment.
Derive the public DeviceSession member list from the class source, or assert that this allowlist matches it. As written, adding any new public method without updating this array makes the documentation test pass without examining that method, defeating the regression guard precisely when the session API expands.
Useful? React with 👍 / 👎.
| * Sanitizes persisted data into the current snapshot shape and slot count. | ||
| * Invalid snapshots fail closed to {@link emptySnapshot}; extra slots vanish. | ||
| * | ||
| * @param {unknown} value Deserialized snapshot candidate. | ||
| * @returns {SlotSnapshot} Defensive copy safe for reducer operations. |
There was a problem hiding this comment.
Do not return the documented SlotSnapshot contract for arbitrary persisted entries without validating their state. For example, {slots: [{sessionId: "x", state: "bogus"}]} is copied verbatim by this function, and slotView() subsequently emits an undefined color even though SessionSlot.state is documented as a known shared state and the result is described as safe for reducer operations; validate state and pending values or narrow the contract.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
prototype/src/profile-workflow.js (1)
66-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the preservation report shape.
reportis declared asobject, butprototype/tests/profile-session.test.mjs:84-110readsreview.report.nativeLayerPreserved. Define the report fields and types, preferably with a shared typedef, so consumers can rely on this boundary contract.🤖 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 `@prototype/src/profile-workflow.js` around lines 66 - 77, Document the preservation report contract used by the profile-building function, preferably via a shared typedef referenced by its return annotation. Define the report shape to include the boolean nativeLayerPreserved field consumed by profile-session tests, along with the other fields returned by the transformer, so consumers have explicit field types.
🤖 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 `@prototype/src/profile-workflow.js`:
- Around line 43-50: Update the sha256Hex JSDoc parameter declaration to match
its supported inputs: allow null and the minimal injectable object exposing
subtle.digest, while preserving the existing failure behavior for unavailable or
rejected Web Crypto.
In `@scripts/lib/hid-device.mjs`:
- Around line 123-129: Update the onForeignWrite contract in the handle/session
JSDoc declarations and the `#resolve`() callback invocation consistently: either
await and catch rejected promises with defined failure behavior, or remove
Promise<void> from both callback types and keep them synchronous. Ensure the
constructor and DeviceSession.open declarations match the behavior implemented
by `#resolve`().
- Around line 178-187: Preserve validation errors from buildRequest in
DeviceSession.call instead of wrapping them as WRITE_FAILED in `#run`. Validate
the method and request id before queueing, or identify and rethrow the dedicated
validation error separately from HID write failures; document this behavior in
the call method’s JSDoc.
In `@scripts/lib/hid-lighting.mjs`:
- Around line 111-116: Update the exported JSDoc blocks for
threadsLightingParams and rgbConfigParams to declare that they may throw Error
propagated from threadEntry and zoneSide respectively, without changing runtime
behavior.
In `@scripts/lib/thread-slots.mjs`:
- Around line 19-31: Complete the SessionSlot typedef by declaring the kind and
startedAt properties with the types returned by claude agents --json, matching
the fields applyRoster persists. Keep the existing applyRoster behavior
unchanged rather than removing those values.
- Around line 39-40: Clarify the JSDoc for SlotSnapshot.overflow to state that
applyRoster accumulates the number of sessions unable to claim a slot across
reconciliations, rather than representing the current live-session count; do not
change the reconciliation logic.
- Around line 84-90: Update the JSDoc for stateFromHookEvent to explicitly
document that idle_prompt is an exception to the non-blocking notification rule
and returns STATES.idle; retain the existing null behavior description for other
non-blocking or unrecognized events.
In `@tests/api-docs.test.mjs`:
- Around line 56-60: Update exportedFunctionNames and the related
export-contract checks to enumerate every supported source export declaration,
including exported const bindings and classes in addition to function
declarations. Preserve the declaration kind for each match so callable exports
and constant/class exports receive their appropriate JSDoc contract validation,
ensuring newly added exports cannot bypass the equality check.
---
Nitpick comments:
In `@prototype/src/profile-workflow.js`:
- Around line 66-77: Document the preservation report contract used by the
profile-building function, preferably via a shared typedef referenced by its
return annotation. Define the report shape to include the boolean
nativeLayerPreserved field consumed by profile-session tests, along with the
other fields returned by the transformer, so consumers have explicit field
types.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b269e342-ccc5-4fee-990f-1d768b579a9a
📒 Files selected for processing (10)
CONTRIBUTING.fr.mdCONTRIBUTING.mdprototype/src/profile-workflow.jsscripts/lib/hid-device.mjsscripts/lib/hid-frame.mjsscripts/lib/hid-lighting.mjsscripts/lib/thread-slots.mjsshared/input-profile.mjsshared/thread-status-palette.mjstests/api-docs.test.mjs
| /** | ||
| * Computes a browser-compatible SHA-256 digest without making hashing a hard | ||
| * requirement. Unavailable or rejected Web Crypto returns an empty string. | ||
| * | ||
| * @param {string} text UTF-8 text to hash. | ||
| * @param {Crypto} [crypto] Injectable Web Crypto implementation. | ||
| * @returns {Promise<string>} Lowercase hexadecimal digest, or `""` on failure. | ||
| */ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 '\bsha256Hex\s*\(' prototype --glob '*.js' --glob '*.mjs'Repository: thannous/claude-codex-micro
Length of output: 2471
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== profile-workflow sha256Hex implementation =="
sed -n '35,65p' prototype/src/profile-workflow.js
echo
echo "== sha256Hex call sites with crypto argument =="
rg -n -C 3 '\bsha256Hex\s*\(\s*[^,\n]+,\s*([^,\n]+)' prototype/src prototype/tests --glob '*.js' --glob '*.mjs'Repository: thannous/claude-codex-micro
Length of output: 2835
Normalize the crypto parameter type to match supported inputs.
sha256Hex is called with null and a minimal { subtle: { digest } } object. Update the JSDoc type to reflect nullable and minimal injectable inputs, or narrow the runtime contract and change the 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 `@prototype/src/profile-workflow.js` around lines 43 - 50, Update the sha256Hex
JSDoc parameter declaration to match its supported inputs: allow null and the
minimal injectable object exposing subtle.digest, while preserving the existing
failure behavior for unavailable or rejected Web Crypto.
| /** | ||
| * @param {{on: Function, write: Function, close: Function}} handle Open HID handle. | ||
| * @param {object} [options] Session callbacks. | ||
| * @param {(method: string, response: unknown) => void|Promise<void>} [options.onForeignWrite] | ||
| * Called for orphan responses to known lighting methods. | ||
| * @param {(line: string) => void} [options.onDebugLine] Called for firmware debug lines. | ||
| */ |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle rejected onForeignWrite promises or remove Promise<void> from the contract.
The constructor and DeviceSession.open contracts accept an asynchronous callback. #resolve() invokes this callback at Line [278] without awaiting or catching the returned promise. A rejected callback promise becomes unhandled. Either handle the promise and define its failure behavior, or restrict both JSDoc types to void.
Also applies to: 142-151
🤖 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/lib/hid-device.mjs` around lines 123 - 129, Update the onForeignWrite
contract in the handle/session JSDoc declarations and the `#resolve`() callback
invocation consistently: either await and catch rejected promises with defined
failure behavior, or remove Promise<void> from both callback types and keep them
synchronous. Ensure the constructor and DeviceSession.open declarations match
the behavior implemented by `#resolve`().
| /** | ||
| * Queues one RPC call and resolves only its correlated response. Calls run | ||
| * sequentially with {@link CALL_SPACING_MS} between start times. | ||
| * | ||
| * @param {string} method Firmware method name. | ||
| * @param {unknown} [params] Method parameters; `null` when omitted. | ||
| * @param {number} [id] Explicit request id, primarily for deterministic tests. | ||
| * @returns {Promise<object>} Parsed firmware response envelope. | ||
| * @throws {DeviceError} Via rejection on timeout, write, RPC, or disconnect failure. | ||
| */ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve request-validation errors in DeviceSession.call().
buildRequest() in scripts/lib/hid-frame.mjs rejects empty methods and identifiers outside [0, 999). #run() catches those errors in the same block as HID write failures and wraps them as WRITE_FAILED at Lines [228]-[237]. Callers cannot distinguish invalid input from a transport failure. Validate before queueing or preserve a dedicated validation error, then document the behavior here.
🤖 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/lib/hid-device.mjs` around lines 178 - 187, Preserve validation
errors from buildRequest in DeviceSession.call instead of wrapping them as
WRITE_FAILED in `#run`. Validate the method and request id before queueing, or
identify and rethrow the dedicated validation error separately from HID write
failures; document this behavior in the call method’s JSDoc.
| /** | ||
| * Builds the parameter array for `v.oai.thstatus`. | ||
| * | ||
| * @param {ThreadLightingInput[]} entries Repository-facing slot updates. | ||
| * @returns {Array<object>} Firmware-facing entries in the original order. | ||
| */ |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Document propagated validation errors on the exported wrappers.
threadsLightingParams calls threadEntry for each entry. rgbConfigParams calls zoneSide for both zones. Both helpers can throw Error, but these exported JSDoc blocks do not declare @throws.
Proposed documentation update
* `@param` {ThreadLightingInput[]} entries Repository-facing slot updates.
* `@returns` {Array<object>} Firmware-facing entries in the original order.
+ * `@throws` {Error} When an entry fails `threadEntry` validation.
*/
export function threadsLightingParams(entries) {
* `@param` {{ambient: ZoneLightingInput, keys: ZoneLightingInput}} input Zone inputs.
* `@returns` {{ambient: object, keys: object}} Firmware-facing complete configuration.
+ * `@throws` {Error} When a zone fails `zoneSide` validation.
*/
export function rgbConfigParams({ ambient, keys }) {Also applies to: 139-144
🤖 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/lib/hid-lighting.mjs` around lines 111 - 116, Update the exported
JSDoc blocks for threadsLightingParams and rgbConfigParams to declare that they
may throw Error propagated from threadEntry and zoneSide respectively, without
changing runtime behavior.
| /** | ||
| * @typedef {object} SessionSlot | ||
| * @property {string} sessionId Claude Code session identifier. | ||
| * @property {string} state One of the shared `STATES` values. | ||
| * @property {number} [updatedAt] Timestamp of the last state transition. | ||
| * @property {string} [name] Display name from the official roster. | ||
| * @property {string} [cwd] Session working directory. | ||
| * @property {number} [pid] Session process identifier. | ||
| * @property {string} [tty] Terminal identifier reported by `ps`. | ||
| * @property {string} [terminalApp] Supported terminal application name. | ||
| * @property {string} [entrypoint] Claude Code host entrypoint. | ||
| * @property {string} [hostSessionId] Host grouping id, not a navigation target. | ||
| */ |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Complete the SessionSlot typedef.
applyRoster copies kind and startedAt into session entries at Lines 268-270, but SessionSlot does not declare either property. Add both fields with the types emitted by claude agents --json, or stop persisting them.
🤖 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/lib/thread-slots.mjs` around lines 19 - 31, Complete the SessionSlot
typedef by declaring the kind and startedAt properties with the types returned
by claude agents --json, matching the fields applyRoster persists. Keep the
existing applyRoster behavior unchanged rather than removing those values.
| * @property {number} overflow Live sessions that could not claim a slot. | ||
| */ |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Clarify the lifecycle of SlotSnapshot.overflow.
applyRoster increments next.overflow for each session that cannot claim a slot. The current text reads as a current live-session count. Document that the value is cumulative, or reset it before reconciliation if a current count is intended.
Proposed documentation update
- * `@property` {number} overflow Live sessions that could not claim a slot.
+ * `@property` {number} overflow Cumulative count of roster sessions that could not claim a slot.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * @property {number} overflow Live sessions that could not claim a slot. | |
| */ | |
| * `@property` {number} overflow Cumulative count of roster sessions that could not claim a slot. | |
| */ |
🤖 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/lib/thread-slots.mjs` around lines 39 - 40, Clarify the JSDoc for
SlotSnapshot.overflow to state that applyRoster accumulates the number of
sessions unable to claim a slot across reconciliations, rather than representing
the current live-session count; do not change the reconciliation logic.
| /** | ||
| * Converts one Claude hook payload into a lighting state transition. | ||
| * Non-blocking notifications and unrecognized events deliberately return null. | ||
| * | ||
| * @param {object|null|undefined} event Normalized hook event. | ||
| * @returns {string|null} A shared `STATES` value, or null for no transition. | ||
| */ |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Document the idle_prompt exception.
The comment states that non-blocking notifications return null, but stateFromHookEvent returns STATES.idle for idle_prompt at Lines 97-100. List this exception in the contract.
Proposed documentation update
- * Non-blocking notifications and unrecognized events deliberately return null.
+ * Unrecognized events return null. Blocking notifications return `STATES.blocked`;
+ * `idle_prompt` returns `STATES.idle`; other non-blocking notifications return null.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Converts one Claude hook payload into a lighting state transition. | |
| * Non-blocking notifications and unrecognized events deliberately return null. | |
| * | |
| * @param {object|null|undefined} event Normalized hook event. | |
| * @returns {string|null} A shared `STATES` value, or null for no transition. | |
| */ | |
| /** | |
| * Converts one Claude hook payload into a lighting state transition. | |
| * Unrecognized events return null. Blocking notifications return `STATES.blocked`; | |
| * `idle_prompt` returns `STATES.idle`; other non-blocking notifications return null. | |
| * | |
| * `@param` {object|null|undefined} event Normalized hook event. | |
| * `@returns` {string|null} A shared `STATES` value, or null for no transition. | |
| */ |
🤖 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/lib/thread-slots.mjs` around lines 84 - 90, Update the JSDoc for
stateFromHookEvent to explicitly document that idle_prompt is an exception to
the non-blocking notification rule and returns STATES.idle; retain the existing
null behavior description for other non-blocking or unrecognized events.
| function exportedFunctionNames(source) { | ||
| return [...source.matchAll(/export\s+(?:async\s+)?function\s+([A-Za-z_$][\w$]*)/g)] | ||
| .map((match) => match[1]) | ||
| .sort(); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Enumerate all source-only exports.
exportedFunctionNames only matches export function declarations. If prototype/src/profile-workflow.js adds export const createThing = () => {} or an exported class, the equality check remains unchanged and no JSDoc contract is required.
Collect all supported export declaration kinds. Apply the callable or constant contract check by declaration kind.
Also applies to: 153-158
🤖 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 `@tests/api-docs.test.mjs` around lines 56 - 60, Update exportedFunctionNames
and the related export-contract checks to enumerate every supported source
export declaration, including exported const bindings and classes in addition to
function declarations. Preserve the declaration kind for each match so callable
exports and constant/class exports receive their appropriate JSDoc contract
validation, ensuring newly added exports cannot bypass the equality check.
1 participant
Summary
Validation
node --test tests/api-docs.test.mjs— 10/10 passednpm run validate— profile, presets, and 46 Markdown files validatednpm run check— 109 root tests passed; 73.40% lines, 75.48% branches, 73.99% functionsImpact
Documentation and maintainability guard only; no runtime behavior changes.
Summary by CodeRabbit
Documentation
Tests