fix: Refactor project builder lib testing and project-builder-web creation invalidation#785
Merged
kingston merged 6 commits intoFeb 26, 2026
Conversation
🦋 Changeset detectedLatest commit: a1f9ef4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 21 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 |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
✏️ Tip: You can disable in-progress messages and the fortune message in your review settings. 📝 WalkthroughWalkthroughAdds a public testing export for project-builder-lib, refactors withDefault to use ZodPrefault (removing .optional()), reorganizes and exposes test helpers, updates fixtures to use ProjectDefinitionInput, and removes many defensive optional chaining assumptions across server, web, and plugin code paths. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (9)
plugins/plugin-storage/src/storage/transformers/node.ts (1)
18-20: Guardrelationsexplicitly before.findfor clearer compiler errors.On Line 18,
model.model.relationsbeing undefined now throws a generic runtime error before the contextual error at Line 22 can run. Add an explicit invariant check so failures stay actionable.Proposed adjustment
- const foreignRelation = model.model.relations.find( + const relations = model.model.relations; + if (!relations) { + throw new Error(`Model ${model.name} is missing relations`); + } + + const foreignRelation = relations.find( (relation) => relation.id === fileRelationRef, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/plugin-storage/src/storage/transformers/node.ts` around lines 18 - 20, Guard access to model.model.relations before calling .find so undefined relations produce an explicit invariant error instead of a generic runtime error; add an explicit check for model.model.relations (e.g., if (!model.model.relations) throw new Error(...) or use an invariant helper) immediately before the line that computes foreignRelation which uses fileRelationRef and ensure the error message names the model and fileRelationRef for context so downstream code that expects foreignRelation still runs with a clear, actionable error.plugins/plugin-storage/src/storage/core/node.ts (2)
52-57: Preserve transformer-specific error context when relations are absent.Line 52 can fail with a generic property-access error if
m.model.relationsis missing, bypassing the clearer message at Line 56. Add an explicit guard before.find.Proposed adjustment
- const relation = m.model.relations.find( + const relations = m.model.relations; + if (!relations) { + throw new Error(`Model ${m.id} is missing relations`); + } + + const relation = relations.find( (r) => r.id === t.fileRelationRef, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/plugin-storage/src/storage/core/node.ts` around lines 52 - 57, The code currently calls m.model.relations.find(...) which will throw a generic property-access error if m.model.relations is missing; add an explicit guard before the .find (e.g. check Array.isArray(m.model.relations) or m.model.relations != null) and if the relations collection is absent throw a clear Error that includes the transformer id (t.id) and any model identifier (m.model.id or similar), otherwise proceed to call .find for t.fileRelationRef and preserve the existing error when the relation itself is not found.
101-104: Use an explicit invariant check foradminAppbefore reading.enabled.Line 103 currently relies on property access that can throw an opaque runtime error if
adminAppis ever missing. An explicit check keeps the fail-fast intent but improves debuggability.Proposed adjustment
+ if (!appDefinition.adminApp) { + throw new Error('appDefinition.adminApp is required'); + } + if ( !appDefinition.includeUploadComponents && !appDefinition.adminApp.enabled ) { return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/plugin-storage/src/storage/core/node.ts` around lines 101 - 104, The condition reads appDefinition.adminApp.enabled directly which can throw if adminApp is undefined; change the check to first assert or test that appDefinition.adminApp exists before accessing .enabled (e.g., replace the current combined condition in the if around appDefinition.includeUploadComponents with an explicit presence check for appDefinition.adminApp, then check adminApp.enabled), referencing the existing appDefinition.includeUploadComponents and appDefinition.adminApp.enabled expressions so the guard is applied where that condition is evaluated.packages/project-builder-cli/e2e/fixtures/server-fixture.test-helper.ts (1)
174-178: Use a unique service ID peraddProject()call.
tempDiris unique, butidis constant (Line 237). Repeated calls in one test can collide or overwrite depending on manager behavior.♻️ Suggested change
- const tempDir = path.join( + const nextProjectIdx = projectIdx++; + const tempDir = path.join( temporaryDirectory, - `project-${projectIdx}`, + `project-${nextProjectIdx}`, ); - projectIdx++; + const projectId = `test-project-${parallelIndex}-${nextProjectIdx}`; ... const service = server.builderServiceManager.addService({ - id: 'test-project', + id: projectId, directory: tempDir, - name: 'test-project', + name: projectId, isInternalExample: false, });Also applies to: 236-247
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-cli/e2e/fixtures/server-fixture.test-helper.ts` around lines 174 - 178, In addProject(), the tempDir uses projectIdx but the returned project id is a constant and can collide across calls; change the id generation in the addProject() return so it is unique per call (for example incorporate projectIdx, tempDir, a timestamp or a UUID) and ensure projectIdx is incremented before creating the id so each call to addProject() produces a distinct id; update references to the id variable accordingly (symbols: addProject, tempDir, projectIdx, id).packages/project-builder-web/src/routes/admin-sections.$appKey/-components/columns/foreign-column-form.tsx (1)
59-63: Minor inconsistency in optional chaining style.Line 62 uses
model?.model.relations.length— optional chaining stops atmodel?.model, but then accesses.relations.lengthdirectly. Ifmodel.modelexists butrelationswere undefined, this would throw.Given ZodPrefault guarantees
relationsis always defined, this is safe, but the mixed style could be confusing. Consider either:
model?.model.relations.length ?? 0(current, relies on schema guarantee)model?.model?.relations?.length ?? 0(fully defensive, but redundant now)No action required if the schema guarantees are solid.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-web/src/routes/admin-sections`.$appKey/-components/columns/foreign-column-form.tsx around lines 59 - 63, The isAvailableForModel arrow property currently uses mixed optional chaining (model?.model.relations.length ?? 0) which can be confusing and brittle; update the expression in isAvailableForModel so it consistently uses defensive optional chaining (e.g., model?.model?.relations?.length ?? 0) to avoid any potential undefined access and make intent clear when locating the check in the isAvailableForModel implementation.packages/project-builder-web/src/routes/packages/-components/new-dialog.tsx (1)
52-56: NarrowappTypeto explicit values and make branching exhaustive.Using
stringhere allows accidental unsupported values to fall into the web-port path silently. Tightening the type keeps this helper safer for future call sites.🔒 Suggested typing hardening
+type NewAppType = 'backend' | 'web'; + function getNextDevPort( config: ProjectDefinition, - appType: string, + appType: NewAppType, appName: string, ): number { const { portOffset } = config.settings.general; const existingPorts = new Set(config.apps.map((app) => app.devPort)); let port: number; if (appType === 'backend') { port = portOffset + 1; - } else { + } else if (appType === 'web') { // For web apps, compute alphabetical index including the new app const webAppNames = config.apps .filter((app) => app.type === 'web') .map((app) => app.name); webAppNames.push(appName); webAppNames.sort((a, b) => compareStrings(a, b)); const webAppIndex = webAppNames.indexOf(appName); port = portOffset + 30 + webAppIndex; + } else { + throw new Error(`Unsupported app type: ${appType}`); }Also applies to: 61-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-web/src/routes/packages/-components/new-dialog.tsx` around lines 52 - 56, Change getNextDevPort to accept a narrowed appType union (e.g., "web" | "api" | "worker" or whatever concrete app kinds your app supports) instead of string and update its internal branching to handle each union member explicitly and exhaustively (use a final default that throws for unknown types or a TypeScript never check) so unsupported values no longer fall through to the web-port path; update any call sites that pass other strings to use the new union values.packages/project-builder-web/src/routes/admin-sections.$appKey/edit.$sectionKey.tsx (1)
48-50: ComputesectionIdonce instead of per-item callback calls.
adminSectionEntityType.idFromKey(sectionKey)is recalculated for each element in both searches. Precompute it once to reduce repetition and improve clarity.♻️ Suggested refactor
beforeLoad: ({ params: { sectionKey }, context: { adminApp } }) => { if (!adminApp) return {}; + const sectionId = adminSectionEntityType.idFromKey(sectionKey); const section = adminApp.sections.find( - (s) => adminSectionEntityType.idFromKey(sectionKey) === s.id, + (s) => sectionId === s.id, );const onSubmit = handleSubmit((data) => { const { id: _, ...sectionData } = data; + const sectionId = adminSectionEntityType.idFromKey(sectionKey); return saveDefinitionWithFeedback((draftConfig) => { const webApp = draftConfig.apps.find((a) => a.id === app.id); if (webApp?.type !== 'web') return; const existingIndex = webApp.adminApp.sections.findIndex( - (s) => s.id === adminSectionEntityType.idFromKey(sectionKey), + (s) => s.id === sectionId, );Also applies to: 92-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-web/src/routes/admin-sections`.$appKey/edit.$sectionKey.tsx around lines 48 - 50, Precompute the result of adminSectionEntityType.idFromKey(sectionKey) into a local constant (e.g., sectionId) and use that constant inside the array callbacks instead of calling adminSectionEntityType.idFromKey(sectionKey) repeatedly; update the usages in the adminApp.sections.find call that assigns section and the other occurrence around lines 92-94 so both use sectionId for clarity and efficiency.plugins/plugin-storage/src/storage/admin-crud/node.ts (1)
22-24: Fail fast on missing transformer before relation lookup.This avoids a misleading “relation not found” error when the real issue is an absent file transformer.
Proposed refactor
const transformer = model.service.transformers.find( (t): t is FileTransformerDefinition => t.id === definition.modelRelationRef && t.type === 'file', ); + if (!transformer) { + throw new Error( + `Could not find file transformer ${definition.modelRelationRef} in model ${model.name}`, + ); + } const relation = model.model.relations.find( - (r) => r.id === transformer?.fileRelationRef, + (r) => r.id === transformer.fileRelationRef, ); @@ - const category = transformer?.category; + const category = transformer.category;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/plugin-storage/src/storage/admin-crud/node.ts` around lines 22 - 24, Add an explicit guard that throws/returns an immediate error when transformer is missing before attempting relation lookup: check the transformer variable (referenced as transformer) at the start of the block containing the relation lookup (the code using model.model.relations.find and transformer?.fileRelationRef) and fail fast with a clear error message indicating the missing file transformer so the subsequent search for relation (variable relation) does not produce a misleading “relation not found” error.packages/project-builder-lib/src/testing/definition-helpers.test-helper.ts (1)
26-26: Move schema creation insidecreateTestModelto ensure test isolation.The module-level
modelSchemaconstant reuses a single parser context across allcreateTestModel()calls. While no test failures are currently visible, the codebase explicitly documents this pattern as a concern (seecreatePluginWithConfigSchemacomment about avoiding metadata accumulation on shared schemas via the global ref registry). Creating a fresh context per call ensures clean test isolation.💡 Suggested change
-const modelSchema = createModelSchema(createEmptyParserContext()); - export function createTestModel( model: Partial<ModelConfigInput> = {}, ): ModelConfig { @@ - return modelSchema.parse(input); + return createModelSchema(createEmptyParserContext()).parse(input); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-lib/src/testing/definition-helpers.test-helper.ts` at line 26, The module-level modelSchema created via createModelSchema(createEmptyParserContext()) should be moved into createTestModel so each call gets a fresh parser context; remove the top-level const modelSchema and instead call createModelSchema(createEmptyParserContext()) inside createTestModel (the function that builds test models) to avoid shared/global metadata accumulation from the parser's ref registry and ensure test isolation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/project-builder-cli/e2e/fixtures/server-fixture.test-helper.ts`:
- Around line 223-233: readProjectDefinition() assumes
baseplate/project-definition.json always exists and throws ENOENT when
addProject() runs with no input (no file written). Fix by guarding the read:
before calling fs.readFile in readProjectDefinition(), check for file existence
(e.g., fs.pathExists or fs.stat) and either return a sensible default/undefined
ProjectDefinition or throw a clear error; update any callers that expect a
nullable value. Refer to readProjectDefinition, writeProjectDefinition, tempDir
and the path 'baseplate/project-definition.json' when making the change.
In `@packages/project-builder-lib/tsconfig.build.json`:
- Line 13: Replace the unsupported extglob pattern
"src/!(testing)/**/*.test-helper.ts" in tsconfig.build.json: remove that include
entry (it is ignored) and instead either add an explicit exclude like
"src/testing/**" to exclude the testing directory from the build or, if you
meant to include test-helper files from specific dirs, list those directories
explicitly (e.g. add explicit include/exclude patterns for the intended paths).
Ensure the exact problematic pattern "src/!(testing)/**/*.test-helper.ts" is
removed or replaced with the explicit patterns.
In
`@packages/project-builder-web/src/app/project-definition-provider/project-definition-provider.tsx`:
- Around line 172-173: The router.invalidate() call inside the .then() success
handler should be isolated so its rejection doesn't override a successful save:
after the save success path (the .then() that currently awaits
router.invalidate()), wrap the router.invalidate() call in its own try-catch
block (referencing router.invalidate()) and ensure the original success response
is returned regardless of invalidate errors; log or handle the invalidate error
inside the catch without altering the returned { success: true } from the save
flow and do not let it propagate to the existing .catch() that reports save
failure.
---
Nitpick comments:
In `@packages/project-builder-cli/e2e/fixtures/server-fixture.test-helper.ts`:
- Around line 174-178: In addProject(), the tempDir uses projectIdx but the
returned project id is a constant and can collide across calls; change the id
generation in the addProject() return so it is unique per call (for example
incorporate projectIdx, tempDir, a timestamp or a UUID) and ensure projectIdx is
incremented before creating the id so each call to addProject() produces a
distinct id; update references to the id variable accordingly (symbols:
addProject, tempDir, projectIdx, id).
In `@packages/project-builder-lib/src/testing/definition-helpers.test-helper.ts`:
- Line 26: The module-level modelSchema created via
createModelSchema(createEmptyParserContext()) should be moved into
createTestModel so each call gets a fresh parser context; remove the top-level
const modelSchema and instead call createModelSchema(createEmptyParserContext())
inside createTestModel (the function that builds test models) to avoid
shared/global metadata accumulation from the parser's ref registry and ensure
test isolation.
In
`@packages/project-builder-web/src/routes/admin-sections`.$appKey/-components/columns/foreign-column-form.tsx:
- Around line 59-63: The isAvailableForModel arrow property currently uses mixed
optional chaining (model?.model.relations.length ?? 0) which can be confusing
and brittle; update the expression in isAvailableForModel so it consistently
uses defensive optional chaining (e.g., model?.model?.relations?.length ?? 0) to
avoid any potential undefined access and make intent clear when locating the
check in the isAvailableForModel implementation.
In
`@packages/project-builder-web/src/routes/admin-sections`.$appKey/edit.$sectionKey.tsx:
- Around line 48-50: Precompute the result of
adminSectionEntityType.idFromKey(sectionKey) into a local constant (e.g.,
sectionId) and use that constant inside the array callbacks instead of calling
adminSectionEntityType.idFromKey(sectionKey) repeatedly; update the usages in
the adminApp.sections.find call that assigns section and the other occurrence
around lines 92-94 so both use sectionId for clarity and efficiency.
In `@packages/project-builder-web/src/routes/packages/-components/new-dialog.tsx`:
- Around line 52-56: Change getNextDevPort to accept a narrowed appType union
(e.g., "web" | "api" | "worker" or whatever concrete app kinds your app
supports) instead of string and update its internal branching to handle each
union member explicitly and exhaustively (use a final default that throws for
unknown types or a TypeScript never check) so unsupported values no longer fall
through to the web-port path; update any call sites that pass other strings to
use the new union values.
In `@plugins/plugin-storage/src/storage/admin-crud/node.ts`:
- Around line 22-24: Add an explicit guard that throws/returns an immediate
error when transformer is missing before attempting relation lookup: check the
transformer variable (referenced as transformer) at the start of the block
containing the relation lookup (the code using model.model.relations.find and
transformer?.fileRelationRef) and fail fast with a clear error message
indicating the missing file transformer so the subsequent search for relation
(variable relation) does not produce a misleading “relation not found” error.
In `@plugins/plugin-storage/src/storage/core/node.ts`:
- Around line 52-57: The code currently calls m.model.relations.find(...) which
will throw a generic property-access error if m.model.relations is missing; add
an explicit guard before the .find (e.g. check Array.isArray(m.model.relations)
or m.model.relations != null) and if the relations collection is absent throw a
clear Error that includes the transformer id (t.id) and any model identifier
(m.model.id or similar), otherwise proceed to call .find for t.fileRelationRef
and preserve the existing error when the relation itself is not found.
- Around line 101-104: The condition reads appDefinition.adminApp.enabled
directly which can throw if adminApp is undefined; change the check to first
assert or test that appDefinition.adminApp exists before accessing .enabled
(e.g., replace the current combined condition in the if around
appDefinition.includeUploadComponents with an explicit presence check for
appDefinition.adminApp, then check adminApp.enabled), referencing the existing
appDefinition.includeUploadComponents and appDefinition.adminApp.enabled
expressions so the guard is applied where that condition is evaluated.
In `@plugins/plugin-storage/src/storage/transformers/node.ts`:
- Around line 18-20: Guard access to model.model.relations before calling .find
so undefined relations produce an explicit invariant error instead of a generic
runtime error; add an explicit check for model.model.relations (e.g., if
(!model.model.relations) throw new Error(...) or use an invariant helper)
immediately before the line that computes foreignRelation which uses
fileRelationRef and ensure the error message names the model and fileRelationRef
for context so downstream code that expects foreignRelation still runs with a
clear, actionable error.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (43)
.changeset/project-builder-lib-testing-export.md.changeset/withdefault-removes-optional-wrapper.mdpackages/project-builder-cli/e2e/fixtures/server-fixture.test-helper.tspackages/project-builder-lib/package.jsonpackages/project-builder-lib/src/definition/model/model-field-utils.tspackages/project-builder-lib/src/definition/model/model-field-utils.unit.test.tspackages/project-builder-lib/src/definition/model/model-utils.tspackages/project-builder-lib/src/references/extend-parser-context-with-refs.tspackages/project-builder-lib/src/references/extract-definition-refs.unit.test.tspackages/project-builder-lib/src/schema/creator/extend-parser-context-with-defaults.tspackages/project-builder-lib/src/schema/models/models.tspackages/project-builder-lib/src/testing/definition-helpers.test-helper.tspackages/project-builder-lib/src/testing/expression-stub-parser.test-helper.tspackages/project-builder-lib/src/testing/index.tspackages/project-builder-lib/src/testing/parser-context.test-helper.tspackages/project-builder-lib/src/tools/model-merger/model-merger.unit.test.tspackages/project-builder-lib/tsconfig.build.jsonpackages/project-builder-server/src/compiler/backend/authorizers.tspackages/project-builder-server/src/compiler/backend/graphql.tspackages/project-builder-server/src/compiler/backend/models.tspackages/project-builder-server/src/compiler/web/admin/crud/index.tspackages/project-builder-server/src/compiler/web/admin/index.tspackages/project-builder-server/src/compiler/web/admin/sections.tspackages/project-builder-server/src/compiler/web/web-compiler.tspackages/project-builder-server/src/core-modules/admin-crud-column-compiler.tspackages/project-builder-server/src/core-modules/admin-crud-input-compiler.tspackages/project-builder-web/src/app/app.tsxpackages/project-builder-web/src/app/project-definition-provider/project-definition-provider.tsxpackages/project-builder-web/src/routes/admin-sections.$appKey/-components/columns/foreign-column-form.tsxpackages/project-builder-web/src/routes/admin-sections.$appKey/-components/crud-table-actions-form.tsxpackages/project-builder-web/src/routes/admin-sections.$appKey/-components/inputs/embedded-local-input-config.tsxpackages/project-builder-web/src/routes/admin-sections.$appKey/-components/inputs/foreign-input-config.tsxpackages/project-builder-web/src/routes/admin-sections.$appKey/-components/new-admin-section-dialog.tsxpackages/project-builder-web/src/routes/admin-sections.$appKey/edit.$sectionKey.tsxpackages/project-builder-web/src/routes/admin-sections.$appKey/index.tsxpackages/project-builder-web/src/routes/admin-sections.$appKey/route.tsxpackages/project-builder-web/src/routes/data/enums/-hooks/use-enum-form.tsxpackages/project-builder-web/src/routes/data/models/-components/new-model-dialog.tsxpackages/project-builder-web/src/routes/packages/-components/new-dialog.tsxplugins/plugin-auth/src/local-auth/core/node.tsplugins/plugin-storage/src/storage/admin-crud/node.tsplugins/plugin-storage/src/storage/core/node.tsplugins/plugin-storage/src/storage/transformers/node.ts
| "exclude": [ | ||
| "src/**/*.test.ts", | ||
| "src/**/*.test-helper.ts", | ||
| "src/!(testing)/**/*.test-helper.ts", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
TypeScript tsconfig.json include exclude glob patterns extglob support !(pattern)
💡 Result:
TypeScript’s tsconfig.json include/exclude patterns are not full Bash-style globs and do not support extglob like !(pattern).
Supported wildcards are only: *, ?, and **/ (recursive directories) in both include and exclude. Anything like !(...), +(...), @(…), {a,b}, etc. is not part of the supported syntax. [1]
Also note: exclude only filters what include finds—a file can still be part of the program if it’s pulled in via import, /// <reference>, types, or files. [1] [2]
Workaround for “everything except X”: use a broad include, then list the unwanted paths in exclude explicitly, e.g.
{
"include": ["src/**/*"],
"exclude": ["src/**/__generated__/**", "src/**/*.spec.ts"]
}Sources: TypeScript TSConfig reference (include/exclude) [1], TypeScript Wiki FAQ on exclude behavior [2].
🏁 Script executed:
#!/bin/bash
# First, examine the tsconfig.build.json to understand the context
echo "=== tsconfig.build.json context ==="
cat packages/project-builder-lib/tsconfig.build.json
echo ""
echo "=== Searching for *.test-helper.ts files ==="
fd --type f '\.test-helper\.ts$' packages/project-builder-lib/
echo ""
echo "=== Specifically checking src/testing vs other locations ==="
fd --type f '\.test-helper\.ts$' packages/project-builder-lib/ | while read file; do
if [[ $file == *"src/testing"* ]]; then
echo "IN testing: $file"
else
echo "OUTSIDE testing: $file"
fi
doneRepository: halfdomelabs/baseplate
Length of output: 1243
Line 13 uses an unsupported extglob pattern that has no effect.
TypeScript's tsconfig.json include/exclude matchers support only *, ?, and **/ wildcards. The pattern !(testing) is not recognized and will be ignored. While test-helper files currently only exist in src/testing/, this exclude rule is non-functional and should be replaced with explicit paths or removed if it's redundant. Consider either listing specific directories to exclude or using a different strategy for separating test helpers from compiled output.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/project-builder-lib/tsconfig.build.json` at line 13, Replace the
unsupported extglob pattern "src/!(testing)/**/*.test-helper.ts" in
tsconfig.build.json: remove that include entry (it is ignored) and instead
either add an explicit exclude like "src/testing/**" to exclude the testing
directory from the build or, if you meant to include test-helper files from
specific dirs, list those directories explicitly (e.g. add explicit
include/exclude patterns for the intended paths). Ensure the exact problematic
pattern "src/!(testing)/**/*.test-helper.ts" is removed or replaced with the
explicit patterns.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/project-builder-cli/e2e/fixtures/server-fixture.test-helper.ts (1)
114-116:⚠️ Potential issue | 🟡 MinorType mismatch: interface declares
ProjectDefinitionbut implementation usesProjectDefinitionInput.The
writeProjectDefinitionparameter in the interface is typed asProjectDefinition, but the actual implementation at line 215 usesProjectDefinitionInput. These are semantically different types (schema output vs input).🔧 Proposed fix
writeProjectDefinition: ( - projectDefinition: ProjectDefinition, + projectDefinition: ProjectDefinitionInput, ) => Promise<void>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-cli/e2e/fixtures/server-fixture.test-helper.ts` around lines 114 - 116, The interface declaration for writeProjectDefinition currently expects ProjectDefinition but the implementation uses ProjectDefinitionInput; update one side so both use the same type: either change the interface method signature writeProjectDefinition(projectDefinition: ProjectDefinition) => Promise<void> to accept ProjectDefinitionInput, or convert the implementation to construct/validate a ProjectDefinition before calling writeProjectDefinition; reference the symbols writeProjectDefinition, ProjectDefinition, and ProjectDefinitionInput and make the typesconsistent across the interface and its implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/project-builder-cli/e2e/fixtures/server-fixture.test-helper.ts`:
- Around line 114-116: The interface declaration for writeProjectDefinition
currently expects ProjectDefinition but the implementation uses
ProjectDefinitionInput; update one side so both use the same type: either change
the interface method signature writeProjectDefinition(projectDefinition:
ProjectDefinition) => Promise<void> to accept ProjectDefinitionInput, or convert
the implementation to construct/validate a ProjectDefinition before calling
writeProjectDefinition; reference the symbols writeProjectDefinition,
ProjectDefinition, and ProjectDefinitionInput and make the typesconsistent
across the interface and its implementation.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/project-builder-cli/e2e/fixtures/server-fixture.test-helper.tspackages/project-builder-web/src/app/project-definition-provider/project-definition-provider.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/project-builder-web/src/app/project-definition-provider/project-definition-provider.tsx
kingston
deleted the
kingston/eng-981-fix-redirect-after-creating-new-enumpluginapp
branch
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Chores