feat(cascadepolicy): disable inputs if cascade policy exists #4175
Conversation
|
""" WalkthroughThe changes simplify the cascade policy flag handling by renaming and consolidating the existing AI extraction cascade policy flag to a general cascade policy flag. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MetadataInstanceEditor
participant CascadePolicy
User->>MetadataInstanceEditor: Open metadata instance editor
MetadataInstanceEditor->>CascadePolicy: Render with isExistingCascadePolicy flag
CascadePolicy->>CascadePolicy: Check isExistingCascadePolicy
alt Cascade Policy Exists
CascadePolicy->>User: Show InlineNotice message
CascadePolicy->>User: Disable cascade policy controls (RadioButtons, Toggle, AgentSelector)
else No Existing Policy
CascadePolicy->>User: Show enabled cascade policy controls
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (6)
🧰 Additional context used🧠 Learnings (2)📓 Common learningssrc/features/metadata-instance-editor/CascadePolicy.js (6)🧬 Code Graph Analysis (1)src/features/metadata-instance-editor/CascadePolicy.js (1)
🪛 Biome (1.9.4)src/features/metadata-instance-editor/CascadePolicy.js[error] 61-61: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax. TypeScript only syntax (parse) ⏰ 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 comments (6)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/features/metadata-instance-editor/messages.js (1)
210-215: Align tooltip punctuation and wording with surrounding messages.Nearly all
defaultMessagestrings in this file terminate with a period and avoid repetitive phrasing. Adding the final period and replacing the second “policy” with “it” tightens the copy and keeps style consistent.- 'Cascade policy cannot be edited. To change the policy, remove the current policy to set up a new one' + 'Cascade policy cannot be edited. To change it, remove the current policy and set up a new one.'i18n/en-US.properties (1)
1228-1228: Consider slight wording refinement for clarityRewrite the second sentence as “… and set up a new one” to make the sequence of actions clearer and avoid repeating “policy.”
-boxui.metadataInstanceEditor.cascadePolicyOptionsDisabledTooltip = Cascade policy cannot be edited. To change the policy, remove the current policy to set up a new one +boxui.metadataInstanceEditor.cascadePolicyOptionsDisabledTooltip = Cascade policy cannot be edited. To change the policy, remove the current policy and set up a new one
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/features/metadata-instance-editor/__tests__/__snapshots__/CascadePolicy.test.js.snapis excluded by!**/*.snapsrc/features/metadata-instance-editor/__tests__/__snapshots__/Instance.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
i18n/en-US.properties(1 hunks)src/features/metadata-instance-editor/CascadePolicy.js(7 hunks)src/features/metadata-instance-editor/Instance.js(2 hunks)src/features/metadata-instance-editor/__tests__/CascadePolicy.test.js(0 hunks)src/features/metadata-instance-editor/__tests__/Instance.test.js(2 hunks)src/features/metadata-instance-editor/messages.js(1 hunks)src/features/metadata-instance-editor/stories/tests/CascadePolicy-visual.stories.js(1 hunks)
💤 Files with no reviewable changes (1)
- src/features/metadata-instance-editor/tests/CascadePolicy.test.js
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: ahorowitz123
PR: box/box-ui-elements#4102
File: src/features/metadata-instance-editor/Instance.js:647-649
Timestamp: 2025-05-14T17:46:25.370Z
Learning: In the metadata-instance-editor component, `isExistingAIExtractionCascadePolicy` specifically checks if the cascade policy fetched from the backend has AI folder extraction enabled, using props rather than state to reflect the original server-side configuration rather than current UI state.
src/features/metadata-instance-editor/messages.js (1)
Learnt from: ahorowitz123
PR: box/box-ui-elements#4102
File: src/features/metadata-instance-editor/Instance.js:647-649
Timestamp: 2025-05-14T17:46:25.370Z
Learning: In the metadata-instance-editor component, `isExistingAIExtractionCascadePolicy` specifically checks if the cascade policy fetched from the backend has AI folder extraction enabled, using props rather than state to reflect the original server-side configuration rather than current UI state.
i18n/en-US.properties (1)
undefined
<retrieved_learning>
Learnt from: ahorowitz123
PR: #4102
File: src/features/metadata-instance-editor/Instance.js:647-649
Timestamp: 2025-05-14T17:46:25.370Z
Learning: In the metadata-instance-editor component, isExistingAIExtractionCascadePolicy specifically checks if the cascade policy fetched from the backend has AI folder extraction enabled, using props rather than state to reflect the original server-side configuration rather than current UI state.
</retrieved_learning>
src/features/metadata-instance-editor/Instance.js (1)
Learnt from: ahorowitz123
PR: box/box-ui-elements#4102
File: src/features/metadata-instance-editor/Instance.js:647-649
Timestamp: 2025-05-14T17:46:25.370Z
Learning: In the metadata-instance-editor component, `isExistingAIExtractionCascadePolicy` specifically checks if the cascade policy fetched from the backend has AI folder extraction enabled, using props rather than state to reflect the original server-side configuration rather than current UI state.
src/features/metadata-instance-editor/stories/tests/CascadePolicy-visual.stories.js (1)
Learnt from: ahorowitz123
PR: box/box-ui-elements#4102
File: src/features/metadata-instance-editor/Instance.js:647-649
Timestamp: 2025-05-14T17:46:25.370Z
Learning: In the metadata-instance-editor component, `isExistingAIExtractionCascadePolicy` specifically checks if the cascade policy fetched from the backend has AI folder extraction enabled, using props rather than state to reflect the original server-side configuration rather than current UI state.
src/features/metadata-instance-editor/__tests__/Instance.test.js (1)
Learnt from: ahorowitz123
PR: box/box-ui-elements#4102
File: src/features/metadata-instance-editor/Instance.js:647-649
Timestamp: 2025-05-14T17:46:25.370Z
Learning: In the metadata-instance-editor component, `isExistingAIExtractionCascadePolicy` specifically checks if the cascade policy fetched from the backend has AI folder extraction enabled, using props rather than state to reflect the original server-side configuration rather than current UI state.
src/features/metadata-instance-editor/CascadePolicy.js (4)
Learnt from: ahorowitz123
PR: box/box-ui-elements#4102
File: src/features/metadata-instance-editor/Instance.js:647-649
Timestamp: 2025-05-14T17:46:25.370Z
Learning: In the metadata-instance-editor component, `isExistingAIExtractionCascadePolicy` specifically checks if the cascade policy fetched from the backend has AI folder extraction enabled, using props rather than state to reflect the original server-side configuration rather than current UI state.
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4160
File: src/elements/content-sidebar/SidebarToggle.js:21-27
Timestamp: 2025-06-25T13:09:54.538Z
Learning: The box-ui-elements project uses Flow for type annotations in JavaScript files, as indicated by @flow directives in file headers. Type annotations like `: Props` are valid Flow syntax, not TypeScript syntax.
Learnt from: jpan-box
PR: box/box-ui-elements#4166
File: src/elements/content-sidebar/SidebarNav.js:126-126
Timestamp: 2025-07-11T14:43:02.641Z
Learning: In the box-ui-elements repository, there's a file-type-based pattern for internationalization: TypeScript files (.tsx) predominantly use the modern useIntl hook (41 vs 15 files), while JavaScript files (.js) predominantly use the legacy injectIntl HOC (64 vs 5 files). New TypeScript components should use useIntl, while existing JavaScript components typically continue using injectIntl for consistency.
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4144
File: src/elements/content-sidebar/versions/VersionsList.js:24-33
Timestamp: 2025-06-17T13:30:02.172Z
Learning: In the box-ui-elements codebase, Flow components use .flow.js type definition files, not TypeScript .ts files. The InternalSidebarNavigation type is a union type where different variants may have different properties like versionId, and proper type safety is ensured through conditional checks in methods like getSelectedVersionId.
🪛 Biome (1.9.4)
src/features/metadata-instance-editor/CascadePolicy.js
[error] 52-55: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ 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: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (8)
src/features/metadata-instance-editor/Instance.js (2)
644-644: LGTM! Clean simplification of cascade policy logic.The simplified logic correctly determines if a cascade policy exists by checking if cascading is enabled through props. This is cleaner than the previous approach and aligns well with the PR objective of disabling options when any cascade policy exists.
683-683: Prop renaming is consistent and clear.The prop name
isExistingCascadePolicyis more descriptive and aligns with the simplified logic in the component.src/features/metadata-instance-editor/stories/tests/CascadePolicy-visual.stories.js (1)
1-61: Excellent visual test coverage for the tooltip functionality.The Storybook stories provide comprehensive coverage of the enabled/disabled states and tooltip behavior:
- Proper use of
TooltipProviderwrapper- Clear test scenarios with descriptive names
- Appropriate use of
userEvent.hover()for testing tooltip visibility- Good reliance on Chromatic snapshots for visual verification
The stories effectively demonstrate the new UI behavior introduced by the cascade policy changes.
src/features/metadata-instance-editor/CascadePolicy.js (3)
48-71: Well-designed conditional tooltip wrapper component.The
MetadataEditorWrappercomponent provides a clean abstraction for conditionally wrapping children with a tooltip when cascade policy options are disabled. The implementation:
- Properly handles the conditional rendering logic
- Spreads additional props correctly with
...rest- Maintains clean separation of concerns
- Uses the appropriate Blueprint Web Tooltip component
This is a good pattern for reusable conditional tooltip behavior.
142-147: Comprehensive disabled state implementation.All interactive elements are properly disabled when
isExistingCascadePolicyis true:
- RadioButton components for cascade mode selection
- Toggle for AI folder extraction
- BoxAiAgentSelector component
This ensures users cannot modify cascade policy settings when a policy already exists, which aligns perfectly with the PR objective.
Also applies to: 167-167, 183-183
52-55: Ignore the static analysis warning - Flow syntax is correct.The static analysis warning about TypeScript-only syntax is a false positive. Based on the retrieved learnings, this codebase uses Flow for type annotations in JavaScript files, and the syntax
{ children: React.Node, isExistingCascadePolicy?: boolean }is valid Flow syntax.The warning can be safely ignored as Flow type annotations are the standard pattern in this codebase.
src/features/metadata-instance-editor/__tests__/Instance.test.js (2)
837-847: Improved test reliability with explicit cascade policy setup.The tests now explicitly enable the cascade policy before testing AI extraction functionality:
- Uses simplified
cascadePolicy: { canEdit: true }prop- Explicitly clicks the cascade policy toggle before testing AI extraction
- Makes the test flow more predictable and reliable
This is a good improvement that makes the test setup more explicit and easier to understand.
Also applies to: 896-904
874-891: Comprehensive test coverage for disabled state.The consolidated test effectively verifies that all cascade policy options are properly disabled when a cascade policy exists:
- Checks both skip and overwrite radio buttons are disabled
- Verifies the AI toggle is disabled
- Tests the core functionality introduced by this PR
The test structure is clean and provides good coverage of the new behavior.
|
@CodeRabbit can you update the PR description with the changes |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll help you update the PR description with the changes. Let me first get the current PR description and then create an updated version that better reflects the actual changes made. 🏁 Script executed: Length of output: 2873 Now I'll create an updated PR description that provides more comprehensive information about the changes made: 🏁 Script executed: Length of output: 4898 @ahorowitz123 I've successfully updated the PR description with comprehensive information about the changes made. The new description includes: What was added:Overview section - Explains the purpose and user experience enhancement Detailed changes breakdown organized into:
Files modified section - Lists all the key files that were changed User impact section - Explains the benefits to end users The updated description provides a much clearer picture of what this PR accomplishes, making it easier for reviewers and future developers to understand the scope and impact of the changes. It maintains the original merge instructions and CodeRabbit summary while providing more comprehensive context about the cascade policy disabling functionality. 🧠 Learnings used |
src/features/metadata-instance-editor/__tests__/CascadePolicy.test.js
Outdated
Show resolved
Hide resolved
c5f2ccf to
afec233
Compare
Overview
This PR enhances the cascade policy user experience by preventing modifications to cascade policy options when a cascade policy already exists. Instead of allowing potentially confusing edits, the interface now clearly communicates why options are disabled and how users can modify them.
Changes Made
Core Functionality
isExistingAIExtractionCascadePolicytoisExistingCascadePolicyto broaden the scope from AI-specific to general cascade policy detectionUser Experience Improvements
InlineNoticecomponent that appears when cascade policy options are disabledTechnical Implementation
CascadePolicycomponent to handle the new disabled state and display the noticeFiles Modified
src/features/metadata-instance-editor/CascadePolicy.js- Main component logic and UI updatessrc/features/metadata-instance-editor/Instance.js- Simplified cascade policy flag logicsrc/features/metadata-instance-editor/messages.js- Added new localization messagesi18n/en-US.properties- Added new localization stringsUser Impact
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation