chore(i18n): migrate i18n.js to TypeScript and add banana-i18n types #671
Conversation
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughThe PR refactors TestBenchCreator.vue to bind to store-driven state management instead of local state, introduces group-based data grids with bitwidth controls and test type toggles, migrates i18n module from JavaScript to TypeScript with banana-i18n support, and updates tsconfig.json with trailing comma formatting. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Multiple TestBenchCreator.vue versions undergo substantial UI/logic refactoring with store integration, group management, grid layouts, and bitwidth handling; i18n migration involves straightforward porting logic plus new TypeScript type definitions; tsconfig change is trivial. Heterogeneity of changes (UI logic, state management, i18n setup, types) across multiple files demands separate reasoning per cohort. Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/components/Panels/TestBenchPanel/TestBenchCreator.vue (3)
312-325: Allow adding the first input column when none exist.Guard blocks adding the first column; compute rows safely instead.
- groups.forEach((group) => { - if (group.inputs.length === 0) return; - - group.inputs.push([]); - - for (let i = 0; i < group.inputs[0].length; i++) { - group.inputs[group.inputs.length - 1].push("0"); - } - }); + groups.forEach((group) => { + const rows = group.inputs[0]?.length ?? 0; + group.inputs.push(Array.from({ length: rows }, () => "0")); + });
339-352: Allow adding the first output column when none exist.Same issue for outputs.
- groups.forEach((group) => { - if (group.outputs.length === 0) return; - - group.outputs.push([]); - - for (let i = 0; i < group.outputs[0].length; i++) { - group.outputs[group.outputs.length - 1].push("0"); - } - }); + groups.forEach((group) => { + const rows = group.outputs[0]?.length ?? 0; + group.outputs.push(Array.from({ length: rows }, () => "0")); + });
447-469: Harden CSV parsing against blank/malformed lines.Trailing empty lines or missing ':' cause
values.spliton undefined.- lines.forEach(line => { - if (line.startsWith('G-')) { + lines.forEach((raw) => { + const line = raw?.trim(); + if (!line) return; + if (line.startsWith('G-')) { ... - } else { - const [name, values] = line.split(':'); + } else { + const sep = line.indexOf(':'); + if (sep === -1) return; + const name = line.slice(0, sep).trim(); + const values = line.slice(sep + 1).trim(); const isInput = name.startsWith('I-') && inputsName.value.includes(name.slice(2)); const isOutput = name.startsWith('O-') && outputsName.value.includes(name.slice(2)); if (isInput) { group.inputs.push(values.split(',')); } else if (isOutput) { group.outputs.push(values.split(',')); } } });
♻️ Duplicate comments (2)
v0/src/components/Panels/TestBenchPanel/TestBenchCreator.vue (1)
1-751: Same issues as src/components/Panels/TestBenchPanel/TestBenchCreator.vue.Please apply the fixes noted in the src file: empty-state guards, single results loop, numeric bitwidth inputs, CSV parsing hardening, first-column add logic, and minor TS typing.
v1/src/components/Panels/TestBenchPanel/TestBenchCreator.vue (1)
1-751: Same issues as src/components/Panels/TestBenchPanel/TestBenchCreator.vue.Please apply the fixes noted in the src file.
🧹 Nitpick comments (6)
v1/src/simulator/src/i18n.ts (1)
5-6: Consider augmenting the Window interface for type safety.Using
(window as any).localebypasses type checking. For better type safety, you could augment the Window interface:declare global { interface Window { locale?: string } }Then use:
const locale: string = window.locale || 'en'src/components/Panels/TestBenchPanel/TestBenchCreator.vue (5)
111-117: Avoid TypeScript assertions in templates; ensure numeric maxlength.Template uses
as number. Prefer runtime-safe cast.Apply:
- :maxlength="inputsBandWidth[i] as number" + :maxlength="Number(inputsBandWidth[i])"Similarly for outputs:
- :maxlength="outputsBandWidth[i]" + :maxlength="Number(outputsBandWidth[i])"
165-170: Type clamp for clarity and safety.Explicitly type input/output; coerce at entry.
-const clamp = (val) => { +const clamp = (val: unknown): number => { if (!val) return 1 const n = Number(val) if (isNaN(n)) return 1 return Math.min(Math.max(n, 1), 64) }
156-156: Prefer generic typing on reactive array.This avoids implicit any on nested arrays.
-const results: boolean[][][] = reactive([]); +const results = reactive<boolean[][][]>([]);
96-103: Use stable keys for groups.Index keys can cause rendering glitches on reorder/delete; prefer stable ids.
Introduce
id: stringin Group and use:key="group.id". I can provide a minimal UUID helper if desired.
12-28: String literals not localized.If i18n migration is in scope later, wrap UI strings with $t(...) to align with Banana-i18n.
Would you like a follow-up PR to wire these to i18n keys?
Also applies to: 31-48, 96-103, 135-144
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/components/Panels/TestBenchPanel/TestBenchCreator.vue(6 hunks)tsconfig.json(1 hunks)v0/src/components/Panels/TestBenchPanel/TestBenchCreator.vue(6 hunks)v1/src/components/Panels/TestBenchPanel/TestBenchCreator.vue(6 hunks)v1/src/simulator/src/i18n.js(0 hunks)v1/src/simulator/src/i18n.ts(1 hunks)v1/src/simulator/src/types/banana-i18n.d.ts(1 hunks)
💤 Files with no reviewable changes (1)
- v1/src/simulator/src/i18n.js
🧰 Additional context used
🧬 Code graph analysis (2)
v1/src/simulator/src/types/banana-i18n.d.ts (1)
src/simulator/src/i18n.js (1)
messages(8-10)
v1/src/simulator/src/i18n.ts (2)
v1/src/simulator/src/types/banana-i18n.d.ts (1)
Banana(2-8)src/simulator/src/i18n.js (2)
finalFallback(6-6)messages(8-10)
🔇 Additional comments (4)
tsconfig.json (1)
24-24: LGTM: Trailing comma improves maintainability.The trailing comma is valid in tsconfig.json and follows common practices for easier future additions.
v1/src/simulator/src/types/banana-i18n.d.ts (1)
1-9: Type declarations look good and match the usage.The declarations appropriately cover the banana-i18n API surface needed by i18n.ts. The
params?: any[]on line 6 trades some type safety for flexibility, which is acceptable for a library wrapper.v1/src/simulator/src/i18n.ts (2)
19-19: Verifyrequire()compatibility with ESNext module configuration.The tsconfig.json specifies
"module": "esnext", but this code uses CommonJSrequire()for dynamic imports. While this may work with your current bundler (webpack/vite), consider whether dynamicimport()expressions would be more consistent with the module configuration.If you want to align with ESNext modules, the pattern would be:
const messages: Messages = { [finalFallback]: await import(`./i18n/${finalFallback}.json`), }However, this would require making the module async or using top-level await. Given that the current approach likely works with your build setup, this is more of a consistency consideration than a blocking issue.
22-27: Good error handling for locale loading.The try-catch with console warning provides appropriate fallback behavior when a locale file is missing.
| <div v-if="groups.length === 0" class="empty-state"> | ||
| <p>No test groups added yet. Click "+ New Group" to start creating your test.</p> | ||
| </div> | ||
| <div class="testCol"> | ||
| <div class="testRow firstCol"> | ||
| Label | ||
| </div> | ||
| <div class="testContainer"> | ||
| <div v-for="(_, i) in inputsName" class="testRow" | ||
| :style="{ width: 100 / inputsBandWidth.length + '%' }"> | ||
| <input class="inputField dataGroupTitle smInputName" type="text" v-model="inputsName[i]" /> | ||
| <span @mousedown="deleteInput(i)" class="plusMinusBtn">-</span> | ||
| </div> | ||
| </div> | ||
| <div class="testContainer"> | ||
| <div v-for="(_, i) in outputsName" class="testRow" | ||
| :style="{ width: 100 / outputsBandWidth.length + '%' }"> | ||
| <input class="inputField dataGroupTitle smInputName" type="text" v-model="outputsName[i]" /> | ||
| <span @mousedown="deleteOutput(i)" class="plusMinusBtn">-</span> | ||
|
|
||
| <div v-else class="data-table-container"> | ||
| <div class="data-grid header-grid" :class="{ 'with-results': testBenchStore.showResults }"> |
There was a problem hiding this comment.
Fix conditional rendering: labels/bitwidth grids still render in empty-state.
labels-grid and bitwidth-grid are outside the v-else container, so they render even when groups.length === 0. Wrap them with a guard.
Apply:
- <div class="data-grid labels-grid" :class="{ 'with-results': testBenchStore.showResults }">
+ <div v-if="groups.length > 0" class="data-grid labels-grid" :class="{ 'with-results': testBenchStore.showResults }">
...
- <div class="data-grid bitwidth-grid" :class="{ 'with-results': testBenchStore.showResults }">
+ <div v-if="groups.length > 0" class="data-grid bitwidth-grid" :class="{ 'with-results': testBenchStore.showResults }">Also applies to: 52-95
🤖 Prompt for AI Agents
In src/components/Panels/TestBenchPanel/TestBenchCreator.vue around lines 31-36
(and also apply the same fix to lines 52-95), the labels-grid and bitwidth-grid
are rendering even when groups.length === 0 because they sit outside the v-else
block; wrap those components (or the containing elements) with the same
conditional guard so they only render when groups.length > 0 (either move them
inside the existing v-else .data-table-container block or add
v-if="groups.length > 0" to their root elements), ensuring all grid UI is hidden
in the empty-state.
| <div v-for="(bw, i) in inputsBandWidth" :key="`in-bw-${i}`" class="io-cell bitwidth-row"> | ||
| <v-btn icon size="x-small" variant="text" @click="inputsBandWidth[i] = Math.max(1, inputsBandWidth[i] - 1)" title="Decrease bitwidth"> | ||
| <v-icon>mdi-minus</v-icon> | ||
| </v-btn> | ||
| <input class="io-input bitwidth-input no-spinner" v-model="inputsBandWidth[i]" min="1" max="64" @blur="inputsBandWidth[i] = clamp(inputsBandWidth[i])"/> | ||
| <v-btn icon size="x-small" variant="text" @click="inputsBandWidth[i] = Math.min(64, inputsBandWidth[i] + 1)" title="Increase bitwidth"> | ||
| <v-icon>mdi-plus</v-icon> | ||
| </v-btn> | ||
| </div> |
There was a problem hiding this comment.
Make bitwidth inputs numeric; enforce clamping at source.
Inputs are type="text", so min/max are ignored and arrays may get strings.
Apply:
- <input class="io-input bitwidth-input no-spinner" v-model="inputsBandWidth[i]" min="1" max="64" @blur="inputsBandWidth[i] = clamp(inputsBandWidth[i])"/>
+ <input class="io-input bitwidth-input no-spinner" type="number" v-model.number="inputsBandWidth[i]" min="1" max="64" @change="inputsBandWidth[i] = clamp(inputsBandWidth[i])" aria-label="Input bitwidth"/>
- <input class="io-input bitwidth-input no-spinner" v-model="outputsBandWidth[i]" min="1" max="64" @blur="outputsBandWidth[i] = clamp(outputsBandWidth[i])"/>
+ <input class="io-input bitwidth-input no-spinner" type="number" v-model.number="outputsBandWidth[i]" min="1" max="64" @change="outputsBandWidth[i] = clamp(outputsBandWidth[i])" aria-label="Output bitwidth"/>Also applies to: 84-92
| <div v-if="testBenchStore.showResults" class="grid-cell results-col"> | ||
| <div v-for="(_, i) in results[groupIndex]" class="io-cell result-cell" :key="`g${groupIndex}-res-${i}-${testIndex}`" :class="results[groupIndex][i][testIndex] ? 'success' : 'fail'"> | ||
| <div | ||
| v-for="(_, i) in (results[groupIndex] || [])" | ||
| class="io-cell result-cell" | ||
| :key="`g${groupIndex}-res-${i}-${testIndex}`" | ||
| :class="(results[groupIndex]?.[i]?.[testIndex]) ? 'success' : 'fail'" | ||
| > | ||
| {{ (results[groupIndex]?.[i]?.[testIndex]) ? '✔' : '✘' }} | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Remove duplicated results loop and shadowed index.
Nested v-for repeats results and shadows i; produces duplicate/empty cells.
Use a single loop:
- <div v-if="testBenchStore.showResults" class="grid-cell results-col">
- <div v-for="(_, i) in results[groupIndex]" class="io-cell result-cell" :key="`g${groupIndex}-res-${i}-${testIndex}`" :class="results[groupIndex][i][testIndex] ? 'success' : 'fail'">
- <div
- v-for="(_, i) in (results[groupIndex] || [])"
- class="io-cell result-cell"
- :key="`g${groupIndex}-res-${i}-${testIndex}`"
- :class="(results[groupIndex]?.[i]?.[testIndex]) ? 'success' : 'fail'"
- >
- {{ (results[groupIndex]?.[i]?.[testIndex]) ? '✔' : '✘' }}
- </div>
- </div>
- </div>
+ <div v-if="testBenchStore.showResults" class="grid-cell results-col">
+ <div
+ v-for="(_, i) in (results[groupIndex] || [])"
+ class="io-cell result-cell"
+ :key="`g${groupIndex}-res-${i}-${testIndex}`"
+ :class="(results[groupIndex]?.[i]?.[testIndex]) ? 'success' : 'fail'"
+ >
+ {{ (results[groupIndex]?.[i]?.[testIndex]) ? '✔' : '✘' }}
+ </div>
+ </div>📝 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.
| <div v-if="testBenchStore.showResults" class="grid-cell results-col"> | |
| <div v-for="(_, i) in results[groupIndex]" class="io-cell result-cell" :key="`g${groupIndex}-res-${i}-${testIndex}`" :class="results[groupIndex][i][testIndex] ? 'success' : 'fail'"> | |
| <div | |
| v-for="(_, i) in (results[groupIndex] || [])" | |
| class="io-cell result-cell" | |
| :key="`g${groupIndex}-res-${i}-${testIndex}`" | |
| :class="(results[groupIndex]?.[i]?.[testIndex]) ? 'success' : 'fail'" | |
| > | |
| {{ (results[groupIndex]?.[i]?.[testIndex]) ? '✔' : '✘' }} | |
| </div> | |
| </div> | |
| <div v-if="testBenchStore.showResults" class="grid-cell results-col"> | |
| <div | |
| v-for="(_, i) in (results[groupIndex] || [])" | |
| class="io-cell result-cell" | |
| :key="`g${groupIndex}-res-${i}-${testIndex}`" | |
| :class="(results[groupIndex]?.[i]?.[testIndex]) ? 'success' : 'fail'" | |
| > | |
| {{ (results[groupIndex]?.[i]?.[testIndex]) ? '✔' : '✘' }} | |
| </div> | |
| </div> |
🤖 Prompt for AI Agents
In src/components/Panels/TestBenchPanel/TestBenchCreator.vue around lines 119 to
129, there is a duplicated nested v-for that repeats the results loop and
shadows the index variable, causing duplicate/empty cells; remove the outer (or
inner) duplicate loop so only a single v-for iterates (e.g., v-for="(_,
resIndex) in (results[groupIndex] || [])"), update the key to use that unique
index (e.g., `g${groupIndex}-res-${resIndex}-${testIndex}`) and use safe access
for the boolean check (results[groupIndex]?.[resIndex]?.[testIndex]) to
determine classes/content.
2 participants
Fixes #
Migrated
i18n.jsto TypeScript for better type safety and maintainability, and added type definitions forbanana-i18n.Changes
i18n.js→i18n.tsbanana-i18n.d.tstype declaration fileVerified
Note
UI components are not fully wired to use Banana-i18n keys yet, so Hindi text may not appear visually. This PR focuses only on TypeScript migration .
Summary by CodeRabbit