Skip to content

Commit eb44872

Browse files
ndycodendycode
committed
fix: tighten storage identity and persistence contracts
1 parent e359d02 commit eb44872

2 files changed

Lines changed: 50 additions & 69 deletions

File tree

‎lib/storage.ts‎

Lines changed: 42 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,12 @@ export interface ImportAccountsResult {
8484
backupError?: string;
8585
}
8686

87+
export interface ImportPreviewResult {
88+
imported: number;
89+
total: number;
90+
skipped: number;
91+
}
92+
8793
/**
8894
* Custom error class for storage operations with platform-aware hints.
8995
*/
@@ -1199,31 +1205,44 @@ async function readAndNormalizeImportFile(filePath: string): Promise<{
11991205
return { resolvedPath, normalized };
12001206
}
12011207

1208+
function analyzeImportedAccounts(
1209+
existingAccounts: AccountMetadataV3[],
1210+
importedAccounts: AccountStorageV3["accounts"],
1211+
): ImportPreviewResult & { accounts: AccountMetadataV3[] } {
1212+
const merged = [...existingAccounts, ...importedAccounts];
1213+
1214+
if (merged.length > ACCOUNT_LIMITS.MAX_ACCOUNTS) {
1215+
const deduped = deduplicateAccountsForStorage(merged);
1216+
if (deduped.length > ACCOUNT_LIMITS.MAX_ACCOUNTS) {
1217+
throw new Error(
1218+
`Import would exceed maximum of ${ACCOUNT_LIMITS.MAX_ACCOUNTS} accounts (would have ${deduped.length})`,
1219+
);
1220+
}
1221+
}
1222+
1223+
const accounts = deduplicateAccountsForStorage(merged);
1224+
const imported = Math.max(0, accounts.length - existingAccounts.length);
1225+
const skipped = Math.max(0, importedAccounts.length - imported);
1226+
return {
1227+
accounts,
1228+
imported,
1229+
total: accounts.length,
1230+
skipped,
1231+
};
1232+
}
1233+
12021234
export async function previewImportAccounts(
12031235
filePath: string,
1204-
): Promise<{ imported: number; total: number; skipped: number }> {
1236+
): Promise<ImportPreviewResult> {
12051237
const { normalized } = await readAndNormalizeImportFile(filePath);
12061238

12071239
return withAccountStorageTransaction((existing) => {
12081240
const existingAccounts = existing?.accounts ?? [];
1209-
const merged = [...existingAccounts, ...normalized.accounts];
1210-
1211-
if (merged.length > ACCOUNT_LIMITS.MAX_ACCOUNTS) {
1212-
const deduped = deduplicateAccountsForStorage(merged);
1213-
if (deduped.length > ACCOUNT_LIMITS.MAX_ACCOUNTS) {
1214-
throw new Error(
1215-
`Import would exceed maximum of ${ACCOUNT_LIMITS.MAX_ACCOUNTS} accounts (would have ${deduped.length})`,
1216-
);
1217-
}
1218-
}
1219-
1220-
const deduplicatedAccounts = deduplicateAccountsForStorage(merged);
1221-
const imported = deduplicatedAccounts.length - existingAccounts.length;
1222-
const skipped = normalized.accounts.length - imported;
1241+
const analysis = analyzeImportedAccounts(existingAccounts, normalized.accounts);
12231242
return Promise.resolve({
1224-
imported,
1225-
total: deduplicatedAccounts.length,
1226-
skipped,
1243+
imported: analysis.imported,
1244+
total: analysis.total,
1245+
skipped: analysis.skipped,
12271246
});
12281247
});
12291248
}
@@ -1312,18 +1331,8 @@ export async function importAccounts(
13121331
}
13131332
}
13141333

1315-
const merged = [...existingAccounts, ...normalized.accounts];
1316-
1317-
if (merged.length > ACCOUNT_LIMITS.MAX_ACCOUNTS) {
1318-
const deduped = deduplicateAccountsForStorage(merged);
1319-
if (deduped.length > ACCOUNT_LIMITS.MAX_ACCOUNTS) {
1320-
throw new Error(
1321-
`Import would exceed maximum of ${ACCOUNT_LIMITS.MAX_ACCOUNTS} accounts (would have ${deduped.length})`
1322-
);
1323-
}
1324-
}
1325-
1326-
const deduplicatedAccounts = deduplicateAccountsForStorage(merged);
1334+
const analysis = analyzeImportedAccounts(existingAccounts, normalized.accounts);
1335+
const deduplicatedAccounts = analysis.accounts;
13271336

13281337
const mappedActiveIndex = (() => {
13291338
if (deduplicatedAccounts.length === 0) return 0;
@@ -1359,12 +1368,10 @@ export async function importAccounts(
13591368

13601369
await persist(newStorage);
13611370

1362-
const imported = deduplicatedAccounts.length - existingAccounts.length;
1363-
const skipped = normalized.accounts.length - imported;
13641371
return {
1365-
imported,
1366-
total: deduplicatedAccounts.length,
1367-
skipped,
1372+
imported: analysis.imported,
1373+
total: analysis.total,
1374+
skipped: analysis.skipped,
13681375
backupStatus,
13691376
backupPath,
13701377
backupError,

‎test/storage.test.ts‎

Lines changed: 8 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
22
import { promises as fs, existsSync } from "node:fs";
33
import { basename, dirname, join } from "node:path";
44
import { tmpdir } from "node:os";
5-
import {
5+
import {
66
deduplicateAccounts,
77
deduplicateAccountsByEmail,
88
normalizeAccountStorage,
@@ -25,11 +25,6 @@ import {
2525
withFlaggedAccountStorageTransaction,
2626
} from "../lib/storage.js";
2727

28-
// Mocking the behavior we're about to implement for TDD
29-
// Since the functions aren't in lib/storage.ts yet, we'll need to mock them or
30-
// accept that this test won't even compile/run until we add them.
31-
// But Task 0 says: "Tests should fail initially (RED phase)"
32-
3328
describe("storage", () => {
3429
describe("getWorkspaceIdentityKey", () => {
3530
it("uses organizationId and accountId when both are present", () => {
@@ -141,56 +136,43 @@ describe("storage", () => {
141136
});
142137

143138
it("should export accounts to a file", async () => {
144-
// @ts-ignore - exportAccounts doesn't exist yet
145-
const { exportAccounts } = await import("../lib/storage.js");
146-
147139
const storage = {
148140
version: 3,
149141
activeIndex: 0,
150142
accounts: [{ accountId: "test", refreshToken: "ref", addedAt: 1, lastUsed: 2 }]
151143
};
152-
// @ts-ignore
153144
await saveAccounts(storage);
154-
155-
// @ts-ignore
145+
156146
await exportAccounts(exportPath);
157-
147+
158148
expect(existsSync(exportPath)).toBe(true);
159149
const exported = JSON.parse(await fs.readFile(exportPath, "utf-8"));
160150
expect(exported.accounts[0].accountId).toBe("test");
161151
});
162152

163153
it("should fail export if file exists and force is false", async () => {
164-
// @ts-ignore
165-
const { exportAccounts } = await import("../lib/storage.js");
166154
await fs.writeFile(exportPath, "exists");
167-
168-
// @ts-ignore
155+
169156
await expect(exportAccounts(exportPath, false)).rejects.toThrow(/already exists/);
170157
});
171158

172159
it("should import accounts from a file and merge", async () => {
173-
// @ts-ignore
174-
const { importAccounts } = await import("../lib/storage.js");
175-
176160
const existing = {
177161
version: 3,
178162
activeIndex: 0,
179163
accounts: [{ accountId: "existing", refreshToken: "ref1", addedAt: 1, lastUsed: 2 }]
180164
};
181-
// @ts-ignore
182165
await saveAccounts(existing);
183-
166+
184167
const toImport = {
185168
version: 3,
186169
activeIndex: 0,
187170
accounts: [{ accountId: "new", refreshToken: "ref2", addedAt: 3, lastUsed: 4 }]
188171
};
189172
await fs.writeFile(exportPath, JSON.stringify(toImport));
190-
191-
// @ts-ignore
173+
192174
await importAccounts(exportPath);
193-
175+
194176
const loaded = await loadAccounts();
195177
expect(loaded?.accounts).toHaveLength(2);
196178
expect(loaded?.accounts.map(a => a.accountId)).toContain("new");
@@ -625,9 +607,6 @@ describe("storage", () => {
625607
});
626608

627609
it("should enforce MAX_ACCOUNTS during import", async () => {
628-
// @ts-ignore
629-
const { importAccounts } = await import("../lib/storage.js");
630-
631610
const manyAccounts = Array.from({ length: 21 }, (_, i) => ({
632611
accountId: `acct${i}`,
633612
refreshToken: `ref${i}`,
@@ -641,31 +620,26 @@ describe("storage", () => {
641620
accounts: manyAccounts
642621
};
643622
await fs.writeFile(exportPath, JSON.stringify(toImport));
644-
645-
// @ts-ignore
623+
646624
await expect(importAccounts(exportPath)).rejects.toThrow(/exceed maximum/);
647625
});
648626

649627
it("should fail export when no accounts exist", async () => {
650-
const { exportAccounts } = await import("../lib/storage.js");
651628
setStoragePathDirect(testStoragePath);
652629
await expect(exportAccounts(exportPath)).rejects.toThrow(/No accounts to export/);
653630
});
654631

655632
it("should fail import when file does not exist", async () => {
656-
const { importAccounts } = await import("../lib/storage.js");
657633
const nonexistentPath = join(testWorkDir, "nonexistent-file.json");
658634
await expect(importAccounts(nonexistentPath)).rejects.toThrow(/Import file not found/);
659635
});
660636

661637
it("should fail import when file contains invalid JSON", async () => {
662-
const { importAccounts } = await import("../lib/storage.js");
663638
await fs.writeFile(exportPath, "not valid json {[");
664639
await expect(importAccounts(exportPath)).rejects.toThrow(/Invalid JSON/);
665640
});
666641

667642
it("should fail import when file contains invalid format", async () => {
668-
const { importAccounts } = await import("../lib/storage.js");
669643
await fs.writeFile(exportPath, JSON.stringify({ invalid: "format" }));
670644
await expect(importAccounts(exportPath)).rejects.toThrow(/Invalid account storage format/);
671645
});

0 commit comments

Comments
 (0)