filtering data grays out part of the chart #266
Conversation
…long with examples of code modifications.
…nd template_graphs.py
…rts_data_explorer.py
…y-chart.py and summary.py
… and natural_ventilation.py.
- updated the Menu structure - centred the Sun and cloud tab - fixed the Outdoor Comfort / UTCI chart's bar chart - made the Apply filter button more visible - made the dropdown for the filtering is open by default
optimize Playwright tests for faster execution
Fixed the issues of number 1, 3, 5, and 6
- fixed the yearly chart breaking - fixed the legend color - removed the grey fill in Cloud coverage, Psychrometric Chart, and the UTCI chart - extracted the grey fill function
Add grey fill in most of the charts
WalkthroughVersion bumped to 0.10.1. CI moved from Cypress to Playwright (Cypress configs/tests removed, Playwright tests added). UI migrated from Dash Bootstrap to Dash Mantine and centralized identifiers/metadata (ElementIds, Variables, TabNames, IdButtons, VariableInfo) were introduced. A global month/hour filter store and filtering flow were added and threaded through pages; Dockerfile switched to Pipenv; many CSS assets and requirements.txt removed. Changes
Sequence Diagram(s)%%{init: {"themeVariables": {"actorBackground":"#f5f7fb","actorBorder":"#dbe5f8","sequenceBackground":"#ffffff"}}}%%
sequenceDiagram
participant User as Tools Menu (user)
participant Store as Tools Global Filter Store
participant PageCB as Page Callback
participant Utils as Filtering Utils
participant Chart as Chart Renderer
User->>Store: Apply filter (month/hour, invert flags)
Store-->>PageCB: global_filter_data (active, month_range, hour_range, invert flags)
PageCB->>Utils: apply_global_month_hour_filter(df, global_filter_data)
Utils-->>PageCB: {df_unfiltered, df_filtered, _is_filtered}
alt _is_filtered true
PageCB->>Chart: render Unfiltered (color) + Filtered (gray) traces
else
PageCB->>Chart: render Unfiltered only
end
Chart-->>User: updated chart visuals
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas needing extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pages/lib/template_graphs.py (2)
1267-1273: Do not passNoneas the Plotly margin.
tight_margins.copy().update({"t": 55})mutates in place and returnsNone, soupdate_layout(..., margin=None, ...)drops the layout margins entirely. Create a copy, update it, and pass the dict instead (e.g.,margins = {**tight_margins, "t": 55}).
1503-1511: Fix the filter description string.When appending the range summary you concatenate
str(min_val) + " and " + str(min_val) + filter_unit, so the max bound and spacing are wrong in the rendered title. Use the realmax_valand include a space before the unit.pages/psy-chart.py (1)
213-218: Don't clobber_is_filteredduring the local data filter step.When a global month/hour filter is active,
apply_global_month_hour_filteradds_is_filtered(and original-value columns) soseparate_filtered_datacan keep filtered points separate. The linedf[mask] = Nonenow wipes those helper columns toNone, so_is_filteredbecomes NA. Downstream we then hitseparate_filtered_data(..., Variables.DBT.col_name), and the boolean mask with NA values causes pandas to raiseValueError: cannot mask with non-boolean array containing NA / NaN values(you can repro by enabling the global filter, applying the local min/max filter, and triggering the callback).Preserve
_is_filteredwhen nulling rows for the local value filter:- if min_val <= max_val: - mask = (df[data_filter_var] < min_val) | (df[data_filter_var] > max_val) - df[mask] = None + if min_val <= max_val: + mask = (df[data_filter_var] < min_val) | (df[data_filter_var] > max_val) + cols_to_clear = [col for col in df.columns if col != "_is_filtered"] + df.loc[mask, cols_to_clear] = NoneThat keeps
_is_filteredboolean while still blanking the rows so the chart drops them.
🧹 Nitpick comments (11)
Pipfile (1)
27-28: Unpin test framework versions for reproducibility.Test dependencies should be version-pinned rather than using wildcard (
*). This ensures reproducible test runs across environments.Apply this diff to pin test framework versions:
[dev-packages] cleanpy = "*" pytest = "*" bump2version = "*" black = "*" ruff = "*" pre-commit = "*" -pytest-playwright = "*" -pytest-xdist = "*" +pytest-playwright = "==0.50.1" +pytest-xdist = "==3.6.1"Note: Replace version numbers with the latest stable versions available at the time of commit.
.github/workflows/python.yml (1)
38-40: Consider capturing background process logs for debugging.If
main.pyfails to start, the current setup will wait for the full 60-second timeout before reporting an error. Consider redirecting output to a log file that can be uploaded as an artifact on failure.Example improvement:
- name: Start Clima run: |- - pipenv run python main.py & + pipenv run python main.py > clima.log 2>&1 & + echo $! > clima.pidThen add a cleanup step at the end:
- name: Upload logs on failure if: failure() uses: actions/upload-artifact@v3 with: name: clima-logs path: clima.logdocs/contributing/run-project-locally.md (1)
97-115: LGTM! Comprehensive local deployment workflow.The manual deployment section provides clear steps for building, testing locally, and deploying. The explicit platform specification (
--platform linux/amd64) is important for Apple Silicon compatibility.Consider adding a brief note about the difference between the two docker run examples (lines 107 vs 109-110) to clarify when to use each approach.
Dockerfile (1)
13-15: Consider usingpipenv sync --systemfor production containers.Running pipenv inside a Docker container creates an unnecessary virtualenv. For production images, consider adding the
--systemflag to install packages directly into the system Python.# Install dependencies -RUN pipenv sync +RUN pipenv sync --systemAlternatively, generate
requirements.txtduring CI and use standard pip install for smaller image size and faster startup.config.py (1)
57-57: Correct capitalization to match the title case convention used throughout PageInfo.The inconsistency is confirmed. SELECT_NAME uses sentence case ("Select weather file"), while all other page names use title case ("Climate Summary", "Temperature and Humidity", etc.). Change line 57 to:
SELECT_NAME = "Select Weather File"tests/test_select.py (1)
13-26: Consider using centralized ElementIds constants.The test uses hardcoded strings and CSS selectors to locate UI elements. According to the PR summary, centralized public UI constants (ElementIds, IdButtons, TabNames) have been introduced. Using these constants would make tests more maintainable and resilient to UI changes.
For example, instead of:
expect(page.locator("#alert")).to_contain_text("upload an EPW file") expect(page.locator("#upload-data-button")).to_be_visible()Consider importing and using:
from pages.lib.global_element_ids import ElementIds expect(page.locator(f"#{ElementIds.ALERT}")).to_contain_text("upload an EPW file")tests/test_outdoor.py (1)
16-39: Consider using centralized ElementIds constants.Similar to other test files, this test relies on hardcoded strings and CSS selectors. The PR introduces centralized public UI constants (ElementIds, IdButtons) that should be used to make tests more maintainable.
For example, instead of:
expect(page.locator("#image-selection")).to_be_visible() expect(page.locator("#utci-heatmap")).to_be_visible()Consider importing and using ElementIds constants to reference these UI elements consistently.
tests/utils.py (1)
28-32: XPath locator may be brittle.The XPath expression
xpath=ancestor::a[1] | ancestor::button[1]assumes a specific DOM structure. While this may be necessary for the Mantine NavLink structure, it could break if the component hierarchy changes.Consider adding a comment explaining why this XPath traversal is necessary and what DOM structure it expects, to help future maintainers understand the dependency on Mantine's internal structure.
# Find the navigation link container whose id starts with "nav-" and text matches nav_link = page.locator(f'[id^="nav-"] >> text="{tab_name}"').first + # Navigate up to the clickable ancestor (a or button) in Mantine NavLink structure + # This XPath is coupled to Mantine's internal DOM hierarchy # Go up to the clickable element (<a> or <button>) clickable = nav_link.locator("xpath=ancestor::a[1] | ancestor::button[1]")tests/test_wind.py (1)
24-35: Consider using centralized ElementIds constants.Like the other test files, this test uses hardcoded CSS selectors. The PR introduces centralized ElementIds constants that should be used for better maintainability.
tests/test_sun.py (1)
30-48: Consider using centralized ElementIds constants.This test, like others in the suite, uses hardcoded CSS selectors. Adopting the centralized ElementIds constants introduced in the PR would improve maintainability and reduce the risk of tests breaking due to UI changes.
tests/test_t_rh.py (1)
16-40: Consider using centralized ElementIds constants.Following the pattern across the test suite, this test should use the centralized ElementIds constants introduced in the PR rather than hardcoded CSS selectors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Pipfile.lockis excluded by!**/*.locktests/node/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (75)
.bumpversion.cfg(1 hunks).dockerignore(0 hunks).github/workflows/cypress.yml(0 hunks).github/workflows/python.yml(1 hunks)Dockerfile(1 hunks)Pipfile(2 hunks)assets/banner.css(0 hunks)assets/construction.css(0 hunks)assets/fonts.css(0 hunks)assets/footer.css(0 hunks)assets/layout.css(0 hunks)assets/manifest.json(1 hunks)assets/tabs.css(0 hunks)config.py(1 hunks)docs/README.md(1 hunks)docs/contributing/contributing.md(3 hunks)docs/contributing/run-project-locally.md(1 hunks)docs/documentation/tabs-explained/outdoor-comfort/utci-explained.md(1 hunks)docs/documentation/tabs-explained/psychrometric-chart/README.md(1 hunks)docs/documentation/tabs-explained/psychrometric-chart/psychrometric-chart-explained.md(1 hunks)docs/documentation/tabs-explained/sun-and-cloud/README.md(0 hunks)docs/documentation/tabs-explained/sun-and-cloud/cloud-coverage.md(1 hunks)docs/documentation/tabs-explained/sun-and-cloud/customizable-daily-and-hourly-maps.md(0 hunks)docs/documentation/tabs-explained/sun-and-cloud/global-and-diffuse-horizontal-solar-radiation/README.md(1 hunks)docs/documentation/tabs-explained/sun-and-cloud/global-and-diffuse-horizontal-solar-radiation/global-diffuse-and-normal-solar-radiation-explained.md(1 hunks)docs/documentation/tabs-explained/tab-summary/README.md(0 hunks)docs/documentation/tabs-explained/tab-summary/clima-dataframe.md(1 hunks)docs/documentation/tabs-explained/tab-summary/climate-profiles-explained.md(1 hunks)docs/documentation/tabs-explained/tab-summary/degree-days-explained.md(1 hunks)docs/version/changelog.md(1 hunks)main.py(1 hunks)pages/explorer.py(11 hunks)pages/lib/charts_data_explorer.py(6 hunks)pages/lib/charts_summary.py(1 hunks)pages/lib/charts_sun.py(7 hunks)pages/lib/extract_df.py(7 hunks)pages/lib/global_element_ids.py(1 hunks)pages/lib/global_id_buttons.py(1 hunks)pages/lib/global_scheme.py(2 hunks)pages/lib/global_tab_names.py(1 hunks)pages/lib/global_variables.py(1 hunks)pages/lib/layout.py(1 hunks)pages/lib/template_graphs.py(18 hunks)pages/lib/utils.py(10 hunks)pages/natural_ventilation.py(9 hunks)pages/not_found_404.py(2 hunks)pages/outdoor.py(10 hunks)pages/psy-chart.py(9 hunks)pages/select.py(11 hunks)pages/summary.py(6 hunks)pages/sun.py(3 hunks)pages/t_rh.py(2 hunks)pages/wind.py(10 hunks)requirements.txt(0 hunks)tests/conftest.py(1 hunks)tests/node/cypress.config.js(0 hunks)tests/node/cypress/.gitignore(0 hunks)tests/node/cypress/e2e/spec.cy.js(0 hunks)tests/node/cypress/fixtures/example.json(0 hunks)tests/node/cypress/support/commands.js(0 hunks)tests/node/cypress/support/e2e.js(0 hunks)tests/node/package.json(0 hunks)tests/node/test.epw(0 hunks)tests/python/test_utils.py(0 hunks)tests/test_explorer.py(1 hunks)tests/test_filter.py(1 hunks)tests/test_natural_ventilation.py(1 hunks)tests/test_outdoor.py(1 hunks)tests/test_psy-chart.py(1 hunks)tests/test_select.py(1 hunks)tests/test_summary.py(1 hunks)tests/test_sun.py(1 hunks)tests/test_t_rh.py(1 hunks)tests/test_wind.py(1 hunks)tests/utils.py(1 hunks)
💤 Files with no reviewable changes (21)
- tests/node/cypress/support/e2e.js
- tests/node/cypress/.gitignore
- tests/node/test.epw
- .github/workflows/cypress.yml
- assets/banner.css
- tests/node/package.json
- assets/footer.css
- assets/construction.css
- docs/documentation/tabs-explained/sun-and-cloud/customizable-daily-and-hourly-maps.md
- docs/documentation/tabs-explained/sun-and-cloud/README.md
- assets/tabs.css
- tests/node/cypress/e2e/spec.cy.js
- assets/layout.css
- tests/node/cypress.config.js
- tests/node/cypress/support/commands.js
- tests/python/test_utils.py
- assets/fonts.css
- tests/node/cypress/fixtures/example.json
- .dockerignore
- requirements.txt
- docs/documentation/tabs-explained/tab-summary/README.md
🧰 Additional context used
🧬 Code graph analysis (29)
tests/test_outdoor.py (2)
tests/utils.py (2)
upload_epw_file(5-20)open_tab(23-37)tests/conftest.py (1)
base_url(5-6)
tests/test_natural_ventilation.py (2)
tests/utils.py (2)
upload_epw_file(5-20)open_tab(23-37)tests/conftest.py (1)
base_url(5-6)
tests/test_select.py (2)
tests/utils.py (1)
upload_epw_file(5-20)tests/conftest.py (1)
base_url(5-6)
main.py (2)
pages/lib/layout.py (1)
create_collapsible_layout(430-466)pages/lib/global_element_ids.py (1)
ElementIds(4-241)
tests/test_summary.py (2)
tests/utils.py (2)
upload_epw_file(5-20)open_tab(23-37)tests/conftest.py (1)
base_url(5-6)
tests/test_psy-chart.py (2)
tests/utils.py (2)
upload_epw_file(5-20)open_tab(23-37)tests/conftest.py (1)
base_url(5-6)
tests/test_t_rh.py (2)
tests/utils.py (2)
upload_epw_file(5-20)open_tab(23-37)tests/conftest.py (1)
base_url(5-6)
tests/test_explorer.py (2)
tests/utils.py (2)
upload_epw_file(5-20)open_tab(23-37)tests/conftest.py (1)
base_url(5-6)
tests/test_sun.py (2)
tests/utils.py (2)
upload_epw_file(5-20)open_tab(23-37)tests/conftest.py (1)
base_url(5-6)
pages/lib/layout.py (5)
pages/lib/global_variables.py (2)
Variables(104-548)IP(55-59)config.py (2)
DocLinks(82-96)UnitSystem(16-20)pages/lib/global_element_ids.py (1)
ElementIds(4-241)pages/lib/utils.py (3)
determine_month_and_hour_filter(300-308)get_default_global_filter_store_data(348-359)get_global_filter_state(362-379)pages/lib/template_graphs.py (1)
time_filtering(1515-1535)
pages/lib/charts_data_explorer.py (2)
pages/lib/utils.py (1)
get_max_min_value(327-345)pages/lib/global_variables.py (7)
Variables(104-548)VariableInfo(63-101)from_col_name(95-101)get_name(74-76)get_unit(78-82)get_range(84-88)get_color(90-92)
pages/lib/extract_df.py (2)
config.py (1)
UnitSystem(16-20)pages/lib/global_variables.py (3)
Variables(104-548)VariableInfo(63-101)IP(55-59)
pages/lib/charts_summary.py (1)
pages/lib/global_variables.py (1)
Variables(104-548)
tests/test_wind.py (2)
tests/utils.py (2)
upload_epw_file(5-20)open_tab(23-37)tests/conftest.py (1)
base_url(5-6)
tests/test_filter.py (3)
tests/utils.py (1)
upload_epw_file(5-20)pages/wind.py (1)
sliders(28-64)tests/conftest.py (1)
base_url(5-6)
pages/lib/global_variables.py (1)
config.py (1)
UnitSystem(16-20)
pages/lib/utils.py (2)
config.py (1)
UnitSystem(16-20)pages/lib/global_variables.py (7)
Variables(104-548)VariableInfo(63-101)from_col_name(95-101)get_name(74-76)get_unit(78-82)get_range(84-88)get_color(90-92)
pages/summary.py (8)
config.py (2)
DocLinks(82-96)UnitSystem(16-20)pages/lib/charts_summary.py (1)
world_map(6-37)pages/lib/global_variables.py (4)
Variables(104-548)VariableInfo(63-101)from_col_name(95-101)get_unit(78-82)pages/lib/global_element_ids.py (1)
ElementIds(4-241)pages/lib/global_id_buttons.py (1)
IdButtons(1-29)pages/lib/global_tab_names.py (1)
TabNames(1-47)pages/lib/utils.py (5)
title_with_tooltip(154-180)title_with_link(183-211)generate_chart_name(30-46)generate_units_degree(57-58)generate_units(49-54)pages/lib/layout.py (1)
apply_global_month_hour_filter(565-621)
pages/lib/charts_sun.py (2)
pages/lib/utils.py (2)
get_max_min_value(327-345)separate_filtered_data(399-428)pages/lib/global_variables.py (7)
Variables(104-548)VariableInfo(63-101)from_col_name(95-101)get_unit(78-82)get_range(84-88)get_name(74-76)get_color(90-92)
pages/select.py (4)
pages/lib/extract_df.py (1)
convert_df_units(507-521)pages/lib/global_variables.py (1)
Variables(104-548)pages/lib/global_element_ids.py (1)
ElementIds(4-241)pages/lib/utils.py (2)
generate_chart_name(30-46)get_default_global_filter_store_data(348-359)
pages/outdoor.py (6)
pages/lib/global_element_ids.py (1)
ElementIds(4-241)pages/lib/global_variables.py (1)
Variables(104-548)pages/lib/global_id_buttons.py (1)
IdButtons(1-29)pages/lib/global_tab_names.py (1)
TabNames(1-47)pages/lib/utils.py (6)
get_time_filter_from_store(382-396)dropdown(311-324)title_with_link(183-211)title_with_tooltip(154-180)generate_units_degree(57-58)generate_chart_name(30-46)pages/lib/layout.py (1)
apply_global_month_hour_filter(565-621)
pages/psy-chart.py (7)
pages/lib/utils.py (7)
get_max_min_value(327-345)separate_filtered_data(399-428)dropdown(311-324)title_with_link(183-211)get_global_filter_state(362-379)determine_month_and_hour_filter(300-308)generate_chart_name(30-46)pages/lib/global_element_ids.py (1)
ElementIds(4-241)pages/lib/global_variables.py (7)
Variables(104-548)VariableInfo(63-101)from_col_name(95-101)get_unit(78-82)get_name(74-76)get_color(90-92)get_range(84-88)pages/lib/global_id_buttons.py (1)
IdButtons(1-29)pages/lib/global_tab_names.py (1)
TabNames(1-47)pages/lib/layout.py (1)
apply_global_month_hour_filter(565-621)pages/lib/template_graphs.py (1)
filter_df_by_month_and_hour(1538-1565)
pages/lib/global_scheme.py (1)
pages/lib/global_variables.py (4)
Variables(104-548)VariableInfo(63-101)from_col_name(95-101)get_name(74-76)
pages/natural_ventilation.py (7)
pages/lib/global_variables.py (7)
Variables(104-548)VariableInfo(63-101)from_col_name(95-101)get_unit(78-82)get_name(74-76)get_color(90-92)IP(55-59)pages/lib/global_element_ids.py (1)
ElementIds(4-241)pages/lib/global_id_buttons.py (1)
IdButtons(1-29)pages/lib/global_tab_names.py (1)
TabNames(1-47)pages/lib/utils.py (7)
separate_filtered_data(399-428)title_with_link(183-211)title_with_tooltip(154-180)get_global_filter_state(362-379)determine_month_and_hour_filter(300-308)generate_chart_name(30-46)has_filtered_data(431-432)config.py (2)
DocLinks(82-96)UnitSystem(16-20)pages/lib/layout.py (1)
apply_global_month_hour_filter(565-621)
pages/t_rh.py (7)
pages/lib/template_graphs.py (3)
heatmap(822-933)yearly_profile(93-466)daily_profile(470-643)pages/lib/global_variables.py (1)
Variables(104-548)pages/lib/global_element_ids.py (1)
ElementIds(4-241)pages/lib/global_id_buttons.py (1)
IdButtons(1-29)pages/lib/global_tab_names.py (1)
TabNames(1-47)pages/lib/utils.py (7)
dropdown(311-324)title_with_link(183-211)title_with_tooltip(154-180)generate_units_degree(57-58)generate_chart_name(30-46)generate_units(49-54)summary_table_tmp_rh_tab(214-297)pages/lib/layout.py (1)
apply_global_month_hour_filter(565-621)
pages/lib/template_graphs.py (2)
pages/lib/utils.py (10)
get_max_min_value(327-345)has_filtered_data(431-432)get_variable_info(435-442)get_variable_range(451-464)get_original_column_values(467-472)calculate_daily_statistics(475-481)unpack_variable_info(445-448)code_timer(15-27)determine_month_and_hour_filter(300-308)separate_filtered_data(399-428)pages/lib/global_variables.py (7)
Variables(104-548)VariableInfo(63-101)IP(55-59)from_col_name(95-101)get_color(90-92)get_unit(78-82)get_name(74-76)
pages/wind.py (7)
pages/lib/global_element_ids.py (1)
ElementIds(4-241)pages/lib/template_graphs.py (2)
heatmap(822-933)wind_rose(949-1080)pages/lib/global_variables.py (1)
Variables(104-548)pages/lib/global_id_buttons.py (1)
IdButtons(1-29)pages/lib/global_tab_names.py (1)
TabNames(1-47)pages/lib/utils.py (3)
title_with_link(183-211)generate_units(49-54)generate_chart_name(30-46)pages/lib/layout.py (1)
apply_global_month_hour_filter(565-621)
pages/sun.py (7)
pages/lib/global_element_ids.py (1)
ElementIds(4-241)pages/lib/global_variables.py (2)
Variables(104-548)IP(55-59)pages/lib/global_id_buttons.py (1)
IdButtons(1-29)pages/lib/global_tab_names.py (1)
TabNames(1-47)pages/lib/utils.py (5)
title_with_link(183-211)dropdown(311-324)generate_units(49-54)generate_chart_name(30-46)generate_custom_inputs(61-68)pages/lib/layout.py (1)
apply_global_month_hour_filter(565-621)pages/lib/charts_sun.py (3)
monthly_solar(22-260)polar_graph(263-600)custom_cartesian_solar(603-908)
pages/explorer.py (7)
pages/lib/global_element_ids.py (1)
ElementIds(4-241)pages/lib/global_variables.py (1)
Variables(104-548)pages/lib/global_id_buttons.py (1)
IdButtons(1-29)pages/lib/global_tab_names.py (1)
TabNames(1-47)pages/lib/utils.py (9)
dropdown(311-324)title_with_link(183-211)title_with_tooltip(154-180)generate_chart_name(30-46)generate_custom_inputs(61-68)generate_units(49-54)get_global_filter_state(362-379)determine_month_and_hour_filter(300-308)summary_table_tmp_rh_tab(214-297)pages/lib/layout.py (1)
apply_global_month_hour_filter(565-621)pages/lib/template_graphs.py (4)
yearly_profile(93-466)daily_profile(470-643)heatmap(822-933)filter_df_by_month_and_hour(1538-1565)
🪛 LanguageTool
docs/contributing/contributing.md
[uncategorized] ~15-~15: Use a comma before “but” if it connects two independent clauses (unless they are closely connected and short).
Context: ...our project, please do not open an issue but instead please fill in this [form](http...
(COMMA_COMPOUND_SENTENCE_3)
[typographical] ~15-~15: Consider adding a comma here.
Context: ... please do not open an issue but instead please fill in this [form](https://forms.gle/L...
(PLEASE_COMMA)
[uncategorized] ~19-~19: The official name of this software platform is spelled with a capital “H”.
Context: ... fork the origin repository to your own github repository, then clone the repository t...
(GITHUB)
[uncategorized] ~73-~73: Possible missing comma found.
Context: ...git checkout -b (your branch name) ``` Finally update and push to your repository bran...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~153-~153: Loose punctuation mark.
Context: ... Common Commit Types: - Main (Master): Stable branch, merge code that passes r...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~155-~155: Loose punctuation mark.
Context: ...th multiple collaborators. - Feature/*: feature development branch, cut out fro...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~156-~156: Loose punctuation mark.
Context: ... after completing the feature. - Fix/*: defect repair branch, the same process ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~158-~158: Loose punctuation mark.
Context: ...oing regressions and tagging. - docs/*, chore/*, refactor/*, test/*: docu...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~159-~159: Loose punctuation mark.
Context: ... refactor, test type branches. - Style: style modification (does not affect the...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~160-~160: Loose punctuation mark.
Context: ...stment, naming rules unity. - Refactor: Code Refactoring: Refactor existing cod...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~161-~161: Loose punctuation mark.
Context: ...ode to improve maintainability. - Test: Add or modify tests: add unit tests, in...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~162-~162: Loose punctuation mark.
Context: ...n tests, or modify test logic. - Chore: Build Configuration, Dependency Managem...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~163-~163: Loose punctuation mark.
Context: ...t, CI/CD Configuration Updates. - Perf: Performance Optimisation: Optimising co...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~164-~164: Loose punctuation mark.
Context: ...ution efficiency or memory usage. - Ci: CI Configuration Related: Changing Cont...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~164-~164: The official name of this software platform is spelled with a capital “H”.
Context: ...ntinuous Integration Configurations for Github Actions, Travis, Jenkins, etc. - `Build...
(GITHUB)
[uncategorized] ~166-~166: Loose punctuation mark.
Context: ...pts, packaging configuration. - Revert: Rollback Commit: Undoing a Previous Com...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~168-~168: Loose punctuation mark.
Context: ...ependencies to prevent attacks. - Deps: Dependency Management: Dependency Manag...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~175-~175: Do not mix variants of the same word (‘minimise’ and ‘minimize’) within a single text.
Context: ...s has been reached. - Changes have been minimised (small steps/phased submission) to avoi...
(EN_WORD_COHERENCY)
🪛 markdownlint-cli2 (0.18.1)
docs/contributing/contributing.md
55-55: Hard tabs
Column: 3
(MD010, no-hard-tabs)
56-56: Hard tabs
Column: 3
(MD010, no-hard-tabs)
57-57: Hard tabs
Column: 3
(MD010, no-hard-tabs)
82-82: Link text should be descriptive
(MD059, descriptive-link-text)
docs/documentation/tabs-explained/psychrometric-chart/README.md
25-25: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pytest
🔇 Additional comments (26)
docs/documentation/tabs-explained/tab-summary/degree-days-explained.md (1)
15-15: Clarify the purpose of this no-op change.Line 15 appears to be a no-op modification (removed and re-added identically per the AI summary), and this documentation file appears unrelated to the PR's stated objective about filtering data graying out part of the chart. Please confirm whether this change was intentional or if it was included by accident.
docs/documentation/tabs-explained/psychrometric-chart/README.md (1)
25-25: Static analysis warning appears to be a false positive.The line contains a Gitbook embed tag (
{% embed %}), not a bare URL in markdown syntax. Markdownlint's warning (MD034) is incorrectly flagging the URL inside the embed directive.Since this is a formatting-only change (removing trailing whitespace) and the embed syntax is correct for Gitbook, this can be safely merged without modification.
docs/documentation/tabs-explained/sun-and-cloud/cloud-coverage.md (1)
21-21: Image asset verified; documentation change is valid.The image asset at the referenced path exists and resolves correctly. The documentation change is properly formatted with a valid asset reference.
Note: While this is a valid documentation update, verify with the PR author whether including this file in a PR focused on chart filtering/graying is intentional.
docs/documentation/tabs-explained/sun-and-cloud/global-and-diffuse-horizontal-solar-radiation/README.md (1)
14-15: Documentation formatting only.This change is a trailing newline addition with no semantic impact on documentation content.
docs/documentation/tabs-explained/psychrometric-chart/psychrometric-chart-explained.md (1)
30-31: Documentation formatting only.This change is a trailing newline addition with no semantic impact on documentation content.
docs/documentation/tabs-explained/outdoor-comfort/utci-explained.md (1)
20-21: Documentation formatting only.This change is a trailing newline addition with no semantic impact on documentation content.
docs/documentation/tabs-explained/tab-summary/climate-profiles-explained.md (1)
18-18: No-op image reference change.The image reference line was removed and re-added identically; this appears to be a formatting or merge artifact with no semantic impact.
assets/manifest.json (1)
470-470: Version bump is consistent with release.The manifest "id" field has been correctly updated to align with the 0.10.0 release.
Pipfile (1)
7-7: Verify Dash 3.2 and dash-mantine-components 2.2.1 compatibility.The upgrade from Dash 2.15 to 3.2 and from dash-mantine-components 0.12.1 to 2.2.1 are major version changes. These upgrades likely introduce breaking changes that need to be validated in the application code (not provided in the current review context).
Confirm that:
- Application code properly handles Dash 3.2 breaking changes
- dash-mantine-components 2.2.1 API changes are reflected throughout the codebase
- No compatibility issues exist with other pinned dependencies (e.g., plotly==5.18.0, pandas==2.2.0)
Also applies to: 12-12
docs/README.md (1)
37-42: New contributors added with consistent formatting.The six new contributors have been properly added to the contributions list with consistent formatting and roles. Note that some contributors lack associated URLs (GitHub or LinkedIn), which is consistent with pre-existing patterns in the contributor list (see line 32: Yunzhu Ji).
Please verify that:
- All contributor names and profiles are accurate
- Any missing profile URLs (for Wenshu lyu, Ziqi Liu, Tianchi Liu, Feng Wang) are intentional or should be added
docs/documentation/tabs-explained/tab-summary/clima-dataframe.md (1)
32-32: No-op variable reference change.The "Saturation pressure" bullet was removed and re-added identically; this appears to be a merge or rebase artifact with no semantic impact.
.bumpversion.cfg (1)
2-2: LGTM! Version bump is consistent with PR scope.The version increment to 0.10.0 aligns with the major changes in this PR (testing framework migration, UI library migration, global filters).
.github/workflows/python.yml (3)
34-36: LGTM! Efficient browser installation.Installing only chromium keeps CI execution time minimal while providing adequate test coverage.
42-44: LGTM! Robust health check implementation.The 60-second timeout with curl health checks is appropriate for ensuring the service is ready before running tests.
48-49: LGTM! Clean test execution setup.The test command properly isolates the test directory and passes the base URL as a parameter.
pages/not_found_404.py (1)
16-16: LGTM! Proper Dash Mantine Components API usage.The updates from
className="mb-2"tomb="sm"andleftIcontoleftSectionalign with the Dash Mantine Components migration mentioned in the PR summary.Also applies to: 30-30
docs/version/changelog.md (1)
3-16: LGTM! Comprehensive changelog entry.The Version 0.9.0 entry appropriately documents the migration to Playwright, UI changes, and bug fixes. The date (2025-10-30) is consistent with this PR being prepared for the next version (0.10.0).
docs/contributing/run-project-locally.md (1)
119-119: LGTM! Better syntax highlighting.Changing code blocks from
texttobashimproves readability and provides proper syntax highlighting.Also applies to: 125-125
Dockerfile (4)
5-6: LGTM! Essential for containerized logging.
PYTHONUNBUFFERED=Trueensures that Python logs appear immediately in Cloud Run/Docker logs without buffering.
8-11: LGTM! Efficient package installation.The consolidated apt-get command with
--no-install-recommendsand cleanup in the same layer follows Docker best practices for minimal image size.
16-25: LGTM! Optimal layer caching strategy.Copying
PipfileandPipfile.lockbefore the application code ensures that dependency installation is cached and only re-runs when dependencies change. This significantly speeds up builds during development.
29-29: LGTM! Consistent with Pipenv workflow.The CMD correctly uses
pipenv runto execute within the Pipenv-managed environment. If you adopt the--systemflag suggestion above, this should be updated toCMD ["python", "main.py"].tests/conftest.py (1)
4-6: LGTM!The session-scoped
base_urlfixture is correctly implemented and provides a clean way to access the application base URL across all tests.pages/lib/charts_summary.py (1)
8-12: Excellent refactoring to centralized constants.The migration from hardcoded dictionary keys to Variables constants improves maintainability and consistency across the codebase. This change aligns well with the PR's goal of introducing centralized global constants.
tests/test_wind.py (1)
38-46: The review comment is incorrect—"Sep and Dec" is the correct expected value.The implementation in
pages/wind.py(lines 350-353) explicitly definesfall_months = [9, 12], which correctly corresponds to September and December. The test expectations intests/test_wind.pyline 42 accurately reflect this. December appearing in both winter and fall (seewinter_months = [12, 2]on line 350) is intentional and represents a valid meteorological/seasonal convention where months overlap or serve as boundary markers.Likely an incorrect or invalid review comment.
pages/lib/global_element_ids.py (1)
229-241: Well-structured tools menu section.This section demonstrates good practices with consistent prefixing (
TOOLS_for enum names,"tools-"for values) and clear delineation. All IDs are unique within this section, which contrasts with the duplicate issues found earlier in the file.
| if global_filter_data and global_filter_data.get("filter_active", False): | ||
| from pages.lib.layout import apply_global_month_hour_filter | ||
|
|
||
| df = apply_global_month_hour_filter(df, global_filter_data) | ||
| else: | ||
| # Use local filtering when global filter is not active | ||
| df = filter_df_by_month_and_hour(df, True, [1, 12], [0, 24], [], [], df.columns) |
There was a problem hiding this comment.
Global filter isn’t applied to the scatter inputs
apply_global_month_hour_filter defaults to filtering only DBT. Because we don’t pass var_x, var_y, color_by (or the optional data-filter variable), rows outside the global month/hour window still carry valid values for those fields, so the two-/three-variable charts continue to plot the unfiltered data.
Please forward the plotted columns into the filter call.
- df = apply_global_month_hour_filter(df, global_filter_data)
+ target_cols = {
+ var_x,
+ var_y,
+ color_by,
+ data_filter_var,
+ }
+ target_cols.discard(None)
+ df = apply_global_month_hour_filter(
+ df,
+ global_filter_data,
+ list(target_cols),
+ )
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
tests/test_summary.py (1)
91-93: Call the Playwright assertion methods.The assertions reference
to_be_visiblewithout calling it, so the expectations never run and the test passes even if the unit switch fails.Apply this diff to invoke the assertion methods:
- expect(info_section.get_by_text("°F")).to_be_visible - expect(info_section.get_by_text("ft")).to_be_visible - expect(info_section.get_by_text("kBtu/ft2")).to_be_visible + expect(info_section.get_by_text("°F")).to_be_visible() + expect(info_section.get_by_text("ft")).to_be_visible() + expect(info_section.get_by_text("kBtu/ft2")).to_be_visible()pages/explorer.py (2)
631-636: Scatter plots ignore the global filter (still outstanding).This is the same gap flagged earlier: calling
apply_global_month_hour_filterwithout the plotted columns means onlyDBTgets masked.var_x,var_y,color_by, and the optional filter variable keep their values, so both scatter charts continue to render the unfiltered rows. Please forward every plotted column into the filter.- df = apply_global_month_hour_filter(df, global_filter_data) + target_cols = {var_x, var_y, color_by, data_filter_var} + target_cols.discard(None) + df = apply_global_month_hour_filter( + df, + global_filter_data, + list(target_cols), + )
449-453: Heatmap still shows unfiltered data.Same issue here: without supplying the chosen
var, the global month/hour filter only blanksDBT, leaving the actual heatmap variable untouched. Users therefore see gray overlay plus the full dataset, defeating the filter. Pass the selected variable intoapply_global_month_hour_filter.- df = apply_global_month_hour_filter(df, global_filter_data) + df = apply_global_month_hour_filter(df, global_filter_data, var)
🧹 Nitpick comments (5)
docs/contributing/contributing.md (5)
54-58: Replace hard tabs with spaces for markdown consistency.Lines 55–57 use hard tabs for indentation; markdown linters prefer spaces. This is a minor formatting fix but helps maintain consistency across the documentation.
Replace the hard tabs with spaces (typically 2–4 spaces per indent level) in the git output block.
19-19: Capitalize "GitHub" consistently.Lines 19 and 164 refer to "github" and "Github" respectively; the official brand is "GitHub" (always capitalized). Apply this minor polish for consistency and brand respect.
- your own github repository + your own GitHub repository- - `Ci`: CI Configuration Related: Changing Continuous Integration Configurations for Github Actions, Travis, Jenkins, etc. + - `Ci`: CI Configuration Related: Changing Continuous Integration Configurations for GitHub Actions, Travis, Jenkins, etc.Also applies to: 164-164
82-82: Use descriptive link text for accessibility.Link text "here" is not descriptive for screen readers. Consider a more informative label.
- Available [here](code_of_conduct.md) + Review our [Code of Conduct](code_of_conduct.md)
15-15: Minor grammar: Add comma before "but".Consider adding a comma before "but" for clarity: "...do not open an issue, but instead please fill in this form..."
- If you have a general feedback about our project, please do not open an issue but instead please fill in this [form](https://forms.gle/LRUq3vsFnE1QCLiA6) + If you have a general feedback about our project, please do not open an issue, but instead please fill in this [form](https://forms.gle/LRUq3vsFnE1QCLiA6)
175-175: Standardize spelling to American English.Line 175 uses "minimised" (British); the rest of the documentation uses American English. Consider standardizing to "minimized" for consistency.
- - Changes have been minimised (small steps/phased submission) to avoid "mega PRs". + - Changes have been minimized (small steps/phased submission) to avoid "mega PRs".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
docs/contributing/contributing.md(3 hunks)docs/contributing/run-project-locally.md(1 hunks)pages/explorer.py(11 hunks)pages/lib/global_element_ids.py(1 hunks)pages/lib/layout.py(1 hunks)pages/natural_ventilation.py(9 hunks)pages/outdoor.py(10 hunks)pages/psy-chart.py(9 hunks)pages/select.py(11 hunks)pages/summary.py(6 hunks)pages/sun.py(3 hunks)pages/t_rh.py(2 hunks)pages/wind.py(10 hunks)tests/test_select.py(1 hunks)tests/test_summary.py(1 hunks)tests/test_t_rh.py(1 hunks)tests/utils.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/contributing/run-project-locally.md
- tests/test_select.py
- tests/utils.py
- tests/test_t_rh.py
🧰 Additional context used
🧬 Code graph analysis (11)
pages/summary.py (5)
pages/lib/charts_summary.py (1)
world_map(6-37)pages/lib/template_graphs.py (1)
violin(22-89)pages/lib/global_variables.py (4)
Variables(104-548)VariableInfo(63-101)from_col_name(95-101)get_unit(78-82)pages/lib/utils.py (3)
generate_chart_name(30-46)generate_units_degree(57-58)generate_units(49-54)pages/lib/layout.py (1)
apply_global_month_hour_filter(565-621)
pages/psy-chart.py (7)
pages/lib/utils.py (7)
get_max_min_value(327-345)separate_filtered_data(399-428)dropdown(311-324)title_with_link(183-211)get_global_filter_state(362-379)determine_month_and_hour_filter(300-308)generate_chart_name(30-46)pages/lib/global_element_ids.py (1)
ElementIds(4-202)pages/lib/global_variables.py (7)
Variables(104-548)VariableInfo(63-101)from_col_name(95-101)get_unit(78-82)get_name(74-76)get_color(90-92)get_range(84-88)pages/lib/global_id_buttons.py (1)
IdButtons(1-29)pages/lib/global_tab_names.py (1)
TabNames(1-47)pages/lib/layout.py (1)
apply_global_month_hour_filter(565-621)pages/lib/template_graphs.py (1)
filter_df_by_month_and_hour(1538-1565)
pages/lib/layout.py (5)
pages/lib/global_variables.py (2)
Variables(104-548)IP(55-59)config.py (2)
DocLinks(82-96)UnitSystem(16-20)pages/lib/global_element_ids.py (1)
ElementIds(4-202)pages/lib/utils.py (3)
determine_month_and_hour_filter(300-308)get_default_global_filter_store_data(348-359)get_global_filter_state(362-379)pages/lib/template_graphs.py (1)
time_filtering(1515-1535)
pages/wind.py (7)
pages/lib/global_element_ids.py (1)
ElementIds(4-202)pages/lib/template_graphs.py (2)
heatmap(822-933)wind_rose(949-1080)pages/lib/global_variables.py (1)
Variables(104-548)pages/lib/global_id_buttons.py (1)
IdButtons(1-29)pages/lib/global_tab_names.py (1)
TabNames(1-47)pages/lib/utils.py (3)
title_with_link(183-211)generate_units(49-54)generate_chart_name(30-46)pages/lib/layout.py (1)
apply_global_month_hour_filter(565-621)
pages/t_rh.py (8)
pages/lib/template_graphs.py (3)
heatmap(822-933)yearly_profile(93-466)daily_profile(470-643)pages/lib/global_variables.py (1)
Variables(104-548)pages/lib/global_element_ids.py (1)
ElementIds(4-202)pages/lib/global_id_buttons.py (1)
IdButtons(1-29)pages/lib/global_tab_names.py (1)
TabNames(1-47)pages/lib/utils.py (6)
dropdown(311-324)title_with_link(183-211)title_with_tooltip(154-180)generate_chart_name(30-46)generate_units(49-54)summary_table_tmp_rh_tab(214-297)pages/explorer.py (2)
layout(63-68)update_table(697-726)pages/lib/layout.py (1)
apply_global_month_hour_filter(565-621)
tests/test_summary.py (2)
tests/utils.py (2)
upload_epw_file(5-23)open_tab(26-40)tests/conftest.py (1)
base_url(5-6)
pages/select.py (5)
pages/lib/extract_df.py (4)
convert_df_units(507-521)create_df(145-440)get_data(26-49)get_location_info(53-77)pages/lib/global_variables.py (1)
Variables(104-548)pages/lib/global_element_ids.py (1)
ElementIds(4-202)pages/lib/global_tab_names.py (1)
TabNames(1-47)pages/lib/utils.py (2)
generate_chart_name(30-46)get_default_global_filter_store_data(348-359)
pages/outdoor.py (7)
config.py (1)
DocLinks(82-96)pages/lib/global_element_ids.py (1)
ElementIds(4-202)pages/lib/global_variables.py (1)
Variables(104-548)pages/lib/global_id_buttons.py (1)
IdButtons(1-29)pages/lib/global_tab_names.py (1)
TabNames(1-47)pages/lib/utils.py (6)
get_time_filter_from_store(382-396)dropdown(311-324)title_with_link(183-211)title_with_tooltip(154-180)generate_units_degree(57-58)generate_chart_name(30-46)pages/lib/layout.py (1)
apply_global_month_hour_filter(565-621)
pages/natural_ventilation.py (6)
pages/lib/global_variables.py (7)
Variables(104-548)VariableInfo(63-101)from_col_name(95-101)get_unit(78-82)get_name(74-76)get_color(90-92)IP(55-59)pages/lib/global_element_ids.py (1)
ElementIds(4-202)pages/lib/global_id_buttons.py (1)
IdButtons(1-29)pages/lib/global_tab_names.py (1)
TabNames(1-47)pages/lib/utils.py (4)
separate_filtered_data(399-428)get_global_filter_state(362-379)determine_month_and_hour_filter(300-308)generate_chart_name(30-46)pages/lib/layout.py (1)
apply_global_month_hour_filter(565-621)
pages/sun.py (7)
pages/lib/global_element_ids.py (1)
ElementIds(4-202)pages/lib/global_variables.py (2)
Variables(104-548)IP(55-59)pages/lib/global_id_buttons.py (1)
IdButtons(1-29)pages/lib/global_tab_names.py (1)
TabNames(1-47)pages/lib/utils.py (4)
title_with_link(183-211)dropdown(311-324)generate_units(49-54)generate_chart_name(30-46)pages/lib/layout.py (1)
apply_global_month_hour_filter(565-621)pages/lib/charts_sun.py (3)
monthly_solar(22-260)polar_graph(263-600)custom_cartesian_solar(603-908)
pages/explorer.py (7)
pages/lib/global_element_ids.py (1)
ElementIds(4-202)pages/lib/global_variables.py (1)
Variables(104-548)pages/lib/global_id_buttons.py (1)
IdButtons(1-29)pages/lib/global_tab_names.py (1)
TabNames(1-47)pages/lib/utils.py (9)
dropdown(311-324)title_with_link(183-211)title_with_tooltip(154-180)generate_chart_name(30-46)generate_custom_inputs(61-68)generate_units(49-54)get_global_filter_state(362-379)determine_month_and_hour_filter(300-308)summary_table_tmp_rh_tab(214-297)pages/lib/layout.py (1)
apply_global_month_hour_filter(565-621)pages/lib/template_graphs.py (4)
yearly_profile(93-466)daily_profile(470-643)heatmap(822-933)filter_df_by_month_and_hour(1538-1565)
🪛 LanguageTool
docs/contributing/contributing.md
[uncategorized] ~15-~15: Use a comma before “but” if it connects two independent clauses (unless they are closely connected and short).
Context: ...our project, please do not open an issue but instead please fill in this [form](http...
(COMMA_COMPOUND_SENTENCE_3)
[typographical] ~15-~15: Consider adding a comma here.
Context: ... please do not open an issue but instead please fill in this [form](https://forms.gle/L...
(PLEASE_COMMA)
[uncategorized] ~19-~19: The official name of this software platform is spelled with a capital “H”.
Context: ... fork the origin repository to your own github repository, then clone the repository t...
(GITHUB)
[uncategorized] ~73-~73: Possible missing comma found.
Context: ...git checkout -b ``` Finally update and push to your repository bran...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~153-~153: Loose punctuation mark.
Context: ... Common Commit Types: - Main (Master): Stable branch, merge code that passes r...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~155-~155: Loose punctuation mark.
Context: ...th multiple collaborators. - Feature/*: feature development branch, cut out fro...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~156-~156: Loose punctuation mark.
Context: ... after completing the feature. - Fix/*: defect repair branch, the same process ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~158-~158: Loose punctuation mark.
Context: ...oing regressions and tagging. - docs/*, chore/*, refactor/*, test/*: docu...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~159-~159: Loose punctuation mark.
Context: ... refactor, test type branches. - Style: style modification (does not affect the...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~160-~160: Loose punctuation mark.
Context: ...stment, naming rules unity. - Refactor: Code Refactoring: Refactor existing cod...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~161-~161: Loose punctuation mark.
Context: ...ode to improve maintainability. - Test: Add or modify tests: add unit tests, in...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~162-~162: Loose punctuation mark.
Context: ...n tests, or modify test logic. - Chore: Build Configuration, Dependency Managem...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~163-~163: Loose punctuation mark.
Context: ...t, CI/CD Configuration Updates. - Perf: Performance Optimisation: Optimising co...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~164-~164: Loose punctuation mark.
Context: ...ution efficiency or memory usage. - Ci: CI Configuration Related: Changing Cont...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~164-~164: The official name of this software platform is spelled with a capital “H”.
Context: ...ntinuous Integration Configurations for Github Actions, Travis, Jenkins, etc. - `Build...
(GITHUB)
[uncategorized] ~166-~166: Loose punctuation mark.
Context: ...pts, packaging configuration. - Revert: Rollback Commit: Undoing a Previous Com...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~168-~168: Loose punctuation mark.
Context: ...ependencies to prevent attacks. - Deps: Dependency Management: Dependency Manag...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~175-~175: Do not mix variants of the same word (‘minimise’ and ‘minimize’) within a single text.
Context: ...s has been reached. - Changes have been minimised (small steps/phased submission) to avoi...
(EN_WORD_COHERENCY)
🪛 markdownlint-cli2 (0.18.1)
docs/contributing/contributing.md
55-55: Hard tabs
Column: 3
(MD010, no-hard-tabs)
56-56: Hard tabs
Column: 3
(MD010, no-hard-tabs)
57-57: Hard tabs
Column: 3
(MD010, no-hard-tabs)
82-82: Link text should be descriptive
(MD059, descriptive-link-text)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pytest
🔇 Additional comments (3)
docs/contributing/contributing.md (1)
17-78: Previous issues have been resolved—LGTM!The git commands and examples now use angle-bracket placeholders (
<your-account-name>,<your-branch-name>) instead of spaces or parentheses, making them paste-friendly and correct. The typo ingit checkouthas been fixed. These changes align well with contributor usability.pages/lib/global_element_ids.py (1)
1-202: LGTM: Centralized ID constants follow a clean pattern.The consolidation of shared store IDs (SHARED_DF_STORE, SHARED_META_STORE, etc.) addresses the previous concern about duplicate values. The enum structure provides type safety and a single source of truth for element IDs across the application.
pages/wind.py (1)
500-505: Confirmed: Noon wind rose bug has been fixed.The previous critical issue where
noon_dfincorrectly filtered onmorning_timeshas been resolved. The code now correctly usesnoon_times[0]andnoon_times[1]to select hours 14-21 for the noon wind rose.
| if global_filter_data and global_filter_data.get("filter_active", False): | ||
| from pages.lib.layout import apply_global_month_hour_filter | ||
|
|
||
| df = apply_global_month_hour_filter(df, global_filter_data) | ||
|
|
There was a problem hiding this comment.
Global filter bypasses the daily chart variable.
apply_global_month_hour_filter defaults to masking only Variables.DBT. Because we don’t pass the selected var, any variable other than DBT (e.g. humidity, solar radiation) keeps its original data, so the daily profile continues to plot the unfiltered time range. Forward the dropdown selection into the filter call.
- df = apply_global_month_hour_filter(df, global_filter_data)
+ df = apply_global_month_hour_filter(df, global_filter_data, var)🤖 Prompt for AI Agents
In pages/explorer.py around lines 415–419 the global filter is applied without
passing the currently selected variable from the dropdown, so
apply_global_month_hour_filter only masks Variables.DBT by default and other
variables remain unfiltered; forward the selected variable (the dropdown value
used to pick the daily chart variable—e.g., var or selected_var in this file) as
an argument into apply_global_month_hour_filter when calling it, and update the
call site (and adjust the apply_global_month_hour_filter signature if necessary)
so the function uses that passed variable to build its mask instead of
defaulting to DBT.
| def _safe_get_column(df, column_name, default_value=0): | ||
| if column_name in df.columns: | ||
| return df[column_name] | ||
| else: | ||
| return [default_value] * len(df) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused helper function.
The _safe_get_column function is defined but never called anywhere in this file. Remove it to reduce clutter.
-def _safe_get_column(df, column_name, default_value=0):
- if column_name in df.columns:
- return df[column_name]
- else:
- return [default_value] * len(df)
-
-📝 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.
| def _safe_get_column(df, column_name, default_value=0): | |
| if column_name in df.columns: | |
| return df[column_name] | |
| else: | |
| return [default_value] * len(df) |
🤖 Prompt for AI Agents
In pages/psy-chart.py around lines 37 to 42, the helper function
_safe_get_column is defined but never used; delete the entire function
definition (the def block) and any now-unnecessary surrounding blank lines to
remove clutter; run tests/lint to ensure no references remain and commit the
removal.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pages/lib/layout.py (2)
82-92: Consider aligning the hour slider range with the data range.The hour slider range is [0, 24], but according to the Variables definition, the data contains hours [1, 24]. While the filtering logic handles this correctly (since hour 0 never exists in the data), the mismatch could confuse users who expect the slider to reflect actual data bounds.
Consider updating the slider to match the data:
dcc.RangeSlider( id=ElementIds.TOOLS_HOUR_SLIDER, - min=0, + min=1, max=24, step=1, - value=[0, 24], - marks={0: "0", 24: "24"}, + value=[1, 24], + marks={1: "1", 24: "24"}, tooltip={
489-500: Simplify redundant width logic.All widths in the WIDTHS dictionary are set to 230, making the conditional logic redundant. If these values are placeholders for future differentiation, consider adding a comment. Otherwise, simplify to a single constant.
-WIDTHS = {"default": 230, "pages": 230, "tools": 230} +WIDTH = 230 if tools_opened is not None: tools_expanded = tools_opened - navbar["width"] = ( - WIDTHS["tools"] - if tools_opened - else (WIDTHS["pages"] if pages_opened else WIDTHS["default"]) - ) + navbar["width"] = WIDTH elif pages_opened is not None: - navbar["width"] = WIDTHS["pages"] if pages_opened else WIDTHS["default"] + navbar["width"] = WIDTH
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.bumpversion.cfg(1 hunks)assets/manifest.json(1 hunks)pages/lib/layout.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- assets/manifest.json
- .bumpversion.cfg
🧰 Additional context used
🧬 Code graph analysis (1)
pages/lib/layout.py (5)
pages/lib/global_variables.py (2)
Variables(104-548)IP(55-59)config.py (2)
DocLinks(82-96)UnitSystem(16-20)pages/lib/global_element_ids.py (1)
ElementIds(4-202)pages/lib/utils.py (3)
determine_month_and_hour_filter(300-308)get_default_global_filter_store_data(348-359)get_global_filter_state(362-379)pages/lib/template_graphs.py (1)
time_filtering(1515-1535)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pytest
| for target_col in target_columns: | ||
| df_copy[f"_{target_col}_original"] = df_copy[target_col] | ||
|
|
||
| from pages.lib.template_graphs import time_filtering | ||
|
|
||
| time_filtering( | ||
| df_copy, start_month, end_month, Variables.MONTH.col_name, target_col | ||
| ) | ||
| time_filtering( | ||
| df_copy, start_hour, end_hour, Variables.HOUR.col_name, target_col | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move the import statement outside the loop.
The import of time_filtering is inside the for loop, causing it to be executed on every iteration. While Python caches imports, this is inefficient and violates style conventions.
Move the import to the top of the function:
+from pages.lib.template_graphs import time_filtering
+
for target_col in target_columns:
df_copy[f"_{target_col}_original"] = df_copy[target_col]
- from pages.lib.template_graphs import time_filtering
-
time_filtering(
df_copy, start_month, end_month, Variables.MONTH.col_name, target_col
)📝 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.
| for target_col in target_columns: | |
| df_copy[f"_{target_col}_original"] = df_copy[target_col] | |
| from pages.lib.template_graphs import time_filtering | |
| time_filtering( | |
| df_copy, start_month, end_month, Variables.MONTH.col_name, target_col | |
| ) | |
| time_filtering( | |
| df_copy, start_hour, end_hour, Variables.HOUR.col_name, target_col | |
| ) | |
| from pages.lib.template_graphs import time_filtering | |
| for target_col in target_columns: | |
| df_copy[f"_{target_col}_original"] = df_copy[target_col] | |
| time_filtering( | |
| df_copy, start_month, end_month, Variables.MONTH.col_name, target_col | |
| ) | |
| time_filtering( | |
| df_copy, start_hour, end_hour, Variables.HOUR.col_name, target_col | |
| ) |
🤖 Prompt for AI Agents
In pages/lib/layout.py around lines 609 to 619, the import of time_filtering is
currently inside the for loop which is inefficient and against style
conventions; move the import statement (from pages.lib.template_graphs import
time_filtering) to the top of the enclosing function (or module top if more
appropriate) and remove the in-loop import so the loop only calls
time_filtering(df_copy, start_month, end_month, Variables.MONTH.col_name,
target_col) and time_filtering(df_copy, start_hour, end_hour,
Variables.HOUR.col_name, target_col).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_summary.py (2)
56-68: Consider extracting expected values to constants.The hard-coded expected values are tightly coupled to the test EPW file. While acceptable for integration tests, extracting these to module-level constants or a fixture would improve maintainability if multiple tests need to reference the same values.
Example refactor:
BOLOGNA_LOCATION_INFO = { "location": "Bologna Marconi AP, ITA", "longitude": "11.2969", "latitude": "44.5308", "elevation": "37.0 m", # ... other fields } def test_location_info_loaded(page: Page): info_section = page.locator("#location-info") expect(info_section).to_be_visible() for key, value in BOLOGNA_LOCATION_INFO.items(): expect(info_section).to_contain_text(value)
80-81: Remove commented-out debug code.The commented-out navigation code appears to be leftover debug or experimental code that is no longer needed.
Apply this diff:
- # nav_controls = page.locator("#nav-group-controls") - # nav_controls.click(force=True) - # Click the "IP" option
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_summary.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_summary.py (2)
tests/utils.py (2)
upload_epw_file(5-23)open_tab(26-40)tests/conftest.py (1)
base_url(5-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pytest
🔇 Additional comments (4)
tests/test_summary.py (4)
6-12: LGTM! Clean setup fixture.The autouse fixture correctly initializes test state by navigating to the base URL, uploading the EPW file, and opening the Climate Summary tab. The use of helper utilities from
tests/utils.pypromotes good code reuse.
16-47: LGTM! Comprehensive element visibility test.The test effectively verifies all core UI elements using appropriate Playwright selectors. The loop for checking profile graphs (lines 40-47) follows the DRY principle.
84-88: Verify the necessity offorce=Truein click operation.The
force=Trueflag bypasses Playwright's actionability checks (visibility, stability, etc.). While this may be an intentional workaround for UI timing issues, it can mask real problems like overlapping elements or animations.Consider whether the preceding
scroll_into_view_if_needed()andwait_for(state="visible")calls are sufficient to make the element actionable without forcing the click. If the element is genuinely actionable, removingforce=Truewould make the test more robust.If
force=Trueis necessary, please add a comment explaining why (e.g., "force needed due to overlay animation").
91-93: LGTM! Assertions are correctly invoked.The Playwright assertions properly call
to_be_visible()with parentheses, ensuring the checks execute as expected. This addresses the critical issue flagged in the previous review.
| """Check if location info section displays properly""" | ||
| """Verify that location info section shows correct values""" |
There was a problem hiding this comment.
Remove duplicate docstring.
Two consecutive docstrings are present. Keep only one descriptive docstring.
Apply this diff:
def test_location_info_loaded(page: Page):
- """Check if location info section displays properly"""
"""Verify that location info section shows correct values"""📝 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.
| """Check if location info section displays properly""" | |
| """Verify that location info section shows correct values""" | |
| def test_location_info_loaded(page: Page): | |
| """Verify that location info section shows correct values""" |
🤖 Prompt for AI Agents
In tests/test_summary.py around lines 52 to 53, there are two consecutive
docstrings; remove the duplicate so only one descriptive docstring remains
(choose the clearer one and delete the other), ensuring the remaining docstring
accurately describes the test purpose and keep surrounding indentation and
formatting intact.
Summary by CodeRabbit
New Features
Improvements
Documentation
Other