Skip to content

fix(code): respect manually-renamed task titles#2144

Open
pauldambra wants to merge 3 commits into
mainfrom
posthog-code/respect-manual-task-rename
Open

fix(code): respect manually-renamed task titles#2144
pauldambra wants to merge 3 commits into
mainfrom
posthog-code/respect-manual-task-rename

Conversation

@pauldambra
Copy link
Copy Markdown
Member

@pauldambra pauldambra commented May 13, 2026

Problem

If a user manually renamed a task and then sent their first prompt, the auto-title generator would still overwrite their title with an LLM-generated one. The guard against this only ran on subsequent generations — first-generation runs were exempted, so the user's rename got clobbered immediately.

A secondary bug: when the guard did fire, it returned out of the whole run, also skipping the unrelated conversation-summary update.

Changes

  • Drop the isFirstGeneration exemption in useChatTitleGenerator so a manual rename is respected from prompt feat: generated api client for tasks #1 onward.
  • Move the title_manually_set guard into an if/else around just the title update, so the summary still gets written when the title is skipped.

How did you test this?

  • pnpm --filter code typecheck — clean.
  • pnpm --filter code test useChatTitleGenerator — 7 tests pass, including two new ones:
    • skips first generation when title_manually_set is true
    • still updates the conversation summary when the title is skipped due to a manual rename
  • All CI checks pass (build, unit-test, integration-test, typecheck, semgrep, CodeQL, Wiz).
  • The new unit tests exercise the same scenario as the manual repro (rename → first prompt → title held), so the manual repro is covered by automation rather than required by hand.

Publish to changelog?

no


Created with PostHog Code

The auto-title generator only skipped the LLM-driven rename on
subsequent generations, so the very first auto-title (after the 1st
prompt) would still overwrite a title the user had already set
manually. Drop the first-generation exemption and only skip the title
update — the conversation summary still updates either way.

Generated-By: PostHog Code
Task-Id: fafd9cbe-c2b3-47d1-879c-6a316008634a
@pauldambra pauldambra marked this pull request as ready for review May 13, 2026 23:10
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 13, 2026

Reviews (1): Last reviewed commit: "fix(code): respect manually-renamed task..." | Re-trigger Greptile

Copy link
Copy Markdown
Member Author

Note

🤖 Automated comment by QA Swarm — not written by a human. Posted by Claude on Paul's behalf.

Multi-perspective review: qa-team (security, frontend, performance, copy, two generalists), paul-reviewer, xp-reviewer

Verdict: ✅ APPROVE WITH NITS

Both bugs the PR claims to fix are correctly addressed. Two convergent concerns flagged HIGH didn't survive verification: (a) the "cache freshness" race is closed by the server contract typing title_manually_set as required in the response (apps/code/src/renderer/api/generated.ts:12780), and (b) the "TaskDetail drift" concern is moot — useTaskData reads through useTasks() (the list query the auto-title already updates), and taskKeys.detail has no consumers in the codebase today. Residual items below are follow-ups.

Key findings

🟡 MEDIUM

  • log.debug on the success path logs full LLM-derived title and summary (useChatTitleGenerator.ts:91-95, 105-109). Titles and summaries are derived from user prompts and enriched file content. If renderer debug logs are ever persisted or shipped, this leaks user content.
  • Hook orchestrates LLM + auth + mutation + cache write + session-store update + service call. Per CLAUDE.md ("Services Over Hooks for Business Logic") this belongs in a main-process service. Not new in this PR; the bug being fixed is exactly the kind of accident a service boundary prevents.

🟢 LOW / ⚪ NIT

  • Narrow residual race: if the user renames after line 77's getCachedTask read but before the auto-title's client.updateTask + setQueriesData resolve, the auto-title can still overlay the rename. Window is bounded by auth + network latency; optimistic-write rename will eventually re-win on the next refetch.
  • Test name "skips first generation when title_manually_set" (useChatTitleGenerator.test.ts:106) describes a state that no longer exists — the first-gen exemption was removed. "skips title update when title_manually_set" would read more accurately.
  • Nested if (title) { if (manualSet) {} else { … } } (useChatTitleGenerator.ts:76-98) could flatten via if (title && !getCachedTask(taskId)?.title_manually_set), reading more like the intent ("update title unless user renamed; always update summary").
  • lastGeneratedAtCount is mildly imprecise — promptCountAtLastGen lies less.
  • LLM call still runs when the title is locked. generateTitleAndSummary returns both fields and the summary is still consumed, so this is unavoidable without splitting the call. Worth a comment, not a fix.
  • Test coverage gaps: rename mid-LLM-call, rename-after-auto-title-fired, server rollback path.

Convergence (independently flagged by 2+ reviewers)

Finding Reviewers Verified verdict
Cache-freshness / TOCTOU around title_manually_set qa-team/frontend + qa-team/generalist-a + qa-team/generalist-b + qa-team/security + paul Downgraded — server response types the field as required (api/generated.ts:12780). Narrow residual race remains.
Auto-title doesn't invalidate taskKeys.detail qa-team/generalist-b (sole, escalated by analogy) Dismissed — no useQuery consumes taskKeys.detail; TaskDetail reads through useTaskDatauseTasks() (the list query). The useTasks.ts:138 invalidation is itself currently dead defensive code.
Test name #1 is now misleading qa-team/generalist-b + paul + xp Confirmed — NIT
Nested if pyramid could flatten qa-team/generalist-a + xp Confirmed — NIT
Sensitive user content in log.debug security (sole) Confirmed — MEDIUM

Reviewer summaries

Reviewer Assessment
🔍 qa-team Both fixed bugs are addressed. Security: titles + summaries flow into log.debug and contain user prompt content. Frontend: hook is doing too much per CLAUDE.md; effect-deps fine. Performance: no regression — selector and getCachedTask cadence unchanged. Copy: log line stays accurate; test names slightly off. Generalists converged on cache-freshness, which verification softens.
👤 paul Fix lands; doesn't bail before the LLM call when title is locked (but that's blocked by the summary still needing it). Test #1 name lies. PR title accurately describes what changed.
📐 xp Inverting the guard would flatten the pyramid. Test name #1 lies. lastGeneratedAtCount lies a little. Mock setup is verbose but tractable — no DRY violation to chase.

Automated by QA Swarm — not a human review

- Drop user-derived title/summary from success-path debug logs so the
  LLM output isn't leaked if renderer debug logs are persisted.
- Flatten the nested title/manual-rename conditional into a single
  if/else-if pair using a `titleLocked` local — reads as intent.
- Rename misleading test ("skips first generation…" → "skips title
  update…") to match the post-fix behaviour.

Generated-By: PostHog Code
Task-Id: fafd9cbe-c2b3-47d1-879c-6a316008634a
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 14, 2026

Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
apps/code/src/renderer/features/sessions/hooks/useChatTitleGenerator.test.ts:106-145
**Prefer parameterised tests for related scenarios**

The two new tests exercise the same `title_manually_set: true` branch with different `summary` values (empty vs non-empty) and different expected side-effects. Per the team's stated preference, these would be cleaner as a single `it.each` covering both cases, reducing duplicated setup and making it easy to add further variants.

Reviews (2): Last reviewed commit: "refactor(code): tighten auto-title guard..." | Re-trigger Greptile

Per Greptile review feedback, fold the two summary-variant tests into a
single it.each so the no-summary and with-summary cases share setup.

Generated-By: PostHog Code
Task-Id: fafd9cbe-c2b3-47d1-879c-6a316008634a
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 14, 2026

Reviews (3): Last reviewed commit: "test(code): parameterise title_manually_..." | Re-trigger Greptile

@pauldambra pauldambra requested a review from a team May 14, 2026 01:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant