feat(watchsync): expand provider plugin contract - #13
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe watch-sync protocol now supports device authorization, favorite, watchlist, and scrobbling operations, filtered remote-state families, list ordering, provider item keys, runtime service registration, and expanded SDK validation and serialization tests. ChangesWatch-sync protocol expansion
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Host
participant WatchSyncDeviceAuthorizationService
participant User
participant WatchSyncProvider
Host->>WatchSyncDeviceAuthorizationService: Start device authorization
WatchSyncDeviceAuthorizationService-->>Host: User code, verification URL, and polling metadata
Host->>User: Present authorization data
Host->>WatchSyncDeviceAuthorizationService: Poll authorization state
WatchSyncDeviceAuthorizationService-->>Host: Pending state or credentials
Host->>WatchSyncProvider: Apply watch-sync events
WatchSyncProvider-->>Host: Updated synchronization result
Possibly related PRs
Suggested reviewers: 🚥 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: 2
🧹 Nitpick comments (1)
pkg/pluginproto/silo/plugin/v1/watch_sync_provider_test.go (1)
72-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the exact values of all new round-trip fields.
pkg/pluginproto/silo/plugin/v1/watch_sync_provider_test.go#L72-L75: AssertVerificationUrl,VerificationUrlComplete, andProviderStateafter unmarshalling.pkg/pluginsdk/convert/watch_sync_provider_test.go#L35-L37: Assert thatAuthMethods[0]isWATCH_SYNC_AUTH_METHOD_DEVICE_CODE.🤖 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 `@pkg/pluginproto/silo/plugin/v1/watch_sync_provider_test.go` around lines 72 - 75, Strengthen the round-trip assertions in pkg/pluginproto/silo/plugin/v1/watch_sync_provider_test.go:72-75 by validating the exact VerificationUrl, VerificationUrlComplete, and ProviderState values after unmarshalling, alongside the existing fields. In pkg/pluginsdk/convert/watch_sync_provider_test.go:35-37, assert that AuthMethods[0] equals WATCH_SYNC_AUTH_METHOD_DEVICE_CODE.
🤖 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 `@proto/silo/plugin/v1/watch_sync_provider.proto`:
- Around line 19-20: Rename the request and response message declarations used
by StartDeviceAuthorization and PollDeviceAuthorization to
WatchSyncProviderStartDeviceAuthorizationRequest/Response and
WatchSyncProviderPollDeviceAuthorizationRequest/Response, update all RPC and
code references to the new names, and regenerate the protobuf bindings.
- Around line 283-285: Change list_position to an optional int32 field in the
proto definition to preserve the distinction between an omitted value and
explicit zero, then regenerate the corresponding bindings. Add or update tests
to verify presence detection for both absent and zero-valued list_position.
---
Nitpick comments:
In `@pkg/pluginproto/silo/plugin/v1/watch_sync_provider_test.go`:
- Around line 72-75: Strengthen the round-trip assertions in
pkg/pluginproto/silo/plugin/v1/watch_sync_provider_test.go:72-75 by validating
the exact VerificationUrl, VerificationUrlComplete, and ProviderState values
after unmarshalling, alongside the existing fields. In
pkg/pluginsdk/convert/watch_sync_provider_test.go:35-37, assert that
AuthMethods[0] equals WATCH_SYNC_AUTH_METHOD_DEVICE_CODE.
🪄 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: 6f8e2c97-8774-479d-a9d4-cc845ce78c41
⛔ Files ignored due to path filters (2)
pkg/pluginproto/silo/plugin/v1/watch_sync_provider.pb.gois excluded by!**/*.pb.gopkg/pluginproto/silo/plugin/v1/watch_sync_provider_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (6)
README.mdpkg/pluginproto/silo/plugin/v1/watch_sync_provider_test.gopkg/pluginsdk/convert/watch_sync_provider_test.gopkg/pluginsdk/manifest/manifest.gopkg/pluginsdk/manifest/watch_sync_provider_test.goproto/silo/plugin/v1/watch_sync_provider.proto
![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: 3b705d7e88
ℹ️ 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 `@pkg/pluginproto/silo/plugin/v1/watch_sync_provider_test.go`:
- Around line 119-121: Update the test around WatchSyncEvent to exercise the
omitted wire path: marshal an empty WatchSyncEvent, unmarshal the bytes into a
new WatchSyncEvent, and assert the resulting ListPosition is nil. Retain the
existing direct struct assertion only if useful, and use the test’s existing
protobuf error-handling conventions.
🪄 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: f89db974-bf19-4355-afb3-2ff0fadb6afb
⛔ Files ignored due to path filters (2)
pkg/pluginproto/silo/plugin/v1/watch_sync_provider.pb.gois excluded by!**/*.pb.gopkg/pluginproto/silo/plugin/v1/watch_sync_provider_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (7)
README.mdpkg/pluginproto/silo/plugin/v1/watch_sync_provider_compat_test.gopkg/pluginproto/silo/plugin/v1/watch_sync_provider_test.gopkg/pluginsdk/convert/watch_sync_provider_test.gopkg/pluginsdk/runtime/runtime.gopkg/pluginsdk/runtime/watch_sync_provider_test.goproto/silo/plugin/v1/watch_sync_provider.proto
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/pluginsdk/convert/watch_sync_provider_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/pluginproto/silo/plugin/v1/watch_sync_provider_test.go (1)
124-141: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExercise the
Watchlisttombstone path too.This test covers only
Favorite. The contract permits tombstones withoutMediain bothFavoriteandWatchlist. A regression in theWatchlistwire path would pass this test. Run the same marshal/unmarshal assertions for both fields, preferably with table-driven cases.🤖 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 `@pkg/pluginproto/silo/plugin/v1/watch_sync_provider_test.go` around lines 124 - 141, Extend TestWatchSyncListTombstoneDoesNotRequireMedia to cover both Favorite and Watchlist tombstones, preferably with table-driven cases. For each field, marshal and unmarshal a WatchSyncRemoteState containing a Removed tombstone without Media, then assert proto equality, nil Media, and the tombstone’s Removed value.
🤖 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 `@pkg/pluginproto/silo/plugin/v1/watch_sync_provider_test.go`:
- Around line 124-141: Extend TestWatchSyncListTombstoneDoesNotRequireMedia to
cover both Favorite and Watchlist tombstones, preferably with table-driven
cases. For each field, marshal and unmarshal a WatchSyncRemoteState containing a
Removed tombstone without Media, then assert proto equality, nil Media, and the
tombstone’s Removed value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dce1af35-e341-4f7a-9c0f-0cdc57854438
⛔ Files ignored due to path filters (2)
pkg/pluginproto/silo/plugin/v1/watch_sync_provider.pb.gois excluded by!**/*.pb.gopkg/pluginproto/silo/plugin/v1/watch_sync_provider_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (3)
README.mdpkg/pluginproto/silo/plugin/v1/watch_sync_provider_test.goproto/silo/plugin/v1/watch_sync_provider.proto
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- proto/silo/plugin/v1/watch_sync_provider.proto
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c33f66caa5
ℹ️ 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: d4b0770d73
ℹ️ 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".
Contributor
Author
|
Final review fixes are at
|
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.
Problem
The released watch_sync_provider.v1 contract only covers API-key or authorization-code setup, watched-state exchange, and completed playback. That is not enough to extract the existing Trakt, Simkl, and MDBList providers or implement a self-hosted Floppy provider without provider-specific host code.
Approach
The additions are protobuf-wire compatible with v0.12 plugins. New fields default to disabled, so hosts expose only operations a plugin actually advertises.
Validation
Part of Silo-Server/silo-server#475.
Release note
The server and first Floppy plugin should consume this through a v0.13.0 SDK release after merge.
AI use disclosure
OpenAI Codex was used to inspect the existing contract, implement the protobuf and validator changes, and run the recorded tests. The maintainer is responsible for the submitted design and changes.
Summary by CodeRabbit