Conversation
carlagn
requested review from
AmanVarshney01,
ArthurGamby,
aidankmcalister,
ankur-arch,
mhartington and
mhessdev
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThis pull request introduces a migration feature showcasing database schema evolution steps with syntax-highlighted Prisma and SQL code. It adds a new Shiki-based highlighter module with a custom theme, includes a new migration marketing page with interactive code display components, updates styling, and refactors the button component's render logic. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
apps/site/src/lib/shiki_prisma.ts (1)
225-243: Use promise-based singleton for more robust highlighter initializationThe current null-check pattern can race if
activeStepchanges during the initialcreateHighlighter()call, potentially instantiating multiple highlighters. While the impact is minor (browser-side, single call site), using the nullish coalescing operator with a promise is cleaner and more resilient.Suggested pattern
-let prisma_highlighter: Awaited<ReturnType<typeof createHighlighter>> | null = - null; +let prismaHighlighterPromise: ReturnType<typeof createHighlighter> | null = null; async function getHighlighter() { - if (!prisma_highlighter) { - prisma_highlighter = await createHighlighter({ + prismaHighlighterPromise ??= createHighlighter({ themes: [prismaTheme], langs: [ "typescript", "javascript", "jsx", "tsx", "json", "bash", "sh", "prisma", "sql", "diff", ], - }); - } - return prisma_highlighter; + }); + return prismaHighlighterPromise; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/site/src/lib/shiki_prisma.ts` around lines 225 - 243, The current null-check in getHighlighter can race; switch to a promise-based singleton by assigning the createHighlighter promise to prisma_highlighter before awaiting it (e.g. in getHighlighter use prisma_highlighter ||= createHighlighter({...}) and then await prisma_highlighter), so multiple concurrent calls reuse the same promise; update any related type to reflect that prisma_highlighter holds the creation promise and ensure getHighlighter awaits and returns the resolved highlighter (references: getHighlighter, prisma_highlighter, createHighlighter).apps/site/src/components/migrate/hero-code.tsx (1)
51-57: Makediff-adddetection line-start based, not token-presence based.Current logic can mark lines as added when
+appears later in the line (not as a diff prefix).💡 Proposed refactor
const processedSchemaHtml = schemaHtml.replace( /<span class="line">([\s\S]*?)(<\/span>)/g, (match, content, closing) => { - // Check if line starts with + (accounting for any leading spans) - const startsWithPlus = - /<span[^>]*>\+/.test(content) || content.trim().startsWith("+"); + const plainTextLine = content.replace(/<[^>]+>/g, ""); + const startsWithPlus = plainTextLine.trimStart().startsWith("+"); if (startsWithPlus) { return `<span class="line diff-add">${content}${closing}`; } return match; }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/site/src/components/migrate/hero-code.tsx` around lines 51 - 57, The current startsWithPlus detection in the processedSchemaHtml.replace callback incorrectly flags lines with a '+' anywhere in the token content; instead determine if the first visible character of the line is '+' by stripping any leading span tags and whitespace from the captured content and then checking the very first character — update the logic in the replace callback (where processedSchemaHtml and the captured content variable are used) to remove leading <span...> tags and whitespace and only then test for a leading '+' so only true diff-prefix additions are marked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/site/src/app/migrate/page.tsx`:
- Around line 146-148: Update the h4 heading text currently rendered as
"Deterministic/ Repeatable" inside the JSX element (the <h4 className="text-xl
text-foreground-neutral font-sans-display font-extrabold">) to correct the slash
spacing; change it to either "Deterministic/Repeatable" (no space) or
"Deterministic / Repeatable" (space on both sides) for consistent punctuation
spacing in the UI copy.
- Around line 340-344: The paragraph element with className
"text-foreground-neutral-weak" under the "Streamlined collaboration" card is
duplicating the previous card's version-control copy; update that paragraph to
match the "Streamlined collaboration" heading by replacing the text with a
description about collaborative workflows (e.g., shared migration history, team
reviews, and safe rollbacks) so the card content aligns with its title; locate
the JSX block in page.tsx that renders the "Streamlined collaboration" card and
change the inner text of the paragraph element accordingly.
In `@apps/site/src/components/migrate/hero-code.tsx`:
- Around line 20-22: The component assumes steps has entries and dereferences
steps[activeStep] unconditionally, which will crash for empty arrays; update the
HeroCode component (types HeroCodeProps / HeroCodeStep, variables steps and
activeStep) to first guard that steps?.length > 0 (or return a lightweight
fallback/placeholder) before accessing steps[activeStep], clamp activeStep to
the valid index range (e.g., Math.min(activeStep, steps.length - 1)), and
replace direct property access with optional chaining/default values so all uses
(the places referencing steps[activeStep]) safely handle an empty steps array.
- Around line 69-73: When catching highlight errors in the try/catch (currently
using isMounted and setIsLoading), also clear the component's highlighted HTML
state so stale code isn't shown; call the state updater that stores the rendered
HTML (e.g., setHighlightedHtml('') or setHighlightedCode('') — whichever exists
in this component) inside the catch before setting isLoading to false, and only
do both calls when isMounted is true.
---
Nitpick comments:
In `@apps/site/src/components/migrate/hero-code.tsx`:
- Around line 51-57: The current startsWithPlus detection in the
processedSchemaHtml.replace callback incorrectly flags lines with a '+' anywhere
in the token content; instead determine if the first visible character of the
line is '+' by stripping any leading span tags and whitespace from the captured
content and then checking the very first character — update the logic in the
replace callback (where processedSchemaHtml and the captured content variable
are used) to remove leading <span...> tags and whitespace and only then test for
a leading '+' so only true diff-prefix additions are marked.
In `@apps/site/src/lib/shiki_prisma.ts`:
- Around line 225-243: The current null-check in getHighlighter can race; switch
to a promise-based singleton by assigning the createHighlighter promise to
prisma_highlighter before awaiting it (e.g. in getHighlighter use
prisma_highlighter ||= createHighlighter({...}) and then await
prisma_highlighter), so multiple concurrent calls reuse the same promise; update
any related type to reflect that prisma_highlighter holds the creation promise
and ensure getHighlighter awaits and returns the resolved highlighter
(references: getHighlighter, prisma_highlighter, createHighlighter).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c627cf95-fa6a-4625-a15b-1e94ae7b7d3d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
apps/site/package.jsonapps/site/src/app/global.cssapps/site/src/app/layout.tsxapps/site/src/app/migrate/page.tsxapps/site/src/components/migrate/hero-code.tsxapps/site/src/lib/cn.tsapps/site/src/lib/shiki_prisma.tspackages/eclipse/src/components/button.tsxpackages/ui/src/styles/globals.css
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/site/src/components/migrate/hero-code.tsx (2)
69-73:⚠️ Potential issue | 🟡 MinorClear highlighted HTML on highlight failure to avoid stale UI state.
When highlighting fails, the component keeps previous HTML visible while step metadata can change, which is misleading.
💡 Suggested fix
} catch (error) { console.error("Failed to highlight code:", error); if (isMounted) { + setHighlightedSchema(""); + setHighlightedMigration(""); setIsLoading(false); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/site/src/components/migrate/hero-code.tsx` around lines 69 - 73, On highlight failure, clear the previously rendered highlighted HTML so stale content isn't shown: inside the catch block where you currently call console.error and setIsLoading(false) (guarded by isMounted), also call setHighlightedHtml('') (and any related state like setHighlightedLang('') if present) while still checking isMounted so the component shows no highlighted output after an error.
20-22:⚠️ Potential issue | 🟠 MajorGuard
stepsaccess and clamp active index to prevent render/runtime crashes.
steps[activeStep]is dereferenced in several places without guarding empty arrays or out-of-range indices after prop changes. This can crash rendering and the highlighter path.💡 Suggested fix
interface HeroCodeProps { - steps: HeroCodeStep[]; + steps: [HeroCodeStep, ...HeroCodeStep[]]; } const HeroCode: React.FC<HeroCodeProps> = ({ steps }) => { const [activeStep, setActiveStep] = useState(0); + if (!steps.length) return null; + const safeStepIndex = Math.min(activeStep, steps.length - 1); + const currentStep = steps[safeStepIndex]; @@ - highlighter.codeToHtml(steps[activeStep].schema, { + highlighter.codeToHtml(currentStep.schema, { lang: "prisma", theme: "prisma-dark", }), - highlighter.codeToHtml(steps[activeStep].migrateFileContents, { + highlighter.codeToHtml(currentStep.migrateFileContents, { lang: "sql", theme: "prisma-dark", }), @@ - activeStep === steps.length - 1 - ? setActiveStep(0) - : setActiveStep(activeStep + 1); + setActiveStep((prev) => (prev >= steps.length - 1 ? 0 : prev + 1)); @@ - {steps[activeStep].title} + {currentStep.title} @@ - {steps[activeStep].migrateFileName} + {currentStep.migrateFileName} @@ - transform: `translate(${steps[activeStep].arrowOffset.x}px, ${steps[activeStep].arrowOffset.y}px) rotate(${steps[activeStep].arrowOffset.rotation}deg)`, + transform: `translate(${currentStep.arrowOffset.x}px, ${currentStep.arrowOffset.y}px) rotate(${currentStep.arrowOffset.rotation}deg)`,Also applies to: 40-45, 86-89, 119-120, 152-156, 183-184
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/site/src/components/migrate/hero-code.tsx` around lines 20 - 22, The component dereferences steps[activeStep] in multiple places (e.g., rendering the highlighted path and using step properties) without guarding for an empty or out-of-range steps array; update the component that uses HeroCodeProps (references: steps, activeStep, HeroCode) to (1) clamp/normalize activeStep to the valid range (0 .. steps.length-1) whenever props change (use Math.max/Math.min or similar) and (2) guard all usages of steps[activeStep] with a safe fallback (check steps.length > 0 or use optional chaining and a default empty step object) before passing values into the highlighter or rendering to prevent runtime crashes when steps is empty or activeStep is out-of-bounds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/site/src/components/migrate/hero-code.tsx`:
- Around line 69-73: On highlight failure, clear the previously rendered
highlighted HTML so stale content isn't shown: inside the catch block where you
currently call console.error and setIsLoading(false) (guarded by isMounted),
also call setHighlightedHtml('') (and any related state like
setHighlightedLang('') if present) while still checking isMounted so the
component shows no highlighted output after an error.
- Around line 20-22: The component dereferences steps[activeStep] in multiple
places (e.g., rendering the highlighted path and using step properties) without
guarding for an empty or out-of-range steps array; update the component that
uses HeroCodeProps (references: steps, activeStep, HeroCode) to (1)
clamp/normalize activeStep to the valid range (0 .. steps.length-1) whenever
props change (use Math.max/Math.min or similar) and (2) guard all usages of
steps[activeStep] with a safe fallback (check steps.length > 0 or use optional
chaining and a default empty step object) before passing values into the
highlighter or rendering to prevent runtime crashes when steps is empty or
activeStep is out-of-bounds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 43767418-04b3-4537-961f-c5e93c55faa0
📒 Files selected for processing (1)
apps/site/src/components/migrate/hero-code.tsx
aidankmcalister
previously approved these changes
Mar 25, 2026
Contributor
|
Hi @carlagn, I added a few comments on slack here : https://prisma-company.slack.com/archives/C02UA0D53KM/p1774518475702329?thread_ts=1774462124.529269&cid=C02UA0D53KM Could you also ask claude quickly to take a look a this Major warning to see if it's a false alarm or not Let me know if you need more infos |
mhartington
previously approved these changes
Mar 27, 2026
carlagn
dismissed stale reviews from mhartington and aidankmcalister
via
e7dbbe5
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/site/src/components/migrate/hero-code.tsx (3)
84-89: Consider simplifying with modulo arithmetic.The current ternary works correctly, but modulo arithmetic expresses the cycling intent more directly:
♻️ Optional simplification
const cycleActiveStep = () => { - activeStep === steps.length - 1 - ? setActiveStep(0) - : setActiveStep(activeStep + 1); + setActiveStep((activeStep + 1) % steps.length); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/site/src/components/migrate/hero-code.tsx` around lines 84 - 89, The cycleActiveStep function currently uses a ternary to wrap the index manually; simplify it to use modulo arithmetic by setting the new active step to (activeStep + 1) % steps.length via setActiveStep so the index automatically wraps when it reaches the end (update cycleActiveStep to call setActiveStep((activeStep + 1) % steps.length)).
132-136:dangerouslySetInnerHTMLusage—acceptable with trusted data.Static analysis correctly flags this as a potential XSS vector. However, since the
stepsdata originates frommigrateStepsinpage.tsx(hardcoded for this marketing page), the risk is mitigated.That said, if
stepsever accepts user-provided or external data in the future, you'd need sanitization. Consider adding a brief comment documenting this assumption:// Safety: HTML is Shiki-generated from trusted, hardcoded schema/migration strings dangerouslySetInnerHTML={{ __html: highlightedSchema, }}Also applies to line 167-169.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/site/src/components/migrate/hero-code.tsx` around lines 132 - 136, Add a short inline safety comment where dangerouslySetInnerHTML is used (the div that uses __html: highlightedSchema and the other similar occurrence) stating that the HTML is trusted because it is generated by Shiki from hardcoded migrateSteps in page.tsx; reference the variables highlightedSchema and migrateSteps and the component (hero-code / HeroCode) so future readers know the assumption and that sanitization will be required if steps become external/user-provided.
82-82: Note onstepsin dependency array.Including
stepsin the dependency array means the effect re-runs whenever the array reference changes, even if the content is identical. If the parent component re-creates thestepsarray on each render (common with inline array literals), you'll get unnecessary highlighting runs.The cached highlighter from
shiki_prisma.tsmitigates the cost, but if you notice performance issues, consider memoizingstepsin the parent withuseMemoor extracting it to a stable reference.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/site/src/components/migrate/hero-code.tsx` at line 82, The effect in hero-code.tsx is re-running due to a non-stable steps reference in the dependency array ([activeStep, steps]); to fix, ensure steps is a stable reference by memoizing it where it’s created (e.g., wrap the array in useMemo in the parent) or move the memoization into this component before the useEffect so the dependency uses the memoized variable (e.g., memoizedSteps) instead of a freshly created array; update the useEffect dependency from [activeStep, steps] to [activeStep, memoizedSteps] (or keep steps if the parent memoizes) and ensure any symbols referenced like activeStep and steps/memoizedSteps match the names in hero-code.tsx.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/site/src/components/migrate/hero-code.tsx`:
- Around line 51-62: The current regex in processedSchemaHtml that tries to
detect diff lines from schemaHtml is fragile because Shiki emits nested <span>
tags and the non-greedy capture will stop at the first inner </span>, corrupting
HTML; fix it by not attempting to parse nested HTML with a regex: either
(preferred) mark diff lines before highlighting (use steps[activeStep].schema to
build a Set of line indices that start with '+' and then inject classes by
iterating over occurrences of <span class="line"> in the highlighted output) or
(alternative) parse the highlighted HTML with a DOMParser and add the diff-add
class to the outer .line elements; update the logic around processedSchemaHtml
to use one of these approaches instead of the current regex.
---
Nitpick comments:
In `@apps/site/src/components/migrate/hero-code.tsx`:
- Around line 84-89: The cycleActiveStep function currently uses a ternary to
wrap the index manually; simplify it to use modulo arithmetic by setting the new
active step to (activeStep + 1) % steps.length via setActiveStep so the index
automatically wraps when it reaches the end (update cycleActiveStep to call
setActiveStep((activeStep + 1) % steps.length)).
- Around line 132-136: Add a short inline safety comment where
dangerouslySetInnerHTML is used (the div that uses __html: highlightedSchema and
the other similar occurrence) stating that the HTML is trusted because it is
generated by Shiki from hardcoded migrateSteps in page.tsx; reference the
variables highlightedSchema and migrateSteps and the component (hero-code /
HeroCode) so future readers know the assumption and that sanitization will be
required if steps become external/user-provided.
- Line 82: The effect in hero-code.tsx is re-running due to a non-stable steps
reference in the dependency array ([activeStep, steps]); to fix, ensure steps is
a stable reference by memoizing it where it’s created (e.g., wrap the array in
useMemo in the parent) or move the memoization into this component before the
useEffect so the dependency uses the memoized variable (e.g., memoizedSteps)
instead of a freshly created array; update the useEffect dependency from
[activeStep, steps] to [activeStep, memoizedSteps] (or keep steps if the parent
memoizes) and ensure any symbols referenced like activeStep and
steps/memoizedSteps match the names in hero-code.tsx.
🪄 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: d76a1c3f-ac68-4d94-9caa-37f766dbd359
📒 Files selected for processing (2)
apps/site/src/app/migrate/page.tsxapps/site/src/components/migrate/hero-code.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/site/src/app/migrate/page.tsx
Comment on lines
+51
to
+62
| const processedSchemaHtml = schemaHtml.replace( | ||
| /<span class="line">([\s\S]*?)(<\/span>)/g, | ||
| (match, content, closing) => { | ||
| // Check if line starts with + (accounting for any leading spans) | ||
| const startsWithPlus = | ||
| /<span[^>]*>\+/.test(content) || content.trim().startsWith("+"); | ||
| if (startsWithPlus) { | ||
| return `<span class="line diff-add">${content}${closing}`; | ||
| } | ||
| return match; | ||
| }, | ||
| ); |
Contributor
There was a problem hiding this comment.
Regex for diff detection is fragile with Shiki's nested spans.
Shiki generates deeply nested <span> elements for token highlighting. The pattern ([\s\S]*?)(<\/span>) is non-greedy and will match up to the first </span> encountered—which is typically a nested token span, not the closing tag for the .line span.
For example, Shiki might output:
<span class="line"><span style="color:#...">+</span><span style="...">model</span></span>Your regex would capture only <span style="color:#...">+ as content, potentially corrupting the HTML structure.
A more robust approach would be to either:
- Use a proper HTML parser (e.g.,
DOMParserin browser) - Match the line span more precisely by finding balanced tags
- Process the raw code before highlighting to mark diff lines, then use Shiki's built-in diff support
💡 Alternative: mark lines before highlighting
// Pre-process to identify diff lines by index
const schemaLines = steps[activeStep].schema.split('\n');
const diffLineIndices = new Set(
schemaLines
.map((line, i) => (line.trimStart().startsWith('+') ? i : -1))
.filter(i => i >= 0)
);
// After highlighting, inject class by line index
const processedHtml = schemaHtml.replace(
/<span class="line">/g,
(() => {
let lineIndex = 0;
return (match: string) => {
const isDiff = diffLineIndices.has(lineIndex++);
return isDiff ? '<span class="line diff-add">' : match;
};
})()
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/site/src/components/migrate/hero-code.tsx` around lines 51 - 62, The
current regex in processedSchemaHtml that tries to detect diff lines from
schemaHtml is fragile because Shiki emits nested <span> tags and the non-greedy
capture will stop at the first inner </span>, corrupting HTML; fix it by not
attempting to parse nested HTML with a regex: either (preferred) mark diff lines
before highlighting (use steps[activeStep].schema to build a Set of line indices
that start with '+' and then inject classes by iterating over occurrences of
<span class="line"> in the highlighted output) or (alternative) parse the
highlighted HTML with a DOMParser and add the diff-add class to the outer .line
elements; update the logic around processedSchemaHtml to use one of these
approaches instead of the current regex.
ArthurGamby
previously approved these changes
Mar 30, 2026
carlagn
force-pushed
the
feat/DR-7750-migrate
branch
from
2fe2af6 to
9cd2cab
Compare
mhartington
approved these changes
Mar 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Style
Chores