Conversation
…nt page, including new SQL methods and toast feedback.
e024d0f to
fc0fdce
Compare
…and primary key type support, and enhance table data editing in the UI.
… and UI component.
There was a problem hiding this comment.
Pull request overview
Adds row-edit/update capabilities to the Deployment database UI by introducing a client-side “Row Details” edit form with toast feedback and a new backend route + SQL helper to execute row updates.
Changes:
- Added a toast system and integrated it into
DeploymentPage. - Converted the Row Details drawer into an editable form that submits updates via a new API endpoint.
- Added
POST/api/deployment/table/updateandupdateTableData()to execute update statements after applying write transformers.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| web/pages/DeploymentPage.tsx | Adds toast UI, makes row details editable, and submits updates to the API. |
| api/sql.ts | Adds updateTableData() that applies write transformers and builds an UPDATE query. |
| api/routes.ts | Adds POST/api/deployment/table/update and refactors deployment table access checks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| data[key] = JSON.parse(val as string) | ||
| } catch { | ||
| data[key] = val | ||
| } |
There was a problem hiding this comment.
For JSON/Array/Map columns, this code parses the textarea value and puts an object/array into data. On the server, updateTableData serializes non-strings via String(v), which would turn objects into "[object Object]" (or otherwise corrupt/invalid SQL). Send a string representation (optionally validating it with JSON.parse) or ensure the server-side update path JSON-stringifies complex values before embedding them into SQL.
| try { | |
| data[key] = JSON.parse(val as string) | |
| } catch { | |
| data[key] = val | |
| } | |
| const raw = val as string | |
| try { | |
| // Validate JSON but always send a string representation | |
| JSON.parse(raw) | |
| } catch { | |
| // If not valid JSON, still send the raw string | |
| } | |
| data[key] = raw |
| const sets = Object.entries(transformedData).map(([k, v]) => { | ||
| const val = v === null | ||
| ? 'NULL' | ||
| : typeof v === 'string' | ||
| ? `'${v.replace(/'/g, "''")}'` | ||
| : String(v) | ||
| return `${k} = ${val}` | ||
| }) | ||
|
|
||
| const pkVal = typeof pk.value === 'string' | ||
| ? `'${String(pk.value).replace(/'/g, "''")}'` | ||
| : String(pk.value) |
There was a problem hiding this comment.
updateTableData serializes values with typeof v === 'string' ? ... : String(v). This will break for objects/arrays (becoming "[object Object]") and for Uint8Array values produced by write transformers (e.g. compressed blobs), leading to invalid SQL or data corruption. Add explicit handling (e.g. JSON.stringify for plain objects/arrays, and a defined encoding strategy for binary data) or reject unsupported types with a clear error before building the SQL string.
| const sets = Object.entries(transformedData).map(([k, v]) => { | |
| const val = v === null | |
| ? 'NULL' | |
| : typeof v === 'string' | |
| ? `'${v.replace(/'/g, "''")}'` | |
| : String(v) | |
| return `${k} = ${val}` | |
| }) | |
| const pkVal = typeof pk.value === 'string' | |
| ? `'${String(pk.value).replace(/'/g, "''")}'` | |
| : String(pk.value) | |
| const serializeSqlValue = (value: unknown): string => { | |
| if (value === null) { | |
| return 'NULL' | |
| } | |
| if (typeof value === 'string') { | |
| return `'${value.replace(/'/g, "''")}'` | |
| } | |
| if (typeof value === 'number' || typeof value === 'bigint') { | |
| return String(value) | |
| } | |
| if (typeof value === 'boolean') { | |
| return value ? 'TRUE' : 'FALSE' | |
| } | |
| if (value instanceof Date) { | |
| const iso = value.toISOString() | |
| return `'${iso.replace(/'/g, "''")}'` | |
| } | |
| if (value instanceof Uint8Array) { | |
| throw new Error( | |
| 'Unsupported value type for SQL serialization: Uint8Array. ' + | |
| 'Please encode binary data to a supported format (e.g. base64 string) before updating.', | |
| ) | |
| } | |
| if (Array.isArray(value) || (typeof value === 'object' && value !== null)) { | |
| const json = JSON.stringify(value) | |
| return `'${json.replace(/'/g, "''")}'` | |
| } | |
| throw new Error( | |
| `Unsupported value type for SQL serialization: ${typeof value}`, | |
| ) | |
| } | |
| const sets = Object.entries(transformedData).map(([k, v]) => { | |
| const val = serializeSqlValue(v) | |
| return `${k} = ${val}` | |
| }) | |
| const pkVal = serializeSqlValue(pk.value) |
| const query = `UPDATE ${table} SET ${ | ||
| sets.join(', ') | ||
| } WHERE ${pk.key} = ${pkVal}` |
There was a problem hiding this comment.
When data is empty, sets becomes empty and the query becomes UPDATE <table> SET WHERE ..., which is invalid SQL. Guard against empty updates (return early / throw a 400 with a helpful message) before executing the query.
| effect(() => { | ||
| const dep = url.params.dep | ||
| if (dep) { | ||
| schema.fetch({ url: dep }) | ||
| } | ||
| }) | ||
|
|
There was a problem hiding this comment.
This effect duplicates the schema-fetch effect a few lines below, so schema.fetch() will run twice on the same dep change (extra network calls / flicker). Remove one of the two effects and keep a single source of truth for schema loading.
| effect(() => { | |
| const dep = url.params.dep | |
| if (dep) { | |
| schema.fetch({ url: dep }) | |
| } | |
| }) |
| disabled={readOnly} | ||
| /> |
There was a problem hiding this comment.
FormData only includes checked checkboxes; if a boolean is currently true and the user unchecks it, the field won't be present in formData.entries() so the update payload won't include setting it to false. Consider adding a hidden input for unchecked state or, in onUpdateRow, explicitly read checked from the form elements for Bool columns.
| disabled={readOnly} | |
| /> | |
| disabled={readOnly} | |
| value='true' | |
| /> | |
| <input | |
| type='hidden' | |
| name={name} | |
| value='false' | |
| /> |
| for (const [key, val] of formData.entries()) { | ||
| if (row[key] === val) continue | ||
| const col = tableDef.columns.find((c) => c.name === key) | ||
| if (!col) continue | ||
| const type = col.type | ||
| if ( | ||
| type.includes('Int') || type.includes('Float') || | ||
| type.includes('Decimal') | ||
| ) { | ||
| data[key] = Number(val) | ||
| } else if (type.includes('Bool')) { | ||
| data[key] = val === 'on' | ||
| } else if ( | ||
| type.includes('JSON') || type.includes('Array') || type.includes('Map') | ||
| ) { | ||
| try { | ||
| data[key] = JSON.parse(val as string) | ||
| } catch { | ||
| data[key] = val | ||
| } | ||
| } else { | ||
| data[key] = val | ||
| } |
There was a problem hiding this comment.
The change-detection check if (row[key] === val) continue will almost never skip unchanged fields because FormData values are strings while row[key] is often a number/boolean/object. This will cause unnecessary updates and can also change types unexpectedly. Normalize val to the column type (or compare against a normalized version of row[key]) before deciding whether to include the field in data.
| for (const [key, val] of formData.entries()) { | |
| if (row[key] === val) continue | |
| const col = tableDef.columns.find((c) => c.name === key) | |
| if (!col) continue | |
| const type = col.type | |
| if ( | |
| type.includes('Int') || type.includes('Float') || | |
| type.includes('Decimal') | |
| ) { | |
| data[key] = Number(val) | |
| } else if (type.includes('Bool')) { | |
| data[key] = val === 'on' | |
| } else if ( | |
| type.includes('JSON') || type.includes('Array') || type.includes('Map') | |
| ) { | |
| try { | |
| data[key] = JSON.parse(val as string) | |
| } catch { | |
| data[key] = val | |
| } | |
| } else { | |
| data[key] = val | |
| } | |
| for (const [key, rawVal] of formData.entries()) { | |
| const col = tableDef.columns.find((c) => c.name === key) | |
| if (!col) continue | |
| const type = col.type | |
| let normalizedVal: unknown = rawVal | |
| if ( | |
| type.includes('Int') || type.includes('Float') || | |
| type.includes('Decimal') | |
| ) { | |
| normalizedVal = Number(rawVal) | |
| } else if (type.includes('Bool')) { | |
| normalizedVal = rawVal === 'on' | |
| } else if ( | |
| type.includes('JSON') || type.includes('Array') || type.includes('Map') | |
| ) { | |
| try { | |
| normalizedVal = JSON.parse(rawVal as string) | |
| } catch { | |
| normalizedVal = rawVal | |
| } | |
| } | |
| if (row[key] === normalizedVal) continue | |
| data[key] = normalizedVal |
| } | ||
|
|
||
| try { | ||
| await api['POST/api/deployment/table/update'].fetch({ | ||
| deployment: url.params.dep!, | ||
| table: tableName, | ||
| pk: { key: pk, value: row[pk] as unknown as string }, | ||
| data, | ||
| }) |
There was a problem hiding this comment.
If the user submits the form without any effective changes, data can be empty but the code still calls the update endpoint. The backend currently builds SET ${sets.join(', ')} and will produce invalid SQL when there are no fields to set. Add a guard like “no changes to update” (and optionally disable the button) before calling the update API.
| const drawerViews: Record<DrawerTab, JSX.Element> = { | ||
| history: <QueryHistory />, | ||
| insert: <ComingSoon title='Insert Row' />, | ||
| insert: <div></div>, |
There was a problem hiding this comment.
The insert drawer view was changed from a user-visible placeholder to an empty <div>, which makes the drawer appear broken/blank when opened. Either restore a placeholder message (or remove/disable the insert drawer entry) until insert is implemented.
| insert: <div></div>, | |
| insert: ( | |
| <div class='p-4'> | |
| <h3 class='text-lg font-semibold mb-4'>Insert not yet available</h3> | |
| <p> | |
| The insert view is not implemented yet. Please use another view from the sidebar. | |
| </p> | |
| </div> | |
| ), |
| 'POST/api/deployment/table/update': route({ | ||
| authorize: withUserSession, | ||
| fn: (ctx, { deployment, table, pk, data }) => { | ||
| const dep = withDeploymentTableAccess(ctx, deployment) |
There was a problem hiding this comment.
The update route forwards table, pk.key, and arbitrary data keys directly into SQL construction. Unlike POST/api/deployment/table/data, it does not validate the table/columns against the cached schema, which makes identifier injection possible and can update columns that don't exist. Fetch the cached schema here (like the data route does) and reject unknown table/columns before calling updateTableData.
| const dep = withDeploymentTableAccess(ctx, deployment) | |
| const dep = withDeploymentTableAccess(ctx, deployment) | |
| // Validate table and columns against cached schema to prevent identifier injection | |
| const schema = DatabaseSchemasCollection.get(deployment) | |
| if (!schema) { | |
| throw respond.NotFound({ message: 'Schema not cached yet' }) | |
| } | |
| const tableDef = schema.tables.find((t) => t.table === table) | |
| if (!tableDef) { | |
| throw respond.NotFound({ message: 'Table not found in schema' }) | |
| } | |
| const columnsMap = new Map(tableDef.columns.map((c) => [c.name, c])) | |
| // Ensure the primary key column exists in the schema | |
| if (!columnsMap.has(pk.key)) { | |
| throw respond.BadRequest({ | |
| message: `Unknown primary key column: ${pk.key}`, | |
| }) | |
| } | |
| // Ensure all updated columns exist in the schema | |
| for (const columnName of Object.keys(data || {})) { | |
| if (!columnsMap.has(columnName)) { | |
| throw respond.BadRequest({ | |
| message: `Unknown column in update data: ${columnName}`, | |
| }) | |
| } | |
| } |
| await api['POST/api/deployment/table/update'].fetch({ | ||
| deployment: url.params.dep!, | ||
| table: tableName, | ||
| pk: { key: pk, value: row[pk] as unknown as string }, |
There was a problem hiding this comment.
pk: { [pk]: row[pk] as unknown as string },| }) | ||
|
|
||
| const pkVal = typeof pk.value === 'string' | ||
| ? `'${String(pk.value).replace(/'/g, "''")}'` |
There was a problem hiding this comment.
${String(pk.value).replaceAll("'", "''")}
feat: Add functionality to edit and update table rows on the deployment page, including new SQL methods and toast feedback.