Deduplicate GitHub Copilot accounts#140
Conversation
✅ AI Code Review CompletedReview finished. Check the PR for inline comments. 📋 View Logs | 🤖 Model: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cff7b87f4c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return true | ||
| } | ||
|
|
||
| return lhs.details.copilotUsedRequests != nil || rhs.details.copilotUsedRequests != nil |
There was a problem hiding this comment.
In isDuplicateCopilotUsage, the final fallback return lhs.details.copilotUsedRequests != nil || rhs.details.copilotUsedRequests != nil merges candidates even when account identities are different, as long as entitlement/remaining (and optionally plan/used/limit) happen to match. This will collapse distinct Copilot accounts that share the same quota state (for example, two accounts both at 0/300 early in a cycle), causing valid account rows to disappear. Deduplication should only happen when identity signals overlap (or another strict unique key matches), not merely because usage fields are present.
Useful? React with 👍 / 👎.
![op-gg-ai-devops[bot]](https://avatars.githubusercontent.com/in/2680015?s=60&v=4){kind=small}
There was a problem hiding this comment.
nice cleanup overall — moving Copilot dedup off the "no usage merge" pattern and onto an identity+usage check is a real improvement, and dropping cookie-source placeholders when token sources have real data fixes the obvious "ghost row" annoyance. the helpers (normalizedCopilotIdentity, copilotIdentityCandidates) are tidy and the case-insensitive normalization matches the email-first stability rule in AGENTS.md.
that said, the isDuplicateCopilotUsage fallback is doing a bit too much heavy lifting. read it carefully:
if !lhsIdentity.isEmpty && !rhsIdentity.isEmpty && !lhsIdentity.isDisjoint(with: rhsIdentity) {
return true
}
return lhs.details.copilotUsedRequests != nil || rhs.details.copilotUsedRequests != nilwhen both sides have non-empty identities but they don't overlap (i.e. genuinely different accounts), execution falls through and merges them anyway as long as either side has copilotUsedRequests. so two distinct Copilot accounts (e.g. personal + work) that happen to sit at the same number — like both fresh at 0/300 Pro right after a billing reset — would collapse into one row in the menu. that's a regression in the multi-account UX this PR is explicitly trying to improve. flagged inline with a suggested guard.
couple smaller things:
CopilotCLIProvider.swift:300still uses the oldisSameUsage: { _, _ in false }and a raw$0.accountIdkey. if dedup matters in the GUI path, the CLI path has the same problem; worth applying the same helpers to both call sites so they don't drift.primaryDetailsatCopilotProvider.swift:384still pulls fromcookieCandidateeven afterremovePlaceholderCandidatesWhenRealUsageExistshas filtered it out. tiny edge case but the top-level details could show stale/empty cookie info when a real token-based account is the survivor.- no tests for the new dedup helpers in
CopilotProviderTests.swift. given how nuancedisDuplicateCopilotUsageis, a few cases (same email different case, same usage no identity overlap, placeholder removal) would lock the behavior in.
functionally not blocking — the changes do improve the common case — but the fallback merge is sketchy enough that I'd tighten it before shipping.
| let lhsIdentity = copilotIdentityCandidates(lhs) | ||
| let rhsIdentity = copilotIdentityCandidates(rhs) | ||
| if !lhsIdentity.isEmpty && !rhsIdentity.isEmpty && !lhsIdentity.isDisjoint(with: rhsIdentity) { | ||
| return true | ||
| } | ||
|
|
||
| return lhs.details.copilotUsedRequests != nil || rhs.details.copilotUsedRequests != nil |
There was a problem hiding this comment.
this fallback merges two distinct accounts when their identities are non-empty but don't overlap — i.e. different accounts — as long as either side has copilotUsedRequests. example: a user with two GitHub Copilot accounts (personal + work) both freshly at 0/300 on the Pro plan after a billing reset will collapse into one row, hiding one of the accounts in the menu.
since the explicit identity-overlap check sits right above this return, the fallback should only kick in when one side genuinely has no identity to compare against (auth source returned nothing). suggestion:
let lhsIdentity = copilotIdentityCandidates(lhs)
let rhsIdentity = copilotIdentityCandidates(rhs)
// If either side has no identity at all, we can't tell them apart —
// fall back to usage equality + at least one side having a real used count.
if lhsIdentity.isEmpty || rhsIdentity.isEmpty {
return lhs.details.copilotUsedRequests != nil || rhs.details.copilotUsedRequests != nil
}
// Both sides have identities — only merge if they actually overlap.
return !lhsIdentity.isDisjoint(with: rhsIdentity)that keeps the "merge when we genuinely don't know who it is" intent without conflating two real accounts that just happen to look identical numerically.
| let candidates = removePlaceholderCandidatesWhenRealUsageExists(candidates) | ||
| let merged = CandidateDedupe.merge( | ||
| candidates, | ||
| accountId: { $0.accountId }, | ||
| isSameUsage: { _, _ in false }, | ||
| accountId: { normalizedCopilotIdentity($0.accountId) }, | ||
| isSameUsage: { isDuplicateCopilotUsage($0, $1) }, | ||
| priority: { $0.sourcePriority } | ||
| ) |
There was a problem hiding this comment.
this PR upgrades the GUI CopilotProvider.finalizeResult to a proper identity-aware dedup, but the CLI sibling here still uses the old isSameUsage: { _, _ in false } and a raw $0.accountId key — so the CLI will still show duplicate Copilot rows for the same account discovered from cookies + tokens, and won't drop browser-cookie placeholders the way the GUI now does.
since both providers share the exact same CopilotAccountCandidate shape, the new normalizedCopilotIdentity / isDuplicateCopilotUsage / removePlaceholderCandidatesWhenRealUsageExists helpers could be lifted into a shared extension and reused here so the two paths don't drift. otherwise the CLI just inherits the bug this PR is fixing.
| candidates: [CopilotAccountCandidate], | ||
| cookieCandidate: CopilotAccountCandidate? | ||
| ) -> ProviderResult { | ||
| let candidates = removePlaceholderCandidatesWhenRealUsageExists(candidates) |
There was a problem hiding this comment.
small consistency thing: cookieCandidate is captured before removePlaceholderCandidatesWhenRealUsageExists runs (line 342). when the cookie path fell back to an empty/placeholder result but a token candidate has real usage, the cookie candidate gets filtered out of accountResults — yet primaryDetails here still reaches for cookieCandidate?.details, so the top-level provider details could show stale/zeroed cookie data while the actual surviving account row is a token-based account.
probably safer to prefer accountResults.first?.details (highest-priority survivor) and only fall back to cookieCandidate?.details if accountResults is empty, or just check that the cookie candidate survived the placeholder filter before using it.
✅ AI Code Review CompletedReview finished. Check the PR for inline comments. 📋 View Logs | 🤖 Model: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1af5491531
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return !lhsIdentity.isDisjoint(with: rhsIdentity) | ||
| } | ||
|
|
||
| return lhs.details.copilotUsedRequests != nil || rhs.details.copilotUsedRequests != nil |
There was a problem hiding this comment.
The fallback return lhs.details.copilotUsedRequests != nil || rhs.details.copilotUsedRequests != nil marks two candidates as duplicates even when their identity sets do not overlap (or are missing), so any two accounts with matching entitlement/remaining can be collapsed into one. This is reproducible when one source lacks accountId/email (for example, cookie login missing) and another source has quota data: the higher-priority row wins and a valid separate account row disappears.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
nice follow-up — this round actually addresses every concern from the previous review. the distinct-accounts-collapse bug at line 446 is gone (identity overlap is now strictly required when both sides have identities), the CLI sibling got the same helpers so the two paths won't drift, and primaryDetails now prefers the highest-priority survivor over the cookie candidate. the case-insensitive normalizedCopilotIdentity keeps things consistent with the email-first stability rule in AGENTS.md. solid cleanup.
couple of small follow-ups left:
- DRY violation:
normalizedCopilotIdentity/copilotIdentityCandidates/isDuplicateCopilotUsage/removePlaceholderCandidatesWhenRealUsageExistsare now duplicated byte-for-byte acrossCopilotProvider.swiftandCopilotCLIProvider.swift.AGENTS.mdexplicitly says "Don't make copy & paste duplicated code." worth lifting into a smallCopilotCandidateDedupenamespace / shared file so future bug fixes only need to land once. - Fragile string match:
authSource.contains("browser cookies")(line 408 GUI / 366 CLI) leans on a free-form display label to decide what to drop. if anyone renames the auth source string later (e.g. adds another browser, capitalizes differently, switches separator), placeholder removal silently breaks. asourceenum onCopilotAccountCandidateor akindflag set at construction time would be safer than substring matching on UI strings. - No tests yet: the prior review asked for regression tests on the new dedup logic and they're still missing.
isDuplicateCopilotUsagehas enough branches now (entitlement guard, used/limit mismatch, plan mismatch, identity overlap, empty-identity fallback) that a few unit tests inCopilotProviderTests.swiftwould lock the multi-account behavior in.
functionally APPROVE — the regression risk this PR was fixing is real and is properly fixed. the items above are tidiness, not blockers.
| if entitlement > 0 { return true } | ||
|
|
||
| let authSource = candidate.details.authSource?.lowercased() ?? "" | ||
| return !authSource.contains("browser cookies") |
There was a problem hiding this comment.
the placeholder-removal decision hinges on authSource.contains("browser cookies") (case-insensitive), but authSource is a free-form display string built at the call site (e.g. "Browser Cookies (Chrome/Brave/Arc/Edge)" at CopilotProvider.swift:99 and :658, CopilotCLIProvider.swift:97). a future rename like "Browser Cookies / Comet" or "Cookies (Browser)" would silently break this filter — the placeholder removes nothing, ghost rows come back, and there's no test catching it.
safer signal would be a typed flag on the candidate itself rather than substring-matching on a UI label. minimal-touch option:
private struct CopilotAccountCandidate {
let accountId: String?
let usage: ProviderUsage
let details: DetailedUsage
let sourcePriority: Int
let isCookiePlaceholder: Bool // set true only when constructed from cookie path
}then removePlaceholderCandidatesWhenRealUsageExists checks candidate.isCookiePlaceholder instead of grepping authSource. bonus: the intent reads off the struct, not off a string label.
| private func isDuplicateCopilotUsage( | ||
| _ lhs: CopilotAccountCandidate, | ||
| _ rhs: CopilotAccountCandidate | ||
| ) -> Bool { | ||
| guard lhs.usage.totalEntitlement == rhs.usage.totalEntitlement, | ||
| lhs.usage.remainingQuota == rhs.usage.remainingQuota, | ||
| (lhs.usage.totalEntitlement ?? 0) > 0 else { | ||
| return false | ||
| } | ||
|
|
||
| if let lhsUsed = lhs.details.copilotUsedRequests, | ||
| let rhsUsed = rhs.details.copilotUsedRequests, | ||
| lhsUsed != rhsUsed { | ||
| return false | ||
| } | ||
|
|
||
| if let lhsLimit = lhs.details.copilotLimitRequests, | ||
| let rhsLimit = rhs.details.copilotLimitRequests, | ||
| lhsLimit != rhsLimit { | ||
| return false | ||
| } | ||
|
|
||
| let lhsPlan = normalizedCopilotIdentity(lhs.details.planType) | ||
| let rhsPlan = normalizedCopilotIdentity(rhs.details.planType) | ||
| if let lhsPlan, let rhsPlan, lhsPlan != rhsPlan { | ||
| return false | ||
| } | ||
|
|
||
| let lhsIdentity = copilotIdentityCandidates(lhs) | ||
| let rhsIdentity = copilotIdentityCandidates(rhs) | ||
| if !lhsIdentity.isEmpty && !rhsIdentity.isEmpty { | ||
| return !lhsIdentity.isDisjoint(with: rhsIdentity) | ||
| } | ||
|
|
||
| return lhs.details.copilotUsedRequests != nil || rhs.details.copilotUsedRequests != nil | ||
| } |
There was a problem hiding this comment.
the prior review explicitly asked for unit tests on the new dedup helpers, and this commit doesn't add any. isDuplicateCopilotUsage now has six distinct decision points (entitlement-zero guard, used mismatch, limit mismatch, plan mismatch, identity-overlap, empty-identity fallback), and the new placeholder filter has its own branch.
a handful of cases in CopilotProviderTests.swift would lock the multi-account behavior in and stop future "tidy this up" refactors from re-introducing the 0/300 Pro collapse bug:
- two candidates with same usage but disjoint emails → must NOT merge
- same email different case (
Foo@Example.comvsfoo@example.com) → must merge - one side identity-less + matching usage + at least one
copilotUsedRequests→ must merge - both identity-less + nil
copilotUsedRequests→ must NOT merge - placeholder cookie candidate + token candidate with real entitlement → cookie filtered out,
primaryDetailsfrom token survivor
not a blocker for this PR, but worth filing a follow-up issue if it's not landing here.
| private func removePlaceholderCandidatesWhenRealUsageExists( | ||
| _ candidates: [CopilotAccountCandidate] | ||
| ) -> [CopilotAccountCandidate] { | ||
| let hasRealUsage = candidates.contains { candidate in | ||
| (candidate.usage.totalEntitlement ?? 0) > 0 | ||
| } | ||
| guard hasRealUsage else { return candidates } | ||
|
|
||
| return candidates.filter { candidate in | ||
| let entitlement = candidate.usage.totalEntitlement ?? 0 | ||
| if entitlement > 0 { return true } | ||
|
|
||
| let authSource = candidate.details.authSource?.lowercased() ?? "" | ||
| return !authSource.contains("browser cookies") | ||
| } | ||
| } | ||
|
|
||
| private func isDuplicateCopilotUsage( | ||
| _ lhs: CopilotAccountCandidate, | ||
| _ rhs: CopilotAccountCandidate | ||
| ) -> Bool { | ||
| guard lhs.usage.totalEntitlement == rhs.usage.totalEntitlement, | ||
| lhs.usage.remainingQuota == rhs.usage.remainingQuota, | ||
| (lhs.usage.totalEntitlement ?? 0) > 0 else { | ||
| return false | ||
| } | ||
|
|
||
| if let lhsUsed = lhs.details.copilotUsedRequests, | ||
| let rhsUsed = rhs.details.copilotUsedRequests, | ||
| lhsUsed != rhsUsed { | ||
| return false | ||
| } | ||
|
|
||
| if let lhsLimit = lhs.details.copilotLimitRequests, | ||
| let rhsLimit = rhs.details.copilotLimitRequests, | ||
| lhsLimit != rhsLimit { | ||
| return false | ||
| } | ||
|
|
||
| let lhsPlan = normalizedCopilotIdentity(lhs.details.planType) | ||
| let rhsPlan = normalizedCopilotIdentity(rhs.details.planType) | ||
| if let lhsPlan, let rhsPlan, lhsPlan != rhsPlan { | ||
| return false | ||
| } | ||
|
|
||
| let lhsIdentity = copilotIdentityCandidates(lhs) | ||
| let rhsIdentity = copilotIdentityCandidates(rhs) | ||
| if !lhsIdentity.isEmpty && !rhsIdentity.isEmpty { | ||
| return !lhsIdentity.isDisjoint(with: rhsIdentity) | ||
| } | ||
|
|
||
| return lhs.details.copilotUsedRequests != nil || rhs.details.copilotUsedRequests != nil | ||
| } | ||
|
|
||
| private func copilotIdentityCandidates(_ candidate: CopilotAccountCandidate) -> Set<String> { | ||
| var identities = Set<String>() | ||
| if let accountId = normalizedCopilotIdentity(candidate.accountId) { | ||
| identities.insert(accountId) | ||
| } | ||
| if let email = normalizedCopilotIdentity(candidate.details.email) { | ||
| identities.insert(email) | ||
| } | ||
| return identities | ||
| } | ||
|
|
||
| private func normalizedCopilotIdentity(_ value: String?) -> String? { | ||
| guard let value else { return nil } | ||
| let normalized = value | ||
| .trimmingCharacters(in: .whitespacesAndNewlines) | ||
| .lowercased() | ||
| return normalized.isEmpty ? nil : normalized | ||
| } | ||
|
|
There was a problem hiding this comment.
these four helpers (removePlaceholderCandidatesWhenRealUsageExists, isDuplicateCopilotUsage, copilotIdentityCandidates, normalizedCopilotIdentity) are now byte-for-byte identical to the ones in CopilotMonitor/CopilotMonitor/Providers/CopilotProvider.swift:395-466. AGENTS.md is pretty direct about this: "Don't make copy & paste duplicated code."
since the only thing tying them to the enclosing type is the private CopilotAccountCandidate struct (which itself has the same shape in both files), the cheap fix is to either:
- Lift
CopilotAccountCandidateand the four helpers into a shared file (e.g.CopilotCandidateDedupe.swift) as a fileprivate-or-internal namespace, and have both providers call into it; or - Make a small
protocol CopilotCandidateOwner+ default-implementation extension that both providers conform to.
otherwise the next time someone tweaks dedup behavior (and the previous review already shows that's a frequent area), they'll have to remember to mirror the change in both files — and the GUI/CLI drift problem the previous review flagged is right back.
✅ AI Code Review CompletedReview finished. Check the PR for inline comments. 📋 View Logs | 🤖 Model: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b30e86738
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| let primaryDetails = cookieCandidate?.details ?? accountResults.first?.details | ||
| let primaryDetails = accountResults.first?.details ?? cookieCandidate?.details |
There was a problem hiding this comment.
Selecting accountResults.first?.details before cookieCandidate?.details can drop valid overage data in multi-account setups: buildCandidateFromToken does not populate copilotOverageCost/copilotOverageRequests, so when a higher-priority token-backed account sorts first, the provider-level details lose the browser-fetched add-on fields even though cookieCandidate has them. In that case the app treats Copilot add-on spend as zero and can hide the add-on row, which is a billing-accuracy regression.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| let primaryDetails = cookieCandidate?.details ?? accountResults.first?.details | ||
| let primaryDetails = accountResults.first?.details ?? cookieCandidate?.details |
There was a problem hiding this comment.
The same precedence change in the CLI provider makes details come from the first sorted account instead of the browser candidate; since token-derived candidates omit copilotOverageCost and copilotOverageRequests, CLI output can silently lose those fields whenever a token account outranks the browser account. This causes downstream tooling to read $0/missing add-on usage even when cookie-based usage data was successfully fetched.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
nice — the latest commit (8b30e86) cleanly addresses the regression flagged in the previous round. isDuplicateCopilotUsage now uses guard !lhsIdentity.isEmpty, !rhsIdentity.isEmpty else { return false } and returns the identity-overlap result directly, which is exactly what was needed. two distinct Copilot accounts that happen to land on the same numbers (e.g. both 0/300 Pro right after a billing reset) will no longer collapse into one row in the menu.
paired with the earlier round's fixes that already landed — CLI provider mirroring the same helpers, primaryDetails preferring the highest-priority survivor over the cookie candidate, and removePlaceholderCandidatesWhenRealUsageExists dropping cookie placeholders when token sources have real data — the multi-account dedup story is in a solid place. the normalizedCopilotIdentity lowercased/trimmed comparison also keeps things consistent with the email-first stability rule in AGENTS.md.
CI signal looks healthy too: SwiftLint ✅, GitHub Actions Lint ✅, Build & Test ✅ (one Build matrix still running at review time).
Items already noted in previous reviews (not re-raising):
- duplicated dedup helpers across
CopilotProvider.swiftandCopilotCLIProvider.swift— still worth lifting into a sharedCopilotCandidateDedupenamespace authSource.contains("browser cookies")substring match — still fragile to label renames- no unit tests yet for
isDuplicateCopilotUsage/removePlaceholderCandidatesWhenRealUsageExists
these are tidiness, not blockers, and were already discussed inline in the prior review threads — author can address them in a follow-up.
APPROVE 🚀
✅ AI Code Review CompletedReview finished. Check the PR for inline comments. 📋 View Logs | 🤖 Model: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42659757c1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| let primaryDetails = cookieCandidate?.details ?? accountResults.first?.details | ||
| let primaryDetails = accountResults.first?.details ?? cookieCandidate?.details |
There was a problem hiding this comment.
In finalizeResult, selecting accountResults.first?.details before cookieCandidate?.details can drop browser-derived overage fields when a token-backed account sorts first. Token candidates built in buildCandidateFromToken do not set copilotOverageCost/copilotOverageRequests, so this change can make the app report Copilot add-on spend as zero and hide the add-on spend row even when cookie usage fetched billing data successfully.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| let primaryDetails = cookieCandidate?.details ?? accountResults.first?.details | ||
| let primaryDetails = accountResults.first?.details ?? cookieCandidate?.details |
There was a problem hiding this comment.
The CLI provider now prefers accountResults.first?.details over cookieCandidate?.details, which can silently discard copilotOverageCost and copilotOverageRequests whenever a token-derived account is first in priority/order. In that case JSON consumers see missing/zero add-on usage despite successful cookie billing fetches, producing inaccurate automation/reporting outputs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
this is the cleanup round we were asking for. the four shared helpers (normalizedIdentity, identityCandidates, isSameAccountUsage, shouldDropPlaceholder) are now living in a single CopilotCandidateDedupe namespace in ProviderResult.swift, the fragile authSource.contains("browser cookies") string match got replaced with a typed isPlaceholder: Bool flag set at candidate construction, and there are actual unit tests covering the dedup regression cases. all three follow-ups from the prior reviews are properly closed. solid follow-up.
the identity guard in isSameAccountUsage (guard !lhsIdentity.isEmpty, !rhsIdentity.isEmpty) is exactly the right shape — two distinct accounts that happen to sit at identical numbers (e.g. both 0/300 Pro after a billing reset) stay as separate rows, and the case-insensitive normalizedIdentity aligns with the email-first stability rule in AGENTS.md.
a few small tidiness items left, not blockers:
- the private wrappers
isDuplicateCopilotUsage/removePlaceholderCandidatesWhenRealUsageExists+ thededupeInputcomputed property are still byte-identical acrossCopilotProvider.swiftandCopilotCLIProvider.swift. the heavy logic is shared now, so this is much smaller than before, but the wrappers themselves could collapse intoCopilotCandidateDedupeto fully kill the copy-paste. shouldDropPlaceholder(_:whenAnyCandidateHasRealUsage:)re-checkshasRealUsageeven though both call sites already guard on it before calling. the parameter is redundant.- tests cover identity overlap and placeholder removal, but the plan-mismatch and used/limit-mismatch branches of
isSameAccountUsagedon't have coverage yet. easy to add.
CI is healthy: SwiftLint ✅, GitHub Actions Lint ✅, Build ✅, Build & Test in progress at review time. APPROVE.
| private func removePlaceholderCandidatesWhenRealUsageExists( | ||
| _ candidates: [CopilotAccountCandidate] | ||
| ) -> [CopilotAccountCandidate] { | ||
| let hasRealUsage = candidates.contains { candidate in | ||
| (candidate.usage.totalEntitlement ?? 0) > 0 | ||
| } | ||
| guard hasRealUsage else { return candidates } | ||
|
|
||
| return candidates.filter { candidate in | ||
| !CopilotCandidateDedupe.shouldDropPlaceholder( | ||
| candidate.dedupeInput, | ||
| whenAnyCandidateHasRealUsage: hasRealUsage | ||
| ) | ||
| } |
There was a problem hiding this comment.
the heavy lifting is already in CopilotCandidateDedupe, but this wrapper and its sibling isDuplicateCopilotUsage are now byte-identical with the ones in CopilotCLIProvider.swift:368-389. same goes for the dedupeInput computed property on CopilotAccountCandidate. consider lifting the wrappers into CopilotCandidateDedupe (e.g. static func filterPlaceholders<T>(_:dedupeInput:)) so the two provider paths can't drift again on the next bug fix.
also shouldDropPlaceholder(_:whenAnyCandidateHasRealUsage:) re-checks hasRealUsage even though both callers already early-return on it via the guard hasRealUsage else { return candidates } above. you can drop the bool parameter and let the function just answer "is this a draggable placeholder", which is what it really means.
| func testCopilotDedupeRejectsMatchingUsageWithDifferentIdentities() { | ||
| let personal = makeDedupeInput(accountId: "personal", email: "personal@example.com") | ||
| let work = makeDedupeInput(accountId: "work", email: "work@example.com") | ||
|
|
||
| XCTAssertFalse(CopilotCandidateDedupe.isSameAccountUsage(personal, work)) | ||
| } | ||
|
|
||
| func testCopilotDedupeAcceptsMatchingUsageWithCaseInsensitiveIdentity() { | ||
| let first = makeDedupeInput(accountId: "Foo", email: "Foo@Example.com") | ||
| let second = makeDedupeInput(accountId: "foo", email: "foo@example.com") | ||
|
|
||
| XCTAssertTrue(CopilotCandidateDedupe.isSameAccountUsage(first, second)) | ||
| } | ||
|
|
||
| func testCopilotDedupeRejectsIdentitylessMatchingUsage() { | ||
| let identified = makeDedupeInput(accountId: "foo", email: "foo@example.com") | ||
| let identityless = makeDedupeInput(accountId: nil, email: nil) | ||
|
|
||
| XCTAssertFalse(CopilotCandidateDedupe.isSameAccountUsage(identified, identityless)) | ||
| } | ||
|
|
||
| func testCopilotDedupeDropsOnlyPlaceholderWithoutRealUsage() { | ||
| let placeholder = makeDedupeInput( | ||
| accountId: nil, | ||
| email: nil, | ||
| entitlement: 0, | ||
| remaining: 0, | ||
| isPlaceholder: true | ||
| ) | ||
| let emptyRealCandidate = makeDedupeInput( | ||
| accountId: nil, | ||
| email: nil, | ||
| entitlement: 0, | ||
| remaining: 0, | ||
| isPlaceholder: false | ||
| ) | ||
|
|
||
| XCTAssertTrue( | ||
| CopilotCandidateDedupe.shouldDropPlaceholder( | ||
| placeholder, | ||
| whenAnyCandidateHasRealUsage: true | ||
| ) | ||
| ) | ||
| XCTAssertFalse( | ||
| CopilotCandidateDedupe.shouldDropPlaceholder( | ||
| emptyRealCandidate, | ||
| whenAnyCandidateHasRealUsage: true | ||
| ) | ||
| ) | ||
| XCTAssertFalse( | ||
| CopilotCandidateDedupe.shouldDropPlaceholder( | ||
| placeholder, | ||
| whenAnyCandidateHasRealUsage: false | ||
| ) | ||
| ) | ||
| } |
There was a problem hiding this comment.
nice that the regression cases are locked in. one gap left: isSameAccountUsage has explicit branches for plan-type mismatch (lhsPlan != rhsPlan) and used/limit-request mismatch (lhsUsed != rhsUsed, lhsLimit != rhsLimit) that nothing in this file exercises. worth a couple more cases — same accountId+email, identical entitlement/remaining, but different plan or different copilotUsedRequests — so a future refactor can't accidentally collapse those branches without a red test.
1 participant
Overview
This PR deduplicates GitHub Copilot account rows when the same account is discovered through multiple credential sources.
Previously, the same GitHub Copilot account could appear multiple times when OpenCode auth, copilot-cli Keychain auth, and browser cookies all exposed equivalent usage data.
Changes
Validation