fix(streamnzb): add hook to report filtered streams back to streamnzb#771
Open
Gaisberg wants to merge 1 commit intoViren070:mainfrom
Open
fix(streamnzb): add hook to report filtered streams back to streamnzb#771Gaisberg wants to merge 1 commit intoViren070:mainfrom
Gaisberg wants to merge 1 commit intoViren070:mainfrom
Conversation
Contributor
WalkthroughAdds a post-processing hook Changes
Sequence DiagramsequenceDiagram
participant Main as Main Processing
participant PresetClass as Preset Class
participant StreamNZB as StreamNZBPreset
participant Reporter as Failover Reporter
Main->>PresetClass: compute/filter streams per preset
PresetClass-->>Main: return preset streams
Main->>Main: group final streams by preset type
loop for each preset type with streams
Main->>PresetClass: call onStreamsReady(streams)
alt Preset implements hook
PresetClass->>StreamNZB: onStreamsReady(streams)
StreamNZB->>StreamNZB: group streams by manifestUrl
StreamNZB->>StreamNZB: compute baseUrl per group
StreamNZB->>Reporter: reportFailoverOrder(streams, baseUrl)
Reporter->>Reporter: POST baseUrl/failover_order
end
end
Main->>Main: return final response
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/main.ts`:
- Around line 2312-2326: The loop that groups streams by preset and calls hooks
must not allow PresetManager.fromId or PresetClass.onStreamsReady to
throw/reject and crash processing; wrap the lookup and hook invocation in a
try/catch per presetType (i.e., around PresetManager.fromId(presetType) and the
call to PresetClass.onStreamsReady(list)), and invoke the hook in a
fire-and-forget manner (e.g., call and swallow/handle a returned promise with
.catch or await inside try/catch) so any synchronous throw or rejected promise
is logged and ignored without affecting stream response.
In `@packages/core/src/presets/streamnzb.ts`:
- Around line 100-121: The fallback IIFE in onStreamsReady can throw when
list[0].addon.manifestUrl is an empty string (new URL('') fails); update
onStreamsReady to defensively handle missing/empty manifestUrl by checking that
list[0].addon.manifestUrl is a non-empty string before constructing new URL, and
if it is empty use a safe fallback (e.g., derive baseUrl solely from
addon.preset.options?.url or skip/continue) so that reportFailoverOrder always
receives a valid baseUrl; reference the symbols ParsedStream, addon.manifestUrl,
addon.preset.options?.url, onStreamsReady and reportFailoverOrder when making
the change.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/main.tspackages/core/src/presets/preset.tspackages/core/src/presets/streamnzb.ts
Comment on lines
+2312
to
+2326
| const byPresetType = new Map<string, ParsedStream[]>(); | ||
| for (const s of finalStreams) { | ||
| const type = s.addon?.preset?.type ?? ''; | ||
| if (type) { | ||
| const list = byPresetType.get(type) ?? []; | ||
| list.push(s); | ||
| byPresetType.set(type, list); | ||
| } | ||
| } | ||
| for (const [presetType, list] of byPresetType) { | ||
| const PresetClass = PresetManager.fromId(presetType); | ||
| if (typeof PresetClass.onStreamsReady === 'function') { | ||
| PresetClass.onStreamsReady(list); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Missing error handling around hook invocation could crash stream processing.
The PresetManager.fromId(presetType) call (line 2322) throws if the preset type is unknown. Additionally, if onStreamsReady throws synchronously or returns a rejected promise, it will either crash processing or result in an unhandled promise rejection.
Given the hook is described as "fire-and-forget", errors should be caught to prevent impacting the user's stream response.
🛡️ Proposed fix with error handling
for (const [presetType, list] of byPresetType) {
- const PresetClass = PresetManager.fromId(presetType);
- if (typeof PresetClass.onStreamsReady === 'function') {
- PresetClass.onStreamsReady(list);
- }
+ try {
+ const PresetClass = PresetManager.fromId(presetType);
+ if (typeof PresetClass.onStreamsReady === 'function') {
+ Promise.resolve(PresetClass.onStreamsReady(list)).catch((err) => {
+ logger.debug(
+ `onStreamsReady hook failed for preset ${presetType}: ${err instanceof Error ? err.message : String(err)}`
+ );
+ });
+ }
+ } catch (err) {
+ logger.debug(
+ `Failed to invoke onStreamsReady for preset ${presetType}: ${err instanceof Error ? err.message : String(err)}`
+ );
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const byPresetType = new Map<string, ParsedStream[]>(); | |
| for (const s of finalStreams) { | |
| const type = s.addon?.preset?.type ?? ''; | |
| if (type) { | |
| const list = byPresetType.get(type) ?? []; | |
| list.push(s); | |
| byPresetType.set(type, list); | |
| } | |
| } | |
| for (const [presetType, list] of byPresetType) { | |
| const PresetClass = PresetManager.fromId(presetType); | |
| if (typeof PresetClass.onStreamsReady === 'function') { | |
| PresetClass.onStreamsReady(list); | |
| } | |
| } | |
| const byPresetType = new Map<string, ParsedStream[]>(); | |
| for (const s of finalStreams) { | |
| const type = s.addon?.preset?.type ?? ''; | |
| if (type) { | |
| const list = byPresetType.get(type) ?? []; | |
| list.push(s); | |
| byPresetType.set(type, list); | |
| } | |
| } | |
| for (const [presetType, list] of byPresetType) { | |
| try { | |
| const PresetClass = PresetManager.fromId(presetType); | |
| if (typeof PresetClass.onStreamsReady === 'function') { | |
| Promise.resolve(PresetClass.onStreamsReady(list)).catch((err) => { | |
| logger.debug( | |
| `onStreamsReady hook failed for preset ${presetType}: ${err instanceof Error ? err.message : String(err)}` | |
| ); | |
| }); | |
| } | |
| } catch (err) { | |
| logger.debug( | |
| `Failed to invoke onStreamsReady for preset ${presetType}: ${err instanceof Error ? err.message : String(err)}` | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/main.ts` around lines 2312 - 2326, The loop that groups
streams by preset and calls hooks must not allow PresetManager.fromId or
PresetClass.onStreamsReady to throw/reject and crash processing; wrap the lookup
and hook invocation in a try/catch per presetType (i.e., around
PresetManager.fromId(presetType) and the call to
PresetClass.onStreamsReady(list)), and invoke the hook in a fire-and-forget
manner (e.g., call and swallow/handle a returned promise with .catch or await
inside try/catch) so any synchronous throw or rejected promise is logged and
ignored without affecting stream response.
Comment on lines
+100
to
121
| static override onStreamsReady(streams: ParsedStream[]): void { | ||
| if (streams.length === 0) return; | ||
| const byManifest = new Map<string, ParsedStream[]>(); | ||
| for (const s of streams) { | ||
| const key = s.addon.manifestUrl ?? ''; | ||
| const list = byManifest.get(key) ?? []; | ||
| list.push(s); | ||
| byManifest.set(key, list); | ||
| } | ||
| const configQuery = this.buildConfigQuery(userData); | ||
| if (configQuery) { | ||
| url.search += (url.search ? '&' : '') + configQuery; | ||
| for (const [, list] of byManifest) { | ||
| const baseUrl = | ||
| (list[0].addon.preset.options?.url as string) | ||
| ?.replace(/\/manifest\.json.*$/i, '') | ||
| ?.replace(/\/+$/, '') ?? | ||
| (() => { | ||
| const u = new URL(list[0].addon.manifestUrl ?? ''); | ||
| u.pathname = u.pathname.replace(/\/manifest\.json$/i, '') || '/'; | ||
| return u.toString().replace(/\/+$/, ''); | ||
| })(); | ||
| this.reportFailoverOrder(list, baseUrl); | ||
| } | ||
| return url.toString(); | ||
| } |
Contributor
There was a problem hiding this comment.
Potential runtime error when manifestUrl is empty.
The fallback IIFE at lines 114-118 will throw a TypeError if manifestUrl is an empty string, as new URL('') is invalid. Although options.url is marked as required in the metadata, defensive coding would prevent unexpected crashes if validation is bypassed or misconfigured.
🛡️ Proposed defensive fix
const baseUrl =
(list[0].addon.preset.options?.url as string)
?.replace(/\/manifest\.json.*$/i, '')
?.replace(/\/+$/, '') ??
(() => {
+ const manifestUrl = list[0].addon.manifestUrl;
+ if (!manifestUrl) return '';
- const u = new URL(list[0].addon.manifestUrl ?? '');
+ const u = new URL(manifestUrl);
u.pathname = u.pathname.replace(/\/manifest\.json$/i, '') || '/';
return u.toString().replace(/\/+$/, '');
})();
+ if (!baseUrl) return;
this.reportFailoverOrder(list, baseUrl);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/presets/streamnzb.ts` around lines 100 - 121, The fallback
IIFE in onStreamsReady can throw when list[0].addon.manifestUrl is an empty
string (new URL('') fails); update onStreamsReady to defensively handle
missing/empty manifestUrl by checking that list[0].addon.manifestUrl is a
non-empty string before constructing new URL, and if it is empty use a safe
fallback (e.g., derive baseUrl solely from addon.preset.options?.url or
skip/continue) so that reportFailoverOrder always receives a valid baseUrl;
reference the symbols ParsedStream, addon.manifestUrl,
addon.preset.options?.url, onStreamsReady and reportFailoverOrder when making
the change.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/core/src/presets/streamnzb.ts (1)
111-119:⚠️ Potential issue | 🟠 MajorGuard empty URL inputs before constructing
new URL(...).Line 111 can yield
''(so fallback is skipped), and Line 115 can throw onnew URL(''). That can interruptonStreamsReadyand prevent failover reporting for remaining groups.Suggested patch
- const baseUrl = - (list[0].addon.preset.options?.url as string) - ?.replace(/\/manifest\.json.*$/i, '') - ?.replace(/\/+$/, '') ?? - (() => { - const u = new URL(list[0].addon.manifestUrl ?? ''); - u.pathname = u.pathname.replace(/\/manifest\.json$/i, '') || '/'; - return u.toString().replace(/\/+$/, ''); - })(); - this.reportFailoverOrder(list, baseUrl); + const optionUrl = (list[0].addon.preset.options?.url as string)?.trim(); + let baseUrl = + optionUrl?.replace(/\/manifest\.json.*$/i, '')?.replace(/\/+$/, '') ?? + ''; + + if (!baseUrl) { + const manifestUrl = list[0].addon.manifestUrl?.trim(); + if (!manifestUrl) continue; + const u = new URL(manifestUrl); + u.pathname = u.pathname.replace(/\/manifest\.json$/i, '') || '/'; + baseUrl = u.toString().replace(/\/+$/, ''); + } + + if (!baseUrl) continue; + this.reportFailoverOrder(list, baseUrl);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/presets/streamnzb.ts` around lines 111 - 119, The code builds baseUrl from (list[0].addon.preset.options?.url as string) or by calling new URL(list[0].addon.manifestUrl), but it doesn't guard against empty strings which can cause new URL('') to throw and abort onStreamsReady; update the baseUrl construction in the block that computes baseUrl (the expression before the this.reportFailoverOrder call) to first check that the preset.options?.url and addon.manifestUrl are non-empty (non-falsy, non-blank) before using them, and if both are empty fall back to a safe default (e.g. '/' or undefined) so new URL(...) is never called with an empty string; ensure baseUrl is always defined (or handled) before calling this.reportFailoverOrder(list, baseUrl).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/core/src/presets/streamnzb.ts`:
- Around line 111-119: The code builds baseUrl from
(list[0].addon.preset.options?.url as string) or by calling new
URL(list[0].addon.manifestUrl), but it doesn't guard against empty strings which
can cause new URL('') to throw and abort onStreamsReady; update the baseUrl
construction in the block that computes baseUrl (the expression before the
this.reportFailoverOrder call) to first check that the preset.options?.url and
addon.manifestUrl are non-empty (non-falsy, non-blank) before using them, and if
both are empty fall back to a safe default (e.g. '/' or undefined) so new
URL(...) is never called with an empty string; ensure baseUrl is always defined
(or handled) before calling this.reportFailoverOrder(list, baseUrl).
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/main.tspackages/core/src/presets/preset.tspackages/core/src/presets/streamnzb.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/main.ts
- packages/core/src/presets/preset.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/core/src/presets/streamnzb.ts (1)
119-128:⚠️ Potential issue | 🟠 MajorHarden base URL derivation to avoid throws and malformed failover endpoints.
Line 124 can still throw on empty/invalid
manifestUrl, and the fallback currently keeps query/hash parts, which can produce an incorrect/failover_orderURL after concatenation.#!/bin/bash set -euo pipefail # 1) Confirm URL parsing path and current guards in StreamNZB preset. rg -n -C4 "new URL\\(|manifestUrl \\?\\?|reportFailoverOrder\\(" packages/core/src/presets/streamnzb.ts # 2) Check how onStreamsReady is invoked and whether errors are isolated upstream. rg -n -C6 "onStreamsReady\\(|PresetClass\\.onStreamsReady|try\\s*\\{|catch\\s*\\(" \ packages/core/src/main.ts packages/core/src/presets/preset.ts🔧 Proposed fix
- const baseUrl = - (list[0].addon.preset.options?.url as string) - ?.replace(/\/manifest\.json.*$/i, '') - ?.replace(/\/+$/, '') ?? - (() => { - const u = new URL(list[0].addon.manifestUrl ?? ''); - u.pathname = u.pathname.replace(/\/manifest\.json$/i, '') || '/'; - return u.toString().replace(/\/+$/, ''); - })(); - this.reportFailoverOrder(list, baseUrl); + const optionUrl = ((list[0].addon.preset.options?.url as string) ?? '').trim(); + let baseUrl = optionUrl + .replace(/\/manifest\.json.*$/i, '') + .replace(/\/+$/, ''); + + if (!baseUrl) { + const manifestUrl = (list[0].addon.manifestUrl ?? '').trim(); + if (!manifestUrl) continue; + try { + const u = new URL(manifestUrl); + u.pathname = u.pathname.replace(/\/manifest\.json$/i, '') || '/'; + u.search = ''; + u.hash = ''; + baseUrl = u.toString().replace(/\/+$/, ''); + } catch { + continue; + } + } + + if (!baseUrl) continue; + this.reportFailoverOrder(list, baseUrl);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/presets/streamnzb.ts` around lines 119 - 128, The baseUrl derivation can throw or retain query/hash parts; update the logic around list[0].addon.preset.options?.url and the fallback that uses new URL(list[0].addon.manifestUrl) so it safely handles empty/invalid manifestUrl: validate that manifestUrl is a non-empty string before calling new URL (wrap the URL construction in a try/catch or conditional), strip search and hash components (use only origin + pathname), normalize pathname by removing any trailing /manifest.json and trimming trailing slashes, and fall back to a safe default (e.g., origin or '/' ) if parsing fails; keep the final value used by reportFailoverOrder(list, baseUrl) free of query/hash and without trailing slashes.
🧹 Nitpick comments (2)
packages/core/src/presets/streamnzb.ts (2)
138-142: Skip payload entries withoutfailoverIdbefore POSTing failover order.Filtering missing IDs and bailing out on an empty filtered list avoids avoidable noisy requests.
♻️ Proposed refactor
- const body = { - streams: streams.map((s) => ({ - failoverId: (s as { failoverId?: string }).failoverId, - })), - }; + const orderedIds = streams + .map((s) => (s as { failoverId?: string }).failoverId) + .filter((id): id is string => typeof id === 'string' && id.length > 0); + + if (orderedIds.length === 0) return; + + const body = { + streams: orderedIds.map((failoverId) => ({ failoverId })), + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/presets/streamnzb.ts` around lines 138 - 142, Filter out stream entries that lack a failoverId before building and POSTing the failover payload: transform streams into a filtered list (e.g., const valid = streams.filter(s => (s as {failoverId?: string}).failoverId)) and map only those to the body (const body = { streams: valid.map(s => ({ failoverId: (s as {failoverId?: string}).failoverId })) }); if valid.length === 0, skip the POST/return early to avoid sending an empty or noisy request. Ensure you update any subsequent code that references body to use the filtered list.
91-91: Deduplicate the'AIOStreams'literal into one constant.This prevents drift between metadata and request headers.
🧹 Proposed tidy-up
const logger = createLogger('streamnzb'); +const STREAMNZB_USER_AGENT = 'AIOStreams'; const FAILOVER_ORDER_PATH = '/failover_order'; @@ - USER_AGENT: 'AIOStreams', + USER_AGENT: STREAMNZB_USER_AGENT, @@ - 'User-Agent': 'AIOStreams', + 'User-Agent': STREAMNZB_USER_AGENT, @@ - 'User-Agent': 'AIOStreams', + 'User-Agent': STREAMNZB_USER_AGENT,Also applies to: 149-150, 177-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/presets/streamnzb.ts` at line 91, Declare a single file-scoped constant (e.g., const AIO_USER_AGENT = 'AIOStreams') at the top of packages/core/src/presets/streamnzb.ts and replace every hard-coded 'AIOStreams' literal with that constant (the places shown include the USER_AGENT property and the other occurrences around lines 149-150 and 177). Update any header/metadata assignments that currently use the string literal so they reference AIO_USER_AGENT instead to ensure a single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/core/src/presets/streamnzb.ts`:
- Around line 119-128: The baseUrl derivation can throw or retain query/hash
parts; update the logic around list[0].addon.preset.options?.url and the
fallback that uses new URL(list[0].addon.manifestUrl) so it safely handles
empty/invalid manifestUrl: validate that manifestUrl is a non-empty string
before calling new URL (wrap the URL construction in a try/catch or
conditional), strip search and hash components (use only origin + pathname),
normalize pathname by removing any trailing /manifest.json and trimming trailing
slashes, and fall back to a safe default (e.g., origin or '/' ) if parsing
fails; keep the final value used by reportFailoverOrder(list, baseUrl) free of
query/hash and without trailing slashes.
---
Nitpick comments:
In `@packages/core/src/presets/streamnzb.ts`:
- Around line 138-142: Filter out stream entries that lack a failoverId before
building and POSTing the failover payload: transform streams into a filtered
list (e.g., const valid = streams.filter(s => (s as {failoverId?:
string}).failoverId)) and map only those to the body (const body = { streams:
valid.map(s => ({ failoverId: (s as {failoverId?: string}).failoverId })) }); if
valid.length === 0, skip the POST/return early to avoid sending an empty or
noisy request. Ensure you update any subsequent code that references body to use
the filtered list.
- Line 91: Declare a single file-scoped constant (e.g., const AIO_USER_AGENT =
'AIOStreams') at the top of packages/core/src/presets/streamnzb.ts and replace
every hard-coded 'AIOStreams' literal with that constant (the places shown
include the USER_AGENT property and the other occurrences around lines 149-150
and 177). Update any header/metadata assignments that currently use the string
literal so they reference AIO_USER_AGENT instead to ensure a single source of
truth.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/main.tspackages/core/src/presets/preset.tspackages/core/src/presets/streamnzb.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/presets/preset.ts
- packages/core/src/main.ts
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.
Summary by CodeRabbit
New Features
Refactor