Skip to content

fix(dynamix): prevent string offset warning when thousands separator is none - #2789

Open
Lewi (ainto) wants to merge 3 commits into
unraid:masterfrom
ainto:master
Open

Lewi (ainto) wants to merge 3 commits into
unraid:masterfrom
ainto:master

Conversation

@ainto

@ainto Lewi (ainto) commented Oct 16, 2026 •

Copy link
Copy Markdown

Description
This PR fixes a PHP warning (Warning: Uninitialized string offset 1) that occurs in Helpers.php when a user selects the [G] none option for the thousands separator in Display Settings.

The functions my_number() and my_scale() assume that the $display['number'] string always contains two characters (a decimal point and a thousands separator). When [G] none is selected, the string only contains one character (the decimal point), causing a warning when the code attempts to access the second character ($number[1]).

How to reproduce the bug

  1. Go to Settings -> Display Settings.
  2. Change the Number format, Number scaling, or Page view dropdown to [D] dot : [G] none (or [D] comma : [G] none).
  3. Click Apply.
  4. Navigate to the Main tab or Dashboard where large numbers (like disk sizes or file counts >= 10,000) are displayed.
  5. A PHP warning will appear at the top of the page (The premise has enabled error display): Warning: Uninitialized string offset 1 in /usr/local/emhttp/plugins/dynamix/include/Helpers.php on line ...

Solution / Changes Made
I used the null coalescing operator (??) to safely check if the thousands separator character exists before using it. If it doesn't exist (i.e., the user selected none), it defaults to an empty string ''.

Modified functions in /usr/local/emhttp/plugins/dynamix/include/Helpers.php:

  • my_scale()
  • my_number()

Impact

  • Positive: Fixes the PHP warning for users who prefer not to use a thousands separator.
  • Compatibility: Fully backward compatible. Users with standard settings (e.g., [D] dot : [G] comma) will see no change in behavior, as $number[1] will still be correctly read.
  • Performance: Negligible. The null coalescing operator is highly optimized in PHP.

Summary by CodeRabbit

  • Bug Fixes
    • Number formatting now avoids adding a thousands separator when none is configured.
  • Style
    • UPS header details now appear on the same line as the status, and tile header text stays on a single line.

@coderabbitai

coderabbitai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 9d3ad024-4d0e-4e51-9776-65dc28ce679b
📥 Commits

Reviewing files that changed from the base of the PR and between d14d89b and 057332b.

📒 Files selected for processing: 2
  • emhttp/plugins/dynamix/DashStats.page
  • emhttp/plugins/dynamix/sheets/DashStats.css
💤 Files with no reviewable changes: 1
  • emhttp/plugins/dynamix/DashStats.page

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review.

Walkthrough

The number-format helpers now use an empty thousands separator when the configured format has no second character. The UPS dashboard header no longer inserts an explicit line break after its status element, and its main text does not wrap or shrink.

Changes

Number Format Helpers

Layer / File(s) Summary
Thousands separator fallback
emhttp/plugins/dynamix/include/Helpers.php
Both helpers use an empty separator when the configured number-format string has no second character. my_scale keeps its > 9999 threshold, and my_number keeps its >= 10000 threshold.

UPS Dashboard Header

Layer / File(s) Summary
UPS header layout
emhttp/plugins/dynamix/DashStats.page, emhttp/plugins/dynamix/sheets/DashStats.css
The UPS status element no longer has an explicit line break before the model and load details. .tile-header-main does not wrap or shrink.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 05733

The targeted formatting fix handles the no-separator setting, and the dashboard header layout concern was not reproduced. No material PR-introduced risk remains.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the primary fix for the string offset warning caused by a missing thousands separator.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

A rabbit checks the numbers with care,
No missing separator waits there.
The UPS details follow in line,
Header text stays neat and fine.
One hop, one change, all clear to see.

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

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