-
Notifications
You must be signed in to change notification settings - Fork 0
feat(zellij): add zjstatus dev environment with git status bar #127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Add zellij config with zjstatus plugin layouts (single-pane and dev split) - Add zellij-git-status script for live git info in status bar - Add shell integrations (fish/bash) for pipe-based status updates - Add zellij-update-theme for theme synchronization - Configure autostart on workspace 3 via Hyprland - Update AGENTS.md docs and add zellij package to registry Closes #126
Code Review: feat(zellij): add zjstatus dev environment with git status barSummaryThis PR adds Zellij terminal multiplexer configuration with zjstatus plugin for git-aware status bar display. The implementation is comprehensive and well-documented. 🚨 Critical Issues1. Security: Hardcoded SSH Credentials in Config# .bashrc and config.fish
alias agents='ssh -i ~/.ssh/acfs_ed25519 ubuntu@100.74.195.52'Issue: This exposes:
Recommendation: Remove this alias from tracked dotfiles. Store sensitive connection details in 2. Binary Files Committed:
|
| Issue | Severity | File |
|---|---|---|
| SSH alias with hardcoded credentials | High | .bashrc, config.fish |
| Opencode PATH export exposes username | Low | .bashrc |
🧪 Test Coverage
- ✅ Manual testing documented in PR description
⚠️ No automated tests for new shell scripts (zellij-git-status,zellij-update-theme)- Consider adding basic shellcheck validation for new scripts
📦 Performance
- Shell integration hooks (
__zjstatus_git_update) run on every prompt - Uses
disownto prevent blocking - good practice - Git operations are lightweight (status, rev-parse)
watch -t -n 2in git-dev layout adds minimal overhead
Recommended Actions Before Merge
-
Must fix:
- Remove SSH alias with hardcoded credentials from tracked files
- Add
uv,uvxto.gitignoreand remove from tracking
-
Should fix:
- Add
zjstatus.wasmto.gitignore(install via curl instead) - Fix confusing path in
envscripts - Verify Typora symlinks or remove broken ones
- Add
-
Nice to have:
- Add shellcheck step for new bin scripts
- Add
set -euo pipefailto shell scripts
Overall Assessment
This is a solid addition that brings modern terminal multiplexer support with git integration. The configuration is well-documented and follows project conventions. However, the security issues (exposed SSH credentials) and large binary files should be addressed before merging.
Recommendation: Request changes for security fixes, approve after addressing critical issues.
🤖 Generated with Claude Code
Code Review: feat(zellij): add zjstatus dev environment with git status barThis is a substantial PR adding Zellij terminal multiplexer configuration with git-aware status bar integration. Overall, the implementation is well-structured with comprehensive documentation. However, there are several issues that should be addressed before merging. Security Concerns1. Hardcoded SSH credentials in tracked files (Critical) Both alias agents='ssh -i ~/.ssh/acfs_ed25519 ubuntu@100.74.195.52'This violates the project's own guidelines in AGENTS.md:
While the private key itself isn't committed, the alias should be in Recommendation: Move this alias to 2. Broken symlinks in Typora themes (Bug) The new symlinks in This relative path assumes Code Quality Issues3. Binary WASM file committed (Major) The PR commits
While zjstatus.wasm is much smaller, binary files should generally not be committed. Consider:
4. Hardcoded paths in layouts In pane size="60%" focus=true cwd="~/s"The 5. Hardcoded timezone In both datetime_timezone "America/New_York"This should either be removed (to use system default) or made configurable. Best Practices6. PATH manipulation concerns In export PATH="$HOME/.local/share/../bin:$PATH"The path 7. Missing shebang validation The script 8. Potential race condition in shell integration The bash integration spawns background processes: zellij pipe "zjstatus::pipe::pipe_git::${git_status}" &>/dev/null &
disown &>/dev/nullWhile this is non-blocking, it could lead to many orphaned processes if the user runs many commands quickly. Consider adding rate limiting or debouncing. Positive Aspects
Summary
Recommendation: Address the security concern (#1) and the broken symlinks (#2) before merging. The binary file (#3) and other issues can be addressed in follow-up PRs if preferred. Generated with Claude Code |
- Remove SSH alias with hardcoded credentials from .bashrc and config.fish - Add zjstatus.wasm to .gitignore (install via curl per README) - Remove broken Typora symlinks in old-themes directory - Simplify PATH in env scripts ($HOME/.local/share/../bin → $HOME/.local/bin)
Code Review: feat(zellij): add zjstatus dev environment with git status barOverall this is a solid addition that provides a productive terminal multiplexer setup with git-aware status bar integration. Here's my detailed feedback: ✅ What's Good
🐛 Potential Issues1. Hardcoded path in
|
| Category | Status |
|---|---|
| Code quality | |
| Best practices | ✅ Good separation of concerns |
| Performance | |
| Security | ✅ No concerns |
| Test coverage | ❌ No new tests |
Recommendation: Address the hardcoded paths before merging. The performance items are nice-to-haves that can be addressed in follow-up PRs.
🤖 Generated with Claude Code
Code Review: feat(zellij): add zjstatus dev environment with git status barThanks for this PR! The Zellij configuration with zjstatus integration looks like a solid addition to the dotfiles. Here is my review: High Priority Issues1. Hardcoded Home Directory Path in .bashrcThe PATH export uses a hardcoded /home/jc path which breaks for any user other than jc. This should use the HOME environment variable instead. 2. Race Condition in zellij-update-themeThe temp file replacement pattern is vulnerable to race conditions. Consider using mktemp for safer atomic file operations with proper trap cleanup. Medium Priority Issues3. Missing Quoting on PATH ExportThe PATH export should use proper quoting to prevent issues with paths containing spaces. 4. Complex Autostart CommandThe autostart-opencode.conf command is dense and hard to maintain. Consider extracting to a dedicated script for readability and easier debugging. 5. awk getline Edge Cases in zellij-update-themeThe getline usage in the awk script could behave unexpectedly if the comment line is not followed by another line or if brace matching is off in the config. Consider adding validation. Low Priority Suggestions6. Shebang PortabilityScripts use #!/bin/bash. For better portability, consider using #!/usr/bin/env bash. 7. Missing set -euo pipefailConsider adding strict mode to shell scripts, or at least set -o pipefail to catch pipe failures. 8. PATH Deduplication in .bashrcUnlike fish_add_path (which handles this automatically), bash does not deduplicate PATH entries. Consider adding a check before appending. 9. Fish disown May Be UnnecessaryIn integration.fish, fish 3.x handles background jobs automatically with &, so disown may be unnecessary. What Looks Good
SummaryThe PR is well-structured and adds useful functionality. The main blocking issue is the hardcoded home directory path which should be fixed before merging. The other items are improvements that could be addressed now or in follow-up PRs. Overall: Approve with requested changes (fix the hardcoded path) |
- Only update git status when PWD changes or after 2 second interval - Prevent race conditions with update guard flag (bash) - Fix fish syntax: use >/dev/null 2>&1 instead of &>
Code Review: feat(zellij): add zjstatus dev environment with git status barSummaryThis PR adds a comprehensive Zellij terminal multiplexer setup with git-aware status bar integration. It is a well-organized feature that follows the repository conventions. Positive Highlights
Issues to AddressSecurity Concerns
Potential Bugs
Code Quality
Performance Considerations
Test Coverage
Minor Suggestions
SummaryOverall: This is a solid, well-documented feature addition. The main items to address before merging:
The documentation improvements to AGENTS.md are a nice bonus, making the file more scannable while preserving essential information. |
Summary
Closes #126
Changes
New Files
.config/zellij/layouts/zjstatus.kdl.config/zellij/layouts/zjstatus-dev.kdl.config/zellij/integration.{fish,bash}.local/bin/zellij-git-status.local/bin/zellij-update-themeModified Files
.config/fish/config.fish.bashrc.config/hypr/autostart-opencode.conflib/packages.shAGENTS.mdTesting
Note
uvbinary (52MB) included - may want to add to.gitignoreor use Git LFS in future.