Closed
Conversation
Contributor
WalkthroughComprehensive macOS support implementation including a new platform abstraction layer with SDL2-based windowing, native utilities, and thread management; CoreAudio backend integration via RtAudio with device enumeration and non-interleaved buffer handling; plus supporting API improvements for automation visibility, pattern management, playlist observation, and DPI-aware rendering. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant WM as AestraWindowManager
participant Win as PlatformWindowmacOS
participant Rend as NUIRendererGL
participant SDL as SDL2
App->>WM: initialize(WindowDesc)
WM->>Win: create(desc)
Win->>SDL: SDL_CreateWindow
SDL-->>Win: SDL window + GL context
Win->>Rend: setDPIScale(dpiScale)
Rend->>Rend: updateFramebufferSize()
WM->>Rend: resize(actualWidth, actualHeight)
WM->>WM: setRootComponentBounds(logicalSize)
Note over WM,Rend: DPI-aware layout and rendering ready
sequenceDiagram
participant App as Application
participant TM as TrackManager
participant MM as PlaylistMixer
participant RTA as RtAudioDriver
participant RtAudio as RtAudio(CoreAudio)
App->>TM: initialize with audio config
TM->>RTA: openDevice(AudioDeviceConfig)
RTA->>RtAudio: RtAudio::openStream()
RtAudio-->>RTA: stream opened
TM->>RTA: startStream(callback)
RTA->>RtAudio: startStream()
Note over RTA,RtAudio: Audio frames flowing
loop Audio Callback
RtAudio->>RTA: rtAudioCallback(frames)
RTA->>RTA: prepare non-interleaved buffers
RTA->>MM: IAudioCallback::process(input, output, frames)
MM-->>RTA: processed audio
end
TM->>RTA: stopStream()
RTA->>RtAudio: stopStream()
sequenceDiagram
participant PM as PlaylistModel
participant Obs as ChangeObserver
participant UI as UI Components
PM->>PM: addChangeObserver(callback)
Obs->>Obs: store callback
Note over PM,Obs: Observer registered
PM->>PM: createLane() / addClip() / setClipDuration()
PM->>PM: notifyObservers()
loop Each Registered Observer
PM->>Obs: invoke callback()
Obs->>UI: update UI state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Contributor
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan Review Summary by QodoAdd macOS platform support with CoreAudio audio driver
WalkthroughsDescription• Add complete macOS platform support with SDL2 windowing and CoreAudio • Implement macOS-specific utilities, threading, and audio driver • Fix DPI scaling for Retina displays in UI renderer • Resolve API mismatches and improve code consistency • Update build system for macOS framework linking Diagramflowchart LR
A["macOS Platform Layer"] -->|SDL2 Windowing| B["PlatformWindowmacOS"]
A -->|System Utils| C["PlatformUtilsmacOS"]
A -->|Thread Priority| D["PlatformThreadmacOS"]
E["CoreAudio Backend"] -->|RtAudio Driver| F["RtAudioDriver"]
B -->|DPI Scale| G["NUIRendererGL"]
G -->|Framebuffer Size| H["Retina Support"]
I["CMake Build"] -->|Framework Linking| J["Cocoa/OpenGL/CoreAudio"]
J -->|Executable| K["Aestra macOS App"]
File Changes1. AestraPlat/src/macOS/PlatformWindowmacOS.h
|
Contributor
Code Review by Qodo
1. No macOS driver registry
|
📚 API Documentation Quality CheckStatus: ❌ Needs Improvement
❌ Please fix documentation errors before merging. 📖 Documentation Guidelines
See coding-style.md for details. |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+494
to
+529
| elseif(APPLE) | ||
| # macOS - CoreAudio via RtAudio | ||
| set(AESTRA_AUDIO_MACOS_SOURCES | ||
| src/macOS/RtAudioDriver.cpp | ||
| ) | ||
|
|
||
| set(AESTRA_AUDIO_MACOS_HEADERS | ||
| src/macOS/RtAudioDriver.h | ||
| include/Drivers/AudioPlatformRegistry.h | ||
| ${RTAUDIO_HEADERS} | ||
| ) | ||
|
|
||
| add_library(AestraAudioMacOS STATIC | ||
| ${AESTRA_AUDIO_MACOS_SOURCES} | ||
| ${AESTRA_AUDIO_MACOS_HEADERS} | ||
| ${RTAUDIO_SOURCES} | ||
| ) | ||
|
|
||
| target_include_directories(AestraAudioMacOS | ||
| PUBLIC | ||
| # Nothing public | ||
| PRIVATE | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/include | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/src/macOS | ||
| ${RTAUDIO_DIR} | ||
| ) | ||
|
|
||
| target_link_libraries(AestraAudioMacOS | ||
| PUBLIC | ||
| AestraAudioCore | ||
| PRIVATE | ||
| ${RTAUDIO_LIBS} | ||
| ) | ||
|
|
||
| add_library(AestraAudio ALIAS AestraAudioMacOS) | ||
|
|
Contributor
There was a problem hiding this comment.
1. No macos driver registry 🐞 Bug ✓ Correctness
On APPLE, AestraAudioMacOS builds only src/macOS/RtAudioDriver.cpp and does not compile any macOS RegisterPlatformDrivers implementation, so AudioDeviceManager ends up calling the no-op stub and registers zero real drivers on macOS (no device enumeration/streaming).
Agent Prompt
## Issue description
On macOS, `AudioDeviceManager::initialize()` calls `RegisterPlatformDrivers(*this)`, but the APPLE build of `AestraAudioMacOS` does not compile any macOS implementation of `RegisterPlatformDrivers`. The only available definition is the no-op stub, so zero real audio drivers are registered.
## Issue Context
Linux and Windows both compile a platform registry translation unit (e.g. `src/Linux/LinuxDriverRegistry.cpp`, `src/Win32/WindowsDriverRegistry.cpp`) that registers at least one real driver.
## Fix Focus Areas
- Add a macOS registry TU implementing `Aestra::Audio::RegisterPlatformDrivers(AudioDeviceManager&)` and (optionally) `GetPlatformAudioInfo()`.
- Ensure the macOS registry registers a driver that implements `IAudioDriver` (either a macOS RtAudio-based `IAudioDriver` implementation or reuse/adapt the existing Linux `RtAudioDriver` pattern).
- Add the new registry source file to the APPLE `AESTRA_AUDIO_MACOS_SOURCES` list.
### Fix Focus Areas (exact locations)
- AestraAudio/CMakeLists.txt[494-529]
- AestraAudio/src/Core/AudioDeviceManager.cpp[31-60]
- AestraAudio/src/Core/DriverRegistryStub.cpp[1-14]
- AestraAudio/src/Linux/LinuxDriverRegistry.cpp[14-24]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+96
to
+192
| RtAudio::StreamOptions options; | ||
| options.flags = RTAUDIO_MINIMIZE_LATENCY | RTAUDIO_NONINTERLEAVED; | ||
| options.numberOfBuffers = 2; | ||
| options.priority = 0; | ||
|
|
||
| unsigned int bufferFrames = bufferSize_; | ||
| const RtAudioErrorType error = rtAudio_->openStream( | ||
| &outputParams_, inputParams, RTAUDIO_FLOAT32, config.sampleRate, &bufferFrames, &RtAudioDriver::rtAudioCallback, | ||
| this, &options); | ||
| if (!isRtAudioOk(error)) { | ||
| AESTRA_LOG_ERROR("Failed to open audio device"); | ||
| return false; | ||
| } | ||
|
|
||
| bufferSize_ = bufferFrames; | ||
| outputBuffers_.resize(numOutputChannels_); | ||
| for (auto& buf : outputBuffers_) { | ||
| buf.resize(bufferSize_); | ||
| } | ||
| outputBufferPtrs_.resize(numOutputChannels_); | ||
|
|
||
| isStreamOpen_ = true; | ||
|
|
||
| RtAudio::DeviceInfo info = rtAudio_->getDeviceInfo(deviceId); | ||
| AESTRA_LOG_INFO("Audio device opened: " << info.name << " @ " << config.sampleRate << "Hz"); | ||
| return true; | ||
| } | ||
|
|
||
| void RtAudioDriver::closeDevice() { | ||
| stopStream(); | ||
| if (isStreamOpen_ && rtAudio_) { | ||
| try { | ||
| rtAudio_->closeStream(); | ||
| } catch (...) {} | ||
| isStreamOpen_ = false; | ||
| } | ||
| } | ||
|
|
||
| bool RtAudioDriver::startStream(IAudioCallback* callback) { | ||
| if (!isStreamOpen_ || isStreamRunning_) | ||
| return false; | ||
|
|
||
| callback_.store(callback, std::memory_order_release); | ||
|
|
||
| if (!isRtAudioOk(rtAudio_->startStream())) { | ||
| AESTRA_LOG_ERROR("Failed to start audio stream"); | ||
| callback_.store(nullptr); | ||
| return false; | ||
| } | ||
|
|
||
| isStreamRunning_ = true; | ||
| AESTRA_LOG_INFO("Audio stream started"); | ||
| return true; | ||
| } | ||
|
|
||
| void RtAudioDriver::stopStream() { | ||
| if (!isStreamRunning_ || !rtAudio_) | ||
| return; | ||
|
|
||
| try { | ||
| rtAudio_->stopStream(); | ||
| } catch (...) {} | ||
|
|
||
| isStreamRunning_ = false; | ||
| callback_.store(nullptr); | ||
| } | ||
|
|
||
| int RtAudioDriver::rtAudioCallback(void* outputBuffer, void* inputBuffer, unsigned int nFrames, | ||
| double streamTime, RtAudioStreamStatus status, void* userData) { | ||
| (void)streamTime; | ||
|
|
||
| auto* driver = static_cast<RtAudioDriver*>(userData); | ||
|
|
||
| if (status) { | ||
| driver->xrunCount_++; | ||
| } | ||
|
|
||
| IAudioCallback* cb = driver->callback_.load(std::memory_order_acquire); | ||
| if (!cb) { | ||
| // Silence | ||
| float* out = static_cast<float*>(outputBuffer); | ||
| std::memset(out, 0, nFrames * driver->numOutputChannels_ * sizeof(float)); | ||
| return 0; | ||
| } | ||
|
|
||
| // Setup non-interleaved buffer pointers | ||
| float* out = static_cast<float*>(outputBuffer); | ||
| for (unsigned int ch = 0; ch < driver->numOutputChannels_; ch++) { | ||
| driver->outputBufferPtrs_[ch] = out + (ch * nFrames); | ||
| } | ||
|
|
||
| // Process audio through callback | ||
| const float* in = static_cast<const float*>(inputBuffer); | ||
|
|
||
| // Call the audio callback with non-interleaved data | ||
| cb->process(in, driver->outputBufferPtrs_[0], nFrames); | ||
|
|
Contributor
There was a problem hiding this comment.
2. Rtaudio fills one channel 🐞 Bug ✓ Correctness
The macOS RtAudio callback passes only channel 0 to IAudioCallback::process even though the stream is opened with RTAUDIO_NONINTERLEAVED and may have multiple output channels, leaving other channels uninitialized and producing garbage audio.
Agent Prompt
## Issue description
`RtAudioDriver::rtAudioCallback` sets up pointers for each non-interleaved output channel, but only invokes `IAudioCallback::process` with `outputBufferPtrs_[0]`, so channels 1..N are left uninitialized.
## Issue Context
The stream is opened with `RTAUDIO_NONINTERLEAVED`, meaning the output buffer contains `nFrames` samples per channel concatenated back-to-back. A correct implementation must ensure every channel’s block is written (or switch to interleaved mode and pass the interleaved buffer to the callback).
## Fix Focus Areas
- Decide on one buffer contract for `IAudioCallback`:
- If the engine expects interleaved: remove `RTAUDIO_NONINTERLEAVED` and pass `outputBuffer` directly.
- If you want non-interleaved: change `IAudioCallback::process` to accept all channels (e.g., `float** outputs` or `(float* output, unsigned int channels)` with documented layout) and write all channels.
- Ensure the callback silences *all* output channels in the “no callback” path.
### Fix Focus Areas (exact locations)
- AestraAudio/src/macOS/RtAudioDriver.cpp[96-104]
- AestraAudio/src/macOS/RtAudioDriver.cpp[181-192]
- AestraAudio/src/macOS/RtAudioDriver.h[26-31]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
18
to
27
| #elif AESTRA_PLATFORM_MACOS | ||
| // macOS stub implementation - runtime parity pending | ||
| // No Cocoa/AppKit headers required for headless builds | ||
| #ifdef AESTRA_HAS_SDL2 | ||
| #include "macOS/PlatformUtilsmacOS.h" | ||
| #include "macOS/PlatformWindowmacOS.h" | ||
|
|
||
| #include <SDL2/SDL.h> | ||
| #endif | ||
| #endif | ||
|
|
||
| namespace Aestra { |
Contributor
There was a problem hiding this comment.
3. Macos sdl calls unguarded 🐞 Bug ✓ Correctness
Platform::initialize() on macOS calls SDL_Init/SDL_SetHint even when AESTRA_HAS_SDL2 is not defined, but SDL headers are only included under AESTRA_HAS_SDL2, causing build failures when SDL2 isn’t found (which CMake explicitly allows).
Agent Prompt
## Issue description
`Platform::initialize()` on macOS calls SDL APIs unconditionally, but SDL is only included/available when `AESTRA_HAS_SDL2` is defined. The CMake configuration explicitly allows SDL2 to be missing on macOS (it prints a warning and does not define `AESTRA_HAS_SDL2`), so this currently fails to compile in that configuration.
## Issue Context
Linux already has the correct pattern (`#ifdef AESTRA_HAS_SDL2` around SDL init with an `#else` error path). macOS should match that structure.
## Fix Focus Areas
- Wrap the macOS SDL init/hints and `PlatformUtilsmacOS` instantiation in `#ifdef AESTRA_HAS_SDL2`.
- Add an `#else` branch that logs a clear error and returns false (mirroring the Linux implementation).
### Fix Focus Areas (exact locations)
- AestraPlat/src/Platform.cpp[18-25]
- AestraPlat/src/Platform.cpp[90-103]
- AestraPlat/CMakeLists.txt[36-53]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 26
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
AestraAudio/include/Models/PlaylistModel.h (1)
227-248:⚠️ Potential issue | 🔴 CriticalValidate the destination lane before removing the clip.
If
newLaneIdis syntactically valid but no longer exists inm_laneMap, this branch erases the clip from its old lane, leavesm_clipLaneMapstale, and still returnstrue. That breaks the contract documented on Line 212 to stay in the current lane when the target is invalid.🩹 Proposed fix
- if (newLaneId.isValid()) { + if (newLaneId.isValid()) { auto oldLaneIt = m_clipLaneMap.find(clipId); - if (oldLaneIt != m_clipLaneMap.end() && oldLaneIt->second != newLaneId) { + auto newLaneIdxIt = m_laneMap.find(newLaneId); + if (newLaneIdxIt != m_laneMap.end() && + oldLaneIt != m_clipLaneMap.end() && + oldLaneIt->second != newLaneId) { // Remove from old lane auto oldLaneIdxIt = m_laneMap.find(oldLaneIt->second); if (oldLaneIdxIt != m_laneMap.end()) { auto& oldClips = m_lanes[oldLaneIdxIt->second].clips; oldClips.erase(std::remove_if(oldClips.begin(), oldClips.end(), [&clipId](const ClipInstance& c) { return c.id == clipId; }), oldClips.end()); } // Add to new lane - auto newLaneIdxIt = m_laneMap.find(newLaneId); - if (newLaneIdxIt != m_laneMap.end()) { - m_lanes[newLaneIdxIt->second].clips.push_back(clipCopy); - m_clipLaneMap[clipId] = newLaneId; - } + m_lanes[newLaneIdxIt->second].clips.push_back(clipCopy); + m_clipLaneMap[clipId] = newLaneId; notifyClipChanged(clipId); notifyObservers(); return true; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AestraAudio/include/Models/PlaylistModel.h` around lines 227 - 248, The code removes a clip from its old lane before verifying the destination exists, which can leave m_clipLaneMap stale when newLaneId is valid syntactically but absent from m_laneMap; fix by first checking m_laneMap.find(newLaneId) and only proceeding to erase from m_lanes[old] and push into m_lanes[new] (and update m_clipLaneMap) if the newLaneId is present; if not present, do nothing and return false so that clipId, m_clipLaneMap, and lane contents remain unchanged; ensure notifyClipChanged(clipId) and notifyObservers() are called only after a successful move.AestraUI/Graphics/OpenGL/NUIRendererGL.cpp (1)
1630-1640: 🧹 Nitpick | 🔵 TrivialRemove the stray debugging/editor notes.
This block reads like leftover investigation chatter from an earlier compile error, not source documentation. It obscures the actual centering logic for no benefit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AestraUI/Graphics/OpenGL/NUIRendererGL.cpp` around lines 1630 - 1640, Remove the stray debugging/editor note block that was accidentally left in NUIRendererGL.cpp: delete the commented investigation chatter around measureText and NUISize (references to measureText, NUISize, textSize) so only the actual centering logic and any single legitimate comment remain; ensure you do not remove the real declarations or usages of measureText, NUISize, or textSize—just strip the leftover notes to restore the clean centering code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AestraAudio/include/Models/ClipInstance.h`:
- Around line 63-64: Add a short clarifying comment above the using alias
LocalEdits = ClipEdits; explaining why the alias exists (e.g., to convey
semantic intent for per-instance/local edit semantics or to reserve the name for
future specialized behavior) and when to prefer LocalEdits versus ClipEdits;
reference the alias symbol LocalEdits and the underlying type ClipEdits so
reviewers can easily find and understand the intention in ClipInstance.h.
- Around line 76-78: Remove the dead instance-level field `muted` from the
ClipInstance class: this field is never read or written—mute state is stored in
`ClipInstance::edits.muted` (type `ClipEdits`). Delete the `bool muted = false;`
declaration in ClipInstance and ensure no remaining references exist (the
serializer and UI already use `edits.muted` as seen in ProjectSerializer and
TrackUIComponent).
In `@AestraAudio/include/Models/PatternManager.h`:
- Around line 112-123: In clonePattern (function PatternID clonePattern) the
cloned PatternSource currently doesn't preserve the source's mixer routing;
update the clone creation to copy source->m_mixerChannel into the new pattern
(pattern->m_mixerChannel = source->m_mixerChannel) so the cloned pattern retains
its mixer assignment, or alternatively add a clear comment in
PatternSource/clonePattern explaining why m_mixerChannel is intentionally reset.
- Around line 130-132: removePattern(PatternID id) erases the unique_ptr-held
PatternSource in m_patterns causing shared_ptrs returned by getAllPatterns()
(which use no-op deleters and raw pointers) to dangle; fix by either converting
m_patterns to hold std::shared_ptr<PatternSource> so getAllPatterns() can return
shared_ptrs that correctly extend object lifetime, or change getAllPatterns() to
return non-owning references (e.g., PatternID list or raw/non-owning pointers)
instead of shared_ptrs; update removePattern, m_patterns' type, and
getAllPatterns()/PatternSource handling consistently so ownership semantics are
clear and no use-after-free can occur.
In `@AestraAudio/include/Models/PlaylistMixer.h`:
- Around line 12-14: The int overload setResamplingQuality(int) casts blindly to
ClipResamplingQuality; either remove this overload or add validation to ensure
the incoming integer is within the valid enum range (0–3) before casting—e.g.,
check if quality >= 0 && quality <= 3, log or return early on invalid values,
otherwise call
setResamplingQuality(static_cast<ClipResamplingQuality>(quality)); reference the
setResamplingQuality(int) and ClipResamplingQuality symbols and update callers
if you choose to remove the overload to force enum usage.
In `@AestraAudio/include/Models/PlaylistModel.h`:
- Line 316: The code currently pushes observers via addChangeObserver and
invokes them from notifyObservers while holding m_mutex/unique_lock on the
non-recursive std::shared_mutex, which can deadlock if observers access the
model and races with addChangeObserver; fix by snapshotting m_changeObservers
under the lock (e.g., copy to a local vector), then release the lock and
iterate/call the observers from that local copy; apply the same pattern wherever
notifyObservers is called (also replace direct iteration over m_changeObservers
with a locked copy in those locations).
In `@AestraAudio/src/macOS/RtAudioDriver.cpp`:
- Around line 96-99: The hardcoded RtAudio::StreamOptions.numberOfBuffers = 2
should be made configurable via the existing AudioDeviceConfig so callers can
tune latency vs underrun risk; update the code that constructs
RtAudio::StreamOptions (look for RtAudio::StreamOptions and numberOfBuffers in
RtAudioDriver.cpp) to read a new field (e.g., audioDeviceConfig.numberOfBuffers
or provide a getter) from AudioDeviceConfig, fall back to the current default of
2 when unset, and validate the value (clamp to a sensible range like 1..8)
before assigning to options.numberOfBuffers.
- Around line 124-132: In RtAudioDriver::closeDevice, avoid silently swallowing
all exceptions: catch the exception thrown by rtAudio_->closeStream() (while
still preventing propagation) and log the error using the existing logging
facility (or add one) with context including RtAudioDriver::closeDevice,
isStreamOpen_ and rtAudio_ before setting isStreamOpen_ = false; keep the
try/catch to prevent throwing but ensure the catch records the exception
message/what() or a generic error if unavailable so failures during
closeStream() are visible for debugging.
- Around line 181-194: The callback is only given outputBufferPtrs_[0] so
multi-channel output is left unprocessed; update the code to either (A) call the
callback per channel in a loop (for each ch in driver->numOutputChannels_ call
cb->process(input channel pointer or per-channel input,
driver->outputBufferPtrs_[ch], nFrames)) so the driver performs
channel-by-channel processing, or (B) change the IAudioCallback::process
signature to accept an array of output pointers (e.g., const float* const
output[] or float** output) and then pass driver->outputBufferPtrs_ directly;
modify usages of IAudioCallback::process accordingly to handle non-interleaved
multi-channel buffers and ensure driver->outputBufferPtrs_ and
driver->numOutputChannels_ are correctly used.
In `@AestraAudio/src/macOS/RtAudioDriver.h`:
- Around line 15-24: AudioDeviceConfig currently declares four independent
members (inputChannels, outputChannels, numInputChannels, numOutputChannels)
which can diverge; pick a single canonical pair (e.g.,
inputChannels/outputChannels) and remove the duplicate num* members or replace
them with inline accessors that forward to the canonical members so they cannot
get out of sync, then update any callers (including openDevice()) to read/write
only the canonical members or the new accessors; ensure defaults remain the same
and no direct storage for num* remains to prevent silent divergence.
In `@AestraPlat/CMakeLists.txt`:
- Around line 113-121: The macOS link block conditionally links SDL2 using
SDL2_FOUND which misses the case where SDL2 is provided as an imported target
(add_subdirectory) and causes unresolved symbols; update the
target_link_libraries call for target AestraPlat to gate SDL2 linkage with
generator expression $<TARGET_EXISTS:SDL2::SDL2> (matching the source-selection
logic that sets AESTRA_HAS_SDL2) instead of using SDL2_FOUND so the SDL2::SDL2
target is linked when it exists.
In `@AestraPlat/src/macOS/PlatformThreadmacOS.cpp`:
- Around line 23-27: When handling ThreadPriority::Low (the
setpriority(PRIO_PROCESS, 0, 10) call), check setpriority's return value for -1
and inspect errno; on failure log or propagate the error (include errno or
strerror(errno) for context) and return false instead of always returning true —
update the block that currently handles ThreadPriority::Low to validate
setpriority's result, handle errno, and return an appropriate failure indicator.
- Around line 38-62: The function setCurrentThreadPriority incorrectly returns
true for unhandled ThreadPriority values and wrongly uses
setpriority(PRIO_PROCESS, ...) which affects the whole process; change the
default return to false and replace all uses of setpriority(PRIO_PROCESS, ...)
(seen in this file and the RealtimeAudio fallback) with per-thread APIs: use
pthread_setschedparam for SCHED_FIFO/SCHED_OTHER adjustments where appropriate,
and for realtime audio prefer thread_policy_set with
THREAD_TIME_CONSTRAINT_POLICY or pthread_set_qos_class_self_np for QoS-based
priorities; update the RealtimeAudio branch to attempt thread_policy_set or
pthread_setschedparam and only fall back to a per-thread qos change, returning
true on success and false on failure.
In `@AestraPlat/src/macOS/PlatformUtilsmacOS.cpp`:
- Around line 40-56: The three macOS stub methods
(PlatformUtilsmacOS::openFileDialog, saveFileDialog, selectFolderDialog)
currently just log and return an empty string; replace each with a native Cocoa
implementation using NSOpenPanel/NSSavePanel (move/rename the file to .mm if
needed) that runs on the main thread, configures allowed file types and
directory selection based on the existing parameters (use NSOpenPanel for
open/select-folder and NSSavePanel for save), presents the panel modally,
captures the selected NSURL, converts to std::string (UTF-8) for the return, and
properly handle cancelation by returning empty string; ensure to guard Cocoa
code with `#ifdef` __APPLE__ and keep function signatures (openFileDialog,
saveFileDialog, selectFolderDialog) unchanged.
- Around line 77-81: The getProcessorCount implementation uses an uninitialized
local int count and ignores sysctlbyname()'s return value, causing undefined
behavior on failure; in PlatformUtilsmacOS::getProcessorCount() initialize count
to 0, call sysctlbyname("hw.logicalcpu", &count, &size, nullptr, 0), check the
returned errno/return value (or retval) and only use count if the call succeeded
and count > 0, otherwise return a safe default (e.g., 1); mirror the error-check
pattern used in getSystemMemory() to log or handle failures consistently.
- Around line 17-33: Replace the file-scope globals and unsynchronized init with
a function-local thread-safe static for the timebase and compute using a safe
cast: remove or stop using s_timebaseInfo and s_timebaseInitialized, and in
PlatformUtilsmacOS::getTime() call mach_absolute_time(), then ensure a
function-local static mach_timebase_info_data_t (initialized via
mach_timebase_info inside the static initializer or use std::call_once) so the
first-time init is atomic/thread-safe; compute nanoseconds by casting the
uint64_t time to double before multiplying by the timebase numer and dividing by
denom (e.g., double nanoseconds = static_cast<double>(time) * info.numer /
info.denom) to avoid integer overflow and return nanoseconds/1e9.
In `@AestraPlat/src/macOS/PlatformWindowmacOS.cpp`:
- Around line 330-333: The setMouseCapture implementation
(PlatformWindowmacOS::setMouseCapture) currently toggles SDL state without
checking m_window; guard against a null m_window before calling
SDL_SetWindowGrab and SDL_SetRelativeMouseMode by returning early (or only
performing SDL calls) when m_window is null, while still updating
m_mouseCaptured as appropriate; reference m_window, m_mouseCaptured,
SDL_SetWindowGrab, SDL_SetRelativeMouseMode and consider lifecycle hooks such as
create() and destroy() to ensure no null window is passed to SDL.
- Around line 162-186: The SDL_TEXTINPUT handler in PlatformWindowmacOS.cpp
currently decodes only the first UTF-8 codepoint from e.text.text and drops the
rest; change the logic in the SDL_TEXTINPUT case (where the current
single-codepoint decode is implemented) to iterate over the null-terminated
UTF-8 buffer byte-by-byte, decode each full UTF-8 sequence into a Unicode
codepoint (handling 1-4 byte sequences and validating continuation bytes), and
invoke m_charCallback(codepoint) for every decoded codepoint until the buffer
terminator; keep existing branch decoding logic but perform it inside a loop
that advances the pointer by the number of bytes consumed for each codepoint and
stops at '\0'.
- Around line 144-149: The SDL_MOUSEWHEEL handler in PlatformWindowmacOS.cpp
unconditionally reads e.wheel.preciseY (broken on SDL < 2.0.18) and ignores
e.wheel.direction (SDL_MOUSEWHEEL_FLIPPED), so update the SDL_MOUSEWHEEL case:
read delta from e.wheel.preciseY only when SDL_VERSION_ATLEAST(2,0,18)
(otherwise fall back to e.wheel.y), then if e.wheel.direction ==
SDL_MOUSEWHEEL_FLIPPED multiply delta by -1 to correct macOS natural scrolling,
and finally call m_mouseWheelCallback(delta); modify the SDL_MOUSEWHEEL switch
block (the code that computes delta and calls m_mouseWheelCallback) to implement
these checks.
In `@AestraPlat/src/Platform.cpp`:
- Around line 90-102: The macOS initialization block currently calls SDL_Init,
SDL_SetHint and constructs PlatformUtilsmacOS unconditionally; wrap this entire
branch in the same AESTRA_HAS_SDL2 preprocessor guard used for the includes so
that when SDL2 is unavailable you return false instead of referencing SDL
symbols and PlatformUtilsmacOS at compile time. Specifically, enclose the
SDL_Init/SDL_SetHint calls and the s_utils = new PlatformUtilsmacOS() /
AESTRA_LOG_INFO lines within `#ifdef` AESTRA_HAS_SDL2 ... `#else` { return false; }
`#endif` (matching the Linux path behavior) so SDL_Init, SDL_SetHint and
PlatformUtilsmacOS are only used when AESTRA_HAS_SDL2 is defined.
In `@AestraUI/CMakeLists.txt`:
- Around line 228-254: The FreeType fallback only hints Homebrew paths; update
the CMake find step so find_library (AESTRAUI_FREETYPE_LIBRARY) and find_path
(AESTRAUI_FREETYPE_INCLUDE_DIR) also include common Linux locations (e.g.
/usr/lib, /usr/lib64, /usr/local/lib for libraries and /usr/include,
/usr/include/freetype2 for headers) in their HINTS or add additional
PATHS/PATH_SUFFIXES so the imported target AestraUI_Freetype will be found on
Linux systems as well.
In `@AestraUI/Graphics/OpenGL/NUIRendererGL.cpp`:
- Around line 520-533: The clip coordinate scaling is using dpiScale_ which is
the window scale, causing offscreen/cached passes (started by
renderToTextureBegin() / beginOffscreen()) to scissor against the wrong pixel
bounds; replace uses of dpiScale_ when computing
scaledX1/scaledY1/scaledX2/scaledY2 and bottomGL with the active render-target
ratio (the same ratio used to compute the current
framebufferWidth_/framebufferHeight_), i.e. use the render-target scale variable
(the one used when you set framebufferWidth_/framebufferHeight_ during
renderToTextureBegin()/beginOffscreen()) instead of dpiScale_ so scissor boxes
align to the actual target pixels.
In `@CMakeLists.txt`:
- Around line 64-72: The SDL2 inclusion block is never run on macOS because
AESTRA_PLATFORM_MACOS is never defined; update the platform check in the SDL2
block to use CMake's built-in APPLE variable instead of AESTRA_PLATFORM_MACOS.
Concretely, change the condition in the SDL2 include block (the if(...) that
currently references AESTRA_PLATFORM_LINUX OR AESTRA_PLATFORM_MACOS) to
reference AESTRA_PLATFORM_LINUX OR APPLE (or alternatively initialize
AESTRA_PLATFORM_MACOS from APPLE earlier), ensuring the find_package(SDL2 ...) /
add_subdirectory(external/SDL2) logic executes on macOS.
In `@MACOS_SUPPORT.md`:
- Line 127: Summary: macOS Support note mentions x86_64 only; add native ARM64
testing to CI. Update the CI build matrix to include an ARM64 job alongside
x86_64 (so both x86_64 and ARM64 architectures are built and tested), ensure
jobs run on macOS runners that support Apple Silicon (or on self-hosted M1/M2/M3
machines), set the appropriate architecture build flags/vars (e.g., ARCH/GOARCH
or equivalent build targets) for the ARM64 job, and produce native ARM64 or
universal2 artifacts instead of relying only on Rosetta 2 emulation; reflect
this change in the MACOS_SUPPORT.md note about testing and CI.
- Around line 58-82: The markdown has missing blank lines around headings and
fenced code blocks in the "Build Instructions" section; update MACOS_SUPPORT.md
so each heading ("Build Instructions", "Prerequisites", "Configure and Build")
is preceded and followed by a blank line and ensure every fenced code block (the
```bash blocks) is separated from surrounding text with blank lines (add a blank
line before the opening ``` and after the closing ```). This will resolve the
lint warnings about surrounding blank lines for headings and code fences.
In `@MAINTENANCE_SUMMARY.md`:
- Around line 85-98: Update the "Build Instructions" section in
MAINTENANCE_SUMMARY.md to remove the hardcoded user-specific path; replace
occurrences of "/Users/cedrick/Aestra" with a relative path or generic
placeholder such as "Aestra" or "<repo-root>" (e.g., change the `cd
/Users/cedrick/Aestra` step to `cd Aestra` or `cd <repo-root>`), and update
verification examples accordingly (e.g., `file build/bin/Aestra` instead of
`file /Users/cedrick/Aestra/build/bin/Aestra`) so the steps work for any
developer cloning the repository.
---
Outside diff comments:
In `@AestraAudio/include/Models/PlaylistModel.h`:
- Around line 227-248: The code removes a clip from its old lane before
verifying the destination exists, which can leave m_clipLaneMap stale when
newLaneId is valid syntactically but absent from m_laneMap; fix by first
checking m_laneMap.find(newLaneId) and only proceeding to erase from
m_lanes[old] and push into m_lanes[new] (and update m_clipLaneMap) if the
newLaneId is present; if not present, do nothing and return false so that
clipId, m_clipLaneMap, and lane contents remain unchanged; ensure
notifyClipChanged(clipId) and notifyObservers() are called only after a
successful move.
In `@AestraUI/Graphics/OpenGL/NUIRendererGL.cpp`:
- Around line 1630-1640: Remove the stray debugging/editor note block that was
accidentally left in NUIRendererGL.cpp: delete the commented investigation
chatter around measureText and NUISize (references to measureText, NUISize,
textSize) so only the actual centering logic and any single legitimate comment
remain; ensure you do not remove the real declarations or usages of measureText,
NUISize, or textSize—just strip the leftover notes to restore the clean
centering code.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 404711d6-1f29-4cbe-9dbc-192233df43e8
📒 Files selected for processing (40)
AestraAudio/CMakeLists.txtAestraAudio/include/AestraUUID.hAestraAudio/include/Core/AutomationCurve.hAestraAudio/include/DSP/SampleRateConverter.hAestraAudio/include/IO/WaveformCache.hAestraAudio/include/Models/ClipInstance.hAestraAudio/include/Models/ClipSource.hAestraAudio/include/Models/MeterSnapshot.hAestraAudio/include/Models/PatternManager.hAestraAudio/include/Models/PatternSource.hAestraAudio/include/Models/PlaylistMixer.hAestraAudio/include/Models/PlaylistModel.hAestraAudio/include/Models/TrackManager.hAestraAudio/include/Playback/TimelineClock.hAestraAudio/src/macOS/RtAudioDriver.cppAestraAudio/src/macOS/RtAudioDriver.hAestraPlat/CMakeLists.txtAestraPlat/src/Platform.cppAestraPlat/src/macOS/PlatformThreadmacOS.cppAestraPlat/src/macOS/PlatformUtilsmacOS.cppAestraPlat/src/macOS/PlatformUtilsmacOS.hAestraPlat/src/macOS/PlatformWindowmacOS.cppAestraPlat/src/macOS/PlatformWindowmacOS.hAestraUI/CMakeLists.txtAestraUI/Core/NUIAnimation.cppAestraUI/Core/NUITypes.hAestraUI/Graphics/NUIRenderer.hAestraUI/Graphics/OpenGL/NUIRenderCache.cppAestraUI/Graphics/OpenGL/NUIRendererGL.cppAestraUI/Graphics/OpenGL/NUIRendererGL.hAestraUI/Platform/NUIPlatformBridge.cppCMakeLists.txtCMakePresets.jsonMACOS_SUPPORT.mdMAINTENANCE_SUMMARY.mdSource/Components/TrackUIComponent.hSource/Core/AestraWindowManager.cppSource/Core/Preferences.cppSource/Panels/WindowPanel.cppSource/Settings/AudioSettingsPage.cpp
💤 Files with no reviewable changes (2)
- AestraAudio/include/Models/MeterSnapshot.h
- Source/Core/Preferences.cpp
Comment on lines
+63
to
+64
| using LocalEdits = ClipEdits; | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a comment explaining LocalEdits alias purpose.
If LocalEdits is intended for semantic distinction or future expansion, a brief comment would help maintainability. Otherwise, direct usage of ClipEdits may be clearer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AestraAudio/include/Models/ClipInstance.h` around lines 63 - 64, Add a short
clarifying comment above the using alias LocalEdits = ClipEdits; explaining why
the alias exists (e.g., to convey semantic intent for per-instance/local edit
semantics or to reserve the name for future specialized behavior) and when to
prefer LocalEdits versus ClipEdits; reference the alias symbol LocalEdits and
the underlying type ClipEdits so reviewers can easily find and understand the
intention in ClipInstance.h.
Comment on lines
+76
to
78
| bool muted = false; | ||
|
|
||
| ClipEdits edits; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how both muted fields are used to understand intended semantics
rg -n '\.muted\b' --type=cpp -C 2 | head -80
# Check for any logic combining the two muted states
rg -n 'edits\.muted|instance.*muted' --type=cpp -C 3Repository: currentsuspect/Aestra
Length of output: 5985
🏁 Script executed:
#!/bin/bash
# Check the ClipInstance.h header structure
cat -n AestraAudio/include/Models/ClipInstance.h | grep -A 10 -B 10 "muted\|ClipEdits"Repository: currentsuspect/Aestra
Length of output: 1757
🏁 Script executed:
#!/bin/bash
# More thorough search for any usage of muted as a member of ClipInstance (not edits)
# Search patterns: obj.muted, instance.muted, clip.muted (not clip.edits.muted)
rg -n '\bmuted\b' --type=cpp -A 2 -B 2 | grep -v 'edits\.muted\|channel->muted\|info\.muted\|existing->muted' | head -100Repository: currentsuspect/Aestra
Length of output: 6943
🏁 Script executed:
#!/bin/bash
# Search for any direct assignment or read of ClipInstance muted field
# Look for patterns like: instance.muted, clip.muted (not clip.edits.muted), ->muted (for pointers)
rg -n 'clip\s*\.\s*muted(?!\s*\.)|instance\s*\.\s*muted|->muted' --type=cpp | grep -v 'edits\|channel\|lane\|mixer\|info\|existing'Repository: currentsuspect/Aestra
Length of output: 341
🏁 Script executed:
#!/bin/bash
# Search for ClipInstance constructor usage or initialization
ast-grep --pattern 'ClipInstance $$ = $$$' | head -50Repository: currentsuspect/Aestra
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Simple search for clip.muted or instance.muted patterns
rg -n '\bclip\b.*\bmuted\b' --type=cpp | head -50Repository: currentsuspect/Aestra
Length of output: 424
🏁 Script executed:
#!/bin/bash
# Alternative: search for assignment to muted field on ClipInstance context
rg -n '\bmuted\s*=' --type=cpp -B 3 | grep -A 3 -B 3 'ClipInstance\|clip\.' | head -80Repository: currentsuspect/Aestra
Length of output: 1788
Remove unused instance-level muted field.
The instance-level muted field on line 76 is dead code—it is never read or written anywhere in the codebase. All actual mute state is managed through ClipInstance::edits.muted, which is used in serialization (ProjectSerializer.cpp), UI rendering (TrackUIComponent.cpp), and tests.
Remove the unused field to eliminate confusion and keep the codebase clean.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AestraAudio/include/Models/ClipInstance.h` around lines 76 - 78, Remove the
dead instance-level field `muted` from the ClipInstance class: this field is
never read or written—mute state is stored in `ClipInstance::edits.muted` (type
`ClipEdits`). Delete the `bool muted = false;` declaration in ClipInstance and
ensure no remaining references exist (the serializer and UI already use
`edits.muted` as seen in ProjectSerializer and TrackUIComponent).
Comment on lines
+112
to
+123
| PatternID clonePattern(PatternID sourceId) { | ||
| auto* source = getPattern(sourceId); | ||
| if (!source) return PatternID{}; | ||
|
|
||
| PatternID newId{nextId++}; | ||
| auto pattern = std::make_unique<PatternSource>(); | ||
| pattern->id = newId; | ||
| pattern->name = source->name + " (Copy)"; | ||
| pattern->lengthBeats = source->lengthBeats; | ||
| pattern->type = source->type; | ||
| pattern->payload = source->payload; | ||
| m_patterns[newId.value] = std::move(pattern); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd '^PatternSource\.h$' | head -n1)"
if [[ -z "${file}" ]]; then
echo "PatternSource.h not found" >&2
exit 1
fi
echo "Inspecting ${file}"
sed -n '1,240p' "${file}"Repository: currentsuspect/Aestra
Length of output: 2782
Clone silently drops the mixer channel assignment.
The clonePattern method does not copy m_mixerChannel, causing cloned patterns to lose their mixer routing. Add pattern->m_mixerChannel = source->m_mixerChannel; to preserve this state, or explicitly document why clones should start with a reset channel assignment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AestraAudio/include/Models/PatternManager.h` around lines 112 - 123, In
clonePattern (function PatternID clonePattern) the cloned PatternSource
currently doesn't preserve the source's mixer routing; update the clone creation
to copy source->m_mixerChannel into the new pattern (pattern->m_mixerChannel =
source->m_mixerChannel) so the cloned pattern retains its mixer assignment, or
alternatively add a clear comment in PatternSource/clonePattern explaining why
m_mixerChannel is intentionally reset.
Comment on lines
+130
to
+132
| void removePattern(PatternID id) { | ||
| m_patterns.erase(id.value); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "PatternManager.h" -type fRepository: currentsuspect/Aestra
Length of output: 110
🏁 Script executed:
cat -n "AestraAudio/include/Models/PatternManager.h"Repository: currentsuspect/Aestra
Length of output: 5040
🏁 Script executed:
rg "getAllPatterns" --type cpp --type h -B 2 -A 5Repository: currentsuspect/Aestra
Length of output: 4009
🏁 Script executed:
rg "removePattern" --type cpp --type h -B 2 -A 5Repository: currentsuspect/Aestra
Length of output: 1102
removePattern() introduces a use-after-free vulnerability in getAllPatterns().
getAllPatterns() returns std::shared_ptr<PatternSource> objects with no-op deleters, containing raw pointers to objects owned by the unique_ptr in m_patterns. When removePattern() erases the map entry, it deletes the underlying PatternSource object, but any retained shared_ptr instances still reference the freed memory. This is a critical memory safety issue.
The code comment at line 96 acknowledges this is "not ideal but works for shim," indicating the developers were aware of the design flaw.
Resolve by either:
- Backing
m_patternswithshared_ptrinstead ofunique_ptrto share ownership, or - Refactoring
getAllPatterns()to return non-owning views (IDs or raw pointers) instead ofshared_ptr.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AestraAudio/include/Models/PatternManager.h` around lines 130 - 132,
removePattern(PatternID id) erases the unique_ptr-held PatternSource in
m_patterns causing shared_ptrs returned by getAllPatterns() (which use no-op
deleters and raw pointers) to dangle; fix by either converting m_patterns to
hold std::shared_ptr<PatternSource> so getAllPatterns() can return shared_ptrs
that correctly extend object lifetime, or change getAllPatterns() to return
non-owning references (e.g., PatternID list or raw/non-owning pointers) instead
of shared_ptrs; update removePattern, m_patterns' type, and
getAllPatterns()/PatternSource handling consistently so ownership semantics are
clear and no use-after-free can occur.
Comment on lines
+12
to
+14
| static void setResamplingQuality(int quality) { | ||
| setResamplingQuality(static_cast<ClipResamplingQuality>(quality)); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check ClipResamplingQuality enum definition to determine valid range
ast-grep --pattern $'enum class ClipResamplingQuality {
$$$
}'
# Fallback: search for enum definition
rg -n 'enum.*ClipResamplingQuality' -A 10Repository: currentsuspect/Aestra
Length of output: 18416
🏁 Script executed:
# Find all calls to setResamplingQuality with int argument
rg -n 'setResamplingQuality\s*\(\s*\d+\s*\)|setResamplingQuality\s*\(\s*\w+\s*\)' -A 2 -B 2
# Also search for the implementation in PlaylistMixer
rg -n 'setResamplingQuality' -A 3 -B 1 AestraAudio/include/Models/PlaylistMixer.h
# Check if there's a definition in PlaylistMixer.h
cat -n AestraAudio/include/Models/PlaylistMixer.h | head -50Repository: currentsuspect/Aestra
Length of output: 1191
🏁 Script executed:
# Search for calls to setResamplingQuality(int) - look for numeric literals or int variables
rg -n 'setResamplingQuality\(' --type cpp --type h | grep -E '\(.*\)' | head -30
# More specific: search for setResamplingQuality calls with int or numeric values
rg -n 'PlaylistMixer.*setResamplingQuality|\.setResamplingQuality' -B 2 -A 2
# Check AudioSettingsPage.cpp mentioned in original review
fd 'AudioSettingsPage' -type f | head -5Repository: currentsuspect/Aestra
Length of output: 2198
🏁 Script executed:
# Examine AudioSettingsPage.cpp around the call to setResamplingQuality
sed -n '370,400p' Source/Settings/AudioSettingsPage.cppRepository: currentsuspect/Aestra
Length of output: 1288
🏁 Script executed:
# Check what determines qIdx and its range
sed -n '360,390p' Source/Settings/AudioSettingsPage.cppRepository: currentsuspect/Aestra
Length of output: 1219
Int overload lacks validation — consider removing or adding bounds checks.
The setResamplingQuality(int) overload casts any int directly to ClipResamplingQuality without validating it's in the valid range [0–3]. While the current call site in AudioSettingsPage.cpp validates before calling, the function signature itself permits invalid values and offers no protection against future misuse.
Consider either:
- Removing the
intoverload to force explicit enum usage - Adding bounds validation (return early or log if out of range)
- Documenting the valid range and expected usage pattern
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AestraAudio/include/Models/PlaylistMixer.h` around lines 12 - 14, The int
overload setResamplingQuality(int) casts blindly to ClipResamplingQuality;
either remove this overload or add validation to ensure the incoming integer is
within the valid enum range (0–3) before casting—e.g., check if quality >= 0 &&
quality <= 3, log or return early on invalid values, otherwise call
setResamplingQuality(static_cast<ClipResamplingQuality>(quality)); reference the
setResamplingQuality(int) and ClipResamplingQuality symbols and update callers
if you choose to remove the overload to force enum usage.
Comment on lines
+520
to
+533
| const float scaledX1 = x1 * dpiScale_; | ||
| const float scaledY1 = y1 * dpiScale_; | ||
| const float scaledX2 = x2 * dpiScale_; | ||
| const float scaledY2 = y2 * dpiScale_; | ||
|
|
||
| int glX = static_cast<int>(std::floor(scaledX1)); | ||
| int glRight = static_cast<int>(std::ceil(scaledX2)); | ||
| int glWidth = std::max(0, glRight - glX); | ||
|
|
||
| // Convert to GL coords (bottom-up) | ||
| // Range in UI (y-down): [y1, y2] | ||
| // Range in GL (y-up): [height_ - y2, height_ - y1] | ||
|
|
||
| float bottomGL = static_cast<float>(height_) - y2; | ||
| float topGL = static_cast<float>(height_) - y1; | ||
| float bottomGL = static_cast<float>(framebufferHeight_) - scaledY2; |
Contributor
There was a problem hiding this comment.
Use the active render-target ratio here instead of dpiScale_.
renderToTextureBegin() and beginOffscreen() temporarily replace framebufferWidth_/framebufferHeight_ with the target size, but dpiScale_ still holds the window scale. Multiplying clip coordinates by dpiScale_ here makes offscreen/cached passes scissor at the wrong pixel bounds on Retina displays.
🎯 Suggested fix
- const float scaledX1 = x1 * dpiScale_;
- const float scaledY1 = y1 * dpiScale_;
- const float scaledX2 = x2 * dpiScale_;
- const float scaledY2 = y2 * dpiScale_;
+ const float scaleX = width_ > 0
+ ? static_cast<float>(framebufferWidth_) / static_cast<float>(width_)
+ : 1.0f;
+ const float scaleY = height_ > 0
+ ? static_cast<float>(framebufferHeight_) / static_cast<float>(height_)
+ : 1.0f;
+ const float scaledX1 = x1 * scaleX;
+ const float scaledY1 = y1 * scaleY;
+ const float scaledX2 = x2 * scaleX;
+ const float scaledY2 = y2 * scaleY;📝 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
| const float scaledX1 = x1 * dpiScale_; | |
| const float scaledY1 = y1 * dpiScale_; | |
| const float scaledX2 = x2 * dpiScale_; | |
| const float scaledY2 = y2 * dpiScale_; | |
| int glX = static_cast<int>(std::floor(scaledX1)); | |
| int glRight = static_cast<int>(std::ceil(scaledX2)); | |
| int glWidth = std::max(0, glRight - glX); | |
| // Convert to GL coords (bottom-up) | |
| // Range in UI (y-down): [y1, y2] | |
| // Range in GL (y-up): [height_ - y2, height_ - y1] | |
| float bottomGL = static_cast<float>(height_) - y2; | |
| float topGL = static_cast<float>(height_) - y1; | |
| float bottomGL = static_cast<float>(framebufferHeight_) - scaledY2; | |
| const float scaleX = width_ > 0 | |
| ? static_cast<float>(framebufferWidth_) / static_cast<float>(width_) | |
| : 1.0f; | |
| const float scaleY = height_ > 0 | |
| ? static_cast<float>(framebufferHeight_) / static_cast<float>(height_) | |
| : 1.0f; | |
| const float scaledX1 = x1 * scaleX; | |
| const float scaledY1 = y1 * scaleY; | |
| const float scaledX2 = x2 * scaleX; | |
| const float scaledY2 = y2 * scaleY; | |
| int glX = static_cast<int>(std::floor(scaledX1)); | |
| int glRight = static_cast<int>(std::ceil(scaledX2)); | |
| int glWidth = std::max(0, glRight - glX); | |
| // Convert to GL coords (bottom-up) | |
| // Range in UI (y-down): [y1, y2] | |
| // Range in GL (y-up): [height_ - y2, height_ - y1] | |
| float bottomGL = static_cast<float>(framebufferHeight_) - scaledY2; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AestraUI/Graphics/OpenGL/NUIRendererGL.cpp` around lines 520 - 533, The clip
coordinate scaling is using dpiScale_ which is the window scale, causing
offscreen/cached passes (started by renderToTextureBegin() / beginOffscreen())
to scissor against the wrong pixel bounds; replace uses of dpiScale_ when
computing scaledX1/scaledY1/scaledX2/scaledY2 and bottomGL with the active
render-target ratio (the same ratio used to compute the current
framebufferWidth_/framebufferHeight_), i.e. use the render-target scale variable
(the one used when you set framebufferWidth_/framebufferHeight_ during
renderToTextureBegin()/beginOffscreen()) instead of dpiScale_ so scissor boxes
align to the actual target pixels.
Comment on lines
+64
to
+72
| # SDL2 (required for Linux and macOS platform layers) | ||
| if(AESTRA_PLATFORM_LINUX OR AESTRA_PLATFORM_MACOS) | ||
| find_package(SDL2 QUIET) | ||
| if(NOT SDL2_FOUND) | ||
| if(EXISTS "${CMAKE_SOURCE_DIR}/external/SDL2/CMakeLists.txt") | ||
| add_subdirectory(external/SDL2) | ||
| else() | ||
| message(STATUS "SDL2 not found. Install with: brew install sdl2 (macOS) or apt install libsdl2-dev (Linux)") | ||
| endif() |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 '\bAESTRA_PLATFORM_MACOS\b' --glob 'CMakeLists.txt' --glob '*.cmake' --glob 'CMakePresets.json'Repository: currentsuspect/Aestra
Length of output: 387
🏁 Script executed:
# Check the entire CMakeLists.txt for variable definitions and initialization patterns
head -100 CMakeLists.txt | cat -nRepository: currentsuspect/Aestra
Length of output: 4317
🏁 Script executed:
# Search for all AESTRA_PLATFORM_* variable definitions across the entire repository
rg -n 'AESTRA_PLATFORM_' --glob 'CMakeLists.txt' -A2 -B2Repository: currentsuspect/Aestra
Length of output: 1230
🏁 Script executed:
# Check if there are any other CMake files that might define AESTRA_PLATFORM_MACOS
fd -e cmake -e txt | xargs rg 'AESTRA_PLATFORM' 2>/dev/null || echo "No matches in other files"Repository: currentsuspect/Aestra
Length of output: 347
🏁 Script executed:
# Look for any cache variable definitions or set() calls that might initialize platform flags
rg -n 'set\(AESTRA_PLATFORM' -A1 -B1Repository: currentsuspect/Aestra
Length of output: 224
AESTRA_PLATFORM_MACOS is never defined; SDL2 block is dead code for macOS.
AESTRA_PLATFORM_LINUX is set only when UNIX AND NOT APPLE (line 45). On macOS, neither platform variable is initialized, so the SDL2 dependency block (line 65) never executes. Replace with the built-in APPLE CMake variable:
Fix
-if(AESTRA_PLATFORM_LINUX OR AESTRA_PLATFORM_MACOS)
+if(AESTRA_PLATFORM_LINUX OR APPLE)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CMakeLists.txt` around lines 64 - 72, The SDL2 inclusion block is never run
on macOS because AESTRA_PLATFORM_MACOS is never defined; update the platform
check in the SDL2 block to use CMake's built-in APPLE variable instead of
AESTRA_PLATFORM_MACOS. Concretely, change the condition in the SDL2 include
block (the if(...) that currently references AESTRA_PLATFORM_LINUX OR
AESTRA_PLATFORM_MACOS) to reference AESTRA_PLATFORM_LINUX OR APPLE (or
alternatively initialize AESTRA_PLATFORM_MACOS from APPLE earlier), ensuring the
find_package(SDL2 ...) / add_subdirectory(external/SDL2) logic executes on
macOS.
Comment on lines
+58
to
+82
| ## Build Instructions | ||
|
|
||
| ### Prerequisites | ||
|
|
||
| ```bash | ||
| # Install dependencies via Homebrew | ||
| brew install cmake sdl2 freetype | ||
| ``` | ||
|
|
||
| ### Configure and Build | ||
|
|
||
| ```bash | ||
| cd Aestra | ||
|
|
||
| # Configure | ||
| cmake -B build -DAESTRA_ENABLE_TESTS=OFF | ||
|
|
||
| # Build everything | ||
| cmake --build build -j$(sysctl -n hw.ncpu) | ||
|
|
||
| # Or build specific targets | ||
| cmake --build build --target AestraHeadless # Headless only | ||
| cmake --build build --target AestraPlat # Platform layer | ||
| cmake --build build --target AestraAudioMacOS # Audio driver | ||
| ``` |
Contributor
There was a problem hiding this comment.
Fix markdown formatting issues.
Static analysis flagged several markdown lint issues: headings should be surrounded by blank lines, and a fenced code block at line 81 should be surrounded by blank lines.
📝 Proposed fix for markdown formatting
## Build Instructions
### Prerequisites
+
```bash
# Install dependencies via Homebrew
-brew install cmake sdl2 freetype
+brew install cmake sdl2 freetypeConfigure and Build
cd Aestra
# Configure
cmake -B build -DAESTRA_ENABLE_TESTS=OFF
# Build everything
cmake --build build -j$(sysctl -n hw.ncpu)
# Or build specific targets
cmake --build build --target AestraHeadless # Headless only
cmake --build build --target AestraPlat # Platform layer
cmake --build build --target AestraAudioMacOS # Audio driver</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.0)</summary>
[warning] 61-61: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
---
[warning] 63-63: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
---
[warning] 66-66: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
---
[warning] 69-69: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
---
[warning] 75-75: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
---
[warning] 81-81: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @MACOS_SUPPORT.md around lines 58 - 82, The markdown has missing blank lines
around headings and fenced code blocks in the "Build Instructions" section;
update MACOS_SUPPORT.md so each heading ("Build Instructions", "Prerequisites",
"Configure and Build") is preceded and followed by a blank line and ensure every
fenced code block (the bash blocks) is separated from surrounding text with blank lines (add a blank line before the opening and after the closing ```).
This will resolve the lint warnings about surrounding blank lines for headings
and code fences.
</details>
<!-- fingerprinting:phantom:medusa:ocelot:f3804fe6-1862-405a-ac8b-d553feaf11c2 -->
<!-- This is an auto-generated comment by CodeRabbit -->
|
|
||
| 1. Implement native Cocoa file dialogs (NSOpenPanel/NSSavePanel) | ||
| 2. Add VST3 plugin hosting support | ||
| 3. Test on Apple Silicon (M1/M2/M3) - currently built for x86_64 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Apple Silicon testing is critical for production macOS support.
The note about testing on Apple Silicon (M1/M2/M3) is important. The current x86_64 build should work via Rosetta 2, but native ARM64 builds will provide better performance. Consider adding ARM64 to the build matrix in CI.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MACOS_SUPPORT.md` at line 127, Summary: macOS Support note mentions x86_64
only; add native ARM64 testing to CI. Update the CI build matrix to include an
ARM64 job alongside x86_64 (so both x86_64 and ARM64 architectures are built and
tested), ensure jobs run on macOS runners that support Apple Silicon (or on
self-hosted M1/M2/M3 machines), set the appropriate architecture build
flags/vars (e.g., ARCH/GOARCH or equivalent build targets) for the ARM64 job,
and produce native ARM64 or universal2 artifacts instead of relying only on
Rosetta 2 emulation; reflect this change in the MACOS_SUPPORT.md note about
testing and CI.
Comment on lines
+85
to
+98
| ## Build Instructions | ||
|
|
||
| ```bash | ||
| cd /Users/cedrick/Aestra | ||
|
|
||
| # Configure | ||
| cmake -B build -DAESTRA_ENABLE_TESTS=OFF | ||
|
|
||
| # Build (parallel) | ||
| cmake --build build -j$(sysctl -n hw.ncpu) | ||
|
|
||
| # Run | ||
| ./build/bin/Aestra | ||
| ``` |
Contributor
There was a problem hiding this comment.
Remove user-specific paths from documentation.
The hardcoded path /Users/cedrick/Aestra is user-specific and won't apply to other developers. Use relative paths or generic placeholders.
📝 Proposed fix
## Build Instructions
```bash
-cd /Users/cedrick/Aestra
+cd Aestra
# Configure
cmake -B build -DAESTRA_ENABLE_TESTS=OFFAnd for verification:
## Verification
```bash
# Verify executable
-$ file /Users/cedrick/Aestra/build/bin/Aestra
+$ file build/bin/Aestra
Mach-O 64-bit executable x86_64🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MAINTENANCE_SUMMARY.md` around lines 85 - 98, Update the "Build Instructions"
section in MAINTENANCE_SUMMARY.md to remove the hardcoded user-specific path;
replace occurrences of "/Users/cedrick/Aestra" with a relative path or generic
placeholder such as "Aestra" or "<repo-root>" (e.g., change the `cd
/Users/cedrick/Aestra` step to `cd Aestra` or `cd <repo-root>`), and update
verification examples accordingly (e.g., `file build/bin/Aestra` instead of
`file /Users/cedrick/Aestra/build/bin/Aestra`) so the steps work for any
developer cloning the repository.
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.
Summary
Verification
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation