Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDefault TFT firmware changed to nspanel_blank.tft; added an "NSPanel Blank" model and HMI (startup script + blank page); reduced several boot Text widths by 2 units; and added docs describing a blank-first TFT installation workflow. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ESPHomeAddOn as ESPHome Add‑on
participant ESP32 as NSPanel (ESP32)
participant Nextion as Nextion Display
User->>ESPHomeAddOn: Initiate TFT upload (select "NSPanel Blank")
ESPHomeAddOn->>ESP32: Upload `nspanel_blank.tft` (serial/OTA)
ESP32->>Nextion: Write blank HMI to display
Note right of ESP32: Boots running new Program.s and blank page
User->>ESPHomeAddOn: Later upload regional TFT (EU/US/CJK/etc.)
ESPHomeAddOn->>ESP32: Upload selected regional .tft
ESP32->>Nextion: Replace blank HMI with regional HMI
Note right of Nextion: Final regional UI active
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ff45ceeae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
esphome/nspanel_esphome_addon_upload_tft.yaml (1)
215-222:⚠️ Potential issue | 🔴 CriticalOff-by-one error in index boundary check causes wrong TFT for CJK US Landscape.
The condition
idx <= 6excludes the last option "NSPanel US Landscape (CJK languages)" (index 7). When users select this option, the code falls through to the else branch and uses${nextion_update_url}(blank.tft) instead of the correctnspanel_CJK_us_land.tft.Options indices:
- 0: "Use nextion_update_url"
- 1-7: Model-specific TFT files
The condition should be
idx <= 7to cover all 7 model options.🐛 Proposed fix
- } else if (idx >= 1 && idx <= 6) { + } else if (idx >= 1 && idx <= 7) { static const char* const FILENAME_PARTS[] = { "blank", "eu", "us", "us_land", "CJK_eu", "CJK_us", "CJK_us_land" };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@esphome/nspanel_esphome_addon_upload_tft.yaml` around lines 215 - 222, Off-by-one in the index boundary: change the condition that checks idx from "idx >= 1 && idx <= 6" to "idx >= 1 && idx <= 7" so the final option ("CJK_us_land") is handled; update the branch that builds resolved_url using FILENAME_PARTS[idx-1] (which already contains seven entries) so idx==7 maps correctly to "CJK_us_land", ensuring resolved_url uses nextion_update_base_url + version + the correct nspanel_CJK_us_land.tft.
🧹 Nitpick comments (2)
hmi/dev/nspanel_blank/blank.txt (2)
70-75: Version string format.The version
2026020appears to be a date-based version (YYYYMMDD) but seems incomplete or oddly formatted. If this represents February 2026, it might be intended as20260220(8 chars) but is truncated to fitMax. Text Size: 7.Verify if this is the intended format, or if the max text size should be increased to accommodate full date-based versions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hmi/dev/nspanel_blank/blank.txt` around lines 70 - 75, The version variable named "version" currently has Text set to "2026020" and Max. Text Size set to 7 which truncates an 8-digit YYYYMMDD value; either update the Text to the intended full date (e.g., "20260220") or increase the variable's Max. Text Size to 8 (or appropriate length) so the full date-based version fits; modify the "Text" and/or "Max. Text Size" attributes for the variable "version" accordingly and ensure any consumers expecting 8-digit format are updated.
298-300: QR code URL should reference local documentation or document the cross-repository link.The QR code links to
Blackymas/NSPanel_HA_Blueprintdocumentation. While this may be intentional given NSPanel-Easy's positioning as a migration path from that project, relying on external documentation creates fragility if that repository's structure changes or documentation moves.Add equivalent documentation to this repository's docs directory, or at minimum include a comment in blank.txt explaining why the Blackymas reference is intentional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hmi/dev/nspanel_blank/blank.txt` around lines 298 - 300, Update the QR code target in hmi/dev/nspanel_blank/blank.txt (the "Text" field that currently points to the Blackymas URL) by either adding equivalent documentation under this repository's docs and changing the URL to that local docs path, or if the external link is intentionally authoritative, add a clear inline comment in blank.txt next to the "Text" entry explaining why the cross-repo reference is deliberate and stable (include the original Blackymas URL and a brief justification), so future maintainers know this is intentional and not an oversight.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@esphome/nspanel_esphome_addon_upload_tft.yaml`:
- Around line 215-222: Off-by-one in the index boundary: change the condition
that checks idx from "idx >= 1 && idx <= 6" to "idx >= 1 && idx <= 7" so the
final option ("CJK_us_land") is handled; update the branch that builds
resolved_url using FILENAME_PARTS[idx-1] (which already contains seven entries)
so idx==7 maps correctly to "CJK_us_land", ensuring resolved_url uses
nextion_update_base_url + version + the correct nspanel_CJK_us_land.tft.
---
Nitpick comments:
In `@hmi/dev/nspanel_blank/blank.txt`:
- Around line 70-75: The version variable named "version" currently has Text set
to "2026020" and Max. Text Size set to 7 which truncates an 8-digit YYYYMMDD
value; either update the Text to the intended full date (e.g., "20260220") or
increase the variable's Max. Text Size to 8 (or appropriate length) so the full
date-based version fits; modify the "Text" and/or "Max. Text Size" attributes
for the variable "version" accordingly and ensure any consumers expecting
8-digit format are updated.
- Around line 298-300: Update the QR code target in
hmi/dev/nspanel_blank/blank.txt (the "Text" field that currently points to the
Blackymas URL) by either adding equivalent documentation under this repository's
docs and changing the URL to that local docs path, or if the external link is
intentionally authoritative, add a clear inline comment in blank.txt next to the
"Text" entry explaining why the cross-repo reference is deliberate and stable
(include the original Blackymas URL and a brief justification), so future
maintainers know this is intentional and not an oversight.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
esphome/nspanel_esphome_addon_upload_tft.yamlhmi/dev/nspanel_CJK_eu_code/boot.txthmi/dev/nspanel_CJK_us_land_code/boot.txthmi/dev/nspanel_blank/Program.s.txthmi/dev/nspanel_blank/blank.txthmi/dev/nspanel_eu_code/boot.txthmi/dev/nspanel_us_land_code/boot.txthmi/dev/ui/fonts/3.zihmi/nspanel_CJK_eu.HMIhmi/nspanel_CJK_eu.tfthmi/nspanel_CJK_us_land.HMIhmi/nspanel_CJK_us_land.tfthmi/nspanel_blank.HMIhmi/nspanel_blank.tfthmi/nspanel_eu.HMIhmi/nspanel_eu.tfthmi/nspanel_us_land.HMIhmi/nspanel_us_land.tft
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/nspanel_blank.md (1)
10-12: Tighten wording for a more direct tone.Small wording cleanup can make instructions read more crisply (e.g., replace “Because of that” and “feel free”).
Also applies to: 40-41
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/nspanel_blank.md` around lines 10 - 12, Tighten the prose by replacing verbose phrases like "Because of that" with a direct phrase (e.g., "Therefore" or "Use") and removing informal filler such as "feel free"; specifically update the sentences referencing `nspanel_blank.tft` to read more directly (for example: "Use `nspanel_blank.tft` first — it is much smaller than a regular `.tft` file and makes the initial replacement easier.") and apply the same concise rewording to the similar text at the other occurrence (lines noted as 40-41) so both spots use a crisp, direct tone.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/nspanel_blank.md`:
- Around line 22-24: The current instructions are ambiguous about model
selection and can cause regional TFT mismatches; update the text around "Upload
TFT display - Model" / "Upload TFT display" to explicitly instruct users to
first select "NSPanel Blank" for the initial transfer, then, before performing
the final TFT upload, change the selection to the correct regional model ("EU",
"US", or "US_Land") and then click "Upload TFT display" again so the correct
regional firmware is applied.
---
Nitpick comments:
In `@docs/nspanel_blank.md`:
- Around line 10-12: Tighten the prose by replacing verbose phrases like
"Because of that" with a direct phrase (e.g., "Therefore" or "Use") and removing
informal filler such as "feel free"; specifically update the sentences
referencing `nspanel_blank.tft` to read more directly (for example: "Use
`nspanel_blank.tft` first — it is much smaller than a regular `.tft` file and
makes the initial replacement easier.") and apply the same concise rewording to
the similar text at the other occurrence (lines noted as 40-41) so both spots
use a crisp, direct tone.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/pics/page_blank.pngis excluded by!**/*.png
📒 Files selected for processing (4)
docs/nspanel_blank.mdesphome/nspanel_esphome_addon_upload_tft.yamlhmi/nspanel_blank.HMIhmi/nspanel_blank.tft
🚧 Files skipped from review as they are similar to previous changes (1)
- esphome/nspanel_esphome_addon_upload_tft.yaml
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b663198e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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 `@docs/nspanel_blank.md`:
- Around line 14-16: The final TFT options are inconsistent: the paragraph
referencing nspanel_eu.tft, nspanel_us.tft and nspanel_us_land.tft should match
the later mention of CJK variants; update the text so both sections list the
same allowed final TFT files (e.g., include nspanel_cjk.tft /
nspanel_cjk_land.tft or explicitly state which variants are valid), or add a
short note mentioning CJK variants are also valid; edit the two occurrences that
reference final TFT filenames (the line containing nspanel_eu.tft,
nspanel_us.tft, nspanel_us_land.tft and the later CJK mention) so they present a
consistent set of final model options.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/pics/ha_device_configuration_tft_upload_model_selector.pngis excluded by!**/*.png
📒 Files selected for processing (1)
docs/nspanel_blank.md
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/nspanel_blank.md (2)
46-46: Optional style enhancement from static analysis.The phrase "feel free to create" is perfectly clear but somewhat common. If you want a slightly more varied tone, you could consider alternatives like "please create" or "you're welcome to create."
This is purely a style suggestion with no functional impact—the current phrasing works perfectly well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/nspanel_blank.md` at line 46, Replace the phrase "and feel free to create a new one asking for more personalized assistance." in docs/nspanel_blank.md with a slightly varied alternative to improve tone; for example, change it to "and please create a new one asking for more personalized assistance." or "and you're welcome to create a new one asking for more personalized assistance." Pick the preferred wording, update that sentence in the file, and ensure punctuation and capitalization remain consistent with surrounding text.
11-11: Consider clarifying "selecting" vs "default" phrasing.The phrase "we highly recommend selecting NSPanel Blank" followed by "(this is the default for new installations)" creates slight ambiguity—if it's the default, does the user still need to actively select it, or is it pre-selected?
Consider rephrasing to make it clearer, for example: "We highly recommend using NSPanel Blank as your first upload, which is pre-selected as the default for new installations" or "We highly recommend NSPanel Blank as your first upload (it will be pre-selected by default for new installations)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/nspanel_blank.md` at line 11, Edit the sentence that mentions "NSPanel Blank" so it removes ambiguity about whether users must take action; replace "We highly recommend selecting **NSPanel Blank** as your first upload (this is the default for new installations)" with a clearer phrasing such as "We highly recommend using **NSPanel Blank** as your first upload, which is pre-selected by default for new installations" or "We highly recommend **NSPanel Blank** as your first upload (it will be pre-selected for new installations)"; update the line containing the literal "NSPanel Blank" to one of these alternatives.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/nspanel_blank.md`:
- Line 46: Replace the phrase "and feel free to create a new one asking for more
personalized assistance." in docs/nspanel_blank.md with a slightly varied
alternative to improve tone; for example, change it to "and please create a new
one asking for more personalized assistance." or "and you're welcome to create a
new one asking for more personalized assistance." Pick the preferred wording,
update that sentence in the file, and ensure punctuation and capitalization
remain consistent with surrounding text.
- Line 11: Edit the sentence that mentions "NSPanel Blank" so it removes
ambiguity about whether users must take action; replace "We highly recommend
selecting **NSPanel Blank** as your first upload (this is the default for new
installations)" with a clearer phrasing such as "We highly recommend using
**NSPanel Blank** as your first upload, which is pre-selected by default for new
installations" or "We highly recommend **NSPanel Blank** as your first upload
(it will be pre-selected for new installations)"; update the line containing the
literal "NSPanel Blank" to one of these alternatives.
Reorganize and clarify log collection guidelines for bug reporting.
Removed unnecessary line breaks in the bug report template for better readability.
Emphasize the importance of sharing relevant logs when reporting issues.
1 participant
Fixes #23
Summary by CodeRabbit
New Features
Documentation
Style