Skip to content

bugfix(replay): Handle replay deletion during version mismatch prompt#1837

Open
bobtista wants to merge 8 commits into
TheSuperHackers:mainfrom
bobtista:bobtista/fix-replay-deletion-crash
Open

bugfix(replay): Handle replay deletion during version mismatch prompt#1837
bobtista wants to merge 8 commits into
TheSuperHackers:mainfrom
bobtista:bobtista/fix-replay-deletion-crash

Conversation

@bobtista

@bobtista bobtista commented Nov 12, 2025

Copy link
Copy Markdown

Fixes #1732

Summary

  • Revalidates the replay after the version-mismatch prompt.
  • Keeps the Replay Menu open, shows “REPLAY NOT FOUND,” and refreshes the list if validation or final playback fails.
  • Enters recorder playback mode only after initialization succeeds, preventing a failed open from leaving a null replay file.
  • Applies the fix to both Generals and Zero Hour.

Notes

  • Both reallyLoadReplay() and loadReplay() now handle playbackFile() failure by keeping the menu open, showing the replay-not-found message, and refreshing the list.
  • Recorder playback mode is only entered after initialization succeeds.
  • The changes are mirrored across Generals and Zero Hour.
  • Side benefit now ReplayMenu.cpp can be unified to core

Testing

  1. Select a replay with a different executable hash/version.
  2. Double-click it or press Load.
  3. Delete the replay externally while the version-mismatch prompt is open.
  4. Confirm the prompt.
  5. Verify that the game does not crash, displays “REPLAY CANNOT BE LOADED,” and refreshes the replay list.

@bobtista bobtista self-assigned this Nov 12, 2025
@bobtista
bobtista force-pushed the bobtista/fix-replay-deletion-crash branch from 66502a8 to 9d06998 Compare December 3, 2025 21:03
@bobtista
bobtista marked this pull request as ready for review December 3, 2025 21:03

@Stubbjax Stubbjax left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There is already an established pathway for handling file deletion during a confirmation popup in ReplayMenu::delateReplay. Adopting this logic would simplify the implementation and resolve the message and list refresh inconsistencies of this change, which I've tabulated below for clarity.

Action Refreshes list Message
DELETE REPLAY Yes The system cannot find the specified file.
LOAD REPLAY No This replay cannot be loaded because the file no longer exists on this device.

@bobtista
bobtista force-pushed the bobtista/fix-replay-deletion-crash branch from 9d06998 to c5b68b2 Compare February 5, 2026 03:17
@bobtista

bobtista commented Feb 5, 2026

Copy link
Copy Markdown
Author

There is already an established pathway for handling file deletion during a confirmation popup in ReplayMenu::delateReplay. Adopting this logic would simplify the implementation and resolve the message and list refresh inconsistencies of this change, which I've tabulated below for clarity.

Action Refreshes list Message
DELETE REPLAY Yes The system cannot find the specified file.
LOAD REPLAY No This replay cannot be loaded because the file no longer exists on this device.

Thank you - I implemented it this way, rebased the latest main, and force pushed

@greptile-apps

greptile-apps Bot commented Feb 5, 2026

Copy link
Copy Markdown

Greptile Summary

The PR hardens replay loading in both game targets.

  • Revalidates replay metadata and map availability after the version-mismatch prompt.
  • Keeps the replay menu open, displays a generic load error, and refreshes the replay list when playback fails.
  • Delays entering recorder playback mode until initialization succeeds.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
Generals/Code/GameEngine/Source/Common/Recorder.cpp Defers playback mode until replay initialization completes, preventing failed opens from exposing invalid playback state.
Generals/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/ReplayMenu.cpp Revalidates prompted replays, restores the missing-map guard, and handles final playback failures with generic feedback and list refresh.
GeneralsMD/Code/GameEngine/Source/Common/Recorder.cpp Mirrors the safe recorder mode transition for Zero Hour.
GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/ReplayMenu.cpp Mirrors the corrected replay validation and failure handling for Zero Hour.

Reviews (9): Last reviewed commit: "bugfix(replay): Recheck replay map after..." | Re-trigger Greptile

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

4 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

Comment thread Generals/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/ReplayMenu.cpp Outdated
@bobtista
bobtista force-pushed the bobtista/fix-replay-deletion-crash branch from c5b68b2 to cbc36ff Compare February 5, 2026 15:52
@bobtista

bobtista commented Feb 5, 2026

Copy link
Copy Markdown
Author

@greptileai

@bobtista

bobtista commented Feb 5, 2026

Copy link
Copy Markdown
Author

The latest push does this:

  • Use readReplayMapInfo() to validate the file before playback (same pattern as loadReplay)
  • Show the "REPLAY NOT FOUND" message that loadReplay uses
  • Refresh the listbox on failure (matching deleteReplay behavior)

@bobtista

Copy link
Copy Markdown
Author

There is already an established pathway for handling file deletion during a confirmation popup in ReplayMenu::delateReplay. Adopting this logic would simplify the implementation and resolve the message and list refresh inconsistencies of this change, which I've tabulated below for clarity.

Action Refreshes list Message
DELETE REPLAY Yes The system cannot find the specified file.
LOAD REPLAY No This replay cannot be loaded because the file no longer exists on this device.

The latest push uses readReplayMapInfo() to validate the file before playback (like loadReplay), shows the "REPLAY NOT FOUND" message, and refreshes the listbox on failure. Both the message and list refresh should now be consistent with deleteReplay. Ready for re-review when you get a chance.

@Stubbjax

Copy link
Copy Markdown

The latest push uses readReplayMapInfo() to validate the file before playback (like loadReplay), shows the "REPLAY NOT FOUND" message, and refreshes the listbox on failure. Both the message and list refresh should now be consistent with deleteReplay. Ready for re-review when you get a chance.

Is the latest push actually pushed? Issues still persist:

  1. The NOT FOUND message is inconsistent between load and delete actions.
  2. When attempting to load a replay, the game crashes after clicking OK on the NOT FOUND message.

@bobtista

Copy link
Copy Markdown
Author

The latest push uses readReplayMapInfo() to validate the file before playback (like loadReplay), shows the "REPLAY NOT FOUND" message, and refreshes the listbox on failure. Both the message and list refresh should now be consistent with deleteReplay. Ready for re-review when you get a chance.

Is the latest push actually pushed? Issues still persist:

  1. The NOT FOUND message is inconsistent between load and delete actions.
  2. When attempting to load a replay, the game crashes after clicking OK on the NOT FOUND message.

Yes latest was pushed, but you're right. RE Inconsistent messages — I'll switch to using the same FormatMessage/GetLastError pattern that deleteReplay uses, with "GUI:Error" as the title. RE Crash after clicking OK, I think that's something else - I'll investigate.

@bobtista
bobtista force-pushed the bobtista/fix-replay-deletion-crash branch from 6e161cc to c50a803 Compare February 25, 2026 00:02
@xezon
xezon requested a review from Stubbjax March 10, 2026 21:39
@xezon

xezon commented Mar 10, 2026

Copy link
Copy Markdown

Needs re-review @Stubbjax

@xezon xezon added GUI For graphical user interface Minor Severity: Minor < Major < Critical < Blocker ThisProject The issue was introduced by this project, or this task is specific to this project Crash This is a crash, very bad labels Mar 10, 2026
@xezon

xezon commented Jul 4, 2026

Copy link
Copy Markdown

@Stubbjax

@bobtista

Copy link
Copy Markdown
Author

Reworked this.

RE message consistency, reallyLoadReplay now re-validates with readReplayMapInfo() and shows the same "REPLAY NOT FOUND" message as loadReplay, instead of the raw OS FormatMessage string.

RE Crash after OK, RecorderClass::playbackFile(): it was setting m_mode = RECORDERMODETYPE_PLAYBACK before opening the file, so a failed open left the recorder in playback mode with a NULL m_file, and the next update dereferenced it. Moved the assignment after the successful header read.

Also drops the Win32/VC6 file-attribute code :) Ready for re-review.

Comment thread GeneralsMD/Code/GameEngine/Source/Common/Recorder.cpp Outdated
@bobtista
bobtista force-pushed the bobtista/fix-replay-deletion-crash branch from c50a803 to 89eec2b Compare July 25, 2026 11:44
@bobtista

Copy link
Copy Markdown
Author

Apologies - on 19 Jul I committed locally but never pushed, so you were both reviewing the February version. Pushed now and rebased onto current main.

@bobtista bobtista changed the title fix: crash when replay file is deleted during version mismatch prompt bugfix(replay): Handle replay deletion during version mismatch prompt Jul 25, 2026

@xezon xezon left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The bot has a complaint.

@xezon xezon left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code looks ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Crash This is a crash, very bad GUI For graphical user interface Minor Severity: Minor < Major < Critical < Blocker ThisProject The issue was introduced by this project, or this task is specific to this project

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Deleting a replay during the compatibility prompt leads to a crash

3 participants