feat: Have synced URLS with normal expressions#776
Open
Crunging wants to merge 12 commits intoViren070:mainfrom
Open
feat: Have synced URLS with normal expressions#776Crunging wants to merge 12 commits intoViren070:mainfrom
Crunging wants to merge 12 commits intoViren070:mainfrom
Conversation
Contributor
WalkthroughAdds placeholder-driven synced-URL support: legacy synced lists are migrated into Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as Frontend UI
participant Config as ConfigModal / Migration
participant Core as Core (regex/sel)
participant Remote as Remote URL
User->>UI: Open config / Edit filters
UI->>Config: Request user config
Config->>Core: Fetch raw userData
Core-->>Config: raw userData
Config->>Config: applyMigrations(raw userData)
Config-->>UI: migrated config (with <SYNCED: URL> placeholders)
UI->>UI: parseSyncedUrl() / show placeholders
User->>UI: Add synced URL
UI->>Remote: Fetch patterns/expressions for URL
Remote-->>Core: supply remote patterns
Core->>Core: resolve transforms & overrides per-URL
Core-->>UI: expanded patterns/expressions
UI->>UI: replace placeholder with resolved items
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
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}
Contributor
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 (1)
packages/frontend/src/components/menu/filters/_components/synced-patterns.tsx (1)
787-803:⚠️ Potential issue | 🟠 MajorFix duplicate detection fallback for URL adds.
Line 787 initialises
knownUrlsto[]whenexistingUrlsis not provided, so Line 801 can allow duplicates already present insyncConfig.urls. This reintroduces duplicate URL entries and repeated fetches.Suggested patch
- const knownUrls = existingUrls || []; + const knownUrls = Array.from( + new Set([...(existingUrls ?? []), ...urls]) + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/components/menu/filters/_components/synced-patterns.tsx` around lines 787 - 803, The duplicate-detection uses knownUrls = existingUrls || [] which misses URLs already in syncConfig.urls and allows duplicates; update the logic that builds knownUrls (used by validateAndAdd) to include both existingUrls and syncConfig.urls (or merge them into a Set) so the includes check in validateAndAdd detects duplicates against the full current source of truth (existingUrls and syncConfig.urls); modify the code around the knownUrls initialization and ensure validateAndAdd still references that combined collection to prevent re-adding URLs.
🧹 Nitpick comments (1)
packages/core/src/utils/regex-access.ts (1)
270-297: Cache per-URL sync results within this method call.If the same
<SYNCED: ...>marker appears multiple times, the same URL is resolved repeatedly. A small per-call cache keeps ordering behaviour intact while avoiding repeated fetch/transform work.♻️ Proposed refactor
+ const processedByUrl = new Map<string, Promise<U[]>>(); + // Helper to process and transform patterns from a URL const processPatterns = async (url: string): Promise<U[]> => { - const patterns = await this.resolvePatterns([url], userData); - const syncedItems: U[] = []; - for (const regex of patterns) { - const override = overrides.find( - (o) => - o.pattern === regex.pattern || - (regex.name && o.originalName === regex.name) - ); - - if (override?.disabled) continue; - - const item = transform( - override - ? { - ...regex, - name: override.name ?? regex.name, - score: - override.score !== undefined ? override.score : regex.score, - } - : regex - ); - - syncedItems.push(item); - } - return syncedItems; + const cached = processedByUrl.get(url); + if (cached) return [...(await cached)]; + + const run = (async (): Promise<U[]> => { + const patterns = await this.resolvePatterns([url], userData); + const syncedItems: U[] = []; + for (const regex of patterns) { + const override = overrides.find( + (o) => + o.pattern === regex.pattern || + (regex.name && o.originalName === regex.name) + ); + + if (override?.disabled) continue; + + const item = transform( + override + ? { + ...regex, + name: override.name ?? regex.name, + score: + override.score !== undefined ? override.score : regex.score, + } + : regex + ); + syncedItems.push(item); + } + return syncedItems; + })(); + + processedByUrl.set(url, run); + return [...(await run)]; };Also applies to: 304-329
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/utils/regex-access.ts` around lines 270 - 297, processPatterns currently calls this.resolvePatterns([url], ...) for each URL occurrence which can re-resolve the same "<SYNCED:...>" marker multiple times; add a small per-call cache (e.g., a Map keyed by the raw URL string) inside processPatterns to store and return the resolved patterns for a URL on subsequent requests within the same invocation, use the cached patterns instead of calling resolvePatterns again, and then proceed with the existing override lookup/transform/push logic (affecting the processPatterns function and similarly the analogous block around lines 304-329).
🤖 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/frontend/src/components/menu/filters/_components/filter-inputs.tsx`:
- Around line 317-318: The current guard uses newValue.includes('<SYNCED') which
blocks any input containing that substring; replace it with a strict validation
that only short-circuits when the entire token is a valid synced tag (e.g.,
match with a regex like /^\s*<SYNCED:\s*[^>]+>\s*$/) before returning, then call
handleValueChange(newValue, index) as before; apply this same change to every
occurrence of the includes check (the conditional guarding newValue before
handleValueChange at the shown locations).
In
`@packages/frontend/src/components/menu/filters/_components/synced-patterns.tsx`:
- Around line 677-687: The add-UI currently becomes a silent no-op when
onUrlAdded is not provided; change the logic so the add form is only shown when
a real callback exists by deriving a flag like showAddForm = !hideAddForm &&
!!onUrlAdded and use that flag to conditionally render the add UI instead of
relying on optional chaining; also update the add handlers (the places calling
onUrlAdded?.(...) in this component) to assume onUrlAdded exists when the form
is shown (or guard with an assertion) so you no longer present a clickable UI
that does nothing.
In `@packages/frontend/src/context/userData.tsx`:
- Around line 261-262: The migration currently deduplicates by name (using the
accessor v => v.name); change it to deduplicate by pattern instead by using v =>
v.pattern when calling migrateSyncedUrls for 'syncedPreferredRegexUrls' ->
'preferredRegexPatterns', keeping the creation mapping (url => ({ name:
syncedTag(url), pattern: syncedTag(url) })) unchanged so migrated entries retain
the same pattern and display name but are de-duplicated by pattern to make the
migration idempotent.
---
Outside diff comments:
In
`@packages/frontend/src/components/menu/filters/_components/synced-patterns.tsx`:
- Around line 787-803: The duplicate-detection uses knownUrls = existingUrls ||
[] which misses URLs already in syncConfig.urls and allows duplicates; update
the logic that builds knownUrls (used by validateAndAdd) to include both
existingUrls and syncConfig.urls (or merge them into a Set) so the includes
check in validateAndAdd detects duplicates against the full current source of
truth (existingUrls and syncConfig.urls); modify the code around the knownUrls
initialization and ensure validateAndAdd still references that combined
collection to prevent re-adding URLs.
---
Nitpick comments:
In `@packages/core/src/utils/regex-access.ts`:
- Around line 270-297: processPatterns currently calls
this.resolvePatterns([url], ...) for each URL occurrence which can re-resolve
the same "<SYNCED:...>" marker multiple times; add a small per-call cache (e.g.,
a Map keyed by the raw URL string) inside processPatterns to store and return
the resolved patterns for a URL on subsequent requests within the same
invocation, use the cached patterns instead of calling resolvePatterns again,
and then proceed with the existing override lookup/transform/push logic
(affecting the processPatterns function and similarly the analogous block around
lines 304-329).
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/core/src/utils/config.tspackages/core/src/utils/regex-access.tspackages/core/src/utils/sel-access.tspackages/frontend/src/components/config-modal.tsxpackages/frontend/src/components/menu/filters/_components/filter-inputs.tsxpackages/frontend/src/components/menu/filters/_components/synced-patterns.tsxpackages/frontend/src/components/menu/filters/index.tsxpackages/frontend/src/context/userData.tsx
packages/frontend/src/components/menu/filters/_components/filter-inputs.tsx
Show resolved
Hide resolved
packages/frontend/src/components/menu/filters/_components/synced-patterns.tsx
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/frontend/src/context/userData.tsx`:
- Around line 244-249: The migration assumes (config as any)[legacyKey] is an
array of strings and (config as any)[valuesKey] is an array — add defensive
guards before using them: check Array.isArray(urls) and that each element is a
string (skip non-strings) and ensure values is an array (if not, initialize to
[]), then iterate only the validated urls and use getKey(safeValue) /
syncedTag(url) as before; update the variables used by createItem, getKey,
syncedTag and ensure this logic lives inside the same migration block
(applyMigrations) so malformed saved configs are skipped rather than causing
.some or .push to throw.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/frontend/src/context/userData.tsx`:
- Around line 244-249: The migration loop may call getKey(v) on null/undefined
entries and throw, so update the dedupe check in the loop (the values array
handling around variables values, getKey, syncedTag, createItem) to defensively
handle nulls: ensure values is filtered or the some() predicate guards v (e.g.,
values.some((v: any) => v != null && getKey(v) === syncedTag(url))) and
similarly skip/clean null/undefined entries before pushing createItem(url); this
prevents TypeErrors during migration and avoids discarding the rest of the
config.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/frontend/src/context/userData.tsx (1)
244-251:⚠️ Potential issue | 🟡 MinorNormalise legacy URL entries before creating placeholder tags.
At Line 247, raw
urlsentries are iterated directly; malformed persisted values can be coerced into invalid placeholders (for example,<SYNCED: [object Object]>). Please filter to non-empty strings (and trim) before dedupe/push.Suggested patch
- const urls = (config as any)[legacyKey]; - if (!Array.isArray(urls) || urls.length === 0) return; + const rawUrls = (config as any)[legacyKey]; + if (!Array.isArray(rawUrls) || rawUrls.length === 0) return; + const urls = rawUrls + .filter((u: unknown): u is string => typeof u === 'string') + .map((u) => u.trim()) + .filter((u) => u.length > 0); + if (urls.length === 0) { + delete (config as any)[legacyKey]; + return; + } const values: any[] = Array.isArray((config as any)[valuesKey]) ? (config as any)[valuesKey] : []; for (const url of urls) { - if (!values.some((v: any) => v != null && getKey(v) === syncedTag(url))) { + const tag = syncedTag(url); + if (!values.some((v: any) => v != null && getKey(v) === tag)) { values.push(createItem(url)); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/context/userData.tsx` around lines 244 - 251, The loop over legacy `urls` may include non-string or empty entries producing invalid placeholders; before iterating/filtering use a normalized array: map each entry to a trimmed string, filter out non-strings and empty strings (e.g., const normalizedUrls = urls.map(u => typeof u === 'string' ? u.trim() : '').filter(Boolean)), then iterate `normalizedUrls` instead of `urls` when checking `values.some((v) => v != null && getKey(v) === syncedTag(url))` and when calling `createItem(url)` so dedupe and placeholder creation use validated, trimmed URL strings; keep references to `legacyKey`, `valuesKey`, `getKey`, `syncedTag`, and `createItem` unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/frontend/src/context/userData.tsx`:
- Around line 244-251: The loop over legacy `urls` may include non-string or
empty entries producing invalid placeholders; before iterating/filtering use a
normalized array: map each entry to a trimmed string, filter out non-strings and
empty strings (e.g., const normalizedUrls = urls.map(u => typeof u === 'string'
? u.trim() : '').filter(Boolean)), then iterate `normalizedUrls` instead of
`urls` when checking `values.some((v) => v != null && getKey(v) ===
syncedTag(url))` and when calling `createItem(url)` so dedupe and placeholder
creation use validated, trimmed URL strings; keep references to `legacyKey`,
`valuesKey`, `getKey`, `syncedTag`, and `createItem` unchanged.
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.
This allows users to put their synced urls anywhere so they can have expressions come before and after them. I also got rid of pattern deduplication as it works fine without it and matches the frontend better.
To achieve this I had to use a new system of how synced urls are stored. There is a legacy fallback so that existing configs will still work, and will be migrated to the new version on next save.
Summary by CodeRabbit
New Features
Refactor