fix: preserve transaction state in $use, $unuse, and $unuseAll

[pkudinov]

Fixes #2494

Problem

Calling $use(), $unuse(), or $unuseAll() on a transaction client returns a new client that runs queries outside the active transaction.

The constructor always creates a fresh Kysely instance from kyselyProps (this.kysely = new Kysely(this.kyselyProps)), but the transaction state lives in txClient.kysely which was set to the Kysely Transaction object after construction. The new client never receives this.

Fix

After constructing the new client in $use(), $unuse(), and $unuseAll(), propagate the transaction Kysely instance:

if (this.kysely.isTransaction) {
    newClient.kysely = this.kysely;
}

Tests

Three new tests in tests/e2e/orm/client-api/transaction.test.ts verify that $unuseAll(), $unuse(), and $use() on a transaction client stay in the same transaction by checking that writes roll back when the transaction fails.

Summary by CodeRabbit

• Bug Fixes

  • Fixed preservation of active transaction context when creating cloned client instances.

• Tests

  • Added end-to-end tests confirming transaction isolation and rollback behavior when using client helper methods inside transactions.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: b95e347b-4ac5-469f-b258-68b830204756

📥 Commits

Reviewing files that changed from the base of the PR and between ce30ea2 and abeac6f.

📒 Files selected for processing (2)

• packages/orm/src/client/client-impl.ts
• tests/e2e/orm/client-api/transaction.test.ts

✅ Files skipped from review due to trivial changes (1)

• tests/e2e/orm/client-api/transaction.test.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• packages/orm/src/client/client-impl.ts

---

📝 Walkthrough

Walkthrough

Client cloning now preserves active transaction context by reusing the base client's Kysely transaction instance when creating new ClientImpl clones. Five end-to-end tests were added to verify transactional helper APIs keep isolation and rollback behavior inside a transaction.

Changes

Cohort / File(s)	Summary
Client transaction propagation packages/orm/src/client/client-impl.ts	Constructor for ClientImpl now conditionally assigns the existing transaction Kysely instance from baseClient (when baseClient?.isTransaction) to the new client, ensuring cloned clients run inside the active transaction.
E2E transaction tests tests/e2e/orm/client-api/transaction.test.ts	Added five interactive-transaction tests that call transactional helper APIs ($unuseAll, $unuse('nonexistent'), $use(...), $setAuth(undefined), $setOptions(...)) inside client.$transaction, force rollback, assert rejection, and verify no persisted data.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 I hopped into the transaction den,
Found clones that slipped outside again.
I tied the thread of Kysely tight,
So every query stays in flight.
🥕✨ Rollbacks safe beneath moonlight.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately describes the primary fix—preserving transaction state when calling $use, $unuse, and $unuseAll methods on transaction clients.
Linked Issues check	✅ Passed	The PR implementation matches the linked issue #2494 requirements: it propagates the transaction Kysely instance to newly constructed clients in the conditional check, and includes E2E tests verifying transaction preservation.
Out of Scope Changes check	✅ Passed	All changes are directly aligned with the stated objective of fixing transaction isolation in $use, $unuse, and $unuseAll; no extraneous modifications are present.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

tests/e2e/orm/client-api/transaction.test.ts (1)

136-148: Consider using a valid plugin structure.

The plugin object uses handle, which isn't a recognized plugin hook based on the ClientImpl implementation (which looks for onQuery, onProcedure, client, result). While this works as a no-op plugin (unrecognized properties are ignored), using the correct interface would make the test more representative:

🔧 Suggested improvement

         it('$use preserves transaction isolation', async () => {
             await expect(
                 client.$transaction(async (tx) => {
-                    await (tx as any).$use({ id: 'noop', handle: (_node: any, proceed: any) => proceed(_node) }).user.create({
+                    await (tx as any).$use({ id: 'noop' }).user.create({
                         data: { email: 'u1@test.com' },
                     });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/orm/client-api/transaction.test.ts` around lines 136 - 148, The
test uses client.$use with a plugin object that defines a nonstandard "handle"
property; update the plugin to use a valid plugin hook (e.g., onQuery or
onProcedure) so it matches the ClientImpl plugin shape and remains a
no-op—replace the object passed to (tx as any).$use (id: 'noop', handle: ...)
with one that provides a recognized hook like onQuery/onProcedure that simply
calls proceed, keeping the id 'noop' and the overall behavior under
client.$transaction intact.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/e2e/orm/client-api/transaction.test.ts`:
- Around line 136-148: The test uses client.$use with a plugin object that
defines a nonstandard "handle" property; update the plugin to use a valid plugin
hook (e.g., onQuery or onProcedure) so it matches the ClientImpl plugin shape
and remains a no-op—replace the object passed to (tx as any).$use (id: 'noop',
handle: ...) with one that provides a recognized hook like onQuery/onProcedure
that simply calls proceed, keeping the id 'noop' and the overall behavior under
client.$transaction intact.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 4a081a18-ec8d-4f80-8906-472b677ced06

📥 Commits

Reviewing files that changed from the base of the PR and between 00768de and 03736ed.

📒 Files selected for processing (2)

• packages/orm/src/client/client-impl.ts
• tests/e2e/orm/client-api/transaction.test.ts

[ymc9]

Thanks @pkudinov , this is an important fix!

There're a few other places that need the same care: $setAuth, $setOptions. I think we can consolidate the change into the constructor. When baseClient is passed and baseClient.kysely is a transaction, we directly reuse it in the newly constructed client. What do you think?

[pkudinov]

Implemented in abeac6fb.

I moved the transaction reuse into the constructor for the non-executor clone path, so $setAuth and $setOptions are covered automatically alongside $use/$unuse/$unuseAll. The working version was to reuse the transaction through the public accessors (baseClient.isTransaction / baseClient.$qb) rather than reading private state directly.

I also extended the e2e coverage to include $setAuth and $setOptions, and updated the $use test to use a real onQuery hook.

Please let me know what you think @ymc9