Adding PATH cutouts to F4PZ #41
Conversation
profxj
left a comment
profxj
left a comment
There was a problem hiding this comment.
color me super impressed!
just a couple of doc string comments from me
|
|
||
| log = logging.getLogger(__name__) | ||
|
|
||
| def path_cutout_proxy(request, frb_name: str) -> HttpResponse: |
There was a problem hiding this comment.
please add brief comments on the inputs
| return resp | ||
|
|
||
|
|
||
| def path_cutout_zoomin_proxy(request, frb_name: str) -> HttpResponse: |
There was a problem hiding this comment.
Pull Request Overview
This PR adds functionality to display PATH cutout images for FRB transients by proxying image requests from the private chime-path GitHub repository. The implementation requires a GITHUB_TOKEN environment variable for authentication.
Key Changes:
- Added two new proxy view functions to fetch PATH cutout images (standard and zoom-in) from the GitHub API
- Integrated the cutout images into the FRB transient detail page UI
- Configured the Docker environment to accept a GitHub token
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| settings.py | Added blank line at end of file |
| docker/docker-compose.yml | Added GITHUB_TOKEN environment variable configuration |
| YSE_App/views.py | Implemented two proxy functions for fetching PATH cutouts from GitHub API and added browse URL generation in transient detail view |
| YSE_App/urls.py | Added URL patterns for the two new proxy endpoints |
| YSE_App/templates/YSE_App/frb_transient_detail.html | Restructured layout to display PATH cutout images alongside existing footprint carousel and external links |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| f"chime_path/{year}/{transient_obj.name}" | ||
| ) | ||
|
|
||
| print("browse_url:", browse_url) # will appear in server logs/stdout |
There was a problem hiding this comment.
Debug print statement should be removed or replaced with proper logging using the log object already defined at line 950.
| print("browse_url:", browse_url) # will appear in server logs/stdout | |
| log.info(f"browse_url: {browse_url}") # replaced print with logging |
| blob = None | ||
| if data.get("encoding") == "base64" and data.get("content"): | ||
| try: | ||
| cleaned = "".join(str(data["content"]).splitlines()) |
There was a problem hiding this comment.
[nitpick] The str() cast is unnecessary if data['content'] is already a string. Consider removing it or add a comment explaining when this would be a non-string type.
| cleaned = "".join(str(data["content"]).splitlines()) | |
| cleaned = "".join(data["content"].splitlines()) |
| cleaned = "".join(str(data["content"]).splitlines()) | ||
| blob = base64.b64decode(cleaned) |
There was a problem hiding this comment.
This duplicates the exact same base64 decoding logic from lines 1006-1007. Consider extracting this into a helper function to reduce code duplication.
| .path-cutout-img { max-width: 320px; height: auto; } | ||
| .path-fallback { display:none; width:320px; height:320px; line-height:320px; text-align:center; margin:0; } |
There was a problem hiding this comment.
[nitpick] The hardcoded dimensions (320px) create a magic number that's repeated multiple times. Consider using a CSS variable or moving this to a shared stylesheet.
| .path-cutout-img { max-width: 320px; height: auto; } | |
| .path-fallback { display:none; width:320px; height:320px; line-height:320px; text-align:center; margin:0; } | |
| :root { --cutout-size: 320px; } | |
| .path-cutout-img { max-width: var(--cutout-size); height: auto; } | |
| .path-fallback { display:none; width:var(--cutout-size); height:var(--cutout-size); line-height:var(--cutout-size); text-align:center; margin:0; } |
Adding HTML + code to query PATH cutouts from the chime-path repo. Note that this does require setting the GITHUB access token in the .env!