mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-21 00:02:30 +00:00
fix(db-manager): address review findings on the schema diagram
- The auto-select effect read the state it wrote, so unchecking the last table refilled the schema and an empty schema looped it until the pane died. It now runs once per entry into the mode. - Returning to the diagram re-queued the foreign-key query every time; it is now re-read only when the schema itself was reloaded. - A switch to a non-PostgreSQL database left the diagram on screen with no toggle to leave it by. - A checkbox click in diagram mode also queued the per-table foreign-key job of the data view, whose result the diagram never reads. - Update the registered-counter tally in docs/feature-telemetry.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MXECAKye6KgN13wznsuRzP
This commit is contained in:
co-authored by
Claude Opus 5
parent
d2842e0751
commit
520bfb7abb
@@ -4,10 +4,10 @@
|
||||
anonymous usage-stats payload. It answers "does anyone use this, and which variant do they pick"
|
||||
without any identifying data leaving the instance.
|
||||
|
||||
It currently carries 42 registered actions across seventeen features (`ai_session`, `ai_chat`,
|
||||
`ai_fix`, `ai_agent`, `ai_agent_eval`, `app_sandbox`, `datatable`, `flow_editor`, `flow_run`,
|
||||
`flow_step`, `run_form`, `debugger`, `trigger`, `command_script`, `hub_script`, `usage_meter`,
|
||||
`sso_groups_claim`). Nearly all of the
|
||||
It currently carries 43 registered actions across eighteen features (`ai_session`, `ai_chat`,
|
||||
`ai_fix`, `ai_agent`, `ai_agent_eval`, `app_sandbox`, `datatable`, `db_manager`, `flow_editor`,
|
||||
`flow_run`, `flow_step`, `run_form`, `debugger`, `trigger`, `command_script`, `hub_script`,
|
||||
`usage_meter`, `sso_groups_claim`). Nearly all of the
|
||||
product is uninstrumented, so new user-facing work is the opportunity to change that.
|
||||
|
||||
## When to instrument
|
||||
|
||||
@@ -100,13 +100,18 @@
|
||||
onImport
|
||||
}: Props = $props()
|
||||
|
||||
let viewMode = $state<DbManagerViewMode>('data')
|
||||
let requestedViewMode = $state<DbManagerViewMode>('data')
|
||||
|
||||
// PostgreSQL is the only database whose foreign keys can be read for the whole
|
||||
// database in one query; the others would need one job per table. A caller
|
||||
// already using the sidebar checkboxes to collect tables keeps them.
|
||||
let supportsDiagram = $derived(dbType === 'postgresql' && !multiSelectMode)
|
||||
|
||||
// The manager is not remounted when the drawer switches database, so a switch
|
||||
// away from PostgreSQL would otherwise leave the diagram on screen with no
|
||||
// toggle left to leave it by.
|
||||
let viewMode = $derived(supportsDiagram ? requestedViewMode : 'data')
|
||||
|
||||
// The tables drawn on the diagram. Kept apart from `selectedTables` so that
|
||||
// checking a table to see it in the diagram can never add it to whatever the
|
||||
// caller's multi-select is collecting.
|
||||
@@ -266,6 +271,13 @@
|
||||
selected = { schemaKey, tableKey: table }
|
||||
}
|
||||
|
||||
/** Selecting a table fetches its foreign keys for the data view. The diagram
|
||||
* shows no preview and reads every relation from one query of its own, so a
|
||||
* checkbox there must not queue that per-table job. */
|
||||
function previewTableFromSidebar(schemaKey: string, table: string) {
|
||||
if (viewMode !== 'diagram') selectTable(schemaKey, table)
|
||||
}
|
||||
|
||||
/** Where a foreign key's `schema.table` target lives in the sidebar, or
|
||||
* undefined when it cannot be opened from here. */
|
||||
function resolveForeignKeyTarget(
|
||||
@@ -369,15 +381,19 @@
|
||||
// Fetched once for the whole database rather than per table: the diagram needs
|
||||
// every relation at once, and the per-table query would be one job each.
|
||||
let relationsError = $state<string | undefined>(undefined)
|
||||
// The keys are only re-read when the schema itself was reloaded, which is what
|
||||
// a new `colDefs` identity means. Toggling back to the diagram must not queue
|
||||
// the query again.
|
||||
let relationsFetchedFor: Record<string, ColumnDef[]> | undefined
|
||||
let relations = resource(
|
||||
[() => viewMode, () => colDefs],
|
||||
async ([mode]): Promise<DbRelation[]> => {
|
||||
// Keeps what was fetched when leaving the diagram, so coming back to it
|
||||
// doesn't queue the query again.
|
||||
if (mode !== 'diagram') return relations.current ?? []
|
||||
async ([mode, defs], _prev, { data }): Promise<DbRelation[]> => {
|
||||
if (mode !== 'diagram' || (data && relationsFetchedFor === defs)) return data ?? []
|
||||
relationsError = undefined
|
||||
try {
|
||||
return await dbSchemaOps.onFetchAllForeignKeys()
|
||||
const fetched = await dbSchemaOps.onFetchAllForeignKeys()
|
||||
relationsFetchedFor = defs
|
||||
return fetched
|
||||
} catch (e) {
|
||||
relationsError = (e as any)?.body ?? (e as Error)?.message ?? String(e)
|
||||
return []
|
||||
@@ -387,15 +403,24 @@
|
||||
|
||||
// Opening the diagram on an empty canvas would make it look broken, so the
|
||||
// current schema is drawn to start with — unless it is big enough that drawing
|
||||
// all of it is a choice the user should make.
|
||||
// all of it is a choice the user should make. Once per entry into the mode:
|
||||
// re-running it would refill a selection the user has just emptied.
|
||||
const DIAGRAM_AUTOSELECT_LIMIT = 40
|
||||
let autoSelected = false
|
||||
$effect(() => {
|
||||
if (viewMode !== 'diagram' || diagramTables.length) return
|
||||
const schemaKey = untrack(() => selected.schemaKey)
|
||||
if (!schemaKey) return
|
||||
const tables = Object.keys(dbSchema.schema[schemaKey] ?? {})
|
||||
if (tables.length > DIAGRAM_AUTOSELECT_LIMIT) return
|
||||
diagramTables = tables.map((table) => ({ schema: schemaKey, table }))
|
||||
if (viewMode !== 'diagram') {
|
||||
autoSelected = false
|
||||
return
|
||||
}
|
||||
if (autoSelected) return
|
||||
autoSelected = true
|
||||
untrack(() => {
|
||||
const schemaKey = selected.schemaKey
|
||||
if (!schemaKey || diagramTables.length) return
|
||||
const tables = Object.keys(dbSchema.schema[schemaKey] ?? {})
|
||||
if (!tables.length || tables.length > DIAGRAM_AUTOSELECT_LIMIT) return
|
||||
diagramTables = tables.map((table) => ({ schema: schemaKey, table }))
|
||||
})
|
||||
})
|
||||
|
||||
let _dbTable: DBTable | undefined = $state()
|
||||
@@ -410,7 +435,7 @@
|
||||
{/if}
|
||||
{#if supportsDiagram}
|
||||
<ToggleButtonGroup
|
||||
bind:selected={viewMode}
|
||||
bind:selected={requestedViewMode}
|
||||
noWFull
|
||||
onSelected={(v) => logFeatureUsage('db_manager', 'view_mode', { key: v })}
|
||||
>
|
||||
@@ -553,12 +578,12 @@
|
||||
role="button"
|
||||
tabindex="0"
|
||||
onclick={() => {
|
||||
selectTable(schemaKey, tableKey)
|
||||
previewTableFromSidebar(schemaKey, tableKey)
|
||||
toggleTableSelection(schemaKey, tableKey)
|
||||
}}
|
||||
onkeydown={(e) => {
|
||||
if (e.key === 'Enter' || e.key === ' ') {
|
||||
selectTable(schemaKey, tableKey)
|
||||
previewTableFromSidebar(schemaKey, tableKey)
|
||||
toggleTableSelection(schemaKey, tableKey)
|
||||
}
|
||||
}}
|
||||
@@ -705,7 +730,7 @@
|
||||
loading={relations.loading}
|
||||
error={relationsError}
|
||||
onOpenTable={({ schema, table }) => {
|
||||
viewMode = 'data'
|
||||
requestedViewMode = 'data'
|
||||
selectTable(schema, table)
|
||||
}}
|
||||
/>
|
||||
|
||||
@@ -0,0 +1,45 @@
|
||||
import { describe, expect, it } from 'vitest'
|
||||
import { CARD_WIDTH, cardHeight, layoutTables, type DiagramTable } from './dbDiagramModel'
|
||||
|
||||
function table(name: string, columnCount: number): DiagramTable {
|
||||
return {
|
||||
key: `shop.${name}`,
|
||||
schema: 'shop',
|
||||
table: name,
|
||||
columns: Array.from({ length: columnCount }, (_, i) => ({
|
||||
name: `c${i}`,
|
||||
datatype: 'text',
|
||||
isPrimaryKey: i === 0
|
||||
}))
|
||||
}
|
||||
}
|
||||
|
||||
describe('layoutTables', () => {
|
||||
// Runs again on every checkbox click, so cards must not land on top of each
|
||||
// other for any mix of card heights.
|
||||
it('never overlaps two cards', () => {
|
||||
const tables = Array.from({ length: 40 }, (_, i) => table(`t${i}`, 1 + (i % 17)))
|
||||
const expanded = new Set(['shop.t3', 'shop.t20'])
|
||||
const positions = layoutTables(tables, expanded)
|
||||
|
||||
const boxes = tables.map((t) => ({
|
||||
x: positions[t.key].x,
|
||||
y: positions[t.key].y,
|
||||
h: cardHeight(t, expanded.has(t.key))
|
||||
}))
|
||||
for (let i = 0; i < boxes.length; i++) {
|
||||
for (let j = i + 1; j < boxes.length; j++) {
|
||||
const a = boxes[i]
|
||||
const b = boxes[j]
|
||||
const apart =
|
||||
a.x + CARD_WIDTH <= b.x || b.x + CARD_WIDTH <= a.x || a.y + a.h <= b.y || b.y + b.h <= a.y
|
||||
expect(apart, `${tables[i].key} overlaps ${tables[j].key}`).toBe(true)
|
||||
}
|
||||
}
|
||||
})
|
||||
|
||||
it('places the same tables in the same spots every time', () => {
|
||||
const tables = Array.from({ length: 12 }, (_, i) => table(`t${i}`, 3 + i))
|
||||
expect(layoutTables(tables, new Set())).toEqual(layoutTables(tables, new Set()))
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user