Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions apps/client/src/main/github/githubIssueSync.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -194,15 +194,14 @@ describe('reconcileGitHubIssues', () => {
const jira = { ...card({ id: 'j' }), source: 'jira', externalSource: 'jira' } as Task;
const adhoc = { ...card({ id: 'a' }), source: 'adhoc', externalSource: null } as Task;
const result = reconcileGitHubIssues([jira, adhoc], [], opts);
expect(result).toMatchObject({ upserts: [], removals: [], restoreIds: [] });
expect(result).toMatchObject({ upserts: [], removals: [] });
});

it('brings an archived card back when its issue returns to the query', () => {
it('leaves an archived card alone even when its issue returns to the query', () => {
const gone = card({ archivedAt: 5, archivedReason: 'left-query' });
const result = reconcileGitHubIssues([gone], [issue(1)], opts);
expect(result.restoreIds).toEqual(['gh-acme-web-1']);
// The same ROW comes back — the id is the card's, not a new one.
expect(result.upserts[0].id).toBe('gh-acme-web-1');
expect(result.upserts).toEqual([]);
expect(result.removals).toEqual([]);
});

it('archives a card GitHub was asked about and still has, but the query dropped', () => {
Expand Down
25 changes: 13 additions & 12 deletions apps/client/src/main/github/githubIssueSync.ts
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,6 @@ export interface GitHubIssueSyncResult {
upserts: Task[];
/** Cards to take off the board — every one confirmed by a question GitHub answered. */
removals: ForgeRemoval[];
/** Ids of archived cards whose issue is back in the query — put them back on the board. */
restoreIds: string[];
/** Removals `guardRemovals` would not let through. Nothing was done to these. */
refused: ForgeRemoval[];
/** What the human should be told about this sync, or null when there is nothing to say. */
Expand Down Expand Up @@ -404,8 +402,11 @@ export function parseIssueKey(key: string): IssueRef | null {
*
* 1. **blocked** — untouched. A blocked card is never removed, so no answer could change
* the outcome and there is no question worth asking.
* 2. **archived** — already off the board. Back in the query ⇒ it returns (`restoreIds`);
* still absent ⇒ nothing to say.
* 2. **archived** — already off the board, and sync leaves it there whether or not the
* query returns its issue again. A removed card stays off the board until an explicit
* restore; nothing here brings it back. (If that restore is of an issue the search
* genuinely no longer matches, the next sync re-archives it through the ordinary
* `left-query` path below — correct given the query, and not this function's problem.)
* 3. **the search was truncated** — everything is kept, whatever else is true of it.
* 4. **the re-read did not run, or this issue's own call failed** — kept. A card must not
* be archived on a question that errored.
Expand All @@ -428,18 +429,19 @@ export function reconcileGitHubIssues(
): GitHubIssueSyncResult {
const existingByKey = githubTasksByKey(existing);
const seen = new Set<string>();
const restoreIds: string[] = [];

const upserts = issues.map((issue, i) => {
const upserts: Task[] = [];
for (let i = 0; i < issues.length; i++) {
const issue = issues[i];
const { owner, repo } = repoRefFrom(issue.repository_url);
const key = issueKeyFor(owner, repo, issue.number);
seen.add(key);
const prior = existingByKey.get(key);
// Archived, and the query returns it again: the issue matches, so the card comes back to
// the board — the same row, with the timeline, files and links it left with.
if (prior?.archivedAt != null) restoreIds.push(prior.id);
return issueToTask(issue, prior, opts, i);
});
// Archived cards stay off the board even when the query returns their issue again — the
// sync must never resurrect a removed card; only an explicit restore does.
if (prior?.archivedAt != null) continue;
upserts.push(issueToTask(issue, prior, opts, i));
}

const rechecked = opts.rechecked ?? null;
const recheckedKeys = asSet(opts.recheckedKeys);
Expand Down Expand Up @@ -534,7 +536,6 @@ export function reconcileGitHubIssues(
return {
upserts,
removals: guarded.removals,
restoreIds,
refused: guarded.refused,
warning: notes.length ? notes.join(' ') : null,
};
Expand Down
70 changes: 29 additions & 41 deletions apps/client/src/main/ipc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3268,30 +3268,23 @@ export function registerIpcHandlers(mainWindow: BrowserWindow): Engine {
// Recorded before the reconcile rather than after: the applies below cannot throw a
// network error, so this is the last point at which "the query we just ran" is true.
store.saveJiraLastQuery(jql);
const { upserts, removals, restoreIds, refused, warning } = reconcileJiraTasks(
personalForSync,
issues,
{
baseUrl: jira.baseUrl,
overrides: jira.statusCategoryOverrides,
learned: jira.learnedStatusColumns,
epicFieldId: epicField,
epicNames,
sprintFieldId: sprintField,
identity,
rechecked,
recheckedKeys,
queryChecked: confirmed?.checked ?? null,
queryMatches: confirmed?.matching ?? null,
truncated,
queryChanged,
now,
retentionMs: Math.max(0, jira.doneRetentionDays) * 24 * 60 * 60 * 1000,
},
);
// Restore first: a ticket that has come back into the query lands on its own card again,
// rather than beside the archived one it used to be.
for (const id of restoreIds) store.unarchiveTask(id);
const { upserts, removals, refused, warning } = reconcileJiraTasks(personalForSync, issues, {
baseUrl: jira.baseUrl,
overrides: jira.statusCategoryOverrides,
learned: jira.learnedStatusColumns,
epicFieldId: epicField,
epicNames,
sprintFieldId: sprintField,
identity,
rechecked,
recheckedKeys,
queryChecked: confirmed?.checked ?? null,
queryMatches: confirmed?.matching ?? null,
truncated,
queryChanged,
now,
retentionMs: Math.max(0, jira.doneRetentionDays) * 24 * 60 * 60 * 1000,
});
for (const t of upserts) store.upsertJiraTask(t);
// ARCHIVED, not deleted. A card leaving the board is not the human deleting it — the row
// keeps its timeline, its files and its links, and "Removed cards" can put it back. Each
Expand Down Expand Up @@ -3467,23 +3460,18 @@ export function registerIpcHandlers(mainWindow: BrowserWindow): Engine {
// Recorded before the reconcile rather than after: nothing below can throw a network
// error, so this is the last point at which "the query we just ran" is true.
store.saveGitHubLastQuery(query);
const { upserts, removals, restoreIds, refused, warning } = reconcileGitHubIssues(
personalForSync,
items,
{
overrides: github.labelColumnOverrides,
learned: github.learnedLabelColumns,
identity,
comments,
rechecked,
recheckedKeys,
truncated,
queryChanged,
now,
retentionMs: Math.max(0, github.doneRetentionDays) * 24 * 60 * 60 * 1000,
},
);
for (const id of restoreIds) store.unarchiveTask(id);
const { upserts, removals, refused, warning } = reconcileGitHubIssues(personalForSync, items, {
overrides: github.labelColumnOverrides,
learned: github.learnedLabelColumns,
identity,
comments,
rechecked,
recheckedKeys,
truncated,
queryChanged,
now,
retentionMs: Math.max(0, github.doneRetentionDays) * 24 * 60 * 60 * 1000,
});
for (const t of upserts) store.upsertJiraTask(t);
// ARCHIVED, not deleted — see the same loop in `syncJira`. The row keeps its timeline,
// its files and its links, and "Removed cards" can put it back.
Expand Down
17 changes: 6 additions & 11 deletions apps/client/src/main/jira/jiraSync.integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,6 @@ describe('a 300-card board survives a sync', () => {

expect(run.upserts.map((t) => t.id)).toEqual(board.map((t) => t.id));
expect(run.upserts.every((t) => t.externalUrl?.startsWith('https://jira.company.com/browse/')));
expect(run.restoreIds).toEqual([]);
});

it('costs nothing extra: a whole answer asks no by-key questions at all', async () => {
Expand Down Expand Up @@ -536,7 +535,7 @@ describe('a truncated re-read takes nothing off the board', () => {

// ---------------------------------------------------------------------------

describe('a sprint switch narrows the board, and the way back restores it', () => {
describe('a sprint switch narrows the board, and archived cards stay off it', () => {
const board = range(1, BOARD_SIZE).map((n) => cardFor(n));
/** The new sprint carries 60 of the 300 over; the other 240 are last sprint's. */
const carriedOver = range(1, 60).map((n) => issueFor(n));
Expand Down Expand Up @@ -579,7 +578,7 @@ describe('a sprint switch narrows the board, and the way back restores it', () =
expect(run.warning).toContain('more than 25% of the board in one sync');
});

it('the way back: the same rows return, same ids, nothing re-created', async () => {
it('the sprint rolling back does not resurrect the 240 that left it', async () => {
// The board a sync later, with the 240 archived — which is what `getPersonalTasksForSync`
// hands the reconciler, and the reason it is the ONE read that includes archived cards.
const afterSwitch = [
Expand All @@ -596,15 +595,12 @@ describe('a sprint switch narrows the board, and the way back restores it', () =
reconcile: { queryChanged: true },
});

expect(run.restoreIds).toEqual(range(61, BOARD_SIZE).map((n) => `card-${key(n)}`));
expect(run.removals).toEqual([]);
expect(run.refused).toEqual([]);
// The whole point of archiving rather than deleting: the ticket lands back on the row it
// left on. A `jira-*` id in here would mean a brand-new card beside the archived one, and
// everything the old row carried stranded on a card nobody can see.
expect(run.upserts).toHaveLength(BOARD_SIZE);
expect(run.upserts.map((t) => t.id)).toEqual(afterSwitch.map((t) => t.id));
expect(run.upserts.some((t) => t.id.startsWith('jira-'))).toBe(false);
// The 240 archived rows stay off the board — no upsert brings any of them back, whatever
// their issue does in JIRA. Only the 60 that were never archived are upserted.
expect(run.upserts).toHaveLength(60);
expect(run.upserts.map((t) => t.id)).toEqual(range(1, 60).map((n) => `card-${key(n)}`));
});

it('an archived card the query still does not return is left alone, not asked about again', async () => {
Expand All @@ -624,7 +620,6 @@ describe('a sprint switch narrows the board, and the way back restores it', () =
expect(run.retained).toEqual([]);
expect(searcher.asked).toEqual([]);
expect(run.removals).toEqual([]);
expect(run.restoreIds).toEqual([]);
expect(run.warning).toBeNull();
});
});
32 changes: 19 additions & 13 deletions apps/client/src/main/jira/jiraSync.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -766,31 +766,37 @@ describe('reconcileJiraTasks — no card leaves without JIRA answering about it'
});
});

describe('reconcileJiraTasks — a card comes back', () => {
it('restores an archived card whose issue is in the query again, on the same row', () => {
describe('reconcileJiraTasks — an archived card stays archived', () => {
it('leaves an archived card alone even though its issue is in the query again', () => {
const archived = jiraTask({
archivedAt: 1_700,
projectTagId: 'p-billing',
agentProjectId: 'agent-1',
lastReadCommentAt: 555,
});
const result = reconcileJiraTasks([archived], [issue('1', 'PROJ-1', 'indeterminate')], opts);
expect(result.restoreIds).toEqual(['jira-1']);
expect(result.upserts).toHaveLength(1);
// The same row, with everything JIRA has never heard of still on it.
expect(result.upserts[0]).toMatchObject({
id: 'jira-1',
projectTagId: 'p-billing',
agentProjectId: 'agent-1',
lastReadCommentAt: 555,
status: 'in-progress',
});
expect(result.upserts).toEqual([]);
expect(result.removals).toEqual([]);
});

it('says nothing about an archived card the query still does not return', () => {
const archived = jiraTask({ archivedAt: 1_700 });
const result = reconcileJiraTasks([archived], [], { ...opts, ...asked('PROJ-1') });
expect(result).toMatchObject({ upserts: [], removals: [], restoreIds: [], refused: [] });
expect(result).toMatchObject({ upserts: [], removals: [], refused: [] });
});

it('regression: a card archived left-query does not come back when the query finds it again', () => {
// The reported bug: sync one) removes a card that left the query, sync two) sees the
// ticket in a fresh page of the SAME query and must not resurrect the card for it.
const archived = jiraTask({
id: 'jira-42',
externalKey: 'PROJ-42',
archivedAt: 2_000,
archivedReason: 'left-query',
});
const result = reconcileJiraTasks([archived], [issue('42', 'PROJ-42', 'indeterminate')], opts);
expect(result.upserts).toEqual([]);
expect(result.removals).toEqual([]);
});
});

Expand Down
25 changes: 13 additions & 12 deletions apps/client/src/main/jira/jiraSync.ts
Original file line number Diff line number Diff line change
Expand Up @@ -173,8 +173,6 @@ export interface JiraSyncResult {
* leaving the board is not the human deleting it. See `store.archiveTask`.
*/
removals: JiraRemoval[];
/** Ids of archived cards whose issue is back in the query — put them back on the board. */
restoreIds: string[];
/** Removals `guardRemovals` would not let through. Nothing was done to these. */
refused: JiraRemoval[];
/** What the human should be told about this sync, or null when there is nothing to say. */
Expand Down Expand Up @@ -436,8 +434,11 @@ export function removalCandidateKeys(
* about it any more (since `isBlockedishStatus`, a Blocked ticket is exactly what puts
* a card here), but because the answer could not change the outcome: a blocked card is
* never removed, so there is no question worth asking.
* 2. **archived** — already off the board. Back in the query ⇒ it returns (`restoreIds`);
* still absent ⇒ nothing to say.
* 2. **archived** — already off the board, and sync leaves it there whether or not the
* query returns its issue again. A removed card stays off the board until an explicit
* restore; nothing here brings it back. (If that restore is of a ticket the JQL
* genuinely no longer matches, the next sync re-archives it through the ordinary
* `left-query` path below — correct given the query, and not this function's problem.)
* 3. **the search was truncated** — everything is kept, whatever else is true of it.
* 4. **never asked about** (`queryChecked` has no such key) — kept.
* 5. **asked, and JIRA says it still matches** (`queryMatches`) — kept, and counted: this
Expand All @@ -459,16 +460,17 @@ export function reconcileJiraTasks(
): JiraSyncResult {
const existingByKey = jiraTasksByKey(existing);
const seen = new Set<string>();
const restoreIds: string[] = [];

const upserts = issues.map((issue, i) => {
const upserts: Task[] = [];
for (let i = 0; i < issues.length; i++) {
const issue = issues[i];
seen.add(issue.key);
const prior = existingByKey.get(issue.key);
// Archived, and the query returns it again: the ticket matches, so the card comes back
// to the board — the same row, with the timeline, files and links it left with.
if (prior?.archivedAt != null) restoreIds.push(prior.id);
return issueToTask(issue, prior, opts, i);
});
// Archived cards stay off the board even when the query returns their issue again — the
// sync must never resurrect a removed card; only an explicit restore does.
if (prior?.archivedAt != null) continue;
upserts.push(issueToTask(issue, prior, opts, i));
}

const rechecked = opts.rechecked ? new Map(opts.rechecked.map((i) => [i.key, i])) : null;
const recheckedKeys = asSet(opts.recheckedKeys);
Expand Down Expand Up @@ -567,7 +569,6 @@ export function reconcileJiraTasks(
return {
upserts,
removals: guarded.removals,
restoreIds,
refused: guarded.refused,
warning: notes.length ? notes.join(' ') : null,
};
Expand Down
Loading