Skip to content

Conversation

@Aerilym
Copy link
Collaborator

@Aerilym Aerilym commented Nov 19, 2025

No description provided.

@Aerilym Aerilym requested review from Bilb and Copilot and removed request for Copilot November 19, 2025 05:35
Copilot finished reviewing on behalf of Aerilym November 19, 2025 05:36
Copilot AI review requested due to automatic review settings November 19, 2025 05:48
Copilot finished reviewing on behalf of Aerilym November 19, 2025 05:50
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces functionality to retrieve database creation timestamps for triggering Call-to-Action (CTA) dialogs, and refactors the Pro Info Modal system into a more generalized CTA system that supports both Pro-related and donation CTAs.

  • Added a new utility function getFileCreationTimestampMs to safely retrieve file creation timestamps with proper error handling
  • Refactored SessionProInfoModal into SessionCTA to support multiple CTA variants beyond Pro features
  • Introduced donation CTA functionality that triggers 7 days after database creation

Reviewed Changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
ts/node/fs_utility.ts New utility function to get file creation timestamps with validation and error handling
ts/node/sql.ts Adds getDBCreationTimestampMs function and exports it
ts/data/dataInit.ts Registers the new getDBCreationTimestampMs channel
ts/data/data.ts Exposes getDBCreationTimestampMs through the Data layer
ts/util/logger/rotatingPinoDest.ts Updates log rotation to use the new utility function with null safety
ts/components/dialog/cta/types.ts Defines new CTAVariant enum with Pro and donation variants
ts/components/dialog/SessionCTA.tsx Refactored and generalized modal supporting both Pro and donation CTAs
ts/components/dialog/ProCTATitle.tsx Extracted Pro-specific title logic into separate component
ts/components/dialog/ProCTADescription.tsx Extracted Pro-specific description logic into separate component
ts/components/dialog/CTADescriptionList.tsx Reusable component for CTA feature lists
ts/components/leftpane/ActionsPanel.tsx Triggers donation CTA after 7 days of database existence
ts/state/ducks/modalDialog.tsx Renamed action from updateSessionProInfoModal to updateSessionCTA
ts/state/onboarding/ducks/modals.ts Updates type references from SessionProInfoState to SessionCTAState
ts/state/ducks/conversations.ts Updates function call to handleTriggeredProCTAs
ts/components/dialog/DefaultSettingsPage.tsx Updates donation URL to use getsession.org
ts/components/basic/SessionButton.tsx Refactors hover color logic for solid buttons
Multiple component files Updates imports and references from Pro-specific to generalized CTA system

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +319 to +320
// FIXME: replace with localised string
return 'Skip';
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

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

The FIXME comment indicates this button text needs localization. This should be addressed before merging or tracked in a follow-up issue to ensure proper internationalization support.

Suggested change
// FIXME: replace with localised string
return 'Skip';
// Use localized string for "Skip"
return tr('skip');

Copilot uses AI. Check for mistakes.
Comment on lines 130 to +135
color: ${props =>
props.isDarkTheme && props.color && props.color !== SessionButtonColor.Tertiary
? `var(--${props.color}-color)`
: `var(--button-solid-text-hover-color)`};
border: 1px solid
${props =>
props.isDarkTheme && props.color
props.isDarkTheme
? props.color && props.color !== SessionButtonColor.Tertiary
? `var(--${props.color}-color)`
: `var(--button-solid-text-hover-color)`};
: 'var(--primary-color)'
: `var(--button-solid-text-hover-color)`};
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

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

The nested ternary operator logic is difficult to read and understand. Consider refactoring to use a more explicit conditional structure:

color: ${props => {
  if (props.isDarkTheme) {
    return props.color && props.color !== SessionButtonColor.Tertiary
      ? `var(--${props.color}-color)`
      : 'var(--primary-color)';
  }
  return 'var(--button-solid-text-hover-color)';
}};

This makes the logic clearer and easier to maintain.

Copilot uses AI. Check for mistakes.
Comment on lines +169 to +170
// FIXME: replace with localised string
return 'Session Needs Your Help';
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

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

The FIXME comment indicates this string needs localization. This should be addressed before merging or tracked in a follow-up issue to ensure proper internationalization support.

Suggested change
// FIXME: replace with localised string
return 'Session Needs Your Help';
// Localized string for donate generic CTA
return tr(MergedLocalizerTokens.cta_donate_generic_title);

Copilot uses AI. Check for mistakes.
Comment on lines +191 to +198
// FIXME: replace with localised string
return (
<>
{`Session is fighting powerful forces trying to weaken privacy, but we can’t continue this fight alone.`}
<br />
<br />
{`Donating keeps Session secure, independent, and online.`}
</>
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

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

The FIXME comment indicates this description needs localization. This should be addressed before merging or tracked in a follow-up issue to ensure proper internationalization support.

Copilot uses AI. Check for mistakes.
Comment on lines +265 to +266
{/** FIXME: replace with localised string */}
Donate
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

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

The FIXME comment indicates this button text needs localization. This should be addressed before merging or tracked in a follow-up issue to ensure proper internationalization support.

Suggested change
{/** FIXME: replace with localised string */}
Donate
{/** FIXME removed: now using localised string */}
{tr('donate')}

Copilot uses AI. Check for mistakes.
@Aerilym Aerilym changed the title feat: get database creation time for use in cta triggers feat: donate cta Nov 19, 2025
Comment on lines 152 to 154
// TODO: add checks around if the user has interacted with the donation url dialog before
const interacted = false;
if (!interacted) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

you have everything you need to do this now, correct?

Comment on lines 157 to 158
case CTAVariant.NIL:
return null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

what does this corresponds to?

const rawInteractions = Storage.get(SettingsKey.urlInteractions);
const result = UrlInteractionsSchema.safeParse(rawInteractions);
if (result.error) {
window?.log?.error(`faild to parse ${SettingsKey.urlInteractions}`, result.error);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
window?.log?.error(`faild to parse ${SettingsKey.urlInteractions}`, result.error);
window?.log?.error(`failed to parse ${SettingsKey.urlInteractions}`, result.error);

Comment on lines 90 to 101
export function urlInteractionToString(interaction: URLInteraction) {
switch (interaction) {
case URLInteraction.OPEN:
return 'Open';
case URLInteraction.COPY:
return 'Copy';
case URLInteraction.TRUST:
return 'Trust';
default:
return 'Unknown';
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is debug only, but should it be using the corresponding localised strings ) if we have them?

type VariantForNonGroupFeature = (typeof variantsForNonGroupFeatures)[number];

export function isProCTAFeatureVariant(variant: CTAVariant): variant is VariantForNonGroupFeature {
return variantsForNonGroupFeatures.includes(variant as any);
Copy link
Collaborator

Choose a reason for hiding this comment

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

could be as number I reckon

width: 100%;
height: 100%;
object-fit: cover;
object-position: right center;
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this need to be dependent on rtl?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The whole modal has rtl now so it supports rtl 🥳

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.

3 participants