refactor(FR-1057): Refactor size unit formatting in UI components#3723
Merged
Conversation
5 tasks
Member
Author
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has required the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
graphite-app
Bot
changed the base branch from
chore/display-exact-memory-value
to
graphite-base/3723
graphite-app
Bot
force-pushed
the
refactor/converter-size
branch
from
cb2839c to
b3d875a
Compare
graphite-app
Bot
force-pushed
the
graphite-base/3723
branch
from
f9dac72 to
fb95980
Compare
graphite-app
Bot
force-pushed
the
refactor/converter-size
branch
from
b3d875a to
c185c39
Compare
yomybaby
force-pushed
the
refactor/converter-size
branch
from
c185c39 to
80f88c3
Compare
Contributor
Coverage report for
|
St.❔ |
Category | Percentage | Covered / Total |
|---|---|---|---|
| 🔴 | Statements | 4.66% (+0.11% 🔼) |
486/10440 |
| 🔴 | Branches | 4.16% (+0.13% 🔼) |
306/7359 |
| 🔴 | Functions | 2.7% (+0.03% 🔼) |
89/3300 |
| 🔴 | Lines | 4.61% (+0.11% 🔼) |
471/10222 |
Show files with reduced coverage 🔻
St.❔ |
File | Statements | Branches | Functions | Lines |
|---|---|---|---|---|---|
| 🔴 | ... / DynamicUnitInputNumber.tsx |
1.52% (-0.02% 🔻) |
0% | 0% | 1.52% (-0.02% 🔻) |
| 🔴 | ... / ResourceAllocationFormItems.tsx |
13.52% | 9.29% (-0.06% 🔻) |
12.33% | 13.28% |
Test suite run failed
Failed tests: 1/159. Failed suites: 1/15.
● DomainSelect › default render
Cannot find module './__generated__/DomainSelectorQuery.graphql' from 'src/components/DomainSelector.tsx'
16 | const { t } = useTranslation();
17 |
> 18 | const { domains } = useLazyLoadQuery<DomainSelectorQuery>(
| ^
19 | graphql`
20 | query DomainSelectorQuery {
21 | domains(is_active: true) {
at Resolver.resolveModule (../node_modules/.pnpm/jest-resolve@27.5.1/node_modules/jest-resolve/build/resolver.js:324:11)
at DomainSelector (src/components/DomainSelector.tsx:18:39)
at Object.react-stack-bottom-frame (../node_modules/.pnpm/react-dom@19.0.0_react@19.0.0/node_modules/react-dom/cjs/react-dom-client.development.js:22428:20)
at renderWithHooks (../node_modules/.pnpm/react-dom@19.0.0_react@19.0.0/node_modules/react-dom/cjs/react-dom-client.development.js:5757:22)
at updateFunctionComponent (../node_modules/.pnpm/react-dom@19.0.0_react@19.0.0/node_modules/react-dom/cjs/react-dom-client.development.js:8018:19)
at beginWork (../node_modules/.pnpm/react-dom@19.0.0_react@19.0.0/node_modules/react-dom/cjs/react-dom-client.development.js:9683:18)
at runWithFiberInDEV (../node_modules/.pnpm/react-dom@19.0.0_react@19.0.0/node_modules/react-dom/cjs/react-dom-client.development.js:543:16)
at performUnitOfWork (../node_modules/.pnpm/react-dom@19.0.0_react@19.0.0/node_modules/react-dom/cjs/react-dom-client.development.js:15052:22)
at workLoopSync (../node_modules/.pnpm/react-dom@19.0.0_react@19.0.0/node_modules/react-dom/cjs/react-dom-client.development.js:14870:41)
at renderRootSync (../node_modules/.pnpm/react-dom@19.0.0_react@19.0.0/node_modules/react-dom/cjs/react-dom-client.development.js:14850:11)
at performWorkOnRoot (../node_modules/.pnpm/react-dom@19.0.0_react@19.0.0/node_modules/react-dom/cjs/react-dom-client.development.js:14384:44)
at performWorkOnRootViaSchedulerTask (../node_modules/.pnpm/react-dom@19.0.0_react@19.0.0/node_modules/react-dom/cjs/react-dom-client.development.js:15931:7)
at flushActQueue (../node_modules/.pnpm/react@19.0.0/node_modules/react/cjs/react.development.js:862:34)
at Object.<anonymous>.process.env.NODE_ENV.exports.act (../node_modules/.pnpm/react@19.0.0/node_modules/react/cjs/react.development.js:1151:10)
at ../node_modules/.pnpm/@testing-library+react@16.2.0_@testing-library+dom@10.4.0_@types+react-dom@19.0.4_@type_5aef3ee8b2555a079c2b9ce844db8ed7/node_modules/@testing-library/react/dist/act-compat.js:47:25
at renderRoot (../node_modules/.pnpm/@testing-library+react@16.2.0_@testing-library+dom@10.4.0_@types+react-dom@19.0.4_@type_5aef3ee8b2555a079c2b9ce844db8ed7/node_modules/@testing-library/react/dist/pure.js:188:26)
at render (../node_modules/.pnpm/@testing-library+react@16.2.0_@testing-library+dom@10.4.0_@types+react-dom@19.0.4_@type_5aef3ee8b2555a079c2b9ce844db8ed7/node_modules/@testing-library/react/dist/pure.js:287:10)
at Object.<anonymous> (src/components/DomainSelector.test.tsx:28:34)
Report generated by 🧪jest coverage report action from c2a1b03
yomybaby
force-pushed
the
refactor/converter-size
branch
3 times, most recently
from
94676a6 to
f9df4d5
Compare
yomybaby
changed the title
yomybaby
changed the title
yomybaby
force-pushed
the
refactor/converter-size
branch
6 times, most recently
from
2dfc63e to
30dbc25
Compare
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors size unit formatting across the UI by replacing legacy conversion functions with the new APIs (convertToBinaryUnit and convertToDecimalUnit) to deliver consistent, display-ready output. Key changes include updating unit parsing/formatting in tests and components, renaming functions for clarity, and adjusting expected outputs accordingly.
Reviewed Changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| react/src/helper/big-number.test.ts | Updated tests to reflect new conversion function API and expectations. |
| react/src/components/VFolderNodeDescription.tsx | Replaced convertDecimalSizeUnit with convertToDecimalUnit. |
| react/src/components/SessionUsageMonitor.tsx | Updated conversion calls to use convertToBinaryUnit and convertToDecimalUnit. |
| react/src/components/SessionMetricGraph.tsx | Switched to the new conversion functions for metric unit formatting. |
| react/src/components/ServiceLauncherPageContent.tsx | Adjusted conversion function calls and output properties for size values. |
| react/src/components/ResourcePresetSettingModal.tsx | Updated memory conversion logic, now returning displayValue. |
| react/src/components/ResourcePresetList.tsx | Replaced legacy conversion function parameters with the new API. |
| react/src/components/ResourceNumber.tsx | Updated conversion usage for memory sizes with new return values. |
| react/src/components/ResourceAllocationFormItems.tsx | Refactored unit conversion calls to match the new conversion API. |
| react/src/components/QuotaPerStorageVolumePanelCard.tsx | Updated quota display formatting using convertToDecimalUnit. |
| react/src/components/NumberWithUnit.tsx | Updated conversion logic to use new conversion functions and display properties. |
| react/src/components/KeypairResourcePolicySettingModal.tsx | Replaced legacy conversion function calls with the new API. |
| react/src/components/DynamicUnitInputNumberWithSlider.tsx | Updated conversion function calls for slider component. |
| react/src/components/DynamicUnitInputNumber.tsx | Switched from parseUnit to parseValueWithUnit and updated conversion calls. |
| react/src/components/ComputeSessionNodeItems/SessionSlotCell.tsx | Replaced conversion function usage for memory display values. |
| react/src/components/AvailableResourcesCard.tsx | Updated conversion calls to use convertToBinaryUnit and displayValue. |
| react/src/components/AllocationHistoryStatistics.tsx | Refactored conversion function calls; now using new convertToBinaryUnit/convertToDecimalUnit. |
| react/src/components/AgentSummaryList.tsx | Updated conversion usage for memory display in summary components. |
| react/src/components/AgentList.tsx | Replaced legacy conversion functions with the new API for memory values. |
| react/src/components/AgentDetailModal.tsx | Adjusted conversion function usage for memory and network stats displays. |
ragingwind
reviewed
Jun 5, 2025
ragingwind
reviewed
Jun 5, 2025
ragingwind
reviewed
Jun 5, 2025
ragingwind
requested changes
Jun 5, 2025
Contributor
ragingwind
left a comment
ragingwind
left a comment
There was a problem hiding this comment.
And more it would great to have resources(links and doc) why we need to those of changes like small units.
yomybaby
force-pushed
the
refactor/converter-size
branch
2 times, most recently
from
1de8a35 to
a86289d
Compare
ragingwind
approved these changes
Jun 5, 2025
nowgnuesLee
requested changes
Jun 5, 2025
Contributor
nowgnuesLee
left a comment
nowgnuesLee
left a comment
There was a problem hiding this comment.
Please resolve the conflicts :)
yomybaby
force-pushed
the
refactor/converter-size
branch
2 times, most recently
from
46e02ad to
c2a1b03
Compare
yomybaby
force-pushed
the
refactor/converter-size
branch
from
c2a1b03 to
0ec5abc
Compare
yomybaby
changed the base branch from
graphite-base/3723
to
feat/setup-babel-relay
5 tasks
Merge activity
|
) Resolves #3741 (FR-1057) ## Refactor Size Unit Display and Conversion Functions This PR refactors how size units are displayed across the UI by introducing more consistent formatting methods. It replaces the previous conversion functions (`convertBinarySizeUnit` and `convertDecimalSizeUnit`) with improved versions (`convertToBinaryUnit` and `convertToDecimalUnit`) that provide better formatted output. The changes improve readability by: 1. Standardizing the display of binary units (KiB, MiB, GiB) and decimal units (KB, MB, GB) 2. Adding `displayValue` and `displayUnit` properties to conversion results 3. Simplifying unit parsing and conversion logic 4. Making the API more consistent across all components Refactoring of Function Responsibilities: - `convertUnitValue`: Converts the unit of a value expressed with a lowercase unit. The existing 'b' unit is removed, and cases with no unit (an empty string) are treated the same as the previous 'b'. - `convertToBinaryUnit` & `convertToDecimalUnit`: The `displayValue` is shown with the unit in uppercase, followed by either 'iB' or 'B' (e.g., 123 GiB). Key improvements: - Renamed functions to better reflect their purpose (`convertToBinaryUnit` instead of `convertBinarySizeUnit`) - Changed the return value structure to include both raw values and display-ready formatted strings - Updated all component references to use the new properties - Improved handling of unit-less values and automatic unit selection - Fixed inconsistencies in how memory sizes were displayed throughout the application These changes ensure that size units are displayed with proper formatting across agent details, resource allocation forms, storage volume panels, and other components that display size information.
graphite-app
Bot
force-pushed
the
feat/setup-babel-relay
branch
from
8fab82d to
0348798
Compare
graphite-app
Bot
force-pushed
the
refactor/converter-size
branch
from
0ec5abc to
3a64247
Compare
graphite-app Bot
pushed a commit
that referenced
this pull request
Jun 16, 2025
Resolves #3783 (FR-1090) ## Fix shared memory validation using correct memory value This PR fixes a bug in the resource allocation form where shared memory validation was using the display value instead of the actual value from `appMemUnitResult`. By changing from `.displayValue` to `.value`, the validation now correctly compares the shared memory against the application memory value. This bug is caused by #3723 (FR-1057) | Before | After | |---------|--------| |  |  |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #3741 (FR-1057)
Refactor Size Unit Display and Conversion Functions
This PR refactors how size units are displayed across the UI by introducing more consistent formatting methods. It replaces the previous conversion functions (
convertBinarySizeUnitandconvertDecimalSizeUnit) with improved versions (convertToBinaryUnitandconvertToDecimalUnit) that provide better formatted output.The changes improve readability by:
displayValueanddisplayUnitproperties to conversion resultsRefactoring of Function Responsibilities:
convertUnitValue: Converts the unit of a value expressed with a lowercase unit. The existing 'b' unit is removed, and cases with no unit (an empty string) are treated the same as the previous 'b'.convertToBinaryUnit&convertToDecimalUnit: ThedisplayValueis shown with the unit in uppercase, followed by either 'iB' or 'B' (e.g., 123 GiB).Key improvements:
convertToBinaryUnitinstead ofconvertBinarySizeUnit)These changes ensure that size units are displayed with proper formatting across agent details, resource allocation forms, storage volume panels, and other components that display size information.