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

Make sure thumbnails can never block the rendering of the whole grid #99

Open
wetneb opened this issue Mar 7, 2024 · 7 comments
Open

Comments

@wetneb
Copy link
Member

wetneb commented Mar 7, 2024

In some circumstances the code that renders thumbnails into cells can fail and that will abort the rendering of the entire grid.

See this forum post for instance: https://forum.openrefine.org/t/openrefine-3-7-9-released/1259/9?u=antonin_d

We should make sure that any exception thrown in our cell renderer is caught appropriately and a fallback is used, so that the rest of the grid is still rendered.
This safety mechanism should also be implemented on OpenRefine's side of the extension point.

@wetneb
Copy link
Member Author

wetneb commented Mar 7, 2024

Duplicate of #96

@trnstlntk
Copy link
Contributor

Seems to happen on Wikimedia PAWS too. For some users. Not for me, it works fine when I try

See https://phabricator.wikimedia.org/project/view/1648/

wetneb added a commit to wetneb/OpenRefine that referenced this issue Apr 8, 2024
This will let the next cell renderers attempt to render the cell
instead of aborting the rendering of the entire grid.

For OpenRefine/CommonsExtension#99.
@wetneb
Copy link
Member Author

wetneb commented Apr 8, 2024

This issue would still be worth solving in the Commons extension but to mitigate the problem, I am proposing a change in OpenRefine to avoid the problem from blocking the rendering of the whole grid (which I should have thought about when I introduced the extension point…)
OpenRefine/OpenRefine#6514

Hopefully we can include this in 3.8, meaning that people would be able to use the existing Commons extension with 3.8 and it wouldn't crash like that (instead, certain cells wouldn't be rendered as thumbnails but OpenRefine would still remain usable).

wetneb added a commit to OpenRefine/OpenRefine that referenced this issue Apr 9, 2024
This will let the next cell renderers attempt to render the cell
instead of aborting the rendering of the entire grid.

For OpenRefine/CommonsExtension#99.
wetneb added a commit to OpenRefine/OpenRefine that referenced this issue Apr 10, 2024
This will let the next cell renderers attempt to render the cell
instead of aborting the rendering of the entire grid.

For OpenRefine/CommonsExtension#99.
@sebastian-berlin-wmse
Copy link
Contributor

Duplicate of #96

Shouldn't either issue be closed in that case? Not sure which one since this one is referenced in other places, but the other has more discussion.

@wetneb
Copy link
Member Author

wetneb commented Sep 9, 2024

Let's close #96 which was about the bug itself, which is mitigated by the change on OpenRefine's side
We can keep #99 (this issue) for adding the additional safeguards in the commons extension itself.

@sunilnatraj
Copy link

@wetneb Carried out below test and also reviewed the thumbnail rendering code and it is handling failures. Let me know if there is a different test case for this issue.

  • Create a new project
  • Add couple of records with File name with no thumbnail
  • Reconcile the column
  • All records get rendered

@wetneb
Copy link
Member Author

wetneb commented Nov 18, 2024

Apparently, one of the errors that can happen is this:

JQMIGRATE: Migrate is installed with logging active, version 3.4.0
project-bundle.js:59615 Uncaught TypeError: Cannot read properties of undefined (reading 'identifierSpace')
    at ThumbnailReconRenderer.render (project-bundle.js:59615:71)
    at DataTableCellUI._render (project-bundle.js:46509:36)
    at new DataTableCellUI (project-bundle.js:46466:8)
    at renderRow (project-bundle.js:45731:9)
    at DataTableView._renderDataTables (project-bundle.js:45743:5)
    at DataTableView.render (project-bundle.js:45405:8)
    at project-bundle.js:45750:10
    at Object.success (project-bundle.js:38198:9)
    at fire (project-bundle.js:3502:31)
    at Object.fireWith [as resolveWith] (project-bundle.js:3632:7)

(see this comment). This is one of the failures the current code doesn't handle itself.

So it would be worth checking if the reconciliation service is well defined before accessing its .identifierSpace. More generally, the entire render() method could do with a try {} catch {} block to catch any exceptions thrown in it and maybe report them in the console as errors.

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

No branches or pull requests

4 participants