docs(guardrails): enforce JSDoc on every new or modified function#3555
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a JSDoc documentation requirement to the project's AI guardrails, mandating that every new or modified function must have a JSDoc header with description, parameter documentation, and return value documentation. The change responds to recent PRs where functions were added without proper documentation.
Changes:
- Added JSDoc requirement to CLAUDE.md guardrails section
- Added corresponding entry to ERRORS.md for AI mistake tracking
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| CLAUDE.md | Adds JSDoc requirement to "Always-on guardrails" section |
| ERRORS.md | Records the recurring mistake of missing JSDoc headers |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3555 +/- ##
=======================================
Coverage 95.93% 95.93%
=======================================
Files 20 20
Lines 516 516
Branches 140 140
=======================================
Hits 495 495
Misses 21 21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@copilot review |
|
@PierreBrisorgueil I've opened a new pull request, #3556, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@copilot review |
|
@PierreBrisorgueil I've opened a new pull request, #3557, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@copilot review |
|
@PierreBrisorgueil I've opened a new pull request, #3558, to work on those changes. Once the pull request is ready, I'll request review from you. |
… consistent Markdown rendering
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
src/lib/helpers/tools.js:14
- This JSDoc
@paramentry is missing the parameter name (should documentrelease), and the@returnstype/value looks incorrect: the function returns an array (aftersplit('.')/pop()), not a string like "23". Please correct the JSDoc to match the actual signature and return type.
* @desc Function to evaluate numbers of release from last release
* @param {String} 2.3.4
* @returns {String} 23
*/
export const releasesNumber = (release) => {
const numbers = release[0] === 'v' ? release.substr(1).split('.') : release.split('.'); // get numbers last release
numbers.pop();
numbers[0] = numbers[0] === '1' ? '1' : String(parseInt(numbers[0], 10) * 10); // calc aproximativly number of release
return numbers;
- theme.js: add param names to isDark and style, document both args - tools.js: fix releasesNumber param name and Array return type - tools.js: fix serverItemsLength @returns to number - model.js: fix description (null only) and improve @param/@returns - CLAUDE.md, copilot-instructions.md: remove /pr row (wrong PR)
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
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. 📝 WalkthroughWalkthroughAdds a requirement for JSDoc headers for new/modified functions across docs; normalizes JSDoc tags and clarifies descriptions in helper and middleware files; updates tests to match adjusted Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/helpers/theme.js (1)
3-10:⚠️ Potential issue | 🟡 MinorJSDoc/behavior mismatch risk for
isDark.Line 3–5 states
themecan be'dark' | 'light' | 'auto'and returns a dark value, but the implementation (Line 10) returnstruefor any truthy non‑'auto'value (so'light'becomestrue). If'light'should map tofalse, please adjust logic or tighten the doc to reflect current behavior.Suggested logic fix (only if 'light' should be false)
- return !!theme; + return theme === true || theme === 'dark';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/helpers/theme.js` around lines 3 - 10, The isDark function currently treats any truthy non-'auto' theme as dark, which makes 'light' return true; update isDark so it explicitly returns true only for theme === 'dark', returns the prefers-color-scheme check when theme === 'auto', and returns false for theme === 'light' or any other value (adjust the JSDoc on the top of the function to match this behavior if needed); locate and modify the exported function isDark in src/lib/helpers/theme.js.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/lib/helpers/theme.js`:
- Around line 3-10: The isDark function currently treats any truthy non-'auto'
theme as dark, which makes 'light' return true; update isDark so it explicitly
returns true only for theme === 'dark', returns the prefers-color-scheme check
when theme === 'auto', and returns false for theme === 'light' or any other
value (adjust the JSDoc on the top of the function to match this behavior if
needed); locate and modify the exported function isDark in
src/lib/helpers/theme.js.
| * @param {String} theme - Theme option from config ('dark', 'light', or 'auto') | ||
| * @returns {Boolean} true if dark mode is active, false otherwise | ||
| */ | ||
| export const isDark = (theme) => { |
There was a problem hiding this comment.
The JSDoc for isDark documents theme as {String}, but the config comment indicates it can also be a boolean. Once boolean handling is restored, please update the param type/description to reflect the accepted union (e.g. Boolean|String).
| * @param {String} theme - Theme option from config ('dark', 'light', or 'auto') | |
| * @returns {Boolean} true if dark mode is active, false otherwise | |
| */ | |
| export const isDark = (theme) => { | |
| * @param {Boolean|String} theme - Theme option from config: boolean (true = dark, false = light) or string ('dark', 'light', or 'auto') | |
| * @returns {Boolean} true if dark mode is active, false otherwise | |
| */ | |
| export const isDark = (theme) => { | |
| if (typeof theme === 'boolean') { | |
| return theme; | |
| } |
| it('should return true when theme is dark', () => { | ||
| expect(isDark('dark')).toBe(true); | ||
| }); | ||
|
|
||
| it('should return false when theme is explicitly false', () => { | ||
| expect(isDark(false)).toBe(false); | ||
| it('should return false when theme is light', () => { | ||
| expect(isDark('light')).toBe(false); | ||
| }); | ||
|
|
||
| it('should return false when theme is undefined', () => { | ||
| it('should return false for unknown or missing theme (light by default)', () => { | ||
| expect(isDark(undefined)).toBe(false); | ||
| }); | ||
|
|
||
| it('should return false when theme is null', () => { | ||
| expect(isDark(null)).toBe(false); | ||
| expect(isDark('')).toBe(false); |
There was a problem hiding this comment.
PR description says isDark should handle both string and boolean theme values, but the updated tests only cover string/undefined/null cases. Add boolean test cases (true/false) to prevent regressions and to reflect the documented config behavior.
| * @param {String} 2.3.4 | ||
| * @return {String} 23 | ||
| * @param {String} release - Version string (e.g. '2.3.4' or 'v2.3.4') | ||
| * @returns {Array} Array of version number parts |
There was a problem hiding this comment.
The releasesNumber JSDoc says it returns {Array}, but the function actually returns an array of strings (e.g. ['20','3']) and always drops the patch segment. Consider documenting this more precisely (e.g. Array<String> and mention that the result is [majorAdjusted, minor]).
| * @returns {Array} Array of version number parts | |
| * @returns {Array<String>} Array of version number parts as strings: [majorAdjusted, minor] (patch segment is dropped) |
| * @return {Object} result | ||
| * @desc Function to clean object (pick from model, remove null values) | ||
| * @param {Object} data - Source object to clean | ||
| * @param {Array} model - Array of keys to pick from data |
There was a problem hiding this comment.
In clean, the model param is documented as {Array}, but it’s expected to be an array of field names. Consider tightening this to Array<String> (or similar) to make the contract clear and help catch misuse in editors/IDE tooling.
| * @param {Array} model - Array of keys to pick from data | |
| * @param {Array<string>} model - Array of keys to pick from data |
Summary
Why
Recent PRs introduced functions without JSDoc headers. This rule makes it an always-on constraint for all AI interactions rather than a post-hoc catch.
Scope
Docs + JSDoc migration in src/lib — no business logic change (isDark logic fix aligns implementation with documented behavior)
Summary by CodeRabbit
Release Notes
Documentation
Refactor
Tests