Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR centralizes layered caching (MultiCache), adds validator/header helpers, rewires schedule/iCal/PDF caching to use those utilities (including queued disk writes), updates settings/runtime for disk cache and TTLs, expands tests for multi-backend behavior, and supplies a frontend modernization plan plus small HTML/template fixes. ChangesCache infrastructure and response handling modernization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (6)
MODERNIZE.md (1)
338-345: ⚡ Quick winAdd a cached-response CSP smoke test.
The checklist covers manifest and
collectstatic, but not the nonce headers thatCspMiddlewareuses on cached HTML responses. Add one check that exercises a cache hit and confirms the CSP nonces still survive.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MODERNIZE.md` around lines 338 - 345, Add a smoke test that exercises a cache hit and verifies CspMiddleware preserves nonces on cached HTML responses: create a test that requests a known HTML view twice (first request to populate the cache, second to produce a cache hit), ensure caching is enabled for the view (so a cache-hit response is returned), read the CSP header set by CspMiddleware and assert it contains a nonce token, and also assert the same nonce value appears in the response HTML (e.g., nonce attributes on inline script/style tags) to confirm the nonce survives caching.plan/common/utils.py (1)
205-229: 💤 Low valueClarify the hardcoded restriction to "disk" cache for queued writes.
Line 219 restricts queued response caching to the
"disk"cache alias only, raisingValueErrorfor other aliases. This hardcoded restriction couples the helper function to a specific cache backend name. Consider documenting why queued writes are disk-only, or parameterizing the restriction if other backends may support queued writes in the future.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plan/common/utils.py` around lines 205 - 229, store_cached_response currently hardcodes queued writes to cache_alias == "disk" and raises ValueError otherwise; replace this coupling by either making the restriction configurable or by detecting backend capability: add a configurable set/constant (e.g., QUEUED_CACHE_ALIASES) or check the cache backend for a capability flag (e.g., caches[cache_alias].supports_queued_writes) before calling enqueue_cache_set, and update the function docstring to explain queued-write requirements; keep the existing enqueue_cache_set call and cache_alias/queued parameters but remove the literal "disk" string check and raise ValueError only when the backend is not in the allowed set or lacks the capability.plan/common/snapshot.py (2)
59-77: 💤 Low valueReview memoization scope of cache factory.
_schedule_snapshot_cache_for_configis decorated with@cache(functools.cache), which memoizes at the module level based on the two TTL arguments. This means:
- A single
MultiCacheinstance is reused for each unique(default_ttl, disk_ttl)pair.- If Django settings change at runtime (unusual but possible in tests), the memoized instance won't reflect the new config.
Confirm this memoization behavior is intentional and appropriate for your use case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plan/common/snapshot.py` around lines 59 - 77, The function _schedule_snapshot_cache_for_config is currently decorated with `@cache`, causing a module-level memoization keyed on (default_ttl, disk_ttl) which prevents later changes to settings from being reflected; remove the `@cache` decorator from _schedule_snapshot_cache_for_config (or replace it with a non-module-global strategy) so each call constructs a fresh MultiCache based on current settings.TIMETABLE_SNAPSHOT_CACHE_DEFAULT_TTL and settings.TIMETABLE_SNAPSHOT_CACHE_DISK_TTL, leaving the function body and the _schedule_snapshot_cache wrapper unchanged.
80-88: 💤 Low valueDefensive ValueError for cached None may be overly strict.
Lines 84-87 raise
ValueErrorwhenresult.hitisTruebutresult.valueisNone. SinceScheduleSnapshotinstances should never beNonein normal operation, this is defensive. However, raising here might mask the root cause if a bug elsewhere accidentally cachesNone. Consider whether logging and returning a fallback would be more appropriate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plan/common/snapshot.py` around lines 80 - 88, The current get_schedule_snapshot raises ValueError when _schedule_snapshot_cache().get(key) returns a hit with value None; instead, log a warning including the key and the unexpected None (use the module logger), invalidate the cached entry via _schedule_snapshot_cache().delete(key) (or equivalent), and treat it as a cache miss so the function continues to compute and return a fresh ScheduleSnapshot; update code in get_schedule_snapshot (and references to schedule_snapshot_cache_key and _schedule_snapshot_cache()) to perform logging + invalidation rather than raising.plan/common/cache.py (1)
24-38: ⚡ Quick winConsider error handling for cache backend operations.
The
getmethod performs cache operations (lines 28, 32) without error handling. If a cache backend is unavailable or fails, exceptions will propagate to callers. Django cache backends typically suppress errors and returnNoneon failure, but defensive error handling would improve robustness.🛡️ Optional defensive wrapper
def get(self, key: str) -> CacheResult[T]: missed: list[tuple[str, int | None]] = [] for name, ttl in self.layers: - value = self.backends[name].get(key, _MISSING) + try: + value = self.backends[name].get(key, _MISSING) + except Exception: + # Treat cache backend errors as misses + value = _MISSING if value is not _MISSING: for missed_name, missed_ttl in missed: - self.backends[missed_name].set(key, value, missed_ttl) + try: + self.backends[missed_name].set(key, value, missed_ttl) + except Exception: + # Log but don't fail on backfill errors + pass return CacheResult(hit=True, value=value) missed.append((name, ttl)) return CacheResult(hit=False)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plan/common/cache.py` around lines 24 - 38, The get method currently calls backend.get and backend.set without guarding against backend failures; wrap the calls to self.backends[name].get(key, _MISSING) and self.backends[missed_name].set(key, value, missed_ttl) in try/except blocks that catch Exception, treat a failing get as a cache miss (continue the loop) and a failing set as non-fatal (log if a logger exists on the instance, e.g., self.logger.exception(...), otherwise ignore), and ensure missed.append((name, ttl)) only occurs for genuine misses (not crashes). Reference the get method, self.layers, self.backends, _MISSING and CacheResult when making the changes.plan/settings/runtime.py (1)
325-335: Confirm cache-duration assignment semantics in plan/settings/runtime.py
timetable_ical_cache_duration_secondsis a non-optionalint(default30*24*60*60), soTIMETABLE_ICAL_CACHE_DURATIONis always atimedelta; an env value of0yieldstimedelta(0)rather thanNone.timetable_schedule_cache_duration_secondsisint | None(defaultNone), soTIMETABLE_SCHEDULE_CACHE_DURATIONis intentionally only set when the env value is provided.- This makes the downstream
is not Noneguard forTIMETABLE_ICAL_CACHE_DURATIONinplan/ical/views.pyeffectively redundant (harmless).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plan/settings/runtime.py` around lines 325 - 335, The TIMETABLE_ICAL_CACHE_DURATION constant is always a timedelta (even zero) because env.timetable_ical_cache_duration_seconds is non-optional, so remove the redundant "is not None" guard in plan/ical/views.py and instead check the actual duration value (e.g., TIMETABLE_ICAL_CACHE_DURATION.total_seconds() > 0) where the guard was used; update references to use TIMETABLE_ICAL_CACHE_DURATION directly and only treat zero as disabled if that semantic is intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@MODERNIZE.md`:
- Around line 51-57: Update the allowed-approaches bullet list in MODERNIZE.md
by removing the "inline styles" option and instead explicitly recommend nonced
<style> tags, CSS custom properties, generated CSS palette maps, or static
classes/data attrs that remain server-safe; locate the line containing "inline
styles" in the list and delete it, then edit the surrounding sentence to mention
nonced <style> tags (or generated classes/CSS custom properties) to ensure
guidance matches the strict CSP constraint.
In `@plan/common/cache.py`:
- Around line 17-22: In MultiCache.__init__, avoid direct indexing into Django's
caches; validate each requested cache name exists or catch KeyError and raise a
clearer error. Iterate the provided layers (self.layers or layers.items()),
check membership in caches (or try to access each caches[name] in a try/except),
and if missing raise a descriptive ValueError referencing the missing cache name
and the MultiCache initializer; then populate self.backends from the validated
names. Ensure error messages mention the problematic cache key and the expected
Django CACHES configuration.
In `@plan/common/snapshot.py`:
- Around line 49-56: delete_schedule_snapshot_cache currently assumes
caches["disk"] exists and can raise KeyError if disk backend isn't configured;
change it to defensively check for the disk cache (e.g., use caches.get("disk")
or "disk" in caches) before calling delete so only configured backends are
touched. Update the delete_schedule_snapshot_cache function (which uses
schedule_snapshot_cache_key and calls caches["default"].delete and
caches["disk"].delete) to delete from the default cache as before and
conditionally delete from the disk cache only when it is present.
---
Nitpick comments:
In `@MODERNIZE.md`:
- Around line 338-345: Add a smoke test that exercises a cache hit and verifies
CspMiddleware preserves nonces on cached HTML responses: create a test that
requests a known HTML view twice (first request to populate the cache, second to
produce a cache hit), ensure caching is enabled for the view (so a cache-hit
response is returned), read the CSP header set by CspMiddleware and assert it
contains a nonce token, and also assert the same nonce value appears in the
response HTML (e.g., nonce attributes on inline script/style tags) to confirm
the nonce survives caching.
In `@plan/common/cache.py`:
- Around line 24-38: The get method currently calls backend.get and backend.set
without guarding against backend failures; wrap the calls to
self.backends[name].get(key, _MISSING) and self.backends[missed_name].set(key,
value, missed_ttl) in try/except blocks that catch Exception, treat a failing
get as a cache miss (continue the loop) and a failing set as non-fatal (log if a
logger exists on the instance, e.g., self.logger.exception(...), otherwise
ignore), and ensure missed.append((name, ttl)) only occurs for genuine misses
(not crashes). Reference the get method, self.layers, self.backends, _MISSING
and CacheResult when making the changes.
In `@plan/common/snapshot.py`:
- Around line 59-77: The function _schedule_snapshot_cache_for_config is
currently decorated with `@cache`, causing a module-level memoization keyed on
(default_ttl, disk_ttl) which prevents later changes to settings from being
reflected; remove the `@cache` decorator from _schedule_snapshot_cache_for_config
(or replace it with a non-module-global strategy) so each call constructs a
fresh MultiCache based on current settings.TIMETABLE_SNAPSHOT_CACHE_DEFAULT_TTL
and settings.TIMETABLE_SNAPSHOT_CACHE_DISK_TTL, leaving the function body and
the _schedule_snapshot_cache wrapper unchanged.
- Around line 80-88: The current get_schedule_snapshot raises ValueError when
_schedule_snapshot_cache().get(key) returns a hit with value None; instead, log
a warning including the key and the unexpected None (use the module logger),
invalidate the cached entry via _schedule_snapshot_cache().delete(key) (or
equivalent), and treat it as a cache miss so the function continues to compute
and return a fresh ScheduleSnapshot; update code in get_schedule_snapshot (and
references to schedule_snapshot_cache_key and _schedule_snapshot_cache()) to
perform logging + invalidation rather than raising.
In `@plan/common/utils.py`:
- Around line 205-229: store_cached_response currently hardcodes queued writes
to cache_alias == "disk" and raises ValueError otherwise; replace this coupling
by either making the restriction configurable or by detecting backend
capability: add a configurable set/constant (e.g., QUEUED_CACHE_ALIASES) or
check the cache backend for a capability flag (e.g.,
caches[cache_alias].supports_queued_writes) before calling enqueue_cache_set,
and update the function docstring to explain queued-write requirements; keep the
existing enqueue_cache_set call and cache_alias/queued parameters but remove the
literal "disk" string check and raise ValueError only when the backend is not in
the allowed set or lacks the capability.
In `@plan/settings/runtime.py`:
- Around line 325-335: The TIMETABLE_ICAL_CACHE_DURATION constant is always a
timedelta (even zero) because env.timetable_ical_cache_duration_seconds is
non-optional, so remove the redundant "is not None" guard in plan/ical/views.py
and instead check the actual duration value (e.g.,
TIMETABLE_ICAL_CACHE_DURATION.total_seconds() > 0) where the guard was used;
update references to use TIMETABLE_ICAL_CACHE_DURATION directly and only treat
zero as disabled if that semantic is intended.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3dcba1c6-060f-4dc6-a670-20a6e27220fb
📒 Files selected for processing (17)
MODERNIZE.mdplan/common/cache.pyplan/common/snapshot.pyplan/common/tests/__init__.pyplan/common/tests/test_cache.pyplan/common/tests/test_converters.pyplan/common/tests/test_utils.pyplan/common/tests/test_views.pyplan/common/utils.pyplan/common/views.pyplan/ical/queue.pyplan/ical/tests.pyplan/ical/urls.pyplan/ical/views.pyplan/pdf/views.pyplan/settings/env.pyplan/settings/runtime.py
💤 Files with no reviewable changes (1)
- plan/ical/urls.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plan/common/tests/test_views.py`:
- Around line 593-595: The test's CSP regex is brittle because it expects
exactly two spaces after "style-src"; update the regexes that assign
script_match and style_match to tolerate varying whitespace by replacing the
literal spaces with a whitespace pattern (e.g. \s+ or \s*) between tokens so the
pattern still captures the nonce from the policy string variable policy; update
the regex used for script_match and the one used for style_match (the
expressions around "script-src" and "style-src", "'self'", and "'nonce-...'" in
test_views.py) to use whitespace-tolerant separators.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 40dc1bb8-9721-494c-80c1-cacbf50d2ba8
📒 Files selected for processing (17)
MODERNIZE.mdnix/workspace.nixplan/common/cache.pyplan/common/snapshot.pyplan/common/tests/test_cache.pyplan/common/tests/test_converters.pyplan/common/tests/test_utils.pyplan/common/tests/test_views.pyplan/common/utils.pyplan/ical/queue.pyplan/ical/views.pyplan/templates/about.htmlplan/templates/base.htmlplan/templates/courses.htmlplan/templates/lectures.htmlplan/templates/schedule.htmlplan/templates/statistics.html
💤 Files with no reviewable changes (1)
- plan/templates/about.html
✅ Files skipped from review due to trivial changes (2)
- plan/templates/statistics.html
- MODERNIZE.md
🚧 Files skipped from review as they are similar to previous changes (4)
- plan/common/utils.py
- plan/common/cache.py
- plan/common/tests/test_converters.py
- plan/ical/queue.py
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.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests
Documentation