Fix the issue where multiple identical breaks UI#233
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds an opt-in mechanism during schema dereferencing to remove duplicated "$id" fields (which can cause the UI to fail) when dereferencing params and ui schemas, while keeping "$id" for artifacts and connections.
Changes:
- Add
StripIDtodefinition.DereferenceOptionsand conditionally delete"$id"when dereferencing Massdriver remote refs. - Update bundle schema dereferencing to pass
StripID=truefor Params/UI andfalsefor Artifacts/Connections.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/definition/dereference.go | Adds StripID option and strips "$id" from Massdriver-referenced schemas when enabled. |
| internal/bundle/dereference.go | Passes StripID per schema type (Params/UI vs Artifacts/Connections). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if task.schema == nil { | ||
| *task.schema = map[string]any{ | ||
| "properties": make(map[string]any), | ||
| } | ||
| } |
There was a problem hiding this comment.
The nil check here is ineffective: task.schema is a pointer created via &b.Params/etc and will never be nil. If the intent is to initialize missing schemas, this should check *task.schema == nil (or len(*task.schema)==0 depending on desired semantics) before assigning the default map; otherwise a nil map schema will flow into DereferenceSchema and later fail the map type assertion.
| // This is a hack to get around the issue of the UI choking if the params schema has 2 or more of the same $id in it. | ||
| // We need the "$id" in artifacts and connections, but we need to strip it out of params and ui schemas, hence the conditional. | ||
| // This logic should be removed when we have a better solution for this in the UI/API - probably after resource types are in OCI | ||
| if opts.StripID { | ||
| delete(referencedSchema, "$id") | ||
| } |
There was a problem hiding this comment.
This introduces new behavior (conditionally removing "$id" from dereferenced Massdriver schemas) but there is no test coverage validating that StripID=true actually strips "$id" and StripID=false preserves it. Consider adding/adjusting a unit test in internal/definition/dereference_test.go that mocks a remote schema containing a "$id" and asserts the output based on the StripID option.
| Client *client.Client | ||
| Cwd string |
There was a problem hiding this comment.
StripID reads like it will remove "$id" everywhere, but the current implementation only deletes it for Massdriver remote refs (in dereferenceMassdriverRef). To avoid misuse/confusion for future callers, consider renaming the option (or documenting it in the field comment) to make the scope explicit (e.g., only applies to Massdriver-referenced schemas).
| Client *client.Client | |
| Cwd string | |
| Client *client.Client | |
| Cwd string | |
| // StripID removes the top-level "$id" only from schemas loaded via Massdriver remote refs | |
| // in dereferenceMassdriverRef. It does not strip "$id" from all schemas encountered during dereferencing. |
If a params schema has multiple
$idfields that are identical, it breaks the UI (due to a validation library we use??). We don't need$idin params, but we need it inartifactsandconnections. So this change will conditionally strip any$idfields that are pulled in via$refstatements in the params and UI schemas.