Update from template#65
Conversation
* Upgrade to React 19, Tailwind 4, and new shadcn baseline Propagate changes from paranext-core upgrade-shadcn branch: - Update package.json deps (React 19, TW4, @tailwindcss/postcss, @dreamsicle.io/stylelint-config-tailwindcss, tw-animate-css, shadcn@4.3.0, @fontsource-variable/ibm-plex-sans, lucide-react@1.8, eslint-plugin-react-hooks@5) - Slim tailwind.config.ts (TW4 moves theme/colors/etc into CSS) - postcss.config.ts uses @tailwindcss/postcss - .stylelintrc.js uses @Dreamsicle config and ignores new TW4 at-rules - src/tailwind.css rewritten for TW4: imports tailwindcss with prefix(tw), tw-animate-css, shadcn/tailwind.css, fontsource font; @theme inline, @custom-variant dark, oklch theme variables (light/dark/paratext-light/ paratext-dark) - web-view-resolve-webpack-plugin.ts: bind resolver.join to resolver Co-Authored-By: Claude Code <noreply@anthropic.com> * Propagated override * Added success color variable * Rename success → success-foreground, add warning theme variables - Rename CSS variable `--success` → `--success-foreground` (and the corresponding `--color-success` → `--color-success-foreground` in `@theme inline`) to align with shadcn's `*-foreground` naming. - Add `--warning` and `--warning-foreground` (amber-400/amber-950 in light themes, amber-900/amber-50 in dark themes) plus their `--color-warning` / `--color-warning-foreground` entries in `@theme inline`. Pulled from https://ui.shadcn.com/docs/theming#adding-new-tokens. - Add a CUSTOM "Theme variable policy" disclaimer at the top of the file reminding extension authors to prefer existing theme variables and to get UX approval before introducing new semantic colors. * Fixed package-lock.json, audit fix --------- Co-authored-by: Claude Code <noreply@anthropic.com>
📝 WalkthroughWalkthroughTailwind CSS v3 is upgraded to v4 across the entire codebase: dependencies are updated to React 19 and Tailwind v4.3.0; PostCSS configuration switches to the ChangesTailwind CSS v3 → v4 Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
AGENTS.md (1)
58-58: ⚡ Quick winClarify the negative example to isolate the variant-order issue.
The negative example
hover:tw-px-3mixes two problems: wrong variant order AND the oldtw-prefix. This could confuse developers who are learning the new syntax. Consider usinghover:tw:px-3in the negative example to focus on the order issue while showing the correct new prefix.📝 Proposed clarification
-All UI uses Tailwind CSS (via `src/tailwind.css`). Every Tailwind class is prefixed `tw:` to avoid collisions with Platform.Bible's own styles (configured in `tailwind.config.ts`). For modifier variants the prefix comes first: `tw:hover:px-3`, not `hover:tw-px-3`. +All UI uses Tailwind CSS (via `src/tailwind.css`). Every Tailwind class is prefixed `tw:` to avoid collisions with Platform.Bible's own styles (configured in `tailwind.config.ts`). For modifier variants the prefix comes first: `tw:hover:px-3`, not `hover:tw:px-3`.🤖 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 `@AGENTS.md` at line 58, The negative example in the Tailwind prefix guidance mixes two issues (wrong variant order and an old prefix form); update the negative example to isolate the variant-order problem by replacing the current example that uses the obsolete prefix with the same correct prefix but wrong order (use "hover:tw:px-3" as the negative example) and keep the positive example "tw:hover:px-3"; reference the Tailwind prefix convention in src/tailwind.css and the configuration in tailwind.config.ts when making the change.
🤖 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 @.stylelintrc.js:
- Around line 32-43: The Stylelint configuration's ignoreAtRules array is
missing Tailwind v4's "utility" at-rule, causing lint errors for `@utility`;
update the ignoreAtRules array in .stylelintrc.js (the ignoreAtRules setting) to
include 'utility' alongside the existing entries so Stylelint allows `@utility`
no-scrollbar in src/tailwind.css.
In `@src/tailwind.css`:
- Around line 42-49: Move the block of Tailwind imports and the `@config`
statement so they appear before any other at-rules: relocate the lines
containing "@import 'tailwindcss' prefix(tw);", "@import 'tw-animate-css';",
"@import 'shadcn/tailwind.css';", "@import
'@fontsource-variable/ibm-plex-sans';" and the "@config
'../tailwind.config.ts';" so they come at the very top of the stylesheet, ahead
of the "@layer base" and any "@utility" blocks; preserve the existing comments
and ordering of those imports when moving them.
---
Nitpick comments:
In `@AGENTS.md`:
- Line 58: The negative example in the Tailwind prefix guidance mixes two issues
(wrong variant order and an old prefix form); update the negative example to
isolate the variant-order problem by replacing the current example that uses the
obsolete prefix with the same correct prefix but wrong order (use
"hover:tw:px-3" as the negative example) and keep the positive example
"tw:hover:px-3"; reference the Tailwind prefix convention in src/tailwind.css
and the configuration in tailwind.config.ts when making the change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d4a02f5-4cd4-4d1f-9dd6-4b02065619fb
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
.stylelintrc.jsAGENTS.mdpackage.jsonpostcss.config.tssrc/__tests__/components/ContinuousView.test.tsxsrc/__tests__/components/PhraseBox.test.tsxsrc/components/ContinuousScrollToggle.tsxsrc/components/ContinuousView.tsxsrc/components/Interlinearizer.tsxsrc/components/InterlinearizerLoader.tsxsrc/components/PhraseBox.tsxsrc/components/ScriptureNavControls.tsxsrc/components/SegmentView.tsxsrc/components/TokenChip.tsxsrc/hooks/useOptimisticBooleanSetting.tssrc/interlinearizer.web-view.tsxsrc/tailwind.csstailwind.config.tswebpack/web-view-resolve-webpack-plugin.ts
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc reviewed 20 files and all commit messages, and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on imnasnainaec, jasonleenaylor, and myieye).
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc reviewed 3 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on imnasnainaec and jasonleenaylor).
imnasnainaec
left a comment
There was a problem hiding this comment.
@imnasnainaec reviewed 20 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on alex-rawlings-yyc and jasonleenaylor).
a discussion (no related file):
This looks good, but wouldn't build for me. I had to delete both node_modules/ and package-lock.json and run npm i to regenerate package-lock.json (and also run npm run core:reset for good measure), and then it worked.
Previously, imnasnainaec (D. Ror.) wrote…
Are there changes here that caused that as far as you can tell? |
I'm not sure, but it was webpack related. While trying to pin down the problem, I found that at least running In any case, I'm suggesting you regenerate |
|
I think we may just have to advise that people run the The current Good to merge for now? |
Just running |
|
Nope. Very odd |
|
Perhaps we should add a script that runs rm -rf node_modules package-lock.json
npm installOr perhaps just tell people to run that after template updates? I can't think of any other way to fix this |
|
I reverted to latest |
|
I did a fresh install for both core and this branch and build broke again (see below). One possible difference is that I have yalc set up locally. I'll work on a script for deleting the |
imnasnainaec
left a comment
There was a problem hiding this comment.
@imnasnainaec reviewed 5 files and all commit messages, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on jasonleenaylor).
fc206ee to
721c428
Compare
0ea8827 to
b3be80e
Compare
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc reviewed 10 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on imnasnainaec and jasonleenaylor).
b3be80e to
791ffd6
Compare
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc reviewed 1 file and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on imnasnainaec and jasonleenaylor).
imnasnainaec
left a comment
There was a problem hiding this comment.
@imnasnainaec reviewed 5 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on jasonleenaylor).
@coderabbitai ignore
This change is
Summary by CodeRabbit
Dependencies
Documentation
Bug Fixes
Configuration