feat: set image thumbnail to show as preview in analysis tab#2671
feat: set image thumbnail to show as preview in analysis tab#2671adambasha0 wants to merge 7 commits intomainfrom
Conversation
LCOV of commit
|
Chemotion.dev.37.webm |
There was a problem hiding this comment.
Pull Request Overview
This PR enables users to select and display a preferred thumbnail for analysis-related attachments across samples, reactions, devices, research plans, and cell lines. It adds thumbnail selection functionality with visual feedback including loading spinners and highlighted preferred thumbnails.
- Refactored ImageModal component to support preferred thumbnail selection and display loading states
- Updated all analysis-related parent components to pass preferred thumbnail props to ImageModal
- Added backend support to expose preferred_thumbnail in container API metadata
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| app/javascript/src/utilities/imageHelper.js | Added debug logging for preferred attachment ID tracking |
| app/javascript/src/fetchers/AttachmentFetcher.js | Added debug logging for attachment fetcher parameters |
| app/javascript/src/components/common/ImageModal.js | Major refactor to support preferred thumbnail selection, loading states, and thumbnail pagination |
| app/javascript/src/apps/mydb/elements/details/samples/analysesTab/SampleDetailsContainersDnd.js | Added updateContainerPreferredThumbnail prop to ContainerRow |
| app/javascript/src/apps/mydb/elements/details/samples/analysesTab/SampleDetailsContainersCom.js | Added updateContainerPreferredThumbnail prop to ReactionsDisplay |
| app/javascript/src/apps/mydb/elements/details/samples/analysesTab/SampleDetailsContainersAux.js | Updated AnalysesHeader to handle preferred thumbnail changes |
| app/javascript/src/apps/mydb/elements/details/samples/analysesTab/SampleDetailsContainers.js | Added updateContainerPreferredThumbnail method and prop passing |
| app/javascript/src/apps/mydb/elements/details/researchPlans/analysesTab/ResearchPlanDetailsContainers.js | Added preferred thumbnail handling to research plan containers |
| app/javascript/src/apps/mydb/elements/details/deviceDescriptions/analysesTab/AnalysisHeader.js | Added preferred thumbnail handling to device description analysis |
| app/javascript/src/apps/mydb/elements/details/cellLines/analysesTab/Header.js | Added preferred thumbnail handling to cell line analysis |
| app/api/entities/container_entity.rb | Exposed preferred_thumbnail field in container API metadata |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| .filter((key) => key !== 'id') | ||
| .map((key) => [key, params[key]]) | ||
| ); | ||
| console.log(params, urlParams); |
There was a problem hiding this comment.
Remove debug console.log statement before merging to production. Debug logging should not be left in production code.
| console.log(params, urlParams); |
|
|
||
| fetchThumbnails() { | ||
| const { ChildrenAttachmentsIds } = this.props; | ||
| console.log('ChildrenAttachmentsIds', ChildrenAttachmentsIds); |
There was a problem hiding this comment.
Remove debug console.log statement before merging to production. Debug logging should not be left in production code.
| console.log('ChildrenAttachmentsIds', ChildrenAttachmentsIds); |
| onKeyDown={(e) => { | ||
| if (e.key === 'Enter' || e.key === ' ') { | ||
| e.preventDefault(); | ||
| this.handleSetPreferred(thumb); | ||
| } | ||
| }} |
There was a problem hiding this comment.
[nitpick] Extract this keyboard event handler to a separate method to improve readability and reduce inline complexity.
| const onChangePreferredThumbnail = (currentPreferredThumbnail) => { | ||
| // Handle the change of preferred thumbnail | ||
| if (currentPreferredThumbnail !== preferredThumbnail) { | ||
| // Handle the change of preferred thumbnail |
There was a problem hiding this comment.
Remove duplicate comment. Both lines say 'Handle the change of preferred thumbnail'.
| // Handle the change of preferred thumbnail |
| const onChangePreferredThumbnail = (currentPreferredThumbnail) => { | ||
| // Handle the change of preferred thumbnail | ||
| if (currentPreferredThumbnail !== preferredThumbnail) { | ||
| // Handle the change of preferred thumbnail |
There was a problem hiding this comment.
Remove duplicate comment. Both lines say 'Handle the change of preferred thumbnail'.
| // Handle the change of preferred thumbnail |
| const onChangePreferredThumbnail = (currentPreferredThumbnail) => { | ||
| // Handle the change of preferred thumbnail | ||
| if (currentPreferredThumbnail !== preferredThumbnail) { | ||
| // Handle the change of preferred thumbnail |
There was a problem hiding this comment.
Remove duplicate comment. Both lines say 'Handle the change of preferred thumbnail'.
| // Handle the change of preferred thumbnail | |
| const onChangePreferredThumbnail = (currentPreferredThumbnail) => { | ||
| // Handle the change of preferred thumbnail | ||
| if (currentPreferredThumbnail !== preferredThumbnail) { | ||
| // Handle the change of preferred thumbnail |
There was a problem hiding this comment.
Remove duplicate comment. Both lines say 'Handle the change of preferred thumbnail'.
| // Handle the change of preferred thumbnail | |
|
Hi @adambasha0! Great feature implementation overall. Here are the key issues and suggestions I found during review: 🐛 Functional Issues
🔧 Code Quality Issues
|
alizaib1217
left a comment
There was a problem hiding this comment.
Requested changes in above comment.
|
Hi @alizaib1217, Thanks for the review. I fixed Code Quality issues.
|
68046fd to
cf7d5e4
Compare
LCOV of commit
|
Hi, Thanks for the quick response. @adambasha0 Can you share a small video of thumbnail arrow working for more than 2 images? I have three images in dataset, and it seems disabled in the modal view. I am clicking the button, but nothing happens. |
The current thumbnailsPerPage is 6. this means you have to upload at least 7 images for arrows to start working. Less than this you would only have 1 page and the arrows will be disabled |
alizaib1217
left a comment
There was a problem hiding this comment.
Hi, looks good. Thanks for the update.
|
Hi all, do you have a video for how the functions looks like after the extension/correction? Or a description of how it works? Actually I find it quite challenging to follow the discussion and get an idea of how the function looks like atm. |
cf7d5e4 to
15cf3bc
Compare
| metadata[:kind] = object.extended_metadata['kind'] | ||
| metadata[:index] = object.extended_metadata['index'] | ||
| metadata[:instrument] = object.extended_metadata['instrument'] | ||
| metadata[:preferred_thumbnail] = object.extended_metadata['preferred_thumbnail'] |
There was a problem hiding this comment.
Metrics/CyclomaticComplexity: Cyclomatic complexity for extended_metadata is too high. [9/7]
| metadata[:kind] = object.extended_metadata['kind'] | ||
| metadata[:index] = object.extended_metadata['index'] | ||
| metadata[:instrument] = object.extended_metadata['instrument'] | ||
| metadata[:preferred_thumbnail] = object.extended_metadata['preferred_thumbnail'] |
There was a problem hiding this comment.
Metrics/PerceivedComplexity: Perceived complexity for extended_metadata is too high. [10/8]
| metadata[:kind] = object.extended_metadata['kind'] | ||
| metadata[:index] = object.extended_metadata['index'] | ||
| metadata[:instrument] = object.extended_metadata['instrument'] | ||
| metadata[:preferred_thumbnail] = object.extended_metadata['preferred_thumbnail'] |
There was a problem hiding this comment.
Metrics/BlockLength: Block has too many lines. [26/25]
LCOV of commit
|
Hi @nicolejung, you can check the functionality of the feature using this instance: |
- Add preferred thumbnail functionality to ImageModal component - Enable thumbnail selection interface with pagination and visual feedback - Pass preferred thumbnail and children attachment IDs to ImageModal - Add thumbnail fetching capabilities via AttachmentFetcher - Implement thumbnail grid display with current selection highlighting - Add navigation controls for multi-page thumbnail viewing - Update analysis headers to support preferred thumbnail display - Include console logging for debugging preferred thumbnail functionality
- Add preferred thumbnail functionality to ImageModal component - Enable thumbnail selection interface with pagination and visual feedback - Pass preferred thumbnail and children attachment IDs to ImageModal - Add thumbnail fetching capabilities via AttachmentFetcher - Implement thumbnail grid display with current selection highlighting - Add navigation controls for multi-page thumbnail viewing - Update analysis headers to support preferred thumbnail display - Include console logging for debugging preferred thumbnail functionality
Major refactor of ImageModal: - Added support for preferredThumbnail and onChangePreferredThumbnail props. - Added loading spinner UI while fetching images. - Improved preferred thumbnail highlighting and selection logic. - Fixed integer comparison for thumbnail IDs. - Cleaned up and optimized state handling. Updated usage of ImageModal in: - Sample analysis, reaction analysis, device description, cell line, and researchplan analysis modals/components - Changes done to pass new props and handle preferred thumbnail changes. Ensured all parent components propagate preferredThumbnail, ChildrenAttachmentsIds, and onChangePreferredThumbnail as needed. - Container API: Exposed preferred_thumbnail in container entity metadata.
…umbnail fetch error handling- remove debug logs
15cf3bc to
e38b6b4
Compare
LCOV of commit
|
LCOV of commit
|
LCOV of commit
|


PR: Preferred Thumbnail Selection for ImageModal:
1. Problem Description
2. Solution
3. Implementation
Refactored ImageModal to:
Backend: Updated container entity to expose preferred_thumbnail in API responses.
To be discussed:
rather 1-story 1-commit than sub-atomic commits
commit title is meaningful => git history search
commit description is helpful => helps the reviewer to understand the changes
code is up-to-date with the latest developments of the target branch (rebased to it or whatever) => ⏩-merge for linear history is favoured
added code is linted
tests are passing (at least locally): we still have some random test failure on CI. thinking of asking spec/examples.txt to be commited
in case the changes are visible to the end-user, video or screenshots should be added to the PR => helps with user testing
testing coverage improvement is improved.
CHANGELOG : add a bullet point on top (optional: reference to github issue/PR )
parallele PR for documentation on docusaurus if the feature/fix is tagged for a release