fix(beta): restore custom contact and avatar behavior - #147
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR restores two behaviors that were dropped on the beta branch: custom supplier NPCs now appear in the Contacts app relationship graph, and non-viewmodel equippables built with
Confidence Score: 4/5Safe to merge; all changes are additive and the existing viewmodel path is unchanged. The contacts patch and avatar dispatch logic are straightforward and well-scoped. The one area worth a second look is the callback-component swap in EquippableBuilder.Build(): a stale null-assign to callbackComponent.AvatarEquippable remains from the old implementation before ApplyAvatarPresentation overwrites it, which is harmless today but could confuse a future reader editing that path. The test also leaves private type registrations in the static NPC sets with no teardown, which could surface as unexpected noise if the test suite grows. Files Needing Attention: S1API/Items/EquippableBuilder.cs (stale assignment in callback-component swap path) and S1API.Tests/Entities/ContactsAppRoleTests.cs (no teardown for static NPC type registrations)
|
| Filename | Overview |
|---|---|
| S1API/Internal/Patches/ContactsAppPatches.cs | Adds IsContactRole helper and expands AddRelationCircles to include supplier NPCs alongside customers and dealers |
| S1API.Tests/Entities/ContactsAppRoleTests.cs | New unit test for IsContactRole; registers test types into static NPC sets without any teardown, leaving those types registered for the remainder of the test session |
| S1API/Internal/Patches/EquippableAvatarPatches.cs | New Harmony patch that drives SendEquippable_Networked for non-viewmodel equippables with a configured AvatarEquippable child component |
| S1API/Internal/Utils/RuntimePrefabCache.cs | New utility that parents prefab templates under a DontDestroyOnLoad inactive root, keeping activeSelf true while preventing component ticks |
| S1API/Items/EquippableBuilder.cs | Extracts ApplyAvatarPresentation into a shared method applied to all equippable types; replaces manual DontDestroyOnLoad+SetActive(false) with RuntimePrefabCache.Store; leaves one stale null-assign in the callback-component swap path |
Sequence Diagram
sequenceDiagram
participant GM as Game (Equippable.Equip)
participant HAP as EquippableAvatarPatches
participant TG as TryGetNonViewmodelAvatar
participant PL as Player.Local
GM->>HAP: Equip_Postfix(__instance)
HAP->>TG: TryGetNonViewmodelAvatar(equippable)
alt is Equippable_Viewmodel
TG-->>HAP: false (skip — native viewmodel lifecycle handles avatar)
else has AvatarEquippable child with non-empty AssetPath
TG-->>HAP: true, avatar
HAP->>PL: SendEquippable_Networked(avatar.AssetPath)
end
GM->>HAP: Unequip_Prefix(__instance)
HAP->>TG: TryGetNonViewmodelAvatar(equippable)
alt is Equippable_Viewmodel or no avatar child
TG-->>HAP: false (skip)
else has AvatarEquippable child
TG-->>HAP: true
HAP->>PL: SendEquippable_Networked(string.Empty)
end
Comments Outside Diff (1)
-
S1API/Items/EquippableBuilder.cs, line 188-189 (link)Stale null-assign before ApplyAvatarPresentation
At this point in the callback-component swap,
viewmodelEquippable.AvatarEquippable(saved intoavatarEquippabletwo lines earlier) is alwaysnull—ApplyAvatarPresentation()hasn't run yet and the freshly-created viewmodel has no avatar component attached. The assignmentcallbackComponent.AvatarEquippable = avatarEquippabletherefore unconditionally writesnull, whichApplyAvatarPresentation()(called at line 210) immediately overwrites with the correctly constructed component. The line is a no-op leftover from the previous implementation and can be removed.Prompt To Fix With AI
This is a comment left during a code review. Path: S1API/Items/EquippableBuilder.cs Line: 188-189 Comment: **Stale null-assign before ApplyAvatarPresentation** At this point in the callback-component swap, `viewmodelEquippable.AvatarEquippable` (saved into `avatarEquippable` two lines earlier) is always `null` — `ApplyAvatarPresentation()` hasn't run yet and the freshly-created viewmodel has no avatar component attached. The assignment `callbackComponent.AvatarEquippable = avatarEquippable` therefore unconditionally writes `null`, which `ApplyAvatarPresentation()` (called at line 210) immediately overwrites with the correctly constructed component. The line is a no-op leftover from the previous implementation and can be removed. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
S1API.Tests/Entities/ContactsAppRoleTests.cs:8-18
**Missing teardown for static NPC type registrations**
`NPC.RegisterCustomerType`, `RegisterDealerType`, and `RegisterSupplierType` write into static `HashSet` collections on the `NPC` class. The test registers `CustomerContact`, `DealerContact`, and `SupplierContact` but never removes them. Any other test that runs after this one in the same session and inspects NPC type counts or enumerates registered types will see these test-only private classes in the sets. Adding an `IDisposable` teardown (or `[Fact]`-level cleanup via `IAsyncLifetime`) that calls the corresponding deregister/clear helpers — or a dedicated test-scoped setup that uses a fresh static-reset path if one exists — would keep the test hermetic.
### Issue 2 of 2
S1API/Items/EquippableBuilder.cs:188-189
**Stale null-assign before ApplyAvatarPresentation**
At this point in the callback-component swap, `viewmodelEquippable.AvatarEquippable` (saved into `avatarEquippable` two lines earlier) is always `null` — `ApplyAvatarPresentation()` hasn't run yet and the freshly-created viewmodel has no avatar component attached. The assignment `callbackComponent.AvatarEquippable = avatarEquippable` therefore unconditionally writes `null`, which `ApplyAvatarPresentation()` (called at line 210) immediately overwrites with the correctly constructed component. The line is a no-op leftover from the previous implementation and can be removed.
Reviews (1): Last reviewed commit: "fix(beta): restore custom contact and av..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| [Fact] | ||
| public void IsContactRole_IncludesCustomersDealersAndSuppliers() | ||
| { | ||
| NPC.RegisterCustomerType(typeof(CustomerContact)); | ||
| NPC.RegisterDealerType(typeof(DealerContact)); | ||
| NPC.RegisterSupplierType(typeof(SupplierContact)); | ||
|
|
||
| Assert.True(ContactsAppPatches.IsContactRole(typeof(CustomerContact))); | ||
| Assert.True(ContactsAppPatches.IsContactRole(typeof(DealerContact))); | ||
| Assert.True(ContactsAppPatches.IsContactRole(typeof(SupplierContact))); | ||
| Assert.False(ContactsAppPatches.IsContactRole(typeof(NonContact))); |
There was a problem hiding this comment.
NPC.RegisterCustomerType, RegisterDealerType, and RegisterSupplierType write into static HashSet collections on the NPC class. The test registers CustomerContact, DealerContact, and SupplierContact but never removes them. Any other test that runs after this one in the same session and inspects NPC type counts or enumerates registered types will see these test-only private classes in the sets. Adding an IDisposable teardown (or [Fact]-level cleanup via IAsyncLifetime) that calls the corresponding deregister/clear helpers — or a dedicated test-scoped setup that uses a fresh static-reset path if one exists — would keep the test hermetic.
Prompt To Fix With AI
This is a comment left during a code review.
Path: S1API.Tests/Entities/ContactsAppRoleTests.cs
Line: 8-18
Comment:
**Missing teardown for static NPC type registrations**
`NPC.RegisterCustomerType`, `RegisterDealerType`, and `RegisterSupplierType` write into static `HashSet` collections on the `NPC` class. The test registers `CustomerContact`, `DealerContact`, and `SupplierContact` but never removes them. Any other test that runs after this one in the same session and inspects NPC type counts or enumerates registered types will see these test-only private classes in the sets. Adding an `IDisposable` teardown (or `[Fact]`-level cleanup via `IAsyncLifetime`) that calls the corresponding deregister/clear helpers — or a dedicated test-scoped setup that uses a fresh static-reset path if one exists — would keep the test hermetic.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Summary
Validation
dotnet build S1API.sln -c MonoMelon --no-restore -p:AutomateLocalDeployment=falsedotnet test S1API.Tests/S1API.Tests.csproj -c MonoMelon --no-restore --no-build --filter FullyQualifiedName~ContactsAppRoleTestsdotnet build S1API.sln -c Il2CppMelon --no-restore -p:AutomateLocalDeployment=falsedotnet test S1API.Tests/S1API.Tests.csproj -c Il2CppMelon --no-restore --no-build --filter FullyQualifiedName~ContactsAppRoleTestsCompatibility
No public or protected signatures, durable IDs, save formats, or network payload shapes changed. Existing viewmodel avatar behavior is preserved; non-viewmodel equippables now receive the configured avatar presentation.