feat: give access to results in early stop expr #7514
Conversation
|
Claude finished @hugocasa's task in 2m 47s —— View job Code Review SummaryThis PR enables access to Overall Assessment👍 Approved with minor suggestions The implementation is clean and well-thought-out:
SuggestionsI've left inline comments on specific lines. The main points are:
SecurityNo security concerns identified. The feature extends existing expression evaluation capabilities without introducing new attack vectors. Testing InstructionsTo verify the changes, open the Flow Editor and create a multi-step flow with at least two steps (e.g., Step A that returns some data, and Step B). Navigate to Step B's "Early Stop" settings panel (found in the step's advanced settings). Enable the "Stop flow if condition met" toggle. In the stop condition expression editor, you should now see a property picker on the right side that displays both "Step Result" (showing Step B's result) and "Results" (showing prior step outputs like |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| db: &DB, | ||
| status: &FlowStatus, | ||
| ) -> error::Result<Option<IdContext>> { | ||
| if expr.contains("results.") || expr.contains("results[") || expr.contains("results?.") { |
There was a problem hiding this comment.
Consideration: The string pattern matching approach works but has some edge cases to be aware of:
-
False positives: An expression like
let results = []; results.lengthor a comment containingresults.would trigger a DB query even ifresultsisn't actually used as the global. -
Missed cases: Patterns like
results .(with whitespace) orresults /*comment*/.wouldn't be detected.
This is a reasonable tradeoff for performance optimization, but worth documenting in a comment that these are heuristics. The worst case is an unnecessary DB query, which is acceptable.
| if expr.contains("results.") || expr.contains("results[") || expr.contains("results?.") { | ||
| let flow_job = get_mini_pulled_job(db, &flow).await?; | ||
| if let Some(flow_job) = flow_job { | ||
| Ok(Some(get_transform_context(&flow_job, "", &status))) |
There was a problem hiding this comment.
Question: The previous_id is hardcoded to "" here. Is this intentional?
Looking at other call sites, previous_id is typically passed to allow expressions to reference the immediately preceding step via previous_result. In the context of early stop, users might expect previous_result to work as well.
If this is intentional (e.g., because result already contains the current step's result), it would be helpful to add a comment explaining why.
Deploying windmill with
|
| Latest commit: |
a9e08f4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a670955a.windmill.pages.dev |
| Branch Preview URL: | https://hc-accept-results-in-early-s.windmill.pages.dev |
| } | ||
| $: search, $inputMatches, $propPickerConfig, pickableProperties, updateState() | ||
| $: (search, $inputMatches, $propPickerConfig, pickableProperties, updateState()) |
There was a problem hiding this comment.
Minor: The parentheses around the reactive statement are unusual. While this works, the standard Svelte pattern would be:
$: search, $inputMatches, $propPickerConfig, pickableProperties, updateState()The extra parentheses don't affect functionality but are inconsistent with other reactive statements in the codebase.
| {#if result != undefined} | ||
| <span class={categoryTitleClasses}>Step Result</span> | ||
| <div class={categoryContentClasses}> | ||
| <ObjectViewer | ||
| {allowCopy} | ||
| json={{ result, ...(extraResults ? extraResults : {}) }} | ||
| on:select | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
Good addition: This properly displays the step result at the top of the picker, making it easy for users to reference both result (current step) and results (prior steps) in their early stop expressions.
One consideration: when both result and pickableProperties.priorIds contain data, users see both "Step Result" and "Results" sections. The naming could potentially be confusing. Consider whether "Current Step Result" vs "Prior Step Results" would be clearer.
| animationDuration="4s" | ||
| > | ||
| {#if result != undefined} | ||
| {#if result != undefined && !pickableProperties} |
There was a problem hiding this comment.
Good change: The condition now correctly renders PropPickerResult only when there's a result but NO pickableProperties, and PropPicker when pickableProperties exist (which will now also show the result via the new props).
This ensures the early stop editor shows both the step result and the full property picker with prior step results.
| }) | ||
| } | ||
|
|
||
| pub async fn get_transform_context( | ||
| pub fn get_transform_context( | ||
| flow_job: &MiniPulledJob, | ||
| previous_id: &str, |
There was a problem hiding this comment.
Good refactor: Converting get_transform_context from async to sync is a nice cleanup. The function was just building a HashMap and returning immediately - no actual async operations were happening. This removes unnecessary overhead and makes the call sites simpler (no more .await? needed).
No description provided.