Skip to content

Conversation

@DedeHai
Copy link
Collaborator

@DedeHai DedeHai commented Nov 15, 2025

command is used in pxmagic tool, maybe others.
since people may have the tool manually installed already, updating to 0.16 will break it so adding legacy support for this command is needed.

Summary by CodeRabbit

  • Bug Fixes

    • Added support for legacy file list query patterns to improve backward compatibility with existing API requests.
  • Improvements

    • Enhanced control flow for the editor handler to properly accommodate legacy and current query methods.

@DedeHai DedeHai requested a review from willmmiles November 15, 2025 15:37
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 15, 2025

Walkthrough

Modified the web server request handler to support a legacy list trigger pattern by introducing a new s_list constant and a legacyList flag. The code now detects list requests through either the func parameter or a list argument, enabling the legacy query pattern ?list=/. Adjusted the /edit handler control flow to accommodate this pathway while maintaining existing file listing logic.

Changes

Cohort / File(s) Summary
Legacy list trigger support
wled00/wled_server.cpp
Added s_list constant and legacyList flag for detecting legacy list requests. Modified /edit handler control flow to check for both func == "list" and legacyList conditions. Adjusted file listing pathway to support legacy query pattern while preserving existing functionality.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verify the legacyList flag logic correctly identifies legacy list requests (?list=/ pattern)
  • Confirm the modified control flow in the /edit handler doesn't introduce unintended routing behavior
  • Check that file listing output remains consistent with the new pathway

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main changes: adding legacy support for 'edit?list=/' command and fixing indentation, both of which are confirmed in the code changes.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1948293 and 540270e.

📒 Files selected for processing (1)
  • wled00/wled_server.cpp (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.cpp

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use 2-space indentation for C++ source files (.cpp)

Files:

  • wled00/wled_server.cpp
🧠 Learnings (10)
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/html_*.h : DO NOT edit generated embedded web header files (wled00/html_*.h)

Applied to files:

  • wled00/wled_server.cpp
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, verify that file operations (especially file.open()) respect LittleFS filename limitations. Assume default WLED configuration with LittleFS default filename limit of 255 bytes. Do not assume extreme configuration values like WLED_MAX_SEGNAME_LEN = 512 which would not be standard configurations.

Applied to files:

  • wled00/wled_server.cpp
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.

Applied to files:

  • wled00/wled_server.cpp
📚 Learning: 2025-09-24T18:52:34.117Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4956
File: wled00/data/edit.htm:295-303
Timestamp: 2025-09-24T18:52:34.117Z
Learning: WLED file system stores all files in the root directory only (flat structure), so file paths don't have subdirectories. When working with WLED file editors, using just the filename (name) rather than full path is correct since there are no nested directories.

Applied to files:

  • wled00/wled_server.cpp
📚 Learning: 2025-09-24T18:52:06.843Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4956
File: wled00/data/edit.htm:376-379
Timestamp: 2025-09-24T18:52:06.843Z
Learning: In WLED firmware, all files reside in the root directory ("/"), so file tree refresh operations should always target the root path regardless of the parameter passed to refreshPath().

Applied to files:

  • wled00/wled_server.cpp
📚 Learning: 2025-09-25T03:51:29.606Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4956
File: wled00/data/edit.htm:213-238
Timestamp: 2025-09-25T03:51:29.606Z
Learning: For WLED file editor: Path handling can be simplified since the system operates in root directory context, complex path traversal protection is not needed.

Applied to files:

  • wled00/wled_server.cpp
📚 Learning: 2025-11-14T13:37:11.994Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, file operations (especially file.open()) should be checked to ensure they respect LittleFS filename limitations. The default LittleFS filename limit is 255 bytes (LFS_NAME_MAX). Reviews should assume default WLED configuration defines and not extreme edge-case values (e.g., WLED_MAX_SEGNAME_LEN = 512 would not be standard). File paths should be validated to stay within the 255-byte limit.

Applied to files:

  • wled00/wled_server.cpp
📚 Learning: 2025-07-31T18:21:49.868Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4793
File: wled00/file.cpp:481-513
Timestamp: 2025-07-31T18:21:49.868Z
Learning: In WLED, essential configuration files that require backup have short, controlled names (like "/cfg.json", "/presets.json") that are well within a 32-character buffer limit. The file naming is controlled by developers, making buffer overflow in backup filename construction highly unlikely.

Applied to files:

  • wled00/wled_server.cpp
📚 Learning: 2025-11-08T08:25:18.724Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4956
File: wled00/wled_server.cpp:237-262
Timestamp: 2025-11-08T08:25:18.724Z
Learning: In WLED's filesystem code (ESP8266/ESP32), when iterating directory entries with `openNextFile()`, explicit `close()` calls between iterations are not required. The File class assignment operator properly handles closing the previous file handle when reassigning (e.g., `rootfile = rootdir.openNextFile()`). Both explicit and implicit patterns exist in the codebase, but relying on assignment operator cleanup is valid.

Applied to files:

  • wled00/wled_server.cpp
📚 Learning: 2025-09-14T18:43:59.338Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4932
File: wled00/data/cpal/cpal.htm:14-14
Timestamp: 2025-09-14T18:43:59.338Z
Learning: In WLED, static web pages like cpal.htm are served directly at the root URL level (e.g., "/cpal.htm") via handleStaticContent() from embedded page data, regardless of their physical source folder location in the repository. Relative script references like "common.js" resolve correctly from the served URL path, not the source file path.

Applied to files:

  • wled00/wled_server.cpp
⏰ 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). (20)
  • GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
  • GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
  • GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
  • GitHub Check: wled_build / Build Environments (esp32c3dev_qio)
  • GitHub Check: wled_build / Build Environments (esp32_wrover)
  • GitHub Check: wled_build / Build Environments (esp32_eth)
  • GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
  • GitHub Check: wled_build / Build Environments (lolin_s2_mini)
  • GitHub Check: wled_build / Build Environments (esp32c3dev)
  • GitHub Check: wled_build / Build Environments (esp32dev_debug)
  • GitHub Check: wled_build / Build Environments (usermods)
  • GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
  • GitHub Check: wled_build / Build Environments (esp8266_2m)
  • GitHub Check: wled_build / Build Environments (esp32dev)
  • GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
  • GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
  • GitHub Check: wled_build / Build Environments (esp8266_2m_160)
  • GitHub Check: wled_build / Build Environments (nodemcuv2_160)
  • GitHub Check: wled_build / Build Environments (esp01_1m_full)
  • GitHub Check: wled_build / Build Environments (nodemcuv2)
🔇 Additional comments (5)
wled00/wled_server.cpp (5)

30-30: LGTM: Well-structured constant addition.

The new s_list constant follows the established pattern in the file and properly uses PROGMEM for flash storage, which is a best practice for embedded systems.


230-233: LGTM: Legacy support implementation is sound.

The legacy list detection correctly uses hasArg() to identify the legacy query pattern. Note that this checks only for the presence of the "list" parameter, not its value, meaning ?list=/, ?list=, or ?list=anything are all treated identically. This appears intentional since the parameter value isn't utilized in the listing logic.


235-239: LGTM: Correct control flow for legacy support.

The modified condition properly ensures that when the legacy ?list=/ pattern is detected, the editor page isn't served and execution continues to the list handler below. This preserves backward compatibility without affecting normal operation.


241-266: LGTM: Dual-mode list handling correctly implemented.

The condition now properly handles both the modern func=list pattern and the legacy ?list=/ pattern, ensuring backward compatibility with existing tools while maintaining the new implementation.


251-260: LGTM: Indentation corrected per coding guidelines.

The file listing logic has been properly reformatted to use 2-space indentation, aligning with the coding guidelines for C++ source files. The functionality remains unchanged—files are correctly enumerated with wsec.json filtered out.

As per coding guidelines.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant