Add inspector SQLite shell and property bindings#4410
Add inspector SQLite shell and property bindings#4410NathanFlurry wants to merge 7 commits intomainfrom
Conversation
|
🚅 Deployed to the rivet-pr-4410 environment in rivet-frontend
|
railway-app
bot
temporarily deployed
to
rivet-frontend / rivet-pr-4410
|
PR Review: Add inspector SQLite shell and property bindings This is a solid feature addition. The backend refactoring (PRAGMA-based schema introspection, named bindings, withDatabase lock helper) is a genuine improvement over the previous approach. BUGS parseEditedCellValue regex truncates type names (actor-database.tsx): The regex /\b(int|real|floa|doub|dec|num|bool)\b/ uses truncated fragments 'floa' and 'doub' with word boundaries that prevent matching the actual SQLite type names FLOAT or DOUBLE. Values in those columns will not be coerced to numbers. Fix: use full names 'float' and 'double'. DEFAULT_SQL hardcodes test_data (actor-database.tsx): Leftover development artifact. Any actor without a test_data table will see an error when opening the SQL shell. Better default: SELECT name FROM sqlite_master WHERE type = 'table'. CORRECTNESS applyEdits is not transactional (actor-database.tsx): Staged cell edits are applied as individual sequential UPDATE statements. If one fails midway, earlier updates are already committed and the UI shows the batch as still pending. Wrapping the batch in BEGIN/COMMIT/ROLLBACK would make multi-cell edits atomic. Null round-trip is broken (actor-database.tsx): formatCellDraft returns an empty string for null. When saved, parseEditedCellValue returns empty string not null. A NULL cell cannot be edited back to NULL unless the user types the word null. Consider using 'NULL' as the initial draft value for null cells to match expected parsing behavior. MINOR normalizeSqlitePropertyBindings triple-keys every property (actor-inspector.ts): Each unadorned key is expanded to three entries (colon, at-sign, dollar). Only one is ever used by SQLite. A brief comment explaining the intent would help future readers. withDatabase relies on result as T cast (actor-inspector.ts): The let result pattern works today but is fragile. Returning directly from the lock callback would be more explicit. jsonSafe silently truncates bigints (actor-inspector.ts): BigInts beyond MAX_SAFE_INTEGER lose precision. Acceptable for a debugging tool but worth a comment. Named binding extraction does not skip string literals (actor-database.tsx): extractNamedBindings matches raw SQL, so bindings inside quoted strings generate spurious UI fields. Minor UX issue, not a security concern. TEST COVERAGE: Happy-path tests are good. Suggested additions: empty sql, both args and properties supplied simultaneously, invalid binding type, malformed SQL. DOCUMENTATION: Additions in debugging.mdx, sqlite.mdx, and skill-base-rivetkit.md are clear and accurate. SUMMARY: The two bugs (regex truncation and DEFAULT_SQL) and the null round-trip should be fixed before merge. Non-transactional edit batching is a correctness concern worth addressing. Everything else is minor polish. |
jog1t
left a comment
jog1t
left a comment
There was a problem hiding this comment.
some stuff related to the react code, which llm did you use? i think we need to install react skills in the repo for better results.
| }, [stagedEdits]); | ||
| const stagedEditCount = stagedEditList.length; | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
this should happen on callback, no need for useEffect here, common llm mistake
just call those cleanup functions when the setTable i called, in transition for batched update.
| setTableEditError(null); | ||
| }, []); | ||
|
|
||
| const applyEdits = useCallback(async () => { |
There was a problem hiding this comment.
i wonder if this could be moved to the mutation, instead of doing everything here
| const hasNextPage = page < totalPages - 1; | ||
| const hasPrevPage = page > 0; | ||
|
|
||
| const beginCellEdit = useCallback( |
There was a problem hiding this comment.
there's a lot of logic that can be encapsulated in the separate hook so the row editing is a separate hook.
| Record<string, string> | ||
| >({}); | ||
| const [bindingChangeToken, setBindingChangeToken] = useState(0); | ||
| const [result, setResult] = useState<DatabaseExecuteResult | null>(null); |
There was a problem hiding this comment.
there's no need to have this here, you can use the mutation state for that.
| const hasMixedBindings = hasNamedBindings && hasPositionalBindings; | ||
|
|
||
| useEffect(() => { | ||
| setPropertyDrafts((current) => { |
There was a problem hiding this comment.
thats create a race condition that can overwrite user interaction (seen below), this can be derived on a first render.
| }; | ||
| }, [namedBindings, propertyDrafts]); | ||
|
|
||
| const resultColumns = useMemo(() => { |
There was a problem hiding this comment.
no need for useMemos, they obstruct the code and are added by the react compiler
| bindingError === null && | ||
| propertiesError === null; | ||
|
|
||
| useEffect(() => { |
| <ScrollArea className="h-full w-full"> | ||
| {result === null ? null : result.rows.length === 0 ? ( | ||
| <div className="px-3 py-4 text-sm text-muted-foreground"> | ||
| Statement executed successfully. No rows were | ||
| returned. | ||
| </div> | ||
| ) : resultRowsAreTable && resultColumns.length > 0 ? ( | ||
| <DatabaseTable | ||
| className="overflow-hidden" | ||
| columns={resultColumns} | ||
| enableColumnResizing={false} | ||
| enableRowSelection={false} | ||
| enableSorting={false} | ||
| data={result.rows} | ||
| /> | ||
| ) : ( | ||
| <pre className="overflow-auto px-3 py-4 text-xs"> | ||
| {JSON.stringify(result.rows, null, 2)} | ||
| </pre> | ||
| )} | ||
| </ScrollArea> |
There was a problem hiding this comment.
i think this view should be rethinked -> inline editing is not possible when using db shell, is that correct?
| } | ||
|
|
||
| const response = await fetch( | ||
| `${computeActorUrl({ ...credentials, actorId })}/inspector/database/execute`, |
There was a problem hiding this comment.
why does this not follow the BARE protocol?
2 participants
Description
This adds a manual SQLite shell to the actor Inspector UI, including query execution, result rendering, and editable named-property inputs that re-run the preview. It also adds
POST /inspector/database/executesupport for both positionalargsand namedproperties, updates the raw SQLite binding helpers, and documents the new flow.Type of change
How Has This Been Tested?
Ran
pnpm exec vitest run tests/driver-memory.test.ts -t "POST /inspector/database/execute|GET /inspector/state returns actor state"inrivetkit-typescript/packages/rivetkit.Checklist: