-
Notifications
You must be signed in to change notification settings - Fork 78
fix(FR-1786): fix session launcher resource form initialization #4811
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(FR-1786): fix session launcher resource form initialization #4811
Conversation
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. |
Coverage report for
|
St.❔ |
Category | Percentage | Covered / Total |
|---|---|---|---|
| 🔴 | Statements | 4.38% | 517/11816 |
| 🔴 | Branches | 3.59% | 298/8312 |
| 🔴 | Functions | 2.63% | 95/3615 |
| 🔴 | Lines | 4.35% | 503/11557 |
Test suite run success
145 tests passing in 14 suites.
Report generated by 🧪jest coverage report action from 6968722
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a critical bug in the session launcher resource form where page refreshes caused incorrect initialization of resource allocation values. The root cause was improper handling of undefined values in the InputNumberWithSlider component, which triggered unwanted form updates and preset changes.
Key Changes:
- Fixed
InputNumberWithSliderto preventsetValue(0)calls when value isundefinedby adding_.isNil(value)check - Added optional chaining for safer access to
shmemfield properties in resource allocation form - Replaced dynamic field key retrieval with explicit field key declaration in
SessionLauncherPreviewfor reliable error detection
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| react/src/components/InputNumberWithSlider.tsx | Added null/undefined check before integer normalization to prevent NaN-related bugs that triggered unwanted onChange events |
| react/src/components/SessionFormItems/ResourceAllocationFormItems.tsx | Added optional chaining for shmem field access; refactored allocation preset checks to use consistent variable reference; removed trailing whitespace |
| react/src/components/SessionLauncherPreview.tsx | Replaced dynamic field key retrieval with explicit array of resource field keys to ensure proper error detection even when field values are undefined |
After thoroughly reviewing this PR, I found that all changes are well-implemented and appropriately address the described issue. The fixes are:
-
InputNumberWithSlider.tsx: The addition of
!_.isNil(value)correctly prevents the component from trying to normalizeundefinedvalues, which was causingundefined % 1to returnNaNand triggering unwantedsetValue(0)calls. -
ResourceAllocationFormItems.tsx: The optional chaining additions (
?.shmem) are appropriate defensive programming practices. The refactoring fromform.getFieldValue('allocationPreset')tocurrentAllocationPresetimproves code consistency and readability without changing logic. -
SessionLauncherPreview.tsx: The change from dynamically retrieving keys via
_.some(form.getFieldValue('resource'), (_v, key) => ...)to explicitly declaring the field keys is a solid fix. When field values don't exist,getFieldValuemay not return all keys, which would cause error detection to fail.
All changes follow TypeScript best practices, maintain proper null safety, and align with the project's coding standards. No i18n violations were found, and the code quality is good.
nowgnuesLee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
1596adc to
7592523
Compare
yomybaby
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Merge activity
|
### Resolves #4821 ([FR-1786](https://lablup.atlassian.net/browse/FR-1786)) Add RESOURCE_ALLOCATION_INITIAL_FORM_VALUES to SessionLauncherPage This PR adds the `RESOURCE_ALLOCATION_INITIAL_FORM_VALUES` constant to the merged initial values in the `SessionLauncherPage` component. This ensures that resource allocation form values are properly initialized when the page loads. ## Changes - Import `RESOURCE_ALLOCATION_INITIAL_FORM_VALUES` from ResourceAllocationFormItems - Include this constant in the merged initial values calculation to ensure proper resource allocation defaults This change helps maintain consistent resource allocation values during page initialization and prevents potential undefined values in the form. [FR-1786]: https://lablup.atlassian.net/browse/FR-1786?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
7592523 to
6968722
Compare

Resolves #4821 (FR-1786)
Add RESOURCE_ALLOCATION_INITIAL_FORM_VALUES to SessionLauncherPage
This PR adds the
RESOURCE_ALLOCATION_INITIAL_FORM_VALUESconstant to the merged initial values in theSessionLauncherPagecomponent. This ensures that resource allocation form values are properly initialized when the page loads.Changes
RESOURCE_ALLOCATION_INITIAL_FORM_VALUESfrom ResourceAllocationFormItemsThis change helps maintain consistent resource allocation values during page initialization and prevents potential undefined values in the form.