You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
I've reviewed this PR that adds auto-start functionality for niri as adminUser on boot via greetd configuration.
Todo List:
Gather context by reading the changed file and repo guidelines
Analyze code quality and best practices
Check for potential bugs or issues
Review performance considerations
Assess security concerns
Evaluate test coverage needs
Provide comprehensive review feedback
Review Feedback
Summary
The PR adds an initial_session configuration to greetd that automatically starts the niri Wayland compositor for the admin user on boot. This is a clean, minimal change that follows the existing patterns in the codebase.
Code Quality and Best Practices ✅
Good: The change follows NixOS module conventions properly
Good: Consistent with the existing code style and patterns
Good: Properly adds adminUser to the function parameters
Good: Reuses the existing runNiri wrapper which already handles environment setup
Potential Bugs or Issues ⚠️
Missing validation: There's no check to ensure adminUser.name is defined. While this appears to be a standard parameter in this codebase (used in multiple other modules), it could cause evaluation errors if undefined.
Consider adding an assertion or making this feature optional with lib.mkIf
Session conflict: If a user is already logged in or if the greeter is still active, having an initial_session might cause conflicts. The current implementation doesn't handle this edge case.
No error recovery: If niri fails to start for any reason, there's no fallback mechanism to show the greeter instead.
Performance Considerations ✅
No significant performance concerns. The initial_session starts only once at boot.
Uses the same optimized runNiri wrapper that includes systemd-cat integration for proper logging.
Security Concerns ⚠️
Automatic login bypass: This configuration effectively bypasses the login screen for the admin user, which could be a security concern in multi-user environments or systems that require authentication at boot.
Consider documenting this behavior clearly
May want to make this configurable with an option like services.greetd.autoLogin.enable
Admin privileges: Starting a session automatically as the admin user grants immediate access to administrative capabilities without authentication.
Test Coverage 🔍
No tests are included in this PR
Consider adding:
NixOS VM tests to verify the auto-start behavior
Tests to ensure the greeter still works when initial_session fails
Documentation or comments explaining the intended use case
Suggestions for Improvement
Make it configurable: Consider wrapping this in an option:
services.greetd.autoLogin={enable=lib.mkEnableOption"automatic login for admin user";user=lib.mkOption{type=lib.types.str;default=adminUser.name;description="User to automatically log in";};command=lib.mkOption{type=lib.types.str;default="${runNiri}/bin/niri";description="Command to run for auto-login session";};};
Add documentation: Include a comment explaining why auto-login is desired and any security implications.
Consider the use case: Is this for a single-user system, kiosk mode, or development environment? The appropriateness of auto-login depends heavily on the context.
Overall Assessment
The implementation is technically correct and follows established patterns. However, the security implications of automatic admin login should be carefully considered. I'd recommend making this behavior optional through a configuration flag rather than always-on.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.