feat: data table integrations for raw apps #7436
Conversation
|
Claude finished @rubenfiszel's task in 3m 2s —— View job PR Review: Data Table Integrations for Raw AppsI've completed a comprehensive review of this pull request which adds data table integration functionality to raw apps in Windmill. Todo List
General ObservationsWhat's Good ✅
Areas of Concern
|
Deploying windmill with
|
| Latest commit: |
90778c7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://06511456.windmill.pages.dev |
| Branch Preview URL: | https://rf-datatablerawapps.windmill.pages.dev |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| * Pass a getter function that returns the workspace to create a reactive dependency. | ||
| */ | ||
| export function createDatatablesResource(getWorkspace: () => string | undefined) { | ||
| return resource.pre<string[]>([() => getWorkspace() ?? ''], async () => { |
There was a problem hiding this comment.
Potential Issue: Using resource.pre without proper error handling could leave users without feedback on persistent failures. Consider adding retry logic or user-facing error states.
Also, the empty dependency array [() => getWorkspace() ?? ''] in line 12 may not correctly trigger re-fetches when the workspace changes. The resource.pre API typically expects the callback to be called once initially, but the reactive dependency should be more explicit.
| return resource.pre<string[]>([() => getWorkspace() ?? ''], async () => { | |
| return resource<string[]>([() => getWorkspace() ?? ''], async () => { |
| let dbManagerContent: DBManagerContent | undefined = $state() | ||
| let hasReplResult = $state(false) | ||
| let isRefreshing = $state(false) | ||
| let windowWidth = $state(window.innerWidth) |
There was a problem hiding this comment.
Consistency Issue: Using $state(window.innerWidth) during SSR will fail since window is not defined. While this component is likely client-only, it's good practice to handle this:
| let windowWidth = $state(window.innerWidth) | |
| let windowWidth = $state(0) |
And then set the actual value after mount or via the svelte:window binding which already exists at line 150.
| dbManagerContent?.clearReplResult() | ||
| } | ||
| let windowWidth = $state(window.innerWidth) |
There was a problem hiding this comment.
Same SSR Issue: Accessing window.innerWidth during module initialization will fail during SSR.
| let windowWidth = $state(window.innerWidth) | |
| let windowWidth = $state(0) |
| fn: async ({ args, helpers, toolId, toolCallbacks }) => { | ||
| const parsedArgs = getExecDatatableSqlSchema().parse(args) | ||
|
|
||
| // Enforce datatable creation policy when new_table is specified |
There was a problem hiding this comment.
Security Consideration: The exec_datatable_sql tool allows executing arbitrary SQL queries. While there is policy enforcement for table creation, consider:
- Adding rate limiting to prevent abuse
- Logging SQL executions for audit purposes
- Consider restricting DDL operations (DROP TABLE, ALTER TABLE) to prevent accidental data loss
The current implementation only restricts table creation but doesn't prevent destructive operations like DROP TABLE.
| * Parse a string ref into a DataTableRef object | ||
| * Format: <datatableName>/<schema>:<table> or <datatableName>/<table> (for public schema) | ||
| */ | ||
| export function parseDataTableRef(ref: string): DataTableRef { |
There was a problem hiding this comment.
Good Practice: The string parsing logic is clean, but consider adding input validation. Malformed refs could cause unexpected behavior:
| export function parseDataTableRef(ref: string): DataTableRef { | |
| export function parseDataTableRef(ref: string): DataTableRef { | |
| if (!ref || typeof ref !== 'string') { | |
| throw new Error('Invalid data table reference: ref must be a non-empty string') | |
| } | |
| const slashIdx = ref.indexOf('/') |
| } | ||
| aiChatManager.instructions = initialPrompt.trim() | ||
| aiChatManager.sendRequest() | ||
| }, 500) |
There was a problem hiding this comment.
Magic Number: The 500ms delay for triggering AI is arbitrary. Consider:
- Using a proper event-based approach to know when the editor is initialized
- Making this configurable or at least defining it as a constant with documentation
const AI_TRIGGER_DELAY_MS = 500 // Delay to ensure editor is initialized
| aiChatManager.datatableCreationPolicy.enabled && | ||
| !aiChatManager.datatableCreationPolicy.datatable | ||
| ) { | ||
| aiChatManager.datatableCreationPolicy.datatable = datatables.current[0] |
There was a problem hiding this comment.
Potential Race Condition: The $effect that auto-selects the first datatable runs on every change to datatables.current.length. If datatables load asynchronously, this could overwrite a user's manual selection. Consider tracking whether the user has made a manual selection:
let userHasSelected = $state(false)
$effect(() => {
if (
datatables.current.length > 0 &&
aiChatManager.datatableCreationPolicy.enabled &&
!aiChatManager.datatableCreationPolicy.datatable &&
!userHasSelected
) {
aiChatManager.datatableCreationPolicy.datatable = datatables.current[0]
}
})
| hasReplResult = !!replResultData | ||
| }) | ||
| let cachedColDefs: Record<string, TableMetadata> = {} |
There was a problem hiding this comment.
Memory Consideration: The cachedColDefs object grows indefinitely during a session. Consider implementing a cache eviction strategy (LRU cache) or size limit to prevent memory issues in long-running sessions with many tables.
const MAX_CACHED_TABLES = 100
async function getColDefs(tableKey: string): Promise<TableMetadata> {
// ... cache refresh logic ...
// Evict oldest entries if cache is too large
const keys = Object.keys(cachedColDefs)
if (keys.length > MAX_CACHED_TABLES) {
delete cachedColDefs[keys[0]]
}
// ...
}
mov-2025-12-23--00-10-01.mp4