Skip to content

FIX: Remove empty bracketed strings in hover menu#454

Open
mkanzler wants to merge 1 commit into
NagVis:masterfrom
mkanzler:fix-empty-bracketed-strings-hover-menu
Open

FIX: Remove empty bracketed strings in hover menu#454
mkanzler wants to merge 1 commit into
NagVis:masterfrom
mkanzler:fix-empty-bracketed-strings-hover-menu

Conversation

@mkanzler

@mkanzler mkanzler commented Jul 3, 2026

Copy link
Copy Markdown

Fixes the empty bracket variables in hover menu:
image

To this:
image

It still shows downtimes etc.

@loocars

loocars commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Overview

This changes the dynamic-macro substitution so that any [token] not matching a defined macro is replaced with an empty string instead of being left as-is:

// before (master)
return Object.prototype.hasOwnProperty.call(oMacros, name) ? oMacros[name] : match;
// after (this PR)
return Object.prototype.hasOwnProperty.call(oMacros, name) ? oMacros[name] : "";

The real problem it targets: optional state macros like obj_in_downtime, obj_acknowledged, obj_stale are only added to oMacros conditionally. When the object isn't in downtime/acknowledged/stale, the token has no entry, so master renders the literal text [obj_in_downtime] in the hover menu — the empty brackets in the first screenshot.

Critical issue: this reintroduces a bug that was just fixed 🔴

The changed line is the exact one added by commit ad0c4981"FIX: Keep bracketed strings in service descriptions in hover menu" — which is already on master (this PR's base). That commit deliberately returns match so bracketed text belonging to real data survives:

  • The macro pass runs over the whole template after child rows are substituted, so a child service description like Switch1 [xxx] is fed back through this same regex.
  • With this PR, [xxx] isn't a known macro → replaced with "" → the description renders as Switch1 — the precise regression ad0c4981 fixed.

The comment immediately above the changed line still says "Keep the original [name] text for tokens that are not actual macros", so after this change the code directly contradicts its own comment. The PR is effectively a partial revert of ad0c4981 and trades one bug for another.

Recommended approach

Both requirements are satisfiable at once — the two cases only look the same. The distinction is: known NagVis macro that is currently unset (drop it) vs. arbitrary bracketed text in data (keep it).

Pre-seed every optional macro with a default of "" so hasOwnProperty is true and it substitutes to empty, while genuinely-unknown tokens still fall through to match:

// defaults so unset optional macros collapse to "" instead of showing "[obj_...]"
const oMacros = {
    obj_summary_acknowledged: "", obj_acknowledged: "",
    obj_summary_in_downtime: "", obj_in_downtime: "",
    obj_summary_stale: "", obj_stale: "",
};

…then keep the : match branch unchanged. Optional macros collapse to empty; Switch1 [xxx] stays intact.

Other notes

  • Style: the _ rename of the unused arg is a fine cleanup and consistent with the codebase.
  • Test coverage: none. The interaction between the child-substitution pass and this macro pass is exactly the kind of regression that keeps recurring here — at minimum, verify manually against a service whose description contains literal brackets.
  • Security: unchanged and out of scope (this dynamic pass already does not htmlspecialchars its output, unlike replaceStaticMacros).

Verdict

Request changes. The intent is valid (unset optional macros shouldn't leak [obj_...] into the UI), but returning "" for all unknown tokens re-breaks bracketed service descriptions that ad0c4981 fixed and leaves the surrounding comment contradicting the code. Seed the known optional macros with empty defaults instead of dropping the : match fallback.

@loocars loocars left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

const oMacros = {
    obj_summary_acknowledged: "", obj_acknowledged: "",
    obj_summary_in_downtime: "", obj_in_downtime: "",
    obj_summary_stale: "", obj_stale: "",
};

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants