mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-05 16:03:47 +00:00
fix(datatables): a role belongs to the data table it was chosen on
An app's default data table can be changed after it is created, and both editors that change it kept the role picked for the previous one — so its queries named a role that data table has never heard of. The rule is one function now, used by both, and the selector lists schemas as the role rather than as whatever the data table resolves to by default. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S5arH3G2Sa1Qqm32veJQ1n
This commit is contained in:
co-authored by
Claude Opus 5
parent
e4ce3a466a
commit
a274fd85bc
@@ -1 +1 @@
|
||||
4c6cbb234019db102c44a91ada36c4a197721aa4
|
||||
cc8e28e80bb352269c576ff82b8e679d7a505444
|
||||
|
||||
@@ -6,7 +6,10 @@
|
||||
const aiChatManager = getAiChatManager()
|
||||
import DefaultDatabaseSelector from '$lib/components/raw_apps/DefaultDatabaseSelector.svelte'
|
||||
import { workspaceStore } from '$lib/stores'
|
||||
import { createDatatablesResource } from '$lib/components/raw_apps/datatableUtils.svelte'
|
||||
import {
|
||||
createDatatablesResource,
|
||||
roleAfterDatatableChange
|
||||
} from '$lib/components/raw_apps/datatableUtils.svelte'
|
||||
|
||||
// Load available datatables from workspace using shared utility
|
||||
const datatables = createDatatablesResource(() => $workspaceStore)
|
||||
@@ -37,8 +40,10 @@
|
||||
}
|
||||
|
||||
function handleDefaultChange(datatable: string | undefined, schema: string | undefined) {
|
||||
aiChatManager.datatableCreationPolicy.datatable = datatable
|
||||
aiChatManager.datatableCreationPolicy.schema = schema
|
||||
const policy = aiChatManager.datatableCreationPolicy
|
||||
policy.role = roleAfterDatatableChange(policy.datatable, datatable, policy.role)
|
||||
policy.datatable = datatable
|
||||
policy.schema = schema
|
||||
}
|
||||
</script>
|
||||
|
||||
@@ -67,6 +72,7 @@
|
||||
<DefaultDatabaseSelector
|
||||
datatable={aiChatManager.datatableCreationPolicy.datatable}
|
||||
schema={aiChatManager.datatableCreationPolicy.schema}
|
||||
role={aiChatManager.datatableCreationPolicy.role}
|
||||
onChange={handleDefaultChange}
|
||||
description="Set the default datatable and schema for new tables. When table creation is enabled, AI can create tables here if needed."
|
||||
/>
|
||||
|
||||
@@ -4,8 +4,8 @@
|
||||
import Select from '$lib/components/select/Select.svelte'
|
||||
import { workspaceStore } from '$lib/stores'
|
||||
import {
|
||||
createDatatableAccessResource,
|
||||
createDatatablesResource,
|
||||
createSchemasResource,
|
||||
toDatatableItems,
|
||||
toSchemaItems
|
||||
} from './datatableUtils.svelte'
|
||||
@@ -20,6 +20,9 @@
|
||||
datatable: string | undefined
|
||||
/** Currently selected schema */
|
||||
schema: string | undefined
|
||||
/** The data table role the app's queries run as, if it names one. What a
|
||||
* role may see is what the schema list has to be read as. */
|
||||
role?: string | undefined
|
||||
/** Callback when either value changes */
|
||||
onChange?: (datatable: string | undefined, schema: string | undefined) => void
|
||||
/** Description text to show in the popover */
|
||||
@@ -29,19 +32,21 @@
|
||||
let {
|
||||
datatable,
|
||||
schema,
|
||||
role = undefined,
|
||||
onChange,
|
||||
description = 'Set the default datatable and schema for new tables. This is where AI will create new tables when needed.'
|
||||
}: Props = $props()
|
||||
|
||||
// Load available datatables and schemas using shared utilities
|
||||
const datatables = createDatatablesResource(() => opWs)
|
||||
const schemas = createSchemasResource(
|
||||
const access = createDatatableAccessResource(
|
||||
() => datatable,
|
||||
() => role,
|
||||
() => opWs
|
||||
)
|
||||
|
||||
const datatableItems = $derived(toDatatableItems(datatables.current))
|
||||
const schemaItems = $derived(toSchemaItems(schemas.current))
|
||||
const schemaItems = $derived(toSchemaItems(access.current.schemas))
|
||||
|
||||
// Track datatable changes to reset schema
|
||||
let previousDatatable = $state<string | undefined>(undefined)
|
||||
|
||||
@@ -16,6 +16,8 @@
|
||||
defaultDatatable?: string | undefined
|
||||
/** Default schema for new tables */
|
||||
defaultSchema?: string | undefined
|
||||
/** The data table role the app's queries run as, if it names one. */
|
||||
defaultRole?: string | undefined
|
||||
onAdd?: () => void
|
||||
onRemove?: (index: number) => void
|
||||
onSelect?: (ref: DataTableRef, index: number) => void
|
||||
@@ -31,6 +33,7 @@
|
||||
dataTableRefs = [],
|
||||
defaultDatatable = undefined,
|
||||
defaultSchema = undefined,
|
||||
defaultRole = undefined,
|
||||
onAdd,
|
||||
onRemove,
|
||||
onSelect,
|
||||
@@ -95,6 +98,7 @@
|
||||
<DefaultDatabaseSelector
|
||||
datatable={defaultDatatable}
|
||||
schema={defaultSchema}
|
||||
role={defaultRole}
|
||||
onChange={onDefaultChange}
|
||||
/>
|
||||
{/if}
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
<script lang="ts">
|
||||
import { roleAfterDatatableChange } from './datatableUtils.svelte'
|
||||
import { Pane, Splitpanes } from 'svelte-splitpanes'
|
||||
import { paneMinPercent } from '$lib/utils/splitpaneSizing'
|
||||
import RawAppInlineScriptsPanel from './RawAppInlineScriptsPanel.svelte'
|
||||
@@ -2364,14 +2365,18 @@
|
||||
}}
|
||||
defaultDatatable={data.datatable}
|
||||
defaultSchema={data.schema}
|
||||
defaultRole={data.role}
|
||||
onDefaultChange={(datatable, schema) => {
|
||||
const role = roleAfterDatatableChange(data.datatable, datatable, data.role)
|
||||
data.datatable = datatable
|
||||
data.schema = schema
|
||||
data.role = role
|
||||
// Also sync to aiChatManager
|
||||
aiChatManager.datatableCreationPolicy = {
|
||||
...aiChatManager.datatableCreationPolicy,
|
||||
datatable,
|
||||
schema
|
||||
schema,
|
||||
role
|
||||
}
|
||||
}}
|
||||
{runnables}
|
||||
|
||||
@@ -39,6 +39,7 @@
|
||||
defaultDatatable?: string | undefined
|
||||
/** Default schema for new tables */
|
||||
defaultSchema?: string | undefined
|
||||
defaultRole?: string | undefined
|
||||
onDefaultChange?: (datatable: string | undefined, schema: string | undefined) => void
|
||||
}
|
||||
|
||||
@@ -60,6 +61,7 @@
|
||||
onDataTableRefsChange,
|
||||
defaultDatatable = undefined,
|
||||
defaultSchema = undefined,
|
||||
defaultRole = undefined,
|
||||
onDefaultChange
|
||||
}: Props = $props()
|
||||
|
||||
@@ -153,6 +155,7 @@
|
||||
{dataTableRefs}
|
||||
{defaultDatatable}
|
||||
{defaultSchema}
|
||||
{defaultRole}
|
||||
onAdd={() => dataTableDrawer?.openDrawer()}
|
||||
onRemove={handleRemoveDataTable}
|
||||
onSelect={handleSelectDataTable}
|
||||
|
||||
@@ -1,10 +1,24 @@
|
||||
import { resource } from 'runed'
|
||||
import { workspaceStore, dbSchemas } from '$lib/stores'
|
||||
import { workspaceStore } from '$lib/stores'
|
||||
import { WorkspaceService } from '$lib/gen'
|
||||
import { getDbSchemas } from '$lib/components/apps/components/display/dbtable/metadata'
|
||||
import { ADMIN_DATATABLE_ROLE } from '$lib/components/dbTypes'
|
||||
import { get } from 'svelte/store'
|
||||
|
||||
/**
|
||||
* The role an app keeps when its default data table changes.
|
||||
*
|
||||
* A data table role is defined on one data table, so it does not follow the
|
||||
* default to another: kept, the app's queries would name a role that data table
|
||||
* has never heard of, and the one it names here may not be the one it gets.
|
||||
*/
|
||||
export function roleAfterDatatableChange(
|
||||
previous: string | undefined,
|
||||
next: string | undefined,
|
||||
role: string | undefined
|
||||
): string | undefined {
|
||||
return next === previous ? role : undefined
|
||||
}
|
||||
|
||||
/**
|
||||
* Creates a resource that loads available datatables from the workspace.
|
||||
* Pass a getter function that returns the workspace to create a reactive dependency.
|
||||
@@ -22,43 +36,6 @@ export function createDatatablesResource(getWorkspace: () => string | undefined)
|
||||
})
|
||||
}
|
||||
|
||||
/**
|
||||
* Creates a resource that loads schemas for a given datatable.
|
||||
* The getDatatable getter is used as a reactive dependency - when it changes, schemas are refetched.
|
||||
*/
|
||||
export function createSchemasResource(
|
||||
getDatatable: () => string | undefined,
|
||||
getWorkspace: () => string | undefined = () => get(workspaceStore)
|
||||
) {
|
||||
return resource<string[]>([() => getDatatable() ?? '', () => getWorkspace() ?? ''], async () => {
|
||||
const datatable = getDatatable()
|
||||
const workspace = getWorkspace()
|
||||
if (!datatable || !workspace) return []
|
||||
|
||||
const resourcePath = `datatable://${datatable}`
|
||||
// Key the schema cache by workspace too: a datatable of the same name can
|
||||
// exist in both the nav and the acting workspace, so `datatable://<name>`
|
||||
// alone would let one workspace's schema be reused for the other.
|
||||
const cacheKey = `${workspace}:${resourcePath}`
|
||||
const schemas = get(dbSchemas)
|
||||
let dbSchema = schemas[cacheKey]
|
||||
|
||||
if (!dbSchema) {
|
||||
try {
|
||||
schemas[cacheKey] = await getDbSchemas('postgresql', resourcePath, workspace, (msg) =>
|
||||
console.error('Schema error:', msg)
|
||||
)
|
||||
dbSchema = get(dbSchemas)[cacheKey]
|
||||
} catch (e) {
|
||||
console.error(`Failed to load schema for ${datatable}:`, e)
|
||||
return []
|
||||
}
|
||||
}
|
||||
|
||||
if (!dbSchema?.schema) return []
|
||||
return Object.keys(dbSchema.schema)
|
||||
})
|
||||
}
|
||||
|
||||
/**
|
||||
* Creates a resource that loads the roles the caller may use on a datatable,
|
||||
|
||||
@@ -0,0 +1,16 @@
|
||||
import { describe, expect, it } from 'vitest'
|
||||
import { roleAfterDatatableChange } from './datatableUtils.svelte'
|
||||
|
||||
describe('roleAfterDatatableChange', () => {
|
||||
it('drops a role that belongs to the data table being left', () => {
|
||||
// The role exists on `main` and says nothing about `second`, so an app
|
||||
// whose default moves must not keep naming it.
|
||||
expect(roleAfterDatatableChange('main', 'second', 'analyst')).toBe(undefined)
|
||||
expect(roleAfterDatatableChange('main', undefined, 'analyst')).toBe(undefined)
|
||||
})
|
||||
|
||||
it('keeps it when only the schema moved', () => {
|
||||
expect(roleAfterDatatableChange('main', 'main', 'analyst')).toBe('analyst')
|
||||
expect(roleAfterDatatableChange(undefined, undefined, undefined)).toBe(undefined)
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user