src/mudlet-lua/lua/GUIUtils.lua (1)

2055-2055: Improve comment to explain the reason for custom implementation.

The comment is informal and lacks technical detail about why rex.gsub was crashing. Consider documenting:

• The specific issue with rex.gsub (e.g., crash conditions, bug reference)
• Any behavioral differences from the original
• Limitations of the custom implementation (e.g., only uses first capture group)

--- rex.gsub was crashing so here is a homemade
+-- Custom implementation of rex.gsub to work around crashes in the native implementation.
+-- Note: Only the first capture group is used if the pattern has multiple captures.

src/mudlet-lua/lua/DateTime.lua (2)

49-49: Enhance the comment with crash details.

The comment briefly states that rex.gsub was crashing but lacks important context such as the conditions under which it crashes, affected platforms (Linux/macOS/Windows), or any relation to the Sentry integration. Consider documenting:

• Specific error or crash behavior observed
• Platforms or build configurations where the crash occurs
• Whether this is a known issue with the rex library version in use

---

50-73: Approve implementation but refactor to eliminate code duplication.

The custom rex_gsub_custom implementation correctly handles the datetime directive substitution use case. The logic is sound: it iterates through matches, assembles output pieces, and properly handles both function and static replacements.

However, this exact function also exists in GUIUtils.lua (lines 2055-2078). Duplicating utility functions across multiple files violates the DRY principle and increases maintenance burden.

Consider extracting this function into a shared utility module (e.g., StringUtils.lua or RexUtils.lua) that both DateTime.lua and GUIUtils.lua can require.

.github/workflows/update-en-us-plural.yml (1)

21-22: Submodule initialization is redundant with checkout configuration.

The checkout step on line 19 already specifies submodules: true, which initializes submodules. The additional recursive initialization step may only be necessary if the sentry-native submodule contains nested submodules that require explicit recursive initialization.

Consider whether the sentry-native submodule actually requires recursive initialization. If it doesn't have nested submodules, you could simplify by either:

1. Removing this step entirely and relying on the checkout action's submodules: true, or
2. Changing the checkout action to submodules: recursive and removing this separate step:

     - uses: actions/checkout@v5
       with:
         ref: development
-        submodules: true
+        submodules: recursive
-
-    - name: Init all submodules recursively
-      run: git submodule update --init --recursive

src/sentry_dsn.txt (1)

1-1: Document that users should configure their own Sentry DSN.

This file contains a Sentry DSN (Data Source Name) in plaintext. While DSNs are designed to be client-facing and don't grant access to view crash reports, this appears to be the Mudlet project's DSN. Users building their own forks or variants should be instructed to:

1. Create their own Sentry project
2. Replace this DSN with their own
3. Set appropriate SENTRY_AUTH_TOKEN for debug file uploads

Consider adding a comment or documentation (perhaps in a nearby README or build documentation) explaining:

• This DSN is for the official Mudlet project
• Fork maintainers should create their own Sentry project and replace this DSN
• How to disable Sentry entirely by building without -DWITH_SENTRY=ON

Example comment at the top of this file:

+# Official Mudlet Sentry DSN for crash reporting
+# If you're building a fork, create your own Sentry project at https://sentry.io
+# and replace this DSN with your own, or build without -DWITH_SENTRY=ON
 https://b207505f6b88191fa4bbcb9b166f61ae@o4510296017338368.ingest.de.sentry.io/4510330003849296

.github/workflows/performance-analysis.yml (1)

30-31: Submodule initialization may be redundant with checkout configuration.

The checkout step on line 28 already specifies submodules: true. This additional recursive initialization step is only necessary if nested submodules exist within the sentry-native submodule.

See the comment on .github/workflows/update-en-us-plural.yml lines 21-22 for consolidation options.

.github/workflows/clangtidy-diff-analysis.yml (1)

39-40: Submodule initialization may be redundant with checkout configuration.

The checkout step on line 32 already specifies submodules: true. See the comment on .github/workflows/update-en-us-plural.yml lines 21-22 for consolidation options.

.github/workflows/update-translations.yml (1)

24-25: Submodule initialization may be redundant with checkout configuration.

The checkout step on line 20 already specifies submodules: true. See the comment on .github/workflows/update-en-us-plural.yml lines 21-22 for consolidation options.

.github/workflows/codeql-analysis.yml (1)

49-50: Redundant submodule initialization.

The checkout action at line 47 already has submodules: true, which initializes submodules. The explicit git submodule update --init --recursive step is redundant.

Consider removing this step:

-    - name: Init all submodules recursively
-      run: git submodule update --init --recursive
-

Note: If the intent is to initialize submodules recursively (which submodules: true does not do by default in older actions/checkout versions), you can use submodules: recursive in the checkout step instead.

.github/workflows/update-3rdparty.yml (2)

107-108: Redundant submodule initialization.

The checkout action at line 104 already has submodules: true, making this explicit initialization redundant.

Consider removing this step or use submodules: recursive in the checkout action if recursive initialization is specifically needed:

-      - name: Init all submodules recursively
-        run: git submodule update --init --recursive
-

---

151-152: Redundant submodule initialization.

The checkout action at line 148 already has submodules: true, making this explicit initialization redundant.

Consider removing this step or use submodules: recursive in the checkout action if recursive initialization is specifically needed:

-      - name: Init all submodules recursively
-        run: git submodule update --init --recursive
-

.github/workflows/build-mudlet.yml (1)

69-70: Redundant submodule initialization.

The checkout action at line 66 already has submodules: true, making this explicit initialization redundant.

Consider removing this step or use submodules: recursive in the checkout action if recursive initialization is specifically needed:

-    - name: Init all submodules recursively
-      run: git submodule update --init --recursive
-

.github/workflows/build-mudlet-win.yml (1)

35-36: Redundant submodule initialization.

The checkout action at line 32 already has submodules: true, making this explicit initialization redundant.

Consider removing this step or use submodules: recursive in the checkout action if recursive initialization is specifically needed:

-    - name: Init all submodules recursively
-      run: git submodule update --init --recursive
-

src/crash_reporter/CMakeLists.txt (1)

7-36: Avoid version duplication and make EXE_MUDLET_TARGET usage more robust

• APP_VERSION is hard-coded to 4.19.1 here while the main project already carries the version elsewhere (qmake VERSION and top-level CMake). This will drift on the next release and desynchronize Sentry crash reports from the actual app version. Prefer wiring this from the top-level (e.g. via a cache variable, project(VERSION ...), or a generated header) instead of hard-coding it locally.

• if(TARGET ${EXE_MUDLET_TARGET}) assumes EXE_MUDLET_TARGET is always defined. If this file is ever reused without that variable set, CMake will see if(TARGET ) and error. A safer pattern is:

if(DEFINED EXE_MUDLET_TARGET AND TARGET "${EXE_MUDLET_TARGET}")
    set_target_properties(MudletCrashReporter PROPERTIES
        RUNTIME_OUTPUT_DIRECTORY $<TARGET_FILE_DIR:${EXE_MUDLET_TARGET}>
    )
endif()

• If you ever include this crash reporter subdirectory from the same CMake build that defines the sentry_native ExternalProject (in WithSentry.cmake), consider adding an explicit dependency so the Sentry libraries are guaranteed to exist before linking, e.g.:

add_dependencies(MudletCrashReporter sentry_native)

This avoids order-of-build surprises when building everything purely via CMake.

CI/send_debug_files_to_sentry.sh (1)

79-81: Remove unused SCRIPT_DIR or put it to use

SCRIPT_DIR is computed but never referenced, which ShellCheck also flags (SC2034). Either drop the assignment or use it to e.g. resolve the script-relative path to send_debug_files_to_sentry.sh or other assets. Keeping unused locals around makes maintenance harder and can hide real issues in static-analysis noise.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: vadi2
Repo: Mudlet/Mudlet PR: 8515
File: test/CMakeLists.txt:56-58
Timestamp: 2025-11-14T14:16:57.626Z
Learning: In the Mudlet Windows MSYS2 build environment, the Qt6Keychain package (mingw-w64-x86_64-qtkeychain-qt6) provides the CMake target named `qt6keychain` (lowercase, non-namespaced), not the standard `Qt6Keychain::Qt6Keychain` target. The test/CMakeLists.txt and src/CMakeLists.txt files correctly use `qt6keychain` for both USE_OWN_QTKEYCHAIN=ON and USE_OWN_QTKEYCHAIN=OFF cases.

src/cmake/WithSentry.cmake (4)

11-12: This issue was previously flagged: Remove hard-coded toolchain and generator.

As noted in the previous review, forcing clang/clang++ and Ninja (lines 11-12, 16) breaks builds on systems where these tools are unavailable or when the main project uses a different toolchain (MSVC, GCC). The ExternalProject should inherit the parent's toolchain and generator.

Also applies to: 16-16

---

66-66: This issue was previously flagged: Remove APP_DIR_PATH compile definition.

As noted in the previous review, APP_DIR_PATH="${CMAKE_BINARY_DIR}/src" (lines 66, 72) bakes a build-time path into the binary that won't exist after installation. The runtime code should use QCoreApplication::applicationDirPath() instead.

Also applies to: 72-72

---

92-92: This issue was previously flagged: Unconditional curl linking will fail on Windows.

As noted in the previous review, curl is linked unconditionally (line 92) but SENTRY_TRANSPORT=curl is only set for UNIX AND NOT APPLE (line 20). On Windows or systems without libcurl, this will cause link failures. Either guard the curl link with UNIX AND NOT APPLE or verify that sentry-native provides it.

---

138-138: This issue was previously flagged: bash dependency breaks Windows builds.

As noted in the previous review, the bash command (line 138) will fail on Windows systems without bash. If this feature is CI-only, document it; otherwise, guard it with UNIX or provide a portable wrapper.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

src/crash_reporter/main.cpp (1)

24-28: Avoid private Sentry headers/transport internals and add proper error handling & cleanup.

This function still relies on Sentry’s internal transport details and lacks basic error handling/cleanup:

• Lines 24–28: Including "sentry_options.h" and "sentry_transport.h" exposes you to internal, unstable APIs. The public integration should only need "sentry.h".
• Lines 52–63: opts->transport and the sentry__transport_* calls are private/internal APIs; they depend on the layout of sentry_options_t and can break with Sentry upgrades. There’s also:
  • No check that the envelope file exists or is readable before calling sentry_envelope_read_from_file.
  • No logging on failure.
  • No sentry_options_free or sentry_envelope_free on any path (including the if (!opts || !envelope) early return), leaking resources.

Refactor this to use only the documented public API and clean up resources, e.g.:

-#include "sentry.h"
-#include "sentry_options.h"
-extern "C" {
-    #include "sentry_transport.h"
-}
+#include "sentry.h"
@@
 void sendCrashReport(const char *envelopePath)
 {
-    sentry_options_t *  opts = sentry_options_new();
-    sentry_envelope_t * envelope = sentry_envelope_read_from_file(envelopePath);
-
-    if (!opts || !envelope) {
-        return;
-    }
-    sentry_options_set_dsn(opts, SENTRY_DSN);
-    sentry__transport_startup(opts->transport, opts);
-    sentry__transport_send_envelope(opts->transport, envelope);
-    sentry__transport_shutdown(opts->transport, 30 * 1000);
+    QFileInfo fileInfo(envelopePath);
+    if (!fileInfo.exists() || !fileInfo.isReadable()) {
+        qWarning() << "Envelope file does not exist or is not readable:" << envelopePath;
+        return;
+    }
+
+    sentry_options_t *opts = sentry_options_new();
+    if (!opts) {
+        qWarning() << "Failed to create Sentry options";
+        return;
+    }
+
+    sentry_envelope_t *envelope = sentry_envelope_read_from_file(envelopePath);
+    if (!envelope) {
+        qWarning() << "Failed to read envelope from file:" << envelopePath;
+        sentry_options_free(opts);
+        return;
+    }
+
+    sentry_options_set_dsn(opts, SENTRY_DSN);
+
+    // Use public API: initialize, capture the envelope, then shut down.
+    if (sentry_init(opts) == 0) {
+        sentry_capture_envelope(envelope);  // Sentry now owns the envelope.
+        sentry_close();
+    } else {
+        qWarning() << "Failed to initialize Sentry";
+        sentry_envelope_free(envelope);
+        sentry_options_free(opts);
+    }
 }

Note: Adjust the exact calls if your sentry-native version differs, but the key points are:

• Drop the private headers and sentry__transport_* usage.
• Use sentry_init / sentry_capture_envelope / sentry_close as the public flow.
• Ensure every allocation (opts, envelope) is freed on failure paths.

Also applies to: 52-63

src/SentryWrapper.h (1)

1-8: Add header guard / pragma once and fix minor header hygiene (optional).

This header is small but should still follow typical header hygiene: add an include guard or #pragma once, and ensure there is a newline at EOF. If the project expects a GPL copyright header on all public headers (as in src/crash_reporter/main.cpp), consider adding the standard license block here for consistency.

src/mudlet-lua/lua/StringUtils.lua (2)

224-224: Potential infinite loop risk with zero-width matches.

While advancing by end_pos + 1 (line 235) generally prevents infinite loops, if rex.find returns start_pos == end_pos for a zero-width match at the end of the string, the next iteration will start beyond the string length. Consider adding a safeguard to detect when start_pos == end_pos == #str to explicitly break.

Apply this diff to add a safeguard:

     local start_pos, end_pos, cap = rex.find(str, pattern, last_end)
-    if not start_pos then
+    if not start_pos or (start_pos == end_pos and end_pos == #str) then
       table.insert(out, str:sub(last_end))
       break
     end

---

219-220: Add documentation for the new global function.

Since rex_gsub_custom is now part of the public API used across multiple modules, it should have documentation similar to other functions in this file. Include the purpose, parameters, return value, and any known limitations (e.g., multiple captures, return value compatibility).

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

src/SentryWrapper.cpp (1)

39-42: Cache path doc vs actual QStandardPaths layout

The comment (Lines 39–42) documents cache paths like ~/.cache/mudlet/sentry, but QStandardPaths::CacheLocation is already app‑specific. Appending "/mudlet/sentry" on Line 49 can easily yield something like ~/.cache/mudlet/mudlet/sentry (or ~/.cache/Mudlet/mudlet/sentry), which diverges from what’s documented.

To align behavior with the comment, either:

• Base this on GenericCacheLocation and append "mudlet/sentry", or
• Append only "/sentry" to CacheLocation and update the comment accordingly.

Also applies to: 49-49

src/SentryWrapper.h (2)

23-26: Avoid pulling Sentry/Qt internals into this public header

The header doesn’t use qScopeGuard or any Sentry types; only the implementation in SentryWrapper.cpp needs them. Including <QtCore/qscopeguard.h> and "sentry.h" here increases coupling and rebuild surface for every includer. Consider moving these includes into SentryWrapper.cpp and keeping the header limited to <string> and forward declarations.

---

34-34: Add trailing newline to satisfy linters

CodeFactor is flagging the missing newline at end of file. Adding a final newline after Line 34 will silence the warning and follow the usual POSIX text‑file convention.

src/SentryWrapper.cpp (2)

44-62: Wire sentry_close() to application shutdown rather than never calling it

initSentry() now initializes Sentry and leaves it running for the process lifetime, which is much better than the previous immediate sentry_close(). However, there is currently no shutdown at all, so queued events may not be flushed on clean exit and SDK resources are never released.

Consider:

• Ensuring initSentry() is only called once (e.g., with a static bool initialized guard), and
• Registering a single sentry_close() at application shutdown (e.g., via a static qScopeGuard, std::atexit, or a QCoreApplication::aboutToQuit handler).

This keeps crash reporting active while still following sentry‑native’s recommended init/close lifecycle.

---

91-91: Add trailing newline to satisfy linters

Similar to the header, CodeFactor is reporting a missing newline at end of file. Adding a final newline after Line 91 will clear the warning.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

src/SentryWrapper.cpp (1)

54-54: Remove redundant "mudlet" subdirectory nesting in cache path.

QStandardPaths::CacheLocation is application-specific and already includes the app name in the path. Mudlet sets setApplicationName("Mudlet") and setOrganizationName("Mudlet"), so CacheLocation on Linux resolves to ~/.cache/mudlet. Appending /mudlet/sentry creates redundant nesting: ~/.cache/mudlet/mudlet/sentry.

Other cache usages in the codebase—dlgProfilePreferences.cpp:3496 and TMedia.cpp:904—use CacheLocation directly without adding /mudlet. Align with this pattern:

-        QString path = QStandardPaths::writableLocation(QStandardPaths::CacheLocation) + "/mudlet/sentry";
+        QString path = QStandardPaths::writableLocation(QStandardPaths::CacheLocation) + "/sentry";

src/SentryWrapper.h (1)

23-26: Remove unused qscopeguard.h include.

The #include <QtCore/qscopeguard.h> is no longer needed. The original implementation used qScopeGuard to call sentry_close(), but that code was removed per previous review feedback. Since qscopeguard.h serves no purpose now, remove it to reduce unnecessary dependencies.

Apply this diff:

 #ifdef WITH_SENTRY
-    #include <QtCore/qscopeguard.h>
     #include "sentry.h"
 #endif

src/SentryWrapper.cpp (2)

53-53: Consider using Qt's applicationDirPath() for simplicity and consistency.

The current getExeDir() implementation works correctly across platforms, but the codebase already uses QCoreApplication::applicationDirPath() extensively elsewhere. Using Qt's API here would:

• Reduce platform-specific code
• Improve maintainability
• Align with established patterns in the codebase

Apply this diff:

-        std::string         runtimeAppDir = getExeDir();
+        std::string         runtimeAppDir = QCoreApplication::applicationDirPath().toStdString();

If you adopt this, you could also simplify or remove the getExeDir() function if it's not needed elsewhere. Note that initSentry() must be called after the QCoreApplication instance is created in main().

Also applies to: 62-63

---

78-114: Consider caching the executable directory for better performance.

The function is thread-safe, but it recomputes the executable path on every call. Since this path doesn't change during process lifetime, you could cache it:

 std::string getExeDir()
 {
-    static std::mutex mtx;
-    std::lock_guard<std::mutex> lock(mtx);
+    static std::once_flag flag;
+    static std::string cachedDir;
+    
+    std::call_once(flag, []() {
 
     #if defined(_WIN32) || defined(_WIN64)
         char path[MAX_PATH];
         GetModuleFileNameA(NULL, path, MAX_PATH);
         std::string exePath(path);
         size_t pos = exePath.find_last_of("\\/");
-        std::string dir = exePath.substr(0, pos);
-        std::replace(dir.begin(), dir.end(), '\\', '/');
-
-        return dir;
+        cachedDir = exePath.substr(0, pos);
+        std::replace(cachedDir.begin(), cachedDir.end(), '\\', '/');
     #elif defined(__APPLE__)
         char path[PATH_MAX];
         uint32_t size = sizeof(path);
         if (_NSGetExecutablePath(path, &size) != 0) {
-            return ".";
+            cachedDir = ".";
+            return;
         }
         std::string exePath(path);
         size_t pos = exePath.find_last_of("/");
         if (pos == std::string::npos) {
-            return exePath;
+            cachedDir = exePath;
+            return;
         }
-        return exePath.substr(0, pos);
+        cachedDir = exePath.substr(0, pos);
     #else
         char result[PATH_MAX];
         ssize_t count = readlink("/proc/self/exe", result, PATH_MAX);
         if (count <= 0) {
-            return std::string(".");
+            cachedDir = ".";
+            return;
         }
         std::string exePath(result, count);
         size_t pos = exePath.find_last_of("/");
-        return exePath.substr(0, pos);
+        cachedDir = exePath.substr(0, pos);
     #endif
+    });
+    
+    return cachedDir;
 }

Note: The static analysis warning about a race condition at line 106 (readlink("/proc/self/exe")) is a false positive—reading /proc/self/exe is safe and has no time-of-check-to-time-of-use issue.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

src/SentryWrapper.cpp (1)

78-82: Consider whether the mutex is necessary

The mutex protects getExeDir() for thread safety, but the executable path is constant throughout the process lifetime and the platform APIs used are thread-safe for reads. Even if multiple threads call simultaneously, they'd compute the same result and return by value (no shared mutable state).

The mutex adds overhead (minimal, but measureable if called frequently), though it provides defensive safety. If getExeDir() is only called once at startup via initSentry(), the mutex is harmless but not strictly necessary.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

src/mudlet.pro (1)

658-659: Add error handling for crash_reporter build.

The cmake commands to configure and build crash_reporter lack error checking. If these commands fail, the build continues silently, potentially leading to runtime issues.

Add error checking similar to the pattern used elsewhere in qmake:

-    system(cmake -S $$PWD/crash_reporter/ -B $$PWD/crash_reporter/build/ -DCMAKE_RUNTIME_OUTPUT_DIRECTORY=$$APP_DIR_PATH/)
-    system(cmake --build $$PWD/crash_reporter/build/)
+    !system(cmake -S $$PWD/crash_reporter/ -B $$PWD/crash_reporter/build/ -DCMAKE_RUNTIME_OUTPUT_DIRECTORY=$$APP_DIR_PATH/) {
+        error("Failed to configure crash_reporter with CMake")
+    }
+    !system(cmake --build $$PWD/crash_reporter/build/) {
+        error("Failed to build crash_reporter")
+    }

Apply the same pattern to the sentry-native build commands at lines 637-639.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: NicolasKeita
Repo: Mudlet/Mudlet PR: 8540
File: CI/package-mudlet-for-windows.sh:111-113
Timestamp: 2025-11-18T19:24:57.339Z
Learning: In CI/package-mudlet-for-windows.sh for the Mudlet project, when WITH_SENTRY is enabled, the Sentry binaries (crashpad_handler.exe and crashpad_wer.dll) are mandatory and the script should fail with an error if they are missing, rather than continuing with a warning.

Learnt from: NicolasKeita
Repo: Mudlet/Mudlet PR: 8540
File: CI/package-mudlet-for-windows.sh:111-113
Timestamp: 2025-11-18T19:24:57.339Z
Learning: In CI/package-mudlet-for-windows.sh for the Mudlet project, when WITH_SENTRY is enabled, the Sentry binaries (crashpad_handler.exe and crashpad_wer.dll) are mandatory and the script should fail with an error if they are missing, rather than continuing with a warning.