fix(core): escape dotted keys in flattenAttributes to correct log rendering #2987
Conversation
🦋 Changeset detectedLatest commit: a8fa6a5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 28 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis PR fixes logging behavior for objects containing dotted keys in their attribute names. The changes introduce helper functions to escape dots and backslashes in keys and parse dot-notated paths with escape support. The flattenAttributes function signature is updated to include an optional maxAttributeCount parameter, and the flattening logic is adjusted to use the new escapeKey helper for proper key handling. The unflattenAttributes function now uses the new splitPath helper instead of a naive split operation to correctly handle escaped separators. Tests are updated with formatting adjustments and Vitest imports. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔴 Map keys containing dots are not escaped, causing incorrect unflattening
The PR fixes dotted keys for regular objects by using escapeKey() at line 233, but Map keys at line 150 are not escaped.
Click to expand
How the bug is triggered
When a Map has a key containing a dot (e.g., "Key 0.002mm"), the current code builds the path without escaping:
// Line 150 - Map keys are NOT escaped
this.#processValue(value, `${prefix || "map"}.${keyStr}`, depth);Compare to regular object keys at line 233:
// Line 233 - Object keys ARE escaped
const newPrefix = `${prefix ? `${prefix}.` : ""}${Array.isArray(obj) ? `[${key}]` : escapeKey(key)}`;Actual vs Expected behavior
Actual: Map with key "Key 0.002mm" produces flattened path myMap.Key 0.002mm, which splitPath() splits into ['myMap', 'Key 0', '002mm'], resulting in nested object { myMap: { 'Key 0': { '002mm': value } } }
Expected: Path should be myMap.Key 0\.002mm, which splitPath() correctly splits into ['myMap', 'Key 0.002mm'], preserving the original structure { myMap: { 'Key 0.002mm': value } }
Impact
This causes data corruption during the flatten/unflatten cycle for Maps with dotted keys - the exact same issue the PR was trying to fix, but only fixed for regular objects.
(Refers to line 150)
Recommendation: Escape the Map key using escapeKey(): this.#processValue(value, \${prefix || "map"}.${escapeKey(keyStr)}`, depth);`
Was this helpful? React with 👍 or 👎 to provide feedback.
Review CompleteYour review story is ready! Comment !reviewfast on this PR to re-generate the story. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/v3/utils/flattenAttributes.ts (2)
144-153:⚠️ Potential issue | 🟠 MajorMap keys containing dots are not escaped.
Map key handling at line 150 directly interpolates the key without escaping. If a Map contains a key like
"a.b", it will flatten to"map.a.b"and unflatten incorrectly as a nested structure{map: {a: {b: ...}}}.This is inconsistent with the fix applied to regular object keys at line 233.
🐛 Proposed fix to escape Map keys
// Use the key directly if it's a string, otherwise convert it const keyStr = typeof key === "string" ? key : String(key); - this.#processValue(value, `${prefix || "map"}.${keyStr}`, depth); + this.#processValue(value, `${prefix || "map"}.${escapeKey(keyStr)}`, depth);
38-47:⚠️ Potential issue | 🟡 MinorAdd tests for keys containing dots to verify round-trip escaping.
The implementation includes
escapeKey()to handle dots in keys (line 8) andsplitPath()to unescape during reconstruction (line 12), but the test suite lacks explicit verification. Add a round-trip test to ensure keys with dots (e.g.,"Key 0.002mm") are properly escaped during flattening and correctly reconstructed during unflattening.
🤖 Fix all issues with AI agents
In `@packages/core/src/v3/utils/flattenAttributes.ts`:
- Around line 12-36: The splitPath function can drop a trailing unescaped
backslash because isEscaped may be true at loop end; fix it by detecting when
isEscaped is still true after the loop in splitPath and append a literal
backslash (e.g., add "\\" to current) before pushing the final segment, so paths
like "key\\" preserve the trailing backslash instead of silently losing it.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/fix-dotted-keys-logging.mdpackages/core/src/v3/utils/flattenAttributes.tspackages/core/test/flattenAttributes.test.ts
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
**/*.{ts,tsx}: Always import tasks from@trigger.dev/sdk, never use@trigger.dev/sdk/v3or deprecatedclient.defineJobpattern
Every Trigger.dev task must be exported and have a uniqueidproperty with no timeouts in the run function
Files:
packages/core/src/v3/utils/flattenAttributes.tspackages/core/test/flattenAttributes.test.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
packages/core/src/v3/utils/flattenAttributes.tspackages/core/test/flattenAttributes.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Import from
@trigger.dev/coreusing subpaths only, never import from root
Files:
packages/core/src/v3/utils/flattenAttributes.tspackages/core/test/flattenAttributes.test.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.ts: When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
packages/core/src/v3/utils/flattenAttributes.tspackages/core/test/flattenAttributes.test.ts
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
packages/core/src/v3/utils/flattenAttributes.tspackages/core/test/flattenAttributes.test.ts
{packages,integrations}/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Add a changeset when modifying any public package in
packages/*orintegrations/*usingpnpm run changeset:add
Files:
packages/core/src/v3/utils/flattenAttributes.tspackages/core/test/flattenAttributes.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use vitest for all tests in the Trigger.dev repository
Files:
packages/core/test/flattenAttributes.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.{ts,tsx,js,jsx}: Test files should live beside the files under test and use descriptivedescribeanditblocks
Tests should avoid mocks or stubs and use the helpers from@internal/testcontainerswhen Redis or Postgres are needed
Use vitest for running unit tests
**/*.test.{ts,tsx,js,jsx}: Use vitest exclusively for testing and never mock anything - use testcontainers instead
Place test files next to source files with naming pattern: source file (e.g.,MyService.ts) →MyService.test.ts
Files:
packages/core/test/flattenAttributes.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use testcontainers helpers (
redisTest,postgresTest,containerTest) from@internal/testcontainersfor Redis/PostgreSQL testing instead of mocks
Files:
packages/core/test/flattenAttributes.test.ts
🧠 Learnings (5)
📚 Learning: 2026-01-15T10:48:02.687Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-15T10:48:02.687Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use vitest for running unit tests
Applied to files:
packages/core/test/flattenAttributes.test.ts
📚 Learning: 2025-11-27T16:26:37.432Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T16:26:37.432Z
Learning: Applies to **/*.{test,spec}.{ts,tsx} : Use vitest for all tests in the Trigger.dev repository
Applied to files:
packages/core/test/flattenAttributes.test.ts
📚 Learning: 2026-01-15T11:50:06.067Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T11:50:06.067Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use vitest exclusively for testing and never mock anything - use testcontainers instead
Applied to files:
packages/core/test/flattenAttributes.test.ts
📚 Learning: 2026-01-15T10:48:02.687Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-15T10:48:02.687Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Test files should live beside the files under test and use descriptive `describe` and `it` blocks
Applied to files:
packages/core/test/flattenAttributes.test.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use logger methods (debug, log, info, warn, error) from `trigger.dev/sdk/v3` for structured logging in tasks
Applied to files:
.changeset/fix-dotted-keys-logging.md
🔇 Additional comments (8)
.changeset/fix-dotted-keys-logging.md (1)
1-5: LGTM!The changeset correctly documents the patch for
@trigger.dev/coreand clearly describes the fix for issue#1510.packages/core/src/v3/utils/flattenAttributes.ts (4)
8-10: LGTM!The escape order is correct—backslashes are escaped first to prevent double-escaping when dots are subsequently escaped.
228-246: LGTM!The core fix correctly applies
escapeKeyto object property keys while preserving bracket notation for array indices. This properly addresses the dotted-key issue.
312-329: LGTM!Using
splitPathinstead of naivesplit(".")correctly handles escaped separators, ensuring round-trip consistency with the escaping applied during flattening.
54-57: LGTM!Minor formatting adjustment in the constructor body.
packages/core/test/flattenAttributes.test.ts (3)
1-1: LGTM!Vitest imports added correctly per coding guidelines.
300-322: LGTM!Formatting adjustments for empty function bodies are consistent throughout the file.
416-433: LGTM!Consistent formatting for empty callbacks in Promise tests.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| function splitPath(path: string): string[] { | ||
| const parts: string[] = []; | ||
| let current = ""; | ||
| let isEscaped = false; | ||
|
|
||
| for (let i = 0; i < path.length; i++) { | ||
| const char = path[i]; | ||
|
|
||
| if (isEscaped) { | ||
| current += char; | ||
| isEscaped = false; | ||
| } else if (char === "\\") { | ||
| isEscaped = true; | ||
| } else if (char === ".") { | ||
| parts.push(current); | ||
| current = ""; | ||
| } else { | ||
| current += char; | ||
| } | ||
| } | ||
|
|
||
| parts.push(current); | ||
|
|
||
| return parts; | ||
| } |
There was a problem hiding this comment.
Minor edge case: trailing backslash is silently dropped.
If a path ends with an unescaped backslash (e.g., "key\\"), the isEscaped flag is set but no subsequent character exists to append. The trailing backslash is silently dropped.
This is unlikely to occur in practice since escapeKey only produces escaped sequences like \\\\ (for literal backslash) or \\. (for literal dot), never a dangling backslash. However, for defensive completeness, you could append the backslash if isEscaped is still true after the loop.
🛡️ Optional defensive fix
parts.push(current);
+ if (isEscaped) {
+ // Handle trailing backslash (shouldn't occur with proper escaping)
+ parts[parts.length - 1] += "\\";
+ }
+
return parts;
}🤖 Prompt for AI Agents
In `@packages/core/src/v3/utils/flattenAttributes.ts` around lines 12 - 36, The
splitPath function can drop a trailing unescaped backslash because isEscaped may
be true at loop end; fix it by detecting when isEscaped is still true after the
loop in splitPath and append a literal backslash (e.g., add "\\" to current)
before pushing the final segment, so paths like "key\\" preserve the trailing
backslash instead of silently losing it.
1 participant
Summary
Fixes #1510
Problem
Logging objects with keys containing dots (e.g. { 'Key 0.002mm': 31.4 }) resulted in incorrect nested object structure in the UI/logs (e.g. { 'Key 0': { '002mm': 31.4 } }) because dots were treated as separators during unflattening.
Fix
Implemented key escaping in \ lattenAttributes.ts:
This ensures dotted keys are preserved as single keys during the flatten/unflatten cycle.