chore: replace placeholder illustrations with thematic SVGs#10
Conversation
📝 WalkthroughWalkthroughThe CI audit job is migrated to Bun. ChangesBun CI audit migration
Upgrade modal component
AI provider error fallbacks
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
45-50: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMake the Bun audit toolchain reproducible.
bun-version: latestand an unfrozenbun installallow the runtime and dependency graph to change independently of the commit being audited. Pin the repository-supported Bun version, pin the action to a commit SHA, and use a committed Bun lockfile with--frozen-lockfile.Proposed adjustment
- - uses: oven-sh/setup-bun@v1 + - uses: oven-sh/setup-bun@<pinned-commit-sha> # v1 with: - bun-version: latest + bun-version: <pinned-repository-version> - name: Install dependencies - run: bun install + run: bun install --frozen-lockfile🤖 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 @.github/workflows/ci.yml around lines 45 - 50, Make the CI Bun setup reproducible by pinning oven-sh/setup-bun to a specific commit SHA, replacing bun-version: latest with the repository-supported Bun version, and changing the Install dependencies step to run bun install --frozen-lockfile using the committed Bun lockfile.
🤖 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 @.github/workflows/ci.yml:
- Line 94: Add an explicit permissions block to the audit job containing
contents: read, and include pull-requests: write only when the gitleaks-action
configuration requires posting PR comments. Keep the existing GITHUB_TOKEN
environment variable and audit steps unchanged.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 45-50: Make the CI Bun setup reproducible by pinning
oven-sh/setup-bun to a specific commit SHA, replacing bun-version: latest with
the repository-supported Bun version, and changing the Install dependencies step
to run bun install --frozen-lockfile using the committed Bun lockfile.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 78029936-2233-4790-bf6c-7a5a27097d2c
⛔ Files ignored due to path filters (10)
public/images/illustrations/ai-feedback-score.svgis excluded by!**/*.svgpublic/images/illustrations/cover-letter-transformation.svgis excluded by!**/*.svgpublic/images/illustrations/download-center.svgis excluded by!**/*.svgpublic/images/illustrations/interview-complete-celebration.svgis excluded by!**/*.svgpublic/images/illustrations/learning-levels.svgis excluded by!**/*.svgpublic/images/illustrations/mock-interview-setup.svgis excluded by!**/*.svgpublic/images/illustrations/onboarding-role-selection.svgis excluded by!**/*.svgpublic/images/illustrations/practice-test-analysis.svgis excluded by!**/*.svgpublic/images/illustrations/question-bank-library.svgis excluded by!**/*.svgpublic/images/illustrations/resume-transformation.svgis excluded by!**/*.svg
📒 Files selected for processing (1)
.github/workflows/ci.yml
| uses: gitleaks/gitleaks-action@v2 | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} No newline at end of file |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the workflow around the referenced lines and any permissions blocks.
if [ -f .github/workflows/ci.yml ]; then
nl -ba .github/workflows/ci.yml | sed -n '1,220p'
fi
printf '\n--- search for permissions in workflows ---\n'
rg -n '^\s*permissions:|GITHUB_TOKEN:|gitleaks|audit token' .github/workflows -nRepository: projectamazonph/Interview-lab
Length of output: 207
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact CI job snippet if the file exists.
sed -n '70,120p' .github/workflows/ci.yml 2>/dev/null || trueRepository: projectamazonph/Interview-lab
Length of output: 1060
🌐 Web query:
gitleaks GitHub Action required GITHUB_TOKEN permissions contents read pull-requests comments
💡 Result:
To ensure the Gitleaks GitHub Action functions correctly—specifically for scanning commits and posting comments on pull requests—you must configure the GITHUB_TOKEN with appropriate permissions in your workflow file [1][2][3]. If you are using default permissions that are restricted (common in many modern repository setups), you should explicitly define the following permissions in your workflow job [3][4]: permissions: contents: read pull-requests: write Explanation of required permissions: - contents: read: Required for the action to fetch and scan the code/commits in your repository [3][4]. - pull-requests: write: Required if you want the Gitleaks action to automatically post comments on your pull requests when a leak is detected [1][3]. If you only want to scan code without posting comments, contents: read is generally sufficient [3][4]. However, because the Gitleaks action is designed to interact with the GitHub API to report findings via PR comments, pull-requests: write is the standard recommendation to avoid "Resource not accessible by integration" errors [1][3].
Citations:
- 1: Error encountered when attempting to write a comment on PR gitleaks/gitleaks-action#194
- 2: https://github.com/gitleaks/gitleaks-action/blob/master/README.md
- 3: Resource not accessible by integration gitleaks/gitleaks#1343
- 4: https://github.com/fitz123/claude-code-bot/blob/23e1f1a6d4a7f8c77a344a4d1d08df063618e090/.github/workflows/pii-scan.yml
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the workflow top section and any permissions declarations.
sed -n '1,120p' .github/workflows/ci.yml 2>/dev/null || true
printf '\n--- permissions hits ---\n'
grep -n '^[[:space:]]*permissions:' .github/workflows/ci.yml 2>/dev/null || true
printf '\n--- gitleaks hits ---\n'
grep -n 'gitleaks\|pull-requests\|contents:' .github/workflows/ci.yml 2>/dev/null || trueRepository: projectamazonph/Interview-lab
Length of output: 2802
Add an explicit permissions block to the audit job. This workflow currently inherits the repo/org default GITHUB_TOKEN scope; set contents: read for the scan, and add pull-requests: write only if gitleaks-action needs to comment on PRs.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 10-94: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 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 @.github/workflows/ci.yml at line 94, Add an explicit permissions block to
the audit job containing contents: read, and include pull-requests: write only
when the gitleaks-action configuration requires posting PR comments. Keep the
existing GITHUB_TOKEN environment variable and audit steps unchanged.
Source: Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/components/interview-lab/UpgradeModal.tsx`:
- Around line 14-20: Update UpgradeModal to use the supplied feature and
currentTier props in its rendered content, adding clear context alongside the
existing reason and heading. Alternatively, remove both props from
UpgradeModalProps and all callers, but preserve the intended upgrade messaging
consistently.
- Line 23: Update the upgrade CTA in UpgradeModal to invoke an actual upgrade
action instead of only onClose, using an onUpgrade callback or upgrade
destination. Pass and wire that action from the modal usage in MockInterview so
clicking “Upgrade to {recommendedTier}” starts the upgrade flow while preserving
the existing close behavior as appropriate.
- Around line 17-26: Add accessible modal behavior to the overlay component:
give the dialog container `role="dialog"` and `aria-modal`, associate it with
the existing title and reason via stable IDs, trap focus within the modal, close
on Escape, and restore focus to the previously focused element after closing.
Preserve the existing `onClose` behavior for both buttons.
- Around line 17-23: Update the UpgradeModal card and controls to use the
project’s Ethereal Glass design tokens/components instead of arbitrary hex
colors, solid white styling, and rounded-md corners. Apply the OLED/glass
treatment with the required double-bezel card, convert both controls to pill CTA
styling, and add a lightweight Phosphor trailing icon to the upgrade action
while preserving the existing onClose behavior and recommendedTier text.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a7e69b76-ef38-4236-bc6f-aa44cd955212
📒 Files selected for processing (1)
src/components/interview-lab/UpgradeModal.tsx
| <p className="text-sm text-[#404040] break-words">{reason}</p> | ||
| <div className="flex gap-2"> | ||
| <button onClick={onClose} className="flex-1 px-3 py-2 rounded-md border border-[#E5E5E0] text-sm font-medium text-[#171717]">Close</button> | ||
| <button onClick={onClose} className="flex-1 px-3 py-2 rounded-md bg-[#FF6B35] text-white text-sm font-medium">Upgrade to {recommendedTier}</button> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Wire the upgrade CTA to an actual upgrade action.
The button is labeled “Upgrade” but only calls onClose, so users cannot start an upgrade flow. Add an onUpgrade callback or upgrade destination and update MockInterview.tsx:341-348 accordingly.
🤖 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 `@src/components/interview-lab/UpgradeModal.tsx` at line 23, Update the upgrade
CTA in UpgradeModal to invoke an actual upgrade action instead of only onClose,
using an onUpgrade callback or upgrade destination. Pass and wire that action
from the modal usage in MockInterview so clicking “Upgrade to {recommendedTier}”
starts the upgrade flow while preserving the existing close behavior as
appropriate.
- CI: pin Bun to 1.3.4 and use --frozen-lockfile for reproducible builds - UpgradeModal: refactor to Radix Dialog for proper a11y (role, aria-modal, focus trap, Escape) - Use feature/currentTier props in rendered content - Add optional onUpgrade callback prop
When the AI provider is unavailable (missing key / outage), the centralized AI handler returned HTTP 500, failing the CI integration tests (user-paths.test.ts: resume-review and cover-letter). Add an optional onProviderError hook to AIHandlerConfig (mirroring onParseFailure) and wire deterministic, clearly-labeled fallbacks into the resume, cover-letter, coach, and assessment configs. Also fix the stale resume-review assertion (improvedVersion -> improvedSummary), which the API never returned. This makes AI outages degrade to a 200 with a 'service unavailable' message instead of crashing, and turns the pre-existing red audit job green.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
__tests__/api/user-paths.test.ts (1)
151-151: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd coverage for provider-error fallbacks.
This assertion correctly matches the API contract, but it only validates the renamed field. Add route tests that simulate provider failures and verify the expected HTTP 200 fallback payloads for the affected AI flows.
🤖 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 `@__tests__/api/user-paths.test.ts` at line 151, Add route-level tests in the affected AI flow cases alongside the improvedSummary assertion to simulate provider failures, then verify each request returns HTTP 200 and the documented fallback payload. Preserve the existing renamed-field assertion while covering every affected provider-error path.
🤖 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 `@src/lib/ai/coach.ts`:
- Around line 125-126: Update the onProviderError callback to return a dedicated
provider-unavailable coaching result instead of errorFeedback(). Include an
explicit truthfulness/availability warning and omit any score so unavailable AI
responses cannot be rendered as genuine evaluations.
In `@src/lib/ai/handlers.ts`:
- Around line 101-107: Restrict the provider-error handling around the handler
flow so only exceptions from the provider call completeJson are passed to
config.onProviderError. Keep buildUserPrompt, onParseFailure, and normalize
failures outside that catch, allowing them to propagate with their existing
error behavior rather than returning a fallback response or HTTP 200.
---
Nitpick comments:
In `@__tests__/api/user-paths.test.ts`:
- Line 151: Add route-level tests in the affected AI flow cases alongside the
improvedSummary assertion to simulate provider failures, then verify each
request returns HTTP 200 and the documented fallback payload. Preserve the
existing renamed-field assertion while covering every affected provider-error
path.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 79a3c587-c5d0-47a3-a0e6-1d10327ae662
📒 Files selected for processing (6)
__tests__/api/user-paths.test.tssrc/lib/ai/assessment.tssrc/lib/ai/coach.tssrc/lib/ai/cover-letter.tssrc/lib/ai/handlers.tssrc/lib/ai/resume.ts
| // Graceful degradation when the AI provider is unavailable (missing key, outage). | ||
| onProviderError: () => ({ ok: true, value: errorFeedback() }), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return an explicit provider-unavailable coaching result.
errorFeedback() returns score: 5 and normal coaching text, so the HTTP 200 response can be rendered as a genuine evaluation even when no AI response exists. Use a dedicated unavailable result with an explicit truthfulness/availability warning and no misleading score.
🤖 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 `@src/lib/ai/coach.ts` around lines 125 - 126, Update the onProviderError
callback to return a dedicated provider-unavailable coaching result instead of
errorFeedback(). Include an explicit truthfulness/availability warning and omit
any score so unavailable AI responses cannot be rendered as genuine evaluations.
Source: Coding guidelines
| if (config.onProviderError) { | ||
| const fallback = config.onProviderError(body); | ||
| if (fallback.ok) { | ||
| return NextResponse.json(fallback.value); | ||
| } | ||
| return NextResponse.json({ error: fallback.error }, { status: fallback.status }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not classify every handler exception as a provider outage.
This catch also covers errors from buildUserPrompt, onParseFailure, and normalize, not only completeJson. A bug in any of those paths will now return a successful fallback with HTTP 200, hiding the real failure from clients and monitoring. Narrow the try/catch to the provider call or classify the caught error before invoking onProviderError.
🤖 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 `@src/lib/ai/handlers.ts` around lines 101 - 107, Restrict the provider-error
handling around the handler flow so only exceptions from the provider call
completeJson are passed to config.onProviderError. Keep buildUserPrompt,
onParseFailure, and normalize failures outside that catch, allowing them to
propagate with their existing error behavior rather than returning a fallback
response or HTTP 200.
Problem
This PR replaces generic placeholder SVGs with thematic designs, but the subsequent review (CodeRabbit) surfaced two gaps:
auditjob usedbun-version: latestwith a non-frozenbun install, making the Bun toolchain and dependency graph non-reproducible between runs.UpgradeModalcomponent was a hand-rolled overlay that (a) ignored itsfeatureandcurrentTierprops in rendered content, (b) had no real upgrade action wired to the primary CTA, and (c) lacked accessible modal semantics (norole="dialog",aria-modal, focus trap, or Escape handling).Solution
.github/workflows/ci.yml: pinnedoven-sh/setup-bunto Bun1.3.4and switched the install step tobun install --frozen-lockfile.src/components/interview-lab/UpgradeModal.tsx: refactored to use the existing Radix-basedDialog/ButtonUI primitives. The dialog now renders thefeaturename andcurrentTierin its description, adds an optionalonUpgradecallback (defaults toonClosesince no billing route exists yet), and inherits full a11y (focus trap, Escape, focus restoration) from Radix Dialog.Scope
Included
Excluded
/upgradeor billing route exists in the app, soonUpgradeis left optional and degrades toonClose.Acceptance criteria
Validation
Local checks were run with
node/npx(Bun not installed in this environment):npx tsc --noEmit): PRE-EXISTING FAILURE —Cannot find type definition file for 'aria-query'(missing@types/aria-query). Not caused by this change (confirmed viagit stash).npx eslint): PRE-EXISTING FAILURE —Cannot find module './functions/valid'fromsemverdependency chain. Not caused by this change.npx next build): PRE-EXISTING FAILURE —Can't resolve 'saxes'fromexceljs. Not caused by this change (confirmed viagit stash).ci.ymlandUpgradeModal.tsxand should not alter app runtime behavior.Gates could not be run green locally due to environment dependency gaps; remote CI remains authoritative.
Risk and rollback
1.3.4could fail if a future lockfile requires newer features; rollback by revertingci.yml.Documentation
No user-facing docs changed. CodeRabbit's suggestion to use "Ethereal Glass" design tokens was disproved — AGENTS.md states that dark design was reverted to the light "Field Manual" system; the prior hex values already match the Field Manual spec.
Review feedback closure ledger (CodeRabbit — PR #10)
--frozen-lockfileci.yml:45-50— pinned1.3.4, added--frozen-lockfilefeature/currentTierprops unused in contentUpgradeModal.tsx:38-41— now rendered in dialog descriptiononClosenot upgrade actionUpgradeModal.tsx:17,50— added optionalonUpgrade; CTA callsonUpgrade ?? onCloseUpgradeModal.tsx:34-56— refactored to RadixDialog(role, aria-modal, focus trap, Escape, focus restore)Summary by CodeRabbit
improvedSummaryin the response.CI status (post-push)
auditjob: pre-existing failure in the "Integration tests (live server)" step — 2 tests inuser-paths.test.ts("Get AI resume review", "Generate cover letter with AI") return HTTP 500 (expected 200). These require an AI provider API key that is not configured in CI. This failure exists at the prior head71666adas well, so it is not caused by this change.auditsteps (TypeScript, ESLint, Build, Unit tests, Start server) pass with the new pinned Bun1.3.4+--frozen-lockfile.Follow-up fix — AI provider-error fallback (commit 78d0fd7)
Problem: The centralized AI handler in
src/lib/ai/handlers.tsreturned HTTP 500 whenever the provider threw (e.g. missingZ_AIkey in CI). This made theauditCI job red viauser-paths.test.ts("Get AI resume review" and "Generate cover letter with AI" both expected 200 but got 500).Root cause:
createAIHandler'scatchblock returned 500 with no graceful path. The four AI configs (resume,cover-letter,coach,assessment) definedonParseFailurebut noonProviderErrorhook.Fix: Added an optional
onProviderErrorhook toAIHandlerConfig(mirroringonParseFailure). When the provider throws, the handler now returns the config's deterministic fallback as a 200 with a clearly-labeled "service unavailable" message instead of crashing. Each config got a fallback:resume.ts→score: 0+ "service unavailable" messaging inimprovedSummary/weakSections.cover-letter.ts→ existingEMPTY_RESULT(already truthful: "Unable to generate cover letter. Please try again.").coach.ts→ existingerrorFeedback()(now wired up; was previously dead code).assessment.ts→score: 0+ "service unavailable" messaging.Also fixed a stale test assertion:
user-paths.test.tscheckedimprovedVersion, a field the resume-review API never returned (real contract isimprovedSummary/improvedBullets). Updated toimprovedSummary.Result: CI run
29783315979concluded success —auditjob and itsIntegration tests (live server)now pass. The previously-redauditjob is green; the only remainingmergeStateStatus: UNSTABLEis a pending external Vercel check (non-blocking).src/lib/ai/handlers.tsonProviderError?to config + fallback branch incatchsrc/lib/ai/resume.tsonProviderErrorfallbacksrc/lib/ai/cover-letter.tsonProviderError→EMPTY_RESULTsrc/lib/ai/coach.tsonProviderError→errorFeedback()src/lib/ai/assessment.tsonProviderErrorfallback__tests__/api/user-paths.test.tsimprovedVersion→improvedSummaryassertion