refactor: improve time complexity of select algorithms

[jaskfla]

🦸 Review Hero

• Run Review Hero
• Auto-fix review suggestions
• Auto-fix CI failures

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

[review-hero]

🦸 Review Hero Summary
9 agents reviewed this PR | 0 critical | 0 suggestions | 0 nitpicks | Filtering: consensus 3 voters

No issues found. Looks good!

[review-hero]

🦸 Review Hero Summary
9 agents reviewed this PR | 0 critical | 2 suggestions | 0 nitpicks | Filtering: consensus 3 voters, 2 below threshold

Below consensus threshold (2 unique issues not confirmed by majority)

Location	Agent	Severity	Comment
packages/web-config-server/src/apiV1/dataBuilders/generic/percentage/percentagesOfValueCounts.js:36	Design & Architecture	suggestion	[valuesSet, valuesToCompareSet] is destructured from fraction.dataValues.map(arr => new Set(arr)), but values and valuesToCompare are also still destructured from fraction.dataValues for ...
packages/web-config-server/src/apiV1/dataBuilders/generic/percentage/percentagesOfValueCounts.js:38	Design & Architecture	nitpick	The early-return pattern if (set1Count === 0) return false; … return set1Count === set2Count; is a micro-optimisation that defers computing set2 but adds a branch where none is needed. The orig...

Local fix prompt (copy to your coding agent)

Fix these issues identified on the pull request. One commit per issue fixed.

---

packages/report-server/src/reportBuilder/output/functions/matrix/matrixBuilder.ts:64: The inclusions Set is constructed from (string | MatrixEntityCell)[], but inclusions.has(columnName) will never match a MatrixEntityCell entry because columnName is a string and Set.has uses reference equality for objects. The MatrixEntityCell entries in this Set are dead weight. The entity-label case is already handled by fieldNames. Consider: const inclusions = new Set(includeFields.filter((f): f is string => !isMatrixEntityCell(f))) to make the intent clear and avoid storing objects that can never produce a hit.

---

packages/web-config-server/src/apiV1/dataBuilders/generic/percentage/percentagesOfValueCounts.js:22: This PR's stated goal is time-complexity improvement, but it also adds a new validation guard (!fraction.dataValues.every(Array.isArray)) that changes the function's behaviour for previously-unchecked inputs. Behaviour changes embedded in a refactor are hard to review and reason about; the validation fix should be a separate commit/PR so reviewers can assess it independently.

[jaskfla]

@claude review

[claude]

Clean performance refactor (array.includes → Set.has); prior cursor bug and review-hero suggestions have been addressed in subsequent commits, and semantic equivalence checks out.

Extended reasoning...

Overview

This PR is a focused performance refactor across four files, replacing O(n*m) repeated Array.prototype.includes lookups with O(1) Set.prototype.has lookups in: matrix column filtering, percentage builders, and grouped map overlay lookups. checkValueSatisfiesCondition.js only adds JSDoc @type {const} annotations (no behavior change).

Security risks

None. The PR touches internal data shaping and report-building logic. No auth, crypto, permissions, query construction, or user-facing input handling is modified.

Level of scrutiny

Moderate. This touches data calculation logic in the percentage report builder, which is production code, but the changes are mechanical and the two genuine behavior tweaks (every(Array.isArray) validation guard + set1Count === 0 short-circuit) are both behavior-preserving for valid inputs — the short-circuit returns false exactly as the original set1Count > 0 && set1Count === count2Count would, and the validation simply throws earlier on malformed inputs that would have crashed inside includes anyway.

Other factors

The earlier high-severity cursor bug (Sets passed to countAnalyticsThatSatisfyConditions which calls .includes) was fixed in subsequent commits — current code passes the original values/valuesToCompare arrays to that helper while using the Sets only for the local filter. The review-hero suggestion about dead MatrixEntityCell entries in the inclusions Set was also resolved by the cleaner single-Set design with explicit entityLabel extraction. The author explicitly requested re-review (@claude review) after these revisions. Bug hunting found no new issues this run.

[claude]

Mechanical array→Set lookup refactor for hot loops; type-only changes in checkValueSatisfiesCondition; prior reviewer findings have been addressed.

Extended reasoning...

Overview

Four files touched: matrixBuilder.ts, checkValueSatisfiesCondition.js, percentagesOfValueCounts.js, and findAccessibleGroupedMapOverlays.js. All changes are O(n²) → O(n) lookup optimizations swapping Array.prototype.includes with Set.prototype.has, plus as const / JSDoc @type {const} annotations that are type-only.

Security risks

None. Changes are purely algorithmic — no auth, crypto, input parsing, or external-boundary code is touched.

Level of scrutiny

Low-to-moderate. These are mechanical refactors with equivalent semantics:

• matrixBuilder.ts: the new merged fields Set produces the same exclusion set as the prior entityFieldNames ∪ excludeFields ∪ includeFields chain (MatrixEntityCell objects in the old includeFields.includes(columnName) check never matched a string columnName by reference anyway, so dropping them is safe).
• percentagesOfValueCounts.js: the Sets are only used for the inner .filter(r => valuesSet.has(r.dataElement)) lookup; the original array values/valuesToCompare is still correctly passed to countAnalyticsThatSatisfyConditions, so the prior Cursor "Set passed where includes() expected" bug does not apply to the current code.
• findAccessibleGroupedMapOverlays.js and checkValueSatisfiesCondition.js: trivial.

Other factors

Two incidental behavior changes embedded in the refactor: the new !fraction.dataValues.every(Array.isArray) validation guard (defensive — throws earlier on bad input instead of silently misbehaving) and the set1Count === 0 short-circuit (avoids the second filter+count pass; preserves the original return set1Count > 0 && set1Count === count2Count result). Both are safe. Prior reviewer suggestions about MatrixEntityCell entries in the inclusions Set were addressed in commit 176d177. No outstanding unresolved comments.