improvements to HR demo for HN video #2362
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Pull Request Overview
A refactor of the live heart‐rate leaderboard demo into an app subdirectory, adding Apple Watch support, avatar rendering, and improved default selection.
- Migrated the standalone Streamlit app into
app/streamlit_app.py, added default-user selection, avatars, and HTML‐based leaderboard with scroll control. - Integrated Apple Watch pipeline: new datamodel, streaming function, ingestion pipeline, and unified packet transformation.
- Updated APIs and datamodels to use
processed_timestampfilters, maderr_interval_msoptional, and cleaned up commented logging.
Reviewed Changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| templates/live-heartrate-leaderboard/streamlit_app.py | Removed legacy standalone Streamlit app |
| templates/live-heartrate-leaderboard/app/streamlit_app.py | Refactored app; default user, avatar column, HTML leaderboard |
| templates/live-heartrate-leaderboard/app/scripts/.../1.generate... | Commented out logger calls and added no-op pass |
| templates/live-heartrate-leaderboard/app/pipelines/pipelines.py | Added appleWatchHRPipeline and imported Apple Watch function |
| templates/live-heartrate-leaderboard/app/main.py | Included appleWatchHRPipeline in imports |
| templates/live-heartrate-leaderboard/app/functions/*.py | Disabled logger lines; new Apple Watch streaming logic |
| templates/live-heartrate-leaderboard/app/datamodels/ | Made rr_interval_ms optional; added AppleWatchHRPacket |
| templates/live-heartrate-leaderboard/app/apis/*.py | Switched to processed_timestamp filters in SQL |
Files not reviewed (1)
- templates/live-heartrate-leaderboard/mock-user-db.json: Language not supported
Comments suppressed due to low confidence (1)
templates/live-heartrate-leaderboard/app/streamlit_app.py:153
- [nitpick] This function now returns HTML and a row count rather than a DataFrame; consider renaming it (e.g.,
render_leaderboard_html) to reflect its output.
def update_leaderboard():
| 0, | ||
| "avatar", | ||
| df_display["user_name"].apply( | ||
| lambda u: f"<img src='{get_profile_image(u)}' style='width:32px;border-radius:50%;'>" |
There was a problem hiding this comment.
Calling get_profile_image inside a per-row apply causes repeated file I/O; consider loading mock-user-db.json once (e.g., cache or module‐level) and reusing the data.
There was a problem hiding this comment.
That Looks like a good suggestion
| # st.markdown(""" | ||
| # <style> | ||
| # .modern-banner { | ||
| # background: linear-gradient(135deg, #7f00ff, #e100ff); | ||
| # color: white; | ||
| # padding: 2rem; | ||
| # text-align: center; | ||
| # font-size: 1.7rem; | ||
| # font-weight: 600; | ||
| # font-family: 'Segoe UI', sans-serif; | ||
| # border-radius: 15px; | ||
| # box-shadow: 0 6px 18px rgba(0, 0, 0, 0.25); | ||
| # animation: slideFadeIn 0.8s ease-out, pulse 2.5s ease-in-out infinite; | ||
| # margin-bottom: 2rem; | ||
| # } | ||
|
|
||
| # @keyframes slideFadeIn { | ||
| # 0% { | ||
| # transform: translateY(-20px); | ||
| # opacity: 0; | ||
| # } | ||
| # 100% { | ||
| # transform: translateY(0); | ||
| # opacity: 1; | ||
| # } | ||
| # } | ||
|
|
There was a problem hiding this comment.
[nitpick] The large commented-out CSS banner block adds clutter; remove or extract it if it's no longer needed to improve readability.
| # st.markdown(""" | |
| # <style> | |
| # .modern-banner { | |
| # background: linear-gradient(135deg, #7f00ff, #e100ff); | |
| # color: white; | |
| # padding: 2rem; | |
| # text-align: center; | |
| # font-size: 1.7rem; | |
| # font-weight: 600; | |
| # font-family: 'Segoe UI', sans-serif; | |
| # border-radius: 15px; | |
| # box-shadow: 0 6px 18px rgba(0, 0, 0, 0.25); | |
| # animation: slideFadeIn 0.8s ease-out, pulse 2.5s ease-in-out infinite; | |
| # margin-bottom: 2rem; | |
| # } | |
| # @keyframes slideFadeIn { | |
| # 0% { | |
| # transform: translateY(-20px); | |
| # opacity: 0; | |
| # } | |
| # 100% { | |
| # transform: translateY(0); | |
| # opacity: 1; | |
| # } | |
| # } |
| stream=True, | ||
| table=True | ||
| )) | ||
| # Create a ingest pipeline for Appple Health |
There was a problem hiding this comment.
Typo in comment: correct "Appple" to "Apple".
| # Create a ingest pipeline for Appple Health | |
| # Create an ingest pipeline for Apple Health |
I think keep it simple in templates! |
callicles
left a comment
callicles
left a comment
There was a problem hiding this comment.
Looks good to me, some small suggestions from copilot seem relevant
cjus
left a comment
cjus
left a comment
There was a problem hiding this comment.
Are you planning on adding more context on what this template does before the getting started section in the README.md?
agreed. |
There was a problem hiding this comment.
Looks good, but:
- The readme appears to be malformatted (a code block is not closed); and you should follow this form: https://github.com/514-labs/moose/tree/main/templates/python
- is this the template description we want?
- live-heartrate-leaderboard (python) - live heart rate leaderboard. Inspired by F45
- is apple watch the only supported bluetooth emitter?
Still debating if this should go in the templates or in a separate project as I build this out for the series of video vision (i.e do stuff with AI).
What are your thoughts here @okane16 ? Should I peel off in to an examples repo where we build out more fleshed out moose projects and keep templates for getting started?