-
Notifications
You must be signed in to change notification settings - Fork 1
FE-578 Fix Shift+Enter Line Breaks in Jira Descriptions (ADF Native Processing) #49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FE-578 Fix Shift+Enter Line Breaks in Jira Descriptions (ADF Native Processing) #49
Conversation
…. & Add ADF manipulation utilities to facilitate direct ADF operations
- implement ADF to Markdown conversion utilities for AI prompt generation
…enhance prompt generation details
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the codebase to work natively with ADF (Atlassian Document Format) instead of performing lossy Markdown round-trip conversions. The primary goal is to preserve hardBreak nodes (Shift+Enter line breaks) and other ADF formatting that gets lost when converting ADF → Markdown → ADF.
Key changes:
- Renamed conversion functions to
convertMarkdownToAdf_NewContentOnly()andconvertAdfToMarkdown_AIPromptOnly()with clear usage constraints - Created comprehensive ADF manipulation utilities in
adf-operations.ts - Implemented ADF-native shell story parser to replace Markdown-based parsing
- Refactored business logic in
write-next-storyandwrite-shell-storiestools to use native ADF operations - Introduced
AIPromptContextinterface to enforce one-way Markdown conversion for AI prompts only
Benefits: Preserves all Jira formatting (hardBreaks, nested lists, marks, etc.), reduces AI token costs by 3-4x, prevents data corruption, and improves overall system reliability.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
specs/adf-native-processing-refactor.md |
Comprehensive implementation plan and design decisions for the ADF native refactor |
server/providers/atlassian/markdown-converter.ts |
Renamed conversion functions with usage warnings in JSDoc |
server/providers/atlassian/adf-operations.ts |
New pure ADF manipulation utilities (extract, remove, append, replace sections) |
server/providers/combined/tools/shared/shell-story-adf-parser.ts |
New ADF-based parser replacing Markdown parser, preserves all node types |
server/providers/combined/tools/shared/ai-prompt-context.ts |
Centralized AI prompt context conversion with _AIPromptOnly naming |
server/providers/combined/tools/writing-shell-stories/figma-screen-setup.ts |
Updated interface to return ADF fields instead of Markdown |
server/providers/combined/tools/writing-shell-stories/core-logic.ts |
Refactored to use ADF operations and AIPromptContext |
server/providers/combined/tools/write-next-story/core-logic.ts |
Refactored to use ADF operations for epic updates |
server/providers/combined/tools/write-next-story/prompt-story-generation.ts |
Updated to build prompts from parsed fields instead of rawContent |
server/providers/combined/tools/shared/screen-analysis-pipeline.ts |
Updated to use prepareAIPromptContext() |
server/providers/atlassian/test-fixtures/adf/*.json |
Test fixtures for ADF structures with hardBreaks, shell stories |
server/providers/combined/tools/shared/shell-story-adf-parser.test.ts |
Comprehensive test suite for ADF parser (733 lines) |
server/providers/atlassian/adf-operations.test.ts |
Comprehensive test suite for ADF operations (501 lines) |
server/providers/atlassian/markdown-converter.test.ts |
Updated function names in existing tests |
| Various tool files | Updated import statements to use renamed conversion functions |
server/providers/combined/tools/write-next-story/prompt-story-generation.ts
Outdated
Show resolved
Hide resolved
server/providers/combined/tools/write-next-story/prompt-story-generation.ts
Outdated
Show resolved
Hide resolved
server/providers/combined/tools/shared/screen-analysis-pipeline.ts
Outdated
Show resolved
Hide resolved
server/providers/combined/tools/shared/shell-story-adf-parser.ts
Outdated
Show resolved
Hide resolved
server/providers/combined/tools/shared/shell-story-adf-parser.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| epicSansShellStoriesMarkdown, | ||
| epicSansShellStoriesAdf, | ||
| epicDescriptionAdf: description, | ||
| shellStoriesAdf, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
epicContext, epicMarkdown & contentWithoutShellStories needed more explicit names & to clarify if they were markdown or adf. I think another file is misusing epicContent as if it was epicSansScopeAnalysis.
Nitpick:
- personally I'd use
epicSansShellStoriesMDinstead ofepicSansShellStoriesMarkdown(&epicSansShellStoriesADF), but I kept with repo conventions for now - I also think
epicContentshould be renamed across the entire repo, but for now only renamed it in select files
| epicSansShellStoriesMarkdown, | ||
| epicSansShellStoriesAdf, | ||
| epicDescriptionAdf: description, | ||
| shellStoriesAdf, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- rename
epicContent&contentWithoutShellStoriesfor clarity - provide
epicDescriptionAdfinstead ofepicMarkdown, so write-next-story can update adf directly & not have data loss - provide
shellStoriesAdfso write-next-story doesn't have to duplicate the extraction. (shellStoriesAdf is already provided when contentWithoutShellStories are is extracted from the adfDoc)
| */ | ||
| export function parseShellStories(shellStoriesContent: string): ParsedShellStory[] { | ||
| const stories: ParsedShellStory[] = []; | ||
| export function parseShellStories(shellStoriesContent: string): ParsedShellStoryMarkdown[] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@justinbmeyer
parseShellStories & ParsedShellStoryMarkdown are only used in write-next-story-old.ts.
Can we delete that file and delete these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
| yamlContent, | ||
| epicContext, | ||
| contentWithoutSection, | ||
| epicSansShellStoriesMarkdown: epicContext, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use epicWithoutShellStoriesMarkdown
Without is more comman than Sans outside design contexts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do. updated in next commit.
That's what I initially had anyways, but then I over-optimized trying to make it shorter. I was also on the fence over it bc Sans isn't as quickly comprehended, didn't know that it was used in design contexts.
| * "https://bitovi.atlassian.net/browse/PROJ-123" | ||
| * ); | ||
| */ | ||
| export function addCompletionMarkerToStory( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be addCompletionMarkerToShellStory. It's probably good to distinguish between "stories" and "shell stories".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Updated in next commit.
| // Convert entire listItem to markdown for AI prompts (preserves original formatting) | ||
| const rawContent = convertAdfNodesToMarkdown([listItem]); | ||
|
|
||
| return { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is any of this actually needed?
We just needed to know if shell stories exist, I don't believe we needed any of this sort of information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These were all added for parity with the original function, but you're right that some aren't needed.
I'm removing
timestampsincludedlowPriorityexcludedquestions
and adding comments to the rest to point out where they're used
…logic - Deleted `write-next-story-old.ts` - Renamed variables and properties - removed unneeded returns from `parseShellStoryFromListItem`
|
|
||
| // Validate shellStoriesAdf data structure | ||
| if (!Array.isArray(setupResult.shellStoriesAdf)) { | ||
| throw new Error(`Epic ${setupResult.epicKey} has invalid shellStoriesAdf data (expected array, got ${typeof setupResult.shellStoriesAdf})`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Users can see these errors we throw. I'm not sure this would be a good error for them to understand. Under what circumstances could this ever happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unneeded & removed. I think this was a holdover from some other refactoring I reverted.
| const storyBlocks = shellStoriesContent.split(/\n- /); | ||
| // Find the story's listItem | ||
| for (const node of newSection) { | ||
| if (node.type === 'bulletList' && node.content) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there some way to clean up this?
I wonder if there's some sort of JSON-query type library we could bring in that would let us parse this more simply ...
const para = itemContent.find(node => node.type === 'paragraph') ?? null;
if (!para?.content) return null;
for (const textNode of para.content) {
if (textNode.type === 'text' && hasMarkType(textNode, 'code')) {
const match = textNode.text?.match(/^st\d+$/);
if (match) return match[0];
}
}
.find( [
{type: 'bulletList'},
{type: 'paragraph', index: 0},
{type: 'text'}, {text: /^st\d+$/}]
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for each main shell story list item
check if it has the same id
if it has the same id
for each paragraph in the list item
for nodes in the paragraph
IF storyId, skip (but mark we found the id)
IF title, turn node into a link
IF separator, mark as found
IF description
check for `em` for date
IF no date, add date at the end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
findInSequence(p, [{type: 'text', marks: ['cide']}, {type: 'text'}]
forEachWithContent(newSection, {type: 'bulletList'}, (bulletedList) => {
const id = extractStoryId(listItem.content);
if (id === storyId) {
forEachWithContent(listItem.content, {type: 'paragraph'}, (para) => {
const {title, id, description, timestamp } = findTitleParts(para.content)
const titleNode = findShellStoryTitleNodeFromParagraph(para.content);
if(title) addLinkToNode( title, {url} )
if(!timestamp){
para.content.push({
type: 'text',
text: ' '
});
para.content.push({
type: 'text',
text: `(${new Date().toISOString()})`,
marks: [{ type: 'em' }]
});
} else{
timestamp.text = `(${new Date().toISOString()})`,
}
})
}
})
function findTitleParts(){
}
| if (id !== storyId) return; | ||
| storyFound = true; | ||
| forEachWithContent(listItem.content!, { type: 'paragraph' }, (paragraph) => { | ||
| if (!paragraph.content) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forEachWithContent already ensures paragraph.content (hence the "WithContent" part of the helper). This check isn't needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed. Added the type-narrowing in forEachWithContent that was missing
| } | ||
|
|
||
| // Find title parts from a paragraph content: title nodes between ID and separator | ||
| function findTitleParts(content: ADFNode[]): { titleNodes: ADFNode[] } { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why doesn't this look more like: const {title, id, description, timestamp } = findTitleParts(para.content) as suggested in the comment?
This way it could be used also by extractStoryId and we can do an "upsert" more or less on the timestamp?
Also, part of the goal was to separate parsing and mutation. appendOrUpdateTimestamp is doing both still now.
This would but
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why doesn't this look more like: const {title, id, description, timestamp } = findTitleParts(para.content) as suggested in the comment?
updated. id & description aren't needed here though, so I left them out. findTitleParts became extractTitleParts & does return storyId & descriptionString, but those are only used in other functions
This way it could be used also by extractStoryId ...
extractStoryId needs to do a depth search, so I kept them separate.
part of the goal was to separate parsing and mutation. appendOrUpdateTimestamp is doing both still now.
resolved by doing them inline like the initial example instead of splitting appendOrUpdateTimestamp
justinbmeyer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comments
| forEachWithContent(itemContent, { type: 'bulletList' }, (bulletList) => { | ||
| forEachWithContent(bulletList.content, { type: 'listItem' }, (listItem) => { | ||
| forEachWithContent(listItem.content, { type: 'paragraph' }, (paragraph) => { | ||
| const content = paragraph.content ?? []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't forEachWithContent mean paragraph.content will always exist and have one element?
| forEachWithContent(itemContent, { type: 'bulletList' }, (bulletList) => { | ||
| forEachWithContent(bulletList.content, { type: 'listItem' }, (listItem) => { | ||
| forEachWithContent(listItem.content, { type: 'paragraph' }, (paragraph) => { | ||
| const content = paragraph.content ?? []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comments as above

Problem
Users were losing Shift+Enter line breaks (hard breaks) within bullet lists when updating Jira issues through our MCP tools. This happened because the codebase was doing lossy round-trip conversions: ADF → Markdown , which stripped out
hardBreaknodes that don't cleanly map to Markdown, then converting that back -> ADF & uploading to Jira.Solution
Refactored the write-next-story to work directly with native ADF (Atlassian Document Format) for all data manipulation, eliminating conversions entirely. Markdown is now only used one-way for AI prompt generation (never converted back).
Changes
write-next-story/core-logic.tsuses newparseShellStoriesFromAdf()&addCompletionMarkerToStory()for epic updatesfigma-screen-setup.tsreturns ADF structures as well as Markdown now. Renames some of it's return values for clarity.hardBreak, nested lists, marks, inline cards, mentions, etc.