Improve xDS web UI: navigation, pagination, history diff, and client … - #1320
Merged
Conversation
…subscriptions Motivation: - Selecting a group landed on the Listeners page instead of the Overview page, making it hard to get a quick summary of the group at a glance. - The Resources and History pages had no pagination, making large groups hard to browse. - The group-wide History page showed commit summaries as plain text with no way to inspect what actually changed in a commit. - The References page gave no indication that inbound references from other groups are not shown, which could be confusing. - The control plane Clients table had no visibility into what resource names each client subscribed to. Modifications: - Change the default navigation target when selecting a group (group list, sidebar dropdown, breadcrumb, and the `useXdsRoute` fallback) from `listeners` to `overview`. - Add client-side pagination (default 10 rows) to the Resources and History pages. - Add a Subscriptions column to the Clients table. - Track subscribed resource names in `XdsClientStatusTracker`. Result: - Selecting a group now lands on the Overview page. - Resources and History pages are paginated. - Every commit in the group-wide History is inspectable with a before/after diff. - The References page clearly communicates its cross-group limitation.
📝 WalkthroughWalkthroughAdds xDS runtime status APIs and views, new group overview/history/reference sections, Kubernetes endpoint preview support, and a sample ADS client/server harness. ChangesxDS control-plane and viewer updates
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
xds/src/test/java/com/linecorp/centraldogma/xds/XdsTestServer.java (1)
181-229: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFail the demo token when bootstrap prerequisites fail.
bootstrapStep()only logs bad responses, so this method can still return a non-nullappTokenafter the READ grant or an earlier setup call failed. The startup banner then advertises an app-scoped demo that is not actually available. MakebootstrapSampleData()returnnullunless every required bootstrap call succeeds, or havebootstrapStep()fail hard on non-2xx/409 responses.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xds/src/test/java/com/linecorp/centraldogma/xds/XdsTestServer.java` around lines 181 - 229, The bootstrap flow in bootstrapSampleData is too lenient because bootstrapStep only logs failures, so appToken can still be returned after a failed prerequisite and advertise a demo token that does not work. Update bootstrapSampleData to return null unless every required call succeeds, or change bootstrapStep to throw/fail on any non-2xx/409 response so the later TestAuthMessageUtil.getAccessToken and READ grant only complete when the full setup succeeded.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webapp/src/dogma/common/components/JsonDiffEditor.tsx`:
- Line 58: The Monaco theme selection in JsonDiffEditor should use the core
light theme identifier instead of the wrapper alias. Update the theme prop logic
in JsonDiffEditor so that the light branch passes "vs" rather than "light",
while keeping the dark branch unchanged as "vs-dark".
In `@webapp/src/dogma/features/xds/ClientStatusTable.tsx`:
- Around line 159-166: The Last seen column in ClientStatusTable is using a
string accessor, so sorting is lexicographic instead of chronological. Update
the columnHelper.accessor for lastSeen to return the numeric timestamp from
row.lastSeen, and keep formatTime(info.row.original.lastSeen) only in the cell
renderer. Make this change in the lastSeen column definition so sorting uses the
actual timestamp value.
In `@webapp/src/dogma/features/xds/GroupOverview.tsx`:
- Around line 38-60: The overview cards currently treat query failures as empty
results, so update GroupOverview’s data handling to distinguish loading,
success, and error states instead of falling back to 0. In the components using
useListResourcesQuery and useListK8sAggregatorsQuery, check the query error
flags and render a failure state or placeholder when a request fails, while
keeping the count only for successful responses. Use the existing GroupOverview
and useListK8sAggregatorsQuery patterns to apply the same fix consistently.
In `@webapp/src/dogma/features/xds/K8sAggregatorStatus.tsx`:
- Around line 62-85: The K8sAggregatorStatus display currently treats any fetch
error other than a 404 as a successful sync, which can show the Synced badge on
failed requests. Update the conditional rendering in K8sAggregatorStatus so that
error && !notSynced is handled before the success branch, and render an explicit
error state instead of the green synced summary when a non-404 fetch failure
occurs.
In `@webapp/src/dogma/features/xds/ResourceHistory.tsx`:
- Line 167: Update the revision comparison label in ResourceHistory so the left
side does not render “revision 0” when revision is 1. In the UI text that
currently uses revision - 1 alongside revision, add a conditional in the
relevant ResourceHistory render path to show an “empty document” label for the
first commit, while keeping the existing revision numbering for later commits;
apply the same change anywhere else the same label pattern is used.
- Around line 107-132: The ResourceHistory diff calculation is treating a failed
`before` fetch as an empty snapshot, which makes the modal show a fake “all
files added” diff. Update the `ResourceHistory` logic around
`isLoading`/`changedFiles` to distinguish `before.isError` from an intentionally
skipped `before` query, and render the warning/error state instead of falling
back to `new Map()` when `revision > 1` and the `before` request fails. Also
make sure the same error handling is reflected in the related UI path referenced
by the affected `ResourceHistory` render logic.
- Around line 274-283: The Revision column in ResourceHistory is sorting
lexicographically because the accessor in columnHelper.accessor is returning
String(row.revision). Update the revision column to use the numeric revision
value as the accessor and keep only the display cell formatting in the Badge, so
TanStack Table can sort revisions numerically. Locate the change in the cols
array within ResourceHistory and adjust the revision column definition
accordingly.
- Around line 289-297: The diff trigger in ResourceHistory is using a Link
without an href, so it is not keyboard-accessible. Update the cell renderer in
the ResourceHistory table to use button semantics for the click target that
calls openDiff, keeping the same styling if needed, so the summary/diff control
is focusable and operable via keyboard.
In `@webapp/src/dogma/features/xds/SnapshotViewer.tsx`:
- Around line 55-60: The repo/app selector logic in SnapshotViewer is treating
query failures the same as empty results because useGetReposQuery and
useGetXdsAppsQuery only read data; update the SnapshotViewer data flow to also
consume isLoading and error from both queries and propagate them into the
rendered state. Ensure the group/app selection UI and any dependent checks in
the affected lookup paths (including the related repo/app handling around the
later lookup block) render a loading or error state when the backend call fails
instead of falling back to “no group”/“no app connected” messaging.
- Around line 72-79: The scope handling in SnapshotViewer’s
useGetXdsSnapshotQuery falls back to the global snapshot when scope is app but
selectedAppId is empty, which makes the “By application identity” view render
the wrong data. Update the SnapshotViewer logic so it does not call
useGetXdsSnapshotQuery until a real app id is available for app scope, and
instead keep the app snapshot empty/loading state until selectedAppId is
populated. Use the existing SnapshotViewer scope check and the
useGetXdsSnapshotQuery call site to gate the request correctly.
In `@webapp/src/dogma/features/xds/xdsApiSlice.ts`:
- Around line 359-361: The getGroupHistory query in xdsApiSlice is interpolating
filePath directly into the URL, which can break history requests for paths
containing reserved characters. Update the query builder in getGroupHistory to
encode the filePath value before concatenating it into the commits/head query
string, while leaving the group and maxCommits handling unchanged.
- Around line 418-446: The reference graph builder in xdsApiSlice currently
fetches each file one-by-one inside the nested REF_TYPES/filesByType loop,
causing serial latency accumulation. Refactor the content-read portion of the
graph construction to start the /contents requests in parallel (for example by
mapping files to fetch promises and awaiting them together) while keeping the
existing extractReferences, resolveReference, and edges assembly logic
unchanged.
In
`@xds/src/main/java/com/linecorp/centraldogma/xds/internal/ControlPlaneService.java`:
- Around line 194-198: The EDS registration in ControlPlaneService currently
bypasses repo/group authorization and makes every endpoint readable by any
authenticated user. Update the annotatedService setup for XdsEndpointReadService
to enforce the same READ permission checks used for normal group access, or
restrict this endpoint to system admins only. Keep the change localized to the
endpoint registration and XdsEndpointReadService path so endpoint reads are no
longer cross-group accessible.
- Around line 190-192: The xdsApplicationService registration is currently
bypassing auth decoration, leaving native gRPC methods exposed; restore the
management GrpcService decorator on serverBuilder.service(xdsApplicationService)
or apply an equivalent decorator that covers the native gRPC routes as well as
the transcoded /api/v1/** paths. Use the xdsApplicationService and
ControlPlaneService setup to locate the service registration and ensure the same
auth protection applies to both HTTP and direct gRPC access.
In
`@xds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsClientStatusTracker.java`:
- Around line 113-120: The subscription tracking in XdsClientStatusTracker is
treating an empty resource_names list as “no change” forever, which prevents
reverting back to wildcard once a named subscription has been recorded. Update
the request handling around the names/resourceNames logic so the tracker can
distinguish an ACK/NACK heartbeat empty list from an intentional wildcard
subscription, and make sure the stored type.resourceNames is cleared back to
empty when the client really switches to wildcard. Preserve the existing
snapshot behavior in XdsClientStatusTracker while fixing the update path that
currently leaves stale named subscriptions visible in the JSON output.
In
`@xds/src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesService.java`:
- Around line 396-402: The preview failure handler in XdsKubernetesService
currently converts every exception from resolvePreview() into Status.INTERNAL,
which hides client-side errors like IllegalArgumentException and
EntryNotFoundException. Update the CompletableFuture.allOf(...).handle(...)
block to reuse the same exception-to-status mapping used by create/update, or
pass through any existing StatusRuntimeException so client errors remain
INVALID_ARGUMENT instead of being rewritten to INTERNAL.
---
Outside diff comments:
In `@xds/src/test/java/com/linecorp/centraldogma/xds/XdsTestServer.java`:
- Around line 181-229: The bootstrap flow in bootstrapSampleData is too lenient
because bootstrapStep only logs failures, so appToken can still be returned
after a failed prerequisite and advertise a demo token that does not work.
Update bootstrapSampleData to return null unless every required call succeeds,
or change bootstrapStep to throw/fail on any non-2xx/409 response so the later
TestAuthMessageUtil.getAccessToken and READ grant only complete when the full
setup succeeded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 98e7e579-ce20-417c-b540-104f6373a925
📒 Files selected for processing (41)
webapp/.gitignorewebapp/src/dogma/common/components/JsonDiffEditor.tsxwebapp/src/dogma/features/api/apiSlice.tswebapp/src/dogma/features/xds/ClientStatusTable.tsxwebapp/src/dogma/features/xds/ControlPlaneStatusDto.tswebapp/src/dogma/features/xds/DangerZone.tsxwebapp/src/dogma/features/xds/GroupList.tsxwebapp/src/dogma/features/xds/GroupOverview.tsxwebapp/src/dogma/features/xds/GroupSelector.tsxwebapp/src/dogma/features/xds/K8sAggregatorEditor.tsxwebapp/src/dogma/features/xds/K8sAggregatorList.tsxwebapp/src/dogma/features/xds/K8sAggregatorPreviewModal.tsxwebapp/src/dogma/features/xds/K8sAggregatorStatus.tsxwebapp/src/dogma/features/xds/ResourceEditor.tsxwebapp/src/dogma/features/xds/ResourceHistory.tsxwebapp/src/dogma/features/xds/ResourceList.tsxwebapp/src/dogma/features/xds/ResourceReferences.tsxwebapp/src/dogma/features/xds/Sidebar.tsxwebapp/src/dogma/features/xds/SnapshotViewer.tsxwebapp/src/dogma/features/xds/XdsLayout.tsxwebapp/src/dogma/features/xds/useGroupAdminAccess.tswebapp/src/dogma/features/xds/useGroupExists.tswebapp/src/dogma/features/xds/useGroupReadAccess.tswebapp/src/dogma/features/xds/useGroupWriteAccess.tswebapp/src/dogma/features/xds/useXdsRoute.tswebapp/src/dogma/features/xds/xdsApiSlice.tswebapp/src/dogma/features/xds/xdsReferences.tswebapp/src/pages/_app.tsxwebapp/src/pages/app/xds/control-plane/index.tsxwebapp/src/pages/app/xds/group.tsxwebapp/src/pages/app/xds/index.tsxwebapp/tsconfig.tsbuildinfoxds/build.gradlexds/src/main/java/com/linecorp/centraldogma/xds/internal/ControlPlaneService.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsClientStatusTracker.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsControlPlaneStatusService.javaxds/src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesService.javaxds/src/main/proto/centraldogma/xds/k8s/v1/xds_kubernetes.protoxds/src/test/java/com/linecorp/centraldogma/xds/XdsTestClient.javaxds/src/test/java/com/linecorp/centraldogma/xds/XdsTestServer.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsClientStatusTrackerTest.java
minwoox
marked this pull request as ready for review
June 26, 2026 05:22
jrhee17
approved these changes
Jun 29, 2026
jrhee17
left a comment
jrhee17
left a comment
Contributor
There was a problem hiding this comment.
Understood java-related changes are
- exposing APIs for administrative purposes
- applying auth for native gRPC paths
ikhoon
approved these changes
Jun 29, 2026
| // the end of the underlying gRPC call instead, which covers every termination path. onStreamOpen is | ||
| // invoked synchronously from the service method with the Armeria context mounted. | ||
| final ServiceRequestContext ctx = ServiceRequestContext.currentOrNull(); | ||
| if (ctx != null) { |
Contributor
There was a problem hiding this comment.
If ctx is absent in the thread local, should we treat it as a bug?
assert ctx != null;
| streams.forEach((streamId, stream) -> { | ||
| final ObjectNode streamNode = array.addObject(); | ||
| streamNode.put("streamId", streamId); | ||
| streamNode.put("nodeId", stream.nodeId); |
Contributor
There was a problem hiding this comment.
I noticed that toClientsJson could be called while stream is being updated, so it may not serialize a constant snapshot of stream.
Contributor
Author
There was a problem hiding this comment.
Nice catch. Updated.
| * Lists the application identities that have connected to the discovery API, with the groups each can read. | ||
| */ | ||
| @Get("/xds/apps") | ||
| public ArrayNode apps() { |
Contributor
There was a problem hiding this comment.
Optional) I prefer defining DTO object to track changes in the serialzed output more easily.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
xds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsClientStatusTracker.java (1)
63-70: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winResolve the request context before registering the stream.
If
ServiceRequestContext.current()fails here, Line 63 has already inserted the stream and nothing rolls that entry back, so/xds/clientscan retain a phantom client until it is explicitly removed later. Move the context lookup ahead ofstreams.put(...), or remove the entry on failure.Suggested fix
`@Override` public void onStreamOpen(long streamId, String typeUrl) throws RequestException { + final ServiceRequestContext ctx = ServiceRequestContext.current(); streams.put(streamId, new StreamState(System.currentTimeMillis())); // The controlplane invokes onStreamClose/onStreamCloseWithError only for a graceful close or an error // surfaced through the request stream; an abrupt client disconnect is delivered as a cancellation // (onCancelHandler), which does NOT trigger those callbacks and would leak the entry. Tie removal to // the end of the underlying gRPC call instead, which covers every termination path. onStreamOpen is // invoked synchronously from the service method with the Armeria context mounted. - final ServiceRequestContext ctx = ServiceRequestContext.current(); ctx.log().whenComplete().thenAccept(unused -> streams.remove(streamId)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsClientStatusTracker.java` around lines 63 - 70, Resolve the request context before inserting the stream entry in XdsClientStatusTracker.onStreamOpen. Move the ServiceRequestContext.current() lookup ahead of streams.put(...) so a failure cannot leave a stale StreamState behind, or otherwise roll back the insertion if context resolution fails. Keep the cleanup tied to ctx.log().whenComplete() after the context is safely obtained.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@xds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsClientStatusTracker.java`:
- Around line 63-70: Resolve the request context before inserting the stream
entry in XdsClientStatusTracker.onStreamOpen. Move the
ServiceRequestContext.current() lookup ahead of streams.put(...) so a failure
cannot leave a stale StreamState behind, or otherwise roll back the insertion if
context resolution fails. Keep the cleanup tied to ctx.log().whenComplete()
after the context is safely obtained.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 34dc2436-97e6-47ff-ba94-8781865ad90a
📒 Files selected for processing (11)
webapp/src/dogma/features/xds/ControlPlaneStatusDto.tswebapp/src/pages/app/xds/group.tsxxds/src/main/java/com/linecorp/centraldogma/xds/internal/ControlPlaneService.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsAppDto.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsClientStatusTracker.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsClientStreamDto.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsControlPlaneStatusService.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsResourceTypeDto.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsSnapshotDto.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsTypeStateDto.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsClientStatusTrackerTest.java
💤 Files with no reviewable changes (1)
- webapp/src/dogma/features/xds/ControlPlaneStatusDto.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- webapp/src/pages/app/xds/group.tsx
- xds/src/main/java/com/linecorp/centraldogma/xds/internal/ControlPlaneService.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…subscriptions
Motivation:
Modifications:
useXdsRoutefallback) fromlistenerstooverview.XdsClientStatusTracker.Result: