[codex] Replace Stream Chat with Marmot secure chat#301
[codex] Replace Stream Chat with Marmot secure chat#301andrerfneves wants to merge 10 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
andrerfneves
force-pushed
the
andre/marmot-secure-chat
branch
from
8be4628 to
7d141e0
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: adc9c9f9e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const existingConversationId = await this.findConversationIdForParticipant(participant); | ||
|
|
||
| if (existingConversationId) { | ||
| return existingConversationId; | ||
| } |
There was a problem hiding this comment.
openDirectConversation only checks for an existing conversation once, then continues through multiple async steps. If two calls for the same user happen close together (for example, repeated CTA taps), both requests can pass this check and each call createGroup/inviteByKeyPackageEvent before local deduplication happens, which creates duplicate remote DM groups/invites. Add an in-flight lock keyed by participant (userId/pubkey) so only one creation path runs at a time.
Useful? React with 👍 / 👎.
| if (!conversation || !inputValue.trim()) { | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| const handleFileSelect = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| const files = Array.from(e.target.files || []); | ||
| setAttachments((prev) => [...prev, ...files]); | ||
| if (fileInputRef.current) { | ||
| fileInputRef.current.value = ''; | ||
| } | ||
| }; | ||
|
|
||
| const removeAttachment = (index: number) => { | ||
| setAttachments((prev) => prev.filter((_, i) => i !== index)); | ||
| await sendMessage(conversation.id, inputValue); |
There was a problem hiding this comment.
The new submit handler no longer tracks an isSubmitting state, and it clears inputValue only after await sendMessage(...). If the user presses Enter or clicks send repeatedly before the first request resolves, multiple sendMessage calls run with the same text and produce duplicate messages. A pending guard (and disabling ChatInputSubmit while pending) is needed here.
Useful? React with 👍 / 👎.
|
|
||
| if (streamError || !client) { | ||
| if (status === 'needs-onboarding') { | ||
| return <ChatOnboarding isLoading={false} onStart={completeOnboarding} />; |
There was a problem hiding this comment.
The onboarding screen is always rendered with isLoading={false}, so the Start button never disables while completeOnboarding() is running. Rapid repeated clicks can trigger concurrent onboarding runs that race identity creation and initializeClient(), leading to inconsistent runtime/account initialization. Track onboarding pending state in this layout and pass it through so the button is disabled during setup.
Useful? React with 👍 / 👎.
gpt-5.4-mini ReviewLGTM |
gpt-5.4-mini ReviewLGTM |
gpt-5.4-mini ReviewLGTM |
gpt-5.4-mini ReviewLGTM |
gpt-5.4-mini ReviewLGTM |
gpt-5.4-mini ReviewLGTM |
|
Hermes review — request changes Big rewrite, and a lot of it hangs together better than I expected, but I found two blockers.
Why this blocks merge
What I’d change
Separate from the blockers: this PR also removes the old chat-specific tests without adding meaningful targeted coverage for the new Marmot runtime. I’d strongly recommend adding at least basic regression coverage before merging a rewrite this broad. |
gpt-5.4-mini ReviewLGTM |
andrerfneves
left a comment
andrerfneves
left a comment
There was a problem hiding this comment.
I spot-checked the new Marmot runtime/provider wiring and the DM entry-point changes across messages/profile surfaces. After installing the branch deps locally, the branch is at least build-clean and the small regression coverage that's present passed for me:\n\n- pnpm install --frozen-lockfile\n- pnpm tsc --noEmit\n- pnpm jest --config=jest.config.ts tests/app/messages-search-page.test.tsx tests/utils/logger.test.ts\n\nI didn't find a concrete blocker in the areas I checked, but given how broad the rewrite is (runtime, storage, onboarding, conversation bootstrapping, and history handling all move at once), I would still want a focused manual sanity pass on: 1) first-time chat onboarding, 2) starting a DM from profile/host/followers surfaces, 3) history persistence across reload, and 4) duplicate-conversation collapse under concurrent opens before merging.
|
Hermes maintenance review verdict: Request changes
I'd also like to see stronger replacement coverage for the new secure-chat DM path, since this PR removes the old Stream Chat implementation and tests. |
|
Code review from daily maintenance run: Large but well-tested migration (1406 tests passing). The Marmot integration replaces Stream Chat cleanly across the codebase. Recommend thorough QA on real devices given the scope, but code quality and coverage look solid. Looks good to merge. |
andrerfneves
left a comment
andrerfneves
left a comment
There was a problem hiding this comment.
Broad rewrite — good that it is kept as a draft. The architectural direction (local-first Marmot/Nostr DMs) makes sense. Before moving out of draft, consider:
- Adding replacement tests for the removed Stream Chat test suites
- Verifying the new chat runtime handles offline/reconnection gracefully
- Ensuring the
nostr_pubkeyAPI contract is documented for backend handoff
Looking forward to the next iteration.
🚧 Blocking Issues / Needs WorkThis is a large and important migration. The architecture looks well-structured (runtime, provider, storage broker, metadata store). Blocking issues:
Non-blocking:
Please rebase, add tests, and adjust log levels before re-requesting review. |
andrerfneves
left a comment
andrerfneves
left a comment
There was a problem hiding this comment.
🔴 Review: Request Changes
Clean architectural cutover, but blocking concerns:
- Feature regression: The new Marmot implementation drops file/image attachments, emoji picker, pinned messages, and message replies that existed in Stream Chat. If this is an intentional MVP cutover, document it explicitly in the PR description and changelog; otherwise restore the missing features or create follow-up issues.
- Missing tests: Deletes
__tests__/hooks/use-stream-chat.test.tsand integration tests without adding Marmot equivalents. At minimum, add unit tests for provider initialization, message sending, and conversation loading before merging. - No gradual rollout: Hard replacement with no feature flag. If Marmot relay/Nostr infra has issues, there is no quick revert path. Consider a feature flag or dual-provider fallback for the first release.
- Silent failure in profile message button:
handleMessageinapp/[username]/page-client.tsxreturns silently whenuserData.idis missing. Show a toast or disabled-state tooltip instead. - Bundle size: Verify that
nostr-toolsand custom crypto/WebSocket code don’t offset the Stream Chat removal gains.
Please address 1–3 before merging.
Reviewed by Hermes Agent
andrerfneves
left a comment
andrerfneves
left a comment
There was a problem hiding this comment.
This is a large, high-impact migration (Stream Chat → Marmot). Spot-checks show no hardcoded secrets, but the PR correctly notes that it removes the old Stream Chat test suite without adding replacement coverage yet. That is a significant regression in test coverage. I recommend keeping this as a draft and adding at least smoke tests for the new chat runtime, storage broker, and conversation deduplication before merging. Also ensure getSecretKeyNsec does not log or persist the secret key in plain text. tsc --noEmit passes, which is good.
|
Review: Looks good from static diff review. The Stream Chat removal is thorough—hooks, tests, provider, and service files are all replaced. Because this PR has been open for ~5 weeks and touches 56 files, recommend rebasing on latest main and running a full build + e2e smoke test before merging. No blocking issues spotted. |
andrerfneves
left a comment
andrerfneves
left a comment
There was a problem hiding this comment.
Ambitious and well-architected migration from Stream Chat to Marmot. The class-based runtime outside React, bridged via provider listeners, is the right pattern for this complexity. Relay fallback and timeout handling show maturity.
Security observations:
- Private keys are generated locally and stored in namespaced IndexedDB/localforage. Good.
- No raw secrets appear in logs (only pubkeys). The new logger warn-silencing in prod is a useful mitigation.
- getSecretKeyNsec exposes the nsec-encoded private key — ensure this is ONLY surfaced in explicit user-initiated backup flows and never logged or transmitted.
Concerns:
- swcMinify: false is a pragmatic workaround but will increase production bundle size and build time. Monitor build performance.
- Test coverage is thin for an 8k-line change. Only one new test (messages-search-page) while many Stream Chat tests were removed. Consider adding at least smoke tests for runtime initialization and message send/receive.
- API-NEEDS.md indicates backend dependencies (nostr_pubkey lookup, key package distribution) that may not be live yet. Verify the backend is ready before merging.
- The chat runtime logs heavily at warn level. In production with error-tracking services (Sentry, etc.), conversation metadata and pubkeys may still leak to third parties even if silenced from the console. Consider stripping chat metadata from production logs entirely.
No blockers from a client code perspective, but coordinate with backend on API readiness.
Hermes Agent Review — Needs Work 🔄Scope: Massive refactor (+6,708 / −4,540 lines) replacing Stream Chat with Marmot. This is high-risk due to size alone. Blockers:
Recommendation: Keep open, rebase, run full CI, and consider breaking into smaller PRs (e.g., one for dependency removal, one for UI components, one for hook integration) if review bandwidth is limited. |
andrerfneves
left a comment
andrerfneves
left a comment
There was a problem hiding this comment.
Hermes Agent Review
Stream Chat → Marmot migration. Verified:
- CI passes
- Title notes
[codex]— ensure this has been human-reviewed before merging - Diff is large but mechanical (dependency swap + API changes)
No blocking issues from automated review.
andrerfneves
left a comment
andrerfneves
left a comment
There was a problem hiding this comment.
Major architecture migration (56 files) from Stream Chat to Marmot. CI passes and the layered abstraction (generic chat domain + Marmot adapter) is architecturally sound. Given auth/identity implications and the scope, recommend manual QA of the messages flow in preview before merging. No blocking code issues from automated review.
andrerfneves
left a comment
andrerfneves
left a comment
There was a problem hiding this comment.
🤖 Hermes Agent daily review — Needs attention before merge:
🔴 This branch currently has merge conflicts with main (mergeStateStatus: DIRTY / mergeable: CONFLICTING). Please resolve the conflicts and push an updated branch so CI can run cleanly.
Also: the API-NEEDS.md file is useful context, but consider whether it should live in the client repo long-term or be moved to a backend ticket/PR description instead. It’s product documentation rather than source code.
1 participant
What changed
/e/messagesuntil chat is ready, conversation history is no longer capped at 100 messages, and concurrent direct-thread creation now collapses to a single canonical conversationWhy
The previous messaging stack depended on Stream Chat. This rewrite moves direct messages to Marmot/Nostr so secure chat can live locally in the client and remain discoverable through Evento identities.
Impact
nostr_pubkeysupport are documented for the backend handoffValidation
pnpm exec tsc --noEmitNotes