Skip to content

Commit 3973e2c

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 3973e2c

4 files changed

Lines changed: 515 additions & 55 deletions

File tree

align_browser/build.py

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,6 @@ def build_frontend(
110110

111111
print(f"Data generated in {data_output_dir}")
112112

113-
114113
return output_dir
115114

116115

align_browser/static/app.js

Lines changed: 159 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -828,9 +828,16 @@ document.addEventListener("DOMContentLoaded", () => {
828828
const newKDMAs = { ...currentKDMAs, [kdmaType]: initialValue };
829829
run.kdmaValues = newKDMAs;
830830

831+
// Update the columnParameters to sync with the new KDMA values
832+
const params = getParametersForRun(runId);
833+
params.kdmas = newKDMAs;
834+
831835
// Refresh the comparison display to show new KDMA control
832836
updateComparisonDisplay();
833837

838+
// Reload experiment data for the new KDMA combination
839+
reloadPinnedRun(runId);
840+
834841
// Update URL state
835842
urlState.updateURL();
836843
};
@@ -846,9 +853,16 @@ document.addEventListener("DOMContentLoaded", () => {
846853
// Update the run's KDMA values directly
847854
run.kdmaValues = currentKDMAs;
848855

856+
// Update the columnParameters to sync with the new KDMA values
857+
const params = getParametersForRun(runId);
858+
params.kdmas = currentKDMAs;
859+
849860
// Refresh the comparison display
850861
updateComparisonDisplay();
851862

863+
// Reload experiment data for the new KDMA combination
864+
reloadPinnedRun(runId);
865+
852866
// Update URL state
853867
urlState.updateURL();
854868
};
@@ -878,9 +892,16 @@ document.addEventListener("DOMContentLoaded", () => {
878892
// Update the run's KDMA values directly
879893
run.kdmaValues = currentKDMAs;
880894

895+
// Update the columnParameters to sync with the new KDMA values
896+
const params = getParametersForRun(runId);
897+
params.kdmas = currentKDMAs;
898+
881899
// Refresh the comparison display
882900
updateComparisonDisplay();
883901

902+
// Reload experiment data for the new KDMA combination
903+
reloadPinnedRun(runId);
904+
884905
// Update URL state
885906
urlState.updateURL();
886907
};
@@ -929,7 +950,7 @@ document.addEventListener("DOMContentLoaded", () => {
929950
// Update the display value immediately
930951
const valueDisplay = document.getElementById(`kdma-value-${runId}-${kdmaType}`);
931952
if (valueDisplay) {
932-
valueDisplay.textContent = newValue.toFixed(1);
953+
valueDisplay.textContent = formatKDMAValue(newValue);
933954
}
934955

935956
currentKDMAs[kdmaType] = newValue;
@@ -1106,7 +1127,7 @@ document.addEventListener("DOMContentLoaded", () => {
11061127

11071128
// Add Overall Score (on top)
11081129
if (scoreItem && scoreItem.score !== undefined) {
1109-
html += `<div style="margin: 8px 0;"><strong>Score:</strong> ${scoreItem.score.toFixed(3)}</div>`;
1130+
html += `<div style="margin: 8px 0;"><strong>Score:</strong> ${formatScoreValue(scoreItem.score)}</div>`;
11101131
}
11111132

11121133
// Add Average Decision Time
@@ -1132,14 +1153,8 @@ document.addEventListener("DOMContentLoaded", () => {
11321153
};
11331154
}
11341155

1135-
// Generate experiment key from parameters
1136-
const kdmaParts = [];
1137-
Object.entries(kdmas || {}).forEach(([kdma, value]) => {
1138-
kdmaParts.push(`${kdma}-${value.toFixed(1)}`);
1139-
});
1140-
const kdmaString = kdmaParts.sort().join("_");
1141-
// If no KDMAs, don't append the trailing underscore
1142-
const experimentKey = kdmaString ? `${admType}_${llmBackbone}_${kdmaString}` : `${admType}_${llmBackbone}`;
1156+
// Generate experiment key from parameters using shared utility
1157+
const experimentKey = buildExperimentKey(admType, llmBackbone, kdmas);
11431158

11441159
console.log("Loading experiment data:", experimentKey, "Scenario:", scenario);
11451160

@@ -1286,8 +1301,17 @@ document.addEventListener("DOMContentLoaded", () => {
12861301
return;
12871302
}
12881303

1304+
// Prevent concurrent reloads for the same run
1305+
if (run.isReloading) {
1306+
console.log(`Skipping reload for run ${runId} - already in progress`);
1307+
return;
1308+
}
1309+
12891310
console.log(`Reloading data for run ${runId}`);
12901311

1312+
// Mark as reloading to prevent concurrent requests
1313+
run.isReloading = true;
1314+
12911315
// Show loading state
12921316
run.loadStatus = 'loading';
12931317
updateComparisonDisplay();
@@ -1327,6 +1351,9 @@ document.addEventListener("DOMContentLoaded", () => {
13271351
} catch (error) {
13281352
console.error(`Failed to reload data for run ${runId}:`, error);
13291353
run.loadStatus = 'error';
1354+
} finally {
1355+
// Clear the reloading flag
1356+
run.isReloading = false;
13301357
}
13311358

13321359
// Re-render the comparison table (current run data is unaffected)
@@ -1729,6 +1756,102 @@ document.addEventListener("DOMContentLoaded", () => {
17291756
experiments[baseKey].scenarios[run.scenario];
17301757
}
17311758

1759+
// Check if a specific KDMA can be removed from a run
1760+
function canRemoveSpecificKDMA(runId, kdmaType) {
1761+
const run = appState.pinnedRuns.get(runId);
1762+
if (!run) return false;
1763+
1764+
const currentKDMAs = run.kdmaValues || {};
1765+
1766+
// Create a copy of current KDMAs without the one we want to remove
1767+
const remainingKDMAs = { ...currentKDMAs };
1768+
delete remainingKDMAs[kdmaType];
1769+
1770+
// If no KDMAs would remain, use the original canRemoveKDMAsForRun check
1771+
if (Object.keys(remainingKDMAs).length === 0) {
1772+
return canRemoveKDMAsForRun(runId);
1773+
}
1774+
1775+
// Check if experiments exist with the remaining KDMAs for this specific scenario
1776+
// We need to directly check the manifest instead of using getValidOptionsForConstraints
1777+
// because that function might be too permissive
1778+
return checkExperimentExistsForScenario(run.scenario, run.admType, run.llmBackbone, remainingKDMAs);
1779+
}
1780+
1781+
// Format KDMA value consistently across the application
1782+
function formatKDMAValue(value) {
1783+
return typeof value === 'number' ? value.toFixed(1) : value;
1784+
}
1785+
1786+
// Format score value consistently across the application
1787+
function formatScoreValue(value) {
1788+
return typeof value === 'number' ? value.toFixed(3) : value.toString();
1789+
}
1790+
1791+
// Generate experiment key from parameters (shared utility function)
1792+
function buildExperimentKey(admType, llmBackbone, kdmas) {
1793+
const kdmaParts = [];
1794+
Object.entries(kdmas || {}).forEach(([kdma, value]) => {
1795+
kdmaParts.push(`${kdma}-${formatKDMAValue(value)}`);
1796+
});
1797+
const kdmaString = kdmaParts.sort().join("_");
1798+
return kdmaString ? `${admType}_${llmBackbone}_${kdmaString}` : `${admType}_${llmBackbone}`;
1799+
}
1800+
1801+
// Check if experiments exist for a specific scenario with given parameters
1802+
function checkExperimentExistsForScenario(scenario, admType, llmBackbone, kdmas) {
1803+
const experiments = manifest.experiment_keys || manifest;
1804+
1805+
// Build the experiment key using shared utility
1806+
const experimentKey = buildExperimentKey(admType, llmBackbone, kdmas);
1807+
1808+
// Check if this experiment exists and has the target scenario
1809+
if (experiments[experimentKey] &&
1810+
experiments[experimentKey].scenarios &&
1811+
experiments[experimentKey].scenarios[scenario]) {
1812+
return true;
1813+
}
1814+
1815+
// If direct key lookup fails, try all possible orderings of KDMAs
1816+
// since the experiment keys might have different KDMA ordering
1817+
const kdmaKeys = Object.keys(kdmas || {});
1818+
if (kdmaKeys.length > 1) {
1819+
const permutations = getKDMAPermutations(kdmaKeys);
1820+
for (const permutation of permutations) {
1821+
const reorderedKdmas = {};
1822+
permutation.forEach(kdmaName => {
1823+
if (kdmas[kdmaName] !== undefined) {
1824+
reorderedKdmas[kdmaName] = kdmas[kdmaName];
1825+
}
1826+
});
1827+
1828+
const altKey = buildExperimentKey(admType, llmBackbone, reorderedKdmas);
1829+
if (experiments[altKey] &&
1830+
experiments[altKey].scenarios &&
1831+
experiments[altKey].scenarios[scenario]) {
1832+
return true;
1833+
}
1834+
}
1835+
}
1836+
1837+
return false;
1838+
}
1839+
1840+
// Generate all permutations of KDMA keys for experiment key lookup
1841+
function getKDMAPermutations(kdmaKeys) {
1842+
if (kdmaKeys.length <= 1) return [kdmaKeys];
1843+
1844+
const permutations = [];
1845+
for (let i = 0; i < kdmaKeys.length; i++) {
1846+
const rest = kdmaKeys.slice(0, i).concat(kdmaKeys.slice(i + 1));
1847+
const restPermutations = getKDMAPermutations(rest);
1848+
for (const perm of restPermutations) {
1849+
permutations.push([kdmaKeys[i]].concat(perm));
1850+
}
1851+
}
1852+
return permutations;
1853+
}
1854+
17321855
// Create KDMA controls HTML for table cells
17331856
function createKDMAControlsForRun(runId, currentKDMAs) {
17341857
const run = appState.pinnedRuns.get(runId);
@@ -1745,25 +1868,31 @@ document.addEventListener("DOMContentLoaded", () => {
17451868
html += createSingleKDMAControlForRun(runId, kdmaType, value, index);
17461869
});
17471870

1748-
// Add button
1749-
if (canAddMore) {
1750-
const availableKDMAs = getValidKDMAsForRun(runId);
1751-
const availableTypes = Object.keys(availableKDMAs).filter(type =>
1752-
!currentKDMAs || currentKDMAs[type] === undefined
1753-
);
1754-
1755-
if (availableTypes.length > 0) {
1756-
html += `<button class="add-kdma-btn" onclick="addKDMAToRun('${runId}')"
1757-
style="margin-top: 5px; font-size: 12px; padding: 2px 6px;">
1758-
Add KDMA
1759-
</button>`;
1871+
// Add button - always show but enable/disable based on availability
1872+
const availableKDMAs = getValidKDMAsForRun(runId);
1873+
const availableTypes = Object.keys(availableKDMAs).filter(type =>
1874+
!currentKDMAs || currentKDMAs[type] === undefined
1875+
);
1876+
1877+
const canAdd = canAddMore && availableTypes.length > 0;
1878+
const disabledAttr = canAdd ? '' : 'disabled';
1879+
1880+
// Determine tooltip text for disabled state
1881+
let tooltipText = '';
1882+
if (!canAdd) {
1883+
if (!canAddMore) {
1884+
tooltipText = `title="Maximum KDMAs reached (${maxKDMAs})"`;
17601885
} else {
1761-
html += `<div style="font-size: 12px; color: #666; margin-top: 5px;">All KDMA types added</div>`;
1886+
tooltipText = 'title="All available KDMA types have been added"';
17621887
}
1763-
} else {
1764-
html += `<div style="font-size: 12px; color: #666; margin-top: 5px;">Max KDMAs reached (${maxKDMAs})</div>`;
17651888
}
17661889

1890+
html += `<button class="add-kdma-btn" onclick="addKDMAToRun('${runId}')"
1891+
${disabledAttr} ${tooltipText}
1892+
style="margin-top: 5px; font-size: 12px; padding: 2px 6px;">
1893+
Add KDMA
1894+
</button>`;
1895+
17671896
html += '</div>';
17681897
return html;
17691898
}
@@ -1808,12 +1937,12 @@ document.addEventListener("DOMContentLoaded", () => {
18081937
min="0" max="1" step="0.1"
18091938
value="${value}"
18101939
oninput="handleRunKDMASliderInput('${runId}', '${kdmaType}', this)">
1811-
<span class="table-kdma-value-display" id="kdma-value-${runId}-${kdmaType}">${value.toFixed(1)}</span>
1940+
<span class="table-kdma-value-display" id="kdma-value-${runId}-${kdmaType}">${formatKDMAValue(value)}</span>
18121941
18131942
<button class="table-kdma-remove-btn"
18141943
onclick="removeKDMAFromRun('${runId}', '${kdmaType}')"
1815-
${!canRemoveKDMAsForRun(runId) ? 'disabled' : ''}
1816-
title="${!canRemoveKDMAsForRun(runId) ? 'No experiments available without KDMAs' : 'Remove KDMA'}">×</button>
1944+
${!canRemoveSpecificKDMA(runId, kdmaType) ? 'disabled' : ''}
1945+
title="${!canRemoveSpecificKDMA(runId, kdmaType) ? 'No valid experiments exist without this KDMA' : 'Remove KDMA'}">×</button>
18171946
</div>
18181947
`;
18191948
}
@@ -1845,7 +1974,7 @@ document.addEventListener("DOMContentLoaded", () => {
18451974

18461975
switch (type) {
18471976
case 'number':
1848-
return typeof value === 'number' ? value.toFixed(3) : value.toString();
1977+
return formatScoreValue(value);
18491978

18501979
case 'longtext':
18511980
if (typeof value === 'string' && value.length > 800) {
@@ -1910,7 +2039,7 @@ document.addEventListener("DOMContentLoaded", () => {
19102039
kdmaEntries.forEach(([kdmaName, kdmaValue]) => {
19112040
kdmaHtml += `<div class="kdma-value-item">
19122041
<span class="kdma-name">${escapeHtml(kdmaName)}:</span>
1913-
<span class="kdma-number">${typeof kdmaValue === 'number' ? kdmaValue.toFixed(1) : kdmaValue}</span>
2042+
<span class="kdma-number">${formatKDMAValue(kdmaValue)}</span>
19142043
</div>`;
19152044
});
19162045
kdmaHtml += '</div>';

align_browser/static/style.css

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -500,6 +500,13 @@ footer {
500500
box-shadow: 0 0 0 2px rgba(0, 123, 255, 0.25);
501501
}
502502

503+
.add-kdma-btn:disabled {
504+
background-color: #6c757d;
505+
border-color: #6c757d;
506+
cursor: not-allowed;
507+
opacity: 0.6;
508+
}
509+
503510
.table-kdma-remove-btn {
504511
background-color: #dc3545;
505512
color: white;

0 commit comments

Comments
 (0)