fix(components): map DataProfiler column icons by type - #888
MohamedIdhries wants to merge 3 commits into
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Hi @MohamedIdhries |
|
Hi @MohamedIdhries |
|
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! |
3d6531c to
01906d2
Compare
|
thanks for replay, ci is working now, waiting CI results |
|
Hi @cevheri, thanks for checking in. I’ve rebased the PR onto the latest |
|
can you run format on your local machine |
|
Hi @MohamedIdhries 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 The cause is not your mapping, it is the input. The route sets a The good news is the real type is already in the component, one lookup away. Step 1, read the type from the schema instead. 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 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 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 Step 5, the red CI check is only the formatter. Run 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 typecheckPlease use 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, Ask here if any step is unclear, happy to walk through it. |
…fix type matching (libredb#888)
|
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 Run these from the repo root: |
|
sory for that @MohamedIdhries , #880 closed by yusuf for some urgent reason |
Description
Fixes #880 by displaying column icons based on the column data type instead of rendering a static numeric icon for all columns.
Changes
getColumnIcon) inDataProfiler.tsx:INTEGER,FLOAT,DECIMAL,SERIAL, etc.) useHashVARCHAR,TEXT, etc.) useTypeTIMESTAMP,DATE,TIME) useCalendarBOOLEAN,BOOL) useToggleLeftFileTextrenders column icons according to col.typeinDataProfiler.test.tsxVerification
bun run typecheck— passed (0 errors)