Copilot commented on code in PR #1698:
URL: https://github.com/apache/daffodil-vscode/pull/1698#discussion_r3248886896
##########
src/launchWizard/launchWizard.ts:
##########
@@ -755,6 +755,49 @@ class LaunchWizard {
<input style="${tdmlPathVisOrHiddenStyle}" class="file-input"
value="${tdmlPath}" id="tdmlPath">
</div>
+ <div id="tunablesDiv" class="setting-div" style="margin-top: 15px;">
+ <p>Tunables:</p>
+ <p class="setting-description">Key/value configuration options</p>
+
+ <table style="width: 100%; border-collapse: collapse; margin-top:
5px;">
+ <thead>
+ <tr>
+ <th style="text-align: left;">Key</th>
+ <th style="text-align: left;">Value</th>
+ <th></th>
+ </tr>
+ </thead>
+ <tbody id="tunablesTableBody">
+ <!-- rows later -->
+ </tbody>
+ </table>
+
+ <button id="addTunableBtn" onclick="addTunableRow()"
style="margin-top: 10px;">
+ + Add Tunable
+ </button>
+
+
+ <div id="VariablesDiv" class="setting-div" style="margin-top: 15px;">
Review Comment:
Starting VariablesDiv here without closing tunablesDiv leaves the new
sections (and the following data editor/save controls) nested inside each
other. This malformed DOM can cause layout/styling and event-scope surprises;
close each setting div before opening the next one.
##########
src/launchWizard/script.js:
##########
@@ -508,4 +618,21 @@ async function updateDaffodilDebugClasspath(message) {
break
}
})
+
+ // IMPORTANT: tell extension we are ready
+ // updateConfig was not getting used before i added this. It may have been
working before because the extension was sending the config values before the
webview was ready, so the first message was getting lost. Now, we wait to send
the message until after the webview is ready, so we know for sure that the
config values will be received and rendered in the webview when it loads.
+ // Without this, tunables table would not pull from launch.json
+ //Other fields worked however, maybe because they were different data types?
+ //Confirm if this is a good addition to the code or if there is a better way
to ensure the config values are received and rendered in the webview on load
+ window.addEventListener('DOMContentLoaded', () => {
+ // vscode is injected by extension
+ if (typeof vscode !== 'undefined') {
+ vscode.postMessage({
+ command: 'updateConfigValue',
+ configIndex: 0,
Review Comment:
The initial update request always asks for configIndex 0. In a workspace
without launch.json this makes updateWebViewConfigValues try to read a
non-existent file, and after copyConfig reloads a nonzero selected config this
overwrites the form with the first configuration instead of the selected one.
##########
src/launchWizard/script.js:
##########
@@ -370,6 +476,8 @@ function copyConfig() {
type: configValues.infosetOutputType,
path: configValues.infosetOutputFilePath,
},
+ tunables: tunables,
+ variables: variables,
Review Comment:
copyConfig references tunables and variables that are not defined in this
scope. Since getConfigValues() is the only local source for those table values,
clicking Copy Config will throw a ReferenceError before posting the copy
request.
##########
src/launchWizard/launchWizard.ts:
##########
@@ -769,9 +812,9 @@ class LaunchWizard {
${dataEditorLogLevelSelect}
</select>
</div>
-
<br/>
<button class="save-button" type="button" onclick="save()">Save</button>
+ <script src="scripts.js"></script>
Review Comment:
This raw script tag points at scripts.js, but the wizard already inlines
script.js above and webview resources need to be loaded via webview URIs. As
written it adds a broken/duplicate script load in the webview.
##########
src/launchWizard/script.js:
##########
@@ -346,6 +353,105 @@ function save() {
updateOrCreate: updateOrCreate,
})
}
+// Function for adding row to tunables table
+function addTunableRow() {
+ const tableBody = document.getElementById('tunablesTableBody')
+
+ const row = document.createElement('tr')
+
+ row.innerHTML = `
+ <td><input class="file-input" /></td>
+ <td><input class="file-input" /></td>
+ <td><button onclick="this.closest('tr').remove()">X</button></td>
+ `
+
+ tableBody.appendChild(row)
+}
+
+function getTunablesFromTable() {
+ const rows = document.querySelectorAll('#tunablesTableBody tr')
+ const tunables = {}
+
+ rows.forEach((row) => {
+ const key = row.children[0].querySelector('input')?.value?.trim()
+ const value = row.children[1].querySelector('input')?.value
+
+ if (!key) return
+
+ tunables[key] = value
+ })
+
+ return tunables
+}
+
+// function to pull tunables from config and render them in the tunables
table, if there are any
+function renderTunables(tunables = {}) {
+ const tableBody = document.getElementById('tunablesTableBody')
+ // clear existing UI
+ tableBody.innerHTML = ''
+
+ Object.entries(tunables).forEach(([key, value]) => {
+ const row = document.createElement('tr')
+
+ row.innerHTML = `
+ <td><input class="file-input" value="${key}" /></td>
+ <td><input class="file-input" value="${value}" /></td>
Review Comment:
The tunable key/value from launch.json is interpolated into innerHTML
without escaping. A value containing quotes or markup can break out of the
value attribute and inject HTML/script into the webview; create the input
elements and assign .value instead.
##########
src/launchWizard/script.js:
##########
@@ -346,6 +353,105 @@ function save() {
updateOrCreate: updateOrCreate,
})
}
+// Function for adding row to tunables table
+function addTunableRow() {
+ const tableBody = document.getElementById('tunablesTableBody')
+
+ const row = document.createElement('tr')
+
+ row.innerHTML = `
+ <td><input class="file-input" /></td>
+ <td><input class="file-input" /></td>
+ <td><button onclick="this.closest('tr').remove()">X</button></td>
+ `
+
+ tableBody.appendChild(row)
+}
+
+function getTunablesFromTable() {
+ const rows = document.querySelectorAll('#tunablesTableBody tr')
+ const tunables = {}
+
+ rows.forEach((row) => {
+ const key = row.children[0].querySelector('input')?.value?.trim()
+ const value = row.children[1].querySelector('input')?.value
+
+ if (!key) return
+
+ tunables[key] = value
+ })
+
+ return tunables
+}
+
+// function to pull tunables from config and render them in the tunables
table, if there are any
+function renderTunables(tunables = {}) {
+ const tableBody = document.getElementById('tunablesTableBody')
+ // clear existing UI
+ tableBody.innerHTML = ''
+
+ Object.entries(tunables).forEach(([key, value]) => {
+ const row = document.createElement('tr')
+
+ row.innerHTML = `
+ <td><input class="file-input" value="${key}" /></td>
+ <td><input class="file-input" value="${value}" /></td>
+ <td><button onclick="this.closest('tr').remove()">X</button></td>
+ `
+
+ tableBody.appendChild(row)
+ })
+}
+
+// Function for adding row to variables table
+function addVariableRow() {
+ const tableBody = document.getElementById('variablesTableBody')
+
+ const row = document.createElement('tr')
+
+ row.innerHTML = `
+ <td><input class="file-input" /></td>
+ <td><input class="file-input" /></td>
+ <td><button onclick="this.closest('tr').remove()">X</button></td>
+ `
+
+ tableBody.appendChild(row)
+}
+
+function getVariablesFromTable() {
+ const rows = document.querySelectorAll('#variablesTableBody tr')
+ const variables = {}
+
+ rows.forEach((row) => {
+ const key = row.children[0].querySelector('input')?.value?.trim()
+ const value = row.children[1].querySelector('input')?.value
+
+ if (!key) return
+
+ variables[key] = value
+ })
+
+ return variables
+}
+
+// function to pull tunables from config and render them in the tunables
table, if there are any
Review Comment:
This comment says renderVariables pulls tunables, which is a copy/paste
mismatch and makes the variable rendering code harder to follow.
##########
src/launchWizard/script.js:
##########
@@ -346,6 +353,105 @@ function save() {
updateOrCreate: updateOrCreate,
})
}
+// Function for adding row to tunables table
+function addTunableRow() {
+ const tableBody = document.getElementById('tunablesTableBody')
+
+ const row = document.createElement('tr')
+
+ row.innerHTML = `
+ <td><input class="file-input" /></td>
+ <td><input class="file-input" /></td>
+ <td><button onclick="this.closest('tr').remove()">X</button></td>
+ `
+
+ tableBody.appendChild(row)
+}
+
+function getTunablesFromTable() {
+ const rows = document.querySelectorAll('#tunablesTableBody tr')
+ const tunables = {}
+
+ rows.forEach((row) => {
+ const key = row.children[0].querySelector('input')?.value?.trim()
+ const value = row.children[1].querySelector('input')?.value
+
+ if (!key) return
+
+ tunables[key] = value
+ })
+
+ return tunables
+}
+
+// function to pull tunables from config and render them in the tunables
table, if there are any
+function renderTunables(tunables = {}) {
+ const tableBody = document.getElementById('tunablesTableBody')
+ // clear existing UI
+ tableBody.innerHTML = ''
+
+ Object.entries(tunables).forEach(([key, value]) => {
+ const row = document.createElement('tr')
+
+ row.innerHTML = `
+ <td><input class="file-input" value="${key}" /></td>
+ <td><input class="file-input" value="${value}" /></td>
+ <td><button onclick="this.closest('tr').remove()">X</button></td>
+ `
+
+ tableBody.appendChild(row)
+ })
+}
+
+// Function for adding row to variables table
+function addVariableRow() {
+ const tableBody = document.getElementById('variablesTableBody')
+
+ const row = document.createElement('tr')
+
+ row.innerHTML = `
+ <td><input class="file-input" /></td>
+ <td><input class="file-input" /></td>
+ <td><button onclick="this.closest('tr').remove()">X</button></td>
+ `
+
+ tableBody.appendChild(row)
+}
+
+function getVariablesFromTable() {
+ const rows = document.querySelectorAll('#variablesTableBody tr')
+ const variables = {}
+
+ rows.forEach((row) => {
+ const key = row.children[0].querySelector('input')?.value?.trim()
+ const value = row.children[1].querySelector('input')?.value
+
+ if (!key) return
+
+ variables[key] = value
+ })
+
+ return variables
+}
+
+// function to pull tunables from config and render them in the tunables
table, if there are any
+function renderVariables(variables = {}) {
+ const tableBody = document.getElementById('variablesTableBody')
+ // clear existing UI
+ tableBody.innerHTML = ''
+
+ Object.entries(variables).forEach(([key, value]) => {
+ const row = document.createElement('tr')
+
+ row.innerHTML = `
+ <td><input class="file-input" value="${key}" /></td>
+ <td><input class="file-input" value="${value}" /></td>
Review Comment:
The variable key/value from launch.json is interpolated into innerHTML
without escaping. A value containing quotes or markup can break out of the
value attribute and inject HTML/script into the webview; create the input
elements and assign .value instead.
##########
src/launchWizard/script.js:
##########
@@ -346,6 +353,105 @@ function save() {
updateOrCreate: updateOrCreate,
})
}
+// Function for adding row to tunables table
+function addTunableRow() {
+ const tableBody = document.getElementById('tunablesTableBody')
+
+ const row = document.createElement('tr')
+
+ row.innerHTML = `
+ <td><input class="file-input" /></td>
+ <td><input class="file-input" /></td>
+ <td><button onclick="this.closest('tr').remove()">X</button></td>
Review Comment:
The remove-row button is labeled only "X", which is not descriptive for
screen reader users. Give it an accessible label such as the action and row
type so users can understand what it removes.
##########
src/launchWizard/script.js:
##########
@@ -346,6 +353,105 @@ function save() {
updateOrCreate: updateOrCreate,
})
}
+// Function for adding row to tunables table
+function addTunableRow() {
+ const tableBody = document.getElementById('tunablesTableBody')
+
+ const row = document.createElement('tr')
+
+ row.innerHTML = `
+ <td><input class="file-input" /></td>
+ <td><input class="file-input" /></td>
+ <td><button onclick="this.closest('tr').remove()">X</button></td>
+ `
+
+ tableBody.appendChild(row)
+}
+
+function getTunablesFromTable() {
+ const rows = document.querySelectorAll('#tunablesTableBody tr')
+ const tunables = {}
+
+ rows.forEach((row) => {
+ const key = row.children[0].querySelector('input')?.value?.trim()
+ const value = row.children[1].querySelector('input')?.value
+
+ if (!key) return
+
+ tunables[key] = value
+ })
+
+ return tunables
+}
+
+// function to pull tunables from config and render them in the tunables
table, if there are any
+function renderTunables(tunables = {}) {
+ const tableBody = document.getElementById('tunablesTableBody')
+ // clear existing UI
+ tableBody.innerHTML = ''
+
+ Object.entries(tunables).forEach(([key, value]) => {
+ const row = document.createElement('tr')
+
+ row.innerHTML = `
+ <td><input class="file-input" value="${key}" /></td>
+ <td><input class="file-input" value="${value}" /></td>
+ <td><button onclick="this.closest('tr').remove()">X</button></td>
+ `
+
+ tableBody.appendChild(row)
+ })
+}
+
+// Function for adding row to variables table
+function addVariableRow() {
+ const tableBody = document.getElementById('variablesTableBody')
+
+ const row = document.createElement('tr')
+
+ row.innerHTML = `
+ <td><input class="file-input" /></td>
+ <td><input class="file-input" /></td>
+ <td><button onclick="this.closest('tr').remove()">X</button></td>
Review Comment:
The remove-row button is labeled only "X", which is not descriptive for
screen reader users. Give it an accessible label such as the action and row
type so users can understand what it removes.
##########
src/launchWizard/script.js:
##########
@@ -346,6 +353,105 @@ function save() {
updateOrCreate: updateOrCreate,
})
}
+// Function for adding row to tunables table
+function addTunableRow() {
+ const tableBody = document.getElementById('tunablesTableBody')
+
+ const row = document.createElement('tr')
+
+ row.innerHTML = `
+ <td><input class="file-input" /></td>
+ <td><input class="file-input" /></td>
+ <td><button onclick="this.closest('tr').remove()">X</button></td>
+ `
+
+ tableBody.appendChild(row)
+}
+
+function getTunablesFromTable() {
+ const rows = document.querySelectorAll('#tunablesTableBody tr')
+ const tunables = {}
+
+ rows.forEach((row) => {
+ const key = row.children[0].querySelector('input')?.value?.trim()
+ const value = row.children[1].querySelector('input')?.value
+
+ if (!key) return
+
+ tunables[key] = value
+ })
+
+ return tunables
+}
+
+// function to pull tunables from config and render them in the tunables
table, if there are any
+function renderTunables(tunables = {}) {
+ const tableBody = document.getElementById('tunablesTableBody')
+ // clear existing UI
+ tableBody.innerHTML = ''
+
+ Object.entries(tunables).forEach(([key, value]) => {
+ const row = document.createElement('tr')
+
+ row.innerHTML = `
+ <td><input class="file-input" value="${key}" /></td>
+ <td><input class="file-input" value="${value}" /></td>
+ <td><button onclick="this.closest('tr').remove()">X</button></td>
+ `
+
+ tableBody.appendChild(row)
+ })
+}
+
+// Function for adding row to variables table
+function addVariableRow() {
+ const tableBody = document.getElementById('variablesTableBody')
+
+ const row = document.createElement('tr')
+
+ row.innerHTML = `
+ <td><input class="file-input" /></td>
+ <td><input class="file-input" /></td>
+ <td><button onclick="this.closest('tr').remove()">X</button></td>
+ `
+
+ tableBody.appendChild(row)
+}
+
+function getVariablesFromTable() {
+ const rows = document.querySelectorAll('#variablesTableBody tr')
+ const variables = {}
+
+ rows.forEach((row) => {
+ const key = row.children[0].querySelector('input')?.value?.trim()
+ const value = row.children[1].querySelector('input')?.value
+
+ if (!key) return
+
+ variables[key] = value
+ })
+
+ return variables
+}
+
+// function to pull tunables from config and render them in the tunables
table, if there are any
+function renderVariables(variables = {}) {
+ const tableBody = document.getElementById('variablesTableBody')
+ // clear existing UI
+ tableBody.innerHTML = ''
+
+ Object.entries(variables).forEach(([key, value]) => {
+ const row = document.createElement('tr')
+
+ row.innerHTML = `
+ <td><input class="file-input" value="${key}" /></td>
+ <td><input class="file-input" value="${value}" /></td>
+ <td><button onclick="this.closest('tr').remove()">X</button></td>
Review Comment:
The remove-row button is labeled only "X", which is not descriptive for
screen reader users. Give it an accessible label such as the action and row
type so users can understand what it removes.
##########
src/launchWizard/script.js:
##########
@@ -508,4 +618,21 @@ async function updateDaffodilDebugClasspath(message) {
break
}
})
+
+ // IMPORTANT: tell extension we are ready
+ // updateConfig was not getting used before i added this. It may have been
working before because the extension was sending the config values before the
webview was ready, so the first message was getting lost. Now, we wait to send
the message until after the webview is ready, so we know for sure that the
config values will be received and rendered in the webview when it loads.
+ // Without this, tunables table would not pull from launch.json
+ //Other fields worked however, maybe because they were different data types?
+ //Confirm if this is a good addition to the code or if there is a better way
to ensure the config values are received and rendered in the webview on load
Review Comment:
These comments include unresolved implementation questions and first-person
debugging notes. They should be resolved or replaced with a concise explanation
before merging so the production code does not carry uncertainty about whether
this lifecycle hook is correct.
##########
src/launchWizard/script.js:
##########
@@ -346,6 +353,105 @@ function save() {
updateOrCreate: updateOrCreate,
})
}
+// Function for adding row to tunables table
+function addTunableRow() {
+ const tableBody = document.getElementById('tunablesTableBody')
+
+ const row = document.createElement('tr')
+
+ row.innerHTML = `
+ <td><input class="file-input" /></td>
+ <td><input class="file-input" /></td>
+ <td><button onclick="this.closest('tr').remove()">X</button></td>
+ `
+
+ tableBody.appendChild(row)
+}
+
+function getTunablesFromTable() {
+ const rows = document.querySelectorAll('#tunablesTableBody tr')
+ const tunables = {}
+
+ rows.forEach((row) => {
+ const key = row.children[0].querySelector('input')?.value?.trim()
+ const value = row.children[1].querySelector('input')?.value
+
+ if (!key) return
+
+ tunables[key] = value
+ })
+
+ return tunables
+}
+
+// function to pull tunables from config and render them in the tunables
table, if there are any
+function renderTunables(tunables = {}) {
+ const tableBody = document.getElementById('tunablesTableBody')
+ // clear existing UI
+ tableBody.innerHTML = ''
+
+ Object.entries(tunables).forEach(([key, value]) => {
+ const row = document.createElement('tr')
+
+ row.innerHTML = `
+ <td><input class="file-input" value="${key}" /></td>
+ <td><input class="file-input" value="${value}" /></td>
+ <td><button onclick="this.closest('tr').remove()">X</button></td>
Review Comment:
The remove-row button is labeled only "X", which is not descriptive for
screen reader users. Give it an accessible label such as the action and row
type so users can understand what it removes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
