Added search history for commands used. #106
Conversation
|
🚀 Hi @SauravBhowmick! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
WalkthroughAdded two new commands— Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant App
participant Context as ShellContext
participant ISearch as InteractiveSearchCommand
participant Search as SearchHistoryCommand
participant Scanner as SystemScanner
Note over App,Scanner: App startup — create Scanner and inject into context
App->>Scanner: new Scanner(System.in)
App->>Context: setScanner(sc)
User->>App: invoke "isearch" or "searchhistory [term]"
alt isearch flow
App->>ISearch: execute(args, context)
ISearch->>Context: getHistory()
Context-->>ISearch: history[]
ISearch->>Context: getScanner()
Context-->>ISearch: Scanner
loop interactive loop
ISearch->>User: prompt "search term (q to quit)"
User-->>ISearch: term
ISearch->>ISearch: filter history (reverse-chronological, case-insensitive)
ISearch->>User: display up to 10 matches (show count, more indicator)
User-->>ISearch: select index / q
alt valid selection
ISearch->>User: show selected command
else invalid selection
ISearch->>User: show error message
end
end
else searchhistory flow
App->>Search: execute(args, context)
Search->>Context: getHistory()
Context-->>Search: history[]
Search->>Search: if args -> filter history (case-insensitive)
Search->>User: print full history or matched list (with bracket highlights)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/java/com/mycmd/commands/InteractiveSearchCommand.java (1)
99-112: Consider simplifying the reverse operation.The current implementation works correctly, but the nested collectors can be simplified for better readability.
Apply this diff for a more concise approach:
private List<String> searchHistory(List<String> history, String searchTerm) { String lowerSearch = searchTerm.toLowerCase(); - // Search in reverse order (most recent first) - return history.stream() + List<String> matches = history.stream() .filter(cmd -> cmd.toLowerCase().contains(lowerSearch)) - .collect(Collectors.collectingAndThen( - Collectors.toList(), - list -> { - java.util.Collections.reverse(list); - return list; - } - )); + .collect(Collectors.toList()); + + // Reverse to show most recent first + java.util.Collections.reverse(matches); + return matches; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/mycmd/App.java(2 hunks)src/main/java/com/mycmd/commands/InteractiveSearchCommand.java(1 hunks)src/main/java/com/mycmd/commands/SearchHistoryCommand.java(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: ⚙️ Java CI Build
src/main/java/com/mycmd/commands/SearchHistoryCommand.java
[error] 101-101: method does not override or implement a method from a supertype
[error] 106-106: method does not override or implement a method from a supertype
🔇 Additional comments (5)
src/main/java/com/mycmd/App.java (2)
156-157: LGTM! Command identifiers follow the established pattern.The new command identifiers are properly defined and follow the naming conventions used throughout the codebase.
235-236: LGTM! Command registration is consistent.The registration of the new commands follows the existing pattern and correctly maps the command identifiers to their implementations.
src/main/java/com/mycmd/commands/SearchHistoryCommand.java (2)
17-51: LGTM! Well-structured search implementation.The execute method handles edge cases appropriately, provides clear user feedback, and implements case-insensitive searching correctly.
77-99: LGTM! Effective highlighting implementation.The highlighting logic correctly handles all occurrences of the search term with case-insensitive matching. The bracket-based highlighting is simple and works across all terminals.
src/main/java/com/mycmd/commands/InteractiveSearchCommand.java (1)
34-92: LGTM! Well-designed interactive search flow.The interactive loop provides a good user experience with clear prompts, appropriate error handling, and intuitive navigation. The limit of 10 results keeps the output manageable.
|
🚀 Hi @SauravBhowmick! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/com/mycmd/commands/SearchHistoryCommand.java (1)
73-99: Correct implementation with room for enhancement.The case-insensitive highlighting logic correctly identifies and marks all occurrences of the search term. Using
StringBuilderfor string concatenation is efficient and appropriate here.As noted in the comment on line 75, consider enhancing the highlighting with ANSI escape codes for color output in terminals that support it:
// Example ANSI highlighting (optional enhancement): private String highlightTerm(String text, String term) { String HIGHLIGHT = "\u001B[33m"; // Yellow String RESET = "\u001B[0m"; // ... rest of logic using ANSI codes instead of brackets }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/mycmd/commands/SearchHistoryCommand.java(1 hunks)
🔇 Additional comments (5)
src/main/java/com/mycmd/commands/SearchHistoryCommand.java (5)
1-14: LGTM! Clean structure and clear documentation.The package structure, imports, and class documentation are well-organized. The Javadoc clearly explains the command's purpose and usage patterns.
53-71: Nice formatting logic!The dynamic width calculation based on the number of entries ensures clean, aligned output regardless of history size. The optional highlighting integration is well-designed.
101-104: Previous issue resolved! Method now correctly implements the interface.The method name has been corrected from
getDescription()todescription()to properly override the Command interface method. The description is clear and concise.
106-113: Previous issue resolved! Excellent usage documentation.The method name has been corrected from
getUsage()tousage()to properly override the Command interface method. The usage examples are comprehensive and clearly demonstrate the command's capabilities, including the multi-word search feature.
16-51: No changes needed — the code is safe as written.The code already handles null safety correctly:
getHistory()contract: The method inShellContextreturnsnew ArrayList<>(history), which is always non-null (even if history is empty). The method explicitly returns a defensive copy.
contextparameter: As a method implementing theCommandinterface, the framework guarantees non-null invocation context (standard framework pattern).Existing safety check: Line 20's
if (history.isEmpty())properly handles the empty case without risk of NPE.The Java 17 project correctly uses
.toList()(supported since Java 16), which is the idiomatic modern approach.
|
Hey @SauravBhowmick , In |
|
Ok will check and resolve it |
|
🚀 Hi @SauravBhowmick! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
|
🚀 Hi @SauravBhowmick! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/com/mycmd/commands/InteractiveSearchCommand.java (1)
101-114: Consider simplifying the reverse collection logic.The searchHistory method works correctly, filtering case-insensitively and returning results in reverse-chronological order. However, the
collectingAndThenwithCollections.reverseis verbose.Consider this more concise alternative:
private List<String> searchHistory(List<String> history, String searchTerm) { String lowerSearch = searchTerm.toLowerCase(); - - // Search in reverse order (most recent first) - return history.stream() - .filter(cmd -> cmd.toLowerCase().contains(lowerSearch)) - .collect(Collectors.collectingAndThen( - Collectors.toList(), - list -> { - java.util.Collections.reverse(list); - return list; - } - )); + + // Search in reverse order (most recent first) + List<String> matches = new java.util.ArrayList<>(); + for (int i = history.size() - 1; i >= 0; i--) { + if (history.get(i).toLowerCase().contains(lowerSearch)) { + matches.add(history.get(i)); + } + } + return matches; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/mycmd/commands/InteractiveSearchCommand.java(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: ⚙️ Java CI Build
src/main/java/com/mycmd/commands/InteractiveSearchCommand.java
[error] 30-34: cannot find symbol: method getScanner() referenced on variable context of type com.mycmd.ShellContext. Compilation failure during Maven build.
🔇 Additional comments (4)
src/main/java/com/mycmd/commands/InteractiveSearchCommand.java (4)
1-8: LGTM!Package declaration and imports are clean and appropriate for the command's functionality.
9-27: LGTM!The class documentation is clear, and the empty history guard clause provides good user feedback.
32-95: Verify that displaying (not executing) the selected command is the intended behavior.The interactive loop logic is well-structured with proper input validation and error handling. However, Line 84 displays the selected command but doesn't execute it. The message tells users to "type it at the prompt" manually.
Verify this matches your requirements—typical interactive history search tools (like bash Ctrl+R) execute or place the command in the input buffer for immediate use. If auto-execution is desired, you'll need to handle command execution here or return the selected command to the main shell loop.
116-126: LGTM!The description() and usage() methods provide clear, concise documentation for users.
|
🚀 Hi @SauravBhowmick! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
|
🚀 Hi @SauravBhowmick! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/mycmd/ShellContext.java (1)
14-32: Missing Scanner field declaration causes build failure.The code references
scannerat lines 32, 43, 53, and 56, but the field is never declared. This is causing all four pipeline compilation errors.Add the Scanner field declaration after line 16:
@Setter @NonNull private File currentDir; private List<String> history; private Map<String, String> aliases; + private Scanner scanner; private static final String ALIAS_FILE = ".mycmd_aliases";
🧹 Nitpick comments (1)
src/main/java/com/mycmd/ShellContext.java (1)
42-44: Consider adding validation to enforce single-initialization contract.The Javadoc states this "should only be called once," but there's no runtime enforcement. Additionally, accepting a null Scanner would break getScanner's contract.
Apply this diff to add guards:
public void setScanner(Scanner scanner) { + if (this.scanner != null) { + throw new IllegalStateException("Scanner already initialized"); + } + if (scanner == null) { + throw new IllegalArgumentException("Scanner cannot be null"); + } this.scanner = scanner; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/mycmd/App.java(3 hunks)src/main/java/com/mycmd/ShellContext.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/mycmd/App.java
🧰 Additional context used
🪛 GitHub Actions: ⚙️ Java CI Build
src/main/java/com/mycmd/ShellContext.java
[error] 32-32: cannot find symbol: variable scanner
[error] 43-43: cannot find symbol: variable scanner
[error] 53-53: cannot find symbol: variable scanner
[error] 56-56: cannot find symbol: variable scanner
🔇 Additional comments (1)
src/main/java/com/mycmd/ShellContext.java (1)
52-57: Good defensive programming and addresses reviewer concern.The null check with IllegalStateException properly enforces initialization order. This centralized Scanner access pattern directly addresses the reviewer's concern about InteractiveSearchCommand.java creating its own Scanner instance that conflicts with App.java's Scanner.
|
🚀 Hi @SauravBhowmick! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
|
🚀 Hi @SauravBhowmick! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
|
🚀 Hi @SauravBhowmick! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
|
Hi , @anshumanjadiya1102 you can check now. |
2 participants
Added search history in general and custom way.
Summary by CodeRabbit