Feature/improve module management ux#5059
Open
harsitagarwalla187 wants to merge 6 commits into
Open
Conversation
…ule-management-ux
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (3)
Summary by CodeRabbit
WalkthroughModule queries now prefetch ordered mentors, and the frontend updates markdown spacing, module title layout, and mentor login field rendering with matching tests. ChangesModule management UX
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 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}
Contributor
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
Contributor
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
Contributor
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
harsitagarwalla187
force-pushed
the
feature/improve-module-management-ux
branch
from
a96afee to
bdf299b
Compare
Contributor
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
Author
|
@arkid15r Can you please review the PR |
…dit prop, and update test assertions
|
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.
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