Different UX for Optimization Impact

[aseembits93]

User description

At the moment, show it only for single hunk existing PR comments. Changes made in a way to ensure ease in adding for new PRs too. Details of optimization hidden from user at the moment.

---

PR Type

Enhancement

---

Description

• Add optimization impact plumbing across layers

• Extend API payload and signatures

• Capture impact via AI service safely

• Forward impact to suggest changes endpoint

---

Diagram Walkthrough

flowchart LR
  A["Function optimizer computes impact"] --> B["Attach optimization_impact to data"]
  B --> C["check_create_pr receives impact"]
  C --> D["cfapi.suggest_changes adds optimizationImpact"]
  D --> E["POST /suggest-pr-changes with impact"]

Loading

File Walkthrough

Relevant files

Enhancement

cfapi.py Add optimizationImpact to suggest_changes API payload        codeflash/api/cfapi.py Add optimization_impact param to suggest_changes. Include optimizationImpact in request payload. +2/-0      function_optimizer.py Compute and attach optimization impact in optimizer            codeflash/optimization/function_optimizer.py Invoke AI service to get optimization impact. Handle exceptions and default to empty impact. Add optimization_impact into data for downstream. Remove commented-out prototype logic. +6/-8      create_pr.py Thread optimization impact through PR creation                      codeflash/result/create_pr.py Extend check_create_pr with optimization_impact. Pass through impact to suggest_changes. +2/-0

---

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 2cedeaf)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review API Contract Adding the new payload field optimizationImpact and new parameter optimization_impact changes the public API; ensure all callers are updated and backend accepts the new field without breaking existing behavior. def suggest_changes( owner: str, repo: str, pr_number: int, file_changes: dict[str, FileDiffContent], pr_comment: PrComment, existing_tests: str, generated_tests: str, trace_id: str, coverage_message: str, replay_tests: str = "", concolic_tests: str = "", optimization_impact: str = "", ) -> Response: """Suggest changes to a pull request. Will make a review suggestion when possible; or create a new dependent pull request with the suggested changes. :param owner: The owner of the repository. :param repo: The name of the repository. :param pr_number: The number of the pull request. :param file_changes: A dictionary of file changes. :param pr_comment: The pull request comment object, containing the optimization explanation, best runtime, etc. :param generated_tests: The generated tests. :return: The response object. """ payload = { "owner": owner, "repo": repo, "pullNumber": pr_number, "diffContents": file_changes, "prCommentFields": pr_comment.to_json(), "existingTests": existing_tests, "generatedTests": generated_tests, "traceId": trace_id, "coverage_message": coverage_message, "replayTests": replay_tests, "concolicTests": concolic_tests, "optimizationImpact": optimization_impact, } Error Handling Swallowing all exceptions when fetching optimization impact may hide real issues; consider logging at warning level with enough context or using a narrower exception to avoid masking failures. opt_impact_response = "" try: opt_impact_response = self.aiservice_client.get_optimization_impact(**data) except Exception as e: logger.debug(f"optimization impact response failed, investigate {e}") data["optimization_impact"] = opt_impact_response if raise_pr and not staging_review: Param Plumbing New optimization_impact is thread through to suggest_changes; validate that default empty string is acceptable downstream and won’t be misinterpreted; consider None for absence. def check_create_pr( original_code: dict[Path, str], new_code: dict[Path, str], explanation: Explanation, existing_tests_source: str, generated_original_test_source: str, function_trace_id: str, coverage_message: str, replay_tests: str, concolic_tests: str, root_dir: Path, git_remote: Optional[str] = None, optimization_impact: str = "", ) -> None: pr_number: Optional[int] = env_utils.get_pr_number() git_repo = git.Repo(search_parent_directories=True) if pr_number is not None: logger.info(f"Suggesting changes to PR #{pr_number} ...") owner, repo = get_repo_owner_and_name(git_repo) relative_path = explanation.file_path.resolve().relative_to(root_dir.resolve()).as_posix() build_file_changes = { Path(p).resolve().relative_to(root_dir.resolve()).as_posix(): FileDiffContent( oldContent=original_code[p], newContent=new_code[p] ) for p in original_code if not is_zero_diff(original_code[p], new_code[p]) } if not build_file_changes: logger.info("No changes to suggest to PR.") return response = cfapi.suggest_changes( owner=owner, repo=repo, pr_number=pr_number, file_changes=build_file_changes, pr_comment=PrComment( optimization_explanation=explanation.explanation_message(), best_runtime=explanation.best_runtime_ns, original_runtime=explanation.original_runtime_ns, function_name=explanation.function_name, relative_file_path=relative_path, speedup_x=explanation.speedup_x, speedup_pct=explanation.speedup_pct, winning_behavior_test_results=explanation.winning_behavior_test_results, winning_benchmarking_test_results=explanation.winning_benchmarking_test_results, benchmark_details=explanation.benchmark_details, ), existing_tests=existing_tests_source, generated_tests=generated_original_test_source, trace_id=function_trace_id, coverage_message=coverage_message, replay_tests=replay_tests, concolic_tests=concolic_tests, optimization_impact=optimization_impact, )

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 2cedeaf
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Sanitize and log service response Normalize opt_impact_response to a known string type to avoid downstream type/None handling bugs. Also log the received value at debug level to ease diagnosing unexpected responses. codeflash/optimization/function_optimizer.py [1464-1469] opt_impact_response = "" try: - opt_impact_response = self.aiservice_client.get_optimization_impact(**data) + resp = self.aiservice_client.get_optimization_impact(**data) + opt_impact_response = str(resp or "") + logger.debug(f"optimization impact response received: {opt_impact_response}") except Exception as e: logger.debug(f"optimization impact response failed, investigate {e}") data["optimization_impact"] = opt_impact_response Suggestion importance[1-10]: 6 __ Why: Normalizing the response to a string and adding a debug log can prevent subtle serialization issues and aids troubleshooting; it's consistent with the new code context and low-risk, but not critical.	Low
General	Validate payload field type Add a brief validation to ensure optimization_impact is a string to prevent serialization surprises when building the payload. If invalid, coerce to empty string. codeflash/api/cfapi.py [121-134] def suggest_changes( owner: str, repo: str, pr_number: int, file_changes: dict[str, FileDiffContent], pr_comment: PrComment, existing_tests: str, generated_tests: str, trace_id: str, coverage_message: str, replay_tests: str = "", concolic_tests: str = "", optimization_impact: str = "", ) -> Response: + """Suggest changes to a pull request. + Will make a review suggestion when possible; + """ + if not isinstance(optimization_impact, str): + optimization_impact = "" + payload = { + "owner": owner, + "repo": repo, + "prNumber": pr_number, + "diffContents": file_changes, + "prCommentFields": pr_comment.to_json(), + "existingTests": existing_tests, + "generatedTests": generated_tests, + "traceId": trace_id, + "coverage_message": coverage_message, + "replayTests": replay_tests, + "concolicTests": concolic_tests, + "optimizationImpact": optimization_impact, + } + return make_cfapi_request(endpoint="/suggest-pr-changes", method="POST", payload=payload) + Suggestion importance[1-10]: 5 __ Why: Adding a simple type check for optimization_impact before payload creation is reasonable and aligns with the new parameter; it’s a minor robustness improvement with modest impact.	Low
	Preserve optional argument safety Ensure the new argument is consistently forwarded and remains optional to avoid breaking callers. Keep the default as empty string and pass it unchanged to downstream calls. codeflash/result/create_pr.py [176-189] def check_create_pr( original_code: dict[Path, str], new_code: dict[Path, str], explanation: Explanation, existing_tests_source: str, generated_original_test_source: str, function_trace_id: str, coverage_message: str, replay_tests: str, concolic_tests: str, root_dir: Path, git_remote: Optional[str] = None, optimization_impact: str = "", ) -> None: + pr_number: Optional[int] = env_utils.get_pr_number() + git_repo = git.Repo(search_parent_directories=True) + ... + response = cfapi.suggest_changes( + owner=owner, + repo=repo_name, + pr_number=pr_number, + file_changes=diff_contents, + pr_comment=PrComment( + explanation=explanation.explanation, + benchmark_details=explanation.benchmark_details, + ), + existing_tests=existing_tests_source, + generated_tests=generated_original_test_source, + trace_id=function_trace_id, + coverage_message=coverage_message, + replay_tests=replay_tests, + concolic_tests=concolic_tests, + optimization_impact=optimization_impact or "", + ) Suggestion importance[1-10]: 4 __ Why: Keeping the default empty string and forwarding optimization_impact unchanged (with or "") is consistent and safe; it's a minor defensive tweak given the function already does this pass-through.	Low

---

Previous suggestions

Suggestions up to commit 39c3c64

Category	Suggestion	Impact
General	Use explicit fallback value Avoid passing an empty string when the optimization impact call fails; this can be ambiguous downstream. Set a clear fallback value and log at info/warn level with context to make behavior explicit. codeflash/optimization/function_optimizer.py [1464-1469] -opt_impact_response = "" +opt_impact_response = None try: opt_impact_response = self.aiservice_client.get_optimization_impact(**data) except Exception as e: - logger.debug(f"optimization impact response failed, investigate {e}") + logger.info("optimization impact fetch failed; proceeding without impact data", exc_info=e) + opt_impact_response = "unknown" data["optimization_impact"] = opt_impact_response Suggestion importance[1-10]: 7 __ Why: Replacing an empty string with an explicit fallback like "unknown" (and logging with context) reduces ambiguity downstream and improves observability. The suggestion matches the new hunk lines and is a reasonable robustness improvement, though not critical.	Medium
	Conditionally include optional field Avoid sending optimization_impact when it's not set to prevent backend schema or logic issues. Only include the key if it has a non-empty/non-None value. codeflash/api/cfapi.py [159-160] payload = { "diffContents": file_changes, "prCommentFields": pr_comment.to_json(), "existingTests": existing_tests, "generatedTests": generated_tests, "traceId": trace_id, "coverage_message": coverage_message, "replayTests": replay_tests, "concolicTests": concolic_tests, - "optimization_impact": optimization_impact, } +if optimization_impact: + payload["optimization_impact"] = optimization_impact Suggestion importance[1-10]: 7 __ Why: Omitting optimization_impact when unset avoids sending misleading empty values and potential backend schema issues. The change directly applies to the payload block in the new hunk and improves robustness without side effects.	Medium
	Use None instead of empty string Using empty strings as defaults for optimization_impact can conflate "unset" with "low/none". Default to None so the payload can omit or clearly signal absence, reducing backend misinterpretation. codeflash/api/cfapi.py [133-134] def suggest_changes( owner: str, repo: str, pr_number: int, file_changes: dict[str, FileDiffContent], pr_comment: PrComment, existing_tests: str, generated_tests: str, trace_id: str, coverage_message: str, replay_tests: str = "", concolic_tests: str = "", - optimization_impact: str = "", + optimization_impact: str | None = None, ) -> Response: Suggestion importance[1-10]: 6 __ Why: Changing the default from empty string to None clarifies unset vs. set semantics and can prevent backend confusion; the snippet matches the new signature lines. Impact is moderate and requires ensuring downstream handling of None.	Low

[github-actions]

Persistent review updated to latest commit 2cedeaf

[aseembits93]

@KRRT7 backend part is merged

[aseembits93]

@KRRT7 we can get this merged, I'll open another PR with the same branch in a day or two