Conversation
📝 WalkthroughWalkthroughAdds an MkDocs documentation site (config, pages, slideshow JS/CSS, assets), a GitHub Actions workflow to build and deploy the site on pushes to Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant GH as GitHub Actions
participant Runner as Ubuntu Runner
participant UV as uv / MkDocs
participant GH_Pages as GitHub Pages
Dev->>GH: push to `main` (trigger)
GH->>Runner: start "Deploy MkDocs" job
Runner->>Runner: checkout repo, setup Python 3.12
Runner->>UV: run uv to build docs and gh-deploy
UV->>GH_Pages: publish site (gh-pages)
GH_Pages-->>Dev: site updated
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Summary of ChangesHello @mhumzaarain, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the project's documentation by integrating a new MkDocs-based system. It provides detailed guides for various user roles, outlines the system's architecture, and establishes clear contribution guidelines. This change aims to improve clarity, onboarding, and overall understanding of the RADIS platform for all stakeholders. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Pull Request Review: Documentation of RADISThank you for adding comprehensive MkDocs documentation to RADIS! This is a valuable addition that will significantly improve the project's usability and onboarding experience. Here's my detailed review: ✅ Strengths1. Comprehensive Documentation Structure
2. Quality Content
3. Proper Dependency Management
🔍 Issues & Recommendations1. GitHub Actions Workflow - Security ConcernFile: on:
push:
branches:
- main
- docs_radis # ⚠️ ISSUEIssue: The workflow triggers on the Recommendation: Remove on:
push:
branches:
- main2. Orphaned Documentation FilesFiles: Issue: These files exist in the Recommendation: Either:
3. Duplicate Screenshot FilesLocation: Issue: The directory contains both old screenshots ( Recommendation: Clean up old screenshot files to avoid confusion: rm resources/screenshots/Screenshot[1-4].png4. Documentation Quality Issuesa) Custom HTML/CSS/JS in Markdown (index.md:3-143)Issue: The slideshow implementation uses inline
Recommendation: Consider using MkDocs Material's built-in features or a plugin like mkdocs-glightbox for image galleries. b) Inconsistent Terminology (admin-guide.md:164)File: 3. **Add Token**:
- Click **Add Token**
- **Owner**: Select the user who will own the token
- **Token hashed**: Enter the hashed token value # ⚠️ ConfusingIssue: Asking admins to "Enter the hashed token value" is unclear. Do they enter a raw token that gets hashed, or a pre-hashed value? Recommendation: Clarify the token creation process. If RADIS auto-generates tokens, update the documentation accordingly. c) Incomplete Information (user-guide.md:117)File: ## RADIS Client
RADIS Client is a Python library that provides programmatic access to RADIS features without using the web interface.
To use the RADIS Client, you must have an API token provided by an administrator. For instructions on generating an API token, refer to the [Admin Guide](admin-guide.md).Issue: The section mentions the RADIS Client but provides no usage examples or installation instructions. Recommendation: Add basic usage information or reference the existing ### Installation
```bash
pip install radis-clientQuick Examplefrom radis_client import RadisClient
client = RadisClient(
server_url="https://radis.example.com",
auth_token="your-token"
)
# Search reports
reports = client.search_reports(query="pneumonia")For more examples, see our Developer Guide. Issue: While Recommendation: Update to use the official GitHub Pages deployment action: permissions:
contents: read
pages: write
id-token: write
jobs:
deploy:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: actions/setup-python@v5
with:
python-version: 3.x
cache: 'pip'
- run: pip install mkdocs-material mkdocs-minify-plugin
- run: mkdocs build
- uses: actions/upload-pages-artifact@v3
with:
path: site/
- uses: actions/deploy-pages@v46. Navigation StructureFile: Recommendation: Consider a more descriptive navigation structure: nav:
- Home: index.md
- User Documentation:
- 'User Guide': user-docs/user-guide.md
- 'Admin Guide': user-docs/admin-guide.md
- Developer Documentation:
- 'Contributing': dev-docs/contributing.md
- 'Architecture': dev-docs/architecture.mdThis improves readability and follows MkDocs best practices. 🔒 Security Considerations
📊 Test CoverageObservation: This PR adds documentation only, no code changes requiring tests. However, consider:
Example CI addition: - name: Build documentation
run: |
pip install mkdocs-material mkdocs-minify-plugin
mkdocs build --strict🎯 Performance Considerations
✨ SummaryOverall Assessment: This is a high-quality documentation addition that will greatly benefit the project. The content is well-written, comprehensive, and professionally presented. Required Changes:
Recommended Changes:
Approval Status: Great work on this comprehensive documentation effort! 🎉 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive documentation for the RADIS project using MkDocs, which is an excellent addition. The new documentation covers architecture, contribution guidelines, and user/admin guides. My review includes suggestions to improve the clarity, consistency, and maintainability of these new documentation files, such as refining markdown formatting, separating CSS/JS from content, and clarifying instructions for better user experience.
| @@ -0,0 +1,89 @@ | |||
| # **RADIS Architecture Documentation** | |||
There was a problem hiding this comment.
The use of both a heading (#) and bold markdown (**...**) is redundant. For a top-level heading, using just # is sufficient and cleaner.
| # **RADIS Architecture Documentation** | |
| # RADIS Architecture Documentation |
|
|
||
| ## Backend Architecture | ||
|
|
||
| **Django Web/API Server**: Central coordination engine providing REST API endpoints, authentication, user/session management, static assets, and task orchestration. Creates job/task records in PostgreSQL and schedules background work. |
There was a problem hiding this comment.
For better document structure and consistency, it's recommended to use markdown subheadings (###) instead of just bolding the text for section titles. This will also ensure they appear in the table of contents. This applies to other similar headings in this file (e.g., PostgreSQL Database, Background Workers, etc.).
| **Django Web/API Server**: Central coordination engine providing REST API endpoints, authentication, user/session management, static assets, and task orchestration. Creates job/task records in PostgreSQL and schedules background work. | |
| ### Django Web/API Server |
| Code Style | ||
| We adhere to the Google Python Style [Guide](https://google.github.io/styleguide/pyguide.html). |
There was a problem hiding this comment.
This section title should be a markdown heading for consistency. This improves the document's structure and readability.
| Code Style | |
| We adhere to the Google Python Style [Guide](https://google.github.io/styleguide/pyguide.html). | |
| ## Code Style | |
| We adhere to the Google Python Style [Guide](https://google.github.io/styleguide/pyguide.html). |
| This repository includes a [Dev Container](https://code.visualstudio.com/docs/devcontainers/create-dev-container). | ||
| If you open the project in VS Code after cloning, you should see a prompt: | ||
|
|
||
| “Reopen in Dev Container” |
There was a problem hiding this comment.
For UI elements or text that the user needs to type or see, it's better to use backticks (`...`) for code formatting instead of typographic quotes (“...“). This makes it clearer that it's a literal string from the UI.
| “Reopen in Dev Container” | |
| `Reopen in Dev Container` |
| <style> | ||
| .slideshow-container { | ||
| position: relative; | ||
| max-width: 100%; | ||
| margin: 2rem auto; | ||
| overflow: hidden; | ||
| border-radius: 8px; | ||
| box-shadow: 0 6px 20px rgba(0,0,0,0.15); | ||
| } | ||
|
|
||
| .slides-wrapper { | ||
| display: flex; | ||
| transition: transform 0.6s ease-in-out; | ||
| width: 100%; | ||
| } | ||
|
|
||
| .slide { | ||
| min-width: 100%; | ||
| } | ||
|
|
||
| .slide img { | ||
| width: 100%; | ||
| display: block; | ||
| } | ||
|
|
||
| /* Navigation buttons */ | ||
| .prev, .next { | ||
| cursor: pointer; | ||
| position: absolute; | ||
| top: 50%; | ||
| padding: 12px; | ||
| color: white; | ||
| font-size: 18px; | ||
| background-color: rgba(0,0,0,0.5); | ||
| user-select: none; | ||
| transform: translateY(-50%); | ||
| border-radius: 3px; | ||
| } | ||
|
|
||
| .next { | ||
| right: 10px; | ||
| } | ||
|
|
||
| .prev { | ||
| left: 10px; | ||
| } | ||
|
|
||
| .prev:hover, .next:hover { | ||
| background-color: rgba(0,0,0,0.8); | ||
| } | ||
|
|
||
| /* Dots */ | ||
| .dot-container { | ||
| text-align: center; | ||
| position: absolute; | ||
| bottom: 10px; | ||
| width: 100%; | ||
| } | ||
|
|
||
| .dot { | ||
| cursor: pointer; | ||
| height: 12px; | ||
| width: 12px; | ||
| margin: 0 4px; | ||
| background-color: rgba(255,255,255,0.5); | ||
| border-radius: 50%; | ||
| display: inline-block; | ||
| } | ||
|
|
||
| .dot.active { | ||
| background-color: rgba(255,255,255,0.9); | ||
| } | ||
| </style> | ||
|
|
||
| <script> | ||
| let slideIndex = 0; | ||
| let timer; | ||
|
|
||
| function showSlide(index) { | ||
| const slidesWrapper = document.querySelector(".slides-wrapper"); | ||
| const dots = document.querySelectorAll(".dot"); | ||
| const totalSlides = dots.length; | ||
|
|
||
| if (index >= totalSlides) slideIndex = 0; | ||
| if (index < 0) slideIndex = totalSlides - 1; | ||
|
|
||
| slidesWrapper.style.transform = `translateX(-${slideIndex * 100}%)`; | ||
|
|
||
| dots.forEach(dot => dot.classList.remove("active")); | ||
| dots[slideIndex].classList.add("active"); | ||
|
|
||
| clearTimeout(timer); | ||
| timer = setTimeout(() => { | ||
| slideIndex++; | ||
| showSlide(slideIndex); | ||
| }, 4000); | ||
| } | ||
|
|
||
| function changeSlide(n) { | ||
| slideIndex += n; | ||
| showSlide(slideIndex); | ||
| } | ||
|
|
||
| function currentSlide(n) { | ||
| slideIndex = n; | ||
| showSlide(slideIndex); | ||
| } | ||
|
|
||
| document.addEventListener("DOMContentLoaded", () => { | ||
| showSlide(slideIndex); | ||
| }); | ||
| </script> |
There was a problem hiding this comment.
Embedding large blocks of CSS and JavaScript directly in a Markdown file is not recommended for maintainability. It's better to move this CSS and JavaScript into separate files (e.g., docs/assets/css/slideshow.css and docs/assets/js/slideshow.js) and include them in your mkdocs.yml configuration like this:
extra_css:
- assets/css/slideshow.css
extra_javascript:
- assets/js/slideshow.jsThis separates content from presentation and behavior, making the documentation easier to manage.
| ## Installation | ||
|
|
||
| ```terminal | ||
| Clone the repository: git clone https://github.com/openradx/radis.git |
There was a problem hiding this comment.
The descriptive text Clone the repository: is not a command and can be confusing inside a terminal block. It's better to either move it outside the block or comment it out to clearly separate instructions from the commands to be executed.
| Clone the repository: git clone https://github.com/openradx/radis.git | |
| # Clone the repository | |
| git clone https://github.com/openradx/radis.git |
|
|
||
| 1. **Verify no active jobs** | ||
| 2. **Enable maintenance mode**: In Django Admin, navigate to **Common** → **Project Settings** and check the "Maintenance mode" checkbox, then save | ||
| 3. Navigate to Production folder |
There was a problem hiding this comment.
The instruction Navigate to Production folder is a bit vague. It would be helpful to specify what or where this 'Production folder' is to avoid confusion for the administrator.
| 2. Click "Add Subscription" to set up a new subscription | ||
| 3. Enter the Subscription Name and add filter questions. | ||
|
|
||
| - Write each question and select Accept when answer is Yes or No.\ |
There was a problem hiding this comment.
There seems to be a stray backslash (\) at the end of this line. It should be removed.
| - Write each question and select Accept when answer is Yes or No.\ | |
| - Write each question and select Accept when answer is Yes or No. |
| - attr_list # required for Material’s grid layouts:contentReference[oaicite:15]{index=15} | ||
| - md_in_html # required for Material’s grid layouts:contentReference[oaicite:16]{index=16} |
There was a problem hiding this comment.
These comments appear to be auto-generated artifacts and don't provide useful information. They should be removed for clarity.
- attr_list
- md_in_html![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@docs/index.md`:
- Around line 172-178: Replace the bolded section titles "**Developed at**" and
"**in Partnership with**" with proper Markdown headings (e.g., "## Developed at"
and "## in Partnership with" or the appropriate heading level used elsewhere in
the document) to satisfy MD036; keep the link and list content unchanged and use
ATX-style headings for consistency.
In `@docs/user-docs/admin-guide.md`:
- Around line 139-142: Update the example announcement under the
"<strong>Scheduled Maintenance:</strong>" snippet by replacing the past date
string "March 15, 2024 from 2:00 AM to 4:00 AM UTC" with a future date or a
neutral placeholder (e.g., "YYYY-MM-DD from HH:MM to HH:MM UTC") so the example
remains accurate; locate the "<strong>Scheduled Maintenance:</strong>" HTML
block in admin-guide.md and change the date text accordingly.
In `@docs/user-docs/user-guide.md`:
- Around line 96-104: Change the section heading "### 5. Extraction" to "### 5.
Extractions" and update any in-section occurrences of the singular term
"Extraction" (e.g., the header and the phrase "In the next step, search query
will be automatically generated based on these fields") to use the plural
"Extractions" where it refers to the feature name so it matches the overview;
keep the rest of the content unchanged except for matching pluralization for
consistency.
🧹 Nitpick comments (5)
docs/dev-docs/contributing.md (1)
6-7: Promote “Code Style” to a heading for TOC visibility.This will improve MkDocs navigation and keeps the section structure consistent.
♻️ Proposed change
-Code Style -We adhere to the Google Python Style Guide. +## Code Style +We adhere to the Google Python Style Guide..github/workflows/deploy_mkdocs.yml (1)
17-19: Pin the workflow Python version to the project’s supported runtime.Using
3.xcan change over time; aligning withrequires-pythonavoids surprise build breaks.♻️ Proposed change
- - uses: actions/setup-python@v5 - with: - python-version: 3.x + - uses: actions/setup-python@v5 + with: + python-version: "3.12"docs/dev-docs/architecture.md (1)
19-23: Use subheadings instead of bold labels for key components.This improves TOC navigation and addresses markdownlint MD036.
♻️ Proposed change
-**Django Web/API Server**: Central coordination engine providing REST API endpoints, authentication, user/session management, static assets, and task orchestration. Creates job/task records in PostgreSQL and schedules background work. - -**PostgreSQL Database**: System of record storing user accounts, reports, collections, subscriptions, task queue entries, execution history, and search indexes. Uses pg_search and pg_vector extensions for hybrid search capabilities. - -**Background Workers**: Docker containers polling PostgreSQL for tasks, executing AI-powered extractions and subscription processing using LLMs. +#### Django Web/API Server +Central coordination engine providing REST API endpoints, authentication, user/session management, static assets, and task orchestration. Creates job/task records in PostgreSQL and schedules background work. + +#### PostgreSQL Database +System of record storing user accounts, reports, collections, subscriptions, task queue entries, execution history, and search indexes. Uses pg_search and pg_vector extensions for hybrid search capabilities. + +#### Background Workers +Docker containers polling PostgreSQL for tasks, executing AI-powered extractions and subscription processing using LLMs.docs/index.md (1)
20-28: Use button controls for keyboard-accessible slideshow navigation.
Anchors without href are not focus-friendly; buttons give keyboard support by default.♻️ Proposed update
-<a class="prev" onclick="changeSlide(-1)">❮</a> -<a class="next" onclick="changeSlide(1)">❯</a> +<button class="prev" type="button" aria-label="Previous slide" onclick="changeSlide(-1)">❮</button> +<button class="next" type="button" aria-label="Next slide" onclick="changeSlide(1)">❯</button>.prev, .next { cursor: pointer; position: absolute; top: 50%; padding: 12px; color: white; font-size: 18px; background-color: rgba(0,0,0,0.5); user-select: none; transform: translateY(-50%); border-radius: 3px; + border: none; + font: inherit; }Also applies to: 57-77
mkdocs.yml (1)
64-65: Remove straycontentReference[...]artifacts from comments.
They read like accidental injections and reduce clarity.♻️ Proposed cleanup
- - attr_list # required for Material’s grid layouts:contentReference[oaicite:15]{index=15} - - md_in_html # required for Material’s grid layouts:contentReference[oaicite:16]{index=16} + - attr_list # required for Material’s grid layouts + - md_in_html # required for Material’s grid layouts
| ```html | ||
| <strong>Scheduled Maintenance:</strong> RADIS will be offline for maintenance on | ||
| <strong>March 15, 2024 from 2:00 AM to 4:00 AM UTC</strong>. Please plan your | ||
| extractions and subscriptions accordingly. |
There was a problem hiding this comment.
Update the example announcement date to avoid past-dated guidance.
The example uses March 15, 2024, which is in the past as of January 24, 2026. Consider a future date or a placeholder.
💡 Suggested tweak
-<strong>March 15, 2024 from 2:00 AM to 4:00 AM UTC</strong>.
+<strong>March 15, 2026 from 2:00 AM to 4:00 AM UTC</strong>.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ```html | |
| <strong>Scheduled Maintenance:</strong> RADIS will be offline for maintenance on | |
| <strong>March 15, 2024 from 2:00 AM to 4:00 AM UTC</strong>. Please plan your | |
| extractions and subscriptions accordingly. |
🤖 Prompt for AI Agents
In `@docs/user-docs/admin-guide.md` around lines 139 - 142, Update the example
announcement under the "<strong>Scheduled Maintenance:</strong>" snippet by
replacing the past date string "March 15, 2024 from 2:00 AM to 4:00 AM UTC" with
a future date or a neutral placeholder (e.g., "YYYY-MM-DD from HH:MM to HH:MM
UTC") so the example remains accurate; locate the "<strong>Scheduled
Maintenance:</strong>" HTML block in admin-guide.md and change the date text
accordingly.
| ### 5. Extraction | ||
|
|
||
| - Create extraction jobs to analyze multiple reports. | ||
| - Add extraction fields by specifying the following for each field: | ||
| - Name | ||
| - Output Type (Boolean, Text, or Numeric) | ||
| - Description | ||
| - In the next step, search query will be automatically generated based on these fields, which you can then review and refine. | ||
|
|
There was a problem hiding this comment.
Keep feature naming consistent (“Extractions” vs “Extraction”).
The overview uses “Extractions” (plural). Matching the section heading reduces confusion.
💡 Suggested tweak
-### 5. Extraction
+### 5. Extractions📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### 5. Extraction | |
| - Create extraction jobs to analyze multiple reports. | |
| - Add extraction fields by specifying the following for each field: | |
| - Name | |
| - Output Type (Boolean, Text, or Numeric) | |
| - Description | |
| - In the next step, search query will be automatically generated based on these fields, which you can then review and refine. | |
| ### 5. Extractions | |
| - Create extraction jobs to analyze multiple reports. | |
| - Add extraction fields by specifying the following for each field: | |
| - Name | |
| - Output Type (Boolean, Text, or Numeric) | |
| - Description | |
| - In the next step, search query will be automatically generated based on these fields, which you can then review and refine. |
🤖 Prompt for AI Agents
In `@docs/user-docs/user-guide.md` around lines 96 - 104, Change the section
heading "### 5. Extraction" to "### 5. Extractions" and update any in-section
occurrences of the singular term "Extraction" (e.g., the header and the phrase
"In the next step, search query will be automatically generated based on these
fields") to use the plural "Extractions" where it refers to the feature name so
it matches the overview; keep the rest of the content unchanged except for
matching pluralization for consistency.
PR Review: Documentation with MkDocsThanks for adding comprehensive documentation to RADIS! Here is my detailed review: Positive Aspects
Issues to Address1. Orphaned Documentation Files (Priority: Medium)Two markdown files exist in docs/ but are not included in the navigation: docs/Backups.md and docs/Maintenance.md Recommendation: Either add these to mkdocs.yml nav section or move/merge them into existing docs. The content seems relevant for the admin guide. 2. Default Groups Configuration (Priority: Low)In pyproject.toml line 91, the docs group is added to default-groups. This means every developer will install docs dependencies by default, even if they are not working on documentation. Recommendation: Remove docs from default-groups and only install when needed. 3. Documentation Content IssuesAdmin Guide (user-docs/admin-guide.md:166): Unclear token creation instructions - Token hashed: Enter the hashed token value needs clarification about how to generate tokens. Contributing Guide (dev-docs/contributing.md:7): Broken markdown link format for Google Python Style Guide. 4. Accessibility Concerns (Priority: Medium)The slideshow navigation uses Unicode arrows and dots without proper ARIA labels. Add aria-label attributes for screen readers on navigation buttons and dots. 5. CSS Improvements (Priority: Low)Consider adding keyboard focus styles for accessibility on .prev:focus and .next:focus elements. Testing Recommendations
SummaryThis is a solid documentation PR that significantly improves the project. Main issues: orphaned docs files, documentation dependency in default groups, accessibility improvements needed for slideshow, and minor content clarifications. Recommendation: Approve with minor changes requested. The issues identified are non-blocking but should be addressed before or shortly after merge. Great work on the comprehensive documentation! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/index.md`:
- Around line 67-68: Indent the paragraph following the admonition header '!!!
important "Beta Status"' so it is nested under the admonition (use four spaces
or a single tab before the content lines); specifically, add indentation to the
current line starting with "RADIS is currently..." so MkDocs/MkDocs-Material
recognizes it as the admonition body.
♻️ Duplicate comments (2)
docs/index.md (1)
57-65: Replace bold emphasis with headings to satisfy MD036.Lines 57 and 61 use bold text (
**Developed at**,**in Partnership with**) as pseudo-headings, which triggers markdownlint MD036. Use proper heading syntax for structural consistency.mkdocs.yml (1)
65-66: Remove auto-generated artifact comments.The comments
:contentReference[oaicite:15]{index=15}and:contentReference[oaicite:16]{index=16}appear to be AI-generated artifacts and should be removed.🧹 Proposed fix
- - attr_list # required for Material's grid layouts:contentReference[oaicite:15]{index=15} - - md_in_html # required for Material's grid layouts:contentReference[oaicite:16]{index=16} + - attr_list # required for Material's grid layouts + - md_in_html # required for Material's grid layouts
🧹 Nitpick comments (1)
docs/assets/js/slideshow.js (1)
16-40: Confusing parameter usage inshowSlide— index is only used for bounds checks.The
indexparameter is checked against bounds but the function relies on the globalslideIndexbeing pre-set by callers. This works because all current call sites (changeSlide,currentSlide, and the timeout) updateslideIndexbefore calling, but the function signature is misleading.Consider setting
slideIndex = indexat the start for clarity:♻️ Proposed fix
function showSlide(index) { const slidesWrapper = document.querySelector(".slides-wrapper"); const dots = document.querySelectorAll(".dot"); // Guard: Exit early if slideshow markup is not present on this page if (!slidesWrapper || dots.length === 0) { return; } const totalSlides = dots.length; + slideIndex = index; if (index >= totalSlides) slideIndex = 0; if (index < 0) slideIndex = totalSlides - 1;
| !!! important "Beta Status" | ||
| RADIS is currently in an early phase of development. While we are actively building and refining its features, users should anticipate ongoing updates and potential breaking changes as the platform evolves. We appreciate your understanding and welcome feedback to help us shape the future of RADIS. |
There was a problem hiding this comment.
Fix admonition content indentation.
The admonition content on line 68 must be indented (typically 4 spaces) to be properly nested under the !!! important block. Without indentation, MkDocs will not render this correctly.
📝 Proposed fix
!!! important "Beta Status"
-RADIS is currently in an early phase of development. While we are actively building and refining its features, users should anticipate ongoing updates and potential breaking changes as the platform evolves. We appreciate your understanding and welcome feedback to help us shape the future of RADIS.
+ RADIS is currently in an early phase of development. While we are actively building and refining its features, users should anticipate ongoing updates and potential breaking changes as the platform evolves. We appreciate your understanding and welcome feedback to help us shape the future of RADIS.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| !!! important "Beta Status" | |
| RADIS is currently in an early phase of development. While we are actively building and refining its features, users should anticipate ongoing updates and potential breaking changes as the platform evolves. We appreciate your understanding and welcome feedback to help us shape the future of RADIS. | |
| !!! important "Beta Status" | |
| RADIS is currently in an early phase of development. While we are actively building and refining its features, users should anticipate ongoing updates and potential breaking changes as the platform evolves. We appreciate your understanding and welcome feedback to help us shape the future of RADIS. |
🤖 Prompt for AI Agents
In `@docs/index.md` around lines 67 - 68, Indent the paragraph following the
admonition header '!!! important "Beta Status"' so it is nested under the
admonition (use four spaces or a single tab before the content lines);
specifically, add indentation to the current line starting with "RADIS is
currently..." so MkDocs/MkDocs-Material recognizes it as the admonition body.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/user-docs/admin-guide.md`:
- Line 160: There's a spelling typo in the admin guide text: replace the
misspelled word "Naviagte" with "Navigate" in the sentence that references the
"Token Authentication" section (look for the line containing "**Naviagte** to
**Token Authentication**" or the "Token Authentication" heading) so the
instructions read "**Navigate** to **Token Authentication**".
Pull Request Review - Documentation of RADISThank you for this comprehensive documentation addition! This is a valuable contribution that will significantly improve the onboarding experience for users, admins, and developers. Below is my detailed review: ✅ Overall AssessmentStrengths:
🔧 Code Quality & Best PracticesJavaScript (slideshow.js)Positive aspects:
Issues to address:
CSS (slideshow.css)Positive aspects:
Minor suggestions:
GitHub Actions Workflow (deploy_mkdocs.yml)Positive aspects:
Issues:
🐛 Potential Bugs
🔒 Security Concerns
⚡ Performance Considerations
📚 Documentation QualityExcellent work overall! The documentation is comprehensive and well-organized. Minor suggestions:
🧪 Test CoverageConcern: No tests for the slideshow JavaScript functionality Recommendation:
Example test structure: describe('Slideshow', () => {
test('should navigate to next slide', () => {
// Test implementation
});
test('should handle keyboard navigation', () => {
// Test implementation
});
});📋 Additional Recommendations
🎯 Action Items SummaryHigh Priority:
Medium Priority: Low Priority: ✨ ConclusionThis is a high-quality PR that adds significant value to the project. The documentation is comprehensive, well-written, and properly structured. The slideshow implementation is functional and accessible. The main issues are relatively minor and can be addressed quickly:
Once these items are addressed, this PR will be ready to merge. Excellent work! 🎉 Approval Status: Please address the high-priority items before merging. Feel free to tackle medium/low priority items in follow-up PRs if preferred. |
Pull Request Review: Documentation of RADISThank you for this comprehensive documentation PR! This is a valuable addition to the project. Below is my detailed review covering code quality, potential issues, and recommendations. ✅ Strengths
🔍 Issues & Recommendations1. Security & Best PracticesCritical: Inline Event Handlers (docs/index.md:20-27)The slideshow uses inline Current code: <a class="prev" onclick="changeSlide(-1)" aria-label="Previous slide">❮</a>
<span class="dot" onclick="currentSlide(0)" role="button" tabindex="0"></span>Recommendation: Remove inline Why this matters: If RADIS implements CSP headers (recommended for a medical application), inline event handlers will be blocked by default. 2. Code Quality IssuesJavaScript: Redundant Event Listeners (docs/assets/js/slideshow.js)The slideshow.js already uses proper event delegation with CSS: Missing Responsive Design (docs/assets/css/slideshow.css)The slideshow CSS lacks responsive breakpoints. On mobile devices:
Recommendation: Add responsive adjustments: @media (max-width: 768px) {
.prev, .next {
padding: 8px;
font-size: 14px;
}
.dot {
height: 10px;
width: 10px;
}
}3. Workflow ConfigurationMissing Branch Protection (.github/workflows/deploy_mkdocs.yml:36)The workflow uses - name: Deploy MkDocs
run: uv run mkdocs gh-deploy --forceRecommendation: Consider removing Missing Workflow TriggersCurrently only triggers on push to main. Consider adding: on:
push:
branches: [main]
paths:
- 'docs/**'
- 'mkdocs.yml'
- '.github/workflows/deploy_mkdocs.yml'
workflow_dispatch: # Allow manual triggersThis prevents unnecessary deployments when docs aren't changed and allows manual deployment. 4. Documentation Content IssuesInconsistent Command Examples
Recommendation: Ensure command consistency across all documentation. Verify that Incomplete API Token Instructions (admin-guide.md:160)The section on "Naviagte" has a typo and the token management instructions could be more detailed. Typo on line 160: 1. **Naviagte** to **Token Authentication**Should be: Navigate Missing Information
5. Dependency ManagementDevelopment Dependencies Added to Default Groups (pyproject.toml:92)default-groups = ["dev", "client", "docs"]Concern: This means MkDocs dependencies will be installed by default even in production environments where they're not needed. Recommendation: Keep docs as a separate optional group and only install when needed: default-groups = ["dev", "client"]Users who need docs can explicitly run: 6. Testing & ValidationNo Documentation Build ValidationThe main CI workflow (ci.yml) doesn't validate that documentation builds correctly. A broken link or malformed markdown could slip through. Recommendation: Add a docs build check to CI: - name: Validate documentation builds
run: uv run --group docs mkdocs build --strict7. Asset OrganizationDuplicate ScreenshotsScreenshots exist in both Recommendation:
📋 Suggested Changes ChecklistHigh Priority:
Medium Priority:
Low Priority:
🎯 Overall AssessmentThis is a well-structured documentation PR that significantly improves the project. The main concerns are:
The documentation content itself is comprehensive and well-organized. Once the inline event handlers are removed and the dependency groups are adjusted, this will be ready to merge. Recommendation: Request changes to address the high-priority items before merging. Great work on putting this together! Let me know if you have questions about any of these suggestions. |
PR Review: Documentation of RADISThank you for this comprehensive documentation addition! This is a valuable contribution that will significantly improve the onboarding experience for users and developers. Below is my detailed review: ✅ Strengths
🔍 Code Quality Issues1. Inline Event Handlers (Security & Best Practices)Location: The slideshow HTML uses inline <a class="prev" onclick="changeSlide(-1)" aria-label="Previous slide">❮</a>
<a class="next" onclick="changeSlide(1)" aria-label="Next slide">❯</a>
<span class="dot" onclick="currentSlide(0)" role="button" tabindex="0"></span>Issue: Inline event handlers violate Content Security Policy (CSP) best practices and mix JavaScript with HTML. Recommendation: The JavaScript in document.querySelector(".prev")?.addEventListener("click", () => changeSlide(-1));
document.querySelector(".next")?.addEventListener("click", () => changeSlide(1));
dots.forEach((dot, index) => {
dot.addEventListener("click", () => currentSlide(index));
});2. Documentation Dependency ScopeLocation: default-groups = ["dev", "client", "docs"]Issue: Documentation dependencies are added to Recommendation: Documentation should only be installed when needed (i.e., when building docs). Consider:
3. GitHub Actions Workflow SecurityLocation: run: uv run mkdocs gh-deploy --forceIssue: The Recommendation:
4. Missing Workflow Concurrency ControlLocation: Issue: Multiple simultaneous deployments could conflict if commits are pushed rapidly to main. Recommendation: Add concurrency control: concurrency:
group: pages
cancel-in-progress: false🎨 Style & Maintainability5. Line Length in DocumentationSome markdown files have very long lines (e.g., 6. Slideshow CSS: Missing Responsive DesignLocation: The slideshow doesn't have responsive breakpoints for smaller screens. Consider adding media queries to:
7. JavaScript Memory Leak PotentialLocation: The // For MkDocs instant loading/SPA navigation
document.addEventListener('DOMContentSwitch', cleanupSlideshow);📝 Content Suggestions8. Architecture DocumentationLocation: The model name reference "SmolLM2-135M-Instruct" might become outdated. Consider adding a note that this is the default at time of writing and may change. 9. Contributing GuideLocation: The localhost URL assumes default port. Consider mentioning that the port may vary based on configuration. 10. Admin Guide - Update ProcessLocation: Excellent detailed update process! Consider adding:
🧪 Testing11. Missing Tests for New CodeThe slideshow JavaScript has no automated tests. While this is documentation code, consider:
📊 Performance12. Image OptimizationLocation: Screenshot PNG files in Binary files were added but their sizes aren't visible in the diff. Ensure:
13. Slideshow PerformanceThe slideshow auto-advances every 4 seconds, which could be distracting for users trying to read content. Consider:
🔒 Security14. Workflow PermissionsLocation: permissions:
contents: read
pages: write
id-token: writeStatus: ✅ Good! Minimal necessary permissions are correctly specified. 15. No Sanitization Needed for Static ContentThe documentation is static and doesn't accept user input, so XSS risks are minimal. However, ensure that:
📋 SummaryOverall Assessment: This is a solid documentation addition with good structure and content. The main concerns are:
Recommended Changes# docs/index.md
-<a class="prev" onclick="changeSlide(-1)" aria-label="Previous slide">❮</a>
-<a class="next" onclick="changeSlide(1)" aria-label="Next slide">❯</a>
+<a class="prev" aria-label="Previous slide">❮</a>
+<a class="next" aria-label="Next slide">❯</a>
-<span class="dot" onclick="currentSlide(0)" role="button" tabindex="0" aria-label="Go to slide 1"></span>
+<span class="dot" role="button" tabindex="0" aria-label="Go to slide 1"></span>
# (repeat for all dots)
# pyproject.toml
-default-groups = ["dev", "client", "docs"]
+default-groups = ["dev", "client"]
# .github/workflows/deploy_mkdocs.yml
on:
push:
branches:
- main
+
+concurrency:
+ group: pages
+ cancel-in-progress: false
# .github/workflows/deploy_mkdocs.yml
- run: uv run mkdocs gh-deploy --force
+ run: uv run mkdocs gh-deployGreat work overall! The documentation structure is excellent and will be very helpful for users and contributors. 🎉 |
Pull Request Review: Documentation of RADISThank you for this comprehensive documentation effort! This is a valuable addition to the project. Here's my detailed review: ✅ Strengths
🔍 Issues & RecommendationsCritical
High Priority
Medium Priority
Low Priority
🔒 Security Considerations
📊 Test Coverage
📝 Suggested Additions
🎯 SummaryThis is solid documentation work that significantly improves the project's usability. The main concerns are:
Recommendation: Request changes to address critical and high-priority issues. The rest can be addressed in follow-up PRs. Overall assessment: Approve with changes requested Great work on getting this together! The documentation structure is excellent and will be very helpful for users and contributors. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/index.md`:
- Around line 3-30: Update the slideshow markup to be semantic and accessible:
change the interactive prev/next elements (class="prev" and class="next") to use
actual <button> elements or add role="button" and tabindex="0", ensure they have
clear aria-labels (e.g., "Previous slide" / "Next slide"), add role="region" or
aria-label on the slides container (class="slides-wrapper") and role="group" or
role="list" on each slide (class="slide") with aria-live or aria-hidden toggling
so screen readers announce slide changes, add aria-current or aria-label
attributes to the dot indicators (class="dot" data-slide-index="...") and make
them focusable (tabindex="0") with accessible labels, and implement keyboard
handlers (Left/Right and Enter/Space) for class="prev", class="next", and
class="dot" interactions to enable full keyboard navigation and visible keyboard
navigation hints.
🧹 Nitpick comments (1)
docs/index.md (1)
7-17: Consider more descriptive alt text for screenshots.The alt text for the slideshow images is generic ("RADIS Screenshot 1", etc.). While functional, more descriptive alt text would improve accessibility by explaining what each screenshot demonstrates (e.g., "RADIS search interface with filters", "Report collection view", etc.).
… improved event handling
PR Review: Documentation with MkDocsSummaryThis PR adds comprehensive MkDocs documentation with GitHub Pages deployment. Overall, this is a well-structured addition that will significantly improve the project's documentation. The implementation is clean and follows good practices. ✅ StrengthsDocumentation Structure
Code Quality
GitHub Actions Workflow
🔍 Issues & Suggestions1. Security: GitHub Actions Workflow (MODERATE)Location: The workflow uses - name: Deploy MkDocs
run: uv run mkdocs gh-deploy --forceIssue: The Recommendation: Remove - name: Deploy MkDocs
run: uv run mkdocs gh-deploy2. Documentation Content Issues (MINOR)Location: uv run cli stack-deploy # builds and starts Docker containersIssue: This command is mentioned in the installation section, but according to CLAUDE.md, Recommendation: Clarify the context or split into development vs production installation sections. Location: Code Style
We adhere to the Google Python Style [Guide](https://google.github.io/styleguide/pyguide.html).Issue: Missing heading level (should be Recommendation: ## Code Style
We adhere to the Google Python Style [Guide](https://google.github.io/styleguide/pyguide.html).3. Slideshow Implementation (MINOR)Location: Observation: The MutationObserver watches the entire mutationObserver.observe(document.body, {
childList: true,
subtree: true,
});Potential Issue: This could be performance-intensive on pages with frequent DOM changes, though for documentation pages it's likely fine. Recommendation (optional optimization): Consider observing a closer parent element if one exists, or add a check to only observe when on the home page. 4. Missing Alt Text Quality (MINOR)Location: The screenshot alt texts are generic: <img src="assets/screenshots/Screenshot01_radis.png" alt="RADIS Screenshot 1">Recommendation: Provide descriptive alt text that explains what each screenshot shows: <img src="assets/screenshots/Screenshot01_radis.png" alt="RADIS search interface showing report list with filters">
<img src="assets/screenshots/Screenshot02_radis.png" alt="RADIS report detail view with metadata">5. MkDocs Configuration (SUGGESTION)Location: The comment formatting has references that look like artifacts: - attr_list # required for Material's grid layouts:contentReference[oaicite:15]{index=15}Issue: The Recommendation: Clean up the comments: - attr_list # required for Material's grid layouts
- md_in_html # required for Material's grid layouts🧪 Testing Considerations
📊 Performance Considerations
🔒 Security Review
📈 Code Metrics
✅ Final RecommendationAPPROVED with minor suggestions This is a solid PR that adds valuable documentation infrastructure. The code quality is high, accessibility is well-considered, and the documentation content is comprehensive. Before Merging:
After Merging:
Great work on bringing comprehensive documentation to RADIS! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/assets/js/slideshow.js`:
- Around line 89-100: The cleanup function cleanupSlideshow should also reset
the slideshow position by resetting slideIndex to its initial value to avoid
resurrecting the previous position on navigation; update cleanupSlideshow to set
slideIndex back to the slideshow's initial state (e.g., slideIndex = 0 or the
initialSlideIndex variable if one exists) alongside clearing timer and removing
eventListeners.
- Around line 172-177: The slideshow currently only cleans up via the deprecated
DOMContentSwitch and never re-initializes on MkDocs Material instant navigation;
replace the DOMContentSwitch usage by subscribing to the Material document$
observable and refactor the initialization into a reusable function (e.g.,
extract current setup code into initSlideshow or similar) so you can call it
from both the DOMContentLoaded handler and document$.subscribe(), and call
cleanupSlideshow before re-initializing; remove the
document.addEventListener("DOMContentSwitch", ...) line and add a
document$.subscribe(...) that runs cleanupSlideshow then initSlideshow to handle
SPA navigation.
🧹 Nitpick comments (1)
docs/assets/js/slideshow.js (1)
48-55: Consider using only one ofaria-pressedoraria-selected.Both attributes are set on the dot indicators, but they serve different semantic purposes:
aria-pressedis for toggle buttons, whilearia-selectedis for tabs or listbox items. Using both simultaneously may confuse assistive technologies.Choose one based on the dots' role:
- If dots are buttons: use
aria-pressed- If dots are tabs/tablist: use
aria-selected♻️ Suggested fix (assuming button role)
dots.forEach((dot, i) => { dot.classList.remove("active"); dot.setAttribute("aria-pressed", "false"); - dot.setAttribute("aria-selected", "false"); }); dots[slideIndex].classList.add("active"); dots[slideIndex].setAttribute("aria-pressed", "true"); - dots[slideIndex].setAttribute("aria-selected", "true");
| function cleanupSlideshow() { | ||
| // Clear the timer | ||
| if (timer) { | ||
| clearTimeout(timer); | ||
| timer = null; | ||
| } | ||
|
|
||
| // Remove all tracked event listeners | ||
| eventListeners.forEach(({ element, event, handler }) => { | ||
| element.removeEventListener(event, handler); | ||
| }); | ||
| eventListeners = []; |
There was a problem hiding this comment.
Reset slideIndex during cleanup.
When the slideshow is cleaned up (e.g., during SPA navigation), slideIndex retains its previous value. If the user navigates back to the page, the slideshow may start at an unexpected position.
🔧 Proposed fix
function cleanupSlideshow() {
// Clear the timer
if (timer) {
clearTimeout(timer);
timer = null;
}
+
+ // Reset slide index for potential re-initialization
+ slideIndex = 0;
// Remove all tracked event listeners🤖 Prompt for AI Agents
In `@docs/assets/js/slideshow.js` around lines 89 - 100, The cleanup function
cleanupSlideshow should also reset the slideshow position by resetting
slideIndex to its initial value to avoid resurrecting the previous position on
navigation; update cleanupSlideshow to set slideIndex back to the slideshow's
initial state (e.g., slideIndex = 0 or the initialSlideIndex variable if one
exists) alongside clearing timer and removing eventListeners.
2 participants
Adds RADIS documentation using MkDocs (GitHub Pages ready) and updates README with latest screenshots.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.