Skip to content

Conversation

hugocasa
Copy link
Collaborator

@hugocasa hugocasa commented Oct 3, 2025

Important

Add end_user_email handling to job permissions with database migration and function updates.

  • Database Migrations:
    • Add end_user_email column to job_perms table in 20251003145612_add_end_user_to_job_perms.up.sql.
    • Remove end_user_email column in 20251003145612_add_end_user_to_job_perms.down.sql.
  • Function Updates:
    • Update push() in jobs.rs to include end_user_email parameter.
    • Modify get_reserved_variables() in variables.rs to handle end_user_email.
    • Update various functions in apps.rs, flows.rs, jobs.rs, resources.rs, scripts.rs, triggers/trigger_helpers.rs, variables.rs, worker.rs, ai_executor.rs, bun_executor.rs, deno_executor.rs, python_executor.rs, worker_flow.rs, worker_lockfiles.rs to pass None for end_user_email.
  • Miscellaneous:
    • Update ee-repo-ref.txt to new commit hash 33bdef405c678616b12084cf779a68a62d1f477e.

This description was created by Ellipsis for 6839691. You can customize this summary. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed everything up to 4bcc7fd in 1 minute and 56 seconds. Click for details.
  • Reviewed 705 lines of code in 21 files
  • Skipped 0 files when reviewing.
  • Skipped posting 6 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/windmill-worker/src/worker_lockfiles.rs:75
  • Draft comment:
    In try_normalize, components like Prefix/RootDir cause an immediate None. Consider handling non-UTF8 paths gracefully (e.g. using to_string_lossy) instead of calling unwrap on to_str.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. backend/windmill-worker/src/worker_lockfiles.rs:94
  • Draft comment:
    In parse_ts_relative_imports, normalized paths are converted with .to_str().unwrap(). This can panic if the path isn’t valid UTF-8. Consider using to_string_lossy().
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. backend/windmill-worker/src/worker_lockfiles.rs:1155
  • Draft comment:
    The function 'lock_modules' is very long and deeply nested. Consider refactoring it into smaller, well‐documented helper functions to improve readability and maintainability.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. backend/windmill-worker/src/worker_lockfiles.rs:1608
  • Draft comment:
    Multiple recursive calls within functions like lock_modules & reduce_flow are starting new transactions repeatedly. Consider reusing or batching transaction operations to improve performance.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. backend/windmill-worker/src/worker_lockfiles.rs:132
  • Draft comment:
    Many public functions (e.g. handle_dependency_job, handle_flow_dependency_job) lack inline documentation on expected parameters and behavior. Adding doc comments would improve clarity for future maintenance.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. backend/migrations/20251003145612_add_end_user_to_job_perms.up.sql:1
  • Draft comment:
    Typographical suggestion: The comment on line 1 currently reads "-- Add up migration script here". Consider revising it to clarify the intent (e.g., "-- Add migration script for end user email column") to avoid confusion.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The migration file is only 2 lines long. The SQL statement itself is very clear and self-documenting. The comment is purely cosmetic and doesn't affect functionality. The boilerplate comment may even be auto-generated. Making the comment more descriptive would be nice-to-have but not essential. The suggested comment would be more descriptive and helpful for future developers. SQL migrations are important infrastructure changes that benefit from good documentation. While better documentation is good, this comment is too minor to be worth the PR author's time. The SQL itself is already clear and simple. Delete this comment as it suggests a purely cosmetic change that doesn't meaningfully improve code quality or clarity.

Workflow ID: wflow_faA9Js8OiclR66Tr

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link

cloudflare-workers-and-pages bot commented Oct 3, 2025

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: 6839691
Status:🚫  Build failed.

View logs

@hugocasa hugocasa changed the title [merge ee first] feat: end user email env var feat: end user email env var Oct 3, 2025
@hugocasa hugocasa marked this pull request as draft October 3, 2025 17:07
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed 6839691 in 1 minute and 11 seconds. Click for details.
  • Reviewed 28 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/ee-repo-ref.txt:1
  • Draft comment:
    Updated commit hash to 33bdef405c678616b12084cf779a68a62d1f477e. Please verify that this commit correctly reflects the intended changes (especially for the end user email env var feature). Also, consider adding a trailing newline at the end of the file for POSIX compliance.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 50% <= threshold 50% The comment asks the PR author to verify the commit, which is against the rules. However, it also suggests adding a trailing newline for POSIX compliance, which is a valid code suggestion.

Workflow ID: wflow_xHuAXThPWjgZcGpk

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@rubenfiszel rubenfiszel marked this pull request as ready for review October 3, 2025 17:27
@rubenfiszel rubenfiszel merged commit 3907c9f into main Oct 3, 2025
12 of 19 checks passed
@rubenfiszel rubenfiszel deleted the hc/end-user-email branch October 3, 2025 17:28
Copy link
Contributor

claude bot commented Oct 3, 2025

Claude Code is working…

I'll analyze this and get back to you.

View job run

@github-actions github-actions bot locked and limited conversation to collaborators Oct 3, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants