Skip to content

Conversation

@rubenfiszel
Copy link
Contributor

@rubenfiszel rubenfiszel commented Nov 28, 2025

🤖 I have created a release beep boop

1.587.0 (2025-11-29)

Features


This PR was generated with Release Please. See documentation.

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Nov 28, 2025

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: 69dc5eb
Status: ✅  Deploy successful!
Preview URL: https://8c6492cf.windmill.pages.dev
Branch Preview URL: https://release-please--branches--ma.windmill.pages.dev

View logs

@claude
Copy link
Contributor

claude bot commented Nov 28, 2025

Claude finished @rubenfiszel's task —— View job


Review of Release PR 1.587.0 - Workspace Dependencies Feature


Overview

This release PR introduces the workspace dependencies feature (#7124), a significant enhancement allowing workspace administrators to define centralized dependency configurations that can be shared across scripts, flows, and apps. The implementation spans 96 files with +6929/-1944 lines changed.

Note: Since this is a release-please PR merging already-committed changes, inline comments on specific lines are not available. All feedback is provided in this top-level review.

Code Quality Assessment

Strengths

  1. Strong Security Implementation

    • All mutation endpoints (create, archive, delete) properly use require_admin checks (backend/windmill-api/src/workspace_dependencies.rs:40, 92, 124)
    • Admin-only access ensures workspace dependencies can only be managed by authorized users
    • No SQL injection vulnerabilities - all queries use parameterized statements via sqlx macros
  2. Excellent Database Design

    • Well-designed schema with proper constraints (backend/migrations/20251106152104_raw_requirements.up.sql:1-24)
    • UNIQUE constraints prevent duplicate active workspace dependencies: one_non_archived_per_name_language_constraint
    • Performance indexes on common query patterns
    • Proper use of archived flag for soft deletes maintaining history
  3. No SELECT * Anti-Pattern

    • Verified: No SELECT * queries in the codebase
    • All queries explicitly list required columns (adhering to backend best practices for worker compatibility)
    • Critical for backwards compatibility when workers lag behind API servers
  4. Smart Caching Strategy

    • 10-second in-memory cache for get_latest operations (backend/windmill-common/src/workspace_dependencies.rs:22-26)
    • Uses quick_cache with proper cache key structure: (workspace_id, language, name)
    • Reduces database load for frequently accessed dependencies
  5. Version Compatibility Handling

    • Minimum version check (MIN_VERSION_WORKSPACE_DEPENDENCIES = "1.587.0") ensures workers support the feature
    • Debug flag WMDEBUG_FORCE_V0_WORKSPACE_DEPENDENCIES for testing
    • Graceful degradation when workers are behind
  6. Comprehensive Testing

    • New test suite: backend/tests/workspace_dependencies.rs
    • Tests cover Python, Bun, and PHP implementations
    • Fixtures for dependency map testing

⚠️ Issues & Concerns

Critical Issues

  1. Frontend Svelte 4 Anti-Pattern (WorkspaceDependenciesEditor.svelte:18)

    const dispatch = createEventDispatcher()
    • Uses deprecated Svelte 4 createEventDispatcher instead of Svelte 5 callback props
    • According to frontend/svelte5-best-practices.mdc, should use callback props instead
    • Impact: Inconsistent with Svelte 5 migration, technical debt
  2. Missing Error Context (multiple locations)

    • backend/windmill-api/src/workspace_dependencies.rs doesn't add location tracking to errors
    • Should use #[track_caller] attribute on error-prone functions
    • Frontend error handling logs to console but doesn't always provide actionable user feedback
  3. Potential Race Condition (backend/windmill-worker/src/workspace_dependencies.rs:42-75)

    let already_rebuilt = windmill_common::global_settings::load_value_from_global_settings(
        db, &setting_name
    ).await?.is_some();
    • Check-then-act pattern for workspace_dependencies_map_rebuilt setting
    • If two admins create first unnamed dependencies concurrently, both might trigger rebuild
    • Mitigation: Low probability, rebuild is idempotent, but could waste resources

Performance Concerns

  1. Unbounded Dependency Tree Loading (DependenciesDeploymentWarning.svelte:92-124)

    async function loadChildren(node: DependencyNode) {
        const dependents = await WorkspaceService.getDependents({...})
        const amounts = await WorkspaceService.getDependentsAmounts({...})
    }
    • Recursive dependency tree loading without depth limits
    • Deep dependency chains could cause performance issues
    • Recommendation: Add max depth limit and pagination
  2. Cache Invalidation Gaps

    • WORKSPACE_DEPENDENCIES_CACHE has 10-second timeout
    • No explicit cache invalidation on create/archive/delete operations
    • Workers might use stale cached dependencies for up to 10 seconds after changes
    • Impact: Minor - 10 seconds is reasonable, but could be more explicit

Code Quality Issues

  1. Inconsistent Naming (frontend components)

    • Mix of "workspace dependencies" and "enforced dependencies" in UI strings
    • Example: WorkspaceDependenciesSettings.svelte:69 uses "enforced dependencies" in toast
    • Recommendation: Standardize on one term
  2. TODO Comments Left in Code

    • backend/windmill-worker/src/workspace_dependencies.rs:16: // TODO: Make Option, or optimize it in any other way.
    • These should be tracked as issues or resolved before release
  3. Dead Code (WorkspaceDependenciesEditor.svelte:133-145)

    // Commented out Go support
    // go: `module mymod...`
    • Should either be removed or tracked for future implementation
  4. Complex Function - Needs Decomposition (backend/windmill-common/src/workspace_dependencies.rs:322-397)

    • WorkspaceDependenciesPrefetched::extract is 75 lines with nested async logic
    • Violates Single Responsibility Principle
    • Recommendation: Extract sub-functions for explicit/implicit/none cases

Documentation & Testing

  1. Missing OpenAPI Descriptions

    • backend/windmill-api/openapi.yaml lists endpoints but could have better descriptions
    • Should document request/response schemas more thoroughly
  2. Ignored Test (backend/tests/workspace_dependencies.rs:43)

    #[ignore]
    async fn basic_manual_named(db: Pool<Postgres>)
    • Main workspace dependencies test is ignored
    • Critical: Should be enabled or removed before release
  3. Incomplete Test Coverage

    • No frontend tests for new components
    • Missing integration tests for archive/delete with dependents
    • Edge cases (concurrent modifications, version mismatches) not fully tested

🔒 Security Assessment

Overall Security: GOOD ✅

  • ✅ All admin-only operations properly gated
  • ✅ No SQL injection vulnerabilities
  • ✅ Input validation via sqlx types and Rust type system
  • ✅ No path traversal issues (paths are validated)
  • ✅ No XSS risks in frontend (Svelte auto-escapes)

Minor Concerns:

  • Frontend displays raw error messages from API (could leak internal details)
  • No rate limiting on workspace dependencies operations (admin-only reduces risk)

📊 Performance Impact

Positive:

  • Caching reduces DB queries
  • Indexes support efficient lookups
  • Worker-based dependency recomputation is async

Negative:

  • Initial unnamed dependency creation triggers full workspace dependency map rebuild
  • Archiving/deleting triggers recomputation for all dependent scripts
  • Large workspaces could see significant load

Recommendations

Must Fix Before Release

  1. Enable the ignored test (workspace_dependencies.rs:43) or document why it's disabled
  2. Fix Svelte 4 anti-pattern - migrate createEventDispatcher to callback props
  3. Add depth limit to dependency tree loading (prevent DoS)

Should Fix

  1. Add explicit cache invalidation on mutations
  2. Decompose complex extract function
  3. Standardize UI terminology
  4. Add #[track_caller] to error-prone functions
  5. Remove or implement commented-out Go support

Nice to Have

  1. Add frontend tests for new components
  2. Add integration tests for concurrent operations
  3. Better OpenAPI documentation
  4. Rate limiting on dependency operations

Testing Instructions for QA

To verify the workspace dependencies feature works correctly, navigate to the Workspace Settings page in the Windmill web interface and select the "Workspace Dependencies" or "Enforced Dependencies" tab. As a workspace administrator, you should be able to create new workspace-wide dependency configurations by selecting a language (Python, Bun/TypeScript, or PHP) and choosing between creating a workspace default (applies to all scripts of that language) or a named dependency set (referenced explicitly by scripts). After creating dependencies, test editing the content by modifying the dependency specifications (such as changing version numbers or adding new packages), then save and verify the changes are reflected. Test the archive and delete operations by first checking the "Referenced From" functionality to see which scripts/flows/apps depend on a particular dependency configuration, then attempt to archive or delete it and confirm the warning dialog appears showing dependent items. Finally, create a script in one of the supported languages and verify it automatically picks up the workspace default dependencies, or manually reference a named dependency set using language-specific annotations in the script code to ensure the dependency resolution works end-to-end.


Overall Recommendation

APPROVE WITH RESERVATIONS

The workspace dependencies feature is well-architected with strong security and database design. However, there are critical issues that should be addressed:

  1. The ignored test needs attention before shipping
  2. Svelte 4 patterns should be migrated to Svelte 5
  3. Unbounded dependency tree loading is a potential performance/security issue

These issues don't block the release but should be tracked and fixed in a follow-up PR.


Branch: release-please--branches--main--components--windmill

@rubenfiszel rubenfiszel force-pushed the release-please--branches--main--components--windmill branch from 17e6988 to 2731bc9 Compare November 29, 2025 01:25
@rubenfiszel rubenfiszel force-pushed the release-please--branches--main--components--windmill branch from 9ac89cd to e1faf12 Compare November 29, 2025 15:28
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.

2 participants