Skip to content

Handle the case when a code block has no grandchildren #919

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Sep 13, 2024
Merged

Handle the case when a code block has no grandchildren #919

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4b0748e
content [nfc]: Use single and last instead to access child.
PIG208 Aug 30, 2024
5d36977
content: Handle code block elements without grandchildren.
PIG208 Aug 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions lib/model/content.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -986,18 +986,18 @@ class _ZulipContentParser {
&& divElement.className == "codehilite");

if (divElement.nodes.length != 1) return null;
final child = divElement.nodes[0];
final child = divElement.nodes.single;
if (child is! dom.Element) return null;
if (child.localName != 'pre') return null;

if (child.nodes.length > 2) return null;
if (child.nodes.length > 2 || child.nodes.isEmpty) return null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: include the key "why" information in the commit message, rather than rely on the PR description. We aim for the Git history to be able to serve as our primary record of what we did.

In particular key points include

  • this was a crash bug;
  • the affected messages in public chat.zulip.org history ranged from message ID 5020 to 29855, all in 2016.

Details that can be read off from the commit's added test case don't need to be repeated in the commit message, though.

if (child.nodes.length == 2) {
final first = child.nodes[0];
if (first is! dom.Element
|| first.localName != 'span'
|| first.nodes.isNotEmpty) return null;
}
final grandchild = child.nodes[child.nodes.length - 1];
final grandchild = child.nodes.last;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a pure refactor, or does this change behavior somehow? I noticed there's a line final first = child.nodes[0]; above this; should that also be changed to say child.nodes.first?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This refactoring is purely for readability. Let's get an nfc commit for both.

if (grandchild is! dom.Element) return null;
if (grandchild.localName != 'code') return null;

Expand Down
12 changes: 12 additions & 0 deletions test/model/content_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,17 @@ class ContentExample {
]),
]);

// Current servers no longer produce this, but it can be found in ancient
// messages. For example:
// https://chat.zulip.org/#narrow/stream/2-general/topic/Error.20in.20dev.20server/near/18765
static final codeBlockWithEmptyBody = ContentExample(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's have a comment that makes it clear we don't expect this in messages from current servers. The link to the ancient message you put in the PR description is helpful; we could include that here too.

'code block, with an empty body',
'```',
'<div class="codehilite"><pre></pre></div>', [
blockUnimplemented(
'<div class="codehilite"><pre></pre></div>'),
]);

static final codeBlockWithHighlightedLines = ContentExample(
'code block, with syntax highlighting and highlighted lines',
'```\n::markdown hl_lines="2 4"\n# he\n## llo\n### world\n```',
Expand Down Expand Up @@ -1149,6 +1160,7 @@ void main() {
testParseExample(ContentExample.codeBlockPlain);
testParseExample(ContentExample.codeBlockHighlightedShort);
testParseExample(ContentExample.codeBlockHighlightedMultiline);
testParseExample(ContentExample.codeBlockWithEmptyBody);
testParseExample(ContentExample.codeBlockWithHighlightedLines);
testParseExample(ContentExample.codeBlockWithUnknownSpanType);
testParseExample(ContentExample.codeBlockFollowedByMultipleLineBreaks);
Expand Down