send alternatives on timeout toast

[diegoimbert]

Screen.Recording.2025-10-30.at.15.37.04.mov

Screen.Recording.2025-10-30.at.15.38.18.mov

[cloudflare-workers-and-pages]

Deploying windmill with    Cloudflare Pages

Latest commit:	0a2d350
Status:	✅  Deploy successful!
Preview URL:	https://30f89217.windmill.pages.dev
Branch Preview URL:	https://di-toast-alternatives-on-tim.windmill.pages.dev

View logs

[claude]

Claude finished @diegoimbert's task —— View job

---

Code Review Complete ✅

Reviewing PR: send alternatives on timeout toast

---

Summary

This PR implements a toast notification system that provides actionable alternatives when operations take longer than expected (>4 seconds). The PR includes significant refactoring: extracting the runs page component and audit logs loading logic to use a new CancelablePromiseUtils library.

Key Changes:

• New cancelable-promise-utils.ts utility for composable promise operations with cancellation support
• Timeout toasts with action buttons (e.g., "Reduce to 25 items per page")
• Extraction of RunsPage logic from route file into separate component
• Applied to both runs page and audit logs
• Package version bump for @tutorlatin/svelte-tiny-virtual-list (3.0.15 → 3.0.16)

---

Detailed Findings

1. New Utility: cancelable-promise-utils.ts ✅ Good

Positive:

• Well-structured functional API for composing cancelable promises
• Clear naming conventions (then, map, catchErr, finallyDo, onTimeout)
• Addresses a real need for cancellation in async operations

Issues:

🔴 Critical: Memory Leak in pipe function (line 41-44)

export function pipe<T>(
    promise: CancelablePromise<T>,
    f: (value: T) => void
): CancelablePromise<T> {
    promise.then((value) => { f(value) })
    return promise
}

Problem: The pipe function attaches a .then() handler but never handles errors. If the promise rejects, this creates an unhandled promise rejection. Additionally, it doesn't integrate properly with the cancellation chain.

Recommendation: Either remove this function if unused, or fix it:

export function pipe<T>(
    promise: CancelablePromise<T>,
    f: (value: T) => void
): CancelablePromise<T> {
    let p = new CancelablePromise<T>((resolve, reject) => {
        promise
            .then((value) => {
                f(value)
                resolve(value)
            })
            .catch((err) => reject(err))
    })
    p.cancel = () => promise.cancel()
    return p
}

⚠️ Minor: Double catch in catchErr (line 60)

.catch((err) => {
    let promise2 = f(err)
    promiseToBeCanceled = promise2
    return promise2.then((value2) => resolve(value2)).catch((err2) => reject(err2))
})
.catch((err) => reject(err)) // <- Redundant

The second .catch at line 60 is redundant since all errors from the first .catch block are already handled. This doesn't cause a bug but adds unnecessary complexity.

ℹ️ Documentation: Missing JSDoc

The utility functions would benefit from JSDoc comments explaining:

• What each function does
• When to use them
• Examples of composition

---

2. JobsLoader.svelte Refactoring ✅ Good with minor issues

Positive:

• Proper integration of CancelablePromiseUtils.onTimeout
• Clean error handling with CancelError check
• Timeout set to 4 seconds with helpful toast messages

Issues:

⚠️ Variable shadowing (line 438-440)

let queueTs: string | undefined
if (minQueueCreatedAt) {
    const queueTs = new Date(minQueueCreatedAt).toISOString() // shadows outer
    lastQueueTs = queueTs
}

The const queueTs inside the if block shadows the outer let queueTs. This appears intentional but is confusing. Should be:

if (minQueueCreatedAt) {
    queueTs = new Date(minQueueCreatedAt).toISOString()
    lastQueueTs = queueTs
}

ℹ️ Cleanup opportunity (line 244)

CancelablePromiseUtils.pipe(promise, () => {
    loadingFetch = false
})

This uses the problematic pipe function. Should use finallyDo instead:

promise = CancelablePromiseUtils.finallyDo(promise, () => {
    loadingFetch = false
})

---

3. RunsPage.svelte - Large New Component ⚠️

Positive:

• Clean separation of concerns
• Good use of Svelte 5 runes ($state, $derived, $effect)
• Proper timeout handling integrated

Issues:

🔴 State management complexity

The component has 76 reactive state variables tracked in the $effect at line 558-579. This creates an extremely broad reactivity graph that will trigger re-computations even for unrelated changes.

Recommendation: Consider breaking this into smaller, focused components or using more granular effects:

• Separate effects for different concerns (filters, date range, workspace)
• Use untrack() more strategically to prevent unnecessary triggers
• Consider using derived stores for related state

⚠️ TODO Comment (line 1)

The {#key perPage} wrapper in the route file (line 11-13 of +page.svelte) is a workaround. This indicates the component's reactivity isn't properly scoped.

Recommendation: Address the underlying state management issue rather than relying on the {#key} workaround.

⚠️ Performance: Multiple $effect blocks

Lines 739-775 have three sequential $effect blocks:

1. Line 739-741: Loading management
2. Line 742-769: Query parameter sync
3. Line 770-775: Additional query sync

These could potentially be consolidated or better organized to reduce effect overhead.

---

4. AuditLogsFilters.svelte ✅ Well Done

Positive:

• Clean integration of timeout toast
• Proper cancellation handling with CancelError check
• Good use of untrack() to control reactivity

Minor:

• No significant issues found in this file

---

5. Select Component Changes ✅ Minor fixes

The changes to Select.svelte and related files appear to be bug fixes related to the value binding. These look appropriate and don't introduce issues.

---

Security Considerations

✅ No security issues identified

• No user input directly interpolated into HTML
• API calls properly use workspace store
• No exposure of sensitive data

---

Performance Considerations

⚠️ Potential Issue: Toast Creation

Every load operation that times out creates a new toast with a closure capturing the perPage value. If users trigger multiple loads quickly, this could create many toast callbacks.

Recommendation: Debounce or deduplicate timeout toasts for the same operation type.

⚠️ Large State Reactivity in RunsPage

As mentioned above, the 76-variable reactivity graph in RunsPage.svelte could cause performance issues with frequent updates.

---

Best Practices & Code Quality

✅ Good Practices Observed:

1. Proper use of Svelte 5 runes throughout
2. Consistent error handling patterns
3. Good separation of concerns with utility functions
4. Proper cleanup in $effect return functions (promise cancellation)

⚠️ Areas for Improvement:

1. Documentation: Add JSDoc to cancelable-promise-utils.ts
2. Testing: No tests visible for the new utility functions
3. Type Safety: Some any types in RunsPage.svelte (e.g., line 107: let argFilter: any)
4. Error Messages: Consider making toast messages translatable/configurable

---

Svelte 5 Compliance

✅ Generally compliant with Svelte 5 best practices:

• Proper use of $state, $derived, $bindable
• Correct $effect patterns with cleanup
• No deprecated Svelte 4 patterns found

Minor improvement suggestion:
Consider using $state.raw() for large state objects that don't need deep reactivity (e.g., the jobs array).

---

Recommendations

High Priority:

1. 🔴 Fix the pipe function in cancelable-promise-utils.ts to handle errors properly
2. 🔴 Address RunsPage state complexity - consider breaking into smaller components
3. ⚠️ Fix variable shadowing in JobsLoader.svelte line 438-440

Medium Priority:

4. ⚠️ Remove redundant catch in catchErr function (line 60)
5. ⚠️ Replace pipe with finallyDo in JobsLoader.svelte line 244
6. ⚠️ Address the TODO about {#key} workaround in the route file

Low Priority:

7. Add JSDoc documentation to cancelable-promise-utils.ts
8. Consider adding unit tests for the new utility functions
9. Add debouncing/deduplication for timeout toasts
10. Consider using $state.raw() for performance optimization

---

Testing Instructions

How to Test the Timeout Toast Feature:

To see the feature in action:

Navigate to the Runs page from the main navigation menu. If you have a large workspace with many job runs, you'll see the timeout toast automatically when loading takes longer than 4 seconds. The toast will display "Operation taking longer than expected" with a button to "Reduce to 25 items per page" (if the current perPage setting is > 25). Clicking this button will immediately reduce the pagination limit and reload the data. The same behavior applies to the Audit Logs page - navigate there and observe similar timeout behavior when filtering through large datasets or applying complex filters that take time to process.

To artificially trigger the timeout for testing purposes, you can use browser dev tools to throttle your network connection to "Slow 3G" before loading either page, or temporarily modify the timeout value in the code from 4000ms to a lower value like 500ms to trigger it immediately during normal operations.

---

Overall Assessment

This is a valuable feature that improves user experience by providing actionable feedback during long operations. The implementation is generally solid, but there are a few critical issues that should be addressed before merging:

1. The pipe function has a memory leak that needs fixing
2. The RunsPage component's state management complexity may cause maintenance and performance issues down the line

The feature works as intended and the timeout mechanism is well-implemented. With the recommended fixes, this will be a great addition to the codebase.

---

[claude]

🔴 Critical: Potential memory leak and unhandled rejection

The pipe function doesn't handle promise rejections, which will cause unhandled promise rejection warnings. Additionally, it doesn't properly integrate with the cancellation chain.

Suggested fix:

export function pipe<T>(
	promise: CancelablePromise<T>,
	f: (value: T) => void
): CancelablePromise<T> {
	let p = new CancelablePromise<T>((resolve, reject) => {
		promise
			.then((value) => {
				f(value)
				resolve(value)
			})
			.catch((err) => reject(err))
	})
	p.cancel = () => promise.cancel()
	return p
}

Alternatively, if this function is not heavily used, consider using finallyDo instead, which already handles this pattern correctly.

[claude]

⚠️ Minor: Redundant catch block

The second .catch((err) => reject(err)) is redundant since all errors are already handled in the chain above. The first .catch block either resolves or rejects, so there's no error that would propagate to this second catch.

Consider removing this line for clarity.

[claude]

ℹ️ Use finallyDo instead of pipe

The pipe function has issues (see comment on its implementation). For side effects that should run regardless of success/failure, use finallyDo:

promise = CancelablePromiseUtils.finallyDo(promise, () => {
	loadingFetch = false
})

This ensures proper error handling and cancellation.

[claude]

⚠️ Performance: Large reactivity graph

This $effect tracks 26 different reactive variables (lines 558-579). Every change to any of these variables will trigger the entire effect to re-run, including onParamChanges() which performs data fetching.

Recommendations:

1. Consider splitting into multiple, more focused effects based on related concerns
2. Use untrack() more strategically for variables that shouldn't trigger re-fetch
3. Group related state into derived objects to reduce the number of dependencies

This is especially important given the TODO comment at the top of the file about refactoring state management.

[claude]

⚠️ TODO: Address underlying state management issue

The {#key perPage} workaround (line 11) forces the entire RunsPage component to remount when perPage changes. This is a sign that the component's internal state isn't properly scoped or managed.

Instead of forcing remount, consider:

1. Ensuring perPage changes properly trigger the necessary effects in RunsPage
2. Using $effect cleanup functions to cancel ongoing operations when params change
3. Better state initialization in RunsPage to handle prop changes

Remounting loses all component state (selections, scroll position, etc.) which might not be the desired UX.

[ellipsis-dev]

Typographical error: 'mefore' should be 'before'.

Suggested change

	// Extend MinTs to fetch jobs mefore minTs and show a correct concurrency graph
	// Extend MinTs to fetch jobs before minTs and show a correct concurrency graph