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.
6ad30e2 to
4673846
Compare
4 participants
Description
The metadata sidebar assumed every template and instance is addressed by a legacy
scopestring. Box metadata is moving to hierarchical namespaces, and during that transition a
template can arrive with a
scope, a namespace FQN, or both. The sidebar had no way torepresent that, so namespaced templates could be mismatched, cached under a colliding key,
or dropped entirely.
This PR makes the redesigned metadata sidebar namespace-aware and adds opt-in template
management (browse namespaces, create and edit templates) on top of it. The sidebar
resolves one of three modes —
SCOPED(today's behaviour),MIGRATION(scope andnamespace both present),
FINAL(namespace only) — which the host supplies, together withthe enterprise id, via new props. Nothing changes for existing consumers: with the opt-in
feature flag off, host mode and enterprise id are ignored and the sidebar behaves exactly
as before.
Screenshots/Videos
Recording of the template browser dropdown and the create/edit template modal to be added.
Related issues
Tracked in Box's internal issue tracker.
Changes made
API layer
src/api/MetadataNamespaces.js, a collaborator that owns namespace-scoped requests:listing child namespaces, listing templates for a namespace, creating a template,
applying a JSON-patch update, and fetching a template schema for the editor.
Metadata.jsre-exports these so callers keep a single entry point.
src/api/metadataNamespaceUtils.jswith pure helpers (no I/O): resolving the URLpath segment for a scope or namespace given the current mode, deriving the enterprise root
FQN from a list of instances, and deciding whether a template that is missing locally is
externally owned.
hardcoding the
enterpriseshorthand. This fixes cache collisions between distinctnamespaces that share a
templateKey.resolves to empty levels so the rest of the metadata still renders.
Types and constants
namespaceis now an optional field on template, instance, and template-instance types,and
scopebecame optional for namespace-only records.taxonomy-list operations.
Sidebar
useMetadataNamespaceContextresolves the effective mode and enterprise FQN from theopt-in flag plus host props, and exposes a loading state for when the host has not
resolved its configuration yet. The sidebar does not fetch enterprise configuration or
the current user itself — hosts own those values.
useMetadataTemplateItemsServiceprovides the data service backing the template browser(paginated namespace navigation, per-namespace template lists, client-side search over
already-loaded templates).
useMetadataTemplateEventServicebridges the browser's template shape back to the editorshape on selection, so downstream sidebar code keeps working unchanged.
useMetadataTemplateEditorowns the create/edit modal lifecycle and hands API-readypayloads back to the sidebar, which performs the network calls.
MetadataTemplateDropdownselects between the legacy static template dropdown and thebrowser-backed dropdown based on whether template management is enabled.
metadataTemplateIdentitycentralises template comparison. Two records match ontemplateKeyplus a resolved FQN, and two missing FQNs never match — previously thatwould collapse distinct child-namespace templates sharing a
templateKey.metadataTaxonomyFetchergained a taxonomy catalogue fetcher and a singletaxonomy-by-key resolver, which the template editor needs to hydrate the display name and
levels of taxonomy fields on an existing template.
Public props
MetadataSidebarandMetadataSidebarRedesignacceptmetadataNamespaceMode(
SCOPED|MIGRATION|FINAL|null) andenterpriseId(numeric, numeric string,or
enterprise_<id>FQN). Both are ignored unless the namespaces opt-in flag is on.Type of change
Testing done
New unit suites cover the namespace API collaborator, the pure namespace helpers, the
template identity comparison, the taxonomy fetchers, and each of the new sidebar hooks.
Existing metadata sidebar and taxonomy-picker suites were updated for the new prop and
fetcher signatures. Manual verification was done in Storybook in every mode, both against
the local namespace mocks and against live API responses.
Dependencies
@box/metadata-template-editor(dev + peer).@box/metadata-editor,@box/metadata-taxonomy-picker, and@box/metadata-template-browser.@dnd-kit/core,@dnd-kit/sortable,@dnd-kit/utilities, andyup(dev + peer) —required by the template editor's drag-to-reorder field list and form validation.
@box/metadata-template-editorwas added to the i18n build config and the JesttransformIgnorePatternsallowlist.Deployment notes
No migration steps. Everything new is gated behind the
metadata.namespacesOptIn.enabledfeature flag; with it off, the sidebar keeps the current scope-based behaviour and the new
props are ignored. Namespace requests currently run against local mocks
(
IS_NAMESPACE_API_MOCKEDinsrc/api/metadataNamespaceMocks.js) so the UI can bedeveloped and demoed ahead of the backend rollout — flip that flag to
falseto exercisethe real endpoints.
How to test
nvm use && yarn install && yarn startto open Storybook.confirm the sidebar is unchanged: same template dropdown, same applied instances.
metadataNamespaceMode="MIGRATION"plus anenterpriseId. The template dropdown should switch to the namespace browser — drillinto child namespaces, select a template, and confirm it applies as before.
template. Confirm the editor modal opens in the right mode, that an existing taxonomy
field shows its display name and levels, and that saving refreshes the sidebar.
metadataNamespaceMode="FINAL"and repeat step 3 to confirm namespace-onlytemplates resolve.
metadataNamespaceMode={null}to confirm the loading state does not flash thelegacy dropdown.
How to review
Suggested order:
src/api/metadataNamespaceUtils.jsandsrc/elements/content-sidebar/utils/metadataTemplateIdentity.ts— the two pure modulesthat encode the migration rules everything else depends on. The comparison rules in
isSameMetadataTemplateare the subtlest part of the PR and deserve the closest look.src/api/MetadataNamespaces.jsand theMetadata.jsdiff — check the scope/namespaceresolution and the schema cache key change for backwards compatibility in
SCOPEDmode.useMetadataNamespaceContext— the gating logic. Worth confirming the flag-off path isgenuinely inert.
MetadataTemplateDropdown— mostly wiring.MetadataSidebarRedesign.tsx— the integration point.Self-review checklist
Additional notes
Suggested title:
feat(metadata-sidebar): add namespace support and template management.The 28 commits on this branch are incremental; a squash merge keeps history readable.