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 |
JakubKida
force-pushed
the
metadata-namespace-migration-support
branch
from
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
JakubKida
force-pushed
the
metadata-namespace-migration-support
branch
from
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.
| /** | ||
| * Extracts the full enterprise scope FQN (e.g. `enterprise_123456`) from a list | ||
| * of metadata instances. Used in MIGRATION/FINAL mode where the `'enterprise'` | ||
| * shorthand is no longer a valid template-fetch scope. | ||
| * | ||
| * Checks `$scope` first (populated in MIGRATION mode) and falls back to the | ||
| * leading `enterprise_XXXXX` segment of `$namespace` (used in FINAL mode). | ||
| * | ||
| * @param {Array<MetadataInstanceV2>} instances - raw metadata instances | ||
| * @return {string|null} full enterprise FQN or null when none found | ||
| */ | ||
| getEnterpriseScopeFromInstances(instances: Array<MetadataInstanceV2>): string | null { | ||
| for (const inst of instances) { | ||
| if (inst.$scope?.startsWith(METADATA_SCOPE_ENTERPRISE)) { | ||
| return inst.$scope; | ||
| } | ||
| if (inst.$namespace?.startsWith(METADATA_SCOPE_ENTERPRISE)) { | ||
| // Namespace FQNs may be "enterprise_123/key" — take the leading segment. | ||
| return inst.$namespace.split('/')[0]; | ||
| } | ||
| } | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files ==\n'
git ls-files 'src/api/Metadata.js' 'src/**/metadataNamespaceMocks.js' 'src/**/constants.js' 'src/**/MetadataSidebarRedesign.tsx' | sed 's#^`#-` #'
printf '\n== Search namespace delimiter usage ==\n'
rg -n "split\\('/'\\)|split\\('\\.'\\)|METADATA_SCOPE_ENTERPRISE|METADATA_NAMESPACE_GLOBAL|enterpriseScope|enterpriseId|mockListNamespaces|namespaceFqn" src --glob '!**/node_modules/**'
printf '\n== Metadata.js around getEnterpriseScopeFromInstances ==\n'
sed -n '1015,1070p' src/api/Metadata.js
printf '\n== Metadata.js around getMetadata enterprise templates ==\n'
sed -n '1090,1160p' src/api/Metadata.js
printf '\n== metadataNamespaceMocks.js ==\n'
file=$(git ls-files 'src/**/metadataNamespaceMocks.js' | head -n 1)
[ -n "${file:-}" ] && sed -n '1,220p' "$file"
printf '\n== constants.js ==\n'
file=$(git ls-files 'src/**/constants.js' | head -n 1)
[ -n "${file:-}" ] && sed -n '1,220p' "$file"
printf '\n== MetadataSidebarRedesign.tsx excerpts ==\n'
file=$(git ls-files 'src/**/MetadataSidebarRedesign.tsx' | head -n 1)
[ -n "${file:-}" ] && sed -n '1,260p' "$file"Repository: box/box-ui-elements
Length of output: 40404
Use the dot-delimited namespace prefix here — split('/') won’t trim names like enterprise_123456.legal, so enterpriseScope can be wrong when passed to getTemplates(...).
🧰 Tools
🪛 Biome (2.5.3)
[error] 1043-1043: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
(parse)
[error] 1043-1043: return 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 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.
| }); | ||
| templates[idx] = updated; | ||
| } | ||
| successCallback({ type: 'metadata_template', namespace: namespaceFqn, templateKey }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Return the fully updated template on success.
The mockUpdateMetadataTemplate function currently returns a stub object to the successCallback. The real Metadata API returns the fully updated template upon a successful update. If the UI relies on the callback payload to refresh its state, returning a stub could result in missing properties (e.g., id, displayName, fields) and cause UI inconsistencies or errors.
🐛 Proposed fix
- successCallback({ type: 'metadata_template', namespace: namespaceFqn, templateKey });
+ successCallback(idx !== -1 ? templates[idx] : { type: 'metadata_template', namespace: namespaceFqn, templateKey });📝 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.
| successCallback({ type: 'metadata_template', namespace: namespaceFqn, templateKey }); | |
| successCallback(idx !== -1 ? templates[idx] : { type: 'metadata_template', namespace: namespaceFqn, templateKey }); |
🤖 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/metadataNamespaceMocks.js` at 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.
| const templateInstance = templates.find( | ||
| template => template.templateKey === templateKey && (template.scope || template.namespace), | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Match the scope parameter exactly.
The condition (template.scope || template.namespace) only checks for the presence of a scope or namespace (truthiness) and completely ignores the scope parameter passed into the extractSuggestions function. This causes the logic to blindly match the first template with the correct templateKey, even if it belongs to a completely different scope.
Compare the template's properties against the scope parameter directly.
🐛 Proposed fix
- const templateInstance = templates.find(
- template => template.templateKey === templateKey && (template.scope || template.namespace),
- );
+ const templateInstance = templates.find(
+ template => template.templateKey === templateKey && (template.scope === scope || template.namespace === scope),
+ );📝 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.
| const templateInstance = templates.find( | |
| template => template.templateKey === templateKey && (template.scope || template.namespace), | |
| ); | |
| const templateInstance = templates.find( | |
| template => template.templateKey === templateKey && (template.scope === scope || template.namespace === scope), | |
| ); |
🤖 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/hooks/useSidebarMetadataFetcher.ts` around lines
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.
| // Real enterprise FQN (e.g. "enterprise_123456") derived from the already-loaded templates. | ||
| // The numeric ID (without the "enterprise_" prefix) is used for the enterprise configurations API. | ||
| const enterpriseId = templates.find(t => t.scope?.startsWith(`${METADATA_SCOPE_ENTERPRISE}_`))?.scope; | ||
| const enterpriseNumericId = enterpriseId?.slice(METADATA_SCOPE_ENTERPRISE.length + 1); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Possible crash: templates.find(...) called without a null-check.
templates is treated as possibly falsy elsewhere in this same file (isViewMode at Line 402 and metadataDropdown at Line 410 both guard with templates &&, and Line 198 passes templates ?? []). Here, templates.find(...) is called directly with no guard — if templates is undefined (e.g. before the initial fetch resolves), this throws a TypeError and breaks the component render.
🐛 Proposed fix
- const enterpriseId = templates.find(t => t.scope?.startsWith(`${METADATA_SCOPE_ENTERPRISE}_`))?.scope;
+ const enterpriseId = templates?.find(t => t.scope?.startsWith(`${METADATA_SCOPE_ENTERPRISE}_`))?.scope;📝 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.
| // Real enterprise FQN (e.g. "enterprise_123456") derived from the already-loaded templates. | |
| // The numeric ID (without the "enterprise_" prefix) is used for the enterprise configurations API. | |
| const enterpriseId = templates.find(t => t.scope?.startsWith(`${METADATA_SCOPE_ENTERPRISE}_`))?.scope; | |
| const enterpriseNumericId = enterpriseId?.slice(METADATA_SCOPE_ENTERPRISE.length + 1); | |
| // Real enterprise FQN (e.g. "enterprise_123456") derived from the already-loaded templates. | |
| // The numeric ID (without the "enterprise_" prefix) is used for the enterprise configurations API. | |
| const enterpriseId = templates?.find(t => t.scope?.startsWith(`${METADATA_SCOPE_ENTERPRISE}_`))?.scope; | |
| const enterpriseNumericId = enterpriseId?.slice(METADATA_SCOPE_ENTERPRISE.length + 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/elements/content-sidebar/MetadataSidebarRedesign.tsx` around lines 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.
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.
| // handleEditTemplateById alongside this file. | ||
| // ───────────────────────────────────────────────────────────────────────────── | ||
|
|
||
| export const IS_NAMESPACE_API_MOCKED: boolean = true; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '\n## Files with IS_NAMESPACE_API_MOCKED\n'
rg -n "IS_NAMESPACE_API_MOCKED" -S .
printf '\n## metadataNamespaceMocks.js\n'
sed -n '1,220p' src/api/metadataNamespaceMocks.js
printf '\n## metadata usage references\n'
rg -n "metadataNamespaceMocks|namespace.*mock|IS_NAMESPACE_API_MOCKED|listNamespaces|createNamespace|updateNamespace" src -SRepository: box/box-ui-elements
Length of output: 11297
🏁 Script executed:
set -euo pipefail
printf '\n## Metadata.js around namespace methods\n'
sed -n '280,520p' src/api/Metadata.js
printf '\n## Environment / build gating in Metadata.js and metadataNamespaceMocks.js\n'
rg -n "NODE_ENV|process\.env|__DEV__|dev|test|mock" src/api/Metadata.js src/api/metadataNamespaceMocks.js -S
printf '\n## Callers of Metadata namespace APIs\n'
rg -n "\.listNamespaces\(|\.listTemplatesForNamespace\(|\.createMetadataTemplate\(|\.updateMetadataTemplate\(|\.getTemplateSchemaForEditor\(" src -SRepository: box/box-ui-elements
Length of output: 13518
Gate namespace mocks behind dev/test config src/api/metadataNamespaceMocks.js:16 leaves IS_NAMESPACE_API_MOCKED hardcoded to true, so namespace list/create/update flows stay on the in-memory mock store and never reach the backend. Default this to the live API and enable the mocks only in development or test.
🧰 Tools
🪛 Biome (2.5.3)
[error] 16-16: 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/metadataNamespaceMocks.js` at 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.
| const cancelledRef = useRef(false); | ||
|
|
||
| useEffect(() => { | ||
| if (!isEnabled || !file || !enterpriseNumericId) { | ||
| setMode(null); | ||
| setIsLoading(false); | ||
| return undefined; | ||
| } | ||
|
|
||
| cancelledRef.current = false; | ||
| setIsLoading(true); | ||
|
|
||
| const metadataAPI = api.getMetadataAPI(false); | ||
| metadataAPI.getMetadataNamespaceMode(file, enterpriseNumericId).then(resolvedMode => { | ||
| if (!cancelledRef.current) { | ||
| setMode(resolvedMode); | ||
| setIsLoading(false); | ||
| } | ||
| }); | ||
|
|
||
| return () => { | ||
| cancelledRef.current = true; | ||
| }; | ||
| }, [api, file, enterpriseNumericId, isEnabled]); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Fix race condition and type mismatch in effect.
Using useRef for cancellation in useEffect creates a race condition. When dependencies change, the cleanup of the previous effect runs (cancelledRef.current = true), followed immediately by the new effect (cancelledRef.current = false). If the old fetch resolves after the new effect starts, it reads false from the shared ref and updates state with stale data. Use a local variable inside the effect instead.
Additionally, since getMetadataNamespaceMode returns Promise<string | null>, resolvedMode must be cast to MetadataScopeMode | null to prevent a TypeScript compilation error.
🔒️ Proposed fix using a local variable and type cast
- const cancelledRef = useRef(false);
-
useEffect(() => {
if (!isEnabled || !file || !enterpriseNumericId) {
setMode(null);
setIsLoading(false);
return undefined;
}
- cancelledRef.current = false;
+ let isCancelled = false;
setIsLoading(true);
const metadataAPI = api.getMetadataAPI(false);
metadataAPI.getMetadataNamespaceMode(file, enterpriseNumericId).then(resolvedMode => {
- if (!cancelledRef.current) {
- setMode(resolvedMode);
+ if (!isCancelled) {
+ setMode(resolvedMode as MetadataScopeMode | null);
setIsLoading(false);
}
});
return () => {
- cancelledRef.current = true;
+ isCancelled = true;
};
}, [api, file, enterpriseNumericId, isEnabled]);📝 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.
| const cancelledRef = useRef(false); | |
| useEffect(() => { | |
| if (!isEnabled || !file || !enterpriseNumericId) { | |
| setMode(null); | |
| setIsLoading(false); | |
| return undefined; | |
| } | |
| cancelledRef.current = false; | |
| setIsLoading(true); | |
| const metadataAPI = api.getMetadataAPI(false); | |
| metadataAPI.getMetadataNamespaceMode(file, enterpriseNumericId).then(resolvedMode => { | |
| if (!cancelledRef.current) { | |
| setMode(resolvedMode); | |
| setIsLoading(false); | |
| } | |
| }); | |
| return () => { | |
| cancelledRef.current = true; | |
| }; | |
| }, [api, file, enterpriseNumericId, isEnabled]); | |
| useEffect(() => { | |
| if (!isEnabled || !file || !enterpriseNumericId) { | |
| setMode(null); | |
| setIsLoading(false); | |
| return undefined; | |
| } | |
| let isCancelled = false; | |
| setIsLoading(true); | |
| const metadataAPI = api.getMetadataAPI(false); | |
| metadataAPI.getMetadataNamespaceMode(file, enterpriseNumericId).then(resolvedMode => { | |
| if (!isCancelled) { | |
| setMode(resolvedMode as MetadataScopeMode | null); | |
| setIsLoading(false); | |
| } | |
| }); | |
| return () => { | |
| isCancelled = true; | |
| }; | |
| }, [api, file, enterpriseNumericId, isEnabled]); |
🤖 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/hooks/useMetadataNamespaceMode.ts` around lines
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.
2 participants
Summary by CodeRabbit