[8216] Update dropTargetsLookup in parseLegacyFormDefinition to use new DataSource type instead of legacy.#4806
Open
jvega190 wants to merge 2 commits intocraftercms:developfrom
Open
[8216] Update dropTargetsLookup in parseLegacyFormDefinition to use new DataSource type instead of legacy.#4806jvega190 wants to merge 2 commits intocraftercms:developfrom
jvega190 wants to merge 2 commits intocraftercms:developfrom
Conversation
…ew DataSource type instead of legacy
WalkthroughThe changes update data source handling across multiple components to support an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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
🧹 Nitpick comments (2)
ui/app/src/components/FormsEngine/controls/NodeSelector.tsx (1)
529-530:isEmbeddedis already defined on line 524 — reuse it.Both
pathandembeddedrecomputepickerChoice.strategy === 'embedded'inline, which is redundant.♻️ Proposed simplification
- path: pickerChoice.strategy === 'embedded' ? contextItem.path : processPath(pickerChoice.path), - embedded: pickerChoice.strategy === 'embedded' + path: isEmbedded ? contextItem.path : processPath(pickerChoice.path), + embedded: isEmbedded🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/controls/NodeSelector.tsx` around lines 529 - 530, The code redundantly recomputes pickerChoice.strategy === 'embedded' for both path and embedded; reuse the already-computed boolean isEmbedded (declared earlier on line 524) instead: set path to isEmbedded ? contextItem.path : processPath(pickerChoice.path) and set embedded to isEmbedded, leaving pickerChoice and processPath unchanged so the logic and semantics remain identical.ui/app/src/services/contentTypes.ts (1)
428-432: Stale comment and deadlegacyDatasourcemutation after thedropTargetsLookupassignment change.Since
dropTargetsLookup[datasource.id]now points todataSources[datasource.id](line 432), thelegacyDatasourcevariable serves no purpose:
- The comment on line 428 ("Also update legacyDatasource, since dropTargetsLookup references it…") is no longer true.
- Line 429 (
legacyDatasource.properties[property.name] = value;) is dead fromdropTargetsLookup's perspective, but it also silently mutatesdatasource.properties(same object reference via the shallow copy) — an unintended side-effect.legacyDatasource.typeon line 431 is always equal todatasource.type.♻️ Proposed cleanup
- const legacyDatasource = { ...datasource }; asArray(datasource.properties?.property).forEach((property) => { let value: unknown = property.value; // ... dataSources[datasource.id].properties[property.name] = value; - // Also update legacyDatasource, since dropTargetsLookup references it for 'components' type datasources. - legacyDatasource.properties[property.name] = value; }); - if (legacyDatasource.type === 'components') { + if (datasource.type === 'components') { dropTargetsLookup[datasource.id] = dataSources[datasource.id]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/services/contentTypes.ts` around lines 428 - 432, Remove the stale comment and the dead mutation of legacyDatasource.properties — do not mutate datasource.properties via legacyDatasource; instead delete the line setting legacyDatasource.properties[property.name] and any comment claiming dropTargetsLookup references legacyDatasource; ensure the conditional uses datasource.type (e.g., if (datasource.type === 'components')) and keep the assignment dropTargetsLookup[datasource.id] = dataSources[datasource.id] as-is so we no longer rely on legacyDatasource or unintentionally mutate datasource objects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/src/components/FormsEngine/controls/NodeSelector.tsx`:
- Line 533: The create-flow key derivation is inconsistent: change the
isEmbedded branch that sets key (currently using result.values.objectId) to
mirror handleOpenItem's onSave logic by deriving the key from
(values[XmlKeys.fileName] || values.objectId), stripping a trailing ".xml" via
.replace(/\.xml$/, ''), so key generation for embedded items uses the same
preference and normalization as handleOpenItem's onSave (refer to key,
isEmbedded, result.values, XmlKeys.fileName, and handleOpenItem onSave).
In `@ui/app/src/services/contentTypes.ts`:
- Around line 172-183: The call to parseComponentsDataSourceContentTypesProperty
is using a stale narrow cast and passing non-strings to split; change the first
argument to be a DataSource (remove or replace the ComponentsDatasource cast)
when calling parseComponentsDataSourceContentTypesProperty, and ensure the
property value passed (and any other place using value?.split(',')) is
coerced/checked as a string (e.g. use typeof value === 'string' ? value :
String(value) or default to '' before splitting) so split is only invoked on
strings; update references to dropTargetsLookup[itemManagerId].properties,
systemValidationsKeysMap, validations, and immutableEmptyArray accordingly.
---
Nitpick comments:
In `@ui/app/src/components/FormsEngine/controls/NodeSelector.tsx`:
- Around line 529-530: The code redundantly recomputes pickerChoice.strategy ===
'embedded' for both path and embedded; reuse the already-computed boolean
isEmbedded (declared earlier on line 524) instead: set path to isEmbedded ?
contextItem.path : processPath(pickerChoice.path) and set embedded to
isEmbedded, leaving pickerChoice and processPath unchanged so the logic and
semantics remain identical.
In `@ui/app/src/services/contentTypes.ts`:
- Around line 428-432: Remove the stale comment and the dead mutation of
legacyDatasource.properties — do not mutate datasource.properties via
legacyDatasource; instead delete the line setting
legacyDatasource.properties[property.name] and any comment claiming
dropTargetsLookup references legacyDatasource; ensure the conditional uses
datasource.type (e.g., if (datasource.type === 'components')) and keep the
assignment dropTargetsLookup[datasource.id] = dataSources[datasource.id] as-is
so we no longer rely on legacyDatasource or unintentionally mutate datasource
objects.
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.
craftercms/craftercms#8216
Summary by CodeRabbit