fix: Fix usage of register entity type URL#786
Merged
kingston merged 5 commits intoFeb 26, 2026
Merged
Conversation
…with a typed discriminated union navigation target syst
🦋 Changeset detectedLatest commit: f190c1d 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 |
📝 WalkthroughWalkthroughThis PR refactors entity-type navigation and schema handling by introducing a plugin-spec-based URL registration flow with typed discriminated unions, centralizing schema instantiation through a parser-context pattern, and updating reference-handling APIs to accept concrete Zod schemas instead of creator functions. Changes
Sequence Diagram(s)sequenceDiagram
participant Component as UI Component
participant DefContainer as ProjectDefinitionContainer
participant PluginContainer as PluginSpecStore
participant WebSpec as entityTypeUrlWebSpec
participant NavService as entity-type service
participant NavTarget as EntityNavTarget
Component->>DefContainer: get entity
Component->>NavService: getEntityNavOptions(defContainer, entity, pluginContainer)
NavService->>DefContainer: resolveParentEntity(parentId)
DefContainer-->>NavService: parent entity (validated)
NavService->>PluginContainer: get entityTypeUrlWebSpec
PluginContainer-->>NavService: web spec instance
NavService->>WebSpec: getNavBuilder(entityType)
WebSpec-->>NavService: builder function
NavService->>NavService: builder(params) → EntityNavTarget
NavService->>NavTarget: resolveEntityNavTarget(target)
NavTarget-->>NavService: ValidateNavigateOptions
NavService-->>Component: navigation options
Component->>Component: apply spread options to link
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 2
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-web/src/components/ref-issue-dialog/ref-issue-dialog.tsx (1)
65-74:⚠️ Potential issue | 🟠 MajorLogic bug: Path length comparison is inverted.
The condition on line 70 should select the entity with the longest matching path (deepest ancestor), but the current logic keeps
referenceParentwhen its path is longer than the candidate's path. This means it will select the shortest matching path instead.🐛 Proposed fix
if ( issuePath.startsWith(entityPath) && (!referenceParent || - referenceParent.path.length > entityPath.length) + referenceParent.path.length < entityPath.length) ) { referenceParent = e; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-web/src/components/ref-issue-dialog/ref-issue-dialog.tsx` around lines 65 - 74, The current loop that finds the reference parent (variables: entities, e, entityPath, issuePath, referenceParent) accidentally keeps the shorter match because it checks referenceParent.path.length > entityPath.length; change the comparison so it chooses the longest matching path (deepest ancestor) — e.g., when a candidate e matches (issuePath.startsWith(entityPath)) update referenceParent if referenceParent is null OR e.path.length > referenceParent.path.length (or compare entityPath.split('.').length > referenceParent.path.length) so the deepest/longest matching entity is selected.
🧹 Nitpick comments (2)
packages/project-builder-lib/src/references/extract-definition-refs.unit.test.ts (1)
729-733: Consider extracting parser-context setup helper in expression tests.Line 729 and similar blocks repeat the same parser-context boilerplate. A tiny local helper would reduce duplication and make each test’s intent clearer.
Also applies to: 764-768, 861-865, 946-950, 1027-1031
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-lib/src/references/extract-definition-refs.unit.test.ts` around lines 729 - 733, Several tests repeatedly construct the same parser-context boilerplate (calls to createDefinitionSchemaParserContext with new PluginSpecStore and then passing to schemaCreator); extract a small local helper function (e.g., makeParserContext or parserCtx) at the top of the test file that returns createDefinitionSchemaParserContext({ plugins: new PluginSpecStore() }) and replace each repeated block (places invoking schemaCreator(createDefinitionSchemaParserContext(...)) such as the occurrences around schemaCreator, createDefinitionSchemaParserContext, and PluginSpecStore) with a single call to that helper to remove duplication and clarify intent.packages/project-builder-lib/src/definition/project-definition-container.ts (1)
67-70: Optional: reuseentityFromIdin name helpers to remove duplicate lookups.
entityFromId,nameFromId, andsafeNameFromIdrepeat the same linear search pattern. Reusing the helper reduces drift and keeps behavior centralized.Also applies to: 80-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-lib/src/definition/project-definition-container.ts` around lines 67 - 70, The name helper methods duplicate the linear search performed in entityFromId; refactor nameFromId and safeNameFromId to call entityFromId(id) instead of repeating this.entities.find(...) so lookups are centralized and behavior stays consistent; update nameFromId to return entityFromId(id)?.name and safeNameFromId to return entityFromId(id)?.name ?? '' (or the existing fallback) while preserving their current return semantics and references to DefinitionEntity and entity.id.
🤖 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-lib/src/definition/project-definition-container.ts`:
- Around line 143-147: The constructor call creating ProjectDefinitionContainer
should receive a parserContext whose pluginStore matches the updated pluginStore
being passed into the container; currently the code passes the original context
and the new pluginStore separately, leaving this.parserContext.pluginStore out
of sync. Update the factory calls that invoke new
ProjectDefinitionContainer(...) (the ones building
parsedDefinition/context/pluginStore/schema around ProjectDefinitionContainer)
to clone or replace the incoming context so that parserContext.pluginStore is
set to the same pluginStore object you pass as the pluginStore argument (i.e.,
construct a new context or assign contextWithUpdatedPlugins = { ...context,
pluginStore } and pass that instead). Apply the same change to the other factory
invocation referenced (the second occurrence around the other constructor call)
so both places keep parserContext.pluginStore consistent with pluginStore.
In `@packages/project-builder-lib/src/web/specs/entity-type-url-web-spec.ts`:
- Around line 66-71: The register function currently overwrites existing nav
builders in the builders Map silently; update the register implementation (the
register closure that accepts DefinitionEntityType and EntityTypeNavBuilder) to
first check builders.has(entityType.name) and if true throw an Error (or
otherwise fail fast) indicating a duplicate registration for that entity type
name; this ensures duplicate registrations for
DefinitionEntityType/EntityTypeNavBuilder are detected and prevented rather than
replaced silently.
---
Outside diff comments:
In
`@packages/project-builder-web/src/components/ref-issue-dialog/ref-issue-dialog.tsx`:
- Around line 65-74: The current loop that finds the reference parent
(variables: entities, e, entityPath, issuePath, referenceParent) accidentally
keeps the shorter match because it checks referenceParent.path.length >
entityPath.length; change the comparison so it chooses the longest matching path
(deepest ancestor) — e.g., when a candidate e matches
(issuePath.startsWith(entityPath)) update referenceParent if referenceParent is
null OR e.path.length > referenceParent.path.length (or compare
entityPath.split('.').length > referenceParent.path.length) so the
deepest/longest matching entity is selected.
---
Nitpick comments:
In `@packages/project-builder-lib/src/definition/project-definition-container.ts`:
- Around line 67-70: The name helper methods duplicate the linear search
performed in entityFromId; refactor nameFromId and safeNameFromId to call
entityFromId(id) instead of repeating this.entities.find(...) so lookups are
centralized and behavior stays consistent; update nameFromId to return
entityFromId(id)?.name and safeNameFromId to return entityFromId(id)?.name ?? ''
(or the existing fallback) while preserving their current return semantics and
references to DefinitionEntity and entity.id.
In
`@packages/project-builder-lib/src/references/extract-definition-refs.unit.test.ts`:
- Around line 729-733: Several tests repeatedly construct the same
parser-context boilerplate (calls to createDefinitionSchemaParserContext with
new PluginSpecStore and then passing to schemaCreator); extract a small local
helper function (e.g., makeParserContext or parserCtx) at the top of the test
file that returns createDefinitionSchemaParserContext({ plugins: new
PluginSpecStore() }) and replace each repeated block (places invoking
schemaCreator(createDefinitionSchemaParserContext(...)) such as the occurrences
around schemaCreator, createDefinitionSchemaParserContext, and PluginSpecStore)
with a single call to that helper to remove duplication and clarify intent.
ℹ️ 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 (21)
.changeset/entity-type-nav-spec.mdpackages/project-builder-lib/src/definition/project-definition-container.test-utils.tspackages/project-builder-lib/src/definition/project-definition-container.tspackages/project-builder-lib/src/parser/parser.tspackages/project-builder-lib/src/references/deserialize-schema.tspackages/project-builder-lib/src/references/deserialize-schema.unit.test.tspackages/project-builder-lib/src/references/extract-definition-refs.unit.test.tspackages/project-builder-lib/src/references/fix-ref-deletions.tspackages/project-builder-lib/src/references/fix-ref-deletions.unit.test.tspackages/project-builder-lib/src/references/parse-schema-with-references.tspackages/project-builder-lib/src/references/serialize-schema.tspackages/project-builder-lib/src/references/serialize-schema.unit.test.tspackages/project-builder-lib/src/web/specs/entity-type-url-web-spec.tspackages/project-builder-lib/src/web/specs/index.tspackages/project-builder-web/src/app/project-definition-provider/project-definition-provider.tsxpackages/project-builder-web/src/components/ref-issue-dialog/ref-issue-dialog.tsxpackages/project-builder-web/src/core-modules/entity-type-urls-core-module.tspackages/project-builder-web/src/core-modules/index.tspackages/project-builder-web/src/routes/data/models/edit.$key/index.tsxpackages/project-builder-web/src/routes/data/models/edit.$key/service.tsxpackages/project-builder-web/src/services/entity-type.ts
💤 Files with no reviewable changes (2)
- packages/project-builder-web/src/routes/data/models/edit.$key/service.tsx
- packages/project-builder-web/src/routes/data/models/edit.$key/index.tsx
Comment on lines
143
to
+147
| return new ProjectDefinitionContainer( | ||
| parsedDefinition, | ||
| context, | ||
| pluginStore, | ||
| schema, |
There was a problem hiding this comment.
Keep parserContext.pluginStore consistent with pluginStore inside the container.
Both factories pass an updated pluginStore (Lines 147/169) but still pass the original context (Lines 145/168). This can leave this.parserContext.pluginStore out of sync with this.pluginStore.
Suggested fix
static fromDefinition(
definition: ProjectDefinition,
context: SchemaParserContext,
): ProjectDefinitionContainer {
const { definition: parsedDefinition, pluginStore } =
parseProjectDefinitionWithReferences(definition, context);
const schema = createProjectDefinitionSchema(
createDefinitionSchemaParserContext({ plugins: pluginStore }),
);
+ const parserContext: SchemaParserContext = {
+ ...context,
+ pluginStore,
+ };
return new ProjectDefinitionContainer(
parsedDefinition,
- context,
+ parserContext,
pluginStore,
schema,
);
}
@@
static fromSerializedConfig(
config: unknown,
context: SchemaParserContext,
): ProjectDefinitionContainer {
const plugins = createPluginSpecStore(context.pluginStore, config);
const schema = createProjectDefinitionSchema(
createDefinitionSchemaParserContext({ plugins }),
);
+ const parserContext: SchemaParserContext = {
+ ...context,
+ pluginStore: plugins,
+ };
return new ProjectDefinitionContainer(
deserializeSchemaWithTransformedReferences(schema, config),
- context,
+ parserContext,
plugins,
schema,
);
}Also applies to: 166-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/project-builder-lib/src/definition/project-definition-container.ts`
around lines 143 - 147, The constructor call creating ProjectDefinitionContainer
should receive a parserContext whose pluginStore matches the updated pluginStore
being passed into the container; currently the code passes the original context
and the new pluginStore separately, leaving this.parserContext.pluginStore out
of sync. Update the factory calls that invoke new
ProjectDefinitionContainer(...) (the ones building
parsedDefinition/context/pluginStore/schema around ProjectDefinitionContainer)
to clone or replace the incoming context so that parserContext.pluginStore is
set to the same pluginStore object you pass as the pluginStore argument (i.e.,
construct a new context or assign contextWithUpdatedPlugins = { ...context,
pluginStore } and pass that instead). Apply the same change to the other factory
invocation referenced (the second occurrence around the other constructor call)
so both places keep parserContext.pluginStore consistent with pluginStore.
Comment on lines
+66
to
+71
| register: <THasParent extends boolean>( | ||
| entityType: DefinitionEntityType<THasParent>, | ||
| builder: EntityTypeNavBuilder<THasParent>, | ||
| ): void => { | ||
| builders.set(entityType.name, builder as EntityTypeNavBuilder); | ||
| }, |
There was a problem hiding this comment.
Prevent silent nav builder overrides on duplicate entity type registration.
At Line 70, Map#set replaces existing builders without warning. In a plugin system, duplicate registration should fail fast to avoid route behavior changing by load order.
Suggested fix
register: <THasParent extends boolean>(
entityType: DefinitionEntityType<THasParent>,
builder: EntityTypeNavBuilder<THasParent>,
): void => {
+ if (builders.has(entityType.name)) {
+ throw new Error(
+ `Duplicate entity nav builder registration for "${entityType.name}"`,
+ );
+ }
builders.set(entityType.name, builder as EntityTypeNavBuilder);
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/project-builder-lib/src/web/specs/entity-type-url-web-spec.ts`
around lines 66 - 71, The register function currently overwrites existing nav
builders in the builders Map silently; update the register implementation (the
register closure that accepts DefinitionEntityType and EntityTypeNavBuilder) to
first check builders.has(entityType.name) and if true throw an Error (or
otherwise fail fast) indicating a duplicate registration for that entity type
name; this ensures duplicate registrations for
DefinitionEntityType/EntityTypeNavBuilder are detected and prevented rather than
replaced silently.
kingston
deleted the
kingston/eng-937-centralize-registerentitytypeurl-to-single-file
branch
Merged
This was referenced Mar 22, 2026
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
Release Notes
New Features
Refactoring