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

feat(skymp5-client): update loadOrderVerificationService.ts #2318

Merged
merged 2 commits into from
Feb 7, 2025

Conversation

Pospelove
Copy link
Contributor

@Pospelove Pospelove commented Feb 7, 2025

Important

getServerMods() in loadOrderVerificationService.ts now uses a hardcoded URL for server mods retrieval, with TODOs to unhardcode it.

  • Behavior:
    • getServerMods() in loadOrderVerificationService.ts now uses a hardcoded URL https://gateway.skymp.net/api/servers/sweetpie/manifest.json instead of constructing it from targetIp and uiPort.
    • Adds TODO comments to unhardcode the master address and serverId.
  • Misc:
    • Removes usage of getServerIp() and getServerUiPort() in getServerMods() function.

This description was created by Ellipsis for 9b8b37b. It will automatically update as commits are pushed.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Reviewed everything up to 9b8b37b in 1 minute and 10 seconds

More details
  • Looked at 18 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. skymp5-client/src/services/services/loadOrderVerificationService.ts:112
  • Draft comment:
    Beware: The base URL already ends with 'manifest.json', so using .get('/manifest.json') might append it twice.
  • Reason this comment was not posted:
    Marked as duplicate.
2. skymp5-client/src/services/services/loadOrderVerificationService.ts:112
  • Draft comment:
    TODO left: unhardcode master address and serverId ('sweetpie'). Consider parameterizing or configuring these values.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    The comment is about changed code, as this section was modified to use a hardcoded URL instead of constructing it from targetIp and uiPort. However, the code already has TODO comments marking these issues. The automated comment just restates what's already marked and doesn't provide additional actionable information beyond suggesting "parameterizing or configuring" which is obvious.
    The comment does make a constructive suggestion about parameterizing the values. Maybe this additional suggestion makes it worth keeping?
    The suggestion to parameterize is obvious and implicit in the TODO comments. Anyone addressing the TODOs would naturally consider configuration/parameterization as the solution.
    The comment should be deleted as it's redundant with the TODO comments already in the code and doesn't provide additional actionable value.

Workflow ID: wflow_edczSyUEj5f67peE


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

return new HttpClient(`http://${targetIp}:${uiPort}`)
// TODO: unhardcode master address
// TODO: unhardcode serverId (sweetpie)
let addr = "https://gateway.skymp.net/api/servers/sweetpie/manifest.json";
Copy link

Choose a reason for hiding this comment

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

Using a full URL ending with 'manifest.json' and then calling .get('/manifest.json') can cause path duplication.

@Pospelove Pospelove changed the title Update loadOrderVerificationService.ts feat(skymp5-client): update loadOrderVerificationService.ts Feb 7, 2025
@Pospelove Pospelove merged commit e3d9ac6 into main Feb 7, 2025
17 checks passed
@Pospelove Pospelove deleted the Pospelove-patch-7 branch February 7, 2025 17:44
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.

1 participant