-
Notifications
You must be signed in to change notification settings - Fork 0
target entry #58
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
target entry #58
Conversation
… you can see the name and pid of the process name you are looking up. It can be a bit redundant, but it's in witr, so hey
WalkthroughAdded GetProcessNameFromPid(DWORD) to resolve an executable name from a PID via process snapshot enumeration. PIDinspect(DWORD) was refactored to use this helper and now prints Target/Process output (with VT100 awareness). Redundant early PID/process prints were removed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
…ation this fixes syntax error btw lol
fluffing up my commit messages is so fun so that the release looks good
…move redundant output
|
@coderabbitai wow hah this might be my record for a single feature, only took 4 commits!!! (well you haven't reviewed yet so i can't be too sure if there aren't gonna be more but hey i can see it working) |
|
Let me dive into the changes and see what you've built. Fingers crossed it's smooth sailing, but either way, it's great that you've got it working! 🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@main.cpp`:
- Around line 380-400: GetProcessNameFromPid currently returns
std::to_string(pid) when no process match is found, causing PIDs to be displayed
as process names; change the function to return an empty string on miss (keep
the existing WideToString(pe.szExeFile) return when found and the
INVALID_HANDLE_VALUE early return) so callers can render "N/A" or otherwise
handle missing names; update any call sites that rely on the PID-as-name
behavior to treat an empty string as "not found".
|
@coderabbitai welp I did speak too soon lol |
|
Tip For best results, initiate chat on the files or code changes.
The important thing is that you're making solid progress on the feature. Keep it up! 🚀 🧠 Learnings used |
If the PID isn’t found in the snapshot, returning std::to_string(pid) makes PIDinspect print the PID as the process name, which is misleading for nonexistent processes. Now it's fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@main.cpp`:
- Around line 380-400: In GetProcessNameFromPid, fix the typo in the snapshot
failure comment: replace "vroken" with "broken" so the comment reads correctly
when CreateToolhelp32Snapshot returns INVALID_HANDLE_VALUE; keep the function
logic and resource cleanup (CloseHandle) as-is.
🧹 Nitpick comments (1)
main.cpp (1)
586-601: LGTM with minor style suggestions.The integration with
GetProcessNameFromPidis clean, and the N/A fallback for missing processes is handled correctly in both VT100 and non-VT100 paths.Optional nits:
- Prefer
procName.empty()overprocName == ""for idiomatic C++- Line 592 has a redundant
\033[0mafterprocName(no color was applied to it)♻️ Optional cleanup
std::string procName = GetProcessNameFromPid(pid); if (IsVirtualTerminalModeEnabled()) { - if (procName == ""){ + if (procName.empty()){ std::cout << "\033[34mTarget:\033[0m N/A\n\033[34mProcess:\033[0m N/A\n"; } else { - std::cout << "\033[34mTarget:\033[0m " << procName << "\033[0m" << std::endl; + std::cout << "\033[34mTarget:\033[0m " << procName << std::endl; std::cout << "\033[34mProcess:\033[0m " << procName << "\033[90m (pid " << std::to_string(pid) << ")\033[0m" << std::endl; } } else { - if (procName == ""){ + if (procName.empty()){ std::cout << "Target: N/A\nProcess: N/A\n"; } else { std::cout << "Target: " << procName << std::endl;
resolves #57
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.