From 025f061383706377b0bf07a0295e3aa2954b6fb3 Mon Sep 17 00:00:00 2001 From: d fei Date: Tue, 1 Sep 2026 21:01:42 -0700 Subject: [PATCH] fix(docker): pre-fill the copied case instead of blanking two fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous version cleared the case name and the in-container directory on the grounds that they must differ. That left a form with three fields mysteriously filled and two empty, and turned the most common operation — changing /srv/app/api to /srv/app/web — into retyping a long path. Both are now pre-filled, with focus on the in-container directory and the caret at the end, since the tail is what changes. What stops an unmodified submit is no longer an empty field but a guard: the values applied are recorded, compared at submit time, and if nothing changed the reason is stated next to the field and focus moves to it, without sending a request that is certain to be refused. The server refuses these anyway (a duplicate case name, a twin case on the same container and directory) and its errors are clear; but making a round trip to be told "you forgot to edit the field you are looking at" is worse than saying so on the spot. The guard only applies when a source case was actually selected, so filling the adopt form from scratch is unaffected. ⚠️ The status text is written into dockerLinkStatus. My first version referenced an id that does not exist (dockerAdoptStatus), which made the explanation vanish silently and left only a toast. The test now extracts that id from the code and looks it up in index.html, pinning that it must really exist. (cherry picked from commit ba21ae11f40eb44ca9b0651583b35f0292d1204d) --- src/web/public/session-ui.js | 58 +++++++++++++++++++++++++++-- test/docker-adopt-duplicate.test.ts | 57 +++++++++++++++++++++++----- 2 files changed, 103 insertions(+), 12 deletions(-) diff --git a/src/web/public/session-ui.js b/src/web/public/session-ui.js index 68e382f4..67cbfad4 100644 --- a/src/web/public/session-ui.js +++ b/src/web/public/session-ui.js @@ -3210,6 +3210,7 @@ Object.assign(CodemanApp.prototype, { option.dataset.container = c.docker.container; option.dataset.hostId = c.docker.hostId; option.dataset.path = c.docker.path; + option.dataset.workdir = c.docker.containerWorkdir || c.docker.path; select.appendChild(option); } // Nothing to duplicate yet: an empty picker is noise on the first adoption. @@ -3236,9 +3237,49 @@ Object.assign(CodemanApp.prototype, { set('dockerContainerName', option.dataset.container); set('dockerHostId', option.dataset.hostId); set('dockerWorkspacePath', option.dataset.path); - set('dockerCaseName', ''); - set('dockerAdoptWorkdir', ''); - document.getElementById('dockerAdoptWorkdir')?.focus(); + // Pre-filled, NOT cleared: these two must differ from the source, but editing + // `/srv/app/api` into `/srv/app/web` beats retyping a long path, and the same + // goes for the name. What keeps a duplicate from being submitted unchanged is + // the guard below (dockerCloneGuard), which is a better trade than an empty + // field: the form stays a starting point instead of a blank form with three + // fields mysteriously filled in. + set('dockerCaseName', option.value); + set('dockerAdoptWorkdir', option.dataset.workdir); + // Remembered so the guard can tell "unchanged" from "happens to look similar". + select.dataset.appliedName = option.value; + select.dataset.appliedWorkdir = option.dataset.workdir || ''; + const workdir = document.getElementById('dockerAdoptWorkdir'); + workdir?.focus(); + // Caret at the end: the tail is the part that changes. + if (workdir) workdir.setSelectionRange(workdir.value.length, workdir.value.length); + }, + + /** + * Refuse a duplicate that still carries the source case's name or directory. + * + * Both are pre-filled so they can be EDITED, which means both can also be left + * alone by accident. The server refuses either (an existing case name, or an + * exact same-container-same-directory twin) with a clear message, but a + * round-trip to be told "you forgot to change the field you were looking at" is + * worse than saying so here, next to the field, before anything is sent. + * + * Returns the offending element, or null when the form is fine. + */ + dockerCloneGuard() { + const select = document.getElementById('dockerAdoptCloneFrom'); + if (!select || !select.value) return null; + const name = document.getElementById('dockerCaseName'); + const workdir = document.getElementById('dockerAdoptWorkdir'); + if (name && name.value.trim() === (select.dataset.appliedName || '')) { + return { el: name, message: `"${name.value.trim()}" is the case you copied from — give this one a new name.` }; + } + if (workdir && workdir.value.trim() === (select.dataset.appliedWorkdir || '')) { + return { + el: workdir, + message: 'Same container and same directory as the case you copied from — point this one at another directory.', + }; + } + return null; }, async _loadDockerContainerOptions() { @@ -3341,6 +3382,17 @@ Object.assign(CodemanApp.prototype, { this.showToast('Enter the name of the running container to attach to', 'error'); return; } + // A duplicate that still carries the source's name or directory: say so here, + // beside the field, rather than sending a request certain to come back refused. + const cloneIssue = adopting ? this.dockerCloneGuard() : null; + if (cloneIssue) { + this.showToast(cloneIssue.message, 'error'); + const statusEl = document.getElementById('dockerLinkStatus'); + if (statusEl) statusEl.textContent = cloneIssue.message; + cloneIssue.el.focus(); + cloneIssue.el.select?.(); + return; + } try { if (statusEl) { diff --git a/test/docker-adopt-duplicate.test.ts b/test/docker-adopt-duplicate.test.ts index 3196907d..10bb90f2 100644 --- a/test/docker-adopt-duplicate.test.ts +++ b/test/docker-adopt-duplicate.test.ts @@ -55,8 +55,8 @@ describe('the picker only offers what the server would accept', () => { }); }); -describe('applying a source fills what stays and clears what must differ', () => { - const fn = ui.slice(ui.indexOf('applyDockerCloneSource()'), ui.indexOf('applyDockerCloneSource()') + 1400); +describe('applying a source fills every field, including the two that must differ', () => { + const fn = ui.slice(ui.indexOf('applyDockerCloneSource()'), ui.indexOf('dockerCloneGuard()')); it('carries over container, host and workspace', () => { for (const id of ['dockerContainerName', 'dockerHostId', 'dockerWorkspacePath']) { @@ -64,15 +64,21 @@ describe('applying a source fills what stays and clears what must differ', () => } }); - it('clears the case name and the container workdir', () => { - // Keeping either would pre-fill a value the server is certain to refuse — - // the name as an existing case, the workdir as an exact twin. - expect(fn).toContain("set('dockerCaseName', '')"); - expect(fn).toContain("set('dockerAdoptWorkdir', '')"); + it('PRE-FILLS the case name and container workdir rather than clearing them', () => { + // Editing `/srv/app/api` into `/srv/app/web` beats retyping a long path, and a + // form with three fields mysteriously filled and two blank reads as broken. + // What stops an unchanged submit is the guard, not an empty field. + expect(fn).toContain("set('dockerCaseName', option.value)"); + expect(fn).toContain("set('dockerAdoptWorkdir', option.dataset.workdir)"); }); - it('focuses the workdir, the field the user came here to change', () => { - expect(fn).toMatch(/getElementById\('dockerAdoptWorkdir'\)\?\.focus\(\)/); + it('remembers what it applied, so the guard can tell unchanged from similar', () => { + expect(fn).toContain('select.dataset.appliedName = option.value'); + expect(fn).toContain('select.dataset.appliedWorkdir ='); + }); + + it('focuses the workdir with the caret at the END, where the edit happens', () => { + expect(fn).toMatch(/setSelectionRange\(workdir\.value\.length, workdir\.value\.length\)/); }); it('does nothing for the blank "start from scratch" option', () => { @@ -80,6 +86,39 @@ describe('applying a source fills what stays and clears what must differ', () => }); }); +describe('the guard refuses a duplicate that was never edited', () => { + const fn = ui.slice(ui.indexOf('dockerCloneGuard()'), ui.indexOf('dockerCloneGuard()') + 1400); + + it('flags an unchanged case name', () => { + expect(fn).toMatch(/appliedName/); + expect(fn).toContain('give this one a new name'); + }); + + it('flags an unchanged container workdir', () => { + expect(fn).toMatch(/appliedWorkdir/); + expect(fn).toContain('another directory'); + }); + + it('stays silent when no source was picked', () => { + // Typing a fresh adoption by hand must not be second-guessed. + expect(fn).toMatch(/if \(!select \|\| !select\.value\) return null;/); + }); + + it('runs BEFORE the request, and focuses the offending field', () => { + const submit = ui.slice(ui.indexOf('const cloneIssue'), ui.indexOf('const cloneIssue') + 500); + expect(submit).toContain('cloneIssue.el.focus()'); + expect(submit).toContain('return;'); + }); + + it('reports into a status element that actually exists', () => { + // A dead id would silently drop the explanation next to the field. + const submit = ui.slice(ui.indexOf('const cloneIssue'), ui.indexOf('const cloneIssue') + 500); + const id = /getElementById\('([^']+)'\)/.exec(submit)?.[1]; + expect(id).toBeTruthy(); + expect(html).toContain(`id="${id}"`); + }); +}); + describe('the row is wired into the adoption panel', () => { it('lives in the adopt-only block and starts hidden', () => { expect(html).toMatch(/id="dockerAdoptCloneRow"[^>]*hidden/);