fix(import-export): preserve list hierarchy on karakeep round-trip

[mgrusso]

When importing a karakeep JSON export, top-level lists (those with no parent in the source file) were placed under the import-session root list instead of being created at the top level of the user's list space. This meant that a round-trip export → import produced a different hierarchy than the original, making it impossible to restore state on another instance.

Add a restoreTopLevelLists option to importBookmarksFromFile that, when enabled, creates exported top-level lists without a parent so the full original hierarchy is faithfully reproduced. The import-session root list is still created and serves as the fallback container for any bookmarks that carry no list assignment.

Enable the option automatically for the "karakeep" source in the web hook so that every karakeep-format import is a true state restoration.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3be3d476-f4de-4148-b9aa-fc51caceb45c

📥 Commits

Reviewing files that changed from the base of the PR and between fba7108 and b433d6d.

📒 Files selected for processing (3)

• apps/web/lib/hooks/useBookmarkImport.ts
• packages/shared/import-export/importer.test.ts
• packages/shared/import-export/importer.ts

---

Walkthrough

This PR adds support for restoring imported list hierarchy at the top level when importing from Karakeep. It introduces a new restoreTopLevelLists option that allows top-level lists in the export to remain top-level in the import, rather than being nested under an import session root.

Changes

Cohort / File(s)	Summary
Hook Configuration apps/web/lib/hooks/useBookmarkImport.ts	Added restoreTopLevelLists: source === "karakeep" option to importBookmarksFromFile invocation to enable hierarchy restoration for Karakeep imports.
Core Importer Logic packages/shared/import-export/importer.ts	Extended ImportOptions interface with optional restoreTopLevelLists boolean flag. Updated list-parent assignment logic: top-level lists (no parent in export) now use parentId: undefined when flag is true; unresolved lists behave conditionally based on flag value.
Import Tests packages/shared/import-export/importer.test.ts	Added comprehensive test case for restoreTopLevelLists: true scenario, verifying that root-level lists are created without parentId, child lists maintain proper hierarchy, and bookmarks are correctly assigned to their restored lists.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• fix: check import quota before importing bookmarks #2232: Modifies the bookmark import flow and updates how importBookmarksFromFile is invoked with additional options.
• feat: export lists in backups and exports #2484: Implements import/export list handling logic that directly interacts with the list-creation and hierarchy restoration behavior introduced here.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'fix(import-export): preserve list hierarchy on karakeep round-trip' directly summarizes the main change: preserving list hierarchy during karakeep import round-trips by restoring top-level lists.
Description check	✅ Passed	The description clearly explains the problem (top-level lists placed incorrectly), the solution (restoreTopLevelLists option), and implementation details (automatic enablement for karakeep source), all directly related to the changeset.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

📝 Coding Plan

• Generate coding plan for human review comments

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Tip

CodeRabbit can suggest fixes for GitHub Check annotations.

Configure the reviews.tools.github-checks setting to adjust the time to wait for GitHub Checks to complete.

[greptile-apps]

Greptile Summary

This PR fixes a long-standing round-trip fidelity bug: top-level lists in a karakeep export were previously always nested under the import-session root list on re-import, making a faithful state restoration impossible. The fix adds a restoreTopLevelLists option to importBookmarksFromFile and enables it automatically for "karakeep" imports via the web hook.

Key changes:

• importer.ts: New restoreTopLevelLists?: boolean option; when true, lists with parentExternalId === null are created without a parentId (global top level) instead of being nested under rootList.id. The import-session root list is still created and serves as the fallback container for unassigned bookmarks.
• useBookmarkImport.ts: Passes restoreTopLevelLists: source === "karakeep" transparently alongside the existing custom-parser optimisation.
• importer.test.ts: A new test validates the full hierarchy (import root still created, top-level lists get no parent, child lists get the correct new parent, assigned/unassigned bookmarks land in the right places).

Issues found:

• In the cycle-breaking fallback (!createdAny branch), all unresolved lists are placed at the top level when restoreTopLevelLists is true, even those that had a non-null parentExternalId. These lists were never originally top-level, so promoting them can unexpectedly pollute the user's global list space. The regular resolution path correctly gates this behaviour on parentExternalId === null, and the fallback should do the same.
• The pre-existing "creates smart lists" test uses source: "karakeep" without restoreTopLevelLists: true, so it asserts behaviour that no longer matches what actually happens in production for karakeep imports.

Confidence Score: 4/5

• Safe to merge with a minor logic fix recommended in the cycle-breaking fallback.
• The primary code path (lists with no original parent) is correctly implemented and well-tested by the new test case. The only concern is the edge-case cycle-breaking fallback which, under restoreTopLevelLists: true, can incorrectly promote orphaned/cycled lists to the global top level rather than containing them under the import root. This is unlikely to be triggered by valid karakeep exports but worth tightening before merge.
• packages/shared/import-export/importer.ts — specifically the !createdAny cycle-breaking fallback block at lines 142–156.

Important Files Changed

Filename	Overview
packages/shared/import-export/importer.ts	Adds restoreTopLevelLists option that, when true, creates originally-top-level lists without a parent; the cycle-breaking fallback also promotes unresolvable lists to the top level, which is a subtle edge-case inconsistency.
packages/shared/import-export/importer.test.ts	New test case correctly covers the restoreTopLevelLists behavior; the existing "creates smart lists" test uses source: "karakeep" without restoreTopLevelLists: true, so it doesn't reflect the actual production path where the hook always enables the option.
apps/web/lib/hooks/useBookmarkImport.ts	Cleanly wires restoreTopLevelLists: source === "karakeep" into the options object; no issues found.

Comments Outside Diff (2)

1. packages/shared/import-export/importer.ts, line 142-156 (link)

   Cycle-break fallback promotes non-top-level lists to root

   When restoreTopLevelLists is true, the cycle-breaking path places all remaining unresolved lists at the top level — but these lists, by definition, had a non-null parentExternalId that couldn't be resolved (cycle or dangling reference). They were never originally at the top level, so promoting them to the global list space is inconsistent with the "faithful state restoration" goal of the option.

   The regular loop (lines 120–124) correctly gates the restoreTopLevelLists logic on list.parentExternalId being null. The same guard should apply here: only truly-top-level lists (no parentExternalId) should be exempt from being placed under rootList.id.

   This ensures that only genuinely parentless lists are left at the top level during a restore; orphaned/cycled lists are still safely tucked under the import root.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: packages/shared/import-export/importer.ts
   Line: 142-156
   
   Comment:
   **Cycle-break fallback promotes non-top-level lists to root**
   
   When `restoreTopLevelLists` is `true`, the cycle-breaking path places **all** remaining unresolved lists at the top level — but these lists, by definition, had a non-`null` `parentExternalId` that couldn't be resolved (cycle or dangling reference). They were never originally at the top level, so promoting them to the global list space is inconsistent with the "faithful state restoration" goal of the option.
   
   The regular loop (lines 120–124) correctly gates the `restoreTopLevelLists` logic on `list.parentExternalId` being `null`. The same guard should apply here: only truly-top-level lists (no `parentExternalId`) should be exempt from being placed under `rootList.id`.
   
   
   
   This ensures that only genuinely parentless lists are left at the top level during a restore; orphaned/cycled lists are still safely tucked under the import root.
   
   How can I resolve this? If you propose a fix, please make it concise.

2. packages/shared/import-export/importer.test.ts, line 549-578 (link)

   Existing karakeep test doesn't reflect production behavior

   This test uses source: "karakeep" but omits restoreTopLevelLists: true, so it asserts that the smart and manual lists are nested under "Imported" (parentId: "Imported"). In production, however, the hook always passes restoreTopLevelLists: true for the "karakeep" source, which means those same lists would actually be created without a parent.

   The test still has value as a unit test for the default option value, but it might be worth either:

   • Adding a companion test (or updating this one) that passes restoreTopLevelLists: true and asserts parentId is undefined for those lists, or
   • Adding a comment noting that this path is explicitly testing the non-restore behaviour.

   Without this, a future reader might be surprised when the real karakeep import produces a hierarchy that none of the smart-list tests cover.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: packages/shared/import-export/importer.test.ts
   Line: 549-578
   
   Comment:
   **Existing karakeep test doesn't reflect production behavior**
   
   This test uses `source: "karakeep"` but omits `restoreTopLevelLists: true`, so it asserts that the smart and manual lists are nested under `"Imported"` (`parentId: "Imported"`). In production, however, the hook always passes `restoreTopLevelLists: true` for the `"karakeep"` source, which means those same lists would actually be created **without** a parent.
   
   The test still has value as a unit test for the default option value, but it might be worth either:
   - Adding a companion test (or updating this one) that passes `restoreTopLevelLists: true` and asserts `parentId` is `undefined` for those lists, or  
   - Adding a comment noting that this path is explicitly testing the non-restore behaviour.
   
   Without this, a future reader might be surprised when the real karakeep import produces a hierarchy that none of the smart-list tests cover.
   
   How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: packages/shared/import-export/importer.ts
Line: 142-156

Comment:
**Cycle-break fallback promotes non-top-level lists to root**

When `restoreTopLevelLists` is `true`, the cycle-breaking path places **all** remaining unresolved lists at the top level — but these lists, by definition, had a non-`null` `parentExternalId` that couldn't be resolved (cycle or dangling reference). They were never originally at the top level, so promoting them to the global list space is inconsistent with the "faithful state restoration" goal of the option.

The regular loop (lines 120–124) correctly gates the `restoreTopLevelLists` logic on `list.parentExternalId` being `null`. The same guard should apply here: only truly-top-level lists (no `parentExternalId`) should be exempt from being placed under `rootList.id`.

```suggestion
      if (!createdAny) {
        for (const [externalId, list] of unresolvedLists) {
          const createdList = await deps.createList({
            name: list.name.substring(0, MAX_LIST_NAME_LENGTH),
            ...(restoreTopLevelLists && !list.parentExternalId
              ? {}
              : { parentId: rootList.id }),
            icon: list.icon ?? "📁",
            description: list.description,
            ...(list.type === "smart" && list.query
```

This ensures that only genuinely parentless lists are left at the top level during a restore; orphaned/cycled lists are still safely tucked under the import root.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: packages/shared/import-export/importer.test.ts
Line: 549-578

Comment:
**Existing karakeep test doesn't reflect production behavior**

This test uses `source: "karakeep"` but omits `restoreTopLevelLists: true`, so it asserts that the smart and manual lists are nested under `"Imported"` (`parentId: "Imported"`). In production, however, the hook always passes `restoreTopLevelLists: true` for the `"karakeep"` source, which means those same lists would actually be created **without** a parent.

The test still has value as a unit test for the default option value, but it might be worth either:
- Adding a companion test (or updating this one) that passes `restoreTopLevelLists: true` and asserts `parentId` is `undefined` for those lists, or  
- Adding a comment noting that this path is explicitly testing the non-restore behaviour.

Without this, a future reader might be surprised when the real karakeep import produces a hierarchy that none of the smart-list tests cover.

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: "fix(import-export): ..."}