Skip to content

Commit 5ddb05e

Browse files
Just-InsaneJust-Insane
committed
fix: address review comments on room abbreviations PR
1 parent bf9deae commit 5ddb05e

4 files changed

Lines changed: 48 additions & 53 deletions

File tree

‎src/app/components/message/RenderBody.tsx‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -73,10 +73,9 @@ export function buildAbbrReplaceTextNode(
7373
if (!segments.some((s) => s.termKey !== undefined)) return undefined;
7474
return (
7575
<>
76-
{segments.map((seg, i) =>
76+
{segments.map((seg) =>
7777
seg.termKey !== undefined ? (
78-
// eslint-disable-next-line react/no-array-index-key
79-
<AbbreviationTerm key={i} text={seg.text} definition={abbrMap.get(seg.termKey) ?? ''} />
78+
<AbbreviationTerm key={seg.id} text={seg.text} definition={abbrMap.get(seg.termKey) ?? ''} />
8079
) : (
8180
seg.text
8281
)
@@ -114,17 +113,15 @@ export function RenderBody({
114113
if (segments.some((s) => s.termKey !== undefined)) {
115114
return (
116115
<>
117-
{segments.map((seg, i) => {
116+
{segments.map((seg) => {
118117
if (seg.termKey !== undefined) {
119118
const definition = abbrMap.get(seg.termKey) ?? '';
120119
return (
121-
// eslint-disable-next-line react/no-array-index-key
122-
<AbbreviationTerm key={i} text={seg.text} definition={definition} />
120+
<AbbreviationTerm key={seg.id} text={seg.text} definition={definition} />
123121
);
124122
}
125123
return (
126-
// eslint-disable-next-line react/no-array-index-key
127-
<Linkify key={i} options={linkifyOpts}>
124+
<Linkify key={seg.id} options={linkifyOpts}>
128125
{highlightRegex
129126
? highlightText(highlightRegex, scaleSystemEmoji(seg.text))
130127
: scaleSystemEmoji(seg.text)}

‎src/app/features/room-settings/abbreviations/RoomAbbreviations.tsx‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -253,8 +253,7 @@ export function RoomAbbreviations({ requestClose, isSpace }: AbbreviationsProps)
253253
<>
254254
{entries.map((entry, index) => (
255255
<SequenceCard
256-
// eslint-disable-next-line react/no-array-index-key
257-
key={`room-${index}`}
256+
key={entry.term}
258257
className={SequenceCardStyle}
259258
variant="SurfaceVariant"
260259
direction="Row"
@@ -287,10 +286,9 @@ export function RoomAbbreviations({ requestClose, isSpace }: AbbreviationsProps)
287286
))}
288287
{!isSpace &&
289288
ancestorGroups.flatMap(({ spaceId, spaceName, entries: spEntries }) =>
290-
spEntries.map((entry, index) => (
289+
spEntries.map((entry) => (
291290
<SequenceCard
292-
// eslint-disable-next-line react/no-array-index-key
293-
key={`${spaceId}-${index}`}
291+
key={`${spaceId}-${entry.term}`}
294292
className={SequenceCardStyle}
295293
variant="SurfaceVariant"
296294
direction="Row"

‎src/app/utils/abbreviations.test.ts‎

Lines changed: 30 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -60,96 +60,96 @@ describe('splitByAbbreviations', () => {
6060

6161
it('returns a single plain segment when the map is empty', () => {
6262
expect(splitByAbbreviations('hello FOSS world', new Map())).toEqual([
63-
{ text: 'hello FOSS world' },
63+
{ id: 'txt-0', text: 'hello FOSS world' },
6464
]);
6565
});
6666

6767
it('returns a single plain segment when there are no matches', () => {
68-
expect(splitByAbbreviations('hello world', map)).toEqual([{ text: 'hello world' }]);
68+
expect(splitByAbbreviations('hello world', map)).toEqual([{ id: 'txt-0', text: 'hello world' }]);
6969
});
7070

7171
it('returns a single plain segment for an empty string', () => {
72-
expect(splitByAbbreviations('', map)).toEqual([{ text: '' }]);
72+
expect(splitByAbbreviations('', map)).toEqual([{ id: 'txt-0', text: '' }]);
7373
});
7474

7575
it('splits a term match in the middle of a string', () => {
7676
expect(splitByAbbreviations('Use FOSS software', map)).toEqual([
77-
{ text: 'Use ' },
78-
{ text: 'FOSS', termKey: 'foss' },
79-
{ text: ' software' },
77+
{ id: 'txt-0', text: 'Use ' },
78+
{ id: 'txt-1', text: 'FOSS', termKey: 'foss' },
79+
{ id: 'txt-2', text: ' software' },
8080
]);
8181
});
8282

8383
it('splits a term match at the start of a string', () => {
8484
expect(splitByAbbreviations('FOSS is great', map)).toEqual([
85-
{ text: 'FOSS', termKey: 'foss' },
86-
{ text: ' is great' },
85+
{ id: 'txt-0', text: 'FOSS', termKey: 'foss' },
86+
{ id: 'txt-1', text: ' is great' },
8787
]);
8888
});
8989

9090
it('splits a term match at the end of a string', () => {
9191
expect(splitByAbbreviations('I love FOSS', map)).toEqual([
92-
{ text: 'I love ' },
93-
{ text: 'FOSS', termKey: 'foss' },
92+
{ id: 'txt-0', text: 'I love ' },
93+
{ id: 'txt-1', text: 'FOSS', termKey: 'foss' },
9494
]);
9595
});
9696

9797
it('handles multiple terms in the same string', () => {
9898
expect(splitByAbbreviations('FOSS and RTFM', map)).toEqual([
99-
{ text: 'FOSS', termKey: 'foss' },
100-
{ text: ' and ' },
101-
{ text: 'RTFM', termKey: 'rtfm' },
99+
{ id: 'txt-0', text: 'FOSS', termKey: 'foss' },
100+
{ id: 'txt-1', text: ' and ' },
101+
{ id: 'txt-2', text: 'RTFM', termKey: 'rtfm' },
102102
]);
103103
});
104104

105105
it('matches case-insensitively (termKey is always lowercase)', () => {
106106
expect(splitByAbbreviations('I like foss and Foss', map)).toEqual([
107-
{ text: 'I like ' },
108-
{ text: 'foss', termKey: 'foss' },
109-
{ text: ' and ' },
110-
{ text: 'Foss', termKey: 'foss' },
107+
{ id: 'txt-0', text: 'I like ' },
108+
{ id: 'txt-1', text: 'foss', termKey: 'foss' },
109+
{ id: 'txt-2', text: ' and ' },
110+
{ id: 'txt-3', text: 'Foss', termKey: 'foss' },
111111
]);
112112
});
113113

114114
it('does not match a term that is a prefix of a longer word (word boundary)', () => {
115115
// OSS is in the map, but should not match inside FOSS because the F provides no boundary
116116
const ossOnlyMap = buildAbbreviationsMap([{ term: 'OSS', definition: 'Open Source Software' }]);
117-
expect(splitByAbbreviations('FOSS rocks', ossOnlyMap)).toEqual([{ text: 'FOSS rocks' }]);
117+
expect(splitByAbbreviations('FOSS rocks', ossOnlyMap)).toEqual([{ id: 'txt-0', text: 'FOSS rocks' }]);
118118
});
119119

120120
it('does not match a term embedded inside a longer word', () => {
121121
// OSS should not match inside CROSS or GLOSS
122122
const ossOnlyMap = buildAbbreviationsMap([{ term: 'OSS', definition: 'Open Source Software' }]);
123123
expect(splitByAbbreviations('CROSS the GLOSS', ossOnlyMap)).toEqual([
124-
{ text: 'CROSS the GLOSS' },
124+
{ id: 'txt-0', text: 'CROSS the GLOSS' },
125125
]);
126126
});
127127

128128
it('matches a shorter term standalone when the longer overlapping term is also defined', () => {
129129
// OSS is a suffix of FOSS; when standalone it should still match
130130
expect(splitByAbbreviations('OSS is related to FOSS', map)).toEqual([
131-
{ text: 'OSS', termKey: 'oss' },
132-
{ text: ' is related to ' },
133-
{ text: 'FOSS', termKey: 'foss' },
131+
{ id: 'txt-0', text: 'OSS', termKey: 'oss' },
132+
{ id: 'txt-1', text: ' is related to ' },
133+
{ id: 'txt-2', text: 'FOSS', termKey: 'foss' },
134134
]);
135135
});
136136

137137
it('prefers the longer term when a shorter one is a suffix of it', () => {
138138
// FOSS contains OSS; FOSS should win and OSS should not be matched separately
139139
expect(splitByAbbreviations('Use FOSS today', map)).toEqual([
140-
{ text: 'Use ' },
141-
{ text: 'FOSS', termKey: 'foss' },
142-
{ text: ' today' },
140+
{ id: 'txt-0', text: 'Use ' },
141+
{ id: 'txt-1', text: 'FOSS', termKey: 'foss' },
142+
{ id: 'txt-2', text: ' today' },
143143
]);
144144
});
145145

146146
it('preserves plain text between two consecutive matches', () => {
147147
expect(splitByAbbreviations('FOSS, RTFM, OSS', map)).toEqual([
148-
{ text: 'FOSS', termKey: 'foss' },
149-
{ text: ', ' },
150-
{ text: 'RTFM', termKey: 'rtfm' },
151-
{ text: ', ' },
152-
{ text: 'OSS', termKey: 'oss' },
148+
{ id: 'txt-0', text: 'FOSS', termKey: 'foss' },
149+
{ id: 'txt-1', text: ', ' },
150+
{ id: 'txt-2', text: 'RTFM', termKey: 'rtfm' },
151+
{ id: 'txt-3', text: ', ' },
152+
{ id: 'txt-4', text: 'OSS', termKey: 'oss' },
153153
]);
154154
});
155155
});

‎src/app/utils/abbreviations.ts‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -26,13 +26,14 @@ export const buildAbbreviationsMap = (entries: AbbreviationEntry[]): Map<string,
2626
* plain segments and is the lowercase lookup key for abbreviation segments.
2727
*/
2828
export type TextSegment = {
29+
id: string;
2930
text: string;
3031
/** Undefined for plain text; the lowercase map key for an abbreviation. */
3132
termKey?: string;
3233
};
3334

3435
export const splitByAbbreviations = (text: string, abbrMap: Map<string, string>): TextSegment[] => {
35-
if (abbrMap.size === 0) return [{ text }];
36+
if (abbrMap.size === 0) return [{ id: 'txt-0', text }];
3637

3738
// Build a regex that matches any of the terms at word boundaries.
3839
// Sort longest first so "HTTP/2" matches before "HTTP".
@@ -42,20 +43,19 @@ export const splitByAbbreviations = (text: string, abbrMap: Map<string, string>)
4243

4344
const segments: TextSegment[] = [];
4445
let lastIndex = 0;
45-
let match: RegExpExecArray | null;
4646

47-
// eslint-disable-next-line no-cond-assign
48-
while ((match = pattern.exec(text)) !== null) {
49-
if (match.index > lastIndex) {
50-
segments.push({ text: text.slice(lastIndex, match.index) });
47+
for (const match of text.matchAll(pattern)) {
48+
const matchIndex = match.index!;
49+
if (matchIndex > lastIndex) {
50+
segments.push({ id: `txt-${segments.length}`, text: text.slice(lastIndex, matchIndex) });
5151
}
52-
segments.push({ text: match[0], termKey: match[0].toLowerCase() });
53-
lastIndex = match.index + match[0].length;
52+
segments.push({ id: `txt-${segments.length}`, text: match[0], termKey: match[0].toLowerCase() });
53+
lastIndex = matchIndex + match[0].length;
5454
}
5555

5656
if (lastIndex < text.length) {
57-
segments.push({ text: text.slice(lastIndex) });
57+
segments.push({ id: `txt-${segments.length}`, text: text.slice(lastIndex) });
5858
}
5959

60-
return segments.length > 0 ? segments : [{ text }];
60+
return segments.length > 0 ? segments : [{ id: 'txt-0', text }];
6161
};

0 commit comments

Comments
 (0)