Complete major migration tasks for the new site #6538
Conversation
…ocation) directly
# Conflicts: # site-new/package-lock.json
…ort url outbound click
haneepark
requested review from
ikhoon,
jrhee17,
minwoox and
trustin
as code owners
|
Important Review skippedMore than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review. 65 files out of 205 files are above the max files limit of 100. Please upgrade to Pro plan to get higher limits. You can disable this status message by setting the WalkthroughThis PR restructures a Docusaurus documentation site by migrating content to Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
♻️ Duplicate comments (5)
site-new/src/content/docs/advanced/athenz.mdx (1)
1-1: Import path uses@sitealias.Consistent with the import change in
setup.mdx. Ensure the alias configuration andversions.jsonfile are in place (per review ofsetup.mdx).site-new/src/content/docs/tutorials/thrift/index.mdx (1)
6-6: Import path uses@sitealias.Consistent with the pattern in other files. Verification of
@sitealias andversions.jsonlocation applies here as well.site-new/src/content/docs/advanced/scalapb.mdx (1)
1-1: Import path uses@sitealias.Consistent with the pattern in other files.
site-new/src/content/docs/advanced/kotlin.mdx (1)
1-1: Import path uses@sitealias.Consistent with the pattern in other files.
site-new/src/content/docs/tutorials/rest/index.mdx (1)
13-13: Import path uses@sitealias.Consistent with the pattern in other files.
🟡 Minor comments (16)
site-new/src/content/blog/en/2020-07-03-reactive-streams-armeria-2.mdx-387-387 (1)
387-387: Fix typo in the footnote.Line 387 contains "Subsclasses" which should be "Subclasses".
Apply this diff:
- - Check Direct Known Subsclasses on [SimpleDecoratingHttpService](https://javadoc.io/doc/com.linecorp.armeria/armeria-javadoc/latest/com/linecorp/armeria/server/SimpleDecoratingHttpService.html) and [SimpleDecoratingClient](https://javadoc.io/doc/com.linecorp.armeria/armeria-javadoc/latest/com/linecorp/armeria/client/SimpleDecoratingClient.html) + - Check Direct Known Subclasses on [SimpleDecoratingHttpService](https://javadoc.io/doc/com.linecorp.armeria/armeria-javadoc/latest/com/linecorp/armeria/server/SimpleDecoratingHttpService.html) and [SimpleDecoratingClient](https://javadoc.io/doc/com.linecorp.armeria/armeria-javadoc/latest/com/linecorp/armeria/client/SimpleDecoratingClient.html)site-new/src/content/blog/en/2020-07-03-reactive-streams-armeria-2.mdx-287-287 (1)
287-287: Fix typo in the code comment.Line 287 contains "StreamObservser" which should be "StreamObserver".
Apply this diff:
-2. Convert to gRPC `StreamObservser` and send the data to an external location. +2. Convert to gRPC `StreamObserver` and send the data to an external location.site-new/src/content/blog/ko/2020-02-18-reactive-streams-armeria-1.mdx-136-136 (1)
136-136: Fix typo in the link text.Line 136 contains "Reative Streams TCK" which should be "Reactive Streams TCK".
Apply this diff:
-이 명세에 맞춰 직접 구현한 기능은 [Reative Streams TCK](https://github.com/reactive-streams/reactive-streams-jvm/tree/master/tck)라는 툴로 검증할 수 있는데요. 해당 분야의 전문가가 아니라면 모든 규칙을 만족하도록 구현하는 게 꽤나 까다로운 일입니다. 특히 `Publisher`를 구현하는 게 어렵습니다. +이 명세에 맞춰 직접 구현한 기능은 [Reactive Streams TCK](https://github.com/reactive-streams/reactive-streams-jvm/tree/master/tck)라는 툴로 검증할 수 있는데요. 해당 분야의 전문가가 아니라면 모든 규칙을 만족하도록 구현하는 게 꽤나 까다로운 일입니다. 특히 `Publisher`를 구현하는 게 어렵습니다.site-new/src/content/blog/en/2021-07-09-monitoring-prometheus-metrics-from-armeria.mdx-38-39 (1)
38-39: Minor wording/consistency fixes in the Prometheus metrics articleThe post is technically solid; a few tiny textual tweaks would improve readability and consistency:
- Add missing spaces around “GET” and “API”:
-I've also prepared a simple REST API that responds to a `/hello/{seq}`GET request with "Success" or "Failure" for the purpose of our exercise. +I've also prepared a simple REST API that responds to a `/hello/{seq}` GET request with "Success" or "Failure" for the purpose of our exercise.-Also check if a request with the `/metrics`API properly returns the Prometheus metrics as a response. +Also check if a request with the `/metrics` API properly returns the Prometheus metrics as a response.
- Metric name consistency:
Earlier, the examples focus on
my_http_service_requests_total, but the explanation later mentionsarmeria_server_requests_total. To align with the examples:-The `armeria_server_requests_total` metric is a [Counter](https://prometheus.io/docs/concepts/metric_types/#counter)-type metric that only continues to increase until you reset it. +The `my_http_service_requests_total` metric is a [Counter](https://prometheus.io/docs/concepts/metric_types/#counter)-type metric that only continues to increase until you reset it.
- Remove an extra space before a period:
-What you should instead focus on, is how many times it either succeeded or failed in a given time frame . +What you should instead focus on, is how many times it either succeeded or failed in a given time frame.Also applies to: 101-102, 191-195
site-new/src/content/blog/en/2021-08-04-customizing-armeria-metrics.mdx-47-58 (1)
47-58: Correct a few Java snippet/encoding issues in the metrics customization articleThe overall content is great, but a few code/encoding glitches will confuse readers copying snippets:
- Chained
ServerBuildercalls split after a semicolonCurrent snippet:
ServerBuilder sb = Server.builder(); .http(8083) .meterRegistry(meterRegistry);This isn’t valid Java. It should chain off
Server.builder()without the early semicolon:-ServerBuilder sb = Server.builder(); - .http(8083) - .meterRegistry(meterRegistry); +ServerBuilder sb = Server.builder() + .http(8083) + .meterRegistry(meterRegistry);
- Extra closing parenthesis in the lambda example
MeterIdPrefixFunction.ofDefault("my.http.service") .andThen((registry, log, prefix) -> prefix.append(log.requestHeaders().method().name())))There’s one
)too many. Suggested fix:-MeterIdPrefixFunction.ofDefault("my.http.service") - .andThen((registry, log, prefix) -> prefix.append(log.requestHeaders().method().name()))) +MeterIdPrefixFunction.ofDefault("my.http.service") + .andThen((registry, log, prefix) -> + prefix.append(log.requestHeaders().method().name()))
- Incomplete / corrupted snippet in the gRPC metrics section
The
annotatedService+ gRPC service snippet currently has an unterminated builder call and mojibake (ì��ë�µ). A minimal, syntactically correct version that still illustrates the intent could be:-sb.annotatedService(new MyAnnotatedService(), - MetricCollectingService.builder(MeterIdPrefixFunction.ofDefault("my.http.service") - // ... ì��ë�µ ... -sb.service(GrpcService.builder() - .addService(new MyGrpcService()) - .build(), - MetricCollectingService.newDecorator(GrpcMeterIdPrefixFunction.of("my.grpc.service"))); +sb.annotatedService(new MyAnnotatedService(), + MetricCollectingService.builder(MeterIdPrefixFunction.ofDefault("my.http.service")) + .newDecorator()); +sb.service(GrpcService.builder() + .addService(new MyGrpcService()) + .build(), + MetricCollectingService.newDecorator( + GrpcMeterIdPrefixFunction.of("my.grpc.service"))); sb.service("/metrics", PrometheusExpositionService.of(meterRegistry.getPrometheusRegistry()));(Adjust the details if you prefer to re‑introduce the earlier
andThencustomizer; the main point is to ensure this block compiles and remove the mojibake comment.)Also applies to: 76-79, 215-225
site-new/src/content/blog/en/2016-08-04-applying-circuitbreaker-to-channel-gateway.mdx-69-70 (1)
69-70: Fix a few small doc/code typos in the CircuitBreaker articleThere are a couple of minor issues that will trip up readers, especially when copying code:
- Backticks inside Java code block (invalid Java if copied):
return new `CircuitBreakerBuilder`(name()).exceptionFilter(exceptionFilter())Consider removing the inline backticks so the snippet is valid Java:
- public CircuitBreaker circuitBreaker(CircuitBreakerListener listener) { - return new `CircuitBreakerBuilder`(name()).exceptionFilter(exceptionFilter()) - .listener(listener) - .build(); - } + public CircuitBreaker circuitBreaker(CircuitBreakerListener listener) { + return new CircuitBreakerBuilder(name()).exceptionFilter(exceptionFilter()) + .listener(listener) + .build(); + }
- Grammar typo (“For you information”)
-For you information, the actual code has code related to IMON Logger1 to check if `CircuitBreaker` is working as intended. +For your information, the actual code has code related to IMON Logger1 to check if `CircuitBreaker` is working as intended.
- Spelling of
counterSlidingWindowin the table-| counterSlidingWindow | Duration | 20 seconds | Recent `counterSlidngWindow` values are used to determine whether CircuitBreaker should open or not. The default value is set to only recognize the requests received in the last 20 seconds. | +| counterSlidingWindow | Duration | 20 seconds | Recent `counterSlidingWindow` values are used to determine whether CircuitBreaker should open or not. The default value is set to only recognize the requests received in the last 20 seconds. |Also applies to: 141-148, 166-167
site-new/src/content/blog/en/2018-09-13-making-a-basic-server-with-java-armeria.mdx-124-131 (1)
124-131: Tiny punctuation fix in the basic server articleThe tutorial reads well; there’s just one small punctuation glitch:
-Let's focus on getting our Armeria server up and running.Open a web browser, enter the URL, http://127.0.0.1:8080/hello, and you will be greeted with `"Hello, Armeria...!"`. +Let's focus on getting our Armeria server up and running. Open a web browser, enter the URL, http://127.0.0.1:8080/hello, and you will be greeted with `"Hello, Armeria...!"`.Everything else looks good for a faithful 2018‑era guide.
site-new/README.md-40-40 (1)
40-40: Address markdown linting issues: Remove or properly format console prompts.Lines 40 and 48 show dollar signs (
$) without command output, which violates markdown best practices (MD014). For easier copy-paste, either remove the$prefix or include the expected output on a following line.Apply this diff:
- $ GIT_USER=<Your GitHub username> npm run deploy + GIT_USER=<Your GitHub username> npm run deploy If you are using GitHub pages for hosting, this command is a convenient way to build the website and push to the `gh-pages` branch. ### Generating release notes - $ npm run release-note <version> + npm run release-note <version>Also applies to: 48-48
site-new/src/content/blog/ja/2020-03-26-reactive-streams-armeria-1.mdx-2-125 (1)
2-125: Fix typo and resolve date inconsistencyTwo issues to address:
Line ~109: Typo in "Reative Streams TCK"—should be "Reactive Streams TCK"
Front matter date mismatch: The file path indicates
2020-03-26while the frontmatter showsdate: 2020-05-06T10:00. Update one to match the other to maintain consistency for readers and RSS feeds.site-new/src/content/blog/en/2020-05-06-reactive-streams-armeria-1.mdx-61-196 (1)
61-196: Fix a few minor typos in the English articleContent is solid; there are just a couple of small spelling issues worth fixing:
-... take a look into the [observer patten](http://reactivex.io/documentation/observable.html), push method, and pull method ... +... take a look into the [observer pattern](http://reactivex.io/documentation/observable.html), push method, and pull method ... -Netflix is responsible for the development of RxJava, Pivotal for WebFlux, and Lightbend for Akka, an implemntation of distributed processing actor models. +Netflix is responsible for the development of RxJava, Pivotal for WebFlux, and Lightbend for Akka, an implementation of distributed processing actor models. -`Observable` in RxJava can be converted to Aremeria's `HttpResponse` or Project Reactor's `Flux` through Reactive Streams. +`Observable` in RxJava can be converted to Armeria's `HttpResponse` or Project Reactor's `Flux` through Reactive Streams.site-new/src/content/blog/en/2019-05-14-armeria-sprint-1.mdx-145-146 (1)
145-146: Fix split Markdown link text for “The In-Person Event Handbook”The last bullet currently splits the link text into two separate links (
[Th](...)and[e In-Person Event Handbook ...](...)), which looks broken in the rendered article. Suggest consolidating into a single link:-- [Th](https://opensourceevents.github.io/)[e In-Person Event Handbook by opensource-events](https://opensourceevents.github.io/) +- [The In-Person Event Handbook by opensource-events](https://opensourceevents.github.io/)site-new/src/content/blog/en/2019-04-16-thank-you-for-contributing-to-armeria.mdx-125-175 (1)
125-175: Fix mojibake em dashes and one awkward sentence in the English “Thank you” postThere are a couple of visible text issues worth cleaning up:
- Mojibake for em dashes
On Line 128 (and a few other lines such as around Lines 153, 164, and 172), the sequenceâ��appears in place of an em dash. This likely came from an encoding round‑trip and will render oddly on the site. Consider replacing these with a proper em dash (—) or a simple-. Example:-... when I was working at LINE in the LINE Shop team â�� it was a really great coincidence ... +... when I was working at LINE in the LINE Shop team — it was a really great coincidence ...
- Unnatural wording in the contributor bullet
On Line 107, the phrase “was very clever idea that it was worth of a prize” is ungrammatical. A clearer version would be:-- Your optimization of weighted round-robin load balancing algorithm was very clever idea that it was worth of a prize. +- Your optimization of the weighted round-robin load balancing algorithm was such a clever idea that it was worthy of a prize.These are content-only fixes and won’t affect any surrounding logic or structure.
site-new/src/plugins/rss-generator.ts-48-55 (1)
48-55: RSS link title attribute should be escaped for HTML entities.The
options.titleis interpolated directly into an HTML attribute. If the title contains quotes or special HTML characters, it could break the HTML or introduce XSS vectors.Consider escaping the title:
injectHtmlTags({ content }) { return { headTags: [ - `<link rel="alternate" type="application/rss+xml" href="${options.path}/rss.xml" title="${options.title} RSS Feed">`, + `<link rel="alternate" type="application/rss+xml" href="${options.path}/rss.xml" title="${escapeHtml(options.title)} RSS Feed">`, ], }; },You'll need to add a simple escape helper:
function escapeHtml(str: string): string { return str.replace(/[&<>"']/g, (c) => ({ '&': '&', '<': '<', '>': '>', '"': '"', "'": ''' }[c] || c)); }site-new/release-note.ts-244-246 (1)
244-246: Array mutation withshift()modifies the source data.Using
shift()mutateschange.results, which could cause issues if the samePullRequestobject is processed multiple times or referenced elsewhere.if (change.category === Category.Dependency) { - builder.push(change.results.shift()); + builder.push(change.results[0]); } else {Or if all results should be included for dependencies:
builder.push(...change.results);site-new/src/content/blog/ja/2020-01-14-monitoring-a-spring-boot-app-in-kubernetes-what-i-learned-from-devoxx-belgium-2019.mdx-317-317 (1)
317-317: Minor Japanese grammar issue: ら抜き言葉The phrase "引っ張ってこれる" should be "引っ張ってこられる" (missing ら in the potential form).
-けれども、`deployment.yaml`ファイルを変更して、Prometheusが今回のアプリケーションからメトリクスを引っ張ってこれるように次の設定を追加することをお勧めします。 +けれども、`deployment.yaml`ファイルを変更して、Prometheusが今回のアプリケーションからメトリクスを引っ張ってこられるように次の設定を追加することをお勧めします。site-new/src/plugins/rss-generator.ts-244-253 (1)
244-253: Channel title and description should use CDATA or be escaped.Unlike item titles which use CDATA wrapping, the channel
<title>and<description>are inserted directly. Ifoptions.titleoroptions.descriptioncontain XML special characters (<,>,&), the RSS feed will be malformed.Apply CDATA wrapping consistently:
return `<?xml version="1.0" encoding="utf-8"?> <rss version="2.0" xmlns:dc="http://purl.org/dc/elements/1.1/" xmlns:content="http://purl.org/rss/1.0/modules/content/"> <channel> - <title>${options.title}</title> + <title><![CDATA[${options.title}]]></title> <link>${channelLink}</link> - ${options.description ? `<description>${options.description}</description>` : ''} + ${options.description ? `<description><![CDATA[${options.description}]]></description>` : ''}
🧹 Nitpick comments (16)
site-new/src/theme/NotFound/Content/index.module.css (1)
34-42: Minor:animation-play-state: runningis the browser default.Line 41 (
animation-play-state: running;) is redundant sincerunningis the initial value. Removing it slightly reduces CSS verbosity without functional change.opacity: 0; transition-property: opacity; animation-name: pulse; animation-duration: 2.5s; animation-timing-function: ease-out; animation-iteration-count: infinite; - animation-play-state: running; }site-new/src/css/custom.css (1)
126-146: Refactor pseudo-element display property and add keyboard focus state.Two concerns:
Line 135:
display: flexis semantically unusual for a pseudo-element with no nested content. Usedisplay: inline-blockinstead, which is more appropriate for a replaced icon element.Missing focus state: Header links need a visible focus indicator for keyboard navigation (WCAG 2.4.7). Add a focus state to ensure keyboard users can see which link they're on.
Apply this diff:
&::before { content: ''; height: 24px; - display: flex; + display: inline-block; mask-repeat: no-repeat; mask-position: center; background-color: var(--ifm-navbar-link-color); transition: background-color var(--ifm-transition-fast) var(--ifm-transition-timing-default); } &:hover::before { background-color: var(--ifm-navbar-link-hover-color); } + + &:focus-visible::before { + outline: 2px solid var(--ifm-color-primary); + outline-offset: 2px; + }site-new/src/components/blog-language-selector.tsx (1)
31-42: Optimize initial state to avoid flash of incorrect language during SSR/hydration.The current implementation initializes
selectedto English (items[0]) on line 27, then relies onuseEffectto correct it based on the path. This causes a brief flash where the wrong language displays during SSR and initial client hydration.Consider using a lazy initializer for
useStateto derive the correct initial state immediately:- const [selected, setSelected] = React.useState(items[0]); + const [selected, setSelected] = React.useState(() => { + const path = location.pathname; + if (path.startsWith('/blog/ja')) return items[1]; + if (path.startsWith('/blog/ko')) return items[2]; + return items[0]; + }); const isBrowser = useIsBrowser(); const location = useLocation(); useEffect(() => { - if (isBrowser) { - const path = location.pathname; - if (path.startsWith('/blog/ja')) { - setSelected(items[1]); - } else if (path.startsWith('/blog/ko')) { - setSelected(items[2]); - } else { - setSelected(items[0]); - } - } - }, [location, isBrowser]); + const path = location.pathname; + if (path.startsWith('/blog/ja')) { + setSelected(items[1]); + } else if (path.startsWith('/blog/ko')) { + setSelected(items[2]); + } else { + setSelected(items[0]); + } + }, [location]);Additional note: The
isBrowsercheck insideuseEffectis unnecessary since effects only execute on the client side (they never run during SSR).site-new/src/components/redirect.tsx (1)
37-39: Return null instead of empty string for better React semantics.Returning an empty string works but
nullis more idiomatic for rendering nothing in React.) : ( - '' + null )}site-new/src/components/cookie-consent.tsx (1)
6-24: Move useCallback to component body for proper memoization.The useCallback hook at line 17 is defined inline within the JSX, which means it's recreated on every render, defeating its purpose. Move it to the component body.
Apply this diff:
export const CookieConsent: React.FC = () => { + const handleDecline = useCallback(() => { + window.open('https://tools.google.com/dlpage/gaoptout/'); + }, []); + + return ( -export const CookieConsent: React.FC = () => ( <ReactCookieConsent declineButtonText="Opt out" containerClasses={styles.cookieConsentContainer} contentClasses={styles.cookieConsentContent} buttonClasses={`${styles.cookieConsentAcceptButton} button button--primary`} declineButtonClasses={`${styles.cookieConsentDeclineButton} button button--secondary`} sameSite="strict" enableDeclineButton disableButtonStyles acceptOnScroll - onDecline={useCallback(() => { - window.open('https://tools.google.com/dlpage/gaoptout/'); - }, [])} + onDecline={handleDecline} > This website uses anonymous cookies to ensure we provide you the best experience. </ReactCookieConsent> -); + ); +};site-new/src/theme/BlogPostItem/Header/index.tsx (1)
10-71: i18n header logic is sound; consider future‑proofing for >2 languagesThe
I18nLinkcomponent correctly readsother_languagesfrom front matter and shows an info Admonition only on full blog post pages. For the current setup (each post linking to at most two other locales), the “Korean and Japanese” style output is fine.If you ever add more locales or want to support more than two alternate translations per post, you may want to generalize the rendering from:
{links.length === 1 ? links[0] : <>{links[0]} and {links[1]}</>}to joining all
linkswith commas and an “and” for the last item.site-new/src/content/blog/en/2020-07-08-circuit-breakers-armeria.mdx (3)
46-46: Minor style improvement: Remove redundant "back" in "return the response back".Line 46 reads: "When you receive a response from Server2, you return the response back to the client."
Consider simplifying to: "When you receive a response from Server2, you return it to the client." or "...you return the response to the client."
161-161: Minor style improvement: Remove redundant "first" in "when the server first started".Line 161 reads: "When the server first started, the initial state of
CircuitBreakerwasCLOSED."Consider simplifying to: "When the server started, the initial state of
CircuitBreakerwasCLOSED." The word "first" is redundant with the context.
177-177: Avoid repetition of "again" in the same sentence.Line 177 reads: "This process is repeated again because it is
CLOSEDagain."This creates unnecessary repetition. Consider: "This process repeats as the circuit breaker is now
CLOSED." or "The cycle repeats since it is back to theCLOSEDstate."site-new/src/components/news-redirect.tsx (1)
4-6: HandlelatestNewsPathmore defensively and avoidString()coercionRight now
customFields.latestNewsPathis assumed to exist and is coerced withString(), which would redirect to"undefined"if the config is missing or mis-typed. You can tighten the typing and pass a proper string:-export default function NewsRedirect() { - const { latestNewsPath } = useDocusaurusContext().siteConfig.customFields; - return <Redirect href={String(latestNewsPath)} />; -} +export default function NewsRedirect() { + const { siteConfig } = useDocusaurusContext(); + const { latestNewsPath } = siteConfig.customFields as { + latestNewsPath: string; + }; + return <Redirect href={latestNewsPath} />; +}site-new/src/components/cookie-consent.module.css (1)
1-18: Cookie consent styling looks good; consider trimming!importantif not neededThe layout and margins are consistent with the component usage, and
margin-left: autocorrectly overrides the left value from the shorthand. If ReactCookieConsent’s defaults aren’t fighting you, you could drop some of the!importantflags to make future overrides easier, but it’s fine as-is if they’re required to win specificity.site-new/src/components/star-begging.module.css (1)
1-42: Consider using a scoped class instead of the global#starBeggingidBecause this is a CSS module, keeping
#starBeggingas a global id slightly undercuts the benefit of local scoping and could lead to accidental conflicts if another element uses the same id. Consider switching to a class and wiring it via the importedstylesmap, e.g.:-#starBegging { +.starBegging { transition: transform .25s; transform: scale(0); ... } - -#starBegging.on { +.starBegging.on { transform: scale(1); } -#starBegging.off { +.starBegging.off { transform: translateX(400px); }This would also require updating the
star-begging.tsxmarkup to useclassName={styles.starBegging}instead of anid.site-new/src/plugins/short-url.ts (1)
1-41: Short URL plugin implementation looks solid; only tiny optional nitThe plugin cleanly generates per‑short‑URL data files and routes and looks correct for Docusaurus’
contentLoadedAPI. The defensiveoptions.shortUrls || []also makes the plugin robust against misconfigured options.If you ever want the types to reflect that behavior more precisely, you could mark
shortUrlsas optional inShortUrlPluginOptionsand use nullish coalescing (options.shortUrls ?? []), but that’s purely a style/readability tweak and not required.site-new/src/components/star-begging.tsx (1)
7-71: Tighten typing and lifecycle behavior for keyboard handling and timeoutThe component behavior looks good overall. Two small TS/React hygiene improvements:
Type the keyboard event and avoid deprecated
keyCodeExplicitly typing the event and checking
e.keyis clearer and future‑proof:
- function handleKeyDown(e) {
- if (e.keyCode === 13) {
close();- }
- }
- function handleKeyDown(e: React.KeyboardEvent) {
- if (e.key === 'Enter') {
e.preventDefault();close();- }
- }
Clear the show‑popup timeout on unmount
To avoid setting state on an unmounted component if the layout ever stops rendering this, store and clear the timeout:
useEffect(() => {
- if (localStorage.getItem('dismissed') !== 'true') {
setTimeout(() => setDisplay(true), 10000); // 10 seconds- }
- }, []);
- if (localStorage.getItem('dismissed') === 'true') {
return;- }
- const timer = window.setTimeout(() => setDisplay(true), 10000); // 10 seconds
- return () => window.clearTimeout(timer);
- }, []);
These are non‑blocking but will make the component more robust and type‑safe.
site-new/src/plugins/rss-generator.ts (1)
100-112: Falling back to current date may mask extraction issues.When date extraction fails, the function silently returns
new Date(). This could cause items to appear with incorrect dates in the RSS feed without any indication of the problem.Consider logging a warning when falling back:
function extractDate(html: string): Date { const dateFromHeader = extractDateFromReleaseNotes(html); if (dateFromHeader && !isNaN(dateFromHeader.getTime())) { return dateFromHeader; } const dateFromMetaOrTime = extractDateFromMetaOrTime(html); if (dateFromMetaOrTime && !isNaN(dateFromMetaOrTime.getTime())) { return dateFromMetaOrTime; } + console.warn('[rss-generator] Could not extract date, using current date as fallback'); return new Date(); }site-new/release-note.ts (1)
192-209: DOM scraping for GitHub data is fragile.
getLinkedIssuesandgetParticipantsrely on specific CSS selectors (.Truncate.my-1,a.participant-avatar) from GitHub's HTML. These selectors may break if GitHub updates their UI.Consider adding error handling or fallback behavior when selectors don't match.
function getLinkedIssues(dom: JSDOM): number[] { const elements = dom.window.document.querySelectorAll('.Truncate.my-1'); + if (elements.length === 0) { + console.warn('[release-note] No linked issues found - GitHub UI may have changed'); + } return Array.from(elements) .map((el: any) => el.children[0].href) + .filter((href): href is string => !!href) .map((href: string) => href.replace(/^.*\/issues\//, '')) .map((number: string) => parseInt(number, 10)) + .filter((n) => !isNaN(n)); }
ikhoon
left a comment
ikhoon
left a comment
There was a problem hiding this comment.
Thanks a lot, @haneepark! 🙇♂️🚀
minwoox
left a comment
minwoox
left a comment
There was a problem hiding this comment.
I've added the site-new/build.gradle file. Thanks! 🙇
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
site-new/build.gradle (1)
113-172: Consider parameterizing the hard-coded javadoc.io URL.The API index task hard-codes the javadoc.io URL with "latest" version (line 132). While this is convenient, it assumes the published javadoc is always the latest. If this should instead reference the current build version or a configurable URL, consider extracting this as a project property.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
site-new/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
settings.gradle(1 hunks)site-new/build.gradle(1 hunks)site-new/src/content/community/index.mdx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- site-new/src/content/community/index.mdx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: build-ubicloud-standard-16-jdk-21-snapshot-blockhound
- GitHub Check: build-ubicloud-standard-16-jdk-25
- GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-17-coverage
- GitHub Check: build-ubicloud-standard-16-jdk-17-leak
- GitHub Check: build-ubicloud-standard-16-jdk-8
- GitHub Check: build-windows-latest-jdk-25
- GitHub Check: build-macos-latest-jdk-25
- GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-11
- GitHub Check: build-ubicloud-standard-16-jdk-11
- GitHub Check: site
- GitHub Check: Kubernetes Chaos test
- GitHub Check: lint
- GitHub Check: flaky-tests
- GitHub Check: Summary
🔇 Additional comments (3)
settings.gradle (1)
265-265: Site-new project properly included in multi-project build.The addition of
:site-newaligns with the PR's new site infrastructure. Placement and configuration are consistent with the existing:siteproject.site-new/build.gradle (2)
1-30: Imports and configuration are well-justified.The jsoup dependency is appropriately added to the buildscript for HTML parsing in the API index task. Node/npm versions are current and reasonable.
32-91: Task orchestration and dependency wiring are correct.The
generateSiteSourcescomposite task properly aggregates all prerequisite tasks, and downstream tasks correctly depend on it. The decision to excludenpmCleanfromclean(lines 37–38) is intentional and well-documented to avoid unnecessary npm installations.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6538 +/- ##
============================================
- Coverage 74.46% 74.17% -0.29%
- Complexity 22234 23401 +1167
============================================
Files 1963 2102 +139
Lines 82437 87579 +5142
Branches 10764 11498 +734
============================================
+ Hits 61385 64964 +3579
- Misses 15918 17124 +1206
- Partials 5134 5491 +357 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Thank you, @minwoox ! I've reviewed your changes and they look good to me. |
3 participants
Modifications:
(FYI, Algolia's
apikeyin the docusaurous.config.js is safe to commit.)Result: