feat: Add documentation notes system with UI components and tests#999
Open
Flashl3opard wants to merge 4 commits intoalphaonelabs:mainfrom
Open
feat: Add documentation notes system with UI components and tests#999Flashl3opard wants to merge 4 commits intoalphaonelabs:mainfrom
Flashl3opard wants to merge 4 commits intoalphaonelabs:mainfrom
Conversation
Contributor
👀 Peer Review RequiredHi @Flashl3opard! This pull request does not yet have a peer review. Before this PR can be merged, please request a review from one of your peers:
Thank you for contributing! 🎉 |
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds a documentation-notes feature: new Django models, migration, admin interfaces, views and URL routes, templates and components, a TypeScript frontend module for AJAX navigation/progress, and a management command to seed sample content. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser
participant Frontend as Docs TS
participant Server as Django Server
participant DB as Database
Browser->>Frontend: user clicks section link
Frontend->>Server: GET /docs/{topic}/{section}/ (AJAX/content fetch)
Server->>DB: query topic, sections, section content, progress
DB-->>Server: return data
Server-->>Frontend: HTML/JSON content
Frontend->>Browser: render content (marked, MathJax), update UI
Browser->>Frontend: user finishes viewing
Frontend->>Server: POST /docs/{topic}/section/{section}/viewed/ (CSRF)
Server->>DB: mark section viewed, update progress
DB-->>Server: updated progress
Server-->>Frontend: progress JSON
Frontend->>Browser: update progress bar and sidebar state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@static/ts/documentation-notes.ts`:
- Around line 62-64: Current direct binding using
document.querySelectorAll('a[href^="#"]') causes new anchors added by
contentArea.innerHTML replacements to miss the onAnchorClick handler; replace
this with event delegation by attaching a single click listener on the
containing element (e.g., contentArea or document) that checks event.target (or
closest) for 'a[href^="#"]' and calls this.onAnchorClick(event) so newly
injected anchors work without re-binding; update both locations that currently
call document.querySelectorAll (lines ~62-64 and ~184-189) to use the delegated
listener and remove the per-anchor addEventListener usage.
- Around line 184-185: The code sets innerHTML directly from
newContent.innerHTML via contentArea.innerHTML (and another similar assignment
later) — add a sanitization step to defensively clean the HTML before
assignment: call a sanitizer (e.g., DOMPurify.sanitize or an existing
sanitizeHTML function) on newContent.innerHTML and assign the sanitized result
to contentArea.innerHTML; ensure you import/require DOMPurify or wire the
sanitizer into the module and apply it both where contentArea.innerHTML =
newContent.innerHTML and at the other innerHTML assignment spot.
In `@web/documentation_views.py`:
- Around line 82-88: The GET handlers documentation_topic_detail and
documentation_section_detail currently call
DocumentationNoteProgress.objects.get_or_create(...) and
progress.mark_section_as_viewed(section), which mutates state; remove those
lines from the GET views and instead route progress updates through the existing
POST endpoint (or call the same service function used by that POST handler).
Concretely: delete or comment out the get_or_create + mark_section_as_viewed
calls in the two GET view functions, and ensure the POST progress-update
endpoint (the function that currently handles progress writes) is used by
client-side code or internal server calls to record section views; reuse the
same helper/service used by that POST handler so no duplicate write logic
remains.
- Around line 168-169: The mark_section_viewed handler is fetching the topic by
slug without checking publication status; update its topic lookup to mirror the
read views by calling get_object_or_404(DocumentationNoteTopic, slug=topic_slug,
is_published=True) so unpublished topics are inaccessible, and keep the existing
section lookup (get_object_or_404(DocumentationNoteSection, slug=section_slug,
topic=topic)) as-is; ensure you modify the lookup inside the mark_section_viewed
function to use the is_published=True filter on DocumentationNoteTopic.
In `@web/management/commands/create_sample_docs.py`:
- Around line 427-473: Currently get_or_create is used for
DocumentationNoteTopic, DocumentationNoteSection, and DocumentationNoteContent
so rerunning the command won't refresh changed fields; replace each
get_or_create call with update_or_create (or call get() then set fields and
save) so the title, description, icon, color, order and markdown_content are
updated from PROJECTILE_MOTION_CONTENT when the record exists (use
slugify(section_data["title"]) for section lookups and keep created/updated
messaging logic intact).
In `@web/models.py`:
- Around line 3324-3341: mark_section_as_viewed currently accepts sections from
any topic; ensure the section belongs to the same topic as the progress instance
before mutating state by validating section.topic (or section.topic_id) equals
self.topic (or self.topic_id) at the start of mark_section_as_viewed; if it does
not match, either raise a ValueError (preferred) or return without change; only
then call self.sections_viewed.add(section), set self.current_section, call
update_progress() and save() so completion_percentage and completed_at stay
consistent; also consider using the existing update_progress and sections_viewed
identifiers to locate where to add the guard.
- Around line 3206-3207: Remove the duplicate indexes from the model's indexes
list: delete the explicit models.Index(fields=["slug"]) (since
SlugField(unique=True) already creates a unique index) and remove any explicit
index duplicating the unique_together (user, topic) unique constraint; retain
the models.Index(fields=["is_published"]) entry for query performance. Locate
the indexes = [...] declaration (the models.Index usage and the
unique_together/SlugField(unique=True) definitions) and update it so only
is_published remains indexed.
In `@web/templates/documentation_notes/components/breadcrumb.html`:
- Around line 2-12: The breadcrumb <nav> lacks an accessible label and the
icon-only Home link lacks an accessible name; update the <nav> element used for
breadcrumbs to include an ARIA label (e.g., aria-label="Breadcrumb" or
aria-label="Breadcrumb navigation") and ensure the home anchor (<a href="{% url
'index' %}" ...>) includes an accessible name (e.g., aria-label="Home" or a
visually hidden text label) so screen readers can identify both the navigation
region and the icon-only link.
In `@web/templates/documentation_notes/components/navigation.html`:
- Around line 45-47: Replace runtime Tailwind classes in the Next button (anchor
in navigation.html that builds the docs_section_detail link) with an explicit
mapping from topic.color values to concrete class strings (e.g., map "blue" =>
"bg-blue-500 hover:bg-blue-600 dark:bg-blue-600 dark:hover:bg-blue-700
text-white focus:outline-none focus:ring-2 focus:ring-offset-2
focus:ring-blue-400") and use that mapped class variable in the template; ensure
you add dark mode variants and focus state classes in each mapping entry; apply
the same pattern to the similar dynamic color usages in sidebar.html (e.g.,
mappings for dark:bg-.../20 and text-... classes) so Tailwind can statically
detect and preserve the classes.
In `@web/templates/documentation_notes/components/sidebar.html`:
- Around line 21-22: The mobile toggle button is targeting "#sidebar-content"
which doesn't exist; update the sidebar markup so the element being toggled has
the matching id (add id="sidebar-content" to the <nav class="space-y-1"> element
that wraps the {% for section in sections %} loop) or change the button's
data-target to the existing element id—ensure the id referenced by the toggle
and the nav element (or the container around the sections) match so the mobile
show/hide works.
- Line 40: The template is performing per-row DB membership checks via "section
in progress.sections_viewed.all" inside the sections loop; change the view that
renders this template to precompute a collection of viewed section IDs (e.g.,
viewed_section_ids = set(progress.sections_viewed.values_list('id', flat=True)))
and pass it into the template context, then update the template check to use
"section.id in viewed_section_ids" (keep existing symbols like
user.is_authenticated, section, and progress) to avoid repeated queryset
evaluation.
In `@web/templates/documentation_notes/topic_detail.html`:
- Around line 5-29: Remove the <style> block and any inline style attributes and
replace the custom CSS classes and variables (e.g., :root color variables and
classes topic-styled, topic-accent, topic-border, topic-bg) with equivalent
Tailwind utility classes; for example, convert the
gradient/foreground/background/border uses in
topic-styled/topic-accent/topic-border/topic-bg to Tailwind gradient,
text-color, border-color and bg-opacity utilities (choose the nearest
teal/teal-dark shades and rgba background using bg-opacity or bg-opacity +
bg-teal-... classes), and update all occurrences noted (lines referenced in the
comment) to use those Tailwind utilities instead of the custom classes or inline
styles.
- Around line 1-4: The template topic_detail.html currently extends "base.html"
and reimplements breadcrumb/sidebar/shell markup that already exists in the
docs-specific base; change the extends to "documentation_notes/base.html",
remove the duplicated breadcrumb/sidebar/shell markup (the repeated block around
lines 73-93) so the layout comes from the parent, and keep only the
page-specific content inside the existing block title and block content (use {{
block.super() }} or ensure blocks match the parent if you need to preserve any
parent content).
- Around line 137-142: The template inside the script tag with id
"doc-markdown-source" is emitting literal braces instead of rendering the Django
variable content.markdown_content; update the interpolation so the template
engine outputs the variable (use Django template variable output for
content.markdown_content with the correct brace syntax and no extra
spaces/newlines that make the braces literal) so the markdown is rendered rather
than shown as raw text.
- Around line 234-235: The code assigns window.marked.parse(rawMarkdown)
directly to markdownTarget.innerHTML, which is an XSS risk; update the template
to sanitize the parsed HTML before assigning it by either: (A) using a
client-side sanitizer function (e.g., integrate the project’s existing sanitizer
or a bundled DOM-sanitizer) to clean the output of
window.marked.parse(rawMarkdown) and then set markdownTarget.innerHTML to the
sanitized HTML, or (B) perform server-side sanitization with the existing bleach
usage so rawMarkdown is already safe when rendered; reference markdownTarget,
window.marked.parse, rawMarkdown, and innerHTML when making the change.
- Around line 243-252: Guard the progress POST by only running the fetch when
the user is authenticated and use the cookie-based CSRF token instead of
querying a non-existent form field: wrap or conditionalize the fetch block using
the template authentication flag (e.g. {% if user.is_authenticated %} ... {%
endif %}) so anonymous users never run the request, and replace the CSRF source
referenced by document.querySelector('[name=csrfmiddlewaretoken]') with a cookie
lookup (getCookie('csrftoken') or equivalent) and include that token in the
'X-CSRFToken' header for the fetch to the URL generated by {% url
"docs_mark_section_viewed" topic.slug current_section.slug %}.
In `@web/templates/documentation_notes/topics_list.html`:
- Around line 5-35: Remove the <style> block and any inline styles and replace
the .topic-color-header, .topic-card and the variant selectors
(.topic-card.color-blue, .topic-card.color-purple, .topic-card.color-pink,
.topic-card.color-green) with Tailwind utility classes only: pick equivalent
Tailwind background, gradient and height utilities (e.g., h-1, bg-gradient-to-r,
from-*, to-*, bg-*) and apply them conditionally in the template where the topic
card is rendered (map each variant name – color-blue, color-purple, color-pink,
color-green – to the appropriate Tailwind classes), remove custom CSS variables
(--topic-light, --topic-dark) and ensure no inline style attributes remain on
elements like the topic header or card container.
In `@web/urls.py`:
- Around line 83-96: The route for documentation_views.user_progress is being
shadowed by the dynamic slug routes (paths using "docs/<slug:topic_slug>/" and
"docs/<slug:topic_slug>/<slug:section_slug>/"); move the
path("docs/user/progress/", documentation_views.user_progress,
name="docs_user_progress") entry higher in the urlpatterns (place it before any
"docs/<slug:...>/" routes) so the literal "user/progress" URL is matched before
the slug-based patterns.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
static/ts/documentation-notes.tsweb/admin.pyweb/documentation_views.pyweb/management/commands/__init__.pyweb/management/commands/create_sample_docs.pyweb/migrations/0064_add_documentation_notes.pyweb/models.pyweb/templates/documentation_notes/base.htmlweb/templates/documentation_notes/components/breadcrumb.htmlweb/templates/documentation_notes/components/navigation.htmlweb/templates/documentation_notes/components/sidebar.htmlweb/templates/documentation_notes/topic_detail.htmlweb/templates/documentation_notes/topics_list.htmlweb/tests/test_views.pyweb/urls.py
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (12)
web/models.py (2)
3324-3326:⚠️ Potential issue | 🟠 MajorHandle zero-section topics by resetting progress state.
Early return leaves stale
completion_percentage/completed_atvalues if a topic ends up with no sections.Suggested fix
def update_progress(self): """Calculate and update completion percentage""" total_sections = self.topic.sections.count() if total_sections == 0: - return + self.completion_percentage = 0.0 + self.completed_at = None + self.save(update_fields=["completion_percentage", "completed_at", "last_accessed_at"]) + return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/models.py` around lines 3324 - 3326, When total_sections is zero the current early return leaves stale progress on the model; in the branch where total_sections == 0 (the check using self.topic.sections.count()) reset progress fields by setting completion_percentage to 0 and completed_at to None on the instance (and persist the change via save() or an update_fields call), instead of returning immediately so the topic's progress state is cleared.
3206-3206: 🧹 Nitpick | 🔵 TrivialRemove redundant indexes covered by uniqueness constraints.
SlugField(unique=True)andunique_together = ("user", "topic")already enforce indexed uniqueness; the extra explicit indexes duplicate that work.Also applies to: 3317-3317
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/models.py` at line 3206, The Meta.indexes lists include explicit indexes that duplicate uniqueness constraints — remove the redundant models.Index that targets the slug (since SlugField(unique=True) already creates a unique index) and any Index that duplicates the unique_together (e.g., the pair enforced by unique_together = ("user", "topic")); keep only non-duplicative indexes such as models.Index(fields=["is_published"]) in the class Meta.indexes definitions (apply the same removal to the second occurrence around the unique_together mention).static/ts/documentation-notes.ts (2)
187-187:⚠️ Potential issue | 🟠 MajorHarden HTML insertion paths before assigning to
innerHTML.Both fetched HTML and markdown output are inserted directly into the DOM. This is an avoidable XSS risk in the client layer.
Also applies to: 261-261
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@static/ts/documentation-notes.ts` at line 187, The assignment contentArea.innerHTML = newContent.innerHTML (seen where contentArea and newContent are used) directly injects fetched/generated HTML and is an XSS risk; replace direct innerHTML usage by sanitizing the HTML before insertion (e.g., run newContent.innerHTML through a sanitizer like DOMPurify or a project-approved allowlist sanitizer) or convert to safe DOM nodes via a DOMParser and strip unsafe tags/attributes, then insert using appendChild or textContent for untrusted strings; update both occurrences (the assignments involving contentArea and newContent at the two locations) to use the sanitizer utility so only allowed tags/attributes are injected.
62-66:⚠️ Potential issue | 🔴 CriticalFix delegated anchor handling:
currentTargetis wrong in this flow.With delegated listeners,
event.currentTargetis the document, not the anchor. The current implementation can throw when callinggetAttributeand breaks smooth scrolling.Suggested fix
- document.addEventListener('click', (e) => { - const link = (e.target as HTMLElement).closest('a[href^="#"]'); - if (link) { - this.onAnchorClick(e as MouseEvent); - } - }); + document.addEventListener('click', (e) => { + const link = (e.target as HTMLElement | null)?.closest('a[href^="#"]') as HTMLAnchorElement | null; + if (link) { + this.onAnchorClick(e, link); + } + }); @@ - private onAnchorClick(event: Event): void { - const link = event.currentTarget as HTMLAnchorElement; - const href = link.getAttribute('href'); + private onAnchorClick(event: Event, link?: HTMLAnchorElement): void { + const anchor = link ?? (event.currentTarget as HTMLAnchorElement); + const href = anchor.getAttribute('href');Also applies to: 86-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@static/ts/documentation-notes.ts` around lines 62 - 66, The delegated click handler uses event.currentTarget (document) which breaks anchor handling; locate the document.addEventListener('click', ...) block and the onAnchorClick method and change the call to pass the resolved anchor element (the result of (e.target as HTMLElement).closest('a[href^="#"]')) into onAnchorClick (e.g., this.onAnchorClick(e as MouseEvent, link as HTMLAnchorElement)), and update onAnchorClick to prefer the passed anchor parameter instead of reading event.currentTarget; apply the same change for the other delegated listener at the 86-89 region.web/templates/documentation_notes/topic_detail.html (4)
1-1: 🛠️ Refactor suggestion | 🟠 MajorExtend the docs base template instead of duplicating the docs layout shell.
This file still reimplements breadcrumb/sidebar/shell markup that should be centralized in the docs base, which increases drift risk.
Also applies to: 73-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/documentation_notes/topic_detail.html` at line 1, topic_detail.html reimplements the docs layout (breadcrumb/sidebar/shell) instead of using the centralized docs base; update the template to extend the docs base (use the docs base template name used in the project, e.g. "docs/base.html" or the canonical docs layout) and remove the duplicated markup (the breadcrumb/sidebar/shell markup blocks found in topic_detail.html and lines ~73-93), leaving only the page-specific blocks (e.g. block content, block title or any small overrides). Ensure you reference the template symbol topic_detail.html and use the docs base's block names so the page content renders inside the centralized docs layout.
241-245:⚠️ Potential issue | 🟠 MajorCurrent markdown “sanitization” is incomplete for safe
innerHTMLassignment.Replacing a few encoded tags is not sufficient to neutralize unsafe HTML/URLs emitted by markdown parsing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/documentation_notes/topic_detail.html` around lines 241 - 245, The code assigns parsed markdown (window.marked.parse(rawMarkdown)) directly to markdownTarget.innerHTML and attempts weak tag replacements; instead, sanitize the parsed HTML with a proven sanitizer before assigning: call a sanitizer (e.g., DOMPurify.sanitize(parsedHtml)) on the value returned from window.marked.parse(rawMarkdown) and assign that sanitized string to markdownTarget.innerHTML (or use markdownTarget.textContent for plain text output); update the code paths around markdownTarget, window.marked.parse, and rawMarkdown to import/configure DOMPurify (or equivalent) and replace the manual .replace(...) lines with a single sanitize step.
257-270:⚠️ Potential issue | 🟠 MajorUse reliable CSRF sourcing and robust JSON response handling for progress POST.
Token lookup may be empty on this page, and the response is parsed as JSON without checking
response.ok/content type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/documentation_notes/topic_detail.html` around lines 257 - 270, The progress POST currently grabs csrfToken only from meta or form and calls response.json() unconditionally; update the code around csrfToken and the fetch to reliably source CSRF by adding a fallback getCookie('csrftoken') lookup (use a small getCookie helper) and only send the X-CSRFToken header when a token exists, and make the response handling for the fetch to '{% url "docs_mark_section_viewed" topic.slug current_section.slug %}' robust by checking response.ok and the Content-Type header (e.g., const ct = response.headers.get('content-type') || '') before calling response.json(), and handle non-OK or non-JSON responses gracefully (skip parsing or catch JSON parse errors) instead of assuming response.json() will always succeed.
5-29:⚠️ Potential issue | 🟠 MajorRemove custom CSS and inline styles; use Tailwind utilities only.
The template still uses a
<style>block and inlinestyle=attributes.As per coding guidelines: "
**/*.{html,htm}: Never use inline styles" and "**/*.{html,htm,css}: Never use custom CSS classes".Also applies to: 111-112, 153-157, 177-178, 186-186
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/documentation_notes/topic_detail.html` around lines 5 - 29, Remove the embedded <style> block and all custom CSS classes (.topic-styled, .topic-accent, .topic-border, .topic-bg and any :root vars) and replace them in the template markup with equivalent Tailwind utility classes (e.g., gradient background, text-white, text-teal-500, border-teal-500, bg-teal-50, etc.); also drop any inline style="..." usages referenced in the review and convert them to Tailwind utilities on the same elements so the template uses only Tailwind classes and no custom CSS or inline styles.web/documentation_views.py (2)
91-100:⚠️ Potential issue | 🟠 MajorPrecompute
viewed_section_idsin context for sidebar rendering.The sidebar currently performs per-section membership checks against
progress.sections_viewed; pass a precomputed id set from the view instead.Also applies to: 140-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/documentation_views.py` around lines 91 - 100, Precompute a set of viewed section IDs in the view and pass it into the template context instead of having the template call membership on progress.sections_viewed repeatedly; e.g. build viewed_section_ids = {s.id for s in progress.sections_viewed} (or use values_list('id', flat=True) if it's a queryset) and add "viewed_section_ids": viewed_section_ids to the context alongside topic, sections, current_section, current_section_index, content, progress, next_section (section.get_next_section()), and previous_section (section.get_previous_section()); do the same replacement for the second context block around the other occurrence (lines ~140-149) so the sidebar uses viewed_section_ids for membership checks.
83-86:⚠️ Potential issue | 🟠 MajorAvoid state mutation in GET views by removing
get_or_createfrom read handlers.The two GET detail endpoints still write progress rows on page render; writes should stay in the POST progress endpoint.
Also applies to: 132-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/documentation_views.py` around lines 83 - 86, The GET detail handlers currently mutate state by calling DocumentationNoteProgress.objects.get_or_create (assigning to progress); change those calls to read-only queries (e.g., use DocumentationNoteProgress.objects.filter(user=request.user, topic=topic).first() or a try/except with DocumentationNoteProgress.objects.get) so no row is created during GET rendering, remove the get_or_create call and any code that assumes creation there, and ensure that creation/updating of DocumentationNoteProgress remains only in the POST progress endpoint (leave that logic unchanged).web/templates/documentation_notes/components/sidebar.html (1)
66-69:⚠️ Potential issue | 🟡 MinorAdd ARIA state/relationship to the mobile sections toggle.
The button toggles content visibility but does not expose
aria-controls/aria-expanded, so assistive tech cannot track state changes.As per coding guidelines: "Ensure proper HTML structure and accessibility in templates" and "Include proper ARIA labels where needed for accessibility".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/documentation_notes/components/sidebar.html` around lines 66 - 69, The toggle button that calls document.getElementById('sidebar-content').classList.toggle('hidden') must expose ARIA state and relationship so assistive tech can track it: add aria-controls="sidebar-content" on the button and add an aria-expanded attribute that is programmatically updated to "true"/"false" when the click handler toggles the 'hidden' class; also include an accessible label (e.g., aria-label or keep the visible text) and ensure the controlled element has the id "sidebar-content" (or update the id reference) so aria-controls points to the correct element.web/templates/documentation_notes/topics_list.html (1)
60-63:⚠️ Potential issue | 🟠 MajorReplace inline progress width styling with a Tailwind-compatible pattern.
The progress bar still uses inline
style="width: ...".As per coding guidelines: "
**/*.{html,htm}: Never use inline styles".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/documentation_notes/topics_list.html` around lines 60 - 63, The progress bar is using an inline style for width; replace it by mapping the computed percentage to a Tailwind-compatible class instead. Add a template filter (e.g., progress_width_class) that takes the value from user_progress|dict_key:topic.id, normalizes/rounds it to your supported buckets (e.g., 0,10,20,...,100) and returns a class name like "w-10p" or an existing Tailwind class; update the inner div in topics_list.html to remove style="width:..." and add the returned class (e.g., class="h-full transition-all duration-300 {{ user_progress|dict_key:topic.id|progress_width_class }}"), and add the corresponding CSS/Tailwind utilities (or generate w-0..w-100% utilities) so the mapping classes (progress_width_class) control width without inline styles.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@static/ts/documentation-notes.ts`:
- Around line 190-192: The renderer currently calls renderMarkdown(contentArea)
but internally scans for [data-markdown] which doesn't match the page DOM;
update the DOM query logic so renderMarkdown (or the code path that scans for
markdown) looks for the actual selectors '#doc-markdown-source' and
'.doc-markdown' inside the provided container (contentArea) and processes those
nodes after AJAX replacement; also update the other occurrence in the same file
where renderMarkdown is invoked (the block that mirrors lines ~255-263) to use
the same selector logic so newly loaded sections are rendered correctly.
- Around line 206-214: Replace the brittle input-based CSRF lookup used before
the POST fetch with cookie-based retrieval: implement or call a
getCookie('csrftoken') helper to read the CSRF token from document.cookie
(falling back to the existing
document.querySelector('input[name="csrfmiddlewaretoken"]') only if the cookie
is missing) and use that value for the 'X-CSRFToken' header in the fetch; update
the code that assigns csrfToken and the fetch call to reference the
cookie-derived value (variables: csrfToken, and the POST fetch to
`/docs/${this.config.topicSlug}/section/${sectionSlug}/viewed/`).
In `@web/documentation_views.py`:
- Line 29: Replace the prefetch-only query with an annotated count to avoid
per-card DB hits: import Count from django.db.models, change the query that sets
topics (DocumentationNoteTopic.objects.filter(...)) to include
.annotate(section_count=Count("sections")) (you can keep
.prefetch_related("sections") if you still need the related objects), and update
the template to use topic.section_count instead of calling
topic.sections.count(), so each topic row uses the annotated value rather than
triggering extra queries.
In `@web/models.py`:
- Around line 3346-3349: Remove the redundant second DB write by deleting the
trailing self.save() after setting self.sections_viewed and
self.current_section; keep the calls to self.sections_viewed.add(section),
self.current_section = section, and self.update_progress() since
update_progress() already persists the model, so do not call save() again.
In `@web/templates/documentation_notes/topic_detail.html`:
- Line 127: The template currently checks current_section.icon but renders a
hardcoded emoji; update the conditional to render the configured icon value
(current_section.icon) instead of "📚". Locate the line using the conditional
with current_section.icon in the topic detail template and replace the hardcoded
emoji with the template expression that outputs current_section.icon (ensuring
it is escaped/safe as needed for your templating engine).
- Around line 256-284: The template has unbalanced template and JS block
boundaries: the `{% if user.is_authenticated %}` is never closed and the
trailing closing tokens around the fetch promise chain are mismatched. Fix by
closing the Django if block with `{% endif %}` after the fetch/.catch(...)
promise chain and correct the JS punctuation so the fetch call's
parentheses/braces are balanced (ensure the promise chain ends with `});` for
the fetch invocation and then place `{% endif %}` before the closing
`</script>`). Reference symbols: `user.is_authenticated`, the fetch to `{% url
"docs_mark_section_viewed" topic.slug current_section.slug %}`, and the
`progressBar` handling.
In `@web/templates/documentation_notes/topics_list.html`:
- Around line 50-53: The current conditional uses truthiness on
user_progress|dict_key:topic.id which treats 0 as false and hides valid 0%
progress; change the check to explicitly test for presence (not None) so zero is
allowed — e.g. replace occurrences like "{% if user.is_authenticated and
user_progress|dict_key:topic.id %}" with "{% if user.is_authenticated and
user_progress|dict_key:topic.id is not None %}", and make the same change at the
other occurrence around lines 59-60; reference symbols:
user_progress|dict_key:topic.id and topic.id.
---
Duplicate comments:
In `@static/ts/documentation-notes.ts`:
- Line 187: The assignment contentArea.innerHTML = newContent.innerHTML (seen
where contentArea and newContent are used) directly injects fetched/generated
HTML and is an XSS risk; replace direct innerHTML usage by sanitizing the HTML
before insertion (e.g., run newContent.innerHTML through a sanitizer like
DOMPurify or a project-approved allowlist sanitizer) or convert to safe DOM
nodes via a DOMParser and strip unsafe tags/attributes, then insert using
appendChild or textContent for untrusted strings; update both occurrences (the
assignments involving contentArea and newContent at the two locations) to use
the sanitizer utility so only allowed tags/attributes are injected.
- Around line 62-66: The delegated click handler uses event.currentTarget
(document) which breaks anchor handling; locate the
document.addEventListener('click', ...) block and the onAnchorClick method and
change the call to pass the resolved anchor element (the result of (e.target as
HTMLElement).closest('a[href^="#"]')) into onAnchorClick (e.g.,
this.onAnchorClick(e as MouseEvent, link as HTMLAnchorElement)), and update
onAnchorClick to prefer the passed anchor parameter instead of reading
event.currentTarget; apply the same change for the other delegated listener at
the 86-89 region.
In `@web/documentation_views.py`:
- Around line 91-100: Precompute a set of viewed section IDs in the view and
pass it into the template context instead of having the template call membership
on progress.sections_viewed repeatedly; e.g. build viewed_section_ids = {s.id
for s in progress.sections_viewed} (or use values_list('id', flat=True) if it's
a queryset) and add "viewed_section_ids": viewed_section_ids to the context
alongside topic, sections, current_section, current_section_index, content,
progress, next_section (section.get_next_section()), and previous_section
(section.get_previous_section()); do the same replacement for the second context
block around the other occurrence (lines ~140-149) so the sidebar uses
viewed_section_ids for membership checks.
- Around line 83-86: The GET detail handlers currently mutate state by calling
DocumentationNoteProgress.objects.get_or_create (assigning to progress); change
those calls to read-only queries (e.g., use
DocumentationNoteProgress.objects.filter(user=request.user, topic=topic).first()
or a try/except with DocumentationNoteProgress.objects.get) so no row is created
during GET rendering, remove the get_or_create call and any code that assumes
creation there, and ensure that creation/updating of DocumentationNoteProgress
remains only in the POST progress endpoint (leave that logic unchanged).
In `@web/models.py`:
- Around line 3324-3326: When total_sections is zero the current early return
leaves stale progress on the model; in the branch where total_sections == 0 (the
check using self.topic.sections.count()) reset progress fields by setting
completion_percentage to 0 and completed_at to None on the instance (and persist
the change via save() or an update_fields call), instead of returning
immediately so the topic's progress state is cleared.
- Line 3206: The Meta.indexes lists include explicit indexes that duplicate
uniqueness constraints — remove the redundant models.Index that targets the slug
(since SlugField(unique=True) already creates a unique index) and any Index that
duplicates the unique_together (e.g., the pair enforced by unique_together =
("user", "topic")); keep only non-duplicative indexes such as
models.Index(fields=["is_published"]) in the class Meta.indexes definitions
(apply the same removal to the second occurrence around the unique_together
mention).
In `@web/templates/documentation_notes/components/sidebar.html`:
- Around line 66-69: The toggle button that calls
document.getElementById('sidebar-content').classList.toggle('hidden') must
expose ARIA state and relationship so assistive tech can track it: add
aria-controls="sidebar-content" on the button and add an aria-expanded attribute
that is programmatically updated to "true"/"false" when the click handler
toggles the 'hidden' class; also include an accessible label (e.g., aria-label
or keep the visible text) and ensure the controlled element has the id
"sidebar-content" (or update the id reference) so aria-controls points to the
correct element.
In `@web/templates/documentation_notes/topic_detail.html`:
- Line 1: topic_detail.html reimplements the docs layout
(breadcrumb/sidebar/shell) instead of using the centralized docs base; update
the template to extend the docs base (use the docs base template name used in
the project, e.g. "docs/base.html" or the canonical docs layout) and remove the
duplicated markup (the breadcrumb/sidebar/shell markup blocks found in
topic_detail.html and lines ~73-93), leaving only the page-specific blocks (e.g.
block content, block title or any small overrides). Ensure you reference the
template symbol topic_detail.html and use the docs base's block names so the
page content renders inside the centralized docs layout.
- Around line 241-245: The code assigns parsed markdown
(window.marked.parse(rawMarkdown)) directly to markdownTarget.innerHTML and
attempts weak tag replacements; instead, sanitize the parsed HTML with a proven
sanitizer before assigning: call a sanitizer (e.g.,
DOMPurify.sanitize(parsedHtml)) on the value returned from
window.marked.parse(rawMarkdown) and assign that sanitized string to
markdownTarget.innerHTML (or use markdownTarget.textContent for plain text
output); update the code paths around markdownTarget, window.marked.parse, and
rawMarkdown to import/configure DOMPurify (or equivalent) and replace the manual
.replace(...) lines with a single sanitize step.
- Around line 257-270: The progress POST currently grabs csrfToken only from
meta or form and calls response.json() unconditionally; update the code around
csrfToken and the fetch to reliably source CSRF by adding a fallback
getCookie('csrftoken') lookup (use a small getCookie helper) and only send the
X-CSRFToken header when a token exists, and make the response handling for the
fetch to '{% url "docs_mark_section_viewed" topic.slug current_section.slug %}'
robust by checking response.ok and the Content-Type header (e.g., const ct =
response.headers.get('content-type') || '') before calling response.json(), and
handle non-OK or non-JSON responses gracefully (skip parsing or catch JSON parse
errors) instead of assuming response.json() will always succeed.
- Around line 5-29: Remove the embedded <style> block and all custom CSS classes
(.topic-styled, .topic-accent, .topic-border, .topic-bg and any :root vars) and
replace them in the template markup with equivalent Tailwind utility classes
(e.g., gradient background, text-white, text-teal-500, border-teal-500,
bg-teal-50, etc.); also drop any inline style="..." usages referenced in the
review and convert them to Tailwind utilities on the same elements so the
template uses only Tailwind classes and no custom CSS or inline styles.
In `@web/templates/documentation_notes/topics_list.html`:
- Around line 60-63: The progress bar is using an inline style for width;
replace it by mapping the computed percentage to a Tailwind-compatible class
instead. Add a template filter (e.g., progress_width_class) that takes the value
from user_progress|dict_key:topic.id, normalizes/rounds it to your supported
buckets (e.g., 0,10,20,...,100) and returns a class name like "w-10p" or an
existing Tailwind class; update the inner div in topics_list.html to remove
style="width:..." and add the returned class (e.g., class="h-full transition-all
duration-300 {{ user_progress|dict_key:topic.id|progress_width_class }}"), and
add the corresponding CSS/Tailwind utilities (or generate w-0..w-100% utilities)
so the mapping classes (progress_width_class) control width without inline
styles.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
static/ts/documentation-notes.tsweb/documentation_views.pyweb/models.pyweb/templates/documentation_notes/components/breadcrumb.htmlweb/templates/documentation_notes/components/sidebar.htmlweb/templates/documentation_notes/topic_detail.htmlweb/templates/documentation_notes/topics_list.htmlweb/urls.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (11)
static/ts/documentation-notes.ts (3)
206-213:⚠️ Potential issue | 🟠 MajorCSRF retrieval should use the
csrftokencookie first.Lines 207-210 depend on optional DOM nodes and can skip progress POSTs when those nodes are absent.
Suggested fix
+ const getCookie = (name: string): string => { + const cookie = document.cookie + .split('; ') + .find((row) => row.startsWith(`${name}=`)); + return cookie ? decodeURIComponent(cookie.split('=')[1]) : ''; + }; + - let csrfToken = document.querySelector('meta[name="csrf-token"]')?.getAttribute('content'); + let csrfToken = getCookie('csrftoken'); if (!csrfToken) { + csrfToken = document.querySelector('meta[name="csrf-token"]')?.getAttribute('content') || ''; + } + if (!csrfToken) { const csrfInput = document.querySelector('input[name="csrfmiddlewaretoken"]') as HTMLInputElement; csrfToken = csrfInput?.value; }#!/bin/bash # Check whether docs templates expose csrf meta/input fallbacks. rg -n "csrf-token|csrfmiddlewaretoken" web/templates/documentation_notes --glob '*.html'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@static/ts/documentation-notes.ts` around lines 206 - 213, The CSRF lookup currently only checks a meta tag and form input (variable csrfToken via document.querySelector), which can be absent and cause POSTs to be skipped; modify the retrieval so it first reads the "csrftoken" cookie, then falls back to the meta tag ('meta[name="csrf-token"]') and finally to the input ('input[name="csrfmiddlewaretoken"]') before bailing, ensuring any code that uses csrfToken continues to work when templates omit the meta/input.
62-66:⚠️ Potential issue | 🟠 MajorDelegated anchor handling is using the wrong target object.
Line 87 reads
event.currentTarget, but with delegation from Line 62 that value isdocument, not the clicked anchor.Suggested fix
- document.addEventListener('click', (e) => { - const link = (e.target as HTMLElement).closest('a[href^="#"]'); - if (link) { - this.onAnchorClick(e as MouseEvent); - } - }); + document.addEventListener('click', (e) => { + const link = (e.target as HTMLElement | null)?.closest('a[href^="#"]') as HTMLAnchorElement | null; + if (link) { + this.onAnchorClick(e as MouseEvent, link); + } + }); @@ - private onAnchorClick(event: Event): void { - const link = event.currentTarget as HTMLAnchorElement; - const href = link.getAttribute('href'); + private onAnchorClick(event: Event, link: HTMLAnchorElement): void { + const href = link.getAttribute('href');#!/bin/bash # Verify delegated click binding and onAnchorClick target usage. sed -n '60,95p' static/ts/documentation-notes.tsAlso applies to: 86-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@static/ts/documentation-notes.ts` around lines 62 - 66, Delegated click handler uses document as currentTarget, so update the delegation to pass the clicked anchor to onAnchorClick instead of relying on event.currentTarget: inside the document.addEventListener callback, capture the anchor (const link = (e.target as HTMLElement).closest('a[href^="#"]') as HTMLAnchorElement) and call this.onAnchorClick(e as MouseEvent, link) or this.onAnchorClick.call(this, e as MouseEvent, link); then update the onAnchorClick method signature (onAnchorClick(event: MouseEvent, anchor?: HTMLAnchorElement)) to use the provided anchor (or fallback to event.target) when computing the target element instead of using event.currentTarget.
271-273:⚠️ Potential issue | 🔴 CriticalMarkdown output is injected without real sanitization.
Replacing encoded
<scriptstrings does not sanitize actual HTML returned bymarked.parse(...)beforeinnerHTML.Suggested fix
+import DOMPurify from 'dompurify'; @@ - (el as any).innerHTML = (window as any).marked.parse(content).replace(/<script/gi, '&lt;script').replace(/<iframe/gi, '&lt;iframe').replace(/<object/gi, '&lt;object'); + const rendered = (window as any).marked.parse(content); + (el as HTMLElement).innerHTML = DOMPurify.sanitize(rendered);#!/bin/bash # Verify markdown parsing output is directly assigned to innerHTML. rg -n "marked\\.parse|innerHTML|replace\\(/<script" static/ts/documentation-notes.ts -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@static/ts/documentation-notes.ts` around lines 271 - 273, The code assigns marked.parse(content) directly into (el as any).innerHTML after only replacing encoded tag prefixes, which doesn't sanitize real HTML; wrap the parsed HTML with a proper sanitizer before assignment (e.g., import and use DOMPurify.sanitize(parsed) or an equivalent HTML sanitizer) and assign the sanitized string to innerHTML instead of the raw marked output; reference the code around marked.parse(content) and the (el as any).innerHTML assignment, and ensure you configure the sanitizer (allowed tags/attributes or remove scripts/iframes) and fall back to setting textContent when sanitization isn't available.web/templates/documentation_notes/topic_detail.html (5)
249-253:⚠️ Potential issue | 🔴 Critical
markedoutput is injected without robust sanitization.The replace chain only targets encoded substrings and does not safely sanitize full parsed HTML before
innerHTML.Suggested fix
+ <script src="https://cdn.jsdelivr.net/npm/dompurify@3.2.6/dist/purify.min.js"></script> @@ - markdownTarget.innerHTML = window.marked - .parse(rawMarkdown) - .replace(/<script/gi, '&lt;script') - .replace(/<iframe/gi, '&lt;iframe') - .replace(/<object/gi, '&lt;object'); + const rendered = window.marked.parse(rawMarkdown) + markdownTarget.innerHTML = DOMPurify.sanitize(rendered)#!/bin/bash # Verify markdown parse result is assigned directly to innerHTML. rg -n "marked\\.parse|innerHTML|replace\\(/<script" web/templates/documentation_notes/topic_detail.html -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/documentation_notes/topic_detail.html` around lines 249 - 253, The code assigns window.marked.parse(rawMarkdown) into markdownTarget.innerHTML with only minimal replace calls, which is unsafe; update the rendering in the handler that calls window.marked.parse(rawMarkdown) and sets markdownTarget.innerHTML to instead sanitize the parsed HTML (e.g., pass the parse result through a sanitizer like DOMPurify or use a safe renderer) before assigning to markdownTarget.innerHTML, ensuring all uses of window.marked.parse, rawMarkdown, and markdownTarget.innerHTML are updated so the parsed HTML cannot inject scripts/iframes/objects.
5-29:⚠️ Potential issue | 🟠 MajorRemove custom CSS and inline styles; keep Tailwind utility classes only.
This block reintroduces
<style>-defined custom classes and multiple inlinestyle=attributes.As per coding guidelines: "
**/*.{html,htm}: Always use Tailwind CSS classes for styling HTML elements", "**/*.{html,htm}: Never use inline styles", and "**/*.{html,htm,css}: Never use custom CSS classes".Also applies to: 111-112, 159-163, 183-184, 192-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/documentation_notes/topic_detail.html` around lines 5 - 29, Remove the entire <style> block (including :root variables and the CSS classes .topic-styled, .topic-accent, .topic-border, .topic-bg) and replace usages of those classes in the template with equivalent Tailwind utility classes (e.g., gradient, text, border, and bg utilities) applied directly on the elements that reference .topic-styled, .topic-accent, .topic-border, and .topic-bg; ensure no inline style= attributes remain and update all occurrences mentioned (lines referenced in the review) so styling comes solely from Tailwind classes.
137-143:⚠️ Potential issue | 🔴 CriticalFix malformed markdown template interpolation.
Lines 137-143 render literal braces instead of
content.markdown_content, so markdown source is not output correctly.Suggested fix
- <script id="doc-markdown-source" type="text/plain"> - { - { - content.markdown_content - } - } - </script> + <script id="doc-markdown-source" type="text/plain">{{ content.markdown_content }}</script>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/documentation_notes/topic_detail.html` around lines 137 - 143, The template currently outputs literal braces instead of the variable; update the script block with correct Django template interpolation so the markdown is rendered from content.markdown_content (the script element with id "doc-markdown-source"). Replace the malformed "{{ { content.markdown_content } }}" with the proper "{{ content.markdown_content }}" and, if the markdown must be included unescaped, append the |safe filter (e.g., content.markdown_content|safe).
268-276:⚠️ Potential issue | 🟠 MajorUse cookie-derived CSRF token for the POST request.
Lines 268-269 rely on meta/input lookups that are often missing in this template path.
Suggested fix
- const csrfToken = document.querySelector('meta[name="csrf-token"]')?.content || - document.querySelector('[name=csrfmiddlewaretoken]')?.value || ''; + function getCookie(name) { + const value = `; ${document.cookie}`; + const parts = value.split(`; ${name}=`); + return parts.length === 2 ? parts.pop().split(';').shift() : ''; + } + const csrfToken = getCookie('csrftoken') || + document.querySelector('meta[name="csrf-token"]')?.content || + document.querySelector('[name=csrfmiddlewaretoken]')?.value || '';#!/bin/bash # Verify CSRF token sources referenced in this template. rg -n "csrf-token|csrfmiddlewaretoken|docs_mark_section_viewed" web/templates/documentation_notes/topic_detail.html -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/documentation_notes/topic_detail.html` around lines 268 - 276, The current CSRF lookup for csrfToken (variable csrfToken) only checks meta and input elements which may be absent; update the fetch request preparation (the fetch to {% url "docs_mark_section_viewed" topic.slug current_section.slug %}) to also derive the CSRF token from the cookie (the standard Django "csrftoken" cookie) as a fallback: implement a small cookie-parsing fallback that reads document.cookie for "csrftoken" and uses that value when meta/input lookups return empty so the 'X-CSRFToken' header always has a token.
263-267:⚠️ Potential issue | 🔴 CriticalThe auth conditional is invalid template syntax and breaks JavaScript parsing.
Lines 263-267 and 294-296 emit
%tokens as literal JS, which can prevent the whole handler from running.Suggested fix
- { - % - if user.is_authenticated % - } + {% if user.is_authenticated %} @@ - } { - % endif % - } + {% endif %}#!/bin/bash # Inspect the broken auth-guard block in script. sed -n '260,299p' web/templates/documentation_notes/topic_detail.htmlAlso applies to: 294-296
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/documentation_notes/topic_detail.html` around lines 263 - 267, The template contains malformed Jinja tags that emit literal "%" into the JavaScript (the auth-guard block using "if user.is_authenticated" currently has stray braces/percent tokens); fix the block by replacing the invalid tokens with proper Jinja control tags so it reads {% if user.is_authenticated %} ... {% endif %} (no extra braces or standalone "%" characters) around the JS progress-tracking code that references the auth conditional, and ensure the same fix is applied to the second occurrence of the auth block so no "%" tokens are output into the script.web/models.py (1)
3324-3326:⚠️ Potential issue | 🟠 MajorReset persisted progress when a topic has zero sections.
When
total_sections == 0, Line 3326 returns immediately and leaves any previouscompletion_percentage/completed_atvalues unchanged.Suggested fix
def update_progress(self): """Calculate and update completion percentage""" total_sections = self.topic.sections.count() if total_sections == 0: - return + self.completion_percentage = 0.0 + self.completed_at = None + self.save(update_fields=["completion_percentage", "completed_at", "last_accessed_at"]) + return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/models.py` around lines 3324 - 3326, When total_sections computed by self.topic.sections.count() is zero you must reset persisted progress instead of returning early: set the model fields completion_percentage = 0 and completed_at = None (or null), persist the change (e.g. self.save() or an update with update_fields=['completion_percentage','completed_at']), then return; use the existing total_sections check, the completion_percentage and completed_at fields, and the enclosing method where self.topic.sections.count() is used to locate where to apply this change.web/documentation_views.py (1)
85-88:⚠️ Potential issue | 🟠 MajorGET detail views still write progress rows via
get_or_create().Lines 85-88 and 134-137 mutate state during read handlers; use read-only lookup in GET and keep writes in
mark_section_viewed.Suggested fix
- progress = None - if request.user.is_authenticated: - progress, _ = DocumentationNoteProgress.objects.get_or_create( - user=request.user, - topic=topic, - ) + progress = None + if request.user.is_authenticated: + progress = DocumentationNoteProgress.objects.filter( + user=request.user, + topic=topic, + ).first()Also applies to: 134-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/documentation_views.py` around lines 85 - 88, The GET handlers currently call DocumentationNoteProgress.objects.get_or_create(...) (see usage of DocumentationNoteProgress and get_or_create) which mutates DB; change these to read-only lookups (e.g., DocumentationNoteProgress.objects.filter(user=request.user, topic=topic).first() or try/except DocumentationNoteProgress.DoesNotExist) so the GET detail views do not create rows, and ensure any creation/updating of progress remains only inside the existing mark_section_viewed function (call that from POST/side-effect endpoints if needed).web/templates/documentation_notes/topics_list.html (1)
65-65:⚠️ Potential issue | 🟡 MinorAvoid inline width styling for progress fill.
Line 65 uses inline
style, which violates the template styling constraints.As per coding guidelines: "
**/*.{html,htm}: Never use inline styles" and "**/*.{html,htm,css}: Never use custom CSS classes".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/documentation_notes/topics_list.html` at line 65, Replace the inline style on the progress-fill div (the expression user_progress|dict_key:topic.id|floatformat:0 used to set width) by emitting the progress value as a data attribute (e.g. data-progress="{{ user_progress|dict_key:topic.id|floatformat:0 }}") and move the width logic into the stylesheet using an attribute selector (e.g. targeting the existing progress-fill element via [data-progress] and setting width via content/var or attr() in the CSS); update the template to remove the style attribute and set data-progress instead and update the CSS to compute the fill width from that attribute so there are no inline styles or new custom classes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/documentation_views.py`:
- Line 80: The view currently uses get_object_or_404 to fetch
DocumentationNoteContent (the line setting content =
get_object_or_404(DocumentationNoteContent, section=section)), which causes a
404 when a section intentionally has no content; change this to fetch the
content non-exceptionally (e.g., use
DocumentationNoteContent.objects.filter(section=section).first() or try to get()
and catch DocumentationNoteContent.DoesNotExist to set content = None) so the
detail view can render an empty section; apply the same change to the other
occurrence around the code that currently uses get_object_or_404 for
DocumentationNoteContent (the second instance referenced at lines ~129).
In `@web/templates/documentation_notes/topics_list.html`:
- Line 51: Template currently calls topic.sections.count which triggers
per-topic DB queries; replace that with the annotated field topic.section_count
(the annotation added in the view) so the template reads topic.section_count and
uses the precomputed value from the queryset instead of invoking
topic.sections.count. Ensure any pluralization or display formatting still works
when switching to topic.section_count.
---
Duplicate comments:
In `@static/ts/documentation-notes.ts`:
- Around line 206-213: The CSRF lookup currently only checks a meta tag and form
input (variable csrfToken via document.querySelector), which can be absent and
cause POSTs to be skipped; modify the retrieval so it first reads the
"csrftoken" cookie, then falls back to the meta tag ('meta[name="csrf-token"]')
and finally to the input ('input[name="csrfmiddlewaretoken"]') before bailing,
ensuring any code that uses csrfToken continues to work when templates omit the
meta/input.
- Around line 62-66: Delegated click handler uses document as currentTarget, so
update the delegation to pass the clicked anchor to onAnchorClick instead of
relying on event.currentTarget: inside the document.addEventListener callback,
capture the anchor (const link = (e.target as
HTMLElement).closest('a[href^="#"]') as HTMLAnchorElement) and call
this.onAnchorClick(e as MouseEvent, link) or this.onAnchorClick.call(this, e as
MouseEvent, link); then update the onAnchorClick method signature
(onAnchorClick(event: MouseEvent, anchor?: HTMLAnchorElement)) to use the
provided anchor (or fallback to event.target) when computing the target element
instead of using event.currentTarget.
- Around line 271-273: The code assigns marked.parse(content) directly into (el
as any).innerHTML after only replacing encoded tag prefixes, which doesn't
sanitize real HTML; wrap the parsed HTML with a proper sanitizer before
assignment (e.g., import and use DOMPurify.sanitize(parsed) or an equivalent
HTML sanitizer) and assign the sanitized string to innerHTML instead of the raw
marked output; reference the code around marked.parse(content) and the (el as
any).innerHTML assignment, and ensure you configure the sanitizer (allowed
tags/attributes or remove scripts/iframes) and fall back to setting textContent
when sanitization isn't available.
In `@web/documentation_views.py`:
- Around line 85-88: The GET handlers currently call
DocumentationNoteProgress.objects.get_or_create(...) (see usage of
DocumentationNoteProgress and get_or_create) which mutates DB; change these to
read-only lookups (e.g.,
DocumentationNoteProgress.objects.filter(user=request.user, topic=topic).first()
or try/except DocumentationNoteProgress.DoesNotExist) so the GET detail views do
not create rows, and ensure any creation/updating of progress remains only
inside the existing mark_section_viewed function (call that from
POST/side-effect endpoints if needed).
In `@web/models.py`:
- Around line 3324-3326: When total_sections computed by
self.topic.sections.count() is zero you must reset persisted progress instead of
returning early: set the model fields completion_percentage = 0 and completed_at
= None (or null), persist the change (e.g. self.save() or an update with
update_fields=['completion_percentage','completed_at']), then return; use the
existing total_sections check, the completion_percentage and completed_at
fields, and the enclosing method where self.topic.sections.count() is used to
locate where to apply this change.
In `@web/templates/documentation_notes/topic_detail.html`:
- Around line 249-253: The code assigns window.marked.parse(rawMarkdown) into
markdownTarget.innerHTML with only minimal replace calls, which is unsafe;
update the rendering in the handler that calls window.marked.parse(rawMarkdown)
and sets markdownTarget.innerHTML to instead sanitize the parsed HTML (e.g.,
pass the parse result through a sanitizer like DOMPurify or use a safe renderer)
before assigning to markdownTarget.innerHTML, ensuring all uses of
window.marked.parse, rawMarkdown, and markdownTarget.innerHTML are updated so
the parsed HTML cannot inject scripts/iframes/objects.
- Around line 5-29: Remove the entire <style> block (including :root variables
and the CSS classes .topic-styled, .topic-accent, .topic-border, .topic-bg) and
replace usages of those classes in the template with equivalent Tailwind utility
classes (e.g., gradient, text, border, and bg utilities) applied directly on the
elements that reference .topic-styled, .topic-accent, .topic-border, and
.topic-bg; ensure no inline style= attributes remain and update all occurrences
mentioned (lines referenced in the review) so styling comes solely from Tailwind
classes.
- Around line 137-143: The template currently outputs literal braces instead of
the variable; update the script block with correct Django template interpolation
so the markdown is rendered from content.markdown_content (the script element
with id "doc-markdown-source"). Replace the malformed "{{ {
content.markdown_content } }}" with the proper "{{ content.markdown_content }}"
and, if the markdown must be included unescaped, append the |safe filter (e.g.,
content.markdown_content|safe).
- Around line 268-276: The current CSRF lookup for csrfToken (variable
csrfToken) only checks meta and input elements which may be absent; update the
fetch request preparation (the fetch to {% url "docs_mark_section_viewed"
topic.slug current_section.slug %}) to also derive the CSRF token from the
cookie (the standard Django "csrftoken" cookie) as a fallback: implement a small
cookie-parsing fallback that reads document.cookie for "csrftoken" and uses that
value when meta/input lookups return empty so the 'X-CSRFToken' header always
has a token.
- Around line 263-267: The template contains malformed Jinja tags that emit
literal "%" into the JavaScript (the auth-guard block using "if
user.is_authenticated" currently has stray braces/percent tokens); fix the block
by replacing the invalid tokens with proper Jinja control tags so it reads {% if
user.is_authenticated %} ... {% endif %} (no extra braces or standalone "%"
characters) around the JS progress-tracking code that references the auth
conditional, and ensure the same fix is applied to the second occurrence of the
auth block so no "%" tokens are output into the script.
In `@web/templates/documentation_notes/topics_list.html`:
- Line 65: Replace the inline style on the progress-fill div (the expression
user_progress|dict_key:topic.id|floatformat:0 used to set width) by emitting the
progress value as a data attribute (e.g. data-progress="{{
user_progress|dict_key:topic.id|floatformat:0 }}") and move the width logic into
the stylesheet using an attribute selector (e.g. targeting the existing
progress-fill element via [data-progress] and setting width via content/var or
attr() in the CSS); update the template to remove the style attribute and set
data-progress instead and update the CSS to compute the fill width from that
attribute so there are no inline styles or new custom classes.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
static/ts/documentation-notes.tsweb/documentation_views.pyweb/models.pyweb/templates/documentation_notes/components/breadcrumb.htmlweb/templates/documentation_notes/topic_detail.htmlweb/templates/documentation_notes/topics_list.html
github-actions
bot
dismissed
their stale review
All review conversations have been resolved.
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.
Description
Added a comprehensive documentation notes system with the following features:
New Features
DocumentationNoteTopic,DocumentationNoteSection,DocumentationNoteContent, andDocumentationNoteProgressmodels for managing hierarchical documentationdocumentation-notes.tsfor client-side functionality including progress tracking and interactive featurescreate_sample_docscommand for generating sample documentation dataImages
Files modified/created:
web/models.py- Added DocumentationNote modelsweb/documentation_views.py- New API viewsweb/urls.py- Added documentation routesweb/admin.py- Registered models in adminweb/migrations/0064_add_documentation_notes.py- Database schemaweb/management/commands/create_sample_docs.py- Sample data generationstatic/ts/documentation-notes.ts- Frontend functionalityweb/templates/documentation_notes/- UI templatesweb/tests/test_views.py- Fixed flaky testChecklist
Summary by CodeRabbit