Skip to content

Commit d048238

Browse files
committed
unify dup checking
1 parent 7831169 commit d048238

4 files changed

Lines changed: 198 additions & 80 deletions

File tree

frontend/src/lib/components/AddFeedModal.svelte

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,7 @@
88
import { api } from '$lib/services/api';
99
import { syncStore } from '$lib/stores/sync.svelte';
1010
import {
11-
crossTypePairsForHost,
12-
normalizeSiteHost,
11+
crossTypeDuplicatesForAdded,
1312
type CrossTypeDuplicate,
1413
} from '$lib/services/subscriptionDedup';
1514
import { dismissUnifyHost } from '$lib/services/unifyDismiss';
@@ -128,12 +127,12 @@
128127
let isAdding = $state(false);
129128
130129
// Open the just-added feed and close, unless the add duplicates an existing
131-
// source on the same host — then pause on the unify step instead.
132-
function finishAdd(id: number, siteUrl: string | undefined) {
130+
// source on a shared host — then pause on the unify step instead. The added
131+
// sub already carries its feedUrl (and, for standard.site, its siteUrl), so
132+
// crossTypeDuplicatesForAdded derives the host from the sub itself.
133+
function finishAdd(id: number) {
133134
const sub = subscriptionsStore.getById(id);
134-
const host = normalizeSiteHost(siteUrl);
135-
const pairs =
136-
sub && host ? crossTypePairsForHost(subscriptionsStore.subscriptions, sub, host) : [];
135+
const pairs = sub ? crossTypeDuplicatesForAdded(subscriptionsStore.subscriptions, sub) : [];
137136
if (pairs.length > 0) {
138137
unifyPairs = pairs;
139138
unifyKeptFeedId = id;
@@ -170,8 +169,7 @@
170169
});
171170
}
172171
173-
// The RSS sub's siteUrl isn't resolved yet, so match on the URL's host.
174-
finishAdd(id, url);
172+
finishAdd(id);
175173
} catch (e) {
176174
error = e instanceof Error ? e.message : 'Failed to add feed';
177175
isDiscovering = false;
@@ -205,7 +203,7 @@
205203
// Fetch this publication's documents now so they appear immediately
206204
// (also refreshed on the regular cycle).
207205
void fetchAllDocuments(subscriptionsStore.subscriptions);
208-
finishAdd(id, site.url);
206+
finishAdd(id);
209207
} catch (e) {
210208
error = e instanceof Error ? e.message : 'Failed to add subscription';
211209
isDiscovering = false;

frontend/src/lib/components/sidebar/SidebarAddFeed.svelte

Lines changed: 17 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,7 @@
99
import { searchBlueskyActors, type BlueskySearchResult } from '$lib/services/blueskySearch';
1010
import { api } from '$lib/services/api';
1111
import {
12-
crossTypePairsForHost,
13-
normalizeSiteHost,
12+
crossTypeDuplicatesForAdded,
1413
type CrossTypeDuplicate,
1514
} from '$lib/services/subscriptionDedup';
1615
import { dismissUnifyHost } from '$lib/services/unifyDismiss';
@@ -247,20 +246,21 @@
247246
let subscribingStandardSub = $state<string | null>(null);
248247
249248
// Cross-type duplicate pairs the just-added subs form with existing sources.
250-
// Each added sub is matched on its site host (passed in, since a fresh RSS
251-
// sub's siteUrl hasn't resolved yet).
252-
function detectUnifyPairs(added: Array<{ id: number; siteUrl?: string }>): CrossTypeDuplicate[] {
249+
// crossTypeDuplicatesForAdded derives each added sub's hosts from the sub
250+
// itself (its feedUrl host is set immediately, even before siteUrl resolves),
251+
// so this matches identically to the /sources scan. Deduped to one notice per
252+
// shared host — UnifyNotice is keyed and dismissed per host.
253+
function detectUnifyPairs(added: Array<{ id: number }>): CrossTypeDuplicate[] {
253254
const subs = subscriptionsStore.subscriptions;
254-
const pairs: CrossTypeDuplicate[] = [];
255-
const seenHosts = new Set<string>();
256-
for (const { id, siteUrl } of added) {
257-
const host = normalizeSiteHost(siteUrl);
255+
const byHost = new Map<string, CrossTypeDuplicate>();
256+
for (const { id } of added) {
258257
const sub = subscriptionsStore.getById(id);
259-
if (!host || !sub || seenHosts.has(host)) continue;
260-
seenHosts.add(host);
261-
pairs.push(...crossTypePairsForHost(subs, sub, host));
258+
if (!sub) continue;
259+
for (const pair of crossTypeDuplicatesForAdded(subs, sub)) {
260+
if (!byHost.has(pair.host)) byHost.set(pair.host, pair);
261+
}
262262
}
263-
return pairs;
263+
return [...byHost.values()];
264264
}
265265
266266
// After adding, either pause on the unify notice or open the kept feed.
@@ -338,7 +338,7 @@
338338
339339
socialStore.loadFeed(true);
340340
void fetchAllDocuments(subscriptionsStore.subscriptions);
341-
settleAdd(detectUnifyPairs([{ id: subId, siteUrl: sub.publication.url }]), subId);
341+
settleAdd(detectUnifyPairs([{ id: subId }]), subId);
342342
} catch (e) {
343343
error = e instanceof Error ? e.message : 'Failed to subscribe';
344344
} finally {
@@ -455,8 +455,7 @@
455455
});
456456
}
457457
458-
// The RSS sub's siteUrl isn't resolved yet, so match on the URL's host.
459-
settleAdd(detectUnifyPairs([{ id, siteUrl: url }]), id);
458+
settleAdd(detectUnifyPairs([{ id }]), id);
460459
} catch (e) {
461460
error = e instanceof Error ? e.message : 'Failed to add feed';
462461
mode = 'idle';
@@ -512,7 +511,7 @@
512511
error = null;
513512
isSubscribing = true;
514513
let firstAddedId: number | null = null;
515-
const added: Array<{ id: number; siteUrl?: string }> = [];
514+
const added: Array<{ id: number }> = [];
516515
517516
try {
518517
for (const pubUri of selectedPublications) {
@@ -529,7 +528,7 @@
529528
siteUrl: pub.url,
530529
feedUrl: pubUri,
531530
});
532-
added.push({ id: subId, siteUrl: pub.url });
531+
added.push({ id: subId });
533532
if (!firstAddedId) firstAddedId = subId;
534533
if (pub.iconUrl) {
535534
await subscriptionsStore.updateLocal(subId, {

frontend/src/lib/services/subscriptionDedup.test.ts

Lines changed: 83 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,9 @@ import {
77
createInFlightGuard,
88
DuplicateInFlightError,
99
normalizeSiteHost,
10+
subscriptionHosts,
1011
findCrossTypeDuplicates,
11-
crossTypePairsForHost,
12+
crossTypeDuplicatesForAdded,
1213
} from './subscriptionDedup';
1314
import type { Subscription } from '$lib/types';
1415

@@ -447,20 +448,54 @@ describe('findCrossTypeDuplicates', () => {
447448
});
448449

449450
/**
450-
* crossTypePairsForHost is the add-time counterpart: the just-added sub may not
451-
* have a resolved siteUrl yet (a fresh RSS row), so the host is passed in
452-
* explicitly rather than read off the row.
451+
* subscriptionHosts is the single source of truth both the add-time check and
452+
* the /sources scan derive their hosts from.
453453
*/
454-
describe('crossTypePairsForHost', () => {
455-
it('pairs a freshly-added RSS sub (no siteUrl) with an existing standard.site on the host', () => {
454+
describe('subscriptionHosts', () => {
455+
it('collects both the siteUrl host and the feedUrl host', () => {
456+
const s = sub({
457+
feedUrl: 'https://feeds.example.com/rss',
458+
siteUrl: 'https://www.example.com',
459+
});
460+
expect([...subscriptionHosts(s)].sort()).toEqual(['example.com', 'feeds.example.com']);
461+
});
462+
463+
it('falls back to the feedUrl host when siteUrl is unresolved', () => {
464+
const s = sub({ feedUrl: 'https://blog.example.com/feed.xml', siteUrl: undefined });
465+
expect([...subscriptionHosts(s)]).toEqual(['blog.example.com']);
466+
});
467+
468+
it('ignores an at:// feedUrl (standard.site streams match on siteUrl only)', () => {
469+
const s = sub({
470+
sourceType: 'atproto.documents',
471+
feedUrl: 'at://did:plc:abc/site.standard.publication/blog',
472+
siteUrl: 'https://example.com',
473+
});
474+
expect([...subscriptionHosts(s)]).toEqual(['example.com']);
475+
});
476+
});
477+
478+
/**
479+
* crossTypeDuplicatesForAdded is the add-time counterpart of
480+
* findCrossTypeDuplicates. It shares subscriptionHosts and the same kind/host
481+
* matching, so the two can never disagree — the consistency tests below pin
482+
* that down on the exact case the old split logic got wrong.
483+
*/
484+
describe('crossTypeDuplicatesForAdded', () => {
485+
it('pairs a freshly-added RSS sub (no siteUrl) with an existing standard.site via feedUrl host', () => {
456486
const std = sub({
457487
id: 1,
458488
sourceType: 'atproto.documents',
459489
subjectDid: 'did:plc:abc',
460490
siteUrl: 'https://blog.example.com',
461491
});
462-
const addedRss = sub({ id: 2, sourceType: 'rss', siteUrl: undefined });
463-
const pairs = crossTypePairsForHost([std, addedRss], addedRss, 'blog.example.com');
492+
const addedRss = sub({
493+
id: 2,
494+
sourceType: 'rss',
495+
feedUrl: 'https://blog.example.com/feed.xml',
496+
siteUrl: undefined,
497+
});
498+
const pairs = crossTypeDuplicatesForAdded([std, addedRss], addedRss);
464499
expect(pairs).toHaveLength(1);
465500
expect(pairs[0].rss.id).toBe(2);
466501
expect(pairs[0].standard.id).toBe(1);
@@ -475,7 +510,7 @@ describe('crossTypePairsForHost', () => {
475510
subjectDid: 'd',
476511
siteUrl: 'https://example.com',
477512
});
478-
const pairs = crossTypePairsForHost([rss, addedStd], addedStd, 'example.com');
513+
const pairs = crossTypeDuplicatesForAdded([rss, addedStd], addedStd);
479514
expect(pairs).toHaveLength(1);
480515
expect(pairs[0].rss.id).toBe(1);
481516
expect(pairs[0].standard.id).toBe(2);
@@ -485,17 +520,52 @@ describe('crossTypePairsForHost', () => {
485520
const addedRss = sub({ id: 1, sourceType: 'rss', siteUrl: 'https://example.com' });
486521
const otherRss = sub({ id: 2, sourceType: 'rss', siteUrl: 'https://example.com' });
487522
// Only same-type rows on the host: no opposite type to pair with.
488-
expect(crossTypePairsForHost([addedRss, otherRss], addedRss, 'example.com')).toEqual([]);
523+
expect(crossTypeDuplicatesForAdded([addedRss, otherRss], addedRss)).toEqual([]);
489524
});
490525

491-
it('returns nothing when no existing sub shares the host', () => {
526+
it('returns nothing when no existing sub shares a host', () => {
492527
const std = sub({
493528
id: 1,
494529
sourceType: 'atproto.documents',
495530
subjectDid: 'd',
496531
siteUrl: 'https://other.com',
497532
});
498-
const addedRss = sub({ id: 2, sourceType: 'rss', siteUrl: undefined });
499-
expect(crossTypePairsForHost([std, addedRss], addedRss, 'example.com')).toEqual([]);
533+
const addedRss = sub({
534+
id: 2,
535+
sourceType: 'rss',
536+
feedUrl: 'https://example.com/feed.xml',
537+
siteUrl: undefined,
538+
});
539+
expect(crossTypeDuplicatesForAdded([std, addedRss], addedRss)).toEqual([]);
540+
});
541+
542+
// The regression that motivated unifying the two paths: an existing RSS feed
543+
// with no resolved siteUrl (only a feedUrl host) that a newly-added
544+
// standard.site duplicates. The old add-time check matched existing subs on
545+
// siteUrl alone, so it stayed silent while /sources later flagged the pair.
546+
it('warns at add time on the same pair /sources would show (RSS has only a feedUrl host)', () => {
547+
const existingRss = sub({
548+
id: 1,
549+
sourceType: 'rss',
550+
feedUrl: 'https://blog.example.com/feed.xml',
551+
siteUrl: undefined,
552+
});
553+
const addedStd = sub({
554+
id: 2,
555+
sourceType: 'atproto.documents',
556+
subjectDid: 'd',
557+
siteUrl: 'https://blog.example.com',
558+
});
559+
const all = [existingRss, addedStd];
560+
561+
const addTime = crossTypeDuplicatesForAdded(all, addedStd);
562+
const onSources = findCrossTypeDuplicates(all);
563+
564+
expect(addTime).toHaveLength(1);
565+
expect(onSources).toHaveLength(1);
566+
// Both paths agree: same RSS, same standard, same host.
567+
expect(addTime[0].rss.id).toBe(onSources[0].rss.id);
568+
expect(addTime[0].standard.id).toBe(onSources[0].standard.id);
569+
expect(addTime[0].host).toBe(onSources[0].host);
500570
});
501571
});

0 commit comments

Comments
 (0)