Implement site-wide navigation menu with caching and error logging#48
Conversation
…s and enhance caching and error logging requirements
…upport and accessibility features
WalkthroughA new Navigation Menu System is introduced to QuakerCMS, featuring a Django app with Wagtail-based models, blocks, template tags, and responsive HTML templates. The system supports 2-level nested menus with page links, external links, and dropdowns, includes comprehensive tests and management scaffolding, and integrates with base templates via a template tag. Documentation and development workflow guidance are updated to reflect uv-based commands. Changes
Sequence DiagramsequenceDiagram
participant User
participant Template as base.html
participant Tag as navigation_menu tag
participant DB as NavigationMenuSetting
participant Cache as Page/Translation Cache
participant NavTpl as navigation.html
User->>Template: Request page
Template->>Tag: {% navigation_menu %}
Tag->>DB: Fetch menu_items & settings
DB-->>Tag: StreamField data
Tag->>Cache: Bulk fetch pages & translations
Cache-->>Tag: Cached page objects
Tag->>Tag: process_menu_item(recursive)
Note over Tag: Resolve translations,<br/>detect current page,<br/>filter unpublished
Tag-->>NavTpl: Render context with menu_items
NavTpl->>NavTpl: Desktop/mobile variants
NavTpl->>NavTpl: ARIA + keyboard JS
NavTpl-->>User: Responsive navigation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (2)src/**/migrations/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
src/**/{tests.py,tests/**/*.py}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (1)📚 Learning: 2025-10-26T14:57:19.987ZApplied to files:
🧬 Code graph analysis (2)src/navigation/tests.py (5)
src/navigation/management/commands/scaffold_navbar_content.py (3)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #48 +/- ##
==========================================
+ Coverage 92.92% 94.55% +1.63%
==========================================
Files 26 33 +7
Lines 904 1707 +803
==========================================
+ Hits 840 1614 +774
- Misses 64 93 +29 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
specs/002-nav-menu-system/spec.md (1)
245-249: Update template structure requirement to match implementation.FR-041 specifies three separate template files (
navigation.html,navigation_item.html,navigation_dropdown.html), but the implementation uses a single consolidated template atsrc/navigation/templates/navigation/navigation.html. This discrepancy between specification and implementation should be resolved.Either:
- Update the spec to reflect the single-template approach (recommended for simplicity), or
- Refactor the template into separate includes per the spec
The current single-template implementation is simpler and easier to maintain for this use case, so updating the spec is likely the better approach:
- **FR-041**: Navigation templates MUST be structured as reusable components: - - `navigation.html` - Main navigation wrapper - - `navigation_item.html` - Simple link rendering - - `navigation_dropdown.html` - Dropdown menu rendering + - `navigation.html` - Complete navigation template with mobile and desktop rendering
🧹 Nitpick comments (2)
src/navigation/templates/navigation/navigation.html (1)
13-61: Consider extracting common menu item rendering logic.The desktop (lines 13-61) and mobile (lines 103-141) menu sections contain similar logic for iterating and rendering menu items. While the HTML structure differs due to responsive requirements, the item type checking and URL/title rendering follows the same pattern.
Consider creating a reusable template include for menu items if the duplication grows with future features. For now, the different ARIA attributes and responsive classes justify separate implementations.
Also applies to: 103-141
src/navigation/blocks.py (1)
6-27: Consider adding Meta.template to blocks for rendering.The custom StructBlock classes don't define
Meta.templateattributes for rendering. The current implementation processes blocks programmatically innavigation_tags.pyand renders them innavigation.html, which works but differs from the established pattern.As per coding guidelines
Based on learnings:
- Custom StreamField blocks should use StructBlock and provide a Meta.template for rendering
Consider adding template-based rendering:
class PageLinkBlock(blocks.StructBlock): # ... existing fields ... class Meta: icon = "doc-full" label = "Page Link" template = "navigation/blocks/page_link.html"This would allow blocks to self-render and simplify the template tag logic. However, the current approach enables locale-aware processing, so this refactor should be evaluated against those requirements.
Also applies to: 29-50, 52-63, 65-81, 83-93
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
AGENTS.md(1 hunks)CHANGELOG.md(1 hunks)specs/002-nav-menu-system/spec.md(5 hunks)src/core/settings/base.py(1 hunks)src/core/templates/base.html(2 hunks)src/navigation/__init__.py(1 hunks)src/navigation/admin.py(1 hunks)src/navigation/apps.py(1 hunks)src/navigation/blocks.py(1 hunks)src/navigation/migrations/0001_initial.py(1 hunks)src/navigation/migrations/0002_alter_navigationmenusetting_menu_items.py(1 hunks)src/navigation/migrations/__init__.py(1 hunks)src/navigation/models.py(1 hunks)src/navigation/templates/navigation/navigation.html(1 hunks)src/navigation/templatetags/__init__.py(1 hunks)src/navigation/templatetags/navigation_tags.py(1 hunks)src/navigation/tests.py(1 hunks)src/navigation/views.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/core/settings/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Do not define or access language/i18n constants via Django settings; import them from core.constants
Files:
src/core/settings/base.py
src/**/migrations/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Check migration files into version control
Files:
src/navigation/migrations/__init__.pysrc/navigation/migrations/0001_initial.pysrc/navigation/migrations/0002_alter_navigationmenusetting_menu_items.py
src/**/models.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/models.py: Models must import language/i18n constants (e.g., LANGUAGE_CHOICES) from core.constants instead of defining them inline
Define custom StreamField blocks using StructBlock and provide a Meta.template for rendering
Use EmbedBlock for embeds and never use RawHTMLBlock (avoid XSS risk)
Name StreamField blocks accurately (e.g., use 'rich_text' for RichTextBlock rather than generic names like 'paragraph')
Files:
src/navigation/models.py
src/**/{tests.py,tests/**/*.py}
📄 CodeRabbit inference engine (AGENTS.md)
Organize tests with descriptive classes by feature: ModelTests, StreamFieldTests, TranslationTests, AdminTests, IntegrationTests
Files:
src/navigation/tests.py
🧠 Learnings (7)
📚 Learning: 2025-10-26T14:57:19.987Z
Learnt from: CR
Repo: WesternFriend/QuakerCMS PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-26T14:57:19.987Z
Learning: Languages are configured at runtime via LocaleSettings in Wagtail admin, not hard-coded in code
Applied to files:
src/navigation/admin.py
📚 Learning: 2025-10-26T14:57:19.987Z
Learnt from: CR
Repo: WesternFriend/QuakerCMS PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-26T14:57:19.987Z
Learning: Applies to src/core/models.py : Do not add Django models in the core app (core contains settings/constants only)
Applied to files:
src/core/settings/base.py
📚 Learning: 2025-10-26T14:57:19.987Z
Learnt from: CR
Repo: WesternFriend/QuakerCMS PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-26T14:57:19.987Z
Learning: Applies to src/**/models.py : Use EmbedBlock for embeds and never use RawHTMLBlock (avoid XSS risk)
Applied to files:
AGENTS.md
📚 Learning: 2025-10-26T14:57:19.987Z
Learnt from: CR
Repo: WesternFriend/QuakerCMS PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-26T14:57:19.987Z
Learning: Applies to src/**/models.py : Define custom StreamField blocks using StructBlock and provide a Meta.template for rendering
Applied to files:
AGENTS.mdsrc/navigation/blocks.py
📚 Learning: 2025-10-26T14:57:19.987Z
Learnt from: CR
Repo: WesternFriend/QuakerCMS PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-26T14:57:19.987Z
Learning: Applies to src/**/models.py : Name StreamField blocks accurately (e.g., use 'rich_text' for RichTextBlock rather than generic names like 'paragraph')
Applied to files:
AGENTS.mdsrc/navigation/blocks.py
📚 Learning: 2025-10-26T14:57:19.987Z
Learnt from: CR
Repo: WesternFriend/QuakerCMS PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-26T14:57:19.987Z
Learning: Applies to src/**/{tests.py,tests/**/*.py} : Organize tests with descriptive classes by feature: ModelTests, StreamFieldTests, TranslationTests, AdminTests, IntegrationTests
Applied to files:
src/navigation/tests.py
📚 Learning: 2025-10-26T14:57:19.987Z
Learnt from: CR
Repo: WesternFriend/QuakerCMS PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-26T14:57:19.987Z
Learning: Run makemigrations after model changes; StreamField changes require migrations
Applied to files:
src/navigation/migrations/0002_alter_navigationmenusetting_menu_items.py
🧬 Code graph analysis (5)
src/navigation/templatetags/navigation_tags.py (1)
src/navigation/models.py (1)
NavigationMenuSetting(11-27)
src/navigation/models.py (1)
src/navigation/blocks.py (6)
TopLevelMenuBlock(83-93)Meta(24-26)Meta(47-49)Meta(59-62)Meta(78-80)Meta(90-93)
src/navigation/tests.py (2)
src/home/models.py (1)
HomePage(11-34)src/navigation/models.py (1)
NavigationMenuSetting(11-27)
src/navigation/migrations/0001_initial.py (1)
src/navigation/migrations/0002_alter_navigationmenusetting_menu_items.py (1)
Migration(7-105)
src/navigation/migrations/0002_alter_navigationmenusetting_menu_items.py (1)
src/navigation/migrations/0001_initial.py (1)
Migration(8-114)
🔇 Additional comments (12)
src/navigation/migrations/__init__.py (1)
1-1: LGTM!Standard migrations package initializer with appropriate docstring.
src/navigation/admin.py (1)
1-3: LGTM!Clear documentation that navigation configuration is handled through Wagtail Settings rather than Django admin, which aligns with Wagtail best practices for site-wide configuration.
src/navigation/__init__.py (1)
1-1: LGTM!Standard package initializer with descriptive docstring.
CHANGELOG.md (1)
12-30: LGTM!Comprehensive and well-structured changelog entry documenting the navigation menu system. The entry clearly describes the feature's capabilities, technical implementation, accessibility compliance, and testing coverage.
src/core/settings/base.py (1)
33-33: LGTM!Navigation app correctly registered in INSTALLED_APPS with appropriate placement among other local apps.
src/navigation/templatetags/__init__.py (1)
1-1: LGTM!Standard template tags package initializer required for Django template tag discovery.
src/navigation/views.py (1)
1-3: LGTM!Clear documentation explaining that navigation uses template tag-based rendering rather than traditional Django views, which aligns with the architectural approach described in the PR.
AGENTS.md (1)
54-59: LGTM!Excellent documentation of the navigation menu pattern, clearly explaining the 2-level nesting constraint and how it's enforced at the schema level. This provides valuable architectural guidance for developers working with the navigation system.
src/navigation/templates/navigation/navigation.html (1)
160-175: Verify dropdown positioning logic with hidden elements.The
getBoundingClientRect()call on line 162 may return inaccurate dimensions if the dropdown menu is not yet visible whenshowDropdown()is first called. For reliable edge detection, the menu should be temporarily displayed (e.g., withvisibility: hiddenbutdisplay: block) before measuring.Test the dropdown positioning behavior:
- Create a dropdown menu near the right edge of the viewport
- Hover over the dropdown trigger for the first time
- Verify that the dropdown correctly positions itself to avoid overflow
- If the dropdown overflows, the positioning logic needs adjustment to measure the menu while it's rendered but not yet visible
Consider this approach if issues are found:
const showDropdown = () => { + // Temporarily make menu measurable + menu.style.visibility = 'hidden'; + menu.style.display = 'block'; + const buttonRect = button.getBoundingClientRect(); const menuRect = menu.getBoundingClientRect(); const viewportWidth = window.innerWidth; + // Restore visibility + menu.style.visibility = ''; + // Check if dropdown would overflow right edge if (buttonRect.left + menuRect.width > viewportWidth) {src/navigation/models.py (1)
10-27: Verify that caching and error logging will be implemented.The specification (FR-024-Cache and FR-024-Limits in spec.md) requires caching with 1-hour TTL, cache invalidation on save, error logging, and soft limit warnings at 100 items. The current implementation doesn't include these features.
Confirm whether:
- Caching and error logging are planned for a follow-up PR, or
- These should be implemented in this PR
If these features are intended for this PR, the model should override
save()anddelete()methods to invalidate cache, and implement validation to warn when approaching 100 total menu items.src/navigation/templatetags/navigation_tags.py (1)
11-40: Implement caching as specified in FR-024-Cache.The specification requires caching with 1-hour TTL, per-site/locale cache keys, and error logging (FR-024-Cache, lines 202-208 in spec.md). The current implementation queries the database on every request without caching.
Confirm whether caching will be added in this PR or a follow-up. If implementing now, the template tag should:
from django.core.cache import cache from django.conf import settings import logging logger = logging.getLogger(__name__) @register.inclusion_tag("navigation/navigation.html", takes_context=True) def navigation_menu(context): """Render navigation menu with locale-aware page links.""" request = context["request"] current_locale = Locale.get_active() current_site = request.site # Generate cache key per site and locale cache_key = f"navigation_menu_{current_site.pk}_{current_locale.language_code}" # Try to get from cache try: cached_result = cache.get(cache_key) if cached_result is not None: return cached_result except Exception as e: logger.error(f"Navigation cache retrieval failed: {e}", exc_info=True) # Continue without cache # ... existing menu processing logic ... result = { "menu_items": processed_items, "current_page": context.get("page"), "request": request, } # Cache the result try: cache.set(cache_key, result, 3600) # 1 hour TTL except Exception as e: logger.error(f"Navigation cache storage failed: {e}", exc_info=True) return resultAlso add cache invalidation in
NavigationMenuSetting.save()anddelete()methods.specs/002-nav-menu-system/spec.md (1)
20-22: Acknowledge comprehensive caching and monitoring requirements.The new Session 2025-11-03 additions provide clear, actionable requirements for caching (1-hour TTL, per-site/locale keys, cache invalidation) and error logging (ERROR level, console + file). These requirements are well-specified and testable.
The specification clearly defines:
- Cache strategy with specific TTL and key format
- Graceful degradation when cache fails
- Soft limit approach (warning at 100 items, allow override)
- Error logging configuration
These additions strengthen the feature requirements and provide clear implementation guidance.
Also applies to: 202-212
| class DropdownMenuBlock(blocks.StructBlock): | ||
| """Dropdown menu with child items.""" | ||
|
|
||
| title = blocks.CharBlock( | ||
| required=True, | ||
| max_length=100, | ||
| help_text="Dropdown menu label", | ||
| ) | ||
| items = MenuItemBlock( | ||
| required=True, | ||
| help_text="Links to display in dropdown menu", | ||
| ) | ||
|
|
||
| class Meta: | ||
| icon = "list-ul" | ||
| label = "Dropdown Menu" | ||
|
|
There was a problem hiding this comment.
Validate that dropdowns contain at least one item.
The items field is marked as required=True, but StreamBlock's required parameter only ensures the field is present, not that it contains items. A dropdown could be saved with zero child items, resulting in an empty, non-functional dropdown.
Consider adding custom validation to the DropdownMenuBlock:
class DropdownMenuBlock(blocks.StructBlock):
title = blocks.CharBlock(
required=True,
max_length=100,
help_text="Dropdown menu label",
)
items = MenuItemBlock(
required=True,
help_text="Links to display in dropdown menu",
)
def clean(self, value):
cleaned = super().clean(value)
if cleaned and not cleaned.get('items'):
raise blocks.StreamBlockValidationError(
non_block_errors=['Dropdown menu must contain at least one item']
)
return cleaned
class Meta:
icon = "list-ul"
label = "Dropdown Menu"🤖 Prompt for AI Agents
In src/navigation/blocks.py around lines 65 to 81, the DropdownMenuBlock marks
items as required but does not validate that the StreamBlock actually contains
one or more child items; override clean(self, value) to call
super().clean(value) and if cleaned is truthy but cleaned.get('items') is
empty/falsey raise a blocks.StreamBlockValidationError (with a clear message
like "Dropdown menu must contain at least one item") so the block cannot be
saved with zero items; keep existing Meta unchanged.
- Add 12 new template tag tests for navigation_menu and process_menu_item - Fix page translation handling to use get_translation() method - Add fallback to original page when translation doesn't exist - Test page links, external links, dropdowns, and edge cases - Improve code coverage for navigation module - All 20 navigation tests now passing
- Add 6 new integration tests that actually render templates - Test page links, external links, and dropdown rendering - Test current page marking with aria-current attribute - Test filtering of unpublished and deleted pages - Test graceful handling of missing settings - All 26 navigation tests passing (increased from 20)
- Update FR-041 to specify single consolidated template instead of three separate templates - Update Frontend Template Structure section to describe actual implementation - Update plan.md file structure to show navigation.html only - Update requirements checklist to reflect DaisyUI component usage - Remove references to navigation_item.html and navigation_dropdown.html - Align spec with actual implementation for maintainability
…paration - Document why desktop and mobile menu rendering are not extracted to includes - Explain different ARIA patterns (menubar vs menu) - Note different interaction models (button/hover vs details/summary) - Clarify different JavaScript requirements (edge detection vs native HTML5) - Reference WCAG requirements for desktop vs mobile/touch patterns - Prevent future refactoring that could break accessibility compliance
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
src/navigation/tests.py (4)
29-48: Guarantee a HomePage fixture exists before dereferencing.This test calls
HomePage.objects.first()at line 32 and immediately accesseshome.idat line 40, which will raiseAttributeErrorif noHomePageexists in the test database. The setUp should create aHomePagewhenfirst()returnsNoneto make tests self-contained.Apply this diff to provision the page in setUp:
def setUp(self): """Set up test data.""" self.site = Site.objects.get(is_default_site=True) + self.home = HomePage.objects.first() + if self.home is None: + root_page = self.site.root_page.specific + self.home = HomePage(title="Navigation Test Home", slug="navigation-test-home") + root_page.add_child(instance=self.home) + self.home.save_revision().publish()Then update the test to use
self.home:def test_navigation_menu_with_page_link(self): """Can create menu with page link.""" - # Get the home page - home = HomePage.objects.first() - menu = NavigationMenuSetting.objects.create( site=self.site, menu_items=[ { "type": "page_link", "value": { - "page": home.id, + "page": self.home.id, "custom_title": "", "anchor": "", }, }, ], )Based on past review comments.
86-109: Ensure HomePage exists in setUp before tests use it.
self.home = HomePage.objects.first()at line 89 can returnNone, causing tests to fail when accessingself.home.idat line 99 (and lines 107, 144). Make setUp create aHomePageif one doesn't exist.Apply this diff:
def setUp(self): """Set up test data.""" self.site = Site.objects.get(is_default_site=True) self.home = HomePage.objects.first() + if self.home is None: + root_page = self.site.root_page.specific + self.home = HomePage(title="Navigation Test Home", slug="navigation-streamfield-test") + root_page.add_child(instance=self.home) + self.home.save_revision().publish()Based on past review comments.
164-196: Create HomePage fixture in setUp to prevent AttributeError.Line 168 assigns
self.home = HomePage.objects.first()which can beNone, then line 178 accessesself.home.idwithout checking. Add a fallback to create the page if it doesn't exist.Apply this diff:
def setUp(self): """Set up test data.""" self.site = Site.objects.get(is_default_site=True) self.locale = Locale.get_default() self.home = HomePage.objects.first() + if self.home is None: + root_page = self.site.root_page.specific + self.home = HomePage(title="Navigation Test Home", slug="navigation-integration-test") + root_page.add_child(instance=self.home) + self.home.save_revision().publish()Based on past review comments.
202-216: Provision HomePage in setUp before using it.
self.home = HomePage.objects.first()at line 207 may returnNone, yet line 215 (and many subsequent tests) accessself.homeor create children under it. Ensure setUp creates aHomePagewhen none exists.Apply this diff:
def setUp(self): """Set up test data.""" self.factory = RequestFactory() self.site = Site.objects.get(is_default_site=True) self.default_locale = Locale.get_default() self.home = HomePage.objects.first() + if self.home is None: + root_page = self.site.root_page.specific + self.home = HomePage(title="Navigation Test Home", slug="navigation-templatetag-test") + root_page.add_child(instance=self.home) + self.home.save_revision().publish() # Create a content page for testingBased on past review comments.
src/navigation/templatetags/navigation_tags.py (1)
43-115: N+1 query problem: prefetch pages and translations upfront.The
process_menu_itemfunction triggers separate database queries for each page's locale lookup (page.get_translation()at lines 55 and 58), plus potential queries forurlandtitleproperties. A menu with 20 items could generate 40+ queries, severely impacting performance.The past review provides a detailed prefetching solution that collects all page IDs upfront and uses
select_related('locale').prefetch_related('localized')to load pages and translations in a single query. Consider implementing that approach or using Django'sprefetch_related_objects()before processing menu items.Based on past review comments.
🧹 Nitpick comments (2)
src/navigation/tests.py (2)
226-476: Consider extracting MockBlockValue to reduce duplication.The
MockBlockValueclass is defined inline in 10+ test methods with near-identical implementations. Extract it to a helper method or setUp to improve maintainability.Example refactor:
class TemplateTagTests(WagtailTestUtils, TestCase): """Tests for navigation template tags.""" def _make_mock_block(self, block_type, value): """Helper to create mock block values for testing.""" class MockBlockValue: def __init__(self, bt, val): self.block_type = bt self.value = val return MockBlockValue(block_type, value)Then replace inline definitions with:
item = self._make_mock_block("page_link", value)
496-520: Consider moving Template imports to the module level.The
from django.template import Context, Templatestatement appears inside multiple test methods (lines 496, 510, 549, 577, 621, 658, 702, 736) rather than at the top of the file. Moving to module-level imports would follow Python conventions.Apply this diff:
from django.test import RequestFactory, TestCase +from django.template import Context, Template from wagtail.models import Locale, Site from wagtail.test.utils import WagtailTestUtilsThen remove the inline imports from test methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/navigation/templatetags/navigation_tags.py(1 hunks)src/navigation/tests.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/{tests.py,tests/**/*.py}
📄 CodeRabbit inference engine (AGENTS.md)
Organize tests with descriptive classes by feature: ModelTests, StreamFieldTests, TranslationTests, AdminTests, IntegrationTests
Files:
src/navigation/tests.py
🧠 Learnings (1)
📚 Learning: 2025-10-26T14:57:19.987Z
Learnt from: CR
Repo: WesternFriend/QuakerCMS PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-26T14:57:19.987Z
Learning: Applies to src/**/{tests.py,tests/**/*.py} : Organize tests with descriptive classes by feature: ModelTests, StreamFieldTests, TranslationTests, AdminTests, IntegrationTests
Applied to files:
src/navigation/tests.py
🧬 Code graph analysis (2)
src/navigation/templatetags/navigation_tags.py (1)
src/navigation/models.py (1)
NavigationMenuSetting(11-27)
src/navigation/tests.py (4)
src/content/models.py (1)
ContentPage(40-69)src/home/models.py (1)
HomePage(11-34)src/navigation/models.py (1)
NavigationMenuSetting(11-27)src/navigation/templatetags/navigation_tags.py (1)
process_menu_item(43-115)
🔇 Additional comments (3)
src/navigation/tests.py (1)
13-14: Well-organized test structure.The test classes follow the coding guidelines with descriptive feature-based organization:
ModelTests,StreamFieldTests,IntegrationTests, andTemplateTagTests. This structure enhances readability and maintainability.As per coding guidelines.
src/navigation/templatetags/navigation_tags.py (2)
47-75: Solid error handling and locale fallback logic.The page_link processing gracefully handles missing pages, unpublished pages, and translation failures with a sensible fallback chain (current locale → default locale → original page). The anchor handling and is_current detection are also correct.
90-113: Dropdown recursion properly filters invalid children.The dropdown processing correctly filters out invalid child items and only includes the dropdown if at least one valid child exists. The
is_openflag based on whether a child is current is a good UX pattern for highlighting active sections.
- Document architectural decision to NOT use block templates - Explain context-dependent rendering (locale, current page, viewport) - Note business logic separation (template tag processes, template renders) - Clarify desktop vs mobile use different ARIA patterns for same data - Prevent future refactoring that would leak business logic into templates - Reference navigation_tags.py as the processing layer
- Added clean() method to DropdownMenuBlock that validates at least one item exists - Removed required=True from items field to allow custom validation - Validation catches StructBlockValidationError and raises clearer ValidationError - Added test test_dropdown_block_validates_non_empty_items to verify behavior - All 27 navigation tests passing
- Added id="dropdown-trigger-{{ forloop.counter }}" to dropdown button elements
- This allows aria-labelledby to properly associate the dropdown menu with its trigger
- Improves screen reader accessibility by establishing valid ARIA relationship
- Added _collect_page_ids() helper to recursively gather all page IDs from menu structure
- Modified navigation_menu() to prefetch all pages with select_related('locale') in single query
- Added pages_cache parameter to process_menu_item() to use prefetched pages
- Eliminates N+1 query problem where each page triggered separate locale lookups
- A menu with 20 items now uses ~3 queries instead of 40+
- All 27 navigation tests passing
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/navigation/tests.py (1)
16-224: Provision a HomePage fixture before using it.
HomePage.objects.first()can beNonein a fresh test database, so these tests crash withAttributeErrorbefore making assertions. This was raised earlier and is still unresolved—please create/publish aHomePageinsetUp(and reuse it in the tests) so the suite is self-contained. Apply the same helper toStreamFieldTests,IntegrationTests, andTemplateTagTests.def setUp(self): - """Set up test data.""" - self.site = Site.objects.get(is_default_site=True) + super().setUp() + self.site = Site.objects.get(is_default_site=True) + self.home = HomePage.objects.first() + if self.home is None: + root_page = self.site.root_page.specific + self.home = HomePage( + title="Navigation Test Home", + slug="navigation-test-home", + ) + root_page.add_child(instance=self.home) + self.home.save_revision().publish() @@ - # Get the home page - home = HomePage.objects.first() - menu = NavigationMenuSetting.objects.create( site=self.site, menu_items=[ { "type": "page_link", "value": { - "page": home.id, + "page": self.home.id,src/navigation/templates/navigation/navigation.html (1)
33-50: Restore thearia-labelledbytarget on dropdown triggers.The submenu references
aria-labelledby="dropdown-trigger-{{ forloop.counter }}", but the button still lacks thatid, so assistive tech cannot associate the trigger and popup. Please add the missingid.<li role="none" class="dropdown dropdown-hover"> <button + id="dropdown-trigger-{{ forloop.counter }}" role="menuitem" aria-haspopup="true" aria-expanded="false"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
specs/002-nav-menu-system/checklists/requirements.md(1 hunks)specs/002-nav-menu-system/plan.md(1 hunks)specs/002-nav-menu-system/spec.md(7 hunks)src/navigation/blocks.py(1 hunks)src/navigation/templates/navigation/navigation.html(1 hunks)src/navigation/tests.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/{tests.py,tests/**/*.py}
📄 CodeRabbit inference engine (AGENTS.md)
Organize tests with descriptive classes by feature: ModelTests, StreamFieldTests, TranslationTests, AdminTests, IntegrationTests
Files:
src/navigation/tests.py
🧠 Learnings (2)
📚 Learning: 2025-10-26T14:57:19.987Z
Learnt from: CR
Repo: WesternFriend/QuakerCMS PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-26T14:57:19.987Z
Learning: Applies to src/**/{tests.py,tests/**/*.py} : Organize tests with descriptive classes by feature: ModelTests, StreamFieldTests, TranslationTests, AdminTests, IntegrationTests
Applied to files:
src/navigation/tests.py
📚 Learning: 2025-10-26T14:57:19.987Z
Learnt from: CR
Repo: WesternFriend/QuakerCMS PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-26T14:57:19.987Z
Learning: Applies to src/**/models.py : Define custom StreamField blocks using StructBlock and provide a Meta.template for rendering
Applied to files:
src/navigation/blocks.py
🧬 Code graph analysis (1)
src/navigation/tests.py (5)
src/content/models.py (1)
ContentPage(40-69)src/home/models.py (1)
HomePage(11-34)src/navigation/models.py (1)
NavigationMenuSetting(11-27)src/navigation/templatetags/navigation_tags.py (1)
process_menu_item(43-115)src/navigation/blocks.py (2)
DropdownMenuBlock(76-106)clean(88-102)
🪛 LanguageTool
specs/002-nav-menu-system/spec.md
[grammar] ~268-~268: Ensure spelling is correct
Context: ...ucture including mobile drawer, desktop menubar, and dropdown menus with ARIA semantics...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (7)
specs/002-nav-menu-system/spec.md (7)
18-22: Session 2025-11-03 Q&A additions are clear and well-integrated.The three new questions (caching, error logging, soft limits) establish design decisions that are appropriately deferred to the requirements section. Answers align with the functional requirements added later in the spec.
156-171: Responsive Breakpoints section is well-structured and provides essential context.The new table and usage section clarify the breakpoint-based navigation behavior. Definitions align with standard Tailwind CSS values, and the distinct mobile/tablet vs. desktop interaction models are clearly communicated. This consolidates information that was previously scattered and improves specification readability.
Verify that edge cases on line 149 and FR-032a/c align with this breakpoint mapping (e.g., "< lg" references in requirements match the
lg ≥ 1024pxdefinition).
149-149: Edge case updated to specify breakpoint clearly.Aligns with the new Responsive Breakpoints section by explicitly referencing
< lg/ < 1024px. The addition makes the edge case more testable and consistent with breakpoint terminology used throughout the spec.
202-212: FR-024-Cache and FR-024-Limits are detailed and implementable, with minor clarification needed.The caching requirements specify concrete technical choices (Django cache backend, 1-hour TTL, site/locale keys, ERROR logging). The soft limit approach avoids artificial blockers. However:
- Soft limit threshold: "approaches 100 items" (line 210) is ambiguous—should the warning trigger at 80, 90, or 100?
- Success criteria alignment: Verify that SC-008 or similar covers cache performance validation and that SC-019 includes caching scenarios.
Clarify the soft limit warning threshold (e.g., "when total menu items reaches 90 or above" or "when within 10% of 100-item limit").
229-231: Dropdown interaction requirements properly distinguish breakpoints and interaction models.Requirements clearly specify hover for desktop (
lg+), click/tap for mobile/tablet (< lg), and accessibility fallback across all devices. This aligns with earlier session clarifications and ensures both usability and accessibility. The breakpoint values are consistent with the Responsive Breakpoints section.
241-241: Frontend template consolidation and structure are well-specified.FR-041 consolidates the navigation into a single
navigation.htmltemplate, reducing complexity. The template structure section (lines 268–269) provides comprehensive guidance on component organization, ARIA semantics, DaisyUI integration, and theme support. Skip link placement inbase.htmlas the first focusable element correctly implements WCAG 2.1 requirements.Note on LanguageTool hint: "menubar" is the correct term for a horizontal navigation menu bar and is not a spelling error; this is a false positive from the static analysis tool.
Also applies to: 245-247, 268-269
12-13: Comprehensive specification update with excellent internal consistency and clarity.The additions across Session 2025-11-03 updates, Responsive Breakpoints section, and new functional requirements (FR-024-Cache, FR-024-Limits) are well-integrated. Key strengths:
- Breakpoint consistency: Terminology (
< lg,≥ lg) is used uniformly across new and existing content- Requirements completeness: Caching requirements (1-hour TTL, site/locale keys, ERROR logging, graceful degradation) are specific and implementable
- Soft limits approach: Avoids artificial blockers while providing admin guidance; threshold could be clarified (see earlier comment on line 210)
- Template consolidation: Moving from multiple templates to a single
navigation.htmlsimplifies the architecture- Mobile/desktop behavior: Clearly separated interaction models (hover desktop, click/tap mobile) with accessibility fallbacks
- Accessibility integration: Skip link placement, ARIA semantics, and progressive enhancement with
<details>are properly specifiedMinor note: Success criteria (SC-001 through SC-019) may benefit from additional criteria covering cache invalidation validation and soft limit behavior, but these can be addressed iteratively if needed.
Also applies to: 18-22, 156-171, 202-212, 229-231, 245-247, 268-269
- Modified ModelTests, StreamFieldTests, and IntegrationTests setUp methods - Each setUp now checks if HomePage.objects.first() returns None - If no HomePage exists, creates one as a child of the root page - Stores HomePage instance on self.home for reuse across all tests - Prevents AttributeError when tests expect a HomePage to exist - All 27 navigation tests passing
- Move empty items check BEFORE super().clean() - This prevents catching/swallowing parent validation errors - Custom message still provided for empty dropdown case - Test passing confirms proper error propagation
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/navigation/templates/navigation/navigation.html (1)
232-233: Consider updating both drawer labels for consistency.The
querySelector('label[for="nav-drawer"]')on line 232 only selects the first label (the hamburger button at line 75), not the close button (line 101). While the hamburger button is the primary control, updating both labels'aria-expandedstate would be more consistent.Apply this diff to update both labels:
// Update drawer button aria-expanded state const drawerToggle = document.getElementById('nav-drawer'); - const drawerButton = document.querySelector('label[for="nav-drawer"]'); + const drawerButtons = document.querySelectorAll('label[for="nav-drawer"]'); - if (drawerToggle && drawerButton) { + if (drawerToggle && drawerButtons.length > 0) { drawerToggle.addEventListener('change', () => { - drawerButton.setAttribute('aria-expanded', drawerToggle.checked); + drawerButtons.forEach(button => { + button.setAttribute('aria-expanded', drawerToggle.checked); + }); }); }src/navigation/templatetags/navigation_tags.py (2)
40-48: Add prefetch_related to reduce translation lookup queries.The current code fetches pages with
select_related("locale")but doesn't prefetch translations. Eachpage.get_translation()call on line 100 triggers a separate database query, causing N+1 query problems. A menu with 20 items could generate 20+ additional queries for locale lookups.Note: While
get_translation()doesn't automatically use the prefetch cache, prefetching translations enables manual lookup optimization inprocess_menu_item.Apply this diff to prefetch translations:
if page_ids: # Fetch pages and prefetch their locale relations - # Note: We can't prefetch translations directly, so we fetch all locales - # and let get_translation() handle the lookup efficiently - pages = Page.objects.filter(id__in=page_ids).select_related("locale") + pages = Page.objects.filter(id__in=page_ids).select_related( + "locale" + ).prefetch_related("localized") for page in pages: pages_cache[page.id] = pageTo fully leverage the prefetch, consider manually looking up translations in
process_menu_itemfrom the prefetched queryset instead of callingget_translation(), though this adds complexity.
100-107: Consider removing unnecessary AttributeError catch.Lines 101 and 104 catch
AttributeError, butpage.get_translation()only raisesPage.DoesNotExist, notAttributeError. Since line 91 already validates thatpageexists, theAttributeErrorcatch is likely unnecessary defensive code.If you want to simplify the exception handling, you can catch only
Page.DoesNotExist:# Try to get page translation try: localized_page = page.get_translation(current_locale) - except (Page.DoesNotExist, AttributeError): + except Page.DoesNotExist: try: localized_page = page.get_translation(default_locale) - except (Page.DoesNotExist, AttributeError): + except Page.DoesNotExist: # If page doesn't exist in any locale or doesn't support translations, # use the original page localized_page = pageHowever, keeping
AttributeErroras defensive programming is also acceptable if you anticipate edge cases with non-standard page types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/navigation/blocks.py(1 hunks)src/navigation/templates/navigation/navigation.html(1 hunks)src/navigation/templatetags/navigation_tags.py(1 hunks)src/navigation/tests.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/navigation/tests.py
- src/navigation/blocks.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/navigation/templatetags/navigation_tags.py (1)
src/navigation/models.py (1)
NavigationMenuSetting(11-27)
🔇 Additional comments (3)
src/navigation/templates/navigation/navigation.html (1)
10-15: Excellent documentation of intentional duplication.The comments clearly explain why desktop and mobile rendering are separate despite similar data iteration, highlighting the different ARIA patterns, interaction models, and JavaScript requirements. This prevents future refactoring mistakes.
Also applies to: 109-113
src/navigation/templatetags/navigation_tags.py (2)
11-19: Clean recursive helper for page ID collection.The
_collect_page_idsfunction efficiently handles both flat links and nested dropdowns, correctly extracting page IDs for prefetching.
135-159: Robust dropdown handling with proper child filtering.The dropdown processing correctly:
- Recursively processes children with locale translations
- Filters out invalid children (line 147 check)
- Only includes dropdowns with valid children (line 153)
- Sets
is_openstate when any child is current (line 158)
- Changed querySelector to querySelectorAll for all drawer labels - Now updates both hamburger button (line 75) and close button (line 101) - Improves consistency in ARIA state management - Screen readers get accurate state for both controls
- Collect all translation_keys from menu pages - Fetch all translations in single query - Build translations_cache keyed by (translation_key, locale_id) - Update process_menu_item to check cache before calling get_translation() - Falls back to database query if translation not in cache - Eliminates additional N+1 queries for locale lookups in menus with translated pages
- Remove unnecessary AttributeError catch - get_translation() only raises Page.DoesNotExist - Page existence already validated earlier in the function - Makes exception handling more precise and cleaner
- Changed z-index from z-[1] to z-50 - Ensures dropdown appears above modals, sticky headers, and overlays - Prevents dropdown from being hidden behind other UI elements - Uses standard Tailwind z-index scale (z-50 is appropriate for dropdowns)
- Temporarily make menu visible for accurate getBoundingClientRect() measurement - Use visibility:hidden + display:block to render menu without showing it - Prevents getting width:0 for keyboard-triggered dropdown opens - Apply positioning based on correct measurements - Restore display/visibility after positioning - Set aria-expanded after visual state is configured - Ensures dropdown positioning works reliably for both mouse and keyboard interactions
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 (2)
src/navigation/templates/navigation/navigation.html(1 hunks)src/navigation/templatetags/navigation_tags.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/navigation/templates/navigation/navigation.html
🧰 Additional context used
🧬 Code graph analysis (1)
src/navigation/templatetags/navigation_tags.py (1)
src/navigation/models.py (1)
NavigationMenuSetting(11-27)
🔇 Additional comments (4)
src/navigation/templatetags/navigation_tags.py (4)
11-19: LGTM - clean recursive ID collection.The helper correctly collects page IDs from both page_link items and nested dropdown structures.
22-83: Excellent query optimization with prefetching.The implementation successfully addresses the N+1 query issue raised in the previous review by:
- Collecting all page IDs upfront
- Fetching pages in bulk with
select_related("locale")- Prefetching all translations in a single query
- Building efficient lookup caches
This reduces queries from potentially 40+ (for a 20-item menu) down to 2-3 bulk queries.
153-164: LGTM - external link handling is correct.The implementation properly constructs URLs with optional anchors and correctly sets
is_current: Falsefor external links.
166-191: LGTM - robust dropdown handling with good UX.The recursive processing correctly:
- Passes caches to child items to maintain query optimization
- Filters out empty dropdowns
- Sets
is_openwhen any child is current (enables parent highlighting for active sections)
SECURITY FIX: Prevent unpublished translations from appearing in navigation - Check localized_page.live before returning menu item - Original page was checked for live status, but translated pages were not - translations_cache doesn't filter by live=True - get_translation() method doesn't filter by live status - This could expose unpublished content in non-default locales - Now returns None if localized translation is not live - Ensures only published content appears in navigation menu
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/navigation/templatetags/navigation_tags.py (1)
56-59: Optimize translations prefetch to only fetch needed locales.The current implementation fetches translations for all locales, but only
current_localeanddefault_localeare actually used (see lines 120 and 125). With many locales, this can fetch significantly more data than needed.Apply this diff to optimize the query:
# Prefetch all translations for these pages in a single query if translation_keys: all_translations = Page.objects.filter( translation_key__in=translation_keys, + locale__in=[current_locale, default_locale], ).select_related("locale")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/navigation/templatetags/navigation_tags.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/navigation/templatetags/navigation_tags.py (1)
src/navigation/models.py (1)
NavigationMenuSetting(11-27)
🔇 Additional comments (1)
src/navigation/templatetags/navigation_tags.py (1)
86-197: LGTM! Excellent implementation with proper safeguards.The
process_menu_itemfunction correctly implements locale-aware navigation with comprehensive safeguards:
- Lines 109 and 140-142 properly validate live status for both original and localized pages, addressing the past critical issue
- Translation fallback logic (current → default → original) is correct
- Recursive dropdown processing with proper validation
- Clean separation of concerns for different item types
PERFORMANCE: Filter translations query to current and default locales only - Previously fetched translations for ALL locales in the system - Only current_locale and default_locale are actually used (lines 120, 125) - For sites with many locales (e.g., 10+ languages), this was wasteful - Now filters to locale__in=[current_locale, default_locale] - Reduces data transfer and memory usage - Example: Site with 15 locales now fetches 2 instead of 15 translations per page - ~87% reduction in translation data for 15-locale site
…d navigation app details
PageLinkBlock is a StructBlock with 'page', 'custom_title', and 'anchor' fields. The scaffold command was incorrectly setting the value to raw page IDs (integers) instead of proper dict structure. This was causing TypeError: 'argument of type 'int' is not iterable' when rendering navigation menu. Changes: - Updated scaffold_navbar_content to create proper PageLinkBlock values - Added 'anchor' field to ExternalLinkBlock values (was missing) - Added explanatory comment about block structure - All 27 navigation tests passing
…ment and testing configuration
… for package management and Python commands
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/navigation/templates/navigation/navigation.html (1)
26-26: Raise dropdown z-index so menus stay on top.Lines 26 and 97 still use
z-[1]on the dropdown menus, so the popovers can render behind other sticky or overlay content. Please bump these to a higher stacking context.- class="menu menu-sm dropdown-content bg-white text-black rounded-box z-[1] mt-3 w-52 p-2 shadow hidden" + class="menu menu-sm dropdown-content bg-white text-black rounded-box z-50 mt-3 w-52 p-2 shadow hidden" ... - <ul class="p-2 bg-white text-black rounded-box shadow z-[1]" role="menu"> + <ul class="p-2 bg-white text-black rounded-box shadow z-50" role="menu">Also applies to: 97-97
🧹 Nitpick comments (5)
README.md (3)
139-139: Consider deferring the Navigation System milestone until PR is merged.Marking the Navigation System as ✅ complete (line 139) may be premature since this PR (#48) is still under review and not yet merged. Consider updating this milestone to reflect the in-progress status until the PR is finalized and deployed.
114-120: Add concrete guidance to the "For Site Administrators" and "For Content Managers" sections.Lines 115-120 contain only placeholder text ("coming soon" and "in development") without any actionable guidance or direction. Since this PR introduces navigation system configuration, consider providing at least skeleton documentation or links to where these guides will be located.
For example:
### For Site Administrators Deploy and administer QuakerCMS instances: - Configuring navigation menus and site-wide settings - Managing user roles and permissions - Deployment and hosting options - Troubleshooting and support _Full documentation: Coming soon._ ### For Content Managers Create and manage content for your community: - Creating and editing pages - Setting up navigation menus - Publishing content in multiple languages - Managing site-wide settings _Full documentation: In development._
32-39: Highlight performance and reliability improvements in "Currently Implemented".The "Currently Implemented" section appropriately lists user-facing features. However, the PR specifically implements caching and error logging for the navigation system—performance and reliability features that deserve mention. Consider adding a point about these technical improvements:
- **⚡ Performance Optimized** - Caching and error logging for reliable, fast navigationThis signals to both end-users and operators that the navigation system is production-ready with performance safeguards.
CONTRIBUTING.md (2)
109-122: Tailwind CSS setup section is practical but could clarify the return path.The navigation between directories is clear, but a note reminding developers they're back in the
srcdirectory after step 10 would improve clarity for the next steps. Consider adding a comment after line 121.# Return to the src directory cd ../.. + +# You are now back in the src directoryAlternatively, rephrase line 121 to:
# Return to the src directory (QuakerCMS/src)
408-437: Address static analysis feedback: stylistic improvements.Two minor wording improvements recommended by static analysis:
Line ~432: "all the time instead of only on focus" → Consider "constantly appears instead of appearing only on focus" or "remains visible instead of appearing only on focus" for stronger wording.
Line ~434: "requires compiled CSS" → Replace with "requires compiled CSS" to "needs to be compiled" for clarity.
-**Problem**: The accessibility skip link appears all the time instead of only on focus. +**Problem**: The accessibility skip link appears constantly instead of appearing only on focus. -**Solution**: The Tailwind CSS watcher needs to be running. The `sr-only` utility class requires compiled CSS: +**Solution**: The Tailwind CSS watcher needs to be running. The `sr-only` utility class needs to be compiled:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CONTRIBUTING.md(7 hunks)README.md(3 hunks)src/core/templates/base.html(2 hunks)src/navigation/README.md(1 hunks)src/navigation/management/commands/scaffold_navbar_content.py(1 hunks)src/navigation/migrations/0003_alter_navigationmenusetting_menu_items.py(1 hunks)src/navigation/templates/navigation/navigation.html(1 hunks)src/navigation/templatetags/navigation_tags.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/navigation/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/templates/base.html
🧰 Additional context used
📓 Path-based instructions (1)
src/**/migrations/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Check migration files into version control
Files:
src/navigation/migrations/0003_alter_navigationmenusetting_menu_items.py
🧠 Learnings (1)
📚 Learning: 2025-10-26T14:57:19.987Z
Learnt from: CR
Repo: WesternFriend/QuakerCMS PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-26T14:57:19.987Z
Learning: Run makemigrations after model changes; StreamField changes require migrations
Applied to files:
src/navigation/migrations/0003_alter_navigationmenusetting_menu_items.py
🧬 Code graph analysis (2)
src/navigation/templatetags/navigation_tags.py (1)
src/navigation/models.py (1)
NavigationMenuSetting(11-27)
src/navigation/management/commands/scaffold_navbar_content.py (4)
src/content/models.py (1)
ContentPage(40-69)src/home/models.py (1)
HomePage(11-34)src/navigation/models.py (1)
NavigationMenuSetting(11-27)src/locales/models.py (1)
save(126-151)
🪛 LanguageTool
CONTRIBUTING.md
[style] ~432-~432: Consider using a different adverb to strengthen your wording.
Context: ...Problem*: The accessibility skip link appears all the time instead of only on focus. Solution...
(ALL_THE_TIME_CONSTANTLY)
[style] ~434-~434: The double modal “requires compiled” is nonstandard (only accepted in certain dialects). Consider “to be compiled”.
Context: ...g. The sr-only utility class requires compiled CSS: ```bash cd src/theme/static_src n...
(NEEDS_FIXED)
🔇 Additional comments (11)
CONTRIBUTING.md (9)
25-40: Installation instructions are clear and comprehensive.The three installation options (macOS/Linux, Windows, and pip) provide good flexibility. The organization follows a logical platform-specific progression. No issues detected.
51-54:uv synccommand explanation is helpful.The bullet-point breakdown clearly explains what
uv syncdoes, which helps developers understand the setup process beyond just running a command.
59-81: Virtual environment activation steps are well-organized.Platform-specific instructions (macOS/Linux, Windows PowerShell, Windows Command Prompt) are clearly separated, making it easy for developers on any OS to find their path. The additional step to navigate into the Django project directory (line 78-81) is a practical addition.
124-147: Test navigation scaffolding instructions are thorough and well-documented.The explanation of what
scaffold_navbar_contentcreates, plus the--deleteoption for resetting, provides developers with clear expectations and recovery paths. This is excellent for onboarding.
178-215: Development workflow section clearly explains the two-server requirement.The distinction between Option A (two terminals) and Option B (honcho) is well-explained. Option A is marked as recommended, which aligns with typical Django development patterns. The explanation of what each server does (auto-reload for Python/templates, auto-rebuild for CSS) helps developers understand why both are needed.
217-251: Navigation menu configuration section strikes a good balance.The section provides both a quick-setup path (scaffold command) and manual configuration steps. The inline documentation of menu item types and automatic features (aria-current, translations, responsive behavior, WCAG compliance) is valuable. However, the manual configuration step links directly to the admin URL without explanation of the streamlined UI—this is acceptable for experienced developers but may benefit less experienced contributors.
472-487: Database migration troubleshooting section is thorough.The progression from checking migration status to applying migrations to handling conflicts provides a solid troubleshooting path. Clear and practical.
519-583: Quick Reference section significantly improves developer ergonomics.The consolidated quick reference with common commands, important URLs, key files, and development tips is excellent for reducing context-switching during development. The URLs for navigation menu settings (line 566) and key files list (lines 570-575) directly support the new navigation system features.
339-370: Project Structure diagram accurately reflects new navigation app.The updated diagram correctly includes:
navigation/app with blocks, models, templatetags, templates, management commands, and docs (lines 344-350)- Updated theme and core structure references (lines 339-341, 353-358)
- New directories like
specs/(line 361)The diagram is consistent with the file-level guidance provided in quick reference section. No issues detected.
src/navigation/migrations/0003_alter_navigationmenusetting_menu_items.py (1)
16-100: StreamField schema looks consistent.The altered StreamField definition lines up with the updated block hierarchy and settings, so no further tweaks are needed here.
src/navigation/templatetags/navigation_tags.py (1)
36-144: Nice job prefetching pages and guarding unpublished translations.The upfront ID collection plus shared caches avoid the N+1 lookups, and the live checks keep unpublished translations out of the menu. This is a solid implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/navigation/tests.py (2)
762-766: Ensure HomePage fixture exists for consistency.Similar to
TemplateTagTests, this class retrievesHomePage.objects.first()without checking forNone. While the management command tests may create pages, ensuring the fixture exists insetUpwould improve test isolation and consistency withModelTests,StreamFieldTests, andIntegrationTests.Apply this diff:
def setUp(self): """Set up test data.""" self.site = Site.objects.get(is_default_site=True) self.locale = Locale.get_default() self.home = HomePage.objects.first() + if not self.home: + # Get the root page + root = Site.objects.get(is_default_site=True).root_page + self.home = HomePage( + title="Test Home Page", + slug="home", + ) + root.add_child(instance=self.home) + self.home.save_revision().publish()
247-261: Ensure HomePage fixture exists before use.Unlike
ModelTests,StreamFieldTests, andIntegrationTestswhich guarantee aHomePageexists insetUp, this class retrievesHomePage.objects.first()without checking forNone. Line 260 will raiseAttributeErrorif noHomePageexists when tests run in isolation.Apply this diff to guarantee the fixture:
def setUp(self): """Set up test data.""" self.factory = RequestFactory() self.site = Site.objects.get(is_default_site=True) self.default_locale = Locale.get_default() self.home = HomePage.objects.first() + if not self.home: + # Get the root page + root = Site.objects.get(is_default_site=True).root_page + self.home = HomePage( + title="Test Home Page", + slug="home", + ) + root.add_child(instance=self.home) + self.home.save_revision().publish() # Create a content page for testing self.about_page = ContentPage(
🧹 Nitpick comments (4)
.vscode/settings.json (1)
14-14: Consider cross-platform Python path compatibility.The path
${workspaceFolder}/.venv/bin/pythonis Unix/Linux/Mac specific. Windows developers will need.venv\Scripts\python.exeinstead.While VSCode can often auto-detect Python environments (making explicit path optional), you may want to either:
- Document this setup requirement for Windows developers, or
- Consider leveraging VSCode's environment detection without hardcoding a platform-specific path
This is a minor concern since developers can override settings locally if needed.
src/navigation/tests.py (3)
178-193: Consider moving imports to module level.The test imports
ValidationErrorandDropdownMenuBlockinside the method, which is inconsistent with the module-level imports at the top of the file. While this works, moving these to the top would improve consistency.Apply this diff to move imports to module level:
+from django.core.exceptions import ValidationError from django.test import RequestFactory, TestCase from wagtail.models import Locale, Site from wagtail.test.utils import WagtailTestUtils from content.models import ContentPage from home.models import HomePage +from navigation.blocks import DropdownMenuBlock from navigation.models import NavigationMenuSetting from navigation.templatetags.navigation_tags import process_menu_itemAnd update the test method:
def test_dropdown_block_validates_non_empty_items(self): """Dropdown menu block requires at least one item.""" - from django.core.exceptions import ValidationError - - from navigation.blocks import DropdownMenuBlock - block = DropdownMenuBlock() # Empty items should raise validation error
263-530: Consider extracting MockBlockValue to reduce repetition.Multiple tests define inline
MockBlockValueclasses with nearly identical structure. Extracting to a helper method or test fixture would improve maintainability.Example helper in the test class:
def _create_mock_block(self, block_type, value): """Helper to create mock block values for testing.""" class MockBlockValue: def __init__(self, bt, val): self.block_type = bt self.value = val return MockBlockValue(block_type, value)Then simplify tests:
def test_process_menu_item_page_link(self): """Process page link returns correct structure.""" value = { "page": self.home, "custom_title": "Custom Home", "anchor": "top", } item = self._create_mock_block("page_link", value) # ... rest of test
1-845: Consider adding TranslationTests for multi-lingual support coverage.The PR emphasizes multi-lingual support as a key feature, and the coding guidelines recommend organizing tests with a
TranslationTestsclass. Whileprocess_menu_itemincludes locale and translation handling (visible in the relevant code snippets), there are no explicit tests verifying translation/locale-switching behavior for navigation menu items.Based on coding guidelines
Consider adding a test class like:
class TranslationTests(WagtailTestUtils, TestCase): """Tests for navigation menu translation and multi-lingual support.""" def setUp(self): """Set up test data with multiple locales.""" # Setup default and alternate locales # Create translated pages # Create navigation menus with translated content def test_menu_displays_translated_pages(self): """Menu items display pages in current locale.""" # Test that navigation menu shows translated version of pages def test_menu_falls_back_to_default_locale(self): """Menu falls back to default locale when translation unavailable.""" # Test fallback behavior def test_dropdown_items_respect_locale(self): """Dropdown menu items are translated correctly.""" # Test dropdown translation behavior
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.vscode/settings.json(1 hunks)AGENTS.md(3 hunks)src/navigation/tests.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- AGENTS.md
🧰 Additional context used
📓 Path-based instructions (1)
src/**/{tests.py,tests/**/*.py}
📄 CodeRabbit inference engine (AGENTS.md)
Organize tests with descriptive classes by feature: ModelTests, StreamFieldTests, TranslationTests, AdminTests, IntegrationTests
Files:
src/navigation/tests.py
🧠 Learnings (1)
📚 Learning: 2025-10-26T14:57:19.987Z
Learnt from: CR
Repo: WesternFriend/QuakerCMS PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-26T14:57:19.987Z
Learning: Applies to src/**/{tests.py,tests/**/*.py} : Organize tests with descriptive classes by feature: ModelTests, StreamFieldTests, TranslationTests, AdminTests, IntegrationTests
Applied to files:
src/navigation/tests.py
🧬 Code graph analysis (1)
src/navigation/tests.py (5)
src/content/models.py (1)
ContentPage(40-69)src/home/models.py (1)
HomePage(11-34)src/navigation/models.py (1)
NavigationMenuSetting(11-27)src/navigation/templatetags/navigation_tags.py (1)
process_menu_item(87-198)src/navigation/blocks.py (2)
DropdownMenuBlock(76-102)clean(88-98)
🔇 Additional comments (3)
.vscode/settings.json (1)
13-24: Configuration supports Python testing workflow well.The VSCode settings align with the new navigation app's testing and development requirements. Python environment auto-activation and unittest framework configuration are appropriately set up for the Django/Wagtail project structure.
src/navigation/tests.py (2)
13-88: LGTM! ModelTests class is well-structured.The test class properly ensures HomePage fixture exists in setUp (addressing previous review feedback) and provides good coverage of NavigationMenuSetting model creation, page links, external links, and request-based retrieval.
196-241: LGTM! IntegrationTests class follows the established pattern.The test class consistently ensures HomePage fixture exists and provides appropriate end-to-end coverage for menu creation with multiple item types.
…sample pages and improve deletion logic
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 (2)
src/navigation/management/commands/scaffold_navbar_content.py(1 hunks)src/navigation/tests.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/navigation/tests.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/navigation/management/commands/scaffold_navbar_content.py (3)
src/content/models.py (1)
ContentPage(40-69)src/home/models.py (1)
HomePage(11-34)src/navigation/models.py (1)
NavigationMenuSetting(11-27)
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/navigation/management/commands/scaffold_navbar_content.py(1 hunks)src/navigation/tests.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/{tests.py,tests/**/*.py}
📄 CodeRabbit inference engine (AGENTS.md)
Organize tests with descriptive classes by feature: ModelTests, StreamFieldTests, TranslationTests, AdminTests, IntegrationTests
Files:
src/navigation/tests.py
🧠 Learnings (2)
📚 Learning: 2025-10-26T14:57:19.987Z
Learnt from: CR
Repo: WesternFriend/QuakerCMS PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-26T14:57:19.987Z
Learning: Applies to src/**/{tests.py,tests/**/*.py} : Organize tests with descriptive classes by feature: ModelTests, StreamFieldTests, TranslationTests, AdminTests, IntegrationTests
Applied to files:
src/navigation/tests.py
📚 Learning: 2025-10-26T14:57:19.987Z
Learnt from: CR
Repo: WesternFriend/QuakerCMS PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-26T14:57:19.987Z
Learning: Languages are configured at runtime via LocaleSettings in Wagtail admin, not hard-coded in code
Applied to files:
src/navigation/tests.py
🧬 Code graph analysis (2)
src/navigation/tests.py (5)
src/content/models.py (1)
ContentPage(40-69)src/home/models.py (1)
HomePage(11-34)src/navigation/blocks.py (2)
DropdownMenuBlock(76-102)clean(88-98)src/navigation/models.py (1)
NavigationMenuSetting(11-27)src/navigation/templatetags/navigation_tags.py (1)
process_menu_item(87-198)
src/navigation/management/commands/scaffold_navbar_content.py (3)
src/content/models.py (1)
ContentPage(40-69)src/home/models.py (1)
HomePage(11-34)src/navigation/models.py (1)
NavigationMenuSetting(11-27)
🔇 Additional comments (5)
src/navigation/tests.py (3)
15-16: Excellent test organization.The test classes are well-organized by feature (ModelTests, StreamFieldTests, IntegrationTests, TemplateTagTests, TranslationTests, ManagementCommandTests) with descriptive names, comprehensive coverage, and clear docstrings.
Based on coding guidelines.
Also applies to: 93-94, 194-195, 242-243, 710-711, 1056-1057
710-1054: Comprehensive translation test coverage.The TranslationTests class thoroughly validates locale-aware navigation rendering, fallback behavior, unpublished translation filtering, and mixed-locale scenarios. The
activate_localecontext manager helper (lines 1049-1053) is a clean approach for testing locale-specific behavior.
1056-1319: Thorough management command test coverage.The ManagementCommandTests class comprehensively validates the scaffold command behavior including page creation, menu creation, deletion with
--delete, idempotency without--delete, selective deletion of scaffolded pages, and error handling for missing sites.src/navigation/management/commands/scaffold_navbar_content.py (2)
128-147: Idempotent page creation approach works well.The
get_or_create_pagehelper function ensures the command can be run multiple times without errors or duplicates, making the scaffolding operation safe and predictable.
85-123: Deletion scope correctly restricted to scaffolded content.The deletion logic properly restricts removal to known scaffolded slugs (lines 87-91) and uses
page.get_descendants()to handle child pages (line 104), preventing accidental deletion of user content.
…nd reuse statistics
…verwrite existing navigation menu
1 participant
Enhance the navigation menu system by adding caching and error logging specifications, clarifying mobile and desktop behavior, and implementing multi-lingual support with accessibility features.
Summary by CodeRabbit
New Features
Documentation