Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions crates/forge_api/src/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,16 @@ pub trait API: Sync + Send {
limit: Option<usize>,
) -> Result<Option<Vec<Conversation>>>;

/// Return an FTS5 snippet for a (conversation, query) pair — a short
/// highlighted excerpt of the matched passage. Used by the search UI
/// to render a preview pane when the user picks a search hit.
async fn get_conversation_snippet(
&self,
conversation_id: &ConversationId,
query: &str,
token_count: usize,
) -> Result<Option<String>>;
Comment on lines +143 to +151

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Expand the new method's Rust doc comments to include explicit # Arguments entries for each parameter and a # Errors section describing when the Result returns an error. [custom_rule]

Severity Level: Minor ⚠️

Why it matters? 🤔

This is a newly added public trait method, and its Rust doc comment only contains a summary. It has three parameters and returns a Result, but the docs do not include the required # Arguments section or any # Errors section. That matches the custom documentation rule violation.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** crates/forge_api/src/api.rs
**Line:** 143:151
**Comment:**
	*Custom Rule: Expand the new method's Rust doc comments to include explicit `# Arguments` entries for each parameter and a `# Errors` section describing when the `Result` returns an error.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎


/// Renames a conversation by setting its title
///
/// # Arguments
Expand Down
11 changes: 11 additions & 0 deletions crates/forge_api/src/forge_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,17 @@ impl<
self.services.get_conversations_by_cwd(cwd, limit).await
}

async fn get_conversation_snippet(
&self,
conversation_id: &ConversationId,
query: &str,
token_count: usize,
) -> Result<Option<String>> {
self.services
.get_conversation_snippet(conversation_id, query, token_count)
.await
}

async fn rename_conversation(
&self,
conversation_id: &ConversationId,
Expand Down
21 changes: 21 additions & 0 deletions crates/forge_app/src/services.rs
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,16 @@ pub trait ConversationService: Send + Sync {
cwd: &str,
limit: Option<usize>,
) -> anyhow::Result<Option<Vec<Conversation>>>;

/// Return an FTS5 snippet for a (conversation, query) pair — a short
/// highlighted excerpt of the matched passage. Used by the search UI
/// to render a preview pane when the user picks a search hit.
async fn get_conversation_snippet(
&self,
conversation_id: &ConversationId,
query: &str,
token_count: usize,
) -> anyhow::Result<Option<String>>;
Comment on lines +320 to +328

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Expand the doc comment for this new trait method to include # Arguments for conversation_id, query, and token_count, and add a # Errors section describing failure conditions from the snippet lookup. [custom_rule]

Severity Level: Minor ⚠️

Why it matters? 🤔

This is a newly added public trait method, so it must be documented with Rustdoc.
The current comment explains the method but does not include the required
# Arguments and # Errors sections for its parameters and failure cases,
so the suggestion correctly identifies a real documentation rule violation.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** crates/forge_app/src/services.rs
**Line:** 320:328
**Comment:**
	*Custom Rule: Expand the doc comment for this new trait method to include `# Arguments` for `conversation_id`, `query`, and `token_count`, and add a `# Errors` section describing failure conditions from the snippet lookup.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

}

#[async_trait::async_trait]
Expand Down Expand Up @@ -761,6 +771,17 @@ impl<I: Services> ConversationService for I {
.get_conversations_by_cwd(cwd, limit)
.await
}

async fn get_conversation_snippet(
&self,
conversation_id: &ConversationId,
query: &str,
token_count: usize,
) -> anyhow::Result<Option<String>> {
self.conversation_service()
.get_conversation_snippet(conversation_id, query, token_count)
.await
}
}
#[async_trait::async_trait]
impl<I: Services> ProviderService for I {
Expand Down
55 changes: 55 additions & 0 deletions crates/forge_domain/src/conversation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,61 @@ impl MetaData {
}
}

/// Sort key for the session viewer selector.
///
/// Each variant maps to an `ORDER BY` clause in the `conversations` table.
/// `Default` is `Updated` because the most common workflow is "show me what
/// I was working on most recently" — especially after a crash recovery when
/// the user is trying to find the parent session of a stranded subagent.
#[derive(Debug, Default, Display, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)]
#[serde(rename_all = "lowercase")]
pub enum ConversationSort {
/// Sort by `updated_at` DESC (most recent first). Default.
#[default]
#[display("updated")]
Updated,
/// Sort by `created_at` DESC (newest first).
#[display("created")]
Created,
/// Sort by `message_count` DESC, then `updated_at` DESC.
/// This is the canonical "turns" view the user asked for.
#[display("turns")]
Turns,
/// Sort by `title` ASC, NULLS LAST.
#[display("title")]
Title,
/// Sort by `cwd` ASC, NULLS LAST, then `updated_at` DESC.
/// Useful for finding all sessions in a specific repo.
#[display("cwd")]
Cwd,
}

impl ConversationSort {
/// Stable lowercase identifier used for CLI parsing and storage.
/// Also used by the UI handler for `:sort <key>` echo.
pub fn name(self) -> &'static str {
match self {
ConversationSort::Updated => "updated",
ConversationSort::Created => "created",
ConversationSort::Turns => "turns",
ConversationSort::Title => "title",
ConversationSort::Cwd => "cwd",
}
}

/// Parse a sort key from a user-supplied string. Unknown keys fall
/// back to `Updated` and the caller is expected to print a hint.
pub fn parse(s: &str) -> Self {
Comment on lines +120 to +122

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Add a Rustdoc # Arguments section for this public method to document the input parameter as required by the project documentation rule. [custom_rule]

Severity Level: Minor ⚠️

Why it matters? 🤔

This is a newly added public function that takes a parameter, but its Rustdoc only has a brief description and no # Arguments section. That matches the project documentation rule violation.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** crates/forge_domain/src/conversation.rs
**Line:** 120:122
**Comment:**
	*Custom Rule: Add a Rustdoc `# Arguments` section for this public method to document the input parameter as required by the project documentation rule.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

match s.trim().to_ascii_lowercase().as_str() {
"created" => ConversationSort::Created,
"turns" | "messages" | "msgs" => ConversationSort::Turns,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WARNING: ConversationSort::parse and handle_sort accept inconsistent aliases

parse accepts msgs and alphabetical but silently falls back to Updated for msg, count, created_at, oldest, updated_at, and recent. handle_sort does the opposite: it accepts those keywords but rejects msgs and alphabetical.

This creates a public API mismatch: parse("created_at") returns Updated while :sort created_at correctly maps to Created. Any external consumer of forge_domain using parse will get incorrect fallback behavior for these aliases.


Reply with @kilocode-bot fix it to have Kilo Code address this issue.

"title" | "name" | "alphabetical" => ConversationSort::Title,
"cwd" | "dir" | "directory" => ConversationSort::Cwd,
_ => ConversationSort::Updated,
}
}
}

impl Conversation {
pub fn new(id: ConversationId) -> Self {
let created_at = Utc::now();
Expand Down
22 changes: 22 additions & 0 deletions crates/forge_domain/src/repo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,28 @@ pub trait ConversationRepository: Send + Sync {
limit: Option<usize>,
) -> Result<Vec<Conversation>>;

/// Returns a short FTS5 snippet (~32 tokens) for a single
/// `(conversation_id, query)` pair, with the matched terms wrapped in
/// `[…]` and the surrounding text wrapped in `…`. Used by the UI to
/// render a "matched passage" preview for the currently selected
/// search hit without forcing the main search query to include the
/// snippet column (which would couple the row layout to
/// `ConversationRecord`).
///
/// Returns `Ok(None)` when the query does not match that conversation
/// — callers should treat `None` as "no preview available" and fall
/// back to the conversation title.
///
/// # Errors
/// Returns an error if the FTS query is malformed or the database
/// call fails.
async fn get_conversation_snippet(
&self,
conversation_id: &ConversationId,
query: &str,
token_count: usize,
) -> Result<Option<String>>;
Comment on lines +173 to +193

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Add a # Arguments section to this new public method's doc comment and document each parameter (conversation_id, query, and token_count) to satisfy the required Rust doc structure for parameterized APIs. [custom_rule]

Severity Level: Minor ⚠️

Why it matters? 🤔

This is a newly added public method with three parameters, but its doc comment only includes a # Errors section.
The required Rust doc structure in the rule specifies that parameterized APIs must include a # Arguments section documenting each parameter, so the violation is real.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** crates/forge_domain/src/repo.rs
**Line:** 173:193
**Comment:**
	*Custom Rule: Add a `# Arguments` section to this new public method's doc comment and document each parameter (`conversation_id`, `query`, and `token_count`) to satisfy the required Rust doc structure for parameterized APIs.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎


/// Reclaims FTS5 segment shadow data by running
/// `INSERT INTO conversations_fts(conversations_fts) VALUES('optimize')`.
///
Expand Down
2 changes: 2 additions & 0 deletions crates/forge_main/src/conversation_selector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,8 @@ mod tests {
context: None,
metrics: Metrics::default().started_at(now),
metadata: MetaData { created_at: now, updated_at: Some(now) },
cwd: None,
message_count: None,
parent_id: None,
source: None,
}
Expand Down
6 changes: 6 additions & 0 deletions crates/forge_main/src/info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -979,6 +979,8 @@ mod tests {
context: None,
metrics,
metadata: forge_domain::MetaData::new(Utc::now()),
cwd: None,
message_count: None,
parent_id: None,
source: None,
};
Expand Down Expand Up @@ -1008,6 +1010,8 @@ mod tests {
context: None,
metrics,
metadata: forge_domain::MetaData::new(Utc::now()),
cwd: None,
message_count: None,
parent_id: None,
source: None,
};
Expand Down Expand Up @@ -1055,6 +1059,8 @@ mod tests {
context: Some(context),
metrics,
metadata: forge_domain::MetaData::new(Utc::now()),
cwd: None,
message_count: None,
parent_id: None,
source: None,
};
Expand Down
13 changes: 13 additions & 0 deletions crates/forge_main/src/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -712,6 +712,18 @@ pub enum AppCommand {
target: Vec<String>,
},

/// Sort the conversation selector. Usage: `:sort <key>` where key is
/// one of `updated`, `created`, `turns`, `title`. Persists in
/// `UIState.sort` until the session exits or another `:sort` is run.
#[strum(props(usage = "Sort the conversation selector. Usage: :sort <key> (updated|created|turns|title)"))]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The :sort usage string is out of sync with the actual command behavior: it omits supported options like cwd and --reset that are handled in handle_sort. This creates an API/help-contract mismatch where users cannot discover valid inputs from built-in help. Update the usage text to include all accepted keys and reset behavior. [api mismatch]

Severity Level: Major ⚠️
- ⚠️ `forge list command` shows outdated `:sort` usage.
- ⚠️ REPL command listing omits `cwd` and `--reset` options.
- ⚠️ Users may not discover directory-based sorting feature.
- ⚠️ Users may not discover sort reset behavior via help.
Steps of Reproduction ✅
1. Run the CLI with `forge list command` (TopLevelCommand::List::Command in
`crates/forge_main/src/cli.rs:7-13`), which is handled in `UI::run` and dispatched to
`on_show_commands` at `crates/forge_main/src/ui.rs:515-522`.

2. Inside `on_show_commands` (`crates/forge_main/src/ui.rs:1583-1595`), a
`ForgeCommandManager::default()` is created, which in turn calls
`ForgeCommandManager::default_commands()` at `crates/forge_main/src/model.rs:171-41`.

3. `default_commands()` iterates `AppCommand::iter()` and builds `ForgeCommand` entries
using `name: command.name().to_string()` and `description: command.usage().to_string()`
(`crates/forge_main/src/model.rs:32-40`). For `AppCommand::Sort { .. }`, `usage()`
resolves to the `#[strum(props(usage = ...))]` string at
`crates/forge_main/src/model.rs:718`, which is: `Sort the conversation selector. Usage:
:sort <key> (updated|created|turns|title)`.

4. In the interactive REPL or from `forge list command`, observe the printed description
for the `sort` command: it does not mention the `cwd` sort key or the `--reset` flag, even
though the actual handler `handle_sort` in `crates/forge_main/src/ui.rs:2441-2455` clearly
supports `cwd` (branch at line 29) and `--reset` (lines 14–20) and prints a more complete
usage string: `"Usage: :sort <turns|updated|created|title|cwd> | :sort --reset"`. This
demonstrates a concrete mismatch between the built-in help text and the real command
contract.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** crates/forge_main/src/model.rs
**Line:** 718:718
**Comment:**
	*Api Mismatch: The `:sort` usage string is out of sync with the actual command behavior: it omits supported options like `cwd` and `--reset` that are handled in `handle_sort`. This creates an API/help-contract mismatch where users cannot discover valid inputs from built-in help. Update the usage text to include all accepted keys and reset behavior.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

#[command(alias = "so")]
Sort {
/// Sort key: `updated` (default), `created`, `turns`, or `title`.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SUGGESTION: Field doc omits cwd sort key

This doc lists updated (default), created, turns, or title, but the handler also accepts cwd (and dir, directory). The usage string at line 718 already suffers from the same omission and is flagged.


Reply with @kilocode-bot fix it to have Kilo Code address this issue.

/// Anything else falls back to `updated` and prints a hint.
#[arg(trailing_var_arg = true, num_args = 0..)]
target: Vec<String>,
},

/// Full-text search over conversation titles and contents (FTS5 BM25).
/// Usage: `:search <query>` or `:search "rust refactor"`.
#[strum(props(usage = "Search conversation history. Usage: :search <query>"))]
Expand Down Expand Up @@ -812,6 +824,7 @@ impl AppCommand {
AppCommand::Parent => "parent",
AppCommand::Reparent { .. } => "reparent",
AppCommand::Cwd { .. } => "cwd",
AppCommand::Sort { .. } => "sort",
AppCommand::Search { .. } => "search",
AppCommand::Delete => "delete",
AppCommand::Rename { .. } => "rename",
Expand Down
7 changes: 7 additions & 0 deletions crates/forge_main/src/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use std::time::Instant;

use derive_setters::Setters;
use forge_api::{ConversationId, Environment};
use forge_domain::ConversationSort;

//TODO: UIState and ForgePrompt seem like the same thing and can be merged
/// State information for the UI
Expand All @@ -18,6 +19,10 @@ pub struct UIState {
/// scopes its results to conversations whose `cwd` column matches.
/// This is the "filter by project directory" UX.
pub cwd_filter: Option<String>,
/// Sort key for the conversation selector. Re-exported from
/// `forge_domain::ConversationSort` so there's one canonical enum
/// across the repo / service / UI layers.
pub sort: ConversationSort,
}

impl Default for UIState {
Expand All @@ -29,6 +34,7 @@ impl Default for UIState {
loop_enabled: false,
last_activity: Instant::now(),
cwd_filter: None,
sort: ConversationSort::default(),
}
}
}
Expand All @@ -42,6 +48,7 @@ impl UIState {
loop_enabled: false,
last_activity: Instant::now(),
cwd_filter: None,
sort: ConversationSort::default(),
}
}
}
61 changes: 61 additions & 0 deletions crates/forge_main/src/ui.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2317,6 +2317,21 @@ impl<A: API + ConsoleWriter + 'static, F: Fn(ForgeConfig) -> A + Send + Sync> UI
.await?
{
let conversation_id = conversation.id;

// Fetch a short FTS5 snippet (~32 tokens) so the user can see
// *why* this conversation matched. `None` means no preview —
// fall through silently (the title is already shown above).
if let Ok(Some(snippet)) = self
.api
.get_conversation_snippet(&conversation_id, &query, 32)
.await
{
self.writeln_title(TitleFormat::info(format!(
" matched: {}",
snippet.dimmed()
)))?;
}

self.state.conversation_id = Some(conversation_id);
self.on_show_last_message(conversation, false).await?;
self.writeln_title(TitleFormat::info(format!(
Expand Down Expand Up @@ -2423,6 +2438,49 @@ impl<A: API + ConsoleWriter + 'static, F: Fn(ForgeConfig) -> A + Send + Sync> UI
Ok(())
}

async fn handle_sort(&mut self, target: Vec<String>) -> anyhow::Result<()> {
use forge_domain::ConversationSort;

if target.is_empty() || target.iter().any(|t| t == "--help" || t == "-h") {
self.writeln_title(TitleFormat::info(
"Usage: :sort <turns|updated|created|title|cwd> | :sort --reset",
))?;
return Ok(());
}

if target.iter().any(|t| t == "--reset") {
self.state.sort = ConversationSort::default();
self.writeln_title(TitleFormat::info(format!(
"Sort reset to {}",
ConversationSort::default().name().bold()
)))?;
return Ok(());
}

let requested = target.join(" ").trim().to_lowercase();
let new_sort = match requested.as_str() {
"turns" | "messages" | "msg" | "count" => ConversationSort::Turns,
"updated" | "updated_at" | "recent" => ConversationSort::Updated,
"created" | "created_at" | "oldest" => ConversationSort::Created,
"title" | "name" => ConversationSort::Title,
"cwd" | "dir" | "directory" => ConversationSort::Cwd,
other => {
self.writeln_title(TitleFormat::error(format!(
"Unknown sort key: {} (use: turns|updated|created|title|cwd)",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SUGGESTION: Error message lists only 5 keys but handler accepts many more aliases

The error says use: turns|updated|created|title|cwd but handle_sort also accepts messages, msg, count, updated_at, recent, created_at, oldest, dir, directory, and name.


Reply with @kilocode-bot fix it to have Kilo Code address this issue.

other
)))?;
return Ok(());
}
};

self.state.sort = new_sort;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Use the selected sort when loading conversations

When a user runs :sort turns/title/cwd, this only stores new_sort and prints success; the selector/list paths I checked still call get_parent_conversations(...), which orders by updated_at.desc(), and rg "state\.sort" shows no reader outside initialization and this handler. As a result the next :conversation picker remains in recency order, so the new sort command has no observable effect.

Useful? React with 👍 / 👎.

self.writeln_title(TitleFormat::info(format!(
"Sort set to {}",
new_sort.name().bold()
)))?;
Ok(())
}

fn user_initiated_conversations(conversations: Vec<Conversation>) -> Vec<Conversation> {
let related_ids: HashSet<ConversationId> = conversations
.iter()
Expand Down Expand Up @@ -2483,6 +2541,9 @@ impl<A: API + ConsoleWriter + 'static, F: Fn(ForgeConfig) -> A + Send + Sync> UI
AppCommand::Cwd { target } => {
self.handle_cwd(target).await?;
}
AppCommand::Sort { target } => {
self.handle_sort(target).await?;
}
AppCommand::Search { query } => {
self.handle_search(query).await?;
}
Expand Down
Loading
Loading