Add interviewer question templates and answer keys - #16
Add interviewer question templates and answer keys#16AndroidDevAccount wants to merge 46 commits into
Conversation
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughAdds authenticated interview question sets, interviewer template and answer-key controls, admin account and session management, dashboard navigation, candidate-session recovery, and draggable activity-indicator positioning. ChangesInterview workspace and administration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant TemplatesAPI
participant JWT
participant Firebase
participant Firepad
participant ACE
participant AnswerKey
Admin->>TemplatesAPI: request templates with bearer token
TemplatesAPI->>JWT: verify admin token
TemplatesAPI->>Firebase: read custom question sets
TemplatesAPI-->>Firepad: return templates and question sets
Firepad->>ACE: replace editor content
Firepad->>AnswerKey: populate and display answer key
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
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 `@app.html`:
- Around line 735-738: Remove the unconditional interview-templates.js loading
from app.html and move InterviewTemplates, including answerKey data and
interviewer-only plans, behind a server-side authenticated endpoint or Firebase
Security Rules based on verified interviewer claims. Update the interviewer
initialization flow to fetch these templates only after server authorization
succeeds, and ensure candidate sessions never receive or expose the protected
data; client-side isAdmin checks must remain UI-only.
- Around line 561-571: Add an inline default-hidden style to the
template-selector select element, matching the existing answer-key-btn guard, so
it remains hidden until setupSettingsSync confirms an admin user. Preserve the
current JavaScript visibility behavior for authorized users.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9fe27cc9-1022-4938-9339-e106e25ad26f
📒 Files selected for processing (4)
app.htmlscripts/firepad.jsscripts/interview-templates.jsstyles/main.css
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
scripts/firepad.js (2)
575-582: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winValidate
value.idbefore using it as an identity/key.
idis copied verbatim from the imported file. A non-string (or whitespace)idproduces keys likecustom:[object Object]:0and breaks the dedupe filter at Line 675, so re-importing the same file accumulates duplicate sets.♻️ Proposed normalization
+ const importedId = typeof value.id === 'string' && value.id.trim() + ? value.id.trim().slice(0, 80) + : `set-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; return { - id: value.id || `set-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`, + id: importedId, name: value.name.trim().slice(0, 120),🤖 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 `@scripts/firepad.js` around lines 575 - 582, Normalize and validate value.id before assigning it in the set object: accept only a non-whitespace string, trim it, and otherwise generate the existing fallback identifier. Ensure the resulting id is always a stable string so the dedupe logic remains effective across repeated imports.
720-724: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueToggle reads the inline style rather than the effective visibility.
answerKeyPanel.style.displayis''untilopenAnswerKeyPanel/closeAnswerKeyPanelwrites it, so this comparison depends on the panel having been opened at least once (currently guaranteed only because the button is disabled until a template loads). Any future path that hides the panel via CSS/class instead of an inline style silently inverts the toggle.♻️ More robust visibility check
- if (answerKeyPanel.style.display === 'none') { + const isHidden = getComputedStyle(answerKeyPanel).display === 'none'; + if (isHidden) { openAnswerKeyPanel(template); } else { closeAnswerKeyPanel(); }🤖 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 `@scripts/firepad.js` around lines 720 - 724, Update the toggle logic around answerKeyPanel to determine visibility from its effective rendered state rather than answerKeyPanel.style.display, such as via the established DOM visibility mechanism. Preserve opening when the panel is hidden and closing when it is visible, including before any inline style has been assigned or when CSS/classes control visibility.
🤖 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 `@scripts/activity-monitor.js`:
- Around line 555-561: Update the saved-position restoration in the activity
indicator initialization to clamp the restored left/top coordinates to the
current viewport after the indicator’s final content has rendered. Add a resize
handler that re-applies the same bounds calculation so the indicator remains
reachable when the viewport changes.
In `@scripts/app.js`:
- Around line 379-384: Update the Auth.logout flow to remove the localStorage
key recent_admin_session when the admin session ends, ensuring no persisted
interview join code remains available to a later user. Keep rememberAdminSession
unchanged and perform the cleanup alongside the existing logout state reset.
In `@scripts/auth-api.js`:
- Around line 45-48: Update the candidate_session restoration logic around
JSON.parse so it ignores all stored role fields and reconstructs a normalized
candidate-only session with isAdmin forced to false before assigning session.
Preserve the existing handling for invalid or missing stored data, and ensure
Auth.isAdmin() cannot return a forged interviewer role.
- Around line 42-52: Update the auth_session restoration flow so it returns only
when the stored admin session is valid and successfully restored; when it is
expired, removed, or otherwise invalid, continue into the candidate_session
lookup instead of returning early. Preserve the existing candidate session
parsing and cleanup behavior.
In `@scripts/firepad.js`:
- Around line 629-659: Extract the template fetch and related initialization
from setupSettingsSync into an async bootstrapTemplates function, and invoke it
without awaiting so sync wiring proceeds immediately. Remove async from
setupSettingsSync, register the theme selector listener and
settingsRef.on('value') subscription synchronously, and move import, answer-key,
and auto-load handlers into bootstrapTemplates().then(...). Ensure template-load
failure only disables template UI and does not return from or prevent settings
synchronization.
- Around line 695-713: Update loadInterviewTemplate to confirm before calling
editor.setValue when the editor contains non-default content, while preserving
the currentSessionIsNew auto-load path without prompting. Pass { skipConfirm:
true } from the auto-load invocation near the existing template-loading call,
and cancel the template load if the user declines.
In `@styles/main.css`:
- Around line 877-915: Update `#firepad-container.with-answer-key` to reserve the
answer key panel’s effective width, including the 420px minimum, instead of
always using 46%. Add a narrow-screen media query that switches both
`#answer-key-panel` and the editor reservation to a compatible full-width or
stacked layout so the panel never overlays editor content.
---
Nitpick comments:
In `@scripts/firepad.js`:
- Around line 575-582: Normalize and validate value.id before assigning it in
the set object: accept only a non-whitespace string, trim it, and otherwise
generate the existing fallback identifier. Ensure the resulting id is always a
stable string so the dedupe logic remains effective across repeated imports.
- Around line 720-724: Update the toggle logic around answerKeyPanel to
determine visibility from its effective rendered state rather than
answerKeyPanel.style.display, such as via the established DOM visibility
mechanism. Preserve opening when the panel is hidden and closing when it is
visible, including before any inline style has been assigned or when CSS/classes
control visibility.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 83a2b367-10c3-4140-acbb-2486253b7760
📒 Files selected for processing (12)
README.mdapi/interview/templates-data.jsapi/interview/templates.jsapp.htmldocs/QUESTION_SETS.mddocs/question-set-example.jsonscripts/activity-monitor.jsscripts/app.jsscripts/auth-api.jsscripts/firepad.jsscripts/interview-notes.jsstyles/main.css
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/firepad.js (1)
796-842: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winInconsistent null guards inside the change/click handlers.
answerKeyButton(lines 806, 818),questionSetFileInput(lines 799-800), andanswerKeyPanel(line 837) are dereferenced unguarded here, while the surrounding code (lines 733, 770, 832) treats each as optional. If any of those elements is missing the handler throws mid-way, leavingsettingsRefpartially updated.🛡️ Proposed guards
templateSelector.addEventListener('change', function() { if (this.value === '__add_custom__') { this.value = activeInterviewTemplateKey || ''; - questionSetFileInput.value = ''; - questionSetFileInput.click(); + if (questionSetFileInput) { + questionSetFileInput.value = ''; + questionSetFileInput.click(); + } return; } if (this.value === '__change_set__') { activeQuestionSetKey = null; activeInterviewTemplateKey = null; - answerKeyButton.disabled = true; + if (answerKeyButton) answerKeyButton.disabled = true;and at line 837:
- if (getComputedStyle(answerKeyPanel).display === 'none') { + if (!answerKeyPanel || getComputedStyle(answerKeyPanel).display === 'none') {🤖 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 `@scripts/firepad.js` around lines 796 - 842, Make the templateSelector change handler safe when optional elements are absent by guarding questionSetFileInput before clearing or clicking it, and guarding answerKeyButton before assigning disabled in both change branches. In the answerKeyButton click handler, also guard answerKeyPanel before calling getComputedStyle or toggling the panel. Preserve the existing settings updates and selection behavior when these elements are present.
🧹 Nitpick comments (5)
styles/main.css (1)
2395-2403: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
.dashboard-pagerelies on JS supplyingdisplay: flex.
align-items/justify-contentonly take effect if the element is a flex container, but the rule sets nodisplay; the markup starts atdisplay: noneandscripts/firepad.js(line 944) assigns'flex'. Any other show path that assigns'block'loses the centering. Safer to declare the flex behaviour in CSS and toggle a class ordisplay: noneonly.🤖 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 `@styles/main.css` around lines 2395 - 2403, Update the .dashboard-page rule to declare its intended flex layout in CSS instead of relying on scripts/firepad.js to assign display:flex. Preserve hidden-state toggling, but ensure every visible show path retains flex behavior rather than allowing display:block to override the centering alignment.scripts/firepad.js (2)
466-481: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument-level click listener is never removed.
It is bound once via
dataset.menuBound, butresetCollaborativeSessiondoesn't clear that flag or detach the listener, so the closure overusersListoutlives the session. Harmless today since the node id is stable; worth tidying if session teardown ever re-renders the top bar.🤖 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 `@scripts/firepad.js` around lines 466 - 481, Update the collaborative session teardown, specifically resetCollaborativeSession, to retain the document-level click handler registered by the user-count menu and remove it during reset. Also clear userCountButton.dataset.menuBound so a subsequent session can bind the handler again, while preserving the existing usersList toggle behavior.
705-731: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBootstrap can pop a
confirm()during page load.When a session has a saved
questionSetbut no validactiveQuestionTemplate, line 721 auto-loads the first question withskipConfirm: currentSessionIsNew; on an interviewer refresh of an in-progress session that surfaces a blocking "replace the current code for everyone" dialog before the workspace has settled. Consider skipping the auto-load entirely when the editor already holds candidate content and just rendering the picker.🤖 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 `@scripts/firepad.js` around lines 705 - 731, Update the initialization flow around loadInterviewTemplate in the saved-question-set fallback so it does not auto-load the first question when the editor already contains candidate content. In that case, skip the confirm-triggering load and render the question-set picker instead; preserve the existing first-question auto-load for empty editors and keep normal rendering behavior unchanged.api/interview/templates.js (1)
74-116: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNo delete/replace path for custom sets.
Each import creates a fresh
custom-*id, so re-importing a corrected file leaves the stale set permanently in the dropdown. A DELETE method (or upsert by client-suppliedid) would keep the picker usable. Notescripts/firepad.jsalready computes a stableidfor imported sets and sends it, but this handler ignores it.🤖 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 `@api/interview/templates.js` around lines 74 - 116, The POST handler in the custom question-set flow ignores the stable client-supplied id, causing corrected imports to create stale duplicate sets. Read and validate the imported set id from the request body, reuse it when constructing the Firebase path (with the existing custom-id format/validation), and preserve generated-id behavior when no valid id is supplied so re-imports upsert the existing set.api/auth/login.js (1)
10-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the Firebase bootstrap +
emailKeyinto a shared module.The same init block and
emailKeyimplementation now exist verbatim inapi/admins.js(lines 7-20) and the init block again inapi/interview/templates.js(lines 7-16). A divergence in the key encoding would silently break login for accounts created byapi/admins.js.🤖 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 `@api/auth/login.js` around lines 10 - 23, Extract the Firebase initialization block and emailKey function from login.js into a shared module, then update login.js, admins.js, and interview/templates.js to import and reuse those shared symbols. Remove the duplicated implementations while preserving the existing environment-based initialization and base64url email-key behavior.
🤖 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 `@api/admins.js`:
- Around line 22-32: Restrict admin-account creation in the POST handler to the
configured owner by requiring the authenticated token’s email to match
process.env.ADMIN_EMAIL, rather than allowing every token accepted by
requireAdmin. Preserve the existing admin authentication and disabled-account
behavior, and ensure unauthorized non-owner admins receive the appropriate
forbidden response.
- Around line 51-63: Update the GET handler’s additionalAdmins projection to
include each account’s disabled status in the returned admin objects, preserving
the existing owner and other account fields so operators can distinguish
disabled accounts.
In `@api/auth/login.js`:
- Around line 75-89: Guard the ownerLogin calculation so it is true only when
ADMIN_EMAIL is configured and the normalized request email matches it; do not
compare against String(undefined). Update the password-selection flow around
ownerLogin while preserving the Firebase account lookup for requests when
ADMIN_EMAIL is unset or does not match.
In `@api/interview/templates.js`:
- Around line 118-140: The GET handler currently loads and inlines every custom
question set and question body into each response. Update the customQuestionSets
retrieval and response construction to return lightweight set metadata by
default, and include question content and answerKey only for the requested setId
when one is provided; preserve the existing templates/questionSets shape for
built-in data and ensure the selected custom set still exposes its question keys
and bodies.
In `@docs/QUESTION_SETS.md`:
- Around line 3-6: Update the documentation describing the Custom Questions
group to label the file-loading option as “Load custom questions from file…” so
it matches the UI rendered by firepad.js. Keep the surrounding grouping and
import behavior description unchanged.
In `@scripts/app.js`:
- Around line 1840-1844: Reset the sessionStarting flag in the
main-container-missing branch before the early return, so subsequent
startSession() calls can run after the error. Update the session-start flow
around the shown console.error, alert, and return without changing the existing
error messaging.
- Around line 1832-1857: In the main-container setup, capture the existing
dataset.sessionCode before assigning the new sessionCode, then use that captured
previous value for the mismatch check. Remove the redundant re-query or ensure
it does not occur after overwriting the dataset, preserving the existing reload
and resetCollaborativeSession behavior when switching sessions.
---
Outside diff comments:
In `@scripts/firepad.js`:
- Around line 796-842: Make the templateSelector change handler safe when
optional elements are absent by guarding questionSetFileInput before clearing or
clicking it, and guarding answerKeyButton before assigning disabled in both
change branches. In the answerKeyButton click handler, also guard answerKeyPanel
before calling getComputedStyle or toggling the panel. Preserve the existing
settings updates and selection behavior when these elements are present.
---
Nitpick comments:
In `@api/auth/login.js`:
- Around line 10-23: Extract the Firebase initialization block and emailKey
function from login.js into a shared module, then update login.js, admins.js,
and interview/templates.js to import and reuse those shared symbols. Remove the
duplicated implementations while preserving the existing environment-based
initialization and base64url email-key behavior.
In `@api/interview/templates.js`:
- Around line 74-116: The POST handler in the custom question-set flow ignores
the stable client-supplied id, causing corrected imports to create stale
duplicate sets. Read and validate the imported set id from the request body,
reuse it when constructing the Firebase path (with the existing custom-id
format/validation), and preserve generated-id behavior when no valid id is
supplied so re-imports upsert the existing set.
In `@scripts/firepad.js`:
- Around line 466-481: Update the collaborative session teardown, specifically
resetCollaborativeSession, to retain the document-level click handler registered
by the user-count menu and remove it during reset. Also clear
userCountButton.dataset.menuBound so a subsequent session can bind the handler
again, while preserving the existing usersList toggle behavior.
- Around line 705-731: Update the initialization flow around
loadInterviewTemplate in the saved-question-set fallback so it does not
auto-load the first question when the editor already contains candidate content.
In that case, skip the confirm-triggering load and render the question-set
picker instead; preserve the existing first-question auto-load for empty editors
and keep normal rendering behavior unchanged.
In `@styles/main.css`:
- Around line 2395-2403: Update the .dashboard-page rule to declare its intended
flex layout in CSS instead of relying on scripts/firepad.js to assign
display:flex. Preserve hidden-state toggling, but ensure every visible show path
retains flex behavior rather than allowing display:block to override the
centering alignment.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a504216-46bf-40be-b539-a40a26394704
📒 Files selected for processing (11)
api/admins.jsapi/auth/login.jsapi/interview/templates-data.jsapi/interview/templates.jsapp.htmldocs/QUESTION_SETS.mdscripts/activity-monitor.jsscripts/app.jsscripts/auth-api.jsscripts/firepad.jsstyles/main.css
🚧 Files skipped from review as they are similar to previous changes (3)
- api/interview/templates-data.js
- scripts/activity-monitor.js
- scripts/auth-api.js
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/admins.js (2)
60-62: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReuse one normalized owner email everywhere.
At Lines 66-67 the owner email is trimmed and lowercased, but the GET owner entry and duplicate-owner check use an untrimmed value. If
ADMIN_EMAILcontains surrounding whitespace, the API can display a mismatched owner and allow creation for the configured owner address. Compute the normalized value once and reuse it in both branches.Also applies to: 80-81
🤖 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 `@api/admins.js` around lines 60 - 62, Normalize process.env.ADMIN_EMAIL once by trimming and lowercasing it, then reuse that normalized value when constructing the GET owner entry and performing the duplicate-owner check. Update the relevant owner handling in the surrounding admin flow, including the alternate branch at lines 80-81, so display and comparison use the same normalized email.
84-95: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake admin creation atomic.
The
once('value')check and laterset()are separate writes, so concurrent owner requests for the same email can both see no account, both return201, and the last write can overwrite the password hash. Use a transaction/conditional create onadminAccounts/<emailKey>and return409when the reservation fails.🤖 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 `@api/admins.js` around lines 84 - 95, Replace the separate existence check and set in the admin creation flow with an atomic transaction or conditional create on accountRef for adminAccounts/<emailKey>. Ensure only one concurrent request reserves the account, return 409 when the reservation fails, and preserve the existing account fields and successful creation response.
🤖 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.
Outside diff comments:
In `@api/admins.js`:
- Around line 60-62: Normalize process.env.ADMIN_EMAIL once by trimming and
lowercasing it, then reuse that normalized value when constructing the GET owner
entry and performing the duplicate-owner check. Update the relevant owner
handling in the surrounding admin flow, including the alternate branch at lines
80-81, so display and comparison use the same normalized email.
- Around line 84-95: Replace the separate existence check and set in the admin
creation flow with an atomic transaction or conditional create on accountRef for
adminAccounts/<emailKey>. Ensure only one concurrent request reserves the
account, return 409 when the reservation fails, and preserve the existing
account fields and successful creation response.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4c4e523b-ce0c-48bb-a05c-7881ce654aca
📒 Files selected for processing (8)
api/admins.jsapi/auth/login.jsapi/interview/templates.jsapp.htmldocs/QUESTION_SETS.mdscripts/app.jsscripts/firepad.jsstyles/main.css
🚧 Files skipped from review as they are similar to previous changes (7)
- docs/QUESTION_SETS.md
- styles/main.css
- api/auth/login.js
- app.html
- scripts/firepad.js
- scripts/app.js
- api/interview/templates.js
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
Validation
Summary by CodeRabbit