fix(codex): show provider thread titles in the sidebar - #1061
fix(codex): show provider thread titles in the sidebar#1061apple-ouyang wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughCodex synchronization now resolves titles from ChangesSession name provenance
Codex title synchronization
Sequence Diagram(s)sequenceDiagram
participant SessionsWatcher
participant SessionSynchronizerService
participant CodexSessionSynchronizer
participant session_index_jsonl
participant state_sqlite
participant sessionsDb
SessionsWatcher->>SessionSynchronizerService: start synchronization with initializing
SessionSynchronizerService->>CodexSessionSynchronizer: synchronize with options
CodexSessionSynchronizer->>session_index_jsonl: read latest valid thread names
CodexSessionSynchronizer->>state_sqlite: read newest thread titles
CodexSessionSynchronizer->>sessionsDb: update provider names
sessionsDb-->>CodexSessionSynchronizer: preserve manual names
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 2
🧹 Nitpick comments (2)
server/modules/database/repositories/sessions.db.ts (1)
214-230: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMerge can drop the duplicate's manual name.
The duplicate row is deleted, but its
custom_nameis only adopted when the surviving row's name isNULL. If the surviving row already holds a provider-derived name and the duplicate holds a'manual'one, the user's rename is silently lost. Prefer manual provenance from either side.♻️ Prefer the manual name from either row
custom_name = CASE WHEN custom_name_source = 'manual' THEN custom_name + WHEN ? = 'manual' THEN ? ELSE COALESCE(custom_name, ?) END, custom_name_source = CASE WHEN custom_name_source = 'manual' THEN custom_name_source + WHEN ? = 'manual' THEN 'manual' ELSE COALESCE(custom_name_source, ?) END,Bind
duplicate.custom_name_source/duplicate.custom_namefor the added placeholders.🤖 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 `@server/modules/database/repositories/sessions.db.ts` around lines 214 - 230, Update the custom_name and custom_name_source merge logic in the session upsert so a duplicate with manual provenance overrides a provider-derived name on the surviving row. Add the duplicate’s custom_name_source and custom_name as bindings for the new placeholders, while preserving the surviving name when neither row has manual provenance.server/modules/providers/list/codex/codex-session-synchronizer.provider.ts (1)
135-148: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPer-entry lookups scale with the whole Codex history.
nameMapholds every thread in the lateststate_*.sqliteplus everysession_index.jsonlentry, and each iteration issues up to twoSELECTs regardless of whether the session is known to CloudCLI. On long-lived Codex installs this is thousands of queries on every sync tick. Consider fetching the codex sessions once (session_id/provider_session_id/custom_name) and intersecting in memory.Also, the name reads as index-only but the map also carries state-DB titles —
updateProviderSessionNameswould be more accurate.🤖 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 `@server/modules/providers/list/codex/codex-session-synchronizer.provider.ts` around lines 135 - 148, Refactor updateIndexedSessionNames to avoid per-entry getSessionByProviderSessionId/getSessionById queries: fetch the relevant CloudCLI sessions once, index them by provider/session ID, and intersect nameMap in memory before updating changed names. Rename the method to updateProviderSessionNames to reflect that names come from both the session index and state database, and update all callers accordingly.
🤖 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 `@server/modules/database/migrations.ts`:
- Around line 71-77: Update the conflict-handling SQL in the session migration
so blank or whitespace-only excluded.custom_name values are treated as absent,
matching the INSERT branch. In the ON CONFLICT SET expressions, prevent such
values from replacing sessions.custom_name and keep the existing
custom_name_source unless the excluded name contains non-whitespace content.
In `@server/modules/providers/tests/codex-sessions.test.ts`:
- Around line 104-108: Update the seeded session_index.jsonl entry in the
titled-session test to use the codex-app-titled session id exercised by that
test, while preserving the blank thread_name needed for the transcript fallback
scenario.
---
Nitpick comments:
In `@server/modules/database/repositories/sessions.db.ts`:
- Around line 214-230: Update the custom_name and custom_name_source merge logic
in the session upsert so a duplicate with manual provenance overrides a
provider-derived name on the surviving row. Add the duplicate’s
custom_name_source and custom_name as bindings for the new placeholders, while
preserving the surviving name when neither row has manual provenance.
In `@server/modules/providers/list/codex/codex-session-synchronizer.provider.ts`:
- Around line 135-148: Refactor updateIndexedSessionNames to avoid per-entry
getSessionByProviderSessionId/getSessionById queries: fetch the relevant
CloudCLI sessions once, index them by provider/session ID, and intersect nameMap
in memory before updating changed names. Rename the method to
updateProviderSessionNames to reflect that names come from both the session
index and state database, and update all callers accordingly.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2874a024-3f20-4598-aff9-6f6b31151208
📒 Files selected for processing (7)
server/modules/database/migrations.tsserver/modules/database/repositories/sessions.db.tsserver/modules/database/schema.tsserver/modules/database/tests/sessions.db.integration.test.tsserver/modules/providers/README.mdserver/modules/providers/list/codex/codex-session-synchronizer.provider.tsserver/modules/providers/tests/codex-sessions.test.ts
2ad3f74 to
1419d64
Compare
|
Addressed the remaining review findings in 1419d64: duplicate manual names now win during provider-ID merge, and Codex title backfill uses one provider-scoped query while retaining archived sessions. The README out-of-scope warning is a false positive: the Claude plugin/MCP guidance was pre-existing; this PR only changes the Codex row and trailing blank lines. Fresh verification: 123 server tests, typecheck, targeted ESLint, server build, and independent review all pass. @coderabbitai review |
|
Your plan includes PR reviews subject to rate limits. Reviews are available now. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@server/modules/providers/list/codex/codex-session-synchronizer.provider.ts`:
- Around line 101-133: Update readIndexedNameMap to retain the createReadStream
instance used by readline.createInterface and attach an explicit error handler
to that input stream, ensuring ENOENT and other read failures are caught and
handled by the existing optional-index fallback without unhandled stream errors.
Keep valid-line parsing and nameMap behavior unchanged.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8bacd410-c28f-4526-829a-cb8c657280f4
📒 Files selected for processing (8)
server/modules/database/migrations.tsserver/modules/database/repositories/sessions.db.tsserver/modules/database/schema.tsserver/modules/database/tests/sessions-provider-mapping.test.tsserver/modules/database/tests/sessions.db.integration.test.tsserver/modules/providers/README.mdserver/modules/providers/list/codex/codex-session-synchronizer.provider.tsserver/modules/providers/tests/codex-sessions.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- server/modules/database/schema.ts
- server/modules/database/tests/sessions.db.integration.test.ts
- server/modules/providers/tests/codex-sessions.test.ts
- server/modules/database/migrations.ts
- server/modules/providers/README.md
- server/modules/database/repositories/sessions.db.ts
1419d64 to
d4bbaf5
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/modules/providers/list/codex/codex-session-synchronizer.provider.ts (1)
56-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract
'Untitled Codex Session'into a shared constant.The sentinel/fallback string is duplicated across the "still untitled" checks (line 56), the manual/indexed/state/transcript fallback calls (lines 253, 261, 266, 290), and the map-based backfill (line 185). A future edit to one occurrence without updating the others would silently break the "session is still untitled" detection.
♻️ Suggested constant extraction
+const UNTITLED_CODEX_SESSION_NAME = 'Untitled Codex Session'; + export class CodexSessionSynchronizer implements IProviderSessionSynchronizer {Then replace each literal occurrence with
UNTITLED_CODEX_SESSION_NAME.Also applies to: 165-190, 253-253, 261-261, 266-266, 290-290
🤖 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 `@server/modules/providers/list/codex/codex-session-synchronizer.provider.ts` around lines 56 - 58, Extract the repeated “Untitled Codex Session” sentinel into a shared constant named UNTITLED_CODEX_SESSION_NAME in the codex session synchronizer, then replace every literal occurrence across the still-untitled checks, map-based backfill, and manual/indexed/state/transcript fallback calls with that constant.
🤖 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.
Nitpick comments:
In `@server/modules/providers/list/codex/codex-session-synchronizer.provider.ts`:
- Around line 56-58: Extract the repeated “Untitled Codex Session” sentinel into
a shared constant named UNTITLED_CODEX_SESSION_NAME in the codex session
synchronizer, then replace every literal occurrence across the still-untitled
checks, map-based backfill, and manual/indexed/state/transcript fallback calls
with that constant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c8805375-c7ee-4e11-9d34-172d22ed974a
📒 Files selected for processing (8)
server/modules/database/migrations.tsserver/modules/database/repositories/sessions.db.tsserver/modules/database/schema.tsserver/modules/database/tests/sessions-provider-mapping.test.tsserver/modules/database/tests/sessions.db.integration.test.tsserver/modules/providers/README.mdserver/modules/providers/list/codex/codex-session-synchronizer.provider.tsserver/modules/providers/tests/codex-sessions.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- server/modules/database/schema.ts
- server/modules/database/tests/sessions-provider-mapping.test.ts
- server/modules/providers/tests/codex-sessions.test.ts
- server/modules/database/tests/sessions.db.integration.test.ts
- server/modules/database/migrations.ts
- server/modules/providers/README.md
- server/modules/database/repositories/sessions.db.ts
|
Hey @apple-ouyang, thanks for the PR. There has been a recent big update and there are merge conflicts as a result. Can you fix it so that I can review it easily? |
Reason: - Codex titles can arrive after transcript indexing. - Provider refreshes must not overwrite CloudCLI manual names. Changes: - Read state and append-only index titles with metadata-only backfill. - Track manual versus provider names and preserve legacy custom names. - Cover title precedence, incremental updates, and legacy upgrades.
d4bbaf5 to
8052bf0
Compare
|
Fixed the merge conflicts and updated the branch against the latest |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@server/modules/providers/list/codex/codex-session-synchronizer.provider.ts`:
- Around line 190-215: Update updateProviderSessionNames to skip entries whose
name is empty or whitespace-only before calling normalizeSessionName or updating
the database. Mirror the existing indexedSessionName?.trim() guard used by
processSessionFile, while preserving processing for non-blank names.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7076b283-39ea-4d6f-a584-4c2b201c20cf
📒 Files selected for processing (12)
server/modules/database/migrations.tsserver/modules/database/repositories/sessions.db.tsserver/modules/database/schema.tsserver/modules/database/tests/sessions-provider-mapping.test.tsserver/modules/database/tests/sessions.db.integration.test.tsserver/modules/providers/README.mdserver/modules/providers/list/codex/codex-session-synchronizer.provider.tsserver/modules/providers/services/session-synchronizer.service.tsserver/modules/providers/services/sessions-watcher.service.tsserver/modules/providers/tests/codex-sessions.test.tsserver/modules/providers/tests/provider-token-usage.service.test.tsserver/shared/interfaces.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- server/modules/database/schema.ts
- server/modules/database/tests/sessions-provider-mapping.test.ts
- server/modules/database/migrations.ts
- server/modules/database/repositories/sessions.db.ts
- server/modules/database/tests/sessions.db.integration.test.ts
- server/modules/providers/README.md
| private updateProviderSessionNames(nameMap: Map<string, string>): void { | ||
| const sessions = sessionsDb.getSessionsByProvider(this.provider); | ||
| const sessionsByLookupId = new Map<string, (typeof sessions)[number]>(); | ||
| for (const session of sessions) { | ||
| if (session.provider_session_id) { | ||
| sessionsByLookupId.set(session.provider_session_id, session); | ||
| } | ||
| } | ||
| for (const session of sessions) { | ||
| if (!sessionsByLookupId.has(session.session_id)) { | ||
| sessionsByLookupId.set(session.session_id, session); | ||
| } | ||
| } | ||
|
|
||
| for (const [providerSessionId, name] of nameMap) { | ||
| const existingSession = sessionsByLookupId.get(providerSessionId); | ||
| if (!existingSession) { | ||
| continue; | ||
| } | ||
|
|
||
| const normalizedName = normalizeSessionName(name, 'Untitled Codex Session'); | ||
| if (normalizedName !== existingSession.custom_name) { | ||
| sessionsDb.updateSessionProviderName(existingSession.session_id, normalizedName); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Blank index/state names can downgrade an already-titled session to "Untitled Codex Session".
updateProviderSessionNames does not skip blank name values before calling normalizeSessionName(name, 'Untitled Codex Session'). If nameMap contains an entry with an empty or whitespace-only name for a session that already has a real title (for example, a blank thread_name written to the append-only session_index.jsonl after a real one for the same id), normalizedName becomes 'Untitled Codex Session' and Line 212 overwrites the session's existing title on the next non-initializing full sync.
This is inconsistent with processSessionFile, which explicitly guards against blank indexed names with indexedSessionName?.trim() at Line 283. Add the same guard here so a blank name never wins over an existing title.
🐛 Proposed fix
for (const [providerSessionId, name] of nameMap) {
const existingSession = sessionsByLookupId.get(providerSessionId);
- if (!existingSession) {
+ if (!existingSession || !name.trim()) {
continue;
}📝 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.
| private updateProviderSessionNames(nameMap: Map<string, string>): void { | |
| const sessions = sessionsDb.getSessionsByProvider(this.provider); | |
| const sessionsByLookupId = new Map<string, (typeof sessions)[number]>(); | |
| for (const session of sessions) { | |
| if (session.provider_session_id) { | |
| sessionsByLookupId.set(session.provider_session_id, session); | |
| } | |
| } | |
| for (const session of sessions) { | |
| if (!sessionsByLookupId.has(session.session_id)) { | |
| sessionsByLookupId.set(session.session_id, session); | |
| } | |
| } | |
| for (const [providerSessionId, name] of nameMap) { | |
| const existingSession = sessionsByLookupId.get(providerSessionId); | |
| if (!existingSession) { | |
| continue; | |
| } | |
| const normalizedName = normalizeSessionName(name, 'Untitled Codex Session'); | |
| if (normalizedName !== existingSession.custom_name) { | |
| sessionsDb.updateSessionProviderName(existingSession.session_id, normalizedName); | |
| } | |
| } | |
| } | |
| private updateProviderSessionNames(nameMap: Map<string, string>): void { | |
| const sessions = sessionsDb.getSessionsByProvider(this.provider); | |
| const sessionsByLookupId = new Map<string, (typeof sessions)[number]>(); | |
| for (const session of sessions) { | |
| if (session.provider_session_id) { | |
| sessionsByLookupId.set(session.provider_session_id, session); | |
| } | |
| } | |
| for (const session of sessions) { | |
| if (!sessionsByLookupId.has(session.session_id)) { | |
| sessionsByLookupId.set(session.session_id, session); | |
| } | |
| } | |
| for (const [providerSessionId, name] of nameMap) { | |
| const existingSession = sessionsByLookupId.get(providerSessionId); | |
| if (!existingSession || !name.trim()) { | |
| continue; | |
| } | |
| const normalizedName = normalizeSessionName(name, 'Untitled Codex Session'); | |
| if (normalizedName !== existingSession.custom_name) { | |
| sessionsDb.updateSessionProviderName(existingSession.session_id, normalizedName); | |
| } | |
| } | |
| } |
🤖 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 `@server/modules/providers/list/codex/codex-session-synchronizer.provider.ts`
around lines 190 - 215, Update updateProviderSessionNames to skip entries whose
name is empty or whitespace-only before calling normalizeSessionName or updating
the database. Mirror the existing indexedSessionName?.trim() guard used by
processSessionFile, while preserving processing for non-blank names.
2 participants
Summary
~/.codex/state_*.sqlitesession_index.jsonlas append-only and prefer its latest manual nameDisplayed title precedence is:
session_index.jsonlnameMigration safety
Existing non-empty session names are conservatively marked as manual during upgrade because older releases did not record provenance. This prevents an upgrade from overwriting a name the user entered in CloudCLI. Newly synchronized names are marked as provider-derived and can be refreshed by later Codex metadata.
Testing
sessionsandsession_nameslayoutsFixes #1060
Summary by CodeRabbit