Skip to content

fix(dotai): design and QA follow-ups from the Angular migration (#37538) - #37541

Merged
fmontes merged 11 commits into
mainfrom
issue-37538-dotai-feedback
Sep 16, 2026
Merged

fmontes merged 11 commits into
mainfrom
issue-37538-dotai-feedback

Conversation

@fmontes

@fmontes fmontes commented Sep 14, 2026

Copy link
Copy Markdown
Member

Design and QA review of the new Angular dotAI portlet (#37417, implemented in #37423) raised seven items. Researching one of them — how you edit what is in an index — turned up that you cannot, because nothing persists the query that built it, which made the build/delete dialog worth fixing here rather than later (#37543).

Each section below has a slot for its screen recording. Every clip is against a locally-built container on the demo starter.


1. Empty states use the shared component

The portlet rolled its own dot-ai-empty-state — a bare title, no icon — so its tabs looked nothing like the rest of the admin. Deleted, and all eight call sites (Search ×3, Chat, Image, Embeddings ×2, Config Values ×2) now render through dot-empty-container from @dotcms/ui.

They are centred now too, which the first pass got wrong: dot-empty-container centres itself with h-full, and a percentage height needs a parent that has one. Every call site nested it under an auto-height wrapper, so each state measured only as tall as its own text and sat at the top of a blank pane. Search and Chat move theirs out of the container mx-auto column into the scroll box; Embeddings and Config Values replace the table entirely rather than sitting in a <td>, which is sized by its own content.

01-empty-states-use-the-shared-component.webm

2. Fields and Velocity template explain themselves

Velocity template had no hint and no placeholder at all, and the Fields hint was wrong on both counts.

The two are not independent, which is the part nobody could have guessed: BulkEmbeddingsRunner.run() tries the template first and only falls back to the field path when it renders nothing. So a template silently overrides Fields, and leaving both empty makes dotCMS guess (WYSIWYG, Story Block, Textarea, file, binary) rather than "embed the whole contentlet". Both hints say so now, and the template carries a worked example.

02-fields-and-velocity-template-copy.webm

3. View provider config removed

The button, its JSON dialog, the two message keys, and the maskCredentials helper that had no other caller.

03-view-provider-config-removed.webm

4. Rebuild DB is a plain outlined button

A permanently-red control in the toolbar read as a warning about the screen. The destructive step is the confirm dialog it opens.

04-rebuild-db-is-a-plain-outlined-button.webm

5. The Config Values host line says what it means

It read demo.dotcms.com (falls back to system host), assembled server-side by concatenating the hostname with an English suffix — untranslatable, and it claimed the fallback whether or not it had happened.

GET /api/v1/ai/completions/config now returns configHost and configHostInherited as separate fields, derived from whether ConfigService actually fell back to the System Host's secrets, and the client renders the label from a message key.

05-config-host-message-is-clear.webm

Two defects

6. Build errors stay in the modal

Submitting Build an index closed the dialog and reported the outcome on the tab underneath — so a rejected Lucene query arrived after the modal had taken the query away with it, and you had to reopen and retype.

The dialog now owns its submit, renders the failure and the "nothing matched" warning inline with the query still in the field, and closes only on success. It also cannot be dismissed at all while its request is outstanding — the answer is coming back into this form, so it has to still be there to receive it.

06-build-error-stays-in-the-modal.webm

7. A new index appears without a page reload

buildIndex called markIndexBuilding and then loadIndexes, and that refresh erased the flag it was meant to act on: applyIndexes rebuilt the status map from the server's list alone, and embedding is asynchronous so the new index is not in indexCount yet. With no BUILDING status left, the poll never started, so nothing re-fetched and the row only showed up on a manual reload.

Seeded builds now hold a placeholder row until the server catches up, the fragment snapshot is taken from the server's response only, and a grace period stops the poll if a build never materialises.

07-new-index-appears-without-a-reload.webm

Also here: #37543

8. Build and remove are separate dialogs

The Mode toggle made the form reshape under the user (Fields and Velocity template vanished on switching) and put a destructive action one click from a create action, sharing its submit button. Build an index only builds.

Removing content is its own toolbar action beside New Index — the two are the same shape of operation, a Lucene query plus an index to scope it to. It is deliberately not a row action: nothing records the query that built an index, so the query is written blind either way, and hanging it off a row would promise context the backend cannot give. The index is a picker over the real list rather than a free-text field, and nothing is preselected, because this removes embeddings.

The dialog also states three things the old delete mode said nowhere: it removes embeddings and not content; the query matches content as it is now, so content that changed is left behind and archived content cannot be reached; and unlike building it is not limited to live content.

08-build-and-remove-are-separate-dialogs.webm

9. Every action reports what it did

Rebuild DB, Delete index and Remove content were completely silent — three of the four returned a count the store dropped on the floor. One indexNotice channel now carries the operation and its outcome, because where an outcome belongs is decided by the outcome rather than the action: anything the user must act on stays in the dialog that submitted it, everything else is a toast. A removal that matched nothing is empty, not success — the server answers 200 with deleted: 0.

Toasts, as dot-locales and dot-experiments do it. The tab no longer carries a standing banner for a successful build.

09-every-action-reports-what-it-did.webm

Testing

  • nx test portlets-dot-ai-portlet — 283 passing
  • nx affected -t test — 2 267 passing across every affected project
  • CompletionsResourceTest — 4 new unit tests on the backend rule, including the "nothing configured anywhere" branch it exists for
  • nx affected -t lint clean across 36 projects; nx format:check clean
  • Verified in a browser against a locally-built container throughout

block-editor has 35 test failures that also fail on main; confirmed unrelated by re-running it with this branch's shared-lib changes reverted.

Notes for the reviewer

Two review rounds ran over this branch and both found real defects, which are fixed and pinned by tests that fail without them. Worth knowing where they were, since they all lived in the plumbing rather than the user-facing fixes:

  • A deleted index came back as a ghost Building row for two minutes, offered in the retrieval picker and eligible to become the selected index — so Search and Chat could point at an index that no longer existed.
  • A dialog reopened over an in-flight request closed on the previous request's outcome while exhaustMap silently dropped the new one.
  • A 403 left a modal nobody could close but a page reload, because the dialog settles only on a notice and that path emitted none.
  • Outcomes no open dialog would render were suppressed anyway, so a delete that failed while another dialog was up was never reported.

Open question for whoever owns EmbeddingsResource: building appends +live:true to your query and deleting appends nothing, so the same string does not select the same content. The remove dialog states the asymmetry rather than deciding it.

Worth its own issue: index builds persist none of their inputs — not the query, the fields, the template or the model — so an index cannot be inspected, edited or re-run, and builds are additive, meaning re-running with a narrower query silently leaves the old content behind.

Known and deliberate: deleting an index while its build is still in flight lets the build's response re-seed it; reaching it needs a portlet-tab switch, since the dialog is modal-locked during flight. And the dialog's header X returns on the user's next interaction rather than instantly after a failure — closable is a plain property the host reads in a template binding, and the unlock runs inside an effect already part of the pass in progress. Cancel and Escape are live throughout.

Fixes #37538. Closes #37543.

🤖 Generated with Claude Code

This PR fixes: #37538

Seven items from the design and QA review of the new Angular dotAI portlet.

Empty states now go through `dot-empty-container` from `@dotcms/ui`, as every
other portlet does. The portlet's own `dot-ai-empty-state` is deleted and all
eight call sites converted, each with an icon.

`Velocity template` had no hint and no placeholder at all, and the `Fields`
hint was wrong. The two are not independent: `BulkEmbeddingsRunner` tries the
template first and only falls back to the field path when it renders nothing,
so a template silently overrides `Fields`, and an empty pair means dotCMS
guesses the fields rather than embedding the whole contentlet. Both hints now
say so, and the template carries an example.

`View provider config` and its JSON dialog are removed, along with the
`maskCredentials` helper that had no other caller.

`Rebuild DB` is a plain outlined button. A permanently-red control in the
toolbar read as a warning about the screen; the destructive step is the
confirm dialog it opens.

The Config Values host line was built server-side by concatenating the
hostname with " (falls back to system host)" — untranslatable, and it claimed
the fallback whether or not it had happened. `/completions/config` now returns
`configHost` and `configHostInherited` separately, and the label comes from a
message key.

Two defects:

- A rejected Lucene query was reported on the tab *after* the modal had closed
  over the query that caused it. The dialog now owns the submit, renders the
  failure and the "nothing matched" warning inline with the query still in the
  field, and closes only on success.

- A new index did not appear until a manual page reload. `markIndexBuilding`
  set the BUILDING flag and the refresh that followed erased it: `applyIndexes`
  rebuilt the status map from the server's list alone, and embedding is
  asynchronous so `indexCount` does not return the index yet. With no BUILDING
  status left the poll never started. Seeded builds now hold a placeholder row
  until the server catches up, with a TTL so an abandoned build stops the poll.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added Area : Backend PR changes Java/Maven backend code Area : Frontend PR changes Angular/TypeScript frontend code labels Sep 14, 2026
@claude

claude Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Claude finished @fmontes's task in 5m 19s —— View job


Code Review

Reviewed the full diff against origin/main (backend CompletionsResource/AiKeys/tests, the three store features, the shared dialog util, the new remove-content dialog, and the tab components). This is careful, well-tested work — the tricky ownership/seed/poll logic all holds up under trace:

  • isInheritedConfig is correct on all four branches (own config, fell-back-and-found, nothing-anywhere, case-insensitive) and the getProviderConfig() gate is the right guard against the "System Host reported even when empty" fallback.
  • The result.indexName$target coupling that could have stranded the build dialog's spinner is safe: EmbeddingsResource echoes form.indexName verbatim (EmbeddingsResource.java:134), and EmbeddingsDTO does no normalisation, so the notice's index always matches what submitted() recorded.
  • The #toasted identity guard + indexNoticeOwner correctly prevents the double-report (inline in the dialog, then a toast on close) — the release() on destroy re-runs the tab effect but the notice === this.#toasted short-circuit stops a second emission.
  • Object.prototype.hasOwnProperty.call(seeds, …) and the indexName === undefined (not falsy) checks are both real edge cases handled correctly.
  • All message keys referenced by NOTICE_MESSAGE_KEYS, toEmptyStateConfig, and both dialog templates exist in Language.properties, and maskCredentials / DotAiEmptyState / indexBuildNotice are removed with no dangling references.

New Issues

  • 🟡 Medium: core-web/libs/portlets/dot-ai/src/lib/store/features/with-ai-indexes.feature.ts:133,155 — a still-building index now enters the retrieval set. offered is derived from merged (server list plus the zero-content pending placeholders), and it both populates indexOptions (the Search/Chat picker) and drives the settingsIndexName fallback. On a first build against an instance with no other indexes, offered = [newIndex], so the picker auto-selects an index that has nothing in dot_embeddings yet; searching it returns empty until the build lands.
    • Assumption: the retrieval/search backend answers an empty index with empty results rather than an error (not verified against EmbeddingsResource search path).
    • What to verify: that selecting/searching a zero-fragment index is harmless UX (empty result set, no error toast). If so this is acceptable and self-healing as the build completes; flagging so it's a conscious choice rather than a side effect of merging placeholders into indexes(). The deleted-ghost variant of this is correctly closed by forgetIndexBuildSeeds. Fix this →

No blocking issues found. The rollback-safety of the configHost/configHostInherited contract change is already covered by the automated M-3 analysis above (additive optional field, client defaults it, self-healing) — no additional mitigation needed.
· issue-37538-dotai-feedback

fmontes and others added 3 commits September 14, 2026 12:53
`dot-empty-container` centres itself with `h-full`, and a percentage height
needs a parent that has one. Every dotAI call site nested it somewhere with an
auto height, so each state measured only as tall as its own text and pinned
itself to the top of an otherwise blank pane.

Search and Chat: the empty states move out of the `container mx-auto` column
and become direct children of the scroll box, which is a flex child with a
real height. The results and the loading skeletons keep the column.

Embeddings and Config Values: the empty states move out of the table's
`emptymessage` and replace the table entirely, as the forbidden state already
did. A `<td>` is sized by its own content, so nothing done to the host could
centre it there — and this also drops a header row with nothing under it.

The Embeddings empty state now picks its copy: "No indexes yet" only when
there are none, and "No indexes match this filter." when a filter is what
emptied the table. Telling someone to create their first index when they have
six and mistyped the filter is the wrong instruction.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`justify-center` belongs on the wrapper, not just inside the host. The host is
a flex *item* in that panel, so it was sized to its own content — a 30rem
column — and sat against the left edge; its own `justify-center` only centred
the text within that column. The other four tabs give the host a full-width
block, which is why this one was the only one still off.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cleanup pass over the feedback fixes; no behaviour change.

`indexStatuses` is a projection, not state. `applyIndexes` keeps a seed only
while its index still derives as BUILDING, so "seeded" and "building" are the
same fact by the time anything reads it — holding both only created somewhere
for the two to drift, and `markIndexBuilding` had to write each separately.
It is now a computed over `indexes` and `indexBuildSeeds`, which also makes
the poll guard say what it means: is any build outstanding.

The build-seed TTL now covers only the window before the index is listed at
all. Measured from the start of the build, as it was, a build of a few
thousand contentlets would have its seed dropped at two minutes, flip to Ready
mid-flight and stop the poll — the exact failure the feature exists to
prevent. Once the index is listed the fragment delta owns its lifecycle and no
clock is involved.

`indexBuildInFlight` leaves the state model: it disabled one button in one
dialog and nothing outside read it, so it is a local signal there instead.
The dialog now clears an unrendered outcome in `DestroyRef.onDestroy` rather
than in `cancel()` — PrimeNG's header X and Escape call `DynamicDialogRef.close`
directly and never reached that handler, leaving a failure set with nothing
showing it.

Also: `markIndexBuilding` no longer re-implements the guard already inside
`withPendingIndexes`; `applyIndexes` stops building and discarding a label per
index for one membership test; the tab's build notice loses a nested `@if`;
and the ten near-identical `PrincipalConfiguration` literals collapse onto one
`toEmptyStateConfig` helper.

Documents the dialog-owns-its-submit exception in libs/portlets/CLAUDE.md,
which still said `close(formValue)` was the only pattern.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Pull Request Unsafe to Rollback!!!

  • Category: M-3 — REST / GraphQL / Headless API Contract Change
  • Risk Level: 🟡 MEDIUM
  • Why it's unsafe: GET /api/v1/ai/completions/config changed its response contract and the Angular admin frontend was migrated to the new shape in the same PR. In dotCMS/src/main/java/com/dotcms/ai/rest/CompletionsResource.java, the configHost field's value changed from a descriptive sentence (host.getHostname() + " (falls back to system host)") to the bare hostname, and a new AiKeys.CONFIG_HOST_INHERITED boolean field was added (AiKeys.java line 43). The frontend (core-web/libs/portlets/dot-ai/.../dot-ai-config-values.component.ts) now builds its label from these two separate fields via dotai.config.host / dotai.config.host.inherited message keys instead of rendering the server string verbatim. If a browser/SPA session keeps running N's JS after a rollback to N-1 (e.g. a tab left open across the redeploy, or a cached bundle), it will call the old backend, which still returns the concatenated sentence in configHost and omits configHostInherited entirely.
  • Code that makes it unsafe: dotCMS/src/main/java/com/dotcms/ai/rest/CompletionsResource.java (the map.put(AiKeys.CONFIG_HOST, ...) / map.put(AiKeys.CONFIG_HOST_INHERITED, ...) block, ~line 196-202) and dotCMS/src/main/java/com/dotcms/ai/AiKeys.java line 43 (new CONFIG_HOST_INHERITED key), paired with the same-PR frontend migration in core-web/libs/portlets/dot-ai/src/lib/tabs/dot-ai-config-values/dot-ai-config-values.component.ts and core-web/libs/data-access/src/lib/dot-ai/dot-ai-config.service.ts.
  • Impact note: this is a soft/self-healing case, not a hard break — the frontend already defaults the missing configHostInherited to false (dot-ai-config.service.ts: raw?.configHostInherited ?? false), so under the old backend the screen just shows a redundant label (e.g. "Showing settings for demo.dotcms.com (falls back to system host)") rather than throwing. No data loss, no 500s, no crash — it resolves itself on the next full page load once client and server are back in sync.
  • Alternative (if possible): per the M-3 guidance, this is exactly the low-risk end of the pattern already — the new field is additive and optional, and the client defensively defaults it. No further mitigation is required beyond what's already in place; flagging for visibility only.

@fmontes

fmontes commented Sep 14, 2026

Copy link
Copy Markdown
Member Author

Follow-up filed as #37543 — splitting the build/delete dialog, moving "delete from index" onto the index row, and fixing the silent destructive actions (Rebuild DB, Delete index and Delete from index all currently report nothing).

Scope of this PR is unchanged; #37543 came out of researching how index editing works and is better reviewed on its own.

…nd in review

Five fixes from a review pass over the #37538 follow-ups, each one a case where
the screen said something that was not true.

Config Values blamed the filter for every empty table. `resolvedConfig` is a
computed that always returns an object, so the rows are empty during the initial
async window and after a failed load as well as after a filter that matched
nothing -- and the pane said "No settings match this filter." with the filter box
untouched, on the one screen that exists to be trusted when nothing else works
(FR-048). A failed load now says so; the loading window renders nothing, because
a brief blank pane says less than a wrong sentence does.

The pending-build expiry only ran on success. It lived inside `applyIndexes`,
which a failing request never reaches, so a build started just before the server
went away held its seed forever: the poll ticked every five seconds for the life
of the page, raising an error dialog each time. Pruning now runs wherever the
request lands, the 403 and generic failure branches included.

A build abandoned mid-flight failed into silence. Escape and the header X destroy
the dialog before the outcome arrives, and the tab rendered `built` only -- so a
success was announced and a failure disappeared. The tab now reports every
outcome the dialog is no longer there to render. The handover fires on
`onDestroy` rather than `onClose`, because `close()` emits `onClose` immediately
and only then plays the leave animation: handing over there would flash an
outcome onto the tab for the frames before the dialog's own teardown withdrew it.

`deriveIndexStatuses` was a seed-survival predicate wearing a status function's
name. The status the table reads comes from the `indexStatuses` computed; this
one's output fed a filter, and every READY it derived was discarded. It is now
`stillBuildingSeeds` and returns what it is actually asked for, so there is one
place that decides what "building" means.

`configHostInherited` claimed an inheritance that had not happened. ConfigService
reports the System Host as the resolved host whenever a site has no secrets of
its own, whether or not the System Host had any either, so an instance with
nothing configured anywhere read "inherited from System Host" underneath the
shell's "not configured" banner. Now gated on there being a configuration.

Tests: 256 in the portlet (11 new), 927 in data-access. The two expiry
regressions were confirmed red without the fix.
Verified `openapi.yaml` is unchanged by a full `mvnw compile -pl :dotcms-core`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ort (#37543)

The Embeddings tab packed two opposite actions into one dialog and none of the
destructive ones said what they had done.

**Split.** "Build an index" only builds. The Mode toggle made the form reshape
under the user — Fields and Velocity template vanished on switching — and put a
destructive action one click from a create action, sharing its submit button.

**Removing content moved onto the index row.** It always targets exactly one
index, yet the index name was a free-text field: a typo removed nothing and a
near-miss hit a different index, both silently. The row's trash icon is now a
menu — Remove content…, Delete index — and the dialog opens titled for the row,
with the index as context rather than an input.

**It explains itself.** Three facts the old delete mode stated nowhere: it
removes embeddings and not content; the query matches content as it is now, so
content that has changed and no longer matches is left behind and archived or
deleted content cannot be reached at all; and unlike building, it is not
limited to live content — `embed()` appends `+live:true` to your query and
`deleteByQuery` appends nothing.

**Every action reports.** Rebuild DB, Delete index and Remove content were
completely silent — three of the four returned a count the store dropped on the
floor. `indexBuildNotice` becomes `indexNotice`, one channel carrying the
operation and its outcome, because where an outcome belongs is decided by the
outcome rather than the action: anything the user has to act on stays in the
dialog holding the field that caused it, everything else is a toast. A removal
that matched nothing is `empty`, not success — the server answers 200 with
`deleted: 0`.

**Toasts, as the rest of the admin does.** The tab no longer carries a standing
banner for a successful build; `dot-locales` and `dot-experiments` host their
own `p-toast` the same way.

Closes #37543

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

test

@claude

claude Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

(Ignore the previous comment — that was an accidental test post while verifying tool access. Rollback-safety analysis below.)

@claude

claude Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Pull Request Unsafe to Rollback!!!

  • Category: M-3 — REST / GraphQL / Headless API Contract Change
  • Risk Level: 🟡 MEDIUM
  • Why it's unsafe: CompletionsResource.java's AI-config endpoint changed the shape of an existing response field and the Angular admin frontend was co-migrated in the same PR to consume the new shape. Previously, AiKeys.CONFIG_HOST held a single English string that sometimes included a baked-in suffix (host.getHostname() + " (falls back to system host)"). Now configHost is always just the plain hostname, and a new boolean field AiKeys.CONFIG_HOST_INHERITED (configHostInherited) was added to convey the same fact. The frontend (dot-ai-config-values.component.ts) was updated in this same PR to branch on config.configHostInherited to pick a message key (dotai.config.host vs dotai.config.host.inherited), rather than just displaying the old field. If the backend is rolled back to N-1 while the N frontend bundle is still cached/served (browser cache or CDN), configHostInherited will be undefined (falsy) on the old response, so the UI falls through to the plain dotai.config.host key with a configHost value that already contains the old "(falls back to system host)" suffix — producing a garbled label (e.g. "Showing settings for demo.dotcms.com (falls back to system host)"). This is a display-only degradation, not a crash or data loss, consistent with MEDIUM.
  • Code that makes it unsafe:
    • dotCMS/src/main/java/com/dotcms/ai/rest/CompletionsResource.java (response map construction, ~lines 193-210): replaces the single AiKeys.CONFIG_HOST string with a plain hostname plus new AiKeys.CONFIG_HOST_INHERITED boolean.
    • dotCMS/src/main/java/com/dotcms/ai/AiKeys.java: adds CONFIG_HOST_INHERITED = "configHostInherited".
    • core-web/libs/portlets/dot-ai/src/lib/tabs/dot-ai-config-values/dot-ai-config-values.component.ts (lines ~68-80): reads config.configHostInherited to select the message key, assuming the new field exists.
    • core-web/libs/portlets/dot-ai/src/lib/store/features/with-ai-config.feature.ts (lines ~57-58, 100-101): propagates configHostInherited through the store from the REST response.
  • Alternative (if possible): Keep the endpoint backward compatible for one release — continue emitting the old combined string in configHost (or a clearly-deprecated field) alongside the new configHostInherited flag, and only have the frontend prefer the new flag when present, falling back to string-parsing (or just displaying the old value as-is) when it's absent. Drop the compatibility string in a later release once N-1 is outside the rollback window.

On a row it implied it knew what was in that index. Nothing records the query
that built an index, so the query typed here is written blind either way, and
the row placement promised a context the backend cannot give.

The two are also the same shape of operation — a Lucene query plus an index to
scope it to, run against current content — so making one a toolbar button and
the other a row action made them look like different kinds of thing.

The index is a picker over the real list rather than a free-text field, which
is the one thing the row placement was right about: as free text on the old
delete mode, a typo removed nothing and a near-miss hit a different index, both
in silence. The row goes back to a single delete action, so the toolbar reads
Remove content / Rebuild DB as quiet outlined buttons beside a primary
New Index.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Pull Request Unsafe to Rollback!!!

  • Category: M-3 — REST / GraphQL / Headless API Contract Change
  • Risk Level: 🟡 MEDIUM
  • Why it's unsafe: The configHost field in the dotAI completions-config REST response changed its value semantics — it used to carry a human-readable descriptive string ("demo.dotcms.com (falls back to system host)") and now carries just the bare hostname, with a new configHostInherited boolean added to convey the "inherited from System Host" fact separately. The Angular admin frontend was updated in the same PR to consume the new shape (an explicit M-3 signal per the reference doc). If a browser has the N frontend bundle cached (or the deploy is a mixed-version window) and the backend is rolled back to N-1, the cached N JS will read configHostInherited as undefinedfalse and will display N-1's old descriptive string as if it were a bare hostname — a UI display bug, not a crash, but user-visible incorrect/confusing config info until a hard refresh.
  • Code that makes it unsafe: dotCMS/src/main/java/com/dotcms/ai/rest/CompletionsResource.java (lines ~196-214) — replaced map.put(AiKeys.CONFIG_HOST, host.getHostname() + " (falls back to system host)") with map.put(AiKeys.CONFIG_HOST, requestedHost) + new map.put(AiKeys.CONFIG_HOST_INHERITED, ...); co-migrated in core-web/libs/dotcms-models/src/lib/dot-ai.model.ts and core-web/libs/data-access/src/lib/dot-ai/dot-ai-config.service.ts (RawCompletionsConfig.configHostInherited, #toResolvedConfig).
  • Alternative (if possible): Already partially mitigated — the frontend treats configHostInherited as optional (raw?.configHostInherited ?? false), so a rollback-mismatched backend won't throw. To fully close the gap, the frontend could also detect the legacy "(falls back to system host)" suffix in configHost as a compatibility fallback for one release cycle, per the two-phase contract pattern in the reference doc.

Six findings from a review pass over the notice and seed machinery.

**A deleted index came back as a ghost row.** Nothing cleared a build seed when
its index was deleted, so `withPendingIndexes` put the index straight back in
the table as a zeroed BUILDING row for the rest of the two-minute grace period —
offered in the retrieval picker, and eligible to become `settingsIndexName`, so
Search and Chat could point at an index that no longer existed. Rebuild DB had
the same hole. `forgetIndexBuildSeeds` is called by both.

**A reopened dialog closed on the previous submission's outcome.** Both store
methods are rxMethods on the shell-scoped store, so they outlive the dialog that
started them: abandon a slow build, reopen, submit another, and `exhaustMap`
dropped the second request while the first one's success closed the dialog and
reported an index the user never asked for. Dialogs now match the notice against
the index they themselves submitted, and the operator is `concatMap` so a second
submit is neither dropped nor cancels the first. Double-submit is already
prevented by the dialog disabling its own button.

**A failure shown inline was toasted again on close.** Suppressed notices were
not marked as reported, so dismissing the dialog re-ran the effect and toasted
the error the user had just read. The comment claiming the teardown hook dropped
it described behaviour that did not exist.

**The build dialog's effect did not filter by operation**, so any other
operation's result cleared its spinner mid-build — the remove dialog filtered
correctly. Both now share `watchIndexOperation`; writing this twice is what let
them drift.

**Opening a dialog discarded an unreported outcome.** `dismissIndexNotice` on
open threw away a delete that had just failed. With dialogs filtering by their
own index, a stale notice cannot reach them anyway, so the call is gone — and
with it the now-unused store method.

**Remove content preselected a target the user never chose** — `indexes()[0]`,
which is alphabetically first and often `cache`. It starts empty.

Also: the toast message keys are an explicit `Record` rather than interpolated
at call time. Assembled keys are invisible to a grep, so a copy audit reports
them as orphaned, and an undefined combination renders its own key to the user.

`CompletionsResource.isInheritedConfig` is extracted and unit tested — the
"nothing configured anywhere" branch it exists for had no coverage.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The answer comes back into the form — a rejected query belongs in the field
that produced it — so the dialog has to still be there to receive it. All three
ways out are closed while a request is outstanding: the header X is hidden,
Cancel is disabled, and Escape declines.

Escape needed taking over. PrimeNG binds its own document listener once, when
the dialog opens, and never rereads `closeOnEscape` — so mutating the config
mid-flight does nothing. Both dialogs now open with `closeOnEscape: false` and
the shared helper owns the key, which keeps Escape closing the dialog as the
portlet guide requires while letting it stand down mid-request.

Known gap, deliberately not papered over: `closable` is a plain property the
dialog host reads in a template binding, so hiding the X lands immediately (it
happens in a click handler) but restoring it waits for that host's next
change-detection pass — the unlock runs inside an effect, already part of the
pass in progress. The X therefore returns on the user's next interaction rather
than instantly. It only happens on a failed or empty outcome, which is exactly
when they are about to type in the field again, and Cancel and Escape are live
throughout. A `setTimeout` was tried and changed nothing, so it is not left
behind for the look of it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Pull Request Unsafe to Rollback!!!

  • Category: M-3 — REST / GraphQL / Headless API Contract Change
  • Risk Level: 🟡 MEDIUM
  • Why it's unsafe: GET /api/v1/ai/completions/config changed the meaning/shape of the existing configHost field and added a new configHostInherited boolean. Previously configHost was a display string ("<hostname> (falls back to system host)"); now it's the bare hostname, and the "inherited from System Host" fact moved into the new configHostInherited field. The Angular AI portlet was updated in this same PR to derive its "inherited" label from configHostInherited instead of parsing the old string — this is the exact M-3 signal: "Any API change where the Angular frontend is updated in the same PR to use the new shape." If the release is rolled back to N-1 while a browser/CDN still has the N frontend bundle cached, the N-1 backend will resume returning the old string format and omit configHostInherited; the frontend's raw?.configHostInherited ?? false default means the UI silently shows "not inherited" (loses the inherited-from-System-Host indication) instead of erroring — a degraded/incorrect display, not a hard break, consistent with the MEDIUM tier.
  • Code that makes it unsafe:
    • dotCMS/src/main/java/com/dotcms/ai/rest/CompletionsResource.java:196-200map.put(AiKeys.CONFIG_HOST, requestedHost) (was host.getHostname() + " (falls back to system host)") and map.put(AiKeys.CONFIG_HOST_INHERITED, isInheritedConfig(requestedHost, appConfig)); new helper at :231 (isInheritedConfig).
    • core-web/libs/portlets/dot-ai/src/lib/models/dot-ai-portlet.models.ts:128-130,218-219 — new configHostInherited: boolean field, defaulted via raw?.configHostInherited ?? false.
    • core-web/libs/portlets/dot-ai/src/lib/tabs/dot-ai-config-values/dot-ai-config-values.component.ts:79 — branches the displayed message key (dotai.config.host.inherited vs dotai.config.host) on config.configHostInherited.
  • Alternative (if possible): Ship configHostInherited as an additive field for one release while the old descriptive configHost string is still emitted unchanged, and keep the frontend able to derive "inherited" from either signal (string suffix or the new boolean) for that release. Switch the frontend to depend on configHostInherited exclusively only in the following release, once N-1 is outside the rollback window. Given the narrow blast radius (one internal admin-only config panel, with a safe boolean default), this could reasonably be accepted as a documented rollback caveat instead.

…utcomes

Second review pass, on the fixes from the first.

**A 403 left an unclosable modal.** `report` returned early on 403 without a
notice, and a dialog settles only on one — so with the new "cannot dismiss
while outstanding" rule, `$submitting` stayed true, the header X stayed hidden,
Cancel stayed disabled and Escape kept declining. Nothing but a page reload got
the user out. Every path notifies now, 403 included; the tab still enters its
forbidden state.

**Outcomes no dialog would render were suppressed anyway.** The tab decided
"this belongs to the dialog" from `#dialogOpen` alone, but a dialog renders
only outcomes matching the operation *and* index it submitted. A delete that
failed while the build dialog happened to be up was therefore suppressed by the
tab, ignored by the dialog, and never reported — the silence this whole change
set exists to remove. Ownership is now declared: the dialog claims
`(operation, indexName)` at submit and releases it on teardown, and the tab
reports everything else. Declaring at submit rather than at render keeps it
independent of which effect runs first. `#dialogOpen` and the tab's two
`onDestroy` subscriptions go with it.

**A deleted index could still be left standing.** `forgetIndexBuildSeeds`
dropped the seed but left the placeholder row for the next refresh to clear —
and `loadIndexes`' error branch leaves `indexes` untouched, so a failed refresh
left the deleted index there as a plausible READY row with zeroes. The row goes
with the seed now.

**Three tests could never fail.** They asserted `dismissIndexNotice` was not
called, on a method deleted in the same change — tautologies where the one at
"should not discard an outcome that has not been reported yet" was precisely
the test that should have caught the swallowing above. Rewritten against
observable behaviour; they fail without these fixes.

Also: `forgetIndexBuildSeeds('')` cleared every seed, and '' is a real index
name in this module (a store-wide rebuild reports it) — it checks `undefined`
now. The Escape handler bails on `defaultPrevented` so nested overlays close
themselves first, and unbinds at close rather than at teardown, which for
DynamicDialog runs after the leave animation.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Pull Request Unsafe to Rollback!!!

  • Category: M-3 — REST / GraphQL / Headless API Contract Change
  • Risk Level: 🟡 MEDIUM
  • Why it's unsafe: CompletionsResource.getConfig() changes the shape/semantics of the configHost field in the /api/v1/ai/config response and adds a new configHostInherited boolean. Previously configHost carried a human-readable display string ("demo.dotcms.com (falls back to system host)"); now it's just the plain hostname, and the "inherited" signal moved to the new configHostInherited field. The Angular admin frontend is updated in this same PR to consume the new field (dot-ai-config-values.component.ts now branches on config.configHostInherited to pick a translation key instead of rendering configHost verbatim). If a browser has N's cached JS bundle after a rollback to N-1, it will call the same endpoint but receive N-1's old response shape (no configHostInherited, and configHost containing the old embedded suffix text). The UI would fall through to the non-inherited label while still interpolating the old display string into it, producing a garbled/duplicated message (e.g. "...inherited from demo.dotcms.com (falls back to system host)") — a degraded but non-fatal UI, consistent with MEDIUM.
  • Code that makes it unsafe: dotCMS/src/main/java/com/dotcms/ai/rest/CompletionsResource.java lines 196-200 (field value/shape change) and 231-234 (isInheritedConfig new field logic); dotCMS/src/main/java/com/dotcms/ai/AiKeys.java (new CONFIG_HOST_INHERITED key); consumed at core-web/.../dot-ai-config-values.component.ts (branches on configHostInherited for the translation key) and core-web/.../dot-ai-portlet.models.ts (new optional configHostInherited field, defaulted with ??).
  • Alternative (if possible): Per M-3 guidance, keep configHost backward-compatible (continue emitting the old display string, or gate the format with @JsonProperty/versioning) while introducing configHostInherited as a pure addition, rather than changing the existing field's value semantics in the same release the frontend adopts it. Since the frontend already defaults configHostInherited with raw?.configHostInherited ?? false this is a low-severity, self-healing case — no action required beyond awareness, but flagging per policy.

…ing as Building

`indexStatuses` decided BUILDING with `index.name in seeds`. The seed map comes
from `Object.fromEntries`, so `in` walks its prototype: an index named
`constructor`, `toString`, `valueOf` or `hasOwnProperty` read as seeded when it
was not, and then rendered Building forever — nothing clears a seed that never
existed. The create form's name pattern accepts all four and the backend stores
whatever it is given, so it is reachable rather than theoretical.

A regression from the commit that made `indexStatuses` a computed; the code it
replaced used `Set.has`, which has no prototype exposure.

Also corrects a doc reference to `BUILD_SEED_TTL_MS`, renamed to
`PENDING_SEED_TTL_MS` when its meaning narrowed to the pre-listing window.

Both raised by the review bot on #37541.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@fmontes

fmontes commented Sep 15, 2026

Copy link
Copy Markdown
Member Author

Both Medium findings fixed in 51869fe.

index.name in seeds walking the prototype chain — confirmed reachable before fixing, not just plausible:

'constructor' in seeds  => true      ← reads as seeded when it is not
hasOwnProperty.call     => false

The seed map comes from Object.fromEntries, so it carries Object.prototype, and INDEX_NAME_PATTERN = /^[a-zA-Z0-9_-]+$/ accepts all four colliding names. Now Object.prototype.hasOwnProperty.call(seeds, index.name).

You were right that it is a regression rather than a latent quirk: the code it replaced used Set.has, and the exposure arrived with the commit that made indexStatuses a computed. Pinned with a test over constructor / toString / valueOf / hasOwnProperty, which I verified fails against the old in check before keeping it.

Worth noting for anyone reading later: the blast radius stops at the badge. liveSeeds and stillBuilding iterate with Object.entries, withPendingIndexes uses a real Set, and the poll guard is Object.keys(...).length > 0 — so a colliding name rendered a permanently wrong status but never started a runaway poll or produced a phantom row.

Stale BUILD_SEED_TTL_MS reference — corrected to PENDING_SEED_TTL_MS. I renamed the constant when its meaning narrowed to the window before the server lists an index, and missed the reference in the state model's doc block.

The M-3 rollback flag is being taken as a documented caveat rather than actioned, which matches your own read of it: the field is additive, the client defaults it with raw?.configHostInherited ?? false, and the worst case after a rollback is a redundant label until the next page load. Flagging it in the release notes instead of adding a compatibility branch for one release.

284 tests passing, lint and format clean.

@KevinDavilaDotCMS KevinDavilaDotCMS left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Non-blocking review — nothing here should hold the PR up. Left a couple of edge cases and some nits inline; the rest is praise, because there is plenty of it.

What I really liked

  • watchIndexOperation is the best decision in the PR: matching on operation and indexName kills the whole class of "dialog settles on someone else's outcome" bugs, and documenting the exception pattern in libs/portlets/CLAUDE.md is exactly where it belongs.
  • I traced the dialog DI chain (PrimeNG 21 DynamicDialogInjector -> the tab's node injector -> the shell's DotAiStore): the claim/notice machinery resolves the same store instance in production, not just under test. The ownership design actually holds at runtime.
  • isInheritedConfig semantics check out against ConfigService.config(host) — including the blank-providerConfig gate for the "nothing configured anywhere" case, which CompletionsResourceTest covers explicitly. Nice to see the fallback path of ConfigService reasoned about rather than assumed.
  • hasOwnProperty vs in in indexStatuses is a genuine catch — the name pattern accepts constructor/toString/valueOf/hasOwnProperty, and any of those would have rendered Building forever.
  • Scoping PENDING_SEED_TTL_MS to the window before the index is listed is the subtle right call: a start-of-build timer would have flipped large builds to Ready mid-flight, which is the exact failure this feature exists to prevent.
  • The empty-state rework is thorough: all eight call sites through dot-empty-container, with the h-full-needs-a-parent-with-height reasoning captured at each restructured call site.
  • Backward compatibility handled on both ends (configHostInherited ?? false client-side) and the {0}/{1} arg swap in build.ok applied consistently across every call site, including the two dialog templates.

One thing to confirm

  • Ticket #37543 AC #2 asked for "remove from the index row"; this PR deliberately puts it in the toolbar, with a reasoned argument (nothing records the query that built an index, so row context would promise something the backend cannot give). The reasoning is sound — just worth an explicit sign-off from QA/product on the deviation, since it is the only AC not met literally.

The two medium-ish items inline (stale notice re-toast on tab re-entry, and the dialog that cannot be dismissed if the store dies mid-flight) are both edge cases — happy to file follow-up issues for either.

@KevinDavilaDotCMS KevinDavilaDotCMS left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Inline comments for the summary review above — all non-blocking.

Comment thread dotCMS/src/main/java/com/dotcms/ai/rest/CompletionsResource.java
Comment thread dotCMS/src/main/webapp/WEB-INF/messages/Language.properties
@fmontes
fmontes added this pull request to the merge queue Sep 16, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 16, 2026
@fmontes
fmontes added this pull request to the merge queue Sep 16, 2026
Merged via the queue into main with commit f5ae6ef Sep 16, 2026
152 checks passed
@fmontes
fmontes deleted the issue-37538-dotai-feedback branch September 16, 2026 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area : Backend PR changes Java/Maven backend code Area : Frontend PR changes Angular/TypeScript frontend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

dotAI portlet: design + QA follow-ups from the Dojo to Angular migration dotAI: split the build/delete dialog and fix silent index actions

3 participants