Initial Code Review #1

ewanek1 · 2025-07-21T22:24:05Z

Hello! PR for code review 🙂. Thank you!

WilliamBergamin

Overall this is great work 💯 its an awesome example of how to use the features of Bolt 🚀

Some small things to consider:

When opening PRs, its helpful for reviewers to include things like a testing steps, Demo video/images, a short description of what the changes are etc. For this PR I had a decent amount of context on the app and could follow the README, but it isn't always the case

Including a .gitignore file in your project is useful in order to filter out files that shouldn't be checked into the repositories, for example the node_modules folder or the .env file that contains the values for the environment variables.

Linters and formatters are great tools to facilitate collaboration on projects, they force every maintainer to adhere to a set of rules. In our SDKs we use biome

You've written some great unit tests, leveraging tools like c8 to figure out exactly what sections of your code are getting covered is extremely useful. Trying to reach 100% code coverage is overkill, but these types of tools help highlight "critical" sections that may have been missed

I don't think this is needed for your project but using some sort of continuous integration pipeline to run the linters, unit tests and code coverage on every change introduced in a project can greatly improve the reviewers/maintainers confidence in the change. It helps to ensure that new changes do not create unexpected behaviors. In out SDKs we use github actions to do this.

WilliamBergamin · 2025-07-24T15:03:41Z

app.js

+  signingSecret: process.env.SLACK_SIGNING_SECRET,
+  socketMode: true,
+  appToken: process.env.SLACK_APP_TOKEN,
+  //logLevel: LogLevel. 


Could this be some code that was intended to be deleted?

Ah yes! Thanks for catching that 🫡

WilliamBergamin · 2025-07-24T15:10:02Z

utils/timeZoneMap.js

@@ -0,0 +1,71 @@
+const tzMap = {


This makes sense but I would like to know why you opted to place this in timeZoneMap.js rather then constants.js?

Hm I believe since the map was a more crucial part of my code I just separated it out in its own file.

WilliamBergamin · 2025-07-24T15:15:03Z

commands/convertTimeLogic.js

+/* The `convertTimeCommandText` function parses 
+ * and converts a time between time zones 
+ */
+function convertTimeCommandText(text) {


What do you think about adding proper JSdocs here to add information on what text is expected to be and what the outputs of the function is?

As an outsider it can sometimes be hard to determine this just from the code

WilliamBergamin · 2025-07-24T15:16:53Z

commands/convertTimeLogic.js

+    return {
+      error: `Sorry, I don't recognize the time zone abbreviation ${fromZoneAbbr}`,
+    };
+  } else if (!toZone) {


Because the if statement returns early I don't think we need the else keyword, I think we could get away with just if blocks 🤔 what dod you think?

Yes I agree! I suppose another technique would be to store the error messages inside an array especially if I want to notify a user if both their to and from zones are invalid.

WilliamBergamin · 2025-07-24T15:20:21Z

commands/teamClock.js

+      output_message = "Invalid input. Please use `/teamclock` or `/teamclock @usergroup`";
+    }
+  } catch (error) {
+    console.error("Error fetching user list:", error);


This might be overkill for the project but this function could accept some sort of context input parameter that contains a logger, this way your entire project could use the Bolt logger 💡

Edit: Heh I realize now I'm making similar comments elsewhere - using the context value is a very interesting idea though! 🧠 ✨

WilliamBergamin · 2025-07-24T15:21:18Z

tests/convertLogic.test.js

@@ -0,0 +1,105 @@
+const { convertTimeCommandText } = require("../commands/convertTimeLogic.js");


WilliamBergamin · 2025-07-24T15:25:03Z

workflows/conversionWorkflows.js

+/* Formats a moment object into a time string and its timezone abbreviation
+ * Example: 3pm EST
+ */
+function formatTimeWithAbbr(momentObject) {


WilliamBergamin · 2025-07-24T16:16:59Z

app.js

+// Initializing Bolt app with Socket Mode
+const app = new App({
+  token: process.env.SLACK_BOT_TOKEN,
+  signingSecret: process.env.SLACK_SIGNING_SECRET,


💭 Socket mode apps do not need the signing secret it is used to validate HTTP requests coming from Slack

Ohh thanks for that!

WilliamBergamin · 2025-07-24T16:20:51Z

README.md

+  Example: `/convert 3pm est pst`  
+  Converts a time between two time zones. Inputs are not case sensitive.
+
+- **`/time [time] @user`**  


Should this be /timefor instead of /time?

Does this command only accept one parameter?

Yes thanks for catching this. I'll fix it. I initially wrote this as /time @user. But I thought about it some more and figured a person would use their current time.

WilliamBergamin · 2025-07-24T16:24:10Z

commands/timeFor.js

+  if (parts.length !== 1) {
+    return {
+      error:
+        "Invalid input format. Please use the `/time_for @user`\nExample: `/time_for @username`",


I think there might be a typo in the error message 🤔 how do you think we could improve this so that we maintain consistency across the project?

Got it - timefor not time_for.

zimeg

@ewanek1 This is an impressive setup all around!

I left a handful of comments building off of what @WilliamBergamin shared, but I do want to callout a few thoughts I was having:

Reducing dependencies: I'm not sure if the native date handling of javascript can substitute the "moment" package, but for ongoing maintenance that might be optimal if so! Also, we might want to use @slack/bolt as the interface for all Slack interactions in this app.
Separating handlers: Listeners and logic and handlers alike are found in similar places which is nice for fast context but can cause confusion in later enhancements. Some ideas on changes to this were left in comment!
Error handling: Generous use of try/catch would move this app towards production readiness! Logging error cases might follow that, but I'm hoping most to avoid unexpected crashes.

Overall though, this is very solid development with a great grasp on the event driven setups of bolt! 🤖

zimeg · 2025-07-24T19:19:30Z

commands/convertTime.js

+  const { error, result } = convertTimeCommandText(command.text);
+
+  if (error) {
+    await respond({ text: error });
+    return;
+  }
+
+  await respond({ text: result });


📝 I'm a big fan of treating errors as values, but this might miss an unexpected throw from calling code, or even from a failing ack above!

Some examples in documentation might not show this at the moment, but a nice pattern to use can be found in this sample:

https://github.com/slack-samples/bolt-js-starter-template/blob/main/listeners/commands/sample-command.js

Changing the returning error to a throw might take additional logic to parse and be excessive, but I do think a try/catch at the listener level is most useful to avoid a crashed program 🤓

zimeg · 2025-07-24T19:32:49Z

commands/teamClockLogic.js

+ * Expects input: '/teamclock @usergroup' for usergroup
+ */
+
+const moment = require("moment-timezone");


⏳ The time happenings included can no doubt use functions from elsewhere and I'm wondering if the Intl.DateTimeFormat class was considered instead of moment-timezone?

AFAICT it might be a suitable replacement, albeit with not as concise syntax... But I do tend to prefer fewer dependencies when possible!

zimeg · 2025-07-24T19:40:56Z

commands/teamClockLogic.js

+  { start: "17:00", end: "20:00", message: "After hours 🌙" },
+  { start: "20:00", end: "24:00", message: "After hours 🌙" },
+  { start: "00:00", end: "09:00", message: "Sleeping 😴" },


🤔 Some of these time ranges have overlap at the end time which was confusing to me at a glance.

I'm somewhat curious if an ordered status list could be used instead to avoid some of the special case boundary checking below?

const timeRanges = [ { start: "00:00", message: "Sleeping 😴" }, { start: "09:00", message: "Available ✅" }, { start: "16:00", message: "Getting late 🕓" }, { start: "17:00", message: "After hours 🌙" }, ];

A following check can iterate in reverse here (or somehow else) to find the closest start time here for the corresponding message, though I admit that's makes the ordering important.

zimeg · 2025-07-24T19:44:30Z

commands/teamClockLogic.js

+      const status = teamClockCommandText(curTime);
+
+      // Format output message line 
+      messages.push(`${user.real_name}: ${curTimeFormatted}  ⇔  ${status}`);


👁️‍🗨️ IIRC the real_name value isn't guaranteed here - formatting this to prefer the <@${user.id}> might be most safe for all outputs!

Thank you 😊

zimeg · 2025-07-24T19:46:31Z

commands/timeFor.js

@@ -0,0 +1,76 @@
+const { WebClient } = require("@slack/web-api");
+const { convertTimeBetweenUsers } = require("./timeForLogic.js"); 
+const client = new WebClient(process.env.SLACK_BOT_TOKEN);


Suggested change

const client = new WebClient(process.env.SLACK_BOT_TOKEN);

🪓 Passing the app.client to these functions might be a best practice to ensure the same client "options" are kept throughout the app.

I don't believe @slack/bolt changes these much, but if retry logic was changed during app initialization we'd want to make sure it matches here too!

zimeg · 2025-07-24T19:58:07Z

commands/teamClock.js

👁️‍🗨️ I like that logic for each command is separated, but the separation between teamClock and teamClockLogic isn't so clear to me and I might favor a single file for all of this?

The listeners pattern in samples can be useful too once projects gain enough functionalities ✨

zimeg · 2025-07-24T20:02:01Z

manifest.json

+        "org_deploy_enabled": true,
+        "socket_mode_enabled": true,
+        "token_rotation_enabled": false,
+        "hermes_app_type": "remote",


Suggested change

"hermes_app_type": "remote",

🪓 I believe this is leftover but the same as function_runtime...

zimeg · 2025-07-24T20:02:31Z

package.json

+  },
+  "dependencies": {
+    "@slack/bolt": "^4.4.0",
+    "@slack/web-api": "^7.9.3",


Suggested change

"@slack/web-api": "^7.9.3",

We might have access to the client from @slack/bolt!

zimeg · 2025-07-24T20:07:40Z

utils/constants.js

🗣️ IMHO utils often hints at the need for another directory!

The time details that follow, and perhaps timezone specific logic for various listeners, could be moved into paths like so?

app.js - listeners - commands - functions # for workflow steps - tests - timezones

The exact logic splits might not be right, but I do think similar organization can make the separation of Slack logic and timezone handling more clear 👾

Mmm I see. Thank you!

zimeg · 2025-07-24T20:10:45Z

workflows/conversionWorkflows.js

+module.exports = {
+  handleTimeZoneConversionWF,
+  handleUserConversionWF,
+};


🤖 Exporting these functions into separate files might make future additions more obvious since fewer questions of where something should go might appear.

The shared logic within complicates that a smidge, but I think for app setups it might still be preferred.

Fixes the typo in the `timefor` command and removes the necessity for a signing secret since Chrono uses socket mode.

Removes signing secret because Chrono uses socket mode.

- Removed timefor slash command typos. - Removed signing secret mentionings since socket mode is used. - Removed the timeZoneMap.js file and moved contents into constants.js for better organization. - Removed the 'else' in the if/else statement in convertTimeLogic.js. - Addressed the overlap in the timeRanges array. - Handled cases where real_name is not guaranteed in teamClockLogic.js.

WilliamBergamin

Great app 💯 most comments seem to be address 🚀

Add files via upload

9e23613

ewanek1 requested a review from WilliamBergamin July 22, 2025 17:47

ewanek1 marked this pull request as ready for review July 22, 2025 17:48

ewanek1 changed the title ~~Add files via upload~~ Initial Code Review Jul 22, 2025

WilliamBergamin reviewed Jul 24, 2025

View reviewed changes

zimeg reviewed Jul 24, 2025

View reviewed changes

ewanek1 and others added 3 commits July 25, 2025 11:11

Update README.md

12d5ae2

Fixes the typo in the `timefor` command and removes the necessity for a signing secret since Chrono uses socket mode.

Update app.js

14bc301

Removes signing secret because Chrono uses socket mode.

WilliamBergamin approved these changes Jul 31, 2025

View reviewed changes

		@@ -0,0 +1,105 @@
		const { convertTimeCommandText } = require("../commands/convertTimeLogic.js");

Initial Code Review #1

Are you sure you want to change the base?

Initial Code Review #1

Uh oh!

Conversation

ewanek1 commented Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WilliamBergamin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WilliamBergamin Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WilliamBergamin Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zimeg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WilliamBergamin left a comment

Choose a reason for hiding this comment

Uh oh!

ewanek1 commented Jul 21, 2025 •

edited

Loading

WilliamBergamin left a comment •

edited

Loading

WilliamBergamin Jul 24, 2025 •

edited

Loading

WilliamBergamin Jul 24, 2025 •

edited

Loading