From a13c08a45b712245bfb9b1e8f10d9e3306b74e2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9mie=20Astori?= Date: Wed, 19 Jul 2017 01:26:29 -0400 Subject: [PATCH] Enforce correct order for previews on server-side prefectch rather than at client parsing This has the benefit of not adding `.preview` divs everywhere, anytime we use `parse()`, and also to un-tie the position of the preview blocks from the result of the helper. This means that templates that call `parse` and have some extra markup after that are not constrained anymore. This is effectively an alternative, better way to fix https://github.com/thelounge/lounge/issues/1343, but the initial fix that was put in place (https://github.com/thelounge/lounge/pull/1347) is still relevant, for example to make sure a preview stays hidden (and does not add extra margin/padding/etc.) if the link does not prefetch. --- client/js/libs/handlebars/parse.js | 3 -- client/views/actions/action.tpl | 4 ++ client/views/msg.tpl | 8 +++- src/models/msg.js | 1 + src/plugins/irc-events/link.js | 20 ++++----- src/plugins/irc-events/message.js | 3 +- test/client/js/libs/handlebars/parse.js | 56 +++++++------------------ test/plugins/link.js | 26 +++++++----- test/util.js | 1 + 9 files changed, 55 insertions(+), 67 deletions(-) diff --git a/client/js/libs/handlebars/parse.js b/client/js/libs/handlebars/parse.js index 1a581904..b5e9e5d7 100644 --- a/client/js/libs/handlebars/parse.js +++ b/client/js/libs/handlebars/parse.js @@ -81,8 +81,5 @@ module.exports = function parse(text) { } return fragments; - }).join("") + linkParts.map((part) => { - const escapedLink = Handlebars.Utils.escapeExpression(part.link); - return `
`; }).join(""); }; diff --git a/client/views/actions/action.tpl b/client/views/actions/action.tpl index 2941459c..0f7c2133 100644 --- a/client/views/actions/action.tpl +++ b/client/views/actions/action.tpl @@ -1,2 +1,6 @@ {{> ../user_name nick=from}} {{{parse text}}} + +{{#each links}} +
+{{/each}} diff --git a/client/views/msg.tpl b/client/views/msg.tpl index 6b5eaf7e..b28c1ba0 100644 --- a/client/views/msg.tpl +++ b/client/views/msg.tpl @@ -7,5 +7,11 @@ {{> user_name nick=from}} {{/if}} - {{{parse text}}} + + {{{parse text}}} + + {{#each links}} +
+ {{/each}} + diff --git a/src/models/msg.js b/src/models/msg.js index 4421b5de..c0ede193 100644 --- a/src/models/msg.js +++ b/src/models/msg.js @@ -31,6 +31,7 @@ function Msg(attr) { _.defaults(this, attr, { from: "", id: id++, + links: [], previews: [], text: "", type: Msg.Type.MESSAGE, diff --git a/src/plugins/irc-events/link.js b/src/plugins/irc-events/link.js index fbf00479..c6f75a12 100644 --- a/src/plugins/irc-events/link.js +++ b/src/plugins/irc-events/link.js @@ -24,19 +24,19 @@ module.exports = function(client, chan, msg) { return; } - Array.from(new Set( // Remove duplicate links + msg.links = Array.from(new Set( // Remove duplicate links links.map((link) => escapeHeader(link.link)) - )) - .slice(0, 5) // Only preview the first 5 URLs in message to avoid abuse - .forEach((link) => { - fetch(link, function(res) { - if (res === null) { - return; - } + )).slice(0, 5); // Only preview the first 5 URLs in message to avoid abuse - parse(msg, link, res, client); - }); + msg.links.forEach((link) => { + fetch(link, function(res) { + if (res === null) { + return; + } + + parse(msg, link, res, client); }); + }); }; function parse(msg, url, res, client) { diff --git a/src/plugins/irc-events/message.js b/src/plugins/irc-events/message.js index f83f1b4a..039f5d62 100644 --- a/src/plugins/irc-events/message.js +++ b/src/plugins/irc-events/message.js @@ -89,11 +89,12 @@ module.exports = function(irc, network) { self: self, highlight: highlight }); - chan.pushMessage(client, msg, !self); // No prefetch URLs unless are simple MESSAGE or ACTION types if ([Msg.Type.MESSAGE, Msg.Type.ACTION].indexOf(data.type) !== -1) { LinkPrefetch(client, chan, msg); } + + chan.pushMessage(client, msg, !self); } }; diff --git a/test/client/js/libs/handlebars/parse.js b/test/client/js/libs/handlebars/parse.js index aa0e5050..3697a5ba 100644 --- a/test/client/js/libs/handlebars/parse.js +++ b/test/client/js/libs/handlebars/parse.js @@ -37,15 +37,13 @@ describe("parse Handlebars helper", () => { expected: "" + "irc://freenode.net/thelounge" + - "" + - "
" + "" }, { input: "www.nooooooooooooooo.com", expected: "" + "www.nooooooooooooooo.com" + - "" + - "
" + "" }, { input: "look at https://thelounge.github.io/ for more information", expected: @@ -53,8 +51,7 @@ describe("parse Handlebars helper", () => { "" + "https://thelounge.github.io/" + "" + - " for more information" + - "
" + " for more information" }, { input: "use www.duckduckgo.com for privacy reasons", expected: @@ -62,26 +59,13 @@ describe("parse Handlebars helper", () => { "" + "www.duckduckgo.com" + "" + - " for privacy reasons" + - "
" + " for privacy reasons" }, { input: "svn+ssh://example.org", expected: "" + "svn+ssh://example.org" + - "" + - "
" - }, { - input: "https://example.com https://example.org", - expected: - "" + - "https://example.com" + - " " + - "" + - "https://example.org" + - "" + - "
" + - "
" + "" }]; const actual = testCases.map((testCase) => parse(testCase.input)); @@ -97,8 +81,7 @@ describe("parse Handlebars helper", () => { "bonuspunkt: your URL parser misparses this URL: " + "" + "https://msdn.microsoft.com/en-us/library/windows/desktop/ms644989(v=vs.85).aspx" + - "" + - "
"; + ""; const actual = parse(input); @@ -113,8 +96,7 @@ describe("parse Handlebars helper", () => { "" + "https://theos.kyriasis.com/~kyrias/stats/archlinux.html" + "" + - ">" + - "
" + ">" }, { input: "abc (www.example.com)", expected: @@ -122,22 +104,19 @@ describe("parse Handlebars helper", () => { "" + "www.example.com" + "" + - ")" + - "
" + ")" }, { input: "http://example.com/Test_(Page)", expected: "" + "http://example.com/Test_(Page)" + - "" + - "
" + "" }, { input: "www.example.com/Test_(Page)", expected: "" + "www.example.com/Test_(Page)" + - "" + - "
" + "" }]; const actual = testCases.map((testCase) => parse(testCase.input)); @@ -274,8 +253,7 @@ describe("parse Handlebars helper", () => { "freenode.net" + "/" + "thelounge" + - "" + - "
" + "" }, { input: "\x02#\x038,9thelounge", expected: @@ -314,16 +292,14 @@ describe("parse Handlebars helper", () => { "like.." + "" + "http://example.com" + - "" + - "
" + "" }, { input: "like..HTTP://example.com", expected: "like.." + "" + "HTTP://example.com" + - "" + - "
" + "" }]; const actual = testCases.map((testCase) => parse(testCase.input)); @@ -339,8 +315,7 @@ describe("parse Handlebars helper", () => { "" + "" + "http://example.com/#hash" + - "" + - "
" + "" }]; const actual = testCases.map((testCase) => parse(testCase.input)); @@ -355,8 +330,7 @@ describe("parse Handlebars helper", () => { expect(actual).to.equal( "Url: http://example.com/path " + - "Channel: ##channel" + - "
" + "Channel: ##channel" ); }); }); diff --git a/test/plugins/link.js b/test/plugins/link.js index 3f2aa9bc..61e00b72 100644 --- a/test/plugins/link.js +++ b/test/plugins/link.js @@ -27,8 +27,9 @@ describe("Link plugin", function() { }); it("should be able to fetch basic information about URLs", function(done) { + const url = "http://localhost:9002/basic"; const message = this.irc.createMessage({ - text: "http://localhost:9002/basic" + text: url }); link(this.irc, this.network.channels[0], message); @@ -41,8 +42,10 @@ describe("Link plugin", function() { expect(data.preview.type).to.equal("link"); expect(data.preview.head).to.equal("test title"); expect(data.preview.body).to.equal("simple description"); - expect(data.preview.link).to.equal("http://localhost:9002/basic"); - expect(message.previews.length).to.equal(1); + expect(data.preview.link).to.equal(url); + + expect(message.links).to.deep.equal([url]); + expect(message.previews).to.deep.equal([data.preview]); done(); }); }); @@ -178,6 +181,11 @@ describe("Link plugin", function() { link(this.irc, this.network.channels[0], message); + expect(message.links).to.deep.equal([ + "http://localhost:9002/one", + "http://localhost:9002/two" + ]); + this.app.get("/one", function(req, res) { res.send("first title"); }); @@ -186,22 +194,18 @@ describe("Link plugin", function() { res.send("second title"); }); - const loaded = { - one: false, - two: false - }; + const previews = []; this.irc.on("msg:preview", function(data) { if (data.preview.link === "http://localhost:9002/one") { expect(data.preview.head).to.equal("first title"); - loaded.one = true; } else if (data.preview.link === "http://localhost:9002/two") { expect(data.preview.head).to.equal("second title"); - loaded.two = true; } + previews.push(data.preview); - if (loaded.one && loaded.two) { - expect(message.previews.length).to.equal(2); + if (previews.length === 2) { + expect(message.previews).to.deep.equal(previews); done(); } }); diff --git a/test/util.js b/test/util.js index bee4c3af..69a84d60 100644 --- a/test/util.js +++ b/test/util.js @@ -21,6 +21,7 @@ MockClient.prototype.createMessage = function(opts) { text: "dummy message", nick: "test-user", target: "#test-channel", + links: [], previews: [], }, opts);