Repository navigation
feat(document): show text and code documents in a code viewer - #528
Conversation
caro3801
left a comment
There was a problem hiding this comment.
Very nice improvement of the code display. I only have suggestions for this PR considering that the cases mentions have low probability of occurring, let me know what you think.
| return this.contentType.indexOf('text/') === 0 | ||
| || this.contentType.endsWith('+xml') | ||
| || this.isJson | ||
| || codeTypes.includes(this.contentType) | ||
| } |
There was a problem hiding this comment.
suggestion: the raw content type can contain multiple info
(application/xml; charset=utf-8) the exact match branch would then and lands on "Preview is not available", while text/...; charset=... sails through on the prefix test. This index demonstrably stores parameters (isTweet at line 412 matches
'application/json; twint').
Could we strip the parameter once before testing? codeLanguage.js:14 already does the same split(';') , so the shape is familiar (may be factorized).
| return this.contentType.indexOf('text/') === 0 | |
| || this.contentType.endsWith('+xml') | |
| || this.isJson | |
| || codeTypes.includes(this.contentType) | |
| } | |
| const mimeType = this.contentType.split(';')[0].trim() | |
| return mimeType.indexOf('text/') === 0 | |
| || mimeType.endsWith('+xml') | |
| || this.isJson | |
| || codeTypes.includes(mimeType) |
There was a problem hiding this comment.
Done, the type is now stripped of its parameters before the checks.
| * @return {Object[]} - The `{ from, to }` document ranges, in document order. | ||
| */ | ||
| export function findIndexMatches(chunks, doc, term) { | ||
| const { folded } = foldWithSourceIndexes(term) |
There was a problem hiding this comment.
suggestion: foldWithSourceIndexes builds two offset arrays over the term that are destructured away immediately.
foldForFilter (added in this diff, already imported on line 1) returns the same string without them. Worth dropping foldWithSourceIndexes from the line 1 import once this changes, since it becomes unused in this file.
| const { folded } = foldWithSourceIndexes(term) | |
| const folded = foldForFilter(term) |
There was a problem hiding this comment.
Done, and the unused import is removed.
| sourceIndexes.push(...Array(units).fill(index)) | ||
| sourceEnds.push(...Array(units).fill(end)) |
There was a problem hiding this comment.
question (perf): any particular reason to drop the push loop ? These two lines allocate two throwaway arrays plus two spreads per character, on the path that folds for every candidate chunk. A plain push loop keeps the behaviour exactly and drops the allocations.
| sourceIndexes.push(...Array(units).fill(index)) | |
| sourceEnds.push(...Array(units).fill(end)) | |
| for (let unit = 0; unit < units; unit++) { | |
| sourceIndexes.push(index) | |
| sourceEnds.push(end) | |
| } |
There was a problem hiding this comment.
No reason, the spread version was just extra allocations. The loop is back.
| watch(toRef(props, 'document'), async (document) => { | ||
| blurred.value = await isBlurred(document) | ||
| blurredContent.value = blurred.value ? await getBlurredContentBanner(document) : null | ||
| }, { immediate: true }) |
There was a problem hiding this comment.
suggestion: The superseded-load guards around load are careful and correct on every path, including abort, 404, too-large and unmount. The blurred-content watcher is the one async path that did not get the same superseded-load treatment, so a fast document switch can strand the new document behind the previous project's banner with search disabled.
Could we re-check the document identity after the awaits, the way isCurrentLoad does for the source?
| watch(toRef(props, 'document'), async (document) => { | |
| blurred.value = await isBlurred(document) | |
| blurredContent.value = blurred.value ? await getBlurredContentBanner(document) : null | |
| }, { immediate: true }) | |
| watch(toRef(props, 'document'), async (document) => { | |
| const value = await isBlurred(document) | |
| const banner = value ? await getBlurredContentBanner(document) : null | |
| // A document swapped in while this one resolved owns the banner now. | |
| if (document !== props.document) { | |
| return | |
| } | |
| blurred.value = value | |
| blurredContent.value = banner | |
| }, { immediate: true }) |
There was a problem hiding this comment.
Done, a superseded result is now dropped. There's a new test for a fast document switch.
Co-authored-by: Caroline Desprat <cdesprat@icij.org>
Co-authored-by: Caroline Desprat <cdesprat@icij.org>
Co-authored-by: Caroline Desprat <cdesprat@icij.org>
6c42254 to
78961bf
Compare
Text, code, JSON, XML and HTML documents now open in a read-only code viewer with syntax highlighting, line numbers and search.
Preview