43469 frontend [ workflows ] Move Filters to Header #70
Conversation
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
Issue on line in frontend/src/public/components/UI/Select/FilterSelect.tsx:202:
isSelectAll can drift from props.selectedOptions, so the Select All checkbox stays checked and triggers resetFilter even when not all are selected. Consider deriving both the checked state and click handler from areAllSelected only to keep UI and behavior in sync.
🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
…component and getRenderPlaceholder into a separate function
…e table from a 'starters' filter to plain text
…maticapp/pneumaticworkflow into frontend/workflows/43469__move_filters_to_header
…GET /templates/titles-by-tasks and in the response rename the property workflows_count to count
| export type TGetTemplatesTitlesByTasksResponse = ITemplateTitleBaseWithCount[]; | ||
|
|
||
| export function getTemplatesTitlesByTasks(completionStatus: ETaskListCompletionStatus) { | ||
| const { |
There was a problem hiding this comment.
Destructuring api.urls assumes config exists. If getBrowserConfigEnv() returns {}, it throws. Consider guarding with a default or optional chaining before reading urls.
| const { | |
| const config = getBrowserConfigEnv(); | |
| const baseUrl = config?.api?.urls?.templatesTitlesByTasks ?? ''; | |
| const query = getTemplatesTitlesQueryString(completionStatus); | |
| const url = `${baseUrl}${query}`; |
🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
…property workflows_count to count
…ET /templates/titles-by-workflows and in the response rename the property workflows_count to count
- for the 'FilterSelect' component, add the ability to disable it - in 'workflowTable' component: replace the header in the 'performers' column in the table from a 'performers' filter to plain text - in the performers filter at the top: added the icon and added displaying text on the badge
Maria-Lordwill
force-pushed
the
frontend/workflows/43469__move_filters_to_header
branch
from
10114d7 to
eaf976f
Compare
| const getFilteredValues = () => { | ||
| const normalizedSearchText = searchText.toLowerCase(); | ||
|
|
||
| if (!groupedOptions) { |
There was a problem hiding this comment.
getFilteredValues ignores flatGroupedOptions: when groupedOptions is absent it uses options instead of allOptions, which desyncs the list from the "Select all" and placeholder. Consider returning/filtering allOptions here.
| if (!groupedOptions) { | |
| if (!groupedOptions) { | |
| if (!searchText) { | |
| return allOptions; | |
| } | |
| return getFilteredOptions(allOptions, normalizedSearchText); | |
| } |
🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
| case EHighlightsActions.SetTemplatesTitles: | ||
| return { ...state, templatesTitles: action.payload, isTemplatesTitlesLoading: false }; | ||
| case EHighlightsActions.LoadHighlights: | ||
| return { ...state }; |
There was a problem hiding this comment.
Reducers return new objects with no changes. Suggest returning state (including in default) to preserve referential equality and avoid unnecessary re-renders.
| return { ...state }; | |
| return state; |
🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
…3, remove actions.ts
| updateWorkflowsTemplateStepsCounters(); | ||
| } | ||
| }, [statusFilter, templatesIdsFilter, performersIdsFilter, workflowStartersIdsFilter]); | ||
| }, [statusFilter, performersIdsFilter, workflowStartersIdsFilter]); |
There was a problem hiding this comment.
useEffect that calls updateWorkflowsTemplateStepsCounters() omits templatesIdsFilter and stepsIdsFilter from its dependencies, so counters won’t refresh when templates/steps change. Consider adding both to the dependency array.
| }, [statusFilter, performersIdsFilter, workflowStartersIdsFilter]); | |
| }, [statusFilter, performersIdsFilter, workflowStartersIdsFilter, templatesIdsFilter, stepsIdsFilter]); |
🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
…to frontend/workflows/43469__move_filters_to_header
frontend/src/public/components/Workflows/WorkflowsGridPage/WorkflowsGridPage.tsx
Outdated
Show resolved
Hide resolved
frontend/src/public/components/Workflows/WorkflowsGridPage/WorkflowsGridPage.tsx
Outdated
Show resolved
Hide resolved
frontend/src/public/components/Workflows/WorkflowsGridPage/WorkflowsGridPage.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| def __init__( | ||
| self, | ||
| user: User, |
There was a problem hiding this comment.
user: User isn’t defined in this module (you use UserModel = get_user_model() above and other queries type as UserModel). This will raise NameError at import. Consider changing the annotation to UserModel for consistency.
| user: User, | |
| user: UserModel, |
🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
| if (changedFiltersRef.current.size > 0) { | ||
| changedFiltersRef.current = new Set(); | ||
| } | ||
| }, [changedFiltersRef.current.size]); |
There was a problem hiding this comment.
useLayoutEffect depends on changedFiltersRef.current.size, but mutating a ref doesn’t trigger re-renders, so the effect can miss updates. Consider keying the effect off the actual filter values (it already early-returns if nothing changed) so refs sync reliably.
| }, [changedFiltersRef.current.size]); | |
| }, [statusFilter, templatesIdsFilter, stepsIdsFilter, workflowStartersIdsFilter, performersIdsFilter, sorting]); |
🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
| {props.selectedOption && renderResetOption()} | ||
| {rendeSelectAllOption()} | ||
| {renderSelectAllOption()} | ||
| {foundValues.map((option) => { |
There was a problem hiding this comment.
isTitle options render as selectable items. Consider treating isTitle: true as a non-interactive header: render with the title style, disable the checkbox, skip handleChange, and set toggle={false} so clicks don’t select these items.
🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
| renderPlaceholder, | ||
| } = props; | ||
|
|
||
| const allOptions = flatGroupedOptions || options; |
There was a problem hiding this comment.
allOptions uses options when groupedOptions is provided without flatGroupedOptions, so the placeholder, Select All, and “all selected” checks operate on the wrong set. Consider deriving allOptions by flattening groupedOptions when present.
| const allOptions = flatGroupedOptions || options; | |
| const allOptions = flatGroupedOptions || (groupedOptions ? Array.from(groupedOptions.values()).flatMap(g => g.options) : options); |
🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
Maria-Lordwill
force-pushed
the
frontend/workflows/43469__move_filters_to_header
branch
from
f16d1a8 to
56e45de
Compare
| <> | ||
| {props.selectedOption && renderResetOption()} | ||
| {rendeSelectAllOption()} | ||
| {selectedOption !== null && renderResetOption()} |
There was a problem hiding this comment.
renderResetOption() now runs in multi-select because selectedOption !== null is true when selectedOption is undefined. Consider guarding by select mode (and nullish) so the reset option only shows for single-select.
| {selectedOption !== null && renderResetOption()} | |
| {!isMultiple && selectedOption != null && renderResetOption()} |
🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
| user: User, | ||
| with_tasks_in_progress: Optional[bool] = None, | ||
| workflows_status: Optional[WorkflowStatus] = None, | ||
| status: Optional[WorkflowStatus] = None, |
There was a problem hiding this comment.
status is typed as Optional[WorkflowStatus], but the serializer passes WorkflowApiStatus strings. This mismatch can cause a KeyError in WorkflowApiStatus.MAP[status]. Consider updating the type hint to Optional[WorkflowApiStatus] to reflect actual usage.
| status: Optional[WorkflowStatus] = None, | |
| status: Optional[WorkflowApiStatus] = None, |
🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
…rkflows update when selecting a group; also removed the unused CSS class dropdown__active-value
| return new Map( | ||
| selectedTemplates.map((template) => [ | ||
| template.id, | ||
| { |
There was a problem hiding this comment.
getRenderPlaceholder returns selectedOption?.name for ERenderPlaceholderType.Task, but name is a React element in flatGroupedOptions. This may render as [object Object]. Consider using a string field (e.g., selectedOption.searchByText or the raw step name) for the placeholder instead, or document that name must be a string here.
🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
Move workflows filters to the header by introducing Redux Toolkit slice, new filter select components, and replacing
/templates/titleswith/templates/titles-by-workflowsand/templates/titles-by-tasksreturningcountReplace legacy workflows actions with a Redux Toolkit slice, add header filter components (templates, starters, tasks, performers), and switch templates titles APIs to
/templates/titles-by-workflowsand/templates/titles-by-taskswith responses keyed bycount; backend adds corresponding viewset actions, queries, and serializers, and updates tests.📍Where to Start
Start with the workflows endpoints in
TemplateViewSetin template.py, then follow the queries in queries.py and the frontend slice in slice.ts.Changes since #70 opened
FilterSelectcomponent reset option rendering condition from truthy check to explicit null comparison and addedaria-labelattribute to clear button [f16d1a8]WorkflowsTableandWorkflowsLayoutcomponents [f16d1a8]ResizeObservercleanup and lifecycle management inWorkflowsTableandWorkflowsLayoutcomponents [f16d1a8]helpersmodule to use modern patterns and reduce complexity [f16d1a8]HighlightsFeedcomponent to avoid variable shadowing [f16d1a8]performersGroupsIdsFilterinto theWorkflowsLayoutcomponent by importing and usinggetWorkflowPerformersGroupsIdsFilterselector, tracking the filter value withprevPerformersGroupsIdsFilterRef, including it incurrentFiltersValuesRefanddependenciesRefs, adding it to the dependency array of the effect that triggersapplyFilters, and introducedisFirstRenderRefto ensureupdateCurrentPerformersCountersruns on the initial render regardless of filter changes [ec615ed]getWorkflowPerformersGroupsIdsFilterselector to retrievestate.workflows.workflowsSettings.values.performersGroupIdsFilterfrom Redux state [ec615ed].dropdown__active-valueCSS rule from the Select component stylesheet [ec615ed]📊 Macroscope summarized ec615ed. 43 files reviewed, 28 issues evaluated, 27 issues filtered, 1 comment posted
🗂️ Filtered Issues
backend/src/processes/queries.py — 0 comments posted, 1 evaluated, 1 filtered
user: Useron line 1738 usesUserwhich is not imported inbackend/src/processes/queries.py. The file showsUserModel = get_user_model()at line 34, andTemplateTitlesByEventsQueryat line 1674 correctly usesuser: UserModel. This inconsistency will cause aNameError: name 'User' is not definedwhen the module is loaded. [ Already posted ]backend/src/processes/views/template.py — 0 comments posted, 3 evaluated, 3 filtered
titlestotitles_by_workflows(as shown in code object 5's diff), but the permissions check at line 154 still references'titles'instead of'titles_by_workflows'. This causes thetitles_by_workflowsaction to fall through to the default permissions case at line 180-186, which requiresUserIsAdminOrAccountOwner()permission that wasn't required before. Non-admin users who could previously access this endpoint will now receive permission denied errors. [ Already posted ]get_permissionsmethod at line 154 still references the old action name'titles'instead of the new name'titles_by_workflows'. Sinceaction_serializer_classeswas changed to rename'titles'to'titles_by_workflows', the permission check will never match for thetitles_by_workflowsaction. This causes requests totitles_by_workflowsto fall through to the default permissions (lines 180-186) which incorrectly requiresUserIsAdminOrAccountOwner(), making the endpoint more restrictive than intended. [ Already posted ]get_permissions()method at line 154 checks for action name'titles', but the action was renamed to'titles_by_workflows'(line 488). This causes thetitles_by_workflowsaction to fall through to the default permissions at lines 180-186, which incorrectly requiresUserIsAdminOrAccountOwner()permission instead of the intended less restrictive permissions. Users who should have access to this endpoint will receive permission denied errors. [ Already posted ]frontend/src/public/components/TuneViewModal/TuneViewModal.tsx — 0 comments posted, 1 evaluated, 1 filtered
useEffectdependency array on line 53 is missingsavedFields. The effect usessavedFieldsto initializeselectedFieldsstate, but ifsavedFieldschanges in Redux whileisOpen,templateId, andtemplateTasksremain the same, the effect won't re-run and the component will display stale data. [ Out of scope ]frontend/src/public/components/UI/Select/FilterSelect.tsx — 0 comments posted, 2 evaluated, 2 filtered
groupedOptionsis provided butflatGroupedOptionsis not,allOptionsfalls back tooptions(line 108). However, the actual selectable options exist withingroupedOptions.values().options. This meansrenderPlaceholder(allOptions)at line 374, the select all functionality at lines 247-250, and theareAllSelectedcheck at lines 257-261 will operate on the wrong set of options, potentially causing incorrect counts, missing selections, or the select all feature to not work correctly with grouped options. [ Already posted ]props.selectedOption && renderResetOption()toselectedOption !== null && renderResetOption(). In multiple select mode (isMultiple: true),selectedOptionis typed asneverand will beundefinedat runtime. Sinceundefined !== nullevaluates totrue,renderResetOption()will now be called in multiple select mode when it wasn't before. IfnoValueLabelis provided, this will render an unnecessary reset option item in the dropdown for multi-select, which is likely unintended. [ Already posted ]frontend/src/public/components/UI/Select/SelectMenu.tsx — 0 comments posted, 1 evaluated, 1 filtered
radioStyles['select-menu__radio--checked']on line 97 appears to reference a class that likely doesn't exist inRadioButton.css. The naming conventionselect-menu__radio--checkedis inconsistent with the other radio style classes used (radio,radio__box,radio__title) which follow standard BEM naming. This would result in the checked state styling not being applied whenwithRadiois true and an item is selected. [ Low confidence ]frontend/src/public/components/Workflows/WorkflowsGridPage/WorkflowsGridPage.tsx — 0 comments posted, 4 evaluated, 4 filtered
useCallback(debounce(debounceTime, onSearch), [])has an empty dependency array but capturesonSearchanddebounceTime. IfonSearchprop changes, the debounced function will continue calling the stale callback. The dependency array should includeonSearchanddebounceTime, or useuseRefto hold the latest callback. [ Out of scope ]key={item}prop on line 147 and 155 will beundefinedfor each item since the array contains empty slots. Even if the iteration bug is fixed, using the item value as a key when items are allnullor identical values will cause React key warnings and potential rendering issues. Should use the index:map((_, index) => <WorkflowCardLoader key={index} />). [ Out of scope ]Array(INIT_SKELETION_QUANTITY).map()does not iterate over empty slots created byArray(n). The callback is never executed, soloaderwill be an empty array and no skeleton loaders will render. Should useArray.from({ length: INIT_SKELETION_QUANTITY })orArray(INIT_SKELETION_QUANTITY).fill(null).map(). [ Out of scope ]Array(SCROLL_LOADERS_QUANTITY).map()has the same bug - empty slots are not iterated. The infinite scroll loader will never display skeleton cards. Should useArray.from({ length: SCROLL_LOADERS_QUANTITY })orArray(SCROLL_LOADERS_QUANTITY).fill(null).map(). [ Out of scope ]frontend/src/public/components/Workflows/WorkflowsTablePage/WorkflowsTable/WorkflowsTable.tsx — 0 comments posted, 3 evaluated, 3 filtered
savedGlobalWidthsandsavedOptionalWidthsare read fromlocalStorageon every render (lines 99-104) rather than being memoized. WhencurrentUseris initiallynull/undefinedduring auth loading, the localStorage key becomesworkflow-column-widths-undefined-global, potentially loading incorrect cached data. Once auth completes andcurrentUser.idis available, the component re-reads localStorage with the correct key, but the initial render already used stale/wrong widths which are then used to initialize column widths in theuseEffectat lines 378-393. [ Out of scope ]debounceOnSearchcallback at line 112 has an empty dependency array[], but it capturesonSearchfrom props. If the parent component passes a differentonSearchfunction on re-render, the debounced callback will continue using the stale original reference, potentially calling an outdated handler. [ Low confidence ]createResizeHandleris called withcurrentUser?.idandtemplatesIdsFilter[0]. IfcurrentUserisnull/undefined(before auth state loads) or iftemplatesIdsFilteris empty, these will beundefined. This passesundefinedto the resize handler which then saves column widths to localStorage with keys containing 'undefined', causing width data to be saved to wrong keys and lost when the user/template values become available. [ Already posted ]frontend/src/public/components/Workflows/WorkflowsTablePage/WorkflowsTable/WorkflowsTableActions.tsx — 0 comments posted, 1 evaluated, 1 filtered
isDisabledlogic was changed from&&to||operators, fundamentally altering when the tune view button is disabled. Previously: disabled whentemplatesIdsFilter.length !== 1AND(status === Loaded || status === EmptyList). Now: disabled whentemplatesIdsFilter.length !== 1ORstatus === LoadingList. This means duringLoadingNextPagewith multiple templates, the button is now enabled (was disabled), and duringLoadingListwith exactly 1 template, the button is now disabled (was enabled). If the old behavior was correct, this introduces a regression. [ Low confidence ]frontend/src/public/components/icons/PerformerFilterIcon.tsx — 0 comments posted, 1 evaluated, 1 filtered
React.SVGAttributes<SVGElement>as its props type, which allows callers to pass any SVG attribute (likeonClick,aria-label,style, etc.), but onlyclassNameis destructured and passed to the<svg>element. All other props are silently ignored, which could lead to unexpected behavior if callers expect those props to be applied. [ Already posted ]frontend/src/public/components/icons/StartRoundIcon.tsx — 0 comments posted, 1 evaluated, 1 filtered
fillandclassNamefrom props, discarding all other props. Previously, the component used...restspread to forward additional props (likeonClick,aria-label,style,id,tabIndex, etc.) to the underlying<svg>element. Any code that passes additional props toStartRoundIconwill now silently ignore those props, breaking event handlers, accessibility attributes, and styling. [ Already posted ]frontend/src/public/components/icons/TableViewIcon.tsx — 0 comments posted, 1 evaluated, 1 filtered
React.SVGAttributes<SVGElement>as props but only destructuresfillwithout spreading remaining props to the<svg>element. If callers pass other standard SVG attributes likeclassName,style,onClick, oraria-label, they will be silently ignored, causing unexpected behavior. [ Low confidence ]frontend/src/public/layout/WorkflowsLayout/PerformerFilterSelect.tsx — 0 comments posted, 2 evaluated, 2 filtered
useMemodependency array[groups, performersCounters]on line 57 should includeperformersCountersMapsince that variable is used inside the memoized callback. While this works due to the indirect dependency chain (whenperformersCounterschanges,performersCountersMapis recalculated), React's hooks rules expect direct dependencies to be listed. [ Low confidence ]useMemodependency array[activeUsers, performersCounters]on line 79 usesperformersCountersMapinside the callback but listsperformersCountersas the dependency instead ofperformersCountersMap. This is the same indirect dependency issue as theperformersGroupOptionsmemoization. [ Already posted ]frontend/src/public/layout/WorkflowsLayout/TaskFilterSelect.tsx — 1 comment posted, 2 evaluated, 1 filtered
...(count && { count })on line 52 will not includecountwhen its value is0because0is falsy. Ifcount: 0is a valid value that should be displayed (e.g., showing that a step has zero workflows), this value will be omitted from the options object. [ Already posted ]frontend/src/public/layout/WorkflowsLayout/WorkflowsLayout.tsx — 0 comments posted, 3 evaluated, 3 filtered
currentFiltersValuesRefis initialized with prop/selector values at line 92-100, but these values become stale immediately after initialization since refs don't update on re-renders. The synchronization effect at lines 271-289 updates this ref, but there's a timing window during the first render wherecurrentFiltersValuesRef.currentmay have stale values if accessed by theuseLayoutEffectat line 297 before the sync effect runs. [ Low confidence ]useEffectat line 230-234 that callsupdateWorkflowsTemplateStepsCounters()hadtemplatesIdsFilterremoved from its dependency array (replaced withstepsIdsFilter). If step counters need to be recalculated when the template filter changes, this is now broken - changingtemplatesIdsFilterwill no longer triggerupdateWorkflowsTemplateStepsCounters(). [ Already posted ]changedFiltersRef.current.sizeas a dependency inuseLayoutEffect(line 311) is unreliable. React's dependency array comparison does not track ref mutations - refs don't trigger re-renders. While this may work incidentally when other state changes cause re-renders, ifchangedFiltersRef.currentis modified without an accompanying state change that causes a re-render, this effect won't execute. This could leave theprev*Refvalues stale, causingcheckFilterDependenciesChangedto return incorrect results in subsequent renders. [ Already posted ]frontend/src/public/layout/WorkflowsLayout/utils.ts — 0 comments posted, 1 evaluated, 1 filtered
selectedOptionisundefined(because no option matchesfilterIds[0]),getUserFullName(selectedOption)returns an empty string''instead of a meaningful fallback message. This results in an empty/blank placeholder in the UI, unlike theERenderPlaceholderType.Task/Templatebranches which return a fallback message containing'sorting.unknown-filter-value'. [ Already posted ]frontend/src/public/redux/workflows/slice.ts — 0 comments posted, 1 evaluated, 1 filtered
isObjectChangedfunction inhelpers.tscallsisObjectEmpty, butisObjectEmptywas removed from that file (shown in the diff). This will cause a runtimeReferenceErrorwhencheckFiltersChangedis called, which happens whenever any workflow filter is updated viaupdateWorkflowsFilterValue(e.g.,setFilterStatus,setFilterTemplate, etc.). [ Low confidence ]