feat(cc-session-tokens): init #1364

florian-sanders-cc · 2025-03-13T18:01:25Z

Fixes #1360

What does this PR do?

Adds a cc-session-tokens component

How to review?

Important

We don't have info about the user agent / IP address of the session so we can only show the dates,
The component doesn't handle expired tokens because the API is only supposed to provide alive sessions,
There can only be one current session and there should always be one. It's very difficult to reflect this with types:
- I do have something that gets really close to that with recursive types but it's difficult to read so I chose to stay simple,
- We could add something to throw an error if we don't have 1 (exactly 1) current session at runtime. (Usually it's not something we do but I've broken the smart a few times when refactoring 🤷).
The list of sessions can never be empty since there's always your current session,
There is no sandbox / demo-smart because only almighty consumers may list tokens.

review the commit,
- unit tests have been AI generated and reworked 👼,
- I'm not sure about the isExpirationClose helper.
  - The UX is validated, we need different thresholds depending on the token lifetime (the longer the lifetime is, the higher the threshold is).
  - Maybe I overcomplicated the code and namings are wrong, feel free to suggest rework or improvements 🙌.
check all the stories within the preview,
proceed with the QA in the usual google drive 😉

TODO Before merging

Remove MockApi from the smart,
Remove @ts-ignore eslint-disable comments,
Only order sessions by creation date, no specific treatment for current sessions.

github-actions · 2025-03-14T08:02:22Z

🔎 A preview has been automatically published : https://clever-components-preview.cellar-c2.services.clever-cloud.com/cc-session-tokens/init/index.html.

This preview will be deleted once this PR is closed.

Fixes #1360

florian-sanders-cc · 2025-03-14T13:30:33Z

src/lib/utils.js

+ */
+
+/** @type {ExpirationWarningThresholds} */
+const DEFAULT_THRESHOLDS = [


Maybe I should put this inside the isExpirationClose helper instead? 🤷
I placed it here because I wanted to use it as the function param default value but we can easily refactor to fallback to this if no thresholds are provided 👍

marion-izd · 2025-03-14T13:55:03Z

In terms of design, my only comment, which we forgot to mention during our synchro sessions, concerns the display order. To be able to harmonise with the 3 screens, I recommend displaying by creation date (last creation on top). But there's the question of the session screen with the ‘current session’ item: we expect to see it at the top even if it's not the most recent creation date. I'm continuing to think about this and checking some use cases 🤔

pdesoyres-cc

Well done @florian-sanders-cc . The design is very nice.

About component name

Maybe we could rename it with the list suffix like for emails and ssh-keys components.

About expiration warning threshold

I understand why you do that and I'm ok with that even if I find it a bit overkill.
I think that the utils.js file should remain for things very general. I propose to move the logic into a lib/tokens.js file, the types in a lib/tokens.types.d.ts file, and the tests in test/tokens.test.js file.
Anyway, this code should be moved to the client so that the clever-tools can benefit from it.

pdesoyres-cc · 2025-03-14T13:13:04Z

src/components/cc-session-tokens/cc-session-tokens.js

+        tabindex=${ifDefined(tabIndex)}
+        ${isCurrentSession ? ref(this._currentSessionCardRef) : ''}
+      >
+        ${isCurrentSession || isCleverTeam || isExpirationClose


suggestion: you could make this if inside the this._renderCardHeader method.

I'd rather keep very explicit render & subrender. Moving these inside the subrender would read as "there's always a card header" and you would have to go within the subrender to discover that in reality the header in only here in specific cases.

It's very subjective of course but I find the verbosity okay here and it feels easy to read / maintain as it is.

pdesoyres-cc · 2025-03-14T13:17:36Z

src/components/cc-session-tokens/cc-session-tokens.js

+
+  /** @param {CcSessionTokensPropertyValues} changedProperties */
+  willUpdate(changedProperties) {
+    if (changedProperties.has('state') && (this.state.type === 'loaded' || this.state.type === 'revoking-all')) {


nitpick: computing the tokens order and expiration here is a kind of optimization. But in that case, it is maybe overkill since there will never be a huge amount of tokens to handle. Maybe, you want to simplify the code by sorting at every render and get rid of the _sortedAndFormattedTokens private class field.

Yes you're right, I'll move that to a const within the render method 👍

pdesoyres-cc · 2025-03-14T13:25:54Z

src/components/cc-session-tokens/cc-session-tokens.types.d.ts

+};
+
+// FIXME: remove when clever-client exposes types
+export type RawSessionTokenData = {


nitpick: this could be an interface

pdesoyres-cc · 2025-03-14T13:29:18Z

src/components/cc-session-tokens/cc-session-tokens.types.d.ts

+  creationDate: number | string; // timestamp as number or string with ISO format with timezone information
+  expirationDate: number | string; // timestamp as number or string with ISO format with timezone information
+  lastUsedDate: number | string; // timestamp as number or string with ISO format with timezone information


question: is it really necessary to provide two ways of specifying dates? I mean, the smart could do the job of providing that in the right format. I would have permit only ISO string since this is what the API aims at doing for every dates (even if not fully the case right now).

You have a point, I don't know to be honest 🤷

I'm okay with only accepting Date and make the smart do the work 👍 (that's what you are suggesting right?)

Yes, this is what I mean

pdesoyres-cc · 2025-03-14T13:36:31Z

src/components/cc-session-tokens/cc-session-tokens.types.d.ts

+export interface SessionTokensStateLoaded {
+  type: 'loaded';
+  tokens: Array<SessionTokenState>;
+}
+
+export interface SessionTokensStateRevokingAllTokens {
+  type: 'revoking-all';
+  tokens: Array<SessionTokenStateRevoking | SessionTokenStateCurrent>;
+}


suggestion: We could ensure that there is always one and only one current session token with this design:

export interface SessionTokensStateLoaded { type: 'loaded'; currentToken: SessionToken; otherTokens: Array<SessionTokenStateIdle|SessionTokenStateRevoking>; } export interface SessionTokensStateRevokingAllTokens { type: 'revoking-all'; currentToken: SessionToken; otherTokens: Array<SessionToken>; }

No state is necessary for the currentToken as it is always in idle state.

When revoking all, no state is necessary on otherTokens as they are all considered in revoking state.

I really like this idea, I'll wait for other opinions but I think I'll go with that, thanks!

I like @pdesoyres-cc idea 👍

I wanted to propose tokens and currentTokenId. This makes the smart easier as you can just pass the token list as is and just set the current token ID you already have from the apiConfig. Donwside is you have to find the current token in the list and in theory it the find could fail (from the UI component perspective). Pierre's solution does not have this problem, it seems better 👍 💪

pdesoyres-cc · 2025-03-14T14:01:30Z

src/components/cc-session-tokens/cc-session-tokens.smart.js

+    function updateOneToken(tokenId, callback) {
+      updateComponent('state', (state) => {
+        if (!('tokens' in state)) {
+          return;
+        }
+
+        const sessionTokenToUpdate = state.tokens.find((token) => token.id === tokenId);
+
+        if (sessionTokenToUpdate != null) {
+          callback(sessionTokenToUpdate);
+        }
+      });
+    }


nitpick: you could rely on state.type instead of the presence of the tokens property, this is more the philosophy of our algebraic types. And anyway, when this function is called, the sate is always in loaded state, so you could just type the function param :

Suggested change

function updateOneToken(tokenId, callback) {

updateComponent('state', (state) => {

if (!('tokens' in state)) {

return;

}

const sessionTokenToUpdate = state.tokens.find((token) => token.id === tokenId);

if (sessionTokenToUpdate != null) {

callback(sessionTokenToUpdate);

}

});

}

function updateOneToken(tokenId, callback) {

updateComponent('state',

/** @param {SessionTokensStateLoaded} state */

(state) => {

const sessionTokenToUpdate = state.tokens.find((token) => token.id === tokenId);

if (sessionTokenToUpdate != null) {

callback(sessionTokenToUpdate);

}

});

}

Yes, dunno why I went this way to be honest 👍

pdesoyres-cc · 2025-03-14T14:04:26Z

src/translations/translations.en.js

+  'cc-session-tokens.card.label.creation': `Creation: `,
+  'cc-session-tokens.card.label.expiration': `Expiration: `,
+  'cc-session-tokens.card.label.last-used': `Last used: `,


I worry about this trailing space, I wonder if once integrated inside the console it will be dropped by the build process.

They might be, but I use flex + gap anyway.
These are mostly here for screen reader users with braille output, it's better if they do retrieve the spaces (but if they don't, it's not a big deal).

ah ok, this is good to know

pdesoyres-cc · 2025-03-14T14:12:04Z

src/lib/utils.js

+export function isExpirationClose({ creationDate, expirationDate }, thresholds = DEFAULT_THRESHOLDS) {
+  const formattedExpirationDate = new Date(expirationDate);
+  const now = new Date();
+  const MILLISECONDS_PER_DAY = 1000 * 60 * 60 * 24;


fix: can be move outside the function

src/translations/translations.fr.js

pdesoyres-cc · 2025-03-14T14:16:07Z

src/lib/utils.js

+ * @param {Date|string|number} tokenInfo.creationDate - The creation date of the token
+ * @param {Date|string|number} tokenInfo.expirationDate - The expiration date of the token


fix: I'm not sure we have to handle multiple format. Date is enough.

Very good point, I'll change to Date only 👍

florian-sanders-cc · 2025-03-17T08:34:05Z

@pdesoyres-cc thanks for your review! 😍

About component name

Maybe we could rename it with the list suffix like for emails and ssh-keys components.

Great idea, it would also remove the confusion between OauthTokensState & OauthTokenState 👍

I understand why you do that and I'm ok with that even if I find it a bit overkill.
I think that the utils.js file should remain for things very general. I propose to move the logic into a lib/tokens.js file, the types in a > lib/tokens.types.d.ts file, and the tests in test/tokens.test.js file.
Anyway, this code should be moved to the client so that the clever-tools can benefit from it.

Very good points, I agree with you. I'll move to a separate file within the component project first but moving to the client seems like a good idea?

florian-sanders-cc

Following @marion-izd research & feedback, let's go with date creation ordering and no special treatment for the current session (TODO @florian-sanders-cc)

roberttran-cc

Nice work! Looking forward seeing it in the console.

I only have minor feedback.

roberttran-cc · 2025-03-18T11:02:38Z

src/components/cc-session-tokens/cc-session-tokens.js

+import { ifDefined } from 'lit/directives/if-defined.js';
+import { createRef, ref } from 'lit/directives/ref.js';


🤓 nitpick: import order

roberttran-cc · 2025-03-18T11:43:29Z

src/components/common.types.d.ts

@@ -273,6 +273,13 @@ export interface NotificationOptions {
  closeable?: boolean;
 }

+export type ExpirationWarningThresholds = Array<{
+  /** Number of days representing the maximum lifetime for which the given threshold is applicable */
+  maxApplicableTokenLifetimeInDays: number;


🎯 suggestion: maybe remove the mention to token, to make the type a little more generic & less coupled to this use case.

Suggested change

maxApplicableTokenLifetimeInDays: number;

maxApplicableLifetimeInDays: number;

roberttran-cc · 2025-03-18T13:26:14Z

src/components/cc-session-tokens/cc-session-tokens.js

+    const hasTokens =
+      (this.state.type === 'loaded' || this.state.type === 'revoking-all') && this.state.tokens.length > 1;
+
+    if (this.state.type === 'error') {
+      return html`<cc-notice intent="warning" message="${i18n('cc-session-tokens.error')}"></cc-notice>`;
+    }


🤓 nitpick: invert code blocks

roberttran-cc · 2025-03-18T14:27:00Z

src/components/cc-session-tokens/cc-session-tokens.stories.js

+  ],
+});
+
+export const dataLoadedWithCleverTeamAsCurrentSession = makeStory(conf, {


🎯 suggestion: show cases where:

isExpirationClose is true and isCleverTeam: true

isExpirationClose is true and isCleverTeam: true and type: 'current'

roberttran-cc · 2025-03-18T14:32:03Z

src/components/cc-session-tokens/cc-session-tokens.smart.js

+ * @typedef {import('./cc-session-tokens.types.js').SessionTokensStateRevokingAllTokens} SessionTokensStateRevokingAllTokens
+ * @typedef {import('./cc-session-tokens.types.js').SessionTokenStateRevoking} SessionTokensStateRevoking


💣 chore: unused types

roberttran-cc · 2025-03-18T14:48:20Z

src/components/cc-session-tokens/cc-session-tokens.smart.js

+   * @param {SessionTokenState[]} allTokens - An array of all session tokens
+   * @returns {Promise<{ remainingTokens: SessionTokenState[], revokedTokens: string[], errors: any[] }>} A promise that resolves when all tokens are revoked
+   */
+  revokeAllSessionTokens(allTokens) {


🎯 suggestion:

Suggested change

revokeAllSessionTokens(allTokens) {

async revokeAllSessionTokens(allTokens) {

Galimede

Hey, nice work Florian! 💪

Nothing much to say apart from what my colleagues already said.

Also, I noticed that the spacing between the top of the block and the title is not the same between the loading and loaded state.

Galimede · 2025-03-21T10:24:22Z

src/components/cc-session-tokens/cc-session-tokens.js

+
+  /** @param {CcSessionTokensPropertyValues} changedProperties */
+  willUpdate(changedProperties) {
+    if (changedProperties.has('state') && (this.state.type === 'loaded' || this.state.type === 'revoking-all')) {


question: if you're in revoking-all, why would you need to do all of the below?

Galimede · 2025-03-21T10:31:35Z

src/components/cc-session-tokens/cc-session-tokens.js

+        row-gap: 0.5em;
+      }
+
+      @supports (grid-template-columns: subgrid) {


praise: that's cool to be able to do this

florian-sanders-cc force-pushed the cc-session-tokens/init branch 6 times, most recently from 4644f1c to fc38ae6 Compare March 14, 2025 07:56

florian-sanders-cc self-assigned this Mar 14, 2025

florian-sanders-cc requested review from hsablonniere, Galimede, roberttran-cc, pdesoyres-cc, HeleneAmouzou and marion-izd March 14, 2025 10:13

florian-sanders-cc marked this pull request as ready for review March 14, 2025 10:13

florian-sanders-cc force-pushed the cc-session-tokens/init branch from fc38ae6 to 2cad92a Compare March 14, 2025 10:36

feat(cc-session-tokens): init

431c3e0

Fixes #1360

florian-sanders-cc force-pushed the cc-session-tokens/init branch from 2cad92a to 431c3e0 Compare March 14, 2025 10:40

florian-sanders-cc commented Mar 14, 2025

View reviewed changes

pdesoyres-cc requested changes Mar 14, 2025

View reviewed changes

florian-sanders-cc commented Mar 17, 2025

View reviewed changes

roberttran-cc reviewed Mar 18, 2025

View reviewed changes

Galimede reviewed Mar 21, 2025

View reviewed changes

		* @param {Date\|string\|number} tokenInfo.creationDate - The creation date of the token
		* @param {Date\|string\|number} tokenInfo.expirationDate - The expiration date of the token

		import { ifDefined } from 'lit/directives/if-defined.js';
		import { createRef, ref } from 'lit/directives/ref.js';

	maxApplicableTokenLifetimeInDays: number;
	maxApplicableLifetimeInDays: number;

		* @typedef {import('./cc-session-tokens.types.js').SessionTokensStateRevokingAllTokens} SessionTokensStateRevokingAllTokens
		* @typedef {import('./cc-session-tokens.types.js').SessionTokenStateRevoking} SessionTokensStateRevoking

	revokeAllSessionTokens(allTokens) {
	async revokeAllSessionTokens(allTokens) {

feat(cc-session-tokens): init #1364

Are you sure you want to change the base?

feat(cc-session-tokens): init #1364

Conversation

florian-sanders-cc commented Mar 13, 2025 • edited Loading

What does this PR do?

How to review?

TODO Before merging

github-actions bot commented Mar 14, 2025

florian-sanders-cc Mar 14, 2025 • edited Loading

Choose a reason for hiding this comment

marion-izd commented Mar 14, 2025

pdesoyres-cc left a comment

Choose a reason for hiding this comment

About component name

About expiration warning threshold

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

florian-sanders-cc commented Mar 17, 2025

florian-sanders-cc left a comment

Choose a reason for hiding this comment

roberttran-cc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Galimede left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

florian-sanders-cc commented Mar 13, 2025 •

edited

Loading

florian-sanders-cc Mar 14, 2025 •

edited

Loading