From d6d5bf590b6c6290b88ae6836f0796d0c1d5d5e8 Mon Sep 17 00:00:00 2001 From: Kristian Wind Date: Tue, 18 Aug 2026 10:35:06 +0200 Subject: [PATCH] fix(sections): a section can hold more than one task MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported as "sections can only have one task", and that is exactly what it looked like. TaskList read the dragged task from its own `draggingId`, which is only set when the drag *started* in that list. A task dragged in from the unsectioned rows above, or from another section, arrived as null and the handler returned early — after it had already called `stopPropagation`, so the section around it never got the drop either. So a section could only be dropped into while it was empty, because an empty one has no row in the way to swallow the drop. Put one task in and there was no route to a second. The id comes out of `dataTransfer` at drop time now, and `dragover` decides with `carries()` rather than local state — which is the only thing a target can read while a drag is in the air. The stuck frame was the same cause: the section's own cleanup never ran, so the highlight stayed lit after the drop. It is cleared on `dragend` now, which fires once per drag on the source and bubbles to the window — the one place that is true however a drag ends, including one abandoned with Escape, which produces no drop at all and left the frame standing too. The smoke test drops a second task onto the row already in the section, which is the gesture that did nothing, and runs in WebKit as well. Co-Authored-By: Claude Opus 5 --- internal/httpapi/projects_test.go | 56 +++++++++++++++++++ web/e2e/smoke.spec.js | 69 ++++++++++++++++++++++++ web/src/lib/components/TaskList.svelte | 23 ++++++-- web/src/lib/locales/da.js | 4 +- web/src/lib/locales/en.js | 4 +- web/src/routes/projekt/[id]/+page.svelte | 18 +++++++ 6 files changed, 168 insertions(+), 6 deletions(-) diff --git a/internal/httpapi/projects_test.go b/internal/httpapi/projects_test.go index faebdd6..8742ccc 100644 --- a/internal/httpapi/projects_test.go +++ b/internal/httpapi/projects_test.go @@ -365,3 +365,59 @@ func TestAProjectCanBeCreatedIntoAGroup(t *testing.T) { t.Errorf("the stored project has group_id = %v, want %q", found["group_id"], groupID) } } + +// A section holds as many tasks as you put in it. +// +// Reported from use: "sections can only have one task". Driven through the same +// endpoint the interface uses, with the same `after_id` the drop handler sends — +// the second task goes after the first, which is the case that would fail if +// positioning inside a section were wrong. +func TestASectionHoldsMoreThanOneTask(t *testing.T) { + ts := newTestServer(t) + ts.bootstrap(t) + + _, project := ts.do(t, "POST", "/api/v1/projects", map[string]any{"name": "Skovvænget"}) + projectID := project["id"].(string) + _, section := ts.do(t, "POST", "/api/v1/projects/"+projectID+"/sections", + map[string]any{"name": "Håndværker"}) + sectionID := section["id"].(string) + + var previous string + for _, content := range []string{"knager på badeværelset", "plade på opvaskemaskine", "fuge om vinduet"} { + _, created := ts.do(t, "POST", "/api/v1/tasks", map[string]any{ + "content": content, "project_id": projectID, + }) + id := created["id"].(string) + + resp, moved := ts.do(t, "POST", "/api/v1/tasks/"+id+"/move", map[string]any{ + "project_id": projectID, "section_id": sectionID, "after_id": previous, + }) + if resp.StatusCode != http.StatusOK { + t.Fatalf("move %q: status %d, body %v", content, resp.StatusCode, moved) + } + if moved["section_id"] != sectionID { + t.Fatalf("%q did not land in the section: %v", content, moved["section_id"]) + } + previous = id + } + + _, listed := ts.do(t, "GET", "/api/v1/tasks?project_id="+projectID, nil) + tasks, _ := listed["tasks"].([]any) + inSection := []string{} + for _, raw := range tasks { + task := raw.(map[string]any) + if task["section_id"] == sectionID { + inSection = append(inSection, task["content"].(string)) + } + } + if len(inSection) != 3 { + t.Fatalf("the section holds %d tasks, want 3: %v", len(inSection), inSection) + } + // And in the order they were placed, since each went after the one before it. + want := []string{"knager på badeværelset", "plade på opvaskemaskine", "fuge om vinduet"} + for i, content := range want { + if inSection[i] != content { + t.Errorf("position %d is %q, want %q — the whole list: %v", i, inSection[i], content, inSection) + } + } +} diff --git a/web/e2e/smoke.spec.js b/web/e2e/smoke.spec.js index 631a143..f2241c2 100644 --- a/web/e2e/smoke.spec.js +++ b/web/e2e/smoke.spec.js @@ -1203,6 +1203,75 @@ test('færdige opgaver kan vises og skjules igen', async ({ page }) => { expect(trouble).toEqual([]); }); +/** + * A section holds more than one task, and stops being highlighted afterwards. + * + * Both reported from use, and both the same root cause. TaskList read the dragged + * task from its *own* `draggingId`, which is only set when the drag started in + * that list — so a task dragged in from the unsectioned rows arrived as null and + * the handler returned early. It had already called `stopPropagation`, so the + * section around it never got the drop either. + * + * The result was a section you could only drop into while it was empty, because + * an empty one has no rows to aim at. Put one task in, and there was no way to add + * a second: it looked exactly like a section that holds one task. + * + * The same `stopPropagation` left the section's frame lit, since the handler that + * clears it never ran. + */ +test('en sektion kan rumme flere opgaver, og rammen forsvinder bagefter @forms', async ({ + page +}) => { + const trouble = watchForTrouble(page); + await page.goto('/'); + + const sidebar = page.getByRole('navigation', { name: 'Hovedmenu' }); + await sidebar.getByRole('button', { name: 'Nyt projekt' }).click(); + await sidebar.getByLabel('Projektnavn').fill('Badeværelset'); + await sidebar.getByLabel('Projektnavn').press('Enter'); + await expect(page.getByRole('heading', { name: 'Badeværelset' })).toBeVisible(); + + await page.getByRole('button', { name: '+ Tilføj sektion' }).click(); + await page.getByLabel('Ny sektion').fill('Håndværker'); + await page.getByLabel('Ny sektion').press('Enter'); + await expect(page.getByRole('heading', { name: 'Håndværker' })).toBeVisible(); + + for (const what of ['knager op', 'fuge om vinduet']) { + await page.getByLabel('Ny opgave').fill(what); + await page.getByLabel('Ny opgave').press('Enter'); + await expect(page.getByText(what, { exact: true })).toBeVisible(); + } + + const section = page.locator('section').filter({ + has: page.getByRole('heading', { name: 'Håndværker' }) + }); + const row = (what) => page.locator('.row').filter({ hasText: what }); + + // The first goes in while the section is empty — the only drop that used to + // work, because there was no row in the way to swallow it. + await row('knager op').dragTo(section); + await expect(section.getByText('knager op', { exact: true })).toBeVisible(); + + // And the second, aimed at the row that is now in there. This is the one that + // did nothing at all. + await row('fuge om vinduet').dragTo(section.getByText('knager op', { exact: true })); + await expect(section.getByText('fuge om vinduet', { exact: true })).toBeVisible(); + await expect(section.locator('.row')).toHaveCount(2); + + // The frame is a drop target being offered, not a state. Once the drag is over + // it has nothing left to say. + await expect(section).not.toHaveClass(/\bover\b/); + + // It survives a reload, so this is the server's answer and not the page's. + await page.reload(); + const after = page.locator('section').filter({ + has: page.getByRole('heading', { name: 'Håndværker' }) + }); + await expect(after.locator('.row')).toHaveCount(2); + + expect(trouble).toEqual([]); +}); + /** * The sidebar never scrolls sideways. * diff --git a/web/src/lib/components/TaskList.svelte b/web/src/lib/components/TaskList.svelte index ec48629..bff8a97 100644 --- a/web/src/lib/components/TaskList.svelte +++ b/web/src/lib/components/TaskList.svelte @@ -12,8 +12,9 @@ * drawn where the row would land. */ import { app } from '$lib/stores.svelte.js'; + import { t } from '$lib/i18n.svelte.js'; import { api } from '$lib/api.js'; - import { TASK, startDrag, carries, accept } from '$lib/dnd.js'; + import { TASK, startDrag, carries, dragged, accept } from '$lib/dnd.js'; import TaskRow from './TaskRow.svelte'; let { tasks, projectId, sectionId = '', canEdit = true } = $props(); @@ -33,7 +34,12 @@ } function onDragOver(event, task) { - if (!canEdit || !carries(event, TASK) || !draggingId || draggingId === task.id) return; + // `carries` rather than `draggingId`: a task dragged from another list — the + // unsectioned rows above, or another section — never called this component's + // own dragstart, so its `draggingId` is null. Refusing on that made every + // list reject everything that came from outside it, which is the whole point + // of dragging between them. + if (!canEdit || !carries(event, TASK) || draggingId === task.id) return; accept(event); // Which half of the row the pointer is in decides the gap. Comparing against @@ -54,13 +60,22 @@ // gesture and for sections with no rows to aim at. A drop that landed on a // row has already said exactly where it goes, so it stops here. event.stopPropagation(); - const id = draggingId; + // From the drag itself, not from `draggingId`, which is only set when the + // drag *started* in this list. A task dragged in from the unsectioned rows + // or another section left it null, so this returned early — and it had + // already called stopPropagation, so the section around it never got the + // drop either. The result: a task could only be dropped into a section with + // no rows in it to aim at, which made every section look like it could hold + // exactly one task. + const id = dragged(event, TASK) || draggingId; const below = overBelow; draggingId = null; overId = null; if (!id || !canEdit || id === target.id) return; const ordered = [...tasks].sort((a, b) => a.sort_order - b.sort_order); + // The dragged task may not be in this list at all — that is the cross-list + // case — so `without` is "this list minus it" either way. const without = ordered.filter((t) => t.id !== id); const at = without.findIndex((t) => t.id === target.id); if (at < 0) return; @@ -101,7 +116,7 @@ ); } catch { app.replace(id, previous); - app.toast('Kunne ikke flytte opgaven.'); + app.toast(t('task.moveFailed')); } } diff --git a/web/src/lib/locales/da.js b/web/src/lib/locales/da.js index be96652..0953121 100644 --- a/web/src/lib/locales/da.js +++ b/web/src/lib/locales/da.js @@ -587,5 +587,7 @@ export const da = { 'view.done': 'Færdige', 'view.showDone': 'Vis færdige', - 'view.hideDone': 'Skjul færdige' + 'view.hideDone': 'Skjul færdige', + + 'task.moveFailed': 'Kunne ikke flytte opgaven.' }; diff --git a/web/src/lib/locales/en.js b/web/src/lib/locales/en.js index 2c238f3..2da314c 100644 --- a/web/src/lib/locales/en.js +++ b/web/src/lib/locales/en.js @@ -582,5 +582,7 @@ export const en = { 'view.done': 'Done', 'view.showDone': 'Show done', - 'view.hideDone': 'Hide done' + 'view.hideDone': 'Hide done', + + 'task.moveFailed': 'The task could not be moved.' }; diff --git a/web/src/routes/projekt/[id]/+page.svelte b/web/src/routes/projekt/[id]/+page.svelte index e9ed300..a2be3f0 100644 --- a/web/src/routes/projekt/[id]/+page.svelte +++ b/web/src/routes/projekt/[id]/+page.svelte @@ -197,6 +197,24 @@ */ let overSection = $state(null); + /** + * Clears the highlight when the drag ends, however it ends. + * + * `dragleave` and the section's own `ondrop` are not enough. A drop on a *row* + * inside a section is handled by TaskList, which stops propagation — so the + * section never sees the drop and its frame stayed lit afterwards. A drag + * abandoned with Escape has the same shape: no drop at all, and the frame kept + * standing until the next one. + * + * `dragend` fires exactly once per drag, on the source, and bubbles to the + * window — so it is the one place that is true for every way a drag can finish. + */ + $effect(() => { + const clear = () => (overSection = null); + window.addEventListener('dragend', clear); + return () => window.removeEventListener('dragend', clear); + }); + function onSectionDragOver(event, sectionId) { if (!canEdit || !carries(event, TASK)) return; accept(event);