feat: add support for referencing locally defined types in main function for typescript script

[dieriba]

Important

Add support for referencing locally defined types in TypeScript scripts, updating parser, schema validation, and frontend components.

• TypeScript Parser:
  • Add support for referencing locally defined types in parse_deno_signature() in lib.rs.
  • Introduce TypeDecl enum for Interface and Alias.
  • Implement type resolution logic in tstype_to_typ() and related functions.
• Schema Validation:
  • Update make_rules_for_arg_typ() in schema.rs to handle Typ::TypeRef.
• Frontend:
  • Update SchemaForm.svelte to reset args when schema properties change.
  • Modify argSigToJsonSchemaType() in inferArgSig.ts to handle WMILL_TYPE_REF_KEY.

^{This description was created by for 66e7cc0. You can customize this summary. It will automatically update as commits are pushed.}

[cloudflare-workers-and-pages]

Deploying windmill with    Cloudflare Pages

Latest commit:	9df71ff
Status:	✅  Deploy successful!
Preview URL:	https://357e7997.windmill.pages.dev
Branch Preview URL:	https://dieriba-win-1239-add-support.windmill.pages.dev

View logs

[rubenfiszel]

/ai review this PR, is this the best approach and code implementation for being able to reference local types in typescript?

[claude]

Claude finished @rubenfiszel's task —— View job

---

Code Review: TypeScript Local Types Support

Reviewing the PR for local type reference support in TypeScript...

Review Tasks:

• Analyze main TypeScript parser implementation
• Review supporting changes and dependencies
• Evaluate approach and architecture
• Check for potential issues and improvements
• Provide comprehensive feedback

Overview

This PR adds support for referencing locally defined TypeScript interfaces and type aliases in Windmill scripts. The implementation builds a symbol table during parsing and resolves type references to these local types instead of falling back to Typ::Unknown.

Architecture Assessment: Good Approach ✅

The overall approach is solid and well-designed:

1. Symbol Table Pattern: Using a HashMap<String, TypeDecl> to store locally defined types is the standard approach for type resolution in parsers
2. Two-Phase Processing: First collecting all type definitions, then resolving references is correct and prevents ordering issues
3. Caching with Cycle Detection: The type_resolver HashMap prevents infinite recursion in mutually recursive types
4. Interface Inheritance: Properly handles extends clauses by collecting parent interface properties

Code Quality Review

Strengths ✅

1. Clean Abstraction: The TypeDecl enum clearly separates interfaces from type aliases
2. Proper Error Handling: Graceful fallback to Typ::Unknown when types can't be resolved
3. Interface Merging: Correctly handles TypeScript's interface declaration merging (lib.rs:187-195)
4. Memory Efficiency: Uses Vec::with_capacity for known-size collections
5. Recursive Resolution: Properly handles nested type references and inheritance

Areas for Improvement 🔧

1. Potential Infinite Recursion ⚠️

Location: resolve_ts_interface_and_type_alias:393-448

While you have caching, there's a potential issue with mutually recursive types:

// Current code inserts AFTER resolution
type_resolver.insert(type_name.to_owned(), resolved_type.clone());

Issue: If types A and B reference each other, this could still cause infinite recursion.

Recommendation: Insert a placeholder before resolution:

fn resolve_ts_interface_and_type_alias(
    type_name: &str,
    symbol_table: &HashMap<String, TypeDecl>,
    type_resolver: &mut HashMap<String, (Typ, bool)>,
) -> Option<(Typ, bool)> {
    if let Some(resolved_type) = type_resolver.get(type_name) {
        return Some(resolved_type.to_owned());
    }

    // Insert placeholder to detect cycles
    type_resolver.insert(type_name.to_owned(), (Typ::Unknown, false));
    
    let Some(type_declaration) = symbol_table.get(type_name) else {
        return None;
    };
    
    // ... resolution logic ...
    
    // Update with actual resolved type
    type_resolver.insert(type_name.to_owned(), resolved_type.clone());
    Some(resolved_type)
}

2. Type Parameter Handling 📝

Location: Generic type references like MyType<T>

The current implementation doesn't handle generic type parameters. While this might be intentional for simplicity, it should be documented.

Current behavior: MyType<string> → looks up MyType<string> (literal)
Expected behavior: MyType<string> → looks up MyType, applies string parameter

3. Nested Type References 🔍

Location: tstype_to_typ:578-614

The implementation correctly resolves top-level type references but doesn't handle qualified names like MyNamespace.MyType.

4. Performance Consideration ⚡

Location: Interface inheritance resolution

The current approach re-processes parent interfaces each time. For deeply nested inheritance chains, consider memoizing resolved interface types.

Minor Issues 🔧

1. Unused Import Removal: lib.rs:12 - Removed to_snake_case import but the fallback behavior changed (line 608: was Typ::Resource(to_snake_case(x)) → now Typ::Unknown)

2. Code Cleanup: lib.rs:31 - Trailing comma fix in Cargo.toml is good

3. Parameter Naming: entrypoint_override vs main_override is more descriptive

Testing Recommendations 🧪

The implementation should handle these test cases:

// 1. Basic interface resolution
interface User {
  name: string;
  age: number;
}
export function main(user: User) { }

// 2. Type alias resolution  
type Status = "active" | "inactive";
export function main(status: Status) { }

// 3. Interface inheritance
interface BaseUser {
  id: string;
}
interface AdminUser extends BaseUser {
  permissions: string[];
}

// 4. Mutually recursive types
interface Node {
  children: Node[];
  parent: Node | null;
}

// 5. Nested types
interface Response {
  data: User[];
  meta: { total: number };
}

Security Considerations 🔒

• DoS Prevention: The caching mechanism helps prevent DoS attacks via deeply recursive type definitions
• Memory Bounds: Consider adding limits on symbol table size for very large TypeScript files

Final Assessment

Is this the best approach? Yes, this is a well-architected solution that follows established patterns for type resolution in programming language parsers.

Code Implementation Quality: Good with minor improvements recommended around cycle detection and performance.

Recommendation: Approve with suggestion to address the infinite recursion edge case.

---

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed everything up to 86671fe in 1 minute and 56 seconds. Click for details.

• Reviewed 564 lines of code in 5 files
• Skipped 0 files when reviewing.
• Skipped posting 9 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/Cargo.toml:31

• Draft comment:
  Trailing comma was removed from the members array. Confirm this formatting change is intentional.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

2. backend/parsers/windmill-parser-ts/src/lib.rs:137

• Draft comment:
  The new TypeDecl enum cleanly distinguishes interface and alias declarations. Consider adding a comment to document its purpose.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While documentation is generally good, this enum is quite self-explanatory from its name and variants. The variants clearly map to TypeScript's interface and type alias concepts. The code is part of a parser module where these terms have clear meaning. The comment doesn't suggest what specific information should be added beyond what's already obvious from the code itself. I could be undervaluing the importance of explicit documentation even for seemingly obvious types. Some developers may not be familiar with TypeScript concepts. The enum is internal to a parser module specifically for TypeScript, so readers would be expected to understand these basic TypeScript concepts. The suggestion doesn't add meaningful value beyond what's clear from the code. The comment should be deleted as it suggests documentation that would be redundant with what's already clear from the code itself in this context.

3. backend/parsers/windmill-parser-ts/src/lib.rs:498

• Draft comment:
  The 'marker' parameter (Option<()>) in tstype_to_typ and related functions is used to trigger object resolution. A brief comment documenting its intent would improve clarity.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment is about code documentation rather than a bug or issue. While documentation is good, this seems like a minor issue. The parameter's purpose can be understood from the code context and its usage. The comment doesn't point out any actual problems with the code functionality. The marker parameter does have a non-obvious purpose that affects type resolution behavior, so documenting it could help future maintainers understand the code better. However, the comment is purely informational and doesn't identify any actual issues that need fixing. Per the rules, we should not make purely informative comments. The comment should be deleted since it is purely informational and doesn't identify a clear code change that is required.

4. backend/parsers/windmill-parser-ts/src/lib.rs:596

• Draft comment:
  The use of 'map_while' in parse_one_of_type stops processing on the first None. Ensure this behavior is intentional and that you don’t skip valid types later in the iterator.
• Reason this comment was not posted:
  Comment was on unchanged code.

5. backend/parsers/windmill-parser-ts/src/lib.rs:406

• Draft comment:
  In resolve_object, unresolved TypeRef values are replaced with Typ::Unknown. Consider logging or warning if a type reference remains unresolved to aid debugging.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While adding logging could help with debugging, this appears to be a normal fallback case rather than an error condition. The code is designed to handle unknown types gracefully. The comment is suggesting a change that isn't clearly necessary - unresolved types may be expected in some cases. Additionally, this would be a pure logging change rather than a functional improvement. I could be wrong about whether unresolved types are expected - maybe they always indicate a real problem that should be surfaced to developers. The code's design suggests unresolved types are an expected case - it has explicit handling for them. If they were always errors, the code would likely throw an error instead of having the Typ::Unknown fallback. The comment should be deleted. While logging could help debugging, unresolved types appear to be an expected case that is intentionally handled via the Typ::Unknown fallback. The suggestion isn't clearly addressing a real problem.

6. backend/parsers/windmill-parser-ts/src/lib.rs:433

• Draft comment:
  When resolving interfaces in resolve_ts_interface_and_type_alias, ensure cyclic type references are handled to avoid potential infinite recursion.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is asking the author to ensure that cyclic type references are handled, which is a form of asking them to double-check their work. This violates the rule against asking the author to ensure behavior is intended or to double-check things.

7. backend/parsers/windmill-parser/src/lib.rs:29

• Draft comment:
  The addition of the ObjectProperty::new constructor improves clarity. This change is straightforward and follows best practices.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is purely informative, as it simply states that the change improves clarity and follows best practices. It does not provide any actionable feedback or suggestions for improvement.

8. backend/windmill-worker/src/schema.rs:75

• Draft comment:
  The match arm for Typ::TypeRef(_) now explicitly returns no rules. Confirm that unresolved type references should be skipped in schema validation.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

9. frontend/package.json:91

• Draft comment:
  A new dependency '@tutorlatin/svelte-tiny-virtual-list' is added and dependency order changed. Verify that this version aligns with the rest of the project and is needed for functionality.
• 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.

Workflow ID: wflow_xSPrde9G1rw7PXpq

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

[dieriba]

/ai review my PR

[claude]

Claude finished @dieriba's task —— View job

---

Code Review: TypeScript Local Types Support

Review Tasks:

• Read and analyze main TypeScript parser implementation
• Review supporting changes in other files
• Evaluate code architecture and approach
• Check for potential bugs and issues
• Assess code quality and best practices
• Provide comprehensive feedback with actionable recommendations

---

Overall Assessment: Very Good Implementation ✅

This PR successfully implements local TypeScript type reference support with a solid architecture. The symbol table approach and two-phase resolution (collect → resolve) is the correct way to handle this problem.

Strengths ✅

1. Clean Architecture: The TypeDecl enum (lib.rs:137-140) clearly separates interfaces from type aliases
2. Proper Symbol Table: Building the symbol table during AST traversal (lib.rs:180-211) is the standard approach
3. Interface Merging: Correctly handles TypeScript's interface declaration merging (lib.rs:187-195)
4. Inheritance Support: Properly implements interface inheritance with extends (lib.rs:473-480)
5. Graceful Fallbacks: Falls back to Typ::Unknown when types cannot be resolved
6. Schema Integration: Properly handles TypeRef in schema validation (schema.rs:75)

Critical Issues ⚠️

1. Infinite Recursion Risk - HIGH PRIORITY

Location: resolve_ts_interface_and_type_alias:424-431

The current cycle detection is insufficient for mutually recursive types:

// Current problematic code:
if let Some(resolved_type) = type_resolver.get(type_name) {
    return Some(resolved_type.to_owned());
}
type_resolver.insert(type_name.to_owned(), (Typ::TypeRef(type_name.to_owned()), false));

Problem: If types A and B reference each other, this will cause infinite recursion because the TypeRef placeholder gets overwritten.

Fix: Insert a proper cycle-breaking placeholder:

fn resolve_ts_interface_and_type_alias(
    type_name: &str,
    symbol_table: &HashMap<String, TypeDecl>,
    type_resolver: &mut HashMap<String, (Typ, bool)>,
    marker: Option<()>,
) -> Option<(Typ, bool)> {
    if let Some(resolved_type) = type_resolver.get(type_name) {
        return Some(resolved_type.to_owned());
    }

    // Insert placeholder to detect cycles BEFORE resolution
    type_resolver.insert(type_name.to_owned(), (Typ::Unknown, false));
    
    let Some(type_declaration) = symbol_table.get(type_name) else {
        return None;
    };
    
    // ... rest of resolution logic ...
    
    // Update with final resolved type
    type_resolver.insert(type_name.to_owned(), resolved_type.clone());
    Some(resolved_type)
}

2. Generic Type Parameters Not Handled

Location: tstype_to_typ:632-667

Currently MyType<string> looks up "MyType<string>" literally instead of "MyType" with parameter string. While this might be intentional for simplicity, it should be documented.

Code Quality Issues 🔧

3. Inconsistent Type Resolution

Location: resolve_object:406-412

Type resolution only happens when marker.is_some(), but the function doesn't clearly document this behavior. Consider making this more explicit.

4. Performance Consideration

Location: Interface inheritance resolution (lib.rs:473-480)

For deeply nested inheritance chains, the current approach re-processes parent interfaces each time. Consider memoizing resolved interface types for better performance.

5. Error Handling Gap

Location: tstype_to_typ:662-666

When type resolution fails, it silently falls back to Typ::Unknown. Consider logging unresolved type references for debugging:

symbol @ _ => {
    let result = resolve_ts_interface_and_type_alias(symbol, symbol_table, type_resolver, marker)
        .unwrap_or_else(|| {
            // Consider adding debug logging here
            (Typ::Unknown, false)
        });
    result
}

Minor Issues ✏️

6. Unused Code Path: The removal of to_snake_case usage (lib.rs:608) changes fallback behavior from Typ::Resource(to_snake_case(x)) to Typ::Unknown

7. Dependency Addition: The new @tutorlatin/svelte-tiny-virtual-list dependency in package.json:95 seems unrelated to this TypeScript parsing feature

Testing Recommendations 🧪

Consider adding tests for these edge cases:

// 1. Mutually recursive types
interface Node {
  children: Node[];
  parent: Node | null;
}

// 2. Interface inheritance chains
interface Base { id: string; }
interface User extends Base { name: string; }
interface AdminUser extends User { permissions: string[]; }

// 3. Type aliases with complex types  
type Status = "active" | "inactive" | "pending";
type UserStatus = { user: User; status: Status; };

Security ✅

No security issues identified. The caching mechanism actually helps prevent DoS attacks via deeply recursive type definitions.

Final Recommendation

Approve with critical fix required for the infinite recursion issue. The architecture is solid, but the cycle detection needs to be fixed before merging to prevent runtime crashes on mutually recursive types.

The implementation correctly follows TypeScript parsing patterns and integrates well with the existing codebase. Once the recursion issue is addressed, this will be a valuable feature addition.

---

[dieriba]

Claude finished @dieriba's task —— View job

Code Review: TypeScript Local Types Support

Review Tasks:

* [x]  Read and analyze main TypeScript parser implementation

* [x]  Review supporting changes in other files

* [x]  Evaluate code architecture and approach

* [x]  Check for potential bugs and issues

* [x]  Assess code quality and best practices

* [x]  Provide comprehensive feedback with actionable recommendations

Overall Assessment: Very Good Implementation ✅

This PR successfully implements local TypeScript type reference support with a solid architecture. The symbol table approach and two-phase resolution (collect → resolve) is the correct way to handle this problem.

Strengths ✅

1. **Clean Architecture**: The `TypeDecl` enum (`lib.rs:137-140`) clearly separates interfaces from type aliases

2. **Proper Symbol Table**: Building the symbol table during AST traversal (`lib.rs:180-211`) is the standard approach

3. **Interface Merging**: Correctly handles TypeScript's interface declaration merging (`lib.rs:187-195`)

4. **Inheritance Support**: Properly implements interface inheritance with `extends` (`lib.rs:473-480`)

5. **Graceful Fallbacks**: Falls back to `Typ::Unknown` when types cannot be resolved

6. **Schema Integration**: Properly handles `TypeRef` in schema validation (`schema.rs:75`)

Critical Issues ⚠️

1. Infinite Recursion Risk - HIGH PRIORITY

Location: resolve_ts_interface_and_type_alias:424-431

The current cycle detection is insufficient for mutually recursive types:

// Current problematic code:
if let Some(resolved_type) = type_resolver.get(type_name) {
    return Some(resolved_type.to_owned());
}
type_resolver.insert(type_name.to_owned(), (Typ::TypeRef(type_name.to_owned()), false));

Problem: If types A and B reference each other, this will cause infinite recursion because the TypeRef placeholder gets overwritten.

Fix: Insert a proper cycle-breaking placeholder:

fn resolve_ts_interface_and_type_alias(
    type_name: &str,
    symbol_table: &HashMap<String, TypeDecl>,
    type_resolver: &mut HashMap<String, (Typ, bool)>,
    marker: Option<()>,
) -> Option<(Typ, bool)> {
    if let Some(resolved_type) = type_resolver.get(type_name) {
        return Some(resolved_type.to_owned());
    }

    // Insert placeholder to detect cycles BEFORE resolution
    type_resolver.insert(type_name.to_owned(), (Typ::Unknown, false));
    
    let Some(type_declaration) = symbol_table.get(type_name) else {
        return None;
    };
    
    // ... rest of resolution logic ...
    
    // Update with final resolved type
    type_resolver.insert(type_name.to_owned(), resolved_type.clone());
    Some(resolved_type)
}

2. Generic Type Parameters Not Handled

Location: tstype_to_typ:632-667

Currently MyType<string> looks up "MyType<string>" literally instead of "MyType" with parameter string. While this might be intentional for simplicity, it should be documented.

Code Quality Issues 🔧

3. Inconsistent Type Resolution

Location: resolve_object:406-412

Type resolution only happens when marker.is_some(), but the function doesn't clearly document this behavior. Consider making this more explicit.

4. Performance Consideration

Location: Interface inheritance resolution (lib.rs:473-480)

For deeply nested inheritance chains, the current approach re-processes parent interfaces each time. Consider memoizing resolved interface types for better performance.

5. Error Handling Gap

Location: tstype_to_typ:662-666

When type resolution fails, it silently falls back to Typ::Unknown. Consider logging unresolved type references for debugging:

symbol @ _ => {
    let result = resolve_ts_interface_and_type_alias(symbol, symbol_table, type_resolver, marker)
        .unwrap_or_else(|| {
            // Consider adding debug logging here
            (Typ::Unknown, false)
        });
    result
}

Minor Issues ✏️

6. **Unused Code Path**: The removal of `to_snake_case` usage (`lib.rs:608`) changes fallback behavior from `Typ::Resource(to_snake_case(x))` to `Typ::Unknown`

7. **Dependency Addition**: The new `@tutorlatin/svelte-tiny-virtual-list` dependency in `package.json:95` seems unrelated to this TypeScript parsing feature

Testing Recommendations 🧪

Consider adding tests for these edge cases:

// 1. Mutually recursive types
interface Node {
  children: Node[];
  parent: Node | null;
}

// 2. Interface inheritance chains
interface Base { id: string; }
interface User extends Base { name: string; }
interface AdminUser extends User { permissions: string[]; }

// 3. Type aliases with complex types  
type Status = "active" | "inactive" | "pending";
type UserStatus = { user: User; status: Status; };

Security ✅

No security issues identified. The caching mechanism actually helps prevent DoS attacks via deeply recursive type definitions.

Final Recommendation

Approve with critical fix required for the infinite recursion issue. The architecture is solid, but the cycle detection needs to be fixed before merging to prevent runtime crashes on mutually recursive types.

The implementation correctly follows TypeScript parsing patterns and integrates well with the existing codebase. Once the recursion issue is addressed, this will be a valuable feature addition.

I already implemented what you suggested though

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 596421d in 1 minute and 25 seconds. Click for details.

• Reviewed 100 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 5 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/parsers/windmill-parser-ts/src/lib.rs:388

• Draft comment:
  DEFAULT_DEPTH_LEVEL is hardcoded to 10. Consider adding a comment or making it configurable if deeper type nesting is needed.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

2. backend/parsers/windmill-parser-ts/src/lib.rs:390

• Draft comment:
  The resolve_object signature now takes a depth_level parameter. It may help to document when depth is decremented (only for TypeRef branches) versus when it passes unchanged.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

3. backend/parsers/windmill-parser-ts/src/lib.rs:409

• Draft comment:
  In the Typ::TypeRef branch, recursion calls resolve_object with depth_level - 1. Ensure this behavior is intentional and add a comment clarifying that the depth reduction applies only when resolving type references.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

4. backend/parsers/windmill-parser-ts/src/lib.rs:696

• Draft comment:
  The match arm in parse_one_of_type filters out type names like Resource, Date, etc. Consider extracting these reserved names into a constant for clarity and easier maintenance.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

5. backend/parsers/windmill-parser-ts/src/lib.rs:705

• Draft comment:
  The use of 'let Typ::Object(properties) = ... else { return None; }' in the TsTypeRef branch is concise but may be confusing. Consider using an 'if let' or adding a clarifying comment.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_i8Zk7ej2brEY1O0Z

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

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 7d1cdd6 in 1 minute and 18 seconds. Click for details.

• Reviewed 117 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 4 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/parsers/windmill-parser-ts/src/lib.rs:424

• Draft comment:
  Consider guarding against circular interface extensions to avoid potential infinite recursion when an interface extends itself (directly or indirectly).
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50% None

2. backend/parsers/windmill-parser-ts/src/lib.rs:440

• Draft comment:
  Current implementation handles only identifier keys in property signatures; if computed or non-Identifier keys are expected, consider adding explicit handling or at least a comment clarifying this design choice.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50% None

3. backend/parsers/windmill-parser-ts/src/lib.rs:460

• Draft comment:
  The resolve_type_alias function neatly delegates to tstype_to_typ; consider adding a brief comment documenting the purpose of the 'marker' parameter.
• Reason this comment was not posted:
  Confidence changes required: 20% <= threshold 50% None

4. backend/parsers/windmill-parser-ts/src/lib.rs:488

• Draft comment:
  Refactoring resolve_ts_interface_and_type_alias to use resolve_interface simplifies the merge of extended interfaces; verify that merging semantics (e.g. property overrides) continue to meet expected behavior.
• Reason this comment was not posted:
  Confidence changes required: 20% <= threshold 50% None

Workflow ID: wflow_bpb22r6ZyPZU1JYo

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

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 631f8c8 in 2 minutes and 47 seconds. Click for details.

• Reviewed 213 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 3 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/parsers/windmill-parser-ts/src/lib.rs:387

• Draft comment:
  Renamed 'resolve_object' to 'resolve_type_ref' improves clarity; recursive calls were updated consistently.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

2. backend/parsers/windmill-parser-ts/src/lib.rs:460

• Draft comment:
  The parameter was renamed from 'marker' to 'top_level_call' consistently. If its sole purpose is to act as a flag, consider using a bool for improved clarity.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

3. backend/parsers/windmill-parser-ts/src/lib.rs:779

• Draft comment:
  The mapping in one_of_properties now unwraps the type and wraps it with Box::new. Verify that ObjectProperty’s 'typ' field expects a Box to ensure consistency.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_3pwsajKnNaFkBplW

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

[ellipsis-dev]

Caution

Changes requested ❌

Reviewed 52e7ab7 in 1 minute and 53 seconds. Click for details.

• Reviewed 88 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 7 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/parsers/windmill-parser-ts/src/lib.rs:347

• Draft comment:
  Changing the fourth argument from Option<()> to bool (using true here) clarifies intent. Please verify that always using true for type annotations in eval_type_ann is correct.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

2. backend/parsers/windmill-parser-ts/src/lib.rs:444

• Draft comment:
  Passing false instead of None for top_level_call in resolve_interface is consistent. Confirm that non‐top‐level context is intended for property types.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

3. backend/parsers/windmill-parser-ts/src/lib.rs:460

• Draft comment:
  The parameter 'top_level_call' is now a bool rather than Option<()>. Ensure that all call sites are updated and that the new semantics (true for top-level, false otherwise) are well documented.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the author to ensure that all call sites are updated and that the new semantics are documented. This falls under asking the author to ensure behavior is intended and to double-check things, which violates the rules.

4. backend/parsers/windmill-parser-ts/src/lib.rs:469

• Draft comment:
  Similarly, updating top_level_call in resolve_ts_interface_and_type_alias to bool improves clarity. Confirm that all invocations now supply an appropriate boolean value.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 50% <= threshold 50% The comment suggests a change to improve clarity by updating a variable type to boolean. However, it also asks the author to confirm that all invocations supply an appropriate boolean value, which violates the rule against asking for confirmation. The suggestion to change the type is valid, but the request for confirmation is not.

5. backend/parsers/windmill-parser-ts/src/lib.rs:510

• Draft comment:
  The function tstype_to_typ now accepts a bool for top_level_call. The change is consistent, but double-check that all downstream recursive calls use the intended flag.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to double-check the use of a flag in recursive calls, which violates the rule against asking for confirmation or double-checking. It doesn't provide a specific suggestion or point out a clear issue.

6. backend/parsers/windmill-parser-ts/src/lib.rs:690

• Draft comment:
  The change in parse_one_of_type’s parameter from Option<()> to bool is consistent with other changes. Ensure that caller sites supply the correct boolean flag.
• 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.

7. backend/parsers/windmill-parser-ts/src/lib.rs:763

• Draft comment:
  Updating the top_level_call parameter in one_of_properties to bool maintains API consistency. Confirm that recursively processing nested types behaves as expected.
• 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.

Workflow ID: wflow_JvXda4lwkeMwYjmG

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

[ellipsis-dev]

The new pattern matching with let Typ::Object(properties) = followed by unreachable!() assumes that the resolved type is always an Object. This may panic if an alias resolves to a non-object type. Consider handling non-object cases gracefully or documenting this requirement clearly.

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 9e475c3 in 1 minute and 44 seconds. Click for details.

• Reviewed 26 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 2 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/parsers/windmill-parser-ts/src/lib.rs:497

• Draft comment:
  Removed forced object deconstruction. This change allows non-object types (e.g. type aliases) to be handled.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is purely informative, explaining what a change does without providing any actionable feedback or suggestions. It doesn't ask for confirmation or suggest improvements.

2. backend/parsers/windmill-parser-ts/src/lib.rs:504

• Draft comment:
  The new if condition uses a global check over type_resolver for unresolved TypeRefs. Consider verifying if checking the current resolved type would be clearer.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The comment makes a reasonable point - checking the entire type_resolver map seems broader than necessary when we could just check the current resolved type. However, there may be a reason for the global check that isn't immediately obvious. The code is dealing with complex type resolution and reference handling. I may be missing important context about why a global check is needed - there could be edge cases or recursive type references that require checking the full resolver state. While checking just the current type would be more focused, the global check may be intentional to handle complex type resolution scenarios. Without deeper understanding of the type system, we can't be certain the suggested change would be safe. The comment raises a valid point about code clarity but may be missing important context about why the broader check is needed.

Workflow ID: wflow_xvwSqcBfozsy2fSd

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

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed d6024bb in 1 minute and 8 seconds. Click for details.

• Reviewed 17 lines of code in 1 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/parsers/windmill-parser-ts/src/lib.rs:501

• Draft comment:
  Removing the check with type_resolver.iter().any(...) makes the resolution unconditional when top_level_call is true. Ensure this is intentional since it always triggers recursion even if no TypeRef exists. Verify the performance and semantic impact.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

Workflow ID: wflow_gYRpNqag4q1D8tvU

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

[HugoCasa]

• resource types
• s3object

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 1697a3d in 1 minute and 10 seconds. Click for details.

• Reviewed 119 lines of code in 2 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/parsers/windmill-parser-ts/src/lib.rs:421

• Draft comment:
  Filtering out WMILL_TYPE_REF_KEY in object properties prevents unwanted recursion when resolving type references. Ensure this behavior is documented.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50% None

2. backend/parsers/windmill-parser-ts/src/lib.rs:502

• Draft comment:
  Added an early check to ensure type_name exists in symbol_table. Confirm that this doesn’t block valid external type references if needed.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 40% <= threshold 50% The comment is asking the author to confirm that the change doesn't block valid external type references. This falls under asking the author to ensure behavior is intended, which is against the rules. However, it does point out a potential issue with the change, which could be useful for the author to consider. The comment is somewhat borderline, but it leans towards being a violation because it asks for confirmation rather than suggesting a specific change or improvement.

3. backend/parsers/windmill-parser-ts/src/lib.rs:522

• Draft comment:
  Appending a WMILL_TYPE_REF_KEY property to object types tracks local type references. Verify downstream consumers properly ignore this metadata.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to verify that downstream consumers properly ignore the metadata, which is against the rules. It doesn't provide a specific suggestion or point out a specific issue with the code.

4. backend/parsers/windmill-parser-ts/src/lib.rs:716

• Draft comment:
  Fallback now returns Typ::Resource(to_snake_case(symbol)) instead of Typ::Unknown. Ensure this aligns with the intended semantics for unresolved type references.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the author to ensure that the change aligns with the intended semantics, which is against the rules. It does not provide a specific suggestion or ask for a specific test to be written. Therefore, it should be removed.

5. frontend/src/lib/inferArgSig.ts:77

• Draft comment:
  Special-casing WMILL_TYPE_REF_KEY: setting newS.format based on prop.typ.typeref. Confirm that prop.typ provides the expected structure and that this is the intended behavior.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to confirm the structure and intended behavior, which violates the rule against asking for confirmation of intention or behavior. It does not provide a specific code suggestion or ask for a specific test to be written.

6. frontend/src/lib/inferArgSig.ts:142

• Draft comment:
  Deleting properties[WMILL_TYPE_REF_KEY] cleans up internal metadata from the final schema. Verify this removal does not accidentally drop user data.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50% None

Workflow ID: wflow_drVUmwcVWecg4Anx

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

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 823e508 in 3 minutes and 38 seconds. Click for details.

• Reviewed 94 lines of code in 2 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. frontend/src/lib/components/SchemaForm.svelte:293

• Draft comment:
  In the shouldResetValue function, multiple conditions compare types and formats (e.g. resource[] vs non-resource). Consider adding inline comments to explain each branch for future maintenance.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The code is fairly complex with multiple type conversion cases. However, the conditions are already quite self-documenting through clear variable names and logical structure. The return values directly relate to the conditions in a straightforward way. Adding comments might just restate what's already clear from the code. The code could be confusing to someone unfamiliar with the resource types and format system. The relationship between conditions and return values may not be immediately obvious without domain knowledge. While the domain concepts need to be understood, adding inline comments wouldn't help explain those concepts - that belongs in higher-level documentation. The function logic itself is clear once you understand the domain. The comment should be deleted. The code is already self-documenting through clear variable names and logical structure. Adding inline comments would likely just restate what's already evident from the code.

2. frontend/src/lib/components/SchemaForm.svelte:228

• Draft comment:
  The $effect.pre block uses a standalone expression referencing 'schema.order' with no visible effect. Consider adding a comment to clarify that it is used solely to trigger reactivity.
• 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. frontend/src/lib/components/SchemaForm.svelte:289

• Draft comment:
  Using structuredClone($state.snapshot(schema.properties)) to update lastSchemaSnapshot may incur performance overhead for large schemas. Consider optimizing or memoizing this deep clone if possible.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While the comment raises a valid point about potential performance impact, it's speculative ("may incur") and doesn't provide a clear actionable solution. The suggestion to "consider optimizing" is vague. The code appears to need a deep clone for tracking schema changes, and structuredClone is a reasonable choice. The untrack() usage shows the authors already considered performance. The comment identifies a real potential performance issue. Large schemas with many properties could indeed see noticeable overhead from repeated deep cloning. However, the comment doesn't provide enough value - it points out a potential issue without offering a concrete solution, and the current implementation seems necessary for the functionality. The untrack() usage suggests performance was already considered. Delete the comment. While it raises a valid concern, it's not actionable enough and the current implementation appears to be a reasonable approach given the requirements.

4. frontend/src/lib/components/SchemaForm.svelte:246

• Draft comment:
  The loop 'for (const key in args) { args[key] }' appears to only access each property to trigger reactivity. Consider adding a clarifying comment to explain this intentional side-effect.
• 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. frontend/src/lib/components/SchemaForm.svelte:342

• Draft comment:
  A 'svelte-ignore' comment suppresses accessibility warnings for click events without key events. Consider adding keyboard event handling to improve accessibility instead of ignoring the warning.
• 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. frontend/src/lib/components/SchemaForm.svelte:76

• Draft comment:
  This component uses custom reactive helpers like $props, $bindable, $state, and $derived. Consider adding documentation or inline comments to explain their purpose and usage for future 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.

Workflow ID: wflow_iE55OsBsDWaJIzgc

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

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 66e7cc0 in 1 minute and 29 seconds. Click for details.

• Reviewed 23 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 2 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/parsers/windmill-parser-ts/src/lib.rs:743

• Draft comment:
  Using .any(|s| *s == symbol) is more concise and idiomatic than .find(...).is_some().
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

2. backend/parsers/windmill-parser-ts/src/lib.rs:758

• Draft comment:
  The fallback now returns Typ::Resource(to_snake_case(symbol)) instead of Typ::Unknown. Ensure this is intentional since the subsequent pattern match expects a Typ::Object. Consider adding a clarifying comment if this fallback behavior for unresolved local types is desired.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_tRYPlq9eL3QGktOk

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