diff --git a/TESTING.md b/TESTING.md index 83f9eb0..65e9f6b 100644 --- a/TESTING.md +++ b/TESTING.md @@ -441,6 +441,18 @@ Recorded shell deviations: `#/saved/`. Opening one shows its page with the saved toolbar value and Refine filters, and the view's name as the h1, breadcrumb and title. A value its page no longer accepts (a removed Source or Folder) stays default; an unknown id opens Images. + A toolbar change made while the sources are still loading keeps the saved Source and + Folder until discovery settles. A saved view, like Favorites, never replaces the plain + Images or Library return state. Writes (POST, PATCH) validate strictly; a state over + 4 KB is refused in the browser with "The view is too large to save. Shorten its + search, folder or tags and try again." Startup and restore check only the table's + structure and that each row's state is a JSON object of at most 4 KB, never the value + rules, so a row saved under older rules (a missing or retired key, a sort no longer + offered) never stops the database from opening: reads fill missing keys with their + defaults, drop unknown keys, default invalid values and make the name displayable + (`tests/test_saved_views.py`). Only request validation is a 400; any other failure is + a logged 500 with a generic message. The Favorites count re-read after starring is + coalesced to one read 300 ms after the last toggle. `a saved view row matches the concept's` saves one view in both documents and compares the rows; `tests/browser/workspace-saved-views.spec.mjs` covers saving, reopening, the defaults that replace rejected values, and failed reads and saves. @@ -461,7 +473,8 @@ Recorded shell deviations: saved view's heading leads with **Edit**: "Edit saved view" with "View name" and Delete… (danger), Cancel and Save. Save re-reads the list, retitles the page and toasts "Saved view renamed."; an empty name keeps the dialog open. Delete… swaps the - same dialog to "Delete saved view" (Cancel / Delete; never `window.confirm`); Delete + same dialog to "Delete saved view" (Cancel / Delete; never `window.confirm`; its Cancel + closes the dialog without deleting rather than returning to Edit); Delete removes the view, replaces the dead route with the view's plain page and toasts "Saved view deleted.". Failures toast "The view could not be renamed. Try again." or "The view could not be deleted. Try again."; a view another tab already deleted counts diff --git a/database/saved_views.py b/database/saved_views.py index 4df3d3e..5612add 100644 --- a/database/saved_views.py +++ b/database/saved_views.py @@ -2,8 +2,13 @@ A view's state holds exactly its page's toolbar values and its Refine-this-view refinements, whitelisted per page and stored as compact JSON of at most 4 KB. -The same validation guards writes and restores. Optional in old backups, -required once a database is current (see ``library_schema.py``). + +Writes (the API's POST and PATCH) validate strictly. Startup and restore check +only the structure (the table, its columns and each row's types, sizes and JSON +object), never the value rules: a row saved under older rules must not stop the +database from opening. Reads normalize instead, so a stale row opens with its +usable values and defaults for the rest. Optional in old backups, required once +a database is current (see ``library_schema.py``). """ import json @@ -24,25 +29,32 @@ _LIBRARY_SORTS = ("newest", "oldest", "name", "rating") _MEDIA = ("all", "image", "video") _CONTROL = re.compile(r"[\x00-\x1f\x7f]") _SOURCE = re.compile(r"all|0|[1-9][0-9]{0,8}") +FALLBACK_NAME = "Saved view" -# (kind, limit-or-choices) per state key; "text" values are length-capped. + +class SavedViewInvalid(ValueError): + """A request's page, name or state breaks the write rules (a 400).""" + + +# (kind, limit-or-choices, default) per state key; "text" values are +# length-capped. The defaults match the client's (saved-views.mjs FIELDS). _STATE_FIELDS = { "images": { - "q": ("text", 500), - "category": ("text", 100), - "sort": ("choice", _IMAGE_SORTS), - "source": ("source", None), - "folder": ("text", 1024), - "media": ("choice", _MEDIA), - "tags": ("tags", None), + "q": ("text", 500, ""), + "category": ("text", 100, ""), + "sort": ("choice", _IMAGE_SORTS, "newest"), + "source": ("source", None, "all"), + "folder": ("text", 1024, ""), + "media": ("choice", _MEDIA, "all"), + "tags": ("tags", None, ()), # The Favorites page's filter (#249 D51): a view saved there reopens it. - "favorites": ("bool", None), + "favorites": ("bool", None, False), }, "library": { - "q": ("text", 500), - "category": ("text", 100), - "sort": ("choice", _LIBRARY_SORTS), - "tags": ("tags", None), + "q": ("text", 500, ""), + "category": ("text", 100, ""), + "sort": ("choice", _LIBRARY_SORTS, "newest"), + "tags": ("tags", None, ()), }, } @@ -71,19 +83,22 @@ def _compact(sql: str) -> str: def validate_name(name: object) -> str: """The trimmed name, 1 to 100 characters without control characters.""" if not isinstance(name, str): - raise ValueError("Name must be a string") + raise SavedViewInvalid("Name must be a string") trimmed = name.strip() if not 1 <= len(trimmed) <= MAX_NAME_LENGTH: - raise ValueError(f"Name must be 1 to {MAX_NAME_LENGTH} characters") + raise SavedViewInvalid(f"Name must be 1 to {MAX_NAME_LENGTH} characters") if _CONTROL.search(trimmed): - raise ValueError("Name contains control characters") + raise SavedViewInvalid("Name contains control characters") return trimmed def _validate_value(key: str, kind: str, limit, value: object) -> None: + """Raise ``SavedViewInvalid`` when *value* breaks *key*'s write rule.""" if kind == "tags": if not isinstance(value, list) or len(value) > MAX_TAGS: - raise ValueError(f"State '{key}' must be a list of at most {MAX_TAGS}") + raise SavedViewInvalid( + f"State '{key}' must be a list of at most {MAX_TAGS}" + ) for tag in value: if ( not isinstance(tag, str) @@ -92,61 +107,93 @@ def _validate_value(key: str, kind: str, limit, value: object) -> None: or "," in tag or _CONTROL.search(tag) ): - raise ValueError(f"State '{key}' holds an invalid tag") + raise SavedViewInvalid(f"State '{key}' holds an invalid tag") return if kind == "bool": if not isinstance(value, bool): - raise ValueError(f"State '{key}' must be true or false") + raise SavedViewInvalid(f"State '{key}' must be true or false") return if not isinstance(value, str): - raise ValueError(f"State '{key}' must be a string") + raise SavedViewInvalid(f"State '{key}' must be a string") if kind == "choice" and value not in limit: - raise ValueError(f"State '{key}' is not a supported value") + raise SavedViewInvalid(f"State '{key}' is not a supported value") if kind == "source" and not _SOURCE.fullmatch(value): - raise ValueError(f"State '{key}' must be 'all' or a source index") + raise SavedViewInvalid(f"State '{key}' must be 'all' or a source index") if kind == "text" and (len(value) > limit or _CONTROL.search(value)): - raise ValueError(f"State '{key}' is too long or has control characters") + raise SavedViewInvalid(f"State '{key}' is too long or has control characters") def encode_state(page: object, state: object) -> str: """Validate *state* for *page* and return its compact JSON (at most 4 KB).""" if page not in PAGES: - raise ValueError("Page must be 'images' or 'library'") + raise SavedViewInvalid("Page must be 'images' or 'library'") fields = _STATE_FIELDS[page] if not isinstance(state, dict): - raise ValueError("State must be an object") + raise SavedViewInvalid("State must be an object") if set(state) != set(fields): - raise ValueError( + raise SavedViewInvalid( f"State for {page} must contain exactly " + ", ".join(sorted(fields)) ) - for key, (kind, limit) in fields.items(): + for key, (kind, limit, _default) in fields.items(): _validate_value(key, kind, limit, state[key]) encoded = json.dumps( {key: state[key] for key in fields}, ensure_ascii=False, separators=(",", ":") ) if len(encoded.encode("utf-8")) > MAX_STATE_BYTES: - raise ValueError("State must encode to at most 4 KB") + raise SavedViewInvalid("State must encode to at most 4 KB") return encoded -def _row_error(page, name, state) -> bool: - """True when a persisted row would not pass the write validation.""" +def _decode_object(raw: object) -> Optional[dict]: + """The JSON object stored in *raw*, or None when it holds no object.""" + if isinstance(raw, dict): + return raw + if not isinstance(raw, str): + return None try: - if validate_name(name) != name: - return True - decoded = json.loads(state) - return encode_state(page, decoded) != state - except (ValueError, TypeError): - return True + decoded = json.loads(raw) + except (ValueError, RecursionError): + return None + return decoded if isinstance(decoded, dict) else None + + +def normalize_state(page: str, raw: object) -> dict: + """The page's full state from a stored row, never raising. + + Every key of the page's current whitelist is present: a stored value that + passes today's write rule is kept, anything else (missing, invalid, or a + row that is not a JSON object) takes the key's default, and keys the + whitelist no longer has are dropped. An unknown page has no keys. + """ + fields = _STATE_FIELDS.get(page, {}) + stored = _decode_object(raw) or {} + state = {} + for key, (kind, limit, default) in fields.items(): + value = stored.get(key, default) + try: + _validate_value(key, kind, limit, value) + except SavedViewInvalid: + value = default + state[key] = list(value) if kind == "tags" else value + return state + + +def normalize_name(name: object) -> str: + """A stored name made displayable: trimmed, capped, never empty.""" + text = _CONTROL.sub("", name) if isinstance(name, str) else "" + trimmed = text.strip()[:MAX_NAME_LENGTH].strip() + return trimmed or FALLBACK_NAME def saved_views_schema_error( conn: sqlite3.Connection, *, allow_missing: bool = True ) -> Optional[str]: - """Reject unsupported saved-views schemas and invalid persisted values. + """Reject unsupported saved-views schemas and structurally invalid rows. - Reads only. A missing table is acceptable for backups that predate it - unless ``allow_missing`` is false. + Reads only. Checks the table, its columns, each row's types and sizes and + that each state is a JSON object; never the value rules, which change + between versions and are normalized on read instead. A missing table is + acceptable for backups that predate it unless ``allow_missing`` is false. """ objects = conn.execute( "SELECT type,name,sql FROM sqlite_master WHERE name=? " @@ -169,6 +216,7 @@ def saved_views_schema_error( # Literal SQL: the table name and limits are constants of this module. if conn.execute( "SELECT 1 FROM saved_views WHERE typeof(page) != 'text' " + "OR page NOT IN ('images','library') " "OR typeof(name) != 'text' OR typeof(state) != 'text' " "OR typeof(position) != 'integer' OR typeof(created_at) != 'text' " "OR typeof(updated_at) != 'text' " @@ -178,8 +226,8 @@ def saved_views_schema_error( "OR length(CAST(updated_at AS BLOB)) > 64 LIMIT 1" ).fetchone(): return "Saved views contains invalid values" - for page, name, state in conn.execute("SELECT page, name, state FROM saved_views"): - if _row_error(page, name, state): + for (state,) in conn.execute("SELECT state FROM saved_views"): + if _decode_object(state) is None: return "Saved views contains invalid values" return None @@ -204,7 +252,12 @@ def migrate_saved_views(conn: sqlite3.Connection) -> None: def _view(row) -> dict: view_id, page, name, state = row - return {"id": view_id, "page": page, "name": name, "state": json.loads(state)} + return { + "id": view_id, + "page": page, + "name": normalize_name(name), + "state": normalize_state(page, state), + } class SavedViewOperations: diff --git a/py/api/saved_views.py b/py/api/saved_views.py index 5000fd5..66485e0 100644 --- a/py/api/saved_views.py +++ b/py/api/saved_views.py @@ -1,7 +1,10 @@ """Saved views (D16): list, create, rename and delete named toolbar states. The page, the name and the whitelisted per-page state are validated here -(through ``database/saved_views.py``) before anything is written. +(through ``database/saved_views.py``) before anything is written. Only that +request validation (``SavedViewInvalid``) is a 400 carrying its message; any +other failure, including a ``ValueError`` raised by the database layer, is a +logged 500 with a generic message. """ from functools import wraps @@ -10,9 +13,9 @@ from typing import Callable from aiohttp import web try: - from ...database.saved_views import encode_state, validate_name + from ...database.saved_views import SavedViewInvalid, encode_state, validate_name except ImportError: - from database.saved_views import encode_state, validate_name + from database.saved_views import SavedViewInvalid, encode_state, validate_name _CREATE_FIELDS = frozenset({"page", "name", "state"}) _RENAME_FIELDS = frozenset({"name"}) @@ -34,7 +37,7 @@ def saved_view_response(handler: Callable) -> Callable: return web.json_response( {"success": False, "error": "Saved view not found"}, status=404 ) - except (ValueError, TypeError) as exc: + except SavedViewInvalid as exc: return web.json_response({"success": False, "error": str(exc)}, status=400) except Exception: self.logger.exception("Saved view operation failed") @@ -47,21 +50,26 @@ def saved_view_response(handler: Callable) -> Callable: async def _body(request: web.Request, fields: frozenset) -> dict: """A JSON object whose keys are exactly ``fields``.""" - body = await request.json() + try: + body = await request.json() + except (ValueError, UnicodeDecodeError): # JSONDecodeError is a ValueError + raise SavedViewInvalid("Expected a JSON object") from None if not isinstance(body, dict): - raise ValueError("Expected a JSON object") + raise SavedViewInvalid("Expected a JSON object") if set(body) != fields: - raise ValueError("Request must contain exactly " + ", ".join(sorted(fields))) + raise SavedViewInvalid( + "Request must contain exactly " + ", ".join(sorted(fields)) + ) return body def _view_id(request: web.Request) -> int: raw = request.match_info["view_id"] if not raw.isascii() or not raw.isdigit(): - raise ValueError("Invalid saved view ID") + raise SavedViewInvalid("Invalid saved view ID") value = int(raw) if not 1 <= value <= _MAX_ID: - raise ValueError("Invalid saved view ID") + raise SavedViewInvalid("Invalid saved view ID") return value diff --git a/tests/browser/server.py b/tests/browser/server.py index 0428ac6..91895c9 100644 --- a/tests/browser/server.py +++ b/tests/browser/server.py @@ -470,6 +470,29 @@ def create_app(root: Path) -> web.Application: ) return web.json_response({"success": True, **manifest}) + @routes.post("/__test__/saved-views/raw") + async def insert_raw_saved_view(request: web.Request) -> web.Response: + """Store a view row as an older build wrote it, bypassing the API's rules.""" + data = await request.json() + if ( + not isinstance(data, dict) + or set(data) != {"page", "name", "state"} + or not all(isinstance(data[key], str) for key in data) + ): + raise web.HTTPBadRequest() + + def insert() -> int: + with api.db.model.get_connection() as conn: + return conn.execute( + "INSERT INTO saved_views (page, name, state, position, " + "created_at, updated_at) VALUES (?, ?, ?, 0, 'x', 'x')", + (data["page"], data["name"], data["state"]), + ).lastrowid + + return web.json_response( + {"success": True, "id": await api._run_in_executor(insert)} + ) + @routes.post("/__test__/add-nested-image") async def add_nested_image(request: web.Request) -> web.Response: destination = root / "output" / "project" / "day" / "new-output.png" diff --git a/tests/browser/workspace-saved-views.spec.mjs b/tests/browser/workspace-saved-views.spec.mjs index 7a1f1e9..8525bfa 100644 --- a/tests/browser/workspace-saved-views.spec.mjs +++ b/tests/browser/workspace-saved-views.spec.mjs @@ -196,6 +196,75 @@ test('a saved value the page no longer accepts stays default while the rest appl await expect(imageCards(page)).toHaveAttribute('data-image-key', '1:shared/same.png'); }); +test('a view stored under older rules still lists and opens with its usable values (#249 I1)', async ({ page, request }) => { + // No favorites key, a retired key and a sort that is no longer offered, as an older build stored it. + const raw = async data => { + const response = await request.post('/__test__/saved-views/raw', { data }); + await expect(response).toBeOK(); + return (await response.json()).id; + }; + const imagesId = await raw({ page: 'images', name: 'Old cats', state: JSON.stringify({ q: 'cat', sort: 'shuffle', layout: 'grid' }) }); + await raw({ page: 'library', name: '', state: JSON.stringify({ q: 'dog', sort: 'name-desc' }) }); + expect(await listed(request)).toEqual([ + { id: imagesId, page: 'images', name: 'Old cats', state: { ...IMAGE_DEFAULTS, q: 'cat' } }, + expect.objectContaining({ page: 'library', name: 'Saved view', state: { q: 'dog', category: '', sort: 'newest', tags: [] } }), + ]); + await page.goto(`${workspace}#/saved/${imagesId}`); + await expect(heading(page)).toHaveText('Old cats'); + await expect(viewRows(page)).toHaveText(['Old cats', 'Saved view']); + await expect(search(page)).toHaveValue('cat'); + await expect(sortBy(page)).toHaveValue('newest'); + await expect(imageCards(page)).toHaveCount(1); + await savedNav(page).getByRole('link', { name: 'Saved view', exact: true }).click(); + await expect(heading(page)).toHaveText('Saved view'); + await expect(search(page)).toHaveValue('dog'); + await expect(promptCards(page)).toHaveCount(1); +}); + +test('leaving a saved view returns Images and the Library to their own last state (#249 M2)', async ({ page, request }) => { + const dogs = await create(request, { page: 'images', name: 'Dogs', state: { ...IMAGE_DEFAULTS, q: 'dogs' } }); + const cats = await create(request, { page: 'library', name: 'Cats', state: { q: 'cat', category: '', sort: 'newest', tags: [] } }); + await page.goto(`${workspace}#/images`); + await search(page).fill('cat'); + await expect(imageCards(page)).toHaveCount(1); + await viewRows(page).filter({ hasText: 'Dogs' }).click(); + await expect(heading(page)).toHaveText('Dogs'); + await expect(search(page)).toHaveValue('dogs'); + await page.locator('#primary-nav a[data-route="images"]').click(); + await expect(heading(page)).toHaveText('Images'); + await expect(search(page)).toHaveValue('cat'); + await expect(imageCards(page)).toHaveCount(1); + + await page.locator('#primary-nav a[data-route="library"]').click(); + await expect(promptCards(page)).toHaveCount(2); + await search(page).fill('dog'); + await expect(promptCards(page)).toHaveCount(1); + await page.goto(`${workspace}#/saved/${cats.id}`); + await expect(heading(page)).toHaveText('Cats'); + await expect(search(page)).toHaveValue('cat'); + await page.locator('#primary-nav a[data-route="library"]').click(); + await expect(heading(page)).toHaveText('Prompt library'); + await expect(search(page)).toHaveValue('dog'); + await expect(promptCards(page)).toHaveCount(1); + expect(dogs.id).not.toBe(cats.id); +}); + +test('a toolbar change while sources load keeps the saved Source and Folder (#249 M3)', async ({ page, request }) => { + const view = await create(request, { page: 'images', name: 'Input shared', state: { ...IMAGE_DEFAULTS, source: '1', folder: 'shared' } }); + let release; + const held = new Promise(resolve => { release = resolve; }); + await page.route('**/prompt_manager/gallery/subfolders?**', async route => { await held; await route.fallback(); }); + await page.goto(`${workspace}#/saved/${view.id}`); + await expect(heading(page)).toHaveText('Input shared'); + await sortBy(page).selectOption('name'); + await expect(imageCards(page)).toHaveCount(0); + release(); + await expect(filtersButton(page)).toHaveText('Filters · 2'); + await expect(sortBy(page)).toHaveValue('name'); + await expect(imageCards(page)).toHaveCount(1); + await expect(imageCards(page)).toHaveAttribute('data-image-key', '1:shared/same.png'); +}); + test('an unknown saved view opens Images, and a failed listing offers Retry', async ({ page, request }) => { await page.goto(`${workspace}#/saved/999`); await expect(page).toHaveURL(/#\/images$/); diff --git a/tests/js/workspace-saved-views.test.js b/tests/js/workspace-saved-views.test.js index 3d45868..79f530a 100644 --- a/tests/js/workspace-saved-views.test.js +++ b/tests/js/workspace-saved-views.test.js @@ -187,3 +187,16 @@ test('renaming and deleting a view re-read the list, whatever the outcome', asyn assert.throws(() => store.rename(0, 'X'), TypeError); assert.throws(() => store.remove('9'), TypeError); }); + +test('a state over the API 4 KB cap is refused before anything is sent (#249 M5)', async () => { + const { createSavedViews, stateFits, SavedViewTooLarge, MAX_STATE_BYTES } = await source; + assert.equal(MAX_STATE_BYTES, 4096); + assert.equal(stateFits(IMAGE_DEFAULTS), true); + // Each value is within its own limit; their multibyte UTF-8 together passes 4 KB. + const big = { ...IMAGE_DEFAULTS, q: 'é'.repeat(500), folder: 'é'.repeat(1024), tags: Array.from({ length: 20 }, () => 'é'.repeat(50)) }; + assert.equal(stateFits(big), false); + const { api } = fakeApi(); + const store = createSavedViews(api); + await assert.rejects(store.create('images', 'Big', big), error => error instanceof SavedViewTooLarge && error instanceof RangeError); + assert.deepEqual(api.calls, []); +}); diff --git a/tests/test_saved_views.py b/tests/test_saved_views.py index a0a4e7e..6a67ee1 100644 --- a/tests/test_saved_views.py +++ b/tests/test_saved_views.py @@ -20,8 +20,12 @@ from database.operations import PromptDatabase from database.saved_views import ( MAX_STATE_BYTES, TABLE, + SavedViewInvalid, + encode_state, migrate_saved_views, + normalize_state, saved_views_schema_error, + validate_name, ) from utils.hashing import generate_prompt_hash @@ -38,6 +42,31 @@ IMAGES_STATE = { "favorites": False, } LIBRARY_STATE = {"q": "", "category": "", "sort": "rating", "tags": []} +IMAGES_DEFAULTS = { + "q": "", + "category": "", + "sort": "newest", + "source": "all", + "folder": "", + "media": "all", + "tags": [], + "favorites": False, +} +LIBRARY_DEFAULTS = {"q": "", "category": "", "sort": "newest", "tags": []} +# A row saved by an older build: no favorites key, a retired key, a sort +# that is no longer offered and an over-long folder. +OLD_IMAGES_STATE = json.dumps( + { + "q": "sunset", + "category": "", + "sort": "shuffle", + "source": "all", + "folder": "x" * 2000, + "media": "image", + "tags": ["golden"], + "layout": "grid", + } +) class TestSavedViewsApi(ImageAPITestCase): @@ -228,6 +257,79 @@ class TestSavedViewsApi(ImageAPITestCase): data, {"success": False, "error": "Saved view operation failed"} ) + async def test_internal_value_errors_are_a_generic_500(self): + # Only the request's own validation is a 400; a ValueError or + # TypeError raised by the database layer is a server fault (M4). + for error in ( + ValueError("secret parser detail"), + TypeError("secret type detail"), + json.JSONDecodeError("secret json detail", "{", 0), + ): + + def broken(*_args, error=error): + raise error + + for method, path, body, attr in ( + ("GET", "", None, "list_saved_views"), + ( + "POST", + "", + {"page": "library", "name": "L", "state": LIBRARY_STATE}, + "create_saved_view", + ), + ("PATCH", "/1", {"name": "X"}, "rename_saved_view"), + ("DELETE", "/1", None, "delete_saved_view"), + ): + with self.subTest(error=type(error).__name__, method=method): + setattr(self.api.db, attr, broken) + try: + with self.assertLogs("prompt_manager.api", level="ERROR"): + response, data = await self.send(method, path, body) + finally: + delattr(self.api.db, attr) + self.assertEqual(response.status, 500) + self.assertEqual( + data, + {"success": False, "error": "Saved view operation failed"}, + ) + + async def test_a_row_saved_under_older_rules_lists_normalized(self): + stale = json.dumps( + { + "q": "cat", + "sort": "random", + "media": "video", + "tags": ["ok", 7], + "retired": True, + } + ) + with self.api.db.model.get_connection() as conn: + conn.execute( + f"INSERT INTO {TABLE} (page, name, state, position, created_at, " + "updated_at) VALUES ('images', ' Old\tview ', ?, 0, 'x', 'x')", + (stale,), + ) + conn.execute( + f"INSERT INTO {TABLE} (page, name, state, position, created_at, " + "updated_at) VALUES ('library', '', '{}', 1, 'x', 'x')" + ) + response, data = await self.send("GET") + self.assertEqual(response.status, 200, data) + images, library = data["views"] + self.assertEqual(images["name"], "Oldview") + self.assertEqual( + images["state"], + {**IMAGES_DEFAULTS, "q": "cat", "media": "video"}, + ) + self.assertEqual(library["name"], "Saved view") + self.assertEqual(library["state"], LIBRARY_DEFAULTS) + # A stale view still renames and deletes. + response, data = await self.send( + "PATCH", f"/{images['id']}", {"name": "Renamed"} + ) + self.assertEqual(response.status, 200, data) + self.assertEqual(data["view"]["state"], images["state"]) + class TestSavedViewsSchema(unittest.TestCase): def setUp(self): @@ -334,7 +436,6 @@ class TestSavedViewsSchema(unittest.TestCase): return src def test_restore_rejects_tampered_saved_views(self): - bad_state = json.dumps({**IMAGES_STATE, "unknown": 1}) cases = { "definition": ( f"DROP TABLE {TABLE}", @@ -347,12 +448,11 @@ class TestSavedViewsSchema(unittest.TestCase): "PRAGMA ignore_check_constraints = ON", f"UPDATE {TABLE} SET page = 'history'", ), - "name": (f"UPDATE {TABLE} SET name = ''",), - "long name": (f"UPDATE {TABLE} SET name = '{'x' * 101}'",), - "untrimmed": (f"UPDATE {TABLE} SET name = ' Cats'",), + "long name": (f"UPDATE {TABLE} SET name = '{'x' * 401}'",), "blob name": (f"UPDATE {TABLE} SET name = CAST('Cats' AS BLOB)",), "state json": (f"UPDATE {TABLE} SET state = '{{'",), - "state keys": (f"UPDATE {TABLE} SET state = '{bad_state}'",), + "state array": (f"UPDATE {TABLE} SET state = '[]'",), + "state string": (f"UPDATE {TABLE} SET state = '\"q\"'",), "state size": ( f"UPDATE {TABLE} SET state = '{'x' * (MAX_STATE_BYTES + 1)}'", ), @@ -368,6 +468,100 @@ class TestSavedViewsSchema(unittest.TestCase): with self.assertRaises(ValueError): self.db.model.restore_from_file(src) + def _insert_old_rows(self, path): + """Rows written under older value rules, straight into the file.""" + with closing(sqlite3.connect(path)) as conn: + conn.execute( + f"INSERT INTO {TABLE} (page, name, state, position, created_at, " + "updated_at) VALUES ('images', 'Golden hour', ?, 5, 'x', 'x')", + (OLD_IMAGES_STATE,), + ) + conn.execute( + f"INSERT INTO {TABLE} (page, name, state, position, created_at, " + "updated_at) VALUES ('library', ?, ?, 6, 'x', 'x')", + (" " + "n" * 120, json.dumps({"sort": "name-desc", "q": 4})), + ) + conn.commit() + + def _assert_old_rows_normalized(self, views): + self.assertEqual( + [(view["page"], view["name"], view["state"]) for view in views], + [ + ( + "images", + "Golden hour", + { + **IMAGES_DEFAULTS, + "q": "sunset", + "media": "image", + "tags": ["golden"], + }, + ), + ("library", "n" * 100, LIBRARY_DEFAULTS), + ], + ) + + def test_a_database_with_rows_from_older_rules_still_opens(self): + self.db.close_all() + self._insert_old_rows(self.path) + PromptModel.reset_schema_cache() + + reopened = PromptDatabase(self.path) + try: + self._assert_old_rows_normalized(reopened.list_saved_views()) + finally: + reopened.close_all() + ok, reason = PromptModel.verify_database_file( + self.path, require_library_state=True + ) + self.assertTrue(ok, reason) + + def test_restore_accepts_rows_from_older_rules(self): + src = self._source() + self._insert_old_rows(src) + ok, reason = PromptModel.verify_database_file(src, require_library_state=True) + self.assertTrue(ok, reason) + + self.db.model.restore_from_file(src) + + self._assert_old_rows_normalized(self.db.list_saved_views()) + + def test_normalize_state_keeps_valid_values_and_defaults_the_rest(self): + self.assertEqual(normalize_state("images", IMAGES_STATE), IMAGES_STATE) + self.assertEqual(normalize_state("library", LIBRARY_STATE), LIBRARY_STATE) + self.assertEqual( + normalize_state("images", json.dumps(IMAGES_STATE)), IMAGES_STATE + ) + for raw in (None, "{", "[]", '"q"', 7, ["q"], b"{}"): + with self.subTest(raw=raw): + self.assertEqual(normalize_state("library", raw), LIBRARY_DEFAULTS) + self.assertEqual( + normalize_state( + "images", + { + "q": "bad\x00", + "source": 3, + "favorites": "true", + "tags": [f"t{n}" for n in range(21)], + "folder": "kept", + }, + ), + {**IMAGES_DEFAULTS, "folder": "kept"}, + ) + self.assertEqual(normalize_state("history", {}), {}) + + def test_validation_errors_are_saved_view_invalid(self): + for call in ( + lambda: validate_name(""), + lambda: validate_name(3), + lambda: encode_state("history", {}), + lambda: encode_state("library", {"q": ""}), + lambda: encode_state("library", {**LIBRARY_STATE, "sort": "x"}), + ): + with self.assertRaises(SavedViewInvalid): + call() + self.assertTrue(issubclass(SavedViewInvalid, ValueError)) + def test_migration_refuses_an_unsupported_table(self): path = os.path.join(self.tmpdir, "foreign.db") with closing(sqlite3.connect(path)) as conn: diff --git a/web/js/workspace/images.mjs b/web/js/workspace/images.mjs index a421c7f..e3ab44b 100644 --- a/web/js/workspace/images.mjs +++ b/web/js/workspace/images.mjs @@ -141,10 +141,13 @@ export function mountImages(main, api, parentScope, {embedded = false, fetchPage if (!IMAGE_SORT_OPTIONS.some(([value]) => value === filters.sort)) filters.sort = 'newest'; // A saved Source or Folder is only listed once discovery confirms it still exists; until then nothing is fetched. let savedScopePending = Boolean(saved) && (filters.root !== 'all' || filters.subfolder !== ''); + // The first discovery settled (found or failed): only then may a pending saved scope be kept or dropped (#249 M3). + let discoverySettled = false; scope.cleanup(() => { // A Set category write still in flight may change what the saved collection shows: the return reloads instead. - // A favorites-only collection is not the Images collection, so it never becomes the Images return state. - if (!embedded && !filters.favorites) lastVisit = state && !bar.writing() ? { filters: {...filters, tags: [...filters.tags]}, state, layout, density, selection, checked: bar.checked(), scrollY: window.scrollY } : null; + // A favorites-only collection or a saved view is not the plain Images collection, so it never becomes the + // Images return state (#249 D51, and M2 for saved views). + if (!embedded && !filters.favorites && !saved) lastVisit = state && !bar.writing() ? { filters: {...filters, tags: [...filters.tags]}, state, layout, density, selection, checked: bar.checked(), scrollY: window.scrollY } : null; cancelAnimationFrame(restoreFrame); }); main.classList.add('images-main'); @@ -616,6 +619,9 @@ export function mountImages(main, api, parentScope, {embedded = false, fetchPage // The ruled selection lifetime: a replaced collection may no longer hold the checked files. bar?.clear(); for (const video of details.querySelectorAll('video')) releaseVideo(video); + // A toolbar change while discovery is still pending keeps the saved Source and Folder; the settled discovery + // then lists the view with every current value (#249 M3). + if (savedScopePending && !discoverySettled) { updateScope(); return; } settleSavedScope(); updateScope(); stream.reset(filters, {load:!recovering}); @@ -777,6 +783,7 @@ export function mountImages(main, api, parentScope, {embedded = false, fetchPage // The page's one discovery alert is written once both first loads settled (#249 final review m6). if (!embedded) Promise.all([loadOptions(), loadCategories()]).then(() => { if (scope.signal.aborted) return; + discoverySettled = true; renderLoadStatus(); if (savedScopePending) reset(); }); diff --git a/web/js/workspace/index.mjs b/web/js/workspace/index.mjs index 5068f3a..64d9a2f 100644 --- a/web/js/workspace/index.mjs +++ b/web/js/workspace/index.mjs @@ -1,7 +1,7 @@ import '../api-client.js'; import { createButton, createScope } from '../shared/toolkit/core.js'; import { routes, mountRouter, summarizeStats, routeTitle, isHandoff, loadNavCounts, - LIBRARY_WRITE_COUNTS, NAV_COUNT_ROUTES, FAVORITE_WRITE_COUNTS } from './state.mjs'; + LIBRARY_WRITE_COUNTS, NAV_COUNT_ROUTES, FAVORITE_WRITE_COUNTS, FAVORITE_COUNT_DELAY_MS } from './state.mjs'; import { mountWorkspaceTheme } from './theme.mjs'; import { mountImages, onFavoriteChange } from './images.mjs'; import { mountLibrary, mountNewPromptAction, onLibraryWrite } from './library.mjs'; @@ -111,8 +111,14 @@ function mountWorkspace() { scope.cleanup(() => { for (const request of countRequests.values()) request.abort(); }); // A confirmed Library or vocabulary write cannot change the Images count, so only Library and Tags are re-read. scope.cleanup(onLibraryWrite(() => refreshCounts(LIBRARY_WRITE_COUNTS))); - // Starring or unstarring anywhere (Images, Favorites, Library media) re-reads the Favorites count (D51). - scope.cleanup(onFavoriteChange(() => refreshCounts(FAVORITE_WRITE_COUNTS))); + // Starring or unstarring anywhere (Images, Favorites, Library media) re-reads the Favorites count (D51), once + // after a run of quick toggles settles rather than once per click (#249 M1). + let favoriteCountTimer = 0; + scope.cleanup(onFavoriteChange(() => { + clearTimeout(favoriteCountTimer); + favoriteCountTimer = setTimeout(() => refreshCounts(FAVORITE_WRITE_COUNTS), FAVORITE_COUNT_DELAY_MS); + })); + scope.cleanup(() => clearTimeout(favoriteCountTimer)); refreshCounts(NAV_COUNT_ROUTES); let viewScope; let firstRender = true; diff --git a/web/js/workspace/library.mjs b/web/js/workspace/library.mjs index bac5149..dc40115 100644 --- a/web/js/workspace/library.mjs +++ b/web/js/workspace/library.mjs @@ -413,7 +413,8 @@ export function mountLibrary(main,api,parentScope,{positiveId,savedState=null,on scope.cleanup(()=>{ const savedFilters={...filters,tags:[...filters.tags]},savedAnchor=pendingAnchor || anchor || captureAnchor(); // A write while mounted makes this stream stale: keep only what still means something. - shelfVisit=mountedGeneration!==writeGeneration?{filters:savedFilters,anchor:savedAnchor}:state?{filters:savedFilters,state,scrollY:window.scrollY,layout:{inlineSize,columns:columnCount,heights:[...heights],anchor:savedAnchor}}:null; + // A saved view is not the plain shelf, so it never replaces the Library return state (#249 M2, as Favorites). + if(!saved) shelfVisit=mountedGeneration!==writeGeneration?{filters:savedFilters,anchor:savedAnchor}:state?{filters:savedFilters,state,scrollY:window.scrollY,layout:{inlineSize,columns:columnCount,heights:[...heights],anchor:savedAnchor}}:null; toolbarScope.dispose();stream.dispose();optionsRequest?.abort();observer?.disconnect();measurementObserver?.disconnect();cancelAnimationFrame(frame);cancelAnimationFrame(loadFrame);for(const card of cards.values())removeCard(card);cards.clear();}); loadCategories();if(restorable) {render(stream.state);if(restorable.state.loading || restorable.state.error)stream.loadMore();} else reset(); } diff --git a/web/js/workspace/saved-views.mjs b/web/js/workspace/saved-views.mjs index 6ebb645..a179f52 100644 --- a/web/js/workspace/saved-views.mjs +++ b/web/js/workspace/saved-views.mjs @@ -11,7 +11,7 @@ * Delete… (danger), Cancel and Save; Delete… asks inside the same dialog, never through `window.confirm`. */ import { createButton } from '../shared/toolkit/core.js'; -import { openInputDialog, openContentDialog, closeWorkspaceDialog } from './workspace-dialogs.mjs'; +import { openInputDialog, openContentDialog, closeWorkspaceDialog, field } from './workspace-dialogs.mjs'; import { showWorkspaceToast } from './toast.mjs'; export const SAVED_VIEWS_URL = '/prompt_manager/saved-views'; @@ -21,6 +21,9 @@ export const RENAMED_TOAST = 'Saved view renamed.'; export const DELETED_TOAST = 'Saved view deleted.'; export const RENAME_FAILED_TOAST = 'The view could not be renamed. Try again.'; export const DELETE_FAILED_TOAST = 'The view could not be deleted. Try again.'; +export const TOO_LARGE_TOAST = 'The view is too large to save. Shorten its search, folder or tags and try again.'; +// The API's cap on a view's encoded state (database/saved_views.py MAX_STATE_BYTES), in UTF-8 bytes of compact JSON. +export const MAX_STATE_BYTES = 4096; const MAX_NAME_LENGTH = 100; const MAX_TAGS = 20; const MAX_TAG_LENGTH = 50; @@ -57,6 +60,16 @@ export function normalizeViewState(page, state) { })); } +/** True when the state's compact JSON fits the API's 4 KB cap (the server's encoding: no spaces, UTF-8). */ +export function stateFits(state) { + return new TextEncoder().encode(JSON.stringify(state)).length <= MAX_STATE_BYTES; +} + +/** The error `create()` rejects with when the state is over the cap; nothing is sent. */ +export class SavedViewTooLarge extends RangeError { + constructor() { super('Saved view state is over 4 KB'); this.name = 'SavedViewTooLarge'; } +} + /** The concept's default name (a1336): `Search: ` while there is a search, else "My workspace view". */ export function defaultViewName(q) { const search = typeof q === 'string' ? q.trim() : ''; @@ -136,7 +149,9 @@ export function createSavedViews(api, signal) { }); } async function create(page, name, state) { - const data = await api.post(SAVED_VIEWS_URL, { page, name, state: normalizeViewState(page, state) }, { signal }); + const full = normalizeViewState(page, state); + if (!stateFits(full)) throw new SavedViewTooLarge(); + const data = await api.post(SAVED_VIEWS_URL, { page, name, state: full }, { signal }); const view = parseSavedView(data?.view); if (!view) throw new TypeError('Invalid saved view'); await load(); @@ -193,15 +208,15 @@ export function openSaveViewDialog({ scope, trigger, q, onSave }) { try { await onSave(name); showWorkspaceToast(SAVED_TOAST); - } catch { - if (!scope.signal.aborted) showWorkspaceToast(SAVE_FAILED_TOAST); + } catch (error) { + if (!scope.signal.aborted) showWorkspaceToast(error instanceof SavedViewTooLarge ? TOO_LARGE_TOAST : SAVE_FAILED_TOAST); } } }); } /** * D51: the open view's Edit dialog. "View name" (Save keeps the dialog open on an empty name; an unchanged one closes it), then Delete… (danger), Cancel - * and Save. Delete… swaps the same dialog to its confirmation, Cancel / Delete, so nothing is removed by one press and + * and Save. Delete… swaps the same dialog to its confirmation, Cancel / Delete (its Cancel closes the dialog), so nothing is removed by one press and * no browser `confirm()` appears. `onRename(name)` and `onDelete()` return the store's promise; the toast reports either * outcome, and the shell navigates after a delete. */ @@ -212,12 +227,6 @@ export function openEditViewDialog({ scope, trigger, view, onRename, onDelete }) const remove = createButton('Delete…', 'danger'); const cancel = createButton('Cancel'); const save = createButton('Save', 'ca-primary'); - const field = document.createElement('div'); - field.className = 'field'; - const label = document.createElement('label'); - label.textContent = 'View name'; - label.htmlFor = input.id = 'action-input'; - field.append(label, input); const actions = document.createElement('div'); actions.className = 'dialog-actions'; actions.append(remove, cancel, save); @@ -235,7 +244,7 @@ export function openEditViewDialog({ scope, trigger, view, onRename, onDelete }) } }); remove.addEventListener('click', () => confirmDelete({ scope, trigger, view, onDelete })); - openContentDialog({ scope, trigger, title: 'Edit saved view', nodes: [field, actions] }); + openContentDialog({ scope, trigger, title: 'Edit saved view', nodes: [field('View name', input, 'action-input'), actions] }); input.focus(); input.select(); } @@ -255,7 +264,7 @@ function confirmDelete({ scope, trigger, view, onDelete }) { await onDelete(); showWorkspaceToast(DELETED_TOAST); } catch { - showWorkspaceToast(DELETE_FAILED_TOAST); + if (!scope.signal.aborted) showWorkspaceToast(DELETE_FAILED_TOAST); } }); openContentDialog({ scope, trigger, title: 'Delete saved view', nodes: [text, actions] }); diff --git a/web/js/workspace/state.mjs b/web/js/workspace/state.mjs index b5c6568..1cf43cb 100644 --- a/web/js/workspace/state.mjs +++ b/web/js/workspace/state.mjs @@ -78,6 +78,8 @@ export const LIBRARY_WRITE_COUNTS = Object.freeze(['library', 'tags']); /** The count a confirmed favorite change can change. */ export const FAVORITE_WRITE_COUNTS = Object.freeze(['favorites']); +// Trailing delay that coalesces quick star toggles into one Favorites count read (#249 M1). +export const FAVORITE_COUNT_DELAY_MS = 300; /** Read the given sidebar counts (all by default), reporting each as its own read settles. */ export async function loadNavCounts(api, signal, onCount = () => {}, ids = NAV_COUNT_ROUTES) { diff --git a/web/js/workspace/workspace-dialogs.mjs b/web/js/workspace/workspace-dialogs.mjs index ace1106..78179d8 100644 --- a/web/js/workspace/workspace-dialogs.mjs +++ b/web/js/workspace/workspace-dialogs.mjs @@ -32,7 +32,8 @@ function el(tag, text, className) { return node; } -function field(label, control, id) { +/** A labelled `.field` row: `label` names `control`, which takes `id` (shared with the saved-view Edit dialog). */ +export function field(label, control, id) { const wrapper = el('div', undefined, 'field'); const caption = el('label', label); caption.htmlFor = id;