Skip to content
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.

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

First release #1

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

First release #1

wants to merge 6 commits into from

Conversation

jannatkhandev
Copy link

@jannatkhandev jannatkhandev commented Jan 7, 2025

A fully functional end-to-end test RC app for Todoist integration.
closes RocketChat/Rocket.Chat#34896

🚀 Looking forward to add many more features.

Screen.Recording.2025-01-07.at.5.1.mp4

Todoist App provides you the following slash commands, /todoist:

  1. help: shows this list.
  2. auth: starts the process to authorize your Todoist Account.
  3. task: opens modal to create a new task.
  4. projects: fetches the projects you are part of.
  5. tasks: fetches the active tasks.
  6. sections: fetches the active tasks.
  7. labels: fetches your personal labels.
  8. shared-labels: fetches your shared labels.

Allows users to create tasks from message, create task within project.

@jannatkhandev
Copy link
Author

@casalsgh sorry to tag you, but this is ready to merge.
Please take a look.
Best,
Jannat.

@Gustrb
Copy link

Gustrb commented Jan 13, 2025

Hey, @jannatkhandev just to make you aware, this is a 4k+ line pull request, the review will take more time than it would be since, I can't look at this PR in my free time. So I'll have to check if it makes sense with the rest of the company for me to help you through it.
But don't worry, I didn't forgot about you :)

Copy link

@Gustrb Gustrb left a comment

Choose a reason for hiding this comment

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

I couldn't review everything, but since the code is mostly similar, I think you'll be able to extrapolate a lot of those comments to more places

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
TodoistApp.ts Outdated Show resolved Hide resolved
TodoistApp.ts Outdated Show resolved Hide resolved
src/commands/subcommands/authorize.ts Outdated Show resolved Hide resolved
src/commands/subcommands/authorize.ts Outdated Show resolved Hide resolved
src/commands/subcommands/labels.ts Outdated Show resolved Hide resolved
src/commands/subcommands/labels.ts Outdated Show resolved Hide resolved
src/commands/subcommands/labels.ts Outdated Show resolved Hide resolved
@jannatkhandev
Copy link
Author

Thanks for the detailed feedback, I will ensure I inspect rest of the code as well.
I really appreciate your time.
Best,
Jannat.

@jannatkhandev
Copy link
Author

Hey there,
I have made changes as per the review, please take a look.

TodoistApp.ts Show resolved Hide resolved
src/helpers/message.ts Outdated Show resolved Hide resolved
src/helpers/message.ts Outdated Show resolved Hide resolved
src/helpers/message.ts Outdated Show resolved Hide resolved
src/helpers/message.ts Outdated Show resolved Hide resolved
src/helpers/message.ts Outdated Show resolved Hide resolved
src/helpers/message.ts Outdated Show resolved Hide resolved
src/helpers/message.ts Outdated Show resolved Hide resolved
src/helpers/message.ts Outdated Show resolved Hide resolved
src/helpers/notification.ts Outdated Show resolved Hide resolved
src/lib/share/shareItem.ts Outdated Show resolved Hide resolved
@jannatkhandev
Copy link
Author

Thanks for the detailed feedback. I have addressed the review and have made changes accordingly.
Also have tested my app end to end.

@jannatkhandev
Copy link
Author

Thanks for the detailed feedback. I have addressed the review and have made changes accordingly. Also have tested my app end to end.

I really appreciate your time, I shall be highly grateful to you :)

@jannatkhandev jannatkhandev requested a review from Gustrb January 16, 2025 17:24
src/commands/subcommands/authorize.ts Outdated Show resolved Hide resolved
src/commands/subcommands/labels.ts Outdated Show resolved Hide resolved
async function createLabelSection(label: ILabel): Promise<LayoutBlock[]> {
const labelNameBlock = getSectionBlock(label.name);
const labelContextBlock = getContextBlock(
`Color: ${String(label.color).charAt(0).toUpperCase() + String(label.color).slice(1)} | ` +
Copy link

Choose a reason for hiding this comment

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

Lets put this into a separate function wdyt?
this function is dealing contextually with label blocks, I don't think it is good for it to know exactly how the color field is internally

src/commands/subcommands/labels.ts Show resolved Hide resolved
src/commands/subcommands/projects.ts Show resolved Hide resolved
src/lib/delete/deleteItem.ts Outdated Show resolved Hide resolved
src/lib/delete/deleteItem.ts Outdated Show resolved Hide resolved
src/lib/fetch/fetchComments.ts Outdated Show resolved Hide resolved
src/lib/fetch/fetchComments.ts Outdated Show resolved Hide resolved
src/lib/share/shareItem.ts Outdated Show resolved Hide resolved
@Gustrb
Copy link

Gustrb commented Jan 17, 2025

Ok so, I took some time to look at your code, and it is getting pretty good, congratulations 🚀
But I have A few issues I found during the review.
Most of them are due to:

  • Poor logging (you'll thank me later for being annoying with this, logging is our friend when everything stops working)
  • No domaing splitting (you are grouping functions by "what they do" instead of by "what they are"), doing that will make the code easier to read and navigate
  • Mixing up what is domain and what is infraestructure, please, avoid doing direct HTTP calls from your UI components, where you build your UI should not care about HTTP stuff such as the error that came back, or if the body is empty. When you start defering that to a service class, it will become clear there are a few issues with how everything is being handled, so the first issue is:
  1. There are not a lot of validations on what we are sending the network (nothing on the typesystem): Since our HTTP calls go through a bridge, then through (possibliy) a proxy to get over to the Todoist backend, it might be really slow, so we must be pedantic and only allow http calls that are going to succeed.
  2. There are no validations on the recieving end, everything is being typed as any and then being typed accordingly through a type assertion, this is really dangerous, since we have no idea when the api changes, and small things might start breaking slowly as the time goes by, and we will have 0 visibility into it. So please, use a library to validate your json schema from the response (and type it accordingly in the typesystem, so it is easier to mantain, and one needn't to have the API docs open to change simple stuff in the code)

@Gustrb
Copy link

Gustrb commented Jan 17, 2025

Also, dropping here a cool article about defensive programming, I think you'll benefit a lot from thinking like this: https://medium.com/@arumukherjee121/the-art-of-defensive-programming-62c6f22b2758

Since it is really annoying to update apps, and manage your deployed apps, we need to get the first version as stable as possible, and we do it, by programming defensively

@jannatkhandev
Copy link
Author

I really appreciate your time, the feedback and review really helps me grow as a developer.
As I am still learning and trying to improve, your review really made me understand industry standards.

I will make all the improvements over the weekend and will get ready by Monday addressing all the comments.

Yet again, thanks for your time and feedback.
Best,
Jannat :)

@Gustrb
Copy link

Gustrb commented Jan 17, 2025

Don't worry, I'm happy to help :)
I still haven't started doing actual testing, but after this next round of reviews we can start doing this.
You're doing a great job, keep going 🚀

Also, don't worry about being quick to ship stuff, take your time, ponder on your code, experiment, change stuff, that is really important for your growth as a developer.
Have a good weekend!

@CLAassistant
Copy link

CLAassistant commented Jan 24, 2025

CLA assistant check
All committers have signed the CLA.

@jannatkhandev jannatkhandev force-pushed the main branch 2 times, most recently from a3745a8 to 45fa44a Compare January 24, 2025 13:57
@jannatkhandev jannatkhandev requested a review from Gustrb January 24, 2025 14:05
@jannatkhandev
Copy link
Author

Hello sir,

I deeply retrospected the code and made all requested changes, made the app more robust and code easy to follow.
Please take look.

(Also sorry for messing up the commits, I changed my machine [now I run VPS on a server and use it in VSCode via SSH])

};

constructor(info: IAppInfo, logger: ILogger, accessors: IAppAccessors) {
super(info, logger, accessors);
Copy link

Choose a reason for hiding this comment

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

why not just:

this.labelService = new LabelService(this);
...

Copy link
Author

Choose a reason for hiding this comment

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

If I call the constructor here it goes into a cyclic loop of invocations as we use TodoistApp in the constructor of each service, hence.

@Gustrb
Copy link

Gustrb commented Jan 30, 2025

Looking nice, I just think your validations are a little to optimistic, besides that, the code is looking good, congrats!

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