Add inspector SQLite shell and property bindings#4414
Add inspector SQLite shell and property bindings#4414NathanFlurry wants to merge 9 commits intomainfrom
Conversation
|
🚅 Deployed to the rivet-pr-4414 environment in rivet-frontend
|
ScreenshotsTables
Query results
Inline edit
Staged edits
Updated rows
Verify applied result
Verify discard result
|
Code ReviewOverall this is a well-structured addition. Clean API design, solid test coverage for the happy paths, and good validation gating in the router. A few things worth looking at. Functional Issues1. normalizeSqlitePropertyBindings triple-duplicates every key actor-inspector.ts lines 408-422: for an unprefixed key like foo, the function emits :foo, @foo, and $foo simultaneously. SQLite silently ignores the unused prefixes at runtime so it works, but a properties object with N keys becomes 3N bindings. A simpler approach would be to add : as the single default prefix for unprefixed keys, or scan the SQL to determine which prefix each placeholder actually uses. 2. extractNamedBindings matches inside string literals and comments actor-database.tsx lines 940-943: the regex runs over the entire SQL string including single-quoted string literals and -- comments. A query like SELECT comment-style :note FROM t would surface note as a binding input in the UI even though it is not an actual parameter. 3. hasPositionalBindings flags question marks inside string literals actor-database.tsx line 537: sql.includes is too broad. A query with a question mark inside a string literal triggers the positional-bindings warning and blocks execution even though no actual placeholder is present. 4. Conflict check misses non-array args router.ts lines 308-319: the guard checks Array.isArray on body.args before checking body.properties. If body.args is present but not an array and body.properties is also provided, the conflict check is skipped. The subsequent isSqliteBindingArray check fires with args must be a SQLite binding array rather than use either args or properties, not both. Not a breakage, but the user gets a confusing error message. Missing Test CasesThe new tests cover happy paths well. The following edge cases are not exercised:
Minor Nit: parseBindingDraft silent fallbackactor-database.tsx lines 962-964: when JSON.parse throws, the raw string is returned as the binding value with error: null. This is a reasonable UX choice, but a user cannot easily distinguish intentionally passing the string true vs. the boolean true. A small UI hint like bare text is treated as a string would help without changing behavior. Checklist itemsThe ladle build in the Railway deployment is failing, worth confirming that is pre-existing and not introduced by this PR. The normalizeSqlitePropertyBindings triple-prefix behavior and the bindingChangeToken debounce pattern in ActorDatabaseSqlShell would both benefit from a brief comment explaining their intent. |
PR Review: Add inspector SQLite shell and property bindingsOverall this is a solid feature addition with good test coverage and documentation. The UI architecture is clean and the backend API is well-structured. Issues1. File: For an unprefixed key like 2. File: The pattern uses 3. Files: The detection uses 4. File:
5. No error-path tests for File: All three new tests cover happy paths only. Suggested error cases: empty 6. File: Typing 7. File: SQLite Minor observations
|
1 participant
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: