3.13.3: QoL #76
3.13.3: QoL #76
Conversation
WalkthroughThis pull request introduces a comprehensive update to a browser extension, primarily focusing on the Magister API implementation, manifest files, and various UI components. The changes include version updates to 3.13.3, modifications to content scripts, enhanced theme management, and a significant refactoring of the API structure from procedural to class-based design. The update expands the extension's functionality to support a new domain (study-tools.nl) and improves overall code organisation and user interaction. Changes
Sequence DiagramsequenceDiagram
participant User
participant Extension
participant MagisterAPI
participant StudyToolsWebsite
User->>Extension: Install/Update
Extension->>Extension: Initialize API
Extension->>MagisterAPI: Authenticate
MagisterAPI-->>Extension: Provide Credentials
Extension->>StudyToolsWebsite: Add Content Scripts
StudyToolsWebsite-->>Extension: Enable Enhanced Functionality
Extension->>User: Ready to Use
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
src/scripts/grades.js (1)
Line range hint
942-951: Improve animation state management.The animation state management could be improved to be more robust and maintainable.
Consider extracting the animation logic into a dedicated function:
-widgetItemsContainer.dataset.navigate = 'still' -setTimeout(() => { - widgetItemsContainer.dataset.navigate = targetIndex > visibleChildIndex ? 'forwards' : targetIndex < visibleChildIndex ? 'backwards' : 'still' - visibleChildIndex = targetIndex - setTimeout(() => { - children.forEach((child, index) => child.style.display = index === targetIndex ? 'flex' : 'none'); - document.querySelectorAll('#st-start-widget-grades-scroll-pagn>div').forEach((d, index) => d.dataset.current = index === targetIndex); - widgetElement.dataset.unread = children[targetIndex]?.dataset.unread || false - }, 60); -}, 10); +function animateWidgetTransition(targetIndex) { + widgetItemsContainer.dataset.navigate = 'still'; + + requestAnimationFrame(() => { + const direction = targetIndex > visibleChildIndex ? 'forwards' : + targetIndex < visibleChildIndex ? 'backwards' : 'still'; + widgetItemsContainer.dataset.navigate = direction; + visibleChildIndex = targetIndex; + + setTimeout(() => { + children.forEach((child, index) => { + child.style.display = index === targetIndex ? 'flex' : 'none'; + }); + document.querySelectorAll('#st-start-widget-grades-scroll-pagn>div') + .forEach((d, index) => d.dataset.current = index === targetIndex); + widgetElement.dataset.unread = children[targetIndex]?.dataset.unread || false; + }, 60); + }); +}src/scripts/today.js (2)
218-226: Improve animation timing and state management.The animation state management could be more robust by using CSS custom properties for timing values.
Consider this approach:
+// Define animation timings as CSS custom properties +:root { + --st-animation-duration: 40ms; + --st-animation-delay: 10ms; +} -schedule.dataset.navigate = 'still' -setTimeout(() => schedule.dataset.navigate = 'forwards', 10) -setTimeout(renderSchedule, 40) +function animateSchedule(direction) { + schedule.dataset.navigate = 'still'; + requestAnimationFrame(() => { + schedule.dataset.navigate = direction; + setTimeout(renderSchedule, + parseFloat(getComputedStyle(document.documentElement) + .getPropertyValue('--st-animation-duration'))); + }); +}Also applies to: 486-486
🧰 Tools
🪛 Biome (1.9.4)
[error] 221-222: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 225-226: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
715-720: Improve sample data handling in API calls.The sample data handling could be improved by using a more structured approach.
Consider using a dedicated context manager:
class SampleDataContext { constructor(api) { this.api = api; } async with(callback) { this.api.useSampleData = true; try { return await callback(); } finally { this.api.useSampleData = false; } } } // Usage: const sampleData = new SampleDataContext(magisterApi); await sampleData.with(async () => { return await magisterApi.logs(); });Also applies to: 742-747, 807-816, 1107-1112
🧰 Tools
🪛 Biome (1.9.4)
[error] 715-736: Promise executor functions should not be
async.(lint/suspicious/noAsyncPromiseExecutor)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
popup/dist/assets/index-BUGULQ7V.jsis excluded by!**/dist/**popup/dist/assets/index-CQN93ZlL.cssis excluded by!**/dist/**popup/dist/assets/index-CzqbSv-Z.jsis excluded by!**/dist/**popup/dist/index.htmlis excluded by!**/dist/**popup/dist/themePresets.jsis excluded by!**/dist/**popup/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
manifest-firefox.json(3 hunks)manifest.json(4 hunks)popup/public/themePresets.js(1 hunks)popup/src/App.vue(1 hunks)popup/src/components/About.vue(1 hunks)popup/src/components/settings-views/ThemeView.vue(2 hunks)src/background.js(1 hunks)src/scripts/api.js(1 hunks)src/scripts/gamification.js(3 hunks)src/scripts/grades.js(10 hunks)src/scripts/login.js(1 hunks)src/scripts/main.js(1 hunks)src/scripts/style.js(2 hunks)src/scripts/theme-store.js(1 hunks)src/scripts/today.js(19 hunks)src/scripts/util.js(3 hunks)src/service-worker.js(1 hunks)src/styles/main.css(2 hunks)src/styles/today.css(9 hunks)
✅ Files skipped from review due to trivial changes (1)
- popup/src/components/settings-views/ThemeView.vue
🧰 Additional context used
🪛 Biome (1.9.4)
src/service-worker.js
[error] 130-130: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 131-131: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
src/background.js
[error] 102-102: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 103-103: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
src/scripts/today.js
[error] 221-222: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 225-226: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 715-736: Promise executor functions should not be async.
(lint/suspicious/noAsyncPromiseExecutor)
[error] 742-763: Promise executor functions should not be async.
(lint/suspicious/noAsyncPromiseExecutor)
[error] 948-948: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 949-949: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 1107-1154: Promise executor functions should not be async.
(lint/suspicious/noAsyncPromiseExecutor)
src/scripts/api.js
[error] 45-48: Promise executor functions should not be async.
(lint/suspicious/noAsyncPromiseExecutor)
[error] 127-140: Promise executor functions should not be async.
(lint/suspicious/noAsyncPromiseExecutor)
[error] 144-167: Promise executor functions should not be async.
(lint/suspicious/noAsyncPromiseExecutor)
[error] 172-192: Promise executor functions should not be async.
(lint/suspicious/noAsyncPromiseExecutor)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (32)
src/scripts/theme-store.js (1)
3-8: Updated 'popstate' function looks goodThe simplification of the
popstatefunction and removal of unnecessary event listeners improve code clarity and performance. No issues found.src/scripts/login.js (1)
1-38: 'login' function modifications are acceptableThe changes to the
loginfunction enhance user feedback via thefooterNoticeelement. Asynchronous operations and event handling are correctly implemented.popup/src/components/About.vue (1)
51-51: LGTM! Version number formatting looks good.The version number display logic has been enhanced to:
- Safely handle undefined manifest versions
- Transform four-segment version numbers (e.g., "3.13.3.1") into beta format (e.g., "3.13.3-beta1")
- Maintain the fallback to "onbekend"
Let's verify the version number format in the manifest files:
✅ Verification successful
Version number formatting implementation is correct and matches manifest versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check version numbers in manifest files echo "Checking manifest files for version numbers..." rg '"version":\s*"[^"]*"' manifest*.jsonLength of output: 252
src/background.js (1)
85-93: LGTM! Tab creation logic is correct.The implementation properly handles opening the options page in a new tab while passing URL parameters.
src/service-worker.js (1)
118-121: LGTM! Tab creation logic is correct.The implementation properly handles opening the options page in a new tab while passing URL parameters.
popup/src/App.vue (1)
29-32: LGTM! Tab parameter handling is implemented correctly.The implementation properly:
- Updates the selected category based on the URL parameter
- Cleans up by resetting the parameter after use
src/scripts/main.js (1)
89-89: LGTM! API instance usage is consistent.The change from
MagisterApi.accountInfo()tomagisterApi.accountInfo()aligns with the refactoring of the API from static class methods to instance methods.Let's verify that all API calls in the file use the instance:
✅ Verification successful
Verification successful: API instance usage is consistent
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining static MagisterApi calls rg "MagisterApi\." src/scripts/main.jsLength of output: 40
Script:
#!/bin/bash # Verify file exists and show all MagisterApi related patterns echo "=== Checking if file exists ===" fd "main.js" src/scripts echo -e "\n=== Checking all MagisterApi references ===" rg -i "magisterapi" src/scripts/main.js echo -e "\n=== Getting context around API usage ===" rg -B 2 -A 2 "magisterApi\." src/scripts/main.jsLength of output: 731
popup/public/themePresets.js (1)
109-109: LGTM! Theme CSS improvements.The CSS updates for the "Bliss" theme enhance the visual presentation with better z-indexing, background sizing, and layout properties.
src/scripts/gamification.js (1)
Line range hint
184-198: LGTM! API instance usage is consistent.All API calls have been correctly updated to use instance methods instead of static class methods.
Let's verify that all API calls in the file use the instance:
✅ Verification successful
Verification successful: All API calls use instance methods
All API calls in
src/scripts/gamification.jsconsistently use the instance patternmagisterApi.with no static method calls present.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining static MagisterApi calls rg "MagisterApi\." src/scripts/gamification.jsLength of output: 48
Script:
#!/bin/bash # Verify file existence and content echo "=== File check ===" ls -l src/scripts/gamification.js echo -e "\n=== Instance API calls ===" rg "magisterApi\." src/scripts/gamification.js echo -e "\n=== Any API-like calls ===" rg -i "api\." src/scripts/gamification.jsLength of output: 2099
🧰 Tools
🪛 Biome (1.9.4)
[error] 187-187: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
src/scripts/util.js (3)
236-240: Improved midnight function implementation.The function has been enhanced to:
- Accept a targetDate parameter for flexibility
- Set time to 00:00:00 for more accurate date comparisons
- Use function declaration for better hoisting
261-263: Updated date comparison methods.The date prototype methods now use the improved midnight function for more accurate date comparisons.
579-582: Enhanced button callback handling.The notify function now supports both
callbackandonclickproperties for buttons, providing more flexibility in event handling.src/scripts/grades.js (3)
134-134: LGTM! API call updated to use the new instance.The change from
MagisterApi.years()tomagisterApi.years()correctly reflects the shift to instance-based API access.
156-156: LGTM! API calls updated consistently.The changes to use
magisterApi.gradesForYear()andmagisterApi.gradesColumnInfo()maintain consistency with the new API structure.Also applies to: 252-252
Line range hint
866-875: LGTM! Grade statistics API calls updated.The changes correctly update the API calls while maintaining the filtering and sorting logic for grade statistics.
src/scripts/style.js (2)
1689-1765: LGTM! Well-structured container layout.The new app container layout uses modern CSS features effectively:
- Flexbox for layout control
- Container queries for responsive design
- Logical properties for better i18n support
1787-1794: LGTM! Consistent heading styles.The heading styles maintain consistency with the design system using CSS custom properties.
src/scripts/today.js (1)
Line range hint
104-128: LGTM! Enhanced greeting system.The greeting system improvements add more variety and personality to the user experience.
manifest.json (4)
5-5: LGTM! Version bump to 3.13.3.The version number has been updated following semantic versioning.
29-30: LGTM! Correct dependency order.The order change ensures that utility functions are available before API initialization.
81-89: LGTM! New content script for study-tools.nl.The new content script correctly includes the required utilities and theme store for the study tools domain.
103-104: LGTM! Updated web accessible resources.The web accessible resources have been correctly updated to include the study tools domain.
manifest-firefox.json (3)
5-5: Version bump looks good.The version has been updated to 3.13.3, following semantic versioning.
87-95: New content script configuration for study-tools.nl domain.The content script configuration is properly structured with the required scripts:
- util.js for utility functions
- theme-store.js for theme management
109-110: Web accessible resources updated correctly.The web_accessible_resources section has been updated to include the study-tools.nl domain alongside magister.net.
src/styles/main.css (3)
306-306: Improved focus visibility for buttons and dropdowns.The outline color has been updated to use the accent color for better visibility when elements are focused.
738-738: Enhanced focus state for metric links.The outline color for metric links has been updated to match the new accent color scheme.
Line range hint
908-961: Enhanced widget editing interactions.Good improvements to the widget editing experience:
- Clear visual feedback with opacity and blur effects
- Proper cursor states for dragging
- Smooth transitions for focus states
- Accessible outline colours
src/styles/today.css (4)
21-21: Consistent focus outline for list items and events.The outline color has been updated to match the accent color scheme used in main.css.
154-154: Well-structured animation properties.Good addition of scale modifier and navigation animations:
- Clear naming convention for animation states
- Proper scale variable for consistent animations
Also applies to: 158-169
172-200: Comprehensive navigation animations.Well-implemented keyframes for different navigation states:
- Backwards movement
- Forwards movement
- Jump forwards
- Consistent use of scale modifier
710-722: Smooth widget visibility transitions.Good implementation of widget state transitions:
- Proper use of opacity and translate
- Clean starting styles
- Correct display handling
| return new Promise(async (resolve, reject) => { | ||
| magisterApiCache.logs ??= | ||
| fetchWrapper( | ||
| `https://${magisterApiSchoolName}.magister.net/api/leerlingen/$USERID/logboeken/count`, null, 'logs' | ||
| ) | ||
| resolve( | ||
| Array((await magisterApiCache.logs).count || 0) || [] | ||
| ) | ||
| }) | ||
| }, | ||
| absences: { | ||
| forYear: async (year) => { | ||
| return new Promise(async (resolve, reject) => { | ||
| magisterApiCache['absencesYear' + year?.id] ??= | ||
| fetchWrapper( | ||
| `https://${magisterApiSchoolName}.magister.net/api/personen/$USERID/absenties?van=${year.begin}&tot=${year.einde}`, null, 'absences' | ||
| ) | ||
| resolve( | ||
| (await magisterApiCache['absencesYear' + year?.id])?.Items || [] | ||
| ) | ||
| }) | ||
| } | ||
| }, | ||
| if (!magisterApi.userId || !magisterApi.userToken) { | ||
| await magisterApi.updateApiCredentials() | ||
| .catch(err => console.error(err)); | ||
| } | ||
| let res = await fetch(url.replace(/(\$USERID)/gi, magisterApi.userId), { | ||
| headers: { | ||
| Authorization: magisterApi.userToken, | ||
| 'X-Request-Source': 'study-tools' | ||
| }, | ||
| ...options | ||
| }); | ||
| if (res.ok) { | ||
| let json = await res.json(); | ||
| console.info(`APIRQ OK after ${new Date() - calledAt} ms (@ ${this.identifier})`); | ||
| resolve(json); | ||
| } else { | ||
| console.info(`APIRQ ERR: ${res.status}.`); | ||
| reject(res); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Remove 'async' from Promise executor functions
Including async in a Promise executor can lead to errors not being properly caught. Removing the async ensures proper error propagation.
Apply this diff to fix the issue:
- return new Promise(async (resolve, reject) => {
+ return new Promise((resolve, reject) => {Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 172-192: Promise executor functions should not be async.
(lint/suspicious/noAsyncPromiseExecutor)
| this.permissions = (await new MagisterApiRequestAccount().get())?.Groep?.[0]?.Privileges?.filter(p => p.AccessType.includes('Read')).map(p => p.Naam); | ||
| resolve(this.permissions); | ||
| }); |
There was a problem hiding this comment.
Remove 'async' from Promise executor functions
Using an async function as the executor of a Promise can lead to unexpected behaviour, as errors may not be caught properly. It's recommended to remove the async keyword from the executor function.
Apply this diff to fix the issue:
- return new Promise(async (resolve) => {
+ return new Promise((resolve) => {Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 45-48: Promise executor functions should not be async.
(lint/suspicious/noAsyncPromiseExecutor)
| return new Promise(async (resolve, reject) => { | ||
| magisterApiCache.messages ??= | ||
| fetchWrapper( | ||
| `https://${magisterApiSchoolName}.magister.net/api/berichten/postvakin/berichten?top=20&skip=0&gelezenStatus=ongelezen`, null, 'messages' | ||
| ) | ||
| resolve( | ||
| (await magisterApiCache.messages)?.items || [] | ||
| ) | ||
| }) | ||
| }, | ||
| activities: async () => { | ||
| if (MagisterApi.useSampleData) { | ||
| return [null] | ||
| } | ||
| if (magisterApi.useSampleData && this.sample) { | ||
| resolve(this.sample); | ||
| } else if (!this.identifier || !this.path) { | ||
| reject(); | ||
| } else if (magisterApi.cache[this.identifier] && !magisterApi.cache[this.identifier].then) { | ||
| resolve(magisterApi.cache[this.identifier]); | ||
| } else { | ||
| let res = await this.#fetchWrapper( | ||
| `https://${magisterApi.schoolName}.magister.net/${this.path}`, | ||
| ); | ||
| resolve(res); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Remove 'async' from Promise executor functions
Using an async function inside a Promise constructor is unnecessary and can cause issues with error handling. Removing the async keyword will ensure the Promise behaves as expected.
Apply this diff to fix the issue:
- return new Promise(async (resolve, reject) => {
+ return new Promise((resolve, reject) => {Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 127-140: Promise executor functions should not be async.
(lint/suspicious/noAsyncPromiseExecutor)
| return new Promise(async (resolve, reject) => { | ||
| magisterApiCache.activities ??= | ||
| fetchWrapper( | ||
| `https://${magisterApiSchoolName}.magister.net/api/personen/$USERID/activiteiten?status=NogNietAanEisVoldaan`, null, 'activities' | ||
| ) | ||
| resolve( | ||
| (await magisterApiCache.activities)?.Items || [] | ||
| ) | ||
| }) | ||
| }, | ||
| logs: async () => { | ||
| if (MagisterApi.useSampleData) { | ||
| return [null] | ||
| } | ||
| if (!magisterApi.userId || !magisterApi.userToken) { | ||
| await magisterApi.updateApiCredentials() | ||
| .catch(err => console.error(err)); | ||
| } | ||
|
|
||
| try { | ||
| let res = await this.#executeRequest(url, options); | ||
| resolve(this.#formatOutput(res)); | ||
| magisterApi.cache[this.identifier] = this.#formatOutput(res); | ||
| } catch (error) { | ||
| console.error(error); | ||
| try { | ||
| await new Promise(resolve => setTimeout(resolve, 100)); | ||
| await magisterApi.updateApiCredentials(); | ||
| let res2 = await this.#executeRequest(url, options); | ||
| resolve(this.#formatOutput(res2)); | ||
| magisterApi.cache[this.identifier] = this.#formatOutput(res2); | ||
| } catch (error) { | ||
| console.error(error); | ||
| reject(error); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Remove 'async' from Promise executor functions
An async executor in a Promise can result in unhandled exceptions. It's best to remove the async keyword to prevent potential issues.
Apply this diff to fix the issue:
- return new Promise(async (resolve, reject) => {
+ return new Promise((resolve, reject) => {Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 144-167: Promise executor functions should not be async.
(lint/suspicious/noAsyncPromiseExecutor)
src/background.js
Outdated
| browser.runtime.onMessageExternal.addListener(async (request, sender, sendResponse) => { | ||
| switch (request.action) { | ||
| case 'addPersonalTheme': | ||
| const obj = request.obj | ||
| const storedThemes = Object.values((await chrome.storage.local.get('storedThemes')).storedThemes) | ||
| if (!storedThemes || storedThemes.length >= 9) return | ||
|
|
||
| storedThemes.push(obj) | ||
|
|
||
| //TODO: only if not exist | ||
|
|
||
| await chrome.storage.local.set({ 'storedThemes': storedThemes }) | ||
| break | ||
|
|
||
| default: | ||
| return 0 | ||
| } | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance theme management with duplicate checking and error handling.
The external message listener for theme management needs improvements:
- The TODO comment indicates missing duplicate checking
- Variables are declared within the switch case without a block scope
- No error handling for storage operations
Apply this diff to improve the implementation:
chrome.runtime.onMessageExternal.addListener(async (request, sender, sendResponse) => {
switch (request.action) {
case 'addPersonalTheme': {
- const obj = request.obj
- const storedThemes = Object.values((await chrome.storage.local.get('storedThemes')).storedThemes)
- if (!storedThemes || storedThemes.length >= 9) return
+ try {
+ const obj = request.obj
+ const storedThemes = Object.values((await chrome.storage.local.get('storedThemes')).storedThemes || [])
+ if (storedThemes.length >= 9) {
+ sendResponse({ success: false, error: 'Maximum themes limit reached' })
+ return
+ }
- storedThemes.push(obj)
+ // Check for duplicates
+ if (storedThemes.some(theme => theme.id === obj.id)) {
+ sendResponse({ success: false, error: 'Theme already exists' })
+ return
+ }
- //TODO: only if not exist
+ storedThemes.push(obj)
+ await chrome.storage.local.set({ 'storedThemes': storedThemes })
+ sendResponse({ success: true })
+ } catch (error) {
+ console.error('Failed to add personal theme:', error)
+ sendResponse({ success: false, error: error.message })
+ }
+ break
+ }
- await chrome.storage.local.set({ 'storedThemes': storedThemes })
- break📝 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.
| browser.runtime.onMessageExternal.addListener(async (request, sender, sendResponse) => { | |
| switch (request.action) { | |
| case 'addPersonalTheme': | |
| const obj = request.obj | |
| const storedThemes = Object.values((await chrome.storage.local.get('storedThemes')).storedThemes) | |
| if (!storedThemes || storedThemes.length >= 9) return | |
| storedThemes.push(obj) | |
| //TODO: only if not exist | |
| await chrome.storage.local.set({ 'storedThemes': storedThemes }) | |
| break | |
| default: | |
| return 0 | |
| } | |
| }) | |
| browser.runtime.onMessageExternal.addListener(async (request, sender, sendResponse) => { | |
| switch (request.action) { | |
| case 'addPersonalTheme': { | |
| try { | |
| const obj = request.obj | |
| const storedThemes = Object.values((await chrome.storage.local.get('storedThemes')).storedThemes || []) | |
| if (storedThemes.length >= 9) { | |
| sendResponse({ success: false, error: 'Maximum themes limit reached' }) | |
| return | |
| } | |
| // Check for duplicates | |
| if (storedThemes.some(theme => theme.id === obj.id)) { | |
| sendResponse({ success: false, error: 'Theme already exists' }) | |
| return | |
| } | |
| storedThemes.push(obj) | |
| await chrome.storage.local.set({ 'storedThemes': storedThemes }) | |
| sendResponse({ success: true }) | |
| } catch (error) { | |
| console.error('Failed to add personal theme:', error) | |
| sendResponse({ success: false, error: error.message }) | |
| } | |
| break | |
| } | |
| default: | |
| return 0 | |
| } | |
| }) |
🧰 Tools
🪛 Biome (1.9.4)
[error] 102-102: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 103-103: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
src/service-worker.js
Outdated
| chrome.runtime.onMessageExternal.addListener(async (request, sender, sendResponse) => { | ||
| switch (request.action) { | ||
| case 'addPersonalTheme': | ||
| const obj = request.obj | ||
| const storedThemes = Object.values((await chrome.storage.local.get('storedThemes')).storedThemes) | ||
| if (!storedThemes || storedThemes.length >= 9) return | ||
|
|
||
| storedThemes.push(obj) | ||
|
|
||
| //TODO: only if not exist | ||
|
|
||
| await chrome.storage.local.set({ 'storedThemes': storedThemes }) | ||
| break | ||
|
|
||
| default: | ||
| return 0 | ||
| } | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance theme management with duplicate checking and error handling.
The external message listener for theme management needs the same improvements as in background.js:
- The TODO comment indicates missing duplicate checking
- Variables are declared within the switch case without a block scope
- No error handling for storage operations
Apply the same improvements as suggested for background.js, but maintain the Chrome API usage:
chrome.runtime.onMessageExternal.addListener(async (request, sender, sendResponse) => {
switch (request.action) {
case 'addPersonalTheme': {
- const obj = request.obj
- const storedThemes = Object.values((await chrome.storage.local.get('storedThemes')).storedThemes)
- if (!storedThemes || storedThemes.length >= 9) return
+ try {
+ const obj = request.obj
+ const storedThemes = Object.values((await chrome.storage.local.get('storedThemes')).storedThemes || [])
+ if (storedThemes.length >= 9) {
+ sendResponse({ success: false, error: 'Maximum themes limit reached' })
+ return
+ }
- storedThemes.push(obj)
+ // Check for duplicates
+ if (storedThemes.some(theme => theme.id === obj.id)) {
+ sendResponse({ success: false, error: 'Theme already exists' })
+ return
+ }
- //TODO: only if not exist
+ storedThemes.push(obj)
+ await chrome.storage.local.set({ 'storedThemes': storedThemes })
+ sendResponse({ success: true })
+ } catch (error) {
+ console.error('Failed to add personal theme:', error)
+ sendResponse({ success: false, error: error.message })
+ }
+ break
+ }
- await chrome.storage.local.set({ 'storedThemes': storedThemes })
- break📝 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.
| chrome.runtime.onMessageExternal.addListener(async (request, sender, sendResponse) => { | |
| switch (request.action) { | |
| case 'addPersonalTheme': | |
| const obj = request.obj | |
| const storedThemes = Object.values((await chrome.storage.local.get('storedThemes')).storedThemes) | |
| if (!storedThemes || storedThemes.length >= 9) return | |
| storedThemes.push(obj) | |
| //TODO: only if not exist | |
| await chrome.storage.local.set({ 'storedThemes': storedThemes }) | |
| break | |
| default: | |
| return 0 | |
| } | |
| }) | |
| chrome.runtime.onMessageExternal.addListener(async (request, sender, sendResponse) => { | |
| switch (request.action) { | |
| case 'addPersonalTheme': { | |
| try { | |
| const obj = request.obj | |
| const storedThemes = Object.values((await chrome.storage.local.get('storedThemes')).storedThemes || []) | |
| if (storedThemes.length >= 9) { | |
| sendResponse({ success: false, error: 'Maximum themes limit reached' }) | |
| return | |
| } | |
| // Check for duplicates | |
| if (storedThemes.some(theme => theme.id === obj.id)) { | |
| sendResponse({ success: false, error: 'Theme already exists' }) | |
| return | |
| } | |
| storedThemes.push(obj) | |
| await chrome.storage.local.set({ 'storedThemes': storedThemes }) | |
| sendResponse({ success: true }) | |
| } catch (error) { | |
| console.error('Failed to add personal theme:', error) | |
| sendResponse({ success: false, error: error.message }) | |
| } | |
| break | |
| } | |
| default: | |
| return 0 | |
| } | |
| }) |
🧰 Tools
🪛 Biome (1.9.4)
[error] 130-130: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 131-131: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
Summary by CodeRabbit
Release Notes v3.13.3
New Features
Improvements
Bug Fixes
Performance