Revert PR #732#733
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThe PR removes message ChangesChat persistence and rehydration
Calendar data access
Service-worker assets
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
|
ngoiyaeric seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
There was a problem hiding this comment.
Verified as a clean revert of PR #732.
- The current head
46a59c0cae4c4518da837729a0961583eb9e7b7bis a single Git revert of merge commit097871f1b9380ef053915cc338e477c8779fd173. - Its tree
db2b8e48e3cb6f74a18ec7e70e5858d1031aa7dcexactly matches the pre-#732 tree ate773257319bcd5a63d5709def6a156befb4c73e1; the 11-file, +31/-1206 diff is the exact inverse of #732. - I found no blocking correctness or regression issues in the changed code or PR discussion.
Deployment note (non-blocking): removing migration 0009_add_message_type_name from the journal does not physically undo it on databases that already applied PR #732; those databases will retain the nullable messages.type/messages.name columns and migration record. That is harmless to this reverted code, but an explicit down migration would be needed if physical schema rollback is required.
Existing checks at review time: GitGuardian Security Checks and Vercel Preview passed; CodeRabbit and Vercel – qcx were pending. No tests or CI commands were run during this review.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/actions.tsx (1)
713-713: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winRemoved
default: return nullfallthroughs allowundefinedentries inUIState, crashingChatMessages.The
default: return nullbranches were removed from both the user-message type switch (line 713) and the assistant-message type switch (lines 772–773). Any message with an unhandledtype(e.g.,role: 'user'withtype: 'skip', orrole: 'assistant'withtype: 'tool') now falls through the switch without returning, causing the.map()callback to produceundefined.The filter at line 871 was simultaneously changed from
!== nullto only filternull, soundefinedentries survive into the returnedUIState. The downstream consumerChatMessages(components/chat-messages.tsx, lines 14–28) callsmessages.reduceand directly accessesmessage.idandmessage.componenton each element — encounteringundefinedwill throw aTypeErrorat runtime.🔒 Proposed fix: restore null fallbacks and filter both null and undefined
switch (type) { case 'input': case 'input_related': // ... existing cases ... case 'inquiry': return { id, component: <CopilotDisplay content={content as string} /> } + default: + return null } break case 'assistant': // ... existing cases ... case 'resolution_search_result': { // ... existing case ... } + default: + return null } breakAnd restore the filter to exclude both
nullandundefined:- .filter(message => message !== null) as UIState + .filter(message => message !== null && message !== undefined) as UIStateAlso applies to: 772-773, 871-871
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/actions.tsx` at line 713, Restore the default null-return branches in both the user-message and assistant-message type switches within the UIState mapping so unhandled message types produce null rather than undefined. Update the resulting filter to exclude both null and undefined entries before returning UIState, preserving only valid message components for ChatMessages.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/actions.tsx`:
- Line 664: Prevent the synthetic `{ content: 'end', type: 'end' }` message from
being included in the `updatedMessages` payload passed to `saveChat` in the save
flow around `app/actions.tsx`. Filter or remove this terminal marker before
persistence while preserving all genuine chat messages and the existing UI
completion behavior.
In `@app/search/`[id]/page.tsx:
- Around line 51-58: Update getUIStateFromAIState’s message hydration mapping to
restore a valid type for every persisted message instead of leaving type
undefined. Reconstruct the type from each dbMsg’s available role/content or
persisted metadata, preserving the existing role, content, and createdAt
mappings so reopening a chat renders its transcript.
---
Outside diff comments:
In `@app/actions.tsx`:
- Line 713: Restore the default null-return branches in both the user-message
and assistant-message type switches within the UIState mapping so unhandled
message types produce null rather than undefined. Update the resulting filter to
exclude both null and undefined entries before returning UIState, preserving
only valid message components for ChatMessages.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 804fcb37-c859-469d-9f88-c830c744df8e
📒 Files selected for processing (11)
app/actions.tsxapp/search/[id]/page.tsxdrizzle/migrations/0009_add_message_type_name.sqldrizzle/migrations/meta/0006_snapshot.jsondrizzle/migrations/meta/_journal.jsonlib/actions/calendar.tslib/actions/chat-db.tslib/actions/chat.tslib/db/schema.tspublic/sw.jstests/targeted.test.ts
💤 Files with no reviewable changes (5)
- tests/targeted.test.ts
- drizzle/migrations/meta/0006_snapshot.json
- drizzle/migrations/0009_add_message_type_name.sql
- drizzle/migrations/meta/_journal.json
- lib/db/schema.ts
📜 Review details
🧰 Additional context used
🪛 ast-grep (0.44.1)
public/sw.js
[warning] Avoid using the initial state variable in setState
Context: setTimeout(t,e)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
🔇 Additional comments (5)
public/sw.js (1)
2-2: LGTM!lib/actions/chat.ts (1)
140-143: LGTM!Also applies to: 170-170, 213-219
lib/actions/chat-db.ts (1)
147-147: LGTM!Also applies to: 269-269
app/search/[id]/page.tsx (1)
21-21: LGTM!lib/actions/calendar.ts (1)
3-3: LGTM!Also applies to: 30-35, 54-54, 81-81, 107-107
| path, | ||
| title, | ||
| messages: updatedMessages.filter(msg => !(msg.content === 'end' && msg.type === 'end')) | ||
| messages: updatedMessages |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
end messages are now persisted to the DB and accumulate across saves.
updatedMessages includes the appended { content: 'end', type: 'end' } message (lines 641–649). Since the mapping in saveChat (lib/actions/chat.ts, lines 213–219) drops type, this message is stored as role: 'assistant', content: 'end'. Each subsequent save appends a new end message with a fresh nanoid(), so multiple end rows accumulate per chat over time. While these are filtered from the UI on rehydration (no type → null), they waste storage and could confuse message-count or ordering logic.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/actions.tsx` at line 664, Prevent the synthetic `{ content: 'end', type:
'end' }` message from being included in the `updatedMessages` payload passed to
`saveChat` in the save flow around `app/actions.tsx`. Filter or remove this
terminal marker before persistence while preserving all genuine chat messages
and the existing UI completion behavior.
| role: dbMsg.role as AIMessage['role'], // Cast role, ensure AIMessage['role'] includes all dbMsg.role possibilities | ||
| content: dbMsg.content, | ||
| createdAt: dbMsg.createdAt ? new Date(dbMsg.createdAt) : undefined, | ||
| type: dbMsg.type ? (dbMsg.type as AIMessage['type']) : undefined, | ||
| name: dbMsg.name || undefined, | ||
| // 'type' and 'name' are not in the basic Drizzle 'messages' schema. | ||
| // These would be undefined unless specific logic is added to derive them. | ||
| // For instance, if a message with role 'tool' should have a 'name', | ||
| // or if some messages have a specific 'type' based on content or other flags. | ||
| // This mapping assumes standard user/assistant messages primarily. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether the Chat component or any other mechanism reconstructs UI state
# from AI state on page load, which could bypass getUIStateFromAIState.
# Check how the Chat component consumes AI/UI state
ast-grep outline components/chat.tsx --items all --type function,component
# Search for getUIState or onGetUIState usage to understand when it's called
rg -nC3 'getUIState|onGetUIState' --type=ts --glob '!**/node_modules/**'
# Check if initialUIState is set to anything other than empty array
rg -nC3 'initialUIState' app/actions.tsxRepository: QueueLab/QCX
Length of output: 197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files ==\n'
git ls-files 'app/search/[id]/page.tsx' 'app/actions.tsx' 'components/chat.tsx'
printf '\n== Outline: app/search/[id]/page.tsx ==\n'
ast-grep outline 'app/search/[id]/page.tsx' || true
printf '\n== Outline: app/actions.tsx ==\n'
ast-grep outline 'app/actions.tsx' || true
printf '\n== Outline: components/chat.tsx ==\n'
ast-grep outline 'components/chat.tsx' || true
printf '\n== Search for getUIState / onGetUIState / initialUIState ==\n'
rg -nC3 'getUIState|onGetUIState|initialUIState' --glob '!**/node_modules/**' app components || true
printf '\n== Relevant slices: app/actions.tsx ==\n'
sed -n '640,720p' app/actions.tsx || true
printf '\n== Relevant slices: components/chat.tsx ==\n'
sed -n '1,260p' components/chat.tsx || true
printf '\n== Relevant slice: app/search/[id]/page.tsx ==\n'
sed -n '1,180p' 'app/search/[id]/page.tsx' || trueRepository: QueueLab/QCX
Length of output: 17459
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the specific message/UI types to understand whether `type` is required
# and how the page reconstructs state.
rg -nC3 'type:|UIMessage|AIMessage|Message' app components --glob '!**/node_modules/**' || true
# Look for any conversion from DB messages to UI messages besides app/search/[id]/page.tsx
rg -nC3 'messages.map|createdAt:|role: dbMsg.role|content: dbMsg.content' app components --glob '!**/node_modules/**' || trueRepository: QueueLab/QCX
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Match files ==\n'
git ls-files | rg '^(app/actions\.tsx|app/search/\[id\]/page\.tsx|components/chat\.tsx)$'
printf '\n== app/actions.tsx around getUIStateFromAIState ==\n'
python3 - <<'PY'
from pathlib import Path
p = Path('app/actions.tsx')
text = p.read_text()
for needle in ['getUIStateFromAIState', 'initialUIState', 'onGetUIState', 'type ===', '!type']:
idx = text.find(needle)
print(f'needle={needle!r} idx={idx}')
if idx != -1:
start = max(0, text.rfind('\n', 0, idx-200))
end = text.find('\n', idx+400)
print(text[start:end])
print('---')
PY
printf '\n== components/chat.tsx around state wiring ==\n'
python3 - <<'PY'
from pathlib import Path
p = Path('components/chat.tsx')
text = p.read_text()
for needle in ['initialUIState', 'onGetUIState', 'getUIState', 'useUIState', 'useChat']:
idx = text.find(needle)
print(f'needle={needle!r} idx={idx}')
if idx != -1:
start = max(0, text.rfind('\n', 0, idx-250))
end = text.find('\n', idx+500)
print(text[start:end])
print('---')
PY
printf '\n== app/search/[id]/page.tsx around DB message mapping ==\n'
python3 - <<'PY'
from pathlib import Path
p = Path('app/search/[id]/page.tsx')
text = p.read_text()
for needle in ['createdAt: dbMsg.createdAt', 'role: dbMsg.role', 'type:', 'messages.map', 'getUIStateFromAIState']:
idx = text.find(needle)
print(f'needle={needle!r} idx={idx}')
if idx != -1:
start = max(0, text.rfind('\n', 0, idx-250))
end = text.find('\n', idx+500)
print(text[start:end])
print('---')
PYRepository: QueueLab/QCX
Length of output: 6472
Restore type when hydrating messages (app/search/[id]/page.tsx:51-58) getUIStateFromAIState drops messages with no type, so reopening an existing chat renders an empty transcript unless the message type is reconstructed or persisted.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/search/`[id]/page.tsx around lines 51 - 58, Update
getUIStateFromAIState’s message hydration mapping to restore a valid type for
every persisted message instead of leaving type undefined. Reconstruct the type
from each dbMsg’s available role/content or persisted metadata, preserving the
existing role, content, and createdAt mappings so reopening a chat renders its
transcript.
Reverting the last pull request #732 as requested.
Summary by CodeRabbit
Bug Fixes
Performance
Chat Updates