Skip to content

Conversation

@jwm4
Copy link
Collaborator

@jwm4 jwm4 commented Jan 7, 2026

Summary

Implements Issue #5 - automatically collapse previous roadmap comments when posting a new one, similar to how Amber auto-review handles this.

Changes

review_roadmap/github/client.py

  • Added minimize_old_roadmap_comments() method that:
    • Fetches all issue comments on the PR
    • Filters for comments starting with the roadmap header prefix
    • Uses GitHub's GraphQL minimizeComment mutation with OUTDATED classifier
    • Returns (minimized_count, error_count) tuple

review_roadmap/main.py

  • Added ROADMAP_HEADER_PREFIX constant to identify roadmap comments
  • Updated format_pr_comment() to use the constant
  • Added minimize logic before posting (non-blocking on failure)
  • Shows count of collapsed comments in output

tests/test_github_client.py

  • Added 6 new tests covering all scenarios

Behavior

When using --post:

  1. All existing comments starting with 🗺️ **Auto-Generated Review Roadmap** are collapsed
  2. Comments from any user are collapsed (not just bots)
  3. Non-roadmap comments are never touched
  4. Minimize failures are non-blocking (warning shown, posting continues)
  5. Output: Collapsed N previous roadmap(s)

Testing

All 76 tests pass with 85% coverage.

Closes #5

Implements GitHub issue #5. When using --post, previous roadmap comments
are automatically minimized (collapsed) using GitHub's GraphQL API before
posting the new roadmap. This keeps PR conversations clean.

Changes:
- Add minimize_old_roadmap_comments() to GitHubClient
- Call minimize before posting in main.py
- Extract ROADMAP_HEADER_PREFIX constant for comment identification
- Add 6 tests for the new functionality

Behavior:
- All comments starting with the roadmap header are collapsed
- Comments from any user are collapsed (not just bots)
- Minimize failures are non-blocking (warnings shown, posting continues)
- Uses OUTDATED classifier for collapsed comments
@jwm4

This comment has been minimized.

@jwm4
Copy link
Collaborator Author

jwm4 commented Jan 7, 2026

🗺️ Auto-Generated Review Roadmap

This roadmap was automatically generated by review-roadmap.
Model: anthropic-vertex/claude-opus-4-5


🗺️ Review Roadmap: Auto-Collapse Old Roadmap Comments

High-Level Summary

This PR implements automatic cleanup of stale roadmap comments. When you post a new review roadmap using --post, the tool now finds all previous roadmap comments on the PR and collapses them using GitHub's GraphQL minimizeComment mutation with the OUTDATED classifier. This prevents comment clutter as the PR evolves through multiple review cycles.

The implementation adds a new GraphQL-based method to the GitHub client, wires it into the main posting workflow, and includes comprehensive test coverage.


Recommended Review Order

Review in dependency order — understand the core API addition first, then the integration, then validate tests.

1️⃣ GitHub Client — Core Feature (client.py)

File: review_roadmap/github/client.py

This is the heart of the feature. The new minimize_old_roadmap_comments() method:

  • Fetches all issue comments via REST API
  • Filters for comments starting with the roadmap header
  • Calls GitHub's GraphQL endpoint for each matching comment
Focus Area What to Verify
GraphQL mutation structure Check the mutation string and variable binding
Node ID usage Critical: Ensure it uses node_id from comments, not numeric id
Error handling Verify individual failures don't abort the entire operation
Return semantics Confirm (minimized_count, error_count) tuple is correctly populated

2️⃣ Main Orchestration — Integration (main.py)

File: review_roadmap/main.py

Focus Area Where to Look
Constant definition ROADMAP_HEADER_PREFIX at line 14 — verify it matches what's used in format_pr_comment()
Updated comment formatting Lines 24-36 — confirm the constant is used consistently
Minimize call placement Should happen before posting, with non-blocking error handling
User feedback Check the "Collapsed N previous roadmap(s)" output logic

3️⃣ Test Suite — Validation (test_github_client.py)

File: tests/test_github_client.py

The PR adds 6 new tests. Skim for coverage of:

Scenario What to Check
Happy path Multiple roadmaps found and minimized, returns (N, 0)
No matches Non-roadmap comments present, returns (0, 0)
Empty state No comments at all, returns (0, 0)
Partial failure Some mutations fail, error_count increments correctly
GraphQL mocking Mocked responses should match GitHub's actual GraphQL schema

⚠️ Watch Outs

1. Node ID vs Numeric ID

The GraphQL minimizeComment mutation requires a global node ID (e.g., MDEyOklzc3VlQ29tbWVudDEyMzQ=), not the REST API's numeric ID. Verify the code extracts node_id from the comment response.

2. Prefix Matching Edge Cases

The filtering uses body.startswith(header_prefix). Consider:

  • Comments with leading whitespace or newlines
  • Unicode normalization issues with the 🗺️ emoji
  • Case sensitivity (though emojis don't have case)

3. GraphQL Authentication

The client is configured for REST API. Verify the same auth headers work for the GraphQL endpoint (https://api.github.com/graphql) or if additional setup is needed.

4. Permission Model

The description says "Comments from any user are collapsed" — verify:

  • Does the token need special permissions to minimize others' comments?
  • Is there test coverage for permission denied scenarios?

5. Constant Duplication Risk

ROADMAP_HEADER_PREFIX is defined in main.py but passed to client.py. If client.py ever needs this constant independently, there's drift risk. Acceptable for now, but worth noting.


Existing Discussions Summary

The existing comment on this PR is itself a roadmap output (meta!). It calls out the same concerns I've noted:

  • Node ID vs numeric ID verification
  • Prefix matching robustness
  • Non-blocking failure handling
  • Missing permission error test coverage

This suggests these areas deserve extra scrutiny during your review.


Quick Review Checklist

  • node_id is used for GraphQL mutations (not numeric id)
  • ROADMAP_HEADER_PREFIX constant is used consistently in both files
  • Minimize failures log warnings but don't prevent posting
  • Test mocks accurately represent GitHub's GraphQL response structure
  • No hardcoded header strings outside the constant

@jwm4 jwm4 merged commit 1d5d4d9 into main Jan 11, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Accordion collapse

2 participants