Skip to content

docs: design den dendritic migration#46

Open
phucisstupid wants to merge 14 commits into
mainfrom
den-dendritic-migration
Open

docs: design den dendritic migration#46
phucisstupid wants to merge 14 commits into
mainfrom
den-dendritic-migration

Conversation

@phucisstupid

Copy link
Copy Markdown
Owner

No description provided.

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request migrates the repository to a Den dendritic layout, restructuring configuration into modular aspects, hosts, and users while implementing flake-file for automated flake.nix generation. The review identifies several critical logic errors, specifically the omission of the den argument in function headers across multiple modules, which will prevent successful evaluation. Furthermore, the home-manager darwin module is not correctly imported, a shell command in the aerospace configuration is left incomplete, and user identity metadata is hardcoded in the Git aspect instead of being centralized for maintainability.

Comment on lines +2 to +17
{inputs, ...}: let
hostConfig = {
darwin.home-manager = {
useUserPackages = true;
useGlobalPkgs = true;
backupFileExtension = "backup";
sharedModules = [
inputs.catppuccin.homeModules.catppuccin
inputs.stylix.homeModules.stylix
inputs.lazyvim.homeManagerModules.lazyvim
inputs.nvf.homeManagerModules.default
inputs.nix4nvchad.homeManagerModules.default
inputs.spicetify-nix.homeManagerModules.spicetify
];
};
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

This module has two critical issues:

  1. The den argument is missing from the function header, which will cause an 'undefined variable' error when accessing den.aspects on line 47.
  2. The home-manager module for nix-darwin is not imported. Without inputs.home-manager.darwinModules.home-manager in the imports list, the darwin.home-manager options will not be defined, leading to evaluation errors.
References
  1. The migration plan explicitly required importing the home-manager darwin module and using the den argument. (link)
  2. Nix modules must import the necessary provider modules (like home-manager's darwin module) to make their options available.

Comment on lines +1 to +34
_: let
mod = "alt";
aerospace = {
homeManager = {lib, ...}: {
services.jankyborders = {
enable = true;
settings = {
active_color = "0xffcba6f7";
hidpi = "on";
};
};
programs.aerospace = {
enable = true;
launchd.enable = true;
settings = {
key-mapping = {
preset = "colemak";
key-notation-to-key-code = {
g = "b";
d = "g";
h = "m";
z = "x";
x = "c";
c = "d";
b = "z";
m = "h";
};
};
default-root-container-layout = "tiles";
automatically-unhide-macos-hidden-apps = true;
exec-on-workspace-change = [
"/bin/bash"
"-c"
];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

This module contains critical errors:

  1. The den argument is missing from the header, causing an error on line 147.
  2. The exec-on-workspace-change command is incomplete. ["/bin/bash", "-c"] requires a command string as the third element. If no script is intended, this block should be removed to avoid execution errors.

@@ -0,0 +1,22 @@
# Common nix-darwin host settings.
_: let

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The den argument is missing from the function header. This will cause an 'undefined variable' error when the module attempts to access den.aspects on line 21.

Comment thread modules/users/users.nix
@@ -0,0 +1,8 @@
# Default user context.
{lib, ...}: {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The den argument is missing from the function header, which will cause an error when accessing den.schema.user on line 3.

Comment on lines +1 to +10
_: let
git = {
homeManager = {
programs.git = {
enable = true;
settings = {
user = {
name = "phucisstupid";
email = "125681538+phucisstupid@users.noreply.github.com";
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

  1. The den argument is missing from the header, which will break the build on line 46.
  2. User identity (name/email) is hardcoded. Since this migration aims for a dendritic layout with shared aspects, consider using a central profile aspect for user metadata to ensure consistency across Git, Jujutsu, and other tools.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant