Browse Source

perf(nodes): keep the node table element across unrelated re-renders

rc-table re-runs every cell renderer whenever the Table re-renders, and
NodeList rebuilt its columns and table props on every render (the relative
time formatter was a fresh function each time). Any re-render of the Nodes
page therefore re-rendered all rows even when no node had changed: about
390ms per re-render for 150 nodes in jsdom.

The formatter is now stable and the table element is memoized on its
inputs, so a re-render that leaves the nodes untouched costs 0.5ms. A
heartbeat push that does change the nodes still re-renders every row.
Sanaei 14 hours ago
parent
commit
3fa44915c1
2 changed files with 124 additions and 42 deletions
  1. 56 42
      frontend/src/pages/nodes/NodeList.tsx
  2. 68 0
      frontend/src/test/node-list-rerender.test.tsx

+ 56 - 42
frontend/src/pages/nodes/NodeList.tsx

@@ -145,17 +145,22 @@ function formatUptime(secs?: number): string {
   return `${mins}m`;
 }
 
+// Stable per language: the columns memo depends on it, and a fresh function each
+// render rebuilt every column, re-rendering all rows on each heartbeat push.
 function useRelativeTime() {
   const { t } = useTranslation();
-  return (unixSeconds?: number) => {
-    if (!unixSeconds) return t('pages.nodes.never');
-    const diffSec = Math.max(0, Math.floor(Date.now() / 1000 - unixSeconds));
-    if (diffSec < 5) return t('pages.nodes.justNow');
-    if (diffSec < 60) return `${diffSec}s`;
-    if (diffSec < 3600) return `${Math.floor(diffSec / 60)}m`;
-    if (diffSec < 86400) return `${Math.floor(diffSec / 3600)}h`;
-    return `${Math.floor(diffSec / 86400)}d`;
-  };
+  return useMemo(
+    () => (unixSeconds?: number) => {
+      if (!unixSeconds) return t('pages.nodes.never');
+      const diffSec = Math.max(0, Math.floor(Date.now() / 1000 - unixSeconds));
+      if (diffSec < 5) return t('pages.nodes.justNow');
+      if (diffSec < 60) return `${diffSec}s`;
+      if (diffSec < 3600) return `${Math.floor(diffSec / 60)}m`;
+      if (diffSec < 86400) return `${Math.floor(diffSec / 3600)}h`;
+      return `${Math.floor(diffSec / 86400)}d`;
+    },
+    [t],
+  );
 }
 
 export default function NodeList({
@@ -530,6 +535,47 @@ export default function NodeList({
     ],
   );
 
+  // rc-table re-runs every cell renderer whenever the Table re-renders, so keep the
+  // same element until its inputs change rather than re-rendering all rows each time.
+  const nodeTable = useMemo(
+    () => (
+      <Table<NodeRow>
+        dataSource={dataSource}
+        columns={columns}
+        pagination={false}
+        loading={loading}
+        scroll={{ x: 'max-content' }}
+        size="middle"
+        rowKey="key"
+        rowSelection={
+          dataSource.length > 1
+            ? {
+                selectedRowKeys: selectedIds,
+                onChange: (keys) =>
+                  onSelectionChange(keys.filter((k) => typeof k === 'number') as number[]),
+                getCheckboxProps: (record) => ({
+                  disabled: !!record.transitive || !isUpdateEligible(record),
+                }),
+              }
+            : undefined
+        }
+        locale={{
+          emptyText: (
+            <div className="card-empty">
+              <ClusterOutlined style={{ fontSize: 32, marginBottom: 8 }} />
+              <div>{t('noData')}</div>
+            </div>
+          ),
+        }}
+        expandable={{
+          expandedRowRender: (record) => <NodeHistoryPanel node={record} />,
+          rowExpandable: (record) => !record.transitive,
+        }}
+      />
+    ),
+    [dataSource, columns, loading, selectedIds, onSelectionChange, t],
+  );
+
   return (
     <Card size="small" hoverable>
       <div className="toolbar">
@@ -806,39 +852,7 @@ export default function NodeList({
           </Modal>
         </>
       ) : (
-        <Table<NodeRow>
-          dataSource={dataSource}
-          columns={columns}
-          pagination={false}
-          loading={loading}
-          scroll={{ x: 'max-content' }}
-          size="middle"
-          rowKey="key"
-          rowSelection={
-            dataSource.length > 1
-              ? {
-                  selectedRowKeys: selectedIds,
-                  onChange: (keys) =>
-                    onSelectionChange(keys.filter((k) => typeof k === 'number') as number[]),
-                  getCheckboxProps: (record) => ({
-                    disabled: !!record.transitive || !isUpdateEligible(record),
-                  }),
-                }
-              : undefined
-          }
-          locale={{
-            emptyText: (
-              <div className="card-empty">
-                <ClusterOutlined style={{ fontSize: 32, marginBottom: 8 }} />
-                <div>{t('noData')}</div>
-              </div>
-            ),
-          }}
-          expandable={{
-            expandedRowRender: (record) => <NodeHistoryPanel node={record} />,
-            rowExpandable: (record) => !record.transitive,
-          }}
-        />
+        nodeTable
       )}
     </Card>
   );

+ 68 - 0
frontend/src/test/node-list-rerender.test.tsx

@@ -0,0 +1,68 @@
+import type { ReactNode } from 'react';
+import { render } from '@testing-library/react';
+import { QueryClientProvider } from '@tanstack/react-query';
+import { describe, expect, it, vi } from 'vitest';
+
+import { ThemeProvider } from '@/hooks/useTheme';
+import NodeList from '@/pages/nodes/NodeList';
+import type { NodeRecord } from '@/schemas/node';
+
+import { makeTestQueryClient } from './test-utils';
+
+const updateChecks = vi.hoisted(() => ({ count: 0 }));
+
+vi.mock('@/lib/panel-version', async (importOriginal) => {
+  const actual = await importOriginal<typeof import('@/lib/panel-version')>();
+  return {
+    ...actual,
+    isPanelUpdateAvailable: (...args: Parameters<typeof actual.isPanelUpdateAvailable>) => {
+      updateChecks.count++;
+      return actual.isPanelUpdateAvailable(...args);
+    },
+  };
+});
+
+// Every heartbeat push re-rendered all rows, unchanged ones too: the columns and
+// table props were rebuilt on each render, so every cell re-ran its renderer.
+describe('NodeList re-render', () => {
+  it('leaves the rows alone when its parent re-renders with the same nodes', () => {
+    const queryClient = makeTestQueryClient();
+    const wrapper = ({ children }: { children: ReactNode }) => (
+      <QueryClientProvider client={queryClient}>
+        <ThemeProvider>{children}</ThemeProvider>
+      </QueryClientProvider>
+    );
+    const nodes: NodeRecord[] = [1, 2, 3].map((id) => ({
+      id,
+      name: `node-${id}`,
+      guid: `g${id}`,
+      transitive: false,
+      enable: true,
+      status: 'online',
+      panelVersion: '3.0.0',
+    }));
+    const noop = () => {};
+    const props = {
+      nodes,
+      isMobile: false,
+      latestVersion: '3.0.1',
+      selectedIds: [] as number[],
+      onSelectionChange: noop,
+      onAdd: noop,
+      onMtls: noop,
+      onEdit: noop,
+      onDelete: noop,
+      onProbe: noop,
+      onToggleEnable: noop,
+      onUpdateNode: noop,
+      onUpdateSelected: noop,
+    };
+    const view = render(<NodeList {...props} />, { wrapper });
+    expect(updateChecks.count).toBeGreaterThan(0);
+
+    updateChecks.count = 0;
+    view.rerender(<NodeList {...props} />);
+
+    expect(updateChecks.count).toBe(0);
+  });
+});