fix: stabilize sheet focus and dom contract - #1356
Conversation
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
GraphQL review-thread replies returned 403; resolved the fixed duplicate-height threads directly after validation. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
# Conflicts: # docs/branch-review-ledger.md Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
…mendations-fix Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthroughUpdated responsive height and flex-layout behavior for Sheet, AccessibleTable, and search dropdowns. Added regression assertions and appended PR ChangesResponsive UI sizing
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:a401258517
ℹ️ 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".
| expect(clinicalDashboardSource).toContain("resolveMobileComposerReserve("); | ||
| expect(clinicalDashboardSource).toContain('from "@/components/clinical-dashboard/mobile-composer-reserve"'); | ||
| expect(clinicalDashboardSource).not.toContain('bottomComposerHidden ? "max(0.75rem, env(safe-area-inset-bottom))"'); | ||
| expect(clinicalDashboardSource).not.toMatch(/pb-\[max\([^"']*safe-area-inset-bottom/); |
There was a problem hiding this comment.
When ClinicalDashboard legitimately adds visible-composer padding such as pb-[max(1rem,env(safe-area-inset-bottom))], this file-wide regex fails even though visible chrome may consume the safe-area inset; meanwhile, equivalent hidden padding using var(--safe-area-bottom) passes. This makes the guard both reject valid UI and miss an equivalent regression, so scope it to the hidden-composer branch and cover both inset forms.
AGENTS.md reference: AGENTS.md:L216-L218
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ui/sheet.tsx`:
- Around line 282-284: Update the max-height detection checks near
contentClassTokens to recognize Tailwind v4 trailing ! modifiers, including
unprefixed and sm-prefixed classes such as max-h-[88dvh]! and
sm:max-h-[min(80dvh,36rem)]!, while preserving existing prefix-! support. Add a
regression test covering both suffix forms and verify the Sheet does not append
its default cap.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eff51b4b-66fe-420d-a844-43d3d26f6097
📒 Files selected for processing (8)
docs/branch-review-ledger.mdsrc/components/AccessibleTable.tsxsrc/components/clinical-dashboard/universal-search-command-surface.tsxsrc/components/ui/sheet.tsxsrc/components/universal-search-command-mockups.tsxtests/clinical-dashboard-merge-artifacts.test.tstests/search-command-surface.test.tstests/sheet.dom.test.tsx
| const contentClassTokens = contentClassName?.split(/\s+/) ?? []; | ||
| const hasMobileMaxHeight = contentClassTokens.some((token) => /^!?max-h-/.test(token)); | ||
| const hasSmallScreenMaxHeight = contentClassTokens.some((token) => /^sm:!?max-h-/.test(token)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
printf'\n== sheet.tsx around the predicates ==\n'
sed -n '270,360p' src/components/ui/sheet.tsx
printf'\n== Tailwind docs in node_modules (if present) ==\n'
find node_modules/next/dist/docs -maxdepth 3 -type f \( -iname '*tailwind*' -o -iname '*css*' -o -iname '*important*'\)| sed -n '1,80p'printf'\n== Search for important modifier references ==\n'
rg -n "trailing important|important modifier|!$|max-h-\\[|max-h-" node_modules/next/dist/docs src/components/ui/sheet.tsx -S ||trueRepository: BigSimmo/Database
Length of output: 10359
🏁 Script executed:
#!/bin/bashset -euo pipefail
printf'\n== package manifests mentioning tailwind ==\n'
rg -n '"tailwindcss"|tailwindcss|`@tailwind`' package.json pnpm-lock.yaml yarn.lock package-lock.json bun.lockb . -g '!node_modules' -S ||trueprintf'\n== sheet.tsx snippet with line numbers ==\n'
cat -n src/components/ui/sheet.tsx | sed -n '278,356p'printf'\n== search for related tests or utilities ==\n'
rg -n "hasMobileMaxHeight|hasSmallScreenMaxHeight|max-h-\\[|max-h-" src test tests __tests__ . -g '!node_modules' -S ||trueRepository: BigSimmo/Database
Length of output: 33149
🏁 Script executed:
#!/bin/bashset -euo pipefail
printf'\n== package.json tailwind entries ==\n'
python3 - <<'PY'import json, pathlibp = pathlib.Path('package.json')if p.exists(): data = json.loads(p.read_text()) for section in ('dependencies', 'devDependencies', 'peerDependencies', 'optionalDependencies'): deps = data.get(section, {}) for name, ver in deps.items(): if 'tailwind' in name.lower(): print(section, name, ver)else: print('package.json not found')PYprintf'\n== lockfile tailwind entries (first matches) ==\n'forfin package-lock.json pnpm-lock.yaml yarn.lock bun.lockb;do
[ -f"$f" ] ||continueecho"--- $f ---"
rg -n "tailwindcss|tailwind""$f" -S | sed -n '1,40p'doneprintf'\n== local docs search for important modifier phrasing ==\n'
rg -n "important modifier|trailing .*!|prefix .*!|max-h-\\[" node_modules/next/dist/docs . -g '!node_modules/**/dist/**' -S ||trueRepository: BigSimmo/Database
Length of output: 10681
🌐 Web query:
Tailwind CSS v4 important modifier trailing exclamation mark utility syntax max-h-[88dvh]!
💡 Result:
In Tailwind CSS v4, the syntax for marking a utility as important has changed. You must now place the exclamation mark (!) at the end of the class name, rather than at the beginning [1][2]. For your specific case, the correct syntax is: max-h-[88dvh]! Key details regarding this change: - New Placement: The! modifier is now a suffix [1]. For example, a class like!text-blue-500 (v3 style) should be updated to text-blue-500! (v4 style) [1][2]. - Compatibility: While Tailwind v4 still supports the older prefix syntax (e.g.,!text-blue-500) for backwards compatibility, it is officially deprecated and you are encouraged to migrate to the suffix syntax [1]. - Arbitrary Values: The syntax for arbitrary values (square brackets) remains compatible with this modifier. Placing the! after the closing bracket correctly applies!important to the generated CSS rule [1][2]. - Global Important: If you intended to make all utilities important, the v4 approach is to use the important flag in your CSS import directive rather than the tailwind.config.js file [3][4]: @import "tailwindcss" important;
Citations:
- 1: https://tailwindcss.com/docs/upgrade-guide
- 2: https://github.com/dxos/dxos/blob/e4919fd46f87a8107cac7af0def56ea6136edc1f/TAILWIND_V4_UTILITY_CHANGES.md
- 3: Add support for
importantin v4 tailwindlabs/tailwindcss#14448 - 4: How to make all the classes important by default tailwindlabs/tailwindcss#17994
🏁 Script executed:
#!/bin/bashset -euo pipefail
printf'\n== tailwindcss package version and docs hits ==\n'
node - <<'JS'const pkg = require('./node_modules/tailwindcss/package.json');console.log(pkg.version);JSprintf'\n== search installed tailwindcss for important modifier docs/code ==\n'
rg -n "important modifier|prefix it with !|!mt-4|max-h-\\[88dvh\\]!|trailing" node_modules/tailwindcss node_modules/@tailwindcss -S ||trueprintf'\n== inspect candidate parser/source symbols for ! handling ==\n'
fd -a "" node_modules/tailwindcss | sed -n '1,120p'Repository: BigSimmo/Database
Length of output: 728
Recognize Tailwind v4 trailing ! important modifier. These checks miss classes like max-h-[88dvh]! and sm:max-h-[min(80dvh,36rem)]!, so the Sheet can still append its default cap when callers mark their height utility important. Accept the suffix form and add a regression test.
🤖 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/ui/sheet.tsx` around lines 282 - 284, Update the max-height
detection checks near contentClassTokens to recognize Tailwind v4 trailing !
modifiers, including unprefixed and sm-prefixed classes such as max-h-[88dvh]!
and sm:max-h-[min(80dvh,36rem)]!, while preserving existing prefix-! support.
Add a regression test covering both suffix forms and verify the Sheet does not
append its default cap.
Summary
RAG impact
Summary by CodeRabbit
Bug Fixes
Tests
Documentation