Skip to content

Commit 11db289

Browse files
committed
fix: KDMA delete button asymmetric behavior and slider race conditions
- Fix asymmetric KDMA delete button logic to properly validate single-KDMA experiments - Add canRemoveSpecificKDMA function for per-KDMA validation instead of global check - Fix decimal formatting in experiment key construction (0.0 vs 0) - Add race condition guard to prevent concurrent reloadPinnedRun calls - Add comprehensive test for asymmetric delete button behavior - Ensure exactly one delete button enabled when valid single-KDMA experiments exist Resolves issues where: 1. Both delete buttons remained disabled when one should be enabled 2. Rapid slider changes caused 404 errors from concurrent HTTP requests 3. Experiment key lookup failed due to decimal format mismatch
1 parent 3be749d commit 11db289

2 files changed

Lines changed: 191 additions & 3 deletions

File tree

align_browser/static/app.js

Lines changed: 99 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1286,8 +1286,17 @@ document.addEventListener("DOMContentLoaded", () => {
12861286
return;
12871287
}
12881288

1289+
// Prevent concurrent reloads for the same run
1290+
if (run.isReloading) {
1291+
console.log(`Skipping reload for run ${runId} - already in progress`);
1292+
return;
1293+
}
1294+
12891295
console.log(`Reloading data for run ${runId}`);
12901296

1297+
// Mark as reloading to prevent concurrent requests
1298+
run.isReloading = true;
1299+
12911300
// Show loading state
12921301
run.loadStatus = 'loading';
12931302
updateComparisonDisplay();
@@ -1327,6 +1336,9 @@ document.addEventListener("DOMContentLoaded", () => {
13271336
} catch (error) {
13281337
console.error(`Failed to reload data for run ${runId}:`, error);
13291338
run.loadStatus = 'error';
1339+
} finally {
1340+
// Clear the reloading flag
1341+
run.isReloading = false;
13301342
}
13311343

13321344
// Re-render the comparison table (current run data is unaffected)
@@ -1729,6 +1741,91 @@ document.addEventListener("DOMContentLoaded", () => {
17291741
experiments[baseKey].scenarios[run.scenario];
17301742
}
17311743

1744+
// Check if a specific KDMA can be removed from a run
1745+
function canRemoveSpecificKDMA(runId, kdmaType) {
1746+
const run = appState.pinnedRuns.get(runId);
1747+
if (!run) return false;
1748+
1749+
const currentKDMAs = run.kdmaValues || {};
1750+
1751+
// Create a copy of current KDMAs without the one we want to remove
1752+
const remainingKDMAs = { ...currentKDMAs };
1753+
delete remainingKDMAs[kdmaType];
1754+
1755+
// If no KDMAs would remain, use the original canRemoveKDMAsForRun check
1756+
if (Object.keys(remainingKDMAs).length === 0) {
1757+
return canRemoveKDMAsForRun(runId);
1758+
}
1759+
1760+
// Check if experiments exist with the remaining KDMAs for this specific scenario
1761+
// We need to directly check the manifest instead of using getValidOptionsForConstraints
1762+
// because that function might be too permissive
1763+
return checkExperimentExistsForScenario(run.scenario, run.admType, run.llmBackbone, remainingKDMAs);
1764+
}
1765+
1766+
// Check if experiments exist for a specific scenario with given parameters
1767+
function checkExperimentExistsForScenario(scenario, admType, llmBackbone, kdmas) {
1768+
const experiments = manifest.experiment_keys || manifest;
1769+
1770+
// Build the experiment key pattern to look for
1771+
const kdmaKeys = Object.keys(kdmas).sort(); // Sort for consistent ordering
1772+
let experimentKey = `${admType}_${llmBackbone}`;
1773+
1774+
// Add KDMAs to the key
1775+
for (const kdmaName of kdmaKeys) {
1776+
const kdmaValue = kdmas[kdmaName];
1777+
// Ensure decimal format for consistency with manifest keys
1778+
const formattedValue = typeof kdmaValue === 'number' ? kdmaValue.toFixed(1) : kdmaValue;
1779+
experimentKey += `_${kdmaName}-${formattedValue}`;
1780+
}
1781+
1782+
1783+
// Check if this experiment exists and has the target scenario
1784+
if (experiments[experimentKey] &&
1785+
experiments[experimentKey].scenarios &&
1786+
experiments[experimentKey].scenarios[scenario]) {
1787+
return true;
1788+
}
1789+
1790+
// If direct key lookup fails, try all possible orderings of KDMAs
1791+
// since the experiment keys might have different KDMA ordering
1792+
if (kdmaKeys.length > 1) {
1793+
const permutations = getKDMAPermutations(kdmaKeys);
1794+
for (const permutation of permutations) {
1795+
let altKey = `${admType}_${llmBackbone}`;
1796+
for (const kdmaName of permutation) {
1797+
const kdmaValue = kdmas[kdmaName];
1798+
// Ensure decimal format for consistency with manifest keys
1799+
const formattedValue = typeof kdmaValue === 'number' ? kdmaValue.toFixed(1) : kdmaValue;
1800+
altKey += `_${kdmaName}-${formattedValue}`;
1801+
}
1802+
1803+
if (experiments[altKey] &&
1804+
experiments[altKey].scenarios &&
1805+
experiments[altKey].scenarios[scenario]) {
1806+
return true;
1807+
}
1808+
}
1809+
}
1810+
1811+
return false;
1812+
}
1813+
1814+
// Generate all permutations of KDMA keys for experiment key lookup
1815+
function getKDMAPermutations(kdmaKeys) {
1816+
if (kdmaKeys.length <= 1) return [kdmaKeys];
1817+
1818+
const permutations = [];
1819+
for (let i = 0; i < kdmaKeys.length; i++) {
1820+
const rest = kdmaKeys.slice(0, i).concat(kdmaKeys.slice(i + 1));
1821+
const restPermutations = getKDMAPermutations(rest);
1822+
for (const perm of restPermutations) {
1823+
permutations.push([kdmaKeys[i]].concat(perm));
1824+
}
1825+
}
1826+
return permutations;
1827+
}
1828+
17321829
// Create KDMA controls HTML for table cells
17331830
function createKDMAControlsForRun(runId, currentKDMAs) {
17341831
const run = appState.pinnedRuns.get(runId);
@@ -1812,8 +1909,8 @@ document.addEventListener("DOMContentLoaded", () => {
18121909
18131910
<button class="table-kdma-remove-btn"
18141911
onclick="removeKDMAFromRun('${runId}', '${kdmaType}')"
1815-
${!canRemoveKDMAsForRun(runId) ? 'disabled' : ''}
1816-
title="${!canRemoveKDMAsForRun(runId) ? 'No experiments available without KDMAs' : 'Remove KDMA'}">×</button>
1912+
${!canRemoveSpecificKDMA(runId, kdmaType) ? 'disabled' : ''}
1913+
title="${!canRemoveSpecificKDMA(runId, kdmaType) ? 'No valid experiments exist without this KDMA' : 'Remove KDMA'}">×</button>
18171914
</div>
18181915
`;
18191916
}

align_browser/test_frontend_real_data.py

Lines changed: 92 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -264,8 +264,9 @@ def test_kdma_combination_default_value_issue(page, real_data_test_server):
264264
scenario_values = [opt.get_attribute("value") for opt in scenario_options if opt.get_attribute("value")]
265265
print(f"Available scenarios: {scenario_values}")
266266

267-
# Find a June2025-AF-train scenario
267+
# Find a June2025-AF-train scenario (required for this test)
268268
june_scenarios = [s for s in scenario_values if "June2025-AF-train" in s]
269+
assert len(june_scenarios) > 0, f"June2025-AF-train scenarios required for this test, but only found: {scenario_values}"
269270

270271
scenario_select.select_option(june_scenarios[0])
271272
# Wait for scenario selection to take effect
@@ -282,6 +283,9 @@ def test_kdma_combination_default_value_issue(page, real_data_test_server):
282283
# Look for "Add KDMA" button
283284
add_kdma_button = page.locator(".add-kdma-btn")
284285

286+
# This test requires the ability to add a second KDMA
287+
assert add_kdma_button.count() > 0, "Add KDMA button must be available for this test"
288+
285289
# Click Add KDMA button
286290
add_kdma_button.click()
287291

@@ -322,3 +326,90 @@ def test_kdma_combination_default_value_issue(page, real_data_test_server):
322326
assert scenario_select_value != "", "Scenario select should not go blank after adding KDMA"
323327
assert "June2025-AF-train" in scenario_select_value, "Should still have June2025-AF-train scenario selected"
324328

329+
330+
def test_kdma_delete_button_enabled_after_adding_second_kdma(page, real_data_test_server):
331+
"""Test that KDMA delete buttons become enabled after adding a second KDMA when valid single-KDMA experiments exist."""
332+
page.goto(real_data_test_server)
333+
334+
# Wait for table to load
335+
page.wait_for_selector(".comparison-table", timeout=10000)
336+
page.wait_for_function(
337+
"document.querySelectorAll('.table-adm-select').length > 0", timeout=10000
338+
)
339+
340+
# Select pipeline_baseline ADM to enable KDMA functionality
341+
adm_select = page.locator(".table-adm-select").first
342+
adm_select.select_option("pipeline_baseline")
343+
page.wait_for_load_state("networkidle")
344+
345+
# Select June2025-AF-train scenario to get multi-KDMA support
346+
scenario_select = page.locator(".table-scenario-select").first
347+
348+
# Check what scenarios are available
349+
scenario_options = scenario_select.locator("option").all()
350+
scenario_values = [opt.get_attribute("value") for opt in scenario_options if opt.get_attribute("value")]
351+
352+
# Find a June2025-AF-train scenario (required for this test)
353+
june_scenarios = [s for s in scenario_values if "June2025-AF-train" in s]
354+
assert len(june_scenarios) > 0, f"June2025-AF-train scenarios required for this test, but only found: {scenario_values}"
355+
356+
scenario_select.select_option(june_scenarios[0])
357+
page.wait_for_load_state("networkidle")
358+
359+
# Check initial KDMA delete buttons - should be disabled with single KDMA
360+
initial_delete_buttons = page.locator(".table-kdma-remove-btn")
361+
initial_delete_count = initial_delete_buttons.count()
362+
363+
# This test requires at least one initial delete button
364+
assert initial_delete_count > 0, "Should have at least one delete button with initial KDMA"
365+
366+
initial_button = initial_delete_buttons.first
367+
initial_disabled = initial_button.is_disabled()
368+
print(f"Initial delete button disabled: {initial_disabled}")
369+
370+
# With single KDMA, delete button should be disabled (can't remove all KDMAs)
371+
assert initial_disabled, "Delete button should be disabled with single KDMA"
372+
373+
# Look for "Add KDMA" button
374+
add_kdma_button = page.locator(".add-kdma-btn")
375+
376+
# This test requires the ability to add a second KDMA
377+
assert add_kdma_button.count() > 0, "Add KDMA button must be available for this test"
378+
379+
# Click Add KDMA button to add second KDMA
380+
add_kdma_button.click()
381+
382+
# Wait for new KDMA slider to be added
383+
page.wait_for_function(
384+
"document.querySelectorAll('.table-kdma-value-slider').length > 1",
385+
timeout=5000
386+
)
387+
388+
# Now check delete buttons after adding second KDMA
389+
updated_delete_buttons = page.locator(".table-kdma-remove-btn")
390+
updated_delete_count = updated_delete_buttons.count()
391+
392+
assert updated_delete_count > 1, "Should have delete buttons for multiple KDMAs"
393+
394+
# Check delete button states for asymmetric KDMA deletion
395+
# With two KDMAs (affiliation + merit) for June2025-AF-train scenario:
396+
# - affiliation alone: experiments exist for this scenario ✅
397+
# - merit alone: experiments DON'T exist for this scenario (they exist for June2025-MF-train) ❌
398+
# Therefore: only merit KDMA should be deletable (leaving affiliation alone)
399+
all_delete_buttons = updated_delete_buttons.all()
400+
assert len(all_delete_buttons) == 2, f"Should have exactly 2 delete buttons for 2 KDMAs, got {len(all_delete_buttons)}"
401+
402+
disabled_states = [btn.is_disabled() for btn in all_delete_buttons]
403+
print(f"Delete buttons disabled states: {disabled_states}")
404+
405+
# Expected behavior: exactly one delete button enabled, one disabled
406+
# The merit KDMA should be deletable (disabled=False, leaving affiliation alone)
407+
# The affiliation KDMA should NOT be deletable (disabled=True, merit alone doesn't exist for this scenario)
408+
enabled_count = sum(1 for disabled in disabled_states if not disabled)
409+
disabled_count = sum(1 for disabled in disabled_states if disabled)
410+
411+
assert enabled_count == 1, f"Expected exactly 1 enabled delete button for June2025-AF-train scenario, but got {enabled_count} enabled buttons: {disabled_states}"
412+
assert disabled_count == 1, f"Expected exactly 1 disabled delete button for June2025-AF-train scenario, but got {disabled_count} disabled buttons: {disabled_states}"
413+
414+
print("✓ Correctly identified asymmetric KDMA deletion: one deletable, one not")
415+

0 commit comments

Comments
 (0)