Message history #38
Message history #38
Conversation
- Add MarmotGroup.id to easily get group id
- Introduced group history management with MarmotGroupHistoryStore for persistent message storage. - Updated group subscription logic to utilize the new history store, ensuring messages are saved and retrieved efficiently. - Improved error handling and type safety across group-related functionalities. - Added new dependencies and updated existing ones for better compatibility and performance.
WalkthroughIntroduces pluggable group history with IndexedDB-backed storage for rumors, refactors stores to use EventEmitter for notifications instead of callbacks, makes MarmotClient and MarmotGroup generic over history types, adds per-account database management, replaces direct store observables with derived event streams, and updates chat components to use the new history hooks. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
chat/package.json (1)
28-52: @types/react@19.2.10 does not exist; use 19.2.9 instead. Verify idb@8.0.3 breaking changes.
- @types/react@19.2.10 is not a published version (latest available is 19.2.9 from Jan 20, 2026). Update the version specification.
- idb@8.0.3 includes breaking changes: dropped support for legacy EdgeHTML-based Microsoft Edge and browsers without
IDBCursor.requestsupport; removed the separate async iterators build. Verify these changes are compatible with your target browser support.- react@19.2.4, react-dom@19.2.4, react-router@7.13.0 are routine security/patch updates with no breaking changes.
- Ensure the lockfile is updated to reflect these changes.
🤖 Fix all issues with AI agents
In `@chat/src/hooks/use-group-messages.ts`:
- Around line 20-27: The hook useGroupMessages currently omits addNewMessages
from its returned object despite JSDoc/examples showing it; update the return
type and the actual returned object of useGroupMessages to include
addNewMessages (the function that performs optimistic insertion of Rumor
messages into the group state), export or inline the existing addNewMessages
implementation used elsewhere in the hook, and ensure the signature matches the
documented API so callers can call addNewMessages alongside messages,
loadMoreMessages, loadingMore, and loadingDone.
- Around line 51-58: The loadMoreMessages callback can leave loadingMore stuck
true if paginatedLoader.next() throws; wrap the await call and subsequent logic
in a try/finally so setLoadingMore(false) always runs. Specifically, inside
loadMoreMessages (which references paginatedLoader, addNewMessages,
setLoadingDone) set loadingMore to true before the try, execute const page =
await paginatedLoader.next() and calls to addNewMessages(page.value) /
setLoadingDone(page.done) inside the try, and call setLoadingMore(false) in the
finally block to ensure the UI resets even on errors.
- Around line 73-81: The live "rumor" listener in useEffect currently appends
directly via setMessages which can cause duplicates/order issues; replace the
inline setMessages call inside the listener to call the existing addNewMessages
function (passing the incoming Rumor or an array containing it) so the same
deduplication and sorting logic used by pagination is applied; keep the listener
registration and cleanup on group.history.addListener("rumor", listener) and
removeListener unchanged but update the listener implementation to invoke
addNewMessages instead of setMessages.
In `@chat/src/lib/group-store.ts`:
- Around line 43-52: Update the JSDoc comment above groupHistoryStore$ to
correct the typo: replace "Group a group history store" with "Create a group
history store" so the comment clearly describes the purpose of the exported
observable that builds the MarmotGroupHistoryStore/IdbGroupHistoryStore based on
accountManager.active$.
In `@chat/src/lib/idb-group-history.ts`:
- Around line 62-76: The IDBKeyRange queries currently include the `until`
timestamp (making it inclusive) which violates the documented exclusive
semantics; update the `IDBKeyRange` calls that set an upper bound so the upper
bound is open/exclusive: in the branch using IDBKeyRange.bound([...], [...],
false, false) change the final argument to true, and in the branch using
IDBKeyRange.upperBound([groupKey, until], false) change the second argument to
true; keep the `since` lower-bound behavior unchanged. Ensure you modify the
usages around the variables `range`, `since`, `until`, `groupKey` and the calls
to `IDBKeyRange.bound` and `IDBKeyRange.upperBound`.
In `@chat/src/lib/runtime.ts`:
- Around line 28-45: The subscriber passed to marmotClient$.subscribe must
serialize start/stop operations and catch exceptions: add a small per-subscriber
guard (e.g., an in-scope boolean "running" or an incrementing "opId"/"seq"
token) to ensure that concurrent emissions cannot run overlapping await
groupMgr.start()/inviteMgr.start() or groupMgr.stop()/inviteMgr.stop() calls,
and wrap the entire async callback body in try-catch to handle and log errors;
locate the subscriber where marmotClient$.subscribe is called and apply this
guard around the branches that construct/stop GroupSubscriptionManager and
InvitationInboxManager and around await
groupMgr.start()/inviteMgr.start()/groupMgr.stop()/inviteMgr.stop() so only the
latest emission performs actions and exceptions are caught instead of leaking as
unhandled rejections.
In `@src/client/marmot-client.ts`:
- Around line 50-51: Fix the JSDoc typo "instaces" -> "instances" for the
group-related comments: update the comment above the groupHistory property (the
JSDoc for groupHistory in marmot-client.ts referencing
MarmotGroupOptions<THistoryStore>["history"]) and the other JSDoc occurrence
around lines 78-79 that refers to group instances so both comments read
"instances".
In `@src/store/marmot-group-history-store.ts`:
- Around line 87-121: The final page is being returned instead of yielded in
createPaginatedLoader, so for-await consumers miss it; change the logic so that
when a non-empty page has rumors.length < limit you yield that page first and
then return/stop the generator (e.g., replace the if (rumors.length < limit) {
return rumors; } with if (rumors.length < limit) { yield rumors; return; }),
keeping the existing early return for the empty page case (rumors.length === 0)
unchanged.
🧹 Nitpick comments (8)
chat/src/lib/invitation-inbox-manager.ts (1)
2-6: Useimport typeforEventSigner.
EventSigneris used only as a type annotation on line 28, so it should be imported with thetypekeyword to align with TypeScript conventions and project guidelines.♻️ Suggested change
-import { EventSigner, mapEventsToTimeline } from "applesauce-core"; +import { mapEventsToTimeline } from "applesauce-core"; +import type { EventSigner } from "applesauce-core";chat/src/pages/groups/create.tsx (1)
1-32: Move the RxJS import into the external-import block.
Keep external libs together before alias imports to match the project convention.♻️ Suggested import reordering
import { bytesToHex } from "@noble/hashes/utils.js"; import { use$ } from "applesauce-react/hooks"; import { Loader2, XCircle } from "lucide-react"; import { CompleteKeyPackage, createCredential, generateKeyPackage, } from "marmot-ts"; import { useEffect, useMemo, useState } from "react"; import { useNavigate } from "react-router"; +import { of, switchMap } from "rxjs"; import type { CiphersuiteName, KeyPackage } from "ts-mls"; import { defaultCryptoProvider, getCiphersuiteFromName, getCiphersuiteImpl, } from "ts-mls"; import { PubkeyListCreator } from "@/components/form/pubkey-list-creator"; @@ import { marmotClient$ } from "@/lib/marmot-client"; import { extraRelays$ } from "@/lib/settings"; -import { of, switchMap } from "rxjs";As per coding guidelines, external imports should appear before alias imports.
chat/src/lib/group-subscription-manager.ts (1)
3-11: Usetypekeyword for type-only imports.Per coding guidelines,
MarmotGroupandMarmotGroupHistoryStoreare used only as type annotations in this file and should be imported with thetypekeyword.♻️ Suggested fix
import { deserializeApplicationRumor, getNostrGroupIdHex, GROUP_EVENT_KIND, MarmotClient, - MarmotGroup, - MarmotGroupHistoryStore, } from "marmot-ts"; +import type { MarmotGroup, MarmotGroupHistoryStore } from "marmot-ts"; import { BehaviorSubject, Subscription } from "rxjs";chat/src/lib/group-store.ts (1)
44-49: Redundant optional chaining.On line 47,
account?.pubkeyuses optional chaining, butaccountis already guaranteed to be truthy by theaccount &&guard on line 46. Consider usingaccount.pubkeyfor consistency.♻️ Suggested fix
export const groupHistoryStore$ = accountManager.active$.pipe( map((account) => account && MarmotGroupHistoryStore.makeGroupHistoryInterface( - new IdbGroupHistoryStore(`${account?.pubkey}-group-history`), + new IdbGroupHistoryStore(`${account.pubkey}-group-history`), ) ),src/store/marmot-group-history-store.ts (1)
47-55: Silent error swallowing may hide deserialization issues.The
catchblock silently swallows errors without any logging. While it's reasonable to skip non-rumor messages, silently discarding errors could mask legitimate bugs or unexpected data formats during development.♻️ Suggested fix
async saveMessage(message: Uint8Array): Promise<void> { try { const rumor = deserializeApplicationRumor(message); await this.saveRumor(rumor); } catch (error) { - // Failed to read rumor, skip saving + // Failed to parse as MIP-03 rumor, skip saving + // This is expected for non-rumor application messages } }The current behavior is acceptable if non-rumor messages are expected. If debugging is needed later, consider adding a debug-level log.
chat/src/pages/groups/[id].tsx (1)
115-123: Unused props in MessageListProps interface.The props
loadMoreMessages,loadingMore, andloadingDoneare defined inMessageListPropsbut are not destructured or used in theMessageListcomponent. The load-more button is rendered outside the component (lines 649-662). Consider removing these unused props from the interface.♻️ Suggested fix
interface MessageListProps { messages: Rumor[]; currentUserPubkey: string | null; - loadMoreMessages?: () => Promise<void>; - loadingMore?: boolean; - loadingDone?: boolean; } function MessageList({ messages, currentUserPubkey }: MessageListProps) {src/client/group/marmot-group.ts (2)
205-209: Consider adding braces for consistency.The
elsebranch lacks braces while theifbranch has them. This is a minor style inconsistency.♻️ Suggested fix
// Create the history store if (typeof options.history === "function") { this.history = options.history(this.id); - } else this.history = options.history; + } else { + this.history = options.history; + }
746-752: History save failure could block message processing.If
this.history.saveMessage()throws an error, it will propagate up and potentially halt the entireingest()operation. Consider wrapping in try-catch to ensure message processing continues even if history persistence fails.♻️ Suggested fix
// Save application message to history if (this.history) { - await this.history.saveMessage(this.id, result.message); + try { + await this.history.saveMessage(this.id, result.message); + } catch (historyError) { + console.warn( + `[MarmotGroup.ingest] Failed to save message to history:`, + historyError, + ); + } }
| const loadMoreMessages = useCallback(async () => { | ||
| if (!paginatedLoader) return; | ||
| setLoadingMore(true); | ||
| const page = await paginatedLoader.next(); | ||
| addNewMessages(page.value); | ||
| if (page.done) setLoadingDone(page.done); | ||
| setLoadingMore(false); | ||
| }, [paginatedLoader, addNewMessages]); |
There was a problem hiding this comment.
Ensure loadingMore is reset if paging fails.
If paginatedLoader.next() throws, loadingMore stays true. Wrap in try/finally so the UI can recover. As per coding guidelines.
🛡️ Suggested fix
const loadMoreMessages = useCallback(async () => {
if (!paginatedLoader) return;
setLoadingMore(true);
- const page = await paginatedLoader.next();
- addNewMessages(page.value);
- if (page.done) setLoadingDone(page.done);
- setLoadingMore(false);
+ try {
+ const page = await paginatedLoader.next();
+ addNewMessages(page.value);
+ if (page.done) setLoadingDone(true);
+ } finally {
+ setLoadingMore(false);
+ }
}, [paginatedLoader, addNewMessages]);📝 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.
| const loadMoreMessages = useCallback(async () => { | |
| if (!paginatedLoader) return; | |
| setLoadingMore(true); | |
| const page = await paginatedLoader.next(); | |
| addNewMessages(page.value); | |
| if (page.done) setLoadingDone(page.done); | |
| setLoadingMore(false); | |
| }, [paginatedLoader, addNewMessages]); | |
| const loadMoreMessages = useCallback(async () => { | |
| if (!paginatedLoader) return; | |
| setLoadingMore(true); | |
| try { | |
| const page = await paginatedLoader.next(); | |
| addNewMessages(page.value); | |
| if (page.done) setLoadingDone(true); | |
| } finally { | |
| setLoadingMore(false); | |
| } | |
| }, [paginatedLoader, addNewMessages]); |
🤖 Prompt for AI Agents
In `@chat/src/hooks/use-group-messages.ts` around lines 51 - 58, The
loadMoreMessages callback can leave loadingMore stuck true if
paginatedLoader.next() throws; wrap the await call and subsequent logic in a
try/finally so setLoadingMore(false) always runs. Specifically, inside
loadMoreMessages (which references paginatedLoader, addNewMessages,
setLoadingDone) set loadingMore to true before the try, execute const page =
await paginatedLoader.next() and calls to addNewMessages(page.value) /
setLoadingDone(page.done) inside the try, and call setLoadingMore(false) in the
finally block to ensure the UI resets even on errors.
chat/src/lib/group-store.ts
Outdated
| /** Group a group history store based on the active account */ | ||
| export const groupHistoryStore$ = accountManager.active$.pipe( | ||
| map((account) => | ||
| account && MarmotGroupHistoryStore.makeGroupHistoryInterface( | ||
| new IdbGroupHistoryStore(`${account?.pubkey}-group-history`), | ||
| ) | ||
| ), | ||
| // Only create a single instance of the group history store | ||
| shareReplay(1), | ||
| ); |
There was a problem hiding this comment.
Fix typo in JSDoc comment.
The JSDoc comment has a typo: "Group a group history store" should be "Create a group history store".
📝 Suggested fix
-/** Group a group history store based on the active account */
+/** Create a group history store based on the active account */
export const groupHistoryStore$ = accountManager.active$.pipe(📝 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.
| /** Group a group history store based on the active account */ | |
| export const groupHistoryStore$ = accountManager.active$.pipe( | |
| map((account) => | |
| account && MarmotGroupHistoryStore.makeGroupHistoryInterface( | |
| new IdbGroupHistoryStore(`${account?.pubkey}-group-history`), | |
| ) | |
| ), | |
| // Only create a single instance of the group history store | |
| shareReplay(1), | |
| ); | |
| /** Create a group history store based on the active account */ | |
| export const groupHistoryStore$ = accountManager.active$.pipe( | |
| map((account) => | |
| account && MarmotGroupHistoryStore.makeGroupHistoryInterface( | |
| new IdbGroupHistoryStore(`${account?.pubkey}-group-history`), | |
| ) | |
| ), | |
| // Only create a single instance of the group history store | |
| shareReplay(1), | |
| ); |
🤖 Prompt for AI Agents
In `@chat/src/lib/group-store.ts` around lines 43 - 52, Update the JSDoc comment
above groupHistoryStore$ to correct the typo: replace "Group a group history
store" with "Create a group history store" so the comment clearly describes the
purpose of the exported observable that builds the
MarmotGroupHistoryStore/IdbGroupHistoryStore based on accountManager.active$.
chat/src/lib/runtime.ts
Outdated
| marmotClient$.subscribe( | ||
| async (client) => { | ||
| if (!client) { | ||
| // Client is not created, stop the managers | ||
| groupMgr?.stop(); | ||
| inviteMgr?.stop(); | ||
|
|
||
| groupMgr = null; | ||
| inviteMgr = null; | ||
| } else { | ||
| // Client is created, start the managers | ||
| groupMgr ??= new GroupSubscriptionManager(client); | ||
| inviteMgr ??= new InvitationInboxManager({ signer: client.signer }); | ||
|
|
||
| await groupMgr.start(); | ||
| await inviteMgr.start(); | ||
| } | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
RxJS subscribe async callback behavior multiple emissions error handling
💡 Result:
1) subscribe(async value => …) does not “await” between emissions
subscribe treats your next handler as a normal callback; its return value is ignored (including a returned Promise). So if the source emits A, B, C quickly, RxJS will call your handler for A/B/C immediately; it will not wait for the await in the prior call to finish. This is why Subject.next() (and other sources) won’t “backpressure” on an async subscriber. [2]
Also, callbacks passed to subscribe are not guaranteed to be async; an Observable may call them synchronously. [1]
Implication: you can easily get overlapping in-flight async work and out-of-order completion.
2) Error handling: “Observable errors” vs “errors inside your async next”
A) Errors emitted by the Observable
If the Observable sends an error notification, your subscribe({ error }) handler runs; if you don’t provide one, the error is effectively unhandled. [1]
To recover and keep a longer-lived stream alive, handle errors in the pipeline with catchError (e.g., return a fallback Observable/value). This is something the subscribe error callback can’t do because it’s already terminal. [3]
B) Errors thrown/rejected inside an async next handler
If you write:
source$.subscribe(async x => {
await doWork(x);
throw new Error('boom');
});that throw becomes a rejected Promise. RxJS won’t automatically route that rejection into the Observable’s error channel, so you can get an unhandled promise rejection instead. (In Node.js, unhandled rejections are treated as a serious unhandled error and have had escalating severity over time.) [5]
So you must handle it yourself:
source$.subscribe(x => {
void (async () => {
try { await doWork(x); }
catch (e) { /* handle/report */ }
})();
});3) The RxJS-native way: move async work into the stream
Instead of subscribe(async …), project each value to an Observable/Promise and control concurrency:
Sequential (wait for each):
source$.pipe(
concatMap(x => from(doWork(x))), // doWork returns Promise
catchError(err => of(/* fallback */))
).subscribe();Concurrent (allow overlap):
source$.pipe(
mergeMap(x => from(doWork(x)))
).subscribe();This way, errors propagate through RxJS and can be handled with catchError, and you get explicit control over ordering and concurrency. [3]
🏁 Script executed:
cat -n chat/src/lib/runtime.ts | head -60Repository: marmot-protocol/marmot-ts
Length of output: 1830
Guard async start/stop against overlapping emissions and add error handling.
When marmotClient$ emits multiple times rapidly (e.g., during login/logout cycles), the async subscriber will invoke start() and stop() calls concurrently without waiting for prior operations to complete. Additionally, errors thrown inside the async callback become unhandled promise rejections instead of being caught by RxJS, which violates the coding guideline to use try-catch blocks for expected failures.
✅ One way to serialize and guard starts
let groupMgr: GroupSubscriptionManager | null = null;
let inviteMgr: InvitationInboxManager | null = null;
+let runId = 0;
marmotClient$.subscribe(
- async (client) => {
- if (!client) {
- // Client is not created, stop the managers
- groupMgr?.stop();
- inviteMgr?.stop();
-
- groupMgr = null;
- inviteMgr = null;
- } else {
- // Client is created, start the managers
- groupMgr ??= new GroupSubscriptionManager(client);
- inviteMgr ??= new InvitationInboxManager({ signer: client.signer });
-
- await groupMgr.start();
- await inviteMgr.start();
- }
- },
+ (client) => {
+ const token = ++runId;
+ (async () => {
+ try {
+ if (!client) {
+ groupMgr?.stop();
+ inviteMgr?.stop();
+ groupMgr = null;
+ inviteMgr = null;
+ return;
+ }
+
+ groupMgr ??= new GroupSubscriptionManager(client);
+ inviteMgr ??= new InvitationInboxManager({ signer: client.signer });
+
+ await groupMgr.start();
+ if (token !== runId) return;
+ await inviteMgr.start();
+ } catch (err) {
+ console.error("Failed to start runtime managers", err);
+ }
+ })();
+ },
);🤖 Prompt for AI Agents
In `@chat/src/lib/runtime.ts` around lines 28 - 45, The subscriber passed to
marmotClient$.subscribe must serialize start/stop operations and catch
exceptions: add a small per-subscriber guard (e.g., an in-scope boolean
"running" or an incrementing "opId"/"seq" token) to ensure that concurrent
emissions cannot run overlapping await groupMgr.start()/inviteMgr.start() or
groupMgr.stop()/inviteMgr.stop() calls, and wrap the entire async callback body
in try-catch to handle and log errors; locate the subscriber where
marmotClient$.subscribe is called and apply this guard around the branches that
construct/stop GroupSubscriptionManager and InvitationInboxManager and around
await groupMgr.start()/inviteMgr.start()/groupMgr.stop()/inviteMgr.stop() so
only the latest emission performs actions and exceptions are caught instead of
leaking as unhandled rejections.
src/client/marmot-client.ts
Outdated
| /** The group history interface to be passed to group instaces */ | ||
| groupHistory: MarmotGroupOptions<THistoryStore>["history"]; |
There was a problem hiding this comment.
Fix typo in JSDoc comments.
"instaces" should be "instances" (appears on lines 51 and 78).
📝 Suggested fix
- /** The group history interface to be passed to group instaces */
+ /** The group history interface to be passed to group instances */
groupHistory: MarmotGroupOptions<THistoryStore>["history"];And similarly for line 78-79:
- /** Group history interface to be passed to group instaces */
+ /** Group history interface to be passed to group instances */
private groupHistory: MarmotGroupOptions<THistoryStore>["history"];📝 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.
| /** The group history interface to be passed to group instaces */ | |
| groupHistory: MarmotGroupOptions<THistoryStore>["history"]; | |
| /** The group history interface to be passed to group instances */ | |
| groupHistory: MarmotGroupOptions<THistoryStore>["history"]; |
🤖 Prompt for AI Agents
In `@src/client/marmot-client.ts` around lines 50 - 51, Fix the JSDoc typo
"instaces" -> "instances" for the group-related comments: update the comment
above the groupHistory property (the JSDoc for groupHistory in marmot-client.ts
referencing MarmotGroupOptions<THistoryStore>["history"]) and the other JSDoc
occurrence around lines 78-79 that refers to group instances so both comments
read "instances".
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 (4)
chat/src/lib/invitation-inbox-manager.ts (1)
2-29: Usetypemodifier forEventSignerimport.
EventSigneris only used in a type position (InvitationInboxManagerOptions on line 29), so import it with thetypekeyword to align with the coding guideline for this file pattern.♻️ Suggested change
-import { EventSigner, mapEventsToTimeline } from "applesauce-core"; +import { mapEventsToTimeline, type EventSigner } from "applesauce-core";chat/src/pages/key-packages/[id].tsx (1)
383-397: Potential undefinedpublicPackagepassed to button components.If
decodeKeyPackagereturnsnull(line 297),publicPackagewill beundefined. The type assertionas KeyPackageon lines 387 and 391 masks this, potentially passingundefinedtoPublishKeyPackageButtonandBroadcastKeyPackageButton, which could cause runtime errors.🐛 Suggested fix: guard against undefined publicPackage
<CardFooter className="flex gap-2"> - {isLocal ? ( + {isLocal && publicPackage ? ( <> <PublishKeyPackageButton event={event} - keyPackage={publicPackage as KeyPackage} + keyPackage={publicPackage} /> <BroadcastKeyPackageButton event={event} - keyPackage={publicPackage as KeyPackage} + keyPackage={publicPackage} /> <DeleteKeyPackageButton event={event} keyPackageRef={keyPackage.keyPackageRef} /> </> + ) : isLocal ? ( + <div className="text-sm text-muted-foreground"> + Failed to decode key package data. + </div> ) : (src/store/key-package-store.ts (1)
41-55: Update JSDoc example to reflect newkeyPackageTlsAPI.The example in the class documentation (line 46) shows
await store.add({ publicPackage, privatePackage }), but the API now expects{ keyPackageTls, privatePackage }. This could confuse consumers.📝 Suggested fix
* `@example` * ```typescript * const store = new KeyPackageStore(backend); * * // Add a complete key package - * await store.add({ publicPackage, privatePackage }); + * await store.add({ keyPackageTls: encodeKeyPackage(publicPackage), privatePackage }); * // List all key packages * const packages = await store.list();src/client/group/marmot-group.ts (1)
853-858: Advance history cursor even when a commit is rejected.Currently
actionTaken === "reject"short-circuits without marking the outer event as processed, which can stall resume cursors and cause repeated reprocessing of a permanently invalid commit.🛠️ Suggested fix
- if (result.actionTaken === "reject") { - continue; - } + if (result.actionTaken === "reject") { + if (this.history) { + const outer: OuterCursor = { + created_at: event.created_at, + id: event.id, + }; + try { + await this.history.markOuterEventProcessed(outer); + } catch { + // best-effort + } + } + continue; + }
🤖 Fix all issues with AI agents
In `@chat/src/lib/group-subscription-manager.ts`:
- Around line 239-260: The code assumes group.history exists before calling
group.history.getResumeCursor(); add a defensive null/undefined guard for
group.history (e.g., if (!group.history) { /* skip backfill: set resume
undefined or return/continue */ }) so you don't call getResumeCursor on
undefined; ensure downstream logic that uses resume (the filters.since
assignment and events filtering) works when resume is undefined by preserving
the existing behavior of treating resume as absent.
In `@chat/src/pages/groups/`[id].tsx:
- Around line 529-531: The Tailwind class string on the <pre> element containing
jsonStringifySafe(groupDetails.state) uses an invalid utility
"wrap-break-words"; replace that token with the standard Tailwind class
"break-all" in the className for the <pre> element so it becomes "text-xs
whitespace-pre-wrap break-all border rounded p-3 bg-muted/30".
In `@src/__tests__/ingest-watermark-mixed-messages.test.ts`:
- Around line 79-81: Update MemoryHistoryStore.queryRumors to match the
GroupHistoryStore signature by adding the required filter parameter and
appropriate type (e.g., change async queryRumors(): Promise<Rumor[]> to async
queryRumors(filter: RumorFilter): Promise<Rumor[]> or the exact filter type used
by GroupHistoryStore), and implement a minimal body (for tests you can ignore
the filter and return an empty Rumor[] or forward the filter to the real logic)
so the method conforms to the interface; reference
MemoryHistoryStore.queryRumors (and GroupHistoryStore.queryRumors) when making
the change.
🧹 Nitpick comments (11)
chat/src/lib/lifecycle.ts (2)
84-95: Consider logging parse failures for debugging.The catch block silently swallows errors, which makes it difficult to diagnose issues with malformed key package events. While filtering out invalid events is correct behavior, a warning log would help identify problems during development.
🔧 Suggested improvement
const parseEvent = async (event: NostrEvent) => { try { const keyPackage = getKeyPackage(event); const keyPackageRef = await calculateKeyPackageRef( keyPackage, client?.cryptoProvider, ); return { event, keyPackage, keyPackageRef } as PublishedKeyPackage; - } catch { + } catch (error) { + console.warn(`Failed to parse key package event ${event.id}:`, error); return null; } };
106-111: Deduplication efficiency could be improved with a Map.The current
Array.some()lookup is O(n) per event. For small key package counts this is fine, but a Map-based approach would be O(1) and prevent subtle performance degradation as the array grows.🔧 Optional optimization using Map
- const published = merge(existing$, updates$).pipe( - scan((acc, curr) => { - const seen = acc.some((p) => p.event.id === curr.event.id); - return seen ? acc : [...acc, curr]; - }, [] as PublishedKeyPackage[]), - ); + const published = merge(existing$, updates$).pipe( + scan( + (acc, curr) => { + if (acc.seen.has(curr.event.id)) return acc; + acc.seen.add(curr.event.id); + return { seen: acc.seen, list: [...acc.list, curr] }; + }, + { seen: new Set<string>(), list: [] as PublishedKeyPackage[] }, + ), + map((acc) => acc.list), + );chat/src/pages/key-packages/[id].tsx (1)
33-35: Import ordering: external library imports should come before path alias imports.The
decodeKeyPackageimport fromts-mls/keyPackage.jsis an external library import and should be placed with the other external imports (lines 1-13), not after the@/path alias imports.♻️ Suggested fix
Move the import up to line 13 (after the other ts-mls import):
import { KeyPackage } from "ts-mls"; +import { decodeKeyPackage } from "ts-mls/keyPackage.js"; import accounts, { publish, user$ } from "../../lib/accounts"; -import { decodeKeyPackage } from "ts-mls/keyPackage.js";As per coding guidelines: "Order imports as: external libraries first, then path alias imports (
@/*), then relative imports"chat/src/pages/key-packages.tsx (2)
4-6: Import ordering: group external library imports together.The
decodeKeyPackageimport fromts-mls/keyPackage.jsshould be grouped with other external library imports, separate from React imports.♻️ Suggested fix
import { bytesToHex, relaySet, type NostrEvent } from "applesauce-core/helpers"; import { use$ } from "applesauce-react/hooks"; import { Plus } from "lucide-react"; import { getKeyPackageClient, ListedKeyPackage } from "marmot-ts"; +import { decodeKeyPackage } from "ts-mls/keyPackage.js"; import { useMemo } from "react"; -import { decodeKeyPackage } from "ts-mls/keyPackage.js";As per coding guidelines: "Order imports as: external libraries first, then path alias imports (
@/*), then relative imports"
72-78: Consider memoizing the decoded key package to avoid repeated parsing.The IIFE decodes
keyPackageTlson every render. While likely fast, this could be optimized by memoizing the result, especially sinceKeyPackageItemis rendered for each item in a list.♻️ Suggested refactor
function KeyPackageItem({ keyPackage, event, }: { keyPackage: ListedKeyPackage & { isLocal?: boolean }; event?: NostrEvent; }) { const location = useLocation(); const linkTo = useMemo( () => `/key-packages/${bytesToHex(keyPackage.keyPackageRef)}`, [keyPackage.keyPackageRef], ); const isActive = location.pathname === linkTo; const client = event ? getKeyPackageClient(event) : undefined; const timeAgo = event ? formatTimeAgo(event.created_at) : "Unpublished"; + const cipherSuiteId = useMemo(() => { + const decoded = decodeKeyPackage(keyPackage.keyPackageTls, 0); + return decoded?.[0]?.cipherSuite; + }, [keyPackage.keyPackageTls]); return ( <Link ... <div className="flex items-center gap-2"> - {(() => { - const decoded = decodeKeyPackage(keyPackage.keyPackageTls, 0); - const [publicPackage] = decoded ?? []; - return publicPackage ? ( - <CipherSuiteBadge cipherSuite={publicPackage.cipherSuite} /> - ) : null; - })()} + {cipherSuiteId !== undefined && ( + <CipherSuiteBadge cipherSuite={cipherSuiteId} /> + )} </div>src/store/marmot-group-history-store.ts (1)
115-128: Consider logging failed message parsing for debugging purposes.The
saveMessagemethod silently swallows parsing errors (line 125-126). While the intent to skip invalid messages is documented, completely suppressing errors can make debugging difficult. Consider logging at debug level.♻️ Suggested enhancement
async saveMessage(message: Uint8Array): Promise<void> { try { const rumor = deserializeApplicationRumor(message); // Best-effort placeholder: use rumor metadata as a stable, composite cursor. // This avoids timestamp-only pagination loss. const outer: OuterCursor = { created_at: rumor.created_at, id: rumor.id, }; await this.addRumor({ rumor, outer }); - } catch { + } catch (err) { // Failed to read rumor, skip saving + // Uncomment for debugging: console.debug("saveMessage: failed to parse rumor", err); } }src/store/key-package-store.ts (1)
113-115: Consider extracting repeated decode-and-validate pattern to a private helper.The same decode pattern appears in
ensureKeyPackageRef(lines 113-115),add(lines 142-144), andgetPublicKey(lines 181-183). Extracting to a helper would reduce duplication and centralize error handling.♻️ Suggested refactor
// Add as a private method: private decodePublicPackage(keyPackageTls: Uint8Array): KeyPackage { const decoded = decodeKeyPackage(keyPackageTls, 0); if (!decoded) { throw new Error("Failed to decode key package TLS bytes"); } return decoded[0]; } // Then simplify usages: // In ensureKeyPackageRef: const publicPackage = this.decodePublicPackage(keyPackage.keyPackageTls); // In add: const publicPackage = this.decodePublicPackage(keyPackage.keyPackageTls); // In getPublicKey: return this.decodePublicPackage(stored.keyPackageTls);Also applies to: 142-144, 181-183
chat/src/pages/groups/create.tsx (1)
1-33: Reorder imports to keep external libs grouped.
rxjsshould sit with other external imports before alias imports.♻️ Proposed diff
import { useEffect, useMemo, useState } from "react"; import { useNavigate } from "react-router"; import type { CiphersuiteName, KeyPackage } from "ts-mls"; import { defaultCryptoProvider, getCiphersuiteFromName, getCiphersuiteImpl, } from "ts-mls"; import { decodeKeyPackage } from "ts-mls/keyPackage.js"; +import { of, switchMap } from "rxjs"; import { PubkeyListCreator } from "@/components/form/pubkey-list-creator"; import { RelayListCreator } from "@/components/form/relay-list-creator"; @@ import { marmotClient$ } from "@/lib/marmot-client"; import { extraRelays$ } from "@/lib/settings"; -import { of, switchMap } from "rxjs";As per coding guidelines, “Order imports as: external libraries first, then path alias imports (
@/*), then relative imports”.chat/src/pages/groups/[id].tsx (3)
115-123: Unused props inMessageListPropsinterface.
loadMoreMessages,loadingMore, andloadingDoneare declared in the interface but not destructured or used in theMessageListcomponent. The load-more button is rendered separately in the parent. Consider removing these props from the interface if they're not intended for use here.♻️ Option 1: Remove unused props
interface MessageListProps { messages: Rumor[]; currentUserPubkey: string | null; - loadMoreMessages?: () => Promise<void>; - loadingMore?: boolean; - loadingDone?: boolean; }
287-293: FrequentmarkGroupSeencalls on every message.The effect runs on every
messages.lengthchange, callingmarkGroupSeen(groupIdHex, unixNow())each time. This may be intentional for keeping the group marked as seen while viewing, but consider if this is necessary for every single message arrival.
655-668: Consider error handling forloadMoreMessages.If
loadMoreMessagesthrows, the error is not caught or displayed. Consider wrapping in try-catch with user feedback.🛡️ Suggested error handling
- <Button onClick={loadMoreMessages} disabled={loadingMore}> + <Button + onClick={async () => { + try { + await loadMoreMessages(); + } catch (err) { + console.error('Failed to load older messages:', err); + } + }} + disabled={loadingMore} + >
| try { | ||
| const filters = { | ||
| const resume = await group.history.getResumeCursor(); | ||
|
|
||
| const filters: Filter = { | ||
| kinds: [GROUP_EVENT_KIND], | ||
| "#h": [groupIdHex], | ||
| }; | ||
|
|
||
| const events = await this.client.network.request(relays, filters); | ||
| // Best-effort incremental backfill: relays support `since` by timestamp only. | ||
| // We still apply composite `(created_at, id)` inclusivity locally. | ||
| if (resume) { | ||
| filters.since = resume.created_at; | ||
| } | ||
|
|
||
| const eventsAll = await this.client.network.request(relays, filters); | ||
| const events = resume | ||
| ? eventsAll.filter( | ||
| (e) => | ||
| e.created_at > resume.created_at || | ||
| (e.created_at === resume.created_at && e.id > resume.id), | ||
| ) | ||
| : eventsAll; |
There was a problem hiding this comment.
Add null check for group.history before accessing.
group.history could be undefined. While the type constraint MarmotGroup<MarmotGroupHistoryStore> suggests it should exist, adding a guard improves robustness.
🛡️ Suggested defensive check
try {
+ if (!group.history) {
+ console.warn(`[GroupSubscriptionManager] No history store for group ${groupIdHex}`);
+ return;
+ }
const resume = await group.history.getResumeCursor();📝 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.
| try { | |
| const filters = { | |
| const resume = await group.history.getResumeCursor(); | |
| const filters: Filter = { | |
| kinds: [GROUP_EVENT_KIND], | |
| "#h": [groupIdHex], | |
| }; | |
| const events = await this.client.network.request(relays, filters); | |
| // Best-effort incremental backfill: relays support `since` by timestamp only. | |
| // We still apply composite `(created_at, id)` inclusivity locally. | |
| if (resume) { | |
| filters.since = resume.created_at; | |
| } | |
| const eventsAll = await this.client.network.request(relays, filters); | |
| const events = resume | |
| ? eventsAll.filter( | |
| (e) => | |
| e.created_at > resume.created_at || | |
| (e.created_at === resume.created_at && e.id > resume.id), | |
| ) | |
| : eventsAll; | |
| try { | |
| if (!group.history) { | |
| throw new Error(`[GroupSubscriptionManager] No history store for group ${groupIdHex}`); | |
| } | |
| const resume = await group.history.getResumeCursor(); | |
| const filters: Filter = { | |
| kinds: [GROUP_EVENT_KIND], | |
| "#h": [groupIdHex], | |
| }; | |
| // Best-effort incremental backfill: relays support `since` by timestamp only. | |
| // We still apply composite `(created_at, id)` inclusivity locally. | |
| if (resume) { | |
| filters.since = resume.created_at; | |
| } | |
| const eventsAll = await this.client.network.request(relays, filters); | |
| const events = resume | |
| ? eventsAll.filter( | |
| (e) => | |
| e.created_at > resume.created_at || | |
| (e.created_at === resume.created_at && e.id > resume.id), | |
| ) | |
| : eventsAll; |
🤖 Prompt for AI Agents
In `@chat/src/lib/group-subscription-manager.ts` around lines 239 - 260, The code
assumes group.history exists before calling group.history.getResumeCursor(); add
a defensive null/undefined guard for group.history (e.g., if (!group.history) {
/* skip backfill: set resume undefined or return/continue */ }) so you don't
call getResumeCursor on undefined; ensure downstream logic that uses resume (the
filters.since assignment and events filtering) works when resume is undefined by
preserving the existing behavior of treating resume as absent.
| <pre className="text-xs whitespace-pre-wrap wrap-break-words border rounded p-3 bg-muted/30"> | ||
| {jsonStringifySafe(groupDetails.state)} | ||
| </pre> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for wrap-break-words class definition
rg "wrap-break-words" -t css -t scss -t ts -t tsx -t js -t jsxRepository: marmot-protocol/marmot-ts
Length of output: 97
🏁 Script executed:
# Also check for Tailwind configuration and global styles
find . -type f \( -name "tailwind.config.*" -o -name "globals.css" -o -name "globals.scss" \) | head -20Repository: marmot-protocol/marmot-ts
Length of output: 51
🏁 Script executed:
# Examine the specific file context around lines 529-531
sed -n '520,540p' chat/src/pages/groups/[id].tsxRepository: marmot-protocol/marmot-ts
Length of output: 845
🏁 Script executed:
# Search for wrap-break-words in all files
rg "wrap-break-words"Repository: marmot-protocol/marmot-ts
Length of output: 217
🏁 Script executed:
# Find Tailwind config and CSS files
find . -type f -name "tailwind.config.*" -o -name "*.css" -o -name "*.scss" | grep -v node_modules | head -30Repository: marmot-protocol/marmot-ts
Length of output: 114
🏁 Script executed:
# Check for CSS/SCSS in src directory specifically
find ./chat -type f \( -name "*.css" -o -name "*.scss" \) 2>/dev/nullRepository: marmot-protocol/marmot-ts
Length of output: 89
🏁 Script executed:
# Check the CSS file content
cat ./chat/src/index.cssRepository: marmot-protocol/marmot-ts
Length of output: 4456
🏁 Script executed:
# Look for tailwind config file more thoroughly
find . -type f -name "tailwind.config*" 2>/dev/null | grep -v node_modulesRepository: marmot-protocol/marmot-ts
Length of output: 51
🏁 Script executed:
# Search for break-words usage in the project (standard Tailwind)
rg "break-words" --type ts --type tsx --type jsx --type jsRepository: marmot-protocol/marmot-ts
Length of output: 96
🏁 Script executed:
# Search for break-all usage
rg "break-all"Repository: marmot-protocol/marmot-ts
Length of output: 5038
🏁 Script executed:
# Check for any other custom classes with similar patterns
rg "wrap-" --type cssRepository: marmot-protocol/marmot-ts
Length of output: 51
🏁 Script executed:
# Verify whitespace-pre-wrap is standard Tailwind
rg "whitespace-pre-wrap"Repository: marmot-protocol/marmot-ts
Length of output: 427
Replace wrap-break-words with break-all.
wrap-break-words is not a valid Tailwind CSS class and is not defined as a custom utility in this project. Replace it with the standard Tailwind class break-all, which is used consistently throughout the codebase for breaking long words in similar contexts.
🤖 Prompt for AI Agents
In `@chat/src/pages/groups/`[id].tsx around lines 529 - 531, The Tailwind class
string on the <pre> element containing jsonStringifySafe(groupDetails.state)
uses an invalid utility "wrap-break-words"; replace that token with the standard
Tailwind class "break-all" in the className for the <pre> element so it becomes
"text-xs whitespace-pre-wrap break-all border rounded p-3 bg-muted/30".
| async queryRumors(): Promise<Rumor[]> { | ||
| throw new Error("not needed"); | ||
| } |
There was a problem hiding this comment.
Match GroupHistoryStore.queryRumors signature.
MemoryHistoryStore.queryRumors() currently has no parameters, but the interface requires a filter argument. This can fail TypeScript conformance checks.
🛠️ Suggested fix
- async queryRumors(): Promise<Rumor[]> {
+ async queryRumors(
+ _filter: { until?: OuterCursor; limit?: number },
+ ): Promise<Rumor[]> {
throw new Error("not needed");
}📝 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.
| async queryRumors(): Promise<Rumor[]> { | |
| throw new Error("not needed"); | |
| } | |
| async queryRumors( | |
| _filter: { until?: OuterCursor; limit?: number }, | |
| ): Promise<Rumor[]> { | |
| throw new Error("not needed"); | |
| } |
🤖 Prompt for AI Agents
In `@src/__tests__/ingest-watermark-mixed-messages.test.ts` around lines 79 - 81,
Update MemoryHistoryStore.queryRumors to match the GroupHistoryStore signature
by adding the required filter parameter and appropriate type (e.g., change async
queryRumors(): Promise<Rumor[]> to async queryRumors(filter: RumorFilter):
Promise<Rumor[]> or the exact filter type used by GroupHistoryStore), and
implement a minimal body (for tests you can ignore the filter and return an
empty Rumor[] or forward the filter to the real logic) so the method conforms to
the interface; reference MemoryHistoryStore.queryRumors (and
GroupHistoryStore.queryRumors) when making the change.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/src/components/error-boundary.tsx (1)
7-26: UseFallbackPropstype from react-error-boundary instead ofunknown.The library guarantees error is of type
Error, notunknown. Import and useFallbackPropsto get proper typing and eliminate the need for casting:♻️ Proposed fix
-import { ErrorBoundary as ReactErrorBoundary } from "react-error-boundary"; +import { ErrorBoundary as ReactErrorBoundary, type FallbackProps } from "react-error-boundary"; -function ErrorFallback({ error }: { error: unknown }) { +function ErrorFallback({ error }: FallbackProps) { return ( <div className="alert alert-error">- <div className="text-sm">{(error as Error).message}</div> + <div className="text-sm">{error.message}</div>
🤖 Fix all issues with AI agents
In `@examples/src/examples/key-package/create.tsx`:
- Around line 388-396: Replace the UTF-8 JSON encoding with the library TLS
encoder: import encodeKeyPackage from "ts-mls/keyPackage.js" and call
encodeKeyPackage(keyPackage.publicPackage) when building the object passed to
keyPackageStore.add (instead of TextEncoder + JSON.stringify); this will produce
the TLS wire-format bytes expected by decodeKeyPackage(keyPackage.keyPackageTls,
0).
🧹 Nitpick comments (1)
examples/src/App.tsx (1)
93-96: Remove redundant type cast in ErrorBoundary fallbackRender.In
react-error-boundaryv6.0.2, theerrorparameter is already typed asError, making the cast(error as Error)redundant. Simplify to{error.message}directly in both fallbackRender blocks.♻️ Proposed fix
<ErrorBoundary fallbackRender={({ error }) => ( - <div className="text-red-500">{(error as Error).message}</div> + <div className="text-red-500">{error.message}</div> )} >Also applies to: 101-104
| // Extract the parts needed for storage - encode public package to TLS bytes | ||
| const encoder = new TextEncoder(); | ||
| // Create a minimal representation - the store will handle encoding | ||
| const key = await keyPackageStore.add({ | ||
| keyPackageTls: encoder.encode( | ||
| JSON.stringify(keyPackage.publicPackage), | ||
| ), | ||
| privatePackage: keyPackage.privatePackage, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how keyPackageStore handles keyPackageTls in the codebase
echo "=== Checking key-package-store implementation ==="
rg -n -A5 "keyPackageTls" --type ts
echo ""
echo "=== Looking for encodeKeyPackage usage ==="
rg -n "encodeKeyPackage" --type ts
echo ""
echo "=== Check ts-mls keyPackage exports ==="
ast-grep --pattern 'export { $$$encodeKeyPackage$$$ }'Repository: marmot-protocol/marmot-ts
Length of output: 9255
Use encodeKeyPackage for TLS encoding instead of JSON.stringify.
The store's decodeKeyPackage(keyPackage.keyPackageTls, 0) expects TLS-encoded wire format bytes, but the code uses encoder.encode(JSON.stringify(keyPackage.publicPackage)), which produces UTF-8 JSON bytes. This mismatch will cause decoding to fail at runtime.
All other similar files in the codebase correctly use encodeKeyPackage(keyPackage.publicPackage) from "ts-mls/keyPackage.js". Import this function and use it here as well.
🤖 Prompt for AI Agents
In `@examples/src/examples/key-package/create.tsx` around lines 388 - 396, Replace
the UTF-8 JSON encoding with the library TLS encoder: import encodeKeyPackage
from "ts-mls/keyPackage.js" and call encodeKeyPackage(keyPackage.publicPackage)
when building the object passed to keyPackageStore.add (instead of TextEncoder +
JSON.stringify); this will produce the TLS wire-format bytes expected by
decodeKeyPackage(keyPackage.keyPackageTls, 0).
Fix client and group class generic types
Make all UI components rely on MarmotClient class
hzrd149
force-pushed
the
message-history
branch
from
59c2c0d to
ad001ae
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
chat/src/lib/marmot-client.ts (1)
1-23: Convert type-only imports toimport type.These symbols are only used in type annotations:
NostrEvent,GroupRumorHistory,MarmotGroup,NostrNetworkInterface,PublishResponse, andObservable. Separating them withimport typekeeps runtime imports clean.Suggested update
-import { NostrEvent } from "applesauce-core/helpers/event"; +import type { NostrEvent } from "applesauce-core/helpers/event"; import { - GroupRumorHistory, - MarmotClient, - MarmotGroup, - NostrNetworkInterface, - PublishResponse, + MarmotClient, } from "marmot-ts"; +import type { + GroupRumorHistory, + MarmotGroup, + NostrNetworkInterface, + PublishResponse, +} from "marmot-ts"; import { firstValueFrom, from, fromEvent, lastValueFrom, map, merge, of, shareReplay, startWith, switchMap, } from "rxjs"; +import type { Observable } from "rxjs";Per coding guidelines for
chat/**/*.{ts,tsx}.
🤖 Fix all issues with AI agents
In `@chat/src/lib/account-database.ts`:
- Around line 137-158: getCustomDatabaseForAccount currently caches the raw
promise from openDB so a failed open leaves a rejected promise in
customDatabases; update getCustomDatabaseForAccount to create the openDB
promise, immediately store that transient promise if you want deduplication,
then await it inside a try/catch: on success replace the cache entry with the
resolved IDBPDatabase and return it, on failure delete
customDatabases.delete(pubkey) and throw a descriptive Error (include pubkey and
original error) so callers can retry; reference the getCustomDatabaseForAccount
method, the customDatabases Map, and the openDB call when making this change.
In `@chat/src/lib/marmot-client.ts`:
- Around line 66-83: The pipe for marmotClient$ currently calls
databaseBroker.getStorageInterfacesForAccount without error handling, which can
cause the observable to error and stop; wrap the await
databaseBroker.getStorageInterfacesForAccount(account.pubkey) call in a
try-catch inside the switchMap(async (account) => { ... }) block, log the caught
error with context (e.g., include account.pubkey and that
getStorageInterfacesForAccount failed), and return undefined so the observable
stream (accounts.active$ -> marmotClient$) stays alive instead of terminating;
keep the rest of the function returning new MarmotClient({ signer:
account.signer, groupStore, keyPackageStore, network: networkInterface,
historyFactory }) when successful.
In `@chat/src/pages/groups.tsx`:
- Around line 3-4: The imports for MarmotGroup and GroupRumorHistory are only
used as types; change the import to a type-only import so they are not emitted
at runtime: replace the current import of MarmotGroup and GroupRumorHistory from
"marmot-ts" with a type import (reference MarmotGroup and GroupRumorHistory) to
ensure only type information is imported while leaving any runtime imports
(e.g., use$ from "applesauce-react/hooks") unchanged.
In `@chat/src/pages/groups/`[id].tsx:
- Around line 1-12: Change the runtime imports of the purely-typed symbols to
type-only imports: replace the current runtime import of Rumor from
"applesauce-common/helpers/gift-wrap" with an "import type { Rumor }" and
replace the runtime import of MarmotGroup from "marmot-ts" with "import type {
MarmotGroup }"; keep the other values from those modules (e.g.,
extractMarmotGroupData, getGroupMembers, getNostrGroupIdHex, unixNow) as normal
imports so only Rumor and MarmotGroup are moved to type-only imports.
In `@src/client/group/group-rumor-history.ts`:
- Around line 84-105: createPaginatedLoader currently calls
this.backend.queryRumors with only until and limit, dropping caller-provided
filters (since, authors, kinds, tags, etc.); update the call in
createPaginatedLoader so you merge the original filter into the query parameters
and only override limit and until (e.g., pass {...filter, until: cursor, limit})
when calling this.backend.queryRumors so pagination preserves all caller filters
while still controlling page size and cursor.
In `@src/client/group/marmot-group.ts`:
- Around line 54-58: The interface BaseGroupHistory declares a misspelled method
prugeMessages; rename it to purgeMessages in the interface and update all
implementations/usages (e.g., any classes implementing BaseGroupHistory and
calls to prugeMessages) to the correct name purgeMessages (also fix the other
occurrence mentioned). Ensure method signatures remain the same (Promise<void>)
and run the typechecker to catch any missed references.
- Around line 799-806: Wrap the call to this.history.saveMessage(result.message)
in its own try/catch so history persistence is treated as best-effort: call
await this.history.saveMessage(result.message) inside try, catch and log the
error (e.g. this.logger?.error(...) or this.emit("historyError", err)) but do
NOT rethrow or change control flow; continue to yield result and
emit("applicationMessage", result.message) as before so a save failure won't
mark the message unreadable or cause retries.
In `@src/client/marmot-client.ts`:
- Around line 220-238: The destroyGroup method currently calls group.destroy()
which already removes the group from the persistent store, and then calls
this.groupStore.remove(groupId) without awaiting it, causing redundant work and
potential unhandled rejections; remove the redundant this.groupStore.remove(...)
call and instead await the in-memory cache deletion and event emission after the
awaited group.destroy() completes (i.e., keep await group.destroy(), then
this.#groups.delete(id) and emit "groupDestroyed" and "groupsUpdated"), ensuring
you reference the same id variable derived from groupId and do not call
groupStore.remove anywhere in destroyGroup.
In `@src/store/group-store.ts`:
- Around line 92-100: The update() method currently emits the wrong event:
instead of emitting clientStateUpdated it emits clientStateAdded; locate the
update(clientState: ClientState) function and change the emitted event to
"clientStateUpdated" (or, if intent is upsert semantics, remove the
clientStateUpdated entry from GroupStoreEvents and keep emitting
clientStateAdded) so subscribers receive the correct notification; use the
existing symbols resolveStorageKey, serializeClientState, backend.setItem, and
this.emit(...) in your change to ensure consistency with GroupStoreEvents.
🧹 Nitpick comments (5)
src/store/key-package-store.ts (1)
155-157: Avoid emitting private key material to listeners.
keyPackageAddedcurrently emits aStoredKeyPackage(includesprivatePackage). Consider emittingListedKeyPackageor just the ref and let consumers fetch private data explicitly when needed.♻️ Suggested adjustment to minimize exposure
type KeyPackageStoreEvents = { - keyPackageAdded: (keyPackage: StoredKeyPackage) => any; + keyPackageAdded: (keyPackage: ListedKeyPackage) => any; /** Emitted when a key package is removed */ keyPackageRemoved: (keyPackageRef: Uint8Array) => any; }; const stored = await this.ensureKeyPackageRef(serialized); - this.emit("keyPackageAdded", stored); + this.emit("keyPackageAdded", { + keyPackageRef: stored.keyPackageRef, + publicPackage: stored.publicPackage, + });chat/src/lib/lifecycle.ts (1)
106-111: Consider using a Set for deduplication if event volume grows.The current linear scan (
acc.some(...)) is O(n) per event, resulting in O(n²) overall for n events. For typical key package counts this is fine, but if volumes grow significantly, consider using aMapkeyed byevent.idfor O(1) lookups.♻️ Optional optimization using Map
- const published = merge(existing$, updates$).pipe( - scan((acc, curr) => { - const seen = acc.some((p) => p.event.id === curr.event.id); - return seen ? acc : [...acc, curr]; - }, [] as PublishedKeyPackage[]), + const published = merge(existing$, updates$).pipe( + scan( + (acc, curr) => { + if (acc.map.has(curr.event.id)) return acc; + acc.map.set(curr.event.id, curr); + return { map: acc.map, list: [...acc.list, curr] }; + }, + { map: new Map<string, PublishedKeyPackage>(), list: [] as PublishedKeyPackage[] }, + ), + map((acc) => acc.list), );examples/src/lib/key-package-store.ts (1)
16-16:storeChanges$appears to be dead code.The
storeChanges$BehaviorSubject is initialized but never updated anywhere in the file. With the new event-based pattern usingfromEvent(lines 34-35), this subject seems to serve no purpose. ThekeyPackageCount$observable still references it, but since it never emits new values, it effectively only triggers once on initial subscription.Consider removing
storeChanges$and updatingkeyPackageCount$to use the event-based pattern instead.♻️ Proposed fix to remove dead code
-// Observable that triggers whenever the store changes -const storeChanges$ = new BehaviorSubject<number>(0); - // Create and export a shared KeyPackageStore instance export const keyPackageStore$ = accounts.active$.pipe( // ...existing code... ); // Observable for the count of key packages in the store // This will automatically update when the store changes export const keyPackageCount$ = keyPackageStore$.pipe( - combineLatestWith(storeChanges$), - switchMap(([store, _]) => store.count()), + switchMap((store) => store.count()), );src/store/group-store.ts (1)
73-81: Consider extracting shared logic betweenadd()andupdate().Both methods now have nearly identical implementations (serialize, store, emit). If the semantic difference between add/update is minimal, consider consolidating into a single internal method.
♻️ Optional consolidation
+ private async persist(clientState: ClientState, event: "clientStateAdded" | "clientStateUpdated"): Promise<string> { + const key = this.resolveStorageKey(clientState.groupContext.groupId); + const storedClientState = serializeClientState(clientState); + await this.backend.setItem(key, storedClientState); + this.emit(event, clientState); + return key; + } + async add(clientState: ClientState): Promise<string> { - const key = this.resolveStorageKey(clientState.groupContext.groupId); - const storedClientState = serializeClientState(clientState); - - await this.backend.setItem(key, storedClientState); - this.emit("clientStateAdded", clientState); - - return key; + return this.persist(clientState, "clientStateAdded"); } async update(clientState: ClientState): Promise<string> { - const key = this.resolveStorageKey(clientState.groupContext.groupId); - const storedClientState = serializeClientState(clientState); - - await this.backend.setItem(key, storedClientState); - this.emit("clientStateAdded", clientState); - - return key; + return this.persist(clientState, "clientStateUpdated"); }Also applies to: 92-100
chat/src/pages/groups.tsx (1)
101-106: Add JSDoc for the exported page component.Suggested update
+/** Groups landing page. */ export default function GroupsPage() {As per coding guidelines,
chat/**/*.{ts,tsx,js,jsx}: Use JSDoc documentation for exported functions and hooks.
| /** Get, open, or create a database for a given account */ | ||
| private async getCustomDatabaseForAccount( | ||
| pubkey: string, | ||
| ): Promise<IDBPDatabase<RumorDatabaseSchema>> { | ||
| const existing = this.customDatabases.get(pubkey); | ||
| if (existing) return existing; | ||
|
|
||
| // Create a new database for the account | ||
| const db = openDB<RumorDatabaseSchema>(pubkey, DB_VERSION, { | ||
| upgrade(db) { | ||
| const rumors = db.createObjectStore("rumors", { | ||
| keyPath: ["groupId", "id"], | ||
| }); | ||
| rumors.createIndex("by_group_created_at", ["groupId", "created_at"]); | ||
| }, | ||
| }).then((open) => { | ||
| this.customDatabases.set(pubkey, open); | ||
| return open; | ||
| }); | ||
|
|
||
| this.customDatabases.set(pubkey, db); | ||
| return db; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n chat/src/lib/account-database.ts | sed -n '130,165p'Repository: marmot-protocol/marmot-ts
Length of output: 1513
🏁 Script executed:
head -n 30 chat/src/lib/account-database.tsRepository: marmot-protocol/marmot-ts
Length of output: 690
🏁 Script executed:
rg "getCustomDatabaseForAccount" chat/src/lib/account-database.ts -B 2 -A 2Repository: marmot-protocol/marmot-ts
Length of output: 463
🏁 Script executed:
rg "this\.customDatabases" chat/src/lib/account-database.ts -B 1 -A 1Repository: marmot-protocol/marmot-ts
Length of output: 361
Clear failed DB promises to allow recovery on retry.
If openDB fails, the rejected promise is cached in customDatabases, so subsequent calls retrieve the same rejected promise and never recover. Remove the entry from the cache on failure and throw a descriptive error.
Suggested update
private async getCustomDatabaseForAccount(
pubkey: string,
): Promise<IDBPDatabase<RumorDatabaseSchema>> {
const existing = this.customDatabases.get(pubkey);
if (existing) return existing;
// Create a new database for the account
- const db = openDB<RumorDatabaseSchema>(pubkey, DB_VERSION, {
- upgrade(db) {
- const rumors = db.createObjectStore("rumors", {
- keyPath: ["groupId", "id"],
- });
- rumors.createIndex("by_group_created_at", ["groupId", "created_at"]);
- },
- }).then((open) => {
- this.customDatabases.set(pubkey, open);
- return open;
- });
-
- this.customDatabases.set(pubkey, db);
- return db;
+ const dbPromise = openDB<RumorDatabaseSchema>(pubkey, DB_VERSION, {
+ upgrade(db) {
+ const rumors = db.createObjectStore("rumors", {
+ keyPath: ["groupId", "id"],
+ });
+ rumors.createIndex("by_group_created_at", ["groupId", "created_at"]);
+ },
+ });
+
+ this.customDatabases.set(pubkey, dbPromise);
+
+ try {
+ const db = await dbPromise;
+ this.customDatabases.set(pubkey, db);
+ return db;
+ } catch (error) {
+ this.customDatabases.delete(pubkey);
+ throw new Error(
+ `Failed to open rumor database for account ${pubkey}: ${String(error)}`,
+ );
+ }🤖 Prompt for AI Agents
In `@chat/src/lib/account-database.ts` around lines 137 - 158,
getCustomDatabaseForAccount currently caches the raw promise from openDB so a
failed open leaves a rejected promise in customDatabases; update
getCustomDatabaseForAccount to create the openDB promise, immediately store that
transient promise if you want deduplication, then await it inside a try/catch:
on success replace the cache entry with the resolved IDBPDatabase and return it,
on failure delete customDatabases.delete(pubkey) and throw a descriptive Error
(include pubkey and original error) so callers can retry; reference the
getCustomDatabaseForAccount method, the customDatabases Map, and the openDB call
when making this change.
| export const marmotClient$ = accounts.active$.pipe( | ||
| switchMap(async (account) => { | ||
| // Ensure all stores are created and setup | ||
| if (!account) return; | ||
|
|
||
| // Get storage interfaces for the account | ||
| const { groupStore, keyPackageStore, historyFactory } = | ||
| await databaseBroker.getStorageInterfacesForAccount(account.pubkey); | ||
|
|
||
| // Create a new marmot client for the active account | ||
| return new MarmotClient({ | ||
| signer: account.signer, | ||
| groupStore, | ||
| keyPackageStore, | ||
| network: networkInterface, | ||
| historyFactory, | ||
| }); | ||
| }), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and get line count
wc -l chat/src/lib/marmot-client.tsRepository: marmot-protocol/marmot-ts
Length of output: 102
🏁 Script executed:
# Read the specific lines mentioned (66-83)
sed -n '60,90p' chat/src/lib/marmot-client.tsRepository: marmot-protocol/marmot-ts
Length of output: 1135
🏁 Script executed:
# Search for databaseBroker.getStorageInterfacesForAccount to understand what it does
rg "getStorageInterfacesForAccount" chat/src/lib/marmot-client.ts -B 3 -A 3Repository: marmot-protocol/marmot-ts
Length of output: 338
🏁 Script executed:
# Check if there's already try-catch around this area
sed -n '50,100p' chat/src/lib/marmot-client.ts | grep -E "(try|catch)"Repository: marmot-protocol/marmot-ts
Length of output: 51
🏁 Script executed:
# Find databaseBroker definition and usage
rg "databaseBroker" chat/src/lib/marmot-client.ts -B 2 -A 2Repository: marmot-protocol/marmot-ts
Length of output: 466
🏁 Script executed:
# Search for databaseBroker definition in the codebase
rg "const databaseBroker|export.*databaseBroker" chat/src --type ts --type tsx -A 3Repository: marmot-protocol/marmot-ts
Length of output: 96
🏁 Script executed:
# Check if there's any error handling in the observable chain (catchError, onError, etc.)
sed -n '60,95p' chat/src/lib/marmot-client.tsRepository: marmot-protocol/marmot-ts
Length of output: 1253
🏁 Script executed:
# Find and read account-database.ts to understand getStorageInterfacesForAccount
find chat/src -name "*account-database*" -type fRepository: marmot-protocol/marmot-ts
Length of output: 101
🏁 Script executed:
# Read the account-database file to understand getStorageInterfacesForAccount
cat chat/src/lib/account-database.tsRepository: marmot-protocol/marmot-ts
Length of output: 5942
🏁 Script executed:
# Check if databaseBroker is singleton/shared and how errors are handled
rg "getStorageInterfacesForAccount" chat/src -B 2 -A 5Repository: marmot-protocol/marmot-ts
Length of output: 1191
🏁 Script executed:
# Verify file location and check imports to confirm it's a TypeScript file
head -20 chat/src/lib/marmot-client.tsRepository: marmot-protocol/marmot-ts
Length of output: 495
🏁 Script executed:
# Check if there's any error handling at the observable level (catchError operator)
rg "catchError|onError|error =>" chat/src/lib/marmot-client.tsRepository: marmot-protocol/marmot-ts
Length of output: 51
Wrap databaseBroker.getStorageInterfacesForAccount in try-catch to prevent observable errors.
If databaseBroker.getStorageInterfacesForAccount throws (IndexedDB/localForage blocked or initialization fails), the observable will error and stop emitting. Catch the error, log with context, and return undefined to keep the stream alive.
Suggested update
export const marmotClient$ = accounts.active$.pipe(
switchMap(async (account) => {
// Ensure all stores are created and setup
if (!account) return;
- // Get storage interfaces for the account
- const { groupStore, keyPackageStore, historyFactory } =
- await databaseBroker.getStorageInterfacesForAccount(account.pubkey);
-
- // Create a new marmot client for the active account
- return new MarmotClient({
- signer: account.signer,
- groupStore,
- keyPackageStore,
- network: networkInterface,
- historyFactory,
- });
+ try {
+ // Get storage interfaces for the account
+ const { groupStore, keyPackageStore, historyFactory } =
+ await databaseBroker.getStorageInterfacesForAccount(account.pubkey);
+
+ // Create a new marmot client for the active account
+ return new MarmotClient({
+ signer: account.signer,
+ groupStore,
+ keyPackageStore,
+ network: networkInterface,
+ historyFactory,
+ });
+ } catch (error) {
+ console.error(
+ "Failed to initialize MarmotClient for account",
+ account.pubkey,
+ error,
+ );
+ return;
+ }
}),
startWith(undefined),
shareReplay(1),
);📝 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.
| export const marmotClient$ = accounts.active$.pipe( | |
| switchMap(async (account) => { | |
| // Ensure all stores are created and setup | |
| if (!account) return; | |
| // Get storage interfaces for the account | |
| const { groupStore, keyPackageStore, historyFactory } = | |
| await databaseBroker.getStorageInterfacesForAccount(account.pubkey); | |
| // Create a new marmot client for the active account | |
| return new MarmotClient({ | |
| signer: account.signer, | |
| groupStore, | |
| keyPackageStore, | |
| network: networkInterface, | |
| historyFactory, | |
| }); | |
| }), | |
| export const marmotClient$ = accounts.active$.pipe( | |
| switchMap(async (account) => { | |
| // Ensure all stores are created and setup | |
| if (!account) return; | |
| try { | |
| // Get storage interfaces for the account | |
| const { groupStore, keyPackageStore, historyFactory } = | |
| await databaseBroker.getStorageInterfacesForAccount(account.pubkey); | |
| // Create a new marmot client for the active account | |
| return new MarmotClient({ | |
| signer: account.signer, | |
| groupStore, | |
| keyPackageStore, | |
| network: networkInterface, | |
| historyFactory, | |
| }); | |
| } catch (error) { | |
| console.error( | |
| "Failed to initialize MarmotClient for account", | |
| account.pubkey, | |
| error, | |
| ); | |
| return; | |
| } | |
| }), | |
| startWith(undefined), | |
| shareReplay(1), | |
| ); |
🤖 Prompt for AI Agents
In `@chat/src/lib/marmot-client.ts` around lines 66 - 83, The pipe for
marmotClient$ currently calls databaseBroker.getStorageInterfacesForAccount
without error handling, which can cause the observable to error and stop; wrap
the await databaseBroker.getStorageInterfacesForAccount(account.pubkey) call in
a try-catch inside the switchMap(async (account) => { ... }) block, log the
caught error with context (e.g., include account.pubkey and that
getStorageInterfacesForAccount failed), and return undefined so the observable
stream (accounts.active$ -> marmotClient$) stays alive instead of terminating;
keep the rest of the function returning new MarmotClient({ signer:
account.signer, groupStore, keyPackageStore, network: networkInterface,
historyFactory }) when successful.
| import { use$ } from "applesauce-react/hooks"; | ||
| import { extractMarmotGroupData, getGroupIdHex } from "marmot-ts"; | ||
| import { ClientState } from "ts-mls/clientState.js"; | ||
| import { GroupRumorHistory, MarmotGroup } from "marmot-ts"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n chat/src/pages/groups.tsx | head -150Repository: marmot-protocol/marmot-ts
Length of output: 5824
Use type-only imports for MarmotGroup and GroupRumorHistory.
Both are used only in type positions (line 29) and should be imported with the type keyword to avoid unnecessary runtime imports.
Suggested update
-import { GroupRumorHistory, MarmotGroup } from "marmot-ts";
+import type { GroupRumorHistory, MarmotGroup } from "marmot-ts";📝 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.
| import { use$ } from "applesauce-react/hooks"; | |
| import { extractMarmotGroupData, getGroupIdHex } from "marmot-ts"; | |
| import { ClientState } from "ts-mls/clientState.js"; | |
| import { GroupRumorHistory, MarmotGroup } from "marmot-ts"; | |
| import { use$ } from "applesauce-react/hooks"; | |
| import type { GroupRumorHistory, MarmotGroup } from "marmot-ts"; |
🤖 Prompt for AI Agents
In `@chat/src/pages/groups.tsx` around lines 3 - 4, The imports for MarmotGroup
and GroupRumorHistory are only used as types; change the import to a type-only
import so they are not emitted at runtime: replace the current import of
MarmotGroup and GroupRumorHistory from "marmot-ts" with a type import (reference
MarmotGroup and GroupRumorHistory) to ensure only type information is imported
while leaving any runtime imports (e.g., use$ from "applesauce-react/hooks")
unchanged.
| import { Rumor } from "applesauce-common/helpers/gift-wrap"; | ||
| import { use$ } from "applesauce-react/hooks"; | ||
| import { Loader2, XCircle } from "lucide-react"; | ||
| import { mapEventsToTimeline } from "applesauce-core"; | ||
| import type { NostrEvent } from "applesauce-core/helpers"; | ||
| import { use$ } from "applesauce-react/hooks"; | ||
| import { Loader2, XCircle } from "lucide-react"; | ||
| import { | ||
| extractMarmotGroupData, | ||
| getNostrGroupIdHex, | ||
| getGroupMembers, | ||
| getNostrGroupIdHex, | ||
| MarmotGroup, | ||
| unixNow, | ||
| } from "marmot-ts"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l chat/src/pages/groups/[id].tsxRepository: marmot-protocol/marmot-ts
Length of output: 103
🏁 Script executed:
cat -n chat/src/pages/groups/[id].tsx | head -100Repository: marmot-protocol/marmot-ts
Length of output: 3684
🏁 Script executed:
# Search for uses of Rumor and MarmotGroup in the file
rg "Rumor|MarmotGroup" chat/src/pages/groups/[id].tsx -nRepository: marmot-protocol/marmot-ts
Length of output: 759
Use import type for Rumor and MarmotGroup.
Both are used only in type annotations and don't require runtime values.
Suggested update
-import { Rumor } from "applesauce-common/helpers/gift-wrap";
+import type { Rumor } from "applesauce-common/helpers/gift-wrap";
import {
extractMarmotGroupData,
getGroupMembers,
getNostrGroupIdHex,
- MarmotGroup,
unixNow,
} from "marmot-ts";
+import type { MarmotGroup } from "marmot-ts";🤖 Prompt for AI Agents
In `@chat/src/pages/groups/`[id].tsx around lines 1 - 12, Change the runtime
imports of the purely-typed symbols to type-only imports: replace the current
runtime import of Rumor from "applesauce-common/helpers/gift-wrap" with an
"import type { Rumor }" and replace the runtime import of MarmotGroup from
"marmot-ts" with "import type { MarmotGroup }"; keep the other values from those
modules (e.g., extractMarmotGroupData, getGroupMembers, getNostrGroupIdHex,
unixNow) as normal imports so only Rumor and MarmotGroup are moved to type-only
imports.
| async *createPaginatedLoader( | ||
| filter?: Filter, | ||
| ): AsyncGenerator<Rumor[], Rumor[]> { | ||
| const limit = filter?.limit ?? 50; | ||
| let cursor = filter?.until ?? undefined; | ||
|
|
||
| while (true) { | ||
| const rumors = await this.backend.queryRumors({ | ||
| until: cursor, | ||
| limit, | ||
| }); | ||
|
|
||
| // If no rumors returned, we've reached the end | ||
| if (rumors.length === 0) return rumors; | ||
|
|
||
| // Find the oldest timestamp in the current page | ||
| // and set it as the new `until` for the next page (going backwards) | ||
| const oldest = Math.min(...rumors.map((rumor) => rumor.created_at)); | ||
|
|
||
| // Set the next `until` to be just before the oldest message | ||
| // This ensures we get the next older page without duplicates | ||
| if (!cursor || oldest < cursor) cursor = oldest - 1; |
There was a problem hiding this comment.
Preserve caller filters in createPaginatedLoader.
The current call drops since/authors/kinds (and tags), so pagination ignores filters. Pass through the remaining filter fields while overriding until/limit.
Suggested update
- const limit = filter?.limit ?? 50;
- let cursor = filter?.until ?? undefined;
+ const { limit = 50, until, ...rest } = filter ?? {};
+ let cursor = until;
while (true) {
const rumors = await this.backend.queryRumors({
+ ...rest,
until: cursor,
limit,
});🤖 Prompt for AI Agents
In `@src/client/group/group-rumor-history.ts` around lines 84 - 105,
createPaginatedLoader currently calls this.backend.queryRumors with only until
and limit, dropping caller-provided filters (since, authors, kinds, tags, etc.);
update the call in createPaginatedLoader so you merge the original filter into
the query parameters and only override limit and until (e.g., pass {...filter,
until: cursor, limit}) when calling this.backend.queryRumors so pagination
preserves all caller filters while still controlling page size and cursor.
| export interface BaseGroupHistory { | ||
| /** Saves a new application message to the group history */ | ||
| saveMessage(message: Uint8Array): Promise<void>; | ||
| /** Purge the group history, called when group is destroyed */ | ||
| prugeMessages(): Promise<void>; |
There was a problem hiding this comment.
Fix typo in history purge method name.
prugeMessages should be purgeMessages to avoid a misspelled public API.
Proposed fix
export interface BaseGroupHistory {
/** Saves a new application message to the group history */
saveMessage(message: Uint8Array): Promise<void>;
/** Purge the group history, called when group is destroyed */
- prugeMessages(): Promise<void>;
+ purgeMessages(): Promise<void>;
}
@@
async destroy() {
- if (this.history) await this.history.prugeMessages();
+ if (this.history) await this.history.purgeMessages();Also applies to: 905-910
🤖 Prompt for AI Agents
In `@src/client/group/marmot-group.ts` around lines 54 - 58, The interface
BaseGroupHistory declares a misspelled method prugeMessages; rename it to
purgeMessages in the interface and update all implementations/usages (e.g., any
classes implementing BaseGroupHistory and calls to prugeMessages) to the correct
name purgeMessages (also fix the other occurrence mentioned). Ensure method
signatures remain the same (Promise<void>) and run the typechecker to catch any
missed references.
| // Save application message to history | ||
| if (this.history) { | ||
| await this.history.saveMessage(result.message); | ||
| } | ||
|
|
||
| yield result; | ||
| this.emit("applicationMessage", result.message); | ||
| } |
There was a problem hiding this comment.
Don’t let history persistence failures mark messages unreadable.
If history.saveMessage() throws, the outer catch treats the event as unreadable after state has already advanced, which can lead to retries or dropped messages. Treat history persistence as best‑effort and swallow/log errors.
Proposed fix
// Save application message to history
if (this.history) {
- await this.history.saveMessage(result.message);
+ try {
+ await this.history.saveMessage(result.message);
+ } catch (err) {
+ console.warn(
+ "[MarmotGroup.ingest] Failed to persist history message",
+ err,
+ );
+ }
}📝 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.
| // Save application message to history | |
| if (this.history) { | |
| await this.history.saveMessage(result.message); | |
| } | |
| yield result; | |
| this.emit("applicationMessage", result.message); | |
| } | |
| // Save application message to history | |
| if (this.history) { | |
| try { | |
| await this.history.saveMessage(result.message); | |
| } catch (err) { | |
| console.warn( | |
| "[MarmotGroup.ingest] Failed to persist history message", | |
| err, | |
| ); | |
| } | |
| } | |
| yield result; | |
| this.emit("applicationMessage", result.message); |
🤖 Prompt for AI Agents
In `@src/client/group/marmot-group.ts` around lines 799 - 806, Wrap the call to
this.history.saveMessage(result.message) in its own try/catch so history
persistence is treated as best-effort: call await
this.history.saveMessage(result.message) inside try, catch and log the error
(e.g. this.logger?.error(...) or this.emit("historyError", err)) but do NOT
rethrow or change control flow; continue to yield result and
emit("applicationMessage", result.message) as before so a save failure won't
mark the message unreadable or cause retries.
| /** Destroys a group and purges the group history */ | ||
| async destroyGroup(groupId: Uint8Array | string): Promise<void> { | ||
| const id = typeof groupId === "string" ? groupId : bytesToHex(groupId); | ||
|
|
||
| // Get the existing instance or load a new one | ||
| const group = this.#groups.get(id) || (await this.loadGroup(groupId)); | ||
|
|
||
| // Use the instance to destroy the group | ||
| await group.destroy(); | ||
|
|
||
| // Remove the group from the cache without emitting an event | ||
| this.groupStore.remove(groupId); | ||
|
|
||
| // Remove the group from the cache | ||
| this.#groups.delete(id); | ||
|
|
||
| // Emit events | ||
| this.emit("groupDestroyed", group.id); | ||
| this.emit("groupsUpdated", this.groups); |
There was a problem hiding this comment.
Avoid double removal and un-awaited store deletion in destroyGroup.
group.destroy() already removes the group from the store. The extra groupStore.remove() is redundant and currently not awaited, which risks unhandled rejections or errors if removal isn’t idempotent. Prefer a single removal path and await it.
Proposed fix
- // Remove the group from the cache without emitting an event
- this.groupStore.remove(groupId);
-
- // Remove the group from the cache
+ // Remove the group from the cache
this.#groups.delete(id);📝 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.
| /** Destroys a group and purges the group history */ | |
| async destroyGroup(groupId: Uint8Array | string): Promise<void> { | |
| const id = typeof groupId === "string" ? groupId : bytesToHex(groupId); | |
| // Get the existing instance or load a new one | |
| const group = this.#groups.get(id) || (await this.loadGroup(groupId)); | |
| // Use the instance to destroy the group | |
| await group.destroy(); | |
| // Remove the group from the cache without emitting an event | |
| this.groupStore.remove(groupId); | |
| // Remove the group from the cache | |
| this.#groups.delete(id); | |
| // Emit events | |
| this.emit("groupDestroyed", group.id); | |
| this.emit("groupsUpdated", this.groups); | |
| /** Destroys a group and purges the group history */ | |
| async destroyGroup(groupId: Uint8Array | string): Promise<void> { | |
| const id = typeof groupId === "string" ? groupId : bytesToHex(groupId); | |
| // Get the existing instance or load a new one | |
| const group = this.#groups.get(id) || (await this.loadGroup(groupId)); | |
| // Use the instance to destroy the group | |
| await group.destroy(); | |
| // Remove the group from the cache | |
| this.#groups.delete(id); | |
| // Emit events | |
| this.emit("groupDestroyed", group.id); | |
| this.emit("groupsUpdated", this.groups); |
🤖 Prompt for AI Agents
In `@src/client/marmot-client.ts` around lines 220 - 238, The destroyGroup method
currently calls group.destroy() which already removes the group from the
persistent store, and then calls this.groupStore.remove(groupId) without
awaiting it, causing redundant work and potential unhandled rejections; remove
the redundant this.groupStore.remove(...) call and instead await the in-memory
cache deletion and event emission after the awaited group.destroy() completes
(i.e., keep await group.destroy(), then this.#groups.delete(id) and emit
"groupDestroyed" and "groupsUpdated"), ensuring you reference the same id
variable derived from groupId and do not call groupStore.remove anywhere in
destroyGroup.
| async update(clientState: ClientState): Promise<string> { | ||
| return await this.add(clientState); | ||
| const key = this.resolveStorageKey(clientState.groupContext.groupId); | ||
| const storedClientState = serializeClientState(clientState); | ||
|
|
||
| await this.backend.setItem(key, storedClientState); | ||
| this.emit("clientStateAdded", clientState); | ||
|
|
||
| return key; | ||
| } |
There was a problem hiding this comment.
update() emits clientStateAdded instead of clientStateUpdated.
The update() method emits clientStateAdded (line 97), but GroupStoreEvents defines a separate clientStateUpdated event (lines 23-24). This creates a semantic inconsistency: subscribers expecting update notifications would never receive them.
If updates should be treated identically to additions (upsert semantics), consider removing the clientStateUpdated event definition to avoid confusion. Otherwise, emit clientStateUpdated when updating an existing entry.
🔧 Option A: Emit the correct event for updates
async update(clientState: ClientState): Promise<string> {
const key = this.resolveStorageKey(clientState.groupContext.groupId);
const storedClientState = serializeClientState(clientState);
await this.backend.setItem(key, storedClientState);
- this.emit("clientStateAdded", clientState);
+ this.emit("clientStateUpdated", clientState);
return key;
}🔧 Option B: Remove unused event type if upsert semantics are intended
type GroupStoreEvents = {
/** Emitted when a client state is added */
clientStateAdded: (clientState: ClientState) => any;
- /** Emitted when a client state is updated */
- clientStateUpdated: (clientState: ClientState) => any;
/** Emitted when a client state is removed */
clientStateRemoved: (groupId: Uint8Array) => any;
};📝 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.
| async update(clientState: ClientState): Promise<string> { | |
| return await this.add(clientState); | |
| const key = this.resolveStorageKey(clientState.groupContext.groupId); | |
| const storedClientState = serializeClientState(clientState); | |
| await this.backend.setItem(key, storedClientState); | |
| this.emit("clientStateAdded", clientState); | |
| return key; | |
| } | |
| async update(clientState: ClientState): Promise<string> { | |
| const key = this.resolveStorageKey(clientState.groupContext.groupId); | |
| const storedClientState = serializeClientState(clientState); | |
| await this.backend.setItem(key, storedClientState); | |
| this.emit("clientStateUpdated", clientState); | |
| return key; | |
| } |
🤖 Prompt for AI Agents
In `@src/store/group-store.ts` around lines 92 - 100, The update() method
currently emits the wrong event: instead of emitting clientStateUpdated it emits
clientStateAdded; locate the update(clientState: ClientState) function and
change the emitted event to "clientStateUpdated" (or, if intent is upsert
semantics, remove the clientStateUpdated entry from GroupStoreEvents and keep
emitting clientStateAdded) so subscribers receive the correct notification; use
the existing symbols resolveStorageKey, serializeClientState, backend.setItem,
and this.emit(...) in your change to ensure consistency with GroupStoreEvents.
This PR adds a new
GroupHistoryStoreinterface andhistoryfield to theMarmotGroupclass. the interface is a generic type on the class or that the app can override it with a custom class that handles MLS message history in a custom way (better loading for UI / live events)I then added a
MarmotGroupHistoryStoregroup history storage class that only saves the MIP-03 rumors from the MLS messages to a backend. this is intended to be used by most marmot apps since they only care about valid nostr rumor events and not other stuff that could be sent over MLSThe chat app them implements a
idbbased backend for storing group rumor messages and then provides that to theMarmotClientclass so it can be pass down to theMarmotGroupclass forgroup.historyThe main chat view is now updated to use a
useGroupMessageshook that only uses thegroup.historyinterface to listen for new messages and load old messages (via pagination)Summary by CodeRabbit
Release Notes
New Features
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.