Watcher exception resolved#318
Conversation
Signed-off-by: msivasubramaniaan <msivasub@redhat.com>
msivasubramaniaan
requested review from
adietish,
azatsarynnyy and
vrubezhny
as code owners
|
Warning Review limit reached
More reviews will be available in 17 minutes and 40 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesDevWorkspaceWatchManager API cleanup and downstream adaptation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #318 +/- ##
==========================================
+ Coverage 0.00% 23.44% +23.44%
==========================================
Files 4 101 +97
Lines 26 4201 +4175
Branches 0 777 +777
==========================================
+ Hits 0 985 +985
- Misses 26 3085 +3059
- Partials 0 131 +131 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/kotlin/com/redhat/devtools/gateway/devworkspace/DevWorkspaceWatcher.kt (1)
89-89: Keep the explicit parameter requirement for clarity, but this is optional refactoring.The default argument
emptyMap()is never invoked in the current codebase—allstart()calls inDevSpacesWorkspacesStepView.ktexplicitly passlastResourceVersions(e.g., line 192), so there is no actual silent no-op risk today. However, removing the default argument is still good practice to make the API contract explicit and prevent future callers from accidentally omitting namespaces. Consider:- fun start(lastResourceVersions: Map<String, String?> = emptyMap()) { + fun start(lastResourceVersions: Map<String, String?>) {Apply the same change to the
WorkspacesWatch.start()wrapper (line 572 inDevSpacesWorkspacesStepView.kt).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/com/redhat/devtools/gateway/devworkspace/DevWorkspaceWatcher.kt` at line 89, Remove the default argument `emptyMap()` from the `lastResourceVersions` parameter in the `start` function signature in DevWorkspaceWatcher to make the API contract explicit and prevent future callers from accidentally omitting this required parameter. Since all current callers explicitly pass the argument anyway, this change will not break existing code. Apply the same refactoring to the `WorkspacesWatch.start()` wrapper method in DevSpacesWorkspacesStepView.kt to keep the API consistent across both implementations.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/kotlin/com/redhat/devtools/gateway/view/steps/DevSpacesWorkspacesStepView.kt`:
- Around line 208-210: The current code uses a conditional let block that only
adds the namespace to lastResourceVersions when resourceVersion is not null,
which prevents namespaces with null resourceVersion from being included in the
map. Since DevWorkspaceWatchManager.start() iterates over map entries to create
watchers, missing namespaces will never have their watchers started. Modify the
code to always add the namespace entry to lastResourceVersions regardless of
whether dwListResult.resourceVersion is null, and only update the value when
resourceVersion is present. This ensures all namespaces are tracked in the map
and can be processed by the watcher manager.
---
Nitpick comments:
In
`@src/main/kotlin/com/redhat/devtools/gateway/devworkspace/DevWorkspaceWatcher.kt`:
- Line 89: Remove the default argument `emptyMap()` from the
`lastResourceVersions` parameter in the `start` function signature in
DevWorkspaceWatcher to make the API contract explicit and prevent future callers
from accidentally omitting this required parameter. Since all current callers
explicitly pass the argument anyway, this change will not break existing code.
Apply the same refactoring to the `WorkspacesWatch.start()` wrapper method in
DevSpacesWorkspacesStepView.kt to keep the API consistent across both
implementations.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 0a2909c3-dff7-41cf-830e-7a7d3d9a7d25
📒 Files selected for processing (2)
src/main/kotlin/com/redhat/devtools/gateway/devworkspace/DevWorkspaceWatcher.ktsrc/main/kotlin/com/redhat/devtools/gateway/view/steps/DevSpacesWorkspacesStepView.kt
Signed-off-by: msivasubramaniaan <msivasub@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/kotlin/com/redhat/devtools/gateway/devworkspace/DevWorkspaces.kt (1)
287-287: ⚡ Quick winAvoid re-creating
DevWorkspacesinside the poll loopThis loop runs every second;
DevWorkspaces(client).get(...)allocates a new wrapper each iteration. Useget(namespace, name)directly on the current instance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/com/redhat/devtools/gateway/devworkspace/DevWorkspaces.kt` at line 287, The poll loop that runs every second is inefficiently re-allocating a new DevWorkspaces wrapper instance on each iteration by calling DevWorkspaces(client).get(namespace, name). Instead, call the get method directly on the current DevWorkspaces instance to avoid unnecessary object allocation in the tight polling loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/kotlin/com/redhat/devtools/gateway/devworkspace/DevWorkspaces.kt`:
- Around line 286-290: The catch block in the retry loop within DevWorkspaces is
currently catching all Exception types, which means it will retry on permanent
failures like authentication or permission errors instead of failing fast.
Replace the generic catch (_: Exception) clause to only catch
transient/temporary failure exceptions (such as timeout exceptions, connection
reset exceptions, or temporary HTTP status codes like 429 or 503). Let other
exceptions like authentication or API contract failures propagate immediately so
the failure is reported right away instead of being retried until timeout.
---
Nitpick comments:
In `@src/main/kotlin/com/redhat/devtools/gateway/devworkspace/DevWorkspaces.kt`:
- Line 287: The poll loop that runs every second is inefficiently re-allocating
a new DevWorkspaces wrapper instance on each iteration by calling
DevWorkspaces(client).get(namespace, name). Instead, call the get method
directly on the current DevWorkspaces instance to avoid unnecessary object
allocation in the tight polling loop.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: db7f3152-3a91-4cea-a95c-f524fa2261bd
📒 Files selected for processing (3)
src/main/kotlin/com/redhat/devtools/gateway/devworkspace/DevWorkspaceWatcher.ktsrc/main/kotlin/com/redhat/devtools/gateway/devworkspace/DevWorkspaces.ktsrc/main/kotlin/com/redhat/devtools/gateway/view/steps/DevSpacesWorkspacesStepView.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/kotlin/com/redhat/devtools/gateway/view/steps/DevSpacesWorkspacesStepView.kt
- src/main/kotlin/com/redhat/devtools/gateway/devworkspace/DevWorkspaceWatcher.kt
Signed-off-by: msivasubramaniaan <msivasub@redhat.com>
Signed-off-by: msivasubramaniaan <msivasub@redhat.com>
2 participants
Fixes: https://redhat.atlassian.net/browse/CRW-11309