-
Notifications
You must be signed in to change notification settings - Fork 188
feat: use readonly keyword in document and row model params #1174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe change updates TypeScript model generation in two templates: templates/react-native/src/models.ts.twig and templates/web/src/models.ts.twig. Property declarations in generated types now conditionally include the readonly modifier when property.readOnly is true. The optional marker (?) and type resolution via getSubSchema remain unchanged. This affects only the emitted type property lines in the generated model declarations. Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
templates/react-native/src/models.ts.twig (1)
14-14
: Potential breaking type change—confirm release notes and versioningAdding
readonly
can break existing consumers that mutate these props. Please:
- Mark this as a breaking change in release notes.
- Confirm the generator’s supported TypeScript baseline includes
readonly
on properties.- Consider adding a small generation test with a spec that includes
readOnly: true
to ensure compiled output type-checks.Happy to provide a minimal schema and expected output for the test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
templates/react-native/src/models.ts.twig
(1 hunks)templates/web/src/models.ts.twig
(1 hunks)
⏰ 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). (12)
- GitHub Check: build (8.3, Ruby30)
- GitHub Check: build (8.3, Ruby31)
- GitHub Check: build (8.3, AppleSwift56)
- GitHub Check: build (8.3, Node18)
- GitHub Check: build (8.3, PHP80)
- GitHub Check: build (8.3, Go112)
- GitHub Check: build (8.3, KotlinJava8)
- GitHub Check: build (8.3, CLINode18)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, DartStable)
- GitHub Check: build (8.3, FlutterBeta)
- GitHub Check: build (8.3, Android5Java17)
🔇 Additional comments (3)
templates/react-native/src/models.ts.twig (1)
14-14
: Readonly placement and syntax look correctUsing
readonly
before the property name (and before?
when optional) is valid TypeScript and matches intended semantics forreadOnly
schema fields.templates/web/src/models.ts.twig (2)
17-17
: Matches RN template and TypeScript semanticsThe conditional
readonly
is correctly positioned and keeps optionality and types intact. Parity with the React Native template is good.
17-17
: Surface area check and commsSame note as RN: this is a type-level breaking change for consumers who were mutating read-only fields. Please call this out in the changelog and verify the supported TypeScript version across web consumers.
What does this PR do?
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
(Write your answer here.)
Summary by CodeRabbit