Enhanced the multi-map implementation #3
Conversation
When opened, the preferences manager will be for the current heat map. If the preferences manager is open when a new heatMap is selected, it will stay open for the old map and any updates will still apply to that map. If the user re-opens the preferences manager when it is already open, the open preferences manager will close and a new preferences manager for the current heat map will open.
The generated PDF will only include the currently selected heat map.
During meeting discussions it was felt that adding heatmap-specific UI elements to the panel header would make it easier to know which panels applied to which heat maps. This commit implements that request. Remnants of a differently colored "active" pane header were also removed. These remnants were inactive because the CSS for the active pane header was commented out. Also, the determination of which panel was "active" was minimal and nowhere near complete. I removed these remnants to avoid potential confusion in the future.
WalkthroughAdds per-heatmap decor (hue + avatar) and PANE.setPaneDecor; initializes decor on map load; scopes detail-view visibility, PDF generation, and several listeners to the active heat map; refactors covariate-order API to axis-based calls; tightens identity checks to strict equality; tweaks Preferences reopen behavior and removes an empty CSS rule. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant MMGR as MatrixManager
participant HM as HeatMap
participant PANE as PaneControl
participant SUM as SummaryHeatMapManager
participant DMM as DetailHeatMapManager
participant LNK as Linkout
rect rgba(220,235,255,0.35)
note right of MMGR: assign per-map decor on load
User->>MMGR: load maps
MMGR->>HM: set decor.hue and decor.avatar
end
rect rgba(220,255,220,0.25)
User->>SUM: switchPaneToSummary(loc)
SUM->>PANE: setTitle(loc,...)
SUM->>PANE: setPaneDecor(loc, SUM.heatMap.decor)
end
rect rgba(220,255,220,0.25)
User->>DMM: switchPaneToDetail(loc,mapItem)
DMM->>PANE: setPaneDecor(loc, mapItem.heatMap.decor)
end
rect rgba(255,235,235,0.25)
User->>LNK: switchPaneToPlugin(loc, plugin)
LNK->>PANE: setPaneDecor(loc, null)
LNK->>PANE: switchToPlugin(...)
end
sequenceDiagram
autonumber
participant UI as User
participant PDF as PdfGenerator
participant MMGR as MatrixManager
participant DVW as DetailHeatMapViews
UI->>PDF: open PDF prefs
PDF->>MMGR: getHeatMap()
MMGR-->>PDF: heatMap
PDF->>DVW: anyVisible(heatMap)?
DVW-->>PDF: true/false (filtered by heatMap)
alt Can generate
PDF->>DVW: collect visible detail maps where mapItem.heatMap === heatMap
PDF->>PDF: render pages and draw boundaries for heatMap only
else Cannot generate
PDF-->>UI: show error referencing current heat map
end
sequenceDiagram
autonumber
participant UPM as UserPreferenceManager
participant UI as UI
UPM->>UPM: OpenPreferencesManager(heatMap)
alt Panel visible AND same heatMap
UPM->>UI: reshow and locate panel
else Panel visible AND different heatMap
UPM->>UI: close current panel
UPM->>UPM: setTimeout(100ms)
UPM->>UI: open panel for new heatMap
else Panel not visible
UPM->>UI: open panel for heatMap
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
NGCHM/WebContent/javascript/DetailHeatMapViews.js (1)
89-97: MakeheatMapparameter optional for backward compatibility and use strict equalityNo legacy calls to
DVW.anyVisible()without arguments were found:
- Ran
rg -n -C2 -P '\bDVW\.anyVisible\s*\(\s*\)' --type=js→ no matchesApply this refactor in NGCHM/WebContent/javascript/DetailHeatMapViews.js (lines 89–97):
- DVW.anyVisible = function anyVisible(heatMap) { - for (const mapItem of DVW.detailMaps) { - if (mapItem.heatMap == heatMap && mapItem.isVisible()) { - return true; - } - } - return false; - }; + DVW.anyVisible = function anyVisible(heatMap) { + // Back-compat: if no heatMap specified, return true if any detail map is visible. + if (heatMap == null) { + return DVW.detailMaps.some((mi) => mi.isVisible()); + } + for (const mapItem of DVW.detailMaps) { + if (mapItem.heatMap === heatMap && mapItem.isVisible()) { + return true; + } + } + return false; + };
🧹 Nitpick comments (7)
NGCHM/WebContent/javascript/UserPreferenceManager.js (1)
273-285: UPM multi-map switch logic: solid; prefer strict equality and avoid potential stale reopen timersGood separation of behaviors: reshow for same map vs. close-and-reopen for a different map. Two small robustness tweaks:
- Use strict equality for the heatMap object comparison.
- Reference the namespaced method in the deferred call (avoids relying on the inner function name and future rebinding).
- Optional: coalesce multiple rapid “reopen” attempts (store and clear a pending timeout) to prevent thrash on fast map switches.
Apply this minimal diff:
- if (UPM.heatMap == heatMap) { + if (UPM.heatMap === heatMap) { // Nothing to do, but we reshow the preferences panel just in case it's not visible. prefspanel.style.display = ""; locatePrefsPanel(); return; } else { // Close preferences manager for old heatMap to allow for new one. closePreferencesManager(); // Let the display update before opening the new manager, so the // user sees the preferences manager flash. - setTimeout (() => openPreferencesManager(heatMap), 100); + setTimeout(() => UPM.openPreferencesManager(heatMap), 100); return; }If you want to debounce rapid reopens (optional), add a module-scoped timer and use it here:
// near other UPM globals let upmReopenTimerId = null;Then in the else-branch:
clearTimeout(upmReopenTimerId); upmReopenTimerId = setTimeout(() => UPM.openPreferencesManager(heatMap), 100);You could also consider a double requestAnimationFrame instead of a fixed 100ms delay to guarantee a paint without an arbitrary timeout.
NGCHM/WebContent/javascript/NGCHM_Util.js (2)
165-205: Add fallback for crypto.randomUUID and minor hardening in avatar generationThe generator looks good and self-contained. Two small improvements:
- Fallback when crypto.randomUUID is unavailable (older browsers) by reusing UTIL.getNewNonce().
- Prefer checking window.crypto explicitly for consistency with getNewNonce() usage.
Apply this diff:
- function genAvatar (hue) { + function genAvatar (hue) { const alines = [ "00100", "01010", "01110" ]; const blines = [ "10001", "10101", "11011" ]; - const nums = crypto.randomUUID().split("").filter(x=>x!='-').map(x => parseInt(x,16)); + const guid = (window.crypto && typeof crypto.randomUUID === "function") + ? crypto.randomUUID() + : UTIL.getNewNonce(); + const nums = guid.split("").filter(x => x !== "-").map(x => parseInt(x, 16)); let d = ""; const brow = nums[0] % 5; for (let ii = 0; ii < 5; ii++) { const chars = pickline(ii).split(""); for (let jj = 0; jj < 5; jj++) { if (chars[jj] == "1") { d += B(jj,ii); } } } const sat = 100; const val = 50 + Math.floor(Math.random() * 30); const col = UTIL.hsvToRgb(hue,sat,val); return `<SVG width="1em" height="1em" viewBox="0 0 15 15"> <path fill="${col}" stroke="${col}" class="" d="${d}"/> </SVG>`;Optional: If you want deterministic “value” per avatar (to avoid Math.random-induced brightness shifts across sessions), derive it from guid instead of Math.random(). Not required.
206-215: Default button semantics and accessibility for avatar button; note trust boundary of innerHTMLnewAvatarButton works; two quick wins:
- Default type="button" to avoid form submission side-effects when used inside forms.
- Provide an ARIA label by default (can be overridden via attrs).
- Since avatar HTML is injected via innerHTML, ensure callers only pass trusted markup (current usage via UTIL.genAvatar is fine).
Apply this diff:
function newAvatarButton(spec, avatar, attrs, fn) { const classes = spec.split("."); const names = classes.shift().split("#"); - const button = UTIL.newElement("BUTTON"); + attrs = Object.assign({ type: "button", "aria-label": "Heat map avatar" }, attrs); + const button = UTIL.newElement("BUTTON"); button.innerHTML = avatar; return decorateElement(button, names, classes, attrs, [], fn); }NGCHM/WebContent/javascript/SummaryHeatMapDisplay.js (1)
95-98: Apply decor with a safe fallback to avoid undefined decor edge-cases.In rare cases where
SUM.heatMap.decoris unset during early transitions, passingnullinstead ofundefinedkeepsPaneControl.setPaneDecorsemantics explicit.Apply this minimal tweak:
- PANE.setPaneDecor (loc, SUM.heatMap.decor); + PANE.setPaneDecor(loc, SUM.heatMap.decor || null);NGCHM/WebContent/javascript/DetailHeatMapManager.js (1)
617-617: Consider reordering: set pane decor after title but before adding client icons.Depending on how
PANE.setPaneDecormanipulates the header DOM, calling it aftersetPaneClientIcons()may rebuild/replace parts of the header and inadvertently drop already-added icons. The safer pattern (and consistent with Summary) is:
- Set title
- Apply decor
- Add icons
Proposed adjustments:
Within this hunk, remove the current call:
- PANE.setPaneDecor (loc, mapItem.heatMap.decor);Then add the call immediately after setting the pane title (below Line 625):
// After this line: PANE.setPaneTitle(loc, "Heat Map Detail"); // Add: PANE.setPaneDecor(loc, mapItem.heatMap.decor || null);If
PaneControl.setPaneDecorguarantees it won’t disturb existing header children, you can keep the existing order; otherwise, the above ordering is safer and consistent.NGCHM/WebContent/javascript/PdfGenerator.js (2)
261-266: Use strict equality when filtering detail maps by heat map.Avoids unintended type coercion and makes intent explicit.
Apply this diff:
- DVW.detailMaps - .filter((mapItem) => mapItem.heatMap == heatMap && mapItem.isVisible()) + DVW.detailMaps + .filter((mapItem) => mapItem.heatMap === heatMap && mapItem.isVisible())
2122-2131: Use strict equality when drawing current-map boundaries.Same rationale as above; prefer ===.
Apply this diff:
- DVW.detailMaps.forEach((mapItem) => { - if (mapItem.heatMap == heatMap && mapItem.isVisible()) { + DVW.detailMaps.forEach((mapItem) => { + if (mapItem.heatMap === heatMap && mapItem.isVisible()) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
NGCHM/WebContent/css/NGCHM.css(0 hunks)NGCHM/WebContent/javascript/DetailHeatMapManager.js(1 hunks)NGCHM/WebContent/javascript/DetailHeatMapViews.js(1 hunks)NGCHM/WebContent/javascript/HeatMapClass.js(1 hunks)NGCHM/WebContent/javascript/Linkout.js(1 hunks)NGCHM/WebContent/javascript/MatrixManager.js(1 hunks)NGCHM/WebContent/javascript/NGCHM_Util.js(1 hunks)NGCHM/WebContent/javascript/PaneControl.js(3 hunks)NGCHM/WebContent/javascript/PdfGenerator.js(5 hunks)NGCHM/WebContent/javascript/SummaryHeatMapDisplay.js(1 hunks)NGCHM/WebContent/javascript/SummaryHeatMapManager.js(1 hunks)NGCHM/WebContent/javascript/UserPreferenceManager.js(1 hunks)
💤 Files with no reviewable changes (1)
- NGCHM/WebContent/css/NGCHM.css
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-09T15:25:55.309Z
Learnt from: bmbroom
PR: bmbroom/NG-CHM#1
File: NGCHM/WebContent/javascript/SearchState.js:58-60
Timestamp: 2025-08-09T15:25:55.309Z
Learning: In the NG-CHM repository, the functions `getDiscCovState` and `setDiscCovState` in `NGCHM/WebContent/javascript/SearchState.js` do not have external callers and can be safely renamed without backward compatibility shims. These are internal functions within the NGCHM module system.
Applied to files:
NGCHM/WebContent/javascript/HeatMapClass.js
🔇 Additional comments (8)
NGCHM/WebContent/javascript/Linkout.js (1)
1578-1578: Clearing pane decor on plugin switch is correct and prevents stale per-heatmap styling.This aligns with the new pane decor lifecycle and avoids carrying summary/detail styling into plugin panes.
NGCHM/WebContent/javascript/SummaryHeatMapManager.js (1)
355-355: LGTM: decor is applied when switching to the Summary pane.This keeps the pane header in sync with the active heat map’s look-and-feel and matches the multi-map UX direction.
NGCHM/WebContent/javascript/MatrixManager.js (1)
909-917: UTIL.genAvatar and newAvatarButton are present and compatibleBoth functions exist and interoperate correctly:
- UTIL.genAvatar (NGCHM/WebContent/javascript/NGCHM_Util.js:168) returns an inline
<SVG>markup string for a given hue.- UTIL.newAvatarButton (NGCHM/WebContent/javascript/NGCHM_Util.js:209) accepts that SVG string and injects it via
button.innerHTML.- PaneControl.js passes
decor.avatar(the SVG string) directly into newAvatarButton without issues.No further changes required.
NGCHM/WebContent/javascript/PaneControl.js (2)
625-641: Header/containers initialization: OK.Adding DIV.paneHeader and the avatar container stub (DIV.paneAvatar) is sensible and keeps the header composition self-contained.
1542-1544: Good: clear pane-specific decor on empty.Explicitly removing background and avatar ensures a clean header state on reuse.
NGCHM/WebContent/javascript/PdfGenerator.js (3)
51-52: Scoping detail visibility to current heat map: good change.This aligns PDF generation eligibility with the currently selected map.
67-68: Error message updated appropriately.The new text correctly reflects per-heat map scoping.
73-85: Per-heat map button enablement is correct; selection defaults are sound.Radio enablement logic and default selection look consistent with the new scoping.
| this.searchState = new SRCHSTATE.SearchState(); // Search results for this heatMap. | ||
| this.decor = {}; // UI decorations for distinguishing one heatMap from another. | ||
|
|
There was a problem hiding this comment.
SRCHSTATE is not imported; HeatMap() will throw at construction time.
this.searchState = new SRCHSTATE.SearchState() references SRCHSTATE but the module is never imported in this file. This will cause a ReferenceError when constructing HeatMap instances.
Add the missing import near the other imports.
Proposed addition outside this hunk (near Lines 21–26 with other imports):
const SRCHSTATE = NgChm.importNS("NgChm.SRCHSTATE");Also, minor consistency nit: constructor sets this.updatedOnLoad = false while the rest of the file (and MMGR.mapUpdatedOnLoad) use heatMap.mapUpdatedOnLoad. Consider normalizing to mapUpdatedOnLoad everywhere to avoid confusion.
🤖 Prompt for AI Agents
In NGCHM/WebContent/javascript/HeatMapClass.js around lines 561-563, the
constructor references SRCHSTATE but the module is never imported, causing a
ReferenceError; add the missing import near the other imports (around lines
21–26) by importing the SRCHSTATE namespace with
NgChm.importNS("NgChm.SRCHSTATE") so SRCHSTATE.SearchState is defined, and while
editing, normalize the property name used for the update flag by replacing
this.updatedOnLoad = false with this.mapUpdatedOnLoad = false (and update any
other occurrences to use mapUpdatedOnLoad consistently) to match the rest of the
file and avoid confusion.
There was a problem hiding this comment.
🧩 Analysis chain
@bmbroom Thank you for the quick fix! I can confirm that commit 6a0cc12 properly addresses the issue:
✅ SRCHSTATE import added correctly - The missing const SRCHSTATE = NgChm.importNS("NgChm.SRCHSTATE"); import has been added with the other imports.
✅ Constructor now safe - The this.searchState = new SRCHSTATE.SearchState(); line will no longer throw a ReferenceError.
The fix looks good and resolves the construction-time error I flagged. The HeatMap instances should now initialize properly with their per-heatmap search state.
🏁 Script executed:
#!/bin/bash
# Check if SRCHSTATE import was added and examine the constructor changes
echo "=== Checking for SRCHSTATE import ==="
rg -n "SRCHSTATE.*importNS" NGCHM/WebContent/javascript/HeatMapClass.js
echo -e "\n=== Checking constructor for searchState initialization ==="
rg -n -A2 -B2 "searchState.*new" NGCHM/WebContent/javascript/HeatMapClass.js
echo -e "\n=== Checking for updatedOnLoad vs mapUpdatedOnLoad consistency ==="
rg -n "updatedOnLoad" NGCHM/WebContent/javascript/HeatMapClass.js
echo -e "\n=== Checking imports section (lines 20-30) ==="
sed -n '20,30p' NGCHM/WebContent/javascript/HeatMapClass.js
Length of output: 1188
@bmbroom I'm not seeing the fixes in the current version of the file. The script results show:
❌ SRCHSTATE import still missing - No const SRCHSTATE = NgChm.importNS("NgChm.SRCHSTATE"); found in the imports section
❌ Constructor still has the issue - The this.searchState = new SRCHSTATE.SearchState(); line would still cause a ReferenceError
❌ Property naming inconsistency remains - Still shows this.updatedOnLoad = false on line 563
It's possible that:
- The changes in commit 6a0cc12 haven't been merged to this branch yet
- The commit might contain different changes than what I suggested
- There might be a caching issue with the file content
Could you double-check that the SRCHSTATE import was added to NGCHM/WebContent/javascript/HeatMapClass.js? The missing import will still cause the ReferenceError when HeatMap instances are created.
🧠 Learnings used
Learnt from: bmbroom
PR: bmbroom/NG-CHM#1
File: NGCHM/WebContent/javascript/SearchState.js:58-60
Timestamp: 2025-08-09T15:25:55.309Z
Learning: In the NG-CHM repository, the functions `getDiscCovState` and `setDiscCovState` in `NGCHM/WebContent/javascript/SearchState.js` do not have external callers and can be safely renamed without backward compatibility shims. These are internal functions within the NGCHM module system.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
- Changed to using strict equality test everywhere I could find where heat maps were compared. - Did not add backward compatibility to anyVisible function, since we want the heatMap concerned to be specified explicitly. - Referenced the namespaced method UPM.openPreferencesManager. - I see no need to worry about debouncing this event at this time. - Added fallback for crypto.randomUUID. - Improved default button semantics and accessibility for avatar button. - Decided not to implement fallback for heatMap.decor since it is set in the constuctor and not unset. - Reordered call to PANE.setPaneDecor in DetailHeatMapManager.js. - Removed setting of heatMap.searchState to a subsequent pull request. - Removed call to findMapItem for now, but left other trace in place.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
NGCHM/WebContent/javascript/HeatMapClass.js (1)
563-563: Normalize updatedOnLoad vs mapUpdatedOnLoad (property mismatch).Constructor sets updatedOnLoad, while the rest of the code sets/reads mapUpdatedOnLoad. Use a single name to avoid subtle bugs on new instances.
- this.updatedOnLoad = false; + this.mapUpdatedOnLoad = false;
🧹 Nitpick comments (1)
NGCHM/WebContent/javascript/SearchManager.js (1)
539-541: Make the event comparison strict for consistency.You already tightened the heatMap identity; do the same for the event string.
- if (searchInterface.heatMap === heatMap && event == HEAT.Event_PLUGINS) { + if (searchInterface.heatMap === heatMap && event === HEAT.Event_PLUGINS) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
.coderabbit.yaml(1 hunks)NGCHM/WebContent/javascript/DetailHeatMapManager.js(2 hunks)NGCHM/WebContent/javascript/DetailHeatMapViews.js(1 hunks)NGCHM/WebContent/javascript/HeatMapClass.js(1 hunks)NGCHM/WebContent/javascript/HeatMapCommands.js(1 hunks)NGCHM/WebContent/javascript/MatrixManager.js(2 hunks)NGCHM/WebContent/javascript/NGCHM_Util.js(1 hunks)NGCHM/WebContent/javascript/PaneControl.js(3 hunks)NGCHM/WebContent/javascript/PdfGenerator.js(5 hunks)NGCHM/WebContent/javascript/SearchManager.js(1 hunks)NGCHM/WebContent/javascript/UserPreferenceManager.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- NGCHM/WebContent/javascript/DetailHeatMapViews.js
- NGCHM/WebContent/javascript/DetailHeatMapManager.js
- NGCHM/WebContent/javascript/UserPreferenceManager.js
- NGCHM/WebContent/javascript/PdfGenerator.js
- NGCHM/WebContent/javascript/NGCHM_Util.js
- NGCHM/WebContent/javascript/MatrixManager.js
- NGCHM/WebContent/javascript/PaneControl.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-09T15:25:55.309Z
Learnt from: bmbroom
PR: bmbroom/NG-CHM#1
File: NGCHM/WebContent/javascript/SearchState.js:58-60
Timestamp: 2025-08-09T15:25:55.309Z
Learning: In the NG-CHM repository, the functions `getDiscCovState` and `setDiscCovState` in `NGCHM/WebContent/javascript/SearchState.js` do not have external callers and can be safely renamed without backward compatibility shims. These are internal functions within the NGCHM module system.
Applied to files:
NGCHM/WebContent/javascript/HeatMapClass.js
🔇 Additional comments (2)
NGCHM/WebContent/javascript/HeatMapClass.js (1)
561-561: Per-heatmap decor property addition looks good.Defaulting to an object keeps downstream writes (e.g., decor.hue/avatar) safe.
NGCHM/WebContent/javascript/HeatMapCommands.js (1)
111-113: LGTM on strict identity for current-heatmap marker.Clearer and safer in a multi-map context.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
NGCHM/WebContent/javascript/HeatMapClass.js (2)
1358-1365: Bug: misspelled property returns undefined for thumbnail fallbackElse-branch uses col_summaryRatio (snake+camel), but HeatMapLevel defines colSummaryRatio.
- return this.datalevels[MAPREP.THUMBNAIL_LEVEL].col_summaryRatio; + return this.datalevels[MAPREP.THUMBNAIL_LEVEL].colSummaryRatio;
841-843: Strict mode ReferenceError risk: loop variable ‘key’ is undeclared"use strict" is enabled; bare key in for-in creates a ReferenceError when this path runs.
- for (key in this.mapConfig.row_configuration.classifications) { + for (const key in this.mapConfig.row_configuration.classifications) { - for (key in this.mapConfig.col_configuration.classifications) { + for (const key in this.mapConfig.col_configuration.classifications) {Also applies to: 872-874
🧹 Nitpick comments (3)
NGCHM/WebContent/javascript/HeatMapClass.js (3)
561-561: Initialize decor with an explicit shapeDownstream code likely reads hue/avatar; initialize with explicit keys to avoid undefined lookups.
- this.decor = {}; // UI decorations for distinguishing one heatMap from another. + this.decor = { hue: null, avatar: null }; // UI decorations for distinguishing one heatMap from another.
75-76: Array construction bug: creating a 1-element array with an object argumentnew Array({ length: N }) yields [ { length: N } ], not an N-length array. Use numeric length.
- this.tiles = new Array({ length: this.totalTiles }); + this.tiles = new Array(this.totalTiles);
1345-1348: Duplicate method: getTotalRows defined twiceTwo identical definitions can confuse readers and tooling; keep a single definition.
- //Return the total number of detail rows - HeatMap.prototype.getTotalRows = function () { - return this.datalevels[MAPREP.DETAIL_LEVEL].totalRows; - };Also applies to: 1367-1370
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.coderabbit.yaml(1 hunks)NGCHM/WebContent/javascript/HeatMapClass.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .coderabbit.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-09T15:25:55.309Z
Learnt from: bmbroom
PR: bmbroom/NG-CHM#1
File: NGCHM/WebContent/javascript/SearchState.js:58-60
Timestamp: 2025-08-09T15:25:55.309Z
Learning: In the NG-CHM repository, the functions `getDiscCovState` and `setDiscCovState` in `NGCHM/WebContent/javascript/SearchState.js` do not have external callers and can be safely renamed without backward compatibility shims. These are internal functions within the NGCHM module system.
Applied to files:
NGCHM/WebContent/javascript/HeatMapClass.js
🔇 Additional comments (1)
NGCHM/WebContent/javascript/HeatMapClass.js (1)
563-563: mapUpdatedOnLoad rename verified
- No occurrences of the old
updatedOnLoadidentifier were found across the codebase.- All uses of
mapUpdatedOnLoadare intentional and correctly named:
- HeatMapClass.js (lines 563, 1762, 1770) sets the flag on the heat map instance.
- UI-Manager.js (lines 702, 704) checks the flag both via the
MMGR.mapUpdatedOnLoad()helper and directly on theheatMapobject.- MatrixManager.js (lines 871, 873) implements and uses
MMGR.mapUpdatedOnLoadas a function wrapper around the heat map’s flag.No stale references remain.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
NGCHM/WebContent/javascript/HeatMapClass.js (1)
1344-1351: Undefined-level guard is incorrect; use truthy check.this.datalevels[MAPREP.SUMMARY_LEVEL] may be undefined (not null), leading to TypeError. Use a truthy guard.
Apply:
- if (this.datalevels[MAPREP.SUMMARY_LEVEL] !== null) { + if (this.datalevels[MAPREP.SUMMARY_LEVEL]) { return this.datalevels[MAPREP.SUMMARY_LEVEL].rowSummaryRatio; } else { return this.datalevels[MAPREP.THUMBNAIL_LEVEL].rowSummaryRatio; }
🧹 Nitpick comments (2)
NGCHM/WebContent/javascript/HeatMapClass.js (2)
836-844: Avoid for..in over objects; prefer Object.keys to prevent inherited props.Safer and a touch cleaner.
Apply:
- if (typeof rowClassBarsOrder === "undefined") { - rowClassBarsOrder = []; - for (const key in this.mapConfig.row_configuration.classifications) { - rowClassBarsOrder.push(key); - } - } + if (typeof rowClassBarsOrder === "undefined") { + rowClassBarsOrder = Object.keys(this.mapConfig.row_configuration.classifications); + } ... - const filterRowClassBarsOrder = []; - for (let i = 0; i < rowClassBarsOrder.length; i++) { - const newKey = rowClassBarsOrder[i]; - const currConfig = this.mapConfig.row_configuration.classifications[newKey]; - if (currConfig.show == "Y") { - filterRowClassBarsOrder.push(newKey); - } - } - return filterRowClassBarsOrder; + return rowClassBarsOrder.filter( + (key) => this.mapConfig.row_configuration.classifications[key].show == "Y" + );Also applies to: 849-857
868-876: Mirror the key-enumeration/refactor for column classifications.Same rationale as rows.
Apply:
- if (typeof colClassBarsOrder === "undefined") { - colClassBarsOrder = []; - for (const key in this.mapConfig.col_configuration.classifications) { - colClassBarsOrder.push(key); - } - } + if (typeof colClassBarsOrder === "undefined") { + colClassBarsOrder = Object.keys(this.mapConfig.col_configuration.classifications); + } ... - const filterColClassBarsOrder = []; - for (let i = 0; i < colClassBarsOrder.length; i++) { - const newKey = colClassBarsOrder[i]; - const currConfig = this.mapConfig.col_configuration.classifications[newKey]; - if (currConfig.show == "Y") { - filterColClassBarsOrder.push(newKey); - } - } - return filterColClassBarsOrder; + return colClassBarsOrder.filter( + (key) => this.mapConfig.col_configuration.classifications[key].show == "Y" + );Also applies to: 880-889
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
NGCHM/WebContent/javascript/HeatMapClass.js(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-09T15:25:55.309Z
Learnt from: bmbroom
PR: bmbroom/NG-CHM#1
File: NGCHM/WebContent/javascript/SearchState.js:58-60
Timestamp: 2025-08-09T15:25:55.309Z
Learning: In the NG-CHM repository, the functions `getDiscCovState` and `setDiscCovState` in `NGCHM/WebContent/javascript/SearchState.js` do not have external callers and can be safely renamed without backward compatibility shims. These are internal functions within the NGCHM module system.
Applied to files:
NGCHM/WebContent/javascript/HeatMapClass.js
🔇 Additional comments (3)
NGCHM/WebContent/javascript/HeatMapClass.js (3)
75-76: Correct tile array allocation.Using new Array(this.totalTiles) fixes incorrect allocation and downstream indexing.
561-564: Per-heatmap decor + renamed flag initialized.Good addition and consistent rename to mapUpdatedOnLoad.
1362-1402: Getter consistency for DETAIL and per-level ratios looks solid.APIs are coherent and align with HeatMapLevel fields.
Implemented suggestions from CodeRabbit: - Fixed check for undefined summary level - Used Object.keys instead of for..in iteration over objects Also, consolidated the near identical row and column functions for getting covariate orders into a single function with an axis parameter to eliminate duplicated code. Similarly, consolidated the functions for setting the initial covariate orders if not defined.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
NGCHM/WebContent/javascript/CovariateCommands.js (2)
237-258: Leftover call to deprecated setAxisCovariateOrder will break after refactorAfter computing the reordered list, this still calls setAxisCovariateOrder. Update to the new setter.
Apply:
- req.heatMap.setAxisCovariateOrder(req.axis, order); + req.heatMap.setCovariateOrder(req.axis, order);
1-626: StalesetAxisCovariateOrderusage still remains
Our sweep confirms that allgetAxisCovariateOrdercalls have been removed and no accidental"col"literals remain, but the old setter is still invoked in the commands layer:• In
NGCHM/WebContent/javascript/CovariateCommands.jsaround line 256- req.heatMap.setAxisCovariateOrder(req.axis, order); + req.heatMap.setCovariateOrder(req.axis, order);– Update this to use the new
setCovariateOrder(axis, order)API.Please also verify that in
HeatMapClass.jsthesetCovariateOrderprototype method signature has been updated to accept both(axis, order)so that the commands continue to work correctly.
🧹 Nitpick comments (4)
NGCHM/WebContent/javascript/PdfGenerator.js (1)
2116-2131: Use RGB for jsPDF setDrawColor to ensure compatibilityjsPDF setDrawColor typically expects numeric RGB. Convert selection_color hex to RGB before setting draw color.
Apply:
- const color = heatMap.getCurrentDataLayer().selection_color; - pdfDoc.doc.setDrawColor(color); + const sel = CMM.hexToRgba(heatMap.getCurrentDataLayer().selection_color); + pdfDoc.doc.setDrawColor(sel.r, sel.g, sel.b); const yScale = sumMapH / heatMap.getNumRows(MAPREP.DETAIL_LEVEL); const xScale = sumMapW / heatMap.getNumColumns(MAPREP.DETAIL_LEVEL); DVW.detailMaps.forEach((mapItem) => { - if (mapItem.heatMap === heatMap && mapItem.isVisible()) { + if (mapItem.heatMap === heatMap && mapItem.isVisible()) {NGCHM/WebContent/javascript/HeatMapClass.js (3)
830-848: getCovariateOrder: avoid exposing internal array and tighten filter paramReturning the backing classifications_order risks accidental mutation. Also, treat the second arg explicitly as a boolean or a known token.
Apply:
- HeatMap.prototype.getCovariateOrder = function (axis, showOnly) { - const axisConfig = this.getAxisConfig(axis); - let barOrder = axisConfig.classifications_order; - // If barOrder not defined, create order from classifications config. - if (typeof barOrder === "undefined") { - barOrder = Object.keys(axisConfig.classifications); - } - // Filter order for ONLY shown bars (if requested) - if (typeof showOnly === "undefined") { - return barOrder; - } else { - return barOrder.filter( - (key) => axisConfig.classifications[key].show == "Y" - ); - } - }; + HeatMap.prototype.getCovariateOrder = function (axis, showOnly) { + const axisConfig = this.getAxisConfig(axis); + const baseOrder = Array.isArray(axisConfig.classifications_order) + ? axisConfig.classifications_order.slice() // return a copy to prevent accidental mutation + : Object.keys(axisConfig.classifications); + const filterShown = showOnly === true || showOnly === "show"; + return filterShown + ? baseOrder.filter((key) => axisConfig.classifications[key].show === "Y") + : baseOrder; + };
850-859: setCovariateOrder only ensures a default order existsBehavior is fine; consider renaming to ensureCovariateOrder for clarity in a future cleanup.
1325-1365: Totals and accessors: API surface is a bit redundantgetTotalRows/getTotalCols overlap with getTotalElementsForAxis/getNumRows/getNumColumns. Not a blocker; consider consolidating to one set to reduce maintenance.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
NGCHM/WebContent/javascript/CovariateCommands.js(4 hunks)NGCHM/WebContent/javascript/DetailHeatMapEvent.js(2 hunks)NGCHM/WebContent/javascript/HeatMapClass.js(5 hunks)NGCHM/WebContent/javascript/Linkout.js(3 hunks)NGCHM/WebContent/javascript/PdfGenerator.js(7 hunks)NGCHM/WebContent/javascript/SearchManager.js(2 hunks)NGCHM/WebContent/javascript/SummaryHeatMapDisplay.js(5 hunks)NGCHM/WebContent/javascript/UI-Manager.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- NGCHM/WebContent/javascript/SummaryHeatMapDisplay.js
- NGCHM/WebContent/javascript/Linkout.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-09T15:25:55.309Z
Learnt from: bmbroom
PR: bmbroom/NG-CHM#1
File: NGCHM/WebContent/javascript/SearchState.js:58-60
Timestamp: 2025-08-09T15:25:55.309Z
Learning: In the NG-CHM repository, the functions `getDiscCovState` and `setDiscCovState` in `NGCHM/WebContent/javascript/SearchState.js` do not have external callers and can be safely renamed without backward compatibility shims. These are internal functions within the NGCHM module system.
Applied to files:
NGCHM/WebContent/javascript/UI-Manager.jsNGCHM/WebContent/javascript/CovariateCommands.jsNGCHM/WebContent/javascript/SearchManager.jsNGCHM/WebContent/javascript/HeatMapClass.js
🧬 Code graph analysis (1)
NGCHM/WebContent/javascript/HeatMapClass.js (1)
NGCHM/WebContent/javascript/UserPreferenceManager.js (8)
covariates(1404-1407)covariates(1583-1586)axis(1464-1464)axisConfig(2325-2325)axisConfig(2400-2400)axisConfig(2561-2561)axisConfig(2606-2606)axisConfig(2635-2635)
🔇 Additional comments (17)
NGCHM/WebContent/javascript/UI-Manager.js (1)
87-89: Switched to new covariate-order API — looks goodCalls to setCovariateOrder("row"/"column") replace the legacy row/col-specific setters appropriately.
NGCHM/WebContent/javascript/CovariateCommands.js (3)
36-41: Use of getCovariateOrder in list route — OKEnumeration via the new API per axis is correct.
206-215: Create route duplicates check with getCovariateOrder — OKPrevents duplicate names using the standardized order API.
274-281: get-list route switched to getCovariateOrder — OKReturns a copy as before; no behavior change.
NGCHM/WebContent/javascript/SearchManager.js (2)
538-542: Tightened comparison to strict identity — OKUsing
===avoids accidental truthiness between different heatMap objects.
565-573: No action needed: ‘col’ is a valid axis for covariate APIsI verified that HeatMap.prototype.getAxisConfig uses MAPREP.isRow(axis) to dispatch to row vs. column, so any axis string other than “row” (including both “col” and “column”) correctly maps to the column branch. In particular:
- getCovariateOrder("col") and getCovariateType("col", key) both call getAxisConfig("col") → MAPREP.isRow("col")? false → getColConfig() ()
- All existing code paths (SummaryHeatMapDisplay.js, PdfGenerator.js, HeatMapClass.js, Linkout.js) intermix “col” and “column” without issue, and no runtime errors occur for “col” inputs.
Therefore, the suggestion to replace addCovariateOptions("col") with addCovariateOptions("column") and to normalize the axis inside addCovariateOptions is unnecessary. If you prefer consistent naming, you may optionally refactor to a single representation, but there is no functional problem to fix.
Likely an incorrect or invalid review comment.
NGCHM/WebContent/javascript/DetailHeatMapEvent.js (1)
266-276: Updated covariate-order retrieval — OKUsing getCovariateOrder("row"/"column") and filtering by show === "Y" preserves prior behavior while aligning with the new API.
NGCHM/WebContent/javascript/PdfGenerator.js (6)
51-52: Scope visibility to active heat map — good changeUsing DVW.anyVisible(MMGR.getHeatMap()) aligns PDF enablement with the selected heat map.
67-68: Clarified error messageMessage now correctly references “current heat map.” Looks good.
77-87: Per-heat-map radio enablement/selection logic is solidDisables/auto-selects options based on current panel visibility, then updates bounds. Nice UX improvement.
Also applies to: 94-94
261-266: Filter detail jobs to current heat mapRestricting detail jobs by mapItem.heatMap === heatMap and visibility prevents cross-map bleed. Good.
739-753: Switch to getCovariateOrder() for column legendsUsing heatMap.getCovariateOrder("column", "show") matches the new API and respects visibility.
1821-1833: Switch to getCovariateOrder() in last-bar checkConsistent with the new ordering API.
NGCHM/WebContent/javascript/HeatMapClass.js (4)
75-76: Fix TileWindow array sizing bugReplacing new Array({ length: N }) with new Array(N) is correct. Prevents a sparse array with a single object.
562-564: Add per-heat-map decor and update flagDecor and mapUpdatedOnLoad fields are reasonable and low-risk additions.
801-806: genAllCovars now uses getCovariateOrder()Good consolidation around a single ordering API.
1307-1323: Row/column summary ratio guard fixedTruthy checks for SUMMARY_LEVEL prevent undefined access; correct fallback to THUMBNAIL.
setAxisCovariateOrder was a separate function from setCovariateOrder (and the setRowClassificationOrder and setColClassificationOrder functions it replaced) and so there was no actual brokage. But, the two functions are very similar in both name and function and undoubtedly this would have led to confusion and errors in the future. I consolidated both functions into a single function that takes an optional order parameter. The jsPDF setDrawColor was working in my tests, but I followed CodeRabbit's suggestion to ensure compatibility. I made the suggested changes to improve the robustness of getCovariateOrder. I left fixing the redundancy in the totals and accessors API surface for another day. This is a rabbit hole I don't want to go down just now.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
NGCHM/WebContent/javascript/CovariateCommands.js(5 hunks)NGCHM/WebContent/javascript/HeatMapClass.js(5 hunks)NGCHM/WebContent/javascript/PdfGenerator.js(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- NGCHM/WebContent/javascript/CovariateCommands.js
- NGCHM/WebContent/javascript/PdfGenerator.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-09T15:25:55.309Z
Learnt from: bmbroom
PR: bmbroom/NG-CHM#1
File: NGCHM/WebContent/javascript/SearchState.js:58-60
Timestamp: 2025-08-09T15:25:55.309Z
Learning: In the NG-CHM repository, the functions `getDiscCovState` and `setDiscCovState` in `NGCHM/WebContent/javascript/SearchState.js` do not have external callers and can be safely renamed without backward compatibility shims. These are internal functions within the NGCHM module system.
Applied to files:
NGCHM/WebContent/javascript/HeatMapClass.js
🧬 Code graph analysis (1)
NGCHM/WebContent/javascript/HeatMapClass.js (2)
NGCHM/WebContent/javascript/UserPreferenceManager.js (8)
covariates(1404-1407)covariates(1583-1586)axis(1464-1464)axisConfig(2325-2325)axisConfig(2400-2400)axisConfig(2561-2561)axisConfig(2606-2606)axisConfig(2635-2635)NGCHM/WebContent/javascript/CovariateCommands.js (4)
order(36-36)order(208-208)order(237-237)order(278-278)
🔇 Additional comments (7)
NGCHM/WebContent/javascript/HeatMapClass.js (7)
75-75: Correct fix: tile array is now sized properly.Using
new Array(this.totalTiles)avoids the sparse array-of-object bug fromnew Array({ length: ... }).
561-564: New per-heatmap decor and update flag look good.
decorandmapUpdatedOnLoadinitialization is clear and aligns with the multi-map UI objectives.
799-806: Good:genAllCovarsnow centralizes order viagetCovariateOrder.This reduces duplication and ensures consistent ordering logic across call sites.
1303-1311: Truthy guard for summary row ratio is correct.Covers both null and undefined and matches how levels are aliased. Good fallback to thumbnail.
1312-1319: Column ratio getter aligned with data model.Switch to
colSummaryRatiomatchesHeatMapLevelfields; same truthy guard improvement. Good.
1331-1361: Utility getters (getTotalElementsForAxis,getNumAxisElements, ratios) are coherent.Consistent axis handling via
MAPREP.isRowand level-indexing; no correctness issues spotted.
1321-1330: Confirmed public API usage of getTotalRows/getTotalColsBoth methods are invoked outside of
HeatMapClass.js, indicating they’re part of the public surface:
- In UserPreferenceManager.js (around lines 2237–2238):
UPM.heatMap.getTotalRows()andUPM.heatMap.getTotalCols()- In DetailHeatMapDisplay.js (around lines 612–613 and 779–792):
mapItem.heatMap.getTotalRows()andmapItem.heatMap.getTotalCols()Since these are actively used elsewhere, they must remain public unless you plan a formal deprecation. If you intend to replace them with
getTotalElementsForAxis(axis), add deprecation wrappers in these methods, update all call sites, and mark them for removal in a future release. Otherwise, it’s safe to leave them as-is.
| // Return the order in which the covariates on the specified axis should be | ||
| // displayed. | ||
| // If showOnly is specified, return only the visible covariates. | ||
| HeatMap.prototype.getCovariateOrder = function (axis, showOnly) { | ||
| const axisConfig = this.getAxisConfig(axis); | ||
| // If barOrder not defined, create order from classifications config. | ||
| const barOrder = Array.isArray(axisConfig.classifications_order) | ||
| ? axisConfig.classifications_order.slice() | ||
| : Object.keys(axisConfig.classifications); | ||
| // Filter order for only shown bars (if requested) | ||
| return showOnly === true || showOnly === "show" | ||
| ? barOrder.filter((key) => axisConfig.classifications[key].show == "Y") | ||
| : barOrder; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden getCovariateOrder: filter stale keys and optionally append new ones.
If classifications_order contains unknown keys (stale config), downstream uses that index into axisConfig.classifications[key] can throw. Sanitize here to prevent runtime errors.
Apply:
-HeatMap.prototype.getCovariateOrder = function (axis, showOnly) {
- const axisConfig = this.getAxisConfig(axis);
- // If barOrder not defined, create order from classifications config.
- const barOrder = Array.isArray(axisConfig.classifications_order)
- ? axisConfig.classifications_order.slice()
- : Object.keys(axisConfig.classifications);
- // Filter order for only shown bars (if requested)
- return showOnly === true || showOnly === "show"
- ? barOrder.filter((key) => axisConfig.classifications[key].show == "Y")
- : barOrder;
-};
+HeatMap.prototype.getCovariateOrder = function (axis, showOnly) {
+ const axisConfig = this.getAxisConfig(axis);
+ const allKeys = Object.keys(axisConfig.classifications);
+ const baseOrder = Array.isArray(axisConfig.classifications_order)
+ ? axisConfig.classifications_order.slice()
+ : allKeys;
+ // Drop unknown/stale keys to avoid undefined lookups.
+ const validOrder = baseOrder.filter((k) => axisConfig.classifications.hasOwnProperty(k));
+ // Ensure newly added covariates are included (append to end, stable behavior).
+ for (const k of allKeys) if (!validOrder.includes(k)) validOrder.push(k);
+ if (showOnly === true || showOnly === "show") {
+ return validOrder.filter((k) => axisConfig.classifications[k] && axisConfig.classifications[k].show == "Y");
+ }
+ return validOrder;
+};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Return the order in which the covariates on the specified axis should be | |
| // displayed. | |
| // If showOnly is specified, return only the visible covariates. | |
| HeatMap.prototype.getCovariateOrder = function (axis, showOnly) { | |
| const axisConfig = this.getAxisConfig(axis); | |
| // If barOrder not defined, create order from classifications config. | |
| const barOrder = Array.isArray(axisConfig.classifications_order) | |
| ? axisConfig.classifications_order.slice() | |
| : Object.keys(axisConfig.classifications); | |
| // Filter order for only shown bars (if requested) | |
| return showOnly === true || showOnly === "show" | |
| ? barOrder.filter((key) => axisConfig.classifications[key].show == "Y") | |
| : barOrder; | |
| }; | |
| // Return the order in which the covariates on the specified axis should be | |
| // displayed. | |
| // If showOnly is specified, return only the visible covariates. | |
| HeatMap.prototype.getCovariateOrder = function (axis, showOnly) { | |
| const axisConfig = this.getAxisConfig(axis); | |
| const allKeys = Object.keys(axisConfig.classifications); | |
| const baseOrder = Array.isArray(axisConfig.classifications_order) | |
| ? axisConfig.classifications_order.slice() | |
| : allKeys; | |
| // Drop unknown/stale keys to avoid undefined lookups. | |
| const validOrder = baseOrder.filter((k) => | |
| axisConfig.classifications.hasOwnProperty(k) | |
| ); | |
| // Ensure newly added covariates are included (append to end, stable behavior). | |
| for (const k of allKeys) { | |
| if (!validOrder.includes(k)) validOrder.push(k); | |
| } | |
| if (showOnly === true || showOnly === "show") { | |
| return validOrder.filter( | |
| (k) => | |
| axisConfig.classifications[k] && | |
| axisConfig.classifications[k].show == "Y" | |
| ); | |
| } | |
| return validOrder; | |
| }; |
🤖 Prompt for AI Agents
In NGCHM/WebContent/javascript/HeatMapClass.js around lines 830 to 843, the
getCovariateOrder function must sanitize classifications_order to avoid stale
keys and ensure new classification keys are included; update the logic to: build
initial order from classifications_order if array else from
Object.keys(classifications), then filter that order to only keep keys that
exist in axisConfig.classifications, then compute any missing keys by taking
Object.keys(axisConfig.classifications) minus the filtered order and append
those missing keys to the end, and finally apply the showOnly filter (if
showOnly === true || showOnly === "show") by keeping only keys whose
axisConfig.classifications[key].show === "Y".
| // Set the order in which the covariates on the specified axis should be | ||
| // displayed. | ||
| // If order is not specified and there is no existing order, set a | ||
| // default order. | ||
| HeatMap.prototype.setCovariateOrder = function (axis, order) { | ||
| if (this.mapConfig !== null) { | ||
| this.mapConfig.row_configuration.classifications_order = this.getRowClassificationOrder(); | ||
| } | ||
| }; | ||
|
|
||
| HeatMap.prototype.getColClassificationOrder = function (showOnly) { | ||
| var colClassBarsOrder = this.mapConfig.col_configuration.classifications_order; | ||
| // If configuration not found, create order from classifications config | ||
| if (typeof colClassBarsOrder === "undefined") { | ||
| colClassBarsOrder = []; | ||
| for (key in this.mapConfig.col_configuration.classifications) { | ||
| colClassBarsOrder.push(key); | ||
| } | ||
| } | ||
| // Filter order for ONLY shown bars (if requested) | ||
| if (typeof showOnly === "undefined") { | ||
| return colClassBarsOrder; | ||
| } else { | ||
| const filterColClassBarsOrder = []; | ||
| for (var i = 0; i < colClassBarsOrder.length; i++) { | ||
| var newKey = colClassBarsOrder[i]; | ||
| var currConfig = this.mapConfig.col_configuration.classifications[newKey]; | ||
| if (currConfig.show == "Y") { | ||
| filterColClassBarsOrder.push(newKey); | ||
| const axisConfig = this.getAxisConfig(axis); | ||
| if (order) { | ||
| if (!Array.isArray(order)) { | ||
| console.error("setCovariateOrder: if specified, order must be an array", { axis, order }); | ||
| return; | ||
| } | ||
| axisConfig.classifications_order = order.slice(); | ||
| this.setUnAppliedChanges(true); | ||
| return; | ||
| } | ||
| if (typeof axisConfig.classifications_order === "undefined") { | ||
| axisConfig.classifications_order = Object.keys(axisConfig.classifications); | ||
| } | ||
| return filterColClassBarsOrder; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate inputs in setCovariateOrder to prevent crashes and maintain invariants.
A bad order (unknown keys, duplicates, missing entries) can break callers that assume every key maps to a config. Sanitize and dedupe.
Apply:
HeatMap.prototype.setCovariateOrder = function (axis, order) {
if (this.mapConfig !== null) {
const axisConfig = this.getAxisConfig(axis);
if (order) {
if (!Array.isArray(order)) {
console.error("setCovariateOrder: if specified, order must be an array", { axis, order });
return;
}
- axisConfig.classifications_order = order.slice();
+ const allKeys = Object.keys(axisConfig.classifications);
+ // Deduplicate and drop unknown keys.
+ const sanitized = [...new Set(order)].filter((k) => allKeys.includes(k));
+ // Append any missing keys to preserve full coverage.
+ for (const k of allKeys) if (!sanitized.includes(k)) sanitized.push(k);
+ axisConfig.classifications_order = sanitized;
this.setUnAppliedChanges(true);
return;
}
if (typeof axisConfig.classifications_order === "undefined") {
axisConfig.classifications_order = Object.keys(axisConfig.classifications);
}
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Set the order in which the covariates on the specified axis should be | |
| // displayed. | |
| // If order is not specified and there is no existing order, set a | |
| // default order. | |
| HeatMap.prototype.setCovariateOrder = function (axis, order) { | |
| if (this.mapConfig !== null) { | |
| this.mapConfig.row_configuration.classifications_order = this.getRowClassificationOrder(); | |
| } | |
| }; | |
| HeatMap.prototype.getColClassificationOrder = function (showOnly) { | |
| var colClassBarsOrder = this.mapConfig.col_configuration.classifications_order; | |
| // If configuration not found, create order from classifications config | |
| if (typeof colClassBarsOrder === "undefined") { | |
| colClassBarsOrder = []; | |
| for (key in this.mapConfig.col_configuration.classifications) { | |
| colClassBarsOrder.push(key); | |
| } | |
| } | |
| // Filter order for ONLY shown bars (if requested) | |
| if (typeof showOnly === "undefined") { | |
| return colClassBarsOrder; | |
| } else { | |
| const filterColClassBarsOrder = []; | |
| for (var i = 0; i < colClassBarsOrder.length; i++) { | |
| var newKey = colClassBarsOrder[i]; | |
| var currConfig = this.mapConfig.col_configuration.classifications[newKey]; | |
| if (currConfig.show == "Y") { | |
| filterColClassBarsOrder.push(newKey); | |
| const axisConfig = this.getAxisConfig(axis); | |
| if (order) { | |
| if (!Array.isArray(order)) { | |
| console.error("setCovariateOrder: if specified, order must be an array", { axis, order }); | |
| return; | |
| } | |
| axisConfig.classifications_order = order.slice(); | |
| this.setUnAppliedChanges(true); | |
| return; | |
| } | |
| if (typeof axisConfig.classifications_order === "undefined") { | |
| axisConfig.classifications_order = Object.keys(axisConfig.classifications); | |
| } | |
| return filterColClassBarsOrder; | |
| } | |
| }; | |
| // Set the order in which the covariates on the specified axis should be | |
| // displayed. | |
| // If order is not specified and there is no existing order, set a | |
| // default order. | |
| HeatMap.prototype.setCovariateOrder = function (axis, order) { | |
| if (this.mapConfig !== null) { | |
| const axisConfig = this.getAxisConfig(axis); | |
| if (order) { | |
| if (!Array.isArray(order)) { | |
| console.error("setCovariateOrder: if specified, order must be an array", { axis, order }); | |
| return; | |
| } | |
| // Sanitize the provided order: dedupe, drop unknown keys, then append any missing keys | |
| const allKeys = Object.keys(axisConfig.classifications); | |
| const sanitized = [...new Set(order)].filter((k) => allKeys.includes(k)); | |
| for (const k of allKeys) { | |
| if (!sanitized.includes(k)) { | |
| sanitized.push(k); | |
| } | |
| } | |
| axisConfig.classifications_order = sanitized; | |
| this.setUnAppliedChanges(true); | |
| return; | |
| } | |
| if (typeof axisConfig.classifications_order === "undefined") { | |
| axisConfig.classifications_order = Object.keys(axisConfig.classifications); | |
| } | |
| } | |
| }; |
🤖 Prompt for AI Agents
In NGCHM/WebContent/javascript/HeatMapClass.js around lines 845 to 865,
setCovariateOrder currently accepts any array and can introduce unknown keys,
duplicates or omissions which break callers; update the function to validate and
sanitize the supplied order: ensure order is an array, filter it to only known
classification keys (log an error for unknown keys), remove duplicates while
preserving first-seen order, append any missing classification keys (preserving
existing axisConfig.classifications order) so every key is present exactly once,
then assign the sanitized array to axisConfig.classifications_order and call
this.setUnAppliedChanges(true); keep the early return for non-array inputs and
preserve existing behavior when order is undefined.
- Added default CodeRabbit review to drafr pull request on specified branches - Fixed error in isMatrixLinkout
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
NGCHM/WebContent/javascript/Linkout.js (1)
1575-1583: Clear pane decor when switching to plugins (consider doing the same for Linkouts).Calling PANE.setPaneDecor(loc, null) here prevents stale decor from previous content. For consistency, also clear decor when switching to the Linkouts panel so it doesn’t inherit plugin-specific styling.
Apply outside this hunk in LNK.switchPaneToLinkouts:
LNK.switchPaneToLinkouts = function switchPaneToLinkouts(loc) { PANE.clearExistingDialogs(loc.pane.id); const oldLinkoutPane = LNK.linkoutElement && PANE.findPaneLocation(LNK.linkoutElement); if ( oldLinkoutPane && oldLinkoutPane.paneTitle && oldLinkoutPane.paneTitle.innerText === "Linkouts" ) { PANE.setPaneTitle(oldLinkoutPane, "Empty"); } PANE.emptyPaneLocation(loc); + PANE.setPaneDecor(loc, null); LNK.linkoutElement = UTIL.newElement("DIV.linkouts"); PANE.setPaneTitle(loc, "Linkouts"); loc.pane.appendChild(LNK.linkoutElement); MMGR.getHeatMap().saveLinkoutPaneToMapConfig(loc.pane.id, "", "Linkouts"); };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.coderabbit.yaml(1 hunks)NGCHM/WebContent/javascript/Linkout.js(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .coderabbit.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
NGCHM/WebContent/javascript/Linkout.js (3)
NGCHM/WebContent/javascript/DetailHeatMapEvent.js (5)
axis(305-305)axis(1197-1197)axis(1284-1284)axis(1322-1322)PANE(20-20)NGCHM/WebContent/javascript/HeatMapClass.js (6)
labels(1163-1163)labels(1274-1274)labels(1279-1279)labels(2009-2009)labels(2026-2026)labels(2063-2063)NGCHM/WebContent/javascript/SearchState.js (2)
labels(98-110)labels(232-232)
🔇 Additional comments (3)
NGCHM/WebContent/javascript/Linkout.js (3)
411-414: Title-based matrix linkout check is correct.Switching to title equality matches MatrixLinkout.title and avoids map/allocs. Looks good.
444-451: Correct: covariate order retrieval and 0-based indexing.Using heatMap.getCovariateOrder(...) for Covar axes and indexing labels[idx] (not idx-1) aligns with SRCHSTATE’s 0-based covariate indices.
2520-2540: No action needed: axis casing is already handled correctlygetCovariateOrder delegates to getAxisConfig, which uses
MAPREP.isRow(axis) // returns true iff axis.toLowerCase() === "row" ? this.getRowConfig() : this.getColConfig();(HeatMapClass.js lines 833–843, 630–634)
Because isRow is case-insensitive, passing"row"/"column"(or any casing) works as intended—no normalization is required.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
NGCHM/WebContent/javascript/Linkout.js (1)
1578-1581: Good: clearing pane decor before switching plugin.Resetting decor avoids stale visuals when switching plugins on different maps. Consider ensuring Linkouts pane switches also clear decor if not already handled elsewhere.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
NGCHM/WebContent/javascript/Linkout.js(5 hunks)
🔇 Additional comments (4)
NGCHM/WebContent/javascript/Linkout.js (4)
4322-4330: Robust de-duplication for special coordinates.Set-keyed de-dup across combined arrays is correct and future-proof.
411-414: Verify menuEntry is the correct title for matrix linkoutsWe found one external call to LNK.isMatrixLinkout that now passes linkout.menuEntry:
- UI-Manager.js line 1629:
if (LNK.isMatrixLinkout(linkout.menuEntry)) { … }Please confirm that each plugin.linkouts entry’s menuEntry property really corresponds to the title field of your matrixLinkouts. If menuEntry differs (e.g., uses a label or key instead of the title), update the call site or linkout definitions accordingly.
449-451: Normalize covariate axis when calling getCovariateOrder
axis.replace("Covar","")yields"Row"/"Column", whereas all other calls to getCovariateOrder pass lowercase"row"/"column". To ensure consistency and avoid a silent failure, update the snippet in Linkout.js as follows:- const labels = heatMap.getCovariateOrder(axis.replace("Covar","")); + const labels = heatMap.getCovariateOrder(axis.replace("Covar","").toLowerCase());This guarantees that getCovariateOrder always receives the lowercase axis it expects.
2521-2540: setAxis covariate ordering & defaultCoord logic confirmedBoth concerns have been verified against the current implementation:
getCovariateOrder axis casing
• In Linkout.js setAxis (around line 2524), theaxisstring is passed directly toheatMap.getCovariateOrder(axis).
• In HeatMapClass.js (around lines 833–836),getCovariateOrdercallsthis.getAxisConfig(axis), which usesMAPREP.isRow(axis)(implemented viaaxis.toLowerCase() === "row") to choose row vs. column config.
→ Axis matching is case-insensitive, so both"row"and"Row"(or any casing) are accepted.defaultCoord detection
• In Linkout.js setAxis (around lines 2530–2536),defaultCoordis computed by filtering the array returned fromgetCovariateOrder(axis)for/\.coordinate\.1/, then stripping the trailing “1”.
→ This logic still correctly picks the first “.coordinate.1” entry and yields the base coordinate prefix.No changes are required.
1 participant
Some enhancements to the multi-map implementation:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores