Add crowdsourced health check support for Newznab indexers #552
Conversation
WalkthroughAdded persistent Changes
Sequence DiagramsequenceDiagram
actor User
participant Preset as NewznabPreset
participant Addon as NewznabAddon
participant API as NewznabApi (BaseNabApi)
participant HealthProxy as Zyclops
User->>Preset: Enable/configure zyclopsHealthProxy
Preset->>Addon: Generate addon config (with zyclopsHealthProxy fields)
Addon->>Addon: buildZyclopsHealthProxyConfig() (validate identifiers, resolve endpoint/path/params)
Addon->>API: Instantiate NewznabApi(..., apiPath?, params)
User->>API: Perform search/request
API->>API: Merge per-call params + persistent params into URLSearchParams and cache key
API->>HealthProxy: HTTP request (with appended routing params)
HealthProxy-->>API: Response (includes health metadata)
API-->>User: Return results with zyclopsHealth attached where present
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/core/src/utils/env.ts (1)
348-351: Consider usingurl()validator for URL validation consistency.The
HEALTH_PROXY_ENDPOINTis a URL but usesstr()validator. Other URL environment variables in this file (e.g.,BASE_URL,INTERNAL_URL,COMET_URL) use theurl()validator which provides URL format validation at startup.- HEALTH_PROXY_ENDPOINT: str({ + HEALTH_PROXY_ENDPOINT: url({ default: 'https://zyclops.elfhosted.com', desc: 'Base URL of the Zyclops health proxy endpoint used by the Newznab preset.', }),packages/core/src/presets/newznab.ts (2)
33-34: Health status message could be more informative.The parsed message only includes the emoji itself (
NZB Health: 🧝), which may not clearly convey meaning to users. Consider extracting or mapping to a more descriptive status label.- const status = stream.description?.match(/🧝/u)?.[0]; - return status ? `NZB Health: ${status}` : undefined; + const hasHealthCheck = stream.description?.includes('🧝'); + return hasHealthCheck ? 'NZB Health: Checked ✓' : undefined;
448-451: Redundant fallback chain inhealthProxyTargetassignment.
resolvedHealthProxyTarget(Line 428-434) already falls back tosanitizedNewznabUrlwhen empty. The additional fallbacks|| options.healthProxyTarget || options.newznabUrlon lines 449-451 appear redundant sinceresolvedHealthProxyTargetwould already contain a value in those cases.healthProxyTarget: - resolvedHealthProxyTarget || - options.healthProxyTarget || - options.newznabUrl, + resolvedHealthProxyTarget,packages/core/src/builtins/newznab/addon.ts (2)
16-18: Consider extracting shared constants to avoid duplication.These constants are duplicated in
packages/core/src/presets/newznab.ts(lines 17-19). Consider extracting them to a shared location to maintain consistency.
157-161: Redundant condition in validation check.The check
userProviderHosts.length > 0is redundant sincehasProviderHostalready implies this condition (Line 155:hasProviderHost = providerHosts.length > 0, andproviderHostsis only populated whenuserProviderHosts.length > 0).- if (hasBackbone && hasProviderHost && userProviderHosts.length > 0) { + if (hasBackbone && hasProviderHost) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/core/src/builtins/base/nab/api.ts(5 hunks)packages/core/src/builtins/newznab/addon.ts(3 hunks)packages/core/src/presets/newznab.ts(6 hunks)packages/core/src/utils/env.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (6)
packages/core/src/presets/newznab.ts (2)
298-322: Validation logic is appropriately placed for early error detection.The validation correctly enforces mutual exclusivity between backbone and provider host selection before addon generation. This complements the validation in
addon.tsand catches configuration errors at the earliest possible stage.
219-275: Well-structured health proxy options with appropriate user warnings.The options are clearly organised with a section header, appropriate intents, and hidden from simple mode. The warning on
healthProxyEnabledappropriately alerts users to the privacy and ToS implications.packages/core/src/builtins/base/nab/api.ts (2)
228-243: Well-implemented extraParams support with proper type conversion.The implementation correctly:
- Converts all parameter values to strings for URL compatibility
- Stores the converted values for consistent usage
- Accepts flexible input types (string | number | boolean)
398-402: Correct parameter merging logic.ExtraParams are correctly merged without overwriting explicitly-provided parameters, ensuring that user-specified values take precedence.
packages/core/src/builtins/newznab/addon.ts (2)
31-48: Schema and interface definitions are well-structured.The schema correctly validates health proxy configuration fields with appropriate optional markers and string constraints.
84-190: Health proxy configuration builder is comprehensive.The method correctly handles endpoint resolution, parameter building, validation, and logging. The defensive check at lines 94-98 is unreachable given the default constant, but provides a safety net if the default ever changes.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/presets/newznab.ts (1)
408-462: Consider decoupling Newznab API path default from the Zyclops path and simplifying target resolutionThe health proxy resolution block is generally solid (trimming values and providing sane fallbacks), but there are a couple of maintainability nits:
normalizedApiPathfalls back toDEFAULT_ZYCLOPS_HEALTH_PROXY_PATH, which conceptually belongs to the Zyclops endpoint, not the upstream Newznab API. If the Zyclops path ever changes, the implicit Newznab default will change with it.resolvedHealthProxyTargetand the laterhealthProxyTargetassignment double‑fallback throughoptions.healthProxyTarget, and can still propagate an untrimmed empty string.You could decouple these concerns and make the target resolution a bit clearer, for example:
- const normalizedApiPath = sanitizedApiPathNoTrailing - ? sanitizedApiPathNoTrailing.startsWith('/') - ? sanitizedApiPathNoTrailing - : `/${sanitizedApiPathNoTrailing}` - : DEFAULT_ZYCLOPS_HEALTH_PROXY_PATH; + const normalizedApiPath = sanitizedApiPathNoTrailing + ? sanitizedApiPathNoTrailing.startsWith('/') + ? sanitizedApiPathNoTrailing + : `/${sanitizedApiPathNoTrailing}` + : '/api'; // default Newznab API path - const resolvedHealthProxyTarget = - (typeof options.healthProxyTarget === 'string' - ? options.healthProxyTarget.trim() - : '') || - (sanitizedNewznabUrl - ? `${sanitizedNewznabUrl}${normalizedApiPath}` - : undefined); + const resolvedHealthProxyTarget = + typeof options.healthProxyTarget === 'string' && + options.healthProxyTarget.trim().length > 0 + ? options.healthProxyTarget.trim() + : sanitizedNewznabUrl + ? `${sanitizedNewznabUrl}${normalizedApiPath}` + : undefined; @@ - healthProxyTarget: - resolvedHealthProxyTarget || - options.healthProxyTarget || - options.newznabUrl, + healthProxyTarget: + resolvedHealthProxyTarget ?? options.newznabUrl,This keeps Newznab defaults independent of Zyclops internals and avoids ever emitting a blank
healthProxyTargetwhile keeping the current behaviour for typical configurations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/presets/newznab.ts(6 hunks)
🔇 Additional comments (4)
packages/core/src/presets/newznab.ts (4)
1-20: Zyclops constants and environment defaults look reasonableImporting
ParsedStreamandBuiltinStreamParserand introducingZYCLOPS_BACKBONE_OPTIONSplus env‑backed defaults for the endpoint/path is clear and keeps the feature off by default while being easy to turn on. No functional issues spotted here.
21-36: Parser gating onhealthProxyEnabledis soundConditioning the extra message on
this.addon?.preset?.options?.healthProxyEnabledavoids leaking Zyclops‑specific noise when the feature is off, and the implementation ofgetMessageis straightforward. If Zyclops later introduces more than the🧝marker, you can extend the regex, but the current logic is fine.
219-275: Health proxy option metadata is well structuredThe Zyclops section, enable toggle, backbone/provider host, and advanced flags are clearly described, with appropriate defaults and intent (including the warning copy about sending the indexer URL/API key). The UI copy matches the intended behaviour described in the PR.
350-363: Typed multi‑service addon generation is clearUsing
NonNullable<UserData['services']>[number]and mapping overservice.idfor both multiple‑instance and multi‑service modes keeps the typing honest and the control flow easy to follow. No issues here.
packages/core/src/presets/newznab.ts
Outdated
| if (options.healthProxyEnabled) { | ||
| const backbonesSelected = Array.isArray(options.healthProxyBackbone) | ||
| ? options.healthProxyBackbone.filter((value: string) => value?.trim()) | ||
| .length > 0 | ||
| : false; | ||
| const providerHostSpecified = | ||
| typeof options.healthProxyProviderHost === 'string' | ||
| ? options.healthProxyProviderHost | ||
| .split(',') | ||
| .map((value: string) => value.trim()) | ||
| .filter((value: string) => value.length > 0).length > 0 | ||
| : false; | ||
|
|
||
| if (backbonesSelected && providerHostSpecified) { | ||
| throw new Error( | ||
| 'Zyclops health checks accept only one identifier. Choose either Backbones or Provider Host, not both.' | ||
| ); | ||
| } | ||
|
|
||
| if (!backbonesSelected && !providerHostSpecified) { | ||
| throw new Error( | ||
| 'Zyclops health checks require either a Backbone selection or a Provider Host when enabled.' | ||
| ); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Tighten health proxy validation and make backbone detection more defensive
The backbone vs provider‑host validation is a good safeguard, but there are two small gaps:
- The description says
healthProxyShowUnknownis incompatible withhealthProxySingleIp, but this is not enforced. filter((value: string) => value?.trim())will throw if a non‑string slips intohealthProxyBackbone.
You can address both with a slightly more defensive check and an extra validation branch:
- if (options.healthProxyEnabled) {
- const backbonesSelected = Array.isArray(options.healthProxyBackbone)
- ? options.healthProxyBackbone.filter((value: string) => value?.trim())
- .length > 0
- : false;
+ if (options.healthProxyEnabled) {
+ const backbonesSelected =
+ Array.isArray(options.healthProxyBackbone) &&
+ options.healthProxyBackbone.some(
+ (value) => typeof value === 'string' && value.trim().length > 0
+ );
const providerHostSpecified =
typeof options.healthProxyProviderHost === 'string'
? options.healthProxyProviderHost
.split(',')
.map((value: string) => value.trim())
.filter((value: string) => value.length > 0).length > 0
: false;
if (backbonesSelected && providerHostSpecified) {
throw new Error(
'Zyclops health checks accept only one identifier. Choose either Backbones or Provider Host, not both.'
);
}
if (!backbonesSelected && !providerHostSpecified) {
throw new Error(
'Zyclops health checks require either a Backbone selection or a Provider Host when enabled.'
);
}
+
+ if (options.healthProxyShowUnknown && options.healthProxySingleIp) {
+ throw new Error(
+ 'Zyclops health checks: "Show Unknown Releases" is incompatible with Single-IP mode; please disable one of them.'
+ );
+ }
}This keeps runtime behaviour aligned with the option descriptions and makes the validation more robust against unexpected input.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/core/src/presets/newznab.ts (2)
316-333: Consider enforcing the documented incompatibility betweenhealthProxyShowUnknownandhealthProxySingleIp.The description for
healthProxyShowUnknownstates it is "Incompatible with single IP mode", but the validation logic ingenerateAddons()(lines 356-380) does not enforce this constraint. Users could enable both options simultaneously, leading to unexpected behaviour.This was also noted in a past review comment. Consider adding validation:
if (!backbonesSelected && !providerHostSpecified) { throw new Error( 'Zyclops health checks require either a Backbone selection or a Provider Host when enabled.' ); } + + if (options.healthProxyShowUnknown && options.healthProxySingleIp) { + throw new Error( + 'Zyclops health checks: "Show Unknown Releases" is incompatible with Single-IP mode; please disable one of them.' + ); + } }
356-380: Consider a more defensive backbone selection check.The current check
options.healthProxyBackbone.filter((value: string) => value?.trim())assumes all array elements are strings. A slightly more defensive approach would handle unexpected types:- const backbonesSelected = Array.isArray(options.healthProxyBackbone) - ? options.healthProxyBackbone.filter((value: string) => value?.trim()) - .length > 0 - : false; + const backbonesSelected = + Array.isArray(options.healthProxyBackbone) && + options.healthProxyBackbone.some( + (value) => typeof value === 'string' && value.trim().length > 0 + );This aligns with the past review suggestion and adds resilience against unexpected input types.
🧹 Nitpick comments (3)
packages/core/src/presets/newznab.ts (1)
506-520: Simplify redundant fallback chain forhealthProxyTarget.The fallback logic on lines 511-514 appears redundant:
healthProxyTarget: resolvedHealthProxyTarget || options.healthProxyTarget || options.newznabUrl,Since
resolvedHealthProxyTargetis already computed fromoptions.healthProxyTarget(trimmed) with a fallback tofallbackTarget(which derives fromnewznabUrl), the subsequent|| options.healthProxyTarget || options.newznabUrlchecks will never be reached. Consider simplifying:🔎 Suggested simplification
Object.assign(config, { healthProxyEnabled: true, healthProxyEndpoint: resolvedHealthProxyEndpoint, healthProxyPath: resolvedHealthProxyPath, - healthProxyTarget: - resolvedHealthProxyTarget || - options.healthProxyTarget || - options.newznabUrl, + healthProxyTarget: resolvedHealthProxyTarget, healthProxyBackbone: options.healthProxyBackbone, healthProxyProviderHost: options.healthProxyProviderHost, healthProxyShowUnknown: options.healthProxyShowUnknown, healthProxySingleIp: options.healthProxySingleIp, });packages/core/src/builtins/newznab/addon.ts (2)
23-36: Consider extracting shared health proxy constants.The
DEFAULT_HEALTH_PROXY_ENDPOINTandDEFAULT_HEALTH_PROXY_PATHconstants are duplicated between this file andpackages/core/src/presets/newznab.ts. Consider extracting these to a shared constants file to maintain consistency and reduce duplication.
164-168: Simplify redundant condition in validation.The condition on line 164 is unnecessarily complex:
if (hasBackbone && hasProviderHost && userProviderHosts.length > 0)Since
hasProviderHostis derived fromproviderHosts.length > 0, andproviderHostsis set fromuserProviderHostswhenuserProviderHosts.length > 0, the final check is redundant.🔎 Suggested simplification
- if (hasBackbone && hasProviderHost && userProviderHosts.length > 0) { + if (hasBackbone && hasProviderHost) { throw new Error( 'Crowdsourced health checks only accept one identifier. Choose either a backbone selection or a provider host.' ); }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/builtins/newznab/addon.ts(8 hunks)packages/core/src/debrid/utils.ts(1 hunks)packages/core/src/presets/newznab.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/core/src/presets/newznab.ts (2)
packages/core/src/presets/builtin.ts (1)
BuiltinStreamParser(8-81)packages/core/src/db/schemas.ts (3)
Stream(674-674)ParsedStream(778-778)UserData(503-503)
packages/core/src/builtins/newznab/addon.ts (3)
packages/core/src/debrid/utils.ts (3)
NZB(64-69)TorrentWithSelectedFile(71-78)NZBWithSelectedFile(80-87)packages/core/src/utils/constants.ts (1)
BuiltinServiceId(244-244)packages/core/src/db/schemas.ts (1)
Stream(674-674)
🔇 Additional comments (9)
packages/core/src/debrid/utils.ts (1)
64-69: LGTM!The addition of the optional
zyclopsHealthproperty to theNZBinterface is a clean, non-breaking extension. Thestringtype is appropriate for carrying health status metadata from the Zyclops proxy, and the optional nature ensures backward compatibility with existing code.packages/core/src/presets/newznab.ts (4)
6-19: LGTM!The backbone options are well-structured with
as constfor type safety, and the default endpoint/path constants appropriately use environment variables with sensible fallbacks.
21-44: Type assertion for extended stream property is acceptable but could be documented.The cast
(stream as { zyclopsHealth?: string }).zyclopsHealthis necessary because theStreamtype doesn't include this custom property. This is a reasonable approach given the property is dynamically added in_createStream. Consider adding a brief inline comment explaining why this assertion is needed.
46-93: LGTM!The emoji mapping covers a comprehensive range of health status keywords and degrades gracefully by returning
undefinedfor unrecognised values, allowing the raw health string to be displayed as a fallback.
408-421: LGTM!The
NonNullable<UserData['services']>[number]typing ensures type safety when mapping over services, preventing potential null/undefined access issues.packages/core/src/builtins/newznab/addon.ts (4)
38-55: LGTM!The schema extensions with Zod validation and the
HealthProxyConfiginterface are well-structured. Thez.string().min(1)constraint on backbone array elements ensures only non-empty strings are accepted.
82-89: LGTM!The constructor correctly builds the health proxy configuration and conditionally applies it to the API instantiation with appropriate fallbacks to direct user data values when health proxy is not enabled.
213-239: LGTM!The health data extraction and conditional assignment is well-implemented. Using optional chaining for
result.newznab?.zyclopsHealth?.toString()safely handles missing values, and conditionally adding the property only when present keeps the NZB objects clean.
279-300: LGTM!The
_createStreamoverride correctly extends the parent behaviour by callingsuper._createStreamfirst, then conditionally propagating thezyclopsHealthproperty to the stream. The type guard checks are thorough, and the cast toRecord<string, unknown>is an appropriate solution for adding a property not defined in theStreamtype.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/core/src/presets/newznab.ts (1)
355-379: Address the validation concerns from the previous review.The validation logic still lacks the check for incompatibility between
healthProxyShowUnknownandhealthProxySingleIpas noted in the previous review. Additionally, the backbone filtering should be more defensive to handle non-string values.Please refer to the previous review comment which provides the complete fix for both issues.
packages/core/src/builtins/newznab/addon.ts (1)
100-106: Remove unreachable dead code.The condition
if (!endpoint)at line 101 can never be true becauseendpointis assigned with a fallback toDEFAULT_HEALTH_PROXY_ENDPOINTat line 100, which always has a value. This warning block is unreachable.🔎 Suggested fix
const endpoint = endpointInput || DEFAULT_HEALTH_PROXY_ENDPOINT; - if (!endpoint) { - this.logger.warn( - 'Crowdsourced health checks are enabled for Newznab but no proxy endpoint was provided.' - ); - return undefined; - }
🧹 Nitpick comments (2)
packages/core/src/presets/newznab.ts (1)
465-492: Consider extracting URL normalization logic for readability.The URL and path normalization logic (lines 474-492) involves multiple fallbacks and string manipulations. Whilst correct, extracting this into a helper function would improve readability and make the manifest URL generation easier to follow.
Example refactor
private static normalizeHealthProxyConfig(options: Record<string, any>) { const resolvedEndpoint = options.healthProxyEndpoint?.trim() || DEFAULT_ZYCLOPS_HEALTH_PROXY_ENDPOINT; const resolvedPath = options.healthProxyPath?.trim() || DEFAULT_ZYCLOPS_HEALTH_PROXY_PATH; const sanitizedUrl = options.newznabUrl?.trim().replace(/\/+$/, '') || ''; const normalizedApiPath = this.resolveApiPath(options.apiPath); const fallbackTarget = sanitizedUrl ? `${sanitizedUrl}${normalizedApiPath}` : undefined; const resolvedTarget = options.healthProxyTarget?.trim() || fallbackTarget; return { resolvedEndpoint, resolvedPath, resolvedTarget }; } private static resolveApiPath(apiPath?: string): string { const raw = apiPath?.trim() || ''; const withoutTrailing = raw.replace(/\/+$/, ''); return withoutTrailing ? withoutTrailing.startsWith('/') ? withoutTrailing : `/${withoutTrailing}` : DEFAULT_ZYCLOPS_HEALTH_PROXY_PATH; }packages/core/src/builtins/newznab/addon.ts (1)
213-213: Add defensive type checking before callingtoString().The code calls
toString()onresult.newznab?.zyclopsHealthwithout validating its type. Whilst the optional chaining prevents errors ifzyclopsHealthis undefined, if it's an unexpected type (e.g., object, array),toString()could produce unexpected results like[object Object].🔎 Suggested fix
- const zyclopsHealth = result.newznab?.zyclopsHealth?.toString(); + const zyclopsHealth = + typeof result.newznab?.zyclopsHealth === 'string' || + typeof result.newznab?.zyclopsHealth === 'number' + ? result.newznab.zyclopsHealth.toString() + : undefined;
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/core/src/builtins/newznab/addon.ts(8 hunks)packages/core/src/debrid/utils.ts(1 hunks)packages/core/src/presets/newznab.ts(6 hunks)packages/core/src/utils/env.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/debrid/utils.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/core/src/presets/newznab.ts (1)
packages/core/src/presets/builtin.ts (2)
BuiltinStreamParser(8-83)BuiltinAddonPreset(85-154)
packages/core/src/builtins/newznab/addon.ts (4)
packages/core/src/builtins/base/nab/api.ts (1)
BaseNabApi(222-459)packages/core/src/debrid/utils.ts (2)
NZB(65-71)NZBWithSelectedFile(82-89)packages/core/src/utils/constants.ts (1)
BuiltinServiceId(252-252)packages/core/src/db/schemas.ts (1)
Stream(674-674)
🔇 Additional comments (5)
packages/core/src/presets/newznab.ts (3)
6-19: LGTM!The health proxy constants are well-defined with appropriate fallbacks.
21-94: LGTM!The
NewznabStreamParserprovides clear health status messaging with appropriate emoji mapping. The implementation correctly checks if health checks are enabled before processing health data.
407-420: LGTM!The type annotations with
NonNullableimprove type safety, and the logic correctly handles both single and multiple instance scenarios.packages/core/src/builtins/newznab/addon.ts (2)
28-35: LGTM!The constructor correctly propagates
extraParamsto the base class for health proxy routing.
288-309: LGTM!The override correctly propagates
zyclopsHealthfrom NZB results to the stream object with appropriate type guards.
Signed-off-by: David Young <davidy@funkypenguin.co.nz>
Signed-off-by: David Young <davidy@funkypenguin.co.nz>
Signed-off-by: David Young <davidy@funkypenguin.co.nz>
Signed-off-by: David Young <davidy@funkypenguin.co.nz>
Signed-off-by: David Young <davidy@funkypenguin.co.nz>
Signed-off-by: David Young <davidy@funkypenguin.co.nz>
Signed-off-by: David Young <davidy@funkypenguin.co.nz>
Signed-off-by: David Young <davidy@funkypenguin.co.nz>
Signed-off-by: David Young <davidy@funkypenguin.co.nz>
Signed-off-by: David Young <davidy@funkypenguin.co.nz>
Signed-off-by: David Young <davidy@funkypenguin.co.nz>
Signed-off-by: David Young <davidy@funkypenguin.co.nz>
…attrs Signed-off-by: David Young <davidy@funkypenguin.co.nz>
Viren070
force-pushed
the
add-zyclops-support
branch
from
eeb61cf to
808203e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/core/src/presets/newznab.ts (1)
209-274: Enforce the documented incompatibility between Show Unknown Releases and Single-IP Mode.The description at line 259 states that "Show Unknown Releases" is "Incompatible with single IP mode", but this constraint is not validated in the configuration generation logic. Users could enable both options and encounter unexpected behaviour.
As per the past review comment, add this validation in
generateManifestUrlalongside the existing backbone/provider host checks:🔎 Proposed validation
if (!backbonesSelected && providerHosts.length === 0) { throw new Error( `${this.METADATA.NAME}: Zyclops health checks require either a Backbone selection or a Provider Host when enabled.` ); } + + if (options.zyclopsHealthProxy.showUnknown && options.zyclopsHealthProxy.singleIp) { + throw new Error( + `${this.METADATA.NAME}: Zyclops health checks: "Show Unknown Releases" is incompatible with Single-IP mode; please disable one of them.` + ); + }packages/core/src/builtins/newznab/addon.ts (1)
118-128: Consolidate duplicated validation logic.This validation for backbone vs provider host is duplicated from the preset file (
packages/core/src/presets/newznab.ts, lines 400-410). The duplication violates DRY principles and could lead to inconsistencies if validation requirements change. Additionally, the error messages differ slightly between the two locations ("Crowdsourced health checks" vs "Zyclops health checks").As suggested in the past review comments, consider either:
- Performing validation only in the preset (since it runs first during configuration), or
- Extracting the validation to a shared utility function
This will ensure consistency and reduce maintenance burden.
🧹 Nitpick comments (4)
packages/core/src/presets/newznab.ts (2)
6-27: Consider simplifying boolean check and handling edge cases.A few minor refinements:
- Line 11: The
Boolean()wrapper is redundant since optional chaining already coerces to boolean.- Lines 18-21: The type guard could be more concise.
- Line 23:
replace()only replaces the first occurrence; usereplaceAll()if multiple instances of 'healthy' might appear.- If
stream.zyclopsHealthis an empty string, it will return'NZB Health: 'which may not be desired.🔎 Proposed refactor
protected override getMessage( stream: Stream, currentParsedStream: ParsedStream ): string | undefined { - const healthChecksEnabled = Boolean( - this.addon.preset.options?.zyclopsHealthProxy?.enabled - ); + const healthChecksEnabled = + this.addon.preset.options?.zyclopsHealthProxy?.enabled; if (!healthChecksEnabled) { return undefined; } - const zyclopsHealth = - typeof stream.zyclopsHealth === 'string' - ? stream.zyclopsHealth - : undefined; - if (zyclopsHealth) { + const zyclopsHealth = + typeof stream.zyclopsHealth === 'string' && stream.zyclopsHealth.trim() + ? stream.zyclopsHealth + : undefined; + if (zyclopsHealth) { - return 'NZB Health: ' + zyclopsHealth.replace('healthy', '🧝'); + return 'NZB Health: ' + zyclopsHealth.replaceAll('healthy', '🧝'); } return undefined; }
392-398: Consider more defensive validation for the backbones array.The current check at line 398 only validates that the array has items, but doesn't verify that those items are non-empty strings. If the array contains empty strings, falsy values, or non-string types, the validation could pass incorrectly.
Based on the pattern suggested in past review comments:
🔎 Proposed refinement
- const backbonesSelected = options.zyclopsHealthProxy?.backbones?.length; + const backbonesSelected = + Array.isArray(options.zyclopsHealthProxy.backbones) && + options.zyclopsHealthProxy.backbones.some( + (value) => typeof value === 'string' && value.trim().length > 0 + );This ensures only valid, non-empty string backbones are considered.
packages/core/src/builtins/newznab/addon.ts (2)
108-110: Filter empty strings when mapping backbones.The current mapping only trims whitespace but doesn't filter out empty strings. If the configuration contains empty backbone values, they could be included in the comma-separated parameter, potentially causing issues with the health proxy endpoint.
🔎 Proposed refinement
const selectedBackbones = ( this.userData.zyclopsHealthProxy.backbones || [] - ).map((backbone) => backbone?.trim()); + ).map((backbone) => backbone?.trim()).filter((backbone) => backbone && backbone.length > 0);
247-268: Type assertion is functional but could be more type-safe.The override correctly propagates
zyclopsHealthfrom NZB to Stream, but the type assertion(stream as Record<string, unknown>)bypasses type checking. Whilst this is functional, it could mask type issues if the Stream interface changes.If you find yourself extending Stream properties frequently, consider either:
- Adding an optional
zyclopsHealth?: stringfield directly to the Stream interface inpackages/core/src/db/index.ts, or- Creating a type-safe extension pattern for addon-specific metadata.
However, given that this is likely the only place where health data is attached, the current approach is acceptable.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/core/src/builtins/base/nab/api.tspackages/core/src/builtins/newznab/addon.tspackages/core/src/debrid/utils.tspackages/core/src/presets/newznab.tspackages/core/src/utils/env.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/debrid/utils.ts
- packages/core/src/utils/env.ts
- packages/core/src/builtins/base/nab/api.ts
🔇 Additional comments (5)
packages/core/src/presets/newznab.ts (2)
30-32: LGTM!The static parser registration is clean and follows the expected pattern for extending
BuiltinAddonPreset.
323-336: LGTM!The explicit type annotations using
NonNullable<UserData['services']>[number]improve type safety and make the service mapping clearer.packages/core/src/builtins/newznab/addon.ts (3)
25-32: LGTM!The addition of optional
extraParamsto route health proxy parameters through the API is clean and backward-compatible.
37-45: LGTM!The Zod schema extension correctly uses Zod 4 syntax and appropriately validates the health proxy configuration structure.
172-198: LGTM!The extraction and conditional attachment of
zyclopsHealthto NZB objects is well-guarded and only adds the property when health data is actually present.
|
Would it be possible to use |
I don't think this will be possible, no. Zyclops reaches out directly to your indexer, there's no option for it to proxy that request. |
|
Okay, thanks |
3 participants
This PR (mostly written by copilot and tweaked by me) adds optional Zyclops crowdsourced health checks for results from Newznab indexers.
The Zyclops solution is described at https://zyclops.elfhosted.com - I've tried to make the way it works (public health checks ingest one untested NZB per-query for further testing to enrich the health database) as clear as possible, but happy for feedback / revision / refactor requests! 😄
In the background, there are 300+ workers performing health checks against all the backbones, keeping the health data "fresh" and testing new ingestions 🔥
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.