Skip to content

Fixes #27162: table version history shows current datatype instead of historical one #27478

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
miantalha45 wants to merge 11 commits into
base: main
Choose a base branch
from
+87 −72
Open

Fixes #27162: table version history shows current datatype instead of historical one #27478

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b2ee2a7
fix: use historical column snapshot in table version view
miantalha45 Apr 17, 2026
e5bb63c
Merge branch 'main' into table-history-issue
miantalha45 Apr 17, 2026
3a523f0
Merge branch 'main' into table-history-issue
miantalha45 Apr 17, 2026
b76028b
Merge branch 'main' into table-history-issue
miantalha45 Apr 17, 2026
0bc5ac7
fix paging issue
miantalha45 Apr 17, 2026
d4475c5
Merge branch 'main' into table-history-issue
miantalha45 Apr 19, 2026
2c4882f
Merge branch 'main' into table-history-issue
miantalha45 Apr 20, 2026
640ab1a
Fix github suggestions
miantalha45 Apr 20, 2026
c8fc9f4
Merge branch 'main' into table-history-issue
miantalha45 Apr 20, 2026
b5d3ddf
fix copilot suggestions
miantalha45 Apr 20, 2026
321ecb3
Merge branch 'main' into table-history-issue
miantalha45 Apr 20, 2026
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
86 changes: 27 additions & 59 deletions ...-ui/src/main/resources/ui/src/components/Database/TableVersion/TableVersion.component.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,6 @@ import {
import { Operation } from '../../../generated/entity/policies/policy';
import { TagSource } from '../../../generated/type/tagLabel';
import { usePaging } from '../../../hooks/paging/usePaging';
import { useFqn } from '../../../hooks/useFqn';
import {
getTableColumnsByFQN,
searchTableColumnsByFQN,
} from '../../../rest/tableAPI';
import { getPartialNameFromTableFQN } from '../../../utils/CommonUtils';
import {
getColumnsDataWithVersionChanges,
Expand Down Expand Up @@ -89,7 +84,6 @@ const TableVersion: React.FC<TableVersionProp> = ({
paging,
handlePagingChange,
} = usePaging(PAGE_SIZE_LARGE);
const { fqn: tableFqn } = useFqn();
const [searchText, setSearchText] = useState('');
// Pagination state for columns
const [tableColumns, setTableColumns] = useState<Column[]>([]);
Expand All @@ -98,47 +92,23 @@ const TableVersion: React.FC<TableVersionProp> = ({
currentVersionData.changeDescription as ChangeDescription
);

// Function to fetch paginated columns or search results
const fetchPaginatedColumns = useCallback(
async (page = 1, searchQuery?: string) => {
if (!tableFqn) {
return;
}

setColumnsLoading(true);
try {
const offset = (page - 1) * pageSize;
const allHistoricalColumns = useMemo(
() => pruneEmptyChildren(currentVersionData.columns) ?? [],
[currentVersionData.columns]
);

// Use search API if there's a search query, otherwise use regular pagination
const response = searchQuery
? await searchTableColumnsByFQN(tableFqn, {
q: searchQuery,
limit: pageSize,
offset: offset,
fields: 'tags',
})
: await getTableColumnsByFQN(tableFqn, {
limit: pageSize,
offset: offset,
fields: 'tags',
});
const filteredHistoricalColumns = useMemo(() => {
if (!searchText) {
return allHistoricalColumns;
}
const lower = searchText.toLowerCase();

setTableColumns(pruneEmptyChildren(response.data) || []);
handlePagingChange(response.paging);
} catch {
// Set empty state if API fails
setTableColumns([]);
handlePagingChange({
offset: 1,
limit: pageSize,
total: 0,
});
} finally {
setColumnsLoading(false);
}
},
[tableFqn, pageSize]
);
return allHistoricalColumns.filter(
(col) =>
col.name?.toLowerCase().includes(lower) ||
col.description?.toLowerCase().includes(lower)
);
}, [allHistoricalColumns, searchText]);
Comment on lines +99 to +105
Copy link

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

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

filteredHistoricalColumns only matches on top-level name and description, and it is not recursive. This changes search behavior vs VersionTable’s searchInColumns (which searches displayName, dataType, and children). Consider reusing the same searchInColumns logic (or an equivalent recursive filter) for the full historical column tree before slicing, so nested/typed matches aren’t dropped.

Copilot uses AI. Check for mistakes.
Comment on lines +99 to +105
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

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

VersionTable already performs local column filtering based on its own searchText state, and this component now filters again via filteredHistoricalColumns. This duplicates work and can cause brief UI inconsistency (e.g., user types a search term and the child filters the current page slice before the parent updates pagination/slice). Consider making a single source of truth for filtering—either let TableVersion own search + pagination and disable the internal filtering in VersionTable for this usage, or pass the full (unfiltered) columns to VersionTable and let it handle filtering while TableVersion only handles pagination totals/offsets.

Copilot uses AI. Check for mistakes.

const handleSearchAction = useCallback(
(searchValue: string) => {
Expand All @@ -150,10 +120,9 @@ const TableVersion: React.FC<TableVersionProp> = ({

const handleColumnsPageChange = useCallback(
({ currentPage }: PagingHandlerParams) => {
fetchPaginatedColumns(currentPage, searchText);
handlePageChange(currentPage);
},
[paging, fetchPaginatedColumns, searchText]
[handlePageChange]
);

const paginationProps = useMemo(
Expand Down Expand Up @@ -355,20 +324,19 @@ const TableVersion: React.FC<TableVersionProp> = ({
]
);

// Fetch columns when search changes
useEffect(() => {
if (tableFqn && !isVersionLoading) {
// Reset to first page when search changes
fetchPaginatedColumns(currentPage, searchText || undefined);
if (isVersionLoading) {
return;
}
}, [
isVersionLoading,
tableFqn,
searchText,
fetchPaginatedColumns,
pageSize,
currentPage,
]);
const offset = (currentPage - 1) * pageSize;
setTableColumns(filteredHistoricalColumns.slice(offset, offset + pageSize));
handlePagingChange({
offset,
limit: pageSize,
total: filteredHistoricalColumns.length,
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

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

The local paging state is updated with { offset, limit, total } only, leaving paging.after/paging.before unset. However NextPrevious disables Next/Previous in cursor mode when paging.after/before are missing. Since paginationProps.isNumberBased is still Boolean(searchText) (cursor mode when search is empty), this change can make column pagination non-functional by disabling both buttons. Align the paging mode with the data you provide: either always use number-based paging here, or compute/set after/before cursors for local pagination.

Suggested change
setTableColumns(filteredHistoricalColumns.slice(offset, offset + pageSize));
handlePagingChange({
offset,
limit: pageSize,
total: filteredHistoricalColumns.length,
const total = filteredHistoricalColumns.length;
const nextOffset = offset + pageSize;
const previousOffset = Math.max(offset - pageSize, 0);
setTableColumns(filteredHistoricalColumns.slice(offset, offset + pageSize));
handlePagingChange({
offset,
limit: pageSize,
total,
before: offset > 0 ? String(previousOffset) : undefined,
after: nextOffset < total ? String(nextOffset) : undefined,

Copilot uses AI. Check for mistakes.
});
setColumnsLoading(false);
Comment thread
gitar-bot[bot] marked this conversation as resolved.
Outdated
Copy link

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

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

columnsLoading is now driven entirely by synchronous local filtering/slicing, but it still starts as true and is set to false in an effect. This will render the schema table in a loading state briefly even when isVersionLoading is false. Consider removing columnsLoading state entirely (or initialize it to false and only use it when truly async work is happening).

Copilot uses AI. Check for mistakes.
}, [isVersionLoading, filteredHistoricalColumns, currentPage, pageSize]);
Copy link

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

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

The useEffect dependency array omits handlePagingChange even though it’s used inside the effect. This is likely to trigger react-hooks/exhaustive-deps lint warnings and can capture a stale reference if the hook implementation changes. Add handlePagingChange to the dependency list (or refactor to avoid the effect).

Suggested change
}, [isVersionLoading, filteredHistoricalColumns, currentPage, pageSize]);
}, [
isVersionLoading,
filteredHistoricalColumns,
currentPage,
pageSize,
handlePagingChange,
]);

Copilot uses AI. Check for mistakes.

return (
<>
Expand Down
51 changes: 50 additions & 1 deletion ...adata-ui/src/main/resources/ui/src/components/Database/TableVersion/TableVersion.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,14 @@
*/

import { act, fireEvent, render, screen } from '@testing-library/react';
import { DataType } from '../../../generated/entity/data/table';
import { ENTITY_PERMISSIONS } from '../../../mocks/Permissions.mock';
import { tableVersionMockProps } from '../../../mocks/TableVersion.mock';
import {
mockTableData,
tableVersionMockProps,
} from '../../../mocks/TableVersion.mock';
import TableVersion from './TableVersion.component';
import VersionTable from '../../Entity/VersionTable/VersionTable.component';

const mockNavigate = jest.fn();
jest.mock(
Expand Down Expand Up @@ -138,6 +143,50 @@ describe('TableVersion tests', () => {
);
});

it('should pass historical columns from currentVersionData to VersionTable, not from a live API call', async () => {
const historicalVersionData = {
...mockTableData,
columns: [
{
name: 'order_amount',
dataType: DataType.Decimal,
dataTypeDisplay: 'decimal(9,1)',
precision: 9,
scale: 1,
fullyQualifiedName:
'sample_data.ecommerce_db.shopify.raw_product_catalog.order_amount',
tags: [],
ordinalPosition: 1,
},
],
changeDescription: {
fieldsAdded: [],
fieldsUpdated: [],
fieldsDeleted: [],
previousVersion: 0.1,
},
};

const mockedVersionTable = VersionTable as jest.MockedFunction<typeof VersionTable>;
mockedVersionTable.mockClear();

await act(async () => {
render(
<TableVersion
{...tableVersionMockProps}
currentVersionData={historicalVersionData}
/>
);
});

const calls = mockedVersionTable.mock.calls;
expect(calls.length).toBeGreaterThan(0);
const columnsPassedToVersionTable = calls[calls.length - 1][0].columns;

expect(columnsPassedToVersionTable).toHaveLength(1);
expect(columnsPassedToVersionTable[0].dataTypeDisplay).toBe('decimal(9,1)');
});

describe('ViewCustomFields Permission Tests', () => {
it('should render custom properties tab when ViewCustomFields is true', async () => {
await act(async () => {
Expand Down
Loading