Conversation
![windsurf-bot[bot]](https://avatars.githubusercontent.com/in/1066231?s=60&v=4){kind=small}
| if (allowOnlyIpAddresses) { | ||
| throw e | ||
| } |
There was a problem hiding this comment.
Consider adding more context to the error when throwing in strict mode (allowOnlyIpAddresses=true). This would make debugging easier.
| if (allowOnlyIpAddresses) { | |
| throw e | |
| } | |
| if (allowOnlyIpAddresses) { | |
| throw new Error(`Invalid IP address or CIDR: ${address}`); | |
| } |
| '*.home.fritz.net', | ||
| 'adguard-home.fritz.box', | ||
| 'foo.bar', | ||
| 'fritz.box', |
There was a problem hiding this comment.
The wildcard pattern '*.home.fritz.net' is placed before other domain names in the sorted result, which seems inconsistent with the alphabetical sorting of other non-IP strings. Consider either sorting wildcard patterns alphabetically along with other domain names or documenting the special ordering rule if this is intentional behavior.
|
/windsurf-review |
| } | ||
| }; | ||
|
|
||
| const getAsElementForSorting = (item: any) => { |
There was a problem hiding this comment.
The function name getAsElementForSorting is misleading since it doesn't return an element for sorting but rather converts the input to a string. Consider renaming to something like getStringForSorting or normalizeItemToString to better reflect its purpose.
| import { withTranslation } from 'react-i18next'; | ||
|
|
||
| import { sortIp } from '../../../helpers/helpers'; | ||
| import { sortAddress, sortAddressByTld } from '../../../helpers/helpers'; |
There was a problem hiding this comment.
The sortAddressByTld function is imported but not used in this file. Consider removing the unused import.
| return "" + item; | ||
| } |
There was a problem hiding this comment.
Missing semicolon at the end of the function.
| return "" + item; | |
| } | |
| return "" + item; | |
| } |
1 participant
Closes #6320
This introduces additional sorting function -
sortAddress- that extends the behaviour of existingsortIp. There were two options effectively hereThe second approach was selected. The unit tests for the previous implementation pass 100%, there is a new set of tests for the new function as well.
I also cleaned up a bit the code around sorting, added some additional type safety and removed a need for es-lint overrides.