Convert pageKey field to dropdown in Change Page block #1196
Conversation
WalkthroughThis change introduces comprehensive placeholder support for dropdown and select components across the UI layer, integrating asynchronous select dropdowns into the builder's text field editor. It adds placeholder value injection logic, styling, and backend metadata to support predefined page keys as dropdown options. Changes
Sequence DiagramsequenceDiagram
participant BuilderFieldsText
participant WdsSelect
participant Option Storage
Note over BuilderFieldsText,WdsSelect: Initialization Phase
BuilderFieldsText->>BuilderFieldsText: Compute options (Record<string, string>)
BuilderFieldsText->>BuilderFieldsText: Derive selectOptions (array of {value, label})
BuilderFieldsText->>BuilderFieldsText: Evaluate shouldUseDropdown
alt shouldUseDropdown = true
BuilderFieldsText->>WdsSelect: Mount async component<br/>(options passed as prop)
WdsSelect->>WdsSelect: Normalize options to selectOptions
WdsSelect->>WdsSelect: Compute shouldInjectPlaceholder
WdsSelect->>WdsSelect: Prepend PLACEHOLDER_VALUE if needed
else shouldUseDropdown = false
BuilderFieldsText->>BuilderFieldsText: Render BuilderTemplateInput
end
Note over BuilderFieldsText,WdsSelect: User Selection Phase
User->>WdsSelect: Select option or placeholder
WdsSelect->>WdsSelect: Update currentValueArray
WdsSelect->>WdsSelect: Compute selectedOptions & currentLabel
WdsSelect->>WdsSelect: Apply isPlaceholderSelected styling
WdsSelect->>BuilderFieldsText: Emit selection (via selectValue setter)
BuilderFieldsText->>BuilderFieldsText: Update fieldViewModel.content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
sheikhuzairhussain
changed the title
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
HackerOne Code Security Review
🟢 Scan Complete: 1 Issue(s)
Here's how the code changes were interpreted and info about the tools used for scanning.
📖 Summary of Changes
The changes focus on enhancing dropdown and select components across the UI, introducing new placeholder styling, and improving page key selection functionality. Modifications include adding WdsSelect component, refining dropdown menu items, and extending template configuration to support predefined page key options. The updates aim to improve user interface interactions and form field management.| File | Summary |
|---|---|
| src/ui/src/builder/settings/BuilderFieldsText.vue | Added WdsSelect component for dropdown functionality, introduced new predefined option function for page keys, and implemented computed properties for select options and dropdown visibility based on template type and available options. |
| src/ui/src/wds/WdsDropdownMenu.vue | The changes include adding an optional isPlaceholder property to the WdsDropdownMenuOption type and adding a border to the .WdsDropdownMenu class in the CSS. The hasNoResults section was also removed from the template. |
| src/ui/src/wds/WdsDropdownMenuItem.vue | Added a new CSS class 'WdsDropdownMenuItem--placeholder' to the template and style section, introducing styling for placeholder menu items with specific color variations for label and detail text. |
| src/ui/src/wds/WdsSelect.vue | The changes include adding a new required prop, enhancing placeholder handling, improving option selection logic, and adding a new CSS class for placeholder styling with more robust option management and validation. |
| src/writer/blocks/changepage.py | Added "options": "pageKeys" to the "pageKey" field in the AbstractTemplate's writer configuration, which suggests providing a predefined list of page key options for the field. |
ℹ️ Issues Detected
NOTE: These may not require action!
Below are unvalidated results from the Analysis Tools that ran during the latest scan for transparency. We investigate each of these for accuracy and relevance before surfacing them as a potential problem.
How will I know if something is a problem?
When validation completes, any concerns that warrant attention prior to merge will be posted as inline comments. These will show up in 2 ways:
- Expert review (most cases): Issues will be posted by experts who manually reviewed and validated them. These are real HackerOne engineers (not bots) reviewing through an integrated IDE-like tool. You can communicate with them like any other reviewer. They'll stay assigned and get notified with commit & comment updates.
- Automatically: In cases where our validation checks have highest confidence the problem is legitimate and urgent. These will include a description of contextual reasoning why & actionable next steps.
| File & Line | Issue |
|---|---|
src/ui/src/builder/settings/BuilderFieldsText.vue Line 177 |
The code introduces a new dropdown feature for page keys without proper validation. The selectValue computed property allows setting any value, including potentially malicious ones, directly into the component's field value. The transformation function doesn't sanitize or validate this input before using it. |
🧰 Analysis tools
- [ ✅ ] HackerOne AI Code Analysis
- [ ✅ ] HackerOne AI Code Validation
- [ ✅ ] semgrep
- [ ✅ ] bandit
⏱️ Latest scan covered changes up to commit 2a1791e (latest)
There was a problem hiding this comment.
✅ Jim K reviewed all the included code changes and associated automation findings and determined that there were no immediately actionable security flaws. Note that they will continue to be notified of any new commits or comments and follow up as needed throughout the duration of this pull request's lifecycle.
Reviewed with ❤️ by PullRequest
…pdown support and placeholder options - Added WdsSelect component with dropdown functionality and placeholder handling. - Updated BuilderFieldsText to conditionally render WdsSelect based on input type. - Introduced isPlaceholder property in WdsDropdownMenu for better UI representation. - Enhanced styling for placeholder items in WdsDropdownMenuItem. - Updated changepage.py to include pageKeys as options for the target page key.
sheikhuzairhussain
force-pushed
the
feat/page-key-dropdown
branch
from
6cbd25c to
2a1791e
Compare
sheikhuzairhussain
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/ui/src/wds/WdsDropdownMenuItem.vue (1)
48-53: Placeholder styling on dropdown items is coherent; minor variant interactionThe new
WdsDropdownMenuItem--placeholderclass and corresponding muted colors make placeholder entries clearly distinct, and theoption.isPlaceholderhook lines up with the updated option type.One nuance: if an option were ever marked both
variant: 'danger'andisPlaceholder, the placeholder text color (defined later) would override the danger color. If that combination is not expected, this is fine; otherwise you might want a more specific rule for that case.Also applies to: 157-162
src/ui/src/wds/WdsSelect.vue (2)
108-109: Placeholder injection viaselectOptionsis reasonable; watch for custom placeholder edge casesUsing
PLACEHOLDER_VALUE = ""plusshouldInjectPlaceholderto prepend an auto-generated placeholder option (withisPlaceholder: true) only for non-required, single-select cases is a good default.One edge case to be aware of: if a caller passes an option with
isPlaceholder: truebut a non-emptyvalue, your injection logic will remove that option (filter((option) => !option.isPlaceholder)) when adding the empty-string placeholder. If you ever want to support custom-valued placeholders, you may want to instead treat anyoption.isPlaceholderoroption.value === PLACEHOLDER_VALUEas “a placeholder already exists” and skip adding a new one, without filtering those options out.Also applies to: 136-172
174-179: Unknown-option handling is solid; minor naming nitThe flow from
currentValueArray→findOption→selectedOptionsandhasUnknowOptionSelectedlooks robust:
- Unknown values are preserved as synthetic options
{ value, label: String(value) }, so the user still sees what’s in the model.currentLabelfalls back to raw model values when unknowns are present, which makes the state debuggable instead of silently blank.Only nit:
hasUnknowOptionSelectedis slightly misspelled; if you touch this code again, consider renaming tohasUnknownOptionSelectedfor clarity.Also applies to: 181-193, 196-214
src/ui/src/builder/settings/BuilderFieldsText.vue (3)
4-11: Conditional WdsSelect usage for text fields is wired correctly; consider hiding icons explicitlyThe switch to render
WdsSelectwhenshouldUseDropdownis true and fall back toBuilderTemplateInputotherwise looks clean, and passing:options="selectOptions"plus:placeholder="defaultValue"matches the new option/placeholder semantics inWdsSelect.You’re currently passing
default-icon=""without settinghide-icons. BecauseWdsSelectuses??when choosing the icon, this will result in an empty string being forwarded toWdsIconrather than suppressing the icon entirely. If you don’t want any icon for this builder control, it would be clearer to pass:hide-icons="true"instead of an emptydefault-icon.Also applies to: 13-24
50-55:pageKeyshelper correctly backs the new ChangePage dropdown; consider typing the accumulatorThe new async
WdsSelectimport and thepageKeyshelper together complete the loop from the Python metadata"options": "pageKeys"to concrete dropdown entries:
pageKeyswalks root-level components of type"page", pulls theircontent.keyas the option value, and chooses a human-friendly label viatitle→name→key→id.- This gives a stable mapping of page keys to labels for the Change Page block.
For TypeScript friendliness, you might want to explicitly type the reducer accumulator as
Record<string, string>(similar to theoptionscomputed) instead of relying on{}inference:- return pages.reduce((acc, page) => { + return pages.reduce((acc: Record<string, string>, page) => { // ... return acc; - }, {}); + }, {} as Record<string, string>);This would avoid any implicit
any/indexing issues onacc.Also applies to: 96-113
137-150:optionscomputed is clearer now; a small guard could make it more robustTyping
optionsascomputed<Record<string, string>>and delegating throughfield.options(function, string key, or object) is a good cleanup.If
field.optionsis ever a string that doesn’t correspond to a key inpredefinedOptionFns,predefinedOptionFns?.[field.options](...)will throw at runtime. You could harden this slightly by guarding the lookup:- if (typeof field.options === "string") { - return predefinedOptionFns?.[field.options](wf, props.componentId); - } + if (typeof field.options === "string") { + const fn = predefinedOptionFns?.[field.options as keyof typeof predefinedOptionFns]; + return fn ? fn(wf, props.componentId) : {}; + }This would fail closed (empty options) instead of crashing if someone mistypes the options key.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/ui/src/builder/settings/BuilderFieldsText.vue(5 hunks)src/ui/src/wds/WdsDropdownMenu.vue(1 hunks)src/ui/src/wds/WdsDropdownMenuItem.vue(2 hunks)src/ui/src/wds/WdsSelect.vue(8 hunks)src/writer/blocks/changepage.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-28T14:01:53.761Z
Learnt from: sheikhuzairhussain
Repo: writer/writer-framework PR: 1147
File: src/ui/src/components/core/input/CoreSelectInput.vue:170-172
Timestamp: 2025-10-28T14:01:53.761Z
Learning: In CoreSelectInput.vue (src/ui/src/components/core/input/CoreSelectInput.vue), the watcher on fields.allowMultiSelect intentionally resets formValue to defaultValue when the multi-select mode toggles, rather than preserving the user's current selection.
Applied to files:
src/ui/src/builder/settings/BuilderFieldsText.vuesrc/ui/src/wds/WdsSelect.vue
🔇 Additional comments (5)
src/writer/blocks/changepage.py (1)
19-24: HookingpageKeyup topageKeysoptions looks correctWiring
"options": "pageKeys"into the ChangePage metadata matches the newpredefinedOptionFns.pageKeyshelper inBuilderFieldsText.vue, so the builder can render a dropdown for valid page keys without affecting runtime behavior inrun(). I don’t see issues here.src/ui/src/wds/WdsDropdownMenu.vue (1)
87-101: ExtendingWdsDropdownMenuOptionwithisPlaceholderis safeAdding the optional
isPlaceholder?: booleanflag cleanly extends the option contract and aligns with the new styling inWdsDropdownMenuItem.vue. Existing callers remain valid because the field is optional.src/ui/src/wds/WdsSelect.vue (2)
5-8: Trigger placeholder behavior is consistent across single and multi selectThe trigger’s placeholder handling (
WdsSelect__trigger--placeholder, multi-select placeholder text, andcurrentLabel ?? placeholderLabelfor single-select) is internally consistent and lines up with the computedplaceholderLabelandisPlaceholderSelected. This should give a clear, uniform UX for “no selection yet” states.Also applies to: 40-44, 52-53, 341-343
226-265:ensureValidSelectioneagerly normalizes invalid values; verify that’s desirable for all callersThe
watch([...], ensureValidSelection, { immediate: true })plusensureValidSelectionlogic ensures:
- For single-select +
required, an invalid or empty current value is coerced to the first available option.- For single-select + non-required, any value not present in
selectOptionsis coerced toPLACEHOLDER_VALUE(the injected placeholder).This is a sensible default for most form-style selects, but it does mean the component will mutate the bound v-model on mount or when options change if the current value is not in the options (e.g., stale config, removed page, etc.).
If some usages need to preserve unknown values until the user consciously changes them, you may want to gate this behavior (e.g. behind a prop) or narrow it to “no value provided” cases only.
src/ui/src/builder/settings/BuilderFieldsText.vue (1)
152-162: Select options / value mapping is sound; be aware of how unknown values are normalizedThe pipeline
options: computed<Record<string, string>>selectOptions: computed<Option[]>fromObject.entriesshouldUseDropdowngating ontype === "template" && selectOptions.length > 0selectValueas the v-model bridge betweenWdsSelectandfieldViewModelis well-structured and typed.
One behavioral detail:
selectValue’s getter returnsundefinedwheneverinputValueisn’t one of the currentselectOptions. Combined withWdsSelect’sensureValidSelection, this means that opening the editor while the underlying field holds a value that’s no longer in the options (e.g. a deleted page key) will cause the model to be normalized to the placeholder (or first option, ifrequiredis set onWdsSelect). If that’s the intended behavior for stale configs, you’re good; otherwise you may want to preserve and surface unknown values instead of normalizing them.Also applies to: 169-179
|
Due to inactivity, PullRequest has cancelled this review job. You can reactivate the code review job from the PullRequest dashboard. |
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.