Skip to content

Conversation

@suhaibabsi-inst
Copy link
Contributor

@suhaibabsi-inst suhaibabsi-inst commented Dec 28, 2025

refs: MBL-19565
affects: Student,Teacher, Parent
builds: Student, Teacher, Parent
release note: Enhanced VoiceOver support in SpeedGrader

Screen Recording

speedgrader_grade_submit_demo.MP4

Test Plan

1- In Teacher app, go to an assignment with few submissions.
2- Open one of submission in SpeedGrader, and try to edit a grade.
3- Now upon successful edit request, you should see a snacker showing with "Grade Submitted" message.
4- Turn on VoiceOver, and attempt to submit another grade.
5- Upon request, Snackbar should be shown, and the message ought to be read out by VoiceOver.

Checklist

  • Follow-up e2e test ticket created
  • A11y checked
  • Tested on phone
  • Tested on tablet
  • Tested in dark mode
  • Tested in light mode
  • Approve from product

refs: MBL-19565
affects: Student,Teacher, Parent
builds: Student, Teacher, Parent
release note: Enhanced VoiceOver support in SpeedGrader
@suhaibabsi-inst suhaibabsi-inst self-assigned this Dec 28, 2025
@claude
Copy link

claude bot commented Dec 28, 2025

Claude Code Review

Updated: 2025-12-28

Issues Found:

  • Build Failure: SwiftLint violation - Trailing whitespace on line 195 of SpeedGraderSubmissionGradesViewModel.swift (must be fixed to pass CI)

Review:

  • ✅ Feature correctly implements snackbar success feedback after grade submission
  • ✅ Proper weak self capture with guard pattern in closure
  • ✅ Sound switch statement logic for completion vs. error handling
  • ✅ Localization string properly added
  • ✅ Non-breaking change that cleanly separates success/error handling
  • ❌ SwiftLint violation prevents build from passing

⚠️ Issues found

@inst-danger
Copy link
Contributor

inst-danger commented Dec 28, 2025

Warnings
⚠️ One or more files are below the minimum test coverage 50%

Release Note:

Enhanced VoiceOver support in SpeedGrader

Affected Apps: Student, Teacher, Parent

Builds: Student, Teacher, Parent

MBL-19565

Coverage New % Master % Delta
Canvas iOS 91.29% 81.06% 10.22%
Core/Core/Common/CommonUI/NavigationBar/UIKit/ColoredNavViewProtocol.swift 28.57% 100% -71.43%
Core/Core/Common/Extensions/PDFKit/PDFDocumentExtensions.swift 0% -- --

Generated by 🚫 dangerJS against e2cfe7e

@inst-danger
Copy link
Contributor

inst-danger commented Dec 28, 2025

Builds

Commit: Comments (e2cfe7e)
Build Number: 1173
Built At: Jan 20 21:55 CET (01/20 01:55 PM MST)

Student
Teacher
Parent

@suhaibabsi-inst suhaibabsi-inst marked this pull request as ready for review December 28, 2025 14:01
petkybenedek
petkybenedek previously approved these changes Jan 5, 2026
Copy link
Contributor

@petkybenedek petkybenedek left a comment

Choose a reason for hiding this comment

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

QA +1
This only affects teacher, the release notes shoud only appear there.

Also, on my phone, which is set to Hungarian language, VoiceOver did not announce the snackbar, but in English it did.

Copy link
Contributor

@rh12 rh12 left a comment

Choose a reason for hiding this comment

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

The task is to display the snackbar when the user navigates away from the given submission (like swiping to next student or closing SpeedGrader), not each time a grade is submitted.
Please see the comments in the ticket and the linked Figma design.

@claude
Copy link

claude bot commented Jan 6, 2026

Claude Code Review

Updated: 2026-01-06

Approved - No critical issues found.

Code quality observations:

  • Feature correctly shows snack notification for grade submissions (dismiss and page transitions)
  • Proper abstraction with CurrentGradeChangeEvaluator protocol for testability
  • State tracking of initial grade to detect changes is sound
  • Localization properly added and tests cover both happy paths
  • Non-breaking change, properly scoped to SpeedGrader feature

@claude
Copy link

claude bot commented Jan 6, 2026

Claude Code Review

Updated: 2026-01-06

Issues Found

  • Bug in isGradeChanged logic (line 154): When initialGrade is nil, the condition checks lastSubmittedGrade == gradeState.originalGrade, but this returns true when grades are equal, which contradicts the property name. Should be lastSubmittedGrade != gradeState.originalGrade to correctly detect a change.

  • Snackbar race condition: The static method didDismiss(withGradeChanges:) accesses SnackBarViewModel.topControllerModel during dismissal, but there's no guarantee the correct view controller is still the top controller at that point, especially during navigation transitions.

  • Memory leak in DefaultGradeChangeEvaluator: The weak reference to SpeedGraderScreenViewModel and the strong reference back from SpeedGraderScreenViewModel to currentGradeChangeEvaluator creates a potential retain cycle if the evaluator isn't properly released.

Minor Concerns

  • Locale-specific formatting applied globally (en_US) may cause issues for international builds
  • No error handling if findSnackBarViewModel() returns nil

❌ Issues found

@claude
Copy link

claude bot commented Jan 20, 2026

Claude Code Review

🔴 Critical Logic Issue Found:

isGradeChanged property (SpeedGraderSubmissionGradesViewModel.swift:152-158):
The logic is inverted. Line 154-155 checks if lastSubmittedGrade equals originalGrade, but should check if they are NOT equal. The code returns true when grades HAVEN'T changed, opposite of intended behavior. This causes snackbar to show for unchanged grades and prevents showing it for actual changes.

Snackbar on page transition (SpeedGraderScreenViewModel.swift:278-284):
When swiping to another submission, snackbar shows based on PREVIOUS submission's grade change status. This conflates two different events and may confuse users about which submission's grade was affected.

Thread safety gap:
SnackBarViewModel.topControllerModel (line 156, 99) accesses AppEnvironment.shared.topViewController without synchronization. If called during dismissal off-main-thread, could crash.

@suhaibabsi-inst suhaibabsi-inst marked this pull request as draft January 20, 2026 15:55
@claude
Copy link

claude bot commented Jan 20, 2026

Claude Code Review

Updated: 2026-01-20

CRITICAL: Logic bug in isGradeChanged property (SpeedGraderSubmissionGradesViewModel.swift:152-158)

  • When initialGrade is nil, returns true when lastSubmittedGrade equals gradeState.originalGrade
  • This indicates NO change was made, but the condition treats it as a change
  • Should check if grade DIFFERS from original, not if it matches
  • Results in snackbar showing incorrectly for submissions without initial grade

Potential issue with grade state tracking

  • lastSubmittedGrade is set optimistically when saveGrade() is called but may not match persisted value
  • If server saves a different grade, isGradeChanged logic will be incorrect

ℹ️ Tests added for snackbar scenarios

@suhaibabsi-inst suhaibabsi-inst marked this pull request as ready for review January 20, 2026 20:22
@claude
Copy link

claude bot commented Jan 20, 2026

Claude Code Review

Updated: 2026-01-20

CRITICAL BUG: Inverted logic in isGradeChanged (SpeedGraderSubmissionGradesViewModel.swift:157-158)

  • Returns true when lastSubmittedGrade == gradeState.originalGrade (they MATCH)
  • Should return true when they DIFFER
  • Causes snackbar to show when NO grade change occurred on first submission

Secondary issue: Grade equality check flaw

  • lastSubmittedGrade is set optimistically before server confirmation
  • If server saves differently, snackbar logic will be incorrect

❌ Issues found

@rh12 rh12 marked this pull request as draft January 23, 2026 11:56
@rh12
Copy link
Contributor

rh12 commented Jan 23, 2026

@suhaibabsi-inst Converting this into draft for now, while this change is being figured out on product level.

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.

5 participants