WIP external defensives#89
Conversation
- add reconcile and remove-stale flows for incomplete spell color keys - remove matching stale spell color entries from persisted and discovered stores - show full spell color key details on ctrl-hover - block duplicate extra icon adds and moves across viewers - refresh pending item drafts automatically and improve special-row ordering/tooltips - stop spell/item toggle clicks from refocusing the extra icon input - update dialogs, tests, docs, locale strings, and luacheck config
Experiment with embedded icons for the extra icons ui.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR broadens ECM’s “external defensives” work by introducing new external/extra icon modules, refactoring spell-color handling into scoped stores, and landing a large embedded LibSettingsBuilder v2-style API/docs/test suite update.
Changes:
- Refactors
SpellColorsinto per-scopeSpellColorStoreinstances and centralizes shared bar styling inBarStyle. - Updates runtime layout ordering (ExtraIcons first) and adds schema V12 migration (
itemIcons→extraIcons.viewers), removing the oldItemIconsmodule. - Reworks/extends settings library packaging (
embed.xml), docs, and tests; updates error messages and locales accordingly.
Reviewed changes
Copilot reviewed 63 out of 111 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/Modules/BuffBars_spec.lua | Updates BuffBars tests to use scoped SpellColors store + BarStyle/shared helpers. |
| Tests/Migration_spec.lua | Uses CURRENT_SCHEMA_VERSION in assertions and adds V12 migration tests. |
| Tests/ImportExport_spec.lua | Aligns tests to new import/export error message strings. |
| Tests/FrameMixin_spec.lua | Adds coverage for chain anchoring preference with ExtraIcons. |
| Tests/ECM_Runtime_spec.lua | Adds tests for Runtime.GetDesiredAlpha() and ExtraIcons layout ordering. |
| Tests/ChatCommand_spec.lua | Updates combat-blocked options message expectation. |
| Tests/BarMixin_spec.lua | Adds test ensuring BarStyle exports shared child-bar styling helpers. |
| SpellColors.lua | Introduces scoped SpellColorStore instances and new removal APIs; updates config/default handling. |
| Runtime.lua | Updates layout ordering (ExtraIcons first), adds GetDesiredAlpha(), and clarifies preview comment wording. |
| README.md | Expands project description and removes older sections; tweaks tagline text. |
| Modules/ItemIcons.lua | Removes old ItemIcons module implementation. |
| Modules/BuffBars.lua | Switches styling to BarStyle.StyleChildBar and routes spell colors through a store. |
| Migration.lua | Adds V12 migration from itemIcons flags to extraIcons.viewers. |
| Locales/en.lua | Renames/expands module strings; updates import/export and spell-colors UI strings. |
| Libs/LibSettingsBuilder/embed.xml | Adds ordered XML loader for split LibSettingsBuilder source files. |
| Libs/LibSettingsBuilder/docs/TROUBLESHOOTING.md | Updates troubleshooting guidance for new declarative API. |
| Libs/LibSettingsBuilder/docs/QUICK_START.md | Replaces imperative/table-driven examples with declarative root registration examples. |
| Libs/LibSettingsBuilder/docs/MIGRATION_GUIDE.md | Updates mapping guidance and emphasizes new canonical row types and list/input support. |
| Libs/LibSettingsBuilder/docs/INSTALLATION.md | Updates installation steps to load embed.xml and clarifies built-in vs custom controls. |
| Libs/LibSettingsBuilder/docs/API_REFERENCE.md | Re-documents the narrow public surface and declarative schema. |
| Libs/LibSettingsBuilder/Tests/Core_spec.lua | Adds core tests for split-library loading, public surface, and store/default bindings. |
| Libs/LibSettingsBuilder/Tests/Controls_spec.lua | Adds tests for mixin hook installation behavior. |
| Libs/LibSettingsBuilder/Tests/Collections_spec.lua | Adds tests for list/sectionList initializer creation. |
| Libs/LibSettingsBuilder/Tests/Builder_spec.lua | Adds builder tests for v2 public surface, deprecated field rejection, and pageActions. |
| Libs/LibSettingsBuilder/README.md | Updates library README to reflect new declarative API and row types. |
| Libs/LibSettingsBuilder/Primitives/Layout.lua | Adds layout/category creation primitives for the split library. |
| Libs/LibSettingsBuilder/Primitives/BlizzardControls.lua | Adds scroll dropdown support and slider inline editing hooks for Settings UI. |
| Libs/LibSettingsBuilder/Controls/Rows.lua | Adds layout row builders (Header/PageActions/Subheader/Info/Canvas/Button) and confirm dialog support. |
| Libs/LibSettingsBuilder/Controls/Collections.lua | Adds List/SectionList row builders and refresh handling. |
| Libs/LibSettingsBuilder/Controls/Base.lua | Adds canonical persisted controls including new input row type. |
| Libs/LibSettingsBuilder/CompositeControls/Lists.lua | Adds composite list helpers (ColorPickerList/CheckboxList). |
| Libs/LibSettingsBuilder/CompositeControls/Groups.lua | Adds composite group helpers (HeightOverrideSlider/FontOverrideGroup/BorderGroup). |
| Libs/LibEvent/LibEvent.lua | Simplifies re-embed instance creation behavior. |
| ImportExport.lua | Adjusts export/import error keys/messages and validation responses. |
| EnhancedCooldownManager.toc | Updates load order for new modules, BarStyle, and LibSettingsBuilder embed loader. |
| ECM.lua | Simplifies version metadata lookup and registers an additional bundled font. |
| Defaults.lua | Replaces itemIcons defaults with externalBars + extraIcons defaults and updates type docs. |
| Constants.lua | Replaces ItemIcons constants with ExternalBars/ExtraIcons constants and adds builtin stack/racial tables. |
| BarStyle.lua | Introduces shared BuffBars/ExternalBars child-bar styling helpers. |
| BarMixin.lua | Adjusts chain anchor selection to prefer ExtraIcons main anchor when enabled. |
| ARCHITECTURE.md | Updates architecture docs for ExtraIcons-first layout and new module set + SpellColors scoping. |
| AGENTS.md | Expands contributor guidance (simplicity, validation, deprecated APIs list). |
| .serena/memories/task_completion.md | Refreshes validation checklist and completion guidelines. |
| .serena/memories/style_and_conventions.md | Updates/condenses style + architecture rules. |
| .serena/memories/repo/options-load-order-acedb.md | Adds guidance about options load order before AceDB init. |
| .serena/memories/project_overview.md | Updates project overview for new structure and schema. |
| .serena/memories/externalbars-runtime-fade-ownership.md | Documents Runtime ownership of fade alpha and ExternalBars restore behavior. |
| .luacheckrc | Adds busted std for tests and expands globals/read_globals. |
| .github/agents/LuaReview.agent.md | Adds a Lua-focused review agent definition. |
| .github/agents/Iterate.agent.md | Adds an agent to orchestrate implement/review loops. |
| .github/agents/Developer.agent.md | Adds a developer agent definition with repo-specific constraints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| instance._events = instance._events or {} | ||
| instance._stats = instance._stats or {} | ||
| end | ||
|
|
There was a problem hiding this comment.
Removing the re-embed initialization means an existing embedded instance table that lacks _events/_stats will now keep them as nil, which can crash later when event registration assumes the tables exist. To preserve safe re-embed/upgrade behavior, reintroduce the minimal guards (e.g., instance._events = instance._events or {} / instance._stats = instance._stats or {}) when instance is already a table.
| instance._events = instance._events or {} | |
| instance._stats = instance._stats or {} |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7298a976e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Combine actions on the spell colours page.
Improved readability and fixed minor grammatical issues in the README.
Updated interface and notes for Enhanced Cooldown Manager.
Removed mention of detachable and customizable aura bar colors from notes.
|
@codex review this PR and fill in the description. Bucket related changes into larger omnibus bullet points. Highlight key features of extra icon and external defensives changes. |
|
@copilot Remove the fetch-libs call in the release pipeline. the unit test one works so review that. |
Agent-Logs-Url: https://github.com/argium/EnhancedCooldownManager/sessions/6c5181b9-9291-4f96-9a96-9a17f3c671b6 Co-authored-by: argium <15852038+argium@users.noreply.github.com>
Done — updated the PR description with omnibus bullet points covering Extra Icons, External Defensives, SpellColorStore refactor, schema migration, LibSettingsBuilder rewrite, and supporting changes.
The Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
To use Codex here, create a Codex account and connect to github. |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review this PR and fill in the description. Bucket related changes into larger omnibus bullet points. Highlight key features of extra icon and external defensives changes. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c6ddca914
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| end | ||
|
|
||
| local function resolveKnownSpell(spellId) | ||
| if spellId and C_SpellBook.IsSpellKnown(spellId) then |
There was a problem hiding this comment.
This resolver calls C_SpellBook.IsSpellKnown, which is explicitly banned by the repo rules in AGENTS.md under “Deprecated Blizzard APIs (12.0.5)”. Because this is a compatibility shim API, relying on it in core icon resolution makes ExtraIcons fragile to client updates: once the shim is removed or behavior diverges, spell-based extra icons will silently fail to resolve and disappear for users.
Useful? React with 👍 / 👎.
| end | ||
| ns.Runtime.ScheduleLayoutUpdate(0, "OptionsChanged") | ||
| refreshPage() |
There was a problem hiding this comment.
Option edits only schedule layout refreshes here, but ExtraIcons caches _trackedEquipSlots and uses that cache to gate PLAYER_EQUIPMENT_CHANGED handling; the cache is only rebuilt during OnEnable. After enabling/disabling equip-slot-backed entries in the options UI, equipment swaps can stop triggering immediate relayouts until reload/module re-enable, leaving trinket/equip-slot icons stale in normal runtime editing flows.
Useful? React with 👍 / 👎.
Update metrics docs to note that they're disabled by default. Add init serena to instructions.
No description provided.