feat(edit-content): add page references dialog in general tab#35442
feat(edit-content): add page references dialog in general tab#35442adrianjm-dotCMS wants to merge 9 commits intomainfrom
Conversation
### Summary of Changes - Updated the component to conditionally display references based on the presence of references. - Introduced a new computed property to determine if references exist. - Added a dialog for viewing references, improving user interaction. - Updated the service to fetch content references and added a new model to represent page references. ### Localization - Added new keys to for reference dialog titles and columns. This update improves the user experience by providing clear visibility of content references and enhancing the overall functionality of the sidebar component.
The REST resource read the reference map with key "personaName", but the service layer stores the value under "persona". As a result the personaName field in the response was always null. Also add integration tests for the /references and /references/count endpoints covering data shape, language filtering and fallback, session-language resolution, identifier/inode lookup, 404 on missing content, 403 without READ permission, and count scenarios. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Adjusted class bindings in the sidebar component to use parentheses for CSS variables, enhancing readability and consistency. - Added background color to the tab panels for better visual distinction. - Refactored HTML structure for improved clarity and maintainability in the sidebar information component. - Updated the dialog title localization to include dynamic content for better context. These changes enhance the user interface and maintainability of the sidebar components.
github-actions
Bot
added
Area : Backend
|
Claude finished @adrianjm-dotCMS's task in 5m 32s —— View job Code Review
Must fix1. Unused import will break Checkstyle
2. Null dereference in
const { identifier } = this.#dialogConfig.data;
const data = this.#dialogConfig.data;
if (!data?.identifier) {
this.$loading.set(false);
this.#dialogRef.close();
return;
}
const { identifier } = data;Should fix3. Error state renders "Not used on any page yet" — actively wrong
4. Empty-state test assertion silently passes for wrong reason
expect(table?.querySelectorAll('tbody tr[data-testid]').length).toBe(0);No 5. Error-state test asserts signal internals, not DOM
expect(spectator.component.$loading()).toBe(false);The same reviewer who asked for DOM assertions accepted signal assertions in the error block. If the loading skeleton was removed from the template, this test would still pass. Assert via Minor6. Paginator visible when all rows fit on one page
7.
What looks good
|
- Added computed properties in the sidebar information component to provide detailed tooltips and loading states for contentlet references. - Improved the references dialog component by implementing `takeUntilDestroyed` for better resource management during subscriptions. - Updated comments for clarity and maintainability, enhancing the overall code documentation. These changes improve the user experience by providing clearer information and more efficient resource handling in the sidebar components.
- Added unit tests for the `$hasReferences` computed property to verify its behavior based on `referencesPageCount`. - Implemented dialog opening functionality for the references card, ensuring it opens correctly and prevents multiple instances. - Updated the sidebar information component to include necessary imports and improve the structure of the references dialog. These changes improve the user experience by providing clear visibility of content references and enhancing the overall functionality of the sidebar component.
There was a problem hiding this comment.
Pull request overview
Adds an Edit Content “Page References” dialog from the General tab’s References section, backed by the existing /api/v1/content/{inodeOrIdentifier}/references endpoint, and fixes a backend persona-name mapping bug used by that endpoint.
Changes:
- Makes the References section clickable when the contentlet is referenced, opening a DynamicDialog with a paginated PrimeNG table.
- Adds frontend models/service method for fetching contentlet page references and new i18n keys for the dialog/table columns.
- Fixes backend
ContentResourceto read persona from the correct map key ("persona").
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| dotcms-integration/src/test/java/com/dotcms/rest/api/v1/content/ContentResourceIntegrationTest.java | Adds integration tests for references listing/count endpoints (language behavior, personas, permissions). |
| dotCMS/src/main/webapp/WEB-INF/messages/Language.properties | Adds dialog title + table column labels for references dialog. |
| dotCMS/src/main/java/com/dotcms/rest/api/v1/content/ContentResource.java | Fixes persona key mismatch when mapping reference results to ContentReferenceView. |
| core-web/libs/edit-content/src/lib/services/dot-edit-content.service.ts | Adds getContentletReferences() client call for the dialog. |
| core-web/libs/edit-content/src/lib/models/dot-edit-content.model.ts | Adds DotContentReference + DotReferencesDialogData typings for the dialog. |
| core-web/libs/edit-content/src/lib/components/dot-edit-content-sidebar/dot-edit-content-sidebar.component.html | Adjusts sidebar tab panel styling. |
| core-web/libs/edit-content/src/lib/components/.../dot-edit-content-sidebar-references-dialog/* | Adds the references dialog component (UI + tests). |
| core-web/libs/edit-content/src/lib/components/.../dot-edit-content-sidebar-information.component.{ts,html,spec.ts} | Makes References card conditionally clickable and opens the dialog. |
| DotReferencesDialogData | ||
| } from '../../../../../models/dot-edit-content.model'; | ||
| import { DotEditContentService } from '../../../../../services/dot-edit-content.service'; | ||
|
|
There was a problem hiding this comment.
Done. Added a class-level JSDoc and JSDocs for the $references, $loading, and $rows signals.
| this.$references.set(refs); | ||
| this.$loading.set(false); | ||
| }, | ||
| error: () => this.$loading.set(false) |
There was a problem hiding this comment.
This error callback swallows every HTTP failure silently — on a 500/403/timeout the dialog lands on the empty-state template and shows "Not used on any page yet", which is actively wrong for a contentlet the user just opened because it advertised live references. At minimum, inject DotHttpErrorManagerService and call handle(err) here so the user sees the standard error toast; ideally also track an $error signal and render a distinct error row in the table instead of the empty message.
There was a problem hiding this comment.
Done. Injected DotHttpErrorManagerService and calling handle(err) in the error callback so the user sees the standard error toast on HTTP failures.
| export class DotEditContentSidebarInformationComponent { | ||
| #dotMessageService = inject(DotMessageService); | ||
| readonly #dotMessageService = inject(DotMessageService); | ||
| readonly #dialogService = inject(DialogService); |
There was a problem hiding this comment.
DialogService is inject()ed but not declared in the component's providers. For standalone components using DynamicDialog it's safer to add providers: [DialogService] so the service is scoped to this component's injector (especially important if anything ever wraps this in a different outlet). Also worth adding DotEditContentSidebarReferencesDialogComponent to the imports array for consistency.
There was a problem hiding this comment.
Done. Added providers: [DialogService] to the @Component decorator so the service is scoped to this component's injector.
| contentlet: mockContentlet, | ||
| contentType: mockContentType, | ||
| referencesPageCount: '3', | ||
| loading: false |
There was a problem hiding this comment.
It's more valuable to verify that the rendered elements actually exist in the DOM than to assert the value of $hasReferences() directly. These tests pass purely on the computed signal's return value — if the references card were removed from the template entirely, they would still pass, which is incorrect. Please assert via byTestId('references-card') after setInput + detectChanges() (the references card describe block below already follows this pattern).
There was a problem hiding this comment.
Done. Replaced the $hasReferences() signal assertions with DOM queries using byTestId('references-card') so the tests fail if the element is removed from the template.
| * Input that contains the data of the contentlet. | ||
| */ | ||
| /** The sidebar data including the contentlet, content type, loading state, and references count. */ | ||
| $data = input.required<ContentSidebarInformation>({ alias: 'data' }); |
There was a problem hiding this comment.
Minor: mark these signal fields as readonly for consistency with #dotMessageService, #dialogService, #destroyRef in the same class. Applies to $data, $jsonUrl, $createdTooltipMessage, $hasReferences here, and to $references, $loading in the dialog component.
There was a problem hiding this comment.
Done. Added readonly to $data, $jsonUrl, $createdTooltipMessage, $hasReferences in the information component and $references, $loading in the dialog component.
| expect(dialogRefMock.close).toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
There's no coverage for the error branch of getContentletReferences. Adding a describe('on service error') block with a throwError(() => new Error('boom')) mock would (a) document that the loading flag resets and (b) catch the silent-failure issue flagged in the main review comment. Once the error UX is added (toast and/or error row), assert both here.
There was a problem hiding this comment.
Done. Added an on service error describe block that mocks throwError, asserts the loading flag resets to false, and asserts DotHttpErrorManagerService.handle was called.
| /** Whether the contentlet has at least one page reference. Controls the clickable card variant. */ | ||
| $hasReferences = computed(() => { | ||
| const count = this.$data().referencesPageCount; | ||
| return count && count !== '0'; |
There was a problem hiding this comment.
Prefer an explicit boolean: return !!count && count !== '0';. Current return type is string | false | undefined which works in @if today but couples behavior to JS truthiness instead of expressing the predicate clearly.
There was a problem hiding this comment.
Done. Already fixed as part of the readonly signal cleanup — $hasReferences now returns !!count && count !== '0'.
| it('should render the table with no rows', () => { | ||
| expect(spectator.query(byTestId('references-table'))).toBeTruthy(); | ||
| expect(spectator.component.$references()).toEqual([]); | ||
| }); |
There was a problem hiding this comment.
Same note as the other computed-signal tests: assert the DOM (e.g. expect(spectator.queryAll('p-table tr[data-testid]').length).toBe(0) or check that the #emptymessage content is visible) rather than the internal $references() signal value.
There was a problem hiding this comment.
Done. Replaced the $references() signal assertion with a DOM check — the test now asserts the table is present and has no data rows.
| spectator.detectChanges(); | ||
|
|
||
| expect(spectator.queryAll('p-skeleton').length).toBeGreaterThan(0); | ||
| expect(spectator.query(byTestId('references-table'))).toBeFalsy(); |
There was a problem hiding this comment.
Minor: queryAll('p-skeleton') is a generic tag selector — prefer byTestId('loading-skeleton') (adding the testid to the skeleton element in the template). Same applies to spectator.query('p-skeleton') in the sibling sidebar-information spec.
There was a problem hiding this comment.
Done. Added data-testid="loading-skeleton" to the skeleton wrapper in the dialog template and to both skeleton elements in the information template. Updated both specs to use byTestId('loading-skeleton') instead of the generic tag selectors.
- Add providers: [DialogService] to information component for proper DI scoping
- Inject DotHttpErrorManagerService and call handle(err) on HTTP errors in dialog
- Add class-level and signal JSDocs to dialog component
- Mark all signal/computed fields as readonly in both components
- Fix $hasReferences to return explicit boolean (!!count && count !== '0')
- Replace signal assertions with DOM queries in $hasReferences spec tests
- Add data-testid="loading-skeleton" to skeleton elements in both templates
- Update specs to use byTestId('loading-skeleton') over generic tag selectors
- Add error branch test coverage for dialog (DotHttpErrorManagerService.handle)
- Replace $references() signal assertion with DOM check in empty state test
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
Closes #30584
ContentResource.javawherepersonaNamewas always null because it was read from the wrong map key ("personaName"instead of"persona")DotReferencesDialogDatainterface todot-edit-content.model.tsalongsideDotContentReferenceIMAGES
Test plan