Skip to content

fix(clone): treat Clone Repo's custom folder as a parent, like Create New - #774

Merged
Ark0N merged 4 commits into
Ark0N:masterfrom
opticon454:fix/clone-custom-folder-parent
Nov 6, 2026
Merged

Ark0N merged 4 commits into
Ark0N:masterfrom
opticon454:fix/clone-custom-folder-parent

Conversation

@opticon454

Copy link
Copy Markdown
Contributor

Summary

In Add Case, Clone Repo → Clone into a custom folder uses the folder you pick as the clone target itself, so picking a parent such as a projects folder fails with " already has files in it". Create New's custom folder and the default clone both put the case in a subfolder named after it, so Clone Repo was the odd one out.

Mode Where the case goes
Clone, default ~/codeman-cases/<name>
Create New, custom folder <parent>/<name> (the page sends the joined path)
Clone, custom folder, before <folder> itself
Clone, custom folder, after <folder>/<name>

This makes a custom folder behave as "use this folder instead of ~/codeman-cases" in both tabs.

Change

Page only, mirroring _newCaseTargetPath():

  • _cloneTargetPath() returns the parent field plus the case name, and the request's path carries that joined path.
  • The "Will clone into:" line shows the joined path and follows the case name as it is typed or filled from the URL.
  • The field is labelled Parent Folder like Create New's, and the hint texts say the repository goes into a subfolder named after the case.
  • Docs: the Clone Repo paragraph in docs/architecture-invariants.md and the Core Concepts wiki table.

The API is unchanged: path on POST /api/cases/clone is still the exact target, judged by prepareNewCasePath(), so the same checks apply (parent must exist, blocked trees, symlinks, empty or new target) and existing API callers are unaffected. The server still answers "already has files in it" if <folder>/<name> is not empty.

Behaviour change

A user who used to type the exact empty folder they wanted the repo in now gets <that folder>/<name>. They can type the parent and let the name supply the last segment.

Verification

  • New test/clone-custom-folder-ui.test.ts runs the real index.html and session-ui.js in JSDOM: the request carries <folder>/<name> (trailing slash trimmed), the preview follows a name edit, and no path is sent without the checkbox.
  • check:frontend-syntax and check:public-assets pass.
  • Full npm test: 688 files passed, 14,112 tests passed, 12 skipped, 1 failed. The failure is test/frontend-public-tooling.test.ts, which needs gitignored spreadsheet assets missing from my test checkout; it does not touch this change.
  • Not run: the browser suite, and a live Add Case clone in the real UI.

@opticon454

Copy link
Copy Markdown
Contributor Author

The one failed check is not from this change. It is test/routes/case-clone-custom-folder.test.ts > a name linked while the clone ran, which started two clones with the same name at once and expected exactly one to win; both succeeded (expected [ true, true ] to have a length of 1 but got 2). This PR touches only the Add Case page, docs and a new JSDOM test, not that route or its test, and the base has not moved (0 commits behind master).

The test depends on timing: the route re-reads linked-cases.json just before writing it and the code comment says that "only narrows the window", so two clones that both read before either writes both pass the check. On this branch, which carries the same server code as master, I ran that single test 15 times and it failed 5 times, so master's green runs are luck of the timing. In my own full run of this branch it passed (14,112 passed, 1 failed, the failure being the unrelated frontend-public-tooling).

I cannot re-run the job (no admin rights on the repository), so could you re-run the failed job? I did not push a dummy commit to trigger it.

If you want the race fixed, I can send that separately: serialize the final check-and-write of the registry inside the clone route, which would make the test deterministic and also stop a lost update in real use.

@Ark0N

Ark0N commented Nov 6, 2026

Copy link
Copy Markdown
Owner

Thanks @opticon454 for this! It makes Clone Repo's Clone into a custom folder field a parent folder: the repository lands in <folder>/<case name>, the same way Create New's custom folder works, and path on POST /api/cases/clone stays the exact target.

It does what the description says. A real clone through the tab in Chromium landed in <parent>/<name>, left the parent's existing files alone and registered as a linked case. Typecheck, lint, format and the frontend checks are green.

One design question is open. #744 made this field the exact clone target, and the 1.48.0 release notes announce it that way ("Clone Repo can clone straight into a folder you pick, empty or not yet created"). This PR reverses that, so the field has to be one or the other: the exact target, as shipped, or a parent, like Create New's.

  • A parent matches Create New and the default clone, and picking a projects folder that already has files no longer fails with "already has files in it".
  • The exact target works with an existing empty folder under any name. With a parent, the last path segment is always the case name, and cloneCase() limits that to letters, digits, - and _ (src/web/public/session-ui.js:4451). The tab can then no longer reach an empty folder such as /mnt/my.repo, or give a case a name that differs from its folder. feat(cases): clone a repository into a custom folder #744 built the direct clone so a mount point works; from the tab, that now holds only when the mount's name is a legal case name and the case takes that name. The API still accepts either.

That decision is not made yet. 68bd7e4 moved away from a parent on purpose, and neither the commit nor #744 records why. If that reason still applies, please add it here.

If the parent behaviour stays, these need to change:

  1. The preview goes stale after the preflight (src/web/public/session-ui.js:4403). _applyClonePreflight() replaces the case name with the server's suggestedName but does not refresh the preview. The first commit of feat(cases): clone a repository into a custom folder #744 (0bd15dd) refreshed it at three sites, and this PR restores two of them. With https://github.com/opticon454/bifrost?tab=readme-ov-file the page shows Will clone into: <parent>/bifrost-tab-readme-ov-file while the request sends <parent>/bifrost. Please add this.updateCloneCasePathPreview(); right after that line, and add a case to test/clone-custom-folder-ui.test.ts that drives _applyClonePreflight() with a suggestedName that differs from the local guess.
  2. The Clone Repo paragraph contradicts itself (docs/architecture-invariants.md:477). It still calls the checkbox the folder cloned into, "exactly as given (the case name adds no subfolder)", while your new text at line 488 says the field is a parent. Please keep that sentence about the API's path and take the checkbox out of it.
  3. Two wording leftovers (src/web/public/session-ui.js:4294 and :4458). The picker title and the empty-field toast still say "Choose the folder to clone into". Create New's say "create the case in", so "clone inside" fits here.

The red Unit & integration tests job is not from this PR, so there is nothing for you to fix there. It is test/routes/case-clone-custom-folder.test.ts ("a name linked while the clone ran"), a race already on master in code this PR does not touch, and #764 targets it.

This PR stays open while the question above is open.

@opticon454

Copy link
Copy Markdown
Contributor Author

Items 1 to 3 are done in a4bd35b7:

  1. _applyClonePreflight() now refreshes the preview after it replaces the case name, and test/clone-custom-folder-ui.test.ts has a case that drives it with a suggestedName that differs from the local guess (bifrost-tab-readme-ov-file becomes bifrost).
  2. The Clone Repo paragraph in docs/architecture-invariants.md keeps its sentence about the API's path (exactly as given, no case-name subfolder) and no longer mentions the checkbox there.
  3. The picker title and the empty-field toast now read "Choose the folder to clone inside".

The frontend checks and that test file pass. I did not rerun the full suite.

On the open design question (exact target as shipped in #744, or a parent): I have no recorded reason for 68bd7e4, and the point about /mnt/my.repo-style empty folders and names the case-name rule rejects is fair. I will come back with a decision rather than guess.

Thanks for pointing at #764 for the registry race; I had a fix for it in progress and will drop it.

@opticon454

Copy link
Copy Markdown
Contributor Author

Decision on the open design question: keep the parent behaviour.

68bd7e4 was a mistake on my side, not a considered choice. Most people pick a projects folder in the picker and expect the repository in a subfolder named after the case, which is what the default clone and Create New already do. There was no mount-point requirement behind it.

I accept the cost you pointed out: from the Add Case tab, an existing empty folder such as /mnt/my.repo is no longer reachable directly, and the case name has to be a legal case name. The API's path still takes an exact target, so a script or a mount point can still use it.

Your three items are already in a4bd35b7, so nothing else is pending on my side. The release note for #744 will read slightly differently once this merges; I have not touched it.

@Ark0N

Ark0N commented Nov 6, 2026

Copy link
Copy Markdown
Owner

Thanks @opticon454 for turning those three items around so quickly! This PR makes Clone Repo's Clone into a custom folder field a parent folder: the repository lands in <folder>/<case name>, as it does with Create New's custom folder, and path on POST /api/cases/clone stays the exact target.

All three items from my last comment are in at a4bd35b7, and both CI jobs are green on that commit. The preview refreshes after the preflight, and your new test fails without that line. The Clone Repo paragraph in docs/architecture-invariants.md no longer contradicts itself. The picker title and the toast read "Choose the folder to clone inside". A real clone through the tab in Chromium at that commit landed in <parent>/<name>, left the parent's files alone and registered as a linked case, and an existing empty <parent>/<name> was cloned into directly.

The design question is still open. #744 shipped this field as the exact clone target in 1.48.0, and this PR makes it a parent. With a parent, the last path segment from the tab is always the case name. The tab then can't reach an empty folder such as /mnt/my.repo, because it refuses that name (src/web/public/session-ui.js:4452), and a case can't have a name that differs from its folder. The API still accepts both. With the exact target, picking a projects folder that already has files keeps failing with "already has files in it". No decision has been made between the two.

If the parent behaviour stays, one small thing is left:

  1. The preview refresh on URL input has no test (src/web/public/session-ui.js:4348). If you delete that this.updateCloneCasePathPreview(); line, all four cases in test/clone-custom-folder-ui.test.ts stay green. Please add a case that sets a parent folder, sets cloneRepoUrl, calls app.onCloneUrlInput() and asserts that the preview shows the name taken from the URL. isCliAvailable() lives in settings-ui.js, which that test does not load, so stub it on app, and clear app._clonePreflightTimer at the end of the case.

This PR stays open while the question above is open.

@opticon454

Copy link
Copy Markdown
Contributor Author

Added in ec4c9011: a case in test/clone-custom-folder-ui.test.ts that sets a parent folder and cloneRepoUrl, calls app.onCloneUrlInput() and asserts the preview shows the name taken from the URL (/mnt/projects/bifrost). It stubs isCliAvailable() and clears _clonePreflightTimer as you described. I checked it fails (1 of 5) with the updateCloneCasePathPreview() line removed from onCloneUrlInput(), and passes with it. Prettier is clean.

On the design question: my comment from earlier today gives the decision. Keep the parent behaviour; 68bd7e4 was a mistake on my side with no mount-point requirement behind it, and I accept the cost for the tab.

@Ark0N

Ark0N commented Nov 6, 2026

Copy link
Copy Markdown
Owner

Thanks @opticon454 for the quick follow-up! This PR makes Clone Repo's Clone into a custom folder field a parent folder: the repository lands in <folder>/<case name>, as it does with Create New's custom folder, and path on POST /api/cases/clone stays the exact target.

The test from my last comment is in at ec4c9011, and both CI jobs are green on that commit. Removing any one of the three preview refresh calls now fails only the case that covers it. A real clone through the tab in Chromium landed in <parent>/<name>, left the parent's files alone and registered as a linked case. An existing empty <parent>/<name> was cloned into directly. A ~/projects parent, a missing parent and the cases folder as parent all behaved as documented.

The design question is still open. Thanks for your answer (keep the parent, 68bd7e4 was a mistake). It is on record here, and no further answer is needed from you. I have not decided between the exact target, as shipped in 1.48.0, and a parent.

If the parent behaviour stays, two small things are left:

  1. src/web/routes/case-routes.ts:751: the "already has files" error still says to pick an empty folder. The field is a parent now, so the user needs another case name or another parent instead. Please change the message to state the requirement, which holds for the tab and for API callers: ${customPath} already has files in it. The clone target must be empty or not exist yet. test/routes/case-clone-custom-folder.test.ts:122 matches /already has files/, so it keeps passing.
  2. test/clone-custom-folder-ui.test.ts:97: the clearTimeout in the new test does not clear the timer. That is Node's clearTimeout, and the timer belongs to the JSDOM window, so the preflight still fires 450 ms later. It does no harm, and my last comment did not say which one to call. Please return win from bootClone() and call win.clearTimeout(app._clonePreflightTimer).

This PR stays open while the question above is open.

@opticon454

Copy link
Copy Markdown
Contributor Author

Both done in the next push.

  1. case-routes.ts:751 now reads ${customPath} already has files in it. The clone target must be empty or not exist yet.
  2. bootClone() returns win, and the URL-input test calls win.clearTimeout(app._clonePreflightTimer), so the JSDOM timer is actually cleared.

test/clone-custom-folder-ui.test.ts and test/routes/case-clone-custom-folder.test.ts pass (28 of 28) and Prettier is clean.

@Ark0N

Ark0N commented Nov 6, 2026

Copy link
Copy Markdown
Owner

Thanks @opticon454 for the follow-up! This PR makes Clone Repo's Clone into a custom folder field a parent folder: the repository lands in <folder>/<case name>, as it does with Create New's custom folder, and path on POST /api/cases/clone stays the exact target.

Both items from my last comment are in at 51264ec0, and both CI jobs are green on that commit.

  • src/web/routes/case-routes.ts:751 now states the requirement. A clone through the tab onto a <parent>/<name> that holds files answered 409 ALREADY_EXISTS with the new text and left the files alone.
  • test/clone-custom-folder-ui.test.ts:97 now clears the timer on the JSDOM window.

At this commit a real clone through the tab in Chromium landed in <parent>/<name> beside the parent's own files and registered as a linked case, and a ~/projects parent was expanded by the server. A missing parent and the cases folder as parent were both refused with a clear message.

Nothing is left for you to change.

The design question is still open. #744 shipped this field as the exact clone target in 1.48.0, and this PR makes it a parent. A parent matches Create New and the default clone, and picking a projects folder that already has files works. The exact target works with an existing empty folder under any name, such as /mnt/my.repo, which the tab can't reach with a parent, because the last path segment is then the case name and the tab refuses that name. Your answer is on record here, and no further answer is needed from you. That decision has not been made.

This PR stays open while that question is open.

@Ark0N
Ark0N merged commit 9c39b2a into Ark0N:master Nov 6, 2026
2 checks passed
Ark0N pushed a commit that referenced this pull request Nov 6, 2026
…like Create New

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Ark0N pushed a commit that referenced this pull request Nov 6, 2026
…eview)

Clone Repo's custom folder became a parent that must already exist (#774),
while its picker still had no New folder button (#761 turned it on for
Create New only). A parent that does not exist yet was refused with no way
to make it from the form, so the same job worked on one tab and dead-ended
on the other.

The test pin that kept Clone off dated from when the field was the exact
target, which could be a folder not yet created. Clone moves to the on side
of that pin, so every picker in the file stays covered.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@opticon454
opticon454 deleted the fix/clone-custom-folder-parent branch November 6, 2026 09:45
@Ark0N

Ark0N commented Nov 6, 2026

Copy link
Copy Markdown
Owner

Thank you, this is in the 1.49.0 release, and the design question is settled: I decided to keep the parent, so the field works like Create New's, and path on POST /api/cases/clone stays the exact target. The release notes call out the change against 1.48.0.

Your head is merged as it was, including the clearer 409 text. On top of it: the picker has the New folder button, because a parent that does not exist yet was a dead end there, and the test file gained more parent shapes and the Browse path through to the request.

The race you found in test/routes/case-clone-custom-folder.test.ts is fixed by #764, in this release too, so that test is deterministic now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants