Skip to content

fix(filters): preserve non-default null filter state in useUrlState to fix the "All" filter - #417

Open
RaminGe wants to merge 3 commits into
orangecoding:masterfrom
RaminGe:fix/filter-all-button
Open

fix(filters): preserve non-default null filter state in useUrlState to fix the "All" filter#417
RaminGe wants to merge 3 commits into
orangecoding:masterfrom
RaminGe:fix/filter-all-button

Conversation

@RaminGe

@RaminGe RaminGe commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Summary

Nice new filter update! I noticed the "All" activity button does not work (actually already before, just forgot to report it).
This PR fixes the issue where clicking the "All" activity filter (active: null) failed to stay active and immediately reset to active: true on re-render.

Root Cause

useUrlState unconditionally deleted null values from search parameters. For filters like active whose defaultValue is true, removing the key from the URL caused useUrlState to fall back to true, preventing active: null from being retained.

Fix

  • Encode null as "all" in parseNullableBoolean.stringify.
  • Preserve non-default null parameter state in useUrlState.setValues so active=all persists in the URL.
  • Added unit tests for active=all URL serialization and reading.

@orangecoding

Copy link
Copy Markdown
Owner

@RaminGe

Thanks for tracking this down, the diagnosis is spot on ;) I applied it locally and the "All" button works.

The null -> "all" encoding is the right call. active defaults to true, so absence of the param already means "active only", which leaves null with no way to be spelled in the URL. Giving it "all" fixes that, and it round-trips correctly since parse maps anything that isn't "true"/"false" back to null. Old bookmarks are unaffected.

One thing I'd like changed before merging. The diff also removes the value == null guard in setValues for all codecs, not just the nullable one. parseNumber.stringify and parseBoolean.stringify are bare String(v), so null now ends up in the URL literally:

set URL read back
distance = null distance=null NaN
page = undefined page=undefined NaN
buildings = null buildings=null false

Nothing triggers this today (the map's distance Select has no showClear), but it's a trap for whoever adds a clearable numeric filter next, and a NaN would go straight into the API query.

Suggestion: keep setValues exactly as you wrote it, and let each codec decide whether null means "absence" or "a value":

export const parseNumber = {
  parse: (v) => Number(v),
  stringify: (v) => (v == null ? null : String(v)),
};

export const parseBoolean = {
  parse: (v) => v === 'true',
  stringify: (v) => (v == null ? null : String(v)),
};

// For state that is null | true | false. Null is a real value here, not an absence:
// `active` defaults to true, so absence already means true and null needs a spelling.
export const parseNullableBoolean = {
  parse: (v) => (v === 'true' ? true : v === 'false' ? false : null),
  stringify: (v) => (v === null ? 'all' : String(v)),
};

That keeps your fix and puts the null handling where it belongs.

Could you also add:

  • a test that setValue('page', null) drops the param instead of writing "null"
  • a test that after "clear all filters" the URL keeps active=all while the active filter count reads 0 (that's the user-visible contract here)

Happy to merge once those are in. Nice catch on this one.

@RaminGe

RaminGe commented Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

@orangecoding makes sense! Added and pushed

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.

2 participants