feat: SafeQL playground#480
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughThis pull request adds a complete playground at apps/playground: a Vite+Vue app that embeds three Monaco editors (SQL/TS/JSON) with Shiki theming and quick-fixes, an in-browser TypeScript VFS, libpg-query WASM parsing, and a dual-worker architecture (DB worker + real-lint worker) coordinated via SharedArrayBuffer and Atomics. The main app manages state hydration/sharing via a compressed Base64URL hash, debounced linting through lintReal, draggable panel resizing, and dark-mode UI styling; Vite plugins and Node shims enable browser execution and cross-origin isolation headers are provided for hosting. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Newbie012
force-pushed
the
feat/playground
branch
3 times, most recently
from
59c7f2c to
b93cf75
Compare
Greptile SummaryThis PR adds a fully in-browser SafeQL playground (
Confidence Score: 3/5The playground works correctly for the default The
Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
apps/playground/src/worker/lint.worker.ts (1)
90-95: ⚡ Quick winAdd error recovery to prevent queue stalling.
If an unexpected error escapes
handleRequest(e.g., frompostMessageitself), the queue promise rejects and all subsequent requests silently skip. Append a.catch()to ensure the queue always resolves:♻️ Proposed fix
self.onmessage = (event: MessageEvent<WorkerRequest>) => { - queue = queue.then(() => handleRequest(event.data)); + queue = queue.then(() => handleRequest(event.data)).catch(() => {}); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/playground/src/worker/lint.worker.ts` around lines 90 - 95, The queue can reject if handleRequest or postMessage throws, which stalls all future requests; update the self.onmessage handler so the Promise chain uses queue = queue.then(() => handleRequest(event.data)).catch(err => { /* log or post error */ }); ensuring errors are caught and the chain always resolves so subsequent messages run; reference the existing symbols queue, handleRequest, and self.onmessage and add a .catch that logs the error (e.g., via console.error or postMessage) and returns void to continue the chain.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/playground/src/App.vue`:
- Around line 34-74: The runLint function can be overwritten by out-of-order
async completions; add a monotonic run token (e.g., a numeric runId or a per-run
symbol) stored in a shared reactive (e.g., currentRunId) and capture its value
at the start of runLint, then before mutating lintError.value,
diagnostics.value, rendered.value, status.value, and loading.value check that
the captured token still equals currentRunId; only apply results when they
match. Implement the same token-guard pattern for the other async lint result
handlers referenced (the other run-lint call sites around the same component) so
older promises cannot overwrite newer state.
- Around line 195-199: The header text omits configuration parse errors: update
the computed statusText so it includes configError (e.g. use lintError.value ??
configError.value ?? shareStatus.value ?? status.value) so that when configError
is set the UI shows that message; change the existing statusText computed that
currently references only lintError, shareStatus and status to include
configError.value before shareStatus.value/status.value to match the logic in
statusTone (which already checks configError).
In `@apps/playground/src/lib/parse-schema.ts`:
- Around line 4-8: The current tableHead regex in parse-schema.ts only captures
a single identifier so schema-qualified names like CREATE TABLE public.users
(...) are ignored; update the tableHead pattern to allow an optional schema
prefix (both unquoted and quoted schema forms, e.g. schema.table or
"schema"."table") and capture the final table identifier (quoted or unquoted) as
the table name, then adjust the name assignment that uses match[1]/match[2] to
pull the correct capture group for the table-only identifier and normalize it
(e.g. toLowerCase) as before; reference the tableHead regex and the code that
sets const name = (match[1] ?? match[2] ?? "").toLowerCase() for the change.
In `@apps/playground/src/lib/playground-config.ts`:
- Around line 33-35: readTag currently treats an empty string as a valid tag
which causes findSqlTemplates to match too broadly; update readTag (the function
that checks isRecord(target) && typeof target.tag === "string") to reject empty
tags by returning undefined (or null) when target.tag.trim() === "" (or
target.tag.length === 0) so downstream code like findSqlTemplates will not
consider empty tags as matches; ensure any callers handle the undefined result
appropriately.
In `@apps/playground/src/lib/render-error.ts`:
- Line 94: The current regex in tagMatch is too narrow and can misfire; replace
it with a pattern that enforces a word boundary before "sql", allows optional
generic type params with optional intervening whitespace, and permits optional
whitespace before the template-backtick so forms like `sql<T> \`...\`` are
matched while suffixes like `notsql\`...\`` are not. Update the regex used in
tagMatch (the line that builds tagMatch) to use a word boundary and optional
`\s*<[^>]*>\s*` around the identifier and then capture the template contents as
before.
In `@apps/playground/src/lib/resolved-target.ts`:
- Around line 53-54: The code builds a quoted property key without escaping,
causing invalid TS when keys contain quotes/backslashes; in the expression that
sets keyString (used by serializeResolvedTarget), escape any backslashes and
single quotes in key before wrapping it in single quotes (e.g., replace "\" with
"\\\\" and "'" with "\\'" on the key string) so the emitted
`${keyString}${optional}: ${serializeResolvedTarget(value, options)}` produces
valid TypeScript identifiers/strings.
In `@apps/playground/src/shims/libpg-query.ts`:
- Around line 59-79: The parse and parseSync functions call
wasmModule!._wasm_parse_query and immediately pass its return to ptrToString
without guarding for a null/zero pointer; add a check after calling
_wasm_parse_query in both parse and parseSync to handle resultPtr === 0 (free
the queryPtr, avoid calling ptrToString, and throw a descriptive Error including
context like "WASM returned null/zero pointer from _wasm_parse_query" or OOM) so
ptrToString is only invoked when resultPtr is non-zero and resources are cleaned
up in the error path as well.
In `@apps/playground/src/style.css`:
- Line 21: Fix the stylelint violations in the CSS by correcting declaration
spacing and keyword casing: in the style.css occurrences (notably the
color-scheme declaration and the other two offending declarations) ensure there
is exactly one space after the colon and no extra spaces before it (e.g.,
"color-scheme: dark;"), and normalize keyword casing to lowercase for property
values and keywords; after making those edits run stylelint --fix (or your
project's linter) to verify the three reported violations are resolved.
- Around line 286-302: Add an accessible keyboard focus style for .share-button
by implementing a .share-button:focus-visible rule that mirrors the hover visual
cues (e.g., change border-color to var(--brand) and background to
var(--brand-soft)) and adds a clear focus indicator such as a visible outline or
box-shadow (with outline-offset to avoid clipping); update the transition to
include the focus-visible property if needed so the focus change animates
consistently. Reference the .share-button selector and add
.share-button:focus-visible with the chosen border-color, background, and a
high-contrast outline/box-shadow to improve keyboard navigation feedback.
In `@apps/playground/src/worker/lint.worker.ts`:
- Around line 48-62: In ensureSchema, avoid leaving lastSchema stale when
RESET_SCHEMA_SQL succeeds but database.exec(schema) fails: set lastSchema = ''
before running await database.exec(RESET_SCHEMA_SQL), and only assign lastSchema
= schema and increment schemaVersion after the user schema exec completes
successfully; wrap the user exec (await database.exec(schema)) in a try/catch
that rethrows so failures don’t update lastSchema or schemaVersion (ensuring the
next call will attempt to re-apply the previous schema).
---
Nitpick comments:
In `@apps/playground/src/worker/lint.worker.ts`:
- Around line 90-95: The queue can reject if handleRequest or postMessage
throws, which stalls all future requests; update the self.onmessage handler so
the Promise chain uses queue = queue.then(() =>
handleRequest(event.data)).catch(err => { /* log or post error */ }); ensuring
errors are caught and the chain always resolves so subsequent messages run;
reference the existing symbols queue, handleRequest, and self.onmessage and add
a .catch that logs the error (e.g., via console.error or postMessage) and
returns void to continue the chain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fc3f24e3-0095-4f6c-b8b3-f5bffa8e1648
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (32)
apps/playground/index.htmlapps/playground/package.jsonapps/playground/src/App.vueapps/playground/src/components/EditorPanel.vueapps/playground/src/lib/defaults.tsapps/playground/src/lib/lint-core.tsapps/playground/src/lib/lint.tsapps/playground/src/lib/parse-schema.tsapps/playground/src/lib/pglite-sql.tsapps/playground/src/lib/playground-config.tsapps/playground/src/lib/playground-linter.tsapps/playground/src/lib/pointer.tsapps/playground/src/lib/polyagent-shiki.tsapps/playground/src/lib/polyagent-theme.tsapps/playground/src/lib/render-error.tsapps/playground/src/lib/render-error.types.tsapps/playground/src/lib/resolved-target.tsapps/playground/src/lib/share-url.tsapps/playground/src/lib/shiki.tsapps/playground/src/lib/ts-completions.tsapps/playground/src/main.tsapps/playground/src/postgres-socket.d.tsapps/playground/src/shims/libpg-query.tsapps/playground/src/shims/path-stub.tsapps/playground/src/shims/postgres-errors.tsapps/playground/src/shims/postgres.tsapps/playground/src/style.cssapps/playground/src/worker/lint.worker.tsapps/playground/tsconfig.jsonapps/playground/vite.config.tspackage.jsonpnpm-workspace.yaml
Newbie012
force-pushed
the
feat/playground
branch
3 times, most recently
from
5d1832a to
f1dffab
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
apps/playground/src/style.css (1)
20-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix active stylelint violations before merge.
Line [20], Line [41], and Line [108] still violate configured stylelint rules (
declaration-empty-line-before,value-keyword-case).Suggested patch
:root { --tracking-label: 0.08em; + color-scheme: dark; } @@ - text-rendering: optimizeLegibility; + text-rendering: optimizelegibility; @@ - background: currentColor; + background: currentcolor;Also applies to: 41-41, 108-108
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/playground/src/style.css` at line 20, Fix the stylelint violations in style.css by correcting the offending declarations: ensure the declaration-empty-line-before rule is satisfied (add or remove the empty line immediately before the declaration blocks as required) and normalize keyword casing to match value-keyword-case (use the expected lowercase/uppercase for value keywords used in declarations such as color-scheme and any other value tokens on the reported lines). Locate the problematic declarations by searching for the CSS property name "color-scheme" and other nearby declarations around the reported spots and update spacing and keyword case to conform to the configured stylelint rules.Source: Linters/SAST tools
🧹 Nitpick comments (1)
apps/playground/src/shims/path-stub.ts (1)
63-65:path-stub.relative()isn’t Node-compatible for non-prefix paths. The shim returnstowith a shared-prefix stripped (to.startsWith(from) ? ... : to), which won’t produce Node-style..segments (e.g.,/a/b→/a/c). Repo search found nopath.relative(...)call sites inapps/playgroundbeyond this shim itself (apps/playground/src/shims/path-stub.ts:63). If runtime consumers rely on Node semantics, implement Node’spath.relativebehavior; otherwise document the limitation or keep as-is.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/playground/src/shims/path-stub.ts` around lines 63 - 65, The current relative(from, to) implementation only strips a shared prefix and does not produce Node-style “..” segments for non-prefix paths; replace the body of the exported function relative(from: string, to: string) with a proper POSIX-compatible algorithm: normalize both paths (remove trailing slashes), split into segments, find the common ancestor length, build an array of '..' for each remaining segment in from, append the remaining segments from to, and join with '/' (handle identical paths returning '' or '.', per Node semantics), or alternatively add a clear comment on the function declaring it is intentionally non-Node-compatible and document the limitation where path.relative is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/playground/src/lib/monaco-shiki.ts`:
- Around line 20-30: The setupPromise is currently assigned to an async IIFE
that never resets on errors, causing permanent failure; wrap the body of the
async initializer (the code that registers LANGUAGES, calls
createHighlighterCore and shikiToMonaco) in a try/catch, and in the catch reset
setupPromise to undefined (or null) before rethrowing the error so subsequent
calls to setupMonacoShiki()/the initializer can retry initialization; reference
the setupPromise variable and the async block that calls createHighlighterCore
and shikiToMonaco when applying the change.
In `@apps/playground/src/lib/real-lint-client.ts`:
- Around line 100-109: When handling lint.onerror, in addition to calling
rejectAllPending, clear the cached lint worker reference so future calls
recreate it: inside the lint.onerror handler (the function named lint.onerror
that currently calls rejectAllPending(new Error(...))), set lintWorker =
undefined (or null) and any other cached lint-related state to reset the worker.
This ensures lintReal will not keep posting to a crashed worker.
In `@apps/playground/src/lib/share-url.ts`:
- Around line 35-46: Detect availability of
CompressionStream/DecompressionStream in encodePlaygroundState and
decodePlaygroundState and fall back to no-compression encoding when absent: in
encodePlaygroundState check if typeof globalThis.CompressionStream ===
"function" (or globalThis.CompressionStream) and only use new
CompressionStream("deflate-raw") when present, otherwise skip pipeThrough and
pass the JSON bytes straight to bytesToBase64Url; in decodePlaygroundState check
for globalThis.DecompressionStream and only call new
DecompressionStream("deflate-raw")/pipeThrough when available, otherwise decode
the base64UrlToBytes directly to JSON text; ensure both functions swallow
compression absence without throwing so callers like writePlaygroundStateToUrl
remain safe.
In `@apps/playground/src/worker/real-lint.worker.ts`:
- Around line 36-43: Replace the unbounded Atomics.wait(control, 0, 0) with a
bounded wait and error handling: call Atomics.wait(control, 0, 0, TIMEOUT_MS)
(e.g. 5000) or loop with repeated waits and a total timeout, check the return
value for "timed-out" and throw or return a descriptive error so the lint cycle
doesn't hang; keep the surrounding logic that uses Atomics.store(control, 0, 0),
self.postMessage({ type: "generate-request", schema: currentSchema, request:
args[0] }), and the subsequent Atomics.load(control, 1) /
TextDecoder().decode(data.slice(0, length)) but only proceed to read length when
the wait succeeded.
---
Duplicate comments:
In `@apps/playground/src/style.css`:
- Line 20: Fix the stylelint violations in style.css by correcting the offending
declarations: ensure the declaration-empty-line-before rule is satisfied (add or
remove the empty line immediately before the declaration blocks as required) and
normalize keyword casing to match value-keyword-case (use the expected
lowercase/uppercase for value keywords used in declarations such as color-scheme
and any other value tokens on the reported lines). Locate the problematic
declarations by searching for the CSS property name "color-scheme" and other
nearby declarations around the reported spots and update spacing and keyword
case to conform to the configured stylelint rules.
---
Nitpick comments:
In `@apps/playground/src/shims/path-stub.ts`:
- Around line 63-65: The current relative(from, to) implementation only strips a
shared prefix and does not produce Node-style “..” segments for non-prefix
paths; replace the body of the exported function relative(from: string, to:
string) with a proper POSIX-compatible algorithm: normalize both paths (remove
trailing slashes), split into segments, find the common ancestor length, build
an array of '..' for each remaining segment in from, append the remaining
segments from to, and join with '/' (handle identical paths returning '' or '.',
per Node semantics), or alternatively add a clear comment on the function
declaring it is intentionally non-Node-compatible and document the limitation
where path.relative is used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d08f3d41-f6f2-462a-89cc-eafd110d87b4
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (37)
apps/playground/index.htmlapps/playground/package.jsonapps/playground/public/_headersapps/playground/src/App.vueapps/playground/src/components/MonacoEditor.vueapps/playground/src/lib/defaults.tsapps/playground/src/lib/eslint-engine.tsapps/playground/src/lib/monaco-shiki.tsapps/playground/src/lib/pglite-sql.tsapps/playground/src/lib/playground-config.tsapps/playground/src/lib/real-lint-client.tsapps/playground/src/lib/safeql-theme.tsapps/playground/src/lib/share-url.tsapps/playground/src/lib/ts-vfs.tsapps/playground/src/main.tsapps/playground/src/postgres-socket.d.tsapps/playground/src/shims/eslint-browser.tsapps/playground/src/shims/eslint-unsupported.tsapps/playground/src/shims/libpg-query.tsapps/playground/src/shims/node-crypto.tsapps/playground/src/shims/node-empty.tsapps/playground/src/shims/node-globals.tsapps/playground/src/shims/node-os.tsapps/playground/src/shims/node-url.tsapps/playground/src/shims/node-util.tsapps/playground/src/shims/path-stub.tsapps/playground/src/shims/postgres-errors.tsapps/playground/src/shims/postgres.tsapps/playground/src/shims/synckit.tsapps/playground/src/style.cssapps/playground/src/worker/db.worker.tsapps/playground/src/worker/real-lint.worker.tsapps/playground/tsconfig.jsonapps/playground/vercel.jsonapps/playground/vite.config.tspackage.jsonpnpm-workspace.yaml
✅ Files skipped from review due to trivial changes (4)
- apps/playground/src/shims/node-crypto.ts
- apps/playground/src/shims/eslint-browser.ts
- apps/playground/src/shims/node-util.ts
- apps/playground/package.json
🚧 Files skipped from review as they are similar to previous changes (11)
- apps/playground/index.html
- apps/playground/src/main.ts
- apps/playground/src/shims/postgres-errors.ts
- apps/playground/src/shims/postgres.ts
- pnpm-workspace.yaml
- apps/playground/src/postgres-socket.d.ts
- apps/playground/tsconfig.json
- apps/playground/src/lib/defaults.ts
- apps/playground/vite.config.ts
- apps/playground/src/shims/libpg-query.ts
- apps/playground/src/lib/playground-config.ts
Add an in-browser playground (apps/playground) that runs the genuine @ts-safeql/check-sql ESLint rule entirely in the browser: write a query and see SafeQL's inferred result types and inline lint diagnostics live. The rule drives an async PGlite-backed `generate` from inside its sync ESLint context via a SharedArrayBuffer + Atomics bridge across two workers, with a small browser-compat shim layer for the node builtins it loads. Registers apps/* in the pnpm workspace and adds a root `playground` script.
Newbie012
force-pushed
the
feat/playground
branch
from
f1dffab to
2319f05
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/playground/src/lib/pglite-sql.ts`:
- Around line 104-114: The finally method (finally(onfinally?: (() => void) |
null)) currently calls onfinally?.() and ignores its return value, so async
rejections don't propagate; fix it by invoking and awaiting the onfinally result
via Promise.resolve(onfinally?.()) and chaining its resolution before returning
the original fulfilled value or rethrowing the original rejection (i.e., in the
success handler return Promise.resolve(onfinally?.()).then(() => value) and in
the failure handler return Promise.resolve(onfinally?.()).then(() => { throw
reason })), preserving native Promise.prototype.finally semantics in this
method.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c4767d75-4931-4b14-96eb-316222771f77
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (38)
apps/playground/index.htmlapps/playground/package.jsonapps/playground/public/_headersapps/playground/src/App.vueapps/playground/src/components/MonacoEditor.vueapps/playground/src/lib/defaults.tsapps/playground/src/lib/eslint-engine.tsapps/playground/src/lib/monaco-quickfix.tsapps/playground/src/lib/monaco-shiki.tsapps/playground/src/lib/pglite-sql.tsapps/playground/src/lib/playground-config.tsapps/playground/src/lib/real-lint-client.tsapps/playground/src/lib/safeql-theme.tsapps/playground/src/lib/share-url.tsapps/playground/src/lib/ts-vfs.tsapps/playground/src/main.tsapps/playground/src/postgres-socket.d.tsapps/playground/src/shims/eslint-browser.tsapps/playground/src/shims/eslint-unsupported.tsapps/playground/src/shims/libpg-query.tsapps/playground/src/shims/node-crypto.tsapps/playground/src/shims/node-empty.tsapps/playground/src/shims/node-globals.tsapps/playground/src/shims/node-os.tsapps/playground/src/shims/node-url.tsapps/playground/src/shims/node-util.tsapps/playground/src/shims/path-stub.tsapps/playground/src/shims/postgres-errors.tsapps/playground/src/shims/postgres.tsapps/playground/src/shims/synckit.tsapps/playground/src/style.cssapps/playground/src/worker/db.worker.tsapps/playground/src/worker/real-lint.worker.tsapps/playground/tsconfig.jsonapps/playground/vercel.jsonapps/playground/vite.config.tspackage.jsonpnpm-workspace.yaml
✅ Files skipped from review due to trivial changes (7)
- pnpm-workspace.yaml
- apps/playground/src/postgres-socket.d.ts
- apps/playground/src/main.ts
- apps/playground/src/lib/defaults.ts
- apps/playground/index.html
- apps/playground/package.json
- apps/playground/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (23)
- apps/playground/vercel.json
- apps/playground/src/lib/safeql-theme.ts
- apps/playground/src/shims/eslint-unsupported.ts
- apps/playground/src/shims/node-os.ts
- apps/playground/src/shims/node-globals.ts
- apps/playground/src/shims/postgres.ts
- apps/playground/src/shims/node-util.ts
- apps/playground/src/shims/eslint-browser.ts
- apps/playground/src/shims/node-crypto.ts
- apps/playground/src/shims/postgres-errors.ts
- apps/playground/src/shims/node-url.ts
- apps/playground/public/_headers
- apps/playground/src/shims/node-empty.ts
- package.json
- apps/playground/src/worker/real-lint.worker.ts
- apps/playground/src/shims/synckit.ts
- apps/playground/src/lib/ts-vfs.ts
- apps/playground/vite.config.ts
- apps/playground/src/lib/monaco-shiki.ts
- apps/playground/src/lib/playground-config.ts
- apps/playground/src/shims/libpg-query.ts
- apps/playground/src/lib/share-url.ts
- apps/playground/src/App.vue
| finally(onfinally?: (() => void) | null) { | ||
| return this.then( | ||
| (value) => { | ||
| onfinally?.(); | ||
| return value; | ||
| }, | ||
| (reason) => { | ||
| onfinally?.(); | ||
| throw reason; | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
MDN / ECMAScript semantics: Promise.prototype.finally behavior when callback returns a Promise that rejects.
💡 Result:
When the callback passed to Promise.prototype.finally returns a Promise that rejects, the Promise returned by the finally method will reject with the same reason [1][2]. According to the ECMAScript specification [3][4][5][6], Promise.prototype.finally works by scheduling internal 'then' and 'catch' handlers. When the provided onFinally callback is invoked, if it returns a promise, the resulting promise waits for that promise to settle. If that returned promise rejects, the finally-returned promise will reject with the same rejection reason [1][2]. Essentially, if the finally callback returns a rejected promise or throws an error, it acts as an abrupt completion that overrides the original settlement state of the parent promise, causing the new promise returned by finally to reject with that new reason [1][7][2]. If the callback does not return a rejected promise (and does not throw), the finally-returned promise will settle with the same state (fulfillment value or rejection reason) as the original promise [1][2].
Citations:
- 1: https://github.com/mdn/content/blob/main/files/en-us/web/javascript/reference/global_objects/promise/finally/index.md
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/finally
- 3: https://tc39.es/ecma262/multipage/control-abstraction-objects.html
- 4: https://tc39.es/ecma262/2025/multipage/control-abstraction-objects.html
- 5: https://tc39.es/ecma262/pr/3782/multipage/control-abstraction-objects.html
- 6: https://tc39.es/ecma262/2024/multipage/control-abstraction-objects.html
- 7: https://github.com/tc39/proposal-promise-finally
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print the relevant section around the reported lines
FILE="apps/playground/src/lib/pglite-sql.ts"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
echo "=== File: $FILE (lines 80-140) ==="
nl -ba "$FILE" | sed -n '80,140p'
echo
echo "=== Search for 'PendingQuery' and adapter usage in the same file ==="
rg -n "PendingQuery|onfinally|finally\\(" "$FILE" || trueRepository: ts-safeql/safeql
Length of output: 189
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="apps/playground/src/lib/pglite-sql.ts"
echo "Repo files check:"
ls -la apps/playground/src/lib | sed -n '1,120p' || true
echo
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
echo "=== File: $FILE (lines 80-140) ==="
python3 - <<PY
import itertools
path = "$FILE"
start, end = 80, 140
with open(path, "r", encoding="utf-8") as f:
for i, line in enumerate(f, start=1):
if start <= i <= end:
print(f"{i:5d}: {line.rstrip()}")
if i > end:
break
PY
echo
echo "=== Search within file for PendingQuery/onfinally/finally usage ==="
rg -n "PendingQuery|onfinally|finally\\(" "$FILE" || true
echo
echo "=== Show definition signature/type of finally() if present near matches ==="
# Print a bit wider around the first occurrence
rg -n "finally\\(onfinally\\?" "$FILE" -n || trueRepository: ts-safeql/safeql
Length of output: 3875
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for postgres.PendingQuery type/definitions in the repo
echo "=== rg PendingQuery definitions/usages ==="
rg -n "PendingQuery" . || true
echo
echo "=== rg 'finally(onfinally' or 'finally\\(' patterns in repo ==="
rg -n "finally\\(onfinally|finally\\(" apps/playground/src/lib/pglite-sql.ts || trueRepository: ts-safeql/safeql
Length of output: 948
🌐 Web query:
postgres PendingQuery finally signature onfinally PromiseLike<void> types
💡 Result:
In the postgres (porsager/postgres) TypeScript types, PendingQuery is a thenable/P̆romise-like object, and its.finally typing is the Promise-based overload. 1) Why you see PromiseLike / “finally signature” issues - PendingQuery is defined as an interface that extends Promise<RowList> (i.e., it is Promise-like / thenable) [1]. Because it is Promise-like, TypeScript also applies Promise.prototype typings for.finally (the callback receives no arguments and returns void or a promise-like value, depending on the contextual typing) [2]. - The common pattern is that if you await or type-narrow a PendingQuery as resolving to void (e.g., via an execution path that you’ve typed as Promise), then the.finally callback gets contextual typing consistent with Promise / PromiseLike. That’s where the PromiseLike wording typically comes from. 2) The specific “finally” behavior in postgres types - In the currently published type definitions surfaced here, PendingQuery extends Promise<RowList> [1], and Promise semantics for.finally are defined by the standard library: it accepts an optional onFinally callback ( => void) and returns a Promise (settlement completion) [2]. - Additionally, postgres’ type helper “Fragment” is aliased to PendingQuery, meaning fragments are still typed as thenables/Promises in the type system [3]. This can lead to “fragment vs query” confusion in async helper code (for example returning it from an async function), as discussed in the upstream issue [4]. 3) Practical guidance (fixing the typing) - If you have an async function that is meant to build a query fragment and you return sql..., don’t make the builder async unless you wrap/transform the result, because the returned value is a PendingQuery (thenable) [4]. That pattern affects how TypeScript contextual-reads the resulting type, which can cascade into.finally callback signatures like PromiseLike. - If your goal is to call.finally without the Promise contextual typing, ensure the expression you call.finally on is typed as PendingQuery (not Promise). Concretely: make sure the generic type T of PendingQuery isn’t being erased to void by your own wrapper typings. Grounding notes on sources used - PendingQuery extends Promise<RowList> in the postgres type surface [1]. - Promise.prototype.finally in TS’s Promise typings is specified with onFinally?: ( => void) and Promise return behavior [2]. - The upstream discussion highlights that postgres query fragments are thenables and are executed/treated as queries when awaited/returned from async flows, motivating separation concerns [4]. - The type alias Fragment = PendingQuery means fragments are typed as thenables as well [3]. If you paste your exact TypeScript error text (and the type of the value you’re calling.finally on), I can tell you exactly where the void/PromiseLike context is coming from and what minimal type annotation/wrapper fixes it.
Citations:
- 1: https://tessl.io/registry/tessl/npm-postgres/3.4.0
- 2: https://typhonjs-typedoc.github.io/ts-lib-docs/2023/esm/interfaces/Promise.html
- 3: https://cdn.jsdelivr.net/npm/[email protected]/types/index.d.ts
- 4: Cannot return SQL fragment directly from async function porsager/postgres#1019
🌐 Web query:
tessl npm-postgres 3.4.0 PendingQuery finally onfinally?: () => void | PromiseLike<void>
💡 Result:
In the postgres npm package (also published as tessl/npm-postgres) version 3.4.0, the PendingQuery interface does not include a finally or onfinally method [1][2][3]. PendingQuery is designed to extend the native JavaScript Promise class [1][2][3]. Since it behaves as a standard Promise, you should use the native.finally method provided by the Promise prototype to handle cleanup or final execution logic [1][2][3]. The PendingQuery interface primarily provides methods for query execution control and result processing, such as: - simple - execute - describe - cancel - cursor - forEach - values - raw - readable - writable You can chain.finally directly to your query object just as you would with any other promise [1][2]. For example: sqlSELECT * FROM users.finally( => { // Cleanup logic here });
Citations:
- 1: https://tessl.io/registry/tessl/npm-postgres/3.4.0
- 2: https://tessl.io/registry/tessl/npm-postgres
- 3: https://cdn.jsdelivr.net/npm/[email protected]/types/index.d.ts
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="apps/playground/src/lib/pglite-sql.ts"
python3 - <<'PY'
path = "apps/playground/src/lib/pglite-sql.ts"
with open(path, "r", encoding="utf-8") as f:
lines = f.readlines()
for i in range(1, 170):
if i-1 < len(lines):
print(f"{i:5d}: {lines[i-1].rstrip()}")
PYRepository: ts-safeql/safeql
Length of output: 5335
🌐 Web query:
TypeScript lib.es2018.promise.d.ts Promise.finally onfinally signature return type PromiseLike<void>
💡 Result:
In TypeScript's standard library definition (lib.es2018.promise.d.ts), the finally method on the Promise interface is defined with the following signature: finally(onfinally?: ( => void) | undefined | null): Promise; This signature requires the onfinally callback to return void [1][2][3]. Historically, this has been a point of discussion within the TypeScript community because the JavaScript Promise.prototype.finally method does wait for a returned Promise to resolve (or reject) before settling the returned promise [4][5]. Users have frequently requested that the signature be updated to allow PromiseLike<void> (or void | PromiseLike<void>) to align the type definition with actual runtime behavior and with the pattern used in Promise.prototype.then [6][5]. Despite these requests, the standard lib.es2018.promise.d.ts declaration remains restricted to () => void [1]. If you encounter type errors when returning a Promise from a finally block, it is because the TypeScript compiler strictly enforces the defined return type of void, which effectively ignores any returned value rather than treating it as a Promise to be awaited [7][6][5]. Top results: [1][6][5]
Citations:
- 1: https://github.com/microsoft/TypeScript/blob/main/src/lib/es2018.promise.d.ts
- 2: https://github.com/Microsoft/TypeScript/blob/v3.8.3/lib/lib.es2018.promise.d.ts
- 3: https://mariusschulz.com/blog/using-promise-prototype-finally-in-typescript.html
- 4: Add shim function to typings for promise.prototype.finally DefinitelyTyped/DefinitelyTyped#13339
- 5: Promise#finally's callback doesn't accept Promise<void> callback, unlike Promise#then microsoft/TypeScript#44980
- 6: Promise#finally: also accept callback that returns Promise<void> microsoft/TypeScript#38752
- 7: Wrong type for Promise.finally in lib.es2018.promise.d.ts microsoft/TypeScript#29885
Fix finally() to await onfinally return value (Promise-finally semantics)
finally(onfinally?: (() => void) | null) calls onfinally?.() but ignores its return value; if onfinally returns a rejected Promise (e.g., an async callback), that rejection won’t propagate, unlike native Promise.prototype.finally.
Suggested fix
finally(onfinally?: (() => void) | null) {
return this.then(
(value) => {
- onfinally?.();
- return value;
+ return Promise.resolve(onfinally?.()).then(() => value);
},
(reason) => {
- onfinally?.();
- throw reason;
+ return Promise.resolve(onfinally?.()).then(() => {
+ throw reason;
+ });
},
);
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| finally(onfinally?: (() => void) | null) { | |
| return this.then( | |
| (value) => { | |
| onfinally?.(); | |
| return value; | |
| }, | |
| (reason) => { | |
| onfinally?.(); | |
| throw reason; | |
| }, | |
| ); | |
| finally(onfinally?: (() => void) | null) { | |
| return this.then( | |
| (value) => { | |
| return Promise.resolve(onfinally?.()).then(() => value); | |
| }, | |
| (reason) => { | |
| return Promise.resolve(onfinally?.()).then(() => { | |
| throw reason; | |
| }); | |
| }, | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/playground/src/lib/pglite-sql.ts` around lines 104 - 114, The finally
method (finally(onfinally?: (() => void) | null)) currently calls onfinally?.()
and ignores its return value, so async rejections don't propagate; fix it by
invoking and awaiting the onfinally result via Promise.resolve(onfinally?.())
and chaining its resolution before returning the original fulfilled value or
rethrowing the original rejection (i.e., in the success handler return
Promise.resolve(onfinally?.()).then(() => value) and in the failure handler
return Promise.resolve(onfinally?.()).then(() => { throw reason })), preserving
native Promise.prototype.finally semantics in this method.
1 participant
Summary
Adds an in-browser SafeQL playground (
apps/playground) — write a query and see SafeQL's inferred result types and inline lint diagnostics live, running entirely in the browser via PGlite and@ts-safeql/generate.Wiring: registers
apps/*in the pnpm workspace and adds a rootplaygroundscript (pnpm playground).Summary by CodeRabbit