Remove mobx from quotas page by jvorcak · Pull Request #2174 · redpanda-data/console

jvorcak · 2026-01-23T09:08:08Z

No description provided.

blacksmith-sh · 2026-01-23T09:59:39Z

Found 1 test failure on Blacksmith runners:

Failure

Test	View Logs
`[chromium] › quotas/` `pagination.spec.ts:9:7 › Quotas - Display 50 quotas › should create 50 quotas and verif` `y all are visible on the page › Verify all 50 quotas are visible on the page`	View Logs

frontend/tests/test-variant-console-enterprise/playwright/.auth/user.json

frontend/src/routes/quotas.tsx

frontend/src/components/pages/quotas/quotas-list.tsx

jvorcak · 2026-02-03T17:40:16Z

@malinskibeniamin I've addressed all your comments

malinskibeniamin · 2026-02-04T09:02:17Z

frontend/src/components/pages/quotas/quotas-list.tsx

+      // Transform REST API settings to protobuf Value format
+      const values: Quota_Value[] = item.settings.map(
+        (setting: QuotaResponseSetting): Quota_Value => ({
+          valueType: mapValueTypeToProto(setting.key),
+          value: setting.value,
+          $typeName: 'redpanda.api.dataplane.v1.Quota.Value',
+        })
      );


Feels like we should be doing this inside the hook like useListQuotas

Also having $typeName here feels awkward, should we try to create connect query key or similar to keep REST/gRPC calls use the same cache

malinskibeniamin · 2026-02-04T09:02:54Z

frontend/tests/test-variant-console/quotas/quota-pagination.spec.ts

+
+        await createClientIdQuota({
+          clientId: quotaClientId,
+          producerByteRate: 6_291_456, // 6MB


nit: 6 * ONE_MB (use storage utils)

malinskibeniamin · 2026-02-04T09:03:11Z

frontend/tests/test-variant-console/quotas/quota-pagination.spec.ts

+
+        await createClientIdQuota({
+          clientId: quotaClientId,
+          producerByteRate: 5_242_880, // 5MB


malinskibeniamin · 2026-02-04T09:03:51Z

frontend/tests/test-variant-console/quotas/quota-pagination.spec.ts

+  test('should navigate to next page using pagination controls', async ({ page }) => {
+    const quotaPage = new QuotaPage(page);
+    const timestamp = Date.now();
+    const quotaIds: string[] = [];
+    const QUOTA_COUNT = 55; // More than one page
+    const PAGE_SIZE = 20;


maybe just to make the test faster, make it a page size of 1 and quota count of 2?

malinskibeniamin · 2026-02-04T09:04:02Z

frontend/tests/test-variant-console/quotas/quota-display.spec.ts

+    });
+
+    await test.step('Reload page and verify quota still visible', async () => {
+      await quotaPage.reloadPage();


do we need a full page reload?

malinskibeniamin · 2026-02-04T09:04:32Z

frontend/src/routes/quotas.tsx

+import { uiState } from '../state/ui-state';
+
+const quotasSearchSchema = z.object({
+  page: fallback(z.number().int().min(0).optional(), 0),


page 0 is accurate? Shouldn't it be 1?

malinskibeniamin · 2026-02-04T09:04:43Z

@claude review

claude · 2026-02-04T09:05:04Z

Claude finished @malinskibeniamin's task in 2m 2s —— View job

Code Review Complete

I've reviewed the PR to remove MobX from the quotas page. Here are my findings:

Architecture & Implementation ✓

The migration from MobX to React Query + TanStack Router is well-executed:

Successfully removed MobX observer pattern and replaced with modern React patterns
Properly integrated TanStack Router for URL-based pagination state
Good separation of concerns with the new useQuotasQuery hook

Issues Found

1. Missing `fallback` property in route definition

Location: frontend/src/routes/quotas.tsx:22

The page parameter uses fallback() but is optional. According to the review comment, you should add an explicit fallback property.

Current:

page: fallback(z.number().int().min(0).optional(), 0),

This should be addressed per the review comment.

2. Hook location - should move to hooks directory

Location: frontend/src/hooks/use-quotas-query.ts

The file is already in the correct location (frontend/src/hooks/), so this review comment may have been addressed. Good job!

3. Link component usage inconsistency

Location: frontend/src/components/pages/quotas/quotas-list.tsx:15

import { Link } from 'components/redpanda-ui/components/typography';

Issue: Using the legacy Link from typography which wraps react-router-dom. Should use TanStack Router's Link component or the Redpanda UI registry link for consistency.

Current usage (line 147):

<Link href="https://docs.redpanda.com/docs/manage/console/" target="_blank">

Recommendation: For external links, the current usage is actually fine. TanStack Router's Link is for internal navigation. However, for consistency across the codebase as noted in the review, consider using the UI registry link component.

4. Cache key strategy needs alignment

Location: frontend/src/hooks/use-quotas-query.ts:24-28 and frontend/src/components/pages/quotas/quotas-list.tsx:93

Issue: The $typeName property is manually added to transform REST data into proto-like format. This creates inconsistency between REST and gRPC calls.

Current approach:

// Hook creates Connect Query key for REST endpoint
const queryKey = createConnectQueryKey({
  schema: listQuotas,
  input: {},
  cardinality: 'finite',
});

Then in component:

$typeName: 'redpanda.api.dataplane.v1.Quota.Value',

Recommendation: This bridging strategy should be in the hook, not the component. Consider creating a unified cache key strategy or keeping REST/gRPC keys separate until the protobuf endpoint is ready. The manual $typeName injection feels like a leaky abstraction.

5. Test improvements - storage utilities

Locations:

frontend/tests/test-variant-console/quotas/quota-pagination.spec.ts:209 - producerByteRate: 5_242_880, // 5MB
frontend/tests/test-variant-console/quotas/quota-pagination.spec.ts:284 - producerByteRate: 6_291_456, // 6MB

Issue: Magic numbers for byte values. Should use storage utility constants.

Recommendation:

// Instead of: producerByteRate: 6_291_456
// Use: producerByteRate: 6 * ONE_MB

However, I couldn't find ONE_MB or similar constants in the codebase. If they don't exist, this is fine as-is, or you could define them in the test utilities.

6. Test optimization opportunity

Location: frontend/tests/test-variant-console/quotas/quota-pagination.spec.ts:36

Creating 55 quotas per test may be slow. Consider reducing to minimum needed:

const QUOTA_COUNT = 55; // More than one page
const PAGE_SIZE = 20;

Suggestion: Use pageSize: 1 and QUOTA_COUNT: 2 for faster tests while still validating pagination.

7. Full page reload necessity?

Location: frontend/tests/test-variant-console/quotas/quota-display.spec.ts:153

await quotaPage.reloadPage();

Question: Does the test need a full page reload (page.goto()) or would page.reload() suffice? Full navigation might be slower and unnecessary if just testing data persistence.

8. Page numbering - zero-indexed?

Location: frontend/src/routes/quotas.tsx:22

page: fallback(z.number().int().min(0).optional(), 0),

Question: Is page 0 accurate for the first page? Most UIs use 1-indexed pages for user-facing display. TanStack Table uses 0-indexed internally, but verify this is the intended UX.

What's Good ✓

Comprehensive test coverage - Excellent E2E tests covering display, pagination, edge cases
Type safety - Good use of TypeScript types and Zod schemas
Error handling - Proper permission and error state handling
Code organization - Clean separation of concerns
Pagination implementation - URL-based state management is correct
Test structure - Well-organized test steps with clear cleanup

Minor Notes

The .gitignore addition of /playwright/.auth/ is correct and prevents accidental auth file commits
The PR has 26 commits - consider squashing before merge for cleaner history
Good use of refetchOnMount: 'always' in the query hook for data freshness

Summary

The MobX removal is successful and the new implementation follows modern React patterns. Main concerns are:

Cache strategy alignment (REST vs gRPC)
Link component consistency
Minor test optimizations

Most issues are minor polish items that don't block functionality.

jvorcak · 2026-02-04T14:45:41Z

frontend/src/hooks/use-quotas-query.ts

+  return useQuery<QuotaResponse | null>({
+    queryKey,
+    queryFn: async () => {
+      const response = await config.fetch(`${config.restBasePath}/quotas`, {


We should move from REST to grpc if possible

jvorcak added 2 commits January 23, 2026 10:06

Remove mobx from quotas page

bb62009

Fixes lint

9f3f8dd

jvorcak force-pushed the refactor/remove-mobx-from-quotas-page branch from 123ce0c to 9f3f8dd Compare January 23, 2026 09:17

jvorcak added 2 commits January 23, 2026 10:25

Quotas: do comparisions with constants instead of numbers

405bdb7

Fixes quotas page

a6454a9

redpanda-data deleted a comment from blacksmith-sh bot Jan 23, 2026

Fixes quotas page

48cd773

jvorcak force-pushed the refactor/remove-mobx-from-quotas-page branch from d6907a0 to 48cd773 Compare January 23, 2026 11:44

Update breadcrumbs in Quotas after mobx migration

93c2ef1

jvorcak force-pushed the refactor/remove-mobx-from-quotas-page branch from f180b54 to 3b49d7c Compare January 23, 2026 13:09

wait for all quotas to load (without pagination)

e99d002

jvorcak force-pushed the refactor/remove-mobx-from-quotas-page branch from 3b49d7c to e99d002 Compare January 23, 2026 13:17

jvorcak added 2 commits January 24, 2026 19:41

Decrease e2e test limit to 15

084b0af

Dont' run pagination tests in paralllel

bb4001b

jvorcak force-pushed the refactor/remove-mobx-from-quotas-page branch from 4275d70 to bb4001b Compare January 24, 2026 20:16

jvorcak marked this pull request as ready for review January 26, 2026 16:31

More quotas e2e tests

37c9237

jvorcak force-pushed the refactor/remove-mobx-from-quotas-page branch from 68210c4 to 37c9237 Compare January 26, 2026 18:35

jvorcak added 2 commits February 3, 2026 15:12

Fixes quota tests

74ad66d

Fixes e2e

e104821

jvorcak force-pushed the refactor/remove-mobx-from-quotas-page branch from 9d48946 to e104821 Compare February 3, 2026 14:28

jvorcak requested a review from malinskibeniamin February 3, 2026 14:52

malinskibeniamin reviewed Feb 3, 2026

View reviewed changes

frontend/tests/test-variant-console-enterprise/playwright/.auth/user.json Outdated Show resolved Hide resolved

malinskibeniamin reviewed Feb 3, 2026

View reviewed changes

frontend/src/routes/quotas.tsx Show resolved Hide resolved

malinskibeniamin reviewed Feb 3, 2026

View reviewed changes

frontend/src/components/pages/quotas/quotas-list.tsx Outdated Show resolved Hide resolved

malinskibeniamin reviewed Feb 3, 2026

View reviewed changes

frontend/src/components/pages/quotas/quotas-list.tsx Outdated Show resolved Hide resolved

Improved e2e tests for quotas

d463bdf

jvorcak force-pushed the refactor/remove-mobx-from-quotas-page branch from 7bac1c4 to d463bdf Compare February 3, 2026 16:51

malinskibeniamin reviewed Feb 4, 2026

View reviewed changes

jvorcak commented Feb 4, 2026

View reviewed changes

Conversation

jvorcak commented Jan 23, 2026

Uh oh!

blacksmith-sh bot commented Jan 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Failure

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jvorcak commented Feb 3, 2026

Uh oh!

malinskibeniamin Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

malinskibeniamin Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

malinskibeniamin Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

malinskibeniamin Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

malinskibeniamin Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

malinskibeniamin Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

malinskibeniamin commented Feb 4, 2026

Uh oh!

claude bot commented Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Complete

Architecture & Implementation ✓

Issues Found

1. Missing fallback property in route definition

2. Hook location - should move to hooks directory

3. Link component usage inconsistency

4. Cache key strategy needs alignment

5. Test improvements - storage utilities

6. Test optimization opportunity

7. Full page reload necessity?

8. Page numbering - zero-indexed?

What's Good ✓

Minor Notes

Summary

Uh oh!

jvorcak Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

blacksmith-sh bot commented Jan 23, 2026 •

edited

Loading

malinskibeniamin Feb 4, 2026 •

edited

Loading

claude bot commented Feb 4, 2026 •

edited

Loading

1. Missing `fallback` property in route definition