Conversation
📝 WalkthroughWalkthroughThe PR extends the MiSTer resolution picker list with a new ChangesConfiguration and Launch Updates
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 `@scripts/run-macos-mister-core.sh`:
- Around line 19-21: The ZAPAROO_CRT_PREVIEW_SCALE export is ineffective because
the CRT-native path is disabled by removing the --crt flag; restore passing the
--crt flag to the FRONTEND invocation (or add logic to append --crt when
ZAPAROO_CRT_PREVIEW_SCALE is set) so that the scale env var is actually
consumed; update the exec line that invokes "${FRONTEND}" to include the --crt
option (or conditionalize based on ZAPAROO_CRT_PREVIEW_SCALE) to re-enable the
CRT-native path.
- Line 18: The script currently hardcodes ZAPAROO_CORE_ENDPOINT to a private LAN
IP which breaks other environments; change it so the variable is overrideable by
checking for an existing environment value and falling back to a sensible
default (e.g., ws://localhost:7497/api/v0.1) using shell parameter expansion
instead of the fixed IP, and update the comment near the ZAPAROO_CORE_ENDPOINT
declaration to note that callers can override it via the environment.
🪄 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: da96923d-5721-4c0e-97ad-b9c644c6209f
📒 Files selected for processing (2)
rust/frontend/src/models/settings.rsscripts/run-macos-mister-core.sh
| @@ -17,5 +17,5 @@ fi | |||
|
|
|||
| export ZAPAROO_CORE_ENDPOINT="ws://192.168.1.176:7497/api/v0.1" | |||
There was a problem hiding this comment.
Avoid committing a machine-specific core endpoint in the shared runner script.
Line 18 hardcodes a private LAN IP, which will fail for most environments. Make it overrideable with a default so the script works out-of-the-box and still supports local customization.
Suggested fix
-export ZAPAROO_CORE_ENDPOINT="ws://192.168.1.176:7497/api/v0.1"
+: "${ZAPAROO_CORE_ENDPOINT:=ws://127.0.0.1:7497/api/v0.1}"
+export ZAPAROO_CORE_ENDPOINT
export ZAPAROO_CRT_PREVIEW_SCALE=3
exec "${FRONTEND}"📝 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.
| export ZAPAROO_CORE_ENDPOINT="ws://192.168.1.176:7497/api/v0.1" | |
| : "${ZAPAROO_CORE_ENDPOINT:=ws://127.0.0.1:7497/api/v0.1}" | |
| export ZAPAROO_CORE_ENDPOINT | |
| export ZAPAROO_CRT_PREVIEW_SCALE=3 | |
| exec "${FRONTEND}" |
🤖 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 `@scripts/run-macos-mister-core.sh` at line 18, The script currently hardcodes
ZAPAROO_CORE_ENDPOINT to a private LAN IP which breaks other environments;
change it so the variable is overrideable by checking for an existing
environment value and falling back to a sensible default (e.g.,
ws://localhost:7497/api/v0.1) using shell parameter expansion instead of the
fixed IP, and update the comment near the ZAPAROO_CORE_ENDPOINT declaration to
note that callers can override it via the environment.
| export ZAPAROO_CRT_PREVIEW_SCALE=3 | ||
| exec "${FRONTEND}" --crt | ||
| exec "${FRONTEND}" | ||
| # exec "${FRONTEND}" --crt |
There was a problem hiding this comment.
ZAPAROO_CRT_PREVIEW_SCALE is ineffective without --crt, causing a behavior regression.
On Line 20, removing --crt disables the CRT-native path; in that path, Line 19’s ZAPAROO_CRT_PREVIEW_SCALE is actually consumed. This makes the new scale export a no-op and changes startup behavior.
Suggested fix
export ZAPAROO_CORE_ENDPOINT="ws://192.168.1.176:7497/api/v0.1"
export ZAPAROO_CRT_PREVIEW_SCALE=3
-exec "${FRONTEND}"
+exec "${FRONTEND}" --crt
# exec "${FRONTEND}" --crt📝 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.
| export ZAPAROO_CRT_PREVIEW_SCALE=3 | |
| exec "${FRONTEND}" --crt | |
| exec "${FRONTEND}" | |
| # exec "${FRONTEND}" --crt | |
| export ZAPAROO_CRT_PREVIEW_SCALE=3 | |
| exec "${FRONTEND}" --crt | |
| # exec "${FRONTEND}" --crt |
🤖 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 `@scripts/run-macos-mister-core.sh` around lines 19 - 21, The
ZAPAROO_CRT_PREVIEW_SCALE export is ineffective because the CRT-native path is
disabled by removing the --crt flag; restore passing the --crt flag to the
FRONTEND invocation (or add logic to append --crt when ZAPAROO_CRT_PREVIEW_SCALE
is set) so that the scale env var is actually consumed; update the exec line
that invokes "${FRONTEND}" to include the --crt option (or conditionalize based
on ZAPAROO_CRT_PREVIEW_SCALE) to re-enable the CRT-native path.
1 participant
Summary
Really just add the resolutions in the list, seems to go fine
close #155
Summary by CodeRabbit