[Dashboard Agent] Extract safe dashboard attachment integration refactors#261422
Conversation
rbrtj
left a comment
There was a problem hiding this comment.
great improvements!
left a few questions
Make the shared dashboard converter accept attachment data directly so callers don't need to pass the full attachment wrapper. This keeps the extracted review scoped to the dashboard-agent API cleanup before the larger functionality lands. Made-with: Cursor
Rename the shared dashboard state converter so its API clearly reflects that it returns attachment data. This keeps the extracted dashboard-agent cleanup small and makes later functional review easier. Made-with: Cursor
Move dashboard schema definitions into a dedicated shared module and keep the attachment types file focused on attachment wrappers and re-exports. This further separates non-functional API cleanup from the harder behavior changes that will follow. Made-with: Cursor
Introduce a reusable dashboard attachment type guard and use it in the current dashboard sync and management paths. This keeps attachment narrowing centralized before the larger attachment workflow changes land. Made-with: Cursor
Restore the shared converter comment wording so the extracted dashboard-agent package matches the source branch exactly. This keeps the cleanup stack consistent before we move on to functional changes. Made-with: Cursor
Use the shared dashboard section grid schema in manage dashboard operations instead of maintaining a duplicate local definition. This keeps dashboard operation validation aligned with the shared attachment schema. Made-with: Cursor
Rename the dashboard preview helper and align its preview flow with the source branch behavior. This keeps the public integration cleanup separate from the larger dashboard app integration changes. Made-with: Cursor
Introduce the dashboard app live updates subscription as a standalone extraction from the source branch. This keeps the new subscription logic reviewable before wiring in the wider app integration flow. Made-with: Cursor
Introduce the dashboard origin sync subscription and compose it inside the existing mount sync helper. This keeps the new origin sync logic isolated while preserving the target branch's current mount integration shape. Made-with: Cursor
Keep dashboard attachment origins aligned with saves and deleted dashboards without pulling in the manual changes subscription refactor. Made-with: Cursor
Align the dashboard attachment manual-sync wiring with the extracted subscription helper so mount sync composes the target structure more directly. Made-with: Cursor
Rename the dashboard app integration entrypoint and align its API with the target structure while preserving the current origin and manual sync behavior. Made-with: Cursor
Move dashboard attachment mount wiring into the dashboard app integration flow, remove the temporary on-attachment mount wrapper, and keep the latest origin sync adjustments together with that cleanup. Made-with: Cursor
0c8fa5f to
148c2fa
Compare
| const linkedDashboardExists = attachment.origin | ||
| ? await checkSavedDashboardExist(attachment.origin) | ||
| : false; | ||
|
|
||
| if (dashboardHasBeenDeleted) { | ||
| await updateOrigin(''); | ||
| attachmentLinkedSavedObjectId = undefined; | ||
| } | ||
|
|
||
| // b) Viewing saved dashboard + attachment not linked -> navigate to new unsaved dashboard | ||
| if (!attachmentLinkedSavedObjectId && !currentSavedObjectId) { | ||
| if (!currentSavedObjectId && !linkedDashboardExists) { | ||
| // b) Viewing unsaved dashboard + attachment linked to deleted dashboard | ||
| dashboardApi.setState(dashboardState); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🟢 Low dashboard_integration/preview_attachment.ts:36
The updateOrigin parameter is passed to previewAttachmentInDashboard but never called. When attachment.origin references a deleted dashboard, the function no longer clears the stale reference, leaving attachment.origin pointing to a non-existent dashboard indefinitely. Consider restoring the updateOrigin('') call when linkedDashboardExists is false.
- const linkedDashboardExists = attachment.origin
+ let linkedDashboardExists = false;
+ if (attachment.origin) {
+ linkedDashboardExists = await checkSavedDashboardExist(attachment.origin);
+ if (!linkedDashboardExists) {
+ await updateOrigin('');
+ }
+ }
if (!currentSavedObjectId && !linkedDashboardExists) {🤖 Copy this AI Prompt to have your agent fix this:
In file x-pack/platform/plugins/shared/dashboard_agent/public/attachment_types/dashboard_integration/preview_attachment.ts around lines 36-44:
The `updateOrigin` parameter is passed to `previewAttachmentInDashboard` but never called. When `attachment.origin` references a deleted dashboard, the function no longer clears the stale reference, leaving `attachment.origin` pointing to a non-existent dashboard indefinitely. Consider restoring the `updateOrigin('')` call when `linkedDashboardExists` is false.
Evidence trail:
x-pack/platform/plugins/shared/dashboard_agent/public/attachment_types/dashboard_integration/preview_attachment.ts lines 16, 23 (parameter defined and destructured but never called in function body lines 24-54). git_diff MERGE_BASE..REVIEWED_COMMIT shows this is a new file. git_grep for 'updateOrigin' shows it IS called in other files like use_register_canvas_action_buttons.ts:99,104 and origin_sync_subscription.ts:45,57, demonstrating intended usage pattern.
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Module Count
Public APIs missing comments
Async chunks
History
|
* commit 'bfc2446fdbcba2b3183f4518817c9757198c95ef': [Cascade] make cascade layout enabled by default (elastic#260698) [Dashboard Agent] Extract safe dashboard attachment integration refactors (elastic#261422) [One Workflow] Replace workflows:aiAgent:enabled with agentBuilder:experimentalFeatures (elastic#261330) [EDR Workflows] Osquery: hide query code from dropdown and show Elastic for automated Run By (elastic#261394) [Observability Onboarding] Add data detection & loading indicators to onboarding flows (elastic#257870) [Significant events] Format event count with locale-aware number separators (elastic#261570) [Fleet] Fix deprecated filter in browse integrations (elastic#261459) [Lens as code] Split `xyStateSchema` config (elastic#261089) [Data Views as Code] Use `ref_id` and add metadata in data views schemas (elastic#261181) Made-with: Cursor # Conflicts: # x-pack/platform/packages/shared/dashboard-agent/dashboard-agent-common/types.ts
…r-uid-to-id * commit '2dc1eb699581a0b24f3b433de8db41d312bc5c93': [Dashboard Agent] Extract safe dashboard attachment integration refactors (elastic#261422) # Conflicts: # x-pack/platform/packages/shared/dashboard-agent/dashboard-agent-common/types.ts
Description
This PR pulls out the lower-risk dashboard agent refactors from the larger source work so the remaining session-management changes can be reviewed separately.
Summary
attachmentToDashboardState->attachmentDataToDashboardStatedashboardStateToAttachment->dashboardStateToAttachmentDatatypes.tsintodashboard_schema_types.ts, and reuse the shared section grid schema from the common package.isDashboardAttachmenttype guard and adopt it where dashboard attachments are handled.preview_attachment_in_dashboardtopreview_attachmentand simplify preview/origin handling around deleted linked dashboards.agent_live_updates_subscription.tsorigin_sync_subscription.tsmanual_changes_subscription.tsdashboard_app_integration.tson_attachment_mountwrapper (not used in the future) and inline dashboard app integration wiring into attachment registration.Why
These changes isolate naming cleanups, shared-type extraction, and dashboard integration decomposition before the larger attachment session refactor lands. That should make the remaining review focus on the harder behavioral changes instead of mechanical moves.