Refactor header layout, fix weather data URLs, and update version#274
Refactor header layout, fix weather data URLs, and update version#274FedericoTartarini wants to merge 17 commits intomainfrom
Conversation
…-problem fix: refresh One Building weather data and fix broken URLs #272
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR bumps the app to v0.10.2, adds Changesv0.10.2 Release & Climate Zone Optimization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
@t-kramer I opened this pull request to deploy the code to main but there is a merging issue. I assume we can accept your changes but I will let you look into it. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
.gitignore (1)
4-7: ⚡ Quick winConsider tracking data maintenance scripts in the repository.
Since these scripts were used to fix broken URLs and maintain weather data (per the PR description), they might be valuable to preserve in version control for future data updates, team collaboration, and reproducibility. If these are reusable utilities rather than one-off or environment-specific scripts, consider removing them from
.gitignoreand committing them to ascripts/ortools/directory.If these scripts are intentionally excluded (e.g., they contain sensitive paths, are personal utilities, or are truly one-off), please confirm this decision is deliberate.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.gitignore around lines 4 - 7, The .gitignore currently excludes two maintenance scripts (check_links.py and fix_broken_urls.py); either stop ignoring them and add them to the repo under a dedicated directory (e.g., move check_links.py and fix_broken_urls.py into a new scripts/ or tools/ folder and commit) or leave them ignored but document/confirm why (sensitive paths or one-off) by adding a note in the repo README or a comment in .gitignore next to their entries; update .gitignore accordingly and ensure the committed scripts include any necessary config or scrubbed secrets before pushing.Pipfile (1)
19-19: ⚡ Quick winPin
kgcpyto the tested version for consistency with other dependencies.
kgcpyis currently unbounded with*, while nearly all other runtime dependencies in the Pipfile use explicit version pins (e.g.,dash==3.2,pvlib==0.9.1). Pipenv has locked it to==1.1.8in Pipfile.lock, but the wildcard can cause different versions to be resolved if the lock file is regenerated. Update tokgcpy = "==1.1.8"to match the project's explicit pinning convention and prevent accidental version drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Pipfile` at line 19, The Pipfile currently lists kgcpy with an unbounded version ("kgcpy = \"*\""); update this to an explicit pin to match the project's convention and the lockfile by changing the entry to pin kgcpy to the tested version (kgcpy ==1.1.8) so regenerating the lockfile won't allow unintended version drift.pages/summary.py (1)
265-271: ⚡ Quick winAvoid silent climate-lookup failures.
The blanket
except Exception: passmakes regressions hard to diagnose when climate text disappears. Keep fallback behavior, but emit a debug/warning log with context.Proposed refactor
+import logging import dash import pandas as pd @@ from pages.lib.utils import ( @@ ) +logger = logging.getLogger(__name__) + @@ - except Exception: - pass + except Exception as exc: + logger.debug( + "Koppen-Geiger lookup failed (lat=%s, lon=%s): %s", + meta.get(Variables.LAT.col_name), + meta.get(Variables.LON.col_name), + exc, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pages/summary.py` around lines 265 - 271, The try/except around the climate lookup silently swallows failures; change it to catch Exception as e and log a warning or debug message including the latitude/longitude (meta[Variables.LAT.col_name], meta[Variables.LON.col_name]) and the exception, while keeping the fallback behavior (do not re-raise). Update the block that calls lookupCZ and reads KG_DESCRIPTIONS (and sets climate_text) to use a logger (existing module logger if available, otherwise logging.getLogger(__name__)) and include the zone, lat/lon, and exception details in the log so missing climate_text is diagnosable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@docs/documentation/tabs-explained/temperature-and-humidity/relative-humidity-explained.md`:
- Line 7: The markdown after the "ANSI/ASHRAE Standard 160" link has stray bold
markers around the comma (`**,**`) which break rendering; edit the sentence
containing the "ANSI/ASHRAE Standard 160" link in relative-humidity-explained.md
to remove the extra `**` so the comma is plain (e.g., change `](... )**,**` to
`](...),`) and ensure the link and following punctuation render normally.
- Around line 40-42: The fenced code block around the HTML figure causes it to
render as literal code; remove the surrounding triple backticks so the
<figure>...<figcaption> block is raw HTML (i.e., delete the opening ``` before
<figure> and the closing ``` after </figure>) leaving the <figure> tag and its
contents unchanged.
In `@pages/select.py`:
- Around line 270-272: The current block builds location_string using meta
without verifying meta is present or contains the expected keys; update the
guard so you only construct location_string when meta is non-None and contains
the required keys (e.g., Variables.CITY.col_name and
Variables.COUNTRY.col_name); otherwise set location_string to a safe default
(empty string or "Location unavailable") and keep disable_links behavior
consistent. Locate the usage around the variables data, meta, location_string
and disable_links and change the condition from checking data alone to a
combined check (or add an inner if) that ensures meta and the specific meta keys
exist before indexing.
In `@tests/test_summary.py`:
- Around line 94-99: Remove the flaky intermediate assertion that expects
info_section to have attribute "data-dash-is-loading" and only wait for the
final non-loading state; specifically, delete the
expect(info_section).to_have_attribute("data-dash-is-loading", "true",
timeout=10000) call and keep (or adjust) the
expect(info_section).not_to_have_attribute("data-dash-is-loading", "true",
timeout=20000) check so the test reliably waits until info_section is no longer
loading before continuing to assert updated IP values.
---
Nitpick comments:
In @.gitignore:
- Around line 4-7: The .gitignore currently excludes two maintenance scripts
(check_links.py and fix_broken_urls.py); either stop ignoring them and add them
to the repo under a dedicated directory (e.g., move check_links.py and
fix_broken_urls.py into a new scripts/ or tools/ folder and commit) or leave
them ignored but document/confirm why (sensitive paths or one-off) by adding a
note in the repo README or a comment in .gitignore next to their entries; update
.gitignore accordingly and ensure the committed scripts include any necessary
config or scrubbed secrets before pushing.
In `@pages/summary.py`:
- Around line 265-271: The try/except around the climate lookup silently
swallows failures; change it to catch Exception as e and log a warning or debug
message including the latitude/longitude (meta[Variables.LAT.col_name],
meta[Variables.LON.col_name]) and the exception, while keeping the fallback
behavior (do not re-raise). Update the block that calls lookupCZ and reads
KG_DESCRIPTIONS (and sets climate_text) to use a logger (existing module logger
if available, otherwise logging.getLogger(__name__)) and include the zone,
lat/lon, and exception details in the log so missing climate_text is
diagnosable.
In `@Pipfile`:
- Line 19: The Pipfile currently lists kgcpy with an unbounded version ("kgcpy =
\"*\""); update this to an explicit pin to match the project's convention and
the lockfile by changing the entry to pin kgcpy to the tested version (kgcpy
==1.1.8) so regenerating the lockfile won't allow unintended version drift.
🪄 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: a1369943-b857-4a15-82ef-cfaa2d3b5818
⛔ Files ignored due to path filters (3)
Pipfile.lockis excluded by!**/*.lockassets/data/OneBuilding files.zipis excluded by!**/*.zipassets/data/one_building.csvis excluded by!**/*.csv
📒 Files selected for processing (15)
.bumpversion.cfg.gitignorePipfileassets/manifest.jsondocs/contributing/run-project-locally.mddocs/documentation/tabs-explained/temperature-and-humidity/relative-humidity-explained.mdpages/lib/import_one_building_files.pypages/lib/layout.pypages/lib/template_graphs.pypages/lib/utils.pypages/select.pypages/summary.pypages/t_rh.pytests/test_summary.pytests/utils.py
| ``` | ||
| <figure><img src="../../../.gitbook/assets/Desc stat RH.png" alt=""><figcaption><p>Descriptive statistics of relative humidity trend in a temperate climate, Berkeley (USA)</p></figcaption></figure> | ||
| ``` |
There was a problem hiding this comment.
Remove code fences around the <figure> block.
Wrapping the figure in fenced code makes it render as literal code instead of an image/figure block. Keep the HTML figure block unfenced.
Proposed fix
-```
<figure><img src="../../../.gitbook/assets/Desc stat RH.png" alt=""><figcaption><p>Descriptive statistics of relative humidity trend in a temperate climate, Berkeley (USA)</p></figcaption></figure>
-```🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 40-40: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@docs/documentation/tabs-explained/temperature-and-humidity/relative-humidity-explained.md`
around lines 40 - 42, The fenced code block around the HTML figure causes it to
render as literal code; remove the surrounding triple backticks so the
<figure>...<figcaption> block is raw HTML (i.e., delete the opening ``` before
<figure> and the closing ``` after </figure>) leaving the <figure> tag and its
contents unchanged.
| if data is not None: | ||
| location_string = f"Location: {meta[Variables.CITY.col_name]}, {meta[Variables.COUNTRY.col_name]}" | ||
| disable_links = False |
There was a problem hiding this comment.
Guard meta before building the location string.
Line 271 can throw when data is present but meta is still None/incomplete. Add a defensive check before indexing meta.
Proposed fix
- if data is not None:
- location_string = f"Location: {meta[Variables.CITY.col_name]}, {meta[Variables.COUNTRY.col_name]}"
+ if (
+ data is not None
+ and meta is not None
+ and Variables.CITY.col_name in meta
+ and Variables.COUNTRY.col_name in meta
+ ):
+ location_string = (
+ f"Location: {meta[Variables.CITY.col_name]}, "
+ f"{meta[Variables.COUNTRY.col_name]}"
+ )
disable_links = False🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pages/select.py` around lines 270 - 272, The current block builds
location_string using meta without verifying meta is present or contains the
expected keys; update the guard so you only construct location_string when meta
is non-None and contains the required keys (e.g., Variables.CITY.col_name and
Variables.COUNTRY.col_name); otherwise set location_string to a safe default
(empty string or "Location unavailable") and keep disable_links behavior
consistent. Locate the usage around the variables data, meta, location_string
and disable_links and change the condition from checking data alone to a
combined check (or add an inner if) that ensures meta and the specific meta keys
exist before indexing.
| expect(info_section).to_have_attribute( | ||
| "data-dash-is-loading", "true", timeout=10000 | ||
| ) | ||
| expect(info_section).not_to_have_attribute( | ||
| "data-dash-is-loading", "true", timeout=20000 | ||
| ) |
There was a problem hiding this comment.
The transient "loading=true" assertion is flaky.
This intermediate state can be missed on fast runs, causing nondeterministic failures. Waiting only for the final non-loading state is safer here (you already assert updated IP values afterward).
Proposed test fix
- expect(info_section).to_have_attribute(
- "data-dash-is-loading", "true", timeout=10000
- )
expect(info_section).not_to_have_attribute(
"data-dash-is-loading", "true", timeout=20000
)📝 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.
| expect(info_section).to_have_attribute( | |
| "data-dash-is-loading", "true", timeout=10000 | |
| ) | |
| expect(info_section).not_to_have_attribute( | |
| "data-dash-is-loading", "true", timeout=20000 | |
| ) | |
| expect(info_section).not_to_have_attribute( | |
| "data-dash-is-loading", "true", timeout=20000 | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_summary.py` around lines 94 - 99, Remove the flaky intermediate
assertion that expects info_section to have attribute "data-dash-is-loading" and
only wait for the final non-loading state; specifically, delete the
expect(info_section).to_have_attribute("data-dash-is-loading", "true",
timeout=10000) call and keep (or adjust) the
expect(info_section).not_to_have_attribute("data-dash-is-loading", "true",
timeout=20000) check so the test reliably waits until info_section is no longer
loading before continuing to assert updated IP values.
There was a problem hiding this comment.
Pull request overview
This pull request focuses on UI/layout refinements and improving data reliability by removing an external Köppen–Geiger lookup dependency, while also updating documentation/tests and bumping the app version.
Changes:
- Switched Köppen–Geiger climate zone lookup from an external HTTP API to the local
kgcpylibrary, adding local zone descriptions. - Refactored parts of the UI (header layout, RH “comfort band” label, summary/stat table rendering) and adjusted loading/test expectations.
- Updated docs/deployment instructions and bumped version references to
0.10.2.
Reviewed changes
Copilot reviewed 14 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
pages/summary.py |
Replaces external climate zone API call with kgcpy.lookupCZ and local descriptions. |
Pipfile |
Adds kgcpy dependency. |
Pipfile.lock |
Locks updated dependency set including kgcpy and other version bumps. |
pages/lib/utils.py |
Refactors summary statistics table rendering from Dash DataTable to Mantine table components; adds typing annotations. |
pages/lib/layout.py |
Refactors header layout to dmc.Flex, updates “Location” subtitle text, bumps footer version string. |
pages/lib/template_graphs.py |
Renames RH band legend label to “ASHRAE 160 humidity range”. |
pages/t_rh.py |
Adjusts skeleton loader height for table section. |
pages/select.py |
Simplifies/standardizes “Location: …” string construction and tab-disable logic. |
pages/lib/import_one_building_files.py |
Makes KML import more robust by skipping entries without a URL match. |
tests/utils.py |
Updates expected banner text to “Location: …”. |
tests/test_summary.py |
Adds assertions around Dash loading attribute transitions for #location-info. |
docs/documentation/tabs-explained/temperature-and-humidity/relative-humidity-explained.md |
Updates RH band explanation and fixes some links/wording (but introduces markdown formatting issues). |
docs/contributing/run-project-locally.md |
Adds Cloud Run deploy tagging guidance for versioned deployments. |
assets/manifest.json |
Bumps manifest app id to 0.10.2. |
.bumpversion.cfg |
Updates current_version to 0.10.2. |
.gitignore |
Ignores local data maintenance scripts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ``` | ||
| <figure><img src="../../../.gitbook/assets/Desc stat RH.png" alt=""><figcaption><p>Descriptive statistics of relative humidity trend in a temperate climate, Berkeley (USA)</p></figcaption></figure> | ||
| ``` |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/documentation/tabs-explained/temperature-and-humidity/relative-humidity-explained.md (1)
40-42:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRender the descriptive-statistics figure as HTML, not fenced code.
The triple backticks make the
<figure>render as literal code instead of the image block. Please remove the surrounding code fence.Proposed fix
-``` <figure><img src="../../../.gitbook/assets/Desc stat RH.png" alt=""><figcaption><p>Descriptive statistics of relative humidity trend in a temperate climate, Berkeley (USA)</p></figcaption></figure> -```🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/documentation/tabs-explained/temperature-and-humidity/relative-humidity-explained.md` around lines 40 - 42, The `<figure>...</figure>` HTML block is currently wrapped in triple backticks so it renders as a code fence; remove the surrounding backticks (the ``` markers) so the `<figure>` element is raw HTML and will render the image and caption correctly, ensuring no leftover backtick characters remain around the `<figure>` tag.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@docs/documentation/tabs-explained/temperature-and-humidity/relative-humidity-explained.md`:
- Around line 40-42: The `<figure>...</figure>` HTML block is currently wrapped
in triple backticks so it renders as a code fence; remove the surrounding
backticks (the ``` markers) so the `<figure>` element is raw HTML and will
render the image and caption correctly, ensuring no leftover backtick characters
remain around the `<figure>` tag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 322ceab1-ee89-4ddd-96d6-904f563c9f1b
📒 Files selected for processing (1)
docs/documentation/tabs-explained/temperature-and-humidity/relative-humidity-explained.md
|
@FedericoTartarini I resolved the merge conflict and implemented a few minor comments made by CodeRabbit. Feel free to merge and deploy now. |
This pull request introduces several improvements and bug fixes across the codebase, focusing on the user interface, code quality, and data accuracy. The most significant changes include switching the Köppen-Geiger climate zone lookup to a local library, updating the humidity comfort band terminology to match ASHRAE standards, improving the summary table rendering, and making several UI and documentation refinements. The version is also bumped to 0.10.2.
Climate zone lookup and data accuracy:
kgcpylibrary for improved reliability and performance inpages/summary.py, and added a local dictionary for zone descriptions. [1] [2] [3] [4]User interface and accessibility improvements:
Code quality and maintainability:
Documentation and deployment:
Versioning and metadata:
.bumpversion.cfg,assets/manifest.json, UI footer). [1] [2] [3]Testing improvements:
Summary by CodeRabbit
New Features
UI/UX
Documentation
Tests
Chores