feat: add import#4
Open
tbinna wants to merge 1 commit into
Open
Conversation
|
Warning Review limit reached
More reviews will be available in 10 minutes and 42 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (26)
📝 WalkthroughWalkthroughThis PR implements the ChangesImport Command Feature
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CliService (import command)
participant ImportService
participant ImportJiraService
participant JiraClient
participant JiraAPI as Jira Cloud API
User->>CLI: ifj import --target --user --api-token --space
CLI->>ImportService: run
ImportService->>JiraClient: verifyGlobalAdmin
JiraClient->>JiraAPI: GET /mypermissions
JiraAPI-->>JiraClient: permissions
ImportService->>ImportService: assessImportPlan(records)
loop for each planned space
ImportService->>JiraClient: targetSpaceAvailable(spaceKey)
JiraClient->>JiraAPI: GET /project
JiraAPI-->>JiraClient: projects list
ImportService->>JiraClient: writeProjectProperty(spaceKey, config)
JiraClient->>JiraAPI: PUT /project/{key}/properties/{propKey}
loop batches of 100 work-item link records
ImportService->>JiraClient: bulkFetchWorkItems(keys)
JiraClient->>JiraAPI: POST /issue/bulkfetch
JiraAPI-->>JiraClient: issues[]
ImportService->>JiraClient: submitIssuePropertyBulkTask(propKey, updates)
JiraClient->>JiraAPI: POST /issue/properties/multi
JiraAPI-->>JiraClient: 303 Location
loop poll every 2s
ImportService->>JiraClient: getTask(location)
JiraClient->>JiraAPI: GET task URL
JiraAPI-->>JiraClient: JiraTaskResponse
end
end
end
ImportService-->>CLI: ImportSummary
CLI-->>User: formatted summary (human or JSON)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/import/import.plan.test.ts (1)
25-98: ⚡ Quick winAdd direct tests for manifest gating and same-site warning paths.
Current coverage is solid for space selection, but it misses the two critical planner branches: fatal
artifact.manifestMissingandSAME_SITE_IMPORTwarning emission.Suggested additions
+ it.effect("fails when first record is not a manifest", () => + Effect.gen(function* () { + const exit = yield* Effect.exit( + assessImportPlan(defaultConfig, [ + { + type: "spaceConfiguration", + spaceKey: "ENG", + configuration: { enabled: true }, + }, + ]), + ); + expect(exit._tag).toBe("Failure"); + }), + ); + + it.effect("emits SAME_SITE_IMPORT warning for same source and target", () => + Effect.gen(function* () { + const plan = yield* assessImportPlan( + { ...defaultConfig, target: "https://source.atlassian.net" }, + artifact(), + ); + expect(plan.warnings).toMatchObject([{ code: "SAME_SITE_IMPORT" }]); + }), + );🤖 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/import/import.plan.test.ts` around lines 25 - 98, The test suite for the import assessment currently covers space selection scenarios but is missing tests for two critical code paths: the artifact.manifestMissing fatal error case and the SAME_SITE_IMPORT warning emission. Add two new it.effect test cases within the describe block that test the assessImportPlan function behavior when the artifact lacks a manifest (should result in a fatal error) and when a same-site import scenario occurs (should emit a warning), ensuring these edge cases are properly validated alongside the existing space selection tests.src/import/jira.service.ts (1)
217-220: 💤 Low valueUnused
spaceKeyparameter inwriteWorkItemConversationLinks.The service interface declares
spaceKeyas a parameter (line 217-218), but the implementation ignores it (line 233 uses_spaceKey). If this parameter is intentionally reserved for future use (e.g., logging, metrics, or scoping), consider documenting that intent. Otherwise, remove it from the interface to avoid confusion.Also applies to: 233-234
🤖 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/import/jira.service.ts` around lines 217 - 220, The `writeWorkItemConversationLinks` method declares a `spaceKey` parameter that is not actively used in the implementation (notice the underscore prefix convention `_spaceKey` at line 233-234 indicates unused parameters). Either remove the `spaceKey` parameter from both the interface declaration at lines 217-218 and the implementation at lines 233-234, or if this parameter is reserved for future use such as logging or metrics, add a clear documentation comment explaining its intended purpose in the interface declaration.src/shared/jira/jira.client.test.ts (1)
463-490: ⚡ Quick winAdd a negative regression test for cross-origin task locations.
This test block verifies only the happy path. Please add a case asserting
getTask(...)fails withjira.malformedwhen the task URL origin differs (guarded at Line 381 insrc/shared/jira/jira.client.ts), so SSRF protection can’t regress silently.🤖 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.client.test.ts` around lines 463 - 490, Add a new negative regression test case in the test file following the existing happy path test that verifies the getTask method rejects cross-origin task locations. The test should invoke getTask with a URL that has a different origin than the configured Jira instance (e.g., a URL from a different domain) and assert that it fails with a jira.malformed error, ensuring the SSRF protection guard at Line 381 in src/shared/jira/jira.client.ts cannot regress silently.
🤖 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/import/import.plan.ts`:
- Around line 27-29: The sameSiteWarning function performs a raw string
comparison between source and target URLs without normalization, which allows
semantically identical URLs with minor differences (such as trailing slashes or
casing variations) to bypass the SAME_SITE_IMPORT warning. Normalize both the
source and target parameters before the comparison in the sameSiteWarning
function (for example, by converting to lowercase and removing trailing
slashes). Apply the same normalization logic to the other location mentioned at
line 84 to ensure consistent URL comparison behavior throughout the import plan
logic.
In `@src/import/import.service.ts`:
- Around line 64-83: The summaryFromState function applies detailLimit to the
warnings field by slicing it, but the skippedSpaces, failedSpaceConfigurations,
skippedWorkItemLinks, and failedWorkItemLinkBatches fields are included without
truncation. Apply the same slice(0, detailLimit) approach to each of these four
fields in the summaryFromState function to ensure they are consistently limited
and prevent the summary from exceeding the intended size constraints.
In `@src/import/jira.service.ts`:
- Around line 115-136: In the resolveWorkItems function, the byKey map is keyed
by the canonical item keys returned from Jira's API, but the lookup attempts to
match against the originally requested keys which may differ in casing.
Normalize both sides of the comparison by converting keys to uppercase: when
creating the byKey map, use item.key.toUpperCase() as the map key, and when
performing the lookup with byKey.get(requestedKey), pass
requestedKey.toUpperCase() instead to ensure case-insensitive matching works
correctly regardless of the case variant of the requested key.
In `@src/shared/jira/jira.client.ts`:
- Around line 337-343: The numeric conversion of propertyUpdate.issueId lacks
validation before calling Number(), which could result in NaN or precision loss
if the issueId is not a valid numeric string. Before mapping over
propertyUpdates and converting issueId with Number() in the
HttpClientRequest.schemaBodyJson call, add validation to ensure each issueId is
a safe integer. This can be done by checking that the issueId string can be
safely parsed as an integer using Number.isSafeInteger() after conversion, or by
validating the string format before conversion to prevent data corruption from
invalid issue targeting.
---
Nitpick comments:
In `@src/import/import.plan.test.ts`:
- Around line 25-98: The test suite for the import assessment currently covers
space selection scenarios but is missing tests for two critical code paths: the
artifact.manifestMissing fatal error case and the SAME_SITE_IMPORT warning
emission. Add two new it.effect test cases within the describe block that test
the assessImportPlan function behavior when the artifact lacks a manifest
(should result in a fatal error) and when a same-site import scenario occurs
(should emit a warning), ensuring these edge cases are properly validated
alongside the existing space selection tests.
In `@src/import/jira.service.ts`:
- Around line 217-220: The `writeWorkItemConversationLinks` method declares a
`spaceKey` parameter that is not actively used in the implementation (notice the
underscore prefix convention `_spaceKey` at line 233-234 indicates unused
parameters). Either remove the `spaceKey` parameter from both the interface
declaration at lines 217-218 and the implementation at lines 233-234, or if this
parameter is reserved for future use such as logging or metrics, add a clear
documentation comment explaining its intended purpose in the interface
declaration.
In `@src/shared/jira/jira.client.test.ts`:
- Around line 463-490: Add a new negative regression test case in the test file
following the existing happy path test that verifies the getTask method rejects
cross-origin task locations. The test should invoke getTask with a URL that has
a different origin than the configured Jira instance (e.g., a URL from a
different domain) and assert that it fails with a jira.malformed error, ensuring
the SSRF protection guard at Line 381 in src/shared/jira/jira.client.ts cannot
regress silently.
🪄 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: 3073dab6-34e2-4172-9fee-f3a1b263ca0c
📒 Files selected for processing (26)
.gitignoreREADME.mdsrc/cli.test.tssrc/cli/cli.service.tssrc/errors.tssrc/export/export.service.test.tssrc/export/export.service.tssrc/export/jira.service.tssrc/import/import.formatter.tssrc/import/import.model.tssrc/import/import.plan.test.tssrc/import/import.plan.tssrc/import/import.service.test.tssrc/import/import.service.tssrc/import/index.tssrc/import/jira.service.test.tssrc/import/jira.service.tssrc/shared/app/app.model.tssrc/shared/artifact/artifact.model.tssrc/shared/artifact/artifact.reader.test.tssrc/shared/config/config.model.tssrc/shared/config/import.config.tssrc/shared/config/index.tssrc/shared/jira/jira.client.test.tssrc/shared/jira/jira.client.tssrc/shared/jira/jira.model.ts
Closes #3
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 #3
Summary by CodeRabbit
New Features
importcommand to CLI for importing Jira data from artifacts with space selection and batching support.Documentation
Tests
Chores
.gitignoreto exclude export and cache files.