firefox: fix bookmarks backwards compatibility

The legacy attrset option type for `firefox.profiles.<name>.bookmarks` was accidentally removed in 9d55428. This adds back support for this type by refactoring the bookmarks submodule. This also adds a new test ensuring this won't happen again.
2026-01-12 01:59:37 +08:00 · 2025-03-20 18:08:40 +01:00
parent 5ff90f09d1
commit 62d6a8931e
7 changed files with 167 additions and 70 deletions
--- a/modules/programs/firefox/mkFirefoxModule.nix
+++ b/modules/programs/firefox/mkFirefoxModule.nix
@@ -158,6 +158,8 @@ let
      })
    else
      (pkgs.wrapFirefox.override { config = bcfg; }) package { };
+
+  bookmarkTypes = import ./profiles/bookmark-types.nix { inherit lib; };
 in {
  options = setAttrByPath modulePath {
    enable = mkOption {
@@ -380,7 +382,7 @@ in {

          bookmarks = mkOption {
            type = (with types;
-              coercedTo (listOf anything) (bookmarks:
+              coercedTo bookmarkTypes.settingsType (bookmarks:
                warn ''
                  ${cfg.name} bookmarks have been refactored into a submodule that now explicitly require a 'force' option to be enabled.

--- a/modules/programs/firefox/profiles/bookmark-types.nix
+++ b/modules/programs/firefox/profiles/bookmark-types.nix
@@ -0,0 +1,70 @@
+{ lib, ... }:
+
+with lib;
+
+rec {
+  settingsType = with types;
+    coercedTo (addCheck (attrsOf nodeType) (attrs: !(attrs ? settings)))
+    attrValues (listOf nodeType);
+
+  bookmarkSubmodule = types.submodule ({ name, ... }: {
+    options = {
+      name = mkOption {
+        type = types.str;
+        default = name;
+        description = "Bookmark name.";
+      };
+
+      tags = mkOption {
+        type = types.listOf types.str;
+        default = [ ];
+        description = "Bookmark tags.";
+      };
+
+      keyword = mkOption {
+        type = types.nullOr types.str;
+        default = null;
+        description = "Bookmark search keyword.";
+      };
+
+      url = mkOption {
+        type = types.str;
+        description = "Bookmark url, use %s for search terms.";
+      };
+    };
+  }) // {
+    description = "bookmark submodule";
+  };
+
+  bookmarkType = types.addCheck bookmarkSubmodule (x: x ? "url");
+
+  directoryType = types.submodule ({ name, ... }: {
+    options = {
+      name = mkOption {
+        type = types.str;
+        default = name;
+        description = "Directory name.";
+      };
+
+      bookmarks = mkOption {
+        type = types.listOf nodeType;
+        default = [ ];
+        description = "Bookmarks within directory.";
+      };
+
+      toolbar = mkOption {
+        type = types.bool;
+        default = false;
+        description = ''
+          Make this the toolbar directory. Note, this does _not_
+          mean that this directory will be added to the toolbar,
+          this directory _is_ the toolbar.
+        '';
+      };
+    };
+  }) // {
+    description = "directory submodule";
+  };
+
+  nodeType = types.either bookmarkType directoryType;
+}
--- a/modules/programs/firefox/profiles/bookmarks.nix
+++ b/modules/programs/firefox/profiles/bookmarks.nix
@@ -3,66 +3,9 @@
 with lib;

 let
-  bookmarkSubmodule = types.submodule ({ name, ... }: {
-    options = {
-      name = mkOption {
-        type = types.str;
-        default = name;
-        description = "Bookmark name.";
-      };
+  bookmarkTypes = import ./bookmark-types.nix { inherit lib; };

-      tags = mkOption {
-        type = types.listOf types.str;
-        default = [ ];
-        description = "Bookmark tags.";
-      };
-
-      keyword = mkOption {
-        type = types.nullOr types.str;
-        default = null;
-        description = "Bookmark search keyword.";
-      };
-
-      url = mkOption {
-        type = types.str;
-        description = "Bookmark url, use %s for search terms.";
-      };
-    };
-  }) // {
-    description = "bookmark submodule";
-  };
-
-  bookmarkType = types.addCheck bookmarkSubmodule (x: x ? "url");
-
-  directoryType = types.submodule ({ name, ... }: {
-    options = {
-      name = mkOption {
-        type = types.str;
-        default = name;
-        description = "Directory name.";
-      };
-
-      bookmarks = mkOption {
-        type = types.listOf nodeType;
-        default = [ ];
-        description = "Bookmarks within directory.";
-      };
-
-      toolbar = mkOption {
-        type = types.bool;
-        default = false;
-        description = ''
-          Make this the toolbar directory. Note, this does _not_
-          mean that this directory will be added to the toolbar,
-          this directory _is_ the toolbar.
-        '';
-      };
-    };
-  }) // {
-    description = "directory submodule";
-  };
-
-  nodeType = types.either bookmarkType directoryType;
+  inherit (bookmarkTypes) settingsType;

  bookmarksFile = bookmarks:
    let
@@ -141,8 +84,7 @@ in {
    };

    settings = mkOption {
-      type = with types;
-        coercedTo (attrsOf nodeType) attrValues (listOf nodeType);
+      type = settingsType;
      default = [ ];
      example = literalExpression ''
        [
--- a/tests/modules/programs/firefox/common.nix
+++ b/tests/modules/programs/firefox/common.nix
@@ -5,6 +5,7 @@ builtins.mapAttrs (test: module: import module [ "programs" name ]) {
  "${name}-final-package" = ./final-package.nix;
  "${name}-policies" = ./policies.nix;
  "${name}-profiles-bookmarks" = ./profiles/bookmarks;
+  "${name}-profiles-bookmarks-attrset" = ./profiles/bookmarks/attrset.nix;
  "${name}-profiles-containers" = ./profiles/containers;
  "${name}-profiles-containers-duplicate-ids" =
    ./profiles/containers/duplicate-ids.nix;
--- a/tests/modules/programs/firefox/profiles/bookmarks/attrset.nix
+++ b/tests/modules/programs/firefox/profiles/bookmarks/attrset.nix
@@ -0,0 +1,82 @@
+modulePath:
+{ config, lib, ... }:
+
+let
+
+  cfg = lib.getAttrFromPath modulePath config;
+
+  firefoxMockOverlay = import ../../setup-firefox-mock-overlay.nix modulePath;
+
+in {
+  imports = [ firefoxMockOverlay ];
+
+  config = lib.mkIf config.test.enableBig (lib.setAttrByPath modulePath {
+    enable = true;
+    profiles.bookmarks = {
+      settings = { "general.smoothScroll" = false; };
+      bookmarks = {
+        home-manager = {
+          toolbar = true;
+          bookmarks = [{
+            name = "Home Manager";
+            url = "https://wiki.nixos.org/wiki/Home_Manager";
+          }];
+        };
+        wikipedia = {
+          name = "wikipedia";
+          tags = [ "wiki" ];
+          keyword = "wiki";
+          url = "https://en.wikipedia.org/wiki/Special:Search?search=%s&go=Go";
+        };
+        kernel-org = {
+          name = "kernel.org";
+          url = "https://www.kernel.org";
+        };
+        nix-sites = {
+          name = "Nix sites";
+          bookmarks = [
+            {
+              name = "homepage";
+              url = "https://nixos.org/";
+            }
+            {
+              name = "wiki";
+              tags = [ "wiki" "nix" ];
+              url = "https://wiki.nixos.org/";
+            }
+            {
+              name = "Nix sites";
+              bookmarks = [
+                {
+                  name = "homepage";
+                  url = "https://nixos.org/";
+                }
+                {
+                  name = "wiki";
+                  url = "https://wiki.nixos.org/";
+                }
+              ];
+            }
+          ];
+        };
+      };
+    };
+  } // {
+    nmt.script = ''
+      bookmarksUserJs=$(normalizeStorePaths \
+        home-files/${cfg.configPath}/bookmarks/user.js)
+
+      assertFileContent \
+        $bookmarksUserJs \
+        ${./expected-bookmarks-user.js}
+
+      bookmarksFile="$(sed -n \
+        '/browser.bookmarks.file/ {s|^.*\(/nix/store[^"]*\).*|\1|;p}' \
+        $TESTED/home-files/${cfg.configPath}/bookmarks/user.js)"
+
+      assertFileContent \
+        $bookmarksFile \
+        ${./expected-bookmarks.html}
+    '';
+  });
+}
--- a/tests/modules/programs/firefox/profiles/bookmarks/default.nix
+++ b/tests/modules/programs/firefox/profiles/bookmarks/default.nix
@@ -24,13 +24,6 @@ in {
              url = "https://wiki.nixos.org/wiki/Home_Manager";
            }];
          }
-          {
-            name = "wikipedia";
-            tags = [ "wiki" ];
-            keyword = "wiki";
-            url =
-              "https://en.wikipedia.org/wiki/Special:Search?search=%s&go=Go";
-          }
          {
            name = "kernel.org";
            url = "https://www.kernel.org";
@@ -62,6 +55,13 @@ in {
              }
            ];
          }
+          {
+            name = "wikipedia";
+            tags = [ "wiki" ];
+            keyword = "wiki";
+            url =
+              "https://en.wikipedia.org/wiki/Special:Search?search=%s&go=Go";
+          }
        ];
      };
    };
--- a/tests/modules/programs/firefox/profiles/bookmarks/expected-bookmarks.html
+++ b/tests/modules/programs/firefox/profiles/bookmarks/expected-bookmarks.html
@@ -10,7 +10,6 @@
  <DL><p>
    <DT><A HREF="https://wiki.nixos.org/wiki/Home_Manager" ADD_DATE="1" LAST_MODIFIED="1">Home Manager</A>
  </DL><p>
-  <DT><A HREF="https://en.wikipedia.org/wiki/Special:Search?search=%s&amp;go=Go" ADD_DATE="1" LAST_MODIFIED="1" SHORTCUTURL="wiki" TAGS="wiki">wikipedia</A>
  <DT><A HREF="https://www.kernel.org" ADD_DATE="1" LAST_MODIFIED="1">kernel.org</A>
  <DT><H3 ADD_DATE="1" LAST_MODIFIED="1">Nix sites</H3>
  <DL><p>
@@ -22,4 +21,5 @@
      <DT><A HREF="https://wiki.nixos.org/" ADD_DATE="1" LAST_MODIFIED="1">wiki</A>
    </DL><p>
  </DL><p>
+  <DT><A HREF="https://en.wikipedia.org/wiki/Special:Search?search=%s&amp;go=Go" ADD_DATE="1" LAST_MODIFIED="1" SHORTCUTURL="wiki" TAGS="wiki">wikipedia</A>
 </DL>