Skip to content

Commit f9ac4da

Browse files
gadenbuiegadenbuie
committed
refactor: simplify tool open state logic
Remove special handling for error states. The tool details setting now applies uniformly regardless of whether an error occurred. This simplifies the implementation and gives users full control over the display state in all cases.
1 parent 9a1b6a0 commit f9ac4da

File tree

5 files changed

+44
-75
lines changed

5 files changed

+44
-75
lines changed

‎pkg-py/src/querychat/_utils.py‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -67,27 +67,21 @@ def get_tool_details_setting() -> Optional[str]:
6767
return os.environ.get("QUERYCHAT_TOOL_DETAILS")
6868

6969

70-
def resolve_tool_open_state(action: str, is_error: bool) -> bool:
70+
def resolve_tool_open_state(action: str) -> bool:
7171
"""
7272
Determine whether a tool card should be open based on action and setting.
7373
7474
Parameters
7575
----------
7676
action : str
7777
The action type ('update', 'query', or 'reset')
78-
is_error : bool
79-
Whether an error occurred
8078
8179
Returns
8280
-------
8381
bool
8482
True if the tool card should be open, False otherwise
8583
8684
"""
87-
# If there's an error, always show collapsed
88-
if is_error:
89-
return False
90-
9185
# Get the tool details setting
9286
setting = get_tool_details_setting()
9387

‎pkg-py/src/querychat/tools.py‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ def update_dashboard(query: str, title: str) -> ContentToolResult:
6565
markdown=markdown + f"\n\n{button_html}",
6666
title=title,
6767
show_request=False,
68-
open=resolve_tool_open_state("update", False),
68+
open=resolve_tool_open_state("update"),
6969
icon=bs_icon("funnel-fill"),
7070
),
7171
},
@@ -139,7 +139,7 @@ def reset_dashboard() -> ContentToolResult:
139139
markdown=button_html,
140140
title=None,
141141
show_request=False,
142-
open=resolve_tool_open_state("reset", False),
142+
open=resolve_tool_open_state("reset"),
143143
icon=bs_icon("arrow-counterclockwise"),
144144
),
145145
},
@@ -208,7 +208,7 @@ def query(query: str, _intent: str = "") -> ContentToolResult:
208208
"display": ToolResultDisplay(
209209
markdown=markdown,
210210
show_request=False,
211-
open=resolve_tool_open_state("query", False),
211+
open=resolve_tool_open_state("query"),
212212
icon=bs_icon("table"),
213213
),
214214
},

‎pkg-py/tests/test_tools.py‎

Lines changed: 16 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -11,52 +11,48 @@ def test_resolve_tool_open_state_default_behavior(monkeypatch):
1111
"""Test default behavior when no setting is provided."""
1212
monkeypatch.delenv("QUERYCHAT_TOOL_DETAILS", raising=False)
1313

14-
assert resolve_tool_open_state("query", False) is True
15-
assert resolve_tool_open_state("update", False) is True
16-
assert resolve_tool_open_state("reset", False) is False
17-
assert resolve_tool_open_state("query", True) is False
14+
assert resolve_tool_open_state("query") is True
15+
assert resolve_tool_open_state("update") is True
16+
assert resolve_tool_open_state("reset") is False
1817

1918

2019
def test_resolve_tool_open_state_expanded(monkeypatch):
2120
"""Test 'expanded' setting."""
2221
monkeypatch.setenv("QUERYCHAT_TOOL_DETAILS", "expanded")
2322

24-
assert resolve_tool_open_state("query", False) is True
25-
assert resolve_tool_open_state("update", False) is True
26-
assert resolve_tool_open_state("reset", False) is True
27-
assert resolve_tool_open_state("query", True) is False
23+
assert resolve_tool_open_state("query") is True
24+
assert resolve_tool_open_state("update") is True
25+
assert resolve_tool_open_state("reset") is True
2826

2927

3028
def test_resolve_tool_open_state_collapsed(monkeypatch):
3129
"""Test 'collapsed' setting."""
3230
monkeypatch.setenv("QUERYCHAT_TOOL_DETAILS", "collapsed")
3331

34-
assert resolve_tool_open_state("query", False) is False
35-
assert resolve_tool_open_state("update", False) is False
36-
assert resolve_tool_open_state("reset", False) is False
37-
assert resolve_tool_open_state("query", True) is False
32+
assert resolve_tool_open_state("query") is False
33+
assert resolve_tool_open_state("update") is False
34+
assert resolve_tool_open_state("reset") is False
3835

3936

4037
def test_resolve_tool_open_state_default_setting(monkeypatch):
4138
"""Test 'default' setting."""
4239
monkeypatch.setenv("QUERYCHAT_TOOL_DETAILS", "default")
4340

44-
assert resolve_tool_open_state("query", False) is True
45-
assert resolve_tool_open_state("update", False) is True
46-
assert resolve_tool_open_state("reset", False) is False
47-
assert resolve_tool_open_state("query", True) is False
41+
assert resolve_tool_open_state("query") is True
42+
assert resolve_tool_open_state("update") is True
43+
assert resolve_tool_open_state("reset") is False
4844

4945

5046
def test_resolve_tool_open_state_case_insensitive(monkeypatch):
5147
"""Test that setting is case-insensitive."""
5248
monkeypatch.setenv("QUERYCHAT_TOOL_DETAILS", "EXPANDED")
53-
assert resolve_tool_open_state("query", False) is True
49+
assert resolve_tool_open_state("query") is True
5450

5551
monkeypatch.setenv("QUERYCHAT_TOOL_DETAILS", "Collapsed")
56-
assert resolve_tool_open_state("query", False) is False
52+
assert resolve_tool_open_state("query") is False
5753

5854
monkeypatch.setenv("QUERYCHAT_TOOL_DETAILS", "DeFaUlT")
59-
assert resolve_tool_open_state("query", False) is True
55+
assert resolve_tool_open_state("query") is True
6056

6157

6258
def test_resolve_tool_open_state_invalid_setting(monkeypatch):
@@ -65,20 +61,8 @@ def test_resolve_tool_open_state_invalid_setting(monkeypatch):
6561

6662
with warnings.catch_warnings(record=True) as w:
6763
warnings.simplefilter("always")
68-
result = resolve_tool_open_state("query", False)
64+
result = resolve_tool_open_state("query")
6965

7066
assert len(w) == 1
7167
assert "Invalid value" in str(w[0].message)
7268
assert result is True # Falls back to default behavior
73-
74-
75-
def test_resolve_tool_open_state_error_always_collapsed(monkeypatch):
76-
"""Test that errors always result in collapsed state."""
77-
monkeypatch.setenv("QUERYCHAT_TOOL_DETAILS", "expanded")
78-
assert resolve_tool_open_state("query", True) is False
79-
80-
monkeypatch.setenv("QUERYCHAT_TOOL_DETAILS", "collapsed")
81-
assert resolve_tool_open_state("query", True) is False
82-
83-
monkeypatch.delenv("QUERYCHAT_TOOL_DETAILS", raising=False)
84-
assert resolve_tool_open_state("query", True) is False

‎pkg-r/R/querychat_tools.R‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -108,12 +108,7 @@ tool_query <- function(data_source) {
108108
)
109109
}
110110

111-
resolve_tool_open_state <- function(action, is_error) {
112-
# If there's an error, always show collapsed
113-
if (is_error) {
114-
return(FALSE)
115-
}
116-
111+
resolve_tool_open_state <- function(action) {
117112
# Get the tool details setting
118113
setting <- querychat_tool_details_option()
119114

@@ -213,7 +208,7 @@ querychat_tool_result <- function(
213208
title = if (action == "update" && !is.null(title)) title,
214209
show_request = is_error,
215210
markdown = display_md,
216-
open = resolve_tool_open_state(action, is_error)
211+
open = resolve_tool_open_state(action)
217212
)
218213
)
219214
)

‎pkg-r/tests/testthat/test-querychat_tools.R‎

Lines changed: 22 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -2,70 +2,66 @@ test_that("resolve_tool_open_state respects default behavior", {
22
withr::local_options(querychat.tool_details = NULL)
33
withr::local_envvar(QUERYCHAT_TOOL_DETAILS = NA)
44

5-
expect_true(resolve_tool_open_state("query", FALSE))
6-
expect_true(resolve_tool_open_state("update", FALSE))
7-
expect_false(resolve_tool_open_state("reset", FALSE))
8-
expect_false(resolve_tool_open_state("query", TRUE))
5+
expect_true(resolve_tool_open_state("query"))
6+
expect_true(resolve_tool_open_state("update"))
7+
expect_false(resolve_tool_open_state("reset"))
98
})
109

1110
test_that("resolve_tool_open_state respects 'expanded' setting via option", {
1211
withr::local_options(querychat.tool_details = "expanded")
1312

14-
expect_true(resolve_tool_open_state("query", FALSE))
15-
expect_true(resolve_tool_open_state("update", FALSE))
16-
expect_true(resolve_tool_open_state("reset", FALSE))
17-
expect_false(resolve_tool_open_state("query", TRUE))
13+
expect_true(resolve_tool_open_state("query"))
14+
expect_true(resolve_tool_open_state("update"))
15+
expect_true(resolve_tool_open_state("reset"))
1816
})
1917

2018
test_that("resolve_tool_open_state respects 'collapsed' setting via option", {
2119
withr::local_options(querychat.tool_details = "collapsed")
2220

23-
expect_false(resolve_tool_open_state("query", FALSE))
24-
expect_false(resolve_tool_open_state("update", FALSE))
25-
expect_false(resolve_tool_open_state("reset", FALSE))
26-
expect_false(resolve_tool_open_state("query", TRUE))
21+
expect_false(resolve_tool_open_state("query"))
22+
expect_false(resolve_tool_open_state("update"))
23+
expect_false(resolve_tool_open_state("reset"))
2724
})
2825

2926
test_that("resolve_tool_open_state respects 'default' setting via option", {
3027
withr::local_options(querychat.tool_details = "default")
3128

32-
expect_true(resolve_tool_open_state("query", FALSE))
33-
expect_true(resolve_tool_open_state("update", FALSE))
34-
expect_false(resolve_tool_open_state("reset", FALSE))
35-
expect_false(resolve_tool_open_state("query", TRUE))
29+
expect_true(resolve_tool_open_state("query"))
30+
expect_true(resolve_tool_open_state("update"))
31+
expect_false(resolve_tool_open_state("reset"))
3632
})
3733

3834
test_that("resolve_tool_open_state respects 'expanded' setting via envvar", {
3935
withr::local_options(querychat.tool_details = NULL)
4036
withr::local_envvar(QUERYCHAT_TOOL_DETAILS = "expanded")
4137

42-
expect_true(resolve_tool_open_state("query", FALSE))
43-
expect_true(resolve_tool_open_state("update", FALSE))
44-
expect_true(resolve_tool_open_state("reset", FALSE))
38+
expect_true(resolve_tool_open_state("query"))
39+
expect_true(resolve_tool_open_state("update"))
40+
expect_true(resolve_tool_open_state("reset"))
4541
})
4642

4743
test_that("resolve_tool_open_state respects 'collapsed' setting via envvar", {
4844
withr::local_options(querychat.tool_details = NULL)
4945
withr::local_envvar(QUERYCHAT_TOOL_DETAILS = "collapsed")
5046

51-
expect_false(resolve_tool_open_state("query", FALSE))
52-
expect_false(resolve_tool_open_state("update", FALSE))
53-
expect_false(resolve_tool_open_state("reset", FALSE))
47+
expect_false(resolve_tool_open_state("query"))
48+
expect_false(resolve_tool_open_state("update"))
49+
expect_false(resolve_tool_open_state("reset"))
5450
})
5551

5652
test_that("resolve_tool_open_state is case-insensitive", {
5753
withr::local_options(querychat.tool_details = "EXPANDED")
58-
expect_true(resolve_tool_open_state("query", FALSE))
54+
expect_true(resolve_tool_open_state("query"))
5955

6056
withr::local_options(querychat.tool_details = "Collapsed")
61-
expect_false(resolve_tool_open_state("query", FALSE))
57+
expect_false(resolve_tool_open_state("query"))
6258
})
6359

6460
test_that("resolve_tool_open_state warns on invalid setting", {
6561
withr::local_options(querychat.tool_details = "invalid")
6662

6763
expect_warning(
68-
resolve_tool_open_state("query", FALSE),
64+
resolve_tool_open_state("query"),
6965
"Invalid value for"
7066
)
7167
})
@@ -74,5 +70,5 @@ test_that("option takes precedence over environment variable", {
7470
withr::local_options(querychat.tool_details = "collapsed")
7571
withr::local_envvar(QUERYCHAT_TOOL_DETAILS = "expanded")
7672

77-
expect_false(resolve_tool_open_state("query", FALSE))
73+
expect_false(resolve_tool_open_state("query"))
7874
})

0 commit comments

Comments
 (0)