feat: add lucide-react dependency and implement settings management#18
Open
Debbl wants to merge 2 commits into
Open
feat: add lucide-react dependency and implement settings management#18Debbl wants to merge 2 commits into
Debbl wants to merge 2 commits into
Conversation
- Added lucide-react as a dependency for icon usage in the application. - Introduced settings management functionality, allowing users to import/export settings and profiles. - Updated background script to handle periodic updates for rule lists based on user-defined intervals. - Enhanced the routing structure to include a new general settings page. - Updated permissions in wxt.config.ts to include 'alarms' for scheduling tasks.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis pull request implements automatic rule list updates for a browser extension via a scheduled alarm mechanism. It adds a new settings interface with configuration for update intervals, introduces storage management for persisted settings, and extends the background service to periodically fetch and update rule list profiles while reapplying proxy settings. Changes
Sequence DiagramsequenceDiagram
participant App as Extension Startup
participant BG as Background Service
participant Alarm as Alarms API
participant Storage as Storage
participant Network as Network/URL
participant Proxy as Browser Proxy
App->>BG: Initialize
BG->>BG: setupAlarm()
BG->>Alarm: Set recurring alarm
Note over Alarm: Time interval elapses
Alarm->>BG: onAlarm event
BG->>BG: updateAllRuleLists()
loop For each RuleListProfile with URL
BG->>Storage: Read profile
BG->>Network: Fetch rule list URL
Network-->>BG: Response data
BG->>BG: Preprocess response
BG->>Storage: Store in profile.raw
end
BG->>Storage: Persist updated profiles
BG->>BG: applyCurrentProxy()
BG->>Storage: Get current profile name
BG->>BG: Compute proxy value
BG->>Proxy: Update browser proxy settings
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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 |
![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 (5)
packages/surf-web/src/entrypoints/background.ts (2)
76-80: AwaitsetupAlarm()in the storage change listener.The
setupAlarm()call is not awaited. While this may work in practice, awaiting ensures the alarm is properly configured before any subsequent operations.♻️ Suggested fix
- browser.storage.onChanged.addListener((changes) => { + browser.storage.onChanged.addListener(async (changes) => { if (changes[settingsStoreKey]) { - setupAlarm() + await setupAlarm() } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/surf-web/src/entrypoints/background.ts` around lines 76 - 80, The storage change listener registered with browser.storage.onChanged.addListener currently calls setupAlarm() without awaiting it; update the listener to await the async setupAlarm() call (or return its promise) when changes[settingsStoreKey] is truthy so the alarm is fully configured before proceeding; modify the anonymous listener callback to be async and await setupAlarm() (or return setupAlarm()) to ensure proper sequencing for setupAlarm and any downstream logic that depends on it.
57-65: Consider usingbrowser.alarms.clear(ALARM_NAME)instead ofclearAll().
clearAll()removes all alarms, which could unintentionally clear alarms set by other parts of the extension (if any exist). Usingclear(ALARM_NAME)is more targeted.♻️ Suggested fix
async function setupAlarm() { const settings = await storageSettings.get() - await browser.alarms.clearAll() + await browser.alarms.clear(ALARM_NAME) if (settings.downloadInterval > 0) { browser.alarms.create(ALARM_NAME, { periodInMinutes: settings.downloadInterval, }) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/surf-web/src/entrypoints/background.ts` around lines 57 - 65, The setupAlarm function currently calls browser.alarms.clearAll(), which can remove alarms from other parts of the extension; replace that call with await browser.alarms.clear(ALARM_NAME) so only the specific ALARM_NAME is cleared before (re)creating it. Keep the rest of the logic (reading settings via storageSettings.get(), checking settings.downloadInterval, and creating the alarm with browser.alarms.create(ALARM_NAME, { periodInMinutes: ... })) unchanged.packages/surf-web/src/entrypoints/index/pages/settings/pages/general.tsx (2)
13-18: Consider debouncing to avoid race conditions on rapid input.The handler persists settings on every change. Rapid input could cause multiple concurrent writes with non-deterministic ordering. Consider debouncing the persistence or using an optimistic update pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/surf-web/src/entrypoints/index/pages/settings/pages/general.tsx` around lines 13 - 18, The handler handleIntervalChange currently calls setSettings and then awaits storageSettings.set on every keystroke, which can produce race conditions; wrap or replace the persistence with a debounced write (e.g., debounce storageSettings.set calls by ~300ms) or implement a cancelable pending-write (store a timeout id or promise and clear it on subsequent calls) while still performing the optimistic UI update via setSettings(downloadInterval) immediately; ensure you reference handleIntervalChange, setSettings, and storageSettings.set when adding the debounce/cancel logic so rapid inputs only trigger the final persistence write.
9-11: Add error handling for settings retrieval.If
storageSettings.get()fails, the promise rejection is unhandled. Consider adding error handling to provide feedback or fallback gracefully.🛡️ Suggested improvement
useEffect(() => { - storageSettings.get().then(setSettings) + storageSettings.get().then(setSettings).catch((err) => { + console.error('Failed to load settings:', err) + }) }, [])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/surf-web/src/entrypoints/index/pages/settings/pages/general.tsx` around lines 9 - 11, The useEffect currently calls storageSettings.get().then(setSettings) without handling rejections; update the effect in general.tsx to handle errors from storageSettings.get() (either by using async/await with try/catch or by adding a .catch()) and take remedial action such as logging the error, setting a local error state, and/or applying a safe fallback setting before calling setSettings; ensure you reference the useEffect block and storageSettings.get so the error path is covered and user feedback or default settings are applied when retrieval fails.packages/surf-web/src/icons/index.tsx (1)
3-3: TypeReactElement | ReactNodeis redundant.
ReactElementis already a subset ofReactNode, so the union is equivalent to justReactNode. However,ReactNodeis very broad (includesnull,undefined,string,number,boolean), which may be overly permissive for an icon type.Consider using a more precise type that captures what icons actually return:
♻️ Suggested type refinement
-export type IIcon = (props: SVGProps<SVGSVGElement>) => ReactElement | ReactNode +export type IIcon = (props: SVGProps<SVGSVGElement>) => ReactElementIf lucide-react icons need accommodation, you could use
React.FC<SVGProps<SVGSVGElement>>or create a union type for internal icons and external icon libraries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/surf-web/src/icons/index.tsx` at line 3, The IIcon type currently uses the redundant union ReactElement | ReactNode and is overly permissive; replace it with a more precise signature such as React.FC<SVGProps<SVGSVGElement>> (or a union like React.FC<SVGProps<SVGSVGElement>> | ((props: SVGProps<SVGSVGElement>) => ReactElement) if you must support non-FC patterns) so icons are constrained to return SVG elements; update the export type IIcon and any usages/imports accordingly to use SVGProps<SVGSVGElement> with React.FC to match lucide-react and internal icon shapes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/surf-web/src/entrypoints/background.ts`:
- Around line 31-55: In updateAllRuleLists, prevent hangs and redundant writes
by adding a fetch timeout using AbortController (create controller, pass
controller.signal to fetch, and set a timer to controller.abort after X ms) and
by comparing the processed fetched content to the existing (profiles[key] as
RuleListProfile).raw before mutating: compute const newRaw = preprocess(raw) ??
'' and only assign (profiles[key] as RuleListProfile).raw = newRaw and set
hasChanges = true when newRaw !== existingRaw; keep the existing try/catch
around fetch and ensure the abort error is handled like other fetch errors, then
only call storageProfiles.set(profiles) and applyCurrentProxy() when hasChanges
is true.
In
`@packages/surf-web/src/entrypoints/index/pages/settings/pages/import-and-export.tsx`:
- Around line 29-36: Replace the unsafe any cast by importing the concrete
Profiles type (from surf-pac) and Settings type (from ~/lib/store), type the
importedProfiles as Record<string, Profiles> (or the correct keyed shape) and
the content[settingsStoreKey] as Settings before storing, and add runtime
validation: filter and map Object.entries(content) to only include keys starting
with '+' and validate each profile object shape (and validate settings) before
calling setProfiles(importedProfiles) and await
storageSettings.set(validatedSettings); if validation fails, surface a
user-facing error instead of writing invalid data.
---
Nitpick comments:
In `@packages/surf-web/src/entrypoints/background.ts`:
- Around line 76-80: The storage change listener registered with
browser.storage.onChanged.addListener currently calls setupAlarm() without
awaiting it; update the listener to await the async setupAlarm() call (or return
its promise) when changes[settingsStoreKey] is truthy so the alarm is fully
configured before proceeding; modify the anonymous listener callback to be async
and await setupAlarm() (or return setupAlarm()) to ensure proper sequencing for
setupAlarm and any downstream logic that depends on it.
- Around line 57-65: The setupAlarm function currently calls
browser.alarms.clearAll(), which can remove alarms from other parts of the
extension; replace that call with await browser.alarms.clear(ALARM_NAME) so only
the specific ALARM_NAME is cleared before (re)creating it. Keep the rest of the
logic (reading settings via storageSettings.get(), checking
settings.downloadInterval, and creating the alarm with
browser.alarms.create(ALARM_NAME, { periodInMinutes: ... })) unchanged.
In `@packages/surf-web/src/entrypoints/index/pages/settings/pages/general.tsx`:
- Around line 13-18: The handler handleIntervalChange currently calls
setSettings and then awaits storageSettings.set on every keystroke, which can
produce race conditions; wrap or replace the persistence with a debounced write
(e.g., debounce storageSettings.set calls by ~300ms) or implement a cancelable
pending-write (store a timeout id or promise and clear it on subsequent calls)
while still performing the optimistic UI update via
setSettings(downloadInterval) immediately; ensure you reference
handleIntervalChange, setSettings, and storageSettings.set when adding the
debounce/cancel logic so rapid inputs only trigger the final persistence write.
- Around line 9-11: The useEffect currently calls
storageSettings.get().then(setSettings) without handling rejections; update the
effect in general.tsx to handle errors from storageSettings.get() (either by
using async/await with try/catch or by adding a .catch()) and take remedial
action such as logging the error, setting a local error state, and/or applying a
safe fallback setting before calling setSettings; ensure you reference the
useEffect block and storageSettings.get so the error path is covered and user
feedback or default settings are applied when retrieval fails.
In `@packages/surf-web/src/icons/index.tsx`:
- Line 3: The IIcon type currently uses the redundant union ReactElement |
ReactNode and is overly permissive; replace it with a more precise signature
such as React.FC<SVGProps<SVGSVGElement>> (or a union like
React.FC<SVGProps<SVGSVGElement>> | ((props: SVGProps<SVGSVGElement>) =>
ReactElement) if you must support non-FC patterns) so icons are constrained to
return SVG elements; update the export type IIcon and any usages/imports
accordingly to use SVGProps<SVGSVGElement> with React.FC to match lucide-react
and internal icon shapes.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
packages/surf-web/package.jsonpackages/surf-web/src/entrypoints/background.tspackages/surf-web/src/entrypoints/index/pages/index.tsxpackages/surf-web/src/entrypoints/index/pages/settings/pages/general.tsxpackages/surf-web/src/entrypoints/index/pages/settings/pages/import-and-export.tsxpackages/surf-web/src/entrypoints/index/router/index.tsxpackages/surf-web/src/icons/index.tsxpackages/surf-web/src/lib/store.tspackages/surf-web/wxt.config.ts
Comment on lines
+31
to
+55
| async function updateAllRuleLists() { | ||
| const profiles = await storageProfiles.get() | ||
| let hasChanges = false | ||
|
|
||
| for (const [key, profile] of Object.entries(profiles)) { | ||
| if (profile.profileType === 'RuleListProfile' && profile.url) { | ||
| try { | ||
| const response = await fetch(profile.url) | ||
| const raw = await response.text() | ||
| ;(profiles[key] as RuleListProfile).raw = preprocess(raw) ?? '' | ||
| hasChanges = true | ||
| } catch (e) { | ||
| console.error( | ||
| `[surf-omni] Failed to update rule list: ${profile.name}`, | ||
| e, | ||
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (hasChanges) { | ||
| await storageProfiles.set(profiles) | ||
| await applyCurrentProxy() | ||
| } | ||
| } |
There was a problem hiding this comment.
Add fetch timeout and optimize by comparing content before updating.
Two concerns with updateAllRuleLists:
- No timeout on fetch: Network requests could hang indefinitely.
- Redundant updates:
hasChangesis set true even if fetched content matches existingrawvalue, causing unnecessary storage writes and proxy reapplication.
🛡️ Suggested improvements
async function updateAllRuleLists() {
const profiles = await storageProfiles.get()
let hasChanges = false
for (const [key, profile] of Object.entries(profiles)) {
if (profile.profileType === 'RuleListProfile' && profile.url) {
try {
- const response = await fetch(profile.url)
+ const controller = new AbortController()
+ const timeoutId = setTimeout(() => controller.abort(), 30000) // 30s timeout
+ const response = await fetch(profile.url, { signal: controller.signal })
+ clearTimeout(timeoutId)
const raw = await response.text()
- ;(profiles[key] as RuleListProfile).raw = preprocess(raw) ?? ''
- hasChanges = true
+ const processed = preprocess(raw) ?? ''
+ if ((profiles[key] as RuleListProfile).raw !== processed) {
+ ;(profiles[key] as RuleListProfile).raw = processed
+ hasChanges = true
+ }
} catch (e) {
console.error(
`[surf-omni] Failed to update rule list: ${profile.name}`,
e,
)
}
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/surf-web/src/entrypoints/background.ts` around lines 31 - 55, In
updateAllRuleLists, prevent hangs and redundant writes by adding a fetch timeout
using AbortController (create controller, pass controller.signal to fetch, and
set a timer to controller.abort after X ms) and by comparing the processed
fetched content to the existing (profiles[key] as RuleListProfile).raw before
mutating: compute const newRaw = preprocess(raw) ?? '' and only assign
(profiles[key] as RuleListProfile).raw = newRaw and set hasChanges = true when
newRaw !== existingRaw; keep the existing try/catch around fetch and ensure the
abort error is handled like other fetch errors, then only call
storageProfiles.set(profiles) and applyCurrentProxy() when hasChanges is true.
Comment on lines
+29
to
+36
| const importedProfiles = Object.fromEntries( | ||
| Object.entries(content).filter(([key]) => key.startsWith('+')), | ||
| ) as any | ||
| setProfiles(profiles) | ||
| setProfiles(importedProfiles) | ||
|
|
||
| if (content[settingsStoreKey]) { | ||
| await storageSettings.set(content[settingsStoreKey]) | ||
| } |
There was a problem hiding this comment.
Avoid as any type assertion and add import validation.
The as any cast on line 31 violates the coding guideline to avoid any type. Additionally, the imported settings are not validated before being stored.
🛡️ Proposed fix with proper typing and validation
- const importedProfiles = Object.fromEntries(
+ const importedProfiles: Profiles = Object.fromEntries(
Object.entries(content).filter(([key]) => key.startsWith('+')),
- ) as any
+ ) as Profiles
setProfiles(importedProfiles)
- if (content[settingsStoreKey]) {
- await storageSettings.set(content[settingsStoreKey])
+ if (content[settingsStoreKey] && typeof content[settingsStoreKey].downloadInterval === 'number') {
+ await storageSettings.set(content[settingsStoreKey] as Settings)
}You'll need to import the Profiles type from surf-pac and Settings from ~/lib/store.
As per coding guidelines: "Use TypeScript strict mode and avoid any type - use precise types instead".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/surf-web/src/entrypoints/index/pages/settings/pages/import-and-export.tsx`
around lines 29 - 36, Replace the unsafe any cast by importing the concrete
Profiles type (from surf-pac) and Settings type (from ~/lib/store), type the
importedProfiles as Record<string, Profiles> (or the correct keyed shape) and
the content[settingsStoreKey] as Settings before storing, and add runtime
validation: filter and map Object.entries(content) to only include keys starting
with '+' and validate each profile object shape (and validate settings) before
calling setProfiles(importedProfiles) and await
storageSettings.set(validatedSettings); if validation fails, surface a
user-facing error instead of writing invalid data.
- Introduced SlidersHorizontal icon from lucide-react to the icon library. - Updated index.tsx to include the new icon in the imports for better accessibility in the application.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes