fix(core): prettyPrintingPacket will now do a structuredClone on non-circular references instead of outputting [Circular] #2508
Conversation
|
Walkthrough
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
![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 (3)
packages/core/src/v3/utils/ioSerialization.ts (3)
446-449: Consider the performance and memory implications of structuredClone.Using
structuredClone()for non-circular references is a good approach, but it may have performance implications for large objects. Consider documenting this behavior change in release notes as it could affect memory usage for deeply nested structures.Consider adding a depth limit or size check before using
structuredClone()for very large objects:if (options?.cloneReferences) { + // Avoid cloning extremely large objects that might cause memory issues + try { return structuredClone(value); + } catch (e) { + // Fall back to [Circular] if cloning fails + return "[Circular]"; + } }
638-665: Consider edge cases in circular pattern detection.The circular pattern detection logic is good but might miss some edge cases. For example, it assumes that circular references always have the same last segment, which might not always be true for complex object graphs.
Consider enhancing the circular detection to handle more complex patterns:
function isCircularPattern(targetPath: string, referencePath: string): boolean { const targetParts = targetPath.split("."); const refParts = referencePath.split("."); // For circular references, the reference path often contains the target path as a prefix // Example: targetPath="user", referencePath="user.details.user" // This means user.details.user points back to user (circular) // Check if reference path starts with target path + additional segments that loop back if (refParts.length > targetParts.length) { // Check if reference path starts with target path let isPrefix = true; for (let i = 0; i < targetParts.length; i++) { if (targetParts[i] !== refParts[i]) { isPrefix = false; break; } } // If reference path starts with target path and ends with target path, // it's likely a circular reference (e.g., "user" -> "user.details.user") if (isPrefix && refParts[refParts.length - 1] === targetParts[targetParts.length - 1]) { return true; } } + + // Also check if the reference path IS the target path (direct circular reference) + if (targetPath === referencePath) { + return true; + } return false; }
395-407: Add an explicit SuperJSON type-guard before accessing rawData.metarawData && rawData.meta avoids crashes but doesn't verify the SuperJSON shape — add a small isSuperJSON(rawData) predicate (ensure rawData is an object, rawData.meta is an object, and expected keys like referentialEqualities exist) and use it to decide cloneReferences.
Location: packages/core/src/v3/utils/ioSerialization.ts:395-407.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
references/hello-world/src/trigger/circularPayload.tsis excluded by!references/**
📒 Files selected for processing (3)
apps/webapp/app/routes/resources.taskruns.$runParam.replay.ts(2 hunks)packages/core/src/v3/utils/ioSerialization.ts(5 hunks)packages/core/test/ioSerialization.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
packages/core/test/ioSerialization.test.tsapps/webapp/app/routes/resources.taskruns.$runParam.replay.tspackages/core/src/v3/utils/ioSerialization.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Our tests are all vitest
Files:
packages/core/test/ioSerialization.test.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
packages/core/test/ioSerialization.test.tsapps/webapp/app/routes/resources.taskruns.$runParam.replay.tspackages/core/src/v3/utils/ioSerialization.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.{ts,tsx,js,jsx}: Unit tests must use Vitest
Tests should avoid mocks or stubs and use helpers from @internal/testcontainers when Redis or Postgres are needed
Test files live beside the files under test and should use descriptive describe and it blocks
Files:
packages/core/test/ioSerialization.test.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/app/routes/resources.taskruns.$runParam.replay.ts
{apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly
Files:
apps/webapp/app/routes/resources.taskruns.$runParam.replay.ts
apps/webapp/app/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead
Files:
apps/webapp/app/routes/resources.taskruns.$runParam.replay.ts
🧬 Code graph analysis (2)
packages/core/test/ioSerialization.test.ts (1)
packages/core/src/v3/utils/ioSerialization.ts (1)
prettyPrintPacket(379-433)
apps/webapp/app/routes/resources.taskruns.$runParam.replay.ts (2)
packages/core/src/v3/utils/ioSerialization.ts (1)
prettyPrintPacket(379-433)packages/core/src/v3/index.ts (1)
prettyPrintPacket(66-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
apps/webapp/app/routes/resources.taskruns.$runParam.replay.ts (1)
111-111: Extract prettyPrintPacket() outside the object literal for clarity.Good refactoring choice. Moving the
prettyPrintPacketcall to a separate variable makes the code more readable and ensures single evaluation of the async function before returning the response.packages/core/test/ioSerialization.test.ts (1)
192-346: Comprehensive test coverage for prettyPrintPacket functionality.Well-structured test suite covering all major scenarios including undefined input, string data, JSON/SuperJSON handling, circular references, filtered keys, and special types (BigInt, RegExp, Set, Map). The tests properly validate the public API changes and the new circular reference handling behavior.
packages/core/src/v3/utils/ioSerialization.ts (2)
415-425: Smart fallback for circular reference handling errors.Excellent defensive programming! The try-catch with
cloneReferencesfallback ensures graceful handling of edge cases where circular reference detection might be incomplete or incorrect. This prevents runtime errors while still attempting to provide useful output.
590-665: Well-implemented circular reference detection logic.The
hasCircularReference()andisCircularPattern()functions correctly handle all three SuperJSON referential equality formats. The implementation is thorough and includes detailed comments explaining the different cases.
…circular references instead of outputting [Circular] This also fixes an issue with replaying of runs that include non-circular references
ericallam
force-pushed
the
circular-reference-replay-fix
branch
from
55d5a09 to
ff7be34
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
apps/webapp/test/fairDequeuingStrategy.test.ts (1)
273-275: Use tolerance to reduce flakiness for timing assertion.Direct less‑than comparisons on timing can be noisy. Consider allowing a small tolerance like you do elsewhere.
Apply:
- // Make sure the second call is faster than the first - expect(distribute2Duration).toBeLessThan(distribute1Duration); + // Make sure the second call is faster than the first (within tolerance) + expect(distribute2Duration).toBeLessThan(withTolerance(distribute1Duration));packages/core/test/ioSerialization.test.ts (2)
254-267: Strengthen non‑circular reference test: assert absence of “[Circular]”.This guarantees the new clone‑on‑shared‑refs path is exercised.
Apply:
const result = await prettyPrintPacket(serialized, "application/super+json"); expect(result).toContain('"person1": {'); expect(result).toContain('"person2": {'); + expect(result).not.toContain("[Circular]");
218-238: Avoid brittle whitespace assertions.String contains with exact indentation can be fragile. Prefer parsing the output and asserting structure (e.g., JSON.parse for JSON cases) or using regex without whitespace coupling.
packages/core/src/v3/utils/ioSerialization.ts (2)
388-406: Guard against invalid SuperJSON input before deserialize.safeJsonParse can return undefined; calling deserialize on undefined will throw. Early‑return the raw string to keep prettyPrintPacket resilient.
Apply:
if (dataType === "application/super+json") { - if (typeof rawData === "string") { - rawData = safeJsonParse(rawData); - } + if (typeof rawData === "string") { + const parsed = safeJsonParse(rawData); + if (!parsed) return rawData; // invalid JSON; return as-is + rawData = parsed; + } const { deserialize } = await loadSuperJSON(); const hasCircularReferences = rawData && rawData.meta && hasCircularReference(rawData.meta);
446-449: structuredClone may be unavailable or may throw; add safe fallback.Older runtimes or uncloneable values can cause failures. Fall back to a best‑effort deep copy, then to “[Circular]” if needed.
Apply:
- if (options?.cloneReferences) { - return structuredClone(value); - } + if (options?.cloneReferences) { + try { + // Prefer native clone when available + // @ts-ignore - guard at runtime + if (typeof structuredClone === "function") return structuredClone(value); + // Fallback: JSON round‑trip (loses non‑JSON types but avoids crash) + return JSON.parse(JSON.stringify(value)); + } catch { + // Last resort: don’t explode, mark as circular + return "[Circular]"; + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
references/hello-world/src/trigger/circularPayload.tsis excluded by!references/**
📒 Files selected for processing (4)
apps/webapp/app/routes/resources.taskruns.$runParam.replay.ts(2 hunks)apps/webapp/test/fairDequeuingStrategy.test.ts(2 hunks)packages/core/src/v3/utils/ioSerialization.ts(5 hunks)packages/core/test/ioSerialization.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webapp/app/routes/resources.taskruns.$runParam.replay.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
apps/webapp/test/fairDequeuingStrategy.test.tspackages/core/test/ioSerialization.test.tspackages/core/src/v3/utils/ioSerialization.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Our tests are all vitest
Files:
apps/webapp/test/fairDequeuingStrategy.test.tspackages/core/test/ioSerialization.test.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
apps/webapp/test/fairDequeuingStrategy.test.tspackages/core/test/ioSerialization.test.tspackages/core/src/v3/utils/ioSerialization.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/test/fairDequeuingStrategy.test.ts
{apps/webapp/**/__tests__/**/*.{ts,tsx},apps/webapp/**/*.{test,spec}.{ts,tsx}}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
{apps/webapp/**/__tests__/**/*.{ts,tsx},apps/webapp/**/*.{test,spec}.{ts,tsx}}: Do not import app/env.server.ts into tests, either directly or indirectly
Tests should only import classes/functions from files under apps/webapp/app/**/*.ts
Files:
apps/webapp/test/fairDequeuingStrategy.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.{ts,tsx,js,jsx}: Unit tests must use Vitest
Tests should avoid mocks or stubs and use helpers from @internal/testcontainers when Redis or Postgres are needed
Test files live beside the files under test and should use descriptive describe and it blocks
Files:
apps/webapp/test/fairDequeuingStrategy.test.tspackages/core/test/ioSerialization.test.ts
🧬 Code graph analysis (1)
packages/core/test/ioSerialization.test.ts (1)
packages/core/src/v3/utils/ioSerialization.ts (1)
prettyPrintPacket(379-433)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
packages/core/test/ioSerialization.test.ts (1)
192-346: Comprehensive coverage of prettyPrintPacket. LGTM.packages/core/src/v3/utils/ioSerialization.ts (2)
415-425: Good fallback on stringify failure.Retrying with cloneReferences: false is a sensible safety net if detection ever misclassifies.
601-666: Circular detection heuristics look reasonable; add tests for edge patterns.The isCircularPattern prefix/suffix check could miss or misclassify complex paths (e.g., sibling cross‑links or deeper ancestor hops). Add cases like target="a.b.c", ref="a.b.c.d.a.b" and non‑circular shared refs across branches.
I can draft additional unit tests covering tricky referentialEqualities shapes if helpful.
2 participants
This also fixes an issue with replaying of runs that include
non-circular references