fix(engine): preserve runtime extensions via non-enumerable symbol during config cloning#25
Merged
jacksonkasi1 merged 6 commits intomainfrom Mar 11, 2026
Merged
fix(engine): preserve runtime extensions via non-enumerable symbol during config cloning#25jacksonkasi1 merged 6 commits intomainfrom
jacksonkasi1 merged 6 commits intomainfrom
Conversation
…ring config cloning
Reviewer's GuideEnsures engine runtime extensions are preserved when table configs are cloned by embedding extensions on the config object under a non-enumerable symbol key and teaching the engine to read them back, while hardening resolveInput against falsy configs and bumping the engine package version. Sequence diagram for resolveInput using embedded runtime extensionssequenceDiagram
actor Developer
participant Builder as TableDefinitionBuilder_T_
participant Engine as EngineFunctions
Developer->>Builder: toConfig()
Builder->>Builder: clone _config to config
Builder->>Builder: attach TABLECRAFT_EXTENSIONS_KEY with _ext
Builder-->>Developer: config_with_extensions
Developer->>Developer: cloned_config = { ...config_with_extensions }
Developer->>Engine: resolveInput(cloned_config)
Engine->>Engine: plainConfig = input
Engine->>Engine: embeddedExt = plainConfig[TABLECRAFT_EXTENSIONS_KEY]
alt extensions_embedded
Engine-->>Developer: { config: plainConfig, ext: embeddedExt }
else no_extensions
Engine->>Engine: create new RuntimeExtensions_any_
Engine-->>Developer: { config: plainConfig, ext: new_extensions }
end
Class diagram for embedded runtime extensions on TableConfigclassDiagram
class TableConfig {
}
class RuntimeExtensions_any_ {
+computedExpressions Map
+transforms Map
+rawSelects Map
+rawWheres Array
+dynamicWheres Array
+rawJoins Array
+rawOrderBys Array
+ctes Map
+sqlJoinConditions Map
+countMode any
}
class RuntimeExtensions_T_ {
+computedExpressions Map
+transforms Map
+rawSelects Map
+rawWheres Array
+dynamicWheres Array
+rawJoins Array
+rawOrderBys Array
+ctes Map
+sqlJoinConditions Map
+countMode any
}
class TableDefinitionBuilder_T_ {
-_config TableConfig
-_ext RuntimeExtensions_T_
+toConfig() TableConfig
}
class EngineFunctions {
+resolveInput(input ConfigInput) ResolveResult
}
class ResolveResult {
+config TableConfig
+ext RuntimeExtensions_any_
}
class TABLECRAFT_EXTENSIONS_KEY {
+symbol Symbol
}
TableDefinitionBuilder_T_ --> TableConfig : builds
TableDefinitionBuilder_T_ --> RuntimeExtensions_T_ : holds_ext
TableDefinitionBuilder_T_ ..> TABLECRAFT_EXTENSIONS_KEY : uses_key_in_toConfig
TableConfig --> RuntimeExtensions_any_ : property_TABLECRAFT_EXTENSIONS_KEY
TABLECRAFT_EXTENSIONS_KEY ..> RuntimeExtensions_any_ : indexes_extensions
EngineFunctions --> ResolveResult : creates
EngineFunctions --> TableConfig : reads_config
EngineFunctions --> RuntimeExtensions_any_ : reads_or_creates_ext
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds persistent runtime extensions support: introduces Changes
Sequence DiagramsequenceDiagram
participant User
participant Builder as TableDefinitionBuilder
participant Config as TableConfig
participant Engine as resolveInput()
User->>Builder: define table (with runtime extensions)
Builder->>Builder: collect and clone runtime extensions
Builder->>Config: toConfig() (attach extensions at TABLECRAFT_EXTENSIONS_KEY)
User->>Engine: pass builder or config
Engine->>Config: inspect TABLECRAFT_EXTENSIONS_KEY
alt embedded extensions present
Engine->>Engine: extract embeddedExt and plainConfig
Engine->>Engine: use embedded extensions for runtime state
else no embedded extensions
Engine->>Engine: use emptyExtensions()
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
toConfig, the symbol-backed extensions property is now created as a normal enumerable property; if you intend it to remain hidden from generic property iteration (beyond just JSON andObject.keys), consider usingObject.definePropertywithenumerable: falseto match the previous behavior more closely. - The new
resolveInputbranch for non-builder inputs can now return{ config: undefined, ext: … }wheninputis falsy; if callers are not expecting a potentially undefinedconfig, it might be safer to either throw early or keep the previous assumption thatinputmust be a valid config. - Using
Symbol.for("__tablecraft_ext")makes the extension key global across realms; if isolation between different consumers or versions of the engine is desired, consider using a localSymbol("__tablecraft_ext")instead and wiring it through internal imports.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `toConfig`, the symbol-backed extensions property is now created as a normal enumerable property; if you intend it to remain hidden from generic property iteration (beyond just JSON and `Object.keys`), consider using `Object.defineProperty` with `enumerable: false` to match the previous behavior more closely.
- The new `resolveInput` branch for non-builder inputs can now return `{ config: undefined, ext: … }` when `input` is falsy; if callers are not expecting a potentially undefined `config`, it might be safer to either throw early or keep the previous assumption that `input` must be a valid config.
- Using `Symbol.for("__tablecraft_ext")` makes the extension key global across realms; if isolation between different consumers or versions of the engine is desired, consider using a local `Symbol("__tablecraft_ext")` instead and wiring it through internal imports.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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: 1
🧹 Nitpick comments (1)
packages/engine/src/define.ts (1)
1056-1063: Expose the augmented return type fromtoConfig().Line 1056 still advertises plain
TableConfig, but Lines 1057-1061 now attachTABLECRAFT_EXTENSIONS_KEY. That makes the new behavior invisible to consumers and is whypackages/engine/src/engine.tshas to re-cast the input. Returning the symbol-bearing type here keeps the API discoverable and removes the unsafe cast downstream.Type-safe shape for the embedded extensions
+export type TableConfigWithExtensions<T extends Table = Table> = + TableConfig & { [TABLECRAFT_EXTENSIONS_KEY]?: RuntimeExtensions<T> }; + - toConfig(): TableConfig { - const config = { ...this._config } as TableConfig & { - [TABLECRAFT_EXTENSIONS_KEY]?: RuntimeExtensions<T>; - }; + toConfig(): TableConfigWithExtensions<T> { + const config = { ...this._config } as TableConfigWithExtensions<T>; config[TABLECRAFT_EXTENSIONS_KEY] = this._ext; return config; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/engine/src/define.ts` around lines 1056 - 1063, The toConfig() method currently declares a return type of TableConfig but actually returns an object that includes the TABLECRAFT_EXTENSIONS_KEY with RuntimeExtensions<T>, causing callers to re-cast; change the declared return type of toConfig() to TableConfig & { [TABLECRAFT_EXTENSIONS_KEY]?: RuntimeExtensions<T> } so the symbol-bearing shape is exposed to consumers (update the signature of toConfig() accordingly and remove downstream unsafe casts such as the re-cast in engine.ts).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/engine/src/engine.ts`:
- Around line 57-78: The resolveInput function currently allows a falsy input
and returns config: plainConfig which can be undefined; change resolveInput to
validate input at the start (e.g., check for null/undefined) and throw a clear,
descriptive error when input is falsy so callers don't later get TypeError when
accessing config.base or config.name; keep using the same
TABLECRAFT_EXTENSIONS_KEY and embeddedExt logic, but ensure plainConfig is
present before building the returned object (throw a ConfigError or similar from
resolveInput if plainConfig is null/undefined).
---
Nitpick comments:
In `@packages/engine/src/define.ts`:
- Around line 1056-1063: The toConfig() method currently declares a return type
of TableConfig but actually returns an object that includes the
TABLECRAFT_EXTENSIONS_KEY with RuntimeExtensions<T>, causing callers to re-cast;
change the declared return type of toConfig() to TableConfig & {
[TABLECRAFT_EXTENSIONS_KEY]?: RuntimeExtensions<T> } so the symbol-bearing shape
is exposed to consumers (update the signature of toConfig() accordingly and
remove downstream unsafe casts such as the re-cast in engine.ts).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: db8bc184-07a4-4d1e-9ec6-5078ccd16a91
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
packages/engine/package.jsonpackages/engine/src/define.tspackages/engine/src/engine.ts
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/engine/src/engine.ts (1)
69-82: Consider reusingemptyExtensions()to reduce duplication.This inline object duplicates the structure from
emptyExtensions()indefine.ts, with the addition ofcountMode: undefined. IfemptyExtensions()were exported and updated to includecountMode, this would reduce maintenance burden and ensure consistency.♻️ Suggested approach
In
define.ts, exportemptyExtensionsand addcountMode:// In define.ts export function emptyExtensions<T extends Table = Table>(): RuntimeExtensions<T> { return { computedExpressions: new Map(), transforms: new Map(), rawSelects: new Map(), rawWheres: [], dynamicWheres: [], rawJoins: [], rawOrderBys: [], ctes: new Map(), sqlJoinConditions: new Map(), countMode: undefined, }; }Then in
engine.ts:+import { emptyExtensions } from './define'; // ... return { config: plainConfig, - ext: - embeddedExt ?? - { - computedExpressions: new Map(), - transforms: new Map(), - rawSelects: new Map(), - rawWheres: [], - dynamicWheres: [], - rawJoins: [], - rawOrderBys: [], - ctes: new Map(), - sqlJoinConditions: new Map(), - countMode: undefined, - }, + ext: embeddedExt ?? emptyExtensions(), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/engine/src/engine.ts` around lines 69 - 82, The inline default extensions object assigned to ext/embeddedExt duplicates the structure from define.ts; export and update the existing emptyExtensions<T>() (RuntimeExtensions) in define.ts to include the countMode property (countMode: undefined) and then replace the inline literal in engine.ts with a call to emptyExtensions() (e.g., ext: embeddedExt ?? emptyExtensions()). This ensures the RuntimeExtensions shape (including countMode) is centralized in emptyExtensions and removes the duplicated object in engine.ts.packages/engine/src/define.ts (2)
1062-1062: Consider documenting or cloning the shared_extreference.The
this._extobject is attached by reference, meaning any subsequent mutations to the builder's extensions (e.g., adding computed expressions) will affect configs already returned bytoConfig(). If this is intentional for memory efficiency, a doc comment would help. If isolation is preferred, a shallow clone of the extension maps would be safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/engine/src/define.ts` at line 1062, The returned config currently embeds the builder's extensions by reference via value: this._ext, which means later mutations to this._ext will mutate previously returned configs from toConfig(); either add a doc comment on the value field explaining the shared-by-reference behavior and intentionality, or change the implementation to return a shallow clone of the extension maps (i.e., clone this._ext and its top-level maps/objects before assigning to value) so each toConfig() result is isolated; locate the assignment using this._ext and the toConfig()/value return and apply the chosen fix.
70-70: Avoid unnecessaryas anycast on the symbol declaration.The
as anycast undermines type safety and is likely unnecessary. Aunique symbolshould be usable directly as a computed property key in TypeScript. If there's a specific type error prompting this cast, it would be better to address the root cause.🔧 Suggested fix
-export const TABLECRAFT_EXTENSIONS_KEY: unique symbol = Symbol("__tablecraft_ext") as any; +export const TABLECRAFT_EXTENSIONS_KEY: unique symbol = Symbol("__tablecraft_ext");If removing the cast causes a TypeScript error, please share the error message so we can address the underlying issue properly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/engine/src/define.ts` at line 70, Remove the unnecessary "as any" cast on the TABLECRAFT_EXTENSIONS_KEY declaration by changing export const TABLECRAFT_EXTENSIONS_KEY: unique symbol = Symbol("__tablecraft_ext") as any; to a plain unique symbol declaration (export const TABLECRAFT_EXTENSIONS_KEY: unique symbol = Symbol("__tablecraft_ext");) and then update any places that used it as a computed key to accept a unique symbol (e.g., adjust index signatures or interfaces that reference [TABLECRAFT_EXTENSIONS_KEY] to use the correct symbol-typed property) so TypeScript no longer requires the cast; if removing the cast produces a compiler error, capture and share the exact TypeScript error so we can adjust the related type (interface/index signature) instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/engine/src/define.ts`:
- Around line 1061-1066: The property defined on config using
Object.defineProperty with key TABLECRAFT_EXTENSIONS_KEY is set enumerable:
false which causes the symbol to be omitted by spread/Object.assign; change the
descriptor to enumerable: true (keep configurable: true and writable: true) when
defining TABLECRAFT_EXTENSIONS_KEY on the config object so the symbol survives
spread/assign while remaining invisible to JSON.stringify/Object.keys; update
the Object.defineProperty call where TABLECRAFT_EXTENSIONS_KEY is used in
define.ts accordingly.
---
Nitpick comments:
In `@packages/engine/src/define.ts`:
- Line 1062: The returned config currently embeds the builder's extensions by
reference via value: this._ext, which means later mutations to this._ext will
mutate previously returned configs from toConfig(); either add a doc comment on
the value field explaining the shared-by-reference behavior and intentionality,
or change the implementation to return a shallow clone of the extension maps
(i.e., clone this._ext and its top-level maps/objects before assigning to value)
so each toConfig() result is isolated; locate the assignment using this._ext and
the toConfig()/value return and apply the chosen fix.
- Line 70: Remove the unnecessary "as any" cast on the TABLECRAFT_EXTENSIONS_KEY
declaration by changing export const TABLECRAFT_EXTENSIONS_KEY: unique symbol =
Symbol("__tablecraft_ext") as any; to a plain unique symbol declaration (export
const TABLECRAFT_EXTENSIONS_KEY: unique symbol = Symbol("__tablecraft_ext");)
and then update any places that used it as a computed key to accept a unique
symbol (e.g., adjust index signatures or interfaces that reference
[TABLECRAFT_EXTENSIONS_KEY] to use the correct symbol-typed property) so
TypeScript no longer requires the cast; if removing the cast produces a compiler
error, capture and share the exact TypeScript error so we can adjust the related
type (interface/index signature) instead.
In `@packages/engine/src/engine.ts`:
- Around line 69-82: The inline default extensions object assigned to
ext/embeddedExt duplicates the structure from define.ts; export and update the
existing emptyExtensions<T>() (RuntimeExtensions) in define.ts to include the
countMode property (countMode: undefined) and then replace the inline literal in
engine.ts with a call to emptyExtensions() (e.g., ext: embeddedExt ??
emptyExtensions()). This ensures the RuntimeExtensions shape (including
countMode) is centralized in emptyExtensions and removes the duplicated object
in engine.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 70b87b85-e233-44c5-bd09-5778bd4a3303
📒 Files selected for processing (2)
packages/engine/src/define.tspackages/engine/src/engine.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/engine/src/define.ts`:
- Line 70: Replace the module-local symbol with a globally registered symbol so
extensions are shared across duplicated package instances: change the
TABLECRAFT_EXTENSIONS_KEY declaration (symbol name TABLECRAFT_EXTENSIONS_KEY) to
use Symbol.for("__tablecraft_ext") instead of Symbol("__tablecraft_ext"); update
any code that imports/compares this symbol (e.g., where engine.ts probes the
extensions key) to rely on the global symbol so different module copies resolve
to the same key.
In `@packages/engine/src/engine.ts`:
- Around line 57-60: The code returns extensions from the live builder field
b._ext, which bypasses the immutable snapshot created by
TableDefinitionBuilder.toConfig(); fix by calling toConfig() first, grab the
config snapshot (e.g. const cfg = b.toConfig()), and return the extensions from
that snapshot (use the snapshot's extension property from cfg) instead of b._ext
so the engine uses the cloned, immutable extensions produced by toConfig();
update the return to use cfg (and its extension field) with
TableDefinitionBuilder.toConfig() and stop referencing
TableDefinitionBuilder._ext directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 03a635f0-d108-439b-a882-e80ef26455b5
📒 Files selected for processing (2)
packages/engine/src/define.tspackages/engine/src/engine.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 the current code and only fix it if needed.
Inline comments:
In `@packages/engine/src/define.ts`:
- Around line 1059-1085: The config emitted by toConfig() stores runtime
extensions under TABLECRAFT_EXTENSIONS_KEY but defineTable(table, cfg) currently
calls emptyExtensions() when rebuilding a TableDefinitionBuilder; change
defineTable to check cfg[TABLECRAFT_EXTENSIONS_KEY] and, if present, use that
extensions object (or a shallow clone of it) instead of emptyExtensions() when
constructing the TableDefinitionBuilder so computedExpressions, transforms,
hooks, etc. are preserved; update the TableDefinitionBuilder constructor call
(and any related typing) to accept the rehydrated RuntimeExtensions<T> and
ensure Maps/arrays from the stored value are used (or cloned) to avoid future
mutation leaks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a226086e-273c-4264-9d3e-5e2f782ab571
📒 Files selected for processing (2)
packages/engine/src/define.tspackages/engine/src/engine.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/engine/src/engine.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/engine/src/define.ts (1)
70-74:⚠️ Potential issue | 🔴 CriticalRemove the stale second extensions-key/type declaration block.
TABLECRAFT_EXTENSIONS_KEYandTableConfigWithExtensionsare declared twice here, which is a TypeScript compile blocker. The later block also switches back toSymbol()instead ofSymbol.for(), so even after deduping it would regress the shared-key behavior. Keep theSymbol.for("__tablecraft_ext")declaration as the single source of truth and delete the later duplicate block.Suggested fix
-export const TABLECRAFT_EXTENSIONS_KEY = Symbol( - "tablecraft.runtimeExtensions", -); - -export type TableConfigWithExtensions<T extends Table = Table> = TableConfig & { - [TABLECRAFT_EXTENSIONS_KEY]?: RuntimeExtensions<T>; -};#!/bin/bash set -euo pipefail sed -n '68,130p' packages/engine/src/define.tsExpected result: exactly one
TABLECRAFT_EXTENSIONS_KEYdeclaration and oneTableConfigWithExtensionsdeclaration remain, and the surviving key usesSymbol.for("__tablecraft_ext").Also applies to: 122-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/engine/src/define.ts` around lines 70 - 74, Delete the duplicate declarations of TABLECRAFT_EXTENSIONS_KEY and TableConfigWithExtensions and keep only the original declaration that uses Symbol.for("__tablecraft_ext"); specifically, remove the later block that re-declares TABLECRAFT_EXTENSIONS_KEY using Symbol() and the duplicate TableConfigWithExtensions type so there is exactly one TABLECRAFT_EXTENSIONS_KEY (Symbol.for("__tablecraft_ext")) and one TableConfigWithExtensions in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/engine/src/define.ts`:
- Around line 70-74: Delete the duplicate declarations of
TABLECRAFT_EXTENSIONS_KEY and TableConfigWithExtensions and keep only the
original declaration that uses Symbol.for("__tablecraft_ext"); specifically,
remove the later block that re-declares TABLECRAFT_EXTENSIONS_KEY using Symbol()
and the duplicate TableConfigWithExtensions type so there is exactly one
TABLECRAFT_EXTENSIONS_KEY (Symbol.for("__tablecraft_ext")) and one
TableConfigWithExtensions in the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e605019-ce3b-480b-be73-5bfc6ce2f709
📒 Files selected for processing (2)
packages/engine/src/define.tspackages/engine/test/columnMeta.test.ts
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.
Summary
enumerable: falsestring to aunique symbol(Symbol.for("__tablecraft_ext")).JSON.stringifyandObject.keys.resolveInputinengine.tsto prevent a potentialTypeErrorif a falsy config is provided.Summary by Sourcery
Preserve engine runtime extensions when cloning table configs and make them discoverable via a hidden symbol key while safely resolving configs that may already carry embedded extensions.
Bug Fixes:
Enhancements:
Build:
Summary by CodeRabbit
Chores
New Features
Bug Fixes
Tests