feat: add export#2
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements a complete Changesifj CLI export and inspect tool
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
package.json (1)
19-20: 💤 Low valueBeta dependencies in production package.
Both
effectand@effect/platform-nodeare beta versions (4.0.0-beta.78). Beta releases may have breaking changes, stability issues, or incomplete features. Consider whether this is appropriate for production use or if the package should remain marked as"private": trueuntil dependencies stabilize.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` around lines 19 - 20, package.json currently pins beta releases for "effect" and "`@effect/platform-node`" (4.0.0-beta.78); replace or mitigate these by either upgrading to a stable release, pinning to a known-good non-beta version, or revert to a stable dependency set and mark the package "private": true until you intentionally adopt the beta. Locate the dependencies named "effect" and "`@effect/platform-node`" and change their versions to a stable tag (or remove them if unused), or set "private": true in package.json to prevent accidental production publishing if you must keep the betas for development.README.md (1)
27-27: 💤 Low valueGrammar nitpick in flag description.
LanguageTool flagged "required output path" as nonstandard phrasing. While understandable, consider "output path (required)" or "path for output file (required)" for better readability.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` at line 27, Update the flag description for `--out PATH` to use clearer phrasing: replace "required output path. Must end with `.jsonl.gz`." with something like "output path (required). Must end with `.jsonl.gz`" or "path for output file (required). Must end with `.jsonl.gz`" so the requirement reads naturally while preserving the `.jsonl.gz` constraint and the `--out PATH` flag name.src/jira.ts (1)
170-178: 🏗️ Heavy liftDefault-scope discovery is fully serialized.
When
--spaceis omitted, this does one project-search call and then awaits each property lookup one by one. On larger Jira tenants that makes startup latency grow linearly with project count and increases rate-limit exposure. A small bounded-concurrency pool forgetSpaceConfiguration()would make the common discovery path much more usable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jira.ts` around lines 170 - 178, The current default-scope discovery serially awaits getSpaceConfiguration for each space returned by listSpaces, causing linear startup latency; change the loop in the method that builds configured (using listSpaces, configured, JiraSpace) to run getSpaceConfiguration(space.key).pipe(Effect.runPromise) in parallel with a small bounded concurrency (e.g., 4–10 workers) rather than one-by-one, collecting results and pushing the corresponding space when configuration !== undefined, then sort and return as before; ensure you preserve the Effect.runPromise call per task and handle/propagate errors consistently (fail fast or collect errors) to match existing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/export.ts`:
- Around line 221-225: The loop calling
jiraClient.searchWorkItemConversationLinks should not let
normalizeConversationLinksProperty throw and abort the export; wrap the call to
normalizeConversationLinksProperty(hit.propertyValue) in a try/catch, and on
catch add a warning (e.g. "MALFORMED_LINK_PROPERTY") with context including
spaceKey, workItemKey: hit.key and the error message, then continue the loop
instead of rethrowing; keep the existing skip for normalized === undefined or
empty arrays unchanged so only malformed shapes cause a warning and skip.
In `@src/jira.ts`:
- Around line 303-345: The requestJson retry loop currently calls fetch()
outside any per-attempt try/catch and has no timeout; wrap each fetch attempt
inside a try/catch and use an AbortController with a per-attempt timeout (clear
the timeout on success) so hung requests are aborted and can be retried; on
catch, build the same context (method, sanitized path, headers from the last
response if available or empty) and if attempt < maxAttempts treat the error as
retryable (use the same backoff via retryAfterMs(...) ?? 250 * 2 ** (attempt -
1) and await sleep(...)) and continue, otherwise throw an AppError (use
"jira.transient" for retryable-network-final-failure or "jira.request" as
appropriate) with the context; ensure helpers referenced (requestJson,
isRetryableStatus, retryAfterMs, sleep, sanitizePath, AppError) are used
consistently.
---
Nitpick comments:
In `@package.json`:
- Around line 19-20: package.json currently pins beta releases for "effect" and
"`@effect/platform-node`" (4.0.0-beta.78); replace or mitigate these by either
upgrading to a stable release, pinning to a known-good non-beta version, or
revert to a stable dependency set and mark the package "private": true until you
intentionally adopt the beta. Locate the dependencies named "effect" and
"`@effect/platform-node`" and change their versions to a stable tag (or remove
them if unused), or set "private": true in package.json to prevent accidental
production publishing if you must keep the betas for development.
In `@README.md`:
- Line 27: Update the flag description for `--out PATH` to use clearer phrasing:
replace "required output path. Must end with `.jsonl.gz`." with something like
"output path (required). Must end with `.jsonl.gz`" or "path for output file
(required). Must end with `.jsonl.gz`" so the requirement reads naturally while
preserving the `.jsonl.gz` constraint and the `--out PATH` flag name.
In `@src/jira.ts`:
- Around line 170-178: The current default-scope discovery serially awaits
getSpaceConfiguration for each space returned by listSpaces, causing linear
startup latency; change the loop in the method that builds configured (using
listSpaces, configured, JiraSpace) to run
getSpaceConfiguration(space.key).pipe(Effect.runPromise) in parallel with a
small bounded concurrency (e.g., 4–10 workers) rather than one-by-one,
collecting results and pushing the corresponding space when configuration !==
undefined, then sort and return as before; ensure you preserve the
Effect.runPromise call per task and handle/propagate errors consistently (fail
fast or collect errors) to match existing behavior.
🪄 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: f0b19728-f50a-43a1-bd20-1257a7c4855d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
.gitignore.prettierignoreREADME.mddocs/artifact.mdeslint.config.jspackage.jsonsrc/artifact.tssrc/cli.tssrc/config.tssrc/errors.tssrc/export.tssrc/ifj.tssrc/index.tssrc/jira.tssrc/links.tssrc/warnings.tstest/index.test.tstsconfig.jsonvitest.config.ts
tbinna
force-pushed
the
add-export
branch
2 times, most recently
from
338c7e9 to
5ec2115
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/jira.ts (1)
245-305:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd request timeout and network failure retry.
The
requestJsonfunction retries on HTTP 5xx/429 status codes but:
- Has no request timeout—hung connections will block indefinitely
- Does not retry on network failures (the
Effect.mapErrorat lines 263-269 fails immediately)Consider adding
Effect.timeoutaround each request attempt and treating network errors as retryable for transient failures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jira.ts` around lines 245 - 305, The requestJson implementation currently can hang and doesn't retry on network errors; wrap the httpClient.get call in an Effect.timeout using a configured request timeout (e.g., requestTimeoutMillis) so each attempt fails after the timeout, and change the Effect.mapError handler around httpClient.get to classify network/transport errors as transient (create an AppError with a retryable marker) so that when the mapped error occurs you run the same retry logic as for isRetryableStatus (use attempt < maxAttempts, Clock.currentTimeMillis, retryDelay and recurse requestJson(path, options, attempt+1)); keep existing handling for 401/403 and non-retryable errors, and ensure rateLimitHeaders/context are preserved when creating failure AppError.
🧹 Nitpick comments (2)
src/artifact.ts (1)
384-402: 💤 Low valueMutation of shared
Setin fold accumulator.
accumulateInspectStatemutatesstate.spaceKeys(aSet) directly while spreading other properties. This works correctly in the sequentialrunFoldcontext but mixes mutable and immutable patterns.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/artifact.ts` around lines 384 - 402, accumulateInspectState currently mutates the shared Set stored in state.spaceKeys when handling "spaceConfiguration" and "workItemConversationLinks"; instead create a new Set copy and add record.spaceKey to it, then return the new InspectState with that new Set (update spaceKeys in the returned object rather than calling state.spaceKeys.add), preserving immutability in the accumulator (functions: accumulateInspectState, InspectState, state.spaceKeys, record.spaceKey).src/cli.ts (1)
61-67: 💤 Low valueSimplify conditional dependency spreading.
The conditional spreading pattern is verbose. Consider using object filtering or a helper.
♻️ Optional simplification
- Effect.flatMap((config) => - runExport(config, { - ...(dependencies.jiraClient === undefined - ? {} - : { jiraClient: dependencies.jiraClient }), - ...(dependencies.now === undefined ? {} : { now: dependencies.now }), - ...(dependencies.stderr === undefined ? {} : { stderr: dependencies.stderr }), - }), - ), + Effect.flatMap((config) => { + const { jiraClient, now, stderr } = dependencies; + return runExport(config, { + ...(jiraClient && { jiraClient }), + ...(now && { now }), + ...(stderr && { stderr }), + }); + }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli.ts` around lines 61 - 67, The conditional spreading around runExport is verbose; simplify by building a cleaned dependencies object before calling runExport (e.g., take the dependencies object and remove keys with undefined values) and pass that object as the second argument to runExport; specifically update the call site referencing runExport and dependencies so you only include jiraClient, now, stderr if they are defined by creating a filtered/compact object (using Object.entries/Object.fromEntries or a small helper) and pass that instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Line 27: Update the README.md description for the `--out PATH` flag to use
clearer wording: replace "required output path. Must end with `.jsonl.gz`." with
something like "required output file path. Must end with `.jsonl.gz`." (or
"required path for output") so the flag description reads naturally for users.
---
Duplicate comments:
In `@src/jira.ts`:
- Around line 245-305: The requestJson implementation currently can hang and
doesn't retry on network errors; wrap the httpClient.get call in an
Effect.timeout using a configured request timeout (e.g., requestTimeoutMillis)
so each attempt fails after the timeout, and change the Effect.mapError handler
around httpClient.get to classify network/transport errors as transient (create
an AppError with a retryable marker) so that when the mapped error occurs you
run the same retry logic as for isRetryableStatus (use attempt < maxAttempts,
Clock.currentTimeMillis, retryDelay and recurse requestJson(path, options,
attempt+1)); keep existing handling for 401/403 and non-retryable errors, and
ensure rateLimitHeaders/context are preserved when creating failure AppError.
---
Nitpick comments:
In `@src/artifact.ts`:
- Around line 384-402: accumulateInspectState currently mutates the shared Set
stored in state.spaceKeys when handling "spaceConfiguration" and
"workItemConversationLinks"; instead create a new Set copy and add
record.spaceKey to it, then return the new InspectState with that new Set
(update spaceKeys in the returned object rather than calling
state.spaceKeys.add), preserving immutability in the accumulator (functions:
accumulateInspectState, InspectState, state.spaceKeys, record.spaceKey).
In `@src/cli.ts`:
- Around line 61-67: The conditional spreading around runExport is verbose;
simplify by building a cleaned dependencies object before calling runExport
(e.g., take the dependencies object and remove keys with undefined values) and
pass that object as the second argument to runExport; specifically update the
call site referencing runExport and dependencies so you only include jiraClient,
now, stderr if they are defined by creating a filtered/compact object (using
Object.entries/Object.fromEntries or a small helper) and pass that instead.
🪄 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: 71ba49e5-60b4-4c56-9b44-a07862917678
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
.gitignore.prettierignoreREADME.mddocs/artifact.mdeslint.config.jspackage.jsonsrc/artifact.tssrc/cli.tssrc/clock.tssrc/config.tssrc/errors.tssrc/export.tssrc/ifj.tssrc/index.tssrc/jira.tssrc/links.tssrc/output.tssrc/warnings.tstest/index.test.tstsconfig.jsonvitest.config.ts
✅ Files skipped from review due to trivial changes (4)
- docs/artifact.md
- vitest.config.ts
- .gitignore
- .prettierignore
🚧 Files skipped from review as they are similar to previous changes (9)
- eslint.config.js
- src/ifj.ts
- src/index.ts
- tsconfig.json
- src/errors.ts
- package.json
- src/links.ts
- src/export.ts
- test/index.test.ts
tbinna
force-pushed
the
add-export
branch
2 times, most recently
from
70f3671 to
e1d67e2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jira.ts`:
- Around line 285-292: The paginated loop in searchProjectSpaces mutates the
incoming params via params.append(), causing duplicate query keys across
iterations; fix it by cloning the params at the start of each paginate callback
and using set (or a fresh URLSearchParams) for the pagination keys so they don't
accumulate. Concretely, inside Stream.paginate(...) in searchProjectSpaces
create e.g. const iterParams = new URLSearchParams(params.toString()) and then
call iterParams.set("startAt", String(startAt)), iterParams.set("maxResults",
String(pageSize)), iterParams.set("properties", spaceConfigurationPropertyKey")
before constructing path and context, instead of mutating the original params
with append.
🪄 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: c53a6dae-f294-488f-bef3-17de23df89cf
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
.gitignore.prettierignoreREADME.mddocs/artifact.mdeslint.config.jspackage.jsonsrc/artifact.tssrc/cli.tssrc/clock.tssrc/config.tssrc/errors.tssrc/export.tssrc/ifj.tssrc/index.tssrc/jira.tssrc/links.tssrc/output.tssrc/warnings.tstest/index.test.tstsconfig.jsonvitest.config.ts
✅ Files skipped from review due to trivial changes (4)
- eslint.config.js
- .prettierignore
- .gitignore
- docs/artifact.md
🚧 Files skipped from review as they are similar to previous changes (12)
- src/clock.ts
- vitest.config.ts
- tsconfig.json
- README.md
- package.json
- src/ifj.ts
- src/index.ts
- src/output.ts
- src/warnings.ts
- src/export.ts
- src/errors.ts
- src/links.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/jira.ts (1)
285-291:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop mutating the shared query params across pages.
Each
append()keeps the previous value, so later requests can send duplicatestartAt,maxResults, andpropertieskeys. That can break both default-space discovery and explicit-space validation.Suggested fix
const searchProjectSpaces = (params: URLSearchParams): Stream.Stream<JiraSpace, AppError> => Stream.paginate(0, (startAt) => { - params.append("startAt", String(startAt)); - params.append("maxResults", String(pageSize)); - params.append("properties", spaceConfigurationPropertyKey); - - const path = `/rest/api/3/project/search?${params.toString()}`; + const pageParams = new URLSearchParams(params); + pageParams.set("startAt", String(startAt)); + pageParams.set("maxResults", String(pageSize)); + pageParams.set("properties", spaceConfigurationPropertyKey); + + const path = `/rest/api/3/project/search?${pageParams.toString()}`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jira.ts` around lines 285 - 291, The bug is that searchProjectSpaces is mutating the shared URLSearchParams 'params' across pages by repeatedly calling params.append(...) inside Stream.paginate, causing duplicate query keys; fix it by creating a fresh URLSearchParams copy per page (e.g., new URLSearchParams(params) or building a new params instance) before appending startAt, maxResults, and properties so each request has exactly one of each key, and update the construction of 'path' inside Stream.paginate to use that per-page params copy (refer to searchProjectSpaces, Stream.paginate, and the startAt/maxResults/properties append calls).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/export.ts`:
- Around line 223-229: The current catch on jiraClient.discoverConfiguredSpaces
swallows real errors by returning [] which later triggers
export.emptyDefaultScope; instead log the warning but rethrow the original error
so transient Jira failures surface (do not return an empty array). Update the
Effect.catch wrapping jiraClient.discoverConfiguredSpaces (the pipeline that
maps with sortSpaces and calls warn(..., "DEFAULT_SCOPE_DISCOVERY_FAILED", {
reason: warningReason(error) })) to call warn/tap for the warning and then
fail/throw the same error (preserve the original error) rather than piping
Effect.as([]), so callers receive the real failure.
In `@src/jira.ts`:
- Around line 509-513: The pagination logic currently treats a missing
response.isLast as end-of-stream; update the ternary so it only stops when
response.isLast === true or response.nextPageToken is undefined. Concretely, in
the block returning [hits, ...] use a condition like (response.isLast === true
|| response.nextPageToken === undefined) ? Option.none() :
Option.some(response.nextPageToken) so that a missing isLast doesn't prevent
continuing when nextPageToken exists; refer to the variables response, isLast,
and nextPageToken in this change.
---
Duplicate comments:
In `@src/jira.ts`:
- Around line 285-291: The bug is that searchProjectSpaces is mutating the
shared URLSearchParams 'params' across pages by repeatedly calling
params.append(...) inside Stream.paginate, causing duplicate query keys; fix it
by creating a fresh URLSearchParams copy per page (e.g., new
URLSearchParams(params) or building a new params instance) before appending
startAt, maxResults, and properties so each request has exactly one of each key,
and update the construction of 'path' inside Stream.paginate to use that
per-page params copy (refer to searchProjectSpaces, Stream.paginate, and the
startAt/maxResults/properties append calls).
🪄 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: 749cf837-57c3-4518-bcca-a500a13e998c
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
.gitignore.prettierignoreREADME.mddocs/artifact.mdeslint.config.jspackage.jsonsrc/artifact.tssrc/cli.tssrc/clock.tssrc/config.tssrc/errors.tssrc/export.tssrc/ifj.tssrc/index.tssrc/jira.tssrc/links.tssrc/output.tssrc/warnings.tstest/index.test.tstsconfig.jsonvitest.config.ts
💤 Files with no reviewable changes (1)
- src/index.ts
✅ Files skipped from review due to trivial changes (6)
- .gitignore
- .prettierignore
- package.json
- eslint.config.js
- README.md
- docs/artifact.md
🚧 Files skipped from review as they are similar to previous changes (5)
- vitest.config.ts
- src/clock.ts
- src/ifj.ts
- src/artifact.ts
- test/index.test.ts
tbinna
force-pushed
the
add-export
branch
2 times, most recently
from
798f5a3 to
7115e2c
Compare
Member
Author
|
@coderabbitai, can you do a full review on this again? |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (9)
.github/workflows/validation.yml (1)
24-24: ⚡ Quick winConsider pinning actions to commit SHA hashes for supply-chain security.
Using mutable tag references (e.g.,
@v4) allows the underlying action code to change without explicit updates to this workflow, which could introduce unexpected behavior or supply-chain attacks if a tag is moved or compromised.Pinning to specific SHA hashes provides immutable references and stronger supply-chain guarantees.
Example SHA-pinned references
- name: Checkout - uses: actions/checkout@v4 + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: persist-credentials: false - name: Setup pnpm - uses: pnpm/action-setup@v4 + uses: pnpm/action-setup@fe02b34f77f8bc703788d5817da081398fad5dd2 # v4.0.0 - name: Setup Node.js - uses: actions/setup-node@v4 + uses: actions/setup-node@39370e3970a6d050c480ffad4ff0ed4d3fdee5af # v4.1.0Note: Verify the specific SHA hashes from the respective action repositories. Comments indicate the tag version for maintainability.
Also applies to: 27-27, 32-32
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/validation.yml at line 24, Replace mutable action tags with SHA-pinned references: change the "uses: actions/checkout@v4" entry (and the other similar "uses" entries noted at lines 27 and 32) to the corresponding commit SHA form (e.g., actions/checkout@<full-commit-sha>) by looking up the exact commit SHA from the action's repository, update the workflow to use that SHA, and add a short comment indicating the original tag (e.g., "// was `@v4`") for maintainability.Source: Linters/SAST tools
src/shared/artifact/artifact.writer.ts (1)
60-89: 💤 Low valueRedundant cleanup in nested error handlers.
Both
writeCompressedArtifact(line 71) andwriteArtifactRecords(line 86) removetempPathon error. SinceremoveArtifactFileusesforce: trueand ignores errors, the double removal is harmless but redundant. Consider removing the cleanup from line 71 to simplify the flow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/artifact/artifact.writer.ts` around lines 60 - 89, The writeCompressedArtifact function contains a nested error handler that calls removeArtifactFile on failure, duplicating cleanup already performed by writeArtifactRecords via removeTempArtifact; remove the Effect.catch(...) cleanup block from writeCompressedArtifact so it simply fails with the original error and lets writeArtifactRecords (which calls removeTempArtifact) perform cleanup; keep writeCompressedArtifact, writeArtifactRecords, tempSiblingPath, removeArtifactFile and removeTempArtifact names as reference when locating and updating the code.src/shared/artifact/artifact.path.ts (1)
91-101: 💤 Low valueConsider adding a depth limit to recursive numbered-path search.
The
findAvailableNumberedOutputPathrecursion has no depth limit. If many numbered files already exist, this could theoretically cause a stack overflow. Adding a maximum suffix (e.g., 1000) and failing with a clear error would prevent runaway recursion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/artifact/artifact.path.ts` around lines 91 - 101, The recursive helper findAvailableNumberedOutputPath can run unbounded; add a max depth (e.g., const MAX_SUFFIX = 1000) and before recursing check if suffix >= MAX_SUFFIX and fail the Effect with a clear AppError (e.g., "Too many numbered artifacts, aborting") instead of recursing; keep the existing behavior otherwise (check candidate via statOptional and recurse with suffix+1) so the function stops when the limit is hit and returns an explicit failure rather than risking stack overflow.src/shared/artifact/artifact.model.ts (2)
7-7: ⚡ Quick winConsider stricter validation for
createdAtfield.The
createdAtfield is typed asSchema.String, which accepts any string. UsingSchema.DateTimeUtcor a pattern-constrained string would catch malformed timestamps during artifact validation and improve data quality.📅 Suggested refinement
export const ManifestRecord = Schema.Struct({ type: Schema.Literal("manifest"), - createdAt: Schema.String, + createdAt: Schema.DateTimeUtc, source: Schema.String, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/artifact/artifact.model.ts` at line 7, The createdAt field currently uses Schema.String which permits any string; change its schema to a stricter datetime validator by replacing createdAt: Schema.String with either createdAt: Schema.DateTimeUtc (preferred) or createdAt: Schema.String.withPattern(/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(\.\d+)?Z$/) in the artifact model (symbol: createdAt in artifact.model.ts), then update any code that constructs artifacts to emit ISO-8601 UTC timestamps and run validation/tests to ensure no callers supply malformed values.
8-8: ⚡ Quick winConsider validating the
sourcefield as a URL.The
sourcefield is currently a plainSchema.Stringwith no format constraint. Validating it as a URL (e.g.,Schema.String.pipe(Schema.url())) would catch malformed source values during artifact inspection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/artifact/artifact.model.ts` at line 8, The artifact model's source field uses a plain Schema.String; change the schema for the source property to enforce URL validation by piping it through Schema.url() (i.e., replace the current source: Schema.String with source: Schema.String.pipe(Schema.url()) in the artifact model definition inside artifact.model.ts) so malformed source values are rejected during validation.src/shared/artifact/artifact.reader.ts (1)
102-117: 💤 Low valueConsider using
RefforrecordNumbercounter.The mutable
let recordNumber = 0(line 102) works correctly in this sequential stream context, but usingRef.make(0)andRef.updateAndGetwould be more consistent with Effect's functional style.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/artifact/artifact.reader.ts` around lines 102 - 117, Replace the mutable counter `let recordNumber = 0` with an Effect-managed Ref so the counter follows the functional style: create a Ref (e.g., `Ref.make(0)`) before piping `artifactLineStream(path)`, and inside the Stream.mapEffect use `Ref.updateAndGet` (or `Ref.modify`/`updateAndGet` equivalent) to atomically increment and obtain the current record number for `parseArtifactLine(line, recordNumber)`, leaving the `artifactLineStream`, `Stream.mapEffect`, `parseArtifactLine`, and `sawManifest` logic unchanged except that you pass the number read from the Ref instead of the mutable variable.src/export/export.service.ts (1)
184-184: 💤 Low valueSimplify the Option handling pattern.
The
?? Option.none<unknown>()pattern suggestsspace.configurationmight beOption<unknown> | undefined. Ifspace.configurationis already anOptiontype, the nullish coalescing is unnecessary. If it's potentiallyundefined, consider makingExportJiraSpace.configurationconsistentlyOption<unknown>to avoid the mixed pattern.♻️ Possible simplification if already an Option
- const configuration = space.configuration ?? Option.none<unknown>(); - if (Option.isNone(configuration)) { + if (Option.isNone(space.configuration)) { return Stream.empty; } - const jsonConfiguration = yield* decodeJson(configuration.value); + const jsonConfiguration = yield* decodeJson(space.configuration.value);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/export/export.service.ts` at line 184, The nullish coalescing in "const configuration = space.configuration ?? Option.none<unknown>()" indicates mixed types; make ExportJiraSpace.configuration consistently an Option<unknown> and remove the unnecessary "?? Option.none<...>()" usage so you can simply use "const configuration = space.configuration" (or, if configuration truly can be undefined, change the ExportJiraSpace type to Option<unknown> and initialize wherever spaces are constructed). Update references to the symbol ExportJiraSpace.configuration and the variable configuration in export.service.ts accordingly to reflect the single Option type.src/cli/cli.service.ts (1)
66-66: ⚖️ Poor tradeoffDocument or reconsider the 1,000 space limit.
The
Flag.between(1, 1_000)constraint caps explicit space selection at 1,000 spaces. While reasonable for most use cases, large Jira instances with extensive space hierarchies might exceed this limit. Consider documenting this limit in user-facing help text or making it configurable if needed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/cli.service.ts` at line 66, The Flag.between(1, 1_000) hard-caps explicit space selection at 1,000; either document this limit in the CLI help text or make it configurable. Update the code around Flag.between to reference a named constant (e.g., MAX_SPACE_SELECTION_LIMIT) or a new CLI option/env var (e.g., --max-spaces / process.env.MAX_SPACES) instead of the literal 1_000, and update the user-facing help/usage string to mention the default limit and how to override it; ensure Flag.between uses that configurable value and add a short note in the help text returned by the CLI so users know the cap and how to change it.src/shared/jira/jira.model.ts (1)
37-42: Review SchemaGetterwithDefault/passthroughusage withSchema.decodeTo(Effect Schema v4)
Effect Schema v4 docs describeSchemaGetter.withDefaultas providing a defaultEffectfor missing values andSchemaGetter.passthrough()as an identity getter intended forSchema.decodeTo, so this usage pattern is aligned. If the goal is simply a default for a missing/undefined key, consider the dedicatedSchema.withDecodingDefault*helpers for consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/jira/jira.model.ts` around lines 37 - 42, The current JiraPropertiesField uses Schema.optionalKey(JiraProperties).pipe(Schema.decodeTo(JiraProperties, { decode: SchemaGetter.withDefault(Effect.succeed({})), encode: SchemaGetter.passthrough() })) — replace this pattern by using the Effect Schema v4 decoding-default helpers for clarity: remove the manual Schema.decodeTo + SchemaGetter.withDefault usage and instead apply the appropriate Schema.withDecodingDefault* helper on JiraProperties (or the optional key) so missing/undefined values get the same {} default while keeping encoding passthrough behavior; update references to JiraPropertiesField, Schema.optionalKey, Schema.decodeTo, SchemaGetter.withDefault, SchemaGetter.passthrough and JiraProperties accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/validation.yml:
- Around line 23-24: Update the Checkout step that uses actions/checkout@v4 to
disable credential persistence by adding persist-credentials: false under the
step (the step with name "Checkout" and uses "actions/checkout@v4"); this
prevents GitHub credentials from being left in the workspace and potentially
leaked via artifacts or dependencies.
In `@README.md`:
- Around line 43-45: Update the README entry for the `--out PATH` CLI flag to
state that `--out` is required (no default) and that the tool enforces overwrite
protection (it will error or prompt if the target file exists), replacing the
current text that claims a default `intercom-for-jira-export.jsonl.gz` in the
CWD; mention the exact flag `--out PATH` and the previous default name
`intercom-for-jira-export.jsonl.gz` so readers understand the change and the
overwrite behavior.
- Around line 3-19: The README incorrectly describes an import phase and shared
token reuse for “import and export” while the shipped CLI only provides export
and inspect; update the top-level description sentence to say ifj exports
Intercom for Jira Cloud data and offers inspection tools (e.g., `export` and
`inspect`) instead of “exports ... and then imports,” remove or rework the
Target Jira site section that describes an import user/token (or mark it as
future/optional), and delete the final sentence about using the same token “for
both import and export” so the docs reflect only the exported functionality and
token requirements for export/inspection.
In `@src/export/export.service.ts`:
- Around line 68-75: The current error assumes the empty result came from
default discovery; update the branch after calling resolveSpaces to distinguish
whether the user passed explicit spaces (check config.spaces and its length) vs
the default-discovery path and throw an appropriate AppError/message for each
case (e.g., keep "export.emptyDefaultScope" and message for no discovered
spaces, and add a new code like "export.emptyExplicitScope" with a message that
validation removed all requested spaces), referencing resolveSpaces,
config.spaces, and the AppError construction so the correct error code and
user-facing message are used depending on how spaces were provided.
- Around line 237-244: The code currently swallows decodeLinkProperty failures
by mapping them to Option.none() inside Effect.matchEffect; instead update the
onFailure branch so that a malformed link property aborts the export by
returning an Effect.fail (or rethrowing the decode failure) after calling
warnMalformedLinkProperty, rather than returning Option.none(). Concretely, in
the decode block that populates maybeLinkProperty (using decodeLinkProperty,
Effect.matchEffect, warnMalformedLinkProperty, warnings, spaceKey, hit.key),
replace the onFailure handler that pipes to Effect.as(Option.none()) with a
handler that first invokes warnMalformedLinkProperty (if you still need the
side-effect) and then returns Effect.fail(failure) so the overall effect fails
and triggers export cleanup instead of skipping the record.
In `@src/shared/app/app.model.ts`:
- Around line 3-9: conversationIdsSchema currently uses Schema.String and thus
allows empty or whitespace-only IDs; update conversationIdsSchema (and ensure
conversationIdsFromArraySchema conversion still works) to trim input and reject
empty/whitespace-only strings by composing a trimmed-string schema and a
non-empty/refinement check (e.g., use Schema.String.pipe(Schema.trim()) then
Schema.refine or Schema.minLength(1) / Schema.nonEmpty depending on your schema
API) so any string consisting only of whitespace fails validation before being
included in the ReadonlySet; keep the transform/encode logic in
conversationIdsFromArraySchema unchanged except it should rely on the new
validated conversationIdsSchema.
In `@src/shared/artifact/artifact.path.ts`:
- Around line 62-104: ensureOutputTarget currently auto-numbers existing files
which contradicts the missing --overwrite behavior; add an overwrite boolean
parameter to ensureOutputTarget(outputPath, overwrite: boolean) and change its
logic so that if outputPath exists as a file it returns outputPath when
overwrite===true, otherwise it fails with an AppError (e.g.,
"export.invalidOutput") instead of recursing to findAvailableNumberedOutputPath;
then plumb the new overwrite flag from the CLI: add a --overwrite boolean option
to exportCommand and pass that flag into ArtifactWriterService.write, and update
ArtifactWriterService.write to forward overwrite into ensureOutputTarget; remove
or deprecate the auto-numbering helper (findAvailableNumberedOutputPath) if you
adopt the overwrite semantics.
In `@src/shared/jira/jira.client.ts`:
- Around line 223-228: The current return uses response.isLast to decide
pagination end; instead rely on response.nextPageToken being present—update the
tuple returned from the pagination function (the return that currently
references response.issues and the Option wrapping logic) to ignore
response.isLast and produce Option.none when response.nextPageToken is
null/undefined, otherwise Option.some(Option.some(response.nextPageToken)); keep
the issues array unchanged and only change the pagination token logic so it
continues whenever nextPageToken is provided.
---
Nitpick comments:
In @.github/workflows/validation.yml:
- Line 24: Replace mutable action tags with SHA-pinned references: change the
"uses: actions/checkout@v4" entry (and the other similar "uses" entries noted at
lines 27 and 32) to the corresponding commit SHA form (e.g.,
actions/checkout@<full-commit-sha>) by looking up the exact commit SHA from the
action's repository, update the workflow to use that SHA, and add a short
comment indicating the original tag (e.g., "// was `@v4`") for maintainability.
In `@src/cli/cli.service.ts`:
- Line 66: The Flag.between(1, 1_000) hard-caps explicit space selection at
1,000; either document this limit in the CLI help text or make it configurable.
Update the code around Flag.between to reference a named constant (e.g.,
MAX_SPACE_SELECTION_LIMIT) or a new CLI option/env var (e.g., --max-spaces /
process.env.MAX_SPACES) instead of the literal 1_000, and update the user-facing
help/usage string to mention the default limit and how to override it; ensure
Flag.between uses that configurable value and add a short note in the help text
returned by the CLI so users know the cap and how to change it.
In `@src/export/export.service.ts`:
- Line 184: The nullish coalescing in "const configuration = space.configuration
?? Option.none<unknown>()" indicates mixed types; make
ExportJiraSpace.configuration consistently an Option<unknown> and remove the
unnecessary "?? Option.none<...>()" usage so you can simply use "const
configuration = space.configuration" (or, if configuration truly can be
undefined, change the ExportJiraSpace type to Option<unknown> and initialize
wherever spaces are constructed). Update references to the symbol
ExportJiraSpace.configuration and the variable configuration in
export.service.ts accordingly to reflect the single Option type.
In `@src/shared/artifact/artifact.model.ts`:
- Line 7: The createdAt field currently uses Schema.String which permits any
string; change its schema to a stricter datetime validator by replacing
createdAt: Schema.String with either createdAt: Schema.DateTimeUtc (preferred)
or createdAt:
Schema.String.withPattern(/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(\.\d+)?Z$/) in
the artifact model (symbol: createdAt in artifact.model.ts), then update any
code that constructs artifacts to emit ISO-8601 UTC timestamps and run
validation/tests to ensure no callers supply malformed values.
- Line 8: The artifact model's source field uses a plain Schema.String; change
the schema for the source property to enforce URL validation by piping it
through Schema.url() (i.e., replace the current source: Schema.String with
source: Schema.String.pipe(Schema.url()) in the artifact model definition inside
artifact.model.ts) so malformed source values are rejected during validation.
In `@src/shared/artifact/artifact.path.ts`:
- Around line 91-101: The recursive helper findAvailableNumberedOutputPath can
run unbounded; add a max depth (e.g., const MAX_SUFFIX = 1000) and before
recursing check if suffix >= MAX_SUFFIX and fail the Effect with a clear
AppError (e.g., "Too many numbered artifacts, aborting") instead of recursing;
keep the existing behavior otherwise (check candidate via statOptional and
recurse with suffix+1) so the function stops when the limit is hit and returns
an explicit failure rather than risking stack overflow.
In `@src/shared/artifact/artifact.reader.ts`:
- Around line 102-117: Replace the mutable counter `let recordNumber = 0` with
an Effect-managed Ref so the counter follows the functional style: create a Ref
(e.g., `Ref.make(0)`) before piping `artifactLineStream(path)`, and inside the
Stream.mapEffect use `Ref.updateAndGet` (or `Ref.modify`/`updateAndGet`
equivalent) to atomically increment and obtain the current record number for
`parseArtifactLine(line, recordNumber)`, leaving the `artifactLineStream`,
`Stream.mapEffect`, `parseArtifactLine`, and `sawManifest` logic unchanged
except that you pass the number read from the Ref instead of the mutable
variable.
In `@src/shared/artifact/artifact.writer.ts`:
- Around line 60-89: The writeCompressedArtifact function contains a nested
error handler that calls removeArtifactFile on failure, duplicating cleanup
already performed by writeArtifactRecords via removeTempArtifact; remove the
Effect.catch(...) cleanup block from writeCompressedArtifact so it simply fails
with the original error and lets writeArtifactRecords (which calls
removeTempArtifact) perform cleanup; keep writeCompressedArtifact,
writeArtifactRecords, tempSiblingPath, removeArtifactFile and removeTempArtifact
names as reference when locating and updating the code.
In `@src/shared/jira/jira.model.ts`:
- Around line 37-42: The current JiraPropertiesField uses
Schema.optionalKey(JiraProperties).pipe(Schema.decodeTo(JiraProperties, {
decode: SchemaGetter.withDefault(Effect.succeed({})), encode:
SchemaGetter.passthrough() })) — replace this pattern by using the Effect Schema
v4 decoding-default helpers for clarity: remove the manual Schema.decodeTo +
SchemaGetter.withDefault usage and instead apply the appropriate
Schema.withDecodingDefault* helper on JiraProperties (or the optional key) so
missing/undefined values get the same {} default while keeping encoding
passthrough behavior; update references to JiraPropertiesField,
Schema.optionalKey, Schema.decodeTo, SchemaGetter.withDefault,
SchemaGetter.passthrough and JiraProperties accordingly.
🪄 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: 7a5b69f6-97dd-48b5-9246-debdda9ce463
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (49)
.github/workflows/validation.yml.gitignore.prettierignoreREADME.mddocs/artifact.mdeslint.config.jspackage.jsonsrc/cli.test.tssrc/cli/cli.service.tssrc/cli/index.tssrc/errors.tssrc/export/export.formatter.tssrc/export/export.model.tssrc/export/export.service.test.tssrc/export/export.service.tssrc/export/index.tssrc/export/jira.service.test.tssrc/export/jira.service.tssrc/ifj.tssrc/index.tssrc/inspect/index.tssrc/inspect/inspect.formatter.tssrc/inspect/inspect.model.tssrc/inspect/inspect.service.test.tssrc/inspect/inspect.service.tssrc/shared/app/app.model.tssrc/shared/app/index.tssrc/shared/artifact/artifact.model.tssrc/shared/artifact/artifact.path.tssrc/shared/artifact/artifact.reader.test.tssrc/shared/artifact/artifact.reader.tssrc/shared/artifact/artifact.writer.test.tssrc/shared/artifact/artifact.writer.tssrc/shared/artifact/index.tssrc/shared/config/config.model.tssrc/shared/config/config.provider.tssrc/shared/config/export.config.tssrc/shared/config/index.tssrc/shared/config/inspect.config.tssrc/shared/jira/index.tssrc/shared/jira/jira.client.test.tssrc/shared/jira/jira.client.tssrc/shared/jira/jira.model.tssrc/shared/jira/retry.util.tssrc/warnings.tstest/index.test.tstsconfig.build.jsontsconfig.jsonvitest.config.ts
💤 Files with no reviewable changes (2)
- test/index.test.ts
- src/index.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1
Summary by CodeRabbit
Release Notes
New Features
ifj exportcommand to export Intercom-for-Jira data with support for source/target Jira Cloud credentials, explicit space selection, and flexible configuration via flags, environment variables, or.envfiles.ifj inspectcommand to validate exported artifacts and display summary statistics.Documentation