feat(watchprovider): add Floppy plugin - #1
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds a complete Floppy WatchSync plugin. It includes SDK runtime wiring, manifest and build configuration, an HTTP API client, credential handling, account validation, scrobbling, paginated state synchronization, media conversion, fault mapping, and provider tests. ChangesFloppy WatchSync integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant WatchSyncHost
participant Server
participant FloppyAPI
WatchSyncHost->>Server: ExchangeAPIKey or ApplyEvents
Server->>FloppyAPI: Validate token or submit scrobble
FloppyAPI-->>Server: Account or scrobble response
Server-->>WatchSyncHost: Credential or event result
sequenceDiagram
participant WatchSyncHost
participant Server
participant FloppyAPI
WatchSyncHost->>Server: ListRemoteState with page cursor
Server->>FloppyAPI: Request watched history or progress page
FloppyAPI-->>Server: Paginated history or progress JSON
Server-->>WatchSyncHost: Converted remote states and next cursor
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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: 8
🧹 Nitpick comments (2)
provider/provider_test.go (1)
96-125: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a case for episode events that carry episode-level external IDs.
This test sets only
SeriesExternalIds, somergedExternalIDsreturns the seriestmdbID and the history match succeeds. The untested case is an episode event withExternalIds["tmdb"]set to an episode-level ID plus a series ID inSeriesExternalIds. That case exercises the namespace conflation described inprovider/provider.go(Line 395) and would show whether a completed watch is re-scrobbled.🤖 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 `@provider/provider_test.go` around lines 96 - 125, Extend the ApplyEvents test around the existing event setup to include episode-level ExternalIds["tmdb"] alongside the series SeriesExternalIds, using distinct episode and series IDs. Verify repeated application still returns APPLIED then NO_CHANGE and scrobbleCalls remains 1, exercising mergedExternalIDs without re-scrobbling a completed watch.provider/provider.go (1)
360-379: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDecode raw JSON values instead of unquoting strings.
strconv.Unquotefails on valid JSON strings that contain escapes such as\/or\uXXXX.rawStringthen returns the value with its surrounding quotes, which leaks quotes intoMediaItemIdandProviderItemKey.stringValuealso relies on the"<nil>"text produced byfmt.Sprintto filter null values innormalizedIDs.♻️ Proposed refactor
func rawString(value json.RawMessage) string { trimmed := strings.TrimSpace(string(value)) - if unquoted, err := strconv.Unquote(trimmed); err == nil { - return unquoted - } + var decoded string + if err := json.Unmarshal(value, &decoded); err == nil { + return decoded + } + if trimmed == "null" { + return "" + } return trimmed } func stringValue(value any) string { switch typed := value.(type) { + case nil: + return "" case string: return typedReturning "" for JSON
nullalso fixes thenullinstance ID case flagged inprovider/list_state.go(Line 115).🤖 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 `@provider/provider.go` around lines 360 - 379, Update rawString to decode the JSON value with JSON unmarshalling, preserving escaped characters correctly and returning an empty string for JSON null or invalid values as appropriate. Update stringValue to handle nil explicitly by returning an empty string instead of relying on fmt.Sprint output, while preserving existing string and numeric conversions used by normalizedIDs.
🤖 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 `@go.mod`:
- Line 23: Upgrade the google.golang.org/grpc dependency from v1.75.1 to at
least v1.82.1, including the transitive runtime dependency introduced through
pluginsdk. Regenerate go.mod and go.sum, then rerun vulnerability scanning to
verify the resolved module graph is no longer affected.
In `@LICENSE`:
- Line 1: Remove the local environment warning preceding the license text in
LICENSE so the file begins directly with the GNU AFFERO GENERAL PUBLIC LICENSE
header.
In `@provider/list_state.go`:
- Around line 115-118: Update the provider key construction around
entry.InstanceID to detect a missing/null instance ID explicitly rather than
relying on strings.HasSuffix(providerKey, ":"). Use the timestamp-based fallback
for null or empty instance IDs, while preserving the normal history key for
valid instance IDs.
- Around line 223-253: The historyContainsEvent lookup must not stop after the
first 10 history entries, since matching completed sessions may appear on later
pages. Change the query limit to defaultPageSize and follow
upstream.Pagination.Next, fetching and examining each page until the match is
found or pagination is exhausted while preserving the existing validation and
matching logic.
In `@provider/provider_test.go`:
- Around line 122-124: Protect all shared test state in the relevant test with
the existing mu: read scrobbleCalls in the assertions around its current checks
and copy or inspect queries under the same lock before asserting. Keep handler
writes and test expectations synchronized consistently so no cross-goroutine
access remains.
- Around line 21-27: Replace t.Fatal/t.Fatalf calls inside all HTTP handler
callbacks in provider tests, including the handlers around the upstream server
and affected locations, with t.Errorf; after recording each failure, send an
HTTP error response and return immediately. Update writeJSON or its handler
usage similarly so handler goroutines never invoke t.Fatal* while preserving the
existing assertions and response behavior for successful requests.
In `@provider/provider.go`:
- Around line 78-89: Make ApplyEvents idempotent across retries for every
event_id by adding durable deduplication or propagating an upstream idempotency
key before applyEvent posts events. Ensure duplicate SCROBBLE_START and
SCROBBLE_PAUSE events are not applied again, and preserve the existing response
behavior while preventing retries after connectionFault from reapplying
completed events.
- Around line 395-411: Update mergedExternalIDs and the mediaMatchesHistory
matching flow so episode-level media uses only SeriesExternalIds when comparing
against history provider IDs, rather than merging episode and series namespaces.
Preserve the existing ID filtering and fallback behavior for non-episode media,
and verify the scrobble payload’s ids handling uses the endpoint-supported
identifier set.
---
Nitpick comments:
In `@provider/provider_test.go`:
- Around line 96-125: Extend the ApplyEvents test around the existing event
setup to include episode-level ExternalIds["tmdb"] alongside the series
SeriesExternalIds, using distinct episode and series IDs. Verify repeated
application still returns APPLIED then NO_CHANGE and scrobbleCalls remains 1,
exercising mergedExternalIDs without re-scrobbling a completed watch.
In `@provider/provider.go`:
- Around line 360-379: Update rawString to decode the JSON value with JSON
unmarshalling, preserving escaped characters correctly and returning an empty
string for JSON null or invalid values as appropriate. Update stringValue to
handle nil explicitly by returning an empty string instead of relying on
fmt.Sprint output, while preserving existing string and numeric conversions used
by normalizedIDs.
🪄 Autofix
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: 9cee788d-2f79-402d-bed6-31033ec81ee6
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
.gitignoreLICENSEMakefileREADME.mdgo.modmain.gomanifest.jsonprovider/client.goprovider/list_state.goprovider/models.goprovider/provider.goprovider/provider_test.go
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4659b76788
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 344356e181
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@provider/provider_test.go`:
- Around line 318-320: Update the state assertion in the provider test to
validate every supplied series external ID, including confirming the “tvdb”
mapping alongside the existing “tmdb” value. Keep the existing media ID and
empty external-ID checks unchanged.
🪄 Autofix
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: f1e019d3-b812-45e5-9f05-5e5bfbafd13f
📒 Files selected for processing (5)
Makefileprovider/client.goprovider/list_state.goprovider/provider.goprovider/provider_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- Makefile
- provider/list_state.go
- provider/provider.go
- provider/client.go
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7fcc330f2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !more { | ||
| return entries, nil | ||
| } | ||
| offset = nextOffset |
There was a problem hiding this comment.
When Floppy returns multiple upstream pages and an item in an earlier page is deleted or completed between those GETs, advancing the numeric offset skips an active item that shifted into the previous page. If that item's updated_at is older than the collected high-water mark, the returned cursor then excludes it from later incremental traversals. Fresh evidence after the earlier pagination fix is that fetchProgressSnapshot still walks the mutable upstream result set with offsets; use a stable boundary or restart/refetch strategy within this loop.
Useful? React with 👍 / 👎.
| if requestFault != nil { | ||
| return &pluginv1.WatchSyncListRemoteStateResponse{Fault: requestFault}, nil | ||
| } | ||
| response := &pluginv1.WatchSyncListRemoteStateResponse{CompleteSnapshot: cursor.IsZero()} |
There was a problem hiding this comment.
When an already-imported Floppy progress row is deleted, it disappears from the completed=false endpoint, but every request with a cursor is marked as an incremental rather than complete snapshot. Because no tombstone is emitted either, Silo is never told to remove that provider item and can retain stale resume progress indefinitely. Return complete active-progress snapshots or otherwise represent deletions.
Useful? React with 👍 / 👎.
| // are not interchangeable with series IDs. | ||
| return seriesIDs | ||
| } | ||
| output := filteredExternalIDs(media.GetExternalIds()) |
There was a problem hiding this comment.
Although series IDs now win when present, an episode carrying only episode-level ExternalIds still falls through here and sends those IDs to Floppy as though they identified the series. Since Floppy resolves episodes using a series ID plus season and episode numbers, this can reject the scrobble or associate it with the wrong show; reject such events when SeriesExternalIds is empty rather than using episode identifiers.
Useful? React with 👍 / 👎.
1 participant
Summary
Dependencies
Validation
AI assistance disclosure
Changes were developed and reviewed with OpenAI Codex on behalf of repository maintainer Quick104. The maintainer directed scope and approved publication; validation includes automated checks and a live isolated deployment.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores