fix(sql-editor): repair Server Beam gates, and make source/target visible - #159
Merged
Merged
Conversation
…ible Review of #158 found main shipped red: `tsc --noEmit` failed with two errors and two unit tests were failing. Fixed those, plus the defects behind them. **main was broken.** - `SqlBinding = ReturnType<typeof makeSqlBinding>` on a function annotated `: SqlBinding` is circular; TS2456 + TS2577. Dropped the annotation and let it infer — the alias still serves consumers. - `server-beam.test.ts` expected /at most 2/ while the message said "cant handle more than 2". Aligned the message (and its missing apostrophe). - `codeCellExec.test.ts` still asserted the OLD contract: #158 deliberately widened normalizeCodeCellReturn so `return 1` / `return [1,2,3]` / a bare object become grids. Updated the assertions to the new behaviour rather than narrowing the feature — only a missing return is rejected now. **Alias lookup accepted inherited keys.** `!beamDialects[key]` let `toString`, `constructor`, `valueOf` and `__proto__` past the unknown-alias check, then used the inherited *function* as the dialect — surfacing as "dialect .toLowerCase is not a function" instead of "Unknown Server Beam alias". Now `Object.hasOwn`. Not exploitable (the parent routes through a Map and fails closed) but a confusing dead end. Third time this class has appeared in this codebase — a shared hasOwn helper or lint rule would be cheaper than a fourth. **Which server is `target` was invisible.** Aliases come from list order, not click order, and `sql.on('target')` is what writes — so a wrong assumption writes to the wrong database. Two changes: a third checked Destination is now an error instead of a silent `slice(0, 2)`, and every beam run prints the resolved mapping ("Server Beam → source = A, target = B") before results. **Samples for both editor cases**, each executed against real SQLite before committing: - general, one server, no alias — plain sql`…`, no beam. - migration, source → target — read, reshape, chunked write, read back. Verified across two separate database files: rows landed in target, and the source was confirmed untouched. Writing the migration sample caught a bug in the sample itself: `domain` was split from the pre-lowercased email, yielding "Example.COM" beside "o'brien@example.com". Normalize once, then derive. 801 tests pass, tsc and eslint clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_2536d2ed-e5f0-4d70-9e8f-3a63d7f85fee) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Review of #158. The feature's design is sound — but it landed with the gates red, and the source/target mapping is invisible in exactly the operation where getting it wrong is worst.
main was shipping broken
Verified by stashing my changes and running against clean
main:cd apps/web && npx tsc --noEmitnpx vitest runSqlBinding = ReturnType<typeof makeSqlBinding>on a function annotated: SqlBindingis circular. Dropped the annotation, let it infer; the alias still serves consumers.server-beam.test.tsexpected/at most 2/but the message read "cant handle more than 2". Aligned the message (and its missing apostrophe).codeCellExec.test.tsstill asserted the old contract. feat: Server Beam — sql.on across two Destinations #158 deliberately widenednormalizeCodeCellReturnsoreturn 1,return [1,2,3]and a bare object all become grids — a good change. I updated the assertions to the new behaviour rather than narrowing the feature back.Alias lookup accepted inherited keys
code-cell-thread.tsguarded with!beamDialects[key]:sql.on('toString')skipped "Unknown Server Beam alias" and used the inherited function as the dialect, dying downstream asdialect.toLowerCase is not a function. NowObject.hasOwn.Not exploitable — the parent routes through a
Mapand fails closed — but a confusing dead end. This is the third appearance of this class here (aftersetBindingin the import parser); a sharedhasOwnhelper or a lint rule would be cheaper than a fourth fix.Which server is
targetwas invisibleThis is the one that can lose data. Aliases come from list order, not click order, and
sql.on('target')is what writes:slice(0, 2)— soparseBeamEndpoints' own "at most 2" error could never fire from the UI. Now an explicit error telling the user to uncheck the extras.Server Beam → source = A, target = B— before results.The server side needed nothing: aliases resolve per-user via
resolveRef, each shares the same permission policy from #154, and dialects are per-alias. Credit where due — a second connection path is where a gate usually gets forgotten, and it wasn't.Samples — one per editor case
Both executed against real SQLite before committing:
sql\…``, no beam. Verified: returns the bound row.o'brien@example.comintact, andsqlite_masterconfirmed the source was untouched.Writing that sample caught a bug in the sample itself —
domainwas split from the pre-lowercased email, producingExample.COMnext too'brien@example.com. Normalize once, then derive.801 tests pass, tsc and ESLint clean.
Note on the earlier hang
My first attempt at the migration sample hung for two minutes. That was my harness, not the product — I had run it before seeding the source database. Isolated it by testing the beam machinery with stubs, the runners standalone, and one
sql.onthrough the worker, all of which passed; the full sample then ran fine once both files existed. Worth knowing the shape though: because the cell clock pauses during a bridged query, a runner that never settles would hang without ever hitting the timeout.🤖 Generated with Claude Code
Note
Medium Risk
Changes affect Server Beam migration writes and alias routing; mistakes could target the wrong database, though the PR adds explicit mapping warnings and stricter destination limits.
Overview
Repairs Server Beam behavior and visibility in the SQL editor: alias resolution, destination selection, and documentation samples.
Server Beam alias lookup in the code-cell worker now uses
Object.hasOwnon the alias→dialect map so inherited keys liketoStringare rejected with a clear unknown-alias error instead of failing later on dialect handling. Tests document that trap.Execute path no longer silently keeps only the first two checked Destinations when more are selected; runs warn and stop when too many are checked, and each beam run prints
Server Beam → source = …, target = …so list order (not click order) is explicit before writes ontarget.makeSqlBindingdrops an explicit return type that caused a circular TypeScript error.Tests for code-cell return normalization now expect scalars, scalar arrays, and bare objects as grids (only null/undefined rejected). Sample bookmarks add one-server general Node SQL and a source→target migration example. Server Beam parse error text is aligned with the “at most 2” cap.
Reviewed by Cursor Bugbot for commit 286a485. Bugbot is set up for automated code reviews on this repo. Configure here.