Add a script injector for debugging#3
Conversation
… to satisfy codeRabbit. Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
📝 WalkthroughWalkthroughAdds a CLI Changes
Sequence DiagramsequenceDiagram
participant User
participant Main as main.cpp
participant Settings as BrowserSettings
participant Session as SebSession
participant QWE as QWebEngineProfile
participant GitHub as GitHub API
User->>Main: start with --inject /path/script.js
Main->>Main: resolve absolute path
Main->>Settings: set injectedScript
Session->>Settings: read injectedScript
Session->>Session: open & read script file
Session->>QWE: register QWebEngineScript "InjectScript" (DocumentReady, include sub-frames)
Session->>GitHub: GET /repos/.../releases/latest
GitHub-->>Session: JSON { "tag_name": "vX.Y.Z" }
Session->>Session: parse & cache version
Session->>Session: build user-agent and append "SEB/<cached-version>"
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main.cpp`:
- Around line 90-92: findConfigPath() must be updated to recognize and skip the
new value-taking option(s) so applyEarlyEnvironment() won't misinterpret
injectedScript file paths as the early config/resource; update findConfigPath()
to treat the -i/--inject (and any other newly added options that accept a value)
exactly like -c/--config (skip the next token), and mirror the same change where
a second parsing branch exists (the code around settings.browser.injectedScript
and the duplicate logic at the 142-145 area) so both early and later parsing
paths stay in sync with value-taking options.
In `@src/seb_session.cpp`:
- Around line 123-137: The injector code that reads
settings_.browser.injectedScript and inserts a QWebEngineScript via
profile_->scripts()->insert must be disabled for production; wrap the entire
block in a developer-only guard (e.g. a compile-time DEV_BUILD macro or a
runtime explicit unsafe flag like settings_.unsafeEnableInjectedScript) so it
only runs when that developer/unsafe mode is enabled, and ensure production
sessions (normal mode) never honor settings_.browser.injectedScript;
additionally emit a clear, high-visibility log message when the unsafe injector
is enabled so operators can detect it.
- Around line 39-79: The function getCachedSebVersion currently blocks on a
GitHub request and overwrites cachedVersion with the remote tag, which causes
the User-Agent SEB/ token to claim a version the binary may not have; instead
initialize cachedVersion from the shipped runtime version (use
QApplication::applicationVersion()) and ensure the QNetworkReply handling does
not replace cachedVersion—if you still need the remote release, store it in a
separate variable (e.g. remoteTag or latestRemoteVersion) and make the network
fetch asynchronous (or remove the blocking QEventLoop/timeout) so startup isn't
delayed; also ensure the UserAgentHeader uses cachedVersion for the SEB/ token
and do not change that value based on the reply or tag, referencing
getCachedSebVersion, cachedVersion, manager, request, reply, and the tag
variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0db49291-cb6c-4aaf-ab0f-b0ee9f6a966c
📒 Files selected for processing (3)
src/main.cppsrc/seb_session.cppsrc/seb_settings.h
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main.cpp (1)
94-97: Consider validating that the inject file exists.
QFileInfo::absoluteFilePath()returns a path regardless of whether the file actually exists. If the user provides a nonexistent path, the error will surface later (presumably inseb_session.cpp). For a better user experience, consider failing early with a clear error message here.♻️ Optional: Add file existence check
if (parser.isSet("inject")) { - settings.browser.injectedScript = QFileInfo(parser.value("inject")).absoluteFilePath(); + QFileInfo injectInfo(parser.value("inject")); + if (!injectInfo.exists() || !injectInfo.isFile()) { + QTextStream err(stderr); + err << "Error: Inject script file not found: " << parser.value("inject") << Qt::endl; + return 1; + } + settings.browser.injectedScript = injectInfo.absoluteFilePath(); }Note: This would require moving the check to
main()afterapplyCommandLineOverrides()or changing the function signature to return a status.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.cpp` around lines 94 - 97, The code currently sets settings.browser.injectedScript to QFileInfo(parser.value("inject")).absoluteFilePath() without verifying the file exists; update the logic so when parser.isSet("inject") you check QFileInfo(parser.value("inject")).exists() and if it returns false emit a clear error and exit (or return a failure status) instead of storing the path; if applyCommandLineOverrides() cannot fail, move this existence check into main() after applyCommandLineOverrides() (or change applyCommandLineOverrides() to return a status) and ensure the error references the missing inject file and prevents proceeding to seb_session.cpp.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main.cpp`:
- Around line 94-97: The code currently sets settings.browser.injectedScript to
QFileInfo(parser.value("inject")).absoluteFilePath() without verifying the file
exists; update the logic so when parser.isSet("inject") you check
QFileInfo(parser.value("inject")).exists() and if it returns false emit a clear
error and exit (or return a failure status) instead of storing the path; if
applyCommandLineOverrides() cannot fail, move this existence check into main()
after applyCommandLineOverrides() (or change applyCommandLineOverrides() to
return a status) and ensure the error references the missing inject file and
prevents proceeding to seb_session.cpp.
Jvr2022
left a comment
Jvr2022
left a comment
There was a problem hiding this comment.
Could we maybe add this in a custom build instead of the whole program?
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/seb_session.cpp (1)
127-133: Verify that the injector is meant to run in an isolated world.Line 132 uses
QWebEngineScript::UserWorld, so hooks to page-owned globals/APIs are invisible to the page’s own JavaScript. If the injected file is supposed to instrument the site itself, this needsMainWorldor a configurable world.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/seb_session.cpp` around lines 127 - 133, The injected script currently uses QWebEngineScript::UserWorld (see script.setWorldId(...)), which isolates it from page scripts; if the intent is to instrument or interact with the page’s globals/APIs switch to QWebEngineScript::MainWorld or make the world configurable so callers choose UserWorld vs MainWorld; update the creation logic around QWebEngineScript script (script.setName("InjectScript"), script.setInjectionPoint(...), script.setWorldId(...)) to accept a parameter or configuration and set script.setWorldId(QWebEngineScript::MainWorld) when instrumentation of the page is required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/build.sh`:
- Around line 9-15: The script currently defaults CONFIG="debug" which makes
debug mode opt-out; change the default to CONFIG="release" and update the arg
parsing loop to set CONFIG="debug" only when an explicit debug flag is passed
(e.g., "-d" or "--debug"); keep the existing "-r"/"--release" handling so it
sets CONFIG="release" if present, and ensure you reference the CONFIG variable
and the argument checks in the for loop when implementing this change.
- Line 17: The qmake6 invocation reuses the same ${ROOT_DIR}/build directory
regardless of CONFIG causing mixed debug/release objects; modify the build.sh
logic around the qmake6 CONFIG+="${CONFIG}" ../seb-linux-qt.pro call to either
(A) select a per-configuration build directory (e.g., use
BUILD_DIR="${ROOT_DIR}/build/${CONFIG}" and run qmake6 from there) or (B) detect
when the previous CONFIG differs from the current CONFIG and run a clean (make
clean) before re-running qmake6, ensuring the build directory is created/changed
appropriately and old object files are removed when CONFIG changes.
In `@src/seb_session.cpp`:
- Around line 443-445: Trim settings_.browser.customUserAgent before deciding to
use it: compute a trimmed string (reuse the local variable agent or a temporary)
and test trimmed.isEmpty() together with settings_.browser.useCustomUserAgent so
whitespace-only values fall back to the default; if non-empty after trimming,
assign the trimmed value to agent (instead of trimming only after the check).
---
Nitpick comments:
In `@src/seb_session.cpp`:
- Around line 127-133: The injected script currently uses
QWebEngineScript::UserWorld (see script.setWorldId(...)), which isolates it from
page scripts; if the intent is to instrument or interact with the page’s
globals/APIs switch to QWebEngineScript::MainWorld or make the world
configurable so callers choose UserWorld vs MainWorld; update the creation logic
around QWebEngineScript script (script.setName("InjectScript"),
script.setInjectionPoint(...), script.setWorldId(...)) to accept a parameter or
configuration and set script.setWorldId(QWebEngineScript::MainWorld) when
instrumentation of the page is required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 970da325-2e9d-4227-b220-f0c4816ec0af
📒 Files selected for processing (3)
scripts/build.shsrc/main.cppsrc/seb_session.cpp
| fi | ||
| done | ||
|
|
||
| qmake6 CONFIG+="${CONFIG}" ../seb-linux-qt.pro |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n scripts/build.sh | head -40Repository: Jvr2022/seb-linux
Length of output: 697
🏁 Script executed:
# Search for any documentation or comments in the repo about build configuration
rg -i "config|debug|release" scripts/ -A 2 -B 2Repository: Jvr2022/seb-linux
Length of output: 675
🌐 Web query:
qmake6 CONFIG mode changes same build directory object reuse detection
💡 Result:
qmake (including qmake6 in Qt6) does not have a specific CONFIG option to control object file reuse detection when changing CONFIG modes (like switching between debug/release) in the same build directory. qmake generates dependency information based on source files, headers (via DEPENDPATH), and Makefile regeneration triggers, but CONFIG changes primarily affect which Makefile is used (e.g., Makefile.Debug vs Makefile.Release when CONFIG += debug_and_release). Key behaviors: - When using CONFIG += debug_and_release (default on Windows, optional elsewhere), qmake generates separate Makefiles for each mode: Makefile, Makefile.Debug, Makefile.Release. Building 'debug' uses Makefile.Debug (which builds objects with debug flags), 'release' uses Makefile.Release (release flags). Objects are typically placed in mode-specific directories if configured via OBJECTS_DIR in CONFIG(debug,debug|release) scopes (e.g., OBJECTS_DIR = .obj/debug). Thus, objects are not shared/reused across modes due to different compiler flags and usually separate paths. - In the same build directory without debug_and_release, changing CONFIG (e.g., adding/removing 'debug') invalidates the Makefile, forcing qmake re-run and full rebuild (no incremental object reuse across config changes). - No evidence of qmake6-specific changes to this behavior; Qt6 docs confirm qmake remains supported for apps with same core logic. - To avoid rebuilds on CONFIG changes, use separate build directories per config (shadow builds, recommended). For object reuse across subprojects, object files are not automatically shared; each subdir rebuilds unless using static libs. - Use 'qmake -nodepend' to disable dependency generation (not recommended, breaks incremental builds). No official CONFIG like 'object_reuse_detection' exists. Best practice: separate OBJECTS_DIR per config, separate build dirs.
Citations:
- 1: https://stackoverflow.com/questions/2580934/how-to-specify-different-debug-release-output-directories-in-qmake-pro-file
- 2: https://forum.qt.io/topic/138588/qmake-obj_dir-behaves-differently-on-linux-than-on-windows
- 3: https://doc.qt.io/qtcreator/creator-build-settings-qmake.html
- 4: https://doc.qt.io/qt-6/qmake-running.html
- 5: https://doc.qt.io/qt-6.6/qmake-variable-reference.html
- 6: https://stackoverflow.com/questions/5470438/why-are-changes-in-source-not-always-reflected-in-machine-code-after-build-while
- 7: https://stackoverflow.com/questions/11913770/what-are-the-merits-of-debug-and-release-config-feature-in-qmake
- 8: https://stackoverflow.com/questions/16961866/qmake-how-exactly-does-qmake-interpret-the-configdebug-debugrelease-synta
- 9: https://stackoverflow.com/questions/7114564/qmake-in-release-and-debug-mode
- 10: https://forum.qt.io/topic/2323/qmake-with-debug_and_release
- 11: https://doc.qt.io/qt-5/qmake-running.html
🏁 Script executed:
find . -name "seb-linux-qt.pro" -o -name "*.pro" | head -5Repository: Jvr2022/seb-linux
Length of output: 151
🏁 Script executed:
cat -n seb-linux-qt.pro | head -50Repository: Jvr2022/seb-linux
Length of output: 2506
🏁 Script executed:
cat seb-linux-qt.pro | grep -i "debug_and_release\|CONFIG.*debug\|CONFIG.*release\|OBJECTS_DIR.*CONFIG"Repository: Jvr2022/seb-linux
Length of output: 43
🏁 Script executed:
wc -l seb-linux-qt.pro && tail -50 seb-linux-qt.proRepository: Jvr2022/seb-linux
Length of output: 2692
🏁 Script executed:
grep -n "CONFIG\|debug_and_release" seb-linux-qt.proRepository: Jvr2022/seb-linux
Length of output: 120
🏁 Script executed:
grep -n "include\|qt_module" seb-linux-qt.pro | head -20Repository: Jvr2022/seb-linux
Length of output: 43
🏁 Script executed:
# Check if there are any cache/state files generated by qmake that might track CONFIG
find . -name ".qmake*" -o -name "*.stash" -o -name "Makefile*" | grep -v ".git" | head -20Repository: Jvr2022/seb-linux
Length of output: 43
🌐 Web query:
Qt6 qmake CONFIG change detection same build directory rebuild behavior
💡 Result:
In Qt6, qmake does not automatically detect changes to the CONFIG variable (or other .pro file changes) in the same build directory during incremental builds with make/nmake. The generated Makefile lacks a dependency on the .pro file itself, so changes to CONFIG (e.g., adding debug, release, or custom features) do not trigger qmake regeneration or rebuilds of affected targets. To apply CONFIG changes: - Manually run qmake in the build directory to regenerate the Makefile. - In Qt Creator: Right-click project > Run qmake, or use Build > Run qmake, or delete Makefile(s) to force regeneration. - Use 'make clean' followed by 'make', but this performs a full rebuild. This behavior persists from Qt5 to Qt6 as qmake remains supported for applications. Official docs confirm qmake usage but note CMake preference for Qt6 builds. Qt Creator may not always auto-run qmake on .pro changes (known longstanding issue). Shadow builds (separate build dir) are recommended to avoid source pollution, but the issue remains for .pro changes.
Citations:
- 1: https://doc.qt.io/qt-6/qt6-buildsystem.html
- 2: https://doc.qt.io/qt-6.6/qmake-advanced-usage.html
- 3: https://doc.qt.io/qt-6/qmake-variable-reference.html
- 4: https://doc.qt.io/qt-6/qmake-manual.html
- 5: https://stackoverflow.com/questions/23026868/qt-not-refreshing-makefiles-after-changes-in-the-pro-file
- 6: https://doc.qt.io/qtcreator/creator-build-settings-qmake.html
- 7: https://doc.qt.io/qt-6/qmake-tutorial.html
- 8: https://doc.qt.io/qt-6/qmake-running.html
- 9: https://stackoverflow.com/questions/31520738/why-do-i-need-to-remove-build-directory-after-changing-something-in-a-qt-pro-fi
- 10: https://stackoverflow.com/questions/26771306/how-to-tell-qt-creator-to-parse-pro-file-when-i-build-after-changing-it
- 11: https://stackoverflow.com/questions/12873437/force-qtcreator-to-run-qmake-when-building
🏁 Script executed:
# Check what happens when qmake runs with CONFIG changes
# Show the build.sh logic again for clarity
cat -n scripts/build.shRepository: Jvr2022/seb-linux
Length of output: 697
Use separate build directories or force clean rebuild when CONFIG changes.
Line 17 reconfigures qmake6 in the same ${ROOT_DIR}/build directory for both debug and release modes. Qt6 qmake does not track CONFIG variable changes in the Makefile, so switching from debug to release (or vice versa) will reuse object files compiled with the previous configuration's compiler flags. This results in binaries with mixed debug/release code and unintended debug overhead.
Solutions: (1) Use separate build directories per mode (e.g., build/debug, build/release), or (2) Detect CONFIG changes and perform make clean before rebuilding.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/build.sh` at line 17, The qmake6 invocation reuses the same
${ROOT_DIR}/build directory regardless of CONFIG causing mixed debug/release
objects; modify the build.sh logic around the qmake6 CONFIG+="${CONFIG}"
../seb-linux-qt.pro call to either (A) select a per-configuration build
directory (e.g., use BUILD_DIR="${ROOT_DIR}/build/${CONFIG}" and run qmake6 from
there) or (B) detect when the previous CONFIG differs from the current CONFIG
and run a clean (make clean) before re-running qmake6, ensuring the build
directory is created/changed appropriately and old object files are removed when
CONFIG changes.
| if (settings_.browser.useCustomUserAgent && !settings_.browser.customUserAgent.isEmpty()) { | ||
| agent = settings_.browser.customUserAgent.trimmed(); | ||
| } else { |
There was a problem hiding this comment.
Trim before deciding whether to honor customUserAgent.
Line 443 only checks isEmpty(). A whitespace-only customUserAgent therefore skips the default UA, then Line 444 trims it to empty and the header degrades to just SEB/... instead of a valid browser UA.
🛠️ Proposed fix
- if (settings_.browser.useCustomUserAgent && !settings_.browser.customUserAgent.isEmpty()) {
- agent = settings_.browser.customUserAgent.trimmed();
+ const QString customAgent = settings_.browser.customUserAgent.trimmed();
+ if (settings_.browser.useCustomUserAgent && !customAgent.isEmpty()) {
+ agent = customAgent;
} else {📝 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.
| if (settings_.browser.useCustomUserAgent && !settings_.browser.customUserAgent.isEmpty()) { | |
| agent = settings_.browser.customUserAgent.trimmed(); | |
| } else { | |
| const QString customAgent = settings_.browser.customUserAgent.trimmed(); | |
| if (settings_.browser.useCustomUserAgent && !customAgent.isEmpty()) { | |
| agent = customAgent; | |
| } else { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/seb_session.cpp` around lines 443 - 445, Trim
settings_.browser.customUserAgent before deciding to use it: compute a trimmed
string (reuse the local variable agent or a temporary) and test
trimmed.isEmpty() together with settings_.browser.useCustomUserAgent so
whitespace-only values fall back to the default; if non-empty after trimming,
assign the trimmed value to agent (instead of trimming only after the check).
2 participants
Summary
Add a script injector for debugging
What Changed
Added a
-i/--injectflag for injecting a javascript script that will run on each page loaded in order to more easily debug problems in the internal web engine.Verification
Builds on my machine and correctly injects script
Summary by CodeRabbit
New Features
Chores