From 87097689005172ac9e4460d5c7d10e0c09722b58 Mon Sep 17 00:00:00 2001 From: dawn <93917549+dawNotPoi@users.noreply.github.com> Date: Wed, 16 Sep 2026 10:54:26 +0800 Subject: [PATCH 1/6] feat(forge): let the repository panel switch git remotes --- src-tauri/src/commands/forge.rs | 111 +++++++++++++++++- src-tauri/src/forge/settings.rs | 9 ++ src/components/forge/forge-page.test.tsx | 75 +++++++++++- src/components/forge/forge-page.tsx | 109 ++++++++++++++++- .../forge/forge-settings-dialog.test.tsx | 15 +++ .../forge/forge-settings-dialog.tsx | 6 + .../forge/forge-start-dialog.test.tsx | 2 + src/i18n/messages/ar.json | 3 +- src/i18n/messages/de.json | 3 +- src/i18n/messages/en.json | 3 +- src/i18n/messages/es.json | 3 +- src/i18n/messages/fr.json | 3 +- src/i18n/messages/ja.json | 3 +- src/i18n/messages/ko.json | 3 +- src/i18n/messages/pt.json | 3 +- src/i18n/messages/zh-CN.json | 3 +- src/i18n/messages/zh-TW.json | 3 +- src/lib/forge-settings.ts | 11 ++ src/lib/types.ts | 5 + 19 files changed, 354 insertions(+), 19 deletions(-) diff --git a/src-tauri/src/commands/forge.rs b/src-tauri/src/commands/forge.rs index f8f39987e3..5ad054996a 100644 --- a/src-tauri/src/commands/forge.rs +++ b/src-tauri/src/commands/forge.rs @@ -25,11 +25,17 @@ use crate::web::event_bridge::{emit_event, EventEmitter, WorkTaskChange, WORK_TA /// Hard cap for one reverse-lookup batch (a screen shows ~30 rows). const LOOKUP_KEYS_CAP: usize = 100; +/// The remote the panel reads when a folder has no saved selection. The +/// historical default, named once so both the resolution and its docs agree. +const DEFAULT_FORGE_REMOTE: &str = "origin"; /// Task card titles inherit the automation convention: 80 chars. const TITLE_CAP: usize = 80; #[derive(Debug, Clone, Serialize)] pub struct ForgeRemote { + /// Which remote this was resolved from — echoed back so the panel can show + /// the active choice instead of inferring it from the URL. + pub remote_name: String, pub server_host: String, pub owner_repo: String, pub remote_url: String, @@ -409,15 +415,26 @@ pub struct ForgeTaskLink { // ── shared business logic (both modes) ────────────────────────────────────── -/// The folder's `origin` remote, parsed into forge coordinates. `None` when -/// there is no origin or its URL is not a recognizable forge repo. +/// The folder's selected remote — the panel's saved choice, or `origin` when it +/// has none — parsed into forge coordinates. `None` when the folder has no such +/// remote or its URL is not a recognizable forge repo. pub async fn folder_forge_remote_core( db: &AppDatabase, folder_id: i32, ) -> Result, AppCommandError> { let folder = get_folder_core(db, folder_id).await?; + // Resolving the selection HERE is what makes every forge operation follow + // it: `resolve_folder_repo` calls this, so lists, comments, merges and task + // creation all read the repository the panel is showing. + let settings = forge::settings::load_effective(&db.conn, folder_id) + .await + .map_err(AppCommandError::db)?; + let remote_name = settings + .remote + .as_deref() + .unwrap_or(DEFAULT_FORGE_REMOTE); let output = crate::process::tokio_command("git") - .args(["-C", &folder.path, "remote", "get-url", "origin"]) + .args(["-C", &folder.path, "remote", "get-url", remote_name]) .output() .await .map_err(|e| AppCommandError::io_error("failed to run git").with_detail(e.to_string()))?; @@ -430,6 +447,7 @@ pub async fn folder_forge_remote_core( }; let profile = forge::host_profile(&db.conn, &server_host).await; Ok(Some(ForgeRemote { + remote_name: remote_name.to_string(), server_host, // A GitLab mounted under a relative URL root puts that mount path in // front of every repository path a git remote carries, while no API @@ -473,7 +491,7 @@ async fn resolve_folder_repo( .await? .ok_or_else(|| { AppCommandError::configuration_missing( - "this folder has no recognizable forge remote (origin)", + "this folder has no recognizable forge remote", ) })?; let auth = @@ -1245,6 +1263,91 @@ fn _assert_forge_error_converts(err: ForgeError) -> AppCommandError { mod tests { use super::*; + /// Minimal git fixture: a repo with remotes and no commits needed. + fn git_run(dir: &std::path::Path, args: &[&str]) { + let status = std::process::Command::new("git") + .args(args) + .current_dir(dir) + .status() + .expect("run git"); + assert!(status.success(), "git {args:?} failed"); + } + + /// The panel follows the folder's SAVED remote selection rather than a + /// hardcoded `origin` — the fork workflow (`origin` = your fork, `upstream` + /// = the parent) is the point of the switch. With nothing saved the read is + /// byte-for-byte the old behavior, which is what keeps existing installs + /// unchanged. + #[tokio::test] + async fn folder_forge_remote_follows_the_saved_selection() { + use crate::db::service::folder_service; + + let db = crate::db::test_helpers::fresh_in_memory_db().await; + let dir = tempfile::tempdir().expect("tempdir"); + git_run(dir.path(), &["init", "-q"]); + git_run(dir.path(), &["remote", "add", "origin", "https://github.com/me/app.git"]); + git_run(dir.path(), &["remote", "add", "upstream", "https://github.com/acme/app.git"]); + let folder = folder_service::add_folder(&db.conn, dir.path().to_str().unwrap()) + .await + .expect("folder row"); + + // Nothing saved → the historical default. + let remote = folder_forge_remote_core(&db, folder.id) + .await + .expect("resolve") + .expect("origin resolves"); + assert_eq!(remote.remote_name, "origin"); + assert_eq!(remote.owner_repo, "me/app"); + + // A folder-scoped save must change what the very next read resolves. + let settings = crate::forge::settings::ForgePanelSettings { + remote: Some("upstream".into()), + ..Default::default() + }; + crate::forge::settings::save(&db.conn, Some(folder.id), Some(settings)) + .await + .expect("save"); + + let remote = folder_forge_remote_core(&db, folder.id) + .await + .expect("resolve") + .expect("upstream resolves"); + assert_eq!(remote.remote_name, "upstream"); + assert_eq!(remote.owner_repo, "acme/app"); + } + + /// A selection naming a remote the folder does not have is `None`, the same + /// answer a missing `origin` gives. The panel explains it instead of + /// spending a request on the wrong repository. + #[tokio::test] + async fn a_missing_selected_remote_resolves_to_none() { + use crate::db::service::folder_service; + + let db = crate::db::test_helpers::fresh_in_memory_db().await; + let dir = tempfile::tempdir().expect("tempdir"); + git_run(dir.path(), &["init", "-q"]); + git_run(dir.path(), &["remote", "add", "origin", "https://github.com/me/app.git"]); + let folder = folder_service::add_folder(&db.conn, dir.path().to_str().unwrap()) + .await + .expect("folder row"); + + let settings = crate::forge::settings::ForgePanelSettings { + remote: Some("upstream".into()), + ..Default::default() + }; + crate::forge::settings::save(&db.conn, Some(folder.id), Some(settings)) + .await + .expect("save"); + + assert!( + folder_forge_remote_core(&db, folder.id) + .await + .expect("resolve") + .is_none(), + "a missing selected remote must not fall back to origin" + ); + } + const URL: &str = "https://github.com/acme/app/issues/7"; fn all_scenarios() -> [ForgeScenario; 4] { diff --git a/src-tauri/src/forge/settings.rs b/src-tauri/src/forge/settings.rs index 35709d55f8..bd097a6aa1 100644 --- a/src-tauri/src/forge/settings.rs +++ b/src-tauri/src/forge/settings.rs @@ -75,6 +75,11 @@ pub struct ForgePanelSettings { /// change (same contract as `stage_prompts`). #[serde(default)] pub scenario_prompts: BTreeMap, + /// Which git remote the panel reads the repository from. `None` is the + /// historical `origin`, so a blob written before this field existed keeps + /// behaving exactly as it did. + #[serde(default)] + pub remote: Option, } fn default_writeback() -> bool { @@ -88,6 +93,7 @@ impl Default for ForgePanelSettings { default_pr_scenario: None, writeback_default: default_writeback(), scenario_prompts: BTreeMap::new(), + remote: None, } } } @@ -117,6 +123,7 @@ impl ForgePanelSettings { fn normalized(mut self) -> Result { self.default_issue_scenario = trim_option(self.default_issue_scenario); self.default_pr_scenario = trim_option(self.default_pr_scenario); + self.remote = trim_option(self.remote); let mut prompts = BTreeMap::new(); for (key, text) in std::mem::take(&mut self.scenario_prompts) { let text = text.trim().to_string(); @@ -305,6 +312,7 @@ mod tests { ] .into_iter() .collect(), + remote: Some(" upstream ".into()), }; let stored = settings.normalized().expect("within the cap"); assert_eq!(stored.default_issue_scenario.as_deref(), Some("plan_first")); @@ -312,6 +320,7 @@ mod tests { assert!(!stored.writeback_default); assert_eq!(stored.scenario_prompts.get("all").map(String::as_str), Some("keep me")); assert!(!stored.scenario_prompts.contains_key("fix")); + assert_eq!(stored.remote.as_deref(), Some("upstream")); } /// Over the cap is refused, not truncated: this text rides in every task diff --git a/src/components/forge/forge-page.test.tsx b/src/components/forge/forge-page.test.tsx index a40a799c72..3a1211fb7e 100644 --- a/src/components/forge/forge-page.test.tsx +++ b/src/components/forge/forge-page.test.tsx @@ -42,6 +42,7 @@ import { import { useForgeRefreshStore } from "@/stores/forge-refresh-store" const REMOTE: ForgeRemote = { + remote_name: "origin", server_host: "github.com", owner_repo: "xintaofei/codeg", remote_url: "https://github.com/xintaofei/codeg.git", @@ -80,6 +81,7 @@ vi.mock("@/lib/api", () => ({ // each of them. forgeSettingsGet: vi.fn(), forgeSettingsSet: vi.fn(), + gitListRemotes: vi.fn(), })) vi.mock("@/lib/platform", () => ({ subscribe: vi.fn().mockResolvedValue(() => {}), @@ -111,7 +113,9 @@ import { forgeListLabels, forgeSetItemState, forgeSettingsGet, + forgeSettingsSet, forgeTabCount, + gitListRemotes, workTaskLookupBySource, } from "@/lib/api" @@ -223,6 +227,71 @@ beforeEach(() => { global: { writeback_default: true, scenario_prompts: {} }, folders: {}, }) + vi.mocked(gitListRemotes).mockResolvedValue([]) +}) + +describe("ForgePage remote picker", () => { + it("lists the folder's remotes and saves the picked one", async () => { + useAppWorkspaceStore.setState({ + folders: [ + { + id: 1, + name: "codeg", + path: "/repo", + parent_id: null, + kind: "regular", + }, + ] as never, + }) + vi.mocked(gitListRemotes).mockResolvedValue([ + { name: "origin", url: "https://github.com/me/codeg.git" }, + { name: "upstream", url: "https://github.com/xintaofei/codeg.git" }, + ]) + vi.mocked(forgeSettingsSet).mockResolvedValue({ + global: { writeback_default: true, scenario_prompts: {} }, + folders: { + "1": { + writeback_default: true, + scenario_prompts: {}, + remote: "upstream", + }, + }, + }) + vi.mocked(forgeListIssues).mockResolvedValue(listOf([])) + // A non-default field the picker does not edit: the save must carry it. + vi.mocked(forgeSettingsGet).mockResolvedValue({ + global: { + writeback_default: false, + scenario_prompts: { all: "Reply in English." }, + }, + folders: {}, + }) + + mount() + + await userEvent.click( + await screen.findByRole("combobox", { name: "Remote" }) + ) + await userEvent.click( + await screen.findByRole("option", { name: "upstream" }) + ) + + // Saved on the FOLDER scope, carrying the rest of the settings forward. + await waitFor(() => + expect(vi.mocked(forgeSettingsSet)).toHaveBeenCalledWith( + 1, + expect.objectContaining({ + remote: "upstream", + writeback_default: false, + scenario_prompts: { all: "Reply in English." }, + }) + ) + ) + // And the page re-reads the repository it is now pointed at. + await waitFor(() => + expect(vi.mocked(folderForgeRemote).mock.calls.length).toBeGreaterThan(1) + ) + }) }) describe("ForgePage list failures", () => { @@ -376,6 +445,7 @@ describe("ForgePage list failures", () => { */ describe("ForgePage sort control", () => { const GITEA: ForgeRemote = { + remote_name: "origin", server_host: "git.corp.example", owner_repo: "acme/app", remote_url: "https://git.corp.example/acme/app.git", @@ -418,6 +488,7 @@ describe("ForgePage sort control", () => { */ describe("ForgePage on a host that is neither forge", () => { const UNSUPPORTED: ForgeRemote = { + remote_name: "origin", server_host: "gitee.com", owner_repo: "someone/thing", remote_url: "https://gitee.com/someone/thing.git", @@ -1715,9 +1786,7 @@ describe("ForgePage writes", () => { vi.mocked(folderForgeRemote).mockResolvedValue(null) vi.mocked(forgeListIssues).mockResolvedValue(listOf([])) mount() - await screen.findByText( - "This folder has no recognizable forge remote (origin)" - ) + await screen.findByText("This folder has no recognizable forge remote") // Nowhere for the issue to go — the backend would refuse it, and a button // that can only fail is worse than no button. expect( diff --git a/src/components/forge/forge-page.tsx b/src/components/forge/forge-page.tsx index 34847ecdd3..428f3ab99e 100644 --- a/src/components/forge/forge-page.tsx +++ b/src/components/forge/forge-page.tsx @@ -9,6 +9,7 @@ import { type ReactNode, } from "react" import { useTranslations } from "next-intl" +import { toast } from "sonner" import { Check, ExternalLink, @@ -76,12 +77,15 @@ import { forgeListIssues, forgeListLabels, forgeSettingsGet, + forgeSettingsSet, forgeTabCount, + gitListRemotes, openSettingsWindow, workTaskLookupBySource, } from "@/lib/api" import { extractAppCommandError, + toErrorMessage, toLocalizedErrorMessage, type AppErrorTranslator, } from "@/lib/app-error" @@ -95,7 +99,10 @@ import { type ForgePageSize, } from "@/lib/forge-list-prefs" import { pageCount, pageSlots } from "@/lib/forge-pagination" -import { effectiveForgeSettings } from "@/lib/forge-settings" +import { + DEFAULT_FORGE_PANEL_SETTINGS, + effectiveForgeSettings, +} from "@/lib/forge-settings" import { openUrl, subscribe } from "@/lib/platform" import { cn } from "@/lib/utils" import type { @@ -108,6 +115,7 @@ import type { ForgeTab, ForgeSettingsStore, ForgeTaskLink, + GitRemote, } from "@/lib/types" import { useAppWorkspaceStore } from "@/stores/app-workspace-store" import { useForgeRefreshStore } from "@/stores/forge-refresh-store" @@ -561,9 +569,20 @@ export function ForgePage() { } return projectFolders[0]?.id ?? null }, [folderId, projectFolders]) + const effectiveFolderPath = useMemo( + () => projectFolders.find((f) => f.id === effectiveFolderId)?.path ?? null, + [projectFolders, effectiveFolderId] + ) const [remote, setRemote] = useState(null) const [remoteLoading, setRemoteLoading] = useState(false) + /** Every remote in the selected folder — the picker's options. Loaded with + * the folder, not with the resolved remote, so a selection that no longer + * resolves still lets the user move off it. */ + const [remotes, setRemotes] = useState([]) + /** Bumped after the selection is saved. The resolution effect depends on it, + * so the page re-reads the repository it is now pointed at. */ + const [remoteVersion, setRemoteVersion] = useState(0) /** Bumped when the backend reports it had this host's forge wrong. It is a * dependency of the remote lookup, so bumping it re-derives `provider` — * which is what makes the correction visible in the tab wording too, not @@ -678,7 +697,55 @@ export function ForgePage() { return () => { cancelled = true } - }, [effectiveFolderId, forgeCorrection]) + }, [effectiveFolderId, forgeCorrection, remoteVersion]) + + // The picker's options come straight from git, so the list is complete even + // when none of them is recognizable as a forge. + useEffect(() => { + if (effectiveFolderPath == null) { + setRemotes([]) + return + } + let cancelled = false + gitListRemotes(effectiveFolderPath) + .then((list) => { + if (!cancelled) setRemotes(list) + }) + .catch(() => { + if (!cancelled) setRemotes([]) + }) + return () => { + cancelled = true + } + }, [effectiveFolderPath]) + + // Switching the remote persists the choice on the FOLDER scope, then re-runs + // the resolution above: the rows on screen belong to the repository the + // backend would read next, so the old ones must not survive the switch. + const handlePickRemote = useCallback( + async (name: string) => { + if (effectiveFolderId == null) return + try { + // The store may not have landed yet. Reading it here avoids saving a + // folder row that would drop the user's standing instructions. + const store = settings ?? (await forgeSettingsGet()) + if (settings == null) setSettings(store) + const current = effectiveForgeSettings(store, effectiveFolderId) + // Spread the settings in force so every field this picker does NOT + // edit survives the save — enumerating them would silently drop a + // field added later. + const next = await forgeSettingsSet(effectiveFolderId, { + ...(current ?? DEFAULT_FORGE_PANEL_SETTINGS), + remote: name, + }) + setSettings(next) + setRemoteVersion((v) => v + 1) + } catch (e) { + toast.error(toErrorMessage(e)) + } + }, + [effectiveFolderId, settings] + ) /** * The remote only when codeg can actually read it. @@ -696,6 +763,13 @@ export function ForgePage() { */ const readable = remote?.supported ? remote : null + /** The folder's selected remote name even when it does not resolve — the + * picker must show what the folder is set to, not only what loaded. */ + const selectedRemoteName = useMemo( + () => effectiveForgeSettings(settings, effectiveFolderId)?.remote ?? null, + [settings, effectiveFolderId] + ) + /** Which list the rows belong to — see [`LoadedList`]. */ const listScope = `${effectiveFolderId}:${tab}` /** Which RESULT SET the badges count — see [`TabCounts`]. No tab, no page, @@ -1408,6 +1482,9 @@ export function ForgePage() { folderId={effectiveFolderId} onPickFolder={pickFolder} remote={remote} + remotes={remotes} + remoteName={remote?.remote_name ?? selectedRemoteName} + onPickRemote={handlePickRemote} /> {/* Only once a repository is resolved: without one there is nowhere @@ -1847,6 +1924,9 @@ function RepoBar({ folderId, onPickFolder, remote, + remotes, + remoteName, + onPickRemote, }: { folders: readonly FolderSelectOption[] folderId: number | null @@ -1854,6 +1934,12 @@ function RepoBar({ /** `null` until the folder resolves, or for a folder with no forge remote — * the picker still has to be usable, so only the right half goes away. */ remote: ForgeRemote | null + /** Every remote in the folder — the picker's options. */ + remotes: GitRemote[] + /** The selected remote. `remote.remote_name` wins when resolution succeeded, + * since that is what the rows on screen came from. */ + remoteName: string | null + onPickRemote: (name: string) => void }) { const t = useTranslations("Forge") @@ -1869,6 +1955,25 @@ function RepoBar({ title={t("pickFolder")} variant="ghost" /> + {remotes.length > 0 ? ( + + ) : null} {remote != null ? ( <> diff --git a/src/components/forge/forge-settings-dialog.test.tsx b/src/components/forge/forge-settings-dialog.test.tsx index ea98265365..0674e97a02 100644 --- a/src/components/forge/forge-settings-dialog.test.tsx +++ b/src/components/forge/forge-settings-dialog.test.tsx @@ -156,6 +156,21 @@ describe("ForgeSettingsDialog global scope", () => { expect(onSaved).toHaveBeenCalledWith({ global: settings, folders: {} }) }) + it("carries a stored remote through a save this dialog does not edit", async () => { + forgeSettingsGet.mockResolvedValue({ + global: { ...GLOBAL, remote: "upstream" }, + folders: {}, + }) + const user = userEvent.setup() + await mountLoaded() + + await user.click(screen.getByRole("button", { name: "Save" })) + + await waitFor(() => expect(forgeSettingsSet).toHaveBeenCalled()) + // The panel's picker owns this field; a settings save must not clear it. + expect(lastSave().settings.remote).toBe("upstream") + }) + it("keeps each scenario's instruction under its own segment, and marks the ones in use", async () => { const user = userEvent.setup() await mountLoaded() diff --git a/src/components/forge/forge-settings-dialog.tsx b/src/components/forge/forge-settings-dialog.tsx index a1db8d0c87..4aa4b0dae2 100644 --- a/src/components/forge/forge-settings-dialog.tsx +++ b/src/components/forge/forge-settings-dialog.tsx @@ -237,6 +237,12 @@ function ForgeSettingsScope({ default_issue_scenario: issueScenario, default_pr_scenario: prScenario, writeback_default: writeback, + // This dialog does not edit the remote. Carrying the stored value + // through is what keeps a settings save from clearing a choice + // made with the panel's picker. + remote: + effectiveForgeSettings(store, isGlobal ? null : scope) + ?.remote ?? null, // Blank entries are dropped here as well as server-side: the // object that comes back is what the page will hand the next // trigger dialog, and it should not carry keys that mean nothing. diff --git a/src/components/forge/forge-start-dialog.test.tsx b/src/components/forge/forge-start-dialog.test.tsx index 1254707605..4ea5a785cb 100644 --- a/src/components/forge/forge-start-dialog.test.tsx +++ b/src/components/forge/forge-start-dialog.test.tsx @@ -41,6 +41,7 @@ vi.mock("@/contexts/workbench-route-context", () => ({ })) const GITHUB: ForgeRemote = { + remote_name: "origin", server_host: "github.com", owner_repo: "o/r", remote_url: "https://github.com/o/r.git", @@ -48,6 +49,7 @@ const GITHUB: ForgeRemote = { supported: true, } const GITLAB: ForgeRemote = { + remote_name: "origin", server_host: "gitlab.com", owner_repo: "group/sub/app", remote_url: "https://gitlab.com/group/sub/app.git", diff --git a/src/i18n/messages/ar.json b/src/i18n/messages/ar.json index 08361d289d..a2f9d8f184 100644 --- a/src/i18n/messages/ar.json +++ b/src/i18n/messages/ar.json @@ -5617,8 +5617,9 @@ }, "Forge": { "title": "لوحة المستودع", + "remote": "remote", "pickFolder": "اختر مجلد مشروع", - "noRemote": "لا يحتوي هذا المجلد على remote معروف (origin)", + "noRemote": "لا يحتوي هذا المجلد على remote معروف", "errors": { "noAccount": "لا يوجد حساب {provider} مُهيّأ للمضيف {host}. أضف حسابًا من الإعدادات ← التحكم بالإصدارات لتحميل هذا المستودع.", "unsupportedHost": "لوحة المستودع تدعم GitHub و GitLab و Gitea فقط، و{host} لا يُعرَف كأي منها. إذا كان مثيلًا مستضافًا ذاتيًا من GitHub Enterprise أو GitLab أو Gitea أو Forgejo، فأضف له حسابًا من الإعدادات ← التحكم بالإصدارات.", diff --git a/src/i18n/messages/de.json b/src/i18n/messages/de.json index a4fcf64caa..cd0fb15404 100644 --- a/src/i18n/messages/de.json +++ b/src/i18n/messages/de.json @@ -5617,8 +5617,9 @@ }, "Forge": { "title": "Repository-Panel", + "remote": "Remote", "pickFolder": "Projektordner wählen", - "noRemote": "Dieser Ordner hat kein erkennbares Forge-Remote (origin)", + "noRemote": "Dieser Ordner hat kein erkennbares Forge-Remote", "errors": { "noAccount": "Für {host} ist kein {provider}-Konto konfiguriert. Füge unter Einstellungen → Versionskontrolle eines hinzu, um dieses Repository zu laden.", "unsupportedHost": "Das Repository-Panel unterstützt nur GitHub, GitLab und Gitea, und {host} wird als keines davon erkannt. Falls es sich um eine selbst gehostete GitHub-Enterprise-, GitLab-, Gitea- oder Forgejo-Instanz handelt, füge unter Einstellungen → Versionskontrolle ein Konto dafür hinzu.", diff --git a/src/i18n/messages/en.json b/src/i18n/messages/en.json index 7c84cab256..3dc0ae2408 100644 --- a/src/i18n/messages/en.json +++ b/src/i18n/messages/en.json @@ -5617,8 +5617,9 @@ }, "Forge": { "title": "Repository panel", + "remote": "Remote", "pickFolder": "Pick a project folder", - "noRemote": "This folder has no recognizable forge remote (origin)", + "noRemote": "This folder has no recognizable forge remote", "errors": { "noAccount": "No {provider} account is configured for {host}. Add one under Settings → Version Control to load this repository.", "unsupportedHost": "The repository panel supports GitHub, GitLab and Gitea only, and {host} is not recognized as any of them. If it is a self-hosted GitHub Enterprise, GitLab, Gitea or Forgejo instance, add an account for it under Settings → Version Control.", diff --git a/src/i18n/messages/es.json b/src/i18n/messages/es.json index 7d7af796b7..195613c9fa 100644 --- a/src/i18n/messages/es.json +++ b/src/i18n/messages/es.json @@ -5617,8 +5617,9 @@ }, "Forge": { "title": "Panel del repositorio", + "remote": "Remoto", "pickFolder": "Elige una carpeta de proyecto", - "noRemote": "Esta carpeta no tiene un remoto reconocible (origin)", + "noRemote": "Esta carpeta no tiene un remoto reconocible", "errors": { "noAccount": "No hay ninguna cuenta de {provider} configurada para {host}. Añade una en Configuración → Control de versiones para cargar este repositorio.", "unsupportedHost": "El panel del repositorio solo admite GitHub, GitLab y Gitea, y {host} no se reconoce como ninguno de ellos. Si es una instancia autoalojada de GitHub Enterprise, GitLab, Gitea o Forgejo, añade una cuenta para ella en Configuración → Control de versiones.", diff --git a/src/i18n/messages/fr.json b/src/i18n/messages/fr.json index 12da2f2c78..61964cc442 100644 --- a/src/i18n/messages/fr.json +++ b/src/i18n/messages/fr.json @@ -5617,8 +5617,9 @@ }, "Forge": { "title": "Panneau du dépôt", + "remote": "Remote", "pickFolder": "Choisir un dossier de projet", - "noRemote": "Ce dossier n'a pas de remote reconnaissable (origin)", + "noRemote": "Ce dossier n'a pas de remote reconnaissable", "errors": { "noAccount": "Aucun compte {provider} n'est configuré pour {host}. Ajoutez-en un dans Paramètres → Contrôle de version pour charger ce dépôt.", "unsupportedHost": "Le panneau du dépôt ne prend en charge que GitHub, GitLab et Gitea, et {host} n'est reconnu comme aucun d'entre eux. S'il s'agit d'une instance auto-hébergée de GitHub Enterprise, GitLab, Gitea ou Forgejo, ajoutez-y un compte dans Paramètres → Contrôle de version.", diff --git a/src/i18n/messages/ja.json b/src/i18n/messages/ja.json index be0fef3a5b..4d54d0f817 100644 --- a/src/i18n/messages/ja.json +++ b/src/i18n/messages/ja.json @@ -5617,8 +5617,9 @@ }, "Forge": { "title": "リポジトリパネル", + "remote": "リモート", "pickFolder": "プロジェクトフォルダを選択", - "noRemote": "このフォルダには認識できるリモート（origin）がありません", + "noRemote": "このフォルダには認識できるリモートがありません", "errors": { "noAccount": "{host} の {provider} アカウントが設定されていません。「設定 → バージョン管理」で追加するとこのリポジトリを読み込めます。", "unsupportedHost": "リポジトリパネルは GitHub、GitLab、Gitea にのみ対応しており、{host} はそのいずれとしても認識できません。自己ホスト型の GitHub Enterprise、GitLab、Gitea、Forgejo インスタンスであれば、「設定 → バージョン管理」でアカウントを追加してください。", diff --git a/src/i18n/messages/ko.json b/src/i18n/messages/ko.json index 43e1b7255d..45e8c6b638 100644 --- a/src/i18n/messages/ko.json +++ b/src/i18n/messages/ko.json @@ -5617,8 +5617,9 @@ }, "Forge": { "title": "리포지토리 패널", + "remote": "원격", "pickFolder": "프로젝트 폴더 선택", - "noRemote": "이 폴더에는 인식 가능한 원격 저장소(origin)가 없습니다", + "noRemote": "이 폴더에는 인식 가능한 원격 저장소가 없습니다", "errors": { "noAccount": "{host}에 대한 {provider} 계정이 설정되지 않았습니다. 설정 → 버전 관리에서 추가하면 이 저장소를 불러올 수 있습니다.", "unsupportedHost": "리포지토리 패널은 GitHub, GitLab, Gitea만 지원하며, {host}는 그중 어느 쪽으로도 인식되지 않습니다. 자체 호스팅 GitHub Enterprise, GitLab, Gitea 또는 Forgejo 인스턴스라면 설정 → 버전 관리에서 계정을 추가하세요.", diff --git a/src/i18n/messages/pt.json b/src/i18n/messages/pt.json index 095a75b598..be4334cc02 100644 --- a/src/i18n/messages/pt.json +++ b/src/i18n/messages/pt.json @@ -5617,8 +5617,9 @@ }, "Forge": { "title": "Painel do repositório", + "remote": "Remoto", "pickFolder": "Escolha uma pasta de projeto", - "noRemote": "Esta pasta não tem um remoto reconhecível (origin)", + "noRemote": "Esta pasta não tem um remoto reconhecível", "errors": { "noAccount": "Nenhuma conta {provider} configurada para {host}. Adicione uma em Configurações → Controle de versão para carregar este repositório.", "unsupportedHost": "O painel do repositório só oferece suporte a GitHub, GitLab e Gitea, e {host} não é reconhecido como nenhum deles. Se for uma instância auto-hospedada do GitHub Enterprise, GitLab, Gitea ou Forgejo, adicione uma conta para ela em Configurações → Controle de versão.", diff --git a/src/i18n/messages/zh-CN.json b/src/i18n/messages/zh-CN.json index dc4e9eeeed..286d0ce657 100644 --- a/src/i18n/messages/zh-CN.json +++ b/src/i18n/messages/zh-CN.json @@ -5617,8 +5617,9 @@ }, "Forge": { "title": "仓库面板", + "remote": "远端", "pickFolder": "选择项目文件夹", - "noRemote": "该文件夹没有可识别的代码托管远端（origin）", + "noRemote": "该文件夹没有可识别的代码托管远端", "errors": { "noAccount": "还没有为 {host} 配置 {provider} 账号。请在「设置 → 版本控制」中添加后再加载该仓库。", "unsupportedHost": "仓库面板目前仅支持 GitHub、GitLab 和 Gitea，而 {host} 不是其中任何一种。如果它是自建的 GitHub Enterprise、GitLab、Gitea 或 Forgejo 实例，请在「设置 → 版本控制」中为它添加账号。", diff --git a/src/i18n/messages/zh-TW.json b/src/i18n/messages/zh-TW.json index d5500e9e49..d142531179 100644 --- a/src/i18n/messages/zh-TW.json +++ b/src/i18n/messages/zh-TW.json @@ -5617,8 +5617,9 @@ }, "Forge": { "title": "儲存庫面板", + "remote": "遠端", "pickFolder": "選擇專案資料夾", - "noRemote": "該資料夾沒有可識別的程式碼託管遠端（origin）", + "noRemote": "該資料夾沒有可識別的程式碼託管遠端", "errors": { "noAccount": "尚未為 {host} 設定 {provider} 帳號。請先在「設定 → 版本控制」中新增，才能載入這個儲存庫。", "unsupportedHost": "儲存庫面板目前僅支援 GitHub、GitLab 與 Gitea，而 {host} 不屬於其中任何一種。如果它是自架的 GitHub Enterprise、GitLab、Gitea 或 Forgejo 執行個體，請在「設定 → 版本控制」中為它新增帳號。", diff --git a/src/lib/forge-settings.ts b/src/lib/forge-settings.ts index 1db8ac22d4..ab1dc8bed4 100644 --- a/src/lib/forge-settings.ts +++ b/src/lib/forge-settings.ts @@ -1,5 +1,16 @@ import type { ForgePanelSettings, ForgeSettingsStore } from "@/lib/types" +/** The built-in defaults for one scope — mirrors `ForgePanelSettings::default` + * (write-back on; everything else unset). Shared so a caller that needs a base + * to spread over does not enumerate the fields it does not edit. */ +export const DEFAULT_FORGE_PANEL_SETTINGS: ForgePanelSettings = { + default_issue_scenario: null, + default_pr_scenario: null, + writeback_default: true, + scenario_prompts: {}, + remote: null, +} + /** * Sentinel folder id of the global row — the same one the task settings dialog * uses for its own "all folders" scope, so the two surfaces speak one language. diff --git a/src/lib/types.ts b/src/lib/types.ts index 8aee63540c..eb3125a5dd 100644 --- a/src/lib/types.ts +++ b/src/lib/types.ts @@ -1988,6 +1988,9 @@ export interface ForgeChangedFileList { /** A folder's `origin` remote parsed into forge coordinates. */ export interface ForgeRemote { + /** Which remote this was resolved from — the panel shows it so the active + * choice is visible rather than inferred from the URL. */ + remote_name: string server_host: string owner_repo: string remote_url: string @@ -2077,6 +2080,8 @@ export interface ForgePanelSettings { /** Standing instructions appended after a scenario's built-in wording, * keyed by scenario id plus the reserved `all` (every scenario). */ scenario_prompts: Record + /** Which git remote the panel reads; absent/null = the historical `origin`. */ + remote?: string | null } /** Every scope of the panel's preferences — mirrors From 4f82a60c9117e8ed947564539a5ce93fe166baef Mon Sep 17 00:00:00 2001 From: dawn <123@qq.com> Date: Thu, 17 Sep 2026 17:27:19 +0800 Subject: [PATCH 2/6] fix(forge): key the panel's state on the repository, not the folder alone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Switching the remote re-points every request at another repository, so everything that was only true of the previous one has to go with it. The page number and the label selection keyed on the folder, which such a switch does not change; the list and count scopes carried no repository at all, so a response already in flight could land under the unchanged scope and paint the rows of the repository the reader had left; and the detail sheet asked for the signed-in account and the permitted merge methods by folder alone. All four now key on the folder AND the remote, derived during render so the reset is committed in the same pass that resolves the new remote — before any effect can fetch against it. A switch also claims a request generation of its own, which is what keeps a stale answer from being believed in the two states where no refetch is fired at all: a remote that resolves to nothing readable, and the frame between the teardown and the resolution. Both sides spell the repository the same way: the page hands the detail sheet the `server_host/owner_repo` string its own scopes are built from. --- .../forge/forge-issue-detail-sheet.test.tsx | 145 +++++++++++++++++ .../forge/forge-issue-detail-sheet.tsx | 87 ++++++---- src/components/forge/forge-page.test.tsx | 150 ++++++++++++++++++ src/components/forge/forge-page.tsx | 87 +++++++++- 4 files changed, 434 insertions(+), 35 deletions(-) diff --git a/src/components/forge/forge-issue-detail-sheet.test.tsx b/src/components/forge/forge-issue-detail-sheet.test.tsx index a013fd124e..6822e2deed 100644 --- a/src/components/forge/forge-issue-detail-sheet.test.tsx +++ b/src/components/forge/forge-issue-detail-sheet.test.tsx @@ -282,6 +282,7 @@ function mount( onRowUpdated?: (updated: ForgeIssueRow) => void onCommentPosted?: (item: { isPr: boolean; number: number }) => void folderId?: number | null + repo?: string | null } = {} ) { const onOpenChange = handlers.onOpenChange ?? vi.fn() @@ -294,6 +295,7 @@ function mount( row={item} link={link} folderId={handlers.folderId === undefined ? 7 : handlers.folderId} + repo={handlers.repo} onOpenChange={onOpenChange} onStart={onStart} onRowUpdated={onRowUpdated} @@ -304,6 +306,34 @@ function mount( return { onOpenChange, onStart, onRowUpdated, onCommentPosted, view } } +/** + * The panel as the PAGE renders it, repository included. + * + * The remote is a page-level fact handed down (see `repoKey` in + * `forge-page.tsx`), so the only way to rehearse a switch of it is to re-render + * this panel with another value — the folder does not move. + */ +function panelWithRepo( + item: ForgeIssueRow | null, + folderId: number, + repo: string | null +) { + return ( + + + + ) +} + beforeEach(() => { vi.clearAllMocks() // A thread that fits its pane, so nothing auto-loads unless a case says so: @@ -2676,6 +2706,121 @@ describe("ForgeIssueDetailSheet conversation rail", () => { const column = card?.previousElementSibling expect(column?.firstElementChild).toHaveClass("rounded-full") }) + + /** + * The repository is part of what the panel's own lookups are ABOUT, so a + * switch of it has to blank them, not just re-ask. + * + * `forgeIdentity` is asked for by FOLDER and answered about whatever remote + * that folder names — and the real backend reads the same folder, so the + * answer for the folder really does change under a switch. Keyed on the folder + * alone the panel went on naming the previous repository's account until its + * own request came back, over a row that had already been re-read from the new + * one. The window is the whole point: both requests are held open here, since + * an answer that resolved would paper over the stale one a moment later. + */ + it("names no account from the repository the switch left, not even for a frame", async () => { + const settle = new Map void>() + forgeIdentity.mockImplementation( + (folderId: number) => + new Promise((resolve) => { + settle.set(folderId, resolve) + }) + ) + + const { view } = mount(row(), null, { + folderId: 7, + repo: "github.com/me/codeg", + }) + await waitFor(() => expect(forgeIdentity).toHaveBeenCalledWith(7)) + settle.get(7)?.({ username: "on-the-fork", avatar_url: null }) + await waitFor(() => + expect( + screen.getByRole("img", { name: "Commenting as on-the-fork" }) + ).toBeInTheDocument() + ) + + // Same folder, other repository — the picker in action. + view.rerender(panelWithRepo(row(), 7, "github.com/acme/codeg-parent")) + + // The new repository's lookup has gone out and has not answered, and the + // fork's account must not be standing in for it. + await waitFor(() => expect(forgeIdentity).toHaveBeenCalledTimes(2)) + expect(screen.queryByRole("img", { name: /Commenting as/ })).toBeNull() + + settle.get(7)?.({ username: "on-the-parent", avatar_url: null }) + await waitFor(() => + expect( + screen.getByRole("img", { name: "Commenting as on-the-parent" }) + ).toBeInTheDocument() + ) + }) + + /** The same rule for the merge box: which methods the forge permits is a fact + * about the repository, and the box must not go on offering the fork's while + * the parent's is on screen. Held open, as the account lookup is — the claim + * is about the window before the new answer lands. */ + it("withholds the merge on a repository switch until the new one answers", async () => { + forgeChangeDetail.mockResolvedValue({ + number: 42, + base_ref: "main", + head_ref: "fix/timeout", + head_repo: null, + head_sha: "abc123", + draft: false, + state: "open", + mergeable: true, + merge_state: "clean", + additions: 1, + deletions: 1, + changed_files: 1, + commits: 1, + checks: { checks: [], available: true, partial: false }, + }) + // Held in a list rather than a `let … | null`: the resolve is stored from + // inside the mock, and TypeScript's control-flow analysis cannot see an + // assignment made in a callback, so it narrows a `let` back to `null` at + // the call site and refuses the call outright. + const settles: Array<(value: ForgeMergeOptions) => void> = [] + forgeMergeOptions + .mockResolvedValueOnce({ + methods: ["squash"], + default_method: "squash", + merge_strategy: "squash", + }) + .mockImplementationOnce( + () => + new Promise((resolve) => { + settles.push(resolve) + }) + ) + + const mergeButton = () => + screen.getByRole("button", { name: /^(Merge|Merging…)$/ }) + const { view } = mount(row({ is_pr: true }), null, { + folderId: 7, + repo: "github.com/me/codeg", + }) + await waitFor(() => expect(mergeButton()).toBeEnabled()) + + view.rerender( + panelWithRepo(row({ is_pr: true }), 7, "github.com/acme/codeg-parent") + ) + + // Blanked rather than carried over: the method the fork prefers is a claim + // about a repository this panel is no longer reading, and the box says so + // by not being willing to merge until the new repository has answered. + await waitFor(() => expect(forgeMergeOptions).toHaveBeenCalledTimes(2)) + expect(mergeButton()).toBeDisabled() + + // And the parent's own answer still lands when it comes. + settles[0]?.({ + methods: ["merge"], + default_method: "merge", + merge_strategy: "merge_commit", + }) + await waitFor(() => expect(mergeButton()).toBeEnabled()) + }) }) /** diff --git a/src/components/forge/forge-issue-detail-sheet.tsx b/src/components/forge/forge-issue-detail-sheet.tsx index 02a7015763..adf43f7746 100644 --- a/src/components/forge/forge-issue-detail-sheet.tsx +++ b/src/components/forge/forge-issue-detail-sheet.tsx @@ -1867,29 +1867,34 @@ function mergeMethodText( */ function useForgeIdentity( folderId: number | null, + repo: string | null | undefined, enabled: boolean ): ForgeIdentity | null { const [identity, setIdentity] = useState(null) - /** The folder the answer above describes. */ - const [shown, setShown] = useState(null) + /** The folder AND remote the answer above describes. Both, because the + * folder picks the repository only until the picker points it somewhere + * else — the account belongs to the repository, and one folder can name + * several in turn. */ + const [shown, setShown] = useState(null) const reqRef = useRef(0) + const key = `${folderId}:${repo}` // Absorbed during RENDER, as [`useMergeOptions`] does: an effect would commit // one frame naming the account of the repository the panel just left. Keyed - // on the FOLDER alone, so closing the panel keeps the answer rather than - // blanking the avatar every time it is reopened. - if (folderId !== shown) { - setShown(folderId) + // on the repository rather than on the folder alone — so closing the panel + // keeps the answer rather than blanking the avatar every time it is reopened, + // while a switch to another remote DOES blank it. + if (key !== shown) { + setShown(key) setIdentity(null) } useEffect(() => { // Claimed BEFORE the early return, so a run that asks for nothing still // invalidates whatever the last one had in flight. Otherwise a lookup for - // the folder the panel was last opened on lands after the reader has - // switched repositories — the reset above has already been and gone by - // then, because it keys on the folder and the folder stopped changing — - // and the next open names an account from the repository before this one. + // the repository the panel was last opened on lands after the reader has + // switched to another — the reset above has already been and gone by then + // — and the next open names an account from the repository before this one. const id = ++reqRef.current if (folderId == null || !enabled) return void forgeIdentity(folderId) @@ -1899,7 +1904,7 @@ function useForgeIdentity( .catch(() => { if (id === reqRef.current) setIdentity(null) }) - }, [folderId, enabled]) + }, [folderId, repo, enabled]) return identity } @@ -1924,18 +1929,21 @@ const FALLBACK_METHODS: readonly ForgeMergeMethod[] = ["merge"] */ function useMergeOptions( folderId: number | null, + repo: string | null | undefined, enabled: boolean ): ForgeMergeOptions | null { const [options, setOptions] = useState(null) - /** The folder the answer above describes, so a folder switch cannot leave - * one repository's permitted methods on another's button. */ - const [shown, setShown] = useState(null) + /** The folder AND remote the answer above describes, so neither a folder + * switch nor a switch of the remote within it can leave one repository's + * permitted methods on another's button. */ + const [shown, setShown] = useState(null) const reqRef = useRef(0) + const key = `${folderId}:${repo}` // Absorbed during RENDER, the same rule `useChangeDetail` follows: an effect // would commit one frame of the previous repository's menu. - if (folderId !== shown) { - setShown(folderId) + if (key !== shown) { + setShown(key) setOptions(null) } @@ -1955,7 +1963,7 @@ function useMergeOptions( }) } }) - }, [folderId, enabled]) + }, [folderId, repo, enabled]) return options } @@ -2209,6 +2217,7 @@ function MergeBox({ function Conversation({ row, folderId, + repo, identity, onCommentPosted, beforeComposer, @@ -2217,6 +2226,9 @@ function Conversation({ }: { row: ForgeIssueRow folderId: number | null + /** The repository `folderId` is pointed at — part of the thread's key, so a + * switch of the remote resets it exactly as a switch of the item does. */ + repo?: string | null /** Who a comment from here would be signed as — see [`useForgeIdentity`]. */ identity: ForgeIdentity | null onCommentPosted: (item: { isPr: boolean; number: number }) => void @@ -2267,16 +2279,19 @@ function Conversation({ - {/* Keyed by the ITEM, not by the row object: the page re-reads the row - from the list on every render, so identity changes whenever anything - behind the panel refreshes — and a thread that remounted on each of - those would re-fetch, lose its loaded pages and scroll the reader back - to the top. The panel is non-modal, though, so clicking a different - row swaps the item underneath without ever closing; the key is what - resets it when that happens. */} + {/* Keyed by the ITEM *and the repository*, not by the row object: the page + re-reads the row from the list on every render, so identity changes + whenever anything behind the panel refreshes — and a thread that + remounted on each of those would re-fetch, lose its loaded pages and + scroll the reader back to the top. The panel is non-modal, though, so + clicking a different row swaps the item underneath without ever + closing; the key is what resets it when that happens. The repository + belongs in the key for the same reason the item does — the same number + names a different item in another repository, and its comments would + be the wrong thread entirely. */} {folderId != null ? ( void /** Opens the page's trigger dialog on this item. */ onStart: () => void @@ -2440,11 +2469,11 @@ export function ForgeIssueDetailSheet({ change != null && row?.state === "open" && (detail.detail?.state ?? "open") === "open" - const mergeOptions = useMergeOptions(change?.folderId ?? null, canMerge) + const mergeOptions = useMergeOptions(change?.folderId ?? null, repo, canMerge) /** Held HERE, above the thread that remounts per item — the account is a - * property of the folder, not of the item being read. Gated on the panel + * property of the repository, not of the item being read. Gated on the panel * being open, because the drawer is mounted whether or not it is. */ - const identity = useForgeIdentity(folderId, row != null) + const identity = useForgeIdentity(folderId, repo, row != null) /** * The element the conversation is scrolled in, for the virtualized thread @@ -2707,6 +2736,7 @@ export function ForgeIssueDetailSheet({ { }) }) +/** + * What a remote switch must TEAR DOWN. + * + * The picker changes which repository every request is aimed at, so every piece + * of state that was only true of the previous one has to go with it. Left + * behind, each of these reads as a fact about the NEW repository: a page + * number, a label vocabulary, and the previous repository's rows — the last one + * painted by a response that was already in flight when the switch happened. + */ +describe("ForgePage remote switch", () => { + const UPSTREAM: ForgeRemote = { + remote_name: "upstream", + server_host: "github.com", + owner_repo: "acme/codeg-parent", + remote_url: "https://github.com/acme/codeg-parent.git", + provider: "github", + supported: true, + } + + function mountWithPicker() { + useAppWorkspaceStore.setState({ + folders: [ + { + id: 1, + name: "codeg", + path: "/repo", + parent_id: null, + kind: "regular", + }, + ] as never, + }) + vi.mocked(gitListRemotes).mockResolvedValue([ + { name: "origin", url: "https://github.com/me/codeg.git" }, + { name: "upstream", url: "https://github.com/acme/codeg-parent.git" }, + ]) + // The first resolution is the fork; every one after the pick is the + // parent, which is what makes the switch observable from the outside. + vi.mocked(folderForgeRemote) + .mockResolvedValueOnce(REMOTE) + .mockResolvedValue(UPSTREAM) + mount() + } + + async function pickUpstream(user: ReturnType) { + await user.click(await screen.findByRole("combobox", { name: "Remote" })) + await user.click(await screen.findByRole("option", { name: "upstream" })) + } + + it("returns to page 1 instead of asking the new repository for the old page", async () => { + const user = userEvent.setup() + vi.mocked(forgeListIssues).mockImplementation(async (_folderId, req) => + listOf([issue(req.page ?? 1, `row on page ${req.page}`)], { + page: req.page ?? 1, + total_count: 57, + has_next: (req.page ?? 1) < 3, + }) + ) + mountWithPicker() + await screen.findByText("row on page 1") + await user.click(screen.getByRole("button", { name: "Go to page 3" })) + await screen.findByText("row on page 3") + + const before = sentQueries().length + await pickUpstream(user) + + // The FIRST request aimed at the new repository is the one that matters: + // page 3 of one repository is a different slice of another, and a header + // saying "page 3" over rows the reader never asked for is the same bug the + // page-size control already avoids. Asserted over EVERY request since the + // pick rather than only the last one: a fetch for the old page can be + // overtaken by the refetch that corrects it, and the wasted round trip — + // and the rows it briefly paints — would go unnoticed. + await waitFor(() => { + expect(vi.mocked(folderForgeRemote).mock.calls.length).toBeGreaterThan(1) + }) + expect(lastQuery().page ?? 1).toBe(1) + expect( + sentQueries() + .slice(before) + .map((q) => q.page ?? 1) + ).not.toContain(3) + }) + + it("clears the label filter, which belongs to the old repository", async () => { + const user = userEvent.setup() + // The parent's vocabulary does NOT include `bug`. Left in place, the filter + // would come back empty and read as "this repository has no issues". + vi.mocked(forgeListIssues).mockResolvedValue(listOf([issue(1, "a row")])) + vi.mocked(forgeListLabels) + .mockResolvedValueOnce({ + labels: [{ name: "bug", color: "#d73a4a" }], + truncated: false, + }) + .mockResolvedValue({ + labels: [{ name: "feature", color: "#0e8a16" }], + truncated: false, + }) + mountWithPicker() + await screen.findByText("a row") + + await user.click(screen.getByRole("button", { name: "Labels" })) + await user.click(await screen.findByRole("option", { name: "bug" })) + // Closing the popover, so the click that follows reaches the picker. + await user.keyboard("{Escape}") + await waitFor(() => expect(lastQuery().labels).toEqual(["bug"])) + + await pickUpstream(user) + + await waitFor(() => { + expect(vi.mocked(folderForgeRemote).mock.calls.length).toBeGreaterThan(1) + }) + expect(lastQuery().labels ?? []).toEqual([]) + // And the label list on offer is the new repository's, not a filter built + // from names that only existed in the old one. + expect(vi.mocked(forgeListLabels).mock.calls.length).toBeGreaterThan(1) + }) + + it("never paints the previous repository's rows over the new one", async () => { + const user = userEvent.setup() + let releaseOld: ((value: ForgeIssueList) => void) | null = null + let calls = 0 + vi.mocked(forgeListIssues).mockImplementation(async () => { + calls += 1 + // The first page is held in flight and released by the test AFTER the + // switch; the second (the parent's) resolves immediately. + if (calls === 1) { + return new Promise((resolve) => { + releaseOld = resolve + }) + } + return listOf([issue(99, "row from the parent")]) + }) + mountWithPicker() + await screen.findByRole("combobox", { name: "Remote" }) + + await pickUpstream(user) + // The parent's rows land first… + await screen.findByText("row from the parent") + // …and only then does the response that was already in flight arrive, + // carrying a row from the fork the panel has left. + await act(async () => { + releaseOld?.(listOf([issue(1, "row from the fork")])) + await Promise.resolve() + }) + + expect(screen.queryByText("row from the fork")).toBeNull() + expect(screen.getByText("row from the parent")).toBeInTheDocument() + }) +}) + describe("ForgePage list failures", () => { it("renders the backend error's text, never the object's toString", async () => { vi.mocked(forgeListIssues).mockRejectedValue( diff --git a/src/components/forge/forge-page.tsx b/src/components/forge/forge-page.tsx index 428f3ab99e..0fd87fe3a8 100644 --- a/src/components/forge/forge-page.tsx +++ b/src/components/forge/forge-page.tsx @@ -633,6 +633,11 @@ export function ForgePage() { const [labelOptions, setLabelOptions] = useState([]) const [labelsTruncated, setLabelsTruncated] = useState(false) const reqRef = useRef(0) + /** The repository the generation above belongs to, and a counter kept in + * step with it. `reqRef` is the guard for EVERY request aimed at the current + * repository, so it has to be claimed when the repository changes and there + * is no request of ours to claim it — see the render-phase bump below. */ + const repoRef = useRef(null) /** Rows taken from a write, keyed by item — what `reconcile` writes back * over a list response that went out before the write. A ref, not state: it * has to be readable by a fetch already in flight, and changing it must @@ -763,6 +768,46 @@ export function ForgePage() { */ const readable = remote?.supported ? remote : null + /** + * WHICH repository everything below is about. + * + * The folder alone is not the answer, and the picker is why: one folder can + * be pointed at several repositories in turn, so the page number, the label + * vocabulary and the counted rows on screen are facts about a (folder, + * remote) PAIR. Keyed on the folder alone they outlive the switch, and each + * one then reads as a fact about the repository now on screen. + * + * A NAME rather than the object, so it is a stable fetch dependency: the + * resolution hands back a fresh object every time, and comparing those would + * re-run every fetch on each re-resolution of the SAME remote. + */ + const repoKey = + readable == null ? "none" : `${readable.server_host}/${readable.owner_repo}` + + /** + * The switch claims a generation of its own. + * + * `reqRef` is what decides whether an answer is still wanted, and today it + * happens to be safe without this: the refetch for the new repository runs on + * the commit that resolves it and takes the next number, so an answer still in + * the air loses. That safety is a consequence of the refetch happening at all, + * though — not of anything the switch does — and the two states where it does + * NOT happen are exactly the ones where a stale answer can still be believed: + * a remote that resolves to nothing readable (no refetch is fired, so no + * number is claimed), and the frame between the teardown and the resolution. + * Taking the number here states the rule directly — a repository change + * invalidates everything aimed at the last one — and needs no fetch to be + * fired for it to hold. + * + * During RENDER, so it is claimed in the same commit that resolves the new + * remote and before any effect can run. Absorbed, because this runs on every + * render and writing state unconditionally would loop. + */ + if (repoRef.current !== repoKey) { + repoRef.current = repoKey + reqRef.current += 1 + } + /** The folder's selected remote name even when it does not resolve — the * picker must show what the folder is set to, not only what loaded. */ const selectedRemoteName = useMemo( @@ -770,11 +815,16 @@ export function ForgePage() { [settings, effectiveFolderId] ) - /** Which list the rows belong to — see [`LoadedList`]. */ - const listScope = `${effectiveFolderId}:${tab}` + /** Which list the rows belong to — see [`LoadedList`]. Carries the remote, + * not just the folder: switching the picker swaps the repository under the + * same folder id, and a page read from one forge must never be shown as the + * other's. */ + const listScope = `${effectiveFolderId}:${repoKey}:${tab}` /** Which RESULT SET the badges count — see [`TabCounts`]. No tab, no page, - * no order: none of the three can change either number. */ - const countsScope = `${effectiveFolderId}:${stateFilter}:${assignedMe}:${labelFilter.join(LABEL_SCOPE_SEP)}:${search}` + * no order: none of the three can change either number. Remote included for + * the same reason as `listScope`: the two repositories have unrelated + * totals. */ + const countsScope = `${effectiveFolderId}:${repoKey}:${stateFilter}:${assignedMe}:${labelFilter.join(LABEL_SCOPE_SEP)}:${search}` /** * Everything that decides whether a row belongs on the page being shown: the * folder and tab, the filter set, and the order and page number that place it @@ -1048,18 +1098,35 @@ export function ForgePage() { // A different repository has a different label vocabulary, so a selection // made against the old one would filter by labels that may not exist here. + // The remote is part of "a different repository" — one folder can be pointed + // at a fork and then its parent — so the key is the pair, not the folder. // Derived during render rather than in an effect: this has to catch the // FALLBACK path too (the stored folder disappearing from the workspace), and // an effect would spend an extra render — and an extra request — doing it. - const [labelledFolder, setLabelledFolder] = useState(effectiveFolderId) - if (labelledFolder !== effectiveFolderId) { - setLabelledFolder(effectiveFolderId) + const [labelledScope, setLabelledScope] = useState( + `${effectiveFolderId}:${repoKey}` + ) + if (labelledScope !== `${effectiveFolderId}:${repoKey}`) { + setLabelledScope(`${effectiveFolderId}:${repoKey}`) if (labelFilter.length > 0) { setLabelFilter([]) setPage(1) } } + // The page number belongs to the repository as much as the label selection + // does: page 3 of a fork is a different slice of its parent, and asking the + // parent for it lands the reader on rows nobody chose. Switched the same way + // — during render, so the reset is committed in the same pass that the new + // repository resolves, BEFORE any effect can fetch the old page against it. + const [pagedScope, setPagedScope] = useState( + `${effectiveFolderId}:${repoKey}` + ) + if (pagedScope !== `${effectiveFolderId}:${repoKey}`) { + setPagedScope(`${effectiveFolderId}:${repoKey}`) + setPage(1) + } + // The repository's label vocabulary — once per repository, not per page: // labels barely change, and on GitHub this runs on the core quota rather than // search's much smaller one. Best-effort: a repository whose labels cannot be @@ -1764,6 +1831,12 @@ export function ForgePage() { // list was fetched with, so a folder switch (which closes the panel — // see the reset effect above) cannot leave the two disagreeing. folderId={effectiveFolderId} + // Which repository that folder is pointed AT. The panel's repository + // facts — the account a comment is signed as, the merge methods the + // forge permits — are asked for by folder, so the folder alone cannot + // tell the panel whether its answer is still about the repository on + // screen. Same spelling as the scopes above, from the same value. + repo={repoKey} onOpenChange={(open) => { if (!open) setDetailRow(null) }} From 0731f0e3f8fd4eb5a8c9b84acd6e19fec8d425a6 Mon Sep 17 00:00:00 2001 From: dawn <123@qq.com> Date: Fri, 18 Sep 2026 09:30:39 +0800 Subject: [PATCH 3/6] fix(forge): give the remote selection a store the settings dialog cannot drop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The picker's choice lived as a field of the panel-settings blob, and that blob is saved WHOLESALE by the trigger dialog — "use the global defaults" saves by dropping a folder's whole row, so the choice went with it, silently. The picker's own save had the mirror-image problem: it spread the settings in force and wrote them back, which detached the folder from the global row from the first pick on, and made every click a read-modify-write race against the dialog. The selection now lives under its own app_metadata key, one entry per folder and no global row: a remote names a git remote that exists in ONE folder's worktree, so there is nothing for an "all folders" default to mean. A folder with no entry reads the historical `origin`, and a blank or absent name clears the entry — absence IS the default answer. The picker gets its own commands (`forge_remote_get` / `forge_remote_set`, in both modes) and an explicit "Default (origin)" item, which is now the only way back to no choice: the settings dialog does not edit this at all, so without that item a folder that had picked a remote could never go back to the default. --- src-tauri/src/commands/forge.rs | 111 +++++++-- src-tauri/src/forge/mod.rs | 1 + src-tauri/src/forge/remotes.rs | 211 ++++++++++++++++++ src-tauri/src/forge/settings.rs | 15 +- src-tauri/src/lib.rs | 2 + src-tauri/src/web/handlers/forge.rs | 27 +++ src-tauri/src/web/router.rs | 8 + src/components/forge/forge-page.test.tsx | 73 +++--- src/components/forge/forge-page.tsx | 100 ++++++--- .../forge/forge-settings-dialog.test.tsx | 18 +- .../forge/forge-settings-dialog.tsx | 6 - src/i18n/messages/ar.json | 1 + src/i18n/messages/de.json | 1 + src/i18n/messages/en.json | 1 + src/i18n/messages/es.json | 1 + src/i18n/messages/fr.json | 1 + src/i18n/messages/ja.json | 1 + src/i18n/messages/ko.json | 1 + src/i18n/messages/pt.json | 1 + src/i18n/messages/zh-CN.json | 1 + src/i18n/messages/zh-TW.json | 1 + src/lib/api.ts | 24 ++ src/lib/forge-settings.ts | 1 - src/lib/types.ts | 16 +- 24 files changed, 523 insertions(+), 100 deletions(-) create mode 100644 src-tauri/src/forge/remotes.rs diff --git a/src-tauri/src/commands/forge.rs b/src-tauri/src/commands/forge.rs index 5ad054996a..e1b13d7b29 100644 --- a/src-tauri/src/commands/forge.rs +++ b/src-tauri/src/commands/forge.rs @@ -426,13 +426,14 @@ pub async fn folder_forge_remote_core( // Resolving the selection HERE is what makes every forge operation follow // it: `resolve_folder_repo` calls this, so lists, comments, merges and task // creation all read the repository the panel is showing. - let settings = forge::settings::load_effective(&db.conn, folder_id) + // The selection has a store of its own rather than living in the panel + // settings: the picker saves it, and a settings save — including the "use + // global defaults" drop — must not be able to take it away. See + // `forge::remotes`. + let selected = forge::remotes::load_selected(&db.conn, folder_id) .await .map_err(AppCommandError::db)?; - let remote_name = settings - .remote - .as_deref() - .unwrap_or(DEFAULT_FORGE_REMOTE); + let remote_name = selected.as_deref().unwrap_or(DEFAULT_FORGE_REMOTE); let output = crate::process::tokio_command("git") .args(["-C", &folder.path, "remote", "get-url", remote_name]) .output() @@ -1049,6 +1050,28 @@ pub async fn forge_settings_set_core( Ok(forge::settings::save(&db.conn, folder_id, settings).await?) } +/// Every folder's remote selection at once — what the picker reads. Held whole +/// for the same reason the panel settings are: switching folders costs no round +/// trip, and a selection that no longer resolves is still shown for what the +/// folder is set to. +pub async fn forge_remote_get_core( + db: &AppDatabase, +) -> Result { + Ok(forge::remotes::load(&db.conn).await?) +} + +/// Save ONE folder's selection and hand back every folder's as stored. `None` +/// (or a blank name) puts the folder back on the default remote — the picker's +/// "default (origin)" answer, and what lets a folder be moved off a choice +/// WITHOUT the settings dialog (which does not edit this at all). +pub async fn forge_remote_set_core( + db: &AppDatabase, + folder_id: i32, + remote: Option, +) -> Result { + Ok(forge::remotes::save(&db.conn, folder_id, remote).await?) +} + pub async fn work_task_lookup_by_source_core( db: &AppDatabase, mut source_keys: Vec, @@ -1252,6 +1275,24 @@ pub async fn forge_settings_set( forge_settings_set_core(&db, folder_id, settings).await } +#[cfg(feature = "tauri-runtime")] +#[cfg_attr(feature = "tauri-runtime", tauri::command)] +pub async fn forge_remote_get( + db: tauri::State<'_, AppDatabase>, +) -> Result { + forge_remote_get_core(&db).await +} + +#[cfg(feature = "tauri-runtime")] +#[cfg_attr(feature = "tauri-runtime", tauri::command)] +pub async fn forge_remote_set( + db: tauri::State<'_, AppDatabase>, + folder_id: i32, + remote: Option, +) -> Result { + forge_remote_set_core(&db, folder_id, remote).await +} + // AppCommandError ← ForgeError conversion lives in `forge::mod` (used above // via `?` and the explicit map for `source_key`). #[allow(unused)] @@ -1300,11 +1341,7 @@ mod tests { assert_eq!(remote.owner_repo, "me/app"); // A folder-scoped save must change what the very next read resolves. - let settings = crate::forge::settings::ForgePanelSettings { - remote: Some("upstream".into()), - ..Default::default() - }; - crate::forge::settings::save(&db.conn, Some(folder.id), Some(settings)) + crate::forge::remotes::save(&db.conn, folder.id, Some("upstream".into())) .await .expect("save"); @@ -1331,11 +1368,7 @@ mod tests { .await .expect("folder row"); - let settings = crate::forge::settings::ForgePanelSettings { - remote: Some("upstream".into()), - ..Default::default() - }; - crate::forge::settings::save(&db.conn, Some(folder.id), Some(settings)) + crate::forge::remotes::save(&db.conn, folder.id, Some("upstream".into())) .await .expect("save"); @@ -1348,6 +1381,54 @@ mod tests { ); } + /// The maintainer's reproducer as a regression: the trigger dialog's "use + /// the global defaults" save DROPS the folder's whole panel-settings row, + /// which used to take the picker's remote selection with it — a choice the + /// user had watched succeed, gone on the next resolve. The selection is not + /// part of that row any more (see `forge::remotes`), so no settings save of + /// any shape can reach it. + #[tokio::test] + async fn a_settings_save_cannot_clear_the_remote_selection() { + use crate::db::service::folder_service; + + let db = crate::db::test_helpers::fresh_in_memory_db().await; + let dir = tempfile::tempdir().expect("tempdir"); + git_run(dir.path(), &["init", "-q"]); + git_run(dir.path(), &["remote", "add", "origin", "https://github.com/me/app.git"]); + git_run(dir.path(), &["remote", "add", "upstream", "https://github.com/acme/app.git"]); + let folder = folder_service::add_folder(&db.conn, dir.path().to_str().unwrap()) + .await + .expect("folder row"); + + // The picker's write: the selection, on its own. + crate::forge::remotes::save(&db.conn, folder.id, Some("upstream".into())) + .await + .expect("save the selection"); + + // Every shape of panel-settings save: the folder's own row, the global + // row, and the drop that "use the global defaults" performs. + let settings = crate::forge::settings::ForgePanelSettings { + writeback_default: false, + ..Default::default() + }; + forge_settings_set_core(&db, Some(folder.id), Some(settings.clone())) + .await + .expect("folder row"); + forge_settings_set_core(&db, None, Some(settings)) + .await + .expect("global row"); + forge_settings_set_core(&db, Some(folder.id), None) + .await + .expect("drop the folder row"); + + let remote = folder_forge_remote_core(&db, folder.id) + .await + .expect("resolve") + .expect("upstream still resolves"); + assert_eq!(remote.remote_name, "upstream"); + assert_eq!(remote.owner_repo, "acme/app"); + } + const URL: &str = "https://github.com/acme/app/issues/7"; fn all_scenarios() -> [ForgeScenario; 4] { diff --git a/src-tauri/src/forge/mod.rs b/src-tauri/src/forge/mod.rs index bd3af8e7c2..e882a59827 100644 --- a/src-tauri/src/forge/mod.rs +++ b/src-tauri/src/forge/mod.rs @@ -10,6 +10,7 @@ pub mod envelope; pub mod gitea; pub mod github; pub mod gitlab; +pub mod remotes; pub mod settings; use std::sync::RwLock; diff --git a/src-tauri/src/forge/remotes.rs b/src-tauri/src/forge/remotes.rs new file mode 100644 index 0000000000..8e6a76f222 --- /dev/null +++ b/src-tauri/src/forge/remotes.rs @@ -0,0 +1,211 @@ +//! Which git remote each folder's forge panel reads. +//! +//! **Not a field of [`ForgePanelSettings`](super::settings::ForgePanelSettings)**, +//! and that separation is the point of this module. The two have opposite write +//! patterns: the picker saves this one on every click, while the panel settings +//! are a blob a dialog rewrites WHOLESALE — "use global defaults" saves by +//! dropping a folder's whole row. Sharing a blob made the picker's write detach +//! every other setting from the global row, and made the dialog's drop take the +//! selection with it: a choice the user watched succeed died without a word. +//! +//! One key, one map: the folders that have chosen a remote. **No global row**, +//! unlike the panel settings — a remote names a git remote that exists in ONE +//! folder's worktree, so there is nothing for an "all folders" default to mean. +//! A folder with no entry reads the historical `origin`; absence is the default +//! answer, so there is no third state to keep in sync with the entry itself. +//! +//! Stored as ONE JSON object in `app_metadata`. A save is a read-modify-write of +//! the single blob — the same shape the panel settings use — so a save against +//! one folder carries the others through untouched rather than replacing them +//! with whatever the saving client happened to be holding. + +use std::collections::BTreeMap; + +use sea_orm::DatabaseConnection; +use serde::{Deserialize, Serialize}; + +use crate::db::error::DbError; +use crate::db::service::app_metadata_service; + +/// `app_metadata` key holding the whole store. +const REMOTES_KEY: &str = "forge_panel_remotes"; + +/// Every folder's selection at once. +/// +/// Keyed by folder id: a folder with no entry is on the default remote, so the +/// map holds only the folders that have chosen one. +#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] +pub struct ForgeRemoteStore { + #[serde(default)] + pub folders: BTreeMap, +} + +impl ForgeRemoteStore { + /// The name this folder is set to, or `None` for the default. + pub fn selected(&self, folder_id: i32) -> Option<&str> { + self.folders.get(&folder_id).map(String::as_str) + } + + /// Apply one folder's save. Pure, so the arithmetic can be tested without a + /// database — [`save`] is this plus the read and the write. + /// + /// A blank or absent name CLEARS the entry (back to the default) rather + /// than storing an empty string the resolver would then have to know to + /// ignore — the same normalization [`super::settings`] applies to its own + /// option fields. + fn apply(&mut self, folder_id: i32, remote: Option) { + match trim(remote) { + Some(name) => { + self.folders.insert(folder_id, name); + } + None => { + self.folders.remove(&folder_id); + } + } + } +} + +fn trim(value: Option) -> Option { + value + .map(|v| v.trim().to_string()) + .filter(|v| !v.is_empty()) +} + +/// Read every folder's selection. Never fails on content: a blob this build +/// cannot parse (hand-edited, or written by a future one) reads as empty — the +/// default remote — rather than taking down every forge read in every folder. +pub async fn load(conn: &DatabaseConnection) -> Result { + let raw = app_metadata_service::get_value(conn, REMOTES_KEY).await?; + Ok(decode(raw.as_deref()).unwrap_or_default()) +} + +/// The one folder's selection — what the panel's resolution actually asks for. +pub async fn load_selected( + conn: &DatabaseConnection, + folder_id: i32, +) -> Result, DbError> { + Ok(load(conn).await?.selected(folder_id).map(str::to_string)) +} + +/// Write ONE folder's selection and hand back every folder's as stored. +pub async fn save( + conn: &DatabaseConnection, + folder_id: i32, + remote: Option, +) -> Result { + let mut store = load(conn).await?; + store.apply(folder_id, remote); + let encoded = serde_json::to_string(&store) + .map_err(|e| DbError::Validation(format!("remote selection not serializable: {e}")))?; + app_metadata_service::upsert_value(conn, REMOTES_KEY, &encoded).await?; + Ok(store) +} + +/// Decode a stored blob, or `None` for anything this build cannot read. +fn decode(raw: Option<&str>) -> Option { + serde_json::from_str(raw?).ok() +} + +#[cfg(test)] +mod tests { + use super::*; + + fn store_with(pairs: &[(i32, &str)]) -> ForgeRemoteStore { + let mut store = ForgeRemoteStore::default(); + for (id, name) in pairs { + store.apply(*id, Some((*name).to_string())); + } + store + } + + /// A folder with nothing saved is on the default, not on a stored empty + /// string the resolver would have to filter out. + #[test] + fn an_untouched_folder_has_no_selection_of_its_own() { + let store = ForgeRemoteStore::default(); + assert_eq!(store.selected(1), None); + assert_eq!(store_with(&[(1, "upstream")]).selected(2), None); + } + + /// The picker's own name goes in trimmed — a hand-edited blob must not + /// resolve against a remote whose name is spelled with spaces around it. + /// A blank or absent name CLEARS the entry: that is how the picker's + /// "default (origin)" answer is stored. + #[test] + fn saving_trims_the_name_and_clearing_removes_the_entry() { + let mut store = ForgeRemoteStore::default(); + store.apply(1, Some(" upstream ".into())); + assert_eq!(store.selected(1), Some("upstream")); + + store.apply(1, Some(" ".into())); + assert_eq!(store.selected(1), None); + assert!(store.folders.is_empty(), "blank is a clear, not an entry"); + + store.apply(1, Some("upstream".into())); + store.apply(1, None); + assert_eq!(store.selected(1), None); + // Idempotent: clearing an unset folder is fine. + store.apply(1, None); + assert!(store.folders.is_empty()); + } + + /// One folder's save must not rewrite another's — the whole store is + /// written back on every save, so this is the one way a picker click could + /// silently move a different folder. + #[test] + fn a_save_against_one_folder_leaves_the_others_untouched() { + let mut store = store_with(&[(1, "upstream"), (2, "backup")]); + store.apply(1, Some("origin".into())); + assert_eq!(store.selected(2), Some("backup")); + store.apply(1, None); + assert_eq!(store.selected(2), Some("backup")); + } + + /// JSON has no integer keys, so folder ids go out as strings — and a store + /// that could not read its own output would lose every selection on the + /// next load. + #[test] + fn folder_keys_survive_the_json_round_trip() { + let store = store_with(&[(42, "upstream")]); + let encoded = serde_json::to_string(&store).expect("serializable"); + assert!(encoded.contains("\"42\""), "{encoded}"); + assert_eq!(decode(Some(&encoded)).expect("decodes"), store); + } + + /// Anything that is not JSON at all is the default, not an error — this + /// read runs on every forge operation, in every folder. + #[test] + fn a_blob_this_build_cannot_parse_reads_as_empty() { + assert_eq!(decode(None).unwrap_or_default(), ForgeRemoteStore::default()); + assert_eq!(decode(Some("not json")).unwrap_or_default(), ForgeRemoteStore::default()); + // A shape from a future build keeps the fields this one knows. + let future = decode(Some(r#"{"folders":{"7":"upstream"},"scope":"worktree"}"#)) + .expect("unknown fields are ignored"); + assert_eq!(future.selected(7), Some("upstream")); + } + + /// Through a real database, because [`save`] is a READ-MODIFY-WRITE: the + /// pure tests above prove the arithmetic, and this proves the load and the + /// store are the same blob — one key typo and every save would land on + /// something the next load never reads. + #[tokio::test] + async fn selections_persist_independently_across_saves() { + let db = crate::db::test_helpers::fresh_in_memory_db().await; + + // Nothing stored is the default, not an error. + assert_eq!(load(&db.conn).await.expect("empty"), ForgeRemoteStore::default()); + + save(&db.conn, 3, Some("upstream".into())).await.expect("folder 3"); + save(&db.conn, 4, Some("backup".into())).await.expect("folder 4"); + let store = load(&db.conn).await.expect("reload"); + assert_eq!(store.selected(3), Some("upstream")); + assert_eq!(store.selected(4), Some("backup")); + assert_eq!(load_selected(&db.conn, 3).await.expect("selected"), Some("upstream".into())); + assert_eq!(load_selected(&db.conn, 5).await.expect("unset"), None); + + // Clearing one folder leaves the other alone. + save(&db.conn, 3, None).await.expect("clear folder 3"); + assert_eq!(load(&db.conn).await.expect("reload").selected(3), None); + assert_eq!(load_selected(&db.conn, 4).await.expect("folder 4"), Some("backup".into())); + } +} diff --git a/src-tauri/src/forge/settings.rs b/src-tauri/src/forge/settings.rs index bd097a6aa1..d80dfd03ea 100644 --- a/src-tauri/src/forge/settings.rs +++ b/src-tauri/src/forge/settings.rs @@ -15,6 +15,12 @@ //! covers a KIND of work item — how an issue should be handled as opposed to a //! review, which is a distinction the task engine has no word for. //! +//! What it does NOT hold either is the panel's remote selection: that lives in +//! [`super::remotes`], because the picker saves it on every click while this +//! blob is rewritten WHOLESALE by a dialog — and one field living in the +//! other's blob is how "use global defaults" came to destroy a choice the +//! picker had already saved. +//! //! Stored as ONE JSON blob in `app_metadata` — the global row and every //! override together — rather than a row per scope. The whole thing is read //! once per page load and once per trigger, and a save is a read-modify-write @@ -75,11 +81,6 @@ pub struct ForgePanelSettings { /// change (same contract as `stage_prompts`). #[serde(default)] pub scenario_prompts: BTreeMap, - /// Which git remote the panel reads the repository from. `None` is the - /// historical `origin`, so a blob written before this field existed keeps - /// behaving exactly as it did. - #[serde(default)] - pub remote: Option, } fn default_writeback() -> bool { @@ -93,7 +94,6 @@ impl Default for ForgePanelSettings { default_pr_scenario: None, writeback_default: default_writeback(), scenario_prompts: BTreeMap::new(), - remote: None, } } } @@ -123,7 +123,6 @@ impl ForgePanelSettings { fn normalized(mut self) -> Result { self.default_issue_scenario = trim_option(self.default_issue_scenario); self.default_pr_scenario = trim_option(self.default_pr_scenario); - self.remote = trim_option(self.remote); let mut prompts = BTreeMap::new(); for (key, text) in std::mem::take(&mut self.scenario_prompts) { let text = text.trim().to_string(); @@ -312,7 +311,6 @@ mod tests { ] .into_iter() .collect(), - remote: Some(" upstream ".into()), }; let stored = settings.normalized().expect("within the cap"); assert_eq!(stored.default_issue_scenario.as_deref(), Some("plan_first")); @@ -320,7 +318,6 @@ mod tests { assert!(!stored.writeback_default); assert_eq!(stored.scenario_prompts.get("all").map(String::as_str), Some("keep me")); assert!(!stored.scenario_prompts.contains_key("fix")); - assert_eq!(stored.remote.as_deref(), Some("upstream")); } /// Over the cap is refused, not truncated: this text rides in every task diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index b33af7db67..0383da1530 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -1823,6 +1823,8 @@ mod tauri_app { forge_commands::work_task_lookup_by_source, forge_commands::forge_settings_get, forge_commands::forge_settings_set, + forge_commands::forge_remote_get, + forge_commands::forge_remote_set, terminal_commands::terminal_spawn, terminal_commands::terminal_write, terminal_commands::terminal_resize, diff --git a/src-tauri/src/web/handlers/forge.rs b/src-tauri/src/web/handlers/forge.rs index b01a757530..b8280c6e0c 100644 --- a/src-tauri/src/web/handlers/forge.rs +++ b/src-tauri/src/web/handlers/forge.rs @@ -9,6 +9,7 @@ use serde::Deserialize; use crate::app_error::AppCommandError; use crate::app_state::AppState; use crate::commands::forge as core; +use crate::forge::remotes::ForgeRemoteStore; use crate::forge::settings::{ForgePanelSettings, ForgeSettingsStore}; use crate::forge::{ ChangeFilesQuery, ChangeMergeRequest, ChangeQuery, CommentDraft, CommentFilters, CountFilters, @@ -297,3 +298,29 @@ pub async fn forge_settings_set( core::forge_settings_set_core(&state.db, params.folder_id, params.settings).await?, )) } + +#[derive(Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct RemoteSetParams { + pub folder_id: i32, + /// The remote to read the folder from, or null for the default. A blank + /// name is the same answer — the store normalizes it (see + /// `forge::remotes`). + #[serde(default)] + pub remote: Option, +} + +pub async fn forge_remote_get( + Extension(state): Extension>, +) -> Result, AppCommandError> { + Ok(Json(core::forge_remote_get_core(&state.db).await?)) +} + +pub async fn forge_remote_set( + Extension(state): Extension>, + Json(params): Json, +) -> Result, AppCommandError> { + Ok(Json( + core::forge_remote_set_core(&state.db, params.folder_id, params.remote).await?, + )) +} diff --git a/src-tauri/src/web/router.rs b/src-tauri/src/web/router.rs index 664342481c..29282abbad 100644 --- a/src-tauri/src/web/router.rs +++ b/src-tauri/src/web/router.rs @@ -1526,6 +1526,14 @@ pub fn build_router( "/forge_settings_set", post(handlers::forge::forge_settings_set), ) + .route( + "/forge_remote_get", + post(handlers::forge::forge_remote_get), + ) + .route( + "/forge_remote_set", + post(handlers::forge::forge_remote_set), + ) .route( "/work_task_deliver_pr", post(handlers::work_task::work_task_deliver_pr), diff --git a/src/components/forge/forge-page.test.tsx b/src/components/forge/forge-page.test.tsx index 34b8548620..920d3aa2ad 100644 --- a/src/components/forge/forge-page.test.tsx +++ b/src/components/forge/forge-page.test.tsx @@ -81,6 +81,10 @@ vi.mock("@/lib/api", () => ({ // each of them. forgeSettingsGet: vi.fn(), forgeSettingsSet: vi.fn(), + // The panel's remote selections — read once on mount, written by the picker. + // Its own store, so the settings mocks above never answer for it. + forgeRemoteGet: vi.fn(), + forgeRemoteSet: vi.fn(), gitListRemotes: vi.fn(), })) vi.mock("@/lib/platform", () => ({ @@ -111,6 +115,8 @@ import { forgeCreateIssue, forgeListComments, forgeListLabels, + forgeRemoteGet, + forgeRemoteSet, forgeSetItemState, forgeSettingsGet, forgeSettingsSet, @@ -227,11 +233,13 @@ beforeEach(() => { global: { writeback_default: true, scenario_prompts: {} }, folders: {}, }) + // Nothing picked in any folder: the panel reads the default remote. + vi.mocked(forgeRemoteGet).mockResolvedValue({ folders: {} }) vi.mocked(gitListRemotes).mockResolvedValue([]) }) describe("ForgePage remote picker", () => { - it("lists the folder's remotes and saves the picked one", async () => { + function mountWithRemotes() { useAppWorkspaceStore.setState({ folders: [ { @@ -247,27 +255,15 @@ describe("ForgePage remote picker", () => { { name: "origin", url: "https://github.com/me/codeg.git" }, { name: "upstream", url: "https://github.com/xintaofei/codeg.git" }, ]) - vi.mocked(forgeSettingsSet).mockResolvedValue({ - global: { writeback_default: true, scenario_prompts: {} }, - folders: { - "1": { - writeback_default: true, - scenario_prompts: {}, - remote: "upstream", - }, - }, - }) vi.mocked(forgeListIssues).mockResolvedValue(listOf([])) - // A non-default field the picker does not edit: the save must carry it. - vi.mocked(forgeSettingsGet).mockResolvedValue({ - global: { - writeback_default: false, - scenario_prompts: { all: "Reply in English." }, - }, - folders: {}, - }) - mount() + } + + it("lists the folder's remotes and saves the picked one in its own store", async () => { + mountWithRemotes() + vi.mocked(forgeRemoteSet).mockResolvedValue({ + folders: { "1": "upstream" }, + }) await userEvent.click( await screen.findByRole("combobox", { name: "Remote" }) @@ -276,22 +272,41 @@ describe("ForgePage remote picker", () => { await screen.findByRole("option", { name: "upstream" }) ) - // Saved on the FOLDER scope, carrying the rest of the settings forward. + // Its OWN command. A picker click must not land in the panel-settings blob: + // that is what used to detach the folder from the global row, and what let + // a later "use global defaults" save destroy the choice. await waitFor(() => - expect(vi.mocked(forgeSettingsSet)).toHaveBeenCalledWith( - 1, - expect.objectContaining({ - remote: "upstream", - writeback_default: false, - scenario_prompts: { all: "Reply in English." }, - }) - ) + expect(vi.mocked(forgeRemoteSet)).toHaveBeenCalledWith(1, "upstream") ) + expect(vi.mocked(forgeSettingsSet)).not.toHaveBeenCalled() // And the page re-reads the repository it is now pointed at. await waitFor(() => expect(vi.mocked(folderForgeRemote).mock.calls.length).toBeGreaterThan(1) ) }) + + it("offers the default explicitly and clears the choice with it", async () => { + // The folder is on `upstream`. Nothing in the settings dialog edits the + // selection, so this item is the only way back to `origin` — and it has to + // CLEAR the entry rather than save the name, or the folder would look like + // it had chosen `origin` rather than gone back to the default. + vi.mocked(forgeRemoteGet).mockResolvedValue({ + folders: { "1": "upstream" }, + }) + mountWithRemotes() + vi.mocked(forgeRemoteSet).mockResolvedValue({ folders: {} }) + + await userEvent.click( + await screen.findByRole("combobox", { name: "Remote" }) + ) + await userEvent.click( + await screen.findByRole("option", { name: "Default (origin)" }) + ) + + await waitFor(() => + expect(vi.mocked(forgeRemoteSet)).toHaveBeenCalledWith(1, null) + ) + }) }) /** diff --git a/src/components/forge/forge-page.tsx b/src/components/forge/forge-page.tsx index 0fd87fe3a8..46baacab03 100644 --- a/src/components/forge/forge-page.tsx +++ b/src/components/forge/forge-page.tsx @@ -76,8 +76,9 @@ import { folderForgeRemote, forgeListIssues, forgeListLabels, + forgeRemoteGet, + forgeRemoteSet, forgeSettingsGet, - forgeSettingsSet, forgeTabCount, gitListRemotes, openSettingsWindow, @@ -99,10 +100,7 @@ import { type ForgePageSize, } from "@/lib/forge-list-prefs" import { pageCount, pageSlots } from "@/lib/forge-pagination" -import { - DEFAULT_FORGE_PANEL_SETTINGS, - effectiveForgeSettings, -} from "@/lib/forge-settings" +import { effectiveForgeSettings } from "@/lib/forge-settings" import { openUrl, subscribe } from "@/lib/platform" import { cn } from "@/lib/utils" import type { @@ -111,6 +109,7 @@ import type { ForgeLabel, ForgeProviderId, ForgeRemote, + ForgeRemoteStore, ForgeSort, ForgeTab, ForgeSettingsStore, @@ -135,6 +134,15 @@ const FOLDER_STORAGE_KEY = "forge:folderId" * matches, so every symbol in here read as one that does not exist. */ const LABEL_SCOPE_SEP = String.fromCharCode(0) +/** The remote the panel reads when a folder has no selection — mirrors + * `DEFAULT_FORGE_REMOTE` in `src-tauri/src/commands/forge.rs`. */ +const DEFAULT_REMOTE = "origin" + +/** The picker's "no selection" item. MUST NOT be a possible git remote name: + * a space cannot appear in a refname, which is what a remote name is — so this + * can never collide with a real remote the folder happens to have. */ +const REMOTE_DEFAULT_ITEM = " default" + /** Must mirror `NO_ACCOUNT_I18N_KEY` in src-tauri/src/forge/mod.rs. The key — * not the error `code` — is the discriminator: `configuration_missing` is a * generic code that other failures share, and offering "add an account" for @@ -629,6 +637,12 @@ export function ForgePage() { * than as a reason to wait. Held as the whole store rather than as one * folder's resolved values so switching folders costs no round trip. */ const [settings, setSettings] = useState(null) + /** Which git remote each folder reads — the picker's own store, held whole + * like the settings above so switching folders costs no round trip. Read + * and written ONLY by the picker: the settings dialog cannot reach it, and + * vice versa. `null` means "not loaded yet, or the read failed", which the + * picker treats as the default rather than as a reason to wait. */ + const [remoteStore, setRemoteStore] = useState(null) const [settingsOpen, setSettingsOpen] = useState(false) const [labelOptions, setLabelOptions] = useState([]) const [labelsTruncated, setLabelsTruncated] = useState(false) @@ -724,32 +738,27 @@ export function ForgePage() { } }, [effectiveFolderPath]) - // Switching the remote persists the choice on the FOLDER scope, then re-runs - // the resolution above: the rows on screen belong to the repository the - // backend would read next, so the old ones must not survive the switch. + // Switching the remote saves the choice on the folder, then re-runs the + // resolution above: the rows on screen belong to the repository the backend + // would read next, so the old ones must not survive the switch. + // + // Its OWN store, not the panel settings: that blob is rewritten wholesale by + // the settings dialog, and the picker writing into it is what used to detach + // the folder from the global row — and what let a later "use global + // defaults" save destroy a choice the user had just made. `null` is the + // picker's "default" item: no selection, so the folder reads `origin`. const handlePickRemote = useCallback( - async (name: string) => { + async (name: string | null) => { if (effectiveFolderId == null) return try { - // The store may not have landed yet. Reading it here avoids saving a - // folder row that would drop the user's standing instructions. - const store = settings ?? (await forgeSettingsGet()) - if (settings == null) setSettings(store) - const current = effectiveForgeSettings(store, effectiveFolderId) - // Spread the settings in force so every field this picker does NOT - // edit survives the save — enumerating them would silently drop a - // field added later. - const next = await forgeSettingsSet(effectiveFolderId, { - ...(current ?? DEFAULT_FORGE_PANEL_SETTINGS), - remote: name, - }) - setSettings(next) + const next = await forgeRemoteSet(effectiveFolderId, name) + setRemoteStore(next) setRemoteVersion((v) => v + 1) } catch (e) { toast.error(toErrorMessage(e)) } }, - [effectiveFolderId, settings] + [effectiveFolderId] ) /** @@ -809,10 +818,15 @@ export function ForgePage() { } /** The folder's selected remote name even when it does not resolve — the - * picker must show what the folder is set to, not only what loaded. */ + * picker must show what the folder is SET to, not only what loaded. Read + * from the selection store rather than from the resolution, which reports + * the default as a name whenever nothing was chosen. */ const selectedRemoteName = useMemo( - () => effectiveForgeSettings(settings, effectiveFolderId)?.remote ?? null, - [settings, effectiveFolderId] + () => + effectiveFolderId == null + ? null + : (remoteStore?.folders[String(effectiveFolderId)] ?? null), + [remoteStore, effectiveFolderId] ) /** Which list the rows belong to — see [`LoadedList`]. Carries the remote, @@ -1069,6 +1083,11 @@ export function ForgePage() { // straight back. A failure is silent on purpose — the trigger dialog falls // back to the built-in defaults, and a toast about preferences nobody asked // for yet would be noise over a page that works. + // + // The remote selections come along for the same ride and the same reason: + // they are the picker's own store, read once, and a failure leaves the + // picker on the default rather than blocking a page that reads repositories + // perfectly well. useEffect(() => { let cancelled = false forgeSettingsGet() @@ -1076,6 +1095,11 @@ export function ForgePage() { if (!cancelled) setSettings(s) }) .catch(() => {}) + forgeRemoteGet() + .then((s) => { + if (!cancelled) setRemoteStore(s) + }) + .catch(() => {}) const open = () => setSettingsOpen(true) window.addEventListener(OPEN_FORGE_SETTINGS_EVENT, open) return () => { @@ -1550,7 +1574,7 @@ export function ForgePage() { onPickFolder={pickFolder} remote={remote} remotes={remotes} - remoteName={remote?.remote_name ?? selectedRemoteName} + remoteName={selectedRemoteName} onPickRemote={handlePickRemote} /> @@ -2009,10 +2033,12 @@ function RepoBar({ remote: ForgeRemote | null /** Every remote in the folder — the picker's options. */ remotes: GitRemote[] - /** The selected remote. `remote.remote_name` wins when resolution succeeded, - * since that is what the rows on screen came from. */ + /** The folder's SAVED selection, or `null` when it is on the default. NOT + * the resolved name: a folder with nothing saved resolves to `origin`, and + * painting that as a picked remote would hide the fact that the folder is + * following the default — and the item that clears a choice. */ remoteName: string | null - onPickRemote: (name: string) => void + onPickRemote: (name: string | null) => void }) { const t = useTranslations("Forge") @@ -2029,7 +2055,12 @@ function RepoBar({ variant="ghost" /> {remotes.length > 0 ? ( - + onPickRemote(value === REMOTE_DEFAULT_ITEM ? null : value) + } + > + {/* The way OFF a choice. Without it a folder that picked a remote + could never go back to the default: this store is not editable + from the settings dialog, and picking `origin` would save a + choice rather than clear one. */} + + {t("remoteDefault", { name: DEFAULT_REMOTE })} + {remotes.map((r) => ( {r.name} diff --git a/src/components/forge/forge-settings-dialog.test.tsx b/src/components/forge/forge-settings-dialog.test.tsx index 0674e97a02..9c476e4f02 100644 --- a/src/components/forge/forge-settings-dialog.test.tsx +++ b/src/components/forge/forge-settings-dialog.test.tsx @@ -156,19 +156,23 @@ describe("ForgeSettingsDialog global scope", () => { expect(onSaved).toHaveBeenCalledWith({ global: settings, folders: {} }) }) - it("carries a stored remote through a save this dialog does not edit", async () => { - forgeSettingsGet.mockResolvedValue({ - global: { ...GLOBAL, remote: "upstream" }, - folders: {}, - }) + it("sends nothing about the remote, which the panel's picker owns", async () => { const user = userEvent.setup() await mountLoaded() await user.click(screen.getByRole("button", { name: "Save" })) await waitFor(() => expect(forgeSettingsSet).toHaveBeenCalled()) - // The panel's picker owns this field; a settings save must not clear it. - expect(lastSave().settings.remote).toBe("upstream") + // The selection lives in its own store and is written by the picker alone. + // A settings save must not carry a value a later read would treat as + // chosen — and must not be able to clear one either: this blob is dropped + // wholesale by "use the global defaults". + expect(Object.keys(lastSave().settings).sort()).toEqual([ + "default_issue_scenario", + "default_pr_scenario", + "scenario_prompts", + "writeback_default", + ]) }) it("keeps each scenario's instruction under its own segment, and marks the ones in use", async () => { diff --git a/src/components/forge/forge-settings-dialog.tsx b/src/components/forge/forge-settings-dialog.tsx index 4aa4b0dae2..a1db8d0c87 100644 --- a/src/components/forge/forge-settings-dialog.tsx +++ b/src/components/forge/forge-settings-dialog.tsx @@ -237,12 +237,6 @@ function ForgeSettingsScope({ default_issue_scenario: issueScenario, default_pr_scenario: prScenario, writeback_default: writeback, - // This dialog does not edit the remote. Carrying the stored value - // through is what keeps a settings save from clearing a choice - // made with the panel's picker. - remote: - effectiveForgeSettings(store, isGlobal ? null : scope) - ?.remote ?? null, // Blank entries are dropped here as well as server-side: the // object that comes back is what the page will hand the next // trigger dialog, and it should not carry keys that mean nothing. diff --git a/src/i18n/messages/ar.json b/src/i18n/messages/ar.json index a2f9d8f184..edfa791616 100644 --- a/src/i18n/messages/ar.json +++ b/src/i18n/messages/ar.json @@ -5618,6 +5618,7 @@ "Forge": { "title": "لوحة المستودع", "remote": "remote", + "remoteDefault": "الافتراضي ({name})", "pickFolder": "اختر مجلد مشروع", "noRemote": "لا يحتوي هذا المجلد على remote معروف", "errors": { diff --git a/src/i18n/messages/de.json b/src/i18n/messages/de.json index cd0fb15404..714a914fdb 100644 --- a/src/i18n/messages/de.json +++ b/src/i18n/messages/de.json @@ -5618,6 +5618,7 @@ "Forge": { "title": "Repository-Panel", "remote": "Remote", + "remoteDefault": "Standard ({name})", "pickFolder": "Projektordner wählen", "noRemote": "Dieser Ordner hat kein erkennbares Forge-Remote", "errors": { diff --git a/src/i18n/messages/en.json b/src/i18n/messages/en.json index 3dc0ae2408..8d0d1626b8 100644 --- a/src/i18n/messages/en.json +++ b/src/i18n/messages/en.json @@ -5618,6 +5618,7 @@ "Forge": { "title": "Repository panel", "remote": "Remote", + "remoteDefault": "Default ({name})", "pickFolder": "Pick a project folder", "noRemote": "This folder has no recognizable forge remote", "errors": { diff --git a/src/i18n/messages/es.json b/src/i18n/messages/es.json index 195613c9fa..f224603971 100644 --- a/src/i18n/messages/es.json +++ b/src/i18n/messages/es.json @@ -5618,6 +5618,7 @@ "Forge": { "title": "Panel del repositorio", "remote": "Remoto", + "remoteDefault": "Predeterminado ({name})", "pickFolder": "Elige una carpeta de proyecto", "noRemote": "Esta carpeta no tiene un remoto reconocible", "errors": { diff --git a/src/i18n/messages/fr.json b/src/i18n/messages/fr.json index 61964cc442..efb9248716 100644 --- a/src/i18n/messages/fr.json +++ b/src/i18n/messages/fr.json @@ -5618,6 +5618,7 @@ "Forge": { "title": "Panneau du dépôt", "remote": "Remote", + "remoteDefault": "Par défaut ({name})", "pickFolder": "Choisir un dossier de projet", "noRemote": "Ce dossier n'a pas de remote reconnaissable", "errors": { diff --git a/src/i18n/messages/ja.json b/src/i18n/messages/ja.json index 4d54d0f817..cf4bb6b852 100644 --- a/src/i18n/messages/ja.json +++ b/src/i18n/messages/ja.json @@ -5618,6 +5618,7 @@ "Forge": { "title": "リポジトリパネル", "remote": "リモート", + "remoteDefault": "デフォルト（{name}）", "pickFolder": "プロジェクトフォルダを選択", "noRemote": "このフォルダには認識できるリモートがありません", "errors": { diff --git a/src/i18n/messages/ko.json b/src/i18n/messages/ko.json index 45e8c6b638..a2bb86dbd1 100644 --- a/src/i18n/messages/ko.json +++ b/src/i18n/messages/ko.json @@ -5618,6 +5618,7 @@ "Forge": { "title": "리포지토리 패널", "remote": "원격", + "remoteDefault": "기본값({name})", "pickFolder": "프로젝트 폴더 선택", "noRemote": "이 폴더에는 인식 가능한 원격 저장소가 없습니다", "errors": { diff --git a/src/i18n/messages/pt.json b/src/i18n/messages/pt.json index be4334cc02..1839a54a8c 100644 --- a/src/i18n/messages/pt.json +++ b/src/i18n/messages/pt.json @@ -5618,6 +5618,7 @@ "Forge": { "title": "Painel do repositório", "remote": "Remoto", + "remoteDefault": "Padrão ({name})", "pickFolder": "Escolha uma pasta de projeto", "noRemote": "Esta pasta não tem um remoto reconhecível", "errors": { diff --git a/src/i18n/messages/zh-CN.json b/src/i18n/messages/zh-CN.json index 286d0ce657..9306e32019 100644 --- a/src/i18n/messages/zh-CN.json +++ b/src/i18n/messages/zh-CN.json @@ -5618,6 +5618,7 @@ "Forge": { "title": "仓库面板", "remote": "远端", + "remoteDefault": "默认（{name}）", "pickFolder": "选择项目文件夹", "noRemote": "该文件夹没有可识别的代码托管远端", "errors": { diff --git a/src/i18n/messages/zh-TW.json b/src/i18n/messages/zh-TW.json index d142531179..997fc13b65 100644 --- a/src/i18n/messages/zh-TW.json +++ b/src/i18n/messages/zh-TW.json @@ -5618,6 +5618,7 @@ "Forge": { "title": "儲存庫面板", "remote": "遠端", + "remoteDefault": "預設（{name}）", "pickFolder": "選擇專案資料夾", "noRemote": "該資料夾沒有可識別的程式碼託管遠端", "errors": { diff --git a/src/lib/api.ts b/src/lib/api.ts index e8e62b3fc8..58f52483e8 100644 --- a/src/lib/api.ts +++ b/src/lib/api.ts @@ -41,6 +41,7 @@ import type { ForgeMergeOptions, ForgePanelSettings, ForgeRemote, + ForgeRemoteStore, ForgeSettingsStore, ForgeSort, ForgeStateAction, @@ -5927,3 +5928,26 @@ export async function forgeSettingsSet( ): Promise { return getTransport().call("forge_settings_set", { folderId, settings }) } + +/** Every folder's remote selection at once — what the panel's picker reads. + * Held as the whole store so switching folders costs no round trip, and a + * selection that no longer resolves is still shown for what the folder is set + * to. */ +export async function forgeRemoteGet(): Promise { + return getTransport().call("forge_remote_get", {}) +} + +/** + * Save ONE folder's remote selection and get every folder's back as stored. + * + * `remote = null` (or a blank name) puts the folder back on the default + * remote — the picker's "default (origin)" answer. Its own command rather than + * a field on the settings save: the picker writes this on every click, and a + * settings save must not be able to take it away. + */ +export async function forgeRemoteSet( + folderId: number, + remote: string | null +): Promise { + return getTransport().call("forge_remote_set", { folderId, remote }) +} diff --git a/src/lib/forge-settings.ts b/src/lib/forge-settings.ts index ab1dc8bed4..5c0fe70ce7 100644 --- a/src/lib/forge-settings.ts +++ b/src/lib/forge-settings.ts @@ -8,7 +8,6 @@ export const DEFAULT_FORGE_PANEL_SETTINGS: ForgePanelSettings = { default_pr_scenario: null, writeback_default: true, scenario_prompts: {}, - remote: null, } /** diff --git a/src/lib/types.ts b/src/lib/types.ts index eb3125a5dd..5c132441cf 100644 --- a/src/lib/types.ts +++ b/src/lib/types.ts @@ -2080,8 +2080,6 @@ export interface ForgePanelSettings { /** Standing instructions appended after a scenario's built-in wording, * keyed by scenario id plus the reserved `all` (every scenario). */ scenario_prompts: Record - /** Which git remote the panel reads; absent/null = the historical `origin`. */ - remote?: string | null } /** Every scope of the panel's preferences — mirrors @@ -2102,6 +2100,20 @@ export interface ForgeSettingsStore { /** Reserved `scenario_prompts` key applied to every scenario. */ export const FORGE_SCENARIO_PROMPT_ALL = "all" +/** Which git remote each folder's forge panel reads — mirrors + * `forge::remotes::ForgeRemoteStore`. + * + * Deliberately NOT a field of `ForgePanelSettings`: the picker saves this on + * every click, while the panel settings are a blob the settings dialog + * rewrites wholesale — so one field living in the other's blob is how "use + * global defaults" came to destroy a choice the picker had already saved. */ +export interface ForgeRemoteStore { + /** Keyed by folder id (JSON has no integer keys, so they arrive as strings). + * A folder with no entry reads the historical `origin` — absence IS the + * default answer, so there is no global row to fall back to. */ + folders: Record +} + /** Discriminated trigger outcome — duplicate/mismatch are answers, not errors. */ export type ForgeCreateResult = | { outcome: "created"; task: WorkTask } From 9f837cd7f16748b53dce7e75446675b9979ad9a2 Mon Sep 17 00:00:00 2001 From: dawn <123@qq.com> Date: Fri, 18 Sep 2026 10:13:05 +0800 Subject: [PATCH 4/6] fix(forge): refuse a write aimed at a repository the folder has left MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The four write commands carried a folder id and nothing else, so a write followed whatever remote the selection named by the time it arrived: a second window, a browser tab or an old build holding a stale view could post, close, file or merge into a repository other than the one its header showed, and nothing reported it — the request was valid for the folder it named. Every write now carries the coordinates the panel is showing, flattened beside its own fields and optional, so a build that predates the check keeps working. The resolution compares them with what the folder's remote actually resolves to BEFORE spending a credential, and refuses a mismatch with an i18n key the panel recognises. The panel's answer is to re-resolve the repository rather than only report the refusal: the panel is stale, and re-resolving is the one thing that fixes it. Reads deliberately do not carry them: a stale read shows a wrong list rather than writing into the wrong place, and the cost of adding them is every read request struct and every frontend call site. --- src-tauri/src/commands/forge.rs | 152 +++++++++++++++-- src-tauri/src/forge/mod.rs | 119 +++++++++++++ .../forge/forge-issue-detail-sheet.test.tsx | 161 +++++++++++++++--- .../forge/forge-issue-detail-sheet.tsx | 151 +++++++++++++--- .../forge/forge-new-issue-dialog.test.tsx | 85 +++++++-- .../forge/forge-new-issue-dialog.tsx | 52 +++++- src/components/forge/forge-page.test.tsx | 52 +++++- src/components/forge/forge-page.tsx | 27 +++ src/i18n/messages/ar.json | 1 + src/i18n/messages/de.json | 1 + src/i18n/messages/en.json | 1 + src/i18n/messages/es.json | 1 + src/i18n/messages/fr.json | 1 + src/i18n/messages/ja.json | 1 + src/i18n/messages/ko.json | 1 + src/i18n/messages/pt.json | 1 + src/i18n/messages/zh-CN.json | 1 + src/i18n/messages/zh-TW.json | 1 + src/lib/api.ts | 33 +++- src/lib/app-error.ts | 14 ++ src/lib/types.ts | 16 ++ 21 files changed, 794 insertions(+), 78 deletions(-) diff --git a/src-tauri/src/commands/forge.rs b/src-tauri/src/commands/forge.rs index e1b13d7b29..67ade4f756 100644 --- a/src-tauri/src/commands/forge.rs +++ b/src-tauri/src/commands/forge.rs @@ -480,6 +480,16 @@ fn redact_userinfo(url: &str) -> String { } } +/// The folder's repository, or the configuration error every caller wants. +async fn folder_forge_remote_required( + db: &AppDatabase, + folder_id: i32, +) -> Result { + folder_forge_remote_core(db, folder_id).await?.ok_or_else(|| { + AppCommandError::configuration_missing("this folder has no recognizable forge remote") + }) +} + /// Resolve the folder's repository AND the credential to read it with — the /// two things every workbench read needs and neither of which the client may /// supply. @@ -488,19 +498,56 @@ async fn resolve_folder_repo( folder_id: i32, account_id: Option<&str>, ) -> Result<(ForgeRemote, forge::ResolvedAuth), AppCommandError> { - let remote = folder_forge_remote_core(db, folder_id) - .await? - .ok_or_else(|| { - AppCommandError::configuration_missing( - "this folder has no recognizable forge remote", - ) - })?; + let remote = folder_forge_remote_required(db, folder_id).await?; let auth = forge::resolve_forge_auth(&db.conn, remote.provider, &remote.server_host, account_id) .await?; Ok((remote, auth)) } +/// Resolve for a WRITE, whose caller also says which repository it believed it +/// was writing to. +/// +/// The belief is checked BEFORE the credential is looked up: a stale panel is +/// told its coordinates are stale — a fact it can act on — rather than about +/// an account it never asked for. A caller that names no coordinates (an older +/// client, or one with nothing readable on screen) is resolved exactly as +/// before, so nothing that worked stops working. +async fn resolve_folder_repo_for_write( + db: &AppDatabase, + folder_id: i32, + account_id: Option<&str>, + expected: Option<(&str, &str)>, +) -> Result<(ForgeRemote, forge::ResolvedAuth), AppCommandError> { + let remote = folder_forge_remote_required(db, folder_id).await?; + if let Some((host, repo)) = expected { + if remote.server_host != host || !forge::same_repo(&remote.owner_repo, repo) { + return Err(write_mismatch(&remote, host, repo)); + } + } + let auth = + forge::resolve_forge_auth(&db.conn, remote.provider, &remote.server_host, account_id) + .await?; + Ok((remote, auth)) +} + +/// The refusal a stale write gets. Carries the i18n key the panel recognises +/// so it can re-resolve instead of leaving the reader on a repository the +/// folder has already left — the same judgement the trigger path makes, in +/// words that fit a comment, a close or a merge (see the key's own note). +fn write_mismatch(remote: &ForgeRemote, expected_host: &str, expected_repo: &str) -> AppCommandError { + let actual = format!("{}/{}", remote.server_host, remote.owner_repo); + let expected = format!("{expected_host}/{expected_repo}"); + let mut params = std::collections::BTreeMap::new(); + params.insert("expected".to_string(), expected.clone()); + params.insert("actual".to_string(), actual.clone()); + AppCommandError::configuration_invalid(format!( + "this panel was showing {expected}, but the folder's remote is now {actual} — the \ + write was refused rather than sent to the wrong repository" + )) + .with_i18n(forge::WRITE_MISMATCH_I18N_KEY, params) +} + pub async fn forge_list_issues_core( db: &AppDatabase, folder_id: i32, @@ -613,7 +660,9 @@ pub async fn forge_create_comment_core( draft: forge::CommentDraft, ) -> Result { let (kind, number, body) = draft.resolve().map_err(AppCommandError::from)?; - let (remote, auth) = resolve_folder_repo(db, folder_id, draft.account_id.as_deref()).await?; + let (remote, auth) = + resolve_folder_repo_for_write(db, folder_id, draft.account_id.as_deref(), draft.expected.pair()) + .await?; Ok(match remote.provider { // No kind: a pull request IS an issue at GitHub, and one endpoint // serves both (`/pulls/{n}/comments` is the review-comment collection, @@ -643,7 +692,13 @@ pub async fn forge_set_item_state_core( request: forge::StateChangeRequest, ) -> Result { let (kind, number, action) = request.resolve().map_err(AppCommandError::from)?; - let (remote, auth) = resolve_folder_repo(db, folder_id, request.account_id.as_deref()).await?; + let (remote, auth) = resolve_folder_repo_for_write( + db, + folder_id, + request.account_id.as_deref(), + request.expected.pair(), + ) + .await?; Ok(match remote.provider { ForgeProvider::GitHub => { forge::github::set_item_state(&auth, &remote.owner_repo, kind, number, action).await? @@ -668,7 +723,9 @@ pub async fn forge_create_issue_core( draft: forge::NewIssueDraft, ) -> Result { let resolved = draft.resolve().map_err(AppCommandError::from)?; - let (remote, auth) = resolve_folder_repo(db, folder_id, draft.account_id.as_deref()).await?; + let (remote, auth) = + resolve_folder_repo_for_write(db, folder_id, draft.account_id.as_deref(), draft.expected.pair()) + .await?; Ok(match remote.provider { ForgeProvider::GitHub => { forge::github::create_issue(&auth, &remote.owner_repo, &resolved).await? @@ -796,7 +853,13 @@ pub async fn forge_merge_change_core( request: forge::ChangeMergeRequest, ) -> Result, AppCommandError> { let (number, method, head_sha) = request.resolve().map_err(AppCommandError::from)?; - let (remote, auth) = resolve_folder_repo(db, folder_id, request.account_id.as_deref()).await?; + let (remote, auth) = resolve_folder_repo_for_write( + db, + folder_id, + request.account_id.as_deref(), + request.expected.pair(), + ) + .await?; let head_sha = head_sha.as_deref(); Ok(match remote.provider { ForgeProvider::GitHub => { @@ -1429,6 +1492,73 @@ mod tests { assert_eq!(remote.owner_repo, "acme/app"); } + /// The maintainer's two-client scenario: one window changes the folder's + /// remote, the other sends a write carrying the coordinates it still has on + /// screen. The write must be REFUSED — never redirected into the repository + /// the selection now names — and the refusal has to be one the panel can + /// recognise (its i18n key) and re-resolve from. + #[tokio::test] + async fn a_write_with_stale_coordinates_is_refused_not_redirected() { + use crate::db::service::folder_service; + + let db = crate::db::test_helpers::fresh_in_memory_db().await; + let dir = tempfile::tempdir().expect("tempdir"); + git_run(dir.path(), &["init", "-q"]); + git_run(dir.path(), &["remote", "add", "origin", "https://github.com/me/app.git"]); + git_run(dir.path(), &["remote", "add", "upstream", "https://github.com/acme/app.git"]); + let folder = folder_service::add_folder(&db.conn, dir.path().to_str().unwrap()) + .await + .expect("folder row"); + + // Window B switches the folder to the parent while window A still + // shows the fork; A's write names the fork. + crate::forge::remotes::save(&db.conn, folder.id, Some("upstream".into())) + .await + .expect("switch"); + + let refused = resolve_folder_repo_for_write( + &db, + folder.id, + None, + Some(("github.com", "me/app")), + ) + .await + .expect_err("stale coordinates must be refused"); + assert!( + matches!( + refused.code, + crate::app_error::AppErrorCode::ConfigurationInvalid + ), + "{:?}", + refused.code + ); + assert_eq!(refused.i18n_key.as_deref(), Some(forge::WRITE_MISMATCH_I18N_KEY)); + let params = refused.i18n_params.expect("both repositories are named"); + assert_eq!(params.get("expected").map(String::as_str), Some("github.com/me/app")); + assert_eq!(params.get("actual").map(String::as_str), Some("github.com/acme/app")); + + // The coordinates the panel is ACTUALLY showing get past the check, and + // stop at the next gate (no account is configured here). That is what + // proves the refusal above came from the coordinate check rather than + // from the folder failing to resolve at all. + let past = resolve_folder_repo_for_write( + &db, + folder.id, + None, + Some(("github.com", "acme/app")), + ) + .await + .expect_err("no account"); + assert_eq!(past.i18n_key.as_deref(), Some(forge::NO_ACCOUNT_I18N_KEY)); + + // Naming nothing behaves exactly as it did before this check existed: + // a build that predates it keeps working. + let unnamed = resolve_folder_repo_for_write(&db, folder.id, None, None) + .await + .expect_err("no account"); + assert_eq!(unnamed.i18n_key.as_deref(), Some(forge::NO_ACCOUNT_I18N_KEY)); + } + const URL: &str = "https://github.com/acme/app/issues/7"; fn all_scenarios() -> [ForgeScenario; 4] { diff --git a/src-tauri/src/forge/mod.rs b/src-tauri/src/forge/mod.rs index e882a59827..aff54f33e4 100644 --- a/src-tauri/src/forge/mod.rs +++ b/src-tauri/src/forge/mod.rs @@ -194,6 +194,14 @@ pub const UNSUPPORTED_HOST_I18N_KEY: &str = "Forge.errors.unsupportedHost"; /// [`NO_ACCOUNT_I18N_KEY`] is. pub const WRONG_FORGE_I18N_KEY: &str = "Forge.errors.wrongForge"; +/// i18n key for the refusal a WRITE gets when the coordinates it carried no +/// longer match the folder's remote (see [`ExpectedCoordinates`]). Root-dotted +/// like the others, and its own key rather than the trigger's +/// `Forge.folderMismatch`: that sentence is written about an ISSUE's +/// repository, and reading it over a comment, a close or a merge names the +/// wrong thing. Same judgement, same recovery, its own words. +pub const WRITE_MISMATCH_I18N_KEY: &str = "Forge.writeMismatch"; + #[derive(Debug, thiserror::Error)] pub enum ForgeError { /// No usable account/token for the requested host (or the token is dead). @@ -392,6 +400,41 @@ pub fn parse_remote_url(url: &str) -> Option<(String, String)> { Some((host, normalize_repo(path)?)) } +/// Where the client believes the folder's repository is, carried by every +/// WRITE so a stale view cannot be redirected into another repository. +/// +/// The panel is not the only client: the same folder can be open in a second +/// window, in a browser tab against this server, or in an old build, and the +/// repository a folder reads is mutable state (see `forge::remotes`). A write +/// that names no repository follows whatever the selection says NOW — which is +/// how a comment meant for one repository lands in another. +/// +/// Both fields are optional in the wire form, and a client that sends neither +/// keeps the old behaviour exactly. Flattened into each request rather than +/// nested, so the write payloads keep the flat shape their other fields have. +#[derive(Debug, Clone, Default, serde::Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct ExpectedCoordinates { + #[serde(default)] + pub expected_server_host: Option, + #[serde(default)] + pub expected_owner_repo: Option, +} + +impl ExpectedCoordinates { + /// The pair to compare against, or `None` when the client named neither + /// side — or only one. Half a coordinate cannot be compared, and inventing + /// the other half would turn a stale client into a wrong refusal. + pub fn pair(&self) -> Option<(&str, &str)> { + let host = self.expected_server_host.as_deref()?.trim(); + let repo = self.expected_owner_repo.as_deref()?.trim(); + if host.is_empty() || repo.is_empty() { + return None; + } + Some((host, repo)) + } +} + /// Provenance snapshot stored in `work_task.source_meta` (JSON) and mirrored /// to the frontend as `ForgeSourceMeta` in `src/lib/types.ts`. #[derive(Debug, Clone, serde::Serialize, serde::Deserialize)] @@ -1008,6 +1051,8 @@ pub struct CommentDraft { pub body: String, #[serde(default)] pub account_id: Option, + #[serde(flatten)] + pub expected: ExpectedCoordinates, } impl CommentDraft { @@ -1081,6 +1126,8 @@ pub struct StateChangeRequest { pub action: ForgeStateAction, #[serde(default)] pub account_id: Option, + #[serde(flatten)] + pub expected: ExpectedCoordinates, } impl StateChangeRequest { @@ -1201,6 +1248,8 @@ pub struct ChangeMergeRequest { pub head_sha: Option, #[serde(default)] pub account_id: Option, + #[serde(flatten)] + pub expected: ExpectedCoordinates, } impl ChangeMergeRequest { @@ -1233,6 +1282,8 @@ pub struct NewIssueDraft { pub labels: Vec, #[serde(default)] pub account_id: Option, + #[serde(flatten)] + pub expected: ExpectedCoordinates, } /// A validated new issue — the only shape a provider client will take. @@ -1746,6 +1797,70 @@ mod tests { assert!(serde_json::to_value(&dead_token).unwrap().get("i18n_key").is_none()); } + /// Coordinates are only comparable when BOTH halves arrive. With one of + /// them missing there is nothing to compare, and inventing the other half + /// would turn a stale client into a wrong refusal. + #[test] + fn expected_coordinates_need_both_halves_to_be_comparable() { + let both = ExpectedCoordinates { + expected_server_host: Some(" github.com ".into()), + expected_owner_repo: Some(" me/app ".into()), + }; + assert_eq!(both.pair(), Some(("github.com", "me/app"))); + + assert_eq!(ExpectedCoordinates::default().pair(), None); + assert_eq!( + ExpectedCoordinates { + expected_server_host: Some("github.com".into()), + expected_owner_repo: None, + } + .pair(), + None, + "half a coordinate cannot be compared" + ); + assert_eq!( + ExpectedCoordinates { + expected_server_host: Some(" ".into()), + expected_owner_repo: Some("me/app".into()), + } + .pair(), + None + ); + } + + /// The shape a write actually puts on the wire: the pair rides FLAT beside + /// the request's own fields, and a payload from a build that predates the + /// check decodes to "named nothing" rather than failing to decode at all. + #[test] + fn a_write_payload_carries_its_coordinates_flat() { + let draft: CommentDraft = serde_json::from_str( + r#"{"kind":"issue","number":7,"body":"hi","expectedServerHost":"github.com","expectedOwnerRepo":"me/app"}"#, + ) + .expect("decodes"); + assert_eq!(draft.expected.pair(), Some(("github.com", "me/app"))); + + let old: CommentDraft = + serde_json::from_str(r#"{"kind":"issue","number":7,"body":"hi"}"#).expect("decodes"); + assert_eq!(old.expected.pair(), None); + + // The other three write payloads flatten the same pair. + let merge: ChangeMergeRequest = serde_json::from_str( + r#"{"number":7,"method":"merge","expectedServerHost":"github.com","expectedOwnerRepo":"me/app"}"#, + ) + .expect("decodes"); + assert_eq!(merge.expected.pair(), Some(("github.com", "me/app"))); + let state: StateChangeRequest = serde_json::from_str( + r#"{"kind":"pr","number":7,"action":"close","expectedServerHost":"github.com","expectedOwnerRepo":"me/app"}"#, + ) + .expect("decodes"); + assert_eq!(state.expected.pair(), Some(("github.com", "me/app"))); + let issue: NewIssueDraft = serde_json::from_str( + r#"{"title":"t","expectedServerHost":"github.com","expectedOwnerRepo":"me/app"}"#, + ) + .expect("decodes"); + assert_eq!(issue.expected.pair(), Some(("github.com", "me/app"))); + } + #[test] fn source_key_normalizes_and_validates() { assert_eq!( @@ -2217,6 +2332,7 @@ mod tests { number, body: body.into(), account_id: None, + expected: ExpectedCoordinates::default(), }; assert_eq!( draft("issue", 7, " looks fixed ").resolve().unwrap(), @@ -2278,6 +2394,7 @@ mod tests { body: body.map(str::to_string), labels: labels.into_iter().map(str::to_string).collect(), account_id: None, + expected: ExpectedCoordinates::default(), }; assert_eq!( draft(" Login times out ", Some(" steps "), vec![" bug ", "", "bug", "docs"]) @@ -2301,6 +2418,7 @@ mod tests { body: None, labels: distinct.clone(), account_id: None, + expected: ExpectedCoordinates::default(), } .resolve() .unwrap(); @@ -2314,6 +2432,7 @@ mod tests { body: None, labels: absurd, account_id: None, + expected: ExpectedCoordinates::default(), } .resolve() .unwrap() diff --git a/src/components/forge/forge-issue-detail-sheet.test.tsx b/src/components/forge/forge-issue-detail-sheet.test.tsx index 6822e2deed..8f80488267 100644 --- a/src/components/forge/forge-issue-detail-sheet.test.tsx +++ b/src/components/forge/forge-issue-detail-sheet.test.tsx @@ -29,6 +29,7 @@ import type { ForgeCheck, ForgeComment, ForgeCommentList, + ForgeExpectedRepo, ForgeIdentity, ForgeIssueRow, ForgeLabel, @@ -283,12 +284,17 @@ function mount( onCommentPosted?: (item: { isPr: boolean; number: number }) => void folderId?: number | null repo?: string | null + /** The pair a write carries — the repository the panel is showing. */ + expected?: ForgeExpectedRepo | null + /** Called when a write comes back refused as stale. */ + onStaleRepository?: () => void } = {} ) { const onOpenChange = handlers.onOpenChange ?? vi.fn() const onStart = handlers.onStart ?? vi.fn() const onRowUpdated = handlers.onRowUpdated ?? vi.fn() const onCommentPosted = handlers.onCommentPosted ?? vi.fn() + const onStaleRepository = handlers.onStaleRepository ?? vi.fn() const view = render( ) - return { onOpenChange, onStart, onRowUpdated, onCommentPosted, view } + return { + onOpenChange, + onStart, + onRowUpdated, + onCommentPosted, + onStaleRepository, + view, + } } /** @@ -989,13 +1004,19 @@ describe("ForgeIssueDetailSheet writes", () => { await user.click(screen.getByRole("button", { name: "Comment" })) await waitFor(() => - expect(forgeCreateComment).toHaveBeenCalledWith(7, { - kind: "issue", - number: 42, - // Trimmed before it goes out — a comment padded with what a keyboard - // left behind is one nobody meant to publish. - body: "looks fixed", - }) + expect(forgeCreateComment).toHaveBeenCalledWith( + 7, + { + kind: "issue", + number: 42, + // Trimmed before it goes out — a comment padded with what a keyboard + // left behind is one nobody meant to publish. + body: "looks fixed", + }, + // No repository named by this caller — see the test below for the one + // that does. + null + ) ) expect(await screen.findByText("looks fixed")).toBeInTheDocument() expect(screen.getByText("alice")).toBeInTheDocument() @@ -1038,6 +1059,93 @@ describe("ForgeIssueDetailSheet writes", () => { expect(submit).toBeDisabled() }) + /** What the backend answers with when the coordinates a write carried no + * longer match the folder's remote (see `WRITE_MISMATCH_I18N_KEY`). */ + const writeMismatch = { + code: "configuration_invalid", + message: + "this panel was showing github.com/me/app, but the folder's remote is now github.com/acme/app", + i18n_key: "Forge.writeMismatch", + i18n_params: { + expected: "github.com/me/app", + actual: "github.com/acme/app", + }, + } + + it("carries the repository this panel is showing on the post", async () => { + const user = userEvent.setup() + forgeListComments.mockResolvedValue(commentPage([])) + forgeCreateComment.mockResolvedValue(comment({ id: "991", body: "hi" })) + mount(row(), null, { + expected: { + expectedServerHost: "github.com", + expectedOwnerRepo: "me/app", + }, + }) + await screen.findByText("No comments yet") + + await user.type(screen.getByPlaceholderText("Leave a comment…"), "hi") + await user.click(screen.getByRole("button", { name: "Comment" })) + + await waitFor(() => + expect(forgeCreateComment).toHaveBeenCalledWith( + 7, + expect.objectContaining({ body: "hi" }), + { expectedServerHost: "github.com", expectedOwnerRepo: "me/app" } + ) + ) + }) + + it("hands a stale-repository refusal to the page instead of only reporting it", async () => { + const user = userEvent.setup() + forgeListComments.mockResolvedValue(commentPage([])) + forgeCreateComment.mockRejectedValue(writeMismatch) + const { onStaleRepository, onCommentPosted } = mount(row(), null, { + expected: { + expectedServerHost: "github.com", + expectedOwnerRepo: "me/app", + }, + }) + await screen.findByText("No comments yet") + + await user.type( + screen.getByPlaceholderText("Leave a comment…"), + "meant for the fork" + ) + await user.click(screen.getByRole("button", { name: "Comment" })) + + // The panel is stale, so re-resolving is the only fix — this is the + // callback the page turns into one. Nothing was published, and nothing was + // adopted as if it had been. + await waitFor(() => expect(onStaleRepository).toHaveBeenCalled()) + expect(onCommentPosted).not.toHaveBeenCalled() + }) + + it("does the same for a close the folder has moved out from under", async () => { + const user = userEvent.setup() + forgeSetItemState.mockRejectedValue(writeMismatch) + const { onStaleRepository, onRowUpdated } = mount(row(), null, { + expected: { + expectedServerHost: "github.com", + expectedOwnerRepo: "me/app", + }, + }) + + await user.click( + screen.getByRole("button", { name: "Close #42 on the forge" }) + ) + await user.click( + within(screen.getByRole("alertdialog")).getByRole("button", { + name: "Close", + }) + ) + + await waitFor(() => expect(onStaleRepository).toHaveBeenCalled()) + // Not flipped locally: the write never happened, and pretending it did is + // the guess this panel refuses to make everywhere else. + expect(onRowUpdated).not.toHaveBeenCalled() + }) + it("confirms a close, then adopts the row the forge answered with", async () => { const user = userEvent.setup() // GitHub's PATCH answers with bare label names on GitLab; here the point @@ -1060,11 +1168,15 @@ describe("ForgeIssueDetailSheet writes", () => { }) ) await waitFor(() => - expect(forgeSetItemState).toHaveBeenCalledWith(7, { - kind: "issue", - number: 42, - action: "close", - }) + expect(forgeSetItemState).toHaveBeenCalledWith( + 7, + { + kind: "issue", + number: 42, + action: "close", + }, + null + ) ) await waitFor(() => expect(onRowUpdated).toHaveBeenCalled()) const adopted = vi.mocked(onRowUpdated).mock.calls[0][0] as ForgeIssueRow @@ -2163,14 +2275,18 @@ describe("ForgeIssueDetailSheet merge box", () => { await user.click(screen.getByRole("button", { name: "Squash and merge" })) await waitFor(() => - expect(forgeMergeChange).toHaveBeenCalledWith(7, { - number: 42, - method: "squash", - // The commit the panel DECIDED on — its diff, its files and its checks - // all describe this one. Both forges refuse with a 409 if the branch - // has moved, which is the point of sending it. - headSha: "abc123", - }) + expect(forgeMergeChange).toHaveBeenCalledWith( + 7, + { + number: 42, + method: "squash", + // The commit the panel DECIDED on — its diff, its files and its checks + // all describe this one. Both forges refuse with a 409 if the branch + // has moved, which is the point of sending it. + headSha: "abc123", + }, + null + ) ) // The FORGE's row, not a local flip: GitHub has no merged state, and only // its answer knows this one landed rather than closed. @@ -2239,7 +2355,8 @@ describe("ForgeIssueDetailSheet merge box", () => { await waitFor(() => expect(forgeMergeChange).toHaveBeenCalledWith( 7, - expect.objectContaining({ headSha: "reviewed1" }) + expect.objectContaining({ headSha: "reviewed1" }), + null ) ) }) diff --git a/src/components/forge/forge-issue-detail-sheet.tsx b/src/components/forge/forge-issue-detail-sheet.tsx index adf43f7746..9833c9d3d3 100644 --- a/src/components/forge/forge-issue-detail-sheet.tsx +++ b/src/components/forge/forge-issue-detail-sheet.tsx @@ -96,6 +96,7 @@ import { forgeSetItemState, } from "@/lib/api" import { + isForgeWriteMismatch, type AppErrorTranslator, toLocalizedErrorMessage, } from "@/lib/app-error" @@ -110,6 +111,7 @@ import type { ForgeCheckList, ForgeCheckState, ForgeComment, + ForgeExpectedRepo, ForgeIdentity, ForgeIssueRow, ForgeMergeMethod, @@ -275,6 +277,8 @@ function CommentThread({ kind, number, identity, + expected, + onStaleRepository, onPosted, beforeComposer, viewportRef, @@ -287,6 +291,12 @@ function CommentThread({ * thread is keyed by the ITEM and remounts as the reader clicks the list, * while the identity is a property of the folder. */ identity: ForgeIdentity | null + /** The repository on screen, passed to the composer so a post is REFUSED + * rather than landing in whichever repository the folder reads by the time + * it arrives. See [`CommentComposer`]. */ + expected: ForgeExpectedRepo | null + /** That refusal reached the composer: the page re-resolves. */ + onStaleRepository: () => void /** A comment landed on the forge, and here it is. The caller bumps the * item's count so the header stops trailing the thread underneath it. */ onPosted: (comment: ForgeComment) => void @@ -487,6 +497,8 @@ function CommentThread({ kind={kind} number={number} identity={identity} + expected={expected} + onStaleRepository={onStaleRepository} onPosted={(comment) => { // Into its own slot, not into the paged collection — see `posted` // for why that ordering and that race both matter. Nothing is @@ -674,6 +686,8 @@ function CommentComposer({ kind, number, identity, + expected, + onStaleRepository, onPosted, }: { folderId: number @@ -682,6 +696,13 @@ function CommentComposer({ /** Who the comment would be signed as, or `null` while that is still being * resolved — or could not be. See [`useForgeIdentity`]. */ identity: ForgeIdentity | null + /** The repository the panel is showing, so the post can be REFUSED rather + * than land in whichever one the folder's selection names by the time it + * arrives. `null` = name nothing, which is the old behaviour. */ + expected: ForgeExpectedRepo | null + /** The refusal above means the panel is stale: hand it to the page, which + * re-resolves and tears this panel down with it. */ + onStaleRepository: () => void onPosted: (comment: ForgeComment) => void }) { const t = useTranslations("Forge") @@ -698,21 +719,46 @@ function CommentComposer({ setPosting(true) setFailure(null) try { - const comment = await forgeCreateComment(folderId, { - kind, - number, - body: trimmed, - }) + const comment = await forgeCreateComment( + folderId, + { + kind, + number, + body: trimmed, + }, + expected + ) // Only now — a draft cleared before the answer would lose what somebody // wrote to a network failure they cannot retry from. setBody("") onPosted(comment) } catch (error) { + if (isForgeWriteMismatch(error)) { + // The folder has moved to another repository, so this panel is stale + // and the page is about to re-resolve — which unmounts this composer + // and the strip with it. A toast survives the teardown, so the reason + // is still readable after the panel it belonged to is gone. + toast.error( + toLocalizedErrorMessage(error, tRoot as unknown as AppErrorTranslator) + ) + onStaleRepository() + return + } setFailure({ error }) } finally { setPosting(false) } - }, [folderId, kind, number, onPosted, posting, trimmed]) + }, [ + expected, + folderId, + kind, + number, + onPosted, + onStaleRepository, + posting, + tRoot, + trimmed, + ]) return (
@@ -846,6 +892,11 @@ type FileStatusLabelKey = const RAIL = "flex gap-2.5" const RAIL_BODY = "min-w-0 flex-1" +/** Stands in for a caller that wired no re-resolve — a fixture, a preview. A + * fresh arrow per render would also invalidate every `useCallback` that + * depends on it, which is the other reason this is one shared value. */ +const NO_OP = () => {} + /** * The gutter's own column, and what pins what sits in it. * @@ -2218,6 +2269,8 @@ function Conversation({ row, folderId, repo, + expected, + onStaleRepository, identity, onCommentPosted, beforeComposer, @@ -2229,6 +2282,10 @@ function Conversation({ /** The repository `folderId` is pointed at — part of the thread's key, so a * switch of the remote resets it exactly as a switch of the item does. */ repo?: string | null + /** The same repository as the pair every write carries. See + * [`ForgeIssueDetailSheet`] — this only passes it down. */ + expected: ForgeExpectedRepo | null + onStaleRepository: () => void /** Who a comment from here would be signed as — see [`useForgeIdentity`]. */ identity: ForgeIdentity | null onCommentPosted: (item: { isPr: boolean; number: number }) => void @@ -2296,6 +2353,8 @@ function Conversation({ kind={row.is_pr ? "pr" : "issue"} number={row.number} identity={identity} + expected={expected} + onStaleRepository={onStaleRepository} // The ITEM, not a row: this fires when the POST resolves, and by // then a close or a list load may have produced a newer copy that a // snapshot taken at submit time would overwrite. @@ -2343,6 +2402,8 @@ export function ForgeIssueDetailSheet({ link, folderId, repo, + expected = null, + onStaleRepository = NO_OP, onOpenChange, onStart, onRowUpdated, @@ -2371,6 +2432,21 @@ export function ForgeIssueDetailSheet({ * reads as "the folder's own identity" and behaves as it did before. */ repo?: string | null + /** + * The repository this panel is SHOWING, as the pair every write carries + * (see `ForgeExpectedRepo`). `repo` above is the same fact spelled for a + * key; this is the spelling the backend can compare, so a write cannot be + * redirected into whatever repository the folder reads by the time it + * arrives. `null` sends none, which is the old behaviour. + */ + expected?: ForgeExpectedRepo | null + /** + * A write came back refused because the folder has moved to another + * repository — the page's job to re-resolve, which re-renders this panel + * against the new one. The message is shown either way; this is the part + * that FIXES the panel rather than merely reporting it. + */ + onStaleRepository?: () => void onOpenChange: (open: boolean) => void /** Opens the page's trigger dialog on this item. */ onStart: () => void @@ -2526,11 +2602,15 @@ export function ForgeIssueDetailSheet({ if (row == null || folderId == null) return setChanging(true) try { - const updated = await forgeSetItemState(folderId, { - kind: row.is_pr ? "pr" : "issue", - number: row.number, - action, - }) + const updated = await forgeSetItemState( + folderId, + { + kind: row.is_pr ? "pr" : "issue", + number: row.number, + action, + }, + expected + ) setPendingAction(null) onRowUpdated(mergeForgeRowUpdate(row, updated)) } catch (error) { @@ -2539,11 +2619,14 @@ export function ForgeIssueDetailSheet({ toast.error( toLocalizedErrorMessage(error, tRoot as unknown as AppErrorTranslator) ) + // Refused because the folder has moved on: the page re-resolves, and + // this panel goes with it. The toast above outlives both. + if (isForgeWriteMismatch(error)) onStaleRepository() } finally { setChanging(false) } }, - [folderId, onRowUpdated, row, tRoot] + [expected, folderId, onRowUpdated, onStaleRepository, row, tRoot] ) const reloadDetail = detail.reload @@ -2552,16 +2635,20 @@ export function ForgeIssueDetailSheet({ if (row == null || folderId == null) return setMerging(true) try { - const updated = await forgeMergeChange(folderId, { - number: row.number, - method: pending.method, - // The commit the DIALOG was armed with, not whatever the panel holds - // now. The diff, the file list and the checks all describe that one, - // so a merge that quietly landed a newer one would land code nobody - // in this conversation ever saw. Both forges answer 409 if the branch - // has moved, in their own words. - headSha: pending.headSha, - }) + const updated = await forgeMergeChange( + folderId, + { + number: row.number, + method: pending.method, + // The commit the DIALOG was armed with, not whatever the panel holds + // now. The diff, the file list and the checks all describe that one, + // so a merge that quietly landed a newer one would land code nobody + // in this conversation ever saw. Both forges answer 409 if the branch + // has moved, in their own words. + headSha: pending.headSha, + }, + expected + ) setPendingMerge(null) // `null` is "it merged, and the row could not be read back" — GitHub's // merge response does not contain the pull request, so the row costs a @@ -2589,6 +2676,11 @@ export function ForgeIssueDetailSheet({ toast.error( toLocalizedErrorMessage(error, tRoot as unknown as AppErrorTranslator) ) + // A refusal because the folder has moved on is the one failure here + // the page can FIX (see the prop's note) — and it must, because every + // later action in this panel would be aimed at the same stale + // repository. + if (isForgeWriteMismatch(error)) onStaleRepository() // The confirmation is DISMISSED on failure, unlike the close/reopen // one that stays put. It has to be: the likeliest refusal is "Head // branch was modified. Review and try the merge again.", and the whole @@ -2604,7 +2696,16 @@ export function ForgeIssueDetailSheet({ setMerging(false) } }, - [folderId, onRowUpdated, reloadDetail, row, t, tRoot] + [ + expected, + folderId, + onRowUpdated, + onStaleRepository, + reloadDetail, + row, + t, + tRoot, + ] ) if (row == null) return null @@ -2737,6 +2838,8 @@ export function ForgeIssueDetailSheet({ row={row} folderId={folderId} repo={repo} + expected={expected} + onStaleRepository={onStaleRepository} identity={identity} onCommentPosted={onCommentPosted} viewportRef={viewportRef} @@ -2796,6 +2899,8 @@ export function ForgeIssueDetailSheet({ row={row} folderId={folderId} repo={repo} + expected={expected} + onStaleRepository={onStaleRepository} identity={identity} onCommentPosted={onCommentPosted} viewportRef={viewportRef} diff --git a/src/components/forge/forge-new-issue-dialog.test.tsx b/src/components/forge/forge-new-issue-dialog.test.tsx index b913536940..92cd7740b6 100644 --- a/src/components/forge/forge-new-issue-dialog.test.tsx +++ b/src/components/forge/forge-new-issue-dialog.test.tsx @@ -13,7 +13,7 @@ import { NextIntlClientProvider } from "next-intl" import { beforeEach, describe, expect, it, vi } from "vitest" import enMessages from "@/i18n/messages/en.json" -import type { ForgeIssueRow, ForgeLabel } from "@/lib/types" +import type { ForgeExpectedRepo, ForgeIssueRow, ForgeLabel } from "@/lib/types" import { ForgeNewIssueDialog, @@ -41,9 +41,16 @@ function created(overrides: Partial = {}): ForgeIssueRow { } } -function mount(labelOptions: ForgeLabel[] = []) { +function mount( + labelOptions: ForgeLabel[] = [], + handlers: { + expected?: ForgeExpectedRepo | null + onStaleRepository?: () => void + } = {} +) { const onOpenChange = vi.fn() const onCreated = vi.fn() + const onStaleRepository = handlers.onStaleRepository ?? vi.fn() render( ) - return { onOpenChange, onCreated } + return { onOpenChange, onCreated, onStaleRepository } } beforeEach(() => { @@ -112,13 +121,19 @@ describe("ForgeNewIssueDialog", () => { await user.click(screen.getByRole("button", { name: "Create issue" })) await waitFor(() => - expect(forgeCreateIssue).toHaveBeenCalledWith(7, { - title: "Login times out", - // Null, not "": GitHub stores an empty string as a body and the issue - // then renders an empty description block. - body: null, - labels: ["bug"], - }) + expect(forgeCreateIssue).toHaveBeenCalledWith( + 7, + { + title: "Login times out", + // Null, not "": GitHub stores an empty string as a body and the issue + // then renders an empty description block. + body: null, + labels: ["bug"], + }, + // No repository named by this caller — the page passes one in + // production (see the test below). + null + ) ) // The forge's row — the number and the URL only exist once it is written. expect(onCreated).toHaveBeenCalledWith( @@ -147,6 +162,56 @@ describe("ForgeNewIssueDialog", () => { // A picker that can only ever open an empty list is worse than no picker. expect(screen.queryByText("Labels")).not.toBeInTheDocument() }) + + it("carries the repository the dialog was opened over", async () => { + const user = userEvent.setup() + forgeCreateIssue.mockResolvedValue(created()) + mount([], { + expected: { + expectedServerHost: "github.com", + expectedOwnerRepo: "me/app", + }, + }) + + await user.type(screen.getByLabelText("Title"), "Login times out") + await user.click(screen.getByRole("button", { name: "Create issue" })) + + await waitFor(() => + expect(forgeCreateIssue).toHaveBeenCalledWith( + 7, + expect.objectContaining({ title: "Login times out" }), + { expectedServerHost: "github.com", expectedOwnerRepo: "me/app" } + ) + ) + }) + + it("tells the page to re-resolve when the folder has moved on", async () => { + const user = userEvent.setup() + // What the backend answers with when the coordinates no longer match the + // folder's remote (`WRITE_MISMATCH_I18N_KEY`). + forgeCreateIssue.mockRejectedValue({ + code: "configuration_invalid", + message: "this panel was showing github.com/me/app", + i18n_key: "Forge.writeMismatch", + i18n_params: { + expected: "github.com/me/app", + actual: "github.com/acme/app", + }, + }) + const { onStaleRepository, onCreated } = mount([], { + expected: { + expectedServerHost: "github.com", + expectedOwnerRepo: "me/app", + }, + }) + + await user.type(screen.getByLabelText("Title"), "Login times out") + await user.click(screen.getByRole("button", { name: "Create issue" })) + + await waitFor(() => expect(onStaleRepository).toHaveBeenCalled()) + // Nothing was filed, and the dialog does not pretend otherwise. + expect(onCreated).not.toHaveBeenCalled() + }) }) /** diff --git a/src/components/forge/forge-new-issue-dialog.tsx b/src/components/forge/forge-new-issue-dialog.tsx index 6c0248f0ca..6dc85bf2a5 100644 --- a/src/components/forge/forge-new-issue-dialog.tsx +++ b/src/components/forge/forge-new-issue-dialog.tsx @@ -3,6 +3,7 @@ import { useCallback, useState } from "react" import { useTranslations } from "next-intl" import { Plus } from "lucide-react" +import { toast } from "sonner" import { ForgeLabelChip } from "@/components/forge/forge-issue-row" import { Button } from "@/components/ui/button" import { @@ -19,11 +20,15 @@ import { Textarea } from "@/components/ui/textarea" import { useImeGuard } from "@/hooks/use-ime-guard" import { forgeCreateIssue } from "@/lib/api" import { + isForgeWriteMismatch, type AppErrorTranslator, toLocalizedErrorMessage, } from "@/lib/app-error" import { cn } from "@/lib/utils" -import type { ForgeIssueRow, ForgeLabel } from "@/lib/types" +import type { ForgeExpectedRepo, ForgeIssueRow, ForgeLabel } from "@/lib/types" + +/** Stands in for a caller that wired no re-resolve (a preview, a fixture). */ +const NO_OP = () => {} /** Mirrors `MAX_TITLE_CHARS` in src-tauri/src/forge/mod.rs. Enforced here as * well as there so the counter and the button agree with what the forge will @@ -52,6 +57,8 @@ export function ForgeNewIssueDialog({ folderId, repo, labelOptions, + expected = null, + onStaleRepository = NO_OP, onOpenChange, onCreated, }: { @@ -59,6 +66,13 @@ export function ForgeNewIssueDialog({ folderId: number /** `owner/repo`, for the description — the backend derives its own. */ repo: string + /** The repository this dialog was opened over, as the pair a write carries + * (see `ForgeExpectedRepo`). `null` sends none, which is the old + * behaviour. */ + expected?: ForgeExpectedRepo | null + /** The write was refused because the folder has moved to another + * repository; the page re-resolves, which closes this dialog. */ + onStaleRepository?: () => void /** The repository's label vocabulary, already fetched by the page. Empty * when it has none (or the read failed), in which case no label control is * offered at all — one that can only show an empty list is worse than none. */ @@ -97,21 +111,45 @@ export function ForgeNewIssueDialog({ setCreating(true) setFailure(null) try { - const row = await forgeCreateIssue(folderId, { - title: trimmedTitle, - body: body.trim() === "" ? null : body.trim(), - labels, - }) + const row = await forgeCreateIssue( + folderId, + { + title: trimmedTitle, + body: body.trim() === "" ? null : body.trim(), + labels, + }, + expected + ) // Only once it exists: clearing before the answer would lose what // somebody wrote to a network failure they cannot retry from. reset() onCreated(row) } catch (error) { + if (isForgeWriteMismatch(error)) { + // The page's re-resolve closes this dialog, so the inline strip would + // be torn down before it could be read — a toast outlives it. + toast.error( + toLocalizedErrorMessage(error, tRoot as unknown as AppErrorTranslator) + ) + onStaleRepository() + return + } setFailure({ error }) } finally { setCreating(false) } - }, [body, canCreate, folderId, labels, onCreated, reset, trimmedTitle]) + }, [ + body, + canCreate, + expected, + folderId, + labels, + onCreated, + onStaleRepository, + reset, + tRoot, + trimmedTitle, + ]) return ( ({ // Its own store, so the settings mocks above never answer for it. forgeRemoteGet: vi.fn(), forgeRemoteSet: vi.fn(), + // Called DURING RENDER to build the pair every write carries, so this mock + // has to answer like the real helper rather than like a spy. + forgeExpectedRepo: ( + remote: { server_host: string; owner_repo: string } | null + ) => + remote === null + ? null + : { + expectedServerHost: remote.server_host, + expectedOwnerRepo: remote.owner_repo, + }, gitListRemotes: vi.fn(), })) vi.mock("@/lib/platform", () => ({ @@ -1976,7 +1987,13 @@ describe("ForgePage writes", () => { await waitFor(() => expect(forgeCreateIssue).toHaveBeenCalledWith( 1, - expect.objectContaining({ title: "Login times out" }) + expect.objectContaining({ title: "Login times out" }), + // The repository the page is SHOWING, handed to the dialog so the + // write is refused rather than redirected if the folder has moved. + { + expectedServerHost: "github.com", + expectedOwnerRepo: "xintaofei/codeg", + } ) ) // Straight into the panel on what was just filed: the number and the link @@ -1993,6 +2010,39 @@ describe("ForgePage writes", () => { expect(vi.mocked(forgeListIssues).mock.calls.length).toBe(before) }) + it("re-resolves the repository when a write says the folder has moved on", async () => { + const user = userEvent.setup() + vi.mocked(forgeListIssues).mockResolvedValue(listOf([])) + // What the backend answers with when the coordinates a write carried no + // longer match the folder's remote (`WRITE_MISMATCH_I18N_KEY`): the write + // was refused, and the page's job is to stop showing a repository the + // folder has left. + vi.mocked(forgeCreateIssue).mockRejectedValue({ + code: "configuration_invalid", + message: "this panel was showing github.com/xintaofei/codeg", + i18n_key: "Forge.writeMismatch", + i18n_params: { + expected: "github.com/xintaofei/codeg", + actual: "github.com/acme/other", + }, + }) + mount() + const before = vi.mocked(folderForgeRemote).mock.calls.length + + await user.click(await screen.findByRole("button", { name: "New issue" })) + await user.type(screen.getByLabelText("Title"), "Login times out") + await user.click(screen.getByRole("button", { name: "Create issue" })) + + // Re-resolved — which is what tears the stale rows, panel and dialogs + // down with it. Merely toasting would leave every later action aimed at + // the same repository the refusal was about. + await waitFor(() => + expect(vi.mocked(folderForgeRemote).mock.calls.length).toBeGreaterThan( + before + ) + ) + }) + it("counts the issue it filed onto the tab badge", async () => { const user = userEvent.setup() vi.mocked(forgeListIssues).mockResolvedValue( diff --git a/src/components/forge/forge-page.tsx b/src/components/forge/forge-page.tsx index 46baacab03..1ad131fd70 100644 --- a/src/components/forge/forge-page.tsx +++ b/src/components/forge/forge-page.tsx @@ -74,6 +74,7 @@ import { ForgeStartDialog } from "@/components/forge/forge-start-dialog" import { useIsMobile } from "@/hooks/use-mobile" import { folderForgeRemote, + forgeExpectedRepo, forgeListIssues, forgeListLabels, forgeRemoteGet, @@ -738,6 +739,18 @@ export function ForgePage() { } }, [effectiveFolderPath]) + /** + * A write came back REFUSED because the folder now reads another repository. + * + * Re-resolve: the rows, the counts, the panel and the trigger dialog all + * belong to a repository this page is no longer showing, and the resolution + * effect's teardown is what puts them away. The message itself is shown by + * whoever caught the refusal — the panel that raised it is about to unmount. + */ + const handleStaleRepository = useCallback(() => { + setRemoteVersion((v) => v + 1) + }, []) + // Switching the remote saves the choice on the folder, then re-runs the // resolution above: the rows on screen belong to the repository the backend // would read next, so the old ones must not survive the switch. @@ -793,6 +806,16 @@ export function ForgePage() { const repoKey = readable == null ? "none" : `${readable.server_host}/${readable.owner_repo}` + /** + * What every WRITE carries: the repository this panel is SHOWING. + * + * Built from the RESOLVED remote rather than from `repoKey`, though the two + * name the same repository — the backend compares this pair against what it + * derives, and splitting the key back apart would put a parser between two + * spellings of one fact. + */ + const expectedRepo = useMemo(() => forgeExpectedRepo(readable), [readable]) + /** * The switch claims a generation of its own. * @@ -1861,6 +1884,8 @@ export function ForgePage() { // tell the panel whether its answer is still about the repository on // screen. Same spelling as the scopes above, from the same value. repo={repoKey} + expected={expectedRepo} + onStaleRepository={handleStaleRepository} onOpenChange={(open) => { if (!open) setDetailRow(null) }} @@ -1880,6 +1905,8 @@ export function ForgePage() { // repository — one read serves both, and the dialog must not wait on // a round trip to draw. labelOptions={labelOptions} + expected={expectedRepo} + onStaleRepository={handleStaleRepository} onOpenChange={setNewIssueOpen} onCreated={(created) => { setNewIssueOpen(false) diff --git a/src/i18n/messages/ar.json b/src/i18n/messages/ar.json index edfa791616..2449587e2e 100644 --- a/src/i18n/messages/ar.json +++ b/src/i18n/messages/ar.json @@ -5777,6 +5777,7 @@ "previewEmpty": "(بدون وصف)", "duplicateBody": "توجد مهمة نشطة تعالج هذا العنصر بالفعل: {title} ({status}). إنشاء أخرى على أي حال؟", "folderMismatch": "remote هذا المجلد هو {remote}، وليس مستودع هذه القضية — اختر المجلد المطابق.", + "writeMismatch": "كانت هذه اللوحة تعرض {expected}، لكن الطرف البعيد للمجلد أصبح الآن {actual} — تم رفض الكتابة بدلاً من إرسالها إلى المستودع الخاطئ. تم تحديث اللوحة؛ حاول مرة أخرى.", "cancel": "إلغاء", "create": "إنشاء مهمة", "creating": "جارٍ الإنشاء…", diff --git a/src/i18n/messages/de.json b/src/i18n/messages/de.json index 714a914fdb..ecb20f9ca5 100644 --- a/src/i18n/messages/de.json +++ b/src/i18n/messages/de.json @@ -5777,6 +5777,7 @@ "previewEmpty": "(keine Beschreibung)", "duplicateBody": "Eine aktive Aufgabe bearbeitet dieses Element bereits: {title} ({status}). Trotzdem eine weitere erstellen?", "folderMismatch": "Das Remote dieses Ordners ist {remote}, nicht das Repository dieses Issues — wähle den passenden Ordner.", + "writeMismatch": "Dieses Panel zeigte {expected}, aber das Remote des Ordners ist jetzt {actual} — der Schreibvorgang wurde abgelehnt statt an das falsche Repository gesendet. Das Panel wurde neu geladen; bitte erneut versuchen.", "cancel": "Abbrechen", "create": "Aufgabe erstellen", "creating": "Erstelle…", diff --git a/src/i18n/messages/en.json b/src/i18n/messages/en.json index 8d0d1626b8..3043a2b5c3 100644 --- a/src/i18n/messages/en.json +++ b/src/i18n/messages/en.json @@ -5777,6 +5777,7 @@ "previewEmpty": "(no description)", "duplicateBody": "An active task already handles this item: {title} ({status}). Create another one anyway?", "folderMismatch": "This folder's remote is {remote}, not this issue's repository — pick the matching folder.", + "writeMismatch": "This panel was showing {expected}, but the folder's remote is now {actual} — the write was refused rather than sent to the wrong repository. The panel has been refreshed; try again.", "cancel": "Cancel", "create": "Create task", "creating": "Creating…", diff --git a/src/i18n/messages/es.json b/src/i18n/messages/es.json index f224603971..edae24c644 100644 --- a/src/i18n/messages/es.json +++ b/src/i18n/messages/es.json @@ -5777,6 +5777,7 @@ "previewEmpty": "(sin descripción)", "duplicateBody": "Ya hay una tarea activa para este elemento: {title} ({status}). ¿Crear otra de todos modos?", "folderMismatch": "El remoto de esta carpeta es {remote}, no el repositorio de este issue: elige la carpeta correcta.", + "writeMismatch": "Este panel mostraba {expected}, pero el remoto de la carpeta ahora es {actual}: la escritura se rechazó en lugar de enviarla al repositorio equivocado. El panel se ha actualizado; inténtalo de nuevo.", "cancel": "Cancelar", "create": "Crear tarea", "creating": "Creando…", diff --git a/src/i18n/messages/fr.json b/src/i18n/messages/fr.json index efb9248716..74cf50a927 100644 --- a/src/i18n/messages/fr.json +++ b/src/i18n/messages/fr.json @@ -5777,6 +5777,7 @@ "previewEmpty": "(aucune description)", "duplicateBody": "Une tâche active traite déjà cet élément : {title} ({status}). En créer une autre quand même ?", "folderMismatch": "Le remote de ce dossier est {remote}, pas le dépôt de cette issue — choisissez le bon dossier.", + "writeMismatch": "Ce panneau affichait {expected}, mais le dépôt distant du dossier est désormais {actual} — l’écriture a été refusée plutôt qu’envoyée au mauvais dépôt. Le panneau a été actualisé ; réessayez.", "cancel": "Annuler", "create": "Créer la tâche", "creating": "Création…", diff --git a/src/i18n/messages/ja.json b/src/i18n/messages/ja.json index cf4bb6b852..fac3e7c3f8 100644 --- a/src/i18n/messages/ja.json +++ b/src/i18n/messages/ja.json @@ -5777,6 +5777,7 @@ "previewEmpty": "（説明なし）", "duplicateBody": "この項目はすでにアクティブなタスクが対応中です：{title}（{status}）。それでも作成しますか？", "folderMismatch": "このフォルダのリモートは {remote} で、この Issue のリポジトリと一致しません。対応するフォルダを選択してください。", + "writeMismatch": "このパネルは {expected} を表示していましたが、フォルダーのリモートは現在 {actual} です — 書き込みは誤ったリポジトリに送られる代わりに拒否されました。パネルを再読み込みしました。もう一度お試しください。", "cancel": "キャンセル", "create": "タスクを作成", "creating": "作成中…", diff --git a/src/i18n/messages/ko.json b/src/i18n/messages/ko.json index a2bb86dbd1..85c8141c21 100644 --- a/src/i18n/messages/ko.json +++ b/src/i18n/messages/ko.json @@ -5777,6 +5777,7 @@ "previewEmpty": "(설명 없음)", "duplicateBody": "이미 활성 작업이 이 항목을 처리 중입니다: {title}({status}). 그래도 새로 만들까요?", "folderMismatch": "현재 폴더의 원격은 {remote}로, 이 Issue의 저장소와 일치하지 않습니다. 일치하는 폴더를 선택하세요.", + "writeMismatch": "이 패널은 {expected}을(를) 표시하고 있었지만 폴더의 원격은 이제 {actual}입니다 — 쓰기는 잘못된 저장소로 보내지지 않고 거부되었습니다. 패널을 새로 고쳤습니다. 다시 시도하세요.", "cancel": "취소", "create": "작업 생성", "creating": "생성 중…", diff --git a/src/i18n/messages/pt.json b/src/i18n/messages/pt.json index 1839a54a8c..928b038ed0 100644 --- a/src/i18n/messages/pt.json +++ b/src/i18n/messages/pt.json @@ -5777,6 +5777,7 @@ "previewEmpty": "(sem descrição)", "duplicateBody": "Já existe uma tarefa ativa para este item: {title} ({status}). Criar outra mesmo assim?", "folderMismatch": "O remoto desta pasta é {remote}, não o repositório deste issue — escolha a pasta correspondente.", + "writeMismatch": "Este painel mostrava {expected}, mas o remoto da pasta agora é {actual} — a gravação foi recusada em vez de enviada para o repositório errado. O painel foi atualizado; tente novamente.", "cancel": "Cancelar", "create": "Criar tarefa", "creating": "Criando…", diff --git a/src/i18n/messages/zh-CN.json b/src/i18n/messages/zh-CN.json index 9306e32019..8a7bf63d27 100644 --- a/src/i18n/messages/zh-CN.json +++ b/src/i18n/messages/zh-CN.json @@ -5777,6 +5777,7 @@ "previewEmpty": "（无描述）", "duplicateBody": "已有一个活跃任务在处理该条目：{title}（{status}）。仍要再建一个吗？", "folderMismatch": "当前文件夹的远端是 {remote}，与该 Issue 的仓库不一致——请选择匹配的文件夹。", + "writeMismatch": "这个面板显示的是 {expected}，但该文件夹的远端现在是 {actual} —— 写入已被拒绝，而不是发到错误的仓库。面板已刷新，请重试。", "cancel": "取消", "create": "创建待办任务", "creating": "创建中…", diff --git a/src/i18n/messages/zh-TW.json b/src/i18n/messages/zh-TW.json index 997fc13b65..c67e9aff88 100644 --- a/src/i18n/messages/zh-TW.json +++ b/src/i18n/messages/zh-TW.json @@ -5777,6 +5777,7 @@ "previewEmpty": "（無描述）", "duplicateBody": "已有一個進行中的任務在處理該條目：{title}（{status}）。仍要再建一個嗎？", "folderMismatch": "目前資料夾的遠端是 {remote}，與該 Issue 的儲存庫不一致——請選擇相符的資料夾。", + "writeMismatch": "這個面板顯示的是 {expected}，但該資料夾的遠端現在是 {actual} —— 寫入已被拒絕，而不是送到錯誤的儲存庫。面板已重新整理，請重試。", "cancel": "取消", "create": "建立待辦任務", "creating": "建立中…", diff --git a/src/lib/api.ts b/src/lib/api.ts index 58f52483e8..d784032a85 100644 --- a/src/lib/api.ts +++ b/src/lib/api.ts @@ -33,6 +33,7 @@ import type { ForgeComment, ForgeCreateResult, ForgeCommentList, + ForgeExpectedRepo, ForgeIdentity, ForgeIssueList, ForgeIssueRow, @@ -5699,6 +5700,22 @@ export async function forgeListComments( }) } +/** + * The pair a write should carry, from the repository the panel is SHOWING. + * + * `null` for a folder with nothing readable on screen: such a write is refused + * by the resolution itself, so there is nothing to compare it against. + */ +export function forgeExpectedRepo( + remote: Pick | null | undefined +): ForgeExpectedRepo | null { + if (remote == null) return null + return { + expectedServerHost: remote.server_host, + expectedOwnerRepo: remote.owner_repo, + } +} + /** * Post one comment, and get back the comment as the FORGE stored it. * @@ -5717,7 +5734,8 @@ export async function forgeCreateComment( number: number body: string accountId?: string | null - } + }, + expected?: ForgeExpectedRepo | null ): Promise { return getTransport().call("forge_create_comment", { folderId, @@ -5726,6 +5744,7 @@ export async function forgeCreateComment( number: draft.number, body: draft.body, accountId: draft.accountId ?? null, + ...(expected ?? {}), }, }) } @@ -5745,7 +5764,8 @@ export async function forgeSetItemState( number: number action: ForgeStateAction accountId?: string | null - } + }, + expected?: ForgeExpectedRepo | null ): Promise { return getTransport().call("forge_set_item_state", { folderId, @@ -5754,6 +5774,7 @@ export async function forgeSetItemState( number: request.number, action: request.action, accountId: request.accountId ?? null, + ...(expected ?? {}), }, }) } @@ -5768,7 +5789,8 @@ export async function forgeCreateIssue( body?: string | null labels?: string[] accountId?: string | null - } + }, + expected?: ForgeExpectedRepo | null ): Promise { return getTransport().call("forge_create_issue", { folderId, @@ -5777,6 +5799,7 @@ export async function forgeCreateIssue( body: draft.body ?? null, labels: draft.labels ?? [], accountId: draft.accountId ?? null, + ...(expected ?? {}), }, }) } @@ -5877,7 +5900,8 @@ export async function forgeMergeChange( method: ForgeMergeMethod headSha?: string | null accountId?: string | null - } + }, + expected?: ForgeExpectedRepo | null ): Promise { return getTransport().call("forge_merge_change", { folderId, @@ -5886,6 +5910,7 @@ export async function forgeMergeChange( method: request.method, headSha: request.headSha ?? null, accountId: request.accountId ?? null, + ...(expected ?? {}), }, }) } diff --git a/src/lib/app-error.ts b/src/lib/app-error.ts index 9ddaab9e53..8854ba1ec0 100644 --- a/src/lib/app-error.ts +++ b/src/lib/app-error.ts @@ -69,6 +69,20 @@ export function extractAppCommandError(error: unknown): AppCommandError | null { // If the backend enum ever renames, both sides must change together. export const NOT_A_GIT_REPO_CODE = "not_a_git_repository" +// Must mirror `WRITE_MISMATCH_I18N_KEY` in src-tauri/src/forge/mod.rs. A WRITE +// that carried coordinates no longer matching the folder's remote comes back +// with this key, and the panel's job is to re-resolve the repository rather +// than leave the reader on one the folder has left. +export const FORGE_WRITE_MISMATCH_I18N_KEY = "Forge.writeMismatch" + +/** Whether this failure is that refusal — the one thing a caller must ACT on + * rather than merely report. */ +export function isForgeWriteMismatch(error: unknown): boolean { + return ( + extractAppCommandError(error)?.i18n_key === FORGE_WRITE_MISMATCH_I18N_KEY + ) +} + export function isNotAGitRepoError(error: unknown): boolean { const appError = extractAppCommandError(error) if (appError?.code === NOT_A_GIT_REPO_CODE) return true diff --git a/src/lib/types.ts b/src/lib/types.ts index 5c132441cf..9995e1fbd2 100644 --- a/src/lib/types.ts +++ b/src/lib/types.ts @@ -1986,6 +1986,22 @@ export interface ForgeChangedFileList { has_next: boolean } +/** + * Which repository a WRITE believes it is writing to — mirrors + * `forge::ExpectedCoordinates`. + * + * Sent flat beside a write's own fields, checked against what the folder's + * remote resolves to at that moment, and refused — never redirected — when the + * two disagree. That is what stops a second window or a stale browser tab from + * posting, closing, filing or merging into the repository the selection has + * since moved to. Absent on a request from a build that predates the check, + * which keeps behaving exactly as it did. + */ +export interface ForgeExpectedRepo { + expectedServerHost: string + expectedOwnerRepo: string +} + /** A folder's `origin` remote parsed into forge coordinates. */ export interface ForgeRemote { /** Which remote this was resolved from — the panel shows it so the active From 30af331d9b51aa7c6af2ee88ffd5eac7970d30ed Mon Sep 17 00:00:00 2001 From: dawn <123@qq.com> Date: Fri, 18 Sep 2026 10:13:05 +0800 Subject: [PATCH 5/6] feat(forge): deliver work from the folder's fork, on all three forges MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Triggering a task from an issue on the parent — which is exactly what the picker is for — pushed the work branch to the parent too, where the contributor who owns the fork has no write access. The task failed at delivery, after the agent had done the work; the same happened to a review of someone else's pull request. The picker is what makes both halves expressible: what the panel reads is the source, and the folder's own `origin` is the copy codeg can write to. The fork is recorded at trigger time and every delivery step reads it. `delivery_push_repo` answers where the work lands for every shape of delivery (a pull request pushes back to its own head, an issue task to the recorded fork, the source otherwise), the push uses it, and a new pull request goes out cross-repository — as `owner:branch` on GitHub and Gitea, and on GitLab as a request ADDRESSED to the fork with the target named in its body, which is what that API accepts: GitLab resolves `source_branch` in the project the request is sent to, so naming the fork while addressing the target is answered "source_branch does not exist". The claim path is the half a single delivery cannot see: the search for an existing pull request, the four-way match and `check_pull_target` all judged the head by the SOURCE, so a retry opened a duplicate — which the forge answers with a 422, after the work has been pushed. The list is the source's either way (a pull request lives where it is merged into); the head's repository is what the match compares against and what GitHub's `head={owner}:{branch}` pre-filter names. GitLab's list payload, which names a foreign source project by number alone (`project-{id}`), is resolved back to the fork's path before anything compares it — keyed on the id the task recorded, so another fork's merge request still matches nothing. GitLab's two project ids — the fork its merge requests are created on, and the target they are aimed at — are resolved at the trigger, and a task whose fork or target cannot be read is refused there, where the user can still choose something else, rather than after the agent's work has nowhere to go. A review from somebody else's fork is delivered to that fork rather than refused up front: whether this account may write there is a server-side fact (the author's "allow edits from maintainers"), so the push is what decides — which is also the only way a maintainer can push a fix into a contributor's review. A refusal there names the way out, and the task keeps its commits either way. --- src-tauri/src/commands/forge.rs | 165 ++++++++- src-tauri/src/forge/deliver.rs | 122 ++++++- src-tauri/src/forge/gitea.rs | 2 + src-tauri/src/forge/gitlab.rs | 132 +++++++- src-tauri/src/forge/mod.rs | 28 ++ src-tauri/src/work_task/engine.rs | 537 +++++++++++++++++++++++++++--- src/lib/types.ts | 5 + 7 files changed, 931 insertions(+), 60 deletions(-) diff --git a/src-tauri/src/commands/forge.rs b/src-tauri/src/commands/forge.rs index 67ade4f756..74da1200de 100644 --- a/src-tauri/src/commands/forge.rs +++ b/src-tauri/src/commands/forge.rs @@ -422,7 +422,6 @@ pub async fn folder_forge_remote_core( db: &AppDatabase, folder_id: i32, ) -> Result, AppCommandError> { - let folder = get_folder_core(db, folder_id).await?; // Resolving the selection HERE is what makes every forge operation follow // it: `resolve_folder_repo` calls this, so lists, comments, merges and task // creation all read the repository the panel is showing. @@ -433,7 +432,27 @@ pub async fn folder_forge_remote_core( let selected = forge::remotes::load_selected(&db.conn, folder_id) .await .map_err(AppCommandError::db)?; - let remote_name = selected.as_deref().unwrap_or(DEFAULT_FORGE_REMOTE); + folder_remote_named( + db, + folder_id, + selected.as_deref().unwrap_or(DEFAULT_FORGE_REMOTE), + ) + .await +} + +/// One NAMED remote of a folder, parsed into forge coordinates — the same +/// mechanics as the selection above, for a remote named outright. +/// +/// Used for the folder's own `origin`, which is a fact about the working copy +/// rather than about what the panel is showing: the picker can point the panel +/// at a parent while the branch codeg can write to stays `origin`. See +/// `ForgeSourceMeta::fork_repo`. +async fn folder_remote_named( + db: &AppDatabase, + folder_id: i32, + remote_name: &str, +) -> Result, AppCommandError> { + let folder = get_folder_core(db, folder_id).await?; let output = crate::process::tokio_command("git") .args(["-C", &folder.path, "remote", "get-url", remote_name]) .output() @@ -961,6 +980,59 @@ pub async fn work_task_create_from_forge_core( None }; + // ── Which repository the WORK will be pushed to ───────────────────────── + // + // The panel's selection names the repository being READ (the parent, in the + // fork workflow: `origin` is the contributor's own copy); the folder's own + // `origin` names the one codeg can WRITE to. Both are known only HERE — + // delivery runs long after the panel may have moved on, and the remote list + // is the folder's mutable state, so the answer is recorded on the task. + let origin_repo = folder_remote_named(db, draft.folder_id, DEFAULT_FORGE_REMOTE) + .await? + // Only a remote on the SAME host is a candidate: the push spends this + // task's credential, which belongs to `server_host`. + .filter(|remote| remote.server_host == server_host) + .map(|remote| remote.owner_repo); + let fork_repo = origin_repo + .clone() + .filter(|repo| !forge::same_repo(repo, &owner_repo)); + + // GitLab spells a cross-project merge request with project IDs rather than a + // qualified head ref — BOTH ends of it — so they are resolved HERE, while + // the user can still be told, and recorded on the task. The fork's id is the + // project the merge request is created ON, and it is also how a retry + // recognises its own merge request again: the list payload names a foreign + // source project by number alone (see `ForgePr::with_resolved_head`). The + // target's id goes in the body, because GitLab does not infer it from the + // fork's upstream — a create without it lands on the fork itself. + let mut fork_project_id = None; + let mut owner_project_id = None; + if let Some(fork) = fork_repo.as_deref() { + if provider == ForgeProvider::GitLab { + let resolved = forge::gitlab::resolve_project_id(&auth, fork).await; + fork_project_id = Some(gitlab_project_id( + resolved, + GitLabEnd::Fork, + fork, + &owner_repo, + )?); + let resolved = forge::gitlab::resolve_project_id(&auth, &owner_repo).await; + owner_project_id = Some(gitlab_project_id( + resolved, + GitLabEnd::Target, + fork, + &owner_repo, + )?); + } + } + + // A review whose head is somebody else's fork is deliberately NOT refused + // here. Whether this account may write into that fork is a server-side fact + // — the author's "allow edits from maintainers", which codeg cannot read up + // front — and a gate here would close a channel that works for exactly the + // people this panel is for (a maintainer pushing a fix into a contributor's + // review). The push decides instead, and its refusal names the way out. + let key = forge::source_key( provider.as_str(), &server_host, @@ -995,6 +1067,9 @@ pub async fn work_task_create_from_forge_core( // the task is queued cannot silently change what gets worked on. head_sha: pull.as_ref().map(|p| p.head_sha.clone()), head_repo: pull.as_ref().map(|p| p.head_repo.clone()), + fork_repo, + fork_project_id, + owner_project_id, result_pr: None, // Always stamped explicitly, both answers: the engine reads it as the // user's decision, and an absent field there means "an older row that @@ -1169,6 +1244,55 @@ fn truncate_chars(input: &str, cap: usize) -> String { input.chars().take(cap).collect() } +/// Which end of a cross-project merge request a project id is being resolved +/// for — the two refusals have different things to tell the user. +/// +/// Both ends are resolved at the TRIGGER, where the user can still choose +/// something else, because a project the token cannot read (private without +/// access, renamed, deleted) leaves the delivery nowhere to go: the merge +/// request is created ON the fork — GitLab resolves `source_branch` in the +/// project the request is addressed to — and the target is named in its body. +/// Every coordinate codeg holds is a path, and GitLab names projects by number, +/// so the two lookups happen here: once, with the target's id recorded on the +/// task rather than looked up again on a retry. +#[derive(Clone, Copy, PartialEq, Eq)] +enum GitLabEnd { + /// The project the merge request is created ON: this folder's `origin`. + Fork, + /// The project it is aimed at: the repository the panel was reading. + Target, +} + +/// A GitLab project id the delivery cannot do without, or the refusal that says +/// which half it was for. +/// +/// Resolved at the trigger, both ends, because that is where the user can still +/// choose something else — and the target is recorded rather than looked up +/// later so a retry or a recovered delivery has the pair on the task already. +fn gitlab_project_id( + resolved: Result, + end: GitLabEnd, + fork_repo: &str, + source_repo: &str, +) -> Result { + resolved.map_err(|e| { + let message = match end { + GitLabEnd::Fork => format!( + "this folder's `origin` is {fork_repo}, so this task's work would be delivered as a \ + merge request FROM that fork — but codeg could not read that GitLab project ({e}), \ + so there would be nowhere to open it. Check that the account can see {fork_repo}, \ + or point the panel at a folder whose `origin` IS {source_repo}." + ), + GitLabEnd::Target => format!( + "this task's work would be delivered as a merge request from {fork_repo} into \ + {source_repo} — but codeg could not read {source_repo} on GitLab ({e}), so there \ + would be nowhere to open it. Refresh the workbench and try again." + ), + }; + AppCommandError::invalid_input(message) + }) +} + // ── Tauri wrappers (desktop mode) ─────────────────────────────────────────── #[cfg(feature = "tauri-runtime")] @@ -1980,6 +2104,9 @@ mod tests { head_ref: None, head_sha: None, head_repo: None, + fork_repo: None, + fork_project_id: None, + owner_project_id: None, result_pr: None, writeback: stored, }; @@ -2019,4 +2146,38 @@ mod tests { assert_eq!(s.is_report(), expect, "{s:?}"); } } + + /// GitLab's cross-project delivery needs the fork's project id, and the id + /// is resolved at the TRIGGER: failing there refuses the task while the + /// user can still choose something else, instead of after the agent's work + /// has nowhere to go. GitHub and Gitea never call this. + #[test] + fn a_gitlab_end_whose_project_id_cannot_be_read_is_refused_at_trigger() { + assert_eq!( + gitlab_project_id(Ok(4711), GitLabEnd::Fork, "me/app", "acme/app").expect("resolves"), + 4711 + ); + + let not_found = || forge::ForgeError::Api { + status: 404, + message: "404 Project Not Found".into(), + }; + let refusal = gitlab_project_id(Err(not_found()), GitLabEnd::Fork, "me/app", "acme/app") + .expect_err("unreadable fork"); + assert!(refusal.message.contains("me/app"), "{}", refusal.message); + assert!(refusal.message.contains("acme/app"), "{}", refusal.message); + assert!( + refusal.message.contains("404"), + "the forge's own reason rides along: {}", + refusal.message + ); + + // The other end says which repository it could not read, because that + // is the part the user can act on — the fork is already settled. + let refusal = + gitlab_project_id(Err(not_found()), GitLabEnd::Target, "me/app", "acme/app") + .expect_err("unreadable target"); + assert!(refusal.message.contains("me/app"), "{}", refusal.message); + assert!(refusal.message.contains("acme/app"), "{}", refusal.message); + } } diff --git a/src-tauri/src/forge/deliver.rs b/src-tauri/src/forge/deliver.rs index cab6c98dd7..9bfef7071d 100644 --- a/src-tauri/src/forge/deliver.rs +++ b/src-tauri/src/forge/deliver.rs @@ -120,13 +120,52 @@ pub struct ForgePr { pub head_sha: String, pub head_ref: String, /// `owner/repo` of the head — compared with `same_repo`, never `==`. + /// GitLab's LIST payload abbreviates a foreign source project to + /// `project-{id}`; see [`ForgePr::with_resolved_head`]. pub head_repo: String, pub base_ref: String, } +impl ForgePr { + /// Turn GitLab's `project-{id}` placeholder back into a repository path. + /// + /// GitLab names a foreign source project by NUMBER alone in every list + /// payload — `map_merge_request` writes that as `project-{id}` on purpose, + /// so a gate does not spend one request per row to learn the same thing. + /// A delivery that knows which id it recorded at trigger time substitutes + /// the fork's path for free, and every comparison downstream (the four-way + /// match's head criterion, `check_pull_target`) then works on the same + /// spelling the detail endpoint would have given. + /// + /// Only the RECORDED id is substituted: another fork's merge request keeps + /// its placeholder, which matches nothing — that is what keeps a + /// stranger's merge request from being adopted. + pub fn with_resolved_head(mut self, fork_project_id: Option, fork_repo: &str) -> Self { + if let Some(id) = fork_project_id { + if self.head_repo == format!("project-{id}") { + self.head_repo = fork_repo.to_string(); + } + } + self + } +} + /// What to open, once we know nothing suitable exists yet. #[derive(Debug, Clone)] pub struct NewPullRequest<'a> { + /// GitLab only: the project that OWNS the head branch, when it is not the + /// repository being merged into. GitHub and Gitea express that in `head` + /// (`owner:branch`); GitLab needs both ends by id, and the request has to be + /// ADDRESSED to this one — GitLab resolves `source_branch` in the project + /// the request is sent to, so naming the fork while addressing the target + /// answers "source_branch does not exist" (verified against gitlab.com). + /// `None` = a same-project merge request, created on `owner_repo` itself. + pub source_project_id: Option, + /// GitLab only, and the other half of the same fact: the project the merge + /// request is aimed at. Required exactly when `source_project_id` is set — + /// GitLab does NOT fall back to the fork's upstream, and a request without + /// it makes the fork its own target. + pub target_project_id: Option, pub title: &'a str, pub head: &'a str, pub base: &'a str, @@ -251,11 +290,20 @@ pub trait ForgeDeliveryApi: Send + Sync { remote_branch: &str, ) -> Result<(), String>; - /// Pull requests whose head is `head_branch` in the source repository, in - /// ANY state — a merged or closed one is exactly what recovery must see. + /// Pull requests whose head is `head_branch` in `head_repo`, in ANY state — + /// a merged or closed one is exactly what recovery must see. + /// + /// Two repositories, deliberately, because a cross-repository delivery has + /// two: the collection that is LISTED is `ctx.owner_repo` (a pull request + /// lives where it is merged into), while `head_repo` is the repository the + /// branch was pushed to, which the match compares a row's head against and + /// which GitHub's `head={owner}:{branch}` pre-filter names. Aiming the list + /// at the head's repository instead comes back empty — which reads as "no + /// pull request exists" and earns a 422 from the create that follows. async fn find_pulls( &self, ctx: &DeliveryCtx<'_>, + head_repo: &str, head_branch: &str, ) -> Result, String>; @@ -341,11 +389,14 @@ impl ForgeDeliveryApi for ForgeDelivery { async fn find_pulls( &self, ctx: &DeliveryCtx<'_>, + head_repo: &str, head_branch: &str, ) -> Result, String> { let auth = resolve(ctx).await?; match ctx.provider { - ForgeProvider::GitHub => find_pulls(&auth, ctx.owner_repo, head_branch).await, + ForgeProvider::GitHub => { + find_pulls(&auth, ctx.owner_repo, head_repo, head_branch).await + } ForgeProvider::GitLab => { gitlab::find_merge_requests(&auth, ctx.owner_repo, head_branch).await } @@ -440,6 +491,12 @@ async fn resolve(ctx: &DeliveryCtx<'_>) -> Result { /// `GET /repos/{o}/{r}/pulls?head={owner}:{branch}&state=all`. /// +/// `owner_repo` is the repository whose pull requests are listed (the one the +/// merge is aimed at); `head_repo` is the one the branch was pushed to, and the +/// ONLY thing taken from it is the owner in the filter — GitHub pre-selects by +/// head owner, so a fork's branch has to be asked for as `owner:branch` while +/// the collection in the path stays the target's. +/// /// Unlike `assignee`/`labels` (silently ignored by this endpoint — see /// `github.rs`), the `head` filter IS applied; verified against the live API /// before this was written. The four-way match still runs locally afterwards: @@ -447,14 +504,17 @@ async fn resolve(ctx: &DeliveryCtx<'_>) -> Result { pub async fn find_pulls( auth: &ResolvedAuth, owner_repo: &str, + head_repo: &str, head_branch: &str, ) -> Result, ForgeError> { let repo = super::normalize_repo(owner_repo) .ok_or_else(|| ForgeError::Invalid(format!("bad repository path: {owner_repo}")))?; - let owner = repo + let head = super::normalize_repo(head_repo) + .ok_or_else(|| ForgeError::Invalid(format!("bad repository path: {head_repo}")))?; + let owner = head .split('/') .next() - .ok_or_else(|| ForgeError::Invalid(format!("bad repository path: {owner_repo}")))?; + .ok_or_else(|| ForgeError::Invalid(format!("bad repository path: {head_repo}")))?; let url = format!( "{}/repos/{}/pulls?head={}:{}&state=all&per_page=100", auth.api_base, @@ -1101,12 +1161,19 @@ mod tests { } fn pull_json(number: i64, merged_at: Option<&str>) -> serde_json::Value { + pull_json_from(number, merged_at, "Acme/App") + } + + /// Same, with the head repository spelled out: the cross-repository case, + /// where the row is in the target's collection while its head lives in a + /// fork. + fn pull_json_from(number: i64, merged_at: Option<&str>, head_repo: &str) -> serde_json::Value { serde_json::json!({ "number": number, "html_url": format!("https://github.test/acme/app/pull/{number}"), "state": if merged_at.is_some() { "closed" } else { "open" }, "merged_at": merged_at, - "head": { "sha": "abc123", "ref": "task/7", "repo": { "full_name": "Acme/App" } }, + "head": { "sha": "abc123", "ref": "task/7", "repo": { "full_name": head_repo } }, "base": { "ref": "main" }, }) } @@ -1133,6 +1200,7 @@ mod tests { // verified against the live API before this was written. let rows = match q.get("head").map(String::as_str) { Some("acme:task/7") => vec![pull_json(4, None)], + Some("me:task/7") => vec![pull_json_from(8, None, "me/app")], Some("acme:merged") => vec![pull_json(5, Some("2026-08-18T00:00:00Z"))], _ => vec![], }; @@ -1144,6 +1212,13 @@ mod tests { async { Json(pull_json(6, None)) } }), ) + .route( + "/repos/me/app/pulls", + // The fork's own collection: the pull request was opened in the + // target, so a search aimed here comes back EMPTY — the shape + // of the mistake, reproduced. + get(|| async { Json(serde_json::Value::Array(vec![])) }), + ) .route( "/conflict/repos/acme/app/pulls", post(|| async { @@ -1182,15 +1257,40 @@ mod tests { let (api_base, _, _, _) = mock_api().await; let auth = auth_for(api_base); - let open = find_pulls(&auth, "Acme/App", "task/7").await.unwrap(); + let open = find_pulls(&auth, "Acme/App", "Acme/App", "task/7").await.unwrap(); assert_eq!(open.len(), 1); assert!(!open[0].merged && open[0].state == "open"); assert_eq!(open[0].head_repo, "Acme/App"); // canonical casing preserved - let merged = find_pulls(&auth, "acme/app", "merged").await.unwrap(); + let merged = find_pulls(&auth, "acme/app", "acme/app", "merged").await.unwrap(); assert!(merged[0].merged, "merged_at must set the merged flag"); - assert!(find_pulls(&auth, "acme/app", "nothing").await.unwrap().is_empty()); + assert!(find_pulls(&auth, "acme/app", "acme/app", "nothing") + .await + .unwrap() + .is_empty()); + } + + /// A cross-repository search: the collection listed is the one being merged + /// into, the `head` filter names the fork that holds the branch. Aiming the + /// collection at the head's repository instead comes back empty, which a + /// delivery reads as "no pull request exists" — the 422 a retry then earns. + #[tokio::test] + async fn a_cross_repo_search_lists_the_target_and_filters_by_the_fork() { + let (api_base, _, _, _) = mock_api().await; + let auth = auth_for(api_base); + let found = find_pulls(&auth, "acme/app", "me/app", "task/7") + .await + .unwrap(); + assert_eq!(found.len(), 1); + assert_eq!(found[0].number, 8); + assert_eq!(found[0].head_repo, "me/app", "the row names the fork"); + // And the head's own repository as the collection: the mock answers the + // way the real endpoint does, with nothing. + assert!(find_pulls(&auth, "me/app", "me/app", "task/7") + .await + .unwrap() + .is_empty()); } #[tokio::test] @@ -1201,6 +1301,8 @@ mod tests { &auth, "acme/app", &NewPullRequest { + source_project_id: None, + target_project_id: None, title: "Fix #7", head: "task/7", base: "main", @@ -1228,6 +1330,8 @@ mod tests { &auth, "acme/app", &NewPullRequest { + source_project_id: None, + target_project_id: None, title: "t", head: "task/7", base: "main", diff --git a/src-tauri/src/forge/gitea.rs b/src-tauri/src/forge/gitea.rs index 11a4b7452d..bae434bd0a 100644 --- a/src-tauri/src/forge/gitea.rs +++ b/src-tauri/src/forge/gitea.rs @@ -2469,6 +2469,8 @@ mod tests { let (api_base, seen) = mock_api().await; let auth = auth_for(api_base); let mut req = NewPullRequest { + source_project_id: None, + target_project_id: None, title: "Fix the crash", head: "codeg/task-1", base: "main", diff --git a/src-tauri/src/forge/gitlab.rs b/src-tauri/src/forge/gitlab.rs index 01bdfe50b0..7aad068c3e 100644 --- a/src-tauri/src/forge/gitlab.rs +++ b/src-tauri/src/forge/gitlab.rs @@ -322,19 +322,33 @@ pub async fn create_merge_request( owner_repo: &str, req: &NewPullRequest<'_>, ) -> Result { - let project = project_ref(owner_repo)?; + // Where the request is ADDRESSED. GitLab resolves `source_branch` in the + // project the request is sent to, so a merge request from a fork is created + // ON the fork, with the project it is aimed at named in the body. Addressing + // the target and naming the fork in `source_project_id` instead is answered + // with `source_branch does not exist` — measured against gitlab.com, not + // guessed. + let project = match req.source_project_id { + Some(fork) => fork.to_string(), + None => project_ref(owner_repo)?, + }; let url = format!("{}/projects/{project}/merge_requests", auth.api_base); let title = if req.draft && !is_draft_title(req.title) { format!("Draft: {}", req.title) } else { req.title.to_string() }; - let body = serde_json::json!({ + let mut body = serde_json::json!({ "source_branch": req.head, "target_branch": req.base, "title": title, "description": req.body, }); + // The other half of a cross-project merge request. Omitting it does not + // fall back to the fork's upstream — it makes the fork its own target. + if let Some(id) = req.target_project_id { + body["target_project_id"] = serde_json::json!(id); + } let raw: RawMergeRequest = api_post(auth, &url, &body) .await? .json() @@ -850,6 +864,33 @@ async fn project_path(auth: &ResolvedAuth, project_id: i64) -> Option { Some(project.path_with_namespace).filter(|p| !p.is_empty()) } +/// `path_with_namespace` → project id — the inverse of [`project_path`], and +/// the one coordinate a cross-project merge request cannot be opened without. +/// +/// GitLab takes the SOURCE project of a merge request by id, while everything +/// codeg holds (a remote URL, a folder, a task's recorded fork) is a path. The +/// id is resolved once, at trigger time, and recorded on the task: the list +/// payload the retry path reads names a foreign source project by number alone +/// (see `map_merge_request`), so the delivery needs this same id to turn that +/// placeholder back into a repository. +pub async fn resolve_project_id( + auth: &ResolvedAuth, + owner_repo: &str, +) -> Result { + #[derive(Deserialize)] + struct RawProject { + id: i64, + } + let project = project_ref(owner_repo)?; + let url = format!("{}/projects/{project}", auth.api_base); + let raw: RawProject = api_get(auth, &url) + .await? + .json() + .await + .map_err(|e| ForgeError::Network(format!("bad project payload: {e}")))?; + Ok(raw.id) +} + pub(crate) async fn api_get( auth: &ResolvedAuth, url: &str, @@ -1425,6 +1466,9 @@ mod tests { let user_hits = Arc::new(AtomicUsize::new(0)); let last_query: Arc>> = Default::default(); let seen = creates.clone(); + // The fork's own create records into the same list from a second + // closure, so it needs its own handle. + let fork_seen = creates.clone(); let issue_notes = notes.clone(); let mr_notes = notes.clone(); let hits = user_hits.clone(); @@ -1460,6 +1504,12 @@ mod tests { (headers, Json(serde_json::Value::Array(rows))) }), ) + // The project itself — what `resolve_project_id` reads, and the id + // a cross-project merge request is opened with. + .route( + "/projects/group%2Fsub%2Fproj", + get(|| async { Json(serde_json::json!({ "id": 4711 })) }), + ) .route( "/projects/group%2Fsub%2Fproj/merge_requests", get(move |Query(q): Query>| async move { @@ -1495,6 +1545,21 @@ mod tests { async { Json(mr_json(8, "opened", 1)) } }), ) + // A merge request created ON a fork: the project in the path owns + // the branch, the target is named in the body. The payload that + // comes back names the fork by number alone, which is what the + // claim path has to turn back into a repository. + .route( + "/projects/4712/merge_requests", + post(move |Json(body): Json| { + fork_seen.lock().unwrap().push(body); + async { + let mut mr = mr_json(9, "opened", 4712); + mr["target_project_id"] = serde_json::json!(4711); + Json(mr) + } + }), + ) .route( "/projects/group%2Fsub%2Fproj/merge_requests/4", get(|| async { @@ -1979,6 +2044,62 @@ mod tests { assert!(get_merge_request(&auth, "not-a-path", 4).await.is_err()); } + /// The path → id lookup a trigger performs, and the cross-project body it + /// The shape of a cross-project merge request, which the live API settled: + /// the request is ADDRESSED to the fork — GitLab resolves `source_branch` + /// in the project in the path, so addressing the target while naming the + /// fork answers `source_branch does not exist` — and the target is named in + /// the body. Omitting that does not fall back to the fork's upstream: it + /// makes the fork its own target. + #[tokio::test] + async fn a_cross_project_merge_request_is_created_on_the_fork() { + let (api_base, creates, _, _, _) = mock_api().await; + let auth = auth_for(api_base); + + assert_eq!( + resolve_project_id(&auth, "Group/Sub/Proj").await.expect("id"), + 4711, + "the path is asked for exactly as the API spells it" + ); + // A path this token cannot see (or that is not a path at all) is an + // error, never a silent 0 — the trigger refuses the task on it. + assert!(resolve_project_id(&auth, "not-a-path").await.is_err()); + + let made = create_merge_request( + &auth, + "group/sub/proj", + &NewPullRequest { + source_project_id: Some(4712), + target_project_id: Some(4711), + title: "From the fork", + head: "task/7", + base: "main", + body: "", + draft: false, + }, + ) + .await + .expect("create"); + assert_eq!(made.number, 9); + let sent = creates.lock().unwrap().first().cloned().unwrap(); + assert_eq!(sent["target_project_id"], 4711); + assert_eq!(sent["source_branch"], "task/7", "the branch stays bare"); + assert!(sent.get("source_project_id").is_none(), "{sent}"); + // And what the target's own list will say about it: a number where a + // repository name belongs, which only the recorded fork id can turn + // back into one. + assert_eq!(made.head_repo, "project-4712"); + assert_eq!( + made.clone().with_resolved_head(Some(4712), "me/fork").head_repo, + "me/fork" + ); + assert_eq!( + made.with_resolved_head(Some(999), "someone/else").head_repo, + "project-4712", + "another fork's merge request is not this task's" + ); + } + /// The delivery's lookup: by source branch, in any state, mapped into the /// same shape the four-way match already knows. #[tokio::test] @@ -2006,6 +2127,8 @@ mod tests { &auth, "group/sub/proj", &NewPullRequest { + source_project_id: None, + target_project_id: None, title: "Fix #7", head: "task/7", base: "main", @@ -2021,12 +2144,17 @@ mod tests { assert_eq!(sent["target_branch"], "main"); assert_eq!(sent["title"], "Draft: Fix #7"); assert_eq!(sent["description"], "Closes #7"); + // A SAME-project merge request must not name a source project: GitLab + // reads that field as "the branch lives over there". + assert!(sent.get("source_project_id").is_none(), "{sent}"); // Not a draft, and an already-prefixed title is not prefixed twice. create_merge_request( &auth, "group/sub/proj", &NewPullRequest { + source_project_id: None, + target_project_id: None, title: "Draft: Fix #7", head: "task/7", base: "main", diff --git a/src-tauri/src/forge/mod.rs b/src-tauri/src/forge/mod.rs index aff54f33e4..ad8eab6c4b 100644 --- a/src-tauri/src/forge/mod.rs +++ b/src-tauri/src/forge/mod.rs @@ -462,6 +462,34 @@ pub struct ForgeSourceMeta { pub head_sha: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub head_repo: Option, + /// The repository the WORK is pushed to when it is not the source — the + /// folder's `origin`, recorded at trigger time whenever the panel was + /// pointed at another remote (the fork workflow: the picker selects the + /// parent, so the issues are the parent's while the branch codeg can write + /// to is the user's own copy). + /// + /// `None` is "push to the source", which is every task triggered before + /// this field existed and every one triggered from a folder that IS the + /// source. Read through `delivery_push_repo`, never directly. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub fork_repo: Option, + /// GitLab only: the FORK's project id, resolved at trigger time. + /// + /// It is the project a cross-project merge request is created ON + /// (`POST /projects/{id}/merge_requests` — GitLab resolves `source_branch` + /// in the project the request is addressed to), and — because GitLab's list + /// payload names a foreign source project by number alone — it is also what + /// turns that `project-{id}` placeholder back into a repository when a + /// delivery has to recognise its own merge request on a retry. See + /// `ForgePr::with_resolved_head`. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub fork_project_id: Option, + /// GitLab only: the id of the project the merge request is AIMED at (the + /// repository the panel was reading). Recorded because the create has to + /// spell it out on a request addressed to the fork — GitLab does not infer + /// the target from the fork's upstream. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub owner_project_id: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub result_pr: Option, /// Whether this task comments its outcome back on the item when it diff --git a/src-tauri/src/work_task/engine.rs b/src-tauri/src/work_task/engine.rs index 23a827b6b8..c46f8662e5 100644 --- a/src-tauri/src/work_task/engine.rs +++ b/src-tauri/src/work_task/engine.rs @@ -49,7 +49,9 @@ use crate::forge::deliver::{ adopt_pull_request, pull_request_body, writeback_comment_body, DeliveryCtx, ForgeDeliveryApi, ForgePr, NewPullRequest, PrAdoption, TaskOutcome, }; -use crate::forge::{ForgeItemKind, ForgeSourceMeta, SOURCE_KIND_ISSUE, SOURCE_KIND_PR}; +use crate::forge::{ + ForgeItemKind, ForgeProvider, ForgeSourceMeta, SOURCE_KIND_ISSUE, SOURCE_KIND_PR, +}; use crate::logging::throttle::{LagLogThrottle, LAG_LOG_WINDOW}; use crate::models::{ AgentType, FollowUpIntent, WorkTaskConfig, WorkTaskFolderSettings, WorkTaskMergeOp, @@ -4035,12 +4037,14 @@ impl TaskEngine { trigger it again" .to_string() })?; - // The push lands in the recorded HEAD repository — the fork, when - // the pull request comes from one. Resolvability is re-checked - // here, before the CAS, so a row whose fork codeg cannot name - // (written by an older build, or hydrated while the fork was - // already gone) is refused with the task left exactly as it was. - pull_push_repo(&meta)?; + // The push lands in the repository this task's work belongs to — + // the pull request's own head (the fork, when it comes from one), + // or the recorded fork of an issue task. Resolvability is + // re-checked here, before the CAS, so a row whose fork codeg + // cannot name (written by an older build, or hydrated while the + // fork was already gone) is refused with the task left exactly as + // it was. + delivery_push_repo(&meta)?; head_ref } else { work_branch.clone() @@ -4337,26 +4341,47 @@ impl TaskEngine { account_id: &meta.account_id, owner_repo: &meta.owner_repo, }; + // Where the work lands: the source, or the fork this folder's `origin` + // named when the task was triggered. Resolved ONCE, before the push, + // because the same answer decides three things — the push's target, the + // `head` a new pull request is created with, and (on GitHub and Gitea) + // which owner the look-before-create asks for. The pull request itself + // is still listed in the SOURCE's collection either way. + let push_repo = delivery_push_repo(meta)?; // Fast-forward push of the same commits is a no-op, so a retry after a // later step failed costs nothing and changes nothing. self.forge - .push_branch(&ctx, wt_path, &meta.owner_repo, work_branch, work_branch) + .push_branch(&ctx, wt_path, &push_repo, work_branch, work_branch) .await .map_err(|e| format!("could not push the task branch: {e}"))?; // Look before creating: the previous attempt may have opened the pull - // request and died on the way to settling it. + // request and died on the way to settling it. The LIST is the source's + // — a pull request lives where it is merged into — while the branch + // being searched for is the fork's, which GitHub's + // `head={owner}:{branch}` pre-filter names and the four-way match + // compares against. Aim it at the fork instead and the list comes back + // empty, which is how a retry opens a second pull request and eats a + // 422. let existing = self .forge - .find_pulls(&ctx, work_branch) + .find_pulls(&ctx, &push_repo, work_branch) .await .map_err(|e| format!("could not check for an existing pull request: {e}"))?; + // GitLab's list payload names a foreign source project by NUMBER only + // (`project-{id}`); the task recorded which id that is at trigger time, + // so the placeholder becomes the fork's path before the match — without + // it, a retry can never recognise the merge request it already opened. + let existing: Vec = existing + .into_iter() + .map(|pr| pr.with_resolved_head(meta.fork_project_id, &push_repo)) + .collect(); match adopt_pull_request( existing, expected_head, work_branch, base_branch, - &meta.owner_repo, + &push_repo, ) { PrAdoption::Merged(pr) | PrAdoption::Open(pr) => Ok(pr), PrAdoption::ClosedUnmerged(pr) => Err(format!( @@ -4375,12 +4400,30 @@ impl TaskEngine { )), PrAdoption::NoMatch => { let body = pull_request_body(&meta.url, meta.number, task_id); + // The head goes to the FORK when there is one — as + // `owner:branch`, which is how both GitHub and Gitea spell a + // repository other than the target. GitLab has no qualified + // head: its side of the same fact is the pair of project ids + // below, and its `source_branch` stays bare. + let head = head_ref_for(meta.provider, &push_repo, &meta.owner_repo, work_branch); + // The id pair a cross-project GitLab merge request needs: the + // project the request is created ON (the fork that holds the + // branch) and the one it is aimed at. A same-project delivery + // carries neither. + let (source_project_id, target_project_id) = + if crate::forge::same_repo(&push_repo, &meta.owner_repo) { + (None, None) + } else { + (meta.fork_project_id, meta.owner_project_id) + }; self.forge .create_pull( &ctx, &NewPullRequest { + source_project_id, + target_project_id, title, - head: work_branch, + head: &head, base: base_branch, body: &body, draft, @@ -4449,7 +4492,7 @@ impl TaskEngine { meta.number )); } - let push_repo = pull_push_repo(meta)?; + let push_repo = delivery_push_repo(meta)?; self.forge .push_branch(&ctx, wt_path, &push_repo, work_branch, remote_branch) .await @@ -4516,18 +4559,20 @@ impl TaskEngine { remote_branch: &str, ) -> Result<(), String> { let noun = meta.provider.change_noun(); - // Compared against the head repository RECORDED at trigger time (the - // fork, when the pull request comes from one — rows without one are - // same-repo by construction), not against the source repository: a - // fork's pull request legitimately lives elsewhere, and the thing - // being caught here is the head moving since the task was made. - let recorded_repo = meta - .head_repo - .as_deref() - .map(str::trim) - .filter(|r| !r.is_empty()) - .unwrap_or(&meta.owner_repo); - if !crate::forge::same_repo(&pr.head_repo, recorded_repo) { + // Compared against the repository this task's work is pushed to — the + // head of a pull-request task, or the recorded fork of an issue task — + // not against the source: a cross-repository delivery legitimately has + // its head elsewhere, and the thing being caught here is that head + // moving since the task was made. Rows written before the fork was + // recorded are same-repo by construction and answer the source. + let recorded_repo = delivery_push_repo(meta)?; + // GitLab's list payload abbreviates a foreign source project to + // `project-{id}`; the task recorded which id is its fork, so the + // placeholder becomes the path before the comparison — otherwise a + // cross-project merge request created a moment ago reads as one that + // was retargeted under us. + let pr = pr.clone().with_resolved_head(meta.fork_project_id, &recorded_repo); + if !crate::forge::same_repo(&pr.head_repo, &recorded_repo) { return Err(format!( "{noun} #{} now comes from {}, not {recorded_repo} — check it before delivering \ again", @@ -4864,8 +4909,22 @@ impl TaskEngine { .await; return; } - let found = match self.forge.find_pulls(&ctx, remote_branch).await { - Ok(prs) => prs, + // An ISSUE task's own branch lives in the repository recorded for it — + // the fork, when the folder's `origin` was not the source. Recovery is + // the one place that has to agree with `push_and_open` about where + // that is, or an interrupted delivery re-opens what already exists. + let push_repo = match delivery_push_repo(&meta) { + Ok(repo) => repo, + Err(e) => { + self.bounce_delivery(task, e).await; + return; + } + }; + let found = match self.forge.find_pulls(&ctx, &push_repo, remote_branch).await { + Ok(prs) => prs + .into_iter() + .map(|pr| pr.with_resolved_head(meta.fork_project_id, &push_repo)) + .collect(), Err(e) => { self.bounce_delivery( task, @@ -4883,7 +4942,7 @@ impl TaskEngine { expected_head, remote_branch, base_branch, - &meta.owner_repo, + &push_repo, ) { PrAdoption::Merged(pr) | PrAdoption::Open(pr) => { if let Err(e) = self.settle_delivery(task_id, &meta, task.run_seq, &pr).await { @@ -6140,31 +6199,59 @@ fn classify_push_refusal(error: &str) -> PushRefusal { PushRefusal::Unknown } -/// The repository a pull-request task's push-back lands in: the HEAD -/// repository recorded at trigger time — the fork, when the pull request comes -/// from one. A row that recorded none falls back to the source repository: -/// builds that predate the field refused forks at trigger, so their rows are -/// same-repo by construction. `Err` is the one head codeg cannot push to ever — -/// a fork it cannot name (GitLab's unresolved `project-{id}` placeholder, or a -/// fork deleted since GitHub hydrated the row). -fn pull_push_repo(meta: &ForgeSourceMeta) -> Result { +/// The repository a task's WORK lands in. +/// +/// Three answers, in order: +/// - a task that IS a pull request pushes back to that pull request's own head +/// repository (the fork, when it comes from one); +/// - an issue task pushes its own branch to the fork its folder's `origin` +/// named when the task was triggered, if that was not the source; +/// - otherwise the source itself, which is every row written before the fork +/// was recorded at all. +/// +/// `Err` is the one repository codeg can never push to: a fork it cannot name +/// (GitLab's unresolved `project-{id}` placeholder, or a fork deleted since the +/// row was written). +fn delivery_push_repo(meta: &ForgeSourceMeta) -> Result { let recorded = meta .head_repo .as_deref() + .or(meta.fork_repo.as_deref()) .map(str::trim) .filter(|r| !r.is_empty()) .unwrap_or(&meta.owner_repo); crate::forge::normalize_repo(recorded).ok_or_else(|| { format!( - "{} #{} comes from a fork whose repository codeg cannot see (it may be private or \ - deleted), so there is nowhere to push the work back to — its commits stay on the \ - task's local branch", + "{} #{} would be delivered from a repository codeg cannot see ({recorded}; it may be \ + private, deleted, or a fork this build cannot name), so there is nowhere to push the \ + work — its commits stay on the task's local branch", meta.provider.change_noun(), meta.number ) }) } +/// The `head` a create-pull request takes. +/// +/// The branch alone when the head repository IS the target, and `owner:branch` +/// when it is not — the qualified form GitHub and Gitea both accept, and the +/// only way to say "from my fork" without a second field. GitLab is excluded +/// because it does not read a qualified head at all: a cross-project merge +/// request is addressed to the FORK and names the target in its body (see +/// [`NewPullRequest`]), so its `source_branch` stays bare. +fn head_ref_for( + provider: ForgeProvider, + push_repo: &str, + target_repo: &str, + branch: &str, +) -> String { + if provider == ForgeProvider::GitLab || crate::forge::same_repo(push_repo, target_repo) { + return branch.to_string(); + } + let owner = push_repo.split('/').next().unwrap_or(push_repo); + format!("{owner}:{branch}") +} + /// Pick the launch mode for a pump-driven launch from the task's history: a /// task with a prior conversation continues (retry semantics); a pristine one /// starts fresh. Explicit returns launch directly with `LaunchMode::Return`. @@ -9227,7 +9314,36 @@ mod tests { /// `(repository, work branch, remote branch)` of every push. pushes: Mutex>, created: Mutex>, + /// `(source project id, target project id)` of every create, in order. + /// GitLab's way of naming both ends of a cross-project merge request — + /// the fork it is created ON and the project it is aimed at — and the + /// pair a same-project create must leave out entirely. + source_projects: Mutex, Option)>>, + /// `(repository the search was AIMED at, head branch)` of every + /// lookup — the head side of a cross-repository delivery is a claim + /// about which repository that must be, and this is what makes it + /// observable from a test. + /// `(repository searched, head repository, head branch)` of every + /// lookup. The search's aim is the one that decides whether a retry + /// sees what it already opened — a pull request lives in the repository + /// being merged INTO — so the fixtures record both sides and the + /// cross-repository tests assert them; `collection_owner` below is what + /// makes a wrong aim fail rather than merely be logged. + finds: Mutex>, + /// Which repository's collection really holds `existing`. + /// + /// A forge lists what its own collection contains, so a search aimed + /// anywhere else comes back empty. `None` = the fixture does not model + /// it, which is fine for a same-repository delivery (the head and the + /// collection are the same repository, so the aim cannot diverge); the + /// cross-repository fixtures set it, so aiming at the head fails the + /// adoption assertions instead of passing quietly. + collection_owner: Mutex>, existing: Mutex>, + /// OID of the branch the last push published — what a real forge + /// records as the new pull request's head, and therefore what the + /// four-way match compares against on the next delivery. + pushed_head: Mutex>, /// What the source repository's base branch points at. The fixture /// seeds it with the task's own base, i.e. "nothing unpushed". remote_base: Mutex>, @@ -9282,6 +9398,11 @@ mod tests { work_branch.to_string(), remote_branch.to_string(), )); + // What the forge now sees on that branch — the OID a created pull + // request will report, and the anchor every later delivery is + // matched against. + *self.pushed_head.lock().await = + task_git::rev_parse(worktree_path, work_branch).await.ok(); if let Some(changed) = self.after_push.lock().await.clone() { *self.existing.lock().await = vec![changed]; } @@ -9290,12 +9411,22 @@ mod tests { async fn find_pulls( &self, - _ctx: &DeliveryCtx<'_>, - _head_branch: &str, + ctx: &DeliveryCtx<'_>, + head_repo: &str, + head_branch: &str, ) -> Result, String> { if let Some(e) = &self.find_error { return Err(e.clone()); } + self.finds.lock().await.push(( + ctx.owner_repo.to_string(), + head_repo.to_string(), + head_branch.to_string(), + )); + let owner = self.collection_owner.lock().await.clone(); + if owner.is_some_and(|owner| owner != ctx.owner_repo) { + return Ok(Vec::new()); + } Ok(self.existing.lock().await.clone()) } @@ -9353,7 +9484,7 @@ mod tests { async fn create_pull( &self, - _ctx: &DeliveryCtx<'_>, + ctx: &DeliveryCtx<'_>, req: &NewPullRequest<'_>, ) -> Result { if let Some(e) = &self.create_error { @@ -9365,16 +9496,44 @@ mod tests { req.base.to_string(), req.draft, )); - Ok(ForgePr { + self.source_projects + .lock() + .await + .push((req.source_project_id, req.target_project_id)); + // The forge's own reading of the request. GitLab names a foreign + // SOURCE PROJECT by number in every list payload — reporting it + // that way is what makes the claim path's placeholder substitution + // load-bearing rather than decorative. The other two spell a fork + // as `owner:branch`, so the head lives in `owner/{name}` while the + // pull request itself sits in the target. + let head_repo = if let Some(id) = req.source_project_id { + format!("project-{id}") + } else if let Some((owner, _)) = req.head.split_once(':') { + match ctx.owner_repo.split_once('/') { + Some((_, name)) => format!("{owner}/{name}"), + None => ctx.owner_repo.to_string(), + } + } else { + ctx.owner_repo.to_string() + }; + let pr = ForgePr { number: 42, html_url: "https://github.test/acme/app/pull/42".to_string(), state: "open".to_string(), merged: false, - head_sha: "unused-by-the-fake".to_string(), - head_ref: req.head.to_string(), - head_repo: "acme/app".to_string(), + head_sha: self + .pushed_head + .lock() + .await + .clone() + .unwrap_or_else(|| "unused-by-the-fake".to_string()), + head_ref: req.head.split_once(':').map_or(req.head, |(_, b)| b).to_string(), + head_repo, base_ref: req.base.to_string(), - }) + }; + // Published: the NEXT delivery of the same task has to find it. + self.existing.lock().await.push(pr.clone()); + Ok(pr) } } @@ -9528,6 +9687,33 @@ mod tests { } } + /// The fixture's task re-pointed at GitLab, with the fork's project id the + /// trigger would have resolved (`resolve_project_id`). + async fn set_gitlab_fork(f: &Delivery, fork_repo: &str, fork_project_id: i64, target_id: i64) { + use sea_orm::{ActiveModelTrait, Set}; + + let task = row(&f.engine, f.task_id).await; + let mut meta: ForgeSourceMeta = + serde_json::from_str(task.source_meta.as_deref().expect("meta")).expect("decode"); + meta.provider = crate::forge::ForgeProvider::GitLab; + meta.fork_repo = Some(fork_repo.to_string()); + meta.fork_project_id = Some(fork_project_id); + meta.owner_project_id = Some(target_id); + // Same as `set_fork`: the fixture's merge requests are the source + // project's, not the fork's. + f.forge + .collection_owner + .lock() + .await + .replace("acme/app".to_string()); + let mut active: crate::db::entities::work_task::ActiveModel = task.into(); + active.source_meta = Set(Some(serde_json::to_string(&meta).expect("encode"))); + active + .update(&f.engine.db.conn) + .await + .expect("record the gitlab fork"); + } + async fn row(engine: &Arc, id: i32) -> crate::db::entities::work_task::Model { work_task_service::get_model(&engine.db.conn, id) .await @@ -9576,6 +9762,187 @@ mod tests { assert!(f.engine.merging.lock().await.is_empty()); } + /// The fixture's task with a recorded FORK — what the trigger writes when + /// the panel's folder points at a parent (the picker's selection is the + /// SOURCE there; the work is pushed to the folder's own `origin`). + async fn set_fork(f: &Delivery, fork_repo: &str) { + use sea_orm::{ActiveModelTrait, Set}; + + let task = row(&f.engine, f.task_id).await; + let mut meta: ForgeSourceMeta = + serde_json::from_str(task.source_meta.as_deref().expect("meta")).expect("decode"); + meta.fork_repo = Some(fork_repo.to_string()); + let mut active: crate::db::entities::work_task::ActiveModel = task.into(); + active.source_meta = Set(Some(serde_json::to_string(&meta).expect("encode"))); + active.update(&f.engine.db.conn).await.expect("record the fork"); + // The fixture's pull requests live in the source's collection, so a + // search aimed at the fork comes back empty — the way a real forge + // answers. + f.forge + .collection_owner + .lock() + .await + .replace("acme/app".to_string()); + } + + /// The row a retry finds: back in review, nothing settled, the delivery's + /// in-flight intent spent. That is the state an interrupted delivery leaves + /// behind, and the one the second attempt has to handle. + async fn reset_to_review(f: &Delivery) { + use sea_orm::{ActiveModelTrait, Set}; + + let task = row(&f.engine, f.task_id).await; + let mut active: crate::db::entities::work_task::ActiveModel = task.into(); + active.status = Set(WorkTaskStatus::Review); + active.completion_kind = Set(None); + active.merge_state = Set(None); + active.finished_at = Set(None); + active.update(&f.engine.db.conn).await.expect("back to review"); + } + + /// The fork workflow end to end: the panel is pointed at the parent, so the + /// issue is the parent's while the branch codeg can write to is the user's + /// own copy. The push has to land THERE and the pull request has to say so — + /// otherwise the account is asked to write to a repository it can only + /// read, and the task fails at the very end, after the agent has done the + /// work. + #[tokio::test] + async fn an_issue_task_pushes_to_its_fork_and_opens_a_cross_repo_pull_request() { + let f = delivery_fixture(FakeForge::default()).await; + set_fork(&f, "me/app").await; + + let url = f + .engine + .deliver_pr(f.task_id, None, false, false) + .await + .expect("delivery"); + + assert_eq!(url, "https://github.test/acme/app/pull/42"); + assert_eq!( + f.forge.pushes.lock().await.as_slice(), + [("me/app".to_string(), "task/7".to_string(), "task/7".to_string())], + "the work branch lands in the fork, not in the source" + ); + assert_eq!( + f.forge.created.lock().await.as_slice(), + [( + // No title was passed, so the task's own title is used. + "#7 · Fix the login flow".to_string(), + "me:task/7".to_string(), + "main".to_string(), + false + )], + "the head is the qualified form that makes the pull request cross-repository" + ); + // The search for an existing pull request: LISTED in the target's + // collection, filtered by the fork that holds it. Aiming the list at + // the head's repository returns nothing — which is how a retry opens a + // second pull request. + assert_eq!( + f.forge.finds.lock().await.as_slice(), + [("acme/app".to_string(), "me/app".to_string(), "task/7".to_string())], + "the collection is the one being merged into, the head filter names the fork" + ); + + let task = row(&f.engine, f.task_id).await; + assert_eq!(task.status, WorkTaskStatus::Done); + assert_eq!(task.completion_kind.as_deref(), Some("delivered_pr")); + } + + /// GitLab's half of the same story: the far side of a cross-project merge + /// request is a project ID (not a qualified head ref), the list payload + /// comes back as `project-{id}` — and the retry still has to ADOPT the merge + /// request the first delivery opened rather than open a second one. + #[tokio::test] + async fn a_gitlab_task_delivers_from_its_fork_and_adopts_its_own_merge_request() { + let f = delivery_fixture(FakeForge::default()).await; + set_gitlab_fork(&f, "me/app", 4711, 4712).await; + + f.engine + .deliver_pr(f.task_id, None, false, false) + .await + .expect("first delivery"); + + assert_eq!( + f.forge.pushes.lock().await.as_slice(), + [("me/app".to_string(), "task/7".to_string(), "task/7".to_string())] + ); + assert_eq!( + f.forge.created.lock().await.as_slice(), + [( + "#7 · Fix the login flow".to_string(), + // The branch stays BARE on GitLab: the other side is the id. + "task/7".to_string(), + "main".to_string(), + false + )] + ); + assert_eq!( + f.forge.source_projects.lock().await.as_slice(), + [(Some(4711), Some(4712))], + "a cross-project merge request names BOTH ends: the fork it is created \ + on and the project it is aimed at" + ); + // What the forge now serves — and what a retry has to recognise. + assert!( + f.forge + .existing + .lock() + .await + .iter() + .any(|pr| pr.head_repo == "project-4711"), + "the list payload names the fork by number" + ); + + reset_to_review(&f).await; + f.engine + .deliver_pr(f.task_id, None, false, false) + .await + .expect("second delivery"); + + assert_eq!( + f.forge.created.lock().await.len(), + 1, + "the placeholder must be resolved, or the retry opens a second merge request" + ); + assert_eq!(row(&f.engine, f.task_id).await.status, WorkTaskStatus::Done); + } + + /// The retry half of the same story, and why the head-side aim is + /// load-bearing rather than cosmetic: a second delivery that searched the + /// SOURCE for this branch would come back empty, conclude nothing exists, + /// and open a SECOND pull request for the same head — which the real forge + /// answers with a 422, after the work has already been pushed. + #[tokio::test] + async fn a_retry_adopts_the_cross_repo_pull_request_instead_of_opening_a_second() { + let f = delivery_fixture(FakeForge::default()).await; + set_fork(&f, "me/app").await; + f.engine + .deliver_pr(f.task_id, None, false, false) + .await + .expect("first delivery"); + assert_eq!(f.forge.created.lock().await.len(), 1); + + reset_to_review(&f).await; + f.engine + .deliver_pr(f.task_id, None, false, false) + .await + .expect("second delivery"); + + assert_eq!( + f.forge.created.lock().await.len(), + 1, + "the pull request the first delivery opened must be adopted, not duplicated" + ); + assert_eq!( + f.forge.source_projects.lock().await.as_slice(), + [(None, None)], + "the project ids are GitLab's spelling of the far side — GitHub and \ + Gitea put it in the head instead, and must not send them" + ); + assert_eq!(row(&f.engine, f.task_id).await.status, WorkTaskStatus::Done); + } + /// The delivery's own version of the offer both other acceptances make: /// take the checkout along. Safe here in a way a bare `branch -D` is not — /// by the time this runs the commits it destroys locally are on the forge. @@ -11774,6 +12141,82 @@ mod tests { assert_eq!(row(&f.engine, f.task_id).await.status, WorkTaskStatus::Done); } + /// A review whose head is somebody ELSE's fork is delivered to that fork, + /// not refused up front: whether this account may write there is a + /// server-side fact (the author's "allow edits from maintainers"), so the + /// push is what decides — which is also the only way a maintainer can push + /// a fix into a contributor's review. + #[tokio::test] + async fn a_review_from_a_third_party_fork_is_pushed_there() { + let f = delivery_fixture(FakeForge::default()).await; + as_pull_request_task(&f, open_pull("at-the-old-head", "feature", "contributor/app")) + .await; + f.forge.existing.lock().await.push(open_pull( + "at-the-old-head", + "feature", + "contributor/app", + )); + + f.engine + .deliver_pr(f.task_id, None, false, false) + .await + .expect("pushed back into the review's own fork"); + assert_eq!( + f.forge.pushes.lock().await.as_slice(), + [( + "contributor/app".to_string(), + "task/7".to_string(), + "feature".to_string() + )], + "the branch goes to the fork the review came from, not to the source" + ); + assert_eq!(row(&f.engine, f.task_id).await.status, WorkTaskStatus::Done); + } + + /// And when that fork refuses us — the author never allowed edits by + /// maintainers, or this account has no business there — the refusal has to + /// name the way out rather than just report the forge's own words. The work + /// stays on the task's branch, so a retry after the box is ticked lands. + #[tokio::test] + async fn a_refused_push_into_a_strangers_fork_names_the_way_out() { + let forge = FakeForge { + push_error: Some( + "remote: You are not allowed to push code to this project. error: 403".into(), + ), + ..FakeForge::default() + }; + let f = delivery_fixture(forge).await; + as_pull_request_task(&f, open_pull("at-the-old-head", "feature", "contributor/app")) + .await; + f.forge.existing.lock().await.push(open_pull( + "at-the-old-head", + "feature", + "contributor/app", + )); + + let err = f + .engine + .deliver_pr(f.task_id, None, false, false) + .await + .expect_err("no permission on the fork"); + assert!(err.contains("contributor/app"), "names the fork: {err}"); + assert!( + err.contains("allow edits from maintainers"), + "names the way out: {err}" + ); + assert_eq!( + row(&f.engine, f.task_id).await.status, + WorkTaskStatus::Review, + "the task goes back to review with its work intact" + ); + assert!( + task_git::rev_parse(f.root.to_str().unwrap(), "refs/heads/task/7") + .await + .is_ok(), + "and the commits are still on the branch" + ); + } + /// Everything that would make the push land somewhere it does not belong /// is refused BEFORE the push, with the task untouched in review. #[tokio::test] diff --git a/src/lib/types.ts b/src/lib/types.ts index 9995e1fbd2..1152b05cde 100644 --- a/src/lib/types.ts +++ b/src/lib/types.ts @@ -1691,6 +1691,11 @@ export interface ForgeSourceMeta { head_ref?: string | null head_sha?: string | null head_repo?: string | null + /** The repository the task's work is pushed to when it is not the source — + * the folder's `origin` recorded at trigger time, for the fork workflow + * (the panel reads the parent; the branch codeg writes is the user's own + * copy). Absent = push to the source. */ + fork_repo?: string | null /** URL of the PR created by the delivery acceptance path (P1). */ result_pr?: string | null /** The trigger dialog's write-back answer, frozen at trigger time. Absent on From 98372ce5bcf6a93827a1d20c9c109014db3db487 Mon Sep 17 00:00:00 2001 From: dawn <93917549+dawNotPoi@users.noreply.github.com> Date: Fri, 18 Sep 2026 21:31:19 +0800 Subject: [PATCH 6/6] fix(forge): persist remote selections per folder key --- src-tauri/src/forge/remotes.rs | 328 +++++++++++++++++++-------------- 1 file changed, 192 insertions(+), 136 deletions(-) diff --git a/src-tauri/src/forge/remotes.rs b/src-tauri/src/forge/remotes.rs index 8e6a76f222..90fe56f334 100644 --- a/src-tauri/src/forge/remotes.rs +++ b/src-tauri/src/forge/remotes.rs @@ -1,39 +1,42 @@ //! Which git remote each folder's forge panel reads. //! //! **Not a field of [`ForgePanelSettings`](super::settings::ForgePanelSettings)**, -//! and that separation is the point of this module. The two have opposite write -//! patterns: the picker saves this one on every click, while the panel settings -//! are a blob a dialog rewrites WHOLESALE — "use global defaults" saves by -//! dropping a folder's whole row. Sharing a blob made the picker's write detach -//! every other setting from the global row, and made the dialog's drop take the -//! selection with it: a choice the user watched succeed died without a word. +//! and not one shared JSON blob either. Each folder owns one metadata key: +//! `forge_panel_remote:`. //! -//! One key, one map: the folders that have chosen a remote. **No global row**, -//! unlike the panel settings — a remote names a git remote that exists in ONE -//! folder's worktree, so there is nothing for an "all folders" default to mean. -//! A folder with no entry reads the historical `origin`; absence is the default -//! answer, so there is no third state to keep in sync with the entry itself. +//! That shape is deliberate. The picker can be used from more than one window, +//! and the database pool has multiple connections. A shared read-modify-write +//! blob lets two saves race: //! -//! Stored as ONE JSON object in `app_metadata`. A save is a read-modify-write of -//! the single blob — the same shape the panel settings use — so a save against -//! one folder carries the others through untouched rather than replacing them -//! with whatever the saving client happened to be holding. +//! 1. window A reads the map, +//! 2. window B reads the same map, +//! 3. A writes folder 1, +//! 4. B writes folder 2 from its stale copy and silently drops folder 1. +//! +//! Per-folder keys remove that race entirely: unrelated folders never rewrite +//! one another. A folder with no key reads the historical `origin`; absence is +//! the default answer. +//! +//! The public API still returns a [`ForgeRemoteStore`] containing every saved +//! folder so the frontend can switch folders without another round trip. That +//! store is assembled from independent rows at read time; it is not persisted +//! as one value. use std::collections::BTreeMap; -use sea_orm::DatabaseConnection; +use sea_orm::{ColumnTrait, DatabaseConnection, EntityTrait, QueryFilter}; use serde::{Deserialize, Serialize}; +use crate::db::entities::app_metadata; use crate::db::error::DbError; use crate::db::service::app_metadata_service; -/// `app_metadata` key holding the whole store. -const REMOTES_KEY: &str = "forge_panel_remotes"; +/// Prefix for one folder's selection. The suffix is the decimal folder id. +const REMOTE_KEY_PREFIX: &str = "forge_panel_remote:"; -/// Every folder's selection at once. +/// Every folder's selection at once, as exposed to the frontend. /// -/// Keyed by folder id: a folder with no entry is on the default remote, so the -/// map holds only the folders that have chosen one. +/// Persistence is per-folder even though the wire shape is aggregated. #[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] pub struct ForgeRemoteStore { #[serde(default)] @@ -45,24 +48,14 @@ impl ForgeRemoteStore { pub fn selected(&self, folder_id: i32) -> Option<&str> { self.folders.get(&folder_id).map(String::as_str) } +} - /// Apply one folder's save. Pure, so the arithmetic can be tested without a - /// database — [`save`] is this plus the read and the write. - /// - /// A blank or absent name CLEARS the entry (back to the default) rather - /// than storing an empty string the resolver would then have to know to - /// ignore — the same normalization [`super::settings`] applies to its own - /// option fields. - fn apply(&mut self, folder_id: i32, remote: Option) { - match trim(remote) { - Some(name) => { - self.folders.insert(folder_id, name); - } - None => { - self.folders.remove(&folder_id); - } - } - } +fn remote_key(folder_id: i32) -> String { + format!("{REMOTE_KEY_PREFIX}{folder_id}") +} + +fn parse_folder_id(key: &str) -> Option { + key.strip_prefix(REMOTE_KEY_PREFIX)?.parse().ok() } fn trim(value: Option) -> Option { @@ -71,141 +64,204 @@ fn trim(value: Option) -> Option { .filter(|v| !v.is_empty()) } -/// Read every folder's selection. Never fails on content: a blob this build -/// cannot parse (hand-edited, or written by a future one) reads as empty — the -/// default remote — rather than taking down every forge read in every folder. +/// Read every saved folder selection. +/// +/// Each row is independent in storage; this aggregation exists only for the +/// frontend's "load once, switch folders locally" API. pub async fn load(conn: &DatabaseConnection) -> Result { - let raw = app_metadata_service::get_value(conn, REMOTES_KEY).await?; - Ok(decode(raw.as_deref()).unwrap_or_default()) + let rows = app_metadata::Entity::find() + .filter(app_metadata::Column::Key.starts_with(REMOTE_KEY_PREFIX)) + .filter(app_metadata::Column::DeletedAt.is_null()) + .all(conn) + .await?; + + let mut folders = BTreeMap::new(); + for row in rows { + let Some(folder_id) = parse_folder_id(&row.key) else { + continue; + }; + let Some(remote) = trim(Some(row.value)) else { + continue; + }; + folders.insert(folder_id, remote); + } + + Ok(ForgeRemoteStore { folders }) } -/// The one folder's selection — what the panel's resolution actually asks for. +/// Read one folder directly from its own key — the hot path used by every forge +/// operation. No shared store is read or rewritten. pub async fn load_selected( conn: &DatabaseConnection, folder_id: i32, ) -> Result, DbError> { - Ok(load(conn).await?.selected(folder_id).map(str::to_string)) + let raw = app_metadata_service::get_value(conn, &remote_key(folder_id)).await?; + Ok(trim(raw)) } -/// Write ONE folder's selection and hand back every folder's as stored. +/// Save exactly one folder's selection and return the aggregated frontend view. +/// +/// A blank or absent name clears the key, putting the folder back on the +/// historical `origin` default. Different folder ids touch different database +/// rows, so concurrent saves cannot overwrite one another. pub async fn save( conn: &DatabaseConnection, folder_id: i32, remote: Option, ) -> Result { - let mut store = load(conn).await?; - store.apply(folder_id, remote); - let encoded = serde_json::to_string(&store) - .map_err(|e| DbError::Validation(format!("remote selection not serializable: {e}")))?; - app_metadata_service::upsert_value(conn, REMOTES_KEY, &encoded).await?; - Ok(store) -} - -/// Decode a stored blob, or `None` for anything this build cannot read. -fn decode(raw: Option<&str>) -> Option { - serde_json::from_str(raw?).ok() + let key = remote_key(folder_id); + match trim(remote) { + Some(name) => app_metadata_service::upsert_value(conn, &key, &name).await?, + None => { + app_metadata::Entity::delete_many() + .filter(app_metadata::Column::Key.eq(key)) + .exec(conn) + .await?; + } + } + load(conn).await } #[cfg(test)] mod tests { use super::*; - fn store_with(pairs: &[(i32, &str)]) -> ForgeRemoteStore { - let mut store = ForgeRemoteStore::default(); - for (id, name) in pairs { - store.apply(*id, Some((*name).to_string())); - } - store + /// A folder with no key is on the default, not on a stored empty string the + /// resolver would have to special-case. + #[tokio::test] + async fn an_untouched_folder_has_no_selection_of_its_own() { + let db = crate::db::test_helpers::fresh_in_memory_db().await; + assert_eq!(load_selected(&db.conn, 1).await.expect("read"), None); + assert_eq!(load(&db.conn).await.expect("store"), ForgeRemoteStore::default()); } - /// A folder with nothing saved is on the default, not on a stored empty - /// string the resolver would have to filter out. - #[test] - fn an_untouched_folder_has_no_selection_of_its_own() { - let store = ForgeRemoteStore::default(); - assert_eq!(store.selected(1), None); - assert_eq!(store_with(&[(1, "upstream")]).selected(2), None); - } + /// One folder's value is trimmed on write; blank and absent values clear the + /// key so the resolver goes back to `origin`. + #[tokio::test] + async fn saving_trims_the_name_and_clearing_removes_the_key() { + let db = crate::db::test_helpers::fresh_in_memory_db().await; - /// The picker's own name goes in trimmed — a hand-edited blob must not - /// resolve against a remote whose name is spelled with spaces around it. - /// A blank or absent name CLEARS the entry: that is how the picker's - /// "default (origin)" answer is stored. - #[test] - fn saving_trims_the_name_and_clearing_removes_the_entry() { - let mut store = ForgeRemoteStore::default(); - store.apply(1, Some(" upstream ".into())); - assert_eq!(store.selected(1), Some("upstream")); - - store.apply(1, Some(" ".into())); - assert_eq!(store.selected(1), None); - assert!(store.folders.is_empty(), "blank is a clear, not an entry"); - - store.apply(1, Some("upstream".into())); - store.apply(1, None); - assert_eq!(store.selected(1), None); - // Idempotent: clearing an unset folder is fine. - store.apply(1, None); - assert!(store.folders.is_empty()); - } + save(&db.conn, 1, Some(" upstream ".into())) + .await + .expect("save"); + assert_eq!( + load_selected(&db.conn, 1).await.expect("selected").as_deref(), + Some("upstream") + ); - /// One folder's save must not rewrite another's — the whole store is - /// written back on every save, so this is the one way a picker click could - /// silently move a different folder. - #[test] - fn a_save_against_one_folder_leaves_the_others_untouched() { - let mut store = store_with(&[(1, "upstream"), (2, "backup")]); - store.apply(1, Some("origin".into())); - assert_eq!(store.selected(2), Some("backup")); - store.apply(1, None); - assert_eq!(store.selected(2), Some("backup")); - } + save(&db.conn, 1, Some(" ".into())) + .await + .expect("blank clears"); + assert_eq!(load_selected(&db.conn, 1).await.expect("selected"), None); - /// JSON has no integer keys, so folder ids go out as strings — and a store - /// that could not read its own output would lose every selection on the - /// next load. - #[test] - fn folder_keys_survive_the_json_round_trip() { - let store = store_with(&[(42, "upstream")]); - let encoded = serde_json::to_string(&store).expect("serializable"); - assert!(encoded.contains("\"42\""), "{encoded}"); - assert_eq!(decode(Some(&encoded)).expect("decodes"), store); + save(&db.conn, 1, Some("upstream".into())) + .await + .expect("save again"); + save(&db.conn, 1, None).await.expect("clear"); + assert_eq!(load_selected(&db.conn, 1).await.expect("selected"), None); } - /// Anything that is not JSON at all is the default, not an error — this - /// read runs on every forge operation, in every folder. - #[test] - fn a_blob_this_build_cannot_parse_reads_as_empty() { - assert_eq!(decode(None).unwrap_or_default(), ForgeRemoteStore::default()); - assert_eq!(decode(Some("not json")).unwrap_or_default(), ForgeRemoteStore::default()); - // A shape from a future build keeps the fields this one knows. - let future = decode(Some(r#"{"folders":{"7":"upstream"},"scope":"worktree"}"#)) - .expect("unknown fields are ignored"); - assert_eq!(future.selected(7), Some("upstream")); + /// The frontend still receives every folder at once even though persistence + /// is one key per folder. + #[tokio::test] + async fn load_aggregates_the_independent_folder_keys() { + let db = crate::db::test_helpers::fresh_in_memory_db().await; + + save(&db.conn, 3, Some("upstream".into())) + .await + .expect("folder 3"); + save(&db.conn, 4, Some("backup".into())) + .await + .expect("folder 4"); + + let store = load(&db.conn).await.expect("reload"); + assert_eq!(store.selected(3), Some("upstream")); + assert_eq!(store.selected(4), Some("backup")); + assert_eq!(store.selected(5), None); } - /// Through a real database, because [`save`] is a READ-MODIFY-WRITE: the - /// pure tests above prove the arithmetic, and this proves the load and the - /// store are the same blob — one key typo and every save would land on - /// something the next load never reads. + /// This is the race the shared JSON blob could not make safe: two windows + /// save different folders at the same time. With per-folder keys each write + /// targets a different row, so neither can erase the other. #[tokio::test] - async fn selections_persist_independently_across_saves() { + async fn concurrent_saves_to_different_folders_cannot_overwrite_each_other() { let db = crate::db::test_helpers::fresh_in_memory_db().await; - // Nothing stored is the default, not an error. - assert_eq!(load(&db.conn).await.expect("empty"), ForgeRemoteStore::default()); + let (left, right) = tokio::join!( + save(&db.conn, 3, Some("upstream".into())), + save(&db.conn, 4, Some("backup".into())) + ); + left.expect("folder 3 save"); + right.expect("folder 4 save"); - save(&db.conn, 3, Some("upstream".into())).await.expect("folder 3"); - save(&db.conn, 4, Some("backup".into())).await.expect("folder 4"); let store = load(&db.conn).await.expect("reload"); assert_eq!(store.selected(3), Some("upstream")); assert_eq!(store.selected(4), Some("backup")); - assert_eq!(load_selected(&db.conn, 3).await.expect("selected"), Some("upstream".into())); - assert_eq!(load_selected(&db.conn, 5).await.expect("unset"), None); + } + + /// Clearing one folder deletes only that folder's row. + #[tokio::test] + async fn clearing_one_folder_leaves_the_others_untouched() { + let db = crate::db::test_helpers::fresh_in_memory_db().await; + + save(&db.conn, 3, Some("upstream".into())) + .await + .expect("folder 3"); + save(&db.conn, 4, Some("backup".into())) + .await + .expect("folder 4"); - // Clearing one folder leaves the other alone. save(&db.conn, 3, None).await.expect("clear folder 3"); - assert_eq!(load(&db.conn).await.expect("reload").selected(3), None); - assert_eq!(load_selected(&db.conn, 4).await.expect("folder 4"), Some("backup".into())); + let store = load(&db.conn).await.expect("reload"); + assert_eq!(store.selected(3), None); + assert_eq!(store.selected(4), Some("backup")); + } + + /// Malformed keys or blank values are ignored when building the aggregate + /// store; one bad metadata row must not break every forge panel. + #[tokio::test] + async fn malformed_rows_do_not_break_the_store() { + let db = crate::db::test_helpers::fresh_in_memory_db().await; + + app_metadata_service::upsert_value( + &db.conn, + &format!("{REMOTE_KEY_PREFIX}not-an-id"), + "upstream", + ) + .await + .expect("malformed id"); + app_metadata_service::upsert_value( + &db.conn, + &format!("{REMOTE_KEY_PREFIX}7"), + " ", + ) + .await + .expect("blank value"); + app_metadata_service::upsert_value( + &db.conn, + &format!("{REMOTE_KEY_PREFIX}8"), + " backup ", + ) + .await + .expect("valid value"); + + let store = load(&db.conn).await.expect("load"); + assert_eq!(store.selected(7), None); + assert_eq!(store.selected(8), Some("backup")); + } + + /// The aggregated wire shape still serializes folder ids as JSON object + /// keys, which is what the TypeScript `Record` consumes. + #[test] + fn folder_keys_survive_the_json_round_trip() { + let store = ForgeRemoteStore { + folders: [(42, "upstream".to_string())].into_iter().collect(), + }; + let encoded = serde_json::to_string(&store).expect("serializable"); + assert!(encoded.contains("\"42\""), "{encoded}"); + assert_eq!( + serde_json::from_str::(&encoded).expect("decodes"), + store + ); } }