1.10.1#283
Merged
digitalghost-dev merged 22 commits intomainfrom May 4, 2026
Merged
Conversation
Co-authored-by: Copilot <copilot@github.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR bumps project versions to v1.10.1 across builds/manifests, refactors API/error handling to return structured errors instead of fatals, adds PostHog analytics to the web app, updates docs/help/goldens, and extends CI/ignore/config entries. No broad algorithmic changes beyond error handling and telemetry. ChangesVersion bump (v1.10.1)
Error-handling refactor (replace fatals with returns & structured errors)
Web analytics and UI changes
Docs, flags, config, ignore updates
Sequence Diagram(s)sequenceDiagram
participant Browser
participant StreamlitApp
participant PostHog
Browser->>StreamlitApp: HTTP request / open page
StreamlitApp->>StreamlitApp: check st.session_state["ph_visited"]
alt not visited
StreamlitApp->>StreamlitApp: ensure st.session_state["ph_distinct_id"]
StreamlitApp->>StreamlitApp: extract X-Forwarded-For IP
StreamlitApp->>StreamlitApp: _truncate_ip(ip)
StreamlitApp->>PostHog: posthog.capture(distinct_id, "$pageview", props incl. $current_url, $ip?)
StreamlitApp->>StreamlitApp: set st.session_state["ph_visited"]=True
end
StreamlitApp->>Browser: render page (SeasonKPIs, map, legend)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
connections/connection.go (1)
41-71: 🏗️ Heavy liftPreserve the original cause when you format endpoint errors.
fetchEndpointnow replacesHTTPStatusError,*url.Error, and JSON errors with fresh string-only errors, so downstream callers can no longer useerrors.As/errors.Ison the returned error. That undercuts the value of moving from process termination to returned errors.A small wrapper that exposes the styled message via
Error()and the original cause viaUnwrap()would keep the current UX without throwing away machine-readable context.Possible shape
+type formattedEndpointError struct { + message string + cause error +} + +func (e formattedEndpointError) Error() string { return e.message } +func (e formattedEndpointError) Unwrap() error { return e.cause } + func formatEndpointError(resourceType string, err error) error { var statusErr HTTPStatusError if errors.As(err, &statusErr) { switch { case statusErr.StatusCode == http.StatusNotFound: - return fmt.Errorf("%s", utils.FormatNotFoundError(resourceType)) + return formattedEndpointError{message: utils.FormatNotFoundError(resourceType), cause: err} case statusErr.StatusCode >= http.StatusInternalServerError: - return fmt.Errorf("%s", utils.FormatServerError(resourceType)) + return formattedEndpointError{message: utils.FormatServerError(resourceType), cause: err} default: - return fmt.Errorf("%s", utils.FormatFetchError(resourceType, err)) + return formattedEndpointError{message: utils.FormatFetchError(resourceType, err), cause: err} } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@connections/connection.go` around lines 41 - 71, The formatEndpointError function is discarding the original error types; implement a small wrapper type (e.g., EndpointError) that stores a formatted message and the original cause, implements Error() to return the styled message and Unwrap() to return the cause, then change formatEndpointError to return that wrapper instead of fmt.Errorf so callers can still use errors.As / errors.Is on HTTPStatusError, *url.Error, json.SyntaxError, json.UnmarshalTypeError, etc.; use the utils.Format* helpers for the message and set the cause to the original err in each branch.connections/connection_test.go (1)
127-157: ⚡ Quick winPlease add coverage for the remaining new error-mapping branches.
These subtests lock in the 500 and malformed-JSON paths, but
formatEndpointErroralso added behavior for*url.Errorand non-404/non-5xx HTTP statuses. Right now a regression in either branch would still slip through the wrapper-level tests.A single transport failure case and a
403case here would cover the whole mapping introduced inconnections/connection.go.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@connections/connection_test.go` around lines 127 - 157, The tests are missing coverage for the formatEndpointError branches that handle transport failures (*url.Error) and non-404/non-5xx HTTP statuses; add two subtests to connections/connection_test.go alongside the existing ones: one that simulates a transport error by calling AbilityApiCall with an invalid server URL (or by using an httptest server that closes connection) and asserts the returned error contains the formatEndpointError message for a transport/*url.Error case, and another that makes the server respond with a non-404/non-5xx status (e.g. 403) and asserts the error contains the mapped message for that status; reference AbilityApiCall and formatEndpointError when locating where to add these subtests.web/analytics.py (1)
6-6: ⚡ Quick winStore PostHog project token as an environment variable rather than a string literal
PostHog's own docs note that "As a rule of thumb, we do not recommend having API keys or tokens in plaintext. Setting it as an environment variable is best." While
phc_project keys are write-only (no read access), leaking them still allows anyone to pollute your analytics data by ingesting arbitrary events.♻️ Proposed refactor
+import os + POSTHOG_API_KEY = "phc_qLvoCFJ5U9qgMS4p6LuJPgc3ZcrCRZYBNHLueHE9MU4C" +POSTHOG_API_KEY = os.environ.get("POSTHOG_API_KEY", "phc_qLvoCFJ5U9qgMS4p6LuJPgc3ZcrCRZYBNHLueHE9MU4C")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/analytics.py` at line 6, The POSTHOG_API_KEY constant is a hard-coded secret; change its initialization to read from an environment variable (e.g., os.environ.get("POSTHOG_API_KEY")) and remove the plaintext token, ensure the code handles a missing env var by raising a clear exception or logging an error and failing fast, and update any uses of POSTHOG_API_KEY to rely on this env-backed value so the secret is no longer stored in source.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@connections/connection.go`:
- Around line 129-130: Change the human-readable label passed into fetchEndpoint
from the type-style "PokémonSpecies" to a natural-language string "Pokémon
species" to make error/CLI messages read better; update the call in
PokemonSpeciesApiCall (which currently invokes
fetchEndpoint[structs.PokemonSpeciesJSONStruct](endpoint, pokemonSpeciesName,
baseURL, "PokémonSpecies")) to pass "Pokémon species" instead.
In `@gitleaks.toml`:
- Around line 9-11: Replace the hardcoded PostHog project API key in
web/analytics.py with an environment variable lookup: find the place where the
literal 'phc_' key is passed to PostHog (e.g., in posthog.init or an
initialize_posthog / get_posthog_client function) and change it to use
os.environ.get('POSTHOG_API_KEY') (or equivalent config accessor), add a clear
error or fallback if the env var is missing, and update any tests or docs that
assumed the hardcoded value.
In `@testdata/main_latest_flag.golden`:
- Line 5: The golden fixture is brittle because the LatestFlag test relies on a
live GitHub API response; change the test to be deterministic by injecting or
mocking the HTTP client used by LatestFlag (or replace the outgoing call with an
httptest server that returns a fixed JSON/body), update the test harness to
point LatestFlag at that mocked server/client, and keep the golden content
static (replace the dynamic version string in the golden fixture with the
expected fixed response used by the mock). Ensure you modify the test setup
where LatestFlag is constructed/called so it accepts the injected client or base
URL and assert against the fixed mock response.
In `@web/analytics.py`:
- Around line 24-30: The code is creating persistent person profiles by always
setting a session-based distinct_id and including $ip in properties; to fix,
gate analytics or make events anonymous: either check a consent flag (e.g.,
st.session_state.get("analytics_consent")) before calling
track_visit()/posthog.capture and skip setting distinct_id/$ip when consent is
false, or explicitly set properties["$process_person_profile"] = False (and
remove $ip) before calling posthog.capture/track_visit so events remain
anonymous; update the block that builds distinct_id and properties (variables
distinct_id, properties) to honor the consent flag or add the
$process_person_profile property.
In `@web/app.py`:
- Around line 92-93: The sum comprehension for total_participants is excluding
tournaments where player_quantity == 0 because it uses a truthy guard; update
the filter to only skip missing/None values instead of falsy zeros (e.g., use
t.get("player_quantity") is not None or "player_quantity" in t) so
total_participants correctly includes zero-valued entries; ensure you still cast
via int(t["player_quantity"]) and preserve behavior when the key is absent or
None.
In `@web/pyproject.toml`:
- Around line 16-18: The posthog package is listed under the optional
dependency-group "analytics" but is imported unconditionally by web/analytics.py
(and transitively by web/app.py), causing runtime failures; update
pyproject.toml by removing "posthog==7.14.0" from the dependency-groups
analytics list and add the same spec to the [project.dependencies] section so
posthog is installed by default; ensure any lock/update commands you use (e.g.,
the project's dependency sync) are run to regenerate the lockfile after moving
the entry.
---
Nitpick comments:
In `@connections/connection_test.go`:
- Around line 127-157: The tests are missing coverage for the
formatEndpointError branches that handle transport failures (*url.Error) and
non-404/non-5xx HTTP statuses; add two subtests to
connections/connection_test.go alongside the existing ones: one that simulates a
transport error by calling AbilityApiCall with an invalid server URL (or by
using an httptest server that closes connection) and asserts the returned error
contains the formatEndpointError message for a transport/*url.Error case, and
another that makes the server respond with a non-404/non-5xx status (e.g. 403)
and asserts the error contains the mapped message for that status; reference
AbilityApiCall and formatEndpointError when locating where to add these
subtests.
In `@connections/connection.go`:
- Around line 41-71: The formatEndpointError function is discarding the original
error types; implement a small wrapper type (e.g., EndpointError) that stores a
formatted message and the original cause, implements Error() to return the
styled message and Unwrap() to return the cause, then change formatEndpointError
to return that wrapper instead of fmt.Errorf so callers can still use errors.As
/ errors.Is on HTTPStatusError, *url.Error, json.SyntaxError,
json.UnmarshalTypeError, etc.; use the utils.Format* helpers for the message and
set the cause to the original err in each branch.
In `@web/analytics.py`:
- Line 6: The POSTHOG_API_KEY constant is a hard-coded secret; change its
initialization to read from an environment variable (e.g.,
os.environ.get("POSTHOG_API_KEY")) and remove the plaintext token, ensure the
code handles a missing env var by raising a clear exception or logging an error
and failing fast, and update any uses of POSTHOG_API_KEY to rely on this
env-backed value so the secret is no longer stored in source.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 107d1cfb-bed7-44ca-852f-9a05caeaea5b
⛔ Files ignored due to path filters (5)
card_data/data_infrastructure_diagram.pngis excluded by!**/*.pngcard_data/data_infrastructure_v2.pngis excluded by!**/*.pngdocs/assets/card.gifis excluded by!**/*.gifdocs/assets/data_infrastructure_diagram.svgis excluded by!**/*.svgweb/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
.github/workflows/ci.yml.gitignore.goreleaser.ymlDockerfileREADME.mdcard_data/README.mdcard_data/pipelines/poke_cli_dbt/dbt_project.ymlcard_data/pyproject.tomlcmd/berry/berry.gocmd/pokemon/pokemon.gocmd/speed/speed.gocmd/speed/speed_test.gocmd/utils/errors.gocmd/utils/errors_test.goconnections/connection.goconnections/connection_test.godocs/Infrastructure_Guide/index.mddocs/commands.mddocs/installation.mdflags/pokemonflagset.gogitleaks.tomlmkdocs.ymlnfpm.yamltestdata/main_latest_flag.goldentestdata/pokemon_help.goldenweb/analytics.pyweb/app.pyweb/pyproject.toml
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
This was
linked to
issues
May 4, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores