Skip to content

MOB-1346: advanced search screen - #3939

Open
sepeterson wants to merge 17 commits into
mainfrom
mob-1346-screen
Open

MOB-1346: advanced search screen#3939
sepeterson wants to merge 17 commits into
mainfrom
mob-1346-screen

Conversation

@sepeterson

@sepeterson sepeterson commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

I'll follow up with tests in a second PR. This one's long enough

As it is, you can't really tell you've got an advanced search applied on the results screen but that will change in https://linear.app/inaturalist/issue/MOB-1348/advanced-search-mode


// `label` / `text` are the two lines of each row in the sort sheet; `labelCaps`
// is the single-line, all-caps version in the dropdown button
export const getSortByValues = ( t: TFunction ) => ( {

@sepeterson sepeterson Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This file is mostly copied from ExploreV1's FilterModal.tsx


const { state: v2State, dispatch: dispatchV2 } = useExploreV2();
const [initialDraft] = useState( ( ) => draftFromV2State( v2State ) );
const [draft, dispatch] = useReducer( advancedSearchReducer, initialDraft );

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Like UniversalSearch, this screen holds a draft of future state locally and only commits it to the context when the user applies the search

case "SET_USER":
return {
...withFilters( draft, { user: action.user, excludeUser: null } ),
subject: withoutSubjectOfType( draft, "user", "unobserved" ),

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Lines like this only exist to translate an initial ExploreV2State set in UniversalSearch with a non-taxon subject. Not the most elegant part of this PR

@jtklein jtklein Aug 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Alright, I think I have understood why that is here now. Not the biggest fan of this. Thinking back I believe that was one of the reasons of the Explore V1 context to not split out any "substate" like a subject and just dump everything into a giant reducer.
It did have it's own problems it seems now: comparing V1 to your V2 reducer I can see a lo of drift in V1, meaning some of the action.keys are not even used anymore by any dispatch. So, maybe it was harder keeping track of what to pass in.
Anyway, not asking you to change anything here. Happy to move these things forward and just see where we end up with V2 once everything is tied up and we have tested the UI state for selecting and de-selecting several combinations of subject and subject-like-but-only-a-filter params.

@jtklein jtklein Aug 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Doesn't the comment of "only taxon is a subject" mean that when calling SET_PROJECT we also need to remove a potential user subject, and here we need to remove a project subject?
Or could we just remove the subject all together in initializing the Advanced Search?

@sepeterson
sepeterson marked this pull request as ready for review August 13, 2026 22:15

@jtklein jtklein left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Only a few minor requested changes inline.

The bigger review: I have also taken some time to compare ExploreContext to the new advancedSearchReducer plus ExploreV2Context. From what I can see most funcionality has been ported successfully. One thing to point out though:

V2 has no snapshoting and discarding back to.
When building Explore V1 the ask from product was that after pressing the “Accept” buttons in Advanced filters the results ideally should not need to load visibly but are already fetched in the background. That’s why all that apparatus with makeSnapshot, differsFromSnapshot, isNotInitialState, discardChanges exists. So, contrary to V2 where you make a draft and commit on “Accept” which means we will see a loading state when coming back. V1 was only closing the filters modal on “Accept” to the results screen with already loaded results. Which then in turn necessitated the machinery to make a snapshot of the state before entering the FilterModal, so that on "Discard" we can revert back to said snapshot. There is a trade-off between results delay and number of API calls made which was decided in the past by product. I haven't looked at all ExploreV2 tickets, so I am not sure if this change now is a decided change to be made. Anyways, I am not opposed to this way, the state flow is a lot easier to comprehend. So, if we prefer this way I am fine with it. If that has not come up yet in discussion with Product, we should at least keep them informed about the change.

When setting a place, we used to store place_guess as well. Do you think this is fully taken care of with place.display_name?

Very grateful for your mapping of realm objects to plain JS objects.

Unrelated to this code, but can we rename
buildExploreV2QueryParams
viewerId to currentUserId

months: null,
} );
case "SET_DATE_OBSERVED_RANGE":
return withFilters( draft, {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This used to have a isValidDateFormat check before settig to state. Is that something we can drop or have replaced with something else somewhere?
Also some other actions had that, I'll leave some Comments.

dateObserved: DATE_OBSERVED.ALL, observed_on: null, d1: null, d2: null, months: null,
} );
case "SET_DATE_OBSERVED_EXACT":
return withFilters( draft, {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

isValidDateFormat

} );
case "SET_DATE_OBSERVED_ALL":
return withFilters( draft, {
dateObserved: DATE_OBSERVED.ALL, observed_on: null, d1: null, d2: null, months: null,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add newlines

} );
case "SET_DATE_UPLOADED_ALL":
return withFilters( draft, {
dateUploaded: DATE_UPLOADED.ALL, created_on: null, created_d1: null, created_d2: null,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Newlines

months: action.months,
} );
case "SET_DATE_UPLOADED_ALL":
return withFilters( draft, {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

isValidDateFormat

<TopAndBottomInsetViewWrapper testID="AdvancedSearch">
<SearchHeader
headerText={t( "ADVANCED-SEARCH" )}
onClose={() => navigation.goBack()}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Haven't looked at the V2 tickets too closely. But comparing this to V1 don't we also want to copy over the WarningSheet? Meaning, if a user selects a bunch of filters here, but presses the back arrow instead of the SEARCH button we'd need to warn that all those filters will not be applied this way.

const selectedTaxon = iconicTaxa
?.filtered( "name ==[c] $0", taxonName );
const iconicTaxon = selectedTaxon && selectedTaxon.length > 0
? Taxon.mapRealmToPojo( selectedTaxon[0] )

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice!

const navigation
= useNavigation<ExploreStackScreenProps<"AdvancedSearch">["navigation"]>();
const { t } = useTranslation();
const iconicTaxa = useIconicTaxa();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice!

? subject.project
: null;
const displayProject = project || subjectProject;
const displayUserCountText = displayUser?.observations_count === undefined

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That seems like defensive overkill to me. If we don't have a user we wouldn't have to display the text. A user from the API should have an observations_count.

</View>
</View>
</ScrollView>
<ButtonBar>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

UI for this is not correct: there is no drop shadow and there is too much space at the bottom.
For a fix, I think you could copy the ButtonBar styling from the UniversalSearch screen, right?

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