From d66b04ccf50659da64714c64486d0683044bd0ab Mon Sep 17 00:00:00 2001 From: Alex Shnitman Date: Mon, 27 Jul 2026 21:34:09 +0300 Subject: [PATCH] feat: let subscriptions be renamed from the list (#1044) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A subscription's name is captured once at subscribe time from the feed's own title, so playlists — particularly the UULF-prefixed channel-uploads playlists — all come back named "Videos" and stay that way. Adds an inline editor on the Name cell, mirroring the existing title-filter edit next to it. The update route already whitelisted `name` and update_subscription already applied it, so this is mostly the missing UI. The backend side is validation: the old `str(changes["name"])` accepted any type, any length and any whitespace, for a value that is persisted and broadcast to every connected client. validate_subscription_name now requires a string, collapses interior whitespace to keep the label single-line, and caps it at 200 characters. The name is display-only — it is used for the subscription list and log lines, never for download paths — so renaming cannot move where files land. --- app/subscriptions.py | 29 +++++++++++++- app/tests/test_subscriptions.py | 70 +++++++++++++++++++++++++++++++++ ui/src/app/app.html | 28 ++++++++++++- ui/src/app/app.spec.ts | 38 +++++++++++++++++- ui/src/app/app.ts | 31 +++++++++++++++ 5 files changed, 192 insertions(+), 4 deletions(-) diff --git a/app/subscriptions.py b/app/subscriptions.py index b845a03..f64e700 100644 --- a/app/subscriptions.py +++ b/app/subscriptions.py @@ -287,6 +287,24 @@ def validate_title_regex(value: Any) -> str: return s +# The name is a display label the user picks; it is persisted and broadcast to +# every connected client, so keep it a bounded single-line string. +SUBSCRIPTION_NAME_MAX_LENGTH = 200 + + +def validate_subscription_name(value: Any) -> str: + """Return a stored subscription name, or raise ValueError if unusable.""" + if not isinstance(value, str): + raise ValueError("name must be a string") + # Collapse newlines/tabs so a pasted title can't break the table layout. + name = " ".join(value.split()) + if not name: + raise ValueError("name must not be empty") + if len(name) > SUBSCRIPTION_NAME_MAX_LENGTH: + raise ValueError(f"name must be at most {SUBSCRIPTION_NAME_MAX_LENGTH} characters") + return name + + def _coerce_bool(value: Any) -> bool: """Accept JSON booleans and common string forms used by API clients.""" if isinstance(value, bool): @@ -674,6 +692,13 @@ class SubscriptionManager: return {"status": "ok"} async def update_subscription(self, sub_id: str, changes: dict) -> dict: + validated_name: Optional[str] = None + if "name" in changes: + try: + validated_name = validate_subscription_name(changes["name"]) + except ValueError as exc: + return {"status": "error", "msg": str(exc)} + validated_tr: Optional[str] = None if "title_regex" in changes: try: @@ -722,8 +747,8 @@ class SubscriptionManager: sub.enabled = validated_enabled if interval_set: sub.check_interval_minutes = validated_interval - if "name" in changes and changes["name"]: - sub.name = str(changes["name"]) + if validated_name is not None: + sub.name = validated_name if validated_tr is not None: sub.title_regex = validated_tr if skip_so_set: diff --git a/app/tests/test_subscriptions.py b/app/tests/test_subscriptions.py index 0f97af1..fbee0db 100644 --- a/app/tests/test_subscriptions.py +++ b/app/tests/test_subscriptions.py @@ -821,6 +821,76 @@ class SubscriptionPersistenceTests(unittest.IsolatedAsyncioTestCase): self.assertEqual(upd["subscription"]["title_regex"], "foo|bar") self.assertEqual(mgr.list_all()[0].title_regex, "foo|bar") + async def _add_one_subscription(self, mgr): + with patch( + "subscriptions.extract_flat_playlist", + return_value=( + {"_type": "channel", "title": "Videos"}, + [{"id": "v1", "title": "One", "webpage_url": "https://example.com/v1"}], + ), + ): + result = await mgr.add_subscription( + "https://example.com/playlist?list=UULFabc", + check_interval_minutes=60, + download_type="video", + codec="auto", + format="any", + quality="best", + folder="", + custom_name_prefix="", + auto_start=True, + playlist_item_limit=0, + split_by_chapters=False, + chapter_template="", + subtitle_language="en", + subtitle_mode="prefer_manual", + ) + return result["subscription"]["id"] + + async def test_update_subscription_renames(self): + """Issue #1044: UULF-style uploads playlists all come back named 'Videos', + so the user needs to be able to relabel them.""" + with tempfile.TemporaryDirectory() as tmp: + mgr = SubscriptionManager(_Config(tmp), _Queue(), _Notifier()) + sub_id = await self._add_one_subscription(mgr) + self.assertEqual(mgr.list_all()[0].name, "Videos") + + upd = await mgr.update_subscription(sub_id, {"name": " Jane's uploads \n"}) + self.assertEqual(upd["status"], "ok") + # Surrounding and interior whitespace is collapsed to keep the name + # a single-line label. + self.assertEqual(upd["subscription"]["name"], "Jane's uploads") + self.assertEqual(mgr.list_all()[0].name, "Jane's uploads") + + async def test_update_subscription_rename_survives_reload(self): + with tempfile.TemporaryDirectory() as tmp: + cfg = _Config(tmp) + mgr = SubscriptionManager(cfg, _Queue(), _Notifier()) + sub_id = await self._add_one_subscription(mgr) + await mgr.update_subscription(sub_id, {"name": "Renamed"}) + + reloaded = SubscriptionManager(cfg, _Queue(), _Notifier()) + self.assertEqual(reloaded.get(sub_id).name, "Renamed") + + async def test_update_subscription_rejects_unusable_name(self): + with tempfile.TemporaryDirectory() as tmp: + mgr = SubscriptionManager(_Config(tmp), _Queue(), _Notifier()) + sub_id = await self._add_one_subscription(mgr) + + for bad in ("", " ", "\n\t", 42, None, ["a"], "x" * 201): + upd = await mgr.update_subscription(sub_id, {"name": bad}) + self.assertEqual(upd["status"], "error", f"expected {bad!r} to be rejected") + self.assertEqual(mgr.list_all()[0].name, "Videos") + + async def test_update_subscription_accepts_name_at_length_limit(self): + with tempfile.TemporaryDirectory() as tmp: + mgr = SubscriptionManager(_Config(tmp), _Queue(), _Notifier()) + sub_id = await self._add_one_subscription(mgr) + + upd = await mgr.update_subscription(sub_id, {"name": "x" * 200}) + self.assertEqual(upd["status"], "ok") + self.assertEqual(mgr.list_all()[0].name, "x" * 200) + async def test_update_subscription_skip_subscriber_only(self): with tempfile.TemporaryDirectory() as tmp: queue = _Queue() diff --git a/ui/src/app/app.html b/ui/src/app/app.html index 5e875f3..c6e436c 100644 --- a/ui/src/app/app.html +++ b/ui/src/app/app.html @@ -958,7 +958,33 @@ [disabled]="downloads.loading" [attr.aria-label]="'Select subscription ' + entry[1].name" /> - {{ entry[1].name }} + + @if (editingNameId === entry[0]) { +
+ + + +
+ } @else { +
+ {{ entry[1].name }} + +
+ } + {{ entry[1].url }} @if (editingTitleRegexId === entry[0]) { diff --git a/ui/src/app/app.spec.ts b/ui/src/app/app.spec.ts index 96cd963..6bd6ed7 100644 --- a/ui/src/app/app.spec.ts +++ b/ui/src/app/app.spec.ts @@ -81,7 +81,10 @@ class SubscriptionsServiceStub { return of({}); } - update() { + updateCalls: [string, unknown][] = []; + + update(id: string, changes: unknown) { + this.updateCalls.push([id, changes]); return of({ status: 'ok' as const }); } @@ -315,4 +318,37 @@ describe('App', () => { expect(errorSpy).toHaveBeenCalledWith('Invalid subscription title filter (regex)'); errorSpy.mockRestore(); }); + + it('renames a subscription and closes the inline editor', () => { + const fixture = TestBed.createComponent(App); + const app = fixture.componentInstance; + const subs = TestBed.inject(SubscriptionsService) as unknown as SubscriptionsServiceStub; + + app.beginEditName('sub1', 'Videos'); + expect(app.editingNameId).toBe('sub1'); + expect(app.nameEditDraft).toBe('Videos'); + + app.nameEditDraft = ' Jane uploads '; + app.saveName('sub1'); + + expect(subs.updateCalls).toEqual([['sub1', { name: 'Jane uploads' }]]); + expect(app.editingNameId).toBeNull(); + }); + + it('blocks renaming a subscription to an empty name', () => { + const toasts = TestBed.inject(ToastService); + const errorSpy = vi.spyOn(toasts, 'error').mockImplementation(() => undefined); + const fixture = TestBed.createComponent(App); + const app = fixture.componentInstance; + const subs = TestBed.inject(SubscriptionsService) as unknown as SubscriptionsServiceStub; + + app.beginEditName('sub1', 'Videos'); + app.nameEditDraft = ' '; + app.saveName('sub1'); + + expect(subs.updateCalls.length).toBe(0); + expect(app.editingNameId).toBe('sub1'); + expect(errorSpy).toHaveBeenCalledWith('Subscription name must not be empty'); + errorSpy.mockRestore(); + }); }); diff --git a/ui/src/app/app.ts b/ui/src/app/app.ts index 67f467e..99b112b 100644 --- a/ui/src/app/app.ts +++ b/ui/src/app/app.ts @@ -102,6 +102,9 @@ export class App implements AfterViewInit, OnInit, OnDestroy { skipSubscriberOnly = false; editingTitleRegexId: string | null = null; titleRegexEditDraft = ''; + editingNameId: string | null = null; + nameEditDraft = ''; + readonly subscriptionNameMaxLength = 200; cachedSubs: [string, SubscriptionRow][] = []; selectedSubscriptionIds = new Set(); checkingSubscriptionIds = new Set(); @@ -663,6 +666,34 @@ export class App implements AfterViewInit, OnInit, OnDestroy { }); } + beginEditName(id: string, current: string | undefined) { + this.editingNameId = id; + this.nameEditDraft = current ?? ''; + this.cdr.markForCheck(); + } + + cancelEditName() { + this.editingNameId = null; + this.nameEditDraft = ''; + this.cdr.markForCheck(); + } + + saveName(id: string) { + const name = (this.nameEditDraft || '').trim(); + if (!name) { + this.toasts.error('Subscription name must not be empty'); + return; + } + this.subscriptionsSvc.update(id, { name }).subscribe((res) => { + const error = this.getStatusError(res); + if (error) { + this.toasts.error(error || 'Update subscription failed'); + return; + } + this.cancelEditName(); + }); + } + deleteSubscription(id: string) { this.subscriptionsSvc.delete([id]).subscribe((res) => { const error = this.getStatusError(res);