Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { getActiveLatencyFieldName } from '~/app/pages/modelCatalog/utils/modelC
import { ManageColumnsModal } from '~/app/shared/components/manageColumns/ManageColumnsModal';
import HardwareConfigurationTableRow from './HardwareConfigurationTableRow';
import HardwareConfigurationFilterToolbar from './HardwareConfigurationFilterToolbar';
import { useHardwareConfigColumns } from './useHardwareConfigColumns';
import { useHardwareConfigColumns, ControlledTableSortProps } from './useHardwareConfigColumns';

type HardwareConfigurationTableProps = {
performanceArtifacts: CatalogPerformanceMetricsArtifact[];
Expand All @@ -28,8 +28,12 @@ const HardwareConfigurationTable: React.FC<HardwareConfigurationTableProps> = ({
// Get the active latency filter field name (if any)
const activeLatencyField = getActiveLatencyFieldName(filterData);

// Use the custom hook that combines manage columns with latency filter effects
const { columns, manageColumnsResult } = useHardwareConfigColumns(activeLatencyField);
// Use the custom hook that combines manage columns with the latency filter + sort logic
const {
columns,
manageColumnsResult,
sortState: { sortIndex, sortDirection, onSortIndexChange, onSortDirectionChange },
} = useHardwareConfigColumns(activeLatencyField);

if (isLoading) {
return <Spinner size="lg" />;
Expand Down Expand Up @@ -59,6 +63,14 @@ const HardwareConfigurationTable: React.FC<HardwareConfigurationTableProps> = ({
/>
);

// Controlled sort props exist at runtime but not in mod-arch-shared Table typings yet
const controlledSortProps: ControlledTableSortProps = {
sortIndex,
sortDirection,
onSortIndexChange,
onSortDirectionChange,
};

return (
<>
<OuterScrollContainer>
Expand All @@ -71,9 +83,10 @@ const HardwareConfigurationTable: React.FC<HardwareConfigurationTableProps> = ({
columns={columns}
toolbarContent={toolbarContent}
onClearFilters={handleClearFilters}
defaultSortColumn={0}
defaultSortColumn={sortIndex}
{...controlledSortProps}
emptyTableView={<DashboardEmptyTableView onClearFilters={handleClearFilters} />}
rowRenderer={(artifact) => (
rowRenderer={(artifact: CatalogPerformanceMetricsArtifact) => (
<HardwareConfigurationTableRow
key={artifact.customProperties?.config_id?.string_value}
performanceArtifact={artifact}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,24 @@ import {
HARDWARE_CONFIG_COLUMNS_STORAGE_KEY,
} from './HardwareConfigurationTableColumns';

/** Controlled sort props for the Table component */
export interface ControlledTableSortProps {
sortIndex: number;
sortDirection: 'asc' | 'desc';
onSortIndexChange: (index: number) => void;
onSortDirectionChange: (direction: 'asc' | 'desc') => void;
}

interface UseHardwareConfigColumnsResult {
/** Final columns to render in the table (sticky + visible managed columns) */
columns: HardwareConfigColumn[];
/** Result from useManageColumns hook, to be passed directly to ManageColumnsModal */
manageColumnsResult: UseManageColumnsResult<CatalogPerformanceMetricsArtifact>;
/**
* Lifted sort state.
* Simplified by reusing the interface we'll use for the Table assertion.
*/
sortState: ControlledTableSortProps;
}

/**
Expand Down Expand Up @@ -55,6 +68,10 @@ export const useHardwareConfigColumns = (
// Track the previous latency filter to detect changes
const prevLatencyFieldRef = React.useRef<LatencyMetricFieldName | undefined>(undefined);

// sort state
const [sortIndex, setSortIndex] = React.useState<number>(0);
const [sortDirection, setSortDirection] = React.useState<'asc' | 'desc'>('asc');

// Separate sticky columns (always visible) from manageable columns
const { stickyColumns, manageableColumns } = React.useMemo(() => {
const sticky = hardwareConfigColumns.filter((col) => STICKY_COLUMN_FIELDS.includes(col.field));
Expand All @@ -74,7 +91,13 @@ export const useHardwareConfigColumns = (
defaultVisibleColumnIds: DEFAULT_VISIBLE_COLUMN_FIELDS,
});

// Effect to update visible columns when latency filter changes
// Combine sticky + visible managed columns
const columns = React.useMemo(
(): HardwareConfigColumn[] => [...stickyColumns, ...manageColumnsResult.visibleColumns],
[stickyColumns, manageColumnsResult.visibleColumns],
);

// Combined effect to update visible columns AND sort when latency filter changes
React.useEffect(() => {
// Only react to changes, not initial mount
if (prevLatencyFieldRef.current === activeLatencyField) {
Expand All @@ -96,33 +119,59 @@ export const useHardwareConfigColumns = (
// - Keep all non-latency/non-TPS columns
// - Remove all latency and TPS columns
// - Add the active latency column and matching TPS column
const newVisibleIds = manageColumnsResult.visibleColumnIds.filter((id) => {
const isLatencyColumn = isLatencyColumnField(id);
const isTpsColumn = isTpsColumnField(id);
return !isLatencyColumn && !isTpsColumn;
});

// Add the active latency column (if not already present)
if (!newVisibleIds.includes(activePropertyKey)) {
newVisibleIds.push(activePropertyKey);
const newVisibleIds = manageColumnsResult.visibleColumnIds.filter(
(id) => !isLatencyColumnField(id) && !isTpsColumnField(id),
);

// Use a Set to ensure uniqueness without manual .includes() checks
const updatedIds = Array.from(
new Set([...newVisibleIds, activePropertyKey, matchingTpsPropertyKey]),
);

manageColumnsResult.setVisibleColumnIds(updatedIds);

const finalColumns = [
...stickyColumns,
...manageableColumns.filter((c) => updatedIds.includes(c.field)),
];
const newIndex = finalColumns.findIndex((col) => col.field === activePropertyKey);

if (newIndex !== -1) {
setSortIndex(newIndex);
setSortDirection('asc');
}
}, [activeLatencyField, manageColumnsResult, stickyColumns, manageableColumns]);

// Add the matching TPS column (if not already present)
if (!newVisibleIds.includes(matchingTpsPropertyKey)) {
newVisibleIds.push(matchingTpsPropertyKey);
// Ensure sort is set correctly when columns are ready (handles initial mount case)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up using two useEffect hooks to keep the sorting in sync. The first one handles the immediate visibility change when a filter is picked, and the second one acts as a 'safety check' to make sure the sortIndex actually points to the right column after the table re-renders.

I tried combining them, but it kept breaking the sort during transitions. This is the best I could come up with, but let me know if you think there's a cleaner way to handle the state dependency

React.useEffect(() => {
if (!activeLatencyField || columns.length === 0) {
return;
}

manageColumnsResult.setVisibleColumnIds(newVisibleIds);
}, [activeLatencyField, manageColumnsResult]);
const parsed = parseLatencyFilterKey(activeLatencyField);
const activePropertyKey = parsed.propertyKey;
const currentIndex = columns.findIndex((col) => col.field === activePropertyKey);

// Only update if the column exists and sort isn't already set correctly
if (currentIndex !== -1 && (sortIndex !== currentIndex || sortDirection !== 'asc')) {
setSortIndex(currentIndex);
setSortDirection('asc');
}
}, [activeLatencyField, columns, sortIndex, sortDirection]);

// Combine sticky + visible managed columns
const columns = React.useMemo(
(): HardwareConfigColumn[] => [...stickyColumns, ...manageColumnsResult.visibleColumns],
[stickyColumns, manageColumnsResult.visibleColumns],
const sortState = React.useMemo(
() => ({
sortIndex,
sortDirection,
onSortIndexChange: setSortIndex,
onSortDirectionChange: setSortDirection,
}),
[sortIndex, sortDirection],
);

return {
columns,
manageColumnsResult,
sortState,
};
};
Loading