feat(gameplay): AI gameplay review and highlight workflows for streamers (#1674)#2856
Conversation
…ed-tool-admission # Conflicts: # app/test/e2e/specs/mega-flow.spec.ts
- Resolve merge conflict in Intelligence.tsx: retain `tasks` tab from main and add `gameplay` tab (i18n key `memory.tab.gameplay`) from this PR - Resolve merge conflict in src/core/all.rs: register gameplay_review controllers in build_registered_controllers and build_declared_controller_schemas - Fix src/openhuman/gameplay_review/schemas.rs compilation: change to_json to take RpcOutcome<T> and return Result<Value, String> via into_cli_compatible_json(), matching the established controller pattern - Add path-traversal guard (validate_session_id) to store.rs - Fix button label in GameplayReviewWorkspace.tsx: show 'Analyzing...' during analysis phase distinct from 'Importing...' during import phase - Add memory.tab.gameplay i18n key to en.ts and all 13 locale chunk files (ar, bn, de, es, fr, hi, id, it, ko, pl, pt, ru, zh-CN) - Reorder gameplay_review in src/openhuman/mod.rs to alphabetical position (between encryption and health) per cargo fmt - Apply cargo fmt line-length formatting across ops.rs, schemas.rs, store.rs Addresses CodeRabbit feedback on formatting, compilation, and diagnostics.
|
Caution Review failedThe pull request is closed. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (19)
📝 WalkthroughWalkthroughThis PR introduces a complete gameplay review feature spanning backend Rust implementation (session/preset storage, analysis orchestration, RPC handlers, schema wiring), a TypeScript service client (frame preparation, RPC wrappers, helpers), a React workspace component with import form and analysis viewer, comprehensive tests for both layers, and i18n translations across 15 languages. ChangesGameplay Review Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
app/src/services/gameplayReviewService.ts (1)
177-179: ⚡ Quick winAlign preset list return type with backend contract.
listGameplayPresets()is typed asGameplayPresetInput[], but the backend returns persisted presets (GameplayReviewPreset) includingupdated_at_ms. This narrows the contract and drops useful typed fields.Suggested diff
export interface GameplayPresetInput { game_id: string; display_name: string; coaching_focus: string[]; audio_feedback: boolean; spoiler_mode: SpoilerMode; notes?: string | null; } + +export interface GameplayReviewPreset extends GameplayPresetInput { + updated_at_ms: number; +} @@ -export async function listGameplayPresets(): Promise<GameplayPresetInput[]> { - return callCoreRpc<GameplayPresetInput[]>({ method: 'openhuman.gameplay_review_list_presets' }); +export async function listGameplayPresets(): Promise<GameplayReviewPreset[]> { + return callCoreRpc<GameplayReviewPreset[]>({ + method: 'openhuman.gameplay_review_list_presets', + }); }🤖 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/src/services/gameplayReviewService.ts` around lines 177 - 179, The function listGameplayPresets is incorrectly typed as returning GameplayPresetInput[] even though the backend RPC openhuman.gameplay_review_list_presets returns persisted presets (GameplayReviewPreset) that include fields like updated_at_ms; update the function signature and any related generics to return Promise<GameplayReviewPreset[]> (and adjust any callers/types if needed) so the TypeScript contract matches the backend response.app/src/components/intelligence/GameplayReviewWorkspace.test.tsx (1)
175-176: ⚡ Quick winAssert highlight text without styling selectors
Line 175 ties the assertion to Tailwind classes, which is an implementation detail and will make this test fragile during harmless style changes. Prefer asserting by visible text/role within the relevant section instead.
As per coding guidelines: “Prefer testing behavior over implementation details.”
🤖 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/src/components/intelligence/GameplayReviewWorkspace.test.tsx` around lines 175 - 176, The test currently asserts the highlight by tying to Tailwind classes via screen.getByText('Highlight: clutch fight', { selector: 'div.font-medium.text-stone-900' }), which is fragile; change the assertion to check visible text/role inside the relevant container instead (e.g., use screen.getByText('Highlight: clutch fight') or queryWithin a parent container/test-id for the highlights section and assert with toBeInTheDocument), removing the selector argument so the test verifies behavior/content rather than styling.
🤖 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 `@app/src/components/intelligence/GameplayReviewWorkspace.tsx`:
- Around line 64-590: GameplayReviewWorkspace contains hard-coded user-visible
strings (labels, placeholders, button text, error messages via setError, and
toast titles/messages passed to onToast) that must be routed through the i18n
hook; replace each literal in GameplayReviewWorkspace (search for use in the
GameplayReviewWorkspace function: input placeholders, <option> text, button
labels, the setError(...) invocations, and onToast calls) with calls to useT()
keys and add corresponding entries to app/src/lib/i18n/en.ts in this PR so all
UI strings (including recap/follow-up text, highlights/clip headings, and status
messages like "Importing…", "Analyzing…", success/error toast titles/messages)
are localized.
In `@app/src/services/gameplayReviewService.ts`:
- Around line 213-227: formatSpoilerMode and normalizeGameplayError emit
hard-coded English strings; change them to return translation keys instead (e.g.
formatSpoilerMode returns keys like "spoiler.mode.off" / "spoiler.mode.full" /
"spoiler.mode.light" and normalizeGameplayError returns the incoming string if
string, error.message if Error, otherwise a key like
"errors.gameplayReviewFailed"). Update the functions formatSpoilerMode and
normalizeGameplayError to only produce these keys (not localized text) and
document that callers (UI components) must call useT() to resolve those keys
into user-visible copy.
In `@src/openhuman/gameplay_review/schemas.rs`:
- Around line 73-167: Several controller schemas currently wrap the payload
under keys like "session", "analysis", "preset", "question", and "clip" but the
handlers deserialize the request body directly into Gameplay*Input structs; fix
each controller (register_session, analyze_session, set_preset, ask_session,
draft_clip_metadata) by removing the wrapper key and making the inputs a single
FieldSchema that refers directly to the corresponding TypeSchema::Ref
(GameplayReviewSessionInput, GameplayReviewAnalysisInput, GameplayPresetInput,
GameplayReviewQuestionInput, GameplayReviewClipInput) so the schema matches the
handler deserialization; apply the same change pattern for any other controllers
listed in the file that use the wrapped pattern.
---
Nitpick comments:
In `@app/src/components/intelligence/GameplayReviewWorkspace.test.tsx`:
- Around line 175-176: The test currently asserts the highlight by tying to
Tailwind classes via screen.getByText('Highlight: clutch fight', { selector:
'div.font-medium.text-stone-900' }), which is fragile; change the assertion to
check visible text/role inside the relevant container instead (e.g., use
screen.getByText('Highlight: clutch fight') or queryWithin a parent
container/test-id for the highlights section and assert with toBeInTheDocument),
removing the selector argument so the test verifies behavior/content rather than
styling.
In `@app/src/services/gameplayReviewService.ts`:
- Around line 177-179: The function listGameplayPresets is incorrectly typed as
returning GameplayPresetInput[] even though the backend RPC
openhuman.gameplay_review_list_presets returns persisted presets
(GameplayReviewPreset) that include fields like updated_at_ms; update the
function signature and any related generics to return
Promise<GameplayReviewPreset[]> (and adjust any callers/types if needed) so the
TypeScript contract matches the backend response.
🪄 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
Run ID: 37c0a5f5-914a-48df-80ef-7aa95377ad97
📒 Files selected for processing (27)
app/src/components/intelligence/GameplayReviewWorkspace.test.tsxapp/src/components/intelligence/GameplayReviewWorkspace.tsxapp/src/lib/i18n/chunks/ar-1.tsapp/src/lib/i18n/chunks/bn-1.tsapp/src/lib/i18n/chunks/de-1.tsapp/src/lib/i18n/chunks/en-1.tsapp/src/lib/i18n/chunks/es-1.tsapp/src/lib/i18n/chunks/fr-1.tsapp/src/lib/i18n/chunks/hi-1.tsapp/src/lib/i18n/chunks/id-1.tsapp/src/lib/i18n/chunks/it-1.tsapp/src/lib/i18n/chunks/ko-1.tsapp/src/lib/i18n/chunks/pl-1.tsapp/src/lib/i18n/chunks/pt-1.tsapp/src/lib/i18n/chunks/ru-1.tsapp/src/lib/i18n/chunks/zh-CN-1.tsapp/src/lib/i18n/en.tsapp/src/pages/Intelligence.tsxapp/src/services/gameplayReviewService.tssrc/core/all.rssrc/openhuman/about_app/catalog.rssrc/openhuman/gameplay_review/mod.rssrc/openhuman/gameplay_review/ops.rssrc/openhuman/gameplay_review/schemas.rssrc/openhuman/gameplay_review/store.rssrc/openhuman/gameplay_review/types.rssrc/openhuman/mod.rs
| export function GameplayReviewWorkspace({ onToast }: GameplayReviewWorkspaceProps) { | ||
| const [form, setForm] = useState(INITIAL_IMPORT_STATE); | ||
| const [selectedFiles, setSelectedFiles] = useState<File[]>([]); | ||
| const [recentSessions, setRecentSessions] = useState<GameplayReviewSession[]>([]); | ||
| const [activeSession, setActiveSession] = useState<GameplayReviewSession | null>(null); | ||
| const [question, setQuestion] = useState('What were my best moments?'); | ||
| const [questionAnswer, setQuestionAnswer] = useState<string>(''); | ||
| const [questionBusy, setQuestionBusy] = useState(false); | ||
| const [importBusy, setImportBusy] = useState(false); | ||
| const [analysisBusy, setAnalysisBusy] = useState(false); | ||
| const [draftBusy, setDraftBusy] = useState(false); | ||
| const [error, setError] = useState<string | null>(null); | ||
| const fileInputRef = useRef<HTMLInputElement | null>(null); | ||
|
|
||
| const refreshSessions = useCallback(async () => { | ||
| try { | ||
| const sessions = await listGameplaySessions(); | ||
| setRecentSessions(sessions); | ||
| setActiveSession(currentSession => currentSession ?? sessions[0] ?? null); | ||
| } catch (err) { | ||
| setError(normalizeGameplayError(err)); | ||
| } | ||
| }, []); | ||
|
|
||
| useEffect(() => { | ||
| void refreshSessions(); | ||
| }, [refreshSessions]); | ||
|
|
||
| const activeHighlights = useMemo(() => flattenHighlights(activeSession), [activeSession]); | ||
| const activeDrafts = useMemo(() => flattenDrafts(activeSession), [activeSession]); | ||
| const activeClips = useMemo(() => flattenClipCandidates(activeSession), [activeSession]); | ||
|
|
||
| const handleSelectFiles = useCallback(() => { | ||
| fileInputRef.current?.click(); | ||
| }, []); | ||
|
|
||
| const handleFilesChanged = useCallback((event: React.ChangeEvent<HTMLInputElement>) => { | ||
| const files = Array.from(event.target.files ?? []); | ||
| setSelectedFiles(files); | ||
| }, []); | ||
|
|
||
| const handleImportSession = useCallback(async () => { | ||
| setError(null); | ||
| if (!form.gameId.trim()) { | ||
| setError('Game name is required.'); | ||
| return; | ||
| } | ||
| if (!form.sessionTitle.trim()) { | ||
| setError('Session title is required.'); | ||
| return; | ||
| } | ||
| if (selectedFiles.length === 0) { | ||
| setError('Choose a folder or a set of keyframe images first.'); | ||
| return; | ||
| } | ||
|
|
||
| setImportBusy(true); | ||
| try { | ||
| const frames = await prepareGameplayFrames(selectedFiles); | ||
| if (frames.length === 0) { | ||
| throw new Error('No image frames were found in that folder.'); | ||
| } | ||
|
|
||
| if (form.presetName.trim()) { | ||
| await saveGameplayPreset({ | ||
| game_id: form.gameId.trim(), | ||
| display_name: form.presetName.trim(), | ||
| coaching_focus: form.coachingFocus | ||
| .split(',') | ||
| .map(item => item.trim()) | ||
| .filter(Boolean), | ||
| audio_feedback: form.audioFeedback, | ||
| spoiler_mode: form.spoilerMode, | ||
| notes: form.notes.trim() || null, | ||
| }); | ||
| } | ||
|
|
||
| const session = await registerGameplaySession({ | ||
| game_id: form.gameId.trim(), | ||
| session_title: form.sessionTitle.trim(), | ||
| source_label: form.sourceLabel.trim() || null, | ||
| spoiler_mode: form.spoilerMode, | ||
| preset_id: form.presetName.trim() || null, | ||
| frames: frames.map( | ||
| frame => | ||
| ({ | ||
| file_name: frame.file_name, | ||
| image_ref: frame.image_ref, | ||
| captured_at_ms: frame.captured_at_ms, | ||
| }) satisfies GameplayFrameInput | ||
| ), | ||
| }); | ||
|
|
||
| setAnalysisBusy(true); | ||
| const analyzed = await analyzeGameplaySession({ | ||
| session_id: session.session_id, | ||
| max_highlights: 5, | ||
| platforms: splitPlatforms(form.platforms), | ||
| }); | ||
| setActiveSession(analyzed); | ||
| setQuestionAnswer(''); | ||
| onToast?.({ | ||
| type: 'success', | ||
| title: 'Gameplay session analyzed', | ||
| message: `Reviewed ${analyzed.frames.length} frame(s) for ${analyzed.game_id}.`, | ||
| }); | ||
| void refreshSessions(); | ||
| } catch (err) { | ||
| const message = normalizeGameplayError(err); | ||
| setError(message); | ||
| onToast?.({ type: 'error', title: 'Gameplay review failed', message }); | ||
| } finally { | ||
| setAnalysisBusy(false); | ||
| setImportBusy(false); | ||
| } | ||
| }, [form, onToast, refreshSessions, selectedFiles]); | ||
|
|
||
| const handleQuestion = useCallback(async () => { | ||
| if (!activeSession) { | ||
| setError('Select or analyze a session first.'); | ||
| return; | ||
| } | ||
| setQuestionBusy(true); | ||
| try { | ||
| const response = await askGameplaySession({ session_id: activeSession.session_id, question }); | ||
| setQuestionAnswer(response.answer); | ||
| } catch (err) { | ||
| setError(normalizeGameplayError(err)); | ||
| } finally { | ||
| setQuestionBusy(false); | ||
| } | ||
| }, [activeSession, question]); | ||
|
|
||
| const handleDraftClips = useCallback(async () => { | ||
| if (!activeSession) return; | ||
| setDraftBusy(true); | ||
| try { | ||
| const drafts = await draftGameplayClipMetadata({ | ||
| session_id: activeSession.session_id, | ||
| platform: splitPlatforms(form.platforms)[0] || 'twitch', | ||
| highlight_id: activeHighlights[0]?.id ?? null, | ||
| }); | ||
| setActiveSession(prev => | ||
| prev | ||
| ? { | ||
| ...prev, | ||
| analysis: prev.analysis | ||
| ? { ...prev.analysis, draft_metadata: drafts } | ||
| : prev.analysis, | ||
| } | ||
| : prev | ||
| ); | ||
| } catch (err) { | ||
| setError(normalizeGameplayError(err)); | ||
| } finally { | ||
| setDraftBusy(false); | ||
| } | ||
| }, [activeHighlights, activeSession, form.platforms]); | ||
|
|
||
| return ( | ||
| <div className="space-y-4" data-testid="gameplay-review-workspace"> | ||
| <div className="rounded-2xl border border-sage-200 bg-gradient-to-br from-white via-sage-50 to-amber-50 p-5 shadow-soft"> | ||
| <div className="flex flex-wrap items-start justify-between gap-4"> | ||
| <div className="space-y-2"> | ||
| <p className="text-xs font-semibold uppercase tracking-[0.24em] text-sage-700"> | ||
| Gameplay review | ||
| </p> | ||
| <h2 className="text-2xl font-bold text-stone-900"> | ||
| Import a session, find the clips, draft the post | ||
| </h2> | ||
| <p className="max-w-2xl text-sm text-stone-600"> | ||
| Load a folder of keyframes from a long session, generate a concise recap, ask | ||
| follow-up questions, and turn the strongest moments into platform-ready clip metadata. | ||
| </p> | ||
| </div> | ||
| <div className="rounded-xl border border-sage-200 bg-white/80 px-4 py-3 text-sm text-stone-700 shadow-sm"> | ||
| <div className="font-semibold text-stone-900">Selected files</div> | ||
| <div>{selectedFiles.length} file(s)</div> | ||
| <div>{formatSpoilerMode(form.spoilerMode)}</div> | ||
| </div> | ||
| </div> | ||
|
|
||
| <div className="mt-4 grid gap-3 md:grid-cols-2 xl:grid-cols-3"> | ||
| <label className="space-y-1 text-sm"> | ||
| <span className="font-medium text-stone-700">Game</span> | ||
| <input | ||
| value={form.gameId} | ||
| onChange={event => setForm(prev => ({ ...prev, gameId: event.target.value }))} | ||
| className="w-full rounded-xl border border-stone-200 bg-white px-3 py-2 outline-none ring-0 transition focus:border-primary-500" | ||
| placeholder="Apex Legends" | ||
| /> | ||
| </label> | ||
| <label className="space-y-1 text-sm"> | ||
| <span className="font-medium text-stone-700">Session title</span> | ||
| <input | ||
| value={form.sessionTitle} | ||
| onChange={event => setForm(prev => ({ ...prev, sessionTitle: event.target.value }))} | ||
| className="w-full rounded-xl border border-stone-200 bg-white px-3 py-2 outline-none ring-0 transition focus:border-primary-500" | ||
| placeholder="Ranked climb on Friday night" | ||
| /> | ||
| </label> | ||
| <label className="space-y-1 text-sm"> | ||
| <span className="font-medium text-stone-700">Source label</span> | ||
| <input | ||
| value={form.sourceLabel} | ||
| onChange={event => setForm(prev => ({ ...prev, sourceLabel: event.target.value }))} | ||
| className="w-full rounded-xl border border-stone-200 bg-white px-3 py-2 outline-none ring-0 transition focus:border-primary-500" | ||
| placeholder="/recordings/apex/night-01" | ||
| /> | ||
| </label> | ||
| <label className="space-y-1 text-sm"> | ||
| <span className="font-medium text-stone-700">Spoiler mode</span> | ||
| <select | ||
| value={form.spoilerMode} | ||
| onChange={event => | ||
| setForm(prev => ({ ...prev, spoilerMode: event.target.value as SpoilerMode })) | ||
| } | ||
| className="w-full rounded-xl border border-stone-200 bg-white px-3 py-2 outline-none ring-0 transition focus:border-primary-500"> | ||
| <option value="off">Off</option> | ||
| <option value="light">Light</option> | ||
| <option value="full">Full</option> | ||
| </select> | ||
| </label> | ||
| <label className="space-y-1 text-sm"> | ||
| <span className="font-medium text-stone-700">Preset name</span> | ||
| <input | ||
| value={form.presetName} | ||
| onChange={event => setForm(prev => ({ ...prev, presetName: event.target.value }))} | ||
| className="w-full rounded-xl border border-stone-200 bg-white px-3 py-2 outline-none ring-0 transition focus:border-primary-500" | ||
| placeholder="Apex coaching preset" | ||
| /> | ||
| </label> | ||
| <label className="space-y-1 text-sm"> | ||
| <span className="font-medium text-stone-700">Focus areas</span> | ||
| <input | ||
| value={form.coachingFocus} | ||
| onChange={event => setForm(prev => ({ ...prev, coachingFocus: event.target.value }))} | ||
| className="w-full rounded-xl border border-stone-200 bg-white px-3 py-2 outline-none ring-0 transition focus:border-primary-500" | ||
| placeholder="aim, positioning, fight selection" | ||
| /> | ||
| </label> | ||
| <label className="space-y-1 text-sm md:col-span-2 xl:col-span-3"> | ||
| <span className="font-medium text-stone-700">Preset notes</span> | ||
| <textarea | ||
| value={form.notes} | ||
| onChange={event => setForm(prev => ({ ...prev, notes: event.target.value }))} | ||
| className="min-h-24 w-full rounded-xl border border-stone-200 bg-white px-3 py-2 outline-none ring-0 transition focus:border-primary-500" | ||
| placeholder="What should the reviewer pay attention to?" | ||
| /> | ||
| </label> | ||
| <label className="space-y-1 text-sm md:col-span-2"> | ||
| <span className="font-medium text-stone-700">Clip platforms</span> | ||
| <input | ||
| value={form.platforms} | ||
| onChange={event => setForm(prev => ({ ...prev, platforms: event.target.value }))} | ||
| className="w-full rounded-xl border border-stone-200 bg-white px-3 py-2 outline-none ring-0 transition focus:border-primary-500" | ||
| placeholder="twitch,kick,youtube" | ||
| /> | ||
| </label> | ||
| <label className="flex items-center gap-2 self-end text-sm text-stone-700 md:col-span-1"> | ||
| <input | ||
| type="checkbox" | ||
| checked={form.audioFeedback} | ||
| onChange={event => | ||
| setForm(prev => ({ ...prev, audioFeedback: event.target.checked })) | ||
| } | ||
| /> | ||
| Enable audio summary notes | ||
| </label> | ||
| </div> | ||
|
|
||
| <div className="mt-4 flex flex-wrap items-center gap-3"> | ||
| <input | ||
| ref={fileInputRef} | ||
| type="file" | ||
| multiple | ||
| accept="image/*" | ||
| {...DIRECTORY_INPUT_PROPS} | ||
| className="hidden" | ||
| onChange={handleFilesChanged} | ||
| /> | ||
| <button | ||
| type="button" | ||
| onClick={handleSelectFiles} | ||
| className="rounded-xl border border-sage-200 bg-white px-4 py-2 text-sm font-semibold text-sage-800 shadow-sm transition hover:bg-sage-50"> | ||
| Choose folder / frames | ||
| </button> | ||
| <button | ||
| type="button" | ||
| onClick={handleImportSession} | ||
| disabled={importBusy || analysisBusy} | ||
| className="rounded-xl bg-primary-600 px-4 py-2 text-sm font-semibold text-white shadow-sm transition hover:bg-primary-700 disabled:cursor-not-allowed disabled:opacity-60"> | ||
| {analysisBusy ? 'Analyzing…' : importBusy ? 'Importing…' : 'Import and analyze'} | ||
| </button> | ||
| <button | ||
| type="button" | ||
| onClick={handleDraftClips} | ||
| disabled={draftBusy || !activeSession} | ||
| className="rounded-xl border border-amber-200 bg-white px-4 py-2 text-sm font-semibold text-amber-800 shadow-sm transition hover:bg-amber-50 disabled:cursor-not-allowed disabled:opacity-60"> | ||
| {draftBusy ? 'Drafting…' : 'Refresh clip metadata'} | ||
| </button> | ||
| <div className="text-xs text-stone-500"> | ||
| The core will analyze image frames. For raw video files, point this at extracted | ||
| keyframes. | ||
| </div> | ||
| </div> | ||
| </div> | ||
|
|
||
| {error && ( | ||
| <div className="rounded-xl border border-coral-200 bg-coral-50 px-4 py-3 text-sm text-coral-800"> | ||
| {error} | ||
| </div> | ||
| )} | ||
|
|
||
| <div className="grid gap-4 lg:grid-cols-[1.2fr_0.8fr]"> | ||
| <section className="space-y-4 rounded-2xl border border-stone-200 bg-white p-5 shadow-soft"> | ||
| <div className="flex items-center justify-between gap-2"> | ||
| <div> | ||
| <h3 className="text-lg font-semibold text-stone-900">Session viewer</h3> | ||
| <p className="text-sm text-stone-500"> | ||
| Recap, highlights, and clip candidates for the current session. | ||
| </p> | ||
| </div> | ||
| <div className="rounded-full border border-stone-200 bg-stone-50 px-3 py-1 text-xs text-stone-600"> | ||
| {activeSession ? activeSession.game_id : 'No session selected'} | ||
| </div> | ||
| </div> | ||
|
|
||
| {activeSession ? ( | ||
| <div className="space-y-4"> | ||
| <div className="rounded-xl border border-sage-200 bg-sage-50 p-4"> | ||
| <div className="flex flex-wrap items-center justify-between gap-2"> | ||
| <div> | ||
| <div className="text-sm font-semibold text-sage-900"> | ||
| {activeSession.session_title} | ||
| </div> | ||
| <div className="text-xs text-sage-700"> | ||
| Imported {formatTimestamp(activeSession.imported_at_ms)} ·{' '} | ||
| {activeSession.frames.length} frame(s) | ||
| </div> | ||
| </div> | ||
| <div className="text-xs font-medium text-sage-700"> | ||
| {formatSpoilerMode(activeSession.spoiler_mode)} | ||
| </div> | ||
| </div> | ||
| {activeSession.analysis?.spoiler_note && ( | ||
| <p className="mt-3 text-sm text-stone-700"> | ||
| {activeSession.analysis.spoiler_note} | ||
| </p> | ||
| )} | ||
| {activeSession.analysis && ( | ||
| <p className="mt-3 whitespace-pre-line text-sm text-stone-800"> | ||
| {activeSession.analysis.recap} | ||
| </p> | ||
| )} | ||
| </div> | ||
|
|
||
| <div className="grid gap-3 md:grid-cols-2"> | ||
| <div className="rounded-xl border border-stone-200 p-4"> | ||
| <div className="text-sm font-semibold text-stone-900">Highlights</div> | ||
| <div className="mt-3 space-y-3"> | ||
| {activeHighlights.length > 0 ? ( | ||
| activeHighlights.map(highlight => ( | ||
| <article | ||
| key={highlight.id} | ||
| className="rounded-lg border border-stone-200 bg-stone-50 p-3"> | ||
| <div className="flex items-center justify-between gap-2"> | ||
| <div className="font-medium text-stone-900">{highlight.title}</div> | ||
| <span className="rounded-full bg-white px-2 py-0.5 text-[11px] text-stone-500"> | ||
| {(highlight.confidence * 100).toFixed(0)}% | ||
| </span> | ||
| </div> | ||
| <p className="mt-1 text-xs uppercase tracking-wide text-stone-500"> | ||
| {highlight.kind} · frame {highlight.frame_index + 1} ·{' '} | ||
| {formatTimestamp(highlight.captured_at_ms)} | ||
| </p> | ||
| <p className="mt-2 text-sm text-stone-700">{highlight.rationale}</p> | ||
| </article> | ||
| )) | ||
| ) : ( | ||
| <p className="text-sm text-stone-500">No highlights detected yet.</p> | ||
| )} | ||
| </div> | ||
| </div> | ||
|
|
||
| <div className="rounded-xl border border-stone-200 p-4"> | ||
| <div className="text-sm font-semibold text-stone-900">Clip candidates</div> | ||
| <div className="mt-3 space-y-3"> | ||
| {activeClips.length > 0 ? ( | ||
| activeClips.map(clip => ( | ||
| <article | ||
| key={clip.id} | ||
| className="rounded-lg border border-amber-100 bg-amber-50 p-3"> | ||
| <div className="font-medium text-amber-950">{clip.start_label}</div> | ||
| <div className="text-xs text-amber-800">through {clip.end_label}</div> | ||
| <p className="mt-2 text-sm text-stone-700">{clip.rationale}</p> | ||
| </article> | ||
| )) | ||
| ) : ( | ||
| <p className="text-sm text-stone-500">No clip candidates yet.</p> | ||
| )} | ||
| </div> | ||
| </div> | ||
| </div> | ||
|
|
||
| <div className="rounded-xl border border-stone-200 p-4"> | ||
| <div className="text-sm font-semibold text-stone-900">Platform drafts</div> | ||
| <div className="mt-3 grid gap-3 md:grid-cols-3"> | ||
| {activeDrafts.length > 0 ? ( | ||
| activeDrafts.map(draft => ( | ||
| <article | ||
| key={`${draft.platform}-${draft.title}`} | ||
| className="rounded-lg border border-stone-200 bg-white p-3"> | ||
| <div className="text-xs font-semibold uppercase tracking-wide text-stone-500"> | ||
| {draft.platform} | ||
| </div> | ||
| <div className="mt-1 font-medium text-stone-900">{draft.title}</div> | ||
| <p className="mt-2 text-sm text-stone-700 whitespace-pre-line"> | ||
| {draft.description} | ||
| </p> | ||
| <div className="mt-3 flex flex-wrap gap-2"> | ||
| {draft.tags.map(tag => ( | ||
| <span | ||
| key={tag} | ||
| className="rounded-full bg-stone-100 px-2 py-0.5 text-[11px] text-stone-600"> | ||
| #{tag} | ||
| </span> | ||
| ))} | ||
| </div> | ||
| </article> | ||
| )) | ||
| ) : ( | ||
| <p className="text-sm text-stone-500"> | ||
| Draft metadata will appear after analysis. | ||
| </p> | ||
| )} | ||
| </div> | ||
| </div> | ||
|
|
||
| <div className="rounded-xl border border-sage-200 bg-sage-50 p-4"> | ||
| <div className="text-sm font-semibold text-sage-900">Ask this session</div> | ||
| <div className="mt-3 flex flex-col gap-3 lg:flex-row"> | ||
| <input | ||
| value={question} | ||
| onChange={event => setQuestion(event.target.value)} | ||
| className="flex-1 rounded-xl border border-sage-200 bg-white px-3 py-2 outline-none ring-0 focus:border-primary-500" | ||
| placeholder="What were my best fights?" | ||
| /> | ||
| <button | ||
| type="button" | ||
| onClick={handleQuestion} | ||
| disabled={questionBusy} | ||
| className="rounded-xl bg-sage-700 px-4 py-2 text-sm font-semibold text-white transition hover:bg-sage-800 disabled:cursor-not-allowed disabled:opacity-60"> | ||
| {questionBusy ? 'Thinking…' : 'Ask session'} | ||
| </button> | ||
| </div> | ||
| {questionAnswer && ( | ||
| <p className="mt-3 whitespace-pre-line text-sm text-stone-800"> | ||
| {questionAnswer} | ||
| </p> | ||
| )} | ||
| {activeSession.analysis?.follow_up_questions && | ||
| activeSession.analysis.follow_up_questions.length > 0 && ( | ||
| <div className="mt-3 flex flex-wrap gap-2 text-xs text-stone-600"> | ||
| {activeSession.analysis.follow_up_questions.map(item => ( | ||
| <button | ||
| key={item} | ||
| type="button" | ||
| onClick={() => setQuestion(item)} | ||
| className="rounded-full border border-sage-200 bg-white px-3 py-1 transition hover:bg-sage-50"> | ||
| {item} | ||
| </button> | ||
| ))} | ||
| </div> | ||
| )} | ||
| </div> | ||
| </div> | ||
| ) : ( | ||
| <div className="rounded-xl border border-dashed border-stone-300 bg-stone-50 p-8 text-center text-sm text-stone-500"> | ||
| Import a folder of keyframes to create the first gameplay session. | ||
| </div> | ||
| )} | ||
| </section> | ||
|
|
||
| <aside className="space-y-4 rounded-2xl border border-stone-200 bg-white p-5 shadow-soft"> | ||
| <div> | ||
| <h3 className="text-lg font-semibold text-stone-900">Session history</h3> | ||
| <p className="text-sm text-stone-500"> | ||
| Previously imported or analyzed sessions for this workspace. | ||
| </p> | ||
| </div> | ||
| <div className="space-y-3"> | ||
| {recentSessions.length > 0 ? ( | ||
| recentSessions.map(session => ( | ||
| <button | ||
| key={session.session_id} | ||
| type="button" | ||
| onClick={() => setActiveSession(session)} | ||
| className={`w-full rounded-xl border p-3 text-left transition ${ | ||
| activeSession?.session_id === session.session_id | ||
| ? 'border-primary-300 bg-primary-50' | ||
| : 'border-stone-200 bg-white hover:bg-stone-50' | ||
| }`}> | ||
| <div className="flex items-start justify-between gap-3"> | ||
| <div> | ||
| <div className="font-medium text-stone-900">{session.session_title}</div> | ||
| <div className="text-xs text-stone-500">{session.game_id}</div> | ||
| </div> | ||
| <div className="text-right text-[11px] text-stone-500"> | ||
| <div>{formatSpoilerMode(session.spoiler_mode)}</div> | ||
| <div>{session.frames.length} frame(s)</div> | ||
| </div> | ||
| </div> | ||
| <div className="mt-2 text-xs text-stone-500"> | ||
| Imported {formatTimestamp(session.imported_at_ms)} | ||
| </div> | ||
| </button> | ||
| )) | ||
| ) : ( | ||
| <p className="text-sm text-stone-500">No saved sessions yet.</p> | ||
| )} | ||
| </div> | ||
| <button | ||
| type="button" | ||
| onClick={() => void refreshSessions()} | ||
| className="w-full rounded-xl border border-stone-200 bg-white px-4 py-2 text-sm font-semibold text-stone-700 transition hover:bg-stone-50"> | ||
| Refresh history |
There was a problem hiding this comment.
Route all user-visible copy through useT() instead of hard-coded literals.
This component currently hard-codes many UI/error/toast strings (for example labels/placeholders in the form, setError(...) messages, and toast titles/messages). That breaks the app-wide i18n contract for app/src/**.
As per coding guidelines: “Every user-visible string in app/src/** … must go through useT() … hard-coded literals are not allowed. Add the new key to app/src/lib/i18n/en.ts in the same PR.”
🤖 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/src/components/intelligence/GameplayReviewWorkspace.tsx` around lines 64
- 590, GameplayReviewWorkspace contains hard-coded user-visible strings (labels,
placeholders, button text, error messages via setError, and toast
titles/messages passed to onToast) that must be routed through the i18n hook;
replace each literal in GameplayReviewWorkspace (search for use in the
GameplayReviewWorkspace function: input placeholders, <option> text, button
labels, the setError(...) invocations, and onToast calls) with calls to useT()
keys and add corresponding entries to app/src/lib/i18n/en.ts in this PR so all
UI strings (including recap/follow-up text, highlights/clip headings, and status
messages like "Importing…", "Analyzing…", success/error toast titles/messages)
are localized.
…erialization (tinyhumansai#2856) Per CodeRabbit: schemas for `register_session`, `analyze_session`, `set_preset`, `ask_session`, and `draft_clip_metadata` advertised wrapped params (`session`, `analysis`, `preset`, `question`, `clip`) under `TypeSchema::Ref(<Input>)`, but the handlers `deserialize_params::<Gameplay*Input>(params)` directly. Schema-driven clients hitting these RPCs would send `{ "session": {...} }` and bounce off serde with "missing field game_id" etc. Inline the input-struct fields in each schema so the wire shape matches what the handlers actually consume. Tests: cargo test gameplay_review 2/2 pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Actionable comments posted: 0 |
…ce branches (tinyhumansai#2856) Add a dedicated Vitest suite for gameplayReviewService (previously 0% coverage) exercising frame preparation, every RPC wrapper, flatten helpers, spoiler-mode formatting, and error normalization — mocking callCoreRpc directly so the real service runs. Extend the GameplayReviewWorkspace suite to cover the previously uncovered branches: initial fetch failure, import validation, empty-frame and registration-error paths, analyzed-session rendering (highlights, clips, drafts, follow-ups, spoiler note), empty states, clip-metadata refresh success/failure, ask-session failure, history switching, and the spoiler/audio/question input handlers. Lifts diff-covered lines well above the 80% gate: service 100% lines, workspace 98% lines. Co-Authored-By: Claude <noreply@anthropic.com>
|
Actionable comments posted: 0 |
graycyrus
left a comment
There was a problem hiding this comment.
@sanil-23 heads up — CI is failing on Rust Core Tests + Quality and Rust Core Coverage, so i'll hold off on a full approval until those are green. i did go through the diff though and spotted a couple of things worth fixing before the next pass.
This is a solid contribution — the architecture is clean, the path-traversal guard on session_id is good, the store is safe, logging density is appropriate, and the test coverage across both the service and workspace component is thorough. The main thing to fix is the return type on listGameplayPresets.
What needs fixing:
listGameplayPresets in gameplayReviewService.ts is typed as Promise<GameplayPresetInput[]>, but the server returns GameplayReviewPreset[] (which includes updated_at_ms). GameplayPresetInput is the inbound write shape, not the stored shape. Any code that calls this and expects updated_at_ms (or that TypeScript checks against GameplayReviewPreset) will either get a type error or silently miss the field at runtime. Needs to be changed to Promise<GameplayReviewPreset[]> with a matching type parameter on the callCoreRpc call.
logGameplayError is exported but never used inside or outside the module. Your PR description already calls this out as a known issue — worth just removing it before merge so you're not shipping dead API surface.
Fix the CI failures and the two above, and i'll come back and approve this.
tinyhumansai#2856) PR tinyhumansai#2853 added `health_snapshot: CORE_RPC_METHODS.healthSnapshot,` to LEGACY_METHOD_ALIASES. The key `health_snapshot` is a valid JS identifier so prettier (correctly) strips quotes from it — different shape from the other entries which are dotted strings (`'openhuman.foo':`) and need quotes. The Rust contract test `frontend_legacy_aliases_match_server_alias_table` always called `quoted_value(legacy)` on the key, panicking on any bare identifier: expected quoted value in `health_snapshot` Accept both forms in the parser: quoted strings via `quoted_value`, bare identifiers as-is. This matches JS object-literal semantics and ends the prettier↔Rust-test fight. Tests: cargo test core::legacy_aliases::tests 20/20 pass (was failing on the frontend match test prior to this commit). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Actionable comments posted: 0 |
graycyrus
left a comment
There was a problem hiding this comment.
@sanil-23 CI is still pending on this one, so holding off on a formal pass — but pinging because the two open items from my last review haven't been addressed yet:
listGameplayPresetsreturn type is stillPromise<GameplayPresetInput[]>— the server returnsGameplayReviewPreset[](includesupdated_at_ms).GameplayPresetInputis the write shape, not the stored record. That's a real type mismatch at any callsite that readsupdated_at_msor uses the preset as a full record.logGameplayErroris still exported and unused. Just delete it.
Once CI is green and those two are sorted, I'll come back and approve.
# Conflicts: # src/core/legacy_aliases.rs
graycyrus
left a comment
There was a problem hiding this comment.
@sanil-23 still holding on this one — CI is all pending again (looks like another upstream merge triggered a fresh run) and the two items from my earlier pass are still open:
listGameplayPresetsingameplayReviewService.ts:188is still typedPromise<GameplayPresetInput[]>. The server returnsGameplayReviewPreset[](includesupdated_at_ms). Fix the generic:callCoreRpc<GameplayReviewPreset[]>.logGameplayErroris exported but never called. Delete it.
Once CI goes green and those are addressed I'll do a final pass and approve.
…adiction on Embedding 401 + Invalid token PRs tinyhumansai#2830 and tinyhumansai#2786 both shipped on main and made contradictory assertions for the SAME wire shape: Embedding API error (401 Unauthorized): {"success":false,"error":"Invalid token"} - tinyhumansai#2830 added `is_embedding_backend_auth_failure` and a test asserting `BackendUserError`. - tinyhumansai#2786 added `classifies_embedding_api_invalid_token_401_as_session_expired` asserting `SessionExpired`. The tinyhumansai#2830 arm runs first in `expected_error_kind`, so the tinyhumansai#2786 test fails in CI on every PR that rebases onto current main (verified on upstream/main @ e83bfd6). Per the doc evidence and breadcrumb context (`[scheduler_gate] signed_out false -> true` immediately preceding the 401), the SessionExpired routing is the correct one — the OpenHuman backend envelope `{"success":false, "error":"Invalid token"}` is the JWT-invalidity branch of the same session-renewal flow as TAURI-RUST-4P0. Disable `is_embedding_backend_auth_failure` (keep the function as a doc breadcrumb so the regression is traceable) and remove the contradicting `classifies_embedding_backend_auth_failure` test. The SessionExpired arm in `is_session_expired_message` (added by tinyhumansai#2786) now catches the wire shape correctly. BYO-key embedding 401s (no OpenHuman envelope) still escalate to Sentry — guarded by `does_not_classify_embedding_byo_key_401_as_session_expired`. Local tests: cargo test core::observability::tests → 117/117 pass. Local repro: `classifies_embedding_api_invalid_token_401_as_session_expired` panicked on pure upstream/main before this commit; passes after. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
graycyrus
left a comment
There was a problem hiding this comment.
@sanil-23 — still holding on this one. CI is pending (Build Tauri, Rust Core Tests, Frontend unit tests, E2E all queued), and the two findings from my first review remain unaddressed:
-
listGameplayPresets(line 188) is still typedPromise<GameplayPresetInput[]>— the server returnsGameplayReviewPreset[]which includesupdated_at_ms. Using the write-shape as the read type will silently drop that field at every callsite. This needs to be corrected before I can approve. -
logGameplayError(line 231) is still exported and still uncalled. You flagged it yourself in the PR description. Delete it.
The new commit (2ecd5dd) is unrelated to gameplay — it fixes the Embedding 401 observability classification conflict between #2786 and #2830 by retiring the is_embedding_backend_auth_failure arm in favour of the SessionExpired routing. The logic is correct and well-commented. One minor cleanup: since the function now unconditionally returns false, you can delete it and its call site entirely rather than leaving it as permanent dead code — but that's a low-priority nit, not a blocker.
Fix the return type and delete the dead export, then let CI run to green. Happy to approve once both are done.
# Conflicts: # src/core/observability.rs
graycyrus
left a comment
There was a problem hiding this comment.
@sanil-23 — checking in again. CI still has pending checks (E2E macOS/Windows Appium, Rust Core Tests + Quality), and the two open threads from my earlier reviews haven't been addressed:
listGameplayPresetsreturn type — still typedPromise<GameplayPresetInput[]>but the server returnsGameplayReviewPreset[]. This is a real type bug that will cause issues at callsites.logGameplayError— still exported and unused. Your PR description already calls it out as a known leftover; just delete it.
Newest commit (a069fc3) is another upstream merge — no gameplay code changed. Fix those two things and get CI green and this is ready to go.
…s return type, drop logGameplayError (tinyhumansai#2856) graycyrus flagged two outstanding items on the gameplay PR: 1. `listGameplayPresets` was typed `Promise<GameplayPresetInput[]>` but the Rust backend returns `GameplayReviewPreset[]` (the read-shape, which adds `updated_at_ms`). Using the write-shape as the read type silently dropped that field at every callsite. Add a `GameplayReviewPreset` interface that extends `GameplayPresetInput` with `updated_at_ms: number` and update the return type. 2. `logGameplayError` was exported but never called from any component (only from its own test). The PR body called it out as deferred — removing it per graycyrus's request before approval. Also dropped the now-unused `errLog` debug instance and the corresponding test block. Verified: pnpm typecheck clean; gameplay tests pass within the 3819-test app suite. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
graycyrus
left a comment
There was a problem hiding this comment.
@sanil-23 CI is still pending so I can't fully sign off yet, but I reviewed the code and found one new issue plus some thread cleanup.
Prior feedback addressed:
listGameplayPresetsreturn type fixed (nowPromise<GameplayReviewPreset[]>). Resolved.logGameplayErrorremoved. Resolved.- Schema wrapper mismatch fixed in ea8c64a. Good.
One new issue to fix before this can land:
preset_id is being stored as the preset's display name but the store looks it up as the game id. In GameplayReviewWorkspace.tsx, preset_id is set to form.presetName.trim() (e.g. "Apex preset"). But store::load_preset keys files by slugify(game_id) — so it would look for apex-preset.json, not apex-legends.json. The result is that presets are silently never loaded during analysis. The fix is to pass preset_id: form.gameId.trim() || null instead, since that matches the key used when the preset was saved.
CodeRabbit's open threads on i18n coverage in the workspace component and service helpers are still outstanding — those need addressing too before merge.
Fix CI, the preset_id assignment, and the two CodeRabbit i18n threads, and this is good to go.
…inyhumansai#2856) graycyrus [major]: `preset_id` was being assigned `form.presetName.trim()` — the display name — but the Rust store keys presets by `slugify(game_id)`. On `analyze_session`, `store::load_preset(&workspace_dir, session.preset_id)` treats `preset_id` as a `game_id`, so it looked for `<displayName>-preset.json` when the file is actually `<gameId>.json`. Presets silently never applied. Use `form.gameId.trim()` instead. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
# Conflicts: # app/src/pages/Intelligence.tsx
graycyrus
left a comment
There was a problem hiding this comment.
@sanil-23 hey! the code looks good to me. you fixed the preset_id/game_id mismatch from my last review, and the full diff is solid — path traversal guard on session_id, atomic writes, good test coverage on both the service and workspace sides, i18n key across all 14 locales. the prior changes i requested are all addressed.
CI is still pending though, so holding off on the formal approval until those checks go green. once they pass i'll come back and approve this.
…umansai#2856) CoderRabbit [Major / Quick win] on gameplayReviewService.ts:230: move user-visible strings out of service helpers and through i18n. `formatSpoilerMode` now returns the i18n key (gameplay.spoiler.{off,light, full}) instead of the literal English string. Callsites in GameplayReviewWorkspace.tsx wrap the result with `t()`. Added the 3 keys to en.ts + all 14 locale chunks (English placeholder per project policy). Tests updated to assert the key strings. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
graycyrus
left a comment
There was a problem hiding this comment.
@sanil-23 hey — the preset_id fix in bd06516 is exactly right, and the formatSpoilerMode i18n wiring looks good too. all prior findings are addressed and there's nothing blocking from my side. CI is still pending though, so i'll hold the approve until those checks go green. just ping me once they pass.
M3gA-Mind
left a comment
There was a problem hiding this comment.
All CI green (25/25 checks). Code review: path traversal guard on session_id correct, atomic writes, preset_id correctly uses game_id not display name, listGameplayPresets return type fixed, logGameplayError removed, formatSpoilerMode returns i18n keys used via t() in UI. Deferred i18n body strings and serial frame analysis perf are acknowledged follow-ups. Tests pass locally (2/2 Rust, i18n gate passes). Approving.
|
@coderabbitai review The three inline findings have all been addressed in subsequent commits:
The full |
|
✅ Actions performedReview triggered.
|
Summary
This supersedes #1677 (@Ayush-Raj-Chourasia) — same feature, rebased on current
mainwith merge conflicts resolved, CodeRabbit review feedback applied, and a compilation error fix.The original PR had
Allow edits by maintainersoff, so I could not push the rebase / fixes onto the contributor's branch. Opening from my fork to keep the work moving. Original authorship preserved viaCo-authored-byon the rebase/fix commit.What changed vs #1677
app/src/pages/Intelligence.tsxandsrc/core/all.rs(main moved forward ~2 weeks since the original push).src/openhuman/agent/schemas.rs— theto_jsonhelper had the wrong signature (32 cargo errors).session_idin the gameplay store.memory.tab.gameplayi18n key across all 14 locale chunks.rustfmt+ alphabeticalmod.rsordering.Deferred follow-ups (not in this PR)
GameplayReviewWorkspace.tsxbody strings (~30 keys).logGameplayErrorexport.Test plan
cargo fmt --checkcleancargo checkcleancargo test --libgreenpnpm typecheckcleanpnpm lintcleanpnpm vitestfull suite greenCloses #1674
Supersedes #1677
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation