Skip to content

Conversation

@ymc9
Copy link
Member

@ymc9 ymc9 commented Nov 18, 2025

fixes #393

Summary by CodeRabbit

  • Bug Fixes

    • Corrected handling of negative numeric default values so they are represented and applied correctly in generated schemas.
  • Tests

    • Added a regression test confirming negative numeric defaults are preserved and applied when creating records.

Copilot AI review requested due to automatic review settings November 18, 2025 02:22
@coderabbitai
Copy link

coderabbitai bot commented Nov 18, 2025

Walkthrough

Introduces a helper to generate negative numeric literals as unary prefix expressions while leaving non-negative numbers as plain numeric literals; adds a regression test asserting a negative default value (-6) is applied when creating a model instance.

Changes

Cohort / File(s) Summary
Numeric literal generation
packages/sdk/src/ts-schema-generator.ts
Added private helper createNumberLiteral(arg: number) that returns a unary minus prefix expression for negative numbers and a numeric literal for non-negative numbers; replaced direct ts.factory.createNumericLiteral(...) usage with this helper where literals are created.
Regression test
tests/regression/test/issue-393.test.ts
Added test verifying a model field with @default(-6) yields -6 when creating a record with empty data.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Localized change in one main source file and one test.
  • Review focus: correctness of unary prefix expression creation and places where numeric literals are produced.
  • Pay extra attention to any other literal generation paths that may still call createNumericLiteral directly.

Possibly related PRs

Poem

🐰 I found a minus lost in code,
I nudged it gently into mode.
Now -6 hops where defaults dwell,
No more a TypeScript error knell.
Hooray — the schema sings, all's well! 🎉

Pre-merge checks and finishing touches

✅ 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 main fix: correcting TypeScript schema generation when a model field has a negative number as its @default value.
Linked Issues check ✅ Passed The code changes directly address the linked issue #393 by adding a helper function that properly handles negative numbers using prefix unary expressions instead of direct numeric literals.
Out of Scope Changes check ✅ Passed All changes are scoped to fixing the negative number default value issue: the helper function in ts-schema-generator.ts and a regression test in issue-393.test.ts.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ 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/issue-393

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9b7b85e and 80a328a.

📒 Files selected for processing (1)
  • packages/sdk/src/ts-schema-generator.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/sdk/src/ts-schema-generator.ts
⏰ 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). (3)
  • GitHub Check: claude-review
  • GitHub Check: build-test (20.x, postgresql)
  • GitHub Check: build-test (20.x, sqlite)

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

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

@claude
Copy link

claude bot commented Nov 18, 2025

Claude finished @ymc9's task —— View job


PR Review Analysis

Reviewing the pull request for issue #393 - negative number default values in TS schema generation.

Review Tasks:

  • Read and analyze the code changes
  • Check fix implementation quality
  • Evaluate test coverage
  • Assess for potential bugs/issues
  • Review for security concerns
  • Check performance considerations
  • Provide constructive feedback

Summary

This is a well-implemented fix for a TypeScript AST generation bug where negative number literals in default values weren't properly handled.

Code Quality ✅

Problem Identified:

  • Line 1077 in packages/sdk/src/ts-schema-generator.ts:1077 was using ts.factory.createNumericLiteral(arg) for all numbers
  • TypeScript's createNumericLiteral() expects a string and cannot handle negative numbers directly
  • For negative numbers, you need to create a prefix unary expression with a minus token

Solution Applied:

  • Extracted logic into createNumberLiteral() method
  • For negative numbers: creates ts.factory.createPrefixUnaryExpression(ts.SyntaxKind.MinusToken, ts.factory.createNumericLiteral(-arg))
  • For positive numbers: uses the original ts.factory.createNumericLiteral(arg)

Implementation Quality ✅

The fix is:

  • Minimal and focused - Only touches the specific problematic code
  • Type-safe - Properly handles TypeScript AST node creation
  • Follows existing patterns - Method naming and structure consistent with codebase
  • Backward compatible - No breaking changes to existing functionality

Test Coverage ✅

Good regression test coverage:

  • Tests the exact scenario that was failing (tz Int @default(-6))
  • Verifies the end-to-end behavior (schema generation → database creation → value insertion)
  • Uses realistic field types and constraints (@db.SmallInt())

Suggestion for improvement:
Could benefit from additional edge cases:

  • Floating point negative numbers (@default(-1.5))
  • Large negative numbers
  • Zero values (boundary case)

Performance ✅

No performance concerns - the fix is a simple conditional check with minimal overhead.

Security ✅

No security implications. This is purely a code generation fix for TypeScript AST creation.

Architecture ✅

The fix properly maintains separation of concerns by keeping number literal creation logic encapsulated in its own method.

Minor Suggestions

  1. Method documentation: Consider adding a JSDoc comment explaining the TypeScript AST limitation:

    /**
     * Creates a TypeScript numeric literal, handling negative numbers properly
     * by wrapping them in a prefix unary expression.
     */
    private createNumberLiteral(arg: number): any {
  2. Type specificity: Consider using more specific return type than any:

    private createNumberLiteral(arg: number): ts.NumericLiteral | ts.PrefixUnaryExpression {

Verdict: APPROVED

This is a clean, targeted fix that addresses the specific issue without introducing risk. The test coverage ensures the fix works correctly and prevents regression.

Copilot finished reviewing on behalf of ymc9 November 18, 2025 02:24
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes issue #393 where the TypeScript schema generator incorrectly handled negative numbers in default values. The fix properly generates negative numeric literals using TypeScript AST's prefix unary expression instead of directly passing negative numbers to createNumericLiteral.

  • Created createNumberLiteral helper method to handle negative numbers by creating a prefix unary expression with a minus token
  • Added regression test to verify that models with negative default values (e.g., @default(-6)) can be successfully generated and used

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
tests/regression/test/issue-393.test.ts Added regression test that verifies schema generation and database operations work correctly with negative default values
packages/sdk/src/ts-schema-generator.ts Refactored numeric literal creation to handle negative numbers using TypeScript AST's createPrefixUnaryExpression with MinusToken

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@claude
Copy link

claude bot commented Nov 18, 2025

Claude finished @ymc9's task —— View job


PR Review Analysis

Reviewing the pull request for issue #393 - negative number default values in TS schema generation.

Review Tasks:

  • Read and analyze the code changes
  • Check fix implementation quality
  • Evaluate test coverage
  • Assess for potential bugs/issues
  • Review for security concerns
  • Check performance considerations
  • Provide constructive feedback

Summary

This is a well-implemented fix for a TypeScript AST generation bug where negative number literals in default values weren't properly handled.

Code Quality ✅

Problem Identified:

  • Line 1077 in packages/sdk/src/ts-schema-generator.ts:1077 was using ts.factory.createNumericLiteral(arg) for all numbers
  • TypeScript's createNumericLiteral() expects a string and cannot handle negative numbers directly
  • For negative numbers, you need to create a prefix unary expression with a minus token

Solution Applied:

  • Extracted logic into createNumberLiteral() method
  • For negative numbers: creates ts.factory.createPrefixUnaryExpression(ts.SyntaxKind.MinusToken, ts.factory.createNumericLiteral(-arg))
  • For positive numbers: uses the original ts.factory.createNumericLiteral(arg)

Implementation Quality ✅

The fix is:

  • Minimal and focused - Only touches the specific problematic code
  • Type-safe - Properly handles TypeScript AST node creation
  • Follows existing patterns - Method naming and structure consistent with codebase
  • Backward compatible - No breaking changes to existing functionality

Test Coverage ✅

Good regression test coverage:

  • Tests the exact scenario that was failing (tz Int @default(-6))
  • Verifies the end-to-end behavior (schema generation → database creation → value insertion)
  • Uses realistic field types and constraints (@db.SmallInt())

Suggestion for improvement:
Could benefit from additional edge cases:

  • Floating point negative numbers (@default(-1.5))
  • Large negative numbers
  • Zero values (boundary case)

Performance ✅

No performance concerns - the fix is a simple conditional check with minimal overhead.

Security ✅

No security implications. This is purely a code generation fix for TypeScript AST creation.

Architecture ✅

The fix properly maintains separation of concerns by keeping number literal creation logic encapsulated in its own method.

Minor Suggestions

  1. Method documentation: Consider adding a JSDoc comment explaining the TypeScript AST limitation:

    /**
     * Creates a TypeScript numeric literal, handling negative numbers properly
     * by wrapping them in a prefix unary expression.
     */
    private createNumberLiteral(arg: number): any {
  2. Type specificity: Consider using more specific return type than any:

    private createNumberLiteral(arg: number): ts.NumericLiteral | ts.PrefixUnaryExpression {

Verdict: APPROVED

This is a clean, targeted fix that addresses the specific issue without introducing risk. The test coverage ensures the fix works correctly and prevents regression.


@ymc9 ymc9 merged commit a034b1c into dev Nov 18, 2025
5 checks passed
@ymc9 ymc9 deleted the fix/issue-393 branch November 18, 2025 02:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants