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

fix: console clean up for bugs & lint issues, general UI/UX improvements #86

Merged
merged 15 commits into from
Feb 6, 2025

Conversation

klausborges
Copy link
Contributor

@klausborges klausborges commented Dec 17, 2024

Fixes for the RC phase.

Includes the #87 PR from @bgelatti as I had to clean up some last issues here.

dev-console:
just _pre-build
@rm -rf build/console
@cd packages/console && npm run build-watch
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added dev-console command to improve experience when developing on the console package

*/
const next = useCallback(async () => {
setErrorMsg("");
const validate = useCallback(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix validation on master password form during setup

@@ -29,8 +34,7 @@
"noCommonJs": "error"
},
"correctness": {
"useImportExtensions": "error",
"useExhaustiveDependencies": "off"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This lint rule is important, it can be tricky to fix issues but it's worth it

@@ -44,7 +52,7 @@
"zod": "3.23.8"
},
"devDependencies": {
"@biomejs/biome": "1.9.3",
"@biomejs/biome": "1.9.4",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Patch update to Biome

@@ -103,6 +103,7 @@ const WrappedStoreRoutes = observer(() => {
}
}, [plugins]);

// biome-ignore lint/correctness/useExhaustiveDependencies: useEffect state machine
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only disabling the rule manually, considering:

  1. Avoid big refactors on useEffect and other hooks
  2. Always put explanation when disabling the rule
  3. MobX observers are used with High-Order Components (HOCs)

See example of #3 in this same file


/**
* Starts the socket connection.
*/
// biome-ignore lint/correctness/useExhaustiveDependencies: mobx observers
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Explicitly state the disable is related to MobX observers, since HOCs are an "old" pattern for modern React

@@ -37,9 +37,6 @@ function AnalysisEdit() {
const initialData = useRef<IAnalysis>({} as IAnalysis);
const loading = !data.id;

/**
* Should return if the initial data is different from the current data.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the obvious/redundant comments, but only in files I'm already modifying for a reason

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These honestly make the code harder to read since there's too much noise

const next = useCallback(
(param: any) => {
if (SetupComponent === StepMasterPassword) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clear example of comparing components, using them as a way to keep state

return null;
}

if (currentStep === "master-password") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Broken down into separate if statements for each step, makes the code "taller" but easier to read and debug

@@ -10,12 +10,14 @@
"outDir": "./build-tsc",
"rootDir": "./src",
"baseUrl": ".",
"jsx": "react",
"jsx": "react-jsx",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was incorrectly configured

"noImplicitAny": true,
"declaration": true,
"noEmit": true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Required by another option


/**
* Uses to trigger a change to the input outside if the hour, minute or time format has changed.
* We ignore the first one because it's not needed.
*/
useEffect(() => {
if (!firstRender.current) {
triggerChange();
triggerChange({ hour, minute, format });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've fixed this in other places as well, making:

  1. useEffect to track the dependencies
  2. Function doesn't use state in dependency array, receives as params

@klausborges klausborges requested a review from felipefdl February 6, 2025 00:23
@klausborges klausborges marked this pull request as ready for review February 6, 2025 00:23
@klausborges klausborges requested a review from bgelatti February 6, 2025 00:25
@felipefdl felipefdl merged commit b786209 into main Feb 6, 2025
6 of 7 checks passed
@felipefdl felipefdl deleted the TCORE-234-tcore-clean-up branch February 6, 2025 12:47
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