feat: implement preview read-only mode with upsell guidance and error handling#38
Merged
Conversation
railway-app
Bot
temporarily deployed
to
applirank / applirank-pr-38
|
🚅 Deployed to the applirank-pr-38 environment in applirank
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a preview read‑only flow: new composable and upsell modal; augments many mutating API paths and UI components to detect PREVIEW_READ_ONLY errors and short‑circuit; hardens server demo‑guard to resolve multiple slugs and fail loudly when misconfigured; updates runtime config, .env.example, seed data, and changelog. Changes
Sequence DiagramsequenceDiagram
autonumber
actor User
participant UI as Vue Component
participant Preview as withPreviewReadOnly / try/catch
participant Composable as usePreviewReadOnly
participant Server as API Server
participant Modal as PreviewUpsellModal
User->>UI: Trigger mutation (create/update/delete)
UI->>Preview: Execute wrapped action or call API in try/catch
Preview->>Server: Perform mutation (POST/PATCH/DELETE)
alt Server returns PREVIEW_READ_ONLY error
Server-->>Preview: PREVIEW_READ_ONLY error
Preview->>Composable: handlePreviewReadOnlyError(error)
Composable->>Modal: openUpsell(message)
Modal-->>User: Show upsell UI
Preview-->>UI: Rethrow / short-circuit
UI-->>User: Skip default error UI
else Success
Server-->>Preview: Success response
Preview-->>UI: Return result
UI-->>User: Update UI/state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
railway-app
Bot
temporarily deployed
to
applirank / applirank-pr-38
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/components/CandidateDetailSidebar.vue (2)
203-212:⚠️ Potential issue | 🟡 Minor
handleFileSelectedshows both the upsell modal AND the inline error banner for PREVIEW_READ_ONLY upload errors
uploadDocumentinternally wraps its$fetchinwithPreviewReadOnly, so a PREVIEW_READ_ONLY error will already open the upsell modal before propagating. The current catch block then setsuploadError.value(usingerr.statusMessagewhich is the preview-read-only message), producing dual UX feedback — the modal and the inline error strip both appear simultaneously.Add a
handlePreviewReadOnlyErrorshort-circuit to match the pattern used inhandleTransitionandsaveNotes:🔧 Proposed fix
} catch (err: any) { + if (handlePreviewReadOnlyError(err)) return uploadError.value = err.data?.statusMessage ?? err.statusMessage ?? 'Upload failed' } finally {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/CandidateDetailSidebar.vue` around lines 203 - 212, handleFileSelected is setting uploadError.value for all errors, causing both the PREVIEW_READ_ONLY upsell modal (thrown by uploadDocument/withPreviewReadOnly) and the inline error banner to appear; update the catch block to short-circuit PREVIEW_READ_ONLY handling by calling the existing helper handlePreviewReadOnlyError(err) and returning early when it handles the error, otherwise set uploadError.value as before; keep the finally block to reset isUploading.value and input.value.
251-263:⚠️ Potential issue | 🟠 MajorRedundant double-wrap:
deleteDocumentalready useswithPreviewReadOnlyinternally
deleteDocument(inuseDocuments.tsline 75) already wraps its$fetchinwithPreviewReadOnly, which opens the upsell and rethrows. Wrapping thedeleteDocumentcall here in yet anotherwithPreviewReadOnlycauseshandlePreviewReadOnlyErrorto be invoked three times for every PREVIEW_READ_ONLY error (once insideuseDocuments, once in this outer wrapper, once in the catch). While the repeatedopenUpsellcalls are idempotent viauseState, this is a design inconsistency and a maintenance trap.Drop the outer
withPreviewReadOnlyand calldeleteDocumentdirectly — the existinghandlePreviewReadOnlyErrorcheck in the catch is sufficient to suppress the fallbackalert():🔧 Proposed fix
try { - await withPreviewReadOnly(() => deleteDocument(docId, candidateId.value!)) + await deleteDocument(docId, candidateId.value!) await refreshCandidate() showDocDeleteConfirm.value = null } catch (err: any) { if (handlePreviewReadOnlyError(err)) return alert(err.data?.statusMessage ?? 'Failed to delete document') } finally { isDeletingDoc.value = false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/CandidateDetailSidebar.vue` around lines 251 - 263, The current handleDeleteDoc function double-wraps deleteDocument with withPreviewReadOnly causing duplicate preview-read-only handling; remove the outer withPreviewReadOnly call and invoke deleteDocument(docId, candidateId.value!) directly inside the try block, keeping the existing catch that calls handlePreviewReadOnlyError(err) and alerts on other errors so preview errors are still suppressed and refreshCandidate/showDocDeleteConfirm logic remains unchanged (references: handleDeleteDoc, deleteDocument, withPreviewReadOnly, handlePreviewReadOnlyError, refreshCandidate).
🧹 Nitpick comments (8)
app/composables/usePreviewReadOnly.ts (2)
25-28: Use??instead of||for the message fallback.
nextMessage || DEFAULT_PREVIEW_MESSAGEtreats an empty string as falsy and silently substitutes the default.??is the correct null-coalescing operator here.♻️ Proposed fix
- message.value = nextMessage || DEFAULT_PREVIEW_MESSAGE + message.value = nextMessage ?? DEFAULT_PREVIEW_MESSAGE🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/composables/usePreviewReadOnly.ts` around lines 25 - 28, In openUpsell replace the fallback using the logical OR so an empty string isn't treated as missing: set message.value to nextMessage ?? DEFAULT_PREVIEW_MESSAGE instead of nextMessage || DEFAULT_PREVIEW_MESSAGE, keeping the rest (isUpsellOpen.value = true) unchanged; reference function openUpsell and the symbols message.value and DEFAULT_PREVIEW_MESSAGE.
41-48:withPreviewReadOnlyshould be generic to preserve callers' return types.As written, all callers receive
Promise<unknown>regardless of the$fetchreturn type. InuseApplication.ts,updateApplicationreturnsunknown; inuseJobQuestions.ts,addQuestionandupdateQuestionreturnunknown. Making the function generic restores the original inferred types:♻️ Proposed generic signature
- async function withPreviewReadOnly(action: () => Promise<unknown>) { + async function withPreviewReadOnly<T>(action: () => Promise<T>): Promise<T> { try { return await action() } catch (error) { handlePreviewReadOnlyError(error) throw error } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/composables/usePreviewReadOnly.ts` around lines 41 - 48, Change withPreviewReadOnly to be a generic wrapper so it preserves the caller's return type: declare a type parameter (e.g., T) on withPreviewReadOnly and type the action parameter as () => Promise<T> and the function return as Promise<T>; keep the same try/catch behavior and still call handlePreviewReadOnlyError(error) in the catch before rethrowing. Update usages (e.g., callers in useApplication.ts and useJobQuestions.ts) will then automatically infer their original return types without needing explicit casts.app/composables/useCandidate.ts (2)
27-38:refresh()andrefreshNuxtData()inside the try/catch widens the error boundary.Any error thrown by
refresh()orrefreshNuxtData()will pass throughhandlePreviewReadOnlyError(it returnsfalseharmlessly, then rethrows), but it's misleading. The cleaner pattern used inuseApplication.tswraps only the$fetchwithwithPreviewReadOnlyand puts the cache-invalidation calls outside:♻️ Proposed refactor
- try { - const updated = await $fetch(`/api/candidates/${candidateId.value}`, { - method: 'PATCH', - body: payload, - }) - await refresh() - await refreshNuxtData('candidates') - return updated - } catch (error) { - handlePreviewReadOnlyError(error) - throw error - } + const updated = await withPreviewReadOnly(() => + $fetch(`/api/candidates/${candidateId.value}`, { + method: 'PATCH', + body: payload, + }), + ) + await refresh() + await refreshNuxtData('candidates') + return updated(Requires also destructuring
withPreviewReadOnlyinstead ofhandlePreviewReadOnlyError.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/composables/useCandidate.ts` around lines 27 - 38, The try/catch currently wraps the $fetch and the cache invalidations (refresh and refreshNuxtData), widening the error boundary and causing preview-readonly handling to apply to refreshes; change to destructure and use withPreviewReadOnly instead of handlePreviewReadOnlyError, call withPreviewReadOnly around just the $fetch(`/api/candidates/${candidateId.value}`, { method: 'PATCH', body: payload }) operation, keep the try/catch only for that wrapped fetch, then move await refresh() and await refreshNuxtData('candidates') outside the try/catch so their errors are not routed through withPreviewReadOnly; ensure you still rethrow or return the updated result as before.
2-2: Explicit import ofusePreviewReadOnlyis redundant and inconsistent.Nuxt auto-imports all composables from
app/composables/.useApplication.tsanduseJobQuestions.tsuseusePreviewReadOnly()without an explicit import. Remove the redundant import to be consistent.♻️ Proposed fix
import type { MaybeRefOrGetter } from 'vue' -import { usePreviewReadOnly } from '~/composables/usePreviewReadOnly'Also applies to: 9-9
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/composables/useCandidate.ts` at line 2, Remove the redundant explicit import of usePreviewReadOnly from useCandidate.ts: locate the import statement "import { usePreviewReadOnly } from '~/composables/usePreviewReadOnly'" and delete it, relying on Nuxt's auto-import of composables so calls to usePreviewReadOnly() within the file remain unchanged; repeat for any other identical explicit imports (e.g., the one at line noted as 9-9) to keep import usage consistent with useApplication.ts and useJobQuestions.ts.app/components/ApplyCandidateModal.vue (2)
36-36:usePreviewReadOnly()composable init should precede thecomputed()at line 35Minor section-ordering violation — composable inits belong before
computeddeclarations.♻️ Proposed reorder
+const { withPreviewReadOnly, handlePreviewReadOnlyError } = usePreviewReadOnly() + const candidates = computed(() => candidateData.value?.data ?? []) -const { withPreviewReadOnly, handlePreviewReadOnlyError } = usePreviewReadOnly()As per coding guidelines: "Order
<script setup>sections: imports → props/emits → composables/state → computed → functions."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/ApplyCandidateModal.vue` at line 36, The usePreviewReadOnly() composable is being initialized after a computed() declaration; move the line that destructures withPreviewReadOnly and handlePreviewReadOnlyError from usePreviewReadOnly() so that the call to usePreviewReadOnly() (and its returned identifiers withPreviewReadOnly and handlePreviewReadOnlyError) appears before the computed() declarations to follow the imports → composables/state → computed ordering; ensure any computed that depends on those symbols still references them after the reorder.
36-36:usePreviewReadOnly()init placed after acomputed()callComposable inits should precede
computeddeclarations per the section-order guideline.♻️ Proposed fix
+const { withPreviewReadOnly, handlePreviewReadOnlyError } = usePreviewReadOnly() + const candidates = computed(() => candidateData.value?.data ?? []) -const { withPreviewReadOnly, handlePreviewReadOnlyError } = usePreviewReadOnly()As per coding guidelines: "Order
<script setup>sections: imports → props/emits → composables/state → computed → functions."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/ApplyCandidateModal.vue` at line 36, The call to usePreviewReadOnly() is declared after a computed(), violating the script-setup section order; move the composable initialization (const { withPreviewReadOnly, handlePreviewReadOnlyError } = usePreviewReadOnly()) to precede any computed() declarations so that composables/state come before computed in the component (look for references to usePreviewReadOnly, withPreviewReadOnly, handlePreviewReadOnlyError and the computed() where it currently appears and relocate the const destructuring above that computed()).app/layouts/dashboard.vue (1)
3-3: Redundant explicit import —usePreviewReadOnlyis already auto-imported
app/components/PreviewUpsellModal.vue(added in this same PR) callsusePreviewReadOnly()without an explicit import, confirming Nuxt's auto-import resolves it. The explicit import on line 3 creates a false impression that this composable requires manual importing and is inconsistent with the companion component.♻️ Proposed cleanup
-import { usePreviewReadOnly } from '~/composables/usePreviewReadOnly'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/layouts/dashboard.vue` at line 3, Remove the redundant explicit import of the composable usePreviewReadOnly from app/layouts/dashboard.vue; since Nuxt auto-imports this composable (as demonstrated by app/components/PreviewUpsellModal.vue calling usePreviewReadOnly() without an import), delete the import line `import { usePreviewReadOnly } from '~/composables/usePreviewReadOnly'` and rely on the auto-imported usePreviewReadOnly symbol in the template/script to keep imports consistent across components.server/middleware/demo-guard.ts (1)
27-27: Prefer!== nullover truthy check for the cached ID
demoOrgIdis typedstring | null; a truthy guard would silently skip the cache for an empty-string ID (impossible in practice but imprecise). Using!== nullbetter communicates the intent.♻️ Proposed cleanup
- if (demoOrgId) return demoOrgId + if (demoOrgId !== null) return demoOrgId🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/middleware/demo-guard.ts` at line 27, The condition currently uses a truthy check "if (demoOrgId) return demoOrgId" which can mis-handle an explicit empty string; update the guard to an explicit null check by changing the conditional to "if (demoOrgId !== null) return demoOrgId" (locate the conditional that references the demoOrgId variable in demo-guard.ts) so the cache semantics match the string | null type precisely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/ApplyCandidateModal.vue`:
- Around line 46-51: The TS2321 error comes from wrapping Nuxt's $fetch inside
withPreviewReadOnly causing excessive type-inference depth; fix by giving the
$fetch call an explicit type or by casting the URL to string to avoid route
literal inference: update the $fetch invocation inside withPreviewReadOnly (the
call that posts { candidateId, jobId: props.jobId }) to either include an
explicit generic response type for $fetch or change the first argument to
'/api/applications' as a string cast so TypeScript skips recursive route
matching.
In `@app/components/PreviewUpsellModal.vue`:
- Around line 17-64: The modal lacks ARIA roles, keyboard handling, and focus
management: add role="dialog" and aria-modal="true" to the inner modal container
element, give the <h3> a unique id and reference it via aria-labelledby on that
container, add `@keydown.esc.window`="closeModal" to capture Escape, add
aria-label="Close dialog" to the close <button>, and implement focus management
by adding a template ref (e.g., modalRef) and code that, when the modal opens,
saves document.activeElement, moves focus into the first focusable element
inside the modal, traps Tab/Shift+Tab inside the modal, and restores focus on
close using the existing closeModal method; you can implement the trap with a
small utility in setup() or use a11y helper lib, and wire the watch/onMounted
hooks to run this behavior when the modal becomes visible.
In `@app/composables/usePreviewReadOnly.ts`:
- Line 21: Add a JSDoc comment block above the exported function
usePreviewReadOnly describing its purpose, return value, and any parameters or
side effects (if applicable); update the comment to follow standard JSDoc syntax
(/** ... */) and include `@returns` with the return type/description and any
`@example` or `@remarks` if helpful, then save so the exported function has a proper
JSDoc header.
In `@server/middleware/demo-guard.ts`:
- Around line 66-77: The current code throws a 503 when guardedOrgId is falsy
(the throw block using createError), which runs before the session org check and
blocks all tenants; instead, change the behavior so that if
getDemoOrgId()/guardedOrgId returns null you do NOT throw — emit a structured
warning via the existing logger (or use console.warn if none) that DEMO_ORG_SLUG
could not be resolved and then return early (fail-open) so the guard remains
voluntary; keep the existing import.meta.dev short-circuit as-is and ensure any
later logic (the session org check) still runs for normal enforcement paths.
---
Outside diff comments:
In `@app/components/CandidateDetailSidebar.vue`:
- Around line 203-212: handleFileSelected is setting uploadError.value for all
errors, causing both the PREVIEW_READ_ONLY upsell modal (thrown by
uploadDocument/withPreviewReadOnly) and the inline error banner to appear;
update the catch block to short-circuit PREVIEW_READ_ONLY handling by calling
the existing helper handlePreviewReadOnlyError(err) and returning early when it
handles the error, otherwise set uploadError.value as before; keep the finally
block to reset isUploading.value and input.value.
- Around line 251-263: The current handleDeleteDoc function double-wraps
deleteDocument with withPreviewReadOnly causing duplicate preview-read-only
handling; remove the outer withPreviewReadOnly call and invoke
deleteDocument(docId, candidateId.value!) directly inside the try block, keeping
the existing catch that calls handlePreviewReadOnlyError(err) and alerts on
other errors so preview errors are still suppressed and
refreshCandidate/showDocDeleteConfirm logic remains unchanged (references:
handleDeleteDoc, deleteDocument, withPreviewReadOnly,
handlePreviewReadOnlyError, refreshCandidate).
---
Duplicate comments:
In `@app/composables/useApplications.ts`:
- Line 2: The import of usePreviewReadOnly in useApplications.ts is redundant
(same as in useCandidate.ts); remove the explicit "import { usePreviewReadOnly }
from '~/composables/usePreviewReadOnly'" and rely on the global/composable
auto-imports or existing import pattern used elsewhere, ensuring any references
to usePreviewReadOnly in this file remain valid without the explicit import.
- Around line 36-46: The POST call and refresh() are both inside the try/catch;
move refresh() outside and only wrap the $fetch call with withPreviewReadOnly so
preview-readonly handling only applies to the network request. Concretely, in
useApplications.ts change the block so you call withPreviewReadOnly(...) around
the $fetch('/api/applications', { method: 'POST', body: payload }) (and keep
handlePreviewReadOnlyError in the catch), then call await refresh() after the
try/catch returns the created result (matching the pattern used in
useApplication.ts and useCandidate.ts).
In `@app/composables/useCandidates.ts`:
- Line 2: The explicit import of usePreviewReadOnly in useCandidates.ts is
redundant (same as in useCandidate.ts); remove the import statement "import {
usePreviewReadOnly } from '~/composables/usePreviewReadOnly'" from
useCandidates.ts and rely on the auto-imported/composable global, ensuring any
references to usePreviewReadOnly inside functions (e.g., in the useCandidates
composable) remain unchanged and still resolve correctly.
- Around line 33-43: The issue: calling await refresh() inside the try block
causes errors from the refresh to be treated as the POST failure (same pattern
as useCandidate.ts). Fix by moving await refresh() out of the try/catch that
wraps the POST: keep the POST call and return created inside try, let the catch
only handle errors from the POST (handlePreviewReadOnlyError(error) / rethrow),
and then call await refresh() after the try/catch (or call it in a separate try
that logs/handles refresh errors) so refresh() failures aren’t misattributed to
the POST.
---
Nitpick comments:
In `@app/components/ApplyCandidateModal.vue`:
- Line 36: The usePreviewReadOnly() composable is being initialized after a
computed() declaration; move the line that destructures withPreviewReadOnly and
handlePreviewReadOnlyError from usePreviewReadOnly() so that the call to
usePreviewReadOnly() (and its returned identifiers withPreviewReadOnly and
handlePreviewReadOnlyError) appears before the computed() declarations to follow
the imports → composables/state → computed ordering; ensure any computed that
depends on those symbols still references them after the reorder.
- Line 36: The call to usePreviewReadOnly() is declared after a computed(),
violating the script-setup section order; move the composable initialization
(const { withPreviewReadOnly, handlePreviewReadOnlyError } =
usePreviewReadOnly()) to precede any computed() declarations so that
composables/state come before computed in the component (look for references to
usePreviewReadOnly, withPreviewReadOnly, handlePreviewReadOnlyError and the
computed() where it currently appears and relocate the const destructuring above
that computed()).
In `@app/composables/useCandidate.ts`:
- Around line 27-38: The try/catch currently wraps the $fetch and the cache
invalidations (refresh and refreshNuxtData), widening the error boundary and
causing preview-readonly handling to apply to refreshes; change to destructure
and use withPreviewReadOnly instead of handlePreviewReadOnlyError, call
withPreviewReadOnly around just the
$fetch(`/api/candidates/${candidateId.value}`, { method: 'PATCH', body: payload
}) operation, keep the try/catch only for that wrapped fetch, then move await
refresh() and await refreshNuxtData('candidates') outside the try/catch so their
errors are not routed through withPreviewReadOnly; ensure you still rethrow or
return the updated result as before.
- Line 2: Remove the redundant explicit import of usePreviewReadOnly from
useCandidate.ts: locate the import statement "import { usePreviewReadOnly } from
'~/composables/usePreviewReadOnly'" and delete it, relying on Nuxt's auto-import
of composables so calls to usePreviewReadOnly() within the file remain
unchanged; repeat for any other identical explicit imports (e.g., the one at
line noted as 9-9) to keep import usage consistent with useApplication.ts and
useJobQuestions.ts.
In `@app/composables/usePreviewReadOnly.ts`:
- Around line 25-28: In openUpsell replace the fallback using the logical OR so
an empty string isn't treated as missing: set message.value to nextMessage ??
DEFAULT_PREVIEW_MESSAGE instead of nextMessage || DEFAULT_PREVIEW_MESSAGE,
keeping the rest (isUpsellOpen.value = true) unchanged; reference function
openUpsell and the symbols message.value and DEFAULT_PREVIEW_MESSAGE.
- Around line 41-48: Change withPreviewReadOnly to be a generic wrapper so it
preserves the caller's return type: declare a type parameter (e.g., T) on
withPreviewReadOnly and type the action parameter as () => Promise<T> and the
function return as Promise<T>; keep the same try/catch behavior and still call
handlePreviewReadOnlyError(error) in the catch before rethrowing. Update usages
(e.g., callers in useApplication.ts and useJobQuestions.ts) will then
automatically infer their original return types without needing explicit casts.
In `@app/layouts/dashboard.vue`:
- Line 3: Remove the redundant explicit import of the composable
usePreviewReadOnly from app/layouts/dashboard.vue; since Nuxt auto-imports this
composable (as demonstrated by app/components/PreviewUpsellModal.vue calling
usePreviewReadOnly() without an import), delete the import line `import {
usePreviewReadOnly } from '~/composables/usePreviewReadOnly'` and rely on the
auto-imported usePreviewReadOnly symbol in the template/script to keep imports
consistent across components.
In `@server/middleware/demo-guard.ts`:
- Line 27: The condition currently uses a truthy check "if (demoOrgId) return
demoOrgId" which can mis-handle an explicit empty string; update the guard to an
explicit null check by changing the conditional to "if (demoOrgId !== null)
return demoOrgId" (locate the conditional that references the demoOrgId variable
in demo-guard.ts) so the cache semantics match the string | null type precisely.
Comment on lines
+17
to
+64
| <div class="fixed inset-0 z-50 flex items-center justify-center p-4"> | ||
| <div class="absolute inset-0 bg-black/50" @click="closeModal" /> | ||
|
|
||
| <div class="relative w-full max-w-md rounded-xl border border-surface-200 bg-white shadow-xl dark:border-surface-800 dark:bg-surface-900"> | ||
| <div class="flex items-center justify-between border-b border-surface-200 px-5 py-4 dark:border-surface-800"> | ||
| <div class="flex items-center gap-2"> | ||
| <Crown class="size-5 text-brand-600 dark:text-brand-400" /> | ||
| <h3 class="text-lg font-semibold text-surface-900 dark:text-surface-50">Unlock full editing</h3> | ||
| </div> | ||
|
|
||
| <button | ||
| class="cursor-pointer text-surface-400 transition-colors hover:text-surface-600 dark:hover:text-surface-200" | ||
| @click="closeModal" | ||
| > | ||
| <X class="size-5" /> | ||
| </button> | ||
| </div> | ||
|
|
||
| <div class="space-y-4 px-5 py-5"> | ||
| <p class="text-sm text-surface-600 dark:text-surface-300"> | ||
| {{ message }} | ||
| </p> | ||
|
|
||
| <p class="text-sm text-surface-500 dark:text-surface-400"> | ||
| Want write access? Upgrade to a paid hosted plan or deploy your own Applirank instance. | ||
| </p> | ||
|
|
||
| <div class="flex flex-wrap items-center gap-2"> | ||
| <a | ||
| href="mailto:sales@applirank.com?subject=Applirank%20Hosted%20Plan" | ||
| class="inline-flex items-center gap-2 rounded-lg bg-brand-600 px-3 py-2 text-sm font-medium text-white transition-colors hover:bg-brand-700" | ||
| > | ||
| <Rocket class="size-4" /> | ||
| Contact sales | ||
| </a> | ||
|
|
||
| <a | ||
| href="https://github.com/applirank/applirank" | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| class="inline-flex items-center rounded-lg border border-surface-300 px-3 py-2 text-sm font-medium text-surface-700 transition-colors hover:bg-surface-50 dark:border-surface-700 dark:text-surface-200 dark:hover:bg-surface-800" | ||
| > | ||
| Deploy your own | ||
| </a> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Non-navigable modal — missing ARIA roles, keyboard handling, and focus management
The modal is inaccessible to keyboard and screen-reader users. The following gaps all need addressing:
| Gap | Fix |
|---|---|
No role="dialog" / aria-modal="true" on the container |
Add to the inner div (line 20) |
No aria-labelledby linking the title |
Give <h3> an id, reference it |
| No Escape key listener | @keydown.esc.window="closeModal" |
| No focus trap | Tab cycles outside the modal |
| Close button has no accessible name | Add aria-label="Close dialog" |
| Focus not moved into modal on open, not restored on close | Programmatic focus management via templateRef + watch/onMounted |
WAI-ARIA Authoring Practices require: closing on ESC, toggling ARIA attributes, and trapping/restoring focus. A modal dialog should have both role="dialog" and aria-modal="true" so screen readers treat it as a modal.
♿ Proposed minimal fix
+<script setup lang="ts">
import { Crown, X, Rocket } from 'lucide-vue-next'
+import { onMounted, useTemplateRef } from 'vue'
const emit = defineEmits<{
(e: 'close'): void
}>()
const { message } = usePreviewReadOnly()
+const dialogRef = useTemplateRef<HTMLDivElement>('dialog')
function closeModal() {
emit('close')
}
+
+function onKeydown(e: KeyboardEvent) {
+ if (e.key === 'Escape') closeModal()
+}
+
+onMounted(() => {
+ dialogRef.value?.focus()
+})
+</script>
<template>
<Teleport to="body">
- <div class="fixed inset-0 z-50 flex items-center justify-center p-4">
+ <div class="fixed inset-0 z-50 flex items-center justify-center p-4" `@keydown`="onKeydown">
<div class="absolute inset-0 bg-black/50" `@click`="closeModal" />
- <div class="relative w-full max-w-md rounded-xl border border-surface-200 bg-white shadow-xl dark:border-surface-800 dark:bg-surface-900">
+ <div
+ ref="dialog"
+ role="dialog"
+ aria-modal="true"
+ aria-labelledby="upsell-title"
+ tabindex="-1"
+ class="relative w-full max-w-md rounded-xl border border-surface-200 bg-white shadow-xl dark:border-surface-800 dark:bg-surface-900 focus:outline-none"
+ >
<div class="flex items-center justify-between border-b border-surface-200 px-5 py-4 dark:border-surface-800">
<div class="flex items-center gap-2">
<Crown class="size-5 text-brand-600 dark:text-brand-400" />
- <h3 class="text-lg font-semibold text-surface-900 dark:text-surface-50">Unlock full editing</h3>
+ <h3 id="upsell-title" class="text-lg font-semibold text-surface-900 dark:text-surface-50">Unlock full editing</h3>
</div>
<button
+ aria-label="Close dialog"
class="cursor-pointer text-surface-400 transition-colors hover:text-surface-600 dark:hover:text-surface-200"
`@click`="closeModal"
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <div class="fixed inset-0 z-50 flex items-center justify-center p-4"> | |
| <div class="absolute inset-0 bg-black/50" @click="closeModal" /> | |
| <div class="relative w-full max-w-md rounded-xl border border-surface-200 bg-white shadow-xl dark:border-surface-800 dark:bg-surface-900"> | |
| <div class="flex items-center justify-between border-b border-surface-200 px-5 py-4 dark:border-surface-800"> | |
| <div class="flex items-center gap-2"> | |
| <Crown class="size-5 text-brand-600 dark:text-brand-400" /> | |
| <h3 class="text-lg font-semibold text-surface-900 dark:text-surface-50">Unlock full editing</h3> | |
| </div> | |
| <button | |
| class="cursor-pointer text-surface-400 transition-colors hover:text-surface-600 dark:hover:text-surface-200" | |
| @click="closeModal" | |
| > | |
| <X class="size-5" /> | |
| </button> | |
| </div> | |
| <div class="space-y-4 px-5 py-5"> | |
| <p class="text-sm text-surface-600 dark:text-surface-300"> | |
| {{ message }} | |
| </p> | |
| <p class="text-sm text-surface-500 dark:text-surface-400"> | |
| Want write access? Upgrade to a paid hosted plan or deploy your own Applirank instance. | |
| </p> | |
| <div class="flex flex-wrap items-center gap-2"> | |
| <a | |
| href="mailto:sales@applirank.com?subject=Applirank%20Hosted%20Plan" | |
| class="inline-flex items-center gap-2 rounded-lg bg-brand-600 px-3 py-2 text-sm font-medium text-white transition-colors hover:bg-brand-700" | |
| > | |
| <Rocket class="size-4" /> | |
| Contact sales | |
| </a> | |
| <a | |
| href="https://github.com/applirank/applirank" | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| class="inline-flex items-center rounded-lg border border-surface-300 px-3 py-2 text-sm font-medium text-surface-700 transition-colors hover:bg-surface-50 dark:border-surface-700 dark:text-surface-200 dark:hover:bg-surface-800" | |
| > | |
| Deploy your own | |
| </a> | |
| </div> | |
| </div> | |
| </div> | |
| </div> | |
| <script setup lang="ts"> | |
| import { Crown, X, Rocket } from 'lucide-vue-next' | |
| import { onMounted, useTemplateRef } from 'vue' | |
| const emit = defineEmits<{ | |
| (e: 'close'): void | |
| }>() | |
| const { message } = usePreviewReadOnly() | |
| const dialogRef = useTemplateRef<HTMLDivElement>('dialog') | |
| function closeModal() { | |
| emit('close') | |
| } | |
| function onKeydown(e: KeyboardEvent) { | |
| if (e.key === 'Escape') closeModal() | |
| } | |
| onMounted(() => { | |
| dialogRef.value?.focus() | |
| }) | |
| </script> | |
| <template> | |
| <Teleport to="body"> | |
| <div class="fixed inset-0 z-50 flex items-center justify-center p-4" `@keydown`="onKeydown"> | |
| <div class="absolute inset-0 bg-black/50" `@click`="closeModal" /> | |
| <div | |
| ref="dialog" | |
| role="dialog" | |
| aria-modal="true" | |
| aria-labelledby="upsell-title" | |
| tabindex="-1" | |
| class="relative w-full max-w-md rounded-xl border border-surface-200 bg-white shadow-xl dark:border-surface-800 dark:bg-surface-900 focus:outline-none" | |
| > | |
| <div class="flex items-center justify-between border-b border-surface-200 px-5 py-4 dark:border-surface-800"> | |
| <div class="flex items-center gap-2"> | |
| <Crown class="size-5 text-brand-600 dark:text-brand-400" /> | |
| <h3 id="upsell-title" class="text-lg font-semibold text-surface-900 dark:text-surface-50">Unlock full editing</h3> | |
| </div> | |
| <button | |
| aria-label="Close dialog" | |
| class="cursor-pointer text-surface-400 transition-colors hover:text-surface-600 dark:hover:text-surface-200" | |
| `@click`="closeModal" | |
| > | |
| <X class="size-5" /> | |
| </button> | |
| </div> | |
| <div class="space-y-4 px-5 py-5"> | |
| <p class="text-sm text-surface-600 dark:text-surface-300"> | |
| {{ message }} | |
| </p> | |
| <p class="text-sm text-surface-500 dark:text-surface-400"> | |
| Want write access? Upgrade to a paid hosted plan or deploy your own Applirank instance. | |
| </p> | |
| <div class="flex flex-wrap items-center gap-2"> | |
| <a | |
| href="mailto:sales@applirank.com?subject=Applirank%20Hosted%20Plan" | |
| class="inline-flex items-center gap-2 rounded-lg bg-brand-600 px-3 py-2 text-sm font-medium text-white transition-colors hover:bg-brand-700" | |
| > | |
| <Rocket class="size-4" /> | |
| Contact sales | |
| </a> | |
| <a | |
| href="https://github.com/applirank/applirank" | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| class="inline-flex items-center rounded-lg border border-surface-300 px-3 py-2 text-sm font-medium text-surface-700 transition-colors hover:bg-surface-50 dark:border-surface-700 dark:text-surface-200 dark:hover:bg-surface-800" | |
| > | |
| Deploy your own | |
| </a> | |
| </div> | |
| </div> | |
| </div> | |
| </div> | |
| </Teleport> | |
| </template> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/components/PreviewUpsellModal.vue` around lines 17 - 64, The modal lacks
ARIA roles, keyboard handling, and focus management: add role="dialog" and
aria-modal="true" to the inner modal container element, give the <h3> a unique
id and reference it via aria-labelledby on that container, add
`@keydown.esc.window`="closeModal" to capture Escape, add aria-label="Close
dialog" to the close <button>, and implement focus management by adding a
template ref (e.g., modalRef) and code that, when the modal opens, saves
document.activeElement, moves focus into the first focusable element inside the
modal, traps Tab/Shift+Tab inside the modal, and restores focus on close using
the existing closeModal method; you can implement the trap with a small utility
in setup() or use a11y helper lib, and wire the watch/onMounted hooks to run
this behavior when the modal becomes visible.
| return maybeError.data?.code === 'PREVIEW_READ_ONLY' | ||
| } | ||
|
|
||
| export function usePreviewReadOnly() { |
There was a problem hiding this comment.
Add JSDoc to the exported usePreviewReadOnly function.
The exported function lacks a /** */ doc comment. As per coding guidelines, exported functions must use JSDoc comments.
📝 Proposed fix
+/**
+ * Composable for managing the preview read-only upsell flow.
+ * Provides shared modal state, error detection, and a mutation wrapper
+ * that automatically shows the upsell modal on PREVIEW_READ_ONLY errors.
+ */
export function usePreviewReadOnly() {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function usePreviewReadOnly() { | |
| /** | |
| * Composable for managing the preview read-only upsell flow. | |
| * Provides shared modal state, error detection, and a mutation wrapper | |
| * that automatically shows the upsell modal on PREVIEW_READ_ONLY errors. | |
| */ | |
| export function usePreviewReadOnly() { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/composables/usePreviewReadOnly.ts` at line 21, Add a JSDoc comment block
above the exported function usePreviewReadOnly describing its purpose, return
value, and any parameters or side effects (if applicable); update the comment to
follow standard JSDoc syntax (/** ... */) and include `@returns` with the return
type/description and any `@example` or `@remarks` if helpful, then save so the
exported function has a proper JSDoc header.
Comment on lines
+66
to
+77
| if (!guardedOrgId) { | ||
| if (import.meta.dev) return | ||
|
|
||
| throw createError({ | ||
| statusCode: 503, | ||
| statusMessage: 'Demo mode is misconfigured. Please contact support.', | ||
| data: { | ||
| code: 'DEMO_GUARD_MISCONFIGURED', | ||
| message: 'DEMO_ORG_SLUG is set but could not be resolved to an organization.', | ||
| }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
503 is thrown before the session check, blocking all tenants when misconfigured
When DEMO_ORG_SLUG is set but the demo org hasn't been seeded/resolved, getDemoOrgId() returns null. The 503 on lines 69–76 fires before the session org check on lines 98–107, so every authenticated write request from every tenant gets 503 until the misconfiguration is resolved. This turns a bad env-var into a full production write outage.
Consider returning early (fail-open) and emitting a structured warning log instead, keeping the guard voluntary when the demo org is absent:
🛡️ Proposed fix — fail-open with warning on misconfiguration
const guardedOrgId = await getDemoOrgId()
if (!guardedOrgId) {
- if (import.meta.dev) return
-
- throw createError({
- statusCode: 503,
- statusMessage: 'Demo mode is misconfigured. Please contact support.',
- data: {
- code: 'DEMO_GUARD_MISCONFIGURED',
- message: 'DEMO_ORG_SLUG is set but could not be resolved to an organization.',
- },
- })
+ // Guard is inoperative — warn loudly but don't block real users
+ console.warn('[demo-guard] DEMO_ORG_SLUG is set but could not be resolved. Demo protection is INACTIVE.')
+ return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!guardedOrgId) { | |
| if (import.meta.dev) return | |
| throw createError({ | |
| statusCode: 503, | |
| statusMessage: 'Demo mode is misconfigured. Please contact support.', | |
| data: { | |
| code: 'DEMO_GUARD_MISCONFIGURED', | |
| message: 'DEMO_ORG_SLUG is set but could not be resolved to an organization.', | |
| }, | |
| }) | |
| } | |
| if (!guardedOrgId) { | |
| // Guard is inoperative — warn loudly but don't block real users | |
| console.warn('[demo-guard] DEMO_ORG_SLUG is set but could not be resolved. Demo protection is INACTIVE.') | |
| return | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/middleware/demo-guard.ts` around lines 66 - 77, The current code
throws a 503 when guardedOrgId is falsy (the throw block using createError),
which runs before the session org check and blocks all tenants; instead, change
the behavior so that if getDemoOrgId()/guardedOrgId returns null you do NOT
throw — emit a structured warning via the existing logger (or use console.warn
if none) that DEMO_ORG_SLUG could not be resolved and then return early
(fail-open) so the guard remains voluntary; keep the existing import.meta.dev
short-circuit as-is and ensure any later logic (the session org check) still
runs for normal enforcement paths.
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 (1)
nuxt.config.ts (1)
6-12:⚠️ Potential issue | 🟠 Major
isRailwayPreviewfalse-positives on Railway's default "production" environment name
"production".startsWith("pr")evaluates totrue, so any Railway deployment whoseRAILWAY_ENVIRONMENT_NAMEis"production"(Railway's project default) will haveisRailwayPreview = truewhenDEMO_ORG_SLUGis not set. This would injectdemoOrgSlug = 'applirank-pr-demo'into the public runtime config, causing the demo banner to appear incorrectly in production.Railway creates all projects with a production environment by default, so this will be triggered on standard production deployments.
Meanwhile,
isRailwayPreviewEnvironmentinserver/utils/env.tsexplicitly guards against this by treating "production" as non-preview, creating a client/server split: the UI shows a demo banner while the server middleware does not guard writes.Add the same exclusion that
server/utils/env.tsapplies:🐛 Proposed fix — exclude the "production" environment name
const isRailwayPreview = - railwayEnvironmentName.startsWith('pr') + !railwayEnvironmentName.includes('production') + && (railwayEnvironmentName.startsWith('pr') || railwayEnvironmentName.includes('pr-') || railwayEnvironmentName.includes('pull request') || railwayEnvironmentName.includes('pull-request') - || railwayEnvironmentName.includes('preview') - || railwayPublicDomain.includes('-pr-') + || railwayEnvironmentName.includes('preview')) + || railwayPublicDomain.includes('-pr-')Also applies to: 58-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nuxt.config.ts` around lines 6 - 12, The isRailwayPreview boolean logic incorrectly treats "production" as a preview because "production".startsWith("pr") is true; update the expression that computes isRailwayPreview (the variable in nuxt.config.ts that references railwayEnvironmentName and railwayPublicDomain) to explicitly exclude the exact "production" environment name (e.g., add a guard like railwayEnvironmentName !== 'production') before evaluating startsWith/includes so that production is never marked as a preview; ensure the same exclusion is applied to the other occurrence at the second location mentioned so both checks mirror server/utils/env.ts behavior.
🧹 Nitpick comments (3)
server/middleware/demo-guard.ts (2)
80-84: Write-method check should precedegetConfiguredDemoSlugs()to avoid unnecessary work on read requestsEvery API GET/HEAD/OPTIONS request calls
getConfiguredDemoSlugs()(line 80) and returns early only at line 84. Since the slug list is only needed for writes, swapping the order avoids the env-read on every read-only request.♻️ Proposed reordering
- const demoSlugs = getConfiguredDemoSlugs() - if (demoSlugs.length === 0) return - - // Only guard write operations - if (!WRITE_METHODS.has(event.method)) return + // Only guard write operations + if (!WRITE_METHODS.has(event.method)) return + + const demoSlugs = getConfiguredDemoSlugs() + if (demoSlugs.length === 0) return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/middleware/demo-guard.ts` around lines 80 - 84, Move the early return based on HTTP method so we avoid calling getConfiguredDemoSlugs() on read-only requests: check WRITE_METHODS.has(event.method) first and return if false, then call getConfiguredDemoSlugs() and proceed with the existing demoSlugs.length === 0 check; update the logic around WRITE_METHODS, event.method, and getConfiguredDemoSlugs() accordingly.
28-40:getConfiguredDemoSlugs()recomputes on every request despite being fully deterministicThe slug list depends solely on env vars that don't change at runtime, so the result is constant for the server's lifetime. Evaluating it once at module load avoids repeated (cheap but unnecessary) re-computation.
♻️ Hoist to module level
-function getConfiguredDemoSlugs(): string[] { - const slugs = new Set<string>() - if (env.DEMO_ORG_SLUG) slugs.add(env.DEMO_ORG_SLUG) - if (isRailwayPreviewEnvironment(env.RAILWAY_ENVIRONMENT_NAME)) slugs.add(DEFAULT_PR_DEMO_ORG_SLUG) - return [...slugs] -} +const CONFIGURED_DEMO_SLUGS: string[] = (() => { + const slugs = new Set<string>() + if (env.DEMO_ORG_SLUG) slugs.add(env.DEMO_ORG_SLUG) + if (isRailwayPreviewEnvironment(env.RAILWAY_ENVIRONMENT_NAME)) slugs.add(DEFAULT_PR_DEMO_ORG_SLUG) + return [...slugs] +})()Then replace
getConfiguredDemoSlugs()at line 80 withCONFIGURED_DEMO_SLUGS.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/middleware/demo-guard.ts` around lines 28 - 40, The getConfiguredDemoSlugs() function is deterministic and should be evaluated once at module load: create a module-level constant CONFIGURED_DEMO_SLUGS initialized with the same logic (use env.DEMO_ORG_SLUG, isRailwayPreviewEnvironment(env.RAILWAY_ENVIRONMENT_NAME) and DEFAULT_PR_DEMO_ORG_SLUG to build a Set and return [...set]) and remove/replace calls to getConfiguredDemoSlugs() (e.g., the place currently referencing getConfiguredDemoSlugs) with CONFIGURED_DEMO_SLUGS; keep the same semantics and types as the original function.nuxt.config.ts (1)
58-58:'applirank-pr-demo'is duplicated betweennuxt.config.tsandDEFAULT_PR_DEMO_ORG_SLUGindemo-guard.tsIf the default PR demo slug ever changes in
demo-guard.ts, this string must be updated here too, with no compile-time enforcement. A shared constant can't be imported across theserver/boundary, so consider a brief comment linking them.♻️ Suggested annotation
- demoOrgSlug: process.env.DEMO_ORG_SLUG || (isRailwayPreview ? 'applirank-pr-demo' : ''), + // Keep in sync with DEFAULT_PR_DEMO_ORG_SLUG in server/middleware/demo-guard.ts + demoOrgSlug: process.env.DEMO_ORG_SLUG || (isRailwayPreview ? 'applirank-pr-demo' : ''),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nuxt.config.ts` at line 58, The literal 'applirank-pr-demo' used in the nuxt.config.ts demoOrgSlug assignment is duplicated from DEFAULT_PR_DEMO_ORG_SLUG in demo-guard.ts; update nuxt.config.ts by replacing the inline string with a short comment that references demo-guard.ts::DEFAULT_PR_DEMO_ORG_SLUG (and optionally mention the environment variable DEMO_ORG_SLUG) so future changes are coordinated—i.e., keep demoOrgSlug: process.env.DEMO_ORG_SLUG || (isRailwayPreview ? 'applirank-pr-demo' : ''), but add a comment like “// keep in sync with demo-guard.ts DEFAULT_PR_DEMO_ORG_SLUG” next to demoOrgSlug to signal the coupling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@nuxt.config.ts`:
- Around line 6-12: The isRailwayPreview boolean logic incorrectly treats
"production" as a preview because "production".startsWith("pr") is true; update
the expression that computes isRailwayPreview (the variable in nuxt.config.ts
that references railwayEnvironmentName and railwayPublicDomain) to explicitly
exclude the exact "production" environment name (e.g., add a guard like
railwayEnvironmentName !== 'production') before evaluating startsWith/includes
so that production is never marked as a preview; ensure the same exclusion is
applied to the other occurrence at the second location mentioned so both checks
mirror server/utils/env.ts behavior.
---
Duplicate comments:
In `@server/middleware/demo-guard.ts`:
- Around line 87-98: When guardedOrgIds.size === 0 in demo-guard.ts, don't throw
a 503 for all tenants; instead implement a fail-open behavior: log a clear
warning (include demoSlugs and that no orgs resolved) using the existing logger
(or console.warn) and return so requests proceed normally. Keep the existing
import.meta.dev early-return behavior, but for non-dev environments replace the
createError throw with a non-fatal warning and a no-op/disabled demo-guard path
so guardedOrgIds being empty does not block writes.
---
Nitpick comments:
In `@nuxt.config.ts`:
- Line 58: The literal 'applirank-pr-demo' used in the nuxt.config.ts
demoOrgSlug assignment is duplicated from DEFAULT_PR_DEMO_ORG_SLUG in
demo-guard.ts; update nuxt.config.ts by replacing the inline string with a short
comment that references demo-guard.ts::DEFAULT_PR_DEMO_ORG_SLUG (and optionally
mention the environment variable DEMO_ORG_SLUG) so future changes are
coordinated—i.e., keep demoOrgSlug: process.env.DEMO_ORG_SLUG ||
(isRailwayPreview ? 'applirank-pr-demo' : ''), but add a comment like “// keep
in sync with demo-guard.ts DEFAULT_PR_DEMO_ORG_SLUG” next to demoOrgSlug to
signal the coupling.
In `@server/middleware/demo-guard.ts`:
- Around line 80-84: Move the early return based on HTTP method so we avoid
calling getConfiguredDemoSlugs() on read-only requests: check
WRITE_METHODS.has(event.method) first and return if false, then call
getConfiguredDemoSlugs() and proceed with the existing demoSlugs.length === 0
check; update the logic around WRITE_METHODS, event.method, and
getConfiguredDemoSlugs() accordingly.
- Around line 28-40: The getConfiguredDemoSlugs() function is deterministic and
should be evaluated once at module load: create a module-level constant
CONFIGURED_DEMO_SLUGS initialized with the same logic (use env.DEMO_ORG_SLUG,
isRailwayPreviewEnvironment(env.RAILWAY_ENVIRONMENT_NAME) and
DEFAULT_PR_DEMO_ORG_SLUG to build a Set and return [...set]) and remove/replace
calls to getConfiguredDemoSlugs() (e.g., the place currently referencing
getConfiguredDemoSlugs) with CONFIGURED_DEMO_SLUGS; keep the same semantics and
types as the original function.
… message retrieval
railway-app
Bot
temporarily deployed
to
applirank / applirank-pr-38
railway-app
Bot
temporarily deployed
to
applirank / applirank-pr-38
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)
app/pages/dashboard/candidates/[id].vue (1)
110-120:⚠️ Potential issue | 🟠 MajorSame
isDeletingstuck-true bug as injobs/[id]/index.vue.
handleDeletehas nofinallyblock. WhenhandlePreviewReadOnlyErrorreturnstrueand the function returns early,isDeleting.valueis never reset tofalse, leaving the Delete button permanently disabled andshowDeleteConfirmstill set.🐛 Proposed fix
async function handleDelete() { isDeleting.value = true try { await deleteCandidate() } catch (err: any) { - if (handlePreviewReadOnlyError(err)) return + if (handlePreviewReadOnlyError(err)) { + showDeleteConfirm.value = false + return + } alert(err.data?.statusMessage ?? 'Failed to delete candidate') showDeleteConfirm.value = false - isDeleting.value = false - } + } finally { + isDeleting.value = false + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/pages/dashboard/candidates/`[id].vue around lines 110 - 120, handleDelete leaves isDeleting.value true and showDeleteConfirm.value set when handlePreviewReadOnlyError(err) returns true because it returns early; update handleDelete (the async function that calls deleteCandidate) to always reset isDeleting.value = false and set showDeleteConfirm.value = false regardless of early returns—either add a finally block that clears isDeleting and showDeleteConfirm or ensure the handler path that returns after handlePreviewReadOnlyError(err) also resets those flags; reference the symbols handleDelete, isDeleting, showDeleteConfirm, handlePreviewReadOnlyError, and deleteCandidate when making the change.app/pages/dashboard/jobs/[id]/index.vue (1)
146-156:⚠️ Potential issue | 🟠 Major
isDeletingis never reset whenhandlePreviewReadOnlyErrorreturnstrue.
handleDeletehas nofinallyblock —isDeleting.value = falseandshowDeleteConfirm.value = falseare only reached for non-preview errors. After a PREVIEW_READ_ONLY error, the delete button stays permanently disabled (until page reload) and the confirmation dialog remains open behind the upsell modal.This is inconsistent with
handleTransitionandhandleSave, which both usefinallyblocks for their loading-flag cleanup.🐛 Proposed fix
async function handleDelete() { isDeleting.value = true try { await deleteJob() } catch (err: any) { - if (handlePreviewReadOnlyError(err)) return + if (handlePreviewReadOnlyError(err)) { + showDeleteConfirm.value = false + return + } alert(err.data?.statusMessage ?? 'Failed to delete job') showDeleteConfirm.value = false - isDeleting.value = false - } + } finally { + isDeleting.value = false + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/pages/dashboard/jobs/`[id]/index.vue around lines 146 - 156, The handleDelete function currently sets isDeleting.value to true but only resets isDeleting and showDeleteConfirm inside the catch for non-preview errors, so when handlePreviewReadOnlyError(err) returns true the flags are never reset; update handleDelete to ensure cleanup in a finally block (after the try/catch) that sets isDeleting.value = false and showDeleteConfirm.value = false regardless of whether handlePreviewReadOnlyError returned true or deleteJob threw, keeping the existing try/await deleteJob() and catch that calls handlePreviewReadOnlyError(err).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/pages/dashboard/candidates/`[id].vue:
- Around line 110-120: handleDelete leaves isDeleting.value true and
showDeleteConfirm.value set when handlePreviewReadOnlyError(err) returns true
because it returns early; update handleDelete (the async function that calls
deleteCandidate) to always reset isDeleting.value = false and set
showDeleteConfirm.value = false regardless of early returns—either add a finally
block that clears isDeleting and showDeleteConfirm or ensure the handler path
that returns after handlePreviewReadOnlyError(err) also resets those flags;
reference the symbols handleDelete, isDeleting, showDeleteConfirm,
handlePreviewReadOnlyError, and deleteCandidate when making the change.
In `@app/pages/dashboard/jobs/`[id]/index.vue:
- Around line 146-156: The handleDelete function currently sets isDeleting.value
to true but only resets isDeleting and showDeleteConfirm inside the catch for
non-preview errors, so when handlePreviewReadOnlyError(err) returns true the
flags are never reset; update handleDelete to ensure cleanup in a finally block
(after the try/catch) that sets isDeleting.value = false and
showDeleteConfirm.value = false regardless of whether handlePreviewReadOnlyError
returned true or deleteJob threw, keeping the existing try/await deleteJob() and
catch that calls handlePreviewReadOnlyError(err).
There was a problem hiding this comment.
🧹 Nitpick comments (5)
app/composables/useJobs.ts (1)
32-43: Return{ success, message }from action catch blocks instead of rethrowing.The coding guideline for composables requires action error handling to return
{ success, message }objects rather than throwing. Both new try/catch blocks rethrow unconditionally, violating this contract. Any caller expecting the settled-return pattern will instead receive an uncaught exception.♻️ Proposed refactor – return `{ success, message }` instead of rethrowing
async function createJob(payload: { title: string description?: string location?: string type?: 'full_time' | 'part_time' | 'contract' | 'internship' }) { try { const created = await $fetch('/api/jobs', { method: 'POST', body: payload, }) await refresh() - return created + return { success: true as const, data: created } } catch (error) { handlePreviewReadOnlyError(error) - throw error + return { success: false as const, message: error instanceof Error ? error.message : 'Failed to create job' } } } async function deleteJob(id: string) { try { await $fetch(`/api/jobs/${id}`, { method: 'DELETE' }) + await refresh() + return { success: true as const } } catch (error) { handlePreviewReadOnlyError(error) - throw error + return { success: false as const, message: error instanceof Error ? error.message : 'Failed to delete job' } } - await refresh() }As per coding guidelines: "Implement action error handling with try-catch blocks returning
{ success, message }objects instead of throwing."Also applies to: 47-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/composables/useJobs.ts` around lines 32 - 43, The catch blocks in the composable action(s) that call $fetch('/api/jobs', ...) currently call handlePreviewReadOnlyError(error) and rethrow the error; change them to call handlePreviewReadOnlyError(error) and then return a settled result object like { success: false, message: (error?.message ?? String(error) || 'Unknown error') } instead of throwing; apply the same change to the second try/catch block around the other job action (the block that also awaits refresh()) so all action functions in useJobs.ts follow the { success, message } error-return contract.app/composables/useDocuments.ts (2)
30-30: Redundantas stringassertions on template literals.
\/api/...`with a runtime interpolation is already typed asstring— theas stringcast is a no-op and can be dropped. If the intent was to bypass Nuxt's typed-route inference for$fetch, the variable itself (beingstring`, not a string-literal type) already achieves that.♻️ Proposed fix
- const endpoint = `/api/candidates/${candidateId}/documents` as string + const endpoint = `/api/candidates/${candidateId}/documents`- const endpoint = `/api/documents/${documentId}` as string + const endpoint = `/api/documents/${documentId}`Also applies to: 80-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/composables/useDocuments.ts` at line 30, Remove the redundant "as string" type assertions on the template-literal endpoints in useDocuments.ts (e.g., the const endpoint = `/api/candidates/${candidateId}/documents` and the similar occurrence around line 80); simply use the template literal without casting since it already evaluates to a string, and ensure any calls that rely on the endpoint (such as $fetch/$axios usage) continue to accept the plain string variable.
32-46:let result: unknownerases the inferred$fetchreturn type.Moving
refreshNuxtDatainside thetryblock is semantically equivalent (thethrowincatchprevents it from running on failure) and avoids theunknownhoisted variable, preserving the inferred response type for callers.♻️ Proposed fix
- let result: unknown - try { - result = await $fetch(endpoint, { - method: 'POST', - body: formData, - }) - } catch (error) { - handlePreviewReadOnlyError(error) - throw error - } - - // Refresh the candidate detail cache so the document list updates - await refreshNuxtData(`candidate-${candidateId}`) - - return result + try { + const result = await $fetch(endpoint, { + method: 'POST', + body: formData, + }) + // Refresh the candidate detail cache so the document list updates + await refreshNuxtData(`candidate-${candidateId}`) + return result + } catch (error) { + handlePreviewReadOnlyError(error) + throw error + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/composables/useDocuments.ts` around lines 32 - 46, The code hoists let result: unknown which loses $fetch's inferred type; instead, move the await $fetch call and the subsequent await refreshNuxtData(`candidate-${candidateId}`) inside the try block and use a const (e.g., const result = await $fetch(...)) so the inferred return type is preserved, keep the catch calling handlePreviewReadOnlyError(error) and rethrow as before to prevent refreshNuxtData running on failure.app/composables/useJobQuestions.ts (2)
29-89: Rethrow pattern violates the{ success, message }action-error guideline.All four catch blocks call
handlePreviewReadOnlyError(error)and immediatelythrow error. The composable guidelines mandate returning{ success, message }objects from action functions instead of rethrowing, so callers don't need their own try/catch.♻️ Proposed refactor — illustrating the pattern on
addQuestion(apply consistently to all four actions)- async function addQuestion(payload: { ... }) { + async function addQuestion(payload: { ... }): Promise<{ success: true; data: ReturnType<typeof $fetch> } | { success: false; message: string }> { try { const created = await $fetch(`/api/jobs/${id.value}/questions`, { method: 'POST', body: payload, }) await refresh() - return created + return { success: true, data: created } } catch (error) { - handlePreviewReadOnlyError(error) - throw error + handlePreviewReadOnlyError(error) + return { success: false, message: error instanceof Error ? error.message : String(error) } } }Note: this is an API-surface change — callers will need to be updated to check
result.successinstead of wrapping in try/catch.Based on learnings: "Implement action error handling with try-catch blocks returning
{ success, message }objects instead of throwing."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/composables/useJobQuestions.ts` around lines 29 - 89, The four actions addQuestion, updateQuestion, deleteQuestion, and reorderQuestions currently call handlePreviewReadOnlyError(error) then rethrow, violating the composable guideline; replace the rethrow pattern so each function returns a standardized action result object instead: on success return an object like { success: true, data?: <created|updated|null> } (include the created/updated payload for addQuestion/updateQuestion, null for delete/reorder) and on error call handlePreviewReadOnlyError(error) then return { success: false, message: error?.message ?? String(error) } instead of throwing; ensure refresh() is still awaited on success where present and update callers to check result.success.
29-62: Optional: moverefresh()outside the try/catch inaddQuestionandupdateQuestion.Currently
refresh()sits inside the try block in both functions, so a refresh failure would incorrectly be passed tohandlePreviewReadOnlyError(harmless today since it won't match, but misleading and inconsistent with thedeleteQuestion/reorderQuestionspattern where refresh correctly lives outside the guard).♻️ Proposed refactor — shown for
addQuestionasync function addQuestion(payload: { ... }) { + let created try { - const created = await $fetch(`/api/jobs/${id.value}/questions`, { + created = await $fetch(`/api/jobs/${id.value}/questions`, { method: 'POST', body: payload, }) - await refresh() - return created } catch (error) { handlePreviewReadOnlyError(error) throw error } + await refresh() + return created }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/composables/useJobQuestions.ts` around lines 29 - 62, Move the await refresh() call out of the try blocks in addQuestion and updateQuestion so refresh failures are not routed to handlePreviewReadOnlyError; specifically, inside addQuestion (the code that awaits $fetch(`/api/jobs/${id.value}/questions`, ...) and currently returns created) and updateQuestion (the code that awaits $fetch(`/api/jobs/${id.value}/questions/${questionId}`, ...) and returns updated), assign the fetched result to a local variable (e.g., created/updated) inside the try, keep the catch to call handlePreviewReadOnlyError(error) and rethrow, then after the try/catch await refresh() and finally return the created/updated value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/composables/useDocuments.ts`:
- Line 30: Remove the redundant "as string" type assertions on the
template-literal endpoints in useDocuments.ts (e.g., the const endpoint =
`/api/candidates/${candidateId}/documents` and the similar occurrence around
line 80); simply use the template literal without casting since it already
evaluates to a string, and ensure any calls that rely on the endpoint (such as
$fetch/$axios usage) continue to accept the plain string variable.
- Around line 32-46: The code hoists let result: unknown which loses $fetch's
inferred type; instead, move the await $fetch call and the subsequent await
refreshNuxtData(`candidate-${candidateId}`) inside the try block and use a const
(e.g., const result = await $fetch(...)) so the inferred return type is
preserved, keep the catch calling handlePreviewReadOnlyError(error) and rethrow
as before to prevent refreshNuxtData running on failure.
In `@app/composables/useJobQuestions.ts`:
- Around line 29-89: The four actions addQuestion, updateQuestion,
deleteQuestion, and reorderQuestions currently call
handlePreviewReadOnlyError(error) then rethrow, violating the composable
guideline; replace the rethrow pattern so each function returns a standardized
action result object instead: on success return an object like { success: true,
data?: <created|updated|null> } (include the created/updated payload for
addQuestion/updateQuestion, null for delete/reorder) and on error call
handlePreviewReadOnlyError(error) then return { success: false, message:
error?.message ?? String(error) } instead of throwing; ensure refresh() is still
awaited on success where present and update callers to check result.success.
- Around line 29-62: Move the await refresh() call out of the try blocks in
addQuestion and updateQuestion so refresh failures are not routed to
handlePreviewReadOnlyError; specifically, inside addQuestion (the code that
awaits $fetch(`/api/jobs/${id.value}/questions`, ...) and currently returns
created) and updateQuestion (the code that awaits
$fetch(`/api/jobs/${id.value}/questions/${questionId}`, ...) and returns
updated), assign the fetched result to a local variable (e.g., created/updated)
inside the try, keep the catch to call handlePreviewReadOnlyError(error) and
rethrow, then after the try/catch await refresh() and finally return the
created/updated value.
In `@app/composables/useJobs.ts`:
- Around line 32-43: The catch blocks in the composable action(s) that call
$fetch('/api/jobs', ...) currently call handlePreviewReadOnlyError(error) and
rethrow the error; change them to call handlePreviewReadOnlyError(error) and
then return a settled result object like { success: false, message:
(error?.message ?? String(error) || 'Unknown error') } instead of throwing;
apply the same change to the second try/catch block around the other job action
(the block that also awaits refresh()) so all action functions in useJobs.ts
follow the { success, message } error-return contract.
… and add frontend intern role
railway-app
Bot
temporarily deployed
to
applirank / applirank-pr-38
…d attract candidates
railway-app
Bot
temporarily deployed
to
applirank / applirank-pr-38
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/scripts/seed.ts (1)
116-121: Missing SEO-relevant columns in the new job definition.The new
Frontend Engineering Internentry (and all existingJOBS_DATAentries) omitsalaryMin,salaryMax,salaryCurrency,salaryUnit,remoteStatus, andvalidThrough. For a demo, leaving thesenullmeans the product's SEO-relevant fields are never showcased. Consider populating them, at minimum for theopen-status jobs.💡 Example enrichment for the new intern job
{ title: 'Frontend Engineering Intern', description: `...`, location: 'Berlin, Germany (On-site)', type: 'internship' as const, status: 'draft' as const, + salaryMin: 1200, + salaryMax: 1500, + salaryCurrency: 'EUR', + salaryUnit: 'MONTH' as const, + remoteStatus: 'onsite' as const, + validThrough: daysAgo(-90), // 90 days from now },Based on learnings: "Job table must include SEO-relevant columns:
salaryMin,salaryMax,salaryCurrency,salaryUnit(YEAR/MONTH/HOUR),remoteStatus(remote/hybrid/onsite),validThrough"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/scripts/seed.ts` around lines 116 - 121, The new job object in JOBS_DATA (the entry with title 'Frontend Engineering Intern') is missing SEO-relevant fields; add salaryMin, salaryMax, salaryCurrency, salaryUnit, remoteStatus, and validThrough to that object (and ensure other JOBS_DATA entries include them too), populating meaningful values for open jobs (e.g., numeric salaryMin/salaryMax, a currency code like "EUR", a salaryUnit of "YEAR"/"MONTH"/"HOUR", remoteStatus as "onsite"/"remote"/"hybrid", and a validThrough ISO date) and leaving them null only for draft/unpublished jobs to preserve types and SEO behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/scripts/seed.ts`:
- Around line 258-272: The file-level header comment that states the total
application count is stale after adding JOB_4_APPS; update the top-of-file doc
comment to reflect the correct total of 57 applications (sum of JOB_0_APPS,
JOB_1_APPS, JOB_2_APPS, JOB_3_APPS, JOB_4_APPS) so the comment matches the data
used to build JOB_APPLICATIONS.
---
Nitpick comments:
In `@server/scripts/seed.ts`:
- Around line 116-121: The new job object in JOBS_DATA (the entry with title
'Frontend Engineering Intern') is missing SEO-relevant fields; add salaryMin,
salaryMax, salaryCurrency, salaryUnit, remoteStatus, and validThrough to that
object (and ensure other JOBS_DATA entries include them too), populating
meaningful values for open jobs (e.g., numeric salaryMin/salaryMax, a currency
code like "EUR", a salaryUnit of "YEAR"/"MONTH"/"HOUR", remoteStatus as
"onsite"/"remote"/"hybrid", and a validThrough ISO date) and leaving them null
only for draft/unpublished jobs to preserve types and SEO behavior.
Comment on lines
+258
to
+272
| // Job 4: Frontend Engineering Intern — active early-career funnel | ||
| const JOB_4_APPS: ApplicationAssignment[] = [ | ||
| { candidateIndex: 0, status: 'interview', score: 88, notes: 'Impressive internship project quality and thoughtful code reviews in GitHub profile.' }, | ||
| { candidateIndex: 2, status: 'interview', score: 84, notes: 'Strong fundamentals in Vue and Tailwind. Team fit interview scheduled.' }, | ||
| { candidateIndex: 4, status: 'screening', score: 79, notes: 'Good learning velocity and clean component architecture examples.' }, | ||
| { candidateIndex: 6, status: 'screening', score: 76, notes: 'Strong front-end fundamentals; evaluating SSR understanding.' }, | ||
| { candidateIndex: 11, status: 'screening', score: 74, notes: 'Promising portfolio with clear UX thinking for early-career level.' }, | ||
| { candidateIndex: 15, status: 'new', score: 71 }, | ||
| { candidateIndex: 17, status: 'new', score: 69 }, | ||
| { candidateIndex: 19, status: 'new', score: 67 }, | ||
| { candidateIndex: 21, status: 'new', score: 65 }, | ||
| { candidateIndex: 23, status: 'rejected', score: 46, notes: 'Limited JavaScript fundamentals demonstrated in practical assessment.' }, | ||
| ] | ||
|
|
||
| const JOB_APPLICATIONS = [JOB_0_APPS, JOB_1_APPS, JOB_2_APPS, JOB_3_APPS, []] | ||
| const JOB_APPLICATIONS = [JOB_0_APPS, JOB_1_APPS, JOB_2_APPS, JOB_3_APPS, JOB_4_APPS] |
There was a problem hiding this comment.
Fix stale application count in the file-level doc comment.
Adding JOB_4_APPS brings the real total to 57 applications (14 + 12 + 11 + 10 + 10), but the file header at Line 9 still reads 65+ applications. Update it to match.
📝 Proposed fix
- * - 65+ applications across all pipeline stages
+ * - 57 applications across all pipeline stages🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/scripts/seed.ts` around lines 258 - 272, The file-level header comment
that states the total application count is stale after adding JOB_4_APPS; update
the top-of-file doc comment to reflect the correct total of 57 applications (sum
of JOB_0_APPS, JOB_1_APPS, JOB_2_APPS, JOB_3_APPS, JOB_4_APPS) so the comment
matches the data used to build JOB_APPLICATIONS.
…t and aligning preview defaults
railway-app
Bot
temporarily deployed
to
applirank / applirank-pr-38
…latform-injected env vars
railway-app
Bot
temporarily deployed
to
applirank / applirank-pr-38
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Type of change
Validation
DCO
Signed-off-by) viagit commit -sSummary by CodeRabbit
New Features
Bug Fixes
Chores