WIP: feat(user-defaults): implement role-based application defaults#45
WIP: feat(user-defaults): implement role-based application defaults#45
Conversation
Companion to the implementation plan with actionable tasks: - Pre-implementation verification (environment, dependencies, WM_CLASS) - Phase 1: Core infrastructure (6 steps with subtasks) - Phase 2: Consumer migration (i3-config.nix refactoring) - Post-implementation validation (build, functional, regression) - Rollback procedure
- Remove redundant mkdir (lib/ already exists) - Fix Step 1.6 verify: use nix-instantiate --parse (import-tree auto-discovers) - Add validation module execution check via build output grep
|
Claude finished @Bad3r's task —— View job PR Review: User Defaults ImplementationI've completed a comprehensive review of this WIP pull request. This is a well-planned implementation with excellent documentation. Here's my detailed analysis: Review Tasks
✅ StrengthsExcellent Planning & Documentation:
Solid Architecture:
Security & Safety:
Repository Compliance:
|
Plan changes: - Add escapeRegex helper to handle special characters in WM_CLASS names - mkAssign now escapes regex metacharacters before concatenation Checklist changes: - Add Step 1.5.1: Intermediate HM verification (critical path) - Add negative test for invalid package paths (validation module) - Add windowClassAliases regex verification test - Add regex escaping verification test
Replace custom escapeRegex helper with nixpkgs built-in lib.strings.escapeRegex. More idiomatic and already tested/maintained upstream.
Architecture improvement: helpers are now in lib/user-defaults-helpers.nix from the beginning, avoiding future duplication when adding sway/hyprland. Plan changes: - Add Step 1.2: Create helper functions library - Renumber Steps 1.3-1.7 accordingly - Step 2.1 now imports helpers instead of defining inline - Update File Changes Summary with new file Checklist changes: - Add Step 1.2 for helpers file creation - Renumber Steps 1.3-1.7 - Step 2.1.2 now imports helpers instead of adding them inline
Create TODO.md with phased task breakdown for user-defaults feature: - Phase 0: Preparation and codebase analysis - Phase 1: Module infrastructure (options, submodule, defaults) - Phase 2: Helper functions library - Phase 3: Wiring & propagation verification - Phase 4: Validation logic (assertions, warnings, strictValidation) - Phase 5: Consumer migration (i3-config.nix refactoring) - Phase 6: Final validation with WM_CLASS reference - Phase 7: Cleanup and documentation Includes verification commands, rollback plan, and progress tracking.
The plan incorrectly claimed modules would be "auto-imported via dendritic pattern" by exporting to flake.nixosModules.meta.userDefaults. Investigation revealed: - Dendritic pattern auto-discovers FILES, not auto-imports NixOS MODULES - Host config (system76/imports.nix) explicitly imports specific modules - 13+ modules contribute to flake.nixosModules.base (aggregator pattern) - flake-parts merges contributions; host imports base once Changes: - Export to flake.nixosModules.base instead of meta.userDefaults - Rewrite Step 1.3 with accurate aggregator pattern explanation - Update data flow diagram, comments, and File Changes Summary - Add base aggregator pattern to Dependencies section - Update TODO.md section 1.5 with correct export task This ensures the module options will actually be available in the host config.
1. Remove unused `name` parameter from appRoleModule - Submodules in `attrsOf` receive `name` automatically - Not needed since validation uses mapAttrsToList for role names - Added comment explaining the design choice 2. Fix misleading osConfig verification - Removed `useGlobalPkgs` test (contradicted by plan's own statement) - Documented that no reliable pre-verification exists - Added actual error message users will see if osConfig is missing 3. Fix incorrect documentation references - settings-options.section.md covers format generators (JSON/YAML) - option-types.section.md has the "Submodule types" section - Updated both references to point to correct section
Address three issues and five checklist gaps identified during critical
review against official NixOS documentation:
Issues fixed:
- Replace mkEnableOption // override with explicit mkOption (clearer)
- Redesign helpers as pure functions accepting app data directly
instead of config + role lookup (improves testability, reduces coupling)
- Document that custom roles require all required fields
Checklist improvements:
- Add custom role test to Phase 6
- Add dendritic discovery verification to Phase 1.6
- Remove fragile hardcoded line numbers from Phase 0.1
- Add concrete strictValidation mismatch example to Phase 4.5
- Clarify that Phase 4 extends Phase 1's module file
Helper function changes:
- Renamed mkAssign -> mkAssignFromApp
- Changed signature from { lib, config } to { lib } (pure)
- Updated all consumer examples to pass app data directly
d1081cc to
c11b4d5
Compare
1b0dc9c to
6482506
Compare
Summary
Implements a centralized user defaults system for application role mapping, replacing hardcoded app references scattered across modules.
Status: 🚧 Work in Progress — Documentation complete, implementation pending
What's included
Implementation Plan:
docs/drafts/user-defaults-implementation-plan.mdappId) and multi-class matching (windowClassAliases)Implementation Checklist:
docs/drafts/user-defaults-checklist.mdDesign decisions
userDefaultsis intentionally separate fromprograms.<name>.extended.package(different concerns)metaOwnerinjection patternTest plan
gui.i3.commands) still worksRemaining work