diff --git a/internal/import.go b/internal/import.go index a148fbd1f..5bc0460cf 100644 --- a/internal/import.go +++ b/internal/import.go @@ -912,6 +912,22 @@ func processImport(app core.App, site *core.Record, zipData []byte, previewOnly // one at random. seenPaths := make(map[string]string) + // Parent/slug maps for every page already in the DB, so the diff pass can + // reject a slug/name fallback match that actually lives in a different + // folder. Without this, a same-name page under another parent (e.g. + // /product/docs vs /support/docs) is reported as "modified" when it's + // really an unrelated page — importPage then creates the correct page, and + // the preview diff disagrees with what the import does. Built once here; + // the DB doesn't change during this read-only diff pass. + existingParentMap := make(map[string]string) + existingSlugMap := make(map[string]string) + if existingPages, err := app.FindRecordsByFilter("pages", "site = {:site}", "", 0, 0, dbx.Params{"site": siteId}); err == nil { + for _, p := range existingPages { + existingParentMap[p.Id] = p.GetString("parent") + existingSlugMap[p.Id] = p.GetString("slug") + } + } + // Iterate in sorted key order so duplicate-route errors (and any other // per-file behavior) are deterministic across runs. sortedFilePaths := make([]string, 0, len(files)) @@ -977,6 +993,23 @@ func processImport(app core.App, site *core.Record, zipData []byte, previewOnly existing, _ = app.FindFirstRecordByFilter("pages", "site = {:site} && name = {:name}", dbx.Params{"site": siteId, "name": pageData.Name}) } + // A slug/name fallback match may point at a page in a different folder + // that happens to share a leaf slug or display name. importPage rejects + // that as a cross-folder false positive and creates a fresh record; + // mirror that here so the diff reports this file as Added, not Modified. + // + // Only do this when the file asserts a specific identity via a valid + // exported _id that the matched record doesn't have — that's the signal + // these are two DISTINCT pages. A file WITHOUT an _id matched by name at + // a different path is a legitimate move/rename of one page (there's no + // competing identity), and must still be reported as Modified. + if existing != nil && pbRecordIdPattern.MatchString(pageData.ID) && existing.Id != pageData.ID { + existingPath := buildFullPagePath(existing.Id, existingParentMap, existingSlugMap) + if existingPath != pagePath { + existing = nil + } + } + // Slug/parent are derived from the file path, not from raw_source, so a // moved or renamed page can have identical bytes. Mirror importPage's // skip condition here so the diff label stays truthful: same bytes AND @@ -1923,6 +1956,37 @@ func importPage(app core.App, site *core.Record, pageData ExportedPage, raw []by slug = pagePath[lastSlash+1:] } + // `existing` was matched in an earlier pass by id, then slug, then name — + // and the slug/name filters are NOT scoped by parent. So a page in a + // different folder that happens to share a leaf slug or a display name + // (e.g. /product/docs vs /support/docs, both named "Docs") can be matched + // as the "existing" record for THIS file. Reusing it would re-parent and + // overwrite the wrong page, silently merging two distinct pages into one. + // + // Drop the match only when this file asserts a specific identity via a valid + // exported _id that the matched record doesn't have, AND the record lives + // under a different parent — that's the signal these are two DISTINCT pages. + // A file WITHOUT an _id matched by name under a different parent is a + // legitimate move/rename of one page (no competing identity), so it must + // keep matching and re-parent rather than fork into a duplicate. + if existing != nil && pbRecordIdPattern.MatchString(pageData.ID) && + existing.Id != pageData.ID && existing.GetString("parent") != parentId { + existing = nil + } + + // A genuine match (same parent) whose record id has drifted from a valid + // exported _id still orphans every cross-file `page:` link that references + // that _id — the link points at an id no page has. The importer already + // rebuilds a page's sections/entries from the file on every import, so the + // record carries no state worth preserving: delete it and let the create + // branch recreate it under the exported _id. Children rebuild as usual. + if existing != nil && pbRecordIdPattern.MatchString(pageData.ID) && existing.Id != pageData.ID { + if err := app.Delete(existing); err != nil { + return "", nil, fmt.Errorf("re-key page %q to exported _id %s: %w", pagePath, pageData.ID, err) + } + existing = nil + } + // No-op guard: if the incoming page bytes are byte-identical to what we // stored on the last import AND the page's file-derived location is // unchanged (same parent + slug), the page is untouched since the dev's diff --git a/internal/import_test.go b/internal/import_test.go index 974236f1a..362910799 100644 --- a/internal/import_test.go +++ b/internal/import_test.go @@ -1544,3 +1544,386 @@ func TestDuplicatePageRouteFailsImport(t *testing.T) { before.GetString("updated"), before.GetString("slug"), after.GetString("updated"), after.GetString("slug")) } } + +// TestReimportPageWithIDAfterIDlessImport reproduces the real primo.build case: +// a page first imported WITHOUT an _id gets a random record id; a later import +// of the same page (same slug) WITH an _id should adopt the exported _id so +// cross-file page: links resolve. If the importer keeps matching by slug and +// preserves the old random id, every link referencing the exported _id dangles. +func TestReimportPageWithIDAfterIDlessImport(t *testing.T) { + app := newImportTestApp(t) + defer app.ResetBootstrapState() + site := createImportTestSite(t, app) + + // First import: pricing page with NO _id -> random record id. + first := baseSiteFiles() + first["pages/pricing.yaml"] = "" + + "name: Pricing\n" + + "page_type: Default\n" + + "sections:\n - block: hero\n content:\n heading: Pricing\n" + if _, err := processImport(app, site, zipFiles(t, first), false); err != nil { + t.Fatalf("first import: %v", err) + } + randomID := findPageByName(t, app, site, "Pricing").Id + t.Logf("first import gave pricing random id=%q", randomID) + + // Second import: same page, now WITH an explicit _id. + const wantID = "pricingpage0001" + second := baseSiteFiles() + second["pages/pricing.yaml"] = "" + + "_id: " + wantID + "\n" + + "name: Pricing\n" + + "page_type: Default\n" + + "sections:\n - block: hero\n content:\n heading: Pricing\n" + if _, err := processImport(app, site, zipFiles(t, second), false); err != nil { + t.Fatalf("second import: %v", err) + } + + got := findPageByName(t, app, site, "Pricing").Id + if got != wantID { + t.Fatalf("re-import did not adopt exported _id: want %q, got %q (old random id=%q) -- links referencing %q would dangle", + wantID, got, randomID, wantID) + } +} + +// TestSameLeafSlugDifferentFolders probes whether /team/about and /careers/about +// (same leaf slug "about", different parents, different _ids) collide during +// import — i.e. does the unscoped slug/name lookup match the wrong page? +func TestSameLeafSlugDifferentFolders(t *testing.T) { + app := newImportTestApp(t) + defer app.ResetBootstrapState() + site := createImportTestSite(t, app) + + f := baseSiteFiles() + // parent pages so children aren't orphaned + f["pages/team/index.yaml"] = "_id: teamindexpage01\nname: Team\npage_type: Default\nsections: []\n" + f["pages/careers/index.yaml"] = "_id: careerindexpg01\nname: Careers\npage_type: Default\nsections: []\n" + f["pages/team/about.yaml"] = "_id: teamaboutpage01\nname: Team About\npage_type: Default\nsections: []\n" + f["pages/careers/about.yaml"] = "_id: careraboutpg001\nname: Careers About\npage_type: Default\nsections: []\n" + + if _, err := processImport(app, site, zipFiles(t, f), false); err != nil { + t.Fatalf("import: %v", err) + } + + // Expect TWO distinct about pages with their exported ids preserved. + teamAbout, err1 := app.FindRecordById("pages", "teamaboutpage01") + careerAbout, err2 := app.FindRecordById("pages", "careraboutpg001") + all, _ := app.FindRecordsByFilter("pages", "site = {:site} && slug = 'about'", "", 0, 0, map[string]any{"site": site.Id}) + t.Logf("pages with slug 'about': %d", len(all)) + for _, p := range all { + t.Logf(" id=%s name=%q parent=%s", p.Id, p.GetString("name"), p.GetString("parent")) + } + if err1 != nil { + t.Errorf("team/about lost its _id teamaboutpage01: %v", err1) + } + if err2 != nil { + t.Errorf("careers/about lost its _id careraboutpg001: %v", err2) + } + if len(all) != 2 { + t.Fatalf("expected 2 distinct 'about' pages, got %d (collision merged them)", len(all)) + } + _ = teamAbout + _ = careerAbout +} + +// TestNameFallbackCrossesFolders isolates the real trigger: when a page's _id +// does NOT match any existing record (so id-lookup misses) and two pages share +// the same NAME, the unscoped name fallback can bind the import to the wrong +// existing page. Simulated by pre-seeding a page whose id differs from the file. +func TestNameFallbackCrossesFolders(t *testing.T) { + app := newImportTestApp(t) + defer app.ResetBootstrapState() + site := createImportTestSite(t, app) + + // First import: a "Docs" page under /product with a known id. + f1 := baseSiteFiles() + f1["pages/product/index.yaml"] = "_id: productindex001\nname: Product\npage_type: Default\nsections: []\n" + f1["pages/product/docs.yaml"] = "_id: productdocs0001\nname: Docs\npage_type: Default\nsections: []\n" + if _, err := processImport(app, site, zipFiles(t, f1), false); err != nil { + t.Fatalf("first import: %v", err) + } + + // Second import ADDS a different "Docs" page under /support, with a NEW _id. + // Same name "Docs", different folder, id-lookup will miss -> name fallback. + f2 := baseSiteFiles() + f2["pages/product/index.yaml"] = "_id: productindex001\nname: Product\npage_type: Default\nsections: []\n" + f2["pages/product/docs.yaml"] = "_id: productdocs0001\nname: Docs\npage_type: Default\nsections: []\n" + f2["pages/support/index.yaml"] = "_id: supportindex001\nname: Support\npage_type: Default\nsections: []\n" + f2["pages/support/docs.yaml"] = "_id: supportdocs0001\nname: Docs\npage_type: Default\nsections: []\n" + if _, err := processImport(app, site, zipFiles(t, f2), false); err != nil { + t.Fatalf("second import: %v", err) + } + + all, _ := app.FindRecordsByFilter("pages", "site = {:site} && name = 'Docs'", "", 0, 0, map[string]any{"site": site.Id}) + t.Logf("pages named 'Docs': %d", len(all)) + for _, p := range all { + t.Logf(" id=%s parent=%s slug=%s", p.Id, p.GetString("parent"), p.GetString("slug")) + } + _, e1 := app.FindRecordById("pages", "productdocs0001") + _, e2 := app.FindRecordById("pages", "supportdocs0001") + if e1 != nil { + t.Errorf("product/docs lost its _id: %v", e1) + } + if e2 != nil { + t.Errorf("support/docs never got its _id supportdocs0001 (name fallback bound it to product/docs): %v", e2) + } + if len(all) != 2 { + t.Fatalf("expected 2 distinct 'Docs' pages, got %d", len(all)) + } +} + +// TestMovePageKeepsIdentity guards that a legitimate move (same _id, new folder) +// is NOT treated as a cross-folder false match by the new parent-scope guard. +// The page should keep its _id and simply re-parent, not get deleted/recreated. +func TestMovePageKeepsIdentity(t *testing.T) { + app := newImportTestApp(t) + defer app.ResetBootstrapState() + site := createImportTestSite(t, app) + + // Import /about with a fixed id. + f1 := baseSiteFiles() + f1["pages/about.yaml"] = "_id: aboutpage000001\nname: About\npage_type: Default\nsections: []\n" + if _, err := processImport(app, site, zipFiles(t, f1), false); err != nil { + t.Fatalf("first import: %v", err) + } + + // Move it to /company/about (same _id), add the parent folder. + f2 := baseSiteFiles() + f2["pages/company/index.yaml"] = "_id: companyindex001\nname: Company\npage_type: Default\nsections: []\n" + f2["pages/company/about.yaml"] = "_id: aboutpage000001\nname: About\npage_type: Default\nsections: []\n" + if _, err := processImport(app, site, zipFiles(t, f2), false); err != nil { + t.Fatalf("second import: %v", err) + } + + moved, err := app.FindRecordById("pages", "aboutpage000001") + if err != nil { + t.Fatalf("moved page lost its _id: %v", err) + } + if moved.GetString("parent") != "companyindex001" { + t.Fatalf("move did not re-parent: parent=%q want companyindex001", moved.GetString("parent")) + } + // Only one About page should exist (moved, not duplicated). + all, _ := app.FindRecordsByFilter("pages", "site = {:site} && name = 'About'", "", 0, 0, map[string]any{"site": site.Id}) + if len(all) != 1 { + t.Fatalf("expected 1 About page after move, got %d", len(all)) + } +} + +// TestFreshImportPreservesIdWithHomepageParent mirrors the real site: a root +// page gets the homepage as its parent (see importPages), and page_type is +// lowercase "default". Reproduces why the marketing-site pricing page didn't +// keep its _id on a fresh import. +func TestFreshImportPreservesIdWithHomepageParent(t *testing.T) { + app := newImportTestApp(t) + defer app.ResetBootstrapState() + site := createImportTestSite(t, app) + + f := baseSiteFiles() + // Home has an explicit _id, like a real exported site. + f["pages/index.yaml"] = "_id: homepage0000001\nname: Home\npage_type: Default\nsections: []\n" + f["pages/pricing.yaml"] = "_id: 87zp1xcvpevbkd6\nname: Pricing\npage_type: Default\nsections: []\n" + + if _, err := processImport(app, site, zipFiles(t, f), false); err != nil { + t.Fatalf("import: %v", err) + } + + got := findPageByName(t, app, site, "Pricing").Id + if got != "87zp1xcvpevbkd6" { + t.Fatalf("fresh import did not preserve pricing _id: got %q want 87zp1xcvpevbkd6", got) + } + if _, err := app.FindRecordById("pages", "homepage0000001"); err != nil { + t.Fatalf("homepage lost its _id: %v", err) + } +} + +// TestSectionLinkResolvesAfterPageRekey addresses CodeRabbit's concern that a +// re-key could leave a block link pointing at a deleted page id. A landing +// page's section links to a target page by the target's exported _id. The +// target is first imported idless (random id), then re-imported WITH its _id +// (triggering the re-key). After import, the section link's page ref must point +// at a live page — specifically the target's exported _id. +func TestSectionLinkResolvesAfterPageRekey(t *testing.T) { + app := newImportTestApp(t) + defer app.ResetBootstrapState() + site := createImportTestSite(t, app) + + const targetID = "targetpage00001" + + // A block whose single field is a link, so a section can store a page ref. + linkBlock := map[string]string{ + "blocks/linkcard/config.yaml": "name: linkcard\n", + "blocks/linkcard/component.svelte": "{cta.label}\n", + "blocks/linkcard/fields.yaml": "- name: cta\n label: CTA\n type: link\n", + "blocks/linkcard/content.yaml": "{}\n", + } + + base := func(withTargetID bool) map[string]string { + f := baseSiteFiles() + for k, v := range linkBlock { + f[k] = v + } + f["page-types/default/config.yaml"] = "name: Default\nallowed_blocks:\n - hero\n - linkcard\n" + idLine := "" + if withTargetID { + idLine = "_id: " + targetID + "\n" + } + f["pages/target.yaml"] = idLine + "name: Target\npage_type: Default\nsections: []\n" + // Landing page's section links to the target by its exported _id. + f["pages/landing.yaml"] = "" + + "_id: landingpage0001\nname: Landing\npage_type: Default\n" + + "sections:\n" + + " - block: linkcard\n" + + " content:\n" + + " cta:\n" + + " label: See pricing\n" + + " page: " + targetID + "\n" + return f + } + + // First import: target has NO _id -> gets a random id; landing's link + // already references the future exported _id (targetID). + if _, err := processImport(app, site, zipFiles(t, base(false)), false); err != nil { + t.Fatalf("first import: %v", err) + } + + // Second import: target now carries its _id -> re-key to targetID. + if _, err := processImport(app, site, zipFiles(t, base(true)), false); err != nil { + t.Fatalf("second import: %v", err) + } + + // The target must now exist under its exported _id. + if _, err := app.FindRecordById("pages", targetID); err != nil { + t.Fatalf("target page not present under exported _id %q: %v", targetID, err) + } + + // The landing section's cta.page ref must point at a LIVE page (targetID). + landing := findPageByName(t, app, site, "Landing") + sections, _ := app.FindRecordsByFilter("page_sections", "page = {:page}", "", 0, 0, map[string]any{"page": landing.Id}) + var refFound, refResolves bool + for _, s := range sections { + entries, _ := app.FindRecordsByFilter("page_section_entries", "section = {:section}", "", 0, 0, map[string]any{"section": s.Id}) + for _, e := range entries { + val := e.GetString("value") + if strings.Contains(val, "\"page\"") && strings.Contains(val, targetID) { + refFound = true + if _, err := app.FindRecordById("pages", targetID); err == nil { + refResolves = true + } + } + } + } + if !refFound { + t.Fatalf("landing section link ref to %q not found in stored entries", targetID) + } + if !refResolves { + t.Fatalf("landing section link points at %q but no such page exists (dangling after re-key)", targetID) + } +} + +// TestIdlessMoveToNewParentKeepsOnePage guards that Part 1's parent-scope guard +// in importPage does not duplicate an idless page that moves under a new parent. +// Without an _id the page is matched by name; a naive "different parent -> drop" +// would create a second page. It must remain a single (moved) page. +func TestIdlessMoveToNewParentKeepsOnePage(t *testing.T) { + app := newImportTestApp(t) + defer app.ResetBootstrapState() + site := createImportTestSite(t, app) + + body := "name: About\npage_type: Default\nsections: []\n" + + first := baseSiteFiles() + first["pages/about.yaml"] = body + if _, err := processImport(app, site, zipFiles(t, first), false); err != nil { + t.Fatalf("first import: %v", err) + } + before := findPageByName(t, app, site, "About") + + // Move under a new parent folder /company (no _id; matched by name). + second := baseSiteFiles() + delete(second, "pages/about.yaml") + second["pages/company/index.yaml"] = "name: Company\npage_type: Default\nsections: []\n" + second["pages/company/about.yaml"] = body + if _, err := processImport(app, site, zipFiles(t, second), false); err != nil { + t.Fatalf("second import: %v", err) + } + + all, _ := app.FindRecordsByFilter("pages", "site = {:site} && name = 'About'", "", 0, 0, map[string]any{"site": site.Id}) + if len(all) != 1 { + t.Fatalf("idless move should keep ONE About page, got %d", len(all)) + } + // The remaining record must be the SAME one, re-parented — not a fresh + // record that merely happens to net one page. + moved := all[0] + if moved.Id != before.Id { + t.Fatalf("moved page identity changed: before=%s after=%s (delete+recreate, not a move)", before.Id, moved.Id) + } + company := findPageByName(t, app, site, "Company") + if moved.GetString("parent") != company.Id { + t.Fatalf("moved page not re-parented: parent=%q want %q (Company)", moved.GetString("parent"), company.Id) + } +} + +// TestSiteContentUrlLinkResolvesAfterRekey covers CodeRabbit's exact concern: +// a site-content link authored as `url: /pricing` is converted to a page ref +// during import. If that conversion ran against pre-rekey page ids, the stored +// ref would dangle after the pricing page is re-keyed to its exported _id. The +// stored site_entries value must carry the FINAL exported id and point at a +// live page. +func TestSiteContentUrlLinkResolvesAfterRekey(t *testing.T) { + app := newImportTestApp(t) + defer app.ResetBootstrapState() + site := createImportTestSite(t, app) + + const pricingID = "pricingpage0001" + + build := func(withPricingID bool) map[string]string { + f := baseSiteFiles() + f["site/fields.yaml"] = "- name: home_cta\n label: Home CTA\n type: link\n" + // Authored as a URL link; the importer converts it to a page ref. + f["site/content.yaml"] = "home_cta:\n label: See pricing\n url: /pricing\n" + idLine := "" + if withPricingID { + idLine = "_id: " + pricingID + "\n" + } + f["pages/pricing.yaml"] = idLine + "name: Pricing\npage_type: Default\nsections: []\n" + return f + } + + // First import: pricing has no _id -> random id; url:/pricing converts to it. + if _, err := processImport(app, site, zipFiles(t, build(false)), false); err != nil { + t.Fatalf("first import: %v", err) + } + // Second import: pricing now carries its _id -> re-key. + if _, err := processImport(app, site, zipFiles(t, build(true)), false); err != nil { + t.Fatalf("second import: %v", err) + } + + // The pricing page exists under its exported _id. + if _, err := app.FindRecordById("pages", pricingID); err != nil { + t.Fatalf("pricing not present under exported _id: %v", err) + } + + // The stored site_entries link value must reference the FINAL id (pricingID), + // not a stale pre-rekey id, and that id must resolve to a live page. + entries, err := app.FindRecordsByFilter( + "site_entries", + "field.site = {:site}", + "", 0, 0, map[string]any{"site": site.Id}, + ) + if err != nil { + t.Fatalf("load site entries: %v", err) + } + var found bool + for _, e := range entries { + val := e.GetString("value") + if strings.Contains(val, "\"page\"") { + found = true + if !strings.Contains(val, pricingID) { + t.Fatalf("site link ref does not point at final id %q; value=%s", pricingID, val) + } + } + } + if !found { + t.Fatalf("no page ref stored for the site url link (conversion did not run); entries=%d", len(entries)) + } +}