Improve logging

[rulasfia]

No description provided.

[github-actions]

Code Review Summary

Overview

This PR introduces a centralized logging package (@reafrac/logger) using Pino to standardize logging across the monorepo. Good architectural improvement for observability.

Code Quality Issues

1. Error Logging in packages/logger/src/index.ts

• Issue: log.error({ error: String(error) }, 'Error refetching feed') loses stack traces
• Line: 118, 135 in refetch-feeds.ts
• Suggestion: Pass error object directly or include stack trace: log.error({ err: error }, 'Error refetching feed')

2. Logger Context in refetch-feeds.ts

• Issue: The idx variable in map is unused after logging refactor (was used for console prefix)
• Line: 17
• Suggestion: Remove unused idx parameter or document why it's intentionally unused

3. Middleware Consistency

• Issue: requestLoggerMiddleware only logs requests, not responses/timings
• Line: apps/web/src/lib/middleware/logger-middleware.ts:7
• Suggestion: Consider adding response logging with duration for full request tracing

Potential Bugs

1. Missing Logger Dependency Installation

• Issue: The @reafrac/logger is added to package.json files but no bun install is shown to update lockfile
• Status: bun.lock diff shows changes - likely handled, but verify during merge

2. TypeScript Composite Project Setup

• Issue: packages/logger/tsconfig.json uses "composite": true but doesn't appear in root tsconfig.json references
• Suggestion: Ensure the new package is included in workspace TypeScript project references

3. Dockerfile COPY Order

• Issue: In Dockerfile stage dependencies, packages/logger/package.json is copied but the stage doesn't use --filter=@reafrac/logger during install
• Line: Dockerfile:52
• Status: This is correct since logger is a dependency package, not a build target

Suggestions for Improvement

1. Logger Child Context Enhancement
The child logger usage in refetch-feeds.ts:18 is good:

const feedLog = log.child({ feedId: feed.id, feedTitle: feed.title, feedLink: feed.link });

Consider adding correlation IDs for tracing across services.

2. Log Level Configuration
The environment variable parsing in packages/logger/src/index.ts:36-39 is solid. Consider adding:

• silent level option for testing environments
• Validation that rejects invalid log levels

3. Missing Test Coverage
No test files found in the repository. Consider adding:

• Unit tests for the logger package
• Integration tests for server logging

4. Changeset Version
The changeset declares a "major" version bump. Since this is a new package (0.1.0 → ?), this might be overkill. Consider if this should be "minor" or if this is intentional for a 1.0.0 release.

Overall Assessment

Good PR with solid architecture. The centralized logging is well-implemented with Pino. Main concerns are around error serialization (stack traces) and ensuring the new package is fully integrated into the TypeScript project references. The code is clean, follows existing patterns, and improves observability significantly.

Recommendation: Approve with minor suggestions addressed (error logging, unused variable removal).

opencode session  |  github run