Fix clip suggestion: dedup vs history, real error surfacing, and title wrap#49
Merged
Merged
Conversation
Feed already-rendered clips for the current video plus in-session suggestions into the AI suggestion prompt as exclude_clips, and drop any returned moment overlapping an excluded range by >50%, so re-running suggestions stops re-proposing the same moments. Make the clip title editor a wrapping textarea so long titles are fully visible instead of scrolling out of a single-line input.
📝 WalkthroughWalkthroughThis PR adds an existing_clips exclusion mechanism to the clip suggestion pipeline: web-server.ts gathers previously rendered and currently suggested clip ranges, passes them through backend/main.py to claude_suggest.py, which filters out overlapping candidates before top-N selection. Also includes an unrelated ClipDetail.tsx textarea change and package.json version bump. ChangesExisting Clips Exclusion Flow
Unrelated UI and Version Updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant WebServer as web-server.ts
participant PythonMain as backend/main.py
participant ClaudeSuggest as claude_suggest.py
WebServer->>WebServer: build existing_clips from clipsHistory and uiState.suggestions
WebServer->>PythonMain: /api/claude-suggest params (segments, top_n, existing_clips)
PythonMain->>PythonMain: read existing_clips from params
PythonMain->>ClaudeSuggest: suggest_with_claude(exclude_clips=existing_clips)
ClaudeSuggest->>ClaudeSuggest: _drop_clips_overlapping(normalized, exclude_clips)
ClaudeSuggest->>ClaudeSuggest: select top-N from filtered clips
ClaudeSuggest-->>PythonMain: filtered clip suggestions
PythonMain-->>WebServer: response with new suggestions
🚥 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.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/ui/client/ClipDetail.tsx (1)
250-250: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueTextarea allows multi-line titles now.
Switching to a textarea permits embedded newlines in
title, which is then rendered elsewhere as a single line (e.g.<h1>{clip.title}</h1>at Line 222, and the generated-title buttons). A newline in the title could cause unexpected layout/wrapping in those single-line contexts. Consider stripping/normalizing newlines on save if titles are expected to remain single-line.🤖 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 `@src/ui/client/ClipDetail.tsx` at line 250, Using a textarea in ClipDetail allows embedded newlines in title, but title is later rendered in single-line places like the h1 and generated-title controls. Update the ClipDetail save flow and related title handling so newlines are stripped or normalized before persisting, and ensure the title state used by the textarea cannot save multiline content if titles must remain single-line.backend/services/claude_suggest.py (1)
687-709: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared overlap-ratio helper to avoid duplicating
_dedupe_clips_by_rangelogic.The overlap-ratio computation (lines 693-705) is nearly identical to the loop body in
_dedupe_clips_by_range(lines 674-681). Consolidating into a shared helper reduces the risk of the two implementations drifting if the 50% threshold or edge-case handling ever changes.♻️ Proposed refactor
+def _overlap_ratio(a_start: float, a_end: float, b_start: float, b_end: float) -> float: + """Overlap duration as a fraction of the shorter of the two ranges.""" + overlap = max(0.0, min(a_end, b_end) - max(a_start, b_start)) + shorter = min(max(0.0, a_end - a_start), max(0.0, b_end - b_start)) or 1.0 + return overlap / shorter + + def _drop_clips_overlapping(clips: list[dict], exclude_clips: list[dict]) -> list[dict]: """Drop clips that overlap an excluded range by >50% of the shorter clip. The prompt already asks the AI to skip these; this enforces it if it doesn't.""" if not exclude_clips: return clips kept = [] for clip in clips: start = float(clip.get("start_second", 0)) end = float(clip.get("end_second", 0)) - dur = max(0.0, end - start) overlaps = False for ex in exclude_clips: ex_start = float(ex.get("start_second", 0)) ex_end = float(ex.get("end_second", 0)) - overlap = max(0.0, min(end, ex_end) - max(start, ex_start)) - shorter = min(dur, max(0.0, ex_end - ex_start)) or 1.0 - if overlap / shorter > 0.5: + if _overlap_ratio(start, end, ex_start, ex_end) > 0.5: overlaps = True break if not overlaps: kept.append(clip) return kept
_dedupe_clips_by_rangecan be similarly updated to call_overlap_ratio.🤖 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 `@backend/services/claude_suggest.py` around lines 687 - 709, The overlap-ratio calculation in _drop_clips_overlapping is duplicated in _dedupe_clips_by_range, so extract a shared helper (for example, _overlap_ratio) and have both functions use it. Centralize the shared start/end/shorter-clip overlap logic so any future threshold or edge-case changes only need to be made in one place.src/ui/web-server.ts (2)
2640-2642: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilent failure on clip history lookup.
.catch(() => [])swallows any error fromclipsHistory.getBySourcewithout logging, making it harder to diagnose why exclusion isn't working if the lookup fails silently in production.🤖 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 `@src/ui/web-server.ts` around lines 2640 - 2642, The clip history lookup in uiState handling is swallowing errors by returning an empty list from clipsHistory.getBySource without any visibility. Update the code around rendered to catch the failure, log the actual error with enough context to identify the uiState.videoPath lookup, and then return the empty fallback only after logging so failures are diagnosable in production.
2638-2648: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider capping
existing_clipshistory size.
renderedpulls the full clip history for the video with no upper bound. As a video accumulates clips over time, this array grows unbounded and gets embedded into the AI prompt on every suggestion request, inflating token usage/cost and eventually risking prompt truncation.💡 Example: cap to most recent N rendered clips
- const rendered = uiState.videoPath - ? await clipsHistory.getBySource(uiState.videoPath).catch(() => []) - : []; + const rendered = uiState.videoPath + ? (await clipsHistory.getBySource(uiState.videoPath).catch(() => [])).slice(-50) + : [];🤖 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 `@src/ui/web-server.ts` around lines 2638 - 2648, Cap the clip history included in the AI prompt so it doesn’t grow without bound. In the suggestion flow that builds rendered and existing_clips in web-server.ts, limit the clips returned from clipsHistory.getBySource(uiState.videoPath) to only the most recent N entries before mapping them into existing_clips. Keep uiState.suggestions included, but ensure the combined prompt payload stays bounded and predictable.
🤖 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 `@package.json`:
- Line 3: The version update is only applied in package.json, while cli/main.go
still hardcodes the older Version value, so both sources need to be kept in
sync. Update the Version variable in cli/main.go to match the new package.json
version and ensure any future bump changes both package.json and the cli/main.go
Version constant together.
---
Nitpick comments:
In `@backend/services/claude_suggest.py`:
- Around line 687-709: The overlap-ratio calculation in _drop_clips_overlapping
is duplicated in _dedupe_clips_by_range, so extract a shared helper (for
example, _overlap_ratio) and have both functions use it. Centralize the shared
start/end/shorter-clip overlap logic so any future threshold or edge-case
changes only need to be made in one place.
In `@src/ui/client/ClipDetail.tsx`:
- Line 250: Using a textarea in ClipDetail allows embedded newlines in title,
but title is later rendered in single-line places like the h1 and
generated-title controls. Update the ClipDetail save flow and related title
handling so newlines are stripped or normalized before persisting, and ensure
the title state used by the textarea cannot save multiline content if titles
must remain single-line.
In `@src/ui/web-server.ts`:
- Around line 2640-2642: The clip history lookup in uiState handling is
swallowing errors by returning an empty list from clipsHistory.getBySource
without any visibility. Update the code around rendered to catch the failure,
log the actual error with enough context to identify the uiState.videoPath
lookup, and then return the empty fallback only after logging so failures are
diagnosable in production.
- Around line 2638-2648: Cap the clip history included in the AI prompt so it
doesn’t grow without bound. In the suggestion flow that builds rendered and
existing_clips in web-server.ts, limit the clips returned from
clipsHistory.getBySource(uiState.videoPath) to only the most recent N entries
before mapping them into existing_clips. Keep uiState.suggestions included, but
ensure the combined prompt payload stays bounded and predictable.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: be70b94f-6515-465d-8283-5e28cf079b93
📒 Files selected for processing (5)
backend/main.pybackend/services/claude_suggest.pypackage.jsonsrc/ui/client/ClipDetail.tsxsrc/ui/web-server.ts
| { | ||
| "name": "podcli", | ||
| "version": "2.2.1", | ||
| "version": "2.3.1", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Version bump not synced with cli/main.go.
cli/main.go hardcodes var Version = "2.2.1" independently of package.json. Bumping only package.json to 2.3.1 leaves the CLI reporting a stale version.
🤖 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 `@package.json` at line 3, The version update is only applied in package.json,
while cli/main.go still hardcodes the older Version value, so both sources need
to be kept in sync. Update the Version variable in cli/main.go to match the new
package.json version and ensure any future bump changes both package.json and
the cli/main.go Version constant together.
nmbrthirteen
changed the title
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.
Three fixes to the clip suggestion / clip detail flow:
1. Suggestions no longer repeat moments. Suggestion now knows about clips already rendered for the current video plus the moments already on screen, and skips those ranges (with a >50%-overlap safety filter dropping any repeat the AI returns anyway).
2. Real failure reason instead of a generic login hint. Suggestion already captured the actual failure (Claude/Codex stderr, a timeout, or unparseable output) but only sent it to transient progress and returned
None, so the user always saw "AI CLI found but suggestion failed - check claude/codex login" no matter the real cause. It now surfaces the actual detail and classifies it (auth vs usage/rate limit vs crash), e.g. "not logged in. Run `claude` once in a terminal to authenticate" or "usage or rate limit reached." Directly addresses the blocker in the studio.3. Long clip titles are fully visible. The clip title editor wraps instead of scrolling a single-line box.
Typecheck, build, and the 48-test suite pass.