feat(metadata-instance-editor): add enhanced ai extract support #4197
Conversation
|
|
|
""" WalkthroughThis change introduces dynamic AI agent selection for metadata cascade policies in the metadata instance editor. It adds new types, constants, and internationalized messages, updates the cascade policy and instance editor components to support agent selection with callbacks, and enhances tests to verify the new selection logic. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CascadePolicy
participant BoxAiAgentSelectorWithApiContainer
participant Instance
User->>CascadePolicy: Opens AI agent selector
CascadePolicy->>BoxAiAgentSelectorWithApiContainer: Calls agentFetcher()
BoxAiAgentSelectorWithApiContainer-->>CascadePolicy: Returns agent list (promise)
User->>BoxAiAgentSelectorWithApiContainer: Selects agent
BoxAiAgentSelectorWithApiContainer->>CascadePolicy: handleAgentSelect(agent)
CascadePolicy->>Instance: onAIAgentSelect(agent)
Instance->>Instance: Updates cascadePolicyConfiguration state
User->>Instance: Clicks Save
Instance->>Instance: Includes cascadePolicyConfiguration in onSave
Estimated code review effort🎯 2 (Simple) | ⏱️ ~18 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (4)📓 Common learningssrc/features/metadata-instance-editor/constants.js (1)Learnt from: ahorowitz123 src/features/metadata-instance-editor/CascadePolicy.js (1)Learnt from: ahorowitz123 src/features/metadata-instance-editor/Instance.js (1)Learnt from: ahorowitz123 🧬 Code Graph Analysis (1)src/features/metadata-instance-editor/Instance.js (1)
🪛 Biome (2.1.2)src/features/metadata-instance-editor/CascadePolicy.js[error] 10-10: 'import { type x ident }' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax. TypeScript only syntax (parse) [error] 90-90: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax. TypeScript only syntax (parse) src/features/metadata-instance-editor/Instance.js[error] 9-9: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax. TypeScript only syntax (parse) [error] 360-360: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax. TypeScript only syntax (parse) [error] 360-360: return type annotation 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 (11)
✨ Finishing Touches
🧪 Generate unit tests
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/common/types/metadata.js (1)
96-105: LGTM! Consider enhancing type safety for the agent property.The new
MetadataCascadePolicyConfigurationtype and its integration withMetadataCascadingPolicyDataare well-structured and maintain backward compatibility with the optional property.Consider using a union type for the
agentproperty to improve type safety:type MetadataCascadePolicyConfiguration = { - agent: string, + agent: 'enhanced_extract_agent' | 'standard_agent', };This would align with the constants defined in the codebase and provide better compile-time checking.
src/features/metadata-instance-editor/Instance.js (1)
350-365: Consider making agent ID comparison more maintainable.The hardcoded agent ID
'2'for the enhanced agent creates a tight coupling between the component logic and the agent data structure. Consider using a constant or deriving this from the agent configuration.Consider extracting the enhanced agent ID to a constant:
+const ENHANCED_AGENT_ID = '2'; + /** * Handles the selection of an AI agent * @param {AgentType | null} agent - The selected agent */ onAIAgentSelect = (agent: AgentType | null): void => { - // '2' is the id for the enhanced agent - if (agent && agent.id === '2') { + // Enhanced agent selection + if (agent && agent.id === ENHANCED_AGENT_ID) { this.setState({ cascadePolicyConfiguration: { agent: ENHANCED_AGENT_CONFIGURATION, }, }); } else { this.setState({ cascadePolicyConfiguration: null }); } };
📜 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__/Instance.test.js.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
package.json(1 hunks)src/common/types/metadata.js(1 hunks)src/features/metadata-instance-editor/CascadePolicy.js(5 hunks)src/features/metadata-instance-editor/Instance.js(8 hunks)src/features/metadata-instance-editor/__tests__/CascadePolicy.test.js(1 hunks)src/features/metadata-instance-editor/constants.js(1 hunks)src/features/metadata-instance-editor/messages.js(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 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.
package.json (1)
Learnt from: rafalmaksymiuk
PR: #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.
src/features/metadata-instance-editor/constants.js (1)
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.
src/common/types/metadata.js (1)
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.
src/features/metadata-instance-editor/messages.js (1)
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.
src/features/metadata-instance-editor/CascadePolicy.js (6)
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.
Learnt from: rafalmaksymiuk
PR: #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: #4166
File: src/elements/content-sidebar/SidebarNav.js:126-126
Timestamp: 2025-07-11T14:43:02.677Z
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: #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.
Learnt from: rafalmaksymiuk
PR: #4160
File: src/elements/content-sidebar/SidebarToggle.js:13-19
Timestamp: 2025-06-25T13:09:45.168Z
Learning: Files with @flow or @flow strict comments at the top use Flow type syntax, not TypeScript. Flow type definitions like type Props = { ... } and type imports like type { RouterHistory } are valid Flow syntax and should not be flagged as TypeScript-only features.
Learnt from: rafalmaksymiuk
PR: #4160
File: src/elements/content-sidebar/SidebarToggle.js:11-11
Timestamp: 2025-06-25T13:10:19.128Z
Learning: In the box-ui-elements codebase, Flow files support import type syntax for type-only imports. The import type statement is valid Flow syntax, not just TypeScript, and should not be flagged as an error when used in Flow files (indicated by @flow comments).
src/features/metadata-instance-editor/Instance.js (1)
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.
src/features/metadata-instance-editor/__tests__/CascadePolicy.test.js (1)
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.
🧬 Code Graph Analysis (1)
src/features/metadata-instance-editor/Instance.js (1)
src/features/metadata-instance-editor/constants.js (1)
ENHANCED_AGENT_CONFIGURATION(5-5)
🪛 Biome (1.9.4)
src/features/metadata-instance-editor/CascadePolicy.js
[error] 6-6: 'import { type x ident }' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 80-80: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 85-85: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
src/features/metadata-instance-editor/Instance.js
[error] 354-354: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 354-354: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 354-354: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
⏰ 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 (24)
src/features/metadata-instance-editor/constants.js (1)
5-7: LGTM! Clean constant addition.The new
ENHANCED_AGENT_CONFIGURATIONconstant follows proper naming conventions and is correctly exported. The string value aligns with the AI agent selection feature being introduced.src/features/metadata-instance-editor/messages.js (1)
238-247: Excellent internationalization implementation!The new AI agent name messages follow all established patterns:
- Descriptive default messages and context descriptions
- Consistent ID naming convention (
boxui.metadataInstanceEditor.*)- Proper structure for internationalization support
This enables proper localization of the AI agent selection feature across different languages.
src/features/metadata-instance-editor/CascadePolicy.js (12)
3-8: LGTM! Clean import additions for enhanced functionality.The new imports appropriately support the internationalization and agent selection features. The use of
useCallbackanduseIntlaligns with modern React patterns, and the type import forAgentTypeensures proper type safety.
32-32: Good addition of optional callback prop.The optional
onAIAgentSelectprop with proper Flow typing allows parent components to handle agent selection events while maintaining backward compatibility.
60-75: Excellent internationalization implementation.The memoized agents array properly uses
useIntlto provide localized agent names, improving the user experience across different languages. The dependency array correctly includesformatMessageto ensure proper re-rendering when locale changes.
77-82: Smart adapter pattern for API compatibility.The
agentFetchercallback elegantly adapts the local agents data to satisfy theBoxAiAgentSelectorWithApiContainer's promise-based interface requirement. The comment clearly explains the design decision.
84-91: Proper callback handling with defensive programming.The
handleAgentSelectcallback correctly checks for the existence ofonAIAgentSelectbefore invoking it, preventing runtime errors. The use ofuseCallbackwith appropriate dependencies optimizes performance.
174-182: Well-integrated conditional rendering.The AI agent selector is appropriately rendered only when both the agent selector capability and AI folder extraction are enabled, ensuring the UI remains clean and contextual.
3-8: LGTM: Clean import additions for enhanced AI agent functionality.The new imports properly support the enhanced AI agent selection feature with internationalization and the updated selector component.
32-32: LGTM: Well-defined optional callback prop.The
onAIAgentSelectprop is properly typed as optional with the correct Flow syntax for handling agent selection events.
58-75: LGTM: Good use of internationalization and memoization.The agents array is properly memoized with internationalized names, including the enhanced agent with its custom icon. The dependency array correctly includes
formatMessageto ensure updates when locale changes.
77-82: LGTM: Clean adapter pattern for API compatibility.The
agentFetchercallback elegantly adapts the static agents data to the Promise-based interface expected byBoxAiAgentSelectorWithApiContainer. The implementation is efficient and properly memoized.
84-91: LGTM: Proper callback handling with safety check.The
handleAgentSelectcallback correctly checks for the existence ofonAIAgentSelectbefore invoking it, preventing potential runtime errors and following defensive programming practices.
174-182: LGTM: Correct component integration.The updated
BoxAiAgentSelectorWithApiContainerusage properly passes the fetcher and selection handler callbacks. The conditional rendering ensures the selector only appears when both agent selection is enabled and AI folder extraction is active.src/features/metadata-instance-editor/Instance.js (10)
27-27: Good import of required constants and types.The import properly brings in the necessary
ENHANCED_AGENT_CONFIGURATIONconstant and ensures type safety with the metadata types.
41-41: Appropriate type addition for cascade policy configuration.The
MetadataCascadePolicyConfigurationtype properly extends the state to support AI agent configuration tracking.
222-246: Proper integration of cascade policy configuration.The
onSavemethod correctly includes thecascadePolicyConfigurationin the cascade policy data, ensuring the selected AI agent configuration is persisted with the metadata changes.
710-710: Clean prop passing for agent selection callback.The
onAIAgentSelectprop is properly passed to theCascadePolicycomponent, completing the integration between parent and child components.
27-27: LGTM: Proper constant import for agent configuration.The import of
ENHANCED_AGENT_CONFIGURATIONconstant provides the necessary configuration value for enhanced agent selection.
41-41: LGTM: Correct type import for cascade policy configuration.The
MetadataCascadePolicyConfigurationtype import supports the new state property for tracking AI agent configuration.
76-76: LGTM: Proper state type extension.The addition of
cascadePolicyConfigurationto the State type correctly allows null values and uses the imported configuration type.
222-222: LGTM: Consistent state destructuring.The destructuring properly includes the new
cascadePolicyConfigurationstate property for use in the onSave method.
246-246: LGTM: Proper integration with save callback.The
cascadePolicyConfigurationis correctly passed to the cascading policy data in the onSave callback, ensuring the AI agent configuration is persisted.
710-710: LGTM: Proper callback prop passing.The
onAIAgentSelectmethod is correctly passed to the CascadePolicy component, enabling the parent-child communication for agent selection events.
src/features/metadata-instance-editor/__tests__/CascadePolicy.test.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/features/metadata-instance-editor/Instance.js (2)
76-76: State property still missing initialization.The
cascadePolicyConfigurationstate property is declared but still not initialized in thegetStatemethod, as previously flagged.
350-365: Method logic is correct, but missing AgentType import.The
onAIAgentSelectmethod correctly handles agent selection by setting enhanced configuration for agent id '2' and clearing it otherwise. However, theAgentTypeimport is still missing as previously flagged.
🧹 Nitpick comments (1)
i18n/en-US.properties (1)
1265-1266: Add translator context comments for the new agent namesUnlike the surrounding keys, these two entries lack a preceding comment that explains where the string is surfaced. A one-line description (e.g., “# Display name shown in the AI Agent selector dropdown – do not translate product names”) helps translators and avoids future confusion.
📜 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__/Instance.test.js.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
i18n/en-US.properties(2 hunks)package.json(1 hunks)src/common/types/metadata.js(1 hunks)src/features/metadata-instance-editor/CascadePolicy.js(5 hunks)src/features/metadata-instance-editor/Instance.js(8 hunks)src/features/metadata-instance-editor/__tests__/CascadePolicy.test.js(1 hunks)src/features/metadata-instance-editor/constants.js(1 hunks)src/features/metadata-instance-editor/messages.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/features/metadata-instance-editor/messages.js
🚧 Files skipped from review as they are similar to previous changes (4)
- package.json
- src/features/metadata-instance-editor/constants.js
- src/common/types/metadata.js
- src/features/metadata-instance-editor/tests/CascadePolicy.test.js
🧰 Additional context used
🧠 Learnings (4)
📓 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.
i18n/en-US.properties (1)
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.
src/features/metadata-instance-editor/CascadePolicy.js (7)
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.
Learnt from: jpan-box
PR: #4166
File: src/elements/content-sidebar/SidebarNav.js:126-126
Timestamp: 2025-07-11T14:43:02.677Z
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: #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: rafalmaksymiuk
PR: #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.
Learnt from: rafalmaksymiuk
PR: #4160
File: src/elements/content-sidebar/SidebarToggle.js:13-19
Timestamp: 2025-06-25T13:09:45.168Z
Learning: Files with @flow or @flow strict comments at the top use Flow type syntax, not TypeScript. Flow type definitions like type Props = { ... } and type imports like type { RouterHistory } are valid Flow syntax and should not be flagged as TypeScript-only features.
Learnt from: rafalmaksymiuk
PR: #4160
File: src/elements/content-sidebar/SidebarToggle.js:11-11
Timestamp: 2025-06-25T13:10:19.128Z
Learning: In the box-ui-elements codebase, Flow files support import type syntax for type-only imports. The import type statement is valid Flow syntax, not just TypeScript, and should not be flagged as an error when used in Flow files (indicated by @flow comments).
Learnt from: rafalmaksymiuk
PR: #4156
File: src/elements/content-sidebar/SidebarNavButton.js:77-102
Timestamp: 2025-06-25T20:05:18.480Z
Learning: Rendering functions (functions that return JSX) are considered anti-patterns in React development because they create new function instances on every render, break React DevTools, interfere with reconciliation, and make testing more difficult.
src/features/metadata-instance-editor/Instance.js (4)
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.
Learnt from: rafalmaksymiuk
PR: #4160
File: src/elements/content-sidebar/SidebarToggle.js:11-11
Timestamp: 2025-06-25T13:10:19.128Z
Learning: In the box-ui-elements codebase, Flow files support import type syntax for type-only imports. The import type statement is valid Flow syntax, not just TypeScript, and should not be flagged as an error when used in Flow files (indicated by @flow comments).
Learnt from: jpan-box
PR: #4166
File: src/elements/content-sidebar/SidebarNav.js:126-126
Timestamp: 2025-07-11T14:43:02.677Z
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: #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.
🧬 Code Graph Analysis (1)
src/features/metadata-instance-editor/Instance.js (1)
src/features/metadata-instance-editor/constants.js (1)
ENHANCED_AGENT_CONFIGURATION(5-5)
🪛 Biome (1.9.4)
src/features/metadata-instance-editor/CascadePolicy.js
[error] 8-8: 'import { type x ident }' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 83-83: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 88-88: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 61-61: Shouldn't redeclare 'formatMessage'. Consider to delete it or rename it.
'formatMessage' is defined here:
(lint/suspicious/noRedeclare)
src/features/metadata-instance-editor/Instance.js
[error] 354-354: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 354-354: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 354-354: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
⏰ 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 (9)
i18n/en-US.properties (1)
1293-1294: Verify that equivalent keys exist in all locale bundlesPlease ensure
boxui.metadataInstanceEditor.enhancedAgentNameand...standardAgentNameare added to every supported locale file to prevent runtime fallback to English or missing-string placeholders.src/features/metadata-instance-editor/CascadePolicy.js (3)
5-10: LGTM! New imports support enhanced AI agent selection functionality.The imports correctly add support for internationalization, memoization, and the new AI agent selector component with proper type annotations.
63-78: Excellent implementation of memoized, internationalized agents.The use of
useMemowith proper dependencies prevents unnecessary re-renders, and the internationalization support makes the component more accessible globally.
187-195: LGTM! AI agent selector integration is properly implemented.The conditional rendering and callback props are correctly configured for the new agent selector component.
src/features/metadata-instance-editor/Instance.js (5)
27-27: LGTM! New constant import supports enhanced agent configuration.The
ENHANCED_AGENT_CONFIGURATIONconstant import is correctly added to support the AI agent selection functionality.
41-41: LGTM! Type import ensures proper typing for cascade policy configuration.The
MetadataCascadePolicyConfigurationtype import is correctly added to support the new state property.
222-222: LGTM! Correctly destructures cascade policy configuration from state.The
cascadePolicyConfigurationis properly extracted from state for use in the save operation.
246-246: LGTM! Cascade policy configuration correctly integrated into save flow.The configuration is properly passed to the save callback, enabling persistence of the selected AI agent settings.
709-709: LGTM! Properly connects AI agent selection callback to child component.The
onAIAgentSelectprop is correctly passed to theCascadePolicycomponent, enabling proper communication between parent and child components.
tjuanitas
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/features/metadata-instance-editor/CascadePolicy.js (1)
8-8: Fix Flow syntax for type import.This file uses Flow (indicated by
@flowcomment), but the type import syntax is TypeScript-specific. In Flow, you can import types directly without thetypekeyword in the import statement.-import { type AgentType } from '@box/box-ai-agent-selector'; +import type { AgentType, AgentListResponse } from '@box/box-ai-agent-selector';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/features/metadata-instance-editor/__tests__/__snapshots__/Instance.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
i18n/en-US.properties(2 hunks)src/features/metadata-instance-editor/CascadePolicy.js(7 hunks)src/features/metadata-instance-editor/__tests__/CascadePolicy.test.js(1 hunks)src/features/metadata-instance-editor/__tests__/Instance.test.js(1 hunks)src/features/metadata-instance-editor/__tests__/Instances.test.js(4 hunks)src/features/metadata-instance-editor/__tests__/MetadataInstanceEditor.test.js(4 hunks)
🧠 Learnings (5)
📓 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/__tests__/Instance.test.js (1)
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.
src/features/metadata-instance-editor/__tests__/Instances.test.js (2)
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.
Learnt from: rafalmaksymiuk
PR: #4156
File: src/elements/content-sidebar/SidebarNavButton.js:77-102
Timestamp: 2025-06-25T20:05:18.480Z
Learning: Rendering functions (functions that return JSX) are considered anti-patterns in React development because they create new function instances on every render, break React DevTools, interfere with reconciliation, and make testing more difficult.
src/features/metadata-instance-editor/__tests__/MetadataInstanceEditor.test.js (1)
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.
src/features/metadata-instance-editor/CascadePolicy.js (7)
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.
Learnt from: jpan-box
PR: #4166
File: src/elements/content-sidebar/SidebarNav.js:126-126
Timestamp: 2025-07-11T14:43:02.677Z
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: #4160
File: src/elements/content-sidebar/SidebarToggle.js:11-11
Timestamp: 2025-06-25T13:10:19.128Z
Learning: In the box-ui-elements codebase, Flow files support import type syntax for type-only imports. The import type statement is valid Flow syntax, not just TypeScript, and should not be flagged as an error when used in Flow files (indicated by @flow comments).
Learnt from: rafalmaksymiuk
PR: #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: rafalmaksymiuk
PR: #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.
Learnt from: rafalmaksymiuk
PR: #4160
File: src/elements/content-sidebar/SidebarToggle.js:13-19
Timestamp: 2025-06-25T13:09:45.168Z
Learning: Files with @flow or @flow strict comments at the top use Flow type syntax, not TypeScript. Flow type definitions like type Props = { ... } and type imports like type { RouterHistory } are valid Flow syntax and should not be flagged as TypeScript-only features.
Learnt from: rafalmaksymiuk
PR: #4156
File: src/elements/content-sidebar/SidebarNavButton.js:77-102
Timestamp: 2025-06-25T20:05:18.480Z
Learning: Rendering functions (functions that return JSX) are considered anti-patterns in React development because they create new function instances on every render, break React DevTools, interfere with reconciliation, and make testing more difficult.
🧬 Code Graph Analysis (3)
src/features/metadata-instance-editor/__tests__/Instance.test.js (1)
src/features/metadata-instance-editor/constants.js (1)
CASCADE_POLICY_TYPE_AI_EXTRACT(3-3)
src/features/metadata-instance-editor/__tests__/Instances.test.js (2)
src/features/metadata-instance-editor/constants.js (1)
CASCADE_POLICY_TYPE_AI_EXTRACT(3-3)src/features/metadata-instance-editor/Instances.js (1)
Instances(24-58)
src/features/metadata-instance-editor/CascadePolicy.js (1)
src/features/metadata-instance-editor/messages.js (1)
messages(3-259)
🪛 Biome (1.9.4)
src/features/metadata-instance-editor/CascadePolicy.js
[error] 8-8: 'import { type x ident }' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 82-82: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 87-87: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
🚧 Files skipped from review as they are similar to previous changes (2)
- i18n/en-US.properties
- src/features/metadata-instance-editor/tests/CascadePolicy.test.js
🧰 Additional context used
🧠 Learnings (5)
📓 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/__tests__/Instance.test.js (1)
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.
src/features/metadata-instance-editor/__tests__/Instances.test.js (2)
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.
Learnt from: rafalmaksymiuk
PR: #4156
File: src/elements/content-sidebar/SidebarNavButton.js:77-102
Timestamp: 2025-06-25T20:05:18.480Z
Learning: Rendering functions (functions that return JSX) are considered anti-patterns in React development because they create new function instances on every render, break React DevTools, interfere with reconciliation, and make testing more difficult.
src/features/metadata-instance-editor/__tests__/MetadataInstanceEditor.test.js (1)
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.
src/features/metadata-instance-editor/CascadePolicy.js (7)
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.
Learnt from: jpan-box
PR: #4166
File: src/elements/content-sidebar/SidebarNav.js:126-126
Timestamp: 2025-07-11T14:43:02.677Z
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: #4160
File: src/elements/content-sidebar/SidebarToggle.js:11-11
Timestamp: 2025-06-25T13:10:19.128Z
Learning: In the box-ui-elements codebase, Flow files support import type syntax for type-only imports. The import type statement is valid Flow syntax, not just TypeScript, and should not be flagged as an error when used in Flow files (indicated by @flow comments).
Learnt from: rafalmaksymiuk
PR: #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: rafalmaksymiuk
PR: #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.
Learnt from: rafalmaksymiuk
PR: #4160
File: src/elements/content-sidebar/SidebarToggle.js:13-19
Timestamp: 2025-06-25T13:09:45.168Z
Learning: Files with @flow or @flow strict comments at the top use Flow type syntax, not TypeScript. Flow type definitions like type Props = { ... } and type imports like type { RouterHistory } are valid Flow syntax and should not be flagged as TypeScript-only features.
Learnt from: rafalmaksymiuk
PR: #4156
File: src/elements/content-sidebar/SidebarNavButton.js:77-102
Timestamp: 2025-06-25T20:05:18.480Z
Learning: Rendering functions (functions that return JSX) are considered anti-patterns in React development because they create new function instances on every render, break React DevTools, interfere with reconciliation, and make testing more difficult.
🧬 Code Graph Analysis (3)
src/features/metadata-instance-editor/__tests__/Instance.test.js (1)
src/features/metadata-instance-editor/constants.js (1)
CASCADE_POLICY_TYPE_AI_EXTRACT(3-3)
src/features/metadata-instance-editor/__tests__/Instances.test.js (2)
src/features/metadata-instance-editor/constants.js (1)
CASCADE_POLICY_TYPE_AI_EXTRACT(3-3)src/features/metadata-instance-editor/Instances.js (1)
Instances(24-58)
src/features/metadata-instance-editor/CascadePolicy.js (1)
src/features/metadata-instance-editor/messages.js (1)
messages(3-259)
🪛 Biome (1.9.4)
src/features/metadata-instance-editor/CascadePolicy.js
[error] 8-8: 'import { type x ident }' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 82-82: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 87-87: 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/CascadePolicy.js (3)
62-77: LGTM! Well-implemented internationalized agents list.The memoized agents list with internationalized names is a good implementation. The dependency on
formatMessageensures proper updates when the locale changes.
188-197: LGTM! Proper conditional rendering of the agent selector.The conditional rendering logic correctly shows the agent selector only when both feature flags are enabled and AI folder extraction is active. The props passed to the new component are appropriate.
86-93: Remove TypeScript syntax from Flow file.The parameter type annotation is TypeScript-only syntax. Since this is a Flow file, remove the type annotation.
- const handleAgentSelect = useCallback( - (agent: AgentType | null) => { - if (onAIAgentSelect) { - onAIAgentSelect(agent); - } - }, - [onAIAgentSelect], - ); + const handleAgentSelect = useCallback( + agent => { + if (onAIAgentSelect) { + onAIAgentSelect(agent); + } + }, + [onAIAgentSelect], + );⛔ Skipped due to learnings
Learnt from: rafalmaksymiuk PR: box/box-ui-elements#4160 File: src/elements/content-sidebar/SidebarToggle.js:13-19 Timestamp: 2025-06-25T13:09:45.168Z Learning: Files with `@flow` or `@flow strict` comments at the top use Flow type syntax, not TypeScript. Flow type definitions like `type Props = { ... }` and type imports like `type { RouterHistory }` are valid Flow syntax and should not be flagged as TypeScript-only features.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: rafalmaksymiuk PR: box/box-ui-elements#4160 File: src/elements/content-sidebar/SidebarToggle.js:11-11 Timestamp: 2025-06-25T13:10:19.128Z Learning: In the box-ui-elements codebase, Flow files support `import type` syntax for type-only imports. The `import type` statement is valid Flow syntax, not just TypeScript, and should not be flagged as an error when used in Flow files (indicated by @flow comments).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.Learnt from: rafalmaksymiuk PR: box/box-ui-elements#4156 File: src/elements/content-sidebar/SidebarNavButton.js:14-14 Timestamp: 2025-06-24T18:35:01.363Z Learning: The `import type` syntax is valid in Flow, not just TypeScript. Flow supports `import type { Type } from 'module'` for importing types, even in .js files with @flow pragma.src/features/metadata-instance-editor/__tests__/Instance.test.js (1)
866-890: LGTM! Comprehensive test coverage for AI agent selector.The test properly sets up the AI extraction cascade policy and verifies all the expected UI elements: cascade policy toggle, AI autofill toggle, and the agent selector combobox with the correct "Standard" label. The test structure is well-organized and matches the component's behavior.
src/features/metadata-instance-editor/__tests__/Instances.test.js (2)
258-274: LGTM! Well-structured test with proper setup.The test correctly configures the cascade policy type to AI_EXTRACT and verifies the presence of all expected UI elements including both toggle switches and the "Standard" combobox. The setup is comprehensive and matches the component's requirements.
288-288: LGTM! Consistent negative test cases.The negative test cases properly verify the absence of the agent selector when the feature flag is disabled or undefined, using the updated "Standard" label consistently.
Also applies to: 299-299
src/features/metadata-instance-editor/__tests__/MetadataInstanceEditor.test.js (2)
540-557: LGTM! Comprehensive integration test.The test properly sets up both feature flags, configures the AI extraction cascade policy, and verifies the complete integration from MetadataInstanceEditor through to the agent selector UI. The assertions cover all the key elements and the test structure is well-organized.
571-571: LGTM! Consistent negative test assertions.The negative test cases maintain consistency with the updated "Standard" label and properly verify the component behavior when the feature is disabled.
Also applies to: 582-582
src/features/metadata-instance-editor/__tests__/MetadataInstanceEditor.test.js
Outdated
Show resolved
Hide resolved
Summary by CodeRabbit