Skip to content

Use appropriate icon for NoApplications. Add embedded column to apps#388

Merged
celdrake merged 1 commit into
flightctl:mainfrom
celdrake:small-changes-to-applications
Nov 26, 2025
Merged

Use appropriate icon for NoApplications. Add embedded column to apps#388
celdrake merged 1 commit into
flightctl:mainfrom
celdrake:small-changes-to-applications

Conversation

@celdrake

@celdrake celdrake commented Nov 26, 2025

Copy link
Copy Markdown
Collaborator
  • Change icon for "NoApplications"
  • Adds "embedded" column to ApplicationsTable.

Summary by CodeRabbit

  • New Features
    • Added "Embedded" column to Applications Table displaying Yes/No status for embedded applications
    • Updated the "No Applications" status icon display

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai

coderabbitai Bot commented Nov 26, 2025

Copy link
Copy Markdown

Walkthrough

This PR adds i18n translations for "Embedded", "Yes", and "No", introduces an "Embedded" column to the ApplicationsTable displaying yes/no status based on appDetails.embedded property, refactors default app details into a reusable object, and replaces an icon in the applications status utility.

Changes

Cohort / File(s) Summary
Embedded Column Feature
libs/i18n/locales/en/translation.json, libs/ui-components/src/components/DetailsPage/Tables/ApplicationsTable.tsx
Adds i18n translations for "Embedded", "Yes", "No"; introduces Embedded column to ApplicationsTable displaying yes/no status based on appDetails.embedded; refactors to use reusable emptyAppDetails object as default for missing statuses
Status Utility
libs/ui-components/src/utils/status/applications.ts
Replaces AsleepIcon with ResourcesEmptyIcon in NoApplications status item

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10–12 minutes

  • Verify the Embedded column renders correctly with the Yes/No/fallback logic in various states
  • Ensure emptyAppDetails object includes all necessary default properties for safe fallback behavior
  • Confirm i18n keys are consistently applied across the component and match translation entries

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the two main changes: updating the icon for NoApplications and adding an embedded column to the applications table.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
libs/ui-components/src/components/DetailsPage/Tables/ApplicationsTable.tsx (1)

16-21: Clarify default Embedded value for spec‑only applications

emptyAppDetails sets embedded: false, and the cell logic maps false to t('No'). This means apps that exist only in specApps (no status yet) will render as “No” rather than “-”, even though we don’t actually know their embedded state.

If you intend “unknown until status arrives”, consider leaving embedded undefined in the fallback so those rows stay “-”:

-const emptyAppDetails: Partial<DeviceApplicationStatus> = {
-  name: '',
-  ready: '-',
-  restarts: 0,
-  embedded: false,
-};
+const emptyAppDetails: Partial<DeviceApplicationStatus> = {
+  name: '',
+  ready: '-',
+  restarts: 0,
+  // embedded intentionally omitted so unknown renders as '-'
+};

The rest of the Embedded column (header + Yes/No/- rendering) looks good.

Also applies to: 46-47, 51-57, 70-70

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 90ae3b4 and 0d5d8d8.

📒 Files selected for processing (3)
  • libs/i18n/locales/en/translation.json (1 hunks)
  • libs/ui-components/src/components/DetailsPage/Tables/ApplicationsTable.tsx (3 hunks)
  • libs/ui-components/src/utils/status/applications.ts (2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 371
File: libs/types/models/AppType.ts:10-10
Timestamp: 2025-10-29T16:47:29.614Z
Learning: PR #371 (flightctl/flightctl-ui) adds the AppTypeQuadlet enum member and related types as a preparatory change. Full implementation of quadlet application support in the UI will be added later, after backend support is available.
📚 Learning: 2025-02-04T09:04:36.106Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 207
File: libs/ui-components/src/components/Device/EditDeviceWizard/steps/ReviewDeviceStep.tsx:55-55
Timestamp: 2025-02-04T09:04:36.106Z
Learning: The codebase has automated CI checks ("lint" step) that verify translation coverage for all text strings, ensuring that new or modified strings have corresponding entries in translation files.

Applied to files:

  • libs/i18n/locales/en/translation.json
🔇 Additional comments (2)
libs/i18n/locales/en/translation.json (1)

249-251: New translations align with Applications table usage

"Embedded", "Yes", and "No" are well-placed next to the device applications table strings and match the new column semantics. No issues from the JSON/i18n perspective.

libs/ui-components/src/utils/status/applications.ts (1)

8-32: Unable to complete verification due to repository access issues

The repository clone operation is failing consistently, preventing execution of the verification script to check for lingering AsleepIcon references. However, based on the visible code snippet (lines 8-32 of libs/ui-components/src/utils/status/applications.ts):

  • The ResourcesEmptyIcon import is correct
  • Its usage as customIcon for ApplicationsSummaryStatusNoApplications is syntactically sound and semantically appropriate
  • No type mismatches are evident in the code shown

The original review comment's approval of the icon change is reasonable, but the requested verification of orphaned AsleepIcon references cannot be confirmed.

@celdrake celdrake merged commit 2756565 into flightctl:main Nov 26, 2025
6 checks passed
@celdrake celdrake deleted the small-changes-to-applications branch November 26, 2025 15:26
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.

2 participants