fix(roundup): address pre-merge review findings for v1.5.6 - #124
Merged
Conversation
- libraryPruner: run once shortly after boot, not only on a 24h setInterval, so frequent restarts (e.g. config saves) don't skip every prune cycle and let seeded dedup keys expire - seen-items TTL raised to match roundup-first-seen's ~5yr backstop; actual removal for deleted items still happens via the daily prune scan, not TTL expiry - roundupScheduler: rebind to the new Discord client on bot restart instead of ignoring the second start() call, matching the existing daily-pick scheduler pattern - configFile: migrate legacy EMBED_SHOW_OVERVIEW to the new _MOVIES/_EPISODES split so users who disabled it (often to avoid episode spoilers) don't get overviews silently re-enabled - interactions: move the Seerr request error messages (including the new quota message) through locales/ instead of hardcoded English - weeklyRoundup: log the underlying parse error instead of an empty catch block in the JELLYFIN_BASE_URL preflight
- roundupScheduler: clearTimeout alone doesn't stop a chain whose tick already fired and is in flight when start() is called again on restart — its .finally(scheduleNext) would still re-arm using the stale client/closure. Add a generation token so a superseded chain becomes a no-op instead of quietly ticking against the destroyed client. - i18n: t() now falls back to the English value for a key missing in the active locale (instead of leaking the raw key string into Discord messages), for locale files that exist but lag behind on newer keys. - locales/fr.json: add the seerr_request_errors block directly too, consistent with de/sv/template. - CHANGELOG: fix the 1.5.6 EMBED_SHOW_OVERVIEW entry, which told users to manually re-configure the setting — it's now migrated automatically.
- roundupScheduler: add stop() (bumps generation, clears the pending timer) and call it from both discordClient.destroy() sites (routes/botRoutes.js stop-bot handler, app.js config-triggered restart). Without this, stopping the bot (not restarting it) left the scheduler ticking against a destroyed client until the failure circuit opened for the week — the same failure mode the previous round's restart fix addressed, just for a different trigger. - locales/fr.json: drop the seerr_request_errors block added last round — it was the English source text copy-pasted in, not an actual French translation, and it silently suppressed the new per-key en fallback warning. Leaving the keys absent lets the fallback (and its logger.warn) do its job until someone translates them for real. - utils/i18n.js: minor cleanup — reuse the already-loaded en.json fallback instead of reading it from disk twice, and log an error if en.json itself fails to load while another locale is active (previously a silent no-fallback-left edge case).
npm audit --audit-level=high started failing on the fix branch with two newly-published high-severity advisories, unrelated to the review fixes in this PR: - body-parser <1.20.6 (GHSA-v422-hmwv-36x6): DoS via invalid limit value silently disabling size enforcement. body-parser is a transitive dep of express, not a direct one, so pinned via overrides like the existing undici/follow-redirects entries. - axios 1.0.0-1.17.0: several newly-disclosed advisories (prototype pollution, DoS via recursion, maxBodyLength bypasses). Resolved by `npm audit fix`, which bumped the resolved version to 1.19.0 — still satisfies the existing `^1.17.0` range in package.json, so no version constraint change needed there. `npm audit --audit-level=high` now reports 0 vulnerabilities.
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
Fixes for the 4 non-security findings from the pre-merge code-review / silent-failure-hunter gate run on PR #106 (Release v1.5.6). Security-auditor found nothing to fix.
setIntervalwith no run at boot. Now also runs once ~5 minutes after boot; seen-items TTL raised to matchroundup-first-seen's ~5yr backstop (actual removal for deleted items still happens via the daily prune scan, not TTL expiry).start()call, so restarting the bot from the dashboard left it bound to the destroyed old Discord client — every tick then failed until the failure circuit opened for the week. Now rebinds to the new client, matching the existing daily-pick scheduler pattern.EMBED_SHOW_OVERVIEW→_MOVIES/_EPISODESsplit had no migration, so anyone who had disabled it (often for episode spoilers) got overviews silently re-enabled. Added to the existing config auto-migration block.locales/. MovedgetSeerrErrorMessage()inbot/interactions.jstot(), addedseerr_request_errors.*keys to en/de/sv/template.weeklyRoundup.js'sJELLYFIN_BASE_URLpreflight with a debug log of the underlying parse error.