fix: wire Firestore client and memory store into main.go (#126)#129
Conversation
- Create Firestore client in main.go with configured database - Initialize FirestoreStore and MemoryStore, pass to WebSocket handler - Wire MemoryStore into ToolHandler so recall_memory tool works at runtime - Persist session state to Firestore on WebSocket disconnect - Update tests for new RegisterWebSocket/handleWebSocket signatures Closes #126 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
✨ 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 |
Summary of ChangesHello @ComBba, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request integrates Google Cloud Firestore for persistent storage of session data and memory, enhancing the application's state management capabilities. It introduces a Firestore client, initializes dedicated session and memory stores, and ensures these are properly wired into the WebSocket handling and tool management logic. A key aspect of this change is the implementation of a graceful fallback mechanism, allowing the application to continue functioning with in-memory stores if Firestore is not configured or accessible, thereby improving resilience. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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: bcfa9385a7
ℹ️ 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".
| toolHandler.SetGenerator(sceneGen) | ||
| toolHandler.SetGenaiClient(client) | ||
| if memStore != nil { | ||
| toolHandler.SetMemoryStore(memStore, "") |
There was a problem hiding this comment.
recall_memory will never return results from the injected store because this initialization hard-codes an empty persona ID. In handleRecallMemory (internal/live/tools.go), the handler returns an empty memory list whenever pid == "", so any memories saved under real persona keys (for example from analysis) are unreachable at runtime even though memStore is present.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request wires up Firestore for session and memory persistence, with a graceful fallback to in-memory storage. The changes correctly initialize the Firestore client and inject the new stores into the WebSocket handler. My review focuses on a few areas for improvement: a critical issue with the recall_memory tool's implementation, incomplete session state persistence, and a minor maintainability point regarding a magic number. Addressing these will ensure the new persistence features are robust and fully functional.
| toolHandler.SetGenerator(sceneGen) | ||
| toolHandler.SetGenaiClient(client) | ||
| if memStore != nil { | ||
| toolHandler.SetMemoryStore(memStore, "") |
There was a problem hiding this comment.
The personaID passed to toolHandler.SetMemoryStore is an empty string. This will cause the recall_memory tool to be non-functional because the check for an empty personaID in handleRecallMemory will always pass, returning no results.
The personaID should be set on the toolHandler once it's known (likely after persona selection). This may require a mechanism for the session.Manager to update the toolHandler with the correct personaID when SetPersona is called.
| sd := &store.SessionData{ | ||
| PersonaName: mgr.PersonaName(), | ||
| State: string(mgr.State()), | ||
| } |
There was a problem hiding this comment.
The session state being persisted to Firestore is incomplete. It's missing fields like MatchedVoice, ReunionCount, and LanguageCode which are part of the session state managed by session.Manager.
This could lead to inconsistent state if the session is restored later. Consider saving all relevant session data.
Note that languageCode is currently a private field in session.Manager and would need a getter to be accessed here.
sd := &store.SessionData{
PersonaName: mgr.PersonaName(),
State: string(mgr.State()),
MatchedVoice: mgr.MatchedVoice(),
ReunionCount: mgr.ReunionCount(),
}| } | ||
|
|
||
| sessionStore := store.NewFirestoreStore(cfg.ProjectID, firestoreClient) | ||
| memStore := memory.NewStore(100, firestoreClient) |
There was a problem hiding this comment.
The value 100 for maxPerKey in memory.NewStore is a magic number. To improve readability and maintainability, consider defining this as a constant (e.g., const maxMemoriesPerPersona = 100) at the top of the file or in a relevant package, and using that constant here.
- Extract magic number 100 to maxMemoriesPerPersona constant - Include MatchedVoice in persisted session data - Use mgr.SessionID() instead of r.RemoteAddr as session key Note: personaID is intentionally empty at connection time; it will be set by the onboarding pipeline when persona is created. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38c86257a9
ℹ️ 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".
| PersonaName: mgr.PersonaName(), | ||
| MatchedVoice: mgr.MatchedVoice(), | ||
| State: string(mgr.State()), |
There was a problem hiding this comment.
handleWebSocket now writes PersonaName, MatchedVoice, and State from mgr on disconnect, but this handler never updates mgr after session.NewManager(...) (no SetPersona or state transitions are invoked in this code path), so persisted records will be saved as empty persona/voice and default onboarding state even after a real reunion flow. This makes the new Firestore persistence effectively incorrect for every session.
Useful? React with 👍 / 👎.
| MatchedVoice: mgr.MatchedVoice(), | ||
| State: string(mgr.State()), | ||
| } | ||
| if err := sessionStore.SaveSession(saveCtx, mgr.SessionID(), sd); err != nil { |
There was a problem hiding this comment.
The session is persisted under mgr.SessionID(), and in this handler that ID is initialized from r.RemoteAddr; source address/port values change across reconnects and can be reused, so saved state is not reliably tied to one logical user session and can be overwritten in multi-client or reconnect-heavy environments.
Useful? React with 👍 / 👎.
1 participant
Summary
main.gowithNewClientWithDatabaseusing configured project/databaseFirestoreStoreandMemoryStore, inject into WebSocket handlerMemoryStoreintoToolHandlersorecall_memorytool works at runtimeIssue
Closes #126
Local CI
Test plan
nilstore parameters (in-memory fallback)🤖 Generated with Claude Code