From e0bf2b61d91f81405f50f76b95cd92766ae08f40 Mon Sep 17 00:00:00 2001 From: parththakkar106 Date: Sat, 29 Aug 2026 02:12:32 +0530 Subject: [PATCH] Add a useDebouncedSave hook and delete Settings.stream Two items from Stage 2 of `plan/17-refactor.md`. `frontend/src/hooks/useDebouncedSave.js` replaces three copies of the same debounce. `PlotPanel` and `ScenarioEditor` held identical per-key timer maps. `ScriptEditor` held a single shared timer, so editing two fields inside the same 600 ms window canceled the first save. The hook gives every key its own timer, which fixes that. `Settings.stream` was dead state. Nothing read it and every turn streams. This removes the column, both schema fields, and adds migration 65 to drop it. It is item S1 in `docs/self-review.md`. Migration 65 needs a new guard. `_column_already_gone` is the counterpart to `_column_already_there`: `create_all` builds the current schema, which is already missing every dropped column, so a fixture that stamps an old version and replays would fail on a column that is not there. Verified: 549 backend tests pass, lint and build are clean, and migration 65 runs both ways, once against a database that still has the column and once against one that does not. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014Dix4oGV3njgWRdu7P9t6r --- backend/app/migrations.py | 26 +++++++++++++++++++- backend/app/models.py | 1 - backend/app/schemas.py | 2 -- docs/self-review.md | 13 +++++++--- frontend/src/hooks/useDebouncedSave.js | 17 +++++++++++++ frontend/src/pages/Play/panels/PlotPanel.jsx | 11 +++------ frontend/src/pages/ScenarioEditor.jsx | 11 +++------ frontend/src/pages/ScriptEditor.jsx | 10 ++++---- 8 files changed, 63 insertions(+), 28 deletions(-) create mode 100644 frontend/src/hooks/useDebouncedSave.js diff --git a/backend/app/migrations.py b/backend/app/migrations.py index 2078fbd..ae75b53 100644 --- a/backend/app/migrations.py +++ b/backend/app/migrations.py @@ -297,6 +297,10 @@ MIGRATIONS: list[tuple[int, str | dict[str, str]]] = [ # under. (63, "ALTER TABLE actions ADD COLUMN parent_id INTEGER REFERENCES actions(id) ON DELETE SET NULL"), (64, "CREATE INDEX IF NOT EXISTS ix_actions_parent ON actions (parent_id)"), + # Phase 17: `Settings.stream` was dead state. Nothing ever read it, and every + # turn streams. This is item S1 in `docs/self-review.md`. The table holds one + # row per user, so the rewrite is small and needs no VACUUM FULL. + (65, "ALTER TABLE settings DROP COLUMN stream"), ] LATEST_VERSION = max((v for v, _ in MIGRATIONS), default=1) @@ -427,6 +431,25 @@ def _for_dialect(sql: str | dict[str, str], dialect: str) -> str: # Matches the ADD COLUMN migrations in this file. Every one is written above, # so this pattern parses only SQL this file controls. _ADD_COLUMN = re.compile(r"^\s*ALTER\s+TABLE\s+(\w+)\s+ADD\s+COLUMN\s+\"?(\w+)\"?", re.I) +_DROP_COLUMN = re.compile(r"^\s*ALTER\s+TABLE\s+(\w+)\s+DROP\s+COLUMN\s+\"?(\w+)\"?", re.I) + + +def _column_already_gone(conn, sql: str) -> bool: + """Returns `True` when `sql` drops a column the table no longer has. + + This is the counterpart to `_column_already_there`, for the same reason. + `create_all` builds the current schema, which is already missing every + column a migration drops. Replaying from an older stamp against a database + built that way would fail on a column that is not there. + """ + match = _DROP_COLUMN.match(sql) + if match is None: + return False + table, column = match.group(1), match.group(2) + inspector = inspect(conn) + if table not in inspector.get_table_names(): + return False + return column not in {col["name"] for col in inspector.get_columns(table)} def _column_already_there(conn, sql: str) -> bool: @@ -946,7 +969,8 @@ def bootstrap(engine: Engine) -> None: statement = _for_dialect(sql, conn.dialect.name) # Skip the DDL when it has already run. The data pass below it # still runs. - if not _column_already_there(conn, statement): + if not (_column_already_there(conn, statement) + or _column_already_gone(conn, statement)): conn.execute(text(statement)) if version == WORLD_DELTA_VERSION: _backfill_world_delta(conn) diff --git a/backend/app/models.py b/backend/app/models.py index 2c697c1..c9de38f 100644 --- a/backend/app/models.py +++ b/backend/app/models.py @@ -619,7 +619,6 @@ class Settings(Base): "for the player's next action." ), ) - stream: Mapped[bool] = mapped_column(Boolean, default=True) # Phase 6: auto-summarization + memory bank summary_model: Mapped[str] = mapped_column(String(200), default="") # "" = main model embedding_model: Mapped[str] = mapped_column(String(200), default="") # "" = bank disabled diff --git a/backend/app/schemas.py b/backend/app/schemas.py index 7fa2cd1..5ca7f19 100644 --- a/backend/app/schemas.py +++ b/backend/app/schemas.py @@ -449,7 +449,6 @@ class SettingsOut(ORMModel): reasoning_max_tokens: int context_token_budget: int narrator_prompt: str - stream: bool summary_model: str embedding_model: str memory_bank_capacity: int @@ -496,7 +495,6 @@ class SettingsUpdate(BaseModel): reasoning_max_tokens: Annotated[int, Field(ge=-1, le=100_000)] | None = None context_token_budget: Annotated[int, Field(ge=256, le=200_000)] | None = None narrator_prompt: Prose | None = None - stream: bool | None = None summary_model: Name | None = None embedding_model: Name | None = None memory_bank_capacity: Annotated[int, Field(ge=1, le=1000)] | None = None diff --git a/docs/self-review.md b/docs/self-review.md index 4750d24..2e09a02 100644 --- a/docs/self-review.md +++ b/docs/self-review.md @@ -125,12 +125,19 @@ break compatibility. This is documented in `engine.py`'s prelude instead. The cl ## Cleanup backlog (reuse / simplification / efficiency / altitude — not bugs, apply later) -- **R1** `frontend/src/pages/Play.jsx:26` + `ScenarioEditor.jsx:19` + `ScriptEditor.jsx:34`: three copies of the debounced-autosave and story-card handlers. Extract a `useDebouncedSave` hook and a shared StoryCardList component. Fixing bugs #15/#16 properly may accomplish this. +- **R1** ~~three copies of the debounced-autosave handler.~~ + **Half applied in phase 17 (2026-08).** All three use + `frontend/src/hooks/useDebouncedSave.js`. A shared StoryCardList component is + still open. - **R2** `backend/seed_demo.py:228`: re-implements create_adventure. Call the router logic instead. - **R3** `backend/app/providers/openai_compatible.py:122`: complete() duplicates _request()'s body building. Add a `stream` param to _request(). -- **R4** `backend/app/routers/adventures.py:476`: six copies of child-resource get+owner-check+404. Extract `get_owned_or_404`. +- **R4** ~~six copies of child-resource get, owner-check, and 404.~~ + **Applied in phase 17 (2026-08).** All 32 handlers take the `current_adventure` + dependency from `routers/adventures/deps.py`. - **R5** `frontend/src/api.js:26`: streamSSE duplicates request()'s error extraction. Extract `throwIfNotOk(resp)`. -- **S1** `backend/app/models.py:210`: `Settings.stream` is dead state (never read). Delete the column and its schema fields. +- **S1** ~~`Settings.stream` is dead state (never read).~~ + **Applied in phase 17 (2026-08).** The column, both schema fields, and migration 65 + drop it. `R4` went with it: the ownership check is the `current_adventure` dependency. - **S2** `frontend/src/pages/Play.jsx:6`: MODES and PLAYER_TYPES are identical constants; lastIsAi/canUndo are computed twice. - **E1** `backend/app/context/builder.py:119`: joins and tokenizes the entire adventure history every turn for the trigger window. Walk reversed(actions) until budget instead. - **E2** `backend/app/scripting/pipeline.py:77`: rebuilds full history dicts, JSON, and a blocking commit per script per hook. Build once per hook, slice to HISTORY_WINDOW first, and commit once. diff --git a/frontend/src/hooks/useDebouncedSave.js b/frontend/src/hooks/useDebouncedSave.js new file mode 100644 index 0000000..64f2cf8 --- /dev/null +++ b/frontend/src/hooks/useDebouncedSave.js @@ -0,0 +1,17 @@ +// Delays a save until its field has been quiet for a moment. +// +// Every `key` gets its own timer. One shared timer cancels the pending save of +// whatever was edited before it, so editing two fields inside the same window +// saves only the second one. + +import { useRef } from 'react' + +function useDebouncedSave(delay = 600) { + const timers = useRef(new Map()) + return (key, fn) => { + clearTimeout(timers.current.get(key)) + timers.current.set(key, setTimeout(fn, delay)) + } +} + +export { useDebouncedSave } diff --git a/frontend/src/pages/Play/panels/PlotPanel.jsx b/frontend/src/pages/Play/panels/PlotPanel.jsx index ac8914d..e955b38 100644 --- a/frontend/src/pages/Play/panels/PlotPanel.jsx +++ b/frontend/src/pages/Play/panels/PlotPanel.jsx @@ -1,21 +1,16 @@ // The plot panel: the adventure's own copy of the scenario text and cards. -import { useRef, useState } from 'react' +import { useState } from 'react' import { api } from '../../../api' import { Field, StoryCardRow, downloadJSON, pickJSONFile, useToast } from '../../../components' +import { useDebouncedSave } from '../../../hooks/useDebouncedSave' import { RefreshModal } from '../RefreshModal' function PlotPanel({ adventure, setAdventure, onWorldStateChanged }) { const toast = useToast() const [plan, setPlan] = useState(null) // non-null while the modal is open const [planning, setPlanning] = useState(false) - // One timer per field/card: a single shared timer would cancel the pending - // save of whatever was edited previously within the debounce window. - const saveTimers = useRef(new Map()) - const debounceSave = (key, fn) => { - clearTimeout(saveTimers.current.get(key)) - saveTimers.current.set(key, setTimeout(fn, 600)) - } + const debounceSave = useDebouncedSave() const setField = (field, value) => { setAdventure({ ...adventure, [field]: value }) diff --git a/frontend/src/pages/ScenarioEditor.jsx b/frontend/src/pages/ScenarioEditor.jsx index 3c22871..f1df5df 100644 --- a/frontend/src/pages/ScenarioEditor.jsx +++ b/frontend/src/pages/ScenarioEditor.jsx @@ -1,7 +1,8 @@ -import { useEffect, useRef, useState } from 'react' +import { useEffect, useState } from 'react' import { useNavigate, useParams } from 'react-router-dom' import { api } from '../api' import { Field, StoryCardRow, downloadJSON, pickJSONFile, useToast } from '../components' +import { useDebouncedSave } from '../hooks/useDebouncedSave' import ArtPicker from '../ArtPicker' import SchemaEditor, { NpcEditor, addNpc } from '../SchemaEditor' @@ -18,13 +19,7 @@ export default function ScenarioEditor() { const [parsedSchema, setParsedSchema] = useState(null) const [schemaView, setSchemaView] = useState('form') // 'form' | 'json' const toast = useToast() - // One timer per field/card: a single shared timer would cancel the pending - // save of whatever was edited previously within the debounce window. - const saveTimers = useRef(new Map()) - const debounceSave = (key, fn) => { - clearTimeout(saveTimers.current.get(key)) - saveTimers.current.set(key, setTimeout(fn, 600)) - } + const debounceSave = useDebouncedSave() useEffect(() => { api.getScenario(id).then((s) => { diff --git a/frontend/src/pages/ScriptEditor.jsx b/frontend/src/pages/ScriptEditor.jsx index 05809e0..3096930 100644 --- a/frontend/src/pages/ScriptEditor.jsx +++ b/frontend/src/pages/ScriptEditor.jsx @@ -1,10 +1,11 @@ -import { useEffect, useRef, useState } from 'react' +import { useEffect, useState } from 'react' import { useNavigate, useParams } from 'react-router-dom' import CodeMirror from '@uiw/react-codemirror' import { javascript } from '@codemirror/lang-javascript' import { oneDark } from '@codemirror/theme-one-dark' import { api } from '../api' import { Field, downloadJSON } from '../components' +import { useDebouncedSave } from '../hooks/useDebouncedSave' const SLOTS = [ { key: 'library_js', label: 'Library', hint: 'Shared code prepended to all three hooks.' }, @@ -19,7 +20,7 @@ export default function ScriptEditor() { const [script, setScript] = useState(null) const [slot, setSlot] = useState('input_js') const [status, setStatus] = useState('') - const saveTimer = useRef(null) + const debounceSave = useDebouncedSave() // Test-run state const [testHook, setTestHook] = useState('input') @@ -33,12 +34,11 @@ export default function ScriptEditor() { const setField = (field, value) => { setScript((prev) => ({ ...prev, [field]: value })) - clearTimeout(saveTimer.current) - saveTimer.current = setTimeout(async () => { + debounceSave(field, async () => { await api.updateScript(id, { [field]: value }) setStatus('Saved') setTimeout(() => setStatus(''), 1500) - }, 600) + }) } const runTest = async () => {