Allow style customization in the documentation service #6235
Conversation
Ivan-Montes
requested review from
ikhoon,
jrhee17,
minwoox and
trustin
as code owners
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6235 +/- ##
============================================
- Coverage 74.46% 74.17% -0.30%
- Complexity 22234 23415 +1181
============================================
Files 1963 2103 +140
Lines 82437 87631 +5194
Branches 10764 11506 +742
============================================
+ Hits 61385 64998 +3613
- Misses 15918 17142 +1224
- Partials 5134 5491 +357 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
core/src/main/java/com/linecorp/armeria/server/docs/DocServiceBuilder.java
Outdated
Show resolved
Hide resolved
Ivan-Montes
force-pushed
the
feature/allow-custom-doc-service
branch
from
ce4d1cc to
37379c0
Compare
|
I think this approach aligns better with your suggestions now. Let me know what you think! We can now customize the appearance of:
|
jrhee17
left a comment
jrhee17
left a comment
There was a problem hiding this comment.
Overall looks good, left only nits
core/src/main/java/com/linecorp/armeria/server/docs/DocService.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/docs/DocService.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/docs/DocServiceBuilder.java
Outdated
Show resolved
Hide resolved
| * @param url the input string to escape | ||
| * @return the escaped string | ||
| */ | ||
| private static String escapeForJavaScriptUrl(String url) { |
There was a problem hiding this comment.
Question) Would it make sense to use URLEncoder.encode(url, "UTF-8") instead?
There was a problem hiding this comment.
Yes, it's definitely a more convenient method.
There was a problem hiding this comment.
I ultimately had to use a different approach because the href attribute of the <link> HTML tag didn’t correctly interpret the value generated by the URLEncoder.encode(url, "UTF-8") method, and the icon wasn't displayed.
There was a problem hiding this comment.
I see. At least on chrome it seems like the browser does encoding somewhat automatically for us. What do you think of removing the encoding logic/validation for uri?
This will also allow users to use base64 variants. e.g. <link src="data:image/png;base64..." which may be useful for users who don't necessarily want to host the favicon separately.
There was a problem hiding this comment.
Currently, I use this method to validate:
private static void validateFaviconUri(String uri, String key) {
requireNonNull(uri, key);
checkArgument(!uri.trim().isEmpty(), "%s is empty.", key);
checkArgument(isValidUri(uri), "%s uri invalid.", key);
checkArgument(hasValidFaviconExtension(uri), "%s extension not allowed.",key);
}
The suggestion would be to delete isValidUri and hasValidFaviconExtension. Do you think we should implement any kind of validation, or just accept the user input as is?
There was a problem hiding this comment.
The suggestion would be to delete isValidUri and hasValidFaviconExtension. Do you think we should implement any kind of validation, or just accept the user input as is?
I think it's fine to remove the validation isValidUri and hasValidFaviconExtension.
I don't think we need to be super-strict with validation since the users setting faviconUri will likely be server-owners (as opposed to input from clients to servers), and it's difficult to validate what will work/not work on the browser.
There was a problem hiding this comment.
Understood, I'll implement that approach, thanks.
Ivan-Montes
force-pushed
the
feature/allow-custom-doc-service
branch
from
37379c0 to
c25ee87
Compare
jrhee17
left a comment
jrhee17
left a comment
There was a problem hiding this comment.
Sorry about the delay, looks almost done to me
| * @param input the uri string to validate | ||
| * @return true if is valid | ||
| */ | ||
| public static boolean isValidUri(String input) { |
There was a problem hiding this comment.
nit; I don't think this needs to be public
There was a problem hiding this comment.
You are right, I'll fix it
There was a problem hiding this comment.
Ok, I'll make the change shortly
core/src/main/java/com/linecorp/armeria/server/docs/DocServiceInjectedScriptsUtil.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/docs/DocServiceInjectedScriptsUtil.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/docs/DocServiceInjectedScriptsUtil.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/docs/DocServiceInjectedScriptsUtil.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/docs/DocServiceInjectedScriptsUtil.java
Outdated
Show resolved
Hide resolved
| final String webAppTitleKey = "webAppTitle"; | ||
| final Integer webAppTitleMaxSize = 50; |
There was a problem hiding this comment.
Can these values be defined as private static final fields?
There was a problem hiding this comment.
Currently, both scopes are entirely within the function. I don’t think it’s necessary to change them, but it’s also true that there’s no big issue in doing so. What would be the best approach to stay aligned with the project?
There was a problem hiding this comment.
Sorry about the late reply. While it may be trivial, we could at the very least save an allocation if these variables are declared statically.
Also, it looks like the Integer can be a primitive as well.
There was a problem hiding this comment.
Ok, I'll add both suggestions
| checkArgument(!webAppTitle.trim().isEmpty(), "%s is empty.", webAppTitleKey); | ||
| checkArgument(webAppTitle.length() <= webAppTitleMaxSize, | ||
| "%s length exceeds %s.", webAppTitleKey, webAppTitleMaxSize); | ||
| docServiceExtraInfo.putIfAbsent(webAppTitleKey, webAppTitle); |
There was a problem hiding this comment.
While this is internal so I'm less concerned, personally I prefer that a statically typed value is passed to the front-end. (so instead of a map, webAppTitle as a field of its own is passed to the front end)
Let me know what you think
There was a problem hiding this comment.
I've developed my approach using a Map because it provides a flexible structure that can accommodate additional information. There are several classes and constructors between the DocServiceBuilder class and the frontend, and this approach is more maintainable. I think we should keep it.
There was a problem hiding this comment.
My thought was that just looking at the field name docServiceExtraInfo , it is difficult for others (maintainers and contributors) to guess what the meaning of this field is. If flexibility is the main concern, we could theoretically just serve a huge map instead of defining typed schema.
Having said this, i'm not too strong on this since it's an internal detail and can be modified later if needed.
There was a problem hiding this comment.
Thanks for the feedback! I see your point regarding the field name and flexibility. In this case, since it's an internal detail and can be adjusted later if needed, I'd prefer to stick with the current approach for now
jrhee17
left a comment
jrhee17
left a comment
There was a problem hiding this comment.
Sorry about the delay, looks pretty much done
| final String webAppTitleKey = "webAppTitle"; | ||
| final Integer webAppTitleMaxSize = 50; |
There was a problem hiding this comment.
Sorry about the late reply. While it may be trivial, we could at the very least save an allocation if these variables are declared statically.
Also, it looks like the Integer can be a primitive as well.
| checkArgument(!webAppTitle.trim().isEmpty(), "%s is empty.", webAppTitleKey); | ||
| checkArgument(webAppTitle.length() <= webAppTitleMaxSize, | ||
| "%s length exceeds %s.", webAppTitleKey, webAppTitleMaxSize); | ||
| docServiceExtraInfo.putIfAbsent(webAppTitleKey, webAppTitle); |
There was a problem hiding this comment.
My thought was that just looking at the field name docServiceExtraInfo , it is difficult for others (maintainers and contributors) to guess what the meaning of this field is. If flexibility is the main concern, we could theoretically just serve a huge map instead of defining typed schema.
Having said this, i'm not too strong on this since it's an internal detail and can be modified later if needed.
| * @return the js script | ||
| */ | ||
| public static String withGotoBackground(String color) { | ||
| final String gotoBackgroundKey = "gotoBackground"; |
There was a problem hiding this comment.
Ditto, these fields can all be static final
There was a problem hiding this comment.
There are three public methods in that class that use a variable related to the key. I'll go ahead and apply your suggestion — making those fields static final — to all of them
| * @param input the uri string to validate | ||
| * @return true if is valid | ||
| */ | ||
| public static boolean isValidUri(String input) { |
| * @param color the color string to validate | ||
| * @param key the name used in error messages | ||
| */ | ||
| private static void validateHexColor(String color, String key) { |
There was a problem hiding this comment.
I think it is easy for users to inject a hexadecimal string without the #, and I'm not sure if rejecting this case is good for user experience.
I prefer that #aaa, aaa, #aaaaaa, aaaaaa are all accepted. When aaa, aaaaaa is supplied, we can prepend a # for users. What do you think?
There was a problem hiding this comment.
Agreed, I'll fix it.
| public static String withFavicon(String uri) { | ||
| final String faviconKey = "favicon"; | ||
| validateFaviconUri(uri, faviconKey); | ||
|
|
||
| return buildFaviconScript(escapeJavaScriptUri(uri)); | ||
| } |
There was a problem hiding this comment.
I prefer flexibility so that multiple formats are supported
<link src="data:image/png;base64..."
<link src="https://..."
While I do think it's possible to strongly validate, I think there are too many cases and it's probably not worth it since the users using this method will be the owners of each server.
What do you think of just relaxing the validation?
| public static String withFavicon(String uri) { | |
| final String faviconKey = "favicon"; | |
| validateFaviconUri(uri, faviconKey); | |
| return buildFaviconScript(escapeJavaScriptUri(uri)); | |
| } | |
| public static String withFavicon(String uri) { | |
| return buildFaviconScript(escapeJavaScriptUri(uri)); | |
| } |
There was a problem hiding this comment.
Thanks! I'll use your example to implement a more flexible validation
|
You can ignore the test failures for now - seems like CI is broken at the moment |
Ivan-Montes
force-pushed
the
feature/allow-custom-doc-service
branch
from
3e7d0c8 to
19a5123
Compare
Ivan-Montes
force-pushed
the
feature/allow-custom-doc-service
branch
from
408eaf1 to
e95ea70
Compare
Ivan-Montes
force-pushed
the
feature/allow-custom-doc-service
branch
from
e95ea70 to
8ca14e8
Compare
Ivan-Montes
force-pushed
the
feature/allow-custom-doc-service
branch
from
8ca14e8 to
b9e9463
Compare
WalkthroughThreads a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
core/src/main/java/com/linecorp/armeria/server/docs/ServiceSpecification.java (2)
131-131: Consider makingdocServiceExtraInfoimmutable for consistency.All other collection fields in this class use immutable types (
ImmutableSortedSet,ImmutableList). The mutableHashMaphere is inconsistent and could lead to unexpected external mutation.Consider using an immutable map or making a defensive copy:
- private Map<String, String> docServiceExtraInfo = new HashMap<>(); + private Map<String, String> docServiceExtraInfo = Map.of();And update the setter:
public void setDocServiceExtraInfo(Map<String, String> docServiceExtraInfo) { - this.docServiceExtraInfo = requireNonNull(docServiceExtraInfo,"docServiceExtraInfo"); + this.docServiceExtraInfo = Map.copyOf(requireNonNull(docServiceExtraInfo, "docServiceExtraInfo")); }
263-269: Minor: Fix Javadoc formatting and add missing space.The Javadoc has a double asterisk and the
requireNonNullcall is missing a space after the comma./** - * * Sets the additional information for the document service. + * Sets the additional information for the document service. * @param docServiceExtraInfo a map containing the extra information for the document service. */ public void setDocServiceExtraInfo(Map<String, String> docServiceExtraInfo) { - this.docServiceExtraInfo = requireNonNull(docServiceExtraInfo,"docServiceExtraInfo"); + this.docServiceExtraInfo = requireNonNull(docServiceExtraInfo, "docServiceExtraInfo"); }docs-client/src/containers/App/index.tsx (1)
609-609: Redundant fallback:composedTitlealways containsdefaultTitle.Since
composedTitleis built as`${...}${defaultTitle} ${artifactVersion}`, it will always includedefaultTitleand never be falsy. The|| defaultTitlefallback is unreachable.Consider simplifying:
- <title>{composedTitle || defaultTitle}</title> + <title>{composedTitle}</title>Same applies to line 627.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
core/src/main/java/com/linecorp/armeria/server/docs/DocService.java(5 hunks)core/src/main/java/com/linecorp/armeria/server/docs/DocServiceBuilder.java(3 hunks)core/src/main/java/com/linecorp/armeria/server/docs/DocServiceInjectableScripts.java(1 hunks)core/src/main/java/com/linecorp/armeria/server/docs/ServiceSpecification.java(2 hunks)core/src/test/java/com/linecorp/armeria/server/docs/DocServiceInjectableScriptsTest.java(1 hunks)docs-client/src/components/GotoSelect/index.tsx(1 hunks)docs-client/src/containers/App/index.tsx(5 hunks)docs-client/src/lib/specification.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: - The primary coding conventions and style guide for this project are defined insite/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.2. Specific check for
@UnstableApi
- Review all newly added public classes and methods to ensure they have the
@UnstableApiannotation.- However, this annotation is NOT required under the following conditions:
- If the class or method is located in a package containing
.internal.- If a public method is part of a class that is already annotated with
@UnstableApi.
Files:
core/src/main/java/com/linecorp/armeria/server/docs/ServiceSpecification.javacore/src/main/java/com/linecorp/armeria/server/docs/DocServiceBuilder.javacore/src/main/java/com/linecorp/armeria/server/docs/DocServiceInjectableScripts.javacore/src/test/java/com/linecorp/armeria/server/docs/DocServiceInjectableScriptsTest.javacore/src/main/java/com/linecorp/armeria/server/docs/DocService.java
🧬 Code graph analysis (2)
core/src/test/java/com/linecorp/armeria/server/docs/DocServiceInjectableScriptsTest.java (1)
core/src/main/java/com/linecorp/armeria/server/docs/DocServiceInjectableScripts.java (1)
DocServiceInjectableScripts(26-191)
docs-client/src/containers/App/index.tsx (1)
docs-client/src/lib/specification.tsx (2)
SpecificationData(104-112)Specification(153-321)
⏰ 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). (13)
- GitHub Check: Kubernetes Chaos test
- GitHub Check: build-ubicloud-standard-16-jdk-21-snapshot-blockhound
- GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-17-coverage
- GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-11
- GitHub Check: build-ubicloud-standard-16-jdk-11
- GitHub Check: build-ubicloud-standard-16-jdk-17-leak
- GitHub Check: build-windows-latest-jdk-25
- GitHub Check: build-ubicloud-standard-16-jdk-8
- GitHub Check: build-macos-latest-jdk-25
- GitHub Check: build-ubicloud-standard-16-jdk-25
- GitHub Check: site
- GitHub Check: lint
- GitHub Check: flaky-tests
🔇 Additional comments (9)
docs-client/src/components/GotoSelect/index.tsx (1)
125-125: LGTM!The addition of the
data-js-targetattribute enables DOM targeting for style customization, consistent with the pattern used in the App component'smain-app-bar.docs-client/src/lib/specification.tsx (2)
111-111: LGTM!The new field properly extends the interface to receive extra info from the backend.
227-229: LGTM!The getter safely exposes the extra info. Since
this.datais deep-cloned in the constructor (line 173), returning the reference here is safe from external mutation of the original input.core/src/main/java/com/linecorp/armeria/server/docs/DocServiceBuilder.java (1)
569-569: Consider behavior whenwebAppTitleis called multiple times.Using
putIfAbsentmeans subsequent calls are silently ignored. If a user accidentally calls this method twice with different values, only the first value takes effect without warning. Consider either:
- Documenting this behavior in the Javadoc
- Using
putto allow overwriting (last-call-wins)- Throwing an exception if already set
docs-client/src/containers/App/index.tsx (4)
459-459: LGTM!Properly initializes the dummy specification with an empty
docServiceExtraInfoto match the updatedSpecificationDatainterface.
490-495: LGTM!Clean extraction of the custom title from extra info with proper conditional setting.
531-537: LGTM!The title composition follows the agreed approach from past reviews (no useMemo, no sanitization). The logic correctly prepends the custom title when available.
613-613: LGTM!The
data-js-targetattribute enables DOM targeting for style customization via injected scripts.core/src/main/java/com/linecorp/armeria/server/docs/DocService.java (1)
133-138: LGTM! Proper propagation ofdocServiceExtraInfo.The changes correctly thread
docServiceExtraInfothrough the DocService construction path:
- Default constructor provides an empty HashMap
- All constructors properly propagate the parameter
- SpecificationLoader stores it with proper null checking (Line 271, addressing previous review feedback)
- The extra info is set on the ServiceSpecification during generation (Line 333)
Also applies to: 143-164, 261-272, 326-336
core/src/main/java/com/linecorp/armeria/server/docs/DocServiceBuilder.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/docs/DocServiceInjectableScripts.java
Show resolved
Hide resolved
core/src/test/java/com/linecorp/armeria/server/docs/DocServiceInjectableScriptsTest.java
Show resolved
Hide resolved
--- Related: line#5900 **Motivation:** This change addresses feedback from multiple users requesting the ability to customize the appearance of the documentation service. The customizable elements now include: - Title - Browser tab title - Header bar background color - Color for the `GotoSelect` component - Buttons colors in the `Debug` component **Modifications:** Backend - In `DocServiceBuilder`, add a `Map` to store relevant data and a builder method for the documentation title. - In `DocService`, modify the constructor to include a new attribute. - In Static Nested `SpecificationLoader`, adjust the constructor and the `generateServiceSpecification` method. - In `ServiceSpecification`, create a private `Map` and add Getter and Setter methods. Frontend - In `lib/specification.tsx`, include a `Record<String, String>` attribute in the `SpecificationData` interface. - In `lib/colors.tsx`, add reusable method to validate color hex code. - In `containers/App/index.tsx`, retrieve the style settings and applied them to the app's title and header. - In `components/GotoSelect/index.tsx`, apply color using inline style. - In `containers/MethodPage/index.tsx`, apply the custom color styling. - In `containers/MethodPage/DebugPage.tsx`, apply custom button color styling via inline styles. **Result:** We can now customize the appearance of: - Title - Browser tab title - Header bar background color - Color for the `GotoSelect` component - Buttons colors in the `Debug` component
…pts` --- Related: line#5900 **Motivation:** This change addresses feedback from multiple users requesting the ability to customize the appearance of the documentation service. The customizable elements now include: - Header title - Header bar background color - Color for the `GotoSelect` component - Browser tab title - Browser tab favicon **Modifications:** Backend - In `DocServiceBuilder`, add a `Map` to store relevant data and a builder method for the documentation title. - In `DocService`, modify the constructor to include a new attribute. - In Static Nested `SpecificationLoader`, adjust the constructor and the `generateServiceSpecification` method. - In `ServiceSpecification`, create a private `Map` and add Getter and Setter methods. - In `DocServiceInjectedScriptsUtil`, add methods to generate customization scripts. - In `DocServiceInjectedScriptsUtilTest`, develop tests for the above logic. Frontend - In `lib/specification.tsx`, include a `Record<String, String>` attribute in the `SpecificationData` interface. - In `containers/App/index.tsx`, retrieve the title and apply it to the app's titles. Add JS hook class. - In `components/GotoSelect/index.tsx`, add JS hook class for custom behavior. **Result:** - Closes: line#5900 We can now customize the appearance of: - Header title - Header bar background color - Color for the `GotoSelect` component - Browser tab title - Browser tab favicon
Ivan-Montes
force-pushed
the
feature/allow-custom-doc-service
branch
from
b9e9463 to
87de0cc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
docs-client/src/containers/App/index.tsx (1)
609-627: Minor: The fallback|| defaultTitleis redundant.Since
composedTitleis always a non-empty string (built via template literal concatenation), the|| defaultTitlefallback will never be reached. Consider simplifying:- <title>{composedTitle || defaultTitle}</title> + <title>{composedTitle}</title>- {composedTitle || defaultTitle} + {composedTitle}This is a minor nit and the current code is harmless.
core/src/main/java/com/linecorp/armeria/server/docs/DocServiceInjectableScripts.java (2)
140-173: Consider escaping newlines and angle brackets for robustness.The
escapeJavaScriptUrimethod handles critical characters well, but consider also escaping:
- Newlines (
\n,\r) which could break the JavaScript string literal- Angle brackets (
<,>) for HTML context safety+ case '\n': + escaped.append("\\n"); + break; + case '\r': + escaped.append("\\r"); + break; + case '<': + escaped.append("\\u003C"); + break; + case '>': + escaped.append("\\u003E"); + break;This adds defense-in-depth against edge cases where the URI might contain these characters.
181-193: Consider inferring MIME type from URI extension.The favicon script hardcodes
type = 'image/x-icon', which is incorrect for PNG (image/png) or SVG (image/svg+xml) favicons. This may cause issues in strict browsers.Consider inferring the type from the URI extension or making it configurable:
// Option: infer from extension private static String inferFaviconMimeType(String uri) { if (uri.endsWith(".png")) return "image/png"; if (uri.endsWith(".svg")) return "image/svg+xml"; return "image/x-icon"; }Alternatively, omit the
typeattribute entirely as modern browsers can usually detect the correct type.core/src/test/java/com/linecorp/armeria/server/docs/DocServiceInjectableScriptsTest.java (1)
57-64: Consider adding tests for edge cases andgotoBackgroundvalidation.The test suite is solid but could benefit from:
- Tests for
gotoBackgroundwith invalid/too-long colors (currently only valid colors tested)- Tests for null and empty string inputs
@Test void titleBackground_givenNullColor_throwsException() { assertThatThrownBy(() -> DocServiceInjectableScripts.titleBackground(null)) .isInstanceOf(NullPointerException.class); } @Test void titleBackground_givenEmptyColor_throwsException() { assertThatThrownBy(() -> DocServiceInjectableScripts.titleBackground("")) .isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("is empty"); }core/src/main/java/com/linecorp/armeria/server/docs/DocService.java (1)
326-336: Consider defensive copy fordocServiceExtraInfo.The
docServiceExtraInfomap is stored by reference and later set on theServiceSpecification. If the caller modifies the map after passing it toDocService, the specification could be affected.Consider making a defensive copy in the constructor:
- this.docServiceExtraInfo = requireNonNull(docServiceExtraInfo, "docServiceExtraInfo"); + this.docServiceExtraInfo = ImmutableMap.copyOf( + requireNonNull(docServiceExtraInfo, "docServiceExtraInfo"));This ensures immutability and prevents unexpected mutations.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
core/src/main/java/com/linecorp/armeria/server/docs/DocService.java(5 hunks)core/src/main/java/com/linecorp/armeria/server/docs/DocServiceBuilder.java(3 hunks)core/src/main/java/com/linecorp/armeria/server/docs/DocServiceInjectableScripts.java(1 hunks)core/src/main/java/com/linecorp/armeria/server/docs/ServiceSpecification.java(2 hunks)core/src/test/java/com/linecorp/armeria/server/docs/DocServiceInjectableScriptsTest.java(1 hunks)docs-client/src/components/GotoSelect/index.tsx(1 hunks)docs-client/src/containers/App/index.tsx(5 hunks)docs-client/src/lib/specification.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- docs-client/src/lib/specification.tsx
- core/src/main/java/com/linecorp/armeria/server/docs/ServiceSpecification.java
- docs-client/src/components/GotoSelect/index.tsx
- core/src/main/java/com/linecorp/armeria/server/docs/DocServiceBuilder.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: - The primary coding conventions and style guide for this project are defined insite/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.2. Specific check for
@UnstableApi
- Review all newly added public classes and methods to ensure they have the
@UnstableApiannotation.- However, this annotation is NOT required under the following conditions:
- If the class or method is located in a package containing
.internalor.testing.- If the class or method is located in a test source set.
- If a public method is part of a class that is already annotated with
@UnstableApi.
Files:
core/src/main/java/com/linecorp/armeria/server/docs/DocServiceInjectableScripts.javacore/src/test/java/com/linecorp/armeria/server/docs/DocServiceInjectableScriptsTest.javacore/src/main/java/com/linecorp/armeria/server/docs/DocService.java
🧬 Code graph analysis (1)
docs-client/src/containers/App/index.tsx (1)
docs-client/src/lib/specification.tsx (2)
SpecificationData(104-112)Specification(153-321)
⏰ 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). (13)
- GitHub Check: Kubernetes Chaos test
- GitHub Check: build-windows-latest-jdk-25
- GitHub Check: build-ubicloud-standard-16-jdk-21-snapshot-blockhound
- GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-17-coverage
- GitHub Check: build-ubicloud-standard-16-jdk-8
- GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-11
- GitHub Check: build-ubicloud-standard-16-jdk-17-leak
- GitHub Check: build-ubicloud-standard-16-jdk-11
- GitHub Check: build-ubicloud-standard-16-jdk-25
- GitHub Check: build-macos-latest-jdk-25
- GitHub Check: flaky-tests
- GitHub Check: lint
- GitHub Check: site
🔇 Additional comments (10)
docs-client/src/containers/App/index.tsx (3)
531-537: LGTM! Title composition logic is clean.The title composition correctly handles the optional
webAppTitleand appends the artifact version. The implementation follows the feedback from past reviews to avoiduseMemofor this lightweight computation.
452-460: LGTM!The
dummySpecificationcorrectly initializesdocServiceExtraInfoas an empty object, matching theSpecificationDatainterface.
490-495: LGTM!The extraction of
webAppTitlefromdocServiceExtraInfois straightforward and correctly handles the case where the value may not be present.core/src/main/java/com/linecorp/armeria/server/docs/DocServiceInjectableScripts.java (3)
85-92: LGTM!The hex color validation is thorough: checks null, empty, length, and format. The regex correctly accepts both 3-digit (
#RGB) and 6-digit (#RRGGBB) hex colors with optional#prefix.
115-121: LGTM!The script generation is safe:
coloris validated as hex format before reaching here, andtargetAttris a hardcoded constant. The block scope{}wrapping prevents variable leakage.
44-75: LGTM!The public API is clean, well-documented, and follows a consistent pattern. The validation-before-building approach ensures safe script generation.
core/src/test/java/com/linecorp/armeria/server/docs/DocServiceInjectableScriptsTest.java (2)
30-55: LGTM!Good test coverage for
titleBackgroundvalidation: valid colors, too-long colors, and invalid hex format. The previous issue with calling the wrong method has been addressed.
66-96: LGTM!The favicon tests properly verify that:
- Valid URIs are correctly escaped (slashes become
\\/)- Malicious URIs with special characters are properly sanitized
The
doesNotContain(uri)assertion correctly verifies the raw input doesn't appear in the output due to escaping.core/src/main/java/com/linecorp/armeria/server/docs/DocService.java (2)
133-164: LGTM!The constructor chain correctly threads
docServiceExtraInfothrough all layers. TherequireNonNullcheck was added as per previous review feedback.
261-272: LGTM!The
SpecificationLoadercorrectly stores and propagatesdocServiceExtraInfowith proper null checking.
jrhee17
left a comment
jrhee17
left a comment
There was a problem hiding this comment.
Sorry about the delay - looks good to me. Thanks for your patience 🙇
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/test/java/com/linecorp/armeria/server/docs/DocServiceInjectableScriptsTest.java (1)
71-105: Good test coverage for titleBackground; consider expanding gotoBackground tests.The
titleBackgroundtests comprehensively cover valid colors, too-long colors, and invalid formats. However,gotoBackgroundonly tests valid colors.If
gotoBackgrounduses the same validation logic astitleBackground, consider adding similar validation tests to ensure the behavior is explicitly documented and protected against regression:@ParameterizedTest @ValueSource(strings = { "#1234567", "#ABCDEFA" }) void gotoBackground_givenTooLongColor_throwsException(String color) { assertThatThrownBy(() -> gotoBackground(color)) .isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("length exceeds"); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/main/java/com/linecorp/armeria/server/docs/DocServiceBuilder.java(4 hunks)core/src/test/java/com/linecorp/armeria/server/docs/DocServiceInjectableScriptsTest.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: - The primary coding conventions and style guide for this project are defined insite/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.2. Specific check for
@UnstableApi
- Review all newly added public classes and methods to ensure they have the
@UnstableApiannotation.- However, this annotation is NOT required under the following conditions:
- If the class or method is located in a package containing
.internalor.testing.- If the class or method is located in a test source set.
- If a public method is part of a class that is already annotated with
@UnstableApi.
Files:
core/src/main/java/com/linecorp/armeria/server/docs/DocServiceBuilder.javacore/src/test/java/com/linecorp/armeria/server/docs/DocServiceInjectableScriptsTest.java
⏰ 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). (13)
- GitHub Check: build-ubicloud-standard-16-jdk-8
- GitHub Check: flaky-tests
- 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-11
- GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-11
- GitHub Check: build-windows-latest-jdk-25
- GitHub Check: build-macos-latest-jdk-25
- GitHub Check: build-ubicloud-standard-16-jdk-21-snapshot-blockhound
- GitHub Check: lint
- GitHub Check: site
- GitHub Check: Kubernetes Chaos test
🔇 Additional comments (6)
core/src/main/java/com/linecorp/armeria/server/docs/DocServiceBuilder.java (3)
48-50: LGTM! Constants and field are well-defined.The constants follow Java naming conventions and are appropriately scoped. The
docServiceExtraInfomap provides a flexible structure for passing extra information to the frontend, as discussed in previous reviews.Also applies to: 69-69
30-30: LGTM! Thread-safe immutable copy ensures safe handoff.The use of
ImmutableMap.copyOf(docServiceExtraInfo)ensures that the extra info is safely passed to theDocServiceconstructor without risk of external modification. The import addition on line 30 is appropriate.Also applies to: 581-581
560-573: LGTM! Solid validation and builder pattern implementation.The method correctly implements validation (null, empty trimmed string, and max length checks) and follows the builder pattern. The
@UnstableApiannotation is properly applied per coding guidelines.Note: The validation uses
trim().isEmpty()to reject whitespace-only strings while storing the original untrimmed value. This allows titles like" My Title "but rejects" ", which is reasonable behavior for preserving user input formatting.Verify that comprehensive tests exist for the
webAppTitlevalidation logic, including edge cases:
- null input (should throw)
- empty string (should throw)
- whitespace-only strings (should throw)
- exactly 50 characters (should pass)
- 51 characters (should throw)
core/src/test/java/com/linecorp/armeria/server/docs/DocServiceInjectableScriptsTest.java (3)
39-59: LGTM! Well-structured test setup.The
ServerExtensionconfiguration effectively tests the integration ofwebAppTitlewith injectable scripts. The test setup provides good coverage of the new API surface.
61-69: Integration test correctly verifies webAppTitle in specification.The test confirms that the
webAppTitlevalue flows through from the builder to the generated specification JSON.Note: The demo mode with
Thread.sleep(Long.MAX_VALUE)on line 64 is unusual but appears intentional for manual testing scenarios.
107-137: Excellent security-conscious testing for favicon URI handling.The tests comprehensively verify both valid URI escaping and sanitization of potentially malicious URIs. The assertions correctly check that:
- The result contains the escaped/sanitized version
- The original input is not present (preventing injection)
This is a strong security practice.
3 participants
Related: #5900
Motivation:
This change addresses feedback from multiple users requesting the ability to customize the appearance of the documentation service. The customizable elements now include:
GotoSelectcomponentDebugcomponentModifications:
Backend
DocServiceBuilder, add aMapto store relevant data and a builder method for the documentation title.DocService, modify the constructor to include a new attribute.SpecificationLoader, adjust the constructor and thegenerateServiceSpecificationmethod.ServiceSpecification, create a privateMapand add Getter and Setter methods.Frontend
lib/specification.tsx, include aRecord<String, String>attribute in theSpecificationDatainterface.lib/colors.tsx, add reusable method to validate color hex code.containers/App/index.tsx, retrieve the style settings and applied them to the app's title and header.components/GotoSelect/index.tsx, apply color using inline style.containers/MethodPage/index.tsx, apply the custom color styling.containers/MethodPage/DebugPage.tsx, apply custom button color styling via inline styles.Result:
We can now customize the appearance of:
GotoSelectcomponentDebugcomponentMain view
Debug modal
