cookiebanner: address PR review feedback

- Detectors: keep batched entries in `pending` until the POST succeeds
  and guard against concurrent flushes, so transient network errors no
  longer silently drop detection reports.
- Worker: add stable tie-breakers to the merge-candidate sort so the
  greedy assignment produces deterministic groups across runs.
- Handler: skip resource entries with an empty URL (zero-value `uri.URI`
  when the `url` field is missing) before persisting them.
- Third-party detector: allow same-origin service worker scripts through
  `processResource` -- service workers are always same-origin by spec,
  so the previous filter made `wrapServiceWorker` unreachable.
- Resource row edit: bump the description cell `colSpan` to 3 so the
  edit row spans all five table columns.
- Resolver: handle `ErrSameResourceCategoryMove` explicitly so the no-op
  move returns a validation error instead of an internal one.

Signed-off-by: Émile Ré <emile@getprobo.com>

apps/console/src/pages/organizations/cookie-banners/configuration/resources/_components/TrackerResourceRowEdit.tsx

@@ -61,7 +61,7 @@ export function TrackerResourceRowEdit({
           placeholder={__("Display name")}
         />
       </Td>
-      <Td className="pr-3" colSpan={2}>
+      <Td className="pr-3" colSpan={3}>
         <div className="flex items-center gap-2">
           <Input
             {...register("description")}

packages/cookie-banner/src/detectors/cookie-detector.ts

@@ -41,6 +41,7 @@ export class CookieDetector implements Detector {
   private readonly reported: Set<string> = new Set();
   private readonly pending: Map<string, DetectedCookieEntry> = new Map();
   private timer: ReturnType<typeof setTimeout> | null = null;
+  private flushing = false;
   private originalDescriptor: PropertyDescriptor | null = null;
   private cookieStoreHandler: ((event: CookieChangeEvent) => void) | null = null;
 
@@ -165,36 +166,46 @@ export class CookieDetector implements Detector {
   }
 
   private scheduleFlush(): void {
-    if (this.timer) return;
+    if (this.timer || this.flushing) return;
     this.timer = setTimeout(() => {
       this.timer = null;
       this.flush();
     }, DEBOUNCE_MS);
   }
 
+  // flush sends one batch from `pending` and only removes entries on
+  // success. Transient failures leave entries in `pending` so they are
+  // retried on the next flush. `flushing` guards against re-sending an
+  // in-flight batch when new entries arrive mid-request.
   private flush(): void {
+    if (this.flushing) return;
     if (this.pending.size === 0) return;
 
-    const iter = this.pending.entries();
+    const batchKeys: string[] = [];
     const entries: DetectedCookieEntry[] = [];
-    for (const [key, entry] of iter) {
+    for (const [key, entry] of this.pending) {
+      batchKeys.push(key);
       entries.push(entry);
-      this.pending.delete(key);
       if (entries.length >= MAX_COOKIES_PER_REQUEST) break;
     }
 
+    this.flushing = true;
     void fetchJSON(this.reportUrl, {
       method: "POST",
       body: { cookies: entries },
-    }).catch((err) => {
+    })
+      .then(() => {
+        for (const key of batchKeys) this.pending.delete(key);
+      })
+      .catch((err) => {
         if (err instanceof NotFoundError) {
           this.pending.clear();
           this.stop();
         }
+      })
+      .finally(() => {
+        this.flushing = false;
+        if (this.pending.size > 0) this.scheduleFlush();
       });
-
-    if (this.pending.size > 0) {
-      this.scheduleFlush();
-    }
   }
 }

packages/cookie-banner/src/detectors/storage-detector.ts

@@ -44,6 +44,7 @@ export class StorageDetector implements Detector {
   private readonly reported: Set<string> = new Set();
   private readonly pending: Map<string, DetectedStorageEntry> = new Map();
   private timer: ReturnType<typeof setTimeout> | null = null;
+  private flushing = false;
   private originalSetItem: typeof Storage.prototype.setItem | null = null;
   private originalIDBOpen: typeof IDBFactory.prototype.open | null = null;
   private originalCachesOpen: typeof CacheStorage.prototype.open | null = null;
@@ -230,35 +231,46 @@ export class StorageDetector implements Detector {
   }
 
   private scheduleFlush(): void {
-    if (this.timer) return;
+    if (this.timer || this.flushing) return;
     this.timer = setTimeout(() => {
       this.timer = null;
       this.flush();
     }, DEBOUNCE_MS);
   }
 
+  // flush sends one batch from `pending` and only removes entries on
+  // success. Transient failures leave entries in `pending` so they are
+  // retried on the next flush. `flushing` guards against re-sending an
+  // in-flight batch when new entries arrive mid-request.
   private flush(): void {
+    if (this.flushing) return;
     if (this.pending.size === 0) return;
 
+    const batchKeys: string[] = [];
     const entries: DetectedStorageEntry[] = [];
     for (const [key, entry] of this.pending) {
+      batchKeys.push(key);
       entries.push(entry);
-      this.pending.delete(key);
       if (entries.length >= MAX_ITEMS_PER_REQUEST) break;
     }
 
+    this.flushing = true;
     void fetchJSON(this.reportUrl, {
       method: "POST",
       body: { storage: entries },
-    }).catch((err) => {
+    })
+      .then(() => {
+        for (const key of batchKeys) this.pending.delete(key);
+      })
+      .catch((err) => {
         if (err instanceof NotFoundError) {
           this.pending.clear();
           this.stop();
         }
+      })
+      .finally(() => {
+        this.flushing = false;
+        if (this.pending.size > 0) this.scheduleFlush();
       });
-
-    if (this.pending.size > 0) {
-      this.scheduleFlush();
-    }
   }
 }

packages/cookie-banner/src/detectors/third-party-detector.ts

@@ -79,6 +79,7 @@ export class ThirdPartyDetector implements Detector {
   private readonly reported: Set<string> = new Set();
   private readonly pending: Map<string, DetectedResourceEntry> = new Map();
   private timer: ReturnType<typeof setTimeout> | null = null;
+  private flushing = false;
   private observer: MutationObserver | null = null;
   private perfObserver: PerformanceObserver | null = null;
   private originalSWRegister: typeof ServiceWorkerContainer.prototype.register | null = null;
@@ -247,7 +248,12 @@ export class ThirdPartyDetector implements Detector {
     }
 
     if (parsed.protocol !== "http:" && parsed.protocol !== "https:") return;
-    if (parsed.origin === this.pageOrigin || parsed.origin === this.proboOrigin) return;
+    // Service workers are always same-origin per browser security rules,
+    // so we never drop them based on the page origin -- they are tracked
+    // regardless of where the script lives. The proboOrigin guard still
+    // applies so we never report our own SDK assets.
+    if (parsed.origin === this.proboOrigin) return;
+    if (resourceType !== "service_worker" && parsed.origin === this.pageOrigin) return;
 
     const identifier = parsed.origin + parsed.pathname;
     const reportKey = `${resourceType}:${identifier}`;
@@ -259,35 +265,46 @@ export class ThirdPartyDetector implements Detector {
   }
 
   private scheduleFlush(): void {
-    if (this.timer) return;
+    if (this.timer || this.flushing) return;
     this.timer = setTimeout(() => {
       this.timer = null;
       this.flush();
     }, DEBOUNCE_MS);
   }
 
+  // flush sends one batch from `pending` and only removes entries on
+  // success. Transient failures leave entries in `pending` so they are
+  // retried on the next flush. `flushing` guards against re-sending an
+  // in-flight batch when new entries arrive mid-request.
   private flush(): void {
+    if (this.flushing) return;
     if (this.pending.size === 0) return;
 
+    const batchKeys: string[] = [];
     const entries: DetectedResourceEntry[] = [];
     for (const [key, entry] of this.pending) {
+      batchKeys.push(key);
       entries.push(entry);
-      this.pending.delete(key);
       if (entries.length >= MAX_ITEMS_PER_REQUEST) break;
     }
 
+    this.flushing = true;
     void fetchJSON(this.reportUrl, {
       method: "POST",
       body: { resources: entries },
-    }).catch((err) => {
+    })
+      .then(() => {
+        for (const key of batchKeys) this.pending.delete(key);
+      })
+      .catch((err) => {
         if (err instanceof NotFoundError) {
           this.pending.clear();
           this.stop();
         }
+      })
+      .finally(() => {
+        this.flushing = false;
+        if (this.pending.size > 0) this.scheduleFlush();
       });
-
-    if (this.pending.size > 0) {
-      this.scheduleFlush();
-    }
   }
 }

pkg/cookiebanner/worker.go

@@ -272,8 +272,20 @@ func findMergeGroups(
 		}
 	}
 
+	// Sort by descending specificity (more fixed characters first), then
+	// descending coverage (more patterns matched first), then template
+	// name for a fully deterministic order. Without these tie-breakers
+	// the greedy assignment below depends on Go's randomised map
+	// iteration and the same input can produce different merge groups
+	// across runs.
 	sort.Slice(candidates, func(i, j int) bool {
+		if candidates[i].fixedChars != candidates[j].fixedChars {
 			return candidates[i].fixedChars > candidates[j].fixedChars
+		}
+		if len(candidates[i].patterns) != len(candidates[j].patterns) {
+			return len(candidates[i].patterns) > len(candidates[j].patterns)
+		}
+		return candidates[i].key.template < candidates[j].key.template
 	})
 
 	assigned := make(map[*coredata.TrackerPattern]bool)

pkg/server/api/console/v1/cookie_banner_resolvers.go

@@ -1155,6 +1155,8 @@ func (r *mutationResolver) MoveTrackerResourceToCategory(ctx context.Context, in
 			return nil, gqlutils.NotFound(ctx, err)
 		case errors.Is(err, cookiebanner.ErrCategoriesBannerMismatch):
 			return nil, gqlutils.NotFoundf(ctx, "tracker resource or target category not found")
+		case errors.Is(err, cookiebanner.ErrSameResourceCategoryMove):
+			return nil, gqlutils.Invalidf(ctx, "tracker resource is already in the target category")
 		default:
 			r.logger.ErrorCtx(ctx, "cannot move tracker resource to category", log.Error(err))
 			return nil, gqlutils.Internal(ctx)

pkg/server/api/cookiebanner/v1/handler.go

@@ -441,6 +441,14 @@ func (h *Handler) handleReportDetectedTrackers(w http.ResponseWriter, r *http.Re
 	}
 
 	for _, res := range body.Resources {
+		// The URI type validates scheme+host on UnmarshalText, so a
+		// non-empty value here is guaranteed to be a valid URL. We
+		// still need to reject the zero value, which occurs when the
+		// `url` JSON field is missing.
+		if res.URL == "" {
+			continue
+		}
+
 		var resourceType coredata.TrackerResourceType
 		switch strings.TrimSpace(res.ResourceType) {
 		case "script":