Remove alpha from Teal lexicon namespace - #68
Conversation
|
related: teal-fm/teal#110 |
![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: 1dd66e8038
ℹ️ 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".
| fmt.Println(clientId, callbackUrl) | ||
|
|
||
| scopes := []string{"atproto", "repo:fm.teal.alpha.feed.play", "repo:fm.teal.alpha.actor.status"} | ||
| scopes := []string{"atproto", "repo:fm.teal.feed.play", "repo:fm.teal.actor.status"} |
There was a problem hiding this comment.
On an upgrade, sessions issued before this commit still contain only the persisted repo:fm.teal.alpha.feed.play and repo:fm.teal.alpha.actor.status grants; changing the scopes requested by new login flows cannot add permissions to those existing tokens. Because GetATProtoClient resumes the stored session while submission and playing-now now write exclusively to the renamed collections, existing users will receive authorization errors until they manually authenticate again. Detect stale scopes and force reauthorization or provide a compatible migration path.
Useful? React with 👍 / 👎.
|
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 selected for processing (1)
📝 WalkthroughWalkthroughThe PR migrates TEAL lexicons and generated APIs from ChangesTEAL namespace migration
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: 🟠 High · up to The PR changes the production lexicon and generated record handling, but it still risks silently losing metadata from previously written records and can create invalid play views when clearing status. These correctness issues should be fixed before merge. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/teal/cbor_gen.go (1)
2503-2521: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve read compatibility for renamed fields.
The previous lexicon defined
originUrlandmusicServiceBaseDomain. Both decoders now accept onlyoriginUriandmusicServiceUri. Old fields are consumed and discarded without an error. If existing PDS records use the previous names, add a dual-read or backfill path before deployment. Apply this to theFeedPlaydecoders at Lines 783-801 and 1000-1018.🤖 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 `@api/teal/cbor_gen.go` around lines 2503 - 2521, Update the FeedPlay decoders, including the paths around the originUri/musicServiceUri cases and the corresponding sections at the earlier decoder locations, to also recognize the legacy originUrl and musicServiceBaseDomain field names. Decode those values into the current FeedPlay fields (or otherwise backfill them when the new names are absent), while preserving existing handling for current names and unknown fields.
🤖 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 `@service/playingnow/playingnow.go`:
- Around line 172-175: Update the actor-status schema and the emptyPlayView
construction in the surrounding playing-now flow so the cleared state is
schema-valid instead of using an empty TrackName. Either model an explicit
cleared state in the schema or make the item optional and omit it, then
regenerate the API and CBOR bindings and update callers to use the generated
representation.
---
Outside diff comments:
In `@api/teal/cbor_gen.go`:
- Around line 2503-2521: Update the FeedPlay decoders, including the paths
around the originUri/musicServiceUri cases and the corresponding sections at the
earlier decoder locations, to also recognize the legacy originUrl and
musicServiceBaseDomain field names. Decode those values into the current
FeedPlay fields (or otherwise backfill them when the new names are absent),
while preserving existing handling for current names and unknown fields.
🪄 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: ad75db14-69c5-47fb-9b75-ab605aa5be6d
📒 Files selected for processing (29)
api/teal/actordefs.goapi/teal/actorgetProfile.goapi/teal/actorgetProfiles.goapi/teal/actorprofile.goapi/teal/actorsearchActors.goapi/teal/actorstatus.goapi/teal/cbor_gen.goapi/teal/feeddefs.goapi/teal/feedgetActorFeed.goapi/teal/feedgetPlay.goapi/teal/feedplay.gocmd/lexgen/main.golexicons/teal/actor/defs.jsonlexicons/teal/actor/getProfile.jsonlexicons/teal/actor/getProfiles.jsonlexicons/teal/actor/profile.jsonlexicons/teal/actor/searchActors.jsonlexicons/teal/actor/status.jsonlexicons/teal/feed/defs.jsonlexicons/teal/feed/getActorFeed.jsonlexicons/teal/feed/getPlay.jsonlexicons/teal/feed/play.jsonmodels/lexicon_format.gomodels/lexicon_format_test.gooauth/atproto/atproto.goservice/atproto/submission.goservice/playingnow/playingnow.goservice/playingnow/playingnow_test.goservice/spotify/spotify.go
| emptyPlayView := &teal.FeedDefs_PlayView{ | ||
| TrackName: "", // Empty track indicates no current playing | ||
| Artists: []*teal.AlphaFeedDefs_Artist{}, | ||
| Artists: []*teal.FeedDefs_Artist{}, | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Define a schema-valid cleared status.
Line 173 sets TrackName to "". lexicons/teal/feed/defs.json requires trackName with minLength: 1. This clear record contains an invalid FeedDefs_PlayView.
Model a cleared state in the actor-status schema, or make item optional before omitting it here. Regenerate the API and CBOR bindings after the schema change.
🤖 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 `@service/playingnow/playingnow.go` around lines 172 - 175, Update the
actor-status schema and the emptyPlayView construction in the surrounding
playing-now flow so the cleared state is schema-valid instead of using an empty
TrackName. Either model an explicit cleared state in the schema or make the item
optional and omit it, then regenerate the API and CBOR bindings and update
callers to use the generated representation.
1 participant
Coauthored by GPT 5.6 Luna
Summary
originUriandmusicServiceUrifields as absolute HTTPS URIs.fm.teal.*namespace.Behavior and compatibility
Piper is a scrobbler/producer and does not consume existing listens, so it does not need dual alpha/production read support. New submissions use
fm.teal.feed.play; legacy alpha records are not rewritten.Validation
make lexicongo test ./...git diff --checklexicons/teal/feed/play.jsonandlexicons/teal/feed/defs.jsonagainst Teal mainCanonical source:
teal-fm/tealmain merge commit82ea66cadbde3f1aee9c72b05f372c6543a4b3f6, merged PR #110.Summary by CodeRabbit