Avatar and security fixes#221
Conversation
📝 WalkthroughWalkthroughThis PR refactors avatar and media visibility gating across the codebase. A new ChangesAvatar and Media Visibility Refactoring
IRC Metadata Handler Hardening
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/components/layout/ServerList.tsxOops! Something went wrong! :( ESLint: 9.39.1 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by src/components/layout/ChannelList.tsxOops! Something went wrong! :( ESLint: 9.39.1 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by src/components/message/EventMessage.tsxOops! Something went wrong! :( ESLint: 9.39.1 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Automated deployment preview for the PR in the Cloudflare Pages. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/components/message/CollapsedEventMessage.tsx (1)
33-36: 💤 Low valueConsider using a selector for
serverto ensure reactivity.
useStore.getState()performs a synchronous read that won't trigger re-renders ifserverschanges. Ifserver.filehostcould change during the component's lifetime, use a selector instead:const mediaSettings = mediaLevelToSettings( useStore((state) => state.globalSettings.mediaVisibilityLevel), ); -const server = useStore.getState().servers.find((s) => s.id === serverId); +const server = useStore((state) => state.servers.find((s) => s.id === serverId));That said, if
filehostis effectively immutable after connection, this is acceptable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/message/CollapsedEventMessage.tsx` around lines 33 - 36, The code uses useStore.getState().servers.find(...) to read the server synchronously which won't trigger re-renders; replace that with a reactive selector like useStore(state => state.servers.find(s => s.id === serverId)) so the component (CollapsedEventMessage) updates if the servers array or server.filehost changes — locate the server lookup in CollapsedEventMessage.tsx and swap the getState() call for the selector form using serverId.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/message/EventMessage.tsx`:
- Around line 34-36: The server lookup in EventMessage.tsx uses a snapshot read
via useStore.getState() which prevents reactive updates; change it to subscribe
to the store using useStore(...) so the component re-renders when
servers/filehost change. Replace the useStore.getState().servers.find(...)
access used for the server constant with a selector passed into useStore
(matching how mediaSettings is retrieved) that returns servers.find(s => s.id
=== message.serverId) to ensure avatar gating updates reactively.
In `@src/lib/irc/handlers/metadata.ts`:
- Around line 14-16: Add unit tests for the colon-stripping behavior in the
metadata handlers: create tests targeting handleMetadata and
handleMetadataWhoisKeyValue that assert (1) values without leading ":" are
preserved, (2) double-encoded values (e.g., "::value" and ":value" variants) are
normalized to a single unprefixed value, (3) empty trailing params (parv length
< 4 or last param equals ":" or "") produce empty strings, and (4) overall
behavior matches draft/metadata-2 examples; exercise the handlers by supplying
representative parv arrays and assert the final stored/returned metadata equals
the expected value for each case.
---
Nitpick comments:
In `@src/components/message/CollapsedEventMessage.tsx`:
- Around line 33-36: The code uses useStore.getState().servers.find(...) to read
the server synchronously which won't trigger re-renders; replace that with a
reactive selector like useStore(state => state.servers.find(s => s.id ===
serverId)) so the component (CollapsedEventMessage) updates if the servers array
or server.filehost changes — locate the server lookup in
CollapsedEventMessage.tsx and swap the getState() call for the selector form
using serverId.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6fe5f406-b6fd-4904-82c2-1b7f9999101f
📒 Files selected for processing (14)
src/components/layout/ChannelList.tsxsrc/components/layout/ChatHeader.tsxsrc/components/layout/MemberList.tsxsrc/components/layout/ServerList.tsxsrc/components/message/CollapsedEventMessage.tsxsrc/components/message/EventMessage.tsxsrc/components/message/MessageAvatar.tsxsrc/components/ui/ChannelListModal.tsxsrc/components/ui/ChannelSettingsModal.tsxsrc/components/ui/UserProfileModal.tsxsrc/lib/irc/handlers/metadata.tssrc/lib/ircUtils.tsxsrc/lib/mediaUtils.tstests/lib/mediaUtils.test.ts
| const server = useStore | ||
| .getState() | ||
| .servers.find((s) => s.id === message.serverId); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find non-reactive server lookups through Zustand snapshots
rg -nP --type=ts --type=tsx 'useStore\.getState\(\)\.servers\.find'Repository: ObsidianIRC/ObsidianIRC
Length of output: 95
🏁 Script executed:
cat -n src/components/message/EventMessage.tsx | head -60Repository: ObsidianIRC/ObsidianIRC
Length of output: 2181
🏁 Script executed:
#!/bin/bash
# Search for the file and its contents to understand context
find . -name "EventMessage.tsx" -type fRepository: ObsidianIRC/ObsidianIRC
Length of output: 109
🏁 Script executed:
#!/bin/bash
# Search for useStore usage patterns - use correct rg syntax
rg -n 'useStore\.getState\(\)' --type ts -A 3 -B 1 | head -50Repository: ObsidianIRC/ObsidianIRC
Length of output: 3193
🏁 Script executed:
#!/bin/bash
# Search for useStore with selector pattern to see best practices in codebase
rg -n 'useStore\(\(' --type ts -A 2 -B 1 | head -50Repository: ObsidianIRC/ObsidianIRC
Length of output: 3824
🏁 Script executed:
cat -n src/components/message/EventMessage.tsx | tail -50Repository: ObsidianIRC/ObsidianIRC
Length of output: 2268
🏁 Script executed:
rg -n 'server' src/components/message/EventMessage.tsx -A 1 -B 1Repository: ObsidianIRC/ObsidianIRC
Length of output: 758
Use a reactive selector for server lookup.
Line 34 uses useStore.getState() (snapshot read), so avatar gating won't update when servers/filehost changes. Subscribe via useStore(...) instead, matching the pattern used for mediaSettings on line 32.
Suggested fix
- const server = useStore
- .getState()
- .servers.find((s) => s.id === message.serverId);
+ const server = useStore((state) =>
+ state.servers.find((s) => s.id === message.serverId),
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const server = useStore | |
| .getState() | |
| .servers.find((s) => s.id === message.serverId); | |
| const server = useStore((state) => | |
| state.servers.find((s) => s.id === message.serverId), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/message/EventMessage.tsx` around lines 34 - 36, The server
lookup in EventMessage.tsx uses a snapshot read via useStore.getState() which
prevents reactive updates; change it to subscribe to the store using
useStore(...) so the component re-renders when servers/filehost change. Replace
the useStore.getState().servers.find(...) access used for the server constant
with a selector passed into useStore (matching how mediaSettings is retrieved)
that returns servers.find(s => s.id === message.serverId) to ensure avatar
gating updates reactively.
| // Strip leading ":" — some clients double-encode the trailing param, leaving one colon after the parser strips its own. | ||
| const rawValue = parv.length >= 4 ? parv[parv.length - 1] : ""; | ||
| const value = rawValue.startsWith(":") ? rawValue.substring(1) : rawValue; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for tests covering metadata handler colon-stripping behavior
# Test: Look for test files covering metadata handlers
fd -e test.ts -e test.tsx -e spec.ts . | xargs rg -l 'metadata' -i
# Test: Search for any tests calling handleMetadata or testing colon behavior
rg -n "handleMetadata|double.*encod|leading.*colon|startsWith.*:" --type ts -g '*test*' -g '*spec*'Repository: ObsidianIRC/ObsidianIRC
Length of output: 931
🏁 Script executed:
#!/bin/bash
# Search for IRC handler test files
echo "=== Looking for IRC handler test structure ==="
find tests -type d -name "handlers" -o -name "irc" 2>/dev/null | head -20
echo ""
echo "=== All test files in tests/lib ==="
fd -type f . tests/lib/ 2>/dev/null | head -20
echo ""
echo "=== Searching for any reference to handleMetadata in tests ==="
rg "handleMetadata" tests/ --type ts --type tsx 2>/dev/null | head -20
echo ""
echo "=== Checking handlers index to see registration ==="
grep -n "metadata" src/lib/irc/handlers/index.ts 2>/dev/null | head -10Repository: ObsidianIRC/ObsidianIRC
Length of output: 370
🏁 Script executed:
#!/bin/bash
echo "=== Checking full metadata.ts implementation (lines 1-50) ==="
head -50 src/lib/irc/handlers/metadata.ts | cat -n
echo ""
echo "=== Checking handlers/index.ts dispatch for metadata ==="
grep -B2 -A2 "METADATA\|metadata" src/lib/irc/handlers/index.ts | head -30
echo ""
echo "=== Looking for any integration tests or test patterns ==="
find tests -type f -name "*.test.ts" -o -name "*.test.tsx" | wc -l
echo ""
echo "=== Check if there are handler-related tests elsewhere ==="
rg "ctx\.triggerEvent|IRCClientContext" tests/ --type ts --type tsx 2>/dev/null | head -10Repository: ObsidianIRC/ObsidianIRC
Length of output: 2749
Add test coverage for colon-stripping edge cases in metadata handlers.
The logic correctly handles double-encoded trailing parameters by conditionally stripping leading colons. This implementation is consistent across handleMetadata (lines 14–16), handleMetadataWhoisKeyValue (lines 37–40), and aligns with the draft/metadata-2 specification.
However, no test coverage exists for this behavior. Recommend adding tests to verify:
- Normal values without leading colons are preserved
- Double-encoded values with extra colons are correctly stripped
- Empty values are handled properly
- Behavior matches the draft/metadata-2 specification
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/irc/handlers/metadata.ts` around lines 14 - 16, Add unit tests for
the colon-stripping behavior in the metadata handlers: create tests targeting
handleMetadata and handleMetadataWhoisKeyValue that assert (1) values without
leading ":" are preserved, (2) double-encoded values (e.g., "::value" and
":value" variants) are normalized to a single unprefixed value, (3) empty
trailing params (parv length < 4 or last param equals ":" or "") produce empty
strings, and (4) overall behavior matches draft/metadata-2 examples; exercise
the handlers by supplying representative parv arrays and assert the final
stored/returned metadata equals the expected value for each case.
2 participants
Summary by CodeRabbit
Bug Fixes
Refactor