Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds NIP‑50 "search" support: a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
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
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/test_nip50.c`:
- Around line 59-66: The TEST_ASSERT_EQUAL_STRING macro can dereference NULL via
strcmp; update the macro (TEST_ASSERT_EQUAL_STRING) to first check if either
expected or actual is NULL, handle cases where both are NULL (treat as equal) or
one is NULL (log which side is NULL), set g_test_failed = 1 and return, and only
call strcmp when both pointers are non-NULL so no undefined behavior occurs.
🧹 Nitpick comments (2)
src/relay_filter.c (1)
437-447: Consider documenting the silent failure when callback is NULL but search is specified.When a filter has a non-empty
searchfield butsearch_cbis NULL, the function returnsfalse. While this is a reasonable defensive behavior, it may silently fail from the caller's perspective if they forget to provide a callback for a filter with search.Consider whether logging a warning or documenting this behavior more explicitly would help developers debug issues where filters unexpectedly don't match.
tests/test_nip50.c (1)
167-186: Testtest_filter_clone_with_searchhas a subtle memory safety concern.The test sets
src.kindsto point to a stack-allocated array (int32_t kinds[] = {1}). While this works becausenostr_filter_clonecopies the data and the stack array remains valid throughout the function, this pattern could be fragile if the test structure changes. Also,nostr_filter_freeis only called ondst, notsrc, which is correct here sincesrcuses stack memory for most fields, butsrc.searchis heap-allocated and properly freed.The test is correct as written, but consider adding a comment explaining the intentional partial initialization for clarity.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/test_nip50.c`:
- Around line 161-173: Capture and assert the result of nostr_filter_parse in
test_filter_get_search before using the filter; e.g., store its return in a
variable (rc) and add an assertion that parsing succeeded (use
TEST_ASSERT_TRUE(rc == 0) or TEST_ASSERT_EQUAL_INT(0, rc) depending on the
project's convention) prior to calling nostr_filter_get_search,
nostr_filter_has_search, and nostr_filter_free.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Adds search field to filters for relay full-text search capability.
Summary by CodeRabbit
New Features
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.