feat: Swap mapping direction by sandornagy517 · Pull Request #107 · PagerDuty/backstage-plugins

sandornagy517 · 2025-12-10T13:48:28Z

Description

To improve loading performance of the mappings page we swap the direction of the data loaded. Until now were were mapping Backstage entities to PagerDuty services, now we swap this and will do the opposite. In this PR I've updated the mappings table to the new Backstage UI table and updated the way we load the data for the component.

Affected plugin

backstage-plugin
backstage-plugin-backend
backstage-plugin-scaffolder-actions
backstage-plugin-entity-processor

Type of change

New feature (non-breaking change which adds functionality)
Fix (non-breaking change which fixes an issue)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

I have performed a self-review of this change
Changes have been tested
Changes have been tested in dark theme
Changes are documented
Changes generate no new warnings
PR title follows conventional commit semantics

If this is a breaking change 👇

I have documented the migration process
I have implemented necessary warnings (if it can live side by side)

Acknowledgement

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

jhfgloria

Left some comments. The PR is quite big (and visual), so probably I would need to test it locally to make more sense of it.

jhfgloria · 2026-01-07T16:00:40Z

plugins/backstage-plugin-backend/src/apis/pagerduty.ts

+  await Promise.all(
+    Object.entries(EndpointConfig).map(async ([account, _]) => {
+      services = await Promise.all(
+        ids.map(async id => await getServiceById(id, account)),


I think we should probably use this API instead, because we can do it in one request? https://github.com/PagerDuty/service-search-service/blob/master/apps/api/doc/api/service_directory.md#api-service_directory-list_services

jhfgloria · 2026-01-07T16:06:00Z

plugins/backstage-plugin-backend/src/service/router.ts

-      mapping => mapping.serviceId === service.id,
+      e =>
+        e.integrationKey ===
+        ent?.metadata.annotations?.['pagerduty.com/integration-key'],


Are there any guarantees that at this point the integration-key was set? 🤔 Why not searching by integration-key OR service-id.

jhfgloria · 2026-01-07T16:08:08Z

plugins/backstage-plugin-backend/src/service/router.ts

+      result.mappings.push({
+        entityRef: '',
+        entityName: '',
+        integrationKey: entityMapping?.integrationKey,
+        serviceId: entityMapping?.serviceId ?? '',
+        status: 'NotMapped',
+        serviceName: '',
+        team: '',
+        escalationPolicy: '',
+        serviceUrl: '',
+        account: '',
+      });


Do we need to push this empty state?

jhfgloria · 2026-01-07T16:10:14Z

plugins/backstage-plugin-backend/src/service/router.ts

  });

-  // GET /mapping/entity
-  router.get('/mapping/entity', async (_, response) => {


Please don't remove any methods. Instead mark them as deprecated. There is a chance that people only update the frontend or just the backend and then there will be missing methods.

jhfgloria · 2026-01-07T16:18:00Z

plugins/backstage-plugin/src/components/PagerDutyPage/MappingsTable/MappingsTable.tsx

+        search: debouncedSearchQuery,
+        searchFields: ['metadata.name', 'spec.owner'],
+      }),
+    staleTime: 5 * 60 * 1000,


Why 5 minutes? 🤔 Is it worth caching at all?

hmm, not sure why did I include this back then, just removed it!

jhfgloria · 2026-01-07T16:20:54Z

plugins/backstage-plugin/src/components/PagerDutyPage/MappingsTable/MappingsTable.tsx

+  const [pageSize, setPageSize] = useState(10);
+  const [searchQuery, setSearchQuery] = useState('');
+  const [debouncedSearchQuery, setDebouncedSearchQuery] = useState('');
+  useDebounce(() => setDebouncedSearchQuery(searchQuery), 500, [searchQuery]);


Use debounce used like this is so awkward... Shouldn't it return something? What's the point of the hook at all? 🤔

plugins/backstage-plugin/src/components/PagerDutyPage/MappingsTable/StatusCell.tsx

plugins/backstage-plugin-backend/src/apis/pagerduty.ts

jhfgloria · 2026-01-15T14:35:17Z

plugins/backstage-plugin-backend/src/apis/pagerduty.ts

  }
 }

+export async function getServicesByIdsServiceDirectory(


I don't think we need to expose the calling service from PagerDuty. getServicesByIdsAndAccount would be more readable and not expose the fact that the call is made to ServicesDirectory.

jhfgloria · 2026-01-15T14:53:30Z

plugins/backstage-plugin-backend/src/service/router.ts

+          };
+
+          const formattedEntity = {
+            name: entity?.metadata?.name,


Why would the entity be null? Doesn't it come from the catalog API? Also, if it is null would be better practice to early return here, instead of dragging the ? around forever?

jhfgloria · 2026-01-15T14:56:47Z

plugins/backstage-plugin-backend/src/service/router.ts

+            const foundServices = pagerDutyServices.filter(
+              s => s.id === serviceId,
+            );
+            if (foundServices.length > 0) {
+              service = foundServices[0];
+            }


Suggested change

const foundServices = pagerDutyServices.filter(

s => s.id === serviceId,

);

if (foundServices.length > 0) {

service = foundServices[0];

}

service = pagerDutyServices.find(s => s.id === serviceId);

jhfgloria · 2026-01-15T14:57:46Z

plugins/backstage-plugin-backend/src/service/router.ts

+                account,
+              );
+            } catch (e) {
+              // Service might not exist, just continue


I think this type of error is only valid if the error is a 404. Otherwise you will not know if there is a map or not and could decide to map something else.

jhfgloria

LGTM! Let's just make some tests in the ServiceMappingComponent (I think it is the topmost component) and test the router.ts new endpoint (which I also think is the topmost backend piece of software). There is no tests in the PR and I think there should be.

jhfgloria

Left some comments :)

jhfgloria · 2026-01-26T14:02:01Z

plugins/backstage-plugin-backend/src/service/router.test.ts

 describe('createRouter', () => {
  let app: express.Express;
+  let store: PagerDutyBackendStore;
+  const mockCatalogApi = {


Do we need to mock the entire class? 🤔

I mocked it for TS reasons only

jhfgloria · 2026-01-26T14:09:48Z

plugins/backstage-plugin-backend/src/service/router.test.ts

+        expect(response.status).toEqual(200);
+        expect(response.body).toHaveProperty('entities');
+        expect(response.body).toHaveProperty('totalCount');
+        expect(Array.isArray(response.body.entities)).toBe(true);


Well, [ ] is also an array, that doesn't make this test more accurate. I think we should check if the responde contains the expected mapping, and not just check if it is an array. Otherwise I can change the implementation of the controller to just return an empty array and it will pass anyway.

jhfgloria · 2026-01-26T14:10:27Z

plugins/backstage-plugin-backend/src/service/router.test.ts

+
+        expect(response.status).toEqual(200);
+        expect(response.body).toHaveProperty('entities');
+        expect(response.body).toHaveProperty('totalCount');


Again, we're not asserting the return of the request, and we should.

jhfgloria · 2026-01-26T14:13:55Z

plugins/backstage-plugin-backend/src/service/router.test.ts

+        expect(response.status).toEqual(200);
+        expect(response.body).toHaveProperty('entities');
+        expect(response.body).toHaveProperty('totalCount');
+      });


Ergh... these tests look all the same thing. The fact that we mock everything in the mockCatalogApi doesn't allow for testing much. The fact that you never assert the contents of entities is the prove itself that the test doesn't really matter. Tbh, unless we change these tests to actually alter the catalog that is queried, I'd even prefer to just wipe these tests. They're not testing anything in my opinion.

jhfgloria · 2026-01-26T14:14:26Z

plugins/backstage-plugin-backend/src/service/router.test.ts

+
+        expect(response.status).toEqual(200);
+        expect(response.body).toHaveProperty('entities');
+        expect(response.body).toHaveProperty('totalCount');


Again, not testing anything.

jhfgloria · 2026-01-26T14:18:50Z

plugins/backstage-plugin/src/components/PagerDutyPage/MappingsTable/MappingsTable.test.tsx

+    await act(async () => {
+      fireEvent.change(searchInput, { target: { value: 'my-component' } });
+    });
+
+    await act(async () => {
+      jest.advanceTimersByTime(500);
+    });


I don't think modern RTL tests still require act wrapping. fireEvent is already wrapped in a act. For advanceTimersByTime you may be need, because it is not a RTL event.

jhfgloria · 2026-01-26T14:20:20Z

plugins/backstage-plugin/src/components/PagerDutyPage/MappingsTable/MappingsTable.test.tsx

+    expect(screen.getByText('Out of Sync')).toBeInTheDocument();
+    expect(screen.getByText('Not Mapped')).toBeInTheDocument();
+    expect(
+      screen.getByText('Error occured while fetching service'),


Does this fit the table column? 🤔

it does not but gets dotted out, on hover the full text is displayed

jhfgloria

LGTM 🚀

* feat: Swap service mapping direction * feat: Get rid of the old MappingTable * feat: Use a different API for retrieving PD services * fix: Revert entity mappings * fix: Resolve conflicts * fix: Service mapping creation and update * feat: Add unit tests * fix: Remove unused import * fix: Add proper expects for router unit tests * fix: Get rid of redundant unit test expects * fix: Mock catalog api the backstage way

sandornagy517 requested a review from a team as a code owner December 10, 2025 13:48

sandornagy517 changed the base branch from main to next December 10, 2025 13:48

jhfgloria reviewed Jan 7, 2026

View reviewed changes

jhfgloria force-pushed the next branch from bb72be1 to 597db0e Compare January 14, 2026 11:56

jhfgloria reviewed Jan 15, 2026

View reviewed changes

jhfgloria requested changes Jan 20, 2026

View reviewed changes

sandornagy517 force-pushed the feat/swap_mapping_direction branch from c38fcb1 to ed2ba57 Compare January 23, 2026 10:54

sandornagy517 added 7 commits January 23, 2026 12:01

feat: Swap service mapping direction

e87a151

feat: Get rid of the old MappingTable

0f9b54f

feat: Use a different API for retrieving PD services

891ba0d

fix: Revert entity mappings

8d98f69

fix: Resolve conflicts

08016cb

fix: Service mapping creation and update

fcd4f9f

feat: Add unit tests

582e3ba

sandornagy517 force-pushed the feat/swap_mapping_direction branch from ed2ba57 to 582e3ba Compare January 23, 2026 11:01

sandornagy517 and others added 2 commits January 26, 2026 14:03

fix: Remove unused import

7a5a78f

Merge branch 'next' into feat/swap_mapping_direction

85cffb7

jhfgloria reviewed Jan 26, 2026

View reviewed changes

sandornagy517 added 3 commits January 26, 2026 17:05

fix: Add proper expects for router unit tests

65af7cd

fix: Get rid of redundant unit test expects

07090b0

fix: Mock catalog api the backstage way

77f8d4d

sandornagy517 changed the base branch from next to service_mappigns_feature January 27, 2026 13:09

jhfgloria approved these changes Jan 27, 2026

View reviewed changes

sandornagy517 merged commit 69099f2 into service_mappigns_feature Jan 27, 2026
15 checks passed

Conversation

sandornagy517 commented Dec 10, 2025

Description

Affected plugin

Type of change

Checklist

Acknowledgement

Uh oh!

jhfgloria left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhfgloria left a comment

Choose a reason for hiding this comment

Uh oh!

jhfgloria left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhfgloria left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants