fix: Fix plugin config migration version not being set correctly when enabling a new plugin via web config editor #709
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 243c531 The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis PR fixes a bug where plugin config migration versions were not set correctly when enabling new plugins via the web config editor. The main change updates the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ 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). (5)
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 |
![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
🧹 Nitpick comments (2)
packages/project-builder-lib/src/tools/model-merger/model-merger.ts (2)
460-469: Consider adding a comment to clarify the two-step lookup pattern.The change from direct
byIdOrThrowlookup tobyName ?? byIdOrThrowmakes the resolution more flexible by accepting both names and IDs. This is useful since the resolver function may receive either format after the serialization/deserialization cycle. However, the code would benefit from a brief comment explaining this.Additionally, the improved error message on line 466 that includes
${model.name}is clearer than using the potentially ambiguousmodelIdparameter.Optional: Add clarifying comment
}, (modelId, fieldName) => { const siblingModel = siblingModels.find( (m) => m.id === modelId || m.name === modelId, ); if (siblingModel) { const field = siblingModel.model.fields.find( (f) => f.name === fieldName, ); if (!field) { throw new Error( `Field ${fieldName} not found in sibling model ${modelId}`, ); } return field.id; } + // modelId may be either a name (from desired state) or an ID (from current state) + // Try name-based lookup first, then fall back to ID-based lookup const model = ModelUtils.byName(definitionContainer.definition, modelId) ?? ModelUtils.byIdOrThrow(definitionContainer.definition, modelId); const field = model.model.fields.find((f) => f.name === fieldName); if (!field) { throw new Error( `Field ${fieldName} not found in model ${model.name}`, ); } return field.id; },
512-516: Consistent two-step lookup pattern; consider adding a clarifying comment.Similar to the earlier change, this implements the same defensive two-step lookup pattern (
byName ?? byIdOrThrow) formodelRefresolution, and explicitly returns the resolvedmodel.id. This ensures consistency and handles both name and ID references robustly.The same suggestion applies here: a brief comment explaining why both lookups are necessary would improve maintainability.
Optional: Add clarifying comment
(modelRef) => { const siblingModel = siblingModels.find( (m) => m.id === modelRef || m.name === modelRef, ); if (siblingModel) { return siblingModel.id; } + // modelRef may be either a name or an ID after serialization/deserialization + // Try name-based lookup first, then fall back to ID-based lookup const model = ModelUtils.byName(definitionContainer.definition, modelRef) ?? ModelUtils.byIdOrThrow(definitionContainer.definition, modelRef); return model.id; },
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
.changeset/fix-plugin-config-migration-version.mdpackages/project-builder-lib/src/definition/plugins/plugin-utils.tspackages/project-builder-lib/src/definition/plugins/plugin-utils.unit.test.tspackages/project-builder-lib/src/tools/model-merger/model-merger.tspackages/project-builder-web/src/routes/plugins/-components/plugin-card.tsxplugins/plugin-auth/src/auth/core/components/auth-definition-editor.tsxplugins/plugin-auth/src/auth0/core/components/auth0-definition-editor.tsxplugins/plugin-auth/src/local-auth/core/components/local-auth-definition-editor.tsxplugins/plugin-auth/src/placeholder-auth/core/components/placeholder-auth-definition-editor.tsxplugins/plugin-queue/src/pg-boss/core/components/pg-boss-definition-editor.tsxplugins/plugin-queue/src/queue/core/components/queue-definition-editor.tsxplugins/plugin-storage/src/storage/core/components/storage-definition-editor.tsx
🧰 Additional context used
📓 Path-based instructions (8)
plugins/**/plugin-*/**/*.{jsx,tsx}
📄 CodeRabbit inference engine (plugins/CLAUDE.md)
All CSS classes used in
classNameattributes within plugin components MUST be prefixed with the plugin name (e.g.,auth:,storage:) to avoid style conflicts between plugins and the main application
Files:
plugins/plugin-storage/src/storage/core/components/storage-definition-editor.tsxplugins/plugin-auth/src/placeholder-auth/core/components/placeholder-auth-definition-editor.tsxplugins/plugin-auth/src/auth/core/components/auth-definition-editor.tsxplugins/plugin-auth/src/auth0/core/components/auth0-definition-editor.tsxplugins/plugin-queue/src/pg-boss/core/components/pg-boss-definition-editor.tsxplugins/plugin-auth/src/local-auth/core/components/local-auth-definition-editor.tsxplugins/plugin-queue/src/queue/core/components/queue-definition-editor.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/code-style.mdc)
**/*.{ts,tsx}: Use TypeScript with strict type checking enabled
Always include return types on top-level functions including React components (React.ReactElement)
Include absolute paths in import statements via tsconfig paths (@src/is the alias forsrc/)
If a particular interface or type is not exported, change the file so it is exported
If caught on a typing loop where forcing theanytype is necessary, do not iterate too much - leave the typing as broken and let the user fix itIf target code is not easily testable, refactor it to be more testable (e.g., export types or functions)
**/*.{ts,tsx}: Import components from '@baseplate-dev/ui-components' package for UI development (e.g., Button, Input, Card, Dialog, etc.)
Use form components with React Hook Form controller variants (InputField, TextareaField, SelectField, CheckboxField, SwitchField, ComboboxField, MultiComboboxField, ColorPickerField, DatePickerField, DateTimePickerField)
Use SidebarLayout, Card, Breadcrumb, NavigationMenu, and NavigationTabs components for consistent layout structure from @baseplate-dev/ui-components
Use Dialog, ConfirmDialog, and useConfirmDialog from @baseplate-dev/ui-components for modal dialogs and confirmation interactions
Always usecompareStringsfrom@baseplate-dev/utilsinstead ofString.prototype.localeCompare()for code generation, file sorting, and internal data structures
If a particular interface or type is not exported, modify the file to export it
Use TsCodeFragment for composable code pieces and TsCodeUtils for manipulating fragments when generating TypeScript code
Create generators usingcreateGeneratorwith configuration via descriptor schema (Zod), organizing into one or more tasks created withcreateGeneratorTask
Tasks should haverun(initialization) andbuild(code generation) phases, export and consume providers, and may be organized into phases for ordered execution
Use provider scopes to control visibility and prevent collisions be...
Files:
plugins/plugin-storage/src/storage/core/components/storage-definition-editor.tsxpackages/project-builder-lib/src/tools/model-merger/model-merger.tspackages/project-builder-lib/src/definition/plugins/plugin-utils.unit.test.tsplugins/plugin-auth/src/placeholder-auth/core/components/placeholder-auth-definition-editor.tsxplugins/plugin-auth/src/auth/core/components/auth-definition-editor.tsxplugins/plugin-auth/src/auth0/core/components/auth0-definition-editor.tsxplugins/plugin-queue/src/pg-boss/core/components/pg-boss-definition-editor.tsxplugins/plugin-auth/src/local-auth/core/components/local-auth-definition-editor.tsxpackages/project-builder-lib/src/definition/plugins/plugin-utils.tsplugins/plugin-queue/src/queue/core/components/queue-definition-editor.tsxpackages/project-builder-web/src/routes/plugins/-components/plugin-card.tsx
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/code-style.mdc)
**/*.{ts,tsx,js}: Node 16 module resolution - include file extensions in imports (.js)
Sort imports by group: external libs first, then local imports
Use camelCase for variables/functions, PascalCase for types/classes
Order functions such that functions are placed below the variables/functions they use
Prefer using nullish coalescing operator (??) instead of logical or (||), enforced via ESLint rule
Prefer barrel exports e.g.export * from './foo.js'instead of individual named exports
Use console.info/warn/error instead of console.log
Files:
plugins/plugin-storage/src/storage/core/components/storage-definition-editor.tsxpackages/project-builder-lib/src/tools/model-merger/model-merger.tspackages/project-builder-lib/src/definition/plugins/plugin-utils.unit.test.tsplugins/plugin-auth/src/placeholder-auth/core/components/placeholder-auth-definition-editor.tsxplugins/plugin-auth/src/auth/core/components/auth-definition-editor.tsxplugins/plugin-auth/src/auth0/core/components/auth0-definition-editor.tsxplugins/plugin-queue/src/pg-boss/core/components/pg-boss-definition-editor.tsxplugins/plugin-auth/src/local-auth/core/components/local-auth-definition-editor.tsxpackages/project-builder-lib/src/definition/plugins/plugin-utils.tsplugins/plugin-queue/src/queue/core/components/queue-definition-editor.tsxpackages/project-builder-web/src/routes/plugins/-components/plugin-card.tsx
.changeset/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Add a new Changeset in the
.changeset/directory for new features or changes, with format'package-name': patchand description of the feature or change
Files:
.changeset/fix-plugin-config-migration-version.md
**/*.{unit.test.ts,int.test.ts}
📄 CodeRabbit inference engine (.cursor/rules/code-style.mdc)
**/*.{unit.test.ts,int.test.ts}: Unit tests must use.unit.test.tssuffix, integration tests must use.int.test.tssuffix
Always import vitest globals explicitly (describe, it, expect)
Files:
packages/project-builder-lib/src/definition/plugins/plugin-utils.unit.test.ts
**/*.{unit,int}.test.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/testing.mdc)
**/*.{unit,int}.test.{ts,tsx}: Use descriptive test names that explain what is being tested
Structure tests with Arrange-Act-Assert: clear setup, execution, and verification phases
Always mock external API calls and file system operations
Each test should be independent and not rely on other tests
Leverage TypeScript for type-safe mocking in tests
Focus on testing public methods and behaviors, not implementation details
Each test should verify one specific behavior
Extract repeated logic into helper functions to reduce duplication
Test broad behavior and common edge cases intelligently to avoid slowing down CI with excessive tests
For file system operations in tests, use memfs and mocknode:fsandnode:fs/promises
When using globby with mocked fs, pass the mocked fs adapter to the globby call
Files:
packages/project-builder-lib/src/definition/plugins/plugin-utils.unit.test.ts
**/*.unit.test.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/testing.mdc)
Unit tests are colocated with source files using
.unit.test.tssuffix
Files:
packages/project-builder-lib/src/definition/plugins/plugin-utils.unit.test.ts
packages/project-builder-web/**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/ui-rules.mdc)
In plugins, prefix all Tailwind classes with the plugin name (e.g.,
auth-,storage-)
Files:
packages/project-builder-web/src/routes/plugins/-components/plugin-card.tsx
🧠 Learnings (10)
📚 Learning: 2025-11-25T22:46:20.505Z
Learnt from: CR
Repo: halfdomelabs/baseplate PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T22:46:20.505Z
Learning: Applies to .changeset/*.md : Add a new Changeset in the `.changeset/` directory for new features or changes, with format `'package-name': patch` and description of the feature or change
Applied to files:
.changeset/fix-plugin-config-migration-version.md
📚 Learning: 2025-11-24T19:45:19.136Z
Learnt from: CR
Repo: halfdomelabs/baseplate PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-11-24T19:45:19.136Z
Learning: Applies to **/*.{unit,int}.test.{ts,tsx} : Leverage TypeScript for type-safe mocking in tests
Applied to files:
packages/project-builder-lib/src/definition/plugins/plugin-utils.unit.test.ts
📚 Learning: 2025-11-24T19:44:46.506Z
Learnt from: CR
Repo: halfdomelabs/baseplate PR: 0
File: examples/todo-with-auth0/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:44:46.506Z
Learning: Applies to examples/todo-with-auth0/**/*.{unit,int}.test.ts : Import test functions explicitly from 'vitest' instead of using globals. Example: `import { describe, expect, it } from 'vitest';`
Applied to files:
packages/project-builder-lib/src/definition/plugins/plugin-utils.unit.test.ts
📚 Learning: 2025-11-24T19:45:19.136Z
Learnt from: CR
Repo: halfdomelabs/baseplate PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-11-24T19:45:19.136Z
Learning: Applies to **/*.{unit,int}.test.{ts,tsx} : Test broad behavior and common edge cases intelligently to avoid slowing down CI with excessive tests
Applied to files:
packages/project-builder-lib/src/definition/plugins/plugin-utils.unit.test.ts
📚 Learning: 2025-11-24T19:44:33.994Z
Learnt from: CR
Repo: halfdomelabs/baseplate PR: 0
File: examples/blog-with-auth/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:44:33.994Z
Learning: Applies to examples/blog-with-auth/**/*.test.ts : Import test functions explicitly from 'vitest' instead of relying on globals (e.g., `import { describe, expect, it } from 'vitest';`)
Applied to files:
packages/project-builder-lib/src/definition/plugins/plugin-utils.unit.test.ts
📚 Learning: 2025-11-24T19:45:01.582Z
Learnt from: CR
Repo: halfdomelabs/baseplate PR: 0
File: .cursor/rules/code-style.mdc:0-0
Timestamp: 2025-11-24T19:45:01.582Z
Learning: Applies to **/*.{unit.test.ts,int.test.ts} : Always import vitest globals explicitly (describe, it, expect)
Applied to files:
packages/project-builder-lib/src/definition/plugins/plugin-utils.unit.test.ts
📚 Learning: 2025-04-23T06:44:30.952Z
Learnt from: kingston
Repo: halfdomelabs/baseplate PR: 510
File: packages/project-builder-server/src/sync/conflict-file-monitor.test.ts:19-24
Timestamp: 2025-04-23T06:44:30.952Z
Learning: In the project-builder-server test suite, Vitest automocks for 'node:fs' and 'node:fs/promises' are already configured to use memfs without needing explicit implementation replacement in each test file.
Applied to files:
packages/project-builder-lib/src/definition/plugins/plugin-utils.unit.test.ts
📚 Learning: 2025-11-24T19:45:19.136Z
Learnt from: CR
Repo: halfdomelabs/baseplate PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-11-24T19:45:19.136Z
Learning: Applies to **/*.{unit,int}.test.{ts,tsx} : Extract repeated logic into helper functions to reduce duplication
Applied to files:
packages/project-builder-lib/src/definition/plugins/plugin-utils.unit.test.ts
📚 Learning: 2025-11-24T19:45:27.654Z
Learnt from: CR
Repo: halfdomelabs/baseplate PR: 0
File: .cursor/rules/ui-rules.mdc:0-0
Timestamp: 2025-11-24T19:45:27.654Z
Learning: Applies to packages/project-builder-web/**/*.tsx : In plugins, prefix all Tailwind classes with the plugin name (e.g., `auth-`, `storage-`)
Applied to files:
packages/project-builder-lib/src/definition/plugins/plugin-utils.tspackages/project-builder-web/src/routes/plugins/-components/plugin-card.tsx
📚 Learning: 2025-11-24T19:45:27.654Z
Learnt from: CR
Repo: halfdomelabs/baseplate PR: 0
File: .cursor/rules/ui-rules.mdc:0-0
Timestamp: 2025-11-24T19:45:27.654Z
Learning: Applies to +(*.tsx|packages/project-builder-web/**/*.tsx|packages/ui-components/**/*.tsx) : Use ShadCN-based components from `baseplate-dev/ui-components` instead of creating custom components
Applied to files:
packages/project-builder-web/src/routes/plugins/-components/plugin-card.tsx
🧬 Code graph analysis (3)
packages/project-builder-lib/src/tools/model-merger/model-merger.ts (1)
packages/project-builder-lib/src/definition/model/model-utils.ts (1)
ModelUtils(107-119)
packages/project-builder-lib/src/definition/plugins/plugin-utils.unit.test.ts (4)
packages/project-builder-lib/src/plugins/plugins.test-utils.ts (2)
createTestPluginMetadata(16-35)createTestMigration(68-80)packages/project-builder-lib/src/plugins/spec/config-spec.ts (2)
PluginConfigMigration(12-26)PluginConfigSpec(31-43)packages/project-builder-lib/src/schema/plugins/definition.ts (1)
BasePluginDefinition(17-17)packages/project-builder-lib/src/definition/project-definition-container.test-utils.ts (1)
createTestProjectDefinitionContainer(43-72)
packages/project-builder-web/src/routes/plugins/-components/plugin-card.tsx (1)
packages/project-builder-lib/src/definition/plugins/plugin-utils.ts (1)
PluginUtils(135-142)
⏰ 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). (5)
- GitHub Check: Verify Sync (blog-with-auth)
- GitHub Check: Verify Sync (todo-with-auth0)
- GitHub Check: Test E2E
- GitHub Check: test
- GitHub Check: Lint
🔇 Additional comments (16)
plugins/plugin-auth/src/local-auth/core/components/local-auth-definition-editor.tsx (1)
99-104: LGTM! Correctly updated to pass the full container.The change from
definitionContainer.pluginStoretodefinitionContaineraligns with the updatedsetPluginConfigsignature, enabling proper migration version calculation for newly enabled plugins.plugins/plugin-queue/src/pg-boss/core/components/pg-boss-definition-editor.tsx (1)
65-70: LGTM! Correctly updated to pass the full container.The change aligns with the updated
setPluginConfigsignature.plugins/plugin-auth/src/placeholder-auth/core/components/placeholder-auth-definition-editor.tsx (1)
90-95: LGTM! Correctly updated to pass the full container.The change aligns with the updated
setPluginConfigsignature..changeset/fix-plugin-config-migration-version.md (1)
1-5: LGTM! Changeset follows proper format.The patch version bump and description accurately reflect the bug fix being implemented.
plugins/plugin-queue/src/queue/core/components/queue-definition-editor.tsx (1)
106-111: LGTM! Correctly updated to pass the full container.The change aligns with the updated
setPluginConfigsignature.plugins/plugin-auth/src/auth/core/components/auth-definition-editor.tsx (1)
117-125: LGTM! Correctly updated to pass the full container.The change aligns with the updated
setPluginConfigsignature.packages/project-builder-web/src/routes/plugins/-components/plugin-card.tsx (1)
62-75: LGTM! This is where the bug fix takes effect.By passing
definitionContainerinstead ofimplementations, thesetPluginConfigfunction can now construct a temporary plugin store internally that includes the new plugin, enabling it to correctly calculate and set the migration version for newly enabled plugins.The
implementationsvariable created earlier (lines 49-53) is still appropriately used for thewebConfiglookup.plugins/plugin-auth/src/auth0/core/components/auth0-definition-editor.tsx (1)
90-95: LGTM! Correctly updated to pass the full container.The change aligns with the updated
setPluginConfigsignature.plugins/plugin-storage/src/storage/core/components/storage-definition-editor.tsx (1)
103-108: LGTM! Correct alignment with refactored API.The update correctly passes
updatedData(with the transformedfeatureRef) and the fulldefinitionContainerto match the newsetPluginConfigsignature. This enables proper migration version calculation for newly enabled plugins.packages/project-builder-lib/src/definition/plugins/plugin-utils.unit.test.ts (4)
21-62: Well-structured test helper for container-based plugin testing.The helper function properly simulates plugin initialization by registering migrations during module initialization, which matches the actual plugin system behavior. This enables accurate testing of migration version calculation.
65-102: LGTM! Correctly tests new plugin migration version.This test verifies the core fix: when adding a new plugin with migrations,
configSchemaVersionis properly set to the latest migration version (2 in this case).
104-138: LGTM! Properly tests edge case of plugin without migrations.This test correctly verifies that when adding a plugin with no migrations,
configSchemaVersionis set toundefinedas expected.
140-193: LGTM! Validates schema version preservation for existing plugins.This test correctly verifies that updating an existing plugin's config preserves the original
configSchemaVersion, ensuring backward compatibility and preventing unintended version changes.packages/project-builder-lib/src/definition/plugins/plugin-utils.ts (3)
1-15: LGTM! Import changes correctly support the refactored API.The imports properly reflect the signature change from
PluginImplementationStoretoProjectDefinitionContainer, and add the necessarycreatePluginImplementationStoreWithNewPluginsutility for temporary store creation.
55-60: LGTM! Signature change enables proper migration version calculation.Accepting
ProjectDefinitionContainerinstead ofPluginImplementationStoreprovides access to both the plugin store and definition, enabling creation of a temporary implementation store that includes the new plugin for proper migration registration.
63-96: Excellent fix! Properly handles migration version for new plugins.The implementation correctly differentiates between adding a new plugin and updating an existing one:
New plugin path (lines 65-90): Creates a temporary
PluginImplementationStorethat includes the new plugin, allowing its migrations to be registered and the correctlastMigrationVersionto be retrieved. This solves the core issue in the PR.Existing plugin path (lines 91-96): Preserves all existing fields including
configSchemaVersion, ensuring backward compatibility.The logic is clean, well-commented, and directly addresses the bug described in the PR objectives.
kingston
deleted the
kingston/eng-933-cannot-find-model-by-id-for-user-when-enabling-storage
branch
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.