Skip to content

virtual-servers-select-all-count#3849

Merged
crivetimihai merged 3 commits intomainfrom
fix/virtual-servers-select-all-count
Mar 26, 2026
Merged

virtual-servers-select-all-count#3849
crivetimihai merged 3 commits intomainfrom
fix/virtual-servers-select-all-count

Conversation

@Nayana-R-Gowda
Copy link
Copy Markdown
Collaborator

@Nayana-R-Gowda Nayana-R-Gowda commented Mar 25, 2026

Signed-off-by: NAYANA.R nayana.r7813@gmail.com

closes #3833

💡 Fix Description

It re-fetches the button using document.getElementById to avoid stale references.
If the button exists:
When count > 0 → updates text to Select All (count)
When count === 0 → resets text to Select All

🧪 Verification

Check Command Status
Lint suite make lint Pass
Unit tests make test pass

📐 MCP Compliance (if relevant)

  • Matches current MCP spec
  • No breaking change to MCP clients

✅ Checklist

  • Code formatted (make black isort pre-commit)
  • No secrets/credentials committed

@Nayana-R-Gowda Nayana-R-Gowda added bug Something isn't working release-fix Critical bugfix required for the release labels Mar 25, 2026
@crivetimihai crivetimihai self-assigned this Mar 26, 2026
Nayana-R-Gowda and others added 3 commits March 26, 2026 21:10
Signed-off-by: NAYANA.R <nayana.r7813@gmail.com>
Signed-off-by: NAYANA.R <nayana.r7813@gmail.com>
…Select functions

Extend the Select All button count fix from initToolSelect to
initResourceSelect, initPromptSelect, and initGatewaySelect for
consistency. Remove stale originalText + setTimeout pattern from all
three sibling functions. Update Playwright assertions to match new
button text format and add JS unit tests for count display behavior.

Closes #3833

Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
@crivetimihai crivetimihai force-pushed the fix/virtual-servers-select-all-count branch from 441b3d5 to d015a00 Compare March 26, 2026 21:32
@crivetimihai
Copy link
Copy Markdown
Member

Review & Changes

Rebased onto main and reviewed the PR. The core approach — moving button text into update() as a single source of truth — is correct.

Changes made during review

Consistency fix: The original PR only applied the count display fix to initToolSelect. The identical originalText + setTimeout bug existed in three sibling functions. Applied the same fix to:

  • initResourceSelect
  • initPromptSelect
  • initGatewaySelect

For each: added Select All (N) count display in update(), removed originalText variable, removed the setTimeout success message, and used update() for error recovery.

Tests updated:

  • Playwright: test_select_all_tools_button assertion updated to match new "Select All (N)" format
  • Playwright: test_clear_all_tools_button — removed TODO bug comment (now fixed) and added assertion that button resets to "Select All"
  • JS unit tests: added 5 new tests covering count display, Clear All reset, null selectBtnId safety, individual checkbox change, and error recovery

All 989 JS tests pass. No security or performance concerns.

Copy link
Copy Markdown
Member

@crivetimihai crivetimihai left a comment

Choose a reason for hiding this comment

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

LGTM — fix is correct and now consistent across all four init*Select functions. Tests added.

@crivetimihai crivetimihai merged commit e036569 into main Mar 26, 2026
37 of 38 checks passed
@crivetimihai crivetimihai deleted the fix/virtual-servers-select-all-count branch March 26, 2026 21:54
brian-hussey pushed a commit that referenced this pull request Mar 27, 2026
* virtual-servers-select-all-count

Signed-off-by: NAYANA.R <nayana.r7813@gmail.com>

* remove unused originalText variable in select all handler

Signed-off-by: NAYANA.R <nayana.r7813@gmail.com>

* fix(ui): apply Select All count display consistently across all init*Select functions

Extend the Select All button count fix from initToolSelect to
initResourceSelect, initPromptSelect, and initGatewaySelect for
consistency. Remove stale originalText + setTimeout pattern from all
three sibling functions. Update Playwright assertions to match new
button text format and add JS unit tests for count display behavior.

Closes #3833

Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

---------

Signed-off-by: NAYANA.R <nayana.r7813@gmail.com>
Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
Co-authored-by: Mihai Criveti <crivetimihai@gmail.com>
madhu-mohan-jaishankar pushed a commit that referenced this pull request Mar 27, 2026
* virtual-servers-select-all-count

Signed-off-by: NAYANA.R <nayana.r7813@gmail.com>

* remove unused originalText variable in select all handler

Signed-off-by: NAYANA.R <nayana.r7813@gmail.com>

* fix(ui): apply Select All count display consistently across all init*Select functions

Extend the Select All button count fix from initToolSelect to
initResourceSelect, initPromptSelect, and initGatewaySelect for
consistency. Remove stale originalText + setTimeout pattern from all
three sibling functions. Update Playwright assertions to match new
button text format and add JS unit tests for count display behavior.

Closes #3833

Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

---------

Signed-off-by: NAYANA.R <nayana.r7813@gmail.com>
Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
Co-authored-by: Mihai Criveti <crivetimihai@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working release-fix Critical bugfix required for the release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG][FE]: Virtual Servers - Associated Tools - Select All is not showing the correct count of the tools selected

2 participants