fix(log-view): address PR #362 review comments

- Normalize hex IDs to lowercase in validateHexId for consistent API comparison
- Remove dead getLog convenience wrapper (unused)
- Simplify getLogs to always use bracket syntax for ID filter
- Add per_page comment explaining why it's needed
- Open all IDs in browser with --web (warn about multiple tabs)
- Use markdown list format for error messages (- `<id>`)
- Use consola logger for missing IDs warning instead of stderr.write
- Use logIds.join(' ') in usage hints
- Extract resolveTarget, warnMissingIds, formatIdList helpers for complexity
- Update trace-id tests for lowercase normalization

Author: BYK

AGENTS.md

@@ -15,13 +15,16 @@ import { buildCommand } from "../../lib/command.js";
 import { ContextError, ValidationError } from "../../lib/errors.js";
 import { formatLogDetails, writeJson } from "../../lib/formatters/index.js";
 import { validateHexId } from "../../lib/hex-id.js";
+import { logger } from "../../lib/logger.js";
 import {
   resolveOrgAndProject,
   resolveProjectBySlug,
 } from "../../lib/resolve-target.js";
 import { buildLogsUrl } from "../../lib/sentry-urls.js";
 import type { DetailedSentryLog, Writer } from "../../types/index.js";
 
+const log = logger.withTag("log-view");
+
 type ViewFlags = {
   readonly json: boolean;
   readonly web: boolean;
@@ -108,6 +111,79 @@ export type ResolvedLogTarget = {
   detectedFrom?: string;
 };
 
+/**
+ * Resolve the target org/project from the parsed arg.
+ *
+ * @param parsed - Result of `parseOrgProjectArg`
+ * @param logIds - Parsed log IDs (used for usage hints)
+ * @param cwd - Current working directory
+ * @param stderr - Stderr stream for diagnostics
+ * @returns Resolved target, or null if resolution produced nothing
+ * @throws {ContextError} If org-all mode is used (requires specific project)
+ */
+function resolveTarget(
+  parsed: ReturnType<typeof parseOrgProjectArg>,
+  logIds: string[],
+  cwd: string,
+  stderr: Writer
+): Promise<ResolvedLogTarget | null> | ResolvedLogTarget {
+  switch (parsed.type) {
+    case "explicit":
+      return { org: parsed.org, project: parsed.project };
+
+    case "project-search":
+      return resolveProjectBySlug(
+        parsed.projectSlug,
+        USAGE_HINT,
+        `sentry log view <org>/${parsed.projectSlug} ${logIds.join(" ")}`,
+        stderr
+      );
+
+    case "org-all":
+      throw new ContextError("Specific project", USAGE_HINT);
+
+    case "auto-detect":
+      return resolveOrgAndProject({ cwd, usageHint: USAGE_HINT });
+
+    default: {
+      const _exhaustiveCheck: never = parsed;
+      throw new ValidationError(
+        `Invalid target specification: ${_exhaustiveCheck}`
+      );
+    }
+  }
+}
+
+/**
+ * Format a list of log IDs as a markdown bullet list.
+ *
+ * @param ids - Log IDs to format
+ * @returns Markdown list string with each ID on its own line
+ */
+function formatIdList(ids: string[]): string {
+  return ids.map((id) => ` - \`${id}\``).join("\n");
+}
+
+/**
+ * Warn about IDs that weren't found in the API response.
+ * Uses the consola logger for structured output to stderr.
+ *
+ * @param logIds - All requested IDs
+ * @param logs - Logs actually returned by the API
+ */
+function warnMissingIds(logIds: string[], logs: DetailedSentryLog[]): void {
+  if (logs.length >= logIds.length) {
+    return;
+  }
+  const foundIds = new Set(logs.map((l) => l["sentry.item_id"]));
+  const missing = logIds.filter((id) => !foundIds.has(id));
+  if (missing.length > 0) {
+    log.warn(
+      `${missing.length} of ${logIds.length} log(s) not found:\n${formatIdList(missing)}`
+    );
+  }
+}
+
 /**
  * Write human-readable output for one or more logs to stdout.
  *
@@ -123,11 +199,11 @@ function writeHumanOutput(
   detectedFrom?: string
 ): void {
   let first = true;
-  for (const log of logs) {
+  for (const entry of logs) {
     if (!first) {
       stdout.write("\n---\n\n");
     }
-    stdout.write(`${formatLogDetails(log, orgSlug)}\n`);
+    stdout.write(`${formatLogDetails(entry, orgSlug)}\n`);
     first = false;
   }
 
@@ -184,40 +260,7 @@ export const viewCommand = buildCommand({
     const { logIds, targetArg } = parsePositionalArgs(args);
     const parsed = parseOrgProjectArg(targetArg);
 
-    let target: ResolvedLogTarget | null = null;
-
-    switch (parsed.type) {
-      case "explicit":
-        target = {
-          org: parsed.org,
-          project: parsed.project,
-        };
-        break;
-
-      case "project-search":
-        target = await resolveProjectBySlug(
-          parsed.projectSlug,
-          USAGE_HINT,
-          `sentry log view <org>/${parsed.projectSlug} ${logIds[0]}`,
-          this.stderr
-        );
-        break;
-
-      case "org-all":
-        throw new ContextError("Specific project", USAGE_HINT);
-
-      case "auto-detect":
-        target = await resolveOrgAndProject({ cwd, usageHint: USAGE_HINT });
-        break;
-
-      default: {
-        // Exhaustive check - should never reach here
-        const _exhaustiveCheck: never = parsed;
-        throw new ValidationError(
-          `Invalid target specification: ${_exhaustiveCheck}`
-        );
-      }
-    }
+    const target = await resolveTarget(parsed, logIds, cwd, this.stderr);
 
     if (!target) {
       throw new ContextError("Organization and project", USAGE_HINT);
@@ -227,36 +270,30 @@ export const viewCommand = buildCommand({
     setContext([target.org], [target.project]);
 
     if (flags.web) {
-      // --web only opens the first log (browser can only open one meaningfully)
-      await openInBrowser(stdout, buildLogsUrl(target.org, logIds[0]), "log");
+      if (logIds.length > 1) {
+        log.warn(`Opening ${logIds.length} browser tabs…`);
+      }
+      for (const id of logIds) {
+        await openInBrowser(stdout, buildLogsUrl(target.org, id), "log");
+      }
       return;
     }
 
     // Fetch all requested log entries
     const logs = await getLogs(target.org, target.project, logIds);
 
     if (logs.length === 0) {
-      const idDisplay =
-        logIds.length === 1
-          ? `ID "${logIds[0]}"`
-          : `IDs ${logIds.map((id) => `"${id}"`).join(", ")}`;
+      const idList = formatIdList(logIds);
       throw new ValidationError(
-        `No logs found with ${idDisplay} in ${target.org}/${target.project}.\n\n` +
-          "Make sure the log IDs are correct and the logs were sent within the last 90 days."
+        logIds.length === 1
+          ? `No log found with ID "${logIds[0]}" in ${target.org}/${target.project}.\n\n` +
+              "Make sure the log ID is correct and the log was sent within the last 90 days."
+          : `No logs found with any of the following IDs in ${target.org}/${target.project}:\n${idList}\n\n` +
+              "Make sure the log IDs are correct and the logs were sent within the last 90 days."
       );
     }
 
-    // Warn about any IDs that weren't found
-    if (logs.length < logIds.length) {
-      const foundIds = new Set(logs.map((l) => l["sentry.item_id"]));
-      const missing = logIds.filter((id) => !foundIds.has(id));
-      if (missing.length > 0) {
-        const stderr = this.stderr;
-        stderr.write(
-          `Warning: ${missing.length} of ${logIds.length} log(s) not found: ${missing.join(", ")}\n`
-        );
-      }
-    }
+    warnMissingIds(logIds, logs);
 
     if (flags.json) {
       // Single ID: output single object for backward compatibility

src/commands/log/view.ts

@@ -1780,7 +1780,7 @@ const DETAILED_LOG_FIELDS = [
 /**
  * Get one or more log entries by their item IDs.
  * Uses the Explore/Events API with dataset=logs and a filter query.
- * For multiple IDs, uses bracket syntax: `sentry.item_id:[id1,id2,...]`.
+ * Bracket syntax (`sentry.item_id:[id1,id2,...]`) works for any count including one.
  *
  * @param orgSlug - Organization slug
  * @param projectSlug - Project slug for filtering
@@ -1792,11 +1792,7 @@ export async function getLogs(
   projectSlug: string,
   logIds: string[]
 ): Promise<DetailedSentryLog[]> {
-  const idFilter =
-    logIds.length === 1
-      ? `sentry.item_id:${logIds[0]}`
-      : `sentry.item_id:[${logIds.join(",")}]`;
-  const query = `project:${projectSlug} ${idFilter}`;
+  const query = `project:${projectSlug} sentry.item_id:[${logIds.join(",")}]`;
   const config = await getOrgSdkConfig(orgSlug);
 
   const result = await queryExploreEventsInTableFormat({
@@ -1806,6 +1802,7 @@ export async function getLogs(
       dataset: "logs",
       field: DETAILED_LOG_FIELDS,
       query,
+      // per_page must match the number of IDs so the API returns all matches
       per_page: logIds.length,
       statsPeriod: "90d",
     },
@@ -1816,24 +1813,6 @@ export async function getLogs(
   return logsResponse.data;
 }
 
-/**
- * Get a single log entry by its item ID.
- * Convenience wrapper around {@link getLogs} for single-ID lookups.
- *
- * @param orgSlug - Organization slug
- * @param projectSlug - Project slug for filtering
- * @param logId - The sentry.item_id of the log entry
- * @returns The detailed log entry, or null if not found
- */
-export async function getLog(
-  orgSlug: string,
-  projectSlug: string,
-  logId: string
-): Promise<DetailedSentryLog | null> {
-  const logs = await getLogs(orgSlug, projectSlug, [logId]);
-  return logs[0] ?? null;
-}
-
 // Trace-log functions
 
 type ListTraceLogsOptions = {

src/lib/api-client.ts

@@ -15,18 +15,21 @@ const MAX_DISPLAY_LENGTH = 40;
 
 /**
  * Validate that a string is a 32-character hexadecimal ID.
- * Trims whitespace before validation.
+ * Trims whitespace and normalizes to lowercase before validation.
  *
- * Returns the trimmed, validated ID so it can be used as a Stricli `parse`
- * function directly.
+ * Normalization to lowercase ensures consistent comparison with API responses,
+ * which return lowercase hex IDs regardless of input casing.
+ *
+ * Returns the trimmed, lowercased, validated ID so it can be used as a Stricli
+ * `parse` function directly.
  *
  * @param value - The string to validate
  * @param label - Human-readable name for error messages (e.g., "log ID", "trace ID")
- * @returns The trimmed, validated ID
+ * @returns The trimmed, lowercased, validated ID
  * @throws {ValidationError} If the format is invalid
  */
 export function validateHexId(value: string, label: string): string {
-  const trimmed = value.trim();
+  const trimmed = value.trim().toLowerCase();
 
   if (!HEX_ID_RE.test(trimmed)) {
     const display =

src/lib/hex-id.ts

@@ -54,18 +54,16 @@ function makeSampleLog(id: string, message = "Test log"): DetailedSentryLog {
 
 function createMockContext() {
   const stdoutWrite = mock(() => true);
-  const stderrWrite = mock(() => true);
   return {
     context: {
       stdout: { write: stdoutWrite },
-      stderr: { write: stderrWrite },
+      stderr: { write: mock(() => true) },
       cwd: "/tmp",
       setContext: mock(() => {
         // no-op for test
       }),
     },
     stdoutWrite,
-    stderrWrite,
   };
 }
 
@@ -134,7 +132,7 @@ describe("viewCommand.func", () => {
       } catch (error) {
         expect(error).toBeInstanceOf(ValidationError);
         expect((error as ValidationError).message).toContain(ID1);
-        expect((error as ValidationError).message).toContain("No logs found");
+        expect((error as ValidationError).message).toContain("No log found");
       }
     });
   });
@@ -204,11 +202,11 @@ describe("viewCommand.func", () => {
       expect(parsed).toHaveLength(2);
     });
 
-    test("warns to stderr when some IDs are not found", async () => {
-      // Only ID1 found, ID2 and ID3 missing
+    test("still outputs found logs when some IDs are missing", async () => {
+      // Only ID1 found, ID2 and ID3 missing — warning goes through consola
       getLogsSpy.mockResolvedValue([makeSampleLog(ID1)]);
 
-      const { context, stderrWrite, stdoutWrite } = createMockContext();
+      const { context, stdoutWrite } = createMockContext();
       const func = await viewCommand.loader();
       await func.call(
         context,
@@ -219,17 +217,13 @@ describe("viewCommand.func", () => {
         ID3
       );
 
-      const stderrOutput = stderrWrite.mock.calls.map((c) => c[0]).join("");
-      expect(stderrOutput).toContain("Warning");
-      expect(stderrOutput).toContain("2 of 3");
-      expect(stderrOutput).toContain(ID2);
-      expect(stderrOutput).toContain(ID3);
-
-      // Should still output the found log
+      // Should still output the found log as JSON
       const stdoutOutput = stdoutWrite.mock.calls.map((c) => c[0]).join("");
       const parsed = JSON.parse(stdoutOutput);
       // Multiple IDs requested → array output even if only one found
       expect(Array.isArray(parsed)).toBe(true);
+      expect(parsed).toHaveLength(1);
+      expect(parsed[0]["sentry.item_id"]).toBe(ID1);
     });
 
     test("throws ValidationError when no logs found for multiple IDs", async () => {
@@ -251,8 +245,9 @@ describe("viewCommand.func", () => {
         expect(error).toBeInstanceOf(ValidationError);
         const msg = (error as ValidationError).message;
         expect(msg).toContain("No logs found");
-        expect(msg).toContain(ID1);
-        expect(msg).toContain(ID2);
+        // Each ID should appear in a markdown list item
+        expect(msg).toContain(` - \`${ID1}\``);
+        expect(msg).toContain(` - \`${ID2}\``);
       }
     });
   });
@@ -272,7 +267,7 @@ describe("viewCommand.func", () => {
       expect(getLogsSpy).not.toHaveBeenCalled();
     });
 
-    test("opens browser for first log when multiple IDs given", async () => {
+    test("opens browser for all IDs when multiple given", async () => {
       openInBrowserSpy.mockResolvedValue(undefined);
 
       const { context } = createMockContext();
@@ -285,9 +280,11 @@ describe("viewCommand.func", () => {
         ID2
       );
 
-      expect(openInBrowserSpy).toHaveBeenCalled();
-      const url = openInBrowserSpy.mock.calls[0][1] as string;
-      expect(url).toContain(ID1);
+      expect(openInBrowserSpy).toHaveBeenCalledTimes(2);
+      const url1 = openInBrowserSpy.mock.calls[0][1] as string;
+      const url2 = openInBrowserSpy.mock.calls[1][1] as string;
+      expect(url1).toContain(ID1);
+      expect(url2).toContain(ID2);
     });
   });