fix: resolve typecheck and lint errors across the codebase#352
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Thank you for following the naming conventions! 🙏 |
Contributor
📝 WalkthroughWalkthroughThis PR refactors the evlog TypeScript type system to improve safety and consistency. Changes include introducing explicit ChangesType system and interface alignment
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
Contributor
Benchmark reportBundle size
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/evlog/src/utils.ts (1)
127-137: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd JSDoc for the updated public API.
filterSafeHeadersis exported and was changed, but it still has no JSDoc block.Proposed fix
+/** + * Returns a copy of headers excluding sensitive keys and undefined values. + */ export function filterSafeHeaders(headers: Partial<Record<string, string | undefined>>): Record<string, string> { const safeHeaders: Record<string, string> = {} for (const [key, value] of Object.entries(headers)) { if (value !== undefined && !SENSITIVE_HEADERS.includes(key.toLowerCase())) { safeHeaders[key] = value } } return safeHeaders }As per coding guidelines,
packages/evlog/src/**/*.{ts,tsx}: Add JSDoc comments on all public APIs.🤖 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 `@packages/evlog/src/utils.ts` around lines 127 - 137, Add a JSDoc comment for the exported function filterSafeHeaders so it documents purpose, parameters and return value; specifically, above the filterSafeHeaders declaration describe what the function does (filters out undefined and sensitive headers), document the headers parameter type and that keys/values are strings (Partial<Record<string,string|undefined>>), and document the returned Record<string,string> of only safe headers; include any relevant tags like `@param` and `@returns` and mark it as part of the public API.
🤖 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 `@packages/evlog/src/nestjs/index.ts`:
- Line 36: The type declares Request.log as required but the middleware may exit
early before assigning req.log; make the type safe by changing the declaration
to optional (e.g., Request.log?: RequestLogger) and update any call sites that
access req.log to handle undefined (guard with if (req.log) or provide a
fallback), or alternatively ensure the middleware always assigns req.log before
any early return; update the Request interface and all references to Request.log
/ req.log / RequestLogger accordingly so the runtime and types match.
In `@packages/evlog/test/core/logger-request-logger.test.ts`:
- Around line 46-47: The test is bypassing type safety by forcing string values
into the numeric-only status field; update the overwrite test to use a
non-reserved, neutral key instead (e.g., replace logger.set({ status: 'pending'
as unknown as number }) and logger.set({ status: 'complete' as unknown as number
}) with logger.set({ phase: 'pending' }) and logger.set({ phase: 'complete' })
), adjusting any assertions that check these values to reference phase rather
than status so the test no longer masks type contract regressions; keep using
the same logger.set function name and test flow.
In `@packages/evlog/test/frameworks/fastify.test.ts`:
- Line 38: The test is reading request.log.set without guarding against
undefined; update the boolean guards (e.g., the hasLogger check and the
include/exclude logger checks) to use optional chaining and a safe typeof check
such as verifying typeof request.log?.set === 'function' (or request.log != null
&& typeof request.log?.set === 'function') so accessing .set won't throw if
request.log is undefined; apply this change to the hasLogger variable and the
include/exclude logger checks referenced in the Fastify test.
---
Outside diff comments:
In `@packages/evlog/src/utils.ts`:
- Around line 127-137: Add a JSDoc comment for the exported function
filterSafeHeaders so it documents purpose, parameters and return value;
specifically, above the filterSafeHeaders declaration describe what the function
does (filters out undefined and sensitive headers), document the headers
parameter type and that keys/values are strings
(Partial<Record<string,string|undefined>>), and document the returned
Record<string,string> of only safe headers; include any relevant tags like
`@param` and `@returns` and mark it as part of the public API.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9dab7f2e-f06f-46df-bb55-6d5e8b55e836
📒 Files selected for processing (31)
packages/evlog/src/adapters/axiom.tspackages/evlog/src/audit.tspackages/evlog/src/catalog.tspackages/evlog/src/fastify/index.tspackages/evlog/src/nestjs/index.tspackages/evlog/src/next/instrumentation.tspackages/evlog/src/next/middleware.tspackages/evlog/src/shared/middleware.tspackages/evlog/src/sveltekit/index.tspackages/evlog/src/utils.tspackages/evlog/test/adapters/axiom.test.tspackages/evlog/test/adapters/better-stack.test.tspackages/evlog/test/ai/ai.test.tspackages/evlog/test/better-auth/better-auth.test.tspackages/evlog/test/core/logger-request-logger.test.tspackages/evlog/test/core/logger.test.tspackages/evlog/test/frameworks/elysia.test.tspackages/evlog/test/frameworks/fastify.test.tspackages/evlog/test/frameworks/nestjs.test.tspackages/evlog/test/frameworks/sveltekit.test.tspackages/evlog/test/helpers/framework.tspackages/evlog/test/http/http.test.tspackages/evlog/test/next/handler.test.tspackages/evlog/test/next/middleware.test.tspackages/evlog/test/next/stream.test.tspackages/evlog/test/nitro-v3/cloudflare-durable-build.test.tspackages/evlog/test/nitro/errorHandler.test.tspackages/evlog/test/nitro/plugin-enrichment.test.tspackages/evlog/test/nitro/plugin.test.tspackages/evlog/test/toolkit/toolkit.test.tspackages/evlog/tsconfig.json
HugoRCD
force-pushed
the
fix/typecheck-lint
branch
from
96ae7bf to
b8b441a
Compare
commit: |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/evlog/test/next/stream.test.ts`:
- Line 76: Replace the repeated chained unknown casts by adding a small typed
helper (e.g. lastMockArg<T>(mockFn: { calls: unknown[][] }, argIndex = 0): T |
undefined) and use it to obtain the last createInstrumentation call argument;
for example call lastMockArg<ExpectedType>(createInstrumentation, 0) instead of
the long cast used in the tests (the symbol to update is createInstrumentation
and the tests that assign lastCall on lines using that cast). This centralizes
the cast, makes types explicit, and reuse it for the other occurrences at 91,
103 and 115.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bcf41dc3-6c99-4327-aeae-0c28a5fcbbd9
📒 Files selected for processing (33)
examples/tanstack-start/nitro.config.tsexamples/tanstack-start/vite.config.tspackages/evlog/src/adapters/axiom.tspackages/evlog/src/audit.tspackages/evlog/src/catalog.tspackages/evlog/src/express/index.tspackages/evlog/src/fastify/index.tspackages/evlog/src/next/instrumentation.tspackages/evlog/src/next/middleware.tspackages/evlog/src/shared/middleware.tspackages/evlog/src/sveltekit/index.tspackages/evlog/src/utils.tspackages/evlog/test/adapters/axiom.test.tspackages/evlog/test/adapters/better-stack.test.tspackages/evlog/test/ai/ai.test.tspackages/evlog/test/better-auth/better-auth.test.tspackages/evlog/test/core/logger-request-logger.test.tspackages/evlog/test/core/logger.test.tspackages/evlog/test/frameworks/elysia.test.tspackages/evlog/test/frameworks/fastify.test.tspackages/evlog/test/frameworks/nestjs.test.tspackages/evlog/test/frameworks/sveltekit.test.tspackages/evlog/test/helpers/framework.tspackages/evlog/test/http/http.test.tspackages/evlog/test/next/handler.test.tspackages/evlog/test/next/middleware.test.tspackages/evlog/test/next/stream.test.tspackages/evlog/test/nitro-v3/cloudflare-durable-build.test.tspackages/evlog/test/nitro/errorHandler.test.tspackages/evlog/test/nitro/plugin-enrichment.test.tspackages/evlog/test/nitro/plugin.test.tspackages/evlog/test/toolkit/toolkit.test.tspackages/evlog/tsconfig.json
|
|
||
| expect(createInstrumentation).toHaveBeenCalled() | ||
| const lastCall = createInstrumentation.mock.calls.at(-1)![0] as { drain?: (ctx: unknown) => Promise<void> } | ||
| const lastCall = ((createInstrumentation.mock.calls.at(-1)!) as unknown as [unknown])[0] as unknown as { drain?: (ctx: unknown) => Promise<void> } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Replace repeated unknown cast chains with a typed mock-call helper.
The current extraction pattern is hard to read and fragile. Use one typed helper for the last createInstrumentation arg to keep tests safer and maintainable.
♻️ Proposed refactor
+type InstrumentationArg = { drain?: (ctx: unknown) => Promise<void> }
+
+function getLastCreateInstrumentationArg(): InstrumentationArg {
+ const call = createInstrumentation.mock.calls.at(-1)
+ expect(call).toBeDefined()
+ return call![0] as InstrumentationArg
+}
+
it('composes user drain and server drain so both run on each event', async () => {
@@
- const lastCall = ((createInstrumentation.mock.calls.at(-1)!) as unknown as [unknown])[0] as unknown as { drain?: (ctx: unknown) => Promise<void> }
+ const lastCall = getLastCreateInstrumentationArg()
@@
it('uses only user drain when server is off', async () => {
@@
- const lastCall = ((createInstrumentation.mock.calls.at(-1)!) as unknown as [unknown])[0] as unknown as { drain?: (ctx: unknown) => Promise<void> }
+ const lastCall = getLastCreateInstrumentationArg()
@@
it('uses only server drain when no user drain is configured', async () => {
@@
- const lastCall = ((createInstrumentation.mock.calls.at(-1)!) as unknown as [unknown])[0] as unknown as { drain?: (ctx: unknown) => Promise<void> }
+ const lastCall = getLastCreateInstrumentationArg()
@@
it('omits drain entirely when neither side is configured', async () => {
@@
- const lastCall = ((createInstrumentation.mock.calls.at(-1)!) as unknown as [unknown])[0] as unknown as { drain?: unknown }
+ const lastCall = getLastCreateInstrumentationArg()For Vitest 4.x, what is the recommended typed pattern for reading the latest mock call argument from `vi.fn().mock.calls` without chained `unknown` casts?
Also applies to: 91-91, 103-103, 115-115
🤖 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 `@packages/evlog/test/next/stream.test.ts` at line 76, Replace the repeated
chained unknown casts by adding a small typed helper (e.g.
lastMockArg<T>(mockFn: { calls: unknown[][] }, argIndex = 0): T | undefined) and
use it to obtain the last createInstrumentation call argument; for example call
lastMockArg<ExpectedType>(createInstrumentation, 0) instead of the long cast
used in the tests (the symbol to update is createInstrumentation and the tests
that assign lastCall on lines using that cast). This centralizes the cast, makes
types explicit, and reuse it for the other occurrences at 91, 103 and 115.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Linked issue
📚 Description
📝 Checklist
Summary by CodeRabbit
Release Notes
Refactor
Tests
Chores