feat(error-tracking): scroll a frame's file at the release commit - #107636
ablaszkiewicz wants to merge 1 commit into
Conversation
🤖 CI report🚨 Trunk lane — universal laneThis PR is assigned to the universal lane. It cannot merge in parallel with other PRs, so it can take longer to merge. Ask dev-ex if you think this is wrong. ✅ Duplication (Python) — cleanNew Python code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying. ✅ Duplication (TypeScript) — cleanNew TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.
|
| File | Size | Δ vs base |
|---|---|---|
render-query/src/render-query/render-query.js |
20.26 MiB | 🔺 +4.3 KiB (+0.0%) |
posthog-app/src/scenes/AuthenticatedShell.js |
248.4 KiB | 🔺 +2.0 KiB (+0.8%) |
Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report
✅ Eager graph — within budget
How much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy import() / React.lazy chunks are not counted.
| Root | Eager (shipped) | Δ vs base | Budget |
|---|---|---|---|
entry (logged-out pages, app bootstrap)src/index.tsx |
1.57 MiB · 22 files | no change | █████████░ 85.5% of 1.84 MiB |
logged-out boot: index + App + bootApp (preloaded by every page, including /login)src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts |
3.56 MiB · 629 files | 🔺 +314 B (+0.0%) | █████████░ 88.4% of 4.03 MiB |
authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
7.38 MiB · 2,332 files | 🔺 +7.9 KiB (+0.1%) | █████████░ 88.4% of 8.34 MiB |
🟢 node_modules/monaco-editor/ stays out of src/index.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 node_modules/monaco-editor/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/layout/navigation-3000/navigationLogic.tsx stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/scenes/dashboard/dashboardLogic.tsx stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/lemon-ui/LemonMarkdown/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/components/RichContentEditor/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/components/CodeSnippet/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/taxonomy/core-filter-definitions-by-group.json stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 node_modules/monaco-editor/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/scenes/session-recordings/player/sessionRecordingPlayerLogic.ts stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
Largest files eagerly shipped from src/index.tsx
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/index.tsx |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 854 B | src/scenes/ChunkLoadErrorBoundary.tsx |
Largest files eagerly shipped from src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
| Size | File |
|---|---|
| 301.8 KiB | ../node_modules/.pnpm/posthog-js@1.434.14_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 267.6 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 99.9 KiB | src/lib/api.ts |
| 88.4 KiB | src/products.tsx |
| 69.4 KiB | src/lib/lemon-ui/icons/icons.tsx |
| 40.1 KiB | src/lib/utils/eventUsageLogic.ts |
| 38.7 KiB | ../node_modules/.pnpm/@dnd-kit+core@6.0.8_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@dnd-kit/core/dist/core.esm.js |
| 33.9 KiB | ../node_modules/.pnpm/kea@4.0.0-pre.6_patch_hash=139b8d1f1304f9d9da452a9a1244c94ea679dbcb85687d8999563146879fb6f5_react@18.3.1/node_modules/kea/lib/index.cjs.js |
| 28.4 KiB | src/scenes/scenes.ts |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 301.8 KiB | ../node_modules/.pnpm/posthog-js@1.434.14_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 271.7 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 267.6 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 153.7 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 99.9 KiB | src/lib/api.ts |
| 98.5 KiB | ../packages/quill/packages/quill/dist/index.js |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
| 88.4 KiB | src/products.tsx |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Toolbar bundle — eager 2.38 MiB within budget
What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.
| Metric | Size | Δ vs base | Budget |
|---|---|---|---|
| Eager (shipped) entry + static imports |
2.38 MiB · 19 files | no change | ████░░░░░░ 41.5% of 5.72 MiB |
| Deferred (lazy) | 2.10 MiB · 44 files | no change | n/a — loads on demand |
Loader dist/toolbar.js |
1.2 KiB | no change | █░░░░░░░░░ 6.0% of 19.5 KiB |
Largest eagerly-shipped chunks
| Size | File |
|---|---|
| 800.0 KiB | dist/toolbar/toolbar-app-6DYXIWRU.css |
| 651.4 KiB | dist/toolbar/chunk-chunk-TVTQKACM.js |
| 483.6 KiB | dist/toolbar/chunk-chunk-6JFSEK3E.js |
| 138.3 KiB | dist/toolbar/chunk-chunk-CP4QT72J.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-FDH2IBXT.js |
| 75.2 KiB | dist/toolbar/toolbar-app-OHYAIWFE.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-TSAL54PB.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-577JVGVT.js |
| 21.0 KiB | dist/toolbar/chunk-chunk-GNB7IYR7.js |
| 6.8 KiB | dist/toolbar/chunk-chunk-DV7IWQNF.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — 🔺 +194.5 KiB (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 946.37 MiB · 🔺 +194.5 KiB (+0.0%)
ℹ️ MCP UI apps size — 33 app(s), 17631.0 KB JS
Built size of each MCP UI app (main.js + styles.css).
| App | JS | CSS |
|---|---|---|
| debug | 597.9 KB | 196.2 KB |
| action | 454.1 KB | 196.2 KB |
| action-list | 564.2 KB | 196.2 KB |
| cohort | 453.1 KB | 196.2 KB |
| cohort-list | 563.2 KB | 196.2 KB |
| email-template | 452.9 KB | 196.2 KB |
| error-details | 469.6 KB | 196.2 KB |
| error-issue | 453.8 KB | 196.2 KB |
| error-issue-list | 564.1 KB | 196.2 KB |
| experiment | 561.3 KB | 196.2 KB |
| experiment-list | 564.9 KB | 196.2 KB |
| experiment-results | 566.3 KB | 196.2 KB |
| feature-flag | 566.8 KB | 196.2 KB |
| feature-flag-list | 570.5 KB | 196.2 KB |
| feature-flag-testing | 457.3 KB | 196.2 KB |
| inline-scan | 453.6 KB | 196.2 KB |
| insight-actors | 562.3 KB | 196.2 KB |
| invite-email-preview | 452.3 KB | 196.2 KB |
| llm-costs | 559.3 KB | 196.2 KB |
| session-recording | 455.3 KB | 196.2 KB |
| survey | 454.7 KB | 196.2 KB |
| survey-global-stats | 561.9 KB | 196.2 KB |
| survey-list | 564.9 KB | 196.2 KB |
| survey-stats | 561.9 KB | 196.2 KB |
| trace-span | 453.5 KB | 196.2 KB |
| trace-span-list | 564.1 KB | 196.2 KB |
| vision-observation-list | 563.3 KB | 196.2 KB |
| workflow | 453.4 KB | 196.2 KB |
| workflow-list | 563.5 KB | 196.2 KB |
| loops-review | 457.8 KB | 196.2 KB |
| query-results | 774.1 KB | 196.2 KB |
| render-ui | 857.4 KB | 196.2 KB |
| visual-review-snapshots | 457.9 KB | 196.2 KB |
|
[Medium risk] Adds source file viewing for error stack frames. This PR is not safe to merge until cached reads honor access revocation and production events provide the required repository path. Reviews (1) · Last reviewed commit: "feat(error-tracking): scroll through a f..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if isinstance(cached, bytes): | ||
| return zstd.decompress(cached).decode() |
There was a problem hiding this comment.
If a team removes its GitHub integration or loses access to a repository after a file is cached, this branch returns the private source without checking current access. Integration removal and repository-access changes do not clear these cache entries, so the endpoint can keep serving the file for up to 24 hours. Recheck access before returning cached content, or invalidate the entries when access changes.
How this was verified: Cached bytes return before the live integration and repository-access check, and the access-change handlers do not clear this cache.
Prompt To Fix With AI
This is a comment left during a code review.
Path: products/error_tracking/backend/logic/repo_files/source_file.py
Line: 162-163
Comment:
**Cached source survives access revocation**
If a team removes its GitHub integration or loses access to a repository after a file is cached, this branch returns the private source without checking current access. Integration removal and repository-access changes do not clear these cache entries, so the endpoint can keep serving the file for up to 24 hours. Recheck access before returning cached content, or invalidate the entries when access changes.
**How this was verified:** Cached bytes return before the live integration and repository-access check, and the access-change handlers do not clear this cache.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| frames = stacktrace.get("frames") if isinstance(stacktrace, dict) else None | ||
| for frame in frames if isinstance(frames, list) else []: | ||
| if isinstance(frame, dict) and frame.get("raw_id") == frame_raw_id: | ||
| repo_path = frame.get("repo_path") |
There was a problem hiding this comment.
This reads repo_path from the stored exception frame, but the production frame types written to $exception_list do not contain that field. The successful test supplies it by hand, so real processed frames return no_repo_path instead of loading the file. Persist the path in the frame writer before relying on it here, and test with a writer-produced event.
Prompt To Fix With AI
This is a comment left during a code review.
Path: products/error_tracking/backend/logic/repo_files/source_file.py
Line: 146
Comment:
**Stored frames lack repo paths**
This reads `repo_path` from the stored exception frame, but the production frame types written to `$exception_list` do not contain that field. The successful test supplies it by hand, so real processed frames return `no_repo_path` instead of loading the file. Persist the path in the frame writer before relying on it here, and test with a writer-produced event.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| except (EgressBudgetExhausted, GitHubRateLimitError): | ||
| return _Unavailable(reason="rate_limited") | ||
| except GitHubIntegrationError: | ||
| return _Unavailable(reason="fetch_failed") |
There was a problem hiding this comment.
For a source file that is not UTF-8, get_file_entry raises UnicodeDecodeError, which these handlers do not catch. Expanding that frame returns a 500 instead of leaving the captured context in place. Handle the decoding failure as an unavailable file.
Prompt To Fix With AI
This is a comment left during a code review.
Path: products/error_tracking/backend/logic/repo_files/source_file.py
Line: 182-185
Comment:
**Non-UTF-8 files cause errors**
For a source file that is not UTF-8, `get_file_entry` raises `UnicodeDecodeError`, which these handlers do not catch. Expanding that frame returns a 500 instead of leaving the captured context in place. Handle the decoding failure as an unavailable file.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 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:
📝 WalkthroughWalkthroughThe backend now retrieves source files for stored error frames from GitHub at the release commit and exposes them through a read API. The frontend selects eligible frames, loads repository source, checks it against captured code, and displays a scrollable source window with fallback states. Backend and frontend tests and a Storybook story cover the retrieval and display paths. Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to The new inline source view builds GitHub requests from file paths stored in events. It does not encode or validate those paths, so a crafted path can change which GitHub API URL is called with the team's token. Some files can also produce server errors instead of an "unavailable" state. The previously reported typegen and error-handling issues are still open. Resolve these before merging. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The new endpoint can return a full repository file to an authorized team user. Its cache can serve a previously fetched file without checking current repository access, and a stored frame path reaches an authenticated GitHub request without file-path validation. Team-scoped lookups and integration checks limit exposure, but revocation and path-handling questions remain. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (4)
products/error_tracking/backend/logic/repo_files/source_file.py-151-151 (1)
151-151: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winValidate
release.idbefore UUID conversion.If an event contains a numeric or structured
release.id, line 151 passes it to_as_uuid.uuid.UUIDcan raiseAttributeErrorfor these values, but_as_uuidcatches onlyValueError. The request can therefore fail with 500 instead of returningno_release.Suggested fix
- release_id=release.get("id") if isinstance(release, dict) else None, + release_id=release.get("id") + if isinstance(release, dict) and isinstance(release.get("id"), str) + else None,products/error_tracking/backend/logic/repo_files/source_file.py-181-185 (1)
181-185: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle non-UTF-8 source files as unavailable.
GitHubIntegration.get_file_entrydecodes inline content with UTF-8. A non-UTF-8 file raisesUnicodeDecodeError, which_fetch_filedoes not catch. The exception can escape instead of returning_Unavailable.Suggested fix
except (EgressBudgetExhausted, GitHubRateLimitError): return _Unavailable(reason="rate_limited") + except UnicodeDecodeError: + return _Unavailable(reason="fetch_failed") except GitHubIntegrationError: return _Unavailable(reason="fetch_failed")frontend/src/lib/components/Errors/Frame/frameSourceFileLogic.ts-110-125 (1)
110-125: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReturn
nullonly on 404. Rethrow every other failure.The bare
catchreturnsnullfor every error: 5xx responses, network failures, and GitHub rate limits. The UI then shows "The source of this frame isn't available." and reports nothing. Checkstatus === 404and rethrow any other error, so the loader's failure path and error tracking still run.frontend/src/lib/components/Errors/Frame/FrameSourceFile.tsx-59-61 (1)
59-61: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winGuard
showMoreBelowagainst repeated dispatches.Each scroll event within
EDGE_PXof the bottom dispatchesshowMoreBelow. The above branch has a guard. This branch has none. The window can therefore grow by several hundred lines during one scroll gesture before React renders the new rows. Add a pending ref, as in the above branch, and clear it whenafter.lengthchanges.
🧹 Nitpick comments (1)
products/error_tracking/backend/tests/api/test_git_provider_file_links.py (1)
176-192: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePatch
installation_can_access_repositoryin the 404 cases.The parameterized 404 test does not mock
installation_can_access_repository. The test passes today because each case fails before the access check runs. If the resolver order changes, the test can reach the real GitHub call and produce a 404 for a different reason. Mock the access check withreturn_value=Trueso that each 404 comes from the stored frame or release.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 3e26aa62-51f8-432d-9b91-d2513fc03d04
⛔ Files ignored due to path filters (2)
products/error_tracking/frontend/generated/api.schemas.tsis excluded by!**/generated/**products/error_tracking/frontend/generated/api.tsis excluded by!**/generated/**
📒 Files selected for processing (15)
.github/new-events-schema-targets.txtfrontend/src/lib/components/Errors/Frame/CollapsibleFrame.stories.tsxfrontend/src/lib/components/Errors/Frame/CollapsibleFrameContent.tsxfrontend/src/lib/components/Errors/Frame/CollapsibleFrameHeader.tsxfrontend/src/lib/components/Errors/Frame/FrameSourceFile.tsxfrontend/src/lib/components/Errors/Frame/frameSourceFileLogic.test.tsfrontend/src/lib/components/Errors/Frame/frameSourceFileLogic.tsfrontend/src/lib/components/Errors/Frame/sourceFileWindow.test.tsfrontend/src/lib/components/Errors/Frame/sourceFileWindow.tsfrontend/src/lib/components/Errors/errorPropertiesLogic.tsproducts/error_tracking/backend/logic/repo_files/metrics.pyproducts/error_tracking/backend/logic/repo_files/source_file.pyproducts/error_tracking/backend/presentation/views/git_provider_file_link_resolver.pyproducts/error_tracking/backend/tests/api/test_git_provider_file_links.pyservices/mcp/src/api/generated.ts
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 1 remain after this review.
| fileWindow: ( | ||
| sourceFile: FrameSourceFileResponseApi | null, | ||
| linesAbove: 15, | ||
| linesBelow: 15 | ||
| ) => SourceFileWindow | null |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Regenerate the kea types. The committed linesAbove/linesBelow types are stale.
Frontend typechecking fails because typegen output differs from the committed file. Typegen produces number for these selector inputs. The committed file declares the literal type 15.
Fix
- linesAbove: 15,
- linesBelow: 15
+ linesAbove: number,
+ linesBelow: number📝 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.
| fileWindow: ( | |
| sourceFile: FrameSourceFileResponseApi | null, | |
| linesAbove: 15, | |
| linesBelow: 15 | |
| ) => SourceFileWindow | null | |
| fileWindow: ( | |
| sourceFile: FrameSourceFileResponseApi | null, | |
| linesAbove: number, | |
| linesBelow: number | |
| ) => SourceFileWindow | null |
🧰 Tools
🪛 GitHub Actions: Frontend CI / Frontend typechecking
[error] 78-79: The schema build command generated changes to this file; git diff --exit-code failed because the working tree is not clean.
Source: Pipeline failures
| return f"error_tracking:source_file:v1:{team_id}:{digest}" | ||
|
|
||
|
|
||
| def _json(value: Any) -> Any: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove Any from the JSON boundary.
_json(value: Any) -> Any weakens type checking for values read from the event record. Use object for the input and result, then narrow each decoded shape at its use site. As per coding guidelines: “Write as if mypy --strict were on. Annotate every signature, avoid Any.”
Source: Coding guidelines
| }, | ||
| ) | ||
| @action(methods=["GET"], detail=False, url_path="source_file") | ||
| def source_file(self, request, **kwargs): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Annotate the new view action.
source_file has no parameter or return annotations. Add types for request, **kwargs, and the response. As per coding guidelines: “Write as if mypy --strict were on. Annotate every signature.”
Source: Coding guidelines
|
|
b72af04 to
2fa9eff
Compare
2fa9eff to
7679de6
Compare
7679de6 to
76a580a
Compare
…mmit An expanded in-app frame with a repo path shows its file from GitHub at the release commit in a fixed-height box centered on the frame line, and shows more lines as you scroll to either edge. The server reads the file path, line, repository and commit from the stored event and its release, checks the file against the captured code line, and caches files for a day. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
76a580a to
b0307ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
products/error_tracking/backend/logic/repo_paths/source_file.py-181-185 (1)
181-185: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle source files that are not UTF-8.
If GitHub returns a source file encoded in another character set,
get_file_entryraisesUnicodeDecodeErrorwhile decoding its bytes. This handler catches onlyGitHubIntegrationError, so the source-file endpoint returns a 500 instead of an unavailable result. Catch the decoding failure at the retrieval boundary and return a defined unavailable reason. (raw.githubusercontent.com)
🧹 Nitpick comments (1)
products/error_tracking/backend/logic/repo_paths/source_file.py (1)
199-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
objectat the JSON boundary instead ofAny.
_jsonnarrows its input withisinstance, and its callers narrow the result again. Annotate the parameter and return value asobjectso later unchecked field access does not bypass type checking. As per coding guidelines: “Write as if mypy--strictwere on. Annotate every signature, avoidAny.”Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: e22201df-95ef-4da5-9071-fca2351fe4e2
⛔ Files ignored due to path filters (2)
products/error_tracking/frontend/generated/api.schemas.tsis excluded by!**/generated/**products/error_tracking/frontend/generated/api.tsis excluded by!**/generated/**
📒 Files selected for processing (3)
products/error_tracking/backend/logic/repo_paths/metrics.pyproducts/error_tracking/backend/logic/repo_paths/source_file.pyproducts/error_tracking/backend/presentation/views/git_provider_file_link_resolver.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 9 remain after this review.
| ) | ||
| if github is None: | ||
| return _Unavailable(reason="no_integration") | ||
| entry = github.get_file_entry(repo.slug.path, repo_path, ref=repo.commit) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🛡️ Detected with Advanced Tier | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
git diff --no-ext-diff --unified=80 7ba6066981569fa829db03b5159107c52654349b b0307ee482138f47f86d740e75328489d41d429b -- products/error_tracking/backend/logic/repo_paths/source_file.py
printf '\n--- source_file ---\n'
sed -n '1,230p' products/error_tracking/backend/logic/repo_paths/source_file.py
printf '\n--- get_file_entry ---\n'
rg -n -A35 -B12 'def get_file_entry|file_path' posthog/models/github.py posthog/models/github_integration_base.py posthog/egress/github products/error_tracking/backend | head -260
printf '\n--- request helpers ---\n'
rg -n -A35 -B12 'def client_request|urljoin|api.github|repos/' posthog/models/github.py posthog/models/github_integration_base.py posthog/egress/github | head -300Repository: PostHog/posthog
Length of output: 41582
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- get_file_entry locations ---'
rg -n -l 'get_file_entry|def api_request' posthog products
printf '%s\n' '--- get_file_entry context ---'
rg -n -A45 -B20 'def get_file_entry' posthog products
printf '%s\n' '--- api_request context ---'
rg -n -A80 -B20 'def api_request' posthog/models posthog/egressRepository: PostHog/posthog
Length of output: 16512
🏁 Script executed:
#!/bin/bash
set -e
sed -n '1,45p' posthog/models/integration/github.py
sed -n '930,978p' posthog/models/integration/github.pyRepository: PostHog/posthog
Length of output: 4245
🏁 Script executed:
#!/bin/bash
set -e
rg -n -A35 -B15 'def _is_safe_github_repo_path|_is_safe_github_repo_path\(' posthog/models/github_integration_base.py posthog/models/integration/github.py posthog products/error_tracking/backend/testsRepository: PostHog/posthog
Length of output: 42624
Path Traversal
Reachability: External
Exploitability: Moderate
CWE: CWE-22 — Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal')
Encode and validate file_path before building the GitHub contents URL.
repo_path comes from the stored event frame. get_file_entry inserts it directly into the authenticated request path, and api_request performs no URL encoding or traversal validation. A path containing dot segments can escape the intended contents path when normalized. ? and # can also alter URL parsing. The repository check validates only repo.slug.path.
Suggested fix
diff --git a/posthog/models/integration/github.py b/posthog/models/integration/github.py
--- a/posthog/models/integration/github.py
+++ b/posthog/models/integration/github.py
@@
import re
import time
import base64
from collections.abc import Mapping
from dataclasses import dataclass, field
from datetime import datetime
from typing import Any
+from urllib.parse import quote
@@
"""
repo_path = repository if "/" in repository else f"{self.organization()}/{repository}"
+ if file_path.startswith("/") or any(part in {".", ".."} for part in file_path.split("/")):
+ raise GitHubIntegrationError(f"Unsafe file path: {file_path!r}")
+ encoded_file_path = quote(file_path, safe="/")
response = self.api_request(
"GET",
- f"/repos/{repo_path}/contents/{file_path}",
+ f"/repos/{repo_path}/contents/{encoded_file_path}",
|
👋 Visual changes detected for this PR. Review and approve in PostHog Visual Review If these changes are unexpected, they may be caused by a flaky test or a broken snapshot on master. Don't approve — rerun the job or wait for a fix. Install the Visual Review Chrome extension to see visual review results at the top of your pull requests. |
This branch has not been deployed
1 participant
Merge order
Problem
repo_path(feat(cymbal): add repo paths to frames #107634) name the exact file and commit, so PostHog can show that file inline.Changes
repo_pathon GitHub shows its file at the release commit. The box has a fixed height, and it opens centered on the frame line.GET /api/projects/:id/error_tracking/git-provider-file-links/source_file/?event_uuid&event_timestamp&frame_raw_id.no_integration,rate_limitedorfile_not_found.error_tracking_repo_paths_source_file_reads_total, by outcome..github/new-events-schema-targets.txt.Note
The first view of each file costs one GitHub API call on the team's installation. The server cache and
Cache-Control: private, max-age=86400absorb repeat views.How did you test this code?
TestFrameSourceFilecatches a read for a frame the stored event does not name, for a frame without arepo_path, and for a release of another team. It also catches a second GitHub fetch for a cached file.sourceFileWindow.test.tscatches off-by-one windows at the file edges and a captured-code check that rejects truncated lines.frameSourceFileLogic.test.tscatches a window that does not grow on scroll, and a lost captured context when the file cannot be read.SourceFileFromRepositorystory renders the "after" state above.temporary_properties, so event inserts fail there before the test runs. An already-listed error tracking test fails the same way. CI runs this leg.👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Release status
The file view appears only on frames with a
repo_path, which only teams with theerror-tracking-repo-pathsflag get.Automatic notifications
Docs update
None. No doc under
docs/covers error tracking source links.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code, Claude Opus 5.5 (
claude-opus-5-5, 1M context)/improving-drf-endpoints,/adopting-generated-api-types,/writing-kea-logics,/writing-ui-components,/writing-user-facing-copy,/writing-dataclasses,/writing-tests,/writing-code-comments,/writing-pr-descriptions,/stacking-prs.🤖 Generated with Claude Code
https://claude.ai/code/session_01DpTwx89x8Wu9Nu9B82mPEj