Feature: add cohort retention chart, convert js to ts, add types, small fixes, move function comments to jsdocs #36

FranjoMindek · 2025-03-20T14:34:03Z

Adds cohort retention chart

Converted js files to ts files
Added missing types because of above
Fixed some bugs that existed because of above
Converted function comments to JSDoc via LLM

Closes: #33

…sdoc

FranjoMindek · 2025-03-20T14:38:55Z

src/analytics/reports/colors.ts

@@ -0,0 +1,115 @@
+/**
+ * Interpolates between multiple colors, returning an array of evenly spaced colors
+ * @param {string[]} colors - Array of hex color codes to interpolate between


Autogenerated by LLM, I've checked logic and it seems fine.

FranjoMindek · 2025-03-20T14:40:02Z

src/utils/logger.js

-import * as logger from "winston";
-
-logger.remove(logger.transports.Console);
-logger.add(new logger.transports.Console(), { colorize: true });


{ colorize: true } threw an error because add function accepts only 1 param

src/analytics/types.ts

FranjoMindek · 2025-03-20T14:42:24Z

src/analytics/reports/periodReport/period.ts

@@ -0,0 +1,123 @@
+import moment from "../../moment";


This file did everything related to periods, so I've renamed it from common

Martinsos

@FranjoMindek nice to see this coming to life!

The biggest issue with this PR is its size and scope of changes. Except for adding this new chart, you also added types, reformatted comments and renamed files that are not directly related to this change.

I started reviewing, but soon realized it will take a lot of time to just figure out what really changed and what didn't.

I suggest that you break this PR into two or more PRs, where each one is focused on one "theme". E.g. the first one might be general refactor where you introduce the types and rename files and similar. In taht one for example you can, if you know that most of the code didn't relaly change, comment in Github only on the parts that did change, so I can review it much faster. The other PR can build on top of this one and implement the actual chart.

WHat is also tricky is when you do changes + also rename the file, it seems Github didn't have great time here doing diff so it just marked old files as deleted and new files as completely new -> again, that makes it quite hard to review. I would probably actually make the first PR that just does the renaming, nothing else or at least minimal changes, and only then the second PR that makes further refactors -> that way diffs will be reasonable and it will be much easier to review.

You can make these PRs rebased one on top of another.

The comments I made so far you can then address in the relevant PR once you create it.

Martinsos · 2025-03-20T18:10:27Z

.gitignore

@@ -1,5 +1,5 @@
 node_modules/
-.env
+.env*


I would call this too aggresive. It will also trigger on stuff like .envision for example (made that up) and that was not your intention.

This was introduced by vault dotenv. Didn't see it as problematic but you're right.

Martinsos · 2025-03-20T18:14:27Z

src/analytics/charts.ts

+  return chart.toURL();
+}
+
+export function buildChartBase(data: ChartData, title: string) {


Doesn't feel like the best name. What is "chart base"? How would you explain that term? That is not a common term, so I can't really know what to expect from it, and also there are no comments on top of function that would define what function promises to return. So the only way for me is to check the implementation and try to guess the intention from it. On the other hand, somebody can come in the future and modify what this function does following their own interpretation of that intention, which is different, therefore changing the expected behaviour and causing issues in the places where it is used.
TLDR: better name and/or function header comments explaining what it does in enough details that consuemr of this function can somewhat rely on it.

Martinsos · 2025-03-20T18:15:12Z

src/analytics/charts.ts

+  title: string,
+) {
+  const chart = buildChartBase(data, title)
+    .cht("bvs") // Type: lines or vertical bars? Could also be other things.


Old comment, not correct here any more (there are no lines here any more, only vertical bars).

Martinsos · 2025-03-20T18:16:57Z

src/analytics/charts.ts

@@ -0,0 +1,50 @@
+import ImageCharts from "image-charts";
+import { interpolateColors } from "./reports/colors";
+import { ChartData } from "./types";


I am not a fan of separting types into separate files, never done it in wahtever language I work in. Sometimes, rarely, there can be a good reason for it, but that is rare. Types are a part of domain same as the rest of the code, and shouldn't be split unneccesarily.

I can reorder it to be more domain oriented.

Martinsos · 2025-03-20T18:18:58Z

src/analytics/charts.ts

+) {
+  const chart = buildChartBase(data, title)
+    .cht("bvs") // Type: lines or vertical bars? Could also be other things.
+    .chco(


functions of IMage Charts are quite badly names so it is hard to figure what th ey do. That is why I was providing so many comments, so reader can undersatnd what is happeing without having to go consult the ImageChart docs immedaitely.
Would be great if you provided similar comments here, because right now I don't know what this does wihtout consulting the docs.

Yeah, forgot to change this one. It's just type. But it was done messily previously.

Martinsos · 2025-03-20T18:20:30Z

src/analytics/charts.ts

+  return new ImageCharts()
+    .chtt(title)
+    .chd(
+      "a:" +
+        Object.values(data.series)
+          .map((s) => s.join(","))
+          .join("|"),
+    ) // Data series.
+    .chl(
+      Object.values(data.series)
+        .map((s) => s.map((v) => v ?? undefined).join("|"))
+        .join("|"),
+    ) // Value labels on bars.
+    .chlps("font.size,12")
+    .chdl(Object.keys(data.series).join("|")) // Legend (per series)
+    .chdlp("b") // Position of legend. 'b' is for bottom.
+    .chxl("0:|" + data.periodEnds.join("|")) // X axis labels.
+    .chxs("0,s,min45max45") // On x-axis (0), skip some labels (s) and use 45 degress angle (min45max45).
+    .chs("700x400") // Size.
+    .chg("20,20") // Solid or dotted grid lines.
+    .chma("0,50,50") // Margins.
+    .chxt("x,y"); // Axes to show.
+}


Since file changed the name, we get this not at all useful diff that shows everyting as one big change. Can you confirm that there was no changes in this block of code so I don't have to go through it line by line and compare? Or if there were changes, can you point me to them?

I mostly pointed out logic changes with my comments.
Only changes was that .cht("bvs") now explictly says it's type for analytics chart, rather than using ternary operator which was always false. Rest is the same.

Martinsos · 2025-03-20T18:22:05Z

src/analytics/cli.ts

@@ -50,12 +59,12 @@ function showReportInCLI(report) {
      }
    }
    if (metric.chart) {
-      console.log("- Chart: ", metric.chart.toURL());
+      console.log("- Chart: ", metric.chart);


Why is toURL not needed any more?

The function was named build...ImageUrl, but it was returning an chart object, rather than imageUrl, which is misleading.
Everywhere where the chart object was used it was just converted to imageUrl though.
I just made the function return imageUrl as it's name states.

Then probably we should also rename that field to metric.chartImageUrl!

Feature: convert js to ts, add types, small fixes, move comments to j…

7f1ed70

…sdoc

FranjoMindek self-assigned this Mar 20, 2025

FranjoMindek commented Mar 20, 2025

View reviewed changes

src/analytics/types.ts Outdated Show resolved Hide resolved

FranjoMindek commented Mar 20, 2025

View reviewed changes

Chore: renames, jsdoc

ca81640

FranjoMindek changed the title ~~Feature: add cohort retention chart, convert js to ts, add types, small fixes, move comments to to jsdocs~~ Feature: add cohort retention chart, convert js to ts, add types, small fixes, move function comments to jsdocs Mar 20, 2025

Martinsos requested changes Mar 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: add cohort retention chart, convert js to ts, add types, small fixes, move function comments to jsdocs #36

Feature: add cohort retention chart, convert js to ts, add types, small fixes, move function comments to jsdocs #36

FranjoMindek commented Mar 20, 2025 •

edited

Loading

FranjoMindek Mar 20, 2025

FranjoMindek Mar 20, 2025 •

edited

Loading

FranjoMindek Mar 20, 2025 •

edited

Loading

Martinsos left a comment •

edited

Loading

Martinsos Mar 20, 2025

FranjoMindek Mar 20, 2025

Martinsos Mar 20, 2025

Martinsos Mar 20, 2025

Martinsos Mar 20, 2025

FranjoMindek Mar 20, 2025

Martinsos Mar 20, 2025

FranjoMindek Mar 20, 2025

Martinsos Mar 20, 2025

FranjoMindek Mar 20, 2025

Martinsos Mar 20, 2025

FranjoMindek Mar 20, 2025

Martinsos Mar 20, 2025

Feature: add cohort retention chart, convert js to ts, add types, small fixes, move function comments to jsdocs #36

Are you sure you want to change the base?

Feature: add cohort retention chart, convert js to ts, add types, small fixes, move function comments to jsdocs #36

Conversation

FranjoMindek commented Mar 20, 2025 • edited Loading

Choose a reason for hiding this comment

FranjoMindek Mar 20, 2025 • edited Loading

Choose a reason for hiding this comment

FranjoMindek Mar 20, 2025 • edited Loading

Choose a reason for hiding this comment

Martinsos left a comment • edited Loading

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

FranjoMindek commented Mar 20, 2025 •

edited

Loading

FranjoMindek Mar 20, 2025 •

edited

Loading

FranjoMindek Mar 20, 2025 •

edited

Loading

Martinsos left a comment •

edited

Loading