Refactor/ts mls v2 migration #41
Conversation
- Update joinGroup, codec, wireformat, and ciphersuite handling to v2 patterns - Add MIGRATION_REPORT.md documenting breaking changes and migration steps - Rename proposeKickUser to proposeRemoveUser for consistency - Use contentTypes constants instead of magic numbers - Implement id-first ciphersuite handling for correctness
|
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:
WalkthroughBroad migration to ts-mls v2 across core, client, store, examples, and tests: API calls moved to context-object patterns, encoders/decoders replaced legacy serializers, numeric ciphersuite/extension/credential IDs adopted, new group-message and marmot-group-data formats added, and examples/tests updated accordingly. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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
🧪 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/client/group/proposals/remove-member.ts (1)
6-12:⚠️ Potential issue | 🟡 MinorJSDoc still references "kick" after rename to
proposeRemoveUser.Line 10 says "the user to kick" — should align with the new naming convention.
📝 Proposed fix
- * `@param` pubkey - The Nostr public key (hex string) of the user to kick + * `@param` pubkey - The Nostr public key (hex string) of the user to removesrc/core/extensions.ts (1)
23-25:⚠️ Potential issue | 🟡 MinorStale JSDoc:
{@linkExtension}should be{@linkCustomExtension}.The JSDoc on Line 24 still references the old
Extensiontype, but the function now operates onCustomExtension[].📝 Proposed fix
-/** Modifies an {`@link` Extension} array to ensure it includes the last_resort extension. +/** Modifies a {`@link` CustomExtension} array to ensure it includes the last_resort extension.src/core/client-state.ts (1)
59-68:⚠️ Potential issue | 🔴 CriticalBug:
getNostrGroupIdHexalways returns the MLS group ID instead of the Nostr group ID.
clientState.groupContext.groupIdis aUint8Array, which is always truthy in JavaScript — even when empty. The condition on line 60 will always be true, so the function never reaches theextractMarmotGroupDatafallback path.This means
getNostrGroupIdHexnow returns the MLS group ID (fromgroupContext.groupId) instead of the Nostr group ID (fromMarmotGroupData.nostrGroupId). These are distinct identifiers:groupContext.groupIdis the MLS group identifier, whilenostrGroupIdis documented as "32-byte identifier for the group used in Nostr protocol operations." The function name and its usage (e.g., "Get the Nostr group ID for the tags" ingroup-message.ts:209) indicate it should return the Nostr-specific identifier.To fix: Check for non-empty length instead of truthiness, or remove the short-circuit entirely if the fallback is the correct path.
Proposed fix options
Option 1: Remove the short-circuit if
extractMarmotGroupDatais always the correct source:export function getNostrGroupIdHex(clientState: ClientState): string { - if (clientState.groupContext.groupId) { - return bytesToHex(clientState.groupContext.groupId); - } const marmotData = extractMarmotGroupData(clientState); if (!marmotData) { throw new Error("MarmotGroupData not found in ClientState"); } return bytesToHex(marmotData.nostrGroupId); }Option 2: Check length if the intent is to prefer a non-empty
groupId:- if (clientState.groupContext.groupId) { + if (clientState.groupContext.groupId?.length > 0) { return bytesToHex(clientState.groupContext.groupId); }Verify which identifier callers actually require.
🤖 Fix all issues with AI agents
In `@MIGRATION_REPORT.md`:
- Around line 67-71: The migration report incorrectly places src/core/message.ts
under "Codec API Changes" (Section 2) even though its changes are
wireformat-related; update the report by moving src/core/message.ts from the
Section 2 affected files list into Section 3 (Wireformat/Message Changes) or
remove it from Section 2 entirely, and mention the specific changes (the
MlsMessage type rename and switching to the wireformats constant) so reviewers
can find the relevant edits.
- Around line 338-349: Remove the duplicate entry for src/core/welcome.ts in the
migration list: locate both occurrences of the string "src/core/welcome.ts" and
delete one so the file is listed only once; after removal, scan the surrounding
entries to ensure no other duplicates and adjust ordering if you want consistent
alphabetical or logical grouping.
In `@package.json`:
- Line 76: The dependency entry "ts-mls": "2.0.0-rc.7" in package.json is
invalid; verify whether the intended package is actually "ts-mls" (Messaging
Layer Security) or a different package such as "fp-ts", confirm the correct
published version (e.g., npm shows ts-mls at 0.2.0 and fp-ts has 2.0.0-rc.7),
and then update the package.json dependency to a valid package name and version
or point to your private registry/git URL if this is a fork; specifically locate
the "ts-mls": "2.0.0-rc.7" entry and either replace it with the correct package
name/version, change it to a valid semver from npm, or remove/redirect it to a
private registry URL so npm install succeeds.
In `@src/__tests__/admin-verification.test.ts`:
- Line 140: Replace the defensive cast-and-fallback pattern for welcome with a
clearer typed access: stop using "(welcome as any).welcome ?? (welcome as any)"
and either (a) declare the test variable with the proper MlsWelcomeMessage type
so you can use "welcome?.welcome" (matching the production pattern noted in
marmot-group.ts comment) or (b) directly use the nullable-safe property access
"welcome?.welcome" in the test; update the test variable/type (or its
initializer) so the compiler knows welcome may be an MlsWelcomeMessage and
remove the unnecessary "as any" fallback.
In `@src/core/group-message.ts`:
- Around line 252-259: The conditional that validates required fields
incorrectly uses "!parsed.kind === undefined" which always evaluates false;
update the check in the validation block that references parsed, id, pubkey, and
kind so that kind is properly tested (e.g., use "parsed.kind === undefined" or
check for presence via "kind" in parsed or parsed.kind == null) and throw the
same error when kind is missing; keep the rest of the id/pubkey checks as-is and
ensure the error message remains "Invalid application data: missing required
fields".
- Around line 160-179: readGroupMessages currently uses Promise.all with
concurrent pushes into the read/unreadable arrays, which loses the original
events ordering; change it so results are recorded in the same order as the
input events (e.g., replace the concurrent events.map with an order-preserving
approach such as iterating with for...of and awaiting readGroupMessage(event,
clientState, ciphersuite) for each event, or map each event to a promise and
then await Promise.allSettled and iterate the settled results in the original
events order), ensuring read and unreadable arrays are populated in input order
while still handling per-event errors from readGroupMessage.
In `@src/core/key-package-event.ts`:
- Around line 231-244: The function getKeyPackageNostrPubkey uses
require("ts-mls") which fails in ESM and duplicates a credential-type guard that
getCredentialPubkey already enforces; replace the require with a top-level ES
import of defaultCredentialTypes from "ts-mls" (matching other files) and remove
the manual credential type check block so getKeyPackageNostrPubkey simply calls
getKeyPackage(...) and returns
getCredentialPubkey(keyPackage.leafNode.credential).
🧹 Nitpick comments (22)
examples/src/lib/group-store.ts (1)
8-8: TODO: Reactivity regression after v2 refactor.This signals a known functional gap. Worth tracking as a follow-up issue so it doesn't get lost.
Would you like me to open an issue to track restoring reactivity for
groupStore$post-v2 migration?src/client/group/proposals/update-metadata.ts (1)
27-30: Type assertions on extensions — consider tighteningreplaceExtensionsignature instead.The double cast (
as GroupContextExtension[]on both input and output) works aroundreplaceExtensionaccepting/returningArray<{ extensionType: number }>. IfGroupContextExtensionis now the canonical extension type in v2, updatingreplaceExtensioninsrc/core/extensions.tsto useGroupContextExtensiondirectly would eliminate the need for these casts.src/core/credential.ts (1)
24-43: The type-narrowing pattern is correct but repeated — consider a helper.The
credentialTypecheck followed byas CredentialBasiccast is duplicated in bothgetCredentialPubkeyandisSameCredential. This works correctly since the runtime check guards the cast, but could be consolidated into a small type guard function.♻️ Optional: Extract a type guard for cleaner narrowing
function isBasicCredential(c: Credential): c is CredentialBasic { return c.credentialType === defaultCredentialTypes.basic; }This would let you replace the check + cast with a single
if (isBasicCredential(credential))block where TypeScript automatically narrows the type.src/core/default-capabilities.ts (1)
26-35: GREASE range check is overly broad but practically safe.The filter
id >= 0x0a0a && id <= 0xfafamatches the entire range, not just the specific GREASE sentinel values (0x0A0A, 0x1A1A, 0x2A2A, …, 0xEAEA). This works in practice because no standard MLS ciphersuites fall in that numeric range, but the comment on Line 31 is slightly misleading. Consider usinggreaseValuesfromts-mls/grease.js(already used incipher-suite-badge.tsx) for precise matching:♻️ Proposed: use greaseValues for precise GREASE detection
+import { greaseValues } from "ts-mls/grease.js"; ... capabilities.ciphersuites = Object.entries(ciphersuites) .filter(([name, id]) => { // Keep the default ciphersuite by name if (name === defaultCiphersuiteName) return true; - // Keep GREASE values (they have high numeric values, not sequential) - // GREASE values are typically in the 0x0A0A-0xFAFA range - if (id >= 0x0a0a && id <= 0xfafa) return true; + // Keep GREASE values + if (greaseValues.includes(id)) return true; return false; }) .map(([, id]) => id) as Capabilities["ciphersuites"];examples/src/examples/key-package/manager.tsx (1)
347-347: Minor type inconsistency:availableCipherSuiteswidened tonumber[]while other props useCiphersuiteId.
FilterControlsPropstypescipherSuiteFilterasCiphersuiteId | "all"(Line 338) butavailableCipherSuitesasnumber[]. For consistency and type safety, consider usingCiphersuiteId[]here, matching the rest of the interface.♻️ Proposed fix
- availableCipherSuites: number[]; + availableCipherSuites: CiphersuiteId[];And correspondingly at Line 582:
- const suites = new Set<number>(); + const suites = new Set<CiphersuiteId>();src/core/key-package.ts (1)
1-9: Duplicate"ts-mls"import statements can be consolidated.Lines 1–5 and Line 9 both import from
"ts-mls". Consider merging them.♻️ Proposed consolidation
import { CryptoProvider, + CustomExtension, defaultCredentialTypes, defaultCryptoProvider, } from "ts-mls"; import { Capabilities } from "ts-mls/capabilities.js"; import { Credential } from "ts-mls/credential.js"; import { CiphersuiteId, CiphersuiteImpl } from "ts-mls/crypto/ciphersuite.js"; -import { CustomExtension } from "ts-mls";examples/src/examples/group/join.tsx (1)
284-284:refreshCountdependency inwelcomeMessagesmemo may be redundant.
giftWrapsalready depends onrefreshCount(line 284), sowelcomeMessageswill recompute whenevergiftWrapschanges. IncludingrefreshCountagain at line 337 adds a redundant trigger. Not harmful, but unnecessary.Also applies to: 337-337
src/client/marmot-client.ts (2)
357-358: Redundant intermediate variable.
mlsWelcomeis immediately assigned towelcomewith no transformation. This can be simplified.♻️ Suggested simplification
- const mlsWelcome = getWelcome(welcomeRumor); - const welcome = mlsWelcome; + const welcome = getWelcome(welcomeRumor);
395-401: Import and use the properEncryptedGroupSecretstype from ts-mls for the secret parameter.The
anytype onsecretat line 395 bypasses type safety on the Welcome secrets structure. According to RFC 9420, each entry inWelcome.secretsshould beEncryptedGroupSecretswith anew_member: KeyPackageReffield. If ts-mls v2 renamesnewMemberor changes its type, this code will silently fail at runtime.Check if ts-mls exports
EncryptedGroupSecretsfromts-mls/welcome.js(alongside the importedWelcometype) and use it explicitly. If the type is not exported, add a comment explaining whyanyis necessary and document the expected RFC 9420 structure.src/core/client-state.ts (1)
120-133: Unused_configparameter — consider removing or documenting intent.The
_config: ClientConfigparameter is accepted but never used. The underscore prefix correctly signals this, and the comment explains why. However, if no future use is planned, removing it would simplify the API and avoid misleading callers who pass their config expecting it to be applied.src/client/group/marmot-group.ts (2)
97-98: Inlineimport(...)type used twice — consider a top-level type import.
import("ts-mls/clientConfig.js").ClientConfigappears at both lines 98 and 190. A top-levelimport type { ClientConfig } from "ts-mls/clientConfig.js"would be cleaner and easier to maintain.♻️ Suggested change
Add at the top of the file with other imports:
import type { ClientConfig } from "ts-mls/clientConfig.js";Then replace both inline imports:
- /** Authentication policy/config used when processing MLS messages */ - clientConfig?: import("ts-mls/clientConfig.js").ClientConfig; + /** Authentication policy/config used when processing MLS messages */ + clientConfig?: ClientConfig;- readonly clientConfig: import("ts-mls/clientConfig.js").ClientConfig; + readonly clientConfig: ClientConfig;Also applies to: 189-191
410-411: Redundant variable assignment.
const mlsMessage = message;is an unnecessary rename.♻️ Suggested simplification
- // The message returned is the MLS message (not privateMessage directly) - const mlsMessage = message; - // Wrap the message in a group event // Use this.state (not newState) to get the exporter_secret for the current epoch const applicationEvent = await createGroupEvent({ - message: mlsMessage, + message, state: this.state, ciphersuite: this.ciphersuite, });src/core/marmot-group-data.ts (2)
16-17: Misleading comment: this is not varint encoding.The comment mentions "Variable-length integer encoding (similar to Protocol Buffers varint)" but the implementation uses a fixed 4-byte big-endian length prefix, which is fundamentally different from varint. Consider updating the comment to avoid confusing future readers.
📝 Suggested comment fix
-// Variable-length integer encoding (similar to Protocol Buffers varint) -// For simplicity, we use a fixed 4-byte length prefix for byte arrays +// Fixed 4-byte big-endian length prefix for byte arrays
66-68: DuplicateisHexKey— consider importing fromcredential.ts.This local
isHexKeyis identical to the exported function insrc/core/credential.ts(lines 5-7). Importing it would reduce duplication.♻️ Proposed fix
-function isHexKey(str: string): boolean { - return /^[0-9a-fA-F]{64}$/.test(str); -}And add to the existing imports:
+import { isHexKey } from "./credential.js";src/__tests__/marmot-group-data.test.ts (1)
334-336: Test name references old API (encodeGroupState/decodeGroupState).The test description still says "encodeGroupState/decodeGroupState round-trip" but the implementation now uses
encode(clientStateEncoder, ...)/decode(clientStateDecoder, ...). Consider updating the test name to match.src/__tests__/admin-verification.test.ts (1)
156-160: Storing rawClientStateasSerializedClientState(Uint8Array) viaas any.
SerializedClientStateisUint8Array, butadminStateEpoch1is aClientStateobject. Theas anycast silences the type mismatch. This works because theMemoryGroupStateBackendis type-agnostic and the test never deserializes from the store, but it could mask bugs if the test orMarmotGroupinternals later expect actual serialized bytes.Consider using
encode(clientStateEncoder, adminStateEpoch1)to properly serialize, consistent with whatingest-commit-race.test.tsdoes.src/__tests__/ingest-commit-race.test.ts (2)
275-284: Dead code and redundant dynamic import.
addProposal(lines 275-278) is created but never used —addProposalTyped(lines 281-284) is used instead. Also,defaultProposalTypesis dynamically imported at line 280 even though it's already imported at the top level (line 12).♻️ Proposed cleanup
- const addProposal = { - proposalType: "add" as const, - add: { keyPackage: memberKeyPackage.publicPackage }, - }; - - const { defaultProposalTypes } = await import("ts-mls"); - const addProposalTyped = { - proposalType: defaultProposalTypes.add, + const addProposalTyped = { + proposalType: defaultProposalTypes.add, add: { keyPackage: memberKeyPackage.publicPackage }, };
409-413: Redundant dynamic import ofdefaultProposalTypes.
defaultProposalTypesis already imported at the top level (line 12). The dynamicawait import("ts-mls")here is unnecessary.♻️ Proposed fix
- const { defaultProposalTypes } = await import("ts-mls"); const addProposal1 = { proposalType: defaultProposalTypes.add,src/__tests__/key-package.test.ts (1)
20-23: Extract the repeated ciphersuite name to a constant.The string
"MLS_128_DHKEMX25519_AES128GCM_SHA256_Ed25519"appears 7 times. A single constant at the top of thedescribeblock would reduce duplication and make it easier to update if the test ciphersuite changes.♻️ Suggested refactor
Add at the top of the
describeblock (aftervalidPubkey):describe("generateKeyPackage", () => { const validPubkey = "884704bd421671e01c13f854d2ce23ce2a5bfe9562f4f297ad2bc921ba30c3a6"; + const ciphersuiteName = "MLS_128_DHKEMX25519_AES128GCM_SHA256_Ed25519" as const;Then replace each occurrence:
const ciphersuiteImpl = await getCiphersuiteImpl( - "MLS_128_DHKEMX25519_AES128GCM_SHA256_Ed25519", + ciphersuiteName, defaultCryptoProvider, );Also applies to: 40-43, 58-61, 79-82, 109-112, 146-149, 171-174
src/core/group-message.ts (3)
245-262: Validation indeserializeApplicationDatais weaker than theisRumorLiketype guard.
isRumorLike(Line 324) validates six fields (id,pubkey,kind,created_at,content,tags) with type checks. This function only checks three fields loosely (and even that is partially broken per the above bug). Consider reusingisRumorLikefor consistent validation.♻️ Suggested refactor
export function deserializeApplicationData(data: Uint8Array): Rumor { const json = new TextDecoder().decode(data); const parsed = JSON.parse(json); - if (!parsed || typeof parsed !== "object") { - throw new Error("Invalid application data: not an object"); - } - - if (!parsed.id || !parsed.pubkey || !parsed.kind === undefined) { - throw new Error("Invalid application data: missing required fields"); - } + if (!isRumorLike(parsed)) { + throw new Error("Invalid application data: not a valid rumor"); + } return parsed as Rumor; }
278-295:sortGroupCommitsmutates the input array in-place.
Array.prototype.sortmutates the receiver. The function name and JSDoc ("returns sorted array") suggest a pure operation. Consider usingtoSorted()or spreading before sorting to avoid surprising callers.♻️ Non-mutating alternative
export function sortGroupCommits( commits: GroupMessagePair[], ): GroupMessagePair[] { - return commits.sort((a, b) => { + return [...commits].sort((a, b) => {
303-319:createProposalEventandcreateCommitEventare currently identical wrappers.These both delegate to
createGroupEventwith the same signature. If the intent is to distinguish proposal vs. commit semantics in the future (e.g., different tags or validation), this is a reasonable placeholder. Otherwise, if there's no planned divergence, they add unnecessary indirection.
| - [`src/core/message.ts`](src/core/message.ts) - Wireformat type changes | ||
| - [`src/core/welcome.ts`](src/core/welcome.ts) - Codec API changes | ||
| - [`src/core/auth-service.ts`](src/core/auth-service.ts) - Context pattern updates | ||
| - [`src/core/client-state.ts`](src/core/client-state.ts) - Serialization updates | ||
| - [`src/core/credential.ts`](src/core/credential.ts) - Type updates | ||
| - [`src/core/default-capabilities.ts`](src/core/default-capabilities.ts) - Proposal types | ||
| - [`src/core/extensions.ts`](src/core/extensions.ts) - Extension factory updates | ||
| - [`src/core/group-message.ts`](src/core/group-message.ts) - Major refactor | ||
| - [`src/core/key-package-event.ts`](src/core/key-package-event.ts) - Codec updates | ||
| - [`src/core/key-package.ts`](src/core/key-package.ts) - Type updates | ||
| - [`src/core/marmot-group-data.ts`](src/core/marmot-group-data.ts) - Full refactor | ||
| - [`src/core/welcome.ts`](src/core/welcome.ts) - Codec API changes |
There was a problem hiding this comment.
Duplicate entry: src/core/welcome.ts appears twice in the file list.
Lines 339 and 349 both list src/core/welcome.ts.
🤖 Prompt for AI Agents
In `@MIGRATION_REPORT.md` around lines 338 - 349, Remove the duplicate entry for
src/core/welcome.ts in the migration list: locate both occurrences of the string
"src/core/welcome.ts" and delete one so the file is listed only once; after
removal, scan the surrounding entries to ensure no other duplicates and adjust
ordering if you want consistent alphabetical or logical grouping.
| "eventemitter3": "^5.0.4", | ||
| "nostr-tools": "^2.23.0", | ||
| "ts-mls": "^1.6.1" | ||
| "ts-mls": "2.0.0-rc.7" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
ts-mls npm package latest version 2.0.0
💡 Result:
The npm package ts-mls’s latest published version is 0.2.0 (not 2.0.0). [1]
If you were expecting 2.0.0, you may be looking at a different package name—ts-mls (Messaging Layer Security / RFC 9420) currently publishes in the 0.x range. [1][2]
🌐 Web query:
ts-mls npm package 2.0.0-rc.7 release versions
💡 Result:
I couldn’t find any evidence that the ts-mls npm package has a 2.0.0-rc.7 (or any 2.x) release. The package is shown at 0.2.0 on npm aggregators, and the upstream repo’s latest GitHub release is v1.4.2 (Nov 20, 2025). [1][2]
If you meant fp-ts (not ts-mls), fp-ts@2.0.0-rc.7 does exist as a published pre-release. [3]
Sources: [1] [2] [3]
This version does not exist — verify the package name and version specification.
The ts-mls package on npm does not have a 2.0.0-rc.7 release. The latest upstream release is v1.4.2 (Nov 2025), and the npm package is at 0.2.0. Confirm whether:
- The package name should be
ts-mls(Messaging Layer Security)? - Or is this intended to be
fp-ts, which does have2.0.0-rc.7? - Or is this a private/forked package with a different registry?
The current specification will fail npm install.
🤖 Prompt for AI Agents
In `@package.json` at line 76, The dependency entry "ts-mls": "2.0.0-rc.7" in
package.json is invalid; verify whether the intended package is actually
"ts-mls" (Messaging Layer Security) or a different package such as "fp-ts",
confirm the correct published version (e.g., npm shows ts-mls at 0.2.0 and fp-ts
has 2.0.0-rc.7), and then update the package.json dependency to a valid package
name and version or point to your private registry/git URL if this is a fork;
specifically locate the "ts-mls": "2.0.0-rc.7" entry and either replace it with
the correct package name/version, change it to a valid semver from npm, or
remove/redirect it to a private registry URL so npm install succeeds.
| export async function readGroupMessages( | ||
| events: NostrEvent[], | ||
| clientState: ClientState, | ||
| ciphersuite: CiphersuiteImpl, | ||
| ): Promise<{ read: GroupMessagePair[]; unreadable: NostrEvent[] }> { | ||
| const read: GroupMessagePair[] = []; | ||
| const unreadable: NostrEvent[] = []; | ||
|
|
||
| await Promise.all( | ||
| events.map(async (event) => { | ||
| try { | ||
| read.push(await readGroupMessage(event, clientState, ciphersuite)); | ||
| } catch { | ||
| unreadable.push(event); | ||
| } | ||
| }), | ||
| ); | ||
|
|
||
| return { read, unreadable }; | ||
| } |
There was a problem hiding this comment.
readGroupMessages does not preserve event ordering.
Promise.all resolves concurrently, so the order in which items are pushed to the read array depends on which decryptions finish first, not the original events order. If callers rely on input ordering (e.g., chronological), this could be surprising.
♻️ Order-preserving alternative
export async function readGroupMessages(
events: NostrEvent[],
clientState: ClientState,
ciphersuite: CiphersuiteImpl,
): Promise<{ read: GroupMessagePair[]; unreadable: NostrEvent[] }> {
const read: GroupMessagePair[] = [];
const unreadable: NostrEvent[] = [];
- await Promise.all(
- events.map(async (event) => {
- try {
- read.push(await readGroupMessage(event, clientState, ciphersuite));
- } catch {
- unreadable.push(event);
- }
- }),
- );
+ const results = await Promise.allSettled(
+ events.map((event) => readGroupMessage(event, clientState, ciphersuite)),
+ );
+
+ for (let i = 0; i < results.length; i++) {
+ const result = results[i];
+ if (result.status === "fulfilled") {
+ read.push(result.value);
+ } else {
+ unreadable.push(events[i]);
+ }
+ }
return { read, unreadable };
}📝 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 async function readGroupMessages( | |
| events: NostrEvent[], | |
| clientState: ClientState, | |
| ciphersuite: CiphersuiteImpl, | |
| ): Promise<{ read: GroupMessagePair[]; unreadable: NostrEvent[] }> { | |
| const read: GroupMessagePair[] = []; | |
| const unreadable: NostrEvent[] = []; | |
| await Promise.all( | |
| events.map(async (event) => { | |
| try { | |
| read.push(await readGroupMessage(event, clientState, ciphersuite)); | |
| } catch { | |
| unreadable.push(event); | |
| } | |
| }), | |
| ); | |
| return { read, unreadable }; | |
| } | |
| export async function readGroupMessages( | |
| events: NostrEvent[], | |
| clientState: ClientState, | |
| ciphersuite: CiphersuiteImpl, | |
| ): Promise<{ read: GroupMessagePair[]; unreadable: NostrEvent[] }> { | |
| const read: GroupMessagePair[] = []; | |
| const unreadable: NostrEvent[] = []; | |
| const results = await Promise.allSettled( | |
| events.map((event) => readGroupMessage(event, clientState, ciphersuite)), | |
| ); | |
| for (let i = 0; i < results.length; i++) { | |
| const result = results[i]; | |
| if (result.status === "fulfilled") { | |
| read.push(result.value); | |
| } else { | |
| unreadable.push(events[i]); | |
| } | |
| } | |
| return { read, unreadable }; | |
| } |
🤖 Prompt for AI Agents
In `@src/core/group-message.ts` around lines 160 - 179, readGroupMessages
currently uses Promise.all with concurrent pushes into the read/unreadable
arrays, which loses the original events ordering; change it so results are
recorded in the same order as the input events (e.g., replace the concurrent
events.map with an order-preserving approach such as iterating with for...of and
awaiting readGroupMessage(event, clientState, ciphersuite) for each event, or
map each event to a promise and then await Promise.allSettled and iterate the
settled results in the original events order), ensuring read and unreadable
arrays are populated in input order while still handling per-event errors from
readGroupMessage.
hzrd149
left a comment
hzrd149
left a comment
There was a problem hiding this comment.
Looks good. I mostly have a bunch of questions
Co-authored-by: hzrd149 <8001706+hzrd149@users.noreply.github.com>
This commit updates the codebase to use ts-mls v2, which introduces several breaking changes: - Codec functions now use `decode(xDecoder, data)` and `encode(xEncoder, value)` - Wireformat types are now a constant object `wireformats` - Credential types are now numeric values via `defaultCredentialTypes` - GroupStore no longer requires `ClientConfig` - Key package events now return `EventTemplate` without `pubkey` See MIGRATION_REPORT.md for full details. BREAKING CHANGE: Multiple API changes as described in MIGRATION_REPORT.md
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/client-state.ts (1)
70-88:⚠️ Potential issue | 🟡 MinorError message gets double-prefixed when
decodereturns null.If
decodereturns null, line 77-79 throws"Failed to deserialize ClientState: clientStateDecoder returned null". This is immediately caught by thecatchon line 82, which re-wraps it into"Failed to deserialize ClientState: Failed to deserialize ClientState: clientStateDecoder returned null".Proposed fix: return/throw before the catch can re-wrap
export function deserializeClientState( stored: SerializedClientState, ): ClientState { - try { - const decoded = decode(clientStateDecoder, stored); - if (!decoded) { - throw new Error( - "Failed to deserialize ClientState: clientStateDecoder returned null", - ); - } - return decoded; - } catch (error) { + let decoded: ClientState | null; + try { + decoded = decode(clientStateDecoder, stored); + } catch (error) { if (error instanceof Error) { throw new Error(`Failed to deserialize ClientState: ${error.message}`); } throw new Error("Failed to deserialize ClientState: Unknown error"); } + if (!decoded) { + throw new Error( + "Failed to deserialize ClientState: clientStateDecoder returned null", + ); + } + return decoded; }
🤖 Fix all issues with AI agents
In `@src/__tests__/ingest-commit-race.test.ts`:
- Line 122: The test uses a defensive expression "welcome!.welcome ?? welcome"
which hides the real shape of the ts-mls response; update code to consistently
unwrap or document the expected type: determine whether the API returns
MlsWelcomeMessage or raw welcome, then either (A) always unwrap the object
before passing it to joinGroup and other call sites (search for the local
variable "welcome" and references in joinGroup and
marmot-group.ts/marmot-client.ts and normalize to welcome.welcome where
appropriate), or (B) add a concise comment next to the expression and at the
call sites (e.g., where joinGroup is invoked) explaining that in ts-mls
v2.0.0-rc.7 the welcome may be wrapped and the fallback is intentional; apply
the chosen approach consistently across ingest-commit-race.test.ts,
marmot-group.ts, and marmot-client.ts to eliminate the mixed handling.
In `@src/client/group/marmot-group.ts`:
- Around line 549-616: There is an unnecessary bare block following the
early-return guard after computing innerWelcome in MarmotGroup.commit; remove
the stray opening and closing braces that wrap the Promise.allSettled call so
the code is simply executed after the `if (!innerWelcome) return response;`
check (i.e., dedent the block containing Promise.allSettled and its async
callback that references options.welcomeRecipients, createWelcomeRumor,
createGiftWrap, this.network.getUserInboxRelays, and this.network.publish),
preserving all existing logic and await behavior and then run the linter/tests
to ensure no formatting or scoping regressions.
In `@src/core/client-state.ts`:
- Around line 40-49: The getNostrGroupIdHex function currently returns
bytesToHex(clientState.groupContext.groupId) early which is semantically
confusing (it returns the MLS groupId, not the Nostr ID) and uses a loose
truthiness check on a Uint8Array; update getNostrGroupIdHex to always obtain
marmotData via extractMarmotGroupData(clientState) and return
bytesToHex(marmotData.nostrGroupId), removing the early return from
clientState.groupContext.groupId, or alternatively add a clear comment
explaining equivalence if you intentionally want to treat MLS groupId as the
Nostr ID; also replace the truthiness check with an explicit null/undefined
check for groupContext.groupId when used elsewhere to avoid confusion.
In `@src/core/key-package-event.ts`:
- Around line 185-190: The code currently finds versionName from
protocolVersions and falls back silently to String(keyPackage.version) which can
produce ambiguous values like "1" — update the logic around protocolVersions,
versionName, and version to detect when versionName is undefined, emit a warning
(use the project's logger or console.warn) indicating the unmatched
keyPackage.version, and then produce a normalized display string (e.g., map
integer versions to "X.0" such as turning 1 into "1.0" or otherwise use
String(keyPackage.version) if non-integer) so consumers get a consistent NIP
tag; apply this change in the block that computes versionName/version.
🧹 Nitpick comments (19)
examples/src/components/extension-badge.tsx (1)
16-17: Redundant alias assignment.
extensionTypeIdis just assignedextensionTypewith no transformation. You could useextensionTypedirectly throughout the component and drop this line.♻️ Suggested simplification
- // Extension type is now always a number in ts-mls v2 - const extensionTypeId = extensionType; - - const isGrease = greaseValues.includes(extensionTypeId); + const isGrease = greaseValues.includes(extensionType); // Find the extension name from the extendedExtensionTypes map const extensionName = Object.entries(extendedExtensionTypes).find( - ([_, value]) => value === extensionTypeId, + ([_, value]) => value === extensionType, )?.[0] ?? (isGrease ? "GREASE" : "Unknown"); // Format the hex ID with 0x prefix - const hexId = `0x${extensionTypeId.toString(16).padStart(4, "0")}`; + const hexId = `0x${extensionType.toString(16).padStart(4, "0")}`;src/client/group/proposals/update-metadata.ts (1)
6-7: Nit: merge the twots-mlsimports into a single statement.-import { defaultProposalTypes } from "ts-mls"; -import { type GroupContextExtension } from "ts-mls"; +import { defaultProposalTypes, type GroupContextExtension } from "ts-mls";examples/src/components/key-package/leaf-node-capabilities.tsx (1)
40-41: Hardcoded version name reduces extensibility.The old code dynamically resolved version names from the
protocolVersionsmap, while this now only recognizesmls10and labels everything else "Unknown". This is fine given MLS 1.0 is currently the only defined version, but ifprotocolVersionsgains more entries in the future, this will need manual updates.Consider a reverse-lookup helper if the
protocolVersionsobject grows, e.g.:♻️ Optional: derive name from the object
- {version} ( - {protocolVersions.mls10 === version ? "mls10" : "Unknown"}) + {version} ( + {Object.entries(protocolVersions).find(([, v]) => v === version)?.[0] ?? "Unknown"})src/__tests__/ingest-commit-race.test.ts (3)
270-278: Dead code:addProposalis unused.The
addProposalvariable (lines 270-273) with the old"add" as constproposal type is never referenced — onlyaddProposalTyped(lines 275-278) is passed tocreateCommitat line 287. Remove the leftover variable.🧹 Proposed fix
- const addProposal = { - proposalType: "add" as const, - add: { keyPackage: memberKeyPackage.publicPackage }, - }; - - const addProposalTyped = { + const addProposal = { proposalType: defaultProposalTypes.add, add: { keyPackage: memberKeyPackage.publicPackage }, };Then update line 287:
- extraProposals: [addProposalTyped as any], + extraProposals: [addProposal as any],
144-147: Excessiveas anycasts undermine type safety of the v2 migration.Throughout the file,
as anyis used heavily (lines 109, 146-147, 199, 244, 287, 300, 354, 377, 416, 429, 478, 542). This specific clone pattern is particularly opaque:const decodedAdminGroupState = decode( clientStateDecoder, encode(clientStateEncoder, adminStateEpoch1) as any, ) as any;If
encodereturns a type thatdecodedoesn't accept, the doubleas anysilently suppresses what could be a real mismatch between the encoder output and decoder input types. Consider either:
- Adding a
structuredClone-based deep-copy helper if the state is serializable, or- Typing the intermediate value explicitly (e.g., casting to the expected
Uint8Arrayor serialized form) instead of blanketas any.The same pattern of
as anyonencode(...)anddecode(...)calls throughout the file suggests a systematic type mismatch between the v2 encoder/decoder generics and the concrete types used here. It would be worth addressing at the source (e.g., a typed wrapper or proper generics) rather than sprinklingas anyin every call site.
344-351: UnusednewStatefromcreateApplicationMessage.
newStateis destructured at line 344 but never used — the test intentionally exercises thegroup.ingest()path instead. Consider prefixing with_(i.e.,_newState) to signal the intentional discard and avoid lint warnings.examples/src/examples/key-package/manager.tsx (1)
337-348: Nit:availableCipherSuitescould useCiphersuiteId[]for type consistency.
cipherSuiteFilteris typed asCiphersuiteId | "all"butavailableCipherSuitesisnumber[]. UsingCiphersuiteId[]here (and at line 582Set<CiphersuiteId>) would keep the typing consistent within this component.examples/src/lib/group-store.ts (1)
8-22: The TODO and the manual namespace sync comment are worth tracking.The
TODO: not reactive after v2 refactor(line 8) indicates a known regression. The comment on lines 14–17 about keeping the storage namespace in sync withmarmot-client.tsis helpful but fragile — consider extracting the namespace pattern into a shared helper to prevent silent divergence.Want me to open an issue to track the reactivity regression and/or create a shared namespace helper?
src/core/marmot-group-data.ts (2)
184-253: Trailing bytes in decoded data are silently ignored.After decoding
imageNonce, the function returns without verifying that all bytes were consumed. If the encoded payload has extra trailing bytes (e.g., from corruption or a format mismatch), they'll be silently discarded. Consider adding a check or at minimum a debug warning.Optional: Add trailing-bytes check
const imageNonceRes = decodeOptionalFixed(data, offset, 12); if (!imageNonceRes) throw new Error("Extension data too short"); - const [imageNonce] = imageNonceRes; + const [imageNonce, finalOffset] = imageNonceRes; + + if (finalOffset !== data.length) { + console.warn( + `MarmotGroupData has ${data.length - finalOffset} trailing bytes`, + ); + } return {
70-72:isHexKeyis duplicated fromsrc/core/credential.tsand should be imported instead.The function at lines 70-72 is an exact copy of the exported function at
src/core/credential.ts:6. Sincecredential.tsalready exportsisHexKeyand there are no circular dependencies between the two files, replace the local definition with an import to maintain a single source of truth.src/__tests__/marmot-group-data.test.ts (1)
1-26: Unused import:defaultCredentialTypes.
defaultCredentialTypesis imported at line 8 but never referenced anywhere in this test file. It may be a leftover from the migration.🧹 Remove unused import
import { CiphersuiteName, ciphersuites, clientStateDecoder, clientStateEncoder, createGroup, decode, - defaultCredentialTypes, encode, getCiphersuiteImpl, unsafeTestingAuthenticationService, } from "ts-mls";src/__tests__/admin-verification.test.ts (1)
152-156: StoringClientStateobject whereSerializedClientState(bytes) is expected.
store.setexpectsSerializedClientState(aUint8Array), but you're passing aClientStateobject withas any. While the store isn't read back in this test so it won't break, it defeats the purpose of the typed store interface and could mask issues if the test evolves.Consider serializing properly (using
serializeClientState) or, if the store is truly unused, remove thestore.setcalls.src/client/marmot-client.ts (4)
348-362: Redundant variable aliasing forwelcome.Lines 349–350 assign
getWelcome(...)tomlsWelcomeand immediately alias it towelcome. One variable suffices.🧹 Simplify
- const mlsWelcome = getWelcome(welcomeRumor); - const welcome = mlsWelcome; + const welcome = getWelcome(welcomeRumor);
122-130: No validation when looking up ciphersuite by name.If
name(or the default) doesn't exist in theciphersuitesmap,idwill beundefinedandgetCiphersuiteImpl(undefined)will likely throw a confusing error downstream. A guard here would give a clearer failure message.🛡️ Add a guard
private async getCiphersuiteImpl(name?: CiphersuiteName) { const ciphersuiteName = name ?? "MLS_128_DHKEMX25519_AES128GCM_SHA256_Ed25519"; const id = ciphersuites[ciphersuiteName]; + if (id === undefined) { + throw new Error(`Unknown ciphersuite name: ${ciphersuiteName}`); + } return await this.cryptoProvider.getCiphersuiteImpl(id); }
387-394: Manual byte-by-byte comparison withas any— fragile and hard to read.The
welcome.secretsentries are typed asany, and the comparison logic reimplements byte equality inline. Consider extracting a smallbytesEqualhelper (or using one from@noble/hashes/utilsif available) to make intent clearer and avoid theanycasts on individual elements.
410-415:console.login production code for debugging.This log statement (and similar ones sprinkled throughout the welcome flow in
marmot-group.ts) should either use a structured logger or be removed before release.src/client/group/marmot-group.ts (3)
400-401: Redundant variable aliasing:const mlsMessage = message.
messagefrom the destructured return is immediately assigned tomlsMessageand used once (line 406). Simplify by usingmessagedirectly or destructuring with the desired name.🧹 Simplify
- const { newState, message } = await createApplicationMessage({ + const { newState, message: mlsMessage } = await createApplicationMessage({ context: { cipherSuite: this.ciphersuite, authService: marmotAuthService, externalPsks: {}, }, state: this.state, message: applicationData, }); - - // The message returned is the MLS message (not privateMessage directly) - const mlsMessage = message;
545-611: Multipleconsole.log/console.warncalls in the welcome flow.Lines 545, 577-580, 582-588, 594-598, 608-611 use
console.logandconsole.warnfor debugging. In production code, these should either be removed or routed through a structured logger with appropriate levels.
613-613: TODO: incomplete error handling for welcome publish failures.The comment notes that publish failures for welcome messages should trigger later retry. Would you like me to open a tracking issue for this?
| cipherSuite: impl, | ||
| authService: unsafeTestingAuthenticationService, | ||
| }, | ||
| welcome: welcome!.welcome ?? welcome, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the test file and understand the context around line 122
head -130 src/__tests__/ingest-commit-race.test.ts | tail -20Repository: marmot-protocol/marmot-ts
Length of output: 850
🏁 Script executed:
# Look for imports and the full context of the function containing this line
head -20 src/__tests__/ingest-commit-race.test.tsRepository: marmot-protocol/marmot-ts
Length of output: 770
🏁 Script executed:
# Find where createCommit comes from - search for its import or definition
rg -n "createCommit" --type=ts -B2 -A2 src/__tests__/ingest-commit-race.test.tsRepository: marmot-protocol/marmot-ts
Length of output: 1481
🏁 Script executed:
# Search for createCommit definition in the codebase
rg -n "createCommit\s*[:(=]" --type=ts --type=tsx -B1 -A5Repository: marmot-protocol/marmot-ts
Length of output: 96
🏁 Script executed:
# Look for ts-mls package files in the repo
find . -type f -name "*.d.ts" | grep -i mls | head -20Repository: marmot-protocol/marmot-ts
Length of output: 51
🏁 Script executed:
# Check if ts-mls is installed and where
fd -t f "ts-mls" -HRepository: marmot-protocol/marmot-ts
Length of output: 51
🏁 Script executed:
# Look for type definitions or source files related to createCommit
rg -n "export.*createCommit" --type=ts -A10Repository: marmot-protocol/marmot-ts
Length of output: 711
🏁 Script executed:
# Check the test file more broadly to understand the welcome type
cat -n src/__tests__/ingest-commit-race.test.ts | sed -n '100,130p'Repository: marmot-protocol/marmot-ts
Length of output: 1544
🏁 Script executed:
# Check package.json for ts-mls
cat package.json | grep -A2 -B2 "ts-mls"Repository: marmot-protocol/marmot-ts
Length of output: 184
🏁 Script executed:
# Look at other uses of createCommit in the codebase to see how welcome is typically handled
rg -n "createCommit" --type=ts -A5 | grep -A5 "welcome"Repository: marmot-protocol/marmot-ts
Length of output: 2415
🏁 Script executed:
# Check the joinGroup function signature to see what it expects for welcome parameter
rg -n "joinGroup" --type=ts -B2 -A8 src/__tests__/ingest-commit-race.test.tsRepository: marmot-protocol/marmot-ts
Length of output: 1040
🏁 Script executed:
# Look for type definitions or comments about the welcome type
rg -n "welcome" --type=ts src/__tests__/ingest-commit-race.test.ts | head -20Repository: marmot-protocol/marmot-ts
Length of output: 532
🏁 Script executed:
# Check how marmot-group.ts handles welcome from createCommit
cat -n src/client/group/marmot-group.ts | sed -n '505,530p'Repository: marmot-protocol/marmot-ts
Length of output: 1356
🏁 Script executed:
# Search for other calls to joinGroup in the codebase to see how welcome is typically passed
rg -n "joinGroup" --type=ts -B3 -A5 -g '!node_modules'Repository: marmot-protocol/marmot-ts
Length of output: 8797
🏁 Script executed:
# Check if there are any type assertions or comments about welcome type elsewhere
rg -n "welcome.*welcome\|MLSWelcome\|Welcome.*type" --type=ts -i -g '!node_modules'Repository: marmot-protocol/marmot-ts
Length of output: 51
🏁 Script executed:
# Check what welcome parameter is passed in marmot-client.ts
cat -n src/client/marmot-client.ts | sed -n '425,445p'Repository: marmot-protocol/marmot-ts
Length of output: 968
🏁 Script executed:
# Check what happens to welcome in marmot-group.ts after it's obtained from createCommit
cat -n src/client/group/marmot-group.ts | sed -n '514,550p'Repository: marmot-protocol/marmot-ts
Length of output: 1833
🏁 Script executed:
# Check the context around line 550 in marmot-group.ts to see the complete comment
cat -n src/client/group/marmot-group.ts | sed -n '548,570p'Repository: marmot-protocol/marmot-ts
Length of output: 1144
🏁 Script executed:
# Check if there's a type definition for what createCommit returns
rg -n "type.*Welcome\|interface.*Welcome" --type=ts src/Repository: marmot-protocol/marmot-ts
Length of output: 51
Inconsistent welcome type handling across codebase: welcome!.welcome ?? welcome masks underlying type uncertainty in ts-mls v2.0.0-rc.7.
The defensive pattern exists in multiple test files and reflects real type instability: marmot-group.ts explicitly comments that "In v2, welcome is wrapped in MlsWelcomeMessage, need to access welcome.welcome" (line 550–551), yet marmot-client.ts passes welcome directly to joinGroup without unwrapping (line 438). This inconsistency suggests the API's return type is either undocumented or genuinely uncertain during this RC phase.
Either pinpoint the actual guaranteed shape from ts-mls v2.0.0-rc.7 and use it consistently, or add a clarifying comment explaining why the fallback is necessary. This pattern currently masks the real type contract and risks silent failures if the API changes.
🤖 Prompt for AI Agents
In `@src/__tests__/ingest-commit-race.test.ts` at line 122, The test uses a
defensive expression "welcome!.welcome ?? welcome" which hides the real shape of
the ts-mls response; update code to consistently unwrap or document the expected
type: determine whether the API returns MlsWelcomeMessage or raw welcome, then
either (A) always unwrap the object before passing it to joinGroup and other
call sites (search for the local variable "welcome" and references in joinGroup
and marmot-group.ts/marmot-client.ts and normalize to welcome.welcome where
appropriate), or (B) add a concise comment next to the expression and at the
call sites (e.g., where joinGroup is invoked) explaining that in ts-mls
v2.0.0-rc.7 the welcome may be wrapped and the fallback is intentional; apply
the chosen approach consistently across ingest-commit-race.test.ts,
marmot-group.ts, and marmot-client.ts to eliminate the mixed handling.
src/client/group/marmot-group.ts
Outdated
| // Send all welcome events in parallel | ||
| await Promise.allSettled( | ||
| options.welcomeRecipients.map(async (recipient) => { | ||
| const welcomeRumor = createWelcomeRumor({ | ||
| welcome, | ||
| author: actorPubkey, | ||
| groupRelays: groupData.relays, | ||
| keyPackageEventId: recipient.keyPackageEventId, | ||
| }); | ||
|
|
||
| // Gift wrap the welcome event to the newly added user | ||
| const giftWrapEvent = await createGiftWrap({ | ||
| rumor: welcomeRumor, | ||
| recipient: recipient.pubkey, | ||
| signer: this.signer, | ||
| }); | ||
|
|
||
| // Get the newly added user's inbox relays using the GroupNostrInterface | ||
| // Fallback to group relays if inbox relays are not available | ||
| let inboxRelays: string[]; | ||
| try { | ||
| inboxRelays = await this.network.getUserInboxRelays( | ||
| recipient.pubkey, | ||
| ); | ||
| console.log( | ||
| `[MarmotGroup.commit] Retrieved inbox relays for recipient:`, | ||
| // In v2, welcome is wrapped in MlsWelcomeMessage, need to access welcome.welcome | ||
| const innerWelcome = welcome?.welcome; | ||
| if (!innerWelcome) return response; | ||
| { | ||
| await Promise.allSettled( | ||
| options.welcomeRecipients.map(async (recipient) => { | ||
| const welcomeRumor = createWelcomeRumor({ | ||
| welcome: innerWelcome, | ||
| author: actorPubkey, | ||
| groupRelays: groupData.relays, | ||
| keyPackageEventId: recipient.keyPackageEventId, | ||
| }); | ||
|
|
||
| // Gift wrap the welcome event to the newly added user | ||
| const giftWrapEvent = await createGiftWrap({ | ||
| rumor: welcomeRumor, | ||
| recipient: recipient.pubkey, | ||
| signer: this.signer, | ||
| }); | ||
|
|
||
| // Get the newly added user's inbox relays using the GroupNostrInterface | ||
| // Fallback to group relays if inbox relays are not available | ||
| let inboxRelays: string[]; | ||
| try { | ||
| inboxRelays = await this.network.getUserInboxRelays( | ||
| recipient.pubkey, | ||
| ); | ||
| console.log( | ||
| `[MarmotGroup.commit] Retrieved inbox relays for recipient:`, | ||
| inboxRelays, | ||
| ); | ||
| } catch (error) { | ||
| console.warn( | ||
| `[MarmotGroup.commit] Failed to get inbox relays for recipient ${recipient.pubkey.slice( | ||
| 0, | ||
| 16, | ||
| )}...:`, | ||
| error, | ||
| ); | ||
| // Fallback to group relays | ||
| inboxRelays = groupData.relays || []; | ||
| } | ||
|
|
||
| if (inboxRelays.length === 0) { | ||
| console.warn( | ||
| `No relays available to send Welcome to recipient ${recipient.pubkey.slice( | ||
| 0, | ||
| 16, | ||
| )}...`, | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| const publishResult = await this.network.publish( | ||
| inboxRelays, | ||
| giftWrapEvent, | ||
| ); | ||
| } catch (error) { | ||
| console.warn( | ||
| `[MarmotGroup.commit] Failed to get inbox relays for recipient ${recipient.pubkey.slice( | ||
| 0, | ||
| 16, | ||
| )}...:`, | ||
| error, | ||
| ); | ||
| // Fallback to group relays | ||
| inboxRelays = groupData.relays || []; | ||
| } | ||
|
|
||
| if (inboxRelays.length === 0) { | ||
| console.warn( | ||
| `No relays available to send Welcome to recipient ${recipient.pubkey.slice( | ||
| 0, | ||
| 16, | ||
| )}...`, | ||
| console.log( | ||
| `[MarmotGroup.commit] Gift wrap publish result:`, | ||
| publishResult, | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| const publishResult = await this.network.publish( | ||
| inboxRelays, | ||
| giftWrapEvent, | ||
| ); | ||
|
|
||
| console.log( | ||
| `[MarmotGroup.commit] Gift wrap publish result:`, | ||
| publishResult, | ||
| ); | ||
|
|
||
| // TODO: need to detect publish failure to attempt to send later | ||
| }), | ||
| ); | ||
| // TODO: need to detect publish failure to attempt to send later | ||
| }), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Orphaned bare block { … } after the early-return guard — likely a refactoring artifact.
After if (!innerWelcome) return response; on line 552, the code continues with a bare { block (line 553) that always executes. This is syntactically valid but misleading — it reads as if it was part of a conditional that lost its if. The braces serve no purpose here.
🧹 Remove the unnecessary bare block
const innerWelcome = welcome?.welcome;
if (!innerWelcome) return response;
- {
- await Promise.allSettled(
- options.welcomeRecipients.map(async (recipient) => {
+ await Promise.allSettled(
+ options.welcomeRecipients.map(async (recipient) => {
// ... body unchanged ...
- }),
- );
- }
+ }),
+ );📝 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.
| // Send all welcome events in parallel | |
| await Promise.allSettled( | |
| options.welcomeRecipients.map(async (recipient) => { | |
| const welcomeRumor = createWelcomeRumor({ | |
| welcome, | |
| author: actorPubkey, | |
| groupRelays: groupData.relays, | |
| keyPackageEventId: recipient.keyPackageEventId, | |
| }); | |
| // Gift wrap the welcome event to the newly added user | |
| const giftWrapEvent = await createGiftWrap({ | |
| rumor: welcomeRumor, | |
| recipient: recipient.pubkey, | |
| signer: this.signer, | |
| }); | |
| // Get the newly added user's inbox relays using the GroupNostrInterface | |
| // Fallback to group relays if inbox relays are not available | |
| let inboxRelays: string[]; | |
| try { | |
| inboxRelays = await this.network.getUserInboxRelays( | |
| recipient.pubkey, | |
| ); | |
| console.log( | |
| `[MarmotGroup.commit] Retrieved inbox relays for recipient:`, | |
| // In v2, welcome is wrapped in MlsWelcomeMessage, need to access welcome.welcome | |
| const innerWelcome = welcome?.welcome; | |
| if (!innerWelcome) return response; | |
| { | |
| await Promise.allSettled( | |
| options.welcomeRecipients.map(async (recipient) => { | |
| const welcomeRumor = createWelcomeRumor({ | |
| welcome: innerWelcome, | |
| author: actorPubkey, | |
| groupRelays: groupData.relays, | |
| keyPackageEventId: recipient.keyPackageEventId, | |
| }); | |
| // Gift wrap the welcome event to the newly added user | |
| const giftWrapEvent = await createGiftWrap({ | |
| rumor: welcomeRumor, | |
| recipient: recipient.pubkey, | |
| signer: this.signer, | |
| }); | |
| // Get the newly added user's inbox relays using the GroupNostrInterface | |
| // Fallback to group relays if inbox relays are not available | |
| let inboxRelays: string[]; | |
| try { | |
| inboxRelays = await this.network.getUserInboxRelays( | |
| recipient.pubkey, | |
| ); | |
| console.log( | |
| `[MarmotGroup.commit] Retrieved inbox relays for recipient:`, | |
| inboxRelays, | |
| ); | |
| } catch (error) { | |
| console.warn( | |
| `[MarmotGroup.commit] Failed to get inbox relays for recipient ${recipient.pubkey.slice( | |
| 0, | |
| 16, | |
| )}...:`, | |
| error, | |
| ); | |
| // Fallback to group relays | |
| inboxRelays = groupData.relays || []; | |
| } | |
| if (inboxRelays.length === 0) { | |
| console.warn( | |
| `No relays available to send Welcome to recipient ${recipient.pubkey.slice( | |
| 0, | |
| 16, | |
| )}...`, | |
| ); | |
| return; | |
| } | |
| const publishResult = await this.network.publish( | |
| inboxRelays, | |
| giftWrapEvent, | |
| ); | |
| } catch (error) { | |
| console.warn( | |
| `[MarmotGroup.commit] Failed to get inbox relays for recipient ${recipient.pubkey.slice( | |
| 0, | |
| 16, | |
| )}...:`, | |
| error, | |
| ); | |
| // Fallback to group relays | |
| inboxRelays = groupData.relays || []; | |
| } | |
| if (inboxRelays.length === 0) { | |
| console.warn( | |
| `No relays available to send Welcome to recipient ${recipient.pubkey.slice( | |
| 0, | |
| 16, | |
| )}...`, | |
| console.log( | |
| `[MarmotGroup.commit] Gift wrap publish result:`, | |
| publishResult, | |
| ); | |
| return; | |
| } | |
| const publishResult = await this.network.publish( | |
| inboxRelays, | |
| giftWrapEvent, | |
| ); | |
| console.log( | |
| `[MarmotGroup.commit] Gift wrap publish result:`, | |
| publishResult, | |
| ); | |
| // TODO: need to detect publish failure to attempt to send later | |
| }), | |
| ); | |
| // TODO: need to detect publish failure to attempt to send later | |
| }), | |
| ); | |
| } | |
| // Send all welcome events in parallel | |
| // In v2, welcome is wrapped in MlsWelcomeMessage, need to access welcome.welcome | |
| const innerWelcome = welcome?.welcome; | |
| if (!innerWelcome) return response; | |
| await Promise.allSettled( | |
| options.welcomeRecipients.map(async (recipient) => { | |
| const welcomeRumor = createWelcomeRumor({ | |
| welcome: innerWelcome, | |
| author: actorPubkey, | |
| groupRelays: groupData.relays, | |
| keyPackageEventId: recipient.keyPackageEventId, | |
| }); | |
| // Gift wrap the welcome event to the newly added user | |
| const giftWrapEvent = await createGiftWrap({ | |
| rumor: welcomeRumor, | |
| recipient: recipient.pubkey, | |
| signer: this.signer, | |
| }); | |
| // Get the newly added user's inbox relays using the GroupNostrInterface | |
| // Fallback to group relays if inbox relays are not available | |
| let inboxRelays: string[]; | |
| try { | |
| inboxRelays = await this.network.getUserInboxRelays( | |
| recipient.pubkey, | |
| ); | |
| console.log( | |
| `[MarmotGroup.commit] Retrieved inbox relays for recipient:`, | |
| inboxRelays, | |
| ); | |
| } catch (error) { | |
| console.warn( | |
| `[MarmotGroup.commit] Failed to get inbox relays for recipient ${recipient.pubkey.slice( | |
| 0, | |
| 16, | |
| )}...:`, | |
| error, | |
| ); | |
| // Fallback to group relays | |
| inboxRelays = groupData.relays || []; | |
| } | |
| if (inboxRelays.length === 0) { | |
| console.warn( | |
| `No relays available to send Welcome to recipient ${recipient.pubkey.slice( | |
| 0, | |
| 16, | |
| )}...`, | |
| ); | |
| return; | |
| } | |
| const publishResult = await this.network.publish( | |
| inboxRelays, | |
| giftWrapEvent, | |
| ); | |
| console.log( | |
| `[MarmotGroup.commit] Gift wrap publish result:`, | |
| publishResult, | |
| ); | |
| // TODO: need to detect publish failure to attempt to send later | |
| }), | |
| ); |
🤖 Prompt for AI Agents
In `@src/client/group/marmot-group.ts` around lines 549 - 616, There is an
unnecessary bare block following the early-return guard after computing
innerWelcome in MarmotGroup.commit; remove the stray opening and closing braces
that wrap the Promise.allSettled call so the code is simply executed after the
`if (!innerWelcome) return response;` check (i.e., dedent the block containing
Promise.allSettled and its async callback that references
options.welcomeRecipients, createWelcomeRumor, createGiftWrap,
this.network.getUserInboxRelays, and this.network.publish), preserving all
existing logic and await behavior and then run the linter/tests to ensure no
formatting or scoping regressions.
| // Get the protocol version - keyPackage.version is a numeric ProtocolVersionValue | ||
| // NIP tag expects a display string like "1.0". | ||
| const versionName = ( | ||
| Object.keys(protocolVersions) as Array<keyof typeof protocolVersions> | ||
| ).find((k) => protocolVersions[k] === keyPackage.version); | ||
| const version = versionName === "mls10" ? "1.0" : String(keyPackage.version); |
There was a problem hiding this comment.
Defensive: versionName could be undefined if protocolVersions has no matching entry.
If keyPackage.version doesn't match any value in protocolVersions, versionName is undefined, and version falls back to String(keyPackage.version) (a raw numeric string like "1"). This is handled, but the resulting tag value may not match what consumers expect (e.g., "1" vs "1.0"). Worth a log/warning if this fallback is hit in practice.
🤖 Prompt for AI Agents
In `@src/core/key-package-event.ts` around lines 185 - 190, The code currently
finds versionName from protocolVersions and falls back silently to
String(keyPackage.version) which can produce ambiguous values like "1" — update
the logic around protocolVersions, versionName, and version to detect when
versionName is undefined, emit a warning (use the project's logger or
console.warn) indicating the unmatched keyPackage.version, and then produce a
normalized display string (e.g., map integer versions to "X.0" such as turning 1
into "1.0" or otherwise use String(keyPackage.version) if non-integer) so
consumers get a consistent NIP tag; apply this change in the block that computes
versionName/version.
- Replace group-store.ts with groups.ts using RxJS observables - Update components to use new groups$, groupSummaries$, and selectedGroup$ observables - Add destroyGroup helper for group deletion - Remove direct GroupStore usage in favor of MarmotClient events - Improve reactivity for group lists and counts - Update group management UI components to use new reactive patterns
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
examples/src/examples/group/chat.tsx (1)
452-466:⚠️ Potential issue | 🟡 Minor
handleMessagesReceivedis recreated every render, causing the effect to re-subscribe continuously.
handleMessagesReceived(Line 430) is defined inline withoutuseCallback, so it gets a new reference on every render. Since it's listed in the effect's dependency array (Line 466), the effect re-runs each render — unsubscribing and re-subscribing the application message callback. This can cause missed messages or duplicates during the teardown/setup cycle.♻️ Stabilize with useCallback
- const handleMessagesReceived = (newMessages: Rumor[]) => { + const handleMessagesReceived = useCallback((newMessages: Rumor[]) => { setMessages((prev) => { const messageMap = new Map<string, Rumor>(); prev.forEach((msg) => { if (msg.id) messageMap.set(msg.id, msg); }); newMessages.forEach((msg) => { if (msg.id) messageMap.set(msg.id, msg); }); const combined = Array.from(messageMap.values()); return combined.sort((a, b) => a.created_at - b.created_at); }); - }; + }, []);examples/src/examples/group/add-member.tsx (1)
645-651:⚠️ Potential issue | 🟡 Minor
selectedGroupDatais fetched but never used after the existence check.In each handler,
groups.find(g => g.groupId === selectedGroupKey)is called and assigned toselectedGroupData, but onlyselectedGroup(from thegroup$observable) is used for the actual operation. This appears to be leftover from the old data model. Either remove the dead lookup or use it for something meaningful.Also applies to: 681-687, 717-723
🤖 Fix all issues with AI agents
In `@examples/src/components/group-store-modal.tsx`:
- Around line 24-30: handleDelete currently awaits destroyGroup(groupIdHex)
without catching rejections, causing unhandled promise errors; update
handleDelete (the function named handleDelete) to wrap the await
destroyGroup(...) call in a try/catch similar to handleClearAll, and on error
call alert(...) or processLogger/error UI with a clear message including the
caught error to inform the user and avoid silent failures.
- Around line 215-216: The UI bug comes from using the array index as the React
key in the entries.map call (entries.map((group, index) => <StoredGroupDetails
key={index} ... />)), which can cause component state to stick to the wrong item
after deletions; change the key to a stable unique identifier from the group
object (e.g., group.id) so update mapping becomes entries.map((group) =>
<StoredGroupDetails key={group.id} group={group} ... />), ensuring
StoredGroupDetails receives the stable key and avoiding stale open/closed state.
In `@examples/src/lib/groups.ts`:
- Around line 72-83: groups$ currently uses Promise.all over
ids.map(client.getGroup) so a single rejection from client.getGroup(id) will
cause the entire switchMap to error and terminate the observable; change the
implementation in groups$ (and the inner switchMap async callback) to call
Promise.allSettled for ids.map(id => client.getGroup(id)) or wrap each
client.getGroup(id) with a try/catch so you collect only successful results,
filter out failed results before returning the groups array, and keep the
observable alive (preserve the existing types/shape returned by groups$).
- Around line 86-105: The mapping inside groupSummaries$ may throw because
getNostrGroupIdHex can throw for groups without MarmotGroupData; modify the
groups$.pipe map so each group's processing is defensive: for each group (inside
the groups.map callback) wrap the extraction of nostrGroupIdHex (call to
getNostrGroupIdHex) in a try/catch and set a safe fallback (e.g., undefined or
null) when it throws, preserving the rest of the returned GroupSummary (use
extractMarmotGroupData, getMemberCount, bytesToHex as before); ensure you do not
allow a throw to bubble up and terminate groupSummaries$ (alternatively filter
out invalid groups instead of throwing).
🧹 Nitpick comments (5)
examples/src/lib/groups.ts (1)
121-135: Redundantnullemissions inselectedGroup$.
of(null)insidemergeon Line 123 andstartWith(null)on Line 135 both cause an initialnullemission. ThestartWith(null)alone would suffice since it guaranteesnullis emitted synchronously before any other value. Theof(null)insidemergecauses a secondnullemission.♻️ Simplified construction
-export const selectedGroup$ = merge( - // emit null immediately for initial render - of(null as MarmotGroup | null), - selectedGroupId$.pipe( +export const selectedGroup$ = selectedGroupId$.pipe( switchMap((groupId) => marmotClient$.pipe( defined(), switchMap(async (client) => { if (!groupId) return null; return await client.getGroup(groupId); }), ), ), - ), -).pipe(startWith(null), shareReplay(1)); + startWith(null), + shareReplay(1), + );examples/src/lib/marmot-client.ts (1)
90-136: Pre-existing: subscription manager is not recreated on account change.When the active account changes,
marmotClient$emits a newMarmotClientinstance, but becausesubscriptionManageris still non-null from the previous client, theif (!subscriptionManager)guard on Line 110 skips recreation. The old manager continues running against the stale client. This predates this PR but is worth tracking since the newgroupsUpdatedHandlerwiring on the stale manager won't behave correctly after an account switch.examples/src/examples/group/manager.tsx (1)
171-193: Intermediate state and effect are unnecessary now that data comes from an observable.
hydratedGroupsis already reactive viauseObservable. TheuseEffect+useStatepattern forgroups/isLoadingadds complexity without benefit —setGroups(hydratedGroups.map(g => g.state))is synchronous and won't throw, so the try/catch is dead code. The loading state can be derived directly fromhydratedGroups === undefined(before the?? []fallback).Additionally,
groupCountin the dependency array is redundant since it's derived from the samegroupIds$source and will change in lockstep withhydratedGroups.♻️ Suggested simplification
function GroupManager() { - const hydratedGroups = useObservable(groups$) ?? []; + const hydratedGroupsRaw = useObservable(groups$); + const hydratedGroups = hydratedGroupsRaw ?? []; const groupCount = useObservable(groupCount$); - const [groups, setGroups] = useState<ClientState[]>([]); - const [isLoading, setIsLoading] = useState(true); + const groups = hydratedGroups.map((g) => g.state); + const isLoading = hydratedGroupsRaw === undefined; const [error, setError] = useState<string | null>(null); - - useEffect(() => { - const loadGroups = async () => { - try { - setIsLoading(true); - setError(null); - setGroups(hydratedGroups.map((g) => g.state)); - } catch (err) { - console.error("Failed to load groups:", err); - setError(err instanceof Error ? err.message : String(err)); - } finally { - setIsLoading(false); - } - }; - - loadGroups(); - }, [hydratedGroups, groupCount]);examples/src/examples/group/add-member.tsx (2)
639-672: All three action handlers duplicate the same validation/admin-check boilerplate.
handlePropose,handleCommit, andhandleInviteshare ~20 identical lines (guard checks, group lookup, account retrieval, admin verification). Extract a shared validation helper to reduce duplication:♻️ Suggested extraction
async function validateAction( selectedGroupKey: string, selectedKeyPackage: NostrEvent | null, selectedGroup: MarmotGroup | null, ): Promise<{ group: MarmotGroup; pubkey: string } | null> { if (!selectedGroupKey || !selectedKeyPackage || !selectedGroup) { console.error("Please select a group and key package"); return null; } const account = accounts.active; if (!account) { error$.next("No active account"); return null; } const pubkey = await account.signer.getPublicKey(); const groupData = selectedGroup.groupData; if (!groupData || !isAdmin(groupData, pubkey)) { error$.next("You must be an admin to perform this action"); return null; } return { group: selectedGroup, pubkey }; }Also applies to: 675-708, 711-742
575-575:useObservableMemois unnecessary for a module-level constant.
groupSummaries$is a stable module-level observable. Wrapping it inuseObservableMemo(() => groupSummaries$, [])adds indirection without benefit. UseuseObservable(groupSummaries$)directly.♻️ Simplify
- const groups = useObservableMemo(() => groupSummaries$, []) ?? []; + const groups = useObservable(groupSummaries$) ?? [];
| const handleDelete = async () => { | ||
| const confirmed = window.confirm( | ||
| `Delete group "${name}"? This will remove it from local storage.`, | ||
| ); | ||
| if (!confirmed) return; | ||
| await destroyGroup(groupIdHex); | ||
| }; |
There was a problem hiding this comment.
handleDelete swallows errors silently.
Unlike handleClearAll (which catches and alerts on failure), handleDelete has no error handling. If destroyGroup rejects, the promise rejection goes unhandled.
♻️ Add error handling
const handleDelete = async () => {
const confirmed = window.confirm(
`Delete group "${name}"? This will remove it from local storage.`,
);
if (!confirmed) return;
- await destroyGroup(groupIdHex);
+ try {
+ await destroyGroup(groupIdHex);
+ } catch (error) {
+ console.error("Failed to delete group:", error);
+ alert("Failed to delete group. Check console for details.");
+ }
};📝 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 handleDelete = async () => { | |
| const confirmed = window.confirm( | |
| `Delete group "${name}"? This will remove it from local storage.`, | |
| ); | |
| if (!confirmed) return; | |
| await destroyGroup(groupIdHex); | |
| }; | |
| const handleDelete = async () => { | |
| const confirmed = window.confirm( | |
| `Delete group "${name}"? This will remove it from local storage.`, | |
| ); | |
| if (!confirmed) return; | |
| try { | |
| await destroyGroup(groupIdHex); | |
| } catch (error) { | |
| console.error("Failed to delete group:", error); | |
| alert("Failed to delete group. Check console for details."); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In `@examples/src/components/group-store-modal.tsx` around lines 24 - 30,
handleDelete currently awaits destroyGroup(groupIdHex) without catching
rejections, causing unhandled promise errors; update handleDelete (the function
named handleDelete) to wrap the await destroyGroup(...) call in a try/catch
similar to handleClearAll, and on error call alert(...) or processLogger/error
UI with a clear message including the caught error to inform the user and avoid
silent failures.
| {entries.map((group, index) => ( | ||
| <StoredGroupDetails key={index} group={group} index={index} /> |
There was a problem hiding this comment.
Using index as key can cause stale UI after group deletion.
When a group is deleted, the remaining groups shift indices, causing React to incorrectly associate state (e.g., the <details> open/closed state) with the wrong group. Use the group ID as key instead.
♻️ Stable key
- {entries.map((group, index) => (
- <StoredGroupDetails key={index} group={group} index={index} />
+ {entries.map((group, index) => {
+ const id = bytesToHex(group.state.groupContext.groupId);
+ return (
+ <StoredGroupDetails key={id} group={group} index={index} />
+ );
+ })}
))}📝 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.
| {entries.map((group, index) => ( | |
| <StoredGroupDetails key={index} group={group} index={index} /> | |
| {entries.map((group, index) => { | |
| const id = bytesToHex(group.state.groupContext.groupId); | |
| return ( | |
| <StoredGroupDetails key={id} group={group} index={index} /> | |
| ); | |
| })} |
🤖 Prompt for AI Agents
In `@examples/src/components/group-store-modal.tsx` around lines 215 - 216, The UI
bug comes from using the array index as the React key in the entries.map call
(entries.map((group, index) => <StoredGroupDetails key={index} ... />)), which
can cause component state to stick to the wrong item after deletions; change the
key to a stable unique identifier from the group object (e.g., group.id) so
update mapping becomes entries.map((group) => <StoredGroupDetails key={group.id}
group={group} ... />), ensuring StoredGroupDetails receives the stable key and
avoiding stale open/closed state.
| export const groups$ = groupIds$.pipe( | ||
| switchMap((ids) => | ||
| marmotClient$.pipe( | ||
| defined(), | ||
| switchMap(async (client) => { | ||
| const groups = await Promise.all(ids.map((id) => client.getGroup(id))); | ||
| return groups; | ||
| }), | ||
| ), | ||
| ), | ||
| shareReplay(1), | ||
| ); |
There was a problem hiding this comment.
Promise.all will terminate the stream if any single group fails to load.
If client.getGroup(id) rejects for one group (e.g., corrupted state), Promise.all rejects and the error propagates into the switchMap, which will terminate the groups$ observable. All downstream subscribers (groupSummaries$, selectedClientState$, etc.) will also stop receiving updates.
Consider using Promise.allSettled and filtering out failures, or wrapping individual calls with a catch:
♻️ Suggested resilient loading
export const groups$ = groupIds$.pipe(
switchMap((ids) =>
marmotClient$.pipe(
defined(),
switchMap(async (client) => {
- const groups = await Promise.all(ids.map((id) => client.getGroup(id)));
- return groups;
+ const results = await Promise.allSettled(
+ ids.map((id) => client.getGroup(id)),
+ );
+ return results
+ .filter(
+ (r): r is PromiseFulfilledResult<MarmotGroup> =>
+ r.status === "fulfilled",
+ )
+ .map((r) => r.value);
}),
),
),
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 groups$ = groupIds$.pipe( | |
| switchMap((ids) => | |
| marmotClient$.pipe( | |
| defined(), | |
| switchMap(async (client) => { | |
| const groups = await Promise.all(ids.map((id) => client.getGroup(id))); | |
| return groups; | |
| }), | |
| ), | |
| ), | |
| shareReplay(1), | |
| ); | |
| export const groups$ = groupIds$.pipe( | |
| switchMap((ids) => | |
| marmotClient$.pipe( | |
| defined(), | |
| switchMap(async (client) => { | |
| const results = await Promise.allSettled( | |
| ids.map((id) => client.getGroup(id)), | |
| ); | |
| return results | |
| .filter( | |
| (r): r is PromiseFulfilledResult<MarmotGroup> => | |
| r.status === "fulfilled", | |
| ) | |
| .map((r) => r.value); | |
| }), | |
| ), | |
| ), | |
| shareReplay(1), | |
| ); |
🤖 Prompt for AI Agents
In `@examples/src/lib/groups.ts` around lines 72 - 83, groups$ currently uses
Promise.all over ids.map(client.getGroup) so a single rejection from
client.getGroup(id) will cause the entire switchMap to error and terminate the
observable; change the implementation in groups$ (and the inner switchMap async
callback) to call Promise.allSettled for ids.map(id => client.getGroup(id)) or
wrap each client.getGroup(id) with a try/catch so you collect only successful
results, filter out failed results before returning the groups array, and keep
the observable alive (preserve the existing types/shape returned by groups$).
| export const groupSummaries$ = groups$.pipe( | ||
| map((groups): GroupSummary[] => { | ||
| return groups.map((group) => { | ||
| const data = extractMarmotGroupData(group.state); | ||
| const name = data?.name ?? "Unnamed Group"; | ||
| const memberCount = getMemberCount(group.state); | ||
| const epoch = Number(group.state.groupContext.epoch); | ||
| const nostrGroupIdHex = getNostrGroupIdHex(group.state); | ||
|
|
||
| return { | ||
| groupId: bytesToHex(group.state.groupContext.groupId), | ||
| name, | ||
| epoch, | ||
| memberCount, | ||
| nostrGroupIdHex, | ||
| }; | ||
| }); | ||
| }), | ||
| shareReplay(1), | ||
| ); |
There was a problem hiding this comment.
getNostrGroupIdHex can throw and terminate groupSummaries$.
From src/core/client-state.ts (lines 39-48), getNostrGroupIdHex throws "MarmotGroupData not found in ClientState" if the group has no groupId and no MarmotGroupData extension. Since this runs inside map, an uncaught throw will error-terminate the entire observable chain.
Consider wrapping the mapping with a try/catch per group, or providing a fallback:
♻️ Suggested defensive mapping
export const groupSummaries$ = groups$.pipe(
map((groups): GroupSummary[] => {
- return groups.map((group) => {
+ return groups.flatMap((group) => {
+ try {
const data = extractMarmotGroupData(group.state);
const name = data?.name ?? "Unnamed Group";
const memberCount = getMemberCount(group.state);
const epoch = Number(group.state.groupContext.epoch);
const nostrGroupIdHex = getNostrGroupIdHex(group.state);
- return {
+ return [{
groupId: bytesToHex(group.state.groupContext.groupId),
name,
epoch,
memberCount,
nostrGroupIdHex,
- };
+ }];
+ } catch {
+ return [];
+ }
});
}),
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 groupSummaries$ = groups$.pipe( | |
| map((groups): GroupSummary[] => { | |
| return groups.map((group) => { | |
| const data = extractMarmotGroupData(group.state); | |
| const name = data?.name ?? "Unnamed Group"; | |
| const memberCount = getMemberCount(group.state); | |
| const epoch = Number(group.state.groupContext.epoch); | |
| const nostrGroupIdHex = getNostrGroupIdHex(group.state); | |
| return { | |
| groupId: bytesToHex(group.state.groupContext.groupId), | |
| name, | |
| epoch, | |
| memberCount, | |
| nostrGroupIdHex, | |
| }; | |
| }); | |
| }), | |
| shareReplay(1), | |
| ); | |
| export const groupSummaries$ = groups$.pipe( | |
| map((groups): GroupSummary[] => { | |
| return groups.flatMap((group) => { | |
| try { | |
| const data = extractMarmotGroupData(group.state); | |
| const name = data?.name ?? "Unnamed Group"; | |
| const memberCount = getMemberCount(group.state); | |
| const epoch = Number(group.state.groupContext.epoch); | |
| const nostrGroupIdHex = getNostrGroupIdHex(group.state); | |
| return [{ | |
| groupId: bytesToHex(group.state.groupContext.groupId), | |
| name, | |
| epoch, | |
| memberCount, | |
| nostrGroupIdHex, | |
| }]; | |
| } catch { | |
| return []; | |
| } | |
| }); | |
| }), | |
| shareReplay(1), | |
| ); |
🤖 Prompt for AI Agents
In `@examples/src/lib/groups.ts` around lines 86 - 105, The mapping inside
groupSummaries$ may throw because getNostrGroupIdHex can throw for groups
without MarmotGroupData; modify the groups$.pipe map so each group's processing
is defensive: for each group (inside the groups.map callback) wrap the
extraction of nostrGroupIdHex (call to getNostrGroupIdHex) in a try/catch and
set a safe fallback (e.g., undefined or null) when it throws, preserving the
rest of the returned GroupSummary (use extractMarmotGroupData, getMemberCount,
bytesToHex as before); ensure you do not allow a throw to bubble up and
terminate groupSummaries$ (alternatively filter out invalid groups instead of
throwing).
Update test to work with ts-mls v2 API where ClientState is immutable. The createCommit function now returns a new state object instead of mutating the original, eliminating the need to clone the admin state between commits.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/__tests__/ingest-commit-race.test.ts (1)
259-267:⚠️ Potential issue | 🟡 MinorDead code:
addProposalon lines 259–262 is never used.
addProposalTyped(lines 264–267) is the variable actually passed tocreateCommitat line 276. The originaladdProposalappears to be a leftover from before the migration.🧹 Proposed fix
- const addProposal = { - proposalType: "add" as const, - add: { keyPackage: memberKeyPackage.publicPackage }, - }; - - const addProposalTyped = { - proposalType: defaultProposalTypes.add, + const addProposal = { + proposalType: defaultProposalTypes.add, add: { keyPackage: memberKeyPackage.publicPackage }, };Then update line 276:
- extraProposals: [addProposalTyped as any], + extraProposals: [addProposal as any],
🧹 Nitpick comments (2)
src/__tests__/ingest-commit-race.test.ts (2)
102-111: Pervasiveas anycasts may hide v2 type mismatches.There are numerous
as anycasts throughout:extraProposals,encode(...),decode(...),mlsMessage, andproposal. While understandable during an RC migration, these suppress type-checking at the exact boundary where type mismatches are most likely. Consider narrowing the casts (e.g., specific type assertions) or adding brief// TODO: remove cast when types stabilizecomments so they don't linger.Also applies to: 135-151, 269-278, 333-340, 397-407, 461-469, 491-499
333-340:newStateis destructured but unused.The
newStatereturned bycreateApplicationMessageon line 333 is never referenced afterward. If it's intentionally unused, prefix it with_to signal intent (_newState), or remove it from the destructuring.
- Add try-catch in group fetching and summary generation to handle errors - Remove unnecessary `as any` type assertions in tests - Remove extra block in welcome event sending for better code structure - Simplify getNostrGroupIdHex by removing redundant check
3 participants
Summary by CodeRabbit
Chores
New Features
Refactor