Conversation
Add comprehensive design for GitHub reactions support including: - Data fetching via GraphQL reactionGroups - Output formatting with emoji support and fallback - New 'react' command with short ID and interactive modes - All 8 GitHub reaction types support Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Comprehensive step-by-step plan with: - 12 tasks broken into 2-5 minute steps - TDD approach with tests first - Exact file paths and code snippets - All commands with expected outputs - Success criteria checklist Ready for execution with superpowers:executing-plans Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Add Reactor interface for users who reacted - Add ReactionGroup interface for grouped reactions - Extend ThreadComment with optional reactionGroups field - Add comprehensive type validation tests Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Extend THREADS_QUERY with reactionGroups field - Extend THREAD_COMMENTS_QUERY with reactionGroups field - Fetch up to 100 reactors per reaction type - Support User, Bot, Organization, and Mannequin reactors - Add tests for query structure validation Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Add REACTION_EMOJI mapping for 8 GitHub reactions - Add emoji support detection for terminals - Add normalizeReaction for input validation - Support emoji, uppercase, and lowercase input formats - Comprehensive test coverage for all utilities Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Add formatReactionGroups function for plain text - Detect emoji support automatically via supportsEmoji - Display reactions after each comment body - Format: emoji/text (count): @user1, @user2 - Extend ProcessedThread comments with reactionGroups field - Add comprehensive tests for formatting Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Include reactionGroups in ProcessedThread comments mapping - Only include reactionGroups when reactions exist - Add tests for JSON formatting with and without reactions - Transform reaction data through threadProcessor Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Add ADD_REACTION_MUTATION to mutations.ts - Add AddReactionMutationData type to apiTypes.ts - Mutation accepts subjectId and ReactionContent - Returns reaction id, content, and subject id - Add test for mutation structure validation Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Add runReactCommand with batch reaction support - Normalize reaction input (emoji or text) - Use shared command utilities for ID resolution - Provide friendly error messages for common failures - Execute reactions sequentially via GitHub GraphQL API - Add tests for reaction normalization and validation Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Add 'react' command with reaction and ids arguments - Support multiple comment IDs for batch reactions - Import runReactCommand dynamically - Add 'react' to SUBCOMMANDS list - Show command in help output - Arguments: <reaction> <ids...> Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Add React Command section to README with usage examples - Document all 8 supported reaction types - Show emoji, uppercase, and lowercase input formats - Add react.ts description to CLAUDE.md Commands section - Include implementation details and error handling Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Summary of changes: - Added Reactor and ReactionGroup types - Extended GraphQL queries with reactionGroups - Implemented reaction formatting utilities - Added reaction display in plain text and JSON output - Implemented react command for adding reactions - Added comprehensive test coverage (127 tests passing) - Updated documentation (README and CLAUDE.md) All tests passing ✅ Type-safe ✅ Linted ✅ Coverage >95% for new code ✅ Implements reactions feature as specified in design document. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a new "react" CLI command and reaction support (utils, GraphQL mutation, tests), introduces TTL pagination caching and cursorCache propagation, enhances data fetching and plain/text output to include reactionGroups and PR file stats, extends filtering for line ranges and nitpick ID matching, and deletes several documentation files. Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as CLI Parser
participant React as React Command
participant Batch as Batch Context
participant GitHub as GitHub API
participant Reporter as Result Reporter
User->>CLI: react THUMBS_UP <id1> <id2>
CLI->>React: runReactCommand([id1, id2], "THUMBS_UP")
React->>React: normalizeReaction("THUMBS_UP")
React->>Batch: prepareBatchCommandContext(ids)
Batch-->>React: validated IDs
React->>GitHub: ADD_REACTION_MUTATION(id1, THUMBS_UP)
alt Success
GitHub-->>React: {addReaction: {reaction: {id, content}, subject: {id}}}
React->>React: Record success + log
else Already Reacted / NotFound / Error
GitHub-->>React: Error
React->>React: Map to friendly error + record failure
end
React->>GitHub: ADD_REACTION_MUTATION(id2, THUMBS_UP)
React->>Reporter: reportBatchResults(successes, failures)
Reporter->>User: Output summary (X succeeded, Y failed)
React->>User: Exit with status 0/1
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
The file contained outdated examples with non-existent options: - --only=userComments (not supported) - .userComments[] in jq output (doesn't exist) - .summary.userCommentsByAuthor (doesn't exist) README.md already contains up-to-date usage examples. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@docs/plans/2026-02-05-reactions-implementation.md`:
- Around line 1088-1104: The markdown block under the "Add reaction to comment"
section uses triple-backtick fences that contain an inner triple-backtick code
block, which triggers markdownlint MD040; update the outer fence to a longer
fence (e.g., change the opening ```markdown to ````markdown and the closing ```
to ````) so the inner ```bash block is not treated as closing the outer fence,
keeping the same content and examples (the GH CLI examples and supported
reactions list) intact.
In `@src/commands/react.test.ts`:
- Around line 9-20: Add unit tests covering the runReactCommand function: write
tests for a successful reaction flow, input validation failures, and mutation
errors by mocking runGhMutation with vitest-mock-extended; specifically, create
a mock for runGhMutation to return successful mutation results for the success
case and throw or return error payloads for mutation-failure cases, and assert
runReactCommand handles batches and error messages correctly (also include tests
asserting normalizeReaction behavior indirectly via runReactCommand validation).
Ensure tests use vitest-mock-extended for type-safe mocks of runGhMutation and
assert expected outputs/errors from runReactCommand.
In `@src/github/apiTypes.ts`:
- Around line 79-89: The AddReactionMutationData type declares addReaction as
required but GraphQL payloads are nullable and react.ts already uses optional
chaining; update the AddReactionMutationData interface so addReaction is
optional (change addReaction to addReaction?: { ... }) and make the nested
fields optional as well (reaction?: { id?: string; content?: string } and
subject?: { id?: string }) to accurately reflect nullable mutation responses;
adjust any imports/usages if the type change surfaces new TypeScript errors.
In `@src/output/plainFormatter.ts`:
- Around line 25-35: formatReactionGroups currently hardcodes two leading spaces
causing reply reactions to be outdented; change its signature to accept an
indent parameter (e.g., commentIndent: string = ' ') instead of using the
hardcoded " ", replace the leading spaces in the return mapping with that
commentIndent, and propagate this new parameter to all callers (including the
other occurrence noted) so they pass the correct nesting indent; keep a default
value to preserve existing behavior when callers are not yet updated and ensure
formatReaction is still used to build the icon.
🧹 Nitpick comments (4)
src/utils/reactions.ts (1)
53-61: Trim input and returnReactionTypefor stronger typing.This improves UX for stray whitespace and tightens the contract for callers.
♻️ Suggested change
-export function normalizeReaction(input: string): string { - const upper = input.toUpperCase(); +export function normalizeReaction(input: string): ReactionType { + const normalized = input.trim(); + const upper = normalized.toUpperCase(); if (VALID_REACTIONS.includes(upper as ReactionType)) { return upper; } - if (EMOJI_TO_REACTION[input]) { - return EMOJI_TO_REACTION[input]; + if (EMOJI_TO_REACTION[normalized]) { + return EMOJI_TO_REACTION[normalized] as ReactionType; } throw new Error(`Invalid reaction: ${input}`); }src/types.test.ts (1)
24-34: Consider typing thecommentobject for stronger validation.The test creates a plain object literal without importing or using the
ThreadCommenttype. This means TypeScript won't actually validate that the object conforms to the interface.💡 Proposed fix to add type annotation
+import type { Reactor, ReactionGroup, ThreadComment } from './types.js'; - it('should validate ThreadComment with optional reactionGroups', () => { - const comment = { + it('should validate ThreadComment with optional reactionGroups', () => { + const comment: ThreadComment = { id: 'IC_test', body: 'test comment', author: { login: 'user', __typename: 'User' }, url: 'https://github.com/test', createdAt: '2026-02-05T10:00:00Z', reactionGroups: [] };src/commands/react.ts (1)
27-61: Consider usingfor...ofinstead ofArray.from().forEach().Using
for...ofis more idiomatic and allows for better control flow (e.g., early exit withbreakif needed in the future).♻️ Proposed refactor
- Array.from(context.resolvedIds.entries()).forEach(([shortId, fullId]) => { + for (const [shortId, fullId] of context.resolvedIds.entries()) { try { const mutationResult = runGhMutation<AddReactionMutationData>( ADD_REACTION_MUTATION, { subjectId: fullId, content: normalizedReaction } ); const reactionId = mutationResult.addReaction?.reaction?.id; result.successful.push(shortId); console.log(`✓ Added ${normalizedReaction} reaction to comment ${shortId}`); if (reactionId) { console.log(` Reaction ID: ${reactionId}`); } } catch (error: unknown) { const errorMessage = error instanceof Error ? error.message : String(error); // Provide friendly error messages if (errorMessage.includes('already reacted')) { result.failed.push({ id: shortId, error: 'You have already reacted with this emoji' }); } else if (errorMessage.includes('Not Found')) { result.failed.push({ id: shortId, error: 'Comment not found or you don\'t have access' }); } else { result.failed.push({ id: shortId, error: errorMessage }); } } - }); + }docs/plans/2026-02-05-reactions-design.md (1)
113-133: Add a language specifier to the fenced code block.The code block for output format examples is missing a language specifier, which triggers a markdown lint warning.
📝 Proposed fix
-``` +```text Plain text (with emoji support): 👍 (3): `@user1`, `@user2`, `@user3` ❤️ (1): `@user4`
| Add after the `resolve` command section: | ||
|
|
||
| ```markdown | ||
| ### Add reaction to comment | ||
| ```bash | ||
| gh-pr-threads react <short_id> <reaction> | ||
| gh-pr-threads react abc123 👍 | ||
| gh-pr-threads react abc123 THUMBS_UP | ||
|
|
||
| # Multiple comments | ||
| gh-pr-threads react abc123 def456 ❤️ | ||
| ``` | ||
|
|
||
| Supported reactions: 👍 👎 😄 🎉 😕 ❤️ 🚀 👀 | ||
|
|
||
| Accepts emoji or text format (THUMBS_UP, thumbs_up, 👍). | ||
| ``` |
There was a problem hiding this comment.
Avoid nested triple‑backtick fences (markdownlint MD040).
The README snippet uses inside amarkdown block, which can terminate the outer fence and trigger MD040. Use a longer outer fence (or indent the inner block).
Proposed fix
-```markdown
+````markdown
### Add reaction to comment
```bash
gh-pr-threads react <short_id> <reaction>
gh-pr-threads react abc123 👍
gh-pr-threads react abc123 THUMBS_UP
# Multiple comments
gh-pr-threads react abc123 def456 ❤️Supported reactions: 👍 👎 😄 🎉 😕 ❤️ 🚀 👀
Accepts emoji or text format (THUMBS_UP, thumbs_up, 👍).
-```
+````
</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.20.0)</summary>
[warning] 1104-1104: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In @docs/plans/2026-02-05-reactions-implementation.md around lines 1088 - 1104,
The markdown block under the "Add reaction to comment" section uses
triple-backtick fences that contain an inner triple-backtick code block, which
triggers markdownlint MD040; update the outer fence to a longer fence (e.g.,
change the opening markdown to ````markdown and the closing to ````) so
the inner ```bash block is not treated as closing the outer fence, keeping the
same content and examples (the GH CLI examples and supported reactions list)
intact.
</details>
<!-- fingerprinting:phantom:poseidon:eagle -->
<!-- This is an auto-generated comment by CodeRabbit -->
Removed files: - PROJECT_SUMMARY.md (193 lines) - outdated project structure - QUICKSTART.md (83 lines) - duplicates README, references old options - CHEATSHEET.md (151 lines) - duplicates README - CONTRIBUTING.md (89 lines) - info already in CLAUDE.md - CHANGELOG.md (23 lines) - outdated, git log serves as changelog Added to CLAUDE.md: - Documentation Policy section - Rule: NEVER create new .md files - Only README.md (user docs) and CLAUDE.md (dev notes) maintained - Git commit messages serve as changelog Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@CLAUDE.md`:
- Around line 88-94: The Documentation Policy in CLAUDE.md currently forbids
creating new .md files and says git commit messages are the changelog, which
conflicts with the later "Publishing Checklist" that requires CHANGELOG.md;
update CLAUDE.md to reconcile by either allowing a single maintained
CHANGELOG.md (modify the "Documentation Policy" bullets to permit CHANGELOG.md
and describe who updates it and when) or by removing CHANGELOG.md from the
Publishing Checklist and replacing it with the established git-commit-based
changelog process; edit the "Documentation Policy" and the "Publishing
Checklist" sections so they consistently reference the same changelog mechanism
and include a short rule about where to record release notes (e.g., commit
messages vs CHANGELOG.md) and who is responsible for updates.
Move reactions from end of comment body to immediately after thread URL, before author name. This places reactions right under the separator line as requested, making them more prominent and easier to see at a glance. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/github/queries.ts`:
- Around line 13-26: The duplicated reactionGroups selection used in
THREADS_QUERY and THREAD_COMMENTS_QUERY should be extracted into a shared
GraphQL fragment: create a constant REACTION_GROUPS_FRAGMENT containing the
fragment definition (fragment ReactionGroupFields on ReactionGroup {
...fields... }) and replace the inline reactionGroups selection in both
THREADS_QUERY and THREAD_COMMENTS_QUERY with the spread ...ReactionGroupFields,
then append or interpolate REACTION_GROUPS_FRAGMENT into the query strings so
both queries include the fragment definition.
🧹 Nitpick comments (1)
src/github/queries.ts (1)
13-26: Consider extracting the reactionGroups selection into a shared fragment.
The same block appears twice, which can drift over time.♻️ Suggested refactor (GraphQL fragment)
+const REACTION_GROUP_FRAGMENT = ` + fragment ReactionGroupFields on ReactionGroup { + content + createdAt + viewerHasReacted + reactors(first: 10) { + totalCount + nodes { + ... on User { login } + ... on Bot { login } + ... on Organization { login } + ... on Mannequin { login } + } + } + } +`; + export const THREADS_QUERY = ` query($owner: String!, $repo: String!, $number: Int!, $after: String) { @@ nodes { id body author { login __typename } url createdAt path line - reactionGroups { - content - createdAt - viewerHasReacted - reactors(first: 10) { - totalCount - nodes { - ... on User { login } - ... on Bot { login } - ... on Organization { login } - ... on Mannequin { login } - } - } - } + reactionGroups { ...ReactionGroupFields } } } @@ -`; + } + ${REACTION_GROUP_FRAGMENT} +`; @@ export const THREAD_COMMENTS_QUERY = ` @@ nodes { id body author { login __typename } url createdAt path line - reactionGroups { - content - createdAt - viewerHasReacted - reactors(first: 10) { - totalCount - nodes { - ... on User { login } - ... on Bot { login } - ... on Organization { login } - ... on Mannequin { login } - } - } - } + reactionGroups { ...ReactionGroupFields } } @@ -`; + } + ${REACTION_GROUP_FRAGMENT} +`;As per coding guidelines, Extract duplicated code into shared utilities.
Also applies to: 99-112
Move reactions from above comment to inside quote block, displayed after "---" separator. This makes the layout match GitHub's UI where reactions appear at the bottom of the comment body. Changes: - Updated formatThread to add reactions after comment body - Added "---" separator before reactions - Updated formatReactionGroups to include quote pipe (│) prefix - Updated all tests to expect new format with pipe prefix Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/output/plainFormatter.ts`:
- Around line 546-556: The reaction separator block misaligns and is uncolored:
modify the block that checks comment.reactionGroups (the lines that push
`${commentIndent}│`, `...│ ---`, and `${commentIndent}│`) to use the same
two-space quote prefix and per-user coloring as wrapInQuote (i.e. use
`${commentIndent} ${userColor('│')}` and color the separator text similarly),
and update formatReactionGroups to either accept a color function/author color
parameter or to emit reaction lines prefixed with the same `${commentIndent}
${userColor('│')}` so reactions vertically align and use the same user color as
the quote bar.
- Around line 46-48: The reaction bar in formatReactionGroups is misaligned with
wrapInQuote because wrapInQuote outputs `${indent} │ content` (two spaces
before the bar) while formatReactionGroups currently returns `${indent}│
${icon} ...` (no spaces). Update the return template in formatReactionGroups
(the line returning `${indent}│ ${icon} ${usersList}${remaining}`) to insert
the same two spaces before the vertical bar so it becomes `${indent} │ ${icon}
${usersList}${remaining}`, ensuring both functions use the same alignment.
🧹 Nitpick comments (1)
src/output/plainFormatter.test.ts (1)
88-91: Consider adding a test for custom indent parameter.The tests use the default indent value. Given the
indentparameter affects output alignment, adding a test case that verifies custom indent values produce correctly structured output would help catch alignment regressions.Example test case
it('should respect custom indent parameter', () => { const groups: ReactionGroup[] = [ { content: 'THUMBS_UP', createdAt: '2026-02-05T10:00:00Z', viewerHasReacted: false, reactors: { totalCount: 1, nodes: [{ login: 'user1' }] } } ]; const result = formatReactionGroups(groups, true, ' '); // Verify the custom indent is applied (6 spaces before the bar) expect(result).toMatch(/^ {2}│/); });
Changes: - Move reactions to end of bodyLines before wrapInQuote call - Add "---" separator and reactions as part of comment body - Remove formatReactionGroups pipe prefix (wrapInQuote adds it) - Update all tests to expect reactions without pipe prefix - Add regex to cleanCommentBody to remove "🧠 Learnings used" details blocks - Add test for Learnings removal This ensures reactions appear inside the quote block with proper alignment, and removes CodeRabbit's "Learnings used" sections that only add noise. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Changes: - Add @ prefix to all author names (not just PR author) - Add "(author owner)" badge for PR author in replies - Use userColor for reply authors (not dim gray) - Consistent colored author display throughout thread This makes it clear who the PR owner is and matches GitHub's @mention style. The color now matches the quote bar color for each author. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/output/plainFormatter.ts`:
- Around line 531-533: The badge text is confusing because it mixes "author" and
"owner"; update the badges construction so that when comment.author === prAuthor
it uses a clear label such as ' (author)' or ' (PR author)' instead of ' (author
owner)'; change the badges variable assignment (used to build authorDisplay) to
reflect the clearer label and ensure authorDisplay =
`@${comment.author}${badges}` continues to work.
- Around line 25-50: The parameter indent in formatReactionGroups is unused
causing lint failure; either (A) apply it when constructing each line in the
returned string inside the nonEmptyGroups.map (prepend indent to the returned
template string so each line begins with the indent) — reference function
formatReactionGroups and the mapping that returns `${icon}
${usersList}${remaining}`, or (B) if indentation isn’t needed now, rename the
parameter to _indent (or prefix it with an underscore) to satisfy the
no-unused-vars rule. Make the change in formatReactionGroups so the declared
parameter is used or clearly marked unused.
Changes: - Add formatAuthor() helper function for consistent author display - Change badge format from "(author owner)" to "(author) (owner)" - Add @ prefix to nitpick authors (@coderabbitai) - Fix duplicate separator issue - don't add extra --- before reactions - Fix escaped \n in split() call (was '\\n', now '\n') This centralizes author formatting logic in one place and fixes the double separator issue where markdown --- from comment body was duplicated with our separator. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Design approved for enhancing PR output with: - Emoji and double-line separators - Total additions/deletions in PR header - Per-file additions/deletions statistics - Soft color palette (greenBright/yellow) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Fixed the issue where code blocks with ```suggestion inside <details> sections were causing "Unknown language: 'suggestion'" errors from cli-highlight library. Changes: - Added language mapping in formatDetailBlock() to convert 'suggestion' to 'diff' - This mirrors the existing mapping in formatMainContent() - Added comprehensive unit tests to verify the fix The fix ensures that suggestion blocks are properly highlighted as diff syntax regardless of whether they appear in the main content or inside details sections. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit fixes two related issues with thread/nitpick filtering: 1. **Nitpick Filtering**: The --thread option now correctly filters and displays nitpicks by their short ID. Previously, when targeting a nitpick ID (e.g., --thread baafe8), the tool would fetch data but fail to display the nitpick. 2. **Line Range Support**: Thread filtering now correctly handles line ranges (e.g., path:13-26). Previously, only the start line was checked, causing threads at the end line to be missed. Changes: - Add src/core/nitpickFilter.ts: New module for filtering nitpicks by ID with support for line ranges and overlapping ranges - Update src/core/threadFilter.ts: Fix line range matching to check if thread.line falls within [startLine, endLine] - Update src/core/dataFetcher.ts: Fetch reviews/comments when targeting nitpicks (path:line format) instead of skipping them - Update src/index.ts: Add nitpick filtering logic and filter bot summaries to show only matching nitpicks - Update src/output/plainFormatter.ts: Display content when nitpicks exist even if processedThreads is empty Fixes filtering behavior where: - gh-pr-threads --thread baafe8 now displays the nitpick - gh-pr-threads --thread ca42b8 now displays threads within line ranges Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Code Review Fixes: - Add comprehensive unit tests for runReactCommand (12 test cases) - Refactor iteration to use for...of with Array.from() for compatibility - Make addReaction optional in mutation type for consistency - Extract duplicated reactionGroups GraphQL fields into shared constant - Fix formatReactionGroups to use indent parameter correctly - Add test coverage for custom indent parameter - Simplify author badge display (remove redundant "owner" label) - Add type annotation to test object for stronger validation - Add input trimming and ReactionType return type to normalizeReaction - Fix PRMeta interface to include totalAdditions/totalDeletions fields Test Coverage: - Added tests for batch operations, error handling, and edge cases - Added whitespace trimming test for reaction normalization - Total: 145 tests passing (+2 new tests) All changes maintain backward compatibility and pass typecheck, lint, and test suite. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Implemented Phase 2 of nested details parsing: - Import findBalancedDetails from parsers/nitpicks for balanced HTML parsing - Update parseDetailsBlocks to use findBalancedDetails instead of regex - Add recursive processing in formatDetailBlock for nested <details> blocks - Strip trailing newlines from code blocks in formatDiffBlock and highlightAndWrapCode This fixes thread 7cac6a where nested <details> tags (e.g., "🧰 Tools" with nested "🪛 GitHub Actions: CI" blocks) were displayed as raw HTML. Now all nested blocks are properly formatted with bold headers and correct indentation, supporting arbitrary nesting depth. Testing: - All 145 tests pass (no regressions) - Thread 7cac6a displays nested blocks correctly - No raw HTML tags in output - Proper visual hierarchy with indentation Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@src/core/dataFetcher.ts`:
- Around line 139-150: When short-circuiting (the branch that returns early when
targetThreadId || !shouldFetchFiles) you are currently returning a brand-new
empty cache which wipes out previously stored cursors; instead return the
existing cachedCursors (or updatedCursorCache) so prior pagination state is
preserved. Update the early-return in the files branch (and the analogous
early-returns for reviews/comments) to set cache to cachedCursors (or merge into
updatedCursorCache) and keep hadNewData false and nodes empty; reference the
existing variables cachedCursors and updatedCursorCache to locate and replace
the empty cache construction in those short-circuit branches.
- Around line 80-89: The current reduction over filesResult.nodes sets
totalAdditions/totalDeletions to 0 when files are skipped (e.g., when
shouldFetchFiles is false or targetThreadId is set); update the logic in the
block that builds PRMetadata (the metadata object created from
metadataWithoutFiles) so that when filesResult.nodes is empty because files were
intentionally not fetched you either (A) set totalAdditions and totalDeletions
to undefined / omit those fields on PRMetadata, or (B) perform a separate
GraphQL query to fetch overall additions/deletions and populate
totalAdditions/totalDeletions; change the code that computes
totalAdditions/totalDeletions (currently using filesResult.nodes.reduce) to
first detect the skip condition (inspect shouldFetchFiles and targetThreadId or
filesResult.nodes.length) and then follow option A or B, updating PRMetadata
accordingly (referencing symbols: totalAdditions, totalDeletions,
filesResult.nodes, shouldFetchFiles, targetThreadId, PRMetadata,
metadataWithoutFiles).
In `@src/core/threadFilter.ts`:
- Around line 22-47: The current logic using const [startLine, endLine] =
lineRange.split('-').map(Number) treats missing or invalid end parts (e.g.,
"10-" or "10-foo") as falsy and falls back to single-line behavior; update
parsing to split into raw parts (e.g., const parts = lineRange.split('-')),
parse startPart and endPart separately, validate with Number.isFinite for each
parsed value, and explicitly detect when a dash is present but endPart is
missing/invalid and return false (or treat the whole targetThreadId as
malformed) instead of treating it as a single-line; adjust the subsequent checks
that reference startLine/endLine/thread.line/path/targetThreadId accordingly so
range versus single-line is determined by presence and validity of the endPart
rather than simply truthiness.
In `@src/index.ts`:
- Around line 123-126: The code always writes prData.updatedCursorCache into
state.cursorCache after registerIds; change this to respect the noCache flag by
only assigning state.cursorCache = prData.updatedCursorCache when noCache is
falsy. Locate the block around registerIds(state, processedThreads, allNitpicks)
and the subsequent state.cursorCache update and wrap the assignment in a
conditional that checks the noCache variable so cache writes are skipped when
noCache is true; leave state.updatedAt and registerIds behavior unchanged.
In `@src/output/plainFormatter.ts`:
- Around line 11-18: The constant terminalWidth is using a fallback of 120 which
contradicts the “larger default” used in getTerminalWidth(); update
terminalWidth to match the larger default by either calling the helper (replace
const terminalWidth = process.stdout.columns || 120 with const terminalWidth =
getTerminalWidth()) or change its fallback to 150 so both getTerminalWidth() and
terminalWidth use the same default behavior; locate references to
getTerminalWidth and terminalWidth to ensure consistency.
- Around line 57-58: formatReactionGroups is emitting an extra quote bar because
wrapInQuote already prefixes one; change the returned lines in
formatReactionGroups to emit plain content with indent + 4 spaces (no "│" or
extra bar) so wrapInQuote produces the single quoted bar, and apply the same
change to the other similar return expressions referenced around lines 603-613;
update the return in formatReactionGroups (and the analogous returns) to remove
the "│" character and its surrounding spacing and instead output only indent
followed by four spaces then the icon and usersList/remaining content.
🧹 Nitpick comments (6)
src/utils/reactions.ts (1)
53-62: Consider a type-safe alternative toincludeswith type assertion.The
VALID_REACTIONS.includes(upper as ReactionType)assertion bypasses TypeScript's type checking. A type guard pattern would be more robust:🔧 Optional: Type-safe validation
+function isValidReaction(value: string): value is ReactionType { + return (VALID_REACTIONS as readonly string[]).includes(value); +} + export function normalizeReaction(input: string): ReactionType { const normalized = input.trim(); const upper = normalized.toUpperCase(); - if (VALID_REACTIONS.includes(upper as ReactionType)) { + if (isValidReaction(upper)) { return upper as ReactionType; }src/state/manager.ts (1)
22-25: Redundant assignment for missing property.Explicitly assigning
undefinedto a missing property is unnecessary since accessing a non-existent property already returnsundefined. However, if the intent is to ensure the property exists on the object (e.g., for consistent serialization orObject.keys()enumeration), this is acceptable.// These are equivalent for most purposes: if (!loaded.cursorCache) { loaded.cursorCache = undefined; // Property now exists but is undefined } // vs simply not having the property at allIf the goal is just backward compatibility for reading, this code can be removed. If it's for consistent object shape, consider adding a brief comment clarifying the intent.
src/core/nitpickFilter.ts (1)
31-33: Consider validating nitpick line parsing for robustness.The code validates
startLinefromtargetId(line 26-29) but doesn't validatenitpickStartparsed fromnitpick.line. If a nitpick has malformed line data (e.g., non-numeric), the comparison would silently fail to match.This is low-risk since malformed nitpicks would simply not match, but adding a debug log for NaN nitpick lines could help with troubleshooting:
🔧 Optional: Add debug for malformed nitpick lines
// Parse nitpick line (may be a range like "13-26" or single line "13") const nitpickLine = nitpick.line; const [nitpickStart, nitpickEnd] = nitpickLine.split('-').map(Number); + + if (Number.isNaN(nitpickStart)) { + debug(`Skipping nitpick with invalid line format: ${nitpickLine}`); + return false; + } debug(`Checking nitpick: path=${nitpick.path}, line=${nitpickLine} against path=${targetPath}, line=${lineRange}`);src/output/plainFormatter-suggestion.test.ts (1)
5-35: Avoid re-implementing the mapping logic in the testThis test re-parses the code block and remaps the language inline, so it can still pass if the production mapping changes or is removed. Consider exercising the actual formatter path (or extracting a shared helper) and asserting the mapped language there.
src/index.ts (1)
133-138: Use a Set to avoid O(n²) filtering
someinsidefilteris quadratic for large nitpick lists. Precompute a Set of IDs to keep this linear.Proposed refactor
- const botSummariesToDisplay = targetThreadId && nitpicksToDisplay.length > 0 && botSummaries.length > 0 + const nitpickIdSet = new Set(nitpicksToDisplay.map(n => n.id)); + const botSummariesToDisplay = targetThreadId && nitpickIdSet.size > 0 && botSummaries.length > 0 ? botSummaries.map(summary => ({ ...summary, - nitpicks: summary.nitpicks?.filter(n => nitpicksToDisplay.some(fn => fn.id === n.id)) + nitpicks: summary.nitpicks?.filter(n => nitpickIdSet.has(n.id)) })).filter(summary => summary.nitpicks && summary.nitpicks.length > 0) : botSummaries;src/output/plainFormatter.ts (1)
267-268: Use options objects for 4+ parameters.
formatSuggestionBlock(Line 267),formatDetailCodeBlock(Line 438), andformatThread(Line 561) exceed 3 params. Please convert these to options objects for readability and consistency. As per coding guidelines: Use options objects for functions with 4+ parameters.♻️ Example refactor (formatSuggestionBlock)
+interface FormatSuggestionBlockOptions { + code: string; + restText: string; + indent: string; + language?: string; +} + -function formatSuggestionBlock(code: string, restText: string, indent: string, language: string = 'typescript'): string[] { +function formatSuggestionBlock({ + code, + restText, + indent, + language = 'typescript' +}: FormatSuggestionBlockOptions): string[] { const lines: string[] = []; const bar = colors.dim('│'); @@ - lines.push(...formatSuggestionBlock(code, restText, indent, language)); + lines.push(...formatSuggestionBlock({ code, restText, indent, language }));
Multiple improvements based on CodeRabbit review feedback: - Add PR additions/deletions to metadata query to fix totals becoming 0 when files are skipped or --thread flag is used - Preserve existing pagination caches when short-circuiting to prevent cache erasure after targeted runs - Add explicit validation for malformed line ranges (e.g., "10-", "10-foo") - Respect noCache flag when persisting cursor cache to state - Add debug logging for invalid nitpick line formats - Use Set instead of Array.some() for O(n) nitpick filtering performance - Align terminalWidth constant with getTerminalWidth() default (150) - Remove duplicate quote bars from reaction formatting (let wrapInQuote handle it) - Add type-safe validation with type guard for reaction normalization All changes include test updates and maintain 100% test pass rate. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation