fix(dotai): design and QA follow-ups from the Angular migration (#37538) - #37541
Conversation
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>
|
Claude finished @fmontes's task in 5m 19s —— View job Code ReviewReviewed the full diff against
New Issues
No blocking issues found. The rollback-safety of the |
`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>
|
Pull Request Unsafe to Rollback!!!
|
|
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>
|
test |
|
(Ignore the previous comment — that was an accidental test post while verifying tool access. Rollback-safety analysis below.) |
|
Pull Request Unsafe to Rollback!!!
|
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>
|
Pull Request Unsafe to Rollback!!!
|
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>
|
Pull Request Unsafe to Rollback!!!
|
…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>
|
Pull Request Unsafe to Rollback!!!
|
…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>
|
Both Medium findings fixed in
The seed map comes from You were right that it is a regression rather than a latent quirk: the code it replaced used Worth noting for anyone reading later: the blast radius stops at the badge. Stale 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 284 tests passing, lint and format clean. |
KevinDavilaDotCMS
left a comment
There was a problem hiding this comment.
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
watchIndexOperationis the best decision in the PR: matching on operation andindexNamekills the whole class of "dialog settles on someone else's outcome" bugs, and documenting the exception pattern inlibs/portlets/CLAUDE.mdis exactly where it belongs.- I traced the dialog DI chain (PrimeNG 21
DynamicDialogInjector-> the tab's node injector -> the shell'sDotAiStore): the claim/notice machinery resolves the same store instance in production, not just under test. The ownership design actually holds at runtime. isInheritedConfigsemantics check out againstConfigService.config(host)— including the blank-providerConfiggate for the "nothing configured anywhere" case, whichCompletionsResourceTestcovers explicitly. Nice to see the fallback path ofConfigServicereasoned about rather than assumed.hasOwnPropertyvsininindexStatusesis a genuine catch — the name pattern acceptsconstructor/toString/valueOf/hasOwnProperty, and any of those would have renderedBuildingforever.- Scoping
PENDING_SEED_TTL_MSto 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 theh-full-needs-a-parent-with-height reasoning captured at each restructured call site. - Backward compatibility handled on both ends (
configHostInherited ?? falseclient-side) and the{0}/{1}arg swap inbuild.okapplied 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
left a comment
There was a problem hiding this comment.
Inline comments for the summary review above — all non-blocking.
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 throughdot-empty-containerfrom@dotcms/ui.They are centred now too, which the first pass got wrong:
dot-empty-containercentres itself withh-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 thecontainer mx-autocolumn 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.
FieldsandVelocity templateexplain themselvesVelocity templatehad no hint and no placeholder at all, and theFieldshint 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 overridesFields, 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 configremovedThe button, its JSON dialog, the two message keys, and the
maskCredentialshelper that had no other caller.03-view-provider-config-removed.webm
4.
Rebuild DBis a plain outlined buttonA 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/confignow returnsconfigHostandconfigHostInheritedas separate fields, derived from whetherConfigServiceactually 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
buildIndexcalledmarkIndexBuildingand thenloadIndexes, and that refresh erased the flag it was meant to act on:applyIndexesrebuilt the status map from the server's list alone, and embedding is asynchronous so the new index is not inindexCountyet. With noBUILDINGstatus 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
indexNoticechannel 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 isempty, not success — the server answers 200 withdeleted: 0.Toasts, as
dot-localesanddot-experimentsdo 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 passingnx affected -t test— 2 267 passing across every affected projectCompletionsResourceTest— 4 new unit tests on the backend rule, including the "nothing configured anywhere" branch it exists fornx affected -t lintclean across 36 projects;nx format:checkcleanblock-editorhas 35 test failures that also fail onmain; 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:
Buildingrow 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.exhaustMapsilently dropped the new one.Open question for whoever owns
EmbeddingsResource: building appends+live:trueto 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 —
closableis 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