-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Toggle history #369
base: main
Are you sure you want to change the base?
Toggle history #369
Conversation
@casistack is attempting to deploy a commit to the morphic Team on Vercel. A member of the Team first needs to authorize it. |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@casistack This is another great update! Thank you! 🥇 Please fix the compilation error that is occurring.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test with LocalStorage:
env STORAGE_PROVIDER=local
LocalStorage wasn't working. To make LocalStorageProvider work, unlike Redis, we need to add logic to save on the client side. ref: #369 (comment)
Since we don't need to go that far, how about making STORAGE_PROVIDER
options either redis
or none
? When set to none
, nothing will be saved to storage. This would satisfy the requirements of #347.
app/actions.tsx
Outdated
@@ -147,8 +157,16 @@ export const AI = createAI<AIState, UIState>({ | |||
onSetAIState: async ({ state, done }) => { | |||
'use server' | |||
|
|||
const redis = await getRedisClient() | |||
const chatHistoryEnabled = await redis.get( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested with Upstash, and even when disabled, it was always saved. The reason was that chatHistoryEnabled
was returning a boolean value. Please check this.
console.log({
value: chatHistoryEnabled,
type: typeof chatHistoryEnabled,
comparison: chatHistoryEnabled === 'false',
strictEquality: Object.is(chatHistoryEnabled, 'false')
})
{
value: false,
type: 'boolean',
comparison: false,
strictEquality: false
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm thats odd i tested with local redis and was working and upstash . ok will have a look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I performed additional testing: #369 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for fixing the STORAGE_PROVIDER
👍 I've made several comments.
@@ -6,6 +6,9 @@ import HistoryContainer from './history-container' | |||
import TopRightMenu from './ui/top-right-menu' | |||
|
|||
export const Header: React.FC = async () => { | |||
// Get storage provider setting from environment | |||
const storageProvider = process.env.STORAGE_PROVIDER || 'redis' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The header will only be controlled on mobile. If you return null in the history-container, you can control everything at once.
https://github.com/miurla/morphic/blob/main/components/history-container.tsx
const storageProvider = process.env.STORAGE_PROVIDER
const enabled = storageProvider !== 'none'
if (!enabled) {
return null
}
try { | ||
if (this.client instanceof Redis) { | ||
const value = await this.client.get(key) | ||
return value ? String(value) : null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I performed verification. In the case of Upstash, the value of user:${userId}:chatHistoryEnabled
was being returned as a boolean, even though it was stored as a string.
Therefore, when the value was false
, it was returning either 'true'
or null
. So, modifying it as follows achieved the expected result.
return value === null ? null : String(value)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yes . makes sense should have spotted that
const chatHistoryEnabled = await redis.get( | ||
'user:anonymous:chatHistoryEnabled' | ||
) | ||
const chatHistoryEnabled = await redis.get('user:anonymous:chatHistoryEnabled') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO: Since chatHistoryEnabled is also being checked in saveChat, checking it in either place should be sufficient.
2804b5e#diff-819d2d9016d052cd621d2190deaf60688a87f953233d709b40fdabcf99d83f47R175-R180
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea just put it as additional safety net but see what you mean
@@ -17,7 +17,9 @@ export async function getChats(userId?: string | null) { | |||
|
|||
try { | |||
const redis = await getRedis() | |||
const chatHistoryEnabled = await redis.get(`user:${userId}:chatHistoryEnabled`) | |||
const chatHistoryEnabled = await redis.get( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use getChatHistorySetting
const chatHistoryEnabled = await getChatHistorySetting(userId)
2804b5e#diff-819d2d9016d052cd621d2190deaf60688a87f953233d709b40fdabcf99d83f47R217
components/chat-panel.tsx
Outdated
@@ -149,7 +148,7 @@ export function ChatPanel({ messages, query }: ChatPanelProps) { | |||
placeholder="Ask a question..." | |||
spellCheck={false} | |||
value={input} | |||
className="resize-none w-full min-h-12 rounded-fill bg-muted border border-input pl-4 pr-10 pt-3 pb-1 text-sm ring-offset-background file:border-0 file:bg-transparent file:text-sm file:font-medium placeholder:text-muted-foreground focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 disabled:cursor-not-allowed disabled:opacity-50'" | |||
className="resize-none w-full min-h-12 rounded-fill bg-muted border border-input pl-4 pr-10 pt-3 pb-1 text-sm ring-offset-background file:border-0 file:bg-transparent file:text-sm file:font-medium placeholder:text-muted-foreground focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-offset-2 disabled:cursor-not-allowed disabled:opacity-50'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok so no cosmetic changes . cool
|
||
const preferredStorage = process.env.STORAGE_PROVIDER?.toLowerCase() || 'redis' | ||
|
||
if (preferredStorage === 'local') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I overlooked it. Let's remove the local storage-related code.
- lib/storage/local-storage.ts
Detailed Changes
Chat History Toggle Feature:
Component Updates:
HistoryList
component to use client-side state management for immediate UI updatesClearHistory
component to properly refresh the history list after clearingHistory
component to handle state transitions and loading statesRedis Integration:
Storage Implementation:
Environment Variables Added:
STORAGE_PROVIDER
: Choose between 'redis' or 'local' storageTesting Instructions
Chat History Toggle:
Storage Testing:
env STORAGE_PROVIDER=redis USE_LOCAL_REDIS=false UPSTASH_REDIS_REST_URL=[URL] UPSTASH_REDIS_REST_TOKEN=[TOKEN]
env STORAGE_PROVIDER=local
Potential Impacts
Additional Notes
Checklist Additions
Closes #[347] Add No-History Options