Skip to content

Conversation

@RichDom2185
Copy link
Member

Description

Wow, this bug is 4 years old.

Indices should not be used as memoization keys. When switching between assessment types, the old image is still shown while the new one loads. Thus if the image load fails, nothing happens.

This fixes it, ensuring the entire assessment card is tied to the assessment ID, thus switching immediately updates the UI to loading state.

Also moves components to use FC instead of function calls as part of the fix (so that key prop works without drilling).

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Code quality improvements

How to test

Checklist

  • I have tested this code
  • I have updated the documentation

@RichDom2185 RichDom2185 requested a review from sayomaki August 14, 2025 10:06
@RichDom2185 RichDom2185 self-assigned this Aug 14, 2025
@RichDom2185 RichDom2185 changed the title Assessment overview memoization Fix incorrect assessment overview memoization Aug 14, 2025
@RichDom2185 RichDom2185 enabled auto-merge (squash) August 14, 2025 10:06
@sayomaki sayomaki disabled auto-merge August 14, 2025 17:49
@coveralls
Copy link

Pull Request Test Coverage Report for Build 16962268299

Details

  • 158 of 191 (82.72%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 43.462%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/commons/assessment/AssessmentInteractButton.tsx 52 62 83.87%
src/commons/assessment/AssessmentOverviewCard.tsx 78 101 77.23%
Totals Coverage Status
Change from base Build 16942913584: 0.01%
Covered Lines: 20655
Relevant Lines: 49720

💛 - Coveralls

Copy link
Contributor

@sayomaki sayomaki left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! It seems that coveralls is working again.

(also, minor note but it is actually the list keys that is not being updated correctly causing react to think that the element was not updated)

@sayomaki sayomaki merged commit e04f7f0 into master Aug 14, 2025
19 of 21 checks passed
@sayomaki sayomaki deleted the assessment-overview-memoization branch August 14, 2025 17:59
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.

4 participants