feat(web): add dedicated Remote Assignment page (re-land of #1384)#1386
Conversation
This reverts commit 865f39b.
…page The resultSlider locator was never referenced in the e2e suite and used a broken selector — AssignmentResultSlider renders as a Radix Sheet that portals its content to document.body, so the `~ div [role="dialog"]` selector relative to the page testid never matched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a remote-assignment session route and nav entry, moves assignment creation into that route, removes creation UI from the assignments page, and updates Playwright coverage and supporting translations/setup. ChangesRemote Assignment Flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant RouteComponent
participant createAssignmentMutation
participant axios
participant AssignmentParse as $Assignment.parse
participant AssignmentResultSlider
RouteComponent->>createAssignmentMutation: mutateAsync(payload)
createAssignmentMutation->>axios: POST /v1/assignments
axios-->>createAssignmentMutation: response.data
createAssignmentMutation->>AssignmentParse: parse(response.data)
AssignmentParse-->>RouteComponent: assignment data
RouteComponent->>AssignmentResultSlider: show assignment.url
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
@joshunrau this re-lands #1384 with the feedback addressed:
Ready for another look when you have a sec. |
![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
🤖 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 `@apps/web/src/routes/_app/datahub/`$subjectId/assignments.tsx:
- Line 118: The Table.Cell expression in the assignments view is hitting the
same unsafe member access on details.title as the remote-assignment page. Fix
the underlying instrument type resolution in the data lookup used by this
component so instruments[assignment.instrumentId] is typed with a safe details
shape, and then keep the title fallback logic in place without any unsafe member
access.
In `@apps/web/src/routes/_app/session/remote-assignment.tsx`:
- Line 140: The `selectedInstrument?.details.title` access in
`remote-assignment.tsx` is being flagged as unsafe by TypeScript/lint. Update
the expression around `selectedInstrument` to either add the requested
`@ts-expect-error` suppression directly above the unsafe member access or
refactor the type assertion/cast so `details` is treated as a safe typed
property before reading `title`.
- Around line 168-176: The onSubmit handler in remote-assignment.tsx is using a
non-null assertion on currentSession.subject, which can crash when the Session
subject is null. Update the createAssignmentMutation.mutateAsync call to avoid
currentSession.subject! and instead use the stable subject reference already
available via currentSession.subjectId, or add an explicit guard before
submitting if a subject is required. Keep the fix localized to the onSubmit flow
around createAssignmentMutation and currentSession.
In `@testing/e2e/src/pages/remote-assignment.page.ts`:
- Around line 23-27: The submit locator in submitAssignmentForm is too brittle
because getByLabel('Submit') is intended for labeled inputs, not buttons. Update
the locator in remote-assignment.page.ts to use getByRole('button', { name:
'Submit' }) on createDialog so the Playwright page object targets the submit
action by accessible role and name, keeping the method reliable if the Form
component’s markup changes.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ac934e56-6564-4ee3-8363-ff4d4d327a2f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
apps/web/src/hooks/useCreateAssignment.tsapps/web/src/hooks/useNavItems.tsapps/web/src/route-tree.tsapps/web/src/routes/_app/datahub/$subjectId/assignments.tsxapps/web/src/routes/_app/session/remote-assignment.tsxapps/web/src/translations/datahub.jsonapps/web/src/translations/layout.jsontesting/e2e/src/2.4-remote-assignment.spec.tstesting/e2e/src/helpers/fixtures.tstesting/e2e/src/pages/remote-assignment.page.ts
💤 Files with no reviewable changes (1)
- apps/web/src/translations/datahub.json
| key={assignment.id} | ||
| onClick={() => onSelection(assignment)} | ||
| > | ||
| <Table.Cell>{instruments[assignment.instrumentId]?.details.title ?? assignment.instrumentId}</Table.Cell> |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Same unsafe .title ESLint error as the remote-assignment page.
instruments[assignment.instrumentId]?.details.title triggers the same no-unsafe-member-access [error]. Likely the same UnilingualInstrumentInfo type-resolution root cause; fixing the type should clear both.
🧰 Tools
🪛 ESLint
[error] 118-118: Unsafe member access .title on a type that cannot be resolved.
(@typescript-eslint/no-unsafe-member-access)
🤖 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 `@apps/web/src/routes/_app/datahub/`$subjectId/assignments.tsx at line 118, The
Table.Cell expression in the assignments view is hitting the same unsafe member
access on details.title as the remote-assignment page. Fix the underlying
instrument type resolution in the data lookup used by this component so
instruments[assignment.instrumentId] is typed with a safe details shape, and
then keep the title fallback logic in place without any unsafe member access.
Source: Linters/SAST tools
| en: 'Assign "{}" to the current subject for remote completion.', | ||
| fr: 'Assigner « {} » au client actuel pour complétion à distance.' | ||
| }, | ||
| { args: [selectedInstrument?.details.title ?? ''] } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP "TranslatedInstrumentInfo" packages/schemas/src -C2
fd -t f 'eslint.config.*' apps/web
rg -nP "no-unsafe-member-access|recommendedTypeChecked|lint" apps/web/package.jsonRepository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 1016
Fix unsafe access to details.title.
TranslatedInstrumentInfo is correctly imported, but TypeScript resolves the .details property as unsafe (likely unknown or any) in this context, triggering the no-unsafe-member-access error. Add a @ts-expect-error disable directive or refactor the type cast to silence the error.
Snippet
// `@ts-expect-error` Unsafe member access on `details`
{ args: [selectedInstrument?.details.title ?? ''] }🧰 Tools
🪛 ESLint
[error] 140-140: Unsafe member access .title on a type that cannot be resolved.
(@typescript-eslint/no-unsafe-member-access)
🤖 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 `@apps/web/src/routes/_app/session/remote-assignment.tsx` at line 140, The
`selectedInstrument?.details.title` access in `remote-assignment.tsx` is being
flagged as unsafe by TypeScript/lint. Update the expression around
`selectedInstrument` to either add the requested `@ts-expect-error` suppression
directly above the unsafe member access or refactor the type assertion/cast so
`details` is treated as a safe typed property before reading `title`.
Source: Linters/SAST tools
| async submitAssignmentForm() { | ||
| const submitButton = this.createDialog.getByLabel('Submit'); | ||
| await submitButton.waitFor({ state: 'visible' }); | ||
| await submitButton.click(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the Form component and inspect how the submit button is rendered.
fd -t f -e tsx -e ts 'Form' apps/web/src 2>/dev/null
rg -nP -C3 'type=["'\'']submit["'\'']|getByRole\(.?button|aria-label' --type=ts --type=tsx apps/web/src | rg -i submit -C2
# Also check other e2e page objects for the prevailing submit-button locator convention.
rg -nP -C2 "getBy(Label|Role)\(" testing/e2e/src/pagesRepository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 3381
🏁 Script executed:
cat apps/web/src/components/Shared/Form.tsx 2>/dev/null || cat apps/web/src/components/Form/Form.tsx 2>/dev/null || fd -t f -e tsx 'Form.tsx' | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && cat "{}"'Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 17089
🏁 Script executed:
fd -t f -e tsx -e ts 'index.tsx' | xargs grep -l "submitBtnLabel" 2>/dev/null | head -5Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 224
🏁 Script executed:
find . -path "*/node_modules" -prune -o -type f -name "*.tsx" -exec grep -l "export.*Form.*props" {} \; -quit | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && cat "{}"'Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 177
🏁 Script executed:
grep -r "submitBtnLabel" --include="*.tsx" --include="*.ts" -A 10 | grep -E "(aria-label|label=|type=[\"']submit)" | head -20Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 177
🏁 Script executed:
cat node_modules/@douglasneuroinformatics/libui/dist/components/form/Form.js 2>/dev/null || grep -r "submitBtnLabel" --include="*.tsx" --include="*.ts" node_modules/@douglasneuroinformatics/libui 2>/dev/null | head -20Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 177
🏁 Script executed:
grep -r "Form" --include="*.tsx" --include="*.ts" apps/web/src/libui 2>/dev/null | head -5Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 177
🏁 Script executed:
ls -la apps/web/src/ | grep -E "lib|ui"Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 177
🏁 Script executed:
cat apps/web/src/routes/_app/session/remote-assignment/page.tsx 2>/dev/nullRepository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 177
🏁 Script executed:
grep -r "submitBtnLabel" --include="*.tsx" --include="*.ts" --include="*.json" apps/ web/ 2>/dev/null -B 2 -A 5 | head -50Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 4650
🏁 Script executed:
fd -e tsx -e ts 'libui' | head -10Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 177
🏁 Script executed:
grep -r "<Form" --include="*.tsx" apps/web/src/routes/_app/session/remote-assignment* 2>/dev/null -B 5 -A 10Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 628
🏁 Script executed:
fd -t f -e tsx 'page.tsx' apps/web/src/routes/_app/session/remote-assignment* 2>/dev/null | xargs catRepository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 177
🏁 Script executed:
grep -r "class Form" --include="*.tsx" --include="*.ts" . | grep -v node_modules | head -10Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 292
🏁 Script executed:
cat apps/web/src/components/IdentificationForm/IdentificationForm.page.tsx 2>/dev/null || fd -e tsx -e ts 'remote-assignment' | xargs -I {} grep -l "Form" {}Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 326
🏁 Script executed:
rg -A 50 "export default function RemoteAssignmentPage" apps/web/src/routes/_app/session/remote-assignment/page.tsx | rg -A 50 "Form"Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 277
🏁 Script executed:
rg "submitBtnLabel" --type tsx --type ts -B 1 -A 1 | rg -A 5 -B 5 "submit.*btn"Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 205
🏁 Script executed:
rg "aria-label.*submit" --type tsx --type ts -i | head -20Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 205
🏁 Script executed:
rg "type.*submit" --type tsx --type ts -B 5 -A 5 | rg -B 5 -A 5 "label" | head -40Repository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 205
🏁 Script executed:
cat apps/web/src/routes/_app/session/remote-assignment.tsxRepository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 7639
🏁 Script executed:
python3 - <<'PY'
import json
import re
# Based on the code, the Form component is from '`@douglasneuroinformatics/libui/components`'
# It receives a 'content' object and 'submitBtnLabel' prop (optional).
# If 'submitBtnLabel' is not provided, it likely defaults to something.
# Let's check if there are any specific type definitions or if we can infer the DOM structure.
# The test uses `getByLabel('Submit')`.
# In the code `remote-assignment.tsx`, the Form has `content` and `onSubmit` but *no* `submitBtnLabel`.
# If the library's default submitBtnLabel is 'Submit', then `getByLabel('Submit')` might work IF:
# 1. The button has an `aria-label="Submit"`
# 2. The button text is "Submit" (GetByLabel works on text for buttons? No. GetByLabel is for inputs with <label> or aria-label. GetByText is for button text.)
#
# If the library wraps the button in a <label> tag, that would be weird.
# Or perhaps the button has `id="submit"` and the label is hidden?
#
# Key finding: The test code uses `getByLabel` on a `Dialog`.
# `getByLabel` usually relies on `aria-label` or a `<label for="...">`.
# A submit button in a form usually has text content ("Submit") or `aria-label`.
#
# Let's check the other forms again.
# `StartSessionForm` has `submitBtnLabel={t('core.submit')}`.
# `remote-assignment.tsx` does NOT provide `submitBtnLabel`.
# Does the libui `Form` component provide a default? Likely.
#
# However, `getByLabel` is technically the WRONG semantic selector for a button.
# `getByRole('button', { name: 'Submit' })` is the robust choice.
# Even if `getByLabel` works now, it relies on implementation details (e.g., if the library adds an aria-label for some reason).
# If the library changes and stops adding aria-label, the test breaks.
#
# Conclusion: The warning in the original review is valid advice for robustness.
# The implementation detail (whether the button currently has aria-label) is secondary to the architectural recommendation of using the correct ARIA role selector.
# But I need to confirm if the specific instance in the code is actually broken or just bad practice.
# In `remote-assignment.tsx`, no `submitBtnLabel` is passed.
# If the default is 'Submit', and the button has text "Submit", `getByRole` works.
# Does `getByLabel` work on text? No, unless it acts like a text value.
#
# Let's assume the reviewer is correct about the risk.
# The code in `remote-assignment.page.ts` line 24 uses `getByLabel`.
# I will recommend switching to `getByRole`.
print("Analysis complete: Using getByRole is the idiomatic and robust approach for buttons.")
PYRepository: DouglasNeuroInformatics/OpenDataCapture
Length of output: 262
Replace getByLabel('Submit') with getByRole('button', { name: 'Submit' }) for better reliability.
getByLabel targets inputs with associated <label> or aria-label attributes. Submit buttons are best targeted by their role (button) and accessible name. This change ensures the locator remains valid even if the underlying Form component changes how it renders the button label.
Snippet
async submitAssignmentForm() {
const submitButton = this.createDialog.getByRole('button', { name: 'Submit' });
await submitButton.waitFor({ state: 'visible' });
await submitButton.click();
}🤖 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 `@testing/e2e/src/pages/remote-assignment.page.ts` around lines 23 - 27, The
submit locator in submitAssignmentForm is too brittle because
getByLabel('Submit') is intended for labeled inputs, not buttons. Update the
locator in remote-assignment.page.ts to use getByRole('button', { name: 'Submit'
}) on createDialog so the Playwright page object targets the submit action by
accessible role and name, keeping the method reliable if the Form component’s
markup changes.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…t setup The 2.4-remote-assignment E2E test timed out waiting for an instrument card to appear. The per-browser groups created in global setup had empty accessibleInstrumentIds, so the group-scoped user saw no instruments and the InstrumentShowcase rendered with no <li> to click. Fetch the instrument list as admin and patch each created group with those accessibleInstrumentIds so the showcase is populated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Avoid the non-null assertion on the nullable currentSession.subject by reading subjectId directly off the session. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
Re-lands #1384, which was reverted in #1385, with review feedback addressed.
What this does
/session/remote-assignmentfor creating remote assignments (choose an instrument, set an expiry, get a shareable link / QR code).Dialog/Formfromdatahub/$subjectId/assignments.tsx. Assignment creation now lives solely on the dedicated page; the datahub assignments tab now only displays existing assignments (view / cancel).2.4-remote-assignment.spec.ts+ page object).Gateway gating (corrected from the original PR description)
The original #1384 description claimed this did not replace the existing creation flow and that the datahub flow "continues to work as before" — that was inaccurate and contributed to the revert. To be precise:
When
isGatewayEnabledis false, there is no assignment UI of any kind — same behavior as before this feature. Both entry points are gated:useNavItems.ts(behindisGatewayEnabled)datahub/$subjectId/route.tsx(behindconfig.setup.isGatewayEnabled)This is intentional and confirmed: server-side
create()always goes throughgatewayService.createRemoteAssignment, so assignment creation already required the gateway.Changes since #1384
resultSliderlocator from the e2e page object (theAssignmentResultSliderRadix Sheet portals todocument.body, so the relative selector never matched and it was never referenced).Test plan
Refs #1317
🤖 Generated with Claude Code
Summary by CodeRabbit