Put the pager's numbers on the page
`2/4` needs the shape of a turn's take group for every message on screen. Asking `attempts.group` per row would put a query behind each one -- the exact cost `variant_count` was cached to avoid, and the reason SP8 could not just drop that column and be done. So one query per page, keyed on the parents the page mentions, and the count and ordinal land on the rows before they are serialised. `take_count` / `take_index` rather than reusing the old pair, because they do not mean the same thing: the old ones cache a coordinate's siblings and say 0 for a turn nobody retook, these count the *turn's* takes across whatever branches they ended up on and say 1/1. SP8 still drops `variant_count` and `variant_index`. Two traps, one of them mine. `parent_id` was not in ACTION_LIST_COLUMNS, so reading it off a windowed row would have been a lazy load per action -- an N+1 hidden behind the thing `load_only` exists to prevent. And the adventure GET does not build `ActionOut` at all: it hands the window to the relationship with `set_committed_value` and lets Pydantic walk it, so patching the three places that do build ActionOut missed the one path every page load takes. The test caught it by reading the wire instead of the helper. 424 tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Dvvqn9ZDR4ixeFPHNbww7
This commit is contained in:
committed by
Parth
co-authored by
Claude Opus 5
parent
2726840c60
commit
c3c9b310eb
@@ -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,
|
||||
)
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
|
||||
@@ -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",
|
||||
|
||||
Reference in New Issue
Block a user