Skip to content

fix(components): map DataProfiler column icons by type - #888

Closed
MohamedIdhries wants to merge 3 commits into
libredb:mainfrom
MohamedIdhries:fix/data-profiler-column-icons
Closed

MohamedIdhries wants to merge 3 commits into
libredb:mainfrom
MohamedIdhries:fix/data-profiler-column-icons

Conversation

@MohamedIdhries

Copy link
Copy Markdown

Description

Fixes #880 by displaying column icons based on the column data type instead of rendering a static numeric icon for all columns.

Changes

  • Added type-based icon mapping (getColumnIcon) in DataProfiler.tsx:
    • Numeric columns (INTEGER, FLOAT, DECIMAL, SERIAL, etc.) use Hash
    • Text/String columns (VARCHAR, TEXT, etc.) use Type
    • Date/time columns (TIMESTAMP, DATE, TIME) use Calendar
    • Boolean columns (BOOLEAN, BOOL) use ToggleLeft
    • Fallback types use FileText
  • Added regression test renders column icons according to col.type in DataProfiler.test.tsx

Verification

  • bun run typecheck — passed (0 errors)
  • Component unit tests — passed (32/32 pass)

@codecov

codecov Bot commented Sep 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.60000% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/components/DataProfiler.tsx 97.60% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!

@cevheri

cevheri commented Sep 16, 2026

Copy link
Copy Markdown
Member

Hi @MohamedIdhries
can you rebase and check CI action error again

@cevheri

cevheri commented Sep 17, 2026

Copy link
Copy Markdown
Member

Hi @MohamedIdhries
any update
if you are too busy to work on this PR, we can close it, you can pick up another task when you're available

@cevheri cevheri added the loop:needs-info Maintainer-loop task blocked on human-reviewed clarification label Sep 17, 2026
@MohamedIdhries

Copy link
Copy Markdown
Author

Hi @cevheri, thanks for checking in! I’m active and working on this. I am rebasing onto the latest main, addressing the CI failure, and will force-push the updated branch shortly. Thank you!

@MohamedIdhries
MohamedIdhries force-pushed the fix/data-profiler-column-icons branch from 3d6531c to 01906d2 Compare September 17, 2026 12:06
@MohamedIdhries

Copy link
Copy Markdown
Author

Hi @cevheri, thanks for checking in! I’ve rebased the PR onto the latest main and force-pushed the updated branch (01906d2). The PR is ready for CI checks and review. Thanks!

@cevheri

cevheri commented Sep 17, 2026

Copy link
Copy Markdown
Member

thanks for replay, ci is working now, waiting CI results

@MohamedIdhries

Copy link
Copy Markdown
Author

Hi @cevheri, thanks for checking in. I’ve rebased the PR onto the latest main and pushed the updated commit 01906d2c. I’m also checking the CI failure as requested. The PR is still active and I’ll keep working on it. Thanks!

@cevheri

cevheri commented Sep 17, 2026

Copy link
Copy Markdown
Member

can you run format on your local machine

@cevheri

cevheri commented Sep 17, 2026

Copy link
Copy Markdown
Member

Hi @MohamedIdhries
I prepared some review notes to help you, please follow them

Thanks for the rebase. The wiring is fine now, so this is about the fix itself. One thing needs to change in the approach, and I want to give you enough detail to do it in one pass.

What I measured: I rendered the profiler with the exact payload /api/db/profile returns and read the icon on each column row. On main every column is lucide-hash. On your branch every column is lucide-file-text. The icon changed, but it still does not vary, which is what the issue is about.

The cause is not your mapping, it is the input. The route sets a type on a column profile in exactly one place, src/app/api/db/profile/route.ts:76, and that line is in the MongoDB arm. The SQL arm builds its objects at route.ts:121-131 and never sets type. That is 14 of our 16 engines, so getColumnIcon(col.type) receives undefined every time and returns at your first line.

The good news is the real type is already in the component, one lookup away.

Step 1, read the type from the schema instead. tableSchema.columns is ColumnSchema[] and its type is a required string holding the engine's own name, integer, varchar(255), timestamp, boolean. Line 167 already reads that array and keeps only the names. Add a lookup next to the existing sensitiveColumnNames memo at line 87:

const columnTypes = useMemo(() => {
  const types = new Map<string, string>();
  for (const c of tableSchema?.columns ?? []) types.set(c.name, c.type);
  return types;
}, [tableSchema]);

Then at line 348, prefer the profile's own type and fall back to the schema:

const ColumnIcon = getColumnIcon(col.type ?? columnTypes.get(col.name));

Step 2, keep your FileText fallback at line 43, it is the right call: a type we cannot classify should not get the numeric glyph. What does need changing is the branch order, because real engine type names now reach the function. "interval".includes("int") and "point".includes("int") are both true and your numeric test is first, so a Postgres interval and a geometric point both render as numbers. Put an interval test above the numeric branch. Your map also has no branch for money, bit, enum, json/xml or uuid; those land on FileText, which is not wrong, just narrower than it could be. I have put the full type list and the measurement behind it in a note on #880, please read that before you start.

Step 3, make the test able to fail. Right now it passes against main's unmodified component, so it does not yet pin your fix. The default tableSchema in that file is mockUsersTable, which already has id integer, name varchar(255), created_at timestamp and is_active boolean, so you do not need a new schema fixture. What you do need is a profile response with no type key on the columns, because that is what a SQL engine really returns. Then assert the icon per column rather than counting svgs:

const iconClass = (name: string) =>
  within(container).getByText(name).parentElement?.querySelector("svg")?.getAttribute("class") ?? "";

expect(iconClass("id")).toContain("lucide-hash");
expect(iconClass("name")).toContain("lucide-type");
expect(iconClass("created_at")).toContain("lucide-calendar");
expect(iconClass("is_active")).toContain("lucide-toggle-left");

Step 4, prove the test works before you push. Temporarily make getColumnIcon return Hash for everything, run the file, and confirm the new case goes red. Put your code back and confirm it goes green. If it stays green both ways the test is not measuring anything, and that is the check I run on every PR here.

Step 5, the red CI check is only the formatter. Run bun run format:fix and commit the result.

Commands, run from the repo root:

bun run format:fix
bun tests/run-tests.ts tests/components/DataProfiler.test.tsx
bun run lint && bun run typecheck

Please use bun tests/run-tests.ts <file> rather than bun test, the runner gives each file its own process and a bare bun test over a directory leaks module mocks between files.

I tried these five steps on your branch before writing this, so I can tell you they hold together: with the lookup in place the file is 32 pass, typecheck and lint are clean, and collapsing getColumnIcon to a single return Hash turns the new case red with Expected to contain: "lucide-type", Received: "lucide lucide-hash". That red is the proof the test is worth having.

Ask here if any step is unclear, happy to walk through it.

@cevheri

cevheri commented Sep 17, 2026

Copy link
Copy Markdown
Member

Thanks @MohamedIdhries , steps 1 to 3 landed as written. I checked out your branch and measured: the file is 32 pass, lint and typecheck are clean, format is a no-op, and collapsing getColumnIcon to return Hash turns your new case red, so the test really pins the fix.

What is red is not the tests, it is the coverage gate. This repo holds 100 percent line coverage and the two branches you added beyond the note are untested: DataProfiler.tsx:67-68 (the spatial branch) and :109 (the json/xml/blob branch). Line 109 is dead either way, it returns the same FileText as the fallback on line 111, so deleting it changes nothing. Do the same for the spatial branch, or keep it and add one case for a point column. Either way the gate goes green.

One more thing, please revert it: line 414 now prints resolvedType instead of col.type, so SQL columns show a type
label they never showed before. That is outside #880 and it has no test.

Run these from the repo root:

  bun run format:fix
  bun tests/run-tests.ts tests/components/DataProfiler.test.tsx
  bun run lint && bun run typecheck
  bun run test:coverage && bun run coverage:check

@cevheri

cevheri commented Sep 18, 2026

Copy link
Copy Markdown
Member

sory for that @MohamedIdhries , #880 closed by yusuf for some urgent reason

@cevheri cevheri closed this Sep 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

duplicate This issue or pull request already exists loop:needs-info Maintainer-loop task blocked on human-reviewed clarification

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Data Profiler shows the numeric icon on every column regardless of type

2 participants