feat: Separate Chat and Code tabs in SessionView#122
feat: Separate Chat and Code tabs in SessionView#122TheRealAshik wants to merge 2 commits intomainfrom
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully implements the separation of Chat and Code views using a TabRow, which is a great enhancement for user experience. The use of isCompactMode and showOnlyArtifacts flags to conditionally render content is a good approach. My review focuses on improving code maintainability and adherence to best practices. Specifically, I've suggested removing temporary Python scripts from the commit, replacing magic numbers with constants for better readability, refactoring ActivityItem to simplify its logic, and using theme colors instead of hardcoded values in CompactArtifactView.
| fun CompactArtifactView(title: String, subtitle: String? = null, isError: Boolean = false) { | ||
| val borderColor = if (isError) Color(0xFFEF4444).copy(alpha = 0.3f) else Color.White.copy(alpha = JulesOpacity.subtle) | ||
| val headerBg = if (isError) Color(0xFFEF4444).copy(alpha = JulesOpacity.subtle) else Color.White.copy(alpha = 0.02f) | ||
|
|
||
| Card( | ||
| colors = CardDefaults.cardColors(containerColor = Color(0xFF09090B)), | ||
| shape = JulesShapes.small, | ||
| modifier = Modifier.fillMaxWidth().border(1.dp, borderColor, JulesShapes.small) | ||
| ) { | ||
| Row( | ||
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .background(headerBg) | ||
| .padding(JulesSpacing.m), | ||
| verticalAlignment = Alignment.CenterVertically | ||
| ) { | ||
| Icon( | ||
| if (isError) Icons.Default.Warning else Icons.Default.Code, | ||
| null, | ||
| tint = if (isError) Color(0xFFF87171) else Color(0xFF71717A), | ||
| modifier = Modifier.size(16.dp) | ||
| ) | ||
| Spacer(modifier = Modifier.width(JulesSpacing.s)) | ||
| Column(modifier = Modifier.weight(1f)) { | ||
| Text( | ||
| title, | ||
| color = if (isError) Color(0xFFF87171) else Color(0xFFD4D4D8), | ||
| style = MaterialTheme.typography.bodySmall, | ||
| fontFamily = FontFamily.Monospace, | ||
| maxLines = 1, | ||
| overflow = TextOverflow.Ellipsis | ||
| ) | ||
| if (subtitle != null) { | ||
| Text( | ||
| subtitle, | ||
| color = Color(0xFF71717A), | ||
| fontSize = 10.sp, | ||
| fontFamily = FontFamily.Monospace, | ||
| maxLines = 1, | ||
| overflow = TextOverflow.Ellipsis | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This composable uses several hardcoded hex color values (e.g., #EF4444, #09090B, #F87171, #71717A). This goes against best practices for theming in Compose. All colors should be sourced from MaterialTheme.colorScheme to ensure your UI adapts correctly to different themes and maintains consistency.
Please refactor this component to use theme colors. For example, Color(0xFFEF4444) should likely be MaterialTheme.colorScheme.error or a related color.
fun CompactArtifactView(title: String, subtitle: String? = null, isError: Boolean = false) {
val borderColor = if (isError) MaterialTheme.colorScheme.error.copy(alpha = 0.3f) else MaterialTheme.colorScheme.outline.copy(alpha = JulesOpacity.subtle)
val headerBg = if (isError) MaterialTheme.colorScheme.errorContainer else MaterialTheme.colorScheme.surfaceVariant
Card(
colors = CardDefaults.cardColors(containerColor = MaterialTheme.colorScheme.surfaceContainer),
shape = JulesShapes.small,
modifier = Modifier.fillMaxWidth().border(1.dp, borderColor, JulesShapes.small)
) {
Row(
modifier = Modifier
.fillMaxWidth()
.background(headerBg)
.padding(JulesSpacing.m),
verticalAlignment = Alignment.CenterVertically
) {
Icon(
if (isError) Icons.Default.Warning else Icons.Default.Code,
null,
tint = if (isError) MaterialTheme.colorScheme.error else MaterialTheme.colorScheme.onSurfaceVariant,
modifier = Modifier.size(16.dp)
)
Spacer(modifier = Modifier.width(JulesSpacing.s))
Column(modifier = Modifier.weight(1f)) {
Text(
title,
color = if (isError) MaterialTheme.colorScheme.error else MaterialTheme.colorScheme.onSurface,
style = MaterialTheme.typography.bodySmall,
fontFamily = FontFamily.Monospace,
maxLines = 1,
overflow = TextOverflow.Ellipsis
)
if (subtitle != null) {
Text(
subtitle,
color = MaterialTheme.colorScheme.onSurfaceVariant,
fontSize = 10.sp,
fontFamily = FontFamily.Monospace,
maxLines = 1,
overflow = TextOverflow.Ellipsis
)
}
}
}
}
}| TabRow( | ||
| selectedTabIndex = selectedTabIndex, | ||
| containerColor = MaterialTheme.colorScheme.surface, | ||
| contentColor = MaterialTheme.colorScheme.primary, | ||
| divider = { HorizontalDivider(color = MaterialTheme.colorScheme.outlineVariant) } | ||
| ) { | ||
| Tab( | ||
| selected = selectedTabIndex == 0, | ||
| onClick = { selectedTabIndex = 0 }, | ||
| text = { Text("Chat") } | ||
| ) | ||
| Tab( | ||
| selected = selectedTabIndex == 1, | ||
| onClick = { selectedTabIndex = 1 }, | ||
| text = { Text("Code") } | ||
| ) | ||
| } |
There was a problem hiding this comment.
The TabRow implementation is a bit repetitive. You can make it more concise and scalable by using a loop. This also helps to move away from using magic numbers (0, 1) for tab indices by defining a list of tab titles.
I'd recommend defining val tabs = listOf("Chat", "Code") and then iterating over it to create the tabs.
TabRow(
selectedTabIndex = selectedTabIndex,
containerColor = MaterialTheme.colorScheme.surface,
contentColor = MaterialTheme.colorScheme.primary,
divider = { HorizontalDivider(color = MaterialTheme.colorScheme.outlineVariant) }
) {
listOf("Chat", "Code").forEachIndexed { index, title ->
Tab(
selected = selectedTabIndex == index,
onClick = { selectedTabIndex = index },
text = { Text(title) }
)
}
}|
|
||
| @Composable | ||
| fun ActivityItem(activity: JulesActivity, defaultCardState: Boolean, onApprovePlan: (String?) -> Unit) { | ||
| fun ActivityItem(activity: JulesActivity, defaultCardState: Boolean, onApprovePlan: (String?) -> Unit, isCompactMode: Boolean = false, showOnlyArtifacts: Boolean = false) { |
There was a problem hiding this comment.
The logic within ActivityItem has become complex due to the introduction of the showOnlyArtifacts flag. The repeated if (!showOnlyArtifacts) checks make the code harder to follow.
Consider refactoring this composable to separate the rendering logic for the 'Chat' and 'Code' tabs more cleanly. You could use a top-level if-else on showOnlyArtifacts to handle the two different views. This would eliminate the need for repeated checks and improve readability.
For example:
@Composable
fun ActivityItem(...) {
// ... common setup ...
if (showOnlyArtifacts) {
// Render for 'Code' tab (only artifacts)
if (hasArtifacts) {
Column(modifier = Modifier.padding(start = 40.dp)) {
activity.artifacts.forEach { artifact ->
ArtifactView(artifact, defaultCardState, isCompactMode = false)
Spacer(modifier = Modifier.height(JulesSpacing.s))
}
}
}
} else {
// Render for 'Chat' tab (full view with compact artifacts)
// ... all the existing logic, but without the `!showOnlyArtifacts` checks
}
}
fix_sessionview.py
Outdated
| import re | ||
|
|
||
| file_path = "composeApp/src/commonMain/kotlin/dev/therealashik/client/jules/ui/screens/SessionView.kt" | ||
|
|
||
| with open(file_path, "r") as f: | ||
| content = f.read() | ||
|
|
||
| # Fix the double call issue in items | ||
| content = content.replace("ActivityItem(activity, defaultCardState, onApprovePlan)(isCompactMode = selectedTabIndex == 0, showOnlyArtifacts = selectedTabIndex == 1)", | ||
| "ActivityItem(activity, defaultCardState, onApprovePlan, isCompactMode = selectedTabIndex == 0, showOnlyArtifacts = selectedTabIndex == 1)") | ||
|
|
||
|
|
||
| with open(file_path, "w") as f: | ||
| f.write(content) |
There was a problem hiding this comment.
This file, along with patch_activity_item.py, patch_artifact_view.py, patch_filter.py, patch_sessionview.py, and update_sessionview.py, appears to be a temporary script used for code generation. Such one-off scripts should not be committed to the repository's source tree. Please remove them to keep the codebase clean.
1 participant
CompactArtifactViewto render non-expandable, simple summaries of bash and diff artifacts in Chat mode.isCompactModeandshowOnlyArtifactsflags toActivityItemto conditionally render system events, conversational text, and full CodeBlocks depending on the selected tab index.PR created automatically by Jules for task 17758077791937901816 started by @TheRealAshik