EDM-3383: Refresh device before displaying YAML#547
Open
celdrake wants to merge 1 commit intoflightctl:mainfrom
Open
EDM-3383: Refresh device before displaying YAML#547celdrake wants to merge 1 commit intoflightctl:mainfrom
celdrake wants to merge 1 commit intoflightctl:mainfrom
Conversation
WalkthroughAdds a translation entry "Refreshing device"; introduces DeviceYamlTab (new React component) that manages loading state and shows a spinner while calling refetch before rendering YamlEditor; updates DeviceDetailsPage to use DeviceYamlTab and renames the edit-disable prop. Changes
Sequence Diagram(s)sequenceDiagram
participant Page as DeviceDetailsPage
participant Tab as DeviceYamlTab
participant API as Refetch/API
participant Editor as YamlEditor
Page->>Tab: render with props (device, refetch, editDisabledReason, canEdit)
Tab->>API: call refetch()
Tab-->>Tab: show spinner briefly
API-->>Tab: return latest device data
Tab->>Editor: render YamlEditor with apiObj=device, refetch, disabledEditReason, canEdit
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
libs/ui-components/src/components/Device/DeviceDetails/DeviceYamlTab.tsx (1)
45-45: PassdisabledEditReasononly whencanEditis true.This keeps the YAML editor aligned with the project’s RBAC UX rule (hide action when no permission; only disable with reason for config constraints).
♻️ Proposed refinement
- return <YamlEditor apiObj={device} refetch={refetch} disabledEditReason={editDisabledReason} canEdit={canEdit} />; + return ( + <YamlEditor + apiObj={device} + refetch={refetch} + disabledEditReason={canEdit ? editDisabledReason : undefined} + canEdit={canEdit} + /> + );Based on learnings: “when canEdit is false (no permission), the Save button is completely hidden (onSave=undefined), so disabledEditReason should not be passed; when canEdit is true, disabledEditReason can be passed to disable the button for configuration reasons.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Device/DeviceDetails/DeviceYamlTab.tsx` at line 45, The DeviceYamlTab currently always passes disabledEditReason to YamlEditor which violates the RBAC UX; update the return so that disabledEditReason is only provided when canEdit is true (keep passing canEdit and refetch/device as before) so that when canEdit is false the YAML editor receives no disabledEditReason (and Save remains hidden via onSave being undefined). Modify the JSX in DeviceYamlTab to conditionally include the disabledEditReason prop based on the canEdit boolean, leaving all other props (apiObj/device, refetch, canEdit) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/i18n/locales/en/translation.json`:
- Line 326: The "Refreshing device" translation key was added directly to the
locale file; remove this manual edit and instead add the call t('Refreshing
device') in the source UI code where the string is used (or fix/remove the
source string), then regenerate the locale JSON by running the i18n extraction
script (npm run i18n) so libs/i18n/locales/en/translation.json is updated from
source keys rather than manual edits.
In `@libs/ui-components/src/components/Device/DeviceDetails/DeviceYamlTab.tsx`:
- Around line 25-35: The current useEffect in DeviceYamlTab relies on an
arbitrary timeout (YAML_TAB_REFETCH_DELAY_MS) after calling the synchronous
refetch(), causing a race; instead have the parent pass the fetch status (e.g.,
isUpdating from useFetchPeriodically) into DeviceYamlTab and gate ready on that
flag: call refetch() as before, remove showSpinnerBriefly and the fixed timeout,
and setReady based on isUpdating (e.g., setReady(!isUpdating) in an effect that
depends on isUpdating and refetch), or if passing isUpdating is not feasible,
update the comment to say refetch() only triggers a background fetch and that
the delay is an intentional UX trade-off; do not change the refetch:
VoidFunction type.
---
Nitpick comments:
In `@libs/ui-components/src/components/Device/DeviceDetails/DeviceYamlTab.tsx`:
- Line 45: The DeviceYamlTab currently always passes disabledEditReason to
YamlEditor which violates the RBAC UX; update the return so that
disabledEditReason is only provided when canEdit is true (keep passing canEdit
and refetch/device as before) so that when canEdit is false the YAML editor
receives no disabledEditReason (and Save remains hidden via onSave being
undefined). Modify the JSX in DeviceYamlTab to conditionally include the
disabledEditReason prop based on the canEdit boolean, leaving all other props
(apiObj/device, refetch, canEdit) unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/i18n/locales/en/translation.jsonlibs/ui-components/src/components/Device/DeviceDetails/DeviceDetailsPage.tsxlibs/ui-components/src/components/Device/DeviceDetails/DeviceYamlTab.tsx
libs/ui-components/src/components/Device/DeviceDetails/DeviceYamlTab.tsx
Show resolved
Hide resolved
celdrake
force-pushed
the
EDM-3383-refresh-device-in-yaml-tab
branch
from
f653751 to
ba314a8
Compare
celdrake
force-pushed
the
EDM-3383-refresh-device-in-yaml-tab
branch
from
ba314a8 to
a742992
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libs/ui-components/src/components/Device/DeviceDetails/DeviceYamlTab.tsx (1)
45-45: Only passdisabledEditReasonwhencanEditis true.To match the YAML editor UX contract, avoid forwarding a disabled reason in the RBAC-hidden path.
♻️ Proposed fix
- return <YamlEditor apiObj={device} refetch={refetch} disabledEditReason={editDisabledReason} canEdit={canEdit} />; + return ( + <YamlEditor + apiObj={device} + refetch={refetch} + disabledEditReason={canEdit ? editDisabledReason : undefined} + canEdit={canEdit} + /> + );Based on learnings: when
canEditis false (no permission), the Save button is hidden anddisabledEditReasonshould not be passed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Device/DeviceDetails/DeviceYamlTab.tsx` at line 45, DeviceYamlTab is currently always forwarding editDisabledReason to YamlEditor; change the JSX so disabledEditReason is only passed when canEdit is true. Locate the return rendering of YamlEditor in DeviceYamlTab and conditionally include the disabledEditReason prop (e.g. spread a prop object only if canEdit) while still passing apiObj={device} and refetch={refetch} and canEdit={canEdit}.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@libs/ui-components/src/components/Device/DeviceDetails/DeviceYamlTab.tsx`:
- Line 45: DeviceYamlTab is currently always forwarding editDisabledReason to
YamlEditor; change the JSX so disabledEditReason is only passed when canEdit is
true. Locate the return rendering of YamlEditor in DeviceYamlTab and
conditionally include the disabledEditReason prop (e.g. spread a prop object
only if canEdit) while still passing apiObj={device} and refetch={refetch} and
canEdit={canEdit}.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/i18n/locales/en/translation.jsonlibs/ui-components/src/components/Device/DeviceDetails/DeviceDetailsPage.tsxlibs/ui-components/src/components/Device/DeviceDetails/DeviceYamlTab.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/i18n/locales/en/translation.json
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
1 participant
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.
When the YAML editor opens, we now refresh the device details so that the editor shows the most updated version.
This helps for example when quickly switching from the Terminal tab to the YAML tab, as the device annotations are updated.
Summary by CodeRabbit
New Features
UI Improvements