Recognize GitHub thread links with query strings or fragments - #1039
Recognize GitHub thread links with query strings or fragments#1039PeterDaveHello wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughGitHub route detection now uses shared pathname helpers. Navigation handling compares pathnames and skips unchanged paths. Tests cover valid thread paths, URL suffixes, navigation changes, and rejected routes. ChangesGitHub path matching
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
PR Summary by QodoRecognize GitHub issue/PR thread links with query, fragments, and trailing slash
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Greptile SummaryThis PR changes GitHub issue and pull-request recognition to use
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains within the eligible follow-up review scope.
|
| Filename | Overview |
|---|---|
| src/content-script/site-adapters/github/index.mjs | Integrates pathname-based thread recognition and pathname-only navigation refresh behavior. |
| src/content-script/site-adapters/github/path-matching.mjs | Defines reusable issue, pull-request, and pathname-change matching helpers. |
| tests/unit/content-script/github-path-matching.test.mjs | Covers trailing slashes, query strings, fragments, path changes, and excluded GitHub subpages. |
Reviews (2): Last reviewed commit: "Recognize GitHub thread links with query..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/content-script/site-adapters/github/index.mjs (1)
20-22: 📐 Maintainability & Code Quality | 🔵 TrivialRun and document the required validation.
This change affects content-script runtime behavior. Before merge, run
npm run lint,npm test, andnpm run build. Verify the expected build artifacts. Perform manual smoke tests in Chromium and Firefox. If a check is skipped, document the reason and the skipped check in the pull request description.As per coding guidelines, JavaScript runtime changes require these checks and documentation for skipped checks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/content-script/site-adapters/github/index.mjs` around lines 20 - 22, For the runtime changes around isPull and isIssue, run npm run lint, npm test, and npm run build; verify the expected build artifacts and manually smoke-test the behavior in Chromium and Firefox. Document the validation results in the pull request, including the specific check and reason for any skipped validation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/content-script/site-adapters/github/index.mjs`:
- Around line 20-22: For the runtime changes around isPull and isIssue, run npm
run lint, npm test, and npm run build; verify the expected build artifacts and
manually smoke-test the behavior in Chromium and Firefox. Document the
validation results in the pull request, including the specific check and reason
for any skipped validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d8c0bac3-3cac-4a5e-a74f-fe74c00874b7
📒 Files selected for processing (3)
src/content-script/site-adapters/github/index.mjssrc/content-script/site-adapters/github/path-matching.mjstests/unit/content-script/github-path-matching.test.mjs
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTo customize comments, go to the Qodo configuration screen, or learn more in the docs. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e331bef18
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Notification links and comment anchors add query or fragment data without changing the underlying issue or pull request path. Full-URL matching skips those supported pages, while treating every fragment change as navigation can unnecessarily discard an existing summary. Match and monitor pathname only, allow an optional trailing slash, and keep pull request subpages such as /files excluded.
8e331be to
3675ddf
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Introduces centralized GitHub pathname matching utilities and updates the GitHub content script to refresh only on meaningful navigation changes (pathname changes), with unit tests covering expected path behavior.
Changes:
- Added
isGitHubIssuePath,isGitHubPullPath, andhasGitHubPathChangedhelpers for GitHub URL/path detection. - Updated the GitHub site adapter to use
location.pathname(ignoring query/hash) when deciding whether to re-mount. - Added unit tests for trailing slashes, ignoring query/fragment, and excluding non-thread paths.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/unit/content-script/github-path-matching.test.mjs | Adds unit tests for GitHub path matching and navigation-change logic. |
| src/content-script/site-adapters/github/path-matching.mjs | New helper module encapsulating GitHub path regex matching and change detection. |
| src/content-script/site-adapters/github/index.mjs | Switches thread detection and refresh logic to use the new helpers and pathname-based comparison. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function isGitHubIssuePath(pathname) { | ||
| return issuePathPattern.test(pathname) | ||
| } |
Problem
GitHub notification links and comment anchors add query or fragment data without changing the underlying issue or pull request. The adapter matched the complete
location.hrefwith an end-anchored expression, so those otherwise supported thread links were not recognized.After switching recognition to
pathname, fragment-only changes also needed to be excluded from the polling remount path; otherwise clicking a comment permalink could discard an existing or in-progress summary.Changes
location.pathname./files,/commits, and/checks.Validation
node --test tests/unit/content-script/github-path-matching.test.mjs— 4 tests passed.node --checkpassed for the changed JavaScript modules.