fix lint - #3
Conversation
WalkthroughAdded end-to-end test scaffolding and CI changes, corrected README plugin naming, made stylistic formatting tweaks in Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Job
participant Cypress as Cypress Runner
participant FlaskCLI as flask --app create_app(...)
participant App as Flask App
participant Plugin as b4uleave after_request
CI->>FlaskCLI: start Flask using app factory (create_app(config_path))
activate FlaskCLI
FlaskCLI->>App: initialize app (register routes)
App->>App: root route serves static HTML
CI->>Cypress: run tests (npx cypress run) against baseUrl
Cypress->>App: GET /
App->>Plugin: after_request -> may inject modal HTML (if enabled)
Plugin-->>App: modified HTML response
App-->>Cypress: 200/404 response with content
deactivate FlaskCLI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
platzky_b4uleave/entrypoint.py (2)
1-7: ExposeFlaskonly for typing viaTYPE_CHECKING.
appis an opaque positional parameter right now. Importing theFlaskclass only inside atyping.TYPE_CHECKINGguard gives IDE/introspection benefits without adding a hard runtime dependency (and keeps the linter happy since you removed the unused import).-from flask import Response +from flask import Response +from typing import TYPE_CHECKING + +if TYPE_CHECKING: # pragma: no cover + from flask import Flask # noqa: F401 (import used only for type-checking)…and then annotate the first argument:
-def process( - app, plugin_config: Dict[str, Any] +def process( + app: "Flask", plugin_config: Dict[str, Any]
17-22: Micro-optimisation: cache compiled HTML once.
add_B4ULeaverebuilds the giant HTML/JS snippet on every request.
Build it once outside the handler (aftermessage/stay/leaveare known) and reuse; this cuts down string formatting and reduces per-request overhead.
| message = app.config["b4uleave"].get( | ||
| "message", "Czy na pewno chcesz<br>opuścić naszą stronę?" | ||
| ) | ||
| stay = app.config["b4uleave"].get("stay", "Stay") | ||
| leave = app.config["b4uleave"].get("leave", "Leave") | ||
|
|
There was a problem hiding this comment.
Unsanitised user-controlled HTML ⇒ reflected-XSS vector.
message, stay, and leave come straight from plugin_config and are interpolated into the DOM without escaping.
An attacker configuring the plugin (or compromising the config source) can inject arbitrary scripts.
-import ...
+import html # at top
...
-message = app.config["b4uleave"].get(
- "message", "Czy na pewno chcesz<br>opuścić naszą stronę?"
-)
-stay = app.config["b4uleave"].get("stay", "Stay")
-leave = app.config["b4uleave"].get("leave", "Leave")
+cfg = app.config["b4uleave"]
+message = html.escape(cfg.get("message", "Czy na pewno chcesz<br>opuścić naszą stronę?"), quote=True)
+stay = html.escape(cfg.get("stay", "Stay"), quote=True)
+leave = html.escape(cfg.get("leave", "Leave"), quote=True)If you intentionally allow HTML in message, use flask.Markup to whitelist only that field and still escape the others.
Also applies to: 69-73
🤖 Prompt for AI Agents
In platzky_b4uleave/entrypoint.py around lines 11 to 16, the variables message,
stay, and leave are taken directly from the config and inserted into the DOM
without escaping, creating a reflected XSS risk. To fix this, apply proper
escaping to stay and leave to prevent script injection, and if HTML is
intentionally allowed in message, wrap it with flask.Markup to safely mark it as
trusted HTML. Also apply the same escaping and marking approach to the similar
code at lines 69 to 73.
| ) | ||
| return response |
There was a problem hiding this comment.
🛠️ Refactor suggestion
"</body>" string replace is brittle; use a real parser.
replace("</body>", html + "</body>")
• fails for <BODY> / mixed-case tags,
• injects nothing if the tag is missing,
• may break when the string appears inside a <script> or <textarea>.
Consider BeautifulSoup or lxml.html for robust insertion:
from bs4 import BeautifulSoup
soup = BeautifulSoup(response.get_data(as_text=True), "html.parser")
if soup.body:
soup.body.append(BeautifulSoup(html, "html.parser"))
response.set_data(str(soup))At minimum, perform a case-insensitive search and fall back gracefully.
🤖 Prompt for AI Agents
In platzky_b4uleave/entrypoint.py around lines 115 to 116, the code uses a
brittle string replace to insert HTML before the closing </body> tag, which
fails with case variations, missing tags, or when the string appears inside
scripts. Replace this with a robust HTML parser like BeautifulSoup: parse the
response data, check for the body tag, append the new HTML safely, and then set
the modified HTML back to the response. This ensures correct insertion
regardless of tag case or content structure.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/tests.yml (2)
49-54: Inline one-liner Flask app is brittle & hard to readPacking the entire Flask micro-app into a single
echoline with multiple semicolons is error-prone and painful to maintain.
A minimal refactor dramatically improves clarity while retaining the same behaviour:- echo "from flask import Flask; from platzky_b4uleave.entrypoint import process; app = Flask(__name__); app.route('/')(process)" > temp_app.py + cat > temp_app.py <<'PY' +from flask import Flask +from platzky_b4uleave.entrypoint import process + +app = Flask(__name__) +app.route('/')(process) +PYNo functional change, just readability and lower risk of quoting bugs.
(Optional but strongly recommended.)
42-46: Consider caching & pinning Cypress installation
npm install cypressre-downloads Cypress on every run, which slows the workflow.
Using the officialcypress-io/github-action(or at minimum enabling Node-module caching withactions/setup-node’scache: 'npm') will cut several minutes off CI times and improve reliability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/tests.yml(1 hunks)tests/unit_tests/test_b4uleave.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit_tests/test_b4uleave.py
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/tests.yml (1)
56-56: Remove trailing whitespace to keep YAML lint-cleanLine 56 has trailing spaces flagged by YAMLlint. Deleting them avoids CI noise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/tests.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/tests.yml
[error] 56-56: trailing spaces
(trailing-spaces)
🔇 Additional comments (3)
.github/workflows/tests.yml (3)
61-63: Confirm Cypress spec glob is correct relative to working directory
working-directoryis alreadytests/e2e_tests, so"cypress/e2e/**/*"resolves totests/e2e_tests/cypress/e2e/**/*. Verify this matches the repo layout; otherwise the run will report “No tests found”.
47-50: Hosts file addition looks goodBinding the Flask server to
goodmap.localhostand pre-populating/etc/hostsis clear and portable.
53-58: PID management still inconsistent—Flask process will not be terminatedYou write the PID to
flask.pid, but never exportFLASK_PID. Consequently, the “Stop Flask server” step sends a signal to an empty variable, leaving the background Flask process running and subsequent jobs flaky.- echo $! > flask.pid + echo $! > flask.pid + export FLASK_PID=$!or simply change the kill step:
- run: kill $FLASK_PID || true + run: kill $(cat flask.pid) || trueLikely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/tests.yml (1)
30-30: Trailing whitespace flagged by YAML-lintLines 30 and 52 contain superfluous spaces, which YAML-lint reports as errors. Remove the extra spaces to keep the workflow lint-clean.
Also applies to: 52-52
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/tests.yml(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/tests.yml
[error] 30-30: trailing spaces
(trailing-spaces)
[error] 52-52: trailing spaces
(trailing-spaces)
[error] 61-61: syntax error: could not find expected ':'
(syntax)
🪛 actionlint (1.7.7)
.github/workflows/tests.yml
60-60: could not parse as YAML: yaml: line 60: could not find expected ':'
(syntax-check)
🔇 Additional comments (1)
.github/workflows/tests.yml (1)
66-68:Stop Flask serverstep is mis-indented and refers to an undefined PID
- The
- name:line is at the root level, not inside thesteps:list, so even after fixing the conflict markers this will still break YAML.$FLASK_PIDis never exported now that the Flask server start step is commented out, sokill $FLASK_PIDwill do nothing.Either:
• Drop this step entirely, or
• Re-enable the server start step and exportFLASK_PID, then indent this block with four spaces so it’s a sibling of the other steps.Example removal:
-# Stop the Flask server using the saved process ID -- name: Stop Flask server - run: kill $FLASK_PID || true
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/tests.yml(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/tests.yml
[error] 30-30: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
.github/workflows/tests.yml (1)
30-30: Remove trailing whitespace to satisfy lintersLine 30 has stray spaces flagged by YAML-lint. Delete them to keep the workflow lint-clean.
[ suggest_nitpick ]
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
tests/e2e_tests/cypress.config.js (1)
5-5: Option: prefer 127.0.0.1 to avoid IPv6 localhost pitfalls on runnersSome runners resolve localhost to ::1 which can cause transient connect issues. Consider pinning IPv4.
Apply this minimal change:
- baseUrl: 'http://localhost:5000/', + baseUrl: 'http://127.0.0.1:5000/',tests/e2e_tests/e2e_test_config.yml (3)
8-8: Remove stray TODO comment from committed test configKeep committed configs free of TODOs; track this in an issue if needed.
Apply:
-#TODO this should not be necessary argument
20-21: Normalize YAML booleans to lowercase (style/lint consistency)Lowercase booleans are commonly enforced by linters.
Apply:
- USE_LAZY_LOADING: True - SHOW_ACCESSIBILITY_TABLE: True + USE_LAZY_LOADING: true + SHOW_ACCESSIBILITY_TABLE: true
21-21: Add trailing newline at EOFYAMLlint flagged “no new line character at the end of file”. Configure your editor to add a newline on save.
tests/e2e_tests/e2e_app.py (1)
5-7: Add return type and import Flask for clarity and toolingHelps IDEs and type checkers; no runtime impact.
Apply:
-from flask import render_template_string +from flask import Flask, render_template_string @@ -def create_app(config_path: str): +def create_app(config_path: str) -> Flask:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.github/workflows/tests.yml(1 hunks)tests/e2e_tests/cypress.config.js(1 hunks)tests/e2e_tests/e2e_app.py(1 hunks)tests/e2e_tests/e2e_test_config.yml(1 hunks)tests/e2e_tests/e2e_test_data.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/e2e_tests/e2e_test_data.json
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e_tests/e2e_app.py (1)
tests/unit_tests/test_b4uleave.py (1)
test_that_plugin_loads_b4uleave(5-53)
🪛 YAMLlint (1.37.1)
tests/e2e_tests/e2e_test_config.yml
[error] 21-21: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (3)
tests/e2e_tests/cypress.config.js (1)
5-5: LGTM: baseUrl aligns with the workflowMatches Flask binding to localhost:5000. Nothing else to change here.
tests/e2e_tests/e2e_test_config.yml (1)
4-7: Verify JSON file DB driver registration
- Data file
tests/e2e_tests/e2e_test_data.jsonexists and contains the"name": "b4uleave"entry.- Confirm the app factory supports
DB.TYPE: json_fileand correctly loads its data from the specifiedDB.PATH.tests/e2e_tests/e2e_app.py (1)
9-29: LGTM: minimal HTML shell suitable for plugin injectionRoute wiring and template are fine for the e2e scenario.
| poetry run flask --app "tests.e2e_tests.e2e_app:create_app(config_path='tests/e2e_tests/e2e_test_config.yml')" run --debug --host=localhost & | ||
| sleep 6 | ||
| cd tests/e2e_tests && npx cypress run --spec "cypress/e2e/**/*" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden e2e step: avoid reloader, wait for readiness, and ensure clean teardown
Current approach uses --debug (spawns a reloader) and a fixed sleep which can flake. Start a single process, wait until it’s responsive, and always kill it on exit.
Apply:
- poetry run flask --app "tests.e2e_tests.e2e_app:create_app(config_path='tests/e2e_tests/e2e_test_config.yml')" run --debug --host=localhost &
- sleep 6
- cd tests/e2e_tests && npx cypress run --spec "cypress/e2e/**/*"
+ set -euo pipefail
+ poetry run flask --app "tests.e2e_tests.e2e_app:create_app(config_path='tests/e2e_tests/e2e_test_config.yml')" run --host=127.0.0.1 --port=5000 --no-reload > flask.log 2>&1 &
+ SERVER_PID=$!
+ trap 'kill "$SERVER_PID" || true' EXIT
+ # Wait up to 30s for server readiness
+ for i in {1..30}; do
+ curl -fsS http://127.0.0.1:5000/ >/dev/null && break
+ sleep 1
+ done
+ cd tests/e2e_tests
+ npx cypress run --spec "cypress/e2e/**/*"Optional: upload flask.log on failure to aid debugging.
📝 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.
| poetry run flask --app "tests.e2e_tests.e2e_app:create_app(config_path='tests/e2e_tests/e2e_test_config.yml')" run --debug --host=localhost & | |
| sleep 6 | |
| cd tests/e2e_tests && npx cypress run --spec "cypress/e2e/**/*" | |
| set -euo pipefail | |
| poetry run flask --app "tests.e2e_tests.e2e_app:create_app(config_path='tests/e2e_tests/e2e_test_config.yml')" \ | |
| run --host=127.0.0.1 --port=5000 --no-reload > flask.log 2>&1 & | |
| SERVER_PID=$! | |
| trap 'kill "$SERVER_PID" || true' EXIT | |
| # Wait up to 30s for server readiness | |
| for i in {1..30}; do | |
| curl -fsS http://127.0.0.1:5000/ >/dev/null && break | |
| sleep 1 | |
| done | |
| cd tests/e2e_tests | |
| npx cypress run --spec "cypress/e2e/**/*" |
🤖 Prompt for AI Agents
.github/workflows/tests.yml lines 48-50: the e2e job currently launches Flask
with --debug (which spawns a reloader) and uses a fixed sleep, causing flakiness
and orphaned processes; change to start a single, non-reloading Flask process
(use the CLI flag to disable the reloader), run it in background capturing its
PID, install a trap/cleanup to always kill that PID on EXIT, replace the fixed
sleep with a readiness probe loop (poll the app URL with a short timeout and
bounded retries until a 200/healthy response or fail), and on failure upload
flask.log for debugging; ensure the readiness loop times out and exits non-zero
so Cypress doesn’t run against an unready server.
2 participants
Summary by CodeRabbit
Style
Documentation
Bug Fixes
Chores
New Features (Tests)