-
Notifications
You must be signed in to change notification settings - Fork 5
Add useRunSearchParams hook for pipeline run filtering #1704
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
1f60f2c to
57a994f
Compare
| const setFilterDebounced = <K extends keyof PipelineRunFilters>( | ||
| key: K, | ||
| value: PipelineRunFilters[K], | ||
| ) => { | ||
| if (debounceTimeoutRef.current) { | ||
| clearTimeout(debounceTimeoutRef.current); | ||
| } | ||
|
|
||
| debounceTimeoutRef.current = setTimeout(() => { | ||
| setFilter(key, value); | ||
| debounceTimeoutRef.current = null; | ||
| }, DEBOUNCE_MS); | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical: Stale closure bug causes filter loss
setFilterDebounced captures the current setFilter function which has filters in its closure. If the URL changes between calling setFilterDebounced and the timeout firing (e.g., from another filter update), the stale filters will be used, causing data loss.
Scenario: User has status=RUNNING, calls setFilterDebounced('pipeline_name', 'test'), then immediately sets created_by=user1. When the debounced timeout fires, it uses the old filters without created_by, losing that filter.
Fix by reading fresh filters in the timeout callback:
const setFilterDebounced = <K extends keyof PipelineRunFilters>(
key: K,
value: PipelineRunFilters[K],
) => {
if (debounceTimeoutRef.current) {
clearTimeout(debounceTimeoutRef.current);
}
debounceTimeoutRef.current = setTimeout(() => {
// Re-parse filters from current URL to avoid stale closure
const currentFilterParam = isRecord(search)
? getStringOrUndefined(search.filter)
: undefined;
const currentFilters = parseFiltersFromUrl(currentFilterParam);
const newFilters = { ...currentFilters };
if (value === undefined || value === null || value === "") {
delete newFilters[key];
} else {
newFilters[key] = value;
}
updateUrl(newFilters);
debounceTimeoutRef.current = null;
}, DEBOUNCE_MS);
};| const setFilterDebounced = <K extends keyof PipelineRunFilters>( | |
| key: K, | |
| value: PipelineRunFilters[K], | |
| ) => { | |
| if (debounceTimeoutRef.current) { | |
| clearTimeout(debounceTimeoutRef.current); | |
| } | |
| debounceTimeoutRef.current = setTimeout(() => { | |
| setFilter(key, value); | |
| debounceTimeoutRef.current = null; | |
| }, DEBOUNCE_MS); | |
| }; | |
| const setFilterDebounced = <K extends keyof PipelineRunFilters>( | |
| key: K, | |
| value: PipelineRunFilters[K], | |
| ) => { | |
| if (debounceTimeoutRef.current) { | |
| clearTimeout(debounceTimeoutRef.current); | |
| } | |
| debounceTimeoutRef.current = setTimeout(() => { | |
| // Re-parse filters from current URL to avoid stale closure | |
| const currentFilterParam = isRecord(search) | |
| ? getStringOrUndefined(search.filter) | |
| : undefined; | |
| const currentFilters = parseFiltersFromUrl(currentFilterParam); | |
| const newFilters = { ...currentFilters }; | |
| if (value === undefined || value === null || value === "") { | |
| delete newFilters[key]; | |
| } else { | |
| newFilters[key] = value; | |
| } | |
| updateUrl(newFilters); | |
| debounceTimeoutRef.current = null; | |
| }, DEBOUNCE_MS); | |
| }; | |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
57a994f to
8d921a9
Compare
8d921a9 to
5de01ff
Compare

Description
Resolves: https://github.com/Shopify/oasis-frontend/issues/456
Added a new
useRunSearchParamshook to manage pipeline run search/filter state with URL synchronization. This hook provides functionality to parse, update, and serialize filter parameters in the URL, making it easier to maintain filter state across page refreshes and navigation.Type of Change
Checklist
Test Instructions
npm test src/hooks/useRunSearchParams.test.tsAdditional Comments
This PR also:
.local/directory to.gitignorefor local notes and scratch filessrc/types/pipelineRunFilters.tsto knip.json ignored files