diff --git a/backend/tests/test_take_edit.py b/backend/tests/test_take_edit.py new file mode 100644 index 0000000..b25ccfa --- /dev/null +++ b/backend/tests/test_take_edit.py @@ -0,0 +1,197 @@ +"""Phase 14 SP9 — editing the take you are actually reading. + +The pager can park a turn on take 2 of 4. The transcript row it sits in is +still keyed by the *live* take, because that is the row the story tells and the +one the window carries; the take being read is only in the client's hand, by +its own node id. + +So "edit this" has two ids to choose from, and the page shipped choosing the +wrong one: it opened the editor on the live take's text and saved over it, from +a row that was showing take 2. That is a client bug and it is fixed in +Play.jsx, but the fix rests on something only the server can promise — + + a take is an ordinary row to the edit endpoint, addressed by its own id, + whether or not it is the one the path runs through + +— and on the group listing telling the truth about it afterwards. Both are +asserted here so the client's fix cannot be quietly undermined. + + python -m pytest tests/test_take_edit.py -v +""" +import os +import tempfile + +_tmp = tempfile.NamedTemporaryFile(suffix=".db", delete=False) +_tmp.close() +os.environ["AIDND_DB_PATH"] = _tmp.name +os.environ.pop("AIDND_DATABASE_URL", None) +os.environ.pop("DATABASE_URL", None) + +import pytest +from fastapi import Depends +from fastapi.testclient import TestClient + +from app import auth, limits, models +from app.database import Base, SessionLocal, engine, get_db +from app.main import app +from app.providers import PromptParts +from app.routers import adventures + + +class ScriptedProvider: + replies: list = [] + calls = 0 + + def __init__(self, *a, **k): + pass + + async def generate(self, parts: PromptParts, *, temperature, max_tokens): + index = min(ScriptedProvider.calls, len(ScriptedProvider.replies) - 1) + ScriptedProvider.calls += 1 + yield ("text", ScriptedProvider.replies[index]) + + +@pytest.fixture() +def client(monkeypatch): + Base.metadata.create_all(bind=engine) + setup = SessionLocal() + user = models.User(is_guest=False, email="takeedit@example.com") + setup.add(user) + setup.flush() + setup.add(models.Settings(user_id=user.id, api_key="enc:dummy", model="test-model")) + scenario = models.Scenario(user_id=user.id, title="S") + setup.add(scenario) + setup.flush() + adv = models.Adventure( + user_id=user.id, title="Vault", scenario_id=scenario.id, + script_state={}, world_state={}, + ) + setup.add(adv) + setup.flush() + setup.add(models.Action(adventure_id=adv.id, index=0, type="start", text="You begin.")) + setup.commit() + adv_id, user_id = adv.id, user.id + setup.close() + + ScriptedProvider.replies = [f"Take {n}." for n in range(1, 40)] + ScriptedProvider.calls = 0 + monkeypatch.setattr(adventures, "OpenAICompatibleProvider", ScriptedProvider) + monkeypatch.setattr(auth, "resolve_provider_config", lambda s: auth.ProviderConfig( + "http://fake", "k", "test-model", False)) + monkeypatch.setattr(limits, "rate_limit", lambda *a, **k: None) + monkeypatch.setattr(limits, "check_row_cap", lambda *a, **k: None) + + def _current_user(db=Depends(get_db)): + return db.get(models.User, user_id) + + app.dependency_overrides[auth.get_current_user] = _current_user + c = TestClient(app) + c.adv_id = adv_id + try: + yield c + finally: + app.dependency_overrides.clear() + adventures._active_turns.clear() + Base.metadata.drop_all(bind=engine) + + +def _play(client, text="look around"): + r = client.post(f"/api/adventures/{client.adv_id}/actions", + json={"type": "do", "text": text}) + assert r.status_code == 200, r.text + + +def _retry(client): + r = client.post(f"/api/adventures/{client.adv_id}/retry") + assert r.status_code == 200, r.text + + +def _takes(client, action_id): + r = client.get(f"/api/adventures/{client.adv_id}/actions/{action_id}/variants") + assert r.status_code == 200, r.text + return r.json() + + +def _edit(client, action_id, text): + return client.patch(f"/api/adventures/{client.adv_id}/actions/{action_id}", + json={"text": text}) + + +def _live_ai(client): + """The AI row the story currently tells, as the transcript reports it.""" + adv = client.get(f"/api/adventures/{client.adv_id}").json() + return [a for a in adv["actions"] if a["type"] == "ai"][-1] + + +def _four_takes(client): + """One turn, played four times. Returns (row the page holds, take list).""" + _play(client) + for _ in range(3): + _retry(client) + row = _live_ai(client) + assert row["take_count"] == 4 + return row, _takes(client, row["id"]) + + +def test_the_pager_reads_four_distinct_takes(client): + """The premise: 2/4 and 4/4 are different rows with different words.""" + row, takes = _four_takes(client) + assert [t["text"] for t in takes] == ["Take 1.", "Take 2.", "Take 3.", "Take 4."] + assert takes[3]["id"] == row["id"], "the newest take is the one the story tells" + assert takes[1]["id"] != row["id"], "2/4 is not the row the transcript is keyed by" + + +def test_editing_a_take_that_is_not_live_edits_that_take(client): + """The bug, at the level the client's fix depends on. + + Saving against take 2's own id must land on take 2 — not be refused for + being off the path, and not be redirected onto the live row. + """ + row, takes = _four_takes(client) + second = takes[1] + + r = _edit(client, second["id"], "Take 2, rewritten.") + assert r.status_code == 200, r.text + assert r.json()["text"] == "Take 2, rewritten." + + after = _takes(client, row["id"]) + assert [t["text"] for t in after] == [ + "Take 1.", "Take 2, rewritten.", "Take 3.", "Take 4.", + ], "one take changed, and only the one addressed" + + +def test_editing_a_take_leaves_the_live_one_alone(client): + """What the page did instead: the reader saw 2/4 and 4/4 was overwritten.""" + row, takes = _four_takes(client) + + _edit(client, takes[1]["id"], "Take 2, rewritten.") + + assert _live_ai(client)["text"] == "Take 4.", "the story still tells what it told" + + +def test_a_take_edit_survives_paging_away_and_back(client): + """The listing is the pager's only source, so the edit has to be in it. + + (The client caches this list per message; the fix drops that cache after an + edit. If the server ever started answering from a copy of its own, stepping + away and back would show the words before the edit and nobody would see it + here — hence the round trip.) + """ + row, takes = _four_takes(client) + _edit(client, takes[1]["id"], "Take 2, rewritten.") + + # Addressed by the live row, as the pager does, and by the edited take + # itself, as a client holding that id would. + assert _takes(client, row["id"])[1]["text"] == "Take 2, rewritten." + assert _takes(client, takes[1]["id"])[1]["text"] == "Take 2, rewritten." + + +def test_an_edited_take_is_still_the_take_it_was(client): + """Editing text is not switching, forking, or reordering.""" + row, takes = _four_takes(client) + _edit(client, takes[1]["id"], "Take 2, rewritten.") + + after = _takes(client, row["id"]) + assert [t["id"] for t in after] == [t["id"] for t in takes], "same rows, same order" + assert [t["active"] for t in after] == [False, False, False, True] + assert _live_ai(client)["take_index"] == 3, "still 4/4 on screen" diff --git a/frontend/src/pages/Play.jsx b/frontend/src/pages/Play.jsx index 6666276..76a9eef 100644 --- a/frontend/src/pages/Play.jsx +++ b/frontend/src/pages/Play.jsx @@ -971,9 +971,15 @@ function WorldStateDrawer({ advId, refreshKey }) { // A take on this branch is a leaf by construction: whatever was played after // this turn was played after the take that is live, so a take that is not live // has nothing under it and the transcript simply ends there. -function TakePager({ advId, action, busy, preview, onPreview, onSwitchedBranch, onError }) { +function TakePager({ + advId, action, busy, preview, takesKey, onPreview, onSwitchedBranch, onError, +}) { const [takes, setTakes] = useState(null) const [loading, setLoading] = useState(false) + // The cached list is only as good as the text in it. Editing a take + // rewrites one of those rows, so the page says so and the list is fetched + // again on the next step. + useEffect(() => { setTakes(null) }, [takesKey]) const count = action.take_count const live = action.take_index const current = preview ? preview.index : live @@ -1513,6 +1519,10 @@ export default function Play() { // decision. It becomes one when something is written below it, and that is // what `after_id` carries. const [preview, setPreview] = useState(null) + // Bumped when a take's stored text changes under the pagers, which cache the + // list they fetched. Nothing else invalidates it: a take is added by playing + // a turn, and that re-reads the whole window anyway. + const [takesKey, setTakesKey] = useState(0) // The transcript is a window on the story, not the whole of it: the page // load brings the newest page and older ones arrive as the reader scrolls // up. `total` is the story's real length, for the "N earlier" line. @@ -1821,8 +1831,16 @@ export default function Play() { return () => window.removeEventListener('keydown', onKey) }) + // A take-edit belongs to the preview that opened it. Anything that leaves + // that take — playing a turn, switching branch — takes the box with it, so + // the pending edit goes too rather than being saved onto a take nobody is + // looking at any more. + useEffect(() => { + if (editing?.take && preview?.takeId !== editing.id) setEditing(null) + }, [editing, preview]) + async function saveEdit() { - const { id: actionId, text, fork } = editing + const { id: actionId, text, fork, take } = editing setEditing(null) if (fork) { // Not an edit at all: the turn is played again with this text, and what @@ -1832,7 +1850,17 @@ export default function Play() { } try { const updated = await api.updateAction(id, actionId, text) - setActions((prev) => prev.map((a) => (a.id === actionId ? updated : a))) + // A take that is only being read is not in `actions` — the row there is + // the live one — so the new text goes back into the preview, which is + // what that row is drawing. The pager holds the take list it fetched, so + // it is told to drop it: stepping away and back would otherwise show the + // words before the edit. + if (take) { + setPreview((p) => (p && p.takeId === actionId ? { ...p, text: updated.text } : p)) + setTakesKey((k) => k + 1) + } else { + setActions((prev) => prev.map((a) => (a.id === actionId ? updated : a))) + } } catch (err) { setToast({ text: err.message, isError: true }) } @@ -1955,7 +1983,14 @@ export default function Play() { // message without making it active (earlier turns only). const previewing = preview?.actionId === action.id ? preview : null - return editing?.id === action.id ? ( + // The editor stands in for the row it was opened from. That row is + // keyed by the live node, so an edit on a take the pager is parked + // on carries the take's id instead and is matched through the + // preview. + const editingHere = editing + && (editing.take ? previewing?.takeId === editing.id : editing.id === action.id) + + return editingHere ? (
setToast({ text: message, isError: true })} @@ -1998,13 +2034,29 @@ export default function Play() { )} + {/* Edits the take that is *on screen*, which is not the + live one while the pager is parked on another. The + row is keyed by the live node's id, so seeding from + `action` here opened the editor on take 4/4's text + while 2/4 was being read — and saved over it. A take + is an ordinary row to the edit endpoint, on the path + or not, so its own id is all this needs. */} + onClick={() => setEditing(previewing + ? { id: previewing.takeId, text: previewing.text, take: true } + : { id: action.id, text: action.text })}>✎ {/* Play this turn again, differently. On the AI's turn that is a regeneration; on your own it opens the text so you can say something else. Either way the story that followed the old take is kept, on the line it - was written on. */} + was written on. + + The id stays the live node's even while another take + is being read: adding a take branches just above the + turn, and the server only accepts a turn that is on + the path. Only the seeded text follows the screen, so + varying the take you are reading starts from its + words. */} {action.type !== 'start' && ( )}