From 94a5461c68947ea4431c92d294c36b8bdc34440a Mon Sep 17 00:00:00 2001 From: Jeff Maki Date: Fri, 25 Sep 2026 11:03:35 -0400 Subject: [PATCH 1/3] Show why a review comment failed to post A failed comment post always said "Please try again", which does not help with any of the reasons OSM refuses one: the changeset is still open, the hourly comment limit was hit, or the proxy denied workspace access. Show the server's message instead. OSM also answers 200 when it drops a changeset comment whose author fails validation (it saves with `create`, not `create!`). Check that the comment landed after re-fetching the discussion, and report it if not rather than clearing the draft as if it had posted. Co-Authored-By: Claude Opus 5.5 (1M context) (cherry picked from commit 45d44b7efb5bae76debf1fdbb954e1fbd919aefe) --- components/review/Discussion.vue | 21 +++++++++- test/e2e/review.spec.ts | 71 ++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 2 deletions(-) diff --git a/components/review/Discussion.vue b/components/review/Discussion.vue index 716b360d..2bf344ae 100644 --- a/components/review/Discussion.vue +++ b/components/review/Discussion.vue @@ -20,6 +20,7 @@ import { toast } from 'vue3-toastify'; import type { ReviewListItem } from '~/services/review'; import { osmClient } from '~/services/index'; +import { resolveHttpErrorMessage } from '~/services/http'; import type ChatBox from '~/components/ChatBox.vue'; import type { ChatMessage } from '~/components/ChatBox.vue'; @@ -77,9 +78,21 @@ function noteComments(note: OsmNote): ChatMessage[] { async function send(message: string) { try { if (props.item.isChangeset) { + const countBefore = messages.value.length; await osmClient.postChangesetComment(workspaceId, props.item.id, message); // Re-fetch so the newly posted comment (and its server timestamp) appears. await refreshChangeset(); + + // OSM answers 200 even when it fails to save the comment (it uses + // `create`, not `create!`, so a comment whose author fails validation is + // dropped silently). Check it actually landed rather than trusting the + // status, and keep the user's text if it did not. + if (messages.value.length <= countBefore) { + toast.error('Your comment was not saved. The server accepted the request but did not ' + + 'record the comment, which usually means a problem with your account. Please ' + + 'contact your workspace administrator.'); + return; + } } else if (props.item.isNote) { await osmClient.postNoteComment(workspaceId, props.item.id, message); @@ -92,8 +105,12 @@ async function send(message: string) { // user's text. chat.value?.clear(); } - catch { - toast.error('Failed to post your comment. Please try again.'); + catch (e: unknown) { + // Show why the server refused -- e.g. the changeset is still open, the + // hourly comment limit was hit, or the proxy denied workspace access -- + // since "try again" does not help with any of those. + const reason = await resolveHttpErrorMessage(e, 'Please try again.'); + toast.error(`Failed to post your comment. ${reason}`); } } diff --git a/test/e2e/review.spec.ts b/test/e2e/review.spec.ts index 7a3ff549..23f3cb6d 100644 --- a/test/e2e/review.spec.ts +++ b/test/e2e/review.spec.ts @@ -495,6 +495,77 @@ test.describe('workspace review', () => { await expect(discussion.locator('.message-list')).not.toContainText('This should fail to post.'); }); + // Opens changeset 4242's discussion. Its comments come from + // changeset/4242.json?include_discussion=true, which answers with `comments` + // every time, so a test can make a post appear to succeed without landing. + async function openChangesetDiscussion(page: Page, comments: object[]) { + await seedAuthenticatedSession(page); + await stubReviewApis(page); + await page.route('**/workspaces/1/changesets/*/adiff', route => + route.fulfill({ json: { actions: [] } }) + ); + await page.route('https://ecn.*.tiles.virtualearth.net/**', route => route.abort()); + await page.route('**/osm/api/0.6/changeset/4242.json**', route => + route.fulfill({ json: { changeset: { ...changesets.changesets[0], comments } } }) + ); + + await page.goto('/workspace/1/review?changeset=4242'); + await expect(page.locator('.review-toolbar')).toContainText('Changeset: #4242'); + await page.locator('.review-toolbar button:has(.md-chat_bubble_outline)').click(); + + const discussion = page.locator('.review-discussion'); + await expect(discussion).toContainText('Looks good to me.'); + return discussion; + } + + const existingChangesetComment = { + id: 1, + text: 'Looks good to me.', + visible: true, + user: 'mapper_jane', + uid: 7, + date: '2026-06-11T09:00:00.000Z' + }; + + // @test e2e: if the server refuses a changeset comment, the toast gives the + // server's reason rather than a generic "try again" + test('a refused changeset comment shows the server reason', async ({ page }) => { + // OSM refuses comments on a changeset that is still open, as plain text. + await page.route('**/osm/api/0.6/changeset/4242/comment**', route => + route.fulfill({ + status: 409, + contentType: 'text/plain', + body: 'The changeset 4242 was not yet closed at 2026-06-10 10:05:00 UTC' + }) + ); + const discussion = await openChangesetDiscussion(page, [existingChangesetComment]); + + await discussion.locator('textarea').fill('Please fix the crossing.'); + await discussion.locator('button[title="Send"]').click(); + + const errorToast = page.locator('.Toastify__toast--error'); + await expect(errorToast).toContainText('was not yet closed'); + await expect(discussion.locator('textarea')).toHaveValue('Please fix the crossing.'); + }); + + // @test e2e: a changeset comment the server accepts but does not save is + // reported, and the draft is kept + test('a changeset comment that does not land is reported', async ({ page }) => { + // OSM answers 200 but drops the comment when its author fails validation. + // The re-fetched discussion is unchanged. + await page.route('**/osm/api/0.6/changeset/4242/comment**', route => + route.fulfill({ contentType: 'application/xml', body: '' }) + ); + const discussion = await openChangesetDiscussion(page, [existingChangesetComment]); + + await discussion.locator('textarea').fill('Please fix the crossing.'); + await discussion.locator('button[title="Send"]').click(); + + const errorToast = page.locator('.Toastify__toast--error'); + await expect(errorToast).toContainText('was not saved'); + await expect(discussion.locator('textarea')).toHaveValue('Please fix the crossing.'); + }); + // @test e2e: validate that all the API calls used on this page match the Swagger spec test('new-API calls conform to the OpenAPI spec', async ({ page }) => { await seedAuthenticatedSession(page); From 84761e76f811b52ff5a9942149f47cc1321297cb Mon Sep 17 00:00:00 2001 From: Jeff Maki Date: Fri, 25 Sep 2026 11:43:13 -0400 Subject: [PATCH 2/3] Confirm a posted changeset comment by its text, not the count A rising comment count does not show that this comment landed: someone else's comment arriving at the same time raises it too. Look for a new comment with the submitted text instead. Not by author, since an OSM display name need not match the TDEI name the client holds. The changeset discussion tests selected the changeset through a deep link, which is flaky headless, and opened the panel while the page was still reloading the list, which closes it. Click the item and wait for the reload. Co-Authored-By: Claude Opus 5.5 (1M context) (cherry picked from commit 78cfb674d431d453d743254bf114a9698f91b76e) --- components/review/Discussion.vue | 12 ++++-- test/e2e/review.spec.ts | 65 ++++++++++++++++++++++++++++++-- 2 files changed, 71 insertions(+), 6 deletions(-) diff --git a/components/review/Discussion.vue b/components/review/Discussion.vue index 2bf344ae..458550bd 100644 --- a/components/review/Discussion.vue +++ b/components/review/Discussion.vue @@ -78,7 +78,7 @@ function noteComments(note: OsmNote): ChatMessage[] { async function send(message: string) { try { if (props.item.isChangeset) { - const countBefore = messages.value.length; + const idsBefore = new Set(messages.value.map(m => m.id)); await osmClient.postChangesetComment(workspaceId, props.item.id, message); // Re-fetch so the newly posted comment (and its server timestamp) appears. await refreshChangeset(); @@ -86,8 +86,14 @@ async function send(message: string) { // OSM answers 200 even when it fails to save the comment (it uses // `create`, not `create!`, so a comment whose author fails validation is // dropped silently). Check it actually landed rather than trusting the - // status, and keep the user's text if it did not. - if (messages.value.length <= countBefore) { + // status, and keep the user's text if it did not. Look for a new comment + // with this text rather than a higher count, which someone else's + // comment could also cause. Not by author: OSM display names need not + // match the TDEI name we hold (some are the account's email address). + const landed = messages.value.some(m => + !idsBefore.has(m.id) && m.text.trim() === message.trim()); + + if (!landed) { toast.error('Your comment was not saved. The server accepted the request but did not ' + 'record the comment, which usually means a problem with your account. Please ' + 'contact your workspace administrator.'); diff --git a/test/e2e/review.spec.ts b/test/e2e/review.spec.ts index 23f3cb6d..8d77084b 100644 --- a/test/e2e/review.spec.ts +++ b/test/e2e/review.spec.ts @@ -509,9 +509,22 @@ test.describe('workspace review', () => { route.fulfill({ json: { changeset: { ...changesets.changesets[0], comments } } }) ); - await page.goto('/workspace/1/review?changeset=4242'); - await expect(page.locator('.review-toolbar')).toContainText('Changeset: #4242'); - await page.locator('.review-toolbar button:has(.md-chat_bubble_outline)').click(); + // Select by clicking rather than a ?changeset= deep link, which is flaky + // headless (see the changeset deep links tests). + await page.goto('/workspace/1/review'); + // Selecting a changeset sets ?changeset=, which makes the page reload the + // list (changesets, then notes) with resolved items included. That swaps + // in a new item object and closes any panel opened meanwhile, so let the + // reload finish before opening one. + const reloaded = page.waitForResponse('**/osm/api/0.6/notes/search.json?*closed=-1*'); + await sidebar(page).locator('.review-item', { hasText: '#4242' }).click(); + await reloaded; + await page.evaluate(() => new Promise(resolve => + requestAnimationFrame(() => requestAnimationFrame(resolve)))); + + const overlay = page.locator('.review-overlay'); + await expect(overlay.locator('.review-toolbar')).toContainText('#4242'); + await overlay.locator('.review-toolbar button:has(.md-chat_bubble_outline)').click(); const discussion = page.locator('.review-discussion'); await expect(discussion).toContainText('Looks good to me.'); @@ -548,6 +561,36 @@ test.describe('workspace review', () => { await expect(discussion.locator('textarea')).toHaveValue('Please fix the crossing.'); }); + // @test e2e: a posted changeset comment appears and clears the draft, even + // when OSM's author name differs from the signed-in user's name + test('a posted changeset comment appears and clears the draft', async ({ page }) => { + await page.route('**/osm/api/0.6/changeset/4242/comment**', route => + route.fulfill({ contentType: 'application/xml', body: '' }) + ); + const discussion = await openChangesetDiscussion(page, [existingChangesetComment]); + + await page.route('**/osm/api/0.6/changeset/4242.json**', route => + route.fulfill({ + json: { + changeset: { + ...changesets.changesets[0], + comments: [ + existingChangesetComment, + { ...existingChangesetComment, id: 2, text: 'Please fix the crossing.', user: 'tester@example.com', uid: 42 } + ] + } + } + }) + ); + + await discussion.locator('textarea').fill('Please fix the crossing.'); + await discussion.locator('button[title="Send"]').click(); + + await expect(discussion.locator('.message-list')).toContainText('Please fix the crossing.'); + await expect(discussion.locator('textarea')).toHaveValue(''); + await expect(page.locator('.Toastify__toast--error')).toHaveCount(0); + }); + // @test e2e: a changeset comment the server accepts but does not save is // reported, and the draft is kept test('a changeset comment that does not land is reported', async ({ page }) => { @@ -558,6 +601,22 @@ test.describe('workspace review', () => { ); const discussion = await openChangesetDiscussion(page, [existingChangesetComment]); + // Someone else's comment lands meanwhile, so the count still goes up: only + // a comment with the submitted text counts as this one landing. + await page.route('**/osm/api/0.6/changeset/4242.json**', route => + route.fulfill({ + json: { + changeset: { + ...changesets.changesets[0], + comments: [ + existingChangesetComment, + { ...existingChangesetComment, id: 2, text: 'Crossing fixed.', user: 'mapper_joe', uid: 8 } + ] + } + } + }) + ); + await discussion.locator('textarea').fill('Please fix the crossing.'); await discussion.locator('button[title="Send"]').click(); From 9694fb7c7dfd0ed535d39df23db5cb072261718c Mon Sep 17 00:00:00 2001 From: Jeff Maki Date: Fri, 25 Sep 2026 14:37:48 -0400 Subject: [PATCH 3/3] Select the changeset without waiting for a reload production's review page predates changeset selection via the URL, so selecting a changeset does not reload the list, and waiting for that reload would never finish. Co-Authored-By: Claude Opus 5.5 (1M context) --- test/e2e/review.spec.ts | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/test/e2e/review.spec.ts b/test/e2e/review.spec.ts index 8d77084b..87ccba65 100644 --- a/test/e2e/review.spec.ts +++ b/test/e2e/review.spec.ts @@ -509,18 +509,8 @@ test.describe('workspace review', () => { route.fulfill({ json: { changeset: { ...changesets.changesets[0], comments } } }) ); - // Select by clicking rather than a ?changeset= deep link, which is flaky - // headless (see the changeset deep links tests). await page.goto('/workspace/1/review'); - // Selecting a changeset sets ?changeset=, which makes the page reload the - // list (changesets, then notes) with resolved items included. That swaps - // in a new item object and closes any panel opened meanwhile, so let the - // reload finish before opening one. - const reloaded = page.waitForResponse('**/osm/api/0.6/notes/search.json?*closed=-1*'); await sidebar(page).locator('.review-item', { hasText: '#4242' }).click(); - await reloaded; - await page.evaluate(() => new Promise(resolve => - requestAnimationFrame(() => requestAnimationFrame(resolve)))); const overlay = page.locator('.review-overlay'); await expect(overlay.locator('.review-toolbar')).toContainText('#4242');