Feeds filters controlled by Formik state. Closes #889#894
Feeds filters controlled by Formik state. Closes #889#894regulartim merged 4 commits intoGreedyBear-Project:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes Feeds filter <Select> controls so they no longer rely on a mutable module-level initialValues, and instead stay synchronized with component/Formik state (issue #889).
Changes:
- Replaced mutable
initialValueswith immutableDEFAULT_VALUESand introduced localfiltersstate. - Updated Feeds filter
<Select>values to be controlled viaformik.values. - Expanded/adjusted Feeds component tests to assert controlled select values and ordering/prioritize interactions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| frontend/src/components/feeds/Feeds.jsx | Reworks filter state management and wiring of <Select> controls to avoid shared mutable state. |
| frontend/tests/components/feeds/Feeds.test.jsx | Adds assertions and a new test to validate controlled filter behavior and ordering/prioritize interaction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
regulartim
left a comment
There was a problem hiding this comment.
Hey @UsamaElareeny ! Could you please react to copilot's suggestions? There not always helpful, I know, but I think it has at least one good point here: on some occasions you just bypass Formik. I think we should either use Formik as source of truth or remove it entirely. What do you think?
|
@regulartim Thanks for pointing this out! Yes, your're aboslutely right. I decided to go with this change though because it combine the However, I agree it's bit of a hybrid and mixing two sources. I would ref the Formik form as the single source of truth and removing I'll work on it right away and push shortly. |
|
Hi @regulartim, I refactored the component so that Formik is now the single source of truth.
The two versions are good, but this should address the concern about bypassing Formik. Could you please take another look and tell me if there is something else I need to do? Thanks! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // I batched url, tableParams, and tableKey into a single state object so that | ||
| // all three update atomically in one render cycle. Previously, having | ||
| // them as separate state variables caused multiple redundent api request before | ||
| // the state fully settled |
There was a problem hiding this comment.
That's fine, but please remove that comment. It does not belong in the code IMO.
There was a problem hiding this comment.
Sure, I’ll remove it. I grouped the state variables to avoid the previous redundant API requests, but I agree the explanation doesn’t need to stay in the code.
| // callbacks | ||
| const onSubmit = React.useCallback((values) => { | ||
| try { | ||
| setFeedsState((prev) => ({ | ||
| url: `${FEEDS_BASE_URI}/${values.feeds_type}/${values.attack_type}/${values.prioritize}.json?ioc_type=${values.ioc_type}`, | ||
| tableParams: { | ||
| feed_type: values.feeds_type, | ||
| attack_type: values.attack_type, | ||
| ioc_type: values.ioc_type, | ||
| prioritize: values.prioritize, | ||
| }, | ||
|
|
||
| // force remount FeedsTable | ||
| setTableKey((prev) => prev + 1); | ||
| } catch (e) { | ||
| console.debug(e); | ||
| } | ||
| }, | ||
| [setUrl, navigate], | ||
| ); | ||
| tableKey: prev.tableKey + 1, | ||
| })); | ||
| } catch (e) { | ||
| console.debug(e); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
You removed navigate({ search: "" }, { replace: true }); . Isn't that necessary to do what the comment says? Clear ordering and query params? (Just a question)
There was a problem hiding this comment.
@regulartim Thanks for taking the time to review. I strongly believe it's better this way since the user can remove the filters as he wants without programmatically removing it for him. Additionally, it keeps applying the filters, while enabling the user to change select values. Also helps with keeping formik as the single source of truth by not interfering in its re-render cycle.
regulartim
left a comment
There was a problem hiding this comment.
Looks good! Thanks for your work! 👍
… (GreedyBear-Project#894) * Fix: Feeds Select controlled by Formik values * test: add coverage for Formik-controlled Feeds filters * refactor: made Formik the single source of truth instead of filters/Formik * review: remove unnecessary explanatory comment
Description
This PR fixes the Feeds filters not being properly controlled by Formik state. Previously, I noticed that the
<Select>components relied on a mutable module-levelinitialValuesobject. This caused inconsistencies between the UI andformik.values, and introduced shared mutable state across renders.To summarize, in this PR:
initialValueswith an immutable default values object<Select>components controlled viaformik.valuesThe filters are now fully synchronized with Formik state and behave consistently.
Related issues
Closes #889
Type of change
Checklist
Formalities
<feature name>. Closes #999develop.develop.Docs and tests
Ruff) gave 0 errors. If you have correctly installed pre-commit, it does these checks and adjustments on your behalf.Note for reviewers: This PR fixes internal state handling for the Feeds filters. There are no changes to the user interface or visible behavior. All changes made are internal to ensure Formik values and UI are fully synchronized.