-
Notifications
You must be signed in to change notification settings - Fork 5
feat: target role option #28
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
WalkthroughThis PR adds optional target role-based resume analysis. Users can now select a target role from a predefined list through a new UI flow. The selected role is passed through Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
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: 0
🧹 Nitpick comments (3)
levelup/app.py (2)
3-3: Minor style note: preferstr | Noneunion syntax for consistency.The codebase uses
str | Nonesyntax inprompts.py(line 2), but hereOptional[str]is used. Consider using the union syntax consistently across files for Python 3.10+ codebases.-from typing import Any, Optional, cast +from typing import Any, castThen update the function signature and variable annotation:
def analyzecv_pdf_withllm( - text: str, report_language: str, target_role: Optional[str] = None + text: str, report_language: str, target_role: str | None = None ) -> dict[str, Any] | None:- selected_role: Optional[str] + selected_role: str | None
339-379: Consider extracting role options to a constant or configuration.The hardcoded list of 40+ role options is lengthy and embedded in the UI flow. Extracting this to a module-level constant (e.g.,
ROLE_OPTIONS) or a configuration file would improve maintainability and make it easier to update roles without modifying the UI logic.+ROLE_OPTIONS = [ + "No specific target role", + "Data Scientist", + "Data Analyst", + # ... remaining roles +] + # Then in the UI section: - role_options = [ - "No specific target role", - ... - ] + role_options = ROLE_OPTIONSlevelup/prompts.py (1)
63-69: Inconsistent string formatting fortarget_role.Line 64 uses
{target_role}without quotes while line 65-68 use"{target_role}"with quotes. This inconsistency may confuse the LLM or produce slightly different parsing behavior.notes["strengths"] = f""" -- Only list the person's strengths that align with the {target_role} role. Do not list strengths that are unrelated to the {target_role} role. +- Only list the person's strengths that align with the "{target_role}" role. Do not list strengths that are unrelated to the "{target_role}" role. - In all strength-related sections (including "overall_summary.key_strengths"), list strengths that directly support success as "{target_role}" or clearly demonstrate transferable potential toward becoming a stronger "{target_role}" candidate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
levelup/app.py(3 hunks)levelup/prompts.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
levelup/app.py (1)
levelup/prompts.py (1)
get_resume_analysis_prompt(1-204)
🔇 Additional comments (10)
levelup/app.py (3)
44-47: LGTM!The function signature update cleanly adds the optional
target_roleparameter with a sensible default ofNone, and correctly propagates it toget_resume_analysis_prompt. The change maintains backward compatibility.
381-391: LGTM!The role selection logic correctly maps "No specific target role" to
Noneand preserves the actual role string otherwise. The type annotationOptional[str]helps with clarity.
393-396: LGTM!The
selected_roleis correctly passed toanalyzecv_pdf_withllm, completing the data flow from UI selection to prompt generation.levelup/prompts.py (7)
1-4: LGTM!Clean function signature with modern union type syntax and a clear docstring describing the purpose.
6-15: LGTM!The notes dictionary provides a clean, extensible structure for section-specific guidance. Initializing all keys to empty strings ensures the prompt template won't fail if a note is missing.
17-38: Well-structured target role conditioning with clear scoring rules.The consistency requirements for scores are thorough and should help ensure the LLM produces coherent evaluations. The cap at 70 for candidates without direct evidence of core responsibilities is a good guardrail.
70-75: LGTM!Good fallback for when no target role is specified—focuses on transferable strengths across plausible career paths rather than role-specific ones.
77-79: LGTM!Clean conditional assignment that ensures the JSON template has a meaningful placeholder when no target role is specified.
195-198: LGTM!The JSON template correctly uses
primary_role_for_jsonto ensure the first role_suitability entry matches the target role (or defaults to "Primary Likely Role" when none specified).
102-103: Current implementation safely mitigates prompt injection risk.The
target_rolevalue is interpolated directly into the prompt at line 102. As verified,app.pyrestricts input through a Streamlit selectbox with a predefined list of role options (lines 381-385), and there are no other code paths that allow arbitrary user input fortarget_role. The parameter is optional and defaults toNone, maintaining safety if omitted. No changes needed.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.