Fix navigation warnings on autotest manager and starter file manager pages, closes 7641

[philipkukulak]

Proposed Changes

On the Settings > Automated Tests page, a warning should pop up if a user attempts to navigate away from the page after making changes and before saving them.

Currently, it appears even after the user has saved their changes. This happens because the Settings > Automated Tests page is rendered with react, which is rendered after the jquery click handler in _navigation_warning.js.erb is registered: submit_clicked can never be flipped to true because the submit button doesn't exist when that handler is registered.

Furthermore, the window.onbeforeunload handler as defined in the _navigation_warning.js.erb partial has no awareness of the Automated Tests page react structure. The page and the partial within it are tracking 'dirtiness' in separate state variables.

With help from claude, the cleanest fix is to override the window.onbeforeunload handler in autotest_manager.jsx and to gate the initialization of the page so that fetchData doesn't instantly 'dirty' the page when react fires onChange upon first receipt of existing form data.

The fix for the Settings -> Starter Files page is far simpler, since it is not rendered using react: we bind the handler to .on('click', ...) instead.

Screenshots of your changes (if applicable)

Screen.Recording.2026-05-29.at.10.56.39.AM.mov

Screen.Recording.2026-05-29.at.3.49.18.PM.mov

Associated documentation repository pull request (if applicable)

n/a

Type of Change

(Write an X or a brief description next to the type or types that best describe your changes.)

Type	Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change)
✨ New feature (non-breaking change that adds functionality)
🐛 Bug fix (non-breaking change that fixes an issue)	x
🎨 User interface change (change to user interface; provide screenshots)
♻️ Refactoring (internal change to codebase, without changing functionality)
🚦 Test update (change that only adds or modifies tests)
📦 Dependency update (change that updates a dependency)
🔧 Internal (change that only affects developers or continuous integration)

Checklist

(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the [ ] into a [x] in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)

Before opening your pull request:

• I have performed a self-review of my changes.
  • Check that all changed files included in this pull request are intentional changes.
  • Check that all changes are relevant to the purpose of this pull request, as described above.
• I have added tests for my changes, if applicable.
  • This is required for all bug fixes and new features.
• I have updated the project documentation, if applicable.
  • This is required for new features.
• If this is my first contribution, I have added myself to the list of contributors.

After opening your pull request:

• I have updated the project Changelog (this is required for all changes).
• I have verified that the pre-commit.ci checks have passed.
• I have verified that the CI tests have passed.
• I have reviewed the test coverage changes reported by Coveralls.
• I have requested a review from a project maintainer.

Questions and Comments

I had the thought that we could also take this a step further and track changes to the page that are 'undone', e.g. checking a box and then unchecking it, and make sure the navigation warning doesn't pop up in that case. I came to the judgment that this would bloat the code more than we could justify an improvement so slight.

I also did not touch the Settings -> Starter Files page (on which this bug is also present) but if it is decided upon review that we'd like to bundle that fix into this PR, I would be happy to do so. Applied this fix to the Settings -> Starter Files page.

[coveralls]

Coverage Report for CI Build 26823406732

Warning

No base build found for commit 00ab1bb on master.
Coverage changes can't be calculated without a base build.
If a base build is processing, this comment will update automatically when it completes.

Coverage: 90.193%

Details

• Patch coverage: 11 of 11 lines across 2 files are fully covered (100%).

Uncovered Changes

No uncovered changes found.

Coverage Regressions

Requires a base build to compare against. How to fix this →

---

Coverage Stats

Relevant Lines:	50084
Covered Lines:	46153
Line Coverage:	92.15%
Relevant Branches:	2226
Covered Branches:	1027
Branch Coverage:	46.14%
Branches in Coverage %:	Yes
Coverage Strength:	123.25 hits per line

---

💛 - Coveralls

[david-yz-liu]

Nice work, @philipkukulak. I left one inline comment, and also it seems like you should now be able to remove the loading of the _navigation_warning.js.erb partial from this page.

Please go ahead and also make the same change to the starter files page.

[david-yz-liu]

Overall I don't think this _initializing attribute is necessary. You can check the current value of this.state.formData before doing any updates, treating Object.keys(this.state.formData).length === 0 as the "initializing" condition.

[philipkukulak]

Wouldn't Object.keys(this.state.formData).length > 0 if any form data already exists, e.g. some tester is already configured? That would mark the page 'dirty' by default whenever there's persistent data. I'm thinking of 'initialization' as the initial fetch for page contents rather than the contents of the page itself.

[david-yz-liu]

I'm also considering "initialization" as the initial fetch. The initial state of this component sets formData: {}. After the data is fetched this will change. (For a new assignment I suppose it's possible the fetched value may be {} as well, I'm not sure. If so, you can change the initial formData state to null instead to disambiguate it.)

[philipkukulak]

Ahhh okay I think I understand. I'll re-request review once I pushed these changes. Thanks David.

[philipkukulak]

Removed the _navigation_warning partial from Settings -> Automated Tests since everything is handled by a react component now. Applied the fix to the Settings -> Starter Files page (much easier to fix here since there is no react on that page.) Also changed the initialization gate on the Settings -> Automated Tests per David's recommendations. Tested it locally. It works! Pretty cool.

Note that overall test coverage has gone down because we've created a new test suite (autotest_manager.test.jsx) that only tests our bug fix.

[david-yz-liu]

Delete this blank line

[david-yz-liu]

This change is quite different from the one for the autotest manager and not what I expected (this file is used by other views as well, so should not be changed).

The starter file page does use React, with the root component being StarterFileManager. Please make the same type of change you made for the AutotestManager in that component.

[philipkukulak]

Sorry, I was unclear; the starter file page doesn't use rjsf to render form contents like the autotester settings page does, which is the library that fires an onChange event right out the gate when fetchData is called.

About to push a commit in which the change to the starter files page mirrors that in the autotester settings page.

[david-yz-liu]

@philipkukulak okay looks good, but you should also now be able to remove the shared/navigation_warning include on the starter files page.