Improve task UI overflow handling and docker update rollback#237
Merged
jamiepine merged 4 commits intoFeb 26, 2026
Conversation
…ban fills the area and shrinks on smaller screens
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughModified AgentTasks.tsx to reformat imports, introduce Markdown rendering for task descriptions in the Task Detail dialog, and adjust Kanban board layout from horizontal overflow to vertical scrolling with flex-wrapping. No public signatures altered. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
interface/src/routes/AgentTasks.tsx (1)
553-553: Minor: Redundantoverflow-autoon description wrapper.The parent div at line 535 already handles scrolling with
overflow-y-auto. The nestedoverflow-autohere is unnecessary and could cause confusing nested scrollbars if the description is very long.Proposed fix
{task.description && ( - <div className="overflow-auto"> + <div> <label className="mb-1 block text-xs text-ink-dull"> Description </label>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/AgentTasks.tsx` at line 553, Remove the redundant scrolling class from the nested description wrapper: in AgentTasks.tsx locate the inner div that currently has className="overflow-auto" (the one inside the parent that already uses "overflow-y-auto") and remove the "overflow-auto" class so only the parent controls scrolling and avoid nested scrollbars; leave any other classes or content intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/update.rs`:
- Around line 512-534: The rollback block currently discards Results with `let _
=` for calls to `docker.rename_container` and `docker.remove_container`; change
these to handle and log any errors instead of ignoring them by matching or using
`if let Err(e) = ...` and call the project logger (e.g.,
`error!`/`log::error!`/`tracing::error!`) including contextual identifiers
(`new_container.id`, `container_id`, `temp_name`, `container_name`) and a short
message for each operation (`rename_container` and `remove_container`) so
failures during the best-effort rollback are recorded for debugging.
---
Nitpick comments:
In `@interface/src/routes/AgentTasks.tsx`:
- Line 553: Remove the redundant scrolling class from the nested description
wrapper: in AgentTasks.tsx locate the inner div that currently has
className="overflow-auto" (the one inside the parent that already uses
"overflow-y-auto") and remove the "overflow-auto" class so only the parent
controls scrolling and avoid nested scrollbars; leave any other classes or
content intact.
Comment on lines
+512
to
+534
| let _ = docker | ||
| .rename_container( | ||
| &new_container.id, | ||
| bollard::container::RenameContainerOptions { name: &temp_name }, | ||
| ) | ||
| .await; | ||
| let _ = docker | ||
| .rename_container( | ||
| &container_id, | ||
| bollard::container::RenameContainerOptions { | ||
| name: &container_name, | ||
| }, | ||
| ) | ||
| .await; | ||
| let _ = docker | ||
| .remove_container( | ||
| &new_container.id, | ||
| Some(bollard::container::RemoveContainerOptions { | ||
| force: true, | ||
| ..Default::default() | ||
| }), | ||
| ) | ||
| .await; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Log rollback failures instead of silently discarding Results.
The rollback operations use let _ = to discard Results, which violates the coding guideline: "Don't silently discard errors; no let _ = on Results. Handle, log, or propagate errors."
Even in best-effort rollback scenarios, logging failures helps with debugging when updates go wrong.
Proposed fix
- let _ = docker
+ if let Err(e) = docker
.rename_container(
&new_container.id,
bollard::container::RenameContainerOptions { name: &temp_name },
)
- .await;
- let _ = docker
+ .await
+ {
+ tracing::warn!(%e, "rollback: failed to rename new container back to temp name");
+ }
+ if let Err(e) = docker
.rename_container(
&container_id,
bollard::container::RenameContainerOptions {
name: &container_name,
},
)
- .await;
- let _ = docker
+ .await
+ {
+ tracing::warn!(%e, "rollback: failed to rename old container back to original name");
+ }
+ if let Err(e) = docker
.remove_container(
&new_container.id,
Some(bollard::container::RemoveContainerOptions {
force: true,
..Default::default()
}),
)
- .await;
+ .await
+ {
+ tracing::warn!(%e, "rollback: failed to remove new container");
+ }As per coding guidelines: "Don't silently discard errors; no let _ = on Results. Handle, log, or propagate errors."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let _ = docker | |
| .rename_container( | |
| &new_container.id, | |
| bollard::container::RenameContainerOptions { name: &temp_name }, | |
| ) | |
| .await; | |
| let _ = docker | |
| .rename_container( | |
| &container_id, | |
| bollard::container::RenameContainerOptions { | |
| name: &container_name, | |
| }, | |
| ) | |
| .await; | |
| let _ = docker | |
| .remove_container( | |
| &new_container.id, | |
| Some(bollard::container::RemoveContainerOptions { | |
| force: true, | |
| ..Default::default() | |
| }), | |
| ) | |
| .await; | |
| if let Err(e) = docker | |
| .rename_container( | |
| &new_container.id, | |
| bollard::container::RenameContainerOptions { name: &temp_name }, | |
| ) | |
| .await | |
| { | |
| tracing::warn!(%e, "rollback: failed to rename new container back to temp name"); | |
| } | |
| if let Err(e) = docker | |
| .rename_container( | |
| &container_id, | |
| bollard::container::RenameContainerOptions { | |
| name: &container_name, | |
| }, | |
| ) | |
| .await | |
| { | |
| tracing::warn!(%e, "rollback: failed to rename old container back to original name"); | |
| } | |
| if let Err(e) = docker | |
| .remove_container( | |
| &new_container.id, | |
| Some(bollard::container::RemoveContainerOptions { | |
| force: true, | |
| ..Default::default() | |
| }), | |
| ) | |
| .await | |
| { | |
| tracing::warn!(%e, "rollback: failed to remove new container"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/update.rs` around lines 512 - 534, The rollback block currently discards
Results with `let _ =` for calls to `docker.rename_container` and
`docker.remove_container`; change these to handle and log any errors instead of
ignoring them by matching or using `if let Err(e) = ...` and call the project
logger (e.g., `error!`/`log::error!`/`tracing::error!`) including contextual
identifiers (`new_container.id`, `container_id`, `temp_name`, `container_name`)
and a short message for each operation (`rename_container` and
`remove_container`) so failures during the best-effort rollback are recorded for
debugging.
jamiepine
approved these changes
Feb 26, 2026
jamiepine
approved these changes
Feb 26, 2026
rktmeister
pushed a commit
to rktmeister/spacebot
that referenced
this pull request
Mar 11, 2026
…-overflow Improve task UI overflow handling and docker update rollback
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation