Add mobile case workspace routes - #191
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (4)
🔇 Additional comments (1)
📝 WalkthroughWalkthroughThis PR adds mobile workspace, facts, timeline, and summary route pages, new mobile UI components, sample workspace data, a timeline category type, and an updated mobile interaction checklist. ChangesMobile Case Workspace Feature
Sequence Diagram(s)sequenceDiagram
participant MobileWorkspacePage
participant getMobileCaseWorkspaceData
participant MobileCaseWorkspace
participant MobileFactsCarousel
participant MobileGenerateReportBar
MobileWorkspacePage->>getMobileCaseWorkspaceData: load workspace data for caseId
getMobileCaseWorkspaceData-->>MobileWorkspacePage: MobileCaseWorkspaceData
MobileWorkspacePage->>MobileCaseWorkspace: render workspace with data
MobileCaseWorkspace->>MobileFactsCarousel: render key facts
MobileCaseWorkspace->>MobileGenerateReportBar: render sticky report CTA
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/components/case-mobile/MobilePatternsSection.tsx (1)
31-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
event.idfor the list key when available.The upstream
MobilePattern.supportingEventstype includes an optionalidfield, but the key here is a composite ofdateanddescription, which could collide for two distinct events sharing the same date/description text.♻️ Proposed fix
- {pattern.supportingEvents.slice(0, 3).map((event) => ( - <p key={`${event.date}-${event.description}`} className="text-xs leading-5 text-neutral-600"> + {pattern.supportingEvents.slice(0, 3).map((event, index) => ( + <p key={event.id ?? `${event.date}-${event.description}-${index}`} className="text-xs leading-5 text-neutral-600">🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/case-mobile/MobilePatternsSection.tsx` around lines 31 - 35, The supporting events list in MobilePatternsSection currently uses a composite key from date and description, which can collide for distinct items. Update the map over pattern.supportingEvents to prefer event.id when present, and only fall back to the existing composite key when id is missing. Keep the change localized to the supportingEvents rendering in MobilePatternsSection and preserve the current display output.src/components/case-mobile/MobileFullSummaryScreen.tsx (1)
10-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse index-based keys instead of
key={paragraph}.If
fullSummaryever contains two identical paragraphs (e.g., a repeated boilerplate line), React will receive duplicate keys, causing a console warning and unreliable reconciliation.♻️ Proposed fix
- {text.split('\n\n').map((paragraph) => ( - <p key={paragraph} className="text-sm leading-7 text-neutral-700"> + {text.split('\n\n').map((paragraph, index) => ( + <p key={index} className="text-sm leading-7 text-neutral-700"> {paragraph} </p> ))}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/case-mobile/MobileFullSummaryScreen.tsx` around lines 10 - 14, The paragraph list in MobileFullSummaryScreen currently uses the paragraph text as the React key, which can collide when duplicate paragraphs appear. Update the map over text.split('\n\n') to use an index-based key in this render path, keeping the existing <p> element and className intact so each paragraph gets a stable unique key from its position.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/case-mobile/index.ts`:
- Around line 1-10: The barrel in case-mobile is missing the
MobileCaseDetailTopBar export, which is already defined in MobileCaseWorkspace
and consumed via `@/components/case-mobile` by the case pages. Update
src/components/case-mobile/index.ts to re-export MobileCaseDetailTopBar
alongside the existing exports so the imports in facts/page.tsx,
timeline/page.tsx, and workspace/summary/page.tsx resolve correctly.
In `@src/components/case-mobile/MobileCaseWorkspace.tsx`:
- Around line 13-20: Avoid the circular dependency caused by importing sibling
components through the barrel in MobileCaseWorkspace; replace the `./index`
import with direct imports from each component’s own module so
`MobileCaseWorkspace` no longer depends on
`src/components/case-mobile/index.ts`. Update the imports for
`MobileCaseSnapshotCard`, `MobileFactsCarousel`, `MobileGenerateReportBar`,
`MobileNarrativePreview`, `MobilePatternsSection`, and `MobileTimelineSnapshot`
to point at their respective files.
In `@src/components/case-mobile/MobileNarrativePreview.tsx`:
- Line 15: The gradient overlay in MobileNarrativePreview should use the
Tailwind v4 linear gradient utility instead of the removed bg-gradient-to-*
class. Update the class on the absolute bottom fade element to use
bg-linear-to-t while keeping the same direction and color stops, so the preview
still fades into transparent correctly.
In `@src/components/case-mobile/MobileTimelineScreen.tsx`:
- Around line 11-19: The filter logic in filters and eventMatchesFilter is
relying on title text for Calls and Exchange, but MobileTimelineEvent.sourceType
does not include those categories, so those chips produce empty or inconsistent
results. Update eventMatchesFilter to use a real sourceType-to-filter mapping
that matches the intended taxonomy from mobileTypes.ts, and ensure Calls and
Exchange are mapped to the correct underlying event types instead of falling
back to event.title matching; also resolve the current overlap between Court and
Evidence so each filter has distinct, predictable matches.
---
Nitpick comments:
In `@src/components/case-mobile/MobileFullSummaryScreen.tsx`:
- Around line 10-14: The paragraph list in MobileFullSummaryScreen currently
uses the paragraph text as the React key, which can collide when duplicate
paragraphs appear. Update the map over text.split('\n\n') to use an index-based
key in this render path, keeping the existing <p> element and className intact
so each paragraph gets a stable unique key from its position.
In `@src/components/case-mobile/MobilePatternsSection.tsx`:
- Around line 31-35: The supporting events list in MobilePatternsSection
currently uses a composite key from date and description, which can collide for
distinct items. Update the map over pattern.supportingEvents to prefer event.id
when present, and only fall back to the existing composite key when id is
missing. Keep the change localized to the supportingEvents rendering in
MobilePatternsSection and preserve the current display output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 800a9c22-1983-49d6-9b68-bf1e7f6750fa
📒 Files selected for processing (17)
docs/mobile-interaction-contract-checklist.mdsrc/app/case/[caseId]/facts/page.tsxsrc/app/case/[caseId]/timeline/page.tsxsrc/app/case/[caseId]/workspace/page.tsxsrc/app/case/[caseId]/workspace/summary/page.tsxsrc/components/case-mobile/MobileCaseSnapshotCard.tsxsrc/components/case-mobile/MobileCaseWorkspace.tsxsrc/components/case-mobile/MobileFactsCarousel.tsxsrc/components/case-mobile/MobileFactsList.tsxsrc/components/case-mobile/MobileFullSummaryScreen.tsxsrc/components/case-mobile/MobileGenerateReportBar.tsxsrc/components/case-mobile/MobileNarrativePreview.tsxsrc/components/case-mobile/MobilePatternsSection.tsxsrc/components/case-mobile/MobileTimelineScreen.tsxsrc/components/case-mobile/MobileTimelineSnapshot.tsxsrc/components/case-mobile/index.tssrc/lib/mobile/caseWorkspaceData.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Production Build
🔇 Additional comments (22)
src/app/case/[caseId]/timeline/page.tsx (2)
1-1: Same barrel-export issue asfacts/page.tsx.Depends on
MobileCaseDetailTopBarbeing re-exported from@/components/case-mobile(see comment onsrc/components/case-mobile/index.ts).
4-21: LGTM otherwise.src/app/case/[caseId]/workspace/summary/page.tsx (2)
1-1: Same barrel-export issue asfacts/page.tsx.Depends on
MobileCaseDetailTopBarbeing re-exported from@/components/case-mobile(see comment onsrc/components/case-mobile/index.ts).
16-16: 📐 Maintainability & Code Quality | 💤 Low valueVerify the
7rembottom padding is intentional here.This page doesn't render a sticky
MobileGenerateReportBar(unlike the main workspace page), yet uses a larger bottom-safe-area offset (7rem) than the facts/timeline pages (3rem). If there's no sticky footer on this screen, the extra padding may just be unused whitespace.src/lib/mobile/caseWorkspaceData.ts (2)
1-96: LGTM!
98-115: LGTM!The
fullSummaryjoin separator correctly matches the consumer's split contract. RendersfullSummaryby splitting the providedtexton double-newlines ('\n\n') into paragraph<p>elements.src/app/case/[caseId]/facts/page.tsx (2)
1-1: Will fail to compile until barrel export is fixed.This import depends on
MobileCaseDetailTopBarbeing re-exported from@/components/case-mobile, which is currently missing (see comment onsrc/components/case-mobile/index.ts).
4-21: LGTM otherwise — params handling, data fetch, and layout look correct.src/components/case-mobile/MobileCaseSnapshotCard.tsx (2)
1-39: LGTM!
26-26: 🎯 Functional CorrectnessNo duplicate
<h1>here
MobileTopBarrenderstitleinside a<span>, soMobileCaseWorkspacedoes not add another heading. The only<h1>on this screen isMobileCaseSnapshotCard.> Likely an incorrect or invalid review comment.src/components/case-mobile/MobileCaseWorkspace.tsx (3)
53-64: 🎯 Functional Correctness | 💤 Low valueInert action buttons: "Select case" and "More actions" have no handler.
onTitleAction={() => undefined}(Line 54) and the "More actions"MobileIconButtoninstances (Lines 61-63, 115-117) have noonClick. They render as interactive controls but do nothing when activated, which can confuse users/screen-reader users expecting an action.Also applies to: 109-118
88-92: 🎯 Functional CorrectnessGenerate Report just appends a query param — confirm downstream handling.
onGenerateReportpushes?report=1onto the current workspace route. No code in this PR reads that query param, so it's unclear what effect this has. Confirm this is wired up for a later phase rather than a no-op.
1-12: LGTM!Also applies to: 22-52, 67-95, 97-121
src/components/case-mobile/MobileFactsCarousel.tsx (1)
1-61: LGTM!src/components/case-mobile/MobileFactsList.tsx (1)
1-24: LGTM!src/components/case-mobile/MobilePatternsSection.tsx (1)
1-30: LGTM!Also applies to: 36-47
src/components/case-mobile/MobileTimelineSnapshot.tsx (1)
1-46: LGTM!src/components/case-mobile/MobileNarrativePreview.tsx (1)
1-26: LGTM!src/components/case-mobile/MobileTimelineScreen.tsx (1)
38-53: LGTM!src/components/case-mobile/MobileGenerateReportBar.tsx (1)
1-19: LGTM!src/app/case/[caseId]/workspace/page.tsx (1)
1-14: LGTM!docs/mobile-interaction-contract-checklist.md (1)
37-51: LGTM!
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/components/case-mobile/MobileCaseWorkspace.tsx (2)
106-127: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
MobileCaseDetailTopBarleft and right actions both navigate to the same route.
left("Go back") andright("Open workspace") both callrouter.push(\/case/${caseId}/workspace`). Presenting two icons for an identical destination is confusing, and usingrouter.pushfor "Go back" (rather thanrouter.back()`) forces a fixed target regardless of navigation history, which may not match user expectation of a back button.💡 Consider differentiating the two actions
left={ - <MobileIconButton label="Go back" onClick={() => router.push(`/case/${caseId}/workspace`)}> + <MobileIconButton label="Go back" onClick={() => router.back()}> <ArrowLeft aria-hidden="true" className="h-5 w-5" /> </MobileIconButton> }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/case-mobile/MobileCaseWorkspace.tsx` around lines 106 - 127, In MobileCaseDetailTopBar, the left “Go back” action and the right “Open workspace” action currently both navigate to the same workspace route. Update the component so the left control uses the Router’s back behavior (router.back()) or another true return action, while keeping the right control as the explicit workspace navigation via router.push. Use the MobileCaseDetailTopBar and MobileIconButton handlers to make the two actions clearly distinct.
49-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLeft and right top-bar buttons perform the identical action.
Both
left("Open menu") andright("Open case navigation") buttons callsetIsDrawerOpen(true)with different icons (MenuvsMoreHorizontal). This is redundant from a UX perspective — two buttons, same effect, different affordance implied by icon/label.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/case-mobile/MobileCaseWorkspace.tsx` around lines 49 - 64, The MobileCaseWorkspace top bar currently renders two buttons with different labels/icons but the same onClick behavior, which is redundant. Update the left and right actions in MobileCaseWorkspace so they represent distinct interactions, or remove one if only a single drawer toggle is needed; use the existing setIsDrawerOpen handler and the MobileIconButton/MobileTopBar props to wire each button to a unique action.
🤖 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.
Nitpick comments:
In `@src/components/case-mobile/MobileCaseWorkspace.tsx`:
- Around line 106-127: In MobileCaseDetailTopBar, the left “Go back” action and
the right “Open workspace” action currently both navigate to the same workspace
route. Update the component so the left control uses the Router’s back behavior
(router.back()) or another true return action, while keeping the right control
as the explicit workspace navigation via router.push. Use the
MobileCaseDetailTopBar and MobileIconButton handlers to make the two actions
clearly distinct.
- Around line 49-64: The MobileCaseWorkspace top bar currently renders two
buttons with different labels/icons but the same onClick behavior, which is
redundant. Update the left and right actions in MobileCaseWorkspace so they
represent distinct interactions, or remove one if only a single drawer toggle is
needed; use the existing setIsDrawerOpen handler and the
MobileIconButton/MobileTopBar props to wire each button to a unique action.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dcb6a1d7-8cff-4f2f-a2f4-150ef5160e60
📒 Files selected for processing (8)
src/components/case-mobile/MobileCaseWorkspace.tsxsrc/components/case-mobile/MobileFullSummaryScreen.tsxsrc/components/case-mobile/MobileNarrativePreview.tsxsrc/components/case-mobile/MobilePatternsSection.tsxsrc/components/case-mobile/MobileTimelineScreen.tsxsrc/components/case-mobile/index.tssrc/lib/mobile/caseWorkspaceData.tssrc/lib/mobile/mobileTypes.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/components/case-mobile/index.ts
- src/components/case-mobile/MobilePatternsSection.tsx
- src/components/case-mobile/MobileFullSummaryScreen.tsx
- src/components/case-mobile/MobileNarrativePreview.tsx
- src/lib/mobile/caseWorkspaceData.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Production Build
- GitHub Check: Unit & Regression Tests
🔇 Additional comments (4)
src/lib/mobile/mobileTypes.ts (1)
15-15: LGTM!src/components/case-mobile/MobileCaseWorkspace.tsx (2)
13-18: Circular-dependency fix confirmed.Imports are now pulled directly from sibling files instead of
./index, resolving the previously flagged circular dependency.
43-98: LGTM!src/components/case-mobile/MobileTimelineScreen.tsx (1)
13-21: 🎯 Functional CorrectnessFilter logic now aligns with
MobileTimelineEvent.category, resolving the prior Calls/Exchange bug.The category-based mapping matches the
mobileTypes.tsunion correctly, and the Court/Evidence overlap is resolved. However,Calls(line 17) andExchange(line 18) checkevent.categoryonly, with nosourceTypefallback — unlikeMessages,Court, andEvidence. If the sample/production data provider (caseWorkspaceData.ts) doesn't consistently populatecategoryfor pin/timeline-sourced events, these two filters will silently return empty result sets, similar to the original bug this replaces.#!/bin/bash # Verify that sample timeline data populates `category` for events intended to match 'call'/'exchange' filters. fd 'caseWorkspaceData.ts' src/lib/mobile -x cat -n {}
This branch was successfully deployed
2 participants
Summary
Validation
Notes
Summary by CodeRabbit