Skip to content

Folders as presets in FilterSearchbar#8409

Open
diegoimbert wants to merge 4 commits intomainfrom
folders-as-presets
Open

Folders as presets in FilterSearchbar#8409
diegoimbert wants to merge 4 commits intomainfrom
folders-as-presets

Conversation

@diegoimbert
Copy link
Contributor

  • Folder presets in filter search bar
  • nit max preset height

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Mar 17, 2026

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: fe1d5b6
Status: ✅  Deploy successful!
Preview URL: https://54f46261.windmill.pages.dev
Branch Preview URL: https://folders-as-presets.windmill.pages.dev

View logs

@diegoimbert diegoimbert marked this pull request as ready for review March 17, 2026 14:12
@claude
Copy link
Contributor

claude bot commented Mar 17, 2026

Claude finished @diegoimbert's task in 2m 42s —— View job


Review of PR #8409 — Folders as presets in FilterSearchbar

  • Gather context and read changed files
  • Review code quality and patterns
  • Post review feedback

Overall

Clean, focused PR that adds folder names as filter presets in the resources and variables pages, and adds a max-height with scroll to the presets container. The changes are well-structured and consistent between the two pages.

Feedback

1. Duplicated logic between resources and variables pages

The folderPresets derived value and loadFolders function are identical in both resources/+page.svelte and variables/+page.svelte. This is fine for now since it's only two pages, but if folder presets are added to more pages (schedules, assets), consider extracting a shared composable (e.g. useFolderPresets(filterSchema)).

2. Consider the number of folders in a workspace

If a workspace has many folders (50+), the presets list could become very long even with max-h-24 overflow-y-auto. The max-h-24 (6rem / ~96px) limit is reasonable, but worth verifying that the scroll behavior works well visually when there are many folders.

3. API call could be deduplicated (minor)

Both loadResources() and loadFolders() are triggered in the same $effect. The folders list is unlikely to change frequently — if both pages are visited in a session, folders are fetched twice. Not a real issue, just noting it.

4. No error handling on loadFolders (minor)

loadFolders doesn't have a try/catch. If FolderService.listFolderNames fails, the error will be unhandled. The existing loadResources/loadVariables functions in these pages also lack explicit error handling in most paths, so this is consistent with the existing pattern — just flagging it.

5. The nested overflow-y-auto (nit)

The presets container (max-h-24 overflow-y-auto) is nested inside a parent that already has overflow-y-auto (line 603 of FilterSearchbar.svelte). Nested scrollable areas can sometimes cause UX issues (scroll trapping), but with a small fixed height like max-h-24 this should be fine in practice.

Looks Good

  • The preset filtering logic correctly hides presets already applied in the search text (existing behavior, preserved)
  • user_folders_only preset is correctly retained alongside the new folder presets
  • The conditional inclusion of user_folders_only based on schema definition is a nice touch

Testing Instructions

Navigate to the Resources or Variables page in a workspace that has multiple folders. Click on the filter search bar — the dropdown should show folder presets (e.g. f/my_folder) under the "Presets" section. Clicking a folder preset should add a path_start filter. The "My folders only" preset should still appear alongside the folder presets. If there are many folders, the preset section should scroll vertically within a fixed height container.


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant