fix(rank): single shared H2H ceremony driver — no more forked placement loops - #70
Conversation
…arity) The /agent-rank ceremony and the main webapp movie flow ran the same RankingSession engine but through two hand-copied drivers: AddMediaModal (main movie flow) and RankingFlowModal (book flow + /agent-rank) each kept a byte-for-byte clone of the placement loop (sessionRef, isProcessingRef, currentComparison, sessionId, proceedFromNotes/handleCompareChoice/handleUndo). Two copies of the head-to-head binary-search loop is a fork waiting to drift — the /agent-rank "wrong comparisons, not the full loop" defect. Extract the driver into a single shared module (hooks/useRankingCeremony): a pure, React-free CeremonyDriver (createCeremonyDriver) owns the RankingSession lifecycle + comparison-log emission, and useRankingCeremony mirrors it into component state. Both modals now consume it, so the agent flow and the main flow drive ONE engine through ONE driver. Behavior of the main flow is preserved (699 baseline green, unchanged); the write path is untouched (still the standard contract path under the agent-rank token client). Adds driver unit tests (comparison selection, the loop, re-entrancy guard, undo, logging) and a parity test proving a seeded tier + fixed choice sequence places identically for the main-flow and agent-flow drivers, plus structural guards that neither modal news its own RankingSession. Tests: 699 → 716 (17 new). vite build green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthroughA shared ChangesRanking Ceremony Flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RankingModal
participant useRankingCeremony
participant CeremonyDriver
participant RankingSession
RankingModal->>useRankingCeremony: begin(selectedItem, selectedTier)
useRankingCeremony->>CeremonyDriver: begin(item, tier)
CeremonyDriver->>RankingSession: start or place
RankingSession-->>CeremonyDriver: comparison or rank
CeremonyDriver-->>useRankingCeremony: CeremonyStep
useRankingCeremony-->>RankingModal: compare or placed
RankingModal->>useRankingCeremony: choose(choice)
useRankingCeremony->>CeremonyDriver: choose(choice)
CeremonyDriver->>RankingSession: advance comparison
RankingSession-->>RankingModal: next comparison or rank
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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.
🧹 Nitpick comments (2)
services/__tests__/agentRankCeremonyParity.test.ts (1)
74-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueParity assertion is largely tautological.
Both
mainFlowandagentFlowrun the samecreateCeremonyDriverdirectly and differ only by thewithLoggingflag; since logging is a read-only side channel, identicaltrace/rankis almost guaranteed by construction. This validates that logging doesn't perturb placement (useful), but it does not exercise the actualAddMediaModal/RankingFlowModalwiring — the "no fork" guarantee actually rests on the structural regex tests below. Consider clarifying the intent, or drive placement through the real modal hooks if you want true wiring parity.🤖 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 `@services/__tests__/agentRankCeremonyParity.test.ts` around lines 74 - 98, Clarify the ceremony parity test around runCeremony to state that it verifies logging does not alter placement, rather than claiming to validate modal wiring. Keep the trace and rank comparisons, and adjust the test description or comments to distinguish this behavioral check from the separate structural regex coverage.hooks/useRankingCeremony.ts (1)
162-178: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winKeep
onComparecurrent inuseRankingCeremony
ensure()/reset()freeze the firstoptionsobject, but callers passonComparestraight through from a render-scoped handler. If the modal rerenders before a compare completes, the driver can keep calling an outdated callback. Threadoptionsthrough a ref or rebuild the driver whenonComparechanges.🤖 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 `@hooks/useRankingCeremony.ts` around lines 162 - 178, The ensure and reset callbacks in useRankingCeremony capture the initial options object, leaving the ceremony driver with a stale onCompare handler after rerenders. Keep the current onCompare available to the driver by storing the latest options or onCompare in a ref and using it when creating or invoking the driver, or recreate the driver when onCompare changes while preserving existing session/reset behavior.
🤖 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.
Nitpick comments:
In `@hooks/useRankingCeremony.ts`:
- Around line 162-178: The ensure and reset callbacks in useRankingCeremony
capture the initial options object, leaving the ceremony driver with a stale
onCompare handler after rerenders. Keep the current onCompare available to the
driver by storing the latest options or onCompare in a ref and using it when
creating or invoking the driver, or recreate the driver when onCompare changes
while preserving existing session/reset behavior.
In `@services/__tests__/agentRankCeremonyParity.test.ts`:
- Around line 74-98: Clarify the ceremony parity test around runCeremony to
state that it verifies logging does not alter placement, rather than claiming to
validate modal wiring. Keep the trace and rank comparisons, and adjust the test
description or comments to distinguish this behavioral check from the separate
structural regex coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa8d011a-94c7-4075-9859-ee5ad6e05113
📒 Files selected for processing (5)
components/media/AddMediaModal.tsxcomponents/media/RankingFlowModal.tsxhooks/__tests__/useRankingCeremony.test.tshooks/useRankingCeremony.tsservices/__tests__/agentRankCeremonyParity.test.ts
1 participant
The /agent-rank card and the main web flow each carried a byte-for-byte private copy of the head-to-head placement driver (sessionRef/comparison/undo/proceed logic) around the shared RankingSession engine. Identical today, guaranteed to drift tomorrow. Extracted one shared driver (hooks/useRankingCeremony.ts: pure core + thin React binding); AddMediaModal and RankingFlowModal both consume it.
Tests 699 → 716 green; vite build green.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests