Conversation
📝 WalkthroughWalkthroughRefactors XDS static resource construction to conditionally include the local cluster, bumps multiple dependency versions, upgrades the Gradle wrapper to 9.4.0 and switches wrapper invocation to JAR mode, and increases Playwright webServer timeouts. Changes
Sequence Diagram(s)(Skipped — changes do not introduce a new multi-component sequential feature requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dependencies.toml (1)
80-80:⚠️ Potential issue | 🟡 MinorSnappy version mismatch with PR objectives.
The PR description states Snappy should be upgraded from 1.1.10.5 to 1.1.10.8, but this file still shows
snappy = "1.1.10.5". Was this intentional due to Curator compatibility requirements (as noted in the comment on lines 78-79), or was this update missed?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dependencies.toml` at line 80, The dependencies.toml still pins Snappy to "1.1.10.5" while the PR states it should be upgraded to "1.1.10.8"; either update the snappy line to snappy = "1.1.10.8" or add a clear comment next to the snappy dependency explaining why "1.1.10.5" is intentionally retained for Curator compatibility (referencing the existing Curator compatibility note), and run dependency checks to ensure no compatibility breakage.
🧹 Nitpick comments (1)
client/java-armeria-xds/src/main/java/com/linecorp/centraldogma/client/armeria/xds/XdsCentralDogmaBuilder.java (1)
281-293: Consider conditionally settinglocalClusterNamefor consistency.The change correctly avoids adding the default
localClusterto static resources. However, on line 289,setLocalClusterName(localCluster.getName())is still called unconditionally. WhenlocalClusteris the default instance, this sets an empty string as the local cluster name inClusterManager.If an empty local cluster name is semantically different from not setting it at all, you may want to make this conditional as well:
💡 Optional: Conditionally set local cluster name
+ final ClusterManager.Builder clusterManagerBuilder = ClusterManager.newBuilder(); + if (localCluster != Cluster.getDefaultInstance()) { + clusterManagerBuilder.setLocalClusterName(localCluster.getName()); + } final Bootstrap bootstrap = Bootstrap.newBuilder() - .setClusterManager(ClusterManager.newBuilder() - .setLocalClusterName(localCluster.getName())) + .setClusterManager(clusterManagerBuilder) .setDynamicResources(dynamicResources)If an empty string is acceptable and doesn't trigger pgv validation issues, the current implementation is fine.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/java-armeria-xds/src/main/java/com/linecorp/centraldogma/client/armeria/xds/XdsCentralDogmaBuilder.java` around lines 281 - 293, The Bootstrap builder currently always calls ClusterManager.newBuilder().setLocalClusterName(localCluster.getName()), which will set an empty name when localCluster == Cluster.getDefaultInstance(); change this to only call setLocalClusterName when localCluster is not Cluster.getDefaultInstance() (i.e., guard the call that sets localClusterName using the same condition used for adding to staticResourcesBuilder), so modify the ClusterManager building logic in XdsCentralDogmaBuilder to conditionally setLocalClusterName based on localCluster != Cluster.getDefaultInstance().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@dependencies.toml`:
- Line 80: The dependencies.toml still pins Snappy to "1.1.10.5" while the PR
states it should be upgraded to "1.1.10.8"; either update the snappy line to
snappy = "1.1.10.8" or add a clear comment next to the snappy dependency
explaining why "1.1.10.5" is intentionally retained for Curator compatibility
(referencing the existing Curator compatibility note), and run dependency checks
to ensure no compatibility breakage.
---
Nitpick comments:
In
`@client/java-armeria-xds/src/main/java/com/linecorp/centraldogma/client/armeria/xds/XdsCentralDogmaBuilder.java`:
- Around line 281-293: The Bootstrap builder currently always calls
ClusterManager.newBuilder().setLocalClusterName(localCluster.getName()), which
will set an empty name when localCluster == Cluster.getDefaultInstance(); change
this to only call setLocalClusterName when localCluster is not
Cluster.getDefaultInstance() (i.e., guard the call that sets localClusterName
using the same condition used for adding to staticResourcesBuilder), so modify
the ClusterManager building logic in XdsCentralDogmaBuilder to conditionally
setLocalClusterName based on localCluster != Cluster.getDefaultInstance().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 03ea4c03-5901-4a2a-a842-b30c828a7075
⛔ Files ignored due to path filters (1)
gradle/wrapper/gradle-wrapper.jaris excluded by!**/*.jar
📒 Files selected for processing (5)
client/java-armeria-xds/src/main/java/com/linecorp/centraldogma/client/armeria/xds/XdsCentralDogmaBuilder.javadependencies.tomlgradle/wrapper/gradle-wrapper.propertiesgradlewgradlew.bat
There was a problem hiding this comment.
🧹 Nitpick comments (1)
webapp/playwright.config.ts (1)
55-71: Hoist the shared web server timeout into a constant.
120 * 1000is now duplicated across both server entries. Pulling it into one constant keeps future tuning in sync and makes the intent clearer.♻️ Proposed refactor
+const webServerTimeout = 120 * 1000; + export default defineConfig({ @@ webServer: [ { command: 'npm run backend', url: 'http://127.0.0.1:36462/monitor/l7check', reuseExistingServer: !process.env.CI, - timeout: 120 * 1000, + timeout: webServerTimeout, stdout: 'ignore', stderr: 'pipe', }, { command: 'npm run develop', url: 'http://127.0.0.1:3000', reuseExistingServer: !process.env.CI, - timeout: 120 * 1000, + timeout: webServerTimeout, stdout: 'ignore', stderr: 'pipe', }, ], });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/playwright.config.ts` around lines 55 - 71, The duplicated numeric timeout (120 * 1000) used in the webServer entries should be hoisted into a single constant to avoid drift; create a top-level constant (e.g., const WEB_SERVER_TIMEOUT = 120 * 1000) and replace both timeout: 120 * 1000 occurrences inside the webServer array with timeout: WEB_SERVER_TIMEOUT so future tuning is centralized and intent is clear (refer to the webServer array in playwright.config.ts).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@webapp/playwright.config.ts`:
- Around line 55-71: The duplicated numeric timeout (120 * 1000) used in the
webServer entries should be hoisted into a single constant to avoid drift;
create a top-level constant (e.g., const WEB_SERVER_TIMEOUT = 120 * 1000) and
replace both timeout: 120 * 1000 occurrences inside the webServer array with
timeout: WEB_SERVER_TIMEOUT so future tuning is centralized and intent is clear
(refer to the webServer array in playwright.config.ts).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 13b1f2db-7a72-4adb-a03b-6518d26f0638
📒 Files selected for processing (1)
webapp/playwright.config.ts
3 participants
Modifications
localClusteris set only if the local cluster is set to avoid pgv validation exceptionsDependencies
Misc) timeout for e2e tests has been increased to handle failure: https://github.com/line/centraldogma/actions/runs/22931861466/job/66559027587