diff --git a/src/renderer/src/store/slices/repo-host-identity.ts b/src/renderer/src/store/slices/repo-host-identity.ts index f21da2b153a..772d5dc2925 100644 --- a/src/renderer/src/store/slices/repo-host-identity.ts +++ b/src/renderer/src/store/slices/repo-host-identity.ts @@ -4,6 +4,9 @@ import { getSettingsFocusedExecutionHostId, type ExecutionHostId } from '../../../../shared/execution-host' +import { getRepoHostIdentityForParts } from '../../../../shared/repo-host-identity' + +export { getRepoHostIdentityForParts } type RepoIdentityParts = Pick @@ -11,12 +14,6 @@ export function getRepoHostIdentity(repo: RepoIdentityParts): string { return getRepoHostIdentityForParts(repo.id, getRepoExecutionHostId(repo)) } -export function getRepoHostIdentityForParts(repoId: string, hostId: string): string { - // Why: host ids and repo ids can contain punctuation; NUL keeps the composite - // key collision-free without escaping user/provider-owned strings. - return `${hostId}\0${repoId}` -} - export function repoMatchesHostIdentity( repo: RepoIdentityParts, repoId: string, diff --git a/src/renderer/src/store/slices/repo-identity-reconcile.ts b/src/renderer/src/store/slices/repo-identity-reconcile.ts index db59e619d97..7535422018a 100644 --- a/src/renderer/src/store/slices/repo-identity-reconcile.ts +++ b/src/renderer/src/store/slices/repo-identity-reconcile.ts @@ -1,4 +1,5 @@ import type { Repo } from '../../../../shared/types' +import { structuralValuesEqual } from '../../../../shared/structural-value-equality' import { getRepoHostIdentity } from './repo-host-identity' // Why: after a drag-reorder we optimistically set `repos`, persist, and main @@ -8,44 +9,9 @@ import { getRepoHostIdentity } from './repo-host-identity' // virtualizer to rebuild + re-measure a tick after the drop — the visible jump. // Reusing equal objects (and the whole array when nothing moved) makes the echo // a no-op render. -// Why: `Repo` carries nested records (hookSettings, upstream, gitRemoteIdentity, repoIcon, path -// arrays). IPC structured-clone rebuilds those every fetch, and main's hydrateRepo always -// reconstructs hookSettings — so a reference compare reports every repo as changed and no repo -// ever reconciles. Compare nested plain values structurally; they are small sanitized records. -export function areValuesEqual(a: unknown, b: unknown): boolean { - if (a === b) { - return true - } - if (typeof a !== 'object' || typeof b !== 'object' || a === null || b === null) { - return false - } - if (Array.isArray(a) || Array.isArray(b)) { - return ( - Array.isArray(a) && - Array.isArray(b) && - a.length === b.length && - a.every((item, index) => areValuesEqual(item, b[index])) - ) - } - // Why: only plain records are safe to walk — anything exotic falls back to reference equality. - const aPrototype = Object.getPrototypeOf(a) - const bPrototype = Object.getPrototypeOf(b) - if ( - (aPrototype !== Object.prototype && aPrototype !== null) || - (bPrototype !== Object.prototype && bPrototype !== null) - ) { - return false - } - const aRecord = a as Record - const bRecord = b as Record - const keys = Object.keys(aRecord) - if (keys.length !== Object.keys(bRecord).length) { - return false - } - return keys.every( - (key) => Object.hasOwn(bRecord, key) && areValuesEqual(aRecord[key], bRecord[key]) - ) -} +// Why the structural compare: `Repo` carries nested records (hookSettings, upstream, +// gitRemoteIdentity, repoIcon, path arrays) that structured-clone and main's hydrateRepo rebuild +// every fetch, so a reference compare would report every repo as changed. /** * Reuses equal rows from `previous` — and the whole array when nothing moved — so a refetch that @@ -61,7 +27,7 @@ export function reconcileCatalogRows( let identical = next.length === previous.length const reconciled = next.map((row, index) => { const existing = previousByIdentity.get(getIdentity(row)) - if (existing !== undefined && areValuesEqual(existing, row)) { + if (existing !== undefined && structuralValuesEqual(existing, row)) { if (existing !== previous[index]) { identical = false } diff --git a/src/renderer/src/store/slices/repos.ts b/src/renderer/src/store/slices/repos.ts index 059baa6dde5..0b8f0dfcb68 100644 --- a/src/renderer/src/store/slices/repos.ts +++ b/src/renderer/src/store/slices/repos.ts @@ -48,12 +48,9 @@ import { applyManualRepoOrder, getManualRepoOrder } from '../../../../shared/man import { getProjectGroupSubtreeIds } from '../../../../shared/project-groups' import { isPathInsideOrEqual } from '../../../../shared/cross-platform-path' import { getRepoIdFromWorktreeId } from '../../../../shared/worktree/id' +import { structuralValuesEqual } from '../../../../shared/structural-value-equality' import { selectProjectGroupRemovalTargets } from './project-group-removal-targets' -import { - areValuesEqual, - reconcileCatalogRows, - reconcileFetchedRepos -} from './repo-identity-reconcile' +import { reconcileCatalogRows, reconcileFetchedRepos } from './repo-identity-reconcile' import { retainValidFilterRepoIds } from './repo-filter-selection' import { mergeSshRepoReadoptions, @@ -1022,7 +1019,7 @@ function mergeByIdentity( changed = true continue } - if (areValuesEqual(merged[index], entry)) { + if (structuralValuesEqual(merged[index], entry)) { continue } merged[index] = entry @@ -1043,7 +1040,8 @@ function unchangedMergeSource( // Why: the sidebar effect watching these catalog arrays is the only thing that refills the // folder path-status cache, so a no-op refetch must not wipe it — nothing would repopulate it. -function catalogRowsUnchanged(next: readonly T[], previous: readonly T[]): boolean { +// Element identity only; `structuralValuesEqual` is the structural counterpart. +function arrayElementsUnchanged(next: readonly T[], previous: readonly T[]): boolean { return ( next === previous || (next.length === previous.length && next.every((row, index) => row === previous[index])) @@ -1061,19 +1059,10 @@ function mergeFetchedReposForHost( const existingHostId = getRepoExecutionHostId(repo) return existingHostId !== hostId || fetchedIdentities.has(getRepoHostIdentity(repo)) }) - const merged = [...preserved] - const indexByIdentity = new Map(merged.map((repo, index) => [getRepoHostIdentity(repo), index])) - for (const repo of fetchedWithProjectGroups) { - const identity = getRepoHostIdentity(repo) - const existingIndex = indexByIdentity.get(identity) - if (existingIndex === undefined) { - indexByIdentity.set(identity, merged.length) - merged.push(repo) - continue - } - merged[existingIndex] = repo - } - return reconcileFetchedRepos(previous, merged) + return reconcileFetchedRepos( + previous, + mergeByIdentity(preserved, fetchedWithProjectGroups, getRepoHostIdentity) + ) } function applyInheritedProjectGroups(previous: readonly Repo[], fetched: readonly Repo[]): Repo[] { @@ -2135,7 +2124,7 @@ export const createRepoSlice: StateCreator = (set, pendingSshRepoReadoptions: reconciliation.pendingReadoptions, ...reconcileReadoptedSshWorktreeState(s, s.pendingSshRepoReadoptions), ...mergedProjectCompatibility, - ...(catalogRowsUnchanged(prunedRepos, s.repos) + ...(arrayElementsUnchanged(prunedRepos, s.repos) ? {} : { folderWorkspacePathStatuses: {} }), activeRepoId: s.activeRepoId && validRepoIds.has(s.activeRepoId) ? s.activeRepoId : null, @@ -2288,7 +2277,7 @@ export const createRepoSlice: StateCreator = (set, pendingSshRepoReadoptions: reconciliation.pendingReadoptions, ...reconcileReadoptedSshWorktreeState(s, s.pendingSshRepoReadoptions), ...mergedProjectCompatibility, - ...(catalogRowsUnchanged(finalizedRepos, s.repos) + ...(arrayElementsUnchanged(finalizedRepos, s.repos) ? {} : { folderWorkspacePathStatuses: {} }), activeRepoId: s.activeRepoId, @@ -2377,7 +2366,7 @@ export const createRepoSlice: StateCreator = (set, const { projectGroups } = mergeFetchedProjectGroupCatalog(catalog, current.projectGroups) return { projectGroups, - ...(catalogRowsUnchanged(projectGroups, current.projectGroups) + ...(arrayElementsUnchanged(projectGroups, current.projectGroups) ? {} : { folderWorkspacePathStatuses: {} }) } @@ -2400,7 +2389,7 @@ export const createRepoSlice: StateCreator = (set, const { projectGroups } = mergeFetchedProjectGroupCatalog(catalog, s.projectGroups) return { projectGroups, - ...(catalogRowsUnchanged(projectGroups, s.projectGroups) + ...(arrayElementsUnchanged(projectGroups, s.projectGroups) ? {} : { folderWorkspacePathStatuses: {} }) } @@ -2464,7 +2453,7 @@ export const createRepoSlice: StateCreator = (set, ) return { folderWorkspaces, - ...(catalogRowsUnchanged(folderWorkspaces, current.folderWorkspaces) + ...(arrayElementsUnchanged(folderWorkspaces, current.folderWorkspaces) ? {} : { folderWorkspacePathStatuses: {} }) } @@ -2502,7 +2491,7 @@ export const createRepoSlice: StateCreator = (set, ) return { folderWorkspaces, - ...(catalogRowsUnchanged(folderWorkspaces, current.folderWorkspaces) + ...(arrayElementsUnchanged(folderWorkspaces, current.folderWorkspaces) ? {} : { folderWorkspacePathStatuses: {} }) } diff --git a/src/renderer/src/store/slices/superseded-ssh-repo-rows.ts b/src/renderer/src/store/slices/superseded-ssh-repo-rows.ts index c18e0f6e5af..a9880577f60 100644 --- a/src/renderer/src/store/slices/superseded-ssh-repo-rows.ts +++ b/src/renderer/src/store/slices/superseded-ssh-repo-rows.ts @@ -1,6 +1,7 @@ import type { SshRepoReadoption } from '../../../../shared/ssh-types' import type { Repo } from '../../../../shared/types' import { getRepoExecutionHostId, toSshExecutionHostId } from '../../../../shared/execution-host' +import { getRepoHostIdentityForParts } from '../../../../shared/repo-host-identity' export type SshRepoReconciliation = { repos: readonly Repo[] @@ -14,10 +15,6 @@ function repoBelongsToTarget(repo: Repo, targetId: string): boolean { ) } -function repoOwnerKey(hostId: string, repoId: string): string { - return `${hostId}\0${repoId}` -} - /** * Drops old-host rows only when main reports the exact repo re-adoption and the * renderer has received the corresponding new-host row. Evidence stays pending @@ -32,7 +29,7 @@ export function reconcileReadoptedSshRepoRows( const directSshOwners = new Set( repos.flatMap((repo) => repo.connectionId && repoBelongsToTarget(repo, repo.connectionId) - ? [repoOwnerKey(getRepoExecutionHostId(repo), repo.id)] + ? [getRepoHostIdentityForParts(repo.id, getRepoExecutionHostId(repo))] : [] ) ) @@ -40,13 +37,18 @@ export function reconcileReadoptedSshRepoRows( for (const readoption of readoptions) { const pendingRepoIds: string[] = [] for (const repoId of readoption.repoIds) { - const newOwner = repoOwnerKey(toSshExecutionHostId(readoption.newTargetId), repoId) + const newOwner = getRepoHostIdentityForParts( + repoId, + toSshExecutionHostId(readoption.newTargetId) + ) const hasNewRow = directSshOwners.has(newOwner) if (!hasNewRow) { pendingRepoIds.push(repoId) continue } - prunedOwners.add(repoOwnerKey(toSshExecutionHostId(readoption.oldTargetId), repoId)) + prunedOwners.add( + getRepoHostIdentityForParts(repoId, toSshExecutionHostId(readoption.oldTargetId)) + ) } if (pendingRepoIds.length > 0) { pendingReadoptions.push({ ...readoption, repoIds: pendingRepoIds }) @@ -60,7 +62,8 @@ export function reconcileReadoptedSshRepoRows( } return { repos: repos.filter( - (repo) => !prunedOwners.has(repoOwnerKey(getRepoExecutionHostId(repo), repo.id)) + (repo) => + !prunedOwners.has(getRepoHostIdentityForParts(repo.id, getRepoExecutionHostId(repo))) ), pendingReadoptions } diff --git a/src/renderer/src/store/slices/worktree-catalog-reconciliation.ts b/src/renderer/src/store/slices/worktree-catalog-reconciliation.ts index 1ef339a1808..500f002dc59 100644 --- a/src/renderer/src/store/slices/worktree-catalog-reconciliation.ts +++ b/src/renderer/src/store/slices/worktree-catalog-reconciliation.ts @@ -1,41 +1,14 @@ +import { structuralValuesEqualIgnoringUndefined } from '../../../../shared/structural-value-equality' + type CatalogRow = { id: string } -function isPlainRecord(value: unknown): value is Record { - if (typeof value !== 'object' || value === null) { - return false - } - const prototype = Object.getPrototypeOf(value) - return prototype === Object.prototype || prototype === null -} - -function catalogValuesEqual(left: unknown, right: unknown): boolean { - if (Object.is(left, right)) { - return true - } - if (Array.isArray(left) || Array.isArray(right)) { - if (!Array.isArray(left) || !Array.isArray(right) || left.length !== right.length) { - return false - } - return left.every((value, index) => catalogValuesEqual(value, right[index])) - } - if (!isPlainRecord(left) || !isPlainRecord(right)) { - return false - } - const keys = new Set([...Object.keys(left), ...Object.keys(right)]) - for (const key of keys) { - if (!catalogValuesEqual(left[key], right[key])) { - return false - } - } - return true -} - // NOTHING HITS THIS TODAY: both callers key on ids that are unique by // construction, so buckets stay at 1-3. It only bounds the damage if that ever // changes — a same-id bucket costs one deep compare per candidate, so an -// unbounded one is O(k^2). A cap beats a second index that would have to stay in -// step with catalogValuesEqual, and reuse is only an optimization: dropping a -// match past the window costs object identity, never correctness. +// unbounded one is O(k^2). A cap beats an index keyed on a second walk that +// would have to stay in step with structuralValuesEqualIgnoringUndefined, and +// reuse is only an optimization: dropping a match past the window costs object +// identity, never correctness. const MAX_DUPLICATE_ID_SCAN = 8 export function reuseEqualCatalogRows( @@ -62,7 +35,7 @@ export function reuseEqualCatalogRows( const scanLimit = Math.min(candidates.length, MAX_DUPLICATE_ID_SCAN) for (let index = 0; index < scanLimit; index++) { const candidate = candidates[index] - if (candidate !== undefined && catalogValuesEqual(candidate, row)) { + if (candidate !== undefined && structuralValuesEqualIgnoringUndefined(candidate, row)) { candidates.splice(index, 1) return candidate } diff --git a/src/shared/manual-repo-order.ts b/src/shared/manual-repo-order.ts index 9b5bfee39a1..9813412c676 100644 --- a/src/shared/manual-repo-order.ts +++ b/src/shared/manual-repo-order.ts @@ -1,8 +1,9 @@ import { getRepoExecutionHostId, normalizeExecutionHostId } from './execution-host' +import { getRepoHostIdentityForParts } from './repo-host-identity' import type { ManualRepoOrderEntry, Repo } from './types' function getEntryKey(entry: ManualRepoOrderEntry): string { - return `${entry.hostId}\0${entry.repoId}` + return getRepoHostIdentityForParts(entry.repoId, entry.hostId) } export function normalizeManualRepoOrder(value: unknown): ManualRepoOrderEntry[] { diff --git a/src/shared/repo-host-identity.ts b/src/shared/repo-host-identity.ts new file mode 100644 index 00000000000..8309899d4e6 --- /dev/null +++ b/src/shared/repo-host-identity.ts @@ -0,0 +1,5 @@ +export function getRepoHostIdentityForParts(repoId: string, hostId: string): string { + // Why: host ids and repo ids can contain punctuation; NUL keeps the composite + // key collision-free without escaping user/provider-owned strings. + return `${hostId}\0${repoId}` +} diff --git a/src/shared/structural-value-equality.test.ts b/src/shared/structural-value-equality.test.ts new file mode 100644 index 00000000000..57a7a471dd5 --- /dev/null +++ b/src/shared/structural-value-equality.test.ts @@ -0,0 +1,96 @@ +import { describe, expect, it } from 'vitest' +import { + structuralValuesEqual, + structuralValuesEqualIgnoringUndefined +} from './structural-value-equality' + +const comparators = [ + ['structuralValuesEqual', structuralValuesEqual], + ['structuralValuesEqualIgnoringUndefined', structuralValuesEqualIgnoringUndefined] +] as const + +describe.each(comparators)('%s', (_name, valuesEqual) => { + it('compares primitives and identical references', () => { + expect(valuesEqual('a', 'a')).toBe(true) + expect(valuesEqual('a', 'b')).toBe(false) + expect(valuesEqual(null, null)).toBe(true) + expect(valuesEqual(null, {})).toBe(false) + expect(valuesEqual(undefined, null)).toBe(false) + const fn = (): void => {} + expect(valuesEqual(fn, fn)).toBe(true) + expect(valuesEqual(fn, (): void => {})).toBe(false) + }) + + it('walks arrays element-wise and never mixes them with records', () => { + expect(valuesEqual([1, [2, { a: 'b' }]], [1, [2, { a: 'b' }]])).toBe(true) + expect(valuesEqual([1, 2], [1, 2, 3])).toBe(false) + expect(valuesEqual([1, 2], [2, 1])).toBe(false) + expect(valuesEqual([], {})).toBe(false) + expect(valuesEqual({ 0: 1, length: 1 }, [1])).toBe(false) + }) + + it('walks nested plain records rebuilt by structured clone', () => { + const record = { id: 'a', nested: { labels: ['one', 'two'], flags: { on: true } } } + expect(valuesEqual(record, structuredClone(record))).toBe(true) + expect( + valuesEqual(record, { ...record, nested: { labels: ['one'], flags: { on: true } } }) + ).toBe(false) + }) + + it('walks null-prototype records like literals', () => { + const nullProto: Record = Object.create(null) + nullProto.a = 1 + expect(valuesEqual(nullProto, { a: 1 })).toBe(true) + expect(valuesEqual(nullProto, { a: 2 })).toBe(false) + }) + + it('falls back to reference equality for non-plain objects', () => { + const date = new Date(0) + expect(valuesEqual(date, date)).toBe(true) + expect(valuesEqual(date, new Date(0))).toBe(false) + expect(valuesEqual(new Map([['a', 1]]), new Map([['a', 1]]))).toBe(false) + expect(valuesEqual(new Set([1]), new Set([1]))).toBe(false) + class Row { + x = 1 + } + expect(valuesEqual(new Row(), new Row())).toBe(false) + expect(valuesEqual(new Row(), { x: 1 })).toBe(false) + }) + + it('ignores symbol keys', () => { + const key = Symbol('marker') + expect(valuesEqual({ [key]: 1, a: 1 }, { [key]: 2, a: 1 })).toBe(true) + }) +}) + +describe('structuralValuesEqual', () => { + it('treats an absent key as different from a key holding undefined', () => { + // Why: repo/project merges branch on `'key' in project`, so key presence is load-bearing. + expect(structuralValuesEqual({ a: 1 }, { a: 1, b: undefined })).toBe(false) + expect(structuralValuesEqual({ a: 1, b: undefined }, { a: 1 })).toBe(false) + expect(structuralValuesEqual({ a: 1, b: undefined }, { a: 1, c: undefined })).toBe(false) + expect(structuralValuesEqual({ a: 1, b: undefined }, { a: 1, b: undefined })).toBe(true) + }) + + it('compares leaves with === so NaN is never equal and -0 matches 0', () => { + expect(structuralValuesEqual({ a: Number.NaN }, { a: Number.NaN })).toBe(false) + expect(structuralValuesEqual({ a: 0 }, { a: -0 })).toBe(true) + }) +}) + +describe('structuralValuesEqualIgnoringUndefined', () => { + it('treats an absent key as equal to a key holding undefined', () => { + // Why: locally built worktree rows carry explicit undefined fields the host omits. + expect(structuralValuesEqualIgnoringUndefined({ a: 1 }, { a: 1, b: undefined })).toBe(true) + expect(structuralValuesEqualIgnoringUndefined({ a: 1, b: undefined }, { a: 1 })).toBe(true) + expect( + structuralValuesEqualIgnoringUndefined({ a: 1, b: undefined }, { a: 1, c: undefined }) + ).toBe(true) + expect(structuralValuesEqualIgnoringUndefined({ a: 1 }, { a: 1, b: 2 })).toBe(false) + }) + + it('compares leaves with Object.is so NaN is equal and -0 differs from 0', () => { + expect(structuralValuesEqualIgnoringUndefined({ a: Number.NaN }, { a: Number.NaN })).toBe(true) + expect(structuralValuesEqualIgnoringUndefined({ a: 0 }, { a: -0 })).toBe(false) + }) +}) diff --git a/src/shared/structural-value-equality.ts b/src/shared/structural-value-equality.ts new file mode 100644 index 00000000000..5714bb7f1ed --- /dev/null +++ b/src/shared/structural-value-equality.ts @@ -0,0 +1,82 @@ +// Why: catalog reconcilers compare rows that IPC structured-clone (and main's hydration) rebuild on +// every fetch, so a reference compare reports every row as changed and nothing ever reconciles. +// Only plain records and arrays are walked; anything exotic (Date, Map, class instance) falls back +// to reference equality rather than being mistaken for an empty record. + +type ValueEqualityPolicy = { + // Why: `Object.is` makes NaN equal NaN but 0 unequal -0; `===` does the opposite. + readonly sameValueLeaves: boolean + readonly absentKeyEqualsUndefined: boolean +} + +const STRICT_OWN_KEYS: ValueEqualityPolicy = { + sameValueLeaves: false, + absentKeyEqualsUndefined: false +} + +const UNION_OF_KEYS: ValueEqualityPolicy = { + sameValueLeaves: true, + absentKeyEqualsUndefined: true +} + +function isPlainRecord(value: unknown): value is Record { + if (typeof value !== 'object' || value === null) { + return false + } + const prototype = Object.getPrototypeOf(value) + return prototype === Object.prototype || prototype === null +} + +function valuesEqual(a: unknown, b: unknown, policy: ValueEqualityPolicy): boolean { + if (policy.sameValueLeaves ? Object.is(a, b) : a === b) { + return true + } + if (Array.isArray(a) || Array.isArray(b)) { + return ( + Array.isArray(a) && + Array.isArray(b) && + a.length === b.length && + a.every((item, index) => valuesEqual(item, b[index], policy)) + ) + } + if (!isPlainRecord(a) || !isPlainRecord(b)) { + return false + } + if (policy.absentKeyEqualsUndefined) { + for (const key of new Set([...Object.keys(a), ...Object.keys(b)])) { + if (!valuesEqual(a[key], b[key], policy)) { + return false + } + } + return true + } + const keys = Object.keys(a) + if (keys.length !== Object.keys(b).length) { + return false + } + return keys.every( + (key) => Object.hasOwn(b, key) && valuesEqual(a[key], b[key], policy) + ) +} + +/** + * Structural compare where an absent own key differs from a key that is present and holds + * `undefined`, and leaves compare with `===`. + * + * Why the strict key set: the repo/project merges branch on `'localWindowsRuntimePreference' in + * project`, so a key appearing or disappearing is a real change even when its value is `undefined`. + */ +export function structuralValuesEqual(a: unknown, b: unknown): boolean { + return valuesEqual(a, b, STRICT_OWN_KEYS) +} + +/** + * Structural compare that treats an absent own key as equal to a key holding `undefined`, and + * compares leaves with `Object.is`. + * + * Why the loose key set: locally constructed worktree catalog rows carry explicit `undefined` + * fields that the host simply omits, and no consumer of those rows uses the `in` operator. + */ +export function structuralValuesEqualIgnoringUndefined(a: unknown, b: unknown): boolean { + return valuesEqual(a, b, UNION_OF_KEYS) +}