From f9b836fc0bb6f2ba523e4cd762b6dfa79fa1ed52 Mon Sep 17 00:00:00 2001 From: parththakkar106 Date: Tue, 18 Aug 2026 20:52:36 +0530 Subject: [PATCH] Put the pager back, and let any turn be played again SP7 replaced the pager with chips, on the grounds that a chip could also offer "take this path" while a pager could only step. Driving it by hand said otherwise, and the reason is worth keeping: the chip meant two different things depending on where the reader was standing -- a real switch at the tip, a preview needing a second button above it further back. Two meanings in one control is what made the tree unusable. So: one control that does one thing. Stepping reads a take and nothing else, and it tells the server nothing, because reading is not a decision. The transcript below a take that is not live simply ends -- such a take is a leaf by construction, since whatever was played after the turn was played after the take that *is* live. The decision is made by writing, and `after_id` carries it. One step does reach the server and is still not a fork: a take with a story of its own lives on its own branch, so going there is a branch switch and only the server can say what is underneath. `branch_id` on the take is what tells the two apart without asking first. And a fork button on every turn but the opening. On the AI's it regenerates; on your own it opens the text so you can say something else. What the story made of the old take is kept, on the line it was written on. `selectVariant` and `forkFromAttempt` leave the client. Both endpoints stay -- tested, and `stand_on` is shared with the write path -- but the pager needs neither. 426 backend tests; lint and build clean. Not yet driven by hand: the frontend still has no test runner, so this needs the `--keep` fixture and eyes, exactly as SP7 did. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017Dvvqn9ZDR4ixeFPHNbww7 --- backend/app/routers/adventures.py | 5 +- backend/app/schemas.py | 8 + frontend/src/api.js | 28 +++- frontend/src/index.css | 65 ++++---- frontend/src/pages/Play.jsx | 259 ++++++++++++++++++------------ 5 files changed, 219 insertions(+), 146 deletions(-) diff --git a/backend/app/routers/adventures.py b/backend/app/routers/adventures.py index 8a6d795..a58a797 100644 --- a/backend/app/routers/adventures.py +++ b/backend/app/routers/adventures.py @@ -46,8 +46,10 @@ ACTION_LIST_COLUMNS = ( 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. + # is here to stop. `branch_id` rides along for the same reason: the pager + # reads it to tell a local step from a branch switch. models.Action.parent_id, + models.Action.branch_id, models.Action.created_at, ) @@ -1014,6 +1016,7 @@ def list_variants( index=i, text=row.text, reasoning=row.reasoning, + branch_id=row.branch_id, created_at=row.created_at.isoformat() if row.created_at else None, active=row.live, ) diff --git a/backend/app/schemas.py b/backend/app/schemas.py index 7a47785..268e65e 100644 --- a/backend/app/schemas.py +++ b/backend/app/schemas.py @@ -200,6 +200,11 @@ class ActionOut(ORMModel): # pre-SP9 pair and SP8 drops them. take_count: int = 1 take_index: int = 0 + # Which line this node is on, so the pager can tell the two kinds of step + # apart without asking the server first: a take on this branch is a leaf + # with nothing under it, and showing it is a local matter; a take on another + # branch has a story of its own, and going there is a branch switch. + branch_id: int | None = None created_at: datetime @@ -211,6 +216,9 @@ class VariantOut(BaseModel): index: int text: str reasoning: str | None = None + # See ActionOut.branch_id: it decides whether choosing this take is a local + # step or a branch switch. + branch_id: int | None = None created_at: str | None = None active: bool = False diff --git a/frontend/src/api.js b/frontend/src/api.js index e3f78de..49bf762 100644 --- a/frontend/src/api.js +++ b/frontend/src/api.js @@ -105,10 +105,15 @@ export const api = { // attempts themselves are fetched when the reader actually pages through. listVariants: (advId, actionId) => request(`/adventures/${advId}/actions/${actionId}/variants`), - selectVariant: (advId, actionId, index) => - request(`/adventures/${advId}/actions/${actionId}/variant`, { - method: 'POST', body: JSON.stringify({ index }), - }), + // The same endpoint under the name the pager uses. "Take" is what the UI + // calls one of these now, and the vocabulary is worth keeping straight — + // `variant` belongs to the pre-tree pair of columns SP8 drops. + listTakes: (advId, actionId) => + request(`/adventures/${advId}/actions/${actionId}/variants`), + // No `selectVariant` / `forkFromAttempt` here any more. Both endpoints still + // exist and are tested, but the pager needs neither: stepping between takes + // tells the server nothing, and what used to be "take this path" is now + // whatever the reader writes next, carried by `after_id` on the turn itself. // The story tree (Phase 14). One request draws the whole shape however many // forks there are. The three that change it answer with the story as it now @@ -122,11 +127,18 @@ export const api = { }), deleteBranch: (advId, branchId) => request(`/adventures/${advId}/branches/${branchId}`, { method: 'DELETE' }), - // Take the story down one attempt. A fork only when it has to be: while the - // attempts are still at the tip they are leaves, and the server switches. - forkFromAttempt: (advId, actionId) => - request(`/adventures/${advId}/actions/${actionId}/fork`, { method: 'POST' }), + // Play a turn again, differently (SP9). An AI turn regenerates; a player's + // own takes the text given. Streams, because it is a turn like any other. + // + // Reaches any turn, not only the newest — which is the whole difference from + // `retry`, and the reason the pager can offer this on every message. + addTake: (advId, actionId, text, handlers, signal) => + streamSSE(`/adventures/${advId}/actions/${actionId}/takes`, { text }, handlers, signal), + // `afterId` names the take the turn is played after. Omitted it means the + // tip, which is every ordinary turn. Naming a take the story moved past is + // what forks a branch — stepping between takes to read them does not, and + // the server is never told about it. sendAction: (advId, payload, handlers, signal) => streamSSE(`/adventures/${advId}/actions`, payload, handlers, signal), retry: (advId, handlers, signal) => streamSSE(`/adventures/${advId}/retry`, {}, handlers, signal), diff --git a/frontend/src/index.css b/frontend/src/index.css index cc40de0..e7ae65f 100644 --- a/frontend/src/index.css +++ b/frontend/src/index.css @@ -426,51 +426,50 @@ button:disabled { opacity: 0.45; cursor: default; transform: none; box-shadow: n } .story .action-tools button:hover { color: var(--text); } -/* The attempts at one AI beat. Stays quiet until hovered — it's a footnote on - the message, not part of the prose. */ -.attempts { +/* The takes of one turn: ‹ 2/4 ›. Stays quiet until hovered — it's a footnote + on the message, not part of the prose. + + Small and unemphatic on purpose. Stepping through takes reads them and does + nothing else, so it should not carry the visual weight of a decision; the + decision is made by writing, which happens in the composer below. */ +.take-pager { display: flex; align-items: center; - gap: 6px; - flex-wrap: wrap; + gap: 2px; margin-top: 6px; font-family: var(--font-ui); opacity: 0.45; transition: opacity 0.15s; } -.story .action:hover .attempts, -.attempts:focus-within { opacity: 1; } -.attempt-chip { - padding: 3px 10px; - font-size: 0.73rem; - line-height: 1.5; - border-radius: 999px; +.story .action:hover .take-pager, +.take-pager:focus-within { opacity: 1; } +.take-pager button { + width: 20px; + height: 20px; + padding: 0; + font-size: 0.9rem; + line-height: 1; color: var(--text-dim); background: transparent; - border: 1px solid var(--border); + border: 1px solid transparent; + border-radius: 4px; } -.attempt-chip:hover:not(:disabled) { color: var(--text); border-color: var(--border-bright); } -.attempt-chip[aria-pressed="true"] { - color: var(--bg); - background: var(--accent); - border-color: var(--accent); - font-weight: 600; +.take-pager button:hover:not(:disabled) { + color: var(--text); + border-color: var(--border-bright); } -.attempt-chip:disabled { opacity: 0.35; cursor: default; } -/* Dashed until hovered: taking a path the story moved past creates a branch, - so it should not look like the same weight of click as browsing one. */ -.take-path { - padding: 3px 10px; - font-size: 0.73rem; - line-height: 1.5; - color: var(--accent); - background: transparent; - border: 1px dashed var(--accent-dim); +.take-pager button:disabled { opacity: 0.3; cursor: default; } +.take-count { + min-width: 30px; + text-align: center; + font-size: 0.72rem; + font-variant-numeric: tabular-nums; + color: var(--text-dim); } -.take-path:hover:not(:disabled) { color: var(--accent-bright); border-style: solid; } -.take-path:disabled { opacity: 0.35; cursor: default; } -.attempt-note { - margin-left: 2px; +/* Only while a take that isn't the live one is on screen: it says what the + next thing typed will do, because that is the click that forks. */ +.take-note { + margin-left: 6px; font-size: 0.72rem; font-style: italic; color: var(--accent-dim); diff --git a/frontend/src/pages/Play.jsx b/frontend/src/pages/Play.jsx index d2d1d9b..6666276 100644 --- a/frontend/src/pages/Play.jsx +++ b/frontend/src/pages/Play.jsx @@ -952,48 +952,59 @@ function WorldStateDrawer({ advId, refreshKey }) { ) } -// The attempts at one turn, and the way onto one the story left behind. +// The takes of one turn: ‹ 2/4 ›, and nothing else. // -// This replaces the ‹ 2/3 › pager, and the reason is not that chips look -// better: a pager can only step between attempts, and stepping has nothing to -// say about the thing the tree makes possible — taking a path the story moved -// past *and keeping both*. Every attempt is its own node now (SP4), so a chip -// is a node, and "take this path" is a fork (SP5). +// SP7 shipped chips instead, on the grounds that a pager can only step between +// takes while a chip could also offer "take this path". Driving it by hand said +// otherwise. The chip meant two different things depending on where the reader +// was standing — a real switch at the tip, a preview needing a second button +// above it — and two meanings in one control is what made the tree unusable. // -// Two cases behind one control. While the turn is the tip its attempts are -// still leaves, so choosing one is a switch and the server restores the state -// that attempt produced. Once the story has moved past, choosing one is a -// local preview — the turns after it were written as a continuation of -// whatever is live — and taking it forks a branch. -function AttemptChips({ advId, action, isLast, busy, preview, onPreview, onSwitched, onForked, onError }) { - const [variants, setVariants] = useState(null) +// So the pager comes back, and stepping is all it does. Stepping is free: it +// tells the server nothing, because reading a take is not a decision. The +// decision is made by *writing* below one, and that is where the branch is +// created (SP9, `after_id`). +// +// One step still reaches the server, and it is not a fork either. A take that +// has a story of its own lives on its own branch, so going there is a branch +// switch — the story below has to change, and only the server can say to what. +// 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 }) { + const [takes, setTakes] = useState(null) const [loading, setLoading] = useState(false) - const count = action.variant_count - const live = action.variant_index + const count = action.take_count + const live = action.take_index const current = preview ? preview.index : live - async function show(next) { - if (next === current || loading || busy) return + async function step(delta) { + const next = current + delta + if (next < 0 || next >= count || loading || busy) return setLoading(true) try { - if (isLast) { + // Fetched once per message, then cached — walking back and forth through + // the takes should not re-hit the server for a list that has not changed. + const list = takes || await api.listTakes(advId, action.id) + if (!takes) setTakes(list) + const target = list[next] + if (target.branch_id !== action.branch_id) { + // It has a story of its own. Only the server knows what is under it. + onPreview(null) + onSwitchedBranch(await api.switchBranch(advId, target.branch_id)) + } else if (next === live) { onPreview(null) - onSwitched(await api.selectVariant(advId, action.id, next)) } else { - // Fetched once per message, then cached — moving back and forth - // between attempts shouldn't re-hit the server. - const list = variants || await api.listVariants(advId, action.id) - if (!variants) setVariants(list) - onPreview(next === live ? null : { + onPreview({ actionId: action.id, index: next, - // The attempt's own node id. A fork is addressed by the node being - // taken, never by its ordinal — the group renumbers whenever an - // attempt is added, and an ordinal held across that points at a - // different take. - attemptId: list[next].id, - text: list[next].text, - reasoning: list[next].reasoning, + // The take's own node id, never its ordinal: the group renumbers + // whenever a take is added, and an ordinal held across that points + // at a different one. This is what `after_id` is given if the reader + // writes from here. + takeId: target.id, + text: target.text, + reasoning: target.reasoning, }) } } catch (err) { @@ -1003,45 +1014,16 @@ function AttemptChips({ advId, action, isLast, busy, preview, onPreview, onSwitc } } - async function take(attemptId) { - if (loading || busy) return - setLoading(true) - try { - const page = await api.forkFromAttempt(advId, attemptId) - onPreview(null) - onForked(page) - } catch (err) { - onError(err.message) - } finally { - setLoading(false) - } - } - + if (count < 2) return null return ( -
- {Array.from({ length: count }, (_, i) => ( - - ))} - {preview?.attemptId != null && ( - - )} +
+ + {current + 1}/{count} + {preview && ( - - the story continued from take {live + 1} - + write below to keep this one )}
) @@ -1524,8 +1506,12 @@ export default function Play() { // (currently "Update from scenario"), which no action count would reflect. const [stateKey, setStateKey] = useState(0) const [inspectActionId, setInspectActionId] = useState(null) - // Read-only browsing of an earlier attempt at a past turn (see AttemptChips). - // One at a time; null when every message is showing its active version. + // Which take is being read, when it is not the live one (see TakePager). + // One at a time; null when every message is showing the take the story tells. + // + // Purely local: the server is not told, because reading a take is not a + // decision. It becomes one when something is written below it, and that is + // what `after_id` carries. const [preview, setPreview] = useState(null) // 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 @@ -1554,6 +1540,15 @@ export default function Play() { () => actions.find((a) => a.type === 'start' || a.type === 'ai')?.id ?? null, [actions], ) + // Where the transcript stops while a take that is not the live one is being + // read. Such a take is a leaf by construction — whatever was played after + // this turn was played after the take that *is* live — so there is nothing + // under it, and showing the rest would attach one line's story to another's + // text. -1 while nothing is being previewed, which is the ordinary case. + const previewCutoff = useMemo( + () => (preview ? actions.findIndex((a) => a.id === preview.actionId) : -1), + [preview, actions], + ) // send() sets streaming to '' before the request goes out; reasoningStream // stays null until reasoning tokens (if any) arrive. Both still at those // values means the request is in flight with nothing to show yet. @@ -1746,14 +1741,28 @@ export default function Play() { function send(type = mode) { const text = input.trim() + // Where the reader is standing. Stepping to a take the story moved past + // told the server nothing; this is the moment it has to be told, and it is + // the moment the branch is made (SP9). + const after_id = preview?.takeId + // The window below belongs to the line being left, so it is re-read rather + // than appended to — same reasoning as `addTake`. + const run = (payload) => runTurn(async (signal) => { + try { + await api.sendAction(id, payload, handleEvent, signal) + } finally { + if (after_id) await resync() + } + }) + setPreview(null) if (type === 'continue') { // Continue never consumes typed text — leave it in the box. - runTurn((signal) => api.sendAction(id, { type: 'continue', text: '' }, handleEvent, signal)) + run({ type: 'continue', text: '', after_id }) return } - const payload = { type: text ? type : 'continue', text } + const payload = { type: text ? type : 'continue', text, after_id } setInput('') - runTurn((signal) => api.sendAction(id, payload, handleEvent, signal)) + run(payload) } function retry() { @@ -1813,8 +1822,14 @@ export default function Play() { }) async function saveEdit() { - const { id: actionId, text } = editing + const { id: actionId, text, fork } = editing setEditing(null) + if (fork) { + // Not an edit at all: the turn is played again with this text, and what + // the story made of the old text is kept on the line it was written on. + addTake(actionId, text) + return + } try { const updated = await api.updateAction(id, actionId, text) setActions((prev) => prev.map((a) => (a.id === actionId ? updated : a))) @@ -1823,6 +1838,42 @@ export default function Play() { } } + // Play a turn again, differently. Anywhere in the story, either kind of node. + // + // The transcript is re-read rather than appended to, which is the difference + // from an ordinary turn: a take above the tip leaves the line it was on and + // the whole window below it belongs to a story this branch no longer tells. + // `handleEvent` appends the new node as it streams; the resync afterwards is + // what drops everything that is no longer under it. + function addTake(actionId, text) { + setPreview(null) + runTurn(async (signal) => { + try { + await api.addTake(id, actionId, text, handleEvent, signal) + } finally { + await resync() + } + }) + } + + // Re-read the newest window from the server. + // + // For a turn that left the line it was on: `handleEvent` appends the new node + // as it streams, and everything already on screen below the take belongs to a + // story this branch no longer tells. Only the server can say what replaces + // it. A failed resync leaves the transcript stale rather than wrong, so it is + // swallowed — the next page load settles it. + async function resync() { + try { + const adv = await api.getAdventure(id) + setActions(adv.actions) + setTotal(adv.action_count ?? adv.actions.length) + setHasMore(adv.actions.length < (adv.action_count ?? adv.actions.length)) + // The branch, the script state and the world state can all have moved. + setStateKey((k) => k + 1) + } catch { /* stale beats wrong */ } + } + async function removeAction(actionId) { try { await api.deleteAction(id, actionId) @@ -1891,6 +1942,8 @@ export default function Play() {
)} {actions.map((action, i) => { + // Below the take being read there is nothing on this line yet. + if (previewCutoff !== -1 && i > previewCutoff) return null const isPlayer = PLAYER_TYPES.includes(action.type) // A player action opens a new turn, so that's where the ornamental // break belongs — never above the very first line on the page. @@ -1926,36 +1979,19 @@ export default function Play() { {action.type === 'ai' && !previewing && ( )} - {action.type === 'ai' && action.variant_count > 1 && ( - { - // Matched on the action we asked about, not on the one - // that came back. Since the story tree made every - // attempt its own row (phase 14 SP4), switching moves - // the story onto a *different* row rather than - // rewriting this one, so the reply carries a new id. - setActions((prev) => prev.map( - (a) => (a.id === action.id ? updated : a))) - // Switching takes is not only a change of text. The - // server puts back that attempt's script and world - // state, withdraws the memory that hung off the - // coordinate, and rewinds both cursors — none of which - // the panels can see, because they key on - // `actions.length` and the story is the same length it - // was. Same class of bug as a branch switch, which - // `adoptWindow` already bumps this for. - setStateKey((k) => k + 1) - }} - onError={(message) => setToast({ text: message, isError: true })} - /> - )} + {/* On every kind of node, not only the AI's: a player's own + turn can be played again too (SP9), so it can have takes + to step through. The pager draws nothing for a count of + one, which is most turns. */} + setToast({ text: message, isError: true })} + /> {!busy && ( {action.type === 'ai' && ( @@ -1964,6 +2000,21 @@ export default function Play() { )} + {/* 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. */} + {action.type !== 'start' && ( + + )} )}