mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-09 08:59:40 +02:00
fix(docker): pre-fill the copied case instead of blanking two fields
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.
This commit is contained in:
@@ -3196,6 +3196,7 @@ Object.assign(CodemanApp.prototype, {
|
|||||||
option.dataset.container = c.docker.container;
|
option.dataset.container = c.docker.container;
|
||||||
option.dataset.hostId = c.docker.hostId;
|
option.dataset.hostId = c.docker.hostId;
|
||||||
option.dataset.path = c.docker.path;
|
option.dataset.path = c.docker.path;
|
||||||
|
option.dataset.workdir = c.docker.containerWorkdir || c.docker.path;
|
||||||
select.appendChild(option);
|
select.appendChild(option);
|
||||||
}
|
}
|
||||||
// Nothing to duplicate yet: an empty picker is noise on the first adoption.
|
// Nothing to duplicate yet: an empty picker is noise on the first adoption.
|
||||||
@@ -3222,9 +3223,49 @@ Object.assign(CodemanApp.prototype, {
|
|||||||
set('dockerContainerName', option.dataset.container);
|
set('dockerContainerName', option.dataset.container);
|
||||||
set('dockerHostId', option.dataset.hostId);
|
set('dockerHostId', option.dataset.hostId);
|
||||||
set('dockerWorkspacePath', option.dataset.path);
|
set('dockerWorkspacePath', option.dataset.path);
|
||||||
set('dockerCaseName', '');
|
// Pre-filled, NOT cleared: these two must differ from the source, but editing
|
||||||
set('dockerAdoptWorkdir', '');
|
// `/srv/app/api` into `/srv/app/web` beats retyping a long path, and the same
|
||||||
document.getElementById('dockerAdoptWorkdir')?.focus();
|
// 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() {
|
async _loadDockerContainerOptions() {
|
||||||
@@ -3327,6 +3368,17 @@ Object.assign(CodemanApp.prototype, {
|
|||||||
this.showToast('Enter the name of the running container to attach to', 'error');
|
this.showToast('Enter the name of the running container to attach to', 'error');
|
||||||
return;
|
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 {
|
try {
|
||||||
if (statusEl) {
|
if (statusEl) {
|
||||||
|
|||||||
@@ -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', () => {
|
describe('applying a source fills every field, including the two that must differ', () => {
|
||||||
const fn = ui.slice(ui.indexOf('applyDockerCloneSource()'), ui.indexOf('applyDockerCloneSource()') + 1400);
|
const fn = ui.slice(ui.indexOf('applyDockerCloneSource()'), ui.indexOf('dockerCloneGuard()'));
|
||||||
|
|
||||||
it('carries over container, host and workspace', () => {
|
it('carries over container, host and workspace', () => {
|
||||||
for (const id of ['dockerContainerName', 'dockerHostId', 'dockerWorkspacePath']) {
|
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', () => {
|
it('PRE-FILLS the case name and container workdir rather than clearing them', () => {
|
||||||
// Keeping either would pre-fill a value the server is certain to refuse —
|
// Editing `/srv/app/api` into `/srv/app/web` beats retyping a long path, and a
|
||||||
// the name as an existing case, the workdir as an exact twin.
|
// form with three fields mysteriously filled and two blank reads as broken.
|
||||||
expect(fn).toContain("set('dockerCaseName', '')");
|
// What stops an unchanged submit is the guard, not an empty field.
|
||||||
expect(fn).toContain("set('dockerAdoptWorkdir', '')");
|
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', () => {
|
it('remembers what it applied, so the guard can tell unchanged from similar', () => {
|
||||||
expect(fn).toMatch(/getElementById\('dockerAdoptWorkdir'\)\?\.focus\(\)/);
|
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', () => {
|
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', () => {
|
describe('the row is wired into the adoption panel', () => {
|
||||||
it('lives in the adopt-only block and starts hidden', () => {
|
it('lives in the adopt-only block and starts hidden', () => {
|
||||||
expect(html).toMatch(/id="dockerAdoptCloneRow"[^>]*hidden/);
|
expect(html).toMatch(/id="dockerAdoptCloneRow"[^>]*hidden/);
|
||||||
|
|||||||
Reference in New Issue
Block a user