Add suspense boundary around search page results #101

acouch · 2024-06-24T21:50:42Z

Summary

Fixes #59

Time to review: 30 mins

Changes proposed

This makes the search page static and adds a suspense boundary for the data being fetched by the server. The data comes from the API and is called from 3 components:

This also simplifies the state model by pushing state changes directly to the browser query params and rerendering the changed items. This makes things a lot simpler and thus a lot of state management code is removed and there results list is no longer wrapped in a form and passing form refs between components. This is the recommended approach by next: https://nextjs.org/learn/dashboard-app/adding-search-and-pagination

There are several items that needed to be shared among the client components: the query, total results count, and total pages. These are wrapped in a <QueryProvider /> that updates the state of these items. This was added so that if someone enters a query in the text box and the clicks a filter their query is not lost, so that the "N Opportunities" text doesn't need to be rerendered when paging or sorting, and so that the pager stays the same length when paging or sorting.

The data is fetched a couple of times in a duplicative fashion, however this follows NextJS best practice since the requests are cached.

The pager has been updated to reload only when there is a change in the page length. Because of an issue with the way the pager renders, it is unavailable while data is being updated:

This is because the Truss React component switches between a link and a button as it renders and there isn't an option to supply query arguments, so if a user where to click it they would lose the query params.

Overall this puts us on nice footing for the upcoming work using NextJS best practice.

github-actions · 2024-07-05T22:31:15Z

Coverage report for `./frontend`

St.^❔	Category	Percentage	Covered / Total
🟢	Statements	89.91% (+6.19% 🔼)	695/773
🟡	Branches	71.18% (+4.19% 🔼)	205/288
🟢	Functions	86.31% (+9.24% 🔼)	145/168
🟢	Lines	90.59% (+6.77% 🔼)	655/723

Show new covered files 🐣

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / QueryProvider.tsx	94.44%	100%	80%	94.12%
🟡	`...` / SearchPagination.tsx	66.67%	71.43%	33.33%	66.67%

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🟡	`...` / SearchFilterAccordion.tsx	70.27% (-6.65% 🔻)	25% (-25% 🔻)	54.55% (+4.55% 🔼)	70.59% (-4.41% 🔻)
🟢	`...` / SearchFilterSection.tsx	84% (-3.1% 🔻)	55.56% (-30.16% 🔻)	66.67% (-6.06% 🔻)	83.33% (-3.33% 🔻)
🟢	`...` / useSearchParamUpdater.ts	96% (-4% 🔻)	71.43% (-28.57% 🔻)	100%	96% (-4% 🔻)
🟢	`...` / SearchBar.tsx	83.33% (-16.67% 🔻)	33.33% (-66.67% 🔻)	75% (-25% 🔻)	90.91% (-9.09% 🔻)
🟢	`...` / SearchResultsHeader.tsx	88.89% (-11.11% 🔻)	60% (-40% 🔻)	100%	100%

Test suite run success

154 tests passing in 47 suites.

Report generated by 🧪jest coverage report action from 78da679

andycochran · 2024-07-08T15:30:04Z

frontend/src/app/services/search/FormDataService.ts

+    // Pass 'true' to handle the special case of splitting 'funding-instrument-'after the second dash
+    return this.getValuesByKeyPrefix("funding-instrument-", true);


Should funding-instrument- be funding_instrument- (with an underscore) instead? I imagine this'll come up again. We might want to establish a standard for how we format our query parameters. Tho I'm not sure if there are downsides to underscores.

~~Is this not a concern for search-sort-by?~~

Edit: I misunderstood this function and should've read more closely before commenting. You can disregard my underscore comment. It doesn't matter for query param presentation

This file was removed actually.

andycochran · 2024-07-08T15:40:08Z

frontend/src/app/services/search/searchfetcher/APISearchFetcher.ts

+  ): Promise<SearchAPIResponse> {
+    try {
+      // Keep commented in case we need to simulate a delay to test loaders
+      await new Promise((resolve) => setTimeout(resolve, 6250));


This should be commented out?

Yes, thanks.

andycochran · 2024-07-08T15:47:26Z

frontend/src/components/Footer.tsx

-              className="maxh-15 margin-bottom-2 tablet:margin-bottom-0"
+              className="margin-bottom-2 tablet:margin-bottom-0"


Why was . maxh-15 removed? It's there so that the logo doesn't grow too large on mobile. Larger breakpoints add a max size. But on mobile the image will just fill the allotted space and be HUGE.

The max width creates an error in the console:

warn-once.js:16 Image with src "/_next/static/media/grants-gov-logo.7f89963e.png" has either width or height modified, but not the other. If you use CSS to change the size of your image, also include the styles 'width: "auto"' or 'height: "auto"' to maintain the aspect ratio

I removed the update and I created a separate ticket for it since it is out of scope for this ticket: #165

rylew1 · 2024-07-08T16:46:39Z

Would be curious to see this diagram updated if you have time: https://drive.google.com/file/d/14iXrcg5EsEvnqqhUeVgR-5F_X8PASw4d/view?usp=sharing

andycochran · 2024-07-08T18:00:59Z

frontend/src/hooks/useSearchParamUpdater.ts

  const updateQueryParams = (
    queryParamValue: string | Set<string>,
    key: string,
+    queryTerm: string | null | undefined,
+    scroll = false,


scroll is a query param?

I documented the reason for this. It is implemented in the bottom pager so that when the user clicks the pager, they aren't left at the bottom of the screen when the results reload.

acouch · 2024-07-26T20:39:04Z

@rylew1

As noted above, the workflow is:

request comes, static version of the page is rendered
search page params are args to search page
static client components are rendered on the server
components within the suspense boundary ask for their data
data is fetched from the server (no change here) and results are streamed to the client
client sees results

When a user makes a change, nothing happens on the server. The query params are captured and pushed to update the new URL and the process from above is repeated.

rylew1 · 2024-07-29T16:22:53Z

Is there a way to get the draw.io link or a larger picture of the updated diagram?

acouch · 2024-07-30T15:38:19Z

@rylew1 here is the file: https://drive.google.com/file/d/1ZPzAm4lZxfW5db9lt74mRCw73w-nbp4X/view?usp=sharing

btabaska · 2024-07-30T16:23:25Z

frontend/src/app/[locale]/search/error.tsx

 import PageSEO from "src/components/PageSEO";
-import { QueryParamData } from "src/services/search/searchfetcher/SearchFetcher";
+import QueryProvider from "./QueryProvider";


Should be changed to import QueryProvider from "src/app/[locale]/search/QueryProvider"; to match expected import pattern

@btabaska if the file is in the same directory I'm not sure if it is better to use the full string or not. I created #91 to decide on a standard. Can we punt to that ticket?

Yep, any of these nitpicks are just suggestions. Feel free to not incorporate into final feedback

I updated anyway, we can figure out the convention in #91 and ideally enforce with a linter.

btabaska · 2024-07-30T16:26:52Z

General feedback- this PR is way too large and in the future we should consider creating team standards around PR size. OpenSource.net has a great article with research that shows that we should keep PRs between 10-250 lines of code to make reviewing more effective.

btabaska

Lots of small nitpicks across the changes. It was hard to review this as a whole since the changes are so sweeping. We should likely break up changes like this into 5-6 smaller more focused PRs in the future to make the review process more effective. Great changes overall though, and running it locally this seems like a major improvement. Bravo!

frontend/src/components/search/SearchBar.tsx

btabaska · 2024-07-30T16:33:15Z

frontend/src/components/search/SearchBar.tsx

 import { Icon } from "@trussworks/react-uswds";
-import { useSearchParamUpdater } from "../../hooks/useSearchParamUpdater";
-import { useState } from "react";
-import { sendGAEvent } from "@next/third-parties/google";


Is there a reason that we are deprecating code changes for Google Analytics?

@btabaska this was moved to updateQueryParams()https://github.com/navapbc/simpler-grants-gov/pull/101/files/df65dbcd61af66cbbd532950994fcec69e9fee1f#diff-2b80310ac8bc745891bc29ca3d584cec3bd2984f0117b59aa2148625e9ab856dR43

btabaska · 2024-07-30T16:33:49Z

frontend/src/components/search/SearchBar.tsx

  const { updateQueryParams } = useSearchParamUpdater();

  const handleSubmit = () => {
-    updateQueryParams(inputValue, "query");
-    sendGAEvent("event", "search", { search_term: inputValue });


See above, if we deprecate the GAEvent it will break Google Analytics support.

Thanks for noting. See comment above.

btabaska · 2024-07-30T16:37:20Z

frontend/src/components/search/SearchFilterAccordion/SearchFilterAccordion.tsx

 interface SearchFilterAccordionProps {
-  initialFilterOptions: FilterOption[];
+  options: FilterOption[];


What are your thoughts around "options" as the variable name? It might come across as confusing to developers working in the codebase in the future to have such a generic name. Since we have so many different types of options across the codebase, it might be more clear to keep the name "filterOptions" or something?

@btabaska since the interface is SearchFilterAccordionProps it seems cleaner to me to have it as just options as it would follow that they are options for SearchFilterAccordion. I changed it because the "initial" didn't make sense anymore as these are not capturing state. It is great to keep in mind community devs and like filterOptions makes more sense for you so updating it.

btabaska · 2024-07-30T16:41:47Z

...tend/src/components/search/SearchFilterAccordion/SearchFilterSection/SearchFilterSection.tsx

-
-import { FilterOption } from "../SearchFilterAccordion";
+import { useState } from "react";
+import { FilterOptionWithChildren } from "../SearchFilterAccordion";


This should be updated to use the absolute import path from src

Updated, thx.

btabaska · 2024-07-30T16:53:31Z

frontend/src/components/search/SearchResultsHeader.tsx

-        {searchResultsLength} Opportunities
+      <h2
+        className="tablet-lg:grid-col-fill margin-top-5 tablet-lg:margin-top-2 tablet-lg:margin-bottom-0"
+        style={{ opacity: loading ? 0.5 : 1 }}


Is this something that we can achieve by using our existing style or by creating a custom USWDS style? It is usually not best practice to style inline because it can cause styling creep over time that makes it hard to parse where CSS is being applied from.

Updated, thanks for noting.

btabaska · 2024-07-30T16:53:56Z

frontend/src/components/search/SearchResultsHeaderFetch.tsx

+"use server";
+import { getSearchFetcher } from "src/services/search/searchfetcher/SearchFetcherUtil";
+import { QueryParamData } from "src/services/search/searchfetcher/SearchFetcher";
+import SearchResultsHeader from "./SearchResultsHeader";


This should be updated to use an absolute path.

btabaska · 2024-07-30T16:57:09Z

frontend/src/components/search/SearchResultsListFetch.tsx

+  }
+
+  if (searchResults.data.length === 0) {
+    return (


These strings need to be migrated into locales so that translate will work on them. I don't think we want a pattern where some text is located there and some is located inline to the file.

@btabaska I agree, they were not translated originally so was trying to keep the scope of the PR down, even as large as it is. I created a follow up: #167

btabaska · 2024-07-30T16:57:48Z

frontend/src/components/search/SearchResultsListFetch.tsx

+      {maxPaginationError && (
+        <h4>
+          {
+            "You''re trying to access opportunity results that are beyond the last page of data."


This string needs to be migrated into locales, we don't want inline text that will not be seen by translate and causes confusion on where we locate text.

btabaska · 2024-07-30T16:59:56Z

frontend/src/hooks/useSearchParamUpdater.ts


+    sendGAEvent("event", "search", { key: finalQueryParamValue });


"key" needs to be changed either to "search_term" to match what we have in GA or it needs to be updated in GA to "key". GA won't pick up or understand what "key" means on its own. I'd recommend we change it to search_term here since "key" is generic and means a lot of different things.

Updated, thanks!

acouch · 2024-07-30T17:08:10Z

@btabaska I agree but not sure how to break this up. The key items are to render the page statically, which wasn't possible w/o introducing the suspense items. Introducing the suspense items one at a time would take a ton of extra work. This takes advantage of the downtime.

btabaska · 2024-07-30T17:10:33Z

@btabaska I agree but not sure how to break this up. The key items are to render the page statically, which wasn't possible w/o introducing the suspense items. Introducing the suspense items one at a time would take a ton of extra work. This takes advantage of the downtime.

Totally fine, and agreed. I just wanted to point that out as a general concern. If we were in a live environment and not during downtime I might suggest a feature branch or something. However, in this instance it isn't a big deal.

btabaska

LGTM!

Fixes #59 This makes the search page static and adds a suspense boundary for the data being fetched by the server. The data comes from the API and is called from 3 components: * [`<SearchPaginationFetch />`](https://github.com/navapbc/simpler-grants-gov/pull/101/files#diff-9dbdda5096b97ad049cccea24c5a046581d26c151a6f94fcc32c05cb33ee9dee) * [`<SearchResultsHeaderFetch />`](https://github.com/navapbc/simpler-grants-gov/pull/101/files#diff-14a084f66c050414cc2bbd0875256511630438971022073301bbfe91c4aa8cd1) * [`<SearchResultsListFetch />`](https://github.com/navapbc/simpler-grants-gov/pull/101/files#diff-aabe6a7d19434a9b26199430bbcde5d31a0790aebc4cd844b922ac2fa1348dce) This also simplifies the state model by pushing state changes directly to the browser query params and rerendering the changed items. This makes things a lot simpler and thus a lot of state management code is removed and there results list is no longer wrapped in a form and passing form refs between components. This is the recommended approach by next: https://nextjs.org/learn/dashboard-app/adding-search-and-pagination There are several items that needed to be shared among the client components: the query, total results count, and total pages. These are wrapped in a `<QueryProvider />` that updates the state of these items. This was added so that if someone enters a query in the text box and the clicks a filter their query is not lost, so that the "N Opportunities" text doesn't need to be rerendered when paging or sorting, and so that the pager stays the same length when paging or sorting. The data is fetched a couple of times in a duplicative fashion, however this follows [NextJS best practice](https://nextjs.org/docs/app/building-your-application/rendering/composition-patterns#sharing-data-between-components) since the requests are cached. The pager has been updated to reload only when there is a change in the page length. Because of an issue with the way the pager renders, it is unavailable while data is being updated: <img width="1229" alt="image" src="https://github.com/navapbc/simpler-grants-gov/assets/512243/a097b0e2-f646-43b5-bc5a-664db02780a2"> This is because the Truss React component [switches between a link and a button as it renders](https://github.com/trussworks/react-uswds/blob/main/src/components/Pagination/Pagination.tsx#L42) and there isn't an option to supply query arguments, so if a user where to click it they would lose the query params. Overall this puts us on nice footing for the upcoming work using NextJS best practice.

github-actions bot added typescript frontend labels Jun 24, 2024

acouch changed the title ~~Add suspense boundary around search page~~ Add suspense boundary around search page results Jun 26, 2024

acouch mentioned this pull request Jun 26, 2024

[Task]: Refactor useSearchFilter logic #93

Closed

acouch marked this pull request as ready for review July 5, 2024 23:01

acouch requested a review from andycochran as a code owner July 5, 2024 23:01

andycochran reviewed Jul 8, 2024

View reviewed changes

acouch mentioned this pull request Jul 23, 2024

[Bug]: Browser Console Error "Image with src "/_next/static/media/grants-gov-logo.7f89963e.png" has either width or height modified" #165

Closed

1 task

acouch force-pushed the acouch/search-suspense branch 2 times, most recently from 3f97eda to df65dbc Compare July 26, 2024 17:31

acouch requested a review from andycochran July 29, 2024 14:28

andycochran requested a review from btabaska July 29, 2024 14:29

btabaska reviewed Jul 30, 2024

View reviewed changes

btabaska suggested changes Jul 30, 2024

View reviewed changes

acouch added 5 commits July 30, 2024 13:55

Add look for temp search page stand-in

1c379e6

Add sorting and layout update

9b527a7

Add query provider

6685ed3

Adds status to look search page

91632c4

Clean up status WIP

0dc625e

acouch added 18 commits July 30, 2024 13:55

Update page.tsx

5f8e662

Update SearchOpportunityAPI.ts

419113d

Format fixes

2736162

Update page.tsx

c5000b3

Add search result header into suspense

70ead97

Lint fix

72be27f

Update tests

f9ee121

Update tests

b24e8a3

Fix e2e tests

6243b10

Update format

28ebc9a

Lint fix

5fa441f

Remove duplicate folder

3a62b6f

Update useSearchParamUpdater.ts to include comments

956e0fa

Update Footer.tsx re-add maxh for logo

924d447

Increase timeout for playrwight

8a076cb

Break up slow tests

d1a4cec

Add review suggestions

6d51e0a

Update SearchFilterAccordion.test.tsx

57f9c81

acouch force-pushed the acouch/search-suspense branch from 94cbc76 to 57f9c81 Compare July 30, 2024 18:17

Update SearchResultsHeader.tsx

711c24c

acouch force-pushed the acouch/search-suspense branch from e011b11 to 711c24c Compare August 6, 2024 16:12

Update SearchResultsHeader.tsx

78da679

btabaska self-assigned this Aug 6, 2024

btabaska approved these changes Aug 6, 2024

View reviewed changes

acouch merged commit 9eb8aa2 into main Aug 6, 2024
12 checks passed

acouch deleted the acouch/search-suspense branch August 6, 2024 18:03

acouch mentioned this pull request Sep 17, 2024

[Bug]: Browser Console Error "Image with src "/_next/static/media/grants-gov-logo.7f89963e.png" has either width or height modified" HHS/simpler-grants-gov#2099

Closed

1 task

		// Pass 'true' to handle the special case of splitting 'funding-instrument-'after the second dash
		return this.getValuesByKeyPrefix("funding-instrument-", true);

		className="maxh-15 margin-bottom-2 tablet:margin-bottom-0"
		className="margin-bottom-2 tablet:margin-bottom-0"


		sendGAEvent("event", "search", { key: finalQueryParamValue });

Add suspense boundary around search page results #101

Add suspense boundary around search page results #101

Conversation

acouch commented Jun 24, 2024 • edited Loading

Summary

Time to review: 30 mins

Changes proposed

github-actions bot commented Jul 5, 2024 • edited Loading

Coverage report for ./frontend

Test suite run success

Choose a reason for hiding this comment

andycochran Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acouch Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

rylew1 commented Jul 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acouch commented Jul 26, 2024 • edited Loading

rylew1 commented Jul 29, 2024

acouch commented Jul 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

btabaska commented Jul 30, 2024

btabaska left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acouch commented Jul 30, 2024

btabaska commented Jul 30, 2024

btabaska left a comment

Choose a reason for hiding this comment

acouch commented Jun 24, 2024 •

edited

Loading

github-actions bot commented Jul 5, 2024 •

edited

Loading

Coverage report for `./frontend`

andycochran Jul 8, 2024 •

edited

Loading

acouch Jul 23, 2024 •

edited

Loading

acouch commented Jul 26, 2024 •

edited

Loading