fix: Improve small details on tutorials

[tristantr]

Improve small UX details on tutorials

[claude]

Claude finished @tristantr's task —— View job

---

Code Review Complete ✅

• Read changed files
• Analyze git diff
• Review code quality and best practices
• Check for bugs and issues
• Provide comprehensive feedback
• Add reproduction instructions

---

Overall Assessment

This PR makes solid improvements to the tutorial banner UX by implementing role-based access control and auto-dismissal when tutorials are completed. The changes are well-structured and address real user experience issues.

Strengths:

• ✅ Smart role-based filtering ensures users only see tutorials relevant to them
• ✅ Auto-dismissal when all accessible tutorials are completed is a great UX improvement
• ✅ The toast notification with "Skip tutorials" action when dismissing is excellent UX
• ✅ Synchronization with backend state on mount ensures consistency
• ✅ Code is well-commented and intent is clear

Areas for Improvement:

I've left inline comments on specific issues, but here are the key points:

1. Svelte 5 Optimization: The getAccessibleTutorialIndexes() function would benefit from using $derived instead of being a regular function, as it depends on reactive state ($userStore). This would make reactivity more explicit and efficient per Svelte 5 best practices.

2. Error Handling: The async onMount doesn't handle potential errors from syncTutorialsTodos(). If the API call fails, the banner could be stuck in an unexpected state.

3. Magic String Duplication: The localStorage key 'tutorial_banner_dismissed' is defined as a constant in TutorialBanner.svelte but hardcoded in tutorialUtils.ts. This creates maintenance risk.

4. Operator Access Clarity: The removal of the operator check in +page.svelte needs verification. If operators have no accessible tutorials, they'll briefly see the banner before it auto-dismisses, which may create unnecessary DOM work.

Security & Performance

• ✅ No security concerns identified
• ✅ No performance regressions expected
• ⚠️ Minor optimization opportunity with $derived (see inline comment)

Testing Instructions

To test these changes through the app interface, navigate to the home page after logging in. The tutorial banner behavior will vary based on your user role and tutorial completion status. As an admin or developer, you'll see the banner at the top of the home page with "View tutorials" and a dismiss button. Click the dismiss X button to see a toast notification offering to skip all tutorials. If you complete all tutorials accessible to your role through the Tutorials page, the banner will automatically disappear on the next home page visit. Operators will now also see the banner if they have accessible tutorials. To test role-based filtering, switch between user accounts with different roles (admin, developer, operator) and observe which tutorials appear and whether the banner shows based on completion status for that role's accessible tutorials.

---

[cloudflare-workers-and-pages]

Deploying windmill with    Cloudflare Pages

Latest commit:	0bcacc1
Status:	✅  Deploy successful!
Preview URL:	https://1b3929ee.windmill.pages.dev
Branch Preview URL:	https://tl-tutorials.windmill.pages.dev

View logs

[claude]

Svelte 5 Best Practice: Consider using $derived for this computation since it depends on reactive state ($userStore). This would make the reactivity more explicit and efficient:

const accessibleTutorialIndexes = $derived.by(() => {
  const indexes = new Set<number>()
  const user = $userStore

  for (const tab of Object.values(TUTORIALS_CONFIG)) {
    if (!hasRoleAccess(user, tab.roles)) {
      continue
    }
    for (const tutorial of tab.tutorials) {
      if (tutorial.index !== undefined && hasRoleAccess(user, tutorial.roles)) {
        indexes.add(tutorial.index)
      }
    }
  }
  return indexes
})

This way the set is automatically recomputed when $userStore changes, and you can reference it directly in onMount without calling a function.

[claude]

Potential Issue: This onMount is now async, but you're not handling potential errors if syncTutorialsTodos() fails. Consider adding error handling:

onMount(async () => {
  try {
    await syncTutorialsTodos()
    // ... rest of logic
  } catch (error) {
    console.error('Failed to sync tutorial progress:', error)
    // Fallback to manual dismissal check only
    isDismissed = getLocalSetting(DISMISSED_KEY) === 'true'
  }
})

This prevents the banner from being stuck in a loading state if the API call fails.

[claude]

Logic Consideration: This filtering logic is good, but there's a potential edge case: if $tutorialsToDo hasn't been populated yet (before syncTutorialsTodos() completes), this could incorrectly show the banner as dismissed.

However, since syncTutorialsTodos() is awaited before this logic runs, this should be safe. Consider adding a comment to clarify this dependency:

// Safe to check tutorialsToDo here since we awaited syncTutorialsTodos() above
const remainingAccessibleTutorials = $tutorialsToDo.filter((index) =>
  accessibleIndexes.has(index)
)

[claude]

Code Smell - Redundant State Update: There's a subtle issue here. If allTutorialsCompleted is true, you're setting isDismissed = true AND also calling storeLocalSetting(DISMISSED_KEY, 'true').

However, if the user clears their localStorage but tutorials remain completed in the backend, this logic will re-set the localStorage key on every page load. While this is probably the desired behavior, consider if this is intentional or if you want to track "auto-dismissed" vs "manually dismissed" separately.

Current behavior is acceptable, but worth documenting the intent.

[claude]

Good Addition: This is a smart coupling between skipAllTodos() and the banner dismissal. When users skip all tutorials, it makes sense to also dismiss the banner.

Minor Issue: However, there's a potential inconsistency. In TutorialBanner.svelte, you're using the constant DISMISSED_KEY = 'tutorial_banner_dismissed', but here you're hardcoding the string 'tutorial_banner_dismissed'.

Consider:

1. Exporting the constant from a shared location, or
2. Importing it from TutorialBanner.svelte (though this creates a dependency), or
3. At minimum, adding a comment here referencing that this matches the key in TutorialBanner

This prevents future bugs if the key name changes.

[claude]

Intentional Change - Operators Can Now See Banner: This removes the check that was hiding the tutorial banner from operators.

Question: Is this intentional? Looking at the commit message "Allow new operator so see the tutorial banner", it seems intentional, but worth clarifying:

1. Do operators have tutorials accessible to them in TUTORIALS_CONFIG?
2. If operators have no accessible tutorials, the banner will show but be immediately dismissed by the role-based logic in TutorialBanner.svelte

If this is intentional to show operators the banner (perhaps they have their own tutorials), this is fine. Otherwise, you might want to keep the conditional but move the role logic to inside TutorialBanner.svelte for consistency.