Conversation
github-actions
bot
added
area/fullstack
Summary of ChangesHello @AndyMik90, 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 enhances the display of subtask titles in the task modal by introducing a dedicated 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
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an optional Changes
sequenceDiagram
participant PlanFile as Plan JSON
participant Backend as Backend (schema + Subtask dataclass)
participant Frontend as Frontend (loader/store)
participant UI as Renderer UI
PlanFile->>Backend: upload/serve plan (may include subtask.title)
Backend->>Backend: validate schema (optional "title")
Frontend->>Backend: fetch plan
Frontend->>Frontend: for each subtask use `title` || extractSubtaskTitle(description)
Frontend->>UI: provide subtasks (title, description)
UI->>UI: render title (line-clamp) and conditional description tooltip
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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 effectively addresses the issue of long subtask titles by introducing a new title field and a fallback utility, extractSubtaskTitle, to generate concise titles from descriptions. The changes are implemented consistently across the backend, frontend, and AI prompts, ensuring backward compatibility. The addition of comprehensive unit tests for the new utility is a great practice. I've included a few suggestions to enhance the title extraction logic and improve the examples in the planner prompt to provide clearer guidance for the AI.
| "title": "Create aggregation Celery task", | ||
| "description": "Create aggregation Celery task", |
There was a problem hiding this comment.
The title and description are identical. According to the new guideline on line 370, the description should contain "full implementation details". To better guide the AI, consider making the description more detailed.
For example: "description": "Create a Celery task that aggregates analytics events from the last hour into a summary table."
| "title": "Identify root cause of memory leak", | ||
| "description": "Identify root cause of memory leak", |
There was a problem hiding this comment.
The title and description are identical. To align with the guideline on line 370, the description should be more detailed than the title.
For example: "description": "Use memory profiling tools to trace the source of the memory leak reported in issue #123. The investigation should focus on the data processing pipeline."
| "title": "Add new auth system alongside old", | ||
| "description": "Add new auth system alongside old", |
There was a problem hiding this comment.
The title and description are identical. The new guideline on line 370 states that the description should provide full implementation details. A more detailed description here would be a better example for the AI.
For instance: "description": "Implement the new JWT-based authentication system in 'src/auth/new_auth.ts' and integrate it into the main auth flow in 'src/auth/index.ts' behind a feature flag."
| const sentenceMatch = trimmed.match(/^(.+?(?:\. |: ))/); | ||
| if (sentenceMatch) { | ||
| let sentence = sentenceMatch[1].trimEnd(); | ||
| // Strip trailing period or colon+space artifacts | ||
| sentence = sentence.replace(/[.:]\s*$/, ''); | ||
| if (sentence.length <= maxLength) { |
There was a problem hiding this comment.
The current regex for sentence extraction only handles sentences ending with a period or colon followed by a space. This can be made more robust to handle other common sentence-ending punctuation (like ! and ?) and to correctly identify sentences at the end of the string that aren't followed by a space.
| const sentenceMatch = trimmed.match(/^(.+?(?:\. |: ))/); | |
| if (sentenceMatch) { | |
| let sentence = sentenceMatch[1].trimEnd(); | |
| // Strip trailing period or colon+space artifacts | |
| sentence = sentence.replace(/[.:]\s*$/, ''); | |
| if (sentence.length <= maxLength) { | |
| const sentenceMatch = trimmed.match(/^(.+?[.?!:])(?=\s+|$)/); | |
| if (sentenceMatch) { | |
| let sentence = sentenceMatch[1]; | |
| // Strip trailing punctuation | |
| sentence = sentence.replace(/[.?!:]$/, ''); | |
| if (sentence.length <= maxLength) { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@apps/backend/implementation_plan/subtask.py`:
- Around line 57-58: The current truthy check in the serialization (in the
Subtask class's to_dict/serialize method) uses "if self.title:" which drops
empty-string titles; change it to "if self.title is not None:" to match the
pattern used for other optional fields like session_id and ensure empty-string
titles are preserved in the resulting dict; update the conditional guarding
result["title"] in the method where self.title is handled.
In `@apps/backend/prompts/planner.md`:
- Around line 276-278: The example subtask "subtask-2-1" currently has identical
"title" and "description", which teaches the wrong pattern; update the example
entries (e.g., subtask-2-1) so the "title" remains a short action phrase and the
"description" provides full implementation details, acceptance criteria, and any
constraints or inputs/outputs expected (distinct from the title) to follow the
guideline that descriptions contain full implementation details.
In `@apps/frontend/src/shared/utils/subtask-title.ts`:
- Around line 22-31: The sentence extraction regex requires a space after the
period or colon so single-sentence descriptions ending with '.' or ':' won't
match; update the regex used to set sentenceMatch to allow a period or colon
followed by either whitespace or end-of-string (e.g. change /^(.+?(?:\. |: ))/
to a pattern that accepts [.:] followed by \s or $) so trimmed sentences like
"Fix the login button styling." are captured; keep the subsequent trimming/strip
logic (variables: trimmed, sentenceMatch, sentence, maxLength) and ensure you
still remove trailing punctuation with the existing replace call.
- Around line 12-13: The function extractSubtaskTitle currently returns the
hardcoded string 'Untitled' which prevents the downstream i18n fallback in
TaskSubtasks.tsx (t('tasks:subtasks.untitled')) from running; modify
extractSubtaskTitle to return an empty string (or a sentinel like
null/undefined) when description is empty OR add an optional fallback parameter
so callers can supply the translation key/value, updating the function signature
(extractSubtaskTitle) and its usages accordingly so no user-facing strings are
hardcoded in the utility.
| if self.title: | ||
| result["title"] = self.title |
There was a problem hiding this comment.
Falsy check drops empty-string titles — use is not None for consistency.
if self.title: skips serialization when title is "". Other optional fields in this method (e.g., session_id on Line 79) correctly use is not None. Use the same pattern here to stay consistent and avoid silently losing data.
Proposed fix
- if self.title:
+ if self.title is not None:
result["title"] = self.title📝 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.
| if self.title: | |
| result["title"] = self.title | |
| if self.title is not None: | |
| result["title"] = self.title |
🤖 Prompt for AI Agents
In `@apps/backend/implementation_plan/subtask.py` around lines 57 - 58, The
current truthy check in the serialization (in the Subtask class's
to_dict/serialize method) uses "if self.title:" which drops empty-string titles;
change it to "if self.title is not None:" to match the pattern used for other
optional fields like session_id and ensure empty-string titles are preserved in
the resulting dict; update the conditional guarding result["title"] in the
method where self.title is handled.
| "id": "subtask-2-1", | ||
| "title": "Create aggregation Celery task", | ||
| "description": "Create aggregation Celery task", |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Example subtask has identical title and description — demonstrates the wrong pattern.
Several examples (e.g., subtask-2-1) have title and description set to the same string. Since the guideline on Line 370 says "Description contains full implementation details," the examples should show descriptions that are more detailed than the title. Otherwise, the LLM will learn to duplicate them.
🤖 Prompt for AI Agents
In `@apps/backend/prompts/planner.md` around lines 276 - 278, The example subtask
"subtask-2-1" currently has identical "title" and "description", which teaches
the wrong pattern; update the example entries (e.g., subtask-2-1) so the "title"
remains a short action phrase and the "description" provides full implementation
details, acceptance criteria, and any constraints or inputs/outputs expected
(distinct from the title) to follow the guideline that descriptions contain full
implementation details.
| // Try to extract first sentence via '. ' or ': ' | ||
| const sentenceMatch = trimmed.match(/^(.+?(?:\. |: ))/); | ||
| if (sentenceMatch) { | ||
| let sentence = sentenceMatch[1].trimEnd(); | ||
| // Strip trailing period or colon+space artifacts | ||
| sentence = sentence.replace(/[.:]\s*$/, ''); | ||
| if (sentence.length <= maxLength) { | ||
| return sentence; | ||
| } | ||
| } |
There was a problem hiding this comment.
Sentence extraction won't match a description that is a single sentence ending with a period (no trailing text).
The regex /^(.+?(?:\. |: ))/ requires a space after the period. A description like "Fix the login button styling." (>80 chars, ending with . and no trailing space) won't match, falling through to word-boundary truncation. This may be intentional but is worth noting since many AI-generated descriptions end with a period.
🤖 Prompt for AI Agents
In `@apps/frontend/src/shared/utils/subtask-title.ts` around lines 22 - 31, The
sentence extraction regex requires a space after the period or colon so
single-sentence descriptions ending with '.' or ':' won't match; update the
regex used to set sentenceMatch to allow a period or colon followed by either
whitespace or end-of-string (e.g. change /^(.+?(?:\. |: ))/ to a pattern that
accepts [.:] followed by \s or $) so trimmed sentences like "Fix the login
button styling." are captured; keep the subsequent trimming/strip logic
(variables: trimmed, sentenceMatch, sentence, maxLength) and ensure you still
remove trailing punctuation with the existing replace call.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🟠 NEEDS REVISION
🟠 Needs revision - 2 issue(s) require attention.
Branch is out of date with base branch. Update branch first - if no conflicts arise, you can merge. If merge conflicts arise, resolve them and run follow-up review again. 2 issue(s) must be addressed (0 required, 2 recommended).
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | Low | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
🚨 Blocking Issues (Must Fix)
- Branch Out of Date: PR branch is behind the base branch and needs to be updated
Findings Summary
- Medium: 2 issue(s)
Generated by Auto Claude PR Review
Findings (2 selected of 2 total)
🟡 [086d238cc4a4] [MEDIUM] Hardcoded 'Untitled' string bypasses i18n fallback in UI component
📁 apps/frontend/src/shared/utils/subtask-title.ts:13
The extractSubtaskTitle() utility returns the hardcoded English string 'Untitled' for empty/null/undefined inputs. This is consumed by project-store.ts:506 which sets title: subtask.title || extractSubtaskTitle(subtask.description). When subtask.description is undefined (e.g., malformed plan JSON), the title becomes the truthy string 'Untitled'.
The UI component TaskSubtasks.tsx:72 already has a proper i18n fallback: {subtask.title || t('tasks:subtasks.untitled')} which maps to 'Untitled subtask' (en) / 'Sous-tâche sans titre' (fr). But since 'Untitled' is truthy, the component's i18n fallback is never reached — French users would see the raw English 'Untitled' instead of 'Sous-tâche sans titre'.
The project's CLAUDE.md mandates: 'All frontend user-facing text MUST use react-i18next translation keys. Never hardcode strings.'
Suggested fix:
Return an empty string `''` instead of `'Untitled'` from `extractSubtaskTitle()` for falsy inputs. This way the component's existing i18n fallback `subtask.title || t('tasks:subtasks.untitled')` will activate correctly since `''` is falsy. Update the tests to expect `''` instead of `'Untitled'`.
🟡 [f6e963a1ded3] [MEDIUM] Test file placed in wrong directory, breaking established pattern
📁 apps/frontend/src/__tests__/subtask-title.test.ts:1
The test for subtask-title.ts is placed at src/__tests__/subtask-title.test.ts, but the established pattern for shared utility tests is to co-locate them at src/shared/utils/__tests__/. Both existing shared utility test files (ansi-sanitizer.test.ts and task-status.test.ts) follow this co-located pattern. The src/__tests__/ directory is reserved for integration and e2e tests (e.g., integration/claude-profile-ipc.test.ts, integration/task-lifecycle.test.ts, e2e/smoke.test.ts), not unit tests for individual utilities.
Verified by Glob search: src/shared/utils/__tests__/ contains ansi-sanitizer.test.ts and task-status.test.ts; src/__tests__/ contains only integration and e2e tests (plus this misplaced file).
Suggested fix:
Move `apps/frontend/src/__tests__/subtask-title.test.ts` to `apps/frontend/src/shared/utils/__tests__/subtask-title.test.ts` and update the import to `import { extractSubtaskTitle } from '../subtask-title';`
This review was generated by Auto Claude.
AndyMik90
force-pushed
the
auto-claude/231-investigate-and-fix-subtask-title-display
branch
from
3bbbfb6 to
c588c83
Compare
|
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/frontend/src/shared/utils/__tests__/subtask-title.test.ts`:
- Around line 5-6: The test imports extractSubtaskTitle using a relative path;
update the import to use the project path alias so it follows tsconfig
conventions: replace "../subtask-title" with "@shared/utils/subtask-title" in
the test file that imports extractSubtaskTitle so the symbol extractSubtaskTitle
is resolved via the `@shared/`* alias.
- Around line 21-31: Add a unit test to cover the regex edge case where a long
description is a single sentence that ends with a period but has no trailing
space: inside the existing describe('long descriptions with sentence boundary')
block add an it(...) that passes a long (>80 chars) string like "Implement the
complete authentication flow for the new user registration module." to
extractSubtaskTitle and asserts the returned title equals that sentence without
the trailing period; this documents and verifies sentence extraction for a
single-sentence description ending with '.' and no following whitespace.
In `@apps/frontend/src/shared/utils/subtask-title.ts`:
- Around line 33-38: The truncation logic in
apps/frontend/src/shared/utils/subtask-title.ts can return trimmed plus an
ellipsis that exceeds maxLength for single-word inputs; update the branch that
computes cutoff (where variables truncated, lastSpace, cutoff are used) so when
no space is found (lastSpace <= 0) you reserve room for the ellipsis by setting
cutoff to max(1, maxLength - 1) (or otherwise subtract 1 from maxLength) before
returning `${trimmed.substring(0, cutoff)}\u2026`, ensuring the final string
length does not exceed maxLength.
| import { describe, it, expect } from 'vitest'; | ||
| import { extractSubtaskTitle } from '../subtask-title'; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using the @shared/ path alias for the import.
Per the project's coding guidelines, frontend files should use path aliases defined in tsconfig.json.
-import { extractSubtaskTitle } from '../subtask-title';
+import { extractSubtaskTitle } from '@shared/utils/subtask-title';As per coding guidelines: "Use path aliases defined in tsconfig.json: … @shared/* → src/shared/*"
🤖 Prompt for AI Agents
In `@apps/frontend/src/shared/utils/__tests__/subtask-title.test.ts` around lines
5 - 6, The test imports extractSubtaskTitle using a relative path; update the
import to use the project path alias so it follows tsconfig conventions: replace
"../subtask-title" with "@shared/utils/subtask-title" in the test file that
imports extractSubtaskTitle so the symbol extractSubtaskTitle is resolved via
the `@shared/`* alias.
| describe('long descriptions with sentence boundary', () => { | ||
| it('should truncate at first sentence ending with period-space', () => { | ||
| const desc = 'Fix the login button styling. Then update the tests and make sure everything works correctly across all browsers.'; | ||
| expect(extractSubtaskTitle(desc)).toBe('Fix the login button styling'); | ||
| }); | ||
|
|
||
| it('should truncate at first sentence ending with colon-space', () => { | ||
| const desc = 'Fix the login button: Then update the tests and make sure everything works correctly across all browsers and devices.'; | ||
| expect(extractSubtaskTitle(desc)).toBe('Fix the login button'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a test case for a single sentence ending with . but no trailing text.
This is the known regex edge case — a long description like "Implement the complete authentication flow for the new user registration module." (>80 chars, ends in . with no space after) will fall through to word-boundary truncation rather than sentence extraction. Adding a test documents this behavior explicitly.
Suggested test
+ it('should fall through to word truncation for a single sentence ending with period (no trailing text)', () => {
+ const desc = 'Implement the complete authentication flow for the new user registration module.';
+ // No '. ' match, so falls to word-boundary truncation if > maxLength
+ const result = extractSubtaskTitle(desc, 40);
+ expect(result.endsWith('\u2026')).toBe(true);
+ });🤖 Prompt for AI Agents
In `@apps/frontend/src/shared/utils/__tests__/subtask-title.test.ts` around lines
21 - 31, Add a unit test to cover the regex edge case where a long description
is a single sentence that ends with a period but has no trailing space: inside
the existing describe('long descriptions with sentence boundary') block add an
it(...) that passes a long (>80 chars) string like "Implement the complete
authentication flow for the new user registration module." to
extractSubtaskTitle and asserts the returned title equals that sentence without
the trailing period; this documents and verifies sentence extraction for a
single-sentence description ending with '.' and no following whitespace.
| // Truncate at last word boundary within maxLength | ||
| const truncated = trimmed.substring(0, maxLength); | ||
| const lastSpace = truncated.lastIndexOf(' '); | ||
| const cutoff = lastSpace > 0 ? lastSpace : maxLength; | ||
|
|
||
| return `${trimmed.substring(0, cutoff)}\u2026`; |
There was a problem hiding this comment.
Edge case: single-word input longer than maxLength produces a result exceeding the limit.
When trimmed has no spaces and exceeds maxLength, lastSpace is -1, so cutoff = maxLength, and the return is maxLength + 1 characters (the truncated text plus the ellipsis). This is technically fine since the ellipsis is a single display character, but worth noting for consistency with the documented "≤80 characters" contract.
🤖 Prompt for AI Agents
In `@apps/frontend/src/shared/utils/subtask-title.ts` around lines 33 - 38, The
truncation logic in apps/frontend/src/shared/utils/subtask-title.ts can return
trimmed plus an ellipsis that exceeds maxLength for single-word inputs; update
the branch that computes cutoff (where variables truncated, lastSpace, cutoff
are used) so when no space is found (lastSpace <= 0) you reserve room for the
ellipsis by setting cutoff to max(1, maxLength - 1) (or otherwise subtract 1
from maxLength) before returning `${trimmed.substring(0, cutoff)}\u2026`,
ensuring the final string length does not exceed maxLength.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@README.md`:
- Around line 39-50: The README contains a beta badge and a BETA_DOWNLOADS table
that reference a non-existent GitHub release tag "v2.7.6-beta.1"; fix this by
either removing the Beta badge block and the entire BETA_DOWNLOADS table or
updating all occurrences of the release tag and asset URLs (the "Beta" badge
line and every link in the BETA_DOWNLOADS table) to point to an actual published
release tag/URLs or an existing stable version. Ensure the badge comment markers
(<!-- BETA_VERSION_BADGE_END --> and <!-- BETA_DOWNLOADS -->) and the exact tag
string "v2.7.6-beta.1" are updated/removed so no broken links remain.
README.md
Outdated
| [](https://github.com/AndyMik90/Auto-Claude/releases/tag/v2.7.6-beta.1) | ||
| <!-- BETA_VERSION_BADGE_END --> | ||
|
|
||
| <!-- BETA_DOWNLOADS --> | ||
| | Platform | Download | | ||
| |----------|----------| | ||
| | **Windows** | [Auto-Claude-2.7.6-beta.5-win32-x64.exe](https://github.com/AndyMik90/Auto-Claude/releases/download/v2.7.6-beta.5/Auto-Claude-2.7.6-beta.5-win32-x64.exe) | | ||
| | **macOS (Apple Silicon)** | [Auto-Claude-2.7.6-beta.5-darwin-arm64.dmg](https://github.com/AndyMik90/Auto-Claude/releases/download/v2.7.6-beta.5/Auto-Claude-2.7.6-beta.5-darwin-arm64.dmg) | | ||
| | **macOS (Intel)** | [Auto-Claude-2.7.6-beta.5-darwin-x64.dmg](https://github.com/AndyMik90/Auto-Claude/releases/download/v2.7.6-beta.5/Auto-Claude-2.7.6-beta.5-darwin-x64.dmg) | | ||
| | **Linux** | [Auto-Claude-2.7.6-beta.5-linux-x86_64.AppImage](https://github.com/AndyMik90/Auto-Claude/releases/download/v2.7.6-beta.5/Auto-Claude-2.7.6-beta.5-linux-x86_64.AppImage) | | ||
| | **Linux (Debian)** | [Auto-Claude-2.7.6-beta.5-linux-amd64.deb](https://github.com/AndyMik90/Auto-Claude/releases/download/v2.7.6-beta.5/Auto-Claude-2.7.6-beta.5-linux-amd64.deb) | | ||
| | **Linux (Flatpak)** | [Auto-Claude-2.7.6-beta.5-linux-x86_64.flatpak](https://github.com/AndyMik90/Auto-Claude/releases/download/v2.7.6-beta.5/Auto-Claude-2.7.6-beta.5-linux-x86_64.flatpak) | | ||
| | **Windows** | [Auto-Claude-2.7.6-beta.1-win32-x64.exe](https://github.com/AndyMik90/Auto-Claude/releases/download/v2.7.6-beta.1/Auto-Claude-2.7.6-beta.1-win32-x64.exe) | | ||
| | **macOS (Apple Silicon)** | [Auto-Claude-2.7.6-beta.1-darwin-arm64.dmg](https://github.com/AndyMik90/Auto-Claude/releases/download/v2.7.6-beta.1/Auto-Claude-2.7.6-beta.1-darwin-arm64.dmg) | | ||
| | **macOS (Intel)** | [Auto-Claude-2.7.6-beta.1-darwin-x64.dmg](https://github.com/AndyMik90/Auto-Claude/releases/download/v2.7.6-beta.1/Auto-Claude-2.7.6-beta.1-darwin-x64.dmg) | | ||
| | **Linux** | [Auto-Claude-2.7.6-beta.1-linux-x86_64.AppImage](https://github.com/AndyMik90/Auto-Claude/releases/download/v2.7.6-beta.1/Auto-Claude-2.7.6-beta.1-linux-x86_64.AppImage) | | ||
| | **Linux (Debian)** | [Auto-Claude-2.7.6-beta.1-linux-amd64.deb](https://github.com/AndyMik90/Auto-Claude/releases/download/v2.7.6-beta.1/Auto-Claude-2.7.6-beta.1-linux-amd64.deb) | | ||
| | **Linux (Flatpak)** | [Auto-Claude-2.7.6-beta.1-linux-x86_64.flatpak](https://github.com/AndyMik90/Auto-Claude/releases/download/v2.7.6-beta.1/Auto-Claude-2.7.6-beta.1-linux-x86_64.flatpak) | |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify release tag and assets exist in GitHub for v2.7.6-beta.1
gh api repos/AndyMik90/Auto-Claude/releases/tags/v2.7.6-beta.1 2>&1 \
| jq -r '.tag_name, (.assets[]?.name)' 2>/dev/null || echo "Release not found or API error"Repository: AndyMik90/Auto-Claude
Length of output: 100
Remove or correct the non-existent beta release links—release v2.7.6-beta.1 does not exist on GitHub.
The GitHub release tag v2.7.6-beta.1 is not published, making all download links in this table (lines 39-50) broken. Either publish the release with these assets, revert to an existing version, or remove the beta section.
🤖 Prompt for AI Agents
In `@README.md` around lines 39 - 50, The README contains a beta badge and a
BETA_DOWNLOADS table that reference a non-existent GitHub release tag
"v2.7.6-beta.1"; fix this by either removing the Beta badge block and the entire
BETA_DOWNLOADS table or updating all occurrences of the release tag and asset
URLs (the "Beta" badge line and every link in the BETA_DOWNLOADS table) to point
to an actual published release tag/URLs or an existing stable version. Ensure
the badge comment markers (<!-- BETA_VERSION_BADGE_END --> and <!--
BETA_DOWNLOADS -->) and the exact tag string "v2.7.6-beta.1" are updated/removed
so no broken links remain.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 1 blocking issue(s) require fixes.
Resolution Status
- ✅ Resolved: 2 previous findings addressed
- ❌ Unresolved: 0 previous findings remain
- 🆕 New Issues: 3 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 1 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 3 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
🚨 Blocking Issues
- quality: Title and description show duplicate content for short subtask descriptions
Verdict
CI Status: ⏳ 1 CI check still pending — cannot approve for merge until all checks pass.
Previous Findings: Both previous findings (i18n bypass and test file location) have been properly resolved in commit c588c83. The author addressed the review feedback accurately.
New Finding [MEDIUM]: NEW-001 identifies that short subtask descriptions (≤80 chars) without an explicit backend title result in duplicate text in the UI (title and description show identical content). This is confirmed valid, though notably this is a partially pre-existing pattern — before this PR, title = description was always true. This PR actually improves the situation for long descriptions and subtasks with explicit titles. A simple conditional render (subtask.description !== subtask.title) would address this.
Low Severity Items (non-blocking): The regex terminal period edge case (NEW-002) and missing test case (CMT-002) are minor improvement suggestions. CMT-001 (maxLength+1) was dismissed as false positive since the test suite explicitly documents this as intended behavior.
Comments: All major CodeRabbit concerns have been addressed. The path alias suggestion was correctly identified as contradicting codebase convention. The CLA signature check is a process issue for the maintainer to resolve.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, comment-analyzer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (3 selected of 3 total)
🟡 [NEW-001] [MEDIUM] Title and description show duplicate content for short subtask descriptions
📁 apps/frontend/src/renderer/components/task-detail/TaskSubtasks.tsx:71
When a subtask has no explicit title from the backend and its description is short (<=80 chars), extractSubtaskTitle() returns the full description as the title. TaskSubtasks.tsx renders both the title (line 72) and the description (line 78) independently, causing the same text to appear twice — once as bold title and again as muted description below. This affects subtasks without explicit backend titles, which is the common case for plans created before this PR. Note: this is partially a pre-existing pattern (title was always set to description before this PR), but the PR could address it by conditionally hiding the description when it equals the title.
Suggested fix:
Conditionally render the description only when it differs from the title: {subtask.description && subtask.description !== subtask.title && (<p>...</p>)}
🔵 [NEW-002] [LOW] Sentence extraction regex does not handle terminal period without trailing space
📁 apps/frontend/src/shared/utils/subtask-title.ts:23
The regex /^(.+?(?:\. |: ))/ only matches sentences ending with '. ' (period-space) or ': ' (colon-space). Descriptions with sentences separated by newlines or ending with a bare period would not match, falling through to word-boundary truncation instead of sentence extraction. This is a minor edge case since the fallback behavior is reasonable.
Suggested fix:
Extend the regex to handle period followed by newline: /^(.+?(?:\.[\s\n]|:\s))/
🔵 [CMT-002] [LOW] Missing test for single sentence ending with period and no trailing text
📁 apps/frontend/src/shared/utils/__tests__/subtask-title.test.ts:0
No test covers a long description that is a single sentence ending with '.' but no trailing text/space. This would document the expected behavior that such input falls through to word-boundary truncation rather than sentence extraction.
Suggested fix:
Add test: it('should truncate at word boundary for single sentence with terminal period', () => { const desc = 'This is a very long single sentence that exceeds the maximum length threshold and ends with a period.'; const result = extractSubtaskTitle(desc); expect(result.endsWith('\u2026')).toBe(true); });
This review was generated by Auto Claude.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| "subtasks": [ | ||
| { | ||
| "id": "subtask-1-1", | ||
| "title": "Create data models for analytics", | ||
| "description": "Create data models for [feature]", | ||
| "service": "backend", | ||
| "files_to_modify": ["src/models/user.py"], |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/frontend/src/renderer/components/task-detail/TaskSubtasks.tsx`:
- Around line 75-88: The Tooltip is being mounted even when no TooltipContent is
rendered, causing Radix accessibility/state issues; change the rendering in
TaskSubtasks (around the subtask.description use) so that when
subtask.description exists but its length is <= 80 you render the plain <p> (the
current paragraph) directly, and only render the <Tooltip> wrapper with
<TooltipTrigger> and <TooltipContent> when subtask.description.length > 80;
update the conditional around Tooltip/TooltipTrigger/TooltipContent to check
subtask.description.length > 80 and keep the existing line-clamp/paragraph
markup outside the Tooltip for short descriptions.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 1 unresolved finding(s) from previous review.
Resolution Status
- ✅ Resolved: 2 previous findings addressed
- ❌ Unresolved: 1 previous findings remain
- 🆕 New Issues: 1 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 5 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 3 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
Verdict
CI Status: ⏳ 1 CI check still pending — cannot approve for merge until all checks pass.
Previous Findings: The blocking MEDIUM finding (NEW-001: duplicate title/description display) has been properly resolved in commit c6745ad. The author added subtask.description !== subtask.title to conditionally suppress the description when it matches the title. Two LOW findings (NEW-002: regex terminal period, CMT-002: missing test) remain unresolved but were re-validated as genuine — they are minor edge cases that don't block functionality.
New Code: 5 new findings were identified by code review and comment analysis. After thorough validation by finding-validator, 4 were dismissed as false positives (schema optional_fields not used by validator, serialization asymmetry follows established patterns, abbreviation edge case unrealistic for AI input, whitespace comparison theoretical only). 1 new finding was confirmed valid: the Tooltip rendering without TooltipContent for short descriptions (LOW severity, accessibility concern flagged by both new-code-reviewer and CodeRabbit).
Bottom Line: Only 3 LOW severity findings remain confirmed valid, and the MEDIUM blocking issue from the previous review is resolved. Once CI passes, this PR would qualify for MERGE_WITH_CHANGES. The remaining findings are minor improvements that could be addressed in a follow-up.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, comment-analyzer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (2 selected of 2 total)
🔵 [NEW-002] [LOW] [UNRESOLVED] Sentence extraction regex does not handle terminal period without trailing space
📁 apps/frontend/src/shared/utils/subtask-title.ts:23
The regex /^(.+?(?:\. |: ))/ only matches sentences ending with '. ' (period-space) or ': ' (colon-space). Descriptions with sentences separated by newlines or ending with a bare period would not match, falling through to word-boundary truncation instead of sentence extraction. This is a minor edge case since the fallback behavior is reasonable.
Resolution note: subtask-title.ts line 23: const sentenceMatch = trimmed.match(/^(.+?(?:\. |: ))/); — the regex still only matches period-space or colon-space. A description >80 chars ending with '.' but no trailing space falls through to word-boundary truncation instead of clean sentence extraction. Commit c6745ad did not modify this file's regex.
Suggested fix:
Extend the regex to handle period followed by newline: /^(.+?(?:\.[\s\n]|:\s))/
🔵 [NCR-003] [LOW] Tooltip without TooltipContent renders for descriptions <= 80 chars
📁 apps/frontend/src/renderer/components/task-detail/TaskSubtasks.tsx:76
When subtask.description exists, differs from subtask.title, but has length <= 80 characters, a Radix wraps but no is produced (line 82 condition is false). This creates a dangling aria-describedby reference — a minor accessibility anti-pattern also flagged by CodeRabbit. Fix: conditionally render the Tooltip wrapper only when description.length > 80, render a plain
otherwise.
Suggested fix:
Restructure rendering: when description.length > 80, wrap in Tooltip/TooltipTrigger/TooltipContent; otherwise render a plain <p> element without Tooltip wrapper.
This review was generated by Auto Claude.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 1 unresolved finding(s) from previous review.
Resolution Status
- ✅ Resolved: 1 previous findings addressed
- ❌ Unresolved: 1 previous findings remain
- 🆕 New Issues: 3 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 1 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 4 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
Verdict
[Recovered via extraction] CI has 1 check still pending, which per guidelines requires NEEDS_REVISION. Additionally, there is 1 unresolved prior finding (NEW-002 regex terminal period) and 3 new low-severity findings identified.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, comment-analyzer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (4 selected of 4 total)
🔵 [FU-64C97113] [LOW] Hardcoded 80 threshold coupled between files (NCR-NEW-001)
📁 unknown:0
[Recovered via extraction] Hardcoded 80 threshold coupled between files (NCR-NEW-001)
🔵 [FU-6FEBC716] [LOW] Degenerate single-char title extraction (NCR-NEW-002)
📁 unknown:0
[Recovered via extraction] Degenerate single-char title extraction (NCR-NEW-002)
🔵 [FU-10F41F34] [LOW] Tooltip trigger not keyboard-focusable (CMT-002)
📁 unknown:0
[Recovered via extraction] Tooltip trigger not keyboard-focusable (CMT-002)
🔵 [NEW-002] [LOW] [UNRESOLVED] [UNRESOLVED] Sentence extraction regex does not handle terminal period without trailing space
📁 apps/frontend/src/shared/utils/subtask-title.ts:23
The regex /^(.+?(?:\. |: ))/ only matches sentences ending with '. ' (period-space) or ': ' (colon-space). Descriptions with sentences separated by newlines or ending with a bare period would not match, falling through to word-boundary truncation instead of sentence extraction. This is a minor edge case since the fallback behavior is reasonable.
Resolution note: subtask-title.ts line 23: const sentenceMatch = trimmed.match(/^(.+?(?:\. |: ))/); — the regex still only matches period-space or colon-space. A description >80 chars ending with '.' but no trailing space falls through to word-boundary truncation instead of clean sentence extraction. Commit c6745ad did not modify this file's regex.
Suggested fix:
Extend the regex to handle period followed by newline: /^(.+?(?:\.[\s\n]|:\s))/
This review was generated by Auto Claude.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 3 unresolved finding(s) from previous review.
Resolution Status
- ✅ Resolved: 1 previous findings addressed
- ❌ Unresolved: 3 previous findings remain
- 🆕 New Issues: 0 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 2 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 3 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
Verdict
Ready once CI passes: 1 check(s) still pending. All code issues addressed, waiting for CI completion.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, comment-analyzer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (3 selected of 3 total)
🔵 [FU-64C97113] [LOW] [UNRESOLVED] Hardcoded 80 threshold coupled between files (NCR-NEW-001)
📁 unknown:0
[Recovered via extraction] Hardcoded 80 threshold coupled between files (NCR-NEW-001)
🔵 [FU-6FEBC716] [LOW] [UNRESOLVED] Degenerate single-char title extraction (NCR-NEW-002)
📁 unknown:0
[Recovered via extraction] Degenerate single-char title extraction (NCR-NEW-002)
🔵 [NEW-002] [LOW] [UNRESOLVED] [UNRESOLVED] [UNRESOLVED] Sentence extraction regex does not handle terminal period without trailing space
📁 apps/frontend/src/shared/utils/subtask-title.ts:23
The regex /^(.+?(?:\. |: ))/ only matches sentences ending with '. ' (period-space) or ': ' (colon-space). Descriptions with sentences separated by newlines or ending with a bare period would not match, falling through to word-boundary truncation instead of sentence extraction. This is a minor edge case since the fallback behavior is reasonable.
Resolution note: subtask-title.ts line 23: const sentenceMatch = trimmed.match(/^(.+?(?:\. |: ))/); — the regex still only matches period-space or colon-space. A description >80 chars ending with '.' but no trailing space falls through to word-boundary truncation instead of clean sentence extraction. Commit c6745ad did not modify this file's regex.
Suggested fix:
Extend the regex to handle period followed by newline: /^(.+?(?:\.[\s\n]|:\s))/
This review was generated by Auto Claude.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 1 blocking issue(s) require fixes.
Resolution Status
- ✅ Resolved: 3 previous findings addressed
- ❌ Unresolved: 0 previous findings remain
- 🆕 New Issues: 4 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 0 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 4 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
🚨 Blocking Issues
- quality: Regex abbreviation splitting issue in subtask title truncation
Verdict
Ready once CI passes: 1 check(s) still pending. All code issues addressed, waiting for CI completion.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (4 selected of 4 total)
🟡 [FU-98D59AB2] [MEDIUM] Regex abbreviation splitting issue in subtask title truncation
📁 unknown:0
[Recovered via extraction] Regex abbreviation splitting issue in subtask title truncation
🔵 [FU-A1AB6453] [LOW] Test assertion too loose - checks 81 instead of 80 character limit
📁 unknown:0
[Recovered via extraction] Test assertion too loose - checks 81 instead of 80 character limit
🔵 [FU-BA31837C] [LOW] Docstring parameter order mismatch in truncation function
📁 unknown:0
[Recovered via extraction] Docstring parameter order mismatch in truncation function
🔵 [FU-D91FC851] [LOW] Custom maxLength test assertion too loose
📁 unknown:0
[Recovered via extraction] Custom maxLength test assertion too loose
This review was generated by Auto Claude.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 3 unresolved finding(s) from previous review.
Resolution Status
- ✅ Resolved: 1 previous findings addressed
- ❌ Unresolved: 3 previous findings remain
- 🆕 New Issues: 2 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 1 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 5 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
🚨 Blocking Issues
- quality: [UNRESOLVED] Regex abbreviation splitting issue in subtask title truncation
Verdict
CI status: 1 check still pending — cannot approve until CI passes. Additionally, 3 of 4 previous findings remain unresolved (1 medium, 2 low), all confirmed valid by finding-validator. The medium-severity regex abbreviation issue (FU-98D59AB2) is technically valid — the non-greedy regex /^(.+?)(?:\.(?:\s|\n)|:\s)/ would incorrectly split on abbreviations like "e.g. " or "Dr. " — though practical impact is low since this processes AI-generated subtask descriptions. The 2 low-severity test assertion issues (assertions are 1 too loose) and 2 new low-severity findings (redundant regex alternation, tooltip char-count vs visual-line mismatch) are minor quality items. 1 previous finding (docstring parameter order) was resolved. 1 new finding (NEW-003 terminal period check asymmetry) was dismissed as a false positive. Overall, the PR is close to merge-ready — once CI passes and the medium-severity regex issue is acknowledged or addressed, this should be good to go.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, comment-analyzer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (5 selected of 5 total)
🟡 [FU-98D59AB2] [MEDIUM] [UNRESOLVED] Regex abbreviation splitting issue in subtask title truncation
📁 unknown:0
[Recovered via extraction] Regex abbreviation splitting issue in subtask title truncation
Resolution note: The regex at line 25 of subtask-title.ts /^(.+?)(?:\.(?:\s|\n)|:\s)/ still uses non-greedy matching that splits on the first . occurrence without abbreviation handling. Input like 'Dr. Smith should fix the login' would extract 'Dr' as the title. No changes were made to address this.
🔵 [FU-A1AB6453] [LOW] [UNRESOLVED] Test assertion too loose - checks 81 instead of 80 character limit
📁 unknown:0
[Recovered via extraction] Test assertion too loose - checks 81 instead of 80 character limit
Resolution note: Line 38 of subtask-title.test.ts still asserts expect(result.length).toBeLessThanOrEqual(81). The implementation guarantees max output of 80 chars (79 content + 1 ellipsis). Same issue at line 112. The assertion should be <= 80.
🔵 [FU-D91FC851] [LOW] [UNRESOLVED] Custom maxLength test assertion too loose
📁 unknown:0
[Recovered via extraction] Custom maxLength test assertion too loose
Resolution note: Line 87 of subtask-title.test.ts still asserts expect(result.length).toBeLessThanOrEqual(41) when testing with maxLength=40. The implementation guarantees max output of 40 chars. The assertion should be <= 40.
🔵 [NEW-004] [LOW] Redundant regex alternation (\s already includes \n)
📁 apps/frontend/src/shared/utils/subtask-title.ts:25
The regex /^(.+?)(?:\.(?:\s|\n)|:\s)/ uses (?:\s|\n) but JavaScript's \s already includes \n. The |\n alternation is redundant and adds unnecessary complexity. Should simplify to /^(.+?)(?:\.\s|:\s)/ or /^(.+?)[.:]\s/.
Suggested fix:
Simplify the regex to `/^(.+?)(?:\.\s|:\s)/` or `/^(.+?)[.:]\s/`
🔵 [NEW-005] [LOW] Tooltip trigger uses character count but visual truncation uses CSS line-clamp
📁 apps/frontend/src/renderer/components/task-detail/TaskSubtasks.tsx:76
The tooltip conditionally renders based on description.length > SUBTASK_TITLE_MAX_LENGTH (80) while both code paths use line-clamp-2 for visual truncation. A description under 80 chars could overflow 2 visual lines on narrow viewports and be clipped without a tooltip, making full text inaccessible to the user.
Suggested fix:
Consider using a ref with ResizeObserver to detect when scrollHeight > clientHeight and conditionally show the tooltip based on actual visual overflow rather than character count.
This review was generated by Auto Claude.
- Fix regex abbreviation splitting (MEDIUM): Replace non-greedy regex with iterative boundary matching that skips common abbreviations (Dr., e.g., etc.) - Fix redundant regex alternation (LOW): Remove unnecessary \n from regex since \s already includes newline characters - Tighten test assertions (LOW): Change toBeLessThanOrEqual from 81/41 to 80/40 to match actual implementation guarantees - Fix tooltip overflow detection (LOW): Replace character-count-based tooltip trigger with ResizeObserver-based visual overflow detection so tooltips appear when text is actually clipped by line-clamp, not just when exceeding an arbitrary character threshold - Add abbreviation handling tests for Dr., e.g., i.e., etc. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
AndyMik90
force-pushed
the
auto-claude/231-investigate-and-fix-subtask-title-display
branch
from
23cf032 to
9a7bb19
Compare
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 1 blocking issue(s) require fixes.
Resolution Status
- ✅ Resolved: 5 previous findings addressed
- ❌ Unresolved: 0 previous findings remain
- 🆕 New Issues: 4 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 0 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 4 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
🚨 Blocking Issues
- quality: OverflowDescription can never revert from overflow state
Verdict
CI status: All 20 CI checks passing ✅. All 5 previous findings (FU-98D59AB2, FU-A1AB6453, FU-D91FC851, NEW-004, NEW-005) are verified as RESOLVED — the code was significantly rewritten with an abbreviation-aware regex approach, correct test assertions, and a ResizeObserver-based overflow detection component. Good progress.
However, 1 new MEDIUM severity finding (NEW-001) was confirmed valid by finding-validator: the OverflowDescription component has a one-way latch where the ref is only attached to the <p> element (non-overflow branch). Once isOverflowing becomes true, the component renders a <button> without the ref, making ref.current null. The ResizeObserver then observes a detached node and checkOverflow can never set isOverflowing back to false. Per strict quality gates, medium-severity confirmed findings require revision.
Additionally, 3 LOW severity findings were identified: terminal period stripping inconsistency with abbreviations (NEW-002), a misleading comment (NEW-003), and a missing test for the abbreviation + terminal period interaction (NEW-004). These are suggestions and do not block merge individually.
No human contributor concerns remain unanswered. CodeRabbit paused its review. CLA check passed.
Review Process
Agents invoked: resolution-verifier, comment-analyzer, new-code-reviewer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (4 selected of 4 total)
🟡 [NEW-001] [MEDIUM] OverflowDescription can never revert from overflow state
📁 apps/frontend/src/renderer/components/task-detail/TaskSubtasks.tsx:26
The OverflowDescription component has a one-way latch problem. The ref is only attached to the <p> element when isOverflowing is false. Once overflow is detected and isOverflowing becomes true, the component renders a <button> WITHOUT the ref, so ref.current becomes null. The ResizeObserver observes a detached node, and checkOverflow can never set isOverflowing back to false. The component is permanently stuck in the tooltip state. Fix: always render a single element with the ref and conditionally wrap with Tooltip, or attach the ref to both render branches.
Suggested fix:
Always render a single element with the ref and conditionally wrap it with Tooltip. For example, render a `<p ref={ref} ...>{text}</p>` in both branches, and wrap it with `<Tooltip>` only when `isOverflowing` is true. This keeps the ref stable across renders so the ResizeObserver continues working correctly.
🔵 [NEW-002] [LOW] Terminal period stripping inconsistent with abbreviations
📁 apps/frontend/src/shared/utils/subtask-title.ts:47
The terminal period stripping logic uses !trimmed.includes('. ') to detect single sentences, but abbreviations like 'Dr. ' contain '. ' which causes the check to fail. This means 'Fix the login button.' strips the period correctly, but 'Talk to Dr. Jones.' does not strip the period. Fix: track whether the boundary loop found any real (non-abbreviation) sentence boundary using a boolean flag, and use that instead of the includes heuristic.
Suggested fix:
Add a `foundRealBoundary` boolean flag in the boundary loop. Set it to true when a non-abbreviation boundary is found. Then replace the `includes` checks in the terminal period logic with `!foundRealBoundary`.
🔵 [NEW-003] [LOW] Misleading comment about reserving space for ellipsis
📁 apps/frontend/src/shared/utils/subtask-title.ts:63
The comment 'Reserve space for ellipsis when truncating' implies explicit reservation, but the word-boundary branch works correctly by coincidence of substring indexing (lastSpace < maxLength inherently). Only the fallback branch (maxLength-1) explicitly reserves space. The comment should be clarified.
Suggested fix:
Update the comment to: '// Truncate at last word boundary; result is lastSpace chars + 1 ellipsis char, which is at most maxLength since lastSpace <= maxLength - 1'
🔵 [NEW-004] [LOW] Missing test for abbreviation + terminal period interaction
📁 apps/frontend/src/shared/utils/__tests__/subtask-title.test.ts:121
The test suite covers abbreviation handling and terminal period stripping separately but never combines both. No test exists for input like 'Talk to Dr. Jones.' which would expose the inconsistency identified in NEW-002. Adding this test would document the current behavior and catch regressions.
Suggested fix:
Add a test case: it('should handle abbreviation with terminal period', () => { const desc = 'Talk to Dr. Jones.'; expect(extractSubtaskTitle(desc)).toBe('Talk to Dr. Jones'); });
This review was generated by Auto Claude.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 1 blocking issue(s) require fixes.
Resolution Status
- ✅ Resolved: 4 previous findings addressed
- ❌ Unresolved: 0 previous findings remain
- 🆕 New Issues: 1 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 0 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 1 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
🚨 Blocking Issues
- quality: ResizeObserver not re-attached when element switches from
to
Verdict
CI status: All 20 CI checks passing ✅. All 4 previous findings (NEW-001 through NEW-004) are verified as RESOLVED — the OverflowDescription component was rewritten with a ResizeObserver-based approach, the abbreviation-aware regex correctly handles terminal period stripping, the misleading comment was replaced, and the combined abbreviation+period test was added. Excellent progress.
However, 1 new MEDIUM severity finding (NEW-005) was identified and CONFIRMED VALID by the finding-validator: the ResizeObserver useEffect dependency array is missing isOverflowing, causing the observer to become detached from the active DOM element when the component switches between
and . This is the same class of issue as the original NEW-001 finding (ref/observer lifecycle management). The fix is a one-line change: add isOverflowing to the dependency array [checkOverflow, text, isOverflowing].
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (1 selected of 1 total)
🟡 [NEW-005] [MEDIUM] ResizeObserver not re-attached when element switches from
to
📁 apps/frontend/src/renderer/components/task-detail/TaskSubtasks.tsx:37
The OverflowDescription component conditionally renders either a
(non-overflow) or
(overflow), both sharing the same ref. The useEffect at line 37 sets up a ResizeObserver with dependencies [checkOverflow, text] but omits isOverflowing. When isOverflowing transitions to true, theis unmounted and
mounts, but the effect doesn't re-run. The ResizeObserver remains attached to the detachedDOM node and never observes the active
. This means if the container is later resized (e.g., window resize) to be large enough for the text, the component cannot detect this and will stay stuck in the overflow/tooltip state. Fix: add isOverflowing to the useEffect dependency array.Suggested fix:
Add isOverflowing to the useEffect dependency array:
useEffect(() => {
checkOverflow();
const observer = new ResizeObserver(checkOverflow);
if (ref.current) {
observer.observe(ref.current);
}
return () => observer.disconnect();
}, [checkOverflow, text, isOverflowing]);
This review was generated by Auto Claude.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 2 blocking issue(s) require fixes.
Resolution Status
- ✅ Resolved: 1 previous findings addressed
- ❌ Unresolved: 0 previous findings remain
- 🆕 New Issues: 2 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 3 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 1 findings verified as genuine issues
- 👤 Needs Human Review: 1 findings require manual verification
🚨 Blocking Issues
- quality: Sentence boundary extraction runs before length check, truncating short descriptions
- quality: Element type switching with isOverflowing in useEffect deps risks render oscillation
Verdict
CI status: All 20 CI checks passing ✅. The previous finding (NEW-005: ResizeObserver not re-attached) is verified as RESOLVED — the useEffect now includes isOverflowing in its dependency array, causing proper re-attachment when the element switches between
and . However, 1 new MEDIUM finding is confirmed valid: extractSubtaskTitle() applies sentence boundary extraction before checking if the input is already short enough, causing short descriptions like 'Fix: do the thing' (18 chars) to be truncated to just 'Fix'. 1 additional MEDIUM finding (render oscillation risk from isOverflowing in useEffect deps) needs human review — the structural concern is real but practical impact is unlikely. 3 LOW findings were dismissed as false positives after code-level validation.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (2 selected of 2 total)
🟡 [NEW-001] [MEDIUM] Sentence boundary extraction runs before length check, truncating short descriptions
📁 apps/frontend/src/shared/utils/subtask-title.ts:30
The sentence boundary loop (lines 30-44) executes before the trimmed.length <= maxLength check (line 54). This means a short description that already fits within maxLength will still be truncated at the first . or : boundary. For example, extractSubtaskTitle('Fix: do the thing') returns 'Fix' instead of 'Fix: do the thing' (18 chars, well under 80-char limit). Similarly, 'Update auth module. Add tests.' (30 chars) would return 'Update auth module'. Fix: move the trimmed.length <= maxLength check to before the boundary loop so short descriptions are returned as-is.
Suggested fix:
Move the length check before the sentence boundary loop:
```typescript
const trimmed = description.trim();
// Short enough already — just strip trailing period if present
if (trimmed.length <= maxLength) {
if (/\.\s*$/.test(trimmed)) {
const stripped = trimmed.replace(/\.\s*$/, '');
if (!ABBREVIATIONS.test(stripped)) {
return stripped;
}
}
return trimmed;
}
// Only try sentence extraction for long descriptions
const boundaryPattern = /(?:\.\.s|:\s)/g;
...
#### 🟡 [NEW-002] [MEDIUM] Element type switching with isOverflowing in useEffect deps risks render oscillation
📁 `apps/frontend/src/renderer/components/task-detail/TaskSubtasks.tsx:37`
The useEffect includes `isOverflowing` in its dependency array (line 44). When isOverflowing toggles, the component switches between a <p> element and a <button> element with different CSS classes and different browser default styles (buttons have default padding/border). While practical oscillation is unlikely (button's default padding makes overflow MORE likely, stabilizing the toggle), a simpler approach exists: always render the same element type and only toggle tooltip visibility. This eliminates the DOM node switch, the need for isOverflowing in deps, and any theoretical oscillation risk.
**Suggested fix:**
Use a single element type (e.g., always
) and conditionally wrap in Tooltip based on overflow state:
function OverflowDescription({ text }: { text: string }) {
const ref = useRef<HTMLParagraphElement>(null);
const [isOverflowing, setIsOverflowing] = useState(false);
useEffect(() => {
const el = ref.current;
if (!el) return;
const check = () => setIsOverflowing(el.scrollHeight > el.clientHeight);
check();
const observer = new ResizeObserver(check);
observer.observe(el);
return () => observer.disconnect();
}, [text]);
const content = (
<p ref={ref} className="mt-1 text-xs text-muted-foreground line-clamp-2 break-words">
{text}
</p>
);
if (isOverflowing) {
return (
<Tooltip><TooltipTrigger asChild>{content}</TooltipTrigger>
<TooltipContent side="bottom" className="max-w-sm"><p className="text-xs">{text}</p></TooltipContent>
</Tooltip>
);
}
return content;
}
---
*This review was generated by Auto Claude.*
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 1 blocking issue(s) require fixes.
Resolution Status
- ✅ Resolved: 2 previous findings addressed
- ❌ Unresolved: 0 previous findings remain
- 🆕 New Issues: 2 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 0 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 2 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
🚨 Blocking Issues
- quality: ResizeObserver becomes stale when isOverflowing toggles due to React tree position change
Verdict
CI status: All 20 CI checks passing ✅. Both previous findings (NEW-001: sentence boundary before length check, NEW-002: render oscillation) are verified as RESOLVED — the commit properly addresses both issues. However, 2 new findings were identified and validated: (1) NCR-001 [medium, confirmed_valid]: The OverflowDescription component's ResizeObserver becomes stale when isOverflowing toggles because the <p> element moves to a different position in the React tree (from direct return to inside Tooltip wrapper), but the useEffect only depends on [text] so it doesn't re-run. After the first overflow toggle, overflow detection breaks permanently. (2) NCR-002 [low, confirmed_valid]: Short descriptions containing both : and . are incorrectly split at the colon-space boundary, contradicting the documented intent in code comments. Medium severity NCR-001 blocks merge per quality gates.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (2 selected of 2 total)
🟡 [NCR-001] [MEDIUM] ResizeObserver becomes stale when isOverflowing toggles due to React tree position change
📁 apps/frontend/src/renderer/components/task-detail/TaskSubtasks.tsx:30
When isOverflowing transitions from false to true, the
element moves from being a direct return to being wrapped inside . React unmounts the old
and mounts a new one at the different tree position. The useEffect only depends on [text], so it does not re-run when the DOM node changes. The ResizeObserver remains attached to the old (unmounted) DOM node and overflow detection breaks permanently. Fix: use a ref callback pattern (e.g., const [el, setEl] = useState(null) with ref={setEl} and useEffect depending on [el]) to re-attach the observer whenever the actual DOM node changes.
Suggested fix:
Use a callback ref instead of useRef to re-attach the ResizeObserver whenever the DOM node changes:
const [el, setEl] = useState<HTMLParagraphElement | null>(null);
useEffect(() => {
if (!el) return;
const check = () => setIsOverflowing(el.scrollHeight > el.clientHeight);
check();
const observer = new ResizeObserver(check);
observer.observe(el);
return () => observer.disconnect();
}, [el]);
const content = (
<p ref={setEl} className="...">{text}</p>
);
🔵 [NCR-002] [LOW] Colon-space boundary splits short descriptions when period-space is also present
📁 apps/frontend/src/shared/utils/subtask-title.ts:43
Short descriptions (<=80 chars) containing both ': ' and '. ' are incorrectly split at the colon-space boundary. For example, "Fix: align items. See related PR" (32 chars) returns "Fix" because: (1) the early return at line 33 is bypassed since the string contains period-space, (2) the boundary loop's regex matches colon-space at position 3 before period-space at position 17. This contradicts the documented intent in code comments at lines 30-32 which state colon-space should NOT be treated as a sentence boundary for short strings.
Suggested fix:
In the boundary loop, skip colon-space matches when `trimmed.length <= maxLength`:
while ((match = boundaryPattern.exec(trimmed)) !== null) {
const prefix = trimmed.substring(0, match.index);
// Skip colon-space for short strings (title-style prefixes)
if (match[0].startsWith(':') && trimmed.length <= maxLength) {
continue;
}
// existing abbreviation check...
}
This review was generated by Auto Claude.
…nents - TaskSubtasks: replace useRef with callback ref (useState<HTMLParagraphElement|null>) so ResizeObserver always attaches to the live DOM node when isOverflowing toggles and the <p> element is remounted inside the Tooltip tree - subtask-title: skip colon-space boundaries for short strings (<=maxLength) in the boundary loop so "Fix: do the thing" style prefixes are not split at ':' - subtask-title: add \b word boundary to ABBREVIATIONS regex to prevent partial-word false positives (e.g. "items" matching the Ms abbreviation) - subtask-title tests: add colon-space short string test case per PR finding Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 1 blocking issue(s) require fixes.
Resolution Status
- ✅ Resolved: 2 previous findings addressed
- ❌ Unresolved: 0 previous findings remain
- 🆕 New Issues: 3 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 0 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 3 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
🚨 Blocking Issues
- quality: OverflowDescription unmounts/remounts
on isOverflowing toggle due to tree restructuring
Verdict
CI status: All 20 CI checks passing ✅. Both previous findings fully resolved: NCR-001 (stale ResizeObserver) fixed via useState-based ref callback with useEffect dependency on [el], and NCR-002 (colon-space splitting) fixed with proper early-return guard and colon-space skip logic in the boundary loop. However, 1 new medium-severity finding (NEW-001, confirmed_valid) was identified: the OverflowDescription component still causes React tree restructuring when isOverflowing toggles (root switches between bare
and ), resulting in an unnecessary DOM teardown/rebuild cycle per overflowing description. The fix is straightforward — always wrap in Tooltip and control visibility via the open prop. Two additional low-severity findings are suggestions only: NEW-002 (missing text in useEffect deps, trivial fix) and NEW-003 (degenerate maxLength edge case, extremely low impact). The medium finding blocks merge per quality gates, though its practical impact is modest — the code is functionally correct with no oscillation.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (3 selected of 3 total)
🟡 [NEW-001] [MEDIUM] OverflowDescription unmounts/remounts
on isOverflowing toggle due to tree restructuring
📁 apps/frontend/src/renderer/components/task-detail/TaskSubtasks.tsx:26
When isOverflowing toggles, the component returns fundamentally different React tree structures: bare
(false) vs
...(true). React reconciliation unmounts the old tree and mounts a new one, causing the
DOM node to be destroyed and recreated. The useState-based ref callback properly handles ResizeObserver re-attachment so no oscillation occurs, but every overflowing description undergoes one unnecessary render cycle with DOM teardown/rebuild. Fix: always render the Tooltip wrapper and control visibility via the open prop (set to undefined when overflowing, false when not), keeping the
at a stable tree position.
Suggested fix:
Always wrap in Tooltip and control visibility:
```tsx
return (
<Tooltip open={isOverflowing ? undefined : false}>
<TooltipTrigger asChild>
<p ref={setEl} className="mt-1 text-xs text-muted-foreground line-clamp-2 break-words">
{text}
</p>
</TooltipTrigger>
{isOverflowing && (
<TooltipContent side="bottom" className="max-w-sm">
<p className="text-xs">{text}</p>
</TooltipContent>
)}
</Tooltip>
);```
🔵 [NEW-002] [LOW] useEffect missing text dependency — overflow state may become stale on text changes
📁 apps/frontend/src/renderer/components/task-detail/TaskSubtasks.tsx:30
The useEffect depends only on [el], not [el, text]. When text changes while the
DOM element stays the same, the effect does not re-run. With line-clamp-2, the rendered box height may remain constant, so ResizeObserver may not fire either, leaving isOverflowing stale. Practical impact is very low since subtask descriptions rarely change while mounted.
Suggested fix:
Add text to the dependency array: `}, [el, text]);`
🔵 [NEW-003] [LOW] Fallback truncation exceeds maxLength for degenerate values (maxLength <= 1)
📁 apps/frontend/src/shared/utils/subtask-title.ts:89
Math.max(1, maxLength - 1) + ellipsis character can produce a string longer than maxLength when maxLength is 0 or 1. No guard prevents reaching this code with such small values. The default maxLength is 80 and no callers pass values this small, making this an extremely low-impact edge case.
Suggested fix:
Either add a guard `if (maxLength < 2) return trimmed.substring(0, Math.max(0, maxLength));` at the top of the function, or document that maxLength must be >= 2.
This review was generated by Auto Claude.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 1 unresolved finding(s) from previous review.
Resolution Status
- ✅ Resolved: 2 previous findings addressed
- ❌ Unresolved: 1 previous findings remain
- 🆕 New Issues: 2 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 1 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 3 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
🚨 Blocking Issues
- quality: Colon-space boundary produces degenerate titles for long descriptions with short prefixes
Verdict
CI status: All 20 CI checks passing ✅. The 3 previous findings show excellent progress — NEW-001 (medium, tree restructuring) and NEW-002 (low, missing text dep) are fully resolved with clean implementations. NEW-003 (low, degenerate maxLength) is partially resolved with a guard for maxLength < 1, though maxLength === 1 remains affected (extreme edge case). However, 1 new medium-severity finding (NCR-NEW-001) was identified and confirmed valid: the colon-space boundary logic produces degenerate single-word titles (e.g., just "Fix") for long descriptions starting with short prefixes like "Fix: ...". This is a realistic AI-generated description pattern that produces poor UX. Per review policy, confirmed medium findings block merge. 1 finding (NCR-NEW-002) was dismissed as a false positive (hypothetical concern about ellipsis char width, not a real bug).
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (2 selected of 2 total)
🟡 [NCR-NEW-001] [MEDIUM] Colon-space boundary produces degenerate titles for long descriptions with short prefixes
📁 apps/frontend/src/shared/utils/subtask-title.ts:52
For long descriptions (> maxLength) starting with short colon-prefixed labels like 'Fix: align the items in the header bar and also update the sidebar navigation component to match', the colon-space skip on line 52 does NOT apply because trimmed.length > maxLength. The function splits at ': ' and returns just 'Fix' as the title, which is nearly meaningless. The skip condition should also check prefix length, e.g.: if (match[0].startsWith(':') && (trimmed.length <= maxLength || prefix.trim().length < 15)) { continue; }
Suggested fix:
Add a minimum prefix length check for colon-space splits:
if (match[0].startsWith(':') && (trimmed.length <= maxLength || prefix.trim().length < 15)) {
continue;
}
🔵 [NCR-NEW-003] [LOW] Title line-clamped without tooltip when description equals title
📁 apps/frontend/src/renderer/components/task-detail/TaskSubtasks.tsx:101
When subtask.description === subtask.title, the OverflowDescription component (which provides overflow tooltips) is not rendered. The title span has line-clamp-2 but no tooltip mechanism. LLM-provided titles have no frontend-enforced length constraint. If a title exceeds ~2 lines of text, it will be visually truncated with no way to see the full text. The OverflowDescription pattern with ResizeObserver already exists and could be applied to titles.
Suggested fix:
Consider adding overflow tooltip to the title span, or enforce title length at the frontend before rendering.
This review was generated by Auto Claude.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…examples Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…interface Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add shared utility for extracting concise titles from subtask descriptions with sentence extraction and word-boundary truncation fallback. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… fallback in project-store Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… fallback in task-store Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…e span Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Return empty string instead of hardcoded 'Untitled' from
extractSubtaskTitle() so the UI component's i18n fallback
(t('tasks:subtasks.untitled')) activates correctly for all locales.
Move subtask-title.test.ts to src/shared/utils/__tests__/ to match
the established co-located test pattern for shared utilities.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address review finding NEW-001 by conditionally rendering description only when it differs from the title. This prevents duplicate text when extractSubtaskTitle() returns the full description for short subtasks. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address review findings from sentry and CodeRabbit: 1. Add 'title' to subtask validation schema optional_fields so the AI planner can actually generate the field (HIGH severity finding) 2. Fix Tooltip rendering to only wrap content when description >80 chars, preventing Radix accessibility issues when TooltipContent is absent Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace paragraph element with button for tooltip trigger to enable keyboard navigation and focus visibility. Adds proper focus ring styling for accessibility compliance. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fixes three LOW severity findings from auto-claude PR review: 1. Decouple hardcoded 80-char threshold using SUBTASK_TITLE_MAX_LENGTH constant 2. Improve regex to handle terminal periods without trailing space/newline 3. Fix truncation to never exceed maxLength even for single-word inputs Changes: - Export SUBTASK_TITLE_MAX_LENGTH constant from subtask-title.ts - Update TaskSubtasks.tsx to use the constant instead of magic number 80 - Enhanced sentence extraction regex to handle period+newline and terminal periods - Improved truncation logic to ensure maxLength-1 + ellipsis for degenerate cases - Added comprehensive tests for terminal period edge cases and degenerate truncation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix regex abbreviation splitting (MEDIUM): Replace non-greedy regex with iterative boundary matching that skips common abbreviations (Dr., e.g., etc.) - Fix redundant regex alternation (LOW): Remove unnecessary \n from regex since \s already includes newline characters - Tighten test assertions (LOW): Change toBeLessThanOrEqual from 81/41 to 80/40 to match actual implementation guarantees - Fix tooltip overflow detection (LOW): Replace character-count-based tooltip trigger with ResizeObserver-based visual overflow detection so tooltips appear when text is actually clipped by line-clamp, not just when exceeding an arbitrary character threshold - Add abbreviation handling tests for Dr., e.g., i.e., etc. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…reviations - Attach ref to both <button> and <p> in OverflowDescription so ResizeObserver can detect when overflow resolves (was a one-way latch) - Fix terminal period stripping to check only the trailing period, not whether '. ' appears anywhere (failed for 'Talk to Dr. Jones.') - Update misleading "Reserve space for ellipsis" comment - Add test for abbreviation + terminal period combination Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add isOverflowing to the useEffect dependency array so the ResizeObserver is re-created when the component switches between the <p> and <button> elements, preventing the observer from staying attached to a detached DOM node. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…e OverflowDescription render oscillation - Move length check before boundary loop in subtask-title.ts so short strings (e.g. "Fix: do the thing") are returned as-is instead of being truncated at the first ": " or ". " boundary; fall through to boundary loop only when the string contains a genuine period-whitespace sentence break - Add a post-loop length guard to handle short strings whose abbreviation periods (Dr., e.g., etc.) caused them to bypass the early-return path - Replace p/button element-type switch in OverflowDescription with a stable <p> ref; conditionally wrap in Tooltip when overflowing, eliminating the DOM node swap that risked render oscillation; remove isOverflowing from ResizeObserver effect deps and drop unused useCallback import Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…nents - TaskSubtasks: replace useRef with callback ref (useState<HTMLParagraphElement|null>) so ResizeObserver always attaches to the live DOM node when isOverflowing toggles and the <p> element is remounted inside the Tooltip tree - subtask-title: skip colon-space boundaries for short strings (<=maxLength) in the boundary loop so "Fix: do the thing" style prefixes are not split at ':' - subtask-title: add \b word boundary to ABBREVIATIONS regex to prevent partial-word false positives (e.g. "items" matching the Ms abbreviation) - subtask-title tests: add colon-space short string test case per PR finding Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- OverflowDescription: always render <Tooltip> wrapper and control
visibility via open prop (open={isOverflowing ? undefined : false})
to prevent React unmounting/remounting the <p> on isOverflowing toggle
- OverflowDescription: add `text` to useEffect dependency array so
overflow is re-measured when text content changes
- extractSubtaskTitle: guard against degenerate maxLength < 1 by
returning '' immediately after the empty-input early return
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
For descriptions like "Fix: align the items in the header bar...", splitting at ': ' would return just "Fix" as the title which is meaningless. Add a minimum prefix length check (< 15 chars) so short label prefixes are skipped and the function falls through to word-boundary truncation instead. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
AndyMik90
force-pushed
the
auto-claude/231-investigate-and-fix-subtask-title-display
branch
from
5f0fe23 to
e0f589e
Compare
…vestigate-and-fix-subtask-title-display
…ssibility Generalize OverflowDescription into a reusable OverflowText component that supports both <p> and <span> elements. Apply it to the subtask title span so that long titles truncated by line-clamp-2 show a tooltip with the full text. The tooltip trigger is also keyboard-focusable (tabIndex=0) when overflow is detected, addressing the accessibility finding from PR review NCR-NEW-003. Also merges latest develop to bring the branch up to date. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
Base Branch
developbranch (required for all feature/fix PRs)main(hotfix only - maintainers)Description
This PR fixes excessively long subtask titles in the task modal by adding a
titlefield to the planner's subtask JSON schema and implementing a fallback extraction utility. Subtask titles now display concisely (≤80 chars) by using the newtitlefield when available, falling back to an intelligent extraction of the first sentence or truncated description. CSS truncation (line-clamp-2) provides an additional UI safety net.Related Issue
Closes #231
Type of Change
Area
Commit Message Format
Follow conventional commits:
<type>: <subject>Types: feat, fix, docs, style, refactor, test, chore
Example:
feat: add user authentication systemAI Disclosure
Tool(s) used: Claude Code
Testing level:
Untested -- AI output not yet verified
Lightly tested -- ran the app / spot-checked key paths
Fully tested -- all tests pass, manually verified behavior
I understand what this PR does and how the underlying code works
Checklist
developbranchPlatform Testing Checklist
CRITICAL: This project supports Windows, macOS, and Linux. Platform-specific bugs are a common source of breakage.
platform/module instead of directprocess.platformchecksfindExecutable()or platform abstractions)If you only have access to one OS: CI now tests on all platforms. Ensure all checks pass before submitting.
CI/Testing Requirements
Screenshots
N/A - No UI changes visible to users; internal data mapping fix.
Feature Toggle
use_feature_nameBreaking Changes
Breaking: No
Details:
Changes are fully backward compatible. The new
titlefield is optional in thePlanSubtaskinterface and in the backend subtask dataclass. Existing implementation plans without atitlefield continue to work via theextractSubtaskTitle()fallback, which intelligently derives a short title from the description.Summary by CodeRabbit
New Features
Improvements
Documentation
Tests