Skip to content

tmp.md

Latest commit

 

History

History
277 lines (180 loc) · 10.9 KB

tmp.md

File metadata and controls

277 lines (180 loc) · 10.9 KB

PR #16 Approve

Thanks for the quick turnaround! The new commits address all the concerns:

  • posix.join reverted — production code stays on native join, tests use platform-aware assertions with path.join(homedir(), ...). Clean.
  • Backslash normalization scoped to pathsEqual only — minimal blast radius, no consistency issues with contractPath/expandPath. This was exactly the alternative I suggested.
  • Misleading "Normalizes path separators" removed from normalizePath JSDoc — code and docs now match.
  • Test portability fixes (tmpdir(), basename(), join()) all look correct.

LGTM — approving.

PR #16 Response

posix.join

Makes sense — thanks for confirming. The platform-aware assertion approach should be a clean fix for that.

normalizePath doc comment said that it normalized the path separators, but that wasn't done in the function

Good catch on the comment/code mismatch. You're right that the doc says "Normalizes path separators" but the function didn't actually do it.

Is there a reason for expandPath to be a separate function? Should expandPath calls just be replaced with normalizePath?

They serve different purposes:

  • expandPath — produces a real OS path you'd pass to fs.readFile(), path.join(), etc. It should return native separators so the OS can use it directly.
  • normalizePath — produces a canonical form for comparison. This is where separator normalization belongs, because you're comparing two paths, not opening a file.

If expandPath normalized backslashes to /, you'd get paths like C:/Users/dan/foo passed to filesystem APIs on Windows. That usually works in Node.js, but it's not the native convention and could cause subtle issues with third-party libraries or string matching against homedir().

So I'd keep them separate — expandPath stays native, normalizePath adds the comparison-friendly normalization on top. The backslash-to-forward-slash conversion is the right addition to normalizePath, just needs contractPath updated to match (per my earlier suggestion) so the path functions stay internally consistent.

PR #21 Review — Request Changes

Thanks for the contribution! The core feature logic is clean and well-tested. There are a few things that need to be addressed before merging.

1. Rebase onto current main — translation files must not be deleted

This PR deletes docs/readme_es.md, docs/readme_fr.md, docs/readme_zh.md and removes the language selector line from README.md. These were added in ff81348 on main after your fork point, so the diff reverts them unintentionally.

Fix: Rebase your branch onto current main:

git fetch upstream main
git rebase upstream/main

Then resolve any conflicts, keeping the translation files and README language selector intact. The only files that should change are the ones related to composer ID support.

2. Null-session path in export.ts — add return after handleError

In src/cli/commands/export.ts around line 160, after the handleError call for a null session, execution technically continues to findWorkspaces. While handleError is typed as never, adding an explicit return makes the control flow obvious and protects against future refactors:

if (!session) {
  const msg =
    indexArg === String(index)
      ? `Session ${index} could not be loaded.`
      : `Session ${indexArg} (index ${index}) could not be loaded.`;
  handleError(new Error(msg));
  return; // unreachable, but clarifies intent
}

Apply the same pattern in src/cli/commands/show.ts at the equivalent null-session check.

3. CLI SessionNotFoundError — clean up unused constructor change

The CLI's SessionNotFoundError in src/cli/errors.ts now accepts identifier: number | string, maxIndex: number, but:

  • For string identifiers, maxIndex is completely ignored.
  • The PR's own code never constructs this CLI error directly anymore — it relies on the lib SessionNotFoundError caught via isSessionNotFoundError() in handleError.

Since this class is now only used as a catch target (not constructed) in the new code paths, either:

(a) Keep it unchanged — revert the constructor signature back to (index: number, maxIndex: number) since no new call sites use the string overload, OR

(b) Remove maxIndex for the string case — make the signature cleaner:

constructor(identifier: number | string, maxIndex?: number) {
  const message =
    typeof identifier === 'number'
      ? maxIndex && maxIndex > 0
        ? `Session #${identifier} not found. Valid range: 1-${maxIndex}`
        : 'No sessions found.'
      : `Session '${identifier}' not found. Use 'list --ids' to see available composer IDs.`;
  super(message, ExitCode.NOT_FOUND);
  this.name = 'SessionNotFoundError';
}

Option (a) is simpler if no one else needs the string variant from the CLI error class.

4. Version bump to 0.12.0

This adds new user-facing functionality (show/export accept composer IDs), which is a minor semver bump per the project's convention. Please update package.json:

"version": "0.12.0"

5. Run formatting and checks before committing

Please make sure the following all pass cleanly before pushing:

npm run format      # Prettier
npm run lint        # ESLint
npm run typecheck   # TypeScript type checker
npm test            # Unit tests

Summary

# Issue Severity
1 Rebase — drop unrelated translation deletions Blocking
2 Add return after handleError in export.ts and show.ts Minor
3 Clean up CLI SessionNotFoundError constructor Minor
4 Version bump to 0.12.0 Required
5 Run Prettier, linter, and typecheck before commit Required

The feature itself — resolveSessionIndex, the show/export integration, test coverage — all looks good. Looking forward to the updated PR!

Tip: If you're using AI coding tools (Cursor, Claude Code, Copilot, etc.), you can pass this review message directly to it — it should be able to apply all the requested changes.

PR (issue #19) Review — Request Changes: code-workspace support

Thanks for tackling this! The approach is sound — supporting workspace key in workspace.json, deduplicating sessions, and deterministic sort order. Tests are thorough. A few things to fix before merge.

1. Rebase onto current main — translation files must not be deleted

Same issue as PR #21. This PR deletes docs/readme_es.md, docs/readme_fr.md, docs/readme_zh.md and removes the language selector line from README.md. These were added in ff81348 on main after your fork point.

Fix: Rebase onto current main:

git fetch upstream main
git rebase upstream/main

Keep the translation files and README language selector intact.

2. Comment/code mismatch in getWorkspacePathFromJson

The JSDoc comment says:

Prefers folder; falls back to configuration for .code-workspace workspaces.

But the code checks data.workspace first — meaning it prefers workspace over folder. The test 'prefers workspace when both folder and workspace exist' confirms the code behavior is intentional. The CLAUDE.md update also says "prefer workspace when both exist".

Fix: Update the comment to match the code:

/**
 * Read workspace path from parsed workspace.json (folder or configuration).
 * Prefers workspace (.code-workspace path); falls back to folder for single-folder workspaces.
 */

3. workspaceUriToPath only handles %20 — other percent-encoded characters are ignored

The function only decodes %20 to space:

function workspaceUriToPath(uri: string): string {
  return uri.replace(/^file:\/\//, '').replace(/%20/g, ' ');
}

Paths with other special characters (e.g. %23 for #, %28/%29 for parens) won't be decoded. Consider using decodeURIComponent instead:

function workspaceUriToPath(uri: string): string {
  try {
    return decodeURIComponent(uri.replace(/^file:\/\//, ''));
  } catch {
    return uri.replace(/^file:\/\//, '');
  }
}

Note: the original code in readWorkspaceJson also only handled %20, so this is a pre-existing issue. If you'd rather keep the scope minimal, just add a code comment noting the limitation and we can address it separately. But since you're already refactoring this into a shared helper, now is a good time to fix it.

4. Deduplication only applies when no --workspace filter — document this clearly

The dedup logic uses seenIds only when options.workspacePath is not set:

const seenIds = options.workspacePath ? null : new Set<string>();

This means cursor-history list --workspace /folder and cursor-history list --workspace /path/to/ws.code-workspace could each show the same session. This is probably the right behavior (the user explicitly asked for that workspace's sessions), but it should be documented in the help text or noted in the CLAUDE.md so future contributors understand the design choice.

5. searchSessions test change — unrelated fix

The diff changes an existing test:

-    const result = await searchSessions('xyz', { limit: 10 }, '/data');
+    const result = await searchSessions('xyz', { limit: 10, contextChars: 50 }, '/data');

This adds contextChars: 50 to an existing test. If this fixes a pre-existing test issue, it should be in a separate commit or at least noted in the PR description. If it's not needed, revert it to keep the diff focused.

6. Version bump

This is a bug fix (sessions not discovered for code-workspace workspaces), so a patch bump is appropriate. Please update package.json:

"version": "0.12.1"

(Or 0.12.0 if this lands before PR #21. Coordinate with the other PR.)

7. Run formatting and checks before committing

Please make sure the following all pass cleanly before pushing:

npm run format      # Prettier
npm run lint        # ESLint
npm run typecheck   # TypeScript type checker
npm test            # Unit tests

Summary

# Issue Severity
1 Rebase — drop unrelated translation deletions Blocking
2 Comment/code mismatch in getWorkspacePathFromJson Minor
3 workspaceUriToPath only decodes %20 Minor (or separate issue)
4 Document dedup-only-without-filter design choice Minor
5 Unrelated searchSessions test change Minor
6 Version bump Required
7 Run Prettier, linter, and typecheck before commit Required

The core fix — workspace key support, dedup logic, deterministic sort, backup path support — is solid. Nice test coverage on the edge cases. Looking forward to the updated PR!

Tip: If you're using AI coding tools (Cursor, Claude Code, Copilot, etc.), you can pass this review message directly to it — it should be able to apply all the requested changes.