From 6964aeabb618c6f821ed4df6074a8460942d8302 Mon Sep 17 00:00:00 2001 From: Tim McCarthy Date: Sat, 13 Jun 2026 08:25:06 -0700 Subject: [PATCH] Add undoable rollback/redo (Phase 2) Rolling back is now non-destructive: instead of truncating the future, it restores the target checkpoint's snapshot and points Game.rollback_head_checkpoint_id at it. Later checkpoints/events are kept and rendered greyed; rolling forward to one redoes the rollback (same endpoint). The abandoned future is discarded only when a new action is taken from a rolled-back state -- maintain_rollback_history truncates it, clears the head, and bumps rollback_timeline_version, now the sole trigger for the frontend log reset. - Game.rollback_head_checkpoint_id (nullable; in SNAPSHOT_EXCLUDE) + Alembic migration. - crud_rollback: rollback_to_checkpoint sets the head (None at latest); _discard_future + head-clear + version-bump moved into the next scene action's capture. - EventLog.svelte: greyed future, redo (forward) affordance, a "rolled back" banner; dropped the now-misleading destructive confirm. - Tests rewritten for head-pointer semantics (+ redo, action-after- rollback truncation, scene-end head clear); HTTP integration test extended to drive the full cycle over the wire. Verified end-to-end in a browser: rollback greys the future and reverts live state (captaincy/ranks), redo restores it, no errors. Co-Authored-By: Claude Opus 4.8 --- TODO.md | 7 +- frontend/src/components/EventLog.svelte | 38 ++++++-- src/pirats/crud_rollback.py | 80 ++++++++++++----- .../e6294f6f8a81_add_rollback_head_pointer.py | 32 +++++++ src/pirats/models.py | 1 + tests/test_game.py | 90 ++++++++++++++++--- 6 files changed, 204 insertions(+), 44 deletions(-) create mode 100644 src/pirats/migrations/versions/e6294f6f8a81_add_rollback_head_pointer.py diff --git a/TODO.md b/TODO.md index b910b0a..2523cbc 100644 --- a/TODO.md +++ b/TODO.md @@ -1,5 +1,5 @@ ## Major features -- [~] **Rollback of game state.** *Phase 1 implemented* (full-state JSON snapshots in a `Checkpoint` table, captured per action by the broadcast middleware, restored by `crud_rollback.rollback_to_checkpoint`; scene-confined; server-enforced for Admins and Deep players; event-log buttons in `EventLog.svelte`). **Phase 1 truncates the future immediately (no undo).** *Phase 2 (remaining):* the greyed-out / undoable redo UX — roll back without discarding the future, grey later entries, and only commit the truncation when a new action is posted. The schema is already forward-compatible for this (add `Game.rollback_head_checkpoint_id`). +- [x] **Rollback of game state.** Full-state JSON snapshots in a `Checkpoint` table, captured per action by the broadcast middleware, restored by `crud_rollback.rollback_to_checkpoint`; scene-confined; server-enforced for Admins and Deep players. Rolling back is non-destructive — it moves a head pointer (`Game.rollback_head_checkpoint_id`), so later log entries grey out and can be redone (roll forward); the abandoned future is only discarded when a new action is taken. UI is the per-event rollback/redo buttons + greyed entries + "rolled back" banner in `EventLog.svelte`. ## Words Words Words @@ -10,3 +10,8 @@ ## Security - [ ] **Defensive coding.** While we trust our players, this app is exposed on the open internet. I'd like to take basic precautions like making sure we're safe against SQL injection (SQLModel should handle this?), doing some basic sanitizing of free inputs from players (valid unicode only + generous character limit should be enough), and making sure that every game action API call is specific and constrained enough that it can't just do arbitrary database updates. + +## Cleanup + +- [ ] **Look for dead code.** While implementing the rollback feature, we discovered /scene/rollback, an unused and buggy single-card-play rollback route that was added but never hooked up to the UI. Look for other code that matches that pattern -- orphan functions and routes that never get called. +- [ ] **Clean up ruff E701 warnings** diff --git a/frontend/src/components/EventLog.svelte b/frontend/src/components/EventLog.svelte index 6547f97..bde1085 100644 --- a/frontend/src/components/EventLog.svelte +++ b/frontend/src/components/EventLog.svelte @@ -39,6 +39,9 @@ // Rollback buttons appear during a scene, for Admins and Deep players (server-enforced too). $: canRollback = state.game?.phase === 'scene' && (state.player?.is_admin || state.player?.role === 'deep'); + // When set, the game is at a rolled-back state; events past this checkpoint are the + // greyed, still-undoable future. Rolling forward to one of them redoes the rollback. + $: head = state.game?.rollback_head_checkpoint_id ?? null; $: mergePolled(state.events); @@ -46,8 +49,9 @@ // otherwise events the player paged back to would vanish as the window slides. async function mergePolled(polled) { if (!polled) return; - // A rollback truncates the log server-side; the version bump tells us to drop - // everything we'd accumulated (those future events no longer exist) and reseed. + // Plain rollback/redo keeps every event (only `head` moves); but taking a new + // action from a rolled-back state truncates the abandoned future and bumps the + // version, which tells us to drop what we'd accumulated and reseed. const version = state.game?.rollback_timeline_version ?? 0; if (timelineVersion !== null && version !== timelineVersion) { events = []; @@ -122,8 +126,10 @@ return new Date(ts * 1000).toLocaleTimeString([], { hour: '2-digit', minute: '2-digit' }); } + // Both rollback (to a past event) and redo (to a greyed future event) are the same + // call — it just moves the head. It's reversible until someone takes a new action, + // so no confirm; the greyed future makes the state obvious. async function rollback(event) { - if (!confirm(`Roll the game back to just after "${event.message}"? Everything after it will be undone.`)) return; error = ''; try { await apiRequest( @@ -147,6 +153,11 @@ {#if error}
{error}
{/if} + {#if head !== null} +
+ ↩ Rolled back — greyed entries are undone. Click one to redo, or take an action to make it permanent. +
+ {/if}
{#if events.length}
@@ -159,16 +170,17 @@ {/if}
{#each events as event (event.id)} -
+ {@const isFuture = head !== null && event.checkpoint_id > head} +
{KIND_ICONS[event.kind] || KIND_ICONS.info} {event.message} {formatTime(event.timestamp)} {#if canRollback && event.checkpoint_id > 0} + >{isFuture ? '↪' : '↩'} {/if}
{/each} @@ -286,6 +298,20 @@ color: var(--text); font-size: 0.8rem; } + .log-rolledback { + margin: 6px 8px 0; + padding: 6px 10px; + background: color-mix(in srgb, var(--accent) 12%, transparent); + border: 1px solid var(--accent); + border-radius: var(--radius-md); + color: var(--text); + font-size: 0.78rem; + line-height: 1.3; + } + .log-entry.greyed { + opacity: 0.45; + font-style: italic; + } .log-icon { font-size: 0.9rem; } diff --git a/src/pirats/crud_rollback.py b/src/pirats/crud_rollback.py index ca975cf..39de19b 100644 --- a/src/pirats/crud_rollback.py +++ b/src/pirats/crud_rollback.py @@ -15,6 +15,13 @@ Each event carries a `checkpoint_id` instead (see models.py): None = in-flight (created since the last capture; the next capture tags it) > 0 = a live rollback target within the current scene 0 = sealed/historical (a past scene or non-scene event; never rollback-able) + +Rolling back is non-destructive: it restores a checkpoint's snapshot into the live +tables and points `Game.rollback_head_checkpoint_id` at it, leaving the later +checkpoints/events in place (the frontend greys them). Rolling forward to a later +checkpoint "undoes" the rollback. The abandoned future is only discarded when a new +action is taken from a rolled-back state (see maintain_rollback_history), which is +what commits the new timeline. """ import json @@ -26,7 +33,7 @@ from .models import Game, Player, Obstacle, Challenge, Vote, GameEvent, Checkpoi # Game columns that are rollback *control* state, not gameplay — never snapshotted, # and preserved (not overwritten) when a snapshot is restored. -SNAPSHOT_EXCLUDE = {"rollback_timeline_version"} +SNAPSHOT_EXCLUDE = {"rollback_timeline_version", "rollback_head_checkpoint_id"} # Additionally never reassigned on restore (identity / secrets that don't change). _APPLY_SKIP = SNAPSHOT_EXCLUDE | {"id", "admin_key"} @@ -117,23 +124,62 @@ def seal_and_purge(db: Session, game: Game) -> None: for cp in db.exec(select(Checkpoint).where(Checkpoint.game_id == game.id)).all(): db.delete(cp) + game.rollback_head_checkpoint_id = None + db.add(game) db.commit() +def _discard_future(db: Session, game: Game, after_checkpoint_id: int) -> None: + """Delete the checkpoints (and their events) created after a checkpoint — the + abandoned timeline when a new action is taken from a rolled-back state.""" + for e in db.exec( + select(GameEvent).where( + GameEvent.game_id == game.id, + GameEvent.checkpoint_id > after_checkpoint_id, + ) + ).all(): + db.delete(e) + for c in db.exec( + select(Checkpoint).where( + Checkpoint.game_id == game.id, + Checkpoint.id > after_checkpoint_id, + ) + ).all(): + db.delete(c) + + def maintain_rollback_history(db: Session, game: Game) -> None: """Called after every successful mutation: snapshot the state while a scene is in progress, otherwise clear the (now-irrelevant) rollback history. This is the only decision the middleware makes; kept here so it can be unit-tested without HTTP.""" - if game.phase == "scene": - capture_checkpoint(db, game) - else: + if game.phase != "scene": seal_and_purge(db, game) + return + + # A new action taken from a rolled-back state commits the new timeline: discard the + # abandoned future first (its events are tagged with checkpoints past the head), + # then clear the head and bump the version so the frontend drops those events. + # (SQLite may reuse a discarded checkpoint's id for the next capture; that's safe + # since every event referencing it was just deleted.) + head = game.rollback_head_checkpoint_id + if head is not None: + _discard_future(db, game, head) + game.rollback_head_checkpoint_id = None + game.rollback_timeline_version += 1 + db.add(game) + db.commit() + + capture_checkpoint(db, game) def rollback_to_checkpoint( db: Session, game: Game, player: Player, checkpoint_id: int ) -> Tuple[bool, str]: - """Restore the game to the given checkpoint and discard everything after it. + """Restore the game to the given checkpoint and point the head at it. + + Non-destructive: later checkpoints/events are kept (the frontend greys them) so the + rollback can be undone by rolling forward again; the same call serves both rollback + and redo. Rolling to the latest checkpoint clears the head (back to live play). Server-enforced: only during a scene, only by an Admin or a Deep player, and only to a checkpoint belonging to this game (which guarantees the current scene, since @@ -149,25 +195,11 @@ def rollback_to_checkpoint( apply_game_state(db, game, cp.state_json) - # Truncate the future: events and checkpoints created after the target. Sealed - # history (0) and the target's own events (== checkpoint_id) are kept. - for e in db.exec( - select(GameEvent).where( - GameEvent.game_id == game.id, - GameEvent.checkpoint_id > checkpoint_id, - ) - ).all(): - db.delete(e) - for c in db.exec( - select(Checkpoint).where( - Checkpoint.game_id == game.id, - Checkpoint.id > checkpoint_id, - ) - ).all(): - db.delete(c) - - # Bump so the frontend EventLog drops the events it had accumulated. - game.rollback_timeline_version += 1 + latest = db.exec( + select(Checkpoint).where(Checkpoint.game_id == game.id) + .order_by(Checkpoint.id.desc()) + ).first() + game.rollback_head_checkpoint_id = None if checkpoint_id == latest.id else checkpoint_id db.add(game) db.commit() return True, "Rolled back." diff --git a/src/pirats/migrations/versions/e6294f6f8a81_add_rollback_head_pointer.py b/src/pirats/migrations/versions/e6294f6f8a81_add_rollback_head_pointer.py new file mode 100644 index 0000000..eff987f --- /dev/null +++ b/src/pirats/migrations/versions/e6294f6f8a81_add_rollback_head_pointer.py @@ -0,0 +1,32 @@ +"""add rollback head pointer + +Revision ID: e6294f6f8a81 +Revises: 480e6d83109e +Create Date: 2026-06-13 06:08:17.170588 + +""" +from alembic import op +import sqlalchemy as sa +import sqlmodel + + +revision = 'e6294f6f8a81' +down_revision = '480e6d83109e' +branch_labels = None +depends_on = None + + +def upgrade() -> None: + # ### commands auto generated by Alembic - please adjust! ### + with op.batch_alter_table('game', schema=None) as batch_op: + batch_op.add_column(sa.Column('rollback_head_checkpoint_id', sa.Integer(), nullable=True)) + + # ### end Alembic commands ### + + +def downgrade() -> None: + # ### commands auto generated by Alembic - please adjust! ### + with op.batch_alter_table('game', schema=None) as batch_op: + batch_op.drop_column('rollback_head_checkpoint_id') + + # ### end Alembic commands ### diff --git a/src/pirats/models.py b/src/pirats/models.py index b3f68b5..a53dc6d 100644 --- a/src/pirats/models.py +++ b/src/pirats/models.py @@ -13,6 +13,7 @@ class Game(SQLModel, table=True): extra_obstacles: int = Field(default=0) # Permanent increase to required obstacles (from Jokers drawn as Obstacles) deck_cards: str = Field(default="[]") # JSON string of list of card codes, e.g. ["AS", "10D", "KH", ...] rollback_timeline_version: int = Field(default=0) # Bumped whenever a rollback truncates events; lets the frontend reset its accumulated log. NEVER snapshotted (see crud_rollback.SNAPSHOT_EXCLUDE) + rollback_head_checkpoint_id: Optional[int] = Field(default=None) # When set, the game is viewing a rolled-back state at this checkpoint; events after it are the greyed, still-undoable future. None = live at the latest. NEVER snapshotted. completed_crew_1: bool = Field(default=False) # Steal a Ship completed_crew_2: bool = Field(default=False) # Choose a Captain completed_crew_3: bool = Field(default=False) # Commit Piracy diff --git a/tests/test_game.py b/tests/test_game.py index 3435497..a3ab364 100644 --- a/tests/test_game.py +++ b/tests/test_game.py @@ -2098,26 +2098,66 @@ def test_rollback_round_trip_restores_state(session): assert crud.get_game_deck(game) == saved_deck assert session.exec(select(Challenge).where(Challenge.game_id == game.id)).all() == [] -def test_rollback_truncates_future_and_bumps_version(session): - game, deep, (p2,) = make_scene_game(session) +def _scene_with_two_checkpoints(session, num_pirats=1): + """A scene with sealed history and two action checkpoints (cp1 'First', cp2 'Second').""" + game, deep, pirats = make_scene_game(session, num_pirats=num_pirats) crud.seal_and_purge(session, game) - crud.add_game_event(session, game.id, "First", kind="info") cp1 = crud.capture_checkpoint(session, game) - v0 = game.rollback_timeline_version - crud.add_game_event(session, game.id, "Second", kind="info") cp2 = crud.capture_checkpoint(session, game) - assert {c.id for c in _checkpoints(session, game)} == {cp1.id, cp2.id} + return game, deep, pirats, cp1, cp2 + +def test_rollback_keeps_future_and_sets_head(session): + game, deep, _, cp1, cp2 = _scene_with_two_checkpoints(session) + v0 = game.rollback_timeline_version ok, msg = crud.rollback_to_checkpoint(session, game, deep, cp1.id) assert ok, msg session.refresh(game) - # The future checkpoint and its event are gone; cp1 and its event survive. - assert [c.id for c in _checkpoints(session, game)] == [cp1.id] - msgs = {e.message for e in _events(session, game)} - assert "First" in msgs and "Second" not in msgs + # Non-destructive: the later checkpoint and its event are kept (the frontend greys + # them); head points at cp1 and the version is unchanged. + assert {c.id for c in _checkpoints(session, game)} == {cp1.id, cp2.id} + assert {e.message for e in _events(session, game)} >= {"First", "Second"} + assert game.rollback_head_checkpoint_id == cp1.id + assert game.rollback_timeline_version == v0 + +def test_redo_after_rollback_clears_head(session): + game, deep, _, cp1, cp2 = _scene_with_two_checkpoints(session) + + crud.rollback_to_checkpoint(session, game, deep, cp1.id) + session.refresh(game) + assert game.rollback_head_checkpoint_id == cp1.id + + # Rolling forward to the latest checkpoint redoes the rollback and returns to live play. + ok, msg = crud.rollback_to_checkpoint(session, game, deep, cp2.id) + assert ok, msg + session.refresh(game) + assert game.rollback_head_checkpoint_id is None + +def test_action_after_rollback_truncates_and_bumps(session): + game, deep, _, cp1, cp2 = _scene_with_two_checkpoints(session) + v0 = game.rollback_timeline_version + + crud.rollback_to_checkpoint(session, game, deep, cp1.id) + + # A new action from the rolled-back state commits the new timeline: maintain_rollback_history + # discards the abandoned future, clears the head, bumps the version, and captures anew. + crud.add_game_event(session, game.id, "New branch", kind="info") + crud.maintain_rollback_history(session, game) + session.refresh(game) + + events = _events(session, game) + msgs = {e.message for e in events} + assert "Second" not in msgs # abandoned future discarded + assert {"First", "New branch"} <= msgs + # cp1 + the freshly captured checkpoint (note: SQLite may reuse cp2's freed id, which + # is safe — every event that referenced it was deleted in the same step). + assert len(_checkpoints(session, game)) == 2 + new_event = next(e for e in events if e.message == "New branch") + assert new_event.checkpoint_id and new_event.checkpoint_id > 0 + assert game.rollback_head_checkpoint_id is None assert game.rollback_timeline_version == v0 + 1 def test_rollback_permissions(session): @@ -2151,16 +2191,23 @@ def test_scene_end_seals_and_purges(session): crud.seal_and_purge(session, game) crud.add_game_event(session, game.id, "Mid-scene", kind="info") cp = crud.capture_checkpoint(session, game) - assert len(_checkpoints(session, game)) == 1 + crud.add_game_event(session, game.id, "Later", kind="info") + crud.capture_checkpoint(session, game) # a later checkpoint so the rollback sets a head + crud.rollback_to_checkpoint(session, game, deep, cp.id) # leave with a head set + session.refresh(game) + assert game.rollback_head_checkpoint_id == cp.id + assert len(_checkpoints(session, game)) == 2 # Leaving the scene runs the non-scene branch: history is purged and sealed. game.phase = "between_scenes" session.add(game) session.commit() crud.maintain_rollback_history(session, game) + session.refresh(game) assert _checkpoints(session, game) == [] assert all(e.checkpoint_id == 0 for e in _events(session, game)) + assert game.rollback_head_checkpoint_id is None # The now-stale checkpoint id is rejected (back in a scene, even). game.phase = "scene" session.add(game) @@ -2211,13 +2258,30 @@ def test_rollback_middleware_capture_and_route_end_to_end(): assert session.get(Obstacle, obs.id).current_value == 10 assert len(_checkpoints(session, game)) == 2 # floor + the one the middleware took - # Roll back to the floor over HTTP. + # Roll back to the floor over HTTP: state reverts, but the later checkpoint is + # kept (greyed/undoable) and the head points at the floor. r = client.post(f"/api/game/{game.id}/player/{deep.id}/rollback", data={"checkpoint_id": floor.id}) assert r.status_code == 200 session.expire_all() assert session.get(Obstacle, obs.id).current_value == 5 - assert [c.id for c in _checkpoints(session, game)] == [floor.id] + assert len(_checkpoints(session, game)) == 2 + session.refresh(game) + assert game.rollback_head_checkpoint_id == floor.id + v_before = game.rollback_timeline_version + + # A new action from the rolled-back state (the floor restored the open challenge + # and the played card to p2's hand): the middleware truncates the abandoned future, + # clears the head, and bumps the version. + r = client.post(f"/api/game/{game.id}/player/{p2.id}/play-card", + data={"obstacle_id": obs.id, "card_code": "10D"}) + assert r.status_code == 200 + session.expire_all() + session.refresh(game) + assert session.get(Obstacle, obs.id).current_value == 10 + assert game.rollback_head_checkpoint_id is None + assert game.rollback_timeline_version == v_before + 1 + assert len(_checkpoints(session, game)) == 2 # floor + the new branch's checkpoint finally: main_module.engine = saved_engine app.dependency_overrides.clear()