Add PostHog thumbs up/down feedback links to PR review comments#1714
Add PostHog thumbs up/down feedback links to PR review comments#1714
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
There was a problem hiding this comment.
Good implementation overall, but there are some important error handling and robustness issues to address before merging. The core functionality looks solid, but needs better exception handling and test coverage.
| try: | ||
| json.loads(text) | ||
| return text | ||
| except Exception: |
There was a problem hiding this comment.
🟠 Important: Using bare except Exception is too broad and can hide bugs. Be more specific about what exceptions you're catching.
| except Exception: | |
| except json.JSONDecodeError: |
This makes it clear you're only catching JSON parsing errors, not other unexpected exceptions like KeyboardInterrupt or SystemExit.
| ReviewComment( | ||
| path=path, | ||
| side=side, | ||
| line=int(line), |
There was a problem hiding this comment.
🟠 Important: Converting line to int without validation could raise ValueError if the agent returns invalid data (e.g., a float or string that isn't a number).
Consider adding try-except or validation:
| line=int(line), | |
| line=int(line) if isinstance(line, (int, str)) else line, |
| except Exception as e: | ||
| logger.error(f"Failed to parse agent JSON output: {e}") | ||
| event = "COMMENT" | ||
| raw = review_content.strip() | ||
| if len(raw) > 20000: | ||
| raw = raw[:20000] + "\n\n...[truncated]..." | ||
|
|
||
| review_body = append_feedback_links( | ||
| "Automated review output could not be parsed into structured comments. " | ||
| "Posting raw output instead.\n\n" + raw, | ||
| base_url=feedback_base_url, | ||
| repo=repo, | ||
| pr_number=pr_number, | ||
| run_id=run_id, | ||
| comment_key="summary", | ||
| ) | ||
| gh_comments = [] |
There was a problem hiding this comment.
🟡 Suggestion: The fallback behavior truncates at 20,000 characters, which could cut off in the middle of important information. Consider truncating at a newline boundary or providing a better summary.
Also, the broad except Exception here could hide unexpected errors. Consider catching more specific exceptions or at least logging the full traceback for debugging.
| (function(t,e){var o,n,p,r;e.__SV=1;window.posthog=e;e._i=[];e.init=function(i,s,a){function g(t,e){var o=e.split(".");2==o.length&&(t=t[o[0]],e=o[1]);t[e]=function(){t.push([e].concat(Array.prototype.slice.call(arguments,0)))}}(p=t.createElement("script")).type="text/javascript";p.async=!0;p.src=s.api_host+"/static/array.js";(r=t.getElementsByTagName("script")[0]).parentNode.insertBefore(p,r);var u=e;for(void 0!==a?u=e[a]=[]:a="posthog",u.people=u.people||[],u.toString=function(t){var e="posthog";return"posthog"!==a&&(e+="."+a),t||(e+=" (stub)"),e},u.people.toString=function(){return u.toString(1)+".people (stub)"},o="capture identify alias people.set people.set_once set_config register register_once unregister opt_out_capturing opt_in_capturing has_opted_out_capturing reset get_distinct_id debug on off".split(" "),n=0;n<o.length;n++)g(u,o[n]);e._i.push([i,s,a])}; | ||
| })(document,window.posthog||[]); | ||
|
|
||
| posthog.init('phc_QkAtbXVsh3Ja0Pw4IK696cxYEmr20Bx1kbnI7QtOCqg', { |
There was a problem hiding this comment.
🟢 Nit: While PostHog project API keys (starting with phc_) are designed to be public and safe to use client-side, it would be helpful to add a comment explaining this to prevent future confusion.
| posthog.init('phc_QkAtbXVsh3Ja0Pw4IK696cxYEmr20Bx1kbnI7QtOCqg', { | |
| // Public PostHog project key - safe to expose client-side | |
| posthog.init('phc_QkAtbXVsh3Ja0Pw4IK696cxYEmr20Bx1kbnI7QtOCqg', { |
| function ensureDistinctId() { | ||
| const key = 'oh_review_feedback_distinct_id'; | ||
| let id = localStorage.getItem(key); | ||
| if (!id) { | ||
| id = (crypto && crypto.randomUUID) ? crypto.randomUUID() : String(Date.now()) + '-' + Math.random().toString(16).slice(2); | ||
| localStorage.setItem(key, id); | ||
| } | ||
| return id; |
There was a problem hiding this comment.
🟠 Important: localStorage can fail in browsers with strict privacy settings or in incognito/private mode. This will cause the entire tracking to fail.
Add try-catch error handling:
| function ensureDistinctId() { | |
| const key = 'oh_review_feedback_distinct_id'; | |
| let id = localStorage.getItem(key); | |
| if (!id) { | |
| id = (crypto && crypto.randomUUID) ? crypto.randomUUID() : String(Date.now()) + '-' + Math.random().toString(16).slice(2); | |
| localStorage.setItem(key, id); | |
| } | |
| return id; | |
| function ensureDistinctId() { | |
| try { | |
| const key = 'oh_review_feedback_distinct_id'; | |
| let id = localStorage.getItem(key); | |
| if (!id) { | |
| id = (crypto && crypto.randomUUID) ? crypto.randomUUID() : String(Date.now()) + '-' + Math.random().toString(16).slice(2); | |
| localStorage.setItem(key, id); | |
| } | |
| return id; | |
| } catch (e) { | |
| // Fallback if localStorage is unavailable | |
| return (crypto && crypto.randomUUID) ? crypto.randomUUID() : String(Date.now()) + '-' + Math.random().toString(16).slice(2); | |
| } |
| document.getElementById('line').textContent = line || '(n/a)'; | ||
|
|
||
| const distinctId = ensureDistinctId(); | ||
| posthog.identify(distinctId); |
There was a problem hiding this comment.
🟠 Important: If PostHog fails to load or initialize (network issues, ad blockers, etc.), this will throw an error. Add error handling:
| posthog.identify(distinctId); | |
| try { | |
| const distinctId = ensureDistinctId(); | |
| posthog.identify(distinctId); | |
| } catch (e) { | |
| console.warn('PostHog identification failed:', e); | |
| } |
|
|
||
| github_token = os.getenv("GITHUB_TOKEN") | ||
| github_token = _get_required_env("GITHUB_TOKEN") | ||
| run_id = os.getenv("GITHUB_RUN_ID") or uuid.uuid4().hex |
There was a problem hiding this comment.
🟡 Suggestion: If GITHUB_RUN_ID is not set (e.g., local testing), using uuid.uuid4().hex is good, but consider logging this for debugging purposes so it's clear when feedback is from a local run vs CI.
| @dataclass(frozen=True) | ||
| class ReviewComment: | ||
| path: str | ||
| side: str | ||
| line: int | ||
| body: str | ||
| start_line: int | None = None | ||
| start_side: str | None = None |
There was a problem hiding this comment.
🔴 Critical: This PR adds significant new functionality but includes no tests. At minimum, add tests for:
_extract_json_object()with various input formatsparse_review_response()with valid and invalid JSONbuild_feedback_url()with different parametersappend_feedback_links()output format
The lack of tests makes it difficult to verify correctness and prevents regressions.
| if (repo && pr && runId && commentKey && (rating === 'up' || rating === 'down') && !alreadySent(dedupeKey)) { | ||
| posthog.capture('pr_review_feedback', { | ||
| repo, | ||
| pr_number: pr, | ||
| run_id: runId, | ||
| comment_key: commentKey, | ||
| rating, | ||
| path, | ||
| line, | ||
| return_to: returnTo | ||
| }); | ||
| markSent(dedupeKey); | ||
| } |
There was a problem hiding this comment.
🟠 Important: If PostHog capture fails, it should be caught to prevent breaking the page functionality:
| if (repo && pr && runId && commentKey && (rating === 'up' || rating === 'down') && !alreadySent(dedupeKey)) { | |
| posthog.capture('pr_review_feedback', { | |
| repo, | |
| pr_number: pr, | |
| run_id: runId, | |
| comment_key: commentKey, | |
| rating, | |
| path, | |
| line, | |
| return_to: returnTo | |
| }); | |
| markSent(dedupeKey); | |
| } | |
| const dedupeKey = [repo, pr, runId, commentKey, rating].join('|'); | |
| if (repo && pr && runId && commentKey && (rating === 'up' || rating === 'down') && !alreadySent(dedupeKey)) { | |
| try { | |
| posthog.capture('pr_review_feedback', { | |
| repo, | |
| pr_number: pr, | |
| run_id: runId, | |
| comment_key: commentKey, | |
| rating, | |
| path, | |
| line, | |
| return_to: returnTo | |
| }); | |
| markSent(dedupeKey); | |
| } catch (e) { | |
| console.warn('Failed to send feedback to PostHog:', e); | |
| } | |
| } |
|
@OpenHands review diff against main. Do you think if it is possible to get the feedback mechanism to work WITHOUT deploying a page? |
|
I'm on it! xingyaoww can track my progress at all-hands.dev |
SummaryI reviewed the diff for PR #1714 which adds PostHog thumbs up/down feedback links to PR review comments. Answer to your question: Can the feedback mechanism work WITHOUT deploying a page?Short answer: Technically yes, but the GitHub Pages approach is the best solution. Why a deployed page is needed:
Alternatives without GitHub Pages:
Why the GitHub Pages approach in this PR is ideal:
Recommendation: Keep the GitHub Pages implementation unless you have a specific requirement to avoid it. If you must avoid it, use a serverless function (Vercel/Cloudflare), but that's more complex and requires maintaining a separate service. No changes were made to the codebase - I only answered your question. |
- Relocate feedback/index.html to .github/pages/feedback/index.html - Update GitHub Actions workflow to deploy from new location - Keeps feedback page hidden in .github directory while maintaining functionality Co-authored-by: openhands <openhands@all-hands.dev>
|
Looks like there are a few issues preventing this PR from being merged!
If you'd like me to help, just leave a comment, like Feel free to include any additional details that might help me get this PR into a better state. You can manage your notification settings |
|
[Automatic Post]: It has been a while since there was any activity on this PR. @xingyaoww, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. |
|
the lamniar evaluation seems like a better idea so close this |
Summary
Adds PostHog-backed thumbs up/down feedback links to every OpenHands PR review comment.
agent_script.pyso feedback links are always appended./feedback/index.html) that capturespr_review_feedbackin PostHog.Checklist
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.12-nodejs22golang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:0c888f2-pythonRun
All tags pushed for this build
About Multi-Architecture Support
0c888f2-python) is a multi-arch manifest supporting both amd64 and arm640c888f2-python-amd64) are also available if needed