Conversation
ref DES-1325 Added layered subpath exports, token surfaces, docs rules, and root deprecation markers.
|
Important Review skippedToo many files! This PR contains 229 files, which is 79 over the limit of 150. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (229)
You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe PR restructures the Shade package into layered entrypoints by adding 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
apps/shade/src/tokens.ts (1)
2-2: Rename exported token list to camelCase to match repo convention.
SHADE_TOKEN_NAMESshould be camelCase inapps/shade/srcfiles.Proposed change
-export const SHADE_TOKEN_NAMES = [ +export const shadeTokenNames = [ ... -] as const; +] as const; -export type ShadeTokenName = (typeof SHADE_TOKEN_NAMES)[number]; +export type ShadeTokenName = (typeof shadeTokenNames)[number];As per coding guidelines, "apps/shade/src/**/*.{ts,tsx,js}: Use
camelCasefor function and variable names".Also applies to: 287-287
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/shade/src/tokens.ts` at line 2, Rename the exported constant SHADE_TOKEN_NAMES to camelCase (e.g., shadeTokenNames) across the codebase: update the export in apps/shade/src/tokens.ts and replace all references/imports (search for SHADE_TOKEN_NAMES) to the new identifier so imports, type annotations, and usages in functions/components continue to work; ensure any re-exports or tests referencing SHADE_TOKEN_NAMES are updated and run the type checker to catch missed occurrences.apps/shade/src/primitives.ts (1)
2-7: Use@alias for internal re-exports instead of relative paths.Please switch these re-exports to the project alias form (e.g.
@/components/layout/page) for consistency with theapps/shade/srcimport convention.As per coding guidelines, "Use the
@alias for internal imports (e.g.,@/lib/utils)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/shade/src/primitives.ts` around lines 2 - 7, Update the internal re-exports in primitives.ts to use the project alias instead of relative paths: replace './components/...' imports with '@/components/...' for each export (e.g., export * from '@/components/layout/page', export {ErrorPage} from '@/components/layout/error-page', export * from '@/components/layout/heading', export * from '@/components/layout/header', export * from '@/components/layout/list-header', export * from '@/components/layout/view-header') so the file follows the apps/shade/src alias convention.apps/shade/src/components.ts (1)
2-59: Use@alias for internal component/asset exports.Please convert internal
./components/...and./assets/...export paths to@/...aliases for consistency with the Shade src import convention.As per coding guidelines, "Use the
@alias for internal imports (e.g.,@/lib/utils)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/shade/src/components.ts` around lines 2 - 59, Update all internal relative export paths to use the project alias: replace every export from './components/ui/...' with '@/components/ui/...' (including the named Icon export: "IconComponents as Icon") and replace the SVG asset exports like "ReactComponent as FacebookLogo", "GhostLogo", "GhostOrb", "GoogleLogo", "TwitterLogo", "XLogo" to import from '@/assets/images/...' instead of './assets/images/...'; leave external package exports (e.g., DropdownMenuCheckboxItemProps from '@radix-ui/react-dropdown-menu') unchanged.apps/shade/src/utils.ts (1)
5-21: Switch local re-exports to@alias paths.For
useGlobalDirtyState,useSimplePagination, and./lib/utilsre-exports, use@/...paths instead of relative imports.As per coding guidelines, "Use the
@alias for internal imports (e.g.,@/lib/utils)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/shade/src/utils.ts` around lines 5 - 21, Replace the local relative re-exports with the project alias paths: change the default export for useGlobalDirtyState to export from "@/hooks/use-global-dirty-state", change useSimplePagination to export from "@/hooks/use-simple-pagination", and change the collective re-exports (cn, debounce, kebabToPascalCase, formatTimestamp, formatNumber, formatDuration, formatPercentage, formatDisplayDate, formatDisplayTime, getCountryFlag, stringToHslColor, abbreviateNumber) to export from "@/lib/utils" so all three re-export statements use the `@` alias instead of relative `./` paths.apps/shade/src/patterns.ts (1)
2-7: Use@alias for internal exports in this entrypoint.Please replace
./components/...paths with@/...paths to keep import style consistent acrossapps/shade/src.As per coding guidelines, "Use the
@alias for internal imports (e.g.,@/lib/utils)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/shade/src/patterns.ts` around lines 2 - 7, The export entries in patterns.ts use relative paths; update each to the project alias form starting with "@/". Replace the module specifiers for ColorPicker (default and ColorPickerProps), PostShareModal, table-filter-tabs, and utm-campaign-tabs (including the type export for CampaignType and TabType) to use "@/components/..." instead of "./components/..." so the file exports: ColorPicker, ColorPickerProps, PostShareModal, table-filter-tabs exports, and utm-campaign-tabs exports all import via the @ alias.
🤖 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/shade/src/app.ts`:
- Around line 2-4: The barrel currently re-exports internal modules using
relative paths; update the re-export sources to use the project alias (@"...")
instead of "./..." so imports follow the coding guideline. Specifically, change
the export sources for the symbols ShadeApp (default export) and ShadeAppProps
(type) from './shade-app' to the corresponding alias path (e.g., '@/shade-app'
or the correct rooted alias that points to that module), and change the export
source for useFocusContext from './providers/shade-provider' to the aliased path
(e.g., '@/providers/shade-provider'); keep the exported symbol names identical
and only replace the module specifiers with the `@-alias`.
In `@apps/shade/src/index.ts`:
- Around line 87-93: The wildcard export ("export * from '@/lib/utils'") is
leaking the entire utils surface and bypassing your curated exports (cn,
debounce, kebabToPascalCase, formatTimestamp, etc.) and the deprecated
domain/transitional helpers (formatUrl, formatQueryDate, isValidDomain, ...).
Remove the wildcard export and only re-export the explicit curated symbols you
intend to expose (the two export lines already shown), ensuring both the primary
helpers (cn, debounce, ...) and the deprecated helpers (formatUrl,
formatQueryDate, ...) are listed; then run type checks to confirm no missing
exports and update the explicit lists if any additional utilities must remain
public.
---
Nitpick comments:
In `@apps/shade/src/components.ts`:
- Around line 2-59: Update all internal relative export paths to use the project
alias: replace every export from './components/ui/...' with
'@/components/ui/...' (including the named Icon export: "IconComponents as
Icon") and replace the SVG asset exports like "ReactComponent as FacebookLogo",
"GhostLogo", "GhostOrb", "GoogleLogo", "TwitterLogo", "XLogo" to import from
'@/assets/images/...' instead of './assets/images/...'; leave external package
exports (e.g., DropdownMenuCheckboxItemProps from
'@radix-ui/react-dropdown-menu') unchanged.
In `@apps/shade/src/patterns.ts`:
- Around line 2-7: The export entries in patterns.ts use relative paths; update
each to the project alias form starting with "@/". Replace the module specifiers
for ColorPicker (default and ColorPickerProps), PostShareModal,
table-filter-tabs, and utm-campaign-tabs (including the type export for
CampaignType and TabType) to use "@/components/..." instead of
"./components/..." so the file exports: ColorPicker, ColorPickerProps,
PostShareModal, table-filter-tabs exports, and utm-campaign-tabs exports all
import via the @ alias.
In `@apps/shade/src/primitives.ts`:
- Around line 2-7: Update the internal re-exports in primitives.ts to use the
project alias instead of relative paths: replace './components/...' imports with
'@/components/...' for each export (e.g., export * from
'@/components/layout/page', export {ErrorPage} from
'@/components/layout/error-page', export * from '@/components/layout/heading',
export * from '@/components/layout/header', export * from
'@/components/layout/list-header', export * from
'@/components/layout/view-header') so the file follows the apps/shade/src alias
convention.
In `@apps/shade/src/tokens.ts`:
- Line 2: Rename the exported constant SHADE_TOKEN_NAMES to camelCase (e.g.,
shadeTokenNames) across the codebase: update the export in
apps/shade/src/tokens.ts and replace all references/imports (search for
SHADE_TOKEN_NAMES) to the new identifier so imports, type annotations, and
usages in functions/components continue to work; ensure any re-exports or tests
referencing SHADE_TOKEN_NAMES are updated and run the type checker to catch
missed occurrences.
In `@apps/shade/src/utils.ts`:
- Around line 5-21: Replace the local relative re-exports with the project alias
paths: change the default export for useGlobalDirtyState to export from
"@/hooks/use-global-dirty-state", change useSimplePagination to export from
"@/hooks/use-simple-pagination", and change the collective re-exports (cn,
debounce, kebabToPascalCase, formatTimestamp, formatNumber, formatDuration,
formatPercentage, formatDisplayDate, formatDisplayTime, getCountryFlag,
stringToHslColor, abbreviateNumber) to export from "@/lib/utils" so all three
re-export statements use the `@` alias instead of relative `./` paths.
🪄 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: 2cb71748-3352-4e73-bb4e-30b72e6549b2
📒 Files selected for processing (13)
apps/shade/package.jsonapps/shade/src/app.tsapps/shade/src/components.tsapps/shade/src/docs/architecture.mdxapps/shade/src/docs/contributing.mdxapps/shade/src/index.tsapps/shade/src/patterns.tsapps/shade/src/primitives.tsapps/shade/src/tokens.tsapps/shade/src/utils.tsapps/shade/styles.cssapps/shade/theme-variables.cssapps/shade/tokens.css
ref DES-1325 Replaced per-app lint deny-lists with physical utility module boundaries.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
apps/shade/src/lib/app-utils.ts (5)
311-311: Avoid mutating moment objects; use.clone()before.startOf().
moment.startOf()mutates the moment object in place. While this currently works becauseitemDateis recreated each iteration, it's a fragile pattern that can introduce subtle bugs if the code is modified later.Proposed fix
- currentWeek = itemDate.startOf('week'); + currentWeek = itemDate.clone().startOf('week');- currentMonth = itemDate.startOf('month'); + currentMonth = itemDate.clone().startOf('month');Also applies to: 355-355
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/shade/src/lib/app-utils.ts` at line 311, Avoid mutating the original moment by cloning before calling startOf: replace uses where you call itemDate.startOf('week') (assigning to currentWeek) with itemDate.clone().startOf('week') so itemDate remains unchanged; apply the same change for the second occurrence referenced around the same block (the other itemDate.startOf('week') at ~355) to ensure both currentWeek assignments use clone().
281-378: Extract aggregation logic to reduce duplication and cognitive complexity.The weekly and monthly aggregation blocks (lines 286-329 and 330-374) share nearly identical logic. SonarCloud flags cognitive complexity failures for these inner functions. Consider extracting a shared aggregation helper.
Proposed refactor sketch
type AggregationUnit = 'week' | 'month'; const aggregateByUnit = <T extends {date: string}>( data: T[], unit: AggregationUnit, fieldName: keyof T, aggregationType: 'sum' | 'avg' | 'exact' ): T[] => { const aggregatedData: T[] = []; let currentPeriod = moment(data[0].date).clone().startOf(unit); let total = 0; let count = 0; let lastValue = 0; const computeValue = () => { if (aggregationType === 'sum') return total; if (aggregationType === 'avg') return count > 0 ? total / count : 0; return lastValue; }; data.forEach((item, index) => { const itemDate = moment(item.date); if (itemDate.isSame(currentPeriod, unit)) { total += Number(item[fieldName]); count += 1; lastValue = Number(item[fieldName]); } else { aggregatedData.push({ ...data[index - 1], date: currentPeriod.format('YYYY-MM-DD'), [fieldName]: computeValue() } as T); currentPeriod = itemDate.clone().startOf(unit); total = Number(item[fieldName]); count = 1; lastValue = Number(item[fieldName]); } if (index === data.length - 1) { aggregatedData.push({ ...item, date: currentPeriod.format('YYYY-MM-DD'), [fieldName]: computeValue() } as T); } }); return aggregatedData; };Then
sanitizeChartDatabecomes:export const sanitizeChartData = <T extends {date: string}>(...) => { if (!data.length) return []; if (range >= 91 && range <= 356) { return aggregateByUnit(data, 'week', fieldName, aggregationType); } else if (range > 356) { return aggregateByUnit(data, 'month', fieldName, aggregationType); } return data; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/shade/src/lib/app-utils.ts` around lines 281 - 378, The weekly and monthly branches in sanitizeChartData duplicate aggregation logic and raise cognitive complexity; extract that logic into a new helper (e.g., aggregateByUnit) that accepts (data, unit: 'week'|'month', fieldName, aggregationType) and encapsulates currentPeriod, total/count/lastValue handling and a computeValue() function, then replace the weekly and monthly branches to call aggregateByUnit(data, 'week', ...) or aggregateByUnit(data, 'month', ...); ensure sanitizeChartData still returns [] for empty data and delegates aggregation to aggregateByUnit for range checks.
403-416: Minor improvements for optional chaining and array access.Static analysis suggests using optional chaining and
.at()for cleaner code.Proposed fix
export const formatMemberName = (member: {name?: string; email?: string}) => { - return (member.name && member.name.trim()) || member.email || 'Unknown Member'; + return member.name?.trim() || member.email || 'Unknown Member'; }; export const getMemberInitials = (member: {name?: string}) => { const name = formatMemberName(member); const words = name.split(' '); if (words.length >= 2) { - return (words[0][0] + words[words.length - 1][0]).toUpperCase(); + return (words[0][0] + words.at(-1)![0]).toUpperCase(); } return name.substring(0, 2).toUpperCase(); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/shade/src/lib/app-utils.ts` around lines 403 - 416, Refactor getMemberInitials to use optional chaining and safer array access: call formatMemberName(member) as before, then split into words and use words.at(0) and words.at(-1) with optional chaining to read first characters; guard against undefined before accessing [0] and fallback to name?.substring(0,2)?.toUpperCase(); ensure formatMemberName remains unchanged and update getMemberInitials to avoid direct numeric indexing and possible undefined property access.
12-95: Acknowledge high cognitive complexity informatUrl.SonarCloud flags this function with cognitive complexity of 23 (allowed 15). However, URL normalization inherently involves many edge cases (mailto, anchors, protocol-relative, relative paths, etc.). If refactoring is desired, consider extracting early-return cases into separate helper functions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/shade/src/lib/app-utils.ts` around lines 12 - 95, The formatUrl function has high cognitive complexity (SonarCloud >15); to fix, extract discrete early-return/edge-case blocks into small named helper functions (e.g., isNullableCase, normalizeEmail, isAnchorLink, isProtocolRelative, ensureAbsoluteWithBase, looksLikeUrl) and move URL-parsing/relative-to-base logic into a separate function (e.g., computeRelativeUrl or normalizeRelativeToBase) called by formatUrl; keep formatUrl as an orchestrator that calls these new helpers and returns early, preserving current behavior and tests while reducing the complexity metric for formatUrl.
4-4: Use@alias for internal imports.The coding guidelines specify using the
@alias for internal imports.Proposed fix
-import {formatDisplayDate} from './ds-utils'; +import {formatDisplayDate} from '@/lib/ds-utils';As per coding guidelines: "Use the
@alias for internal imports (e.g.,@/lib/utils)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/shade/src/lib/app-utils.ts` at line 4, Replace the relative import of formatDisplayDate in app-utils.ts with the project `@` alias: find the import statement that references './ds-utils' and change it to use the alias (e.g., import { formatDisplayDate } from '@/lib/ds-utils') so internal modules follow the coding guideline; update any other similar relative imports in this file to use the `@` alias as well.apps/shade/src/lib/ds-utils.ts (1)
128-130: UseNumber.isNaNandNumber.isFinitefor stricter type checking.Global
isNaNandisFinitecoerce their argument to a number before checking, which can produce unexpected results (e.g.,isNaN("hello")returnstrue). TheNumber.*variants are stricter and only returntruefor actualNaN/finite number values.Proposed fix
export const formatTimestamp = (timestamp: string) => { const date = new Date(timestamp); const now = new Date(); // Handle invalid dates - if (isNaN(date.getTime())) { + if (Number.isNaN(date.getTime())) { return 'Unknown'; }export const formatNumber = (value: number): string => { - if (isNaN(value) || !isFinite(value)) { + if (Number.isNaN(value) || !Number.isFinite(value)) { return '0'; } return new Intl.NumberFormat('en-US').format(Math.round(value)); };Also applies to: 166-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/shade/src/lib/ds-utils.ts` around lines 128 - 130, Replace the loose global checks with the stricter Number.* variants: where the code currently uses isNaN(date.getTime()) (and the similar check around lines 166-169), change to Number.isNaN(date.getTime()) and also guard with Number.isFinite(date.getTime()) as appropriate; update any branches that rely on isFinite or isNaN to use Number.isFinite and Number.isNaN on the value returned by date.getTime() (refer to the date variable and its getTime() calls) so only actual NaN/finite number values are evaluated.
🤖 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/shade/src/lib/ds-utils.ts`:
- Around line 242-249: Change the types of the saturation and lightness
parameters on stringToHslColor from string to number (they represent 0–100
percentages) and update any callers that pass quoted numerals to pass numeric
literals instead; specifically update the function signature for
stringToHslColor and change calls such as in member-avatar (component
MemberAvatar / file apps/posts/src/components/member-avatar.tsx) that currently
pass `'75'` and `'55'` to pass 75 and 55 as numbers so type checking and
concatenation in the return value remain correct.
---
Nitpick comments:
In `@apps/shade/src/lib/app-utils.ts`:
- Line 311: Avoid mutating the original moment by cloning before calling
startOf: replace uses where you call itemDate.startOf('week') (assigning to
currentWeek) with itemDate.clone().startOf('week') so itemDate remains
unchanged; apply the same change for the second occurrence referenced around the
same block (the other itemDate.startOf('week') at ~355) to ensure both
currentWeek assignments use clone().
- Around line 281-378: The weekly and monthly branches in sanitizeChartData
duplicate aggregation logic and raise cognitive complexity; extract that logic
into a new helper (e.g., aggregateByUnit) that accepts (data, unit:
'week'|'month', fieldName, aggregationType) and encapsulates currentPeriod,
total/count/lastValue handling and a computeValue() function, then replace the
weekly and monthly branches to call aggregateByUnit(data, 'week', ...) or
aggregateByUnit(data, 'month', ...); ensure sanitizeChartData still returns []
for empty data and delegates aggregation to aggregateByUnit for range checks.
- Around line 403-416: Refactor getMemberInitials to use optional chaining and
safer array access: call formatMemberName(member) as before, then split into
words and use words.at(0) and words.at(-1) with optional chaining to read first
characters; guard against undefined before accessing [0] and fallback to
name?.substring(0,2)?.toUpperCase(); ensure formatMemberName remains unchanged
and update getMemberInitials to avoid direct numeric indexing and possible
undefined property access.
- Around line 12-95: The formatUrl function has high cognitive complexity
(SonarCloud >15); to fix, extract discrete early-return/edge-case blocks into
small named helper functions (e.g., isNullableCase, normalizeEmail,
isAnchorLink, isProtocolRelative, ensureAbsoluteWithBase, looksLikeUrl) and move
URL-parsing/relative-to-base logic into a separate function (e.g.,
computeRelativeUrl or normalizeRelativeToBase) called by formatUrl; keep
formatUrl as an orchestrator that calls these new helpers and returns early,
preserving current behavior and tests while reducing the complexity metric for
formatUrl.
- Line 4: Replace the relative import of formatDisplayDate in app-utils.ts with
the project `@` alias: find the import statement that references './ds-utils'
and change it to use the alias (e.g., import { formatDisplayDate } from
'@/lib/ds-utils') so internal modules follow the coding guideline; update any
other similar relative imports in this file to use the `@` alias as well.
In `@apps/shade/src/lib/ds-utils.ts`:
- Around line 128-130: Replace the loose global checks with the stricter
Number.* variants: where the code currently uses isNaN(date.getTime()) (and the
similar check around lines 166-169), change to Number.isNaN(date.getTime()) and
also guard with Number.isFinite(date.getTime()) as appropriate; update any
branches that rely on isFinite or isNaN to use Number.isFinite and Number.isNaN
on the value returned by date.getTime() (refer to the date variable and its
getTime() calls) so only actual NaN/finite number values are evaluated.
🪄 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: c0e457e0-9abc-4b93-abc2-825e12bebc83
📒 Files selected for processing (6)
apps/shade/src/app.tsapps/shade/src/index.tsapps/shade/src/lib/app-utils.tsapps/shade/src/lib/ds-utils.tsapps/shade/src/lib/utils.tsapps/shade/src/utils.ts
✅ Files skipped from review due to trivial changes (2)
- apps/shade/src/app.ts
- apps/shade/src/utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/shade/src/index.ts
ref https://linear.app/ghost/issue/DES-1325 Wave A migrates ShadeApp and ShadeAppProps consumption off the root entrypoint so app-shell APIs are consumed via the dedicated app lane.
ref https://linear.app/ghost/issue/DES-1325 Wave B moves consumer imports from the root Shade entrypoint to @tryghost/shade/components and @tryghost/shade/primitives to keep layer boundaries explicit during MS-1 migration.
ref https://linear.app/ghost/issue/DES-1325 Wave C migrates filter/value-source contracts and feature compositions off root and components entrypoints so pattern ownership stays explicit in the subpath API.
ref https://linear.app/ghost/issue/DES-1325 Wave D moves remaining DS-safe and domain helper imports from the root Shade entrypoint to @tryghost/shade/utils and @tryghost/shade/app for explicit lane ownership.
ref https://linear.app/ghost/issue/DES-1325 Phase 5 removes app/utils symbols from the root barrel, publishes migration mapping docs, and blocks new root imports in consuming apps via path-based lint guardrails.
…xports # Conflicts: # apps/posts/src/components/label-picker/label-filter-renderer.tsx # apps/posts/src/components/label-picker/label-picker.tsx # apps/shade/src/index.ts
Kept DES-1325 focused on Shade export-surface and import migration only.
This keeps DES-1325 free of unrelated package version changes.
Aligned barrel re-exports with the @/* import guideline used in shade.
Updated stats tests to mock @tryghost/shade/app and /utils directly, aligned date mocks with Moment-based app helpers, and fixed newsletters type issues surfaced during CI build.
Resolved posts TS errors by aligning percentage and color helper types with Shade signatures after export-surface changes.
|


ref https://linear.app/ghost/issue/DES-1325/split-root-exports