Skip to content

feat: add sonar history command#16

Open
cosmicallycooked wants to merge 2 commits into1a35e1:mainfrom
cosmicallycooked:feature/history-command
Open

feat: add sonar history command#16
cosmicallycooked wants to merge 2 commits into1a35e1:mainfrom
cosmicallycooked:feature/history-command

Conversation

@cosmicallycooked
Copy link
Copy Markdown
Contributor

Summary

  • Adds a new sonar history command to browse previously triaged suggestions (archived, saved, read, skipped, replied)
  • Interactive browser with back/forward navigation (b/n keys, arrow keys) using the same card rendering as the triage session
  • Supports filtering by status (--status archived|saved|read|skipped|replied)
  • Also supports non-interactive output (--no-interactive), table rendering (--render table), and JSON output (--json)

Test plan

  • Run sonar history — verify it shows previously triaged suggestions in interactive browser
  • Press b / up arrow to go back, n / down arrow to go forward
  • Run sonar history --status archived — verify only archived items appear
  • Run sonar history --status saved — verify only saved/later items appear
  • Run sonar history --no-interactive — verify card list output
  • Run sonar history --render table — verify table output
  • Run sonar history --json — verify JSON output
  • Verify pagination loads more items as you navigate forward

🤖 Generated with Claude Code

…tions

Adds a new `sonar history` command that lets users browse suggestions they've
already triaged (archived, saved, read, skipped, replied). Includes an
interactive browser with back/forward navigation (b/n keys, arrow keys)
and supports filtering by status (--status archived|saved|read|skipped|replied).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 9, 2026

📝 Walkthrough

Walkthrough

Adds a new CLI command and UI for browsing previously triaged suggestions: a History command that queries GraphQL, maps/filters results, supports JSON/table/card/interactive output, and a HistoryBrowser component that provides keyboard navigation and paginated fetch-more behavior.

Changes

Cohort / File(s) Summary
History CLI command
src/commands/history.tsx
New Ink/React CLI command History: Zod options (status, limit, render, width, json, interactive), runs HISTORY_QUERY, maps results to HistoryItem, supports --json output, non-interactive table/cards rendering, and interactive mode wiring to fetchMore.
Interactive browser component
src/components/HistoryBrowser.tsx
New HistoryBrowser component and exported HistoryItem type: maintains items/total/index/loading, keyboard handlers (n/b/o/q), auto-fetches more when near end, renders current TweetCard with status label and help legend.
OS URL opener
src/lib/open.ts
New openUrl(url: string) utility: chooses platform command (open/start/xdg-open) and invokes it via execSync, swallowing errors.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant CLI as History CLI
    participant GQL as GraphQL Endpoint
    participant Browser as HistoryBrowser
    participant Card as TweetCard
    participant OS as openUrl

    User->>CLI: run `history` (flags)
    CLI->>GQL: HISTORY_QUERY (status?, limit, offset)
    GQL-->>CLI: suggestions + suggestionCounts
    CLI->>CLI: Filter (remove INBOX if unspecified) & map -> HistoryItem[]
    alt --json
        CLI->>User: write JSON stdout & exit
    else interactive
        CLI->>Browser: render with items, total, fetchMore
        Browser->>Browser: init state (items,total,index=0)
        loop navigation
            User->>Browser: n / b / o / q
            Browser->>Browser: update index or handle quit
            alt open action
                Browser->>OS: openUrl(tweet.xid URL)
            end
            alt near end & items < total
                Browser->>CLI: fetchMore(offset)
                CLI->>GQL: HISTORY_QUERY(offset)
                GQL-->>CLI: more suggestions
                CLI->>Browser: additional items
                Browser->>Browser: append items
            end
            Browser->>Card: render current item
        end
    else non-interactive
        alt render == "table"
            CLI->>CLI: render FeedTable
        else
            CLI->>Card: render stacked TweetCards with status labels
        end
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

Ready to merge

Poem

🐰 I hop through logs where old suggestions play,
Keys click, pages turn, I nibble at the day,
Cards and tables, JSON neat and bright,
I open links and bounce from left to right,
A tiny rabbit cheers the history's sight.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat: add sonar history command' directly and clearly describes the main change: adding a new history command feature to the sonar CLI tool.
Description check ✅ Passed The description is well-related to the changeset, providing a detailed summary of the new history command functionality, supported options, and a comprehensive test plan.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

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 `@src/commands/history.tsx`:
- Around line 226-229: The status color logic in the JSX rendering inside the
Box/Text (using item.status ternary) omits the REPLIED case so REPLIED items
render as blue; replace this inline ternary with a shared status-to-color
mapping (the same mapping used by src/components/HistoryBrowser.tsx) and use
that map to compute the color for item.status (e.g., lookup by item.status or
fallback to a default) so both History.tsx and HistoryBrowser.tsx stay
consistent; update the Text color prop to use the shared mapping function or
constant (referencing item.status, Text, and the existing mapping in
HistoryBrowser) and remove the long ternary.
- Around line 14-18: Constrain the CLI `status` option to the advertised values
by replacing the loose zod.string().optional() in the exported `options` schema
with a zod.enum(['archived','saved','read','skipped','replied']).optional(), and
update the `resolveStatus()` function to accept only that enum type (or
undefined) and map it to the GraphQL enum (e.g. uppercase or the exact enum name
expected by the API) rather than uppercasing arbitrary strings; ensure
`resolveStatus()` handles undefined/omitted status and returns the correct
mapped value for each allowed enum member.

In `@src/components/HistoryBrowser.tsx`:
- Around line 63-77: Pagination currently uses the displayed count
(items.length) as the backend cursor which breaks when rows are filtered
client-side; add a separate raw offset/cursor state (e.g., rawOffset) and use
that when calling fetchMore instead of items.length, increment rawOffset by the
number of rows returned by fetchMore (use the raw count returned, not
more.length if that is filtered), update the useEffect to depend on
rawOffset/index/loading/total as needed, and ensure setItems still appends only
the filtered/displayed rows while rawOffset tracks the true server-side
pagination progress for subsequent fetchMore calls.
- Around line 53-56: HistoryBrowser currently recomputes getFeedWidth() and
ignores the CLI --width override; update the component to accept cardWidth from
props instead of calling getFeedWidth(). Modify the HistoryBrowser signature to
include cardWidth (update HistoryBrowserProps accordingly), remove or replace
the line "const cardWidth = getFeedWidth()" and use the passed-in cardWidth
throughout the component, ensuring any place that referenced getFeedWidth() or
the local cardWidth now uses the prop value so the interactive path respects the
flags.width override.
- Around line 92-95: The current code in HistoryBrowser.tsx builds a URL from
remote-controlled fields (current.tweet.user.username/displayName) and passes it
into execSync which risks command injection and is macOS-only; replace the
execSync("open ...") call with a non-shell invocation (e.g., use the open npm
package via import open from 'open' and await open(url)) or call
execFile/child_process.spawn without a shell and pass the program and an argv
array (choose 'open'|'start'|'xdg-open' per process.platform) so that handle/url
are never interpolated into a shell command; update the branch handling for
input === 'o' to use that safe opener instead of execSync.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 76c1cd22-61e9-49a6-abfb-4ea8cd8b0cad

📥 Commits

Reviewing files that changed from the base of the PR and between 4be6cc0 and 8b7b308.

📒 Files selected for processing (2)
  • src/commands/history.tsx
  • src/components/HistoryBrowser.tsx

Comment thread src/commands/history.tsx
Comment on lines +14 to +18
export const options = zod.object({
status: zod
.string()
.optional()
.describe('Filter by status: archived|saved|read|skipped|replied (default: all non-inbox)'),
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n src/commands/history.tsx | head -80

Repository: 1a35e1/sonar-cli

Length of output: 3073


Constrain --status with Zod enum to match the advertised values.

The status option currently accepts any string value via .string().optional(). Invalid values like --status foo are accepted locally, then resolveStatus() uppercases them to FOO, and the request fails later when GraphQL rejects the unknown enum value. Since the help text advertises a fixed set of values (archived|saved|read|skipped|replied), constrain this option with zod.enum() to fail fast at the CLI layer.

The resolveStatus() function at lines 74-78 should also be updated to work only with the constrained enum values from the Zod schema.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/commands/history.tsx` around lines 14 - 18, Constrain the CLI `status`
option to the advertised values by replacing the loose zod.string().optional()
in the exported `options` schema with a
zod.enum(['archived','saved','read','skipped','replied']).optional(), and update
the `resolveStatus()` function to accept only that enum type (or undefined) and
map it to the GraphQL enum (e.g. uppercase or the exact enum name expected by
the API) rather than uppercasing arbitrary strings; ensure `resolveStatus()`
handles undefined/omitted status and returns the correct mapped value for each
allowed enum member.

Comment thread src/commands/history.tsx
Comment on lines +226 to +229
<Box marginLeft={2} marginBottom={i === items.length - 1 ? 0 : 1}>
<Text dimColor>
status: <Text color={item.status === 'ARCHIVED' ? 'gray' : item.status === 'LATER' ? 'yellow' : item.status === 'SKIPPED' ? 'red' : 'blue'}>{item.status.toLowerCase()}</Text>
</Text>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

REPLIED items render with the wrong color here.

This ternary never handles REPLIED, so replied history is shown as blue instead of green and diverges from src/components/HistoryBrowser.tsx. Reuse a shared status-to-color mapping to keep both render paths consistent.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/commands/history.tsx` around lines 226 - 229, The status color logic in
the JSX rendering inside the Box/Text (using item.status ternary) omits the
REPLIED case so REPLIED items render as blue; replace this inline ternary with a
shared status-to-color mapping (the same mapping used by
src/components/HistoryBrowser.tsx) and use that map to compute the color for
item.status (e.g., lookup by item.status or fallback to a default) so both
History.tsx and HistoryBrowser.tsx stay consistent; update the Text color prop
to use the shared mapping function or constant (referencing item.status, Text,
and the existing mapping in HistoryBrowser) and remove the long ternary.

Comment on lines +53 to +56
export function HistoryBrowser({ items: initialItems, total: initialTotal, fetchMore }: HistoryBrowserProps) {
const { stdout } = useStdout()
const termWidth = stdout.columns ?? 100
const cardWidth = getFeedWidth()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Interactive mode drops the --width override.

src/commands/history.tsx computes getFeedWidth(flags.width), but this component recomputes getFeedWidth() with no override, so the default interactive path ignores the documented --width flag. Pass cardWidth in via props instead of reading config again here.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/components/HistoryBrowser.tsx` around lines 53 - 56, HistoryBrowser
currently recomputes getFeedWidth() and ignores the CLI --width override; update
the component to accept cardWidth from props instead of calling getFeedWidth().
Modify the HistoryBrowser signature to include cardWidth (update
HistoryBrowserProps accordingly), remove or replace the line "const cardWidth =
getFeedWidth()" and use the passed-in cardWidth throughout the component,
ensuring any place that referenced getFeedWidth() or the local cardWidth now
uses the prop value so the interactive path respects the flags.width override.

Comment on lines +63 to +77
// Fetch next page when 3 items from the end
useEffect(() => {
if (!fetchMore || loading) return
if (index >= items.length - 3 && items.length < total) {
setLoading(true)
fetchMore(items.length)
.then(more => {
if (more.length > 0) {
setItems(prev => [...prev, ...more])
}
})
.catch(() => {})
.finally(() => setLoading(false))
}
}, [index, items.length, total, loading])
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

The pagination cursor is based on filtered item count.

fetchMore(items.length) only works if every fetched row becomes a rendered row. In this PR, src/commands/history.tsx filters INBOX items after the GraphQL call, so a page can consume more backend rows than it appends here. Once that happens, the next request reuses a stale raw offset and can duplicate/skip history entries or refetch the same page. Track a raw cursor/offset separately from the displayed item count.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/components/HistoryBrowser.tsx` around lines 63 - 77, Pagination currently
uses the displayed count (items.length) as the backend cursor which breaks when
rows are filtered client-side; add a separate raw offset/cursor state (e.g.,
rawOffset) and use that when calling fetchMore instead of items.length,
increment rawOffset by the number of rows returned by fetchMore (use the raw
count returned, not more.length if that is filtered), update the useEffect to
depend on rawOffset/index/loading/total as needed, and ensure setItems still
appends only the filtered/displayed rows while rawOffset tracks the true
server-side pagination progress for subsequent fetchMore calls.

Comment thread src/components/HistoryBrowser.tsx Outdated
Copy link
Copy Markdown
Owner

@1a35e1 1a35e1 left a comment

Choose a reason for hiding this comment

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

Good feature — history browsing is useful. A few things to fix before merge:

  1. Unused importsHistoryBrowser.tsx imports gql and relativeTime but never uses them. Remove.
  2. process.exit(0) — Use useApp().exit() instead of process.exit(0) for proper Ink cleanup (both in history.tsx and HistoryBrowser.tsx).
  3. Duplicate typesSuggestionItem duplicates the tweet shape from InteractiveSession.tsx. Import and reuse TriageItem instead.
  4. Status mapping — Verify against current triage actions: n=READ, s=LATER (saved), a=ARCHIVED, -=SKIPPED (bad rec). The STATUS_MAP should match.
  5. execSync('open') — Works on macOS only. Consider using xdg-open on Linux fallback, or note it's macOS-only.

Once these are addressed, happy to merge. Thanks!

- Remove unused imports (gql, relativeTime) from HistoryBrowser
- Use useApp().exit() instead of process.exit(0) for clean Ink teardown
- Reuse TriageItem type via extension instead of duplicating the interface
- Add cross-platform openUrl helper (macOS/Linux/Windows) in src/lib/open.ts
- STATUS_MAP verified to match all triage actions (read/saved/archived/skipped/replied)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (3)
src/components/HistoryBrowser.tsx (2)

34-39: ⚠️ Potential issue | 🟠 Major

Interactive path still ignores --width override.

Line 38 recomputes cardWidth with getFeedWidth() inside the component, so interactive rendering can diverge from CLI-provided width. Pass cardWidth via props.

🔧 Proposed fix
 import React, { useState, useEffect } from 'react'
 import { Box, Text, useApp, useInput, useStdout } from 'ink'
 import { TweetCard } from './TweetCard.js'
-import { getFeedWidth } from '../lib/config.js'
 import { openUrl } from '../lib/open.js'
 import type { TriageItem } from './InteractiveSession.js'

 interface HistoryBrowserProps {
   items: HistoryItem[]
   total: number
+  cardWidth: number
   fetchMore?: (offset: number) => Promise<HistoryItem[]>
 }

-export function HistoryBrowser({ items: initialItems, total: initialTotal, fetchMore }: HistoryBrowserProps) {
+export function HistoryBrowser({ items: initialItems, total: initialTotal, cardWidth, fetchMore }: HistoryBrowserProps) {
   const { exit } = useApp()
   const { stdout } = useStdout()
   const termWidth = stdout.columns ?? 100
-  const cardWidth = getFeedWidth()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/components/HistoryBrowser.tsx` around lines 34 - 39, The HistoryBrowser
component currently recomputes cardWidth by calling getFeedWidth() inside the
component, which ignores the CLI/parent provided width; change the component
signature to accept a cardWidth prop (e.g., HistoryBrowser({ items:
initialItems, total: initialTotal, fetchMore, cardWidth }: HistoryBrowserProps))
and remove the internal call to getFeedWidth(), using the passed-in cardWidth
everywhere; update the HistoryBrowserProps type/interface and any callers to
pass the CLI/parent width through so interactive rendering respects the --width
override.

45-53: ⚠️ Potential issue | 🟠 Major

Pagination cursor is still based on filtered/displayed count.

Line 50 uses items.length as backend offset. This breaks when upstream filtering removes rows before render, causing duplicate/skip fetch windows. Track a raw cursor independently from displayed list length.

🔧 Proposed direction
 interface HistoryBrowserProps {
   items: HistoryItem[]
   total: number
-  fetchMore?: (offset: number) => Promise<HistoryItem[]>
+  fetchMore?: (offset: number) => Promise<{ items: HistoryItem[]; rawCount: number }>
 }

+const [rawOffset, setRawOffset] = useState(initialItems.length)
 ...
-      fetchMore(items.length)
-        .then(more => {
-          if (more.length > 0) {
-            setItems(prev => [...prev, ...more])
-          }
+      fetchMore(rawOffset)
+        .then(({ items: more, rawCount }) => {
+          if (more.length > 0) setItems(prev => [...prev, ...more])
+          setRawOffset(prev => prev + rawCount)
         })
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/components/HistoryBrowser.tsx` around lines 45 - 53, The pagination
currently uses items.length as the backend offset inside the useEffect block
that calls fetchMore, which breaks when UI filtering changes displayed items;
add a separate raw cursor state (e.g., rawCursor via useState) and use that
cursor when calling fetchMore(rawCursor) instead of items.length,
increment/update rawCursor by more.length when the promise resolves, and ensure
any initial load sets rawCursor to the initial backend count so fetch windows
are based on unfiltered/raw rows rather than the displayed items array; update
references in the useEffect, setLoading, and the fetchMore resolution handler
(functions/variables: useEffect, fetchMore, items, setItems, setLoading, total,
index) accordingly.
src/lib/open.ts (1)

4-12: ⚠️ Potential issue | 🔴 Critical

Replace shell-interpolated execSync URL opening with argv-based spawn/execFile.

Line 11 builds a shell command from url, which is injection-prone and brittle on Windows (start parsing). Use non-shell process invocation with explicit args.

🔧 Proposed fix
-import { execSync } from 'child_process'
-import { platform } from 'os'
+import { spawn } from 'child_process'

 export function openUrl(url: string): void {
-  const cmd =
-    platform() === 'darwin' ? 'open'
-    : platform() === 'win32' ? 'start'
-    : 'xdg-open'
-
   try {
-    execSync(`${cmd} "${url}"`, { stdio: 'ignore' })
+    const target = new URL(url).toString()
+    const isWin = process.platform === 'win32'
+    const [cmd, args] = process.platform === 'darwin'
+      ? ['open', [target]]
+      : isWin
+        ? ['cmd', ['/c', 'start', '', target]]
+        : ['xdg-open', [target]]
+
+    const child = spawn(cmd, args, { stdio: 'ignore', detached: true, shell: false })
+    child.unref()
   } catch {}
 }
#!/bin/bash
# Verify shell-based URL opening patterns and call sites.
rg -nP --type=ts --type=tsx -C2 '\bexecSync\s*\(' src
rg -nP --type=ts --type=tsx -C2 'openUrl\s*\(' src
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/lib/open.ts` around lines 4 - 12, The openUrl function currently builds a
shell command and calls execSync with a string, which is injection-prone;
replace that with a non-shell child process invocation (child_process.spawn or
child_process.execFile) passing the executable and url as separate args. Update
openUrl to: keep the platform() branch to select the program name, but call
spawn/execFile with an args array (e.g., on macOS/Linux spawn('open'|'xdg-open',
[url]), on Windows spawn('cmd', ['/c', 'start', '""', url]) or use
PowerShell/start-process pattern) with shell: false and stdio: 'ignore', and
remove the string-interpolated execSync call and empty catch block; ensure
errors are caught and handled if needed.
🧹 Nitpick comments (1)
src/lib/open.ts (1)

10-12: Don’t silently swallow opener failures.

Lines 10-12 suppress all errors, so users get no feedback when open fails. Return a boolean (or emit a warning) so the caller can show a non-blocking error message.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/lib/open.ts` around lines 10 - 12, The try/catch around execSync in the
open logic currently swallows all errors; update the open function (the block
calling execSync with `${cmd} "${url}"`) to catch errors and return a boolean
(true on success, false on failure) or emit a non-blocking warning;
specifically, replace the empty catch with logic that logs a concise warning
including the caught error (e.g., via console.warn or a passed logger) and
returns false so callers can surface a user-facing message, while returning true
on success when execSync completes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/components/HistoryBrowser.tsx`:
- Around line 73-75: When building the X URL in HistoryBrowser (variables:
handle, current.tweet.user.username, current.tweet.user.displayName, url,
openUrl), only use the username when present and valid; if username is missing,
construct and open the status-only route (for example use the /i/web/status/{id}
form) instead of falling back to displayName which may include spaces/symbols;
implement a conditional that sets url = `https://x.com/${username}/status/${id}`
when username exists, otherwise url = `https://x.com/i/web/status/${id}`, then
call openUrl(url).

---

Duplicate comments:
In `@src/components/HistoryBrowser.tsx`:
- Around line 34-39: The HistoryBrowser component currently recomputes cardWidth
by calling getFeedWidth() inside the component, which ignores the CLI/parent
provided width; change the component signature to accept a cardWidth prop (e.g.,
HistoryBrowser({ items: initialItems, total: initialTotal, fetchMore, cardWidth
}: HistoryBrowserProps)) and remove the internal call to getFeedWidth(), using
the passed-in cardWidth everywhere; update the HistoryBrowserProps
type/interface and any callers to pass the CLI/parent width through so
interactive rendering respects the --width override.
- Around line 45-53: The pagination currently uses items.length as the backend
offset inside the useEffect block that calls fetchMore, which breaks when UI
filtering changes displayed items; add a separate raw cursor state (e.g.,
rawCursor via useState) and use that cursor when calling fetchMore(rawCursor)
instead of items.length, increment/update rawCursor by more.length when the
promise resolves, and ensure any initial load sets rawCursor to the initial
backend count so fetch windows are based on unfiltered/raw rows rather than the
displayed items array; update references in the useEffect, setLoading, and the
fetchMore resolution handler (functions/variables: useEffect, fetchMore, items,
setItems, setLoading, total, index) accordingly.

In `@src/lib/open.ts`:
- Around line 4-12: The openUrl function currently builds a shell command and
calls execSync with a string, which is injection-prone; replace that with a
non-shell child process invocation (child_process.spawn or
child_process.execFile) passing the executable and url as separate args. Update
openUrl to: keep the platform() branch to select the program name, but call
spawn/execFile with an args array (e.g., on macOS/Linux spawn('open'|'xdg-open',
[url]), on Windows spawn('cmd', ['/c', 'start', '""', url]) or use
PowerShell/start-process pattern) with shell: false and stdio: 'ignore', and
remove the string-interpolated execSync call and empty catch block; ensure
errors are caught and handled if needed.

---

Nitpick comments:
In `@src/lib/open.ts`:
- Around line 10-12: The try/catch around execSync in the open logic currently
swallows all errors; update the open function (the block calling execSync with
`${cmd} "${url}"`) to catch errors and return a boolean (true on success, false
on failure) or emit a non-blocking warning; specifically, replace the empty
catch with logic that logs a concise warning including the caught error (e.g.,
via console.warn or a passed logger) and returns false so callers can surface a
user-facing message, while returning true on success when execSync completes.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9771fa48-6138-484d-9a79-9b491fc5e7ca

📥 Commits

Reviewing files that changed from the base of the PR and between 8b7b308 and 99d2ca0.

📒 Files selected for processing (3)
  • src/commands/history.tsx
  • src/components/HistoryBrowser.tsx
  • src/lib/open.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/commands/history.tsx

Comment on lines +73 to +75
const handle = current.tweet.user.username ?? current.tweet.user.displayName
const url = `https://x.com/${handle}/status/${current.tweet.id}`
openUrl(url)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Handle missing usernames when building X URLs.

Line 73 falls back to displayName, which may contain spaces/symbols and produce invalid profile-path URLs. Use username when present; otherwise open the status-only route.

🔧 Proposed fix
-      const handle = current.tweet.user.username ?? current.tweet.user.displayName
-      const url = `https://x.com/${handle}/status/${current.tweet.id}`
+      const username = current.tweet.user.username?.trim()
+      const tweetId = encodeURIComponent(current.tweet.id)
+      const url = username
+        ? `https://x.com/${encodeURIComponent(username)}/status/${tweetId}`
+        : `https://x.com/i/web/status/${tweetId}`
       openUrl(url)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/components/HistoryBrowser.tsx` around lines 73 - 75, When building the X
URL in HistoryBrowser (variables: handle, current.tweet.user.username,
current.tweet.user.displayName, url, openUrl), only use the username when
present and valid; if username is missing, construct and open the status-only
route (for example use the /i/web/status/{id} form) instead of falling back to
displayName which may include spaces/symbols; implement a conditional that sets
url = `https://x.com/${username}/status/${id}` when username exists, otherwise
url = `https://x.com/i/web/status/${id}`, then call openUrl(url).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants