From 206b25b8d69fb6e6a1983232e3933fc56dd2750d Mon Sep 17 00:00:00 2001 From: JonKazama-Hellion Date: Wed, 6 May 2026 19:18:20 +0200 Subject: [PATCH] fix(integrations): address review findings on HonorificService --- HellionChat/Integrations/HonorificService.cs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/HellionChat/Integrations/HonorificService.cs b/HellionChat/Integrations/HonorificService.cs index c4770b3..2361697 100644 --- a/HellionChat/Integrations/HonorificService.cs +++ b/HellionChat/Integrations/HonorificService.cs @@ -93,6 +93,7 @@ internal sealed class HonorificService : IDisposable } IsAvailable = true; + _versionWarningLogged = false; // Pull the current title once at startup; from here on we rely // on LocalCharacterTitleChanged events. var json = _getLocalCharacterTitle.InvokeFunc(); @@ -117,6 +118,11 @@ internal sealed class HonorificService : IDisposable // so the stale-title window between logout and login isn't user-visible. private void OnTitleChanged(string json) { + // Don't update cached state when we've already decided we can't trust + // Honorific (e.g. version mismatch). Subscription stays live in case a + // compatible Honorific reloads, in which case Ready triggers TryInitialPull + // and sets IsAvailable back to true. + if (!IsAvailable) return; CurrentTitle = ParseTitleJson(json); } @@ -133,6 +139,12 @@ internal sealed class HonorificService : IDisposable // Honorific is unloading. Drop our cached state so the header // hides on the next frame; subscriptions stay registered because // the gates may come back later (Honorific reload). + // + // Race-note: Honorific's NotifyDisposing calls ChangedLocalCharacterTitle(null) + // BEFORE SendMessage on the Disposing gate (IpcProvider.cs:109-111), + // so OnTitleChanged is expected to fire first and already null out + // CurrentTitle. We re-clear here as belt-and-braces; should the + // ordering ever flip, ShouldRenderSlot would still gate on IsAvailable. CurrentTitle = null; IsAvailable = false; DetectedApiVersion = null; @@ -155,6 +167,11 @@ internal sealed class HonorificService : IDisposable // render path that reads CurrentTitle never race. If a future change // moves either side onto a worker thread, switch to volatile/Interlocked // for the CurrentTitle field. + // + // Divergence from ChatTwo/Ipc/ExtraChat.cs: that file uses `volatile` + // on its state fields out of caution. We don't, because the framework- + // thread delivery is the documented Dalamud contract. If the two files + // ever need to share a threading audit, this is the place to revisit. // --- Pure-logic helpers below; tested via HellionChat.Tests/Integrations. ---