Enhance the TypeScript types #27
Conversation
prlabeler
bot
added
the
🧹 Refactoring
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #27 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 11 11
Lines 153 153
Branches 16 16
=========================================
Hits 153 153 ☔ View full report in Codecov by Sentry. |
🔦 Lighthouse Result
📊 Score Scale
|
WalkthroughThis pull request modifies several files to enhance type safety and immutability across the application. In Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/types/services.ts (1)
1-5: Consider improving the type definition.
- The type name
infois too generic. Consider a more specific name likeServiceConfigorContactServiceInfoto better convey its purpose.- The type should be exported since it's used in the exported
servicesconstant.- Consider adding JSDoc comments to document the purpose of each field.
+/** + * Configuration for a contact service + */ -type info = { +export type ServiceConfig = { + /** The base URL for the service */ url: string; + /** The icon identifier from the icon set */ icon: string; + /** The brand color in hex format */ color: string; };
🛑 Comments failed to post (1)
src/libs/contact.ts (1)
24-24: 🛠️ Refactor suggestion
Improve type safety by properly declaring readonly types.
The
as constassertion makes the returned object readonly, but this contradicts the mutablecontacttype declaration. This could lead to type inconsistencies where the object is treated as readonly in some places but mutable in others.Consider this approach instead:
-type contact = { url: string; icon: string; color: string }; +type contact = Readonly<{ url: string; icon: string; color: string }>; export const getContact = (service: service, id: string): contact => { // ... - return { url, icon: specified.icon, color: specified.color } as const; + return { url, icon: specified.icon, color: specified.color }; };This change:
- Makes the
contacttype explicitly readonly- Ensures consistent immutability throughout the codebase
- Removes the need for
as constassertion- Better aligns with TypeScript's type system
Committable suggestion skipped: line range outside the PR's diff.
Avoid slow types and make objects readonly.
5ouma
force-pushed
the
refactor-ts-types
branch
from
efea44c to
27e6c7c
Compare
Signed-off-by: Souma <101255979+5ouma@users.noreply.github.com>
|
@coderabbitai full review |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
src/types/meta.ts (2)
1-1: Remove the file extension from the import statement.TypeScript/JavaScript imports typically work better without file extensions. Some bundlers might have issues with explicit ".ts" extensions.
-import type { serviceName } from "./services.ts"; +import type { serviceName } from "./services";
3-9: LGTM! Great improvements to type safety.The changes effectively enhance type safety by:
- Making the
metatype immutable withReadonly- Ensuring contact objects are immutable
- Using the standardized
serviceNametypeConsider extracting the contact type to improve reusability:
+type Contact = Readonly<{ service: serviceName; id: string }>; + export type meta = Readonly<{ name: string; description: string; icon: string; - contacts: Readonly<{ service: serviceName; id: string }>[]; + contacts: Contact[]; homepage: string; }>;src/libs/contact.ts (2)
7-7: LGTM! Enhanced type safety with readonly properties.Making the type readonly prevents accidental mutations, which is a good practice.
Consider adding URL validation to the
urlproperty using a template literal type for additional type safety:type ValidUrl = `https://${string}`; export type contact = Readonly<{ url: ValidUrl; icon: string; color: string }>;
Line range hint
9-24: Consider performance and type safety improvements.The function implementation is correct, but could benefit from some optimizations.
Consider these improvements:
export const getContact = (service: serviceName, id: string): contact => { const specified: contact = services[service]; if (!specified) throw new Error(`Unsupported service: ${service}`); + // Memoize this check to avoid repeated string comparisons + const isServiceFediverse = isFediverse(service); - - if (isFediverse(service)) { + if (isServiceFediverse) { const parts: string[] = id.split("@"); if (parts.length !== 3 || parts[0] !== "") { throw new Error(`Invalid ${service} ID format`); } } - const url: string = isFediverse(service) + const url: string = isServiceFediverse ? `https://${id.split("@")[2]}/${id}` : specified.url + id; return { url, icon: specified.icon, color: specified.color }; };This change:
- Memoizes the Fediverse check to avoid repeated function calls
- Improves readability by using a more descriptive variable name
src/types/services.ts (2)
1-5: Consider enhancing type safety with readonly and stricter types.The
infotype could be strengthened with:
- Readonly properties to prevent accidental mutations
- Template literal type for URL to ensure valid URL formats
- More specific type for color to ensure valid CSS colors
-type info = { - url: string; - icon: string; - color: string; -}; +type info = { + readonly url: `https://${string}` | `mailto:${string}`; + readonly icon: string; + readonly color: string | ''; // Consider using a union of specific color values +};
53-53: LGTM! Type assertion ensures both immutability and type safety.The use of
as const satisfies Record<string, info>is excellent as it:
- Makes the object deeply readonly
- Ensures type checking while preserving literal types
Consider grouping similar services (e.g., social media, communication) using nested objects for better maintainability:
export const services = { social: { Bluesky: { /* ... */ }, Facebook: { /* ... */ }, // ... }, communication: { Mail: { /* ... */ } } } as const satisfies Record<string, Record<string, info>>;src/components/Contact/Contact.astro (1)
9-9: Consider leveraging TypeScript's type inference.While the current implementation is correct, the explicit type annotation on the destructuring assignment might be unnecessary since TypeScript can infer the types from
getContact's return type.-const { url, icon, color }: contact = getContact(service, id); +const { url, icon, color } = getContact(service, id);Also applies to: 11-11
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
src/components/Bio/Bio.astro(1 hunks)src/components/Contact/Contact.astro(2 hunks)src/components/Homepage/Homepage.astro(2 hunks)src/libs/contact.ts(1 hunks)src/types/meta.ts(1 hunks)src/types/services.ts(2 hunks)
🔇 Additional comments (12)
src/libs/contact.ts (2)
1-1: LGTM! Improved type naming.
The rename from service to serviceName is more descriptive and better represents the type's purpose.
3-5: LGTM! Clean and type-safe implementation.
The function is well-typed and correctly identifies Fediverse services.
src/components/Bio/Bio.astro (3)
11-11: LGTM: Clean props destructuring
The destructuring of props follows Astro's best practices and improves code readability by eliminating repetitive props. prefixes while maintaining type safety.
16-19: LGTM: Clean property references
The template correctly uses the destructured properties, maintaining the same functionality while improving code readability.
16-16: Verify the inferSize prop usage
The inferSize={true} prop on the Image component automatically infers image dimensions at build time. Ensure this is intentional as it affects build-time behavior and could impact performance if the images are large or frequently updated.
✅ Verification successful
Let me gather more information about the Image component usage and configuration to better understand the context.
The inferSize prop usage is appropriate and follows best practices
The usage of inferSize={true} in the Image component is correct and well-implemented:
- It's using the official
astro:assetsImage component (version 4.16.10) - The prop is only used once in the codebase, specifically for the avatar image in the Bio component
- Being an avatar image, it's likely a small, static asset where build-time dimension inference is beneficial for layout stability
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of inferSize prop across Image components
# and verify image file locations
# Check for Image component usage patterns
echo "Checking Image component usage patterns:"
rg -A 2 "Image.*inferSize"
# Find all image files to verify they're properly located
echo -e "\nLocating image files referenced in components:"
fd -e png -e jpg -e jpeg -e gif -e webp -e avif
Length of output: 461
Script:
#!/bin/bash
# Check for Image component imports and configuration
echo "Checking Image component imports and configuration:"
rg "import.*Image.*from.*astro" -A 2
# Look for Image component documentation or type definitions
echo -e "\nLooking for Image component documentation:"
fd "astro.d.ts|Image.d.ts|Image.ts"
# Check package.json for astro version
echo -e "\nChecking Astro version:"
cat package.json | grep "astro"
Length of output: 851
src/types/services.ts (1)
55-55: Verify the type rename impact across the codebase.
The rename from service to serviceName is more descriptive and the type derivation using keyof typeof is correct.
Let's verify all usages have been updated:
✅ Verification successful
Type rename has been properly updated across the codebase
The verification shows that:
- No references to the old
servicetype definition remain - All usages of
serviceNameare properly imported and used consistently across the codebase:- Imported in
meta.ts,contact.ts, andContact.astro - Used correctly in function parameters and type definitions
- Imported in
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining uses of the old type name and verify new type usage
# Test 1: Check for any remaining 'service' type references
echo "Checking for old 'service' type references..."
rg "type\s+service\s*=" || rg ":\s*service[\s;]"
# Test 2: Verify all new 'serviceName' usages
echo "Checking new 'serviceName' usage..."
rg "serviceName"
Length of output: 927
src/components/Contact/Contact.astro (4)
4-5: LGTM! Import changes improve type clarity.
The rename from service to serviceName is more descriptive and the addition of the contact type import enhances type safety.
8-8: LGTM! Props type updated consistently.
The Props type definition correctly reflects the type rename from service to serviceName.
15-15: LGTM! Template changes improve maintainability and accessibility.
The use of destructured properties simplifies the code, and the aria-label provides clear context for screen readers.
Also applies to: 18-18, 20-20
Line range hint 24-36: LGTM! Styling implementation is clean and maintainable.
The use of CSS custom properties and well-defined transitions creates a polished user experience.
src/components/Homepage/Homepage.astro (2)
8-9: LGTM! Type safety improvements look good.
The changes enhance type safety by:
- Making the Props type readonly, preventing accidental mutations
- Adding explicit type annotation to props variable
This aligns well with the PR objectives of improving TypeScript types and making objects read-only.
38-38: Skip reviewing this formatting change.
This is just an empty line addition for better code formatting.
close #
✏️ Description
Avoid slow types and make objects read-only.
🔄 Type of the Change