chore: get changes from longbridge gpui components. - #2
Merged
Conversation
selection/docs - Add syntax highlighting for Astro, Kotlin, Lua, and Svelte (fixtures and highlight queries) - Update Table to support cell selection and document selection modes - Sync upstream fixes: editor perf, table selection, bugfixes (see README) - Update theme docs for ThemeSet and ThemeModePreference - Misc: update dependencies, menu separator rendering, input keybindings, and minor code cleanup
- Replace "Dark Mode" and "Auto Switch Theme" with dropdowns for color theme and mode - Update theme switching logic to use ThemeRegistry and ThemeModePreference - Show theme set name in title bar - Rename Ayu theme to "Ayu" for consistency
error logging - Use `_` instead of `i` in Lua fixture loop for idiomatic style - Adjust menu
- Make resolve_theme return Option instead of panicking on empty ThemeSetEntry; apply_theme_set now warns and returns early - Extract create_default_set helper to remove duplicated ThemeSetEntry construction in init_default_themes and reload - Clear stale selected_row/col/cell in table selection setters so internal state is consistent when switching selection mode - Log parser.set_language/set_included_ranges errors at debug level instead of silently continuing in highlighter - Fix leading separator in menu story (i > 0 && i % 5 == 0) - Change Lua nil capture from @boolean to @constant.builtin - Replace unused loop variable i with _ in test.lua fixture Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Matches the Lua nil fix — null is not a boolean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughAdds theme sets with mode preference, cell-level table selection/events, per-language highlighter injection layers plus Kotlin/Lua support, a new resizable FloatingSidebar component and story, example fixtures, docs updates, UI refinements, and appended task events. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR syncs a batch of upstream gpui-component changes into this fork by expanding syntax highlighting support/fixtures, adding theme-set + system appearance preference selection, and enhancing table selection capabilities (including cell selection), along with related docs and housekeeping updates.
Changes:
- Introduces theme sets +
ThemeModePreference(“System/Light/Dark”) across the UI theme registry and story app menus/settings. - Adds Kotlin/Lua syntax highlighting queries and example fixtures (plus new fixtures for Astro/Svelte).
- Extends table interactions with cell selection events/handling and expands component documentation.
Reviewed changes
Copilot reviewed 39 out of 40 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| themes/ayu.json | Renames the theme-set display name to “Ayu” for grouping. |
| docs/docs/theme.md | Documents theme sets and ThemeModePreference usage. |
| docs/docs/components/table.md | Expands Table docs (selection modes, events, keyboard shortcuts, API reference). |
| crates/ui/src/tree.rs | Adds a focus() helper on TreeState. |
| crates/ui/src/theme/registry.rs | Adds ThemeSetEntry storage + theme-set rebuild logic on reload. |
| crates/ui/src/theme/mod.rs | Adds ThemeModePreference + active theme-set name to global Theme, plus apply-by-set APIs. |
| crates/ui/src/table/state.rs | Adds cell selection mode, cell events, selection tracking, rendering + navigation support. |
| crates/ui/src/tab/tab.rs | Import formatting cleanup. |
| crates/ui/src/sidebar/mod.rs | Refactors header/footer builder closures formatting. |
| crates/ui/src/sidebar/menu.rs | Minor formatting + avoids unnecessary clone in collapsed submenu builder. |
| crates/ui/src/sheet.rs | Refactors slide animation closure formatting. |
| crates/ui/src/radio.rs | Removes overflow_hidden() in radio indicator content. |
| crates/ui/src/popover.rs | Minor formatting for motion duration calculation. |
| crates/ui/src/plot/shape/line.rs | Import formatting cleanup. |
| crates/ui/src/notification.rs | Import formatting cleanup. |
| crates/ui/src/menu/popup_menu.rs | Changes separator rendering from 1px background to bottom border styling. |
| crates/ui/src/input/state.rs | Adds additional keybindings (shift variants, ctrl-backspace on macOS) + formatting tweaks. |
| crates/ui/src/input/search.rs | Minor formatting simplification. |
| crates/ui/src/input/element.rs | Minor formatting simplification. |
| crates/ui/src/highlighter/languages/lua/highlights.scm | Adds Lua highlight query file. |
| crates/ui/src/highlighter/languages/kotlin/highlights.scm | Adds Kotlin highlight query file. |
| crates/ui/src/highlighter/languages.rs | Registers Kotlin/Lua in language enum/name parsing/config selection. |
| crates/ui/src/highlighter/highlighter.rs | Refactors combined injection highlighting to parse once in update() and reuse during matching. |
| crates/ui/src/dialog.rs | Minor import formatting + wraps soft-dismiss animation fallback formatting. |
| crates/ui/src/animation.rs | Adds clippy allow for keyed_presence argument count. |
| crates/ui/src/accordion.rs | Removes an unnecessary clone of layout_anim. |
| crates/ui/Cargo.toml | Adds optional tree-sitter deps (Astro-next, Kotlin, Lua, Svelte-next) to the highlighting feature set. |
| crates/story/src/title_bar.rs | Displays theme set name in title bar instead of variant name. |
| crates/story/src/themes.rs | Persists theme set + mode preference in story state and applies via theme set API. |
| crates/story/src/stories/table_story.rs | Logs new table cell-selection events. |
| crates/story/src/stories/settings_story.rs | Replaces dark-mode toggle with theme-set + mode preference dropdowns. |
| crates/story/src/stories/menu_story.rs | Inserts separators periodically in an example popup menu. |
| crates/story/src/app_menus.rs | Updates menu theme selection to use theme sets + system/light/dark preference. |
| crates/story/examples/fixtures/test.svelte | Adds a Svelte fixture file for highlighting demos. |
| crates/story/examples/fixtures/test.lua | Adds a Lua fixture file for highlighting demos. |
| crates/story/examples/fixtures/test.kt | Adds a Kotlin fixture file for highlighting demos. |
| crates/story/examples/fixtures/test.astro | Adds an Astro fixture file for highlighting demos. |
| README.md | Adds upstream sync status section with referenced commits. |
| Cargo.lock | Updates lockfile for new/updated dependencies. |
| .tasque/events.jsonl | Logs planning/claiming/completion events for this upstream port batch. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+275
to
+280
| cx.stop_propagation(); | ||
| self.selection_mode = SelectionMode::Row; | ||
| self.right_clicked_row = None; | ||
| self.selected_row = Some(row_ix); | ||
| self.selected_col = None; | ||
| self.selected_cell = None; |
There was a problem hiding this comment.
set_selected_row clears right_clicked_row but leaves right_clicked_cell untouched. After a cell right-click, switching to row selection can leave a stale right-click border rendered on an unrelated cell. Clear right_clicked_cell (and/or unify selection state resets) when changing selection modes.
Comment on lines
307
to
312
| pub fn set_selected_col(&mut self, col_ix: usize, cx: &mut Context<Self>) { | ||
| self.selection_state = SelectionState::Column; | ||
| self.selection_mode = SelectionMode::Column; | ||
| self.selected_col = Some(col_ix); | ||
| self.selected_row = None; | ||
| self.selected_cell = None; | ||
| if let Some(col_ix) = self.selected_col { |
There was a problem hiding this comment.
set_selected_col switches to column selection but does not clear right_clicked_cell/right_clicked_row. This can leave stale right-click styling even though the active selection mode changed. Consider clearing the right-click state when a new selection mode is set.
Comment on lines
21
to
53
| @@ -35,6 +36,8 @@ tree-sitter-languages = [ | |||
| "dep:tree-sitter-java", | |||
| "dep:tree-sitter-javascript", | |||
| "dep:tree-sitter-jsdoc", | |||
| "dep:tree-sitter-kotlin-sg", | |||
| "dep:tree-sitter-lua", | |||
| "dep:tree-sitter-make", | |||
| "dep:tree-sitter-md", | |||
| "dep:tree-sitter-php", | |||
| @@ -46,6 +49,7 @@ tree-sitter-languages = [ | |||
| "dep:tree-sitter-sequel", | |||
| "dep:tree-sitter-swift", | |||
| "dep:tree-sitter-toml-ng", | |||
| "dep:tree-sitter-svelte-next", | |||
| "dep:tree-sitter-typescript", | |||
There was a problem hiding this comment.
The tree-sitter-languages feature now enables tree-sitter-astro-next and tree-sitter-svelte-next, but there are no corresponding Language variants / parser wiring / highlight queries for Astro or Svelte (and these crates appear unused in the code). Either add full highlighter integration for Astro/Svelte or remove these deps from the feature set to avoid unnecessary build time and binary size.
Comment on lines
11
to
16
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| struct State { | ||
| theme: SharedString, | ||
| theme_set: SharedString, | ||
| mode_preference: ThemeModePreference, | ||
| scrollbar_show: Option<ScrollbarShow>, | ||
| } |
There was a problem hiding this comment.
The persisted State format changed from theme to theme_set + mode_preference, but Deserialize will now fail on existing target/state.json files (missing fields) and silently reset users to defaults via unwrap_or_default(). If you want to preserve prior selections, add #[serde(default)] plus #[serde(alias = "theme")]/migration logic so older state files can still load.
| /// Parse all combined injections after main tree is updated. | ||
| /// pattern: parse once in update, query many times in render. | ||
| fn parse_combined_injections(&mut self, tree: &Tree) { | ||
| let Some(combined_query) = &self.combined_injections_query else { |
There was a problem hiding this comment.
parse_combined_injections returns early when there is no combined_injections_query, but it never clears injection_layers. If the highlighter previously parsed combined injections (or the language changes), stale injection trees can remain and match_styles will keep applying injection highlights that no longer exist. Consider clearing injection_layers when combined_injections_query is None, and otherwise retaining/removing layers based on the languages found in the current combined_ranges before inserting updated trees.
Suggested change
| let Some(combined_query) = &self.combined_injections_query else { | |
| let Some(combined_query) = &self.combined_injections_query else { | |
| // If combined injections are disabled or unavailable, clear any stale injection layers. | |
| self.injection_layers.clear(); |
Comment on lines
153
to
156
| pub mode: ThemeMode, | ||
| pub mode_preference: ThemeModePreference, | ||
| pub theme_set_name: SharedString, | ||
| /// The font family for the application, default is `.SystemUIFont`. |
There was a problem hiding this comment.
Theme derives Serialize/Deserialize, and this PR adds new non-optional fields (mode_preference, theme_set_name) without #[serde(default)]. Any persisted Theme JSON produced by older versions will fail to deserialize due to missing fields. Add serde defaults for the new fields (or mark the whole struct with #[serde(default)] and ensure sensible defaults) to avoid a breaking change for consumers.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/ui/src/table/state.rs (1)
269-317:⚠️ Potential issue | 🟡 MinorClear stale right-click cell state when selection changes.
right_clicked_cellpersists across left-click selection andclear_selection, so the old right-click highlight can linger after selecting another cell or pressing Esc. Clear it (and the row right-click state) whenever selection changes.🧹 Suggested fix
pub fn set_selected_row(&mut self, row_ix: usize, cx: &mut Context<Self>) { @@ - self.right_clicked_row = None; + self.right_clicked_row = None; + self.right_clicked_cell = None; @@ } pub fn set_selected_col(&mut self, col_ix: usize, cx: &mut Context<Self>) { self.selection_mode = SelectionMode::Column; self.selected_col = Some(col_ix); self.selected_row = None; self.selected_cell = None; + self.right_clicked_row = None; + self.right_clicked_cell = None; @@ } pub fn set_selected_cell(&mut self, row_ix: usize, col_ix: usize, cx: &mut Context<Self>) { self.selection_mode = SelectionMode::Cell; self.selected_cell = Some((row_ix, col_ix)); self.selected_row = None; self.selected_col = None; + self.right_clicked_row = None; + self.right_clicked_cell = None; @@ } pub fn clear_selection(&mut self, cx: &mut Context<Self>) { self.selection_mode = SelectionMode::Row; self.selected_row = None; self.selected_col = None; self.selected_cell = None; + self.right_clicked_row = None; + self.right_clicked_cell = None; cx.emit(TableEvent::ClearSelection); cx.notify(); }Also applies to: 324-347
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ui/src/table/state.rs` around lines 269 - 317, Clear any stale right-click state when changing selection: in set_selected_row and set_selected_col (and in clear_selection), reset both self.right_clicked_cell = None and self.right_clicked_row = None whenever you change selection mode/selected items so the old right-click highlight is removed; update the methods set_selected_row, set_selected_col, and clear_selection to explicitly clear these fields before emitting events/notifications.crates/ui/src/highlighter/languages.rs (1)
32-133: 🧹 Nitpick | 🔵 TrivialAdd test coverage for the new Language variants.
Extending
test_language_nameto include the newly added variants guardsname()regressions and keeps mappings exercised.🔧 Suggested test additions
@@ assert_eq!(Language::JavaScript.name(), "javascript"); assert_eq!(Language::Zig.name(), "zig"); assert_eq!(Language::CSharp.name(), "csharp"); assert_eq!(Language::TypeScript.name(), "typescript"); assert_eq!(Language::Tsx.name(), "tsx"); assert_eq!(Language::Diff.name(), "diff"); assert_eq!(Language::Elixir.name(), "elixir"); assert_eq!(Language::Erb.name(), "erb"); assert_eq!(Language::Ejs.name(), "ejs"); + assert_eq!(Language::Kotlin.name(), "kotlin"); + assert_eq!(Language::Lua.name(), "lua");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ui/src/highlighter/languages.rs` around lines 32 - 133, The test suite is missing coverage for the newly added Language variants; update the existing test (e.g., test_language_name) to assert that Language::Kotlin, ::Lua, ::Make, ::Markdown, ::MarkdownInline, ::Php, ::Proto, ::Python, ::Ruby, ::Rust, ::Scala, ::Sql, ::Swift, ::Toml, ::Tsx, ::TypeScript, ::Yaml, and ::Zig map to their expected name() outputs; add each variant to the test assertions so Language::name() is exercised for all enum variants and future regressions are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/story/examples/fixtures/test.astro`:
- Around line 83-88: The template uses a React-style key prop inside items.map
(the <li key={index}> in the map callback) which is unnecessary in Astro; remove
the key attribute from the <li> elements (or if this JSX-like usage is
intentional for demonstration, add an inline comment near the items.map block
indicating it’s illustrative) so the template emits clean Astro/HTML output;
locate the map invocation (items.map((item, index) => ( ... ))) and either
delete key={index} from the <li> or add a clarifying comment above it.
- Line 6: Remove the unused import GetStaticPaths from the top of the file (the
import statement referencing GetStaticPaths) unless you intend it for
demonstration; if it is meant as an example, replace it with a brief inline
comment clarifying that the import is present only for syntax highlighting/demo,
otherwise delete the import line to eliminate the unused symbol.
In `@crates/story/examples/fixtures/test.kt`:
- Around line 63-67: The function safely currently catches a generic Exception
which triggers the TooGenericExceptionCaught detekt rule; either add
`@Suppress`("TooGenericExceptionCaught") above the safely declaration to silence
the rule, or refactor safely to use Kotlin's runCatching in the safely(block: ()
-> T): Result<T> function (call runCatching { block() } and map success to
Result.Success and failure to Result.Error using the exception.message ?:
"unknown error") so you avoid an explicit catch of Exception.
In `@crates/story/examples/fixtures/test.lua`:
- Line 59: The function declaration HelloWorld:processNames currently defines an
implicit self parameter but doesn't use it, triggering Luacheck W212; either (a)
change the declaration to a dot-style function HelloWorld.processNames(names) if
no instance is needed, or (b) keep the colon and reference self inside the body
or add a Luacheck suppression comment (-- luacheck: ignore 212) directly above
the function to silence the warning; locate the HelloWorld:processNames
definition and apply one of these fixes to remove the unused implicit self
warning.
- Around line 8-112: The Config, LogLevel and describe symbols are unused and
causing Luacheck W211; update HelloWorld.new and/or HelloWorld:generateReport to
reference Config and LogLevel (for example include Config values in self.config
or append LogLevel.DEFAULT to the report) and add a small main/test block that
constructs HelloWorld, calls :generateReport() and calls describe(...) on a
Success/Error and a string to exercise the describe function (use Success/Error
or safely to produce values). Ensure the modifications reference the exact
symbols Config, LogLevel, and describe so they are no longer unused.
In `@crates/story/examples/fixtures/test.svelte`:
- Line 100: The each block using "{`#each` state.items as item, i (item)}" risks
key collisions when duplicate strings exist; change the key to a stable unique
value such as the index or a composite key to guarantee uniqueness (e.g., use
"i" or combine "item" and "i") in the each block that iterates state.items
(references: state.items, item, i, the each block) so Svelte can reliably track
list items.
In `@crates/ui/src/highlighter/highlighter.rs`:
- Around line 41-50: The injection_layers cache is only appended to and can
retain trees for languages whose combined_ranges have been removed; update the
logic in update() to prune injection_layers before parsing new trees: iterate
over the keys in injection_layers and remove any language (key) that is not
present in the current combined_ranges or whose combined_ranges entry is empty,
so InjectionLayer entries are dropped when there are no combined injection
ranges left; keep match_styles() unchanged but rely on the pruned
injection_layers to avoid emitting highlights for non-existent ranges.
In `@crates/ui/src/tree.rs`:
- Around line 396-398: The method signature for focus currently takes &mut self
though it doesn't mutate TreeState; change pub fn focus(&mut self, window: &mut
Window, cx: &mut App) to pub fn focus(&self, window: &mut Window, cx: &mut App)
so callers can use shared refs. Update the impl where focus is defined (the
method named focus that calls self.focus_handle.focus(...)) and ensure
focus_handle.focus accepts &self (no other internal mutability changes
required); then run/adjust any call sites that passed &mut to use & if needed.
In `@docs/docs/components/table.md`:
- Around line 658-669: The events list is missing the ClearSelection event
referenced elsewhere; update the Events section to include the ClearSelection
variant (no payload) alongside the other enum variants (e.g., add
`ClearSelection` to the list) so the API summary and examples match; ensure any
example snippets and references to ClearSelection now point to this documented
event name (ClearSelection) and keep the same formatting as the other entries
like `SelectRow(usize)` and `MoveColumn(usize, usize)`.
---
Outside diff comments:
In `@crates/ui/src/highlighter/languages.rs`:
- Around line 32-133: The test suite is missing coverage for the newly added
Language variants; update the existing test (e.g., test_language_name) to assert
that Language::Kotlin, ::Lua, ::Make, ::Markdown, ::MarkdownInline, ::Php,
::Proto, ::Python, ::Ruby, ::Rust, ::Scala, ::Sql, ::Swift, ::Toml, ::Tsx,
::TypeScript, ::Yaml, and ::Zig map to their expected name() outputs; add each
variant to the test assertions so Language::name() is exercised for all enum
variants and future regressions are caught.
In `@crates/ui/src/table/state.rs`:
- Around line 269-317: Clear any stale right-click state when changing
selection: in set_selected_row and set_selected_col (and in clear_selection),
reset both self.right_clicked_cell = None and self.right_clicked_row = None
whenever you change selection mode/selected items so the old right-click
highlight is removed; update the methods set_selected_row, set_selected_col, and
clear_selection to explicitly clear these fields before emitting
events/notifications.
| // This file demonstrates Astro 5 syntax highlighting features. | ||
| // Frontmatter: TypeScript/JavaScript code that runs at build time | ||
|
|
||
| import type { GetStaticPaths } from 'astro'; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unused import.
GetStaticPaths is imported but never used in this file. If it's intended to demonstrate import syntax for highlighting purposes, consider adding a comment clarifying this. Otherwise, it can be removed.
Remove unused import
-import type { GetStaticPaths } from 'astro';📝 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
| import type { GetStaticPaths } from 'astro'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/story/examples/fixtures/test.astro` at line 6, Remove the unused
import GetStaticPaths from the top of the file (the import statement referencing
GetStaticPaths) unless you intend it for demonstration; if it is meant as an
example, replace it with a brief inline comment clarifying that the import is
present only for syntax highlighting/demo, otherwise delete the import line to
eliminate the unused symbol.
Comment on lines
+83
to
+88
| items.map((item, index) => ( | ||
| <li key={index}> | ||
| <a href={`/${item.toLowerCase()}`}>{item}</a> | ||
| </li> | ||
| )) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
key attribute is not used in Astro templates.
Unlike React, Astro compiles templates at build time to static HTML, so the key prop has no effect here. If this is intentional to demonstrate JSX-like syntax for highlighting, consider adding a comment. Otherwise, it can be removed.
Remove unnecessary key attribute
items.map((item, index) => (
- <li key={index}>
+ <li>
<a href={`/${item.toLowerCase()}`}>{item}</a>
</li>
))📝 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
| items.map((item, index) => ( | |
| <li key={index}> | |
| <a href={`/${item.toLowerCase()}`}>{item}</a> | |
| </li> | |
| )) | |
| } | |
| items.map((item, index) => ( | |
| <li> | |
| <a href={`/${item.toLowerCase()}`}>{item}</a> | |
| </li> | |
| )) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/story/examples/fixtures/test.astro` around lines 83 - 88, The template
uses a React-style key prop inside items.map (the <li key={index}> in the map
callback) which is unnecessary in Astro; remove the key attribute from the <li>
elements (or if this JSX-like usage is intentional for demonstration, add an
inline comment near the items.map block indicating it’s illustrative) so the
template emits clean Astro/HTML output; locate the map invocation
(items.map((item, index) => ( ... ))) and either delete key={index} from the
<li> or add a clarifying comment above it.
Comment on lines
+63
to
+67
| fun <T> safely(block: () -> T): Result<T> = try { | ||
| Result.Success(block()) | ||
| } catch (e: Exception) { | ||
| Result.Error(e.message ?: "unknown error") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the test.kt file
find . -name "test.kt" -type f | head -20Repository: BumpyClock/gpui-component
Length of output: 109
🏁 Script executed:
# Look for detekt configuration files
find . -name "detekt*" -type f | head -20Repository: BumpyClock/gpui-component
Length of output: 89
🏁 Script executed:
# Check for gradle/maven files that might reference detekt
find . \( -name "build.gradle*" -o -name "pom.xml" -o -name "*.gradle" \) -type f | head -20Repository: BumpyClock/gpui-component
Length of output: 51
🏁 Script executed:
cat -n ./crates/story/examples/fixtures/test.kt | sed -n '55,75p'Repository: BumpyClock/gpui-component
Length of output: 842
🏁 Script executed:
cat ./detekt-config.yamlRepository: BumpyClock/gpui-component
Length of output: 10224
🏁 Script executed:
# Check if there's any `@Suppress` annotation in the test.kt file
grep -n "@Suppress" ./crates/story/examples/fixtures/test.ktRepository: BumpyClock/gpui-component
Length of output: 51
🏁 Script executed:
# Check the complete context around the safely function to see if there's any suppression
cat -n ./crates/story/examples/fixtures/test.kt | sed -n '60,70p'Repository: BumpyClock/gpui-component
Length of output: 430
🏁 Script executed:
# Verify if examples/fixtures matches any detekt exclusion patterns
# by checking the exact path structure
ls -la ./crates/story/examples/fixtures/Repository: BumpyClock/gpui-component
Length of output: 1313
Add an explicit suppression or refactor to avoid catching generic Exception.
The TooGenericExceptionCaught detekt rule is active and applies to this file (it's not excluded from linting). Line 65 will be flagged. Either add @Suppress("TooGenericExceptionCaught") or refactor using runCatching:
Refactor using runCatching
-fun <T> safely(block: () -> T): Result<T> = try {
- Result.Success(block())
-} catch (e: Exception) {
- Result.Error(e.message ?: "unknown error")
-}
+fun <T> safely(block: () -> T): Result<T> =
+ kotlin.runCatching(block).fold(
+ onSuccess = { Result.Success(it) },
+ onFailure = { Result.Error(it.message ?: "unknown error") }
+ )📝 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
| fun <T> safely(block: () -> T): Result<T> = try { | |
| Result.Success(block()) | |
| } catch (e: Exception) { | |
| Result.Error(e.message ?: "unknown error") | |
| } | |
| fun <T> safely(block: () -> T): Result<T> = | |
| kotlin.runCatching(block).fold( | |
| onSuccess = { Result.Success(it) }, | |
| onFailure = { Result.Error(it.message ?: "unknown error") } | |
| ) |
🧰 Tools
🪛 detekt (1.23.8)
[warning] 65-65: The caught exception is too generic. Prefer catching specific exceptions to the case that is currently handled.
(detekt.exceptions.TooGenericExceptionCaught)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/story/examples/fixtures/test.kt` around lines 63 - 67, The function
safely currently catches a generic Exception which triggers the
TooGenericExceptionCaught detekt rule; either add
`@Suppress`("TooGenericExceptionCaught") above the safely declaration to silence
the rule, or refactor safely to use Kotlin's runCatching in the safely(block: ()
-> T): Result<T> function (call runCatching { block() } and map success to
Result.Success and failure to Result.Error using the exception.message ?:
"unknown error") so you avoid an explicit catch of Exception.
Comment on lines
+8
to
+112
| -- Module configuration with default values | ||
| local Config = { | ||
| timeout = 5000, | ||
| retries = 3, | ||
| debug = false | ||
| } | ||
|
|
||
| -- Enum-like table for log levels | ||
| local LogLevel = { | ||
| INFO = "INFO", | ||
| WARN = "WARN", | ||
| ERROR = "ERROR" | ||
| } | ||
|
|
||
| -- Create a class using metatables | ||
| local HelloWorld = {} | ||
| HelloWorld.__index = HelloWorld | ||
|
|
||
| function HelloWorld.new(name) | ||
| local self = setmetatable({}, HelloWorld) | ||
| self.name = name or "World" | ||
| self.config = {timeout = 5000, retries = 3, debug = false} | ||
| self.createdAt = os.time() | ||
| self._greetCount = 0 -- private-like field | ||
| return self | ||
| end | ||
|
|
||
| -- Method using colon syntax (implicit self) | ||
| function HelloWorld:greet(...) | ||
| local names = {...} | ||
| local results = {} | ||
|
|
||
| for _, name in ipairs(names) do | ||
| local greeting = string.format("Hello, %s!", name) | ||
| table.insert(results, greeting) | ||
| self._greetCount = self._greetCount + 1 | ||
|
|
||
| if self.config.debug then | ||
| print(string.format(" [debug] %s", greeting)) | ||
| end | ||
| end | ||
|
|
||
| return results | ||
| end | ||
|
|
||
| function HelloWorld:configure(newConfig) | ||
| for k, v in pairs(newConfig) do | ||
| self.config[k] = v | ||
| end | ||
| end | ||
|
|
||
| function HelloWorld:processNames(names) | ||
| local processed = {} | ||
|
|
||
| for _, name in ipairs(names or {}) do | ||
| if name ~= "" and name:match("%S") then | ||
| table.insert(processed, name:upper()) | ||
| end | ||
| end | ||
|
|
||
| table.sort(processed) | ||
| return processed | ||
| end | ||
|
|
||
| function HelloWorld:generateReport() | ||
| local elapsed = os.time() - self.createdAt | ||
| local lines = { | ||
| "HelloWorld Report", | ||
| "=================", | ||
| string.format("Name: %s", self.name), | ||
| string.format("Elapsed: %ds", elapsed), | ||
| string.format("Greetings: %d", self._greetCount), | ||
| string.format("Config: timeout=%d, retries=%d", | ||
| self.config.timeout, self.config.retries) | ||
| } | ||
| return table.concat(lines, "\n") | ||
| end | ||
|
|
||
| -- Metatable for Result type (Success/Error) | ||
| local function Success(data) | ||
| return {success = true, data = data} | ||
| end | ||
|
|
||
| local function Error(message) | ||
| return {success = false, message = message} | ||
| end | ||
|
|
||
| -- Protected call wrapper | ||
| local function safely(fn) | ||
| local status, result = pcall(fn) | ||
| return status and Success(result) or Error(tostring(result)) | ||
| end | ||
|
|
||
| -- Pattern matching-like function | ||
| local function describe(obj) | ||
| if obj == nil then return "nil" | ||
| elseif type(obj) == "string" then | ||
| return string.format('String(%d): "%s"', #obj, obj:sub(1, 20)) | ||
| elseif type(obj) == "table" and obj.success ~= nil then | ||
| return obj.success and ("ok: " .. tostring(obj.data)) | ||
| or ("err: " .. obj.message) | ||
| else | ||
| return type(obj) | ||
| end | ||
| end |
There was a problem hiding this comment.
Luacheck W211: unused Config, LogLevel, and describe.
If luacheck runs on fixtures, these will fail linting. Consider using them in the constructor/report and in the main block (mirroring the Kotlin fixture).
🧩 Example usage to satisfy luacheck
function HelloWorld.new(name)
local self = setmetatable({}, HelloWorld)
self.name = name or "World"
- self.config = {timeout = 5000, retries = 3, debug = false}
+ self.config = {
+ timeout = Config.timeout,
+ retries = Config.retries,
+ debug = Config.debug,
+ level = LogLevel.INFO
+ }
@@
- string.format("Config: timeout=%d, retries=%d",
- self.config.timeout, self.config.retries)
+ string.format("Config: timeout=%d, retries=%d, level=%s",
+ self.config.timeout, self.config.retries, self.config.level)
}
return table.concat(lines, "\n")
end
@@
print("Processed: " .. table.concat(processed, ", "))
@@
if result.success then
print(result.data)
else
print("Failed: " .. result.message)
end
+
+-- Describe samples (mirrors Kotlin fixture intent)
+print(" " .. describe("hello"))
+print(" " .. describe(42))
+print(" " .. describe(nil))
+print(" " .. describe(result))🧰 Tools
🪛 Luacheck (1.2.0)
[warning] 9-9: unused variable 'Config'
(W211)
[warning] 16-16: unused variable 'LogLevel'
(W211)
[warning] 59-59: unused argument 'self'
(W212)
[warning] 102-102: unused function 'describe'
(W211)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/story/examples/fixtures/test.lua` around lines 8 - 112, The Config,
LogLevel and describe symbols are unused and causing Luacheck W211; update
HelloWorld.new and/or HelloWorld:generateReport to reference Config and LogLevel
(for example include Config values in self.config or append LogLevel.DEFAULT to
the report) and add a small main/test block that constructs HelloWorld, calls
:generateReport() and calls describe(...) on a Success/Error and a string to
exercise the describe function (use Success/Error or safely to produce values).
Ensure the modifications reference the exact symbols Config, LogLevel, and
describe so they are no longer unused.
Fix TreeState::focus to take &self, not &mut self Document ClearSelection event in Table delegate
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/story/src/stories/floating_sidebar_story.rs`:
- Around line 43-49: Replace the hardcoded collapsed width px(48.0) with the
existing COLLAPSED_WIDTH constant from the sidebar module: update the local
collapsed_width (and any usages like content_offset) to use COLLAPSED_WIDTH so
the code reads collapsed_width = COLLAPSED_WIDTH (or remove the local var and
substitute COLLAPSED_WIDTH directly), and ensure render_example continues to
reference the same constant for consistency.
In `@crates/ui/src/floating_sidebar/mod.rs`:
- Around line 41-59: FloatingSidebar lacks the Sizable trait required for UI
components; implement impl<E: SidebarItem> Sizable for FloatingSidebar<E> and
add a with_size(self, size: impl Into<Size>) -> Self that forwards the requested
size to the inner Sidebar (or stores it on FloatingSidebar and applies to
Sidebar before layout), ensuring you handle the generic bound E: SidebarItem and
preserve other fields (collapsed, width, min_width, etc.); update any calls that
construct Sidebar inside FloatingSidebar to consume or apply the stored size so
xs/sm/md/lg sizing is respected.
In `@crates/ui/src/highlighter/highlighter.rs`:
- Around line 393-396: Avoid cloning the tree by storing new_tree directly into
self.tree and passing a reference to that stored value to
parse_combined_injections instead of calling new_tree.clone(); specifically,
replace self.tree = Some(new_tree.clone()) with assigning the owned new_tree
into self.tree (self.tree = Some(new_tree)) and then call
parse_combined_injections with a reference obtained from self.tree (e.g.,
self.tree.as_ref().unwrap() or matching on self.tree) so
parse_combined_injections(&...) receives a shared reference to the stored tree.
- Around line 467-486: The injection trees are being reused without applying the
document edit, breaking incremental parsing; in parse_combined_injections (where
you access injection_layers and layer.tree for language_name) stop passing the
stale old_tree into parser.parse_with_options — instead pass None for the old
tree so injections are parsed from scratch (simpler short-term fix), or if you
prefer to preserve incremental parsing, propagate the InputEdit from update()
into parse_combined_injections and call edit() on each old injection tree before
passing it to parser.parse_with_options.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/story/src/main.rs`:
- Around line 164-171: Replace the hardcoded px(48.0) used to compute
collapsed_width in the render logic with the existing COLLAPSED_WIDTH constant
from the sidebar module to ensure consistency; update the expression that sets
collapsed_width (used when self.collapsed is true) to reference
sidebar::COLLAPSED_WIDTH (or the appropriate module path) so that sidebar_width
and content_offset derive from that constant instead of a literal.
In `@crates/ui/src/setting/settings.rs`:
- Around line 349-352: The SidebarShell currently calls
SidebarShell::left(...).resizer_width(px(0.0)) which disables the resize handle;
either remove the .resizer_width(px(0.0)) call to restore a default/visible
resizer, or (if the sidebar is intentionally non-resizable) add a one-line
inline comment next to the SidebarShell::left(...) chain explaining that the
resizer is deliberately disabled for the settings sidebar (reference:
SidebarShell::left and resizer_width(px(0.0)) in settings.rs).
- Around line 73-106: Add a unit test named test_settings_builder that
instantiates the Settings builder, chains the methods use_sidebar_shell(true),
safe_area_top(<Pixels value>), sidebar_shell_inset(<Pixels value>),
sidebar_title("title"), and page_header_action(|_, _| /* closure */), then
assert the backing fields were set: assert_eq!(settings.use_sidebar_shell,
true), assert_eq!(settings.safe_area_top, <expected Pixels>),
assert_eq!(settings.sidebar_shell_inset, <expected Pixels>),
assert_eq!(settings.sidebar_title.as_deref(), Some("title")), and
assert!(settings.page_header_action.is_some()); place this test in a
#[cfg(test)] mod tests in the same file and import any required types (Pixels,
SharedString, Rc) so the test verifies chaining and field assignment for the
methods use_sidebar_shell, safe_area_top, sidebar_shell_inset, sidebar_title,
and page_header_action.
In `@docs/docs/components/settings.md`:
- Line 95: The documentation example uses a raw integer for the inset which may
not compile because sidebar_shell_inset expects something implementing
Into<Pixels>; update the example to pass a Pixels value by calling px(8.0) (or
equivalent constructor) instead of raw 8 so sidebar_shell_inset(...) receives
the correct type; reference the sidebar_shell_inset helper and the px
constructor/Into<Pixels> conversion in the docs.
In `@docs/docs/components/sidebar.md`:
- Line 104: The documentation example uses inset(8) but inset expects an
Into<Pixels>, so update the example to call inset with a Pixels value using the
px helper (e.g., inset(px(8.0)) or inset(px(8))) and mention px/import if
needed; reference the inset method and the Pixels type to locate the example and
ensure it matches the function signature pub fn inset(mut self, inset: impl
Into<Pixels>) -> Self.
---
Duplicate comments:
In `@crates/story/src/stories/floating_sidebar_story.rs`:
- Around line 43-49: The local variable collapsed_width is hardcoded to
px(48.0); replace its usage with the existing COLLAPSED_WIDTH constant from the
sidebar module to ensure consistency. Update the code in
floating_sidebar_story.rs by removing or replacing the collapsed_width variable
and use COLLAPSED_WIDTH in the content_offset calculation (keeping sidebar_width
and inset as-is), and add the appropriate use/import if needed so
COLLAPSED_WIDTH is in scope.
In `@crates/ui/src/floating_sidebar/mod.rs`:
- Around line 41-59: FloatingSidebar lacks the Sizable trait implementation
required by project UI conventions; implement impl<E: SidebarItem + 'static>
Sizable for FloatingSidebar<E> and have its required methods (e.g., current size
getter, set_size/resize, and min/max size accessors as defined by the Sizable
trait in the codebase) read from and update the struct's fields (width,
min_width, max_width, inset, top_inset and resizer_width where applicable), and
ensure any resize callbacks (on_resize_end) are invoked when size changes so the
component behaves consistently with other sizable UI components.
Add logic to ensure width is always within [min_width, max_width] range. Update min_width and max_width setters to maintain valid constraints. Add tests for width clamping and constraint behavior.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/ui/src/floating_sidebar/mod.rs`:
- Around line 224-229: FloatingSidebar lacks Sizable support so callers can't
choose xs/sm/md/lg; add an impl and propagate the chosen size. Add a Size field
to FloatingSidebar (default Size::Md), implement impl<E: SidebarItem> Sizable
for FloatingSidebar<E> with getter and setter (e.g., size(&self) -> Size and
set_size(&mut self, s: Size) -> &mut Self), and ensure this stored size is
applied wherever the component composes children or builds styles: forward the
size to inner components (call their .size(self.size) / set_size) and apply to
the StyleRefinement/state used in render (use the size when computing
classes/styles). Update any builders/constructors to set default Size::Md if
missing.
- Use index as key in Svelte {#each} block to avoid duplicate key issues
- Refine syntax highlighter debug logs for better error context
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)
crates/ui/src/highlighter/highlighter.rs (1)
517-579: 🧹 Nitpick | 🔵 TrivialMake injection layer iteration deterministic.
HashMap iteration order is nondeterministic; this can change highlight precedence across runs/edits. Consider iterating in a stable key order.
♻️ Deterministic iteration order
- for (language_name, layer) in &self.injection_layers { + let mut languages: Vec<_> = self.injection_layers.keys().collect(); + languages.sort_by(|a, b| a.as_ref().cmp(b.as_ref())); + for language_name in languages { + let Some(layer) = self.injection_layers.get(language_name) else { + continue; + }; let Some(query) = self.injection_queries.get(language_name) else { continue; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ui/src/highlighter/highlighter.rs` around lines 517 - 579, The loop over self.injection_layers is nondeterministic because it iterates a HashMap; make iteration deterministic by collecting the keys (language_name) into a Vec, sort them (e.g., ascending string order), and then iterate the sorted keys to lookup layer and query from self.injection_layers and self.injection_queries instead of iterating the map directly; update the block that currently says "for (language_name, layer) in &self.injection_layers" to first build and sort the keys and then use those keys to obtain layer and query so highlight precedence is stable across runs (references: self.injection_layers, self.injection_queries, the injection layer loop in highlighter.rs).
♻️ Duplicate comments (1)
crates/ui/src/highlighter/highlighter.rs (1)
476-492:⚠️ Potential issue | 🟠 MajorDon’t reuse injection trees without applying
edit()first.Tree‑sitter requires edits on old trees before incremental reuse; otherwise positions are stale and parsing can be incorrect. Easiest safe fix is to disable reuse for injections (or propagate
InputEditinto this function and calledit()on each cached tree).🛠️ Safer short‑term fix: disable incremental reuse for injections
- // Try to reuse old tree for incremental parsing - let old_tree = self - .injection_layers - .get(&language_name) - .map(|layer| &layer.tree); + let old_tree = None; // full re-parse; incremental requires edit() on old treetree-sitter incremental parsing requirement edit() old tree parse_with_options🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ui/src/highlighter/highlighter.rs` around lines 476 - 492, The code is reusing cached injection trees from self.injection_layers when calling parser.parse_with_options without applying Tree‑sitter edits, which can lead to stale positions; either stop passing old_tree for injections (disable incremental reuse) or accept and propagate an InputEdit into this routine and call layer.tree.edit(&input_edit) on each cached tree before passing them to parser.parse_with_options; update the logic around injection_layers, old_tree, and the parse_with_options call to ensure edit() is invoked on every cached layer.tree (or set old_tree to None) before incremental parsing.
🤖 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 `@crates/ui/src/highlighter/highlighter.rs`:
- Around line 517-579: The loop over self.injection_layers is nondeterministic
because it iterates a HashMap; make iteration deterministic by collecting the
keys (language_name) into a Vec, sort them (e.g., ascending string order), and
then iterate the sorted keys to lookup layer and query from
self.injection_layers and self.injection_queries instead of iterating the map
directly; update the block that currently says "for (language_name, layer) in
&self.injection_layers" to first build and sort the keys and then use those keys
to obtain layer and query so highlight precedence is stable across runs
(references: self.injection_layers, self.injection_queries, the injection layer
loop in highlighter.rs).
---
Duplicate comments:
In `@crates/ui/src/highlighter/highlighter.rs`:
- Around line 476-492: The code is reusing cached injection trees from
self.injection_layers when calling parser.parse_with_options without applying
Tree‑sitter edits, which can lead to stale positions; either stop passing
old_tree for injections (disable incremental reuse) or accept and propagate an
InputEdit into this routine and call layer.tree.edit(&input_edit) on each cached
tree before passing them to parser.parse_with_options; update the logic around
injection_layers, old_tree, and the parse_with_options call to ensure edit() is
invoked on every cached layer.tree (or set old_tree to None) before incremental
parsing.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
AGENTS.mdcrates/story/examples/fixtures/test.sveltecrates/ui/src/highlighter/highlighter.rs
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
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.
This pull request introduces several new example fixture files for syntax highlighting, updates the theme selection logic to support theme sets and system mode, and documents the latest upstream sync status in the
README.md. It also adds and closes a series of tasks in the project management event log to track the upstream porting and validation process.Theme selection and menu improvements:
crates/story/src/app_menus.rsto useThemeModePreference(with "System", "Light", and "Dark" options) instead of the previousThemeMode, and to support selecting theme sets rather than individual themes. The menu now reflects the user's theme mode preference and the current theme set. [1] [2] [3] [4]New syntax highlighting fixtures:
test.astro), Kotlin (test.kt), Lua (test.lua), and Svelte (test.svelte) incrates/story/examples/fixtures/to demonstrate and test syntax highlighting for these languages. [1] [2] [3] [4]Documentation updates:
README.mdto include a new "Upstream sync status" section, listing the exact upstream commits that were manually ported and the categories of imported fixes/features.Project management and task tracking:
.tasque/events.jsonlto document the manual porting of upstream changes, validation, and documentation updates for the sync batch.