refactor: remove passive shell hook implementation#109
refactor: remove passive shell hook implementation#109AyushSrivastava1818 wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR removes shell-hook support, deletes the hook command modules, unwires the CLI entries, and updates CI, release notes, and README content to describe manual command saving instead. ChangesShell hook feature removal
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/tracker.js (1)
51-52: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStale docs still reference removed
hook/unhookcommands.The CLI cleanly removes the
hook/unhookregistrations and their import here, but downstream release/onboarding docs still instruct users to run the now-deleted command..github/workflows/publish.ymlrelease notes includenpx tracker hookin its Quick Start, andREADME.mdstill documentstracker hook/tracker unhookas available commands. Once this PR merges, following those docs will fail with an unknown-command error.Suggest updating
.github/workflows/publish.ymlandREADME.mdin this PR (or a fast-follow) to drop the hook/unhook references, matching the CLI removal.Also applies to: 214-214
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/tracker.js` around lines 51 - 52, The CLI no longer registers hook/unhook, so update the remaining docs to match the current command set. In README and the publish workflow release notes, remove any `tracker hook` and `tracker unhook` references and replace them with the supported commands shown by `exportCommand` and `favoriteCommand`/`favoritesCommand` from `bin/tracker.js`. Keep the onboarding/release examples aligned with the removed import and command registration so users don’t copy an invalid command.
🧹 Nitpick comments (1)
bin/tracker.js (1)
214-214: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale shell-hook wording left in other command modules.
src/commands/list.jsstill tells users to "run commands normally if shell hook is set up," andsrc/commands/save.jscomments still say "save runs automatically via shell hook." Now that the hook feature is fully removed, these references are misleading.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/tracker.js` at line 214, Remove the leftover shell-hook references from the command modules that still mention the removed feature. Update the user-facing text and comments in src/commands/list.js and src/commands/save.js so they no longer say to “run commands normally if shell hook is set up” or that “save runs automatically via shell hook,” and replace them with the current manual behavior using the relevant command handlers in those modules.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@bin/tracker.js`:
- Around line 51-52: The CLI no longer registers hook/unhook, so update the
remaining docs to match the current command set. In README and the publish
workflow release notes, remove any `tracker hook` and `tracker unhook`
references and replace them with the supported commands shown by `exportCommand`
and `favoriteCommand`/`favoritesCommand` from `bin/tracker.js`. Keep the
onboarding/release examples aligned with the removed import and command
registration so users don’t copy an invalid command.
---
Nitpick comments:
In `@bin/tracker.js`:
- Line 214: Remove the leftover shell-hook references from the command modules
that still mention the removed feature. Update the user-facing text and comments
in src/commands/list.js and src/commands/save.js so they no longer say to “run
commands normally if shell hook is set up” or that “save runs automatically via
shell hook,” and replace them with the current manual behavior using the
relevant command handlers in those modules.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d342f39-75a0-41c7-98aa-6774a738b7ef
📒 Files selected for processing (4)
.github/workflows/ci.ymlbin/tracker.jssrc/commands/hook.jssrc/utils/hook.js
💤 Files with no reviewable changes (3)
- src/commands/hook.js
- src/utils/hook.js
- .github/workflows/ci.yml
|
Hi @adithya-naik! 👋 I've addressed the CodeRabbit feedback and pushed an update to this PR. Changes made
Thanks again for the review and guidance! Please let me know if there's anything else you'd like me to improve. I'm happy to make any additional changes if needed. 🚀 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Line 5: The README heading hierarchy is skipping a level: the subtitle under
the document title is using h3 instead of h2. Update the markdown in README.md
so the line with “A developer tool that categorizes and saves terminal commands
per project for easy revision” uses the proper heading level to follow the
top-level title and satisfy MD001.
- Line 5: The CLI help text is out of sync with the README because
program.description() in the bin/tracker.js entrypoint still says
“auto-captures.” Update that description string to match the current README
wording (“A developer tool that categorizes and saves terminal commands per
project for easy revision”) so the documented behavior and help output stay
consistent.
- Line 168: Update the platform support table entry for Fish in README.md
because the current “Full support” label no longer matches the manual-save
behavior. Adjust the Fish row in the support table to reflect that users must
run tracker save explicitly, and keep the wording consistent with how
PowerShell/CMD are described; use the support-table row for Fish as the unique
location to update.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5422c64a-fa38-4ff4-b162-a903d36ca0a6
📒 Files selected for processing (2)
.github/workflows/publish.ymlREADME.md
💤 Files with no reviewable changes (1)
- .github/workflows/publish.yml
| # 📟 cmd-tracker | ||
|
|
||
| ### A developer tool that auto-captures, categorizes and saves terminal commands per project for easy revision | ||
| ### A developer tool that categorizes and saves terminal commands per project for easy revision |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix heading level increment.
Line 5 uses h3 (###) but follows the document title (h1), violating markdownlint rule MD001. Change to h2 (##) to maintain proper heading hierarchy.
-### A developer tool that categorizes and saves terminal commands per project for easy revision
+## A developer tool that categorizes and saves terminal commands per project for easy revision📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### A developer tool that categorizes and saves terminal commands per project for easy revision | |
| ## A developer tool that categorizes and saves terminal commands per project for easy revision |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 5-5: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 5, The README heading hierarchy is skipping a level: the
subtitle under the document title is using h3 instead of h2. Update the markdown
in README.md so the line with “A developer tool that categorizes and saves
terminal commands per project for easy revision” uses the proper heading level
to follow the top-level title and satisfy MD001.
Source: Linters/SAST tools
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
CLI help text still claims "auto-captures".
The program.description() in bin/tracker.js still reads "📟 A developer tool that auto-captures, categorizes and saves terminal commands per project for easy revision" (lines 10–63 in the CLI entrypoint). Since the README removed the auto-capture narrative, the CLI help string is now inconsistent with the documented behavior. Align the CLI description with the README wording.
// In bin/tracker.js
-program.description('📟 A developer tool that auto-captures, categorizes and saves terminal commands per project for easy revision');
+program.description('📟 A developer tool that categorizes and saves terminal commands per project for easy revision');🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 5-5: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 5, The CLI help text is out of sync with the README
because program.description() in the bin/tracker.js entrypoint still says
“auto-captures.” Update that description string to match the current README
wording (“A developer tool that categorizes and saves terminal commands per
project for easy revision”) so the documented behavior and help output stay
consistent.
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
|
Can u suggest me any option for auto saving the commands instead of a manual saving, as I feel that auto capturing should not be disabled |
|
Thanks for the suggestion! I agree that automatic saving provides a better user experience. Since the passive shell hook was removed due to the concerns raised in this issue, perhaps we could explore an opt-in approach instead. For example, users could explicitly enable auto-save during setup or via a config flag, without modifying shell configuration automatically. That would preserve the manual-first default while still giving users the option to enable automatic command saving if they prefer. |
Will you be able to achieve this? |
|
Yes, I'd be happy to work on this. I'll explore an opt-in auto-save approach that doesn't rely on modifying shell configuration and keeps the current manual workflow as the default. I'll share a proposal or draft implementation once I have a working approach. |
Great !! ❤️ |
|
I explored the current implementation. Since auto-save previously relied on a shell hook, I'm considering an opt-in approach where users explicitly enable it (for example, via a tracker enable-autosave command that provides the required shell snippet instead of modifying shell configuration automatically). This would preserve the manual-first default while still allowing automatic command capture. Does this align with what you had in mind, or were you thinking of a different mechanism (such as a background watcher)? |
Yes that works! |
|
I have one doubt.... |
2 participants
Summary
This PR removes the passive shell hook implementation as part of the security refactor described in #99.
Changes made
tracker hookandtracker unhookCLI commands.src/utils/hook.js, which contained the shell hook installation/removal logic.src/commands/hook.js, since it became unused after removing the hook feature.Validation
npm installnpm run lintnpm testtracker --helpandtracker --versionScope
I intentionally did not modify
package.json(such as the package description or keywords) because this issue is focused on removing the passive shell hook implementation. Those metadata changes are broader project-level updates and can be handled separately if desired, keeping this PR focused on the requested refactor.Summary by CodeRabbit
hookandunhookcommands from the CLI, along with automatic shell-hook setup.tracker savehelp text and removed auto-capture wording.