refactor(news): replace legacy WordPress API with embedded FAF-Newsfeed webview#1162
Open
MrRowey wants to merge 1 commit into
Open
refactor(news): replace legacy WordPress API with embedded FAF-Newsfeed webview#1162MrRowey wants to merge 1 commit into
MrRowey wants to merge 1 commit into
Conversation
…ed webview Migrates the client's news tab away from parsing the old WordPress JSON API and rendering custom PyQt widgets manually. Instead, we now embed the modern FAF-Newsfeed web dashboard directly using QWebEngineView. - Updated `src/news/_newswidget.py` to embed `QWebEngineView` pointing to https://faforever.github.io/FAF-Newsfeed/ - Simplified `src/news/__init__.py` to export only `NewsWidget` - Deleted obsolete files: - `src/news/wpapi.py` (old WordPress API integration) - `src/news/newsmanager.py` (old content management logic) - `src/news/newsitem.py` (old custom Qt item delegate paint rendering) Note: This transition introduces a runtime dependency on `PyQtWebEngine`.
📝 WalkthroughWalkthroughNewsWidget now loads the remote FAF Newsfeed in a PyQt5 QWebEngineView. The previous news item, manager, and WordPress API components are removed, exports are narrowed to NewsWidget, and a standalone web-view harness is added. ChangesNews feed migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant NewsWidget
participant QWebEngineView
participant FAFNewsfeed
NewsWidget->>QWebEngineView: set NEWS_FEED_URL
QWebEngineView->>FAFNewsfeed: load remote page
FAFNewsfeed-->>QWebEngineView: return rendered content
QWebEngineView-->>NewsWidget: notify news loaded
NewsWidget->>stackedWidget: update index when available
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/news/_newswidget.py`:
- Around line 27-35: Update the browser setup in the widget initializer to
connect QWebEngineView.loadFinished to on_news_loaded before calling setUrl, and
update on_news_loaded to transition stackedWidget only when the load succeeds.
Preserve the existing stackedWidget guards and keep the loading page selected
when navigation fails.
- Around line 2-4: Migrate the news widget and isolated test from PyQt5 to
PyQt6: update imports in src/news/_newswidget.py (lines 2-4) and
test_web_isolated.py (lines 2-4), replace app.exec_() with app.exec() in
test_web_isolated.py (lines 25-28), and add the corresponding PyQt6 WebEngine
dependency if QWebEngineView remains in use.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8501b671-ae4a-43a7-90e1-0ef8d25de9bc
📒 Files selected for processing (6)
src/news/__init__.pysrc/news/_newswidget.pysrc/news/newsitem.pysrc/news/newsmanager.pysrc/news/wpapi.pytest_web_isolated.py
💤 Files with no reviewable changes (3)
- src/news/newsmanager.py
- src/news/wpapi.py
- src/news/newsitem.py
Comment on lines
+2
to
+4
| from PyQt5.QtCore import QUrl | ||
| from PyQt5.QtWidgets import QWidget, QVBoxLayout | ||
| from PyQt5.QtWebEngineWidgets import QWebEngineView |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify declared Qt dependencies and binding consistency.
fd -t f 'requirements.*\.txt|pyproject\.toml|setup\.py|setup\.cfg|Pipfile' \
-0 | xargs -0 -r rg -n -i 'pyqt|webengine'
rg -n --type py -C2 'from PyQt[56]|\.exec_\s*\(' \
src test_web_isolated.pyRepository: FAForever/client
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the news widget and the isolated test.
git ls-files 'src/news/_newswidget.py' 'test_web_isolated.py'
printf '\n---\n'
for f in src/news/_newswidget.py test_web_isolated.py; do
if [ -f "$f" ]; then
echo "FILE: $f"
cat -n "$f" | sed -n '1,80p'
echo '---'
fi
doneRepository: FAForever/client
Length of output: 2885
Use PyQt6 throughout the news widget and isolated test
src/news/_newswidget.py and test_web_isolated.py still import PyQt5, and the test harness also uses app.exec_(). Switch both files to PyQt6 and app.exec(). If QWebEngineView stays, add the matching PyQt6 WebEngine dependency.
📍 Affects 2 files
src/news/_newswidget.py#L2-L4(this comment)test_web_isolated.py#L2-L4test_web_isolated.py#L25-L28
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/news/_newswidget.py` around lines 2 - 4, Migrate the news widget and
isolated test from PyQt5 to PyQt6: update imports in src/news/_newswidget.py
(lines 2-4) and test_web_isolated.py (lines 2-4), replace app.exec_() with
app.exec() in test_web_isolated.py (lines 25-28), and add the corresponding
PyQt6 WebEngine dependency if QWebEngineView remains in use.
Comment on lines
+27
to
+35
| self.browser = QWebEngineView(self) | ||
| self.browser.setUrl(QUrl(self.NEWS_FEED_URL)) | ||
|
|
||
| layout.addWidget(self.browser) | ||
|
|
||
| def on_news_loaded(self) -> None: | ||
| self.stackedWidget.setCurrentIndex(1) | ||
| # Keep this trigger alive for the parent manager navigation flow | ||
| if hasattr(self, 'stackedWidget') and self.stackedWidget is not None: | ||
| self.stackedWidget.setCurrentIndex(1) No newline at end of file |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Connect page completion to the stacked-widget transition.
The new browser never invokes on_news_loaded, so a loading page in stackedWidget will remain selected. Connect loadFinished before navigation and avoid revealing a blank browser when loading fails.
Proposed fix
self.browser = QWebEngineView(self)
+ self.browser.loadFinished.connect(self.on_news_loaded)
self.browser.setUrl(QUrl(self.NEWS_FEED_URL))
layout.addWidget(self.browser)
- def on_news_loaded(self) -> None:
+ def on_news_loaded(self, success: bool) -> None:
+ if not success:
+ logger.warning("Failed to load news feed")
+ return
if hasattr(self, 'stackedWidget') and self.stackedWidget is not None:
self.stackedWidget.setCurrentIndex(1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| self.browser = QWebEngineView(self) | |
| self.browser.setUrl(QUrl(self.NEWS_FEED_URL)) | |
| layout.addWidget(self.browser) | |
| def on_news_loaded(self) -> None: | |
| self.stackedWidget.setCurrentIndex(1) | |
| # Keep this trigger alive for the parent manager navigation flow | |
| if hasattr(self, 'stackedWidget') and self.stackedWidget is not None: | |
| self.stackedWidget.setCurrentIndex(1) | |
| self.browser = QWebEngineView(self) | |
| self.browser.loadFinished.connect(self.on_news_loaded) | |
| self.browser.setUrl(QUrl(self.NEWS_FEED_URL)) | |
| layout.addWidget(self.browser) | |
| def on_news_loaded(self, success: bool) -> None: | |
| if not success: | |
| logger.warning("Failed to load news feed") | |
| return | |
| # Keep this trigger alive for the parent manager navigation flow | |
| if hasattr(self, 'stackedWidget') and self.stackedWidget is not None: | |
| self.stackedWidget.setCurrentIndex(1) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/news/_newswidget.py` around lines 27 - 35, Update the browser setup in
the widget initializer to connect QWebEngineView.loadFinished to on_news_loaded
before calling setUrl, and update on_news_loaded to transition stackedWidget
only when the load succeeds. Preserve the existing stackedWidget guards and keep
the loading page selected when navigation fails.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Migrates the client's news tab away from parsing the old WordPress JSON API and rendering custom PyQt widgets manually. Instead, we now embed the modern FAF-Newsfeed web dashboard directly using QWebEngineView.
src/news/_newswidget.pyto embedQWebEngineViewpointing to https://faforever.github.io/FAF-Newsfeed/src/news/__init__.pyto export onlyNewsWidgetsrc/news/wpapi.py(old WordPress API integration)src/news/newsmanager.py(old content management logic)src/news/newsitem.py(old custom Qt item delegate paint rendering)Note: This transition introduces a runtime dependency on
PyQtWebEngine.Summary by CodeRabbit
New Features
Bug Fixes
Refactor