Skip to content

Move components to UI registry for topic detail#2486

Open
jvorcak wants to merge 15 commits into
masterfrom
UX-998-migrate-topic-detail-to-ui-registry
Open

Move components to UI registry for topic detail#2486
jvorcak wants to merge 15 commits into
masterfrom
UX-998-migrate-topic-detail-to-ui-registry

Conversation

@jvorcak

@jvorcak jvorcak commented Jun 5, 2026

Copy link
Copy Markdown
Collaborator

I've migrated components to the new UI registry

Screenshot 2026-06-09 at 14 13 58
  • changed the topic configuraion to be more easy to work with.

Before:
Screenshot 2026-06-09 at 14 15 12

After:
Screenshot 2026-06-09 at 14 14 03

@jvorcak jvorcak force-pushed the UX-998-migrate-topic-detail-to-ui-registry branch 2 times, most recently from 9a82b80 to 9a920fd Compare June 5, 2026 17:08
@jvorcak jvorcak force-pushed the UX-998-migrate-topic-detail-to-ui-registry branch from 9a920fd to 34cd134 Compare June 5, 2026 17:27
jvorcak added 2 commits June 8, 2026 10:47
…wlink setup

startBackendServerWithConfig swallowed the real crash reason: when the
backend container exits during the testcontainers wait strategy, .start()
throws before containerId is assigned, so the diagnostic docker-logs block
was guarded out and only a bare 409 surfaced.

Recover the container ID from the error message (mirrors startBackendServer)
and dump exit code, state, and logs on failure.
…removal

The previous recovery found the crashed container ID but `docker logs` failed
because testcontainers removes a container once its wait strategy fails. Attach
a withLogConsumer that buffers output while the container is alive, and dump
that buffer on failure so the actual backend crash reason is visible.
@jvorcak jvorcak marked this pull request as ready for review June 8, 2026 10:50
@jvorcak jvorcak requested a review from a team June 8, 2026 10:50
jvorcak added 4 commits June 8, 2026 14:09
Remove @redpanda-data/ui from the expanded-message chain:
expanded-message, message-meta-data, payload-component, and
troubleshoot-report-viewer now use registry components and Tailwind.
Replace useToast with sonner and useColorModeValue with theme tokens.
Behind the existing new-topic-page flag, render topic configs grouped by
category with a sidebar that filters to a category, an All/Modified scope
toggle (URL-backed) with a clear-able search, click-to-reveal descriptions,
and reset moved into the edit dialog. Show a modified-count badge on the
Configuration tab title and per category. Default enum/boolean editors to
their first option when a config has no value.
…dings)

The new-topic-page Configuration tab replaced the flat '.configGroupTitle'
list with a category sidebar and titled sections. Update the navigation spec
and topic-page helpers to match (navigation landmark + category buttons +
section headings).
@jvorcak

jvorcak commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator Author

@claude review

@malinskibeniamin malinskibeniamin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Thanks for the migration. Major value is clear: topic detail moves off legacy UI pieces and the new Configuration tab improves discoverability. Requesting changes for a few regressions before merge.

Main blockers: config edits can silently write/reset the wrong value, URL-backed state creates stale/secret-leak paths, pagination can show false empty tables, and some categories/accessibility/security guards regressed.

One additional issue not attached inline: topic-details.tsx tab switching uses historyReplace(${loc.pathname}#${loc.hash}), which drops the search string. Since this PR makes configFilter/configScope URL-backed, switching tabs and returning loses the Configuration search/scope state. Please preserve loc.search/searchStr or navigate through the router.

const defaultCustomValue =
const explicitCustomValue =
editedEntry.isExplicitlySet && !entryHasInfiniteValue(editedEntry) ? editedEntry.value : '';
// For enum-style configs (boolean/select), fall back to the first option when there's

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fallback ignores the resolved/inherited value whenever the config is not explicitly set. For a non-explicit BOOLEAN/SELECT with current value true or a non-first enum, opening Custom and saving without changing the dropdown writes false/the first enum. Please seed from editedEntry.value/current/default synonym when present, and only fall back to the first option when no resolved value exists.


const defaultConfigSynonym = editedEntry.synonyms
?.filter(({ source }) => source !== 'DYNAMIC_TOPIC_CONFIG')
.sort((a, b) => SOURCE_PRIORITY_ORDER.indexOf(a.source) - SOURCE_PRIORITY_ORDER.indexOf(b.source))[0];

@malinskibeniamin malinskibeniamin Jun 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the new reset path (handleReset, lines 197-205; button at 271-274), Reset to default sends DELETE immediately, outside the normal Save path and without its pending state. A slow reset followed by Save can race DELETE vs SET, and an accidental click can remove retention/durability overrides. Please route reset through the Default + Save flow, or add confirmation plus shared isMutating disabling for Reset/Save/Cancel.


const ALLOWED_CATEGORIES = new Set<string>(CONFIG_CATEGORIES.map((c) => c.name));

function categoryForEntry(entry: ConfigEntryExtended): string {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This allow-list collapses existing backend categories like Iceberg, Schema Registry and Validation, Message Handling, Compression, and Storage Internals into Other. The old layout preserved those sections, so the new sidebar loses useful navigation. Please preserve backend categories/order or include the previous display order before falling back to Other.

const [filter, setFilter] = useState<string>('');
const ConfigurationEditorLegacy: FC<ConfigurationEditorProps> = (props) => {
const navigate = useNavigate({ from: '/topics/$topicName/' });
const { configFilter = '' } = useSearch({ from: '/topics/$topicName/' });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This writes raw config search input to the URL. In the legacy path below, configFilter still matches x.value, so searching a sensitive/internal config value leaves it in browser history and shareable links. Please keep config filters local/session-only, or make URL-backed search strictly name/description-only and not value-backed.


const consumers = rawConsumers ?? [];

const paginationParams = usePaginationParams(consumers.length, uiState.topicSettings.consumerPageSize);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The URL page index is passed straight into React Table with autoResetPageIndex: false. A stale ?consumerPage=999 (same pattern in partitions and ACL) renders “No data found” even when rows exist; the previous usePaginationParams clamped this. Please clamp/reset pageIndex when data length or pageSize changes and repair the URL.

<WarningIcon color="orange" size={20} />
</Box>
<Popover>
<PopoverTrigger asChild>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new icon-only button without an accessible name. Screen reader users only get an unlabeled button for partition error details. Please use the registry Button icon size (or add aria-label="Show partition error details").

} catch (inspectError) {
console.error('Could not inspect container (likely already removed):', inspectError.message);
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new log consumer dumps all backend container output on startup failure. These e2e runs can include enterprise config/license material and auth/SASL/JWT errors, so full logs can leak secrets into CI output. Please redact token/password/license-like values and cap this to a safe tail before printing.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've now reverted all changes in global-setup

@jvorcak jvorcak force-pushed the UX-998-migrate-topic-detail-to-ui-registry branch from 96c50fb to ed337aa Compare June 15, 2026 14:07
jvorcak added 3 commits June 16, 2026 09:41
The single-page guard (getPageCount() > 1) hid the 'Page X of Y'
indicator and nav controls for single-page tables, breaking the
AIAgentsListPage visual-regression test which asserts 'Page 1 of 1'.
DataTablePagination is a shared registry primitive; restore the
original always-visible footer to keep behavior consistent app-wide.
@jvorcak jvorcak requested a review from malinskibeniamin June 16, 2026 08:11
…e filters

- migrate javascript-filter-modal to registry Dialog/Input/Label/typography
- show disabled-reason tooltips on Add filter menu items (partition/JS)
- disable JS filter while continuous pagination is enabled
- show "All" in partition dropdown when partition is -1
- restyle message filter tags with Tailwind (lost .filterTag legacy class)
@jvorcak jvorcak requested review from a team, datamali, eblairmckee and yougotashovel and removed request for a team June 16, 2026 15:38

@graham-rp graham-rp left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for self-hosted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants