fix(apiv2): remove legacy web transport and URL leaks - #1051
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # The optional generic allows one level of nesting (api<Partial<Progress>>(...)). | ||
| WEB_CALL = re.compile( | ||
| r"\b(api|apiResponse|apiKeepalive|apiDownload|apiBlob|apiWithProfileRequestContext|apiFetch|fetch|apiFormData|apiUpload|playerFetch)\s*(<(?:[^<>]|<[^<>]*>)*>)?\s*\(" | ||
| r"\b(v2|api|apiResponse|apiKeepalive|apiDownload|apiBlob|apiWithProfileRequestContext|apiFetch|fetch|apiFormData|apiUpload|playerFetch)\s*(<(?:[^<>]|<[^<>]*>)*>)?\s*\(" |
There was a problem hiding this comment.
The scanner now recognizes literal v2("METHOD /api/v2/…") calls, but it still discards calls whose first argument is an identifier. setRoomSuggestionVote calls v2(operation, …), where operation selects the POST or DELETE vote route, so neither consumer is credited and migration-ledger verification can pass with both active operations missing. Resolve statically assigned conditional keys or otherwise record both branches, and add coverage for this call form.
![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: f1ffdd6ce3
ℹ️ 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".
| SeasonNumber *int `json:"season_number" nullable:"true" doc:"null unless an episode" example:"1"` | ||
| EpisodeNumber *int `json:"episode_number" nullable:"true" doc:"null unless an episode" example:"1"` | ||
| PosterURL string `json:"poster_url" doc:"Where to fetch the poster; empty when there is none" example:"/api/v1/images/poster/42"` | ||
| PosterURL string `json:"poster_url" doc:"Where to fetch the poster; empty when there is none" example:"https://media.example/poster.jpg"` |
There was a problem hiding this comment.
Changing this schema example leaves the committed contracts/api/v2/fixtures/list_household_sessions_ok.json response advertising /api/v1/images/poster/42 and /api/v1/images/poster/7, sourced from the unchanged fixtures in internal/apiv2/pilot_fakes_test.go. Contract consumers therefore receive contradictory examples and may continue treating URLs from the namespace that becomes a 410 Gone tombstone at 1.0 as valid v2 data; update the fixture source and regenerate the success fixture with this change.
AGENTS.md reference: AGENTS.md:L193-L199
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b482c165ff
ℹ️ 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".
| lit = first_string_literal(first) if first[:1] in ('"', "'", "`") else None | ||
| literals = [lit] if lit is not None else [] | ||
| if not literals: | ||
| var = re.match(r"(\w+)\s*(?:,|$)", first) |
There was a problem hiding this comment.
When v2 receives an inline conditional expression, this regex accepts only a variable followed immediately by a comma or the end of the arguments, so calls such as the poster/backdrop upload in web/src/api/adminCollections.ts and the user/admin collection operations in web/src/hooks/queries/collections.ts produce no records for either route. Regenerating the migration ledger can therefore omit real first-party consumers and misclassify routes while the v1 namespace is being retired; parse direct conditional operation expressions as well as assigned conditionals.
AGENTS.md reference: AGENTS.md:L193-L199
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b390016a9d
ℹ️ 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".
| method = web_method(args) | ||
| if fn == "v2": | ||
| method = lit.split(None, 1)[0] | ||
| record("web", WEB_BASE, fp, line, method, path, lit, [generic], via) |
There was a problem hiding this comment.
The extractor now records the migrated v2 calls, but the committed contracts/api/v2/migration.json was not rebuilt: it still attributes 606 web call sites to 550 legacy /api/v1 rows and records none on the v2 delegation rows. Freshly running extract_consumers.py, match_consumers.py, and build_ledger.py removes the obsolete mechanical v1 evidence and assigns the current calls to /api/v2/*; verify-migration-ledger does not detect this drift because its new Python check validates only one extractor result, not the committed ledger. Refresh the ledger or add a generated-artifact comparison so retirement decisions are not made from pre-migration consumer data.
AGENTS.md reference: AGENTS.md:L193-L199
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eab1c65a15
ℹ️ 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".
| "consumers": [ | ||
| "unused" | ||
| "web" | ||
| ], |
There was a problem hiding this comment.
The regenerated entry now records the web client as a consumer of the legacy Jellyfin Web update route, but its notes field still says that no first-party or internal consumer was found. Because this ledger is used to support the /api/v1 retirement, those opposing claims make it unclear whether the route is unused or represented by the shared v2 install operation; update the curated note, and preferably the generator's handling of unused-to-used transitions, together with this evidence.
AGENTS.md reference: AGENTS.md:L193-L199
Useful? React with 👍 / 👎.
1 participant
Problem
Related issue: #135
The API v2 migration still returned legacy asset URLs, the migration ledger extractor ignored the migrated web client, and the web bundle retained an unused v1 transport and stale v1 URL documentation.
Approach
The v2 branding upload adapter now constructs its response URL from the v2 route and content reference. The consumer extractor recognizes typed
v2("METHOD /api/v2/...")calls and preserves their method and path. The unused v1 transport wrappers and their tests were removed. The remaining v2 playback-session example now uses a neutral media URL. Generated OpenAPI output was refreshed.Validation
go test -count=2 ./internal/apiv2 ./internal/contractledger ./internal/routeinventorypassed before the final generated-artifact update; the focused post-update run for branding and artifact consistency passed twice.make verify-migration-ledgerpassed.git diff --checkpassed.web/node_modulesis absent; the generated TypeScript schema was updated to match the regenerated OpenAPI example.Risks
The web client no longer exposes the bridge-only v1 transport helpers. Operational health probes and server-side compatibility URL adapters remain unchanged.
Checklist
AI Disclosure