diff --git a/backend/app/routers/adventures.py b/backend/app/routers/adventures.py index 941fee3..89a71c4 100644 --- a/backend/app/routers/adventures.py +++ b/backend/app/routers/adventures.py @@ -44,6 +44,10 @@ ACTION_LIST_COLUMNS = ( models.Action.world_delta, models.Action.variant_count, models.Action.variant_index, + # SP9: the pager's key. Deferred, it would be a lazy load per row — a query + # behind every message on the page, which is the whole thing `load_only` + # is here to stop. + models.Action.parent_id, models.Action.created_at, ) @@ -420,6 +424,10 @@ def get_adventure( """ adventure = get_adventure_or_404(adventure_id, db, user) actions, total, _ = action_window(db, adventure) + # Before the window is handed over, because this path serialises through + # the relationship rather than building ActionOut itself — the pager + # numbers have to be on the rows by the time Pydantic reads them. + annotate_takes(db, adventure.id, actions) # Hand the response the window as if the relationship had loaded it. # `set_committed_value` is the only way to do this safely: assigning # `adventure.actions = [...]` marks the collection dirty, and the @@ -1096,10 +1104,60 @@ def delete_turn( # actually built on, and the line being left is never disturbed. +def annotate_takes( + db: Session, adventure_id: int, actions: list[models.Action] +) -> list[models.Action]: + """Give every action on a page its `2/4` (SP9). + + One query for the whole page, not one per row. The pager needs the shape of + each turn's take group, and asking `attempts.group` per action would put a + query behind every message on screen — the cost `variant_count` was cached + to avoid, and the reason SP8 could not simply drop it and be done. + + Reading the siblings themselves rather than counting them: a group holds a + handful of takes, the page is bounded, and the alternative is a second query + for the ordinal. Only the id and the ordering keys are fetched, so this + stays cheap even where the text does not. + """ + parents = {a.parent_id for a in actions if a.parent_id is not None} + if parents: + rows = ( + db.query( + models.Action.id, + models.Action.parent_id, + models.Action.variant_index, + ) + .filter( + models.Action.adventure_id == adventure_id, + models.Action.parent_id.in_(parents), + ) + .order_by(models.Action.variant_index, models.Action.id) + .all() + ) + else: + rows = [] + siblings: dict[int, list[int]] = {} + for row_id, parent_id, _ in rows: + siblings.setdefault(parent_id, []).append(row_id) + for action in actions: + ids = siblings.get(action.parent_id) if action.parent_id else None + if not ids: + # A root node, or a pre-SP9 row the backfill could not place. Its + # own only take, which is what it was written as. + action.take_count, action.take_index = 1, 0 + continue + action.take_count = len(ids) + action.take_index = ids.index(action.id) if action.id in ids else 0 + return actions + + def current_window(db: Session, adventure: models.Adventure) -> schemas.ActionPage: actions, total, has_more = action_window(db, adventure) return schemas.ActionPage( - actions=[schemas.ActionOut.model_validate(a) for a in actions], + actions=[ + schemas.ActionOut.model_validate(a) + for a in annotate_takes(db, adventure.id, actions) + ], total=total, has_more=has_more, ) @@ -1587,7 +1645,10 @@ def undo_turn( # to be repeated several times in a row. actions, total, has_more = action_window(db, adventure) return schemas.ActionPage( - actions=[schemas.ActionOut.model_validate(a) for a in actions], + actions=[ + schemas.ActionOut.model_validate(a) + for a in annotate_takes(db, adventure.id, actions) + ], total=total, has_more=has_more, ) @@ -2140,7 +2201,10 @@ def list_actions( db, adventure, before_id=before_id, limit=limit ) return schemas.ActionPage( - actions=[schemas.ActionOut.model_validate(a) for a in actions], + actions=[ + schemas.ActionOut.model_validate(a) + for a in annotate_takes(db, adventure.id, actions) + ], total=total, has_more=has_more, ) diff --git a/backend/app/schemas.py b/backend/app/schemas.py index 8965bc1..6f5d547 100644 --- a/backend/app/schemas.py +++ b/backend/app/schemas.py @@ -189,6 +189,17 @@ class ActionOut(ORMModel): # GET /actions/{id}/variants so this payload stays small. variant_count: int = 0 variant_index: int = 0 + # SP9: the pager. How many takes this turn has, and which one is on screen — + # `2/4`. Keyed on the parent, so it counts the takes of *this* turn and not + # every node that happens to share a depth, and so it keeps counting them + # after one has been forked onto a branch of its own. + # + # 1/1 for a turn nobody has retaken, which is most of them; the client draws + # no pager for a count of one. That is a different convention from + # `variant_count`, which says 0 for the same case — those two are the + # pre-SP9 pair and SP8 drops them. + take_count: int = 1 + take_index: int = 0 created_at: datetime diff --git a/backend/tests/test_take_parentage.py b/backend/tests/test_take_parentage.py index 9dd9ab4..2f926bf 100644 --- a/backend/tests/test_take_parentage.py +++ b/backend/tests/test_take_parentage.py @@ -246,6 +246,45 @@ def test_takes_under_one_parent_do_not_count_takes_under_its_sibling(client): assert _group_size(under_c1[0].id) == 2, "C1's line counts only its own two" +def _page(client) -> list[dict]: + return client.get(f"/api/adventures/{client.adv_id}").json()["actions"] + + +def test_the_page_carries_the_pager_numbers(client): + """`2/4` arrives with the page, not from a query per message.""" + _play(client) + _retry(client) + _retry(client) + + ai = [a for a in _page(client) if a["type"] == "ai"] + assert len(ai) == 1, "one take is on the path; the others are behind it" + assert ai[0]["take_count"] == 3 + assert ai[0]["take_index"] == 2, "the newest take is the one being read" + + +def test_a_turn_nobody_retook_reads_one_of_one(client): + _play(client) + for action in _page(client): + assert action["take_count"] == 1 + assert action["take_index"] == 0 + + +def test_the_pager_still_counts_a_take_that_was_forked_away(client): + """The 1/3 case, seen from the wire rather than from `attempts.group`.""" + _play(client) + _retry(client) + _retry(client) + _play(client, "press on") + + first_take = _ai_rows(client.adv_id)[0] + _play(client, "a different way", after_id=first_take.id) + + ai = [a for a in _page(client) if a["id"] == first_take.id] + assert ai, "the forked take is what this branch now tells" + assert ai[0]["take_count"] == 3 + assert ai[0]["take_index"] == 0 + + def _take(client, action_id, text): return client.post( f"/api/adventures/{client.adv_id}/actions/{action_id}/takes",