Repository navigation
Conversation
Bumps the npm_and_yarn group with 1 update in the /frontend directory: [basic-ftp](https://github.com/patrickjuchli/basic-ftp). Updates `basic-ftp` from 5.2.0 to 5.2.1 - [Release notes](https://github.com/patrickjuchli/basic-ftp/releases) - [Changelog](https://github.com/patrickjuchli/basic-ftp/blob/master/CHANGELOG.md) - [Commits](patrickjuchli/basic-ftp@v5.2.0...v5.2.1) --- updated-dependencies: - dependency-name: basic-ftp dependency-version: 5.2.1 dependency-type: indirect dependency-group: npm_and_yarn ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…ASP#4522) Bumps the pip group with 2 updates in the /backend directory: [cryptography](https://github.com/pyca/cryptography) and [langchain-core](https://github.com/langchain-ai/langchain). Updates `cryptography` from 46.0.6 to 46.0.7 - [Changelog](https://github.com/pyca/cryptography/blob/main/CHANGELOG.rst) - [Commits](pyca/cryptography@46.0.6...46.0.7) Updates `langchain-core` from 1.2.23 to 1.2.28 - [Release notes](https://github.com/langchain-ai/langchain/releases) - [Commits](langchain-ai/langchain@langchain-core==1.2.23...langchain-core==1.2.28) --- updated-dependencies: - dependency-name: cryptography dependency-version: 46.0.7 dependency-type: indirect dependency-group: pip - dependency-name: langchain-core dependency-version: 1.2.28 dependency-type: indirect dependency-group: pip ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* feat: add Playwright smoke test for board candidates page * feat: improve board candidates smoke test assertions * Update code --------- Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org> Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com>
Bumps the npm_and_yarn group with 1 update in the /frontend directory: [basic-ftp](https://github.com/patrickjuchli/basic-ftp). Updates `basic-ftp` from 5.2.1 to 5.2.2 - [Release notes](https://github.com/patrickjuchli/basic-ftp/releases) - [Changelog](https://github.com/patrickjuchli/basic-ftp/blob/master/CHANGELOG.md) - [Commits](patrickjuchli/basic-ftp@v5.2.1...v5.2.2) --- updated-dependencies: - dependency-name: basic-ftp dependency-version: 5.2.2 dependency-type: indirect dependency-group: npm_and_yarn ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* ci: add pre-commit ecosystem support for Dependabot * Update code --------- Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org>
* docs: add local testing steps to NestBot development guide Closes OWASP#4333 ## Summary Adds missing local testing instructions to the NestBot Development section in CONTRIBUTING.md. ## Changes - Added Testing NestBot Locally subsection - Included make logs command for checking Django events ## Checklist - [x] Documentation only change - [x] No new dependencies added - [x] Improves contributor onboarding experience * Update code --------- Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org>
…Guidelines (OWASP#4508) Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com>
* fix:add-ecs-containers-health Signed-off-by: hassaansaleem28 <iamhassaans@gmail.com> * fix: add frontend health endpoint Signed-off-by: hassaansaleem28 <iamhassaans@gmail.com> * bot suggestion Signed-off-by: hassaansaleem28 <iamhassaans@gmail.com> * Update fuzz-compose.yml Signed-off-by: hassaansaleem28 <iamhassaans@gmail.com> * edit endpoint to /api/health Signed-off-by: hassaansaleem28 <iamhassaans@gmail.com> * Update ALB variables.tf Signed-off-by: hassaansaleem28 <iamhassaans@gmail.com> * changes after: make check run Signed-off-by: hassaansaleem28 <iamhassaans@gmail.com> * update code --------- Signed-off-by: hassaansaleem28 <iamhassaans@gmail.com> Co-authored-by: Rudransh Shrivastava <rudransh.shrivastava@owasp.org> Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com>
* fix: remove timeline rendering tests and related code from Card component * resolved comments --------- Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com>
* docs: add local testing steps for NestBot * fix: markdown lint issues in NestBot testing section * fix: Remove trailing whitespace at Line 278 --------- Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org>
Bumps [eslint](https://github.com/eslint/eslint) in `/frontend` from 10.0.3 to 10.1.0. Updates `eslint` from 10.0.3 to 10.1.0 - [Release notes](https://github.com/eslint/eslint/releases) - [Commits](eslint/eslint@v10.0.3...v10.1.0) --- updated-dependencies: - dependency-name: eslint dependency-version: 10.1.0 dependency-type: direct:development update-type: version-update:semver-minor dependency-group: npm_and_yarn ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
OWASP#4527) (OWASP#4530) Co-authored-by: Deepak Singh <deepak.singh@example.com> Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com>
* Improve navbar hover and active states * Fix aria-current accessibility issue * Remove unintended lockfile changes * Update code --------- Co-authored-by: Kate <kate@kgthreads.com>
…WASP#3608) * feat: Enable Pagination for Pull Requests on Issue and Module Pages * fix: code rabbit address * fix: code rabbit comments * fix:cubic-dev-ai comment * fix: cubic-dev-ai error * fix: cubic-dev-ai comments * fix: cubic-dev-ai error * fix cubic-dev-ai error * Update code --------- Co-authored-by: Kate <kate@kgthreads.com>
…gation (OWASP#4539) * fix: remove community from HIDDEN_SEGMENTS to restore breadcrumb navigation * Update code --------- Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org>
Contributor
Summary by CodeRabbitRelease Notes
WalkthroughThis PR implements pull-request pagination across the stack: backend GraphQL resolvers now support Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 |
github-actions
Bot
added
docs
|
PR validation failed: No linked issue and no valid closing issue reference in PR description |
|
![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 45 files
Confidence score: 3/5
- There is a concrete regression risk: changing the default resolver behavior in
backend/apps/github/api/internal/nodes/issue.pytolimit=4alters the field contract and can silently truncate results for callers that omitlimit. frontend/src/server/queries/issueQueries.tsintroduces default pagination variables that may cause refetches to return only the first 4 pull requests after mutations, which is user-visible and likely confusing.- Given the high confidence on both findings and direct impact on list completeness, this carries some merge risk, though it looks fixable with targeted default/variable handling updates.
- Pay close attention to
backend/apps/github/api/internal/nodes/issue.py,frontend/src/server/queries/issueQueries.ts- default pagination values can unintentionally truncate returned issue/PR lists.
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/github/api/internal/nodes/issue.py">
<violation number="1" location="backend/apps/github/api/internal/nodes/issue.py:48">
P1: Defaulting this resolver to `limit=4` changes the existing field contract and silently truncates results for callers that do not pass a limit.</violation>
</file>
<file name="frontend/src/server/queries/issueQueries.ts">
<violation number="1" location="frontend/src/server/queries/issueQueries.ts:32">
P2: Adding pagination defaults here collapses the pull-request list back to the first 4 items after mutation refetches, because existing `refetchQueries` callers do not pass the new `limit`/`offset` variables.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Comment on lines
+48
to
+56
| def pull_requests(self, limit: int = 4, offset: int = 0) -> list[PullRequestNode]: | ||
| """Return pull requests linked to this issue.""" | ||
| if (normalized_limit := normalize_limit(limit, MAX_LIMIT)) is None: | ||
| return [] | ||
|
|
||
| offset = max(0, offset) | ||
| return list( | ||
| self.pull_requests.all().order_by("-created_at")[offset : offset + normalized_limit] | ||
| ) |
Contributor
There was a problem hiding this comment.
P1: Defaulting this resolver to limit=4 changes the existing field contract and silently truncates results for callers that do not pass a limit.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/apps/github/api/internal/nodes/issue.py, line 48:
<comment>Defaulting this resolver to `limit=4` changes the existing field contract and silently truncates results for callers that do not pass a limit.</comment>
<file context>
@@ -40,7 +43,17 @@ class IssueNode(strawberry.relay.Node):
- pull_requests: list[PullRequestNode] = strawberry_django.field()
+
+ @strawberry_django.field(prefetch_related=["pull_requests"])
+ def pull_requests(self, limit: int = 4, offset: int = 0) -> list[PullRequestNode]:
+ """Return pull requests linked to this issue."""
+ if (normalized_limit := normalize_limit(limit, MAX_LIMIT)) is None:
</file context>
Suggested change
| def pull_requests(self, limit: int = 4, offset: int = 0) -> list[PullRequestNode]: | |
| """Return pull requests linked to this issue.""" | |
| if (normalized_limit := normalize_limit(limit, MAX_LIMIT)) is None: | |
| return [] | |
| offset = max(0, offset) | |
| return list( | |
| self.pull_requests.all().order_by("-created_at")[offset : offset + normalized_limit] | |
| ) | |
| def pull_requests(self, limit: int | None = None, offset: int = 0) -> list[PullRequestNode]: | |
| """Return pull requests linked to this issue.""" | |
| offset = max(0, offset) | |
| queryset = self.pull_requests.all().order_by("-created_at") | |
| if limit is None: | |
| return list(queryset[offset:]) | |
| if (normalized_limit := normalize_limit(limit, MAX_LIMIT)) is None: | |
| return [] | |
| return list(queryset[offset : offset + normalized_limit]) |
| } | ||
| labels | ||
| pullRequests { | ||
| pullRequests(limit: $limit, offset: $offset) { |
Contributor
There was a problem hiding this comment.
P2: Adding pagination defaults here collapses the pull-request list back to the first 4 items after mutation refetches, because existing refetchQueries callers do not pass the new limit/offset variables.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/server/queries/issueQueries.ts, line 32:
<comment>Adding pagination defaults here collapses the pull-request list back to the first 4 items after mutation refetches, because existing `refetchQueries` callers do not pass the new `limit`/`offset` variables.</comment>
<file context>
@@ -23,7 +29,7 @@ export const GET_MODULE_ISSUE_VIEW = gql`
}
labels
- pullRequests {
+ pullRequests(limit: $limit, offset: $offset) {
id
title
</file context>
rudransh-shrivastava
marked this pull request as ready for review
April 13, 2026 16:44
![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: 3
🧹 Nitpick comments (10)
CONTRIBUTING.md (1)
276-276: Clarify the exact log command for faster local troubleshooting.Consider adding the specific command to view logs (for example, the project’s
maketarget or container command) right here to reduce ambiguity for first-time contributors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CONTRIBUTING.md` at line 276, Update the line "To confirm Slack is reaching your machine, check the backend container logs." to include exact commands for viewing those logs: add the project's make target name (e.g., the make target used for backend logs) and the container orchestration command (e.g., the docker-compose or docker command and the flag to follow logs) so contributors can copy/paste a concrete command; reference the backend service name used in the repo (backend) and show both the make-based and container-based options and mention the follow/stream flag.infrastructure/modules/service/tests/service.tftest.hcl (1)
122-160: Add a non-defaulthealth_check_pathtest case.Current assertions validate the default path, but they don’t prove that overrides work (or that
/isn’t hardcoded).➕ Suggested test addition
+run "test_task_definition_health_check_path_override" { + command = plan + + variables { + health_check_path = "/healthz" + } + + assert { + condition = strcontains(local.container_definition.healthCheck.command[1], ":${var.container_port}/healthz") + error_message = "Health check command must honor overridden health_check_path." + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/modules/service/tests/service.tftest.hcl` around lines 122 - 160, Add a new test run to assert non-default health_check_path is respected: create another run block (similar to run "test_task_definition_has_container_health_check") that sets var.health_check_path to a custom value (e.g., "/custom") and then assert that local.container_definition.healthCheck.command[1] contains ":${var.container_port}${var.health_check_path}" (matching the custom path) to ensure the path is not hardcoded to "/". Keep the checks referencing local.container_definition.healthCheck and var.container_port/var.health_check_path so the test verifies overrides work.frontend/__tests__/e2e/pages/BoardCandidates.spec.ts (1)
4-7: Decouple E2E test expectations from hard-coded board yearsThe test assumes
2025exists and2020does not exist without documenting this assumption or validating it at runtime. Since board data is synced from the externalwww-board-candidatesGitHub repository, the test becomes brittle if that repository changes. Consider sourcing valid and invalid years from the database state at test time, or documenting why these specific years are guaranteed to exist in the test environment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/e2e/pages/BoardCandidates.spec.ts` around lines 4 - 7, Replace the brittle hard-coded years in the E2E test by dynamically deriving a valid and invalid year at runtime: call the app's public API or test helper that returns available board years (or query the test DB) and set validYear to one returned value (e.g., first/most recent), then set invalidYear to a year guaranteed absent (e.g., min(returnedYears) - 1) so validUrl and invalidUrl are built from those variables; if no runtime source is available, add an explicit comment in BoardCandidates.spec.ts explaining why '2025' and '2020' are guaranteed in this test environment.frontend/__tests__/unit/components/CardDetailsPage.test.tsx (1)
991-1014: Add click assertions for the new pagination controls.These tests currently assert render state only; please also verify handler invocation to catch wiring regressions.
Suggested test strengthening
- it('renders Show More button when onLoadMorePullRequests is provided', () => { + it('renders Show More button and triggers load callback', () => { + const onLoadMorePullRequests = jest.fn() render( <CardDetailsPage {...defaultProps} type="module" pullRequests={mockPullRequests as unknown as PullRequest[]} - onLoadMorePullRequests={jest.fn()} + onLoadMorePullRequests={onLoadMorePullRequests} /> ) - expect(screen.getByRole('button', { name: /Show more/i })).toBeInTheDocument() + const showMore = screen.getByRole('button', { name: /Show more/i }) + expect(showMore).toBeInTheDocument() + fireEvent.click(showMore) + expect(onLoadMorePullRequests).toHaveBeenCalledTimes(1) }) - it('renders Show Less button when onResetPullRequests is provided', () => { + it('renders Show Less button and triggers reset callback', () => { + const onResetPullRequests = jest.fn() render( <CardDetailsPage {...defaultProps} type="module" pullRequests={mockPullRequests as unknown as PullRequest[]} - onResetPullRequests={jest.fn()} + onResetPullRequests={onResetPullRequests} onLoadMorePullRequests={undefined} /> ) - expect(screen.getByRole('button', { name: /Show less/i })).toBeInTheDocument() + const showLess = screen.getByRole('button', { name: /Show less/i }) + expect(showLess).toBeInTheDocument() + fireEvent.click(showLess) + expect(onResetPullRequests).toHaveBeenCalledTimes(1) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/components/CardDetailsPage.test.tsx` around lines 991 - 1014, Update the two tests for CardDetailsPage to not only assert the buttons render but also verify the handlers are invoked: replace the inline jest.fn() with named mock functions (e.g., const onLoadMore = jest.fn() and const onReset = jest.fn()), render CardDetailsPage with those mocks passed to onLoadMorePullRequests and onResetPullRequests respectively, use userEvent.click or fireEvent.click on the "Show more" / "Show less" buttons, and add expects that onLoadMore and onReset were called (to ensure the component's pagination wiring via onLoadMorePullRequests and onResetPullRequests works).backend/tests/unit/apps/github/api/internal/nodes/issue_test.py (1)
91-120: Add a negative-offset boundary test for resolver parity.The resolver clamps negative offsets to zero; adding that case here would lock in the pagination contract and prevent regressions.
Suggested additional assertion
def test_pull_requests_resolver(self): """Test pull_requests field resolver with pagination.""" mock_issue = Mock() mock_qs = Mock() mock_issue.pull_requests.all.return_value = mock_qs mock_qs.order_by.return_value = ["pr1", "pr2", "pr3", "pr4", "pr5"] field = self._get_field_by_name("pull_requests", IssueNode) result = field.base_resolver.wrapped_func(mock_issue) assert result == ["pr1", "pr2", "pr3", "pr4"] result = field.base_resolver.wrapped_func(mock_issue, limit=2, offset=2) assert result == ["pr3", "pr4"] + result = field.base_resolver.wrapped_func(mock_issue, limit=2, offset=-10) + assert result == ["pr1", "pr2"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/unit/apps/github/api/internal/nodes/issue_test.py` around lines 91 - 120, Add a negative-offset assertion to the pull_requests resolver test to ensure offsets are clamped to zero: in test_pull_requests_resolver_security (or next to test_pull_requests_resolver) call IssueNode's field.base_resolver.wrapped_func(mock_issue, limit=10, offset=-5) after setting up mock_issue.pull_requests.all().order_by().return_value and mock normalize_limit, and assert the resolver returns the same result as offset=0 (or that the underlying queryset was called with offset 0 behavior); reference IssueNode, field.base_resolver.wrapped_func, and normalize_limit to find where to add this check.frontend/src/components/CardDetailsPage.tsx (1)
416-441: Inconsistent loading state handling between "Show more" and "Show less" buttons.The "Show more" button (line 425) displays "Loading..." text when
isFetchingMoreis true, but the "Show less" button (line 437) always displays "Show less" even when disabled. While clicking "Show less" doesn't trigger a network request, having both buttons disabled with different text might be slightly confusing.This is a minor UX consideration and the current implementation is functional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/CardDetailsPage.tsx` around lines 416 - 441, Update the "Show less" button in CardDetailsPage.tsx to mirror the loading/disabled UI of the "Show more" button: when isFetchingMore is true, render a loading label (e.g., "Loading...") and apply the same disabled state/classname logic for the onResetPullRequests button as used for onLoadMorePullRequests so both buttons behave and appear consistently while isFetchingMore is true.frontend/__tests__/unit/pages/ModuleIssueDetailsPage.test.tsx (1)
566-568: Unnecessaryact()wrapper aroundfireEvent.click.
fireEventfrom React Testing Library already wraps events inact()internally, so the explicitact()wrapper is redundant. This won't cause issues but adds unnecessary noise.♻️ Proposed simplification
- await act(async () => { - fireEvent.click(showMoreButton) - }) + fireEvent.click(showMoreButton)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/pages/ModuleIssueDetailsPage.test.tsx` around lines 566 - 568, Remove the redundant act() wrapper around the fireEvent.click call in the test: replace the block that calls await act(async () => { fireEvent.click(showMoreButton) }) with a direct fireEvent.click(showMoreButton) (or await if awaiting promises), updating the test that uses act and showMoreButton so it relies on React Testing Library's internal act wrapping rather than the explicit act call.frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx (1)
403-467: Complex conditional logic for pagination controls.The condition on lines 403-405 is quite complex:
{(hasMorePRs || (issue.pullRequests || []).length > visibleCount || (visibleCount > limit && (issue.pullRequests || []).length > limit)) && (This combines three conditions that are partially redundant. The inner button visibility conditions (lines 407, 457) add additional checks. Consider simplifying:
♻️ Proposed simplification
- {(hasMorePRs || - (issue.pullRequests || []).length > visibleCount || - (visibleCount > limit && (issue.pullRequests || []).length > limit)) && ( + {/* Show pagination controls if there are more PRs to show or user can collapse */} + {(hasMorePRs || + (issue.pullRequests?.length ?? 0) > visibleCount || + visibleCount > limit) && ( <div className="mt-4 flex justify-start gap-4"> - {(hasMorePRs || (issue.pullRequests || []).length > visibleCount) && ( + {(hasMorePRs || (issue.pullRequests?.length ?? 0) > visibleCount) && ( <button ...>Show more</button> )} - {visibleCount > limit && (issue.pullRequests || []).length > limit && ( + {visibleCount > limit && ( <button ...>Show less</button> )} </div> )}The
(issue.pullRequests || []).length > limitcheck for "Show less" is redundant sincevisibleCount > limitimplies the user has already expanded beyond the initial view.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/my/mentorship/programs/`[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx around lines 403 - 467, The pagination wrapper's conditional combines redundant checks (hasMorePRs, (issue.pullRequests || []).length > visibleCount, and (visibleCount > limit && (issue.pullRequests || []).length > limit)); simplify by showing the container whenever either hasMorePRs is true or the total PR count exceeds the initial limit or current visibleCount (i.e. use: hasMorePRs || (issue.pullRequests || []).length > limit || (issue.pullRequests || []).length > visibleCount), and remove the redundant total-length check from the "Show less" button (rely solely on visibleCount > limit to render the "Show less" button). Update references in the JSX around hasMorePRs, visibleCount, limit, and issue.pullRequests to use the simplified conditions and ensure the inner button checks (the "Show more" click guard) still respect hasMorePRs and fetching state.frontend/src/app/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsx (2)
104-141: Consider extracting pagination logic to a custom hook for reusability.The
onLoadMorePullRequestshandler contains complex pagination logic that's duplicated in the issue details page. Consider extracting this to a reusable hook likeusePaginatedPullRequests.This is a recommended improvement for maintainability given the duplication with
ModuleIssueDetailsPage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/mentorship/programs/`[programKey]/modules/[moduleKey]/page.tsx around lines 104 - 141, Extract the pagination logic inside the onLoadMorePullRequests handler into a reusable custom hook (suggested name: usePaginatedPullRequests) that encapsulates state (visibleCount, isFetchingMore, hasMorePRs), the fetchMore invocation (including variables and updateQuery merging logic that references programModule.recentPullRequests), and the boundary checks/setters (setIsFetchingMore, setVisibleCount). Replace the inline handler in page.tsx with the hook's onLoadMore function and return the necessary values from the hook so ModuleIssueDetailsPage can reuse it as well; ensure the hook API accepts parameters like programKey, moduleKey, limit, and the base recentPullRequests to compute currentLength and determine hasMore behavior.
39-45: Pagination state initialization logic could be clearer.The
useEffectsetshasMorePRsbased on whether the initial PR count equals or exceedslimit:if (prCount <= limit) { setHasMorePRs(prCount >= limit) // true if prCount === limit }This logic is correct (if exactly 4 PRs are returned, there might be more), but the nested condition is a bit confusing. Consider simplifying:
♻️ Proposed simplification
useEffect(() => { const prCount = data?.getModule?.recentPullRequests?.length if (prCount == null) return - if (prCount <= limit) { - setHasMorePRs(prCount >= limit) - } + // If we received exactly `limit` PRs, there might be more on the server + // If we received fewer, we've exhausted the list + setHasMorePRs(prCount >= limit) }, [data, limit])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/mentorship/programs/`[programKey]/modules/[moduleKey]/page.tsx around lines 39 - 45, The current useEffect uses a nested condition that is confusing: it only calls setHasMorePRs when prCount <= limit then sets it to prCount >= limit; instead simplify by, after the null check, unconditionally setting hasMorePRs based on the retrieved count (e.g., compute prCount = data?.getModule?.recentPullRequests?.length; if (prCount == null) return; setHasMorePRs(prCount >= limit)); update the useEffect that uses useEffect, prCount, setHasMorePRs, data?.getModule?.recentPullRequests?.length and limit accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/Header.tsx`:
- Around line 99-104: The active-state and aria-current checks use the raw
link.href while the anchor's href uses a fallback ('/'), causing mismatches for
links without href; resolve the href once (e.g., create a resolvedHref =
link.href || '/') and use resolvedHref both for href={...} and in the comparison
with pathname (and aria-current) so the active styling and aria attributes match
the rendered href; update references to link.href in this component (where cn is
called and aria-current set) to use the resolved variable.
In `@frontend/src/utils/getIcsFileUrl.ts`:
- Around line 10-19: The event dates are extracted in UTC inside parseDate and
getEndDate but the ics library treats DateArray as local time unless you set
input types; update the eventAttributes object (where you build the event
payload for the ics package) to include startInputType: 'utc' and endInputType:
'utc' so the parsed DateArray from parseDate and getEndDate is interpreted as
UTC by the ics library.
In `@infrastructure/modules/service/main.tf`:
- Around line 18-25: The ECS container healthCheck command uses wget but the
images don't install it; update the Dockerfiles to install wget so the health
check works: in the backend Dockerfile add wget to the existing apk --no-cache
add make postgresql16-client=16.13-r0 invocation (i.e., include wget in that
package list), and in the frontend Dockerfile add apk add --no-cache wget in the
runner stage where apk update && apk upgrade --no-cache is run; alternatively
replace the healthCheck command with a tool already present or a shell-native
probe if you prefer not to add wget.
---
Nitpick comments:
In `@backend/tests/unit/apps/github/api/internal/nodes/issue_test.py`:
- Around line 91-120: Add a negative-offset assertion to the pull_requests
resolver test to ensure offsets are clamped to zero: in
test_pull_requests_resolver_security (or next to test_pull_requests_resolver)
call IssueNode's field.base_resolver.wrapped_func(mock_issue, limit=10,
offset=-5) after setting up
mock_issue.pull_requests.all().order_by().return_value and mock normalize_limit,
and assert the resolver returns the same result as offset=0 (or that the
underlying queryset was called with offset 0 behavior); reference IssueNode,
field.base_resolver.wrapped_func, and normalize_limit to find where to add this
check.
In `@CONTRIBUTING.md`:
- Line 276: Update the line "To confirm Slack is reaching your machine, check
the backend container logs." to include exact commands for viewing those logs:
add the project's make target name (e.g., the make target used for backend logs)
and the container orchestration command (e.g., the docker-compose or docker
command and the flag to follow logs) so contributors can copy/paste a concrete
command; reference the backend service name used in the repo (backend) and show
both the make-based and container-based options and mention the follow/stream
flag.
In `@frontend/__tests__/e2e/pages/BoardCandidates.spec.ts`:
- Around line 4-7: Replace the brittle hard-coded years in the E2E test by
dynamically deriving a valid and invalid year at runtime: call the app's public
API or test helper that returns available board years (or query the test DB) and
set validYear to one returned value (e.g., first/most recent), then set
invalidYear to a year guaranteed absent (e.g., min(returnedYears) - 1) so
validUrl and invalidUrl are built from those variables; if no runtime source is
available, add an explicit comment in BoardCandidates.spec.ts explaining why
'2025' and '2020' are guaranteed in this test environment.
In `@frontend/__tests__/unit/components/CardDetailsPage.test.tsx`:
- Around line 991-1014: Update the two tests for CardDetailsPage to not only
assert the buttons render but also verify the handlers are invoked: replace the
inline jest.fn() with named mock functions (e.g., const onLoadMore = jest.fn()
and const onReset = jest.fn()), render CardDetailsPage with those mocks passed
to onLoadMorePullRequests and onResetPullRequests respectively, use
userEvent.click or fireEvent.click on the "Show more" / "Show less" buttons, and
add expects that onLoadMore and onReset were called (to ensure the component's
pagination wiring via onLoadMorePullRequests and onResetPullRequests works).
In `@frontend/__tests__/unit/pages/ModuleIssueDetailsPage.test.tsx`:
- Around line 566-568: Remove the redundant act() wrapper around the
fireEvent.click call in the test: replace the block that calls await act(async
() => { fireEvent.click(showMoreButton) }) with a direct
fireEvent.click(showMoreButton) (or await if awaiting promises), updating the
test that uses act and showMoreButton so it relies on React Testing Library's
internal act wrapping rather than the explicit act call.
In
`@frontend/src/app/mentorship/programs/`[programKey]/modules/[moduleKey]/page.tsx:
- Around line 104-141: Extract the pagination logic inside the
onLoadMorePullRequests handler into a reusable custom hook (suggested name:
usePaginatedPullRequests) that encapsulates state (visibleCount, isFetchingMore,
hasMorePRs), the fetchMore invocation (including variables and updateQuery
merging logic that references programModule.recentPullRequests), and the
boundary checks/setters (setIsFetchingMore, setVisibleCount). Replace the inline
handler in page.tsx with the hook's onLoadMore function and return the necessary
values from the hook so ModuleIssueDetailsPage can reuse it as well; ensure the
hook API accepts parameters like programKey, moduleKey, limit, and the base
recentPullRequests to compute currentLength and determine hasMore behavior.
- Around line 39-45: The current useEffect uses a nested condition that is
confusing: it only calls setHasMorePRs when prCount <= limit then sets it to
prCount >= limit; instead simplify by, after the null check, unconditionally
setting hasMorePRs based on the retrieved count (e.g., compute prCount =
data?.getModule?.recentPullRequests?.length; if (prCount == null) return;
setHasMorePRs(prCount >= limit)); update the useEffect that uses useEffect,
prCount, setHasMorePRs, data?.getModule?.recentPullRequests?.length and limit
accordingly.
In
`@frontend/src/app/my/mentorship/programs/`[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx:
- Around line 403-467: The pagination wrapper's conditional combines redundant
checks (hasMorePRs, (issue.pullRequests || []).length > visibleCount, and
(visibleCount > limit && (issue.pullRequests || []).length > limit)); simplify
by showing the container whenever either hasMorePRs is true or the total PR
count exceeds the initial limit or current visibleCount (i.e. use: hasMorePRs ||
(issue.pullRequests || []).length > limit || (issue.pullRequests || []).length >
visibleCount), and remove the redundant total-length check from the "Show less"
button (rely solely on visibleCount > limit to render the "Show less" button).
Update references in the JSX around hasMorePRs, visibleCount, limit, and
issue.pullRequests to use the simplified conditions and ensure the inner button
checks (the "Show more" click guard) still respect hasMorePRs and fetching
state.
In `@frontend/src/components/CardDetailsPage.tsx`:
- Around line 416-441: Update the "Show less" button in CardDetailsPage.tsx to
mirror the loading/disabled UI of the "Show more" button: when isFetchingMore is
true, render a loading label (e.g., "Loading...") and apply the same disabled
state/classname logic for the onResetPullRequests button as used for
onLoadMorePullRequests so both buttons behave and appear consistently while
isFetchingMore is true.
In `@infrastructure/modules/service/tests/service.tftest.hcl`:
- Around line 122-160: Add a new test run to assert non-default
health_check_path is respected: create another run block (similar to run
"test_task_definition_has_container_health_check") that sets
var.health_check_path to a custom value (e.g., "/custom") and then assert that
local.container_definition.healthCheck.command[1] contains
":${var.container_port}${var.health_check_path}" (matching the custom path) to
ensure the path is not hardcoded to "/". Keep the checks referencing
local.container_definition.healthCheck and
var.container_port/var.health_check_path so the test verifies overrides work.
🪄 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: CHILL
Plan: Pro
Run ID: 61d7c094-d1b8-42dd-927e-787c2eb4af39
⛔ Files ignored due to path filters (5)
backend/poetry.lockis excluded by!**/*.lockfrontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlfrontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/issueQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/moduleQueries.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (40)
.github/dependabot.yml.pre-commit-config.yamlCONTRIBUTING.mdbackend/apps/github/api/internal/nodes/issue.pybackend/apps/mentorship/api/internal/nodes/module.pybackend/apps/slack/management/commands/slack_check_invite_link.pybackend/tests/unit/apps/github/api/internal/nodes/issue_test.pybackend/tests/unit/apps/slack/commands/slack_check_invite_link_test.pydocker-compose/e2e/compose.yamldocker-compose/fuzz/compose.yamlfrontend/__tests__/e2e/pages/BoardCandidates.spec.tsfrontend/__tests__/unit/components/Card.test.tsxfrontend/__tests__/unit/components/CardDetailsPage.test.tsxfrontend/__tests__/unit/components/LoadingSpinner.test.tsxfrontend/__tests__/unit/hooks/useBreadcrumbs.test.tsxfrontend/__tests__/unit/pages/Header.test.tsxfrontend/__tests__/unit/pages/ModuleDetailsPage.test.tsxfrontend/__tests__/unit/pages/ModuleIssueDetailsPage.test.tsxfrontend/__tests__/unit/pages/ProgramDetailsMentorship.test.tsxfrontend/__tests__/unit/pages/PublicMentorshipModulePage.test.tsxfrontend/__tests__/unit/utils/getIcsFileUrl.test.tsfrontend/package.jsonfrontend/src/app/api/health/route.tsfrontend/src/app/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsxfrontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsxfrontend/src/components/Card.tsxfrontend/src/components/CardDetailsPage.tsxfrontend/src/components/Header.tsxfrontend/src/components/LoadingSpinner.tsxfrontend/src/hooks/useBreadcrumbs.tsfrontend/src/server/queries/issueQueries.tsfrontend/src/server/queries/moduleQueries.tsfrontend/src/types/card.tsfrontend/src/types/mentorship.tsfrontend/src/utils/getIcsFileUrl.tsinfrastructure/live/main.tfinfrastructure/modules/alb/variables.tfinfrastructure/modules/service/main.tfinfrastructure/modules/service/tests/service.tftest.hclinfrastructure/modules/service/variables.tf
💤 Files with no reviewable changes (2)
- frontend/tests/unit/components/Card.test.tsx
- frontend/src/components/Card.tsx
Comment on lines
99
to
+104
| href={link.href || '/'} | ||
| className={cn( | ||
| 'navlink px-3 py-2 text-slate-700 hover:text-slate-800 dark:text-slate-300 dark:hover:text-slate-200', | ||
| 'navlink px-3 py-2 text-slate-700 transition-colors duration-200 hover:text-white dark:text-slate-300 dark:hover:text-blue-400', | ||
| pathname === link.href && 'font-bold text-blue-800 dark:text-white' | ||
| )} | ||
| aria-current="page" | ||
| aria-current={pathname === link.href ? 'page' : undefined} |
Contributor
There was a problem hiding this comment.
Align active-state logic with the resolved href.
Line 99 uses a fallback ('/'), but Lines 102/104 compare against raw link.href. For links without href, the rendered URL is '/' while active styling and aria-current never match.
Suggested fix
- <Link
+ {(() => {
+ const resolvedHref = link.href || '/'
+ const isActive = pathname === resolvedHref
+ return (
+ <Link
key={link.text}
- href={link.href || '/'}
+ href={resolvedHref}
className={cn(
'navlink px-3 py-2 text-slate-700 transition-colors duration-200 hover:text-white dark:text-slate-300 dark:hover:text-blue-400',
- pathname === link.href && 'font-bold text-blue-800 dark:text-white'
+ isActive && 'font-bold text-blue-800 dark:text-white'
)}
- aria-current={pathname === link.href ? 'page' : undefined}
+ aria-current={isActive ? 'page' : undefined}
>
{link.text}
</Link>
+ )
+ })()}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/Header.tsx` around lines 99 - 104, The active-state
and aria-current checks use the raw link.href while the anchor's href uses a
fallback ('/'), causing mismatches for links without href; resolve the href once
(e.g., create a resolvedHref = link.href || '/') and use resolvedHref both for
href={...} and in the comparison with pathname (and aria-current) so the active
styling and aria attributes match the rendered href; update references to
link.href in this component (where cn is called and aria-current set) to use the
resolved variable.
Comment on lines
10
to
+19
| const parseDate = (date: string): DateArray => { | ||
| // ISO date string | ||
| const d = new Date(date) | ||
| return [d.getFullYear(), d.getMonth() + 1, d.getDate()] | ||
| return [d.getUTCFullYear(), d.getUTCMonth() + 1, d.getUTCDate()] | ||
| } | ||
|
|
||
| const getEndDate = (start: DateArray, end: DateArray): DateArray => { | ||
| if (start.join('-') === end.join('-')) { | ||
| const [y, m, d] = end | ||
| const nextDay = new Date(y, m - 1, d + 1) | ||
| return [nextDay.getFullYear(), nextDay.getMonth() + 1, nextDay.getDate()] | ||
| const nextDay = new Date(Date.UTC(y, m - 1, d + 1)) | ||
| return [nextDay.getUTCFullYear(), nextDay.getUTCMonth() + 1, nextDay.getUTCDate()] |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For the npm icspackage (v3.x), when passingDateArrayvalues generated from UTC timestamps, do we needstartInputType: 'utc'andendInputType: 'utc' to prevent local-time date shifts?
💡 Result:
Yes, for the npm ics package (v3.x), when passing DateArray values generated from UTC timestamps, you need to set startInputType: 'utc' and endInputType: 'utc' to prevent local-time date shifts. The official documentation explicitly states that the default for startInputType is 'local', meaning DateArray values are interpreted as local time components unless specified otherwise. For endInputType, the default is the value of startInputType (local if unspecified). DateArray is [year, month, date, hour, minute] (1-indexed month). If generated from a UTC timestamp using local Date methods (e.g., new Date(utcTimestamp).getFullYear etc.), the array holds local time components for that UTC instant. Without 'utc', the library treats these as local time, causing it to compute the wrong UTC instant for ICS output (typically UTC by default via startOutputType/endOutputType), resulting in date/time shifts. To correctly interpret the array as UTC components (preserving the original UTC instant), set the input types to 'utc'. Examples from docs use local arrays like [2018, 5, 30, 6, 30] without inputType (implicit local). TypeScript definitions confirm optional input types as 'local' | 'utc'. The convertTimestampToArray(timestamp, inputType) utility (for number timestamps) takes an inputType parameter, supporting both timestamp and array inputs with analogous handling.
Citations:
- 1: https://www.npmjs.com/package/ics
- 2: https://github.com/adamgibbons/ics/blob/master/index.d.ts
- 3: https://www.npmjs.com/package/ics?activeTab=dependents
- 4: https://github.com/adamgibbons
Add startInputType and endInputType properties set to 'utc' in the eventAttributes object.
The UTC date extraction using getUTCFullYear(), getUTCMonth(), and getUTCDate() is correct, but the ics library interprets DateArray as local time by default. Without explicitly setting the input types to 'utc', dates will shift for users in non-UTC timezones.
Proposed fix
const eventAttributes: EventAttributes = {
start: startArray,
end: finalEndArray,
+ startInputType: 'utc',
+ endInputType: 'utc',
title: event.title,
status: 'CONFIRMED',
busyStatus: 'BUSY',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/utils/getIcsFileUrl.ts` around lines 10 - 19, The event dates
are extracted in UTC inside parseDate and getEndDate but the ics library treats
DateArray as local time unless you set input types; update the eventAttributes
object (where you build the event payload for the ics package) to include
startInputType: 'utc' and endInputType: 'utc' so the parsed DateArray from
parseDate and getEndDate is interpreted as UTC by the ics library.
Comment on lines
+18
to
+25
| healthCheck = { | ||
| command = ["CMD-SHELL", "wget --spider http://$(hostname -i):${var.container_port}${var.health_check_path} > /dev/null 2>&1"] | ||
| interval = 30 | ||
| retries = 3 | ||
| startPeriod = 100 | ||
| timeout = 5 | ||
| } | ||
| image = "${aws_ecr_repository.main.repository_url}:${var.image_tag}" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify wget availability in Dockerfiles
echo "=== Checking backend Dockerfile for wget or BusyBox ==="
cat docker/backend/Dockerfile | grep -iE '(wget|busybox|alpine|apk add|apt-get install)' || echo "No explicit wget installation found"
echo ""
echo "=== Checking frontend Dockerfile for wget or BusyBox ==="
cat docker/frontend/Dockerfile | grep -iE '(wget|busybox|alpine|apk add|apt-get install)' || echo "No explicit wget installation found"
echo ""
echo "=== Checking base images ==="
grep -E '^FROM' docker/backend/Dockerfile docker/frontend/Dockerfile 2>/dev/null || echo "Could not find FROM statements"Repository: OWASP/Nest
Length of output: 726
🌐 Web query:
Alpine Linux 3.23 wget availability default packages
💡 Result:
No, wget is not available in Alpine Linux 3.23 default packages. It is provided as a separate package in the main repository (version 1.25.0-r2), but not included in the minimal base installation (alpine-base meta-package), which consists of core components like alpine-baselayout, apk-tools, busybox, musl-utils, and openrc. Users must explicitly install it with apk add wget.
Citations:
- 1: https://pkgs.alpinelinux.org/package/v3.23/main/x86/alpine-base
- 2: https://pkgs.alpinelinux.org/package/v3.23/main/x86_64/wget
- 3: https://www.alpinelinux.org/posts/Alpine-3.23.0-released.html
- 4: https://lowendspirit.com/discussion/10647/alpine-linux-3-23-23mb-ram-71mb-disk-guide
- 5: https://www.alpinelinux.org/downloads
- 6: https://git.alpinelinux.org/aports/tree/main/alpine-base/APKBUILD
🏁 Script executed:
#!/bin/bash
# Check if there are any health check related test files or documentation
find . -type f \( -name "*.tf" -o -name "*.py" -o -name "*.js" -o -name "*.sh" \) | xargs grep -l "healthCheck\|health_check" 2>/dev/null | head -20Repository: OWASP/Nest
Length of output: 249
🏁 Script executed:
#!/bin/bash
echo "=== Backend Dockerfile (full content) ==="
cat docker/backend/Dockerfile
echo ""
echo "=== Frontend Dockerfile (full content) ==="
cat docker/frontend/DockerfileRepository: OWASP/Nest
Length of output: 7287
Health check implementation will fail—wget is not installed in the container images.
The health check command relies on wget, but neither the backend nor frontend Dockerfile explicitly installs it. Alpine Linux 3.23 does not include wget by default; it must be explicitly installed with apk add wget.
Both containers will fail health checks when ECS attempts to execute the command.
Required fix: Add apk add --no-cache wget to the apk add statements in both Dockerfiles:
- Backend: Add to the
apk --no-cache add make postgresql16-client=16.13-r0line - Frontend (runner stage): Add to the
apk update && apk upgrade --no-cachesection
Alternatively, use a built-in health check mechanism (e.g., a health check script that doesn't depend on external tools, or use curl if it's available in the base images).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infrastructure/modules/service/main.tf` around lines 18 - 25, The ECS
container healthCheck command uses wget but the images don't install it; update
the Dockerfiles to install wget so the health check works: in the backend
Dockerfile add wget to the existing apk --no-cache add make
postgresql16-client=16.13-r0 invocation (i.e., include wget in that package
list), and in the frontend Dockerfile add apk add --no-cache wget in the runner
stage where apk update && apk upgrade --no-cache is run; alternatively replace
the healthCheck command with a tool already present or a shell-native probe if
you prefer not to add wget.
arkid15r
enabled auto-merge
April 13, 2026 23:58
arkid15r
disabled auto-merge
April 13, 2026 23:58
arkid15r
merged commit Apr 13, 2026
ed241e6
into
OWASP:feature/nestbot-ai-assistant
63 of 66 checks passed
4 tasks done
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
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
Sync feature/nestbot-ai-assistant with main
Checklist
make check-testlocally: all warnings addressed, tests passed