Skip to content

feat: provide more information when shutting down sessions #3779

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 16 commits into from
Aug 12, 2025
Merged

feat: provide more information when shutting down sessions #3779

merged 16 commits into from
Aug 12, 2025

Conversation

lorenzo-cavazzi
Copy link
Member

@lorenzo-cavazzi lorenzo-cavazzi commented Jul 24, 2025 •
edited
Loading

This PR adds more context to the session shutdown modal to help prevent users from accidentally losing non-saved and non-synced files.

The content is customized based on the project's attached resources:

  • Project with Code and writable Data Connectors
    Screenshot_20250730_162520

  • Project with no storage attached
    Screenshot_20250730_162412

/deploy

fix https://www.notion.so/renku/Improve-Shut-Down-Modal-22d0df2efafc809aa88aff84697d397c?d=2370df2efafc80229d5d001cf13126c9#2370df2efafc8035a5cbd3e11f3e8203

@RenkuBot
Copy link
Contributor

You can access the deployment of this PR at https://renku-ci-ui-3779.dev.renku.ch

@lorenzo-cavazzi lorenzo-cavazzi force-pushed the lorenzo/update-shutdown-session-warnings branch from 307f277 to 5823752 Compare July 30, 2025 14:31
@lorenzo-cavazzi lorenzo-cavazzi force-pushed the lorenzo/update-shutdown-session-warnings branch from d1a15fd to 03a5d58 Compare August 7, 2025 09:07
@lorenzo-cavazzi lorenzo-cavazzi marked this pull request as ready for review August 7, 2025 09:42
@lorenzo-cavazzi lorenzo-cavazzi requested a review from a team as a code owner August 7, 2025 09:42
leafty
leafty reviewed Aug 7, 2025
View reviewed changes
client/src/utils/helpers/DurationUtils.ts
}: {
datetime: DateTime | Date | string;
now: DateTime;
Copy link
Member

Choose a reason for hiding this comment

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

Please revert this, now should be given as a DateTime, that is create it with luxon.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why can't it be just a Date? That's often going to be new Date(), we don't need luxon for that.

client/src/components/container/CollapseBody.tsx Outdated

interface CollapseBodyProps {
className?: string;
children: React.ReactNode;
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: prefer import { ReactNode } from 'react'; so that we do not rely on a shadow import.

client/src/features/sessionsV2/PauseOrDeleteSessionModal.tsx Outdated
import { Loader } from "../../components/Loader";
import { User } from "../../model/renkuModels.types";
import { NOTIFICATION_TOPICS } from "../../notifications/Notifications.constants";
import { NotificationsManager } from "../../notifications/notifications.types";
import { ABSOLUTE_ROUTES } from "../../routing/routes.constants";
import AppContext from "../../utils/context/appContext";
import useLegacySelector from "../../utils/customHooks/useLegacySelector.hook";
import styles from "../session/components/SessionModals.module.scss";
Copy link
Member

Choose a reason for hiding this comment

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

move to its own line below code imports

client/src/features/sessionsV2/PauseOrDeleteSessionModal.tsx Outdated
<ModalHeader toggle={toggleModal}>
{action === "pause" ? "Pause Session" : "Shut Down Session"}
<ModalHeader
className={cx(action === "delete" ? "text-danger" : "")}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
className={cx(action === "delete" ? "text-danger" : "")}
className={cx(action === "delete" && "text-danger")}

client/src/features/sessionsV2/PauseOrDeleteSessionModal.tsx
{hibernationThreshold} of inactivity.
</InfoAlert>
)} */}
{hibernateThreshold && (
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{hibernateThreshold && (
{hibernateThreshold != null && (

client/src/features/sessionsV2/PauseOrDeleteSessionModal.tsx Outdated
@@ -386,11 +397,12 @@ function DeleteSessionModalContent({
</ModalBody>
<ModalFooter>
<Button
color="outline-danger"
color="outline-primary"
Copy link
Member

Choose a reason for hiding this comment

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

This should be outline-danger when the dialog is "delete" (see designs).

client/src/features/sessionsV2/PauseOrDeleteSessionModal.tsx Outdated
Comment on lines 270 to 276
const now = new Date();
const hibernateThreshold = session?.status?.will_hibernate_at
? toHumanRelativeDuration({
datetime: session?.status?.will_hibernate_at,
now,
})
: 0;
Copy link
Member

Choose a reason for hiding this comment

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

Do not do this by hand, use the <TimeCaption> component, it will re-render as appropriate and provide a tooltip with the exact timestamp.

client/src/features/sessionsV2/components/SessionModals/ShoutdownSessionContent.tsx Outdated
Comment on lines 188 to 190
Check your workspace is clear: Ensure no important files
remain in
<code>/home/jovyan/work/</code>.
Copy link
Member

Choose a reason for hiding this comment

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

What does this mean? Do I need to remove all the files in there?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess that means any file there would be lost.
It makes sense to me, but feel free to ping the product team if you think the copy text should be different here

client/src/features/sessionsV2/components/SessionModals/ShoutdownSessionContent.tsx Outdated
)}
<li>
Check your workspace is clear: Ensure no important files
remain in
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
remain in
remain in{" "}

client/src/features/sessionsV2/components/SessionModals/ShoutdownSessionContent.tsx
Commit and push code changes: Run <code>git commit</code>{" "}
and <code>git push</code> in all repositories (
<span className="fst-italic">
{codeRepositories.join(", ")}
Copy link
Member

Choose a reason for hiding this comment

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

This lists repositories I cannot git push to, is it OK?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's acceptable here; even if we check users can push through the integrations, that might have changed and not be valid for the sessions for many reasons, ranging from permissions revoked on the platform to users who authenticated in the sessions or git push using a token.

@lorenzo-cavazzi
Copy link
Member Author

@leafty I've addressed most points except the copy text. There were quite a few iterations on that from the product team. Ofc feel free to ping them if the text should be different, but I prefer to have them involved.

@lorenzo-cavazzi lorenzo-cavazzi requested a review from leafty August 7, 2025 15:31
leafty
leafty previously approved these changes Aug 12, 2025
View reviewed changes
Copy link
Member

@leafty leafty left a comment

Choose a reason for hiding this comment

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

LGTM, nitpick below

client/src/features/sessionsV2/components/SessionModals/ShoutdownSessionContent.tsx Outdated
Check your workspace is clear: Ensure no important files
remain
{launcherMountDirectory && (
<span>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<span>
<>

@lorenzo-cavazzi lorenzo-cavazzi requested a review from leafty August 12, 2025 12:43
@lorenzo-cavazzi lorenzo-cavazzi enabled auto-merge (squash) August 12, 2025 13:29
@lorenzo-cavazzi lorenzo-cavazzi merged commit 9429a6a into main Aug 12, 2025
16 of 18 checks passed
@lorenzo-cavazzi lorenzo-cavazzi deleted the lorenzo/update-shutdown-session-warnings branch August 12, 2025 13:32
@RenkuBot
Copy link
Contributor

Tearing down the temporary RenkuLab deplyoment for this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leafty leafty leafty approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lorenzo-cavazzi @RenkuBot @leafty