Slice/m3.7a suggestions - #405
Open
failerko wants to merge 59 commits into
Open
Conversation
Generalizes the C2 segment-isolation contract beyond <state>: parseSuggestionsBlock extracts chip items independently so a malformed suggestions block never blocks state parsing or vice versa, and stripTrailingBlocks (replacing stripStateBlock) cuts prose at whichever trailing block appears first so the reader never renders raw suggestions XML as prose.
extractSegment's truncation fallback previously read to end-of-string, so a dropped closing tag on <state> (or an inner field like <summary>) silently swallowed an adjacent <suggestions> block into a persisted field with failures:[] — data corruption on the ordinary single-dropped-tag path, not just a display glitch; boundedEnd now caps recovery at the next sibling root tag, applied consistently in extractSegment and stripTrailingBlocks' sliceBlock. Also adds regression coverage for the <item> tag collision between transfers and suggestions (currently safe only because both parsers guard on a required attribute), merges the misplaced nested stripTrailingBlocks describe block into its top-level counterpart, and corrects a tags.ts comment that asserted an order dependency the Math.min cut doesn't have.
buildStorySettings now takes mode first and seeds suggestionCategories from DEFAULT_SUGGESTION_CATEGORIES[mode] when the app default is empty, flips suggestionsEnabled true by default, and falls back to 'creative' (the wizard's actual starting mode) when resetting a draft with no definition yet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XjvLADCdcER2xCQ5xmhbGb
…onstant buildStorySettings now takes mode plus a single app-settings object and reads the per-mode palette from defaultSuggestionCategories[mode] (per data-model.md and story-settings.md), falling back to DEFAULT_SUGGESTION_CATEGORIES[mode] only when that stored palette is empty; the prior plan read a mode-agnostic column nothing writes, leaving the app-level seed dead and silently ignoring a future per-mode editor's saves. Collapsing embeddingModelId/embeddingProviderId/defaultStorySettings/defaultSuggestionCategories into one parameter keeps the signature at two args since both callers already hold the whole app-settings snapshot. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XjvLADCdcER2xCQ5xmhbGb
…ixture Draft rows do carry a definition (both insert paths always write one); reworded the comment to the real reason for the nullable-mode fallback and renamed the test off the false premise. hydrateCurrentDefaults() now seeds a palette distinct from DEFAULT_SUGGESTION_CATEGORIES so the reset test can tell "seeded from the app store" apart from "fell back to the constant" — previously both produced the same array. Swapped the dev-seed story fixture's suggestion-category colors from raw hex to curated slot keys so it exercises the same color path the chip strip renders against. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XjvLADCdcER2xCQ5xmhbGb
Category ids stay off IdBiMap since they aren't an LLM-facing prefix, so each emission gets its own cat1..catN ref map instead, and suggestionsFire is caller-supplied like piggybackFires so buildGenerationContext only renders the slots it's told to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XjvLADCdcER2xCQ5xmhbGb
The fragment no longer says "after the state block" since state emission is independently gated and often absent (bare narrative fold, and the classifier fold Task 6 will reuse this in has no prose block at all); it now opens the same way state-emission.ts does. buildGenerationContext also re-derives suggestionsFire from the actual slot count instead of trusting the caller's flag alone, so a caller passing true against an all-disabled palette still omits the fragment rather than rendering an empty pick list. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XjvLADCdcER2xCQ5xmhbGb
Dropping "after the state block" also dropped its true half: the fragment now forbids mid-prose emission explicitly, since stripTrailingBlocks cuts prose at the earliest trailing-tag occurrence and a model burying <suggestions> mid-narrative would silently truncate the story after it. resolveSuggestionEmission's boolean is renamed fires -> settingsAllowEmission so a future caller can't mistake the settings-level half for the full run-level gate by just reading the field name at the call site. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XjvLADCdcER2xCQ5xmhbGb
Narrative phase now resolves suggestion emission from settings, requests <suggestions> only when the tagged state block is also firing, parses it independently of <state>, drops refs that don't resolve to a real category, and folds captured chips onto the same createStoryEntry metadata so CTRL-Z removes them with the entry. The captured signal keys on items actually resolved, not blockFound, so a bare literal never reads as success. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XjvLADCdcER2xCQ5xmhbGb
Both settings writers validate through storySettingsSchema.parse before currentStoryStore.set, so a partial settings object never reaches the narrative phase in production; reverted the defensive `??` fallback and instead moved the 10 stale test fixtures onto a STORY_SETTINGS_DEFAULTS-derived base, which was the actual gap. Persisted chips now clamp to suggestionCount so an over-emitting model can't override it, the parse-failure warning now also fires on a partial category-ref drop, and coverage gained a prompt-contains-suggestions control plus the state-fails/suggestions-succeed combination. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XjvLADCdcER2xCQ5xmhbGb
Extends fallbackClassifierSchema with a .catch([])-wrapped suggestions field so structured output degrades a malformed chip array without failing the scene-state parse; the phase only asks for it when settings allow emission and chips aren't already captured from the narrative fold, keeping shouldFallbackFire purely state-driven. Fixes e2e/harness/mock-llm.ts to match either schema shape by registering both as the same per-turn-classifier agent, since the base block is no longer a substring of the extended one.
Covers the row-3 clobber path with a test that pre-seeds narrative-fold chips and confirms the metadata spread preserves them (mutation-verified against dropping ...tail.metadata, which the full suite previously let through unnoticed). Adds a classifier.suggestions_parse_failed warn on zero-captured or dropped refs, noting a wiped .catch([]) array is indistinguishable from a genuinely-empty one on this path. Restores the macro's "one or two sentences" length guidance to the classifier prompt and pins its positive render (slot list, count, disabled-category exclusion).
Extracts the resolve-ref/clamp-to-count/measure-drops math both per-turn folds duplicated into resolveSuggestionItems in lib/piggyback/suggestion-slots.ts, so Task 7's refresh pipeline has one place to call rather than a third copy; each fold keeps its own warn condition and delta sink since those genuinely differ. Splits the mock harness's two classifier reply shapes into distinct STRUCTURED_AGENTS names (base vs per-turn-classifier-suggestions) so setStructured can target either independently, since the shared name let one override silently apply regardless of which schema a request actually used. Verified empirically that classifier.spec.ts's seeded story has suggestions enabled and the extended schema is what fires, contradicting the assumption that the rename was a no-op there, and widened its assertion accordingly. Adds "distinct" to the classifier prompt's chip-count line for vocabulary parity with suggestion-emission.ts.
Caller-supplied run parameters ride a new RunCtx/RunState/PhaseContext `inputs` field rather than `intermediates`, per generation-pipeline.md's inputs-vs-intermediates split (intermediates are phase-to-phase scratch); a chained successor does not inherit them since it has its own context. The emission is a structured JSON call on the `suggestion` agent, so the template carries its own JSON-field instruction instead of including macro_suggestion_emission, which instructs the narrative fold's tagged `<suggestions>` block, and its schema deliberately omits `.catch([])` so a malformed array buys a re-ask and then the strip's error state rather than a silent no-chip success. The metadata delta stamps `source: 'ai_classifier'` — the existing generic model-output source the narrative reply already uses — and anchors `entry_id` to the target so a sparing reversal spares the chips; being no-gate, the phase re-reads the row after the call, because CTRL-Z and rollback are free to run during it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XjvLADCdcER2xCQ5xmhbGb
A field-wise undo partial cannot express "this column was null" — a non-optional key's null sentinel decodes to a null VALUE, so reversing the empty-state re-roll on a system or legacy entry left an unparseable metadata blob that also stuck, since the next re-roll saw a truthy object and skipped the scene floor. The story-entries update handler now emits a null partial when the pre-value was null, and reverse-replay reads a null partial on a schema-backed column as the whole-column restore a schema-less column already takes; nothing else can produce that shape, so every other column's encoding is untouched. Also pins the refresh schema's deliberate lack of `.catch([])` against the classifier fold's, which the mocked phase tests could not see, and splits the reused target-missing log kind so the live-reversal case is distinguishable in diagnostics. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XjvLADCdcER2xCQ5xmhbGb
The per-turn fallback classifier and suggestion-refresh asked for the same chip contract in ~500 near-identical characters that had to be edited in lockstep; both now include a second staticContent macro, leaving macro_suggestion_emission to instruct the narrative fold's tagged block — two macros for the two output contracts, rather than the collapse Task 6 correctly refused. Verified byte-equivalent by diffing both surfaces' renders before and after in both suggestionsFire states: the only change is the classifier's lead-in losing its "Also", which no longer reads as a second instruction now that the sentence is shared. Retargets a comment that cited a reader-composer.md section which does not exist (the phrase is the slice doc's) at generation-pipeline.md's Abort contract, and logs the zero-enabled-categories exit so every quiet completion is distinguishable in diagnostics. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XjvLADCdcER2xCQ5xmhbGb
Makes cancelCopy an exhaustive switch (was a ternary defaulting silently to "Cancel generation") so future phases fail to compile instead of shipping wrong copy; verified by temporarily adding a dummy union member. Copy stays hard-coded to match the file's existing convention, with a triage.md entry tracking the i18n debt already noted for EntryCard.
The state prop carries five of canon's six states: `hidden` means the strip never mounts, so the route owns it by not rendering, and chip taps hand back only the prose because forcing composer mode to Free is the route's job. The pulse splits web/native like Skeleton: Reanimated's worklet plugin doesn't run under the web bundler, so a dependency-array-less useAnimatedStyle throws into an error boundary — EntryCard's identical Pulsing has the same latent break on every web runtime. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XjvLADCdcER2xCQ5xmhbGb
Collapse is orthogonal to phase because the chrome row survives it: under one union a refresh fired while collapsed forced the route to either un-collapse or drop the busy signal, and collapsed+loading now renders a hidden body over a busy chrome row. Empty-state drops the chrome refresh — the body's Generate is the same action — but restores it when collapsed hides that button. Correcting my prior commit body: the EntryCard Pulsing break is Storybook-only, since Metro applies the worklet plugin to the web export too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XjvLADCdcER2xCQ5xmhbGb
Drops the Metro parenthetical I never verified, but keeps the Reanimated throw because it reproduces: EntryCard's unguarded copy leaves an empty #storybook-root with that exact ReanimatedError while `vitest --project storybook` reports 20/20, so a green smoke run is not evidence that a story renders. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XjvLADCdcER2xCQ5xmhbGb
RN-Web's Pressable already applies `pointerEvents: 'box-none'` for disabled and gates the responder claim, press, and keyboard paths on it, so the inline style only duplicated the library; the rn-primitives lesson it cited covers Radix-wrapped triggers, not a bare Pressable. Verified by clicking: locked chips fire nothing, visible and orphan chips each fire once. Covers the loading-with-no-chips branch with a story, names the empty-state refresh rule positively, and gates the two body buttons on `locked` like the rest of the file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XjvLADCdcER2xCQ5xmhbGb
isGenerating in both routes now excludes the no-gate refresh kind: per-turn does not block on a refresh, so counting one as a turn swapped Send for Cancel, gated undo/redo, and raised the streaming placeholder over a branch that was not streaming. The strip keeps its band full-bleed and takes the reader's 860px measure through a new contentClassName, mirroring the composer's chrome-outside/measure-inside structure; leaving the branch aborts an in-flight refresh per the branch-switch edge case. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XjvLADCdcER2xCQ5xmhbGb
A refresh outlives its target: per-turn does not block on it, so a turn can land mid-run, move the tail, and trip the reset effect — after which a late 'failed' painted the error over chips belonging to an entry the refresh never touched. Verified red then green by driving the real app. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XjvLADCdcER2xCQ5xmhbGb
The entry-id comparison already subsumes the branch one, so the comment now says the second ref is deliberate legibility rather than a case the first misses — a future reader shouldn't have to derive id uniqueness to know which one is load-bearing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XjvLADCdcER2xCQ5xmhbGb
Fixes the two blockers task 10 found: the seed's assignments had no 'suggestion' target (resolveModel returned no-profile-assigned, halting the refresh pre-flight), so it now reuses the classifier profile since resolveModel keys purely on assignments[target] -> profile id with no kind coupling; and mock-llm.ts's STRUCTURED_AGENTS had no entry for the refresh pipeline's schema (already exported from lib/pipeline), so an unmatched request fell through to {} and failed every refresh case as a provider error instead of exercising it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XjvLADCdcER2xCQ5xmhbGb
Two review polish items: PersistedSuggestions was hand-duplicating NonNullable<EntryMetadata['nextTurnSuggestions']>, so a future field rename or new source variant would silently diverge instead of failing typecheck; and the five-phase journey test now wraps each phase in test.step() so a failure names its phase in the reporter/trace instead of reading as an unlocated failure somewhere in one long test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XjvLADCdcER2xCQ5xmhbGb
Corrects data-model.md's suggestion-category color (curated slot key OR raw hex, palette fixed not theme-derived) and id (stable handle, not necessarily a uuid), reader-composer.md's classifier-fold wire shape, settings-tab routing, loading/disabled state split, and empty-state chrome-refresh visibility, color-picker.md's stale array-typed swatches example, generation-pipeline.md's per-turn pre-flight scope and what shipped for PhaseContext.inputs, testing.md's E2E dev-mode description (static prebuilt dist, not a live expo server, per the false pass this caused), and the reader-composer wireframe's chrome-row position; also fixes the same stale "Story Settings → Composer" path in app-settings.md and component-inventory.md. Adds ten triage.md entries for cross-cutting deferrals surfaced across Tasks 1, 3, 7, 8, and 9: raw trailing-block re-injection in PER_TURN_NARRATIVE, SuggestionCategoriesEditor's hardcoded i18n and the save-session's missing validity channel (both blocking 3.7b), runPreflight's storyModels omission, a Storybook-only EntryCard render throw, the status pill's single-slot/refresh-stranding limitation, the PipelineInputMap declaration-merging idiom, an intentional register.ts asymmetry, and two reversal/no-gate races that suggestion-refresh is first to expose. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XjvLADCdcER2xCQ5xmhbGb
Zero enabled categories no longer offers a dead Generate button — the mount gate now falls back to whether the terminal entry already carries chips, per reader-composer.md's zero-enabled-categories edge case, and rewords a branch-local commit SHA out of a test comment plus files two triage entries from the same whole-slice review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XjvLADCdcER2xCQ5xmhbGb
The previous commit's inline `{received, dropped}` payload span wrapped mid-brace and prettier dropped the continuation line's hanging indent; rewording to individual backtick spans per field keeps the wrap point clear of a brace pair.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XjvLADCdcER2xCQ5xmhbGb
Records the planning-time 3.7a/3.7b split and the three developer decisions it turned on (classifier fold emits a structured suggestions field rather than a sibling tagged block; the per-mode palette seed the Background wrongly assumed M1.5 had landed; category colour stored as a curated slot key or a raw hex), plus the deviations a later author has to know about — the strip's orthogonal collapsed prop, suggestion-refresh being the first no-gate pipeline kind and what that made reachable, the new PhaseContext.inputs seam, the DeltaSource choice, and why the null-metadata undo fix needed reverse-replay as well as register. Migrates the two resolved Open questions and marks the remaining three as 3.7b's. Dividing the brief itself into two docs stays developer-owned. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XjvLADCdcER2xCQ5xmhbGb
3.7a is renamed from the original doc so history follows, and its brief now matches what shipped: the Background's wrong claim that M1.5 seeded the palette is corrected into a scope item, the classifier fold is described as a structured field rather than a sibling block, the chip strip's states become phase-plus-collapsed, and E2E joins the test list. 3.7b is new and carries the settings surface, the three save-session questions 3.11 left open, and two blockers that were sitting in the triage inbox without an owner — the editor's hardcoded English chrome and the section contract's missing validity channel — which move into its Open questions so the plan-slice gate forces them before 3.7b is planned. Milestone slice list, dependency graph, C2 and C7 consumers, and the inbound references in 3.2 and 3.11 all repoint; 3.11's edge to 3.7b is now a full gate rather than partial, since that slice is nothing but a settings section. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XjvLADCdcER2xCQ5xmhbGb
buildStorySettings keeps 3.7a's (mode, app) object form and takes main's effectiveDim as a third param rather than folding it into the app object — it is resolved per story from the wizard's Matryoshka pick, not copied off an app-level column; the ~24 positional call sites main's new app-deps.test.ts added route through a local helper. GenerationStatusPill keeps main's i18n conversion but restores the exhaustive cancelCopy switch so a new phase fails the build instead of silently inheriting "Cancel generation", and the reader derives one activePhase for the pill's two branches. docs/testing.md keeps main's richer structure with 3.7a's two corrections: the mode names match the Playwright projects (dev / packaged), and __DEV__ is false in both — verified against the exported bundle prelude. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The two kinds were mutually exclusive in one direction only — a refresh could not start during a turn, but a turn could start during a refresh — which left the run's own output unreachable: chips persist on the entry that was terminal when ⟳ fired, so a turn landing mid-run writes them behind the new tail, and that turn emits its own chips anyway. Blocking per-turn instead would put friction on the primary action to protect provably-dead work, and `isUserEditBlocked` only covers hard-gate runs, so Send would have stayed live and turned every collision into a rejected turn — which the reader surfaces as a system failure entry. yieldsTo pre-empts the refresh instead, and the chrome ⟳ swaps to ✕ for the run's duration so the cancel sits where the action was fired rather than only in the top-bar pill. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The guard could never be false: the emission phase returns before rendering unless `suggestionsEnabled && enabledCategories > 0`, and buildGenerationContext derives the flag from that same palette — so it read as a real branch where a refresh prompt omits the very section it exists to ask for. It came from template-family symmetry with per-turn and the fallback classifier, both of which gate on a genuine run-level condition the refresh has no counterpart for. Rendered output is byte-identical; the structural-floor stub pack now registers the JSON macro, which the guard had been short-circuiting past. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The BuildArgs comment claimed per-turn passes the open partial chapter and chapter-close the closing one; all three live consumers actually pass the branch's loaded set and differ only in truncation, and chapter-close has no pipeline yet — so it described a per-kind query seam that has never existed and sent this review down a wrong path. The triage entry records the planned unification plus the four things not visible from the call sites: sceneEntities derives from the passed array's tail and breaks under template-side truncation, `entry` is absent from SUBSTITUTABLE_PREFIXES so raw entries would expose UUIDs to pack templates, only the refresh needs a new filter, and the builder's purity is what makes its 17-fixture test file cheap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A failed turn appends a system entry that becomes the branch tail, so the strip anchored to it: the previous reply's still-valid chips went invisible behind the failure card, and the empty-state ⟳ Generate offered there wrote chips into a row that Retry, Dismiss and the next Send all delete via clearSystemEntry — born-dead output, and canon sanctioned it by listing system entries among the empty-state targets. The reader now anchors past them and the emission phase refuses one outright rather than trusting its caller, mirroring the unconditional system exclusion in buildGenerationContext. targetEntryId stays a caller-supplied input: the refresh is no-gate, so recomputing the tail at write time would land chips generated from one entry's context onto whichever entry an undo left behind. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… chips Chips are delta-logged state generated from a context snapshot, and the post-call re-read of the target can only prove the row survived — not that an undone entry edit, an undone entity write, or a redo appending past it left the context intact. Redo is the sharpest: the chips land on a non-tail entry, stay invisible, and resurrect on a later rollback. The gate makes undo / redo / edit / rollback reject at the action layer for the run's duration, and the strip's ✕ is what keeps that from being a trap. Also moves the Actions-menu undo/redo gate from isGenerating to editBlocked — those reject on the gate, so keyed off the turn alone the item stayed enabled and its rejection toasted "nothing to undo" over an intact history. yieldsTo stays as a framework backstop now that the UI cannot start a turn mid-refresh. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
targetEntryId and the window it truncated were both artifacts of the no-gate era. With the gate held, the phase can resolve its own anchor — the branch's last narrative entry — so the read and the write sit inside the same gate, where a caller-supplied id was captured a tick earlier, outside it. The window goes with it: a system entry is a tail singleton, so nothing narrative follows the anchor and the loaded branch is already the right prompt window. suggestionsFire goes too, by building suggestionSlots unconditionally in the context builder — the slots are the story's palette, not an instruction to emit, and withholding them forced this phase to claim it was "firing" to receive its own subject matter. The builder now also scopes entries and entities by branchId, alongside the system exclusion it already did unconditionally. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… chrome Splits the composer's `disabled` into "no story to draft against" (hydration) and a new `sendBlocked` for the gate, which only Send honours: principles.md gates user-origin *mutations*, and neither the draft nor the composer mode is one — both are local until send, so gating them cost the user the one stretch where thinking ahead is all there is to do, and bought no coherence. The mode picker also drops its isGenerating gate for the same reason. The strip's chrome row leads the chip stack instead of trailing it, since the stack's height moves with the chip count and with the empty-state / error bodies, sliding ⟳ and ⌄ under the cursor between re-rolls; a spinner now rides over the outgoing chips rather than replacing them, because a re-roll that returns nothing usable keeps them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Derived rather than picked: a single-line chip row is 62px (2px borders + py-2 + a 20px overline + gap-1 + a 20px text-sm line), so 1.5 rows is 93px. Clamped to the stack because the overlay is positioned rather than clipped — at suggestionCount 1 the untrimmed size would spill over the chrome row above and the composer below, and chips that wrap only make the real stack taller than the estimate, so the clamp errs toward the smaller spinner. Also documents two harness facts found on the way: a running `pnpm desktop` holds 9222 and makes every spec fail in beforeAll at `0ms`, which reads like a mass product failure rather than a port collision. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two manual-test findings. The entry card dimmed to opacity-60 while `disabled`, but the streaming card is rendered without that prop — so narrative text changed shade the frame a turn committed and changed back when the run settled. Reading is never gated (principles.md → What's not gated) and every control in the card already takes `disabled` itself, so the container dim was cosmetic and wrong; dropping it also ends the whole-reader dim a chip re-roll caused. Second, the strip anchored to the last non-system entry, which becomes the just-committed user_action the instant a turn starts — blanking the chips to an empty-state ⟳ Generate that offers to generate while generation is running. The anchor is now the last AI-authored entry, the only kind that carries chips, shared by the route and the emission phase so they cannot disagree; that is also what canon's per-turn lock already described. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… prompts The strip now shows its spinner for a foreign lock too — a turn is about to replace what is on screen and nothing there is actionable, so a static stack read as interactive-but-broken. Deliberately spinner-only: no "Generating suggestions…" line, since the route cannot know this turn will emit chips at all, and no ✕, since there is no strip-owned run to cancel. An anchor with no chips shows it instead of ⟳ Generate, which had been offering to generate the very thing the running turn produces. Separately, both emission macros listed categories as "[cat1] Action: act", which reads as one label and invites the model to send the name as the ref — an unresolvable value whose chip is then dropped. The id is now its own quoted field per line, named after the exact key it populates, and the tagged-block skeleton interpolates a real id instead of the literal word "ref" a model would copy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collapsed, the chrome row is the whole strip — two bare icons floating above the composer with nothing naming them. The wireframe already carried a `Suggestions` title there; this brings the component in line, left-aligned against the controls. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reading RN's ActivityIndicator source, a numeric `size` leaves the native size prop undefined and styles only the wrapper box, which reads as though the Android drawable stays at its intrinsic size — so the entry claimed every Spinner was mis-sized there. On device the drawable scales to those bounds and the enlarged strip spinner renders as intended, so there was never a defect. The measurement replaces the inference: the prop doc now records that numeric sizing works on every platform and warns off re-deriving the opposite from the source. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
f78941f moved suggestion-refresh to hard-gate and triage.md was re-scoped then, but the slice doc still specified no-gate in three places (including "the first no-gate pipeline kind in the codebase") and parked.md still described a stranding scenario that yieldsTo now prevents; the new triage entry is the pre-flight story-override blindness found while verifying the refresh's error taxonomy, which affects every story-override target rather than suggestions alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A refresh that resolved no chip returned completed, leaving the strip pixel-identical to never having pressed ⟳ — so the likeliest model failure (echoing a category label instead of the opaque cat1) was invisible and infinitely repeatable; it now fails with a tagged phase-logic error, the strip renders discriminated copy beside the chips it failed to replace rather than destroying them, a config-resolver failure offers the settings route instead of a Retry that cannot succeed, and ⟳ goes dead when no category is enabled because the phase would no-op there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Model output is untrusted and nothing enforced the prompt's "one or two sentences", so a haywire emission grew the strip past the viewport and pushed the composer off screen with no scroll and no way back but the collapse chevron; chips over 400 characters are now dropped during resolution (dropped, not truncated — a tap inserts the text into the composer verbatim) and the stack scrolls past 38% of viewport height. The overlay spinner now sizes off the measured stack instead of a 62px-per-row estimate that any wrapped chip already invalidated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Items the parser skips never reach `items`, so droppedCount could not see them and one good chip beside two malformed ones left captured=true, dropped=0 and no warning at all — a model that reliably under-delivers produced a permanently thin strip with zero diagnostics; parseSuggestionsBlock now reports malformedCount (counted off opening tags, so a mid-stream truncation counts too) and all three folds warn when the resolved count falls short of the requested one. The two contracts this adds tests for were both verified by mutation: deleting the suggestionsCaptured write, and bounding only <state> in boundedEnd, each previously left the suite green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The {categoryRef, text} shape was declared four times, twice as Zod objects with byte-identical .describe() strings that ship to the model as the structured-output contract — so the file claiming the two surfaces "can't drift" was itself the drift risk; there is now one suggestionRefSchema with types via z.infer, and consumers differ only in array policy. resolveAccentColor returns a branded AccentHex so tintOf can no longer be handed a raw stored colour and silently emit rgba(NaN, NaN, NaN, α), the slot tuple becomes the source of truth instead of a mutable Object.keys cast, and STORY_SETTINGS_DEFAULTS' comment now names the real hazard rather than warning against the spread six test files correctly rely on.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…l site runPipeline took `kind: string` and `inputs?: unknown`, so starting suggestion-refresh with no inputs, or with a misspelled field, compiled fine and failed at runtime through a 'phase-logic' arm that existed only to catch a type error; PipelineInputMap now keys inputs by kind and runPipeline is generic over it, so both mistakes are compile errors. Kinds absent from the map stay unconstrained, which keeps the ad-hoc kinds test harnesses register working — the phase-side guard also stays, since the registry still stores every kind's phases under one PhaseContext and an untyped caller can still cast past the signature. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The refresh template's guard note was inverted by d05a3fb: it says a suggestionsFire guard "could never be false" when the phase stopped setting the flag, so one could never be TRUE — a comment whose stated purpose is preventing a change was inviting it, and bundled-pack.test.ts hand-passed suggestionsFire: true so no test would have caught the restored guard. Also fixes the mock-llm 404 that is really a 200 with `{}`, its emission gate missing the enabled-category half, and its now-failing zero-chip default; plus two prior-approach narrations the commenting rule bans and a "below" pointing 80 lines up. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…r race The hard gate was asserted only as a declaration plus isUserEditBlocked over a synthetic TxState, and branch-switch abort — named in the slice's test plan — had no coverage at any layer, so a regression in undo's own check or in the cleanup effect would have left everything green; both now drive a real run. The E2E spec waited on text that matches the streaming placeholder and then read the DB unretried, and its helper selected by highest position while the strip anchors via findSuggestionAnchor — the two coincide today and would diverge the first time a spec asserts after a submit or a failed turn. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`visible` vs `empty-state` was computed by the route from chips.length and then passed alongside the chips, so the two could disagree: `visible` with no chips rendered bare chrome, and `empty-state` with chips dropped them on the floor — both constructible from Storybook today. Deleting the redundant arms makes each unrepresentable without a discriminated union, which would only have encoded the duplication and forced every story to restate `chips`. Also fixes the chrome ⟳ vanishing entirely when a turn is in flight over a chipless anchor, where the body shows a spinner rather than the Generate button the old flag assumed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The deviations bullet still described inputs as `unknown` on PhaseContext with only a per-phase guard; PipelineInputMap now closes the caller side, and the phase-side guard remains for the reason the bullet did not give. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds next-turn suggestion generation, persistence, refresh, and reader controls. It introduces suggestion defaults, parsing and prompt contracts, typed refresh pipelines, Storybook and E2E coverage, story-settings wiring, and metadata undo handling. ChangesSuggestion generation and refresh
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
lib/pipeline/definitions/suggestion-refresh.test.ts (1)
508-508: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the stale
targetEntryIdkey.The input contract is
{ refreshGuidance }only;readRefreshInputignores the extra field, so this leftover just suggests a target is still passed.♻️ Proposed cleanup
- const { events } = await runEmission({ targetEntryId: 'entry-1', refreshGuidance: ' ' }) + const { events } = await runEmission({ refreshGuidance: ' ' })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/pipeline/definitions/suggestion-refresh.test.ts` at line 508, Remove the stale targetEntryId property from the runEmission invocation in the refreshGuidance whitespace test, passing only refreshGuidance to match the readRefreshInput contract.lib/piggyback/suggestion-slots.test.ts (1)
12-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSuggestion-category fixtures are hand-rolled in three test files with literal
colorvalues. All three buildSuggestionCategory-shaped objects without a cast, so each depends on'red' | 'blue' | 'gray'being members of the accent-color union inlib/themes/core/accent-palette.tsand on the fixture carrying every required field — one union rename breaks all three.
lib/piggyback/suggestion-slots.test.ts#L12-L19: confirm thecat()factory'scolor: 'blue'and field set satisfySuggestionCategory.lib/pipeline/definitions/per-turn-piggyback.test.ts#L38-L48: confirm'red'/'blue'/'gray'are valid accent values, then consider importing a shared fixture instead of redeclaringSUGGESTION_CATEGORIES.lib/pipeline/definitions/per-turn.test.ts#L718-L728: same check, and drop this duplicate in favour of the shared fixture.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/piggyback/suggestion-slots.test.ts` around lines 12 - 19, Ensure the hand-rolled SuggestionCategory fixtures remain type-safe against the accent-color union and required fields: in lib/piggyback/suggestion-slots.test.ts:12-19, validate the cat() factory’s color and complete field set; in lib/pipeline/definitions/per-turn-piggyback.test.ts:38-48, validate SUGGESTION_CATEGORIES values and reuse a shared fixture where available; in lib/pipeline/definitions/per-turn.test.ts:718-728, remove the duplicate fixture and use the same shared fixture.app/reader-composer/[branchId].tsx (1)
544-547: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueRefs mutated directly during render (React Doctor: no-ref-current-in-render) in two files.
Both files mirror a value into a ref by assigning
.currentdirectly in the component body rather than in an effect. This is a well-known "latest ref" pattern and is idempotent here (same value each render pass, no conditional logic), so the practical risk is low, but it technically violates React's render-purity contract and could leak stale values if a render is replayed/discarded without committing (e.g. under Suspense/transitions).
app/reader-composer/[branchId].tsx#L544-L547: movebranchIdRef.current = branchIdandterminalEntryIdRef.current = terminalEntry?.idinto auseEffect(() => { ... }, [branchId, terminalEntry?.id]).components/reader/composer.tsx#L95-L98: movetextRef.current = textandmodeRef.current = modeinto auseEffect(() => { ... }, [text, mode]).♻️ Proposed fix (composer.tsx)
- const textRef = useRef(text) - const modeRef = useRef(mode) - textRef.current = text - modeRef.current = mode + const textRef = useRef(text) + const modeRef = useRef(mode) + useEffect(() => { + textRef.current = text + modeRef.current = mode + }, [text, mode])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/reader-composer/`[branchId].tsx around lines 544 - 547, Move the direct ref mutations into effects: in app/reader-composer/[branchId].tsx lines 544-547, update branchIdRef.current and terminalEntryIdRef.current inside a useEffect dependent on branchId and terminalEntry?.id; in components/reader/composer.tsx lines 95-98, update textRef.current and modeRef.current inside a useEffect dependent on text and mode. Remove the corresponding render-time assignments while preserving the existing ref values.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/tests/suggestions.spec.ts`:
- Around line 168-170: Update the RegExp construction in the reader mode
assertion to escape the translated value from t('reader:composerMode.free')
before interpolation, preserving the intended literal text match regardless of
regex metacharacters in the locale string.
---
Nitpick comments:
In `@app/reader-composer/`[branchId].tsx:
- Around line 544-547: Move the direct ref mutations into effects: in
app/reader-composer/[branchId].tsx lines 544-547, update branchIdRef.current and
terminalEntryIdRef.current inside a useEffect dependent on branchId and
terminalEntry?.id; in components/reader/composer.tsx lines 95-98, update
textRef.current and modeRef.current inside a useEffect dependent on text and
mode. Remove the corresponding render-time assignments while preserving the
existing ref values.
In `@lib/piggyback/suggestion-slots.test.ts`:
- Around line 12-19: Ensure the hand-rolled SuggestionCategory fixtures remain
type-safe against the accent-color union and required fields: in
lib/piggyback/suggestion-slots.test.ts:12-19, validate the cat() factory’s color
and complete field set; in
lib/pipeline/definitions/per-turn-piggyback.test.ts:38-48, validate
SUGGESTION_CATEGORIES values and reuse a shared fixture where available; in
lib/pipeline/definitions/per-turn.test.ts:718-728, remove the duplicate fixture
and use the same shared fixture.
In `@lib/pipeline/definitions/suggestion-refresh.test.ts`:
- Line 508: Remove the stale targetEntryId property from the runEmission
invocation in the refreshGuidance whitespace test, passing only refreshGuidance
to match the readRefreshInput contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4ac904cb-6623-4599-9776-47e3c5c818fa
⛔ Files ignored due to path filters (20)
docs/data-model.mdis excluded by!docs/**docs/followups.mdis excluded by!docs/**docs/generation-pipeline.mdis excluded by!docs/**docs/implementation/milestones/03-memory-floor/milestone.mdis excluded by!docs/**docs/implementation/milestones/03-memory-floor/slices/02-piggyback.mdis excluded by!docs/**docs/implementation/milestones/03-memory-floor/slices/07-suggestions.mdis excluded by!docs/**docs/implementation/milestones/03-memory-floor/slices/07a-suggestions.mdis excluded by!docs/**docs/implementation/milestones/03-memory-floor/slices/07b-suggestion-settings.mdis excluded by!docs/**docs/implementation/milestones/03-memory-floor/slices/11-story-settings-shell.mdis excluded by!docs/**docs/implementation/triage.mdis excluded by!docs/**docs/parked.mdis excluded by!docs/**docs/testing.mdis excluded by!docs/**docs/ui/component-inventory.mdis excluded by!docs/**docs/ui/patterns/color-picker.mdis excluded by!docs/**docs/ui/patterns/generation-status-pill.mdis excluded by!docs/**docs/ui/principles.mdis excluded by!docs/**docs/ui/screens/app-settings/app-settings.mdis excluded by!docs/**docs/ui/screens/reader-composer/reader-composer.htmlis excluded by!docs/**docs/ui/screens/reader-composer/reader-composer.mdis excluded by!docs/**docs/ui/screens/story-settings/story-settings.mdis excluded by!docs/**
📒 Files selected for processing (77)
.claude/rules/testing.mdapp/reader-composer/[branchId].tsxapp/story-settings/[storyId].tsxapp/wizard.tsxcomponents/compounds/entry-card.stories.tsxcomponents/compounds/entry-card.tsxcomponents/compounds/generation-status-pill.stories.tsxcomponents/compounds/generation-status-pill.tsxcomponents/reader/composer.stories.tsxcomponents/reader/composer.tsxcomponents/reader/suggestion-failure.tscomponents/reader/suggestion-strip.stories.tsxcomponents/reader/suggestion-strip.tsxcomponents/reader/system-entry-actions.tsxcomponents/ui/spinner.tsxcomponents/wizard/finish.test.tscomponents/wizard/finish.tscomponents/wizard/wizard-flow.test.tse2e/harness/mock-llm.tse2e/locators/reader.tse2e/tests/classifier.spec.tse2e/tests/suggestions.spec.tslib/actions/delta/reverse-replay.test.tslib/actions/delta/reverse-replay.tslib/actions/embedder-swap/app-deps.test.tslib/actions/index.tslib/actions/stories/create-story.test.tslib/actions/stories/reset-settings.test.tslib/actions/stories/reset-settings.tslib/actions/stories/update-story-settings.test.tslib/actions/story-entries/register.tslib/actions/suggestions/refresh-suggestions.test.tslib/actions/suggestions/refresh-suggestions.tslib/db/app-settings/app-settings-defaults.tslib/db/devtools/seed-dataset.tslib/db/index.tslib/db/stories/default-suggestion-categories.test.tslib/db/stories/default-suggestion-categories.tslib/db/stories/story-settings-defaults.test.tslib/db/stories/story-settings-defaults.tslib/piggyback/index.tslib/piggyback/parse.test.tslib/piggyback/parse.tslib/piggyback/suggestion-slots.test.tslib/piggyback/suggestion-slots.tslib/piggyback/tags.tslib/piggyback/types.tslib/pipeline/__tests__/chained-and-parallel.test.tslib/pipeline/definitions/generation-context.test.tslib/pipeline/definitions/generation-context.tslib/pipeline/definitions/per-turn-piggyback.test.tslib/pipeline/definitions/per-turn-piggyback.tslib/pipeline/definitions/per-turn.test.tslib/pipeline/definitions/per-turn.tslib/pipeline/definitions/suggestion-refresh.test.tslib/pipeline/definitions/suggestion-refresh.tslib/pipeline/index.tslib/pipeline/runtime/orchestrator.tslib/pipeline/types.tslib/prompts/bundled/bundled-pack.test.tslib/prompts/bundled/index.tslib/prompts/bundled/per-turn.test.tslib/prompts/bundled/per-turn.tslib/prompts/bundled/piggyback-fallback-classifier.tslib/prompts/bundled/structural-floor.test.tslib/prompts/bundled/suggestion-emission-json.tslib/prompts/bundled/suggestion-emission.tslib/prompts/bundled/suggestion-refresh.tslib/prompts/ids.tslib/prompts/templateContextMap.tslib/stores/__tests__/namespace-shape.test.tslib/stores/generation/generation.tslib/themes/core/accent-palette.test.tslib/themes/core/accent-palette.tslib/themes/index.tslocales/en/common.jsonlocales/en/reader.json
Comment on lines
+168
to
+170
| await expect(reader.modeTrigger(app.window)).toHaveText( | ||
| new RegExp(t('reader:composerMode.free')), | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Escape the translated string before building a RegExp.
t('reader:composerMode.free') is interpolated into new RegExp(...) unescaped. If the locale string ever contains regex metacharacters, this either throws or produces an unintended match.
🛠️ Proposed fix
- await expect(reader.modeTrigger(app.window)).toHaveText(
- new RegExp(t('reader:composerMode.free')),
- )
+ await expect(reader.modeTrigger(app.window)).toContainText(t('reader:composerMode.free'))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await expect(reader.modeTrigger(app.window)).toHaveText( | |
| new RegExp(t('reader:composerMode.free')), | |
| ) | |
| await expect(reader.modeTrigger(app.window)).toContainText(t('reader:composerMode.free')) |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 168-168: Do not use variable for regular expressions
Context: new RegExp(t('reader:composerMode.free'))
Note: [CWE-1333] Inefficient Regular Expression Complexity. Security best practice.
(regexp-non-literal-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/tests/suggestions.spec.ts` around lines 168 - 170, Update the RegExp
construction in the reader mode assertion to escape the translated value from
t('reader:composerMode.free') before interpolation, preserving the intended
literal text match regardless of regex metacharacters in the locale string.
Source: Linters/SAST tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit