Added a Delete button to each row of the Links page with an in-page…

cmoriarty commented

2026-10-16 23:32:38 -04:00

Owner

What was done

Added a Delete button to each row of the Links page with an in-page "Delete this link?" confirmation; confirming removes the link.
A deleted code no longer resolves and answers the 404 "link does not exist" page, and visits to it count no click.
After a deletion, the Links page shows "Deleted ." once at the top, above the table.


Added a POST /links/<code>/delete endpoint that removes the record and redirects back to the Links page with the deleted code in the query string.
Added two browser tests to e2e/links.spec.ts covering the confirm-delete and cancel-delete paths.
Added a global vitest setup so npm test starts its own embedded Postgres when no DATABASE_URL is set — no external database needed to run the unit tests.
Updated the README to document the delete button and post-deletion behaviour.


How to review

Run npm test — all unit tests pass, including the new delete, endpoint, and page-rendering tests; no external Postgres setup is needed.
Run npx playwright install chromium and then npm run test:e2e — both new browser tests (confirm-delete and cancel) pass.
Set DATABASE_URL (or run scripts/services.mjs first for an embedded Postgres), then npm start, and open http://localhost:3000/links in a browser.
Press a row's Delete button, click Delete in the in-page prompt, and verify the row is gone, "Deleted ." appears at the top, and visiting the deleted code shows the 404 page.

Read src/pages.ts for the delete button and in-page confirmation, and src/server.ts for the new endpoint.

Code review
Reviewed the link-deletion change (the deleteLink db statement, POST /links//delete answering 303 to /links?deleted=, the in-row confirmation UI, and its unit/e2e tests) against the proposal, delta specs and tasks. Found and fixed four bugs: the author stylesheet rule form { display: flex } overrode the UA [hidden] style so every 'Delete this link?' prompt was visible on load and Cancel could not hide it (verified in a real browser; fixed with form[hidden] { display: none; }); both new e2e tests derived the origin from about:blank before first navigation, yielding invalid 'null/...' URLs; the e2e test asserted whole-table row counts against a shared DB that holds other rows; and two unit tests asserted 303 from a fetch that follows redirects. After the fixes: tsc clean, 48/48 unit tests pass, 10/10 e2e tests pass (including the 390px/1280px no-horizontal-scroll checks), playwright --list registers all 24 tests, and openspec validate --strict passes.

Fixed before the commit

blocking, src/pages.ts:16: The author rule form { display: flex; } overrides the UA [hidden] { display: none; } (author origin beats UA origin regardless of specificity), so every row's 'Delete this link?' prompt (form.del-prompt) rendered visible on load and Cancel could not hide it — the spec's in-page confirmation scenarios did not hold in a real browser. Verified empirically: a live chromium run reported the form with hidden="" as visible and the e2e cancel test failed on not.toBeVisible(). Fixed by adding form[hidden] { display: none; } to STYLES; e2e then passes.
major, e2e/links.spec.ts:81: Both new e2e tests computed origin = new URL(page.url()).origin before any navigation; the page starts at about:blank, whose origin is the string 'null', so the URL to shorten was 'null/links?stamp=...' and the form answered 'not a valid URL' — both tests failed in shortenViaForm before reaching the delete UI (lines 81 and 142). Fixed by adding await page.goto('/') at the top of both tests, matching the file's existing test pattern.
major, e2e/links.spec.ts:94: The delete test asserted page.locator('tbody tr') toHaveCount(2) (and later toHaveCount(1) for the remaining row), but the Links page lists every stored link: four rows from earlier tests in the same file (no cleanup between tests) plus rows from shorten.spec.ts and follow.spec.ts, which run in parallel workers against the same Postgres — the counts were wrong on every run. Fixed by scoping both assertions to the test's two stamped rows (a .or() of the two hasText locators) while keeping the newest-first ordering check.
major, test/server.test.ts:257: Two new tests asserted expect(del.status).toBe(303) on fetch(url, { method: 'POST' }) without redirect: 'manual'; Node's fetch follows the 303 to GET /links?deleted= (200), so both unit tests failed (lines 257 and 300). Fixed by adding redirect: 'manual', as the sibling 'removes the stored record' test already did.


Tests
All 11 scenarios of let-a-link-be-deleted-from-shortcut-the-6r94hs have a tagged test.
The test lint found, in the run's tests on the last check:
Recorded, not decided

test/global-setup.ts:1 B7: no relative import: this file asserts on things it constructed itself, which is the measured 'asserted on my own mock' failure

Braid run

Workflow: Braid's OpenSpec flow, change let-a-link-be-deleted-from-shortcut-the-6r94hs (44 steps)
Model: vLLM · qwen3.8-27b
Time: 2 h 8 m from start to this pull request, 2 h 24 m of it in the model
Tokens: 10.5M input, 80k output, 331k reasoning, over 286 model calls
Pre-flight estimate: fits, p50 175k / p80 271k tokens against a 394k-token step allowance
CI: passed its 6 jobs on cff298572c
Run: run_01M53R7621CJW6PCZG7S6R94HS

Opened by Braid.

## What was done - Added a Delete button to each row of the Links page with an in-page "Delete this link?" confirmation; confirming removes the link. - A deleted code no longer resolves and answers the 404 "link does not exist" page, and visits to it count no click. - After a deletion, the Links page shows "Deleted <code>." once at the top, above the table. - Added a `POST /links/<code>/delete` endpoint that removes the record and redirects back to the Links page with the deleted code in the query string. - Added two browser tests to `e2e/links.spec.ts` covering the confirm-delete and cancel-delete paths. - Added a global vitest setup so `npm test` starts its own embedded Postgres when no `DATABASE_URL` is set — no external database needed to run the unit tests. - Updated the README to document the delete button and post-deletion behaviour. ## How to review 1. Run `npm test` — all unit tests pass, including the new delete, endpoint, and page-rendering tests; no external Postgres setup is needed. 2. Run `npx playwright install chromium` and then `npm run test:e2e` — both new browser tests (confirm-delete and cancel) pass. 3. Set `DATABASE_URL` (or run `scripts/services.mjs` first for an embedded Postgres), then `npm start`, and open http://localhost:3000/links in a browser. 4. Press a row's Delete button, click Delete in the in-page prompt, and verify the row is gone, "Deleted <code>." appears at the top, and visiting the deleted code shows the 404 page. 5. Read `src/pages.ts` for the delete button and in-page confirmation, and `src/server.ts` for the new endpoint. ## Code review Reviewed the link-deletion change (the deleteLink db statement, POST /links/<code>/delete answering 303 to /links?deleted=<code>, the in-row confirmation UI, and its unit/e2e tests) against the proposal, delta specs and tasks. Found and fixed four bugs: the author stylesheet rule `form { display: flex }` overrode the UA `[hidden]` style so every 'Delete this link?' prompt was visible on load and Cancel could not hide it (verified in a real browser; fixed with `form[hidden] { display: none; }`); both new e2e tests derived the origin from about:blank before first navigation, yielding invalid 'null/...' URLs; the e2e test asserted whole-table row counts against a shared DB that holds other rows; and two unit tests asserted 303 from a fetch that follows redirects. After the fixes: tsc clean, 48/48 unit tests pass, 10/10 e2e tests pass (including the 390px/1280px no-horizontal-scroll checks), playwright --list registers all 24 tests, and openspec validate --strict passes. **Fixed before the commit** - blocking, `src/pages.ts:16`: The author rule `form { display: flex; }` overrides the UA `[hidden] { display: none; }` (author origin beats UA origin regardless of specificity), so every row's 'Delete this link?' prompt (form.del-prompt) rendered visible on load and Cancel could not hide it — the spec's in-page confirmation scenarios did not hold in a real browser. Verified empirically: a live chromium run reported the form with hidden="" as visible and the e2e cancel test failed on not.toBeVisible(). Fixed by adding `form[hidden] { display: none; }` to STYLES; e2e then passes. - major, `e2e/links.spec.ts:81`: Both new e2e tests computed `origin = new URL(page.url()).origin` before any navigation; the page starts at about:blank, whose origin is the string 'null', so the URL to shorten was 'null/links?stamp=...' and the form answered 'not a valid URL' — both tests failed in shortenViaForm before reaching the delete UI (lines 81 and 142). Fixed by adding `await page.goto('/')` at the top of both tests, matching the file's existing test pattern. - major, `e2e/links.spec.ts:94`: The delete test asserted `page.locator('tbody tr')` toHaveCount(2) (and later toHaveCount(1) for the remaining row), but the Links page lists every stored link: four rows from earlier tests in the same file (no cleanup between tests) plus rows from shorten.spec.ts and follow.spec.ts, which run in parallel workers against the same Postgres — the counts were wrong on every run. Fixed by scoping both assertions to the test's two stamped rows (a .or() of the two hasText locators) while keeping the newest-first ordering check. - major, `test/server.test.ts:257`: Two new tests asserted expect(del.status).toBe(303) on `fetch(url, { method: 'POST' })` without `redirect: 'manual'`; Node's fetch follows the 303 to GET /links?deleted=<code> (200), so both unit tests failed (lines 257 and 300). Fixed by adding `redirect: 'manual'`, as the sibling 'removes the stored record' test already did. ## Tests All 11 scenarios of `let-a-link-be-deleted-from-shortcut-the-6r94hs` have a tagged test. The test lint found, in the run's tests on the last check: **Recorded, not decided** - `test/global-setup.ts:1` B7: no relative import: this file asserts on things it constructed itself, which is the measured 'asserted on my own mock' failure ## Braid run - **Workflow:** Braid's OpenSpec flow, change `let-a-link-be-deleted-from-shortcut-the-6r94hs` (44 steps) - **Model:** vLLM · qwen3.8-27b - **Time:** 2 h 8 m from start to this pull request, 2 h 24 m of it in the model - **Tokens:** 10.5M input, 80k output, 331k reasoning, over 286 model calls - **Pre-flight estimate:** fits, p50 175k / p80 271k tokens against a 394k-token step allowance - **CI:** passed its 6 jobs on `cff298572c` - **Run:** [run_01M53R7621CJW6PCZG7S6R94HS](http://openspecflow.internal.underthere.xyz/run/run_01M53R7621CJW6PCZG7S6R94HS) _Opened by Braid._

cmoriarty added 4 commits

2026-10-16 23:32:38 -04:00