Skip to content

fix(mobile): reset the code-reviewer repo selection sender map on account change - #6825

Closed
iscekic wants to merge 2 commits into
mainfrom
kwf/janitor-mobile-code-reviewer-134c12a82d
Closed

iscekic wants to merge 2 commits into
mainfrom
kwf/janitor-mobile-code-reviewer-134c12a82d

Conversation

@iscekic

@iscekic iscekic commented Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

Changelog for users

  • Signing out or switching accounts no longer carries a pending repo-selection change into the next account.
  • A new account's selected repositories stay as they are instead of being rewritten by the previous account's toggle.

Changelog for maintainers

  • The sender map now lives in its own module and is exported for the account-boundary clear.
  • Sign-out calls the new clear, which cancels every pending debounced send and drops pending and server baselines.
  • Review the personal scope key: it is device-global with no account namespace, so it must be cleared at every boundary.
  • The test reset now delegates to the same clear function, so both paths stay in step.

E2E proof

Published PR head: 2cb22371b3e1105ea240d3914e77066883325e94 (same tree as proved source 29bf48c768b59a8ead88ff4a6703c2500a384a3c).

The module-level repository-selection sender map is keyed only by scope:platform and is never cleared on sign-out or a direct account switch, so a pending personal-scope toggle (or the serverSelection baseline) left by account A survives into account B on the same device and is re-applied and sent under B's credentials, rewriting B's selected repositories.

Code trace: apps/mobile/src/lib/hooks/use-code-reviewer-repo-selection.ts:4 changed in 29bf48c768b59a8ead88ff4a6703c2500a384a3c. Sense check (model): clearSessionScopedState now calls runClear(clearRepoSelectionSenders), and the new repo-selection-senders module's clearRepoSelectionSenders clears timers and the scope:platform map at every account boundary, so account A's pending toggle/serverSelection cannot survive into account B.

Changed lines
diff apps/mobile/src/lib/hooks/use-code-reviewer-repo-selection.ts
@@ -4,6 +4,12 @@ import { hashKey, useMutation, useQueryClient } from '@tanstack/react-query';
 import { i18n } from '@/i18n';
 import { announcingToast } from '@/lib/a11y/announcing-toast';
 import { type ReviewConfigData, type ReviewerPlatform } from '@/lib/code-reviewer-config';
+import {
+  clearRepoSelectionSenders,
+  getRepoSelectionSender,
+  type RepoSelectionSaveVars,
+  type RepoSelectionSender,
+} from '@/lib/hooks/repo-selection-senders';
 import { chainSave } from '@/lib/hooks/save-chain';
 import { trpcClient } from '@/lib/trpc';
 
@@ -18,41 +24,6 @@ import {
 
 export const REPO_SELECTION_DEBOUNCE_MS = 500;
 
-type RepoSelectionDelta = {
-  add: (number | string)[];
-  remove: (number | string)[];
-};
-
-type RepoSelectionSaveVars = RepoSelectionDelta & {
-  optimisticSelection: (number | string)[];
-};
-
-type RepoSelectionSender = {
-  timer: ReturnType<typeof setTimeout> | null;
-  // The latest user-intended selection. Null means no toggle is pending.
-  pendingSelection: (number | string)[] | null;
-  // The last server-confirmed selection. Null means the server state is not
-  // yet known (no toggle and no refetch have synced it).
-  serverSelection: (number | string)[] | null;
-  // The mutation trigger of the hook instance that currently owns this key.
-  mutate: ((vars: RepoSelectionSaveVars) => void) | null;
-};
-
-// One pending debounced send per scope+platform. The timer closes over the
-// sender state, so a remount never retargets an older timer. `serverSelection`
-// is the last server-confirmed selection; `pendingSelection` is the latest
-// user-intended selection and is null while nothing is pending.
-const repoSelectionSenders = new Map<string, RepoSelectionSender>();
-
-function getRepoSelectionSender(key: string): RepoSelectionSender {
-  let sender = repoSelectionSenders.get(key);
-  if (!sender) {
-    sender = { timer: null, pendingSelection: null, serverSelection: null, mutate: null };
-    repoSelectionSenders.set(key, sender);
-  }
-  return sender;
-}
-
 function sameSelection(a: (number | string)[] | null, b: (number | string)[] | null): boolean {
   if (a === null || b === null) {
     return a === b;
@@ -248,10 +219,5 @@ export function useRepoSelectionToggle(scope: string, platform: ReviewerPlatform
 // state so a test never leaks a fire into a later case (same pattern as
 // resetDraftTimersForTests in drafts.ts).
 export function resetRepoSelectionSendersForTests(): void {
-  for (const sender of repoSelectionSenders.values()) {
-    if (sender.timer) {
-      clearTimeout(sender.timer);
-    }
-  }
-  repoSelectionSenders.clear();
+  clearRepoSelectionSenders();
 }
diff apps/mobile/src/lib/auth/session-scoped-state.ts
@@ -8,6 +8,7 @@ import { clearClipboardImages } from '@/lib/agent-attachments/clipboard-image';
 import { clearArtifactMirror } from '@/lib/artifacts/artifact-mirror';
 import { resetArtifactMirrorSyncState } from '@/lib/artifacts/artifact-mirror-sync';
 import { notifyArtifactsChanged } from '@/lib/artifacts/artifact-provider-native';
+import { clearRepoSelectionSenders } from '@/lib/hooks/repo-selection-senders';
 import { clearTrustedHosts } from '@/lib/hooks/use-trusted-hosts';
 import { clearSystemSearchIndex } from '@/lib/native-system-search';
 import { clearRecentPrs } from '@/lib/pr-review/recent-prs';
@@ -105,6 +106,12 @@ export function clearSessionScopedState(): void {
   runClear(clearFilePartCache);
   runClear(clearClipboardImages);
   runClear(clearSessionAutoApprove);
+  // The code-reviewer repo-selection sender map is keyed by scope+platform,
+  // and the personal key (`personal:<platform>`) is device-global with no
+  // account namespace. A pending personal-scope toggle or its server baseline
+  // left by the previous account would otherwise be re-applied and sent under
+  // the next account's credentials, rewriting its selected repositories.
+  runClear(clearRepoSelectionSenders);
   runClear(clearUserSessionTitles);
   runClear(clearSessionGoalCollapseState);
   // Wiping the mirror is what makes "signed out shows nothing to browse" true;
diff apps/mobile/src/lib/hooks/repo-selection-senders.ts
@@ -0,0 +1,52 @@
+type RepoSelectionDelta = {
+  add: (number | string)[];
+  remove: (number | string)[];
+};
+
+export type RepoSelectionSaveVars = RepoSelectionDelta & {
+  optimisticSelection: (number | string)[];
+};
+
+export type RepoSelectionSender = {
+  timer: ReturnType<typeof setTimeout> | null;
+  // The latest user-intended selection. Null means no toggle is pending.
+  pendingSelection: (number | string)[] | null;
+  // The last server-confirmed selection. Null means the server state is not
+  // yet known (no toggle and no refetch have synced it).
+  serverSelection: (number | string)[] | null;
+  // The mutation trigger of the hook instance that currently owns this key.
+  mutate: ((vars: RepoSelectionSaveVars) => void) | null;
+};
+
+// One pending debounced send per scope+platform. The timer closes over the
+// sender state, so a remount never retargets an older timer. `serverSelection`
+// is the last server-confirmed selection; `pendingSelection` is the latest
+// user-intended selection and is null while nothing is pending. The store lives
+// outside the hook so a pending send survives a remount, and is cleared at an
+// account boundary (see `clearSessionScopedState`).
+const repoSelectionSenders = new Map<string, RepoSelectionSender>();
+
… 24 more diff lines
Owner request

Fix 1 janitor finding in mobile/code-reviewer. Fix every one; the proof covers each.

  1. The module-level repository-selection sender map is keyed only by scope:platform and is never cleared on sign-out or a direct account switch, so a pending personal-scope toggle (or the serverSelection baseline) left by account A survives into account B on the same device and is re-applied and sent under B's credentials, rewriting B's selected repositories.
    Trace: apps/mobile/src/lib/hooks/use-code-reviewer-repo-selection.ts:45: The module-level repository-selection sender map is keyed only by scope:platform and is never cleared on sign-out or a direct account switch, so a pending personal-scope toggle (or the serverSelection baseline) left by account A survives into account B on the same device and is re-applied and sent under B's credentials, rewriting B's selected repositories. (janitor area trust).
    Files: apps/mobile/src/lib/hooks/use-code-reviewer-repo-selection.ts.

The module-level repository-selection sender map is keyed only by scope:platform and is never cleared on sign-out or a direct account switch, so a pending personal-scope toggle (or the serverSelection

Asserted value: apps/mobile/src/lib/hooks/use-code-reviewer-repo-selection.ts. Sense check (model): session-scoped-state.ts adds runClear(clearRepoSelectionSenders) to clearSessionScopedState, and the new repo-selection-senders.ts exports the shared map plus clearRepoSelectionSenders, so the sender map and its serverSelection baseline are cleared at the account boundary.

The scripts were proven on an earlier base, so only the head ran.

Head log: backend-assert 615e7c5b992e exited 0
$ git diff --unified=3 d3e0f4e45970ca754b28c6cea9e880f9628707ac b0b6fb8dcefed424827d2bfda65e25d5775e4f37
diff --git a/apps/mobile/src/lib/hooks/use-code-reviewer-repo-selection.ts b/apps/mobile/src/lib/hooks/use-code-reviewer-repo-selection.ts
--- a/apps/mobile/src/lib/hooks/use-code-reviewer-repo-selection.ts
+++ b/apps/mobile/src/lib/hooks/use-code-reviewer-repo-selection.ts
-  for (const sender of repoSelectionSenders.values()) {
-    if (sender.timer) {
-      clearTimeout(sender.timer);
-    }
-  }
-  repoSelectionSenders.clear();
+  clearRepoSelectionSenders();
 }

@iscekic iscekic added the kwf-janitor Admitted to the workflow from a janitor finding label Sep 29, 2026
@iscekic iscekic self-assigned this Sep 29, 2026
@iscekic

iscekic commented Sep 29, 2026

Copy link
Copy Markdown
Collaborator Author

bot: Fixed failing checks in 612b25e.

@iscekic
iscekic marked this pull request as ready for review September 29, 2026 03:43
@iscekic iscekic added the merge-by-human the merge bot routed this PR to a human label Sep 29, 2026
Comment thread .github/workflows/cloud-agent-e2e-tests.yml Outdated
Comment thread apps/mobile/src/components/kilo-pass/kilo-pass-subscription-card.tsx Outdated
@kilo-code-bot

kilo-code-bot Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Executive Summary

The change cleanly extracts the repo-selection sender map into its own module and wires its clear into clearSessionScopedState, so a pending personal-scope toggle and its server baseline are dropped at both the sign-out and direct account-switch boundaries (both call sites confirmed in auth-context.tsx).

Files Reviewed (3 files)
  • apps/mobile/src/lib/hooks/repo-selection-senders.ts
  • apps/mobile/src/lib/hooks/use-code-reviewer-repo-selection.ts
  • apps/mobile/src/lib/auth/session-scoped-state.ts
Previous Review Summaries (2 snapshots, latest commit 698221a)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 698221a)

Status: No Issues Found | Recommendation: Merge

Executive Summary

The branch is now correctly scoped: the unrelated revert of main is gone, and the six-file repository-selection fix clears the module-scoped sender map on sign-out and direct account switch, covered by tests.

Files Reviewed (6 files)
  • apps/mobile/src/lib/hooks/use-code-reviewer-repo-selection.ts
  • apps/mobile/src/lib/hooks/use-code-reviewer-repo-selection.test.ts
  • apps/mobile/src/lib/auth/session-scoped-state.ts
  • apps/mobile/src/lib/auth/session-scoped-state.test.ts
  • apps/mobile/src/lib/auth/auth-context.lifecycle.test.tsx
  • apps/mobile/src/lib/auth/credentials.test.ts

Previous review (commit 612b25e)

Status: 2 Issues Found | Recommendation: Address before merge

Executive Summary

The intended repo-selection fix is correct, but the PR branch is not based on current main: its diff is 561 files / +6,816 / −74,253 and reverts large amounts of recently merged main work (mobile features, a perf fix, and CI automation).

Overview

Severity Count
CRITICAL 2
WARNING 0
SUGGESTION 0
Issue Details (click to expand)

CRITICAL

File Line Issue
.github/workflows/cloud-agent-e2e-tests.yml 35 Reverts #6803: drops the push: trigger, deploy job, and fallback URL defaults for the deployed e2e suite.
apps/mobile/src/components/kilo-pass/kilo-pass-subscription-card.tsx 98 Reverts #6640: re-adds the AppState foreground-refetch burst that PR intentionally removed.

Scope problem

GitHub reports changedFiles: 561, additions: 6,816, deletions: 74,253. Representative unintended reversions of main:

Recommendation: rebase or recreate kwf/janitor-mobile-code-reviewer-134c12a82d on current main and re-apply only the repository-selection change (clearRepoSelectionSenders + its clearSessionScopedState wiring). The merge commit 612b25e appears to have resolved in favor of stale branch content rather than main.

Files Reviewed (6 files)
  • apps/mobile/src/lib/hooks/use-code-reviewer-repo-selection.ts - no issues (fix is correct)
  • apps/mobile/src/lib/hooks/use-code-reviewer-repo-selection.test.ts - no issues
  • apps/mobile/src/lib/auth/session-scoped-state.ts - no issues (clear correctly wired into clearSessionScopedState)
  • apps/mobile/src/lib/auth/auth-context.lifecycle.test.tsx - no issues
  • apps/mobile/src/lib/auth/credentials.test.ts - no issues
  • .github/workflows/cloud-agent-e2e-tests.yml, apps/mobile/src/components/kilo-pass/kilo-pass-subscription-card.tsx - 2 issues

Fix these issues in Kilo Cloud


Reviewed by deepseek-v4.1-flash · Input: 0 · Output: 0 · Cached: 0

Review guidance: REVIEW.md from base branch main

@iscekic
iscekic marked this pull request as draft September 29, 2026 04:05
@iscekic iscekic removed the merge-by-human the merge bot routed this PR to a human label Sep 29, 2026
@iscekic
iscekic marked this pull request as ready for review September 29, 2026 04:40
@iscekic iscekic added the merge-by-human the merge bot routed this PR to a human label Sep 29, 2026
@iscekic
iscekic force-pushed the kwf/janitor-mobile-code-reviewer-134c12a82d branch from 698221a to 2cb2237 Compare September 30, 2026 01:59
@iscekic
iscekic marked this pull request as draft September 30, 2026 02:40
@iscekic iscekic removed the merge-by-human the merge bot routed this PR to a human label Sep 30, 2026
@iscekic

iscekic commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator Author

bot: Fixed failing checks in b0b6fb8.

@iscekic
iscekic marked this pull request as ready for review September 30, 2026 18:49
@iscekic iscekic added the merge-by-human the merge bot routed this PR to a human label Sep 30, 2026
@iscekic
iscekic marked this pull request as draft October 2, 2026 09:48
@iscekic iscekic removed the merge-by-human the merge bot routed this PR to a human label Oct 2, 2026
@iscekic iscekic closed this Oct 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kwf-janitor Admitted to the workflow from a janitor finding

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant