fix: support mid-migration metadata templates - #4653
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:
WalkthroughMetadata APIs and types now support namespace-aware templates and namespace-only instances across SCOPED, MIGRATION, and FINAL modes. The sidebar adds namespace browsing, template creation/editing, schema loading, and metadata refresh flows backed by live or mocked APIs. ChangesNamespace metadata workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
74d0d83 to
c73a5f2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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 `@s.md`:
- Around line 2280-2286: The LIST Cascade Policy FINAL description is using the
wrong operation wording; update the text in the metadata cascade policies
documentation so it says listing/retrieval instead of creation. Locate the
endpoint description around the GET metadata_cascade_policies entry and replace
the misleading phrase in the final section while keeping the rest of the spec
reference intact.
- Around line 2068-2072: The GET Cascade Policy (SCOPED) description is using
the wrong action wording, referring to “creation” in a retrieval endpoint.
Update the endpoint description text in the relevant GET metadata cascade policy
entry so it clearly says retrieval/fetching instead of creation, keeping the
rest of the spec reference unchanged.
- Around line 1832-1843: The LIST Instance FINAL description includes an
irrelevant scope restriction that belongs to other operations. Update the text
in the metadata listing section so it only describes the LIST Instance behavior,
and remove the “If global is passed instead of box.metadata, returns 400”
sentence from the listing entry. Use the surrounding metadata endpoint section
and its LIST Instance heading to locate the affected copy-pasted wording.
- Around line 2229-2235: The LIST Cascade Policy migration text uses the wrong
operation wording in the endpoint description; update the copy in the metadata
cascade policies section so it says “listing” or “retrieval” instead of
“creation.” Locate the affected wording in the migration docs around the GET
metadata_cascade_policies entry and replace the misleading phrase while keeping
the rest of the spec reference unchanged.
- Around line 2185-2191: The operation description for the LIST Cascade Policy
SCOPED endpoint is incorrect because it says “creation” instead of describing a
list/retrieval action. Update the text in the metadata cascade policy section so
the wording matches the GET metadata_cascade_policies endpoint and the
surrounding “LIST Cascade Policy” context, using the relevant SCOPED entry to
locate it.
- Around line 2147-2152: The GET Cascade Policy FINAL description uses the wrong
action term, since this endpoint is for retrieval rather than creation. Update
the wording in the metadata cascade policy GET section so the sentence about
scoped and namespaced cascade policy behavior refers to retrieval or fetching,
and keep the rest of the endpoint description aligned with the GET operation
name and path.
- Around line 2103-2108: The GET Metadata Cascade Policy migration description
is using the wrong operation wording, referring to “creation” for a retrieval
endpoint. Update the text in the GET Cascade Policy section so it describes
fetching/retrieving the policy rather than creating it, keeping the rest of the
endpoint details and spec reference unchanged.
- Line 1: The PR title contains a typo: update “temolates” to “templates” in the
title text so it matches the intended wording.
In `@src/api/Metadata.js`:
- Around line 565-575: The template lookup in Metadata.js can incorrectly match
a namespace-only template when $scope is undefined because the primary
templates.find in the scope-based path matches on undefined scope and blocks the
namespace fallback. Update the template selection logic around the namespace and
scope checks so the first lookup only runs when scope is actually defined, and
let the namespace-based fallback handle namespace-only instances in
MIGRATION/FINAL mode using templateKey plus namespace.
- Around line 423-431: The schema cache in getSchemaByTemplateKey currently uses
only templateKey, which can return the wrong result when scope changes between
calls. Update the cache key generation and lookup in
Metadata.getSchemaByTemplateKey/getMetadataTemplateSchemaCacheKey so it
incorporates scope alongside templateKey, and ensure the same scope-aware key is
used for both cache.has and cache.get before calling
getMetadataTemplateSchemaUrl.
🪄 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: 9cbcb786-2c6b-4362-ad31-a1b633615294
📒 Files selected for processing (3)
s.mdsrc/api/Metadata.jssrc/common/types/metadata.js
| @@ -0,0 +1,2390 @@ | |||
| Namespaces API Specification | |||
There was a problem hiding this comment.
Fix typo in PR title: "temolates" → "templates"
The PR title contains a typo: "temolates" should be "templates".
🤖 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 `@s.md` at line 1, The PR title contains a typo: update “temolates” to
“templates” in the title text so it matches the intended wording.
| 2.5.3 FINAL | ||
| Full URL: | ||
| GET https://api.box.com/2.0/files/{file_id}/metadata | ||
|
|
||
| GET https://api.box.com/2.0/folders/{folder_id}/metadata | ||
|
|
||
| Both scoped and namespaced instance retrieval is allowed. | ||
| If global is passed instead of box.metadata, returns 400. | ||
| See the full specs: | ||
| https://developer.box.com/reference/get-files-id-metadata | ||
| https://developer.box.com/reference/get-folders-id-metadata | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Remove irrelevant restriction from LIST Instance (FINAL)
Line 1839 states "If global is passed instead of box.metadata, returns 400" but the LIST Instance endpoint path (/files/{file_id}/metadata) does not include a scope/namespace path parameter. This appears to be a copy-paste error from GET/CREATE/UPDATE/DELETE sections.
🤖 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 `@s.md` around lines 1832 - 1843, The LIST Instance FINAL description includes
an irrelevant scope restriction that belongs to other operations. Update the
text in the metadata listing section so it only describes the LIST Instance
behavior, and remove the “If global is passed instead of box.metadata, returns
400” sentence from the listing entry. Use the surrounding metadata endpoint
section and its LIST Instance heading to locate the affected copy-pasted
wording.
| GET https://api.box.com/2.0/metadata_cascade_policies/{metadata_cascade_policy_id} | ||
|
|
||
| Only scoped cascade policy creation is allowed. | ||
| See the full spec https://developer.box.com/reference/get-metadata-cascade-policies-id | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix incorrect operation description in GET Cascade Policy (SCOPED)
Line 2070 says "Only scoped cascade policy creation is allowed" but this is the GET endpoint (3.3 GET Cascade Policy). Should be "retrieval" or similar.
🤖 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 `@s.md` around lines 2068 - 2072, The GET Cascade Policy (SCOPED) description
is using the wrong action wording, referring to “creation” in a retrieval
endpoint. Update the endpoint description text in the relevant GET metadata
cascade policy entry so it clearly says retrieval/fetching instead of creation,
keeping the rest of the spec reference unchanged.
| 3.3.2 MIGRATION | ||
| Full URL: | ||
| GET https://api.box.com/2.0/metadata_cascade_policies/{metadata_cascade_policy_id} | ||
|
|
||
| Both scoped and namespaced cascade policy creation is allowed. | ||
| See the full spec https://developer.box.com/reference/get-metadata-cascade-policies-id |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix incorrect operation description in GET Cascade Policy (MIGRATION)
Line 2107 says "Both scoped and namespaced cascade policy creation is allowed" but this is the GET endpoint. Should be "retrieval" or similar.
🤖 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 `@s.md` around lines 2103 - 2108, The GET Metadata Cascade Policy migration
description is using the wrong operation wording, referring to “creation” for a
retrieval endpoint. Update the text in the GET Cascade Policy section so it
describes fetching/retrieving the policy rather than creating it, keeping the
rest of the endpoint details and spec reference unchanged.
| 3.3.3 FINAL | ||
| Full URL: | ||
| GET https://api.box.com/2.0/metadata_cascade_policies/{metadata_cascade_policy_id} | ||
|
|
||
| Both scoped and namespaced cascade policy creation is allowed. | ||
| See the full spec https://developer.box.com/reference/get-metadata-cascade-policies-id |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix incorrect operation description in GET Cascade Policy (FINAL)
Line 2151 says "Both scoped and namespaced cascade policy creation is allowed" but this is the GET endpoint. Should be "retrieval" or similar.
🤖 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 `@s.md` around lines 2147 - 2152, The GET Cascade Policy FINAL description uses
the wrong action term, since this endpoint is for retrieval rather than
creation. Update the wording in the metadata cascade policy GET section so the
sentence about scoped and namespaced cascade policy behavior refers to retrieval
or fetching, and keep the rest of the endpoint description aligned with the GET
operation name and path.
| 3.4.1 SCOPED | ||
| Full URL: | ||
| GET https://api.box.com/2.0/metadata_cascade_policies | ||
|
|
||
| Only scoped cascade policy creation is allowed. | ||
| See the full spec https://developer.box.com/reference/get-metadata-cascade-policies | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix incorrect operation description in LIST Cascade Policy (SCOPED)
Line 2189 says "Only scoped cascade policy creation is allowed" but this is the LIST endpoint (3.4 LIST Cascade Policy). Should be "listing" or "retrieval".
🤖 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 `@s.md` around lines 2185 - 2191, The operation description for the LIST
Cascade Policy SCOPED endpoint is incorrect because it says “creation” instead
of describing a list/retrieval action. Update the text in the metadata cascade
policy section so the wording matches the GET metadata_cascade_policies endpoint
and the surrounding “LIST Cascade Policy” context, using the relevant SCOPED
entry to locate it.
| 3.4.2 MIGRATION | ||
| Full URL: | ||
| GET https://api.box.com/2.0/metadata_cascade_policies | ||
|
|
||
| Both scoped and namespaced cascade policy creation is allowed. | ||
| See the full spec https://developer.box.com/reference/get-metadata-cascade-policies | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix incorrect operation description in LIST Cascade Policy (MIGRATION)
Line 2233 says "Both scoped and namespaced cascade policy creation is allowed" but this is the LIST endpoint. Should be "listing" or "retrieval".
🤖 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 `@s.md` around lines 2229 - 2235, The LIST Cascade Policy migration text uses
the wrong operation wording in the endpoint description; update the copy in the
metadata cascade policies section so it says “listing” or “retrieval” instead of
“creation.” Locate the affected wording in the migration docs around the GET
metadata_cascade_policies entry and replace the misleading phrase while keeping
the rest of the spec reference unchanged.
| 3.4.3 FINAL | ||
| Full URL: | ||
| GET https://api.box.com/2.0/metadata_cascade_policies | ||
|
|
||
| Both scoped and namespaced cascade policy creation is allowed. | ||
| See the full spec https://developer.box.com/reference/get-metadata-cascade-policies | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix incorrect operation description in LIST Cascade Policy (FINAL)
Line 2284 says "Both scoped and namespaced cascade policy creation is allowed" but this is the LIST endpoint. Should be "listing" or "retrieval".
🤖 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 `@s.md` around lines 2280 - 2286, The LIST Cascade Policy FINAL description is
using the wrong operation wording; update the text in the metadata cascade
policies documentation so it says listing/retrieval instead of creation. Locate
the endpoint description around the GET metadata_cascade_policies entry and
replace the misleading phrase in the final section while keeping the rest of the
spec reference intact.
c73a5f2 to
c4696a8
Compare
|
|
JakubKida
changed the title
c4696a8 to
62f27b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/Metadata.js (1)
126-128: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSchema cache key still ignores
scope/namespace — cross-scope collisions unresolved.
getMetadataTemplateSchemaCacheKeykeys solely ontemplateKey, butgetMetadataTemplateSchemaUrl(Line 706) now varies the URL byscope/namespace. AtemplateKeyshared across an enterprise scope and a namespace FQN (exactly the MIGRATION-mode scenario this PR targets) will read/write the wrong cached schema after the first fetch.🐛 Proposed fix
- getMetadataTemplateSchemaCacheKey(templateKey: string): string { - return `${CACHE_PREFIX_METADATA}template_schema_${templateKey}`; + getMetadataTemplateSchemaCacheKey(templateKey: string, scope?: string = METADATA_SCOPE_ENTERPRISE): string { + return `${CACHE_PREFIX_METADATA}template_schema_${scope}_${templateKey}`; }- const key = this.getMetadataTemplateSchemaCacheKey(templateKey); + const key = this.getMetadataTemplateSchemaCacheKey(templateKey, scope);Also applies to: 694-710
🤖 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 `@src/api/Metadata.js` around lines 126 - 128, Update getMetadataTemplateSchemaCacheKey to include the same scope/namespace discriminator used by getMetadataTemplateSchemaUrl, so identical templateKey values in different scopes produce distinct cache keys. Propagate the scope or namespace parameter through all callers, including the schema fetch flow around getMetadataTemplateSchemaUrl, while preserving existing key formatting for the default scope.
♻️ Duplicate comments (1)
src/api/Metadata.js (1)
843-857: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPrimary scope match still short-circuits the namespace fallback when
$scopeis absent.For namespace-only instances,
scopeisundefined. The primary matcht.scope === scopesucceeds for any namespace-only template with matchingtemplateKey(sincet.scopeis alsoundefined), regardless ofnamespace, so the fallback on Line 851-853 never runs when multiple namespace-only templates share atemplateKey. NotedeleteMetadata(Line 1735-1738) in this same diff already uses the correct gated pattern (scope ? ... : ...).🐛 Proposed fix
- let template = templates.find(t => t.templateKey === templateKey && t.scope === scope); + let template = scope + ? templates.find(t => t.templateKey === templateKey && t.scope === scope) + : undefined;🤖 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 `@src/api/Metadata.js` around lines 843 - 857, Update the primary template lookup in the metadata resolution flow to match by scope only when scope is defined, using the gated pattern established by deleteMetadata. For namespace-only instances, skip the scope lookup so the namespace-based fallback selects the template matching namespace, while preserving scoped-instance behavior.
🧹 Nitpick comments (1)
src/elements/content-sidebar/MetadataTemplateDropdown.tsx (1)
72-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winProduction code parses a mock-specific ID encoding format.
This fallback decodes ids using the
"fqn||templateKey"delimiter format that, per the comment, only the mock API produces. Coupling production dropdown logic to a mock implementation detail is fragile — if a real API id ever coincidentally contains||, this would silently mis-parsenamespaceFqn/templateKey. Consider passing the browser-shape template object itself (or its resolved namespace/templateKey) through the event instead of re-deriving it from an encoded id string.🤖 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 `@src/elements/content-sidebar/MetadataTemplateDropdown.tsx` around lines 72 - 82, Remove the mock-specific "fqn||templateKey" parsing from the fallback in MetadataTemplateDropdown and pass the resolved browser-shape template data, including namespaceFqn and templateKey, through the edit event instead. Update the surrounding lookup/event flow so real API IDs are never interpreted as encoded mock IDs, while preserving editing for resolved and newly created templates.
🤖 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/api/Metadata.js`:
- Around line 428-458: Update updateMetadataTemplate to evict the metadata
template schema cache after a successful update, using
getMetadataTemplateSchemaCacheKey(templateKey). Include the relevant scope in
the cache key generation so namespace and enterprise schemas for the same
templateKey cannot share cached entries.
- Around line 1032-1055: The getEnterpriseScopeFromInstances method must extract
the enterprise scope from dot-delimited namespace values as well as path-like
values. Update the $namespace parsing so names such as enterprise_123456.legal
return enterprise_123456, while preserving the existing $scope precedence and
null behavior.
- Around line 1111-1133: Update the non-SCOPED branch around
getEnterpriseScopeFromInstances so enterpriseScope is resolved from an
authoritative bootstrap source before template fetching, rather than relying
only on instances. Preserve the instances-derived value as a fallback if
appropriate, and use the resolved enterprise FQN when gating and calling
getTemplates for enterprise templates so files without metadata can still reach
template management.
In `@src/api/metadataNamespaceMocks.js`:
- Line 200: Update mockUpdateMetadataTemplate so its successCallback receives
the fully updated metadata template, including existing properties such as id,
displayName, fields, and the updated templateKey, instead of the current stub
object; preserve the existing success flow.
In `@src/elements/content-sidebar/hooks/useMetadataTemplateEditor.tsx`:
- Around line 2-8: Add the missing `@box/metadata-template-editor` package to
package.json dependencies so imports in
src/elements/content-sidebar/hooks/useMetadataTemplateEditor.tsx (lines 2-8) and
src/elements/content-sidebar/MetadataSidebarRedesign.tsx (lines 20-24) resolve
successfully; no direct source changes are needed at either import site.
In `@src/elements/content-sidebar/hooks/useSidebarMetadataFetcher.ts`:
- Around line 291-293: Update the template lookup in extractSuggestions to
compare the candidate template’s scope or namespace directly against the
function’s scope parameter, while retaining the templateKey match. Remove the
truthiness-only condition so templates from different scopes cannot be selected.
In `@src/elements/content-sidebar/MetadataSidebarRedesign.tsx`:
- Around line 176-179: Guard the template lookup in the enterprise ID
initialization so it safely handles an undefined templates value, matching the
existing checks in isViewMode and metadataDropdown. Update the expression using
templates.find(...) to fall back to an empty collection while preserving the
existing enterprise scope filtering and numeric ID extraction.
---
Outside diff comments:
In `@src/api/Metadata.js`:
- Around line 126-128: Update getMetadataTemplateSchemaCacheKey to include the
same scope/namespace discriminator used by getMetadataTemplateSchemaUrl, so
identical templateKey values in different scopes produce distinct cache keys.
Propagate the scope or namespace parameter through all callers, including the
schema fetch flow around getMetadataTemplateSchemaUrl, while preserving existing
key formatting for the default scope.
---
Duplicate comments:
In `@src/api/Metadata.js`:
- Around line 843-857: Update the primary template lookup in the metadata
resolution flow to match by scope only when scope is defined, using the gated
pattern established by deleteMetadata. For namespace-only instances, skip the
scope lookup so the namespace-based fallback selects the template matching
namespace, while preserving scoped-instance behavior.
---
Nitpick comments:
In `@src/elements/content-sidebar/MetadataTemplateDropdown.tsx`:
- Around line 72-82: Remove the mock-specific "fqn||templateKey" parsing from
the fallback in MetadataTemplateDropdown and pass the resolved browser-shape
template data, including namespaceFqn and templateKey, through the edit event
instead. Update the surrounding lookup/event flow so real API IDs are never
interpreted as encoded mock IDs, while preserving editing for resolved and newly
created templates.
🪄 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: 8a483e3e-d013-45db-97e8-22bd0b44e780
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
package.jsonsrc/api/Base.jssrc/api/Metadata.jssrc/api/metadataNamespaceMocks.jssrc/common/types/api.jssrc/common/types/metadata.jssrc/constants.jssrc/elements/content-sidebar/MetadataInstanceEditor.tsxsrc/elements/content-sidebar/MetadataSidebarRedesign.tsxsrc/elements/content-sidebar/MetadataTemplateDropdown.tsxsrc/elements/content-sidebar/__tests__/MetadataSidebarRedesign.test.tsxsrc/elements/content-sidebar/hooks/useMetadataNamespaceMode.tssrc/elements/content-sidebar/hooks/useMetadataTemplateEditor.tsxsrc/elements/content-sidebar/hooks/useMetadataTemplateEventService.tssrc/elements/content-sidebar/hooks/useMetadataTemplateItemsService.tssrc/elements/content-sidebar/hooks/useSidebarMetadataFetcher.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/common/types/metadata.js
| async updateMetadataTemplate( | ||
| file: BoxItem, | ||
| namespaceFqn: string, | ||
| templateKey: string, | ||
| patchItems: Array<Object>, | ||
| successCallback: Function, | ||
| errorCallback: ElementsErrorCallback, | ||
| ): Promise<void> { | ||
| // TODO(MDX-2136): remove next two lines when namespace API is deployed | ||
| if (IS_NAMESPACE_API_MOCKED) { | ||
| mockUpdateMetadataTemplate(file, namespaceFqn, templateKey, patchItems, successCallback); | ||
| return; | ||
| } | ||
|
|
||
| const { id }: BoxItem = file; | ||
| this.errorCode = ERROR_CODE_UPDATE_METADATA_TEMPLATE; | ||
| const url = this.getMetadataTemplateSchemaUrl(templateKey, namespaceFqn); | ||
| try { | ||
| const response = await this.xhr.put({ | ||
| url, | ||
| id: getTypedFileId(id), | ||
| headers: { [HEADER_CONTENT_TYPE]: 'application/json-patch+json' }, | ||
| data: patchItems, | ||
| }); | ||
| if (!this.isDestroyed()) { | ||
| successCallback(getProp(response, 'data')); | ||
| } | ||
| } catch (e) { | ||
| errorCallback(e, this.errorCode); | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map Metadata.js structure and locate cache-related symbols
ast-grep outline src/api/Metadata.js --view expanded || true
printf '\n--- Search for cache key and invalidation symbols ---\n'
rg -n "getMetadataTemplateSchemaCacheKey|getSchemaByTemplateKey|cache\.unset|cache\.unsetAll|updateMetadataTemplate|metadata template schema" src/api/Metadata.js
printf '\n--- Relevant surrounding lines ---\n'
nl -ba src/api/Metadata.js | sed -n '360,760p'Repository: box/box-ui-elements
Length of output: 814
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for all references to schema cache key generation and invalidation across the repo
rg -n "getMetadataTemplateSchemaCacheKey|getSchemaByTemplateKey|unsetAll\(|unset\(" srcRepository: box/box-ui-elements
Length of output: 3850
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1080,1225p' src/api/Metadata.js
printf '\n--- schema fetch ---\n'
sed -n '684,725p' src/api/Metadata.js
printf '\n--- updateMetadataTemplate ---\n'
sed -n '428,470p' src/api/Metadata.jsRepository: box/box-ui-elements
Length of output: 8860
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact methods around the cache.unset calls and the schema cache helpers
python3 - <<'PY'
from pathlib import Path
p = Path('src/api/Metadata.js')
lines = p.read_text().splitlines()
for start, end in [(1080, 1225), (684, 725), (1100, 1225), (100, 140)]:
print(f"\n--- lines {start}-{end} ---")
for i in range(start-1, min(end, len(lines))):
print(f"{i+1}: {lines[i]}")
PYRepository: box/box-ui-elements
Length of output: 15194
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('src/api/Metadata.js')
lines = p.read_text().splitlines()
for start, end in [(220, 280), (720, 820), (820, 920)]:
print(f"\n--- lines {start}-{end} ---")
for i in range(start-1, min(end, len(lines))):
print(f"{i+1}: {lines[i]}")
PY
printf '\n--- callers of getSchemaByTemplateKey ---\n'
rg -n "getSchemaByTemplateKey\(" srcRepository: box/box-ui-elements
Length of output: 12725
Clear the metadata template schema cache after updates
updateMetadataTemplate never evicts getMetadataTemplateSchemaCacheKey(templateKey), so a cached schema can stay stale for the rest of the session. The cache key also ignores scope, which can mix schemas for the same templateKey across namespace/enterprise variants.
🧰 Tools
🪛 Biome (2.5.3)
[error] 429-429: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
(parse)
[error] 430-430: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
(parse)
[error] 431-431: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
(parse)
[error] 432-432: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
(parse)
[error] 433-433: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
(parse)
[error] 434-434: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
(parse)
[error] 435-435: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
(parse)
[error] 442-442: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
(parse)
🤖 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 `@src/api/Metadata.js` around lines 428 - 458, Update updateMetadataTemplate to
evict the metadata template schema cache after a successful update, using
getMetadataTemplateSchemaCacheKey(templateKey). Include the relevant scope in
the cache key generation so namespace and enterprise schemas for the same
templateKey cannot share cached entries.
62f27b5 to
064bd17
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
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/api/Metadata.js`:
- Around line 1755-1766: Guard the findIndex results in both metadata cache
removal branches before calling splice: only remove an entry when the returned
index is non-negative. Apply this to the instance lookup and the editor lookup
in the surrounding metadata update logic, preserving the existing matching
predicates and preventing splice(-1, 1) from removing an unrelated last entry.
In `@src/api/metadataNamespaceMocks.js`:
- Line 16: Update IS_NAMESPACE_API_MOCKED in metadataNamespaceMocks.js to
default to the live backend instead of being hardcoded true. Gate the in-memory
namespace mocks behind the existing development or test configuration so list,
create, and update flows use mocks only in those environments.
In `@src/elements/content-sidebar/hooks/useMetadataNamespaceMode.ts`:
- Around line 47-70: Update the useEffect in useMetadataNamespaceMode to use a
local cancellation flag scoped to each effect invocation instead of the shared
cancelledRef, setting it in the cleanup and checking it before applying results.
Cast the Promise result from getMetadataNamespaceMode to MetadataScopeMode |
null before passing it to setMode.
🪄 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: 881464c5-9db4-4df6-be32-68bcde47a46d
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
package.jsonsrc/api/Base.jssrc/api/Metadata.jssrc/api/metadataNamespaceMocks.jssrc/common/types/api.jssrc/common/types/metadata.jssrc/constants.jssrc/elements/content-sidebar/MetadataInstanceEditor.tsxsrc/elements/content-sidebar/MetadataSidebarRedesign.tsxsrc/elements/content-sidebar/MetadataTemplateDropdown.tsxsrc/elements/content-sidebar/__tests__/MetadataSidebarRedesign.test.tsxsrc/elements/content-sidebar/hooks/useMetadataNamespaceMode.tssrc/elements/content-sidebar/hooks/useMetadataTemplateEditor.tsxsrc/elements/content-sidebar/hooks/useMetadataTemplateEventService.tssrc/elements/content-sidebar/hooks/useMetadataTemplateItemsService.tssrc/elements/content-sidebar/hooks/useSidebarMetadataFetcher.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- src/common/types/api.js
- src/common/types/metadata.js
- src/elements/content-sidebar/MetadataInstanceEditor.tsx
- package.json
- src/elements/content-sidebar/hooks/useMetadataTemplateEventService.ts
- src/elements/content-sidebar/hooks/useSidebarMetadataFetcher.ts
- src/elements/content-sidebar/hooks/useMetadataTemplateEditor.tsx
- src/elements/content-sidebar/hooks/useMetadataTemplateItemsService.ts
- src/elements/content-sidebar/MetadataTemplateDropdown.tsx
- src/elements/content-sidebar/tests/MetadataSidebarRedesign.test.tsx
- src/constants.js
- src/elements/content-sidebar/MetadataSidebarRedesign.tsx
| instance => | ||
| instance.templateKey === templateKey && | ||
| (scope ? instance.scope === scope : instance.namespace === namespace), | ||
| ), | ||
| 1, | ||
| ); | ||
| } else { | ||
| metadata.editors.splice( | ||
| metadata.editors.findIndex( | ||
| editor => editor.template.scope === scope && editor.template.templateKey === templateKey, | ||
| editor => | ||
| editor.template.templateKey === templateKey && | ||
| (scope ? editor.template.scope === scope : editor.template.namespace === namespace), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Guard findIndex results before splicing the cache.
When no matching instance or editor exists, findIndex returns -1, causing splice(-1, 1) to remove the last unrelated entry.
Proposed fix
- metadata.instances.splice(metadata.instances.findIndex(instance => /* match */), 1);
+ const instanceIndex = metadata.instances.findIndex(instance => /* match */);
+ if (instanceIndex >= 0) {
+ metadata.instances.splice(instanceIndex, 1);
+ }
- metadata.editors.splice(metadata.editors.findIndex(editor => /* match */), 1);
+ const editorIndex = metadata.editors.findIndex(editor => /* match */);
+ if (editorIndex >= 0) {
+ metadata.editors.splice(editorIndex, 1);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| instance => | |
| instance.templateKey === templateKey && | |
| (scope ? instance.scope === scope : instance.namespace === namespace), | |
| ), | |
| 1, | |
| ); | |
| } else { | |
| metadata.editors.splice( | |
| metadata.editors.findIndex( | |
| editor => editor.template.scope === scope && editor.template.templateKey === templateKey, | |
| editor => | |
| editor.template.templateKey === templateKey && | |
| (scope ? editor.template.scope === scope : editor.template.namespace === namespace), | |
| const instanceIndex = metadata.instances.findIndex( | |
| instance => | |
| instance.templateKey === templateKey && | |
| (scope ? instance.scope === scope : instance.namespace === namespace), | |
| ); | |
| if (instanceIndex >= 0) { | |
| metadata.instances.splice(instanceIndex, 1); | |
| } | |
| } else { | |
| const editorIndex = metadata.editors.findIndex( | |
| editor => | |
| editor.template.templateKey === templateKey && | |
| (scope ? editor.template.scope === scope : editor.template.namespace === namespace), | |
| ); | |
| if (editorIndex >= 0) { | |
| metadata.editors.splice(editorIndex, 1); | |
| } |
🤖 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 `@src/api/Metadata.js` around lines 1755 - 1766, Guard the findIndex results in
both metadata cache removal branches before calling splice: only remove an entry
when the returned index is non-negative. Apply this to the instance lookup and
the editor lookup in the surrounding metadata update logic, preserving the
existing matching predicates and preventing splice(-1, 1) from removing an
unrelated last entry.
4145a1c to
7b3fcd8
Compare
d42825c to
f9ee5bb
Compare
d435ca2 to
fd99409
Compare
9a47365 to
47c2478
Compare
| * The modal JSX to render somewhere stable in the tree (e.g. beside | ||
| * `SidebarContent`). `null` when the editor is closed. | ||
| */ | ||
| modal: React.ReactNode; |
There was a problem hiding this comment.
hmmm i guess there is nothing technically wrong with having a hook return JSX, i'm just not used to seeing it. i would more expect there to be a standalone component to contain the JSX, and then that component uses this hook to manage the data.
i'm fine with doing things your way unless this piques anyone else's interest!
There was a problem hiding this comment.
Fair — a bit unconventional. Kept it this way so openCreate/openEdit stay colocated with the modal lifecycle without an extra wrapper. Happy to split into a component later if it feels awkward in review.
| @@ -342,6 +351,7 @@ export const ERROR_CODE_UPDATE_TASK_COLLABORATOR = 'update_task_collaborator_err | |||
| export const ERROR_CODE_UPDATE_COMMENT = 'update_comment_error'; | |||
| export const ERROR_CODE_UPDATE_SKILLS = 'update_skills_error'; | |||
| export const ERROR_CODE_UPDATE_METADATA = 'update_metadata_error'; | |||
There was a problem hiding this comment.
just curious since this could be confusing - is update_metadata_error for updating metadata instances? or some other metadata issue other than template?
There was a problem hiding this comment.
This is a pre-existing code — used for metadata instance updates (Metadata.js → updateMetadata / updateMetadataRedesign / bulk). Template schema updates use the new ERROR_CODE_UPDATE_METADATA_TEMPLATE (update_metadata_template_error) in MetadataNamespaces.js.
| @@ -312,6 +321,7 @@ | |||
There was a problem hiding this comment.
ope this one does not match either https://github.com/box/box-ui-elements/pull/4653/changes#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R145 quick update pls?
| "@chromatic-com/storybook": "^5.2.1", | ||
| "@commitlint/cli": "^19.8.0", | ||
| "@commitlint/config-conventional": "^19.8.0", | ||
| "@dnd-kit/core": "^6.1.0", |
There was a problem hiding this comment.
just calling out, other box-internal parent apps may need to add these as new deps which they may not be happy about. just something to look out for when developing shared features -> buie. i assume you're working on this same task though so it may be you doing that work
There was a problem hiding this comment.
Correct — they come in via @box/metadata-template-editor. Parent apps that don’t already pull them will need to add them (same pattern as other BUIE peer deps). I’ll call that out in the parent-app bump PRs.
14ccb96 to
ceee4ee
Compare
ceee4ee to
4fa4a50
Compare
74fa62b to
95548f6
Compare
4 participants
Description
Adds namespaced metadata template support to the redesigned Content Sidebar so hosts can run SCOPED → MIGRATION → FINAL without BUIE fetching enterprise configuration or minting tokens.
Hosts pass three things:
features.metadata.namespacesOptIn.enabled(opt-in),metadataNamespaceMode, andenterpriseId. Opt-in is the first gate. When it is off, host mode and enterprise id are ignored and template management stays off. When it is on, omitting mode (or passingSCOPED) is the legacy path;nullmeans flags are still loading. File-preview auth is enough for instances and template schema/list/CRUD.Default behavior without opt-in is unchanged.
Screenshots/Videos
N/A — host-driven; UI is the existing redesigned metadata sidebar plus template browser/management when mode is not SCOPED.
Related issues
https://jira.inside-box.net/browse/MDX-2319 (preview-client host wiring; merge after this PR)
https://jira.inside-box.net/browse/MDX-2318 (EUA app-feature flags)
Changes made
metadataNamespaceMode(SCOPED|MIGRATION|FINAL|null) andenterpriseId(number, numeric string, orenterprise_<id>FQN). BUIE normalizes the id; it does not call/users/meor GraphQL enterprise config.useMetadataNamespaceContextresolves opt-in first, then mode, loading, and whether template management is on.MetadataNamespacescollaborator owns list/create/update namespace HTTP.getMetadatatakes per-call mode and enterprise FQN; mode lives on theMetadataclient (notBase) so later URL helpers on that instance stay in sync.$scope: 'enterprise'is not treated as an FQN, so it cannot be compared againstenterprise_123and marked externally owned.null, we wait instead of fetching SCOPED and then refetching.IS_NAMESPACE_API_MOCKED). List helpers still catch errors and return{ entries: [] }.Type of change
Testing done
yarn test --watchAll=false --testPathPattern="(useSidebarMetadataFetcher|useMetadataNamespaceContext|metadataNamespaceUtils|Metadata.test|MetadataNamespaces.test)"— passing.MetadataSidebarRedesignsuite currently fails to load in this workspace because portal-linked@box/metadata-editorcannot resolve@box/types(unrelated to this change).Dependencies
No new runtime packages for the namespace feature.
yarn.lockchanges on the branch come from the shared-features bump already in history.Follow-up hosts (not this PR):
metadata_namespaces_opt_in→ opt-in, passenterpriseIdnamespacedMetadataTemplatesEnabled/scopedTemplatesMigrationEnabledonAppFeaturesRecordDeployment notes
Merge BUIE first, then preview-client, then EUA.
Omit opt-in / mode → current SCOPED-equivalent sidebar. Do not mint metadata tokens in BUIE. Namespace list/create/update is still mocked until the API is deployed; turn off
IS_NAMESPACE_API_MOCKEDin a follow-up.How to test
nvm use && yarn start(or the Content Sidebar redesign Storybook).features.metadata.namespacesOptIn.enabled: sidebar stays legacy; host mode/enterprise id are ignored.metadataNamespaceMode(or passSCOPED): scoped templates/instances only; no template management.metadataNamespaceMode: 'MIGRATION'or'FINAL', plusenterpriseId: namespaced fetch, template browser/management on. Confirm one metadata fetch on open (no SCOPED flash then refetch).metadataNamespaceMode: nullwhile flags load: sidebar stays loading, then fetches once mode arrives.enterprise_<otherId>) stay marked externally owned;$scope: 'enterprise'shorthand is not treated as a foreign FQN.How to review
useMetadataNamespaceContext.tsandMetadataSidebarRedesignprops. Confirm BUIE never fetches ECR or/users/me.Metadata.js+MetadataNamespaces.js+metadataNamespaceUtils.js. Mode onMetadataonly; per-callgetMetadataoptions; FQN vsenterpriseshorthand.useSidebarMetadataFetcher.ts— one keyed fetch,isLoadingskip.masteris behind origin (those already have their own PRs: refactor(toggle): migrate Toggle from Flow to TypeScript #4764–refactor(modal): migrate Modal from Flow to TypeScript #4771, fix(activity-feed-v2): Map mention contacts via SelectorItem.item #4783).Self-review checklist
MetadataSidebarRedesignsuite blocked by@box/typesin this workspace)Running package.json scripts
nvm useAdditional notes
Suggested title:
feat(metadata-sidebar): add namespaced metadata template migration supportDo not document EUA BFF/token minting in this OSS PR. Preview-client owns the Harness split name (
metadata_namespaces_opt_in, unprefixed so@box/split-iocan prependenterprise_). Do not add that split to EUAsplits.ts.