Add JSDoc to pagination code #762
Conversation
WalkthroughThe pull request introduces improvements to configuration validation, middleware type safety, and documentation across multiple files. The changes focus on enhancing type annotations, initialising variables more robustly, and adding comprehensive JSDoc comments. The modifications aim to improve code clarity, prevent potential runtime errors, and provide better type checking for various utility functions and middleware components. Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
This comment was marked as resolved.
This comment was marked as resolved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/utils/pagination.js (1)
4-10: Add a short function description.While the parameter annotations are helpful, it would be clearer to include a high-level summary of what this function does. This helps maintainers and third-party developers quickly understand its purpose.
/** + * Creates an array of pages or ellipse placeholders for pagination controls. * @param {number} count * @param {number} current * @param {string} ellipsis * @returns {(number | string)[]} */config/util.js (1)
118-124: Mention logging side effects in the doc comment.The JSDoc is clear about what happens if validation fails, but it might be helpful to mention that errors are also logged prior to rethrowing them. This documents the side effects for maintainers.
/** * Validates the config object against the ConfigSchema. * * @param {*} config * @returns {v.InferOutput<typeof ConfigSchema>} * @throws {v.ValibotError} - Logs validation errors and rethrows. */src/middleware/common.middleware.js (2)
130-130: Use a structured or custom error type instead of @ts-ignore.Using
@ts-ignorehere can hide potential type mismatches. Consider extending the error class or using a typed error interface to avoid ignoring type checks.- // @ts-ignore error.status = 404 + (error as CustomError).status = 404
170-186: Consider removing the 'unexpected pagination item' warning or handling other cases.The loop currently covers
'...'and numeric items. If you anticipate other item types, handle them explicitly; otherwise, this warning might create noise in logs.for (const item of pagination(dataRange.maxPageNumber, Math.min(pageNumber, dataRange.maxPageNumber))) { if (item === '...') { ... } else if (typeof item === 'number') { ... } - logger.warn('unexpected pagination item', { item, dataRange, types: types.App, endpoint: req.originalUrl }) + // No further cases expected, consider removing or refactoring. }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
config/util.js(1 hunks)src/middleware/common.middleware.js(5 hunks)src/utils/pagination.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
🔇 Additional comments (3)
src/middleware/common.middleware.js (3)
15-15: Ensure consistent request typing.Specifying
reqasimport('express').Request & { handlerName: string }is a solid enhancement for type safety. Confirm that this updated type is consistent with all middlewares and routes that use thereq.handlerNameproperty.
150-152: Appreciate the new JSDoc type definition.Defining a clear shape for
paginationObjpromotes maintainability and makes this code more self-documenting.
156-156: Excellent choice to initialise items as an empty array.This eliminates the risk of runtime errors when pushing to
paginationObj.items. Good work.
| } else { | ||
| return { | ||
| }) | ||
| } else if (typeof item === 'number') { |
There was a problem hiding this comment.
What will happen if it's not a number? The original code suggests that the item was a string?
There was a problem hiding this comment.
The not a number case is the ellipsis string ('...'), handled in the first branch. But now that I look at it, the warning should be in an 'else'. Updated that.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/middleware/common.middleware.js (2)
130-132: Consider using a custom error class instead of@ts-ignoreRather than suppressing TypeScript errors, consider creating a custom error class that includes the status property. This would provide better type safety and maintainability.
+class HttpError extends Error { + constructor(message: string, status: number) { + super(message); + this.status = status; + } + status: number; +} -const error = new Error('page number not in range') -// @ts-ignore -error.status = 404 +const error = new HttpError('page number not in range', 404)
170-187: Enhance error logging for unexpected pagination itemsThe type checking is good, but the error logging could be more descriptive to aid debugging.
- logger.warn('unexpected pagination item', { item, dataRange, types: types.App, endpoint: req.originalUrl }) + logger.warn('unexpected pagination item type', { + item, + itemType: typeof item, + dataRange, + types: types.App, + endpoint: req.originalUrl + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/middleware/common.middleware.js(5 hunks)
🔇 Additional comments (2)
src/middleware/common.middleware.js (2)
15-15: Well-structured type annotation!The type annotation for the
reqparameter enhances type safety by explicitly defining the expected structure.
150-156: Excellent type definition and initialization!The JSDoc type definition clearly documents the pagination object structure, and initializing
itemsas an empty array prevents potential runtime errors.
What type of PR is this? (check all applicable)
Description
This PR adds some JSDoc comments and to make it easier to spot errors in the pagination code.
Related Tickets & Documents
None
Added/updated tests?
QA sign off
Design has been checked and approvedno design changesProduct and business logic has been checked and provedno logic changesSummary by CodeRabbit
Release Notes
Documentation
Improvements
New Features
These updates focus on enhancing code reliability, type safety, and documentation without introducing significant user-facing changes.