feat(metadata-instance-editor): support for cascade extraction#4613
feat(metadata-instance-editor): support for cascade extraction#4613alexkirillovtech wants to merge 5 commits into
Conversation
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefactors the metadata instance editor to split extract-managed and editable flows. Adds CustomExtractAgentInstanceBody (managed view), EditableInstanceBody (editable form), and InstanceCard (collapsible container). Instance now routes between managed and editable bodies and threads an optional onManageExtractAgent callback through the editor tree. ChangesMetadata instance editor — extract-managed and delegated bodies
Sequence Diagram(s)sequenceDiagram
participant Parent as Parent Component
participant MetadataInstanceEditor
participant Instances
participant Instance
participant InstanceCard
participant CustomExtractAgentInstanceBody
participant EditableInstanceBody
Parent->>MetadataInstanceEditor: provide onManageExtractAgent callback
MetadataInstanceEditor->>Instances: pass onManageExtractAgent
Instances->>Instance: pass onManageExtractAgent
Instance->>Instance: evaluate cascadePolicy
alt ai_extract with custom agent
Instance->>InstanceCard: render card
InstanceCard->>CustomExtractAgentInstanceBody: render managed body
CustomExtractAgentInstanceBody->>CustomExtractAgentInstanceBody: isEditing?
alt isEditing and hasResolvedAgentId and onManageExtractAgent
CustomExtractAgentInstanceBody->>Parent: onManageExtractAgent(numericAgentId)
else collapsed
CustomExtractAgentInstanceBody->>InstanceCard: render read-only child instances
end
else standard or enhanced policy
Instance->>InstanceCard: render card
InstanceCard->>EditableInstanceBody: render editable form
EditableInstanceBody->>EditableInstanceBody: render CascadePolicy, fields, Footer
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
alexkirillovtech
changed the title
alexkirillovtech
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/features/metadata-instance-editor/ExtractManagedInstanceBody.js`:
- Line 27: The button still renders and calls the manage callback when
getNumericAgentId(agentId) returns an empty string; update the render and
click-path to guard on the parsed numeric id (use getNumericAgentId) instead of
the raw agentId: only render the "Manage agent" button when parsedId is
non-empty and ensure the onClick handler passes parsedId (or early-returns if
parsedId is falsy) so the callback is never invoked with ''. Locate uses of
getNumericAgentId and the Manage button/callback in ExtractManagedInstanceBody
(and the click handler around the "Manage agent" action) and replace checks to
gate on the parsed numeric id.
- Around line 44-45: Replace the hardcoded iconAriaLabel="lockicon" in the
ExtractManagedInstanceBody component with the localized message you already
added: call formatMessage(messages.extractManagedNoticeIconAriaLabel) (i.e., set
iconAriaLabel={formatMessage(messages.extractManagedNoticeIconAriaLabel)}), so
the aria label uses the localizable extractManagedNoticeIconAriaLabel string
instead of the literal "lockicon".
- Line 6: Add a Flow suppression comment above the Lock import in
ExtractManagedInstanceBody.js: place "// $FlowFixMe - blueprint-web-assets icons
not typed for Flow" immediately above the existing "import { Lock } from
'`@box/blueprint-web-assets/icons/Line`';" to match the pattern used in
Classification.js; replace the hardcoded iconAriaLabel="lockicon" with a
localized string pulled from react-intl/messages (import the message and use
intl.formatMessage or the <FormattedMessage> equivalent) so the aria label is
localized; and when rendering PrimaryAction, wrap the call to
onManageExtractAgent(...) with a guard that verifies getNumericAgentId(agentId)
returns a non-empty/truthy value before invoking it (use
getNumericAgentId(agentId) in the condition to prevent calling
onManageExtractAgent with an empty id).
🪄 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: 8d7970ad-21f6-43f9-8c83-d2e39c56b2d0
⛔ Files ignored due to path filters (2)
i18n/en-US.propertiesis excluded by!i18n/**src/features/metadata-instance-editor/__tests__/__snapshots__/Instance.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (15)
src/features/metadata-instance-editor/EditableInstanceBody.jssrc/features/metadata-instance-editor/ExtractManagedInstanceBody.jssrc/features/metadata-instance-editor/ExtractManagedInstanceBody.scsssrc/features/metadata-instance-editor/Instance.jssrc/features/metadata-instance-editor/InstanceCard.jssrc/features/metadata-instance-editor/Instances.jssrc/features/metadata-instance-editor/MetadataInstanceEditor.jssrc/features/metadata-instance-editor/__tests__/EditableInstanceBody.test.jssrc/features/metadata-instance-editor/__tests__/ExtractManagedInstanceBody.test.jssrc/features/metadata-instance-editor/__tests__/Instance.test.jssrc/features/metadata-instance-editor/__tests__/InstanceCard.test.jssrc/features/metadata-instance-editor/__tests__/Instances.test.jssrc/features/metadata-instance-editor/__tests__/MetadataInstanceEditor.test.jssrc/features/metadata-instance-editor/__tests__/__fixtures__/metadataInstances.jssrc/features/metadata-instance-editor/messages.js
| */ | ||
| const ExtractManagedInstanceBody = ({ agentId, data, isEditing, onManageExtractAgent, template }: Props) => { | ||
| const { formatMessage } = useIntl(); | ||
| const isProperties = template.templateKey === TEMPLATE_CUSTOM_PROPERTIES; |
There was a problem hiding this comment.
| const isProperties = template.templateKey === TEMPLATE_CUSTOM_PROPERTIES; | |
| const isCustomProperties = template.templateKey === TEMPLATE_CUSTOM_PROPERTIES; |
There was a problem hiding this comment.
it may seem like a new code, but it's actually an old code, that is consistent across multiple files and the value of constant is actually properties. I know it's not ideal, but I would like to keep extra changes at minimum
const TEMPLATE_CUSTOM_PROPERTIES: 'properties' = 'properties';
| .metadata-instance-editor-extract-managed { | ||
| padding: 8px; | ||
|
|
||
| .metadata-instance-editor-extract-managed-actions { |
There was a problem hiding this comment.
What does this CSS selector stand for? Is this overwriting a style from the Blueprint?
There was a problem hiding this comment.
it's a dead code, removed feat(metadata-instance-editor): cleanup (MDX-2041)
| text={formatMessage(messages.extractManagedDescription)} | ||
| title={formatMessage(messages.extractManagedTitle)} | ||
| > | ||
| <> |
There was a problem hiding this comment.
nit: redundant fragment
There was a problem hiding this comment.
| * Extracts the numeric agent id from a prefixed configuration value. | ||
| * e.g. "extract_agent_14637327713" -> "14637327713" | ||
| */ | ||
| const getNumericAgentId = (agentId?: string): string => (agentId ? agentId.replace(/\D/g, '') : ''); |
There was a problem hiding this comment.
Hey, I noticed that the agent value doesn't always follow the extract_agent_ format. In the same module, there is a second type of agent - enhanced - defined as enhanced_extract_agent (https://github.com/box/box-ui-elements/blob/master/src/features/metadata-instance-editor/constants.js#L5).
Furthermore, Instance.js actually saves this value as agent https://github.com/box/box-ui-elements/blob/master/src/features/metadata-instance-editor/Instance.js#L364.
Note that enhanced_extract_agent does not contain any digits.
So, for an instance managed by an enhanced agent:
getNumericAgentId('enhanced_extract_agent')
// 'enhanced_extract_agent'.replace(/\D/g, '') → '' (empty string)
User impact: the instance with the enhanced agent displays the note "Managed by a Box AI extract agent", but provides no way to navigate to agent management - resulting in a dead end.
Is this really how it's supposed to work?
There was a problem hiding this comment.
Thank you for a great Marcin! You actually surfaced a major problem in the way how we identified a custom extract agent managed instance.
My initial understanding what that type const CASCADE_POLICY_TYPE_AI_EXTRACT = 'ai_extract'; is enough to tell that it's create by extract custom agent. What was a big mistake!
Turned out if I enable Box AI on a regular cascade policy via metadata folder it will become ai_extract type. The same as when it's created by extract custom agent it's also ai_extract. So to say that it's for sure custom extract agent we need to make sure that we check for a suffix const CUSTOM_EXTRACT_AGENT_CONFIGURATION_PREFIX = 'extract_agent_';
Unfortunately it's the only way how differentiate them. I brought my concern to a platform team https://box.slack.com/archives/C03ET4QCU7P/p1780928270883609. However, it seems that this prefix is stable enough of now (check thread I attached there)
So to address this problem I made two type of changes:
- Re-work of differentiator logic - feat(metadata-instance-editor): fix custom extract agent logic (MDX-2…
- Re-work of naming system to make sure we do not mess with "extract" (cascade policy with AI) and "custom extract agent" (automate) feat(metadata-instance-editor): work on clarity of custom agent separ…
| data: MetadataFields, | ||
| errors: { [string]: React.Node }, | ||
| isAIFolderExtractionEnabled: boolean, | ||
| isBusy: boolean, |
There was a problem hiding this comment.
Can we rename the isBusy prop to isLoading here? It's only passed to <LoadingIndicatorWrapper isLoading={...}>, so matching the name makes the intent clearer. The parent can still pass isLoading={isBusy} — no need to rename the state variable in Instance.
There was a problem hiding this comment.
this code is not new, but an old component that I moved. I would like to avoid unnecessary refactoring in this PR
| if (shouldConfirmRemove) { | ||
| return ( | ||
| <LoadingIndicatorWrapper isLoading={isBusy}> | ||
| <MetadataInstanceConfirmDialog | ||
| confirmationMessage={confirmationMessage} | ||
| onCancel={onConfirmCancel} | ||
| onConfirm={onRemove} | ||
| /> | ||
| </LoadingIndicatorWrapper> | ||
| ); | ||
| } |
There was a problem hiding this comment.
- The
shouldConfirmRemoveearly return on line 87 means this component has two completely unrelated render paths. A cleaner split would be having Instance.js handles the rendering logic, the same way it already chooses between<CustomExtractAgentInstanceBody>and<EditableInstanceBody>. This would also remove 4 props (shouldConfirmRemove, confirmationMessage, onConfirmCancel, onRemove) for this component. - Since EditableInstanceBody is only used here and isBusy is owned by Instance, consider wrapping it with LoadingIndicatorWrapper at this level instead of inside the body. That way the body stays purely presentational and you can drop the isBusy prop from its interface.
There was a problem hiding this comment.
While you might be right on this, but I would like to avoid making any refactoring/improvements for existing code. I know it shows as a new component, but I actually just extracted it from existing one
- remove confirmation is not related to a CustomExtractAgent, since it's not possible to remove an instance created by extract agent
I know a code is not golden example, but in scope of BUIE I believe smaller steps are the better
| <span | ||
| className={classNames('metadata-instance-editor-instance-title-text', { | ||
| 'metadata-instance-editor-instance-has-error': hasError, | ||
| })} | ||
| > | ||
| {isProperties ? <FormattedMessage {...messages.customTitle} /> : template.displayName} | ||
| </span> |
There was a problem hiding this comment.
This can be blueprint Text component instead ex. something like:
<Text
as="span"
variant="bodyDefaultBold"
color={hasError ? 'textOnLightError' : 'textOnLightDefault'}
>
{isProperties ? <FormattedMessage {...messages.customTitle} /> : template.displayName}
</Text>
There was a problem hiding this comment.
same as prev, it's an old code, don't really want to change it
| import Collapsible from '../../components/collapsible/Collapsible'; | ||
| import IconMetadataColored from '../../icons/general/IconMetadataColored'; | ||
| import IconAlertCircle from '../../icons/general/IconAlertCircle'; | ||
| import { bdlWatermelonRed } from '../../styles/variables'; |
There was a problem hiding this comment.
Can this be replaced with the blueprint variable $watermelon-red-100?
There was a problem hiding this comment.
this code is not new, but an old component that I moved. I would like to avoid unnecessary refactoring in this PR
Co-authored-by: Cursor <cursoragent@cursor.com>
alexkirillovtech
force-pushed
the
MDX-2041-cascade-extract-support-metadata-instance
branch
from
377406f to
afad3c7
Compare
4 participants
Description
Product scope
When a metadata instance on a folder is managed by a Box AI extract agent (i.e. its
cascadePolicy.cascadePolicyType === ai_extract), the values for that instance are populated and kept in sync by the agent. Editing those values inline in the metadata sidebar would put the form's local state out of sync with what the agent will produce on the next cascade run, so this PR removes the inline-edit affordance for those instances and points users to the right place to make changes — the agent itself.Resulting UX for an extract-managed instance:
ActionableInlineNotice(lock icon) titled "Managed by a Box AI extract agent" with body copy explaining that the instance can't be edited here, plus a primary "Manage agent" button. Clicking it invokes a newonManageExtractAgent(agentId)callback, where the numeric agent id is extracted from the cascade policy'sagentconfiguration value. The host app (EUA) wires this up to navigate to the agent management surface.For all non-extract-managed instances the existing behavior is preserved exactly — same form, same cascade-policy controls, same save/remove footer, same confirm-remove dialog.
Selected approach
Instance.jshad grown into a ~700-line class component that owned both the editor state machine and the entire view tree (collapsible chrome, cascade policy, form, footer, confirm dialog). Rather than add a third rendering branch into that file, this PR splits the presentational layer into three focused components and keepsInstanceas the stateful orchestrator:InstanceCard.jsEditableInstanceBody.jsCascadePolicy+TemplatedInstance/CustomInstance+Footer+ confirm-remove dialog. Pure presentational — every handler and flag is passed in as a prop.ExtractManagedInstanceBody.jscascadePolicyType === ai_extract. Renders the read-only view when collapsed and theActionableInlineNotice+ "Manage agent" button when editing.Instance.jsnow:isEditing,isCascadingEnabled,cascadePolicyConfiguration, etc.) and all the change handlers (onSave,onFieldChange,onCascadeToggle,onAIAgentSelect, …) — unchanged behavior;isExtractManaged = cascadePolicy?.cascadePolicyType === CASCADE_POLICY_TYPE_AI_EXTRACT);InstanceCardand the body to one of the two new components.The new
onManageExtractAgentcallback is added as an optional prop and threadedMetadataInstanceEditor → Instances → Instance → ExtractManagedInstanceBody. When it's not provided (or noagentIdis on the cascade policy), the "Manage agent" button is simply not rendered, so existing consumers are unaffected.Why this split rather than inlining the new branch in
Instance:Instanceno longer mixes "what to show when" with "how to lay out a card / form / notice"; each new file is small, props-only, and unit-testable in isolation (see the new test files).ExtractManagedInstanceBody), which makes future tweaks to that UX — copy, additional CTAs, telemetry tags — a single-file change.connect, no lifecycle), so they avoid the snapshot churn that touchingInstanceitself causes; the bulk of the snapshot diff in this PR comes fromInstance.test.jsre-rendering through the new component tree rather than from semantic changes.What's included
Source
Instance.js— refactor: extract chrome + body, addisExtractManagedbranch, accept newonManageExtractAgentprop.InstanceCard.js— new presentational card.EditableInstanceBody.js— new presentational editable body.ExtractManagedInstanceBody.js+.scss— new presentational managed body.Instances.js,MetadataInstanceEditor.js— forward the new optionalonManageExtractAgentprop.messages.js— 4 new i18n strings (extractManagedTitle,extractManagedDescription,extractManagedManageAgent,extractManagedNoticeIconAriaLabel).i18n/en-US.properties— generated.Tests
EditableInstanceBody.test.js,ExtractManagedInstanceBody.test.js,InstanceCard.test.js, plus a shared__fixtures__/metadataInstances.js.Instance.test.js,Instances.test.js,MetadataInstanceEditor.test.jsto cover the new prop pass-through and the extract-managed branching (view + edit).Instance.test.js.snap— large diff, but purely structural (rendering goes throughInstanceCard/EditableInstanceBodywrappers now).All 650 tests pass; coverage on the touched module is 100% statements / ~85% branches for the new files.
How to test
Screenshots
Semantic release type
PR title:
feat(metadata-instance-editor): support cascade extractionfeat— New feature (extract-agent-managed metadata instances)Summary by CodeRabbit
New Features
Refactor
Tests