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

Move route address validation from AccountPage into routes #1145

Merged
merged 2 commits into from Nov 14, 2022

Conversation

lukaw3d
Copy link
Member

@lukaw3d lukaw3d commented Nov 11, 2022

@github-actions
Copy link

github-actions bot commented Nov 11, 2022

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ EDITORCONFIG editorconfig-checker 5 0 0.01s
✅ REPOSITORY checkov yes no 17.15s
✅ REPOSITORY git_diff yes no 0.01s
✅ TSX eslint 5 0 0 6.8s

See errors details in artifact MegaLinter reports on CI Job page
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security

@codecov
Copy link

codecov bot commented Nov 11, 2022

Codecov Report

Merging #1145 (234ba02) into master (ccd5676) will decrease coverage by 0.39%.
The diff coverage is 36.84%.

❗ Current head 234ba02 differs from pull request most recent head 79430aa. Consider uploading reports for the commit 79430aa to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1145      +/-   ##
==========================================
- Coverage   84.91%   84.52%   -0.40%     
==========================================
  Files         126      127       +1     
  Lines        2374     2384      +10     
  Branches      570      574       +4     
==========================================
- Hits         2016     2015       -1     
- Misses        358      369      +11     
Flag Coverage Δ
cypress 52.09% <36.84%> (-0.27%) ⬇️
jest 77.76% <5.26%> (-0.54%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/app/components/ErrorBoundary/index.tsx 0.00% <0.00%> (ø)
src/app/pages/AccountPage/index.tsx 94.91% <75.00%> (-0.25%) ⬇️
src/commonRoutes.tsx 100.00% <100.00%> (ø)
src/routes.tsx 100.00% <100.00%> (ø)

@lukaw3d lukaw3d force-pushed the lw/route-validation branch 2 times, most recently from 110c65e to ef11ea1 Compare November 11, 2022 19:03
<AlertBox color={'status-error'}>{t('errors.invalidAddress', 'Invalid address')}</AlertBox>
</Box>
)
export async function validateRoute(params: AccountPageParams) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why it's async?

Copy link
Member Author

Choose a reason for hiding this comment

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

leftover

@@ -29,6 +25,10 @@ export const commonRoutes: Route[] = [
{
path: 'account/:address/*',
element: <AccountPage />,
errorElement: <ErrorBoundary></ErrorBoundary>,
loader: async ({ params }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

async not needed anymore

Copy link
Member Author

Choose a reason for hiding this comment

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

Oddly it works that way too, but all documentation and router code looks like it intends loader to return a Response or Promise. I'd keep it

@lukaw3d lukaw3d merged commit a371410 into master Nov 14, 2022
@lukaw3d lukaw3d deleted the lw/route-validation branch November 14, 2022 23:37
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.

None yet

2 participants