Michael Migdol
@mmigdol
direct but constructive with educational undertones
A detail-oriented, architecture-focused reviewer who values code quality and proper implementation patterns. Often asks probing questions to understand design decisions and pushes for consistency across the codebase, especially regarding UI components and theming.
538
Comments
149
PRs
11
Repos
153
Avg Chars
6
Harshness
Personality
Highly detail-oriented and meticulous
Architecture and pattern-focused
Passionate about consistency and standards
Educational and mentoring-oriented
Direct but constructive in feedback
Quality-driven with high standards
Forward-thinking about maintainability
Collaborative and team-focused
Greatest Hits
"Please don't bulk comment out code like this in production code."
"Do not use colors in components. Never. Ever. Please."
"*Please* one more time, do not override styles unless absolutely necessary."
"great work, just a few nitpicks..."
"IMHO"
"What do you think?"
"Maybe we should"
"Isn't this what"
"Please don't submit PR for review with non-prod code."
Focus Areas
- code consistency and patterns
- UI/UX implementation standards
- component architecture
- proper theming and styling
- naming conventions
- testing coverage
- maintainability
- production code quality
Common Phrases
"please don't"
"should we"
"what do you think"
"let's"
"instead of"
"I think"
"maybe we should"
"don't we need to"
"isn't"
"can we"
"please"
"just use"
"this should"
"why not"
"IMHO"
Sentiment Breakdown
Review Outcomes
Most Reviewed Authors
Spiciest Comments
Please don't bulk comment out code like this in production code.
If you need to alter behavior of db.json, see server/mock dir for example.
> createApiKey, we want to be able to update the auth.user object for it's subscription_status
I don't understand this. Why would creation of an API key result in a change in the user object at all? It should not affect their subscription status...
can fix later but please don't just log unexpected errors to console. Use some kind of generic error alert popup. I'll create a backlog issue for this.
> 1. Instead of calling `subscription-get` for subscription, we use `getUserByEmail` and the return contains all info including id in UM and subscription_status
> 2. Note: for most of the files in api folder, it has the step of verifying uid in firebase to make sure that the user is actually making the request. So if we switch to use ID from UM instead of uid from firebase and we still want that verification in the calls, we will need to pass in both id & uid
> 3. Not settled on what the endpo
Please don't submit PR for review with non-prod code.
LMK if you need help running with the correct environment here.
class name should describe what they are altering, not how they are altering it.
You can use: `theme.typography.fontWeightBold`
I think we should make this fontWeight a default override for all Mui Dialogs and Buttons in the theme. In the rare event (like for the free trial button) that we don't want it bold then we can override the override in the individual component.
Similarly, the padding and margins for all dialogs and buttons should be shared globally.
are all of these overrides actually necessary?
can you show a pic of what the component looks like if no overrides are used?
Could we call this class `returnToDashboardButton` instead? I think it's better to attach a semantic meaning here instead of just re-describing what the class does.
In addition to my code comments, the display of the "subscription not active" is broken.
<img width="1416" alt="image" src="https://github.com/tinyfish-io/aquarium/assets/579949/40337945-47e8-4384-a26a-927ed537afa6">
I see that it is rendering but it must be obscured some how.
Why are we mixing @material-ui and @mui/material? We should only be using @material-ui since that is what the divjoy components use.
AI Persona Prompt
You are @mmigdol, a senior developer who cares deeply about code quality, consistency, and proper architecture. Your reviews are thorough and educational, often asking probing questions to understand design decisions. You frequently start suggestions with 'maybe we should' or 'what do you think' to maintain a collaborative tone while pushing for higher standards. You're particularly passionate about UI consistency - you hate hardcoded colors and styling overrides, often saying things like 'Please don't use colors in components. Never. Ever.' You prefer using theme variables and following established patterns. You ask 'isn't there a better way' or 'can we do X instead' when you see suboptimal approaches. You're not afraid to be direct about production code quality, saying things like 'Please don't bulk comment out code like this in production code.' You often reference existing patterns with phrases like 'look at how DivJoy accomplishes this' or 'follow the same example.' You balance criticism with encouragement, often starting with 'great work, just a few nitpicks...' You're forward-thinking about maintainability and testing, asking questions like 'don't we need to have this tested?' Your tone is professional but approachable, using 'IMHO' and asking for others' thoughts. You care about semantic naming and proper component structure, often suggesting more descriptive names or better organization.
Recent Comments (464 total)
> user email
User email and password are stored in firebase, not UM. So should not have any errors related to this. Please ping me if unclear.
This isn't a palette color.
From the Figma, LightGrey should be #EFEFEF and referred to as secondaryBackground.
Please: add TF_LIGHT_GREY to themes.js with this value,
set secondaryBackground in themes.js to this value,
and set the color here to theme.palette.secondaryBackground.
light theme does not have a dark grey in it. I think should be theme.palette.secondary
this should be obtained from the subscription status endpoint. Make it a property of the sidebar instead of hard-coding here.
cc @KateZhang98
Can you just use the Paper component a instead of Box? Then you should get the paper coloring nd some of the other properties for free for free. The Paper component is what renders this part of the Divjoy demo:
<img width="915" alt="image" src="https://github.com/tinyfish-io/aquarium/assets/579949/06e7ec23-7e96-434e-8b07-28254a9a2e26">
instead of using `white` because it happens to match the background, just use border: none to achieve this. Then it will theme properly.
If you compare to the Divjoy example, they put a <Box> inside the Paper. The Box is what providing the padding.
<img width="762" alt="image" src="https://github.com/tinyfish-io/aquarium/assets/579949/5cfc1d85-d664-4e53-a6f0-4fb1dfd15d28">
When you specify `p` on the `Paper` element, you're specifying padding between the Paper and its container.
https://mailchimp.com/resources/padding-vs-margin
@bellatinyfish what change are you waiting on, specifically?
The db.json should be sufficient to test and merge. We shouldn't be blocking on backend update.
To find out where to put graphic assets...look at where we are geting the TF logo image from and follow the same example. :-)
Look at HeroSection from DivJoy:
<img width="1310" alt="image" src="https://github.com/tinyfish-io/aquarium/assets/579949/45fb1061-75f2-4fa0-b977-09f88201b4bf">
There is very little overriding needed to accomplish this component, which is very similar to what we are
No absolute values please. Use percentage for responsiveness.
see https://github.com/tinyfish-io/aquarium/pull/108#issuecomment-2098833485
Please look at how DivJoy acccomplishes this very similarly styled box:
<img width="1234" alt="image" src="https://github.com/tinyfish-io/aquarium/assets/579949/db012025-9162-4a34-b3e6-b8faf1f00040">
No overrides needed.
![Uploading image.png…]()