-
Notifications
You must be signed in to change notification settings - Fork 1
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/repo name #136
Fix/repo name #136
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @abdheshnayak - I've reviewed your changes and they look great!
General suggestions:
- Ensure that the change from
console.trace
toconsole.log
for error logging inlib/client/helpers/log.ts
aligns with the logging strategy and does not omit valuable stack trace information in non-development environments. - Review the introduction of
return null;
insrc/apps/console/routes/_main+/$account+/repo+/$repo+/images/sha-dialog.tsx
to ensure it does not unintentionally disable theSHADialog
component. - Consider the implications of changing error handling from throwing errors to using
toast.error
for user notifications insrc/apps/console/routes/_main+/$account+/repo+/$repo+/new-build/route.tsx
. Ensure this aligns with the overall error handling strategy. - Verify that the decoding of the
repo
parameter usingatob
is consistently applied across all relevant parts of the application to ensure smooth functionality and data integrity.
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Docstrings: all looks good
Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨
const { account } = useParams(); | ||
|
||
const { repo } = useParams(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code_refinement): It seems like useParams()
is called twice consecutively to extract account
and repo
. Consider combining these into a single destructuring assignment for cleaner code and potentially better performance.
const { account } = useParams(); | |
const { repo } = useParams(); | |
const { account, repo } = useParams(); |
if (!repoName) { | ||
toast.error('Repository is required!.'); | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (code_refinement): Replacing the throw new Error
with toast.error
and a return statement changes the error handling flow. Ensure that this change aligns with the application's error handling strategy, especially in asynchronous operations where a thrown error might be caught by a higher-level error handler.
if (!repoName) { | |
toast.error('Repository is required!.'); | |
return; | |
if (!repoName) { | |
toast.error('Repository is required!.'); | |
return Promise.reject(new Error('Repository is required!.')); | |
} |
const { repoName } = useOutletContext<IRepoContext>(); | ||
|
||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (bug_risk): The addition of return null;
immediately after fetching repoName
from the context effectively disables the SHADialog
component. If this is intentional for temporary disabling, consider adding a comment to clarify. Otherwise, review the logic to ensure the component functions as expected.
const { repoName } = useOutletContext<IRepoContext>(); | |
return null; | |
const { repoName } = useOutletContext<IRepoContext>(); | |
// TODO: Temporarily returning null. Need to implement or fix the logic to utilize `repoName`. | |
// return null; |
@@ -16,7 +16,7 @@ export const loader = async (ctx: IRemixCtx) => { | |||
|
|||
const promise = pWrapper(async () => { | |||
const { data, errors } = await GQLServerHandler(ctx.request).listBuildRuns({ | |||
repoName: repo, | |||
repoName: atob(repo), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code_refinement): The use of atob
for decoding repo
parameter is consistent across multiple loaders. This is a good practice as it centralizes the decoding logic, making it easier to manage and understand. However, ensure that there's corresponding encoding logic wherever repo
parameters are generated or redirected to maintain consistency.
repoName: atob(repo), | |
repoName: atob(repo), |
* 🐛 Fixed issue with repoName
* 🐛 Fixed issue with repoName
* 🐛 Fixed issue with repoName
fixed issue with reponame. in distribution.