Saved object annotation

[ruanyl]

Description

As the number of dashboards and visualizations grows, finding and organizing saved objects by title alone becomes difficult. This PR introduces saved object tags so users can classify dashboards with reusable, colored labels and filter the dashboard list by tag.

Rather than adding a tag-specific field and lifecycle logic to every saved object type, this PR introduces a generic annotation framework in the Saved Objects service. Tags are the first annotation type built on this framework.

How the annotation framework works

An annotation definition is stored as a regular saved-object-annotation saved object. It contains a type, name, optional description, and optional payload for annotation-specific metadata. Tags use the payload to store their color.

Plugins register annotation types during setup and declare which saved object types they support. The tags plugin registers the tag annotation type for dashboards and visualizations and requires tag names to be unique.

Annotations are attached to target objects through saved object references. For example, assigning a tag to a dashboard adds a reference from the dashboard to the tag definition. This allows the existing Saved Objects infrastructure to handle relationships, export and import, and filtering through hasReference.

The core annotation service provides operations to:

• Create, update, find, and delete annotation definitions.
• Attach and remove annotations from supported saved objects.
• Retrieve annotations assigned to a saved object.
• Validate annotation types and supported target object types.
• Remove references from target objects before deleting an annotation.

Saved object writers do not normally know about references owned by other features. To prevent tags from disappearing when a dashboard is saved or overwritten, core preserves persisted annotation references across regular create, update, bulk-create, and bulk-update operations. Annotation mutations use an internal client path so explicitly removing a reference still works.

The framework is exposed through request-scoped server services, internal HTTP routes, and a browser client. Additional annotation types can reuse the same lifecycle and attachment behavior without introducing custom fields on every target saved object type.

Dashboard integration

The initial UI integration focuses on dashboards. Users can create, assign, and remove colored tags from the dashboard header in either view or edit mode. Hovering over the tag action displays the currently assigned tags.

The dashboard listing includes a Tags column and supports filtering dashboards by a selected tag. Tag options and selected values are consistently rendered as colored labels.

Issues Resolved

Screenshot

Screen.Recording.2026-09-02.at.14.51.30.mov

Testing the changes

Check List

• All tests pass
  • yarn test:jest
  • yarn test:jest_integration
• New functionality includes testing.
• New functionality has been documented.
• Commits are signed per the DCO using --signoff

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 8ef9b52.

⛔ Hard block: Issues at High severity or above will block this PR from merging.

Path	Line	Severity	Description
src/core/server/saved_objects/annotations/routes.ts	16	medium	payloadSchema uses schema.recordOf(schema.string(), schema.any()), allowing arbitrary values to be stored in annotation payloads with no type constraints. Combined with the JSON.parse/stringify round-trip in the service layer, this permits storing unexpected data types that downstream consumers may render unsafely.
src/plugins/saved_object_tags/public/components/tag_list.tsx	66	low	Tag color values retrieved from the annotation payload are passed directly as the EuiBadge color prop without sanitization. A user with write access to annotations could store a crafted color string; if EUI ever passes it to a CSS property without sanitization, this could enable CSS injection.
src/plugins/navigation/public/top_nav_menu/top_nav_menu_data.tsx	47	low	The tooltip field type was broadened from string to ReactNode, allowing arbitrary React elements to be rendered in top-nav tooltips. While the current callers supply application-controlled content, the wider surface increases risk if user-controlled data is ever passed as a tooltip without sanitization.

The table above displays the top 10 most important findings.

Total: 3 | Critical: 0 | High: 0 | Medium: 1 | Low: 2

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit fa4ebef)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Core saved object annotation framework Relevant files: src/core/public/saved_objects/saved_object_annotation_client.ts src/core/public/saved_objects/saved_object_annotation_client.test.ts src/core/public/saved_objects/saved_objects_service.ts src/core/public/saved_objects/saved_objects_service.mock.ts src/core/public/saved_objects/index.ts src/core/public/index.ts src/core/server/saved_objects/annotations/index.ts src/core/server/saved_objects/annotations/registry.ts src/core/server/saved_objects/annotations/registry.test.ts src/core/server/saved_objects/annotations/reference_preservation_wrapper.ts src/core/server/saved_objects/annotations/reference_preservation_wrapper.test.ts src/core/server/saved_objects/annotations/reference_utils.ts src/core/server/saved_objects/annotations/routes.ts src/core/server/saved_objects/annotations/routes.test.ts src/core/server/saved_objects/annotations/saved_object_type.ts src/core/server/saved_objects/annotations/service.ts src/core/server/saved_objects/annotations/service.test.ts src/core/server/saved_objects/saved_objects_config.ts src/core/server/saved_objects/saved_objects_config.test.ts src/core/server/saved_objects/saved_objects_service.ts src/core/server/saved_objects/saved_objects_service.mock.ts src/core/server/saved_objects/saved_objects_service.test.ts src/core/server/saved_objects/index.ts src/core/server/core_route_handler_context.ts src/core/server/core_route_handler_context.test.ts src/core/server/legacy/legacy_service.ts src/core/server/plugins/plugin_context.ts src/core/server/mocks.ts src/core/server/index.ts src/core/types/index.ts src/core/types/saved_object_annotations.ts config/opensearch_dashboards.yml Sub-PR theme: Saved object tags plugin Relevant files: src/plugins/saved_object_tags/common/index.ts src/plugins/saved_object_tags/opensearch_dashboards.json src/plugins/saved_object_tags/public/index.ts src/plugins/saved_object_tags/public/plugin.tsx src/plugins/saved_object_tags/public/types.ts src/plugins/saved_object_tags/public/components/index.ts src/plugins/saved_object_tags/public/components/tag_assignment_modal.tsx src/plugins/saved_object_tags/public/components/tag_assignment_modal.test.tsx src/plugins/saved_object_tags/public/components/tag_list.tsx src/plugins/saved_object_tags/public/components/tag_list.test.tsx src/plugins/saved_object_tags/public/components/tag_selector.tsx src/plugins/saved_object_tags/public/components/tag_selector.test.tsx src/plugins/saved_object_tags/public/components/tag_option.tsx src/plugins/saved_object_tags/server/index.ts src/plugins/saved_object_tags/server/config.ts src/plugins/saved_object_tags/server/plugin.ts src/plugins/saved_object_tags/server/plugin.test.ts Sub-PR theme: Dashboard UI tag integration and shared UI plumbing Relevant files: src/plugins/dashboard/opensearch_dashboards.json src/plugins/dashboard/public/plugin.tsx src/plugins/dashboard/public/types.ts src/plugins/dashboard/public/application/app.scss src/plugins/dashboard/public/application/components/dashboard_listing/dashboard_listing.tsx src/plugins/dashboard/public/application/components/dashboard_listing/dashboard_listing.test.tsx src/plugins/dashboard/public/application/components/dashboard_top_nav/dashboard_top_nav.tsx src/plugins/dashboard/public/application/components/dashboard_top_nav/dashboard_top_nav.test.tsx src/plugins/dashboard/public/application/components/dashboard_top_nav/dashboard_tag_list_tooltip.tsx src/plugins/dashboard/public/application/components/dashboard_top_nav/dashboard_tag_list_tooltip.test.tsx src/plugins/dashboard/public/application/components/dashboard_top_nav/top_nav/get_top_nav_config.ts src/plugins/dashboard/public/application/components/dashboard_top_nav/top_nav/get_top_nav_config.test.ts src/plugins/dashboard/public/application/components/dashboard_top_nav/top_nav/top_nav_ids.ts src/plugins/dashboard/public/application/utils/mocks.ts src/plugins/navigation/public/top_nav_menu/top_nav_menu_data.tsx src/plugins/navigation/public/top_nav_menu/top_nav_menu_item.tsx src/plugins/navigation/public/top_nav_menu/top_nav_menu_item.test.tsx src/plugins/opensearch_dashboards_react/public/table_list_view/table_list_view.tsx src/plugins/opensearch_dashboards_react/public/table_list_view/table_list_view.test.tsx

⚡ Recommended focus areas for review Possible Issue updateAnnotation sets attributes.description = input.description when the caller included description (even as undefined), and similarly serializes payload as JSON.stringify(undefined) producing the string "undefined" which will fail to parse on read. If a client sends { description: undefined } or { payload: undefined } explicitly, the persisted annotation becomes corrupt (payload) or clears fields unintentionally. Consider skipping the field when the value is undefined, mirroring serializeCreateInput. if (hasOwn(input, 'description')) { attributes.description = input.description; } if (hasOwn(input, 'payload')) { attributes.payload = JSON.stringify(input.payload); } Pagination Bug findAll iterates pages using savedObjects.length < response.total, but response.total reflects the total matching, not the number of items appended, and each iteration ignores duplicates or reordering. More importantly, the loop increments page without any exit safety when response.saved_objects is empty (e.g., permissions filter out results), which would loop indefinitely because total never decreases while savedObjects.length never grows. Consider breaking when a page returns zero items. private async findAll<T = unknown>( options: Omit<Parameters<SavedObjectsClientContract['find']>[0], 'page' | 'perPage'> ): Promise<Array<SavedObject<T>>> { const savedObjects: Array<SavedObject<T>> = []; let page = 1; let response: SavedObjectsFindResponse<T>; do { response = await this.client.find<T>({ ...options, page, perPage: FIND_PAGE_SIZE, }); savedObjects.push(...response.saved_objects); page += 1; } while (savedObjects.length < response.total); Missing Route Registration The router registers POST at empty path '' and POST at /_find, but there is no route to list annotations via GET, and the DELETE handler requires the type in the query string while the client sends it as a query param — verify the DELETE query schema matches the client encoding. Additionally, the create/update body allows arbitrary payload: recordOf(string, any), which is later JSON.stringify'd and stored; there is no size limit, allowing a client to store arbitrarily large payloads on a saved object with index: false but consuming index space. router.post( { path: '', validate: { body: schema.object({ type: schema.string(), name: schema.string(), description: schema.maybe(schema.string()), payload: schema.maybe(payloadSchema), }), }, }, router.handleLegacyErrors(async (context, request, response) => { const annotation = await context.core.savedObjects.annotations.createAnnotation(request.body); return response.ok({ body: annotation }); }) ); router.put( { path: '/{annotationId}', validate: { params: schema.object({ annotationId: schema.string(), }), body: schema.object({ type: schema.string(), name: schema.maybe(schema.string()), description: schema.maybe(schema.string()), payload: schema.maybe(payloadSchema), }), }, }, router.handleLegacyErrors(async (context, request, response) => { const annotation = await context.core.savedObjects.annotations.updateAnnotation({ annotationId: request.params.annotationId, ...request.body, }); return response.ok({ body: annotation }); }) ); router.delete( { path: '/{annotationId}', validate: { params: schema.object({ annotationId: schema.string(), }), query: schema.object({ type: schema.string(), }), }, }, router.handleLegacyErrors(async (context, request, response) => { await context.core.savedObjects.annotations.deleteAnnotation({ annotationId: request.params.annotationId, type: request.query.type, }); return response.ok(); }) ); router.post( { path: '/_find', validate: { body: schema.object({ type: schema.string(), }), }, }, router.handleLegacyErrors(async (context, request, response) => { const annotations = await context.core.savedObjects.annotations.findAnnotations(request.body); return response.ok({ body: annotations }); }) ); Missing Return The start method's return { client, annotations } now exists, but confirm the diff wasn't showing a prior return at the removed line—if the method previously did return { client: ... } in a single line and the new implementation replaces it, the shown code appears correct. However the visible __old hunk__ shows only the return line was replaced; ensure no other side-effects (e.g., subscribing to config) were dropped. This is a low-confidence observation and depends on surrounding code not shown. public async start({ http }: { http: CoreStart['http'] }): Promise<SavedObjectsStart> { return { client: new SavedObjectsClient(http), annotations: new SavedObjectAnnotationClient(http), }; } Potential Data Loss preserveReferences calls client.bulkGet for objects being written. If bulkGet returns a non-404 error for a target (e.g., a transient 503), the wrapper throws, which will block otherwise-valid writes in a bulk operation. For bulkCreate/bulkUpdate this converts a partial-success operation into a total failure. Consider either passing the write through unmodified in that case (with a documented risk of losing annotation references) or surfacing the error only on the affected object rather than failing the whole batch. objectIndexesToResolve.forEach((objectIndex, persistedObjectIndex) => { const object = objects[objectIndex]; const persistedObject = persistedObjects[persistedObjectIndex]; if (persistedObject.error) { // A missing target has no annotation references to preserve. Leave it unchanged // and let create, update, or bulkUpdate apply its normal not-found behavior. if (persistedObject.error.statusCode === 404) { return; } // Other lookup errors may hide an existing target, so stop instead of risking // an update that unintentionally removes its annotation references. throw new Error(persistedObject.error.message); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to fa4ebef

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Prevent infinite pagination loop If find returns zero results while total > 0 (e.g. permissions filtering results out), this loop becomes infinite. Also, total can decrease between pages, causing missed data or extra requests. Add a guard that breaks when a page returns no results to prevent an infinite loop. src/core/server/saved_objects/annotations/service.ts [415-433] private async findAll<T = unknown>( options: Omit<Parameters<SavedObjectsClientContract['find']>[0], 'page' | 'perPage'> ): Promise<Array<SavedObject<T>>> { const savedObjects: Array<SavedObject<T>> = []; let page = 1; let response: SavedObjectsFindResponse<T>; do { response = await this.client.find<T>({ ...options, page, perPage: FIND_PAGE_SIZE, }); + if (!response.saved_objects.length) { + break; + } savedObjects.push(...response.saved_objects); page += 1; } while (savedObjects.length < response.total); return savedObjects; } Suggestion importance[1-10]: 7 __ Why: Good defensive coding suggestion; an infinite loop could occur if find returns zero results with non-zero total due to permission filtering, which is a legitimate concern.	Medium
	Preserve this for non-overridden client methods Object.create(client) creates a new object whose prototype is client, but methods on client typically rely on this referring to the client instance. When downstream code calls e.g. wrapper.delete(...) (not overridden here), it dispatches to client.delete with this set to the wrapper object, not the client, which can break clients that use private fields or internal state. Prefer delegating explicitly or using a Proxy, or bind all non-overridden methods to client. src/core/server/saved_objects/annotations/reference_preservation_wrapper.ts [150-155] -return Object.assign(Object.create(client), { - create, - bulkCreate, - update, - bulkUpdate, - }); +const wrapper = Object.create(client); +Object.getOwnPropertyNames(Object.getPrototypeOf(client)).forEach((key) => { + const value = (client as any)[key]; + if (typeof value === 'function' && !['create', 'bulkCreate', 'update', 'bulkUpdate'].includes(key)) { + wrapper[key] = value.bind(client); + } +}); +return Object.assign(wrapper, { create, bulkCreate, update, bulkUpdate }); Suggestion importance[1-10]: 5 __ Why: Valid concern about this binding when using Object.create(client) as prototype. However, this is a common pattern in the codebase for client wrappers, and JavaScript prototype methods typically work with dynamic this, so the actual impact may be limited.	Low
Security	Avoid KQL injection via filter string The findAnnotations filter concatenates the user-controlled type (already partly protected via JSON.stringify) into a KQL string, but the query never validates the type against the registry beyond this.registry.get(type). More importantly, JSON.stringify is not a safe escaping mechanism for KQL — special KQL characters inside the string could still change semantics. Consider filtering by a stricter, registry-verified value or using a term-level query rather than KQL string interpolation. src/core/server/saved_objects/annotations/service.ts [142-151] public async findAnnotations({ type, }: FindSavedObjectAnnotationsOptions): Promise<SavedObjectAnnotation[]> { - this.registry.get(type); + const registration = this.registry.get(type); const savedObjects = await this.findAll<SavedObjectAnnotationAttributes>({ type: SAVED_OBJECT_ANNOTATION_TYPE, - filter: `${SAVED_OBJECT_ANNOTATION_TYPE}.attributes.type: ${JSON.stringify(type)}`, + filter: `${SAVED_OBJECT_ANNOTATION_TYPE}.attributes.type: "${registration.type.replace(/"/g, '\\"')}"`, }); return savedObjects.map((savedObject) => this.deserialize(savedObject)); } Suggestion importance[1-10]: 6 __ Why: Valid concern about KQL escaping via JSON.stringify; the improved code uses the registry-verified type value, which is safer. However, since type is validated via this.registry.get(type) first, actual injection risk is limited.	Low
General	Skip no-op reference updates selectedAnnotationIds is a Set, so iterating it during the final "add selected annotations" loop drops duplicates from the input array silently, but it also loses the caller's ordering intent and — more importantly — deduplication of annotationIds should be an explicit contract. Also, if annotationIds is empty the code still performs a bulkGet for existing references only, which is correct, but the update always runs even when references are unchanged, causing unnecessary writes and version bumps. Consider short-circuiting when the resulting references match the current ones. src/core/server/saved_objects/annotations/service.ts [220-305] public async setAnnotationsForObject({ annotationIds, type, target, }: SetSavedObjectAnnotationsForObjectInput): Promise<void> { - // 1. Validate the target and load its current references. this.validateTarget(type, target.objectType); const targetObject = await this.mutationClient.get(target.objectType, target.objectId); const selectedAnnotationIds = new Set(annotationIds); + // ... build references ... + const referencesUnchanged = + references.length === targetObject.references.length && + references.every((ref, i) => + ref.id === targetObject.references[i].id && ref.type === targetObject.references[i].type + ); + if (referencesUnchanged) { + return; + } Suggestion importance[1-10]: 4 __ Why: Optimization suggestion to avoid unnecessary writes; reasonable but marginal improvement and the improved_code is incomplete/pseudo-code.	Low

---

Previous suggestions

Suggestions up to commit 413d10e

Category	Suggestion	Impact
Possible issue	Add missing React import for JSX file This file uses JSX syntax but is missing the React import. Without import React from 'react', the JSX in the start() method (e.g., <TagSelector ... />) will fail to compile in the classic JSX runtime used elsewhere in this codebase. Add the React import to prevent a build/runtime failure. src/plugins/saved_object_tags/public/plugin.tsx [6-8] +import React from 'react'; import { CoreStart, Plugin } from '../../../core/public'; import { TagAssignmentModal, TagList, TagSelector } from './components'; import { SavedObjectTagsStart } from './types'; Suggestion importance[1-10]: 9 __ Why: The file uses JSX in the start() method but does not import React, which would cause a compile failure with the classic JSX runtime. This is a critical correctness issue.	High
	Prevent infinite loop in pagination helper The findAll loop's termination condition savedObjects.length < response.total can loop forever if total never decreases but a page returns zero results (e.g., due to a filter mismatch or concurrent deletion), because page keeps increasing while savedObjects.length never reaches total. Add a safeguard to break when a page returns empty results. src/core/server/saved_objects/annotations/service.ts [404-423] -public async findAnnotations({ - type, -}: FindSavedObjectAnnotationsOptions): Promise<SavedObjectAnnotation[]> { - this.registry.get(type); - const savedObjects = await this.findAll<SavedObjectAnnotationAttributes>({ - type: SAVED_OBJECT_ANNOTATION_TYPE, - filter: `${SAVED_OBJECT_ANNOTATION_TYPE}.attributes.type: ${JSON.stringify(type)}`, - }); - return savedObjects.map((savedObject) => this.deserialize(savedObject)); +private async findAll<T = unknown>( + options: Omit<Parameters<SavedObjectsClientContract['find']>[0], 'page' | 'perPage'> +): Promise<Array<SavedObject<T>>> { + const savedObjects: Array<SavedObject<T>> = []; + let page = 1; + let response: SavedObjectsFindResponse<T>; + + do { + response = await this.client.find<T>({ + ...options, + page, + perPage: FIND_PAGE_SIZE, + }); + savedObjects.push(...response.saved_objects); + if (response.saved_objects.length === 0) { + break; + } + page += 1; + } while (savedObjects.length < response.total); + + return savedObjects; } Suggestion importance[1-10]: 7 __ Why: Adding a safeguard against an empty page prevents a potential infinite loop if total stays higher than the returned results, which improves robustness of pagination.	Medium
General	Skip bulkGet when no references provided bulkUpdate unconditionally invokes preserveReferences and thus issues a bulkGet even when no object in the batch specifies references. preserveReferences internally guards against this, but the initial filter still runs and the wrapper always reads persisted state. Consider skipping the call entirely when none of the objects include references, matching the pattern used in update for consistency and to avoid an unnecessary extra read for callers that omit references. src/core/server/saved_objects/annotations/reference_preservation_wrapper.ts [141-148] const bulkUpdate: typeof client.bulkUpdate = async ( objects: SavedObjectsBulkUpdateObject[] = [], options = {} ) => { + if (!objects.some((object) => Array.isArray(object.references))) { + return client.bulkUpdate(objects, options); + } const objectsWithPreservedReferences = await preserveReferences(objects); return client.bulkUpdate(objectsWithPreservedReferences, options); }; Suggestion importance[1-10]: 4 __ Why: A minor optimization for consistency with update. The internal preserveReferences already skips the bulkGet when no eligible objects exist, so functional impact is minimal.	Low

Suggestions up to commit 5bbe312

Category	Suggestion	Impact
Possible issue	Import React in a TSX file using JSX The file has a .tsx extension and uses JSX (<TagSelector ... />) inside factory functions, but it is missing the import React from 'react' import. Without React in scope, TSX compilation of these arrow functions will fail at build time. Add the React import at the top of the file. src/plugins/saved_object_tags/public/plugin.tsx [6-13] +import React from 'react'; import { CoreStart, Plugin } from '../../../core/public'; import { TagAssignmentModal, TagList, TagSelector } from './components'; import { SavedObjectTagsStart } from './types'; export class SavedObjectTagsPlugin implements Plugin<void, SavedObjectTagsStart> { public setup() {} public start(core: CoreStart): SavedObjectTagsStart { Suggestion importance[1-10]: 9 __ Why: The file uses JSX syntax in arrow functions but does not import React, which will cause compilation to fail in a TSX file. This is a correct and critical fix.	High
	Prevent duplicate annotation references When a previously-assigned annotation reference has a non-404 error (e.g. 403), the code preserves it but then step 5 unconditionally appends every selected annotation ID, which can create a duplicate reference for the same annotation ID (once preserved, once added). Filter selected IDs to skip those already retained in references to avoid duplicate reference entries pointing to the same annotation. src/core/server/saved_objects/annotations/service.ts [257-283] -// 4. Preserve unrelated references while removing this type and missing annotations. const references = targetObject.references.filter((reference) => { if (!isSavedObjectAnnotationReference(reference)) { return true; } const annotation = annotationsById.get(reference.id); - // Only missing definitions are orphaned; other lookup errors must not delete references. if (!annotation || annotation.error?.statusCode === 404) { return false; } if (annotation.error) { return true; } if (annotation.attributes.type !== type) { return true; } return false; }); -// 5. Add the complete selected annotation set with fresh reference names. +const alreadyReferencedIds = new Set( + references.filter(isSavedObjectAnnotationReference).map(({ id }) => id) +); selectedAnnotationIds.forEach((annotationId) => { + if (alreadyReferencedIds.has(annotationId)) { + return; + } references.push({ name: createSavedObjectAnnotationReferenceName(references), type: SAVED_OBJECT_ANNOTATION_TYPE, id: annotationId, }); }); Suggestion importance[1-10]: 7 __ Why: Valid edge case: when a preserved reference has a non-404 error, and the same annotation ID is in the selection, it could be added twice. The fix prevents duplicate references.	Medium
	Prevent infinite pagination loop If find returns zero results but a non-zero total (which can happen with permission filtering or race conditions), the loop will spin infinitely fetching empty pages. Add a safeguard to break the loop when a page returns no results. src/core/server/saved_objects/annotations/service.ts [404-422] private async findAll<T = unknown>( options: Omit<Parameters<SavedObjectsClientContract['find']>[0], 'page' | 'perPage'> ): Promise<Array<SavedObject<T>>> { const savedObjects: Array<SavedObject<T>> = []; let page = 1; let response: SavedObjectsFindResponse<T>; do { response = await this.client.find<T>({ ...options, page, perPage: FIND_PAGE_SIZE, }); + if (response.saved_objects.length === 0) { + break; + } savedObjects.push(...response.saved_objects); page += 1; } while (savedObjects.length < response.total); return savedObjects; } Suggestion importance[1-10]: 6 __ Why: A reasonable defensive safeguard against infinite loops when total doesn't match returned results, though this is an edge case dependent on client behavior.	Low
General	Ensure annotation writes bypass preservation wrapper If the caller passes references that already contains annotation references, preserveSavedObjectAnnotationReferences drops those in favor of the persisted ones, meaning the annotation service's own update calls (which set references intentionally) will lose their new annotation references through this wrapper. Ensure the annotation service bypasses this wrapper (via excludedWrappers) whenever it writes annotation references; verify that the mutation client path in SavedObjectAnnotationServiceImpl.setAnnotationsForObject/addAnnotationToObject uses mutationClient (which excludes the wrapper) — good — but note that deleteAnnotation calls this.mutationClient.bulkUpdate while other reads use this.client; confirm all annotation-mutating writes go through mutationClient to avoid the wrapper re-adding just-deleted references. src/core/server/saved_objects/annotations/reference_preservation_wrapper.ts [87-105] -const update: typeof client.update = async ( - type, - id, - attributes, - options: SavedObjectsUpdateOptions = {} - ) => { - if (!Array.isArray(options.references)) { - return client.update(type, id, attributes, options); - } +// Verify all annotation-reference mutating writes in SavedObjectAnnotationServiceImpl +// use `mutationClient` (excludedWrappers: [ANNOTATION_REFERENCE_PRESERVATION_WRAPPER_ID]) +// so the wrapper cannot re-introduce dropped annotation references. - const persistedObject = await client.get(type, id); - return client.update(type, id, attributes, { - ...options, - references: preserveSavedObjectAnnotationReferences( - persistedObject.references, - options.references - ), - }); - }; - Suggestion importance[1-10]: 3 __ Why: The suggestion only asks to verify existing behavior, and the improved_code is just a comment rather than an actual change. Lower impact per scoring guidelines.	Low

Suggestions up to commit 8ef9b52

Category	Suggestion	Impact
Possible issue	Prevent infinite loop in pagination The pagination loop can become an infinite loop if response.total is greater than 0 but the current page returns 0 results (e.g., due to permissions filtering or inconsistent counts). Add a guard to break when a page returns no items to prevent hanging requests. src/core/server/saved_objects/annotations/service.ts [323-331] do { response = await this.client.find<T>({ ...options, page, perPage: FIND_PAGE_SIZE, }); savedObjects.push(...response.saved_objects); page += 1; + if (response.saved_objects.length === 0) { + break; + } } while (savedObjects.length < response.total); Suggestion importance[1-10]: 7 __ Why: Valid defensive check; if a page returns 0 items but total is still greater than the accumulated count (due to filtering or race conditions), the loop would hang. Adding a break is a reasonable safeguard.	Medium
	Fix wrapper prototype/this-binding issue Using Object.create(client) with Object.assign sets client as the prototype rather than copying methods; methods inherited via prototype will be called with the wrapper as this, which may break methods that access private fields on the original client. Prefer explicitly spreading/binding the client methods (or extending via a proxy) to preserve this binding correctly. src/core/server/saved_objects/annotations/reference_preservation_wrapper.ts [131-137] export const annotationReferencePreservationWrapper: SavedObjectsClientWrapperFactory = ({ client, }) => { ... - return Object.assign(Object.create(client), { + return { + ...client, create, bulkCreate, update, bulkUpdate, - }); + }; }; Suggestion importance[1-10]: 5 __ Why: The concern about this binding is partially valid, but the improved_code using spread on a class instance would lose methods entirely (since class methods live on the prototype). The proposed fix is worse than the original, though the underlying concern has some merit.	Low
Security	Correctly escape KQL filter value The KQL filter builds a value using JSON.stringify(type), but KQL uses its own quoting/escaping rules, not JSON's. Values containing characters like : or backslashes may not be correctly escaped, causing malformed filters or, worse, filter injection. Use the KQL builder utilities or properly escape the value for KQL syntax. src/core/server/saved_objects/annotations/service.ts [130-139] public async findAnnotations({ type, }: FindSavedObjectAnnotationsOptions): Promise<SavedObjectAnnotation[]> { this.registry.get(type); + const escapedType = `"${type.replace(/\\/g, '\\\\').replace(/"/g, '\\"')}"`; const savedObjects = await this.findAll<SavedObjectAnnotationAttributes>({ type: SAVED_OBJECT_ANNOTATION_TYPE, - filter: `${SAVED_OBJECT_ANNOTATION_TYPE}.attributes.type: ${JSON.stringify(type)}`, + filter: `${SAVED_OBJECT_ANNOTATION_TYPE}.attributes.type: ${escapedType}`, }); return savedObjects.map((savedObject) => this.deserialize(savedObject)); } Suggestion importance[1-10]: 7 __ Why: Legitimate concern: JSON.stringify isn't the correct escaping mechanism for KQL, and while it works for most simple strings, edge cases with special characters could cause issues. The improved code provides a more correct escaping approach.	Medium
General	Notify caller on partial failures If a partial failure occurs during add/remove, onChange is not called but the previously succeeded operations are not rolled back or reflected. Consider invoking onChange?.() in the error branch (or before rethrowing) so callers can refresh their view to reflect the partial state, avoiding stale UI. src/plugins/saved_object_tags/public/components/tag_assignment_modal.tsx [132-148] try { for (const annotationId of tagIdsToAdd) { await annotationService.addAnnotationToObject({ annotationId, type: TAG_ANNOTATION_TYPE, target: { objectId, objectType }, }); } for (const annotationId of tagIdsToRemove) { await annotationService.removeAnnotationFromObject({ annotationId, type: TAG_ANNOTATION_TYPE, target: { objectId, objectType }, }); } + // ... (existing create-and-assign block) +} catch (error) { + onChange?.(); + setErrorMessage(getErrorMessage(error)); + setIsSaving(false); +} Suggestion importance[1-10]: 5 __ Why: Reasonable UX improvement to reflect partial state changes on error, though not critical. Callers may benefit from being notified to refresh their view.	Low

[github-actions]

🔗 Workflow run · commit fa4ebef2051304b62bf92772ed753161d7655dc2

❌ 20 Jest Test Failure(s)

📄 junit-jest-group5-Linux/TEST-Jest Tests.xml

❌ VariableService initialize and getVariables should return empty array when not initialized (0.014s)

Jest Tests.src/plugins/dashboard/public/variables

Error: expect(received).toMatchSnapshot()

Snapshot name: `Dashboard top nav render with all components 1`

- Snapshot  - 0
+ Received  + 1

@@ -973,10 +973,11 @@
                  "createAnnotation": [MockFunction],
                  "deleteAnnotation": [MockFunction],
  … (11 more lines)

❌ Dashboard top nav render in full screen mode, no componenets should be shown (0.023s)

Jest Tests.src/plugins/dashboard/public/application/components/dashboard_top_nav

Error: expect(received).toMatchSnapshot()

Snapshot name: `Dashboard top nav render in full screen mode, no componenets should be shown 1`

- Snapshot  - 0
+ Received  + 1

@@ -973,10 +973,11 @@
                  "createAnnotation": [MockFunction],
                  "deleteAnnotation": [MockFunction],
  … (11 more lines)

❌ Dashboard top nav render in full screen mode with appended URL param but none of the componenets can be forced show (0.042s)

Jest Tests.src/plugins/dashboard/public/application/components/dashboard_top_nav

Error: expect(received).toMatchSnapshot()

Snapshot name: `Dashboard top nav render in full screen mode with appended URL param but none of the componenets can be forced show 1`

- Snapshot  - 0
+ Received  + 1

@@ -973,10 +973,11 @@
                  "createAnnotation": [MockFunction],
                  "deleteAnnotation": [MockFunction],
  … (11 more lines)

❌ Dashboard top nav render in embed mode (0.022s)

Jest Tests.src/plugins/dashboard/public/application/components/dashboard_top_nav

Error: expect(received).toMatchSnapshot()

Snapshot name: `Dashboard top nav render in embed mode 1`

- Snapshot  - 0
+ Received  + 1

@@ -973,10 +973,11 @@
                  "createAnnotation": [MockFunction],
                  "deleteAnnotation": [MockFunction],
  … (11 more lines)

❌ Dashboard top nav render in embed mode, components can be forced show by appending URL param (0.019s)

Jest Tests.src/plugins/dashboard/public/application/components/dashboard_top_nav

Error: expect(received).toMatchSnapshot()

Snapshot name: `Dashboard top nav render in embed mode, components can be forced show by appending URL param 1`

- Snapshot  - 0
+ Received  + 1

@@ -973,10 +973,11 @@
                  "createAnnotation": [MockFunction],
                  "deleteAnnotation": [MockFunction],
  … (11 more lines)

❌ Dashboard top nav render in embed mode, and force hide filter bar (0.019s)

Jest Tests.src/plugins/dashboard/public/application/components/dashboard_top_nav

Error: expect(received).toMatchSnapshot()

Snapshot name: `Dashboard top nav render in embed mode, and force hide filter bar 1`

- Snapshot  - 0
+ Received  + 1

@@ -973,10 +973,11 @@
                  "createAnnotation": [MockFunction],
                  "deleteAnnotation": [MockFunction],
  … (11 more lines)

❌ Test group value suggestions should suggest in phrase after grouping with phrase - opened (0.004s)

Jest Tests.src/plugins/data/public/antlr/dql

Error: Method “simulate” is meant to be run on 1 node. 0 found instead.
    at ReactWrapper.single (/home/runner/work/OpenSearch-Dashboards/OpenSearch-Dashboards/node_modules/enzyme/src/ReactWrapper.js:1168:13)
    at ReactWrapper.single [as simulate] (/home/runner/work/OpenSearch-Dashboards/OpenSearch-Dashboards/node_modules/enzyme/src/ReactWrapper.js:665:17)
    at simulate (/home/runner/work/OpenSearch-Dashboards/OpenSearch-Dashboards/src/plugins/dashboard/public/application/components/dashboard_listing/dashboard_listing.test.tsx:120:64)
    at act (/home/runner/work/OpenSearch-Dashboards/OpenSearch-Dashboards/node_modules/react/cjs/react.development.js:2512:16)
    at Object.<anonymous> (/home/runner/work/OpenSearch-Dashboards/OpenSearch-Dashboards/src/plugins/dashboard/public/application/components/dashboard_listing/dashboard_listing.test.tsx:119:14)

❌ dashboard tags adds a Tags column after Last updated (0.018s)

Jest Tests.src/plugins/dashboard/public/application/components/dashboard_listing

TypeError: Cannot read properties of undefined (reading 'savedObjectType')
    at Object.savedObjectType (/home/runner/work/OpenSearch-Dashboards/OpenSearch-Dashboards/src/plugins/dashboard/public/application/components/dashboard_listing/dashboard_listing.test.tsx:180:22)

❌ AssociatedObjectsTable should call the correct action when clicking on the "Discover" button without query enhancements enabled (0.074s)

Jest Tests.src/plugins/data_source_management/public/components/direct_query_data_sources_components/associated_object_management

Error: expect(received).toEqual(expected) // deep equality

- Expected  - 6
+ Received  + 1

- Array [
-   Object {
-     "id": "b",
-     "title": "B",
-   },
  … (3 more lines)

❌ of() when promise resolves first member of 3-tuple is the promise value (0.014s)

Jest Tests.src/plugins/opensearch_dashboards_utils/common

Error: expect(received).toEqual(expected) // deep equality

- Expected  - 4
+ Received  + 6

- ObjectContaining {
-   "minHeight": 48,
-   "minWidth": 240,
-   "paddingTop": 8,
+ Object {
  … (20 more lines)

📄 junit-jest-group5-Windows/TEST-Jest Tests.xml

❌ VariableService initialize and getVariables should return empty array when not initialized (0.016s)

Jest Tests.src\plugins\dashboard\public\variables

Error: expect(received).toMatchSnapshot()

Snapshot name: `Dashboard top nav render with all components 1`

- Snapshot  - 0
+ Received  + 1

@@ -973,10 +973,11 @@
                  "createAnnotation": [MockFunction],
                  "deleteAnnotation": [MockFunction],
  … (11 more lines)

❌ Dashboard top nav render in full screen mode, no componenets should be shown (0.032s)

Jest Tests.src\plugins\dashboard\public\application\components\dashboard_top_nav

Error: expect(received).toMatchSnapshot()

Snapshot name: `Dashboard top nav render in full screen mode, no componenets should be shown 1`

- Snapshot  - 0
+ Received  + 1

@@ -973,10 +973,11 @@
                  "createAnnotation": [MockFunction],
                  "deleteAnnotation": [MockFunction],
  … (11 more lines)

❌ Dashboard top nav render in full screen mode with appended URL param but none of the componenets can be forced show (0.035s)

Jest Tests.src\plugins\dashboard\public\application\components\dashboard_top_nav

Error: expect(received).toMatchSnapshot()

Snapshot name: `Dashboard top nav render in full screen mode with appended URL param but none of the componenets can be forced show 1`

- Snapshot  - 0
+ Received  + 1

@@ -973,10 +973,11 @@
                  "createAnnotation": [MockFunction],
                  "deleteAnnotation": [MockFunction],
  … (11 more lines)

❌ Dashboard top nav render in embed mode (0.028s)

Jest Tests.src\plugins\dashboard\public\application\components\dashboard_top_nav

Error: expect(received).toMatchSnapshot()

Snapshot name: `Dashboard top nav render in embed mode 1`

- Snapshot  - 0
+ Received  + 1

@@ -973,10 +973,11 @@
                  "createAnnotation": [MockFunction],
                  "deleteAnnotation": [MockFunction],
  … (11 more lines)

❌ Dashboard top nav render in embed mode, components can be forced show by appending URL param (0.062s)

Jest Tests.src\plugins\dashboard\public\application\components\dashboard_top_nav

Error: expect(received).toMatchSnapshot()

Snapshot name: `Dashboard top nav render in embed mode, components can be forced show by appending URL param 1`

- Snapshot  - 0
+ Received  + 1

@@ -973,10 +973,11 @@
                  "createAnnotation": [MockFunction],
                  "deleteAnnotation": [MockFunction],
  … (11 more lines)

❌ Dashboard top nav render in embed mode, and force hide filter bar (0.031s)

Jest Tests.src\plugins\dashboard\public\application\components\dashboard_top_nav

Error: expect(received).toMatchSnapshot()

Snapshot name: `Dashboard top nav render in embed mode, and force hide filter bar 1`

- Snapshot  - 0
+ Received  + 1

@@ -973,10 +973,11 @@
                  "createAnnotation": [MockFunction],
                  "deleteAnnotation": [MockFunction],
  … (11 more lines)

❌ Test group value suggestions should suggest in phrase after grouping with phrase - opened (0.016s)

Jest Tests.src\plugins\data\public\antlr\dql

Error: Method “simulate” is meant to be run on 1 node. 0 found instead.
    at ReactWrapper.single (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\node_modules\enzyme\src\ReactWrapper.js:1168:13)
    at ReactWrapper.single [as simulate] (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\node_modules\enzyme\src\ReactWrapper.js:665:17)
    at simulate (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\src\plugins\dashboard\public\application\components\dashboard_listing\dashboard_listing.test.tsx:120:64)
    at act (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\node_modules\react\cjs\react.development.js:2512:16)
    at Object.<anonymous> (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\src\plugins\dashboard\public\application\components\dashboard_listing\dashboard_listing.test.tsx:119:14)

❌ dashboard tags adds a Tags column after Last updated (0.022s)

Jest Tests.src\plugins\dashboard\public\application\components\dashboard_listing

TypeError: Cannot read properties of undefined (reading 'savedObjectType')
    at Object.savedObjectType (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\src\plugins\dashboard\public\application\components\dashboard_listing\dashboard_listing.test.tsx:180:22)

❌ AssociatedObjectsTable should call the correct action when clicking on the "Discover" button without query enhancements enabled (0.095s)

Jest Tests.src\plugins\data_source_management\public\components\direct_query_data_sources_components\associated_object_management

Error: expect(received).toEqual(expected) // deep equality

- Expected  - 6
+ Received  + 1

- Array [
-   Object {
-     "id": "b",
-     "title": "B",
-   },
  … (3 more lines)

❌ of() when promise resolves first member of 3-tuple is the promise value (0.023s)

Jest Tests.src\plugins\opensearch_dashboards_utils\common

Error: expect(received).toEqual(expected) // deep equality

- Expected  - 4
+ Received  + 6

- ObjectContaining {
-   "minHeight": 48,
-   "minWidth": 240,
-   "paddingTop": 8,
+ Object {
  … (20 more lines)

---

20 failure(s) across 2 suite(s). Full XML reports are in the junit-jest-* artifacts.

[github-actions]

Persistent review updated to latest commit 5bbe312

[github-actions]

Persistent review updated to latest commit 413d10e

[github-actions]

Persistent review updated to latest commit fa4ebef