Skip to content

feat(edit-content): auto-populate Site/Folder field when relating content#35154

Draft
oidacra wants to merge 3 commits intomainfrom
issue-33529-feature-auto-populate-site-folder-field-when-relat
Draft

feat(edit-content): auto-populate Site/Folder field when relating content#35154
oidacra wants to merge 3 commits intomainfrom
issue-33529-feature-auto-populate-site-folder-field-when-relat

Conversation

@oidacra
Copy link
Copy Markdown
Member

@oidacra oidacra commented Mar 30, 2026

Summary

  • Auto-populate the Site/Folder filter in the "Add Related Content" relationship dialog with the current site from GlobalStore
  • Follows the established pattern used in dot-tags-create component — reads GlobalStore.siteDetails() synchronously, no new HTTP requests
  • Users can still manually change or clear the pre-selected site

Closes #33529

Changes

File Change
search.component.ts Inject GlobalStore, pre-populate siteOrFolderId form field in ngAfterViewInit, trigger initial filtered search
site-field.component.ts Inject GlobalStore, enhance writeValue() to handle programmatic site:{id} values
site-field.store.ts Add setInitialSelection(id, type, label) method to create synthetic TreeNodeItem
search.component.spec.ts Add tests for site pre-population (with site / without site)
site-field.component.spec.ts Add tests for writeValue with site and folder values
site-field.store.spec.ts Add tests for setInitialSelection method

Acceptance Criteria

  • AC1: Site/Folder tree-select is pre-populated with the current site from GlobalStore when opening "Add Related Content"
  • AC2: Initial content search is filtered by the pre-selected site
  • AC3: Pre-selected site appears as an active filter chip
  • AC4: User can clear the pre-selected site filter
  • AC5: User can change the pre-selected site to a different site or folder
  • AC6: If GlobalStore has no current site, field remains empty (graceful fallback)
  • AC7: No new HTTP requests — reads from existing GlobalStore signal state

Test Plan

  • Open any content in the new Edit Content experience
  • Go to the Relationships tab and click "Add Related Content"
  • Verify the Site/Folder field is pre-populated with the current site (from the top-bar site selector)
  • Verify the initial search results are filtered by that site
  • Verify you can clear the site filter and search across all sites
  • Verify you can change to a different site/folder
  • Switch the global site selector to a different site, open the dialog again, and verify it picks up the new site

Visual Changes

Screenshots/GIFs needed: This PR includes frontend changes. Please attach visual evidence before marking as ready for review.

  • Before screenshot
  • After screenshot
  • GIF of interaction (if behavioral change)

…tent #33529

Read the current site from GlobalStore and pre-populate the Site/Folder
filter in the "Add Related Content" dialog. This matches the behavior
of the Content Search screen and removes the need for manual site selection.

Closes #33529
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Mar 30, 2026

Claude finished @oidacra's task in 2m 21s —— View job


Code Review (updated)

  • Read changed files
  • Analyze issues from previous review
  • Post updated review

Issue 1 is fixedsiteControl.setValue(this.store.nodeSelected() ...) is now called at site-field.component.ts:133. The TreeSelect will visually reflect the pre-selection and getSiteDisplayLabel has a working fallback via nodeSelected() even if the cast is a bit loose. ✓

Issue 2 is still present — onChange from writeValue marks the form dirty

site-field.component.ts:72-76

effect(() => {
    const valueToSave = this.store.valueToSave();
    this.onChange(valueToSave || '');
});

writeValue('site:xxx')setInitialSelection → patches nodeSelectedvalueToSave becomes truthy → effect fires → onChange is called. Calling onChange from the writeValue path tells Angular the user changed the value, marking the control dirty. The relationship form will appear modified before any user interaction, which can trigger unsolicited "unsaved changes" prompts or dirty-check guards. The interactive path (chooseNode) correctly goes through the same effect — the write path should not.

Fix this →


Misleading test at site-field.component.spec.ts:261

it('should write value to form control', () => {
    spectator.detectChanges();
    component.writeValue(testValue);  // testValue = 'test-site-id' — no colon
    expect(component.siteControl.value).toBe('');
});

testValue = 'test-site-id' has no colon, so it skips the value.includes(':') branch entirely and hits no code path. The test passes trivially and gives false coverage confidence for the fix. A test calling writeValue('site:123') and asserting siteControl.value is a non-empty TreeNodeItem (with the correct label) is needed to actually validate Issue 1's fix.


AC2 unverified — no auto-search on init

ngAfterViewInit patches the form value but doesn't call doSearch(). The search test at line 929 explicitly asserts expect(searchSpy).not.toHaveBeenCalled(). AC2 says "Initial content search is filtered by the pre-selected site". If the product expectation is that results appear pre-filtered when the dialog opens, this needs doSearch() after the patch. If pre-filling the form and letting the user click Search is acceptable, AC2 should be updated.


Minor — split(':') without limit (unchanged)

search.component.ts:278 and site-field.component.ts:126 still use value.split(':') with no limit. split(':', 2) is safer if the format ever carries colons in the ID portion.


Summary: The blocking visual-selection bug is fixed. The form-dirty side effect from writeValue is the main remaining correctness issue; the misleading test masks it. AC2 needs a product decision.

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Mar 30, 2026

Claude finished @oidacra's task in 2m 35s —— View job


Code Review

  • Gather context
  • Read changed files
  • Analyze and post review

Issue 1 — writeValue doesn't update siteControl, breaking visual selection and chip label

site-field.component.ts:117-133

writeValue(value: string): void {
    if (!value) {
        this.siteControl.setValue('');
        this.store.clearSelection();
        return;
    }

    if (value.includes(':')) {
        const [type, id] = value.split(':');
        if (id && (type === 'site' || type === 'folder')) {
            const siteDetails = this.#globalStore.siteDetails();
            const label = type === 'site' && siteDetails?.identifier === id ? siteDetails.hostname : id;
            this.store.setInitialSelection(id, type, label);
            // ← siteControl is never updated here
        }
    }
}

siteControl is bound to the PrimeNG TreeSelect and drives both the visual selection in the dropdown and the label lookup in getSiteDisplayLabel. setInitialSelection only patches store.nodeSelected, but the TreeSelect never gets told about it. Result:

  1. The pre-selected node won't visually appear highlighted in the tree.
  2. getSiteDisplayLabel reads this.$siteField()?.siteControl?.value as TreeNodeItem — since siteControl stays '', this falls back to showing the raw UUID in the chip instead of the hostname.

The test 'should write value to form control' even asserts expect(component.siteControl.value).toBe('') after calling writeValue, confirming this is never set. siteControl.setValue(nodeObject) needs to be called with the synthetic node that setInitialSelection creates.

Fix this →


Issue 2 — onChange fired from inside writeValue marks form as dirty

site-field.component.ts:72-76

effect(() => {
    const valueToSave = this.store.valueToSave();
    this.onChange(valueToSave || '');
});

This effect runs whenever valueToSave changes — including when setInitialSelection is called from writeValue. Calling onChange from within a CVA's writeValue flow tells Angular the user changed the value, which marks the control dirty. The relationship form will appear modified before the user has touched anything. The existing interactive path (user picks a node → chooseNode → effect fires → onChange) is correct. The writeValue path should suppress this by either checking whether the call originated from writeValue (flag) or by calling siteControl.setValue(..., { emitEvent: false }) and not routing through the same effect.


Issue 3 — GlobalStore injected in both SearchComponent and SiteFieldComponent for the same data

SearchComponent.ngAfterViewInit already has siteDetails and could pass it (or at least the hostname) as additional context. Instead, SiteFieldComponent.writeValue independently re-reads GlobalStore.siteDetails() purely to recover the hostname. If the two components ever differ in timing or injection context this would produce inconsistent labels. The redundancy is low-risk here since GlobalStore is a singleton signal, but the design creates hidden coupling between the parent's pre-population logic and the child's label resolution.


Issue 4 — Missing test coverage for chip label fallback

There is no test verifying that the chip label shows demo.dotcms.com rather than the raw UUID after ngAfterViewInit pre-populates the site. The search.component.spec.ts tests for getSiteDisplayLabel mock $siteField().siteControl.value, but no test exercises the end-to-end flow: GlobalStore has a site → ngAfterViewInit patches form → doSearch() is called → $activeFilters() returns a chip with hostname. Given Issue 1, this flow currently breaks silently.


Minor — split(':') with no limit is fragile

search.component.ts:279

const [type, id] = siteOrFolderId.split(':');

dotCMS identifiers are UUIDs (hyphens only), so this is safe today. But if the format ever carries a colon in the ID portion, id would be truncated. split(':', 2) would destructure correctly and is more defensive.


Summary: The implementation is mostly sound — correct store pattern, clean signal usage, no HTTP requests added. The blocking problem is that writeValue sets the store but not siteControl, which means the TreeSelect won't visually show the pre-selection and chips will display raw UUIDs. The onChange-in-writeValue side effect is a subtler correctness issue. Everything else is minor.

oidacra added 2 commits March 30, 2026 12:06
…arch

- Set siteControl to TreeNodeItem in writeValue so PrimeNG TreeSelect
  displays the pre-selected site correctly
- Add store.nodeSelected fallback in getSiteDisplayLabel to show hostname
  instead of raw UUID in filter chips
- Remove doSearch() from ngAfterViewInit to avoid race condition with
  initLoad that was causing double data loading
…ch preselection

When the content type has a Host/Folder field, use the contentlet's
host/folder value to pre-populate the relationship dialog's Site/Folder
filter instead of the GlobalStore current site. Supports both site and
folder preselection. Falls back to GlobalStore when no HOST_FOLDER field.

Routes data through ExistingContentStore instead of DynamicDialogConfig
since template components (HeaderComponent) don't inherit the dialog's
custom injector.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : Frontend PR changes Angular/TypeScript frontend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

[FEATURE] Auto-populate Site/Folder field when relating content

1 participant