Skip to content

Conversation

@longsleep
Copy link
Collaborator

Summary

This PR fixes a regression introduced during the conversion of the Login component from class-based to functional component in October 2021.

🐛 Regression Details

Introduced in: d12fad6bd4ab50bdacfae76e7c6631d845c2fcdc (Oct 12, 2021)
Commit: "Add support for visual branding of identifier"
File: identifier/src/containers/Login/Login.jsx:137

What Happened

During the class-to-functional component conversion:

  • ✅ The logon method was correctly renamed to handleNextClick
  • ✅ The submit button's onClick was correctly updated to use handleNextClick
  • ❌ The form's onSubmit was left with the old this.logon(event) reference

Why It Wasn't Noticed

The application continued to work normally because:

  1. Enter key presses in form fields trigger the submit button's onClick handler (standard HTML behavior)
  2. The onClick={handleNextClick} worked correctly
  3. The broken onSubmit handler was never actually called due to browser behavior
  4. No console errors were visible to end users

🔧 Fix Applied

Before:

<form action="" onSubmit={(event) => this.logon(event)}>
  // ...
  <Button onClick={handleNextClick}>Next</Button>
</form>

After:

<form action="" onSubmit={handleNextClick}>
  // ...  
  <Button onClick={handleNextClick}>Next</Button>
</form>

Verification

  • Form submission now uses the same handler as button click
  • Enter key press behavior remains unchanged (works correctly)
  • Button click behavior remains unchanged (works correctly)
  • No functional regressions introduced
  • Code maintainability improved (consistent handler usage)

📋 Testing

Before Fix

  • ❌ Form's onSubmit referenced non-existent this.logon method
  • ✅ Application worked due to HTML form behavior masking the issue
  • ⚠️ Potential confusion for developers debugging form behavior

After Fix

  • ✅ Form's onSubmit correctly references handleNextClick
  • ✅ Consistent behavior between form submission and button click
  • ✅ Clear code path for debugging and maintenance

🎯 Impact

  • Risk Level: LOW (fixes broken code, no functional changes)
  • User Impact: None (application already worked correctly)
  • Developer Impact: Improved code clarity and maintainability
  • Regression Age: ~3 years (since Oct 2021)

This is a code quality fix that resolves a long-standing regression without changing any user-visible behavior.

…nal conversion

This fixes a regression introduced in commit d12fad6
'Add support for visual branding of identifier' where the Login component was
converted from class-based to functional component.

During the conversion:
- The 'logon' method was correctly renamed to 'handleNextClick'
- The button's onClick was correctly updated to use 'handleNextClick'
- However, the form's onSubmit was left with the old 'this.logon(event)' reference

This resulted in a broken form submission handler that referenced a non-existent
method. The application continued to work because Enter key presses trigger the
submit button's onClick handler due to standard HTML form behavior, masking
the regression.

Changes:
- Replace broken 'onSubmit={(event) => this.logon(event)}' with 'onSubmit={handleNextClick}'
- Ensures consistent behavior between form submission and button click
- Maintains existing preventDefault behavior and proper form handling

Regression introduced: d12fad6 (Oct 12, 2021)
Component: identifier/src/containers/Login/Login.jsx:137
@longsleep longsleep self-assigned this Jun 30, 2025
@longsleep longsleep merged commit c1edac3 into libregraph:master Jun 30, 2025
1 check passed
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.

1 participant