-
Notifications
You must be signed in to change notification settings - Fork 0
feat: 🎨 improve logging and error handling #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughLogger integration is upgraded from root app level to a plugin-based approach with signal handling (SIGINT/SIGTERM). A pino dependency is added, error logging enhanced with structured HTTP context, and the users module improved with explicit error handling via try/catch blocks and expanded route metadata. Documentation clarifies logger usage patterns. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
🔇 Additional comments (1)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
README.md (1)
137-149: Logger usage notes align with implementation; consider adding a concrete call exampleThe guidance about initializing the logger in
src/main.tsand importing it fromsrc/common/loggerin submodules matches the code and should help avoid duplicate registrations. As a small doc improvement, you might add a one‑line example likelog.info('Created user', { userId });under the “✅ Correct” snippet so readers immediately see the intended call pattern.src/main.ts (1)
17-32: Global onError logging is solid; consider whether you want a standardized client error shapeThe structured log (code +
err+ HTTP method/url/referrer) is very useful for debugging and lines up with Elysia’sonErrorusage. Returningerrorhere lets Elysia apply its default error mapping, which is valid, but it also means the exact client‑visible error shape is driven by the framework and the specific error type. If you eventually want tighter control (e.g., always returning a{ message, code }payload by error code, or hiding internal messages in production), this hook is the right place to centralize that instead of returning the raw error.src/modules/users/index.ts (1)
1-2: Route‑level error handling is clearer now; consider logging the root cause and distinguishing expected vs unexpected errorsThe new handlers read well: success paths log with
log.info, response schemas are explicit (200/422), and usingstatus(422, …)withUsersModel.*Errorplussatisfieskeeps things nicely typed.Two improvements to consider:
Log the underlying error before rethrowing 422
Right now the original failure fromUsersService.create/getis discarded, and the globalonErroronly ever sees the 422statuserror. Logging the caught error here would preserve the root cause for ops/debugging:// POST } catch (error) { log.error({ err: error }, 'Failed to create user'); throw status(422, { message: 'Failed to create user' satisfies UsersModel.createError, }); } // GET } catch (error) { log.error({ err: error }, 'Failed to get users'); throw status(422, { message: 'Failed to get users' satisfies UsersModel.getError, }); }Optionally, distinguish validation/business errors from unexpected server errors
IfUsersServicecan throw both “expected” domain/validation errors and hard failures (DB outages, coding bugs, etc.), you may want to narrow the catch to known types (e.g., custom errors orstatus(422, …)thrown from the service) and let unknown exceptions bubble to the globalonErroras 5xx instead of always returning 422.Together these keep the external contract (typed 422 responses) while making observability and HTTP semantics more robust.
Also applies to: 10-19, 36-44
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)README.md(1 hunks)package.json(1 hunks)src/main.ts(2 hunks)src/modules/users/index.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main.ts (1)
src/common/logger.ts (1)
log(4-19)
🔇 Additional comments (3)
CHANGELOG.md (1)
8-15: Unreleased changelog entry clearly reflects the logging/error‑handling workThe new
[Unreleased]section accurately captures the logger/plugin integration, signal handling, and users‑module error handling improvements you’re introducing. No changes needed here.src/main.ts (1)
51-57: Decoupled app.listen from the plugin chain; startup logging remains clearMoving
app.listenout of the method chain keeps the lifecycle configuration (plugins, hooks, routes) separate from startup concerns, and the callback still logs hostname/port plus thedevelopmentflag, so behavior remains clear and easy to extend (e.g., adding more startup diagnostics later).package.json (1)
24-25: Pino dependency is fully compatible with elysia-logger peer requirementsVerification confirms that
pino@^10.1.0satisfies@bogeychan/elysia-logger@^0.1.10's peer dependency requirement ofpino >= 9.6.0. No compatibility issues exist. The dependency addition is safe to proceed.
1797fa9 to
7da2aad
Compare
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.