fix: standardized hex formats and removed duplicate theme keys#728
fix: standardized hex formats and removed duplicate theme keys#728rolex-67 wants to merge 3 commits intoCircuitVerse:mainfrom
Conversation
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughThis change updates theme color values across themes, removes the inline "Custom Theme" object from the static themes declaration, and adds runtime augmentation that safely reads a 'Custom Theme' JSON object from localStorage (with defensive checks and parsing) and merges it into Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR standardizes color formats across all theme definitions and refactors the Custom Theme initialization. The main goal is to ensure compatibility with HTML5 <input type="color"> elements and eliminate build warnings from duplicate keys.
Key Changes:
- Converted all named colors (e.g.,
white,black,green) and 3-digit hex codes (e.g.,#fff,#ddd) to 7-character lowercase hex format (e.g.,#ffffff,#dddddd) - Removed duplicate
--br-circuitkey definition in the Default Theme - Refactored Custom Theme to use spread operator with Default Theme as fallback
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @v0/src/simulator/src/themer/themes.ts:
- Around line 330-334: The code that builds themes['Custom Theme'] directly
accesses localStorage and JSON.parse which can throw in SSR, test envs, private
mode, or on malformed JSON; wrap the retrieval and parse in a safe guard: check
typeof window !== 'undefined' and typeof window.localStorage !== 'undefined'
before accessing, then use a try/catch around localStorage.getItem('Custom
Theme') and JSON.parse(...) to return an empty object on any error, and merge
that safe parsed object into themes['Custom Theme'] while still spreading
themes['Default Theme'].
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v0/src/simulator/src/themer/themes.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (1)
v0/src/simulator/src/themer/themes.ts (1)
5-327: Color standardization successfully verified.All color conversions are correctly implemented:
- Named colors and shorthand hex codes have been converted to 7-character hex format (#RRGGBB)
- No duplicate keys remain in any theme (verified per-theme)
- Nine rgba() values are present for transparency where needed, all valid for CSS and
<input type="color">compatibility- Format consistency confirmed across all six themes (Default Theme, Night Sky, Lite-born Spring, G&W, High Contrast, Color Blind)
|
I have restored the original #eaeaeb tints for 'Lite-born Spring' and 'G&W' and implemented a robust try-catch wrapper for localStorage to handle potential environment issues or data corruption. Ready for review! |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @v0/src/components/Navbar/User/UserMenu.vue:
- Around line 259-268: The watcher on locale calls localStorage.setItem directly
which can throw in restricted environments; wrap the
localStorage.setItem('locale', newLocale) call in a try-catch inside the
watch(locale, ...) callback, swallow or log the error (e.g., console.warn or an
existing logger) and continue to set document.documentElement.lang and dir so
the locale switcher won’t crash; keep document.documentElement.lang = newLocale
and the dir logic unchanged and only guard the storage write.
- Around line 263-267: The current locale-to-direction logic only treats 'ar' as
RTL; update it to treat all RTL languages (e.g., 'ar', 'he', 'fa', 'ur') as RTL
by checking newLocale against a set/array of RTL codes (e.g., const RTL_LOCALES
= ['ar','he','fa','ur']) and then setting document.documentElement.dir =
RTL_LOCALES.includes(newLocale) ? 'rtl' : 'ltr'; apply this change where
newLocale is used to set document.documentElement.dir in UserMenu.vue.
In @v0/src/locales/i18n.ts:
- Around line 12-21: Move the existing availableLocale declaration so it appears
before getInitialLocale(), then in getInitialLocale() replace the hardcoded
supported array with a derived list from availableLocale (e.g., map to their
locale codes) and use that for validation; finally remove the duplicate
availableLocale declaration later in the file so there is a single source of
truth.
- Around line 33-42: The exported availableLocale constant was changed to a
literal tuple type which narrows its public type; restore the explicit broad
type so callers get Array<{ title: string; value: string }>. Update the
availableLocale declaration to include the type annotation Array<{ title:
string; value: string }> (or readonly Array<{ title: string; value: string }> if
immutability is desired) so imports (e.g., in UserMenu.vue) and external
consumers receive the intended public signature; keep the same values but re-add
the type annotation to the exported identifier availableLocale.
🧹 Nitpick comments (2)
v0/src/components/Navbar/User/UserMenu.vue (2)
16-16: Unconventional binding syntax change.The template has been updated from Vue's shorthand syntax (
@click,:prop) to longhand syntax (v-on:click,v-bind:prop). While functionally equivalent, this goes against common Vue.js conventions where shorthand is preferred for brevity and readability.If this is mandated by a project-wide style guide or linting rule, no action needed. Otherwise, consider reverting to shorthand syntax for consistency with Vue ecosystem best practices.
Also applies to: 26-30, 37-37, 54-54, 62-62, 75-75, 83-83, 91-91, 107-107, 121-121, 128-128, 149-149, 155-155, 161-161, 171-171, 181-181, 191-192, 202-202, 216-217, 225-226
273-285: Validate restored locale against available locales.The code restores
savedLocalefrom localStorage without validating it's in theavailableLocalelist. If the user has an old/invalid locale stored, or localStorage is corrupted, this could set an unsupported locale.✅ Proposed validation logic
onMounted(function() { - const savedLocale = localStorage.getItem('locale') - if (savedLocale) { + try { + const savedLocale = localStorage.getItem('locale') + if (savedLocale && availableLocale.includes(savedLocale)) { locale.value = savedLocale + } + } catch (error) { + console.warn('Failed to restore locale preference:', error) } document.documentElement.lang = locale.value
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
v0/src/components/Navbar/User/UserMenu.vuev0/src/locales/bn.jsonv0/src/locales/de.jsonv0/src/locales/es.jsonv0/src/locales/fr.jsonv0/src/locales/i18n.tsv0/src/locales/jp.jsonv0/src/locales/zh.jsonv0/src/simulator/src/minimap.js
✅ Files skipped from review due to trivial changes (2)
- v0/src/locales/zh.json
- v0/src/locales/bn.json
🔇 Additional comments (11)
v0/src/simulator/src/minimap.js (2)
196-199: Behavior change: Removed fade-out animation.The original implementation used jQuery's
fadeOut()which provided a smooth animation when hiding the minimap. The new code setsdisplay = 'none'directly, making the minimap disappear instantly. This changes the user experience.If the animation removal is intentional, this looks good. If not, consider using CSS transitions to preserve the fade effect.
168-175: Verifythis.ctxinitialization;clear()is not currently called.The
clear()method usesthis.ctx(line 174), which is initialized only insetup()(line 79). However,this.canvasis actually initialized at object construction (line 20), not in setup. The claim about initialization order is partially inaccurate. More importantly,miniMapArea.clear()is not called anywhere in the codebase, so this is a theoretical code smell rather than an active issue. If the method is intended to be callable before initialization, add a defensive check:if (!this.ctx) return.v0/src/components/Navbar/User/UserMenu.vue (1)
288-290: LGTM - Good refactoring improvements.The changes improve code consistency and clarity:
- Validation rules use standard function syntax matching the watch/onMounted style
- Error typing in catch block enables proper error handling
- Snackbar color logic is more explicit with the ternary operator
Also applies to: 359-359, 389-389
v0/src/locales/fr.json (1)
1-167: LGTM - Well-structured French localization file.The French locale file is properly formatted with complete translation coverage across all UI sections. The JSON structure is valid and consistent with the i18n pattern.
Note: Translation accuracy verification is outside the scope of code review.
v0/src/locales/de.json (1)
1-167: LGTM - Well-structured German localization file.The German locale file is properly formatted with complete translation coverage matching the expected i18n structure. JSON syntax is valid.
Note: Translation accuracy verification is outside the scope of code review.
v0/src/locales/es.json (1)
1-167: LGTM - Well-structured Spanish localization file.The Spanish locale file is properly formatted with complete translation coverage following the established i18n structure. JSON syntax is valid.
Note: Translation accuracy verification is outside the scope of code review.
v0/src/locales/i18n.ts (4)
1-167: PR description doesn't match the files under review.The PR objectives describe changes to
themes.ts(standardizing hex color formats, removing duplicate "--br-circuit" keys, and improving Custom Theme initialization with localStorage), but the files provided for review are about i18n localization (i18n.tsandjp.json).Please confirm whether:
- Additional theme-related files should be included in this review
- The PR description applies to a different changeset
- These localization changes are intentionally part of the same PR
5-9: LGTM!The new locale imports are properly structured and align with the expanded internationalization support.
25-25: LGTM!The dynamic locale initialization properly restores the user's preferred language from localStorage.
29-29: LGTM!All imported locale modules are correctly registered in the i18n messages configuration.
v0/src/locales/jp.json (1)
1-167: LGTM!This Japanese localization file is well-structured and provides comprehensive UI translations. The JSON structure is valid and mirrors the organization of other locale files.
| watch(locale, function(newLocale) { | ||
| localStorage.setItem('locale', newLocale) | ||
| document.documentElement.lang = newLocale | ||
|
|
||
| if (newLocale === 'ar') { | ||
| document.documentElement.dir = 'rtl' | ||
| } else { | ||
| document.documentElement.dir = 'ltr' | ||
| } | ||
| }) |
There was a problem hiding this comment.
Add error handling for localStorage access.
The localStorage.setItem() call can throw exceptions in private/incognito mode, when storage is full, or when disabled. This would crash the locale switcher.
🛡️ Proposed fix with try-catch
watch(locale, function(newLocale) {
- localStorage.setItem('locale', newLocale)
+ try {
+ localStorage.setItem('locale', newLocale)
+ } catch (error) {
+ console.warn('Failed to save locale preference:', error)
+ }
document.documentElement.lang = newLocale
if (newLocale === 'ar') {📝 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.
| watch(locale, function(newLocale) { | |
| localStorage.setItem('locale', newLocale) | |
| document.documentElement.lang = newLocale | |
| if (newLocale === 'ar') { | |
| document.documentElement.dir = 'rtl' | |
| } else { | |
| document.documentElement.dir = 'ltr' | |
| } | |
| }) | |
| watch(locale, function(newLocale) { | |
| try { | |
| localStorage.setItem('locale', newLocale) | |
| } catch (error) { | |
| console.warn('Failed to save locale preference:', error) | |
| } | |
| document.documentElement.lang = newLocale | |
| if (newLocale === 'ar') { | |
| document.documentElement.dir = 'rtl' | |
| } else { | |
| document.documentElement.dir = 'ltr' | |
| } | |
| }) |
🤖 Prompt for AI Agents
In @v0/src/components/Navbar/User/UserMenu.vue around lines 259 - 268, The
watcher on locale calls localStorage.setItem directly which can throw in
restricted environments; wrap the localStorage.setItem('locale', newLocale) call
in a try-catch inside the watch(locale, ...) callback, swallow or log the error
(e.g., console.warn or an existing logger) and continue to set
document.documentElement.lang and dir so the locale switcher won’t crash; keep
document.documentElement.lang = newLocale and the dir logic unchanged and only
guard the storage write.
| if (newLocale === 'ar') { | ||
| document.documentElement.dir = 'rtl' | ||
| } else { | ||
| document.documentElement.dir = 'ltr' | ||
| } |
There was a problem hiding this comment.
Incomplete RTL language support.
Only Arabic ('ar') is handled for RTL directionality. Other RTL languages like Hebrew ('he'), Farsi/Persian ('fa'), and Urdu ('ur') should also set dir='rtl'.
🌍 Proposed fix for comprehensive RTL support
- if (newLocale === 'ar') {
+ const rtlLanguages = ['ar', 'he', 'fa', 'ur']
+ if (rtlLanguages.includes(newLocale)) {
document.documentElement.dir = 'rtl'
} else {
document.documentElement.dir = 'ltr'
}📝 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.
| if (newLocale === 'ar') { | |
| document.documentElement.dir = 'rtl' | |
| } else { | |
| document.documentElement.dir = 'ltr' | |
| } | |
| const rtlLanguages = ['ar', 'he', 'fa', 'ur'] | |
| if (rtlLanguages.includes(newLocale)) { | |
| document.documentElement.dir = 'rtl' | |
| } else { | |
| document.documentElement.dir = 'ltr' | |
| } |
🤖 Prompt for AI Agents
In @v0/src/components/Navbar/User/UserMenu.vue around lines 263 - 267, The
current locale-to-direction logic only treats 'ar' as RTL; update it to treat
all RTL languages (e.g., 'ar', 'he', 'fa', 'ur') as RTL by checking newLocale
against a set/array of RTL codes (e.g., const RTL_LOCALES =
['ar','he','fa','ur']) and then setting document.documentElement.dir =
RTL_LOCALES.includes(newLocale) ? 'rtl' : 'ltr'; apply this change where
newLocale is used to set document.documentElement.dir in UserMenu.vue.
| export const availableLocale = [ | ||
| { title: 'English', value: 'en' }, | ||
| { title: 'Hindi', value: 'hi' }, | ||
| { title: 'हिंदी', value: 'hi' }, | ||
| { title: 'বাংলা', value: 'bn' }, | ||
| { title: 'Español', value: 'es' }, | ||
| { title: 'Français', value: 'fr' }, | ||
| { title: 'Deutsch', value: 'de' }, | ||
| { title: '日本語', value: 'jp' }, | ||
| { title: '中文', value: 'zh' }, | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n v0/src/locales/i18n.ts | head -50Repository: CircuitVerse/cv-frontend-vue
Length of output: 1590
🏁 Script executed:
rg -n "availableLocale" --type=ts --type=vueRepository: CircuitVerse/cv-frontend-vue
Length of output: 99
🏁 Script executed:
rg -n "import.*availableLocale" --type=ts --type=vue -A 2Repository: CircuitVerse/cv-frontend-vue
Length of output: 99
🏁 Script executed:
rg -n "availableLocale" --type=tsRepository: CircuitVerse/cv-frontend-vue
Length of output: 255
🏁 Script executed:
rg -n "availableLocale" -i | head -30Repository: CircuitVerse/cv-frontend-vue
Length of output: 792
🏁 Script executed:
rg "import.*availableLocale|from.*i18n" -A 2 -B 2Repository: CircuitVerse/cv-frontend-vue
Length of output: 15335
🏁 Script executed:
# Check for type imports or typeof patterns
rg "type.*availableLocale|typeof.*availableLocale"Repository: CircuitVerse/cv-frontend-vue
Length of output: 54
🏁 Script executed:
# Check how availableLocale is used in UserMenu.vue for v0
sed -n '235,250p' v0/src/components/Navbar/User/UserMenu.vueRepository: CircuitVerse/cv-frontend-vue
Length of output: 549
🏁 Script executed:
# Check the context around line 37 where it's used
sed -n '30,50p' v0/src/components/Navbar/User/UserMenu.vueRepository: CircuitVerse/cv-frontend-vue
Length of output: 616
🏁 Script executed:
# Compare v0 type annotation with v1/src versions
cat -n v1/src/locales/i18n.ts | sed -n '18,25p'Repository: CircuitVerse/cv-frontend-vue
Length of output: 286
🏁 Script executed:
# Check if there are any type imports related to availableLocale
rg "type.*Locale|Locale.*type" --type=tsRepository: CircuitVerse/cv-frontend-vue
Length of output: 54
Type annotation was removed from availableLocale export.
The explicit type annotation Array<{ title: string; value: string }> is no longer present in v0's version. TypeScript will infer the type as a readonly tuple with literal values instead of a broad Array type, which is technically more precise but changes the public type signature.
The sole consumer (v0/src/components/Navbar/User/UserMenu.vue) passes availableLocale directly to a Vuetify v-select binding and does not rely on the explicit type annotation, so no runtime or type-checking issues occur for current code. However, if external code imports this for type purposes, it may be impacted.
🤖 Prompt for AI Agents
In @v0/src/locales/i18n.ts around lines 33 - 42, The exported availableLocale
constant was changed to a literal tuple type which narrows its public type;
restore the explicit broad type so callers get Array<{ title: string; value:
string }>. Update the availableLocale declaration to include the type annotation
Array<{ title: string; value: string }> (or readonly Array<{ title: string;
value: string }> if immutability is desired) so imports (e.g., in UserMenu.vue)
and external consumers receive the intended public signature; keep the same
values but re-add the type annotation to the exported identifier
availableLocale.
Fix : #727
Standardized Color Formats: Converted all named colors (e.g., white, black) and 3-digit shorthand hex codes (e.g., #fff, #eee) to full 7-character hexadecimal strings (#ffffff, #000000). This ensures compatibility with HTML5 elements which strictly require the #rrggbb format.
Removed Redundant Keys: Deleted duplicate definitions of the "--br-circuit" key in themes.ts. This resolves the [vite] warning: Duplicate key errors encountered during the build process.
Improved Theme Initialization: Refactored the Custom Theme setup to use a spread-operator merge with the Default Theme. This provides a robust fallback mechanism, preventing undefined values if localStorage is empty.
Code Refactoring: Reduced code redundancy in themes.ts, aligning with the project's goal of moving toward contemporary and cleaner code practices.
Summary by CodeRabbit
Style
Refactor
Internationalization
UI
✏️ Tip: You can customize this high-level summary in your review settings.