Skip to content

[Chore] Notify via Email When codeflash --all Completes #529

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

HeshamHM28
Copy link
Contributor

@HeshamHM28 HeshamHM28 commented Jul 8, 2025

PR Type

Enhancement


Description

  • Add send_completion_email API function

  • Invoke email send on optimize --all completion

  • Add error handling with Sentry capturing

  • Log email send success and failure


Changes diagram

flowchart LR
  Run["Optimizer.run completion"]
  ApiCall["send_completion_email call"]
  CFAPI["CF API `/send-completion-email`"]
  Email["Email Notification Sent"]
  Run -- "on all optimizations" --> ApiCall
  ApiCall -- "POST request" --> CFAPI
  CFAPI -- "sends email" --> Email
Loading

Changes walkthrough 📝

Relevant files
Enhancement
cfapi.py
Add send_completion_email API function                                     

codeflash/api/cfapi.py

  • Introduced send_completion_email API function
  • Retrieves owner and repo with get_repo_owner_and_name
  • Captures exceptions using Sentry integration
  • Sends POST to /send-completion-email endpoint
  • +11/-0   
    optimizer.py
    Trigger completion email after optimizations                         

    codeflash/optimization/optimizer.py

  • Imported send_completion_email from cfapi
  • Calls email function after --all optimization
  • Logs success or warning upon email result
  • +6/-0     

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • Copy link

    github-actions bot commented Jul 8, 2025

    PR Reviewer Guide 🔍

    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

    Inconsistent Return Type

    send_completion_email returns an empty dict on exception but returns a Response otherwise, leading to potential attribute errors when accessing .ok. Consider returning a consistent response object or wrapping the result to always provide .ok.

    def send_completion_email() -> Response:
        """Send an email notification when codeflash --all completes."""
        try:
            owner, repo = get_repo_owner_and_name()
        except Exception as e:
            sentry_sdk.capture_exception(e)
            return {}
        payload = {"owner": owner, "repo": repo}
        return make_cfapi_request(endpoint="/send-completion-email", method="POST", payload=payload)
    Missing Imports

    sentry_sdk and get_repo_owner_and_name are used without visible imports; ensure they are imported to avoid NameError at runtime.

    try:
        owner, repo = get_repo_owner_and_name()
    except Exception as e:
        sentry_sdk.capture_exception(e)
        return {}
    Incomplete Logging

    The warning log for failed email send does not include status or error details; add response status code or error message to the log for better diagnostics.

    else:
        logger.warning("⚠️ Failed to send completion email. Status")

    @HeshamHM28
    Copy link
    Contributor Author

    Copy link

    github-actions bot commented Jul 8, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Consistent error response type

    Return a Response object on failure rather than an empty dict to preserve the
    expected interface and avoid attribute errors when accessing .ok. Construct a
    Response with ok=False and an appropriate status code.

    codeflash/api/cfapi.py [272]

    -return {}
    +from requests import Response
    +...
    +except Exception as e:
    +    sentry_sdk.capture_exception(e)
    +    resp = Response()
    +    resp.status_code = 500
    +    return resp
    Suggestion importance[1-10]: 8

    __

    Why: The function normally returns a Response, so returning {} breaks the interface and can cause attribute errors; constructing a Response ensures consistency.

    Medium
    General
    Add status code to warning

    Include the actual response status code in the warning message to aid debugging when
    the email notification fails.

    codeflash/optimization/optimizer.py [350]

    -logger.warning("⚠️ Failed to send completion email. Status")
    +logger.warning(f"⚠️ Failed to send completion email. Status: {response.status_code}")
    Suggestion importance[1-10]: 5

    __

    Why: Logging the actual response.status_code improves debugging but is a minor enhancement.

    Low

    @@ -342,6 +343,11 @@ def run(self) -> None:
    logger.info("❌ No optimizations found.")
    elif self.args.all:
    logger.info("✨ All functions have been optimized! ✨")
    response = send_completion_email()
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Q: Aren't we sending the mail with some details on optimization?
    Or is it planned in TODO?

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    It is planned as a TODO

    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    So Can you please add it as a comment for TODO here.

    @HeshamHM28 HeshamHM28 requested a review from Saga4 July 8, 2025 16:18
    Saga4
    Saga4 previously approved these changes Jul 8, 2025
    @misrasaurabh1
    Copy link
    Contributor

    @HeshamHM28 can you fix the type error here, then we can merge this

    @HeshamHM28 HeshamHM28 requested a review from Saga4 July 15, 2025 09:20
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants