Improve performance and more #1148
Conversation
📝 WalkthroughWalkthroughCentralized JSON API via JsonApiBase/ApiAccessManager; connectors migrated to parsed/converted flows; many thin parser modules removed; lazy heavy-module imports and BackgroundImporter added; rating-history pagination UI introduced; updater rewired to FilesObtainer async flow; ICE polling signals and status-bar wiring added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/UI
participant JsonApiBase as JsonApiBase
participant ApiAccessManager as ApiAccessManager
participant QNAM as QNetworkAccessManager
participant Handler as ResponseHandler
Client->>JsonApiBase: get_by_query_parsed(query, handler)
JsonApiBase->>ApiAccessManager: get(endpoint, _decode_and_handle(handler))
ApiAccessManager->>QNAM: send request
QNAM-->>ApiAccessManager: reply
ApiAccessManager->>ApiAccessManager: on_request_finished(reply)
ApiAccessManager->>JsonApiBase: decode_reply(reply) -> ApiResponse
JsonApiBase->>JsonApiBase: parse_message(ApiResponse) -> ParsedApiResponse
JsonApiBase->>Handler: invoke handler(parsed)
Handler-->>Client: processed result
sequenceDiagram
participant Updater as Updater
participant Obtainer as FilesObtainer
participant FModApi as FeaturedModApiConnector
participant FModFiles as FeaturedModFilesApiConnector
participant Worker as UpdaterWorker
Updater->>Obtainer: request_files()
Obtainer->>FModApi: get_by_query_parsed(...)
FModApi-->>Obtainer: convert_parsed(parsed) -> main_files
Obtainer->>FModFiles: get_by_query_parsed(...)
FModFiles-->>Obtainer: convert_parsed(parsed) -> fmod_files
Obtainer->>Obtainer: check_finish()
Obtainer-->>Updater: finished(main_files, fmod_files)
Updater->>Worker: do_update((main_files, fmod_files))
Worker->>Worker: update_featured_mod(files)
Worker-->>Updater: progress / completion events
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/player_api.py (1)
42-50: PotentialKeyErrororTypeErrorwhen player has no clan membership.Line 45 accesses
player_dict["clanMembership"]["clan"]without checking ifclanMembershipexists or is notNone. If a player has no clan membership, this will raise an exception.🔎 Proposed fix with defensive check
def handle_player(self, message: PreProcessedApiResponse) -> None: player_dict, = message["data"] player = Player(**player_dict) - if "clan" in player_dict["clanMembership"]: + clan_membership_dict = player_dict.get("clanMembership") + if clan_membership_dict and "clan" in clan_membership_dict: - clan = Clan(**player_dict["clanMembership"]["clan"]) - clan_membership = ClanMembership(**player_dict["clanMembership"]) + clan = Clan(**clan_membership_dict["clan"]) + clan_membership = ClanMembership(**clan_membership_dict) clan_membership.custom_clan = clan player.custom_clan_membership = clan_membership self.player_ready.emit(player)
🧹 Nitpick comments (6)
src/replays/replayitem.py (1)
337-342: Clean refactoring to sorting-based winner detection.The new logic correctly identifies the highest-scoring team and handles ties by clearing
teamWinwhen multiple teams share the top score.Minor: Unused loop variable flagged by Ruff.
The
teamvariable is unpacked but not used within the loop body. Consider using_to indicate it's intentionally unused:🔎 Suggested refactor
- for team, score in sorted_scores[1:]: + for _, score in sorted_scores[1:]: if score >= mvt: self.teamWin = None breaksrc/api/ApiBase.py (1)
237-254: Consider defensive handling for missing reply entries.If
on_request_finishedis invoked for a reply that was never registered (edge case),handlers.pop(reply)at line 252 would raise aKeyError. While unlikely in normal operation, usingpop(reply, None)would be more defensive.🔎 Optional defensive fix
- self.handlers.pop(reply) - self.error_handlers.pop(reply) + self.handlers.pop(reply, None) + self.error_handlers.pop(reply, None)res/player_card/playercard.ui (1)
361-365: Consider adding a maximum value to the spin box.The
defaultRatingPagesSpinBoxhas a minimum of 1 but no maximum. Consider setting a reasonable maximum (e.g., 50 or 100) to prevent users from accidentally requesting an excessive number of pages at once.🔎 Suggested addition
<widget class="QSpinBox" name="defaultRatingPagesSpinBox"> <property name="minimum"> <number>1</number> </property> + <property name="maximum"> + <number>50</number> + </property> </widget>src/stats/leaderboard_widget.py (1)
163-166: Consider guarding against missing meta field.The
ParsedApiResponseTypedDict showsmetaasNotRequired, butprocess_rating_infodirectly accessesparsed["meta"]. If the API ever returns a response without meta, this will raise aKeyError.🔎 Suggested defensive access
def process_rating_info(self, parsed: ParsedApiResponse) -> None: self.createLeaderboard(parsed) - self.processMeta(parsed["meta"]) + if "meta" in parsed: + self.processMeta(parsed["meta"]) self.resetLoading() self.timer.stop()src/api/stats_api.py (1)
189-193: Generator return type may cause single-consumption issue.The
convert_parsedreturns a generator expression(Achievement(...) for ...)typed asIterator[Achievement]. Generators can only be iterated once; if the caller stores the result and tries to iterate twice, the second iteration will be empty.Consider using a list if multiple iterations are expected, or document that this is intentional for memory efficiency.
🔎 Alternative using list for multi-iteration safety
def convert_parsed( self, parsed: ParsedApiResponse, - ) -> dict[str, Iterator[Achievement]]: - return {"values": (Achievement(**entry) for entry in parsed["data"])} + ) -> dict[str, list[Achievement]]: + return {"values": [Achievement(**entry) for entry in parsed["data"]]}src/replays/_replayswidget.py (1)
606-610: Consider adding type guard for robustness.While the current code structure ensures only
LocalReplayBucketIteminstances withLocalReplayItemchildren trigger this handler, adding an explicit type check would make the code more defensive and self-documenting.🔎 Proposed enhancement
def on_item_expanded(self, item: LocalReplayBucketItem) -> None: + if not isinstance(item, LocalReplayBucketItem): + return for index in range(item.childCount()): - cast(LocalReplayItem, item.child(index))._setup_appearance() + child = item.child(index) + if isinstance(child, LocalReplayItem): + child._setup_appearance()
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (62)
requirements.txtres/client/client.uires/player_card/playercard.uisetup.pysrc/api/ApiAccessors.pysrc/api/ApiBase.pysrc/api/coop_api.pysrc/api/featured_mod_api.pysrc/api/matchmaker_queue_api.pysrc/api/parsers/CoopResultParser.pysrc/api/parsers/CoopScenarioParser.pysrc/api/parsers/FeaturedModFileParser.pysrc/api/parsers/FeaturedModParser.pysrc/api/parsers/GeneratedMapParamsParser.pysrc/api/parsers/LeaderboardParser.pysrc/api/parsers/LeaderboardRatingJournalParser.pysrc/api/parsers/LeaderboardRatingParser.pysrc/api/parsers/MapParser.pysrc/api/parsers/MapVersionParser.pysrc/api/parsers/ModParser.pysrc/api/parsers/ModVersionParser.pysrc/api/parsers/PlayerParser.pysrc/api/parsers/ReviewsSummaryParser.pysrc/api/parsers/__init__.pysrc/api/player_api.pysrc/api/sim_mod_updater.pysrc/api/stats_api.pysrc/api/vaults_api.pysrc/chat/channel_tab.pysrc/client/_clientwindow.pysrc/coop/_coopwidget.pysrc/downloadManager/__init__.pysrc/fa/__init__.pysrc/fa/check.pysrc/fa/game_process.pysrc/fa/game_session.pysrc/fa/game_updater/updater.pysrc/fa/game_updater/worker.pysrc/games/_gameswidget.pysrc/heavy_modules.pysrc/notifications/ns_dialog.pysrc/playercard/leagueformatter.pysrc/playercard/playerinfodialog.pysrc/playercard/plot.pysrc/playercard/ratingtabwidget.pysrc/playercard/statistics.pysrc/qt/graphics/labeledbargraphitem.pysrc/qt/graphics/piechartitem.pysrc/replays/_replayswidget.pysrc/replays/replaydetails/replaycard.pysrc/replays/replaydetails/replayreader.pysrc/replays/replaydetails/tabs/gamestats.pysrc/replays/replaydetails/tabs/heatmap.pysrc/replays/replayitem.pysrc/stats/_statswidget.pysrc/stats/leaderboard_widget.pysrc/vaults/detailswidget.pysrc/vaults/mapvault/mapdetails.pysrc/vaults/mapvault/mapvault.pysrc/vaults/modvault/modvault.pysrc/vaults/reviewwidget.pysrc/vaults/vault.py
💤 Files with no reviewable changes (15)
- src/api/parsers/FeaturedModParser.py
- src/api/parsers/FeaturedModFileParser.py
- src/api/parsers/CoopScenarioParser.py
- src/api/parsers/MapParser.py
- src/api/parsers/PlayerParser.py
- src/api/parsers/LeaderboardParser.py
- src/api/parsers/GeneratedMapParamsParser.py
- src/api/parsers/CoopResultParser.py
- src/api/parsers/ModParser.py
- src/api/parsers/LeaderboardRatingParser.py
- src/api/parsers/MapVersionParser.py
- src/stats/_statswidget.py
- src/api/parsers/LeaderboardRatingJournalParser.py
- src/api/parsers/ReviewsSummaryParser.py
- src/api/parsers/ModVersionParser.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-23T00:07:39.893Z
Learnt from: Gatsik
Repo: FAForever/client PR: 1145
File: src/api/vaults_api.py:114-116
Timestamp: 2025-11-23T00:07:39.893Z
Learning: In src/api/vaults_api.py, the MapPoolApiConnector class uses intentional stateful filter mutation. The request_pool_for_queue method sets self._filters and is the intended public API; request_data() should not be called directly on instances of this class.
Applied to files:
src/api/vaults_api.py
🧬 Code graph analysis (29)
src/vaults/vault.py (2)
src/vaults/mapvault/mapvault.py (1)
setup(26-40)src/vaults/modvault/modvault.py (1)
setup(27-47)
src/api/matchmaker_queue_api.py (3)
src/api/ApiBase.py (1)
ParsedApiResponse(38-40)src/api/featured_mod_api.py (2)
convert_parsed(17-21)convert_parsed(32-36)src/api/ApiAccessors.py (1)
convert_parsed(198-199)
src/replays/replayitem.py (1)
src/replays/models.py (1)
score(48-49)
src/api/sim_mod_updater.py (1)
src/api/ApiAccessors.py (1)
get_by_query_parsed(43-53)
src/playercard/playerinfodialog.py (2)
src/config/__init__.py (2)
Settings(43-207)group(51-56)src/playercard/leagueformatter.py (1)
league_formatter_factory(108-116)
src/chat/channel_tab.py (1)
src/util/theme.py (3)
theme(142-143)sound(118-120)sound(355-357)
src/fa/__init__.py (1)
src/fa/game_process.py (1)
kill_all_if_running(198-200)
src/api/featured_mod_api.py (5)
src/api/ApiAccessors.py (4)
DataApiAccessor(32-199)convert_parsed(198-199)requestData(187-196)get_by_endpoint_converted(101-111)src/api/ApiBase.py (1)
ParsedApiResponse(38-40)src/api/coop_api.py (2)
convert_parsed(14-18)convert_parsed(57-63)src/api/models/FeaturedMod.py (1)
FeaturedMod(6-12)src/api/models/FeaturedModFile.py (1)
FeaturedModFile(6-15)
src/api/stats_api.py (4)
src/api/ApiBase.py (2)
ParsedApiResponse(38-40)get_by_query(92-99)src/api/models/Leaderboard.py (2)
Leaderboard(8-37)order(27-31)src/api/models/LeaderboardRating.py (1)
LeaderboardRating(7-14)src/api/ApiAccessors.py (1)
DataApiAccessor(32-199)
src/stats/leaderboard_widget.py (2)
src/api/ApiBase.py (1)
ParsedApiResponse(38-40)src/api/ApiAccessors.py (1)
get_by_query_parsed(43-53)
src/api/vaults_api.py (5)
src/api/ApiBase.py (1)
ParsedApiResponse(38-40)src/api/ApiAccessors.py (3)
requestData(187-196)get_by_query_parsed(43-53)post_and_parse(67-79)src/api/models/MapVersion.py (2)
Map(37-48)MapVersion(51-73)src/api/models/Map.py (1)
Map(10-21)src/util/__init__.py (1)
decapitalize(527-528)
src/vaults/reviewwidget.py (3)
src/api/ApiBase.py (1)
ParsedApiResponse(38-40)src/replays/models.py (1)
score(48-49)src/vaults/detailswidget.py (1)
on_reviews_data(379-391)
src/fa/check.py (2)
src/fa/game_updater/updater.py (2)
FilesObtainer(243-292)Updater(157-235)src/fa/game_updater/worker.py (1)
UpdaterWorker(39-289)
src/vaults/modvault/modvault.py (1)
src/vaults/vault.py (1)
setup(41-109)
src/api/ApiBase.py (1)
src/api/ApiAccessors.py (2)
handle(39-40)handle(85-86)
src/fa/game_updater/worker.py (2)
src/api/models/FeaturedModFile.py (1)
FeaturedModFile(6-15)src/model/transaction.py (1)
emit(18-19)
src/playercard/leagueformatter.py (1)
src/downloadManager/__init__.py (2)
CachedImageDownloader(480-512)image_size(428-429)
src/notifications/ns_dialog.py (1)
src/util/theme.py (2)
sound(118-120)sound(355-357)
src/api/player_api.py (1)
src/api/ApiAccessors.py (1)
get_by_query_parsed(43-53)
src/api/ApiAccessors.py (1)
src/api/ApiBase.py (10)
ApiResourceObject(25-29)ApiResponse(32-35)JsonApiBase(53-147)ParsedApiResponse(38-40)_do_nothing(49-50)handle(64-65)get_by_query(92-99)get_by_endpoint(101-108)post(110-122)post(181-197)
src/coop/_coopwidget.py (1)
src/coop/coopmodel.py (1)
CoopGameFilterModel(5-14)
src/vaults/mapvault/mapdetails.py (3)
src/api/ApiBase.py (1)
ParsedApiResponse(38-40)src/api/ApiAccessors.py (1)
get_by_query_parsed(43-53)src/vaults/detailswidget.py (1)
allow_review(91-92)
src/api/coop_api.py (8)
src/api/ApiBase.py (1)
ParsedApiResponse(38-40)src/api/featured_mod_api.py (2)
convert_parsed(17-21)convert_parsed(32-36)src/api/stats_api.py (2)
convert_parsed(51-60)convert_parsed(189-193)src/api/vaults_api.py (3)
convert_parsed(78-82)convert_parsed(98-105)convert_parsed(123-129)src/api/ApiAccessors.py (1)
convert_parsed(198-199)src/api/matchmaker_queue_api.py (1)
convert_parsed(13-26)src/api/models/CoopScenario.py (1)
CoopScenario(7-13)src/api/models/CoopResult.py (1)
CoopResult(7-14)
src/vaults/detailswidget.py (3)
src/api/ApiBase.py (1)
ParsedApiResponse(38-40)src/vaults/mapvault/mapdetails.py (1)
allow_review(55-58)src/vaults/reviewwidget.py (1)
on_reviews_data(330-337)
src/client/_clientwindow.py (2)
src/fa/game_process.py (1)
kill_all_if_running(198-200)src/connectivity/IceAdapterProcess.py (1)
start(238-241)
src/fa/game_updater/updater.py (5)
src/api/featured_mod_api.py (5)
FeaturedModApiConnector(13-25)FeaturedModFilesApiConnector(28-46)request_fmod_by_name(23-25)get_main_files(42-43)get_mod_files(45-46)src/api/models/FeaturedMod.py (1)
FeaturedMod(6-12)src/api/models/FeaturedModFile.py (1)
FeaturedModFile(6-15)src/fa/game_updater/worker.py (2)
UpdaterWorker(39-289)do_update(258-284)src/config/__init__.py (1)
group(51-56)
src/playercard/ratingtabwidget.py (2)
src/api/models/LeaderboardRating.py (1)
LeaderboardRating(7-14)src/api/models/Leaderboard.py (1)
pretty_name(14-15)
src/replays/_replayswidget.py (5)
src/api/ApiBase.py (1)
ParsedApiResponse(38-40)src/model/playerset.py (1)
get_by_name(19-22)src/replays/models.py (1)
icon(123-124)src/util/theme.py (1)
icon(369-409)src/api/ApiAccessors.py (1)
get_by_query_parsed(43-53)
src/vaults/mapvault/mapvault.py (1)
src/vaults/vault.py (1)
setup(41-109)
🪛 Ruff (0.14.10)
src/replays/replayitem.py
339-339: Loop control variable team not used within loop body
(B007)
src/downloadManager/__init__.py
500-500: Consider moving this statement to an else block
(TRY300)
src/api/ApiBase.py
176-176: Avoid specifying long messages outside the exception class
(TRY003)
194-194: Avoid specifying long messages outside the exception class
(TRY003)
210-210: Avoid specifying long messages outside the exception class
(TRY003)
230-230: Avoid specifying long messages outside the exception class
(TRY003)
src/heavy_modules.py
11-11: Abstract raise to an inner function
(TRY301)
src/fa/game_updater/updater.py
164-164: Undefined name FilesObtainer
(F821)
🔇 Additional comments (100)
src/replays/replaydetails/replayreader.py (1)
463-464: LGTM! Proper fix for table column alignment.The empty
<td></td>ensures consistent column structure in the HTML table when a player has no country. Without this, missing countries would cause subsequent columns (player name, CPM, enhancements) to shift left, breaking the table layout.src/replays/replayitem.py (1)
6-6: LGTM!The
itemgetterimport is correctly used to support the new sorting-based team highscore logic on line 337.src/notifications/ns_dialog.py (1)
84-85: Good defensive check withisLoaded().The
isLoaded()guard prevents attempting to play a sound that hasn't finished loading, which avoids potential runtime issues.src/chat/channel_tab.py (1)
26-30: Function signature looks good.The
channel_ping_sound(theme)function correctly accepts a theme parameter to allow caching per theme instance.src/fa/game_session.py (2)
44-45: LGTM!The new
ice_poll_startedandice_poll_finishedsignals are correctly defined and provide a clean way to coordinate UI updates during the ICE polling lifecycle.
96-99: LGTM!The signal wiring and emission order is correct: the
ice_poll_finishedhandler is connected beforeice_poll_startedis emitted and before the async request is made, ensuring no race condition where the response arrives before the handler is connected.src/downloadManager/__init__.py (2)
428-430: LGTM!Simple accessor method for exposing the configured image size.
484-484: LGTM!Using
os.scandirto build the index is efficient and avoids eagerly loading all images into memory.src/playercard/ratingtabwidget.py (4)
104-106: LGTM!The connection to
valueChangedand initialization of_default_pagesis correct. The signal connection ensures that changes to the spinbox are propagated to the tab.
118-120: LGTM!Restricting updates to
_default_pagesonly when_current_page == 0prevents mid-loading changes from disrupting an ongoing pagination session, which is sensible behavior.
130-133: LGTM!The logic correctly advances
_default_pagesby one when the tab is already loaded, enabling incremental page loading via the "load next" button.
211-217: LGTM!The
default_pages_boxis correctly passed to eachRatingsPlotTabduring setup.src/api/ApiBase.py (4)
49-50: LGTM!The
_do_nothinghelper provides a clean no-op default for optional handlers.
53-58: LGTM!The refactored
JsonApiBasecorrectly delegates network operations to the sharedApiAccessManagerInstance, centralizing request handling.
150-158: LGTM!The
ApiAccessManagerproperly encapsulates network access management with handler tracking. Theoauthclass attribute binding at class level is acceptable for this singleton pattern.
266-266: LGTM!The module-level singleton
ApiAccessManagerInstanceis appropriate for shared network access across the application.res/client/client.ui (1)
322-343: LGTM!The rename from
labelAutomatchInfotolabelStatusBarInfoappropriately reflects its broader purpose of displaying various status messages (ICE polling, game launching, etc.) rather than just automatch-specific information.src/games/_gameswidget.py (1)
252-253: LGTM!The label reference update from
labelAutomatchInfotolabelStatusBarInfocorrectly aligns with the UI resource rename inres/client/client.ui.src/fa/game_process.py (1)
198-200: LGTM!The
kill_all_if_runningutility function provides a clean single entry point to terminate both game and replay processes, simplifying cleanup operations.src/fa/__init__.py (2)
14-14: LGTM!The import correctly exposes
kill_all_if_runningfrom the submodule.
24-24: LGTM!Properly added to
__all__for public API exposure.src/vaults/mapvault/mapdetails.py (1)
36-46: LGTM!The migration to
get_by_query_parsedwith callback-based handling and updatedParsedApiResponsetype annotation aligns well with the broader API refactoring. The implementation correctly accessesresponse["data"]which matches theParsedApiResponsestructure.Also applies to: 55-58
src/vaults/modvault/modvault.py (1)
27-48: LGTM!The refactoring to a
setup()method follows the lazy initialization pattern consistently applied across vault modules. Thesuper().setup()call correctly precedes module-specific initialization.src/replays/replaydetails/tabs/gamestats.py (2)
12-13: LGTM!The lazy loading of
numpyandpyqtgraphviasrc.heavy_modulesis a good performance optimization for deferred import of heavy dependencies.
153-153: Theitemattribute exists and is properly exposed byLabeledBarGraphItem.The class correctly assigns
self.item = pg.BarGraphItem(**opts)in__init__(line 18 of labeledbargraphitem.py), exposing the underlying pyqtgraph item for all usage sites. All seven occurrences in gamestats.py (lines 153, 190, 227, 285, 310, 343, 366) correctly access this attribute.src/vaults/vault.py (1)
39-42: LGTM!The lazy initialization pattern with
_loadedflag and deferredsetup()is a solid performance optimization. UI setup is correctly deferred until the vault is first shown viashowEvent→busy_entered.Also applies to: 258-261
src/playercard/statistics.py (3)
6-7: LGTM!Consistent use of lazy-loaded heavy modules.
50-50: LGTM!Consistent
.itemaccess pattern for bothLabeledBarGraphItemandPieChartItem.Also applies to: 89-89
97-102: Good defensive improvement.The
Generator[pg.PlotWidget]type simplification is valid, and theif player_event.event is not Noneguard at line 101 preventsAttributeErrorwhen accessing.xdonNone.src/vaults/detailswidget.py (2)
18-18: LGTM!Type annotation updated to
ParsedApiResponsealigning with the API layer refactoring.Also applies to: 91-92
379-391: LGTM!The
on_reviews_datahandler correctly processes the parsed response structure. The assumption thatmessage["data"]is a dict with a"versions"key is valid given it's fetching a single item by ID at line 415.src/vaults/reviewwidget.py (2)
317-318: LGTM: Rating initialization correctly handles both new and edit flows.The
_ratingand_hovered_ratingnow initialize fromself.review.score, which correctly handles:
- New reviews: score is 0 (set in line 314)
- Edit reviews: score is the existing review's score
352-354: Verify consistency between_ratingandreview.scorefor button state.Line 354 enables the submit button based on
self.review.score > 0, while line 373 in the event handler usesself._rating > 0. Ifupdate_appearance()is called after the user selects a rating but before submitting, it might incorrectly disable the button based on the oldreview.scorerather than the current_rating.Consider using
self._ratingconsistently:def update_appearance(self) -> None: - self.star_rating.set_rating(self.review.score) - self.ui.submitButton.setEnabled(self.review.score > 0) + self.star_rating.set_rating(self._rating) + self.ui.submitButton.setEnabled(self._rating > 0) self.ui.commentTextEdit.setText(self.review.text)However, verify whether
update_appearance()is intended to reset the UI to the saved review state, in which case the current logic may be correct.src/coop/_coopwidget.py (2)
53-54: LGTM: Clear separation of initialization states.The two flags (
ui_loadedandscenarios_loaded) provide fine-grained control over initialization stages, supporting the lazy-loading pattern effectively.
146-152: LGTM: Lazy initialization pattern correctly implemented.The method properly guards UI initialization and scenario loading with flags, ensuring each happens exactly once. The pattern supports the PR's lazy-loading objectives.
src/api/coop_api.py (2)
14-18: LGTM: Conversion follows established pattern.The
convert_parsedimplementation correctly constructsCoopScenarioobjects from parsed data, consistent with similar implementations infeatured_mod_api.pyandstats_api.py.
57-63: LGTM: Result conversion with filtering correctly implemented.The method properly constructs
CoopResultobjects and applies the unique team filter before returning. The pattern aligns with the broader API refactoring.src/api/matchmaker_queue_api.py (1)
13-26: LGTM: Queue data conversion follows the established pattern.The
convert_parsedmethod correctly extracts and transforms matchmaker queue data, consistent with similar implementations across the API layer.src/fa/check.py (1)
121-123: LGTM: Updater composition refactoring improves separation of concerns.The changes correctly separate file obtainment (
FilesObtainer) from update execution (UpdaterWorker), withUpdatercoordinating both. This aligns with the new architecture described in the relevant code snippets.src/vaults/mapvault/mapvault.py (1)
26-28: The code is safe.self.clientis initialized in the parent classVault.__init__()beforesetup()is invoked via thebusy_entered()slot.src/client/_clientwindow.py (4)
62-62: LGTM! Background import initialization looks correct.The
BackgroundImporteris started after UI setup completes, which will trigger lazy loading of heavy modules (numpy, pyqtgraph, scipy) in a background thread without blocking the UI.Also applies to: 886-886
182-182: Good improvement: Now kills both game and replay processes.Using
fa.kill_all_if_running()instead offa.instance.kill_if_running()ensures both the main FA process and any replay process are terminated when the user selects "Kill FA process" from the tray menu.
1810-1811: LGTM! ICE polling lifecycle handlers are well-implemented.The new signal connections and handlers properly update the status bar to inform users about ICE adapter initialization stages ("Getting ICE Servers..." → "Launching ICE Adapter...").
Also applies to: 2173-2179
845-845: Consistent renaming of status bar label throughout.The label has been consistently renamed from
labelAutomatchInfotolabelStatusBarInfoacross all usages, reflecting its broader purpose for general status updates beyond just automatch.Also applies to: 1855-1856, 1866-1867, 1918-1918, 1993-1993, 2034-2034
src/playercard/plot.py (2)
6-8: LGTM! Lazy imports from heavy_modules.Switching to imports from
src.heavy_modulesenables deferred loading of numpy and pyqtgraph, improving startup performance.
63-63: LGTM! More precise type annotations.The updated
np.typing.NDArray[np.float64]type hints are more explicit about the expected array dtype.Also applies to: 138-139, 141-141, 144-144
res/player_card/playercard.ui (1)
354-398: LGTM! Good UX improvement for rating history pagination.The new pagination controls (spin box for page count, "Load next page" button, and renamed "Load all" button) provide users with more control over data loading, which helps work around API restrictions.
src/qt/graphics/labeledbargraphitem.py (2)
16-21: Good refactor: Composition over inheritance.Switching from inheritance to composition by wrapping
pg.BarGraphItemis a cleaner design. The hover event is properly delegated to this wrapper.
23-34: LGTM! Correct data access pattern after composition refactor.The
hoverEventcorrectly accesses bar graph properties viaself.item.optsinstead ofself.opts, and the type annotation for the event parameter is correct.src/heavy_modules.py (2)
8-22: LGTM! Well-implemented lazy import mechanism.The
lazy_importfunction correctly usesimportlib.util.LazyLoaderto defer module loading until first attribute access. The fallback tobackup_nameprovides graceful degradation.The static analysis hint about TRY301 (abstracting raise to inner function) is a minor style preference and can be safely ignored here.
30-38: LGTM! Background importer triggers lazy loading.Using
dir()to trigger module loading is a clever approach. The thread correctly connectsfinishedtodeleteLaterfor cleanup.src/playercard/leagueformatter.py (3)
16-34: LGTM! Good dependency injection for image downloader.Injecting
CachedImageDownloaderinstead of creating it internally allows for sharing the downloader across multiple components, reducing redundant downloads and memory usage.
92-94: LGTM! Consistent image scaling using downloader's configured size.Using
self._downloader.image_size()ensures the scaled pixmap matches the downloader's configuration, maintaining consistency across the application.
108-116: LGTM! Factory properly propagates the injected downloader.The factory function correctly passes
img_dlerto bothGlobalLeagueFormatterandLeagueFormatterconstructors.src/stats/leaderboard_widget.py (1)
209-214: LGTM! Consistent migration to parsed API responses.The widget correctly uses
get_by_query_parsedfor all API calls and handles the parsed responses appropriately.Also applies to: 283-294, 314-314
src/qt/graphics/piechartitem.py (3)
20-32: Good refactor: Composition over inheritance for PieChartItem.The class now wraps a
pg.GraphicsObjectinstead of inheriting from it. TheboundingRectandpaintmethods are correctly patched onto the wrapped object.
53-79: LGTM! Child items correctly parented to wrapped object.All graphical elements (sectors, legend rectangles, text items) are properly parented to
self.item, ensuring they move and render correctly with the wrapped GraphicsObject.
96-104: LGTM! Empty paint method is intentional.The paint method is correctly left empty with an explanatory comment since the sectors are drawn as child items.
src/fa/game_updater/worker.py (2)
248-249: LGTM!The simplified signature accepting a file list directly aligns with the new file-based workflow, removing the previous modname/modversion indirection.
258-274: LGTM!The two-list file flow is well-implemented:
- Correctly unpacks
main_filesandfmod_filesfrom the tuple- Properly branches based on whether
fmod_filesis present (FAF-first update vs single mod update)- Progress signals appropriately reflect the update stage (1/2 vs 2/2 for dual updates, 1/1 for single)
src/playercard/playerinfodialog.py (5)
45-46: LGTM!Settings retrieval and spin box initialization are correctly implemented with appropriate defaults.
64-66: LGTM!The RatingTabWidgetController now receives the pagination controls, enabling the new UI pagination features.
107-118: LGTM on lazy loading logic.The
_loaded_tabsset correctly prevents redundant chart rendering when revisiting tabs. The early return pattern is clean.
122-130: LGTM!Passing the image downloader to
league_formatter_factoryand storing ratings for deferred use is correct.
164-169: LGTM!Settings persistence for
defaultRatingPageson close ensures user preference is retained.src/fa/game_updater/updater.py (4)
95-96: LGTM!The
on_startedmethod provides appropriate UI feedback when file fetching begins.
238-241: LGTM!
FileGroupas aNamedTupleis a clean, lightweight data structure for grouping mod name and version.
243-268: LGTM on FilesObtainer initialization and signal wiring.The logic correctly distinguishes base FAF mods from custom featured mods, setting up the appropriate file groups for sequential fetching.
289-292: LGTM!The
check_finishmethod correctly uses list mutation to track completion and emits the finished signal with the file tuple when all groups are processed.src/api/vaults_api.py (6)
44-44: LGTM!Using
requestDataaligns with the new converted data flow inDataApiAccessor.
78-82: LGTM!The
convert_parsedimplementation correctly buildsModobjects from parsed data and includes meta information.
93-105: LGTM!The
_convert_mapversionhelper cleanly handles the embedded version assignment, andconvert_parsedcorrectly processes the list comprehension withlatestVersionextraction.
123-129: LGTM!The
convert_parsedimplementation correctly buildsMatchmakerQueueMapPoolobjects. Based on learnings, the intentional stateful filter mutation in this class is by design.
138-141: LGTM!Overriding
request_datato useget_by_query_parsedwithdata_ready.emitis correct for the parsed data flow. This maintains backward compatibility while using the new parsing infrastructure.
175-184: LGTM!The handler signature update to
Callable[[dict[str, Any]], None]and switch topost_and_parsecorrectly aligns with the new parsed response flow.src/api/featured_mod_api.py (3)
17-25: LGTM!The
convert_parsedimplementation correctly constructsFeaturedModobjects from parsed API data, andrequest_fmod_by_nameproperly stores the reply reference.
29-36: LGTM!The signals
main_readyandmod_readyenable separate handling of main files vs mod files, andconvert_parsedcorrectly buildsFeaturedModFileobjects.
38-46: Fix falsy version check to handle version 0 correctly.The condition
if versionevaluates toFalseforversion=0, causing it to use"latest"instead of"0". Since the parameter is typed asint | Noneand version fields elsewhere in the codebase can be 0, useif version is not Noneinstead.src/api/stats_api.py (7)
37-44: LGTM!The handler correctly builds
LeaderboardRatingobjects and sorts them by leaderboard order with a safe fallback for null leaderboards.
51-60: LGTM!Using
methodcaller("order")for sorting is a clean and idiomatic approach.
78-89: LGTM!Tracking replies for abort capability is a good addition. The reply is correctly appended before the async operation.
94-100: LGTM on abort implementation.The
try/except RuntimeErrorcorrectly handles the case where the reply's underlying C++ object may have been deleted. Clearing the list after aborting is correct.
134-147: LGTM!Migration to
get_by_query_parsedand buildingLeagueSeasonScoreobjects from parsed data is correct.
161-164: LGTM!PlayerEventApiAccessor correctly migrated to parsed data flow.
179-182: LGTM!PlayerAchievementApiAccessor correctly migrated to parsed data flow.
src/api/ApiAccessors.py (6)
20-29: LGTM!Both
ApiAccessorandUserApiAccessornow correctly inherit fromJsonApiBase, consolidating the API access foundation.
35-79: LGTM on parsed response handlers.The
_parse_and_handlewrapper and the correspondingget_by_query_parsed,get_by_endpoint_parsed, andpost_and_parsemethods form a clean abstraction for working withParsedApiResponse.
81-111: LGTM on converted response handlers.The
_convert_and_handlewrapper combines parsing and conversion in a single step, and theget_by_query_convertedandget_by_endpoint_convertedmethods complete the two-tier API access pattern.
113-118: LGTM!The
parse_messagemethod now correctly returnsParsedApiResponsetype, properly initializing the result structure.
182-196: LGTM!Returning
{"page": {}}as the default for missing meta is a safe pattern. TherequestDatamethod now correctly usesget_by_query_converted.
198-199: LGTM!The abstract
convert_parsedmethod withNotImplementedErrorenforces that concrete subclasses must implement their own conversion logic.src/replays/_replayswidget.py (8)
24-24: LGTM: Import aligns with API refactoring.The
ParsedApiResponseimport supports the new parsed response pattern used inprocess_replays_dataandget_by_query_parsed.
143-144: LGTM: Type annotations improve clarity.The explicit type hints for
_is_onlineare correct and make the method signature more readable.
381-394: LGTM: Well-implemented lazy loading guard.The
_loadedflag pattern prevents redundant appearance setup calls and improves performance. The implementation is clean and correct.
398-432: LGTM: Consistent use of class-level icon constants.The migration from direct icon loading to class-level constants improves performance by avoiding repeated icon loads.
949-953: LGTM: API migration is correct.The migration from
requestDatatoget_by_query_parsedfollows the new parsed response pattern. The callback signature matches the expectedParsedApiResponsetype.
1203-1203: LGTM: Signature matches new API pattern.The updated signature correctly uses
ParsedApiResponse, which is consistent with theget_by_query_parsedcall at line 949.
1267-1292: LGTM: Effective performance optimization.The tab visibility checks prevent unnecessary updates to the local replay tree when it's not visible, improving performance. The
on_tab_changedhandler correctly triggers updates only when the local tab becomes active.
338-340: The refactoring of class-level icon constants is safe and correct.util.THEMEis fully initialized when theutilmodule is imported (which occurs at the top of_replayswidget.py), well before the class definition. Theicon()method does not depend onloadTheme()being called—it works with the default theme immediately upon THEME initialization.Likely an incorrect or invalid review comment.
setup.py (1)
83-86: No issues found — the module path and configuration are correct.The path
scipy_ndimage.ndimage._filtersis intentional, not a typo. This project uses a custom minimized scipy package (minimize_scipy.py) that extracts only the ndimage module to reduce binary size. Thelazy_importinheavy_modules.pytries to load this custom package first and falls back to standard scipy if unavailable. Excludingscipywhile includingscipy_ndimageis the intended design to bundle only necessary components.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/api/sim_mod_updater.py (2)
20-22: Add validation for empty data array.The code assumes
message["data"]is non-empty when accessing[0]. If the API returns an empty data array, this will raise anIndexError. This concern was raised in a previous review and remains unaddressed.
26-27: Error handler signature mismatch will cause runtime crash.
self.ready.emitis passed as the error handler, butready = pyqtSignal()takes no arguments. According to theget_by_query_parsedsignature, the error handler isCallable[[QNetworkReply], None]and will be called with aQNetworkReplyargument. This will raise aTypeErrorwhen a network error occurs.🔎 Proposed fix
+ def _on_error(self, reply: QNetworkReply) -> None: + self.mod_url = "" + self.ready.emit() + def request_and_get_sim_mod_url_by_id(self, uid: str) -> str: query_dict = {"filter": f"uid=={uid}"} - self.get_by_query_parsed(query_dict, self.get_url_from_message, self.ready.emit) + self.get_by_query_parsed(query_dict, self.get_url_from_message, self._on_error) self.wait_loop.exec() return self.mod_urlYou'll also need to add the import for
QNetworkReply:from PyQt6.QtNetwork import QNetworkReply
🧹 Nitpick comments (1)
src/chat/channel_tab.py (1)
109-110: Consider storing the sound effect in a local variable.The previous critical issue (missing
themeparameter) has been resolved. However, callingchannel_ping_sound(self._theme)twice is slightly inefficient, even though the result is cached.🔎 Optional refactor to eliminate duplicate function call
- if channel_ping_sound(self._theme).isLoaded(): - channel_ping_sound(self._theme).play() + sound = channel_ping_sound(self._theme) + if sound.isLoaded(): + sound.play()
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
requirements.txtsrc/api/sim_mod_updater.pysrc/chat/channel_tab.pysrc/downloadManager/__init__.pysrc/playercard/playerinfodialog.pysrc/playercard/plot.py
🚧 Files skipped from review as they are similar to previous changes (2)
- requirements.txt
- src/playercard/plot.py
🧰 Additional context used
🧬 Code graph analysis (3)
src/api/sim_mod_updater.py (1)
src/api/ApiAccessors.py (2)
DataApiAccessor(32-199)get_by_query_parsed(43-53)
src/chat/channel_tab.py (1)
src/util/theme.py (3)
theme(142-143)sound(118-120)sound(355-357)
src/playercard/playerinfodialog.py (3)
src/downloadManager/__init__.py (1)
CachedImageDownloader(480-513)src/config/__init__.py (2)
Settings(43-207)group(51-56)src/playercard/leagueformatter.py (1)
league_formatter_factory(108-116)
🪛 Ruff (0.14.10)
src/downloadManager/__init__.py
501-501: Consider moving this statement to an else block
(TRY300)
🔇 Additional comments (10)
src/api/sim_mod_updater.py (2)
3-4: LGTM!The PyQt6 imports are correct for the signal and event loop functionality introduced.
12-18: LGTM!The signal-to-QEventLoop pattern is a standard Qt approach for synchronous waiting. The connection ensures the loop exits when
readyis emitted.src/playercard/playerinfodialog.py (4)
45-46: LGTM! Settings initialization and persistence look correct.The default rating pages value is properly read from settings with a sensible default of 10, and line 169 correctly persists it on close. The flow is consistent.
86-93: LGTM! Attribute initialization is well-structured.The leagues image downloader and state tracking attributes are properly initialized. The previous typo (
leauges_img_dler) has been corrected toleagues_img_dler.
120-130: LGTM! Rating processing correctly wires up the image downloader.The league formatter factory receives the new
leagues_img_dlerdependency as expected, and ratings are stored for chart rendering in the stats tab.
163-169: LGTM! State storage and settings persistence are correctly implemented.Player events are stored for later chart rendering, and the rating pages preference is properly persisted on dialog close using the settings group context.
src/downloadManager/__init__.py (4)
428-430: LGTM! Simple accessor for image size.Clean addition providing read access to the configured size.
496-503: LGTM! Previous scaling bug is fixed.The method now correctly creates, caches, and returns the scaled pixmap. The earlier issue where an unscaled pixmap was returned while caching a scaled one has been resolved.
The static analysis hint (TRY300) about moving the return to an
elseblock is a stylistic preference; the current structure is clear and correct.
510-513: LGTM! In-memory caching on save is correctly implemented.Newly downloaded images are properly added to the in-memory cache with appropriate scaling, ensuring consistency with the lazy-load path.
484-484: The cache directories (AVATARS_CACHE_DIRandDIVISIONS_CACHE_DIR) are created at module import time insrc/util/__init__.py(lines 182-191) before anyCachedImageDownloaderinstances are instantiated. Theos.scandir(self.save_dir)call will not raiseFileNotFoundError.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/replays/_replayswidget.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/replays/_replayswidget.py (5)
src/api/ApiBase.py (1)
ParsedApiResponse(38-40)src/model/playerset.py (1)
get_by_name(19-22)src/replays/models.py (1)
icon(123-124)src/util/theme.py (1)
icon(369-409)src/api/ApiAccessors.py (1)
get_by_query_parsed(43-53)
🔇 Additional comments (8)
src/replays/_replayswidget.py (8)
24-24: LGTM!The import of
ParsedApiResponseimproves type safety for API response handling.
143-144: LGTM!The type annotation correctly reflects that this method takes a string and returns a boolean.
348-348: LGTM! Idempotent lazy loading correctly implemented.The
_loadedflag properly guards against redundant_setup_appearancecalls, and the early return at lines 380-381 ensures the method is idempotent. This addresses the previous review comment about dead code, as the problematicif self.isExpanded()check has been removed.Also applies to: 379-392
396-396: Icon constant usage is consistent.The usage of
self.NO_METADATA,self.BROKEN, andself.UNKNOWNis appropriate and consistent across all_setup_*methods. However, this depends on the class-level icon initialization issue (lines 338-340) being addressed.Also applies to: 401-401, 410-410, 430-430
604-604: LGTM! Lazy loading on expansion correctly implemented.The
on_item_expandedhandler appropriately triggers_setup_appearancefor child items when a bucket is expanded, enabling lazy loading of replay appearance data. The cast is safe since bucket items only containLocalReplayItemchildren, and the idempotent_setup_appearanceprevents redundant work.Also applies to: 606-608
947-951: LGTM! API call correctly updated to use parsed response.The transition to
get_by_query_parsedwith typed callbacks aligns with the new API architecture and improves type safety.
1201-1201: LGTM! Type annotation correctly updated.The signature change to accept
ParsedApiResponseimproves type safety and aligns with the refactored API layer.
1265-1265: LGTM! Performance optimizations appropriately applied.The visibility checks and tab-change handler prevent unnecessary UI updates when the local replays tab is not active, improving performance without affecting functionality.
Also applies to: 1277-1278, 1283-1284, 1288-1290
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/replays/_replayswidget.py (1)
616-620: Consider defensive type checking.The cast to
LocalReplayItemis safe based on the current code structure (buckets only containLocalReplayItemchildren per line 702), but adding a runtime type guard would make this more defensive against future changes.🔎 Optional defensive pattern
def on_item_expanded(self, item: LocalReplayBucketItem) -> None: for index in range(item.childCount()): - cast(LocalReplayItem, item.child(index))._setup_appearance() + child = item.child(index) + if isinstance(child, LocalReplayItem): + child._setup_appearance()
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/replays/_replayswidget.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/replays/_replayswidget.py (6)
src/api/ApiBase.py (1)
ParsedApiResponse(38-40)src/replays/models.py (1)
icon(123-124)src/util/theme.py (1)
icon(369-409)src/fa/maps.py (1)
preview(324-367)src/model/playerset.py (1)
get_by_name(19-22)src/api/ApiAccessors.py (1)
get_by_query_parsed(43-53)
🔇 Additional comments (10)
src/replays/_replayswidget.py (10)
338-350: LGTM! Icon lazy loading properly implemented.The module-level cached functions elegantly address the previous concern about eager class-level icon initialization. The
@cachedecorator ensures icons are loaded only on first access and cached thereafter.
144-145: LGTM! Online status check improved.The explicit signature and use of
get_by_namewith a None check is clearer and more type-safe than the previous membership test.
360-360: LGTM! Lazy loading flag initialized.The
_loadedflag properly supports the deferred appearance setup pattern.
391-404: LGTM! Idempotent setup pattern implemented correctly.The early return when
_loadedis True ensures_setup_appearancecan be safely called multiple times without redundant work. This addresses the previous review concern about dead code in__init__and enables efficient lazy loading.
959-963: LGTM! API call aligned with refactored interface.The switch to
get_by_query_parsedcorrectly adapts to the new API layer that returns parsed responses. This aligns with the broader API refactoring mentioned in the PR objectives.
1213-1213: LGTM! Handler signature updated correctly.The parameter type change to
ParsedApiResponseis correct and type-safe. The handler correctly accesses thedatakey (line 1217) which is guaranteed to exist in the TypedDict.
1277-1277: LGTM! Tab change wiring added.Connecting to
currentChangedenables efficient updates only when the local replays tab becomes active.
1300-1302: LGTM! Tab-specific update logic.The index check ensures the local replay tree is only updated when its tab is actually selected, avoiding unnecessary work.
1289-1290: LGTM! Visibility guard in focusEvent.The visibility check prevents unnecessary tree updates when the local replays tab isn't visible, improving performance.
1295-1296: LGTM! Visibility guard in showEvent.Consistent with the focusEvent logic, this ensures updates only occur when the local replays tab is actually visible.
Gatsik
force-pushed
the
perf
branch
2 times, most recently
from
b78173f to
a95dc95
Compare
* Map/Mod vaults * QSoundEffects * local replay items * CoopWidget
to improve application start time
one day we'll do it for API too
rather fill cache on demand
so they share cache
they may be not opened at all, and it reduces default RAM consumption
so they all also share proxy and cache configurations Qt docs say that one QNetworkAccessManager instance should be enough for the whole Qt application, so maybe one day we'll arrive there Game updater now fetches lists of files to update in the main thread with FileObtainer, because QNetworkAccessManager is based on QObject and can only be used from the thread it belongs to
(e.g. only parsing result into dictionary, or parsing and converting further to pydantic API models) this way it is more clear what kind of result will be returned from the request
don't assume any base value as a minimum score because a team can win with total of 0 (and less??) score, e.g: -------------------------- |team 1 | team 2 (winner)| |-------+----------------- |-10 | -10 | |-10 | 10 | --------------------------
display information about getting ICE servers from the API -- it can be noticeably long (a couple of seconds), so the client doesn't look like it's doing nothing for no reason
they are almost all trivial and not reused
* QSpinBox to change default amount of pages to load * QPushButton to load next page
wether this has any effect was not investigated but Qt docs recommend this approach
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/player_api.py (1)
42-50: Add defensive checks for missing data.The implementation has two potential issues:
Line 43: The unpacking
player_dict, = message["data"]will raiseValueErrorif the API returns zero players (player not found) or multiple players.Line 45: Accessing
player_dict["clanMembership"]["clan"]could raiseKeyErrorif the"clanMembership"key is missing fromplayer_dict.Consider adding validation to handle these edge cases gracefully.
🔎 Proposed fix
def handle_player(self, message: PreProcessedApiResponse) -> None: - player_dict, = message["data"] + data = message["data"] + if not data: + logger.warning("Player not found in API response") + return + if len(data) > 1: + logger.warning("Multiple players returned, expected one") + player_dict = data[0] + player = Player(**player_dict) - if "clan" in player_dict["clanMembership"]: + if "clanMembership" in player_dict and "clan" in player_dict["clanMembership"]: clan = Clan(**player_dict["clanMembership"]["clan"]) clan_membership = ClanMembership(**player_dict["clanMembership"]) clan_membership.custom_clan = clan player.custom_clan_membership = clan_membership self.player_ready.emit(player)
♻️ Duplicate comments (2)
src/fa/game_updater/updater.py (1)
164-164: Forward reference is acceptable for Python 3.14+.The static analysis warning about
FilesObtainerbeing undefined is a false positive given the project's Python 3.14+ target, where PEP 649 provides deferred annotation evaluation by default. Based on learnings, this pattern has been discussed and confirmed intentional.Based on learnings, the forward reference pattern is intentional for Python 3.14+.
src/api/ApiAccessors.py (1)
198-199: Template method requires subclass implementation.This method raises
NotImplementedErrorand must be implemented by all subclasses that use the converted response methods. See the critical issue comment on lines 81-88 for details and verification script.
🧹 Nitpick comments (9)
src/chat/channel_tab.py (1)
109-110: Consider storing the cached sound in a local variable.While the code works correctly due to caching, calling
channel_ping_sound(self._theme)twice in succession is slightly redundant. Storing the result improves readability and makes the intent clearer.🔎 Proposed refactor
- if channel_ping_sound(self._theme).isLoaded(): - channel_ping_sound(self._theme).play() + sound = channel_ping_sound(self._theme) + if sound.isLoaded(): + sound.play()src/replays/replayitem.py (1)
337-342: Approve the sort-based tie handling; consider minor refinements.The sort-based approach with tie detection is correct and more explicit than iterative max-detection. The logic properly handles multi-team scenarios and sets
teamWintoNonewhen ties are detected.Two minor refinements to consider:
Unused loop variable: As flagged by Ruff, the
teamvariable in line 339 is unused. Consider using_to indicate this is intentional:for _, score in sorted_scores[1:]:Variable naming:
mvtstores the top score value but the name suggests a team reference. Consider renaming for clarity:self.teamWin, top_score = sorted_scores[0] for _, score in sorted_scores[1:]: if score >= top_score:🔎 Combined refactor
sorted_scores = sorted(scores.items(), key=itemgetter(1), reverse=True) -self.teamWin, mvt = sorted_scores[0] -for team, score in sorted_scores[1:]: - if score >= mvt: +self.teamWin, top_score = sorted_scores[0] +for _, score in sorted_scores[1:]: + if score >= top_score: self.teamWin = None breakBased on static analysis hints.
res/chat/channel.css (1)
108-113: Redundantfont-style: normaldeclaration.Line 112 sets
font-style: normalalongsidefont-weight: bold. Thefont-styleproperty controls italic/oblique/normal, not boldness, so setting it tonormalwhenfont-weight: boldis already specified is redundant.🔎 Suggested simplification
span.my_mention { font-family: 'Segoe UI'; font-size: 9pt; font-weight: bold; - font-style: normal; color: "{{ colors['you'] }}"; }src/api/ApiBase.py (1)
92-147: Note the different response handler signatures.The method signatures show intentional differences in response handling:
get_by_queryandget_by_endpoint: handlers receive decodedApiResponsepost: handler receives decodedApiResponsedeleteandpatch: handlers receive rawQNetworkReplyThis asymmetry suggests different use cases, but consider whether a consistent interface would improve maintainability. If the differences are intentional, consider documenting why
deleteandpatchhandlers work with raw replies instead of decoded responses.src/vaults/reviewwidget.py (1)
345-354: Minor duplication in submit button state logic.The submit button's enabled state is set both in
init_ui(line 349) andupdate_appearance(line 354). Sinceinit_uicallsupdate_appearanceat line 350, line 349 is redundant.🔎 Optional simplification
def init_ui(self) -> None: self.ui.setupUi(self) self.ui.ratingLayout.addWidget(self.star_rating) self.ui.submitButton.clicked.connect(self.submit_review) - self.ui.submitButton.setEnabled(self.review.score > 0) self.update_appearance()res/player_card/playercard.ui (1)
356-373: Consider improving the label text flow.The label is split across multiple elements: "Due to API restrictions, max" +
[spinbox]+ "pages are loaded at first. Here are the buttons to try more:". The phrase "Here are the buttons to try more" is slightly awkward.Suggestion: "pages are loaded initially. Use these buttons to load more:" or similar phrasing would read more naturally.
src/replays/_replayswidget.py (1)
691-695: Remove debug print statement.The
🔎 Proposed fix
start = time.perf_counter() for bucket, items in buckets.items(): bucket_item = LocalReplayBucketItem(bucket, items) self.myTree.addTopLevelItem(bucket_item) - print(time.perf_counter() - start) + logger.debug("Bucket rendering took %.3f seconds", time.perf_counter() - start)Or remove entirely if not needed:
- start = time.perf_counter() for bucket, items in buckets.items(): bucket_item = LocalReplayBucketItem(bucket, items) self.myTree.addTopLevelItem(bucket_item) - print(time.perf_counter() - start)src/api/ApiAccessors.py (2)
142-159: Consider simplifying the redundant isinstance check.Line 153 checks
isinstance(message["data"], (dict))which Pyright flags as unnecessary since we've already checked for list on line 148. While this is defensive programming, you could simplify it to just anelse:clause if you trust the API spec.🔎 Optional simplification
if "data" in message: if isinstance(message["data"], (list)): result: list[dict[str, Any]] = [] for data in message["data"]: result.append(self.parseSingleData(data, included)) return result - elif isinstance(message["data"], (dict)): # pyright: ignore[reportUnnecessaryIsInstance] # noqa: E501 + else: return self.parseSingleData(message["data"], included)However, keeping the explicit check provides runtime safety if the API returns unexpected data types.
187-196:requestDatanow depends onconvert_parsedimplementation.The method has been updated to use
get_by_query_converted, which requires subclasses to implementconvert_parsed(see comment on lines 81-88). Ensure all subclasses that userequestDataproperly implement this method.The conditional on lines 193-196 can be simplified:
🔎 Optional simplification
def requestData( self, query_dict: QueryOptions | None = None, error_handler: Callable[[QNetworkReply], None] | None = None, ) -> QNetworkReply: query_dict = query_dict or {} - if error_handler is None: - return self.get_by_query_converted(query_dict, self.data_ready.emit) - else: - return self.get_by_query_converted(query_dict, self.data_ready.emit, error_handler) + error_handler = error_handler or _do_nothing + return self.get_by_query_converted(query_dict, self.data_ready.emit, error_handler)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (66)
requirements.txtres/chat/channel.cssres/client/client.cssres/client/client.uires/player_card/playercard.uisetup.pysrc/api/ApiAccessors.pysrc/api/ApiBase.pysrc/api/coop_api.pysrc/api/featured_mod_api.pysrc/api/matchmaker_queue_api.pysrc/api/parsers/CoopResultParser.pysrc/api/parsers/CoopScenarioParser.pysrc/api/parsers/FeaturedModFileParser.pysrc/api/parsers/FeaturedModParser.pysrc/api/parsers/GeneratedMapParamsParser.pysrc/api/parsers/LeaderboardParser.pysrc/api/parsers/LeaderboardRatingJournalParser.pysrc/api/parsers/LeaderboardRatingParser.pysrc/api/parsers/MapParser.pysrc/api/parsers/MapVersionParser.pysrc/api/parsers/ModParser.pysrc/api/parsers/ModVersionParser.pysrc/api/parsers/PlayerParser.pysrc/api/parsers/ReviewsSummaryParser.pysrc/api/parsers/__init__.pysrc/api/player_api.pysrc/api/sim_mod_updater.pysrc/api/stats_api.pysrc/api/vaults_api.pysrc/chat/channel_tab.pysrc/chat/channel_view.pysrc/client/_clientwindow.pysrc/coop/_coopwidget.pysrc/downloadManager/__init__.pysrc/fa/__init__.pysrc/fa/check.pysrc/fa/game_process.pysrc/fa/game_session.pysrc/fa/game_updater/updater.pysrc/fa/game_updater/worker.pysrc/games/_gameswidget.pysrc/heavy_modules.pysrc/model/chat/chatline.pysrc/notifications/ns_dialog.pysrc/playercard/leagueformatter.pysrc/playercard/playerinfodialog.pysrc/playercard/plot.pysrc/playercard/ratingtabwidget.pysrc/playercard/statistics.pysrc/qt/graphics/labeledbargraphitem.pysrc/qt/graphics/piechartitem.pysrc/replays/_replayswidget.pysrc/replays/replaydetails/replaycard.pysrc/replays/replaydetails/replayreader.pysrc/replays/replaydetails/tabs/gamestats.pysrc/replays/replaydetails/tabs/heatmap.pysrc/replays/replayitem.pysrc/stats/_statswidget.pysrc/stats/leaderboard_widget.pysrc/vaults/detailswidget.pysrc/vaults/mapvault/mapdetails.pysrc/vaults/mapvault/mapvault.pysrc/vaults/modvault/modvault.pysrc/vaults/reviewwidget.pysrc/vaults/vault.py
💤 Files with no reviewable changes (16)
- src/api/parsers/LeaderboardRatingParser.py
- src/api/parsers/ModParser.py
- src/stats/_statswidget.py
- src/api/parsers/ReviewsSummaryParser.py
- src/api/parsers/FeaturedModFileParser.py
- res/client/client.css
- src/api/parsers/CoopResultParser.py
- src/api/parsers/CoopScenarioParser.py
- src/api/parsers/PlayerParser.py
- src/api/parsers/ModVersionParser.py
- src/api/parsers/LeaderboardParser.py
- src/api/parsers/FeaturedModParser.py
- src/api/parsers/LeaderboardRatingJournalParser.py
- src/api/parsers/MapVersionParser.py
- src/api/parsers/GeneratedMapParamsParser.py
- src/api/parsers/MapParser.py
🚧 Files skipped from review as they are similar to previous changes (8)
- src/replays/replaydetails/replayreader.py
- res/client/client.ui
- src/replays/replaydetails/replaycard.py
- src/vaults/mapvault/mapvault.py
- src/fa/game_process.py
- src/api/coop_api.py
- src/notifications/ns_dialog.py
- src/playercard/plot.py
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-12-27T22:19:06.765Z
Learnt from: Gatsik
Repo: FAForever/client PR: 1148
File: requirements.txt:0-0
Timestamp: 2025-12-27T22:19:06.765Z
Learning: Do not propose upgrading the irc package in requirements.txt from 20.4.0 to 20.5.0 due to a known importlib.resources.files() bug in 20.5.0. In reviews of FAForever/client, maintain the pin at 20.4.0 and flag any upgrade discussion for this package as blocked unless there is a documented, validated workaround.
Applied to files:
requirements.txt
📚 Learning: 2025-12-27T22:31:12.379Z
Learnt from: Gatsik
Repo: FAForever/client PR: 1148
File: src/replays/replaydetails/tabs/heatmap.py:39-43
Timestamp: 2025-12-27T22:31:12.379Z
Learning: In src/replays/replaydetails/tabs/heatmap.py, the gaussian_filter() function checks for "scipy_ndimage.ndimage._filters" in sys.modules to support a build optimization. The project uses minimize_scipy.py to create a minimal scipy distribution containing only the gaussian_filter function and dependencies, avoiding shipping the entire scipy package. This sys.modules check is intentional and should not be replaced with hasattr() or other approaches.
Applied to files:
src/heavy_modules.py
📚 Learning: 2025-12-27T22:23:44.249Z
Learnt from: Gatsik
Repo: FAForever/client PR: 1148
File: src/fa/game_updater/updater.py:274-279
Timestamp: 2025-12-27T22:23:44.249Z
Learning: In src/fa/game_updater/updater.py, the single-element unpacking `mod, = data["values"]` in the `process_fmod` method is intentional and serves as validation that exactly one featured mod is returned. Multiple or zero values should raise a ValueError to signal an API error.
Applied to files:
src/fa/check.pysrc/api/sim_mod_updater.pysrc/fa/game_updater/updater.pysrc/fa/game_updater/worker.py
📚 Learning: 2025-12-27T22:30:11.905Z
Learnt from: Gatsik
Repo: FAForever/client PR: 1148
File: src/replays/replaydetails/replaycard.py:89-90
Timestamp: 2025-12-27T22:30:11.905Z
Learning: In the FAForever/client repository, ReplayDetailsCard (src/replays/replaydetails/replaycard.py) is used as a modal dialog with only one instance at a time, so class-level QNetworkAccessManager doesn't cause cross-instance issues.
Applied to files:
src/fa/game_updater/worker.py
📚 Learning: 2025-12-27T22:31:12.379Z
Learnt from: Gatsik
Repo: FAForever/client PR: 1148
File: src/replays/replaydetails/tabs/heatmap.py:39-43
Timestamp: 2025-12-27T22:31:12.379Z
Learning: In src/replays/replaydetails/tabs/heatmap.py, keep the explicit sys.modules check for scipy_ndimage.ndimage._filters around gaussian_filter(), as used by minimize_scipy.py to build a minimal scipy distribution. Do not replace with hasattr() or similar checks; this is intentional to detect the presence of the reduced module for the build optimization.
Applied to files:
src/replays/replaydetails/tabs/heatmap.py
📚 Learning: 2025-11-23T00:07:39.893Z
Learnt from: Gatsik
Repo: FAForever/client PR: 1145
File: src/api/vaults_api.py:114-116
Timestamp: 2025-11-23T00:07:39.893Z
Learning: In src/api/vaults_api.py, the MapPoolApiConnector class uses intentional stateful filter mutation. The request_pool_for_queue method sets self._filters and is the intended public API; request_data() should not be called directly on instances of this class.
Applied to files:
src/api/vaults_api.py
🧬 Code graph analysis (25)
src/vaults/reviewwidget.py (2)
src/api/ApiBase.py (1)
ParsedApiResponse(38-40)src/vaults/detailswidget.py (1)
on_reviews_data(379-391)
src/chat/channel_view.py (1)
src/model/chat/chatline.py (1)
ChatLineMetadata(34-37)
src/fa/check.py (1)
src/fa/game_updater/updater.py (2)
FilesObtainer(243-292)Updater(157-235)
src/fa/__init__.py (1)
src/fa/game_process.py (1)
kill_all_if_running(198-200)
src/client/_clientwindow.py (1)
src/fa/game_process.py (1)
kill_all_if_running(198-200)
src/replays/_replayswidget.py (3)
src/api/ApiBase.py (1)
ParsedApiResponse(38-40)src/model/playerset.py (1)
get_by_name(19-22)src/api/ApiAccessors.py (1)
get_by_query_parsed(43-53)
src/api/sim_mod_updater.py (1)
src/api/ApiAccessors.py (2)
DataApiAccessor(32-199)get_by_query_parsed(43-53)
src/vaults/modvault/modvault.py (2)
src/vaults/mapvault/mapvault.py (1)
setup(26-40)src/vaults/vault.py (1)
setup(41-109)
src/fa/game_session.py (2)
src/chat/socketadapter.py (2)
connect(85-93)connect(124-127)src/model/transaction.py (1)
emit(18-19)
src/vaults/detailswidget.py (3)
src/api/ApiBase.py (1)
ParsedApiResponse(38-40)src/vaults/mapvault/mapdetails.py (1)
allow_review(55-58)src/vaults/reviewwidget.py (1)
on_reviews_data(330-337)
src/api/matchmaker_queue_api.py (6)
src/api/ApiBase.py (1)
ParsedApiResponse(38-40)src/api/coop_api.py (2)
convert_parsed(14-18)convert_parsed(57-63)src/api/featured_mod_api.py (2)
convert_parsed(17-21)convert_parsed(32-36)src/api/stats_api.py (2)
convert_parsed(51-60)convert_parsed(189-193)src/api/vaults_api.py (3)
convert_parsed(78-82)convert_parsed(98-105)convert_parsed(123-129)src/api/ApiAccessors.py (1)
convert_parsed(198-199)
src/playercard/ratingtabwidget.py (5)
src/api/models/LeaderboardRating.py (1)
LeaderboardRating(7-14)src/api/stats_api.py (1)
LeaderboardRatingJournalApiConnector(63-100)src/replays/models.py (1)
rating(87-90)src/playercard/plot.py (1)
PlotController(163-207)src/api/models/Leaderboard.py (1)
pretty_name(14-15)
src/playercard/leagueformatter.py (1)
src/downloadManager/__init__.py (2)
CachedImageDownloader(480-513)image_size(428-429)
src/chat/channel_tab.py (1)
src/util/theme.py (3)
theme(142-143)sound(118-120)sound(355-357)
src/fa/game_updater/worker.py (3)
src/api/models/FeaturedModFile.py (1)
FeaturedModFile(6-15)src/model/transaction.py (1)
emit(18-19)src/fa/game_updater/misc.py (1)
ProgressInfo(32-35)
src/playercard/statistics.py (1)
src/api/models/PlayerEvent.py (1)
PlayerEvent(8-12)
src/api/player_api.py (1)
src/api/ApiAccessors.py (1)
get_by_query_parsed(43-53)
src/model/chat/chatline.py (2)
src/util/magic_dict.py (1)
MagicDict(1-20)src/replays/models.py (1)
login(51-52)
src/playercard/playerinfodialog.py (4)
src/downloadManager/__init__.py (2)
CachedImageDownloader(480-513)run(108-118)src/contextmenu/playercontextmenu.py (1)
PlayerContextMenu(50-284)src/config/__init__.py (2)
Settings(43-207)group(51-56)src/playercard/leagueformatter.py (1)
league_formatter_factory(108-116)
src/replays/replaydetails/tabs/heatmap.py (2)
src/qt/utils.py (1)
block_signals(41-46)src/replays/replaydetails/helpers.py (1)
seconds_to_human(1-9)
src/stats/leaderboard_widget.py (2)
src/api/ApiBase.py (1)
ParsedApiResponse(38-40)src/api/ApiAccessors.py (1)
get_by_query_parsed(43-53)
src/api/ApiBase.py (2)
src/api/ApiAccessors.py (2)
handle(39-40)handle(85-86)src/oauth/oauth_flow.py (1)
OAuth2Flow(25-99)
src/api/vaults_api.py (2)
src/api/ApiBase.py (1)
ParsedApiResponse(38-40)src/api/ApiAccessors.py (2)
requestData(187-196)post_and_parse(67-79)
src/coop/_coopwidget.py (1)
src/coop/coopmodel.py (1)
CoopGameFilterModel(5-14)
src/vaults/mapvault/mapdetails.py (3)
src/api/ApiBase.py (1)
ParsedApiResponse(38-40)src/api/ApiAccessors.py (1)
get_by_query_parsed(43-53)src/vaults/detailswidget.py (1)
allow_review(91-92)
🪛 Ruff (0.14.10)
src/replays/replayitem.py
339-339: Loop control variable team not used within loop body
(B007)
src/heavy_modules.py
11-11: Abstract raise to an inner function
(TRY301)
src/downloadManager/__init__.py
501-501: Consider moving this statement to an else block
(TRY300)
src/fa/game_updater/updater.py
164-164: Undefined name FilesObtainer
(F821)
src/api/ApiBase.py
176-176: Avoid specifying long messages outside the exception class
(TRY003)
194-194: Avoid specifying long messages outside the exception class
(TRY003)
210-210: Avoid specifying long messages outside the exception class
(TRY003)
230-230: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (83)
src/games/_gameswidget.py (1)
252-253: Label reference update looks good.The change from
labelAutomatchInfotolabelStatusBarInfois straightforward and preserves the intended behavior. All references to the old label name have been updated consistently across the codebase—no remaining instances found.src/chat/channel_tab.py (1)
26-30: LGTM! Well-structured cached sound effect.The implementation correctly uses
@cacheto reuseQSoundEffectinstances per theme. The hashability concern flagged in previous reviews has been confirmed as resolved—ThemeSetinherits identity-based hashing fromQObject, making it compatible with the cache decorator. This aligns with the PR's pattern of centralized, cached sound effects.src/replays/replayitem.py (1)
6-6: LGTM!The
itemgetterimport is appropriately used in the new sort-based team winner determination logic.src/coop/_coopwidget.py (3)
53-54: LGTM: Clear state flags for lazy initialization.The new flags
ui_loadedandscenarios_loadedprovide explicit tracking of initialization stages, making the lazy-loading logic easy to understand.
56-104: LGTM: Well-structured lazy initialization.Extracting UI setup into a dedicated
setup()method enables lazy loading and improves startup performance by deferring heavy initialization (widget setup, signal connections, network manager creation) until the widget is first displayed.
223-223: LGTM: Explicit state tracking.Setting
scenarios_loadedafter processing coop scenarios provides clear state management and prevents redundant scenario requests in subsequentbusy_entered()calls (assuming the duplicate request issue is addressed).src/model/chat/chatline.py (2)
34-37: LGTM!The constructor signature change to use
MagicDictis correct and aligns with how the metadata is built and accessed elsewhere in the codebase.
92-98: LGTM!The
my_mentionattribute is correctly set based on thementions_meboolean, providing the user's login string when mentioned orNoneotherwise. This integrates properly with the usage insrc/chat/channel_view.py.src/downloadManager/__init__.py (3)
428-429: LGTM!The
image_size()method provides a clean way to expose the configured image size for external callers.
484-484: LGTM!The disk-backed index using
os.scandiris efficient and correctly maps image names to their file paths for lazy loading.
489-503: LGTM! Previous issue resolved.The lazy loading implementation correctly:
- Checks the memory cache first (line 492)
- Falls back to disk loading (line 494)
- Loads from disk, scales consistently, caches, and returns the same pixmap (lines 499-501)
The previous review concern about returning an unscaled pixmap while caching a scaled one has been properly addressed.
src/api/ApiBase.py (4)
5-5: LGTM! Improved type safety.The type system enhancements provide better type safety:
Literal["page"]restricts meta keys more precisely than generic stringsParsedApiResponseis a clearer name than the previousPreParsedApiResponse- The union type
PreProcessedApiResponseproperly captures both response typesAlso applies to: 35-35, 38-40
237-254: LGTM! Proper error handling and resource cleanup.The request finished handler correctly:
- Routes to the appropriate error or success handler based on reply status
- Cleans up handler registrations
- Schedules reply deletion with
deleteLater()The pattern ensures no memory leaks and proper resource management.
256-263: LGTM! Safe abort implementation.The abort logic correctly:
- Uses
handlers.copy()to avoid mutation during iteration- Wraps abort calls in try/except to handle already-deleted C++ objects
- Provides appropriate error logging
166-179: None checks are necessary and align with Qt documentation.According to Qt documentation for QNetworkAccessManager, while methods normally return a valid QNetworkReply object, failures can occur during application shutdown, threading issues, or internal errors. The documentation explicitly recommends "check for nullptr if your code must be defensive," which is exactly what these methods do by checking for None and raising RuntimeError. These defensive checks are appropriate and should be retained rather than simplified to assertions, which would be disabled in production builds.
Likely an incorrect or invalid review comment.
src/replays/replaydetails/tabs/gamestats.py (2)
12-13: LGTM! Using lazy-loaded heavy modules.The imports from
src.heavy_modulesenable lazy loading of numpy and pyqtgraph, which can improve application startup time by deferring expensive imports until needed.
153-153: LabeledBarGraphItem.item attribute is properly defined and all usages are correct.The
.itemattribute is defined at line 18 ofsrc/qt/graphics/labeledbargraphitem.pyasself.item = pg.BarGraphItem(**opts). All usages across the codebase (gamestats.py lines 153, 190, 227, 285, 310, 343, 366 and statistics.py line 48) correctly access the.itemattribute. No action required.src/fa/__init__.py (1)
14-14: LGTM! Clean API extension.The addition of
kill_all_if_runningto the public API provides a convenient way to kill all running game instances. The export is properly added to both the import statement and__all__.Also applies to: 24-24
setup.py (1)
83-86: The module path"scipy_ndimage.ndimage._filters"is correct. The project uses a custom minimized scipy distribution created byminimize_scipy.py, not the standard scipy package. This selective bundling is intentional to reduce distribution size.Likely an incorrect or invalid review comment.
requirements.txt (1)
1-2: All dependency versions are valid on PyPI.The package upgrades (cx_Freeze 8.5.1, idna 3.11, numpy 2.3.5, pydantic 2.12.5, pyqt6 6.10.1) are all confirmed released on PyPI with no security concerns identified. The irc package remains pinned at 20.4.0 as intended.
src/vaults/vault.py (2)
39-43: LGTM! Clean lazy initialization pattern.The deferred UI setup approach effectively delays expensive
setupUicalls until the vault is actually displayed. The_loadedflag properly guards against redundant initialization.
258-263: Verify thread safety of the lazy initialization.The
_loadedcheck andsetup()call are not atomic. Ifbusy_enteredcan be invoked from multiple threads or re-entered beforesetup()completes, there's a potential race condition. Given this is a Qt slot triggered by UI events, this is likely safe in practice since Qt's event loop is single-threaded, but worth confirming.src/fa/game_session.py (2)
44-45: LGTM! New signals for ICE polling lifecycle.The signals provide hooks for UI components to track ICE server polling status.
92-101: Consider symmetry for Go adapter path.The
ice_poll_started/ice_poll_finishedsignals are only emitted for the Java adapter path. The Go adapter path at line 101 (start_go_process) bypasses this entirely. If the UI relies on these signals for status indication, the Go path may leave the UI in an inconsistent state.Is this asymmetry intentional because the Go adapter handles ICE server discovery differently (internally)?
src/vaults/mapvault/mapdetails.py (2)
36-46: LGTM! Clean migration to parsed API response pattern.The switch from signal-based
requestDatato callback-basedget_by_query_parsedsimplifies the data flow and aligns with the PR's API refactoring.
55-58: LGTM! Correct TypedDict access.The method signature and dictionary access (
response["data"]) correctly match theParsedApiResponseTypedDict definition.src/vaults/reviewwidget.py (2)
317-318: LGTM! Correct initialization from existing review score.When editing an existing review, the rating fields now preserve the original score rather than resetting to zero. For new reviews,
model_constructat line 314 setsscore=0, so the initial state remains correct.
330-337: LGTM! Correct ParsedApiResponse handling.The method properly handles the TypedDict response and updates the review state.
src/replays/replaydetails/tabs/heatmap.py (2)
39-43: LGTM! Intentional sys.modules check for build optimization.Based on learnings, this check supports the
minimize_scipy.pybuild optimization that creates a minimal scipy distribution. Thesys.modulesapproach correctly detects which structure is present.
377-378: LGTM! Correct lazy invocation pattern.The
gaussian_filter()()double-call pattern correctly obtains the implementation at runtime and then invokes it with the image data.res/player_card/playercard.ui (1)
375-399: LGTM! Good pagination UX.The new "Load next page" button alongside "Load all" provides users with granular control over data loading, which is a sensible improvement for handling API restrictions on rating history.
src/playercard/statistics.py (3)
6-7: LGTM! Centralized heavy module imports.Using
npandpgfromsrc.heavy_modulesaligns with the PR's lazy loading strategy for heavy dependencies.
98-102: Good defensive filter for optional event field.The
if player_event.event is not Noneguard correctly handles the case wherePlayerEvent.eventisNone, preventing a potentialAttributeErrorwhen accessingevent.xd.
50-50: The.itemproperty access pattern is correctly implemented.Both
LabeledBarGraphItem(initialized in src/qt/graphics/labeledbargraphitem.py:18) andPieChartItem(initialized in src/qt/graphics/piechartitem.py:30) properly expose the.itemproperty, returning valid pyqtgraph graphics items (pg.BarGraphItemandpg.GraphicsObjectrespectively) suitable for use withplot.addItem().src/fa/check.py (1)
121-124: LGTM! Clean composition pattern for update orchestration.The refactoring separates concerns effectively:
FilesObtainer: handles API requests for featured mod filesUpdaterWorker: handles actual file update operationsUpdater: orchestrates the workflowThis improves testability and follows single responsibility principle.
src/api/matchmaker_queue_api.py (1)
13-26: LGTM!The
convert_parsedimplementation correctly follows the established pattern used by other API connectors in this codebase (e.g.,FeaturedModApiConnector,CoopApiConnector). The method properly extracts queue data and builds the expected output structure.src/vaults/detailswidget.py (1)
18-18: LGTM!The signature updates to use
ParsedApiResponsealign with the broader API refactor pattern visible in related files (mapdetails.py,reviewwidget.py). The data access patterns remain consistent with the expected response structure.Also applies to: 91-92, 379-391
src/heavy_modules.py (1)
8-22: LGTM - Lazy import mechanism is well-implemented.The
lazy_importfunction correctly usesimportlib.util.LazyLoaderto defer module loading. The fallback mechanism withbackup_nameprovides flexibility for development vs. production environments.src/client/_clientwindow.py (3)
182-182: LGTM!Using
fa.kill_all_if_running()instead offa.instance.kill_if_running()correctly ensures both the main game instance and replay instance are terminated, as shown in the relevant snippet fromgame_process.py.
1810-1811: LGTM - ICE polling lifecycle handlers provide good user feedback.The new
on_ice_poll_startedandon_ice_poll_finishedhandlers update the status bar label appropriately during the ICE adapter initialization phase, improving user experience during game launch.Also applies to: 2173-2179
845-845: LGTM!The label rename to
labelStatusBarInfois consistently applied throughout the file, andBackgroundImporter.start()is appropriately called after UI setup is complete.Also applies to: 886-886
src/playercard/leagueformatter.py (2)
16-39: LGTM - Good dependency injection refactor.Injecting
CachedImageDownloaderinstead of creating it internally improves testability and allows consistent sizing across the application. The constructor properly stores and uses the injected downloader.
92-94: LGTM!The image scaling logic correctly handles the optional size from the downloader - scaling only when a size is configured.
src/stats/leaderboard_widget.py (2)
163-167: LGTM!The method signature update to accept
ParsedApiResponsealigns with the broader API refactor. The data flow throughcreateLeaderboardandprocessMetacorrectly uses the parsed response structure.
209-214: LGTM!The migration to
get_by_query_parsedis consistent with the new parsed API response workflow. The f-string usage for filter construction is cleaner and more readable.Also applies to: 283-294, 314-314
src/playercard/playerinfodialog.py (3)
45-46: LGTM!The rating history pagination settings are properly initialized from persisted settings and saved on close, providing good user preference persistence.
Also applies to: 64-67, 169-169
86-93: LGTM!The dependency injection of
CachedImageDownloaderfor league icons and the introduction of instance attributes (player,player_ratings,player_events) for deferred rendering align well with the lazy-loading tab pattern. The typo fix from the past review (leauges→leagues) has been addressed.Also applies to: 120-130, 148-148, 163-164
106-118: Lazy tab loading implementation looks good.The
_loaded_tabstracking prevents redundant rendering on tab revisits. The potential race condition with stats tab (if API data hasn't arrived) was discussed in previous reviews and accepted given the typically fast API responses.src/qt/graphics/labeledbargraphitem.py (1)
16-21: Composition refactor correctly updated—all callers access.item.The class now uses composition instead of inheritance. All callers have been properly updated to use
plot.addItem(bar.item)instead of attempting to add the wrapper directly to a scene.src/fa/game_updater/worker.py (2)
248-249: LGTM! Clean signature simplification.The refactored signature aligns well with the new FilesObtainer-based architecture, moving file acquisition logic out of the worker into a dedicated component.
258-274: LGTM! Clear dual-update flow.The tuple unpacking and conditional logic correctly implement both single-update and FAF-first dual-update paths. The progress signaling properly reflects which update path is being taken.
src/qt/graphics/piechartitem.py (3)
10-11: LGTM! Lazy-loading optimization.Replacing direct imports with heavy_modules proxies defers loading until first use, improving startup performance.
20-32: Verify the composition pattern intent.The class no longer inherits from
pg.GraphicsObjectand instead uses composition by creating an instance and assigning methods to it (lines 31-32). This pattern works but is unconventional—whenself.item.boundingRect()orself.item.paint()are called,selfwill refer to thePieChartIteminstance.Is this intentional to avoid eagerly loading
pg.GraphicsObjectat class definition time?If the goal is lazy loading, an alternative pattern would be to keep inheritance but defer it:
# In heavy_modules.py or similar def _get_graphics_object_class(): import pyqtgraph as pg return pg.GraphicsObject # Then use: class PieChartItem(_get_graphics_object_class()): ...However, if the current composition pattern is working as intended, it's acceptable.
96-100: LGTM! Signature aligns with composition pattern.The
paintmethod signature correctly placesselffirst to align with the method assignment pattern on line 32.src/fa/game_updater/updater.py (3)
188-193: LGTM! Clean separation of concerns.The new wiring clearly separates file acquisition (FilesObtainer) from file processing (UpdaterWorker), improving modularity. The event loop coordination ensures proper sequencing.
243-292: LGTM! Well-structured file acquisition logic.The
FilesObtainerclass cleanly encapsulates the logic for fetching file lists via API connectors and determining whether a dual-update (FAF + featured mod) or single-update flow is needed. The signal-based coordination with the worker is clear.
274-279: Intentional single-element unpacking for validation.Based on learnings, the unpacking
mod, = data["values"]is intentional—it validates that exactly one featured mod is returned. AValueErrorwill correctly signal an API error if zero or multiple values are returned.Based on learnings, this unpacking pattern serves as validation.
src/api/vaults_api.py (4)
78-82: LGTM! Clean model construction.The
convert_parsedmethod explicitly constructsModinstances from parsed data and preserves pagination metadata.
93-105: LGTM! Proper version embedding.The
_convert_mapversionhelper cleanly combines map info with version data, andconvert_parsedapplies it consistently.
123-129: LGTM! Consistent conversion pattern.The
convert_parsedmethod follows the established pattern for converting parsed data to domain models.
175-184: LGTM! Updated to parsed response flow.The
submit_reviewmethod correctly usespost_and_parseand accepts a handler compatible withdict[str, Any].src/replays/_replayswidget.py (6)
143-144: LGTM! Simplified online status check.Using
get_by_namedirectly simplifies the logic and removes the intermediategetIDcall.
375-388: LGTM! Idempotency guard prevents duplicate work.The
_loadedflag ensures_setup_appearanceonly executes once, which is important given the lazy-loading pattern introduced withon_item_expanded.
600-604: LGTM! Lazy loading for tree items.The
on_item_expandedhandler defers appearance setup until a bucket is actually expanded, improving initial rendering performance.
945-949: LGTM! Consistent ParsedApiResponse migration.The update to
get_by_query_parsedaligns with the broader API refactoring.
1199-1199: LGTM! Signature updated for ParsedApiResponse.The method correctly consumes the new
ParsedApiResponsetype.
1275-1288: LGTM! Smart visibility optimization.Checking
myTree.isVisible()before updating avoids unnecessary work when the local replays tab is not active. Theon_tab_changedhandler (line 1286-1288) ensures updates trigger when the tab becomes visible.src/playercard/ratingtabwidget.py (3)
84-106: LGTM! Clean paging control integration.The
default_pages_boxparameter wires the UI spinbox to the tab's paging logic, allowing users to control how many pages load by default. Theon_default_pages_changedhandler ensures the setting only applies before any data is loaded.
130-133: LGTM! Incremental page loading.The
load_next_rating_pagemethod advances_default_pagesby one if already loaded, providing fine-grained control alongsideload_more_ratings(which loads all remaining pages).
191-217: LGTM! Controller wiring for pagination UI.The controller correctly wires the
load_next_buttonand passesdefault_pages_boxto eachRatingsPlotTab, enabling coordinated paging control across the UI.src/api/stats_api.py (5)
37-44: LGTM! Explicit model construction with sorting.The method constructs
LeaderboardRatinginstances and sorts them by leaderboard order, providing a consistent presentation to consumers.
51-60: LGTM! Idiomatic sorting with methodcaller.Using
methodcaller("order")is a clean, functional approach to sorting by the leaderboard's order method.
78-100: LGTM! Request tracking and cancellation.The
replieslist andabortmethod enable proper cleanup of in-flight requests, preventing stale handlers from firing after the connector is torn down.
134-147: LGTM! Consistent ParsedApiResponse migration.Both
get_player_score_in_leaderboardandget_player_scorescorrectly useget_by_query_parsedand constructLeagueSeasonScoreinstances.
161-193: LGTM! Accessor updates consistent.All accessors (
PlayerEventApiAccessor,PlayerAchievementApiAccessor,AchievementsApiAccessor) correctly migrate toget_by_query_parsed.src/api/featured_mod_api.py (2)
17-21: LGTM! Explicit FeaturedMod construction.The
convert_parsedmethod constructsFeaturedModinstances from parsed data, aligning with the broader API refactoring.
29-46: LGTM! Signal-based file retrieval.The new signals (
main_ready,mod_ready) and corresponding methods (get_main_files,get_mod_files) provide a clean, signal-driven API for retrieving featured mod files. Theendpointmethod correctly handles version resolution with a "latest" fallback.src/api/ApiAccessors.py (6)
1-18: LGTM! Imports align with new functionality.The new imports (
Literal,QByteArray,JsonApiBase,ParsedApiResponse,_do_nothing) are all used in the refactored code and support the parsed/converted response workflow.
43-79: LGTM! Parsed response entry points are well-designed.The new
get_by_query_parsed,get_by_endpoint_parsed, andpost_and_parsemethods provide a clean API for working with parsed responses. They properly delegate to existing methods and use the_parse_and_handlewrapper pattern correctly.
89-111: Converted entry points depend onconvert_parsedimplementation.These methods (
get_by_query_convertedandget_by_endpoint_converted) are correctly implemented and follow the same pattern as the parsed variants. However, they depend onconvert_parsedbeing implemented by subclasses (see comment on lines 81-88).
113-118: LGTM!parse_messagecorrectly structures parsed responses.The method properly parses included items, data, and metadata into a well-typed
ParsedApiResponse. Making this public allows consumers to parse responses directly when needed.
182-185: LGTM! Type hint improvement with Literal.The updated signature with
Literal["page"]provides better type safety by constraining the metadata keys. The default return value correctly matches the type constraint.
20-30: The base class change is compatible with all existing subclasses and usages. ApiAccessor, UserApiAccessor, and all descendant classes (DataApiAccessor and its many subclasses) properly initialize and work with JsonApiBase. The host_config_key pattern is correctly implemented, with the base class providing an empty default that subclasses appropriately override.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/vaults/reviewwidget.py (1)
352-358: Inconsistent state management betweenself._ratingandself.review.score.Lines 353-354 use
self.review.score(the persisted value) while line 373 in_handle_mouse_over_star()usesself._rating(the user's draft selection). This creates an inconsistency:
self._ratingtracks the user's current selectionself.review.scoreis the loaded/persisted value- If
update_appearance()is called after the user selects a star, it would reset both the star display and button state to the persisted value, undoing the user's selection visualizationWhile this currently doesn't cause a bug (since
update_appearance()is only called during initialization and data loading), it's fragile. The button enablement and star display should consistently useself._ratingto reflect the user's draft state.🔎 Proposed fix for consistent state management
def update_appearance(self) -> None: - self.star_rating.set_rating(self.review.score) - self.ui.submitButton.setEnabled(self.review.score > 0) + self.star_rating.set_rating(self._rating) + self.ui.submitButton.setEnabled(self._rating > 0) self.ui.commentTextEdit.setText(self.review.text) cursor = self.ui.commentTextEdit.textCursor() cursor.movePosition(cursor.MoveOperation.End) self.ui.commentTextEdit.setTextCursor(cursor)
♻️ Duplicate comments (1)
src/api/ApiAccessors.py (1)
198-199: Subclasses must implementconvert_parsedto use converted methods.As noted in the past review, calling
get_by_query_converted,get_by_endpoint_converted, orrequestDatawill fail at runtime if the subclass doesn't implementconvert_parsed. This is acceptable ifDataApiAccessoris intended as an abstract base requiring subclass implementation.Consider adding
@abstractmethoddecorator and inheriting fromABCto make this contract explicit and catch missing implementations at import time rather than runtime.
🧹 Nitpick comments (3)
src/chat/channel_tab.py (1)
109-110: Consider storing the sound effect in a local variable.The function
channel_ping_sound(self._theme)is called twice. While the@cachedecorator ensures the same object is returned, storing it in a local variable improves readability and eliminates the redundant lookup.🔎 Suggested refactor
- if channel_ping_sound(self._theme).isLoaded(): - channel_ping_sound(self._theme).play() + sound = channel_ping_sound(self._theme) + if sound.isLoaded(): + sound.play()src/replays/replayitem.py (1)
337-342: Improved team winner detection with proper tie handling.The sort-based approach is cleaner and correctly handles ties. However, the loop variable
teamis unused.🔎 Use underscore for unused loop variable
sorted_scores = sorted(scores.items(), key=itemgetter(1), reverse=True) self.teamWin, mvt = sorted_scores[0] - for team, score in sorted_scores[1:]: + for _, score in sorted_scores[1:]: if score >= mvt: self.teamWin = None breaksrc/api/ApiBase.py (1)
174-176: Consider extracting exception message to a constant (optional).The static analysis tool flags long exception messages inline. While the current message is helpful for debugging, you could optionally extract it to a module-level constant if consistency is desired. This is a minor style preference and not blocking.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (66)
requirements.txtres/chat/channel.cssres/client/client.cssres/client/client.uires/player_card/playercard.uisetup.pysrc/api/ApiAccessors.pysrc/api/ApiBase.pysrc/api/coop_api.pysrc/api/featured_mod_api.pysrc/api/matchmaker_queue_api.pysrc/api/parsers/CoopResultParser.pysrc/api/parsers/CoopScenarioParser.pysrc/api/parsers/FeaturedModFileParser.pysrc/api/parsers/FeaturedModParser.pysrc/api/parsers/GeneratedMapParamsParser.pysrc/api/parsers/LeaderboardParser.pysrc/api/parsers/LeaderboardRatingJournalParser.pysrc/api/parsers/LeaderboardRatingParser.pysrc/api/parsers/MapParser.pysrc/api/parsers/MapVersionParser.pysrc/api/parsers/ModParser.pysrc/api/parsers/ModVersionParser.pysrc/api/parsers/PlayerParser.pysrc/api/parsers/ReviewsSummaryParser.pysrc/api/parsers/__init__.pysrc/api/player_api.pysrc/api/sim_mod_updater.pysrc/api/stats_api.pysrc/api/vaults_api.pysrc/chat/channel_tab.pysrc/chat/channel_view.pysrc/client/_clientwindow.pysrc/coop/_coopwidget.pysrc/downloadManager/__init__.pysrc/fa/__init__.pysrc/fa/check.pysrc/fa/game_process.pysrc/fa/game_session.pysrc/fa/game_updater/updater.pysrc/fa/game_updater/worker.pysrc/games/_gameswidget.pysrc/heavy_modules.pysrc/model/chat/chatline.pysrc/notifications/ns_dialog.pysrc/playercard/leagueformatter.pysrc/playercard/playerinfodialog.pysrc/playercard/plot.pysrc/playercard/ratingtabwidget.pysrc/playercard/statistics.pysrc/qt/graphics/labeledbargraphitem.pysrc/qt/graphics/piechartitem.pysrc/replays/_replayswidget.pysrc/replays/replaydetails/replaycard.pysrc/replays/replaydetails/replayreader.pysrc/replays/replaydetails/tabs/gamestats.pysrc/replays/replaydetails/tabs/heatmap.pysrc/replays/replayitem.pysrc/stats/_statswidget.pysrc/stats/leaderboard_widget.pysrc/vaults/detailswidget.pysrc/vaults/mapvault/mapdetails.pysrc/vaults/mapvault/mapvault.pysrc/vaults/modvault/modvault.pysrc/vaults/reviewwidget.pysrc/vaults/vault.py
💤 Files with no reviewable changes (16)
- res/client/client.css
- src/api/parsers/ModParser.py
- src/api/parsers/CoopScenarioParser.py
- src/api/parsers/GeneratedMapParamsParser.py
- src/stats/_statswidget.py
- src/api/parsers/MapParser.py
- src/api/parsers/LeaderboardParser.py
- src/api/parsers/LeaderboardRatingJournalParser.py
- src/api/parsers/MapVersionParser.py
- src/api/parsers/FeaturedModParser.py
- src/api/parsers/ModVersionParser.py
- src/api/parsers/FeaturedModFileParser.py
- src/api/parsers/PlayerParser.py
- src/api/parsers/LeaderboardRatingParser.py
- src/api/parsers/CoopResultParser.py
- src/api/parsers/ReviewsSummaryParser.py
🚧 Files skipped from review as they are similar to previous changes (15)
- src/fa/game_process.py
- src/api/sim_mod_updater.py
- res/client/client.ui
- res/chat/channel.css
- src/coop/_coopwidget.py
- res/player_card/playercard.ui
- src/fa/init.py
- src/replays/replaydetails/replayreader.py
- src/api/player_api.py
- src/games/_gameswidget.py
- src/fa/check.py
- src/fa/game_session.py
- src/chat/channel_view.py
- src/model/chat/chatline.py
- src/vaults/mapvault/mapvault.py
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-12-27T22:19:06.765Z
Learnt from: Gatsik
Repo: FAForever/client PR: 1148
File: requirements.txt:0-0
Timestamp: 2025-12-27T22:19:06.765Z
Learning: Do not propose upgrading the irc package in requirements.txt from 20.4.0 to 20.5.0 due to a known importlib.resources.files() bug in 20.5.0. In reviews of FAForever/client, maintain the pin at 20.4.0 and flag any upgrade discussion for this package as blocked unless there is a documented, validated workaround.
Applied to files:
requirements.txt
📚 Learning: 2025-12-27T22:30:11.905Z
Learnt from: Gatsik
Repo: FAForever/client PR: 1148
File: src/replays/replaydetails/replaycard.py:89-90
Timestamp: 2025-12-27T22:30:11.905Z
Learning: In the FAForever/client repository, ReplayDetailsCard (src/replays/replaydetails/replaycard.py) is used as a modal dialog with only one instance at a time, so class-level QNetworkAccessManager doesn't cause cross-instance issues.
Applied to files:
src/notifications/ns_dialog.pysrc/fa/game_updater/worker.py
📚 Learning: 2025-12-27T22:23:44.249Z
Learnt from: Gatsik
Repo: FAForever/client PR: 1148
File: src/fa/game_updater/updater.py:274-279
Timestamp: 2025-12-27T22:23:44.249Z
Learning: In src/fa/game_updater/updater.py, the single-element unpacking `mod, = data["values"]` in the `process_fmod` method is intentional and serves as validation that exactly one featured mod is returned. Multiple or zero values should raise a ValueError to signal an API error.
Applied to files:
src/fa/game_updater/worker.pysrc/fa/game_updater/updater.py
📚 Learning: 2025-12-27T22:31:12.379Z
Learnt from: Gatsik
Repo: FAForever/client PR: 1148
File: src/replays/replaydetails/tabs/heatmap.py:39-43
Timestamp: 2025-12-27T22:31:12.379Z
Learning: In src/replays/replaydetails/tabs/heatmap.py, keep the explicit sys.modules check for scipy_ndimage.ndimage._filters around gaussian_filter(), as used by minimize_scipy.py to build a minimal scipy distribution. Do not replace with hasattr() or similar checks; this is intentional to detect the presence of the reduced module for the build optimization.
Applied to files:
src/replays/replaydetails/tabs/heatmap.py
📚 Learning: 2025-12-27T22:30:11.905Z
Learnt from: Gatsik
Repo: FAForever/client PR: 1148
File: src/replays/replaydetails/replaycard.py:89-90
Timestamp: 2025-12-27T22:30:11.905Z
Learning: In src/replays/replaydetails/replaycard.py, ReplayDetailsCard is used as a modal dialog with a single instance at a time. Therefore a class-level QNetworkAccessManager does not cause cross-instance issues. If future dialogs could be opened concurrently, prefer an instance-scoped QNetworkAccessManager to avoid shared state across dialogs.
Applied to files:
src/replays/replaydetails/replaycard.py
📚 Learning: 2025-11-23T00:07:39.893Z
Learnt from: Gatsik
Repo: FAForever/client PR: 1145
File: src/api/vaults_api.py:114-116
Timestamp: 2025-11-23T00:07:39.893Z
Learning: In src/api/vaults_api.py, the MapPoolApiConnector class uses intentional stateful filter mutation. The request_pool_for_queue method sets self._filters and is the intended public API; request_data() should not be called directly on instances of this class.
Applied to files:
src/api/vaults_api.py
📚 Learning: 2025-12-27T22:31:12.379Z
Learnt from: Gatsik
Repo: FAForever/client PR: 1148
File: src/replays/replaydetails/tabs/heatmap.py:39-43
Timestamp: 2025-12-27T22:31:12.379Z
Learning: In src/replays/replaydetails/tabs/heatmap.py, the gaussian_filter() function checks for "scipy_ndimage.ndimage._filters" in sys.modules to support a build optimization. The project uses minimize_scipy.py to create a minimal scipy distribution containing only the gaussian_filter function and dependencies, avoiding shipping the entire scipy package. This sys.modules check is intentional and should not be replaced with hasattr() or other approaches.
Applied to files:
src/heavy_modules.py
🧬 Code graph analysis (20)
src/vaults/reviewwidget.py (3)
src/api/ApiBase.py (1)
ParsedApiResponse(38-40)src/replays/models.py (1)
score(48-49)src/vaults/detailswidget.py (1)
on_reviews_data(379-391)
src/playercard/statistics.py (1)
src/api/models/PlayerEvent.py (1)
PlayerEvent(8-12)
src/chat/channel_tab.py (1)
src/util/theme.py (3)
theme(142-143)sound(118-120)sound(355-357)
src/vaults/mapvault/mapdetails.py (3)
src/api/ApiBase.py (1)
ParsedApiResponse(38-40)src/api/ApiAccessors.py (1)
get_by_query_parsed(43-53)src/vaults/detailswidget.py (1)
allow_review(91-92)
src/api/matchmaker_queue_api.py (5)
src/api/ApiBase.py (1)
ParsedApiResponse(38-40)src/api/coop_api.py (2)
convert_parsed(14-18)convert_parsed(57-63)src/api/featured_mod_api.py (2)
convert_parsed(17-21)convert_parsed(32-36)src/api/vaults_api.py (3)
convert_parsed(78-82)convert_parsed(98-105)convert_parsed(123-129)src/api/ApiAccessors.py (1)
convert_parsed(198-199)
src/playercard/leagueformatter.py (1)
src/downloadManager/__init__.py (2)
CachedImageDownloader(480-513)image_size(428-429)
src/notifications/ns_dialog.py (1)
src/util/theme.py (2)
sound(118-120)sound(355-357)
src/fa/game_updater/worker.py (3)
src/api/models/FeaturedModFile.py (1)
FeaturedModFile(6-15)src/model/transaction.py (1)
emit(18-19)src/fa/game_updater/misc.py (1)
ProgressInfo(32-35)
src/api/coop_api.py (4)
src/api/ApiBase.py (1)
ParsedApiResponse(38-40)src/api/ApiAccessors.py (1)
convert_parsed(198-199)src/api/models/CoopScenario.py (1)
CoopScenario(7-13)src/api/models/CoopResult.py (1)
CoopResult(7-14)
src/stats/leaderboard_widget.py (3)
src/api/ApiBase.py (1)
ParsedApiResponse(38-40)src/api/ApiAccessors.py (1)
get_by_query_parsed(43-53)src/api/stats_api.py (1)
filters(116-122)
src/replays/_replayswidget.py (3)
src/api/ApiBase.py (1)
ParsedApiResponse(38-40)src/model/playerset.py (1)
get_by_name(19-22)src/api/ApiAccessors.py (1)
get_by_query_parsed(43-53)
src/vaults/detailswidget.py (3)
src/api/ApiBase.py (1)
ParsedApiResponse(38-40)src/vaults/mapvault/mapdetails.py (1)
allow_review(55-58)src/vaults/reviewwidget.py (1)
on_reviews_data(330-337)
src/api/ApiAccessors.py (1)
src/api/ApiBase.py (10)
ApiResourceObject(25-29)ApiResponse(32-35)JsonApiBase(53-147)ParsedApiResponse(38-40)_do_nothing(49-50)handle(64-65)get_by_query(92-99)get_by_endpoint(101-108)post(110-122)post(181-197)
src/api/stats_api.py (5)
src/api/ApiBase.py (2)
ParsedApiResponse(38-40)get_by_query(92-99)src/api/models/Leaderboard.py (2)
Leaderboard(8-37)order(27-31)src/api/models/LeaderboardRating.py (1)
LeaderboardRating(7-14)src/api/ApiAccessors.py (2)
get_by_query_parsed(43-53)convert_parsed(198-199)src/api/coop_api.py (2)
convert_parsed(14-18)convert_parsed(57-63)
src/client/_clientwindow.py (1)
src/fa/game_process.py (1)
kill_all_if_running(198-200)
src/api/ApiBase.py (1)
src/api/ApiAccessors.py (2)
handle(39-40)handle(85-86)
src/vaults/vault.py (2)
src/vaults/mapvault/mapvault.py (1)
setup(26-40)src/vaults/modvault/modvault.py (1)
setup(27-47)
src/playercard/playerinfodialog.py (4)
src/contextmenu/playercontextmenu.py (1)
PlayerContextMenu(50-284)src/config/__init__.py (2)
Settings(43-207)group(51-56)src/qt/utils.py (1)
center_widget_on_screen(49-54)src/playercard/leagueformatter.py (1)
league_formatter_factory(108-116)
src/api/vaults_api.py (5)
src/api/ApiBase.py (1)
ParsedApiResponse(38-40)src/api/ApiAccessors.py (3)
requestData(187-196)get_by_query_parsed(43-53)post_and_parse(67-79)src/api/models/Mod.py (1)
Mod(9-15)src/api/models/MapVersion.py (2)
Map(37-48)MapVersion(51-73)src/util/__init__.py (1)
decapitalize(527-528)
src/fa/game_updater/updater.py (5)
src/api/featured_mod_api.py (3)
FeaturedModApiConnector(13-25)FeaturedModFilesApiConnector(28-46)request_fmod_by_name(23-25)src/api/models/FeaturedMod.py (1)
FeaturedMod(6-12)src/api/models/FeaturedModFile.py (1)
FeaturedModFile(6-15)src/fa/game_updater/worker.py (2)
UpdaterWorker(39-289)do_update(258-284)src/fa/game_updater/misc.py (2)
UpdaterResult(25-29)log(47-49)
🪛 Ruff (0.14.10)
src/replays/replayitem.py
339-339: Loop control variable team not used within loop body
(B007)
src/downloadManager/__init__.py
501-501: Consider moving this statement to an else block
(TRY300)
src/api/ApiBase.py
176-176: Avoid specifying long messages outside the exception class
(TRY003)
194-194: Avoid specifying long messages outside the exception class
(TRY003)
210-210: Avoid specifying long messages outside the exception class
(TRY003)
230-230: Avoid specifying long messages outside the exception class
(TRY003)
src/fa/game_updater/updater.py
164-164: Undefined name FilesObtainer
(F821)
src/heavy_modules.py
11-11: Abstract raise to an inner function
(TRY301)
🔇 Additional comments (68)
src/notifications/ns_dialog.py (1)
5-5: LGTM! Clean refactoring to module-level cached sound effect.The change from per-instance to module-level cached
QSoundEffectis well-executed. Since notifications are queued and displayed sequentially (confirmed in the previous discussion), a single shared instance is appropriate and avoids unnecessary object creation. TheisLoaded()check provides safety, and this pattern aligns with similar refactorings elsewhere in the codebase.Based on learnings, the single cached instance doesn't cause cross-instance issues for modal dialogs with sequential display.
Also applies to: 17-24, 84-85
src/chat/channel_tab.py (1)
26-30: Verify concurrent playback behavior with shared QSoundEffect.The cached function means all
ChannelTabinstances sharing the same theme will use a singleQSoundEffectinstance. If multiple channels ping simultaneously, callingplay()on an already-playingQSoundEffectwill restart it from the beginning (per Qt documentation), potentially causing sound interruption or missed pings.Confirm this behavior is acceptable compared to the previous implementation where each tab likely had its own sound effect instance.
src/vaults/reviewwidget.py (2)
20-20: LGTM! API type refactoring is consistent.The migration from
PreParsedApiResponsetoParsedApiResponsealigns with the broader API refactoring mentioned in the PR objectives. The usage inon_reviews_data()correctly handles the response structure.Also applies to: 330-330
317-318: LGTM! Initialization now context-aware.Deriving
_ratingand_hovered_ratingfrom the review's score makes the dialog correctly handle both new reviews (score=0) and editing existing reviews (score=existing value).setup.py (1)
83-86: LGTM! Build includes for lazy-loaded heavy modules.The additions correctly ensure
pyqtgraphand the custom minimizedscipy_ndimage.ndimage._filtersare bundled by cx_Freeze. This aligns with the newheavy_modules.pylazy import mechanism and the existingscipyexclusion on line 90.src/vaults/vault.py (2)
39-42: Good lazy initialization pattern for deferred UI setup.The
_loadedflag andsetup()method correctly defersetupUiuntil the vault is first accessed viabusy_entered(). This aligns with the broader performance optimization pattern used consistently acrossMapVaultandModVault.
258-263: Lazy loading trigger implemented correctly.The guard ensures
setup()runs exactly once on first use, then proceeds with the existing pagination logic.src/vaults/modvault/modvault.py (1)
27-29: Consistent lazy setup pattern.The
setup()method correctly delegates tosuper().setup()first, then performs ModVault-specific initialization. This maintains consistency with the vault hierarchy's deferred initialization approach.src/downloadManager/__init__.py (2)
428-430: Good addition of size accessor.The
image_size()method provides read-only access to the configured size, which is useful for consumers that need to know the scaling dimensions.
484-503: Efficient disk-backed lazy loading implementation.The refactored
CachedImageDownloadernow uses a two-tier cache strategy:
_images_indexprovides O(1) path lookup without loading images into memoryget_imagechecks in-memory cache first, then lazy-loads from disk on demandThis avoids the startup cost of loading all cached images while maintaining fast access for frequently used images.
src/heavy_modules.py (2)
8-22: Well-implemented lazy import mechanism.The
lazy_importfunction correctly:
- Attempts to find the primary module spec
- Falls back to
backup_nameif not found- Uses
LazyLoaderto defer actual module loading until first attribute access- Registers the module in
sys.modulesfor subsequent imports
30-38: Background module preloader design is correctly implemented.The
_BackgroundImportertriggers lazy module loading by callingdir()on each module, which forcesLazyLoaderto execute. ThedeleteLaterconnection ensures proper cleanup.BackgroundImporteris instantiated at module import time, andBackgroundImporter.start()is called in the application lifecycle atsrc/client/_clientwindow.py:886, ensuring background loading occurs at the appropriate point after window initialization.src/replays/replaydetails/tabs/gamestats.py (2)
12-13: Imports updated to use lazy-loaded heavy modules.Switching from direct
numpy/pyqtgraphimports tosrc.heavy_modulesenables deferred loading, improving application startup time.
153-153: Allplot.addItem(bar)calls properly updated to usebar.item.The
LabeledBarGraphItemclass correctly exposes the underlyingBarGraphItemvia the.itemproperty (defined insrc/qt/graphics/labeledbargraphitem.py), and all seven locations ingamestats.py(lines 153, 190, 227, 285, 310, 343, 366) consistently use this pattern.requirements.txt (1)
1-7: Dependency updates and new numpy addition look good.The version upgrades (cx_Freeze, idna, pydantic, pyqt6) are all patch-level updates. The new
numpy==2.3.5dependency aligns with the lazy import mechanism inheavy_modules.py. Keepingirc==20.4.0is correct due to the knownimportlib.resources.files()bug in 20.5.0.src/replays/replaydetails/tabs/heatmap.py (2)
39-43: LGTM - Intentional build optimization for minimal scipy distribution.Based on the learnings from prior discussion, this
sys.modulescheck is intentional to support the minimized scipy distribution created byminimize_scipy.py. The explicit check correctly detects whether the reduced module structure is present at runtime.Consider adding a brief comment documenting this intent for future maintainers, as suggested in the prior review.
378-378: Correct usage of the new gaussian_filter helper.The double-call pattern
gaussian_filter()(img, ...)correctly retrieves the callable first, then invokes it with the image data and sigma parameters.src/client/_clientwindow.py (3)
182-182: Good improvement: Now kills both game and replay processes.Per the relevant snippet from
src/fa/game_process.py,kill_all_if_running()terminates bothinstanceandreplay_instance, which is more comprehensive for the tray menu action.
1810-1811: ICE polling signals properly wired to new handlers.The signal connections are correctly placed after the
GameSessionis created, ensuring the UI can respond to ICE adapter lifecycle events.
2173-2179: New ICE polling handlers provide useful status feedback.The handlers appropriately update the status bar to inform users about the ICE adapter initialization stages ("Getting ICE Servers..." → "Launching ICE Adapter..."), improving transparency during game launch.
src/api/ApiBase.py (3)
49-50: Good addition of a shared no-op handler.The
_do_nothinghelper provides a clean default for optional callbacks, avoiding None checks throughout the codebase.
150-158: Well-structured centralized API access manager.The
ApiAccessManagerclass cleanly centralizes OAuth integration and request lifecycle management. Storing handlers keyed byQNetworkReplyis an appropriate pattern for async response routing.
237-254: Correct cleanup of reply handlers.The
on_request_finishedmethod properly:
- Routes to error or success handler based on reply status
- Removes entries from both handler dictionaries
- Schedules reply deletion via
deleteLater()This prevents memory leaks from accumulated handlers.
src/api/vaults_api.py (2)
138-141: Correct migration to parsed response flow.
ReviewsApiConnector.request_datanow usesget_by_query_parsedand directly passesself.data_ready.emitas the response handler, which correctly emits the parsed response for downstream consumers.
93-96: [rewritten review comment]
[classification tag]src/api/ApiAccessors.py (3)
35-41: Clean closure pattern for response transformation.The
_parse_and_handlemethod creates a closure that wraps the user's handler, applyingparse_messagebefore invoking it. This is a clean functional approach for the transformation pipeline.
81-87: Conversion pipeline correctly chains parsing and conversion.
_convert_and_handleproperly chainsparse_message→convert_parsed→ handler, ensuring the full transformation pipeline is applied before the handler receives the data.
187-196: requestData correctly routes through the converted pipeline.The method properly uses
get_by_query_convertedand emits results viadata_ready, integrating with the new architecture while maintaining the existing signal-based interface.src/vaults/mapvault/mapdetails.py (2)
36-46: Clean migration to direct callback pattern.The refactor from
requestDatawith signal connection toget_by_query_parsedwith direct callback simplifies the control flow. The query structure and callback are now co-located, improving readability.
55-58: Type annotation correctly updated for ParsedApiResponse.The
allow_reviewmethod signature properly reflects the newParsedApiResponsetype, and the access patternresponse["data"]is correct for the typed dict structure.src/playercard/statistics.py (3)
98-102: Good defensive filtering for None events.The comprehension now correctly filters out
PlayerEventinstances whereeventisNone, preventingAttributeErrorwhen accessingevent.xd. This aligns with the model definition showingevent: Event | None.
97-97: Generator type annotation correctly simplified.The simplified
Generator[pg.PlotWidget]syntax is valid for generators that don't usesend()orreturnvalues, and is cleaner than the full three-parameter form.
50-50:.itemproperty confirmed on both chart item classes.The code correctly accesses
bar.itemandpie.item.LabeledBarGraphIteminitializes.itemaspg.BarGraphItem(**opts)(src/qt/graphics/labeledbargraphitem.py:18) andPieChartIteminitializes.itemaspg.GraphicsObject(parent_item)(src/qt/graphics/piechartitem.py:30). Both properties are available and correctly expose the underlying graphics items for the plot.src/api/matchmaker_queue_api.py (2)
13-26: Follows established convert_parsed pattern.The implementation correctly migrates from
prepare_datatoconvert_parsed, following the same structure as other connectors in the codebase (e.g.,FeaturedModApiConnector,CoopScenarioApiConnector).
17-25: API include parameter guarantees leaderboard presence — defensive handling unnecessary.The code at lines 21 and 23 accesses nested
queue["leaderboard"]data. However, the API connector is always invoked withrequestData({"include": "leaderboard"})insrc/games/automatchframe.py(line 101), which guarantees the leaderboard relationship is included in the response. TheKeyErrorrisk does not apply to the current usage pattern.No defensive handling is required unless the API call contract changes or the method is repurposed for other endpoints without the leaderboard include parameter.
src/replays/replaydetails/replaycard.py (1)
89-90: LGTM - Class-level downloader is appropriate for modal dialog.The class-level
QNetworkAccessManageris acceptable given thatReplayDetailsCardis used as a modal dialog with only one instance at a time.Based on learnings, this architectural pattern prevents cross-instance signal conflicts in the modal dialog use case.
src/vaults/detailswidget.py (1)
18-18: LGTM - Consistent with ParsedApiResponse migration.The type updates from
ApiResponse/PreParsedApiResponsetoParsedApiResponsealign with the broader refactoring to use centralized parsed API responses throughout the codebase.Also applies to: 91-91, 379-379
src/api/coop_api.py (1)
14-18: LGTM - Direct model construction simplifies the code.The refactoring removes trivial parser modules in favor of direct model instantiation (
CoopScenario(**scenario),CoopResult(**result)). Thefilter_unique_teamslogic is correctly preserved in the new flow.Also applies to: 57-63
src/playercard/plot.py (1)
7-8: LGTM - Consistent heavy module proxy adoption.The changes introduce lazy-loaded heavy module proxies (
np,pg) and update type hints accordingly. The simplifiedpg.DateAxisItem()call is the correct public API usage.Also applies to: 63-63, 138-144, 167-167
src/qt/graphics/labeledbargraphitem.py (1)
16-19: LGTM - Composition over inheritance refactoring.The class correctly transitions from inheriting
pg.BarGraphItemto using composition with an internalself.item. Event delegation and data access patterns are properly updated.Also applies to: 23-23, 28-30, 37-37
src/playercard/leagueformatter.py (1)
17-17: LGTM - Proper dependency injection for image downloader.The refactoring injects the image downloader as a dependency instead of creating it internally. The
image_size()delegation and factory updates are consistent with the broader image handling improvements.Also applies to: 21-21, 34-34, 93-94, 112-116
src/stats/leaderboard_widget.py (1)
7-7: LGTM - Complete ParsedApiResponse migration.The file consistently migrates to the parsed API response pattern, updating method signatures, call sites (
get_by_query_parsed), and filter construction (f-strings). All changes are coherent and correct.Also applies to: 163-165, 211-214, 244-245, 285-286, 291-291, 314-314
src/playercard/playerinfodialog.py (1)
86-93: LGTM - Deferred rendering pattern implemented consistently.The changes introduce state management (
player,player_ratings,player_events) and deferred tab rendering with_loaded_tabstracking. The stats tab rendering pattern was previously discussed and deemed acceptable given fast API responses and infrequent access patterns. The defensive check on Line 116-117 (if self.player is not None) appropriately guards against null access.Also applies to: 107-118, 122-130, 148-148, 164-164, 169-169
src/qt/graphics/piechartitem.py (3)
10-11: LGTM: Lazy loading via heavy_modules.The deferred imports for
npandpgalign with the PR's goal to improve startup performance by delaying heavy module initialization.
96-104: LGTM: Paint delegation is correctly implemented.The empty
paintmethod is appropriate since rendering is handled by child items attached toself.item. The updated signature with proper type hints is also good.
20-32: The composition pattern with method delegation works correctly and poses no issues in practice. The codebase explicitly usespie.itemwhen adding to the Graphics View (plot.addItem(pie.item)), and there are noisinstancechecks that would be affected by the wrapper pattern. The delegatedboundingRectandpaintmethods function properly with PyQt6's Graphics View framework, as evidenced by the working implementation and child item parenting.src/fa/game_updater/worker.py (2)
248-249: LGTM: Simplified signature.The refactored signature directly accepts the file list, moving version resolution responsibility to the caller (FilesObtainer). This aligns with the single-responsibility principle.
258-284: LGTM: Two-list update flow is correctly implemented.The logic properly handles two scenarios:
- FAF + featured mod: Updates FAF files first (1/2), then mod-specific files (2/2)
- FAF only: Single update pass (1/1)
Progress signaling correctly reflects the current step in each path.
src/playercard/ratingtabwidget.py (3)
78-120: LGTM: Pagination state management is well-designed.The
on_default_pages_changedguard (lines 118-120) prevents modifying the page count during active navigation, which avoids confusing state transitions. The spin box is properly wired to update_default_pages.
130-133: LGTM: Incremental page loading is correctly implemented.The method increments
_default_pageswhen already loaded, enabling "load next page" functionality. This complements the existing "load all" behavior inload_more_ratings.
191-226: LGTM: Controller pagination wiring is well-structured.The controller properly:
- Centralizes button click handling (line 202)
- Shares the
default_pages_boxacross all tabs (lines 211-217)- Delegates to the active tab (lines 224-226)
This maintains consistent pagination behavior across all rating tabs.
src/replays/_replayswidget.py (5)
143-144: LGTM: Correct usage ofget_by_name.The change from a membership check to calling
get_by_name(name) is not Nonealigns with thePlayersetAPI shown in the relevant snippets.
375-388: LGTM: Idempotent setup pattern correctly implemented.The
_loadedguard (lines 376-378) prevents redundant setup calls, which is important since_setup_appearancecan be triggered from multiple sources (expansion, map preview downloads). Setting_loaded = Trueat the end (line 388) ensures the setup runs exactly once.
600-604: LGTM: Lazy loading on bucket expansion.The
on_item_expandedhandler triggers appearance setup only when a bucket is expanded, deferring expensive operations (icon loading, metadata parsing) until needed. This complements the_loadedguard inLocalReplayItem.
943-947: LGTM: ParsedApiResponse migration is consistent.The switch from
requestDatatoget_by_query_parsed(lines 943-947) and the updatedprocess_replays_datasignature (line 1197) properly adopt the new parsed-response pattern introduced across the API layer.Also applies to: 1197-1207
1261-1286: LGTM: Tab visibility checks optimize UI updates.The
isVisible()guards (lines 1273, 1279) prevent unnecessary tree updates when tabs aren't active. The dedicatedon_tab_changedhandler (lines 1284-1286) ensures the local replays list is refreshed when the user switches to that tab.src/fa/game_updater/updater.py (4)
95-96: LGTM: Progress feedback for update start.The
on_startedmethod provides clear UI feedback when file acquisition completes and the actual update begins.
164-193: LGTM: Threading and signal wiring correctly implemented.The refactored initialization properly:
- Moves the worker to a separate thread (lines 184-186)
- Connects
FilesObtainer.finishedto trigger both UI update and worker execution (lines 189-190)- Sets up an event loop to wait for thread completion (lines 192-193)
Note: The forward reference at line 164 is acceptable given Python 3.14's deferred annotation evaluation, as confirmed in previous comments.
197-212: LGTM: Asynchronous update flow is well-designed.The new flow properly separates file acquisition from the actual update:
- Request phase: Shows "Requesting files from API..." and initiates async file fetching (lines 202-205)
- Update phase: When files arrive,
files_obtainer.finishedtriggersworker.do_update(wired at line 190)- Synchronization: Event loop (line 208) blocks until
worker_thread.finishedemits, ensuring clean shutdownThis design keeps the UI responsive during file fetching.
238-292: LGTM: FilesObtainer logic is correctly implemented.The asynchronous file acquisition flow is well-designed:
Setup: Determines group strategy (lines 255-258)
- Single group for FAF variants
- Dual group (FAF + custom mod) for other mods
Request: Fetches mod metadata for each group (lines 270-272)
Routing: Routes to appropriate file API based on group index (lines 274-279)
Completion: Collects responses and emits when all groups processed (lines 289-292)
The
check_finishpop logic (line 290) correctly handles both single-group and dual-group scenarios.Based on learnings, the single-element unpacking at line 275 is intentional to validate exactly one mod is returned.
src/api/featured_mod_api.py (3)
17-25: LGTM: Domain model conversion is properly implemented.The
convert_parsedhook (lines 17-21) cleanly transforms raw API data intoFeaturedModinstances. Storingself.reply(line 25) enables request tracking for potential cancellation.
28-36: LGTM: Separate signals for main and mod files.The dual-signal approach (
main_ready,mod_ready) allows consumers to distinguish between FAF base files and mod-specific files, which aligns with the two-list update strategy inUpdaterWorker.
38-46: LGTM: File fetching API is clean and well-designed.The
endpointmethod (lines 38-40) appropriately defaults to"latest"when no version is specified. The separateget_main_filesandget_mod_filesmethods (lines 42-46) emit to distinct signals, enabling the consumer to track completion of each file group independently.src/api/stats_api.py (5)
32-44: LGTM: LeaderboardRatingApiConnector properly migrated to ParsedApiResponse.The refactored
handle_player_ratings(lines 39-44) correctly:
- Constructs
LeaderboardRatinginstances from parsed data (line 41)- Sorts by leaderboard order with a safe default for None (line 42)
- Emits the expected
{"values": ratings}structure (line 44)
47-60: LGTM: LeaderboardApiConnector conversion is well-implemented.The
convert_parsedmethod (lines 51-60) properly constructs and sortsLeaderboardinstances. Usingmethodcaller("order")(line 58) is a clean approach for the sorting key.
78-100: LGTM: Reply tracking enables proper cancellation.The addition of
self.replies(line 78) and theabortmethod (lines 94-100) enable proper cleanup of in-flight requests. TheRuntimeErrorexception handling (lines 96-98) gracefully handles already-deleted reply objects.
103-147: LGTM: LeagueSeasonScoreApiConnector properly migrated.The migration to
get_by_query_parsed(lines 134, 147) and the updatedhandle_season_scoressignature (line 136) are consistent with the broader API refactor. Thecastat line 147 is appropriate given the string-joined include parameter.
150-193: LGTM: Remaining connectors consistently migrated to ParsedApiResponse.The updates to
PlayerEventApiAccessor(lines 156-161),PlayerAchievementApiAccessor(lines 173-179), andAchievementsApiAccessor(lines 189-193) all follow the established pattern. The use of anIteratorinAchievementsApiAccessor.convert_parsed(line 192) is a good choice for memory efficiency with potentially large datasets.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/chat/channel_view.py (1)
329-331: Critical: XSS vulnerability - mention parameter must be escaped.The
meparameter (username/mention) is inserted into HTML without escaping. This creates two problems:
- Search mismatch: If
mecontains HTML special characters (e.g.,User<tag>), it won't match in the already-escapedtextwhere it appears asUser<tag>.- XSS injection: The unescaped
meis inserted directly into the HTML span, allowing script injection if usernames contain malicious HTML.🔎 Proposed fix to escape the mention parameter
def _wrap_me(self, text: str, me: str) -> str: - return text.replace(me, f"<span class=\"my_mention\">{me}</span>") + escaped_me = irc_escape(me) + return text.replace(escaped_me, f"<span class=\"my_mention\">{escaped_me}</span>")This ensures:
- The escaped mention is searched in the already-escaped text
- The escaped mention is safely inserted into HTML
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/api/sim_mod_updater.pysrc/chat/channel_view.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-27T22:23:44.249Z
Learnt from: Gatsik
Repo: FAForever/client PR: 1148
File: src/fa/game_updater/updater.py:274-279
Timestamp: 2025-12-27T22:23:44.249Z
Learning: In src/fa/game_updater/updater.py, the single-element unpacking `mod, = data["values"]` in the `process_fmod` method is intentional and serves as validation that exactly one featured mod is returned. Multiple or zero values should raise a ValueError to signal an API error.
Applied to files:
src/api/sim_mod_updater.py
🧬 Code graph analysis (1)
src/api/sim_mod_updater.py (2)
src/api/ApiAccessors.py (2)
DataApiAccessor(32-199)get_by_query_parsed(43-53)src/api/ApiBase.py (1)
ParsedApiResponse(38-40)
🔇 Additional comments (5)
src/chat/channel_view.py (1)
347-351: Verify the integration after fixing the XSS issue.The flow correctly escapes
textat line 343 before passing it to_wrap_meat line 351, and the conditional check formentionis appropriate. However, ensure that thementionparameter is escaped within_wrap_me(as flagged in the separate review comment) for this integration to be secure.src/api/sim_mod_updater.py (4)
3-7: LGTM! Imports support the new asynchronous flow.The added imports for
QEventLoop,pyqtSignal, andParsedApiResponsecorrectly support the refactored async coordination pattern using signals and event loops.
13-13: LGTM! Signal declaration is correct.The
readysignal with no arguments is properly declared and used to coordinate asynchronous completion of the mod URL fetch.
18-19: LGTM! Event loop synchronization pattern is correctly implemented.The initialization of
wait_loopand connection to thereadysignal correctly implements the standard PyQt pattern for coordinating asynchronous operations with blocking waits.
27-28: Error handler correctly implemented; blocking pattern acknowledged.The callback signatures are correct:
- Success handler matches
Callable[[ParsedApiResponse], None]- Error handler lambda correctly accepts
QNetworkReplyand emitsreadyThe blocking pattern using
wait_loop.exec()remains as noted in previous reviews, with the maintainer acknowledging this as a future improvement.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/stats_api.py (1)
120-143: Type hint inconsistency between handlers.
handle_scoreusesdicttype hint (line 120) whilehandle_season_scoresusesParsedApiResponse(line 132). Since both are callbacks forget_by_query_parsed, they should have consistent type annotations for clarity.🔎 Suggested fix
- def handle_score(self, message: dict) -> None: + def handle_score(self, message: ParsedApiResponse) -> None: if message["data"]: self.score_ready.emit(LeagueSeasonScore(**message["data"][0]))
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/api/sim_mod_updater.pysrc/api/stats_api.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-27T22:23:44.249Z
Learnt from: Gatsik
Repo: FAForever/client PR: 1148
File: src/fa/game_updater/updater.py:274-279
Timestamp: 2025-12-27T22:23:44.249Z
Learning: In src/fa/game_updater/updater.py, the single-element unpacking `mod, = data["values"]` in the `process_fmod` method is intentional and serves as validation that exactly one featured mod is returned. Multiple or zero values should raise a ValueError to signal an API error.
Applied to files:
src/api/sim_mod_updater.py
📚 Learning: 2025-12-28T02:07:52.863Z
Learnt from: Gatsik
Repo: FAForever/client PR: 1148
File: src/api/ApiAccessors.py:81-88
Timestamp: 2025-12-28T02:07:52.863Z
Learning: In src/api/ApiAccessors.py, DataApiAccessor.convert_parsed intentionally raises NotImplementedError as a template method pattern - subclasses using get_by_query_converted, get_by_endpoint_converted, or requestData must implement convert_parsed.
Applied to files:
src/api/stats_api.py
🧬 Code graph analysis (1)
src/api/sim_mod_updater.py (2)
src/api/ApiAccessors.py (2)
DataApiAccessor(32-199)get_by_query_parsed(43-53)src/api/ApiBase.py (1)
ParsedApiResponse(38-40)
🔇 Additional comments (9)
src/api/sim_mod_updater.py (4)
3-4: LGTM! Appropriate imports for PyQt6 signal-based async handling.The imports support the new event loop and signal pattern introduced in this file.
7-7: LGTM! Proper type import and signal declaration.The
ParsedApiResponseimport enables correct type annotations, and thereadysignal follows standard PyQt6 class-level signal patterns.Also applies to: 13-13
18-19: Blocking pattern acknowledged by the team.The event loop setup enables synchronous-style waiting. While this will block the calling thread, the developer has acknowledged this trade-off for the current implementation.
31-32: Error handler signature issue resolved.The error handler now correctly accepts the
QNetworkReplyparameter via lambda and emits thereadysignal to prevent hanging. The blocking pattern on line 32 has been acknowledged by the team as an intentional trade-off.src/api/stats_api.py (5)
1-22: LGTM!The import changes correctly align with the migration to
ParsedApiResponse-based flows and the addition of theLeaderboardRatingmodel.
31-43: LGTM!The migration to
get_by_query_parsedis clean, and the handler correctly constructsLeaderboardRatingobjects with proper defensive handling forNoneleaderboard cases in the sort key.
50-59: LGTM!The
convert_parsedimplementation follows the template method pattern correctly. The use ofmethodcaller("order")is clean and efficient.
77-96: Good addition for in-flight request management.The abort mechanism correctly handles Qt's "wrapped C++ object deleted" scenario with the
RuntimeErrorcatch. This is a common and appropriate pattern for Qt objects.One minor observation: the broad
RuntimeErrorcatch could theoretically mask unrelated runtime errors, but in this specific context (callingabort()on aQNetworkReply), it's unlikely to cause issues.
185-189: LGTM!The
convert_parsedimplementation correctly follows the template method pattern. Using a generator expression for lazy evaluation is a good choice for performance, and the return typeIterator[Achievement]is properly annotated.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/playercard/ratingtabwidget.py (1)
118-120: Consider clarifying the conditional update logic.The method only updates
_default_pageswhen_current_page == 0, meaning spin box changes are ignored once loading starts. While this prevents mid-load state inconsistencies, a brief comment explaining this constraint would improve maintainability.🔎 Suggested clarification
def on_default_pages_changed(self, new_default: int) -> None: + # Only update default pages before any loading starts to avoid mid-cycle inconsistency if self._current_page == 0: self._default_pages = new_default
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/playercard/ratingtabwidget.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/playercard/ratingtabwidget.py (3)
src/api/models/LeaderboardRating.py (1)
LeaderboardRating(7-14)src/playercard/plot.py (1)
PlotController(163-207)src/api/models/Leaderboard.py (1)
pretty_name(14-15)
🔇 Additional comments (6)
src/playercard/ratingtabwidget.py (6)
9-9: LGTM: Import additions support pagination UI.The QSpinBox import is necessary for the new pagination control, and the pg import from src.heavy_modules suggests a lazy loading optimization pattern to defer heavy module imports until needed.
Also applies to: 15-15
84-84: LGTM: QSpinBox integration follows PyQt6 patterns correctly.The constructor properly accepts the default_pages_box parameter, stores it, wires its valueChanged signal, and initializes internal state from the box value.
Also applies to: 104-106
130-135: LGTM: Pagination logic correctly manages state transitions.The method properly checks boundaries, manipulates
_default_pagesto enable loading exactly one more page, and integrates correctly with the existing_loadedproperty andload_ratings()guard conditions.
197-197: LGTM: Controller wiring extends pagination UI correctly.The constructor properly accepts the new button and spin box parameters, wires the button click signal, and stores the spin box reference for propagation to child tabs.
Also applies to: 199-199, 204-204, 206-206
213-219: LGTM: Spin box correctly shared across all rating tabs.The setup method properly passes
default_pages_boxto each RatingsPlotTab, allowing a single control to govern pagination defaults across all leaderboard tabs.
226-228: LGTM: Clean delegation to active tab.The method correctly delegates to the currently active tab's
load_next_rating_page(), mirroring the existingload_more_ratings()pattern.
2.4.0 is broken with cx_Freeze because of add_docstring shenanigans
highlighting the whole line with red on the dark background looks unreadable
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/playercard/plot.py (1)
63-63: Type hint pattern is verbose but consistent with proxy approach.The
np.typing.NDArray[np.float64]pattern is more verbose than the standardnpt.NDArray[np.float64](withimport numpy.typing as npt). However, since this aligns with the lazy-loading strategy via heavy_modules proxies, the verbosity is acceptable. Just ensure your type checker handles attribute access through the proxy correctly.Also applies to: 138-139, 141-141, 144-144
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
requirements.txtres/chat/channel.cssres/client/client.csssrc/api/sim_mod_updater.pysrc/api/stats_api.pysrc/chat/channel_view.pysrc/model/chat/chatline.pysrc/playercard/plot.pysrc/playercard/ratingtabwidget.py
💤 Files with no reviewable changes (1)
- res/client/client.css
🚧 Files skipped from review as they are similar to previous changes (3)
- src/chat/channel_view.py
- res/chat/channel.css
- requirements.txt
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-28T16:29:07.806Z
Learnt from: Gatsik
Repo: FAForever/client PR: 1148
File: src/api/sim_mod_updater.py:21-27
Timestamp: 2025-12-28T16:29:07.806Z
Learning: In src/api/sim_mod_updater.py, the assertion `assert isinstance(message["data"], list)` in the `get_url_from_message` method is intentional. If the API returns a dict instead of a list, the application should crash to signal an API contract violation rather than handling it gracefully.
Applied to files:
src/api/sim_mod_updater.py
📚 Learning: 2025-12-27T22:23:44.249Z
Learnt from: Gatsik
Repo: FAForever/client PR: 1148
File: src/fa/game_updater/updater.py:274-279
Timestamp: 2025-12-27T22:23:44.249Z
Learning: In src/fa/game_updater/updater.py, the single-element unpacking `mod, = data["values"]` in the `process_fmod` method is intentional and serves as validation that exactly one featured mod is returned. Multiple or zero values should raise a ValueError to signal an API error.
Applied to files:
src/api/sim_mod_updater.py
📚 Learning: 2025-12-28T02:07:52.863Z
Learnt from: Gatsik
Repo: FAForever/client PR: 1148
File: src/api/ApiAccessors.py:81-88
Timestamp: 2025-12-28T02:07:52.863Z
Learning: In src/api/ApiAccessors.py, DataApiAccessor.convert_parsed intentionally raises NotImplementedError as a template method pattern - subclasses using get_by_query_converted, get_by_endpoint_converted, or requestData must implement convert_parsed.
Applied to files:
src/api/stats_api.py
🧬 Code graph analysis (4)
src/api/sim_mod_updater.py (2)
src/api/ApiAccessors.py (2)
DataApiAccessor(32-199)get_by_query_parsed(43-53)src/api/ApiBase.py (1)
ParsedApiResponse(38-40)
src/model/chat/chatline.py (1)
src/util/magic_dict.py (1)
MagicDict(1-20)
src/playercard/ratingtabwidget.py (2)
src/api/models/LeaderboardRating.py (1)
LeaderboardRating(7-14)src/playercard/playerinfodialog.py (1)
on_tab_changed(106-118)
src/api/stats_api.py (4)
src/api/ApiBase.py (2)
ParsedApiResponse(38-40)get_by_query(92-99)src/api/models/Leaderboard.py (2)
Leaderboard(8-37)order(27-31)src/api/models/LeaderboardRating.py (1)
LeaderboardRating(7-14)src/api/ApiAccessors.py (2)
get_by_query_parsed(43-53)DataApiAccessor(32-199)
🔇 Additional comments (22)
src/model/chat/chatline.py (2)
34-37: LGTM!The type hint correction from
ChatLineMetadatatoMagicDictaccurately reflects the actual parameter type, as seen inget_meta()where aMagicDictinstance is passed at line 66.
92-98: LGTM!The
meta.mentions_me()call correctly retrieves the wrapped boolean throughMagicDict.__call__(), andmy_mentionis safely assigned sinceself._me.loginis guaranteed non-None whenmentions_meevaluates to True.src/api/stats_api.py (8)
3-3: LGTM!The import additions align with the API refactoring.
methodcalleris used for clean sorting inconvert_parsed,ParsedApiResponsereplaces the previous type, andLeaderboardRatingis now instantiated inhandle_player_ratings.Also applies to: 13-13, 17-17
36-43: LGTM!The migration to
get_by_query_parsedis correct, and the handler properly constructsLeaderboardRatingobjects with safe null-checking before accessingleaderboard.order().
50-59: LGTM!The
convert_parsedimplementation follows the established template pattern and cleanly sorts leaderboards usingmethodcaller("order"), which aligns with theLeaderboardmodel's ordering logic.
77-77: LGTM!The addition of request tracking via
self.repliesand the newabort()method provides proper request lifecycle management. TheRuntimeErrorhandling gracefully deals with already-finished replies.Also applies to: 85-85, 90-96
130-136: LGTM!The migration to
get_by_query_parsedis consistent, andhandle_season_scorescorrectly usesParsedApiResponsewith a runtime assertion to narrow the type ofdatafromdict | listtolist.Also applies to: 143-143
157-160: LGTM!The migration to
get_by_query_parsedis correct and consistent with the broader API refactoring.
175-178: LGTM!The migration to
get_by_query_parsedaligns with the API refactoring and correctly handles the parsed response.
185-189: LGTM!The
convert_parsedimplementation correctly follows the template pattern and efficiently uses a generator expression for lazy iteration over achievements.src/playercard/plot.py (1)
167-167: DateAxisItem usage is now correct.The change to
pg.DateAxisItem()properly uses the public API as recommended in the previous review. This is clean and appropriate.src/api/sim_mod_updater.py (3)
3-7: LGTM!The imports are appropriate for the PyQt6-based asynchronous flow with parsed API responses.
13-13: LGTM!The signal declaration and wait_loop setup are correctly implemented for the blocking pattern. The blocking event loop approach was already discussed in previous reviews and acknowledged as acceptable for now.
Also applies to: 18-19
31-32: LGTM!The error handler lambda correctly accepts the
QNetworkReplyparameter and emits thereadysignal with no arguments, matching the signal signature. The blockingwait_loop.exec()pattern was previously discussed and acknowledged as acceptable for now.src/playercard/ratingtabwidget.py (8)
9-9: LGTM!The QSpinBox import is correctly added to support the new pagination control.
84-84: Consider validating the minimum value for default_pages_box.The wiring logic correctly stores the spinbox reference, connects the signal, and initializes
_default_pages. However, ifdefault_pages_box.value()is 0 or negative:
_loadedwould immediately be True (since_current_page == 0and0 >= 0)- Line 143 in
load_ratings()would prevent any data from loading- The tab would remain empty
Ensure that the QSpinBox has a minimum value of at least 1, or add validation to handle this edge case.
Also applies to: 104-106
118-120: LGTM!The condition
if self._current_page == 0correctly ensures that the default page target is only updated before loading begins. This prevents confusing behavior where the target changes mid-flight while pages are already loading. Once a tab starts loading, its target page count is "locked in."
130-135: LGTM! Clever pagination logic.The method correctly implements incremental page loading:
- Returns early if all pages are loaded (Line 131)
- If the default pages are reached, increments
_default_pagesby 1 (Line 134)- Calls
load_ratings()which will now proceed because_loadedbecomes False after the incrementThis provides a "load one more page" behavior, complementing
load_more_ratings()which loads all remaining pages at once.
197-197: LGTM!The controller correctly accepts and wires the new pagination controls:
load_next_buttonis connected to the newload_next_rating_page()methoddefault_pages_boxis stored for passing to eachRatingsPlotTabinstanceThe wiring pattern is consistent with the existing
load_more_buttonhandling.Also applies to: 199-199, 204-204, 206-206
213-219: LGTM!The RatingsPlotTab instantiation correctly includes the
default_pages_boxparameter. All tabs share the same spinbox control, which provides a consistent UX where one control affects pagination behavior across all rating leaderboards.
226-228: LGTM!The method correctly delegates the "load next page" action to the currently active tab, following the same pattern as
load_more_ratings().
15-15: The lazy-loading import is correctly implemented.The
from src.heavy_modules import pgimport at line 15 properly leverages the lazy-loading mechanism insrc/heavy_modules.py. The module uses Python'simportlib.util.LazyLoaderto defer the actual import of pyqtgraph until first use, and the pattern is consistently applied across the codebase.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.