mirror of
https://github.com/supabase/supabase.git
synced 2026-09-07 10:29:14 +08:00
create-pull-request/patch
16 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Charis
|
c0e109f662 |
refactor(studio): borrow the wire schema's time range in the query-source registry (#49070)
Second of a stack. **Stacked on #49069** — review that one first; this PR's diff only makes sense on top of it. Base will retarget to `master` automatically when #49069 merges. Net `-72` lines. No behavior change beyond the one noted at the bottom. ## The problem The query-source registry carried its own `LogTimeRange` type and `logTimeRangeSchema`, which had drifted from the notebook wire schema's copy in four ways: | | wire schema | registry | |---|---|---| | discriminant | `_tag: 'relative_time_range'` | `type: 'relative'` | | absolute bounds | `start` / `end` | `from` / `to` | | relative units | minute…year | minute, hour, day | | validation | none | positive int, end-after-start | Two definitions of one concept, neither convertible to the other without a lossy mapping — and the notebook query cell was papering over it by discarding a log cell's persisted range and substituting a default. ## What changed #49069 moved the validations onto the wire schema's `timeRangeSchema` and exported it. This PR deletes the registry's copy and points every consumer at `TimeRange`. The registry keeps what is genuinely runtime: endpoints, labels, availability, defaults. The field renames ripple mechanically through the logs date-picker helpers, the time-range submenu, `useLogsCustomRange`, the SQL editor's session state, and their tests. Coverage for the absolute-range and unit rules moved to `notebook-schema.test.ts` in #49069, alongside the schema that now owns them. `ExplorerQuerySourceMenu` also drops its hand-rolled custom-range construction in favor of `customDateRangeToLogTimeRange`, which already existed and does the same clamping. ## One behavior change `logTimeRangeToDatePickerValue` now renders a range whose unit has no picker preset (week, month, year — allowed by the wire schema, not offered in the UI) as a resolved absolute range, instead of trying and failing to build a helper for it. Previously unreachable, since the registry's narrower unit set made those ranges unrepresentable. ## Verification Typecheck, Prettier, and the lint ratchet clean. 401 tests pass across the notebook schema, query sources, the logs source components, the SQL editor, and the Explorer surfaces. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved log time-range handling across Explorer and SQL Editor. * Custom date ranges now display and resolve correctly, including clamping invalid ranges. * Unsupported relative time units are converted to compatible absolute date-picker values. * **Refactor** * Standardized log queries on a shared time-range format for more consistent validation and behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Joshen Lim <joshenlimek@gmail.com> |
||
|
Saxon Fletcher
|
cc6fe2100a |
refactor(studio): centralize query sources (#49027)
## Summary - define application-owned database and logs source contracts, defaults, validation, labels, and execution endpoints - extract controlled database and logs parameter controls for reuse outside SQL snippets - adapt the SQL editor to the shared source model without changing snippet behavior - standardize source icons at 16px with a 2px stroke - keep relative logs ranges aligned with the existing date picker units ## To test 1. Open an existing query in the SQL Editor and run it against the database. 2. Switch the query source to Logs, change the time range, and confirm the query still runs as expected. ## Why Explorer queries and notebook query cells need to select an execution source without coupling that source to SQL snippets. This provides the shared registry and controlled UI foundation for those consumers. ## Impact Existing SQL snippets retain their current database/logs routing and session behavior. The registry documents the SQL editor legacy database-selector adapter while new consumers own their identifier inline. The shared Logs date picker remains unchanged; query ranges support its existing minute, hour, and day units. This PR does not add the Explorer query tab itself. ## Validation - pnpm --filter studio typecheck - focused Vitest coverage for the registry, canonical log-range utilities, SQL execution adapters, source filtering, retention locking, custom ranges, and preset selection - pnpm --filter studio run lint:ratchet Component and state tests cover this change per the Studio testing guidance; no E2E test is added. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a unified query-source menu for database queries and logs. * Added custom log time-range selection with calendar support and retention-aware upgrade prompts. * Added consistent source icons and improved database selection handling. * Added support for relative and absolute log time ranges. * **Bug Fixes** * Improved log-range validation, defaults, and current-time handling. * Updated query execution to use the correct source-specific endpoints. * **Tests** * Expanded coverage for query sources, log ranges, menus, and retention behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Charis
|
d5436ae826 |
feat(studio): log date range domain + session logRange state (#48401)
## I have read the [CONTRIBUTING.md](https://github.com/supabase/supabase/blob/master/CONTRIBUTING.md) file. YES ## What kind of change does this PR introduce? Feature (+ a small refactor and a docs/convention note). PR 4 of the stacked SQL-editor query-source series (Database vs Logs). ## What is the current behavior? The SQL editor has no representation of a logs query's time range: `querySource.ts` only knows how to map a snippet type to a source (`getSnippetSource`), and session state (`sql-editor-session-state.ts`) tracks results and the row limit but not a per-snippet time range. The Logs date picker's pure range helpers (`parseCustomInput`, `generateDynamicHelper`, the `Unit` type) are trapped inside the `Logs.DatePickers.tsx` React component. ## What is the new behavior? - **Logs time-range domain** in `querySource.ts`: branded `IsoDateTimeString` + `isoDateTimeString()`, `RelativeTimeUnit`, a `LogDateRange` discriminated union (relative/absolute), `DEFAULT_LOG_DATE_RANGE`, a single date-picker parser (`datePickerValueToLogDateRange` / `logDateRangeToDatePickerValue` — handles the five presets *and* dynamic `2h`/`30m` helpers; `calcTo === ''` means "now"; unparseable helpers degrade to absolute), and `resolveLogRunRange` which re-resolves relative ranges against `now` at run time (reusing the existing `ResolvedLogDateRange` shape). - **Session state**: per-snippet `logRange` + `setLogRange` — session-only, never written to snippet content, so it works on read-only shared snippets and is cleaned up in `clearForSnippet`. - **Refactor**: extracted the picker's framework-free helpers into a new pure `Logs.datePickerHelpers.ts`; the logs domain now shares the `Unit` type and reuses `generateDynamicHelper` instead of duplicating them. Importers point at the new module directly (no re-export shim). Hardened the amount parse against `NaN`. - **Full unit coverage** in `querySource.test.ts`. Recorded the no-shim refactoring convention in the `studio-best-practices` skill. Verification: `pnpm typecheck` clean, lint ratchet improved, 43 tests pass (querySource + Logs.Datepickers), Prettier clean. ## Additional context <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added robust Logs date-range modeling with support for relative (e.g., last N units) and absolute time periods. - SQL Editor sessions now remember log date ranges per snippet. - **Bug Fixes** - Safer handling of invalid or missing date inputs, with sensible fallback to default/current time. - **Tests** - Added/expanded automated coverage for date-range conversion, helper parsing, and resolution behavior. - **Refactor** - Centralized date-picker helper utilities for reuse across the Logs and SQL query experience. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Charis
|
fa5eb17277 |
feat(studio): discriminated snippet union + source-aware writes (#48313)
Stacked on #48305. ## What PR 3 of the stacked SQL-editor query-source series (Database vs Logs). Stacked on the PR 2 branch `charislam/log-sql-content-shape`. Turns `SnippetWithContent` into a discriminated union on `type` and makes all snippet writes source-aware: - `data/content/sql-folders-query.ts`: `SnippetWithContent` is now `{ type: 'sql'; content?: SqlSnippets.Content } | { type: 'log_sql'; content?: LogSqlSnippets.Content } | { type: 'report'; content?: never }`. `report` is kept (the content endpoints' wire type carries it) but has no SQL content — its body is `Dashboards.Content`, loaded through the separate `Content` union. - `setSql` brands per type (`untrustedLogSql` vs `untrustedSql`). - `buildUpsertPayload` persists `snippet.type` (no longer hardcoded `'sql'`). - `createSqlSnippetSkeletonV2({ source })` emits the matching type + content shape with the `as any` cast removed. - New `components/interfaces/SQLEditor/querySource.ts`: `SqlSnippetSource` + `getSnippetSource`. - `seedSnippet` test helper gains a `source` arg. - New `remapWireSnippet` boundary helper in `content-remap.ts` concentrates the single wire->domain assertion, so `content-id-query` / `content-upsert-mutation` call sites are cast-free (no `as unknown as`). - Collateral: query result types aligned to the union; `updateSnippet` no longer accepts `type` (source is immutable); db-only editor read paths narrow away `log_sql`. ## Why Impossible-states-impossible typing: a snippet's brand follows its content type, so logs SQL and database SQL can never cross execution paths. No behavior change for existing database snippets. ## Testing - \`pnpm typecheck\` — clean - \`pnpm --filter studio run lint:ratchet\` — no new warnings - \`pnpm test:studio\` (data/content, SQLEditor, state/sql-editor) — passing, including new tests for \`getSnippetSource\`, source-aware \`setSql\`, type-aware \`buildUpsertPayload\`, and both skeleton shapes. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added source-aware creation for SQL editor snippets, including log-based SQL snippets. * Introduced backend source mapping so log snippets are treated as log_sql. * **Bug Fixes** * Improved SQL retrieval/prettification so log snippets no longer use the wrong fallback content. * Ensured log snippets are sanitized and preserve correct type, content, identifiers, and statuses during save/upsert flows. * **Tests** * Expanded unit and integration coverage for log snippet creation, source mapping, editing, prettification, and upsert payloads. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Ayaan Gazali
|
5db1137c56 |
fix(sql-editor): guard removeFavorite against missing snippet like addFavorite (#48111)
## I have read the
[CONTRIBUTING.md](https://github.com/supabase/supabase/blob/master/CONTRIBUTING.md)
file.
YES
## What kind of change does this PR introduce?
Bug fix
## What is the current behavior?
Fixes #48110
In the SQL editor Valtio store, `removeFavorite` guards against a
missing snippet with `if (storeSnippet.snippet)`, which reads `.snippet`
off `undefined` and throws `TypeError: Cannot read properties of
undefined (reading 'snippet')` whenever the id is not loaded in
`sqlEditorState.snippets`. Its counterpart `addFavorite` guards
correctly with `if (storeSnippet)` and no-ops on the same input.
## What is the new behavior?
`removeFavorite` now uses the same `if (storeSnippet)` guard as
`addFavorite`, so un-favoriting an id that is not in the store is a safe
no-op instead of a crash. Behavior for loaded snippets is unchanged.
Since `StateSnippet.snippet` is a required field, the old check was
always true whenever `storeSnippet` existed, so the only real world
difference between the two guards was the crash on the missing case.
I also added a small vitest file covering both methods (favorite set
plus needsSaving queued for loaded snippets, no-op for missing ids). The
missing-id test for `removeFavorite` fails with the exact TypeError
above when run against the old guard, and passes with this fix.
## Additional context
Root cause: `apps/studio/state/sql-editor/sql-editor-state.ts` line 260
(compare `removeFavorite` at lines 258 to 264 with `addFavorite` at
lines 250 to 256).
Gates run locally on top of current master (
|
||
|
Charis
|
1c827c5cbb |
refactor(sql-editor): extract title-gen/execute-params + merge auto-limit functions (#48013)
## Summary Part 2/6 of the SQL Editor testability follow-up, stacked on #47980 (the analyzeQueryIssues/resolveConnectionString PR). - Extracts `shouldAutoGenerateTitle` and `buildExecuteParams` out of `useSqlEditorExecution`'s inline logic into `SQLEditor.utils.ts`. - Merges `checkIfAppendLimitRequired` and `suffixWithLimit` into a single `applyAutoLimit` function — the two were only ever called together and re-parsed the same query twice at every call site. `applyAutoLimit` only accepts `SafeSqlFragment` (never a plain string) and composes the `LIMIT` suffix through `safeSql`/`literal` rather than raw template concatenation, so the only place in the file that reasserts the `SafeSqlFragment` brand on a derived string is the small, dedicated `trimTrailingSemicolons` helper — removing existing terminators can't introduce unsafe content, unlike gluing new text onto the fragment. - Updates the two other `checkIfAppendLimitRequired`/`suffixWithLimit` call sites (`EditorPanel.tsx`, `ReportBlock.tsx`) accordingly; `ReportBlock` now promotes its report SQL once and reuses the result for both its display-only auto-limit hint and its execution, instead of promoting twice. ## Test plan - [x] `pnpm --filter studio typecheck` - [x] `pnpm test:studio -- SQLEditor ReportBlock EditorPanel` (239 tests passing) |
||
|
Ali Waseem
|
b100272376 |
chore(sql-editor): remove Pretty Explain feature (#47981)
Removes the SQL Editor Pretty Explain feature — the Explain tab, the Run EXPLAIN ANALYZE action + shortcut, and its dead plumbing. It's been gated off behind the `DisablePrettyExplainOnSqlEditor` kill switch for weeks with no usage or complaints. `ExplainVisualizer` / `isExplainQuery` are kept — they're used independently by Query Insights, Query Performance, and the EditorPanel quick-runner. Manually-run `EXPLAIN` queries still render as raw rows in the Results tab. Typecheck, lint, and all affected unit tests pass. Closes FE-3930 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Changes** * Removed the SQL editor’s EXPLAIN execution workflow, including its toolbar action, keyboard shortcut, utility tab, and visual query-plan display. * Simplified query execution to focus on standard results and charts. * Improved result clearing when switching databases and refined execution error handling. * Updated SQL editor state and tests to reflect the streamlined experience. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Charis
|
1987f19d0a |
feat(sql-editor): add manual save feature preview (#47745)
## What Adds an opt-in **SQL Editor manual save** feature preview that switches the SQL Editor from autosaving every edit to saving only on demand, and hardens the tab-close flow so unsaved edits are handled correctly. ## Changes **Feature preview** - New `sqlEditorManualSave` flag + `UI_PREVIEW_SQL_EDITOR_MANUAL_SAVE` local-storage toggle, wired into the Feature Preview modal with an explanatory panel. - `useIsSqlEditorManualSaveEnabled` gates behavior on both the flag and the user's preview opt-in. **Editor toolbar** - Save button (with `Cmd+S`) next to Run, plus an autosave status indicator showing dirty/saving/saved state and a shortcut to disable autosave (emits a `sql_editor_autosave_disable_clicked` telemetry event). **Discard on close** - Closing a snippet tab with unsaved edits prompts for confirmation and, on confirm, actually discards the local edits and evicts the cached server copy so the snippet reopens clean. **Decouple tab layout from SQL specifics** - Tabs store gains a generic per-type close-handler registry (`registerTabCloseHandler` / `getCloseConfirmation` / `closeTabs`). The SQL editor registers its discard + confirmation behavior from the save coordinator. - Low-level `removeTab`/`removeTabs` (rename/move re-keying, stale cleanup) intentionally do **not** trigger discard. - Adds `statusOnDiscard` lifecycle transition and `clearSnippetContent` store action. ## Testing - `pnpm --filter=studio typecheck` — clean. - Added unit tests for the close-handler registry (fires on single/multi close, skips re-keying/cleanup removals, respects tab type, selects confirmation copy, unregisters cleanly). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a SQL editor manual-save preview with a “Save” button and `Cmd+S`, plus a modal option to disable manual-save/preview. * Added “unsaved changes” tab status indication when manual-save is enabled. * Introduced tab-type-specific close confirmations (shown only when needed). * **Bug Fixes** * In manual-save mode, closing a SQL tab with unsaved edits now clears local snippet content and refreshes it on reopen. * **Tests** * Added coverage for tab close handlers and confirmation behavior. * **Chores** * Added a persisted setting allowlist entry and tracked autosave-disable clicks via telemetry. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Charis
|
cdc2dc4e26 |
refactor(studio): import SQL editor store from source, delete facade + barrel (#47533)
## What Final PR of the SQL editor state re-layering stack. Removes the compatibility shims left in place during the migration: - Migrates all **23** consumers of the `@/state/sql-editor-v2` facade to import directly from `@/state/sql-editor/sql-editor-state`, where `useSqlEditorV2StateSnapshot`, `getSqlEditorV2StateSnapshot`, `useSnippets`, and `useSnippetFolders` actually live. - Deletes `state/sql-editor-v2.ts` (the facade) and `state/sql-editor/index.ts` (the barrel). Both re-exported the same symbols; nothing imports them after the migration. This collapses the two-layer re-export (`sql-editor-v2` → `index` → source) into direct source imports, matching the repo convention to avoid barrel re-export files. ## Notes - Pure import-path migration — no behavior change. All 23 consumers imported only value symbols that resolve to `sql-editor-state.ts`; none imported the `StateSnippet`/`StateSnippetFolder` types via the facade. - Symbol names keep their `V2` suffix for now — renaming `useSqlEditorV2StateSnapshot` etc. is a separate, larger churn best done on its own. - 25 files: 23 one-line import changes + 2 deletions (23 insertions / 39 deletions). ## Validation - `pnpm --filter studio typecheck` ✅ (confirms no dangling facade/barrel imports anywhere) - `pnpm exec vitest --run state/sql-editor/` ✅ (113 passed) - lint ✅ (0 errors; no ratcheted-rule regressions — a path swap can't add `any`/deps/nested-component violations, and no import-order rule is enforced) - grep confirms zero remaining `sql-editor-v2` references --------- Co-authored-by: supabase-autofix-bot <noreply@supabase.com> |
||
|
Charis
|
7c1ea30e43 |
refactor(studio): extract useSnippetEditor from MonacoEditor (#47500)
## What
PR 7 of the SQL editor state re-layering stack. Extracts the snippet
editing lifecycle out of `MonacoEditor` into a co-located
`useSnippetEditor` hook, and consolidates the edit debounce.
`useSnippetEditor` owns:
- creating the snippet in the store on first edit and routing to its URL
(replace vs push for a `?content=` deep link)
- writing changes back to the store via `setSql` (with
`wasNeverPersisted` → `shouldInvalidate`)
- seeding the editor from the `content` param
- the read-only determination (`canEditSnippet`)
`MonacoEditor` now consumes `{ snippet, disableEdit, handleEditorChange
}` and keeps only the editor shell + Monaco action wiring. It sheds the
`router`/`profile`/`project`/`params`/store/tabs hooks.
`handleEditorChange` was also flattened with an early return.
## Debounce consolidation
Previously there were **two 1s debounces in series**: `useSnippetEditor`
debounced editor changes before writing to the store, and the save
mechanism (`createSaveMechanism`) already debounces persistence. That
added latency (up to ~2s to save) and split the "when to persist" timing
policy across two layers — at odds with PR 5's design where the
scheduler/mechanism owns *when* and dirty state is meant to be
immediate.
This PR removes the editor-side debounce: edits write to the store
synchronously on every change, and the save mechanism's 1s debounce is
the sole throttle. Net effects:
- the store — and the snippet's dirty status — reflects the latest edit
immediately (correct for the future manual-save mode's Save button / nav
guard)
- save fires ~1s after the *last* keystroke instead of up to ~2s
- only the active snippet's own reactive consumers (a lightweight
sidebar item) re-render per keystroke; Monaco is uncontrolled
(`defaultValue`) so it is unaffected
Note: the double-debounce was legacy (the pre-refactor god store had the
same `useDebounce(value, 1000)` in MonacoEditor plus a debounced
module-load subscribe).
## Notes
- Behavior-preserving in outcome — autosave still lands ~1s after typing
stops, just with lower latency and immediate store consistency.
## Validation
- `pnpm --filter studio typecheck` ✅
- `pnpm exec vitest --run state/sql-editor/` ✅ (110 passed)
- lint ✅ (0 errors; no ratcheted-rule regressions)
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* SQL editor changes now apply immediately, with unsaved status
reflected as soon as you edit.
* The editor now keeps the latest snippet details available for saving,
improving reliability when using “Save Query.”
* **Bug Fixes**
* Improved handling for creating and opening snippets from shared links
or prefilled content.
* Fixed status updates so saved snippets correctly switch to unsaved
after edits.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
Charis
|
d153bab849 |
refactor(studio): extract SQL editor session store from god store (#47349)
## What PR 6 of the SQL editor state re-layering stack. Moves ephemeral, never-persisted SQL editor state out of the snippet/folder "god store". **Session store** — `state/sql-editor/sql-editor-session-state.ts` holds per-snippet, read-by-many session state: - query `results` - `explainResults` - the row `limit` …with their mutators (`addResult`/`addResultError`/`resetResult`, `addExplainResult`/`addExplainResultError`/`resetExplainResult`, `resetResults`, `setLimit`). `removeSnippet` drops a snippet's session entries via `clearForSnippet(id)`. **Diff-request slice** — `state/sql-editor/sql-editor-diff-request.ts`. The Assistant's "Insert code" / "Replace code" diff is *not* per-snippet session state: it's a transient, fire-and-forget command produced outside the editor (e.g. query blocks / assistant) and consumed exactly once by whichever editor is active. It's modeled as a consume-once request (`requestDiff` / `consumeDiffRequest`) rather than durable state — the editor drains it on apply, so a stale diff can't leak into a later editor or session. (Previously this was `diffContent` in the god store: never cleared and triggered by object-reference identity.) Consumers read session state from `useSqlEditorSessionSnapshot` and the diff channel from `useSqlEditorDiffRequestSnapshot`, keeping `useSqlEditorV2StateSnapshot` only for snippets/folders. ### Why not the TanStack Query cache for results/explain? Editor execution is a **mutation**, not a keyed query — `mutation.data` is per-hook-instance and not keyed by snippet id, and there's no caching value to capture (re-running SQL must return *fresh* data, never a cached result). `EXPLAIN ANALYZE` actually executes the statement, so a declarative/auto-refetching `useQuery` is semantically wrong. Results/explain are imperative mutation outputs, scoped to the session, read by several decoupled consumers keyed by snippet id — exactly what a small in-memory keyed store models honestly. ## Consumers migrated - `SQLEditor.tsx` — results/explain/limit reads + `addResult`/`addResultError`/`addExplainResult`/`addExplainResultError`/`setLimit`; diff-apply effect now drains a consume-once request - `UtilityPanel.tsx`, `UtilityTabResults.tsx`, `UtilityTabExplain.tsx`, `UtilityActions.tsx` - `QueryBlock/EditQueryButton.tsx` — produces via `requestDiff` ## Notes - Result/explain types are kept verbatim from the god store (pre-existing `any` row/error types come along unchanged; tightening them is out of scope for this move). - `ref()` on result rows is preserved to avoid Valtio proxying large row sets. ## Tests - `sql-editor-session-state.test.ts` — result/explain mutators, `resetResults`, `clearForSnippet`, `limit` - `sql-editor-diff-request.test.ts` — `requestDiff`, `consumeDiffRequest` (drain + queue-of-one) Validation: - `pnpm --filter studio typecheck` ✅ - `pnpm exec vitest --run state/sql-editor/` ✅ (110 passed) - lint ✅ (no new errors) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * SQL editor query results, EXPLAIN output, and the “Limit results to” setting now persist more reliably across a session. * AI-assisted SQL insert/replace actions now use a pending diff workflow to apply updates more consistently. * **Bug Fixes** * Results/EXPLAIN rendering and downloads stay in sync with the latest executed data. * Switching databases/snippets now clears the correct temporary results. * Diff application is more resilient when an editor is still loading, including empty-vs-non-empty editor cases. * **Tests** * Added coverage for the session and diff-request state logic. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Joshen Lim <joshenlimek@gmail.com> |
||
|
Charis
|
5cb81123ae |
refactor(studio): move SQL editor save trigger into a scheduler + provider (5/9) (#47316)
## What
PR 5 of a stacked refactor. Moves *when to save* out of a module-load
`subscribe` and into an injectable **scheduler** armed by a headless
**provider**, splits the save queue, and adds an unsaved-close warning.
### Scheduler (`sql-editor-save-scheduler.ts`)
`createSaveScheduler({ state, saveMechanism, notify, getSaveMode })`
owns the save *policy*:
- **auto** mode drains the dirty snippet queue as edits land; **manual**
mode (the seam for a future opt-in; defaults to `auto`) leaves snippets
queued until `requestSave`. Folder saves always drain.
- `start()` returns an unsubscribe; `requestSave(id)` is the
explicit-save entry.
### Provider (`sql-editor-save-coordinator.tsx`)
Headless `SqlEditorSaveCoordinatorProvider` instantiates the mechanism
(invalidation via the **React Query client from context**, not the
global `getQueryClient`) + scheduler, `start()`s it in an effect
(start/stop with the provider), and exposes `requestSave` via
`useSqlEditorSaveCoordinator()`. Mounted in `ProjectContext` (under the
app's QueryClientProvider). Cmd+S and the SavingIndicator Retry now go
through `requestSave`.
### Queue split
`needsSaving` (snippets) and `pendingFolderSaves` (folders) are separate
queues, drained independently — the old snippet-vs-folder `if/else` is
gone.
### Unsaved-close warning
A `beforeunload` guard triggers the browser's native "Leave site?"
prompt while any snippet's `status !== 'saved'` (failed / in-flight /
never-saved).
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* Improved SQL editor saving with a centralized save flow, including
automatic/manual save handling and immediate “Save Query” requests.
* Added unsaved-change detection so the app can warn before closing or
reloading when edits are still pending.
* **Bug Fixes**
* Retry actions now use the updated save flow for more reliable
re-saving.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
Charis
|
16526bd6bf |
refactor(studio): extract SQL editor save mechanism + model folder lifecycle (4/9) (#47276)
## What
PR 4 of a stacked refactor of the SQL editor snippet/folder state. It
pulls the persistence logic out of the store into an injectable
mechanism, and replaces the folder `'new-folder'` id sentinel with an
explicit lifecycle — plus a concurrency bug fix that surfaced along the
way.
### Save mechanism (`sql-editor-save.ts`)
`createSaveMechanism({ state, upsertContent, createSQLSnippetFolder,
updateSQLSnippetFolder, invalidate, notify, debounceMs })` → `{
saveSnippet, createFolder, updateFolder }`. The store's subscribe now
dispatches to it; *when* to save still lives in the subscribe (the
scheduler/provider move is PR 5). Per-id debounce cache lives in the
factory closure (no module-global leak).
- **`saveSnippet`** reads the live store snippet, guards
`isLoadedSnippet` so a content-less snippet can **never PUT an empty
body** (directly unit-tested), then builds the payload + drives status
transitions + gated invalidation.
- **`toast` is injected** as a `Notifier` (new generic DI contract in
`lib/notifier.ts`) — the mechanism no longer imports sonner.
- **create vs rename are two named-arg functions**, not an `isNew`
branch; rollback is deterministic per operation instead of matching on
`error.message` text.
- **caught errors are `unknown`**, narrowed via the existing
`getErrorMessage` util with a generic fallback — no `any`.
### Folder lifecycle (replaces the `NEW_FOLDER_ID` sentinel)
- **`FolderStatus`** enum (`new_editing | new_saving | editing | saving
| idle`) collapses the persistence and progress axes into one enum —
same pattern as `SnippetStatus` — with `isNewFolder` / `isFolderEditing`
/ `isFolderSaving` predicates. Tagging a folder as new/persisted is now
an explicit field, not an id convention.
- New placeholders get a **unique local id** (`crypto.randomUUID`);
`NEW_FOLDER_ID` is deleted, which also lifts the accidental
one-unsaved-folder-at-a-time limit.
### Bug fix: folder-rename rollback race
The shared `lastUpdatedFolderName` field let two in-flight renames
clobber each other's rollback target (and a shared `finally` could wipe
it). Replaced by a **per-folder `previousName`** on
`StateSnippetFolder`, so concurrent renames of different folders are
isolated. A new test runs two failing renames concurrently and asserts
each restores its own previous name.
## Tests
`sql-editor-save.test.ts` (mechanism — fakes + fake timers, incl.
content-less no-PUT and concurrent-rename isolation) and
folder-lifecycle predicate tests. `pnpm --filter studio typecheck`
clean; 82 state/sql-editor unit tests pass.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* Improved SQL editor folder handling with clearer create, rename, and
save states.
* Added a more consistent notification flow for successful and failed
save actions.
* **Bug Fixes**
* Improved rollback handling when folder renames fail, helping restore
the previous name reliably.
* Updated save behavior to better protect against duplicate or
out-of-order updates.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
Charis
|
d5653f1f92 |
refactor(studio): unify snippet save + persistence into SnippetStatus (3/9) (#47251)
## What PR 3 of a stacked refactor of the SQL editor snippet state. Replaces the two overlapping pieces of snippet lifecycle state — the `savingStates` map (`IDLE|UPDATING|UPDATING_FAILED`) and the `isNotSavedInDatabaseYet` boolean — with a single `SnippetStatus` enum. ## Status is attached at the data layer (never absent) - `SnippetStatus` + `SnippetWithContent` now live in `data/content`. The snippet queries attach `status: 'saved'` via a typed `withSavedStatus()` helper, and `upsertContent` returns `SnippetWithContent` so move/rename responses carry status too. - A SQL-typed `getSqlSnippetById`/`useSqlSnippetByIdQuery` returns `SnippetWithContent` (the generic `useContentIdQuery` stays for Reports, which use it). `[id].tsx` loads content with **no casting**. - `'new'` is attached on local creation (`createSqlSnippetSkeletonV2`). ## Behavior Behavior-preserving for the existing auto-save flow (faithful mapping of both old fields, including the replication-lag swallow). One incidental fix: the read-only/saving indicator now also covers a brand-new snippet's first save (previously only re-saves of persisted snippets had distinct saving/failed states in some paths). ## Tests New `sql-editor-lifecycle.test.ts` (29 tests) covering every predicate and transition; existing rules tests updated. `pnpm --filter studio typecheck` clean; 52 state/sql-editor unit tests pass. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Refactor** * Restructured SQL snippet persistence tracking, replacing boolean flags with a comprehensive status system for clearer visibility into save progress. * Enhanced saving indicator UI to reflect accurate snippet save states. * **Tests** * Added test coverage for snippet persistence state transitions and lifecycle scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Charis
|
e1e2498db0 |
refactor(studio): extract SQL editor domain rules into pure module (2/9) (#47204)
## What PR 2 of a stacked refactor of the SQL editor snippet state. **Stacked on #47203 (PR 1)** — review/merge that first. Extracts scattered business rules + the upsert-payload builder into a new **pure** module `apps/studio/state/sql-editor/sql-editor-rules.ts` (no Valtio, React, toast, or runtime data-layer imports): - `canEditSnippet` — read-only rule (shared snippet you don't own), was inline in `MonacoEditor` `disableEdit` - `isSnippetOwner` — owner check, was inline in `ReadOnlyBadge` / `SavingIndicator` - `validateMoveToFolder` — 'shared snippet cannot be within a folder', was a buried `toast.error` - `buildUpsertPayload` — the PUT /content payload, was an inline object literal (all `??` defaults preserved) - `isLoadedSnippet` — type guard (see below) ## Bug fix: no more empty-content saves (and no non-null assertion) The old payload builder used `{ ...content!, content_id: id }`. Tracing that `!` upstream surfaced a real bug: **favoriting a snippet from the sidebar that had never been opened** enqueued a save with no loaded content, producing a PUT with an empty content body (rejected by API). The requirement that a persisted snippet has loaded content is now enforced **at the type level** rather than by a runtime assertion or comment: - `buildUpsertPayload` accepts only a `LoadedSnippet` (content non-nullable) — the `!` is gone. - the save subscriber crosses that boundary via the `isLoadedSnippet` type guard. - the sidebar favorite toggle loads content first (mirroring `onSelectDuplicate` / the share modals), narrowing the fetched union content to the SQL variant via its discriminant — **no type cast**. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved consistency in read-only behavior and ownership checks across the SQL editor by centralizing permission logic. * Fixed favorite toggle to ensure snippet content is fully loaded before persisting changes. * **Refactor** * Centralized SQL snippet permission rules and validation logic into a dedicated helper module. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Charis
|
2b24065c7b |
refactor(studio): relocate SQL editor store into state/sql-editor/ with facade (1/9) (#47203)
## What PR 1 of a stacked refactor that re-layers the SQL editor snippet state (`apps/studio/state/sql-editor-v2.ts`). This first PR is a **pure structural move with zero behavior change** — no consumer files are touched. - Relocates the Valtio store body into `apps/studio/state/sql-editor/sql-editor-state.ts` - Extracts the type declarations into `apps/studio/state/sql-editor/types.ts` - Adds `apps/studio/state/sql-editor/index.ts` as the public surface - Keeps the old `apps/studio/state/sql-editor-v2.ts` path as a thin re-export **facade**, so all existing importers keep working unchanged ## How to read the diff `sql-editor/sql-editor-state.ts` (~507 lines) is the **verbatim relocation** of the former `sql-editor-v2.ts` body — not new code. Git does not show it as a rename because the old path is intentionally retained as the facade. The only genuinely new lines are `types.ts` (20), `index.ts` (8), and the facade itself (8). ## Why The store has accreted four tangled responsibilities (snippet/folder CRUD, query results, persistence, Assistant diff). The stack incrementally splits these into pure rules, a persistent store, a session store, and an injectable save mechanism whose trigger is a swappable policy (setting up a future auto→manual save migration). Each PR stays ≤300–400 non-test lines and behavior-preserving. ## Verification - `pnpm --filter studio typecheck` passes (only pre-existing unrelated module-resolution errors remain). - Lint passes (no new errors). - No consumer imports changed. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Restructured SQL editor state management into a modular architecture with improved separation of concerns and enhanced code organization. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |