diff --git a/fastlane/Fastfile b/fastlane/Fastfile index a87b6f0e0e..ed391137da 100644 --- a/fastlane/Fastfile +++ b/fastlane/Fastfile @@ -99,22 +99,64 @@ end # direction. REVIEW_SUBMISSION_RACE = 'review submission is already in progress' +# App Store Connect also serializes review state per app across releases: while +# one version is still in review, the next version can neither be created nor +# have a build attached. So a release tagged before the previous one clears +# review fails either at version creation ("You cannot create a new version of +# the App in the current state") or at build-attach ("The specified pre-release +# build could not be added"). Unlike the race above this is NOT softened -- the +# version genuinely did not go out, and a green run would falsely read as +# "released". We only trade the raw spaceship stacktrace for a one-line, +# actionable explanation; the lane still fails. Message-matched against fastlane +# deliver 2.225.0 -- if the phrasing drifts it simply falls back to the raw +# error, which is fine (the job still fails). +VERSION_IN_REVIEW_BLOCKS = [ + 'cannot create a new version', + 'pre-release build could not be added', +].freeze + def submit_to_app_store(extra) upload_to_app_store(deliver_options(extra)) rescue => e - raise unless submit_for_review? && e.message.to_s.downcase.include?(REVIEW_SUBMISSION_RACE) - # Surface as a GitHub Actions run annotation, not just a buried log line, so a - # green job still flags the required manual step. Static text only -- never - # interpolate e/options here (they can carry API key material; see header). - if ENV['GITHUB_ACTIONS'] - puts '::warning title=App Store build uploaded but NOT submitted::A review ' \ - 'submission is already open for this app. Add this build to it in App ' \ - 'Store Connect, or this platform skips this version.' + msg = e.message.to_s.downcase + + # Soft success: the other platform's release lane already opened the shared + # per-app review submission. The binary is uploaded and safe; it just needs to + # be added to the open submission by hand. Surface as a GitHub Actions run + # annotation, not just a buried log line, so a green job still flags the + # required manual step. Static text only -- never interpolate e/options here + # (they can carry API key material; see header). + if submit_for_review? && msg.include?(REVIEW_SUBMISSION_RACE) + if ENV['GITHUB_ACTIONS'] + puts '::warning title=App Store build uploaded but NOT submitted::A review ' \ + 'submission is already open for this app. Add this build to it in App ' \ + 'Store Connect, or this platform skips this version.' + end + UI.important('Build uploaded, but a review submission is already open for this app') + UI.important("(the other platform's release lane created it first). Add this build to") + UI.important('the open submission in App Store Connect for this version, or this platform') + UI.important('will skip this version. Not failing the lane: the binary is uploaded.') + return end - UI.important('Build uploaded, but a review submission is already open for this app') - UI.important("(the other platform's release lane created it first). Add this build to") - UI.important('the open submission in App Store Connect for this version, or this platform') - UI.important('will skip this version. Not failing the lane: the binary is uploaded.') + + # Known "a previous version is still in review" block. Still a hard failure -- + # we only replace the raw stacktrace with a clear, actionable one-liner (the + # raw error still prints after the re-raise). Static text only, same + # key-material reason as above. + if submit_for_review? && VERSION_IN_REVIEW_BLOCKS.any? { |m| msg.include?(m) } + if ENV['GITHUB_ACTIONS'] + puts '::error title=App Store submit blocked: previous version still in review::' \ + 'App Store Connect will not create or submit this version while the ' \ + 'previous version is still in review. Let it finish review (or remove ' \ + 'it from review in App Store Connect), then re-run this release.' + end + UI.error('App Store submit blocked: a previous version is still in review.') + UI.error('App Store Connect will not create or submit this version until the') + UI.error('previous one clears review. Let it finish (or pull it from review in') + UI.error('App Store Connect), then re-run this release.') + end + + raise end platform :ios do diff --git a/src/app/op-log/sync/conflict-resolution.service.spec.ts b/src/app/op-log/sync/conflict-resolution.service.spec.ts index 0fd0b99934..669b6dd0af 100644 --- a/src/app/op-log/sync/conflict-resolution.service.spec.ts +++ b/src/app/op-log/sync/conflict-resolution.service.spec.ts @@ -551,6 +551,97 @@ describe('ConflictResolutionService', () => { expect(taskList).toContain('<img'); }); + it('names the discarded title when the title itself conflicted (#8694 review)', async () => { + const bannerService = TestBed.inject(BannerService); + const openBannerSpy = spyOn(bannerService, 'open'); + // Kept (current) title comes from the store = the winning value; the + // discarded value must still be surfaced so double-check is actionable. + mockStore.select.and.returnValue(of({ title: 'Remote title' })); + const now = Date.now(); + const conflicts: EntityConflict[] = [ + createConflict( + 'task-1', + [ + { + ...createOpWithTimestamp('local-1', 'client-a', now - 1000), + payload: { + actionPayload: { + task: { id: 'task-1', changes: { title: 'My local title' } }, + }, + entityChanges: [], + }, + }, + ], + [ + { + ...createOpWithTimestamp('remote-1', 'client-b', now), + payload: { + actionPayload: { + task: { id: 'task-1', changes: { title: 'Remote title' } }, + }, + entityChanges: [], + }, + }, + ], + ), + ]; + mockOperationApplier.applyOperations.and.resolveTo({ + appliedOps: conflicts[0].remoteOps, + }); + + await service.autoResolveConflictsLWW(conflicts); + + const taskList = openBannerSpy.calls.mostRecent().args[0].translateParams + ?.taskList as string; + expect(taskList).toContain('"Remote title"'); + expect(taskList).toContain('"My local title"'); + }); + + it('escapes the discarded title too (XSS guard on the new field)', async () => { + const bannerService = TestBed.inject(BannerService); + const openBannerSpy = spyOn(bannerService, 'open'); + mockStore.select.and.returnValue(of({ title: 'Kept' })); + const now = Date.now(); + const conflicts: EntityConflict[] = [ + createConflict( + 'task-1', + [ + { + ...createOpWithTimestamp('local-1', 'client-a', now - 1000), + payload: { + actionPayload: { + task: { + id: 'task-1', + changes: { title: '' }, + }, + }, + entityChanges: [], + }, + }, + ], + [ + { + ...createOpWithTimestamp('remote-1', 'client-b', now), + payload: { + actionPayload: { task: { id: 'task-1', changes: { title: 'Kept' } } }, + entityChanges: [], + }, + }, + ], + ), + ]; + mockOperationApplier.applyOperations.and.resolveTo({ + appliedOps: conflicts[0].remoteOps, + }); + + await service.autoResolveConflictsLWW(conflicts); + + const taskList = openBannerSpy.calls.mostRecent().args[0].translateParams + ?.taskList as string; + expect(taskList).not.toContain(' { const bannerService = TestBed.inject(BannerService); const openBannerSpy = spyOn(bannerService, 'open'); diff --git a/src/app/op-log/sync/conflict-resolution.service.ts b/src/app/op-log/sync/conflict-resolution.service.ts index 2f1b1900be..4603755a33 100644 --- a/src/app/op-log/sync/conflict-resolution.service.ts +++ b/src/app/op-log/sync/conflict-resolution.service.ts @@ -575,12 +575,12 @@ export class ConflictResolutionService { contentConflicts: LwwContentConflict[], ): Promise { const MAX_NAMED = 3; - const titles = await Promise.all( + const labels = await Promise.all( contentConflicts .slice(0, MAX_NAMED) - .map((conflict) => this._getContentConflictTitle(conflict.entityId)), + .map((conflict) => this._buildContentConflictLabel(conflict)), ); - const named = titles.map((title) => `"${escapeHtml(title)}"`).join(', '); + const named = labels.join(', '); const taskList = contentConflicts.length > MAX_NAMED ? `${named} …` : named; this.bannerService.open({ @@ -591,6 +591,42 @@ export class ConflictResolutionService { }); } + /** + * Builds the display label for one conflicted task inside the banner's task + * list. Normally just the (escaped, quoted) current title. When the discarded + * edit changed the title, the current title is the *kept* value — useless for + * double-checking on its own — so we also name the discarded title: `"kept" + * (discarded: "dropped")`. Both values are escaped (rendered via `[innerHTML]`, + * see `_showContentConflictBanner`). + */ + private async _buildContentConflictLabel( + conflict: LwwContentConflict, + ): Promise { + const keptTitle = await this._getContentConflictTitle(conflict.entityId); + const kept = `"${escapeHtml(keptTitle)}"`; + const discardedTitle = conflict.discardedTitle?.trim(); + // Skip the annotation when nothing meaningful to add: no title was + // discarded, or the discarded title equals the current one. The equality + // case covers two situations, both correctly silenced: (a) both devices set + // the same title; (b) a title edit lost to a concurrent *other-field* remote + // win — the winner didn't touch the title, so the current state still shows + // the (now-rejected) local title, which equals the discarded value. In both + // an annotation would read `"X" (discarded: "X")` — pure noise, no divergence + // to point at — so we render just the current title. (For the common + // title-vs-title case the current title IS the winning value and differs + // from the discarded one, so the annotation shows.) + if (!discardedTitle || discardedTitle === keptTitle.trim()) { + return kept; + } + const discarded = `"${escapeHtml(discardedTitle)}"`; + return ( + this.translateService?.instant(T.F.SYNC.B.CONTENT_CONFLICT_TITLE_CHANGE, { + kept, + discarded, + }) ?? `${kept} (discarded: ${discarded})` + ); + } + private async _getContentConflictTitle(entityId: string): Promise { const entity = await this.getCurrentEntityState('TASK' as EntityType, entityId); const title = (entity as { title?: string } | undefined)?.title; diff --git a/src/app/op-log/sync/lww-conflict-summary.util.spec.ts b/src/app/op-log/sync/lww-conflict-summary.util.spec.ts index 59d4eb4281..7d54e7f722 100644 --- a/src/app/op-log/sync/lww-conflict-summary.util.spec.ts +++ b/src/app/op-log/sync/lww-conflict-summary.util.spec.ts @@ -70,7 +70,13 @@ describe('findLwwContentConflicts', () => { payloadKeyFor, ); - expect(result).toEqual([{ entityId: 'task-1', discardedFields: ['title'] }]); + expect(result).toEqual([ + { + entityId: 'task-1', + discardedFields: ['title'], + discardedTitle: 'My local title', + }, + ]); }); it('classifies a discarded scheduling edit as routine (no content conflict)', () => { @@ -216,7 +222,9 @@ describe('findLwwContentConflicts', () => { payloadKeyFor, ); - expect(result).toEqual([{ entityId: 'task-1', discardedFields: ['title'] }]); + expect(result).toEqual([ + { entityId: 'task-1', discardedFields: ['title'], discardedTitle: 'local' }, + ]); }); it('merges distinct discarded content fields for the same task', () => { @@ -236,6 +244,80 @@ describe('findLwwContentConflicts', () => { payloadKeyFor, ); - expect(result).toEqual([{ entityId: 'task-1', discardedFields: ['title', 'notes'] }]); + expect(result).toEqual([ + { + entityId: 'task-1', + discardedFields: ['title', 'notes'], + discardedTitle: 'local', + }, + ]); + }); + + it('omits discardedTitle when the discarded edit did not touch the title', () => { + const result = findLwwContentConflicts( + [ + resolution( + 'remote', + [wrappedUpdate({ notes: 'lost note' })], + [wrappedUpdate({ dueDay: null })], + ), + ], + payloadKeyFor, + ); + + expect(result).toEqual([{ entityId: 'task-1', discardedFields: ['notes'] }]); + expect('discardedTitle' in result[0]).toBe(false); + }); + + it('ignores empty/whitespace discarded titles, keeping the real one', () => { + // A concurrent discarded title-clear must not blank out a genuine discarded + // rename in the same batch. + const result = findLwwContentConflicts( + [ + resolution( + 'remote', + [wrappedUpdate({ title: ' ' })], + [wrappedUpdate({ dueDay: null })], + ), + resolution( + 'remote', + [wrappedUpdate({ title: 'real discarded title' })], + [wrappedUpdate({ notes: 'x' })], + ), + ], + payloadKeyFor, + ); + + expect(result).toEqual([ + { + entityId: 'task-1', + discardedFields: ['title'], + discardedTitle: 'real discarded title', + }, + ]); + }); + + it('surfaces the LAST discarded title (final rename), not the first', () => { + // User renamed the task twice offline (A -> B), both discarded on a remote + // win. The final value B is what they will look for. + const result = findLwwContentConflicts( + [ + resolution( + 'remote', + [wrappedUpdate({ title: 'rename A' })], + [wrappedUpdate({ dueDay: null })], + ), + resolution( + 'remote', + [wrappedUpdate({ title: 'rename B' })], + [wrappedUpdate({ notes: 'x' })], + ), + ], + payloadKeyFor, + ); + + expect(result).toEqual([ + { entityId: 'task-1', discardedFields: ['title'], discardedTitle: 'rename B' }, + ]); }); }); diff --git a/src/app/op-log/sync/lww-conflict-summary.util.ts b/src/app/op-log/sync/lww-conflict-summary.util.ts index 8030a493df..a39167bc28 100644 --- a/src/app/op-log/sync/lww-conflict-summary.util.ts +++ b/src/app/op-log/sync/lww-conflict-summary.util.ts @@ -25,6 +25,16 @@ export interface LwwContentConflict { entityId: string; /** The content fields the discarded edit(s) changed. */ discardedFields: string[]; + /** + * The title value the discarded edit set, when the discarded edit changed the + * title. For a title conflict the current (kept) title is the *winning* value, + * so naming the task by it alone gives the user nothing to double-check — this + * is the value that was dropped, so the banner can show "kept X, discarded Y". + * Absent when no discarded edit touched the title (or it only cleared it to + * empty). The LAST non-empty discarded title in the batch wins — the user's + * final rename (deterministic given op order). Never logged (#9). + */ + discardedTitle?: string; } /** @@ -43,7 +53,7 @@ export const findLwwContentConflicts = ( resolutions: LwwResolvedConflict[], payloadKeyFor: (entityType: string) => string, ): LwwContentConflict[] => { - const fieldsByTask = new Map>(); + const byTask = new Map; discardedTitle?: string }>(); for (const { winner, conflict } of resolutions) { if (conflict.entityType !== 'TASK') { @@ -58,18 +68,34 @@ export const findLwwContentConflicts = ( if (op.opType !== OpType.Update) { continue; } - for (const field of Object.keys(extractUpdateChanges(op.payload, payloadKey))) { - if (TASK_CONTENT_FIELDS.includes(field)) { - const fields = fieldsByTask.get(conflict.entityId) ?? new Set(); - fields.add(field); - fieldsByTask.set(conflict.entityId, fields); + const changes = extractUpdateChanges(op.payload, payloadKey); + for (const field of Object.keys(changes)) { + if (!TASK_CONTENT_FIELDS.includes(field)) { + continue; } + const acc = byTask.get(conflict.entityId) ?? { fields: new Set() }; + acc.fields.add(field); + // Keep the LAST non-empty discarded title so the banner names the user's + // final rename, not a stale intermediate one (offline A→B→C, all + // discarded → show C). Ops are processed in append order, so a later + // non-empty value overwrites an earlier one. See + // LwwContentConflict.discardedTitle. + if (field === 'title') { + const value = (changes as { title?: unknown }).title; + if (typeof value === 'string' && value.trim().length) { + acc.discardedTitle = value; + } + } + byTask.set(conflict.entityId, acc); } } } - return [...fieldsByTask].map(([entityId, fields]) => ({ + return [...byTask].map(([entityId, { fields, discardedTitle }]) => ({ entityId, discardedFields: [...fields], + // Omit the key entirely when no title was discarded so routine callers and + // tests keep the minimal { entityId, discardedFields } shape. + ...(discardedTitle !== undefined ? { discardedTitle } : {}), })); }; diff --git a/src/app/t.const.ts b/src/app/t.const.ts index c3fbcb2ebe..f2aed74848 100644 --- a/src/app/t.const.ts +++ b/src/app/t.const.ts @@ -1227,6 +1227,7 @@ const T = { }, B: { CONTENT_CONFLICT_RESOLVED: 'F.SYNC.B.CONTENT_CONFLICT_RESOLVED', + CONTENT_CONFLICT_TITLE_CHANGE: 'F.SYNC.B.CONTENT_CONFLICT_TITLE_CHANGE', CONTENT_CONFLICT_UNTITLED: 'F.SYNC.B.CONTENT_CONFLICT_UNTITLED', }, BTN_RESTORE_FROM_HISTORY: 'F.SYNC.BTN_RESTORE_FROM_HISTORY', diff --git a/src/assets/i18n/en.json b/src/assets/i18n/en.json index 176ce1a21a..69d3079ed2 100644 --- a/src/assets/i18n/en.json +++ b/src/assets/i18n/en.json @@ -1204,6 +1204,7 @@ }, "B": { "CONTENT_CONFLICT_RESOLVED": "Conflicting edits to {{taskList}} were auto-resolved by keeping the most recent version. Older edits may have been discarded — please double-check.", + "CONTENT_CONFLICT_TITLE_CHANGE": "{{kept}} (discarded: {{discarded}})", "CONTENT_CONFLICT_UNTITLED": "Untitled task" }, "BTN_RESTORE_FROM_HISTORY": "Restore from History", diff --git a/src/assets/i18n/fr.json b/src/assets/i18n/fr.json index ba20a177e9..5b6a922fc4 100644 --- a/src/assets/i18n/fr.json +++ b/src/assets/i18n/fr.json @@ -2054,7 +2054,7 @@ "CONFIRM": "Confirmer", "CONTINUE": "Continuer", "DELETE": "Effacer", - "DISMISS": "Rejeter", + "DISMISS": "Ignorer", "DO_IT": "Fais le !", "DUPLICATE": "Dupliquer", "DURATION_DESCRIPTION": "ex: \"5h 23m\" ce qui équivaut à 5 heures et 23 minutes",