Skip to content

Conversation

@oliver-sanders
Copy link
Member

@oliver-sanders oliver-sanders commented Nov 26, 2025

This adds task modifier filtering to the Tree & Table views as well as a bunch of related things:

  • centralise and improve task search and filter controls:
    • Add functionality to the ViewToolbar component:
      • Add action=menu for dropdowns.
      • Add action=input for text input.
      • Allow icon=Object to configure state-dependent icons.
      • Reduce icon spacing and improve group divider.
    • Move the task search & filter controls into the ViewToolbar component.
    • Improve the task state filter control:
      • Add support for task modifiers - closes filtering: task modifier filtering #1666
      • Collapse the control into a single button/menu (take up less space).
      • Reduce the width of the task search input, automatically increase
        this width when focused or when text is present in it.
  • tree: make the toolbar sticky
  • view toolbar: support globs in task ID search
  • vite: ignore nfs files
    • These can cause vite to crash.
  • update caniuse-lite

Check List

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
  • Tests are included (or explain why tests are not needed).
  • Changelog entry included if this is a change that can affect users
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@oliver-sanders oliver-sanders added this to the 2.12.0 milestone Nov 26, 2025
@oliver-sanders oliver-sanders self-assigned this Nov 26, 2025
Comment on lines +268 to +280
// it('Provides a select all functionality', () => {
// cy.visit('/#/tree/one')
// cy.get('[data-cy="control-taskStateFilter"]')
// .get('.v-list-item--active')
// .should('have.length', 0)
// cy.get('[data-cy="control-taskStateFilter"]')
// .click()
// .get('[data-cy=task-filter-select-all]')
// .click()
// cy.get('[data-cy="control-taskStateFilter"]')
// .get('.v-list-item--active')
// .should('have.length', 8)
// })
Copy link
Member Author

@oliver-sanders oliver-sanders Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regression 😨, there's no select-all button any more.

This was kinda useful, though not as straight forward any more, as we would only want to select task states (not modifiers).

Closely related to #2209

Can attempt now if deemed critical enough, but maybe appropriate for followup in #2209?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine IMO

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with leaving this out of this PR. Perhaps add a comment to explain why this is commented out?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it's just occurred to me a deselect-all button would be useful?

.toolbar {
position: sticky;
top: 0;
background: white;
Copy link
Member Author

@oliver-sanders oliver-sanders Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any better ideas than white? Some SCSS variable for the background color?

@oliver-sanders oliver-sanders marked this pull request as draft November 26, 2025 15:33
const numTasks = sortedTasks.length
describe('Options save state', () => {
beforeEach(() => {
localStorage.defaultView = 'Analysis'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the tests in this file (and elsewhere) are searching for menu items in a relatively naive way.

Adding the task state menu to the tree view caused these tests to fail as they were selecting tree view filter controls from the other tab.

The tree view does not need to be open for these tests, so rather than opening the analysis view over the top (and deal with the consequences of having the tree view underneath), I just set it up to open the analysis view by default.

selectOption('#c-gantt-filter-job-name', 'c3')
// Set task times filter option to something other than default ('Total times')
selectOption('#c-gantt-filter-job-timings', 'Queue')
selectOption('#c-gantt-filter-job-timings', 'Queue times')
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Queue also matched the task state Queued item, expanded to Queue times to disambiguate.

Comment on lines -344 to -360
it('should show a summary of tasks if the number of selected items is greater than the maximum limit', () => {
cy.visit('/#/tree/one')
cy.get('[data-cy="filter task state"]')
.click()
// eslint-disable-next-line no-lone-blocks
TaskState.enumValues.forEach(state => {
cy.get('.v-list-item')
.contains(state.name)
.click({ force: true })
})
// Click outside to close dropdown
cy.get('noscript')
.click({ force: true })
cy.get('[data-cy="filter task state"]')
.contains('.v-select__selection', '(+')
})

Copy link
Member Author

@oliver-sanders oliver-sanders Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionality removal/modification, the task state filter no longer shows the filtered states, it just highlights the icon as "active" if filters are in play.

@oliver-sanders oliver-sanders marked this pull request as ready for review December 2, 2025 11:41
Copy link
Member

@wxtim wxtim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 😄 - Can you address my comments (may well not be blockers).

Copy link
Member

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review.

The task state filtering is well thought out, however I found one slight annoyance IMO with the waiting modifiers: clicking the drop down the first time only selects waiting without expanding the modifiers. After this first click does the expand/collapse work. Also it seems the modifiers remain selected after waiting is deselected.

20251204-1755-54.1473635.mp4

Ideally IMO the expansion would work off the bat, and clicking one of the modifiers would automatically select Waiting if it was not already

Copy link
Member

@MetRonnie MetRonnie Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Discussion to possibly take into another issue, not hold up this PR)

IMO this is becoming increasingly difficult to use. This single component controls the presentation and business logic of several input types, making it difficult to reason about how they are working.

In future, I think it would be worth extracting out the input types that are used here into their own SFCs, leaving this as a component that controls presentation and vuetify defaults, with a slot for allowing each view to set its controls (the more conventional way of using Vue components).

While I understand the idea behind this existing implementation (#1809 (comment)), the main advantage being having an object that contains the configuration that can be re-used in the settings page, this can still be achieved under what I'm proposing.

What do you think?

Copy link
Member Author

@oliver-sanders oliver-sanders Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For another issue if you are sufficiently comfortable with this as it is here.

As it stands this is right at the edge of suitability for the current approach, agreed. Happy to look towards restructuring this code going forwards, e.g, something along the lines of the GraphQL form generator. Also if we end up taking this further in the future, a more model-orientated approach (hopefully removing the setOption callback) would be great.

I wouldn't like to see the complexity being decentralised back into the views (which one interpretation of the slots suggestion may result in), but there are defo other ways to do this.

With this PR I think the toolbar should do everything we need from it now and for the foreseeable (just need to add search & de-select all buttons for the graph view PR). So I don't think we should look into refactoring in the short/mid term (time better spent elsewhere). But if / when requirements start to become more advanced...

.get('.v-list-item')
.contains(TaskState.SUCCEEDED.name)
.click({ force: true })
.get('.v-list-item[state=succeeded]')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somewhere in ViewToolbar it is setting a non-standard HTML attribute state? Ideally this should be data-state.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Figured it out!

Vuetify TreeView components are configured with nested objects, e.g:

{
  title: 'A',
  value: '1',
  children: {
    {
      title: 'B',
      value: '2',
    },
    {
      title: 'C',
      value: '3',
    },
  }
}

The field children is expected by TreeView, the remaining fields are user-defined.

It turns out that if you name a field props, these props will be set on the ListItem that the TreeView uses under the hood.

Fixed by renaming from props to taskProps, will push shortly...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved:

  • Removed the state attribute.
  • Got the test to filter by :visible to avoid contamination issues.

.contains(TaskState.SUCCEEDED.name)
.click({ force: true })
.get('.v-list-item[state=succeeded]')
.click({ force: true, multiple: true })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This suggests the menu for the tree view tab is also being rendered (although probably has display: none). Normally vuetify does not render a menu until the first time it is opened (which was the case before this PR).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is the eager property on the v-menu.

Without this, the menu is destroyed-rebuilt with each use resulting in state issues.

@oliver-sanders
Copy link
Member Author

oliver-sanders commented Dec 8, 2025

  • Rebased.
  • Addressed review comments.
  • The expand button next to "waiting" is no longer disabled until "waiting" is selected. Instead, "waiting" is added to the selection if not present or erroneously removed.

TODO:

  • Investigate the scroll issue Tim reported. - resolved
  • Work out where the state attr has come from.

@oliver-sanders oliver-sanders force-pushed the view-toolbar++ branch 2 times, most recently from 2d2ce01 to 0ac3128 Compare December 8, 2025 13:49
@oliver-sanders oliver-sanders requested a review from wxtim December 8, 2025 13:49
Copy link
Member

@wxtim wxtim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Legit - tested as working. Code looks good. :)

* Add functionality to the ViewToolbar component:
  * Add `action=menu` for dropdowns.
  * Add `action=input` for text input.
  * Allow `icon=Object` to configure state-dependent icons.
  * Reduce icon spacing and improve group divider.
* Move the task search & filter controls into the ViewToolbar component.
  * Switch the Tree and Table views over to the ViewToolbar.
  * Addresses cylc#471
* Improve the task state filter control:
  * Add support for task modifiers - closes cylc#1666
  * Collapse the control into a single button/menu (take up less space).
  * Reduce the width of the task search input, automatically increase
    this width when focused or when text is present in it.
* Ensure the ViewToolbar doesn't scroll out of view.
* Closes cylc#991
* These can cause vite to crash.
…ifiers

* Don't allow the waiting state filter to be de-selected if a waiting
  state modifier IS selected.
* This replaces the disabling of the dropdown which previously
  encouraged the correct behaviour.
Copy link
Member

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion for simplifying some stuff at oliver-sanders#122

Couple of points on the glob matching:

  • How are users going to know that you can use globs?
  • Glob matching is not working in a weird way for the table view:
Image

}
return new RegExp(
// escape any regex characters in the glob
// NOTE: the first character is escaped using the `\x` syntax, so we
Copy link
Member

@MetRonnie MetRonnie Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These comments are somewhat confusing without some background reading

Suggested change
// NOTE: the first character is escaped using the `\x` syntax, so we
// NOTE: see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/escape
// The first character is escaped using the `\x` syntax, so we

Comment on lines +54 to +56
.replace(/\\\[\\x21([^]*)\\\]/, '[^$1]')
// `[^X]` -> `[^X]`
.replace(/\\\[([^]*)\\\]/, '[$1]')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backwards?

Suggested change
.replace(/\\\[\\x21([^]*)\\\]/, '[^$1]')
// `[^X]` -> `[^X]`
.replace(/\\\[([^]*)\\\]/, '[$1]')
.replace(/\\\[([^]*)\\\]/, '[$1]')
// `[!X]` -> `[^X]`
.replace(/\\\[\\x21([^]*)\\\]/, '[^$1]')

// NOTE: the first character is escaped using the `\x` syntax, so we
// prefix a space then subtract the first four characters (`\x20`)
// from the result
RegExp.escape(' ' + glob.trim())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm this is a very new feature: https://caniuse.com/wf-regexp-escape

BTW I think our ESLint plugin for browser compatibility is not working for newer features. See #2172 (comment)

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.

filtering: task modifier filtering views: filter tasks by pattern Make filter controls "sticky"

3 participants