From 306af140bd7308d3526da2c5dc529016e24bf5e4 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Thu, 3 Sep 2026 11:48:56 +0200 Subject: [PATCH] Revert "fix: stop counting empty schema-added fields and server metadata as drafts" This reverts commit 9787270ad883722735d3873ee98d37a7fd11ad23. --- frontend/src/lib/userDraft.svelte.ts | 83 +++-------------------- frontend/src/lib/userDraftCompare.test.ts | 63 ----------------- frontend/src/lib/userDraftPrune.test.ts | 9 --- 3 files changed, 10 insertions(+), 145 deletions(-) delete mode 100644 frontend/src/lib/userDraftCompare.test.ts diff --git a/frontend/src/lib/userDraft.svelte.ts b/frontend/src/lib/userDraft.svelte.ts index 0a0c9ba4dd..5d924a5d49 100644 --- a/frontend/src/lib/userDraft.svelte.ts +++ b/frontend/src/lib/userDraft.svelte.ts @@ -200,15 +200,13 @@ function snapshotDraftValue(value: V | undefined): V | undefined { * * The rest are server-managed read-time metadata that ride along on the * loaded deployed payload but never appear in the editor's draft content, so - * comparing them would mask a true baseline match: `draft_saved_at` (the - * draft's own save time), `edited_at` (deploy time), `edited_by` (deploy - * author), `workspace_id`, `version_id` (deployed version), `is_draft` - * (backend presence flag), `assets` (server-derived on deploy from the - * content — the editor's draft value never carries it, so a deployed - * baseline's `assets: []` would otherwise never equal the assets-less draft - * and every untouched load would autosave a phantom draft), and the groups - * below. A field belongs here only if no editor lets anyone change it — - * listing an editable one makes a real edit invisible. + * comparing them would mask a true baseline match: + * `draft_saved_at` (the draft's own save time), `edited_at` (deploy time), + * `edited_by` (deploy author), `workspace_id`, `version_id` (deployed version), + * `is_draft` (backend presence flag), and `assets` (server-derived on deploy + * from the content — the editor's draft value never carries it, so a deployed + * baseline's `assets: []` would otherwise never equal the assets-less draft and + * every untouched load would autosave a phantom draft). */ const DRAFT_COMPARE_IGNORED_FIELDS = [ 'permissioned_as', @@ -222,76 +220,16 @@ const DRAFT_COMPARE_IGNORED_FIELDS = [ 'parent_version', 'is_draft', 'assets', - // Authorship and deploy identity, assigned by the server. - 'created_by', - 'created_at', - 'hash', - 'parent_hashes', - 'lock_error_logs', - // Presence and lifecycle flags the backend computes per read. - 'draft_only', - 'no_deployed', - 'starred', - 'archived', - 'deleted', - // Computed at read time from the parent folder; edited on the folder, not - // here. `labels` itself IS editable and stays compared. - 'inherited_labels', // Fixed at creation and absent from the resource editor's draft shape, so // it only ever shows up on the deployed side of a comparison. - 'resource_type', - // A resource's OAuth/linked-secret state: owned by the OAuth flow and the - // variable it points at, never by the resource form. - 'is_oauth', - 'is_linked', - 'is_refreshed', - 'is_expired', - 'refresh_error', - 'account', - // A schedule's last-trigger error and its owner's email, both written by - // the scheduler rather than the editor. - 'error', - 'email' + 'resource_type' ] as const -/** - * Whether a value means "nothing is set here". A key holding one is dropped - * below, so a key that only exists because a schema gained a property — the - * form materializes `''`, `[]`, `{}` or `false` into every item that never - * carried it — compares equal to no key at all. - * - * `0` is deliberately NOT empty: nothing materializes it, and it is a value - * someone picks (a timeout, a retry count). `false` is, because the only way - * an unchecked box differs from an absent key is that a form drew it. - */ -function isEmptyDraftValue(v: unknown): boolean { - if (v === undefined || v === null || v === '' || v === false) return true - if (Array.isArray(v)) return v.length === 0 - return typeof v === 'object' && Object.keys(v as object).length === 0 -} - -/** Drop empty-valued keys at every depth, innermost first so a branch that - * empties out goes with them. Mutates — only ever called on a fresh parse. */ -function dropEmptyKeys(v: unknown): void { - if (Array.isArray(v)) { - // Never drop elements: that would shift every index after them. - for (const e of v) dropEmptyKeys(e) - return - } - if (v === null || typeof v !== 'object') return - for (const [k, child] of Object.entries(v as Record)) { - dropEmptyKeys(child) - if (isEmptyDraftValue(child)) delete (v as Record)[k] - } -} - /** * Normalize one side of a draft-vs-baseline comparison: JSON round-trip * (drafts are stored as json server-side, which strips `undefined` keys, - * so `{ labels: undefined }` and `{}` must compare equal), drop the ignored - * fields above — top level only, so an item's own content may still use those - * names — and drop empty-valued keys at every depth. Returns the input - * unchanged if unserializable. + * so `{ labels: undefined }` and `{}` must compare equal) and drop the + * ignored fields above. Returns the input unchanged if unserializable. */ export function normalizeDraftForCompare(value: V): V { try { @@ -299,7 +237,6 @@ export function normalizeDraftForCompare(value: V): V { if (v !== null && typeof v === 'object' && !Array.isArray(v)) { for (const f of DRAFT_COMPARE_IGNORED_FIELDS) delete v[f] } - dropEmptyKeys(v) return v as V } catch { return value diff --git a/frontend/src/lib/userDraftCompare.test.ts b/frontend/src/lib/userDraftCompare.test.ts deleted file mode 100644 index c03c1d719b..0000000000 --- a/frontend/src/lib/userDraftCompare.test.ts +++ /dev/null @@ -1,63 +0,0 @@ -import { describe, it, expect, vi } from 'vitest' - -// Importing the module pulls in the generated client and the syncer's -// localStorage / pagehide wiring; stub the same surfaces the syncer tests do -// so the pure-function import stays side-effect free. -vi.mock('./gen', () => ({ DraftService: { updateDraft: vi.fn() } })) -vi.mock('./gen/core/OpenAPI', () => ({ OpenAPI: { BASE: '' } })) -vi.mock('./userDraftDbSyncer.svelte', () => ({ UserDraftDbSyncer: { save: vi.fn() } })) - -import { draftValuesEqual } from './userDraft.svelte' - -describe('draftValuesEqual', () => { - it('ignores a key the schema added but nobody filled in', () => { - const deployed = { args: { host: 'example.com' } } - const settled = { args: { host: 'example.com', port: '', ssl: false, tags: [] } } - expect(draftValuesEqual(settled, deployed)).toBe(true) - }) - - it('still sees the same key once it holds something', () => { - const deployed = { args: { host: 'example.com' } } - expect(draftValuesEqual({ args: { host: 'example.com', port: '5432' } }, deployed)).toBe(false) - }) - - it('sees a value being cleared, because the baseline had one', () => { - expect(draftValuesEqual({ args: { host: '' } }, { args: { host: 'example.com' } })).toBe(false) - }) - - it('treats 0 as a real value, not as empty', () => { - expect(draftValuesEqual({ timeout: 0 }, {})).toBe(false) - }) - - it('drops a nested branch that empties out entirely', () => { - expect(draftValuesEqual({ args: { auth: { token: '' } } }, {})).toBe(true) - }) - - it('keeps array positions when an element empties out', () => { - expect(draftValuesEqual({ xs: [{ a: '' }, { a: 'x' }] }, { xs: [{}, { a: 'x' }] })).toBe(true) - expect(draftValuesEqual({ xs: [{ a: 'x' }, { a: '' }] }, { xs: [{}, { a: 'x' }] })).toBe(false) - }) - - it('ignores server-managed metadata riding along on the deployed payload', () => { - const deployed = { - path: 'u/me/r', - value: { host: 'h' }, - created_by: 'admin', - created_at: '2026-01-01T00:00:00Z', - is_oauth: false, - inherited_labels: ['from-folder'], - starred: true - } - expect(draftValuesEqual({ path: 'u/me/r', value: { host: 'h' } }, deployed)).toBe(true) - }) - - it('still compares labels, which are edited here', () => { - expect(draftValuesEqual({ labels: ['a'] }, { labels: ['b'] })).toBe(false) - }) - - it('only ignores metadata names at the top level', () => { - const a = { value: { created_by: 'someone' } } - const b = { value: { created_by: 'someone else' } } - expect(draftValuesEqual(a, b)).toBe(false) - }) -}) diff --git a/frontend/src/lib/userDraftPrune.test.ts b/frontend/src/lib/userDraftPrune.test.ts index 1fd260c9cc..38507d5f48 100644 --- a/frontend/src/lib/userDraftPrune.test.ts +++ b/frontend/src/lib/userDraftPrune.test.ts @@ -60,15 +60,6 @@ describe('pruneMeaninglessDrafts', () => { expect(discardDraft).not.toHaveBeenCalled() }) - it('ignores the empty fields a moved-on schema added', async () => { - listDrafts.mockResolvedValue([row()]) - getDraftDiffValues.mockResolvedValue( - diff({ draft: { value: { host: 'h', port: '', ssl: false, tags: [] } } }) - ) - await pruneMeaninglessDrafts('main', 'me@x.dev') - expect(discardedPaths()).toEqual(['u/me/r']) - }) - it('never touches a draft-only item — the draft is the whole item', async () => { listDrafts.mockResolvedValue([row({ draft_only: true })]) getDraftDiffValues.mockResolvedValue(diff())