Skip to content
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

🐛 Fix bug in FileExplorer that broke clicking on books. #481

Merged
merged 1 commit into from
Oct 16, 2024

Conversation

JMicheli
Copy link
Collaborator

I noticed an issue in the FileExplorer component that seems to have followed on the sdk overhaul. The function getBook defined here requires an sdk: Api parameter:

/**
* A function that attempts to fetch the book associated with the file, if any exists.
* The queryClient is used in order to properly cache the result.
*/
export const getBook = async (path: string, sdk: Api) => {
try {
const response = await queryClient.fetchQuery(
[sdk.media.keys.get, { path }],
() =>
sdk.media.get({
path: [path],
}),
{
// 15 minutes
cacheTime: 1000 * 60 * 15,
},
)
return response.data?.at(0) ?? null
} catch (error) {
console.error(error)
return null
}
}

However, a few calls to getBook didn't include the parameter, which caused them to fail. As a result, when double clicking a file in the explorer, you'd get this response:

image

The changes I made seem to fix it, but it would be good to have review by someone more familiar with React to make sure that any other changes that I may have missed are made.

@JMicheli JMicheli requested a review from aaronleopold October 16, 2024 01:11
Copy link
Collaborator

@aaronleopold aaronleopold left a comment

Choose a reason for hiding this comment

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

This looks perfect to me. Thanks for finding and fixing this issue!!

@@ -39,7 +40,7 @@ export default function FileExplorerProvider({ rootPath }: Props) {
setPath(entry.path)
} else {
try {
const entity = await getBook(entry.path)
const entity = await getBook(entry.path, sdk)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder why the lints didn't catch this. I'll have to revisit the frontend lint scripts

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a good point, I found the problem easily because ESLint threw an error in VS Code and underlined it in red for me.

Copy link
Collaborator Author

@JMicheli JMicheli Oct 16, 2024

Choose a reason for hiding this comment

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

Well actually here's your answer: typescript-eslint/typescript-eslint#352 (comment)

The salient part:

we purposely do not care about errors that should arise as a result of type checking errors. We only care that the code is syntactically valid (i.e. can be parsed without errors), not about whether it's semantically valid (i.e. don't care if it is type sound).

We need to use tsc to catch this sort of thing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahh yeah, okay. That makes sense. I could have sworn I defined a tsc script somewhere to check types but I'll take a look tomorrow

@JMicheli JMicheli merged commit 4187ec3 into develop Oct 16, 2024
6 checks passed
@JMicheli JMicheli deleted the jm/fix-getbook branch October 16, 2024 01:33
@aaronleopold aaronleopold mentioned this pull request Dec 4, 2024
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