fix: show muted search results as profile cards - #296
Open
dergigi wants to merge 4 commits into
Open
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthrough
ChangesMute-list to profile card expansion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🧹 Nitpick comments (1)
src/lib/search/strategies/authorSearchStrategy.ts (1)
43-49: ⚡ Quick winCap concurrent profile fetches for large mute lists.
Lines 43-49 fan out one async lookup per pubkey with unbounded
Promise.all, which can spike relay/network load on large lists.Suggested refactor
- const profiles = await Promise.all(pubkeys.map(async (pubkey) => { - try { - return await profileEventFromPubkey(pubkey); - } catch { - return null; - } - })); - - return profiles.filter((event): event is NDKEvent => event !== null); + const BATCH_SIZE = 20; + const results: NDKEvent[] = []; + for (let i = 0; i < pubkeys.length; i += BATCH_SIZE) { + const batch = pubkeys.slice(i, i + BATCH_SIZE); + const profiles = await Promise.all(batch.map(async (pubkey) => { + try { + return await profileEventFromPubkey(pubkey); + } catch { + return null; + } + })); + results.push(...profiles.filter((event): event is NDKEvent => event !== null)); + } + return results;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/search/strategies/authorSearchStrategy.ts` around lines 43 - 49, The Promise.all call on the pubkeys.map operation in authorSearchStrategy.ts creates unbounded concurrent requests that can overwhelm relays on large mute lists. Add concurrency limiting to cap the number of simultaneous profileEventFromPubkey calls (e.g., using a concurrency limit utility or batching strategy). Replace the direct Promise.all with a pattern that processes the pubkeys in controlled batches or with a semaphore that ensures only a maximum number of concurrent profile fetches occur at once, then collect all results into the profiles array.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/search/searchTypeDetection.ts`:
- Line 9: The condition on line 9 uses plain substring matching with includes()
for keywords like 'kind:10000' and 'is:muted', which causes false positives when
substrings match longer values (e.g., 'kind:100000' incorrectly matches
'kind:10000'). Replace the plain includes() checks for 'kind:10000' and
'is:muted' with token-boundary matching that only matches when these keywords
are followed by word boundaries (whitespace, end of string, or punctuation)
rather than being part of a longer value. You can use regex with word boundaries
or check that the match is followed by a space or end of string to properly
distinguish between 'kind:10000' and 'kind:100000'.
In `@src/lib/search/strategies/authorSearchStrategy.ts`:
- Around line 183-186: The conditional check in the mute-list expansion logic
for kind 10000 only returns profiles when they exist, but falls through to
return raw events when expansion yields zero results. Modify the logic after
calling expandMuteListResults to explicitly return an empty array when
profiles.length equals zero, ensuring that failed or empty mute-list expansions
do not revert to rendering raw kind 10000 events. This prevents the undesired
fallback behavior and maintains consistent handling of the mute-list expansion
case.
---
Nitpick comments:
In `@src/lib/search/strategies/authorSearchStrategy.ts`:
- Around line 43-49: The Promise.all call on the pubkeys.map operation in
authorSearchStrategy.ts creates unbounded concurrent requests that can overwhelm
relays on large mute lists. Add concurrency limiting to cap the number of
simultaneous profileEventFromPubkey calls (e.g., using a concurrency limit
utility or batching strategy). Replace the direct Promise.all with a pattern
that processes the pubkeys in controlled batches or with a semaphore that
ensures only a maximum number of concurrent profile fetches occur at once, then
collect all results into the profiles array.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8cf9ba19-1fb9-4e1e-aad6-7594203ae23a
📒 Files selected for processing (2)
src/lib/search/searchTypeDetection.tssrc/lib/search/strategies/authorSearchStrategy.ts
Owner
Author
|
It only shows two profiles? It should show way more I think? I guess in the example we have in #294 (comment) it should show one profile card per |
Owner
Author
|
Hm, not happy with it yet. We'll probably need a profile list element or something. Otherwise we aren't able to switch back to the raw json format. |
Owner
Author
|
Pushed a follow-up for this. The result stays anchored to the kind Checked the preview for
I already re-ran |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/MuteListCard.tsx`:
- Around line 43-50: The click handler logic in handleOpenProfile uses user.npub
directly, but the component computes a fallback npub variable for rendering.
Replace all instances of user.npub with the resolved npub variable in the
onAuthorClick callback and window.location.href navigation to ensure consistency
between what is displayed to the user and where the click navigation actually
directs them. This applies to both the initial click handler block (around lines
43-50) and the secondary handler block (around lines 53-60).
In `@src/lib/search/strategies/authorSearchStrategy.ts`:
- Around line 150-161: The authorSearchStrategy function is passing
chosenRelaySet or broadRelaySet directly to subscribeAndCollect calls without
checking if the filters contain a search property. When filter.search is
present, you must use getNip50SearchRelaySet() instead of the default relay sets
to comply with NIP-50 relay selection requirements. Update all three
subscribeAndCollect calls in the function (the one in the map callback around
line 150, and the two else branches around lines 156 and 159-160) to check
whether the filters have a search property and conditionally pass
getNip50SearchRelaySet() as the relaySet parameter when search is present,
otherwise use the appropriate relay set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2fbdd6db-ccc5-4005-8866-63df21fb8e2e
📒 Files selected for processing (4)
src/components/MuteListCard.tsxsrc/components/SearchResultsList.tsxsrc/lib/search/muteListResultData.tssrc/lib/search/strategies/authorSearchStrategy.ts
Comment on lines
+43
to
+50
| if (onAuthorClick) { | ||
| onAuthorClick(user.npub); | ||
| return; | ||
| } | ||
|
|
||
| if (typeof window !== 'undefined') { | ||
| window.location.href = `/p/${user.npub}`; | ||
| } |
Contributor
There was a problem hiding this comment.
Use the resolved npub variable for click navigation/callbacks.
The row computes a fallback npub for rendering, but handleOpenProfile still uses user.npub. If fallback logic is hit, click behavior can point to a different/invalid target than what is displayed.
Suggested patch
- const handleOpenProfile = () => {
+ const handleOpenProfile = () => {
@@
- if (onAuthorClick) {
- onAuthorClick(user.npub);
+ if (onAuthorClick) {
+ onAuthorClick(npub);
return;
}
@@
- window.location.href = `/p/${user.npub}`;
+ window.location.href = `/p/${npub}`;
}
};Also applies to: 53-60
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/MuteListCard.tsx` around lines 43 - 50, The click handler
logic in handleOpenProfile uses user.npub directly, but the component computes a
fallback npub variable for rendering. Replace all instances of user.npub with
the resolved npub variable in the onAuthorClick callback and
window.location.href navigation to ensure consistency between what is displayed
to the user and where the click navigation actually directs them. This applies
to both the initial click handler block (around lines 43-50) and the secondary
handler block (around lines 53-60).
Comment on lines
+150
to
+161
| return await subscribeAndCollect(f, { timeoutMs: 8000, relaySet: chosenRelaySet, abortSignal, onPartial: partialResultsHandler }); | ||
| } catch { return []; } | ||
| })); | ||
| const seen = new Set<string>(); | ||
| for (const r of perSeed) { | ||
| for (const e of r) { if (!seen.has(e.id)) { seen.add(e.id); res.push(e); } } | ||
| } | ||
| } else { | ||
| res = await subscribeAndCollect(filters, { timeoutMs: 8000, relaySet: chosenRelaySet, abortSignal, onPartial: onPartialResults }); | ||
| res = await subscribeAndCollect(filters, { timeoutMs: 8000, relaySet: chosenRelaySet, abortSignal, onPartial: partialResultsHandler }); | ||
| } | ||
| } else { | ||
| res = await subscribeAndCollect(filters, { timeoutMs: 8000, relaySet: chosenRelaySet, abortSignal, onPartial: onPartialResults }); | ||
| res = await subscribeAndCollect(filters, { timeoutMs: 8000, relaySet: chosenRelaySet, abortSignal, onPartial: partialResultsHandler }); |
Contributor
There was a problem hiding this comment.
Use getNip50SearchRelaySet() whenever filter.search is present.
These subscriptions can run with search filters but still pass chosenRelaySet/broadRelaySet directly. That violates the relay-selection contract for NIP-50 searches.
Suggested patch
+import { getNip50SearchRelaySet } from '`@/lib/relays/nip50`';
@@
- return await subscribeAndCollect(f, { timeoutMs: 8000, relaySet: chosenRelaySet, abortSignal, onPartial: partialResultsHandler });
+ const relaySetForSearch = f.search ? await getNip50SearchRelaySet() : chosenRelaySet;
+ return await subscribeAndCollect(f, { timeoutMs: 8000, relaySet: relaySetForSearch, abortSignal, onPartial: partialResultsHandler });
@@
- res = await subscribeAndCollect(filters, { timeoutMs: 8000, relaySet: chosenRelaySet, abortSignal, onPartial: partialResultsHandler });
+ const relaySetForSearch = filters.search ? await getNip50SearchRelaySet() : chosenRelaySet;
+ res = await subscribeAndCollect(filters, { timeoutMs: 8000, relaySet: relaySetForSearch, abortSignal, onPartial: partialResultsHandler });
@@
- res = await subscribeAndCollect(filters, { timeoutMs: 10000, relaySet: broadRelaySet, abortSignal, onPartial: partialResultsHandler });
+ const relaySetForSearch = filters.search ? await getNip50SearchRelaySet() : broadRelaySet;
+ res = await subscribeAndCollect(filters, { timeoutMs: 10000, relaySet: relaySetForSearch, abortSignal, onPartial: partialResultsHandler });As per coding guidelines, "Always pass an explicit NIP-50 relay set (via getNip50SearchRelaySet() in src/lib/relays/nip50.ts) when subscribing with a search filter" and "Never let a search subscription fall through to the default pool or an unfiltered relay set."
Also applies to: 194-194
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/search/strategies/authorSearchStrategy.ts` around lines 150 - 161,
The authorSearchStrategy function is passing chosenRelaySet or broadRelaySet
directly to subscribeAndCollect calls without checking if the filters contain a
search property. When filter.search is present, you must use
getNip50SearchRelaySet() instead of the default relay sets to comply with NIP-50
relay selection requirements. Update all three subscribeAndCollect calls in the
function (the one in the map callback around line 150, and the two else branches
around lines 156 and 159-160) to check whether the filters have a search
property and conditionally pass getNip50SearchRelaySet() as the relaySet
parameter when search is present, otherwise use the appropriate relay set.
Source: Coding guidelines
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 keeps
is:muted by:...results anchored to the underlying kind10000event while rendering the mutedptags as a dedicated profile list, so the raw JSON toggle still works. It also keeps the mute-list loading state in the profile lane and aggregates muted pubkeys across fetched mute-list events for the rendered list. Verified withnpm run lint,npm test -- --runInBand,npm run build, and the Vercel preview foris:muted by:fiatjaf.10000mute-list searches as a dedicated profile-list card instead of flattening them into standalone profile resultsShow raw JSONstill reveals the real kind10000payloadFixes #294