Skip to content
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

feat: add absolute timestamp to uptime #1768

Merged
merged 2 commits into from
Dec 19, 2024

Conversation

artemmufazalov
Copy link
Member

@artemmufazalov artemmufazalov commented Dec 18, 2024

Closes #1200

Screenshot 2024-12-18 at 18 18 25

Stand: https://nda.ya.ru/t/bi-P0mBz7AW239

CI Results

Test Status: βœ… PASSED

πŸ“Š Full Report

Total Passed Failed Flaky Skipped
222 222 0 0 0

😟 No changes in tests. πŸ˜•

Bundle Size: πŸ”Ί

Current: 65.89 MB | Main: 65.88 MB
Diff: +5.32 KB (0.01%)

⚠️ Bundle size increased. Please review.

ℹ️ CI Information
  • Test recordings for failed tests are available in the full report.
  • Bundle size is measured for the entire 'dist' directory.
  • πŸ“Š indicates links to detailed reports.
  • πŸ”Ί indicates increase, πŸ”½ decrease, and βœ… no change in bundle size.

Comment on lines -77 to -84
let Uptime: PreparedNodeSystemState['Uptime'];

if (systemState.DisconnectTime) {
Uptime = getDowntimeFromDateFormatted(systemState.DisconnectTime);
} else {
Uptime = getUptimeFromDateFormatted(systemState.StartTime);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

It's more about display, moved it to NodeUptime component

@artemmufazalov artemmufazalov force-pushed the 1200-add-timestamp-to-uptime branch 2 times, most recently from b5699d7 to 1b7b4b7 Compare December 18, 2024 15:41
Comment on lines +2 to +9
display: inline-flex;

max-width: 100%;

&_full-width {
display: flex;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

We need flex to display progress bars correctly, for other values we need inline-flex for proper align inside table

@artemmufazalov artemmufazalov force-pushed the 1200-add-timestamp-to-uptime branch from 1b7b4b7 to 01181c4 Compare December 18, 2024 15:45
@astandrik astandrik requested a review from Copilot December 19, 2024 09:01

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 15 changed files in this pull request and generated no comments.

Files not reviewed (10)
  • src/components/CellWithPopover/CellWithPopover.scss: Language not supported
  • src/components/UptimeViewer/i18n/en.json: Language not supported
  • src/components/CellWithPopover/CellWithPopover.tsx: Evaluated as low risk
  • src/components/nodesColumns/columns.tsx: Evaluated as low risk
  • src/components/TooltipsContent/TabletTooltipContent/TabletTooltipContent.tsx: Evaluated as low risk
  • src/containers/Tablet/components/TabletInfo/TabletInfo.tsx: Evaluated as low risk
  • src/utils/nodes.ts: Evaluated as low risk
  • src/containers/Tablets/TabletsTable.tsx: Evaluated as low risk
  • src/containers/Tablet/components/TabletTable/TabletTable.tsx: Evaluated as low risk
  • src/utils/dataFormatters/test/formatUptime.test.ts: Evaluated as low risk
Comments suppressed due to low confidence (2)

src/utils/dataFormatters/dataFormatters.ts:49

  • Returning undefined for non-numeric values in formatUptimeInSeconds may cause issues where a placeholder is expected. Consider returning a consistent placeholder value.
return undefined;

src/components/FullNodeViewer/FullNodeViewer.tsx:36

  • The NodeUptime component should handle cases where both StartTime and DisconnectTime are undefined. Currently, it returns EMPTY_DATA_PLACEHOLDER if uptime is not defined, which may not be the desired behavior in all contexts.
value: <NodeUptime StartTime={node?.StartTime} DisconnectTime={node?.DisconnectTime} />,
astandrik
astandrik previously approved these changes Dec 19, 2024
<CellWithPopover
placement={['top', 'auto']}
content={
<DefinitionList responsive>
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add Popover if nor StartTime neither DisconnectTime is provided?

Copy link
Member Author

@artemmufazalov artemmufazalov Dec 19, 2024

Choose a reason for hiding this comment

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

Uptime won't be calculated in such case (getUptimeFromDateFormatted return undefined for undefined input). I made the code more explicit

@artemmufazalov artemmufazalov added this pull request to the merge queue Dec 19, 2024
Merged via the queue into main with commit a77e6db Dec 19, 2024
7 checks passed
@artemmufazalov artemmufazalov deleted the 1200-add-timestamp-to-uptime branch December 19, 2024 10:30
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.

Add absolute time timestamp in addition to uptime
3 participants