Skip to content

feat(metadata-editor): Add externally shared property to template instance#4450

Merged
mergify[bot] merged 16 commits intomasterfrom
AddExternallySharedPropertyToTemplateInstance
Feb 24, 2026
Merged

feat(metadata-editor): Add externally shared property to template instance#4450
mergify[bot] merged 16 commits intomasterfrom
AddExternallySharedPropertyToTemplateInstance

Conversation

@JakubKida
Copy link
Contributor

@JakubKida JakubKida commented Feb 23, 2026

Description

Adds support for externally owned metadata: when a file is shared or collaborated across enterprises, some metadata instances can use a template from another enterprise. This change exposes that information so consumers can show read-only UI or tooltips (e.g. for taxonomy fields) instead of allowing edits.

Changes made

  • getTemplateForInstance() now returns { template, isExternallyOwned } instead of the template only. isExternallyOwned is true when the template was resolved from a different enterprise (e.g. via instance-scoped template fetch).
  • getTemplates() accepts an optional 4th parameter isExternallyOwned. When true, taxonomy levels are not hydrated for the returned templates (they are not editable in that context).
  • createTemplateInstance() accepts an optional 4th parameter isExternallyOwned and includes isExternallyOwned (and type) in the returned template instance.
  • getTemplateInstances() passes through isExternallyOwned from getTemplateForInstance into each created template instance.
  • MetadataTemplateInstance type and tests updated for the new shape and property.

Type of change

  • New feature (non-breaking change adding functionality)

Testing done

  • Unit tests updated and passing in Metadata.test.js for getTemplateForInstance, getEditors, getTemplateInstances, and createTemplateInstance.

Summary by CodeRabbit

  • New Features

    • Templates and template instances now include an ownership flag; externally owned templates skip local taxonomy hydration and are handled across enterprise boundaries.
  • Chores

    • Bumped dev/peer dependency versions for underlying metadata and combobox packages.
  • Tests

    • Updated tests to reflect the new template/instance data shape and ownership flag.

@JakubKida JakubKida requested review from a team as code owners February 23, 2026 10:50
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 23, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Updates dependency versions and extends metadata APIs to carry an isExternallyOwned flag through template lookup, instance creation, and taxonomy hydration so externally owned templates skip taxonomy hydration.

Changes

Cohort / File(s) Summary
Dependencies
package.json
Bumped dev/peer deps: @box/combobox-with-api^1.32.1, @box/metadata-editor^1.38.0.
Core Metadata API
src/api/Metadata.js
Added isExternallyOwned param/flag across flows: getTemplates(...) accepts it, getTemplateForInstance(...) returns { template, isExternallyOwned }, createTemplateInstance(...) accepts & sets it; taxonomy hydration is skipped when true; internal call sites updated to propagate the flag.
Types
src/common/types/metadata.js
Added optional isExternallyOwned?: boolean to MetadataTemplateInstance type.
Tests
src/api/__tests__/Metadata.test.js
Updated mocks and assertions to use { template, isExternallyOwned } returns and to expect isExternallyOwned and type fields on template instances.

Sequence Diagram

sequenceDiagram
    participant Client
    participant MetadataAPI
    participant TemplateStore
    participant HydrationService

    Client->>MetadataAPI: getTemplateForInstance(id, instance, templates)
    MetadataAPI->>TemplateStore: lookup template in provided templates
    alt found locally
        TemplateStore-->>MetadataAPI: template (isExternallyOwned: false)
    else not found
        MetadataAPI->>TemplateStore: fetch cross-enterprise templates
        TemplateStore-->>MetadataAPI: template[] (isExternallyOwned: true)
    end
    MetadataAPI-->>Client: { template, isExternallyOwned }

    Client->>MetadataAPI: createTemplateInstance(instance, template, canEdit, isExternallyOwned)
    alt isExternallyOwned == false
        MetadataAPI->>HydrationService: hydrate taxonomy levels for template
        HydrationService-->>MetadataAPI: hydrated template
    else isExternallyOwned == true
        MetadataAPI-->>MetadataAPI: skip hydration
    end
    MetadataAPI-->>Client: MetadataTemplateInstance { ..., isExternallyOwned }
Loading

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested Reviewers

  • tjuanitas
  • jpan-box

Poem

🐰 I hopped through templates with flag in paw,

Skipping hydrations I once saw,
External templates keep their tone,
Instances now carry the flag they've known,
A tiny hop — metadata grown.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main feature being added: exposing externally shared/owned property on template instances.
Description check ✅ Passed The description is comprehensive, clearly explaining the motivation, changes made to each affected method, and testing done; it exceeds the template requirements.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch AddExternallySharedPropertyToTemplateInstance

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/api/Metadata.js (1)

462-485: Return shape is always non-null now; nullable ? in the return type is stale.

getTemplateForInstance previously could return undefined/null (for the raw template), but now it always returns an object ({ template, isExternallyOwned }). The ? prefix in the return type (Line 466) is no longer accurate and could mislead callers into adding unnecessary null-checks on the wrapper object.

Also, when crossEnterpriseTemplates is empty (Line 480), crossEnterpriseTemplate is undefined yet gets wrapped as { template: undefined, isExternallyOwned: true }. Callers handle this fine (they check if (template)), but the isExternallyOwned: true on a missing template is semantically odd. Consider returning { template: undefined, isExternallyOwned: false } in that edge case, or guarding explicitly.

♻️ Suggested cleanup
     async getTemplateForInstance(
         id: string,
         instance: MetadataInstanceV2,
         templates: Array<MetadataTemplate>,
-    ): Promise<?{ template: MetadataTemplate, isExternallyOwned: boolean }> {
+    ): Promise<{ template: ?MetadataTemplate, isExternallyOwned: boolean }> {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/api/Metadata.js` around lines 462 - 485, Update getTemplateForInstance:
remove the nullable marker from the return type since the function always
returns an object, and change the cross-enterprise branch to handle an empty
crossEnterpriseTemplates result by returning { template: undefined,
isExternallyOwned: false } (or guard and only set isExternallyOwned: true when
crossEnterpriseTemplate is truthy). Specifically adjust the signature (remove
the leading ?), and in the block that sets crossEnterpriseTemplate from
crossEnterpriseTemplates[0] only mark isExternallyOwned true when
crossEnterpriseTemplate exists; otherwise return isExternallyOwned false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.vscode/settings.json:
- Around line 10-14: Remove the personal VS Code color customizations from the
shared settings by deleting the "workbench.colorCustomizations" block (including
"activityBar.background", "titleBar.activeBackground", and
"titleBar.activeForeground") from the settings JSON; if these colors are needed
for your local setup, move them into your user or workspace-specific settings
instead so they are not committed to the repository.

---

Nitpick comments:
In `@src/api/Metadata.js`:
- Around line 462-485: Update getTemplateForInstance: remove the nullable marker
from the return type since the function always returns an object, and change the
cross-enterprise branch to handle an empty crossEnterpriseTemplates result by
returning { template: undefined, isExternallyOwned: false } (or guard and only
set isExternallyOwned: true when crossEnterpriseTemplate is truthy).
Specifically adjust the signature (remove the leading ?), and in the block that
sets crossEnterpriseTemplate from crossEnterpriseTemplates[0] only mark
isExternallyOwned true when crossEnterpriseTemplate exists; otherwise return
isExternallyOwned false.

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fe26ad7 and 48476d6.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (5)
  • .vscode/settings.json
  • package.json
  • src/api/Metadata.js
  • src/api/__tests__/Metadata.test.js
  • src/common/types/metadata.js

@CLAassistant
Copy link

CLAassistant commented Feb 23, 2026

CLA assistant check
All committers have signed the CLA.

AnnaStepchenko
AnnaStepchenko previously approved these changes Feb 23, 2026
@JakubKida JakubKida changed the title Add externally shared property to template instance feat(metadata-editor): Add externally shared property to template instance Feb 23, 2026
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
src/api/Metadata.js (3)

454-466: ⚠️ Potential issue | 🟡 Minor

JSDoc @return tag is stale after the return-shape change

The return type annotation in Flow (line 466) is correct, but the prose JSDoc still says @return {Object|undefined} template for metadata instance — it should describe the new { template, isExternallyOwned } shape.

📝 Suggested doc update
- * `@return` {Object|undefined} template for metadata instance
+ * `@return` {{ template: MetadataTemplate, isExternallyOwned: boolean }|undefined} resolved template with ownership flag, or undefined if not found
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/api/Metadata.js` around lines 454 - 466, Update the JSDoc for
getTemplateForInstance to reflect the new return shape instead of
"Object|undefined": change the `@return` description to indicate it returns an
object with keys "template" (MetadataTemplate) and "isExternallyOwned"
(boolean), or null/undefined when no match is found; ensure the prose matches
the Flow return type Promise<?{ template: MetadataTemplate, isExternallyOwned:
boolean }> and briefly describe each field.

317-330: ⚠️ Potential issue | 🟡 Minor

JSDoc missing @param for the new isExternallyOwned parameter

The existing @param block documents only three parameters; the new 4th isExternallyOwned parameter is undocumented.

📝 Suggested doc update
  * `@param` {string} id - file id
  * `@param` {string} scope - metadata scope
  * `@param` {string|void} [instanceId] - metadata instance id
+ * `@param` {boolean} [isExternallyOwned] - whether the template belongs to a different enterprise; skips taxonomy hydration when true
  * `@return` {Object} array of metadata templates
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/api/Metadata.js` around lines 317 - 330, The JSDoc for getTemplates is
missing documentation for the new boolean parameter isExternallyOwned; update
the comment block for getTemplates to add a `@param` entry describing
isExternallyOwned (type boolean), its purpose (e.g., whether the templates are
for externally owned files), and its default value (false), and ensure the
`@param` order matches the function signature so tools and readers see accurate
parameter docs.

528-540: ⚠️ Potential issue | 🟡 Minor

JSDoc missing @param for isExternallyOwned

📝 Suggested doc update
  * `@param` {Object} instance - metadata instance
  * `@param` {Object} template - metadata template
  * `@param` {boolean} canEdit - can user edit item
+ * `@param` {boolean} [isExternallyOwned] - whether the template is owned by a different enterprise
  * `@return` {Object} metadata template instance
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/api/Metadata.js` around lines 528 - 540, The JSDoc for
createTemplateInstance is missing a `@param` entry for isExternallyOwned; update
the comment block above the createTemplateInstance function to add a line like
"@param {boolean} isExternallyOwned - whether the metadata is owned externally
(defaults to false)" so the parameter is documented alongside instance,
template, and canEdit.
🧹 Nitpick comments (1)
src/api/Metadata.js (1)

518-521: Redundant inner .template truthiness check (same pattern at line 614)

Per the Flow return type ?{ template: MetadataTemplate, isExternallyOwned: boolean }, result.template is always non-nullable when result itself is non-null. The result.template guard is therefore redundant.

🧹 Optional simplification (applies to line 614 as well)
- if (result && result.template) {
-     editors.push(this.createEditor(instance, result.template, canEdit));
+ if (result) {
+     editors.push(this.createEditor(instance, result.template, canEdit));
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/api/Metadata.js` around lines 518 - 521, Remove the redundant truthiness
check on result.template: since getTemplateForInstance returns ?{ template:
MetadataTemplate, isExternallyOwned: boolean }, once result is truthy its
template is non-null, so change the conditional in the block using
getTemplateForInstance (the one that builds editors with createEditor) to only
check result (e.g., if (result) editors.push(this.createEditor(instance,
result.template, canEdit))); apply the same simplification to the other
identical occurrence that currently checks result && result.template.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/api/Metadata.js`:
- Around line 454-466: Update the JSDoc for getTemplateForInstance to reflect
the new return shape instead of "Object|undefined": change the `@return`
description to indicate it returns an object with keys "template"
(MetadataTemplate) and "isExternallyOwned" (boolean), or null/undefined when no
match is found; ensure the prose matches the Flow return type Promise<?{
template: MetadataTemplate, isExternallyOwned: boolean }> and briefly describe
each field.
- Around line 317-330: The JSDoc for getTemplates is missing documentation for
the new boolean parameter isExternallyOwned; update the comment block for
getTemplates to add a `@param` entry describing isExternallyOwned (type boolean),
its purpose (e.g., whether the templates are for externally owned files), and
its default value (false), and ensure the `@param` order matches the function
signature so tools and readers see accurate parameter docs.
- Around line 528-540: The JSDoc for createTemplateInstance is missing a `@param`
entry for isExternallyOwned; update the comment block above the
createTemplateInstance function to add a line like "@param {boolean}
isExternallyOwned - whether the metadata is owned externally (defaults to
false)" so the parameter is documented alongside instance, template, and
canEdit.

---

Nitpick comments:
In `@src/api/Metadata.js`:
- Around line 518-521: Remove the redundant truthiness check on result.template:
since getTemplateForInstance returns ?{ template: MetadataTemplate,
isExternallyOwned: boolean }, once result is truthy its template is non-null, so
change the conditional in the block using getTemplateForInstance (the one that
builds editors with createEditor) to only check result (e.g., if (result)
editors.push(this.createEditor(instance, result.template, canEdit))); apply the
same simplification to the other identical occurrence that currently checks
result && result.template.

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 48476d6 and d7c98ef.

📒 Files selected for processing (2)
  • src/api/Metadata.js
  • src/api/__tests__/Metadata.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/api/tests/Metadata.test.js

@JakubKida JakubKida force-pushed the AddExternallySharedPropertyToTemplateInstance branch from d7c98ef to ce8ecfd Compare February 23, 2026 14:38
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/api/__tests__/Metadata.test.js (1)

834-842: ⚠️ Potential issue | 🟡 Minor

Missing assertion for the new isExternallyOwned argument in createTemplateInstance calls

The test verifies the first two call arguments but omits the third (canEdit) and fourth (isExternallyOwned). Since propagating isExternallyOwned from getTemplateForInstance into createTemplateInstance is the core behavior of this PR, skipping this assertion means a future regression (e.g., dropping the 4th argument) would go undetected.

✅ Proposed fix — add assertions for all four arguments
  expect(metadata.createTemplateInstance.mock.calls[0][0]).toBe(instances[0]);
  expect(metadata.createTemplateInstance.mock.calls[0][1]).toBe('template1');
+ expect(metadata.createTemplateInstance.mock.calls[0][2]).toBe(true);   // canEdit
+ expect(metadata.createTemplateInstance.mock.calls[0][3]).toBe(false);  // isExternallyOwned
  expect(metadata.createTemplateInstance.mock.calls[1][0]).toBe(instances[1]);
  expect(metadata.createTemplateInstance.mock.calls[1][1]).toBe('template2');
+ expect(metadata.createTemplateInstance.mock.calls[1][2]).toBe(true);
+ expect(metadata.createTemplateInstance.mock.calls[1][3]).toBe(false);
  expect(metadata.createTemplateInstance.mock.calls[2][0]).toBe(instances[2]);
  expect(metadata.createTemplateInstance.mock.calls[2][1]).toBe('template3');
+ expect(metadata.createTemplateInstance.mock.calls[2][2]).toBe(true);
+ expect(metadata.createTemplateInstance.mock.calls[2][3]).toBe(false);
  expect(metadata.createTemplateInstance.mock.calls[3][0]).toBe(instances[3]);
  expect(metadata.createTemplateInstance.mock.calls[3][1]).toBe('template4');
+ expect(metadata.createTemplateInstance.mock.calls[3][2]).toBe(true);
+ expect(metadata.createTemplateInstance.mock.calls[3][3]).toBe(false);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/api/__tests__/Metadata.test.js` around lines 834 - 842, The test
currently asserts only the first two args of metadata.createTemplateInstance
calls but misses the third (canEdit) and fourth (isExternallyOwned); update the
assertions for each of the four mock.calls (indices 0–3) to also verify
mock.calls[i][2] equals the expected canEdit value and mock.calls[i][3] equals
the expected isExternallyOwned value returned by getTemplateForInstance so the
test confirms propagation of isExternallyOwned into createTemplateInstance.
🧹 Nitpick comments (2)
src/api/__tests__/Metadata.test.js (2)

388-448: No test verifying that getTaxonomyLevelsForTemplates is skipped when isExternallyOwned: true

The new conditional hydration in getTemplates (the isExternallyOwned ? templates : await this.getTaxonomyLevelsForTemplates(...) branch) is exercised indirectly via getTemplateForInstance, but has no direct unit test.

✅ Proposed additional test
+ test('should skip taxonomy hydration when isExternallyOwned is true', async () => {
+     const templatesFromServer = [{ id: 1, hidden: false }];
+     metadata.getMetadataTemplateUrlForScope = jest.fn().mockReturnValueOnce('template_url');
+     metadata.getTaxonomyLevelsForTemplates = jest.fn();
+     metadata.xhr.get = jest.fn().mockReturnValueOnce({ data: { entries: templatesFromServer } });
+
+     const templates = await metadata.getTemplates('id', 'enterprise', undefined, true);
+
+     expect(templates).toEqual(templatesFromServer);
+     expect(metadata.getTaxonomyLevelsForTemplates).not.toHaveBeenCalled();
+ });
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/api/__tests__/Metadata.test.js` around lines 388 - 448, Add a unit test
for getTemplates that covers the isExternallyOwned branch: stub
metadata.getMetadataTemplateUrlForScope (or getMetadataTemplateUrlForInstance as
appropriate), mock metadata.xhr.get to return entries with a template object
that includes isExternallyOwned: true, and assert that
metadata.getTaxonomyLevelsForTemplates is NOT called and the returned templates
equal the raw entries (not hydrated). Reference getTemplates and
getTaxonomyLevelsForTemplates to locate where to add the test and use
metadata.xhr.get and
metadata.getMetadataTemplateUrlForScope/getMetadataTemplateUrlForInstance in the
setup.

175-274: Missing test coverage for isExternallyOwned: true in createTemplateInstance

Both tests only exercise the default isExternallyOwned: false path. The non-default case (explicitly passing true) is untested, leaving the new feature's primary flag uncovered at the unit level.

✅ Proposed additional test
+ test('should return Metadata Template Instance with isExternallyOwned: true', () => {
+     expect(
+         metadata.createTemplateInstance(
+             { $id: '321', $template: '', $canEdit: false, testField: 'val' },
+             { displayName: 'External', fields: [], id: 'extId', templateKey: 'extTpl', scope: 'enterprise' },
+             false,
+             true,   // isExternallyOwned
+         ),
+     ).toEqual(
+         expect.objectContaining({
+             isExternallyOwned: true,
+             canEdit: false,
+         }),
+     );
+ });
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/api/__tests__/Metadata.test.js` around lines 175 - 274, Add a unit test
covering the non-default external ownership path for createTemplateInstance:
call metadata.createTemplateInstance with the same template and instance inputs
but set the parameter that toggles external ownership to true (so the function
receives the explicit "externally owned" flag), and assert the returned object
includes isExternallyOwned: true in its top-level properties; place the new test
alongside the existing createTemplateInstance tests and mirror the existing
expectations for fields/displayName/id/scope/templateKey while only changing the
isExternallyOwned assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/api/__tests__/Metadata.test.js`:
- Around line 740-744: The mock for getTemplateForInstance incorrectly returns
an object with template: undefined for the "not-found" case; change the final
mockResolvedValueOnce to return undefined (not { template: undefined, ... }) so
it matches the real function contract (getTemplateForInstance returns undefined
when no template is found). Make the same change in the other test suite where
getTemplateInstances is mocked to ensure both mocks return undefined for the
not-found case.

---

Outside diff comments:
In `@src/api/__tests__/Metadata.test.js`:
- Around line 834-842: The test currently asserts only the first two args of
metadata.createTemplateInstance calls but misses the third (canEdit) and fourth
(isExternallyOwned); update the assertions for each of the four mock.calls
(indices 0–3) to also verify mock.calls[i][2] equals the expected canEdit value
and mock.calls[i][3] equals the expected isExternallyOwned value returned by
getTemplateForInstance so the test confirms propagation of isExternallyOwned
into createTemplateInstance.

---

Nitpick comments:
In `@src/api/__tests__/Metadata.test.js`:
- Around line 388-448: Add a unit test for getTemplates that covers the
isExternallyOwned branch: stub metadata.getMetadataTemplateUrlForScope (or
getMetadataTemplateUrlForInstance as appropriate), mock metadata.xhr.get to
return entries with a template object that includes isExternallyOwned: true, and
assert that metadata.getTaxonomyLevelsForTemplates is NOT called and the
returned templates equal the raw entries (not hydrated). Reference getTemplates
and getTaxonomyLevelsForTemplates to locate where to add the test and use
metadata.xhr.get and
metadata.getMetadataTemplateUrlForScope/getMetadataTemplateUrlForInstance in the
setup.
- Around line 175-274: Add a unit test covering the non-default external
ownership path for createTemplateInstance: call metadata.createTemplateInstance
with the same template and instance inputs but set the parameter that toggles
external ownership to true (so the function receives the explicit "externally
owned" flag), and assert the returned object includes isExternallyOwned: true in
its top-level properties; place the new test alongside the existing
createTemplateInstance tests and mirror the existing expectations for
fields/displayName/id/scope/templateKey while only changing the
isExternallyOwned assertion.

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d7c98ef and ce8ecfd.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (4)
  • package.json
  • src/api/Metadata.js
  • src/api/__tests__/Metadata.test.js
  • src/common/types/metadata.js
🚧 Files skipped from review as they are similar to previous changes (2)
  • package.json
  • src/common/types/metadata.js

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
src/api/Metadata.js (2)

540-582: Consider overriding canEdit to false when isExternallyOwned is true.

The current logic allows a returned instance to have { canEdit: true, isExternallyOwned: true } simultaneously. Every downstream consumer must then check both flags to determine editability. Forcing canEdit: false here removes the ambiguity and provides defence-in-depth.

♻️ Suggested change
     return {
         isExternallyOwned,
-        canEdit: instance.$canEdit && canEdit,
+        canEdit: !isExternallyOwned && instance.$canEdit && canEdit,
         displayName: template.displayName,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/api/Metadata.js` around lines 540 - 582, The returned object can
currently have canEdit true while isExternallyOwned is true; modify the return
construction so that canEdit is forced to false when the isExternallyOwned
parameter is true (e.g., set canEdit: isExternallyOwned ? false :
(instance.$canEdit && canEdit)) so downstream code need only check canEdit;
update the return object in the function that builds the
MetadataTemplateInstance (the block that currently returns { isExternallyOwned,
canEdit: instance.$canEdit && canEdit, ... }) to implement this override.

614-618: isExternallyOwned propagation is correct.

result.isExternallyOwned is cleanly threaded into createTemplateInstance. Same minor redundancy as in getEditors: result.template check inside if (result) is unnecessary since template is non-optional in the return type (mirror of the `` comment above).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/api/Metadata.js` around lines 614 - 618, The conditional redundantly
checks result.template even though getTemplateForInstance's return type
guarantees a non-optional template; update the call site in Metadata.js (around
getTemplateForInstance and createTemplateInstance usage) to only check for
result (i.e., remove the "&& result.template" part) and directly use
result.template when calling createTemplateInstance; apply the same cleanup
pattern where you saw a similar redundant template presence check in getEditors
so the code relies on the non-optional return contract instead of
double-checking template.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/api/Metadata.js`:
- Around line 477-485: Add a defensive check for instanceId (derived from
instance.$id) before making the cross-enterprise fetch: if instanceId is falsy,
skip calling getTemplates(id, scope, instanceId, true) (and return undefined or
continue without marking externally owned) instead of invoking
getMetadataTemplateUrlForScope via getTemplates with a missing instanceId;
specifically, guard the block that calls getTemplates and only perform the
cross-enterprise lookup when instanceId is present so
getMetadataTemplateUrlForScope(scope) is not accidentally used for a foreign
enterprise.
- Around line 519-521: In the getEditors method, remove the redundant inner
check of result.template since getTemplateForInstance returns a truthy result
with a non-optional template; after awaiting getTemplateForInstance(id,
instance, templates) simply check if (result) and then call
editors.push(this.createEditor(instance, result.template, canEdit)); ensure you
do not add or change handling of isExternallyOwned (the legacy path
intentionally omits it) and keep references to getTemplateForInstance and
createEditor unchanged.

---

Nitpick comments:
In `@src/api/Metadata.js`:
- Around line 540-582: The returned object can currently have canEdit true while
isExternallyOwned is true; modify the return construction so that canEdit is
forced to false when the isExternallyOwned parameter is true (e.g., set canEdit:
isExternallyOwned ? false : (instance.$canEdit && canEdit)) so downstream code
need only check canEdit; update the return object in the function that builds
the MetadataTemplateInstance (the block that currently returns {
isExternallyOwned, canEdit: instance.$canEdit && canEdit, ... }) to implement
this override.
- Around line 614-618: The conditional redundantly checks result.template even
though getTemplateForInstance's return type guarantees a non-optional template;
update the call site in Metadata.js (around getTemplateForInstance and
createTemplateInstance usage) to only check for result (i.e., remove the "&&
result.template" part) and directly use result.template when calling
createTemplateInstance; apply the same cleanup pattern where you saw a similar
redundant template presence check in getEditors so the code relies on the
non-optional return contract instead of double-checking template.

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ce8ecfd and 2fec132.

📒 Files selected for processing (1)
  • src/api/Metadata.js

jfox-box
jfox-box previously approved these changes Feb 23, 2026
Copy link
Contributor

@jfox-box jfox-box left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but some minor comments.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/api/Metadata.js`:
- Around line 467-483: The code can return { template: undefined,
isExternallyOwned: true } when getTemplates(id, scope, instanceId, true) yields
an empty array; update the branch that handles cross-enterprise lookup to check
crossEnterpriseTemplates.length (or crossEnterpriseTemplate truthiness) and
return undefined if no template was found, otherwise return { template:
crossEnterpriseTemplate, isExternallyOwned: true } so callers never receive an
object with template === undefined; reference the variables getTemplates,
crossEnterpriseTemplates, crossEnterpriseTemplate, template and
isExternallyOwned when making the guard.

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2fec132 and d0579f0.

📒 Files selected for processing (2)
  • src/api/Metadata.js
  • src/api/__tests__/Metadata.test.js

@JakubKida JakubKida force-pushed the AddExternallySharedPropertyToTemplateInstance branch from 9e00d70 to c99ed79 Compare February 24, 2026 11:05
Copy link
Contributor

@jfox-box jfox-box left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@JakubKida JakubKida force-pushed the AddExternallySharedPropertyToTemplateInstance branch from 666433a to ca44dd4 Compare February 24, 2026 22:36
@JakubKida JakubKida force-pushed the AddExternallySharedPropertyToTemplateInstance branch from ca44dd4 to d973747 Compare February 24, 2026 22:46
@mergify mergify bot added the queued label Feb 24, 2026
@mergify mergify bot merged commit a386199 into master Feb 24, 2026
11 checks passed
@mergify mergify bot deleted the AddExternallySharedPropertyToTemplateInstance branch February 24, 2026 23:15
@mergify
Copy link
Contributor

mergify bot commented Feb 24, 2026

Merge Queue Status

Rule: Automatic strict merge


  • Entered queue2026-02-24 23:15 UTC
  • Checks passed · in-place
  • Merged2026-02-24 23:15 UTC · at d973747f9f4aff4b7b722a40a3ada7e07cb8273b

This pull request spent 6 seconds in the queue, with no time running CI.

Required conditions to merge

@mergify mergify bot removed the queued label Feb 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants