Skip to content

feat(frontend): improve db ux#4683

Merged
jog1t merged 1 commit intomainfrom
04-20-feat_frontend_improve_db_ux
Apr 21, 2026
Merged

feat(frontend): improve db ux#4683
jog1t merged 1 commit intomainfrom
04-20-feat_frontend_improve_db_ux

Conversation

@jog1t
Copy link
Copy Markdown
Contributor

@jog1t jog1t commented Apr 21, 2026

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Copy link
Copy Markdown
Contributor Author

jog1t commented Apr 21, 2026

This stack of pull requests is managed by Graphite. Learn more about stacking.

@jog1t jog1t marked this pull request as ready for review April 21, 2026 14:49
@jog1t jog1t requested a review from NathanFlurry April 21, 2026 14:50
Copy link
Copy Markdown
Contributor Author

jog1t commented Apr 21, 2026

Merge activity

  • Apr 21, 2:50 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Apr 21, 2:52 PM UTC: Graphite rebased this pull request as part of a merge.
  • Apr 21, 2:52 PM UTC: @jog1t merged this pull request with Graphite.

@jog1t jog1t changed the base branch from 04-20-refactor_frontend_improve_go_to_actor_ux to graphite-base/4683 April 21, 2026 14:50
@jog1t jog1t changed the base branch from graphite-base/4683 to main April 21, 2026 14:50
@jog1t jog1t force-pushed the 04-20-feat_frontend_improve_db_ux branch from 89a7e13 to 0ae766d Compare April 21, 2026 14:51
@jog1t jog1t merged commit a980040 into main Apr 21, 2026
21 of 26 checks passed
@jog1t jog1t deleted the 04-20-feat_frontend_improve_db_ux branch April 21, 2026 14:52
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 21, 2026

Code Review: feat(frontend): improve db ux

Overview

This PR merges the separate table-browser and SQL-shell views into a single unified actor database panel. It adds CodeMirror SQL editing with schema-aware autocomplete, named-property bindings for UPDATE statements, smart editable-table detection, and AbortSignal.timeout on all inspector fetch calls. The direction is good, but a few issues need attention.


Bugs / Correctness

Rules of Hooks violation in useManagedPool (moderate–high)

function useManagedPool() {
  if (__APP_TYPE__ !== 'cloud') return false; // <-- early return before hooks
  const provider = useCloudNamespaceDataProvider();
  const { data: hasManagedPool } = useSuspenseQuery(...);
  return hasManagedPool;
}

Hooks cannot be called conditionally. Even though __APP_TYPE__ is a build-time constant and the bundle is always consistent, React's rule is unconditional. The lint rule will flag this and it breaks HMR in some toolchains. Fix: always call both hooks and gate on the value:

function useManagedPool() {
  const provider = useCloudNamespaceDataProvider();
  const { data: hasManagedPool } = useSuspenseQuery(
    provider.currentNamespaceHasManagedPoolQueryOptions(),
  );
  return __APP_TYPE__ === 'cloud' && hasManagedPool;
}

Silent error swallowing in selectTable

mutateAsync(request)
  .then((r) => setResult(r))
  .catch(() => {}); // errors silently discarded

If the initial table SELECT fails, the user sees nothing. At minimum set tableEditError or let sqlError from useMutation surface it.

detectEditableTable disables editing on empty results

The function returns null when rows.length === 0. If a user's WHERE clause legitimately returns 0 rows, the Editable badge disappears and staged edits are cleared — a user mid-editing would lose their work by re-running with a filter. Consider keeping the editable state from the previous run when the query text and table haven't changed.

Unused page state

const [page, setPage] = useState(0) is created and reset to 0 in selectTable, but there are no longer pagination controls in the merged view. Dead state should be removed.


Code Quality

sql_text naming — snake_case in TypeScript is inconsistent with the rest of the codebase. Rename to sqlText.

Duplicated refresh logic in applyEdits — After committing edits, applyEdits manually re-executes the current query and calls detectEditableTable — the same sequence as executeRun. This copy-paste will drift. Extract a shared helper and call it from both.

hasPositionalBindings false positivessql_text.includes('?') matches ? inside SQL string literals (e.g. WHERE name = 'what?'). Low severity but can confuse users.

showOnlyFatal prop declared but not implementedactor-status-indicator.tsx adds showOnlyFatal?: boolean to the props type and it's passed from actors-actor-details.tsx, but no filtering implementation is visible in the diff. If this is scaffolding for future work, add a TODO comment; otherwise wire it up.

renderBrowserCell early-return is redundant — The caller already guards with renderCell={canEditRows ? renderBrowserCell : undefined}, so the if (!canEditRows) return ... inside is never reached. Remove it.


Positive Highlights

  • Replacing positional ? args with named properties (:set_val, :pk_0) is a clear improvement for readability and safety.
  • AbortSignal.timeout(10_000) additions across actor-inspector-context.tsx are a good defensive improvement that was missing.
  • The handleRunRef stale-closure pattern is correct for passing handleRun into the CodeMirror keymap without stale captures.
  • fast-deep-equal replacing the JSON.stringify equality check is the right fix.
  • @codemirror/lang-sql with schema-aware autocomplete is a great UX upgrade.

Summary

Two blocking items before the next merge: the Rules of Hooks violation in useManagedPool and the silent error discard in selectTable. The rest (sql_text naming, unused page state, duplicated refresh logic) are cleanup that would improve maintainability.

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 21, 2026

Code Review: feat(frontend): improve db ux

Overview

This PR merges the separate Tables browser and Query SQL shell into a unified database view, adds a CodeMirror SQL editor with schema-aware autocomplete, and carries over inline cell-editing to the combined view. It also adds AbortSignal.timeout to several fetch calls and switches cell-update SQL from positional bindings to named properties.

The direction is good. However, there are several bugs and quality issues to address before merging.


Bugs

1. React Rules of Hooks violation in useManagedPool (critical)

In actors-actor-details.tsx, the new useManagedPool helper calls hooks after a conditional return. React requires hooks to always be called in the same order — returning early before hook calls will throw a “Rendered more hooks than during the previous render” error at runtime. Restructure so hooks are called unconditionally, then apply the condition to the return value.

2. showOnlyFatal prop leaks to the DOM and has no effect

In actor-status-indicator.tsx, showOnlyFatal is added to the interface but is not destructured separately from ...props, so it gets spread onto the underlying span element as an unknown DOM attribute. React will warn about this. The prop also has no effect since it is never read. Destructure it and implement the filtering logic, or remove it.

3. pendingRunConfirm dialog fires on every keystroke

In handleSqlChange, setPendingRunConfirm(true) is called whenever stagedEditCount > 0, which means the confirmation banner appears on every keystroke as the user types SQL — not only when they explicitly click Run (where this flag is also set). Only set pendingRunConfirm inside handleRun.


Code Quality

4. sql_text uses snake_case

This is the only snake_case local variable in the component. Rename to sqlText to match the camelCase convention used everywhere else.

5. page state is dead code

The page state is initialized and reset in selectTable, but the pagination UI (prev/next buttons, page counter) was removed in this PR. The state is never consumed in any rendered output. Remove it and PAGE_SIZE if pagination was intentionally dropped, or restore the pagination controls if they were accidentally omitted.

6. selectTable calls buildSelectSql twice and silently swallows errors

buildSelectSql(tableName) is called once to set state and again to build the request object. Extract to a local variable to avoid the duplicate call. The empty .catch(() => {}) means the user gets no feedback if the auto-selected table query fails — at minimum propagate into sqlError or call setResult(null) in the catch.

7. applyEdits duplicates the post-run refresh logic from executeRun

After committing edits, applyEdits manually calls mutateAsync again and re-runs detectEditableTable, duplicating the logic already in executeRun. Consider extracting a shared refresh helper or calling executeRun directly after the edits are applied.

8. detectEditableTable SQL parsing is fragile

The FROM regex will not handle CTEs (WITH cte AS (...) SELECT ?... FROM cte), schema-qualified names (main.users), subqueries in FROM, or bracket-quoted identifiers ([table name]). This is probably acceptable for de common case, but the known limitations should be noted in a comment so future readers understand when editability will silently not be detected.

9. Cmd+Enter hint is platform-specific

The keymap uses Mod-Enter (which maps to Ctrl on Windows/Linux), but the hint text hardcodes "Cmd+Enter". Change the label to "Ctrl/Cmd+Enter" or detect the platform at runtime.

10. Shared mutation state between auto-select and user-run queries

Both selectTable and executeRun share the same useMutation instance. When a table is auto-selected on mount, isPending becomes true and blocks the Run button. If the auto-select errors, sqlError appears in the query panel even though the user never initiated a run. Consider using a separate mutation or plain fetch for the auto-select path.


Minor

  • handleRunRef pattern: The mutable ref that breaks the stale-closure cycle in the CodeMirror keymap is valid but non-obvious. A short comment explaining why it is needed would help future readers.
  • AbortSignal.timeout additions: Good defensive improvement. Consider whether a uniform 10-second limit is appropriate for SQL execution on large result sets versus lightweight health/metadata calls.
  • PR description: The checklist is entirely unchecked and the description fields are empty. A brief note on what was manually tested would increase confidence.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 21, 2026

Preview packages published to npm

Install with:

npm install rivetkit@pr-4683

All packages published as 0.0.0-pr.4683.9557af4 with tag pr-4683.

Engine binary is shipped via @rivetkit/engine-cli on linux-x64-musl, linux-arm64-musl, darwin-x64, and darwin-arm64. Windows users should use the release installer or set RIVET_ENGINE_BINARY.

Docker images:

docker pull rivetdev/engine:slim-9557af4
docker pull rivetdev/engine:full-9557af4
Individual packages
npm install rivetkit@pr-4683
npm install @rivetkit/react@pr-4683
npm install @rivetkit/rivetkit-native@pr-4683
npm install @rivetkit/sqlite-wasm@pr-4683
npm install @rivetkit/workflow-engine@pr-4683

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant