-
Notifications
You must be signed in to change notification settings - Fork 0
feat(forgecode): v3-cleanup — FTS5 message snippets, ConversationSort SSOT, perf audit #24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Expand the Rust doc comment for this newly added public trait method to include explicit Severity Level: Minor Why it matters? 🤔This is a newly added public trait method, and its Rustdoc only has a short description. (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 Rust doc comment for this newly added public trait method to include explicit `# Arguments` and `# Errors` sections describing each parameter and when the method 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 |
||
| } | ||
|
|
||
| #[async_trait::async_trait] | ||
|
|
@@ -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 { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Expand the Rust docs for this public method to include a Severity Level: Minor Why it matters? 🤔This is a newly added public method with a parameter, and its docs do not include a (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: Expand the Rust docs for this public method to include a `# Arguments` section describing the input parameter.
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, | ||
| "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(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Add a Severity Level: Minor Why it matters? 🤔The method is public and newly added, and it has parameters but no (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 the Rust doc comment for this method and document `conversation_id`, `query`, and `token_count` so the public API docs are complete for a parameterized function.
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')`. | ||
| /// | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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)"))] | ||
|
Comment on lines
+715
to
+718
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The Severity Level: Major
|
||
| #[command(alias = "so")] | ||
| Sort { | ||
| /// Sort key: `updated` (default), `created`, `turns`, or `title`. | ||
| /// 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>"))] | ||
|
|
@@ -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", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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!( | ||
|
|
@@ -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)", | ||
| other | ||
| )))?; | ||
| return Ok(()); | ||
| } | ||
| }; | ||
|
|
||
| self.state.sort = new_sort; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This assignment makes 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() | ||
|
|
@@ -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?; | ||
| } | ||
|
|
||
There was a problem hiding this comment.
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
get_conversation_snippetto include# Argumentsfor all parameters and a# Errorssection describing when theResultcan fail. [custom_rule]Severity Level: Minor⚠️
Why it matters? 🤔
This is a newly added public trait method returning
Resultand taking parameters, but its doc comment only contains a summary. It lacks the required# Argumentssection for the parameters and a# Errorssection describing failure cases, so it violates the Rustdoc rule.(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖