Skip to content

fix(uve): add site URL option to copy URL popover in page editor#35617

Closed
gortiz-dotcms wants to merge 9 commits into
mainfrom
issue-35448-copy-url-site-hostname
Closed

fix(uve): add site URL option to copy URL popover in page editor#35617
gortiz-dotcms wants to merge 9 commits into
mainfrom
issue-35448-copy-url-site-hostname

Conversation

@gortiz-dotcms
Copy link
Copy Markdown
Member

@gortiz-dotcms gortiz-dotcms commented May 7, 2026

What does this PR do?

Adds a third Site URL entry to the copy URL popover in the UVE page editor. Previously, both the Live URL and Current View URL were always built from the admin login origin (or clientHost for headless pages), giving users no way to copy a link scoped to the currently selected site's public hostname.

The new entry appears only when site.hostname is set and it produces a URL that differs from the Live URL already shown, avoiding redundant entries.

Fixes

Closes #35448

Changes

  • edit-ema-editor.component.ts — Extended $pageURLS computed to push a uve.toolbar.page.site.url entry when the site hostname differs from the current host.
  • Language.properties — Added uve.toolbar.page.site.url=Site URL i18n key.
  • edit-ema-editor.component.spec.ts — Added three new unit tests covering: site URL shown when hostname differs, suppressed when hostname matches clientHost, and suppressed when hostname is empty.

Test plan

  • Open the page editor for a traditional (non-headless) page whose site hostname differs from the admin URL — the copy URL popover should now show a third "Site URL" entry pointing to https://<site-hostname>/<page-path>
  • Open the page editor for a headless page where clientHost already matches the site hostname — only two URL entries should appear (no duplicate)
  • Unit tests: yarn nx test portlets-edit-ema-portlet --testPathPattern=edit-ema-editor.component.spec

🤖 Generated with Claude Code

)

The copy URL popover only showed URLs based on the admin login origin.
Now a third "Site URL" entry is added when the page's site hostname
differs from the current host, letting users copy a link scoped to the
selected site.

Refs: #35448

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 7, 2026

Claude finished @gortiz-dotcms's task in 2m 36s —— View job


Review

  • Read changed files
  • Analyze logic and edge cases
  • Check tests
  • Post review

The change is small, focused, and well-tested. A few things worth a second look:

1. Hardcoded https:// ignores the actual scheme — and breaks port-bearing dev setups

edit-ema-editor.component.ts:1476

const siteHostUrl = new URL(path, `https://${site.hostname}`).toString();

Recent commit 6bfb3e41 deliberately hardcoded the scheme. A few consequences:

  • A dotCMS instance accessed over http://demo.dotcms.com produces liveUrl = http://... and siteUrl = https://... — they will not dedupe and the popover will show two near-identical entries. Users who copy "Site URL" land on a non-existent HTTPS endpoint.
  • For dev environments running on http://localhost:8080, the site URL will be https://localhost/... which is unreachable. A copy-button that gives an invalid URL is worse than no button.
  • If the site is on a non-standard HTTPS port (e.g. :8443) and the hostname isn't stored with the port, the URL is wrong.

Worth either: (a) using the live URL's protocol/port, (b) preferring site.httpsRequired-style flag if one exists, or (c) explicitly documenting that this is an intentional production-only assumption.

2. Silent catch {} makes malformed-hostname diagnostics invisible

edit-ema-editor.component.ts:1484

catch {} swallows everything. If a site is misconfigured (whitespace in hostname, non-ASCII without IDN encoding, etc.), the Site URL just silently disappears with no clue why. A one-line console.warn (or the project's logger) would help support debug this when it shows up in the field. Not blocking.

3. Dedup only checks liveUrl, not currentViewUrl

The condition is siteHostUrl !== liveUrl. The current view URL is built via createFullURL(...) which uses clientHost || window.location.origin — it can also coincide with the site hostname (e.g. when there are no extra params). In practice it almost always carries language_id/persona params so this is unlikely, but it means duplicates can theoretically appear when currentViewUrl happens to be hostname-only. Probably fine; flagging because the dedup invariant is "show only when distinct" and the check is partial.

4. The "case-insensitive dedup" test passes by luck of URL normalization

edit-ema-editor.component.spec.ts:993-1019

Earlier commit 69d2d902 dropped the explicit toLowerCase() because URL lowercases the host automatically. That's correct — but the test name implies the code does case-insensitive dedup explicitly. Worth a comment in the code (or test) noting the dedup correctness depends on URL normalization, not on a comparator. Future refactors could regress this.

5. Malformed-hostname test relies on URL throwing for spaces

edit-ema-editor.component.spec.ts:965-991

new URL('/path', 'https://invalid host name') throws in Node/V8 today, but URL parser leniency varies across engines. If a test runner upgrade ever changes this, the test would silently start asserting a different code path (URL construction succeeds, dedup-by-equality kicks in instead of the catch). Low priority but brittle.

6. Minor: comment-free try/catch masks intent

A short // hostname may be malformed; skip site URL if URL ctor throws would document why the catch is empty rather than relying on the test name.


Tests cover the meaningful branches (hostname differs, matches, system-host, empty, port mismatch, casing, malformed). Good coverage.

The biggest risk is #1 — hardcoded https:// is a real footgun in non-prod and HTTP-only deployments. Worth confirming this is the desired UX before merging.
Branch

@gortiz-dotcms gortiz-dotcms changed the title fix(uve): add site URL option to copy URL popover in page editor (#35… fix(uve): add site URL option to copy URL popover in page editor May 7, 2026
@github-actions github-actions Bot added Area : Backend PR changes Java/Maven backend code Area : Frontend PR changes Angular/TypeScript frontend code labels May 7, 2026
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@gortiz-dotcms gortiz-dotcms marked this pull request as ready for review May 8, 2026 14:19
- Hoist liveUrl const to avoid building it twice
- Mirror the active host scheme instead of hard-coding https://
- Compare hostnames (not full URLs) for dedup to avoid scheme-mismatch
  false positives
- Guard URL construction with try/catch for malformed site.hostname
- Add test: site URL shown using origin scheme when clientHost is absent

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…edup

- Lowercase site.hostname before URL construction and comparison to
  match WHATWG URL.hostname normalization, preventing false-positive
  Site URL entries when the backend returns mixed-case hostnames
- Replace silent catch with console.warn for malformed hostname debugging
- Add test: mixed-case hostname treated as duplicate of clientHost

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Extract PageURL type alias to eliminate repeated inline type literal
- Fix port-aware dedup: compare full computed URLs (siteHostUrl !== liveUrl)
  instead of hostnames, so a clientHost with a non-standard port no longer
  suppresses a genuinely different Site URL
- Revert scheme to hardcoded https:// — DotSite has no protocol field so
  borrowing the admin scheme was misleading; add comment explaining rationale
- Remove try/catch: replace with early space-guard (!hostname.includes(' '))
  to skip non-addressable hosts like "System Host" without swallowing errors
- Move Site URL to second position (after Live URL, before Current View URL)
  so public-facing links are grouped together
- Add TODO for alias-aware dedup
- Add tests: port-only difference shown, "System Host" suppressed,
  https always used when clientHost is absent

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…med hostnames

- Remove .toLowerCase() call: the WHATWG URL parser normalises host to
  lowercase automatically so the call was misleading, not functional
- Re-add try/catch around URL construction to guard against malformed
  hostname values that pass the space-check but would produce a broken
  URL (e.g. accidental scheme prefix)
- Extend inline comment to document the http/https dedup edge case on
  local dev setups

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…RL to last position

- Replace !site.hostname.includes(' ') with !site.systemHost to
  correctly exclude the non-addressable System Host via its typed
  boolean rather than a fragile English display-name check
- Move Site URL entry to last position [Live, Current View, Site]
  to match PR description ("third link") and avoid reshuffling the
  two existing entries users already learned
- Compact inline comment block
- Update System Host test to use systemHost: true with normal hostname
- Add systemHost: false to all other site mocks for type accuracy
- Add test: malformed hostname with spaces triggers catch and suppresses
  Site URL entry
- Rename port test for clarity

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Remove stale mirrored-protocol line; site URL now correctly uses
  https:// as intended and documented
- Remove inline comments from $pageURLS computed block

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@gortiz-dotcms
Copy link
Copy Markdown
Member Author

gortiz-dotcms commented May 11, 2026

Closing because PR 35539 already solved the issue

https://github.com/dotCMS/core/pull/35539/changes#diff-24dc496db1eb6feb3e10a031baa4b4b63c20f1cd02ade574065f6b967f11c365R1615-R1636

core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.ts
lines 1615-1636

@gortiz-dotcms gortiz-dotcms deleted the issue-35448-copy-url-site-hostname branch May 11, 2026 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : Backend PR changes Java/Maven backend code Area : Frontend PR changes Angular/TypeScript frontend code

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Copy URL functionality to use the selected site instead of the URL

2 participants