Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix:mirador_crash #3927

Merged
merged 9 commits into from
Feb 4, 2025
Merged

fix:mirador_crash #3927

merged 9 commits into from
Feb 4, 2025

Conversation

fstoe
Copy link

@fstoe fstoe commented Jul 30, 2024

I found out that adding something through the add button in the catalog view which is json but not a manifest makes the viewer crash.
steps to reproduce:

@codecov-commenter
Copy link

codecov-commenter commented Jul 30, 2024

Codecov Report

Attention: Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.

Project coverage is 94.59%. Comparing base (a8daa02) to head (963a934).
Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
src/state/selectors/manifests.js 66.66% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3927   +/-   ##
=======================================
  Coverage   94.59%   94.59%           
=======================================
  Files         315      315           
  Lines       14773    14776    +3     
  Branches     2491     2494    +3     
=======================================
+ Hits        13974    13977    +3     
  Misses        795      795           
  Partials        4        4           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lutzhelm
Copy link
Contributor

lutzhelm commented Aug 2, 2024

Hi @fstoe, could you add a test case for that patch? createManifestoInstance itself is not exported, but getManifestoInstance calls it directly, so this would be a good place to add a test:

https://github.com/ProjectMirador/mirador/blob/master/__tests__/src/selectors/manifests.test.js#L56-L68

Something like

it('does not crash if json is not a manifest', () => {
  const state = { manifests: { x: { json: {} } } };
  const received = getManifestoInstance(state, { manifestId: 'x' });
  expect(received).toBe(undefined);
});

@fstoe
Copy link
Author

fstoe commented Aug 5, 2024

Hey @lutzhelm,
thanks for the comment. I just added a small test case

@gerdesque gerdesque requested a review from lutzhelm December 19, 2024 15:04
@fstoe fstoe requested a review from lutzhelm January 16, 2025 11:11
lutzhelm
lutzhelm previously approved these changes Jan 28, 2025
@lutzhelm lutzhelm self-requested a review January 28, 2025 14:49
@lutzhelm lutzhelm dismissed their stale review January 28, 2025 14:49

Noticed another potential issue in the catalog

@fstoe fstoe requested a review from lutzhelm February 3, 2025 16:14
@lutzhelm lutzhelm merged commit fba3b5d into ProjectMirador:main Feb 4, 2025
8 of 9 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants