Merged
Conversation
Reviewer's GuideSwitches the runtime from Hypercorn/Django runserver to a supervised Uvicorn-based setup, tightens SQLite/Huey/caching configuration, adds a quick-request UX on posters, and removes the DRF-based API and old static/minification stack in favor of ServeStatic and simpler front-end behavior. Sequence diagram for poster quick-request background flowsequenceDiagram
actor User
participant Browser
participant FrontendJS
participant Django_view_request_content as DjangoView_request_content
participant Huey
participant Sonarr_task as SonarrRequestTask
participant Radarr_task as RadarrRequestTask
User->>Browser: Click quick-request button on poster
Browser->>FrontendJS: quick_request_click_event handler
FrontendJS->>FrontendJS: Check ongoing_request guard
FrontendJS->>Django_view_request_content: POST request_content(tmdb_id, content_type, seasons=null, episode_ids=null)
Django_view_request_content->>Huey: Enqueue background task based on content_type
alt TV content
Huey->>Sonarr_task: sonarr_request_background_task(tvdb_id, request_params, sonarr_params)
else Movie content
Huey->>Radarr_task: radarr_request_background_task(tmdb_id, radarr_params)
end
Django_view_request_content-->>FrontendJS: 200 OK
FrontendJS-->>Browser: requested_toast_message(), remove quick-request button
Browser-->>User: Show toast and updated poster state
Sonarr_task->>Sonarr_task: Retry on failure (retries=25, retry_delay=300)
Radarr_task->>Radarr_task: Retry on failure (retries=25, retry_delay=300)
Sequence diagram for supervised Uvicorn and Huey processessequenceDiagram
participant CLI as CLI_run_conreq
participant Django_mgmt as DjangoManagement
participant Supervisor as run_conreq_Command
participant WebProc as UvicornProcess
participant HueyProc as HueyProcess
participant MainDB as SQLite_main
participant HueyDB as SQLite_huey
CLI->>Django_mgmt: manage.py run_conreq --port
Django_mgmt->>Supervisor: handle(port, options)
Supervisor->>Supervisor: cache.clear()
Supervisor->>Django_mgmt: preconfig_conreq
Supervisor->>Django_mgmt: check --deploy
Supervisor->>Django_mgmt: test --noinput --parallel --failfast
Supervisor->>Django_mgmt: migrate --noinput
alt not DEBUG
Supervisor->>Django_mgmt: collectstatic --link --clear --noinput
Supervisor->>Django_mgmt: compress --force
end
Supervisor->>HueyProc: start_huey() in Process
activate HueyProc
HueyProc->>HueyProc: django.setup()
HueyProc->>HueyDB: run_huey (consumer workers)
Supervisor->>WebProc: start_webserver(port) in Process
activate WebProc
WebProc->>WebProc: django.setup()
WebProc->>WebProc: uvicorn.run(conreq.asgi:application)
WebProc->>MainDB: Handle HTTP and WebSocket traffic
loop Supervision loop
Supervisor->>Supervisor: sleep(5)
alt HueyProc crashed
Supervisor->>Supervisor: log warning
Supervisor->>HueyProc: restart Process(start_huey)
end
alt WebProc crashed
Supervisor->>Supervisor: log warning
Supervisor->>WebProc: restart Process(start_webserver)
end
end
Class diagram for ExtendedURLValidator changesclassDiagram
class URLValidator {
<<django.core.validators>>
+ipv4_re
+ipv6_re
+hostname_re
+domain_re
+tld_re
+regex
+__call__(value)
}
class ExtendedURLValidator {
+ipv4_re
+ipv6_re
+hostname_re
+domain_re
+tld_re
+host_re
+regex
+__call__(value)
}
URLValidator <|-- ExtendedURLValidator
ExtendedURLValidator : ipv4_re = URLValidator.ipv4_re
ExtendedURLValidator : ipv6_re = URLValidator.ipv6_re
ExtendedURLValidator : hostname_re = URLValidator.hostname_re
ExtendedURLValidator : domain_re = URLValidator.domain_re
ExtendedURLValidator : tld_re = URLValidator.tld_re
ExtendedURLValidator : host_re = hostname_re + domain_re + tld_re + "|localhost|\w+"
ExtendedURLValidator : regex = compiled pattern
ExtendedURLValidator : __call__(value) validates
ExtendedURLValidator : - host-style URLs (no TLD)
ExtendedURLValidator : - localhost and bare hosts
ExtendedURLValidator : - ports 0-65535 via [0-9]{1,5} pattern
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- The new process supervision loop in
run_conreq.pyruns forever without any shutdown handling and recreates non-daemon processes; consider catchingKeyboardInterrupt/termination signals to cleanly terminate/join the webserver and Huey processes and avoid orphaned children or tight respawn loops. - In
ExtendedURLValidator,host_re/regexare defined twice and thehost_renow allows\w+which may admit unintended hostnames; it would be clearer to collapse to a singleregexdefinition and precisely document/limit the additional host-style patterns you intend to support. - The SQLite
DATABASES['default']['OPTIONS']use ofinit_command/transaction_modeis not a documented option for Django's SQLite backend; you may want to move the PRAGMA configuration into a connection-initialization hook (e.g.connection_createdsignal) to ensure these settings are reliably applied.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new process supervision loop in `run_conreq.py` runs forever without any shutdown handling and recreates non-daemon processes; consider catching `KeyboardInterrupt`/termination signals to cleanly terminate/join the webserver and Huey processes and avoid orphaned children or tight respawn loops.
- In `ExtendedURLValidator`, `host_re`/`regex` are defined twice and the `host_re` now allows `\w+` which may admit unintended hostnames; it would be clearer to collapse to a single `regex` definition and precisely document/limit the additional host-style patterns you intend to support.
- The SQLite `DATABASES['default']['OPTIONS']` use of `init_command`/`transaction_mode` is not a documented option for Django's SQLite backend; you may want to move the PRAGMA configuration into a connection-initialization hook (e.g. `connection_created` signal) to ensure these settings are reliably applied.
## Individual Comments
### Comment 1
<location> `conreq/settings.py:349-358` </location>
<code_context>
"NAME": os.path.join(DATA_DIR, "db.sqlite3"),
"OPTIONS": {
- "timeout": 3, # 3 second query timeout
+ "init_command": (
+ "PRAGMA foreign_keys = ON;"
+ "PRAGMA journal_mode = WAL;"
+ "PRAGMA synchronous = NORMAL;"
+ "PRAGMA busy_timeout = 10000;"
+ "PRAGMA temp_store = MEMORY;"
+ "PRAGMA mmap_size = 134217728;"
+ "PRAGMA journal_size_limit = 67108864;"
+ "PRAGMA cache_size = 2000;"
+ ),
+ "transaction_mode": "IMMEDIATE",
},
}
</code_context>
<issue_to_address>
**issue (bug_risk):** The SQLite `OPTIONS` use unsupported parameters that will likely break database connections.
For SQLite, Django passes `OPTIONS` straight into `sqlite3.connect(**OPTIONS)`, which does not accept `init_command` or `transaction_mode`. This will raise `TypeError: connect() got an unexpected keyword argument 'init_command'` at startup. To apply these PRAGMAs, move them into `connection.execute()` (e.g., via a `connection_created` signal, connection wrapper, or custom backend). Only options actually supported by `sqlite3.connect` (such as `isolation_level` and `timeout`) should stay under `OPTIONS`.
</issue_to_address>
### Comment 2
<location> `conreq/core/base/validators.py:19` </location>
<code_context>
- r"(?<!-)" # can't end with a dash
- r"\.?" # may have a trailing dot
- )
+ host_re = "(" + hostname_re + domain_re + tld_re + "|localhost|\w+)"
- host_re = "(" + hostname_re + domain_re + tld_re + "|localhost)"
</code_context>
<issue_to_address>
**issue (bug_risk):** The `\w+` fragment in `host_re` is not a raw string, which can lead to incorrect or deprecated escape behavior.
In Python, `"...|\w+)"` as a normal string triggers a deprecation warning today and may become an error, and the compiled regex may not match as intended. Please make this part of the pattern raw or escaped, e.g.:
```python
host_re = r"(" + hostname_re + domain_re + tld_re + r"|localhost|\w+)"
```
or otherwise construct the pattern so `\w+` appears inside a raw string literal.
</issue_to_address>
### Comment 3
<location> `conreq/core/base/validators.py:21-28` </location>
<code_context>
+ host_re = "(" + hostname_re + domain_re + tld_re + "|localhost|\w+)"
- host_re = "(" + hostname_re + domain_re + tld_re + "|localhost)"
+ regex = _lazy_re_compile(
+ r"^(?:[a-z0-9.+-]*)://" # scheme is validated separately
+ r"(?:[^\s:@/]+(?::[^\s:@/]*)?@)?" # user:pass authentication
+ r"(?:" + ipv4_re + "|" + ipv6_re + "|" + host_re + ")"
+ r"(?::[0-9]{1,5})?" # port
+ r"(?:[/?#][^\s]*)?" # resource path
+ r"\Z",
+ re.IGNORECASE,
+ )
</code_context>
<issue_to_address>
**suggestion:** The URL validator defines `regex` twice, leaving the first definition redundant and potentially confusing.
There are now two consecutive `regex = _lazy_re_compile(...)` assignments in this class, so the first is dead code because only the last one is effective. Please remove the obsolete assignment (or consolidate if you intended to change the pattern) to avoid confusion.
Suggested implementation:
```python
host_re = "(" + hostname_re + domain_re + tld_re + "|localhost|\w+)"
```
```python
# IP patterns
```
1. This edit removes the newly added `regex = _lazy_re_compile(...)` definition near `host_re`, leaving the original `regex` assignment elsewhere in the class as the single source of truth.
2. If your intention is to *change* the URL pattern to the new one, you should instead:
- Locate the original `regex = _lazy_re_compile(...)` definition in this validator class.
- Replace that original block with the new block that was just removed here.
This will avoid duplication while preserving the updated behavior.
</issue_to_address>
### Comment 4
<location> `conreq/core/base/management/commands/run_conreq.py:60-68` </location>
<code_context>
- # Run background task management
- proc = Process(target=self.start_huey, daemon=True)
- proc.start()
+ huey = Process(target=start_huey)
+ huey.start()
+ webserver = Process(target=start_webserver, kwargs={"port": port})
+ webserver.start()
- # Run the production webserver
- if not DEBUG:
- config = HypercornConfig()
- config.bind = f"0.0.0.0:{port}"
- config.websocket_ping_interval = 20
- config.workers = 3
- config.application_path = "conreq.asgi:application"
- config.accesslog = ACCESS_LOG_FILE
+ while True:
+ if not huey.is_alive():
+ _logger.warning("Background task manager has crashed. Restarting...")
+ huey = Process(target=start_huey, daemon=True)
+ huey.start()
</code_context>
<issue_to_address>
**issue (bug_risk):** The Huey process is created with inconsistent `daemon` flags between initial start and restart.
On first start `huey` is created as `Process(target=start_huey)` (non-daemon), but on restart it’s created as `Process(target=start_huey, daemon=True)`. This changes its lifecycle semantics after a crash and can cause inconsistent or unexpected shutdown behavior. Please use a consistent `daemon` setting in both places based on the intended parent/child exit behavior.
</issue_to_address>
### Comment 5
<location> `conreq/core/base/management/commands/preconfig_conreq.py:82-86` </location>
<code_context>
if not os.path.exists(path):
print("> Creating database")
with sqlite3.connect(path) as cursor:
+ print("> Optimizing database")
+ cursor.execute("PRAGMA optimize;")
print("> Vacuuming database")
- cursor.execute("VACUUM")
- print("> Configuring database")
- cursor.execute("PRAGMA journal_mode = WAL;")
+ cursor.execute("VACUUM;")
+ print("> Reindexing database")
+ cursor.execute("REINDEX;")
if not no_perms and (uid != -1 or gid != -1) and sys.platform == "linux":
</code_context>
<issue_to_address>
**question (bug_risk):** The SQLite preconfig now omits configuring WAL or other durability-related PRAGMAs, which might be unintentional given the runtime settings.
This function used to enforce `PRAGMA journal_mode = WAL;`, but now only runs `PRAGMA optimize;`, `VACUUM;`, and `REINDEX;`. Meanwhile, the runtime config attempts to set WAL via `OPTIONS` that `sqlite3.connect` ignores (see comment on `init_command`/`transaction_mode`), so the DB may silently stay in the default `DELETE` mode. If WAL (and related durability PRAGMAs) are required, we should either keep an explicit `journal_mode = WAL` here or switch to a runtime configuration mechanism that SQLite actually honors.
</issue_to_address>
### Comment 6
<location> `conreq/core/base/static/js/events_click.js:91-96` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 7
<location> `conreq/core/base/static/js/events_click.js:114-145` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 8
<location> `conreq/core/base/static/js/events_click.js:147-149` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 9
<location> `conreq/core/base/static/js/events_click.js:272-278` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 10
<location> `conreq/core/base/static/js/events_click.js:284-290` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 11
<location> `conreq/core/base/static/js/events_click.js:334-340` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 12
<location> `conreq/core/base/static/js/events_click.js:488-494` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 13
<location> `conreq/core/base/static/js/modal.js:20-26` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 14
<location> `conreq/core/base/static/js/viewport.js:160-166` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 15
<location> `conreq/core/base/static/js/viewport.js:160-166` </location>
<code_context>
if (!window_location) {
if (window.history.replaceState) {
// Replace the current page in the browser history to add a hash
window.history.replaceState(
{},
null,
$(".nav-tab a").attr("href"),
);
window_location = window.location.hash.split("#")[1];
}
}
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/merge-nested-ifs))
```suggestion
if (!window_location && window.history.replaceState) {
window.history.replaceState(
{},
null,
$(".nav-tab a").attr("href"),
);
window_location = window.location.hash.split("#")[1];
}
```
<br/><details><summary>Explanation</summary>Reading deeply nested conditional code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>
### Comment 16
<location> `conreq/core/base/validators.py:19` </location>
<code_context>
import re
from django.core.validators import URLValidator
from django.utils.deconstruct import deconstructible
from django.utils.regex_helper import _lazy_re_compile
@deconstructible
class ExtendedURLValidator(URLValidator):
"""URL validator that supports hostnames (ex. https://sonarr:8000)"""
# IP patterns
ipv4_re = URLValidator.ipv4_re
ipv6_re = URLValidator.ipv6_re
hostname_re = URLValidator.hostname_re
domain_re = URLValidator.domain_re
tld_re = URLValidator.tld_re
host_re = "(" + hostname_re + domain_re + tld_re + "|localhost|\w+)"
regex = _lazy_re_compile(
r"^(?:[a-z0-9.+-]*)://" # scheme is validated separately
r"(?:[^\s:@/]+(?::[^\s:@/]*)?@)?" # user:pass authentication
r"(?:" + ipv4_re + "|" + ipv6_re + "|" + host_re + ")"
r"(?::[0-9]{1,5})?" # port
r"(?:[/?#][^\s]*)?" # resource path
r"\Z",
re.IGNORECASE,
)
regex = _lazy_re_compile(
r"^(?:[a-z0-9.+-]*)://" # scheme is validated separately
r"(?:[^\s:@/]+(?::[^\s:@/]*)?@)?" # user:pass authentication
r"(?:" + ipv4_re + "|" + ipv6_re + "|" + host_re + ")"
r"(?::[0-9]{1,5})?" # port
r"(?:[/?#][^\s]*)?" # resource path
r"\Z",
re.IGNORECASE,
)
</code_context>
<issue_to_address>
**issue (code-quality):** Use f-string instead of string concatenation [×3] ([`use-fstring-for-concatenation`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-fstring-for-concatenation/))
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+349
to
+358
| "init_command": ( | ||
| "PRAGMA foreign_keys = ON;" | ||
| "PRAGMA journal_mode = WAL;" | ||
| "PRAGMA synchronous = NORMAL;" | ||
| "PRAGMA busy_timeout = 10000;" | ||
| "PRAGMA temp_store = MEMORY;" | ||
| "PRAGMA mmap_size = 134217728;" | ||
| "PRAGMA journal_size_limit = 67108864;" | ||
| "PRAGMA cache_size = 2000;" | ||
| ), |
There was a problem hiding this comment.
issue (bug_risk): The SQLite OPTIONS use unsupported parameters that will likely break database connections.
For SQLite, Django passes OPTIONS straight into sqlite3.connect(**OPTIONS), which does not accept init_command or transaction_mode. This will raise TypeError: connect() got an unexpected keyword argument 'init_command' at startup. To apply these PRAGMAs, move them into connection.execute() (e.g., via a connection_created signal, connection wrapper, or custom backend). Only options actually supported by sqlite3.connect (such as isolation_level and timeout) should stay under OPTIONS.
| r"(?<!-)" # can't end with a dash | ||
| r"\.?" # may have a trailing dot | ||
| ) | ||
| host_re = "(" + hostname_re + domain_re + tld_re + "|localhost|\w+)" |
There was a problem hiding this comment.
issue (bug_risk): The \w+ fragment in host_re is not a raw string, which can lead to incorrect or deprecated escape behavior.
In Python, "...|\w+)" as a normal string triggers a deprecation warning today and may become an error, and the compiled regex may not match as intended. Please make this part of the pattern raw or escaped, e.g.:
host_re = r"(" + hostname_re + domain_re + tld_re + r"|localhost|\w+)"or otherwise construct the pattern so \w+ appears inside a raw string literal.
Comment on lines
+21
to
+28
| regex = _lazy_re_compile( | ||
| r"^(?:[a-z0-9.+-]*)://" # scheme is validated separately | ||
| r"(?:[^\s:@/]+(?::[^\s:@/]*)?@)?" # user:pass authentication | ||
| r"(?:" + ipv4_re + "|" + ipv6_re + "|" + host_re + ")" | ||
| r"(?::[0-9]{1,5})?" # port | ||
| r"(?:[/?#][^\s]*)?" # resource path | ||
| r"\Z", | ||
| re.IGNORECASE, |
There was a problem hiding this comment.
suggestion: The URL validator defines regex twice, leaving the first definition redundant and potentially confusing.
There are now two consecutive regex = _lazy_re_compile(...) assignments in this class, so the first is dead code because only the last one is effective. Please remove the obsolete assignment (or consolidate if you intended to change the pattern) to avoid confusion.
Suggested implementation:
host_re = "(" + hostname_re + domain_re + tld_re + "|localhost|\w+)"
# IP patterns- This edit removes the newly added
regex = _lazy_re_compile(...)definition nearhost_re, leaving the originalregexassignment elsewhere in the class as the single source of truth. - If your intention is to change the URL pattern to the new one, you should instead:
- Locate the original
regex = _lazy_re_compile(...)definition in this validator class. - Replace that original block with the new block that was just removed here.
This will avoid duplication while preserving the updated behavior.
- Locate the original
Comment on lines
+60
to
+68
| huey = Process(target=start_huey) | ||
| huey.start() | ||
| webserver = Process(target=start_webserver, kwargs={"port": port}) | ||
| webserver.start() | ||
|
|
||
| # Run the production webserver | ||
| if not DEBUG: | ||
| config = HypercornConfig() | ||
| config.bind = f"0.0.0.0:{port}" | ||
| config.websocket_ping_interval = 20 | ||
| config.workers = 3 | ||
| config.application_path = "conreq.asgi:application" | ||
| config.accesslog = ACCESS_LOG_FILE | ||
| while True: | ||
| if not huey.is_alive(): | ||
| _logger.warning("Background task manager has crashed. Restarting...") | ||
| huey = Process(target=start_huey, daemon=True) |
There was a problem hiding this comment.
issue (bug_risk): The Huey process is created with inconsistent daemon flags between initial start and restart.
On first start huey is created as Process(target=start_huey) (non-daemon), but on restart it’s created as Process(target=start_huey, daemon=True). This changes its lifecycle semantics after a crash and can cause inconsistent or unexpected shutdown behavior. Please use a consistent daemon setting in both places based on the intended parent/child exit behavior.
Comment on lines
+82
to
+86
| print("> Optimizing database") | ||
| cursor.execute("PRAGMA optimize;") | ||
| print("> Vacuuming database") | ||
| cursor.execute("VACUUM") | ||
| print("> Configuring database") | ||
| cursor.execute("PRAGMA journal_mode = WAL;") | ||
| cursor.execute("VACUUM;") | ||
| print("> Reindexing database") |
There was a problem hiding this comment.
question (bug_risk): The SQLite preconfig now omits configuring WAL or other durability-related PRAGMAs, which might be unintentional given the runtime settings.
This function used to enforce PRAGMA journal_mode = WAL;, but now only runs PRAGMA optimize;, VACUUM;, and REINDEX;. Meanwhile, the runtime config attempts to set WAL via OPTIONS that sqlite3.connect ignores (see comment on init_command/transaction_mode), so the DB may silently stay in the default DELETE mode. If WAL (and related durability PRAGMAs) are required, we should either keep an explicit journal_mode = WAL here or switch to a runtime configuration mechanism that SQLite actually honors.
Comment on lines
488
to
494
| change_server_setting(setting_name, dropdown_id).then( | ||
| async function () { | ||
| generate_viewport(false); | ||
| } | ||
| }, | ||
| ); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
issue (code-quality): Use const or let instead of var. (avoid-using-var)
Explanation
`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code). `let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than function-scoped.From the Airbnb JavaScript Style Guide
Comment on lines
20
to
26
| // Add click events | ||
| request_btn_click_event(); | ||
| content_modal_click_event(); | ||
| quick_request_click_event(); | ||
| modal_select_all_btn_click_event(); | ||
| modal_expand_btn_click_event(); | ||
| row_title_click_event(); |
There was a problem hiding this comment.
issue (code-quality): Use const or let instead of var. (avoid-using-var)
Explanation
`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code). `let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than function-scoped.From the Airbnb JavaScript Style Guide
Comment on lines
160
to
166
| window.history.replaceState( | ||
| {}, | ||
| null, | ||
| $(".nav-tab a").attr("href") | ||
| $(".nav-tab a").attr("href"), | ||
| ); | ||
| window_location = window.location.hash.split("#")[1]; | ||
| } |
There was a problem hiding this comment.
issue (code-quality): Use const or let instead of var. (avoid-using-var)
Explanation
`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code). `let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than function-scoped.From the Airbnb JavaScript Style Guide
Comment on lines
160
to
166
| window.history.replaceState( | ||
| {}, | ||
| null, | ||
| $(".nav-tab a").attr("href") | ||
| $(".nav-tab a").attr("href"), | ||
| ); | ||
| window_location = window.location.hash.split("#")[1]; | ||
| } |
There was a problem hiding this comment.
suggestion (code-quality): Merge nested if conditions (merge-nested-ifs)
Suggested change
| window.history.replaceState( | |
| {}, | |
| null, | |
| $(".nav-tab a").attr("href") | |
| $(".nav-tab a").attr("href"), | |
| ); | |
| window_location = window.location.hash.split("#")[1]; | |
| } | |
| if (!window_location && window.history.replaceState) { | |
| window.history.replaceState( | |
| {}, | |
| null, | |
| $(".nav-tab a").attr("href"), | |
| ); | |
| window_location = window.location.hash.split("#")[1]; | |
| } | |
Explanation
Reading deeply nested conditional code is confusing, since you have to keep track of whichconditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two
if conditions can be combined usingand is an easy win.
| r"(?<!-)" # can't end with a dash | ||
| r"\.?" # may have a trailing dot | ||
| ) | ||
| host_re = "(" + hostname_re + domain_re + tld_re + "|localhost|\w+)" |
There was a problem hiding this comment.
issue (code-quality): Use f-string instead of string concatenation [×3] (use-fstring-for-concatenation)
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
1 participant
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.
Checklist
Please update this checklist as you complete each item:
By submitting this pull request I agree that all contributions comply with this project's open source license(s).
Summary by Sourcery
Switch the application to a Uvicorn-based runtime with improved process supervision and database tuning, add a quick-request UX for content posters, and remove the deprecated API/DRF stack and legacy static/HTML compression tooling.
New Features:
Bug Fixes:
Enhancements:
Build:
uvwith documented build/run commands.Documentation:
Chores:
Summary by Sourcery
Switch the runtime to a supervised Uvicorn-based ASGI server, streamline static file handling, and improve background task and database reliability while adding a quick-request poster UX and removing the legacy DRF API stack.
New Features:
Bug Fixes:
Enhancements:
Build:
uvwith documented build and run commands.Documentation:
Chores: