refactor: remove jQuery/DOM manipulation from v0/src/components/DialogBox/CombinationalAnalysis.vue #580
Conversation
…gBox/CombinationalAnalysis.vue
WalkthroughThis change refactors the Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@niladrix719 @Arnabdaz pls review the pr , thanks |
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
v0/src/components/DialogBox/CombinationalAnalysis.vue (2)
16-16: Remove unused template ref or document its intended purpose.The
messageBoxRefis declared and assigned to the template but is never used in the component logic. If it's not needed, consider removing it to keep the code clean.@button-click=" (selectedOption, circuitItem, circuitNameVal) => dialogBoxConformation(selectedOption) " - ref="messageBoxRef" />And remove the declaration:
const SimulatorState = useState() -const messageBoxRef = ref(null)Also applies to: 44-44
694-747: Excellent refactoring of printBooleanTable using Vue reactive patterns.The refactored function successfully removes jQuery dependencies and properly uses Vue's reactive system with
nextTick. The HTML generation from reactive data is clean and maintainable.Minor suggestion: Consider extracting the inline styles to a constant for better maintainability.
+const PRINT_TABLE_STYLES = `<style> + table {font: 40px Calibri;} + table, th, td {border: solid 1px #DDD;border-collapse: collapse;} + th, td {padding: 8px; text-align: center;} + thead {background-color: #f2f2f2;} +</style>` async function printBooleanTable() { // ... existing code ... - const style = `<style> - table {font: 40px Calibri;} - table, th, td {border: solid 1px #DDD;border-collapse: collapse;} - th, td {padding: 8px; text-align: center;} - thead {background-color: #f2f2f2;} - </style>` - const win = window.open('', '', 'height=700,width=700') const htmlBody = ` <html> <head> <title>Boolean Logic Table</title> - ${style} + ${PRINT_TABLE_STYLES} </head>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v0/src/components/DialogBox/CombinationalAnalysis.vue(13 hunks)
🔇 Additional comments (3)
v0/src/components/DialogBox/CombinationalAnalysis.vue (3)
27-27: LGTM! Proper Vue 3 imports for reactive refactoring.The addition of
ref,computed, andnextTickimports is appropriate for replacing jQuery/DOM manipulation with Vue's reactive system.
115-115: Good improvement: Proper boolean types for checkbox values.Using
falseinstead of empty strings for checkbox default values is the correct approach and improves type safety.Also applies to: 140-140
216-216: Excellent defensive programming with stricter null checks.The enhanced checks for both
nulland empty arrays (output.value !== null && output.value.length > 0) provide better protection against edge cases and potential runtime errors.Also applies to: 247-247, 265-265, 291-291, 295-295, 677-677
| // Reactive computed property for table data extraction | ||
| const currentTableData = computed(() => { | ||
| if (!tableBody.value || tableBody.value.length === 0) { | ||
| return [] | ||
| } | ||
|
|
||
| const fw = inputArr.value[4]?.val === true ? 1 : 0 | ||
| return tableBody.value.map((row, index) => ({ | ||
| index, | ||
| inputs: row.slice(fw, fw + inputListNames.value.length), | ||
| outputs: row.slice(fw + inputListNames.value.length), | ||
| decimal: fw > 0 ? row[0] : index | ||
| })) | ||
| }) | ||
|
|
There was a problem hiding this comment.
Remove unused computed property.
The currentTableData computed property is well-implemented but is never used anywhere in the component. This adds unnecessary complexity and memory overhead.
Either remove this unused code or implement its intended functionality:
-// Reactive computed property for table data extraction
-const currentTableData = computed(() => {
- if (!tableBody.value || tableBody.value.length === 0) {
- return []
- }
-
- const fw = inputArr.value[4]?.val === true ? 1 : 0
- return tableBody.value.map((row, index) => ({
- index,
- inputs: row.slice(fw, fw + inputListNames.value.length),
- outputs: row.slice(fw + inputListNames.value.length),
- decimal: fw > 0 ? row[0] : index
- }))
-})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Reactive computed property for table data extraction | |
| const currentTableData = computed(() => { | |
| if (!tableBody.value || tableBody.value.length === 0) { | |
| return [] | |
| } | |
| const fw = inputArr.value[4]?.val === true ? 1 : 0 | |
| return tableBody.value.map((row, index) => ({ | |
| index, | |
| inputs: row.slice(fw, fw + inputListNames.value.length), | |
| outputs: row.slice(fw + inputListNames.value.length), | |
| decimal: fw > 0 ? row[0] : index | |
| })) | |
| }) |
🤖 Prompt for AI Agents
In v0/src/components/DialogBox/CombinationalAnalysis.vue between lines 62 and
76, the computed property currentTableData is defined but never used in the
component. To fix this, either remove the entire currentTableData computed
property to eliminate unused code and reduce complexity, or identify where its
data should be used in the component and integrate it accordingly.
| const generateBooleanTableData = (outputListNames) => { | ||
| const data = {}; | ||
|
|
||
| const table = document.querySelector('.content-table'); | ||
| const rows = table?.querySelectorAll('tbody tr') || []; | ||
|
|
||
| const rowData = [...rows].slice(1).map((row, index) => { | ||
| const cells = row.cells; | ||
| const lastValue = cells[cells.length - 1]?.textContent.trim(); | ||
| return { index, value: lastValue }; | ||
| }); | ||
|
|
||
| for (const outputName of outputListNames) { | ||
| data[outputName] = { x: [], 1: [], 0: [] }; | ||
|
|
||
| rowData.forEach(({ index, value }) => { | ||
| if (value === '0') { | ||
| data[outputName]['0'].push(String(index)); | ||
| } else if (value === '1') { | ||
| data[outputName]['1'].push(String(index)); | ||
| } else { | ||
| data[outputName]['x'].push(String(index)); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| return data; | ||
| }; | ||
|
|
||
| const data = {} | ||
|
|
||
| // Use reactive table data instead of DOM manipulation | ||
| const fw = inputArr.value[4]?.val === true ? 1 : 0 | ||
| const outputStartIndex = fw + inputListNames.value.length | ||
|
|
||
| for (const outputName of outputListNames) { | ||
| data[outputName] = { x: [], 1: [], 0: [] } | ||
|
|
||
| tableBody.value.forEach((row, index) => { | ||
| const outputValue = row[outputStartIndex]?.toString().trim() | ||
|
|
||
| if (outputValue === '0') { | ||
| data[outputName]['0'].push(String(index)) | ||
| } else if (outputValue === '1') { | ||
| data[outputName]['1'].push(String(index)) | ||
| } else { | ||
| data[outputName]['x'].push(String(index)) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| return data | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify handling of multiple outputs in refactored function.
The refactored generateBooleanTableData successfully removes jQuery dependencies. However, the function only accesses row[outputStartIndex] which suggests it handles only the first output column, while the original implementation supported multiple outputs.
If multiple outputs are needed, consider updating line 326:
-const outputValue = row[outputStartIndex]?.toString().trim()
+const outputIndex = outputStartIndex + outputListNames.indexOf(outputName)
+const outputValue = row[outputIndex]?.toString().trim()🏁 Script executed:
#!/bin/bash
# Check if the function is called with multiple outputs anywhere
rg -A 5 "generateBooleanTableData" --type vueLength of output: 73
Continuing verification of generateBooleanTableData usage and invocation:
🏁 Script executed:
#!/bin/bash
# Locate the component file and search for generateBooleanTableData usage
comp=$(fd CombinationalAnalysis.vue)
echo "Found component at: $comp"
grep -n "generateBooleanTableData" "$comp" || true
rg -n "generateBooleanTableData\(" --color=never || trueLength of output: 2063
🏁 Script executed:
#!/bin/bash
# Inspect original generateBooleanTableData function implementation to confirm multiple output support
rg -n -B3 -A15 "function generateBooleanTableData" src/simulator/src/combinationalAnalysis.jsLength of output: 609
Ensure multi-output columns are handled in generateBooleanTableData
The refactored generateBooleanTableData correctly removes jQuery, but it always reads from row[outputStartIndex]—only the first output column. To support multiple outputs, each outputName needs its own column offset:
Files needing attention:
v1/src/components/DialogBox/CombinationalAnalysis.vue(around lines 315–339)
Suggested change:
- for (const outputName of outputListNames) {
+ for (let i = 0; i < outputListNames.length; i++) {
+ const outputName = outputListNames[i]
+ const columnIndex = outputStartIndex + i
data[outputName] = { x: [], 1: [], 0: [] }
- tableBody.value.forEach((row, index) => {
- const outputValue = row[outputStartIndex]?.toString().trim()
+ tableBody.value.forEach((row, index) => {
+ const outputValue = row[columnIndex]?.toString().trim()
if (outputValue === '0') {
data[outputName]['0'].push(String(index))
@@This ensures each output name maps to its corresponding column in the reactive table.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const generateBooleanTableData = (outputListNames) => { | |
| const data = {}; | |
| const table = document.querySelector('.content-table'); | |
| const rows = table?.querySelectorAll('tbody tr') || []; | |
| const rowData = [...rows].slice(1).map((row, index) => { | |
| const cells = row.cells; | |
| const lastValue = cells[cells.length - 1]?.textContent.trim(); | |
| return { index, value: lastValue }; | |
| }); | |
| for (const outputName of outputListNames) { | |
| data[outputName] = { x: [], 1: [], 0: [] }; | |
| rowData.forEach(({ index, value }) => { | |
| if (value === '0') { | |
| data[outputName]['0'].push(String(index)); | |
| } else if (value === '1') { | |
| data[outputName]['1'].push(String(index)); | |
| } else { | |
| data[outputName]['x'].push(String(index)); | |
| } | |
| }); | |
| } | |
| return data; | |
| }; | |
| const data = {} | |
| // Use reactive table data instead of DOM manipulation | |
| const fw = inputArr.value[4]?.val === true ? 1 : 0 | |
| const outputStartIndex = fw + inputListNames.value.length | |
| for (const outputName of outputListNames) { | |
| data[outputName] = { x: [], 1: [], 0: [] } | |
| tableBody.value.forEach((row, index) => { | |
| const outputValue = row[outputStartIndex]?.toString().trim() | |
| if (outputValue === '0') { | |
| data[outputName]['0'].push(String(index)) | |
| } else if (outputValue === '1') { | |
| data[outputName]['1'].push(String(index)) | |
| } else { | |
| data[outputName]['x'].push(String(index)) | |
| } | |
| }) | |
| } | |
| return data | |
| } | |
| const generateBooleanTableData = (outputListNames) => { | |
| const data = {} | |
| // Use reactive table data instead of DOM manipulation | |
| const fw = inputArr.value[4]?.val === true ? 1 : 0 | |
| const outputStartIndex = fw + inputListNames.value.length | |
| // Handle each output column by its offset | |
| for (let i = 0; i < outputListNames.length; i++) { | |
| const outputName = outputListNames[i] | |
| const columnIndex = outputStartIndex + i | |
| data[outputName] = { x: [], 1: [], 0: [] } | |
| tableBody.value.forEach((row, index) => { | |
| const outputValue = row[columnIndex]?.toString().trim() | |
| if (outputValue === '0') { | |
| data[outputName]['0'].push(String(index)) | |
| } else if (outputValue === '1') { | |
| data[outputName]['1'].push(String(index)) | |
| } else { | |
| data[outputName]['x'].push(String(index)) | |
| } | |
| }) | |
| } | |
| return data | |
| } |
🤖 Prompt for AI Agents
In v0/src/components/DialogBox/CombinationalAnalysis.vue around lines 315 to
339, the generateBooleanTableData function currently reads output values only
from row[outputStartIndex], which handles just the first output column. To fix
this and support multiple outputs, update the code inside the loop over
outputListNames to calculate the correct column index for each output by adding
the output's index to outputStartIndex, then access row at that computed index.
This ensures each outputName corresponds to its proper column in the reactive
table data.
|
Can you also make a pr for the src folder instead of v0 as we would be syncing both soon. |
|
@Arnabdaz can I make a pr for v1 folder ? |
we'll let you know regarding that for now you can make for |
|
@Arnabdaz alright , thanks for your help |
|
Thanks for the PR but we don't want ts integration in v0 or v1 at the moment, You can try it for files which dont have pre-existing PRs |
|
@ThatDeparted2061 alright |
Fixes #433
Describe the changes you have made in this PR -
Replace jQuery selectors and DOM queries with Vue reactive data processing
in v0/src/components/DialogBox/CombinationalAnalysis.vue"
Screenshots of the changes (If any) -
Note: Please check Allow edits from maintainers. if you would like us to assist in the PR.
Summary by CodeRabbit