-
Notifications
You must be signed in to change notification settings - Fork 66
Description
I asked Claude to analyze our entire codebase (which I prepared via npx repomix and the new repomix.config.json file in the repo). Here's what it suggested.
Makeability Lab Website Code Analysis
Overall Assessment
The codebase is well-architected with good Django conventions, thoughtful documentation, and strong accessibility considerations. The recent refactoring (v2.0+) shows significant improvements. However, there are several areas for improvement.
🔴 Potential Bugs
1. Possible AttributeError in delete_unused_files command (Lines 1020-1029)
# website/management/commands/delete_unused_files.py
pub_pdf_filename = os.path.basename(pub.pdf_file.path) # Will raise if pdf_file is emptyIf pub.pdf_file is empty/None, calling .path will fail. Add a guard:
if pub.pdf_file:
pub_pdf_filename = os.path.basename(pub.pdf_file.path)2. Missing modified_date field referenced in member.py (Line 26408)
person = Person.objects.filter(url_name__iexact=member_name).order_by('-modified_date').first()The Person model doesn't appear to have a modified_date field—only bio_datetime_modified. This will cause a FieldError.
3. Potential N+1 Query in view_project_people.py (Lines 19000-19003)
for pub in person.publication_set.all():
for proj in pub.projects.all():
projects_published_on.add(proj.short_name)This nested loop causes N+1 queries. Use prefetch_related:
pubs = person.publication_set.prefetch_related('projects').all()4. Race condition in unique filename generation (Lines 22876-22882)
while os.path.exists(new_filename_with_path):
new_filename_with_path = os.path.join(...)Time-of-check to time-of-use (TOCTOU) vulnerability. The file could be created between the check and the save. Consider using os.open with O_CREAT | O_EXCL flags or Django's storage backend.
🟡 Code Quality Issues
1. Inconsistent None checking (throughout models)
# Some places use:
if self.end_date == None: # Should use `is None`
# Correct form:
if self.end_date is None:Found in project_role.py line 2376.
2. Unused imports in several files
website/models/person.py:Coalesceimported twice (lines 22820, 22825)- Several management commands import modules they don't use
3. Hardcoded values that should be constants
# member.py line 26421
news_items_num = 5 # Good - but then not used consistently elsewhereConsider a central constants.py or settings.
4. Console.log statements left in production JS (Lines 16288, 16308, etc.)
console.log('pub_admin_custom.js has been loaded');
console.log('DOMContentLoaded called');These should be removed or wrapped in debug checks for production.
5. Typo in comment (Line 2558)
# Get the most recent publication asscoaited with this project with a videoShould be "associated".
🟢 Best Practices Recommendations
1. Security: Upgrade jQuery version
<!-- Currently using jQuery 1.9.1 (line 27691) - very old -->
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.9.1/jquery.min.js"></script>jQuery 1.9.1 is from 2013 and has known vulnerabilities. Since Bootstrap 3.3.6 is used, consider migrating to Bootstrap 5 with modern jQuery (3.x) or dropping jQuery entirely.
2. Add database indexes for common queries
# In Position model, add indexes for frequently filtered fields:
class Meta:
indexes = [
models.Index(fields=['end_date']),
models.Index(fields=['person', 'end_date']),
]3. Use Django's select_related / prefetch_related consistently
Good usage in get_project_leadership(), but missing in several views. For example, in publications.py view, prefetch authors and projects.
4. Add model validation
# In ProjectRole, add clean() method:
def clean(self):
if self.end_date and self.start_date and self.end_date < self.start_date:
raise ValidationError("End date cannot be before start date")5. Consider using Django's timezone-aware dates consistently
# Some places use:
from datetime import date
date.today()
# Should use:
from django.utils import timezone
timezone.now().date() # For timezone-aware comparisons6. Static file organization
Consider using Django's ManifestStaticFilesStorage for cache-busting in production, and SCSS compilation for CSS maintainability.
7. Add type hints to Python code
# Current:
def get_filename_for_artifact(last_name, title, forum_name, date, ext, suffix=None, max_pub_title_length=-1):
# Better:
def get_filename_for_artifact(
last_name: str,
title: str,
forum_name: str,
date: datetime,
ext: str,
suffix: Optional[str] = None,
max_pub_title_length: int = -1
) -> str:✅ Things Done Well
- Excellent documentation in templates with version numbers and clear block comments
- Strong accessibility - WCAG compliance, focus states, skip links, ARIA labels
- Good use of Django signals for model lifecycle events
- Clean separation of concerns (models, views, templates, admin)
- Thoughtful file naming conventions for artifacts
- Design tokens CSS for consistent theming
- Good caching with
@cached_propertyon expensive model methods - Comprehensive admin customization for data entry workflows
Priority Fix List
| Priority | Issue | Impact |
|---|---|---|
| High | Fix modified_date field reference | Runtime error |
| High | Guard pdf_file.path access | Runtime error |
| Medium | Upgrade jQuery | Security |
| Medium | Fix N+1 queries | Performance |
| Low | Remove console.logs | Clean code |
| Low | Add type hints | Maintainability |