make initialSettings and initialSubscriptionsPayload nullable #2972
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAllows both Class diagram for updated nullable config option schemasclassDiagram
class AltairWindowOptionsSchema {
<<schema>>
record initialSubscriptionsPayload optional nullable
}
class AltairConfigOptionsSchema {
<<schema>>
SettingsSchema initialSettings optional nullable
}
AltairConfigOptionsSchema --|> AltairWindowOptionsSchema
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello @imolorhe, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the flexibility of the application's configuration by modifying the schema for Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughSchema validators in the Altair core configuration module were updated to accept null for two optional fields: Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (3)**/*.ts📄 CodeRabbit inference engine (.github/instructions/app-testing.instructions.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/instructions/main.instructions.md)
Files:
packages/altair-core/**/*.ts📄 CodeRabbit inference engine (.github/instructions/main.instructions.md)
Files:
🧠 Learnings (11)📓 Common learnings📚 Learning: 2025-10-03T16:39:27.517ZApplied to files:
📚 Learning: 2025-10-03T16:40:52.954ZApplied to files:
📚 Learning: 2025-10-03T16:39:27.517ZApplied to files:
📚 Learning: 2025-10-03T16:40:52.954ZApplied to files:
📚 Learning: 2025-10-03T16:40:52.954ZApplied to files:
📚 Learning: 2025-10-03T16:41:45.498ZApplied to files:
📚 Learning: 2025-10-03T16:40:52.954ZApplied to files:
📚 Learning: 2025-10-03T16:40:52.954ZApplied to files:
📚 Learning: 2025-09-24T19:40:17.905ZApplied to files:
📚 Learning: 2025-05-29T13:44:11.906ZApplied to files:
⏰ 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). (15)
🔇 Additional comments (2)
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:
- The chaining order of
.optional()and.nullable()is inconsistent betweeninitialSubscriptionsPayload(.optional().nullable()) andinitialSettings(.partial().nullable().optional()); consider standardizing the order to ensure the resulting types are consistent and match your intendednull | undefinedbehavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The chaining order of `.optional()` and `.nullable()` is inconsistent between `initialSubscriptionsPayload` (`.optional().nullable()`) and `initialSettings` (`.partial().nullable().optional()`); consider standardizing the order to ensure the resulting types are consistent and match your intended `null | undefined` behavior.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly makes initialSettings and initialSubscriptionsPayload nullable in the configuration schema, which aligns with the goal of providing more flexible configuration options. The changes are straightforward and the use cases in the reducers handle the new nullable values correctly. I have one minor suggestion to improve code consistency in the schema definition.
| initialSettings: settingsSchema | ||
| .partial() | ||
| .nullable() | ||
| .meta({ description: 'Initial app settings to use' }) | ||
| .optional(), |
There was a problem hiding this comment.
For consistency with how other schemas are defined in this file, and with the other change in this PR for initialSubscriptionsPayload, it would be better to chain .nullable() after .optional(). This improves readability and maintainability by keeping a consistent order of schema modifiers.
| initialSettings: settingsSchema | |
| .partial() | |
| .nullable() | |
| .meta({ description: 'Initial app settings to use' }) | |
| .optional(), | |
| initialSettings: settingsSchema | |
| .partial() | |
| .meta({ description: 'Initial app settings to use' }) | |
| .optional() | |
| .nullable(), |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/altair-core/src/config/options.schema.ts (1)
1-246: Complete PR checklist before merging.The checklist includes three items that need completion:
- Run
yarn test-buildto verify the build succeeds with schema changes- Update relevant documentation to reflect the schema changes
- Ensure altair-core is properly built so altair-static receives updated schema validation
Note: altair-static imports
altairConfigOptionsSchemafrom the built altair-core package and uses it for runtime validation (line 125 in index.ts). No separate type definitions need manual updating in altair-static—they inherit automatically via the imported types and schema from altair-core's build output.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/altair-core/src/config/options.schema.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.github/instructions/app-testing.instructions.md)
Follow project code style using ESLint and Prettier
**/*.ts: Use explicit type annotations for function parameters and return types
Prefer interfaces over type aliases for object shapes
Use union types and literal types for better type safety
Leverage generic types for reusable components
Group imports: external libraries first, then internal modules
Use absolute imports from package roots when possible
Prefer named exports over default exports
Use custom error classes that extend Error
Implement proper error boundaries and handling
Log errors with sufficient context for debugging
Use observables (RxJS) for reactive programming patterns where appropriate
Manage subscriptions to avoid memory leaks
Use appropriate RxJS operators for data transformation
Handle errors in observable streams
Use async/await for sequential operations
Handle promise rejections properly
Use Promise.all() for concurrent operations
Implement timeout handling for long-running operations
Dispose of resources properly (subscriptions, event listeners)
Use weak references where appropriate
Avoid creating unnecessary objects in hot paths
Profile memory usage for performance-critical code
Use tree-shaking-friendly imports
Lazy load heavy modules when possible
Monitor bundle size impacts of new dependencies
Use dynamic imports for code splitting
Validate and sanitize all user inputs
Implement proper XSS and injection prevention
Validate API responses before processing
Sanitize sensitive data in logs
Follow secure coding practices
Group related functionality in modules
Keep files focused and not too large
Use consistent naming conventions
Organize imports and exports clearly
Write JSDoc comments for public APIs
Keep documentation up to date with code changes (inline docs)
Use meaningful variable and function names
Handle environment-specific APIs properly
Use TypeScript features appropriate for the configured version
Files:
packages/altair-core/src/config/options.schema.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/instructions/main.instructions.md)
Use TypeScript for implementation across the codebase
Files:
packages/altair-core/src/config/options.schema.ts
packages/altair-core/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/main.instructions.md)
Modify shared logic/types/utilities in packages/altair-core carefully, as changes impact multiple packages; ensure thorough testing
Files:
packages/altair-core/src/config/options.schema.ts
🧠 Learnings (7)
📚 Learning: 2025-10-03T16:39:27.517Z
Learnt from: CR
Repo: altair-graphql/altair PR: 0
File: .github/instructions/main.instructions.md:0-0
Timestamp: 2025-10-03T16:39:27.517Z
Learning: Applies to packages/altair-core/**/*.ts : Modify shared logic/types/utilities in packages/altair-core carefully, as changes impact multiple packages; ensure thorough testing
Applied to files:
packages/altair-core/src/config/options.schema.ts
📚 Learning: 2025-10-03T16:40:52.954Z
Learnt from: CR
Repo: altair-graphql/altair PR: 0
File: .github/instructions/nestjs-api.instructions.md:0-0
Timestamp: 2025-10-03T16:40:52.954Z
Learning: Applies to packages/altair-api/src/config/**/*.ts : Use type-safe configuration classes
Applied to files:
packages/altair-core/src/config/options.schema.ts
📚 Learning: 2025-10-03T16:40:52.954Z
Learnt from: CR
Repo: altair-graphql/altair PR: 0
File: .github/instructions/nestjs-api.instructions.md:0-0
Timestamp: 2025-10-03T16:40:52.954Z
Learning: Applies to packages/altair-api/src/config/**/*.ts : Validate configuration on startup
Applied to files:
packages/altair-core/src/config/options.schema.ts
📚 Learning: 2025-10-03T16:39:27.517Z
Learnt from: CR
Repo: altair-graphql/altair PR: 0
File: .github/instructions/main.instructions.md:0-0
Timestamp: 2025-10-03T16:39:27.517Z
Learning: Applies to packages/altair-db/**/*.ts : Keep database-related utilities in TypeScript aligned with Prisma usage in packages/altair-db
Applied to files:
packages/altair-core/src/config/options.schema.ts
📚 Learning: 2025-10-03T16:40:52.954Z
Learnt from: CR
Repo: altair-graphql/altair PR: 0
File: .github/instructions/nestjs-api.instructions.md:0-0
Timestamp: 2025-10-03T16:40:52.954Z
Learning: Applies to packages/altair-api/src/**/*.{service,repository}.ts : Use appropriate Prisma query options (select, include, where)
Applied to files:
packages/altair-core/src/config/options.schema.ts
📚 Learning: 2025-10-03T16:41:45.498Z
Learnt from: CR
Repo: altair-graphql/altair PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2025-10-03T16:41:45.498Z
Learning: Applies to packages/altair-app/**/*.{test,spec}.ts : In components, test methods, business logic, event emissions, state changes, lifecycle, and integration with injected services
Applied to files:
packages/altair-core/src/config/options.schema.ts
📚 Learning: 2025-05-29T13:44:11.906Z
Learnt from: imolorhe
Repo: altair-graphql/altair PR: 2835
File: packages/altair-api-utils/src/query.ts:7-10
Timestamp: 2025-05-29T13:44:11.906Z
Learning: For Prisma JSON type compatibility in packages/altair-api-utils/src/query.ts, the authorizationData property requires the `any` type rather than `unknown` due to specific typing constraints with Prisma's JSON type handling.
Applied to files:
packages/altair-core/src/config/options.schema.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). (15)
- GitHub Check: docker / docker
- GitHub Check: tauri / tauri (macos-latest, --target x86_64-apple-darwin)
- GitHub Check: tauri / tauri (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: electron / electron (macos-latest)
- GitHub Check: electron / electron (windows-latest)
- GitHub Check: tauri / tauri (windows-latest)
- GitHub Check: tauri / tauri (ubuntu-22.04)
- GitHub Check: electron / electron (ubuntu-latest)
- GitHub Check: tests / e2e-test (macos-latest)
- GitHub Check: tests / e2e-test (ubuntu-latest)
- GitHub Check: tests / test (macos-latest)
- GitHub Check: tests / test (ubuntu-latest)
- GitHub Check: tests / api-e2e-test
- GitHub Check: Sourcery review
- GitHub Check: build_and_preview
🔇 Additional comments (2)
packages/altair-core/src/config/options.schema.ts (2)
104-107: Schema changes are syntactically correct and safely handled by consumers.The addition of
.nullable()correctly allowsnullvalues for bothinitialSubscriptionsPayloadandinitialSettings. All existing consumers use patterns (truthiness checks and nullish coalescing) that properly handlenullvalues, making this a backward-compatible change.Since this affects
packages/altair-core, ensure thorough testing across all impacted packages.
196-200: Schema change is correct and consumers handle null properly.The addition of
.nullable()correctly allowsnullvalues forinitialSettings. The.nullable()method accepts string | null, while.optional()accepts string | undefined, and.nullish()accepts string | null | undefined. The current chain.partial().nullable().optional()validates correctly and accepts null, undefined, or a partial settings object.The consumer code in
settings.reducer.tsalready handles null values properly using the nullish coalescing operator (initialSettings ?? {}), falling back to an empty object if the value is null or undefined. Since this is inpackages/altair-core, ensure all consumers continue to handle nullish values appropriately, though the existing pattern demonstrates proper null handling.
|
Visit the preview URL for this PR (updated for commit 2d9e304): https://altair-gql--pr2972-imolorhe-make-settin-cdd7cufo.web.app (expires Tue, 30 Dec 2025 10:02:03 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 02d6323d75a99e532a38922862e269d63351a6cf |
Fixes
#2971
Checks
yarn test-buildChanges proposed in this pull request:
Summary by Sourcery
Allow configuration options for initial subscriptions payload and initial app settings to be explicitly null in the Altair config schema.
Enhancements:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.