Better Route Search Remark #182
Conversation
|
It's great if screenshot can be shown. |
Some routes are now missing the origin, would it be better to add them back? |
|
The idea of this change is to only display differences between special routes with their main routes. Since the main route doesn't have any differences with itself, there is no remark. |
Might you please apply the fallback? |
|
Warning Rate limit exceeded@github-actions[bot] has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 41 minutes and 18 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe recent updates focus on enhancing internationalization and optimizing component logic within specified files. Key changes include incorporating the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (4)
src/utils.ts (4)
14-16: Consider adding a comment to explain the purpose oflangSpace.Although the function is simple, a brief comment explaining its purpose (returning a space character for English language settings) could improve code readability and maintainability.
Line range hint
55-59: ReplaceisFinitewithNumber.isFinitefor type safety.- if (typeof position.lat === "number" && isFinite(position.lat) && typeof position.lng === "number" && isFinite(position.lng)) + if (typeof position.lat === "number" && Number.isFinite(position.lat) && typeof position.lng === "number" && Number.isFinite(position.lng))This change ensures that the type coercion pitfalls of
isFiniteare avoided, which enhances the type safety of the function.
Line range hint
310-311: Use the exponentiation operator instead ofMath.pow.- return Math.floor(((lon + 180) / 360) * Math.pow(2, zoom)); + return Math.floor(((lon + 180) / 360) * (2 ** zoom));The exponentiation operator (
**) is more modern and concise thanMath.pow, and it's generally recommended for better readability and performance.
Line range hint
369-370: This else clause can be omitted for clarity.- } else { - // Chrome - }Removing this empty
elseclause will clean up the code and reduce unnecessary lines.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- src/components/route-board/RouteTerminus.tsx (1 hunks)
- src/components/route-eta/RouteHeader.tsx (3 hunks)
- src/i18n/translation.js (1 hunks)
- src/utils.ts (1 hunks)
Additional context used
GitHub Check: build (21.x)
src/components/route-board/RouteTerminus.tsx
[failure] 54-54:
Element implicitly has an 'any' type because expression of type 'string' can't be used to index type 'Terminal'.
[failure] 60-60:
Element implicitly has an 'any' type because expression of type 'string' can't be used to index type 'Terminal'.
[failure] 63-63:
Type 'Company[]' cannot be used as an index type.
[failure] 65-65:
Type 'Company[]' cannot be used as an index type.
[failure] 65-65:
Type 'Company[]' cannot be used as an index type.
[failure] 66-66:
Type 'Company[]' cannot be used as an index type.
[failure] 67-67:
Type 'Company[]' cannot be used as an index type.
[failure] 67-67:
Type 'Company[]' cannot be used as an index type.
[failure] 70-70:
Element implicitly has an 'any' type because expression of type 'string' can't be used to index type '{ en: string; zh: string; }'.
[failure] 73-73:
Element implicitly has an 'any' type because expression of type 'string' can't be used to index type '{ en: string; zh: string; }'.
GitHub Check: build (20.x)
src/components/route-board/RouteTerminus.tsx
[failure] 54-54:
Element implicitly has an 'any' type because expression of type 'string' can't be used to index type 'Terminal'.
[failure] 60-60:
Element implicitly has an 'any' type because expression of type 'string' can't be used to index type 'Terminal'.
[failure] 63-63:
Type 'Company[]' cannot be used as an index type.
[failure] 65-65:
Type 'Company[]' cannot be used as an index type.
[failure] 65-65:
Type 'Company[]' cannot be used as an index type.
[failure] 66-66:
Type 'Company[]' cannot be used as an index type.
[failure] 67-67:
Type 'Company[]' cannot be used as an index type.
[failure] 67-67:
Type 'Company[]' cannot be used as an index type.
[failure] 70-70:
Element implicitly has an 'any' type because expression of type 'string' can't be used to index type '{ en: string; zh: string; }'.
[failure] 73-73:
Element implicitly has an 'any' type because expression of type 'string' can't be used to index type '{ en: string; zh: string; }'.
Biome
src/utils.ts
[error] 61-65: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 80-91: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 104-107: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 123-130: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 300-301: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 369-370: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 392-393: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 401-403: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 409-410: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 426-430: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 55-56: isFinite is unsafe. It attempts a type coercion. Use Number.isFinite instead. (lint/suspicious/noGlobalIsFinite)
See the MDN documentation for more details.
Unsafe fix: Use Number.isFinite instead.
[error] 58-59: isFinite is unsafe. It attempts a type coercion. Use Number.isFinite instead. (lint/suspicious/noGlobalIsFinite)
See the MDN documentation for more details.
Unsafe fix: Use Number.isFinite instead.
[error] 310-311: Use the '**' operator instead of 'Math.pow'. (lint/style/useExponentiationOperator)
Unsafe fix: Use the '**' operator instead of 'Math.pow'.
[error] 323-324: Use the '**' operator instead of 'Math.pow'. (lint/style/useExponentiationOperator)
Unsafe fix: Use the '**' operator instead of 'Math.pow'.
Additional comments not posted (3)
src/components/route-eta/RouteHeader.tsx (1)
18-18: EnsureuseTranslationis properly utilized with bothtandi18nbeing used as intended.src/components/route-board/RouteTerminus.tsx (1)
13-13: EnsureuseTranslationis properly utilized with bothtandi18nbeing used as intended.src/i18n/translation.js (1)
33-39: Ensure consistency in translation strings.
| remark = t("從") + orig[i18n.language] + t("開出"); | ||
| } | ||
| } else { | ||
| let mainRouteFirstStop = stopList[data.stops[co][0]].name; |
There was a problem hiding this comment.
Address the issue where Company[] type cannot be used as an index type.
Consider using a different approach to index or retrieve data from these arrays, as direct indexing with a type that is not supported will lead to runtime errors.
Also applies to: 65-65, 66-66, 67-67
Tools
GitHub Check: build (21.x)
[failure] 63-63:
Type 'Company[]' cannot be used as an index type.
GitHub Check: build (20.x)
[failure] 63-63:
Type 'Company[]' cannot be used as an index type.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This reverts commit 716d217.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/components/route-board/RouteTerminus.tsx (1 hunks)
- src/components/route-eta/RouteHeader.tsx (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/components/route-eta/RouteHeader.tsx
Additional comments not posted (4)
src/components/route-board/RouteTerminus.tsx (4)
1-1: Import statements have been updated to includeDbContext.Ensure that
DbContextis properly set up in the context folder and is providing the necessary data.Also applies to: 6-6
13-13: Integration of theuseTranslationhook andDbContextlooks correct.Verify that all necessary translations and database contexts are correctly configured and accessible.
Also applies to: 16-16
144-150: The Typography component is properly configured to handle text overflow.This ensures that long destination names are gracefully truncated.
155-156: Good use of Typography for displayingrouteRemark.This maintains consistency in the UI and improves readability.
| stopList[data.stops[co[0]][data.stops[co[0]].length - 1]].name | ||
| .zh !== stopList[stops[co[0]][stops[co[0]].length - 1]].name.zh | ||
| ) { | ||
| remark = t("開往") + dest[i18n.language]; |
There was a problem hiding this comment.
Use optional chaining to safely access properties.
- remark = t("開往") + dest[i18n.language];
- remark = t("從") + orig[i18n.language] + t("開出");
+ remark = t("開往") + dest?.[i18n.language];
+ remark = t("從") + orig?.[i18n.language] + t("開出");Ensure that the code does not throw runtime errors when the language key is not found in the dest or orig objects.
Also applies to: 60-60, 70-70, 73-73
| let remark = ""; | ||
| if (Number(terminus.serviceType) >= 2) { | ||
| for (let [, data] of Object.entries(routeList)) { | ||
| if ( | ||
| Number(data.serviceType) === 1 && | ||
| route === data.route && | ||
| JSON.stringify(bound) === JSON.stringify(data.bound) && | ||
| (co[0] === "gmb" | ||
| ? gtfsId === data.gtfsId | ||
| : JSON.stringify(co) === JSON.stringify(data.co)) | ||
| ) { | ||
| if ( | ||
| stopList[data.stops[co[0]][data.stops[co[0]].length - 1]].name | ||
| .zh !== stopList[stops[co[0]][stops[co[0]].length - 1]].name.zh | ||
| ) { | ||
| remark = t("開往") + dest[i18n.language]; | ||
| } else if ( | ||
| stopList[data.stops[co[0]][0]].name.zh !== | ||
| stopList[stops[co[0]][0]].name.zh | ||
| ) { | ||
| if (!terminus.nlbId) { | ||
| remark = t("從") + orig[i18n.language] + t("開出"); | ||
| } | ||
| } else { | ||
| let mainRouteFirstStop = stopList[data.stops[co][0]].name; | ||
| let mainRouteLastStop = | ||
| stopList[data.stops[co[0]][data.stops[co[0]].length - 1]].name; | ||
| let routeFirstStop = stopList[stops[co[0]][0]].name; | ||
| let routeLastStop = | ||
| stopList[stops[co[0]][stops[co[0]].length - 1]].name; | ||
|
|
||
| if (mainRouteLastStop.zh !== routeLastStop.zh) { | ||
| remark = t("開往") + routeLastStop[i18n.language]; | ||
| } else if (mainRouteFirstStop.zh !== routeFirstStop.zh) { | ||
| if (!terminus.nlbId) { | ||
| remark = t("從") + routeFirstStop[i18n.language] + t("開出"); | ||
| } | ||
| } else { | ||
| let difference = stops[co[0]].filter( | ||
| (x) => !data.stops[co[0]].includes(x) | ||
| ); | ||
| let diffName = difference.map( | ||
| (x) => stopList[x].name[i18n.language] | ||
| ); | ||
| if (difference.length > 0) { | ||
| remark = | ||
| t("經") + | ||
| firstLastDiff(diffName).join( | ||
| t( | ||
| diffConsecutive(data.stops[co[0]], difference) | ||
| ? "至" | ||
| : "及" | ||
| ) | ||
| ); | ||
| } else { | ||
| let difference = data.stops[co[0]].filter( | ||
| (x) => !stops[co[0]].includes(x) | ||
| ); | ||
| let diffName = difference.map( | ||
| (x) => stopList[x].name[i18n.language] | ||
| ); | ||
| if (difference.length > 0) { | ||
| remark = | ||
| t("不經") + | ||
| firstLastDiff(diffName).join( | ||
| t( | ||
| diffConsecutive(data.stops[co[0]], difference) | ||
| ? "至" | ||
| : "及" | ||
| ) | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| remark = | ||
| t("從") + | ||
| toProperCase(terminus.orig[i18n.language]) + | ||
| t("開出") + | ||
| " " + | ||
| remark; | ||
| return remark; | ||
| }, [ | ||
| terminus, | ||
| routeList, | ||
| i18n.language, | ||
| bound, | ||
| co, | ||
| dest, | ||
| gtfsId, | ||
| orig, | ||
| route, | ||
| stopList, | ||
| stops, | ||
| t, | ||
| ]); |
There was a problem hiding this comment.
The memoization of routeRemark is implemented to enhance performance.
However, the complexity of this function is quite high due to multiple nested conditions and loops. Consider refactoring to simplify logic and improve readability.
- let remark = "";
- // existing complex logic
+ const remark = generateRemark(terminus, routeList, stopList, co, i18n);Create a separate function generateRemark to handle the remark generation logic.
Committable suggestion was skipped due to low confidence.
|
I've added the fallback and it works in testing. However, the code standard is getting more strict each time. Looks like I'll have to spend more time figuring it out. If anyone could help to sort out the code formatting that would be nice. |
Changes the "route remark" underneath each route in the search screen from the first stop to the difference between itself and the main line.
Continuation of #142.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes