Fix missing page title when calling action entity_details_show#65
Merged
Fix missing page title when calling action entity_details_show#65
entity_details_show#65Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR replaces many compile-time PAGE/BOOT_STEP substitutions with runtime Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
It should still be resolved at compiling time
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nspanel_easy_blueprint.yaml`:
- Around line 7756-7782: The shared alias &entity_details_title_with_icon
currently writes both page_label and icon_state, causing duplicate writers when
used by pages that already own those fields (e.g.,
nspanel_esphome_page_media_player.yaml owns page_label and
nspanel_esphome_page_alarm.yaml owns both); split this helper into two smaller
aliases (e.g., &entity_details_title and &entity_details_icon) or add guards so
it only writes page_label or icon_state when the target page does not already
define them — update references that call &entity_details_title_with_icon to
call the appropriate new alias(es) instead, and ensure the actions referencing
ids page_label and icon_state and the alias name &entity_details_title_with_icon
are adjusted accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6f67410a-1c64-4590-871a-2cda0fa2c0c8
📒 Files selected for processing (24)
esphome/nspanel_esphome_addon_climate_base.yamlesphome/nspanel_esphome_api.yamlesphome/nspanel_esphome_page_alarm.yamlesphome/nspanel_esphome_page_boot.yamlesphome/nspanel_esphome_page_button.yamlesphome/nspanel_esphome_page_buttons.yamlesphome/nspanel_esphome_page_climate.yamlesphome/nspanel_esphome_page_confirm.yamlesphome/nspanel_esphome_page_cover.yamlesphome/nspanel_esphome_page_entities.yamlesphome/nspanel_esphome_page_fan.yamlesphome/nspanel_esphome_page_home.yamlesphome/nspanel_esphome_page_keyb_num.yamlesphome/nspanel_esphome_page_light.yamlesphome/nspanel_esphome_page_media_player.yamlesphome/nspanel_esphome_page_notification.yamlesphome/nspanel_esphome_page_qrcode.yamlesphome/nspanel_esphome_page_screensaver.yamlesphome/nspanel_esphome_page_settings.yamlesphome/nspanel_esphome_page_utilities.yamlesphome/nspanel_esphome_page_water_heater.yamlesphome/nspanel_esphome_page_weather.yamlesphome/nspanel_esphome_version.yamlnspanel_easy_blueprint.yaml
💤 Files with no reviewable changes (4)
- esphome/nspanel_esphome_page_button.yaml
- esphome/nspanel_esphome_page_cover.yaml
- esphome/nspanel_esphome_page_utilities.yaml
- esphome/nspanel_esphome_page_buttons.yaml
Same thing for some remaining points
Prefer on Blueprint side to keep a common constructor between pages
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
esphome/nspanel_esphome_page_media_player.yaml (1)
73-75: Keepget_page_id()keyed off the existing page-name substitution.Line 14 already centralizes the page name, and Line 195 still uses
${PAGE_MEDIA_PLAYER_NAME}for display events. Hardcoding"media_player"here creates a second source of truth, so a later rename/override will desync the page gate from the event handler andpage_changehook.Proposed fix
- if (current_page_id != get_page_id("media_player")) { + if (current_page_id != get_page_id("${PAGE_MEDIA_PLAYER_NAME}")) { ESP_LOGW("${TAG_PAGE_MEDIA_PLAYER}", "Not on Media Player page"); return; } @@ - - lambda: if (new_page_id == get_page_id("media_player")) page_media_player->execute(); + - lambda: if (new_page_id == get_page_id("${PAGE_MEDIA_PLAYER_NAME}")) page_media_player->execute();Also applies to: 254-254
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@esphome/nspanel_esphome_page_media_player.yaml` around lines 73 - 75, Replace the hardcoded page name literal in the page-check with the centralized substitution variable so the gate stays in sync: use the existing PAGE_MEDIA_PLAYER_NAME substitution when calling get_page_id (i.e., call get_page_id("${PAGE_MEDIA_PLAYER_NAME}") wherever get_page_id("media_player") appears). Update both occurrences (the one comparing current_page_id in the handler and the other at line ~254) so get_page_id, current_page_id, TAG_PAGE_MEDIA_PLAYER and the page_change hook all reference the same "${PAGE_MEDIA_PLAYER_NAME}" substitution.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nspanel_easy_blueprint.yaml`:
- Around line 7756-7782: The helper alias entity_details_title_with_icon
currently writes page_label and icon_state to page: "" which can race with
navigation; change the helper to target a variable page (use page: '{{
entity_details_page }}' instead of empty string) and ensure every call site that
invokes *entity_details_title_with_icon sets entity_details_page beforehand
(e.g., variables: { entity_details_page: '{{ pages.cover }}' } or the
appropriate pages.fan/pages.media_player/etc.) so the header writes are directed
to the intended details page and avoid stale-page races.
---
Nitpick comments:
In `@esphome/nspanel_esphome_page_media_player.yaml`:
- Around line 73-75: Replace the hardcoded page name literal in the page-check
with the centralized substitution variable so the gate stays in sync: use the
existing PAGE_MEDIA_PLAYER_NAME substitution when calling get_page_id (i.e.,
call get_page_id("${PAGE_MEDIA_PLAYER_NAME}") wherever
get_page_id("media_player") appears). Update both occurrences (the one comparing
current_page_id in the handler and the other at line ~254) so get_page_id,
current_page_id, TAG_PAGE_MEDIA_PLAYER and the page_change hook all reference
the same "${PAGE_MEDIA_PLAYER_NAME}" substitution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6c492762-945f-40b6-8457-87f53962937f
📒 Files selected for processing (8)
.test/esphome_common_customizations.yamldocs/api.mddocs/customization.mdesphome/nspanel_esphome_addon_display_light.yamlesphome/nspanel_esphome_page_alarm.yamlesphome/nspanel_esphome_page_media_player.yamlesphome/nspanel_esphome_page_screensaver.yamlnspanel_easy_blueprint.yaml
💤 Files with no reviewable changes (1)
- docs/api.md
🚧 Files skipped from review as they are similar to previous changes (1)
- esphome/nspanel_esphome_page_screensaver.yaml
Page name won't change without sync everywhere, so there's no need for a substitution here
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores
Documentation