fix regression due to prefix now in the keys#1553
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ 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). (2)
🧰 Additional context used📓 Path-based instructions (2)packages/app-builder/src/**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
packages/**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (2)📚 Learning: 2026-05-11T13:00:53.337ZApplied to files:
📚 Learning: 2026-05-12T19:51:39.619ZApplied to files:
🔇 Additional comments (1)
📝 WalkthroughWalkthroughIntroduce canonical dataset/topic keys and selection helpers; update UI and exports to use them; use canonical keys in FieldDataset; add DTO section→category mapping; and refactor lists-config normalization to group by section and stop mutating item names. ChangesSelection canonicalization & UI integration
🎯 4 (Complex) | ⏱️ ~45 minutes Sequence Diagram(s) sequenceDiagram
participant UI as DatasetSelectionContent
participant Utils as dataset-selection-provider-utils
participant Consumer as FieldDataset
participant Lists as lists-config
participant Models as screening-config
UI->>Utils: isDatasetKeySelected / setDatasetKey / isTopicKeySelected / setTopicKey
UI->>Utils: clearSectionSelections(sectionKey)
Consumer->>Utils: getCanonicalSelectedKeys(datasets)
Lists->>Utils: buildDatasetKey(section, dataset)
Models->>Lists: DtoSectionToCategory mapping used in getDatasetFromFilters
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/app-builder/src/components/ListAndTopicConfiguration/DatasetSelectionContent.tsx (2)
898-925:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInconsistent selection check — still uses direct key lookup instead of
isTopicKeySelected.At line 900,
isSelectedis computed as!!datasets[item.name], but the rest of this refactor usesisTopicKeySelected(datasets, sectionKey, topicGroup, item.name)for topic items (see lines 693 and 859). This mismatch will cause incorrect checkbox state when composite keys are in use.To select, or not to select, that is the question—
Whether 'tis nobler in the code to suffer
The slings and arrows of inconsistent keys...🐛 Proposed fix
{items.map((item) => { - const isSelected = !!datasets[item.name]; + const isSelected = isTopicKeySelected(datasets, sectionKey, topicGroup, item.name); const itemName = item.title ?? formatItemName(item.name); return (🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app-builder/src/components/ListAndTopicConfiguration/DatasetSelectionContent.tsx` around lines 898 - 925, The checkbox selection logic uses a direct key lookup (!!datasets[item.name]) which is inconsistent with the rest of the refactor; replace this with a call to isTopicKeySelected(datasets, sectionKey, topicGroup, item.name) so the selected state respects composite/topic keys used elsewhere (update the isSelected assignment used by Checkbox and any related UI state for the label/row), ensuring handleClickItem, Checkbox id, and disabled logic remain unchanged.
941-957:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame inconsistency as above — direct key lookup instead of
isTopicKeySelected.Line 943 also uses
!!datasets[item.name]for the non-popover variant of the menu. Apply the same fix for consistency with the composite-key model.🐛 Proposed fix
{items.map((item) => { - const isSelected = !!datasets[item.name]; + const isSelected = isTopicKeySelected(datasets, sectionKey, topicGroup, item.name); const itemName = item.title ?? formatItemName(item.name); return (🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app-builder/src/components/ListAndTopicConfiguration/DatasetSelectionContent.tsx` around lines 941 - 957, The selected-state check currently uses a direct key lookup (!!datasets[item.name]) inside the items.map render, which is inconsistent with the composite-key model; replace that lookup with the existing helper isTopicKeySelected by changing the isSelected assignment to call isTopicKeySelected(item) (use the same helper used elsewhere), so MenuCommand.Item uses isTopicKeySelected(item) for selected state and retains handleClickItem, value={item.name}, disabled={mode === 'view'} and the rest of the rendering unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/app-builder/src/components/ListAndTopicConfiguration/dataset-selection-provider-utils.ts`:
- Around line 102-108: clearSectionSelections currently only clears keys
matching sectionKey and keys prefixed with `${sectionKey}:`, leaving DTO alias
keys like "other" and "adverse_media" (which map to sections such as
"third-parties" and "adverse-media") untouched; update clearSectionSelections to
also clear those legacy alias keys when sectionKey corresponds to those sections
by checking and setting datasets['other'] and datasets['adverse_media'] (or
whichever DTO alias names are used) to false whenever sectionKey ===
'third-parties' or sectionKey === 'adverse-media' (use the function
clearSectionSelections, datasets and sectionKey to locate the change).
---
Outside diff comments:
In
`@packages/app-builder/src/components/ListAndTopicConfiguration/DatasetSelectionContent.tsx`:
- Around line 898-925: The checkbox selection logic uses a direct key lookup
(!!datasets[item.name]) which is inconsistent with the rest of the refactor;
replace this with a call to isTopicKeySelected(datasets, sectionKey, topicGroup,
item.name) so the selected state respects composite/topic keys used elsewhere
(update the isSelected assignment used by Checkbox and any related UI state for
the label/row), ensuring handleClickItem, Checkbox id, and disabled logic remain
unchanged.
- Around line 941-957: The selected-state check currently uses a direct key
lookup (!!datasets[item.name]) inside the items.map render, which is
inconsistent with the composite-key model; replace that lookup with the existing
helper isTopicKeySelected by changing the isSelected assignment to call
isTopicKeySelected(item) (use the same helper used elsewhere), so
MenuCommand.Item uses isTopicKeySelected(item) for selected state and retains
handleClickItem, value={item.name}, disabled={mode === 'view'} and the rest of
the rendering unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b1df66b-e1f8-44c2-9440-82caef5f7d4b
📒 Files selected for processing (6)
packages/app-builder/src/components/ListAndTopicConfiguration/DatasetSelectionContent.tsxpackages/app-builder/src/components/ListAndTopicConfiguration/dataset-selection-provider-utils.tspackages/app-builder/src/components/ListAndTopicConfiguration/index.tspackages/app-builder/src/components/Scenario/Screening/FieldDataset.tsxpackages/app-builder/src/models/screening-config.tspackages/app-builder/src/queries/screening/lists-config.ts
📜 Review details
⏰ 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). (2)
- GitHub Check: check / main
- GitHub Check: e2e
🧰 Additional context used
📓 Path-based instructions (2)
packages/app-builder/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
packages/app-builder/src/**/*.{ts,tsx}: Use internal imports from@app-buildernamespace for models, queries, components, and utilities
Use ui-design-system package for UI components (Button, Modal, Select) and utility functions (cn)
Use TanStack Query hooks with naming convention useGetXyzQuery for data fetching operations
Use ts-pattern for pattern matching with the match function instead of conditional logic
Use TanStack Form for form handling instead of manual form state management
Files:
packages/app-builder/src/components/ListAndTopicConfiguration/index.tspackages/app-builder/src/components/Scenario/Screening/FieldDataset.tsxpackages/app-builder/src/models/screening-config.tspackages/app-builder/src/components/ListAndTopicConfiguration/dataset-selection-provider-utils.tspackages/app-builder/src/queries/screening/lists-config.tspackages/app-builder/src/components/ListAndTopicConfiguration/DatasetSelectionContent.tsx
packages/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Tailwind CSS 4 with the tailwind-preset package for consistent styling across packages
Files:
packages/app-builder/src/components/ListAndTopicConfiguration/index.tspackages/app-builder/src/components/Scenario/Screening/FieldDataset.tsxpackages/app-builder/src/models/screening-config.tspackages/app-builder/src/components/ListAndTopicConfiguration/dataset-selection-provider-utils.tspackages/app-builder/src/queries/screening/lists-config.tspackages/app-builder/src/components/ListAndTopicConfiguration/DatasetSelectionContent.tsx
🧠 Learnings (3)
📚 Learning: 2026-05-11T13:00:53.337Z
Learnt from: william-schlegel
Repo: checkmarble/marble-frontend PR: 1503
File: packages/app-builder/src/components/ContinuousScreening/context/ListAndTopicDatasetConfigurationBridge.tsx:13-20
Timestamp: 2026-05-11T13:00:53.337Z
Learning: In checkmarble/marble-frontend, calls to `createSharp` from the `sharpstate` library should be treated as if they were a React hook. In React `.tsx` components, call `createSharp` unconditionally at the top level of the component function body (not inside conditionals or nested functions). Do not place `createSharp` inside `useMemo`, `useCallback`, `useEffect`, or any other hook, and do not suggest wrapping it in `useMemo`—that is incorrect and should be flagged during review.
Applied to files:
packages/app-builder/src/components/Scenario/Screening/FieldDataset.tsxpackages/app-builder/src/components/ListAndTopicConfiguration/DatasetSelectionContent.tsx
📚 Learning: 2026-05-12T19:51:39.619Z
Learnt from: Pascal-Delange
Repo: checkmarble/marble-frontend PR: 1522
File: packages/app-builder/src/components/Cases/CaseAlerts.tsx:449-449
Timestamp: 2026-05-12T19:51:39.619Z
Learning: In React (.tsx) files, when rendering translated strings that include dynamic count values, always use i18n interpolation rather than appending the count as a separate raw React text node. Prefer `t('translation.key', { count })` (or the project’s equivalent) and include `{{count}}` (or the interpolation placeholder expected by the i18n setup) inside the translation string so each locale controls placement/order. Avoid patterns like `t('key') + ' (' + count + ')'` or rendering `t('key')` followed by `(${count})` as separate nodes, since this can break RTL layout (e.g., Arabic).
Applied to files:
packages/app-builder/src/components/Scenario/Screening/FieldDataset.tsxpackages/app-builder/src/components/ListAndTopicConfiguration/DatasetSelectionContent.tsx
📚 Learning: 2026-05-11T13:53:33.690Z
Learnt from: william-schlegel
Repo: checkmarble/marble-frontend PR: 1503
File: packages/app-builder/src/components/ListAndTopicConfiguration/DatasetSelectionContent.tsx:55-58
Timestamp: 2026-05-11T13:53:33.690Z
Learning: In the ListAndTopicConfiguration component system (e.g., DatasetSelectionContent.tsx), when deciding whether to include/filter server-provided “section” data, you can treat `conditionalTopics` as redundant for non-emptiness: by server contract, any section with `conditionalTopics` will also have `topics`. Therefore prefer the predicate `section.datasets?.length || section.topics` and do not add an extra `|| section.conditionalTopics` check, unless the backend schema/guarantee changes (or the frontend types allow `conditionalTopics` without `topics`).
Applied to files:
packages/app-builder/src/components/ListAndTopicConfiguration/DatasetSelectionContent.tsx
🔇 Additional comments (19)
packages/app-builder/src/components/ListAndTopicConfiguration/DatasetSelectionContent.tsx (10)
25-31: LGTM!
260-267: LGTM!
412-424: LGTM!
476-492: LGTM!
598-612: LGTM!
615-673: LGTM!
675-693: LGTM!
775-789: LGTM!
813-840: LGTM!
857-884: LGTM!packages/app-builder/src/components/Scenario/Screening/FieldDataset.tsx (1)
4-4: LGTM!Also applies to: 12-14
packages/app-builder/src/models/screening-config.ts (2)
93-98: LGTM!
136-147: LGTM!packages/app-builder/src/queries/screening/lists-config.ts (4)
1-1: LGTM!
27-38: LGTM!
40-48: LGTM!
71-77: LGTM!packages/app-builder/src/components/ListAndTopicConfiguration/dataset-selection-provider-utils.ts (1)
8-100: LGTM!packages/app-builder/src/components/ListAndTopicConfiguration/index.ts (1)
7-17: LGTM!
2 participants
Summary by CodeRabbit