fix(v030): SyncConflict noteId→path + populate localText/remoteText (final review fix)
final code review (Opus) 발견 2 important issues: 1. SyncConflict.noteId 가 실제로 export filename slug (date-id8-slug) 였음 — UUID 가 아니라 git checkout path 의 stem. 명명 혼동 → 'path' 로 rename (실제 의미와 일치). 2. ConflictModal preview 가 항상 빈 문자열이라 사용자가 비교 없이 local/remote 선택해야 했음. runSync 의 conflict 분기에서 `git show :2:<path>` (ours) + `:3:<path>` (theirs) 호출 추가하여 localText/remoteText 채움. 영향: - SyncService.SyncConflict + shared/types.ts.SyncConflict: noteId → path - SyncService.resolveConflict(path, choice) — 'notes/...md' 그대로 받음 - pathToNoteId 헬퍼 제거 (불필요) - ConflictModal: c.noteId → c.path, busy 상태 + 표시 모두 path 키 - IPC handler / preload bridge / InboxApi 시그니처 모두 path 로 통일 - SyncService.bidirectional/resolveConflict/sync-ipc/ConflictModal 4 test 갱신 regression 회귀 패턴 검사: rename 후 NoteRepository / SyncService / IPC / UI 의 모든 conflict-related path 일관 (typecheck 0).
This commit is contained in:
altair823
@@ -20,32 +20,34 @@ describe('ConflictModal', () => {
|
||||
vi.clearAllMocks();
|
||||
cleanup();
|
||||
mockListConflicts.mockResolvedValue([
|
||||
{ noteId: 'n1', localText: 'local A', remoteText: 'remote A' },
|
||||
{ noteId: 'n2', localText: 'local B', remoteText: 'remote B' }
|
||||
{ path: 'notes/n1.md', localText: 'local A', remoteText: 'remote A' },
|
||||
{ path: 'notes/n2.md', localText: 'local B', remoteText: 'remote B' }
|
||||
]);
|
||||
mockResolveConflict.mockResolvedValue({ ok: true });
|
||||
});
|
||||
|
||||
it('open 시 listConflicts 호출 + 양 conflict 표시', async () => {
|
||||
it('open 시 listConflicts 호출 + 양 conflict preview 표시', async () => {
|
||||
render(<ConflictModal onClose={() => {}} onResolved={() => {}} />);
|
||||
await waitFor(() => screen.getByText(/local A/));
|
||||
expect(screen.getByText(/local A/)).toBeInTheDocument();
|
||||
expect(screen.getByText(/remote A/)).toBeInTheDocument();
|
||||
expect(screen.getByText(/local B/)).toBeInTheDocument();
|
||||
// path 가 표시됨 (Cut E final review fix — noteId → path)
|
||||
expect(screen.getByText('notes/n1.md')).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it('내 것 사용 클릭 → resolveConflict(noteId, "local") 호출', async () => {
|
||||
it('내 것 사용 클릭 → resolveConflict(path, "local") 호출', async () => {
|
||||
render(<ConflictModal onClose={() => {}} onResolved={() => {}} />);
|
||||
await waitFor(() => screen.getByText(/local A/));
|
||||
const buttons = screen.getAllByRole('button', { name: /내 것 사용/ });
|
||||
fireEvent.click(buttons[0]!);
|
||||
await waitFor(() => {
|
||||
expect(mockResolveConflict).toHaveBeenCalledWith('n1', 'local');
|
||||
expect(mockResolveConflict).toHaveBeenCalledWith('notes/n1.md', 'local');
|
||||
});
|
||||
});
|
||||
|
||||
it('마지막 conflict 해결 → onResolved + onClose 호출', async () => {
|
||||
mockListConflicts.mockResolvedValueOnce([{ noteId: 'n1', localText: 'a', remoteText: 'b' }]);
|
||||
mockListConflicts.mockResolvedValueOnce([{ path: 'notes/n1.md', localText: 'a', remoteText: 'b' }]);
|
||||
const onResolved = vi.fn();
|
||||
const onClose = vi.fn();
|
||||
render(<ConflictModal onClose={onClose} onResolved={onResolved} />);
|
||||
|
||||
@@ -20,6 +20,7 @@ describe('SyncService.sync — 양방향', () => {
|
||||
rebaseAbort: ReturnType<typeof vi.fn>;
|
||||
listConflicts: ReturnType<typeof vi.fn>;
|
||||
push: ReturnType<typeof vi.fn>;
|
||||
run: ReturnType<typeof vi.fn>;
|
||||
};
|
||||
|
||||
beforeEach(() => {
|
||||
@@ -36,7 +37,8 @@ describe('SyncService.sync — 양방향', () => {
|
||||
rebaseOnto: vi.fn(async () => ({ stdout: '', stderr: '', exitCode: 0 })),
|
||||
rebaseAbort: vi.fn(async () => ({ stdout: '', stderr: '', exitCode: 0 })),
|
||||
listConflicts: vi.fn(async () => []),
|
||||
push: vi.fn(async () => {})
|
||||
push: vi.fn(async () => {}),
|
||||
run: vi.fn(async () => ({ stdout: '', stderr: '', exitCode: 0 }))
|
||||
};
|
||||
(GitClient as unknown as ReturnType<typeof vi.fn>).mockImplementation(function () { return gitInstance; });
|
||||
svc = new SyncService(
|
||||
@@ -67,14 +69,24 @@ describe('SyncService.sync — 양방향', () => {
|
||||
expect(r.ok).toBe(true);
|
||||
});
|
||||
|
||||
it('rebase 실패 → abort + reason=conflict + conflicts 포함', async () => {
|
||||
it('rebase 실패 → abort + reason=conflict + conflicts 포함 (path + localText/remoteText)', async () => {
|
||||
gitInstance.rebaseOnto.mockResolvedValueOnce({ stdout: '', stderr: 'CONFLICT', exitCode: 1 });
|
||||
gitInstance.listConflicts.mockResolvedValueOnce(['notes/aaa.md', 'notes/bbb.md']);
|
||||
// Cut E final review fix — runSync calls git.run(['show', ':2:path']) and ':3:path'
|
||||
// for each conflict. Mock returns ours/theirs text per call.
|
||||
gitInstance.run
|
||||
.mockResolvedValueOnce({ stdout: 'aaa local', stderr: '', exitCode: 0 }) // :2:notes/aaa.md
|
||||
.mockResolvedValueOnce({ stdout: 'aaa remote', stderr: '', exitCode: 0 }) // :3:notes/aaa.md
|
||||
.mockResolvedValueOnce({ stdout: 'bbb local', stderr: '', exitCode: 0 }) // :2:notes/bbb.md
|
||||
.mockResolvedValueOnce({ stdout: 'bbb remote', stderr: '', exitCode: 0 }); // :3:notes/bbb.md
|
||||
const r = await svc.sync();
|
||||
expect(gitInstance.rebaseAbort).toHaveBeenCalled();
|
||||
expect(r.ok).toBe(false);
|
||||
expect(r.reason).toBe('conflict');
|
||||
expect(r.conflicts?.length).toBe(2);
|
||||
expect(r.conflicts).toEqual([
|
||||
{ path: 'notes/aaa.md', localText: 'aaa local', remoteText: 'aaa remote' },
|
||||
{ path: 'notes/bbb.md', localText: 'bbb local', remoteText: 'bbb remote' }
|
||||
]);
|
||||
expect(gitInstance.push).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
|
||||
@@ -25,7 +25,7 @@ describe('SyncService.resolveConflict', () => {
|
||||
});
|
||||
|
||||
it('local 선택 → checkout --ours + add + rebase --continue + push', async () => {
|
||||
const r = await svc.resolveConflict('note-id', 'local');
|
||||
const r = await svc.resolveConflict('notes/note-id.md', 'local');
|
||||
expect(gitInstance.run).toHaveBeenCalledWith(['checkout', '--ours', 'notes/note-id.md']);
|
||||
expect(gitInstance.run).toHaveBeenCalledWith(['rebase', '--continue']);
|
||||
expect(gitInstance.push).toHaveBeenCalled();
|
||||
@@ -33,7 +33,7 @@ describe('SyncService.resolveConflict', () => {
|
||||
});
|
||||
|
||||
it('remote 선택 → checkout --theirs + add + rebase --continue + applySyncFromDir + push', async () => {
|
||||
const r = await svc.resolveConflict('note-id', 'remote');
|
||||
const r = await svc.resolveConflict('notes/note-id.md', 'remote');
|
||||
expect(gitInstance.run).toHaveBeenCalledWith(['checkout', '--theirs', 'notes/note-id.md']);
|
||||
expect(importSvc.applySyncFromDir).toHaveBeenCalled();
|
||||
expect(gitInstance.push).toHaveBeenCalled();
|
||||
@@ -42,7 +42,7 @@ describe('SyncService.resolveConflict', () => {
|
||||
|
||||
it('checkout 실패 → ok:false + reason 반환', async () => {
|
||||
gitInstance.run.mockResolvedValueOnce({ stdout: '', stderr: 'fail', exitCode: 1 });
|
||||
const r = await svc.resolveConflict('note-id', 'local');
|
||||
const r = await svc.resolveConflict('notes/note-id.md', 'local');
|
||||
expect(r.ok).toBe(false);
|
||||
expect((r as { reason: string }).reason).toContain('checkout failed');
|
||||
expect(gitInstance.push).not.toHaveBeenCalled();
|
||||
@@ -52,7 +52,7 @@ describe('SyncService.resolveConflict', () => {
|
||||
gitInstance.run
|
||||
.mockResolvedValueOnce({ stdout: '', stderr: '', exitCode: 0 }) // checkout
|
||||
.mockResolvedValueOnce({ stdout: '', stderr: 'still unresolved', exitCode: 1 }); // rebase --continue
|
||||
const r = await svc.resolveConflict('note-id', 'local');
|
||||
const r = await svc.resolveConflict('notes/note-id.md', 'local');
|
||||
expect(r.ok).toBe(false);
|
||||
expect((r as { reason: string }).reason).toContain('rebase --continue failed');
|
||||
expect(gitInstance.push).not.toHaveBeenCalled();
|
||||
|
||||
@@ -25,7 +25,7 @@ function makeDeps() {
|
||||
|
||||
const syncSvc = {
|
||||
getSyncDir: vi.fn(() => '/tmp/sync'),
|
||||
listConflicts: vi.fn(() => [] as { noteId: string; localText: string; remoteText: string }[]),
|
||||
listConflicts: vi.fn(() => [] as { path: string; localText: string; remoteText: string }[]),
|
||||
resolveConflict: vi.fn(async () => ({ ok: true as const })),
|
||||
getLastStatus: vi.fn(() => ({ lastAt: null as string | null, lastResult: null as { ok: boolean } | null }))
|
||||
};
|
||||
@@ -171,7 +171,7 @@ describe('sync IPC channels', () => {
|
||||
describe('sync:list-conflicts', () => {
|
||||
it('returns syncSvc.listConflicts() result', () => {
|
||||
const { deps, syncSvc } = makeDeps();
|
||||
const conflicts = [{ noteId: 'abc', localText: 'local', remoteText: 'remote' }];
|
||||
const conflicts = [{ path: 'notes/abc.md', localText: 'local', remoteText: 'remote' }];
|
||||
syncSvc.listConflicts.mockReturnValue(conflicts);
|
||||
registerSettingsApi(deps as SettingsIpcDeps);
|
||||
const h = getHandler('sync:list-conflicts');
|
||||
@@ -181,28 +181,28 @@ describe('sync IPC channels', () => {
|
||||
});
|
||||
|
||||
describe('sync:resolve-conflict', () => {
|
||||
it('valid choice "local" → delegates to syncSvc.resolveConflict', async () => {
|
||||
it('valid choice "local" → delegates to syncSvc.resolveConflict (path)', async () => {
|
||||
const { deps, syncSvc } = makeDeps();
|
||||
registerSettingsApi(deps as SettingsIpcDeps);
|
||||
const h = getHandler('sync:resolve-conflict');
|
||||
const r = await h({}, 'note-1', 'local');
|
||||
expect(syncSvc.resolveConflict).toHaveBeenCalledWith('note-1', 'local');
|
||||
const r = await h({}, 'notes/note-1.md', 'local');
|
||||
expect(syncSvc.resolveConflict).toHaveBeenCalledWith('notes/note-1.md', 'local');
|
||||
expect(r).toEqual({ ok: true });
|
||||
});
|
||||
|
||||
it('valid choice "remote" → delegates to syncSvc.resolveConflict', async () => {
|
||||
it('valid choice "remote" → delegates to syncSvc.resolveConflict (path)', async () => {
|
||||
const { deps, syncSvc } = makeDeps();
|
||||
registerSettingsApi(deps as SettingsIpcDeps);
|
||||
const h = getHandler('sync:resolve-conflict');
|
||||
await h({}, 'note-2', 'remote');
|
||||
expect(syncSvc.resolveConflict).toHaveBeenCalledWith('note-2', 'remote');
|
||||
await h({}, 'notes/note-2.md', 'remote');
|
||||
expect(syncSvc.resolveConflict).toHaveBeenCalledWith('notes/note-2.md', 'remote');
|
||||
});
|
||||
|
||||
it('invalid choice → ok: false, reason: invalid choice', async () => {
|
||||
const { deps, syncSvc } = makeDeps();
|
||||
registerSettingsApi(deps as SettingsIpcDeps);
|
||||
const h = getHandler('sync:resolve-conflict');
|
||||
const r = await h({}, 'note-1', 'both');
|
||||
const r = await h({}, 'notes/note-1.md', 'both');
|
||||
expect(syncSvc.resolveConflict).not.toHaveBeenCalled();
|
||||
expect(r).toEqual({ ok: false, reason: 'invalid choice' });
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user