Skip to content

Commit 58e0ccd

Browse files
cevhericlaude
andauthored
fix: correct schema-explorer select-limit label + make admin E2E seed-state-independent (#73)
* fix(sql): quote schema-qualified table names per-segment The dialect-aware quoteIdentifier treated a schema-qualified table name as a single identifier, so 'employees.department' became '"employees.department"' — which Postgres reads as one relation literally named with a dot, failing with a query error. Sidebar 'select top 100' on any schema-qualified table was broken. Add quoteQualifiedName(): split on '.', quote each segment only-when-needed, and rejoin. 'employees.department' stays unquoted; 'public.Order' becomes 'public."Order"'. Use it for table names in the query generators and the data profiler (columns still use single-identifier quoting). Verified end-to-end against a Neon Postgres database: the generated 'SELECT * FROM employees.department LIMIT 50;' runs, whereas the old quoted form failed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(package): bump version to 0.9.23 * chore(package): bump version to 0.9.24 * fix(db/postgres): prevent schema introspection timeout on large schemas getSchema() ran a single query that joins five information_schema-based CTEs (columns, PKs, foreign keys, indexes + table sizes). PostgreSQL 12+ inlines single-reference CTEs, and because the planner estimates rows=1 for information_schema views it chose nested-loop joins that re-execute the expensive fk/index CTEs many times. On schemas with 100+ tables this exploded to ~295s and always hit the connection's 60s statement_timeout, so the schema tree never loaded. Two complementary fixes: 1. Mark all CTEs AS MATERIALIZED so each is computed once instead of being re-executed inside nested loops (~295s -> ~2.6s on a 122-table schema). 2. Split schema loading into two phases so a slow/failing stats query can never block the table list: - getSchemaList() -> tables + columns + PKs (fast, ~50ms) [/api/db/schema/list] - getSchemaRelations() -> foreign keys + indexes (heavy, ~2.3s) [/api/db/schema/relations] The client renders the tree from the fast list immediately, then merges relations asynchronously. If relations fail/time out they are logged and skipped — the table list stays intact. getSchema() is kept (MATERIALIZED) for consumers that need the full schema (diff, AI context). New optional provider methods getSchemaList/getSchemaRelations fall back to getSchema()/[] for providers that don't implement them. * test(db/schema): cover two-phase schema introspection (list + relations) Adds regression coverage for the schema-introspection timeout fix (0d60c47), which split schema loading into a fast structural list and a heavy, best-effort relations phase. None of the new code paths were previously tested. Provider (tests/integration/db/postgres-provider.test.ts, +11): - getSchemaList(): returns columns + PKs but intentionally empty indexes/foreignKeys, isPrimary detection, non-public schema prefixing, negative reltuples row_count clamped to 0, and null/empty columns. - getSchemaRelations(): FK/index data keyed by display name, non-public prefixing on both the table and the referenced table, public references kept bare, empty/null fk-index tolerance, and null index columns coerced to []. API routes (new files, 15 tests): - /api/db/schema/list: uses getSchemaList() when present, falls back to getSchema() otherwise, plus auth (401), empty-body (400), missing-type (400), ConnectionError (503) and DatabaseError/generic (500). - /api/db/schema/relations: uses getSchemaRelations() when present, returns [] fallback otherwise, plus the same auth/validation/error matrix. Hook (tests/hooks/use-connection-manager.test.ts, +4): - phase 1 renders the table list, phase 2 merges FKs/indexes by table name; - relations failure does NOT wipe the table list and shows no error toast (the core resilience contract of the fix); - phase 1 failure short-circuits so the heavy relations endpoint is never hit; - tables absent from the relations payload are left unchanged. Full suite green: lint (0 errors), typecheck, 2050 unit/api/integration + 251 hooks + component groups, 0 failures. * fix(db/postgres): hoist schema SQL to module scope + dedupe CTEs for Sonar gate The SonarCloud PR quality gate failed on #71 with 53.3% coverage and 6.6% duplication on new code. Both stem from how the schema introspection SQL was written, not from missing tests. Coverage: the getSchema/getSchemaList/getSchemaRelations SQL lived in multi-line template literals *inside the method bodies*. bun's coverage instruments the interior lines of such literals as 0-hit in any test process that imports the module without exercising the method; the merged lcov (per-file isolated runs) then reports those SQL lines as uncovered even though the methods are tested. Verified empirically: a module-level `const` template is evaluated once at load and reported covered everywhere, while an in-body one is not. Hoisting the three queries to module scope takes postgres.ts new-code line coverage 28% -> 100% (overall new-code coverage 53.9% -> ~99%). Duplication: the hoisted queries shared the same CTEs (tables/columns/pk across full+list, fk/index across full+relations). Extracted each CTE to a single reusable fragment const and compose the three queries from them, removing the duplicated blocks. The SQL text produced is unchanged. Tests: the two route specs intentionally keep their mock.module setup inline (a shared helper breaks bun's per-file mock scoping under the non-isolated `bun run test`, clobbering @/lib/db mocks across files). Added an empty-object ({}) body case to each for the last uncovered branch. Test mock/setup boilerplate is expected harness scaffolding, so tests are excluded from Sonar copy-paste detection (sonar.cpd.exclusions). No behavioural change: same SQL, same provider/route/hook logic. Verified: lint (0 errors), typecheck, 2052 unit/api/integration + hooks + component groups (0 failures), production build, full coverage regenerated. * chore(package): bump version to 0.9.25 * refactor(api/schema): extract shared handler to remove route duplication The Sonar PR gate still flagged 6.2% duplication on new code: the /api/db/schema/list and /api/db/schema/relations routes shared ~39 identical lines of body parsing, auth, connection resolution and error handling (the only difference is which provider method they call). Extract that boilerplate into handleSchemaRequest() in src/lib/api/schema-route.ts; each route is now a thin wrapper that passes a `load` callback selecting the provider method (with the getSchemaList->getSchema and getSchemaRelations->[] fallbacks preserved). Removes both 39-line duplicated blocks. Coverage holds at 100% on new code (the shared handler is fully exercised by the existing route specs). Verified: typecheck, 2052 core tests + 0 failures, coverage regenerated (schema-route.ts 32/32 lines). * fix(db/postgres): join constraint_column_usage on constraint_schema for cross-schema FKs Copilot review on #71 flagged a pre-existing bug in the FK introspection (from 0d60c47, relocated verbatim into CTE_FK_INFO during the dedupe): JOIN information_schema.constraint_column_usage ccu ON ccu.constraint_name = tc.constraint_name AND ccu.table_schema = tc.table_schema -- wrong For a foreign key, constraint_column_usage reports the *referenced* (parent) table's columns, so ccu.table_schema is the referenced schema while tc.table_schema is the referencing schema. Joining them requires the two to be equal, which silently drops every cross-schema foreign key and makes referencedSchema always equal the referencing schema — defeating the non-public referenced-table prefixing (e.g. billing.accounts). Join on the constraint's own schema instead (ccu.constraint_schema = tc.constraint_schema). Because the FK CTE is now single-sourced, this fixes both getSchema() and getSchemaRelations(). Added a SQL-level regression guard: the existing specs mock the query result, so they can't catch a bad join; the new test asserts the emitted SQL joins on constraint_schema and not table_schema. Real cross-schema FK behaviour should be confirmed against a live Postgres via E2E. * fix(ci): restore missing test:ci script used by the npm publish gate The npm-publish workflow's Validate job runs `bun run test:ci`, but the script was dropped from package.json somewhere after v0.9.19 (it was added in #66). The v0.9.25 tag push therefore failed with "Script not found test:ci", blocking the npm publish (the GitHub release, tag and Docker images published fine). Restore it to the per-file-isolated form the publish gate expects: test:ci = bash tests/run-core.sh && bun run test:components This mirrors ci.yml's reliable path and avoids bun's process-wide mock.module load-order flakiness that the single-process `bun run test` is prone to. * fix(schema-explorer): update select action label from 'Select Top 100' to 'Select Top 50' * bump: upgrade version to 0.9.26 * fix(e2e): make admin-dashboard tests seed-state-independent The 'default tab is overview' and 'can switch to operations tab' E2E tests asserted on empty-state-only copy ('Command Center', the 'Select connection' placeholder), which disappears once seed connections load the populated dashboard. They passed in CI (no seed file) but failed locally with SEED_CONFIG_PATH set. Add stable data-testid hooks (admin-content-overview/-operations) to the TabsContent wrappers and assert on testid visibility + tab aria-selected, so the tests hold in both empty and populated states. Verified: 32/32 full E2E pass with seeds loaded, 8/8 admin pass with seeds disabled. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 96eab52 commit 58e0ccd

7 files changed

Lines changed: 21 additions & 17 deletions

File tree

e2e/admin-dashboard.spec.ts

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,15 +23,18 @@ test.describe('Admin Dashboard', () => {
2323
});
2424

2525
test('default tab is overview', async ({ page }) => {
26-
// Overview tab content should be visible by default
27-
await expect(page.locator('text=Command Center').first()).toBeVisible({ timeout: 10000 });
26+
// Overview content is mounted by default — assert on the content region, not
27+
// empty-state copy, so the test holds whether or not seed connections exist.
28+
await expect(page.getByTestId('admin-content-overview')).toBeVisible({ timeout: 10000 });
29+
await expect(page.getByRole('tab', { name: /Overview/i })).toHaveAttribute('aria-selected', 'true');
2830
});
2931

3032
test('can switch to operations tab', async ({ page }) => {
3133
await page.locator('button:has-text("Operations"), [role="tab"]:has-text("Operations")').first().click();
32-
await page.waitForTimeout(500);
33-
// Operations tab content
34-
await expect(page.locator('text=Connection').first()).toBeVisible({ timeout: 5000 });
34+
// Operations content region mounts regardless of connection state (empty
35+
// state or populated dashboard), so this is stable across environments.
36+
await expect(page.getByTestId('admin-content-operations')).toBeVisible({ timeout: 5000 });
37+
await expect(page.getByRole('tab', { name: /Operations/i })).toHaveAttribute('aria-selected', 'true');
3538
});
3639

3740
test('can switch to security tab', async ({ page }) => {

package.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "@libredb/studio",
3-
"version": "0.9.25",
3+
"version": "0.9.26",
44
"private": false,
55
"publishConfig": {
66
"access": "public"
@@ -84,6 +84,7 @@
8484
"lint": "eslint .",
8585
"typecheck": "tsc --noEmit",
8686
"test": "bun test tests/unit tests/api tests/integration && bun test tests/hooks && bun run test:components",
87+
"test:ci": "bash tests/run-core.sh && bun run test:components",
8788
"test:unit": "bun test tests/unit",
8889
"test:integration": "bun test tests/integration",
8990
"test:hooks": "bun test tests/hooks",

src/components/admin/AdminDashboard.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -134,12 +134,12 @@ export default function AdminDashboard() {
134134

135135
<div className="flex-1 overflow-auto">
136136
<TabsContent value="overview" className="m-0 p-0">
137-
<div className="mx-auto max-w-7xl px-4 sm:px-6 py-6">
137+
<div data-testid="admin-content-overview" className="mx-auto max-w-7xl px-4 sm:px-6 py-6">
138138
<OverviewTab user={user} />
139139
</div>
140140
</TabsContent>
141141
<TabsContent value="operations" className="m-0 p-0">
142-
<div className="mx-auto max-w-7xl px-4 sm:px-6 py-6">
142+
<div data-testid="admin-content-operations" className="mx-auto max-w-7xl px-4 sm:px-6 py-6">
143143
<OperationsTab />
144144
</div>
145145
</TabsContent>

src/components/schema-explorer/TableItem.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ function renderMenuItems(
6868
<>
6969
<Item onClick={() => callbacks.onTableClick?.(table.name)}>
7070
<Play strokeWidth={1.5} className="w-3.5 h-3.5 mr-2 text-green-500" />
71-
{labels?.selectAction || 'Select Top 100'}
71+
{labels?.selectAction || 'Select Top 50'}
7272
</Item>
7373
<Item onClick={() => callbacks.onGenerateSelect?.(table.name)}>
7474
<Filter strokeWidth={1.5} className="w-3.5 h-3.5 mr-2 text-blue-500" />

src/lib/db/base-provider.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@ export abstract class BaseDatabaseProvider implements DatabaseProvider {
193193
entityNamePlural: 'Tables',
194194
rowName: 'row',
195195
rowNamePlural: 'rows',
196-
selectAction: 'Select Top 100',
196+
selectAction: 'Select Top 50',
197197
generateAction: 'Generate Query',
198198
analyzeAction: 'Analyze Table',
199199
vacuumAction: 'Vacuum Table',

tests/components/schema-explorer/TableItem.test.tsx

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -176,13 +176,13 @@ describe('TableItem', () => {
176176

177177
// ── Dropdown action callbacks ─────────────────────────────────────────────
178178

179-
test('onTableClick fires with table name on "Select Top 100" click', () => {
179+
test('onTableClick fires with table name on "Select Top 50" click', () => {
180180
const onTableClick = mock((name: string) => { void name; });
181181
const { getByTestId } = render(
182182
<TableItem table={largeTable} isExpanded={false} onToggle={mock(() => {})} isAdmin={false} onTableClick={onTableClick} />
183183
);
184184
const dropdown = within(getByTestId('dropdown'));
185-
fireEvent.click(dropdown.getByText('Select Top 100'));
185+
fireEvent.click(dropdown.getByText('Select Top 50'));
186186
expect(onTableClick).toHaveBeenCalledTimes(1);
187187
expect(onTableClick.mock.calls[0][0]).toBe('users');
188188
});
@@ -316,7 +316,7 @@ describe('TableItem', () => {
316316
expect(dropdown.queryByText('Run Stats')).not.toBeNull();
317317
expect(dropdown.queryByText('Compact')).not.toBeNull();
318318
// Default labels should not appear
319-
expect(dropdown.queryByText('Select Top 100')).toBeNull();
319+
expect(dropdown.queryByText('Select Top 50')).toBeNull();
320320
expect(dropdown.queryByText('Generate Query')).toBeNull();
321321
});
322322

@@ -354,7 +354,7 @@ describe('TableItem', () => {
354354
);
355355
const dropdown = within(getByTestId('dropdown'));
356356
// Click all menu items without providing callbacks - should not throw
357-
fireEvent.click(dropdown.getByText('Select Top 100'));
357+
fireEvent.click(dropdown.getByText('Select Top 50'));
358358
fireEvent.click(dropdown.getByText('Generate Query'));
359359
fireEvent.click(dropdown.getByText('Profile Table'));
360360
fireEvent.click(dropdown.getByText('Generate Code'));
@@ -392,7 +392,7 @@ describe('TableItem', () => {
392392
<TableItem table={largeTable} isExpanded={false} onToggle={mock(() => {})} isAdmin />
393393
);
394394
// Each action should appear twice: once in dropdown, once in context menu
395-
expect(queryAllByText('Select Top 100').length).toBe(2);
395+
expect(queryAllByText('Select Top 50').length).toBe(2);
396396
expect(queryAllByText('Generate Query').length).toBe(2);
397397
expect(queryAllByText('Copy Name').length).toBe(2);
398398
expect(queryAllByText('Profile Table').length).toBe(2);
@@ -407,7 +407,7 @@ describe('TableItem', () => {
407407
<TableItem table={largeTable} isExpanded={false} onToggle={mock(() => {})} isAdmin={false} />
408408
);
409409
// Standard actions appear twice (dropdown + context menu)
410-
expect(queryAllByText('Select Top 100').length).toBe(2);
410+
expect(queryAllByText('Select Top 50').length).toBe(2);
411411
// Admin-only actions should not appear at all
412412
expect(queryAllByText('Analyze Table').length).toBe(0);
413413
expect(queryAllByText('Vacuum Table').length).toBe(0);

tests/unit/db/base-provider.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -233,7 +233,7 @@ describe('BaseDatabaseProvider', () => {
233233
expect(labels.entityNamePlural).toBe('Tables');
234234
expect(labels.rowName).toBe('row');
235235
expect(labels.rowNamePlural).toBe('rows');
236-
expect(labels.selectAction).toBe('Select Top 100');
236+
expect(labels.selectAction).toBe('Select Top 50');
237237
expect(labels.searchPlaceholder).toBe('Search tables or columns...');
238238
});
239239
});

0 commit comments

Comments
 (0)