Skip to content

Conversation

@schiller-manuel
Copy link
Contributor

@schiller-manuel schiller-manuel commented Oct 11, 2025

Summary by CodeRabbit

  • New Features

    • Improved handling when directives are split into separate parameter exports, including conditional default-export support.
  • Bug Fixes

    • More robust export transformation to preserve declarations and handle varied export forms without breaking generated output.
  • Refactor

    • Directive compilation now propagates split-param context through processing and uses safer AST-based export manipulation.
  • Tests

    • Added tests for server functions referencing exported values (including a public schema) and updated snapshots for aliased export naming.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 11, 2025

Walkthrough

Propagates an isDirectiveSplitParam flag through directive compilation, extends compileDirective to accept compileDirectiveOpts, updates export-swapping to handle named and conditional default exports, refactors safeRemoveExports to return/replace AST nodes, and adds tests for server functions referencing exported values.

Changes

Cohort / File(s) Summary
Compiler core: directive flow & exports
packages/directive-functions-plugin/src/compilers.ts
Propagate isDirectiveSplitParam from compileDirectives into findDirectives and compileDirective via a compileDirectiveOpts object; update compileDirective signature and call sites; adjust export-swapping logic to support named and conditional default exports when flag is set; refactor safeRemoveExports to return/replace AST nodes (no path-based removals); update helper types and wiring.
Tests: server function / exported value references
packages/directive-functions-plugin/tests/compiler.test.ts
Add test case verifying a server function references an exported value (zod schema); introduce aliased bindings and adjusted client/SSR/server snapshots to include new imports, alias declarations, and exported server-function bindings; update expectations across related tests.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Dev as Caller
  participant CD as compileDirectives
  participant FD as findDirectives
  participant CDir as compileDirective
  participant SE as safeRemoveExports
  participant AST as AST

  Dev->>CD: provide AST + options
  CD->>CD: compute isDirectiveSplitParam
  CD->>FD: findDirectives(AST, ..., isDirectiveSplitParam)
  FD->>CDir: compileDirective(node, compileDirectiveOpts)
  Note right of CDir #d3f8d3: export handling considers\nnamed vs conditional default when flag=true
  CDir->>SE: request export adjustments
  SE->>AST: return updated/replaced nodes (no path mutation)
  CDir-->>FD: compiled nodes
  FD-->>CD: aggregated transformations
  CD-->>Dev: transformed AST/code
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

I twitched my nose at a split-flag cue,
Hopped through exports to make them true.
I mend the AST with a careful bite,
No path deletions—just nodes set right.
Tests hum softly; the schema says that's new. 🐇

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title Check ❓ Inconclusive The title is concise and indicates a fix related to directive compilation replacements, but it is too generic and does not clearly reflect the primary change of propagating the new isDirectiveSplitParam flag or the associated export handling refinements. Because it omits the key context around split-parameter directives and safe export removal, it is unclear whether it accurately summarizes the core changeset. Consequently, it is inconclusive if this title provides enough specificity for reviewers scanning the history. To improve clarity, consider revising the title to explicitly reference the new isDirectiveSplitParam flag propagation and updated directive export replacement logic, for example “fix: propagate isDirectiveSplitParam and refine directive export handling.”
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-directive-compilation

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 80c9ad2 and c67f4de.

📒 Files selected for processing (1)
  • packages/directive-functions-plugin/tests/compiler.test.ts (6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use TypeScript in strict mode with extensive type safety across the codebase

Files:

  • packages/directive-functions-plugin/tests/compiler.test.ts
🧬 Code graph analysis (1)
packages/directive-functions-plugin/tests/compiler.test.ts (1)
packages/directive-functions-plugin/src/compilers.ts (1)
  • compileDirectives (58-118)
⏰ 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). (1)
  • GitHub Check: Test
🔇 Additional comments (3)
packages/directive-functions-plugin/tests/compiler.test.ts (3)

258-262: LGTM!

The alias declarations correctly preserve original identifiers in the server output while maintaining the RPC-generated names. This allows the server functions to reference exported values by their original names, which aligns with the PR's objective to handle named and conditional default exports properly.

Also applies to: 279-280


558-558: LGTM!

Consistent with the aliasing pattern established elsewhere in the test file. The server output correctly maintains original identifiers through alias declarations.

Also applies to: 568-568


910-974: Excellent test coverage for exported value references.

This test case effectively validates that when a server function references an exported value from the same file, the compilation correctly:

  • Preserves the export in client/SSR builds (lines 941, 951)
  • Removes the export but retains the declaration in the server split file (line 961), allowing the server function to reference it
  • Properly exports only the server function binding (line 972)

Note: The typo flagged in the previous review (safeParesesafeParse) has been corrected on line 923.


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

@nx-cloud
Copy link

nx-cloud bot commented Oct 11, 2025

View your CI Pipeline Execution ↗ for commit c67f4de

Command Status Duration Result
nx affected --targets=test:eslint,test:unit,tes... ✅ Succeeded 52s View ↗
nx run-many --target=build --exclude=examples/*... ✅ Succeeded 1s View ↗

☁️ Nx Cloud last updated this comment at 2025-10-12 05:33:49 UTC

@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 11, 2025

More templates

@tanstack/arktype-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/arktype-adapter@5450

@tanstack/directive-functions-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/directive-functions-plugin@5450

@tanstack/eslint-plugin-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/eslint-plugin-router@5450

@tanstack/history

npm i https://pkg.pr.new/TanStack/router/@tanstack/history@5450

@tanstack/nitro-v2-vite-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/nitro-v2-vite-plugin@5450

@tanstack/react-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-router@5450

@tanstack/react-router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-router-devtools@5450

@tanstack/react-router-ssr-query

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-router-ssr-query@5450

@tanstack/react-start

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start@5450

@tanstack/react-start-client

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-client@5450

@tanstack/react-start-server

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-server@5450

@tanstack/router-cli

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-cli@5450

@tanstack/router-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-core@5450

@tanstack/router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-devtools@5450

@tanstack/router-devtools-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-devtools-core@5450

@tanstack/router-generator

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-generator@5450

@tanstack/router-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-plugin@5450

@tanstack/router-ssr-query-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-ssr-query-core@5450

@tanstack/router-utils

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-utils@5450

@tanstack/router-vite-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-vite-plugin@5450

@tanstack/server-functions-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/server-functions-plugin@5450

@tanstack/solid-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-router@5450

@tanstack/solid-router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-router-devtools@5450

@tanstack/solid-start

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start@5450

@tanstack/solid-start-client

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-client@5450

@tanstack/solid-start-server

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-server@5450

@tanstack/start-client-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-client-core@5450

@tanstack/start-plugin-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-plugin-core@5450

@tanstack/start-server-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-server-core@5450

@tanstack/start-static-server-functions

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-static-server-functions@5450

@tanstack/start-storage-context

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-storage-context@5450

@tanstack/valibot-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/valibot-adapter@5450

@tanstack/virtual-file-routes

npm i https://pkg.pr.new/TanStack/router/@tanstack/virtual-file-routes@5450

@tanstack/zod-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/zod-adapter@5450

commit: c67f4de

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1a74e31 and 80c9ad2.

📒 Files selected for processing (1)
  • packages/directive-functions-plugin/tests/compiler.test.ts (6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use TypeScript in strict mode with extensive type safety across the codebase

Files:

  • packages/directive-functions-plugin/tests/compiler.test.ts
🧬 Code graph analysis (1)
packages/directive-functions-plugin/tests/compiler.test.ts (1)
packages/directive-functions-plugin/src/compilers.ts (1)
  • compileDirectives (58-118)
⏰ 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). (1)
  • GitHub Check: Test

@schiller-manuel schiller-manuel merged commit 3c6e34b into main Oct 12, 2025
6 checks passed
@schiller-manuel schiller-manuel deleted the fix-directive-compilation branch October 12, 2025 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants