ci: remove unneeded mafiahubservice build and bump xcode#203
Merged
Conversation
WalkthroughTwo CI workflow files are updated to use Xcode ChangesCI Workflow and Test Cleanup
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Kheartz
force-pushed
the
mhs_build_removal
branch
from
6b7e2bd to
015733e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/framework/src/scripting/builtins/events.cpp (1)
708-777:⚠️ Potential issue | 🟠 MajorAdd reserved-event guard to
Events.emitLocalto matchemit/emitToand the TS docs
types/core/events.d.tsstates reserved events (e.g.resourceStart,resourceStop,resourceError,playerConnect,playerDisconnect,playerSpawn,serverStart,serverStop) “cannot be emitted from JS”;Events.emitandEvents.emitToenforce this viaIsReservedEvent.Events::EmitLocalCallback(lines 708-777) does not checkIsReservedEvent(eventName), allowing JS to emit reserved framework events viaemitLocal.- Add the same
IsReservedEventguard toEmitLocalCallback(or clarify in the TS docs thatemitLocalintentionally allows reserved names).🤖 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 `@code/framework/src/scripting/builtins/events.cpp` around lines 708 - 777, Events::EmitLocalCallback currently allows JS to emit framework reserved events; add the same reserved-event guard used by Events::EmitCallback/EmitToCallback by checking IsReservedEvent(eventName) after computing eventName (and before collecting handlers) and throw a JS exception via isolate->ThrowException with a clear message (same wording/behavior as Emit/EmitTo). Locate the check placement inside Events::EmitLocalCallback (after std::string eventName = ...) and mirror the existing pattern used in Emit/EmitTo to ensure reserved names like resourceStart/playerConnect cannot be emitted from JS.
🧹 Nitpick comments (1)
.github/workflows/build-test.yml (1)
42-42: ⚡ Quick winXcode version inconsistency across workflows.
This workflow now uses Xcode
26.3, butupdate_services.ymlstill uses16.2.0(snippet 4, line 110). Consider whether both workflows should use the same Xcode version for consistency.🤖 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 @.github/workflows/build-test.yml at line 42, The build/test workflow shows xcode-version: '26.3' while another workflow uses '16.2.0', causing inconsistency; choose a single Xcode version and make the workflows consistent by either (a) updating the xcode-version key where it's '26.3' to the agreed version (or vice versa), or (b) extract a shared XCODE_VERSION variable (repo-level or workflow-level) and reference it in all workflows so both the xcode-version keys use the same value.
🤖 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 @.github/workflows/build-test.yml:
- Line 42: The CI jobs are pinned to different Xcode versions: update the
xcode-version key in the update_services.yml workflow (the job that currently
uses xcode-version: '16.2.0') to match the build-test.yml pin (xcode-version:
'26.3') to avoid toolchain mismatches; if there's a deliberate reason to keep
the older toolchain, instead add a brief comment in update_services.yml
documenting why it must remain at '16.2.0' and the implications, but the
preferred change is to set xcode-version: '26.3' so both workflows use the same
version.
---
Outside diff comments:
In `@code/framework/src/scripting/builtins/events.cpp`:
- Around line 708-777: Events::EmitLocalCallback currently allows JS to emit
framework reserved events; add the same reserved-event guard used by
Events::EmitCallback/EmitToCallback by checking IsReservedEvent(eventName) after
computing eventName (and before collecting handlers) and throw a JS exception
via isolate->ThrowException with a clear message (same wording/behavior as
Emit/EmitTo). Locate the check placement inside Events::EmitLocalCallback (after
std::string eventName = ...) and mirror the existing pattern used in Emit/EmitTo
to ensure reserved names like resourceStart/playerConnect cannot be emitted from
JS.
---
Nitpick comments:
In @.github/workflows/build-test.yml:
- Line 42: The build/test workflow shows xcode-version: '26.3' while another
workflow uses '16.2.0', causing inconsistency; choose a single Xcode version and
make the workflows consistent by either (a) updating the xcode-version key where
it's '26.3' to the agreed version (or vice versa), or (b) extract a shared
XCODE_VERSION variable (repo-level or workflow-level) and reference it in all
workflows so both the xcode-version keys use the same value.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b458da88-0220-466d-85eb-f1b8ade4c64b
📒 Files selected for processing (2)
.github/workflows/build-test.ymlcode/framework/src/scripting/builtins/events.cpp
Kheartz
force-pushed
the
mhs_build_removal
branch
from
015733e to
ded69a0
Compare
Kheartz
changed the title
Kheartz
changed the title
- Remove failing MafiaHubServices build step - Bump macOS runner to Xcode 26.3 so libc++ provides std::jthread (used by Webserver); older Apple toolchains gated it behind -fexperimental-library
Kheartz
force-pushed
the
mhs_build_removal
branch
from
ded69a0 to
03e4c16
Compare
Kheartz
changed the title
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
2 participants
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.
Seems like recent PRs after #200 are currently failing due to absent cmake build target
MafiaHubServicesSee #202 https://github.com/MafiaHub/Framework/actions/runs/27428394133 for example.Since this is prebuilt, this doesn't seem like it needs to be a cmake target any longer?
Also other build fix:
std::jthreadnot found errorSummary by CodeRabbit
Chores
Tests