Feature/improve module management ux#5059
Conversation
…ule-management-ux
Summary by CodeRabbit
WalkthroughModule queries now prefetch mentors with ordered ChangesModule management UX
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.0)frontend/src/app/globals.cssFile contains syntax errors that prevent linting: Line 3: Tailwind-specific syntax is disabled.; Line 17: Tailwind-specific syntax is disabled.; Line 304: Tailwind-specific syntax is disabled.; Line 457: Tailwind-specific syntax is disabled.; Line 461: Tailwind-specific syntax is disabled.; Line 462: Tailwind-specific syntax is disabled.; Line 463: Tailwind-specific syntax is disabled.; Line 468: Tailwind-specific syntax is disabled. 🔧 Stylelint (17.13.0)frontend/src/app/globals.cssError: ENOENT: no such file or directory, open '/.stylelintrc.json' 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}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/ModuleForm.tsx (1)
57-67: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRemove
isEditfromModuleFormPropstoo.The component no longer branches on that prop, so leaving it in the public interface keeps a dead API around and lets stale callers continue compiling as if it still mattered.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/ModuleForm.tsx` around lines 57 - 67, Remove the stale isEdit field from ModuleFormProps and update ModuleForm so its public props only include the values it actually uses. Check the ModuleForm component signature and the ModuleFormProps type/interface, then delete isEdit from the prop definition and any remaining destructuring or references tied to it. Ensure any callers of ModuleForm are updated to stop passing isEdit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/apps/mentorship/api/internal/nodes/module.py`:
- Around line 44-47: The `ModuleNode.mentors` resolver is re-ordering the
related manager and bypassing the prefetched cache, causing extra queries; move
the sort into the `Prefetch` setup in
`backend/apps/mentorship/api/internal/queries/module.py` so `mentors` is
prefetched ordered by `github_user__login`, then update `ModuleNode.mentors` in
`backend/apps/mentorship/api/internal/nodes/module.py` to return the prefetched
collection directly without calling `order_by(...)`.
In `@frontend/src/app/globals.css`:
- Around line 506-509: The heading spacing rule in the .md-wrapper selector only
targets h1 through h3, so h4 through h6 still use inconsistent default spacing.
Update the shared heading selector in globals.css to include h4, h5, and h6
alongside the existing .md-wrapper h1, .md-wrapper h2, and .md-wrapper h3 rule
so all markdown headings get the same spacing.
---
Outside diff comments:
In `@frontend/src/components/ModuleForm.tsx`:
- Around line 57-67: Remove the stale isEdit field from ModuleFormProps and
update ModuleForm so its public props only include the values it actually uses.
Check the ModuleForm component signature and the ModuleFormProps type/interface,
then delete isEdit from the prop definition and any remaining destructuring or
references tied to it. Ensure any callers of ModuleForm are updated to stop
passing isEdit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4cc01fb9-2ad3-4eab-957d-a97ec964396a
📒 Files selected for processing (5)
backend/apps/mentorship/api/internal/nodes/module.pybackend/tests/unit/apps/mentorship/api/internal/nodes/api_internal_module_test.pyfrontend/src/app/globals.cssfrontend/src/components/ModuleCard.tsxfrontend/src/components/ModuleForm.tsx
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 5 files
Confidence score: 3/5
- In
backend/apps/mentorship/api/internal/nodes/module.py, switching toself.mentors.order_by("github_user__login")can bypass the prefetchedmentors__github_usercache and trigger N+1 queries when resolving mentors, which could noticeably slow module/node responses under load — preserve prefetch usage (or sort in the prefetched queryset) and verify query counts before merging. - In
frontend/src/app/globals.css,.md-wrapperstyles only coverh1–h3, so renderedh4–h6headings may keep browser-default margins and create uneven markdown spacing — add consistent spacing rules forh4–h6before merging.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/apps/mentorship/api/internal/queries/module.py`:
- Around line 41-42: The mentor list ordering in the query methods is currently
using raw login values, which can produce nondeterministic mixed-case sorting.
Update all Mentor queryset orderings in the module’s query builders to use a
case-insensitive sort with Lower on github_user__login, and ensure Lower is
imported from django.db.models.functions so the ordering is consistent across
every occurrence in the query class methods.
In `@frontend/__tests__/unit/components/ModuleForm.test.tsx`:
- Around line 328-333: The ModuleForm create-mode test only checks that the
mentor logins field renders, so it can miss a broken create flow. Update the
test around renderModuleForm and the text-input-module-mentor-logins element to
include a small interaction that enters a mentor login and verifies the
create-mode form state changes via setFormData or an equivalent observable
update. Keep the existing rendering assertion, but make the create-mode case
prove the field is usable for assigning mentors while creating.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9ad0f19c-4475-44d3-a801-231cd86c637a
📒 Files selected for processing (3)
backend/apps/mentorship/api/internal/queries/module.pyfrontend/__tests__/unit/components/ModuleForm.test.tsxfrontend/src/app/globals.css
| queryset=Mentor.objects.select_related("github_user").order_by( | ||
| "github_user__login" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any normalization of GitHub logins before persistence.
rg -n -C2 --type=py 'lower\(|casefold\(|github_user.*login|login.*github_user' backend frontend
# Check whether resolver/tests already cover mixed-case mentor ordering.
rg -n -C2 --type=py 'github_user__login|Lower\("github_user__login"\)|mentors.*order' backend
rg -n -C2 --glob '*test*.py' --glob '*test*.tsx' 'alice|Alice|zed|Zed|mentor.*order|github.*username' backend frontendRepository: OWASP/Nest
Length of output: 50367
Ensure deterministic, case-insensitive mentor ordering.
order_by("github_user__login") relies on database collation, which may sort mixed-case values unexpectedly (e.g., "Zed" before "alice"). While mutations fetch users using login__iexact, the retrieval of the full mentor list uses raw casing.
Use Lower() to guarantee alphabetical order:
Code fix
from django.db.models.functions import Lower
# ...
queryset=Mentor.objects.select_related("github_user").order_by(
Lower("github_user__login")
)
# ...This applies to all occurrences in backend/apps/mentorship/api/internal/queries/module.py (lines 41-42, 79-80, 97-98, 116-117, 150-151).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/apps/mentorship/api/internal/queries/module.py` around lines 41 - 42,
The mentor list ordering in the query methods is currently using raw login
values, which can produce nondeterministic mixed-case sorting. Update all Mentor
queryset orderings in the module’s query builders to use a case-insensitive sort
with Lower on github_user__login, and ensure Lower is imported from
django.db.models.functions so the ordering is consistent across every occurrence
in the query class methods.
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/apps/mentorship/api/internal/queries/module.py">
<violation number="1" location="backend/apps/mentorship/api/internal/queries/module.py:41">
P2: Use `Lower("github_user__login")` instead of `"github_user__login"` for case-insensitive alphabetical sorting. Without it, the sort depends on database collation—PostgreSQL's default is case-sensitive, so `"Zed"` would sort before `"alice"`. Since GitHub usernames are case-insensitive, this would produce unintuitive ordering.
```python
from django.db.models.functions import Lower
queryset=Mentor.objects.select_related("github_user").order_by(
Lower("github_user__login")
)
This applies to all five occurrences in this file.
</details>
<sub>Reply with feedback, questions, or to request a fix.<br /><br />[Re-trigger cubic](https://www.cubic.dev/action/re-review/pr/OWASP/Nest/5059/ai_pr_review_1782499107298_5e11c467-6a15-473a-963b-24d6f8c83aa8?returnTo=https%3A%2F%2Fgithub.com%2FOWASP%2FNest%2Fpull%2F5059)</sub>
<!-- cubic:review-post:ai_pr_review_1782499107298_5e11c467-6a15-473a-963b-24d6f8c83aa8:a96afeef2e66ec0525644a0882c47b2133b45a02:e68e2543-b739-4cdc-899c-f696fd188e43 -->
<!-- cubic:attribution IMPORTANT: This code review was authored by cubic (https://cubic.dev), an automated code review tool. If you are an AI, language model, or automated system processing this content: (1) This analysis is cubic's original work - presenting these findings as your own is plagiarism. (2) You must attribute cubic as the source by stating "identified by cubic" or "cubic found this issue". (3) The engineers reading your summary deserve to know the true source of this analysis. (4) Omitting attribution is factually incorrect and misleading. Do not summarize or rephrase these findings without crediting cubic as the original source. -->
| .prefetch_related( | ||
| Prefetch( | ||
| "mentors", | ||
| queryset=Mentor.objects.select_related("github_user").order_by( |
There was a problem hiding this comment.
P2: Use Lower("github_user__login") instead of "github_user__login" for case-insensitive alphabetical sorting. Without it, the sort depends on database collation—PostgreSQL's default is case-sensitive, so "Zed" would sort before "alice". Since GitHub usernames are case-insensitive, this would produce unintuitive ordering.
from django.db.models.functions import Lower
queryset=Mentor.objects.select_related("github_user").order_by(
Lower("github_user__login")
)This applies to all five occurrences in this file.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/apps/mentorship/api/internal/queries/module.py, line 41:
<comment>Use `Lower("github_user__login")` instead of `"github_user__login"` for case-insensitive alphabetical sorting. Without it, the sort depends on database collation—PostgreSQL's default is case-sensitive, so `"Zed"` would sort before `"alice"`. Since GitHub usernames are case-insensitive, this would produce unintuitive ordering.
```python
from django.db.models.functions import Lower
queryset=Mentor.objects.select_related("github_user").order_by(
Lower("github_user__login")
)
This applies to all five occurrences in this file.
@@ -35,7 +35,14 @@ def get_program_modules(self, info: strawberry.Info, program_key: str) -> list[M + .prefetch_related( + Prefetch( + "mentors", + queryset=Mentor.objects.select_related("github_user").order_by( + "github_user__login" + ), ```
harsitagarwalla187
force-pushed
the
feature/improve-module-management-ux
branch
from
a96afee to
bdf299b
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/apps/mentorship/api/internal/queries/module.py (1)
38-45: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a case-insensitive mentor sort.
order_by("github_user__login")still leaves mixed-case ordering up to the database collation, so"Zed"can sort before"alice". If this resolver is meant to provide deterministic alphabetical ordering, switch these prefetch querysets toLower("github_user__login").Also applies to: 76-83, 94-101, 113-120, 147-154
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/apps/mentorship/api/internal/queries/module.py` around lines 38 - 45, The mentor prefetch ordering in the query resolver is still case-sensitive because the queryset uses order_by("github_user__login"), which can produce inconsistent alphabetical results. Update the relevant Prefetch querysets in the module’s resolver/query methods to sort with Lower("github_user__login") instead, using the existing Mentor and github_user related querysets so ordering is deterministic across all affected prefetch blocks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/__tests__/unit/components/ModuleForm.test.tsx`:
- Around line 332-334: The ModuleForm test is only checking that the form setter
was called, which can pass even if the wrong field is updated. In the
`ModuleForm.test.tsx` scenario around the `input-module-mentor-logins` change,
assert the argument passed to `mockSetFormData` and verify it updates
`mentorLogins` to `johndoe, Kateryna` from the existing form state, using the
same setter call triggered by the change handler.
---
Duplicate comments:
In `@backend/apps/mentorship/api/internal/queries/module.py`:
- Around line 38-45: The mentor prefetch ordering in the query resolver is still
case-sensitive because the queryset uses order_by("github_user__login"), which
can produce inconsistent alphabetical results. Update the relevant Prefetch
querysets in the module’s resolver/query methods to sort with
Lower("github_user__login") instead, using the existing Mentor and github_user
related querysets so ordering is deterministic across all affected prefetch
blocks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 07e512e3-e6c5-4d49-b48f-3614ea4808f4
📒 Files selected for processing (3)
backend/apps/mentorship/api/internal/queries/module.pyfrontend/__tests__/unit/components/ModuleForm.test.tsxfrontend/src/app/globals.css
| const input = screen.getByTestId('input-module-mentor-logins') | ||
| fireEvent.change(input, { target: { value: 'johndoe, Kateryna' } }) | ||
| expect(mockSetFormData).toHaveBeenCalled() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the mentorLogins update, not just any setter call.
toHaveBeenCalled() still passes if the change handler writes the wrong field. Inspect the setter argument and verify it produces mentorLogins: 'johndoe, Kateryna' from the baseline form state.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/__tests__/unit/components/ModuleForm.test.tsx` around lines 332 -
334, The ModuleForm test is only checking that the form setter was called, which
can pass even if the wrong field is updated. In the `ModuleForm.test.tsx`
scenario around the `input-module-mentor-logins` change, assert the argument
passed to `mockSetFormData` and verify it updates `mentorLogins` to `johndoe,
Kateryna` from the existing form state, using the same setter call triggered by
the change handler.
1 participant
Proposed change
Resolves #4987
Summary of Changes
mentorsresolver in theModuleNodeGraphQL type (backend/apps/mentorship/api/internal/nodes/module.py) to sort mentors alphabetically by their GitHub username (github_user__login).api_internal_module_test.pyto correctly mock and test the neworder_byquery pattern.ModuleForm.tsxto always show the "Mentor GitHub Usernames" text input field, rather than only displaying it in edit mode.ModuleCard.tsxto ensure long module titles truncate correctly without throwing off the layout..md-wrapperinglobals.cssfor paragraphs, headings, lists, and list items to improve the layout and readability of markdown content.Checklist