Repository navigation
Conversation
JoeMatt
added
the
enhancement
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
7ed6b3e to
276be20
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| for game in games { | ||
| guard let filename = game.file?.fileName else { continue } | ||
| var crc: UInt32? | ||
| if livePatch.hasSourceCRC32, let url = game.file?.url, | ||
| let data = try? Data(contentsOf: url, options: .mappedIfSafe) { | ||
| crc = patchCRC32(data) | ||
| } else if !game.crc.isEmpty, let value = UInt32(game.crc, radix: 16) { | ||
| crc = value | ||
| } |
There was a problem hiding this comment.
Bug: The matchPatch function runs a blocking I/O loop on the main thread to calculate CRCs for BPS/UPS patches, which can freeze the UI with large game libraries.
Severity: HIGH
Suggested Fix
Move the file I/O and CRC calculation to a background thread. The matchPatch function should be made async and use await to call a helper function that performs the heavy lifting off the main thread. This will prevent the UI from freezing during the patch matching process. Consider adding filters to narrow down the games to scan, such as by system or file size, to improve performance.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: PVLibrary/Sources/PVLibrary/Patches/PatchRepository.swift#L133-L141
Potential issue: The `matchPatch` function is executed on the main thread due to the
`@MainActor` annotation on `PatchRepository`. When matching a BPS or UPS patch, the
function iterates through every game in the library. For each game, it reads the entire
ROM file from disk using `Data(contentsOf: url, options: .mappedIfSafe)` and then
calculates a CRC32 checksum. This entire process is a synchronous loop without any
`await` suspension points, blocking the main thread. For users with large game libraries
or large ROM files (e.g., CD images), this can cause a significant UI freeze,
potentially leading to an OS watchdog termination.
Did we get this right? 👍 / 👎 to inform future reviews.
| migration.enumerateObjects(ofType: PVPatch.className()) { oldObject, newObject in | ||
| newObject?["baseGameMD5"] = oldObject?["game"] != nil ? nil : nil | ||
| newObject?["matchState"] = "pending" | ||
| newObject?["matchConfidenceRaw"] = "none" | ||
| newObject?["matchScore"] = 0.0 | ||
| newObject?["hasSourceCRC32"] = false | ||
| newObject?["sourceCRC32"] = 0 | ||
| newObject?["showAsLibraryTile"] = false | ||
| newObject?["isDefaultLaunch"] = (oldObject?["isEnabled"] as? Bool) ?? false | ||
| } |
There was a problem hiding this comment.
Bug: A database migration incorrectly sets baseGameMD5 to nil and matchState to pending for all legacy patches, causing orphaned patch files and incorrect re-matching behavior.
Severity: MEDIUM
Suggested Fix
Correct the migration logic to properly populate the baseGameMD5 field from the linked game's md5Hash. The logic should be something like newObject?["baseGameMD5"] = oldObject?["game"]?.md5Hash. Also, avoid resetting matchState to pending for patches that already have a valid game link.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: PVLibrary/Sources/PVLibrary/Database/Realm
Database/RomDatabase.swift#L309-L318
Potential issue: In the schema 29 migration, the logic `newObject?["baseGameMD5"] =
oldObject?["game"] != nil ? nil : nil` always sets `baseGameMD5` to `nil` for existing
`PVPatch` objects. Additionally, all legacy patches are marked with `matchState =
"pending"`, even those that were already correctly linked to a game. This causes issues
where deleting a base game leaves its patch records behind. It also causes legacy
patches to be re-matched, potentially overwriting a user's deliberate patch link.
Did we get this right? 👍 / 👎 to inform future reviews.
| var bytes = [UInt8]() | ||
| while index + bytes.count < maxLen { | ||
| let i = index + bytes.count | ||
| let same = i < original.count && i < modified.count && original[i] == modified[i] | ||
| if same { break } | ||
| let value: UInt8 = i < modified.count ? modified[i] : 0 | ||
| bytes.append(value) | ||
| // Cap TargetRead chunks to keep VLI encoding simple / bounded. | ||
| if bytes.count >= 1024 { break } | ||
| } |
There was a problem hiding this comment.
Bug: Creating a BPS patch fails when the modified ROM is smaller than the original, as the creator generates a patch with invalid padding that the patcher rejects.
Severity: HIGH
Suggested Fix
Adjust the PatchCreator.createBPS logic to handle cases where the modified file is smaller than the original. The loop should not generate TargetRead data beyond the bounds of the modified file's content. Ensure the generated patch data accurately reflects the changes without adding extra padding that causes an overflow.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: PVPatching/Sources/PVPatching/Patchers/PatchCreator.swift#L138-L147
Potential issue: When creating a BPS patch for a modified ROM that is smaller than the
original, `PatchCreator.createBPS` generates a corrupt patch. The creation loop iterates
up to the length of the longer file (`maxLen`), but the patch header declares the
shorter, modified file's size. When the loop continues past the end of the modified
file, it appends zero bytes as `TargetRead` data. The patcher (`BPSPatcher.apply`)
detects that this padding would write past the declared target size and throws a
`corruptPatchFile` error, causing patch creation to fail.
Did we get this right? 👍 / 👎 to inform future reviews.
7949fc4 to
a1bae28
Compare
JoeMatt
changed the title
a1bae28 to
55d35c4
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 5 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4babdae. Configure here.
| variant.userPreferredCoreID = liveBase?.userPreferredCoreID | ||
| variant.isDownloaded = true | ||
| variant.file = PVFile(withURL: artifact.url) | ||
| variant.romPath = artifact.url.lastPathComponent |
There was a problem hiding this comment.
Variant files resolve to the wrong directory
High Severity
ensureVariant and prepareForLaunch attach the cached patched ROM via PVFile(withURL:) using the default Documents root. PatchCache writes under Library/Caches/PVPatchedROMs, and FileLocationResolver on iOS only searches Documents. The variant file is therefore treated as missing. Launch then falls back to the system ROMs folder by filename, which can silently open the original ROM and rewrite the variant partialPath to that original.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 4babdae. Configure here.
| // Deleting a variant tile removes that patch + variant only (original ROM stays). | ||
| if game.isPatchVariant { | ||
| deletePatchRecords(linkedToVariantMD5: md5, patchID: game.patchID) | ||
| return |
There was a problem hiding this comment.
Deleting a variant can remove the original ROM
High Severity
delete(game:) always removes path(forGame:), which is ROMs/<system>/<filename>, before the isPatchVariant branch. Variant artifacts live in the patch cache, but their filename often matches the base ROM. Deleting a visible variant tile (or its saves) can erase the original ROM and its save directories. The cache file is never removed.
Reviewed by Cursor Bugbot for commit 4babdae. Configure here.
| existing.baseGameMD5 = liveBase?.md5Hash | ||
| existing.patchID = livePatch?.id | ||
| result = existing | ||
| return |
There was a problem hiding this comment.
Matching MD5 converts an existing library game
High Severity
When the patched MD5 already exists as a normal PVGame (a common ROM-hack import), ensureVariant and prepareForLaunch reuse that row and set isPatchVariant / patchID without creating a separate identity. The standalone game is absorbed into the patch. Deleting the base game later removes it. isHiddenPatchVariant is also left unchanged on this path.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 4babdae. Configure here.
| /// - Returns: A query result with live updating as long as the reference is active | ||
| public func gamesForSystem(systemIdentifier: String) -> Results<GameType> { | ||
| database.all(GameType.self).filter(NSPredicate(format: "systemIdentifier == %@", argumentArray: [systemIdentifier])) | ||
| database.all(GameType.self).filter( |
There was a problem hiding this comment.
Hidden variants still appear in some library queries
Medium Severity
gamesForSystem and some SwiftUI observers exclude isHiddenPatchVariant, but favoritesResults, mostPlayedResults, and searchResults do not. An empty search returns every game, including hidden variants. Playing a default patch increments the variant’s playCount, so it can show up in Most Played and search even when “Add as separate tile” was never chosen.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 4babdae. Configure here.
| // Deleting a variant tile removes that patch + variant only (original ROM stays). | ||
| if game.isPatchVariant { | ||
| deletePatchRecords(linkedToVariantMD5: md5, patchID: game.patchID) | ||
| return |
There was a problem hiding this comment.
Variant delete can leave the game record behind
Medium Severity
When isPatchVariant is true, delete(game:) only calls deletePatchRecords and returns. If no matching PVPatch exists, or deleteSinglePatchRecord fails, the PVGame row is never deleted. That path also skips screenshot cleanup, Spotlight, and PatchRepository.deletePatch cache invalidation, so variants can remain in the library after the user deletes them.
Additional Locations (1)
- [
PVLibrary/Sources/PVLibrary/Database/Realm Database/RomDatabase.swift#L1214-L1235](https://github.com/Provenance-Emu/Provenance/blob/4babdae8900bafe73dc7018fdd6d41fbbcf2e08d/PVLibrary/Sources/PVLibrary/Database/Realm Database/RomDatabase.swift#L1214-L1235)
Reviewed by Cursor Bugbot for commit 4babdae. Configure here.
205f22f to
a05c195
Compare
Extend PVPatching with PatchInspector matching, BPS/IPS/IPS32 PatchCreator (round-trip verified), typed PatchArtifact results, and human-readable unique cache stems so patched saves stay isolated. Co-authored-by: Joseph Mattiello <jmattiello+wayfair@wayfair.com>
Persist base/variant links and match state on PVPatch/PVGame with a schema 29 migration. Add PatchRepository for import/match/variant/ export/delete, wire GameImporter to it, rematch pending patches after ROM imports, and hide patch variants from normal library queries. Co-authored-by: Joseph Mattiello <jmattiello+wayfair@wayfair.com>
Add a Patches context menu (Play Original, per-patch launch, import, manage), PatchManagerView / PendingPatchesView / CreatePatchView under Library Management, and SceneCoordinator async patch preparation with default-patch redirect and Play Original bypass. Co-authored-by: Joseph Mattiello <jmattiello+wayfair@wayfair.com>
- Compute patch-match CRCs off the main actor; prefer stored game.crc - Preserve linked baseGameMD5/matchState in schema 29 migration - Stop emitting zero-padding when the modified ROM is shorter Co-authored-by: Joseph Mattiello <jmattiello+wayfair@wayfair.com>
Avoid stacking two SwiftUI fileImporter modifiers and encode patches off the main actor so Create Patch stays responsive. Co-authored-by: Joseph Mattiello <jmattiello+wayfair@wayfair.com>
- Store patched ROMs under Documents/PatchedROMs (Caches on tvOS) - Never absorb a standalone same-MD5 library game into a patch variant - Skip ROMs/ path deletion for variants; remove PatchedROMs artifacts only - Orphan-clean variant PVGame rows when PVPatch is missing - Hide patch variants from favorites, most-played, recents, and search Co-authored-by: Joseph Mattiello <jmattiello+wayfair@wayfair.com>
a05c195 to
c654e18
Compare
This pull request introduces comprehensive support for ROM patch variants in the library database, including new model fields, migration logic, deletion handling, and improved patch import workflows. It ensures patch variants are tracked, hidden from normal library queries by default, and properly associated with their base games and patch metadata. The changes also add unit tests to verify the new patch model behavior.
Database model and migration changes:
PVGameandPVPatchto support patch variants, includingisPatchVariant,baseGameMD5,patchID,isHiddenPatchVariant(forPVGame), and detailed patch metadata such asvariantGame,variantGameMD5,matchScore,matchConfidenceRaw,matchState, and others forPVPatch. [1] [2] [3]Query and deletion logic updates:
Patch import and matching workflow:
PatchRepository, enabling patch matching, variant creation, and idempotent re-imports. After importing a ROM, pending patches are re-matched automatically. [1] [2] [3]Model and computed property improvements:
PVPatchfor easier access to match confidence, display title, and CRC32 value. Improved idempotency and stability of patch primary keys. [1] [2]Testing:
PVPatchModelTeststo verify idempotent patch keys, default hidden state for variants, and correct default match state.### What does this PR doWhere should the reviewer start
How should this be manually tested
Any background context you want to provide
What are the relevant tickets
Screenshots (important for UI changes)
Questions
Note
Medium Risk
Realm migration plus new launch routing and deletion cascades affect core library/ROM behavior, though base ROM files are explicitly preserved.
Overview
Adds end-to-end ROM patch support: import IPS/BPS/UPS patches, auto-match them to base ROMs, apply patches into cached variants with separate saves, and manage them from the library UI.
Data & lifecycle: Realm schema 29 adds patch-variant fields on
PVGame/PVPatch(hidden variants, match state, default launch, variant MD5). Library queries excludeisHiddenPatchVariantuntil the user adds a separate tile.PatchRepositoryowns import, matching, variant creation, launch prep, export, and deletion;GameImporterroutes patch files through it and rematches pending patches after new ROM imports. Deleting a base game cascades patches/variants; deleting a variant removes only that patch.PVPatching:
PatchApplierreturnsPatchArtifactwith hashes, human-readable cache stems, progress, and export; newPatchInspector(CRC/filename ranking),PatchCreator(BPS/IPS with round-trip verify), and shared hashing/cache migration from legacypatched.*names.UI & launch: Game context Patches menu (play original, play patch, import, manage), settings Patches / per-game
PatchManagerView, andCreatePatchView.SceneCoordinatorcan redirect normal launches to a default patch or launch a patch viaprepareForLaunchinto an isolated variant game.Reviewed by Cursor Bugbot for commit 4babdae. Configure here.