Add four phone in-page navigation mockups (phone-first design study) - #1273
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdded a client-side phone in-page navigation mockup with four interactive navigation patterns, shared clinical content, responsive composition, sitemap registration, and route-specific mockup chrome handling. ChangesPhone in-page navigation mockup
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Visitor
participant NavigationPattern
participant ClinicalContent
Visitor->>NavigationPattern: selects a section
NavigationPattern->>ClinicalContent: updates active section
ClinicalContent-->>Visitor: displays matching content
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/mockups/mockups-layout-client.tsx (1)
34-41: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSuppress the shared composer on this isolated route.
Line 41 hides chrome, but Lines 34-40 still pass
searchComposerVisible={true}for/mockups/phone-inpage-navigation. Add!isPhoneInPageNavigationMockupthere too; otherwise the shell can add a second global composer alongside the mockup’s self-contained phone UI. As per coding guidelines, “Shared search chrome must have one page owner” and must not be stacked.🤖 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/app/mockups/mockups-layout-client.tsx` around lines 34 - 41, Update the searchComposerVisible condition in the mockups layout to also exclude isPhoneInPageNavigationMockup. Keep the existing chromeVisible behavior unchanged so the isolated phone navigation mockup owns its search UI without an additional shared composer.Source: Coding guidelines
🤖 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/app/mockups/phone-inpage-navigation/page.tsx`:
- Around line 76-122: Update ClinicalContent to derive the displayed section
labels and supporting copy from the active SectionLabel instead of keeping “Why
matched”, “Safety first”, and “Best fit” fixed. Ensure selecting Overview,
Protocol, or Patient sheet renders content corresponding to that active section
while preserving the existing layout and styling.
---
Outside diff comments:
In `@src/app/mockups/mockups-layout-client.tsx`:
- Around line 34-41: Update the searchComposerVisible condition in the mockups
layout to also exclude isPhoneInPageNavigationMockup. Keep the existing
chromeVisible behavior unchanged so the isolated phone navigation mockup owns
its search UI without an additional shared composer.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 29e28b8c-5de6-4478-89e7-f933ef392acd
📒 Files selected for processing (3)
docs/site-map.mdsrc/app/mockups/mockups-layout-client.tsxsrc/app/mockups/phone-inpage-navigation/page.tsx
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae48e16105
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex resolve actionable Codex review findings for this pull request and current head using the repository instructions. This is the pull request's single automatic repair pass: do not perform a fresh review, create new standalone findings, or request another review. Work only the existing unresolved Codex threads on the current head. Always fix P0 and P1 findings. For P2 and lower findings, fix only clear, scoped, low-risk issues; otherwise disposition them with a concise reason. After fixing or dispositioning a thread, reply in that thread with as the first line, followed by a concise summary; that marker authorizes the workflow to close that exact thread. If human input or new authorization is required, do not use the marker and leave the thread open with the blocker. Finish only after every actionable thread is fixed or dispositioned and closed, or explicitly left open for a human decision. Do not update the branch from main, address unrelated reviews, broaden scope, or create more than one scoped fix commit. Do not use external APIs, paid services, credentials, dependency changes, or broad refactors unless explicitly authorized. Add targeted tests where behavior changes and run the narrowest relevant validation. |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #5933 (success). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7ed26e648
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 130399c243
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 130399c243
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Prevent the center progress control from wrapping back to Overview when already on the last section, matching the disabled Next button. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/app/mockups/phone-inpage-navigation/page.tsx (2)
77-80: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist
contentMapto module scope and type blocks as a fixed 3-tuple.It's fully static, so rebuilding it on every render is wasted work, and a tuple type makes the
blocks[0..2]accesses below type-safe rather than relying on every entry happening to have three items.♻️ Suggested change
-function ClinicalContent({ active }: { active: SectionLabel }) { - const contentMap: Record< - SectionLabel, - Array<{ icon: typeof Sparkles; label: string; copy: string; variant: "default" | "warning" | "muted" }> - > = { +type ContentBlock = { icon: typeof Sparkles; label: string; copy: string; variant: "default" | "warning" | "muted" }; + +const contentMap: Record<SectionLabel, [ContentBlock, ContentBlock, ContentBlock]> = { + // ...existing entries, dedented one level +}; + +function ClinicalContent({ active }: { active: SectionLabel }) {🤖 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/app/mockups/phone-inpage-navigation/page.tsx` around lines 77 - 80, Move the static contentMap definition out of the component and into module scope, preserving its existing SectionLabel mapping and content. Change each mapped blocks value to a fixed three-element tuple type so the blocks[0], blocks[1], and blocks[2] accesses are type-safe.
216-247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the three near-identical block renderers.
The inline
(() => { const Icon = ...; return <Icon/> })()pattern is repeated three times with only colors differing. A small local component keyed offvariantwould remove the duplication and the IIFEs.♻️ Sketch
+const variantStyles = { + default: { wrapper: "border-b border-[`#283033`] p-4", label: "text-[`#64d9dc`]", copy: "text-[`#b6bfbd`]" }, + warning: { wrapper: "bg-[`#342b16`] p-4", label: "text-[`#f0c65d`]", copy: "text-[`#e1ca91`]" }, + muted: { wrapper: "border-t border-[`#41371d`] p-4", label: "text-[`#a7b0ae`]", copy: "text-[`#b6bfbd`]" }, +} as const; + +function ContentBlockRow({ block }: { block: ContentBlock }) { + const { icon: Icon, label, copy, variant } = block; + const styles = variantStyles[variant]; + return ( + <div className={styles.wrapper}> + <p className={`flex items-center gap-2 text-[11px] font-bold uppercase tracking-[0.1em] ${styles.label}`}> + <Icon className="size-3.5" /> {label} + </p> + <p className={`mt-2 text-[13px] leading-5 ${styles.copy}`}>{copy}</p> + </div> + ); +}Then render
{blocks.map((block) => <ContentBlockRow key={block.label} block={block} />)}.🤖 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/app/mockups/phone-inpage-navigation/page.tsx` around lines 216 - 247, Replace the three hard-coded block sections and repeated inline icon IIFEs with a local ContentBlockRow component that accepts a block and selects its styling through a variant. Render the blocks via blocks.map with a stable key such as block.label, while preserving the existing text, icon, color, spacing, and border differences for each variant.
🤖 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/app/mockups/phone-inpage-navigation/page.tsx`:
- Around line 77-80: Move the static contentMap definition out of the component
and into module scope, preserving its existing SectionLabel mapping and content.
Change each mapped blocks value to a fixed three-element tuple type so the
blocks[0], blocks[1], and blocks[2] accesses are type-safe.
- Around line 216-247: Replace the three hard-coded block sections and repeated
inline icon IIFEs with a local ContentBlockRow component that accepts a block
and selects its styling through a variant. Render the blocks via blocks.map with
a stable key such as block.label, while preserving the existing text, icon,
color, spacing, and border differences for each variant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 92e8a08c-4e65-47ef-8bbd-3c4b0dcfc330
📒 Files selected for processing (3)
docs/site-map.mdsrc/app/mockups/mockups-layout-client.tsxsrc/app/mockups/phone-inpage-navigation/page.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/site-map.md
- src/app/mockups/mockups-layout-client.tsx
Replace arbitrary text-[Npx] utilities with named theme steps so Static PR checks pass. Also keep overflow More selected and make phone frame content scroll under the header. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
|
Implemented review follow-ups on tip |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
* docs(ledger): record open-PR review and Bugbot sweep Append review outcomes for the current open PR queue after #1277 merged, including merge-tree classification and pr-bugbot findings. * docs(ledger): record open-PR recommendation implementation Capture closeouts for the unsafe audit cluster and the follow-up fixes pushed to #1273, #1275, #1280, and #1281. --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@cursoragent please resolve all CI failing tests after understanding the issue. Please also fix any comments and resolve them and also ensure that you fix with smallest required fix with best coding practice and u derstabding the fix rather than simple patch. Please fix any merge conflicts and ensure any issues are found. Please resolve any issues you find. Please also run bugbot. Please mark comments resolved also once you resolve them. |
|
PR #1273 is cleaned up on tip CI
Merge
Review comments
Bugbot
Commits
  |
The phone in-page navigation mockup owns its chrome; keep the shared mockup search composer off so it does not stack a second search bar. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
|
@cursoragent please resolve all CI failing tests after understanding the issue. Please also fix any comments and resolve them and also ensure that you fix with smallest required fix with best coding practice and u derstabding the fix rather than simple patch. Please fix any merge conflicts and ensure any issues are found. Please resolve any issues you find. Please also run bugbot. Please mark comments resolved also once you resolve them. |
|
Recheck complete on tip CI: Prior tip was already green (Static, Unit, Build, Production UI, Advisory, PR required). No failing tests to fix. Merge: GitHub Comments: Unresolved review threads 0 (prior Codex P2s already fixed + resolved). Bugbot: No Still in place: type-scale tokens, phone-frame scroll, More overflow selection, progress clamp, shared composer suppressed on this mockup route. |
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
|
@cursoragent fix merge conflict |
|
Fixed. There was no real content conflict — GitHub showed Merged |
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
2 participants
Motivation
Description
src/app/mockups/phone-inpage-navigation/page.tsx).src/app/mockups/mockups-layout-client.tsx) so the study can control the header ↔ navigation relationship.docs/site-map.md) so dev tooling and route-check tests detect it.Testing
npm run ensureconfirmed the app athttp://localhost:3131and the mockup route responded toGET /mockups/phone-inpage-navigation.npm run typecheckand ESLint on the changed files passed, andnpm run test:focused -- tests/site-map.test.tspassed after regenerating the sitemap; the full test sweep was exercised (the site-map test initially failed until the sitemap was updated, then the focused sitemap checks passed).npm run buildcompleted successfully including the new route and client-bundle secret scan.npx playwright install chromiumfailed to download browsers in this environment (HTTP 403), so automated screenshots were not produced.Codex Task
Summary by CodeRabbit