-
Notifications
You must be signed in to change notification settings - Fork 96
feat: added Assets page with carbon design system and service #199
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
base: main
Are you sure you want to change the base?
feat: added Assets page with carbon design system and service #199
Conversation
✅ Deploy Preview for opencost-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
02dd620 to
0ed6cdb
Compare
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
Adds a new Assets page to the OpenCost UI, including routing/navigation wiring and a new AssetsService with mock-data support for development and resiliency.
Changes:
- Introduces
AssetsService+ comprehensive mock payload for Assets API responses. - Adds a new
/assetsroute and a sidebar navigation entry. - Implements a full Assets page UI (summary KPIs, charts, table, detail modal, CSV export) and imports Carbon styles.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/services/assets.mock.js | Adds Assets API-shaped mock response and daily-series generator for fallback/dev use. |
| src/services/assets.js | Adds Assets API client wrapper with mock fallback behavior. |
| src/route.js | Registers new /assets route. |
| src/pages/Assets.js | Implements the Assets page UI, data fetching, window controls, charts, table, modal, and CSV export. |
| src/css/index.css | Imports Carbon global styles. |
| src/components/assets/tokens.js | Adds window/aggregation options and color/icon tokens for Assets UI. |
| src/components/assets/assetTable.js | Adds sortable assets table with utilization indicators and detail trigger. |
| src/components/assets/assetDetailModal.js | Adds modal for detailed per-asset inspection. |
| src/components/assets/assetControls.js | Adds window/aggregation/currency controls and CSV download action. |
| src/components/assets/assetCharts.js | Adds cost/utilization visualizations (pie, bar, treemap). |
| src/components/assets/SummaryCard.js | Adds reusable KPI card with trend indicator. |
| src/components/assets/SavingsOpportunities.js | Adds “savings opportunities” tiles (uses Carbon Tile). |
| src/components/Page.js | Adjusts layout sizing/padding to support full-page content. |
| src/components/Nav/SidebarNav.js | Adds “Assets” link to the left nav. |
| package.json | Adds @carbon/react dependency. |
| package-lock.json | Locks @carbon/react and its transitive dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case "24h": return 1; | ||
| case "48h": return 2; | ||
| case "week": { | ||
| const d = new Date().getDay(); |
Copilot
AI
Feb 8, 2026
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.
getWindowDays uses local time (new Date().getDay()) for the week-to-date calculation, but other window helpers (e.g. util.toVerboseTimeRange) use UTC (getUTCDay). This can cause inconsistent “days” calculations depending on the viewer’s timezone, which affects forecasts and previous-window comparisons. Use UTC consistently for week-to-date (getUTCDay) to match the rest of the app’s window handling.
| const d = new Date().getDay(); | |
| const d = new Date().getUTCDay(); |
| function downloadCSV(assets, currency) { | ||
| const header = "Name,Type,Provider,Cluster,Category,CPU Cost,RAM Cost,GPU Cost,Adjustment,Total Cost\n"; | ||
| const rows = assets.map((a) => { |
Copilot
AI
Feb 8, 2026
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.
downloadCSV takes a currency argument but doesn’t use it (values are always raw toFixed(2)). Either remove the unused parameter or format values consistently (e.g., include currency code or use toCurrency).
| display: "flex", | ||
| overflowY: "scroll", | ||
| margin: "0px", | ||
| backgroundColor: "f3f3f3", |
Copilot
AI
Feb 8, 2026
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.
In the Page container style, backgroundColor is set to "f3f3f3" (missing the leading "#"), which is not a valid CSS color and will be ignored. If the intent is to match the body background, this should be "#f3f3f3" (or remove it and rely on the global body style).
| backgroundColor: "f3f3f3", | |
| backgroundColor: "#f3f3f3", |
| function downloadCSV(assets, currency) { | ||
| const header = "Name,Type,Provider,Cluster,Category,CPU Cost,RAM Cost,GPU Cost,Adjustment,Total Cost\n"; | ||
| const rows = assets.map((a) => { | ||
| return [ | ||
| `"${a.properties?.name || a.key || ""}"`, | ||
| a.type, | ||
| a.properties?.provider || "", | ||
| a.properties?.cluster || "", | ||
| a.properties?.category || "", |
Copilot
AI
Feb 8, 2026
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.
CSV export wraps the name in quotes but doesn’t escape embedded quotes inside the name/key. A name containing '"' will produce an invalid CSV row. Ensure values are properly CSV-escaped (e.g., double quotes inside quoted fields).
| function downloadCSV(assets, currency) { | |
| const header = "Name,Type,Provider,Cluster,Category,CPU Cost,RAM Cost,GPU Cost,Adjustment,Total Cost\n"; | |
| const rows = assets.map((a) => { | |
| return [ | |
| `"${a.properties?.name || a.key || ""}"`, | |
| a.type, | |
| a.properties?.provider || "", | |
| a.properties?.cluster || "", | |
| a.properties?.category || "", | |
| function csvEscape(value) { | |
| if (value === null || value === undefined) { | |
| return '""'; | |
| } | |
| const str = String(value); | |
| const escaped = str.replace(/"/g, '""'); | |
| return `"${escaped}"`; | |
| } | |
| function downloadCSV(assets, currency) { | |
| const header = "Name,Type,Provider,Cluster,Category,CPU Cost,RAM Cost,GPU Cost,Adjustment,Total Cost\n"; | |
| const rows = assets.map((a) => { | |
| return [ | |
| csvEscape(a.properties?.name || a.key || ""), | |
| csvEscape(a.type || ""), | |
| csvEscape(a.properties?.provider || ""), | |
| csvEscape(a.properties?.cluster || ""), | |
| csvEscape(a.properties?.category || ""), |
|
|
||
| import { NavItem } from "./NavItem"; | ||
| import { BarChart, Cloud } from "@mui/icons-material"; | ||
| import { BarChart, Cloud,Dns } from "@mui/icons-material"; |
Copilot
AI
Feb 8, 2026
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.
The icon import has inconsistent spacing ("Cloud,Dns "), which is minor but makes diffs noisy and can violate formatting checks. Please format the named imports consistently (spaces after commas, no extra spaces).
| }, | ||
| { name: "Cloud Costs", href: "/cloud", icon: <Cloud /> }, | ||
| { name: "External Costs", href: "/external-costs", icon: <Cloud /> }, | ||
| { name: "Assets", href: "/assets", icon: <Dns /> }, |
Copilot
AI
Feb 8, 2026
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.
The new Assets nav item JSX has extra spaces ("") and a trailing space at the end of the line. Please format the icon usage consistently to keep the file clean.
src/components/assets/assetTable.js
Outdated
| function bytesToShort(bytes) { | ||
| if (!bytes || bytes === 0) return "-"; | ||
| const sizes = ["B", "KB", "MB", "GB", "TB"]; | ||
| const i = Math.floor(Math.log(bytes) / Math.log(1024)); | ||
| return (bytes / Math.pow(1024, i)).toFixed(1) + " " + sizes[i]; | ||
| } | ||
|
|
Copilot
AI
Feb 8, 2026
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.
bytesToShort() is defined but never used in this component. It’s dead code and will likely trip linting/maintenance. Remove it, or use it for disk/node byte fields if that was the intent.
| function bytesToShort(bytes) { | |
| if (!bytes || bytes === 0) return "-"; | |
| const sizes = ["B", "KB", "MB", "GB", "TB"]; | |
| const i = Math.floor(Math.log(bytes) / Math.log(1024)); | |
| return (bytes / Math.pow(1024, i)).toFixed(1) + " " + sizes[i]; | |
| } |
| if ( | ||
| USE_MOCK_DATA || | ||
| (error.message && | ||
| (error.message.includes("Network Error") || | ||
| error.message.includes("ECONNREFUSED"))) || |
Copilot
AI
Feb 8, 2026
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.
Mock fallback is triggered for network errors / 404 even when REACT_APP_USE_MOCK_DATA is not enabled. This differs from the existing pattern in AllocationService (only mock when the flag is true) and can mask real backend/config errors for users. Consider only falling back when USE_MOCK_DATA is true (or gating automatic fallback behind a non-production check), and otherwise surface the error.
| export function getAssetsMockData(aggregate = "type", accumulate = true) { | ||
| if (accumulate) { | ||
| return { | ||
| code: 200, | ||
| data: [getMockAssetsByType()], |
Copilot
AI
Feb 8, 2026
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.
getAssetsMockData accepts an aggregate argument but the accumulated mock payload is always generated by getMockAssetsByType(), so switching aggregation modes in the UI won’t be reflected when running on mock data. Either implement aggregation-aware mock generation or remove the aggregate parameter to avoid implying support that isn’t there.
src/services/assets.mock.js
Outdated
| function generateDailyData(days) { | ||
| const result = []; | ||
| const base = getMockAssetsByType(); | ||
| for (let d = 0; d < days; d++) { | ||
| const dayData = {}; | ||
| for (const [key, asset] of Object.entries(base)) { | ||
| const factor = 0.8 + Math.random() * 0.4; // vary costs 80%-120% |
Copilot
AI
Feb 8, 2026
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.
generateDailyData uses Math.random() to vary costs, making the mock fallback dataset non-deterministic across refreshes/renders. Prefer a deterministic approach (e.g., seeded by day index) so the same window consistently produces the same values.
| function generateDailyData(days) { | |
| const result = []; | |
| const base = getMockAssetsByType(); | |
| for (let d = 0; d < days; d++) { | |
| const dayData = {}; | |
| for (const [key, asset] of Object.entries(base)) { | |
| const factor = 0.8 + Math.random() * 0.4; // vary costs 80%-120% | |
| function deterministicRandom(dayIndex, key) { | |
| // Simple deterministic hash-based pseudo-random generator in [0, 1) | |
| const str = `${dayIndex}:${key}`; | |
| let hash = 0; | |
| for (let i = 0; i < str.length; i++) { | |
| hash = (hash << 5) - hash + str.charCodeAt(i); | |
| hash |= 0; // Convert to 32-bit integer | |
| } | |
| // Convert to an unsigned 32-bit integer and normalize | |
| return (hash >>> 0) / 0xffffffff; | |
| } | |
| function generateDailyData(days) { | |
| const result = []; | |
| const base = getMockAssetsByType(); | |
| for (let d = 0; d < days; d++) { | |
| const dayData = {}; | |
| for (const [key, asset] of Object.entries(base)) { | |
| const factor = 0.8 + deterministicRandom(d, key) * 0.4; // vary costs 80%-120% deterministically |
- Updated route configuration to include a new Assets page. - Implemented AssetsService to fetch asset data with a fallback to mock data. - Created comprehensive mock data for the Assets API to simulate real API responses. Signed-off-by: Muhammad Adeel <118283843+muhammadadeel147@users.noreply.github.com>
Signed-off-by: Muhammad Adeel <118283843+muhammadadeel147@users.noreply.github.com>
Signed-off-by: Muhammad Adeel <118283843+muhammadadeel147@users.noreply.github.com>
… System Signed-off-by: Muhammad Adeel <118283843+muhammadadeel147@users.noreply.github.com>
ff0e15f to
28c110e
Compare
|
I have reviewed all your files and one thing I see is that the assets service can silently fall back to mock data on network or 404 errors.And since the UI treats this data as real and uses it for efficiency scores, savings, and trends, this could surface fabricated cost data without any indication, which is risky for a cost/finance feature. |
@emphor11 Thanks for the review — this was intentional for the scope of the LFX coding challenge to keep the Assets UI fully reviewable using API-shaped mock data when the backend is unavailable. |
|
@muhammadadeel147 Yeah, thanks for clarifying an that makes sense for the LFX challenge. My concern is mainly that the fallback is silent and indistinguishable from real data at the UI level, especially since efficiency, savings, and trends are computed on top of it. Even a small indicator shown when mock data is in use could help avoid misinterpretation. |
Thanks @emphor11 , that’s a good point. For the challenge, I kept the fallback silent to keep the UI reviewable, but I’ll add an indicator for mock data in a future update. |
What does this PR change?
Does this PR relate to any other PRs?
How will this PR impact users?
Does this PR address any GitHub or Zendesk issues?
How was this PR tested?
Does this PR require changes to documentation?
Have you labeled this PR and its corresponding Issue as "next release" if it should be part of the next OpenCost release? If not, why not?