refactor: replace non-null assertions / unsafe casts with helpers#353
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Thank you for following the naming conventions! 🙏 |
Contributor
📝 WalkthroughWalkthroughWidespread runtime and test hardening: added runtime type guards, safer DOM/context access, deterministic stringify guard, logger/AI formatting hardening, updated framework integrations (Fastify, SvelteKit, ingest), and added test helpers plus mass test migrations to use them. ChangesSafety + test harness migration
Estimated code review effort: Possibly related PRs:
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
HugoRCD
changed the title
Contributor
Benchmark reportBundle size
|
commit: |
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/evlog/test/ai/ai.test.ts (1)
16-35: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winReplace local merge re-implementation with the real source helper.
Line 16 through Line 35 re-implement production merge behavior in tests (
isPlainObject/mergeInto). This can drift from runtime behavior and weaken test fidelity; import the real helper (or expose a test-safe entrypoint) instead.As per coding guidelines, "packages/evlog/test/**/*.{ts,tsx}: Always import real source helpers in tests, never re-implement them in tests."
🤖 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/ai/ai.test.ts` around lines 16 - 35, Replace the local re-implementation of isPlainObject and mergeInto in the test with the canonical helper from the production source: remove the local isPlainObject and mergeInto definitions and import the shared merge helper (the exported function that provides the same merge behavior) into packages/evlog/test/ai/ai.test.ts; if the helper is not exported for tests, add a test-safe export or entrypoint in the production module and import that instead so the test uses the real implementation rather than duplicating logic.
🤖 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/audit.ts`:
- Around line 35-37: isPlainObject is currently treating any non-null, non-array
object (Dates, Maps, class instances) as plain, which breaks canonical hashing
used by stableStringify and signed; change isPlainObject to only return true for
true plain objects (i.e., objects whose [[Class]] is "Object" and whose
prototype is Object.prototype). Specifically, update isPlainObject(value) to
check Object.prototype.toString.call(value) === '[object Object]' and
(Object.getPrototypeOf(value) === Object.prototype || value.constructor ===
Object), so Dates/Maps/instances are not treated as plain and will not be
enumerated by stableStringify/signed({ strategy: ... }). Ensure the function
name isPlainObject remains the same so callers (e.g., stableStringify and
signed) continue to use it.
In `@packages/evlog/src/logger.ts`:
- Around line 440-444: The code assumes JSON.stringify(tc.input) returns a
string and calls .length, which can throw if it returns undefined; update the
logic in the block that builds the truncated input (the variables tc, inputStr,
truncated, hasInputs, toolCallEntries, idx) to safely coerce or guard the
serialized input before checking length—e.g., compute inputStr as a guaranteed
string (use a fallback like '' or String(JSON.stringify(tc.input) || '')) or
check for undefined before accessing .length, then perform the truncation only
when inputStr is a string; apply the same guard to the other similar snippet
around lines referencing inputStr/truncated.
In `@packages/evlog/test/helpers/vite.ts`:
- Around line 28-36: The callConfigResolved helper calls plugin.configResolved
or hook.handler synchronously and doesn't await possible Promises, so change
callConfigResolved to handle async returns: make it async (or return a Promise)
and await the result of invoking plugin.configResolved (for function hooks) or
hook.handler (for object-form hooks) so the plugin's async setup completes
before tests call transform; refer to the callConfigResolved function,
plugin.configResolved, and hook.handler when making this change.
In `@packages/nuxthub/src/module.ts`:
- Around line 37-39: The code assumes config.crons is an array when creating the
crons variable and mutating it; guard it by checking Array.isArray(config.crons)
and default to an empty array if not, e.g. initialize crons only from
config.crons when it's an array so subsequent operations (findIndex on crons and
any mutation) are safe; update the initialization that defines crons and keep
the rest of the logic that computes existing (findIndex for path
'/api/_cron/evlog-cleanup') unchanged.
---
Outside diff comments:
In `@packages/evlog/test/ai/ai.test.ts`:
- Around line 16-35: Replace the local re-implementation of isPlainObject and
mergeInto in the test with the canonical helper from the production source:
remove the local isPlainObject and mergeInto definitions and import the shared
merge helper (the exported function that provides the same merge behavior) into
packages/evlog/test/ai/ai.test.ts; if the helper is not exported for tests, add
a test-safe export or entrypoint in the production module and import that
instead so the test uses the real implementation rather than duplicating logic.
🪄 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: fde77243-2338-47ec-962e-b6d9ea7e1cb3
📒 Files selected for processing (65)
apps/docs/app/components/HeroTerminalDemo.vueapps/docs/app/components/docs/DocsAsideLeftBody.vueapps/docs/app/components/docs/DocsAsideLeftBodyItem.vueapps/docs/app/components/og-image/OgImageDocs.satori.vueapps/docs/server/routes/sitemap.xml.tsapps/just-use-evlog/app/pages/index.vueapps/nuxthub-playground/app/components/AiChat.vueapps/nuxthub-playground/server/api/logs.get.tsapps/playground/app/pages/stream.vueapps/playground/server/api/audit/with-audit.post.tsapps/playground/server/api/test/better-auth/whoami.get.tsexamples/browser/src/client.tsexamples/community-adapter-skeleton/test/acme.test.tsexamples/community-framework-skeleton/src/index.tsexamples/solidstart/src/entry-client.tsxexamples/tanstack-start/src/routes/api/admin.tsexamples/tanstack-start/src/routes/api/checkout.tsexamples/tanstack-start/src/routes/api/hello.tsexamples/tanstack-start/src/routes/api/order.tsexamples/tanstack-start/src/utils/require-request-logger.tsexamples/workers/src/index.tspackages/evlog/bench/baseline/size.jsonpackages/evlog/src/audit.tspackages/evlog/src/fastify/index.tspackages/evlog/src/logger.tspackages/evlog/src/runtime/server/routes/_evlog/ingest.post.tspackages/evlog/src/shared/define.tspackages/evlog/src/shared/storage.tspackages/evlog/src/sveltekit/index.tspackages/evlog/test/README.mdpackages/evlog/test/adapters/fs.test.tspackages/evlog/test/ai/ai.test.tspackages/evlog/test/core/audit.test.tspackages/evlog/test/core/logger-request-logger.test.tspackages/evlog/test/core/logger.test.tspackages/evlog/test/core/middleware.test.tspackages/evlog/test/core/pipeline.test.tspackages/evlog/test/core/redact-integration.test.tspackages/evlog/test/core/redact.test.tspackages/evlog/test/frameworks/elysia.test.tspackages/evlog/test/frameworks/express.test.tspackages/evlog/test/frameworks/fastify.test.tspackages/evlog/test/frameworks/hono.test.tspackages/evlog/test/frameworks/nestjs-real-runtime.test.tspackages/evlog/test/frameworks/nestjs.test.tspackages/evlog/test/frameworks/react-router.test.tspackages/evlog/test/frameworks/sveltekit.test.tspackages/evlog/test/helpers/defined.tspackages/evlog/test/helpers/framework.tspackages/evlog/test/helpers/frameworkMatrix.tspackages/evlog/test/helpers/vite.tspackages/evlog/test/http/stream-server.test.tspackages/evlog/test/next/middleware.test.tspackages/evlog/test/next/storage.test.tspackages/evlog/test/nitro/errorHandler.test.tspackages/evlog/test/nitro/plugin-enrichment.test.tspackages/evlog/test/nitro/plugin.test.tspackages/evlog/test/vite/auto-imports.test.tspackages/evlog/test/vite/client-inject.test.tspackages/evlog/test/vite/source-location.test.tspackages/evlog/test/vite/strip.test.tspackages/nuxthub/src/config.tspackages/nuxthub/src/module.tspackages/nuxthub/src/runtime/drain.tspackages/nuxthub/src/runtime/tasks/evlog-cleanup.ts
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/src/logger.ts`:
- Around line 367-370: The serializeToolInput function currently calls
JSON.stringify which can throw on circular structures or unsupported types
(e.g., BigInt); wrap the JSON.stringify call in a try/catch and provide a safe
fallback (for example, use a replacer to convert BigInt to strings and
detect/replace circular refs, or on error return a deterministic fallback like
util.inspect(input) or String(input) with an explanatory marker). Update
serializeToolInput to attempt JSON.stringify(input) inside try, handle known
edge cases (BigInt -> toString) or catch exceptions and return a safe string so
serialization never throws.
🪄 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: ae31550d-0ed4-489d-9d09-a995877b0d1f
📒 Files selected for processing (10)
packages/evlog/src/audit.tspackages/evlog/src/logger.tspackages/evlog/test/ai/ai.test.tspackages/evlog/test/core/audit.test.tspackages/evlog/test/helpers/vite.tspackages/evlog/test/vite/auto-imports.test.tspackages/evlog/test/vite/client-inject.test.tspackages/evlog/test/vite/source-location.test.tspackages/evlog/test/vite/strip.test.tspackages/nuxthub/src/module.ts
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/core/logger.test.ts`:
- Around line 1160-1162: Add a logger reset to the existing afterEach block:
call initLogger({ pretty: false }) in the afterEach alongside
vi.restoreAllMocks() so the global logger state is returned to defaults after
each test; reference the existing afterEach and the initLogger function to
locate where to add the call.
🪄 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: 6f337a1c-8201-4a1e-96a2-e11f78dddcfd
📒 Files selected for processing (2)
packages/evlog/src/logger.tspackages/evlog/test/core/logger.test.ts
Comment on lines
+1160
to
+1162
| afterEach(() => { | ||
| vi.restoreAllMocks() | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider resetting logger state in afterEach for consistency.
Other test suites in this file (e.g., enabled option, silent option) call initLogger({ pretty: false }) in afterEach to reset global logger state. While this test suite is currently last in the file, adding the reset would be more robust against future test additions or reordering.
Suggested change
afterEach(() => {
vi.restoreAllMocks()
+ initLogger({ pretty: false })
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| afterEach(() => { | |
| vi.restoreAllMocks() | |
| }) | |
| afterEach(() => { | |
| vi.restoreAllMocks() | |
| initLogger({ pretty: false }) | |
| }) |
🤖 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/core/logger.test.ts` around lines 1160 - 1162, Add a
logger reset to the existing afterEach block: call initLogger({ pretty: false })
in the afterEach alongside vi.restoreAllMocks() so the global logger state is
returned to defaults after each test; reference the existing afterEach and the
initLogger function to locate where to add the call.
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