From 46f09e4ac1839da0f016da47a53ea83fcd7d0a6c Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 16 Jul 2026 11:41:12 -0700 Subject: [PATCH] FN-8115: consolidate GitHub import settings reads Reuse request-scoped project settings across GitHub issue import operations. - Read project settings once for single and batch imports - Share settings with translation and GitHub tracking resolution - Cover settings reuse and translated/original batch content Files changed: .../dashboard/src/__tests__/routes-github.test.ts | 105 ++++++++++++++++++++- .../dashboard/src/routes/register-git-github.ts | 56 ++++++++--- 2 files changed, 145 insertions(+), 16 deletions(-) Fusion-Task-Id: FN-8115 Fusion-Task-Lineage: acc046c5-d599-4df4-b380-a1e167d45548 Co-authored-by: Fusion (runfusion.ai) --- .../src/__tests__/routes-github.test.ts | 105 +++++++++++++++++- .../src/routes/register-git-github.ts | 56 +++++++--- 2 files changed, 145 insertions(+), 16 deletions(-) diff --git a/packages/dashboard/src/__tests__/routes-github.test.ts b/packages/dashboard/src/__tests__/routes-github.test.ts index c7ce34340b..4481fe43fc 100644 --- a/packages/dashboard/src/__tests__/routes-github.test.ts +++ b/packages/dashboard/src/__tests__/routes-github.test.ts @@ -44,11 +44,20 @@ const mockCentralListProjects = vi.fn().mockResolvedValue([]); const mockCentralInit = vi.fn().mockResolvedValue(undefined); const mockCentralClose = vi.fn().mockResolvedValue(undefined); const mockCentralReconcileProjectStatuses = vi.fn().mockResolvedValue(undefined); -const { mockPerformUpdateCheck, mockClearUpdateCheckCache, mockExecSync, mockExecFile } = vi.hoisted(() => ({ +const { + mockPerformUpdateCheck, + mockClearUpdateCheckCache, + mockExecSync, + mockExecFile, + mockGetCachedImportTranslation, + mockResolveTargetLocale, +} = vi.hoisted(() => ({ mockPerformUpdateCheck: vi.fn(), mockClearUpdateCheckCache: vi.fn(), mockExecSync: vi.fn(), mockExecFile: vi.fn(), + mockGetCachedImportTranslation: vi.fn(), + mockResolveTargetLocale: vi.fn(), })); vi.mock("../update-check.js", async () => { @@ -60,6 +69,19 @@ vi.mock("../update-check.js", async () => { }; }); +vi.mock("../import-translate-service.js", async () => { + const actual = await vi.importActual( + "../import-translate-service.js", + ); + mockGetCachedImportTranslation.mockImplementation(actual.getCachedImportTranslation); + mockResolveTargetLocale.mockImplementation(actual.resolveTargetLocale); + return { + ...actual, + getCachedImportTranslation: mockGetCachedImportTranslation, + resolveTargetLocale: mockResolveTargetLocale, + }; +}); + vi.mock("node:child_process", async () => { const actual = await vi.importActual("node:child_process"); mockExecSync.mockImplementation(((...args: Parameters) => actual.execSync(...args)) as typeof actual.execSync); @@ -565,6 +587,8 @@ describe("POST /github/issues/import", () => { let getIssueDetailSpy: ReturnType; beforeEach(() => { + mockGetCachedImportTranslation.mockClear(); + mockResolveTargetLocale.mockClear(); mockIsGhAuthenticated.mockReturnValue(true); getIssueSpy = vi.fn(); vi.spyOn(GitHubClient.prototype, "getIssue").mockImplementation(getIssueSpy); @@ -635,6 +659,38 @@ describe("POST /github/issues/import", () => { }); }); + /* + FNXC:GitHubImportTranslate 2026-07-16-11:22: + FN-8115 requires a single project-settings read per single import even though translation and tracking both depend on it. The route test protects the request-scoped sharing invariant rather than the resolver internals. + */ + it("reads project settings once for a single issue import", async () => { + getIssueSpy.mockResolvedValueOnce(mockGitHubIssue); + + const res = await REQUEST(buildApp(), "POST", "/api/github/issues/import", JSON.stringify({ owner: "owner", repo: "repo", issueNumber: 1 }), { + "Content-Type": "application/json", + }); + + expect(res.status).toBe(201); + expect(store.getSettings).toHaveBeenCalledTimes(1); + }); + + it("uses cached translated prose for an open single issue", async () => { + (store.getSettings as ReturnType).mockResolvedValue({ githubImportAutoTranslate: true }); + mockResolveTargetLocale.mockReturnValueOnce("en"); + mockGetCachedImportTranslation.mockResolvedValueOnce({ title: "Translated title", body: "Translated body" }); + getIssueSpy.mockResolvedValueOnce({ ...mockGitHubIssue, title: "Original title", body: "Original body" }); + + const res = await REQUEST(buildApp(), "POST", "/api/github/issues/import", JSON.stringify({ owner: "owner", repo: "repo", issueNumber: 1 }), { + "Content-Type": "application/json", + }); + + expect(res.status).toBe(201); + expect(store.createTask).toHaveBeenCalledWith(expect.objectContaining({ + title: "Translated title", + description: "Translated body\n\nSource: https://github.com/owner/repo/issues/1", + })); + }); + /* FNXC:GitHubImportAttachments 2026-07-15-11:20: Route-level proof that the import path itself downloads issue screenshots into task attachments — the executor's `## Attachments` section and triage's vision blocks only fire for attachments that actually exist on the task, so the wiring (not just the helper) is the invariant. @@ -946,6 +1002,8 @@ describe("POST /github/issues/batch-import", () => { beforeEach(() => { __resetBatchImportRateLimiter(); + mockGetCachedImportTranslation.mockClear(); + mockResolveTargetLocale.mockClear(); fetchSpy = vi.fn(); globalThis.fetch = fetchSpy as any; @@ -1112,6 +1170,7 @@ describe("POST /github/issues/batch-import", () => { expect(res.body.results.every((r: { success: boolean }) => r.success)).toBe(true); expect(throttledSpy).toHaveBeenCalledTimes(3); expect(store.createTask).toHaveBeenCalledTimes(3); + expect(store.getSettings).toHaveBeenCalledTimes(1); expect(store.createTask).toHaveBeenNthCalledWith(1, expect.objectContaining({ sourceIssue: { provider: "github", @@ -1141,6 +1200,50 @@ describe("POST /github/issues/batch-import", () => { })); }); + /* + FNXC:GitHubImportTranslate 2026-07-16-11:22: + Batch imports reuse one request-scoped settings object for every item. Cached translations still apply to open issues, while closed issues always retain original prose even when the cache has a hit. + */ + it("preserves open translations and closed original prose in a batch", async () => { + (store.getSettings as ReturnType).mockResolvedValue({ githubImportAutoTranslate: true }); + mockResolveTargetLocale.mockReturnValueOnce("en").mockReturnValueOnce("en"); + mockGetCachedImportTranslation.mockResolvedValueOnce({ title: "Translated batch title", body: "Translated batch body" }); + vi.spyOn(GitHubClient.prototype, "fetchThrottled") + .mockResolvedValueOnce({ + success: true, + data: { ...mockGitHubIssue(1, "Original open title"), body: "Original open body", state: "open" }, + } as Awaited>) + .mockResolvedValueOnce({ + success: true, + data: { ...mockGitHubIssue(2, "Original closed title"), body: "Original closed body", state: "closed" }, + } as Awaited>); + + const res = await REQUEST( + buildApp(), + "POST", + "/api/github/issues/batch-import", + JSON.stringify({ owner: "owner", repo: "repo", issueNumbers: [1, 2], delayMs: 1 }), + { "Content-Type": "application/json" }, + ); + + expect(res.status).toBe(200); + expect(store.getSettings).toHaveBeenCalledTimes(1); + expect(store.createTask).toHaveBeenNthCalledWith(1, expect.objectContaining({ + title: "Translated batch title", + description: "Translated batch body\n\nSource: https://github.com/owner/repo/issues/1", + })); + expect(store.createTask).toHaveBeenNthCalledWith(2, expect.objectContaining({ + title: "Original closed title", + description: "Original closed body\n\nSource: https://github.com/owner/repo/issues/2", + })); + expect(mockGetCachedImportTranslation).toHaveBeenCalledTimes(2); + expect(mockGetCachedImportTranslation).toHaveBeenNthCalledWith( + 2, + expect.anything(), + expect.objectContaining({ state: "closed" }), + ); + }); + it("marks batch imported issues as tracked when global tracking defaults are on", async () => { const globalSettingsStore = { getSettings: vi.fn().mockResolvedValue({ githubTrackingDefaultEnabledForNewTasks: true }) }; (store.getGlobalSettingsStore as ReturnType).mockReturnValueOnce(globalSettingsStore); diff --git a/packages/dashboard/src/routes/register-git-github.ts b/packages/dashboard/src/routes/register-git-github.ts index 7e07d43829..f28dad6e51 100644 --- a/packages/dashboard/src/routes/register-git-github.ts +++ b/packages/dashboard/src/routes/register-git-github.ts @@ -2101,10 +2101,14 @@ async function resolveImportedIssueTranslation( owner: string, repo: string, issue: { number: number; title: string; body: string | null; state: "open" | "closed" }, + projectSettings: Awaited>, ): Promise<{ title: string; body: string } | null> { try { - const settings = await store.getSettings(); - if (settings.githubImportAutoTranslate !== true) return null; + /* + FNXC:GitHubImportTranslate 2026-07-16-11:22: + FN-8115 passes request-scoped project settings from both import routes so translation and tracking share one project-settings read. FN-8112 first made the prior two-read test setup stable; this consolidation preserves its behavior without a second store lookup. + */ + if (projectSettings.githubImportAutoTranslate !== true) return null; const { getCachedImportTranslation, resolveTargetLocale } = await import( "../import-translate-service.js" @@ -2115,10 +2119,10 @@ async function resolveImportedIssueTranslation( Resolution therefore falls through to the global `language` setting server-side, which also fixes direct API callers and stale clients; the request locale stays as the last tier because `language` is itself unset when a surface browser-detects its locale. */ const targetLocale = resolveTargetLocale( - settings.importTranslateTargetLocale, + projectSettings.importTranslateTargetLocale, // The panel forwards its active locale; a direct API caller may not. (req.body as { targetLocale?: unknown } | undefined)?.targetLocale, - settings.language, + projectSettings.language, ); if (!targetLocale) return null; @@ -2132,8 +2136,14 @@ async function resolveImportedIssueTranslation( } } -async function resolveImportedIssueGithubTracking(store: TaskStore): Promise<{ enabled: true } | undefined> { - const projectSettings = await store.getSettings(); +async function resolveImportedIssueGithubTracking( + store: TaskStore, + projectSettings: Awaited>, +): Promise<{ enabled: true } | undefined> { + /* + FNXC:GithubImportTracking 2026-07-16-11:22: + FN-8115 shares the import request's project settings with translation and tracking, removing duplicate project-store reads after FN-8112 stabilized the prior test setup. The global settings read remains distinct because tracking precedence still requires it. + */ if (projectSettings.githubLinkImportedIssuesToTracking === true) { /* FNXC:GithubImportTracking 2026-07-01-00:00: @@ -4086,12 +4096,20 @@ export function registerGitGitHubRoutes(ctx: ApiRoutesContext): void { Cache-read only: a miss imports the original rather than blocking the import on a fresh model call, because import must stay fast and must never fail because translation failed. The `Source: ` suffix is appended AFTER translation so the URL is never rewritten by the model. */ - const translatedIssue = await resolveImportedIssueTranslation(req, scopedStore, owner, repo, issue); + const projectSettings = await scopedStore.getSettings(); + const translatedIssue = await resolveImportedIssueTranslation( + req, + scopedStore, + owner, + repo, + issue, + projectSettings, + ); const title = (translatedIssue?.title || issue.title).slice(0, 200); const body = (translatedIssue?.body ?? issue.body)?.trim() || "(no description)"; const description = `${body}\n\nSource: ${sourceUrl}`; - const importedIssueGithubTracking = await resolveImportedIssueGithubTracking(scopedStore); + const importedIssueGithubTracking = await resolveImportedIssueGithubTracking(scopedStore, projectSettings); const source = buildGitHubIssueSource(owner, repo, issue); /* FNXC:Workflows 2026-07-05-00:00: @@ -4313,7 +4331,8 @@ export function registerGitGitHubRoutes(ctx: ApiRoutesContext): void { // Get existing tasks to check for duplicates const existingTasks = await scopedStore.listTasks({ slim: false, includeArchived: false }); - const importedIssueGithubTracking = await resolveImportedIssueGithubTracking(scopedStore); + const projectSettings = await scopedStore.getSettings(); + const importedIssueGithubTracking = await resolveImportedIssueGithubTracking(scopedStore, projectSettings); // Process issues sequentially with throttling const results: Array<{ @@ -4386,12 +4405,19 @@ export function registerGitGitHubRoutes(ctx: ApiRoutesContext): void { FNXC:GitHubImportTranslate 2026-07-15-09:30: Batch import carries translations exactly like single import (shared helper) — the requirement is about imported issues, not about which import button was used. */ - const batchTranslation = await resolveImportedIssueTranslation(req, scopedStore, owner, repo, { - number: issue.number, - title: issue.title, - body: issue.body, - state: issue.state === "closed" ? "closed" : "open", - }); + const batchTranslation = await resolveImportedIssueTranslation( + req, + scopedStore, + owner, + repo, + { + number: issue.number, + title: issue.title, + body: issue.body, + state: issue.state === "closed" ? "closed" : "open", + }, + projectSettings, + ); const title = (batchTranslation?.title || issue.title).slice(0, 200); const body = (batchTranslation?.body ?? issue.body)?.trim() || "(no description)"; const description = `${body}\n\nSource: ${sourceUrl}`;