Conversation
- Add blocks?: unknown[] field to SlackMessage interface - Add message_update to SimulatorEvent type union - chatPostMessage: accept blocks, auto-generate block_ids, allow text or blocks - chatUpdate: accept blocks, update on message, emit message_update SSE event
…iminators - handleSimulatorBlockAction supports modal (view_id) and message (message_ts + channel_id) contexts - Action objects include element-specific type, block_id, and value fields - Container field included in block_actions payloads for both contexts - dispatchInteractive accepts container, channel, message in payload type - addTypeDiscriminators adds per-element type to state.values in view submissions
- Add blocks field to SimulatorMessage interface - Add updateMessage function for message_update SSE events - Wire blocks through message and file_shared SSE handlers - Add message_update SSE case to handle chat.update events - Add sendMessageBlockAction for message-context block actions
- Conditionally render BlockKitRenderer when message has blocks - Show text as muted fallback when blocks are present - Add resolveActionFromBlocks helper to identify element type and build payload - Wire handleMessageBlockAction to sendMessageBlockAction dispatcher - Add static_select accessory support to SectionBlock
…bmission format parseBody now JSON-parses stringified arrays/objects in form-urlencoded data, fixing blocks and view payloads sent by Slack's Web API client. addTypeDiscriminators restructures static_select values into selected_option format matching real Slack's view_submission payloads.
… add shared mrkdwn CSS - Replace hand-rolled bold-only regex with slack-markdown toHTML() + DOMPurify.sanitize() - Add plain_text vs mrkdwn type discrimination (plain_text is escaped, not parsed) - Define SANITIZE_CONFIG with allowed tags for all mrkdwn output elements - Add shared .mrkdwn CSS class in app.css for code, pre, links, blockquotes, mentions, emoji
- Add .mrkdwn wrapper class to SectionBlock text and fields elements - Add .mrkdwn wrapper class to ContextBlock text elements - Switch Checkboxes from renderText to renderMrkdwn with .mrkdwn wrapper - Refactor Message.svelte to use renderMrkdwn instead of direct toHTML + DOMPurify - Remove duplicate mrkdwn CSS from Message.svelte (now in app.css), keep message-specific styles
…argins Post-process slack-markdown output to convert bullet (•) and numbered (1. 2. 3.) patterns to proper <ul>/<ol> HTML, strip leading newline from code blocks, and collapse <br> around block elements. Change SectionBlock wrapper from <p> to <div> to allow block-level children.
… markdownToMrkdwn - Custom Slack mrkdwn to HTML parser handling bold, italic, strike, code, links, mentions, lists, blockquotes, emoji - Markdown to Slack mrkdwn converter using marked lexer for LLM output conversion - 45 passing tests covering both conversion directions and edge cases
- Replace slack-markdown import with @botarium/mrkdwn in context.ts - Remove convertLists() and all post-processing workarounds (now handled by mrkdwnToHtml) - Add @botarium/mrkdwn workspace dependency, remove slack-markdown from deps
…s in mrkdwn parser
…bles, horizontal rules
The CSS `br + br` selector ignores text nodes, so it was hiding every
`<br>` after the first one — not just doubled paragraph breaks. Replace
`<br><br>` with a block-level `<span class="p-gap">` spacer and remove
the `br + br { display: none }` rule so single `<br>` tags render
correctly between task list items and other line-by-line content.
- SlackConfirmDialog composition object - SlackOverflowOption with url support - SlackRadioButtonsElement, SlackNumberInputElement, SlackEmailInputElement, SlackUrlInputElement - Added confirm property to Button, StaticSelect, Checkboxes, Overflow elements - Updated SlackBlockElement and SlackInputElement unions
…g and backend - OverflowMenu: vertical dots trigger, click-outside dismissal, URL option support - RadioButtonGroup: labeled radio options following Checkboxes pattern - ActionsBlock: dispatch overflow and radio_buttons element types - SectionBlock: render overflow and radio_buttons as accessories - Message.svelte: buildActionValue handles overflow/radio_buttons selected_option shape - web-api.ts: handleSimulatorBlockAction dispatches selected_option for overflow/radio_buttons
- NumberInput uses native <input type="number"> with step/min/max from element props - EmailInput defaults placeholder to "name@example.com" when bot provides none - UrlInput defaults placeholder to "https://example.com" when bot provides none - All three use PlainTextInput styling for visual consistency
- InputBlock renders number_input, email_text_input, url_text_input, radio_buttons - BlockKitRenderer passes onRadioChange prop through to InputBlock - ModalOverlay extracts initial values and tracks state for all new types - Backend addTypeDiscriminators restructures radio_buttons to selected_option format
- Centered modal overlay with backdrop dismiss and Escape key handling - Title, mrkdwn-capable text body, styled confirm/deny buttons - Supports style property (danger=red, primary/default=green)
- Button: intercepts click, shows confirm before dispatching action - StaticSelect: intercepts change, shows confirm before dispatching - Checkboxes: intercepts toggle, shows confirm before dispatching - OverflowMenu: closes dropdown first, shows confirm before action/URL - RadioButtonGroup: intercepts change, shows confirm before dispatching - All use shared pendingAction/showingConfirm pattern
…ipping Dropdown was getting clipped by ancestor overflow:hidden containers. Now uses fixed positioning calculated from trigger button's viewport rect, with auto-detection for opening above vs below.
- Add SlackDatePickerElement, SlackTimePickerElement, SlackDateTimePickerElement type interfaces - Update SlackBlockElement union with datepicker and timepicker - Update SlackInputElement union with all three picker types - Create DatePicker.svelte with bits-ui Calendar, dark theme, fixed positioning - Create TimePicker.svelte with bits-ui TimeField, 24-hour format - Both components support initial values and confirm dialog pattern
… backend - Wire DatePicker and TimePicker into ActionsBlock, SectionBlock, InputBlock - Add extractInitialValues for datepicker, timepicker, datetimepicker in ModalOverlay - Add selected_date/selected_time action value branches in Message.svelte - Extend sendMessageBlockAction actionValue type with picker fields in dispatcher - Add datepicker/timepicker/datetimepicker handling in handleSimulatorBlockAction - Add picker type discriminators in addTypeDiscriminators for view_submission
- Compose bits-ui Calendar and TimeField in a single dropdown panel - Parse UNIX timestamps to CalendarDate + Time, combine on Done click - Fixed-position dropdown with click-outside/Escape dismissal - Support initial_date_time, confirm dialog, compact mode
…nd message dispatch - Add datetimepicker branch to ActionsBlock and InputBlock - Add selected_date_time (number) to Message.svelte buildActionValue - Update resolveActionFromBlocks return type with selected_date_time - Add SlackDateTimePickerElement to SlackBlockElement union type
- Add explicit width and horizontal overflow detection to DatePicker positionMenu() - Add explicit width and horizontal overflow detection to DateTimePicker positionMenu() - Add max-w-[250px] to StaticSelect in compact mode - Add items-start alignment to ActionsBlock flex container
- Replace direct TimeField with trigger button + popover dropdown - Add Clock icon trigger matching DatePicker/DateTimePicker pattern - Wire onValueChange to local state only, dispatch on Done click - Add click-outside and Escape handlers for popover dismissal - Add fixed positioning with horizontal overflow detection - Confirm dialog gating preserved on Done click - Props interface unchanged (element, value, onChange, compact)
Wrap inline emojis in a tooltip span structure so hovering displays a popover with a large emoji and its :shortcode:, matching Slack behavior.
…acing Replace <br> tags with <span class="c-mrkdwn__br"> matching Slack's Block Kit markup. Collapses consecutive breaks into one, prevents margin collapse in BlockKitRenderer with flex layout, and removes the now-unnecessary p-gap post-processing.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
apps/showcase-bot/src/listeners/actions/showcase-actions.ts (1)
8-48: Extractaction_idto avoid repeated type assertions.The
actionparameter is cast to{ action_id: string }three times (lines 26, 32, 46). Extracting once at the top improves readability and reduces repetition.♻️ Suggested refactor
app.action(/^showcase_/, async ({ action, ack, body, client }) => { await ack() + const actionId = (action as { action_id: string }).action_id + // Build a readable summary of the action payload const actionSummary = JSON.stringify(action, null, 2) @@ -24,15 +26,15 @@ await client.chat.postMessage({ channel, - text: `Action: ${(action as { action_id: string }).action_id}`, + text: `Action: ${actionId}`, blocks: [ { type: 'section', text: { type: 'mrkdwn', - text: `*Received \`block_actions\`* from \`${(action as { action_id: string }).action_id}\``, + text: `*Received \`block_actions\`* from \`${actionId}\``, }, }, @@ -45,7 +47,7 @@ }) slackLogger.info( - { actionId: (action as { action_id: string }).action_id }, + { actionId }, 'Echoed block_action' ) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/showcase-bot/src/listeners/actions/showcase-actions.ts` around lines 8 - 48, Extract the action_id once from the action payload to avoid repeated type assertions: inside the app.action handler (the function starting with app.action(/^showcase_/, ...)), add a single const like actionId = (action as { action_id: string }).action_id and replace the three occurrences of (action as { action_id: string }).action_id with actionId in the client.chat.postMessage text/blocks and in slackLogger.info; keep the existing displaySummary and channel logic unchanged.apps/ui/src/app.css (1)
330-369: Consider using CSS variables for theme consistency.Several hardcoded colour values could be replaced with existing CSS variables to ensure consistency across themes and easier maintenance:
- Line 337:
#e6902cfor inline code could use a defined variable- Line 357:
#1d9bd1for links differs slightly from--color-slack-accent(#1264a3)- Line 366:
#dddfor blockquote border is a light colour that may not suit dark mode wellThe file already defines comprehensive colour variables (e.g.,
--text-primary,--color-slack-accent,--border-color). Using them here would improve theme coherence.Example adjustments
.mrkdwn code { background: `#8881`; border: 1px solid `#8883`; border-radius: 3px; padding: 2px 4px; font-family: var(--font-mono); font-size: 12px; - color: `#e6902c`; + color: var(--color-log-module); /* or define a --color-code variable */ } .mrkdwn a { - color: `#1d9bd1`; + color: var(--color-slack-accent); text-decoration: none; } .mrkdwn blockquote { - border-left: 4px solid `#ddd`; + border-left: 4px solid var(--border-color); padding-left: 12px; color: var(--text-secondary); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/app.css` around lines 330 - 369, Replace hardcoded colors in the .mrkdwn styles with theme CSS variables for consistency: swap the inline code color (`#e6902c`) in .mrkdwn code for an existing or new variable (e.g., --inline-code-color) with a fallback to --text-primary; replace the link color (`#1d9bd1`) in .mrkdwn a with --color-slack-accent (or a new --link-color) and ensure :hover still underlines; and replace the blockquote border color (`#ddd`) with --border-color or a new --blockquote-border variable that adapts for dark mode. Update .mrkdwn pre code/background borders similarly to use --text-primary and --border-color so all these selectors (.mrkdwn code, .mrkdwn a, .mrkdwn blockquote, .mrkdwn pre code) pull colors from theme variables with sensible fallbacks.apps/ui/src/components/blockkit/blocks/SectionBlock.svelte (1)
131-131: Consider omittingselectedOptions={undefined}.Explicitly passing
undefinedis redundant if the prop defaults toundefinedin theCheckboxescomponent. This can be safely removed for cleaner code, unless there's a specific reason to override a non-undefined default.♻️ Suggested simplification
<Checkboxes element={cb} - selectedOptions={undefined} onChange={(options) =>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/blockkit/blocks/SectionBlock.svelte` at line 131, The JSX/Svelte fragment is passing selectedOptions={undefined} to the Checkboxes component in SectionBlock.svelte; remove that explicit prop so the component relies on its default (omit the selectedOptions attribute entirely), unless Checkboxes declares a different non-undefined default—in which case either set the intended default inside Checkboxes or pass the real default value instead; update any tests or call sites that relied on the explicit undefined if necessary.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
apps/showcase-bot/src/listeners/actions/showcase-actions.tsapps/ui/src/app.cssapps/ui/src/components/Message.svelteapps/ui/src/components/Sidebar.svelteapps/ui/src/components/blockkit/BlockKitRenderer.svelteapps/ui/src/components/blockkit/blocks/ContextActionsBlock.svelteapps/ui/src/components/blockkit/blocks/ContextBlock.svelteapps/ui/src/components/blockkit/blocks/DividerBlock.svelteapps/ui/src/components/blockkit/blocks/SectionBlock.svelteapps/ui/src/components/blockkit/blocks/TableBlock.svelteapps/ui/src/components/blockkit/elements/Button.svelteapps/ui/src/components/blockkit/elements/ConfirmDialog.svelteapps/ui/src/components/blockkit/elements/DateTimePicker.svelteapps/ui/src/lib/components/ui/button/button-variants.tsapps/ui/src/lib/components/ui/button/button.svelteapps/ui/src/lib/components/ui/button/index.tsapps/ui/src/lib/dispatcher.svelte.tsapps/ui/src/lib/types.tspackages/mrkdwn/src/mrkdwn-to-html.test.tspackages/mrkdwn/src/mrkdwn-to-html.tstsconfig.json
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/ui/src/lib/components/ui/button/button-variants.ts
- apps/ui/src/components/blockkit/elements/ConfirmDialog.svelte
- apps/ui/src/components/blockkit/blocks/TableBlock.svelte
- apps/ui/src/lib/components/ui/button/index.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-14T21:36:07.933Z
Learnt from: tyom
Repo: tyom/botarium PR: 1
File: apps/ui/src/components/MessagePanel.svelte:10-23
Timestamp: 2026-01-14T21:36:07.933Z
Learning: In the client-only Botarium UI (Vite + Svelte), do not add or rely on SSR-specific guards (e.g., checks for sessionStorage) in components. Since the app runs only in Electron or the browser, code that accesses browser APIs will execute in a non-SSR environment, so SSR-unsafe checks are unnecessary and can be removed for maintainability.
Applied to files:
apps/ui/src/components/blockkit/BlockKitRenderer.svelteapps/ui/src/components/blockkit/blocks/SectionBlock.svelteapps/ui/src/components/blockkit/blocks/ContextActionsBlock.svelteapps/ui/src/components/blockkit/blocks/ContextBlock.svelteapps/ui/src/components/Message.svelteapps/ui/src/components/blockkit/elements/Button.svelteapps/ui/src/components/Sidebar.svelteapps/ui/src/lib/components/ui/button/button.svelteapps/ui/src/components/blockkit/elements/DateTimePicker.svelteapps/ui/src/components/blockkit/blocks/DividerBlock.svelte
🧬 Code graph analysis (2)
packages/mrkdwn/src/mrkdwn-to-html.ts (1)
packages/mrkdwn/src/index.ts (1)
mrkdwnToHtml(1-1)
packages/mrkdwn/src/mrkdwn-to-html.test.ts (1)
packages/mrkdwn/src/mrkdwn-to-html.ts (1)
mrkdwnToHtml(147-257)
🪛 ast-grep (0.40.5)
packages/mrkdwn/src/mrkdwn-to-html.ts
[warning] 225-225: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((${BR_RE.source}){2,}, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 229-229: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((${BR_RE.source})+(<(?:ul|ol|blockquote|pre)>), 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 233-233: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((<\\/(?:ul|ol|blockquote|pre)>)(${BR_RE.source})+, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 238-238: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((${BR_RE.source})+(\uE000CB\\d+\uE000), 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 242-242: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((\uE000CB\\d+\uE000)(${BR_RE.source})+, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 252-252: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((${BR_RE.source})+(<pre>), 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 253-253: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((<\\/pre>)(${BR_RE.source})+, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 Stylelint (17.3.0)
apps/ui/src/app.css
[error] 260-260: Unexpected missing scoping root (nesting-selector-no-missing-scoping-root)
(nesting-selector-no-missing-scoping-root)
[error] 264-264: Unexpected missing scoping root (nesting-selector-no-missing-scoping-root)
(nesting-selector-no-missing-scoping-root)
[error] 257-257: Unexpected unknown at-rule "@Utility" (scss/at-rule-no-unknown)
(scss/at-rule-no-unknown)
🔇 Additional comments (20)
apps/ui/src/components/blockkit/blocks/DividerBlock.svelte (1)
1-1: Looks good — clean presentational tweak.The added spacing and width constraint align with the layout refinements described in the PR and don’t affect behaviour.
apps/ui/src/components/blockkit/blocks/ContextBlock.svelte (1)
16-22: Styling and mrkdwn hook look good.The updated spacing/width and the
mrkdwnclass wrapper are clean, non-breaking presentation tweaks.tsconfig.json (1)
20-21: LGTM!The
excludeconfiguration is valid and sensible for a mono-repo setup. Excluding template and temporary directories from TypeScript compilation prevents unnecessary type-checking of non-source files and keeps the build clean.apps/ui/src/lib/components/ui/button/button.svelte (1)
1-14: Clean refactoring to centralise button variants.The restructuring to import
ButtonPropsandbuttonVariantsfrom a dedicated module improves maintainability and aligns with the broader pattern of modular public API surfaces noted in the PR. The Svelte 5 syntax ($props(),$bindable(),{@render}) is used correctly, and the component's polymorphic behaviour (anchor vs button) with accessible disabled state handling remains intact.apps/showcase-bot/src/listeners/actions/showcase-actions.ts (2)
1-4: LGTM!Clean imports and appropriate constant definition for the showcase channel identifier.
51-84: LGTM!The view submission handler correctly acknowledges, summarises the state values, and echoes to the showcase channel. The higher truncation limit (2000 vs 1500) for modal submissions is sensible given they typically contain more form data.
apps/ui/src/app.css (1)
256-267: LGTM! Static analysis warnings are false positives.The
@utilitydirective is a valid Tailwind CSS v4 feature that provides proper scoping for nested selectors. The Stylelint errors (nesting-selector-no-missing-scoping-rootandscss/at-rule-no-unknown) stem from the linter not recognising Tailwind v4 syntax. The implementation is consistent with existing utilities likepanel-tabandmodule-togglein this file.Consider updating the Stylelint configuration to ignore
@utilityif these warnings become noisy.apps/ui/src/components/blockkit/blocks/SectionBlock.svelte (1)
154-168: LGTM!The local styles are appropriately scoped and follow the existing BEM-like naming convention. The layout properties are reasonable for the section block structure.
packages/mrkdwn/src/mrkdwn-to-html.ts (1)
150-255: Block-level processing and placeholder restoration look solid.packages/mrkdwn/src/mrkdwn-to-html.test.ts (1)
6-181: Test coverage is thorough and well‑structured.apps/ui/src/components/Sidebar.svelte (2)
116-127: Create‑channel CTA and modal wiring look tidy.Also applies to: 235-240
55-87: Context‑menu delete flow is clear and scoped to custom channels.Also applies to: 131-175, 242-254
apps/ui/src/lib/types.ts (1)
79-565: Type additions cover the new Block Kit surface area neatly.apps/ui/src/components/blockkit/elements/Button.svelte (1)
13-56: Confirm‑dialog flow is well contained and resets state cleanly.apps/ui/src/components/blockkit/elements/DateTimePicker.svelte (1)
28-149: Date/time picker composition and confirm handling read cleanly.apps/ui/src/lib/dispatcher.svelte.ts (1)
621-703: Channel CRUD integration with state and logging looks solid.apps/ui/src/components/blockkit/blocks/ContextActionsBlock.svelte (1)
21-57: Context actions render and dispatch action IDs clearly.apps/ui/src/components/blockkit/BlockKitRenderer.svelte (1)
71-105: Renderer routing for rich_text, table and context_actions blocks is straightforward.apps/ui/src/components/Message.svelte (2)
371-387: BlockKitRenderer wiring and richer image‑preview context look good.
94-105: No action needed—sanitisation is properly implemented.
renderMrkdwninapps/ui/src/components/blockkit/context.tsalready sanitises its output via DOMPurify with a restrictive whitelist (lines 57–60). Plain text is HTML-escaped, and mrkdwn is parsed then sanitised before being returned for use with{@html}. The XSS vector does not exist.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ui/src/app.css`:
- Around line 401-443: The emoji tooltip CSS (.mrkdwn .s-emoji-tip, .mrkdwn
.s-emoji-tip::after, .mrkdwn .s-emoji-code) uses three undefined variables
--color-surface-container-high, --color-outline-variant, and
--color-on-surface-variant; either add these variables to the global design
system (e.g., define them on :root with the intended colour values) or replace
their usages in those selectors with the intended fallback literals (e.g.,
`#1a1d21`, `#38393d`, `#ababad`) so the tooltip renders consistently; update both the
background, border/border-top-color, and text color references accordingly.
In `@apps/ui/src/components/blockkit/blocks/SectionBlock.svelte`:
- Around line 98-102: The WorkspaceSelect component is missing an onChange prop
so action callbacks (action_id on SlackWorkspaceSelectElement) never fire;
update the WorkspaceSelect component to accept an onChange handler prop and
invoke it when the user makes a selection, then pass the block accessory's
action_id and the selection payload into that handler from SectionBlock (where
ws = block.accessory as SlackWorkspaceSelectElement), and mirror the same wiring
in ActionsBlock and InputBlock so selections dispatch the parent callback
consistently.
In `@apps/ui/src/components/Message.svelte`:
- Around line 195-262: The resolveActionFromBlocks function misses handling for
'context_actions' blocks, causing those elements to fall through to the
fallback; add a branch like the existing actions handling that checks if
block.type === 'context_actions', casts to SlackContextActionsBlock, iterates
its elements, and when an element with matching action_id is found (elements
like SlackFeedbackButtonsElement or SlackIconButtonElement), call the same
buildActionValue(blockId, el, value) and return that result so the correct
blockId and elementType are preserved.
In `@apps/ui/src/components/Sidebar.svelte`:
- Around line 46-53: handleCreateChannel currently calls the async addChannel
without awaiting it and unconditionally sets showCreateModal = false; change it
to await the Promise returned by addChannel(name), check the returned value
(e.g., !== null) and only set showCreateModal = false on success, otherwise keep
the modal open and set an error state (e.g., createError or similar) so the UI
can surface the failure; update handleCreateChannel to be async and
handle/rethrow errors from addChannel appropriately.
In `@packages/mrkdwn/src/mrkdwn-to-html.ts`:
- Around line 81-87: The current regexes in the text replace calls in
mrkdwn-to-html.ts break when `escapeHtml` has turned `&` into `&` because
they use `[^&]+`; update both replacements to stop matching at the delimiters
(`|` or `>`) instead of excluding `&`. Concretely, change the first regex
(the one producing '<a href="$1" target="_blank">$2</a>') to allow any
characters for the URL and label up to the `|` and `>` respectively (e.g. use
non-greedy match like ([\s\S]*?) for the label and for the URL match up to
`\|`), and change the second regex (the one producing '<a href="$1"
target="_blank">$1</a>') to use a non-greedy ([\s\S]*?) for the URL up to
`>`; update the two text.replace(...) calls accordingly so ampersands inside
URLs/labels are accepted.
---
Duplicate comments:
In `@packages/mrkdwn/src/mrkdwn-to-html.test.ts`:
- Around line 35-50: Update the test expectations in mrkdwn-to-html.test.ts to
assert that anchors produced by mrkdwnToHtml include rel="noopener noreferrer"
in addition to target="_blank"; specifically update the expected strings for the
cases using mrkdwnToHtml('<https://example.com|Example>'),
mrkdwnToHtml('<https://example.com>'), and
mrkdwnToHtml('<mailto:test@example.com|Email>') so the expected <a ...> tags
contain rel="noopener noreferrer" alongside target="_blank".
In `@packages/mrkdwn/src/mrkdwn-to-html.ts`:
- Around line 80-88: The anchor replacements in mrkdwn-to-html.ts (the two
text.replace calls that convert /<((?:https?|mailto):[^|&]+)\|([^&]+)>/
and /<((?:https?|mailto):[^&]+)>/ to '<a href="..." target="_blank">...')
must include rel="noopener noreferrer"; update both replacement strings to add
rel="noopener noreferrer" alongside target="_blank" so external links use rel
for security.
---
Nitpick comments:
In `@apps/showcase-bot/src/listeners/actions/showcase-actions.ts`:
- Around line 8-48: Extract the action_id once from the action payload to avoid
repeated type assertions: inside the app.action handler (the function starting
with app.action(/^showcase_/, ...)), add a single const like actionId = (action
as { action_id: string }).action_id and replace the three occurrences of (action
as { action_id: string }).action_id with actionId in the client.chat.postMessage
text/blocks and in slackLogger.info; keep the existing displaySummary and
channel logic unchanged.
In `@apps/ui/src/app.css`:
- Around line 330-369: Replace hardcoded colors in the .mrkdwn styles with theme
CSS variables for consistency: swap the inline code color (`#e6902c`) in .mrkdwn
code for an existing or new variable (e.g., --inline-code-color) with a fallback
to --text-primary; replace the link color (`#1d9bd1`) in .mrkdwn a with
--color-slack-accent (or a new --link-color) and ensure :hover still underlines;
and replace the blockquote border color (`#ddd`) with --border-color or a new
--blockquote-border variable that adapts for dark mode. Update .mrkdwn pre
code/background borders similarly to use --text-primary and --border-color so
all these selectors (.mrkdwn code, .mrkdwn a, .mrkdwn blockquote, .mrkdwn pre
code) pull colors from theme variables with sensible fallbacks.
In `@apps/ui/src/components/blockkit/blocks/SectionBlock.svelte`:
- Line 131: The JSX/Svelte fragment is passing selectedOptions={undefined} to
the Checkboxes component in SectionBlock.svelte; remove that explicit prop so
the component relies on its default (omit the selectedOptions attribute
entirely), unless Checkboxes declares a different non-undefined default—in which
case either set the intended default inside Checkboxes or pass the real default
value instead; update any tests or call sites that relied on the explicit
undefined if necessary.
Show a Slack-style horizontal line with centered date label (Today, Yesterday, or full date) between messages from different days. Also add full date tooltip on message timestamps.
Show a persistent about section with large avatar, bot name, APP badge, status dot, and description at the top of every bot DM. Also move the channel options dropdown outside the header to fix click interception.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ui/src/components/Message.svelte (1)
89-110:⚠️ Potential issue | 🟡 MinorEscape
userNamewhen injecting into HTML span to prevent XSS.The
renderMrkdwnfunction correctly sanitises output via DOMPurify before returning. However, after sanitisation,userNameis inserted directly into the HTML span at lines 105–108 without entity escaping. IfsimulatorState.simulatedUserNamecontains HTML special characters, this could create an XSS vulnerability. Apply HTML entity escaping touserName:const escapedDisplayName = userName .replace(/&/g, '&') .replace(/</g, '<') .replace(/>/g, '>') .replace(/"/g, '"') .replace(/'/g, ''') html = html.replace( mentionRegex, `<span class="slack-mention">@${escapedDisplayName}</span>` )Also applies to: 395–399 (same usage pattern).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/Message.svelte` around lines 89 - 110, The code builds HTML by inserting userName directly into a replacement span in the formattedText derived value (see variables userName, escapedUserName, mentionRegex and the html.replace call), which can lead to XSS if simulatorState.simulatedUserName contains special characters; fix by HTML-escaping the display name (escape &, <, >, ", ') into a new escapedDisplayName and use that escapedDisplayName in the replacement string while keeping mentionRegex (which should still escape regex metacharacters as currently done); apply the same change to the other identical usage site that wraps `@mentions` in a span (the same pattern elsewhere in the file).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/showcase-bot/config.yamlapps/showcase-bot/src/config/loader.tsapps/ui/src/components/BotAboutHeader.svelteapps/ui/src/components/DaySeparator.svelteapps/ui/src/components/Message.svelteapps/ui/src/components/MessagePanel.svelteapps/ui/src/lib/dispatcher.svelte.tsapps/ui/src/lib/time.tsapps/ui/src/lib/types.tspackages/create-bot/templates/slack-bot/config.yaml.tmplpackages/slack/src/server/types.tspackages/slack/src/server/web-api.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/showcase-bot/config.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-14T21:36:07.933Z
Learnt from: tyom
Repo: tyom/botarium PR: 1
File: apps/ui/src/components/MessagePanel.svelte:10-23
Timestamp: 2026-01-14T21:36:07.933Z
Learning: In the client-only Botarium UI (Vite + Svelte), do not add or rely on SSR-specific guards (e.g., checks for sessionStorage) in components. Since the app runs only in Electron or the browser, code that accesses browser APIs will execute in a non-SSR environment, so SSR-unsafe checks are unnecessary and can be removed for maintainability.
Applied to files:
apps/ui/src/components/BotAboutHeader.svelteapps/ui/src/components/MessagePanel.svelteapps/ui/src/components/Message.svelteapps/ui/src/components/DaySeparator.svelte
🧬 Code graph analysis (3)
apps/ui/src/lib/dispatcher.svelte.ts (2)
apps/ui/src/lib/types.ts (2)
SlackBlock(135-145)Channel(28-33)apps/ui/src/lib/state.svelte.ts (5)
setChannels(202-205)addChannelToState(208-210)switchChannel(272-277)removeChannelFromState(213-220)simulatorState(35-70)
packages/slack/src/server/web-api.ts (4)
apps/electron/model-fetcher.js (4)
result(528-528)text(297-297)text(347-347)text(394-394)packages/slack/src/lib/logger.ts (1)
webApiLogger(34-34)packages/slack/src/server/index.ts (1)
message(549-551)packages/slack/src/server/types.ts (3)
SlackMessage(52-67)ChatPostMessageRequest(260-266)ViewsOpenRequest(171-174)
apps/ui/src/lib/types.ts (1)
packages/slack/src/server/types.ts (2)
SlackFile(35-50)SlackViewTextObject(143-147)
🔇 Additional comments (29)
packages/create-bot/templates/slack-bot/config.yaml.tmpl (1)
31-38: Solid addition ofbot_descriptionschema metadata.Clear label/description and grouping are consistent with the existing settings structure.
apps/showcase-bot/src/config/loader.ts (1)
98-109: Nice convenience export for bot metadata.Deriving
botName/botDescriptionand folding them intoslackConfigkeeps emulator registration tidy.apps/ui/src/lib/time.ts (1)
18-67: Clear date helpers for separators and tooltips.
These additions are straightforward and align with the existing timestamp utilities.apps/ui/src/components/DaySeparator.svelte (1)
1-17: Neat, self‑contained separator component.
Clean structure and styling, easy to reuse.packages/slack/src/server/types.ts (1)
52-67: Type extensions look consistent with the new message features.
Blocks, bot description, view botId, and message lifecycle events are wired coherently.Also applies to: 86-90, 162-169, 377-399, 453-455
packages/slack/src/server/web-api.ts (3)
98-113: Form‑encoded JSON parsing and block_id defaults are a solid upgrade.
This improves compatibility with Slack clients and keeps block actions deterministic.Also applies to: 302-346
407-451: Persisting block updates and returning blocks in history/replies is a good sync step.
This keeps edits and reloads consistent for block‑based messages.Also applies to: 547-604
652-690: Targeted bot dispatch and typed view values are well integrated.
This should make modal submissions and message block actions far more faithful.Also applies to: 726-730, 1403-1413, 1467-1476, 1510-1532, 1630-1740, 1763-1925
apps/ui/src/components/BotAboutHeader.svelte (1)
1-45: Nice, polished DM bot header.
The layout and optional description rendering are clean.apps/ui/src/components/MessagePanel.svelte (1)
43-83: Day separators and DM bot header fit cleanly into the panel.
The new grouping and header layout read well, and the menu relocation keeps the header tidy.Also applies to: 129-145, 205-260, 267-285
apps/ui/src/lib/dispatcher.svelte.ts (1)
6-39: Good extension of the dispatcher for channels and block actions.
Channel management and message update/delete handling look consistent with the emulator changes.Also applies to: 48-67, 272-371, 618-705, 838-888
apps/ui/src/components/Message.svelte (1)
77-88: Ephemeral indicator and BlockKit rendering integration look clean.
The conditional banner and block renderer wiring read well.Also applies to: 318-324, 378-392
apps/ui/src/lib/types.ts (17)
16-26: Nice extension to simulator message typing.
Addingsubtypeandblocksaligns message payloads with richer Block Kit data.
28-41: Preset channels look consistent.
TheisPresetmarker andPRESET_CHANNELSlist are clear and self‑documenting.
61-66: Optional app description fits the new bot metadata.
No concerns here.
103-133: Context actions block types are clean and cohesive.
Looks good.
134-146: Block union expansion is aligned with new UI support.
No issues spotted.
196-288: Rich text types are comprehensive and well‑structured.
The inline/block split reads clearly.
290-307: Table block typing is clear.
No concerns.
309-328: Element union expansion matches new control surface.
LGTM.
329-337: Button confirm support is consistent with Block Kit.
Looks good.
345-352: Static select confirm field is a good addition.
No issues.
354-372: Overflow options and confirm typing are clear.
LGTM.
375-396: Input element union now covers the new controls.
All good.
423-429: Checkbox confirm support is consistent.
No concerns.
431-555: New input element interfaces are well‑typed.
Covers the expected surfaces cleanly.
557-565: Workspace select union is tidy and complete.
LGTM.
567-573: UploadedFile additions align with image preview needs.
No concerns.
576-584: Optional bot description is a sensible extension.
Looks good.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/showcase-bot/src/config/loader.ts`:
- Around line 55-60: The GroupDefinition interface exported from loader.ts
currently uses the field collapsed but the UI expects collapsible and expanded;
update the GroupDefinition interface (export interface GroupDefinition) to
replace collapsed?: boolean with collapsible?: boolean and expanded?: boolean,
and audit any code that constructs GroupDefinition objects (e.g., config
builders/serializers that return the /config HTTP response) to populate these
new fields consistently so the HTTP config payload matches the UI schema.
In `@apps/ui/src/lib/types.ts`:
- Around line 80-87: The SlackConfirmDialog interface currently types title,
text, confirm, and deny as SlackViewTextObject (which allows mrkdwn); change
these four fields to use SlackPlainTextObject instead so the confirm dialog only
permits plain_text per Slack Block Kit rules; keep the optional style?:
'primary' | 'danger' and update any related references or usages of
SlackConfirmDialog to construct SlackPlainTextObject instances for those fields
(search for SlackConfirmDialog, title, text, confirm, deny).
---
Outside diff comments:
In `@apps/ui/src/components/Message.svelte`:
- Around line 89-110: The code builds HTML by inserting userName directly into a
replacement span in the formattedText derived value (see variables userName,
escapedUserName, mentionRegex and the html.replace call), which can lead to XSS
if simulatorState.simulatedUserName contains special characters; fix by
HTML-escaping the display name (escape &, <, >, ", ') into a new
escapedDisplayName and use that escapedDisplayName in the replacement string
while keeping mentionRegex (which should still escape regex metacharacters as
currently done); apply the same change to the other identical usage site that
wraps `@mentions` in a span (the same pattern elsewhere in the file).
---
Duplicate comments:
In `@apps/ui/src/components/Message.svelte`:
- Around line 200-266: The resolveActionFromBlocks function misses handling
Slack "context" blocks, so context action elements fall through to the fallback;
update resolveActionFromBlocks to check for block.type === 'context' (similar to
the 'actions' and 'section' cases), iterate contextBlock.elements, detect
elements with matching action_id, and return buildActionValue(blockId, el,
value) when found so context actions preserve their blockId and elementType in
the payload.
In `@packages/slack/src/server/web-api.ts`:
- Around line 458-483: The current chatDelete method uses
this.state.deleteMessage(ts), which can remove the wrong message when timestamps
collide across channels; update chatDelete (the private async method chatDelete)
to call the channel-scoped deletion API
this.state.deleteMessageByChannelAndTs(channel, ts) (or equivalent channel+ts
method), use its boolean result for the missing-message branch, and continue to
emit the same SSE event and return the same JSON body when deletion succeeds;
ensure you only change the deletion call and related conditional check to use
the channel+ts variant so deletion is scoped to the provided channel.
- Around line 367-401: chat.postEphemeral is persisting ephemeral messages into
the global store by calling state.addMessage; change this so ephemerals are not
added to shared history: either remove the state.addMessage call from
chatPostEphemeral or update State.addMessage to ignore messages where
SlackMessage.subtype === 'ephemeral' and add a dedicated per-user ephemeral
store/API if needed; also ensure read paths (getChannelMessages and
getThreadMessages) explicitly filter out subtype 'ephemeral' so ephemeral
entries never surface in shared history or thread replies.
Security: add rel="noopener noreferrer" to target="_blank" links and window.open calls. Bugs: scope chatDelete to channel, fix duplicate action_id in kitchen-sink, reset StaticSelect on confirm deny, use option.value as Checkboxes each-key. Robustness: INSERT OR IGNORE for duplicate channels, add fetch timeouts to image proxy, filter undefined in updateMessage. Also adds a11y attrs to ImageBlock toggle and minor code quality fixes.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ui/src/lib/state.svelte.ts (1)
252-263:⚠️ Potential issue | 🟡 MinorAdd channel validation when channels load.
When
initFromHashruns before channels are loaded, anyC_-prefixed ID is accepted. OncesetChannelsmarkschannelsLoaded = true(line 207), there's no re-validation ofcurrentChannelagainst the actual loaded channels. If the URL hash pointed to a non-existent channel, the user remains on an invalid channel ID even after the channel list is available.Add validation in
setChannelsto ensurecurrentChannelis valid, and redirect toC_GENERALif it's no longer in the channels list.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/lib/state.svelte.ts` around lines 252 - 263, Add validation in setChannels: after assigning simulatorState.channels and setting simulatorState.channelsLoaded = true (in the setChannels function), check simulatorState.currentChannel (and its channelId) against the newly loaded simulatorState.channels list; if the current channelId is not present, set simulatorState.currentChannel = { channelId: 'C_GENERAL', threadTs: null } and update the URL/hash/navigation accordingly so the app redirects to C_GENERAL. Ensure you reference the existing symbols setChannels, simulatorState.channelsLoaded, simulatorState.channels, and simulatorState.currentChannel when making the change.
🧹 Nitpick comments (5)
apps/ui/src/components/blockkit/blocks/RichTextBlock.svelte (1)
94-96: Consider preserving consecutive line breaks.
newlinesToBreakscurrently collapses multiple\ninto a single break, which can remove intentional blank lines. If you want closer Slack formatting fidelity, replace each newline individually.♻️ Suggested tweak
- function newlinesToBreaks(html: string): string { - return html.replace(/\n+/g, '<span class="rt-br"></span>') - } + function newlinesToBreaks(html: string): string { + return html.replace(/\n/g, '<span class="rt-br"></span>') + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/blockkit/blocks/RichTextBlock.svelte` around lines 94 - 96, The function newlinesToBreaks currently collapses consecutive newlines because it uses /\n+/g; change the replacement to handle each newline individually (e.g. use /\r?\n/g or /\n/g instead of /\n+/g) so every newline is replaced with its own '<span class="rt-br"></span>', preserving intentional blank lines and better matching Slack-style formatting.packages/slack/src/server/persistence.ts (1)
543-564: Consider wrapping deletion in a transaction for atomicity.The method deletes messages first (line 553), then deletes the channel (line 556-558). If the channel deletion fails after messages are removed, the data would be in an inconsistent state. Whilst unlikely with synchronous SQLite operations, using a transaction would provide stronger guarantees.
Suggested transaction wrapper
async removeChannel(id: string): Promise<boolean> { if (!this.db) return false // Refuse to delete preset channels const channel = this.db .query(`SELECT is_preset FROM simulator_channels WHERE id = ?`) .get(id) as { is_preset: number } | null if (!channel || channel.is_preset === 1) return false + this.db.run('BEGIN TRANSACTION') + try { // Delete messages in this channel this.db.run(`DELETE FROM simulator_messages WHERE channel = ?`, [id]) // Delete the channel const result = this.db.run( `DELETE FROM simulator_channels WHERE id = ? AND is_preset = 0`, [id] ) + this.db.run('COMMIT') if (result.changes > 0) { persistenceLogger.info(`Removed channel: ${id}`) } return result.changes > 0 + } catch (err) { + this.db.run('ROLLBACK') + throw err + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/slack/src/server/persistence.ts` around lines 543 - 564, The removeChannel method currently deletes messages then the channel separately, risking partial deletes; update removeChannel to perform the DELETE FROM simulator_messages and DELETE FROM simulator_channels inside a single database transaction (e.g., BEGIN/COMMIT or using the DB's transaction API) so both operations are atomic, and ensure you ROLLBACK on any error; keep the same checks for is_preset (querying simulator_channels), perform the two DELETEs inside the transaction, log via persistenceLogger.info when result.changes > 0, and return the boolean only after committing (or false after rollback) to preserve consistency.packages/slack/src/server/index.ts (1)
490-522: Consider adding URL validation to mitigate SSRF risks.The image proxy fetches arbitrary user-supplied URLs. Whilst this is a development emulator (reducing real-world risk), it could still be used to probe internal network services, localhost endpoints, or cloud metadata APIs (e.g.,
169.254.169.254). The 5-second timeouts are a good addition.Consider validating the URL scheme and rejecting private/internal IP ranges before fetching.
Example URL validation
if (path === '/api/simulator/image-size' && req.method === 'GET') { const imageUrl = url.searchParams.get('url') if (!imageUrl) { return Response.json( { ok: false, error: 'missing url param' }, { status: 400, headers: { 'Access-Control-Allow-Origin': '*' } } ) } + + // Basic validation: only allow http(s) and reject obvious internal targets + try { + const parsed = new URL(imageUrl) + if (!['http:', 'https:'].includes(parsed.protocol)) { + return Response.json( + { ok: false, error: 'invalid protocol' }, + { status: 400, headers: { 'Access-Control-Allow-Origin': '*' } } + ) + } + const host = parsed.hostname.toLowerCase() + if ( + host === 'localhost' || + host === '127.0.0.1' || + host.startsWith('192.168.') || + host.startsWith('10.') || + host === '169.254.169.254' + ) { + return Response.json( + { ok: false, error: 'internal url not allowed' }, + { status: 403, headers: { 'Access-Control-Allow-Origin': '*' } } + ) + } + } catch { + return Response.json( + { ok: false, error: 'invalid url' }, + { status: 400, headers: { 'Access-Control-Allow-Origin': '*' } } + ) + } + try { const res = await fetch(imageUrl, {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/slack/src/server/index.ts` around lines 490 - 522, The image proxy handler for path '/api/simulator/image-size' currently fetches arbitrary URLs; validate the user-supplied imageUrl before any fetch in that handler by parsing it with URL (ensure protocol is http or https), disallow hostnames like "localhost" and IPs in private/reserved ranges (e.g., 127.0.0.0/8, 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, link-local 169.254.0.0/16, and IPv6 equivalents), and reject requests to known cloud metadata endpoints (e.g., 169.254.169.254); perform DNS resolution of the hostname and check the resolved IPs against those ranges before proceeding, returning a 400/403 JSON error if validation fails so no fetch is attempted.apps/ui/src/lib/state.svelte.ts (1)
210-213: Consider a defensive duplicate check.
addChannelToStateappends without checking if a channel with the same ID already exists. If the API ever sends a duplicate (e.g., due to a race or retry), this would create duplicates in the UI list. A guard would be cheap insurance.♻️ Proposed defensive check
// Add a channel to state export function addChannelToState(channel: Channel): void { + if (simulatorState.channels.some((c) => c.id === channel.id)) return simulatorState.channels = [...simulatorState.channels, channel] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/lib/state.svelte.ts` around lines 210 - 213, The addChannelToState function currently appends the new Channel into simulatorState.channels without checking for duplicates; update addChannelToState to first check simulatorState.channels for an existing channel with the same id (Channel.id) and if found either skip adding or replace the existing entry, otherwise append; use Array.prototype.findIndex or some equivalent on simulatorState.channels to detect the duplicate and update the array immutably (e.g., map/replace or early return) so the UI list cannot contain duplicate channel entries.apps/ui/src/components/blockkit/elements/OverflowMenu.svelte (1)
95-119: Consider adding ARIA attributes for menu accessibility.The overflow menu would benefit from standard WAI-ARIA menu button attributes for screen reader users.
♿ Suggested accessibility improvements
<button type="button" class="h-8 px-2 rounded-lg border border-white/20 bg-slack-input text-slack-text-muted hover:bg-white/10 hover:text-slack-text transition-colors inline-flex items-center" onclick={toggle} bind:this={triggerRef} aria-label="More options" aria-expanded={isOpen} + aria-haspopup="menu" > <Ellipsis size={16} /> </button> {`#if` isOpen} <div style={menuStyle} class="min-w-36 bg-slack-sidebar border border-white/20 rounded-md shadow-lg z-200" + role="menu" > {`#each` element.options as option (option.value)} <button type="button" class="block w-full py-2 px-3 bg-transparent border-none text-left text-sm text-slack-text cursor-pointer transition-colors hover:bg-white/10" onclick={() => handleSelect(option)} + role="menuitem" > {renderText(option.text)} </button> {/each} </div> {/if}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/blockkit/elements/OverflowMenu.svelte` around lines 95 - 119, Add standard WAI-ARIA attributes to the OverflowMenu UI: on the trigger button (where toggle, triggerRef, isOpen are used) include aria-haspopup="menu" (if not present), aria-controls pointing to the menu's id, and ensure aria-expanded toggles with isOpen; give the menu container (using menuStyle and element.options) a unique id and role="menu"; for each option rendered via handleSelect and renderText, set role="menuitem", make them keyboard-focusable (tabindex="0") and ensure key handlers (Enter/Space) call handleSelect so keyboard users can activate items.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
apps/showcase-bot/README.mdapps/showcase-bot/package.jsonapps/showcase-bot/src/messages/blocks/10-kitchen-sink.jsonapps/ui/src/components/blockkit/blocks/ImageBlock.svelteapps/ui/src/components/blockkit/blocks/RichTextBlock.svelteapps/ui/src/components/blockkit/context.tsapps/ui/src/components/blockkit/elements/Checkboxes.svelteapps/ui/src/components/blockkit/elements/EmailInput.svelteapps/ui/src/components/blockkit/elements/NumberInput.svelteapps/ui/src/components/blockkit/elements/OverflowMenu.svelteapps/ui/src/components/blockkit/elements/StaticSelect.svelteapps/ui/src/lib/state.svelte.tspackages/mrkdwn/src/mrkdwn-to-html.test.tspackages/mrkdwn/src/mrkdwn-to-html.tspackages/slack/src/server/index.tspackages/slack/src/server/persistence.tspackages/slack/src/server/web-api.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/mrkdwn/src/mrkdwn-to-html.test.ts
- apps/ui/src/components/blockkit/context.ts
- apps/showcase-bot/src/messages/blocks/10-kitchen-sink.json
- apps/ui/src/components/blockkit/elements/EmailInput.svelte
- apps/ui/src/components/blockkit/elements/NumberInput.svelte
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-14T21:36:07.933Z
Learnt from: tyom
Repo: tyom/botarium PR: 1
File: apps/ui/src/components/MessagePanel.svelte:10-23
Timestamp: 2026-01-14T21:36:07.933Z
Learning: In the client-only Botarium UI (Vite + Svelte), do not add or rely on SSR-specific guards (e.g., checks for sessionStorage) in components. Since the app runs only in Electron or the browser, code that accesses browser APIs will execute in a non-SSR environment, so SSR-unsafe checks are unnecessary and can be removed for maintainability.
Applied to files:
apps/ui/src/components/blockkit/blocks/ImageBlock.svelteapps/ui/src/components/blockkit/elements/StaticSelect.svelteapps/ui/src/components/blockkit/elements/OverflowMenu.svelteapps/ui/src/components/blockkit/elements/Checkboxes.svelteapps/ui/src/components/blockkit/blocks/RichTextBlock.svelte
🧬 Code graph analysis (2)
apps/ui/src/lib/state.svelte.ts (1)
apps/ui/src/lib/types.ts (3)
PRESET_CHANNELS(35-38)Channel(28-33)SimulatorMessage(16-26)
packages/slack/src/server/web-api.ts (1)
packages/slack/src/server/types.ts (3)
SlackMessage(52-67)ChatPostMessageRequest(260-266)ViewsOpenRequest(171-174)
🪛 ast-grep (0.40.5)
packages/mrkdwn/src/mrkdwn-to-html.ts
[warning] 225-225: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((${BR_RE.source}){2,}, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 229-229: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((${BR_RE.source})+(<(?:ul|ol|blockquote|pre)>), 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 233-233: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((<\\/(?:ul|ol|blockquote|pre)>)(${BR_RE.source})+, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 238-238: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((${BR_RE.source})+(\uE000CB\\d+\uE000), 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 242-242: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((\uE000CB\\d+\uE000)(${BR_RE.source})+, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 252-252: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((${BR_RE.source})+(<pre>), 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 253-253: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((<\\/pre>)(${BR_RE.source})+, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (32)
apps/showcase-bot/package.json (1)
1-26: LGTM!The package manifest is well-structured. The
@types/bunversion has been correctly pinned to^1.3.6(addressing the previous review feedback), and the dependency versions are appropriate. Zod^4.3.5correctly targets Zod 4.apps/showcase-bot/README.md (1)
1-95: LGTM!The documentation is comprehensive and well-organised. Both previously flagged issues (missing language specifiers on fenced code blocks) have been addressed —
jsat line 34 andtextat line 79.apps/ui/src/components/blockkit/blocks/RichTextBlock.svelte (2)
16-69: Solid sanitisation allowlist and safe link attributes.Allowlisting only required tags/attrs and adding
rel="noopener noreferrer"totarget="_blank"links is a good security baseline.
98-133: Block-level rendering plus final sanitisation looks good.The block rendering flow is clear, and sanitising once on the final HTML keeps the surface tight.
packages/slack/src/server/web-api.ts (13)
98-113: Robust form‑urlencoded JSON parsing.
Good recovery of JSON-encoded fields while keeping plain strings intact.
206-239: New endpoint routing looks correct.
Addingchat.postEphemeral,chat.delete, and token-aware views routes is clean and consistent.
302-363: chat.postMessage Block Kit handling is solid.
Validation plus block_id auto-generation and block persistence are well handled.
407-451: chat.update block handling and re-render signalling look good.
Persisting and emittingmessage_updatekeeps UI state consistent.
458-484: Channel-scoped deletion is a good fix.
UsingdeleteMessageByChannelAndTsprevents cross-channel deletes.
569-599: Blocks passthrough in history/replies is correct.
Including blocks keeps downstream renderers in sync.
652-691: Token-aware view storage is consistent.
StoringbotIdfrom token enables targeted dispatch without breaking legacy flows.
726-729: views.push delegation is fine.
Reusing views.open with token is a clean interim implementation.
1404-1411: Connected bots response enrichment is helpful.
Exposing description improves UI without behavioural risk.
1467-1477: Targeted slash command dispatch is well-structured.
Logging and target selection are clear and low-risk.
1510-1531: Typed view submission dispatch is a good addition.
Normalising values before dispatch improves compatibility with bot handlers.
1630-1740: Value normalisation helper is thorough.
The discriminator map and per‑type reshaping cover the core input elements cleanly.
1764-1925: Block action handling for modal + message contexts looks correct.
Action shaping and trigger context storage align with Slack payload expectations.packages/slack/src/server/index.ts (2)
266-272: LGTM!Conditionally spreading
subtypeandblocksinto the response is a clean approach that avoids sending unnecessary fields when they're undefined.
314-400: LGTM!The channel management endpoints are well-structured:
- Name sanitisation ensures valid channel names
- Duplicate check uses the generated ID
- Preset channels are protected from deletion
- Appropriate HTTP status codes (409 for conflict, 403 for forbidden, 404 for not found)
packages/slack/src/server/persistence.ts (5)
23-28: LGTM!The
MessageRecordinterface correctly includes the new optional fields matching the database schema additions.
115-127: LGTM!The migrations follow the established idempotent pattern using try/catch for
ALTER TABLE, consistent with other migrations in this file.
192-211: LGTM!Good use of
INSERT OR IGNOREfor seeding preset channels, ensuring idempotent initialisation.
226-248: LGTM!The
saveMessagemethod correctly serialisesblocksto JSON and includes bothblocksandsubtypein the upsert, with proper conflict handling.
276-304: LGTM!Query and mapping correctly deserialise
blocksfrom JSON and handle nullablesubtype.apps/ui/src/lib/state.svelte.ts (3)
41-43: LGTM!The channel state initialisation is well structured: spreading
PRESET_CHANNELScreates an independent array, and thechannelsLoadedflag enables deferred validation logic elsewhere.
192-197: LGTM!The dynamic channel lookup with a sensible fallback for unknown IDs is a clean approach.
345-356: LGTM!The addition of
subtypeandblockspropagation follows the existing restoration pattern. The type assertion forblocksis acceptable here since the data originates from the application's own persistence layer.apps/ui/src/components/blockkit/elements/StaticSelect.svelte (1)
27-48: Confirmation flow correctly implemented.The visual selection is now reset before showing the confirmation dialog (line 30) and again defensively in
handleDeny(line 45). This addresses the concern about the select displaying an unconfirmed value when the user denies.apps/ui/src/components/blockkit/elements/OverflowMenu.svelte (1)
50-61: Security fix correctly applied.Both
window.opencalls now include'noopener,noreferrer'to prevent reverse tabnabbing attacks.apps/ui/src/components/blockkit/elements/Checkboxes.svelte (3)
46-64: Confirmation flow correctly defers state changes.The
onChangecallback is only invoked after confirmation, soselectedOptionsremains unchanged during the confirmation dialog. Svelte's reactivechecked={isSelected(option)}binding keeps the visual state consistent with the actual selection state.
68-68: Good fix: usingoption.valueas the iteration key.Using
option.valueinstead ofoption.text.textis more reliable since values should be unique identifiers within the options array.
76-87: No XSS concern—renderMrkdwnproperly sanitises output using DOMPurify.The
renderMrkdwnfunction inapps/ui/src/components/blockkit/context.tsalready sanitises all HTML output. For mrkdwn content, it parses withmrkdwnToHtml()and then sanitises usingDOMPurify.sanitize()with a restricted configuration that allows only safe formatting tags (a, b, blockquote, br, code, del, em, i, li, ol, pre, span, strong, ul) and minimal attributes. For plain text, it escapes HTML entities. The{@html}usage is safe.Likely an incorrect or invalid review comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ui/src/components/blockkit/blocks/ImageBlock.svelte`:
- Around line 21-31: Reset imageSize immediately when the block changes and
ensure the fetch result treats 0 as valid; inside the $effect that builds the
URL using EMULATOR_API_URL and block.image_url, set imageSize = undefined (or
empty) at start, create a local token (e.g., requestId or an AbortController) to
identify/abort stale requests, then only apply formatBytes(data.size) to
imageSize if the token matches and data.ok is true (accept data.size === 0 as
valid). Also handle fetch errors (don't swallow silently) and cancel/ignore
previous inflight requests to prevent race conditions in the ImageBlock
component.
In `@apps/ui/src/components/blockkit/elements/OverflowMenu.svelte`:
- Around line 106-119: Replace the invalid Tailwind class "z-200" with the
arbitrary-value syntax "z-[200]" in the component markup; specifically update
the overflow/menu container in OverflowMenu (the div using class="... z-200")
and make the same replacement in the other affected components (TimePicker,
DateTimePicker, DatePicker) wherever "z-200" appears so the intended z-index is
applied.
In `@apps/ui/src/components/blockkit/elements/StaticSelect.svelte`:
- Around line 52-55: The class attribute in StaticSelect.svelte currently uses
the expression "{compact && 'text-sm'}" which injects the literal "false" when
compact is falsy; change this to a proper conditional so falsy values don't
become classes—either use a ternary ("{compact ? 'text-sm' : ''}") or Svelte's
class directive (class:text-sm={compact}) on the same element to apply the
text-sm class only when the compact prop is truthy.
---
Outside diff comments:
In `@apps/ui/src/lib/state.svelte.ts`:
- Around line 252-263: Add validation in setChannels: after assigning
simulatorState.channels and setting simulatorState.channelsLoaded = true (in the
setChannels function), check simulatorState.currentChannel (and its channelId)
against the newly loaded simulatorState.channels list; if the current channelId
is not present, set simulatorState.currentChannel = { channelId: 'C_GENERAL',
threadTs: null } and update the URL/hash/navigation accordingly so the app
redirects to C_GENERAL. Ensure you reference the existing symbols setChannels,
simulatorState.channelsLoaded, simulatorState.channels, and
simulatorState.currentChannel when making the change.
---
Duplicate comments:
In `@apps/ui/src/lib/state.svelte.ts`:
- Around line 90-105: updateMessage currently mutates the inner Map entry but
may not trigger Svelte reactivity; after updating channelMsgs with
channelMsgs.set(ts, { ...msg, ...defined }), also reassign the parent map by
calling simulatorState.messages.set(channel, channelMsgs) (same pattern used in
addReactionToMessage) to ensure the store change is observed.
In `@packages/mrkdwn/src/mrkdwn-to-html.ts`:
- Around line 80-87: The current regexes in mrkdwn-to-html (the two patterns
/<((?:https?|mailto):[^|&]+)\|([^&]+)>/ and
/<((?:https?|mailto):[^&]+)>/) break URLs/labels containing escaped
ampersands because escapeHtml turns & into &, so [^&]+ stops at the amp;
sequence; replace those character-class-based patterns with non-greedy
dot-matchers that allow escaped entities (e.g.
/<((?:https?|mailto):.+?)\|(.+?)>/g and
/<((?:https?|mailto):.+?)>/g) so the capture groups include &
sequences and full query strings before converting into <a href=...> links.
In `@packages/slack/src/server/web-api.ts`:
- Around line 367-401: chatPostEphemeral currently calls state.addMessage which
stores ephemeral messages in the shared history; change this so ephemeral
messages are not added to the shared store: either (A) avoid calling
state.addMessage for messages where message.subtype === 'ephemeral' in
chatPostEphemeral, and instead record them in a separate ephemeral
per-user/per-channel store, or (B) keep addMessage but update
state.getChannelMessages and state.getThreadMessages to filter out messages with
subtype === 'ephemeral' so they never appear in conversations.history/replies;
update references in chatPostEphemeral, state.addMessage usage,
getChannelMessages, and getThreadMessages accordingly to ensure ephemerals are
surfaced only to the intended user and not returned by shared-history APIs.
---
Nitpick comments:
In `@apps/ui/src/components/blockkit/blocks/RichTextBlock.svelte`:
- Around line 94-96: The function newlinesToBreaks currently collapses
consecutive newlines because it uses /\n+/g; change the replacement to handle
each newline individually (e.g. use /\r?\n/g or /\n/g instead of /\n+/g) so
every newline is replaced with its own '<span class="rt-br"></span>', preserving
intentional blank lines and better matching Slack-style formatting.
In `@apps/ui/src/components/blockkit/elements/OverflowMenu.svelte`:
- Around line 95-119: Add standard WAI-ARIA attributes to the OverflowMenu UI:
on the trigger button (where toggle, triggerRef, isOpen are used) include
aria-haspopup="menu" (if not present), aria-controls pointing to the menu's id,
and ensure aria-expanded toggles with isOpen; give the menu container (using
menuStyle and element.options) a unique id and role="menu"; for each option
rendered via handleSelect and renderText, set role="menuitem", make them
keyboard-focusable (tabindex="0") and ensure key handlers (Enter/Space) call
handleSelect so keyboard users can activate items.
In `@apps/ui/src/lib/state.svelte.ts`:
- Around line 210-213: The addChannelToState function currently appends the new
Channel into simulatorState.channels without checking for duplicates; update
addChannelToState to first check simulatorState.channels for an existing channel
with the same id (Channel.id) and if found either skip adding or replace the
existing entry, otherwise append; use Array.prototype.findIndex or some
equivalent on simulatorState.channels to detect the duplicate and update the
array immutably (e.g., map/replace or early return) so the UI list cannot
contain duplicate channel entries.
In `@packages/slack/src/server/index.ts`:
- Around line 490-522: The image proxy handler for path
'/api/simulator/image-size' currently fetches arbitrary URLs; validate the
user-supplied imageUrl before any fetch in that handler by parsing it with URL
(ensure protocol is http or https), disallow hostnames like "localhost" and IPs
in private/reserved ranges (e.g., 127.0.0.0/8, 10.0.0.0/8, 172.16.0.0/12,
192.168.0.0/16, link-local 169.254.0.0/16, and IPv6 equivalents), and reject
requests to known cloud metadata endpoints (e.g., 169.254.169.254); perform DNS
resolution of the hostname and check the resolved IPs against those ranges
before proceeding, returning a 400/403 JSON error if validation fails so no
fetch is attempted.
In `@packages/slack/src/server/persistence.ts`:
- Around line 543-564: The removeChannel method currently deletes messages then
the channel separately, risking partial deletes; update removeChannel to perform
the DELETE FROM simulator_messages and DELETE FROM simulator_channels inside a
single database transaction (e.g., BEGIN/COMMIT or using the DB's transaction
API) so both operations are atomic, and ensure you ROLLBACK on any error; keep
the same checks for is_preset (querying simulator_channels), perform the two
DELETEs inside the transaction, log via persistenceLogger.info when
result.changes > 0, and return the boolean only after committing (or false after
rollback) to preserve consistency.
- Wire onAction callback for WorkspaceSelect in ActionsBlock - Lower dropdown z-index from z-200 to z-100 across pickers and overflow menu - Convert datetimepicker values to numeric selected_date_time on modal submit - Remove unused build script and dist from mrkdwn package (workspace-only) - Escape mrkdwn-reserved characters (<, >, &) in markdown-to-mrkdwn output
- Filter ephemeral messages from getChannelMessages/getThreadMessages to match real Slack API behavior - Replace undefined CSS variables with their fallback literal values in emoji tooltip styles - Wire onChange handler for WorkspaceSelect in SectionBlock and InputBlock - Handle context_actions blocks in resolveActionFromBlocks for proper action dispatch - Await addChannel in Sidebar and only close modal on success - Fix mrkdwn link regexes to support ampersands in URLs after HTML escaping
ImageBlock: use AbortController to cancel stale fetch requests, reset imageSize on block change, and treat size === 0 as valid. StaticSelect: use ternary to avoid injecting literal "false" into class.
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
apps/ui/src/app.cssapps/ui/src/components/Message.svelteapps/ui/src/components/ModalOverlay.svelteapps/ui/src/components/Sidebar.svelteapps/ui/src/components/blockkit/blocks/ActionsBlock.svelteapps/ui/src/components/blockkit/blocks/ImageBlock.svelteapps/ui/src/components/blockkit/blocks/InputBlock.svelteapps/ui/src/components/blockkit/blocks/SectionBlock.svelteapps/ui/src/components/blockkit/elements/DatePicker.svelteapps/ui/src/components/blockkit/elements/DateTimePicker.svelteapps/ui/src/components/blockkit/elements/OverflowMenu.svelteapps/ui/src/components/blockkit/elements/StaticSelect.svelteapps/ui/src/components/blockkit/elements/TimePicker.svelteapps/ui/src/components/blockkit/elements/WorkspaceSelect.sveltepackages/mrkdwn/package.jsonpackages/mrkdwn/src/markdown-to-mrkdwn.tspackages/mrkdwn/src/mrkdwn-to-html.tspackages/slack/src/server/state.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/ui/src/components/blockkit/blocks/ImageBlock.svelte
- apps/ui/src/components/blockkit/elements/StaticSelect.svelte
- apps/ui/src/components/blockkit/elements/WorkspaceSelect.svelte
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-14T21:36:07.933Z
Learnt from: tyom
Repo: tyom/botarium PR: 1
File: apps/ui/src/components/MessagePanel.svelte:10-23
Timestamp: 2026-01-14T21:36:07.933Z
Learning: In the client-only Botarium UI (Vite + Svelte), do not add or rely on SSR-specific guards (e.g., checks for sessionStorage) in components. Since the app runs only in Electron or the browser, code that accesses browser APIs will execute in a non-SSR environment, so SSR-unsafe checks are unnecessary and can be removed for maintainability.
Applied to files:
apps/ui/src/components/ModalOverlay.svelteapps/ui/src/components/blockkit/elements/OverflowMenu.svelteapps/ui/src/components/blockkit/elements/TimePicker.svelteapps/ui/src/components/blockkit/blocks/InputBlock.svelteapps/ui/src/components/blockkit/blocks/ActionsBlock.svelteapps/ui/src/components/blockkit/blocks/SectionBlock.svelteapps/ui/src/components/blockkit/elements/DateTimePicker.svelteapps/ui/src/components/blockkit/elements/DatePicker.svelteapps/ui/src/components/Sidebar.svelteapps/ui/src/components/Message.svelte
🧬 Code graph analysis (2)
packages/mrkdwn/src/markdown-to-mrkdwn.ts (1)
packages/mrkdwn/src/index.ts (1)
markdownToMrkdwn(2-2)
packages/mrkdwn/src/mrkdwn-to-html.ts (1)
packages/mrkdwn/src/index.ts (1)
mrkdwnToHtml(1-1)
🪛 ast-grep (0.40.5)
packages/mrkdwn/src/mrkdwn-to-html.ts
[warning] 225-225: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((${BR_RE.source}){2,}, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 229-229: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((${BR_RE.source})+(<(?:ul|ol|blockquote|pre)>), 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 233-233: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((<\\/(?:ul|ol|blockquote|pre)>)(${BR_RE.source})+, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 238-238: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((${BR_RE.source})+(\uE000CB\\d+\uE000), 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 242-242: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((\uE000CB\\d+\uE000)(${BR_RE.source})+, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 252-252: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((${BR_RE.source})+(<pre>), 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 253-253: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((<\\/pre>)(${BR_RE.source})+, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 Stylelint (17.3.0)
apps/ui/src/app.css
[error] 260-260: Unexpected missing scoping root (nesting-selector-no-missing-scoping-root)
(nesting-selector-no-missing-scoping-root)
[error] 264-264: Unexpected missing scoping root (nesting-selector-no-missing-scoping-root)
(nesting-selector-no-missing-scoping-root)
[error] 257-257: Unexpected unknown at-rule "@Utility" (scss/at-rule-no-unknown)
(scss/at-rule-no-unknown)
🔇 Additional comments (25)
apps/ui/src/app.css (2)
256-267: LGTM!The
channel-tabutility is well-structured and follows the established pattern of other utilities in this file (panel-tab,module-toggle). The static analysis errors for@utilityand nested selectors are false positives—@utilityis a valid Tailwind CSS v4 directive, and nesting within it is expected behaviour.
311-448: LGTM!The mrkdwn rendering styles are comprehensive and well-organised. Good use of CSS variables where appropriate (
var(--font-mono),var(--text-primary),var(--text-secondary)). The previous review feedback regarding undefined CSS variables in the emoji tooltip has been addressed—the values are now used directly rather than referencing non-existent variables.packages/slack/src/server/state.ts (3)
535-549: Uniqueness of ts is enforced at the database layer; concern is unfounded.The
tscolumn in thesimulator_messagestable has aUNIQUEconstraint (line 90 of persistence.ts), which guarantees global uniqueness across all channels. Therefore,deleteMessage(ts)correctly and safely deletes the intended message without risk of cross-channel collision.Likely an incorrect or invalid review comment.
825-828: Token matching strategy is correct and documented.The
xoxb-${bot.id}format is not an assumption but a documented pattern used consistently throughout the codebase. TheextractBotIdFromTokenmethod inweb-api.ts(lines 59–68) explicitly documents: "Tokens are formatted as xoxb-{botId}". Token validation accepts tokens starting withxoxb-orxoxp-, and the token matching logic ingetBotByTokencorrectly reconstructs and validates tokens against this standardised format. No alternative token generation paths or random suffixes are present in the codebase.Likely an incorrect or invalid review comment.
132-147: No action required.The persistence layer correctly handles message deletion. The
removeChannelmethod in persistence.ts explicitly deletes all messages for the channel (line 553) before removing the channel itself, preventing message reappearance on next load.packages/mrkdwn/src/markdown-to-mrkdwn.ts (2)
10-13: Escaping implementation is appropriate for the conversion direction.The
escapeMrkdwnfunction correctly escapes&,<, and>. Since<and>are escaped, the|character cannot form part of a Slack link construct in the output, making additional|escaping unnecessary for this Markdown→mrkdwn direction.
179-193: Clean entry point with appropriate empty-input handling.The public API is well-structured with early return for empty input and proper trimming of trailing whitespace.
packages/mrkdwn/src/mrkdwn-to-html.ts (3)
80-88: Link regexes now correctly handle URLs containing ampersands.The non-greedy
.*?patterns in the link regexes (lines 82, 86) properly match URLs containing&(escaped ampersands), addressing the previous concern about[^&]+patterns breaking on query strings. Therel="noopener noreferrer"attribute is also correctly included.
220-254: Static analysis ReDoS warnings are false positives.The
ast-grepwarnings about regex construction from variables (lines 225-253) are false positives.BR_REis derived fromBR, which is a hardcoded constant defined at line 161. Since the pattern source is not user-controlled input, there is no ReDoS risk. The pattern is simply the escaped form of a fixed HTML span element.
147-256: Well-structured four-phase parsing pipeline.The implementation cleanly separates concerns: code block extraction, line-by-line block processing, line-break normalisation, and code block restoration. The placeholder approach effectively protects code content from formatting transformations.
packages/mrkdwn/package.json (1)
1-21: Package configuration is consistent for workspace-only consumption.The exports point directly to TypeScript source and the
filesarray includes onlysrcwith no build script present. This is a valid pattern for internal monorepo packages consumed viaworkspace:*resolution, where TypeScript handling is managed by the workspace. Themarkeddependency version ^15.0.0 is correctly specified and publicly available.apps/ui/src/components/Message.svelte (5)
8-52: Imports/prop updates align with the new Block Kit metadata.
No issues spotted in the new imports or the expandedonImagePreviewsignature.
201-286: Action resolution helper looks consistent.
The section/accessory and actions traversal is clear and keeps the fallback behaviour predictable.
288-303: Dispatcher wiring is clean and easy to follow.
The action payload assembly and dispatch path reads well.
335-519: Block Kit rendering + ephemeral UI integration looks solid.
The new header state, indicator, and renderer path are cohesive and tidy.
78-115: The concern about unsanitised output is not applicable.renderMrkdwnalready sanitises HTML usingDOMPurify.sanitize()at line 60 ofapps/ui/src/components/blockkit/context.tsbefore returning, so the output injected via{@html}is pre-sanitised and safe.Likely an incorrect or invalid review comment.
apps/ui/src/components/ModalOverlay.svelte (1)
93-188: Solid initialisation and radio wiring for new input types.The initial value extraction and
handleRadioChangeupdates keepformValuesaligned with the newly supported inputs.apps/ui/src/components/blockkit/elements/DatePicker.svelte (1)
48-116: Confirm flow and popover positioning are clean.The open/close handling, confirmation path, and outside-click/Escape behaviour are well structured.
apps/ui/src/components/blockkit/elements/OverflowMenu.svelte (1)
24-74: Overflow selection and confirm handling are nicely consistent.The pending-action flow and outside-click/Escape close logic read clearly and align with other interactive elements.
apps/ui/src/components/blockkit/blocks/SectionBlock.svelte (1)
31-155: Accessory handling and stacked layout look consistent.The inline/stacked rendering split reads cleanly and the action wiring for new element types is consistent.
apps/ui/src/components/blockkit/elements/TimePicker.svelte (1)
36-95: Time initialisation and confirm flow are tidy.Initialising
localTimeon open and deferring updates via confirm gives a consistent UX.apps/ui/src/components/blockkit/blocks/ActionsBlock.svelte (1)
32-99: Good coverage for new action elements and value mapping.The new branches and onChange handlers are consistent with the existing action dispatch flow.
apps/ui/src/components/blockkit/blocks/InputBlock.svelte (1)
84-179: InputBlock now covers the new input variants cleanly.The additional branches and the
onRadioChangewiring follow the established input/update pattern.apps/ui/src/components/Sidebar.svelte (2)
50-55: TheaddChannelfunction already has internal error handling (try/catch at lines 654–677) and safely returnsnullon failure rather than rejecting. The modal behaviour of staying open when creation fails is sensible and allows the user to retry. No unhandled promise rejection risk exists here.Likely an incorrect or invalid review comment.
244-248: No changes required—z-1000is supported by default in Tailwind v4.1.18.Tailwind CSS v4.1.18 includes
z-<number>utilities by default, soz-1000will compile directly toz-index: 1000;without requiring custom theme configuration. The context menu will render correctly with the current class.Likely an incorrect or invalid review comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ui/src/components/blockkit/elements/DateTimePicker.svelte`:
- Around line 120-149: The UI stays showing the uncommitted selection because
selectedDate/selectedTime are mutated before the confirm step; update the flow
so the component preserves the last committed value and only applies the draft
on confirm: snapshot the current committed state (e.g. lastCommittedTimestamp or
lastCommittedDate/lastCommittedTime) when opening or before calling handleDone,
use separate draft variables (draftSelectedDate/draftSelectedTime or a
draftTimestamp) for the in-dialog edits and have handleDone set pendingAction
but not overwrite the committed state, then implement handleConfirm to apply the
pendingAction and update the committed snapshot, and implement handleDeny to
restore selectedDate/selectedTime/displayText from the committed snapshot and
clear pendingAction/showingConfirm. Ensure references to handleDone,
handleConfirm, handleDeny, pendingAction, selectedDate, selectedTime and
displayText are updated accordingly.
In `@apps/ui/src/components/Message.svelte`:
- Around line 116-199: The datetimepicker branch in buildActionValue currently
does Number(value) unguarded; validate value first (use a truthiness check and
isNaN after conversion) before converting to a number and only set
selected_date_time when the value is present and a valid number; otherwise
return a safe empty/undefined selected_date_time (or omit it) to avoid 0/NaN
timestamps. Locate buildActionValue and the elementType === 'datetimepicker'
case to add the guard around Number(value).
In `@apps/ui/src/components/ModalOverlay.svelte`:
- Around line 204-220: The datetimepicker conversion loop uses block.block_id ||
'' causing a mismatch with extractInitialValues which stores missing block_id
keys as `block-${i}`; update the conversion logic in the loop that iterates
simulatorState.activeModal.view.blocks so it uses the same fallback
(block.block_id ?? `block-${i}`) — ensure you have access to the block index (i)
by switching to a classic for (let i = 0; i < ...; i++) or using
blocks.forEach((block, i) => ...) and then use that computed key when
reading/updating mergedValues[blockKey][el.action_id] so datetimepicker string
values are converted to numeric selected_date_time for blocks without block_id,
matching extractInitialValues.
In `@apps/ui/src/components/Sidebar.svelte`:
- Around line 71-81: handleDeleteChannel currently calls the async removeChannel
without awaiting and clears contextMenu immediately; change it to await
removeChannel and only clear or update UI state after a successful deletion.
Wrap the await in a try/catch around removeChannel (in handleDeleteChannel and
the other delete handler that uses removeChannel) and on error surface the
failure to the user (e.g., alert or show an error toast) instead of silently
clearing contextMenu; only set contextMenu = null and proceed with UI updates
when removeChannel resolves successfully. Ensure you follow the same pattern
used in handleCreateChannel for awaiting and error handling.
In `@packages/mrkdwn/src/markdown-to-mrkdwn.ts`:
- Around line 39-42: The image case in markdown-to-mrkdwn.ts returns
`<${t.href}|${t.text || 'image'}>` and uses t.text directly, which can break
mrkdwn when alt text contains `|` or `>`; update the image branch (case 'image',
token t/Tokens.Image) to sanitize the alt text before interpolation by escaping
or removing problematic characters (at minimum strip or replace `|` and `>` from
t.text, falling back to 'image' when empty) so the generated `<href|label>`
never contains unescaped `|` or `>` and cannot produce malformed mrkdwn links.
In `@packages/mrkdwn/src/mrkdwn-to-html.ts`:
- Around line 178-187: The blockquote detection in mrkdwn-to-html.ts is
inconsistent: the if condition tests both /^>\s?/ on escapeHtml(line) and
/^>\s?/ on raw line, but the subsequent while loop only checks raw lines,
causing mismatches for entity-encoded input; remove the unnecessary
escapeHtml(...) /^>/ check so the if and the while both consistently test raw
lines with /^>\s?/, ensuring quoteLines collection and the
formatInline(...replace(/^>\s?/, '')) logic behave correctly.
In `@packages/slack/src/server/state.ts`:
- Around line 108-123: addCustomChannel currently unconditionally overwrites
this.channels and this.messages; add a defensive guard at the start of the
addCustomChannel(id: string, name: string) method to check if
this.channels.has(id) (or this.messages.has(id)) and if so either return the
existing SlackChannel (or no-op) or throw an error to prevent accidental
overwrites; ensure the persistence.addChannel path is only invoked when creating
a new channel and keep the existing stateLogger.error call for persistence
failures.
---
Duplicate comments:
In `@apps/ui/src/components/blockkit/elements/DatePicker.svelte`:
- Around line 139-141: The popover uses a nonstandard Tailwind class "z-100"
that may not be emitted by the project's Tailwind config; update the div that
wraps Calendar.Root (the element rendered when isOpen is true) to use a
guaranteed class such as z-[100] or a configured z-index token (e.g., your theme
key) so the popover sits above other layers; search for the div containing the
class "z-100" in DatePicker.svelte and replace it with the chosen valid Tailwind
token.
In `@apps/ui/src/components/blockkit/elements/DateTimePicker.svelte`:
- Around line 185-187: The popover uses a Tailwind class "z-100" which may not
be in the project's Tailwind config and thus won't be emitted; update the class
on the element that renders the menu (the div inside the isOpen block in
DateTimePicker.svelte) to use a safe token such as z-[100] or your configured
zIndex token (e.g., z-[var(--z-popover)]), ensuring the menuStyle/overlay
retains correct stacking; search for the div with class "z-100" in
DateTimePicker.svelte and replace it accordingly and run the provided
verification commands to confirm Tailwind emits the utility.
In `@apps/ui/src/components/blockkit/elements/OverflowMenu.svelte`:
- Around line 106-109: The class "z-100" in the OverflowMenu Svelte component
may not exist in the Tailwind z-index scale; update the overflow menu markup in
OverflowMenu.svelte (the element using menuStyle and class="min-w-36
bg-slack-sidebar border border-white/20 rounded-md shadow-lg z-100") to use a
valid Tailwind token such as z-[100] or a configured theme token (e.g., z-menu)
instead, and if you prefer a theme value add/confirm the token in
tailwind.config.js under theme.extend.zIndex so Tailwind will emit it.
In `@apps/ui/src/components/blockkit/elements/TimePicker.svelte`:
- Around line 142-145: The popover div that renders when isOpen (the div using
menuStyle and currently has class "z-100") uses a nonstandard Tailwind token;
replace "z-100" with a guaranteed utility like "z-[100]" or swap to a configured
token (e.g., "z-popover") and update tailwind.config if using a custom key so
the class is emitted; modify the class string on the div that contains
menuStyle/isOpen accordingly and verify the new token appears in the built CSS.
Replace `collapsed` with `collapsible` and `expanded` fields to match the UI-side types and YAML config. Also use placeholder test script for showcase-bot since it has no test files.
… handling - DateTimePicker: separate committed/draft state so the trigger button only updates after confirm, and deny restores the previous value - Message: guard datetimepicker Number(value) against empty/NaN input - ModalOverlay: use block_id ?? `block-` fallback matching extractInitialValues so datetimepicker conversion works for blocks without an explicit block_id - Sidebar: await removeChannel before clearing UI state and alert on failure
…verwrites - Escape | and > in image alt text to prevent malformed mrkdwn links - Remove redundant escapeHtml check in blockquote detection for consistency - Guard addCustomChannel against overwriting existing channels and messages
Use border-separate with internal-only cell borders to avoid double borders at edges, and add self-start to prevent flex stretch in the parent column layout.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
apps/ui/src/components/blockkit/blocks/TableBlock.svelte (2)
65-65: Hardcoded rgba values assume a dark background.The border colours use
rgba(255, 255, 255, …)which works on dark backgrounds but may not suit a light theme. If multi-theme support is planned, consider extracting these to CSS custom properties alongside--color-slack-bg.Also applies to: 78-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/blockkit/blocks/TableBlock.svelte` at line 65, The CSS in TableBlock.svelte uses hardcoded rgba(255,255,255,...) for borders which assumes a dark background; replace those hardcoded values with a CSS custom property (e.g. --table-border-color) and reference it via var(--table-border-color, rgba(255,255,255,0.13)) in the border declarations so themes can override it; update all occurrences mentioned (the current border declaration and the other two occurrences noted) and add a sensible default value for --table-border-color alongside existing theme variables like --color-slack-bg to preserve current appearance if not overridden.
29-33: Consider adding a fallback for unrecognised cell types.Cells with a
typeother than'rich_text'or'raw_text'will render nothing silently. If this is intentional per the Slack API spec, this is fine. Otherwise, consider a fallback to aid debugging or future extensibility.💡 Optional fallback clause
{`#if` cell.type === 'rich_text'} <RichTextBlock block={cell as SlackRichTextBlock} /> {:else if cell.type === 'raw_text'} <span class="text-slack-text">{cell.text}</span> + {:else} + <span class="text-slack-text">[unsupported: {cell.type}]</span> {/if}Apply similarly to lines 45–49 for the
<tbody>cells.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/blockkit/blocks/TableBlock.svelte` around lines 29 - 33, Summary: TableBlock.svelte currently only handles cell.type === 'rich_text' and 'raw_text', so unknown types silently render nothing. Add a fallback branch after the existing conditional in the header and tbody cell render blocks to visibly render or log unrecognized types (e.g., a small span showing "Unknown cell type: {cell.type}" or a debug class), referencing the existing cell.type checks, RichTextBlock, SlackRichTextBlock and the raw_text branch so reviewers can find where to insert it; ensure the same fallback is applied to both the header and tbody cell conditionals.apps/ui/src/components/Sidebar.svelte (3)
71-85: Consider extracting shared delete logic.The confirmation prompt,
removeChannelcall, and error alert are duplicated betweenhandleDeleteChannel(context menu) and the inline delete button's onclick. Extracting a shared helper would reduce repetition.♻️ Proposed refactor
+ async function confirmAndDeleteChannel(channelId: string, channelName: string) { + if ( + window.confirm( + `Delete #${channelName}? All messages in this channel will be lost.` + ) + ) { + const success = await removeChannel(channelId) + if (!success) { + window.alert(`Failed to delete #${channelName}.`) + } + } + } + async function handleDeleteChannel() { if (!contextMenu) return const { channelId, channelName } = contextMenu - if ( - window.confirm( - `Delete #${channelName}? All messages in this channel will be lost.` - ) - ) { - const success = await removeChannel(channelId) - if (!success) { - window.alert(`Failed to delete #${channelName}.`) - } - } + await confirmAndDeleteChannel(channelId, channelName) contextMenu = null }Then update the inline button:
onclick={async (e) => { e.stopPropagation() - if ( - window.confirm( - `Delete #${channel.name}? All messages in this channel will be lost.` - ) - ) { - const success = await removeChannel(channel.id) - if (!success) { - window.alert(`Failed to delete #${channel.name}.`) - } - } + await confirmAndDeleteChannel(channel.id, channel.name) }}Also applies to: 170-182
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/Sidebar.svelte` around lines 71 - 85, Extract the duplicated deletion workflow (confirmation prompt, removeChannel call, error alert, and clearing UI state) into a single helper function (e.g., deleteChannelWithConfirm or confirmAndDeleteChannel) and call it from both handleDeleteChannel and the inline delete button onclick; the helper should accept channelId and channelName, show the same confirm dialog text, await removeChannel(channelId), show the same failure alert if needed, and ensure any UI state cleanup (clearing contextMenu or closing any menus) is performed by the caller or by an optional cleanup callback parameter so both handleDeleteChannel and the inline handler can reuse the logic without duplication.
252-254: Context menu may clip at viewport edges.When right-clicking near the bottom or right edge of the viewport, the menu could render partially off-screen. This is a minor UX polish item – a common fix is to detect available space and flip/shift positioning accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/Sidebar.svelte` around lines 252 - 254, The context menu positioned using contextMenu.x and contextMenu.y can render off-screen; update the logic that sets contextMenu (where the right-click handler or showContextMenu is implemented) to measure the menu's width/height (or use known max dimensions) and clamp or flip the coordinates so left = min(requestedX, window.innerWidth - menuWidth - margin) and top = min(requestedY, window.innerHeight - menuHeight - margin); if necessary compute actual menu dimensions via getBoundingClientRect after mount and adjust contextMenu.x/contextMenu.y before rendering so the <div> with style "left: {contextMenu.x}px; top: {contextMenu.y}px;" always stays inside the viewport.
250-262: Context menu lacks keyboard accessibility.The context menu cannot be dismissed with Escape, doesn't receive focus when opened, and doesn't support keyboard navigation. For an emulator UI this may be acceptable, but consider adding an
onkeydownhandler for Escape at minimum.♻️ Proposed enhancement
<svelte:window onclick={handleCloseContextMenu} /> +<svelte:window onkeydown={(e) => e.key === 'Escape' && handleCloseContextMenu()} />Or combined into a single handler if preferred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/Sidebar.svelte` around lines 250 - 262, Add keyboard accessibility to the context menu by making the menu container focusable and handling Escape: on the context menu div (the element rendered when contextMenu is truthy) add tabindex="0" and an on:keydown handler (or a combined handler) that checks for Escape and clears the contextMenu (call your existing close/clear routine or set contextMenu = null). Also bind the element (bind:this={menuEl}) and, where you open the menu (the code that sets contextMenu), call menuEl.focus() so it receives focus when opened; ensure the existing handleDeleteChannel button remains operable by Enter/Space (it's already a button).packages/slack/src/server/state.ts (1)
135-137: Extract preset channel IDs to shared constants in packages/slack/src/server.The hardcoded
'C_GENERAL'and'C_SHOWCASE'strings appear in at least four files within the server package (state.ts,index.ts,persistence.ts, andtypes.ts). Define these as exported constants to maintain consistency across the codebase and reduce the risk of sync errors if channel IDs change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/slack/src/server/state.ts` around lines 135 - 137, Extract the preset channel IDs into exported constants (e.g., PRESET_CHANNEL_GENERAL and PRESET_CHANNEL_SHOWCASE) in the server package and replace hardcoded strings in all occurrences; update removeCustomChannel (method removeCustomChannel in state.ts) to compare against these constants instead of 'C_GENERAL'/'C_SHOWCASE', and update other files that reference those literals (index.ts, persistence.ts, types.ts) to import and use the new constants so the IDs are defined once and shared across the server package.apps/ui/src/components/Message.svelte (1)
123-154: Consider consolidating duplicate option-lookup logic.The handling for
static_select,overflow, andradio_buttonsis nearly identical—each finds a matching option and returns the same payload shape. This could be consolidated into a shared helper.♻️ Suggested consolidation
+ const OPTION_LOOKUP_TYPES = ['static_select', 'overflow', 'radio_buttons'] + function buildActionValue( blockId: string, element: { type: string; action_id: string; options?: SlackOption[] }, value: string ) { const elementType = element.type - if (elementType === 'static_select' && element.options) { - const opt = element.options.find((o) => o.value === value) - return { - blockId, - elementType, - actionValue: opt - ? { selected_option: { text: opt.text, value: opt.value } } - : { value }, - } - } - - if (elementType === 'overflow' && element.options) { - const opt = element.options.find((o) => o.value === value) - return { - blockId, - elementType, - actionValue: opt - ? { selected_option: { text: opt.text, value: opt.value } } - : { value }, - } - } - - if (elementType === 'radio_buttons' && element.options) { - const opt = element.options.find((o) => o.value === value) + if (OPTION_LOOKUP_TYPES.includes(elementType) && element.options) { + const opt = element.options.find((o) => o.value === value) return { blockId, elementType, actionValue: opt ? { selected_option: { text: opt.text, value: opt.value } } : { value }, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/Message.svelte` around lines 123 - 154, Consolidate the duplicated option-lookup logic for elementType === 'static_select' / 'overflow' / 'radio_buttons' by extracting a small helper (e.g., resolveSelectedOption or similar) that takes element.options and value and returns the actionValue shape ({ selected_option: { text, value } } or { value }); then replace the three repeated blocks in Message.svelte with a single conditional branch that calls this helper and returns { blockId, elementType, actionValue }. Ensure the helper reads element.options and uses elementType/ value to locate the matching option.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/showcase-bot/package.jsonapps/showcase-bot/src/config/loader.tsapps/ui/src/components/Message.svelteapps/ui/src/components/ModalOverlay.svelteapps/ui/src/components/Sidebar.svelteapps/ui/src/components/blockkit/blocks/TableBlock.svelteapps/ui/src/components/blockkit/elements/DateTimePicker.sveltepackages/create-bot/templates/slack-bot/src/config/loader.ts.tmplpackages/mrkdwn/src/markdown-to-mrkdwn.test.tspackages/mrkdwn/src/markdown-to-mrkdwn.tspackages/mrkdwn/src/mrkdwn-to-html.tspackages/slack/src/server/state.ts
✅ Files skipped from review due to trivial changes (1)
- packages/mrkdwn/src/markdown-to-mrkdwn.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/showcase-bot/package.json
- apps/ui/src/components/blockkit/elements/DateTimePicker.svelte
- apps/showcase-bot/src/config/loader.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-14T21:36:07.933Z
Learnt from: tyom
Repo: tyom/botarium PR: 1
File: apps/ui/src/components/MessagePanel.svelte:10-23
Timestamp: 2026-01-14T21:36:07.933Z
Learning: In the client-only Botarium UI (Vite + Svelte), do not add or rely on SSR-specific guards (e.g., checks for sessionStorage) in components. Since the app runs only in Electron or the browser, code that accesses browser APIs will execute in a non-SSR environment, so SSR-unsafe checks are unnecessary and can be removed for maintainability.
Applied to files:
apps/ui/src/components/blockkit/blocks/TableBlock.svelteapps/ui/src/components/Sidebar.svelteapps/ui/src/components/ModalOverlay.svelteapps/ui/src/components/Message.svelte
🧬 Code graph analysis (1)
packages/mrkdwn/src/markdown-to-mrkdwn.ts (1)
packages/mrkdwn/src/index.ts (1)
markdownToMrkdwn(2-2)
🪛 ast-grep (0.40.5)
packages/mrkdwn/src/mrkdwn-to-html.ts
[warning] 225-225: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((${BR_RE.source}){2,}, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 229-229: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((${BR_RE.source})+(<(?:ul|ol|blockquote|pre)>), 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 233-233: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((<\\/(?:ul|ol|blockquote|pre)>)(${BR_RE.source})+, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 238-238: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((${BR_RE.source})+(\uE000CB\\d+\uE000), 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 242-242: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((\uE000CB\\d+\uE000)(${BR_RE.source})+, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 252-252: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((${BR_RE.source})+(<pre>), 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 253-253: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((<\\/pre>)(${BR_RE.source})+, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (24)
apps/ui/src/components/ModalOverlay.svelte (3)
93-131: LGTM! Comprehensive handling of additional input element types.The expanded
extractInitialValuesfunction correctly handlesnumber_input,email_text_input,url_text_input,radio_buttons,datepicker,timepicker, anddatetimepickerelements. The type assertions are necessary due to the element union types, and theradio_buttonshandling correctly mirrors thestatic_selectpattern by storing bothselected_optionandvalue.
176-188: LGTM! Clean implementation following established patterns.The
handleRadioChangefunction is consistent with the existinghandleCheckboxChangepattern and correctly stores bothselected_optionandvaluefor submission flexibility.
299-299: LGTM! Correctly wires the radio change handler to the renderer.apps/ui/src/components/blockkit/blocks/TableBlock.svelte (1)
1-20: LGTM!The script setup is clean. The
getAlignmentfunction correctly handles missingcolumn_settingswith optional chaining and a sensible default.apps/ui/src/components/Sidebar.svelte (1)
50-55: LGTM – Async handlers correctly implemented.Both
handleCreateChannelandhandleDeleteChannelnow properly await their respective async operations and handle failures appropriately:
- Modal only closes on successful channel creation
- Deletion surfaces errors via alert before clearing UI state
This addresses the concerns from previous reviews.
Also applies to: 71-85
packages/create-bot/templates/slack-bot/src/config/loader.ts.tmpl (1)
55-60: No backward-compatibility concern exists for this interface change. Repository search confirms thatcollapsed,collapsible, andexpandedproperties are not used anywhere in the codebase—neither in existing code, configs, nor examples. This appears to be a new interface definition without any legacy usage to break, so the proposed compatibility shim is unnecessary.Likely an incorrect or invalid review comment.
packages/slack/src/server/state.ts (10)
72-77: LGTM!Correct initialization order: loading channels before messages ensures channel references are valid when messages are loaded.
79-103: LGTM!The method correctly guards against overwriting existing in-memory channels and initializes message arrays for newly loaded channels.
108-129: LGTM!The defensive guard at lines 109-110 addresses the past review concern, preventing accidental overwrites of existing channels.
169-169: LGTM!Block Kit blocks are now correctly loaded from persistence, aligning with the PR's Block Kit rendering support.
269-269: LGTM!Consistent with the channel message loading - blocks are preserved for DM messages as well.
456-463: LGTM!Clean utility method for re-persisting modified messages, with consistent error handling.
486-494: LGTM!Correctly filters ephemeral messages from channel history before applying the limit.
496-503: LGTM!Consistent ephemeral filtering applied to thread messages.
535-556: LGTM!Efficient targeted deletion by channel and timestamp, with consistent persistence and logging patterns.
915-926: LGTM!Correctly filters for connected bots and uses optional chaining for safe command lookup.
apps/ui/src/components/Message.svelte (6)
189-198: Past review concern addressed: datetimepicker validation is now correct.The implementation now guards against empty strings (
value ? Number(value) : NaN) and validates with!isNaN(n)before using the numeric value, preventing misleading0orNaNtimestamps.
268-284: Past review concern addressed:context_actionsblock type is now handled.The function now correctly iterates over
context_actionsblocks and matches elements byaction_id.
291-306: LGTM!The action handler correctly resolves the block context and dispatches the action with all necessary parameters.
340-347: Clean implementation of ephemeral message indicator.The visual distinction for ephemeral messages with the Eye icon and "Only visible to you" text provides clear user feedback.
400-422: LGTM!The conditional rendering between
BlockKitRenderer(when blocks are present) and the fallbackformattedTextis correctly structured. TheonImagePreviewcallback is properly wired with all context parameters.
100-111: No XSS vulnerability present;renderMrkdwnsanitises output with DOMPurify.The
renderMrkdwnfunction inapps/ui/src/components/blockkit/context.tsapplies DOMPurify sanitisation after converting markdown to HTML. ThemrkdwnToHtmlfunction inpackages/mrkdwn/src/mrkdwn-to-html.tsproperly escapes all HTML entities before applying formatting. This double-sanitisation approach (HTML entity escaping + DOMPurify with a strict ALLOWED_TAGS allowlist) ensures safe rendering when used with{@html}.packages/mrkdwn/src/markdown-to-mrkdwn.ts (1)
71-111: List rendering with task support looks solid.The indentation and separation of text vs nested blocks are clear and consistent.
packages/mrkdwn/src/mrkdwn-to-html.ts (1)
150-255: Code-block placeholder pipeline is clean and cohesive.The phased handling of code blocks and line-break spans is tidy and easy to follow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ui/src/components/Message.svelte`:
- Around line 204-289: The resolveActionFromBlocks function is missing handling
for Slack input blocks, so actions from input elements fall through to the
'unknown' fallback; add a branch for block.type === 'input' that casts the block
to SlackInputBlock, reads its element property, checks for an action_id match,
and calls buildActionValue(blockId, el, value) (same param shape used in the
existing section/actions/context_actions branches) so input-derived actions
resolve correctly.
In `@packages/mrkdwn/src/markdown-to-mrkdwn.ts`:
- Around line 34-38: The link rendering in the 'link' case (Tokens.Link)
interpolates raw label text from renderTokens into `<${t.href}|${text}>`, which
breaks if the label contains '|' or stray '>' from non-text tokens; sanitize the
label before interpolation by replacing or escaping '|' (e.g. remove or
backslash-escape) and stripping any '>' characters produced by non-text tokens,
then use that sanitized label in the returned `<${t.href}|${sanitizedLabel}>`
string.
In `@packages/slack/src/server/state.ts`:
- Around line 825-832: getBotByToken currently claims to "Find a connected bot"
but is unused and does not enforce bot.status === 'connected'; either remove the
unused getBotByToken method entirely or update its implementation to match the
JSDoc by filtering only connected bots (e.g., in getBotByToken(token: string)
iterate over this.connectedBots.values() and skip any bot where bot.status !==
'connected') and keep the JSDoc consistent with the behavior; choose one
approach and apply it to the ConnectedBot lookup in state.ts (function name:
getBotByToken).
---
Duplicate comments:
In `@apps/ui/src/components/ModalOverlay.svelte`:
- Around line 204-223: The datetimepicker conversion logic in the for-loop over
simulatorState.activeModal.view.blocks correctly uses the same block_id fallback
pattern (`block.block_id ?? \`block-${i}\``) as extractInitialValues, so no code
change is needed; keep the current handling in the loop that reads entry from
mergedValues[blockId]?.[el.action_id] and replaces it with { selected_date_time:
Number(entry.value) } to ensure Slack-compatible numeric selected_date_time
values.
---
Nitpick comments:
In `@apps/ui/src/components/blockkit/blocks/TableBlock.svelte`:
- Line 65: The CSS in TableBlock.svelte uses hardcoded rgba(255,255,255,...) for
borders which assumes a dark background; replace those hardcoded values with a
CSS custom property (e.g. --table-border-color) and reference it via
var(--table-border-color, rgba(255,255,255,0.13)) in the border declarations so
themes can override it; update all occurrences mentioned (the current border
declaration and the other two occurrences noted) and add a sensible default
value for --table-border-color alongside existing theme variables like
--color-slack-bg to preserve current appearance if not overridden.
- Around line 29-33: Summary: TableBlock.svelte currently only handles cell.type
=== 'rich_text' and 'raw_text', so unknown types silently render nothing. Add a
fallback branch after the existing conditional in the header and tbody cell
render blocks to visibly render or log unrecognized types (e.g., a small span
showing "Unknown cell type: {cell.type}" or a debug class), referencing the
existing cell.type checks, RichTextBlock, SlackRichTextBlock and the raw_text
branch so reviewers can find where to insert it; ensure the same fallback is
applied to both the header and tbody cell conditionals.
In `@apps/ui/src/components/Message.svelte`:
- Around line 123-154: Consolidate the duplicated option-lookup logic for
elementType === 'static_select' / 'overflow' / 'radio_buttons' by extracting a
small helper (e.g., resolveSelectedOption or similar) that takes element.options
and value and returns the actionValue shape ({ selected_option: { text, value }
} or { value }); then replace the three repeated blocks in Message.svelte with a
single conditional branch that calls this helper and returns { blockId,
elementType, actionValue }. Ensure the helper reads element.options and uses
elementType/ value to locate the matching option.
In `@apps/ui/src/components/Sidebar.svelte`:
- Around line 71-85: Extract the duplicated deletion workflow (confirmation
prompt, removeChannel call, error alert, and clearing UI state) into a single
helper function (e.g., deleteChannelWithConfirm or confirmAndDeleteChannel) and
call it from both handleDeleteChannel and the inline delete button onclick; the
helper should accept channelId and channelName, show the same confirm dialog
text, await removeChannel(channelId), show the same failure alert if needed, and
ensure any UI state cleanup (clearing contextMenu or closing any menus) is
performed by the caller or by an optional cleanup callback parameter so both
handleDeleteChannel and the inline handler can reuse the logic without
duplication.
- Around line 252-254: The context menu positioned using contextMenu.x and
contextMenu.y can render off-screen; update the logic that sets contextMenu
(where the right-click handler or showContextMenu is implemented) to measure the
menu's width/height (or use known max dimensions) and clamp or flip the
coordinates so left = min(requestedX, window.innerWidth - menuWidth - margin)
and top = min(requestedY, window.innerHeight - menuHeight - margin); if
necessary compute actual menu dimensions via getBoundingClientRect after mount
and adjust contextMenu.x/contextMenu.y before rendering so the <div> with style
"left: {contextMenu.x}px; top: {contextMenu.y}px;" always stays inside the
viewport.
- Around line 250-262: Add keyboard accessibility to the context menu by making
the menu container focusable and handling Escape: on the context menu div (the
element rendered when contextMenu is truthy) add tabindex="0" and an on:keydown
handler (or a combined handler) that checks for Escape and clears the
contextMenu (call your existing close/clear routine or set contextMenu = null).
Also bind the element (bind:this={menuEl}) and, where you open the menu (the
code that sets contextMenu), call menuEl.focus() so it receives focus when
opened; ensure the existing handleDeleteChannel button remains operable by
Enter/Space (it's already a button).
In `@packages/slack/src/server/state.ts`:
- Around line 135-137: Extract the preset channel IDs into exported constants
(e.g., PRESET_CHANNEL_GENERAL and PRESET_CHANNEL_SHOWCASE) in the server package
and replace hardcoded strings in all occurrences; update removeCustomChannel
(method removeCustomChannel in state.ts) to compare against these constants
instead of 'C_GENERAL'/'C_SHOWCASE', and update other files that reference those
literals (index.ts, persistence.ts, types.ts) to import and use the new
constants so the IDs are defined once and shared across the server package.
Add missing input block branch in resolveActionFromBlocks so actions from input elements (selects, checkboxes, date pickers, etc.) resolve correctly instead of falling through to the unknown fallback. Sanitize link label text in markdown-to-mrkdwn to strip | and > characters, matching the existing image alt text sanitization.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
apps/showcase-bot/src/config/loader.ts (2)
64-75: Consider extracting shared types to avoid duplication.
SlashCommandandShortcutinterfaces are duplicated inapps/ui/src/lib/types.ts(lines 46-58). While acceptable for decoupled packages, extracting these to a shared types package would prevent drift and reduce maintenance burden.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/showcase-bot/src/config/loader.ts` around lines 64 - 75, SlashCommand and Shortcut interfaces are duplicated across packages; extract these shared types into a common module and import them instead of redeclaring to prevent drift. Create a shared types package or shared file (e.g., export interfaces SlashCommand and Shortcut from a new shared module) and update apps/showcase-bot/src/config/loader.ts and apps/ui/src/lib/types.ts to import { SlashCommand, Shortcut } from that shared module, removing the local declarations and ensuring any optional fields and literal unions (usage_hint?, type: 'message' | 'global') are preserved exactly.
96-97: Consider adding runtime validation for the config structure.The type assertion
as ConfigFileprovides compile-time safety but won't catch YAML structure mismatches at runtime. Since this is "the single source of truth for bot configuration", consider adding runtime validation (e.g., using Zod or a simple assertion function) to surface configuration errors early with clear messages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/showcase-bot/src/config/loader.ts` around lines 96 - 97, The export currently asserts rawConfig as ConfigFile at compile time but lacks runtime checks; add a validation step in loader.ts that builds a runtime schema (e.g., Zod schema or a small assertion function) for the ConfigFile shape and use it to parse/validate rawConfig (reference rawConfig and ConfigFile) before exporting; on validation failure throw or log a clear, descriptive error (including which keys are invalid) and only export the validated object (e.g., validatedConfig or config) to ensure YAML structure mismatches are caught early.packages/mrkdwn/src/markdown-to-mrkdwn.test.ts (1)
72-82: Consider adding a test for link text containing only special characters.The test suite covers the image fallback case (lines 78-82) when alt text consists only of
|>. For consistency with the suggested fix inmarkdown-to-mrkdwn.ts, consider adding a parallel test for links:it('falls back to URL when link text is only special characters', () => { expect(markdownToMrkdwn('[|>](https://example.com)')).toBe( '<https://example.com|https://example.com>' ) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mrkdwn/src/markdown-to-mrkdwn.test.ts` around lines 72 - 82, Add a unit test to mirror the image-alt fallback case but for links: in the markdown-to-mrkdwn.test.ts suite add an it block that calls markdownToMrkdwn('[|>](https://example.com)') and asserts it returns '<https://example.com|https://example.com>' so link text composed only of special characters falls back to the URL; reference the existing tests for images (the ones asserting '<https://example.com/img.png|image>' and '<https://example.com/img.png|abc>') to match style and placement.packages/slack/src/server/state.ts (1)
135-137: Consider extracting preset channel IDs to constants.The hardcoded strings
'C_GENERAL'and'C_SHOWCASE'appear across multiple files in the server package (types.ts,persistence.ts,index.ts, andstate.ts). Extracting these to named constants in a dedicated constants file (similar to thePRESET_CHANNELSconstant already defined in the UI layer) would reduce duplication and improve maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/slack/src/server/state.ts` around lines 135 - 137, Extract the hardcoded preset channel IDs into a shared constant and replace inline literals: create a constants export (e.g., PRESET_CHANNEL_IDS or PRESET_CHANNELS) that lists 'C_GENERAL' and 'C_SHOWCASE', export it from a new or existing constants module, then import and use this constant in types.ts, persistence.ts, index.ts and state.ts; update removeCustomChannel to check membership via PRESET_CHANNELS.includes(id) instead of comparing id === 'C_GENERAL' || id === 'C_SHOWCASE', and replace any other literal occurrences with the shared constant to remove duplication and centralize maintenance.apps/ui/src/components/Sidebar.svelte (2)
170-182: Consider extracting shared deletion logic.The inline handler duplicates the confirmation and error-handling logic from
handleDeleteChannel(lines 74–82). A shared helper would reduce duplication and ensure consistent messaging.♻️ Proposed refactor
+ async function deleteChannel(channelId: string, channelName: string) { + if ( + window.confirm( + `Delete #${channelName}? All messages in this channel will be lost.` + ) + ) { + const success = await removeChannel(channelId) + if (!success) { + window.alert(`Failed to delete #${channelName}.`) + } + } + } + async function handleDeleteChannel() { if (!contextMenu) return const { channelId, channelName } = contextMenu - if ( - window.confirm( - `Delete #${channelName}? All messages in this channel will be lost.` - ) - ) { - const success = await removeChannel(channelId) - if (!success) { - window.alert(`Failed to delete #${channelName}.`) - } - } + await deleteChannel(channelId, channelName) contextMenu = null }Then update the inline handler:
onclick={async (e) => { e.stopPropagation() - if ( - window.confirm( - `Delete #${channel.name}? All messages in this channel will be lost.` - ) - ) { - const success = await removeChannel(channel.id) - if (!success) { - window.alert(`Failed to delete #${channel.name}.`) - } - } + await deleteChannel(channel.id, channel.name) }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/Sidebar.svelte` around lines 170 - 182, Extract the duplicated confirmation and deletion logic into a single helper (e.g., confirmAndRemoveChannel) and have both the inline onclick handler and the existing handleDeleteChannel call that helper instead of duplicating code; the helper should accept channel.id and channel.name, perform the window.confirm prompt, call removeChannel(channel.id), and show the window.alert on failure so messaging and behavior are consistent across handleDeleteChannel and the onclick handler in Sidebar.svelte.
250-262: Optional: accessibility enhancements for the context menu.The context menu works correctly but could benefit from:
- ARIA attributes (
role="menu"/role="menuitem")- Keyboard support (Escape to close)
- Viewport bounds checking to prevent off-screen rendering
♻️ Minimal accessibility improvement
<div - class="fixed min-w-36 bg-(--main-bg) border border-(--border-color) rounded-md shadow-lg z-1000 overflow-hidden" + class="fixed min-w-36 bg-(--main-bg) border border-(--border-color) rounded-md shadow-lg z-1000 overflow-hidden" + role="menu" style="left: {contextMenu.x}px; top: {contextMenu.y}px;" + onkeydown={(e) => e.key === 'Escape' && handleCloseContextMenu()} > <button class="block w-full py-2 px-3 bg-transparent border-none text-left text-sm text-red-400 cursor-pointer transition-colors hover:bg-(--sidebar-hover)" + role="menuitem" onclick={handleDeleteChannel} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/Sidebar.svelte` around lines 250 - 262, The context menu markup should add ARIA roles and keyboard/viewport support: add role="menu" to the contextMenu container and role="menuitem" plus tabindex="0" to the Delete channel button (which currently uses handleDeleteChannel) so it is focusable/announced by screen readers; add a keydown handler (e.g., on window or the container) that listens for Escape to close the menu by clearing contextMenu and also handles Enter/Space to trigger handleDeleteChannel; before rendering, clamp contextMenu.x and contextMenu.y against the viewport width/height to prevent off-screen placement (adjust the inline style calculation where style="left: {contextMenu.x}px; top: {contextMenu.y}px;").
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/showcase-bot/package.jsonapps/showcase-bot/src/config/loader.tsapps/ui/src/components/Message.svelteapps/ui/src/components/ModalOverlay.svelteapps/ui/src/components/Sidebar.svelteapps/ui/src/components/blockkit/blocks/TableBlock.svelteapps/ui/src/components/blockkit/elements/DateTimePicker.sveltepackages/create-bot/templates/slack-bot/src/config/loader.ts.tmplpackages/mrkdwn/src/markdown-to-mrkdwn.test.tspackages/mrkdwn/src/markdown-to-mrkdwn.tspackages/mrkdwn/src/mrkdwn-to-html.tspackages/slack/src/server/state.ts
✅ Files skipped from review due to trivial changes (1)
- apps/showcase-bot/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/ui/src/components/blockkit/blocks/TableBlock.svelte
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-14T21:36:07.933Z
Learnt from: tyom
Repo: tyom/botarium PR: 1
File: apps/ui/src/components/MessagePanel.svelte:10-23
Timestamp: 2026-01-14T21:36:07.933Z
Learning: In the client-only Botarium UI (Vite + Svelte), do not add or rely on SSR-specific guards (e.g., checks for sessionStorage) in components. Since the app runs only in Electron or the browser, code that accesses browser APIs will execute in a non-SSR environment, so SSR-unsafe checks are unnecessary and can be removed for maintainability.
Applied to files:
apps/ui/src/components/ModalOverlay.svelteapps/ui/src/components/blockkit/elements/DateTimePicker.svelteapps/ui/src/components/Message.svelteapps/ui/src/components/Sidebar.svelte
🧬 Code graph analysis (3)
apps/showcase-bot/src/config/loader.ts (1)
apps/ui/src/lib/types.ts (2)
SlashCommand(47-51)Shortcut(54-59)
packages/mrkdwn/src/markdown-to-mrkdwn.test.ts (1)
packages/mrkdwn/src/index.ts (1)
markdownToMrkdwn(2-2)
packages/slack/src/server/state.ts (1)
packages/slack/src/server/types.ts (3)
SlackChannel(21-27)SlackMessage(52-67)ConnectedBot(351-357)
🪛 ast-grep (0.40.5)
packages/mrkdwn/src/mrkdwn-to-html.ts
[warning] 225-225: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((${BR_RE.source}){2,}, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 229-229: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((${BR_RE.source})+(<(?:ul|ol|blockquote|pre)>), 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 233-233: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((<\\/(?:ul|ol|blockquote|pre)>)(${BR_RE.source})+, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 238-238: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((${BR_RE.source})+(\uE000CB\\d+\uE000), 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 242-242: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((\uE000CB\\d+\uE000)(${BR_RE.source})+, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 252-252: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((${BR_RE.source})+(<pre>), 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 253-253: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((<\\/pre>)(${BR_RE.source})+, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (41)
packages/create-bot/templates/slack-bot/src/config/loader.ts.tmpl (1)
55-61: LGTM!The
GroupDefinitioninterface changes align with the UI's expected schema (collapsibleandexpandedinstead ofcollapsed). This ensures consistency between the template and the showcase-bot implementation.apps/showcase-bot/src/config/loader.ts (4)
1-11: LGTM!Clear documentation and appropriate import structure.
13-53: LGTM!Well-structured type definitions for UI generation with appropriate use of
unknownfor dynamic values.
55-61: LGTM!The
GroupDefinitioninterface now correctly usescollapsibleandexpandedfields, aligning with the UI's expected schema.
99-110: LGTM!Bot name derivation with sensible fallback and well-structured
slackConfigexport combining app info with Slack configuration.apps/ui/src/components/Message.svelte (7)
1-54: LGTM!Imports and Props interface are well-structured. The extended
onImagePreviewsignature with optional contextual parameters (userName, isBot, timestamp, channelName) provides good flexibility for downstream consumers.
190-199: LGTM - Datetimepicker validation properly implemented.The validation now correctly guards against empty strings and invalid values by checking truthiness before conversion and using
!isNaN(n)to ensure a valid timestamp. Returnsundefinedfor invalid values rather than0orNaN.
309-324: LGTM!Clean delegation pattern - resolves action metadata from blocks and dispatches via
sendMessageBlockAction.
358-365: LGTM - Clean ephemeral message indicator.Good UX pattern with the Eye icon and explanatory text. The conditional rendering based on
isEphemeralis straightforward.
418-432: LGTM - BlockKitRenderer integration.Clean wiring of the block action and image preview handlers. The
onImagePreviewcallback correctly passes the additional contextual information (displayName, isBot, timestamp, channelName) to support the enhanced preview modal.
561-569: LGTM!Scoped styles for mention highlighting are appropriate.
101-112: No XSS concern —renderMrkdwnincludes internal DOMPurify sanitisation.The switch to
renderMrkdwndoes not remove sanitisation. The function (line 60 ofapps/ui/src/components/blockkit/context.ts) explicitly appliesDOMPurify.sanitize(html, SANITIZE_CONFIG)to the mrkdwn-parsed output before returning it. The sanitisation step is preserved, merely internalised within the function rather than being a separate call at the usage site.Likely an incorrect or invalid review comment.
packages/mrkdwn/src/markdown-to-mrkdwn.ts (4)
1-13: LGTM!Module structure and
escapeMrkdwnhelper are well-designed. The helper correctly escapes the three characters that have special meaning in Slack mrkdwn (&,<,>).
71-111: LGTM!The list rendering logic correctly handles task lists with checkboxes, ordered/unordered distinction, and nested lists via indentation. The separation of text parts from block parts ensures proper nesting.
113-133: LGTM!Table rendering as a code block with aligned columns is a reasonable approach for Slack mrkdwn, which lacks native table support.
180-194: LGTM!Public API is clean with proper empty input handling and trimmed output.
packages/mrkdwn/src/markdown-to-mrkdwn.test.ts (1)
1-125: LGTM!Comprehensive test coverage for the markdown-to-mrkdwn converter, including edge cases for sanitisation, nested structures, and various block types.
packages/mrkdwn/src/mrkdwn-to-html.ts (4)
220-254: Static analysis warnings are false positives – no action required.The ast-grep warnings about ReDoS from variable input at lines 225-253 are false positives.
BR_REis derived from the constantBR(line 161), which is a static string literal defined in this module, not user input. The regex construction is safe.
80-88: LGTM!Links correctly include
rel="noopener noreferrer"to prevent reverse-tabnabbing, and the non-greedy.*?patterns properly handle URLs with query parameters containing&.
108-124: LGTM!The lookbehind/lookahead patterns for bold, italic, and strikethrough correctly avoid matching within words (e.g.,
some_variable_namewon't trigger italic formatting). This matches Slack's mrkdwn behaviour.
147-256: LGTM!The four-phase processing pipeline is well-structured: code block extraction, line-by-line block processing, cleanup, and placeholder restoration. The use of private-use Unicode characters (
\uE000,\uE001) as placeholder delimiters is a sound approach to prevent collisions with user content.packages/slack/src/server/state.ts (8)
72-77: LGTM!Sensible reordering to ensure channels are available before loading messages that reference them.
79-103: LGTM!Clean implementation with appropriate guards to prevent overwriting preset channels. The message array initialisation at lines 97-99 correctly handles cases where channels exist but have no messages yet.
108-129: LGTM!The defensive guard at lines 109-110 correctly prevents accidental overwrites, addressing the concern from the previous review. The early return pattern is clean and the implementation properly matches the
SlackChannelinterface.
169-169: LGTM!Block persistence support correctly added to both channel message loading (line 169) and DM message loading (line 269), enabling Block Kit content to survive restarts.
456-463: LGTM!Clean utility method for persisting in-place message modifications. The fire-and-forget pattern is consistent with other persistence operations in this file.
487-502: LGTM!Correct implementation of ephemeral message filtering. Ephemeral messages should not appear in regular channel or thread history queries, matching Slack's actual behaviour.
535-556: LGTM!Efficient targeted deletion method appropriate for the
chat.deleteAPI use case. The implementation correctly handles missing channel/message cases and maintains consistent persistence behaviour.
915-926: LGTM!Correct implementation for routing commands to the appropriate connected bot. The
status === 'connected'filter ensures commands are only dispatched to active bots.apps/ui/src/components/Sidebar.svelte (4)
1-9: LGTM!Imports are correctly organised with appropriate type-only import for
Channel.
50-55: LGTM!Async handling is correct: the channel is awaited and the modal only closes on successful creation, keeping it open for retry on failure.
71-85: LGTM!Deletion properly awaits
removeChanneland surfaces failures to the user. The destructuring at line 73 captures values synchronously before any async operations, ensuring correctness even with the window click handler.
243-248: LGTM!Modal wiring is clean with appropriate callbacks for close and create actions.
apps/ui/src/components/ModalOverlay.svelte (4)
10-17: Type import aligns with radio support.Bringing in
SlackRadioButtonsElementis consistent with the new radio handling below.
93-131: Initial value coverage looks consistent.The added branches for number/email/url/radio/date/time/datetime inputs should keep modal state pre-populated correctly.
176-188: Radio change handler cleanly normalises state.Storing both
selected_optionandvaluekeeps the submission payload aligned with other inputs.
291-300: Radio callbacks are wired through the renderer.Passing
onRadioChangethrough keeps modal state in sync with the renderer’s radio components.apps/ui/src/components/blockkit/elements/DateTimePicker.svelte (4)
155-167: Confirm/deny flow now correctly preserves committed state.The separation of
committedDate/committedTimefromselectedDate/selectedTime, combined withhandleDenyrestoring draft from committed values, properly addresses the earlier concern about cancelled confirmations leaving the display out of sync.
1-333: Overall implementation is well-structured.The component cleanly separates concerns: state management with committed/draft pattern, positioning logic, calendar/time selection, and confirmation flow. Accessibility attributes (
aria-label,aria-expanded) are present on the trigger button.
127-153: The timestamp calculation is correct for Slack'sdatetimepickerelement. Slack's datetimepicker accepts only UNIX timestamps and renders the selected time according to the viewer's locale. The implementation correctly converts the user's locally-selected date and time to a UNIX timestamp viagetTime(), which represents an absolute moment in time. Users in different locales will each see times in their local timezone, and the timestamps sent to Slack will be correct. No changes required.Likely an incorrect or invalid review comment.
210-272: API usage verified as correct.The Calendar.Root and TimeField.Root components correctly use bits-ui v2.15.4 API. All props match the documented specifications:
- Calendar.Root:
type="single",weekdayFormat="short", andweekStartsOn={1}are all valid.- TimeField.Root:
hourCycle={24}andgranularity="minute"are all valid.No changes required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ui/src/components/blockkit/elements/DateTimePicker.svelte`:
- Around line 169-183: When the picker is closed via handleClickOutside or
handleKeyDown (Escape) the draft state selectedDate/selectedTime must be reset
to the committed values so reopening doesn't show uncommitted edits; update both
handleClickOutside and handleKeyDown to, before setting isOpen = false, reset
the draft variables (selectedDate and selectedTime) to the component's committed
state (e.g., the props/variables that hold the last committed date/time or call
the existing commit/rollback helper if present) and ensure containerRef usage
remains the same.
In `@apps/ui/src/components/ModalOverlay.svelte`:
- Around line 204-223: The datetimepicker handling loop leaves entries like {
value: '' } when users leave the picker empty; update the logic in the
block-processing loop (where you inspect simulatorState.activeModal.view.blocks
and work with mergedValues, SlackInputBlock, el.action_id and block.block_id) to
explicitly normalize empty strings into the Slack-expected shape—either remove
the entry or set mergedValues[blockId][el.action_id] = { selected_date_time:
undefined } when entry.value === ''—and keep the existing numeric conversion for
non-empty values.
---
Duplicate comments:
In `@apps/ui/src/components/Message.svelte`:
- Around line 269-284: The input block branch duplicates the action-resolution
pattern but uses an inline cast; change the handling to narrowly type the
element and pass a proper Slack action element type to buildActionValue: in the
input block branch (SlackInputBlock / inputBlock.element) keep the 'action_id'
check but replace the ad-hoc cast with the concrete action-element type (e.g.,
SlackActionElement or a named interface) and call buildActionValue(blockId,
element, value) so types are correct and match how
section/actions/context_actions dispatch are resolved (references:
SlackInputBlock, inputBlock.element, actionId, blockId, buildActionValue).
In `@packages/mrkdwn/src/markdown-to-mrkdwn.ts`:
- Around line 34-37: In the 'link' case (Tokens.Link) after computing text =
renderTokens(...).replace(/[|>]/g, ''), add the same empty-after-sanitisation
fallback used in the image branch: if text is an empty string, set it to a
fallback like 'link' before returning `<${t.href}|${text}>`, so you never emit
`<url|>`.
In `@packages/slack/src/server/state.ts`:
- Around line 825-832: The current status-agnostic behavior in getBotByToken
(which searches connectedBots for a match on `xoxb-${bot.id}`) is intentional
per prior discussion, so do not change the lookup logic; instead, add a brief
clarifying comment above the getBotByToken method noting that it intentionally
ignores bot status (connected/active flags) to prevent future maintainers from
modifying it and reference the ConnectedBot type in that comment for context.
---
Nitpick comments:
In `@apps/showcase-bot/src/config/loader.ts`:
- Around line 64-75: SlashCommand and Shortcut interfaces are duplicated across
packages; extract these shared types into a common module and import them
instead of redeclaring to prevent drift. Create a shared types package or shared
file (e.g., export interfaces SlashCommand and Shortcut from a new shared
module) and update apps/showcase-bot/src/config/loader.ts and
apps/ui/src/lib/types.ts to import { SlashCommand, Shortcut } from that shared
module, removing the local declarations and ensuring any optional fields and
literal unions (usage_hint?, type: 'message' | 'global') are preserved exactly.
- Around line 96-97: The export currently asserts rawConfig as ConfigFile at
compile time but lacks runtime checks; add a validation step in loader.ts that
builds a runtime schema (e.g., Zod schema or a small assertion function) for the
ConfigFile shape and use it to parse/validate rawConfig (reference rawConfig and
ConfigFile) before exporting; on validation failure throw or log a clear,
descriptive error (including which keys are invalid) and only export the
validated object (e.g., validatedConfig or config) to ensure YAML structure
mismatches are caught early.
In `@apps/ui/src/components/Sidebar.svelte`:
- Around line 170-182: Extract the duplicated confirmation and deletion logic
into a single helper (e.g., confirmAndRemoveChannel) and have both the inline
onclick handler and the existing handleDeleteChannel call that helper instead of
duplicating code; the helper should accept channel.id and channel.name, perform
the window.confirm prompt, call removeChannel(channel.id), and show the
window.alert on failure so messaging and behavior are consistent across
handleDeleteChannel and the onclick handler in Sidebar.svelte.
- Around line 250-262: The context menu markup should add ARIA roles and
keyboard/viewport support: add role="menu" to the contextMenu container and
role="menuitem" plus tabindex="0" to the Delete channel button (which currently
uses handleDeleteChannel) so it is focusable/announced by screen readers; add a
keydown handler (e.g., on window or the container) that listens for Escape to
close the menu by clearing contextMenu and also handles Enter/Space to trigger
handleDeleteChannel; before rendering, clamp contextMenu.x and contextMenu.y
against the viewport width/height to prevent off-screen placement (adjust the
inline style calculation where style="left: {contextMenu.x}px; top:
{contextMenu.y}px;").
In `@packages/mrkdwn/src/markdown-to-mrkdwn.test.ts`:
- Around line 72-82: Add a unit test to mirror the image-alt fallback case but
for links: in the markdown-to-mrkdwn.test.ts suite add an it block that calls
markdownToMrkdwn('[|>](https://example.com)') and asserts it returns
'<https://example.com|https://example.com>' so link text composed only of
special characters falls back to the URL; reference the existing tests for
images (the ones asserting '<https://example.com/img.png|image>' and
'<https://example.com/img.png|abc>') to match style and placement.
In `@packages/slack/src/server/state.ts`:
- Around line 135-137: Extract the hardcoded preset channel IDs into a shared
constant and replace inline literals: create a constants export (e.g.,
PRESET_CHANNEL_IDS or PRESET_CHANNELS) that lists 'C_GENERAL' and 'C_SHOWCASE',
export it from a new or existing constants module, then import and use this
constant in types.ts, persistence.ts, index.ts and state.ts; update
removeCustomChannel to check membership via PRESET_CHANNELS.includes(id) instead
of comparing id === 'C_GENERAL' || id === 'C_SHOWCASE', and replace any other
literal occurrences with the shared constant to remove duplication and
centralize maintenance.
…values
Roll back draft date/time to committed values when the picker is closed
via click-outside or Escape, preventing stale edits from persisting on
reopen. Normalize empty datetimepicker submissions to
{ selected_date_time: undefined } instead of leaving { value: '' }.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CLAUDE.mdapps/ui/src/components/ModalOverlay.svelteapps/ui/src/components/blockkit/elements/DateTimePicker.svelte
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/ui/src/components/blockkit/elements/DateTimePicker.svelte
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-14T21:36:07.933Z
Learnt from: tyom
Repo: tyom/botarium PR: 1
File: apps/ui/src/components/MessagePanel.svelte:10-23
Timestamp: 2026-01-14T21:36:07.933Z
Learning: In the client-only Botarium UI (Vite + Svelte), do not add or rely on SSR-specific guards (e.g., checks for sessionStorage) in components. Since the app runs only in Electron or the browser, code that accesses browser APIs will execute in a non-SSR environment, so SSR-unsafe checks are unnecessary and can be removed for maintainability.
Applied to files:
apps/ui/src/components/ModalOverlay.svelte
🪛 LanguageTool
CLAUDE.md
[uncategorized] ~36-~36: A comma may be missing after the conjunctive/linking adverb ‘Currently’.
Context: ...mulator, defaultPort, envVarName`). Currently only Slack is implemented but the desig...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[uncategorized] ~36-~36: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...e`). Currently only Slack is implemented but the design supports adding other platfo...
(COMMA_COMPOUND_SENTENCE_2)
[duplication] ~48-~48: Possible typo: you repeated a word.
Context: ...low 1. User types in UI → HTTP POST to emulator /api/simulator/user-message 2. Emulator creates Slack event → sends via WebSock...
(ENGLISH_WORD_REPEAT_RULE)
[duplication] ~49-~49: Possible typo: you repeated a word.
Context: ...ia WebSocket (Socket Mode) to connected bot 3. Bot processes event → calls Slack Web API e...
(ENGLISH_WORD_REPEAT_RULE)
[duplication] ~50-~50: Possible typo: you repeated a word.
Context: ...oints (e.g., chat.postMessage) on the emulator 4. Emulator broadcasts SSE event → UI updates react...
(ENGLISH_WORD_REPEAT_RULE)
🔇 Additional comments (5)
apps/ui/src/components/ModalOverlay.svelte (5)
15-15: LGTM!Import added for
SlackRadioButtonsElementis properly utilised in theextractInitialValuesfunction.
93-131: LGTM!The extended initial value extraction handles the additional element types consistently:
- String-based inputs (
number_input,email_text_input,url_text_input,datepicker,timepicker) default to empty string when no initial value is present.radio_buttonscorrectly captures bothselected_optionandvalue.datetimepickerstores the numeric timestamp as a string, which is correctly converted back toselected_date_timeduring submission.The
'property' in elementguards provide adequate runtime safety before type assertions.
176-188: LGTM!The
handleRadioChangefunction follows the established pattern fromhandleCheckboxChangeand correctly stores bothselected_optionandvalueto match the Slack API payload structure.
203-223: LGTM!The datetimepicker conversion logic correctly addresses the previously raised concerns:
- The
block_idfallback usesblock.block_id ??block-${i}`` matching the pattern inextractInitialValues.- Empty values are properly normalised to
{ selected_date_time: undefined }via the ternaryentry.value ? Number(entry.value) : undefined.
299-299: LGTM!The
onRadioChangeprop is correctly wired toBlockKitRenderer, completing the radio button interaction flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Line 36: Update the sentence in CLAUDE.md describing packages/core to add the
missing commas for clarity: change "Currently only Slack is implemented but the
design supports adding other platforms." to "Currently, only Slack is
implemented, but the design supports adding other platforms." This edit is in
the description of **`packages/core`** (the section mentioning the
BotariumPlugin interface and symbols like createEmulator, defaultPort,
envVarName).
dividers, and context blocks
confirm dialogs, and typed inputs
@botarium/mrkdwnpackage for bidirectional mrkdwn-to-HTML and Markdown-to-mrkdwn conversionSummary by CodeRabbit
New Features
Improvements
Documentation
Tests