feat: add inspector SQLite shell and property bindings#4410
feat: add inspector SQLite shell and property bindings#4410NathanFlurry merged 7 commits intomainfrom
Conversation
railway-app
bot
temporarily deployed
to
rivet-frontend / rivet-pr-4410
|
🚅 Deployed to the rivet-pr-4410 environment in rivet-frontend
|
railway-app
bot
temporarily deployed
to
rivet-frontend / rivet-pr-4410
railway-app
bot
temporarily deployed
to
rivet-frontend / rivet-pr-4410
railway-app
bot
temporarily deployed
to
rivet-frontend / rivet-pr-4410
PR Review: Add inspector SQLite shell and property bindingsThis is a solid feature addition. The backend refactoring (PRAGMA-based schema introspection, named bindings, Bugs1. The current 2. Any actor without 3.
Correctness4. The 5. Each unadorned property like 6. The UI error message should explicitly say Minor / Style
Test CoverageCurrent happy-path tests for the new endpoint are good. Gaps worth adding:
Positive Highlights
|
railway-app
bot
temporarily deployed
to
rivet-frontend / rivet-pr-4410
railway-app
bot
temporarily deployed
to
rivet-frontend / rivet-pr-4410
railway-app
bot
temporarily deployed
to
rivet-frontend / rivet-pr-4410
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?
NathanFlurry
changed the title
NathanFlurry
force-pushed
the
inspector-sqlite-shell
branch
from
1025bc5 to
339c3bc
Compare
railway-app
bot
temporarily deployed
to
rivet-frontend / rivet-pr-4410
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Merge activity
|
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: