Provide search link from the 404 page (#863)

[dan1994]

Add a button that redirects to /search?text=<QUERY> above the "Return to Homepage" button.

The button is wrapped by a paragraph so that it shows on its own line. The logic for calculating the search term is not obvious and is thus explained with a comment on top of the searchTerm variable.

The Open5eError has been refactored to be more gramatically correct, as it doesn't add the data property but just defines what the allowed values for it are.

Closes #863

How was this tested?

Manually

[dan1994]

Here's how it looks:

[calumbell]

Hi @dan1994 thanks for your contribution!

Testing it out on the Netlify test deploy and on local all is working as intended, nice job!

I have requested a few changes, mostly to do with code style. All quite simple fixes. Happy to make them myself if you do not have the bandwidth (please ping me if so), but otherwise I will leave them in your capable hands.

[calumbell]

This is a slightly weird mix of older and newer JavaScript syntax. Something like the following is preferred:

const { data } = props.error;
if (!data) return;

Returning null in Vue is not required and unidiomatic, an implicit undefined return is preferred

[calumbell]

I think this would look much better if we dropped the quotation marks and only bolded the search term:

Search for Goblin

Rather than:

Search for "Goblin"

[dan1994]

I made it italics instead since the rest of the text is already bold. I tried using stronger bold styles with tailwind (e.g. font-extrabold, font-black) but you it was impossible to see the difference. Let me know if this is good enough.

[calumbell]

Hi @dan1994, thanks for making these changes, we aren't quite there yet!

I have pushed a new commit to this PR to course-correct where it was simpler than going back and forth. You should be able to sync up your local fork by running git pull on your feature branch at the command-line.

• The return undefined have been replaced with bare return statement. This is an implicit undefined return and is considered more normal Javascript style.
• Tweaked highlighting of link to search page.
• Replaced references to statusCode to status because my IDE was screaming at me for using a deprecated attribute.

Outstanding work left on this PR includes:

1. Reading the JSON parsing code more carefully:

  try {
    return JSON.parse(data)?.path.replace(/.*\/([^/]*)/, '$1');
  } catch {
    return undefined;
  }

It is rather tangled. The try/catch block or the Regex find/replace is overkill, we can achieve the same thing with string methods and optional chaining. The following does the same thing and is much, much easier to follow:

const parsed = JSON.parse(data);
return parsed?.path?.split('/').at(-1);

2. The code works great for errors thrown by the useFindOne composable when it fails to find a single item in the Open5e data. However, when we hit the error page because a typo was made in the top-level page name we still see the search link as if it were an item that couldn't be located in the O5e data.

So this might happen if somebody was typing in open5e.com/monsters but made a mistake and ended up with nomsters. In this situation, the link to the search page is not helpful and should probably be hidden. Perhaps there is some way of using the error data to control the conditional renderering of the link to the search page?

[dan1994]

Hi @calumbell, just to clarify, do you want the link only for "API terms" (monsters, conditions, etc.) that were not found but not for frontend pages that were not found?
If so it's easy to fix, and will also removed the JSON parsing entirely, as it's needed just for the case where a frontend page is not found.

Instead of

try {
  return JSON.parse(data)?.path.replace(/.*\/([^/]*)/, '$1');
} catch {
  return undefined;
}

we can do

return;

and it will solve both issues.

[dan1994]

BTW, our IDEs are probably configured differently, because my IDE is shouting at me that Property 'status' does not exist on type 'Open5eError'.
I don't mind keeping it as is if it works, but maybe we can compare configs to figure out the discrepancy...
With regards to return unknown v.s. return the commit or push hook (don't remember which) blocked me when I tried to do that. If I run into this again I'll reach out.

[dan1994]

Ping @calumbell

[calumbell]

Thanks for your ping @dan1994

Hi @calumbell, just to clarify, do you want the link only for "API terms" (monsters, conditions, etc.) that were not found but not for frontend pages that were not found?

That is correct! The purpose of this feature is to catch fail state when the API fails to return something when requested.

The UX should be something like: "Oh hey, This thing you were looking for doesn't exist in our DB, maybe it is a dead link for edited the URL directly and made a mistake. Would you like to search for it instead? Might have more luck".

A typo in the top-level page (the bit of the URL before the unique ID) will also throw a 404 but we don't want to link to the search page when a user visits (for example) /monster instead of /monsters. The search results for "monster" don't get you any closer to the page you were looking for.

BTW, our IDEs are probably configured differently, because my IDE is shouting at me that Property 'status' does not exist on type 'Open5eError'.
I don't mind keeping it as is if it works, but maybe we can compare configs to figure out the discrepancy...
With regards to return unknown v.s. return the commit or push hook (don't remember which) blocked me when I tried to do that. If I run into this again I'll reach out.

Puzzling. So two different issues here, one with (a) TypeScript and one with (b) Huskies pre-commit.

For (a) see if anything interesting happens for the following:

• 1. That TS types haven't been generated correctly (try running npx nuxi prepare to generate these) or
• 2. Try importing the Open5eError type directly from composables :

import type { Open5eError } from '@/composables/api'
const props = defineProps<{ error: Open5eError | null }>();

(In an ideal Nuxt world, the auto-imports should just work)

For (b) I am baffled. Would you be able to share the console output for when the commit hook fails?