Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #375 +/- ##
==========================================
- Coverage 74.39% 72.85% -1.54%
==========================================
Files 197 199 +2
Lines 9643 10036 +393
==========================================
+ Hits 7174 7312 +138
- Misses 2469 2724 +255 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1-leo
reviewed
Mar 2, 2026
Contributor
1-leo
left a comment
1-leo
left a comment
There was a problem hiding this comment.
Have you tested the need for dedicated caching, or is the events cache sufficient for this?
| } | ||
|
|
||
| // Close subscriptions when stream is cancelled | ||
| controller.onCancel = () async { |
Contributor
There was a problem hiding this comment.
should also close all on close
| /// [providers] - Optional list of providers to use. If null, uses default providers. | ||
| /// | ||
| /// Returns [Nip85UserMetrics] or null if no assertion found. | ||
| Future<Nip85UserMetrics?> getUserMetrics( |
Contributor
There was a problem hiding this comment.
the default could be the streaming method and the future could be just a stream.toList()
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds NIP‑85 Trusted Assertions: new domain entities and metrics, default provider config, a TrustedAssertions usecase (fetch + stream), presentation-layer wiring and public exports, mock-relay support, tests, and documentation for NIP‑85 usage. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Application
participant Ndk as Ndk.ta\n(TrustedAssertions)
participant ProviderFilter as Provider\nFilter & Grouper
participant Relay as Nostr Relay
participant Parser as Event\nParser
participant Agg as Aggregator
Client->>Ndk: getUserMetrics(pubkey, metrics?, providers?)
Ndk->>ProviderFilter: filter providers by kind=user (or use defaults)
ProviderFilter-->>Ndk: grouped providers by relay
loop per relay
Ndk->>Relay: query(kind=30382, authors=[providers], dTags=[pubkey])
Relay-->>Ndk: Nip01Event[] (assertions)
end
loop per event
Ndk->>Parser: validate kind, extract dTag and metric tags
Parser-->>Ndk: parsed metric entries with createdAt
end
Ndk->>Agg: aggregate metrics by metric.latestCreatedAt
Agg-->>Ndk: Nip85UserMetrics
Ndk-->>Client: Nip85UserMetrics | null
sequenceDiagram
participant Client as Application
participant Ndk as Ndk.ta\n(TrustedAssertions)
participant ProviderFilter as Provider\nFilter & Grouper
participant Relay as Nostr Relay
participant Parser as Event\nParser
participant Controller as StreamController
participant Consumer as Stream\nSubscriber
Client->>Ndk: streamEventMetrics(eventId, metrics?, providers?)
Ndk->>ProviderFilter: filter providers by kind=event
ProviderFilter-->>Ndk: grouped providers by relay
Ndk->>Controller: create StreamController
Ndk-->>Consumer: return stream
loop per relay
Ndk->>Relay: subscribe(kind=30383, authors=[providers], dTags=[eventId])
end
Relay-->>Ndk: Nip01Event (assertion)
Ndk->>Parser: parse event metrics
Parser-->>Ndk: parsed partial metrics
Ndk->>Controller: update aggregation and emit
Consumer->>Client: onData(Nip85EventMetrics)
Client->>Consumer: cancel
Consumer->>Ndk: onCancel
Ndk->>Relay: close subscription(s)
Ndk->>Controller: close
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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: 6
🧹 Nitpick comments (3)
packages/ndk/lib/presentation_layer/ndk_config.dart (1)
85-85: Consider makingdefaultTrustedProvidersimmutable with a defensive copy.Current assignment keeps caller-owned list references, so later external mutations can change runtime behavior unexpectedly.
Suggested refactor
- List<Nip85TrustedProvider> defaultTrustedProviders; + final List<Nip85TrustedProvider> defaultTrustedProviders; ... - this.defaultTrustedProviders = DEFAULT_NIP85_PROVIDERS, - }); + List<Nip85TrustedProvider> defaultTrustedProviders = DEFAULT_NIP85_PROVIDERS, + }) : defaultTrustedProviders = + List<Nip85TrustedProvider>.unmodifiable(defaultTrustedProviders);Also applies to: 117-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ndk/lib/presentation_layer/ndk_config.dart` at line 85, Make the defaultTrustedProviders field immutable and defensive-copied: change the mutable List<Nip85TrustedProvider> defaultTrustedProviders to an immutable field (e.g., final) and when assigning from any incoming parameter (constructor or setter used around the symbol defaultTrustedProviders, including where it’s passed at the other occurrence) copy the list (List.from(...) or List.unmodifiable(...)) so the instance holds its own unmodifiable copy and external mutations cannot affect runtime behavior.packages/ndk/test/usecases/ta/trusted_assertions_test.dart (2)
163-181: Register ad-hoc fixtures withaddTearDown.
ndkNoProvidersand the extraMockRelayinstances are only cleaned up on the happy path. A failing assertion leaves the server or NDK instance alive and can make later tests flaky.♻️ Minimal pattern
final ndkNoProviders = Ndk(NdkConfig( eventVerifier: MockEventVerifier(), cache: MemCacheManager(), engine: NdkEngine.RELAY_SETS, bootstrapRelays: [relay.url], defaultTrustedProviders: [], )); + addTearDown(() async => ndkNoProviders.destroy()); final relay2 = MockRelay(name: 'nip85-relay-2', explicitPort: 5197); + addTearDown(() async => relay2.stopServer());Also applies to: 192-228, 270-318
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ndk/test/usecases/ta/trusted_assertions_test.dart` around lines 163 - 181, The test creates resources (ndkNoProviders and ad-hoc MockRelay instances) that must be cleaned up even on failures; register tear-downs by calling addTearDown for each created resource instead of relying on happy-path awaits — e.g., after constructing ndkNoProviders call addTearDown(() => ndkNoProviders.destroy()), and for any MockRelay (or similar relay fixtures created in these tests) call addTearDown(() => mockRelay.close() or mockRelay.destroy()); apply this pattern to the other test blocks referenced (lines ~192-228 and ~270-318) so all created Ndk and relay fixtures are always cleaned up on test failure.
93-118: Add a user multi-provider/allowlist case.All user-metric tests here still use one pubkey publishing every metric. That won't catch two important regressions: failing to merge user metrics across providers, and accepting extra tags from a provider that is trusted for only one metric.
Also applies to: 130-161, 184-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ndk/test/usecases/ta/trusted_assertions_test.dart` around lines 93 - 118, Add a test case that uses multiple trusted providers so we exercise merging and allowlist behavior: update the NdkConfig.defaultTrustedProviders list to include a second Nip85TrustedProvider with a different pubkey (e.g., a new providerKey2) and the same relay.url, then create events where providerKey and providerKey2 each publish different user metrics and assert the code merging logic combines metrics from both providers (verify via the same assertions used for metrics). Also add a negative case where a Nip85TrustedProvider is trusted only for one Nip85Metric (e.g., followers) and ensure an event from that provider that contains tags for a different metric (e.g., postCount) is rejected or those extra tags are ignored—use the same helper functions and assertions around event acceptance/merging (references: NdkConfig, defaultTrustedProviders, Nip85TrustedProvider, Nip85Kind, Nip85Metric, providerKey, relay.url).
🤖 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/ndk/lib/domain_layer/entities/nip_85.dart`:
- Around line 183-201: Nip85EventMetrics currently stores a single
providerPubkey/createdAt while getEventMetrics merges multiple providers, which
loses provenance; update the model or API so provenance is preserved: either
change Nip85EventMetrics.metrics to map each Nip85Metric to a small
provenance-bearing structure (e.g., value plus providerPubkey and createdAt) so
each metric keeps its source, or change getEventMetrics to return a list of
per-provider assertions (List<Nip85EventMetrics>) instead of a single merged
object; adjust callers of Nip85EventMetrics and getEventMetrics accordingly to
consume the new per-metric or per-provider provenance fields.
In `@packages/ndk/lib/domain_layer/usecases/ta/trusted_assertions.dart`:
- Around line 202-215: When parsing metric tags (using Nip85Metric.fromTagName)
only accept and write into metricsMap if the metric is both requested and
allowed by the provider for that event.publisher; fetch the provider/allowlist
for the event's pubKey on the current relay and compute allowed =
(requestedMetrics == null ? providerAllowlist : requestedMetrics ∩
providerAllowlist) and then only set metricsMap[metric] when metric ∈ allowed
and parsedValue != null; apply the same change to the other metric-parsing
blocks referenced (the parsing blocks around the other occurrences) so each uses
the provider-specific allowlist intersection before writing metricsMap.
- Around line 66-100: The loop that queries relays (using _requests.query with
Filter kinds: [Nip85Kind.user] and parsing via _parseUserMetricsEvent) currently
keeps only the newest Nip85UserMetrics parsed event (result = parsed when
parsed.createdAt > latestCreatedAt) which drops earlier provider/assertion data;
instead, accumulate/merge into a single combined Nip85UserMetrics snapshot by
merging fields from each parsed result (e.g., union provider lists, merge metric
maps, choose the latest createdAt where appropriate) rather than replacing the
whole object; apply the same merging approach to the other similar codepaths
referenced (the blocks around lines 154-159, 493-524, 569-574, 654-685, 730-735)
so that callers receive a complete aggregated result built from all provider
events.
- Around line 627-633: The current lookup/filter logic incorrectly keys NIP-73
external-id queries off the `d` tag and uses `getDtag()`/`dTags`, which will
miss valid assertions whose immutable identifier is in the `i` tag; update the
affected functions to use the `identifier` (NIP-73 i-tag) for filtering and
parsing instead of `d`: replace uses of `dTags` with `iTags` (or equivalent) in
the query predicates, and call the parser method that returns the `i` tag (not
`getDtag()`) when extracting the assertion key; apply the same change at the
other occurrences flagged (the blocks that reference `dTags`/`getDtag()` around
the cited ranges) so all 30385/external-id lookups and parsing use the i-tag as
the canonical key.
In `@packages/ndk/test/mocks/mock_relay.dart`:
- Around line 303-316: The NIP-85 branch that builds eventsForThisFilter from
_nip85Assertions currently omits time bounds; update the predicate inside the
where(...) for that branch (the block handling filter.kinds 30382-30385) to also
enforce filter.since and filter.until by checking each event's createdAt against
filter.since (>=) and filter.until (<=) similar to other branches, so only
events within the requested time window are added; reference the variables
filter, _nip85Assertions, e.getDtag(), e.pubKey, and e.createdAt when adding the
time checks.
In `@packages/ndk/test/usecases/ta/trusted_assertions_test_2.dart`:
- Around line 130-132: The printed label is incorrect: inside the stream.listen
callback (the listener registered on stream) you're logging
metrics.reactionsCount but labeling it "postCount"; update the print statement
in the listener to use a matching label (e.g., "reactionsCount") or print the
correct field for post count (e.g., metrics.postCount) so the label and the
accessed property (metrics.reactionsCount) are consistent.
---
Nitpick comments:
In `@packages/ndk/lib/presentation_layer/ndk_config.dart`:
- Line 85: Make the defaultTrustedProviders field immutable and
defensive-copied: change the mutable List<Nip85TrustedProvider>
defaultTrustedProviders to an immutable field (e.g., final) and when assigning
from any incoming parameter (constructor or setter used around the symbol
defaultTrustedProviders, including where it’s passed at the other occurrence)
copy the list (List.from(...) or List.unmodifiable(...)) so the instance holds
its own unmodifiable copy and external mutations cannot affect runtime behavior.
In `@packages/ndk/test/usecases/ta/trusted_assertions_test.dart`:
- Around line 163-181: The test creates resources (ndkNoProviders and ad-hoc
MockRelay instances) that must be cleaned up even on failures; register
tear-downs by calling addTearDown for each created resource instead of relying
on happy-path awaits — e.g., after constructing ndkNoProviders call
addTearDown(() => ndkNoProviders.destroy()), and for any MockRelay (or similar
relay fixtures created in these tests) call addTearDown(() => mockRelay.close()
or mockRelay.destroy()); apply this pattern to the other test blocks referenced
(lines ~192-228 and ~270-318) so all created Ndk and relay fixtures are always
cleaned up on test failure.
- Around line 93-118: Add a test case that uses multiple trusted providers so we
exercise merging and allowlist behavior: update the
NdkConfig.defaultTrustedProviders list to include a second Nip85TrustedProvider
with a different pubkey (e.g., a new providerKey2) and the same relay.url, then
create events where providerKey and providerKey2 each publish different user
metrics and assert the code merging logic combines metrics from both providers
(verify via the same assertions used for metrics). Also add a negative case
where a Nip85TrustedProvider is trusted only for one Nip85Metric (e.g.,
followers) and ensure an event from that provider that contains tags for a
different metric (e.g., postCount) is rejected or those extra tags are
ignored—use the same helper functions and assertions around event
acceptance/merging (references: NdkConfig, defaultTrustedProviders,
Nip85TrustedProvider, Nip85Kind, Nip85Metric, providerKey, relay.url).
🪄 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: 9315c9c0-c7c3-4f58-8f70-7c8f22a3bc29
⛔ Files ignored due to path filters (1)
packages/ndk_cache_manager_test_suite/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
packages/ndk/lib/config/nip85_defaults.dartpackages/ndk/lib/domain_layer/entities/nip_85.dartpackages/ndk/lib/domain_layer/usecases/ta/trusted_assertions.dartpackages/ndk/lib/entities.dartpackages/ndk/lib/ndk.dartpackages/ndk/lib/presentation_layer/init.dartpackages/ndk/lib/presentation_layer/ndk.dartpackages/ndk/lib/presentation_layer/ndk_config.dartpackages/ndk/test/mocks/mock_relay.dartpackages/ndk/test/usecases/ta/trusted_assertions_test.dartpackages/ndk/test/usecases/ta/trusted_assertions_test_2.dart
Comment on lines
+183
to
+201
| class Nip85EventMetrics { | ||
| /// Subject event ID | ||
| final String eventId; | ||
|
|
||
| /// Provider pubkey (who made the assertion) | ||
| final String providerPubkey; | ||
|
|
||
| /// Timestamp of the assertion | ||
| final int createdAt; | ||
|
|
||
| /// Map of metric values | ||
| final Map<Nip85Metric, dynamic> metrics; | ||
|
|
||
| Nip85EventMetrics({ | ||
| required this.eventId, | ||
| required this.providerPubkey, | ||
| required this.createdAt, | ||
| required this.metrics, | ||
| }); |
There was a problem hiding this comment.
This result shape loses metric provenance.
getEventMetrics() merges values from multiple providers/timestamps, but Nip85EventMetrics exposes only one providerPubkey and one createdAt. A merged object can therefore misattribute some metrics to the newest assertion. Consider storing provenance per metric, or returning per-provider assertions instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ndk/lib/domain_layer/entities/nip_85.dart` around lines 183 - 201,
Nip85EventMetrics currently stores a single providerPubkey/createdAt while
getEventMetrics merges multiple providers, which loses provenance; update the
model or API so provenance is preserved: either change Nip85EventMetrics.metrics
to map each Nip85Metric to a small provenance-bearing structure (e.g., value
plus providerPubkey and createdAt) so each metric keeps its source, or change
getEventMetrics to return a list of per-provider assertions
(List<Nip85EventMetrics>) instead of a single merged object; adjust callers of
Nip85EventMetrics and getEventMetrics accordingly to consume the new per-metric
or per-provider provenance fields.
Comment on lines
+202
to
+215
| // Try to parse as metric | ||
| final metric = Nip85Metric.fromTagName(tagName); | ||
| if (metric != null) { | ||
| // If specific metrics requested, filter | ||
| if (requestedMetrics != null && !requestedMetrics.contains(metric)) { | ||
| continue; | ||
| } | ||
|
|
||
| // Parse value based on metric type | ||
| final parsedValue = int.tryParse(tagValue); | ||
| if (parsedValue != null) { | ||
| metricsMap[metric] = parsedValue; | ||
| } | ||
| } |
There was a problem hiding this comment.
Enforce each provider's metric allowlist during parsing.
Once an event reaches these parsers, every recognized metric tag is accepted. A provider configured only for rank can still populate followers, post_cnt, etc. if it emits them, which defeats the per-metric trust model. Intersect requestedMetrics with the metrics configured for event.pubKey on that relay before writing into metricsMap.
Also applies to: 438-451, 603-612, 764-773
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ndk/lib/domain_layer/usecases/ta/trusted_assertions.dart` around
lines 202 - 215, When parsing metric tags (using Nip85Metric.fromTagName) only
accept and write into metricsMap if the metric is both requested and allowed by
the provider for that event.publisher; fetch the provider/allowlist for the
event's pubKey on the current relay and compute allowed = (requestedMetrics ==
null ? providerAllowlist : requestedMetrics ∩ providerAllowlist) and then only
set metricsMap[metric] when metric ∈ allowed and parsedValue != null; apply the
same change to the other metric-parsing blocks referenced (the parsing blocks
around the other occurrences) so each uses the provider-specific allowlist
intersection before writing metricsMap.
Comment on lines
+627
to
+633
| /// Get external identifier metrics from trusted providers (NIP-73) | ||
| /// | ||
| /// [identifier] - The NIP-73 i-tag value to get metrics for | ||
| /// [metrics] - Optional set of specific metrics to fetch. If null, fetches all available. | ||
| /// [providers] - Optional list of providers to use. If null, uses default providers. | ||
| /// | ||
| /// Returns [Nip85ExternalIdMetrics] or null if no assertion found. |
There was a problem hiding this comment.
Don't key 30385 lookups off d unless d is guaranteed to mirror i.
The API contract here says identifier is the NIP-73 i-tag value, but both query paths filter on dTags and the parser reads getDtag(). If d is just the replaceable key, otherwise-valid external-id assertions will never match these lookups.
Also applies to: 665-668, 719-724, 756-777
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ndk/lib/domain_layer/usecases/ta/trusted_assertions.dart` around
lines 627 - 633, The current lookup/filter logic incorrectly keys NIP-73
external-id queries off the `d` tag and uses `getDtag()`/`dTags`, which will
miss valid assertions whose immutable identifier is in the `i` tag; update the
affected functions to use the `identifier` (NIP-73 i-tag) for filtering and
parsing instead of `d`: replace uses of `dTags` with `iTags` (or equivalent) in
the query predicates, and call the parser method that returns the `i` tag (not
`getDtag()`) when extracting the assertion key; apply the same change at the
other occurrences flagged (the blocks that reference `dTags`/`getDtag()` around
the cited ranges) so all 30385/external-id lookups and parsing use the i-tag as
the canonical key.
Comment on lines
+303
to
+316
| // Match against NIP-85 assertions (kinds 30382-30385) | ||
| else if (filter.kinds != null && | ||
| filter.kinds!.any((k) => k >= 30382 && k <= 30385) && | ||
| filter.authors != null && | ||
| filter.authors!.isNotEmpty) { | ||
| eventsForThisFilter.addAll(_nip85Assertions.values.where((e) { | ||
| bool kindMatches = filter.kinds!.contains(e.kind); | ||
| bool authorMatches = filter.authors!.contains(e.pubKey); | ||
| bool dTagMatches = filter.dTags == null || | ||
| filter.dTags!.isEmpty || | ||
| filter.dTags!.contains(e.getDtag()); | ||
| return kindMatches && authorMatches && dTagMatches; | ||
| }).toList()); | ||
| } |
There was a problem hiding this comment.
Apply time filtering in the NIP-85 branch.
On Line 303–316, NIP-85 matching ignores since/until, so this path can return out-of-range events.
Suggested fix
else if (filter.kinds != null &&
filter.kinds!.any((k) => k >= 30382 && k <= 30385) &&
filter.authors != null &&
filter.authors!.isNotEmpty) {
eventsForThisFilter.addAll(_nip85Assertions.values.where((e) {
bool kindMatches = filter.kinds!.contains(e.kind);
bool authorMatches = filter.authors!.contains(e.pubKey);
bool dTagMatches = filter.dTags == null ||
filter.dTags!.isEmpty ||
filter.dTags!.contains(e.getDtag());
- return kindMatches && authorMatches && dTagMatches;
+ bool timeMatches = _matchesTimeFilter(e, filter);
+ return kindMatches && authorMatches && dTagMatches && timeMatches;
}).toList());
}📝 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.
Suggested change
| // Match against NIP-85 assertions (kinds 30382-30385) | |
| else if (filter.kinds != null && | |
| filter.kinds!.any((k) => k >= 30382 && k <= 30385) && | |
| filter.authors != null && | |
| filter.authors!.isNotEmpty) { | |
| eventsForThisFilter.addAll(_nip85Assertions.values.where((e) { | |
| bool kindMatches = filter.kinds!.contains(e.kind); | |
| bool authorMatches = filter.authors!.contains(e.pubKey); | |
| bool dTagMatches = filter.dTags == null || | |
| filter.dTags!.isEmpty || | |
| filter.dTags!.contains(e.getDtag()); | |
| return kindMatches && authorMatches && dTagMatches; | |
| }).toList()); | |
| } | |
| // Match against NIP-85 assertions (kinds 30382-30385) | |
| else if (filter.kinds != null && | |
| filter.kinds!.any((k) => k >= 30382 && k <= 30385) && | |
| filter.authors != null && | |
| filter.authors!.isNotEmpty) { | |
| eventsForThisFilter.addAll(_nip85Assertions.values.where((e) { | |
| bool kindMatches = filter.kinds!.contains(e.kind); | |
| bool authorMatches = filter.authors!.contains(e.pubKey); | |
| bool dTagMatches = filter.dTags == null || | |
| filter.dTags!.isEmpty || | |
| filter.dTags!.contains(e.getDtag()); | |
| bool timeMatches = _matchesTimeFilter(e, filter); | |
| return kindMatches && authorMatches && dTagMatches && timeMatches; | |
| }).toList()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ndk/test/mocks/mock_relay.dart` around lines 303 - 316, The NIP-85
branch that builds eventsForThisFilter from _nip85Assertions currently omits
time bounds; update the predicate inside the where(...) for that branch (the
block handling filter.kinds 30382-30385) to also enforce filter.since and
filter.until by checking each event's createdAt against filter.since (>=) and
filter.until (<=) similar to other branches, so only events within the requested
time window are added; reference the variables filter, _nip85Assertions,
e.getDtag(), e.pubKey, and e.createdAt when adding the time checks.
Comment on lines
+130
to
+132
| stream.listen((metrics) { | ||
| print('postCount: ${metrics.reactionsCount}'); | ||
| }); |
There was a problem hiding this comment.
Fix mislabeled output field.
On Line 131, the stream prints reactionsCount but labels it as postCount.
Suggested fix
- print('postCount: ${metrics.reactionsCount}');
+ print('reactionsCount: ${metrics.reactionsCount}');📝 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.
Suggested change
| stream.listen((metrics) { | |
| print('postCount: ${metrics.reactionsCount}'); | |
| }); | |
| stream.listen((metrics) { | |
| print('reactionsCount: ${metrics.reactionsCount}'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ndk/test/usecases/ta/trusted_assertions_test_2.dart` around lines
130 - 132, The printed label is incorrect: inside the stream.listen callback
(the listener registered on stream) you're logging metrics.reactionsCount but
labeling it "postCount"; update the print statement in the listener to use a
matching label (e.g., "reactionsCount") or print the correct field for post
count (e.g., metrics.postCount) so the label and the accessed property
(metrics.reactionsCount) are consistent.
frnandu
approved these changes
Apr 8, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/ndk/lib/domain_layer/usecases/ta/trusted_assertions.dart (2)
113-115: Consider logging relay failures instead of silently ignoring them.The empty catch block swallows all exceptions, which can mask connectivity issues or malformed responses during debugging. A brief log or structured error capture would aid troubleshooting without affecting the resilience logic.
- } catch (_) { - // Continue with other relays if one fails - } + } catch (e) { + // Log but continue with other relays + // Logger.log.w(() => "Failed to query relay $relay: $e"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ndk/lib/domain_layer/usecases/ta/trusted_assertions.dart` around lines 113 - 115, Replace the empty catch (_) block in trusted_assertions.dart with a non-disruptive log of the error and relay context so failures aren’t silently swallowed; keep the current behavior of continuing to other relays but call the existing logger (or add one if none exists) to record the exception details and relay identifier (e.g., the relay URL or name) inside the catch so connectivity/malformed-response issues are visible when troubleshooting.
554-580: Addressable metrics not aggregated per-metric like user/event metrics.
getUserMetricsandgetEventMetricsaggregate metrics individually (keeping the latest value per metric across providers), butgetAddressableMetricsjust replaces the entire result with whichever event has the latestcreatedAt. This means metrics from different providers won't be merged.If this inconsistency is intentional, consider documenting the difference in the method's docstring. Otherwise, apply the same per-metric aggregation pattern:
Aggregation pattern matching user/event
- Nip85AddressableMetrics? result; - int latestCreatedAt = 0; + final aggregatedMetrics = <Nip85Metric, dynamic>{}; + final metricCreatedAt = <Nip85Metric, int>{}; + String? latestProviderPubkey; + String? latestEventAddress; + int latestCreatedAt = 0; // ... in the loop ... - if (parsed != null && parsed.createdAt > latestCreatedAt) { - result = parsed; - latestCreatedAt = parsed.createdAt; - } + if (parsed == null) continue; + parsed.metrics.forEach((metric, value) { + final currentMetricCreatedAt = metricCreatedAt[metric] ?? -1; + if (parsed.createdAt >= currentMetricCreatedAt) { + aggregatedMetrics[metric] = value; + metricCreatedAt[metric] = parsed.createdAt; + } + }); + if (parsed.createdAt >= latestCreatedAt) { + latestCreatedAt = parsed.createdAt; + latestProviderPubkey = parsed.providerPubkey; + latestEventAddress = parsed.eventAddress; + }The same inconsistency exists in
getExternalIdMetrics(lines 715-741).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ndk/lib/domain_layer/usecases/ta/trusted_assertions.dart` around lines 554 - 580, The getAddressableMetrics implementation currently picks the entire Nip85AddressableMetrics from the single event with the latest createdAt rather than merging individual metric fields like getUserMetrics/getEventMetrics do; change getAddressableMetrics to perform per-metric aggregation: iterate events from _requests.query, parse with _parseAddressableMetricsEvent, and for each metric field on Nip85AddressableMetrics compare event.createdAt to the stored timestamp for that specific metric and update only that metric and its per-metric timestamp when newer (mirroring the aggregation pattern used by getUserMetrics/getEventMetrics); apply the same fix to getExternalIdMetrics so each metric is merged individually instead of replacing the whole result.
🤖 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/ndk/lib/domain_layer/usecases/ta/trusted_assertions.dart`:
- Around line 464-467: Remove the debug print in the onError closure used for
the relay subscription in trusted_assertions.dart: delete the print("Error in
relay subscription for event metrics: $e") call and either leave the comment "//
Ignore errors from individual relays" or replace the print with a structured
logging call using the project's logging utility (e.g., logger.error or
log.error) that logs the error object and a short context message; ensure you
reference the onError closure for the relay subscription handling so the change
is made in the correct callback.
- Around line 188-226: The stream returned by the logic in
response.stream.listen never closes because there is no onDone handling: modify
the subscription setup in the block using response.stream.listen to increment a
local activeSubscriptions counter when each relay subscription starts and
decrement it in both onDone and onError (or use the listen's onDone callback),
and when the counter reaches zero call controller.close(); also ensure the
existing onCancel still cancels subscriptions; apply the same fix to
streamEventMetrics, streamAddressableMetrics, and streamExternalIdMetrics so
each method tracks active subscriptions and closes the controller when all relay
streams complete.
---
Nitpick comments:
In `@packages/ndk/lib/domain_layer/usecases/ta/trusted_assertions.dart`:
- Around line 113-115: Replace the empty catch (_) block in
trusted_assertions.dart with a non-disruptive log of the error and relay context
so failures aren’t silently swallowed; keep the current behavior of continuing
to other relays but call the existing logger (or add one if none exists) to
record the exception details and relay identifier (e.g., the relay URL or name)
inside the catch so connectivity/malformed-response issues are visible when
troubleshooting.
- Around line 554-580: The getAddressableMetrics implementation currently picks
the entire Nip85AddressableMetrics from the single event with the latest
createdAt rather than merging individual metric fields like
getUserMetrics/getEventMetrics do; change getAddressableMetrics to perform
per-metric aggregation: iterate events from _requests.query, parse with
_parseAddressableMetricsEvent, and for each metric field on
Nip85AddressableMetrics compare event.createdAt to the stored timestamp for that
specific metric and update only that metric and its per-metric timestamp when
newer (mirroring the aggregation pattern used by
getUserMetrics/getEventMetrics); apply the same fix to getExternalIdMetrics so
each metric is merged individually instead of replacing the whole result.
🪄 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: a7289c06-e756-4f46-812d-b244b551675b
📒 Files selected for processing (1)
packages/ndk/lib/domain_layer/usecases/ta/trusted_assertions.dart
Comment on lines
+188
to
+226
| response.stream.listen( | ||
| (event) { | ||
| final parsed = _parseUserMetricsEvent(event, metrics); | ||
| if (parsed == null) return; | ||
|
|
||
| parsed.metrics.forEach((metric, value) { | ||
| final currentMetricCreatedAt = metricCreatedAt[metric] ?? -1; | ||
| if (parsed.createdAt >= currentMetricCreatedAt) { | ||
| aggregatedMetrics[metric] = value; | ||
| metricCreatedAt[metric] = parsed.createdAt; | ||
| } | ||
| }); | ||
|
|
||
| if (parsed.createdAt >= latestCreatedAt) { | ||
| latestCreatedAt = parsed.createdAt; | ||
| latestProviderPubkey = parsed.providerPubkey; | ||
| latestPubkey = parsed.pubkey; | ||
| topics | ||
| ..clear() | ||
| ..addAll(parsed.topics); | ||
| } | ||
|
|
||
| if (latestProviderPubkey != null) { | ||
| controller.add( | ||
| Nip85UserMetrics( | ||
| pubkey: latestPubkey ?? pubkey, | ||
| providerPubkey: latestProviderPubkey!, | ||
| createdAt: latestCreatedAt, | ||
| metrics: Map.from(aggregatedMetrics), | ||
| topics: topics.toList(), | ||
| ), | ||
| ); | ||
| } | ||
| }, | ||
| onError: (e) { | ||
| // Ignore errors from individual relays | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Stream never closes when all relay subscriptions complete.
The onCancel callback cleans up subscriptions when the consumer cancels, but there's no onDone handler on the relay streams. If all relays finish sending events (e.g., due to disconnection or subscription ending), the returned stream stays open indefinitely, potentially causing callers to hang.
Consider tracking active subscriptions and closing the controller when all complete:
Suggested approach
+ var activeCount = providersByRelay.length;
+
for (final entry in providersByRelay.entries) {
// ... existing setup ...
response.stream.listen(
(event) {
// ... existing logic ...
},
onError: (e) {
// Ignore errors from individual relays
},
+ onDone: () {
+ activeCount--;
+ if (activeCount == 0 && !controller.isClosed) {
+ controller.close();
+ }
+ },
);
}This same pattern applies to streamEventMetrics, streamAddressableMetrics, and streamExternalIdMetrics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ndk/lib/domain_layer/usecases/ta/trusted_assertions.dart` around
lines 188 - 226, The stream returned by the logic in response.stream.listen
never closes because there is no onDone handling: modify the subscription setup
in the block using response.stream.listen to increment a local
activeSubscriptions counter when each relay subscription starts and decrement it
in both onDone and onError (or use the listen's onDone callback), and when the
counter reaches zero call controller.close(); also ensure the existing onCancel
still cancels subscriptions; apply the same fix to streamEventMetrics,
streamAddressableMetrics, and streamExternalIdMetrics so each method tracks
active subscriptions and closes the controller when all relay streams complete.
Comment on lines
+464
to
+467
| onError: (e) { | ||
| print("Error in relay subscription for event metrics: $e"); | ||
| // Ignore errors from individual relays | ||
| }, |
There was a problem hiding this comment.
Remove debug print statement.
Line 465 contains a print() call that should be removed or replaced with proper structured logging before merging.
onError: (e) {
- print("Error in relay subscription for event metrics: $e");
// Ignore errors from individual relays
},📝 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.
Suggested change
| onError: (e) { | |
| print("Error in relay subscription for event metrics: $e"); | |
| // Ignore errors from individual relays | |
| }, | |
| onError: (e) { | |
| // Ignore errors from individual relays | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ndk/lib/domain_layer/usecases/ta/trusted_assertions.dart` around
lines 464 - 467, Remove the debug print in the onError closure used for the
relay subscription in trusted_assertions.dart: delete the print("Error in relay
subscription for event metrics: $e") call and either leave the comment "//
Ignore errors from individual relays" or replace the print with a structured
logging call using the project's logging utility (e.g., logger.error or
log.error) that logs the error object and a short context message; ensure you
reference the onError closure for the relay subscription handling so the change
is made in the correct callback.
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
3 participants
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
New Features
Documentation
Tests