Conversation
eb9b61a to
c0b2398
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unit replacement fails for several supported formats, and trimming prevents typing labels containing spaces.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds display-only custom labels while retaining standard unit formatting and keys.
Changes:
- Extends CUE and TypeScript format options with
customLabel. - Applies custom labels to formatted values and unit metadata.
- Adds a custom-label field and focused tests.
File summaries
| File | Description |
|---|---|
cue/common/format.cue |
Adds the optional schema field. |
cue-test/common/format.cue |
Adds a valid schema example. |
components/src/model/units.ts |
Integrates custom-label formatting. |
components/src/model/index.ts |
Exports the new API. |
components/src/model/custom.ts |
Implements label replacement. |
components/src/model/custom.test.ts |
Tests custom-label behavior. |
components/src/FormatControls/UnitSelector.tsx |
Simplifies null handling. |
components/src/FormatControls/FormatControls.tsx |
Adds custom-label controls. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const lastSpace = formatted.lastIndexOf(' '); | ||
| if (lastSpace > 0) { | ||
| return `${formatted.slice(0, lastSpace)} ${label}`; | ||
| } | ||
| return `${formatted} ${label}`; |
There was a problem hiding this comment.
Indeed there are issues with the current logic, that's what I got by trying some units:
- time units are not properly replaced by the logic
in some cases you can even end up with a mix of replaced & not replaced like here:
(here it's with a trend at value 1 and unit=Months)
- for throughput units I guess the output is wrong too?
6611598 to
13af838
Compare
|
As always, screenshot please 🤓 |
Hi Antoine, yes here it is 
|
I dont see the code related to the "showcase" + detail part at the end, are you sure this screenshot is up to date? I'm fine with the code btw, I dont feel this detail is necessary to display |
You’re right, the earlier PR description still mentioned a “Custom” unit / showcase detail that we dropped. |
|
here it is 
|
Strip Intl time suffixes (ms, month, …) and rate units so customLabel fully replaces the standard unit text (Antoine review on perses#297). Signed-off-by: colivi <charles.olivi@gmail.com>
…perses#297) Signed-off-by: colivi <charles.olivi@gmail.com>
753fcc1 to
0071813
Compare
Allow dashboards to keep a CUE-safe unit key (ops/sec, milliseconds, …) while displaying a business label (pnr/mn, latency, …) on axes, legends, and tooltips. - FormatControls: optional Custom label (whitelist of supported units) - applyCustomLabel: strip Intl time suffixes; count-rate throughput - Bits/bytes excluded (SI size prefixes not reapplied to free-form labels) - CUE #format.customLabel (strings.MinRunes(1), factored once) - Tests for percent/temp/ops/sec, multi-scale months, unsupported units Signed-off-by: colivi <charles.olivi@gmail.com>
0071813 to
b10aed4
Compare
Description
Adds support for custom unit display labels without expanding the standard CUE unit enum.
Problem
Operational dashboards (and sometimes customer-facing views) use Grafana-era labels such as "pnr/mn", "trx/s", "pax/mn".
Mapping everything to generic units like "ops/sec" is CUE-valid but loses the domain language users expect.
Growing the shared enum with org-specific aliases (e.g. "msg/s" next to "messages/sec") does not scale.
Approach
Changes
Example JSON
json { "unit": "ops/sec", "customLabel": "pnr/mn", "shortValues": true }Renders like 1.5K pnr/mn (numeric formatting from ops/sec, label from customLabel).
Screenshots
Checklist
[<catalog_entry>] <commit message>naming convention using one of thefollowing
catalog_entryvalues:FEATURE,ENHANCEMENT,BUGFIX,BREAKINGCHANGE,DOC,IGNORE.UI Changes
See e2e docs for more details. Common issues include: