src/activities/home/MyLibraryActivity.cpp (1)

227-241: Pass parameters by const& and handle missing extensions.

1. Copy overhead: Both methods take std::string by value, causing unnecessary allocations on each call. Use const std::string& instead.

2. Edge case: For files without an extension (e.g., README, Makefile), rfind('.') returns npos. In getFileType, substr(npos + 1) wraps to substr(0), returning the entire filename as the type. In getFileName, it returns the full name (acceptable but inconsistent).

♻️ Proposed fix

-std::string MyLibraryActivity::getFileType(std::string filename) const {
+std::string MyLibraryActivity::getFileType(const std::string& filename) const {
   if (filename.back() == '/') {
     return "Folder";
   }
   const auto pos = filename.rfind('.');
+  if (pos == std::string::npos) {
+    return "File";
+  }
   return filename.substr(pos + 1);
 }

-std::string MyLibraryActivity::getFileName(std::string filename) const {
+std::string MyLibraryActivity::getFileName(const std::string& filename) const {
   if (filename.back() == '/') {
     return filename.substr(0, filename.length() - 1);
   }
   const auto pos = filename.rfind('.');
+  if (pos == std::string::npos) {
+    return filename;
+  }
   return filename.substr(0, pos);
 }

src/activities/home/MyLibraryActivity.h (1)

38-39: Update declarations to use const std::string&.

Consistent with the implementation fix suggested in MyLibraryActivity.cpp, update these declarations to avoid pass-by-value copies.

♻️ Proposed fix

-  std::string getFileType(std::string filename) const;
-  std::string getFileName(std::string filename) const;
+  std::string getFileType(const std::string& filename) const;
+  std::string getFileName(const std::string& filename) const;

src/activities/network/NetworkModeSelectionActivity.cpp (1)

10-21: Consider deriving item count from the arrays to prevent drift.

MENU_ITEM_COUNT can diverge from the actual array sizes as items evolve. Using std::array and size() avoids this risk.

♻️ Suggested refactor

+#include <array>
 namespace {
-constexpr int MENU_ITEM_COUNT = 3;
-const char* MENU_ITEMS[MENU_ITEM_COUNT] = {"Join a Network", "Connect to Calibre", "Create Hotspot"};
-const char* MENU_DESCRIPTIONS[MENU_ITEM_COUNT] = {
+constexpr std::array<const char*, 3> MENU_ITEMS = {
+    "Join a Network", "Connect to Calibre", "Create Hotspot"};
+constexpr std::array<const char*, 3> MENU_DESCRIPTIONS = {
     "Connect to an existing WiFi network",
     "Use Calibre wireless device transfers",
     "Create a WiFi network others can join",
 };
-const char* MENU_ICONS[MENU_ITEM_COUNT] = {
+constexpr std::array<const char*, 3> MENU_ICONS = {
     "wifi",
-    "OPDS Browser",
+    "OPDS Browser",
     "hotspot",
 };
+constexpr int MENU_ITEM_COUNT = static_cast<int>(MENU_ITEMS.size());
 }  // namespace

src/activities/network/CrossPointWebServerActivity.cpp (1)

431-494: Avoid double header/subheader rendering.
render() already draws the header/subheader for SERVER_RUNNING; doing it again here adds extra draw work with no visual benefit. If renderServerRunning() is only called from render(), you can drop these calls.

♻️ Suggested trim

-  GUI.drawHeader(renderer, Rect{0, metrics.topPadding, pageWidth, metrics.headerHeight},
-                 isApMode ? "Hotspot Mode" : "Network Transfer", nullptr);
-  GUI.drawSubHeader(renderer, Rect{0, metrics.topPadding + metrics.headerHeight, pageWidth, metrics.tabBarHeight},
-                    connectedSSID.c_str());

src/components/icons/library.h (1)

1-12: Avoid per‑TU duplication of icon data.

static in a header produces one copy per translation unit. For embedded builds this can add up across many icons. If C++17 is available, prefer inline constexpr to keep a single ODR entity, or move the definition to a .cpp with an extern declaration.

💡 Example (C++17 inline variable)

- static const uint8_t LibraryIcon[] = {
+ inline constexpr uint8_t LibraryIcon[] = {

src/components/themes/lyra/LyraTheme.cpp (2)

47-63: Normalize icon names once to avoid case drift and brittle matches.

A single lowercase normalization makes icon lookups resilient to label casing changes and keeps comparisons consistent.

🛠️ Proposed refactor

@@
-#include <algorithm>
+#include <algorithm>
+#include <cctype>
@@
 const uint8_t* iconForName(const std::string& iconName, int size) {
+  std::string key = iconName;
+  std::transform(key.begin(), key.end(), key.begin(),
+                 [](unsigned char c) { return static_cast<char>(std::tolower(c)); });
   if (size == 24) {
-    if (iconName == "Folder") return Folder24Icon;
-    if (iconName == "epub" || iconName == "xtc" || iconName == "xtch") return Book24Icon;
-    if (iconName == "txt") return Text24Icon;
-    if (iconName == "bmp") return Image24Icon;
+    if (key == "folder") return Folder24Icon;
+    if (key == "epub" || key == "xtc" || key == "xtch") return Book24Icon;
+    if (key == "txt") return Text24Icon;
+    if (key == "bmp") return Image24Icon;
   } else if (size == 32) {
-    if (iconName == "epub" || iconName == "xtc" || iconName == "xtch") return BookIcon;
-    if (iconName == "Browse Files") return FolderIcon;
-    if (iconName == "Recents") return RecentIcon;
-    if (iconName == "Settings") return CogIcon;
-    if (iconName == "File Transfer") return TransferIcon;
-    if (iconName == "OPDS Browser") return LibraryIcon;
-    if (iconName == "wifi") return WifiIcon;
-    if (iconName == "hotspot") return HotspotIcon;
+    if (key == "epub" || key == "xtc" || key == "xtch") return BookIcon;
+    if (key == "browse files") return FolderIcon;
+    if (key == "recents") return RecentIcon;
+    if (key == "settings") return CogIcon;
+    if (key == "file transfer") return TransferIcon;
+    if (key == "opds browser") return LibraryIcon;
+    if (key == "wifi") return WifiIcon;
+    if (key == "hotspot") return HotspotIcon;
   }
   return nullptr;
 }

---

223-279: Consider caching icon/value text for large lists to reduce per-frame allocations.

Each redraw allocates multiple std::strings and performs icon lookups per row, which can contribute to the reported file-browser slowdown. Caching resolved icon pointers or switching callbacks to return std::string_view/const char* (or memoized strings) would reduce heap churn.

src/activities/settings/KOReaderAuthActivity.cpp (1)

126-148: Consider using metrics for vertical spacing between text lines.

The hardcoded + 10 spacing between lines (lines 140, 143) could be replaced with metrics.verticalSpacing for consistency with the metrics-driven approach used elsewhere in the render function.

♻️ Suggested improvement

+  auto metrics = UITheme::getInstance().getMetrics();
   const auto pageWidth = renderer.getScreenWidth();
   const auto pageHeight = renderer.getScreenHeight();
   ...
   if (state == SUCCESS) {
     renderer.drawCenteredText(UI_10_FONT_ID, top, "Success!", true, EpdFontFamily::BOLD);
-    renderer.drawCenteredText(UI_10_FONT_ID, top + height + 10, "KOReader sync is ready to use");
+    renderer.drawCenteredText(UI_10_FONT_ID, top + height + metrics.verticalSpacing, "KOReader sync is ready to use");
   } else if (state == FAILED) {
     renderer.drawCenteredText(UI_10_FONT_ID, top, "Error", true, EpdFontFamily::BOLD);
-    renderer.drawCenteredText(UI_10_FONT_ID, top + height + 10, errorMessage.c_str());
+    renderer.drawCenteredText(UI_10_FONT_ID, top + height + metrics.verticalSpacing, errorMessage.c_str());
   }

src/components/themes/lyra/Lyra3CoversTheme.cpp (1)

17-21: Unused mutable global coverWidth in anonymous namespace.

The coverWidth variable at line 20 is declared but never used in this file. The local variable coverWidth at line 54 shadows it. This appears to be dead code copied from LyraTheme.cpp where it serves a caching purpose.

♻️ Proposed fix

 namespace {
 constexpr int hPaddingInSelection = 8;
 constexpr int cornerRadius = 6;
-int coverWidth = 0;
 }  // namespace

src/activities/settings/OtaUpdateActivity.cpp (1)

121-137: Early return after clearScreen() wastes work.

When the progress throttling condition triggers (line 134), the function returns after clearScreen() and drawHeader() have already executed. While this doesn't cause visual issues (since displayBuffer() isn't called), these operations are performed unnecessarily.

♻️ Move throttling check before drawing operations

 void OtaUpdateActivity::render() {
   if (subActivity) {
     // Subactivity handles its own rendering
     return;
   }

+  // Throttle progress updates early to avoid unnecessary draw operations
+  if (state == UPDATE_IN_PROGRESS) {
+    float progress = static_cast<float>(updater.getProcessedSize()) / static_cast<float>(updater.getTotalSize());
+    if (static_cast<int>(progress * 50) == lastUpdaterPercentage / 2) {
+      return;
+    }
+    lastUpdaterPercentage = static_cast<int>(progress * 100);
+  }
+
   auto metrics = UITheme::getInstance().getMetrics();
   const auto pageWidth = renderer.getScreenWidth();
   const auto pageHeight = renderer.getScreenHeight();

   renderer.clearScreen();

   GUI.drawHeader(renderer, Rect{0, metrics.topPadding, pageWidth, metrics.headerHeight}, "Update");
   const auto height = renderer.getLineHeight(UI_10_FONT_ID);
   const auto top = (pageHeight - height) / 2;

-  float updaterProgress = 0;
-  if (state == UPDATE_IN_PROGRESS) {
-    Serial.printf("[%lu] [OTA] Update progress: %d / %d\n", millis(), updater.getProcessedSize(),
-                  updater.getTotalSize());
-    updaterProgress = static_cast<float>(updater.getProcessedSize()) / static_cast<float>(updater.getTotalSize());
-    // Only update every 2% at the most
-    if (static_cast<int>(updaterProgress * 50) == lastUpdaterPercentage / 2) {
-      return;
-    }
-    lastUpdaterPercentage = static_cast<int>(updaterProgress * 100);
-  }
+  float updaterProgress = (state == UPDATE_IN_PROGRESS)
+      ? static_cast<float>(updater.getProcessedSize()) / static_cast<float>(updater.getTotalSize())
+      : 0;

lib/GfxRenderer/GfxRenderer.cpp (1)

323-325: Clarify the heuristic assumption in the comment.

The comment says "Assume if we're not rounding all corners then we are only rounding one side," but the condition triggers when any corner is not rounded. This assumption holds for common cases (e.g., rounding only top or only left side) but could produce unexpected results if:

• Diagonal corners are rounded (top-left + bottom-right)
• Three corners are rounded

If these scenarios are intentionally unsupported, consider documenting that constraint. Otherwise, this is a reasonable pragmatic heuristic.

📝 Suggested comment clarification

-  // Assume if we're not rounding all corners then we are only rounding one side
+  // When partially rounding corners, assume we're rounding one side (top, bottom, left, or right).
+  // Diagonal or three-corner rounding may produce suboptimal radius clamping.
   const int roundedSides = (!roundTopLeft || !roundTopRight || !roundBottomLeft || !roundBottomRight) ? 1 : 2;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/GfxRenderer/GfxRenderer.cpp` around lines 323 - 325, The current comment
misstates the condition used to compute roundedSides: the code sets roundedSides
based on (!roundTopLeft || !roundTopRight || !roundBottomLeft ||
!roundBottomRight) which triggers when any corner is not rounded, so update the
comment to clearly describe this heuristic and its limitations (it assumes
either all corners or effectively one side is rounded and may produce odd
results for diagonal or three-corner cases) and either (A) document that
diagonal/three-corner cases are unsupported, or (B) replace the heuristic by
computing an explicit roundedCount (e.g., count true corner flags) and use that
to derive roundedSides/maxRadius to handle mixed cases; reference the
roundedSides, cornerRadius and maxRadius symbols when making the change.

src/activities/settings/OtaUpdateActivity.cpp (1)

87-105: Avoid doing full render work when throttling progress updates.

When the 2% throttle triggers (Line 102), the function returns after clearScreen() and header draw, doing work that never reaches displayBuffer(). Consider moving the throttle check earlier (before clearing/drawing) to save CPU/battery.

♻️ Possible refactor

-  renderer.clearScreen();
-
-  GUI.drawHeader(renderer, Rect{0, metrics.topPadding, pageWidth, metrics.headerHeight}, tr(STR_UPDATE));
-  const auto height = renderer.getLineHeight(UI_10_FONT_ID);
-  const auto top = (pageHeight - height) / 2;
-
   float updaterProgress = 0;
   if (state == UPDATE_IN_PROGRESS) {
     LOG_DBG("OTA", "Update progress: %d / %d", updater.getProcessedSize(), updater.getTotalSize());
     updaterProgress = static_cast<float>(updater.getProcessedSize()) / static_cast<float>(updater.getTotalSize());
     // Only update every 2% at the most
     if (static_cast<int>(updaterProgress * 50) == lastUpdaterPercentage / 2) {
       return;
     }
     lastUpdaterPercentage = static_cast<int>(updaterProgress * 100);
   }
+
+  renderer.clearScreen();
+  GUI.drawHeader(renderer, Rect{0, metrics.topPadding, pageWidth, metrics.headerHeight}, tr(STR_UPDATE));
+  const auto height = renderer.getLineHeight(UI_10_FONT_ID);
+  const auto top = (pageHeight - height) / 2;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/activities/settings/OtaUpdateActivity.cpp` around lines 87 - 105, Compute
updaterProgress and perform the 2% throttle check before doing any heavy
rendering (i.e., before calling renderer.clearScreen() and GUI.drawHeader()).
Specifically, call updater.getProcessedSize() and updater.getTotalSize(),
compute updaterProgress, compare the throttled percentage against
lastUpdaterPercentage, and return early if unchanged; only if the check passes
proceed to clear the screen, draw the header and continue updating
lastUpdaterPercentage. Update references to updaterProgress,
lastUpdaterPercentage, renderer.clearScreen(), GUI.drawHeader(),
updater.getProcessedSize(), and updater.getTotalSize() accordingly so no
unnecessary work is done when throttled.

src/components/UITheme.h (2)

26-26: Consider passing filename by const reference to avoid unnecessary copy.

The PR mentions the file browser is noticeably slower after adding icons. Passing std::string by value creates an unnecessary copy on each call. Since getFileIcon only reads the string, passing by const std::string& would be more efficient.

♻️ Proposed fix

-  static UIIcon getFileIcon(std::string filename);
+  static UIIcon getFileIcon(const std::string& filename);

The implementation in UITheme.cpp should also be updated to match.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/components/UITheme.h` at line 26, Change the getFileIcon API to accept
the filename by const reference to avoid copies: update the declaration static
UIIcon getFileIcon(const std::string& filename) in UITheme.h and update the
corresponding definition in UITheme.cpp to match (function name getFileIcon).
Also update any call sites that relied on the by-value signature (pass
std::string or const std::string& as before) to ensure they compile. Rebuild and
run the file browser performance regression test to confirm improvement.

---

9-10: Unused forward declaration.

MappedInputManager is forward-declared here but doesn't appear to be used anywhere in this header file. If it's only needed in the implementation file (UITheme.cpp), consider moving the forward declaration or include there instead.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/components/UITheme.h` around lines 9 - 10, Remove the unused forward
declaration of MappedInputManager from UITheme.h; if the type is only referenced
in UITheme.cpp, move the forward declaration or the `#include` for
MappedInputManager into UITheme.cpp (or include the header where it is actually
used) so UITheme.h no longer contains an unused forward declaration.

src/activities/settings/ButtonRemapActivity.cpp (1)

136-141: Help text positioning is tightly coupled to list row count.

The calculations 4 * metrics.listRowHeight and 5 * metrics.verticalSpacing assume exactly 4 list items. While this matches kRoleCount, if the role count changes, these magic multipliers would need manual updates. Consider deriving the position from the list's computed bounds or using kRoleCount in the calculation.

♻️ Proposed improvement

   // Provide side button actions at the bottom of the screen (split across two lines).
+  const int helpTextTop = topOffset + kRoleCount * metrics.listRowHeight + kRoleCount * metrics.verticalSpacing;
   GUI.drawHelpText(renderer,
-                   Rect{0, topOffset + 4 * metrics.listRowHeight + 4 * metrics.verticalSpacing, pageWidth, 20},
+                   Rect{0, helpTextTop, pageWidth, 20},
                    tr(STR_REMAP_RESET_HINT));
   GUI.drawHelpText(renderer,
-                   Rect{0, topOffset + 4 * metrics.listRowHeight + 5 * metrics.verticalSpacing + 20, pageWidth, 20},
+                   Rect{0, helpTextTop + metrics.verticalSpacing + 20, pageWidth, 20},
                    tr(STR_REMAP_CANCEL_HINT));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/activities/settings/ButtonRemapActivity.cpp` around lines 136 - 141, The
help text Y offsets are hard-coded with “4 * metrics.listRowHeight” and “5 *
metrics.verticalSpacing” which assumes 4 list items; update the calculation to
derive the vertical position from the actual list bounds or the canonical role
count instead of magic numbers: use the list's computed bounds (e.g. the
variable that holds the list Rect or call that computes its height) or replace
the literal multipliers with kRoleCount * metrics.listRowHeight and kRoleCount *
metrics.verticalSpacing so GUI.drawHelpText calls compute topOffset +
listBounds.height (or topOffset + kRoleCount * metrics.listRowHeight) +
additional spacing for the two help lines, referencing GUI.drawHelpText,
metrics.listRowHeight, metrics.verticalSpacing, and kRoleCount (or the list
bounds variable) to locate and update the code.

src/components/themes/lyra/Lyra3CoversTheme.h (1)

1-5: Minor: Remove leading blank lines.

The file starts with two blank lines before #pragma once. This is a minor style inconsistency.

♻️ Proposed fix

-
-
 `#pragma` once

 `#include` "components/themes/lyra/LyraTheme.h"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/components/themes/lyra/Lyra3CoversTheme.h` around lines 1 - 5, The file
Lyra3CoversTheme.h contains extra leading blank lines before the `#pragma` once
directive; open the header (Lyra3CoversTheme.h) and remove any blank lines above
the `#pragma` once so the file starts immediately with `#pragma` once, keeping the
existing `#include` "components/themes/lyra/LyraTheme.h" unchanged.

src/activities/home/MyLibraryActivity.cpp (1)

191-197: Consider passing filename by const reference to avoid unnecessary copy.

The function takes std::string by value, creating a copy on each call. Since the string is only read, passing by const std::string& would be more efficient, especially when called repeatedly in list rendering.

♻️ Proposed fix

-std::string getFileName(std::string filename) {
+std::string getFileName(const std::string& filename) {
   if (filename.back() == '/') {
     return filename.substr(0, filename.length() - 1);
   }
   const auto pos = filename.rfind('.');
   return filename.substr(0, pos);
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/activities/home/MyLibraryActivity.cpp` around lines 191 - 197, The
getFileName function currently takes filename by value, causing an unnecessary
copy on each call; change its signature to accept const std::string& filename
and update any internal uses accordingly (e.g., in getFileName and any callers
if needed) so the function reads from filename without copying; keep the same
logic for trimming a trailing '/' and finding the last '.' (ensure you still
handle std::string::npos correctly).

src/activities/settings/KOReaderAuthActivity.cpp (1)

104-109: Consider using metrics for vertical spacing instead of magic number.

The hardcoded + 10 spacing between lines could use metrics.verticalSpacing or a fraction thereof for consistency with the theme system used elsewhere in the file.

♻️ Proposed fix

   } else if (state == SUCCESS) {
     renderer.drawCenteredText(UI_10_FONT_ID, top, tr(STR_AUTH_SUCCESS), true, EpdFontFamily::BOLD);
-    renderer.drawCenteredText(UI_10_FONT_ID, top + height + 10, tr(STR_SYNC_READY));
+    renderer.drawCenteredText(UI_10_FONT_ID, top + height + metrics.verticalSpacing, tr(STR_SYNC_READY));
   } else if (state == FAILED) {
     renderer.drawCenteredText(UI_10_FONT_ID, top, tr(STR_AUTH_FAILED), true, EpdFontFamily::BOLD);
-    renderer.drawCenteredText(UI_10_FONT_ID, top + height + 10, errorMessage.c_str());
+    renderer.drawCenteredText(UI_10_FONT_ID, top + height + metrics.verticalSpacing, errorMessage.c_str());
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/activities/settings/KOReaderAuthActivity.cpp` around lines 104 - 109, The
code uses a magic "+ 10" vertical offset when calling renderer.drawCenteredText
for the second line in both SUCCESS and FAILED branches; replace that hardcoded
10 with the theme metric (e.g., metrics.verticalSpacing or the appropriate Theme
metrics variable used elsewhere in this file) so both calls use top + height +
metrics.verticalSpacing (or a fraction like metrics.verticalSpacing / 2 if
desired) to keep spacing consistent; update both renderer.drawCenteredText calls
that currently use "+ 10" to reference the metric instead.

src/components/themes/lyra/Lyra3CoversTheme.cpp (1)

17-21: File-scope mutable coverWidth is shadowed and unused.

The file-scope coverWidth variable at line 20 is shadowed by the local variable coverWidth declared at line 54. This makes the file-scope variable unused in this implementation while potentially causing confusion.

♻️ Proposed fix - remove unused file-scope variable

 namespace {
 constexpr int hPaddingInSelection = 8;
 constexpr int cornerRadius = 6;
-int coverWidth = 0;
 }  // namespace

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/components/themes/lyra/Lyra3CoversTheme.cpp` around lines 17 - 21, The
file-scope mutable variable coverWidth inside the anonymous namespace is
shadowed by a local coverWidth (declared near line 54) and is unused—remove the
file-scope coverWidth declaration to avoid confusion; keep the anonymous
namespace with the constexprs (hPaddingInSelection, cornerRadius) intact and
ensure all references use the local coverWidth in the function where it is
declared.

src/activities/util/KeyboardEntryActivity.cpp (1)

319-320: Consider using clearer symbols for vertical navigation hints.

The side button hints use ">" and "<" which typically indicate horizontal direction. For up/down navigation, consider using "^" / "v" or arrow unicode characters if supported by the font, to avoid confusion.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/activities/util/KeyboardEntryActivity.cpp` around lines 319 - 320,
Replace the horizontal indicators passed to GUI.drawSideButtonHints(renderer,
">", "<") with vertical navigation symbols to avoid confusion; update the call
to use "^" and "v" (or "↑" and "↓" if your font supports Unicode) so the side
button hints clearly indicate Up/Down navigation.

src/activities/network/CrossPointWebServerActivity.cpp (1)

366-386: Consider making drawQRCode static or moving to anonymous namespace.

The drawQRCode function is defined at file scope without static linkage. If another translation unit defines a function with the same name, this could cause ODR violations or linking errors.

🛠️ Proposed fix

-void drawQRCode(const GfxRenderer& renderer, const int x, const int y, const std::string& data) {
+namespace {
+void drawQRCode(const GfxRenderer& renderer, const int x, const int y, const std::string& data) {
   // Implementation...
 }
+}  // namespace

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/activities/network/CrossPointWebServerActivity.cpp` around lines 366 -
386, drawQRCode currently has external linkage at file scope which can cause
ODR/linkage conflicts; change its linkage to internal by either marking the
function static (add the static keyword to drawQRCode) or move its definition
into an anonymous namespace at top of file so it becomes translation-unit-local;
update any callers in this file to continue using drawQRCode and ensure symbols
like QRCode, qrcode_getBufferSize, qrcode_initText, and qrcode_getModule remain
visible in that scope.

src/components/themes/BaseTheme.cpp (1)

716-719: drawTextField ignores textWidth parameter for bracket positioning.

The textWidth parameter is accepted but the brackets are drawn at fixed positions (rect.x + 10 and rect.x + rect.width - 15) rather than being positioned relative to the text width. This differs from the LyraTheme implementation which uses textWidth to position a centered underline.

🤔 Consider aligning with LyraTheme approach

If the intent is to frame the text, consider using textWidth to position the brackets around the actual text content, or document the different behavior between themes.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/components/themes/BaseTheme.cpp` around lines 716 - 719,
BaseTheme::drawTextField currently ignores the textWidth parameter and uses
fixed positions for brackets; change it to compute the text's horizontal origin
from textWidth (e.g., center the text inside rect or use the same centering
logic as LyraTheme) and place the left bracket relative to that text origin and
the right bracket at textOrigin + textWidth plus a small gap; update
renderer.drawText calls (the ones drawing "[" and "]") to use these computed
positions so brackets frame the actual text width instead of fixed offsets.

src/components/themes/BaseTheme.h (1)

64-64: Consider using enum class for type safety.

The UIIcon enum uses a plain enum which exposes its enumerators to the enclosing scope, potentially causing naming conflicts. Using enum class would provide stronger type safety and scoping.

♻️ Suggested change

-enum UIIcon { Folder, Text, Image, Book, File, Recent, Settings, Transfer, Library, Wifi, Hotspot };
+enum class UIIcon { Folder, Text, Image, Book, File, Recent, Settings, Transfer, Library, Wifi, Hotspot };

This would require updating call sites to use UIIcon::Folder etc., but provides better namespace hygiene and prevents implicit conversions to int.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/components/themes/BaseTheme.h` at line 64, Change the plain enum UIIcon
to a scoped enum (enum class UIIcon) to prevent its enumerators from leaking
into the enclosing scope; update all call sites that currently use raw
enumerators (e.g., Folder, Text, Image, etc.) to qualify them as UIIcon::Folder,
UIIcon::Text, UIIcon::Image, and adjust any implicit-int usage or comparisons
(casts to/from int) to use explicit casts or overloads so code using UIIcon (in
BaseTheme.h and its consumers) compiles cleanly.

src/components/themes/lyra/LyraTheme.cpp (1)

42-42: Mutable global state in anonymous namespace is problematic.

coverWidth is a mutable variable in the anonymous namespace, which means it persists across calls and could cause issues if multiple theme instances exist or if the cover dimensions change. This should be either:

1. A member variable of LyraTheme, or
2. Computed/passed each time it's needed

♻️ Suggested approach

Consider making coverWidth a mutable member of LyraTheme or computing it locally within drawRecentBookCover. The current approach caches the width globally after the first bitmap load, which could lead to stale values if covers of different sizes are loaded.

 // In anonymous namespace, remove:
-int coverWidth = 0;

 // In LyraTheme class (header), add:
+mutable int coverWidth_ = 0;

 // In drawRecentBookCover, update references:
-  if (coverWidth == 0) {
-    coverWidth = LyraMetrics::values.homeCoverHeight * 0.6;
+  if (coverWidth_ == 0) {
+    coverWidth_ = LyraMetrics::values.homeCoverHeight * 0.6;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/components/themes/lyra/LyraTheme.cpp` at line 42, The global mutable
coverWidth in the anonymous namespace should be removed and its value maintained
per-theme or computed on demand: make coverWidth a member variable of the
LyraTheme class (e.g., add an int coverWidth_ field) or compute the width inside
drawRecentBookCover each time by querying the bitmap (replace references to
coverWidth with either this->coverWidth_ or a local variable computed from the
loaded Bitmap), and update any initialization logic that currently caches the
width globally (e.g., where coverWidth is set after bitmap load) to set the new
member or local variable instead; update all usages in
LyraTheme::drawRecentBookCover and related LyraTheme methods to reference the
new member/local name instead of the anonymous-namespace coverWidth.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Learnt from: whyte-j
Repo: crosspoint-reader/crosspoint-reader PR: 733
File: lib/I18n/I18nKeys.h:271-272
Timestamp: 2026-02-16T22:25:35.674Z
Learning: In the crosspoint-reader i18n system (lib/I18n/), missing translation keys in non-English YAML files are automatically filled with English strings at build time by the gen_i18n.py script. All generated string arrays (STRINGS_EN, STRINGS_ES, etc.) have identical lengths, so runtime array indexing is safe without per-string fallback logic. The system prints "INFO: '{key}' missing in {lang_code}, using English fallback" during code generation when this occurs.

Learnt from: CaptainFrito
Repo: crosspoint-reader/crosspoint-reader PR: 725
File: src/activities/network/CalibreConnectActivity.cpp:218-264
Timestamp: 2026-02-12T06:57:35.955Z
Learning: In src/activities/network/CalibreConnectActivity.cpp, button hints (« Exit) are intentionally omitted from the SERVER_STARTING and ERROR states—only the SERVER_RUNNING state displays navigation hints. This is a deliberate design decision by the author.

Learnt from: Levrk
Repo: crosspoint-reader/crosspoint-reader PR: 909
File: src/activities/reader/EpubReaderActivity.cpp:461-461
Timestamp: 2026-02-16T20:43:19.339Z
Learning: In src/activities/reader/EpubReaderActivity.cpp, when using ConfirmationActivity with enterNewActivity(), it's not necessary to call exitActivity() beforehand. The ConfirmationActivity operates as a modal dialog and cleanup is handled via the pendingSubactivityExit flag in the callback lambda. This differs from other activities like EpubReaderChapterSelectionActivity or KOReaderSyncActivity which do require exitActivity() before enterNewActivity().

src/components/themes/lyra/LyraTheme.cpp (2)

451-459: Consider extracting magic numbers for icon positioning.

The icon offset 24 appears both here and in Lyra3CoversTheme.cpp (line 75). Consider defining a constant like constexpr int coverIconOffset = 24; for consistency and maintainability.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/components/themes/lyra/LyraTheme.cpp` around lines 451 - 459, The
drawIcon call in the hasCover false branch uses the magic number 24 for X/Y
offsets (renderer.drawIcon(CoverIcon, ... + 24, ... + 24, 32, 32)); introduce a
named constant (e.g., constexpr int coverIconOffset = 24) in the Lyra theme
source or header shared with Lyra3CoversTheme, replace the hardcoded 24s with
coverIconOffset in LyraTheme.cpp (and update Lyra3CoversTheme.cpp line 75 to use
the same constant) so positioning is consistent and maintainable while leaving
the existing renderer.drawIcon and CoverIcon usage unchanged.

---

39-42: Remove unused mainMenuColumns constant or add a clarifying comment.

The constant mainMenuColumns = 2 at line 41 is never referenced anywhere in the codebase. Either remove it or add a comment explaining its intended purpose if it's planned for future use.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/components/themes/lyra/LyraTheme.cpp` around lines 39 - 42, The constant
mainMenuColumns in LyraTheme.cpp is unused; either remove the declaration
constexpr int mainMenuColumns = 2 from LyraTheme.cpp, or if it’s intended for
future layout logic, keep it but add a one-line comment above the declaration
explaining its intended purpose (e.g., "number of columns for main menu layout -
reserved for future use") so its presence is intentional and clear; update any
relevant unit tests or style checks if they expect no unused symbols.

src/components/themes/BaseTheme.h (1)

64-65: Consider using enum class for stronger type safety.

The plain enum UIIcon places its enumerators (e.g., File, Text, Image) in the enclosing namespace. These generic names risk collisions with other identifiers. Using enum class UIIcon would scope the values and require explicit qualification (e.g., UIIcon::File).

♻️ Proposed refactor

-enum UIIcon { Folder, Text, Image, Book, File, Recent, Settings, Transfer, Library, Wifi, Hotspot };
+enum class UIIcon { Folder, Text, Image, Book, File, Recent, Settings, Transfer, Library, Wifi, Hotspot };

Note: This would require updating all usages to use the UIIcon:: prefix (which already appears to be the convention in the codebase based on the LyraTheme.cpp usage).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/components/themes/BaseTheme.h` around lines 64 - 65, Change the plain
enum UIIcon to a scoped enum (enum class UIIcon) to prevent polluting the
enclosing namespace; update all call sites to use the scoped qualifiers (e.g.,
UIIcon::File, UIIcon::Text, UIIcon::Image, etc.), including usage in
LyraTheme.cpp and any switch/statements or variable declarations that currently
reference unqualified enumerators, and ensure any implicit conversions to/from
integers are made explicit where needed.

src/components/UITheme.h (1)

25-25: Use const reference to avoid unnecessary copies in hot path.

The signature takes std::string by value, which creates a copy on every call. Since this function is called in the rendering loop (both RecentBooksActivity::render() and MyLibraryActivity::render()), it's a hot path. The implementation only reads the filename (checking extensions and directory status), never modifies it, making const std::string& the appropriate choice.

Update both the declaration and definition:

Required changes

-  static UIIcon getFileIcon(std::string filename);
+  static UIIcon getFileIcon(const std::string& filename);

-UIIcon UITheme::getFileIcon(std::string filename) {
+UIIcon UITheme::getFileIcon(const std::string& filename) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/components/UITheme.h` at line 25, Change the hot-path function UIIcon
getFileIcon(std::string filename) to accept a const reference: UIIcon
getFileIcon(const std::string& filename); update the corresponding definition
implementation (the function with the same name) to match the new signature and
ensure all callers (e.g., RecentBooksActivity::render and
MyLibraryActivity::render) continue to compile; this avoids unnecessary copies
on each render while preserving const-correctness since the function only reads
filename.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

src/components/themes/lyra/LyraTheme.cpp (1)

503-533: Consider truncating menu labels after adding icons.
Icons reduce horizontal space, so long labels can clip or crowd. Truncating to the remaining width keeps layout stable.

♻️ Suggested adjustment

-    const char* label = labelStr.c_str();
     int textX = tileRect.x + 16;
     const int lineHeight = renderer.getLineHeight(UI_12_FONT_ID);
     const int textY = tileRect.y + (LyraMetrics::values.menuRowHeight - lineHeight) / 2;

     if (rowIcon != nullptr) {
       UIIcon icon = rowIcon(i);
       const uint8_t* iconBitmap = iconForName(icon, mainMenuIconSize);
       if (iconBitmap != nullptr) {
         renderer.drawIcon(iconBitmap, textX, textY + 3, mainMenuIconSize, mainMenuIconSize);
         textX += mainMenuIconSize + hPaddingInSelection + 2;
       }
     }

-    renderer.drawText(UI_12_FONT_ID, textX, textY, label, true);
+    const int maxLabelWidth = tileRect.width - (textX - tileRect.x) - hPaddingInSelection;
+    auto truncatedLabel =
+        renderer.truncatedText(UI_12_FONT_ID, labelStr.c_str(), maxLabelWidth, EpdFontFamily::REGULAR);
+    renderer.drawText(UI_12_FONT_ID, textX, textY, truncatedLabel.c_str(), true);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/components/themes/lyra/LyraTheme.cpp` around lines 503 - 533, In
LyraTheme::drawButtonMenu the label can overflow when an icon is drawn; compute
the available width for text inside tileRect after accounting for initial textX,
mainMenuIconSize and hPaddingInSelection, then measure and truncate the string
from buttonLabel(i) (append ellipsis) so renderer.drawText(UI_12_FONT_ID, textX,
textY, ...) is passed a label that fits; use renderer text-measure API (e.g.,
getTextWidth or getSubstringWidth) to iteratively trim or binary-search the
maximum characters that fit the available width before calling drawText.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro